From 267022aa7e893f28bcf9892dac16edfda10901b1 Mon Sep 17 00:00:00 2001 From: ghost <49853598+JSONbored@users.noreply.github.com> Date: Sun, 5 Jul 2026 17:54:45 -0700 Subject: [PATCH] fix(review): tighten hyphenated secret placeholder scan --- src/review/secrets-scan.ts | 28 ++++++++++++---------------- test/unit/safety-wiring.test.ts | 13 +++++++++++++ test/unit/secrets-scan.test.ts | 17 +++++++++++++---- 3 files changed, 38 insertions(+), 20 deletions(-) diff --git a/src/review/secrets-scan.ts b/src/review/secrets-scan.ts index 010320cb24..4d51b4819a 100644 --- a/src/review/secrets-scan.ts +++ b/src/review/secrets-scan.ts @@ -40,9 +40,9 @@ const SECRET_PATTERNS: Array<{ name: string; re: RegExp }> = [ // VALUE can be checked against isPlaceholderSecretValue before counting as a hit; the value itself is never // returned from this module (only the kind name), preserving the existing never-echo-the-secret guarantee. const GENERIC_SECRET_ASSIGNMENT_PATTERN = - /(?:api[_-]?key|secret|token|password|passwd|access[_-]?key|client[_-]?secret)["']?\s*[:=]\s*["']([A-Za-z0-9+/=_-]{16,})["']/gi; + /((?:api[_-]?key|secret|token|password|passwd|access[_-]?key|client[_-]?secret))["']?\s*[:=]\s*["']([A-Za-z0-9+/=_-]{16,})["']/gi; -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; +const PLACEHOLDER_VALUE_PATTERN = /placeholder|change[_-]?me|your[_-]|<[^>]*>|\bexample\b|redacted|dummy|\bsample\b|\btodo\b|\bfixme\b|\binsert\b|replace[_-]?me|\bfake\b|\bmock\b/i; // #2553 gate review finding: 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 @@ -63,22 +63,18 @@ function hasLongSequentialRun(value: string): boolean { return false; } -// #3041: a value made ENTIRELY of lowercase words joined by hyphens (2+ segments, e.g. the test-fixture -// literal "installation-token" used 351+ times across this repo's own test suite as a mock fetch-response -// token) reads as an ordinary English-word compound identifier -- a mock/fixture name -- not a generated -// credential. Real secrets/tokens are essentially always alphanumeric, mixed-case, or base64/hex; they are -// never a pure lowercase-hyphenated phrase. Require at least one hyphen (2+ segments) so this stays narrow -// and doesn't broaden into excluding arbitrary single lowercase words that could plausibly be real secrets. -const LOWERCASE_HYPHENATED_COMPOUND_PATTERN = /^[a-z]+(-[a-z]+)+$/; +// #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]+$/; /** 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-hyphenated word compound (e.g. "installation-token") — real - * high-entropy secrets never look like any of these. */ -function isPlaceholderSecretValue(value: string): boolean { + * (e.g. "abcdefghijklmnop123"), or a narrow token fixture name (e.g. "installation-token"). */ +function isPlaceholderSecretValue(key: string, value: string): boolean { if (PLACEHOLDER_VALUE_PATTERN.test(value)) return true; if (new Set(value.toLowerCase()).size <= 2) return true; - if (LOWERCASE_HYPHENATED_COMPOUND_PATTERN.test(value)) return true; + if (key.toLowerCase() === "token" && LOWERCASE_HYPHENATED_TOKEN_FIXTURE_PATTERN.test(value)) return true; return hasLongSequentialRun(value); } @@ -88,9 +84,9 @@ function hasGenericSecretAssignment(text: string): boolean { GENERIC_SECRET_ASSIGNMENT_PATTERN.lastIndex = 0; let match: RegExpExecArray | null; while ((match = GENERIC_SECRET_ASSIGNMENT_PATTERN.exec(text)) !== null) { - // The pattern's sole capturing group is mandatory (not `?`/`*`-wrapped), so it is always present - // whenever the overall match succeeds -- non-null by construction, not a runtime branch. - if (!isPlaceholderSecretValue(match[1]!)) return true; + // 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; } return false; } diff --git a/test/unit/safety-wiring.test.ts b/test/unit/safety-wiring.test.ts index 06aece3164..ce32a36f05 100644 --- a/test/unit/safety-wiring.test.ts +++ b/test/unit/safety-wiring.test.ts @@ -305,6 +305,19 @@ describe("secret-leak finding in the advisory build", () => { expect(out).toContain("### ok.ts (added) +2/-1\n@@\n+const a = 1;"); }); + it("blocks lowercase-hyphenated password assignments as generic secret leaks", () => { + const diff = [ + "### config/prod.env (modified) +1/-0", + "@@ -0,0 +1 @@", + '+password = "alpha-bravo-charlie-delta"', + ].join("\n"); + const finding = secretLeakFinding(diff); + expect(finding?.code).toBe("secret_leak"); + expect(finding?.severity).toBe("critical"); + expect(finding?.title).toContain("generic_secret_assignment"); + expect(finding?.detail).toContain("config/prod.env:1"); + }); + it("FLAG-OFF: a concrete leaked secret STILL produces the secret_leak finding (unconditional, #audit-3.4)", async () => { const env = createTestEnv({ GITTENSORY_REVIEW_SAFETY: "false" }); const adv = advisory(); diff --git a/test/unit/secrets-scan.test.ts b/test/unit/secrets-scan.test.ts index 884ba6821a..4405a07c72 100644 --- a/test/unit/secrets-scan.test.ts +++ b/test/unit/secrets-scan.test.ts @@ -146,10 +146,19 @@ describe("scanForSecrets — deterministic secret-pattern scanner", () => { ["installation-token", 'token: "installation-token"'], ["access-token", 'token = "access-token"'], ["some-mock-secret-value", 'secret: "some-mock-secret-value"'], - ])("does NOT flag a lowercase-hyphenated word compound: %s (#3041)", (_name, snippet) => { + ])("does NOT flag a clear lowercase-hyphenated fixture value: %s (#3041)", (_name, snippet) => { expect(scanForSecrets(snippet).kinds).not.toContain("generic_secret_assignment"); }); + it.each([ + ["password", 'password = "alpha-bravo-charlie-delta"'], + ["passwd", 'passwd: "alpha-bravo-charlie-delta"'], + ["client_secret", 'client_secret = "alpha-bravo-charlie-delta"'], + ["multi-segment token", 'token = "alpha-bravo-charlie-delta"'], + ])("flags a plausible lowercase-hyphenated credential assigned to %s", (_name, snippet) => { + expect(scanForSecrets(snippet).kinds).toContain("generic_secret_assignment"); + }); + it("still flags a real-looking generic secret with digits and mixed case (regression guard for #3041)", () => { // Same fixture as the "high-entropy value" test above — proves the new lowercase-hyphenated exclusion // doesn't broaden past its intended narrow shape: this value has digits + mixed case, not a pure @@ -158,10 +167,10 @@ describe("scanForSecrets — deterministic secret-pattern scanner", () => { expect(scanForSecrets(`fakeSecret = "${fakeSecret}"`).kinds).toContain("generic_secret_assignment"); }); - it("a single lowercase word with no hyphen is unaffected by the new hyphenated-compound exclusion (#3041)", () => { + it("a single lowercase word with no hyphen is unaffected by the token-fixture exclusion (#3041)", () => { // 20 lowercase letters, no repeats and no sequential run, so it isn't already caught by the entropy/ - // placeholder checks either -- proves LOWERCASE_HYPHENATED_COMPOUND_PATTERN specifically requires a - // hyphen (2+ segments) and does not accidentally match a single unhyphenated word. + // placeholder checks either -- proves the token-fixture exclusion specifically requires one two-word + // hyphenated value and does not accidentally match a single unhyphenated word. const singleWord = "qwzxvbnmalskdjfhgpoiu"; expect(scanForSecrets(`token = "${singleWord}"`).kinds).toContain("generic_secret_assignment"); });