diff --git a/bin/gstack-redact-prepush b/bin/gstack-redact-prepush index 39a0b1698..4aee829bd 100755 --- a/bin/gstack-redact-prepush +++ b/bin/gstack-redact-prepush @@ -5,8 +5,10 @@ * * THIS IS A GUARDRAIL, NOT ENFORCEMENT. `git push --no-verify` bypasses it, as * does `GSTACK_REDACT_PREPUSH=skip`. It catches accidental credential pushes, - * the most common real-world leak. It does NOT scan history, binary/LFS/submodule - * files, or non-added lines. History scanning is /cso's job. + * the most common real-world leak. It does NOT scan history, LFS-stored or + * submodule content, or non-added lines. History scanning is /cso's job. Files + * git calls binary (a NUL byte, UTF-16, a `-diff` attribute) are diffed with + * --text, read through scannableText and held to credential findings (GST-2195). * * Git pre-push interface: refs are read from STDIN, one per line: * @@ -42,6 +44,7 @@ import * as fs from "fs"; import * as path from "path"; import { normalizeWithMap, scan, type Finding, type ScanOptions } from "../lib/redact-engine"; import { mkdirpSync } from "../lib/fs-utils"; +import { scannableText } from "../lib/pregate/checks/secrets"; import { resolveStateRoot } from "../lib/state-root"; import { evaluateSealed, publishedCommits, renderSealed, sealedErrorReport, type SealedReport } from "../lib/sealed-guard"; @@ -254,7 +257,7 @@ function addedLinesFromNewCommits(localSha: string, remoteSha: string): AddedFil // gitStrict: a failed diff must never read as "nothing added" (#1946). // Collected per commit so a finding names the commit whose file line it is. out.push(...collectAddedLines(gitStrict([ - "show", "--unified=0", "--no-color", "--no-ext-diff", "--no-textconv", + "show", "--text", "--unified=0", "--no-color", "--no-ext-diff", "--no-textconv", "--src-prefix=a/", "--dst-prefix=b/", "--cc", "--format=", sha, ]), sha)); } @@ -267,7 +270,7 @@ function addedLinesFromNewCommits(localSha: string, remoteSha: string): AddedFil * `text`, read from the hunk headers; `commit` is set when the lines came from * one commit's diff rather than the net diff of the pushed range. */ -interface AddedFile { file: string; text: string; lines: number[]; commit?: string } +interface AddedFile { file: string; text: string; lines: number[]; commit?: string; binary?: boolean } /** Return the added lines for a ref update being pushed. */ function addedLinesFor(localSha: string, remoteSha: string): AddedFile[] { @@ -295,8 +298,11 @@ function addedLinesFor(localSha: string, remoteSha: string): AddedFile[] { // secrets. Reachable from ordinary user config, not hypothetical. (#2498) // --no-textconv: a .gitattributes textconv driver can likewise rewrite // content before we ever see it. (#2498) + // --text: without it a file git calls binary (a NUL byte, UTF-16, a + // `-diff` attribute the push itself can add) prints "Binary files differ" + // and no added lines, so its credentials pushed unscanned. (GST-2195) const diff = gitStrict([ - "diff", "--unified=0", "--no-color", "--no-ext-diff", "--no-textconv", + "diff", "--text", "--unified=0", "--no-color", "--no-ext-diff", "--no-textconv", "--src-prefix=a/", "--dst-prefix=b/", range, ]); return collectAddedLines(diff); @@ -323,7 +329,9 @@ function collectAddedLines(diff: string, commit?: string): AddedFile[] { let nextLine = 0; const flush = () => { const lines = releaseVersionExempt(file, fileLines); - if (lines.length) added.push({ file, text: lines.join("\n"), lines: lineNumbers, ...(commit ? { commit } : {}) }); + const text = lines.join("\n"); + const binary = text.includes("\0"); + if (lines.length) added.push({ file, text: binary ? scannableText(text) : text, lines: lineNumbers, ...(commit ? { commit } : {}), ...(binary ? { binary } : {}) }); fileLines = []; lineNumbers = []; }; @@ -700,6 +708,7 @@ function main() { ? new Set(scanAddedLines(part.text, { ...opts, allowlist }).map(findingKey)) : null; for (const f of findings) { + if (part.binary && f.category !== "secret") continue; if (kept && !kept.has(findingKey(f))) { if (f.severity === "HIGH" || f.severity === "MEDIUM") allowed.push(` ${f.severity} ${f.id} ${findingWhere(f, part)}\n`); continue; diff --git a/changelog.d/scan-binary-files-for-secrets.md b/changelog.d/scan-binary-files-for-secrets.md new file mode 100644 index 000000000..640ac44a0 --- /dev/null +++ b/changelog.d/scan-binary-files-for-secrets.md @@ -0,0 +1,4 @@ +--- +important: security +--- +- **The secret check and the pre-push hook now read files git calls binary.** A GitHub token in a file with a NUL byte, in UTF-16 text, or in a file a `.gitattributes` line marks `-diff` (which the same branch can add) used to pass both guards, because git printed "Binary files differ" instead of the added lines. Both now diff with `--text` and read each line of such a file three ways. They read it as little- and big-endian UTF-16 text without its NUL bytes, even when most of it is Cyrillic or CJK, and as bytes where each run of control or non-UTF-8 bytes reads as a space, so a token is found in whichever reading keeps it whole. That catches a token on one line of UTF-16 text. A keyword and its value split across two lines of UTF-16 text, such as an AWS secret key on the line under its YAML key, are not caught yet. UTF-32 text and compressed containers such as zip, docx and PDF files are not read. Only credential findings count in such a file, so a run of digits in a compressed image is not reported as a phone number; this repo's own PNG, JPEG, GIF and ICNS files add no finding. A very large binary is not read in pieces, so it fails closed instead of passing. A file whose scanned text is over the scanner's 16 MiB limit fails the `secrets` check, and a push whose diff is over 64 MiB is blocked; keep such files in Git LFS, whose small pointer files scan as text. The pregate `secrets` check also stops treating a merge-base diff git did not finish (an error, a kill or a full buffer) as complete: it is now `PREGATE_ENVIRONMENT`, never a pass. Its pass line names how many files it read, for example `12 added lines clean in 3 files (1 binary)`. diff --git a/docs/pregate.md b/docs/pregate.md index 99fe43682..d5e602ab5 100644 --- a/docs/pregate.md +++ b/docs/pregate.md @@ -219,6 +219,16 @@ Two stages with separate clocks, both printed: git diffed (a full-context `git diff`), never the working copy; an untracked file is read whole from disk. A file over the size limit, or whose full diff does not parse, is scanned on its added lines alone, without that context. + A file git calls binary (a NUL byte, UTF-16 text, or a `-diff` attribute) + is still scanned: the diff runs with `--text`, and each added line is read + three ways, as bytes with control runs turned into spaces and as little- and + big-endian UTF-16. Only credential findings count in such a file, so image + bytes that look like a phone number are not reported. A file whose scanned + text is over the scanner's 16 MiB limit fails the check, and the pre-push + hook blocks a push whose diff is over 64 MiB; keep large binaries in Git LFS, + whose small pointer files scan as text. A merge-base diff git does not finish + (an error, a kill or a full buffer) is `PREGATE_ENVIRONMENT`, never a pass. + The pass line names the files read: `12 added lines clean in 3 files (1 binary)`. - `strays`: a new file at the repository root, a new top-level directory, or a scratch name (`zz-*`, or `tmp` or `scratch` as the whole name stem, optionally numbered: `tmp.js`, `tmp2.js`, `tmp_1.txt`, `scratch.md`, but not diff --git a/lib/pregate/checks/secrets.ts b/lib/pregate/checks/secrets.ts index 5f27b8c8f..d08fee348 100644 --- a/lib/pregate/checks/secrets.ts +++ b/lib/pregate/checks/secrets.ts @@ -4,8 +4,9 @@ * go through bin/gstack-redact. Exit 0 is clean; 2 (MEDIUM) and 3 (HIGH) * fail unless the policy's `allowlist` names each finding (lib/pregate/allowlist.ts; * never env.value); a scanner that does not finish or exits outside its - * contract is PREGATE_ENVIRONMENT (kind tool), never a pass. Runs regardless - * of lint tools. + * contract is PREGATE_ENVIRONMENT (kind tool), never a pass, and so is a + * merge-base diff git did not finish. Files git calls binary are read too + * (scannableText). Runs regardless of lint tools. */ import { spawnSync } from 'node:child_process'; import * as fs from 'node:fs'; @@ -28,7 +29,9 @@ export function parseAddedLines(diff: string): Map { if (line.startsWith('diff --')) { file = ''; inHunk = false; continue; } if (line.startsWith('@@')) { inHunk = true; continue; } if (!inHunk) { if (line.startsWith('+++ ')) file = line.startsWith('+++ b/') ? line.slice(6) : ''; continue; } - if (line.startsWith('+')) files.set(file, [...(files.get(file) ?? []), line.slice(1)]); + if (!line.startsWith('+')) continue; + const lines = files.get(file); + if (lines) lines.push(line.slice(1)); else files.set(file, [line.slice(1)]); } return files; } @@ -69,6 +72,32 @@ export function parsePostimages(diff: string): Map { return files; } +/** + * Text the scanner can read from a file git calls binary, which the diff + * passes through with --text (GST-2195). Text with no NUL is returned as is. + * Each line is read three ways. The plain reading turns each run of NUL, + * other control bytes or bytes that are not UTF-8 (an image chunk, an + * executable's padding) into one space. The little- and big-endian UTF-16 + * readings first drop the NULs inside every run of character-then-NUL or + * NUL-then-character pairs, which UTF-16 text puts beside each character of + * its ASCII range however much of the file is Cyrillic or CJK, and set the run + * apart with a space on each side, so the bytes of a CJK character beside it + * cannot join a token. Each distinct reading stays on the line, so a token is + * found in whichever one keeps it whole: a plain token that a NUL-flanked + * byte beside it would glue or cut in a UTF-16 reading, or a UTF-16 token + * whose neighbour pairs up with it in the other byte order. Line breaks stay, so line numbers still hold. Callers keep only credential + * findings from such a file: privacy patterns read its bytes as noise (a run + * of digits in a compressed icon reads as a phone number). + */ +export function scannableText(text: string): string { + if (!text.includes('\0')) return text; + const noise = /[\0-\x08\x0b\x0c\x0e-\x1f\x7f\uFFFD]+/g; + return text.split('\n').map(line => { + const utf16 = (pairs: RegExp) => line.replace(pairs, run => ` ${run.replaceAll('\0', '')} `).replace(noise, ' '); + return [...new Set([utf16(/(?:[^\0-\x08\x0b\x0c\x0e-\x1f\x7f\uFFFD]\0){2,}/g), utf16(/(?:\0[^\0-\x08\x0b\x0c\x0e-\x1f\x7f\uFFFD]){2,}/g), line.replace(noise, ' ')])].join(' '); + }).join('\n'); +} + /** Files whose source-syntax exemption needs the whole file: a line added inside an existing `'''`/`"""` block is a literal (#3630). */ const LEXICAL_CONTEXT_SOURCE = /\.(?:dart|pyi?)$/i; const WHOLE_FILE_MAX_BYTES = 4 * 1024 * 1024; @@ -84,10 +113,15 @@ const WHOLE_FILE_MAX_BYTES = 4 * 1024 * 1024; * line. One too large, or whose full diff does not parse as one hunk holding * its added lines, is scanned under a neutral name, without that exemption. External diff and textconv * drivers are off: either can empty or rewrite the diff, and an empty diff - * reads as clean. + * reads as clean. --text keeps a file git calls binary (a NUL byte, UTF-16, a + * `-diff` attribute the branch itself can add) from printing "Binary files + * differ" in place of its added lines; each such file is marked `binary`, + * read through scannableText and held to its credential findings. A diff git did not finish (an exit, a kill, a + * full buffer) throws: its partial output is never scanned as complete. */ -export function addedLines(ctx: PregateContext): Array<{ file: string; text: string; keep?: number[] }> { - const diff = spawnSync('git', ['diff', '--no-color', '--no-ext-diff', '--no-textconv', '--src-prefix=a/', '--dst-prefix=b/', '--unified=0', '--diff-filter=ACMR', ctx.mergeBase, '--'], { cwd: ctx.repoRoot, encoding: 'utf8', timeout: 60_000, maxBuffer: 256 * 1024 * 1024, env: ctx.env }); +export function addedLines(ctx: PregateContext): Array<{ file: string; text: string; keep?: number[]; binary?: boolean }> { + const diff = spawnSync('git', ['diff', '--text', '--no-color', '--no-ext-diff', '--no-textconv', '--src-prefix=a/', '--dst-prefix=b/', '--unified=0', '--diff-filter=ACMR', ctx.mergeBase, '--'], { cwd: ctx.repoRoot, encoding: 'utf8', timeout: 60_000, maxBuffer: 256 * 1024 * 1024, env: ctx.env }); + if (diff.error || diff.status !== 0) throw new Error(`git diff ${ctx.mergeBase.slice(0, 12)} did not finish (${diff.error?.message ?? (diff.status === null ? `killed by ${diff.signal}` : `exit ${diff.status}: ${(diff.stderr ?? '').trim().split('\n').pop() ?? ''}`)})`); const files = parseAddedLines(diff.stdout ?? ''); const numbers = parseAddedLineNumbers(diff.stdout ?? ''); const whole = new Map(); @@ -113,13 +147,19 @@ export function addedLines(ctx: PregateContext): Array<{ file: string; text: str files.set(t.path, [...(files.get(t.path) ?? []), ...lines]); } catch { /* unreadable: skipped */ } } - return [...files].map(([file, lines]) => whole.get(file) ? { file, ...whole.get(file)! } : { file, text: lines.join('\n') }).filter(p => p.text.trim()); + return [...files] + .map(([file, lines]) => whole.get(file) ? { file, ...whole.get(file)! } : { file, text: lines.join('\n') }) + .map(p => p.text.includes('\0') ? { ...p, text: scannableText(p.text), binary: true } : p) + .filter(p => p.text.trim()); } type Finding = { id?: string; description?: string; severity?: string; category?: string; line?: number; col?: number; file: string; fileLine?: boolean }; type Scanned = Finding & AllowlistFinding; function run(ctx: PregateContext): CheckResult { - const parts = addedLines(ctx); + let parts: ReturnType; + try { parts = addedLines(ctx); } catch (err) { + return environmentResult({ id: 'secrets', stage: 'preflight', status: 'incomplete', inputs: baseInputs(ctx) }, launchCause('tool', (err as Error).message), 'git diff of the added lines did not finish; an incomplete scan never passes'); + } const inputs = baseInputs(ctx, { added_lines: sha256(JSON.stringify(parts)), ...(ctx.config.allowlist.length ? { allowlist: allowlistDigest(ctx.config.allowlist) } : {}) }); if (!parts.length) return { id: 'secrets', stage: 'preflight', status: 'pass', detail: 'no added lines', inputs }; // A run killed mid-scan under the same --run id must not keep its added-line copies: clear them before writing new ones. @@ -141,7 +181,7 @@ function run(ctx: PregateContext): CheckResult { high ||= r.status === 3; medium ||= r.status === 2; const lines = part.text.split('\n'); - try { for (const f of (JSON.parse(r.stdout).findings ?? []).filter((f: Omit) => !part.keep || f.id?.startsWith('engine.') || (f.line !== undefined && part.keep.includes(f.line))).map((f: Omit) => ({ ...f, id: f.id ?? 'finding', category: f.category ?? '?', severity: f.severity ?? '?', file: part.file, fileLine: !!part.keep, lineText: f.line ? lines[f.line - 1] : undefined }))) findings.push(f); } catch { parsedAll = false; } + try { for (const f of (JSON.parse(r.stdout).findings ?? []).filter((f: Omit) => (!part.binary || f.category === 'secret') && (!part.keep || f.id?.startsWith('engine.') || (f.line !== undefined && part.keep.includes(f.line)))).map((f: Omit) => ({ ...f, id: f.id ?? 'finding', category: f.category ?? '?', severity: f.severity ?? '?', file: part.file, fileLine: !!part.keep, lineText: f.line ? lines[f.line - 1] : undefined }))) findings.push(f); } catch { parsedAll = false; } } // The allowlist applies only to findings the scanner reported in full; an unparsed report keeps its exit-code verdict. const applied = parsedAll && ctx.config.allowlist.length ? applyAllowlist(findings, ctx.config.allowlist) : { kept: findings, allowlisted: [], untested: [], missed: [] }; @@ -155,9 +195,11 @@ function run(ctx: PregateContext): CheckResult { // there is no user to answer gstack-redact's per-finding question here. const blocking = kept.filter(f => f.severity === 'HIGH' || (f.severity === 'MEDIUM' && f.category === 'secret')); const advisory = kept.filter(f => f.severity === 'MEDIUM' && f.category !== 'secret'); + const binary = parts.filter(p => p.binary).length; + const scanned = `${parts.length} file${parts.length === 1 ? '' : 's'}${binary ? ` (${binary} binary)` : ''}`; const lineCount = parts.reduce((n, p) => n + (p.keep?.length ?? p.text.split('\n').length), 0); const fail = (lines: string[], detail: string): CheckResult => ({ ...base, status: 'fail', cause: 'diff', lines: [...lines, ...allowLines], code: 'PREGATE_SECRETS', detail: `${detail}${allowNote}`, fix: 'rotate and remove the credential from the added lines (history included); secrets cannot be downgraded (a benign finding is allowlisted by id, path and pattern in the policy, never env.value)' }); - if (!high && !medium) return { ...base, status: 'pass', ...(allowLines.length ? { lines: allowLines } : {}), detail: `${lineCount} added lines clean${kept.length ? ` (${kept.length} WARN)` : ''}${allowNote}` }; + if (!high && !medium) return { ...base, status: 'pass', ...(allowLines.length ? { lines: allowLines } : {}), detail: `${lineCount} added lines clean in ${scanned}${kept.length ? ` (${kept.length} WARN)` : ''}${allowNote}` }; if (blocking.length) return fail([...blocking, ...advisory].map(render), `${blocking.length} credential finding(s) in added lines (${blocking.map(f => f.id).join(', ')})`); if (high) return fail(kept.map(render), 'HIGH finding(s) in added lines'); return { ...base, status: 'warn', cause: 'diff', lines: [...advisory.map(render), ...allowLines], code: 'PREGATE_SECRETS', detail: `${advisory.length} MEDIUM non-credential finding(s) in added lines (${[...new Set(advisory.map(f => f.id))].join(', ')})${allowNote}`, fix: 'read each listed line; gstack-post asks about the same findings when the PR body quotes them; a benign one is allowlisted by id or category, path and pattern in the policy' }; diff --git a/test/pregate-secrets-source-context.test.ts b/test/pregate-secrets-source-context.test.ts index 87dd74d7a..f0d293ddd 100644 --- a/test/pregate-secrets-source-context.test.ts +++ b/test/pregate-secrets-source-context.test.ts @@ -8,15 +8,20 @@ import { spawnSync } from 'child_process'; import * as fs from 'fs'; import * as os from 'os'; import * as path from 'path'; -import { parseAddedLineNumbers, parseAddedLines } from '../lib/pregate/checks/secrets'; +import { parseAddedLineNumbers, parseAddedLines, secretsCheck } from '../lib/pregate/checks/secrets'; +import type { PregateContext } from '../lib/pregate/types'; import { cleanup, commit, makeFixtureRepo, runBin, BASE_PREGATE, type FixtureRepo } from './helpers/pregate-fixture'; const PREGATE = path.resolve(import.meta.dir, '..', 'bin', 'gstack-pregate'); const AWS = 'AKIA' + 'ZQ3XN7P2LM4RT8WV'; +const PAT = ['ghp', 'Zq8Lm2Xv7Nc4Rt9Wy1Kp6Hd3Fs5Gb0Jx8Ue2'].join('_'); +const utf16 = (text: string, bigEndian = false) => { const le = Buffer.from(`\uFEFF${text}`, 'utf16le'); return bigEndian ? le.swap16() : le; }; +/** Every file this checkout stores as binary (PNG, JPEG, GIF, ICNS): real assets, not synthetic noise. */ +const BINARY_ASSETS = spawnSync('git', ['ls-files', '--eol'], { cwd: path.resolve(import.meta.dir, '..'), encoding: 'utf8', timeout: 30_000 }).stdout.split('\n').filter(l => l.startsWith('i/-text')).map(l => l.split('\t').pop()!); const roots: string[] = []; afterEach(() => { for (const r of roots.splice(0)) fs.rmSync(r, { recursive: true, force: true }); }); -function repo(files: Record) { +function repo(files: Record) { const root = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'pregate-secrets-'))); roots.push(root); const dir = path.join(root, 'work'); @@ -37,7 +42,7 @@ function repo(files: Record) { git('push', '-q', 'origin', 'main'); git('fetch', '-q', 'origin'); git('checkout', '-qb', 'feat'); - for (const [rel, text] of Object.entries(files)) fs.writeFileSync(path.join(dir, rel), text); + for (const [rel, text] of Object.entries(files)) { fs.mkdirSync(path.dirname(path.join(dir, rel)), { recursive: true }); fs.writeFileSync(path.join(dir, rel), text); } git('add', '.'); git('commit', '-qm', 'feat'); const secrets = () => { @@ -45,10 +50,18 @@ function repo(files: Record) { const check = JSON.parse(r.stdout).checks.find((c: { id: string }) => c.id === 'secrets'); return { status: check.status as string, detail: check.detail as string, lines: (check.lines ?? []) as string[] }; }; - return { dir, git, secrets }; + return { root, dir, git, env, secrets }; } describe('pregate secrets: source context and diff integrity', () => { + // Value: protects=a large added diff (a binary read with --text) parses in linear time; fails_when=each added line copies the file's earlier lines; why_new=GST-2195 review; seam=pure parser; no_claim=git's own speed + test('parseAddedLines reads 200,000 added lines in one file quickly (GST-2195)', () => { + const diff = 'diff --git a/big.bin b/big.bin\n+++ b/big.bin\n@@ -0,0 +1,200000 @@\n' + '+x\n'.repeat(200_000); + const started = performance.now(); + expect(parseAddedLines(diff).get('big.bin')?.length).toBe(200_000); + expect(performance.now() - started).toBeLessThan(2_000); + }); + test('parseAddedLines groups by file and keeps a "+++" content line inside a hunk', () => { const diff = ['diff --git a/a.ts b/a.ts', '--- a/a.ts', '+++ b/a.ts', '@@ -0,0 +1,2 @@', '+one', '+++two', 'diff --git a/b.env b/b.env', '--- /dev/null', '+++ b/b.env', '@@ -0,0 +1 @@', '+three'].join('\n'); expect([...parseAddedLines(diff)]).toEqual([['a.ts', ['one', '++two']], ['b.env', ['three']]]); @@ -224,4 +237,66 @@ describe('pregate secrets: source context and diff integrity', () => { f.git('config', 'diff.external', driver); expect(f.secrets().status).toBe('fail'); }); + + // Value: protects=a token in a file git calls binary fails the never-downgradable secrets check like the same token in plain text; + // fails_when=a NUL byte, a -diff line the branch itself adds to .gitattributes, or UTF-16 text passes the check unscanned; why_new=GSTA-461 item 5 reached GitHub with both guards green; seam=fixture repo; no_claim=UTF-16 outside the ASCII range + test('a token in a file git calls binary fails like plain text: a NUL byte, a -diff attribute the branch adds, UTF-16LE and UTF-16BE (GST-2195)', () => { + const r = repo({ 'plain.txt': `token ${PAT}\n`, 'nul.txt': `\0token ${PAT}\n`, '.gitattributes': '*.jsonl -diff\n', 'out.jsonl': `{"token":"${PAT}"}\n`, 'le.txt': utf16(`token ${PAT}\r\n`), 'be.txt': utf16(`token ${PAT}\n`, true) }).secrets(); + expect(r.status).toBe('fail'); + for (const file of ['plain.txt', 'nul.txt', 'out.jsonl', 'le.txt', 'be.txt']) expect(r.lines).toContainEqual(expect.stringMatching(new RegExp(`^HIGH github\\.pat .*\\(${file.replace('.', '\\.')}, added line 1\\)$`))); + }); + + // Value: protects=a token in UTF-16 text that is mostly outside the ASCII range is still found; fails_when=Cyrillic or CJK text around the token makes its NUL bytes read as spaces; + // why_new=GST-2195 review: localized UTF-16 files (Localizable.strings, .rc) passed the check; seam=fixture repo; no_claim=UTF-32 + test('a token in UTF-16 text that is mostly Cyrillic or CJK fails like plain text (GST-2195)', () => { + const ru = Array(8).fill('Привет мир, это файл настроек').join('\r\n'); + const zh = Array(8).fill('这是一个本地化的字符串文件内容').join('\r\n'); + const r = repo({ 'ru.strings': utf16(`${ru}\r\ntoken ${PAT}\r\n`), 'zh.strings': utf16(`${zh}\r\ntoken ${PAT}\r\n`), 'zh-be.strings': utf16(`${zh}\ntoken ${PAT}\n`, true) }).secrets(); + expect(r.status).toBe('fail'); + for (const file of ['ru.strings', 'zh.strings', 'zh-be.strings']) expect(r.lines).toContainEqual(expect.stringMatching(new RegExp(`^HIGH github\\.pat .*\\(${file.replace('.', '\\.')}, added line 9\\)$`))); + }); + + // Value: protects=a UTF-16 token right beside a CJK character is found, and so is a plain token beside a run of character-and-NUL pairs; fails_when=the CJK character's bytes glue onto the token, or padding the run cuts the plain token; + // why_new=GST-2195 review (outside voice): U+4E2D before a token read as "-N" glued to it; seam=fixture repo; no_claim=UTF-32 + test('a token right beside a CJK character in UTF-16, or beside a NUL run in binary content, fails like plain text (GST-2195)', () => { + const r = repo({ 'le.strings': utf16(`中${PAT}中文\r\n`), 'be.strings': utf16(`中${PAT}中文\n`, true), 'run.bin': Buffer.from(`\x89\0\0\x01${PAT}\0x\0y\0z\0\n`, 'latin1') }).secrets(); + expect(r.status).toBe('fail'); + for (const file of ['le.strings', 'be.strings', 'run.bin']) expect(r.lines).toContainEqual(expect.stringMatching(new RegExp(`^HIGH github\\.pat .*\\(${file.replace('.', '\\.')}, added line 1\\)$`))); + }); + + // Value: protects=a plain token in binary content stays whole when a NUL-flanked byte sits beside it; fails_when=reading the bytes as UTF-16 glues that byte to the token or cuts its edge character off; + // why_new=GST-2195 review: the UTF-16 reading hid tokens the plain reading found; seam=fixture repo; no_claim=tokens split by a NUL inside them + test.each([ + ['a NUL terminator, then a little-endian integer', PAT + '\0' + '1\0\0\0'], + ['a one-character string after it', '\0' + PAT + '\0x\0'], + ['a one-character string before it', '\0x\0' + PAT + '\0'], + ['a uint16 length before it', '\0\0' + '4\0' + PAT], + ['UTF-16 runs on both sides', 'A\0B\0' + PAT + '\0C\0D\0'], + ])('a token in binary content beside %s fails like plain text (GST-2195)', (_, body) => { + const r = repo({ 'blob.bin': Buffer.concat([Buffer.from([0x89, 0, 0, 1]), Buffer.from(body, 'latin1')]) }).secrets(); + expect(r.status).toBe('fail'); + expect(r.lines).toContainEqual(expect.stringMatching(/^HIGH github\.pat .*\(blob\.bin, added line 1\)$/)); + }); + + // Value: protects=reading binary files does not turn real images into findings; fails_when=a PNG, JPEG, GIF or ICNS from this repo warns or fails the check; + // why_new=GST-2195 scans binary content that was skipped before; seam=fixture repo; no_claim=compiled executables (the repo stores none) + test('the real binary assets this repo stores add no finding, and the pass names how many files it read (GST-2195)', () => { + expect(BINARY_ASSETS.length).toBeGreaterThanOrEqual(10); + const root = path.resolve(import.meta.dir, '..'); + const r = repo(Object.fromEntries(BINARY_ASSETS.map(rel => [rel, fs.readFileSync(path.join(root, rel))]))).secrets(); + expect({ status: r.status, lines: r.lines }).toEqual({ status: 'pass', lines: [] }); + expect(r.detail).toContain(`in ${BINARY_ASSETS.length} files (${BINARY_ASSETS.length} binary)`); + }); + + // Value: protects=a diff git could not produce in full never reads as a clean scan; fails_when=a failed, killed or truncated merge-base diff passes as "no added lines"; + // why_new=the pre-push hook fails closed here (#1946) and pregate did not; seam=check run on a real repo with an unreadable merge base; no_claim=the 60 s timeout itself + test('a merge-base diff git could not finish is PREGATE_ENVIRONMENT, never a clean scan (GST-2195)', async () => { + const f = repo({ 'a.txt': `token ${PAT}\n` }); + const scratch = path.join(f.root, 'scratch'); + fs.mkdirSync(scratch); + const ctx = { repoRoot: f.dir, mergeBase: '0'.repeat(40), wtree: 'w', head: 'h', baseRef: 'origin/main', baseSha: '0'.repeat(40), touched: [], config: { allowlist: [] }, scratch, timeoutMs: 60_000, env: f.env } as unknown as PregateContext; + const check = await secretsCheck.run(ctx); + expect({ status: check.status, code: check.code }).toEqual({ status: 'incomplete', code: 'PREGATE_ENVIRONMENT' }); + expect(check.detail).toContain('git diff'); + }); }); diff --git a/test/redact-prepush-hook.test.ts b/test/redact-prepush-hook.test.ts index 0880322f6..4b30ed0e6 100644 --- a/test/redact-prepush-hook.test.ts +++ b/test/redact-prepush-hook.test.ts @@ -71,6 +71,18 @@ const ZERO = "0000000000000000000000000000000000000000"; // repo's own pre-push scanner (correctly) blocks live-format AWS key shapes, // and the placeholder-suppressed docs key would defeat these detection tests. const FAKE_AWS_KEY = ['AKIA', '1234567890ABCDEF'].join(''); +const FAKE_PAT = ["ghp", "Zq8Lm2Xv7Nc4Rt9Wy1Kp6Hd3Fs5Gb0Jx8Ue2"].join("_"); +const utf16 = (text: string, bigEndian = false) => { const le = Buffer.from(`\uFEFF${text}`, "utf16le"); return bigEndian ? le.swap16() : le; }; + +function commitFiles(files: Record, msg: string): string { + for (const [rel, content] of Object.entries(files)) { + fs.mkdirSync(path.dirname(path.join(repo, rel)), { recursive: true }); + fs.writeFileSync(path.join(repo, rel), content); + } + git(["add", "-A"]); + git(["commit", "-q", "-m", msg]); + return git(["rev-parse", "HEAD"]); +} beforeEach(() => { @@ -774,3 +786,45 @@ describe("#3048: the pushed diff is scanned per file, so TS/JSX source syntax is expect(mediums(runHook(`refs/heads/main ${git(["rev-parse", "HEAD"])} refs/heads/main ${base}\n`).stderr)).toBe(1); }); }); + +// GST-2195 (GSTA-461 item 5): a file git calls binary printed "Binary files differ" and no added lines, so +// a token behind a NUL byte, a -diff attribute or UTF-16 text pushed with the hook green. +describe("files git calls binary are scanned (GST-2195)", () => { + test("a token behind a NUL byte, a -diff attribute the push adds, or UTF-16 blocks like plain text", () => { + const base = git(["rev-parse", "HEAD"]); + const shapes: Array<[string, Record]> = [ + ["plain", { "plain.txt": `token ${FAKE_PAT}\n` }], + ["nul", { "nul.txt": `\0token ${FAKE_PAT}\n` }], + ["-diff", { ".gitattributes": "*.jsonl -diff\n", "out.jsonl": `{"token":"${FAKE_PAT}"}\n` }], + ["utf-16le", { "le.txt": utf16(`token ${FAKE_PAT}\r\n`) }], + ["utf-16be", { "be.txt": utf16(`token ${FAKE_PAT}\n`, true) }], + ]; + const results = shapes.map(([shape, files]) => { + git(["reset", "-q", "--hard", base]); + const head = commitFiles(files, shape); + const { code, stderr } = runHook(`refs/heads/main ${head} refs/heads/main ${base}\n`); + return { shape, code, pat: stderr.includes("github.pat") }; + }); + expect(results).toEqual(shapes.map(([shape]) => ({ shape, code: 1, pat: true }))); + }); + + test("a token added to an existing binary file blocks; the bytes already there are not rescanned", () => { + const png = fs.readFileSync(path.resolve(import.meta.dir, "..", "extension", "icons", "icon-16.png")); + const base = commitFiles({ "icon.png": Buffer.concat([png, Buffer.from(`\n${FAKE_PAT}\n`)]) }, "png with token"); + const head = commitFiles({ "icon.png": Buffer.concat([png, Buffer.from(`\n${FAKE_PAT}\nmore\n`)]) }, "touch png"); + expect(runHook(`refs/heads/main ${head} refs/heads/main ${base}\n`).code).toBe(0); + const leak = commitFiles({ "icon.png": Buffer.concat([png, Buffer.from(`\n${FAKE_PAT}\nmore\n\0${FAKE_PAT.replace("Zq8", "Yr7")}\n`)]) }, "second token"); + const { code, stderr } = runHook(`refs/heads/main ${leak} refs/heads/main ${head}\n`); + expect({ code, pat: stderr.includes("github.pat") }).toEqual({ code: 1, pat: true }); + }); + + test("the real binary assets this repo stores push clean", () => { + const root = path.resolve(import.meta.dir, ".."); + const assets = spawnSync("git", ["ls-files", "--eol"], { cwd: root, encoding: "utf8", timeout: 30_000 }).stdout.split("\n").filter((l) => l.startsWith("i/-text")).map((l) => l.split("\t").pop()!); + expect(assets.length).toBeGreaterThanOrEqual(10); + const base = git(["rev-parse", "HEAD"]); + const head = commitFiles(Object.fromEntries(assets.map((rel) => [rel, fs.readFileSync(path.join(root, rel))])), "assets"); + const { code, stderr } = runHook(`refs/heads/main ${head} refs/heads/main ${base}\n`); + expect({ code, stderr }).toEqual({ code: 0, stderr: "" }); + }); +});