Skip to content

[APPS-2792] Add: runtime network/subprocess guard for local execution - #484

Merged
gh-worker-dd-mergequeue-cf854d[bot] merged 1 commit into
masterfrom
tiffany.trinh/apps-2792-runtime-network-guard
Sep 9, 2026
Merged

[APPS-2792] Add: runtime network/subprocess guard for local execution#484
gh-worker-dd-mergequeue-cf854d[bot] merged 1 commit into
masterfrom
tiffany.trinh/apps-2792-runtime-network-guard

Conversation

@tyffical

@tyffical tyffical commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Motivation

  • Part of APPS-2792 — runtime half of the sandboxing milestone in the Kickoff doc.
  • Stacks on #481#480, sharing their LOCAL_EXECUTION_LOAD_SUFFIX/moduleResolverFor test double.
  • #476 statically rejects Node-builtin/network references in the customer's own .backend.ts, but can't see the same calls made from inside a third-party dependency in node_modules.
  • Local execution runs in-process with no OS-level sandbox boundary (unlike production's Deno sandbox, which never grants --allow-net) — this closes the equivalent gap at the module level.

Architecture

  • network-guard.ts patches 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.
  • Each guard signals failure the way its real Node API does — an async 'error' event, an error-first callback, or a result object with .error set — 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/end are guarded too, since a reused keep-alive socket never calls connect() again; the guard destroy()s the socket instead of throwing, and exempts process.stdout/stderr by an identity captured once at module load so a customer function can't reassign them to exfiltrate data past the block.
  • Block/allow state is read fresh per call from AsyncLocalStorage, not a shared flag:
runScriptLocally
      │
      ▼
┌────────────────────────────────────┐
│ runBlocked(fn)                     │  blockedContext.run(true, fn)
│ isCurrentlyBlocked() → true for    │
│ this async chain only              │
└───────────────┬─────────────────────┘
                │  customer's fn() runs
                ▼
  fn() calls $.Actions.a() and $.Actions.b() concurrently (Promise.all)
                │
      ┌─────────┴──────────┐
      ▼                    ▼
 runAllowed(a)         runAllowed(b)
 allowedContext.run(   allowedContext.run(
   true, a)              true, b)
      │                    │
      ▼                    ▼
┌────────────────────────────────────┐
│ isCurrentlyBlocked() → false       │
│ for EACH call's own async chain —  │
│ overlapping siblings never share   │
│ or contend over one flag           │
└──────┬───────────────────────┬─────┘
       │ a resolves            │ b resolves
       ▼                       ▼
 a's allowedContext        b's allowedContext
 chain ends — no effect    chain ends — no effect
 on b, still in flight     on a (already done)
                │
                ▼
        fn() returns — runBlocked's
        blockedContext chain ends
                │
                ▼
   Back outside any AsyncLocalStorage.run() —
   isCurrentlyBlocked() reads no store, false
  • Each runAllowed() call gets its own AsyncLocalStorage context, so concurrent $.Actions calls never contend over shared exemption state — one call's exit can't prematurely re-block a sibling still in flight.
  • A hung customer function stays blocked fail-safe:
    • runScriptLocally's Promise.race([run(), timeout]) abandons rather than cancels the loser, so the blocked scope never naturally exits.
    • runBlocked exposes a BlockedScopeHandle (abandonIfCurrent(), backed by an epoch counter in execution-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
What changed File
Added hasActiveScope() and forceInvalidate() to EpochGuard, so runAllowed/forceReset can detect and invalidate an active runBlocked scope. execution-epoch.ts
New installGuardedProperty installs a permanent getter/setter on each guarded Node global, rebuilding the exposed wrapper object on every external write. network-guard.ts
New runBlocked(fn)/runAllowed(fn) scope block/allow state per async chain via AsyncLocalStorage, not a shared flag. network-guard.ts
New forceReset() invalidates the shared epoch counter; kept as a test-only reset between test cases, not called from production code. network-guard.ts
runScriptLocally wraps the customer's function call — including its result's assertJsonSerializable check — in runBlocked, not the preceding loadModule/registration calls. local-execution.ts
makeActionsProxy and the action-catalog dispatcher JSON round-trip inputs via serializeActionInputs before entering runAllowed, closing a toJSON()/getter exfiltration path. It shares assertJsonSerializable's stricter validation (via a new assertJsonRoundTrippable helper) instead of a bare round-trip, so a Map/Set/NaN/symbol-keyed input fails loudly instead of reaching the destination action silently corrupted. local-execution.ts
assertJsonRoundTrippable silently omits an undefined-valued plain-object property (return value or $.Actions input) instead of throwing, matching what production's own JSON.stringify already does — an array element's undefined (silently converted to null by JSON.stringify, not dropped) is still caught. local-execution.ts, local-execution.test.ts
makeActionsProxy and the action-catalog dispatcher's identical validate → serialize → runAllowed sequence is extracted into a shared invokeAction helper. local-execution.ts
Extended the guard to dgram (UDP) and the native WebSocket global, both previously unguarded. network-guard.ts
Guarded inbound listener entry points (net.Server.prototype.listen, dgram.Socket.prototype.bind) via the renamed generic guardNetworkMethod. network-guard.ts, network-guard.test.ts
Blocked worker_threads.Worker construction via a Proxy construct trap, mirroring guardWebSocket's shape. network-guard.ts, network-guard.test.ts
Guarded all four DNS resolver surfaces (dns, dns.promises, dns.Resolver.prototype, dns.promises.Resolver.prototype), rejecting rather than throwing on the promise-returning ones. network-guard.ts, network-guard.test.ts
Each guard now captures its own snapshot of the real delegate at build time, instead of a variable shared across all guards on the same property. network-guard.ts
Unit tests cover every guarded target in both directions, including concurrency and abandoned-scope epoch handling. network-guard.test.ts
Integration tests confirm the guard is wired into executeScriptLocally: raw network/subprocess calls are rejected, and concurrent $.Actions calls still succeed. local-execution.test.ts
exec/execFile's guarded wrappers re-attach a util.promisify.custom implementation resolving {stdout, stderr}, matching Node's native contract. network-guard.ts, network-guard.test.ts
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. network-guard.ts, network-guard.test.ts
installGuardedProperty handles a non-object makeGuard result (e.g. guardWebSocket returning undefined) without throwing. network-guard.ts, network-guard.test.ts
Also guards ChildProcess.prototype.spawn directly, since the standalone spawn/exec/etc. functions are thin wrappers a dependency could call through to bypass them. network-guard.ts
Guards net.Socket.prototype.write/end (via destroy(), not a throw) to close a keep-alive-reuse bypass of connect()-only guarding. network-guard.ts, network-guard.test.ts
write/end are configurable specifically under Jest, so Jest's own spyOn/mockRestore (used elsewhere in this repo's test suite, against process.stderr/stdout when CI pipes them into real net.Socket instances) doesn't collide with the guard. network-guard.ts, network-guard.test.ts
guardConnect, and separately guardWebSocket/guardEventSource, and separately guardNetworkMethod/guardNetworkPromiseMethod/guardSubprocess, are each deduped into one shared factory per group. guardSocketWrite/guardSocketEnd share a new guardSocketOp factory the same way, parameterized by the blocked-path return value. network-guard.ts
New signalBlockedSocketOp fixes 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. network-guard.ts, network-guard.test.ts
process.stdout/process.stderr are exempted from the write/end block by identity captured once at module load (trustedStdout/trustedStderr), not the live, reassignable process.stdout/process.stderr getters — a customer function repointing process.stdout to an attacker-controlled socket would otherwise inherit the exemption and exfiltrate data past the block. network-guard.ts, network-guard.test.ts
runBlocked exposes a BlockedScopeHandle (abandonIfCurrent()) to its caller instead of relying on the module-level forceReset(); 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 $.Actions call. network-guard.ts, local-execution.ts, network-guard.test.ts
guardFetch deduped into the existing guardNetworkPromiseMethod/makeGuardWrapper factory, since fetch's blocked-path contract (reject, not throw) was already exactly what that factory provides. network-guard.ts
getSharedContext's per-key AsyncLocalStorage entries on net are now installed via non-configurable Object.defineProperty instead of a plain assignment, closing a bypass where any code holding a net reference could swap in a fake store and silently disable every guard in this file at once (isCurrentlyBlocked() is their shared gate). network-guard.ts, network-guard.test.ts
guardWorker now reuses guardConstructibleGlobal (generalized with an optional blocked-message parameter) instead of its own copy, fixing a crash when the real Worker global is undefined and deduplicating two identical construct-trap implementations. network-guard.ts, network-guard.test.ts
signalBlockedSocketOp's listenerCount('error') > 0 check is deferred via process.nextTick, so attaching the 'error' listener immediately after write()/end() (not just before) still triggers the blocked signal instead of being silently swallowed. network-guard.ts, network-guard.test.ts
net.Server.listen, dgram.Socket.bind/connect/send, and the callback-style dns.resolve* surfaces are now guarded via new guardBindMethod/guardCallbackMethod factories that signal failure the same way each real API does (async 'error' event or error-first callback), replacing the old guardNetworkMethod, which always threw synchronously and could crash a caller relying on the idiomatic async pattern. network-guard.ts, network-guard.test.ts
local-execution.ts now lazily imports network-guard.ts (memoized dynamic import()), confining its process-wide monkeypatch installation to when local execution actually runs instead of to any bundler that transitively imports the apps plugin. local-execution.ts, local-execution.test.ts
loadCustomerModuleEntry awaits the network guard module before invoking loadModule, so a customer module's top-level code can no longer run before network-guard.ts captures trustedStdout/trustedStderr, closing a window where a reassigned process.stdout could permanently bypass the write-blocking guard; its doc comment documents this ordering alongside the remaining, accepted top-level-code gap. local-execution.ts, network-guard.ts, local-execution.test.ts
runScriptLocally's two timeout paths share a new failWithTimeout helper instead of duplicating the conclude/abandon/reject sequence. local-execution.ts
spawn/fork/exec/execFile/ChildProcess.prototype.spawn are now guarded via new guardSpawnFactory/guardExecFactory/guardChildProcessSpawnMethod factories matching each API's real contract (async 'error' event, error-first callback, or a synchronous integer return), replacing the old always-throw guardSubprocess for these five; execSync/execFileSync are unchanged since they genuinely throw synchronously. network-guard.ts, network-guard.test.ts
spawnSync is guarded via a new guardSpawnSyncResult, returning a SpawnSyncReturns-shaped object with .error set instead of throwing, matching its real never-throws contract. network-guard.ts, network-guard.test.ts
createBlockedChildProcessStub fabricates a ChildProcess-shaped stub (real inert stdout/stderr/stdin streams, send()/disconnect()/kill()) for the blocked-path return value of spawn/fork/exec/execFile, so a caller touching those fields doesn't hit an unrelated TypeError. network-guard.ts, network-guard.test.ts
emitAsyncErrorIfListened/invokeCallbackArg extract the repeated "emit error if listened"/"invoke the trailing error-first callback" logic shared across guardBindMethod, guardCallbackMethod, signalBlockedSocketOp, and the new child_process guards. network-guard.ts
installGuardedProperty's setter now detects when the assignment receiver differs from the originally-guarded target (e.g. an instance-level someSocket.write = mock on the shared net.Socket.prototype guard) and installs a per-instance override instead of corrupting the one delegate every other instance's guard still calls through. network-guard.ts, network-guard.test.ts
assertJsonRoundTrippable's symbol-keyed-property check moved from a pre-JSON.stringify pass over the raw value into the JSON.stringify replacer itself, so it also catches a symbol key a custom toJSON() introduces only in its return value. local-execution.ts, local-execution.test.ts
serializeActionInputs now verifies the JSON-round-tripped $.Actions inputs value is still a plain object (rejecting a top-level toJSON() that turns it into a string/array/null, and a raw array passed as inputs), instead of trusting a bare type assertion. local-execution.ts, local-execution.test.ts
The function/symbol-in-array-vs-object error message now correctly describes each case's real JSON.stringify behavior (an array element converts to null; an object property is dropped), instead of one message wrongly claiming both are dropped. local-execution.ts, local-execution.test.ts
New trustedFetch captures the real globalThis.fetch at module load, before installGuardedProperty patches it, so a customer function reassigning globalThis.fetch to an attacker-controlled wrapper can no longer intercept it. network-guard.ts, network-guard.test.ts
New RequestOpts.fetchImpl lets a caller supply a trusted fetch reference instead of doRequest reading globalThis.fetch fresh at call time; defaults to the global lookup so existing mocking (nock, jest.spyOn(global, 'fetch')) is unaffected. types.ts, request.ts, request.test.ts
getAuthenticatedRequest() lazily imports network-guard.ts and passes its trustedFetch as fetchImpl, closing the gap where the dev server's authenticated $.Actions/runtime-context request could be redirected to a customer-installed fetch wrapper and leak the request's auth headers. auth.ts, auth.test.ts

QA Instructions

yarn install
yarn test:unit packages/plugins/apps/src/vite/network-guard.test.ts
# Expected: Test Suites: 1 passed / Tests: 70 passed ✅ VERIFIED
yarn test:unit packages/plugins/apps/src/vite/local-execution.test.ts
# Expected: Test Suites: 1 passed / Tests: 98 passed ✅ VERIFIED
yarn 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) ✅ VERIFIED
yarn test:unit
# Expected: Test Suites: 90 passed / Tests: 2236 passed, 1 skipped ✅ VERIFIED
# (rollupConfig.test.ts's Node v26 --localstorage-file experimental-warning quirk,
# predating this branch, is intermittent — not a guaranteed failure on every run)
yarn workspace @dd/apps-plugin run typecheck && yarn workspace @dd/core run typecheck
# Expected: no output, clean exit ✅ VERIFIED
npx eslint 'packages/plugins/apps/**/*.ts' 'packages/core/**/*.ts' packages/tests/src/_jest/helpers/mocks.ts --quiet
# Expected: no output, clean exit ✅ VERIFIED
  • Jest's collectCoverageFrom gives no usable per-file report in this repo — manually confirmed every branch in network-guard.ts is exercised by at least one test.
  • No standalone HTTP surface; exercised end-to-end via #481's dev server and via a combined manual pass across the full stack (a scaffolded app confirmed raw net/fetch is blocked in a customer function while a real $.Actions call still succeeds).

