diff --git a/changelog.d/keep-quoted-file-names-with-secrets-in-review.md b/changelog.d/keep-quoted-file-names-with-secrets-in-review.md new file mode 100644 index 000000000..a8c64c501 --- /dev/null +++ b/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. diff --git a/docs/ship-policy.md b/docs/ship-policy.md index 9248b3c1c..002d3a6ea 100644 --- a/docs/ship-policy.md +++ b/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 diff --git a/lib/review-collect.ts b/lib/review-collect.ts index 65b43184c..63dd88cf8 100644 --- a/lib/review-collect.ts +++ b/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 = { 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 { 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 ) and rerun' }); return empty; } @@ -95,8 +115,11 @@ function readDiff(root: string, range: string[], errors: DepthError[]): Pick 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(); - 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]); diff --git a/test/review-exclude-secret-value.test.ts b/test/review-exclude-secret-value.test.ts index a9d5ff2e7..2fb2ab800 100644 --- a/test/review-exclude-secret-value.test.ts +++ b/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 = { '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) => 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); +});