[APPS-2792] Add: runtime network/subprocess guard for local execution - #484
Conversation
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🔗 Commit SHA: 42db481 | 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.
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.
Skip guard entry after an execution already timed out
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.
ea84a47 to
4fd59b3
Compare
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 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: 120fcec681
ℹ️ 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".
There was a problem hiding this comment.
🟡 Changes recommended
Critical guard bypasses and asynchronous error-contract issues remain, alongside scope and serialization validation defects.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
packages/plugins/apps/src/vite/network-guard.ts:722
- This checks whether any epoch is active, not whether the blocked scope on the caller's
AsyncLocalStoragechain is still current. If scope A is abandoned and scope B starts before A's continuation callsrunAllowed, B makeshasActiveScope()true and A is incorrectly granted an allowance. Associate the epoch/scope token with the blocked async context and only allow when that specific token is current.
// Exempts `fn`'s own async chain (not siblings) from an active `runBlocked` scope; no-ops if that scope was already abandoned.
export async function runAllowed<T>(fn: () => Promise<T>): Promise<T> {
if (!blockEpoch.hasActiveScope()) {
return fn();
}
return allowedContext.run(true, fn);
- Files reviewed: 5/5 changed files
- Comments generated: 4
- Review effort level: Balanced
| installGuardedProperty<typeof net.Socket.prototype.connect>( | ||
| net.Socket.prototype, | ||
| 'connect', | ||
| (getReal) => makeGuardWrapper(getReal, NETWORK_BLOCKED_MESSAGE, 'throw'), | ||
| ); |
There was a problem hiding this comment.
This is an intentional, already-documented decision from an earlier round — see the comment directly above net.Socket.prototype.connect's guard install: it runs inside the customer function's own async call stack, where a synchronous throw is safely caught, unlike the detached-callback scenario the other guards were redesigned around. Leaving as-is.
…g local execution Introduces network-guard.ts's process-wide runtime guard (net, fetch, dgram, dns, child_process, worker_threads), scoped per-async-chain via AsyncLocalStorage rather than a global toggle, plus $.Actions/return-value JSON round-trip validation in local-execution.ts to close a toJSON()/getter exfiltration path.
Motivation
LOCAL_EXECUTION_LOAD_SUFFIX/moduleResolverFortest double..backend.ts, but can't see the same calls made from inside a third-party dependency innode_modules.--allow-net) — this closes the equivalent gap at the module level.Architecture
network-guard.tspatches every module-level entry point to network/subprocess/worker-thread/DNS access (net,fetch,dgram,child_process,worker_threads.Worker,dns) via a permanent getter/setter on each Node global.'error'event, an error-first callback, or a result object with.errorset — never a uniform synchronous throw, since several of these APIs are used via idiomatic patterns (server.on('error', cb); server.listen(port)) that a throw would break.net.Socket.write/endare guarded too, since a reused keep-alive socket never callsconnect()again; the guarddestroy()s the socket instead of throwing, and exemptsprocess.stdout/stderrby an identity captured once at module load so a customer function can't reassign them to exfiltrate data past the block.AsyncLocalStorage, not a shared flag:runAllowed()call gets its ownAsyncLocalStoragecontext, so concurrent$.Actionscalls never contend over shared exemption state — 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, so the blocked scope never naturally exits.runBlockedexposes aBlockedScopeHandle(abandonIfCurrent(), backed by an epoch counter inexecution-epoch.ts) so a caller's own timeout can abandon exactly its own scope without wrongly un-exempting a different, still-active execution.forceReset()is a test-only reset between test cases, not used in production.Changes
44 changes across 10 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; kept as a test-only reset between test cases, not called from production code.runScriptLocallywraps the customer's function call — including its result'sassertJsonSerializablecheck — inrunBlocked, not the precedingloadModule/registration calls.makeActionsProxyand the action-catalog dispatcher JSON round-tripinputsviaserializeActionInputsbefore enteringrunAllowed, closing atoJSON()/getter exfiltration path. It sharesassertJsonSerializable's stricter validation (via a newassertJsonRoundTrippablehelper) instead of a bare round-trip, so a Map/Set/NaN/symbol-keyed input fails loudly instead of reaching the destination action silently corrupted.assertJsonRoundTrippablesilently omits anundefined-valued plain-object property (return value or$.Actionsinput) instead of throwing, matching what production's ownJSON.stringifyalready does — an array element'sundefined(silently converted tonullbyJSON.stringify, not dropped) is still caught.makeActionsProxyand the action-catalog dispatcher's identical validate → serialize →runAllowedsequence is extracted into a sharedinvokeActionhelper.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.net.Socket.prototype.write/end(viadestroy(), not a throw) to close a keep-alive-reuse bypass of connect()-only guarding.write/endare configurable specifically under Jest, so Jest's ownspyOn/mockRestore(used elsewhere in this repo's test suite, againstprocess.stderr/stdoutwhen CI pipes them into realnet.Socketinstances) doesn't collide with the guard.guardConnect, and separatelyguardWebSocket/guardEventSource, and separatelyguardNetworkMethod/guardNetworkPromiseMethod/guardSubprocess, are each deduped into one shared factory per group.guardSocketWrite/guardSocketEndshare a newguardSocketOpfactory the same way, parameterized by the blocked-path return value.signalBlockedSocketOpfixes two bugs in the write/end guard:destroy()with no'error'listener crashed the whole process (Node's default behavior for an unlistened'error'event), and any write/end completion callback was silently dropped instead of invoked.process.stdout/process.stderrare exempted from the write/end block by identity captured once at module load (trustedStdout/trustedStderr), not the live, reassignableprocess.stdout/process.stderrgetters — a customer function repointingprocess.stdoutto an attacker-controlled socket would otherwise inherit the exemption and exfiltrate data past the block.runBlockedexposes aBlockedScopeHandle(abandonIfCurrent()) to its caller instead of relying on the module-levelforceReset();local-execution.ts's two timeout paths abandon only their own scope, fixing a race where an abandoned scope's timeout could wrongly un-exempt a different, still-active execution's in-flight$.Actionscall.guardFetchdeduped into the existingguardNetworkPromiseMethod/makeGuardWrapperfactory, since fetch's blocked-path contract (reject, not throw) was already exactly what that factory provides.getSharedContext's per-keyAsyncLocalStorageentries onnetare now installed via non-configurableObject.definePropertyinstead of a plain assignment, closing a bypass where any code holding anetreference could swap in a fake store and silently disable every guard in this file at once (isCurrentlyBlocked()is their shared gate).guardWorkernow reusesguardConstructibleGlobal(generalized with an optional blocked-message parameter) instead of its own copy, fixing a crash when the realWorkerglobal isundefinedand deduplicating two identical construct-trap implementations.signalBlockedSocketOp'slistenerCount('error') > 0check is deferred viaprocess.nextTick, so attaching the'error'listener immediately afterwrite()/end()(not just before) still triggers the blocked signal instead of being silently swallowed.net.Server.listen,dgram.Socket.bind/connect/send, and the callback-styledns.resolve*surfaces are now guarded via newguardBindMethod/guardCallbackMethodfactories that signal failure the same way each real API does (async'error'event or error-first callback), replacing the oldguardNetworkMethod, which always threw synchronously and could crash a caller relying on the idiomatic async pattern.local-execution.tsnow lazily importsnetwork-guard.ts(memoized dynamicimport()), confining its process-wide monkeypatch installation to when local execution actually runs instead of to any bundler that transitively imports the apps plugin.loadCustomerModuleEntryawaits the network guard module before invokingloadModule, so a customer module's top-level code can no longer run beforenetwork-guard.tscapturestrustedStdout/trustedStderr, closing a window where a reassignedprocess.stdoutcould permanently bypass the write-blocking guard; its doc comment documents this ordering alongside the remaining, accepted top-level-code gap.runScriptLocally's two timeout paths share a newfailWithTimeouthelper instead of duplicating the conclude/abandon/reject sequence.spawn/fork/exec/execFile/ChildProcess.prototype.spawnare now guarded via newguardSpawnFactory/guardExecFactory/guardChildProcessSpawnMethodfactories matching each API's real contract (async'error'event, error-first callback, or a synchronous integer return), replacing the old always-throwguardSubprocessfor these five;execSync/execFileSyncare unchanged since they genuinely throw synchronously.spawnSyncis guarded via a newguardSpawnSyncResult, returning aSpawnSyncReturns-shaped object with.errorset instead of throwing, matching its real never-throws contract.createBlockedChildProcessStubfabricates aChildProcess-shaped stub (real inertstdout/stderr/stdinstreams,send()/disconnect()/kill()) for the blocked-path return value ofspawn/fork/exec/execFile, so a caller touching those fields doesn't hit an unrelatedTypeError.emitAsyncErrorIfListened/invokeCallbackArgextract the repeated "emiterrorif listened"/"invoke the trailing error-first callback" logic shared acrossguardBindMethod,guardCallbackMethod,signalBlockedSocketOp, and the new child_process guards.installGuardedProperty's setter now detects when the assignment receiver differs from the originally-guarded target (e.g. an instance-levelsomeSocket.write = mockon the sharednet.Socket.prototypeguard) and installs a per-instance override instead of corrupting the one delegate every other instance's guard still calls through.assertJsonRoundTrippable's symbol-keyed-property check moved from a pre-JSON.stringifypass over the raw value into theJSON.stringifyreplacer itself, so it also catches a symbol key a customtoJSON()introduces only in its return value.serializeActionInputsnow verifies the JSON-round-tripped$.Actionsinputs value is still a plain object (rejecting a top-leveltoJSON()that turns it into a string/array/null, and a raw array passed as inputs), instead of trusting a bare type assertion.JSON.stringifybehavior (an array element converts tonull; an object property is dropped), instead of one message wrongly claiming both are dropped.trustedFetchcaptures the realglobalThis.fetchat module load, beforeinstallGuardedPropertypatches it, so a customer function reassigningglobalThis.fetchto an attacker-controlled wrapper can no longer intercept it.RequestOpts.fetchImpllets a caller supply a trustedfetchreference instead ofdoRequestreadingglobalThis.fetchfresh at call time; defaults to the global lookup so existing mocking (nock,jest.spyOn(global, 'fetch')) is unaffected.getAuthenticatedRequest()lazily importsnetwork-guard.tsand passes itstrustedFetchasfetchImpl, closing the gap where the dev server's authenticated$.Actions/runtime-context request could be redirected to a customer-installedfetchwrapper and leak the request's auth headers.QA Instructions
yarn test:unit packages/plugins/apps/src/vite/network-guard.test.ts # Expected: Test Suites: 1 passed / Tests: 70 passed ✅ VERIFIEDyarn test:unit packages/plugins/apps/src/vite/local-execution.test.ts # Expected: Test Suites: 1 passed / Tests: 98 passed ✅ VERIFIEDyarn test:unit packages/core/src/helpers/request.test.ts packages/plugins/apps/src/auth.test.ts # Expected: Test Suites: 2 passed / Tests: 20 passed (15 in request.test.ts, 5 in auth.test.ts) ✅ VERIFIEDcollectCoverageFromgives no usable per-file report in this repo — manually confirmed every branch innetwork-guard.tsis exercised by at least one test.net/fetchis blocked in a customer function while a real$.Actionscall still succeeds).Blast Radius
AsyncLocalStorage; the dev server's own network use is never touched.Out of Scope / Follow-ups
3 items deferred
netstack entirelydns.lookupinterceptionWritable.prototype.write/end.call(aSocket, ...)bypassing thenet.Socket.prototypewrite/end guardstream.Writable.prototypeitself corrupts every otherWritablesubclass'swrite()process-wide (confirmed against Vite's own HTTP client in real bundler tests), sinceinstallGuardedProperty's setter isn't safe for a base class this widely subclassed. Not worth pursuing further — this guard is dev-loop safety, not a hard security boundaryDocumentation