Blast Radius

  • No behavior change for any currently-shipping code path — this stack isn't released yet.
  • Scoped to the async chain of a local execution's customer-function call via AsyncLocalStorage; the dev server's own network use is never touched.
  • An abandoned (timed-out) hung customer function stays blocked for as long as it keeps running — no reset restores its network access early.
  • Risk: low. Additive, defense-in-depth only — production's Deno sandbox is unaffected and remains the real boundary.

Out of Scope / Follow-ups

3 items deferred
Item Status Next step
Native addon bypassing Node's JS-level net stack entirely Accepted residual gap Narrower and rarer than the pure-JS case this closes (most native modules are for CPU-bound work, not networking) — not worth the false-positive risk of blocking native addon loading outright
dns.lookup interception Out of scope Low realistic benefit for this threat model (dev-loop safety, not defending against deliberate DNS-tunneling exfiltration) — would risk breaking legitimate hostname validation for no real gain
Writable.prototype.write/end.call(aSocket, ...) bypassing the net.Socket.prototype write/end guard Accepted residual gap Guarding stream.Writable.prototype itself corrupts every other Writable subclass's write() process-wide (confirmed against Vite's own HTTP client in real bundler tests), since installGuardedProperty'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 boundary

Documentation

@datadog-prod-us1-6

