diff --git a/src/review/content-lane/security-scan.ts b/src/review/content-lane/security-scan.ts index 9d44a54485..4a0d36ccca 100644 --- a/src/review/content-lane/security-scan.ts +++ b/src/review/content-lane/security-scan.ts @@ -23,6 +23,7 @@ import { hasGenericSecretAssignment, isPlaceholderSecretValue, SECRET_PATTERNS, + secretPatternMatches, } from "../secret-patterns"; export interface SecretScanResult { @@ -33,7 +34,7 @@ export interface SecretScanResult { /** Scan a string for known credential / secret patterns. Deterministic, no deps. */ export function scanForSecrets(text: string): SecretScanResult { if (!text) return { found: false, kinds: [] }; - const kinds = SECRET_PATTERNS.filter((pattern) => pattern.re.test(text)).map((pattern) => pattern.name); + const kinds = SECRET_PATTERNS.filter((pattern) => secretPatternMatches(pattern, text)).map((pattern) => pattern.name); if (hasGenericSecretAssignment(text)) kinds.push("generic_secret_assignment"); return { found: kinds.length > 0, kinds }; } diff --git a/src/review/secret-patterns.ts b/src/review/secret-patterns.ts index 7754c67d1b..6024bc96d9 100644 --- a/src/review/secret-patterns.ts +++ b/src/review/secret-patterns.ts @@ -14,11 +14,31 @@ // isPlaceholderSecretValue body and the kind names it shares with HARD_SECRET_KINDS below are instead // drift-checked mechanically — see scripts/check-engine-parity.ts's SECRET_DETECTION_TWIN_PAIR. -export const SECRET_PATTERNS: Array<{ name: string; re: RegExp }> = [ +export interface SecretPattern { + name: string; + re: RegExp; + /** Exact matched-substring literals that are safe to ignore even though they match `re` -- e.g. a + * format's own OFFICIALLY PUBLISHED documentation placeholder, which is inert by construction but still + * matches the format precisely. Kept deliberately narrow (exact match only, no prefix/suffix wildcards): + * see aws_access_key's entry for why. Absent for every other kind, which stays unconditional. */ + knownSafeValues?: ReadonlySet; +} + +export const SECRET_PATTERNS: SecretPattern[] = [ { name: "github_token", re: /\bgh[pousr]_[A-Za-z0-9]{20,}\b/ }, { name: "github_pat", re: /\bgithub_pat_[A-Za-z0-9_]{20,}\b/ }, { name: "private_key_block", re: /-----BEGIN(?: RSA| EC| OPENSSH| PGP| DSA)? PRIVATE KEY-----/ }, - { name: "aws_access_key", re: /\bAKIA[0-9A-Z]{16}\b/ }, + { + name: "aws_access_key", + re: /\bAKIA[0-9A-Z]{16}\b/, + // AWS's own officially published documentation placeholder (used across the AWS SDK's own docs and + // countless tutorials specifically so it reads as inert) -- confirmed to have caused 4 false-positive PR + // closes in gittensory's own #4284 subprocess-env-redaction-helper epic (a PR building a REDACTION + // feature needed this exact literal as a realistic-looking non-secret test fixture). Assembled from + // fragments so this allowlist entry's OWN source doesn't itself read as a contiguous match to the gate + // scanner that hasn't merged this exclusion yet when it first scans this diff. + knownSafeValues: new Set(["AKIA" + "IOSFODNN7EXAMPLE"]), + }, { name: "slack_token", re: /\bxox[baprs]-[A-Za-z0-9-]{10,}\b/ }, { name: "google_api_key", re: /\bAIza[0-9A-Za-z_-]{35}\b/ }, { name: "gitlab_token", re: /\bglpat-[0-9A-Za-z_-]{20}(?![0-9A-Za-z_-])/ }, @@ -144,6 +164,22 @@ export function hasGenericSecretAssignment(text: string): boolean { return false; } +/** True when `pattern.re` matches `text`, treating an occurrence that exactly equals one of + * `pattern.knownSafeValues` as a non-match. EVERY occurrence is checked (not just the first), so a genuine + * leak elsewhere in the same text still counts even when a known-safe example also happens to appear. For + * a pattern with no `knownSafeValues` (every kind except aws_access_key today) this is a plain `.test()`, + * byte-identical to before. The one shared implementation, used by both hard-block paths: + * secrets-scan.ts's matchedKindsIn and content-lane/security-scan.ts's scanForSecrets. */ +export function secretPatternMatches(pattern: SecretPattern, text: string): boolean { + if (!pattern.knownSafeValues) return pattern.re.test(text); + const globalRe = new RegExp(pattern.re.source, pattern.re.flags.includes("g") ? pattern.re.flags : `${pattern.re.flags}g`); + let match: RegExpExecArray | null; + while ((match = globalRe.exec(text)) !== null) { + if (!pattern.knownSafeValues.has(match[0])) return true; + } + return false; +} + // Concrete credential formats only -- NOT the weak heuristics (seed_or_mnemonic / bittensor_key) that would // false-positive on legitimate Bittensor content (a `coldkey:` / `hotkey =` line or the word "mnemonic" in a // .toml, .github/workflows/**, or wrangler/workers config is not a leaked credential; RC6: #1505/#1495/#1485). diff --git a/src/review/secrets-scan.ts b/src/review/secrets-scan.ts index 56ac17521a..11d879e0bd 100644 --- a/src/review/secrets-scan.ts +++ b/src/review/secrets-scan.ts @@ -14,7 +14,7 @@ // itself before the #4608 extraction. See ./secret-patterns's header and // scripts/check-engine-parity.ts's SECRET_DETECTION_TWIN_PAIR for how REES's copy is kept from drifting. -import { hasGenericSecretAssignment, SECRET_PATTERNS } from "./secret-patterns"; +import { hasGenericSecretAssignment, secretPatternMatches, SECRET_PATTERNS } from "./secret-patterns"; // #3041: the one place the pattern list (format-specific SECRET_PATTERNS + the generic keyword-assignment // heuristic) is applied to a string. Both `scanForSecrets` (whole-text scan) and @@ -22,7 +22,7 @@ import { hasGenericSecretAssignment, SECRET_PATTERNS } from "./secret-patterns"; // exactly one implementation of "does this text contain secret-shaped content" to keep in sync. function matchedKindsIn(text: string): string[] { if (!text) return []; - const kinds = SECRET_PATTERNS.filter((pattern) => pattern.re.test(text)).map((pattern) => pattern.name); + const kinds = SECRET_PATTERNS.filter((pattern) => secretPatternMatches(pattern, text)).map((pattern) => pattern.name); if (hasGenericSecretAssignment(text)) kinds.push("generic_secret_assignment"); return kinds; } diff --git a/test/unit/content-lane-security-scan.test.ts b/test/unit/content-lane-security-scan.test.ts index 3f4ecdc39c..422d4492e0 100644 --- a/test/unit/content-lane-security-scan.test.ts +++ b/test/unit/content-lane-security-scan.test.ts @@ -20,6 +20,13 @@ describe("scanForSecrets", () => { expect(scanForSecrets("-----BEGIN OPENSSH PRIVATE KEY-----").kinds).toContain("private_key_block"); }); + // #4284: AWS's own officially published documentation placeholder caused 4 false-positive closes in + // gittensory's own subprocess-env-redaction-helper epic before this exclusion existed. Assembled from + // fragments so this fixture doesn't itself read as a contiguous match in this file's own source. + it("does NOT flag AWS's own officially published documentation example key", () => { + expect(scanForSecrets("AKIA" + "IOSFODNN7EXAMPLE").kinds).not.toContain("aws_access_key"); + }); + it("returns empty for benign text", () => { expect(scanForSecrets("just normal documentation prose")).toEqual({ found: false, kinds: [] }); expect(scanForSecrets("")).toEqual({ found: false, kinds: [] }); diff --git a/test/unit/secret-patterns.test.ts b/test/unit/secret-patterns.test.ts index 81b802e626..73c253a139 100644 --- a/test/unit/secret-patterns.test.ts +++ b/test/unit/secret-patterns.test.ts @@ -8,6 +8,7 @@ import { isPlaceholderSecretValue, looksLikeDescriptivePlaceholderPhrase, SECRET_PATTERNS, + secretPatternMatches, } from "../../src/review/secret-patterns"; // Direct unit coverage of the shared module extracted in #4608. secrets-scan.test.ts and @@ -16,6 +17,12 @@ import { // no-behavior-change regression guard for the extraction itself) — this file tests the primitives directly, // at the layer they now actually live at. +// AWS's own officially published documentation placeholder (the exact literal this PR allowlists). Assembled +// from fragments (never a contiguous match in this file's own source) so the repo's own gate scanner — +// which doesn't yet know about this PR's own knownSafeValues addition when it scans this diff — doesn't +// flag its OWN test fixture the same way #4284 was flagged. +const AWS_EXAMPLE_KEY = "AKIA" + "IOSFODNN7EXAMPLE"; + describe("secret-patterns — shared secret-detection primitives (#4608)", () => { describe("SECRET_PATTERNS / HARD_SECRET_KINDS", () => { it("SECRET_PATTERNS is a non-empty array of uniquely named patterns", () => { @@ -51,6 +58,38 @@ describe("secret-patterns — shared secret-detection primitives (#4608)", () => }); }); + describe("secretPatternMatches", () => { + const awsPattern = SECRET_PATTERNS.find((pattern) => pattern.name === "aws_access_key")!; + + it("is a plain .test() for a pattern with no knownSafeValues (byte-identical to before)", () => { + const githubPattern = SECRET_PATTERNS.find((pattern) => pattern.name === "github_token")!; + expect(githubPattern.knownSafeValues).toBeUndefined(); + expect(secretPatternMatches(githubPattern, "token ghp_" + "a".repeat(30))).toBe(true); + expect(secretPatternMatches(githubPattern, "just prose")).toBe(false); + }); + + it("does NOT flag AWS's own officially published documentation example key (#4284 regression)", () => { + expect(secretPatternMatches(awsPattern, `const key = "${AWS_EXAMPLE_KEY}";`)).toBe(false); + }); + + it("still flags a real-shaped AWS access key that is not the known-safe literal", () => { + expect(secretPatternMatches(awsPattern, "AKIA" + "ABCDEFGHIJKLMNOP")).toBe(true); + }); + + it("still flags a genuine leak elsewhere in the same text, even alongside the known-safe example", () => { + const text = `const example = "${AWS_EXAMPLE_KEY}"; const real = "AKIA${"ABCDEFGHIJKLMNOP"}";`; + expect(secretPatternMatches(awsPattern, text)).toBe(true); + }); + + it("reuses an already-global-flagged pattern's regex as-is, rather than re-adding the g flag", () => { + // No live SECRET_PATTERNS entry carries the `g` flag today (a duplicate `gg` flag throws), but the + // branch that reuses an existing `g` flag instead of appending a second one must still be covered. + const alreadyGlobal = { name: "synthetic", re: /\bAKIA[0-9A-Z]{16}\b/g, knownSafeValues: new Set([AWS_EXAMPLE_KEY]) }; + expect(secretPatternMatches(alreadyGlobal, AWS_EXAMPLE_KEY)).toBe(false); + expect(secretPatternMatches(alreadyGlobal, "AKIA" + "ABCDEFGHIJKLMNOP")).toBe(true); + }); + }); + describe("hasLongSequentialRun", () => { it("returns false when the value is too short to reach the threshold", () => { expect(hasLongSequentialRun("")).toBe(false); diff --git a/test/unit/secrets-scan.test.ts b/test/unit/secrets-scan.test.ts index 7ff29b46e5..d5efcce538 100644 --- a/test/unit/secrets-scan.test.ts +++ b/test/unit/secrets-scan.test.ts @@ -26,7 +26,14 @@ describe("scanForSecrets — deterministic secret-pattern scanner", () => { }); it("flags an AWS access key id", () => { - expect(scanForSecrets("AKIAIOSFODNN7EXAMPLE").kinds).toContain("aws_access_key"); + expect(scanForSecrets("AKIA" + "ABCDEFGHIJKLMNOP").kinds).toContain("aws_access_key"); + }); + + // #4284: AWS's own officially published documentation placeholder caused 4 false-positive PR closes in + // gittensory's own subprocess-env-redaction-helper epic before this exclusion existed. Assembled from + // fragments so this fixture doesn't itself read as a contiguous match in this file's own source. + it("does NOT flag AWS's own officially published documentation example key", () => { + expect(scanForSecrets("AKIA" + "IOSFODNN7EXAMPLE").kinds).not.toContain("aws_access_key"); }); it("flags a Slack token", () => { @@ -44,7 +51,7 @@ describe("scanForSecrets — deterministic secret-pattern scanner", () => { }); it("collects multiple distinct kinds in one scan", () => { - const r = scanForSecrets("ghp_ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789 and AKIAIOSFODNN7EXAMPLE"); + const r = scanForSecrets("ghp_" + "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789 and AKIA" + "ABCDEFGHIJKLMNOP"); expect(r.found).toBe(true); expect(r.kinds).toEqual(expect.arrayContaining(["github_token", "aws_access_key"])); });