From d7089517a26520b228814eac1209879ecb30b455 Mon Sep 17 00:00:00 2001 From: Petrus Pennanen Date: Sun, 20 Sep 2026 09:51:04 +0200 Subject: [PATCH 1/7] Scan reachable history for secrets, not just the working tree check-stageable-secrets.mjs reads files from disk and answers "would `git add -A` stage a credential right now". A key that was committed and deleted in a later commit is gone from the working tree and still sits in the pack file, readable by anyone who clones. This repo is public, so a snapshot pushed into it is world-readable the instant it lands. scripts/scan-history-for-secrets.mjs walks every blob reachable from all refs (git rev-list --objects --all, read through git cat-file --batch) and matches it against the shared pattern list. The patterns, the binary-extension skip list and the size cap move to src/secret-patterns.mjs, imported by both scanners. Two lists is one scanner and one decoy: the day someone adds a key format to the list they are looking at, the other keeps passing while missing that exact shape. A test fails if either script grows a private copy. The shared matcher returns a rule name, a line and a length, never the matched text, so check-stageable-secrets.mjs no longer prints a 6 character prefix of a match either. Fail closed. Unreadable object, undecodable blob, blob over the size cap, git error or timeout all exit 3 (could-not-complete), never 0. A shallow clone exits 4: its cut-off history is not clean, it is unexamined. Exit codes are documented in --help. Every run reports how many commits and blobs it actually examined, and calls out a one-commit history. Also adds two credential formats the bash pre-commit hook already knew about and this list did not: antfarm_ room keys and xai- keys. Co-Authored-By: Claude Opus 5 --- package.json | 1 + scripts/check-stageable-secrets.mjs | 60 ++- scripts/scan-history-for-secrets.mjs | 548 +++++++++++++++++++++++++ src/secret-patterns.mjs | 81 ++++ test/scan-history-for-secrets.test.mjs | 328 +++++++++++++++ 5 files changed, 985 insertions(+), 33 deletions(-) create mode 100644 scripts/scan-history-for-secrets.mjs create mode 100644 src/secret-patterns.mjs create mode 100644 test/scan-history-for-secrets.test.mjs diff --git a/package.json b/package.json index d46e31b..58a35a1 100644 --- a/package.json +++ b/package.json @@ -10,6 +10,7 @@ }, "scripts": { "test": "node --test test/*.test.mjs packages/user-intent-kit/test/*.test.js", + "scan:history": "node scripts/scan-history-for-secrets.mjs", "start": "node bin/cli.mjs serve", "mcp": "node bin/iak-mcp.mjs", "relay": "node scripts/local-relay.mjs", diff --git a/scripts/check-stageable-secrets.mjs b/scripts/check-stageable-secrets.mjs index 7e00895..85084e7 100755 --- a/scripts/check-stageable-secrets.mjs +++ b/scripts/check-stageable-secrets.mjs @@ -38,28 +38,20 @@ import { execFileSync } from 'node:child_process'; import { readFileSync, statSync } from 'node:fs'; +import { MAX_BYTES, SKIP_EXT, matchSecret, ruleLabels } from '../src/secret-patterns.mjs'; -// Shapes worth stopping for. Deliberately narrow: a scanner that cries wolf -// gets disabled, and a disabled scanner is worse than none. Every pattern -// here is a real credential format we use or plausibly would. -const PATTERNS = [ - [/xfb_[a-f0-9]{32,}/i, 'GroupMind agent key'], - // sk-ant- BEFORE the general sk- rule: the broad one also matches an - // Anthropic key and would mislabel it, and a wrong label sends someone - // rotating the wrong credential. - [/sk-ant-[A-Za-z0-9_-]{20,}/, 'Anthropic API key'], - [/sk-[A-Za-z0-9_-]{20,}/, 'OpenAI-style secret key'], - [/AIza[0-9A-Za-z_-]{35}/, 'Google API key'], - [/gh[pousr]_[A-Za-z0-9]{36,}/, 'GitHub token'], - [/github_pat_[A-Za-z0-9_]{50,}/, 'GitHub fine-grained PAT'], - [/-----BEGIN [A-Z ]*PRIVATE KEY-----/, 'private key'], - [/\b(?:api[_-]?key|secret|password|token)\s*[:=]\s*['"]?[A-Za-z0-9_\-]{24,}/i, - 'assigned secret-looking value'], -]; - -// Binaries and lockfiles produce noise, not credentials. -const SKIP_EXT = /\.(png|jpe?g|gif|webp|ico|pdf|zip|gz|tgz|jar|aab|apk|keystore|jks|woff2?|ttf|mp[34]|mov|wav)$/i; -const MAX_BYTES = 2 * 1024 * 1024; +// The pattern list, the binary-extension skip list and the size cap now live +// in src/secret-patterns.mjs, shared with scripts/scan-history-for-secrets.mjs. +// They were moved there the day the history scanner was written, because the +// alternative was a second list - and a second list is how one of them rots +// unnoticed while still looking healthy. That is the same shape of mistake the +// note above describes. Do not re-introduce a local copy here; a test asserts +// that neither scanner has one. +// +// matchSecret() returns a rule name, a line and a length, never the matched +// text. That is why the finding below no longer prints a 6-character prefix of +// the match: a prefix of a live key in a CI transcript is still a prefix of a +// live key. const git = (args) => execFileSync('git', args, { encoding: 'utf8', maxBuffer: 64 * 1024 * 1024 }); @@ -92,17 +84,19 @@ function scan(path) { return null; // unreadable, gone, or a directory: not our problem } if (text.includes('\0')) return null; // binary - for (const [re, label] of PATTERNS) { - const m = text.match(re); - if (m) { - // Report WHERE and WHAT, never the value itself. This output ends up in - // CI logs and terminal scrollback, and a scanner that prints the secret - // it found has simply moved the leak. - const line = text.slice(0, m.index).split('\n').length; - return { label, line, hint: `${m[0].slice(0, 6)}…(${m[0].length} chars)` }; - } - } - return null; + // Report WHERE and WHICH RULE, never the value itself. This output ends up + // in CI logs and terminal scrollback, and a scanner that prints the secret it + // found has simply moved the leak. + const hit = matchSecret(text); + if (!hit) return null; + return { label: hit.label, line: hit.line, hint: `${hit.length} chars, value not printed` }; +} + +// Shared with the history scanner; a test compares the two outputs so the +// lists cannot drift apart silently. +if (process.argv.includes('--print-rules')) { + console.log(ruleLabels().join('\n')); + process.exit(0); } const findings = []; @@ -119,7 +113,7 @@ if (findings.length === 0) { console.error(`FAIL: ${findings.length} stageable file(s) contain credential-shaped data\n`); for (const f of findings) { console.error(` ${f.file}:${f.line}`); - console.error(` ${f.label} — ${f.hint}\n`); + console.error(` ${f.label} - ${f.hint}\n`); } console.error('These are NOT committed yet, and this repo is public.'); console.error('Fix by ignoring the file, not by deleting it — something may be using it:'); diff --git a/scripts/scan-history-for-secrets.mjs b/scripts/scan-history-for-secrets.mjs new file mode 100644 index 0000000..796c06a --- /dev/null +++ b/scripts/scan-history-for-secrets.mjs @@ -0,0 +1,548 @@ +#!/usr/bin/env node +// SPDX-License-Identifier: AGPL-3.0-only +// +// Scan REACHABLE GIT HISTORY for credentials - not the working tree. +// +// THE GAP THIS CLOSES. scripts/check-stageable-secrets.mjs reads files from +// disk and answers one question: "would `git add -A` stage a credential right +// now". That is a useful question and it is not this one. A key that was +// committed on Tuesday and deleted on Wednesday is gone from the working tree, +// gone from `git status`, gone from the file listing - and still sits in the +// pack file, one `git log -p` away from anyone who clones the repo. To the +// disk-reading scanner that repo looks spotless. It is not: it is leaking. +// +// This repo is PUBLIC, and a snapshot pushed into it is world-readable the +// instant it lands, with no private staging period in which to notice. +// +// WHAT IT DOES. Every blob reachable from every ref (`git rev-list +// --objects --all`), read through `git cat-file --batch`, matched against the +// SHARED pattern list in src/secret-patterns.mjs - shared so that the two +// scanners cannot drift apart, which is the house defect this repo already has +// a written-up history of. +// +// WHAT IT REFUSES TO DO. +// +// 1. It never prints a matched value, not partially, not redacted with a +// prefix. Path + commit + rule name is enough to act on. A prefix is not +// "safe", it is the first six characters of a live key in a CI log. +// +// 2. It never renders "I could not check" as "clean". An unreadable object, +// a blob it cannot decode, a blob over the size cap, a timeout: all of +// those exit 3 (could-not-complete), never 0. This is the single most +// repeated bug in this codebase and it gets its own exit code. +// +// 3. It never calls a shallow clone clean. A `--depth` clone's history is +// not absent, it is UNEXAMINED, and the blobs that were cut off are the +// old ones - which is precisely where a deleted-but-reachable key lives. +// Shallow exits 4 and says so at the top of the report. +// +// 4. It always says how much it looked at. A freshly created snapshot repo +// has one commit; "history clean" after examining one commit is not +// reassurance, it is the false confidence this tool exists to prevent. +// +// Run: node scripts/scan-history-for-secrets.mjs [repo-path] [--json] +// Help: node scripts/scan-history-for-secrets.mjs --help + +import { execFileSync } from 'node:child_process'; +import { existsSync } from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { + MAX_BYTES, + SKIP_EXT, + matchSecret, + ruleLabels, +} from '../src/secret-patterns.mjs'; + +// Documented in --help. Verdict precedence when several apply: +// FOUND > INCOMPLETE > SHALLOW > CLEAN. Anything that is not a proven-clean +// full scan of a complete history is a non-zero exit. +const EXIT = { + CLEAN: 0, + FOUND: 1, + USAGE: 2, + INCOMPLETE: 3, + SHALLOW: 4, +}; + +const DEFAULT_MAX_SECONDS = 600; + +const HELP = `scan-history-for-secrets - look for credentials in reachable git history + +USAGE + node scripts/scan-history-for-secrets.mjs [repo-path] [options] + + repo-path repository to scan (default: the current working directory). + +OPTIONS + --json machine-readable report on stdout, nothing else on stdout + --print-rules print the rule names this build scans for, then exit 0 + --max-seconds=N wall-clock budget (default ${DEFAULT_MAX_SECONDS}); exceeding it is + could-not-complete, NOT a pass + --max-bytes=N per-blob size cap (default ${MAX_BYTES}); a blob over the cap + is reported unexamined, NOT passed + -h, --help this text + +WHAT IS SCANNED + Every blob reachable from all refs (git rev-list --objects --all), including + blobs whose file was deleted in a later commit. That is the whole point: a + deleted secret is invisible to a working-tree scan and perfectly readable to + anyone who clones the repo. + + Blobs whose path has a known binary media extension are skipped by policy and + counted separately as NOT examined. Everything else is decoded as UTF-8 and + matched against the shared pattern list in src/secret-patterns.mjs - the same + list scripts/check-stageable-secrets.mjs uses. + +OUTPUT + Findings report the blob path, the commit that introduced the blob, the blob + sha and the rule name. The matched value is never printed, in any mode, not + even truncated. Use the path and commit to inspect it yourself, in private. + +EXIT CODES + 0 clean every reachable blob was examined and nothing matched + 1 found at least one match (report says where, never what) + 2 usage bad arguments, or the path is not a git repository + 3 incomplete something could not be checked: unreadable object, blob that + would not decode, blob over the size cap, git error, timeout. + This is NOT a pass. "I could not check" is its own answer. + 4 shallow the clone is shallow (.git/shallow / --depth). The part that + was scanned may be clean; the part that was cut off is + unexamined, and unexamined is not clean. + + Precedence when several apply: 1 > 3 > 4 > 0. +`; + +class Deadline { + constructor(seconds) { + this.limitMs = seconds * 1000; + this.startedAt = Date.now(); + } + remainingMs() { + return this.limitMs - (Date.now() - this.startedAt); + } + check() { + if (this.remainingMs() <= 0) { + throw new Error(`time budget exceeded (${this.limitMs / 1000}s)`); + } + } + elapsedMs() { + return Date.now() - this.startedAt; + } +} + +/** Did this child process die on the clock rather than answer the question? */ +function isTimeout(err) { + if (!err) return false; + if (err.code === 'ETIMEDOUT' || err.signal === 'SIGTERM') return true; + return String(err.message ?? '').includes('time budget exceeded'); +} + +function makeGit(repo, deadline) { + return function git(args, { input, encoding = 'utf8' } = {}) { + deadline.check(); + return execFileSync('git', ['-C', repo, ...args], { + // Buffer, not string: with encoding 'buffer' node refuses to encode a + // string stdin, and the blob reader needs raw bytes back. + input: typeof input === 'string' ? Buffer.from(input, 'utf8') : input, + encoding, + maxBuffer: 256 * 1024 * 1024, + timeout: Math.max(1, deadline.remainingMs()), + stdio: ['pipe', 'pipe', 'pipe'], + }); + }; +} + +function parseArgs(argv) { + const opts = { + repo: process.cwd(), + json: false, + help: false, + printRules: false, + maxSeconds: DEFAULT_MAX_SECONDS, + maxBytes: MAX_BYTES, + }; + let sawRepo = false; + for (const arg of argv) { + if (arg === '--json') opts.json = true; + else if (arg === '--help' || arg === '-h') opts.help = true; + else if (arg === '--print-rules') opts.printRules = true; + else if (arg.startsWith('--max-seconds=')) { + opts.maxSeconds = Number(arg.slice('--max-seconds='.length)); + if (!Number.isFinite(opts.maxSeconds) || opts.maxSeconds <= 0) { + return { error: `bad --max-seconds: ${arg}` }; + } + } else if (arg.startsWith('--max-bytes=')) { + opts.maxBytes = Number(arg.slice('--max-bytes='.length)); + if (!Number.isFinite(opts.maxBytes) || opts.maxBytes <= 0) { + return { error: `bad --max-bytes: ${arg}` }; + } + } else if (arg.startsWith('-')) { + return { error: `unknown option: ${arg}` }; + } else if (sawRepo) { + return { error: `unexpected extra argument: ${arg}` }; + } else { + opts.repo = arg; + sawRepo = true; + } + } + return { opts }; +} + +/** + * Reachable blobs, as [{ sha, path }]. `git rev-list --objects --all` emits + * " " for blobs and trees and a bare "" for commits; a blob + * can appear at several paths, and the first one is enough to point a human at + * the right place. + */ +function reachableObjects(git) { + const out = git(['rev-list', '--objects', '--all']); + const pathBySha = new Map(); + const order = []; + for (const line of out.split('\n')) { + if (!line) continue; + const sp = line.indexOf(' '); + if (sp === -1) continue; // commit or tag object: no path + const sha = line.slice(0, sp); + if (pathBySha.has(sha)) continue; + pathBySha.set(sha, line.slice(sp + 1)); + order.push(sha); + } + return { pathBySha, order }; +} + +/** Split [{sha,size}] into chunks of roughly `budget` bytes for cat-file --batch. */ +function chunkBySize(items, budget) { + const chunks = []; + let current = []; + let total = 0; + for (const item of items) { + if (current.length > 0 && total + item.size > budget) { + chunks.push(current); + current = []; + total = 0; + } + current.push(item); + total += item.size; + } + if (current.length > 0) chunks.push(current); + return chunks; +} + +/** + * Read a chunk of blobs in one `git cat-file --batch` call and hand each + * body to `onBlob`. The batch format is " \n\n". + */ +function readBatch(git, shas, onBlob) { + const buf = git(['cat-file', '--batch'], { + input: shas.join('\n') + '\n', + encoding: 'buffer', + }); + let at = 0; + while (at < buf.length) { + const nl = buf.indexOf(0x0a, at); + if (nl === -1) throw new Error('git cat-file --batch: truncated header'); + const header = buf.toString('utf8', at, nl); + const parts = header.split(' '); + if (parts.length < 3) { + // " missing" - the object vanished between listing and reading. + throw new Error(`git cat-file --batch: unreadable object (${header})`); + } + const [sha, , sizeStr] = parts; + const size = Number(sizeStr); + const start = nl + 1; + const end = start + size; + if (end > buf.length) throw new Error('git cat-file --batch: truncated body'); + onBlob(sha, buf.subarray(start, end)); + at = end + 1; // trailing newline + } +} + +/** Is `body` something we can honestly claim to have read as text? */ +function undecodableReason(body) { + if (body.includes(0)) return 'nul-bytes'; + // A lossy decode means we scanned something other than what is stored, so + // "no match" would be a claim about the wrong bytes. + const text = body.toString('utf8'); + if (Buffer.byteLength(text, 'utf8') !== body.length) return 'invalid-utf8'; + return null; +} + +/** + * The commit that introduced a blob. Only ever called for findings, so the + * cost of a --find-object walk is paid at most a handful of times. + */ +function introducingCommit(git, sha) { + try { + const out = git(['log', '--all', '--format=%H', '--find-object', sha]); + const lines = out.split('\n').filter(Boolean); + return lines.length > 0 ? lines[lines.length - 1] : null; + } catch { + return null; + } +} + +function isShallow(git, repo) { + let flagged = false; + try { + flagged = git(['rev-parse', '--is-shallow-repository']).trim() === 'true'; + } catch { + // older git: fall through to the file check + } + try { + const gitDir = git(['rev-parse', '--absolute-git-dir']).trim(); + if (existsSync(path.join(gitDir, 'shallow'))) flagged = true; + } catch { + // handled by the caller's error path + } + return flagged; +} + +function scanRepo(opts) { + const deadline = new Deadline(opts.maxSeconds); + const report = { + tool: 'scan-history-for-secrets', + repo: opts.repo, + verdict: 'clean', + exitCode: EXIT.CLEAN, + shallow: false, + examined: { + commits: 0, + refs: 0, + blobsExamined: 0, + bytesExamined: 0, + blobsSkippedBinaryMedia: 0, + blobsUnexamined: 0, + blobsReachable: 0, + }, + findings: [], + unexamined: [], + errors: [], + durationMs: 0, + }; + + let repoRoot; + const probe = makeGit(opts.repo, deadline); + try { + repoRoot = probe(['rev-parse', '--show-toplevel']).trim(); + } catch (err) { + // A git that timed out has told us NOTHING about the path. Reporting that + // as "not a git repository" would be a second-hand version of the bug this + // tool is about: an unanswered question rendered as an answer. + if (isTimeout(err)) throw err; + return { report, usageError: `not a git repository: ${opts.repo}` }; + } + report.repo = repoRoot; + + const git = makeGit(repoRoot, deadline); + + try { + report.shallow = isShallow(git, repoRoot); + report.examined.commits = Number(git(['rev-list', '--all', '--count']).trim()) || 0; + report.examined.refs = git(['for-each-ref', '--format=%(refname)']) + .split('\n').filter(Boolean).length; + + const { pathBySha, order } = reachableObjects(git); + + // One --batch-check pass gives type and size for everything, so the + // expensive --batch read only ever asks for blobs we mean to scan. + const checkOut = git(['cat-file', '--batch-check'], { input: order.join('\n') + '\n' }); + const blobs = []; + for (const line of checkOut.split('\n')) { + if (!line) continue; + const [sha, type, sizeStr] = line.split(' '); + if (type !== 'blob') continue; + blobs.push({ sha, size: Number(sizeStr) || 0, path: pathBySha.get(sha) ?? '(unknown path)' }); + } + report.examined.blobsReachable = blobs.length; + + const toRead = []; + for (const blob of blobs) { + if (SKIP_EXT.test(blob.path)) { + report.examined.blobsSkippedBinaryMedia += 1; + continue; + } + if (blob.size > opts.maxBytes) { + report.unexamined.push({ path: blob.path, blob: blob.sha, reason: 'over-size-cap' }); + continue; + } + toRead.push(blob); + } + + const bySha = new Map(toRead.map((b) => [b.sha, b])); + for (const chunk of chunkBySize(toRead, 32 * 1024 * 1024)) { + readBatch(git, chunk.map((b) => b.sha), (sha, body) => { + const blob = bySha.get(sha); + if (!blob) return; + const bad = undecodableReason(body); + if (bad) { + report.unexamined.push({ path: blob.path, blob: sha, reason: bad }); + return; + } + report.examined.blobsExamined += 1; + report.examined.bytesExamined += body.length; + const hit = matchSecret(body.toString('utf8')); + if (hit) { + // label + line + length only. The value stays in the repo, which is + // the one place it is already. + report.findings.push({ + rule: hit.label, + path: blob.path, + line: hit.line, + blob: sha, + commit: introducingCommit(git, sha), + }); + } + }); + } + } catch (err) { + report.errors.push(String(err && err.message ? err.message : err)); + } + + report.examined.blobsUnexamined = report.unexamined.length; + report.durationMs = deadline.elapsedMs(); + + if (report.findings.length > 0) { + report.verdict = 'found'; + report.exitCode = EXIT.FOUND; + } else if (report.errors.length > 0 || report.unexamined.length > 0) { + report.verdict = 'incomplete'; + report.exitCode = EXIT.INCOMPLETE; + } else if (report.shallow) { + report.verdict = 'shallow'; + report.exitCode = EXIT.SHALLOW; + } else { + report.verdict = 'clean'; + report.exitCode = EXIT.CLEAN; + } + // A shallow repo is never a clean verdict, whatever else happened. + if (report.shallow && report.verdict === 'clean') { + report.verdict = 'shallow'; + report.exitCode = EXIT.SHALLOW; + } + return { report }; +} + +function printHuman(report) { + const e = report.examined; + const out = report.verdict === 'clean' ? console.log : console.error; + + if (report.shallow) { + out(''); + out(' *** SHALLOW CLONE - THIS HISTORY WAS NOT FULLY EXAMINED ***'); + out(' Commits cut off by --depth were never fetched, so nothing here can'); + out(' speak for them. Re-run after: git fetch --unshallow'); + out(''); + } + + out(`repo: ${report.repo}`); + out(`examined: ${e.commits} commits, ${e.refs} refs, ${e.blobsExamined} of ${e.blobsReachable} reachable blobs (${e.bytesExamined} bytes) in ${report.durationMs} ms`); + if (e.blobsSkippedBinaryMedia > 0) { + out(`skipped: ${e.blobsSkippedBinaryMedia} binary media blob(s) by extension - NOT examined`); + } + if (e.commits <= 1) { + out('DEPTH: this history is 1 commit. A history scan of a fresh snapshot'); + out(' repo proves almost nothing - it has no deleted past to hide a key in.'); + } + + for (const u of report.unexamined) { + out(`UNEXAMINED ${u.path} [${u.reason}] blob ${u.blob}`); + } + for (const err of report.errors) { + out(`ERROR ${err}`); + } + + if (report.findings.length > 0) { + out(''); + out(`FOUND: ${report.findings.length} credential-shaped blob(s) in reachable history.`); + for (const f of report.findings) { + out(` ${f.path}:${f.line}`); + out(` rule: ${f.rule}`); + out(` blob: ${f.blob}`); + out(` commit: ${f.commit ?? '(not attributable to a single commit)'}`); + } + out(''); + out('The matched value is deliberately not printed. Inspect it yourself:'); + out(' git cat-file blob # in private, not in CI output'); + out(''); + out('If it is live: ROTATE FIRST. This repo is public, so anyone who cloned'); + out('it already has the object - rewriting history does not un-leak it.'); + } + + switch (report.verdict) { + case 'clean': + out(`PASS: no credential-shaped data in ${e.blobsExamined} reachable blob(s).`); + break; + case 'incomplete': + out('COULD NOT COMPLETE: part of this history was not checked. This is not a pass.'); + break; + case 'shallow': + out('SHALLOW: the fetched part looks clean. The unfetched part is unexamined.'); + break; + default: + break; + } +} + +function main() { + const { opts, error } = parseArgs(process.argv.slice(2)); + if (error) { + console.error(`${error}\n`); + console.error(HELP); + process.exit(EXIT.USAGE); + } + if (opts.help) { + console.log(HELP); + process.exit(EXIT.CLEAN); + } + if (opts.printRules) { + console.log(ruleLabels().join('\n')); + process.exit(EXIT.CLEAN); + } + + let result; + try { + result = scanRepo(opts); + } catch (err) { + // Belt and braces: an unexpected throw is could-not-complete, never clean. + const message = String(err && err.message ? err.message : err); + if (opts.json) { + console.log(JSON.stringify({ + tool: 'scan-history-for-secrets', + repo: opts.repo, + verdict: 'incomplete', + exitCode: EXIT.INCOMPLETE, + errors: [message], + }, null, 2)); + } else { + console.error(`COULD NOT COMPLETE: ${message}`); + } + process.exit(EXIT.INCOMPLETE); + } + + if (result.usageError) { + if (opts.json) { + console.log(JSON.stringify({ + tool: 'scan-history-for-secrets', + repo: opts.repo, + verdict: 'usage', + exitCode: EXIT.USAGE, + errors: [result.usageError], + }, null, 2)); + } else { + console.error(`USAGE: ${result.usageError}`); + } + process.exit(EXIT.USAGE); + } + + const { report } = result; + if (opts.json) console.log(JSON.stringify(report, null, 2)); + else printHuman(report); + process.exit(report.exitCode); +} + +const invokedDirectly = + process.argv[1] && path.resolve(process.argv[1]) === fileURLToPath(import.meta.url); +if (invokedDirectly) main(); + +export { EXIT, scanRepo }; diff --git a/src/secret-patterns.mjs b/src/secret-patterns.mjs new file mode 100644 index 0000000..c1576cb --- /dev/null +++ b/src/secret-patterns.mjs @@ -0,0 +1,81 @@ +// SPDX-License-Identifier: AGPL-3.0-only +// +// The ONE list of credential shapes this repo scans for. +// +// It used to live inside scripts/check-stageable-secrets.mjs. It was lifted +// out when scripts/scan-history-for-secrets.mjs was added, because the +// alternative was two lists: one for "what would `git add -A` stage" and one +// for "what is sitting in reachable history". Two lists is not two scanners, +// it is one scanner and one decoy - the day somebody adds a new key format to +// the list they happen to be looking at, the other one keeps passing and keeps +// looking healthy while missing the exact shape that was just added. +// +// That is the same defect the header of check-stageable-secrets.mjs describes: +// the rule that existed stayed correct, so nothing looked broken, and it was +// always the sibling nobody thought to name. So: one list, imported by both, +// with a test that fails if either grows a private copy. +// +// NOTE the deliberate asymmetry: matchSecret() returns WHERE and WHICH RULE, +// never the matched text. Callers that cannot print the value cannot leak it +// into a CI log, a terminal scrollback or a chat message, and "the scanner +// printed the secret" is a real failure mode, not a theoretical one. +// +// (.githooks/pre-commit carries a third, bash-native list. It cannot import +// this module - it is dependency-free grep by design so it works in a clone +// with no node_modules. Unifying those two is a separate change.) + +// Shapes worth stopping for. Deliberately narrow: a scanner that cries wolf +// gets disabled, and a disabled scanner is worse than none. Every pattern +// here is a real credential format we use or plausibly would. +export const SECRET_PATTERNS = [ + [/xfb_[a-f0-9]{32,}/i, 'GroupMind agent key'], + [/antfarm_[A-Za-z0-9]{32,}/, 'GroupMind room key'], + // sk-ant- BEFORE the general sk- rule: the broad one also matches an + // Anthropic key and would mislabel it, and a wrong label sends someone + // rotating the wrong credential. + [/sk-ant-[A-Za-z0-9_-]{20,}/, 'Anthropic API key'], + [/sk-[A-Za-z0-9_-]{20,}/, 'OpenAI-style secret key'], + [/AIza[0-9A-Za-z_-]{35}/, 'Google API key'], + [/gh[pousr]_[A-Za-z0-9]{36,}/, 'GitHub token'], + [/github_pat_[A-Za-z0-9_]{50,}/, 'GitHub fine-grained PAT'], + [/xai-[A-Za-z0-9]{20,}/, 'xAI API key'], + [/-----BEGIN [A-Z ]*PRIVATE KEY-----/, 'private key'], + [/\b(?:api[_-]?key|secret|password|token)\s*[:=]\s*['"]?[A-Za-z0-9_\-]{24,}/i, + 'assigned secret-looking value'], +]; + +// Binaries and media produce noise, not credentials. A blob skipped by this +// rule is NOT examined - both scanners report the count so a reader can tell +// "we looked at everything" from "we looked at everything we could read". +export const SKIP_EXT = + /\.(png|jpe?g|gif|webp|ico|pdf|zip|gz|tgz|jar|aab|apk|keystore|jks|woff2?|ttf|mp[34]|mov|wav)$/i; + +// Above this, a blob is not scanned. It is reported as unexamined rather than +// silently passed: "too big to check" is not "checked and clean". +export const MAX_BYTES = 2 * 1024 * 1024; + +/** + * Find the first credential-shaped thing in `text`. + * + * Returns null, or { label, line, length } - deliberately WITHOUT the matched + * text. Every caller reports a location and a rule name, so no caller is ever + * one console.log away from copying a live key into a public log. + */ +export function matchSecret(text) { + for (const [re, label] of SECRET_PATTERNS) { + const m = text.match(re); + if (m) { + return { + label, + line: text.slice(0, m.index).split('\n').length, + length: m[0].length, + }; + } + } + return null; +} + +/** Rule names in order. Used by --print-rules in both scanners. */ +export function ruleLabels() { + return SECRET_PATTERNS.map(([, label]) => label); +} diff --git a/test/scan-history-for-secrets.test.mjs b/test/scan-history-for-secrets.test.mjs new file mode 100644 index 0000000..f323628 --- /dev/null +++ b/test/scan-history-for-secrets.test.mjs @@ -0,0 +1,328 @@ +// SPDX-License-Identifier: AGPL-3.0-only +// +// Tests for scripts/scan-history-for-secrets.mjs. +// +// A scanner that cannot fail is worse than no scanner: it turns an unchecked +// push into a blessed one. So the tests here are mostly about proving the tool +// is CAPABLE of saying no - it finds a deleted secret, it refuses to call a +// shallow clone clean, it refuses to call an unreadable object clean - and one +// test proves it is capable of saying yes, because a scanner that always fails +// gets switched off within a week. +// +// Every fixture secret is synthetic and assembled at runtime from harmless +// pieces (see synthetic()), so this test file never itself contains a +// credential-shaped string. This repo is public; a "fake" key in a fixture is +// still a key-shaped thing in a public clone, and it would also be found by +// the scanner scanning its own repo, forever. + +import assert from 'node:assert/strict'; +import test from 'node:test'; +import { spawnSync } from 'node:child_process'; +import { mkdtempSync, mkdirSync, rmSync, writeFileSync, readFileSync, realpathSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); +const scanner = path.join(repoRoot, 'scripts', 'scan-history-for-secrets.mjs'); +const stageableScanner = path.join(repoRoot, 'scripts', 'check-stageable-secrets.mjs'); + +const EXIT = { CLEAN: 0, FOUND: 1, USAGE: 2, INCOMPLETE: 3, SHALLOW: 4 }; + +// Global/system git config is neutralised so a developer's core.hooksPath, +// commit.gpgsign or init.defaultBranch cannot reach into these fixtures. +const GIT_ENV = { + ...process.env, + GIT_CONFIG_GLOBAL: '/dev/null', + GIT_CONFIG_SYSTEM: '/dev/null', + GIT_AUTHOR_NAME: 'iak test', + GIT_AUTHOR_EMAIL: 'test@example.invalid', + GIT_COMMITTER_NAME: 'iak test', + GIT_COMMITTER_EMAIL: 'test@example.invalid', + GIT_TERMINAL_PROMPT: '0', +}; + +const tempDirs = []; +test.after(() => { + for (const dir of tempDirs) rmSync(dir, { recursive: true, force: true }); +}); + +function tempDir(prefix) { + // realpathSync: macOS tmpdir() is behind /var -> /private/var, and git + // reports its own resolved toplevel. + const dir = realpathSync(mkdtempSync(path.join(tmpdir(), prefix))); + tempDirs.push(dir); + return dir; +} + +function git(cwd, args) { + const r = spawnSync('git', args, { cwd, encoding: 'utf8', env: GIT_ENV }); + if (r.status !== 0) throw new Error(`git ${args.join(' ')} -> ${r.status}: ${r.stderr}`); + return r.stdout; +} + +function commitAll(dir, message) { + git(dir, ['add', '-A']); + git(dir, ['commit', '--no-verify', '-q', '-m', message]); +} + +function newRepo(prefix) { + const dir = tempDir(prefix); + git(dir, ['init', '-q', '-b', 'main']); + return dir; +} + +/** + * A credential-SHAPED string that is obviously not a credential, assembled at + * runtime so the literal never appears in this file or in the repo. + */ +function synthetic(marker) { + return ['sk', 'TESTONLY', marker, '0'.repeat(16)].join('-'); +} + +function runScanner(args) { + return spawnSync('node', [scanner, ...args], { encoding: 'utf8', env: GIT_ENV }); +} + +// --------------------------------------------------------------------------- +// The test this tool exists for. +// --------------------------------------------------------------------------- + +test('finds a secret that was committed and then DELETED in a later commit', () => { + const dir = newRepo('iak-hist-deleted-'); + const secret = synthetic('DELETED'); + + writeFileSync(path.join(dir, 'README.md'), '# fixture\n'); + commitAll(dir, 'initial'); + + writeFileSync(path.join(dir, 'leaked-config.json'), `{\n "key": "${secret}"\n}\n`); + commitAll(dir, 'oops'); + + rmSync(path.join(dir, 'leaked-config.json')); + commitAll(dir, 'remove the config again'); + + // Sanity: the working tree is clean now, which is exactly why the + // working-tree scanner cannot see this and this one must. + assert.equal(git(dir, ['status', '--porcelain']).trim(), ''); + assert.ok(!readFileSync(path.join(dir, 'README.md'), 'utf8').includes(secret)); + + const r = runScanner([dir, '--json']); + assert.equal(r.status, EXIT.FOUND, `expected FOUND, got ${r.status}: ${r.stderr}`); + + const report = JSON.parse(r.stdout); + assert.equal(report.verdict, 'found'); + assert.equal(report.findings.length, 1); + assert.equal(report.findings[0].path, 'leaked-config.json'); + assert.equal(report.findings[0].rule, 'OpenAI-style secret key'); + assert.match(report.findings[0].blob, /^[0-9a-f]{40}$/); + // The commit is attributed, so a human can go and look in private. + assert.match(report.findings[0].commit, /^[0-9a-f]{40}$/); + assert.equal(git(dir, ['log', '--format=%s', '-1', report.findings[0].commit]).trim(), 'oops'); + assert.equal(report.examined.commits, 3); +}); + +test('no matched value appears in stdout, stderr or the JSON', () => { + const dir = newRepo('iak-hist-noleak-'); + const secret = synthetic('NOPRINT'); + writeFileSync(path.join(dir, 'leaked-config.json'), `api_key = "${secret}"\n`); + commitAll(dir, 'oops'); + rmSync(path.join(dir, 'leaked-config.json')); + commitAll(dir, 'delete it'); + + for (const args of [[dir], [dir, '--json']]) { + const r = runScanner(args); + assert.equal(r.status, EXIT.FOUND, `expected FOUND for ${args.join(' ')}`); + const blob = `${r.stdout}\n${r.stderr}`; + // The whole value, obviously... + assert.ok(!blob.includes(secret), `value leaked into output of ${args.join(' ')}`); + // ...and no prefix of it either. A "redacted" first-six-characters hint is + // still six characters of a live key in a CI transcript. + for (let n = 8; n <= secret.length; n++) { + assert.ok(!blob.includes(secret.slice(0, n)), `${n}-char prefix leaked`); + } + assert.ok(!blob.includes('TESTONLY'), 'a distinctive fragment of the value leaked'); + // ...but it did tell us where to look. + assert.ok(blob.includes('leaked-config.json'), 'the finding must still be actionable'); + } +}); + +// --------------------------------------------------------------------------- +// It must also be able to say yes. +// --------------------------------------------------------------------------- + +test('a genuinely clean repo exits 0', () => { + const dir = newRepo('iak-hist-clean-'); + writeFileSync(path.join(dir, 'README.md'), '# nothing to see\n'); + commitAll(dir, 'initial'); + writeFileSync(path.join(dir, 'app.mjs'), 'export const answer = 42;\n'); + commitAll(dir, 'code'); + + const r = runScanner([dir, '--json']); + assert.equal(r.status, EXIT.CLEAN, `expected CLEAN, got ${r.status}: ${r.stderr}`); + const report = JSON.parse(r.stdout); + assert.equal(report.verdict, 'clean'); + assert.equal(report.findings.length, 0); + assert.equal(report.shallow, false); + assert.equal(report.examined.commits, 2); + // README.md (unchanged, so one blob) + app.mjs. + assert.equal(report.examined.blobsExamined, 2, 'must say how many blobs it actually read'); +}); + +// --------------------------------------------------------------------------- +// Fail-closed: not-checked is never reported as clean. +// --------------------------------------------------------------------------- + +test('a shallow clone is reported shallow and does NOT exit clean', () => { + const origin = newRepo('iak-hist-origin-'); + const secret = synthetic('SHALLOW'); + writeFileSync(path.join(origin, 'README.md'), '# fixture\n'); + commitAll(origin, 'initial'); + writeFileSync(path.join(origin, 'leaked-config.json'), `key=${secret}\n`); + commitAll(origin, 'oops'); + rmSync(path.join(origin, 'leaked-config.json')); + commitAll(origin, 'delete it'); + writeFileSync(path.join(origin, 'app.mjs'), 'export const ok = true;\n'); + commitAll(origin, 'more work'); + + const parent = tempDir('iak-hist-shallow-'); + const clone = path.join(parent, 'clone'); + git(parent, ['clone', '-q', '--depth', '1', `file://${origin}`, clone]); + + const r = runScanner([clone, '--json']); + // The fetched tip really is clean. That is precisely the trap: the leak is + // in the commits --depth cut off, and "clean" here would be a lie. + assert.notEqual(r.status, EXIT.CLEAN, 'a shallow clone must never exit clean'); + assert.equal(r.status, EXIT.SHALLOW); + const report = JSON.parse(r.stdout); + assert.equal(report.shallow, true); + assert.equal(report.verdict, 'shallow'); + assert.equal(report.findings.length, 0, 'the truncated history really did hide it'); + + const human = runScanner([clone]); + assert.equal(human.status, EXIT.SHALLOW); + assert.match(human.stderr, /SHALLOW CLONE/); + assert.match(human.stderr, /unshallow/); + + // And the same history, unshallowed, is caught. + git(clone, ['fetch', '-q', '--unshallow']); + const after = runScanner([clone, '--json']); + assert.equal(after.status, EXIT.FOUND, 'the full history holds the deleted secret'); + assert.equal(JSON.parse(after.stdout).findings[0].path, 'leaked-config.json'); +}); + +test('an undecodable object is could-not-complete, never clean', () => { + const dir = newRepo('iak-hist-binary-'); + writeFileSync(path.join(dir, 'README.md'), '# fixture\n'); + // NUL bytes: cannot be read as text, so it cannot be claimed as checked. + writeFileSync(path.join(dir, 'payload.dat'), Buffer.from([0x68, 0x00, 0x69, 0x00])); + // Not valid UTF-8 either, and no NULs: the lossy-decode path. + writeFileSync(path.join(dir, 'latin.dat'), Buffer.from([0xff, 0xfe, 0x41, 0x42])); + commitAll(dir, 'binary things'); + + const r = runScanner([dir, '--json']); + assert.equal(r.status, EXIT.INCOMPLETE, `expected INCOMPLETE, got ${r.status}: ${r.stderr}`); + const report = JSON.parse(r.stdout); + assert.equal(report.verdict, 'incomplete'); + assert.equal(report.examined.blobsUnexamined, 2); + const reasons = Object.fromEntries(report.unexamined.map((u) => [u.path, u.reason])); + assert.equal(reasons['payload.dat'], 'nul-bytes'); + assert.equal(reasons['latin.dat'], 'invalid-utf8'); + + const human = runScanner([dir]); + assert.equal(human.status, EXIT.INCOMPLETE); + assert.match(human.stderr, /COULD NOT COMPLETE/); + assert.ok(!/^PASS/m.test(human.stdout), '"could not check" must never render as a pass'); +}); + +test('a blob over the size cap is unexamined, not passed', () => { + const dir = newRepo('iak-hist-big-'); + writeFileSync(path.join(dir, 'big.txt'), 'x'.repeat(4096)); + commitAll(dir, 'big file'); + + const r = runScanner([dir, '--max-bytes=1024', '--json']); + assert.equal(r.status, EXIT.INCOMPLETE); + const report = JSON.parse(r.stdout); + assert.equal(report.unexamined.find((u) => u.path === 'big.txt').reason, 'over-size-cap'); +}); + +test('an exhausted time budget is could-not-complete, never clean', () => { + const dir = newRepo('iak-hist-timeout-'); + writeFileSync(path.join(dir, 'README.md'), '# fixture\n'); + commitAll(dir, 'initial'); + + // A budget this small is guaranteed to be gone before the first git call. + const r = spawnSync('node', [scanner, dir, '--max-seconds=0.001', '--json'], + { encoding: 'utf8', env: GIT_ENV }); + assert.equal(r.status, EXIT.INCOMPLETE, `expected INCOMPLETE, got ${r.status}: ${r.stderr}`); + assert.notEqual(JSON.parse(r.stdout).verdict, 'clean'); +}); + +test('a findable secret still wins over a shallow or incomplete verdict', () => { + const dir = newRepo('iak-hist-precedence-'); + const secret = synthetic('PRECEDENCE'); + writeFileSync(path.join(dir, 'leaked-config.json'), `key=${secret}\n`); + writeFileSync(path.join(dir, 'payload.dat'), Buffer.from([0x00, 0x01])); + commitAll(dir, 'both at once'); + + const r = runScanner([dir, '--json']); + assert.equal(r.status, EXIT.FOUND); + const report = JSON.parse(r.stdout); + assert.equal(report.verdict, 'found'); + assert.equal(report.unexamined.length, 1, 'the unreadable blob is still reported'); +}); + +test('a one-commit snapshot repo is told it proved almost nothing', () => { + const dir = newRepo('iak-hist-snapshot-'); + writeFileSync(path.join(dir, 'README.md'), '# snapshot\n'); + commitAll(dir, 'snapshot'); + + const r = runScanner([dir]); + assert.equal(r.status, EXIT.CLEAN); + assert.match(r.stdout, /1 commits/); + assert.match(r.stdout, /DEPTH:/); +}); + +test('a path that is not a git repository is a usage error, not a pass', () => { + const dir = tempDir('iak-hist-notrepo-'); + mkdirSync(path.join(dir, 'sub')); + const r = runScanner([path.join(dir, 'sub'), '--json']); + assert.equal(r.status, EXIT.USAGE); + assert.equal(JSON.parse(r.stdout).verdict, 'usage'); + + const bad = runScanner(['--nonsense-flag']); + assert.equal(bad.status, EXIT.USAGE); +}); + +test('--help documents every exit code', () => { + const r = runScanner(['--help']); + assert.equal(r.status, 0); + for (const line of [/^\s*0\s+clean/m, /^\s*1\s+found/m, /^\s*2\s+usage/m, + /^\s*3\s+incomplete/m, /^\s*4\s+shallow/m]) { + assert.match(r.stdout, line); + } +}); + +// --------------------------------------------------------------------------- +// The two scanners must not drift apart. +// --------------------------------------------------------------------------- + +test('both scanners use the same pattern list (fails if the lists diverge)', () => { + const history = spawnSync('node', [scanner, '--print-rules'], { encoding: 'utf8', env: GIT_ENV }); + const staged = spawnSync('node', [stageableScanner, '--print-rules'], + { cwd: repoRoot, encoding: 'utf8', env: GIT_ENV }); + assert.equal(history.status, 0, history.stderr); + assert.equal(staged.status, 0, staged.stderr); + assert.ok(history.stdout.trim().length > 0, 'the rule list must not be empty'); + assert.equal(history.stdout, staged.stdout, + 'the history scanner and the stageable-file scanner disagree about what a secret looks like'); + + // A runtime comparison alone would still pass if someone copy-pasted the + // list into one of the scripts, so also assert neither owns a private copy. + for (const file of [scanner, stageableScanner]) { + const src = readFileSync(file, 'utf8'); + assert.match(src, /from '\.\.\/src\/secret-patterns\.mjs'/, + `${path.basename(file)} must import the shared patterns`); + assert.ok(!/^\s*(?:export\s+)?const\s+\w*PATTERNS\s*=\s*\[/m.test(src), + `${path.basename(file)} declares its own pattern list - that is the drift this test exists to stop`); + } +}); From d091acd838b9ca175b6bbc604eba80090458663d Mon Sep 17 00:00:00 2001 From: Petrus Pennanen Date: Sun, 20 Sep 2026 10:04:58 +0200 Subject: [PATCH 2/7] Decide binary-ness by decoding, not by the presence of a NUL The first version refused any blob containing a NUL byte and called the refusal could-not-complete. That conflates two different questions: "contains a byte I associate with binary" and "cannot be decoded as text". Only the second justifies refusing to scan. bin/iak-pending.mjs in this repo's history is the counter-example: 26,507 bytes, exactly one NUL at offset 12,684, used as a deliberate field separator between a host and an id so neither half can forge a collision. It is valid UTF-8, `node --check` passes, it is ordinary JavaScript. The scanner skipped all 26 kB of it, so a key sitting after that byte would have been missed - and the miss would have been reported as a scanning error rather than a finding. Fail-closed is about what could not be read, not about bytes that look alarming. decodeUtf8() in src/secret-patterns.mjs now answers exactly one question with TextDecoder in fatal mode: do these bytes decode. Anything that decodes is scanned. Invalid sequences, over-cap blobs, unreadable objects and timeouts stay could-not-complete. check-stageable-secrets.mjs had the same conflation (readFileSync utf8, then look for a NUL, which could not tell a real binary from text either). It now reads bytes and decodes strictly through the same helper. Tests: a file with a stray NUL AND a synthetic secret after it must be FOUND, not reported as could-not-complete; proven to fail against the old heuristic. The undecodable fixtures are now genuinely invalid UTF-8 rather than NUL-bearing, so the fix cannot swing the other way and swallow real binaries silently. Against this repo: 1116 of 1116 reachable blobs examined, 0 unexamined, where it was 1115 of 1116 before. Co-Authored-By: Claude Opus 5 --- scripts/check-stageable-secrets.mjs | 13 ++++--- scripts/scan-history-for-secrets.mjs | 44 +++++++++++++++-------- src/secret-patterns.mjs | 24 +++++++++++++ test/scan-history-for-secrets.test.mjs | 48 ++++++++++++++++++++++---- 4 files changed, 105 insertions(+), 24 deletions(-) diff --git a/scripts/check-stageable-secrets.mjs b/scripts/check-stageable-secrets.mjs index 85084e7..ab6153f 100755 --- a/scripts/check-stageable-secrets.mjs +++ b/scripts/check-stageable-secrets.mjs @@ -38,7 +38,7 @@ import { execFileSync } from 'node:child_process'; import { readFileSync, statSync } from 'node:fs'; -import { MAX_BYTES, SKIP_EXT, matchSecret, ruleLabels } from '../src/secret-patterns.mjs'; +import { MAX_BYTES, SKIP_EXT, decodeUtf8, matchSecret, ruleLabels } from '../src/secret-patterns.mjs'; // The pattern list, the binary-extension skip list and the size cap now live // in src/secret-patterns.mjs, shared with scripts/scan-history-for-secrets.mjs. @@ -76,14 +76,19 @@ function stageableFiles() { function scan(path) { if (SKIP_EXT.test(path)) return null; - let text; + let bytes; try { if (statSync(path).size > MAX_BYTES) return null; - text = readFileSync(path, 'utf8'); + bytes = readFileSync(path); } catch { return null; // unreadable, gone, or a directory: not our problem } - if (text.includes('\0')) return null; // binary + // Read as bytes and decode strictly, rather than the old "readFileSync utf8 + // then look for a NUL". A NUL is not a proof of binary - bin/iak-pending.mjs + // uses one as a field separator inside 26 kB of valid JavaScript - and the + // lossy utf8 read could not tell a real binary from text anyway. + const text = decodeUtf8(bytes); + if (text === null) return null; // genuinely not text // Report WHERE and WHICH RULE, never the value itself. This output ends up // in CI logs and terminal scrollback, and a scanner that prints the secret it // found has simply moved the leak. diff --git a/scripts/scan-history-for-secrets.mjs b/scripts/scan-history-for-secrets.mjs index 796c06a..c60f79e 100644 --- a/scripts/scan-history-for-secrets.mjs +++ b/scripts/scan-history-for-secrets.mjs @@ -36,7 +36,14 @@ // old ones - which is precisely where a deleted-but-reachable key lives. // Shallow exits 4 and says so at the top of the report. // -// 4. It always says how much it looked at. A freshly created snapshot repo +// 4. It scans anything that DECODES. "Contains a NUL byte" is not the same +// question as "is not text", and an early version of this file failed +// that distinction: it refused 26 kB of valid UTF-8 JavaScript over one +// NUL used as a field separator, and called the refusal an error instead +// of scanning the other 26 kB. Fail-closed is about what you could not +// read, not about bytes that merely look alarming. +// +// 5. It always says how much it looked at. A freshly created snapshot repo // has one commit; "history clean" after examining one commit is not // reassurance, it is the false confidence this tool exists to prevent. // @@ -50,6 +57,7 @@ import { fileURLToPath } from 'node:url'; import { MAX_BYTES, SKIP_EXT, + decodeUtf8, matchSecret, ruleLabels, } from '../src/secret-patterns.mjs'; @@ -90,7 +98,8 @@ WHAT IS SCANNED anyone who clones the repo. Blobs whose path has a known binary media extension are skipped by policy and - counted separately as NOT examined. Everything else is decoded as UTF-8 and + counted separately as NOT examined. Everything else is decoded as UTF-8 (in + fatal mode - a stray NUL in otherwise valid text is scanned, not refused) and matched against the shared pattern list in src/secret-patterns.mjs - the same list scripts/check-stageable-secrets.mjs uses. @@ -258,14 +267,21 @@ function readBatch(git, shas, onBlob) { } } -/** Is `body` something we can honestly claim to have read as text? */ -function undecodableReason(body) { - if (body.includes(0)) return 'nul-bytes'; - // A lossy decode means we scanned something other than what is stored, so - // "no match" would be a claim about the wrong bytes. - const text = body.toString('utf8'); - if (Buffer.byteLength(text, 'utf8') !== body.length) return 'invalid-utf8'; - return null; +/** + * Decode a blob, or say why it cannot be scanned. + * + * Returns { text } or { reason }. The ONLY disqualifier is bytes that do not + * decode as UTF-8: a lossy decode means we scanned something other than what + * is stored, so "no match" would be a claim about the wrong bytes. + * + * A stray NUL is deliberately NOT a disqualifier, see decodeUtf8's note. A + * file can hold a NUL as a field separator and still be 26 kB of readable + * source that could carry a key - refusing it skips the scan AND mislabels the + * skip as an error, which is the worst of both answers. + */ +function decodeBlob(body) { + const text = decodeUtf8(body); + return text === null ? { reason: 'invalid-utf8' } : { text }; } /** @@ -374,14 +390,14 @@ function scanRepo(opts) { readBatch(git, chunk.map((b) => b.sha), (sha, body) => { const blob = bySha.get(sha); if (!blob) return; - const bad = undecodableReason(body); - if (bad) { - report.unexamined.push({ path: blob.path, blob: sha, reason: bad }); + const { text, reason } = decodeBlob(body); + if (reason) { + report.unexamined.push({ path: blob.path, blob: sha, reason }); return; } report.examined.blobsExamined += 1; report.examined.bytesExamined += body.length; - const hit = matchSecret(body.toString('utf8')); + const hit = matchSecret(text); if (hit) { // label + line + length only. The value stays in the repo, which is // the one place it is already. diff --git a/src/secret-patterns.mjs b/src/secret-patterns.mjs index c1576cb..25d7ac7 100644 --- a/src/secret-patterns.mjs +++ b/src/secret-patterns.mjs @@ -75,6 +75,30 @@ export function matchSecret(text) { return null; } +/** + * Decode `buf` as text, or return null if it genuinely is not UTF-8. + * + * NOT a NUL check. Those are two different questions and conflating them cost + * us a real scan: bin/iak-pending.mjs carries ONE NUL byte, 12,684 bytes in, + * as a deliberate field separator between a host and an id (NUL cannot occur + * in either, so neither component can forge a collision). The file is valid + * UTF-8, `node --check` passes, and it is 26 kB of perfectly readable + * JavaScript. The old "contains a NUL, therefore binary" rule refused to look + * at any of it and reported could-not-complete - so a credential sitting after + * that byte would have been missed, and the miss would have been dressed up as + * a scanning error rather than a finding. + * + * The only thing that justifies refusing to scan is bytes that do not decode. + * TextDecoder in fatal mode answers exactly that question and nothing else. + */ +export function decodeUtf8(buf) { + try { + return new TextDecoder('utf-8', { fatal: true }).decode(buf); + } catch { + return null; + } +} + /** Rule names in order. Used by --print-rules in both scanners. */ export function ruleLabels() { return SECRET_PATTERNS.map(([, label]) => label); diff --git a/test/scan-history-for-secrets.test.mjs b/test/scan-history-for-secrets.test.mjs index f323628..4f17fa6 100644 --- a/test/scan-history-for-secrets.test.mjs +++ b/test/scan-history-for-secrets.test.mjs @@ -210,13 +210,14 @@ test('a shallow clone is reported shallow and does NOT exit clean', () => { assert.equal(JSON.parse(after.stdout).findings[0].path, 'leaked-config.json'); }); -test('an undecodable object is could-not-complete, never clean', () => { +test('a genuinely undecodable object is could-not-complete, never clean', () => { const dir = newRepo('iak-hist-binary-'); writeFileSync(path.join(dir, 'README.md'), '# fixture\n'); - // NUL bytes: cannot be read as text, so it cannot be claimed as checked. - writeFileSync(path.join(dir, 'payload.dat'), Buffer.from([0x68, 0x00, 0x69, 0x00])); - // Not valid UTF-8 either, and no NULs: the lossy-decode path. + // Invalid UTF-8 byte sequences, NOT merely a NUL: 0xff/0xfe cannot start a + // UTF-8 sequence, and 0xc3 here is a truncated two-byte lead. These really + // cannot be read as text, so they cannot be claimed as checked. writeFileSync(path.join(dir, 'latin.dat'), Buffer.from([0xff, 0xfe, 0x41, 0x42])); + writeFileSync(path.join(dir, 'truncated.dat'), Buffer.from([0x41, 0xc3])); commitAll(dir, 'binary things'); const r = runScanner([dir, '--json']); @@ -225,8 +226,8 @@ test('an undecodable object is could-not-complete, never clean', () => { assert.equal(report.verdict, 'incomplete'); assert.equal(report.examined.blobsUnexamined, 2); const reasons = Object.fromEntries(report.unexamined.map((u) => [u.path, u.reason])); - assert.equal(reasons['payload.dat'], 'nul-bytes'); assert.equal(reasons['latin.dat'], 'invalid-utf8'); + assert.equal(reasons['truncated.dat'], 'invalid-utf8'); const human = runScanner([dir]); assert.equal(human.status, EXIT.INCOMPLETE); @@ -234,6 +235,41 @@ test('an undecodable object is could-not-complete, never clean', () => { assert.ok(!/^PASS/m.test(human.stdout), '"could not check" must never render as a pass'); }); +test('a stray NUL in valid UTF-8 is SCANNED, not refused as binary', () => { + // Regression for a real miss. bin/iak-pending.mjs in this repo's history + // carries exactly one NUL, 12,684 bytes in, as a deliberate field separator + // between a host and an id - NUL cannot occur in either, so neither half can + // forge a collision. The file is valid UTF-8, `node --check` passes, and it + // is 26 kB of readable JavaScript. The first version of this scanner refused + // to look at any of it and called that could-not-complete, so a key sitting + // after that byte would have been missed AND the miss would have been + // reported as a scanning error rather than a finding. + const dir = newRepo('iak-hist-nul-'); + const secret = synthetic('AFTERNUL'); + const body = Buffer.concat([ + Buffer.from('export function itemKey(item) { return `${item.host}'), + Buffer.from([0x00]), + Buffer.from(`\${item.id}\`; }\n\n// leaked below the separator\nconst apiKey = "${secret}";\n`), + ]); + writeFileSync(path.join(dir, 'pending.mjs'), body); + commitAll(dir, 'a NUL separator and, later in the same file, a key'); + + const r = runScanner([dir, '--json']); + assert.equal(r.status, EXIT.FOUND, + `a file that decodes must be scanned, not refused; got ${r.status}: ${r.stderr}`); + const report = JSON.parse(r.stdout); + assert.equal(report.verdict, 'found'); + assert.equal(report.unexamined.length, 0, 'a decodable file must not count as unexamined'); + assert.equal(report.findings.length, 1); + assert.equal(report.findings[0].path, 'pending.mjs'); + // The match is AFTER the NUL, which is the whole point. + assert.ok(body.indexOf(Buffer.from(secret)) > body.indexOf(0), + 'fixture must place the secret after the NUL'); + + // And the value still never appears in the output. + assert.ok(!`${r.stdout}${r.stderr}`.includes('TESTONLY')); +}); + test('a blob over the size cap is unexamined, not passed', () => { const dir = newRepo('iak-hist-big-'); writeFileSync(path.join(dir, 'big.txt'), 'x'.repeat(4096)); @@ -261,7 +297,7 @@ test('a findable secret still wins over a shallow or incomplete verdict', () => const dir = newRepo('iak-hist-precedence-'); const secret = synthetic('PRECEDENCE'); writeFileSync(path.join(dir, 'leaked-config.json'), `key=${secret}\n`); - writeFileSync(path.join(dir, 'payload.dat'), Buffer.from([0x00, 0x01])); + writeFileSync(path.join(dir, 'payload.dat'), Buffer.from([0xff, 0xfe, 0x00, 0x01])); commitAll(dir, 'both at once'); const r = runScanner([dir, '--json']); From cabf6affd875f259a360ad4a0b12c151fd47878b Mon Sep 17 00:00:00 2001 From: Petrus Pennanen Date: Sun, 20 Sep 2026 10:30:28 +0200 Subject: [PATCH 3/7] Entry points must not be silent through a symlink The scanner produced no output and exited 0 when invoked through a symlinked path. For a security tool exit 0 means clean, so it blessed a repo it had never looked at: node /tmp/scan-wt/scripts/scan-history-for-secrets.mjs EXIT=0, 0 bytes node /private/tmp/scan-wt/scripts/scan-history-for-secrets.mjs EXIT=1, 3622 bytes, 18 findings Node always realpath-resolves import.meta.url and never resolves process.argv[1], so an invoked-directly guard comparing the two is false through any symlink. macOS /tmp IS a symlink to /private/tmp, so every scratch dir, every worktree under /tmp, every ~/bin symlink and any CI checkout in a symlinked workspace hits it. Even --help printed nothing. The scanner now has no guard at all. It is an entry point, nothing imports it, and the guard bought nothing. The engine moved to src/history-scan.mjs, which is pure: no argv, no printing, no process.exit, importing it starts nothing. scripts/scan-history-for-secrets.mjs is the command line and calls main() unconditionally. A tool that cannot run can no longer report success because there is no path on which it does not run. For modules that genuinely need the distinction, src/common/entrypoint.mjs now holds the one implementation, realpathing both sides. Swept every entry point in bin/, scripts/ and src/: 7 sites compared argv[1] against import.meta.url, 6 of them broken, in 3 different shapes. All 7 now call isMainModule(). scripts/local-agent.mjs file://${argv[1]} BROKEN scripts/local-relay.mjs file://${argv[1]} BROKEN src/mcp-server.mjs file://${argv[1]} BROKEN scripts/hosted-canary.mjs pathToFileURL(argv[1]) BROKEN scripts/poller-health-alert.mjs pathToFileURL(argv[1]) BROKEN scripts/scan-history-for-secrets.mjs resolve(argv[1]) BROKEN, now unguarded scripts/team-watchdog.mjs realpathSync(argv[1]) correct, deduplicated Also adds an iak-scan-history bin entry so the intended invocation is unambiguous. Tests: the scanner and the pre-commit gate must produce the same exit code and the same report through a real symlink created by the test, for a symlinked repo root and for a ~/bin-style symlink to the script; a sweep test that fails if any entry point hand-rolls the comparison again; and isMainModule itself, which must say MAIN through a symlink and NOT-MAIN when imported. check-stageable-secrets.mjs was checked for the same shape: it has no guard, runs at import, and was never affected. The test locks that in, since a no-op there lets a commit carrying a credential pass the hook. Co-Authored-By: Claude Opus 5 --- package.json | 3 +- scripts/hosted-canary.mjs | 4 +- scripts/local-agent.mjs | 4 +- scripts/local-relay.mjs | 3 +- scripts/poller-health-alert.mjs | 4 +- scripts/scan-history-for-secrets.mjs | 368 ++----------------------- scripts/team-watchdog.mjs | 10 +- src/common/entrypoint.mjs | 42 +++ src/history-scan.mjs | 357 ++++++++++++++++++++++++ src/mcp-server.mjs | 3 +- test/scan-history-for-secrets.test.mjs | 150 +++++++++- 11 files changed, 584 insertions(+), 364 deletions(-) mode change 100644 => 100755 scripts/scan-history-for-secrets.mjs create mode 100644 src/common/entrypoint.mjs create mode 100644 src/history-scan.mjs diff --git a/package.json b/package.json index 58a35a1..de944bc 100644 --- a/package.json +++ b/package.json @@ -6,7 +6,8 @@ "bin": { "ide-agent-kit": "./bin/cli.mjs", "ide-agent-kit-mcp": "./bin/iak-mcp.mjs", - "iak-pending": "./bin/iak-pending.mjs" + "iak-pending": "./bin/iak-pending.mjs", + "iak-scan-history": "./scripts/scan-history-for-secrets.mjs" }, "scripts": { "test": "node --test test/*.test.mjs packages/user-intent-kit/test/*.test.js", diff --git a/scripts/hosted-canary.mjs b/scripts/hosted-canary.mjs index c558ca0..8740c07 100644 --- a/scripts/hosted-canary.mjs +++ b/scripts/hosted-canary.mjs @@ -33,8 +33,8 @@ // Exit 0 healthy, 1 unhealthy, 2 misconfigured. Run it from cron or launchd. import { existsSync, writeFileSync, unlinkSync, readFileSync } from 'node:fs'; -import { pathToFileURL } from 'node:url'; import { randomUUID } from 'node:crypto'; +import { isMainModule } from '../src/common/entrypoint.mjs'; /** The message we post. A nonce, so reading it back proves OUR write landed * rather than finding someone else's old row. */ @@ -162,6 +162,6 @@ async function main() { process.exit(result.ok ? 0 : 1); } -if (import.meta.url === pathToFileURL(process.argv[1] || '').href) { +if (isMainModule(import.meta.url)) { main(); } diff --git a/scripts/local-agent.mjs b/scripts/local-agent.mjs index e41bd7d..9e3cd30 100644 --- a/scripts/local-agent.mjs +++ b/scripts/local-agent.mjs @@ -137,7 +137,9 @@ export async function runAgent(cfg) { } // CLI: config from IAK dogfood config + env overrides. -if (import.meta.url === `file://${process.argv[1]}`) { +import { isMainModule } from '../src/common/entrypoint.mjs'; + +if (isMainModule(import.meta.url)) { const { readFileSync } = await import('node:fs'); const cfgPath = process.env.IAK_CONFIG || '/Users/petrus/ide-agent-kit/config/dogfood.json'; let base = {}; diff --git a/scripts/local-relay.mjs b/scripts/local-relay.mjs index 307338d..14821cd 100644 --- a/scripts/local-relay.mjs +++ b/scripts/local-relay.mjs @@ -38,6 +38,7 @@ import { appendFileSync, mkdirSync, readFileSync, existsSync } from 'node:fs'; import { randomUUID, timingSafeEqual, createHash } from 'node:crypto'; import { homedir } from 'node:os'; import { dirname, join } from 'node:path'; +import { isMainModule } from '../src/common/entrypoint.mjs'; const DEFAULT_PORT = 18790; const DEFAULT_STORE = join(homedir(), '.iak', 'local-relay.jsonl'); @@ -209,7 +210,7 @@ export function startRelay(opts = {}) { } // CLI entry -if (import.meta.url === `file://${process.argv[1]}`) { +if (isMainModule(import.meta.url)) { const arg = (name) => { const i = process.argv.indexOf(name); return i !== -1 ? process.argv[i + 1] : undefined; diff --git a/scripts/poller-health-alert.mjs b/scripts/poller-health-alert.mjs index 0fb562b..9ff9722 100644 --- a/scripts/poller-health-alert.mjs +++ b/scripts/poller-health-alert.mjs @@ -18,7 +18,7 @@ // supervisor loop - codex review of PR #87). import { existsSync, statSync, readFileSync, writeFileSync, unlinkSync } from 'node:fs'; import { execFileSync } from 'node:child_process'; -import { pathToFileURL } from 'node:url'; +import { isMainModule } from '../src/common/entrypoint.mjs'; export function heartbeatAge(path, now = Date.now()) { if (!existsSync(path)) return Infinity; @@ -136,6 +136,6 @@ async function main() { } } -if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { +if (isMainModule(import.meta.url)) { await main(); } diff --git a/scripts/scan-history-for-secrets.mjs b/scripts/scan-history-for-secrets.mjs old mode 100644 new mode 100755 index c60f79e..01ada0e --- a/scripts/scan-history-for-secrets.mjs +++ b/scripts/scan-history-for-secrets.mjs @@ -1,77 +1,31 @@ #!/usr/bin/env node // SPDX-License-Identifier: AGPL-3.0-only // -// Scan REACHABLE GIT HISTORY for credentials - not the working tree. +// Command line for the reachable-history secret scanner. The engine, and the +// long explanation of what this exists for, live in src/history-scan.mjs. // -// THE GAP THIS CLOSES. scripts/check-stageable-secrets.mjs reads files from -// disk and answers one question: "would `git add -A` stage a credential right -// now". That is a useful question and it is not this one. A key that was -// committed on Tuesday and deleted on Wednesday is gone from the working tree, -// gone from `git status`, gone from the file listing - and still sits in the -// pack file, one `git log -p` away from anyone who clones the repo. To the -// disk-reading scanner that repo looks spotless. It is not: it is leaking. +// THIS FILE IS AN ENTRY POINT AND NOTHING ELSE. main() is called at the bottom +// with no guard around it, deliberately. // -// This repo is PUBLIC, and a snapshot pushed into it is world-readable the -// instant it lands, with no private staging period in which to notice. +// The first version wrapped that call in the usual // -// WHAT IT DOES. Every blob reachable from every ref (`git rev-list -// --objects --all`), read through `git cat-file --batch`, matched against the -// SHARED pattern list in src/secret-patterns.mjs - shared so that the two -// scanners cannot drift apart, which is the house defect this repo already has -// a written-up history of. +// if (path.resolve(process.argv[1]) === fileURLToPath(import.meta.url)) main(); // -// WHAT IT REFUSES TO DO. +// which is broken through a symlink, because node always realpath-resolves +// import.meta.url and never resolves argv[1]. On macOS /tmp IS a symlink to +// /private/tmp, so running the scanner from any scratch dir, any worktree under +// /tmp, or a ~/bin symlink produced NO output and exit 0. For a security tool, +// exit 0 means clean. It blessed a repo it had not looked at. // -// 1. It never prints a matched value, not partially, not redacted with a -// prefix. Path + commit + rule name is enough to act on. A prefix is not -// "safe", it is the first six characters of a live key in a CI log. -// -// 2. It never renders "I could not check" as "clean". An unreadable object, -// a blob it cannot decode, a blob over the size cap, a timeout: all of -// those exit 3 (could-not-complete), never 0. This is the single most -// repeated bug in this codebase and it gets its own exit code. -// -// 3. It never calls a shallow clone clean. A `--depth` clone's history is -// not absent, it is UNEXAMINED, and the blobs that were cut off are the -// old ones - which is precisely where a deleted-but-reachable key lives. -// Shallow exits 4 and says so at the top of the report. -// -// 4. It scans anything that DECODES. "Contains a NUL byte" is not the same -// question as "is not text", and an early version of this file failed -// that distinction: it refused 26 kB of valid UTF-8 JavaScript over one -// NUL used as a field separator, and called the refusal an error instead -// of scanning the other 26 kB. Fail-closed is about what you could not -// read, not about bytes that merely look alarming. -// -// 5. It always says how much it looked at. A freshly created snapshot repo -// has one commit; "history clean" after examining one commit is not -// reassurance, it is the false confidence this tool exists to prevent. +// A guard here bought nothing - nothing imports this file - so the fix is not a +// better comparison, it is no comparison. Modules that genuinely need one use +// isMainModule() from src/common/entrypoint.mjs, which realpaths both sides. // // Run: node scripts/scan-history-for-secrets.mjs [repo-path] [--json] // Help: node scripts/scan-history-for-secrets.mjs --help -import { execFileSync } from 'node:child_process'; -import { existsSync } from 'node:fs'; -import path from 'node:path'; -import { fileURLToPath } from 'node:url'; -import { - MAX_BYTES, - SKIP_EXT, - decodeUtf8, - matchSecret, - ruleLabels, -} from '../src/secret-patterns.mjs'; - -// Documented in --help. Verdict precedence when several apply: -// FOUND > INCOMPLETE > SHALLOW > CLEAN. Anything that is not a proven-clean -// full scan of a complete history is a non-zero exit. -const EXIT = { - CLEAN: 0, - FOUND: 1, - USAGE: 2, - INCOMPLETE: 3, - SHALLOW: 4, -}; +import { MAX_BYTES, ruleLabels } from '../src/secret-patterns.mjs'; +import { EXIT, scanRepo } from '../src/history-scan.mjs'; const DEFAULT_MAX_SECONDS = 600; @@ -122,46 +76,6 @@ EXIT CODES Precedence when several apply: 1 > 3 > 4 > 0. `; -class Deadline { - constructor(seconds) { - this.limitMs = seconds * 1000; - this.startedAt = Date.now(); - } - remainingMs() { - return this.limitMs - (Date.now() - this.startedAt); - } - check() { - if (this.remainingMs() <= 0) { - throw new Error(`time budget exceeded (${this.limitMs / 1000}s)`); - } - } - elapsedMs() { - return Date.now() - this.startedAt; - } -} - -/** Did this child process die on the clock rather than answer the question? */ -function isTimeout(err) { - if (!err) return false; - if (err.code === 'ETIMEDOUT' || err.signal === 'SIGTERM') return true; - return String(err.message ?? '').includes('time budget exceeded'); -} - -function makeGit(repo, deadline) { - return function git(args, { input, encoding = 'utf8' } = {}) { - deadline.check(); - return execFileSync('git', ['-C', repo, ...args], { - // Buffer, not string: with encoding 'buffer' node refuses to encode a - // string stdin, and the blob reader needs raw bytes back. - input: typeof input === 'string' ? Buffer.from(input, 'utf8') : input, - encoding, - maxBuffer: 256 * 1024 * 1024, - timeout: Math.max(1, deadline.remainingMs()), - stdio: ['pipe', 'pipe', 'pipe'], - }); - }; -} - function parseArgs(argv) { const opts = { repo: process.cwd(), @@ -198,247 +112,6 @@ function parseArgs(argv) { return { opts }; } -/** - * Reachable blobs, as [{ sha, path }]. `git rev-list --objects --all` emits - * " " for blobs and trees and a bare "" for commits; a blob - * can appear at several paths, and the first one is enough to point a human at - * the right place. - */ -function reachableObjects(git) { - const out = git(['rev-list', '--objects', '--all']); - const pathBySha = new Map(); - const order = []; - for (const line of out.split('\n')) { - if (!line) continue; - const sp = line.indexOf(' '); - if (sp === -1) continue; // commit or tag object: no path - const sha = line.slice(0, sp); - if (pathBySha.has(sha)) continue; - pathBySha.set(sha, line.slice(sp + 1)); - order.push(sha); - } - return { pathBySha, order }; -} - -/** Split [{sha,size}] into chunks of roughly `budget` bytes for cat-file --batch. */ -function chunkBySize(items, budget) { - const chunks = []; - let current = []; - let total = 0; - for (const item of items) { - if (current.length > 0 && total + item.size > budget) { - chunks.push(current); - current = []; - total = 0; - } - current.push(item); - total += item.size; - } - if (current.length > 0) chunks.push(current); - return chunks; -} - -/** - * Read a chunk of blobs in one `git cat-file --batch` call and hand each - * body to `onBlob`. The batch format is " \n\n". - */ -function readBatch(git, shas, onBlob) { - const buf = git(['cat-file', '--batch'], { - input: shas.join('\n') + '\n', - encoding: 'buffer', - }); - let at = 0; - while (at < buf.length) { - const nl = buf.indexOf(0x0a, at); - if (nl === -1) throw new Error('git cat-file --batch: truncated header'); - const header = buf.toString('utf8', at, nl); - const parts = header.split(' '); - if (parts.length < 3) { - // " missing" - the object vanished between listing and reading. - throw new Error(`git cat-file --batch: unreadable object (${header})`); - } - const [sha, , sizeStr] = parts; - const size = Number(sizeStr); - const start = nl + 1; - const end = start + size; - if (end > buf.length) throw new Error('git cat-file --batch: truncated body'); - onBlob(sha, buf.subarray(start, end)); - at = end + 1; // trailing newline - } -} - -/** - * Decode a blob, or say why it cannot be scanned. - * - * Returns { text } or { reason }. The ONLY disqualifier is bytes that do not - * decode as UTF-8: a lossy decode means we scanned something other than what - * is stored, so "no match" would be a claim about the wrong bytes. - * - * A stray NUL is deliberately NOT a disqualifier, see decodeUtf8's note. A - * file can hold a NUL as a field separator and still be 26 kB of readable - * source that could carry a key - refusing it skips the scan AND mislabels the - * skip as an error, which is the worst of both answers. - */ -function decodeBlob(body) { - const text = decodeUtf8(body); - return text === null ? { reason: 'invalid-utf8' } : { text }; -} - -/** - * The commit that introduced a blob. Only ever called for findings, so the - * cost of a --find-object walk is paid at most a handful of times. - */ -function introducingCommit(git, sha) { - try { - const out = git(['log', '--all', '--format=%H', '--find-object', sha]); - const lines = out.split('\n').filter(Boolean); - return lines.length > 0 ? lines[lines.length - 1] : null; - } catch { - return null; - } -} - -function isShallow(git, repo) { - let flagged = false; - try { - flagged = git(['rev-parse', '--is-shallow-repository']).trim() === 'true'; - } catch { - // older git: fall through to the file check - } - try { - const gitDir = git(['rev-parse', '--absolute-git-dir']).trim(); - if (existsSync(path.join(gitDir, 'shallow'))) flagged = true; - } catch { - // handled by the caller's error path - } - return flagged; -} - -function scanRepo(opts) { - const deadline = new Deadline(opts.maxSeconds); - const report = { - tool: 'scan-history-for-secrets', - repo: opts.repo, - verdict: 'clean', - exitCode: EXIT.CLEAN, - shallow: false, - examined: { - commits: 0, - refs: 0, - blobsExamined: 0, - bytesExamined: 0, - blobsSkippedBinaryMedia: 0, - blobsUnexamined: 0, - blobsReachable: 0, - }, - findings: [], - unexamined: [], - errors: [], - durationMs: 0, - }; - - let repoRoot; - const probe = makeGit(opts.repo, deadline); - try { - repoRoot = probe(['rev-parse', '--show-toplevel']).trim(); - } catch (err) { - // A git that timed out has told us NOTHING about the path. Reporting that - // as "not a git repository" would be a second-hand version of the bug this - // tool is about: an unanswered question rendered as an answer. - if (isTimeout(err)) throw err; - return { report, usageError: `not a git repository: ${opts.repo}` }; - } - report.repo = repoRoot; - - const git = makeGit(repoRoot, deadline); - - try { - report.shallow = isShallow(git, repoRoot); - report.examined.commits = Number(git(['rev-list', '--all', '--count']).trim()) || 0; - report.examined.refs = git(['for-each-ref', '--format=%(refname)']) - .split('\n').filter(Boolean).length; - - const { pathBySha, order } = reachableObjects(git); - - // One --batch-check pass gives type and size for everything, so the - // expensive --batch read only ever asks for blobs we mean to scan. - const checkOut = git(['cat-file', '--batch-check'], { input: order.join('\n') + '\n' }); - const blobs = []; - for (const line of checkOut.split('\n')) { - if (!line) continue; - const [sha, type, sizeStr] = line.split(' '); - if (type !== 'blob') continue; - blobs.push({ sha, size: Number(sizeStr) || 0, path: pathBySha.get(sha) ?? '(unknown path)' }); - } - report.examined.blobsReachable = blobs.length; - - const toRead = []; - for (const blob of blobs) { - if (SKIP_EXT.test(blob.path)) { - report.examined.blobsSkippedBinaryMedia += 1; - continue; - } - if (blob.size > opts.maxBytes) { - report.unexamined.push({ path: blob.path, blob: blob.sha, reason: 'over-size-cap' }); - continue; - } - toRead.push(blob); - } - - const bySha = new Map(toRead.map((b) => [b.sha, b])); - for (const chunk of chunkBySize(toRead, 32 * 1024 * 1024)) { - readBatch(git, chunk.map((b) => b.sha), (sha, body) => { - const blob = bySha.get(sha); - if (!blob) return; - const { text, reason } = decodeBlob(body); - if (reason) { - report.unexamined.push({ path: blob.path, blob: sha, reason }); - return; - } - report.examined.blobsExamined += 1; - report.examined.bytesExamined += body.length; - const hit = matchSecret(text); - if (hit) { - // label + line + length only. The value stays in the repo, which is - // the one place it is already. - report.findings.push({ - rule: hit.label, - path: blob.path, - line: hit.line, - blob: sha, - commit: introducingCommit(git, sha), - }); - } - }); - } - } catch (err) { - report.errors.push(String(err && err.message ? err.message : err)); - } - - report.examined.blobsUnexamined = report.unexamined.length; - report.durationMs = deadline.elapsedMs(); - - if (report.findings.length > 0) { - report.verdict = 'found'; - report.exitCode = EXIT.FOUND; - } else if (report.errors.length > 0 || report.unexamined.length > 0) { - report.verdict = 'incomplete'; - report.exitCode = EXIT.INCOMPLETE; - } else if (report.shallow) { - report.verdict = 'shallow'; - report.exitCode = EXIT.SHALLOW; - } else { - report.verdict = 'clean'; - report.exitCode = EXIT.CLEAN; - } - // A shallow repo is never a clean verdict, whatever else happened. - if (report.shallow && report.verdict === 'clean') { - report.verdict = 'shallow'; - report.exitCode = EXIT.SHALLOW; - } - return { report }; -} - function printHuman(report) { const e = report.examined; const out = report.verdict === 'clean' ? console.log : console.error; @@ -557,8 +230,7 @@ function main() { process.exit(report.exitCode); } -const invokedDirectly = - process.argv[1] && path.resolve(process.argv[1]) === fileURLToPath(import.meta.url); -if (invokedDirectly) main(); - -export { EXIT, scanRepo }; +// No guard. See the header: this file is only ever run, never imported, and a +// guard that silently evaluates false is exactly how this tool once exited 0 +// without scanning anything. +main(); diff --git a/scripts/team-watchdog.mjs b/scripts/team-watchdog.mjs index 721ae63..16e0fbf 100644 --- a/scripts/team-watchdog.mjs +++ b/scripts/team-watchdog.mjs @@ -103,9 +103,9 @@ let ROSTER = []; // State persists to a file so it survives one-shot (StartInterval) runs and // sleep/wake. In-process setTimeout pauses when the Mac sleeps, so the watchdog // runs as a launchd StartInterval one-shot (ONCE=1) instead of a long loop. -import { readFileSync, writeFileSync, realpathSync } from 'node:fs'; +import { readFileSync, writeFileSync } from 'node:fs'; import { execFile } from 'node:child_process'; -import { fileURLToPath } from 'node:url'; +import { isMainModule } from '../src/common/entrypoint.mjs'; const STATE_FILE = '/tmp/team-watchdog-state.json'; let state = {}; function loadState() { try { return JSON.parse(readFileSync(STATE_FILE, 'utf8')); } catch { return {}; } } @@ -240,8 +240,4 @@ async function run() { // Only run the loop when executed directly (node scripts/team-watchdog.mjs), // never when imported by a test. Keeping import side-effect-free is what lets // the pure helpers (loadRoster, lastSeen, ageMinutes, isStale) be unit-tested. -const invokedDirectly = (() => { - try { return !!process.argv[1] && realpathSync(process.argv[1]) === fileURLToPath(import.meta.url); } - catch { return false; } -})(); -if (invokedDirectly) run(); +if (isMainModule(import.meta.url)) run(); diff --git a/src/common/entrypoint.mjs b/src/common/entrypoint.mjs new file mode 100644 index 0000000..03a6602 --- /dev/null +++ b/src/common/entrypoint.mjs @@ -0,0 +1,42 @@ +// SPDX-License-Identifier: AGPL-3.0-only +// +// "Was this module run directly, or imported?" - ONE implementation. +// +// Getting this wrong is silent, and silence from a guard is the worst kind of +// bug: the script starts, does nothing, and exits 0. For a security tool that +// reads as "clean". +// +// The trap is that `import.meta.url` is ALWAYS realpath-resolved by node, and +// `process.argv[1]` never is. So every one of these is broken through a +// symlink: +// +// import.meta.url === `file://${process.argv[1]}` // and spaces +// import.meta.url === pathToFileURL(process.argv[1]).href +// path.resolve(process.argv[1]) === fileURLToPath(import.meta.url) +// +// On macOS /tmp IS a symlink to /private/tmp, so every scratch dir, every +// worktree under /tmp and every ~/bin symlink hits it, as does a CI checkout +// in a symlinked workspace. This repo hit the same bug three times in one day +// in three different files before anyone swept for it, which is why the +// comparison now lives in exactly one place with a test that fails if a new +// entry point rolls its own. +// +// Resolve BOTH sides to a real path and compare those. + +import { realpathSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; + +/** + * True when `importMetaUrl`'s module is the script node was asked to run. + * + * @param {string} importMetaUrl - always pass `import.meta.url`. + */ +export function isMainModule(importMetaUrl) { + try { + if (!process.argv[1]) return false; + return realpathSync(process.argv[1]) === realpathSync(fileURLToPath(importMetaUrl)); + } catch { + // argv[1] gone, unreadable, or not a path at all. Not a main-module run. + return false; + } +} diff --git a/src/history-scan.mjs b/src/history-scan.mjs new file mode 100644 index 0000000..46ba092 --- /dev/null +++ b/src/history-scan.mjs @@ -0,0 +1,357 @@ +// SPDX-License-Identifier: AGPL-3.0-only +// +// Scan REACHABLE GIT HISTORY for credentials - not the working tree. +// +// THE GAP THIS CLOSES. scripts/check-stageable-secrets.mjs reads files from +// disk and answers one question: "would `git add -A` stage a credential right +// now". That is a useful question and it is not this one. A key that was +// committed on Tuesday and deleted on Wednesday is gone from the working tree, +// gone from `git status`, gone from the file listing - and still sits in the +// pack file, one `git log -p` away from anyone who clones the repo. To the +// disk-reading scanner that repo looks spotless. It is not: it is leaking. +// +// This repo is PUBLIC, and a snapshot pushed into it is world-readable the +// instant it lands, with no private staging period in which to notice. +// +// WHAT IT DOES. Every blob reachable from every ref (`git rev-list +// --objects --all`), read through `git cat-file --batch`, matched against the +// SHARED pattern list in src/secret-patterns.mjs - shared so that the two +// scanners cannot drift apart, which is the house defect this repo already has +// a written-up history of. +// +// WHAT IT REFUSES TO DO. +// +// 1. It never prints a matched value, not partially, not redacted with a +// prefix. Path + commit + rule name is enough to act on. A prefix is not +// "safe", it is the first six characters of a live key in a CI log. +// +// 2. It never renders "I could not check" as "clean". An unreadable object, +// a blob it cannot decode, a blob over the size cap, a timeout: all of +// those exit 3 (could-not-complete), never 0. This is the single most +// repeated bug in this codebase and it gets its own exit code. +// +// 3. It never calls a shallow clone clean. A `--depth` clone's history is +// not absent, it is UNEXAMINED, and the blobs that were cut off are the +// old ones - which is precisely where a deleted-but-reachable key lives. +// Shallow exits 4 and says so at the top of the report. +// +// 4. It scans anything that DECODES. "Contains a NUL byte" is not the same +// question as "is not text", and an early version of this file failed +// that distinction: it refused 26 kB of valid UTF-8 JavaScript over one +// NUL used as a field separator, and called the refusal an error instead +// of scanning the other 26 kB. Fail-closed is about what you could not +// read, not about bytes that merely look alarming. +// +// 5. It always says how much it looked at. A freshly created snapshot repo +// has one commit; "history clean" after examining one commit is not +// reassurance, it is the false confidence this tool exists to prevent. +// +// This module is the ENGINE. It is pure: importing it starts nothing, and it +// has no argv parsing, no printing and no process.exit. The command line lives +// in scripts/scan-history-for-secrets.mjs, which is an entry point and only an +// entry point - it calls main() unconditionally, because a run-only-if guard +// that silently evaluates false is how a security tool comes to exit 0 without +// scanning anything. + +import { execFileSync } from 'node:child_process'; +import { existsSync } from 'node:fs'; +import path from 'node:path'; +import { + MAX_BYTES, + SKIP_EXT, + decodeUtf8, + matchSecret, + ruleLabels, +} from '../src/secret-patterns.mjs'; + +// Documented in --help. Verdict precedence when several apply: +// FOUND > INCOMPLETE > SHALLOW > CLEAN. Anything that is not a proven-clean +// full scan of a complete history is a non-zero exit. +export const EXIT = { + CLEAN: 0, + FOUND: 1, + USAGE: 2, + INCOMPLETE: 3, + SHALLOW: 4, +}; + +class Deadline { + constructor(seconds) { + this.limitMs = seconds * 1000; + this.startedAt = Date.now(); + } + remainingMs() { + return this.limitMs - (Date.now() - this.startedAt); + } + check() { + if (this.remainingMs() <= 0) { + throw new Error(`time budget exceeded (${this.limitMs / 1000}s)`); + } + } + elapsedMs() { + return Date.now() - this.startedAt; + } +} + +/** Did this child process die on the clock rather than answer the question? */ +function isTimeout(err) { + if (!err) return false; + if (err.code === 'ETIMEDOUT' || err.signal === 'SIGTERM') return true; + return String(err.message ?? '').includes('time budget exceeded'); +} + +function makeGit(repo, deadline) { + return function git(args, { input, encoding = 'utf8' } = {}) { + deadline.check(); + return execFileSync('git', ['-C', repo, ...args], { + // Buffer, not string: with encoding 'buffer' node refuses to encode a + // string stdin, and the blob reader needs raw bytes back. + input: typeof input === 'string' ? Buffer.from(input, 'utf8') : input, + encoding, + maxBuffer: 256 * 1024 * 1024, + timeout: Math.max(1, deadline.remainingMs()), + stdio: ['pipe', 'pipe', 'pipe'], + }); + }; +} + +/** + * Reachable blobs, as [{ sha, path }]. `git rev-list --objects --all` emits + * " " for blobs and trees and a bare "" for commits; a blob + * can appear at several paths, and the first one is enough to point a human at + * the right place. + */ +function reachableObjects(git) { + const out = git(['rev-list', '--objects', '--all']); + const pathBySha = new Map(); + const order = []; + for (const line of out.split('\n')) { + if (!line) continue; + const sp = line.indexOf(' '); + if (sp === -1) continue; // commit or tag object: no path + const sha = line.slice(0, sp); + if (pathBySha.has(sha)) continue; + pathBySha.set(sha, line.slice(sp + 1)); + order.push(sha); + } + return { pathBySha, order }; +} + +/** Split [{sha,size}] into chunks of roughly `budget` bytes for cat-file --batch. */ +function chunkBySize(items, budget) { + const chunks = []; + let current = []; + let total = 0; + for (const item of items) { + if (current.length > 0 && total + item.size > budget) { + chunks.push(current); + current = []; + total = 0; + } + current.push(item); + total += item.size; + } + if (current.length > 0) chunks.push(current); + return chunks; +} + +/** + * Read a chunk of blobs in one `git cat-file --batch` call and hand each + * body to `onBlob`. The batch format is " \n\n". + */ +function readBatch(git, shas, onBlob) { + const buf = git(['cat-file', '--batch'], { + input: shas.join('\n') + '\n', + encoding: 'buffer', + }); + let at = 0; + while (at < buf.length) { + const nl = buf.indexOf(0x0a, at); + if (nl === -1) throw new Error('git cat-file --batch: truncated header'); + const header = buf.toString('utf8', at, nl); + const parts = header.split(' '); + if (parts.length < 3) { + // " missing" - the object vanished between listing and reading. + throw new Error(`git cat-file --batch: unreadable object (${header})`); + } + const [sha, , sizeStr] = parts; + const size = Number(sizeStr); + const start = nl + 1; + const end = start + size; + if (end > buf.length) throw new Error('git cat-file --batch: truncated body'); + onBlob(sha, buf.subarray(start, end)); + at = end + 1; // trailing newline + } +} + +/** + * Decode a blob, or say why it cannot be scanned. + * + * Returns { text } or { reason }. The ONLY disqualifier is bytes that do not + * decode as UTF-8: a lossy decode means we scanned something other than what + * is stored, so "no match" would be a claim about the wrong bytes. + * + * A stray NUL is deliberately NOT a disqualifier, see decodeUtf8's note. A + * file can hold a NUL as a field separator and still be 26 kB of readable + * source that could carry a key - refusing it skips the scan AND mislabels the + * skip as an error, which is the worst of both answers. + */ +function decodeBlob(body) { + const text = decodeUtf8(body); + return text === null ? { reason: 'invalid-utf8' } : { text }; +} + +/** + * The commit that introduced a blob. Only ever called for findings, so the + * cost of a --find-object walk is paid at most a handful of times. + */ +function introducingCommit(git, sha) { + try { + const out = git(['log', '--all', '--format=%H', '--find-object', sha]); + const lines = out.split('\n').filter(Boolean); + return lines.length > 0 ? lines[lines.length - 1] : null; + } catch { + return null; + } +} + +function isShallow(git, repo) { + let flagged = false; + try { + flagged = git(['rev-parse', '--is-shallow-repository']).trim() === 'true'; + } catch { + // older git: fall through to the file check + } + try { + const gitDir = git(['rev-parse', '--absolute-git-dir']).trim(); + if (existsSync(path.join(gitDir, 'shallow'))) flagged = true; + } catch { + // handled by the caller's error path + } + return flagged; +} + +export function scanRepo(opts) { + const deadline = new Deadline(opts.maxSeconds); + const report = { + tool: 'scan-history-for-secrets', + repo: opts.repo, + verdict: 'clean', + exitCode: EXIT.CLEAN, + shallow: false, + examined: { + commits: 0, + refs: 0, + blobsExamined: 0, + bytesExamined: 0, + blobsSkippedBinaryMedia: 0, + blobsUnexamined: 0, + blobsReachable: 0, + }, + findings: [], + unexamined: [], + errors: [], + durationMs: 0, + }; + + let repoRoot; + const probe = makeGit(opts.repo, deadline); + try { + repoRoot = probe(['rev-parse', '--show-toplevel']).trim(); + } catch (err) { + // A git that timed out has told us NOTHING about the path. Reporting that + // as "not a git repository" would be a second-hand version of the bug this + // tool is about: an unanswered question rendered as an answer. + if (isTimeout(err)) throw err; + return { report, usageError: `not a git repository: ${opts.repo}` }; + } + report.repo = repoRoot; + + const git = makeGit(repoRoot, deadline); + + try { + report.shallow = isShallow(git, repoRoot); + report.examined.commits = Number(git(['rev-list', '--all', '--count']).trim()) || 0; + report.examined.refs = git(['for-each-ref', '--format=%(refname)']) + .split('\n').filter(Boolean).length; + + const { pathBySha, order } = reachableObjects(git); + + // One --batch-check pass gives type and size for everything, so the + // expensive --batch read only ever asks for blobs we mean to scan. + const checkOut = git(['cat-file', '--batch-check'], { input: order.join('\n') + '\n' }); + const blobs = []; + for (const line of checkOut.split('\n')) { + if (!line) continue; + const [sha, type, sizeStr] = line.split(' '); + if (type !== 'blob') continue; + blobs.push({ sha, size: Number(sizeStr) || 0, path: pathBySha.get(sha) ?? '(unknown path)' }); + } + report.examined.blobsReachable = blobs.length; + + const toRead = []; + for (const blob of blobs) { + if (SKIP_EXT.test(blob.path)) { + report.examined.blobsSkippedBinaryMedia += 1; + continue; + } + if (blob.size > opts.maxBytes) { + report.unexamined.push({ path: blob.path, blob: blob.sha, reason: 'over-size-cap' }); + continue; + } + toRead.push(blob); + } + + const bySha = new Map(toRead.map((b) => [b.sha, b])); + for (const chunk of chunkBySize(toRead, 32 * 1024 * 1024)) { + readBatch(git, chunk.map((b) => b.sha), (sha, body) => { + const blob = bySha.get(sha); + if (!blob) return; + const { text, reason } = decodeBlob(body); + if (reason) { + report.unexamined.push({ path: blob.path, blob: sha, reason }); + return; + } + report.examined.blobsExamined += 1; + report.examined.bytesExamined += body.length; + const hit = matchSecret(text); + if (hit) { + // label + line + length only. The value stays in the repo, which is + // the one place it is already. + report.findings.push({ + rule: hit.label, + path: blob.path, + line: hit.line, + blob: sha, + commit: introducingCommit(git, sha), + }); + } + }); + } + } catch (err) { + report.errors.push(String(err && err.message ? err.message : err)); + } + + report.examined.blobsUnexamined = report.unexamined.length; + report.durationMs = deadline.elapsedMs(); + + if (report.findings.length > 0) { + report.verdict = 'found'; + report.exitCode = EXIT.FOUND; + } else if (report.errors.length > 0 || report.unexamined.length > 0) { + report.verdict = 'incomplete'; + report.exitCode = EXIT.INCOMPLETE; + } else if (report.shallow) { + report.verdict = 'shallow'; + report.exitCode = EXIT.SHALLOW; + } else { + report.verdict = 'clean'; + report.exitCode = EXIT.CLEAN; + } + // A shallow repo is never a clean verdict, whatever else happened. + if (report.shallow && report.verdict === 'clean') { + report.verdict = 'shallow'; + report.exitCode = EXIT.SHALLOW; + } + return { report }; +} diff --git a/src/mcp-server.mjs b/src/mcp-server.mjs index ac13703..5179cc4 100644 --- a/src/mcp-server.mjs +++ b/src/mcp-server.mjs @@ -35,6 +35,7 @@ import { nudgeTmux } from './common/notify.mjs'; import { tmuxRun } from './ide/tmux-runner.mjs'; import { loadConfig } from './config.mjs'; import { assertRoomVoice } from './responder-lock.mjs'; +import { isMainModule } from './common/entrypoint.mjs'; import { defaultCallbackBase, createIntent, decideIntent, @@ -1047,7 +1048,7 @@ export async function runMcpServer({ configPath } = {}) { } // Run directly when invoked as a script. -if (import.meta.url === `file://${process.argv[1]}`) { +if (isMainModule(import.meta.url)) { // Allow --config on the command line (mirrors other CLI subcommands). const argv = process.argv.slice(2); let configPath; diff --git a/test/scan-history-for-secrets.test.mjs b/test/scan-history-for-secrets.test.mjs index 4f17fa6..890166d 100644 --- a/test/scan-history-for-secrets.test.mjs +++ b/test/scan-history-for-secrets.test.mjs @@ -18,7 +18,7 @@ import assert from 'node:assert/strict'; import test from 'node:test'; import { spawnSync } from 'node:child_process'; -import { mkdtempSync, mkdirSync, rmSync, writeFileSync, readFileSync, realpathSync } from 'node:fs'; +import { mkdtempSync, mkdirSync, readdirSync, rmSync, symlinkSync, writeFileSync, readFileSync, realpathSync } from 'node:fs'; import { tmpdir } from 'node:os'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -362,3 +362,151 @@ test('both scanners use the same pattern list (fails if the lists diverge)', () `${path.basename(file)} declares its own pattern list - that is the drift this test exists to stop`); } }); + +// --------------------------------------------------------------------------- +// Entry points must not be silent through a symlink. +// +// The failure this guards against is SILENCE: the script starts, the +// run-only-if-invoked-directly guard evaluates false because node realpaths +// import.meta.url and never realpaths process.argv[1], main() is skipped, and +// node exits 0. For a security tool exit 0 means clean, so it blesses a repo it +// never looked at. macOS /tmp IS a symlink to /private/tmp, so this fires for +// every scratch dir, every worktree under /tmp and every ~/bin symlink. +// --------------------------------------------------------------------------- + +/** Strip the one field that legitimately differs between two runs. */ +function stableReport(stdout) { + const report = JSON.parse(stdout); + delete report.durationMs; + delete report.examined.bytesExamined; + return report; +} + +test('the history scanner behaves identically through a symlinked path', () => { + const fixture = newRepo('iak-hist-symlink-'); + const secret = synthetic('SYMLINK'); + writeFileSync(path.join(fixture, 'leaked-config.json'), `key=${secret}\n`); + commitAll(fixture, 'oops'); + rmSync(path.join(fixture, 'leaked-config.json')); + commitAll(fixture, 'delete it'); + + // A real symlink made here, rather than relying on /tmp being one. + const linkDir = tempDir('iak-hist-linkroot-'); + const linkedRepo = path.join(linkDir, 'repo-link'); + symlinkSync(repoRoot, linkedRepo); + const linkedScanner = path.join(linkedRepo, 'scripts', 'scan-history-for-secrets.mjs'); + + const direct = spawnSync('node', [scanner, fixture, '--json'], { encoding: 'utf8', env: GIT_ENV }); + const linked = spawnSync('node', [linkedScanner, fixture, '--json'], { encoding: 'utf8', env: GIT_ENV }); + + assert.ok(linked.stdout.length > 0, + 'a scanner invoked through a symlink must not silently produce nothing'); + assert.equal(linked.status, direct.status, + `exit code differs through a symlink: ${linked.status} vs ${direct.status}`); + assert.equal(linked.status, EXIT.FOUND, 'and it must still be the FOUND it would report directly'); + assert.deepEqual(stableReport(linked.stdout), stableReport(direct.stdout)); + + // The same trap, one level down: the file itself symlinked into a ~/bin. + const binLink = path.join(linkDir, 'scan-history'); + symlinkSync(scanner, binLink); + const viaBin = spawnSync('node', [binLink, fixture, '--json'], { encoding: 'utf8', env: GIT_ENV }); + assert.equal(viaBin.status, EXIT.FOUND, 'a ~/bin-style symlink to the script must still run it'); + assert.deepEqual(stableReport(viaBin.stdout), stableReport(direct.stdout)); + + // --help too: the original bug made even that produce nothing. + const help = spawnSync('node', [linkedScanner, '--help'], { encoding: 'utf8', env: GIT_ENV }); + assert.equal(help.status, 0); + assert.match(help.stdout, /EXIT CODES/); +}); + +test('the pre-commit secret gate fires through a symlinked path', () => { + // If check-stageable-secrets.mjs no-ops, a commit carrying a credential walks + // straight through the hook. Same assertion, because the same trap applies. + const fixture = newRepo('iak-stage-symlink-'); + writeFileSync(path.join(fixture, 'README.md'), '# fixture\n'); + commitAll(fixture, 'initial'); + writeFileSync(path.join(fixture, 'creds.json'), `{ "api_key": "${synthetic('STAGED')}" }\n`); + + const linkDir = tempDir('iak-stage-linkroot-'); + const linkedRepo = path.join(linkDir, 'repo-link'); + symlinkSync(repoRoot, linkedRepo); + + const run = (script) => spawnSync('node', [script], { cwd: fixture, encoding: 'utf8', env: GIT_ENV }); + const direct = run(stageableScanner); + const linked = run(path.join(linkedRepo, 'scripts', 'check-stageable-secrets.mjs')); + + assert.equal(direct.status, 1, 'the gate must fire on a stageable credential at all'); + assert.equal(linked.status, direct.status, + 'the pre-commit gate is a no-op through a symlink: a credential would pass the hook'); + assert.equal(linked.stdout, direct.stdout); + assert.equal(linked.stderr, direct.stderr); + assert.ok(!`${linked.stdout}${linked.stderr}`.includes('TESTONLY'), 'and it still prints no value'); +}); + +test('no entry point hand-rolls the main-module comparison', () => { + // We found this bug three times in one day by tripping over instances one at + // a time. This is the sweep, kept. + // + // The rule is PROXIMITY, not "the file mentions isMainModule somewhere": a + // first version of this test only checked the latter, and it passed happily + // when the guard was reverted to the broken comparison while the (now unused) + // import stayed behind. A check that cannot fail is not a check. + // + // The correct idiom never names process.argv[1] at the call site - the only + // place that does is the shared helper. + const helper = path.join('src', 'common', 'entrypoint.mjs'); // the one implementation + const offenders = []; + for (const dir of ['bin', 'scripts', 'src']) { + for (const name of readdirSync(path.join(repoRoot, dir))) { + if (!/\.(mjs|js|cjs)$/.test(name)) continue; + const rel = path.join(dir, name); + if (rel === helper) continue; + // Drop whole-line comments before looking: this file's own header quotes + // the broken idiom on purpose, and a doc comment is not an entry point. + // Lines with code plus a trailing comment are still scanned, so the + // check errs towards flagging rather than towards silence. + const src = readFileSync(path.join(repoRoot, rel), 'utf8') + .split('\n') + .filter((line) => !/^\s*(\/\/|\*|\/\*)/.test(line)) + .join(' ') + .replace(/\s+/g, ' '); + for (let at = src.indexOf('process.argv[1]'); at !== -1; at = src.indexOf('process.argv[1]', at + 1)) { + const window = src.slice(Math.max(0, at - 200), at + 200); + if (/import\.meta\.url|fileURLToPath|pathToFileURL/.test(window)) { + offenders.push(rel); + break; + } + } + } + } + assert.deepEqual(offenders, [], + `these compare argv[1] against import.meta.url themselves, so they no-op through a symlink: ${offenders.join(', ')}`); +}); + +test('the scanner CLI has no main-module guard at all', () => { + // Structural, not behavioural: the safest guard is the one that is not there. + const src = readFileSync(scanner, 'utf8'); + assert.ok(!/if\s*\([^)]*import\.meta\.url[^)]*\)\s*(\{|main\(\))/.test(src), + 'the entry point must call main() unconditionally'); + assert.match(src, /^main\(\);$/m); +}); + +test('isMainModule resolves symlinks on both sides', () => { + const dir = tempDir('iak-ismain-'); + const real = path.join(dir, 'real-entry.mjs'); + writeFileSync(real, [ + "import { isMainModule } from " + JSON.stringify(path.join(repoRoot, 'src/common/entrypoint.mjs')) + ";", + 'process.stdout.write(isMainModule(import.meta.url) ? "MAIN" : "NOT-MAIN");', + ].join('\n')); + const link = path.join(dir, 'linked-entry.mjs'); + symlinkSync(real, link); + + assert.equal(spawnSync('node', [real], { encoding: 'utf8' }).stdout, 'MAIN'); + assert.equal(spawnSync('node', [link], { encoding: 'utf8' }).stdout, 'MAIN', + 'a symlinked entry point is still the main module'); + + // And it must still say NOT-MAIN when actually imported. + const importer = path.join(dir, 'importer.mjs'); + writeFileSync(importer, `import ${JSON.stringify(link)};\n`); + assert.equal(spawnSync('node', [importer], { encoding: 'utf8' }).stdout, 'NOT-MAIN'); +}); From a159ddd7cfcec3ce995d1253f6810f60ca32f068 Mon Sep 17 00:00:00 2001 From: Petrus Pennanen Date: Sun, 20 Sep 2026 11:15:19 +0200 Subject: [PATCH 4/7] Detect JWTs, report their claims, never their token A Supabase service_role key is a JWT, and this list had no JWT rule. On the GroupMind repo the scanner reported 14 findings and said nothing about test_fetch.js, which carries a service_role token for the live production project: full access, Row Level Security bypassed, valid until 2036. The file is deleted from the working tree and reachable from origin/main, which is the exact failure mode this tool was built for. It walked past it. The root cause is not a missing regex, it is a partial port. This list was lifted out of check-stageable-secrets.mjs and called the single source of truth while .githooks/pre-commit still held a bash list with four formats it lacked. The port took two of them. Missing were: JWT, Moltbook secret key, AgentMail key, Discord bot token. All four are here now, and a test asserts one synthetic sample per format is caught, so the next omission fails the suite instead of passing quietly. JWT hits are triaged by their CLAIMS. describeJwt decodes the header and payload and reports an allowlist - alg, typ, kid, iss, aud, role, ref, scope, iat, nbf, exp - plus whether exp is in the past, how many days remain, and whether there is no exp claim at all, which is worse news rather than better. Claims are metadata, not the secret, and they are the difference between "some JWT" and "a non-expiring service_role key for production". The token, its segments and its signature are never printed, in any mode, and a test asserts every prefix of the token from 8 characters up is absent from stdout, stderr and the JSON. Example JWTs in a README or a .d.ts are found rather than suppressed. A noisy true positive is cheap when the claims are right there; a silent miss is what got us here. matchSecret became matchSecrets: every rule that fires is reported, not just the first. A rule high in the list used to shadow everything below it in the same blob, so a file with an API key on line 3 and a service_role JWT on line 40 reported one finding and hid the other. Overlapping matches of the same value are still reported once, by the most specific rule, so `const apiKey = "sk-..."` does not count twice. Counts after the change: ide-agent-kit 18 -> 32 findings (9 Moltbook keys that no rule caught before, plus room keys that were being shadowed), GroupMind 14 -> 16 (the service_role token, and an example JWT in a committed node_modules test fixture). Co-Authored-By: Claude Opus 5 --- scripts/check-stageable-secrets.mjs | 40 ++++-- scripts/scan-history-for-secrets.mjs | 11 ++ src/history-scan.mjs | 31 +++-- src/secret-patterns.mjs | 133 +++++++++++++++++-- test/scan-history-for-secrets.test.mjs | 170 +++++++++++++++++++++++++ 5 files changed, 349 insertions(+), 36 deletions(-) diff --git a/scripts/check-stageable-secrets.mjs b/scripts/check-stageable-secrets.mjs index ab6153f..a73ca18 100755 --- a/scripts/check-stageable-secrets.mjs +++ b/scripts/check-stageable-secrets.mjs @@ -38,7 +38,7 @@ import { execFileSync } from 'node:child_process'; import { readFileSync, statSync } from 'node:fs'; -import { MAX_BYTES, SKIP_EXT, decodeUtf8, matchSecret, ruleLabels } from '../src/secret-patterns.mjs'; +import { MAX_BYTES, SKIP_EXT, decodeUtf8, matchSecrets, ruleLabels } from '../src/secret-patterns.mjs'; // The pattern list, the binary-extension skip list and the size cap now live // in src/secret-patterns.mjs, shared with scripts/scan-history-for-secrets.mjs. @@ -48,7 +48,7 @@ import { MAX_BYTES, SKIP_EXT, decodeUtf8, matchSecret, ruleLabels } from '../src // note above describes. Do not re-introduce a local copy here; a test asserts // that neither scanner has one. // -// matchSecret() returns a rule name, a line and a length, never the matched +// matchSecrets() returns rule names, lines and lengths, never the matched // text. That is why the finding below no longer prints a 6-character prefix of // the match: a prefix of a live key in a CI transcript is still a prefix of a // live key. @@ -75,26 +75,41 @@ function stageableFiles() { } function scan(path) { - if (SKIP_EXT.test(path)) return null; + if (SKIP_EXT.test(path)) return []; let bytes; try { - if (statSync(path).size > MAX_BYTES) return null; + if (statSync(path).size > MAX_BYTES) return []; bytes = readFileSync(path); } catch { - return null; // unreadable, gone, or a directory: not our problem + return []; // unreadable, gone, or a directory: not our problem } // Read as bytes and decode strictly, rather than the old "readFileSync utf8 // then look for a NUL". A NUL is not a proof of binary - bin/iak-pending.mjs // uses one as a field separator inside 26 kB of valid JavaScript - and the // lossy utf8 read could not tell a real binary from text anyway. const text = decodeUtf8(bytes); - if (text === null) return null; // genuinely not text + if (text === null) return []; // genuinely not text // Report WHERE and WHICH RULE, never the value itself. This output ends up // in CI logs and terminal scrollback, and a scanner that prints the secret it // found has simply moved the leak. - const hit = matchSecret(text); - if (!hit) return null; - return { label: hit.label, line: hit.line, hint: `${hit.length} chars, value not printed` }; + return matchSecrets(text).map((hit) => ({ + label: hit.label, + line: hit.line, + hint: `${hit.length} chars, value not printed`, + detail: hit.detail, + })); +} + +// Claims for a JWT hit: metadata only, never the token. Shared wording with +// the history scanner so a finding reads the same wherever it surfaces. +function describeDetail(detail) { + if (!detail || detail.kind !== 'jwt') return ''; + const claims = Object.entries(detail.claims).map(([k, v]) => `${k}=${v}`).join(' '); + let expiry; + if (detail.noExpiry) expiry = 'NO EXPIRY CLAIM'; + else if (detail.expired) expiry = `EXPIRED ${detail.expiresAt}`; + else expiry = `live until ${detail.expiresAt} (${detail.daysRemaining} days)`; + return `claims: ${claims || '(none readable)'} | ${expiry}`; } // Shared with the history scanner; a test compares the two outputs so the @@ -106,8 +121,7 @@ if (process.argv.includes('--print-rules')) { const findings = []; for (const f of stageableFiles()) { - const hit = scan(f); - if (hit) findings.push({ file: f, ...hit }); + for (const hit of scan(f)) findings.push({ file: f, ...hit }); } if (findings.length === 0) { @@ -118,7 +132,9 @@ if (findings.length === 0) { console.error(`FAIL: ${findings.length} stageable file(s) contain credential-shaped data\n`); for (const f of findings) { console.error(` ${f.file}:${f.line}`); - console.error(` ${f.label} - ${f.hint}\n`); + console.error(` ${f.label} - ${f.hint}`); + if (f.detail) console.error(` ${describeDetail(f.detail)}`); + console.error(''); } console.error('These are NOT committed yet, and this repo is public.'); console.error('Fix by ignoring the file, not by deleting it — something may be using it:'); diff --git a/scripts/scan-history-for-secrets.mjs b/scripts/scan-history-for-secrets.mjs index 01ada0e..93a3c9b 100755 --- a/scripts/scan-history-for-secrets.mjs +++ b/scripts/scan-history-for-secrets.mjs @@ -147,6 +147,17 @@ function printHuman(report) { for (const f of report.findings) { out(` ${f.path}:${f.line}`); out(` rule: ${f.rule}`); + // Claims, never the token. What a JWT IS - service_role for which + // project, expiring when - is the difference between "some JWT" and "a + // non-expiring full-access production database credential", and it is + // metadata, not the secret. + if (f.detail && f.detail.kind === 'jwt') { + const claims = Object.entries(f.detail.claims).map(([k, v]) => `${k}=${v}`).join(' '); + out(` claims: ${claims || '(none readable)'}`); + if (f.detail.noExpiry) out(' expiry: NO EXPIRY CLAIM - this token does not stop working'); + else if (f.detail.expired) out(` expiry: EXPIRED ${f.detail.expiresAt}`); + else out(` expiry: LIVE until ${f.detail.expiresAt} (${f.detail.daysRemaining} days remaining)`); + } out(` blob: ${f.blob}`); out(` commit: ${f.commit ?? '(not attributable to a single commit)'}`); } diff --git a/src/history-scan.mjs b/src/history-scan.mjs index 46ba092..707ae57 100644 --- a/src/history-scan.mjs +++ b/src/history-scan.mjs @@ -60,7 +60,7 @@ import { MAX_BYTES, SKIP_EXT, decodeUtf8, - matchSecret, + matchSecrets, ruleLabels, } from '../src/secret-patterns.mjs'; @@ -314,17 +314,24 @@ export function scanRepo(opts) { } report.examined.blobsExamined += 1; report.examined.bytesExamined += body.length; - const hit = matchSecret(text); - if (hit) { - // label + line + length only. The value stays in the repo, which is - // the one place it is already. - report.findings.push({ - rule: hit.label, - path: blob.path, - line: hit.line, - blob: sha, - commit: introducingCommit(git, sha), - }); + const hits = matchSecrets(text); + if (hits.length > 0) { + // One finding per rule that fired, not just the first: a rule high in + // the list used to shadow everything below it in the same blob. + const commit = introducingCommit(git, sha); + for (const hit of hits) { + // label + line + length only, plus safe metadata for rules that + // can produce it (JWT claims). The value stays in the repo, which + // is the one place it is already. + report.findings.push({ + rule: hit.label, + path: blob.path, + line: hit.line, + blob: sha, + commit, + ...(hit.detail ? { detail: hit.detail } : {}), + }); + } } }); } diff --git a/src/secret-patterns.mjs b/src/secret-patterns.mjs index 25d7ac7..ce02291 100644 --- a/src/secret-patterns.mjs +++ b/src/secret-patterns.mjs @@ -27,7 +27,27 @@ // Shapes worth stopping for. Deliberately narrow: a scanner that cries wolf // gets disabled, and a disabled scanner is worse than none. Every pattern // here is a real credential format we use or plausibly would. +// +// ORDER MATTERS: matchSecrets reports every rule that fires, but a value is +// labelled by the first rule that claims it, and a wrong label sends someone +// rotating the wrong credential. JWT goes first because a base64url segment +// can easily contain "sk-" or "AIza" by chance, while no sk-/AIza key contains +// a dotted JWT triple. +// +// PROVENANCE, worth reading before adding the next one. This list was lifted +// out of check-stageable-secrets.mjs and was supposed to be the single source +// of truth. It was not: .githooks/pre-commit had a bash list with FOUR formats +// this one lacked, and the port took two of them and quietly left the rest. +// One of the four was the JWT rule - and a Supabase service_role key is a JWT, +// so the scanner walked past a non-expiring full-access production database +// credential in a repo that was hours from being published. The rules were not +// wrong. They were incomplete, again, in exactly the way the header of +// check-stageable-secrets.mjs warns about. All four are here now. export const SECRET_PATTERNS = [ + // header.payload.signature, base64url. Supabase, Auth0, Firebase and most + // self-issued session tokens are this shape. describeJwt below turns a hit + // into something a human can triage without ever seeing the token. + [/eyJ[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,}/, 'JWT (Supabase / Auth0 / Firebase style)', describeJwt], [/xfb_[a-f0-9]{32,}/i, 'GroupMind agent key'], [/antfarm_[A-Za-z0-9]{32,}/, 'GroupMind room key'], // sk-ant- BEFORE the general sk- rule: the broad one also matches an @@ -39,6 +59,9 @@ export const SECRET_PATTERNS = [ [/gh[pousr]_[A-Za-z0-9]{36,}/, 'GitHub token'], [/github_pat_[A-Za-z0-9_]{50,}/, 'GitHub fine-grained PAT'], [/xai-[A-Za-z0-9]{20,}/, 'xAI API key'], + [/moltbook_sk_[A-Za-z0-9]{20,}/, 'Moltbook secret key'], + [/\bam_[a-z]{2}_[a-f0-9]{40,}/, 'AgentMail key'], + [/MT[A-Za-z0-9]{22,}\.[A-Za-z0-9_-]{6}\.[A-Za-z0-9_-]{27,}/, 'Discord bot token'], [/-----BEGIN [A-Z ]*PRIVATE KEY-----/, 'private key'], [/\b(?:api[_-]?key|secret|password|token)\s*[:=]\s*['"]?[A-Za-z0-9_\-]{24,}/i, 'assigned secret-looking value'], @@ -54,25 +77,111 @@ export const SKIP_EXT = // silently passed: "too big to check" is not "checked and clean". export const MAX_BYTES = 2 * 1024 * 1024; +// Claims that are safe to print: standard JWT metadata, never the token, never +// the signature, and never a custom claim we have not thought about. An +// allowlist rather than a denylist, because the interesting question - "is this +// a service_role key for production" - is answered by four well-known fields, +// and a custom claim could hold anything. +const JWT_CLAIM_ALLOWLIST = ['alg', 'typ', 'kid', 'iss', 'aud', 'role', 'ref', 'scope', 'iat', 'nbf', 'exp']; +const MAX_CLAIM_CHARS = 64; + /** - * Find the first credential-shaped thing in `text`. + * Decode a JWT's header and payload and describe them. * - * Returns null, or { label, line, length } - deliberately WITHOUT the matched + * Claims yes, token never. The claims are not the secret - they are metadata + * anyone holding the token can read - and they are the whole difference between + * "some JWT" and "a non-expiring service_role key for the production project". + * Reporting them is what makes a hit triageable without anyone pasting the + * credential into a terminal to find out what it is. + * + * Returns null for an eyJ-prefixed string that is not actually a JWT, which is + * a thing that exists: base64 of any JSON object starts "eyJ". + */ +export function describeJwt(token) { + const segments = String(token).split('.'); + if (segments.length < 2) return null; + const decodeSegment = (seg) => { + try { + const json = Buffer.from(seg, 'base64url').toString('utf8'); + const value = JSON.parse(json); + return value && typeof value === 'object' && !Array.isArray(value) ? value : null; + } catch { + return null; + } + }; + const header = decodeSegment(segments[0]); + const payload = decodeSegment(segments[1]); + if (!header && !payload) return null; + + const claims = {}; + for (const source of [header, payload]) { + if (!source) continue; + for (const key of JWT_CLAIM_ALLOWLIST) { + const value = source[key]; + if (value === undefined || value === null) continue; + if (typeof value === 'object') continue; // arrays/objects: not worth dumping + const text = String(value); + claims[key] = text.length > MAX_CLAIM_CHARS + ? `(value omitted: ${text.length} chars)` + : text; + } + } + + const detail = { kind: 'jwt', decoded: true, claims }; + const exp = payload && typeof payload.exp === 'number' ? payload.exp : null; + if (exp !== null) { + const msLeft = exp * 1000 - Date.now(); + detail.expired = msLeft <= 0; + detail.expiresAt = new Date(exp * 1000).toISOString().slice(0, 10); + detail.daysRemaining = Math.round(msLeft / 86400000); + } else { + // No exp at all is worse news than a distant one, not better. + detail.expired = false; + detail.expiresAt = null; + detail.daysRemaining = null; + detail.noExpiry = true; + } + return detail; +} + +/** + * Every credential-shaped thing in `text`, one entry per rule that fires. + * + * Returns [{ label, line, length, detail? }] - deliberately WITHOUT the matched * text. Every caller reports a location and a rule name, so no caller is ever * one console.log away from copying a live key into a public log. + * + * Plural on purpose. It used to return only the first hit, which meant a rule + * high in the list shadowed everything below it in the same blob: a file with + * an API key on line 3 and a service_role JWT on line 40 reported one finding + * and the reader had no idea the second one was there. Silent partial reporting + * is the same family of bug as a silent miss. */ -export function matchSecret(text) { - for (const [re, label] of SECRET_PATTERNS) { +export function matchSecrets(text) { + const hits = []; + // Rule order, so the most specific rule claims a value first. + for (const [re, label, detailOf] of SECRET_PATTERNS) { const m = text.match(re); - if (m) { - return { - label, - line: text.slice(0, m.index).split('\n').length, - length: m[0].length, - }; - } + if (!m) continue; + const start = m.index; + const end = m.index + m[0].length; + // One VALUE reported once. `const apiKey = "sk-..."` matches both the + // OpenAI rule and the generic assigned-secret rule; reporting it twice is + // noise, and noise is how a scanner gets switched off. Two credentials at + // different places in the same blob do not overlap, so both survive. + if (hits.some((h) => start < h.end && end > h.start)) continue; + hits.push({ + label, + line: text.slice(0, start).split('\n').length, + length: m[0].length, + start, + end, + // detailOf sees the matched value; whatever it returns is printed, so it + // must return metadata only. describeJwt returns allowlisted claims. + ...(detailOf ? { detail: detailOf(m[0]) } : {}), + }); } - return null; + return hits.sort((a, b) => a.start - b.start).map(({ start, end, ...hit }) => hit); } /** diff --git a/test/scan-history-for-secrets.test.mjs b/test/scan-history-for-secrets.test.mjs index 890166d..80fde31 100644 --- a/test/scan-history-for-secrets.test.mjs +++ b/test/scan-history-for-secrets.test.mjs @@ -510,3 +510,173 @@ test('isMainModule resolves symlinks on both sides', () => { writeFileSync(importer, `import ${JSON.stringify(link)};\n`); assert.equal(spawnSync('node', [importer], { encoding: 'utf8' }).stdout, 'NOT-MAIN'); }); + +// --------------------------------------------------------------------------- +// JWTs. A Supabase service_role key is a JWT, and this scanner had no JWT rule: +// it reported "clean" on a repo whose only credential was a non-expiring +// full-access production database key. Claims are reported, the token never is. +// --------------------------------------------------------------------------- + +const b64url = (obj) => Buffer.from(JSON.stringify(obj)).toString('base64url'); + +/** + * A synthetic JWT. Nothing is signed: the signature segment is a fixed, + * obviously-fake string, and every ref/sub is a TESTONLY placeholder. Never + * paste a real token into a fixture in a public repo, including one you just + * found somewhere else. + */ +function syntheticJwt(payload, header = { alg: 'HS256', typ: 'JWT' }) { + const signature = ['TESTONLY', 'not', 'a', 'real', 'signature', '0'.repeat(20)].join('-'); + return `${b64url(header)}.${b64url(payload)}.${signature}`; +} + +const LIVE_SERVICE_ROLE = { + iss: 'supabase', + ref: 'TESTONLYPROJECTREF', + role: 'service_role', + iat: 1700000000, + exp: 2085000000, // 2036 +}; + +test('a repo whose ONLY credential is a service_role JWT is FOUND, not clean', () => { + const dir = newRepo('iak-hist-jwt-'); + writeFileSync(path.join(dir, 'test_fetch.js'), + `const SUPABASE_KEY = "${syntheticJwt(LIVE_SERVICE_ROLE)}";\n`); + commitAll(dir, 'add a fetch test'); + + const r = runScanner([dir, '--json']); + assert.equal(r.status, EXIT.FOUND, + `a service_role JWT must not read as clean; got ${r.status}: ${r.stderr}`); + const report = JSON.parse(r.stdout); + assert.equal(report.findings.length, 1); + assert.equal(report.findings[0].path, 'test_fetch.js'); + assert.match(report.findings[0].rule, /JWT/); +}); + +test('JWT claims are reported and the token is not', () => { + const dir = newRepo('iak-hist-jwtclaims-'); + const token = syntheticJwt(LIVE_SERVICE_ROLE); + writeFileSync(path.join(dir, 'client.js'), `const key = "${token}";\n`); + commitAll(dir, 'client'); + + for (const args of [[dir], [dir, '--json']]) { + const r = runScanner(args); + assert.equal(r.status, EXIT.FOUND); + const output = `${r.stdout}\n${r.stderr}`; + + // The claims, which are what make the hit triageable. + assert.ok(output.includes('service_role'), `role claim missing from ${args.join(' ')}`); + assert.ok(output.includes('supabase'), 'iss claim missing'); + assert.ok(output.includes('TESTONLYPROJECTREF'), 'ref claim missing'); + + // The token, which must never appear - whole, prefixed or segmented. + assert.ok(!output.includes(token), 'the token leaked'); + for (let n = 8; n <= token.length; n++) { + assert.ok(!output.includes(token.slice(0, n)), `${n}-char token prefix leaked`); + } + for (const segment of token.split('.')) { + assert.ok(!output.includes(segment), 'a token segment leaked'); + } + } + + const detail = JSON.parse(runScanner([dir, '--json']).stdout).findings[0].detail; + assert.equal(detail.claims.role, 'service_role'); + assert.equal(detail.expired, false); + assert.match(detail.expiresAt, /^\d{4}-\d{2}-\d{2}$/); + assert.ok(detail.daysRemaining > 0); + assert.ok(!JSON.stringify(detail).includes(token.split('.')[2]), 'the signature must not be in the JSON'); +}); + +test('an expired JWT is reported and distinguished from a live one', () => { + const dir = newRepo('iak-hist-jwtexp-'); + writeFileSync(path.join(dir, 'old.js'), + `const stale = "${syntheticJwt({ ...LIVE_SERVICE_ROLE, exp: 1500000000 })}";\n`); // 2017 + writeFileSync(path.join(dir, 'new.js'), + `const live = "${syntheticJwt(LIVE_SERVICE_ROLE)}";\n`); + writeFileSync(path.join(dir, 'forever.js'), + `const forever = "${syntheticJwt({ iss: 'supabase', ref: 'TESTONLYREF2', role: 'anon' })}";\n`); + commitAll(dir, 'three tokens'); + + const report = JSON.parse(runScanner([dir, '--json']).stdout); + const byPath = Object.fromEntries(report.findings.map((f) => [f.path, f.detail])); + assert.equal(byPath['old.js'].expired, true, 'an expired token must be marked expired'); + assert.equal(byPath['new.js'].expired, false); + assert.equal(byPath['forever.js'].noExpiry, true, 'no exp claim is worse news, not better'); + + // All three are still findings: an expired credential is a different + // conversation, not a non-event. + assert.equal(report.findings.length, 3); + const human = runScanner([dir]); + assert.match(human.stderr, /EXPIRED/); + assert.match(human.stderr, /LIVE until/); + assert.match(human.stderr, /NO EXPIRY CLAIM/); +}); + +test('a malformed eyJ-prefixed string does not crash the scanner', () => { + const dir = newRepo('iak-hist-jwtjunk-'); + // base64 of any JSON object starts "eyJ", so eyJ-prefixed non-JWTs exist. + writeFileSync(path.join(dir, 'junk.txt'), [ + `${b64url({ hello: 'world' })}.${'!'.repeat(20)}.${'?'.repeat(20)}`, + `${b64url({ alg: 'HS256' })}.${'Zm9vYmFy'.repeat(3)}x.${'0'.repeat(30)}`, + 'eyJ' + 'A'.repeat(40), + `${b64url({ alg: 'none' })}.${b64url({ exp: 'not-a-number' })}.${'0'.repeat(30)}`, + ].join('\n') + '\n'); + commitAll(dir, 'junk'); + + const r = runScanner([dir, '--json']); + assert.ok([EXIT.CLEAN, EXIT.FOUND].includes(r.status), + `must not crash or report incomplete; got ${r.status}: ${r.stderr}`); + assert.doesNotThrow(() => JSON.parse(r.stdout), 'the JSON report must still be valid'); + assert.equal(JSON.parse(r.stdout).errors.length, 0); +}); + +test('every format the bash pre-commit hook knows is also caught here', () => { + // The JWT miss happened because this list was ported from .githooks/pre-commit + // and the port silently dropped four formats. Capability test, one synthetic + // sample per format, all assembled at runtime so no credential-shaped literal + // sits in this file. + const samples = { + 'GroupMind agent key': 'xfb_' + 'a0'.repeat(20), + 'GroupMind room key': 'antfarm_' + 'T0'.repeat(20), + 'Anthropic API key': ['sk', 'ant', 'TESTONLY' + '0'.repeat(20)].join('-'), + 'OpenAI-style secret key': ['sk', 'TESTONLY' + '0'.repeat(20)].join('-'), + 'Google API key': 'AIza' + 'T0'.repeat(18), + 'GitHub token': 'ghp_' + 'T0'.repeat(20), + 'GitHub fine-grained PAT': 'github_pat_' + 'T0'.repeat(30), + 'xAI API key': 'xai-' + 'T0'.repeat(12), + 'Moltbook secret key': 'moltbook_sk_' + 'T0'.repeat(12), + 'AgentMail key': 'am_' + 'te_' + 'a0'.repeat(22), + 'Discord bot token': 'MT' + 'T0'.repeat(12) + '.' + 'TESTON' + '.' + 'T0'.repeat(15), + 'JWT (Supabase / Auth0 / Firebase style)': syntheticJwt(LIVE_SERVICE_ROLE), + // Split so this file holds no credential-shaped literal, same as the rest: + // the repo's own pre-commit hook blocks the intact header, correctly. + 'private key': '-----BEGIN RSA ' + 'PRIVATE' + ' KEY-----', + }; + + const dir = newRepo('iak-hist-formats-'); + for (const [label, sample] of Object.entries(samples)) { + writeFileSync(path.join(dir, `${label.replace(/[^a-z0-9]+/gi, '-')}.txt`), `value = ${sample}\n`); + } + commitAll(dir, 'one of each'); + + const report = JSON.parse(runScanner([dir, '--json']).stdout); + const found = new Set(report.findings.map((f) => f.rule)); + const missed = Object.keys(samples).filter((label) => !found.has(label)); + assert.deepEqual(missed, [], `formats no rule catches: ${missed.join(', ')}`); +}); + +test('two different credentials in one blob are both reported', () => { + // A rule high in the list used to shadow everything below it in the same + // blob, which is a silent partial miss. + const dir = newRepo('iak-hist-shadow-'); + writeFileSync(path.join(dir, 'both.js'), + `const a = "${['sk', 'TESTONLY', '0'.repeat(16)].join('-')}";\n` + + `const b = "${syntheticJwt(LIVE_SERVICE_ROLE)}";\n`); + commitAll(dir, 'two credentials, one file'); + + const report = JSON.parse(runScanner([dir, '--json']).stdout); + const rules = report.findings.map((f) => f.rule).sort(); + assert.equal(rules.length, 2, `expected both rules to fire, got: ${rules.join(', ')}`); + assert.ok(rules.some((r) => /JWT/.test(r))); + assert.ok(rules.some((r) => /OpenAI/.test(r))); +}); From 3aca2c1bf504830c4de4bbc0d97e0fc52671b1f0 Mon Sep 17 00:00:00 2001 From: Petrus Pennanen Date: Sun, 20 Sep 2026 11:24:14 +0200 Subject: [PATCH 5/7] An encoding problem must never suppress a match Two findings from codexmb's security review of #123, both verified here before being fixed. P1, and it is the bug class this branch exists to remove, reintroduced by this branch's own fix. After the decode-not-sniff change, the stageable gate SKIPPED any file that failed strict UTF-8 decoding. A stageable config.txt holding a plain ASCII credential beside one stray 0xff byte reported PASS and exit 0. The same file through the pre-branch baseline at 9b8aa8c reports FAIL and exit 1. So the encoding check made the gate worse than the code it replaced, and "I could not read these bytes" rendered as "there is nothing here". decodeForScanning() now always returns text: exact when the bytes are valid UTF-8, lossy otherwise, with strict=false to say which. Credential formats are ASCII and a lossy decode preserves ASCII byte for byte, so detection survives. The alternative was to make an undecodable stageable file exit non-zero. Rejected for THIS scanner: its job is to block a commit, not to classify encodings, stageable binaries are routine, and a gate that blocks every commit touching one gets disabled - which is the failure mode the file's own header warns about. It prints a visible NOTE naming the files it had to read lossily, so the limitation is stated rather than swallowed. The history scanner, which reports rather than blocks, now does both: an undecodable blob is scanned lossily AND listed as unexamined, so an ASCII key inside it is found while the verdict still cannot be clean. Those are two different claims and both are true. P2: the extension filter decided a blob's fate from a filename git handed us by chance. rev-list --objects names each blob once, so content committed as both logo.png and notes.txt arrives under whichever path came first, and the .png name excluded readable text from the scan entirely. Collecting every path cannot fix it - git does not give us the others. So the name no longer decides: every blob within the size cap is read, anything that decodes is scanned whatever it is called, and a media extension now only decides the OUTCOME for a blob that did not decode (a real image is skipped quietly instead of being reported as a scanning failure). Counts are unchanged by these fixes: ide-agent-kit 32 findings, groupmind 16, so the false pass is gone without a false positive taking its place. Co-Authored-By: Claude Opus 5 --- scripts/check-stageable-secrets.mjs | 39 ++++++++++--- src/history-scan.mjs | 62 ++++++++++++++------ src/secret-patterns.mjs | 20 +++++++ test/scan-history-for-secrets.test.mjs | 79 ++++++++++++++++++++++++++ 4 files changed, 175 insertions(+), 25 deletions(-) diff --git a/scripts/check-stageable-secrets.mjs b/scripts/check-stageable-secrets.mjs index a73ca18..a7cd576 100755 --- a/scripts/check-stageable-secrets.mjs +++ b/scripts/check-stageable-secrets.mjs @@ -38,7 +38,7 @@ import { execFileSync } from 'node:child_process'; import { readFileSync, statSync } from 'node:fs'; -import { MAX_BYTES, SKIP_EXT, decodeUtf8, matchSecrets, ruleLabels } from '../src/secret-patterns.mjs'; +import { MAX_BYTES, SKIP_EXT, decodeForScanning, matchSecrets, ruleLabels } from '../src/secret-patterns.mjs'; // The pattern list, the binary-extension skip list and the size cap now live // in src/secret-patterns.mjs, shared with scripts/scan-history-for-secrets.mjs. @@ -74,6 +74,11 @@ function stageableFiles() { return files; } +// Files that had to be decoded lossily. Reported at the end: the ASCII scan is +// sound, but "this was not valid UTF-8" is something the operator should see +// rather than something the tool swallows. +const lossyFiles = []; + function scan(path) { if (SKIP_EXT.test(path)) return []; let bytes; @@ -83,12 +88,21 @@ function scan(path) { } catch { return []; // unreadable, gone, or a directory: not our problem } - // Read as bytes and decode strictly, rather than the old "readFileSync utf8 - // then look for a NUL". A NUL is not a proof of binary - bin/iak-pending.mjs - // uses one as a field separator inside 26 kB of valid JavaScript - and the - // lossy utf8 read could not tell a real binary from text anyway. - const text = decodeUtf8(bytes); - if (text === null) return []; // genuinely not text + // Read as bytes and decode for scanning. NOT "decode strictly, else skip": + // that is what this file did for one commit, and it printed PASS on a file + // holding a plain ASCII key beside a single stray 0xff byte - the baseline + // scanner caught that file, so the encoding check made the gate WORSE. An + // encoding problem must never suppress a match. + // + // Lossy is right here specifically. This scanner's job is to block a commit, + // not to classify encodings; credential formats are ASCII and survive a lossy + // decode byte for byte. Making an undecodable file exit non-zero instead was + // the other option, and it was wrong for THIS tool: stageable binaries are + // routine, blocking every commit that touches one gets the hook disabled, and + // a disabled scanner is worse than none. The history scanner, which reports + // rather than blocks, does mark such blobs could-not-complete AND scans them. + const { text, strict } = decodeForScanning(bytes); + if (!strict) lossyFiles.push(path); // Report WHERE and WHICH RULE, never the value itself. This output ends up // in CI logs and terminal scrollback, and a scanner that prints the secret it // found has simply moved the leak. @@ -124,11 +138,22 @@ for (const f of stageableFiles()) { for (const hit of scan(f)) findings.push({ file: f, ...hit }); } +function reportLossy() { + if (lossyFiles.length === 0) return; + console.error(`NOTE: ${lossyFiles.length} stageable file(s) are not valid UTF-8 and were`); + console.error(' scanned as ASCII. Credential shapes are ASCII, so this finds them,'); + console.error(' but text in another encoding would not have been read:'); + for (const f of lossyFiles.slice(0, 10)) console.error(` ${f}`); +} + if (findings.length === 0) { + reportLossy(); console.log('PASS: nothing `git add -A` would stage looks like a credential.'); process.exit(0); } +reportLossy(); + console.error(`FAIL: ${findings.length} stageable file(s) contain credential-shaped data\n`); for (const f of findings) { console.error(` ${f.file}:${f.line}`); diff --git a/src/history-scan.mjs b/src/history-scan.mjs index 707ae57..4fa034b 100644 --- a/src/history-scan.mjs +++ b/src/history-scan.mjs @@ -59,7 +59,7 @@ import path from 'node:path'; import { MAX_BYTES, SKIP_EXT, - decodeUtf8, + decodeForScanning, matchSecrets, ruleLabels, } from '../src/secret-patterns.mjs'; @@ -123,18 +123,28 @@ function makeGit(repo, deadline) { */ function reachableObjects(git) { const out = git(['rev-list', '--objects', '--all']); - const pathBySha = new Map(); + const pathsBySha = new Map(); const order = []; for (const line of out.split('\n')) { if (!line) continue; const sp = line.indexOf(' '); if (sp === -1) continue; // commit or tag object: no path const sha = line.slice(0, sp); - if (pathBySha.has(sha)) continue; - pathBySha.set(sha, line.slice(sp + 1)); - order.push(sha); + const blobPath = line.slice(sp + 1); + // One name per object is ALL git gives us here: rev-list --objects emits a + // blob once, under whichever path it met first. Identical content living at + // both logo.png and notes.txt arrives as a single sha named logo.png, and + // no amount of collecting can recover the other name from this output. + // + // That is why the extension filter below no longer decides anything on its + // own: a name we were handed by chance must not be able to exclude content + // from the scan. The content decides. + if (!pathsBySha.has(sha)) { + pathsBySha.set(sha, blobPath); + order.push(sha); + } } - return { pathBySha, order }; + return { pathsBySha, order }; } /** Split [{sha,size}] into chunks of roughly `budget` bytes for cat-file --batch. */ @@ -197,8 +207,16 @@ function readBatch(git, shas, onBlob) { * skip as an error, which is the worst of both answers. */ function decodeBlob(body) { - const text = decodeUtf8(body); - return text === null ? { reason: 'invalid-utf8' } : { text }; + const { text, strict } = decodeForScanning(body); + // A blob that does not decode is BOTH scanned and reported unexamined. + // + // Those are not contradictory, they are two different claims: the lossy + // decode preserves every ASCII byte, so an ASCII credential is still found - + // refusing to look was how the sibling scanner came to print PASS on a file + // with a key next to one stray 0xff - while "we could not read these bytes as + // text" stays true and keeps the verdict off `clean`. An encoding problem + // must never suppress a match, and it must never be silently forgiven either. + return strict ? { text } : { text, reason: 'invalid-utf8' }; } /** @@ -275,7 +293,7 @@ export function scanRepo(opts) { report.examined.refs = git(['for-each-ref', '--format=%(refname)']) .split('\n').filter(Boolean).length; - const { pathBySha, order } = reachableObjects(git); + const { pathsBySha, order } = reachableObjects(git); // One --batch-check pass gives type and size for everything, so the // expensive --batch read only ever asks for blobs we mean to scan. @@ -285,18 +303,20 @@ export function scanRepo(opts) { if (!line) continue; const [sha, type, sizeStr] = line.split(' '); if (type !== 'blob') continue; - blobs.push({ sha, size: Number(sizeStr) || 0, path: pathBySha.get(sha) ?? '(unknown path)' }); + blobs.push({ sha, size: Number(sizeStr) || 0, path: pathsBySha.get(sha) ?? '(unknown path)' }); } report.examined.blobsReachable = blobs.length; const toRead = []; for (const blob of blobs) { - if (SKIP_EXT.test(blob.path)) { - report.examined.blobsSkippedBinaryMedia += 1; - continue; - } + // A media extension is a hint about what the bytes probably are, not a + // licence to skip reading them. It only decides the OUTCOME for a blob we + // could not decode anyway (a real .png is quietly skipped rather than + // counted as could-not-complete). Text that happens to be named .png is + // read and scanned like anything else. if (blob.size > opts.maxBytes) { - report.unexamined.push({ path: blob.path, blob: blob.sha, reason: 'over-size-cap' }); + if (SKIP_EXT.test(blob.path)) report.examined.blobsSkippedBinaryMedia += 1; + else report.unexamined.push({ path: blob.path, blob: blob.sha, reason: 'over-size-cap' }); continue; } toRead.push(blob); @@ -309,11 +329,17 @@ export function scanRepo(opts) { if (!blob) return; const { text, reason } = decodeBlob(body); if (reason) { + // Undecodable AND named like binary media: an ordinary image, skipped + // by policy and counted, not dressed up as a scanning failure. + if (SKIP_EXT.test(blob.path)) { + report.examined.blobsSkippedBinaryMedia += 1; + return; + } report.unexamined.push({ path: blob.path, blob: sha, reason }); - return; + } else { + report.examined.blobsExamined += 1; + report.examined.bytesExamined += body.length; } - report.examined.blobsExamined += 1; - report.examined.bytesExamined += body.length; const hits = matchSecrets(text); if (hits.length > 0) { // One finding per rule that fired, not just the first: a rule high in diff --git a/src/secret-patterns.mjs b/src/secret-patterns.mjs index ce02291..b326c70 100644 --- a/src/secret-patterns.mjs +++ b/src/secret-patterns.mjs @@ -184,6 +184,26 @@ export function matchSecrets(text) { return hits.sort((a, b) => a.start - b.start).map(({ start, end, ...hit }) => hit); } +/** + * Decode `buf` for SCANNING. Always returns text. + * + * Returns { text, strict }. When the bytes are valid UTF-8, `text` is exact and + * `strict` is true. When they are not, `text` is a lossy decode - every invalid + * byte becomes U+FFFD and every ASCII byte survives untouched - and `strict` is + * false. + * + * Why lossy rather than refusing: credential formats are ASCII, and a lossy + * decode preserves ASCII byte for byte. Refusing the file instead is how the + * stageable gate came to print PASS on a file holding a plain ASCII key next to + * one stray 0xff. An encoding problem must never suppress a match - the caller + * decides separately what a non-strict decode means for its verdict. + */ +export function decodeForScanning(buf) { + const text = decodeUtf8(buf); + if (text !== null) return { text, strict: true }; + return { text: Buffer.from(buf).toString('utf8'), strict: false }; +} + /** * Decode `buf` as text, or return null if it genuinely is not UTF-8. * diff --git a/test/scan-history-for-secrets.test.mjs b/test/scan-history-for-secrets.test.mjs index 80fde31..d841cb7 100644 --- a/test/scan-history-for-secrets.test.mjs +++ b/test/scan-history-for-secrets.test.mjs @@ -680,3 +680,82 @@ test('two different credentials in one blob are both reported', () => { assert.ok(rules.some((r) => /JWT/.test(r))); assert.ok(rules.some((r) => /OpenAI/.test(r))); }); + +// --------------------------------------------------------------------------- +// An encoding problem must never suppress a match. +// +// Regression for a false PASS this branch introduced and codexmb's security +// review of #123 caught: after the decode-not-sniff change, the stageable gate +// SKIPPED any file that failed strict UTF-8 decoding, so a file holding a plain +// ASCII credential beside one stray 0xff byte reported PASS - while the +// pre-branch baseline at 9b8aa8c reported FAIL on the same file. The fix for a +// could-not-check-reads-as-clean bug had reintroduced the same bug one file +// over. +// --------------------------------------------------------------------------- + +test('the stageable gate still finds an ASCII credential beside an invalid byte', () => { + // codexmb's fixture, exactly: config.txt, ASCII dummy credential, one 0xff. + const dir = newRepo('iak-stage-lossy-'); + writeFileSync(path.join(dir, 'config.txt'), Buffer.concat([ + Buffer.from('# notes\n'), + Buffer.from(`api_key = "${synthetic('LOSSY')}"\n`), + Buffer.from([0xff]), + Buffer.from('\ntrailing ascii\n'), + ])); + + const r = spawnSync('node', [stageableScanner], { cwd: dir, encoding: 'utf8', env: GIT_ENV }); + assert.equal(r.status, 1, + 'a stray byte must not hide a credential from the commit gate'); + assert.match(r.stderr, /config\.txt/); + assert.match(r.stderr, /OpenAI-style secret key/); + // The operator is told the decode was lossy rather than it being swallowed. + assert.match(r.stderr, /not valid UTF-8/); + assert.ok(!`${r.stdout}${r.stderr}`.includes('TESTONLY'), 'and still no value printed'); +}); + +test('the history scanner finds an ASCII credential in an undecodable blob AND reports it unexamined', () => { + const dir = newRepo('iak-hist-lossy-'); + writeFileSync(path.join(dir, 'config.dat'), Buffer.concat([ + Buffer.from(`api_key = "${synthetic('LOSSYBLOB')}"\n`), + Buffer.from([0xff, 0xfe]), + ])); + commitAll(dir, 'a credential in bytes that do not decode'); + + const r = runScanner([dir, '--json']); + assert.equal(r.status, EXIT.FOUND, `expected FOUND, got ${r.status}: ${r.stderr}`); + const report = JSON.parse(r.stdout); + // Both claims, and they are not in tension: the ASCII was read, the bytes + // as a whole were not. + assert.equal(report.findings.length, 1, 'the credential must still be found'); + assert.equal(report.findings[0].path, 'config.dat'); + assert.equal(report.unexamined.length, 1, 'and the blob is still not fully examined'); + assert.equal(report.unexamined[0].reason, 'invalid-utf8'); + assert.ok(!`${r.stdout}${r.stderr}`.includes('TESTONLY')); +}); + +test('a media extension cannot hide text content from the scan', () => { + // The filename is whatever git handed us: rev-list --objects names a blob + // once, so identical content committed as both notes.txt and logo.png arrives + // under one name, and the extension filter used to decide the blob's fate + // from that accident. + const dir = newRepo('iak-hist-alias-'); + const body = `key = "${synthetic('ALIAS')}"\n`; + writeFileSync(path.join(dir, 'a-alias.png'), body); // sorts first, wins the name + writeFileSync(path.join(dir, 'z-real.txt'), body); + commitAll(dir, 'same content at two paths'); + + const report = JSON.parse(runScanner([dir, '--json']).stdout); + assert.equal(report.findings.length, 1, 'text content named .png must still be scanned'); + assert.equal(report.examined.blobsSkippedBinaryMedia, 0); + + // ...while a real binary image is still skipped quietly, not reported as a + // scanning failure. Otherwise every repo with an icon exits could-not-complete. + const imageDir = newRepo('iak-hist-realpng-'); + writeFileSync(path.join(imageDir, 'logo.png'), + Buffer.from('89504e470d0a1a0a0000000d49484452ffd8ffe000104a46', 'hex')); + commitAll(imageDir, 'an actual image'); + const imageReport = JSON.parse(runScanner([imageDir, '--json']).stdout); + assert.equal(imageReport.examined.blobsSkippedBinaryMedia, 1); + assert.equal(imageReport.unexamined.length, 0); + assert.equal(imageReport.verdict, 'clean'); +}); From a6fecaca378ce17e7cb8ba8da31a3657330c0e89 Mon Sep 17 00:00:00 2001 From: Petrus Pennanen Date: Sun, 20 Sep 2026 11:47:08 +0200 Subject: [PATCH 6/7] Nothing the scanner does not control may decide what it reports Two findings from codexmb's second review, plus two more of the same idea found by looking for it rather than waiting for a fourth round. P1: a JWT's claims are ATTACKER-CONTROLLED FREE TEXT, and the report printed them. A synthetic secret placed in `iss` came back verbatim in JSON.stringify(matchSecrets(jwt)). Allowlisting claim NAMES constrains nothing about claim VALUES, and this report exists to be shared - pasted into a ticket, handed to a reviewer - so anyone who can get a token into a scanned repo could choose what our output says. A value is now printed only where the value itself is constrained to a vocabulary we defined: alg and typ against the JWT spec's registered names, role against the roles these platforms define, exp/iat/nbf as parsed dates, and ref only when it matches ^[a-z]{20}$. Everything else - iss, kid, aud, scope, sub, azp, jti, and any claim not thought about - is reported as presence and length: "iss=". That is everything triage needs and it cannot carry a payload. The argued exception is ref. It is the Supabase project identifier, it appears in the project's own public URL, it is not a credential, and it answers "which project is this key for" - the difference between an alarming finding and an actionable one. The shape restricts the channel to 20 lowercase letters. Delete SUPABASE_REF_SHAPE and it degrades to presence-and-length like the rest. P2, which my previous round did not fix: the same blob reachable as both a.png and z.txt, containing an ASCII credential plus one 0xff byte, still exited 0 and clean. I had moved WHERE the filename decided, not WHETHER it decided - the media check simply ran later, on the representative path, and skipped the blob before matching. The oversize policy was representative-dependent too. No filename now takes part in any decision. Binary media is recognised by magic bytes, so text called logo.png is scanned and a PNG called notes.txt is skipped. An oversize blob is unexamined whatever it is called. The same rule is applied to check-stageable-secrets.mjs, which had the identical extension skip. Two more instances of the same idea, found by going looking: - A PATH is attacker-controlled free text that both scanners print. It can carry ANSI escapes or a carriage return to forge report lines, and it can BE a credential (keys/sk-live-xxx.txt), which printing would leak. Paths are now sanitized, and a path that matches a secret pattern is withheld like any other value. - "PASS: no credential-shaped data in 0 reachable blob(s)" was printable. Every reachable blob must now land in exactly one bucket - examined, recognised media, or unexamined - and a mismatch is could-not-complete. The human report says NOTHING SCANNED when it read nothing. Counts hold: ide-agent-kit 32 findings, groupmind 16. Co-Authored-By: Claude Opus 5 --- scripts/check-stageable-secrets.mjs | 29 +++-- scripts/scan-history-for-secrets.mjs | 12 +- src/history-scan.mjs | 71 ++++++++--- src/secret-patterns.mjs | 166 +++++++++++++++++++++---- test/scan-history-for-secrets.test.mjs | 147 +++++++++++++++++++++- 5 files changed, 371 insertions(+), 54 deletions(-) diff --git a/scripts/check-stageable-secrets.mjs b/scripts/check-stageable-secrets.mjs index a7cd576..4fd2187 100755 --- a/scripts/check-stageable-secrets.mjs +++ b/scripts/check-stageable-secrets.mjs @@ -38,7 +38,7 @@ import { execFileSync } from 'node:child_process'; import { readFileSync, statSync } from 'node:fs'; -import { MAX_BYTES, SKIP_EXT, decodeForScanning, matchSecrets, ruleLabels } from '../src/secret-patterns.mjs'; +import { MAX_BYTES, decodeForScanning, looksLikeBinaryMedia, matchSecrets, renderClaims, ruleLabels, sanitizeForOutput } from '../src/secret-patterns.mjs'; // The pattern list, the binary-extension skip list and the size cap now live // in src/secret-patterns.mjs, shared with scripts/scan-history-for-secrets.mjs. @@ -79,8 +79,19 @@ function stageableFiles() { // rather than something the tool swallows. const lossyFiles = []; +// A path is repo-controlled free text that this tool prints. It can carry ANSI +// escapes or a newline to forge output lines, and it can itself be a credential +// (keys/sk-live-xxx.txt), which printing would leak. +function safeFile(file) { + const hits = matchSecrets(file); + if (hits.length > 0) return `(path withheld: it matches ${hits[0].label})`; + return sanitizeForOutput(file); +} + function scan(path) { - if (SKIP_EXT.test(path)) return []; + // No extension check. The same filename-decides bug lives here: a stageable + // foo.png holding an ASCII credential would have been skipped unread. Binary + // media is recognised below, by its bytes. let bytes; try { if (statSync(path).size > MAX_BYTES) return []; @@ -102,7 +113,11 @@ function scan(path) { // a disabled scanner is worse than none. The history scanner, which reports // rather than blocks, does mark such blobs could-not-complete AND scans them. const { text, strict } = decodeForScanning(bytes); - if (!strict) lossyFiles.push(path); + if (!strict) { + // Recognised image or archive: nothing to read, and nothing to warn about. + if (looksLikeBinaryMedia(bytes)) return []; + lossyFiles.push(path); + } // Report WHERE and WHICH RULE, never the value itself. This output ends up // in CI logs and terminal scrollback, and a scanner that prints the secret it // found has simply moved the leak. @@ -118,12 +133,12 @@ function scan(path) { // the history scanner so a finding reads the same wherever it surfaces. function describeDetail(detail) { if (!detail || detail.kind !== 'jwt') return ''; - const claims = Object.entries(detail.claims).map(([k, v]) => `${k}=${v}`).join(' '); + const claims = renderClaims(detail.claims); let expiry; if (detail.noExpiry) expiry = 'NO EXPIRY CLAIM'; else if (detail.expired) expiry = `EXPIRED ${detail.expiresAt}`; else expiry = `live until ${detail.expiresAt} (${detail.daysRemaining} days)`; - return `claims: ${claims || '(none readable)'} | ${expiry}`; + return `claims: ${claims} | ${expiry}`; } // Shared with the history scanner; a test compares the two outputs so the @@ -143,7 +158,7 @@ function reportLossy() { console.error(`NOTE: ${lossyFiles.length} stageable file(s) are not valid UTF-8 and were`); console.error(' scanned as ASCII. Credential shapes are ASCII, so this finds them,'); console.error(' but text in another encoding would not have been read:'); - for (const f of lossyFiles.slice(0, 10)) console.error(` ${f}`); + for (const f of lossyFiles.slice(0, 10)) console.error(` ${safeFile(f)}`); } if (findings.length === 0) { @@ -156,7 +171,7 @@ reportLossy(); console.error(`FAIL: ${findings.length} stageable file(s) contain credential-shaped data\n`); for (const f of findings) { - console.error(` ${f.file}:${f.line}`); + console.error(` ${safeFile(f.file)}:${f.line}`); console.error(` ${f.label} - ${f.hint}`); if (f.detail) console.error(` ${describeDetail(f.detail)}`); console.error(''); diff --git a/scripts/scan-history-for-secrets.mjs b/scripts/scan-history-for-secrets.mjs index 93a3c9b..ea147c2 100755 --- a/scripts/scan-history-for-secrets.mjs +++ b/scripts/scan-history-for-secrets.mjs @@ -24,7 +24,7 @@ // Run: node scripts/scan-history-for-secrets.mjs [repo-path] [--json] // Help: node scripts/scan-history-for-secrets.mjs --help -import { MAX_BYTES, ruleLabels } from '../src/secret-patterns.mjs'; +import { MAX_BYTES, renderClaims, ruleLabels } from '../src/secret-patterns.mjs'; import { EXIT, scanRepo } from '../src/history-scan.mjs'; const DEFAULT_MAX_SECONDS = 600; @@ -129,6 +129,10 @@ function printHuman(report) { if (e.blobsSkippedBinaryMedia > 0) { out(`skipped: ${e.blobsSkippedBinaryMedia} binary media blob(s) by extension - NOT examined`); } + if (e.blobsExamined === 0 && e.blobsReachable > 0) { + out('NOTHING SCANNED: 0 of the reachable blobs were read as text. Whatever this'); + out(' report says, it is not based on having looked at the content.'); + } if (e.commits <= 1) { out('DEPTH: this history is 1 commit. A history scan of a fresh snapshot'); out(' repo proves almost nothing - it has no deleted past to hide a key in.'); @@ -152,8 +156,10 @@ function printHuman(report) { // non-expiring full-access production database credential", and it is // metadata, not the secret. if (f.detail && f.detail.kind === 'jwt') { - const claims = Object.entries(f.detail.claims).map(([k, v]) => `${k}=${v}`).join(' '); - out(` claims: ${claims || '(none readable)'}`); + // renderClaims prints a value only where the value is constrained to a + // vocabulary we defined; everything else is presence and length. A + // JWT's claims are free text chosen by whoever made the token. + out(` claims: ${renderClaims(f.detail.claims)}`); if (f.detail.noExpiry) out(' expiry: NO EXPIRY CLAIM - this token does not stop working'); else if (f.detail.expired) out(` expiry: EXPIRED ${f.detail.expiresAt}`); else out(` expiry: LIVE until ${f.detail.expiresAt} (${f.detail.daysRemaining} days remaining)`); diff --git a/src/history-scan.mjs b/src/history-scan.mjs index 4fa034b..c56fe78 100644 --- a/src/history-scan.mjs +++ b/src/history-scan.mjs @@ -58,11 +58,11 @@ import { existsSync } from 'node:fs'; import path from 'node:path'; import { MAX_BYTES, - SKIP_EXT, decodeForScanning, + looksLikeBinaryMedia, matchSecrets, - ruleLabels, -} from '../src/secret-patterns.mjs'; + sanitizeForOutput, +} from './secret-patterns.mjs'; // Documented in --help. Verdict precedence when several apply: // FOUND > INCOMPLETE > SHALLOW > CLEAN. Anything that is not a proven-clean @@ -206,6 +206,21 @@ function readBatch(git, shas, onBlob) { * source that could carry a key - refusing it skips the scan AND mislabels the * skip as an error, which is the worst of both answers. */ +/** + * A repo-controlled path, made safe to print. + * + * A path is attacker-controlled free text that this report prints verbatim. It + * can carry ANSI escapes to repaint the output or a newline to forge an extra + * finding line, and it can BE a credential - keys/sk-live-xxxx.txt is a path, + * and printing it would leak the very thing we refuse to print from inside the + * file. Same rule either way: say what is there, never its content. + */ +export function safeReportPath(blobPath) { + const hits = matchSecrets(blobPath); + if (hits.length > 0) return `(path withheld: it matches ${hits[0].label})`; + return sanitizeForOutput(blobPath); +} + function decodeBlob(body) { const { text, strict } = decodeForScanning(body); // A blob that does not decode is BOTH scanned and reported unexamined. @@ -309,14 +324,12 @@ export function scanRepo(opts) { const toRead = []; for (const blob of blobs) { - // A media extension is a hint about what the bytes probably are, not a - // licence to skip reading them. It only decides the OUTCOME for a blob we - // could not decode anyway (a real .png is quietly skipped rather than - // counted as could-not-complete). Text that happens to be named .png is - // read and scanned like anything else. + // No filename appears in this decision, at any stage. A blob over the cap + // is unexamined, full stop - we did not read it, and the name it happens + // to carry is not evidence about its contents. Raise --max-bytes to scan + // it rather than letting an extension vouch for it. if (blob.size > opts.maxBytes) { - if (SKIP_EXT.test(blob.path)) report.examined.blobsSkippedBinaryMedia += 1; - else report.unexamined.push({ path: blob.path, blob: blob.sha, reason: 'over-size-cap' }); + report.unexamined.push({ path: safeReportPath(blob.path), blob: blob.sha, reason: 'over-size-cap' }); continue; } toRead.push(blob); @@ -329,13 +342,17 @@ export function scanRepo(opts) { if (!blob) return; const { text, reason } = decodeBlob(body); if (reason) { - // Undecodable AND named like binary media: an ordinary image, skipped - // by policy and counted, not dressed up as a scanning failure. - if (SKIP_EXT.test(blob.path)) { + // Undecodable, and the BYTES are a format we recognise: an ordinary + // image, skipped by policy and counted, not dressed up as a scanning + // failure. Recognition is by magic number, so a text file called + // logo.png is still scanned and a PNG called notes.txt is still + // skipped. Anything we do not recognise is lossy-scanned below and + // reported unexamined. + if (looksLikeBinaryMedia(body)) { report.examined.blobsSkippedBinaryMedia += 1; return; } - report.unexamined.push({ path: blob.path, blob: sha, reason }); + report.unexamined.push({ path: safeReportPath(blob.path), blob: sha, reason }); } else { report.examined.blobsExamined += 1; report.examined.bytesExamined += body.length; @@ -345,13 +362,18 @@ export function scanRepo(opts) { // One finding per rule that fired, not just the first: a rule high in // the list used to shadow everything below it in the same blob. const commit = introducingCommit(git, sha); + // The PATH is repo-controlled free text and it is printed. It can + // carry ANSI escapes or a newline to forge report lines, and it can + // BE a credential (keys/sk-live-....txt). Same rule as everywhere + // else: report that there is something there, never its content. + const safePath = safeReportPath(blob.path); for (const hit of hits) { // label + line + length only, plus safe metadata for rules that // can produce it (JWT claims). The value stays in the repo, which // is the one place it is already. report.findings.push({ rule: hit.label, - path: blob.path, + path: safePath, line: hit.line, blob: sha, commit, @@ -362,12 +384,29 @@ export function scanRepo(opts) { }); } } catch (err) { - report.errors.push(String(err && err.message ? err.message : err)); + // git's stderr can quote repo-controlled text (ref names, paths), so it + // gets the same treatment as everything else that reaches the report. + report.errors.push(sanitizeForOutput(String(err && err.message ? err.message : err), 500)); } report.examined.blobsUnexamined = report.unexamined.length; report.durationMs = deadline.elapsedMs(); + // Accounting invariant: every reachable blob must end up in exactly one + // bucket - examined, skipped as recognised media, or unexamined. If the + // numbers do not add up, a blob fell out of the scan without anyone deciding + // that it should, and the report is describing a repo we did not fully walk. + // That is could-not-complete, not clean. (The motivating output was "PASS: no + // credential-shaped data in 0 reachable blob(s)", printed while a blob sat + // unscanned - a sentence that should never have been printable.) + const accounted = report.examined.blobsExamined + + report.examined.blobsSkippedBinaryMedia + + report.unexamined.length; + if (accounted < report.examined.blobsReachable) { + report.errors.push( + `only ${accounted} of ${report.examined.blobsReachable} reachable blobs are accounted for`); + } + if (report.findings.length > 0) { report.verdict = 'found'; report.exitCode = EXIT.FOUND; diff --git a/src/secret-patterns.mjs b/src/secret-patterns.mjs index b326c70..395f555 100644 --- a/src/secret-patterns.mjs +++ b/src/secret-patterns.mjs @@ -67,32 +67,103 @@ export const SECRET_PATTERNS = [ 'assigned secret-looking value'], ]; -// Binaries and media produce noise, not credentials. A blob skipped by this -// rule is NOT examined - both scanners report the count so a reader can tell -// "we looked at everything" from "we looked at everything we could read". -export const SKIP_EXT = - /\.(png|jpe?g|gif|webp|ico|pdf|zip|gz|tgz|jar|aab|apk|keystore|jks|woff2?|ttf|mp[34]|mov|wav)$/i; +// Binary media used to be recognised by file extension here. That decided +// whether content got scanned from a name git handed us by chance, which let a +// credential hide in a blob whose first-seen path ended .png. Recognition is +// now by content: looksLikeBinaryMedia() below. // Above this, a blob is not scanned. It is reported as unexamined rather than // silently passed: "too big to check" is not "checked and clean". export const MAX_BYTES = 2 * 1024 * 1024; -// Claims that are safe to print: standard JWT metadata, never the token, never -// the signature, and never a custom claim we have not thought about. An -// allowlist rather than a denylist, because the interesting question - "is this -// a service_role key for production" - is answered by four well-known fields, -// and a custom claim could hold anything. -const JWT_CLAIM_ALLOWLIST = ['alg', 'typ', 'kid', 'iss', 'aud', 'role', 'ref', 'scope', 'iat', 'nbf', 'exp']; -const MAX_CLAIM_CHARS = 64; +// WHAT MAY BE PRINTED FROM A JWT, AND WHY SO LITTLE. +// +// A JWT's claims are ATTACKER-CONTROLLED FREE TEXT. Anyone who can get a token +// into a scanned repo chooses what our report says, and this report exists to +// be shared: pasted into a ticket, handed to a reviewer, dropped in a room. An +// earlier version allowlisted claim NAMES and printed whatever string sat under +// them, under 65 characters. A reviewer put a synthetic secret in `iss` and it +// came back verbatim. An allowlist of names does not constrain values. +// +// So a value is printed only where the value itself is constrained to a +// vocabulary we defined: +// +// alg, typ the JWT spec's own registered names +// role the handful of roles these platforms define +// exp/iat/nbf a number, rendered as a date +// ref ONLY when it is exactly 20 lowercase letters +// +// Everything else - iss, kid, aud, scope, and any claim we have not thought +// about - is reported as presence and length. "iss present, 42 chars" tells a +// human this token names an issuer and roughly how long it is, which is all +// triage needs, and it cannot carry a payload. +// +// The one argued exception is `ref`. It is the Supabase project identifier, +// it appears in the project's own public URL, it is not a credential, and it +// is the single field that answers "which project is this key for" - the +// difference between an alarming finding and an actionable one. It is printed +// only when it matches ^[a-z]{20}$ exactly, so the channel is 20 lowercase +// letters wide and carries nothing an attacker did not already have to encode +// into that shape. If that trade is not wanted, delete SUPABASE_REF_SHAPE and +// it degrades to presence-and-length like the rest. +const JWT_ALG_VOCABULARY = new Set([ + 'HS256', 'HS384', 'HS512', 'RS256', 'RS384', 'RS512', + 'ES256', 'ES256K', 'ES384', 'ES512', 'PS256', 'PS384', 'PS512', + 'EdDSA', 'none', +]); +const JWT_TYP_VOCABULARY = new Set(['JWT', 'at+jwt', 'at+JWT', 'JOSE', 'JOSE+JSON', 'dpop+jwt']); +const JWT_ROLE_VOCABULARY = new Set([ + 'service_role', 'anon', 'authenticated', 'authenticator', + 'supabase_admin', 'admin', 'user', 'owner', 'editor', 'viewer', 'guest', +]); +const SUPABASE_REF_SHAPE = /^[a-z]{20}$/; +const JWT_TIMESTAMP_CLAIMS = ['exp', 'iat', 'nbf']; +// Reported by presence and length only. Listing them explicitly documents the +// decision; anything NOT in any list is treated the same way by default, which +// is the safe direction. +const JWT_OPAQUE_CLAIMS = ['kid', 'iss', 'aud', 'scope', 'sub', 'azp', 'jti']; + +/** A value we chose to print, or a shape that carries nothing. */ +const printable = (value) => ({ value: String(value) }); +const opaque = (value) => ({ present: true, chars: String(value).length }); + +function describeClaim(name, value) { + if (typeof value === 'object') return null; // arrays/objects: never dumped + const text = String(value); + if (name === 'alg') return JWT_ALG_VOCABULARY.has(text) ? printable(text) : opaque(text); + if (name === 'typ') return JWT_TYP_VOCABULARY.has(text) ? printable(text) : opaque(text); + if (name === 'role') return JWT_ROLE_VOCABULARY.has(text) ? printable(text) : opaque(text); + if (name === 'ref') return SUPABASE_REF_SHAPE.test(text) ? printable(text) : opaque(text); + if (JWT_TIMESTAMP_CLAIMS.includes(name)) { + return typeof value === 'number' && Number.isFinite(value) + ? printable(new Date(value * 1000).toISOString().slice(0, 10)) + : opaque(text); + } + return opaque(text); +} + +const JWT_REPORTED_CLAIMS = [ + 'alg', 'typ', 'role', 'ref', ...JWT_TIMESTAMP_CLAIMS, ...JWT_OPAQUE_CLAIMS, +]; + +/** + * Render one claims map as a single line for humans. Shared so a finding reads + * identically wherever it surfaces. + */ +export function renderClaims(claims) { + const parts = Object.entries(claims).map(([name, claim]) => ( + claim.value !== undefined ? `${name}=${claim.value}` : `${name}=` + )); + return parts.join(' ') || '(none readable)'; +} /** * Decode a JWT's header and payload and describe them. * - * Claims yes, token never. The claims are not the secret - they are metadata - * anyone holding the token can read - and they are the whole difference between - * "some JWT" and "a non-expiring service_role key for the production project". - * Reporting them is what makes a hit triageable without anyone pasting the - * credential into a terminal to find out what it is. + * Claims yes, token never - and only the constrained parts of the claims, see + * the note above. The point is a report that is safe to share: it says what the + * token IS (a service_role key for project x, live until 2036) without + * republishing anything the token's author chose to write. * * Returns null for an eyJ-prefixed string that is not actually a JWT, which is * a thing that exists: base64 of any JSON object starts "eyJ". @@ -116,14 +187,11 @@ export function describeJwt(token) { const claims = {}; for (const source of [header, payload]) { if (!source) continue; - for (const key of JWT_CLAIM_ALLOWLIST) { - const value = source[key]; + for (const name of JWT_REPORTED_CLAIMS) { + const value = source[name]; if (value === undefined || value === null) continue; - if (typeof value === 'object') continue; // arrays/objects: not worth dumping - const text = String(value); - claims[key] = text.length > MAX_CLAIM_CHARS - ? `(value omitted: ${text.length} chars)` - : text; + const described = describeClaim(name, value); + if (described) claims[name] = described; } } @@ -144,6 +212,56 @@ export function describeJwt(token) { return detail; } +// Binary media recognised by CONTENT, never by filename. +// +// The filename is whatever git handed us - rev-list --objects names a blob once, +// under whichever path it met first - so a name must not decide whether content +// gets scanned. These are magic bytes: if we RECOGNISE the format we can skip it +// quietly, and if we do not, it is unexamined and says so. +const BINARY_MEDIA_MAGIC = [ + [0x89, 0x50, 0x4e, 0x47], // PNG + [0xff, 0xd8, 0xff], // JPEG + [0x47, 0x49, 0x46, 0x38], // GIF8 + [0x25, 0x50, 0x44, 0x46], // %PDF + [0x50, 0x4b, 0x03, 0x04], // ZIP / JAR / APK / AAB / docx + [0x50, 0x4b, 0x05, 0x06], // empty ZIP + [0x1f, 0x8b], // gzip / tgz + [0x77, 0x4f, 0x46, 0x46], // wOFF + [0x77, 0x4f, 0x46, 0x32], // wOF2 + [0x00, 0x01, 0x00, 0x00], // TTF + [0x4f, 0x54, 0x54, 0x4f], // OTTO + [0x00, 0x00, 0x01, 0x00], // ICO + [0x52, 0x49, 0x46, 0x46], // RIFF (wav/webp/avi) + [0x49, 0x44, 0x33], // ID3 (mp3) + [0xff, 0xfb], // mp3 frame + [0x66, 0x4c, 0x61, 0x43], // fLaC +]; + +/** True when the bytes ARE a recognised binary media format. */ +export function looksLikeBinaryMedia(buf) { + if (buf.length >= 12) { + // ftyp box at offset 4: mp4 / mov / m4a + if (buf.toString('latin1', 4, 8) === 'ftyp') return true; + } + return BINARY_MEDIA_MAGIC.some((magic) => + magic.length <= buf.length && magic.every((byte, i) => buf[i] === byte)); +} + +/** + * Make a repo-controlled string safe to print. + * + * File paths are attacker-controlled free text too, and they end up in terminal + * output: a path can carry ANSI escapes to repaint the report, a newline to + * forge an extra finding line, or a carriage return to overwrite the verdict. + * Control characters are replaced rather than dropped, so the presence of + * something odd is visible instead of silently swallowed, and the result is + * capped so one absurd path cannot flood a log. + */ +export function sanitizeForOutput(text, maxChars = 300) { + const cleaned = String(text).replace(/[\u0000-\u001f\u007f-\u009f]/g, '?'); + return cleaned.length > maxChars ? `${cleaned.slice(0, maxChars)}...(${cleaned.length} chars)` : cleaned; +} + /** * Every credential-shaped thing in `text`, one entry per rule that fires. * diff --git a/test/scan-history-for-secrets.test.mjs b/test/scan-history-for-secrets.test.mjs index d841cb7..ba3cdef 100644 --- a/test/scan-history-for-secrets.test.mjs +++ b/test/scan-history-for-secrets.test.mjs @@ -564,10 +564,17 @@ test('JWT claims are reported and the token is not', () => { assert.equal(r.status, EXIT.FOUND); const output = `${r.stdout}\n${r.stderr}`; - // The claims, which are what make the hit triageable. + // The claims that are constrained to a vocabulary we defined are printed... assert.ok(output.includes('service_role'), `role claim missing from ${args.join(' ')}`); - assert.ok(output.includes('supabase'), 'iss claim missing'); - assert.ok(output.includes('TESTONLYPROJECTREF'), 'ref claim missing'); + assert.ok(output.includes('2036-'), 'exp claim missing (as a date)'); + // ...and the free-text ones are reported as presence and length instead, + // because their contents are chosen by whoever minted the token. The human + // report renders that as iss=; the JSON carries the same + // thing structurally. + assert.ok(/iss= { } const detail = JSON.parse(runScanner([dir, '--json']).stdout).findings[0].detail; - assert.equal(detail.claims.role, 'service_role'); + assert.equal(detail.claims.role.value, 'service_role'); + assert.equal(detail.claims.iss.value, undefined, 'a free-text claim carries no value'); + assert.equal(detail.claims.iss.chars, 'supabase'.length); assert.equal(detail.expired, false); assert.match(detail.expiresAt, /^\d{4}-\d{2}-\d{2}$/); assert.ok(detail.daysRemaining > 0); @@ -758,4 +767,134 @@ test('a media extension cannot hide text content from the scan', () => { assert.equal(imageReport.examined.blobsSkippedBinaryMedia, 1); assert.equal(imageReport.unexamined.length, 0); assert.equal(imageReport.verdict, 'clean'); + assert.equal(imageReport.errors.length, 0, 'a recognised image is accounted for, not lost'); + // ...but the human output still says plainly that nothing was read. + const imageHuman = runScanner([imageDir]); + assert.match(imageHuman.stdout, /NOTHING SCANNED/); +}); + +// --------------------------------------------------------------------------- +// Attacker-controlled input must not choose what the report says. +// +// Three rounds of review on this PR found three instances of one idea: +// something the scanner does not control deciding what the scanner reports. A +// JWT's claims are free text chosen by whoever minted the token; a blob's +// filename is chosen by whoever committed it. Both reach the output. +// --------------------------------------------------------------------------- + +test('a secret planted in ANY emitted claim does not reach the output', () => { + // The reviewer's finding: an allowlist of claim NAMES does not constrain + // claim VALUES. Plant the same synthetic secret in every claim the scanner + // is willing to name, then grep all three outputs for it. + const planted = synthetic('INCLAIMS'); + const header = { alg: planted, typ: planted, kid: planted }; + const payload = { + iss: planted, aud: planted, scope: planted, sub: planted, azp: planted, + jti: planted, role: planted, ref: planted, + exp: planted, iat: planted, nbf: planted, + }; + const dir = newRepo('iak-hist-claimleak-'); + writeFileSync(path.join(dir, 'token.js'), `const t = "${syntheticJwt(payload, header)}";\n`); + commitAll(dir, 'a token whose claims are hostile'); + + for (const args of [[dir], [dir, '--json']]) { + const r = runScanner(args); + assert.equal(r.status, EXIT.FOUND, `expected FOUND for ${args.join(' ')}`); + const output = `${r.stdout}\n${r.stderr}`; + assert.ok(!output.includes(planted), `a planted claim value reached ${args.join(' ')}`); + for (let n = 8; n <= planted.length; n++) { + assert.ok(!output.includes(planted.slice(0, n)), `${n}-char prefix of a claim value leaked`); + } + assert.ok(!output.includes('TESTONLY'), 'a distinctive fragment of a claim value leaked'); + // It still reports that the claims are there, which is the triage value. + assert.ok(/present,\s*\d+\s*chars|"present"/.test(output), 'presence and length must still be reported'); + } +}); + +test('an unrecognised role or alg is reported by shape, not by value', () => { + const dir = newRepo('iak-hist-vocab-'); + const odd = 'role-' + 'x'.repeat(30); + writeFileSync(path.join(dir, 'odd.js'), + `const t = "${syntheticJwt({ role: odd, iss: 'x' }, { alg: odd, typ: 'JWT' })}";\n`); + commitAll(dir, 'claims outside every vocabulary'); + + const r = runScanner([dir, '--json']); + const claims = JSON.parse(r.stdout).findings[0].detail.claims; + assert.equal(claims.role.value, undefined, 'an unknown role value must not be printed'); + assert.equal(claims.role.chars, odd.length); + assert.equal(claims.alg.value, undefined, 'an alg outside the JWT vocabulary must not be printed'); + assert.ok(!`${r.stdout}${r.stderr}`.includes(odd)); +}); + +test('the same blob named .png and .txt, undecodable, is still FOUND', () => { + // The combined case: the earlier fix moved WHERE the filename decided, not + // WHETHER it decided. Content that does not strictly decode AND whose + // representative path is a media name was skipped unread, so a repo holding a + // credential exited 0 / clean. + const dir = newRepo('iak-hist-combined-'); + const body = Buffer.concat([ + Buffer.from(`api_key = "${synthetic('COMBINED')}"\n`), + Buffer.from([0xff]), + ]); + writeFileSync(path.join(dir, 'a-alias.png'), body); // sorts first, wins the name + writeFileSync(path.join(dir, 'z-real.txt'), body); + commitAll(dir, 'same blob, two names, one invalid byte'); + + const r = runScanner([dir, '--json']); + assert.equal(r.status, EXIT.FOUND, + `a media-looking name must not hide undecodable text; got ${r.status}: ${r.stderr}`); + const report = JSON.parse(r.stdout); + assert.equal(report.findings.length, 1); + assert.equal(report.unexamined.length, 1, 'and it is still reported as not fully examined'); + assert.equal(report.examined.blobsSkippedBinaryMedia, 0, + 'nothing here is recognised binary media'); +}); + +test('an oversize blob is unexamined whatever it is called', () => { + const dir = newRepo('iak-hist-oversize-'); + writeFileSync(path.join(dir, 'big.png'), 'x'.repeat(4096)); + commitAll(dir, 'a large blob with a media name'); + + const report = JSON.parse(runScanner([dir, '--max-bytes=1024', '--json']).stdout); + assert.equal(report.unexamined.length, 1, + 'over the cap means unread, and a filename is not evidence about contents'); + assert.equal(report.unexamined[0].reason, 'over-size-cap'); + assert.equal(report.examined.blobsSkippedBinaryMedia, 0); +}); + +test('every reachable blob is accounted for in exactly one bucket', () => { + const dir = newRepo('iak-hist-accounting-'); + writeFileSync(path.join(dir, 'text.js'), 'const ok = true;\n'); + writeFileSync(path.join(dir, 'image.png'), Buffer.from('89504e470d0a1a0a0000000d49484452', 'hex')); + writeFileSync(path.join(dir, 'weird.dat'), Buffer.from([0xff, 0xfe, 0x41])); + commitAll(dir, 'one of each bucket'); + + const report = JSON.parse(runScanner([dir, '--json']).stdout); + const e = report.examined; + assert.equal(e.blobsExamined + e.blobsSkippedBinaryMedia + report.unexamined.length, + e.blobsReachable, 'a blob that is in no bucket has silently left the scan'); + assert.ok(!report.errors.some((err) => /accounted for/.test(err))); +}); + +test('a path that is itself a credential is withheld, and control characters are neutralised', () => { + const dir = newRepo('iak-hist-paths-'); + // A path can BE a credential, and printing it would leak exactly what we + // refuse to print from inside a file. + const namedSecret = `${synthetic('INPATH')}.txt`; + writeFileSync(path.join(dir, namedSecret), 'nothing secret inside\n'); + // ...and a path can carry escapes that repaint a terminal report. + const forging = `notes${String.fromCharCode(27)}[2K${String.fromCharCode(13)}PASS.txt`; + writeFileSync(path.join(dir, forging), `key = "${synthetic('FORGE')}"\n`); + commitAll(dir, 'hostile filenames'); + + const r = runScanner([dir, '--json']); + const output = `${r.stdout}\n${r.stderr}`; + assert.ok(!output.includes('TESTONLY-INPATH'), 'a credential in a PATH must not be printed'); + assert.ok(!output.includes(String.fromCharCode(27)), 'no escape character may reach the output'); + assert.ok(!output.includes(String.fromCharCode(13)), 'no carriage return may reach the output'); + + const report = JSON.parse(r.stdout); + const forged = report.findings.find((f) => /notes/.test(f.path)); + assert.ok(forged, 'the finding in the oddly-named file is still reported'); + assert.match(forged.path, /notes\?\[2K\?PASS\.txt/, 'control characters are shown as ?, not dropped'); }); From 6e804789d3c7fe268ad7d9ccaeeb5d9f92571615 Mon Sep 17 00:00:00 2001 From: Petrus Pennanen Date: Sun, 20 Sep 2026 11:50:51 +0200 Subject: [PATCH 7/7] Say what the entry-point sweep checks, and route two files through it The sweep test's failure message asserted a defect it had not established. After the rebase it fired on bin/iak-pending.mjs and bin/model-picker.mjs from #122 and #119 and told their authors the files "no-op through a symlink". They do not. Both use the correct realpath idiom, and both were measured behaving identically through a symlink: model-picker direct EXIT=0 563 bytes symlink EXIT=0 563 bytes iak-pending direct EXIT=0 3985 bytes symlink EXIT=0 3985 bytes The check matches process.argv[1] and import.meta.url within 200 characters of each other, which the CORRECT idiom does too. So it enforces "use the shared helper", which is a reasonable policy, and it cannot tell a correct copy from a broken one, so it must not claim to. This is the day's pattern in its mild form: the check fires on the right policy while claiming the wrong fact. It fails loudly rather than passing silently, but a false alarm that overstates its finding erodes trust in the tool exactly like a false pass does, and it is how a useful test gets deleted by the next person who hits it. The message now says what it knows: these hand-roll the comparison instead of calling isMainModule(), which is a drift risk rather than a bug, and a hand-rolled copy may well be correct today. It adds what to look for if the copy is NOT realpathing both sides, without asserting that it is not. The behaviour is proven separately by the isMainModule test and the two symlink parity tests. Then the policy is satisfied rather than argued with: both files call isMainModule(). One implementation is the reason src/common/entrypoint.mjs exists, and this repo got the idiom wrong three times in three files in one day. Their output is byte-identical before and after, direct and through a symlink. Co-Authored-By: Claude Opus 5 --- bin/iak-pending.mjs | 9 ++++--- bin/model-picker.mjs | 19 +++++++------- test/scan-history-for-secrets.test.mjs | 34 ++++++++++++++++++-------- 3 files changed, 39 insertions(+), 23 deletions(-) diff --git a/bin/iak-pending.mjs b/bin/iak-pending.mjs index aa8cedd..94e9384 100755 --- a/bin/iak-pending.mjs +++ b/bin/iak-pending.mjs @@ -42,7 +42,7 @@ // only to hosts that helper already trusts. It is never printed, never logged // and never placed in argv. -import { readFileSync, writeFileSync, renameSync, unlinkSync, mkdirSync, realpathSync } from 'node:fs'; +import { readFileSync, writeFileSync, renameSync, unlinkSync, mkdirSync } from 'node:fs'; import { homedir } from 'node:os'; import { dirname, join } from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -50,6 +50,7 @@ import { fileURLToPath } from 'node:url'; import { loadConfig } from '../src/config.mjs'; import { gateAuthHeadersFor } from '../src/mcp-server.mjs'; import { lanIpReason } from '../packages/user-intent-kit/src/model-capacity.js'; +import { isMainModule } from '../src/common/entrypoint.mjs'; const ROOT = join(dirname(fileURLToPath(import.meta.url)), '..'); const DEFAULT_TIMEOUT_MS = 5000; @@ -773,9 +774,11 @@ export async function main(argv = process.argv.slice(2), { env = process.env, ou return code; } +// The comparison itself lives in src/common/entrypoint.mjs. It is correct here +// too, but one implementation is the point: the idiom has been got wrong three +// times in this repo, and a copy that is right today is a copy that can drift. function invokedDirectly() { - try { return !!process.argv[1] && realpathSync(process.argv[1]) === fileURLToPath(import.meta.url); } - catch { return false; } + return isMainModule(import.meta.url); } if (invokedDirectly()) { diff --git a/bin/model-picker.mjs b/bin/model-picker.mjs index ea4e3ea..8e987fa 100755 --- a/bin/model-picker.mjs +++ b/bin/model-picker.mjs @@ -104,7 +104,6 @@ import { parseArgs } from 'node:util'; import { homedir } from 'node:os'; import { join, dirname } from 'node:path'; import { fileURLToPath } from 'node:url'; -import { realpathSync } from 'node:fs'; import { readFile, writeFile, mkdir, rename } from 'node:fs/promises'; import { @@ -118,6 +117,7 @@ import { // That was fixed once already in PR #52; hand-rolling around the helper that // exists to prevent it would re-open it. import { gateAuthHeadersFor } from '../src/mcp-server.mjs'; +import { isMainModule } from '../src/common/entrypoint.mjs'; const ROOT = join(dirname(fileURLToPath(import.meta.url)), '..'); @@ -752,15 +752,14 @@ export async function main(argv = process.argv.slice(2), { } } -// Only when run directly. realpathSync on both sides because import.meta.url -// is already realpath-resolved and process.argv[1] is not: a `~/bin` symlink -// (the documented way to put this on PATH) or macOS /tmp (itself a symlink) -// made the comparison false, so main() never ran and the program exited 0 - -// a code this file documents as "a selection was applied". -const invokedDirectly = (() => { - try { return !!process.argv[1] && realpathSync(process.argv[1]) === fileURLToPath(import.meta.url); } - catch { return false; } -})(); +// Only when run directly. The comparison lives in src/common/entrypoint.mjs, +// which realpaths both sides because import.meta.url is already resolved and +// process.argv[1] is not: a `~/bin` symlink (the documented way to put this on +// PATH) or macOS /tmp (itself a symlink) made a naive comparison false, so +// main() never ran and the program exited 0 - a code this file documents as +// "a selection was applied". One implementation, because this repo has got the +// idiom wrong three times in three different files. +const invokedDirectly = isMainModule(import.meta.url); if (invokedDirectly) { process.exit(await main()); } diff --git a/test/scan-history-for-secrets.test.mjs b/test/scan-history-for-secrets.test.mjs index ba3cdef..cc0d7e7 100644 --- a/test/scan-history-for-secrets.test.mjs +++ b/test/scan-history-for-secrets.test.mjs @@ -443,17 +443,25 @@ test('the pre-commit secret gate fires through a symlinked path', () => { assert.ok(!`${linked.stdout}${linked.stderr}`.includes('TESTONLY'), 'and it still prints no value'); }); -test('no entry point hand-rolls the main-module comparison', () => { - // We found this bug three times in one day by tripping over instances one at - // a time. This is the sweep, kept. +test('every entry point uses the shared isMainModule(), not its own comparison', () => { + // POLICY, NOT DEFECT. This asserts that the comparison exists in one place. + // It does NOT establish that a file matching here is broken: the correct + // realpath idiom mentions process.argv[1] and import.meta.url in the same + // breath, exactly as the broken ones do, so this check cannot tell them + // apart and must not claim to. // - // The rule is PROXIMITY, not "the file mentions isMainModule somewhere": a - // first version of this test only checked the latter, and it passed happily - // when the guard was reverted to the broken comparison while the (now unused) - // import stayed behind. A check that cannot fail is not a check. + // An earlier version's failure message said the matched files "no-op through + // a symlink". Two files from other PRs matched, both used the correct idiom, + // and both were measured behaving identically through a symlink - so the + // message told their authors their working code was broken. That is how a + // useful test gets deleted by the next person who hits it. A false alarm that + // overstates its finding erodes trust the same way a false pass does. // - // The correct idiom never names process.argv[1] at the call site - the only - // place that does is the shared helper. + // What makes the policy worth enforcing anyway: this repo got the idiom wrong + // three times in three files in one day, and a copy that is correct today is + // a copy that can drift tomorrow. The BEHAVIOUR is proven separately, by + // 'isMainModule resolves symlinks on both sides' and by the two symlink + // parity tests above. const helper = path.join('src', 'common', 'entrypoint.mjs'); // the one implementation const offenders = []; for (const dir of ['bin', 'scripts', 'src']) { @@ -480,7 +488,13 @@ test('no entry point hand-rolls the main-module comparison', () => { } } assert.deepEqual(offenders, [], - `these compare argv[1] against import.meta.url themselves, so they no-op through a symlink: ${offenders.join(', ')}`); + 'these compare process.argv[1] against import.meta.url themselves instead of calling ' + + `isMainModule() from ${helper}: ${offenders.join(', ')}. ` + + 'That is a drift risk, NOT a finding that they are broken - a hand-rolled copy may well ' + + 'be correct today. Route it through the shared helper so there is one implementation to ' + + 'keep correct. (If the copy is NOT realpathing both sides, it is also an active bug: ' + + 'import.meta.url is realpath-resolved and process.argv[1] is not, so it silently no-ops ' + + 'through a symlink, and macOS /tmp is one.)'); }); test('the scanner CLI has no main-module guard at all', () => {