datadog-prod-us1-6 Bot commented Aug 8, 2026

Copy link
Copy Markdown

Tests

All CI checks and tests passed.

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 42db481 | Docs | View more details | Give us feedback!

tyffical added a commit that referenced this pull request Aug 10, 2026
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.
tyffical added a commit that referenced this pull request Aug 20, 2026
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.
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-runtime-network-guard branch from e48778e to 2ad1ce9 Compare August 20, 2026 22:19
@tyffical
tyffical changed the base branch from tiffany.trinh/apps-2792-harden-local-execution-v2 to tiffany.trinh/apps-2792-wire-into-dev-server August 20, 2026 22:25
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-runtime-network-guard branch from 2ad1ce9 to b69e5f2 Compare August 20, 2026 22:28
tyffical added a commit that referenced this pull request Aug 20, 2026
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.
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-runtime-network-guard branch from b69e5f2 to 63539eb Compare August 20, 2026 23:31
tyffical added a commit that referenced this pull request Aug 20, 2026
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.
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-runtime-network-guard branch from 63539eb to 1b11592 Compare August 20, 2026 23:40
tyffical added a commit that referenced this pull request Aug 21, 2026
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.
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-runtime-network-guard branch from 1b11592 to 9d591ec Compare August 21, 2026 03:54
@tyffical
tyffical requested a balanced review from Copilot August 21, 2026 16:24
@DataDog DataDog deleted a comment from chatgpt-codex-connector Bot Aug 21, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Friend, this PR adds runtime restrictions for in-process local backend execution.

