[APPS-2792] Add: reject Node built-in imports in backend files - #476
[APPS-2792] Add: reject Node built-in imports in backend files#476tyffical wants to merge 5 commits into
Conversation
84b9e52 to
ec0f520
Compare
ec0f520 to
e17c9c8
Compare
There was a problem hiding this comment.
Pull request overview
Friend, this PR adds build-time restrictions for unsupported Node built-ins and network globals in backend functions.
Changes:
- Adds AST validation for Node built-in imports and restricted globals.
- Integrates validation into the Vite backend transform.
- Adds unit and transform-level tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
packages/plugins/apps/src/vite/index.ts |
Runs backend restrictions during transformation. |
packages/plugins/apps/src/vite/index.test.ts |
Tests transform-level built-in rejection. |
packages/plugins/apps/src/backend/ast-parsing/reject-restricted-globals.ts |
Detects unresolved restricted globals. |
packages/plugins/apps/src/backend/ast-parsing/reject-restricted-globals.test.ts |
Tests global detection and shadowing. |
packages/plugins/apps/src/backend/ast-parsing/reject-node-builtin-imports.ts |
Detects Node built-in imports. |
packages/plugins/apps/src/backend/ast-parsing/reject-node-builtin-imports.test.ts |
Tests import restrictions and exceptions. |
Suppressed comments (2)
packages/plugins/apps/src/backend/ast-parsing/reject-node-builtin-imports.ts:40
- The suggested remedy is inaccurate for non-privileged built-ins such as
path,util, orevents: an Action Platform action is not a replacement for those APIs. Mention standard JavaScript or a runtime-neutral package for portable functionality, reserving the Action Platform guidance for privileged operations, so the error remains actionable for every module this guard rejects.
`Backend functions run in a restricted environment and must use an Action ` +
`Platform action ($.Actions or an @datadog/action-catalog typed wrapper) instead: ${filePath}`,
packages/plugins/apps/src/backend/ast-parsing/reject-node-builtin-imports.test.ts:84
- Repository guidance disallows passing a function call directly into another call. Store the import declaration first so this test follows that rule.
const ast = program([importDecl(source)]);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9ef9c758b
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Suppressed comments (11)
Previously missed (3) — in code that hasn't changed since the last review.
packages/plugins/apps/src/backend/ast-parsing/reject-node-builtin-imports.test.ts:145
- This type assertion remains contrary to the repository's no-
as-casts rule and to this test helper's comment that metadata can be built without a cast. Declare the specifier with the intersection type before passing it intoimportDecl.
This issue also appears in the following locations of the same file:
- line 159
- line 225
} as ImportDeclaration['specifiers'][number] & TypeScriptImportExportMetadata,
packages/plugins/apps/src/backend/ast-parsing/reject-node-builtin-imports.ts:102
- This error is also raised for
.backend.js/.backend.jsx/.backend.tsxentries and ordinary helper modules, so saying the current file is a.backend.tsfile is inaccurate. Refer to “backend function code” (or the actual path) so the diagnostic matches every checked module.
throw new Error(
`Importing Node built-in module "${value}" is not supported in .backend.ts files. ` +
`Backend functions run in a restricted environment and must use an Action ` +
`Platform action ($.Actions or an @datadog/action-catalog typed wrapper) instead: ${filePath}`,
packages/plugins/apps/src/vite/backend-static-checks-plugin.test.ts:35
- Repository guidance disallows passing a function call directly as another call's argument. Store the mock logger in a named local before creating the plugin.
This issue also appears in the following locations of the same file:
- line 46
- line 57
- line 68
- line 79
- line 90
- ...and 1 more
const plugin = createBackendStaticChecksPlugin('/project', getMockLogger());
packages/plugins/apps/src/backend/ast-parsing/reject-node-builtin-imports.test.ts:159
- This is another
asassertion used to attach TypeScript parser metadata, despite the repository rule prohibiting assertion escape hatches. Give the type-only specifier an explicit intersection-typed local instead.
} as ImportDeclaration['specifiers'][number] & TypeScriptImportExportMetadata,
packages/plugins/apps/src/backend/ast-parsing/reject-node-builtin-imports.test.ts:225
- This re-export fixture still uses an
asassertion, which violates the repository's no-assertion rule. Define an explicitly intersection-typed specifier local and pass that to the helper.
} as ExportNamedDeclaration['specifiers'][number] & TypeScriptImportExportMetadata,
packages/plugins/apps/src/vite/backend-static-checks-plugin.test.ts:46
- Store
getMockLogger()in a local before passing it tocreateBackendStaticChecksPlugin, per the repository's no-inlined-function-call-arguments rule.
const plugin = createBackendStaticChecksPlugin('/project', getMockLogger());
packages/plugins/apps/src/vite/backend-static-checks-plugin.test.ts:57
- Avoid constructing the logger directly inside the plugin factory arguments; name the mock logger first to follow the repository rule.
const plugin = createBackendStaticChecksPlugin('/project', getMockLogger());
packages/plugins/apps/src/vite/backend-static-checks-plugin.test.ts:68
- This nested
getMockLogger()call violates the repository's no-inlined-function-call-arguments rule. Extract it to a named local.
const plugin = createBackendStaticChecksPlugin('/project', getMockLogger());
packages/plugins/apps/src/vite/backend-static-checks-plugin.test.ts:107
- Store the mock logger before calling
createBackendStaticChecksPlugin; directly nestinggetMockLogger()here violates the repository convention.
const plugin = createBackendStaticChecksPlugin('/project', getMockLogger());
packages/plugins/apps/src/vite/backend-static-checks-plugin.test.ts:79
- Create the mock logger in a named local before passing it to the plugin factory, as required by the repository guidance.
const plugin = createBackendStaticChecksPlugin('/project', getMockLogger());
packages/plugins/apps/src/vite/backend-static-checks-plugin.test.ts:90
- The logger factory call should not be nested in another function's arguments. Extract it to a named local first.
const plugin = createBackendStaticChecksPlugin('/project', getMockLogger());
…nd files Static AST checks run against every *.backend.ts file (and its nested backend-module imports) at build/dev-server time: importing a Node built-in, or referencing a network global (fetch, XMLHttpRequest, etc.) directly or via globalThis, fails the build with a clear error instead of surfacing as a runtime failure inside Datadog's execution sandbox.
Closes several ways the restricted-globals/import checks could be bypassed (named re-export, globalThis-qualified access, destructuring, computed-property access, shadowed globalThis, inline type-only specifiers, export-star re-exports) and extends coverage to nested backend-module imports reached from a *.backend.ts file. Also adds a non-fatal warning (rather than a hard reject) for crypto/Intl divergence between local dev and the production sandbox, since those globals behave close enough for most cases to not warrant blocking the build.
…coverage Treats Node's global alias the same as globalThis, rejects a literal dynamic import() of a Node built-in, extends the divergent-globals warning to helper modules the nested backend build resolves, and closes a destructuring-assignment/rest-destructure bypass of the globalThis checks. Adds a dev-server bundle-path regression test and tightens several test/production comments for clarity.
f9ef9c7 to
5de2a92
Compare
…use parse - Extract forEachAmbientGlobalAccess, a shared traversal for every syntactic form that reaches globalThis/global, used by both rejectRestrictedGlobals and warnAboutDivergentGlobals instead of two hand-mirrored copies - Recognize a no-substitution template-literal computed key (globalThis[`fetch`]) the same as a string literal, in the restricted/divergent-globals checks and the Node-builtin dynamic-import check - Resolve a const alias chain of globalThis/global (const g = globalThis) so it's no longer a silent bypass of the restricted-globals check - warnAboutDivergentGlobals now fires on a destructuring assignment (not just a declaration) off globalThis, and warns instead of silently skipping a rest-destructure, matching rejectRestrictedGlobals's existing coverage - Bound warnAboutDivergentGlobals's per-file dedup cache so a long dev-server session can't grow it unbounded - createBackendStaticChecksPlugin reuses the connection-ID collector's already-parsed AST and scope analysis instead of re-deriving both a second time for every module, per build - Replace an `as` escape-hatch cast in reject-node-builtin-imports.test.ts with typed specifier-builder helpers
…dirs on abort, and clarify error wording
`function run({ fetch } = globalThis)` reaches globalThis/global through
an AssignmentPattern, a node shape the destructure walker didn't visit —
closes it in the shared ambient-global-access helper so both the reject
and warn checks pick it up together.
buildBackendFunctions leaked its mkdtemp-created temp directory whenever
a per-function vite.build() call threw (e.g. a static check rejecting a
reachable helper module) — the temp dir was only cleaned up by the
caller on success. Cleans it up on the abort path too.
Also corrects wording in reject-restricted-globals and
reject-node-builtin-imports: both checks also run against non-.backend.ts
helper modules reachable from a backend function (via the nested
static-checks plugin), so "not supported in backend function code" is
accurate where "not supported in .backend.ts files" wasn't.
Motivation
fetch); everything must go through an Action Platform action ($.Actionsor an@datadog/action-catalogtyped wrapper).fs,child_process,net, etc.) in.backend.tsfiles are rejected at build time, so an author gets immediate, actionable feedback instead of code that silently behaves differently (or breaks) once local Node execution lands.fetch,XMLHttpRequest,WebSocket,EventSource) need a separate check: they're bare globals, not imports, so import-specifier restriction can't catch them. This closes a real trap:fetchworks fine during local dev (nothing stopped it before this check existed) but fails once the app is published, since production's sandbox blocks it. Node's ownglobalalias for the ambient global object reaches the same restricted globals and is treated identically.cryptoandIntlare legitimate, working APIs in both runtimes, but their concrete behavior (RNG implementation, bundled ICU data) isn't guaranteed identical between local execution (Node) and production (Deno) — part of the RFC's prod-parity divergence list. These get a warning, never a rejection: editor/console-time guidance pointing authors atnpm run dev:verify's real cloud round trip as the actual parity gate.fetchin the first place. That reduces how often this gets written at all, but only this build-time check guarantees it never ships, regardless of whether the code came from an AI, a human, or a copy-pasted snippet. Both layers exist for a reason — this PR isn't superseded by that guidance work.$that omit Node-specific types) is deferred — see Out of Scope below.Changes
rejectNodeBuiltinImports, which walks a.backend.tsfile's staticImportDeclarations and throws if any source is a Node built-in (vianode:prefix or Node's ownbuiltinModuleslist).rejectRestrictedGlobals, an eslint-scope-based check that throws on any unshadowed reference tofetch/XMLHttpRequest/WebSocket/EventSource— i.e. any reference that doesn't resolve to a local declaration or import sharing the same name, meaning it falls through to the real ambient global.rejectNodeBuiltinImports' doc comment and error message, which previously pointed to fetch-based/isomorphic APIs as the allowed escape hatch — no longer accurate now thatfetchitself is blocked too.this.parse(code)and before export extraction.node:fs, barefs,child_process,net,fs/promises), and edge cases (type-only imports, non-import statements).fetch()calls, referencingfetchwithout calling it,new XMLHttpRequest()/WebSocket()/EventSource()), and allowed cases (an imported action-catalog function, a locally-declared function or parameter that happens to be namedfetch— shadowing-safe)..backend.tsfile with anode:fsimport through the actual transform handler (using rollup's realparseAst, not a hand-built AST) to confirm the rejection fires through the genuine pipeline.warnAboutDivergentGlobals, which warns (once per distinct global per file, never rejects) oncrypto/Intlreferences — bare,globalThis-qualified, or destructured, including multi-property and computed-key destructures, with the same eslint-scope shadowing check as the two reject-style helpers.rejectRestrictedGlobalsnow treats Node'sglobalalias the same asglobalThisin both qualified-access forms (global.fetch, destructuring offglobal).rejectNodeBuiltinImportsnow also rejects a literal dynamicimport('node:fs'), which Rollup represents as anImportExpressionrather than a top-levelImportDeclaration.createBackendStaticChecksPlugin, a nested Vite plugin that re-runs all three static checks (including the new divergent-globals warning) against every app-local module the backend build resolves — not just the.backend.tsentry the outer transform hook sees — so an imported helper can't ship an undetected Node built-in, restricted global, or divergence warning. Wired into both the production build and the dev server's own bundling path.forEachAmbientGlobalAccess, a shared traversal for every syntactic form that reachesglobalThis/global(bare reference, qualified member access, destructuring), used by bothrejectRestrictedGlobalsandwarnAboutDivergentGlobalsso a bypass fix lands once instead of drifting between two hand-mirrored copies.globalThis[\`fetch\`]), and aconstalias ofglobalThis/global— including a chain of aliases — closing two bypasses a code-review pass found in the string-literal-only, no-alias-tracking version of these checks.warnAboutDivergentGlobalsnow also fires on a destructuring assignment (not just a declaration) offglobalThis, and warns — rather than silently skipping — a rest-destructure, matching coveragerejectRestrictedGlobalsalready had for both shapes. Its per-file dedup cache is now bounded so a long dev-server session can't grow it forever.createBackendStaticChecksPluginnow reuses the connection-ID collector's already-parsed AST and scope analysis for a module instead of parsing and scope-analyzing it a second time.QA Instructions
Build the plugin and link it into a scratch Vite project, then confirm a backend file importing a Node built-in — or referencing
fetch— is rejected while an ordinary backend file still transforms correctly.Blast Radius
.backend.tsfiles and the local helper modules they import. No feature flag — the Node-builtin and restricted-globals checks are build-time compile errors for patterns that wasn't previously usable in production anyway, since production's real sandbox already blocks both; thecrypto/Intlcheck only ever logs a warning, never fails a build.importspecifiers and a dynamicimport()whose specifier is a string or no-substitution template literal, but notrequire()or a specifier computed at runtime; the global-reference checks resolve a bare reference, aglobalThis/global-qualified access (including aconstalias chain), and destructuring, but not a reference reached through alet/reassignable binding or a fully dynamic (non-literal) computed key.global, or via a literal dynamic import). Files that referencecrypto/Intlnow get an additional warn-level log line — no build failure, no output change..backend.tsfile with no exports that also imports a banned module now hard-fails the build instead of just warning-and-stripping — catching the banned pattern as soon as it's written rather than waiting for the file to also gain an export.Out of Scope / Follow-ups
backend-function-globals.d.ts(ambient TypeScript type for$that omitsDeno/process/Node-builtin globals).d.tsinto the publisheddist/tarball requires new build-tooling wiring inpackages/tools/src/rollupConfig.mjs(shared by all 5 published bundler plugins), which is disproportionate scope for this PR. Revisit once a scaffold tool exists to actually wire the type into a consumer'stsconfig.json.fetch; not blocking today's v1 rolloutDocumentation
.plans/high-code-apps-local-node-execution-design.md(dd-source repo)