diff --git a/scripts/privacy-scan.ts b/scripts/privacy-scan.ts index 8b0e2dd6cd..f8b4ef69c4 100644 --- a/scripts/privacy-scan.ts +++ b/scripts/privacy-scan.ts @@ -175,6 +175,30 @@ function isAllowedBearerToken(file: string, token: string): boolean { return /^(?:access|stack|usage-debug)-token(?:-value)?-[A-Za-z0-9-]+$/.test(token); } +/** + * Placeholder endpoints that are documentation, not infrastructure. + * + * Deliberately narrow: RFC 2606 reserved names, an obviously templated value, and + * SSH's own `%h`/`%p` tokens. Anything else naming a host or an account is treated + * as real, because the cost of a false positive here is one allowlist line and the + * cost of a false negative is a published endpoint. + */ +function isAllowedSshEndpoint(value: string): boolean { + const v = value.trim(); + if (!v) return true; + // A bare substitution token is a template. A real command that merely CONTAINS + // `%h` is not — `ProxyCommand /opt/homebrew/bin/cloudflared access ssh --hostname %h` + // names the binary, the access method and the tunnel, which is the leak itself. + if (/^%[hpr]$/.test(v)) return true; + // ``, `$HOST`, `{{ runner }}` — templated rather than literal. + if (/^[<{$]/.test(v)) return true; + // RFC 2606 / RFC 6761 reserved documentation names. + if (/(?:^|[.@\s])(?:example\.(?:com|net|org)|example|invalid|localhost|test)(?:$|[\s:/])/i.test(v)) return true; + // Generic account placeholders, matching the home-path allowlist's spirit. + if (/^(?:user|username|me|you|someone|root|ubuntu|runner)$/i.test(v)) return true; + return false; +} + function addFindingsForPattern( findings: Finding[], file: string, @@ -197,9 +221,14 @@ function addFindingsForPattern( /** * Scan already-read text. * - * Split out of `scanFile` so a test can exercise the REAL detectors. This module runs its - * scan on import, so a test that cannot call a function ends up re-declaring the patterns - * instead — and then stays green even if a detector here is deleted. + * Split out of `scanFile` so a test can exercise the REAL detectors rather than + * re-declaring the patterns — a copied regex stays green after the production + * detector is deleted, which is the failure this seam exists to prevent. + * + * Importing this module is side-effect free: the repo scan runs only under + * `import.meta.main` (see `runScan` below). It used to run at module scope, so + * importing `scanText` triggered a full scan and a failing one called + * `process.exit(1)` in the importing process. */ export function scanText(file: string, text: string): Finding[] { const findings: Finding[] = []; @@ -237,6 +266,45 @@ export function scanText(file: string, text: string): Finding[] { /\b(?:sk-[A-Za-z0-9_-]{20,}|ghp_[A-Za-z0-9_]{20,}|eyJ[A-Za-z0-9_-]{20,}\.[A-Za-z0-9_-]{20,})\b/g, match => isAllowedTokenLooking(file, match[0]), ); + /* + * SSH config directives naming a real endpoint. + * + * `privacy-scan` knew about tokens, emails and home paths, but nothing about + * infrastructure — so a devlog could publish a working `Host` block and this + * scan passed. That is how a runner's hostname, login and Cloudflare + * `ProxyCommand` shipped in `260731_pr_merge_round/022`; #4623 removes them by + * hand. The values are deliberately not repeated here — this file is the fix, + * and restating them would outlive the cleanup. + * + * Anchored to the SSH config grammar — directive at the start of a line, with + * optional indent — because `User` is an ordinary English word and matching it + * in prose would make this unusable. `HostName`/`ProxyCommand` are distinctive + * enough on their own but are anchored the same way for consistency. + */ + addFindingsForPattern( + findings, + file, + text, + "ssh-endpoint", + // `HostName` only, and the value must be the whole rest of the line. + // + // `User` is deliberately NOT matched. It is an ordinary English word, and + // anchoring it to the SSH grammar still fires on wrapped prose — "…the\nuser + // configuration." and "…the\nuser notice." both matched a line-anchored + // single-token form during development. The username alone is also the least + // sensitive part of a Host block, and `MAINTAINER_HOME_USERNAME` already + // covers the maintainer's account in path form. + /^[ \t]*HostName[ \t]+(\S+)[ \t]*$/gim, + match => isAllowedSshEndpoint(match[1] ?? ""), + ); + addFindingsForPattern( + findings, + file, + text, + "ssh-proxy-command", + /^[ \t]*ProxyCommand[ \t]+(\S.*)$/gim, + match => isAllowedSshEndpoint(match[1] ?? ""), + ); /* * Meta Model API keys. The pattern above does not match them: the measured shape is * `LLM|<16 digits>|<27 chars>`, verified against a real key's grammar (never its value). @@ -263,26 +331,57 @@ function scanFile(file: string): Finding[] { * A home path or an email is context a reviewer needs in the failure message. A bearer * token or an API key is the very thing the scan exists to keep out of a readable * artifact, so the report names where it is instead of what it is. + * + * `ssh-proxy-command` is redacted for the same reason: the value carries the binary + * path, the access method and the tunnel options, and CI logs are far more widely + * readable than the diff it was caught in. `ssh-endpoint` (a bare `HostName`) is + * not — that one is the context a reviewer needs to find it. + */ +const REDACTED_FINDING_KINDS = new Set([ + "bearer-token", + "token-looking", + "meta-api-key", + "ssh-proxy-command", + // Redacted for the same reason as the ProxyCommand: this scan runs in CI on a + // public repository, so printing the value would republish the endpoint into a + // public log — the scanner leaking what it was written to catch. `file:line` + // already locates it for whoever has to remove it. + "ssh-endpoint", +]); + +/** + * Run the scan only when invoked as a script. + * + * Previously this ran at module scope, so `import { scanText }` executed a full + * repo scan as a side effect — and a failing scan called `process.exit(1)`, + * taking the importing test process with it. That coupling is invisible while + * the tree is clean and bites the moment a detector finds something: adding the + * `ssh-endpoint` rule below broke `privacy-scan-meta-key.test.ts`, which does + * nothing but import the same seam this file exports for testing. */ -const REDACTED_FINDING_KINDS = new Set(["bearer-token", "token-looking", "meta-api-key"]); +if (import.meta.main) { + runScan(); +} -const findings = gitLsFiles() - .filter(existsSync) - .filter(shouldScan) - .flatMap(scanFile); +function runScan(): void { + const findings = gitLsFiles() + .filter(existsSync) + .filter(shouldScan) + .flatMap(scanFile); -if (findings.length > 0) { - console.error("Privacy scan failed:"); - for (const finding of findings) { - // A credential finding must not be echoed: this output goes to stderr and into CI - // logs, so printing the match would copy a leaked secret from one place it should - // not be into another — and CI logs are far more widely readable than a diff. - // The location and kind are enough to find it; the value is one `git show` away - // for whoever is fixing it. - const shown = REDACTED_FINDING_KINDS.has(finding.kind) ? "" : finding.value; - console.error(`${finding.file}:${finding.line} ${finding.kind}: ${shown}`); + if (findings.length > 0) { + console.error("Privacy scan failed:"); + for (const finding of findings) { + // A credential finding must not be echoed: this output goes to stderr and into CI + // logs, so printing the match would copy a leaked secret from one place it should + // not be into another — and CI logs are far more widely readable than a diff. + // The location and kind are enough to find it; the value is one `git show` away + // for whoever is fixing it. + const shown = REDACTED_FINDING_KINDS.has(finding.kind) ? "" : finding.value; + console.error(`${finding.file}:${finding.line} ${finding.kind}: ${shown}`); + } + process.exit(1); } - process.exit(1); -} -console.log("Privacy scan passed"); + console.log("Privacy scan passed"); +} diff --git a/tests/ci-workflows/privacy-scan-ssh-endpoint.test.ts b/tests/ci-workflows/privacy-scan-ssh-endpoint.test.ts new file mode 100644 index 0000000000..fc4eb334d4 --- /dev/null +++ b/tests/ci-workflows/privacy-scan-ssh-endpoint.test.ts @@ -0,0 +1,60 @@ +import { describe, expect, test } from "bun:test"; +import { scanText } from "../../scripts/privacy-scan"; + +/** + * #4623 removed a working SSH `Host` block from a published devlog by hand. + * `privacy:scan` passed on that file, because it knew about tokens, emails and + * home paths but nothing about infrastructure endpoints. These pin the detector + * that closes it — and, just as importantly, the shapes it must NOT fire on, + * since two rounds of false positives on ordinary prose and code are what + * narrowed it to `HostName`/`ProxyCommand`. + */ +describe("privacy-scan — ssh-endpoint", () => { + const kinds = (text: string) => scanText("devlog/x.md", text).map(f => f.kind); + + test("catches a Host block of the shape that shipped", () => { + // Shaped like the block #4623 is removing, with a synthetic endpoint. Using + // the real one would reintroduce it here permanently and undo that cleanup; + // the regex cannot tell the difference, so there is nothing to be gained. + const block = [ + "Host runner-cf", + " HostName ssh-runner.internal-buildfarm.net", + " ProxyCommand /opt/homebrew/bin/cloudflared access ssh --hostname %h", + ].join("\n"); + const k = kinds(block); + expect(k).toContain("ssh-endpoint"); // redacted in the report; file:line locates it + expect(k).toContain("ssh-proxy-command"); // redacted too, see REDACTED_FINDING_KINDS + }); + + test("a templated or reserved host is documentation, not infrastructure", () => { + for (const line of [ + " HostName example.com", + " HostName ", + " HostName $RUNNER_HOST", + " HostName localhost", + " ProxyCommand %h", + ]) { + expect(kinds(line)).not.toContain("ssh-endpoint"); + expect(kinds(line)).not.toContain("ssh-proxy-command"); + } + }); + + test("does not fire on prose or code that merely starts with a directive word", () => { + for (const line of [ + "User aliases are display metadata only. Codex pool aliases live on `CodexAccount`", + "user configuration.", + "user notice.", + " hostname === undefined ? { grokHome } : { grokHome, hostname },", + "The hostname is resolved by the adapter.", + ]) { + expect(kinds(line)).not.toContain("ssh-endpoint"); + } + }); + + test("a ProxyCommand that merely contains %h is still the real command", () => { + // The substitution token does not make the binary path, the access method or + // the tunnel any less of a leak. + expect(kinds(" ProxyCommand /opt/homebrew/bin/cloudflared access ssh --hostname %h")) + .toContain("ssh-proxy-command"); + }); +});