Changes:

  • Adds process-wide network and subprocess guards.
  • Exempts $.Actions calls 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

  • runAllowed can run after its enclosing blocked scope has already been reset. In the existing abandoned-execution scenario, a late call through a captured $.Actions proxy increments from zero, the guarded action rejects, and this applyPatches() then leaves the whole process blocked even though no runBlocked is active; the test's afterEach(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 entering runAllowed.
        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.

Comment thread packages/plugins/apps/src/vite/network-guard.ts Outdated
Comment on lines +428 to +433
// 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));

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread packages/plugins/apps/src/vite/network-guard.ts Outdated
Comment thread packages/plugins/apps/src/vite/network-guard.ts Outdated
chatgpt-codex-connector[bot]

This comment was marked as resolved.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Comment thread packages/plugins/apps/src/vite/network-guard.ts Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

// (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));

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread packages/plugins/apps/src/vite/network-guard.test.ts
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-runtime-network-guard branch from ea84a47 to 4fd59b3 Compare August 26, 2026 02:35
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-runtime-network-guard branch from 4fd59b3 to cb79658 Compare August 26, 2026 04:32
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-runtime-network-guard branch from cb79658 to affa16b Compare August 26, 2026 16:15
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-runtime-network-guard branch from affa16b to fbad040 Compare August 26, 2026 17:31
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-runtime-network-guard branch from fbad040 to 085afa1 Compare August 26, 2026 18:09
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-runtime-network-guard branch from 085afa1 to 64e9169 Compare August 27, 2026 06:05
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-runtime-network-guard branch from 64e9169 to aff9ee7 Compare August 27, 2026 06:15
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-runtime-network-guard branch from aff9ee7 to a2525b5 Compare August 27, 2026 17:08
@tyffical
tyffical force-pushed the tiffany.trinh/apps-2792-runtime-network-guard branch from a2525b5 to 2e2c8eb Compare August 27, 2026 18:10
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-03T23:48:59.562684Z 120fcec Manual request
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

chatgpt-codex-connector[bot]

This comment was marked as resolved.

This comment was marked as resolved.

chatgpt-codex-connector[bot]

This comment was marked as resolved.

This comment was marked as resolved.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread packages/plugins/apps/src/vite/network-guard.ts Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 AsyncLocalStorage chain is still current. If scope A is abandoned and scope B starts before A's continuation calls runAllowed, B makes hasActiveScope() 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

Comment thread packages/plugins/apps/src/vite/network-guard.ts Outdated
Comment on lines +537 to +541
installGuardedProperty<typeof net.Socket.prototype.connect>(
net.Socket.prototype,
'connect',
(getReal) => makeGuardWrapper(getReal, NETWORK_BLOCKED_MESSAGE, 'throw'),
);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread packages/plugins/apps/src/vite/local-execution.ts Outdated
Comment thread packages/plugins/apps/src/vite/local-execution.ts
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants