[APPS] Emit backend connection ID manifests#330
[APPS] Emit backend connection ID manifests#330sdkennedy2 wants to merge 5 commits intographite-base/330from
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a5973a50a
ℹ️ 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".
| if (node.type !== 'ExportNamedDeclaration' || !node.declaration) { | ||
| continue; |
There was a problem hiding this comment.
Handle supported export specifiers in the manifest
When a backend file uses the already-supported function foo() { ... }; export { foo } or export { foo as bar } form, extractExportedFunctions still registers the export, but this helper skips ExportNamedDeclarations without an inline declaration, leaving bodyByExport empty; the loop then records allowedConnectionIds: [] for that function even if its body calls request({ connectionId: '...' }). In that scenario the uploaded backend/manifest.json omits the connection IDs for a function that is still bundled and callable, so the server-side allowlist is wrong.
Useful? React with 👍 / 👎.
| if (node.type === 'CallExpression') { | ||
| const firstArg = node.arguments[0]; | ||
| if (firstArg && firstArg.type === 'ObjectExpression') { | ||
| const prop = findConnectionIdProp(firstArg); |
There was a problem hiding this comment.
Restrict connection-id extraction to request calls
Because every CallExpression with an object first argument is collected, any unrelated call inside an exported backend function such as logger.info({ connectionId: process.env.CONN }) or track({ connectionId: userId }) now either fails the build or adds unrelated IDs to the manifest, even though the manifest is documented for request({ connectionId }) call sites. Check that the callee is the backend request API before reading this property.
Useful? React with 👍 / 👎.
| for (const node of ast.body) { | ||
| if (node.type === 'VariableDeclaration') { | ||
| for (const d of node.declarations) { | ||
| if (d.id.type === 'Identifier' && d.init) { | ||
| localConsts.set(d.id.name, d.init); |
There was a problem hiding this comment.
Reject mutable connection-id bindings
These entries include let and var declarations, so let CONN = 'a'; CONN = getTenantConn(); request({ connectionId: CONN }) is treated as a static 'a' allowlist entry even though the runtime value can change. Since the manifest is used as an allowlist and the public contract above only permits constants, mutable bindings should be rejected instead of resolved from their initializer.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60085e1b61
ℹ️ 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".
| if (!body) { | ||
| // Export was declared but we couldn't locate its function body. | ||
| // This is a shape discovery already rejects — defensive empty entry. | ||
| result.set(name, []); |
There was a problem hiding this comment.
Preserve connection IDs for specifier exports
When a backend file uses the already-supported specifier form (function foo(){ request({ connectionId: 'c' }); } export { foo }, or an alias/import relay), extractExportedFunctions still returns foo but findExportedFunctionBodies never records a body, so this branch emits an empty allowlist. The backend bundle still invokes request with that connection ID, but backend/manifest.json omits it and production execution will be rejected by the server allowlist.
Useful? React with 👍 / 👎.
| if (d.id.type === 'Identifier' && d.init) { | ||
| localConsts.set(d.id.name, d.init); |
There was a problem hiding this comment.
Reject mutable connection ID bindings
When connectionId resolves through a let or var, this stores only the initializer and ignores later assignments (let id = 'a'; id = 'b'; request({ connectionId: id })). The manifest then allowlists a even though runtime can use b, leading to the same server rejection this manifest is meant to avoid; only const bindings should be resolved, with mutable bindings rejected.
Useful? React with 👍 / 👎.
…urce of truth for export shapes; let extract-connection-ids reuse it
e430250 to
3d1a731
Compare
60085e1 to
d304370
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d304370c62
ℹ️ 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".
| } | ||
| }; | ||
|
|
||
| for (const node of ast.body) { |
There was a problem hiding this comment.
Handle scoped const connection IDs
Because the symbol table is built only from ast.body, any const declared inside the backend function or an imported helper is invisible to resolveIdentifier. For example, export function foo() { const ID = 'abc'; request({ connectionId: ID }); } is a fully static connection ID, but the transform now throws identifier 'ID' is not defined; if a top-level ID also exists, the manifest can record the wrong ID instead of the scoped one.
Useful? React with 👍 / 👎.
| if (prop.type !== 'Property' || prop.computed) { | ||
| continue; |
There was a problem hiding this comment.
Reject option spreads carrying connectionId
When an action options object supplies connectionId via a spread, e.g. const opts = { connectionId: 'abc' }; request({ ...opts, url: '/x' }), this branch silently skips the SpreadElement and the manifest omits the ID. The backend bundle is still uploaded/executed with an empty allowlist, so these otherwise valid calls fail at runtime instead of failing the build with an unsupported-form error or resolving the spread.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90922148f4
ℹ️ 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 localInit = mod.symbols.localConsts.get(name); | ||
| if (localInit) { | ||
| return resolveValue(ctx, localInit, mod, state); | ||
| } |
There was a problem hiding this comment.
Respect local scope when resolving connection IDs
When a backend action call uses an identifier that is shadowed inside the function body, this module-level lookup resolves the outer const instead of the value actually passed at runtime. For example, const CONN = 'dev'; export function foo(CONN) { request({ connectionId: CONN }); } will emit dev rather than failing on the dynamic parameter, so the uploaded manifest can omit the real connection and the function will be denied in production. The same scope issue applies to shadowed action imports/local variables, so the walker needs lexical-scope awareness before resolving identifiers from call sites.
Useful? React with 👍 / 👎.

Motivation
High-code app backend functions can call action-catalog actions that require a
connectionId. The server-side App Builder runtime rejects those calls unless the backend function's step configuration includes the connection in its allowed list.The first version of this PR extracted direct
connectionIdliterals from each exported backend function body. That was useful, but too narrow for normal app code organization: backend functions often delegate to local helpers, and those helpers import the action-catalog function and the connection constant.Changes
This PR emits a backend connection manifest and now derives each backend file's allowlist from a bounded local module graph rather than only from direct calls inside an export body.
For every transformed
*.backend.*entry file, the apps plugin now:@datadog/action-catalog/*as action call bindings.estree-walkerfor action-catalog calls whose first argument containsconnectionId.constbindings, imported/exported const bindings, re-export chains, and simple object-member access likeCONNECTIONS.HTTP.Example supported shape:
Dynamic or unsafe forms still fail closed with a build-time error instead of silently producing a too-broad manifest. Unsupported examples include dynamic local imports, local
require, mutablelet/varconnection IDs, dynamic templates, computed object properties, and object spreads.The dev server preview path now forwards the same per-function
allowedConnectionIdsintopreview-asyncso local browser testing exercises the same allowlist behavior as production upload manifests.QA Instructions
Automated checks:
Manual test app verification with
/Users/scott.kennedy/dd/test-action-catalog-app:@datadog/vite-pluginto this branch'spackages/published/vite-plugin.requestfrom@datadog/action-catalog/http/httpand usesCONNECTIONS.HTTP.CONNECTIONS.HTTPto77c14b8b-27e1-4901-985d-8817908b9706andurltohttps://postman-echo.com/get.npm run typecheckin the test app.npm run buildand confirmed the debug manifest includes the module-graph backend function withallowedConnectionIds: ["77c14b8b-27e1-4901-985d-8817908b9706"].NODE_TLS_REJECT_UNAUTHORIZED=0 dd-auth --domain="dd.datad0g.com" --actions-api -- npm run dev -- --host 127.0.0.1 --port 5177and clicked the Playwright-visible probe button.The browser test no longer failed with
Access denied for connection 77c14b8b-27e1-4901-985d-8817908b9706. It progressed to Datadog's action-policy error:Execution blocked for restricted action type: http.request, which means the connection allowlist was accepted and the remaining blocker is server-side policy for executing that action type in this preview context.Blast Radius
This affects high-code-app builds and dev execution through
@datadog/vite-pluginwithapps.enable.The behavior is intentionally conservative: if the module graph contains a statically visible action-catalog call with an unresolvable
connectionId, the build fails instead of producing an unsafe or incomplete allowlist. Backend files without connection-using action calls continue to emit an empty allowlist.Dev server execution now sends the same static allowlist shape as uploaded backend manifests, which should make local preview failures closer to production execution failures.
Documentation