fix(security): merge vault channel_metadata by spread, not Object.assign (#879)

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<string, string> {
   return {
@@ -858,7 +859,9 @@ export async function handler(event: ProcessorEvent): Promise<void> {
     return;
   }
 
-  const channelMetadata: Record<string, string> = {
+  // `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<string, string> = {
     linear_issue_id: issue.id,
     linear_workspace_id: workspaceId,
     linear_project_id: projectId,
@@ -916,16 +919,20 @@ export async function handler(event: ProcessorEvent): Promise<void> {
     // (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

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