From bb672e2bd9ba74539c8ea67f1e4cc1fd31a7e04e Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 9 Jul 2026 23:13:04 -0700 Subject: [PATCH] fix(review): stop generic_secret_assignment false-flagging self-naming fixture/enum values Confirmed live false positives across all three repos this rule covers: metagraphed/gittensory#4524 ("token = default-session-token" / "beta-session-token", both test fixtures), awesome-claude#4758 ("embedded_secret: unsafe_install_or_secret", an enum/category label) -- none had a real secret present, yet all closed the PR outright via the hard-blocking generic_secret_assignment kind. Root cause: isPlaceholderSecretValue's existing fixture carve-out only recognized an EXACT two-segment lowercase-hyphenated value assigned to a literal `token` key (`^[a-z]+-[a-z]+$`) -- missing 3+-segment fixtures and any key name other than `token` entirely. Replaces it with a narrower, evidence-driven check: a value whose OWN last segment self-names as a secret kind (ends in -token/-secret/-key/-password/-passwd) reads as a NAME for a concept, not an opaque credential -- deliberately narrower than "any multi-segment lowercase phrase" so a real Diceware-style passphrase like "alpha-bravo-charlie-delta" (an existing, deliberate test case) still correctly flags. Applied identically across all three independent copies of this heuristic (each self-contained by design, no cross-package imports): the Worker's src/review/secrets-scan.ts (feeds the deterministic hard-block gate), src/review/content-lane/security-scan.ts (awesome-claude content-lane submissions), and review-enrichment/src/analyzers/secret-scan.ts (REES's own standalone-deployed advisory analyzer) -- REES's rule-application loop didn't do ANY placeholder filtering for this kind before, so its advisory briefs were even noisier than the two hard-blocking copies. --- .../src/analyzers/secret-scan.ts | 71 +++++++++++++++++-- review-enrichment/test/secret-scan.test.ts | 38 +++++++++- src/review/content-lane/security-scan.ts | 19 ++++- src/review/secrets-scan.ts | 26 ++++--- test/unit/content-lane-security-scan.test.ts | 16 +++++ test/unit/secrets-scan.test.ts | 30 ++++++++ 6 files changed, 185 insertions(+), 15 deletions(-) diff --git a/review-enrichment/src/analyzers/secret-scan.ts b/review-enrichment/src/analyzers/secret-scan.ts index 1f011c2a98..05d8c6009c 100644 --- a/review-enrichment/src/analyzers/secret-scan.ts +++ b/review-enrichment/src/analyzers/secret-scan.ts @@ -1012,12 +1012,75 @@ const RULES: Rule[] = [ confidence: "medium", }, { + // Captures the VALUE (group 1) so ruleMatches below can reject an obvious non-secret filler before + // counting this as a hit -- see isPlaceholderSecretValue. kind: "generic_secret_assignment", - re: /(?:api[_-]?key|secret|token|password|passwd|access[_-]?key|client[_-]?secret)["']?\s*[:=]\s*["'][A-Za-z0-9+/=_-]{16,}["']/i, + re: /(?:api[_-]?key|secret|token|password|passwd|access[_-]?key|client[_-]?secret)["']?\s*[:=]\s*["']([A-Za-z0-9+/=_-]{16,})["']/i, confidence: "medium", }, ]; +const PLACEHOLDER_VALUE_PATTERN = /placeholder|change[_-]?me|your[_-]|<[^>]*>|\bexample\b|redacted|dummy|\bsample\b|\btodo\b|\bfixme\b|\binsert\b|replace[_-]?me|\bfake\b/i; + +// A string with NO repeated characters (e.g. "abcdefghijklmnop123") has HIGH Shannon entropy by raw +// character-frequency counting, but is obviously not a real secret -- entropy alone only measures frequency, +// not ORDER, so a keyboard-sequential/alphabetical run slips past a pure distinct-character-count check. Detect +// the longest run of consecutive ascending or descending character codes and treat a long one as a +// human-constructed test value, not a randomly generated credential -- real API keys/tokens essentially never +// contain a 6+ character monotonic run. (Mirrors src/review/secrets-scan.ts and +// src/review/content-lane/security-scan.ts in the main Worker repo -- REES deploys standalone so this is a +// deliberate third copy, not a cross-package import; see this repo's own header comment for why.) +const MIN_SEQUENTIAL_RUN_LENGTH = 6; +function hasLongSequentialRun(value: string): boolean { + let ascendingRun = 1; + let descendingRun = 1; + for (let i = 1; i < value.length; i += 1) { + const diff = value.charCodeAt(i) - value.charCodeAt(i - 1); + ascendingRun = diff === 1 ? ascendingRun + 1 : 1; + descendingRun = diff === -1 ? descendingRun + 1 : 1; + if (ascendingRun >= MIN_SEQUENTIAL_RUN_LENGTH || descendingRun >= MIN_SEQUENTIAL_RUN_LENGTH) return true; + } + return false; +} + +// Lowercase hyphenated mock names are fixtures; mixed-case/digit-bearing values containing "mock" remain +// plausible credentials and must still be reported. +const LOWERCASE_HYPHENATED_MOCK_FIXTURE_PATTERN = /^(?:[a-z]+-)*mock(?:-[a-z]+)*$/; + +// All-lowercase-letters value check, shared by the self-naming-suffix exclusion below. +const ALL_LOWERCASE_SEGMENTS_PATTERN = /^[a-z]+(?:[-_][a-z]+)*$/; + +// #4579-followup (confirmed live false positives: metagraphed/gittensory#4524 "token = default-session-token"/ +// "beta-session-token", awesome-claude#4758 "embedded_secret: unsafe_install_or_secret" -- none a real secret): +// a value whose OWN last hyphen/underscore-separated segment is itself one of the same secret-shaped trigger +// words reads as a NAME for a concept ("this is a kind of token/secret"), not an opaque credential -- a real +// generated token/key value never ends by literally restating what kind of thing it is. Deliberately NARROWER +// than "any multi-segment lowercase phrase": a Diceware-style passphrase like "alpha-bravo-charlie-delta" +// doesn't end in a trigger word, so it still correctly flags -- only values that self-identify as a +// token/secret/key/password NAME are excluded. +const SELF_NAMING_FIXTURE_SUFFIX_PATTERN = /[-_](?:token|secret|key|password|passwd)$/i; + +/** True for an obvious non-secret filler value: a known placeholder phrase, a string built from at most 2 + * distinct characters, a long monotonic character-code run, a lowercase hyphenated mock fixture name, or a + * lowercase identifier whose own last segment self-names as a secret kind (e.g. "default-session-token"). */ +function isPlaceholderSecretValue(value: string): boolean { + if (PLACEHOLDER_VALUE_PATTERN.test(value)) return true; + if (new Set(value.toLowerCase()).size <= 2) return true; + if (LOWERCASE_HYPHENATED_MOCK_FIXTURE_PATTERN.test(value)) return true; + if (ALL_LOWERCASE_SEGMENTS_PATTERN.test(value) && SELF_NAMING_FIXTURE_SUFFIX_PATTERN.test(value)) return true; + return hasLongSequentialRun(value); +} + +/** True when `rule` actually matches `text` -- for every format-specific rule this is a plain `.test()`, but + * `generic_secret_assignment` also requires its captured VALUE to clear {@link isPlaceholderSecretValue} + * first, since a keyword-plus-quoted-value SHAPE also matches plenty of non-secrets (a Zod schema field, a + * TypeScript type declaration, a test fixture, an enum/category label). */ +function ruleMatches(rule: Rule, text: string): boolean { + if (rule.kind !== "generic_secret_assignment") return rule.re.test(text); + const match = rule.re.exec(text); + return match !== null && !isPlaceholderSecretValue(match[1]!); +} + /** Extract the inner text of every quoted string literal (single/double/backtick) on a line. Used to catch a * secret whose literal value is split across two adjacent added lines and joined at runtime (e.g. * `const a = "AKIA..."; const b = a + "REST";`) — pure per-line regex matching never sees the runtime-joined @@ -1054,7 +1117,7 @@ export function scanPatch(path: string, patch: string): SecretFinding[] { const content = line.slice(1); let matched = false; for (const rule of RULES) { - if (rule.re.test(content)) { + if (ruleMatches(rule, content)) { findings.push({ file: path, line: newLine, kind: rule.kind, confidence: rule.confidence }); matched = true; break; // one finding per line — first (most specific) rule wins @@ -1068,7 +1131,7 @@ export function scanPatch(path: string, patch: string): SecretFinding[] { if (!matched && lastPrevious !== undefined && firstCurrent !== undefined) { const joined = lastPrevious + firstCurrent; for (const rule of RULES) { - if (rule.re.test(joined)) { + if (ruleMatches(rule, joined)) { // "medium" regardless of the rule's own confidence — a joined pair is a heuristic, not a direct match. findings.push({ file: path, line: newLine, kind: rule.kind, confidence: "medium" }); break; @@ -1095,7 +1158,7 @@ export function scanAddedLinesForSecrets( const findings: SecretFinding[] = []; for (const line of addedLines) { for (const rule of RULES) { - if (rule.re.test(line.text)) { + if (ruleMatches(rule, line.text)) { findings.push({ file: line.file, line: line.line, diff --git a/review-enrichment/test/secret-scan.test.ts b/review-enrichment/test/secret-scan.test.ts index 726de47e42..22df45dca5 100644 --- a/review-enrichment/test/secret-scan.test.ts +++ b/review-enrichment/test/secret-scan.test.ts @@ -2,7 +2,7 @@ import { test } from "node:test"; import assert from "node:assert/strict"; -import { scanPatch } from "../dist/analyzers/secret-scan.js"; +import { scanAddedLinesForSecrets, scanPatch } from "../dist/analyzers/secret-scan.js"; const hunk = (lines) => `@@ -1,0 +1,${lines.length} @@\n${lines.map((l) => `+${l}`).join("\n")}`; @@ -123,6 +123,42 @@ test("scanPatch flags a generic secret assignment", () => { assert.equal(findings[0].confidence, "medium"); }); +// #4579-followup: confirmed live false positives (metagraphed/gittensory#4524 "token = default-session-token"/ +// "beta-session-token", awesome-claude#4758 "embedded_secret: unsafe_install_or_secret") -- none was a real +// secret, both were test fixtures / enum-category labels whose own last segment self-names as a secret kind. +test("scanPatch does NOT flag a self-naming multi-segment fixture value (#4579-followup)", () => { + const findings = scanPatch("src/config.ts", hunk([`const token = "default-session-token";`])); + assert.equal( + findings.some((f) => f.kind === "generic_secret_assignment"), + false, + ); +}); + +test("scanPatch does NOT flag an underscore-separated self-naming enum label (#4579-followup)", () => { + const findings = scanPatch("src/config.ts", hunk([`const embedded_secret = "unsafe_install_or_secret";`])); + assert.equal( + findings.some((f) => f.kind === "generic_secret_assignment"), + false, + ); +}); + +test("scanPatch still flags a generic multi-segment lowercase passphrase that does NOT self-name as a secret kind (regression guard for #4579-followup)", () => { + // Same shape as the excluded fixtures above (all-lowercase, hyphen-separated, no digits) but the value's + // own last segment is "delta", not token/secret/key/password/passwd -- a real Diceware-style passphrase + // must not be swept in by the new self-naming-suffix exclusion. + const findings = scanPatch("src/config.ts", hunk([`const token = "alpha-bravo-charlie-delta";`])); + assert.equal(findings.length, 1); + assert.equal(findings[0].kind, "generic_secret_assignment"); +}); + +test("scanAddedLinesForSecrets applies the same self-naming-fixture exclusion as scanPatch (#4579-followup)", () => { + const findings = scanAddedLinesForSecrets([{ file: "src/config.ts", line: 1, text: `const token = "default-session-token";` }]); + assert.equal( + findings.some((f) => f.kind === "generic_secret_assignment"), + false, + ); +}); + test("scanPatch reports nothing for clean code", () => { const findings = scanPatch("src/app.ts", hunk(['const greeting = "hello world";', "export function run() {}"])); assert.equal(findings.length, 0); diff --git a/src/review/content-lane/security-scan.ts b/src/review/content-lane/security-scan.ts index e8361cd669..44b2f11eb9 100644 --- a/src/review/content-lane/security-scan.ts +++ b/src/review/content-lane/security-scan.ts @@ -63,12 +63,27 @@ function hasLongSequentialRun(value: string): boolean { return false; } +// All-lowercase-letters value check, shared by the self-naming-suffix exclusion below. +const ALL_LOWERCASE_SEGMENTS_PATTERN = /^[a-z]+(?:[-_][a-z]+)*$/; + +// #4579-followup (metagraphed/gittensory#4524 "token = default-session-token"/"beta-session-token", +// awesome-claude#4758 "embedded_secret: unsafe_install_or_secret" -- both confirmed live, no real secret +// present): a value whose OWN last hyphen/underscore-separated segment is itself one of the same secret-shaped +// trigger words reads as a NAME for a concept ("this is a kind of token/secret"), not an opaque credential -- +// a real generated token/key value never ends by literally restating what kind of thing it is. Deliberately +// NARROWER than "any multi-segment lowercase phrase": a Diceware-style passphrase like +// "alpha-bravo-charlie-delta" doesn't end in a trigger word, so it still correctly flags -- only values that +// self-identify as a token/secret/key/password NAME are excluded. +const SELF_NAMING_FIXTURE_SUFFIX_PATTERN = /[-_](?:token|secret|key|password|passwd)$/i; + /** True for an obvious non-secret filler value: a known placeholder phrase, a string built from at most 2 - * distinct characters (e.g. "xxxxxxxxxxxxxxxx", "----------------"), or a long monotonic character-code run - * (e.g. "abcdefghijklmnop123") — real high-entropy secrets never look like any of these. */ + * distinct characters (e.g. "xxxxxxxxxxxxxxxx", "----------------"), a long monotonic character-code run + * (e.g. "abcdefghijklmnop123"), or a lowercase identifier whose own last segment self-names as a secret kind + * (e.g. "default-session-token", "unsafe_install_or_secret") — real high-entropy secrets never look like any of these. */ function isPlaceholderSecretValue(value: string): boolean { if (PLACEHOLDER_VALUE_PATTERN.test(value)) return true; if (new Set(value.toLowerCase()).size <= 2) return true; + if (ALL_LOWERCASE_SEGMENTS_PATTERN.test(value) && SELF_NAMING_FIXTURE_SUFFIX_PATTERN.test(value)) return true; return hasLongSequentialRun(value); } diff --git a/src/review/secrets-scan.ts b/src/review/secrets-scan.ts index 442051a23e..c1af35e412 100644 --- a/src/review/secrets-scan.ts +++ b/src/review/secrets-scan.ts @@ -67,22 +67,32 @@ function hasLongSequentialRun(value: string): boolean { return false; } -// #3041: fixture names like "installation-token" are common in this repo and should not trip the -// generic token-assignment heuristic. Keep that carve-out key-aware and two-word-only: lowercase -// hyphenated values assigned to password/passwd/client_secret remain plausible passphrase-style credentials. -const LOWERCASE_HYPHENATED_TOKEN_FIXTURE_PATTERN = /^[a-z]+-[a-z]+$/; // Lowercase hyphenated mock names are fixtures; mixed-case/digit-bearing values containing "mock" remain // plausible credentials and must still be reported by the generic assignment scanner. const LOWERCASE_HYPHENATED_MOCK_FIXTURE_PATTERN = /^(?:[a-z]+-)*mock(?:-[a-z]+)*$/; +// All-lowercase-letters value check, shared by the self-naming-suffix exclusion below. +const ALL_LOWERCASE_SEGMENTS_PATTERN = /^[a-z]+(?:[-_][a-z]+)*$/; + +// #4579-followup (metagraphed/gittensory#4524 "token = default-session-token"/"beta-session-token", +// awesome-claude#4758 "embedded_secret: unsafe_install_or_secret" -- both confirmed live, no real secret +// present): a value whose OWN last hyphen/underscore-separated segment is itself one of the same secret-shaped +// trigger words reads as a NAME for a concept ("this is a kind of token/secret"), not an opaque credential -- +// a real generated token/key value never ends by literally restating what kind of thing it is. Deliberately +// NARROWER than "any multi-segment lowercase phrase": a Diceware-style passphrase like +// "alpha-bravo-charlie-delta" doesn't end in a trigger word, so it still correctly flags (regression guard +// below) -- only values that self-identify as a token/secret/key/password NAME are excluded. +const SELF_NAMING_FIXTURE_SUFFIX_PATTERN = /[-_](?:token|secret|key|password|passwd)$/i; + /** True for an obvious non-secret filler value: a known placeholder phrase, a string built from at most 2 * distinct characters (e.g. "xxxxxxxxxxxxxxxx", "----------------"), a long monotonic character-code run - * (e.g. "abcdefghijklmnop123"), or a narrow token fixture name (e.g. "installation-token"). */ -function isPlaceholderSecretValue(key: string, value: string): boolean { + * (e.g. "abcdefghijklmnop123"), or a lowercase identifier whose own last segment self-names as a secret kind + * (e.g. "default-session-token", "unsafe_install_or_secret"). */ +function isPlaceholderSecretValue(value: string): boolean { if (PLACEHOLDER_VALUE_PATTERN.test(value)) return true; if (new Set(value.toLowerCase()).size <= 2) return true; if (LOWERCASE_HYPHENATED_MOCK_FIXTURE_PATTERN.test(value)) return true; - if (key.toLowerCase() === "token" && LOWERCASE_HYPHENATED_TOKEN_FIXTURE_PATTERN.test(value)) return true; + if (ALL_LOWERCASE_SEGMENTS_PATTERN.test(value) && SELF_NAMING_FIXTURE_SUFFIX_PATTERN.test(value)) return true; return hasLongSequentialRun(value); } @@ -94,7 +104,7 @@ function hasGenericSecretAssignment(text: string): boolean { while ((match = GENERIC_SECRET_ASSIGNMENT_PATTERN.exec(text)) !== null) { // The key and value groups are mandatory (not `?`/`*`-wrapped), so both are always present // whenever the overall match succeeds -- non-null by construction, not runtime branches. - if (!isPlaceholderSecretValue(match[1]!, match[2]!)) return true; + if (!isPlaceholderSecretValue(match[2]!)) return true; } return false; } diff --git a/test/unit/content-lane-security-scan.test.ts b/test/unit/content-lane-security-scan.test.ts index 6f1a6c08f5..ed35713b52 100644 --- a/test/unit/content-lane-security-scan.test.ts +++ b/test/unit/content-lane-security-scan.test.ts @@ -79,6 +79,22 @@ describe("scanForSecrets", () => { it("does NOT flag a schema field declaration with no literal value", () => { expect(scanForSecrets("password: z.string()").kinds).not.toContain("generic_secret_assignment"); }); + + // #4579-followup: confirmed live false positives (awesome-claude#4758 "embedded_secret: + // unsafe_install_or_secret"; the same self-naming shape as metagraphed/gittensory#4524's + // "token: default-session-token") -- neither is a real secret, both are enum/fixture NAMES whose own last + // segment restates the kind of thing they are. + it.each([ + ["session-token fixture", 'token: "default-session-token"'], + ["enum label ending in _secret", 'embedded_secret: "unsafe_install_or_secret"'], + ["password ending in -passwd", 'password: "legacy-system-passwd"'], + ])("does NOT flag a self-naming multi-segment fixture/enum value: %s (#4579-followup)", (_name, snippet) => { + expect(scanForSecrets(snippet).kinds).not.toContain("generic_secret_assignment"); + }); + + it("still flags a generic multi-segment lowercase passphrase that does NOT self-name as a secret kind (regression guard for #4579-followup)", () => { + expect(scanForSecrets('token = "alpha-bravo-charlie-delta"').kinds).toContain("generic_secret_assignment"); + }); }); describe("scanSubmissionContent", () => { diff --git a/test/unit/secrets-scan.test.ts b/test/unit/secrets-scan.test.ts index 52d166fdbb..b1f7e39e16 100644 --- a/test/unit/secrets-scan.test.ts +++ b/test/unit/secrets-scan.test.ts @@ -201,4 +201,34 @@ describe("scanForSecrets — deterministic secret-pattern scanner", () => { const singleWord = "qwzxvbnmalskdjfhgpoiu"; expect(scanForSecrets(`token = "${singleWord}"`).kinds).toContain("generic_secret_assignment"); }); + + // #4579-followup: confirmed live false positives (metagraphed/gittensory#4524, #4224) closed PRs for + // "missing before/after screenshot table"-unrelated reasons -- a leaked secret that never existed. Both + // values are test FIXTURES (a session-token mock) whose own last segment self-names as "token". + it.each([ + ["default-session-token", 'token: "default-session-token"'], + ["beta-session-token", 'token: "beta-session-token"'], + ["three-segment self-naming secret", 'client_secret = "some-embedded-secret"'], + ])("does NOT flag a self-naming multi-segment fixture value: %s (#4579-followup)", (_name, snippet) => { + expect(scanForSecrets(snippet).kinds).not.toContain("generic_secret_assignment"); + }); + + // #4579-followup: confirmed live false positive (awesome-claude#4758) -- an enum/category LABEL, not a + // credential, assigned to a key that itself contains "secret" (embedded_secret). + it("does NOT flag an underscore-separated self-naming enum label (#4579-followup)", () => { + expect(scanForSecrets('embedded_secret: "unsafe_install_or_secret"').kinds).not.toContain("generic_secret_assignment"); + }); + + it("still flags a generic multi-segment lowercase passphrase that does NOT self-name as a secret kind (regression guard for #4579-followup)", () => { + // Same shape as the excluded fixtures above (all-lowercase, hyphen-separated, no digits) but the value's + // own last segment is "delta", not token/secret/key/password/passwd -- a real Diceware-style passphrase + // must not be swept in by the new self-naming-suffix exclusion. + expect(scanForSecrets('token = "alpha-bravo-charlie-delta"').kinds).toContain("generic_secret_assignment"); + }); + + it("still flags a self-naming-suffix-shaped value that also has mixed case or digits (the exclusion requires ALL-lowercase)", () => { + // Ends in "-token" like the excluded fixtures, but the earlier segment has a digit -- not a plausible + // human-authored fixture name, so it must still be reported. + expect(scanForSecrets('token = "session2024-token"').kinds).toContain("generic_secret_assignment"); + }); });