From 79296b3cd59ca50e1015c65a77e308ef11fa229c Mon Sep 17 00:00:00 2001 From: Abhishek Sharma Date: Tue, 15 Sep 2026 17:03:22 -0700 Subject: [PATCH 1/3] fix(privacy): detect SSH endpoints, and stop scanning on import MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit privacy:scan knew about tokens, emails and home paths, and nothing about infrastructure. A working Host block was therefore publishable: the scan passes on dev today, where devlog/_fin/260731_pr_merge_round/022_remote_test_offload.md still carries a real HostName, account and Cloudflare ProxyCommand. #4623 removes them by hand; nothing stops the next devlog reintroducing them. Two detectors, both anchored to the SSH config grammar: HostName value must be the whole rest of the line ProxyCommand command line, matched separately `User` is deliberately not matched. It is an ordinary English word and even line-anchored it fires on wrapped prose — "…the\nuser configuration." and "…the\nuser notice." both matched during development, as did `hostname === undefined ? ...` in a test file before the value was anchored. The username is also the least sensitive part of a Host block, and MAINTAINER_HOME_USERNAME already covers it in path form. A ProxyCommand containing %h is NOT allowlisted. Only a bare %h is. The substitution token does not make the binary path, the access method or the tunnel any less of a leak — allowing it would have passed the exact line this exists to catch. Also moves the repo scan behind import.meta.main. It ran at module scope, so `import { scanText }` executed a full scan as a side effect and a failing scan called process.exit(1), killing the importing test process. Invisible while the tree is clean; adding the detector above broke privacy-scan-meta-key.test.ts, which does nothing but import the seam this file exports for testing. Refs #4623 --- scripts/privacy-scan.ts | 78 +++++++++++++++++++ .../privacy-scan-ssh-endpoint.test.ts | 54 +++++++++++++ 2 files changed, 132 insertions(+) create mode 100644 tests/ci-workflows/privacy-scan-ssh-endpoint.test.ts diff --git a/scripts/privacy-scan.ts b/scripts/privacy-scan.ts index 8b0e2dd6cd..c4ca0045dc 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, @@ -237,6 +261,44 @@ 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 `ssh-macmini.lidgeai.com`, `User junny` and the + * Cloudflare `ProxyCommand` shipped in `260731_pr_merge_round/022` and had to + * be removed by hand in #4623. + * + * 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-endpoint", + /^[ \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). @@ -266,6 +328,21 @@ function scanFile(file: string): Finding[] { */ const REDACTED_FINDING_KINDS = new Set(["bearer-token", "token-looking", "meta-api-key"]); +/** + * 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. + */ +if (import.meta.main) { + runScan(); +} + +function runScan(): void { const findings = gitLsFiles() .filter(existsSync) .filter(shouldScan) @@ -286,3 +363,4 @@ if (findings.length > 0) { } 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..4641cc6707 --- /dev/null +++ b/tests/ci-workflows/privacy-scan-ssh-endpoint.test.ts @@ -0,0 +1,54 @@ +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 the block that actually shipped", () => { + const block = [ + "Host macmini-cf", + " HostName ssh-macmini.lidgeai.com", + " ProxyCommand /opt/homebrew/bin/cloudflared access ssh --hostname %h", + ].join("\n"); + expect(kinds(block).filter(k => k === "ssh-endpoint")).toHaveLength(2); + }); + + 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"); + } + }); + + 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-endpoint"); + }); +}); From 9e17142b40e9988c4be9a108751acb61ba7a040c Mon Sep 17 00:00:00 2001 From: Abhishek Sharma Date: Tue, 15 Sep 2026 17:43:10 -0700 Subject: [PATCH 2/3] fix(privacy): redact the ProxyCommand value, and correct the scanText doc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups from @lidge-jun on #4734. The scanText JSDoc still claimed the module runs its scan on import — the thing this PR removed. Left as-is, the next contributor writes against a side effect that no longer exists. Now states the import is side-effect free and why the seam exists at all. ProxyCommand findings move to their own kind and join REDACTED_FINDING_KINDS. The value carries the binary path, the access method and the tunnel options, and it was being echoed verbatim into stderr and CI logs — which are far more widely readable than the diff it was caught in. A bare HostName stays visible: that one is the context a reviewer needs to find the line. ...:44 ssh-endpoint: HostName ssh-macmini.lidgeai.com ...:46 ssh-proxy-command: Also indents the runScan body a level, which the extraction had left flat. --- scripts/privacy-scan.ts | 59 ++++++++++++------- .../privacy-scan-ssh-endpoint.test.ts | 7 ++- 2 files changed, 42 insertions(+), 24 deletions(-) diff --git a/scripts/privacy-scan.ts b/scripts/privacy-scan.ts index c4ca0045dc..034262e8cd 100644 --- a/scripts/privacy-scan.ts +++ b/scripts/privacy-scan.ts @@ -221,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[] = []; @@ -295,7 +300,7 @@ export function scanText(file: string, text: string): Finding[] { findings, file, text, - "ssh-endpoint", + "ssh-proxy-command", /^[ \t]*ProxyCommand[ \t]+(\S.*)$/gim, match => isAllowedSshEndpoint(match[1] ?? ""), ); @@ -325,8 +330,18 @@ 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"]); +const REDACTED_FINDING_KINDS = new Set([ + "bearer-token", + "token-looking", + "meta-api-key", + "ssh-proxy-command", +]); /** * Run the scan only when invoked as a script. @@ -343,24 +358,24 @@ if (import.meta.main) { } function runScan(): void { -const findings = gitLsFiles() - .filter(existsSync) - .filter(shouldScan) - .flatMap(scanFile); + 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 index 4641cc6707..bf1628c89e 100644 --- a/tests/ci-workflows/privacy-scan-ssh-endpoint.test.ts +++ b/tests/ci-workflows/privacy-scan-ssh-endpoint.test.ts @@ -18,7 +18,9 @@ describe("privacy-scan — ssh-endpoint", () => { " HostName ssh-macmini.lidgeai.com", " ProxyCommand /opt/homebrew/bin/cloudflared access ssh --hostname %h", ].join("\n"); - expect(kinds(block).filter(k => k === "ssh-endpoint")).toHaveLength(2); + const k = kinds(block); + expect(k).toContain("ssh-endpoint"); // HostName — shown, it locates the leak + expect(k).toContain("ssh-proxy-command"); // redacted in the report, see REDACTED_FINDING_KINDS }); test("a templated or reserved host is documentation, not infrastructure", () => { @@ -30,6 +32,7 @@ describe("privacy-scan — ssh-endpoint", () => { " ProxyCommand %h", ]) { expect(kinds(line)).not.toContain("ssh-endpoint"); + expect(kinds(line)).not.toContain("ssh-proxy-command"); } }); @@ -49,6 +52,6 @@ describe("privacy-scan — ssh-endpoint", () => { // 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-endpoint"); + .toContain("ssh-proxy-command"); }); }); From 16fcef0de8f77c86c8647eccf81caa0b2e96b423 Mon Sep 17 00:00:00 2001 From: Abhishek Sharma Date: Tue, 15 Sep 2026 18:59:42 -0700 Subject: [PATCH 3/3] fix(privacy): redact the endpoint too, and stop the fixture pinning a real host MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two problems with this PR as it stood, both the same shape as the leak it exists to catch. The report printed the `ssh-endpoint` value while redacting the `ProxyCommand`. This scan runs in CI on a public repository, so a finding would have republished the endpoint into a public log — the scanner leaking what it was written to detect. `file:line` already locates it for whoever removes it, which is what the ProxyCommand kind has relied on all along. The regression fixture and the rationale comment both spelled out the real hostname and login. #4623 removes those from the devlog; keeping them here would have undone that cleanup and made this file their permanent home. The fixture now uses a synthetic endpoint — the regex cannot tell the difference — and the comment describes the incident without restating the values. Co-authored-by: Abhishek Sharma --- scripts/privacy-scan.ts | 12 +++++++++--- .../ci-workflows/privacy-scan-ssh-endpoint.test.ts | 13 ++++++++----- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/scripts/privacy-scan.ts b/scripts/privacy-scan.ts index 034262e8cd..f8b4ef69c4 100644 --- a/scripts/privacy-scan.ts +++ b/scripts/privacy-scan.ts @@ -271,9 +271,10 @@ export function scanText(file: string, text: string): Finding[] { * * `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 `ssh-macmini.lidgeai.com`, `User junny` and the - * Cloudflare `ProxyCommand` shipped in `260731_pr_merge_round/022` and had to - * be removed by hand in #4623. + * 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 @@ -341,6 +342,11 @@ const REDACTED_FINDING_KINDS = new Set([ "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", ]); /** diff --git a/tests/ci-workflows/privacy-scan-ssh-endpoint.test.ts b/tests/ci-workflows/privacy-scan-ssh-endpoint.test.ts index bf1628c89e..fc4eb334d4 100644 --- a/tests/ci-workflows/privacy-scan-ssh-endpoint.test.ts +++ b/tests/ci-workflows/privacy-scan-ssh-endpoint.test.ts @@ -12,15 +12,18 @@ import { scanText } from "../../scripts/privacy-scan"; describe("privacy-scan — ssh-endpoint", () => { const kinds = (text: string) => scanText("devlog/x.md", text).map(f => f.kind); - test("catches the block that actually shipped", () => { + 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 macmini-cf", - " HostName ssh-macmini.lidgeai.com", + "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"); // HostName — shown, it locates the leak - expect(k).toContain("ssh-proxy-command"); // redacted in the report, see REDACTED_FINDING_KINDS + 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", () => {