diff --git a/review-enrichment/src/analyzers/secret-scan.ts b/review-enrichment/src/analyzers/secret-scan.ts index 05d8c6009c..dd458dc096 100644 --- a/review-enrichment/src/analyzers/secret-scan.ts +++ b/review-enrichment/src/analyzers/secret-scan.ts @@ -1047,27 +1047,24 @@ function hasLongSequentialRun(value: string): boolean { // 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; +// #4579-followup: these exact live false-positive literals are fixture/enum names, not credentials. Keep +// this allowlist intentionally closed: a broad suffix rule would suppress plausible human-chosen secrets such +// as `client_secret = "correct-horse-battery-secret"`. +const KNOWN_FIXTURE_SECRET_VALUES = new Set([ + "installation-token", + "default-session-token", + "beta-session-token", + "unsafe_install_or_secret", +]); /** 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"). */ + * known fixture/enum literal. */ 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; + if (KNOWN_FIXTURE_SECRET_VALUES.has(value)) return true; return hasLongSequentialRun(value); } diff --git a/review-enrichment/test/secret-scan.test.ts b/review-enrichment/test/secret-scan.test.ts index 22df45dca5..f20da4120c 100644 --- a/review-enrichment/test/secret-scan.test.ts +++ b/review-enrichment/test/secret-scan.test.ts @@ -142,10 +142,13 @@ test("scanPatch does NOT flag an underscore-separated self-naming enum label (#4 ); }); +test("scanPatch still flags lowercase segmented credentials even when their suffix names a secret kind (#4579-followup regression)", () => { + const findings = scanPatch("src/config.ts", hunk([`const client_secret = "correct-horse-battery-secret";`])); + assert.equal(findings.length, 1); + assert.equal(findings[0].kind, "generic_secret_assignment"); +}); + 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"); diff --git a/scripts/check-engine-parity.ts b/scripts/check-engine-parity.ts index 8f095ce8c5..7c6cb53692 100644 --- a/scripts/check-engine-parity.ts +++ b/scripts/check-engine-parity.ts @@ -113,7 +113,7 @@ export const SECRET_DETECTION_MARKERS = Object.freeze([ "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;", + "if (KNOWN_FIXTURE_SECRET_VALUES.has(value)) return true;", "return hasLongSequentialRun(value);", '"github_token"', '"github_pat"', diff --git a/src/review/secret-patterns.ts b/src/review/secret-patterns.ts index 7b1005d5e7..2f07dd42d2 100644 --- a/src/review/secret-patterns.ts +++ b/src/review/secret-patterns.ts @@ -72,29 +72,25 @@ export function hasLongSequentialRun(value: string): boolean { // 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; +// #4579-followup: these exact live false-positive literals are fixture/enum names, not credentials. Keep +// this allowlist intentionally closed: a broad suffix rule would suppress plausible human-chosen secrets such +// as `client_secret = "correct-horse-battery-secret"`. +const KNOWN_FIXTURE_SECRET_VALUES = new Set([ + "installation-token", + "default-session-token", + "beta-session-token", + "unsafe_install_or_secret", +]); /** 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 lowercase identifier whose own last segment self-names as a secret kind - * (e.g. "default-session-token", "unsafe_install_or_secret"). Mirrored (drift-checked, not imported) in + * (e.g. "abcdefghijklmnop123"), or a known fixture/enum literal. Mirrored (drift-checked, not imported) in * review-enrichment/src/analyzers/secret-scan.ts — see this file's header. */ export 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; + if (KNOWN_FIXTURE_SECRET_VALUES.has(value)) return true; return hasLongSequentialRun(value); } diff --git a/test/unit/check-engine-parity-script.test.ts b/test/unit/check-engine-parity-script.test.ts index 6660e82ed0..3b9273502a 100644 --- a/test/unit/check-engine-parity-script.test.ts +++ b/test/unit/check-engine-parity-script.test.ts @@ -369,7 +369,7 @@ describe("check-engine-parity script", () => { "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;", + "if (KNOWN_FIXTURE_SECRET_VALUES.has(value)) return true;", "return hasLongSequentialRun(value);", ].join("\n"); const hostKinds = SECRET_DETECTION_MARKERS.filter((marker) => marker.startsWith('"')).join("\n"); diff --git a/test/unit/content-lane-security-scan.test.ts b/test/unit/content-lane-security-scan.test.ts index 9925686f34..9932d35150 100644 --- a/test/unit/content-lane-security-scan.test.ts +++ b/test/unit/content-lane-security-scan.test.ts @@ -120,19 +120,22 @@ describe("scanForSecrets", () => { }); // #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. + // unsafe_install_or_secret"; the same known fixture shape as metagraphed/gittensory#4524's + // "token: default-session-token") -- these exact literals are enum/fixture names, not credentials. 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) => { + ])("does NOT flag a known 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"); + it.each([ + ["generic passphrase", 'token = "alpha-bravo-charlie-delta"'], + ["client secret ending in -secret", 'client_secret = "correct-horse-battery-secret"'], + ["password ending in -passwd", 'password: "legacy-system-passwd"'], + ["api key ending in -key", 'api_key = "internal-service-key"'], + ])("still flags a lowercase segmented credential: %s (regression guard for #4579-followup)", (_name, snippet) => { + expect(scanForSecrets(snippet).kinds).toContain("generic_secret_assignment"); }); }); diff --git a/test/unit/secret-patterns.test.ts b/test/unit/secret-patterns.test.ts index 26175db99a..657df7c1bf 100644 --- a/test/unit/secret-patterns.test.ts +++ b/test/unit/secret-patterns.test.ts @@ -83,13 +83,21 @@ describe("secret-patterns — shared secret-detection primitives (#4608)", () => expect(isPlaceholderSecretValue("mock-aK9xQ2mZw7Ln4Rv8Pt3Bh6")).toBe(false); }); - it("flags a lowercase identifier whose own last segment self-names as a secret kind", () => { + it("flags every known fixture/enum literal in the closed allowlist", () => { + expect(isPlaceholderSecretValue("installation-token")).toBe(true); expect(isPlaceholderSecretValue("default-session-token")).toBe(true); + expect(isPlaceholderSecretValue("beta-session-token")).toBe(true); expect(isPlaceholderSecretValue("unsafe_install_or_secret")).toBe(true); }); - it("does NOT flag a self-naming-suffix-shaped value once digits/mixed case break the ALL-lowercase check", () => { + it("does NOT flag a token/secret/key/password-suffixed value that isn't in the closed fixture set (#4579-followup regression)", () => { + // Same self-naming SHAPE as the known fixtures above (ends in "-token"/"-secret"/"-key"/"-passwd"), + // but none of these exact literals are in KNOWN_FIXTURE_SECRET_VALUES, so a real human-chosen secret + // is no longer swept in just because its suffix happens to restate what kind of thing it is. expect(isPlaceholderSecretValue("session2024-token")).toBe(false); + expect(isPlaceholderSecretValue("correct-horse-battery-secret")).toBe(false); + expect(isPlaceholderSecretValue("legacy-system-passwd")).toBe(false); + expect(isPlaceholderSecretValue("internal-service-key")).toBe(false); }); it("does NOT flag a multi-segment lowercase passphrase that does not self-name as a secret kind", () => { diff --git a/test/unit/secrets-scan.test.ts b/test/unit/secrets-scan.test.ts index b1f7e39e16..7ff29b46e5 100644 --- a/test/unit/secrets-scan.test.ts +++ b/test/unit/secrets-scan.test.ts @@ -203,13 +203,12 @@ describe("scanForSecrets — deterministic secret-pattern scanner", () => { }); // #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". + // "missing before/after screenshot table"-unrelated reasons -- a leaked secret that never existed. These + // exact session-token literals are known test fixtures. 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) => { + ])("does NOT flag a known multi-segment fixture value: %s (#4579-followup)", (_name, snippet) => { expect(scanForSecrets(snippet).kinds).not.toContain("generic_secret_assignment"); }); @@ -226,9 +225,12 @@ describe("scanForSecrets — deterministic secret-pattern scanner", () => { 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"); + it.each([ + ["client_secret", 'client_secret = "correct-horse-battery-secret"'], + ["password", 'password = "legacy-system-passwd"'], + ["api_key", 'api_key = "internal-service-key"'], + ["token with digit", 'token = "session2024-token"'], + ])("still flags a self-naming-suffix-shaped credential assigned to %s", (_name, snippet) => { + expect(scanForSecrets(snippet).kinds).toContain("generic_secret_assignment"); }); });