From 41dda465301d9d7011d04bc1345f7cbacd7be285 Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Fri, 23 Oct 2026 11:55:32 -0400 Subject: [PATCH 1/2] fix(apps): fail closed on connection IDs from objects written by calls or aliases The connection ID extractor doesn't read a member of an object that a known mutator writes to: Object.assign, defineProperty, defineProperties and setPrototypeOf, and Reflect.set (its receiver, when one is given), defineProperty, deleteProperty and setPrototypeOf, including one named through a const-aliased computed key such as `Object[method]`. The same holds when the object is written through an alias, such as `const gh = CONN.gh`, `gh = CONN.gh`, `const gh = CONN?.gh || {}`, `cur ||= {}`, a member of a rest copy (including one destructured from it), or a `const`, `let` or destructured alias its module exports by name. Such a connectionId takes the unsupported-connectionId path instead of reporting a stale ID. Action options with a toJSON or then key, or a __proto__ key other than a shorthand or null one, are rejected, since each can change the connectionId the call sends. Each extraction walks the module graph for writes at most once. An object that is only read, logged, copied (including a write to a rest copy's own properties) or used to call a method still resolves, as does one whose alias is rebound without being written, or one passed to a local function that shadows Object or Reflect. Writes through `this` in the object's own methods, through a function, container or awaited value the object is passed to, through a default or namespace import of it, or through an aliased call to a mutator are not tracked; the server's allowlist still refuses the resulting connection ID. Co-Authored-By: Claude Opus 5.5 --- .../ast-parsing/ambient-global-access.ts | 2 +- ...t-connection-ids-from-module-graph.test.ts | 438 ++++++++++++++++++ ...xtract-connection-ids-from-module-graph.ts | 43 +- .../src/backend/ast-parsing/module-graph.ts | 271 ++++++++--- .../ast-parsing/static-string-resolution.ts | 170 ++++--- .../src/backend/ast-parsing/type-guards.ts | 17 +- 6 files changed, 816 insertions(+), 125 deletions(-) diff --git a/packages/plugins/apps/src/backend/ast-parsing/ambient-global-access.ts b/packages/plugins/apps/src/backend/ast-parsing/ambient-global-access.ts index eada6f133..718d8f6ed 100644 --- a/packages/plugins/apps/src/backend/ast-parsing/ambient-global-access.ts +++ b/packages/plugins/apps/src/backend/ast-parsing/ambient-global-access.ts @@ -109,7 +109,7 @@ function resolvesToAmbientGlobal( return false; } -function staticMemberName( +export function staticMemberName( node: MemberExpression, scopeAnalysis: ModuleScopeAnalysis, ): string | undefined { diff --git a/packages/plugins/apps/src/backend/ast-parsing/extract-connection-ids-from-module-graph.test.ts b/packages/plugins/apps/src/backend/ast-parsing/extract-connection-ids-from-module-graph.test.ts index 7f60d3eea..788cfc83c 100644 --- a/packages/plugins/apps/src/backend/ast-parsing/extract-connection-ids-from-module-graph.test.ts +++ b/packages/plugins/apps/src/backend/ast-parsing/extract-connection-ids-from-module-graph.test.ts @@ -8,6 +8,21 @@ import { parseAst } from 'rollup/parseAst'; import { extractConnectionIdsFromModuleGraph } from './extract-connection-ids-from-module-graph'; import { createParsedModuleRecord, type ParsedModuleRecord } from './module-graph'; +import { collectMutatedImportedObjectVariables } from './static-string-resolution'; + +jest.mock('./static-string-resolution', () => { + const actual = jest.requireActual( + './static-string-resolution', + ); + return { + ...actual, + collectMutatedImportedObjectVariables: jest.fn( + actual.collectMutatedImportedObjectVariables, + ), + }; +}); + +const collectMutatedMock = jest.mocked(collectMutatedImportedObjectVariables); const buildRoot = '/project'; const entryId = '/project/src/backend/actions.backend.js'; @@ -746,4 +761,427 @@ describe('Backend Functions - extractConnectionIdsFromModuleGraph', () => { expect(extract([entry, helper])).toEqual(['conn-ts']); }); + + const ACTION_CATALOG_HTTP = '@datadog/action-catalog/http/http'; + const LONG_ALIAS_CHAIN = Array.from({ length: 3000 }, (_, i) => `let a${i + 1} = a${i};`).join( + ' ', + ); + + const createObjectConnectionIdEntry = (statements: string, connectionId = 'CONN.github') => + createRecord( + entryId, + ` + import { request } from '${ACTION_CATALOG_HTTP}'; + + const CONN = { + github: 'conn-github', + gh: { id: 'conn-gh' }, + ready: Promise.resolve({}), + someMethod() { return 1; }, + }; + ${statements} + + export function run() { + return request({ connectionId: ${connectionId}, inputs: {} }); + } + `, + [ACTION_CATALOG_HTTP], + ); + + test.each([ + { description: 'Object.assign', statements: "Object.assign(CONN, { github: 'other' });" }, + { + description: 'Object.defineProperty', + statements: "Object.defineProperty(CONN, 'github', { value: 'other' });", + }, + { + description: 'Object.defineProperties', + statements: "Object.defineProperties(CONN, { github: { value: 'other' } });", + }, + { description: 'Object.setPrototypeOf', statements: 'Object.setPrototypeOf(CONN, null);' }, + { description: 'Reflect.set', statements: "Reflect.set(CONN, 'github', 'other');" }, + { + description: 'Reflect.defineProperty', + statements: "Reflect.defineProperty(CONN, 'github', { value: 'other' });", + }, + { + description: 'Reflect.deleteProperty', + statements: "Reflect.deleteProperty(CONN, 'github');", + }, + { + description: 'Reflect.set with a spread where the receiver would be', + statements: "const extra = []; Reflect.set(CONN, 'github', 'other', ...extra);", + }, + { + description: 'Object.assign called with a computed key', + statements: "Object['assign'](CONN, { github: 'other' });", + }, + { + description: 'Object.assign called with a const-aliased computed key', + statements: "const method = 'assign'; Object[method](CONN, { github: 'other' });", + }, + { + description: 'Reflect.setPrototypeOf', + statements: 'Reflect.setPrototypeOf(CONN, null);', + }, + { + description: 'Object.assign on one of its members', + statements: "Object.assign(CONN.gh, { id: 'other' });", + connectionId: 'CONN.gh.id', + }, + { + description: 'a write through an alias', + statements: "const alias = CONN; alias.github = 'other';", + }, + { + description: 'a write through an alias of one of its members', + statements: "const gh = CONN.gh; gh.id = 'other';", + connectionId: 'CONN.gh.id', + }, + { + description: 'a write through an alias declared in a function', + statements: "function rename() { const alias = CONN; alias.github = 'other'; }", + }, + { + description: 'a write through a destructured member alias', + statements: "const { gh } = CONN; gh.id = 'other';", + connectionId: 'CONN.gh.id', + }, + { + description: 'a write through an alias assigned after its declaration', + statements: "let gh; gh = CONN.gh; gh.id = 'other';", + connectionId: 'CONN.gh.id', + }, + { + description: 'Reflect.set with it as the receiver', + statements: "Reflect.set({}, 'github', 'other', CONN);", + }, + { + description: 'a write through an optional-chaining alias', + statements: "const gh = CONN?.gh; gh.id = 'other';", + connectionId: 'CONN.gh.id', + }, + { + description: 'a write through an alias with a fallback', + statements: "const gh = CONN.gh || {}; gh.id = 'other';", + connectionId: 'CONN.gh.id', + }, + { + description: 'a write through a conditional alias', + statements: "const gh = Math.random() > 0.5 ? CONN.gh : null; gh.id = 'other';", + connectionId: 'CONN.gh.id', + }, + { + description: 'a write through a chained-assignment alias', + statements: "let a, b; a = b = CONN; a.github = 'other';", + }, + { + description: 'a write to a member of a rest copy', + statements: "const { github, ...rest } = CONN; rest.gh.id = 'other';", + connectionId: 'CONN.gh.id', + }, + { + description: 'a write through a destructured member of a rest copy', + statements: "const { github, ...rest } = CONN; const { gh } = rest; gh.id = 'other';", + connectionId: 'CONN.gh.id', + }, + { + description: 'a write at the end of a long alias chain', + statements: `let a0 = CONN; ${LONG_ALIAS_CHAIN} a3000.github = 'other';`, + }, + { + description: 'a write through a ||= alias', + statements: "let cur = CONN.gh; (cur ||= {}).id = 'other';", + connectionId: 'CONN.gh.id', + }, + { + description: 'a write through a ??= alias', + statements: "let cur = CONN.gh; const gh = (cur ??= {}); gh.id = 'other';", + connectionId: 'CONN.gh.id', + }, + { + description: 'a write to a member of a fallback expression', + statements: "(CONN.gh || {}).id = 'other';", + connectionId: 'CONN.gh.id', + }, + { + description: 'Object.assign on an alias', + statements: "const alias = CONN; Object.assign(alias, { github: 'other' });", + }, + ])( + 'Should fail closed for an object connection ID mutated by $description', + ({ statements, connectionId }) => { + const entry = createObjectConnectionIdEntry(statements, connectionId); + + expect(() => extract([entry])).toThrow('mutated object binding'); + }, + ); + + test.each([ + { description: 'logged', statements: 'console.log(CONN);' }, + { description: 'passed to Object.keys', statements: 'Object.keys(CONN);' }, + { description: 'passed to JSON.stringify', statements: 'JSON.stringify(CONN);' }, + { description: 'copied by Object.assign', statements: 'Object.assign({}, CONN);' }, + { description: 'aliased without a write', statements: 'const alias = CONN;' }, + { + description: 'read through an unwritten alias', + statements: 'const alias = CONN;', + connectionId: 'alias.github', + }, + { description: 'used to call one of its methods', statements: 'CONN.someMethod();' }, + { + description: 'assigned to an alias that is never written', + statements: 'let gh; gh = CONN.gh;', + }, + { + description: 'destructured into a rest copy that is written', + statements: 'const { gh, ...rest } = CONN; rest.extra = 1;', + }, + { + description: 'passed to a local function named Object', + statements: 'function copy(Object) { Object.assign(CONN, {}); }', + }, + { + description: 'holding a promise whose resolved value is written', + statements: 'async function load() { const result = await CONN.ready; result.x = 1; }', + }, + { + description: 'passed to Reflect.set as the target with another receiver', + statements: "const OTHER = {}; Reflect.set(CONN, 'github', 'other', OTHER);", + }, + { + description: 'guarding a write to another object with &&', + statements: 'const OTHER = {}; const target = CONN.gh && OTHER; target.y = 2;', + }, + ])( + 'Should still resolve an unmutated object connection ID $description', + ({ statements, connectionId }) => { + const entry = createObjectConnectionIdEntry(statements, connectionId); + + expect(extract([entry])).toEqual(['conn-github']); + }, + ); + + const CONNECTIONS_ID = '/project/src/backend/connections.js'; + const CONNECTIONS_CODE = ` + export const CONN = { github: 'conn-github', gh: { id: 'conn-gh' } }; + export const ALIAS = CONN; + export let MUTABLE_ALIAS = CONN.gh; + export const { gh: DESTRUCTURED_ALIAS } = CONN; + export const { github: GITHUB_ID, ...REST } = CONN; + `; + + const extractWithImportedConnections = (statements: string, connectionId = 'CONN.github') => { + const entry = createRecord( + entryId, + ` + import { request } from '${ACTION_CATALOG_HTTP}'; + import { + CONN, + ALIAS, + MUTABLE_ALIAS, + DESTRUCTURED_ALIAS, + REST, + } from './connections.js'; + + ${statements} + + export function run() { + return request({ connectionId: ${connectionId}, inputs: {} }); + } + `, + [ACTION_CATALOG_HTTP, CONNECTIONS_ID], + ); + const connections = createRecord(CONNECTIONS_ID, CONNECTIONS_CODE); + return extract([entry, connections]); + }; + + test.each([ + { description: 'Object.assign', statements: "Object.assign(CONN, { github: 'other' });" }, + { + description: 'Reflect.set with it as the receiver', + statements: "Reflect.set({}, 'github', 'other', CONN);", + }, + { + description: 'a write through an alias its module exports', + statements: "ALIAS.github = 'other';", + }, + { + description: 'a write through a let alias its module exports', + statements: "MUTABLE_ALIAS.id = 'other';", + connectionId: 'CONN.gh.id', + }, + { + description: 'a write through a destructured alias its module exports', + statements: "DESTRUCTURED_ALIAS.id = 'other';", + connectionId: 'CONN.gh.id', + }, + { + description: 'a write to a member of a rest copy', + statements: "const { github, ...rest } = CONN; rest.gh.id = 'other';", + connectionId: 'CONN.gh.id', + }, + { + description: 'a write through a destructured member of a rest copy', + statements: "const { github, ...rest } = CONN; const { gh } = rest; gh.id = 'other';", + connectionId: 'CONN.gh.id', + }, + { + description: 'a write at the end of a long alias chain', + statements: `let a0 = CONN; ${LONG_ALIAS_CHAIN} a3000.github = 'other';`, + }, + { + description: 'a write through an alias assigned after its declaration', + statements: "let gh; gh = CONN.gh; gh.id = 'other';", + connectionId: 'CONN.gh.id', + }, + ])( + 'Should fail closed for an imported object connection ID mutated by $description', + ({ statements, connectionId }) => { + expect(() => extractWithImportedConnections(statements, connectionId)).toThrow( + 'imported-object-mutation', + ); + }, + ); + + test.each([ + { description: 'read directly', statements: '' }, + { + description: 'read through its exported alias', + statements: '', + connectionId: 'ALIAS.github', + }, + { + description: 'assigned to an alias that is never written', + statements: 'let gh; gh = CONN.gh;', + }, + { description: 'copied by Object.assign', statements: 'Object.assign({}, CONN);' }, + { description: 'copied by a rest export that is written', statements: 'REST.extra = 1;' }, + { + description: 'passed to a local function named Reflect', + statements: "function copy(Reflect) { Reflect.set(CONN, 'github', 'other'); }", + }, + ])( + 'Should still resolve an unmutated imported object connection ID $description', + ({ statements, connectionId }) => { + expect(extractWithImportedConnections(statements, connectionId)).toEqual([ + 'conn-github', + ]); + }, + ); + + test('Should still resolve a local object that shares its name with a namespace re-export written elsewhere', () => { + const otherId = '/project/src/backend/other.js'; + const entry = createRecord( + entryId, + ` + import { CONN as NAMESPACE } from './connections.js'; + + Reflect.set(NAMESPACE, 'github', 'other'); + + export function run() {} + `, + [CONNECTIONS_ID], + ); + const connections = createRecord( + CONNECTIONS_ID, + ` + import { request } from '${ACTION_CATALOG_HTTP}'; + + export * as CONN from './other.js'; + const CONN = { github: 'conn-github' }; + + request({ connectionId: CONN.github, inputs: {} }); + `, + [ACTION_CATALOG_HTTP, otherId], + ); + const other = createRecord(otherId, 'export const value = 1;'); + + expect(extract([entry, connections, other])).toEqual(['conn-github']); + }); + + test.each([ + { description: 'a null __proto__', property: '__proto__: null' }, + { description: 'a shorthand __proto__ property', property: '__proto__' }, + ])( + 'Should still extract the connection ID of call options with $description', + ({ property }) => { + const entry = createRecord( + entryId, + ` + import { request } from '${ACTION_CATALOG_HTTP}'; + + const __proto__ = {}; + + export function run() { + return request({ connectionId: 'conn-a', ${property}, inputs: {} }); + } + `, + [ACTION_CATALOG_HTTP], + ); + + expect(extract([entry])).toEqual(['conn-a']); + }, + ); + + test.each([ + { description: 'a __proto__ key', property: "__proto__: { connectionId: 'hidden' }" }, + { + description: 'a quoted __proto__ key', + property: "'__proto__': { connectionId: 'hidden' }", + }, + { description: 'a toJSON method', property: "toJSON() { return { connectionId: 'b' }; }" }, + { description: 'a then method', property: 'then(resolve) { resolve({}); }' }, + { description: 'a shorthand toJSON property', property: 'toJSON' }, + ])('Should fail closed for action-catalog call options with $description', ({ property }) => { + const entry = createRecord( + entryId, + ` + import { request } from '${ACTION_CATALOG_HTTP}'; + + export function run() { + return request({ connectionId: 'conn-a', ${property}, inputs: {} }); + } + `, + [ACTION_CATALOG_HTTP], + ); + + expect(() => extract([entry])).toThrow('could hide a connectionId'); + }); + + test.each([ + { + description: 'several connectionId call sites', + connectionIds: ['a', 'b', 'c'], + walks: 1, + }, + { description: 'no connectionId call site', connectionIds: [], walks: 0 }, + ])( + 'Should walk the module graph for object writes $walks time(s) per extraction with $description', + ({ connectionIds, walks }) => { + const calls = connectionIds.map( + (id) => `request({ connectionId: CONN.${id}, inputs: {} });`, + ); + const entry = createRecord( + entryId, + ` + import { request } from '${ACTION_CATALOG_HTTP}'; + const CONN = { a: 'conn-a', b: 'conn-b', c: 'conn-c' }; + + export function run() { + request({ inputs: {} }); + ${calls.join('\n')} + } + `, + [ACTION_CATALOG_HTTP], + ); + collectMutatedMock.mockClear(); + + const extracted = extract([entry]); + + const expected = connectionIds.map((id) => `conn-${id}`); + expect(extracted).toEqual(expected); + expect(collectMutatedMock).toHaveBeenCalledTimes(walks); + }, + ); }); diff --git a/packages/plugins/apps/src/backend/ast-parsing/extract-connection-ids-from-module-graph.ts b/packages/plugins/apps/src/backend/ast-parsing/extract-connection-ids-from-module-graph.ts index 704b5db23..f8c991882 100644 --- a/packages/plugins/apps/src/backend/ast-parsing/extract-connection-ids-from-module-graph.ts +++ b/packages/plugins/apps/src/backend/ast-parsing/extract-connection-ids-from-module-graph.ts @@ -2,6 +2,7 @@ // This product includes software developed at Datadog (https://www.datadoghq.com/). // Copyright 2019-Present Datadog, Inc. +import type * as eslintScope from 'eslint-scope'; import type { Expression, ObjectExpression, Property, SimpleCallExpression } from 'estree'; import { @@ -10,10 +11,16 @@ import { } from './action-catalog-call-sites'; import { collectActionCatalogImports } from './action-catalog-imports'; import type { ParsedModuleRecord } from './module-graph'; -import { resolveStaticStringValue } from './static-string-resolution'; +import { + collectMutatedImportedObjectVariables, + resolveStaticStringValue, +} from './static-string-resolution'; import { walkModuleGraph } from './walk-module-graph'; const CONNECTION_ID_PROPERTY = 'connectionId'; +// `__proto__` can supply a connectionId from a prototype, and toJSON or then can replace the +// options when they are serialized or awaited. +const OPTION_HOOK_KEYS: ReadonlySet = new Set(['__proto__', 'toJSON', 'then']); type ConnectionIdProperty = Property & { value: Expression }; @@ -27,6 +34,11 @@ export function extractConnectionIdsFromModuleGraph( buildRoot: string, ): string[] { const connectionIds = new Set(); + let mutatedImportedObjectVariables: Set | undefined; + const getMutatedImportedObjectVariables = () => { + mutatedImportedObjectVariables ??= collectMutatedImportedObjectVariables(modules); + return mutatedImportedObjectVariables; + }; // Walk the already-parsed records from this backend entry's build. The // extraction cost is linear in reachable app-local modules, without @@ -36,7 +48,12 @@ export function extractConnectionIdsFromModuleGraph( const scopeAnalysis = analyzeActionCatalogScopes(record.scopeAnalysis, imports); for (const callSite of findActionCatalogCallSites(record.ast, scopeAnalysis, record.id)) { - const connectionId = extractConnectionIdFromActionCall(callSite, modules, record); + const connectionId = extractConnectionIdFromActionCall( + callSite, + modules, + record, + getMutatedImportedObjectVariables, + ); // `''` is a valid declared connectionId, not "none" — only the absence of a // `connectionId` property (`undefined`) means no restriction to record. if (connectionId !== undefined) { @@ -52,6 +69,7 @@ function extractConnectionIdFromActionCall( node: SimpleCallExpression, modules: ReadonlyMap, record: ParsedModuleRecord, + getMutatedImportedObjectVariables: () => ReadonlySet, ): string | undefined { const [firstArg] = node.arguments; if (!firstArg || firstArg.type !== 'ObjectExpression') { @@ -63,7 +81,13 @@ function extractConnectionIdFromActionCall( return undefined; } - const result = resolveStaticStringValue(modules, record.id, connectionIdProperty.value); + const mutatedImportedObjectVariables = getMutatedImportedObjectVariables(); + const result = resolveStaticStringValue( + modules, + record.id, + connectionIdProperty.value, + mutatedImportedObjectVariables, + ); if (result.kind === 'resolved') { return result.value; } @@ -102,6 +126,10 @@ function findConnectionIdProperty( if (property.computed) { throw unsupportedActionCatalogCall(filePath, 'computed object property keys'); } + const key = getStaticPropertyKey(property); + if (key !== undefined && OPTION_HOOK_KEYS.has(key) && !isInertProtoKey(key, property)) { + throw unsupportedActionCatalogCall(filePath, `a "${key}" property`); + } if (isConnectionIdKey(property)) { if (connectionIdProperty) { throw unsupportedActionCatalogCall(filePath, 'multiple connectionId properties'); @@ -131,6 +159,15 @@ function hasExpressionValue(property: Property): property is ConnectionIdPropert ); } +// A shorthand `__proto__` is an own property, and a null prototype supplies nothing. +function isInertProtoKey(key: string, property: Property): boolean { + const { value } = property; + return ( + key === '__proto__' && + (property.shorthand || (value.type === 'Literal' && value.value === null)) + ); +} + function isConnectionIdKey(property: Property): boolean { return getStaticPropertyKey(property) === CONNECTION_ID_PROPERTY; } diff --git a/packages/plugins/apps/src/backend/ast-parsing/module-graph.ts b/packages/plugins/apps/src/backend/ast-parsing/module-graph.ts index d828ac226..7b294352b 100644 --- a/packages/plugins/apps/src/backend/ast-parsing/module-graph.ts +++ b/packages/plugins/apps/src/backend/ast-parsing/module-graph.ts @@ -15,12 +15,12 @@ import type { Pattern, Program, SimpleCallExpression, - Super, VariableDeclaration, } from 'estree'; import { importsBackendFunctionPackage, isBackendSourceModule } from '../backend-sources'; +import { staticMemberName } from './ambient-global-access'; import { analyzeModuleScope, getModuleVariable, @@ -28,7 +28,7 @@ import { type ModuleScopeAnalysis, resolveIdentifier, } from './module-scope'; -import { ensureProgram, isStringLiteral } from './type-guards'; +import { ensureProgram, isNodeOfType, isStringLiteral } from './type-guards'; import { walkAst } from './walk-ast'; /** @@ -378,7 +378,7 @@ function collectDeclarationExportBindings( for (const variable of variables) { exportsByName.set(variable.name, { kind: 'unsupported', - reason: 'binding pattern export', + reason: BINDING_PATTERN_EXPORT_REASON, }); } continue; @@ -514,61 +514,70 @@ function collectVariableDeclarationBindings( } } +interface BindingWriteState { + ast: Program; + scopeAnalysis: ModuleScopeAnalysis; + bindings: Map; + // Variables already followed to their alias sources, for writes to the object and to its members. + expandedObjectWrites: Set; + expandedMemberWrites: Set; +} + function markReassignedTopLevelBindings( ast: Program, scopeAnalysis: ModuleScopeAnalysis, bindings: Map, ): void { - walkAst( + const writeState: BindingWriteState = { ast, - { scopeAnalysis, bindings }, - { - AssignmentExpression(node, { state }) { - markAssignedPattern(node.left, state.scopeAnalysis, state.bindings); - }, - UpdateExpression(node, { state }) { - markAssignedPattern(node.argument, state.scopeAnalysis, state.bindings); - }, - UnaryExpression(node, { state }) { - if (node.operator === 'delete') { - markAssignedPattern(node.argument, state.scopeAnalysis, state.bindings); - } - }, - ForInStatement(node, { state }) { - markForIterationTarget(node.left, state.scopeAnalysis, state.bindings); - }, - ForOfStatement(node, { state }) { - markForIterationTarget(node.left, state.scopeAnalysis, state.bindings); - }, + scopeAnalysis, + bindings, + expandedObjectWrites: new Set(), + expandedMemberWrites: new Set(), + }; + walkAst(ast, writeState, { + AssignmentExpression(node, { state }) { + markAssignedPattern(node.left, state); }, - ); + UpdateExpression(node, { state }) { + markAssignedPattern(node.argument, state); + }, + UnaryExpression(node, { state }) { + if (node.operator === 'delete') { + markAssignedPattern(node.argument, state); + } + }, + CallExpression(node, { state }) { + for (const target of getMutatedCallTargets(node, state.scopeAnalysis)) { + markMutatedObject(target, state); + } + }, + ForInStatement(node, { state }) { + markForIterationTarget(node.left, state); + }, + ForOfStatement(node, { state }) { + markForIterationTarget(node.left, state); + }, + }); } function markForIterationTarget( left: Pattern | VariableDeclaration, - scopeAnalysis: ModuleScopeAnalysis, - bindings: Map, + state: BindingWriteState, ): void { if (left.type !== 'VariableDeclaration') { - markAssignedPattern(left, scopeAnalysis, bindings); + markAssignedPattern(left, state); } } -function markAssignedPattern( - pattern: Pattern | Expression, - scopeAnalysis: ModuleScopeAnalysis, - bindings: Map, -): void { +function markAssignedPattern(pattern: Pattern | Expression, state: BindingWriteState): void { if (pattern.type === 'Identifier') { - markAssignedVariable(pattern, scopeAnalysis, bindings, 'reassigned binding'); + markAssignedVariable(pattern, state.scopeAnalysis, state.bindings, 'reassigned binding'); return; } if (pattern.type === 'MemberExpression') { - const root = getMemberExpressionRoot(pattern); - if (root) { - markAssignedVariable(root, scopeAnalysis, bindings, 'mutated object binding'); - } + markMutatedObject(pattern.object, state); return; } @@ -576,8 +585,7 @@ function markAssignedPattern( for (const property of pattern.properties) { markAssignedPattern( property.type === 'RestElement' ? property.argument : property.value, - scopeAnalysis, - bindings, + state, ); } return; @@ -586,19 +594,188 @@ function markAssignedPattern( if (pattern.type === 'ArrayPattern') { for (const element of pattern.elements) { if (element) { - markAssignedPattern(element, scopeAnalysis, bindings); + markAssignedPattern(element, state); } } return; } if (pattern.type === 'RestElement') { - markAssignedPattern(pattern.argument, scopeAnalysis, bindings); + markAssignedPattern(pattern.argument, state); return; } if (pattern.type === 'AssignmentPattern') { - markAssignedPattern(pattern.left, scopeAnalysis, bindings); + markAssignedPattern(pattern.left, state); + } +} + +export const BINDING_PATTERN_EXPORT_REASON = 'binding pattern export'; + +const OBJECT_MUTATORS = new Map>([ + ['Object', new Set(['assign', 'defineProperty', 'defineProperties', 'setPrototypeOf'])], + ['Reflect', new Set(['set', 'defineProperty', 'deleteProperty', 'setPrototypeOf'])], +]); + +/** Returns the objects that a call such as `Object.assign(target, ...)` writes to. */ +export function getMutatedCallTargets( + call: SimpleCallExpression, + scopeAnalysis: ModuleScopeAnalysis, +): Expression[] { + const { callee } = call; + if (callee.type !== 'MemberExpression' || callee.object.type !== 'Identifier') { + return []; + } + const methodName = staticMemberName(callee, scopeAnalysis); + if ( + methodName === undefined || + !OBJECT_MUTATORS.get(callee.object.name)?.has(methodName) || + resolveIdentifier(callee.object, scopeAnalysis) + ) { + return []; + } + // `Reflect.set` writes to its receiver, the fourth argument, when one is given. With a spread + // among the first four, both the first and fourth are marked, but not one shifted into place. + const [target, , , receiver] = call.arguments; + const isReflectSet = callee.object.name === 'Reflect' && methodName === 'set'; + const hasSpread = call.arguments.slice(0, 4).some((arg) => arg.type === 'SpreadElement'); + let targets = [target]; + if (isReflectSet) { + targets = hasSpread ? [target, receiver] : [receiver ?? target]; + } + return targets.filter( + (argument): argument is Expression => + argument !== undefined && argument.type !== 'SpreadElement', + ); +} + +/** A variable whose object an expression may evaluate to, or reach a member of. */ +export interface ObjectRoot { + identifier: Identifier; + isMember: boolean; +} + +/** Lists the roots of expressions such as `CONN`, `CONN.gh`, `CONN?.gh`, `CONN.gh || {}` or `a = CONN`. */ +export function getObjectRoots(node: unknown, isMember = false): ObjectRoot[] { + if (isNodeOfType(node, 'Identifier')) { + return [{ identifier: node, isMember }]; + } + if (isNodeOfType(node, 'MemberExpression')) { + return getObjectRoots(node.object, true); + } + if (isNodeOfType(node, 'ChainExpression')) { + return getObjectRoots(node.expression, isMember); + } + if (isNodeOfType(node, 'AssignmentExpression')) { + // `a ||= b` and `a ??= b` yield `a` when it already holds an object. + const rightRoots = getObjectRoots(node.right, isMember); + return node.operator === '||=' || node.operator === '??=' + ? [...getObjectRoots(node.left, isMember), ...rightRoots] + : rightRoots; + } + if (isNodeOfType(node, 'SequenceExpression')) { + const lastExpression = node.expressions.at(-1); + return getObjectRoots(lastExpression, isMember); + } + if (isNodeOfType(node, 'LogicalExpression')) { + // `a && b` only yields `a` when it is falsy, which no object is. + const rightRoots = getObjectRoots(node.right, isMember); + return node.operator === '&&' + ? rightRoots + : [...getObjectRoots(node.left, isMember), ...rightRoots]; + } + if (isNodeOfType(node, 'ConditionalExpression')) { + return [ + ...getObjectRoots(node.consequent, isMember), + ...getObjectRoots(node.alternate, isMember), + ]; + } + return []; +} + +type PatternTargetKind = 'member' | 'rest'; + +const patternTargetsByProgram = new WeakMap>(); + +// Identifiers bound by destructuring: a rest element gets a new object, any other target a member. +function getPatternTargets(ast: Program): Map { + let targets = patternTargetsByProgram.get(ast); + if (!targets) { + targets = new Map(); + walkAst(ast, targets, { + ObjectPattern(node, { state }) { + for (const property of node.properties) { + if (property.type === 'Property') { + markMemberTarget(property.value, state); + } + } + }, + ArrayPattern(node, { state }) { + for (const element of node.elements) { + markMemberTarget(element, state); + } + }, + RestElement(node, { state }) { + state.set(node.argument, 'rest'); + }, + }); + patternTargetsByProgram.set(ast, targets); + } + return targets; +} + +function markMemberTarget(node: Pattern | null, targets: Map): void { + const target = node?.type === 'AssignmentPattern' ? node.left : node; + if (target?.type === 'Identifier' && !targets.has(target)) { + targets.set(target, 'member'); + } +} + +/** + * Lists the roots `variable`, declared in `ast`, was assigned from. A rest element copies only + * its source's top-level properties, so it is followed only for a write to one of its members. + */ +export function getAliasSourceRoots( + variable: eslintScope.Variable, + ast: Program, + isMember: boolean, +): ObjectRoot[] { + const patternTargets = getPatternTargets(ast); + const roots: ObjectRoot[] = []; + for (const reference of variable.references) { + const targetKind = patternTargets.get(reference.identifier); + if (!reference.writeExpr || (!isMember && targetKind === 'rest')) { + continue; + } + const isSourceMember = isMember || targetKind === 'member'; + for (const root of getObjectRoots(reference.writeExpr)) { + roots.push({ identifier: root.identifier, isMember: isSourceMember || root.isMember }); + } + } + return roots; +} + +// A write through an alias such as `const gh = CONN.gh` or `gh = CONN.gh` also changes the object it aliases. +function markMutatedObject(writtenObject: unknown, state: BindingWriteState): void { + const pending = getObjectRoots(writtenObject); + for (let root = pending.pop(); root; root = pending.pop()) { + markAssignedVariable( + root.identifier, + state.scopeAnalysis, + state.bindings, + 'mutated object binding', + ); + + const variable = resolveIdentifier(root.identifier, state.scopeAnalysis); + const expanded = root.isMember ? state.expandedMemberWrites : state.expandedObjectWrites; + if (!variable || expanded.has(variable)) { + continue; + } + expanded.add(variable); + const sourceRoots = getAliasSourceRoots(variable, state.ast, root.isMember); + for (const sourceRoot of sourceRoots) { + pending.push(sourceRoot); + } } } @@ -620,16 +797,6 @@ function markAssignedVariable( bindings.set(variable, { kind: 'unsupported', reason }); } -function getMemberExpressionRoot(node: Expression | Super): Identifier | undefined { - if (node.type === 'Identifier') { - return node; - } - if (node.type === 'MemberExpression') { - return getMemberExpressionRoot(node.object); - } - return undefined; -} - function getModuleExportName(node: ModuleExportName): string { if (node.type === 'Identifier') { return node.name; diff --git a/packages/plugins/apps/src/backend/ast-parsing/static-string-resolution.ts b/packages/plugins/apps/src/backend/ast-parsing/static-string-resolution.ts index 6374f3932..4bb565abb 100644 --- a/packages/plugins/apps/src/backend/ast-parsing/static-string-resolution.ts +++ b/packages/plugins/apps/src/backend/ast-parsing/static-string-resolution.ts @@ -14,11 +14,17 @@ import type { Super, TemplateLiteral, VariableDeclaration, - VariableDeclarator, } from 'estree'; -import type { ParsedModuleRecord } from './module-graph'; -import { isImportVariable, resolveIdentifier } from './module-scope'; +import { + BINDING_PATTERN_EXPORT_REASON, + getAliasSourceRoots, + getMutatedCallTargets, + getObjectRoots, + type ObjectRoot, + type ParsedModuleRecord, +} from './module-graph'; +import { getModuleVariable, isImportVariable, resolveIdentifier } from './module-scope'; import { resolveStaticDefinitionForIdentifier, type UnsupportedStaticDefinition, @@ -171,6 +177,19 @@ interface ImportedObjectMutationState { modules: ReadonlyMap; mutatedVariables: Set; record: ParsedModuleRecord; + // The write kinds each variable was already followed to its alias sources for. + expandedAliases: Map>; +} + +interface PendingImportedAlias extends ObjectRoot { + record: ParsedModuleRecord; + isPastImport: boolean; +} + +interface ResolvedImportedAlias { + record: ParsedModuleRecord; + variable: eslintScope.Variable; + isPastImport: boolean; } interface ResolvedStaticExpression { @@ -201,11 +220,14 @@ export function resolveStaticStringValue( modules: ReadonlyMap, moduleId: string, expression: Expression, + mutatedImportedObjectVariables: ReadonlySet = collectMutatedImportedObjectVariables( + modules, + ), ): StaticStringValueResolution { return resolveExpression( { modules, - mutatedImportedObjectVariables: collectMutatedImportedObjectVariables(modules), + mutatedImportedObjectVariables, seenVariables: new Set(), }, moduleId, @@ -456,15 +478,16 @@ function hasExpressionValue(property: Property): property is Property & { value: ); } -function collectMutatedImportedObjectVariables( +export function collectMutatedImportedObjectVariables( modules: ReadonlyMap, ): Set { const mutatedVariables = new Set(); + const expandedAliases = new Map>(); for (const record of modules.values()) { walkAst( record.ast, - { modules, mutatedVariables, record }, + { modules, mutatedVariables, record, expandedAliases }, { AssignmentExpression(node, { state }) { markMutatedImportedPattern(node.left, state); @@ -477,6 +500,11 @@ function collectMutatedImportedObjectVariables( markMutatedImportedPattern(node.argument, state); } }, + CallExpression(node, { state }) { + for (const target of getMutatedCallTargets(node, state.record.scopeAnalysis)) { + markMutatedImportedObject(target, state); + } + }, ForInStatement(node, { state }) { markMutatedImportedForIterationTarget(node.left, state); }, @@ -504,15 +532,16 @@ function markMutatedImportedPattern( state: ImportedObjectMutationState, ): void { if (pattern.type === 'Identifier') { - markMutatedImportedIdentifier(pattern, state); + // Rebinding a local name leaves the object it held unchanged; only an import's own binding counts. + const variable = resolveIdentifier(pattern, state.record.scopeAnalysis); + if (variable && isImportVariable(variable)) { + markMutatedImportedObject(pattern, state); + } return; } if (pattern.type === 'MemberExpression') { - const root = getMemberExpressionRoot(pattern); - if (root) { - markMutatedImportedIdentifier(root, state); - } + markMutatedImportedObject(pattern.object, state); return; } @@ -545,77 +574,82 @@ function markMutatedImportedPattern( } } -function markMutatedImportedIdentifier( - identifier: Identifier, +// Marks the imported objects `writtenObject` may alias. Past an import, the definition module's +// own aliases of the object (e.g. `export const ALIAS = CONN`) are marked too. +function markMutatedImportedObject( + writtenObject: unknown, state: ImportedObjectMutationState, ): void { - const variable = resolveImportedObjectAlias(state, identifier, new Set()); - if (variable) { - state.mutatedVariables.add(variable); + const pending: PendingImportedAlias[] = getObjectRoots(writtenObject).map((root) => ({ + ...root, + record: state.record, + isPastImport: false, + })); + for (let alias = pending.pop(); alias; alias = pending.pop()) { + const resolved = resolveImportedAlias(state, alias); + if (!resolved) { + continue; + } + if (resolved.isPastImport) { + state.mutatedVariables.add(resolved.variable); + } + const expanded = state.expandedAliases.get(resolved.variable) ?? new Set(); + const writeKind = `${alias.isMember}:${resolved.isPastImport}`; + if (expanded.has(writeKind)) { + continue; + } + expanded.add(writeKind); + state.expandedAliases.set(resolved.variable, expanded); + for (const root of getAliasSourceRoots( + resolved.variable, + resolved.record.ast, + alias.isMember, + )) { + pending.push({ ...root, record: resolved.record, isPastImport: resolved.isPastImport }); + } } } -function resolveImportedObjectAlias( +function resolveImportedAlias( state: ImportedObjectMutationState, - identifier: Identifier, - seenVariables: Set, -): eslintScope.Variable | undefined { - const variable = resolveIdentifier(identifier, state.record.scopeAnalysis); - if (!variable || seenVariables.has(variable)) { + { record, identifier, isPastImport }: PendingImportedAlias, +): ResolvedImportedAlias | undefined { + const variable = resolveIdentifier(identifier, record.scopeAnalysis); + if (!variable) { return undefined; } - seenVariables.add(variable); - - if (isImportVariable(variable)) { - const definition = resolveStaticDefinitionForIdentifier( - state.modules, - state.record.id, - identifier, - ); - return definition.kind === 'local' ? definition.variable : undefined; + if (!isImportVariable(variable)) { + return { record, variable, isPastImport }; } - const initializer = getVariableInitializer(variable); - if (!initializer) { - return undefined; - } - - const root = getImportedObjectAliasRoot(initializer); - if (!root) { - return undefined; - } - - return resolveImportedObjectAlias(state, root, seenVariables); + const definition = resolveStaticDefinitionForIdentifier(state.modules, record.id, identifier); + const definitionRecord = state.modules.get(definition.moduleId); + const definitionVariable = + definition.kind === 'local' + ? definition.variable + : getAliasVariable(definition, definitionRecord); + return definitionRecord && definitionVariable + ? { record: definitionRecord, variable: definitionVariable, isPastImport: true } + : undefined; } -function getVariableInitializer(variable: eslintScope.Variable): Expression | undefined { - for (const definition of variable.defs) { - const node = definition.node as VariableDeclarator; - if (node.type === 'VariableDeclarator' && node.init) { - return node.init; - } - } - - return undefined; -} - -function getImportedObjectAliasRoot(rawExpression: Expression): Identifier | undefined { - const expression = unwrapStaticExpression(rawExpression); - if (expression.type === 'Identifier') { - return expression; - } - if (expression.type === 'MemberExpression') { - return getMemberExpressionRoot(expression); +// An exported top-level `let`, `var` or destructured declaration is not a static definition, but +// a write through it still reaches the object it was assigned from. +function getAliasVariable( + definition: UnsupportedStaticDefinition, + record: ParsedModuleRecord | undefined, +): eslintScope.Variable | undefined { + if (!record) { + return undefined; } - return undefined; -} - -function getMemberExpressionRoot(node: Expression | Super): Identifier | undefined { - if (node.type === 'Identifier') { - return node; + if (definition.reason === 'mutable-binding' || definition.reason === 'unsupported-binding') { + return getModuleVariable(definition.variableName, record.scopeAnalysis); } - if (node.type === 'MemberExpression') { - return getMemberExpressionRoot(node.object); + if ( + definition.reason === 'unsupported-export' && + definition.exportReason === BINDING_PATTERN_EXPORT_REASON + ) { + return getModuleVariable(definition.exportName, record.scopeAnalysis); } return undefined; } diff --git a/packages/plugins/apps/src/backend/ast-parsing/type-guards.ts b/packages/plugins/apps/src/backend/ast-parsing/type-guards.ts index fb693d135..5258b61b0 100644 --- a/packages/plugins/apps/src/backend/ast-parsing/type-guards.ts +++ b/packages/plugins/apps/src/backend/ast-parsing/type-guards.ts @@ -2,7 +2,14 @@ // This product includes software developed at Datadog (https://www.datadoghq.com/). // Copyright 2019-Present Datadog, Inc. -import type { BaseNode, Program, SimpleLiteral, TemplateLiteral, VariableDeclarator } from 'estree'; +import type { + BaseNode, + Node, + Program, + SimpleLiteral, + TemplateLiteral, + VariableDeclarator, +} from 'estree'; export type StringLiteral = SimpleLiteral & { value: string }; @@ -43,6 +50,14 @@ export function isVariableDeclaratorNode(node: unknown): node is VariableDeclara return hasNodeType(node, 'VariableDeclarator'); } +// Narrows nodes typed against another copy of `estree`, such as eslint-scope's, by their type. +export function isNodeOfType( + node: unknown, + type: T, +): node is Extract { + return hasNodeType(node, type); +} + function isNoSubstitutionTemplateLiteral(node: unknown): node is TemplateLiteral { if (!hasNodeType(node, 'TemplateLiteral') || !('expressions' in node) || !('quasis' in node)) { return false; From 5c1e4b3139c74c2950339f3802ed6e5d446a2b4f Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Tue, 27 Oct 2026 01:29:57 -0400 Subject: [PATCH 2/2] test(apps): cover Object.assign called with a template-literal computed key Co-Authored-By: Claude Opus 5.5 --- .../extract-connection-ids-from-module-graph.test.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/plugins/apps/src/backend/ast-parsing/extract-connection-ids-from-module-graph.test.ts b/packages/plugins/apps/src/backend/ast-parsing/extract-connection-ids-from-module-graph.test.ts index 788cfc83c..db6d40b9d 100644 --- a/packages/plugins/apps/src/backend/ast-parsing/extract-connection-ids-from-module-graph.test.ts +++ b/packages/plugins/apps/src/backend/ast-parsing/extract-connection-ids-from-module-graph.test.ts @@ -816,6 +816,10 @@ describe('Backend Functions - extractConnectionIdsFromModuleGraph', () => { description: 'Object.assign called with a computed key', statements: "Object['assign'](CONN, { github: 'other' });", }, + { + description: 'Object.assign called with a template-literal computed key', + statements: "Object[`assign`](CONN, { github: 'other' });", + }, { description: 'Object.assign called with a const-aliased computed key', statements: "const method = 'assign'; Object[method](CONN, { github: 'other' });",