From dc1496c4e04d2e8f511c4bd5b2d9e4cb6ab02cb4 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 06:12:26 -0700 Subject: [PATCH] fix(review): reject sequential character runs in secret-shaped values 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 since entropy alone measures frequency, not order. A keyboard-sequential or alphabetical run slipped past the prior distinct- character-count placeholder check. Detect the longest run of consecutive ascending or descending character codes and treat a 6+ character run as a human-constructed test value rather than a randomly generated credential. --- src/review/safety.ts | 9 +++++ src/review/secrets-scan.ts | 63 +++++++++++++++++++++++++++++++++ test/unit/safety-wiring.test.ts | 23 ++++++++++++ test/unit/secrets-scan.test.ts | 48 +++++++++++++++++++++++++ 4 files changed, 143 insertions(+) diff --git a/src/review/safety.ts b/src/review/safety.ts index 986cd93f83..0c4ebca7dd 100644 --- a/src/review/safety.ts +++ b/src/review/safety.ts @@ -16,12 +16,21 @@ import { scanForSecrets } from "./secrets-scan"; // (RC6: #1505/#1495/#1485). A real-format token IS a leak regardless of the file it lives in, so we keep the // concrete formats as hard blockers and ignore only the ambiguous heuristics. This mirrors the same gate the // content lane already uses (src/review/content-lane/security-scan.ts). +// +// #2553: widened to match review-enrichment/src/analyzers/secret-scan.ts's richer, higher-recall rule set. +// google_api_key/jwt are as format-precise as the original five (near-zero false-positive risk). +// generic_secret_assignment is the one keyword-shaped pattern here — secrets-scan.ts already excludes +// placeholder/type-declaration/schema-shaped matches (see isPlaceholderSecretValue there) before this kind +// is ever produced, so it is safe to treat as an unconditional hard blocker like the rest. const HARD_SECRET_KINDS = new Set([ "github_token", "github_pat", "private_key_block", "aws_access_key", "slack_token", + "google_api_key", + "jwt", + "generic_secret_assignment", ]); /** True when the safety scan is enabled. Flag-OFF (default) → every helper below is a no-op pass-through. */ diff --git a/src/review/secrets-scan.ts b/src/review/secrets-scan.ts index b1f0ce4068..da45d4f176 100644 --- a/src/review/secrets-scan.ts +++ b/src/review/secrets-scan.ts @@ -4,6 +4,14 @@ // SELF-CONTAINED NATIVE PORT (reviewbot→gittensory convergence): every type + pattern this module needs is // defined HERE. No imports from reviewbot. The logic is byte-faithful to the reviewbot source // (src/core/secrets-scan.ts); there are no stricter-tsconfig deltas — the module is already total. +// +// #2553: widened to match review-enrichment/src/analyzers/secret-scan.ts's richer, higher-recall rule set +// (google_api_key, jwt, generic_secret_assignment) so the deterministic hard blocker (safety.ts's +// HARD_SECRET_KINDS) catches the same patterns REES's advisory-only enrichment brief already does. Kept as a +// second, independent copy here rather than a cross-package import: review-enrichment deploys standalone on +// Railway with its own tsconfig/build/test pipeline (see review-enrichment/package.json), so importing across +// that boundary would break its independence — the same reasoning this file's own header already documents +// for staying self-contained relative to reviewbot. const SECRET_PATTERNS: Array<{ name: string; re: RegExp }> = [ { name: "github_token", re: /\bgh[pousr]_[A-Za-z0-9]{20,}\b/ }, @@ -11,10 +19,64 @@ const SECRET_PATTERNS: Array<{ name: string; re: RegExp }> = [ { 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: "slack_token", re: /\bxox[baprs]-[A-Za-z0-9-]{10,}\b/ }, + { name: "google_api_key", re: /\bAIza[0-9A-Za-z_-]{35}\b/ }, + { name: "jwt", re: /\beyJ[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,}\b/ }, { name: "seed_or_mnemonic", re: /\b(?:seed phrase|mnemonic)\b/i }, { name: "bittensor_key", re: /\b(?:hot|cold)key\b\s*[:=]/i }, ]; +// Deliberately NOT in SECRET_PATTERNS above: unlike the format-specific patterns (a real GitHub token/AWS key +// ALWAYS matches its exact character format, so a bare .test() is precise enough), a keyword-plus-quoted-value +// SHAPE also matches plenty of non-secrets -- a Zod schema field (`password: z.string()`), a TypeScript type +// declaration, or a placeholder value ("xxx", "your-api-key-here", ""). Captured so each match's +// 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; + +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; + +// #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 +// 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 (e.g. +// "abcdefg" or "9876543") 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. +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; +} + +/** 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. */ +function isPlaceholderSecretValue(value: string): boolean { + if (PLACEHOLDER_VALUE_PATTERN.test(value)) return true; + if (new Set(value.toLowerCase()).size <= 2) return true; + return hasLongSequentialRun(value); +} + +function hasGenericSecretAssignment(text: string): boolean { + // No zero-length-match / lastIndex-stall guard needed: the pattern's captured value alone requires 16+ + // characters, so every match is well over 16 characters long and lastIndex always advances past match.index. + 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; + } + return false; +} + export interface SecretScanResult { found: boolean; kinds: string[]; @@ -23,5 +85,6 @@ export interface SecretScanResult { 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); + if (hasGenericSecretAssignment(text)) kinds.push("generic_secret_assignment"); return { found: kinds.length > 0, kinds }; } diff --git a/test/unit/safety-wiring.test.ts b/test/unit/safety-wiring.test.ts index a78663334a..c868c7bd6b 100644 --- a/test/unit/safety-wiring.test.ts +++ b/test/unit/safety-wiring.test.ts @@ -343,6 +343,29 @@ describe("gate treats secret_leak as a hard blocker", () => { expect(gate.conclusion).toBe("success"); expect(gate.blockers).toEqual([]); }); + + // #2553: the three widened kinds (google_api_key, jwt, generic_secret_assignment) hard-block exactly like + // the original five — same secretLeakFinding -> evaluateGateCheck path, no separate opt-in. + it.each([ + ["google_api_key", `### src/config.ts (modified) +1/-0\n@@\n+const key = "${"AIza" + "SyABCDEFGHIJKLMNOPQRSTUVWXYZ0123456"}";`], + [ + "jwt", + `### src/config.ts (modified) +1/-0\n@@\n+const jwt = "${"eyJhbGciOiJIUzI1NiJ9" + "." + "eyJzdWIiOiIxMjM0NTY3ODkwIn0" + "." + "SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c"}";`, + ], + ["generic_secret_assignment", `### src/config.ts (modified) +1/-0\n@@\n+secret = "${"sk_live_" + "aK9xQ2mZw7Ln4Rv8Pt3Bh6"}"`], + ])("hard-blocks a %s finding", (kind, diff) => { + const finding = secretLeakFinding(diff); + expect(finding?.code).toBe("secret_leak"); + expect(finding?.title).toContain(kind); + const gate = evaluateGateCheck(advisory([finding!]), { confirmedContributor: true }); + expect(gate.conclusion).toBe("failure"); + expect(gate.blockers.map((b) => b.code)).toContain("secret_leak"); + }); + + it("does not hard-block a generic-assignment SHAPE that is only a placeholder value", () => { + const diff = '### src/config.ts (modified) +1/-0\n@@\n+password: "your-secret-token-value"'; + expect(secretLeakFinding(diff)).toBeNull(); + }); }); describe("secretLeakFinding scans only ADDED lines", () => { diff --git a/test/unit/secrets-scan.test.ts b/test/unit/secrets-scan.test.ts index b846ee05ed..429da4221a 100644 --- a/test/unit/secrets-scan.test.ts +++ b/test/unit/secrets-scan.test.ts @@ -48,4 +48,52 @@ describe("scanForSecrets — deterministic secret-pattern scanner", () => { expect(r.found).toBe(true); expect(r.kinds).toEqual(expect.arrayContaining(["github_token", "aws_access_key"])); }); + + // #2553: widened to match review-enrichment/src/analyzers/secret-scan.ts's richer rule set. + it("flags a Google API key", () => { + const fakeKey = "AIza" + "SyABCDEFGHIJKLMNOPQRSTUVWXYZ0123456"; + expect(scanForSecrets(fakeKey).kinds).toContain("google_api_key"); + }); + + it("flags a JWT", () => { + const fakeJwt = "eyJhbGciOiJIUzI1NiJ9" + "." + "eyJzdWIiOiIxMjM0NTY3ODkwIn0" + "." + "SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c"; + expect(scanForSecrets(fakeJwt).kinds).toContain("jwt"); + }); + + it("flags a generic secret/password/token assignment with a high-entropy value", () => { + // Mixed case + digits with no monotonic character-code run (unlike a plain "ABCDEFGH..." fixture, which + // the sequential-run filter below would correctly treat as a low-entropy placeholder, not a real secret). + const fakeSecret = "sk_live_" + "aK9xQ2mZw7Ln4Rv8Pt3Bh6"; + expect(scanForSecrets(`secret = "${fakeSecret}"`).kinds).toContain("generic_secret_assignment"); + expect(scanForSecrets(`password: "${fakeSecret}"`).kinds).toContain("generic_secret_assignment"); + expect(scanForSecrets(`client_secret = "${fakeSecret}"`).kinds).toContain("generic_secret_assignment"); + expect(scanForSecrets(`api_key: '${fakeSecret}'`).kinds).toContain("generic_secret_assignment"); + }); + + it.each([ + ["ascending run", 'token = "abcdefghijklmnop123"'], + ["descending run", 'secret = "zyxwvutsrqponmlkj987"'], + ])("does NOT flag a long monotonic character-code run: %s (gate finding: high distinct-char count is not high entropy)", (_name, snippet) => { + expect(scanForSecrets(snippet).kinds).not.toContain("generic_secret_assignment"); + }); + + it("does NOT flag a Zod/type schema field declaration with no literal value", () => { + expect(scanForSecrets('password: z.string()').kinds).not.toContain("generic_secret_assignment"); + expect(scanForSecrets("type Config = { secretKey: string; apiKey?: string }").kinds).not.toContain("generic_secret_assignment"); + }); + + it.each([ + ["xxx", 'token = "xxx"'], + ["placeholder phrase", 'secret = "your-api-key-placeholder"'], + ["angle-bracket placeholder", 'password: ""'], + ["changeme", 'client_secret: "changeme-please-changeme"'], + ["repeated-character filler", 'api_key = "xxxxxxxxxxxxxxxxxxxx"'], + ["your- prefix", 'token: "your-secret-token-value"'], + ])("does NOT flag a redacted/placeholder value: %s", (_name, snippet) => { + expect(scanForSecrets(snippet).kinds).not.toContain("generic_secret_assignment"); + }); + + it("does NOT flag a short value under the 16-character floor", () => { + expect(scanForSecrets('token = "short12345"').kinds).not.toContain("generic_secret_assignment"); + }); });