From 3c64ae340491867edfe0e4ac17f50cb52524ffa1 Mon Sep 17 00:00:00 2001 From: scottschreckengaust <345885+scottschreckengaust@users.noreply.github.com> Date: Thu, 10 Sep 2026 20:21:45 +0000 Subject: [PATCH 1/3] fix(security): merge vault channel_metadata by spread, not Object.assign (#879) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Object.assign` copies via [[Set]], which invokes the `__proto__` setter, so the pattern sits one refactor away from a prototype-pollution sink. Object spread defines own properties instead, where the same key lands inert. Not reachable today — `vaultMetadata` returns a literal with two hard-coded keys — but semgrep rates javascript.lang.security.insecure-object-assign as Blocking, and `security:sast` runs whole-repo in the pre-push hook while security-pr.yml runs only the ranged gates. So this one line passed PR CI on #831 and then rejected every contributor's `git push`, on every branch, for as long as it sat on main. Spread rather than explicit keyed writes because `vaultMetadata` is the single place that says which vault fields a task carries; restating them at this call site is the silent field-drop the helper exists to prevent. Tests — two source-level guards, failing for different reasons: * The existing structural check keys off the literal-entry form (`linear_workspace_slug: resolved.workspaceSlug,`), so it never fired on this builder at all: the one path `vaultMetadata` was written for was the one path its own guard never covered. Extended to the assignment form, scoped by dedent because an assignment has no literal to terminate. * A ban on `Object.assign` onto a metadata object. Reverting this change is behaviour-preserving, so the existing behavioural test stays green through it — a security property that lives in *how* the copy happens needs a structural check. Mutation-verified: restoring `Object.assign` reds both new guards and neither the behavioural test; deleting the merge reds the assignment-form guard and the behavioural test but not the Object.assign ban. `mise run build` fails locally at `//cdk:synth:quiet` on an unrelated host IAM gap (`ec2:DescribeAvailabilityZones` denied to the local role, needed for the AgentCore AZ context lookup). cdk compile, eslint, and 215 suites / 4542 tests pass. Closes #879 Co-Authored-By: Claude Opus 5 --- cdk/src/handlers/linear-webhook-processor.ts | 31 ++++++++++--- .../handlers/linear-webhook-processor.test.ts | 44 +++++++++++++++++++ 2 files changed, 68 insertions(+), 7 deletions(-) diff --git a/cdk/src/handlers/linear-webhook-processor.ts b/cdk/src/handlers/linear-webhook-processor.ts index 66a770acb..a07273c0b 100644 --- a/cdk/src/handlers/linear-webhook-processor.ts +++ b/cdk/src/handlers/linear-webhook-processor.ts @@ -856,7 +856,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, @@ -914,16 +916,31 @@ 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. + // Through the shared helper, not hand-rolled: `vaultMetadata` is the one place + // that says which vault fields a task carries, so a builder restating them drops + // whatever field is added there next. + // + // SPREAD, not `Object.assign`, and that is a security property rather than a + // style choice. `Object.assign` copies via [[Set]], which invokes the `__proto__` + // setter — a source object carrying that key mutates the target's prototype. + // Spread defines own properties, so the same key would land as an ordinary own + // property and go nowhere. Not reachable today (the helper returns a literal with + // two hard-coded keys), but semgrep flags the capability + // (javascript.lang.security.insecure-object-assign) as Blocking, and because + // `security:sast` runs in the pre-push hook that one line rejected every push + // from every branch while it sat on main (#879). + // + // This builder assigns onto an existing object rather than constructing a + // literal, so the source-level guard in + // cdk/test/handlers/linear-webhook-processor.test.ts used to miss it entirely — + // the ONE path `vaultMetadata` was written for was the one path its own guard + // never covered. That guard now triggers on the assignment form too. + // // 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)); + 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..5bbaeb473 100644 --- a/cdk/test/handlers/linear-webhook-processor.test.ts +++ b/cdk/test/handlers/linear-webhook-processor.test.ts @@ -1106,6 +1106,50 @@ 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. + const lines = src.split('\n'); + const offenders: number[] = []; + lines.forEach((line, i) => { + if (!/^\w+\.linear_workspace_slug = resolved\.workspaceSlug;$/.test(line.trim())) return; + const indent = line.length - line.trimStart().length; + for (let j = i + 1; j < lines.length; j += 1) { + const cur = lines[j]!; + if (cur.includes('...vaultMetadata(resolved)')) return; + if (cur.trim() !== '' && cur.length - cur.trimStart().length < indent) break; + } + 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. + const offenders = src + .split('\n') + .map((line, i) => ({ text: line, n: i + 1 })) + .filter(({ text }) => /Object\.assign\s*\(\s*\w*[Mm]etadata\b/.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 From 37a79bf3e3c091b9cda702fc53f420c310e87a67 Mon Sep 17 00:00:00 2001 From: scottschreckengaust <345885+scottschreckengaust@users.noreply.github.com> Date: Tue, 15 Sep 2026 17:21:36 +0000 Subject: [PATCH 2/3] fix(cdk): drop the dead workspace-id write and correct the stale guard docstring (#880 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review items on the handler, all non-blocking. 1. Deleted `channelMetadata.linear_workspace_id = workspaceId;`. It was dead, and this PR is what killed it: the object literal above already sets that key to the same value from the same variable, and `vaultMetadata` does not emit the key, so the spread cannot clobber it either. Confirmed by enumerating every writer of the key, not by reading the happy path. 2. Corrected `vaultMetadata`'s docstring. It still claimed the source-level check "keys off the object-literal form and cannot see a builder that assigns onto an existing object" — the premise of this PR, and false since the assignment-form guard landed. It now describes three checks: the literal form, the assignment form, and the behavioural one. 3. Trimmed the rationale at the call site from 24 comment lines to 13. Kept the two load-bearing claims — spread rather than `Object.assign` because [[Set]] invokes the `__proto__` setter, and spread the helper so a newly added vault field cannot be silently dropped — plus the recorded-subject note. The guard archaeology and the pre-push history now live in the commit trail and #879, where they cannot drift out of sync with the code. Refs #879. Co-Authored-By: Claude Opus 5 --- cdk/src/handlers/linear-webhook-processor.ts | 44 ++++++++------------ 1 file changed, 17 insertions(+), 27 deletions(-) diff --git a/cdk/src/handlers/linear-webhook-processor.ts b/cdk/src/handlers/linear-webhook-processor.ts index a07273c0b..46b3ec1e1 100644 --- a/cdk/src/handlers/linear-webhook-processor.ts +++ b/cdk/src/handlers/linear-webhook-processor.ts @@ -643,10 +643,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 { @@ -916,30 +917,19 @@ 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: `vaultMetadata` is the one place - // that says which vault fields a task carries, so a builder restating them drops - // whatever field is added there next. + // 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`, and that is a security property rather than a - // style choice. `Object.assign` copies via [[Set]], which invokes the `__proto__` - // setter — a source object carrying that key mutates the target's prototype. - // Spread defines own properties, so the same key would land as an ordinary own - // property and go nowhere. Not reachable today (the helper returns a literal with - // two hard-coded keys), but semgrep flags the capability - // (javascript.lang.security.insecure-object-assign) as Blocking, and because - // `security:sast` runs in the pre-push hook that one line rejected every push - // from every branch while it sat on main (#879). - // - // This builder assigns onto an existing object rather than constructing a - // literal, so the source-level guard in - // cdk/test/handlers/linear-webhook-processor.test.ts used to miss it entirely — - // the ONE path `vaultMetadata` was written for was the one path its own guard - // never covered. That guard now triggers on the assignment form too. - // - // 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; + // 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; From d4ec661b510b7983482f0f277321c73bc7a8232c Mon Sep 17 00:00:00 2001 From: scottschreckengaust <345885+scottschreckengaust@users.noreply.github.com> Date: Tue, 15 Sep 2026 17:22:18 +0000 Subject: [PATCH 3/3] test(cdk): close two holes in the vault-metadata source guards (#880 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both guards now match code, never prose, via a shared `isCommentLine`, and the assignment-form guard requires the merge to hit the same object the trigger wrote to. `assignment-form builders also carry the vault fields` The review's diagnosis was right: the accept window is far wider than it looks. The trigger sits at the shallowest indent of `if (WORKSPACE_REGISTRY_TABLE)`, so the scan ran ~45 lines to the end of that block — a `...vaultMetadata(resolved)` in any later sibling statement satisfied a builder that merged nothing into its own object. The suggested remedy is not applied as written, deliberately. Breaking on the first line indented `<=` the trigger reds the currently passing case on a comment at the same indent, and more fundamentally the real spread sits one level DEEPER, inside `if (resolved.providerName)`, because the merge is conditional while the slug write is not. Same-scope is therefore not an invariant this code can assert; same-object is, and it closes the same hole. The dedent break is kept as the scope exit and now precedes the accept check, which is stricter than before — the old order accepted a dedented spread. `no metadata builder merges via Object.assign` Regex unqualified over this file. `/Object\.assign\s*\(\s*\w*[Mm]etadata\b/` 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 that `security-pr.yml` omits, tracked in #235 rather than bolted on here. Comment-stripping is load-bearing for the vaultMetadata guard, whose `includes()` test was satisfied by a comment merely mentioning the spread (mutation M3 below). For the `Object.assign` ban it is defensive rather than required: the handler's three prose mentions all write the name in backticks, so `\s*` never reaches a `(`. Incidental punctuation, not a property worth depending on. Mutation-verified rather than asserted. Each row was applied to the handler, the named guards observed red, then reverted and the source re-verified by content: M1 merge → `Object.assign(channelMetadata, …)` both guards red M2 merge → `Object.assign(md, …)` both guards red (before: BLIND) M3 merge deleted, comment still names the spread assignment guard red (before: BLIND) M4 merge moved to a sibling block, other object assignment guard red (before: BLIND) M2, M3 and M4 are the differentials — all three passed before this change. Gates: `//cdk:test` 215 suites / 4552 tests green; this file 61/61; `//cdk:eslint` clean with no `--fix` mutations; `//cdk:compile` clean; `//cdk:synth:quiet` green once the AZ context is pinned (it otherwise fails closed on a local role lacking `ec2:DescribeAvailabilityZones`, as documented in DEPLOYMENT_GUIDE.md — unrelated to this change). Note for anyone rerunning: `-t` filtering on this file is unreliable and not a regression. Two behavioural tests in this describe have no `beforeEach` of their own and depend on `probeLinearIssueContextMock` being armed by the first describe's, so any filter that skips it fails them on pristine HEAD too. Run the whole file. Refs #879, #235. Co-Authored-By: Claude Opus 5 --- .../handlers/linear-webhook-processor.test.ts | 38 ++++++++++++++++--- 1 file changed, 33 insertions(+), 5 deletions(-) diff --git a/cdk/test/handlers/linear-webhook-processor.test.ts b/cdk/test/handlers/linear-webhook-processor.test.ts index 5bbaeb473..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 @@ -1117,15 +1125,28 @@ describe('every channel_metadata builder carries the vault fields', () => { // 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) => { - if (!/^\w+\.linear_workspace_slug = resolved\.workspaceSlug;$/.test(line.trim())) return; + 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]!; - if (cur.includes('...vaultMetadata(resolved)')) return; - if (cur.trim() !== '' && cur.length - cur.trimStart().length < indent) break; + 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); }); @@ -1142,10 +1163,17 @@ describe('every channel_metadata builder carries the vault fields', () => { // 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, n: i + 1 })) - .filter(({ text }) => /Object\.assign\s*\(\s*\w*[Mm]etadata\b/.test(text)) + .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([]); });