[APPS-2792] Add: runtime network/subprocess guard for local execution - #484
[APPS-2792] Add: runtime network/subprocess guard for local execution#484tyffical wants to merge 7 commits into
Conversation
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🔗 Commit SHA: e93c1e7 | Docs | View more details | Give us feedback! |
dev-server.test.ts and local-execution.test.ts (build-plugins#480/#484) each defined their own near-identical LoadModule resolver double. Factor the common resolve-or-throw logic into moduleResolverFor in the shared mocks helper so both can build on it instead of duplicating it.
dev-server.test.ts and local-execution.test.ts (build-plugins#480/#484) each defined their own near-identical LoadModule resolver double. Factor the common resolve-or-throw logic into moduleResolverFor in the shared mocks helper so both can build on it instead of duplicating it.
e48778e to
2ad1ce9
Compare
2ad1ce9 to
b69e5f2
Compare
dev-server.test.ts and local-execution.test.ts (build-plugins#480/#484) each defined their own near-identical LoadModule resolver double. Factor the common resolve-or-throw logic into moduleResolverFor in the shared mocks helper so both can build on it instead of duplicating it.
b69e5f2 to
63539eb
Compare
dev-server.test.ts and local-execution.test.ts (build-plugins#480/#484) each defined their own near-identical LoadModule resolver double. Factor the common resolve-or-throw logic into moduleResolverFor in the shared mocks helper so both can build on it instead of duplicating it.
63539eb to
1b11592
Compare
dev-server.test.ts and local-execution.test.ts (build-plugins#480/#484) each defined their own near-identical LoadModule resolver double. Factor the common resolve-or-throw logic into moduleResolverFor in the shared mocks helper so both can build on it instead of duplicating it.
1b11592 to
9d591ec
Compare
There was a problem hiding this comment.
Pull request overview
Friend, this PR adds runtime restrictions for in-process local backend execution.
Changes:
- Adds process-wide network and subprocess guards.
- Exempts
$.Actionscalls and resets guards after timeouts. - Adds unit and integration coverage for guard behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
network-guard.ts |
Implements blocking, exemptions, and reset logic. |
network-guard.test.ts |
Tests guard state and concurrency. |
local-execution.ts |
Integrates guards into local execution. |
local-execution.test.ts |
Tests execution-path guard behavior. |
Suppressed comments (1)
packages/plugins/apps/src/vite/network-guard.ts:165
runAllowedcan run after its enclosing blocked scope has already been reset. In the existing abandoned-execution scenario, a late call through a captured$.Actionsproxy increments from zero, the guarded action rejects, and thisapplyPatches()then leaves the whole process blocked even though norunBlockedis active; the test'safterEach(forceReset)masks the leak. Track whether this call actually entered from an active blocked scope and only reapply in that case, or perform the abandoned check before enteringrunAllowed.
if (currentGeneration === myGeneration) {
allowDepth -= 1;
if (allowDepth === 0) {
applyPatches();
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Blocks net/fetch/child_process for the duration of the customer's | ||
| // function call only — loadModule and the registration calls above | ||
| // (both Vite's own transform pipeline, no network) run unguarded. | ||
| // $.Actions calls made from inside fn are exempted via `runAllowed` | ||
| // in `makeActionsProxy`. See network-guard.ts. | ||
| const result = await runBlocked(() => fn(...args)); |
There was a problem hiding this comment.
Tried fixing this by moving runBlocked to wrap loadModule itself, but reverted it — Vite's real ssrLoadModule pipeline needs genuine network/fs access internally to transform and resolve the customer's module, and blocking that broke the real dev-server integration test outright (not just theoretical: a real @datadog/apps-backend import through a real Vite server started returning 500). Documented as an accepted residual gap in network-guard.ts's own doc comment, alongside the existing native-addon and dgram gaps, rather than engineered around further for now. Leaving unresolved to keep it tracked.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b36f55e0bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // (both Vite's own transform pipeline, no network) run unguarded. | ||
| // $.Actions calls made from inside fn are exempted via `runAllowed` | ||
| // in `makeActionsProxy`. See network-guard.ts. | ||
| const result = await runBlocked(() => fn(...args)); |
There was a problem hiding this comment.
When the timeout fires while loadModule(...) is still pending, forceReset() clears the guard and releases the queue, but the abandoned run() continues and enters this runBlocked call once loading completes. If a newer execution is already blocked, the stale call overwrites its saved snapshots and generation; when either call finishes, the process can be left permanently patched, and a stale function that hangs leaves the same result. Check abandoned before invoking the function/entering a new guard scope.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same finding as the copilot review comment on this line — see my reply there. Traced through carefully and it doesn't currently reproduce; added a regression test proving it (2c6aa37d).
| // (both Vite's own transform pipeline, no network) run unguarded. | ||
| // $.Actions calls made from inside fn are exempted via `runAllowed` | ||
| // in `makeActionsProxy`. See network-guard.ts. | ||
| const result = await runBlocked(() => fn(...args)); |
There was a problem hiding this comment.
Traced through carefully — this doesn't currently reproduce. The if (abandoned) throw check immediately before entering runWithScopedEnv/runBlocked runs synchronously with no await in between, so there's no window for the timeout's setTimeout callback to interleave and flip abandoned to true after the check but before the guards are entered. Added a regression test (2c6aa37d) that specifically simulates this: A's own loadModule for its main function body resolves late, after B (a newer execution) has already started and is still running its own body inside runBlocked/runWithScopedEnv — A correctly bails via the abandoned check without ever touching the guards, leaving B's state untouched.
4fd59b3 to
cb79658
Compare
cb79658 to
affa16b
Compare
affa16b to
fbad040
Compare
fbad040 to
085afa1
Compare
085afa1 to
64e9169
Compare
64e9169 to
aff9ee7
Compare
aff9ee7 to
a2525b5
Compare
a2525b5 to
2e2c8eb
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d1607af92
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Runs the customer function's own call (not loadModule or the action-catalog/ apps-backend registrations, which need real network/fs access) with global net/fetch/child_process access blocked, hardened against every bypass a review pass found: a malicious result's toJSON()/getter still running under the block, and $.Actions/action-catalog calls made from inside the function exempted via runAllowed so the guard doesn't also block trusted API calls.
…ppers
Node's native exec/execFile carry a util.promisify.custom implementation
resolving {stdout, stderr}; wrapping them into a plain function silently
drops that symbol, so promisify() falls back to its generic single-value
behavior instead. This broke @dd/tools' own execute() helper — used by,
among other things, the injection plugin's test suite to run built bundles
and assert on their captured stdout — whenever it ran in the same Jest
worker as a test that imports network-guard.ts, since the property patch
is process-wide. Bisected via git bisect against a CI failure entirely
outside this package (packages/plugins/injection), confirmed by reproducing
the exact symptom in isolation before landing the fix.
Reimplements the {stdout, stderr} contract through the guarded callback
path rather than reusing Node's original custom implementation, which
would call straight into the native binding and bypass the block guard.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Neither goes through net.Socket or fetch, so both fully escaped the existing guard — a backend function could exfiltrate data over UDP or open a raw WebSocket undetected. Also drops a comment's dangling cross-reference to env-guard.ts, which doesn't exist on this branch.
…f, sync ESM child_process bindings
installGuardedProperty's setter treated any incoming value as a new real
implementation, including a previously-read guard object handed back by the
common "capture original, mock, restore" pattern -- corrupting real into a
guard that calls itself forever on the next unblocked invocation. Now tracks
which real value was active when each guard was built, and restores that
value when a tracked guard is written back instead of the guard itself.
Also calls syncBuiltinESMExports() after installing the child_process guards,
since Node keeps ESM named bindings (e.g. `import { spawn } from
'node:child_process'`) as separate references that stay bound to the
original native functions otherwise -- verified with a standalone
`node --input-type=module` script since Jest's CJS transform can't reproduce
the real ESM-binding divergence this fixes.
Corrected a misleading comment: forceReset() invalidates the epoch so a
stale runAllowed call becomes a no-op, it does not restore real network
access -- the block itself stays enforced via AsyncLocalStorage regardless.
…DNS resolvers net.Server.prototype.listen and dgram.Socket.prototype.bind were unguarded inbound entry points, letting a dependency open a real listening socket during blocked execution; both are now guarded the same way the existing outbound dgram send/connect methods are, via the shared guardNetworkMethod wrapper (renamed from guardDgramMethod, since it was always generic). worker_threads.Worker construction is blocked via a Proxy construct trap mirroring guardWebSocket's shape -- a Worker gets a fresh V8 realm that doesn't inherit any of this file's monkeypatches or the AsyncLocalStorage block context, so guarding its internals from the parent thread isn't possible; blocking construction itself is the only enforceable boundary. dns.resolve*()/dns.promises.resolve*() (and their Resolver-class equivalents, across all 4 surfaces: dns, dns.promises, dns.Resolver.prototype, dns.promises.Resolver.prototype) bypass net.Socket/dgram.Socket entirely via Node's native c-ares channel, so neither existing guard ever observed those calls. The promise-returning surfaces (dns.promises.*, dns.promises.Resolver .prototype.*) reject rather than throw synchronously when blocked, matching guardFetch's existing reject-not-throw contract and the native dns.promises API shape -- a caller chaining .catch() outside an async wrapper would otherwise see an uncaught synchronous throw instead of a normal rejection. dns.lookup remains intentionally unguarded, per the existing Out of Scope decision.
…a shared mutable variable The direct-reassignment restore idiom (const original = x; x = mock; ...; x = original) was already handled by tracking which real value was active when each guard was built. A wrapper closure over the previous guard (x = (...a) => previous(...a)) is a distinct pattern some mocking approaches use instead, and it still recursed: the previous guard's own getReal() read the same shared mutable variable the new guard had just set to the wrapper, so unblocked calls looped between the two guards until the stack overflowed. Each guard now closes over its own snapshot of the real delegate taken at build time, so a stale guard reference always resolves to what was real when it was built, regardless of what the property currently holds.
…e essential WHY
Several doc comments and single-line // comments in this stack had grown to
280-1100+ characters or 8+ lines by stacking every contributing justification
instead of keeping the one that matters. Also extracts a repeated
`$.Actions.${pathParts.join('.')}` computation in makeActionsProxy's apply
trap into a single local, since it was now computed three times.
Motivation
LOCAL_EXECUTION_LOAD_SUFFIXmechanism andmoduleResolverFortest double.fetch/XMLHttpRequest/WebSocket/EventSourcereferences in the customer's own.backend.tsfile.net/http/fetchinternally — that static scan never inspectsnode_modules.--allow-netunder any code path (confirmed inwf-actions-worker'sdeno.ts); this PR closes the equivalent gap at the module level.Architecture
network-guard.tspatches every module-level entry point a customer's code (or its dependencies) could use to reach the network directly:net.Socket.connect,fetch,dgramnet.Server.listen,dgram.Socket.bindchild_process's spawn/exec familyworker_threads.Workerdns/dns.promises, allResolvervariantsMost of these are process-wide singletons, so
installGuardedPropertyinstalls a permanentObject.definePropertygetter/setter on each rather than sandboxing the customer's module — there's no process boundary to sandbox with.A worker thread is the exception: its fresh V8 realm inherits none of this file's monkeypatches, so blocking
Workerconstruction itself is the only enforceable boundary.Whether a call is blocked is never a shared boolean or counter — it's read fresh, per call, from two
AsyncLocalStorageinstances scoped to the calling async chain:runAllowed(a)/runAllowed(b)call gets its ownAsyncLocalStoragecontext, two concurrent$.Actionscalls never contend over shared exemption state the way a ref-counter would — one call's exit can't prematurely re-block a sibling still in flight.runScriptLocally'sPromise.race([run(), timeout])abandons rather than cancels the loser, soblockedContext.run(true, fn)never reaches its natural exit and that call stays blocked for as long as it runs.forceReset()doesn't touch this — it only invalidates a separate epoch counter (execution-epoch.ts, shared withlocal-execution.ts's registration-poisoning andenv-guard.ts's env scoping) thatrunAllowed's fast path uses to detect whether anyrunBlockedscope is active anywhere, so a new execution'srunAllowedisn't mistaken for still running inside an old, abandoned scope.afterEachinnetwork-guard.test.tscallsforceReset()to reset shared epoch state between tests, not to un-patch anything — the getter/setter installation is permanent.Changes
18 changes across 5 files
hasActiveScope()andforceInvalidate()toEpochGuard, sorunAllowed/forceResetcan detect and invalidate an activerunBlockedscope.installGuardedPropertyinstalls a permanent getter/setter on each guarded Node global, rebuilding the exposed wrapper object on every external write.runBlocked(fn)/runAllowed(fn)scope block/allow state per async chain viaAsyncLocalStorage, not a shared flag.forceReset()invalidates the shared epoch counter so a new execution'srunAllowedisn't mistaken for an old, abandoned scope.runScriptLocallywraps the customer's function call — including its result'sassertJsonSerializablecheck — inrunBlocked, not the precedingloadModule/registration calls.runScriptLocally's timeout handler now also callsforceReset(), so an abandoned execution's laterrunAllowedcall is a no-op instead of a false exemption.makeActionsProxyand the action-catalog dispatcher JSON round-tripinputsvia newserializeActionInputsbefore enteringrunAllowed, closing atoJSON()/getter exfiltration path.dgram(UDP) and the nativeWebSocketglobal, both previously unguarded.net.Server.prototype.listen,dgram.Socket.prototype.bind) via the renamed genericguardNetworkMethod.worker_threads.Workerconstruction via a Proxy construct trap, mirroringguardWebSocket's shape.dns,dns.promises,dns.Resolver.prototype,dns.promises.Resolver.prototype), rejecting rather than throwing on the promise-returning ones.executeScriptLocally: raw network/subprocess calls are rejected, and concurrent$.Actionscalls still succeed.exec/execFile's guarded wrappers re-attach autil.promisify.customimplementation resolving{stdout, stderr}, matching Node's native contract.installGuardedProperty's restore path tracks which real value was active when each guard was built, so the "capture original, mock, restore" idiom doesn't corrupt it.installGuardedPropertyhandles a non-objectmakeGuardresult (e.g.guardWebSocketreturningundefined) without throwing.ChildProcess.prototype.spawndirectly, since the standalonespawn/exec/etc. functions are thin wrappers a dependency could call through to bypass them.QA Instructions
yarn test:unit packages/plugins/apps/src/vite/network-guard.test.ts # Expected: Test Suites: 1 passed / Tests: 38 passed ✅ VERIFIEDyarn test:unit packages/plugins/apps/src/vite/local-execution.test.ts # Expected: Test Suites: 1 passed / Tests: 88 passed ✅ VERIFIEDyarn workspace @dd/apps-plugin run typecheck # Expected: no output, clean exit ✅ VERIFIEDcollectCoverageFromCLI flag doesn't produce a usable per-file report for either new/changed file in this environment (pre-existing tooling quirk — the coverage table only ever lists_jesthelper files regardless of the glob passed). Manually verified every branch innetwork-guard.tsis exercised by at least one test.net/fetchin a customer function while still letting a real$.Actionscall through, end-to-end.yarn workspace @dd/tests run test:unit --testPathPatterns="apps"), not just this file in isolation — see the QA guide's "Testing a new process-wide guard" section for why that distinction matters for any guard touching a real Node global.Blast Radius
AsyncLocalStorage; the dev server's own network use (before/after that call, and anything unrelated tolocal-execution.ts) is never touched.Out of Scope / Follow-ups
2 items deferred
netstack entirelydns.lookupinterceptionDocumentation