[APPS-2792] Clean up: revert process.env scoping and the Custom Credentials local-file mechanism - #523
Conversation
tyffical
changed the title
PR #523 reverts both mechanisms entirely, so there's no local-execution behavior change left to document for either.
tyffical
changed the title
tyffical
changed the title
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.
Reverting PR #504/#510/#512 wholesale also removed several unrelated protections that happened to live in the same diffs: - build-config.ts's envFile:false/envPrefix:[] block Vite's own static .env-inlining into the production backend bundle — nothing to do with local execution's runtime env scoping. - The Custom Credentials filename deny-list entries (build-package.ts's archive exclusion, index.ts's server.fs.deny and direct-import rejection) protect a leftover datadog-app.local.json a developer already created — removing the resolver doesn't retroactively make that file safe to package, serve, or import. - network-guard.ts's getSharedContext regressed to exposing the raw AsyncLocalStorage instance via a Symbol on the net module instead of a restricted {isActive, run} facade, letting any code with require('net') call .disable() on it and permanently kill network blocking for the rest of the process. Caught by Copilot's review of #523.
Reverting PR #504/#510/#512 wholesale also removed several unrelated protections that happened to live in the same diffs: - build-config.ts's envFile:false/envPrefix:[] block Vite's own static .env-inlining into the production backend bundle — nothing to do with local execution's runtime env scoping. - The Custom Credentials filename deny-list entries (build-package.ts's archive exclusion, index.ts's server.fs.deny and direct-import rejection) protect a leftover datadog-app.local.json a developer already created — removing the resolver doesn't retroactively make that file safe to package, serve, or import. - network-guard.ts's getSharedContext regressed to exposing the raw AsyncLocalStorage instance via a Symbol on the net module instead of a restricted {isActive, run} facade, letting any code with require('net') call .disable() on it and permanently kill network blocking for the rest of the process. Caught by Copilot's review of #523.
7675588 to
6692a7b
Compare
tyffical
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
4eb37ce to
e723d79
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ec51a00a8
ℹ️ 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".
| projectRoot, | ||
| timeoutMs, | ||
| ); | ||
| await Promise.all([actionCatalogRegistration, backendRuntimeRegistration]); |
There was a problem hiding this comment.
When either adapter is installed, these promises execute project-resolved package code before runBlocked begins, so module-initialization code in the adapter or its dependencies can make unrestricted network or subprocess calls while reading the newly exposed ambient environment. The fresh evidence in the reviewed tree is that the registrations are still awaited here, while runBlocked is not entered until line 874, despite the earlier thread saying this ordering was fixed; retain the registrations inside the blocked callback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changed, deliberately: the registration reuses the adapter module instance the customer's backend file has usually already loaded at top level, outside the guard (an accepted, documented gap for module top-level code), so running registration inside runBlocked protects little. It can also block Vite's or a project plugin's own work during that load, making registration fail and retry on every execution. The earlier reply on the other thread said it was moved inside; that was reverted on purpose afterwards and the reply is now corrected. Leaving this open for a human reviewer to confirm the trade-off.
🤖 Addressed by Claude Code
|
Do we need to retain the protections for leftover Separately, should we support standard |
yeah i agree with removing them - i don't think i found any usage. agents kept flagging it so i put it back to be safe but with your agreement i'll remove them fully.
i do support this design generally. i believe I tried this for this PR initially but there was a conflict with Vite - I will re-investigate this! |
envFile: false / envPrefix: [] block Vite's own build-time .env-file/import.meta.env machinery from copying a VITE_-prefixed real process.env value, or a value from a build-root .env file, straight into the production backend bundle. This has nothing to do with local execution's runtime process.env scoping — it protects the same thing Vite's own docs warn about (define/import.meta.env being statically inlined at build time) and was dropped as unrelated collateral damage when #504 was reverted wholesale.
…by the revert The wholesale revert of #504 re-inlined getSharedContext's registry lookup without the protections its deleted helper provided: - The lookup is an own-property check, so a value inherited from net's prototype chain can't be mistaken for an installed entry and skip real installation. - The registry stores a frozen isActive()/run() facade instead of the raw AsyncLocalStorage, so code with require('net') can't .disable() it or reassign its methods. - Shared-context keys are versioned, so a copy from another plugin release (which may store a raw AsyncLocalStorage under the old keys) can't make this one throw at load. The install marker stays unversioned so the other copy's non-configurable accessors are recognized instead of redefined. Also restores coverage the revert dropped (the action-catalog typed-wrapper toJSON() exfiltration test and the full-$ credential exposure scan), adds tests pinning adapter registration outside the blocked scope (its loadModule() is Vite's own resolution, not customer code) and skipped entirely for an execution abandoned during its own module load, restores the busy-loop resilience test's timing margins and the config-hook test's cast-free helper, and updates comments still describing the reverted mechanisms. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
9ec51a0 to
8ff610f
Compare
…2792-revert-custom-credentials-file # Conflicts: # packages/plugins/apps/src/vite/index.ts
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🔗 Commit SHA: f58889c | Docs | View more details | Give us feedback! |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 127af80ef7
ℹ️ 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 frontendAssets = assets | ||
| .filter((asset) => !generatedPaths.has(path.resolve(asset.absolutePath))) | ||
| .filter((asset) => !backendPaths.has(asset.absolutePath)); | ||
| const credentialsIdentity = await resolveCredentialsIdentity(buildRoot); | ||
| const nonCredentialsAssets = ( | ||
| await Promise.all( | ||
| candidateAssets.map(async (asset) => ({ | ||
| asset, | ||
| isCredentialsAsset: await isCustomCredentialsAsset( | ||
| asset.absolutePath, | ||
| credentialsIdentity, | ||
| ), | ||
| })), | ||
| ) | ||
| ).filter(({ isCredentialsAsset }) => !isCredentialsAsset); | ||
| const frontendAssets = nonCredentialsAssets.map(({ asset }) => ({ | ||
| ...asset, | ||
| relativePath: `frontend/${asset.relativePath}`, | ||
| })); | ||
| .filter((asset) => !backendPaths.has(asset.absolutePath)) | ||
| .map((asset) => ({ |
There was a problem hiding this comment.
Projects upgrading from the previous release may still contain the documented datadog-app.local.json with real secrets, and when apps.include matches JSON files this now maps that file directly into frontend/datadog-app.local.json in the deployable archive. Removing the credential resolver should not remove the packaging denylist for this legacy filename (including aliases such as symlinks/hardlinks), or an otherwise routine upgrade and build can upload credentials.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this. Only the 3.4.0-dev.1 prerelease (npm dev tag) ever read datadog-app.local.json, so the maintainers decided on this PR to drop the name-based protections for it rather than keep them indefinitely. A leftover copy reaches the package only if the project's own apps.include matches it.
🤖 Addressed by Claude Code
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
sdkennedy2
left a comment
sdkennedy2
left a comment
There was a problem hiding this comment.
Verified the feedback: the datadog-app.local.json guards, warning, and archive exclusion have been removed. Backend env-file and env-inlining build protections remain. Standard .env support is handled separately in the stacked follow-up #539. The focused build-config and local-execution suites pass (106 tests), and CI unit tests, end-to-end tests, and linting are green.
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: queued
mergequeue-status: in_progress
and removed
mergequeue-status: queued
labels
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
Motivation
process.envto a from-scratch allowlist (SAFE_ENV_KEYS) during local execution — aProxyinstalled onprocess.envitself, scoped viaAsyncLocalStorage, hardened against bypasses in #510. #512 then added local-file resolution for Custom Credentials (datadog-app.local.json) on top of that allowlist.git show 8f9d3486^:...) that nothing from [APPS-2792] Add: wire local execution into the real dev server #481/[APPS-2792] Add: runtime network/subprocess guard for local execution #484's own dev-server/network-guard wiring is lost in the process — the registration calls this revert relocates were already positioned exactly there before [APPS-2792] Add process.env scoping for local execution (Secret Store parity) #504 ever nested them inside the env scope.network-guard.ts's independentnet/fetch/child_processblocking ([APPS-2792] Add: runtime network/subprocess guard for local execution #484) is untouched — this only removes theprocess.envscoping layer.MIGRATIONS.md's "v3 to v4" entry became obsolete as a result (every subsection there documented one of the two reverted mechanisms) — dropped separately in PR #521 (merged).network-guard.ts's shared-context hardening. Both are restored here with their regression coverage.datadog-app.local.jsonis no longer read, and no protections for it remain: only the3.4.0-dev.1prerelease (npmdevtag) ever read it.Reviewing this PR
5 commits:
process.envallowlist itselfbuild-config.ts'senvFile/envPrefix, dropped by reverting [APPS-2792] Add process.env scoping for local execution (Secret Store parity) #504 wholesaleisActive/runfacade, versioned registry keys; restored action-catalog and credential-exposure tests; tests pinning adapter registration outside the blocked scope and skipped for an abandoned execution; an ambient-process.envtest; local execution refuses to run when another release's guard is already installedChanges
9 changes across env-guard.ts, network-guard.ts, local-execution.ts, and supporting files
process.envProxy/AsyncLocalStoragescoping mechanism,SAFE_ENV_KEYS, the/proc/.../environandFileHandlebypass guards,process.reportredactionenv-guard.ts,env-guard.test.tsguarded-wrapper.ts,shared-module-singleton.ts(+ their tests)getSharedContextwith an own-property lookup, a frozen facade and versioned registry keysnetwork-guard.ts,network-guard.test.tsbuildScopedEnv/runWithScopedEnv/resolveCustomCredentialscall sites; adapter registration runs outside the blocked scope, with an abandonment check before and after itlocal-execution.ts,local-execution.test.ts,local-execution.resilience.test.tscustom-credentials-resolver.ts,custom-credentials-resolver.test.tsindex.ts,index.test.ts,build-package.ts,src/index.test.ts,README.mdnetwork-guard.ts,local-execution.ts(+ tests)VITE_*env-inlining guardbuild-config.ts,build-config.test.tspackages/tests/src/_jest/helpers/env.ts(+ test)QA Instructions
Manual QA — real dev server against staging (dd.datad0g.com)
vite.config.tsimports this branch's built plugin (packages/published/vite-plugin/dist/src/index.mjs, rebuilt at 8ff610f), launched with staging credentials:POST /__dd/executeActionwith{"functionName": "<sha256('src/<file>')>.<fn>", "args": []}, after the page has loaded each.backend.tsmodule (registration is lazy):Blast Radius
npm run devbackend functions now see the real, ambientprocess.envof the dev server process — no allowlist, no Custom Credentials merge. This matches the pre-project baseline and the behavior of the rest of the Node ecosystem (Next.js, Vite, CRA all work this way), per the team's decision above.npm run dev. Production continues to resolve declared connections server-side viaresolveCustomCredentialEnvConnections/ResolveConnectionToCredential, unaffected by anything in local execution.datadog-app.local.jsonleft over from3.4.0-dev.1is ignored like any other project file.Out of Scope / Follow-ups
2 items deferred
as unknown as Fremains in the generic guard wrappers.Documentation
MIGRATIONS.mdentry separately (merged)🤖 Generated with Claude Code