[APPS-2792] Add: scope process.env to an allowlist during local execution (Secret Store parity) - #493
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2311dbe9e4
ℹ️ 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".
| const result = await runWithScopedEnv(buildScopedEnv({}), () => | ||
| runBlocked(() => fn(...args)), | ||
| ); |
There was a problem hiding this comment.
When a backend module reads an environment variable at module scope, loadModule(...) executes that customer code before runWithScopedEnv is entered, so code such as const key = process.env.DD_API_KEY can retain the dev server credential and return or use it later from the guarded function. The environment must be scoped during customer-module evaluation as well, even if Vite's own transform work needs a separate unscoped phase.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same underlying cause as the analogous, already-accepted network-guard gap in local-execution.ts (a customer module's own top-level code runs before any guard is entered, since loadModule needs real network/fs access for Vite's own transform pipeline — see that file's own doc comment). Confirmed real for env-guard too: a module-scope const key = process.env.DD_API_KEY would see the real value. Same fix shape as the network-guard case would need (scoping before loadModule, which previously broke the real dev-server integration test outright when tried for network-guard) — not attempting a different fix here without re-litigating that same tradeoff. Flagging alongside the post-timeout gap above for the #security review.
| // would otherwise leave process.env scoped for the rest of this | ||
| // process. | ||
| forceReset(); | ||
| forceResetEnv(); |
There was a problem hiding this comment.
When a customer function continues after exceeding timeoutMs (for example, it awaits a timer and then resumes), the timeout wins the race but does not cancel that function; this reset consequently exposes the real process.env to the still-running customer continuation. Such a function can simply wait past the timeout and then read DD_API_KEY, AWS credentials, or any other shell secret, defeating the new isolation guarantee.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed real, and not something this PR can close on its own. It's the same root cause as the RFC's already-accepted 'abandoned, not cancelled' execution model (Decisions and Trade-Offs, and Milestone 7's own confirmed findings): a timed-out function isn't cancelled, just abandoned, so forceResetEnv() restoring the real env for the next execution necessarily also restores it underneath the still-running old one. Closing this fully needs either true per-execution process isolation or an AsyncLocalStorage-based proxy over process.env (replacing the whole-object-swap approach here with per-access-context resolution) — both bigger than this milestone's scope, and the latter has the same category of complexity the RFC already deferred for action-catalog/apps-backend's own registration scoping. Flagging this explicitly as a residual gap for the Secret Store parity #security review this milestone already requires, rather than leaving it implicit.
There was a problem hiding this comment.
Pull request overview
Friend, this PR scopes environment variables during local backend-function execution to reduce secret exposure.
Changes:
- Adds an environment allowlist guard and reset handling.
- Integrates the guard with local execution and timeouts.
- Adds unit and integration coverage.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
env-guard.ts |
Implements environment scoping and restoration. |
env-guard.test.ts |
Tests filtering, restoration, and generation handling. |
local-execution.ts |
Applies and resets the environment guard. |
local-execution.test.ts |
Tests local-execution environment behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const result = await runWithScopedEnv(buildScopedEnv({}), () => | ||
| runBlocked(() => fn(...args)), | ||
| ); |
| const result = await runWithScopedEnv(buildScopedEnv({}), () => | ||
| runBlocked(() => fn(...args)), | ||
| ); |
There was a problem hiding this comment.
Same root question as the analogous finding on PR #484's local-execution.ts (network-guard side) — traced through carefully and it doesn't currently reproduce, for the same reason: the abandoned check runs synchronously with no await before either guard is entered. See my reply on #484 for the regression test that proves it end-to-end through both runBlocked and runWithScopedEnv together (they're nested in the same call).
2311dbe to
ffeab70
Compare
444e9a0 to
23788af
Compare
23788af to
2c0613b
Compare
170fb1e to
2565a46
Compare
2565a46 to
9597d94
Compare
9597d94 to
5769358
Compare
5769358 to
d0e7887
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
d0e7887 to
ac28c6b
Compare
4be6a31 to
0662e77
Compare
|
fb70b99 to
e7223f7
Compare
…l execution
Milestone 8 (Secret Store parity). Without this, a customer's backend
function reads the dev server's own real process.env with no
restriction at all — leaking the developer's entire shell (AWS
credentials, other API keys, the dev server's own DD_API_KEY/DD_APP_KEY)
into arbitrary, possibly third-party code. Production has no equivalent
gap: each execution gets a fresh Deno subprocess with --allow-env scoped
to exactly the resolved credential names; in-process local execution has
no process boundary to rely on, so this closes the gap at the module
level instead, mirroring network-guard.ts's monkey-patch approach and
reusing the same shared generation-counter protection against the same
abandoned-execution race class.
Also blocks reads of /proc/self/environ (and /proc/<pid>/environ) while
a scoped-env window is active: swapping the JS-level process.env object
alone isn't a real boundary on Linux, since that kernel-backed file
exposes the process's real startup environment unaffected by
reassigning process.env.
Custom Credentials resolution (e.g. STRIPE_API_KEY) remains an open,
undecided question — buildScopedEnv's customCredentials parameter is a
forward-looking extension point, always {} for now.
e7223f7 to
75fdc4b
Compare
Motivation
process.envdirectly would see the dev server's entire real environment — includingDD_API_KEY/DD_APP_KEY(the dev server's own credentials) and the developer's own unrelated shell secrets (AWS keys, etc). Production has no equivalent gap: each execution gets a fresh Deno subprocess with--allow-envscoped to exactly the resolved credential names.Architecture
env-guard.tsmirrorsnetwork-guard.ts's generation-counter design exactly: a monotoniccurrentGeneration, bumped by everyrunWithScopedEnvcall and byforceResetEnv, so a late restore from an abandoned (timed-out) execution can't clobber a newer execution's still-active scoped window.Changes
buildScopedEnv/runWithScopedEnv/forceResetEnv, scopingprocess.envtoPATH/HOME/NODE_ENV/TMPDIRfor the duration of a local execution.network-guard.test.ts's own version of this test).runWithScopedEnv/buildScopedEnvaround the customer function call inrunScriptLocally, nested with the existingrunBlockednetwork guard. AddedforceResetEnv()alongside the existingforceReset()in the timeout handler.executeScriptLocally:DD_API_KEYand an AWS-like credential are never visible to the customer function;PATH/HOME/NODE_ENV/TMPDIRremain visible; realprocess.envis restored after both success and throw.Two deliberate scope decisions, both documented in the RFC's Security section:
PATH/HOME/NODE_ENV/TMPDIRonly) — it does not include build-plugins' ownOVERRIDE_VARIABLES(DD_API_KEY,DD_APP_KEY,DD_SITE, etc.). Those are the dev server's own credentials and must never reach customer code.STRIPE_API_KEY) remains an open, undecided question —buildScopedEnvis always called with{}for now, matching production's own "skip on unresolvable, never fail the task" behavior. The parameter is a forward-looking extension point only; no local-override mechanism exists yet.QA Instructions
yarn test:unit packages/plugins/apps/src/vite/env-guard.test.ts packages/plugins/apps/src/vite/local-execution.test.ts # Expected: Test Suites: 2 passed / Tests: 45 passed ✅ VERIFIEDyarn test:unit packages/plugins/apps # Expected: Test Suites: 26 passed / Tests: 357 passed ✅ VERIFIEDyarn workspace @dd/apps-plugin run typecheck # Expected: no output, clean exit ✅ VERIFIEDReal manual QA against a scaffolded app, per the Testing and QA Guide:
Confirmed with the real dev server process holding
DD_API_KEY/AWS_SECRET_ACCESS_KEYin its actual environment (viaDD_API_KEY=... AWS_SECRET_ACCESS_KEY=... npm run dev) — neither leaked to the customer function,PATHstayed visible.Blast Radius
npm run dev:verify's cloud path — this only affects the in-process local-execution branch.runScriptLocally, and follows an already-reviewed pattern (network-guard.ts).Out of Scope / Follow-ups
$.Actions's own channel)process.envdesignDocumentation