[APPS-2792] Add: runtime network/subprocess guard for local execution - #484
Conversation
|
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.
| function restorePatches(): void { | ||
| if (savedConnect) { | ||
| net.Socket.prototype.connect = savedConnect; | ||
| } | ||
| if (savedFetch) { | ||
| globalThis.fetch = savedFetch; | ||
| } | ||
| if (savedSpawn) { | ||
| child_process.spawn = savedSpawn; | ||
| } | ||
| if (savedExec) { | ||
| child_process.exec = savedExec; | ||
| } | ||
| if (savedExecSync) { | ||
| child_process.execSync = savedExecSync; | ||
| } | ||
| } |
![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.
| const abandonedAction = runAllowed( | ||
| () => | ||
| new Promise<void>((resolve) => { | ||
| resolveAbandonedAction = resolve; | ||
| }), | ||
| ); | ||
|
|
||
| // Simulates the timeout handler abandoning this execution while | ||
| // the $.Actions call above is still in flight. | ||
| forceReset(); | ||
|
|
||
| // A newer execution starts, and its own legitimate $.Actions call | ||
| // must be correctly allowed through and re-blocked afterward. | ||
| const result = await runBlocked(async () => { | ||
| await runAllowed(async () => 'newer allowed call'); | ||
| await expect(fetch('https://example.com')).rejects.toThrow( | ||
| /Network access is not allowed/, | ||
| ); | ||
| return 'newer execution result'; | ||
| }); | ||
| expect(result).toBe('newer execution result'); | ||
|
|
||
| // The abandoned call's runAllowed finally now fires, well after | ||
| // being superseded — it must not touch allowDepth. | ||
| resolveAbandonedAction?.(); | ||
| await abandonedAction; |
There was a problem hiding this comment.
The permanent-property redesign in 68ed39f replaces the old generation-counter check this comment was about — runAllowed now checks hasActiveScope() on a shared epoch guard, and calling it with no active runBlocked scope is a deliberate, meaningful code path (an abandoned execution's late-settling $.Actions call). The existing tests "Should not let an abandoned runAllowed call's late settlement affect later executions" and "Should treat a runAllowed call that only starts after its execution was already abandoned as a no-op..." already exercise exactly that path and assert it doesn't wedge the guard.
51f9d73 to
ceeece1
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
2592d9b to
8d82135
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.
6720f6b to
07cca79
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.
ea84a47 to
4fd59b3
Compare
4fd59b3 to
cb79658
Compare
cb79658 to
affa16b
Compare
affa16b to
fbad040
Compare
fbad040 to
085afa1
Compare
085afa1 to
64e9169
Compare
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.
64e9169 to
aff9ee7
Compare
Motivation
local-execution.tslives there and because this PR shares theLOCAL_EXECUTION_LOAD_SUFFIXmechanism [APPS-2792] Add: wire local execution into the real dev server #481 introduces (see below).fetch/XMLHttpRequest/WebSocket/EventSourcereferences — but only in the customer's own.backend.tsfile. A third-party dependency (e.g. a Postgres or Redis client) that itself callsnet/http/fetchinternally is invisible to that static scan, since it never inspectsnode_modules.wf-actions-worker'sdeno.ts: production's Deno sandbox never grants--allow-net, under any code path — this PR closes the equivalent gap at the module level for local execution.local-execution.tscall-site change (markingloadModule's request withLOCAL_EXECUTION_LOAD_SUFFIXso the frontend RPC-proxy transform hook doesn't intercept it) and the same test-double extraction intomoduleResolverFor. Rebasing onto [APPS-2792] Add: wire local execution into the real dev server #481 let git drop this PR's own now-redundant copies of both commits outright (patch contents already upstream) instead of leaving a real merge conflict for whichever PR merged second.Architecture
net.Socket.prototype.connect,globalThis.fetch, andchild_process'sspawn/exec/execSyncare real, process-wide singletons —network-guard.tsmonkey-patches them directly rather than sandboxing the customer's module, since there's no process boundary to sandbox with. That makes the guard a single shared piece of mutable state (allowDepth+ the saved originals) threaded through one execution's lifetime:The ref-count (
allowDepth), not a boolean, is what makes the overlap safe: two concurrent$.Actionscalls each bump it on entry and drop it on exit, and the guard only re-blocks once the last one exits — a boolean would re-block the instant the faster of two overlapping calls finished, breaking the slower one mid-flight.runBlocked/runAllowed's owntry/finallyonly unwinds whenfnactually settles.runScriptLocally's timeout wraps the whole thing inPromise.race([run(), timeout]), which abandons rather than cancels the loser — a customer function that never resolves meansrun()(and therunBlockedinside it) never reaches itsfinally, so without a separate backstop the block would stay applied for the rest of the process once the timeout fires.forceReset()is that backstop: called directly from the timer callback (unconditionally restoring the real functions and zeroingallowDepth) the moment the timeout fires, independently of whether the abandonedrun()ever settles — alongsidelocal-execution.ts's own epoch-gatedpoisonActionCatalogRegistration()call in the same timer (from #480), since both are closing the same class of "abandoned execution left shared state pointing the wrong way" gap. The same function is used as a JestafterEachinnetwork-guard.test.ts/local-execution.test.ts, for the identical reason at the test level — these are real Node singletons, not per-test-file sandboxed state, so a test that leaves them patched leaks into every test that runs after it in the same Jest worker, including unrelated test files.Changes
runBlocked(fn): monkey-patchesnet.Socket.prototype.connect,fetch, andchild_process'sspawn/exec/execSyncto throw/reject for the duration offn, restoring the real implementations in afinallyregardless of howfncompletes.runAllowed(fn): temporarily restores real network access for the duration offn, ref-counted (not a boolean) so two$.Actionscalls overlapping within a single execution (e.g. inside aPromise.all) don't re-block network on each other mid-flight.forceReset(): unconditionally restores the real functions and zeroesallowDepth, independent ofrunBlocked/runAllowed's ownfinally— the backstop for afnthat's abandoned (timeout) or a test that fails to clean up after itself.runScriptLocallynow wraps the customer's function call (only — not theloadModule/registration calls before it, which need no network) inrunBlocked, and callsforceReset()from the timeout timer itself so an abandoned, still-running hung function can't leave network/subprocess access blocked for the rest of the process.makeActionsProxy'sapplytrap now wraps itsexecuteActioncall inrunAllowed— the one sanctioned network path, exempted from the block.runBlockedcalls, nestedrunAllowedexemption, concurrent-overlap ref-counting, re-block-on-throw). A JestafterEachcallsforceReset()unconditionally as a hard safety net, independent of any test's own cleanup.executeScriptLocally: a customer function using rawnet/fetch/child_processis rejected; a real$.Actionscall still succeeds; network is restored after the execution finishes, including after a timeout abandons a hung function; two real$.Actionscalls made concurrently viaPromise.allkeep network allowed through the entire overlap, exercised through the realexecuteScriptLocally→makeActionsProxypath (not just the unit-levelrunAllowed). SameafterEachsafety net as above.QA Instructions
yarn test:unit packages/plugins/apps/src/vite/network-guard.test.ts # Expected: Test Suites: 1 passed / Tests: 11 passed ✅ VERIFIEDyarn test:unit packages/plugins/apps/src/vite/local-execution.test.ts # Expected: Test Suites: 1 passed / Tests: 30 passed ✅ VERIFIEDyarn test:unit packages/plugins/apps # Expected: Test Suites: 25 passed / Tests: 337 passed ✅ VERIFIEDyarn workspace @dd/apps-plugin run typecheck # Expected: no output, clean exit ✅ VERIFIEDCoverage note: this repo's Jest
collectCoverageFromCLI flag didn't produce a usable per-file report for either new/changed file in this environment (pre-existing tooling quirk, not introduced by this change — the coverage table only ever listed_jesthelper files regardless of the glob passed). Manually verified every branch innetwork-guard.tsis exercised by at least one test.This module isn't independently reachable from a real
npm run devsession on its own — that requires #481. It was exercised as part of a real, combined manual QA pass across the full stack (see #481's QA Instructions): a real scaffolded app, running with the full stack merged locally, confirmed the network guard correctly blocks rawnet/fetchin a customer function while still letting a real$.Actionscall through — end-to-end, not just at the unit-test level.Blast Radius
local-execution.ts) is never touched.forceReset()on timeout narrows, rather than eliminates, an existing gap: the abandoned (not cancelled) hung function keeps running with real network access restored early rather than staying blocked forever — bounded to that one already-abandoned execution, versus the alternative of leaving every future execution in the same dev server process permanently blocked until restart.Out of Scope / Follow-ups
netstack entirelydns.lookupinterceptionAbortSignalthreaded through the customer's own function, which we don't control) or re-architecting local execution onto a worker thread that can be killed outright — bigger change than this PR's scopeDocumentation