diff --git a/cdk/src/handlers/linear-webhook-processor.ts b/cdk/src/handlers/linear-webhook-processor.ts index 1f3eb36e1..22e196b33 100644 --- a/cdk/src/handlers/linear-webhook-processor.ts +++ b/cdk/src/handlers/linear-webhook-processor.ts @@ -645,10 +645,11 @@ interface ProcessorEvent { * workspace does not have: no reactions and no state transitions, on work that * otherwise succeeded. * - * Two checks guard it, because they fail for different reasons. The source-level one - * (in the test file) catches a builder nobody exercised, but keys off the object-literal - * form and cannot see a builder that assigns onto an existing object; the behavioural - * one asserts the fields actually reach `channel_metadata`. + * Three checks guard it, because they fail for different reasons. Two source-level ones + * (in the test file) catch a builder nobody exercised — one keys off the object-literal + * form, the other off the assignment form, so a builder that writes onto an existing + * object is covered too (#879). The behavioural one asserts the fields actually reach + * `channel_metadata`. */ function vaultMetadata(resolved: { providerName?: string; vaultUserId?: string }): Record { return { @@ -858,7 +859,9 @@ export async function handler(event: ProcessorEvent): Promise { return; } - const channelMetadata: Record = { + // `let` because the vault branch below re-derives it by spread rather than mutating + // it in place; see the comment there for why spread and not `Object.assign` (#879). + let channelMetadata: Record = { linear_issue_id: issue.id, linear_workspace_id: workspaceId, linear_project_id: projectId, @@ -916,16 +919,20 @@ export async function handler(event: ProcessorEvent): Promise { // (config.py) can mint its own Linear token via the vault. Absent ⇒ the // agent stays on the Secrets-Manager path. if (resolved.providerName) { - // Through the shared helper, not hand-rolled. This builder assigns onto an - // existing object rather than constructing a literal, which is what hid it from - // the source-level guard in cdk/test/handlers/linear-webhook-processor.test.ts — - // that guard keys off the literal - // form, so the ONE path it was written for was the one path it never covered. - // The subject inside the helper is recorded rather than derived from the - // workspace id, so a single consent can onboard a workspace whose org UUID is - // not yet known; absent ⇒ the agent derives the legacy form. - channelMetadata.linear_workspace_id = workspaceId; - Object.assign(channelMetadata, vaultMetadata(resolved)); + // Spread the helper rather than restating its fields: `vaultMetadata` is the one + // place that says which vault fields a task carries, so a builder that hand-rolls + // them drops whatever field is added there next. (The subject it emits is the + // recorded one, not derived from the workspace id, so a single consent can onboard + // a workspace whose org UUID is not yet known; absent ⇒ the agent derives the + // legacy form.) + // + // SPREAD, not `Object.assign` — a security property, not a style choice. + // `Object.assign` copies via [[Set]], which invokes the `__proto__` setter, so a + // source carrying that key would repoint this object's prototype; spread defines + // own properties, where the same key lands inert. Unreachable today (the helper + // returns a literal with two hard-coded keys), but the capability is what semgrep + // rates Blocking. Guard history and the pre-push consequence: #879. + channelMetadata = { ...channelMetadata, ...vaultMetadata(resolved) }; } resolvedAccessToken = resolved.accessToken; // Probe the issue once for native paperclip attachments + project docs. The diff --git a/cdk/test/handlers/linear-webhook-processor.test.ts b/cdk/test/handlers/linear-webhook-processor.test.ts index 45f8b8c61..90dc63ae4 100644 --- a/cdk/test/handlers/linear-webhook-processor.test.ts +++ b/cdk/test/handlers/linear-webhook-processor.test.ts @@ -1085,6 +1085,14 @@ describe('every channel_metadata builder carries the vault fields', () => { 'utf8', ); + // Both guards below match CODE, never prose (#880 review). The handler's own rationale + // names `Object.assign` and `...vaultMetadata(resolved)`, and a comment mentioning the + // spread used to SATISFY the vaultMetadata guard — deleting the real merge and leaving + // the prose behind passed. Whole-line only, so a trailing `// …` still counts: a false + // positive, which is the safe direction. + const isCommentLine = (trimmed: string) => + trimmed.startsWith('//') || trimmed.startsWith('*') || trimmed.startsWith('/*'); + test('each builder that writes the workspace slug also spreads vaultMetadata', () => { // Scans to the end of the enclosing object literal rather than demanding the spread // on the very next line: adjacency made a harmless key reorder fail, which trains @@ -1106,6 +1114,70 @@ describe('every channel_metadata builder carries the vault fields', () => { expect(offenders).toEqual([]); }); + test('assignment-form builders also carry the vault fields', () => { + // The check above cannot see the label-trigger builder at all: that one assigns onto + // an existing object, so there is no `linear_workspace_slug: resolved.workspaceSlug,` + // literal entry to key off, and the ONE path `vaultMetadata` was written for was the + // one path its own guard never covered (#879). Same question, other syntax — a + // builder that sets the slug by assignment must also pull the vault fields in before + // its enclosing block ends. + // + // End-of-scope is detected by DEDENT rather than a closing brace: an assignment form + // has no literal to terminate, so the first non-blank line indented less than the + // trigger is the end of the scope the trigger lives in. + // + // The accepting line must also merge into the SAME identifier the trigger wrote to + // (#880 review): that dedent window runs ~45 lines to the end of + // `if (WORKSPACE_REGISTRY_TABLE)`, so a spread in any later sibling block would + // otherwise satisfy a builder that merged nothing. Tightening the dedent to `<=` is + // NOT the fix — the real spread sits one level DEEPER, inside + // `if (resolved.providerName)`, because the merge is conditional and the slug write is + // not. Same-object, not same-scope. + const lines = src.split('\n'); + const offenders: number[] = []; + lines.forEach((line, i) => { + const trigger = /^(\w+)\.linear_workspace_slug = resolved\.workspaceSlug;$/.exec(line.trim()); + if (!trigger) return; + const target = trigger[1]!; + const indent = line.length - line.trimStart().length; + for (let j = i + 1; j < lines.length; j += 1) { + const cur = lines[j]!; + const trimmed = cur.trim(); + if (trimmed === '' || isCommentLine(trimmed)) continue; + // Left the trigger's scope without finding the merge. + if (cur.length - cur.trimStart().length < indent) break; + if (trimmed.includes('...vaultMetadata(resolved)') && trimmed.includes(target)) return; + } + offenders.push(i + 1); + }); + expect(offenders).toEqual([]); + }); + + test('no metadata builder merges via Object.assign', () => { + // `Object.assign` copies with [[Set]], which INVOKES the `__proto__` setter, so the + // pattern is one refactor away from a prototype-pollution sink; object spread defines + // own properties, where the same key lands inert. semgrep rates it Blocking. + // + // Asserted here rather than left to `security:sast` because of where that runs: + // whole-repo in the pre-push hook and in security.yml, but NOT in security-pr.yml, + // which runs only the ranged gates. A reintroduction therefore passes PR CI and then + // rejects every contributor's `git push` once it is on main — which is exactly how + // #879 happened. This test reds the PR that causes it instead. + // + // UNQUALIFIED over this file (#880 review). The first form required the first argument + // to be a bare identifier containing "metadata", so `Object.assign(md, …)` and + // `Object.assign(row.channelMetadata, …)` reintroduced the sink without redding the + // test — `\w*` cannot cross a `.`. Scope stays this ONE handler on purpose; the + // class-level net is the whole-repo semgrep gate `security-pr.yml` omits (#235). + const offenders = src + .split('\n') + .map((line, i) => ({ text: line.trim(), n: i + 1 })) + .filter(({ text }) => !isCommentLine(text)) + .filter(({ text }) => /Object\.assign\s*\(/.test(text)) + .map(({ n }) => n); + expect(offenders).toEqual([]); + }); + test('the LABEL-trigger path actually emits the vault fields (behavioural)', async () => { // The structural check above cannot see this builder: it assigns onto an existing // object instead of constructing a literal, so the one path `vaultMetadata` was