Keep quoted file names with secrets in review (GST-1451) (#4203)

* Keep quoted file names with secrets in review (GST-1451)

The review collector now decodes git's C-quoted patch headers, so the secret-line keep rule reads the added lines of a file whose name holds a quote, backslash or control byte. reviewExclude also keeps a matched file the collector holds no lines for, as unread.

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

* Type the expected kept reasons in the quoted-name test

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

* Skip the quoted-name tests on Windows, which cannot create those names

Review-Finding: p1/outside-voice#1 reader=outside-voice
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

---------

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

changelog.d/keep-quoted-file-names-with-secrets-in-review.md

@@ -0,0 +1 @@
+- **A data file whose name git quotes no longer slips past the secret-line keep rule in `review_exclude_paths`.** git still C-quotes patch headers for names holding a double quote, a backslash, a tab or another control byte, even with `core.quotePath=false`, so the review collector stored their added lines under the quoted header text and the keep rule read no lines for the real path: a file such as `r/q"x.json` holding a credential value was excluded from every reader's diff. The collector now decodes quoted headers (octal escapes, `\"`, `\\`, `\t` and the rest) and pins the `a/` and `b/` prefixes, and the exclusion fails closed: a matched text file with changed lines that the collector holds no lines for is kept as unread instead of excluded.

docs/ship-policy.md

@@ -1453,7 +1453,7 @@ Everything else stays in every reader's diff with one reason code:
 | `code` | it is code or a test, not content; a binary file outside the content extensions above, or one with executable magic, counts as code |
 | `secret-line` | an added line names a credential (the targeted pass's secret rule, or a key name given a value such as `OPENAI_API_KEY=`) and carries a value: the credential scan finds a secret on the line, or a run of 20 or more base64 or token characters mixing upper case, lower case and digits follows a name within 200 characters. A line that only names a variable or a credential store (`${OPENAI_API_KEY}`, `$GH_TOKEN`, `process.env.X`, `os.environ["X"]`, a keychain command) keeps nothing (GST-1967: harness-evals #333's captured prompts kept 2,610 files that way). For a file git calls binary only because of its attributes, the added lines come from `git diff --text`, which `.gitattributes` cannot change (readers' own diffs still follow the attributes and show the kept file as binary). When this is the only reason, pass readers get an excerpt, not the whole file ([secret-line excerpts](#secret-line-excerpts)) |
 | `named-by-code` | a code or test file that stays in review (not docs, a data file, a `.log` or binary, and not itself excluded) names its full repository path, at the head or the merge base |
-| `consumer-scan-unavailable` | the `git grep` for those names failed, so every candidate is kept; also a live binary file whose bytes could not be read |
+| `consumer-scan-unavailable` | the `git grep` for those names failed, so every candidate is kept; also a live binary file whose bytes could not be read, and a text file with changed lines the collector read no lines for |
 | `gate-scan-unavailable` | the gate scan failed or did not run at the head, so no hunk's tag is known and every candidate is kept |
 
 Only a full path counts as a name. A basename such as `result.json` would keep

lib/review-collect.ts

@@ -68,11 +68,31 @@ export function loadRegistryAt(root: string, sha: string): RegistryInfo {
   return { sha: resolved, outputs: registry.entries.filter(x => x.command !== null).flatMap(x => x.outputs) };
 }
 
+const C_ESCAPES: Record<string, number> = { a: 7, b: 8, t: 9, n: 10, v: 11, f: 12, r: 13, '"': 34, '\\': 92 };
+/**
+ * A patch header's path (the text after "--- " or "+++ ") without its a/ or b/ prefix. core.quotePath=false stops git
+ * escaping non-ASCII bytes, but a name holding a quote, a backslash or a control byte is still C-quoted in patch headers
+ * (-z does not change patch output), so a quoted name is decoded byte by byte: octal escapes, \" \\ \t and the rest. git
+ * adds a tab after an unquoted name holding a space (GST-1451).
+ */
+function headerPath(rest: string): string {
+  if (!rest.startsWith('"')) return rest.replace(/\t$/, '').slice(2);
+  const src = Buffer.from(rest, 'utf8');
+  const out: number[] = [];
+  for (let k = 1; k < src.length && src[k] !== 0x22; k++) {
+    if (src[k] !== 0x5c) { out.push(src[k]!); continue; }
+    const c = String.fromCharCode(src[++k] ?? 0);
+    if (/[0-7]/.test(c)) { out.push(parseInt(src.subarray(k, k + 3).toString('latin1'), 8) & 0xff); k += 2; }
+    else out.push(C_ESCAPES[c] ?? src[k]!);
+  }
+  return Buffer.from(out).toString('utf8').slice(2);
+}
+
 /** Numstat, deletions and the -U0 patch for `range` (`[mergeBase, head]` or `[mergeBase]` for the working tree). */
 function readDiff(root: string, range: string[], errors: DepthError[]): Pick<Collected, 'perPath' | 'deleted' | 'stampOnly' | 'created' | 'added' | 'removed'> {
   const numstat = git(root, ['diff', '--numstat', '-z', '--no-renames', ...range]);
   const status = git(root, ['diff', '--name-status', '-z', '--no-renames', '--diff-filter=D', ...range]);
-  const patch = git(root, ['diff', '-U0', '--no-color', '--no-ext-diff', '--no-renames', ...range]);
+  const patch = git(root, ['diff', '-U0', '--no-color', '--no-ext-diff', '--no-renames', '--src-prefix=a/', '--dst-prefix=b/', ...range]);
   const empty = { perPath: [], deleted: [], stampOnly: [], created: [], added: {}, removed: {} };
   for (const [r, what] of [[numstat, '--numstat'], [status, '--name-status'], [patch, '-U0']] as const) {
     if (!r.ok) { errors.push({ code: 'numstat_failed', detail: oneLine(r.err || 'git diff failed'), input: `git diff ${what} ${range.join(' ')}`, repair: 'check that both commits exist locally (git fetch origin <base>) and rerun' }); return empty; }
@@ -95,8 +115,11 @@ function readDiff(root: string, range: string[], errors: DepthError[]): Pick<Col
   for (const line of patch.out.split('\n')) {
     if (line.startsWith('diff --git ')) { inHunk = false; cur = ''; minus = ''; continue; }
     if (!inHunk) {
-      if (line.startsWith('--- ')) minus = line === '--- /dev/null' ? '' : line.slice(6).replace(/\t$/, '');
-      else if (line.startsWith('+++ ')) { cur = line === '+++ /dev/null' ? minus : line.slice(6).replace(/\t$/, ''); if (!minus && cur && !scratch(cur)) created.push(cur); }
+      if (line.startsWith('--- ')) minus = line === '--- /dev/null' ? '' : headerPath(line.slice(4));
+      else if (line.startsWith('+++ ')) {
+        cur = line === '+++ /dev/null' ? minus : headerPath(line.slice(4));
+        if (cur && !scratch(cur)) { added[cur] ??= []; if (!minus) created.push(cur); }
+      }
       else if (line.startsWith('@@')) inHunk = true;
       continue;
     }

lib/review-exclude.ts

@@ -346,7 +346,8 @@ function grepFixed(i: ExcludeInput, strings: readonly string[], candidates: Read
  * nothing); `head-only` when only the head does (a notice, never applied); `refused` when the base policy cannot be read or
  * does not validate, or git cannot list the matches; `active` otherwise. Named-by-code keeps a file only when a kept code
  * file names its full repository path (never its basename: receipt trees reuse result.json thousands of times); a git grep
- * failure keeps every candidate (consumer-scan-unavailable), and so does an unknown gate scan (gate-scan-unavailable).
+ * failure keeps every candidate (consumer-scan-unavailable), and so does an unknown gate scan (gate-scan-unavailable) or a
+ * matched text file with changed lines the collector holds no added lines for (GST-1451: it is unread, so it fails closed).
  */
 export function reviewExclude(i: ExcludeInput): Exclusion {
   const load = loadPolicyAt(i.root, i.baseSha, i.ref);
@@ -365,8 +366,10 @@ export function reviewExclude(i: ExcludeInput): Exclusion {
   const claimed = i.perPath.filter(e => e.binary && sel.files.has(e.path)).map(e => e.path);
   const binary = binaryContent(i, sel.files, new Set([...claimed, ...(bytesBinary(i, patterns) ?? sel.files.keys())]), unread, native);
   const text = { ...i, added: { ...i.added, ...attributeBinaryLines(i, claimed.filter(p => !binary.has(p) && !unread.has(p)), unread) } };
+  const collected = new Map(i.perPath.map(e => [e.path, e]));
+  const unseen = (p: string) => { const e = collected.get(p); return !e || (!e.binary && e.lines > 0 && !(p in text.added)); };
   const reasons = new Map<string, KeptReason | null>();
-  for (const [p, m] of sel.files) reasons.set(p, unread.has(p) ? 'consumer-scan-unavailable' : pathReason(text, p, m, binary, native));
+  for (const [p, m] of sel.files) reasons.set(p, unread.has(p) ? 'consumer-scan-unavailable' : pathReason(text, p, m, binary, native) ?? (unseen(p) ? 'consumer-scan-unavailable' : null));
   const candidates = new Set([...reasons].filter(([, r]) => r === null).map(([p]) => p));
   const secretOnly = [...reasons].filter(([p, r]) => r === 'secret-line' && !p.includes('\n')).map(([p]) => p);
   const named = grepFixed(i, [...candidates, ...secretOnly].filter(p => !p.includes('\n')), new Set([...candidates, ...secretOnly]), [i.mergeBase]);

test/review-exclude-secret-value.test.ts

@@ -120,3 +120,58 @@ describe('the argument-byte bound', () => {
     expect(x.record.skipped).toMatchObject([{ pattern: 'receipts/**', kept: COUNT }]);
   }, 120_000);
 });
+
+describe.skipIf(process.platform === 'win32')('names git quotes under review_exclude_paths (GST-1451)', () => {
+  /** Names git's patch headers C-quote even with core.quotePath=false: a quote, a backslash, a tab, a control byte, a space beside a quote. */
+  const QUOTED = ['receipts/q"x.json', 'receipts/back\\slash.json', 'receipts/tab\tx.json', 'receipts/ctl\x01x.json', 'receipts/sp ace "q".json'];
+  const UTF8 = 'receipts/r\u00e9sum\u00e9-\u6f22.json';
+  const SECRET = `{"cmd": "curl -H $OPENAI_API_KEY", "auth": "${BLOB}"}`;
+
+  function quotedBranch() {
+    const repo = makeFixtureRepo({ prefix: 'gstack-quoted-', baseFiles: { '.gstack/ship-policy.json': policyText, 'receipts/old.json': '{"a": 1}\n{"b": 2}\n', 'receipts/gone.json': '{"c": 3}\n' } }); repos.push(repo);
+    const files: Record<string, string | null> = { 'receipts/old.json': '{"a": 1}\n', 'receipts/gone.json': null, 'receipts/q"plain.json': '{"ok": true}\n', 'receipts/\u00e9-plain.json': '{"ok": true}\n', [UTF8]: SECRET + '\n' };
+    for (const p of QUOTED) files[p] = SECRET + '\n';
+    const head = commit(repo, files);
+    const baseSha = repo.git('rev-parse', 'origin/main');
+    const c = collectCommit(repo.work, baseSha, head);
+    const exclude = (added: Record<string, readonly string[]>) => reviewExclude({ root: repo.work, baseSha, ref: 'origin/main', mergeBase: c.mergeBase!, head, perPath: c.perPath, added, classRoot: c.root ?? [], gateTagged: new Set(), context: 'test' });
+    return { repo, c, exclude };
+  }
+  const byPath = (a: string, b: string) => (a < b ? -1 : a > b ? 1 : 0);
+  const keptSecrets = [...QUOTED, UTF8].sort(byPath).map(p => ({ path: p, reason: 'secret-line' as const }));
+
+  // Value: protects=the collector keys added lines by the real path for every name git quotes in a patch header, so the
+  //   secret-line rule reads them; fails_when=readDiff keys added lines by the quoted header text ("b/q\"x.json");
+  //   why_new=GST-1451; seam=collectCommit
+  test('the collector keys added lines by the real path, quoted or UTF-8', () => {
+    const { c } = quotedBranch();
+    for (const p of [...QUOTED, UTF8]) expect({ p, added: c.added[p] }).toEqual({ p, added: [SECRET] });
+    expect(Object.keys(c.added).filter(k => k.includes('"b/') || k.startsWith('"') || k.endsWith('"'))).toEqual([]);
+  }, 120_000);
+
+  // Value: protects=a credential value in a data file git quotes the name of stays in every reader's diff; a quoted or UTF-8 name
+  //   with no secret is still excluded, and so are a deletion-only edit and a whole-file deletion; fails_when=a quoted name slips
+  //   past the secret-line keep rule (GST-1451 repro), or the fail-closed check keeps files the collector did read;
+  //   why_new=GST-1451; seam=reviewExclude over collectCommit, and gstack-review-plan on the live tree
+  test('a quoted or UTF-8 name with a credential value is kept; one without is excluded', () => {
+    const { repo, exclude, c } = quotedBranch();
+    const x = exclude(c.added);
+    expect(x.state === 'active' && x.kept).toEqual(keptSecrets);
+    expect(x.state === 'active' && [...x.excluded].sort()).toEqual(['receipts/gone.json', 'receipts/old.json', 'receipts/q"plain.json', 'receipts/\u00e9-plain.json'].sort());
+    const p = JSON.parse(runBin(repo, 'gstack-review-plan', ['--base', 'main', '--gate-tagged', '0', '--json']).out);
+    expect(p.review_exclude.kept_files).toEqual(keptSecrets);
+    expect(p.review_exclude.excluded).toBe(4);
+  }, 120_000);
+
+  // Value: protects=a matched text file the collector holds no lines for is kept as unread, never excluded unread;
+  //   fails_when=reviewExclude reads a missing added entry as "no lines" and excludes the file; why_new=GST-1451 (fail closed);
+  //   seam=reviewExclude
+  test('a matched file with changed lines but no collector entry is kept as unread', () => {
+    const { exclude, c } = quotedBranch();
+    const added = { ...c.added };
+    delete added['receipts/q"plain.json'];
+    const x = exclude(added);
+    expect(x.state === 'active' && x.kept.find(k => k.path === 'receipts/q"plain.json')).toEqual({ path: 'receipts/q"plain.json', reason: 'consumer-scan-unavailable' });
+    expect(x.state === 'active' && x.excluded.has('receipts/old.json')).toBe(true);
+  }, 120_000);
+});