[APPS-2792] Add process.env scoping for local execution (Secret Store parity) - #504
[APPS-2792] Add process.env scoping for local execution (Secret Store parity)#504tyffical wants to merge 5 commits into
Conversation
bbf9238 to
32ba868
Compare
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🔗 Commit SHA: dd2b919 | Docs | View more details | Give us feedback! |
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. |
![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.
32ba868 to
a0bd2df
Compare
3a6e6dc to
dc63b88
Compare
dc63b88 to
841961c
Compare
841961c to
964fe0d
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…(Secret Store parity) Local execution runs a customer's backend function in-process, so without this it inherits the dev server's own full, real process.env — every secret and credential the dev server process has access to, not just what that function's declared connections should see. env-guard.ts closes this by installing a guarded, scoped env for the duration of runBlocked, mirroring network-guard.ts's shape and AsyncLocalStorage-per-call-chain scoping. Also closes several other read paths that reach the same real environment outside the documented process.env property: the /proc/self/environ (and /proc/[pid]/environ) file, reachable via fs.createReadStream/open/openSync/ promises.open/copyFileSync/copyFile/cpSync/cp (in either string or Buffer/URL path form), via constructing fs.ReadStream directly, via a numeric file descriptor already open against the real environ file, and via createReadStream/ReadStream's own options.fd (or a FileHandle's .fd) — resolved exactly once and reused for both the check and the real call, since an accessor-backed options.fd could otherwise show the check a harmless value and hand the real, separate read a different one; and process.report.excludeEnv, guarded against a customer function reassigning it from inside its own scope the same way process.env itself is guarded. The guard wraps Node's own native excludeEnv get/set (on versions that have one) rather than replacing them with a plain JS variable — a native, non- JS-triggered report (--report-on-fatalerror/--report-on-signal) reads Node's own internal flag directly, so a disconnected shadow would read back whatever value was last written while having no effect on what those reports actually contain. The action-catalog/backend-runtime adapter registrations in local-execution.ts now resolve their npm packages inside the same env/network scope as the customer function itself, rather than before it — their own top-level code would otherwise see the real, unscoped environment and unblocked network on first load in the process. env-guard.ts and network-guard.ts now share one getOrCreateShared() helper for the Symbol.for-keyed singleton pattern both need to survive being evaluated more than once (bundled copies, Jest's per-test-file isolation), instead of each keeping its own copy. env-guard.test.ts and local-execution.test.ts share a new installFakeProcessEnv() test helper for the same reason, instead of each duplicating the same beforeAll/afterAll real-environment swap — captured as a value snapshot rather than a reference to process.env itself, since the latter can already be a guard-installed accessor by the time the snapshot is taken, and restoring through that same reference later is a no-op under the guard's own self-reassignment check, permanently stranding process.env at the fake baseline instead of restoring the real environment for every test file that runs afterward in the same Jest worker. local-execution.ts's two lazy-import-and-memoize blocks for network-guard.ts and env-guard.ts are now one shared lazyImportOnce() helper. The reassignment-rejection check shared by process.env's and process.report.excludeEnv's setters, and the Object.defineProperty shape excludeEnv's native-vs-shadow branches both used, are now each expressed once instead of twice. env-guard.test.ts and local-execution.test.ts shared the same collection-time-capture bug: a describe-body `const` captured process.env before cleanEnv()'s beforeAll had a chance to strip secrets, so afterEach then restored the real, unstripped environment for the rest of the block. Both now capture inside beforeAll instead.
…tion builds configFile: false only skips loading a vite.config.js — it doesn't disable Vite's separate .env-file/import.meta.env machinery. loadEnv() copies any VITE_-prefixed key straight out of the dev server's own real process.env (independently of envFile/envDir), and the define plugin statically inlines that value into the built backend function at build time, bypassing runWithScopedEnv's runtime scoping entirely, since that only wraps module execution, never the bundling step itself. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…t just Source $ now spreads an opaque preview-API response onto it, and the existing test only ever checked .Source for token-shaped keys — so a credential-shaped field added anywhere else on $ would go uncaught. The scan now recurses over all of $ except Actions (a Proxy dispatch mechanism, not a data container) and checks for token/secret/key/ password/credential substrings instead of just "token".
44bd4fc to
196ba96
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1483269fc0
ℹ️ 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".
| applyExcludeEnvValue = (newValue) => { | ||
| excludeEnvValue = newValue; | ||
| }; |
There was a problem hiding this comment.
On the repository's Node 20 target, this shadow value has no effect on native report generation, while only direct getReport() and writeReport() calls are wrapped. A scoped backend function can set process.report.reportOnSignal = true and a filename, call process.kill(process.pid, 'SIGUSR2'), then read the generated report; Node 20 writes the real environment into that file despite excludeEnv reading as true. Guard the signal-triggered path (or prevent scoped code from enabling and triggering it) on runtimes without native exclusion support.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is the same accepted residual gap this file's own comment on excludeEnvGuardInstalled already documents: on Node <22.13 (including this repo's pinned Node 20.19.4 CI target), excludeEnv has no effect on native report generation, and the JS-level getReport()/writeReport() wraps only cover directly-called reports, not ones Node generates on its own via --report-on-signal/--report-on-fatalerror — there's no JS call to intercept for those. Not a new gap introduced by this PR; it's the same JS-level-defense-in-depth-not-a-hard-boundary limitation already called out for the pre-22.13 case elsewhere in this file. Leaving this thread open rather than resolving it, since it's a real (if pre-existing and already-documented) exposure worth an explicit maintainer decision — e.g. disabling signal/fatal-error report generation for scoped executions on old Node — rather than silent acceptance.
3a4616a to
5b8affe
Compare
1b3182c to
fcedb11
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
48894c1 to
bf50e5a
Compare
1a37127 to
7ea5b29
Compare
…bug found in review The process.env Proxy's `set` trap forwarded the mismatched `receiver` argument straight to Reflect.set, which for an existing key falls back to a partial-descriptor defineProperty call that Node's native process.env rejects — breaking any code (dd-trace's require-hook included) that assigns to an existing key from outside a scope. Fixes that by defaulting the receiver to the target object instead. Also closes four more gaps found in review: - The environ-path regex only matched self/thread-self/the dev server's own pid, letting a readable parent /proc entry (or any other accessible pid) through — now matches any numeric pid. - fs.promises.readFile's guard predicate didn't unwrap a FileHandle argument to its underlying fd, so a handle opened against /proc/self/environ before the scope reached the real read unguarded. - process.report.excludeEnv could be disarmed by an unrelated caller writing from outside any scope while a different scope was still active — such writes are now deferred until that scope closes, instead of applied immediately and then clobbered by its cleanup. - The callback-style fs.readFile/open/copyFile/cp were wrapped with the same synchronous-throw guard as their Sync counterparts, violating their real error-first-callback contract. They now report a guard failure through the callback instead.
7ea5b29 to
61176af
Compare
oliverli
left a comment
oliverli
left a comment
There was a problem hiding this comment.
Review pass over the env-guard work. Two blockers and four major findings inline — the two blockers are one-line bypasses of the module's core guarantee. Everything else (guarded-wrapper/network-guard refactor, Vite envPrefix/envFile fix, shared singleton) verified clean.
| // every evaluation must share the same scopedEnvContext/realEnv/activeScopeCount or a later | ||
| // evaluation's Proxy would never consult the storage an earlier evaluation's scope populates. | ||
| function getSharedState(): SharedEnvGuardState { | ||
| return getOrCreateShared(fs, '@dd/apps-plugin/env-guard shared-state', () => ({ |
There was a problem hiding this comment.
[blocker] The real environment is recoverable from the public fs registry
getOrCreateShared stores the whole SharedEnvGuardState — including realEnv and the AsyncLocalStorage instance — as a readable property on the public fs module, keyed with a guessable Symbol.for string. Backend code can do this while the scoped proxy is active:
require('fs')[Symbol.for('@dd/apps-plugin/env-guard shared-state')].realEnv.DD_API_KEYThe property is non-writable/non-configurable, but still readable. Keep security-sensitive state — the real environment and the scope store — off modules untrusted code can import. The same concern applies to network-guard's store on net (store.disable() disarms network blocking), though that pattern is pre-existing; this newly escalates it from "disable a guard" to "read all secrets".
| } | ||
|
|
||
| function currentEnv(): Record<string, string> | NodeJS.ProcessEnv { | ||
| return sharedState.scopedEnvContext.getStore() ?? sharedState.realEnv; |
There was a problem hiding this comment.
[blocker] Scope detection resolves through mutable prototypes and fs helpers
currentEnv() calls sharedState.scopedEnvContext.getStore() — a writable AsyncLocalStorage.prototype method. Backend code can replace it with () => undefined:
- every
process.env.Xread falls back tosharedState.realEnv(full leak), and - the same replacement disables the
/procchecks and the reassignment guard.
Related, same class: isEnvironPath resolves fs.realpathSync/fs.readlinkSync dynamically, so on Linux backend code can save the already-wrapped fs.readFileSync, replace fs.realpathSync with a benign-path stub, and read /proc/self/environ through the saved wrapper — the wrapper's captured native read still runs after the forged check.
Capture and bind the native getStore, realpathSync, and readlinkSync before customer modules load, and use those immutable references for all access decisions.
| // settle and so never reach its finally. | ||
| export function forceResetEnv(): void { | ||
| if (sharedState.activeScopeCount > 0) { | ||
| sharedState.activeScopeCount = 0; |
There was a problem hiding this comment.
[major] forceResetEnv() disarms report redaction for still-running continuations
Zeroing the process-wide activeScopeCount does not stop the abandoned customer continuation — it stays in its AsyncLocalStorage scope and can resume later. Consequences:
- The
getReport()/writeReport()wrappers gate redaction onactiveScopeCount > 0, so a timed-out function that resumes can callprocess.report.getReport()and receive the realenvironmentVariables. - A concurrently active, unrelated scope is disarmed too, and
restoreExcludeEnvIfLastScope()resets the nativeexcludeEnvflag out from under it.
(Triggers from abandonExecutionAndRejectWith in local-execution.ts.) Env scoping itself survives because it is per-continuation — but report redaction is a global count. Gate the report wrappers on the current async scope and do not globally disarm redaction while scoped continuations can still run.
|
|
||
| const originalWriteReport = process.report.writeReport.bind(process.report); | ||
| process.report.writeReport = wrapReportFn(originalWriteReport, (original, ...args) => { | ||
| const filename = original(...args); |
There was a problem hiding this comment.
[major] writeReport() persists the unredacted report before rewriting it
On Node < 22.13 (CI pins 20.19.4, where excludeEnv is a no-op), this wrapper lets Node write the report containing the real environment to disk first, then reads and rewrites it. If the post-processing fs.readFileSync/fs.writeFileSync (mutable references) throws or is replaced, the unredacted file stays on disk — the caller catches the error and reads the file through a saved filesystem function.
Produce the redacted content before the destination becomes observable, rather than rewriting a file Node already persisted with secrets in it.
| // it back unchanged. Must short-circuit before the realEnv assignment below — adopting | ||
| // the proxy as its own currentEnv() fallback would make every future unscoped read | ||
| // resolve back through this same trap, recursing forever. | ||
| if (isEnvProxy(newValue)) { |
There was a problem hiding this comment.
[major] Self-assignment no-op breaks the capture/restore pattern
isEnvProxy(newValue) makes process.env = capturedProxy a silent no-op, so the standard isolation pattern restores nothing:
const saved = process.env; // the proxy
process.env = fake;
// ...
process.env = saved; // no-op — realEnv stays at `fake`Several tests in this PR use exactly this pattern, and dotenv-style tooling in the dev server can do the same. Track which backing environment the proxy currently fronts (or otherwise re-adopt it on self-assignment) instead of treating every proxy assignment as a no-op.
| assertNotInsideActiveScope( | ||
| "Reassigning process.env is not allowed in backend functions — it would corrupt the dev server's real environment for every future execution. Use $.Source or a declared Custom Credential instead.", | ||
| ); | ||
| sharedState.realEnv = newValue; |
There was a problem hiding this comment.
[major] Nullish assignment poisons realEnv
The setter accepts null/undefined and stores it in sharedState.realEnv — isEnvProxy's object guard only prevents throwing at assignment time, not the corruption. The next unscoped read calls Reflect.get(null, ...) and throws a native TypeError. Worse, the added test's own finally { process.env = before } restore is the self-assignment no-op from the previous comment, so the poisoning persists for the rest of the worker — this can cascade into the suite's later tests.
Reject non-object replacements outright, and have the regression test assert a subsequent read stays healthy.
…a self-assignment no-op, and unredacted writeReport output Closes three review findings on the process.env scoping guard: - currentEnv() and the environ-path checks resolved through AsyncLocalStorage.prototype.getStore and fs.realpathSync/readlinkSync dynamically, so a backend function could replace any of them to defeat scope detection or the forged-path check. Native references are now captured and bound at module load, before any customer code runs. - process.env's self-assignment branch (`process.env = capturedProxy`) was a silent no-op, breaking the standard capture/swap/restore pattern. A history stack now pops the pre-swap value on self-assignment, correctly supporting arbitrarily nested save/restore. - writeReport() with an explicit filename let Node persist the real, unredacted report to disk before it was read back and rewritten. The redacted content is now built and written directly for that case, closing the window where the real environment briefly exists on disk.
Motivation
process.env— every secret the dev server has access to, not just what that function's declared connections should see.env-guard.tscloses this with a scoped env for the duration ofrunBlocked, plus the adjacent read paths that reach the same real environment (/proc/.../environ,process.report.excludeEnv) and a Vite build-time leak (VITE_*/.envinlining into the built bundle) — see Architecture for the mechanism and the Changes table for the full file list.process.envProxy'ssettrap forwarded a mismatchedreceiverargument toReflect.set, which for an existing key falls back to a partial-descriptordefinePropertycall Node's nativeprocess.envrejects — breaking any code (dd-trace's own require-hook included) that assigns to an existing key from outside a scope. This and four related bypass gaps found in review (environ-path pid matching, aFileHandleunwrap gap, deferredexcludeEnvwrites, callback-contract violations) are fixed together since they're load-bearing for this guard's own correctness, not deferred hardening.Architecture
env-guard.tsandnetwork-guard.tsshare onegetOrCreateShared()helper (shared-module-singleton.ts) for theSymbol.for-keyed singleton pattern both need to survive being evaluated more than once (bundled copies, Jest's per-test-file isolation).env-guard.tsandnetwork-guard.tsalso share a "call through when unblocked, signal failure when blocked" wrapper in guarded-wrapper.ts:makeGuardWrapper()(generalized to take an explicitshouldBlock(...args)predicate) andmakeGuardCallbackWrapper()(the same shape for callback-style APIs).25 changes across env-guard.ts, env-guard.test.ts, guarded-wrapper.ts, network-guard.ts, shared-module-singleton.ts, local-execution.ts, local-execution.test.ts, build-config.ts, env.ts
process.envto an allowlist for the duration ofrunBlockedpackages/plugins/apps/src/vite/env-guard.ts/proc/.../environvia every fs entry point that can reach it, including a FileHandle argumentpackages/plugins/apps/src/vite/env-guard.tsprocess.report.excludeEnvagainst reassignment from inside a scopepackages/plugins/apps/src/vite/env-guard.tsprocess.envProxy'ssettrap forwarding a mismatchedreceiver, which broke assignment to an existing key from outside a scope (a real CI End-to-End failure, since dd-trace's own require-hook instrumentation hit it)packages/plugins/apps/src/vite/env-guard.ts/proc/<pid>/environregex generalized to match any accessible pid, not just self/thread-self/the dev server's ownpackages/plugins/apps/src/vite/env-guard.tsfs.promises.readFile's guard predicate now unwraps aFileHandleargument to its underlying fdpackages/plugins/apps/src/vite/env-guard.tsprocess.report.excludeEnvwrites from outside an active scope are now deferred until that scope closes, and validated immediately via Node's native setterpackages/plugins/apps/src/vite/env-guard.tsfs.readFile/open/copyFile/cpnow report a guard failure through their callback instead of throwing synchronouslypackages/plugins/apps/src/vite/env-guard.tspackages/plugins/apps/src/vite/guarded-wrapper.ts/proc/.../environblockingpackages/plugins/apps/src/vite/env-guard.test.tsSymbol.for-keyed singleton helperpackages/plugins/apps/src/vite/shared-module-singleton.tspackages/plugins/apps/src/vite/guarded-wrapper.tsnetwork-guard.tsnow uses the sharedmakeGuardWrapperinstead of its own copypackages/plugins/apps/src/vite/network-guard.tspackages/plugins/apps/src/vite/local-execution.tslazyImportOnce()helperpackages/plugins/apps/src/vite/local-execution.ts$credential-leak regression test to scan the whole object, not justSourcepackages/plugins/apps/src/vite/local-execution.test.ts.envloading andVITE_*env-prefix inlining for backend function builds — shared with the production bundling path, see Blast Radiuspackages/plugins/apps/src/vite/build-config.ts.env/VITE_*inlining fixpackages/plugins/apps/src/vite/build-config.test.tsprocess.envacross a describe block, with a defensive copy so a test mutatingprocess.envby property can't corrupt the shared baseline for later testspackages/tests/src/_jest/helpers/env.tsforceResetEnv()into the timeout/abandon path — a genuinely hung backend function previously leftprocess.report.excludeEnvarmed forever, since its scope's ownfinallynever ranpackages/plugins/apps/src/vite/local-execution.tsObject.defineProperty(process.env, key, { configurable: false })now throws a clear, guard-specific error instead of a native Proxy invariantTypeErrorpackages/plugins/apps/src/vite/env-guard.tsprocess.report.writeReport(fileName)'s non-regular-sink check now also runs immediately before the real write, narrowing a symlink-swap TOCTOU windowpackages/plugins/apps/src/vite/env-guard.tsQA Instructions
Test URL: Unit tests run for this PR's tip (
2376475f) — no browsable page exists for this build-plugin/CLI change, so this CI run is the closest equivalent.Manual QA — the
process.envProxyset-trap fix, run as two separate processes (a single combined script that installs both proxies in one process interacts across the twoObject.definePropertycalls and no longer discriminates reliably):Manual QA — a standalone script exercising
env-guard.ts's real exports directly:The 5th outcome (a built backend function bundle no longer inlines a build-machine
VITE_*value or.envfile content) is covered exactly by a dedicated automated test, added in this PR:Manual QA — a zombie execution (a backend function whose
fn()never settles) no longer leavesprocess.report.excludeEnvarmed forever:Blast Radius
runBlockedcontinuation for theprocess.env/process.report.excludeEnv//proc/.../environguarding — no change to production runtime execution (which already runs in its own Deno subprocess).envFile:false/envPrefix:[]ingetBaseBackendBuildConfigare shared with the production backend-bundle build path (build-backend-functions.ts), so uploaded production bundles also stop inlining build-machineVITE_*values and.envfiles at build time, not just in local dev. This is intentional, strictly-more-secure hardening — a server-side backend function has no legitimate use for either being statically inlined into its uploaded bundle.Documentation