From fe5a7a1d97510bcefe98daa3343ce78966b4cc10 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 12 Jul 2026 08:13:30 -0700 Subject: [PATCH] fix(review): stop generic_secret_assignment from auto-closing PRs PR #5346 (a resubmission of #5341) was auto-closed over two inert test-fixture strings that matched the generic_secret_assignment keyword-plus-quoted-value SHAPE but weren't real credentials -- the same heuristic has now caused at least eight prior false-positive incidents (#2613, #3178, #3673, #3866, #4587, #4733, plus several fixture-rewording commits), each patched by narrowing an allowlist rather than fixing the underlying design. REES's own copy of this rule already rates it "medium confidence" ("catches real keys but also the occasional long opaque non-secret"), and content-lane/security-scan.ts's own header states the design principle this violated: a gate that auto-closes with no human queue may only hard-close on a signal unambiguous enough that a false positive is essentially impossible. Split generic_secret_assignment out of HARD_SECRET_KINDS into a new ADVISORY_ONLY_SECRET_KINDS: it still surfaces (a warning-severity possible_secret_assignment finding / a "manual" content-lane verdict), but never auto-blocks or auto-closes on its own. Concrete credential formats (github_token, aws_access_key, private_key_block, ...) are unaffected and remain unconditional hard blockers. Also add a structural placeholder heuristic (looksLikeDescriptive PlaceholderPhrase, mirrored in REES): a value with 5+ lowercase-only hyphen/underscore segments containing an English function word reads as written prose describing the value, not a credential or a chosen passphrase -- this independently resolves both PR #5346 literals without weakening detection of a genuine human-chosen passphrase like "correct-horse-battery-secret" (no function words, by design). --- .../src/analyzers/secret-scan.ts | 38 +++++++- review-enrichment/test/secret-scan.test.ts | 27 ++++++ src/review/content-lane/security-scan.ts | 49 ++++++++-- src/review/safety.ts | 93 ++++++++++++------- src/review/secret-patterns.ts | 63 +++++++++++-- test/unit/content-lane-security-scan.test.ts | 37 ++++++-- test/unit/safety-wiring.test.ts | 55 +++++++++-- test/unit/secret-patterns.test.ts | 62 ++++++++++++- 8 files changed, 350 insertions(+), 74 deletions(-) diff --git a/review-enrichment/src/analyzers/secret-scan.ts b/review-enrichment/src/analyzers/secret-scan.ts index dd458dc096..8d710f82ee 100644 --- a/review-enrichment/src/analyzers/secret-scan.ts +++ b/review-enrichment/src/analyzers/secret-scan.ts @@ -1057,14 +1057,48 @@ const KNOWN_FIXTURE_SECRET_VALUES = new Set([ "unsafe_install_or_secret", ]); +// Closed set of grammatical FUNCTION words — articles, negations, prepositions, auxiliary verbs — chosen for +// having near-zero information content per word. A human-authored placeholder that describes itself in prose +// (e.g. "present-value-not-a-real-token", "test-value-should-never-appear-in-doctor-output" — both real +// false-positive literals from gittensory PR #5346/#5341) naturally reaches for these; a deliberately +// memorable human-CHOSEN passphrase like "correct-horse-battery-secret" is composed of high-entropy CONTENT +// words (nouns/verbs/adjectives) specifically BECAUSE function words carry little entropy per word, so a +// diceware-style passphrase essentially never contains one. Their presence is therefore a reliable structural +// signal for "this is descriptive prose about the value", not "this is someone's chosen secret" — see +// looksLikeDescriptivePlaceholderPhrase below. (Mirrors src/review/secret-patterns.ts — drift-checked, not +// imported; see this file's header.) +const ENGLISH_FUNCTION_WORDS = new Set([ + "a", "an", "the", "is", "are", "was", "were", "be", "been", "not", "no", "and", "or", "to", "of", + "in", "on", "at", "for", "with", "should", "never", "always", "will", "would", "does", "did", "do", + "this", "that", "it", "as", "if", "then", "so", "but", "has", "have", "had", "can", "could", +]); + +// A written-prose fixture description needs several words to say something (both PR #5346 literals split into +// 6 and 8 segments respectively); a memorable diceware-style passphrase conventionally tops out around 4 words +// for memorability. Requiring 5+ keeps this from colliding with a short, genuinely human-chosen passphrase. +const MIN_DESCRIPTIVE_PHRASE_SEGMENTS = 5; + +/** True when `value` reads as a written sentence fragment (a fixture author's own prose description of the + * value) rather than a credential or a chosen passphrase: split on `-`/`_` into 5+ segments, every segment + * purely lowercase ASCII letters (a real token's mixed case/digits would fail this, correctly leaving it + * flagged), with at least one segment a low-entropy English function word (see ENGLISH_FUNCTION_WORDS). */ +function looksLikeDescriptivePlaceholderPhrase(value: string): boolean { + const segments = value.split(/[-_]/); + if (segments.length < MIN_DESCRIPTIVE_PHRASE_SEGMENTS) return false; + if (!segments.every((segment) => /^[a-z]+$/.test(segment))) return false; + return segments.some((segment) => ENGLISH_FUNCTION_WORDS.has(segment)); +} + /** 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 - * known fixture/enum literal. */ + * distinct characters, a long monotonic character-code run, a lowercase hyphenated mock fixture name, a + * known fixture/enum literal, or a descriptive multi-word prose phrase (see + * looksLikeDescriptivePlaceholderPhrase). */ 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 (KNOWN_FIXTURE_SECRET_VALUES.has(value)) return true; + if (looksLikeDescriptivePlaceholderPhrase(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 f20da4120c..7c9e8ae2e7 100644 --- a/review-enrichment/test/secret-scan.test.ts +++ b/review-enrichment/test/secret-scan.test.ts @@ -154,6 +154,33 @@ test("scanPatch still flags a generic multi-segment lowercase passphrase that do assert.equal(findings[0].kind, "generic_secret_assignment"); }); +// gittensory PR #5346/#5341: two inert test-fixture strings ("present-value-not-a-real-token", +// "test-value-should-never-appear-in-doctor-output") matched the generic_secret_assignment SHAPE but neither +// contained a PLACEHOLDER_VALUE_PATTERN keyword, so both auto-closed a legitimate contributor PR twice in a +// row. looksLikeDescriptivePlaceholderPhrase (mirrors src/review/secret-patterns.ts) recognizes both as +// written-prose fixture descriptions instead. +test("scanPatch does NOT flag the two real false-positive fixture values from gittensory PR #5346/#5341 (regression)", () => { + const first = scanPatch("src/config.ts", hunk([`const token = "present-value-not-a-real-token";`])); + assert.equal( + first.some((f) => f.kind === "generic_secret_assignment"), + false, + ); + const second = scanPatch( + "src/config.ts", + hunk([`const secretToken = "test-value-should-never-appear-in-doctor-output";`]), + ); + assert.equal( + second.some((f) => f.kind === "generic_secret_assignment"), + false, + ); +}); + +test("scanPatch still flags a 5+ all-lowercase-word passphrase with no function word (a genuine diceware-style secret)", () => { + const findings = scanPatch("src/config.ts", hunk([`const secret = "correct-horse-battery-staple-secret";`])); + 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( diff --git a/src/review/content-lane/security-scan.ts b/src/review/content-lane/security-scan.ts index 152253aa61..9d44a54485 100644 --- a/src/review/content-lane/security-scan.ts +++ b/src/review/content-lane/security-scan.ts @@ -67,24 +67,34 @@ function firstLineMatching(text: string, re: RegExp): { n: number; text: string } function firstSecretLine(text: string): { n: number; kinds: string[] } | null { - // Per-line scan first — O(n): catches every LINE-CONTAINED hard kind (github_token, jwt, …) and cites its + // Per-line scan — O(n): catches every LINE-CONTAINED concrete kind (github_token, jwt, …) and cites its // exact line. lines[i] is defined for an in-range index (split never yields holes) — assert past - // noUncheckedIndexedAccess. + // noUncheckedIndexedAccess. HARD_SECRET_KINDS no longer includes generic_secret_assignment (see + // ../secret-patterns.ts's doc comment) — that kind is handled separately by + // firstGenericSecretAssignmentLine below and routes to MANUAL, not this function's auto-close caller. const lines = text.split(/\r?\n/); for (let i = 0; i < lines.length; i += 1) { const hits = scanForSecrets(lines[i]!).kinds.filter((k) => HARD_SECRET_KINDS.has(k)); if (hits.length) return { n: i + 1, kinds: hits }; } - // The only HARD kind whose keyword-to-value span can WRAP across lines is generic_secret_assignment, so the - // per-line scan above can miss it (`client_secret =\n"…"`). Recover it with ONE whole-blob pass over just that - // detector — LINEAR, not the quadratic prefix-rescan — citing the line where the non-placeholder match - // COMPLETES, so scanSubmissionContent's auto-close stays in parity with the whole-blob PR-diff gate. + return null; +} + +/** + * generic_secret_assignment is a keyword-plus-quoted-value SHAPE heuristic, not a concrete credential format + * (see ../secret-patterns.ts's HARD_SECRET_KINDS doc comment — split out post-gittensory-PR-#5346, which + * auto-closed a legitimate contributor PR over two inert test-fixture strings). Per this file's own header + * ("only ONE signal is unambiguous enough to hard-close... every other heuristic routes to MANUAL"), a hit + * here routes to MANUAL, never scanSubmissionContent's auto-close. Its keyword-to-value span can wrap across + * lines (`client_secret =\n"…"`), so this is a single whole-blob pass — LINEAR, not a quadratic prefix-rescan + * — citing the line where the non-placeholder match COMPLETES. + */ +function firstGenericSecretAssignmentLine(text: string): number | null { GENERIC_SECRET_ASSIGNMENT_PATTERN.lastIndex = 0; let match: RegExpExecArray | null; while ((match = GENERIC_SECRET_ASSIGNMENT_PATTERN.exec(text)) !== null) { if (!isPlaceholderSecretValue(match[1]!)) { - const n = text.slice(0, match.index + match[0].length).split(/\r?\n/).length; - return { n, kinds: ["generic_secret_assignment"] }; + return text.slice(0, match.index + match[0].length).split(/\r?\n/).length; } } return null; @@ -93,6 +103,8 @@ function firstSecretLine(text: string): { n: number; kinds: string[] } | null { /** * Deterministic security scan of the SUBMITTED content. Returns: * - `close` (embedded_secret) on a concrete embedded credential — cited to a line; or + * - `manual` (possible_secret_assignment) on a secret-shaped-but-not-concrete-format assignment — cited to + * a line (see firstGenericSecretAssignmentLine's doc comment for why this is MANUAL, not close); or * - `manual` (unsafe_install_pipeline) on a pipe-to-shell install in an executable category; or * - null otherwise. * Prompt-injection / exfiltration prose is intentionally NOT matched here: it is indistinguishable @@ -111,6 +123,15 @@ export function scanSubmissionContent(params: { content: string; category: strin }; } + const genericLine = firstGenericSecretAssignmentLine(content); + if (genericLine !== null) { + return { + verdict: "manual", + reasonCode: "possible_secret_assignment", + summary: `Submission contains a secret-shaped assignment (generic_secret_assignment) at line ${genericLine} that doesn't match a concrete credential format — routing to maintainer review to verify it isn't a real secret.`, + }; + } + if (EXECUTABLE_CATEGORIES.has(category)) { const pipe = firstLineMatching(content, PIPED_INSTALL_RE); if (pipe) { @@ -124,8 +145,9 @@ export function scanSubmissionContent(params: { content: string; category: strin return null; } -/** A concrete credential exposed in a LINKED third-party body → manual (flag for a human; don't - * auto-close someone's submission over the linked artifact's own leak). */ +/** A concrete credential — or a secret-shaped-but-not-concrete-format assignment — exposed in a LINKED + * third-party body → manual either way (flag for a human; don't auto-close someone's submission over the + * linked artifact's own leak). */ export function scanLinkedBodiesForSecrets(bodies: string[]): SecurityFinding | null { for (const body of bodies) { const hits = scanForSecrets(body).kinds.filter((k) => HARD_SECRET_KINDS.has(k)); @@ -136,6 +158,13 @@ export function scanLinkedBodiesForSecrets(bodies: string[]): SecurityFinding | summary: `The linked source appears to expose a credential (${hits.join(", ")}) — routing to maintainer review.`, }; } + if (hasGenericSecretAssignment(body)) { + return { + verdict: "manual", + reasonCode: "possible_secret_assignment", + summary: "The linked source contains a secret-shaped assignment (generic_secret_assignment) that doesn't match a concrete credential format — routing to maintainer review.", + }; + } } return null; } diff --git a/src/review/safety.ts b/src/review/safety.ts index 34964cfca7..a213aae7b3 100644 --- a/src/review/safety.ts +++ b/src/review/safety.ts @@ -7,8 +7,8 @@ import type { AdvisoryFinding } from "../types"; import { neutralizePromptInjection, safeReviewTitle } from "./prompt-injection"; -import { HARD_SECRET_KINDS } from "./secret-patterns"; -import { scanDiffForSecretsWithLocations } from "./secrets-scan"; +import { ADVISORY_ONLY_SECRET_KINDS, HARD_SECRET_KINDS } from "./secret-patterns"; +import { scanDiffForSecretsWithLocations, type SecretScanLocationMatch } from "./secrets-scan"; // Concrete credential formats only — NOT the weak heuristics (`seed_or_mnemonic` / `bittensor_key`) that // false-positive on legitimate config/workflow content. A `coldkey:` / `hotkey =` line or the word @@ -78,15 +78,37 @@ export function defangReviewInput(input: SafetyReviewInput): { // produces a readable comment; anything past the cap is summarized as an omitted count instead of listed. const MAX_REPORTED_SECRET_LOCATIONS = 5; +function locationSummaryFor(hits: SecretScanLocationMatch[]): string { + const locations = hits.map((hit) => + hit.line === 0 ? `${hit.path} (filename)` : `${hit.path}:${hit.line}`, + ); + const uniqueLocations = [...new Set(locations)]; + const shown = uniqueLocations.slice(0, MAX_REPORTED_SECRET_LOCATIONS); + const omitted = uniqueLocations.length - shown.length; + return omitted > 0 + ? `Found at: ${shown.join(", ")} (+${omitted} more location${omitted === 1 ? "" : "s"})` + : `Found at: ${shown.join(", ")}`; +} + /** - * Scan the PR diff for leaked secrets and, on a hit, return ONE critical `secret_leak` advisory finding (else - * null). Mapped to gittensory's {@link AdvisoryFinding} shape. The gate treats this code as a hard blocker - * (see rules/advisory.ts) so a leaked secret holds the PR. Only CONCRETE credential formats - * ({@link HARD_SECRET_KINDS}) qualify — the weak `seed_or_mnemonic` / `bittensor_key` heuristics are ignored - * here because they false-positive on legitimate config/workflow content (e.g. `coldkey:` / `hotkey =` lines - * in *.toml, .github/workflows/**, or wrangler/workers config). This is UNCONDITIONAL (#audit-3.4): a concrete, - * real-format committed credential is a leak on any repo, so the caller runs it regardless of the safety flag / - * review allowlist (unlike the prompt-injection defang, which stays flag-gated). + * Scan the PR diff for leaked secrets and, on a hit, return ONE `AdvisoryFinding` (else null). Mapped to + * gittensory's {@link AdvisoryFinding} shape. + * + * Only CONCRETE credential formats ({@link HARD_SECRET_KINDS}) produce the critical `secret_leak` code that + * `rules/advisory.ts`'s `isConfiguredGateBlocker` treats as an unconditional hard blocker — the weak + * `seed_or_mnemonic` / `bittensor_key` heuristics are ignored entirely here because they false-positive on + * legitimate config/workflow content (e.g. `coldkey:` / `hotkey =` lines in *.toml, .github/workflows/**, or + * wrangler/workers config). This is UNCONDITIONAL (#audit-3.4): a concrete, real-format committed credential + * is a leak on any repo, so the caller runs it regardless of the safety flag / review allowlist (unlike the + * prompt-injection defang, which stays flag-gated). + * + * `ADVISORY_ONLY_SECRET_KINDS` (currently just `generic_secret_assignment`) is a keyword-plus-quoted-value + * SHAPE heuristic, not a concrete format — see `secret-patterns.ts`'s `HARD_SECRET_KINDS` doc comment for why + * it was split out (PR #5346 auto-closed a legitimate contributor PR on two inert test-fixture strings). A + * hit on ONLY this kind (no concrete kind present) instead returns a warning-severity `possible_secret_ + * assignment` finding, which `isConfiguredGateBlocker` does not recognize as a blocker code — it surfaces in + * the PR panel for a human/AI reviewer to verify, exactly as REES's own "medium confidence" rating for this + * same signal already treats it, without risking another auto-close false positive. * * #3041: scans the RAW diff directly — `scanDiffForSecretsWithLocations` does its own +/- line-type * distinguishing (only added lines and added/renamed file paths are scanned, matching the previous @@ -94,31 +116,32 @@ const MAX_REPORTED_SECRET_LOCATIONS = 5; * straight at the flagged content instead of forcing them to re-derive it from the whole diff. */ export function secretLeakFinding(diff: string): AdvisoryFinding | null { + const allHits = scanDiffForSecretsWithLocations(diff); // Only CONCRETE credential formats hard-block. The raw scanner also returns the weak `seed_or_mnemonic` / // `bittensor_key` heuristics, which false-positive on `coldkey:` / `hotkey =` / "mnemonic" lines in - // legitimate config/workflow files (RC6); those are filtered out here so they never produce a `secret_leak` - // blocker. A real token (github_token, aws_access_key, …) still blocks regardless of which file it is in. - const hits = scanDiffForSecretsWithLocations(diff).filter((match) => - HARD_SECRET_KINDS.has(match.kind), - ); - if (hits.length === 0) return null; - const kinds = [...new Set(hits.map((hit) => hit.kind))].sort(); - const locations = hits.map((hit) => - hit.line === 0 ? `${hit.path} (filename)` : `${hit.path}:${hit.line}`, - ); - const uniqueLocations = [...new Set(locations)]; - const shown = uniqueLocations.slice(0, MAX_REPORTED_SECRET_LOCATIONS); - const omitted = uniqueLocations.length - shown.length; - const locationSummary = - omitted > 0 - ? `Found at: ${shown.join(", ")} (+${omitted} more location${omitted === 1 ? "" : "s"})` - : `Found at: ${shown.join(", ")}`; - return { - code: "secret_leak", - severity: "critical", - title: `Possible leaked secret in the diff (${kinds.join(", ")})`, - detail: `The PR diff matches secret pattern(s): ${kinds.join(", ")}. ${locationSummary}. A committed credential must be rotated and removed from the change before merge.`, - action: - "Remove the secret from the diff, rotate the exposed credential, then re-run the gate.", - }; + // legitimate config/workflow files (RC6); those are filtered out here so they never produce a finding at + // all. A real token (github_token, aws_access_key, …) still blocks regardless of which file it is in. + const concreteHits = allHits.filter((match) => HARD_SECRET_KINDS.has(match.kind)); + if (concreteHits.length > 0) { + const kinds = [...new Set(concreteHits.map((hit) => hit.kind))].sort(); + return { + code: "secret_leak", + severity: "critical", + title: `Possible leaked secret in the diff (${kinds.join(", ")})`, + detail: `The PR diff matches secret pattern(s): ${kinds.join(", ")}. ${locationSummaryFor(concreteHits)}. A committed credential must be rotated and removed from the change before merge.`, + action: + "Remove the secret from the diff, rotate the exposed credential, then re-run the gate.", + }; + } + const advisoryHits = allHits.filter((match) => ADVISORY_ONLY_SECRET_KINDS.has(match.kind)); + if (advisoryHits.length > 0) { + return { + code: "possible_secret_assignment", + severity: "warning", + title: "Possible secret-shaped assignment in the diff (generic_secret_assignment)", + detail: `The PR diff contains a keyword-plus-quoted-value assignment that resembles a credential but doesn't match a concrete credential format. ${locationSummaryFor(advisoryHits)}. This is a medium-confidence heuristic (it also matches inert test/fixture values) and does not block the gate on its own.`, + action: "Verify the value is not a real credential.", + }; + } + return null; } diff --git a/src/review/secret-patterns.ts b/src/review/secret-patterns.ts index 2f07dd42d2..7754c67d1b 100644 --- a/src/review/secret-patterns.ts +++ b/src/review/secret-patterns.ts @@ -82,15 +82,48 @@ const KNOWN_FIXTURE_SECRET_VALUES = new Set([ "unsafe_install_or_secret", ]); +// Closed set of grammatical FUNCTION words — articles, negations, prepositions, auxiliary verbs — chosen for +// having near-zero information content per word. A human-authored placeholder that describes itself in prose +// (e.g. "present-value-not-a-real-token", "test-value-should-never-appear-in-doctor-output" — both real +// false-positive literals from PR #5346/#5341) naturally reaches for these; a deliberately memorable +// human-CHOSEN passphrase like "correct-horse-battery-secret" is composed of high-entropy CONTENT words +// (nouns/verbs/adjectives) specifically BECAUSE function words carry little entropy per word, so a diceware- +// style passphrase essentially never contains one. Their presence is therefore a reliable structural signal +// for "this is descriptive prose about the value", not "this is someone's chosen secret" — see +// looksLikeDescriptivePlaceholderPhrase below. +const ENGLISH_FUNCTION_WORDS = new Set([ + "a", "an", "the", "is", "are", "was", "were", "be", "been", "not", "no", "and", "or", "to", "of", + "in", "on", "at", "for", "with", "should", "never", "always", "will", "would", "does", "did", "do", + "this", "that", "it", "as", "if", "then", "so", "but", "has", "have", "had", "can", "could", +]); + +// A written-prose fixture description needs several words to say something (both PR #5346 literals split into +// 6 and 8 segments respectively); a memorable diceware-style passphrase conventionally tops out around 4 words +// for memorability. Requiring 5+ keeps this from colliding with a short, genuinely human-chosen passphrase. +const MIN_DESCRIPTIVE_PHRASE_SEGMENTS = 5; + +/** True when `value` reads as a written sentence fragment (a fixture author's own prose description of the + * value) rather than a credential or a chosen passphrase: split on `-`/`_` into 5+ segments, every segment + * purely lowercase ASCII letters (a real token's mixed case/digits would fail this, correctly leaving it + * flagged), with at least one segment a low-entropy English function word (see ENGLISH_FUNCTION_WORDS). */ +export function looksLikeDescriptivePlaceholderPhrase(value: string): boolean { + const segments = value.split(/[-_]/); + if (segments.length < MIN_DESCRIPTIVE_PHRASE_SEGMENTS) return false; + if (!segments.every((segment) => /^[a-z]+$/.test(segment))) return false; + return segments.some((segment) => ENGLISH_FUNCTION_WORDS.has(segment)); +} + /** 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 known fixture/enum literal. Mirrored (drift-checked, not imported) in + * (e.g. "abcdefghijklmnop123"), a known fixture/enum literal, or a descriptive multi-word prose phrase (see + * looksLikeDescriptivePlaceholderPhrase). 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 (KNOWN_FIXTURE_SECRET_VALUES.has(value)) return true; + if (looksLikeDescriptivePlaceholderPhrase(value)) return true; return hasLongSequentialRun(value); } @@ -114,12 +147,23 @@ export function hasGenericSecretAssignment(text: string): boolean { // 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). -// #2553: google_api_key/jwt are as format-precise as the original five (near-zero false-positive risk), and -// generic_secret_assignment already excludes placeholder/type-declaration/schema-shaped matches (see -// isPlaceholderSecretValue above) before the kind is ever produced, so all three are safe unconditional hard -// blockers. voyage_api_key/firecrawl_api_key (#4604) are equally format-precise. Shared by both hard-block -// paths: src/review/safety.ts's secretLeakFinding (PR-diff) and +// #2553: google_api_key/jwt are as format-precise as the original five (near-zero false-positive risk), so +// both are safe unconditional hard blockers. voyage_api_key/firecrawl_api_key (#4604) are equally +// format-precise. Shared by both hard-block paths: src/review/safety.ts's secretLeakFinding (PR-diff) and // src/review/content-lane/security-scan.ts's firstSecretLine/scanLinkedBodiesForSecrets (content-lane). +// +// generic_secret_assignment is deliberately NOT a member (post-PR-5346): unlike every kind above, it is a +// keyword-plus-quoted-value SHAPE heuristic, not a concrete credential format, so isPlaceholderSecretValue's +// closed escape-hatch keyword list can never keep pace with the open-ended ways a contributor phrases an +// inert test value -- this exact gap closed a legitimate contributor PR twice in a row (#5341, then its +// resubmission #5346, on two DIFFERENT non-placeholder-keyword fixture strings) after at least half a dozen +// prior narrow-allowlist patches to this same heuristic (#4587, #3866, #3673, #3178, #2613, #4733) failed to +// stop the pattern for good. REES's own copy of this rule (review-enrichment/src/analyzers/secret-scan.ts) +// already rates it "medium confidence" ("catches real keys but also the occasional long opaque non-secret"), +// and content-lane/security-scan.ts's own header states the underlying design principle this violated: a +// gate that AUTO-CLOSES with no human queue may only hard-close on a signal unambiguous enough that a false +// positive is essentially impossible -- "every other heuristic routes to MANUAL". See +// ADVISORY_ONLY_SECRET_KINDS below for where it still surfaces. export const HARD_SECRET_KINDS = new Set([ "github_token", "github_pat", @@ -135,5 +179,10 @@ export const HARD_SECRET_KINDS = new Set([ "voyage_api_key", "firecrawl_api_key", "jwt", - "generic_secret_assignment", ]); + +// The one kind excluded from HARD_SECRET_KINDS above: still detected and still worth a human's attention, but +// never an unconditional auto-block/auto-close on its own -- see that constant's doc comment for why. Consumed +// by src/review/safety.ts's secretLeakFinding and src/review/content-lane/security-scan.ts to route a hit here +// to an advisory/manual-review signal instead of a hard blocker. +export const ADVISORY_ONLY_SECRET_KINDS = new Set(["generic_secret_assignment"]); diff --git a/test/unit/content-lane-security-scan.test.ts b/test/unit/content-lane-security-scan.test.ts index 9932d35150..3f4ecdc39c 100644 --- a/test/unit/content-lane-security-scan.test.ts +++ b/test/unit/content-lane-security-scan.test.ts @@ -214,10 +214,11 @@ describe("scanSubmissionContent", () => { expect(finding?.reasonCode).toBe("embedded_secret"); }); - it("hard-closes on each #2553-parity kind (google_api_key, jwt, generic_secret_assignment)", () => { + it("hard-closes on each #2553-parity CONCRETE kind (google_api_key, jwt)", () => { // Each must be a HARD_SECRET_KIND so scanSubmissionContent auto-closes, matching the PR-diff gate. + // generic_secret_assignment is deliberately excluded here (#5346) — see the dedicated test below. const jwt = "eyJhbGciOiJIUzI1NiJ9" + "." + "eyJzdWIiOiIxMjM0NTY3ODkwIn0" + "." + "SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c"; - for (const line of ["config: AIza" + "SyABCDEFGHIJKLMNOPQRSTUVWXYZ0123456", `bearer ${jwt}`, `client_secret = "${GENERIC_VALUE}"`]) { + for (const line of ["config: AIza" + "SyABCDEFGHIJKLMNOPQRSTUVWXYZ0123456", `bearer ${jwt}`]) { const finding = scanSubmissionContent({ content: `intro line\n${line}`, category: "skills" }); expect(finding?.verdict, line).toBe("close"); expect(finding?.reasonCode, line).toBe("embedded_secret"); @@ -225,6 +226,19 @@ describe("scanSubmissionContent", () => { } }); + // #5346: generic_secret_assignment is a keyword-plus-quoted-value SHAPE heuristic, not a concrete format — + // it routes to MANUAL, never scanSubmissionContent's auto-close (this file's own header states the design + // principle: only a concrete credential is unambiguous enough to hard-close). + it("routes a generic_secret_assignment hit to MANUAL, never close (#5346)", () => { + const finding = scanSubmissionContent({ + content: `intro line\nclient_secret = "${GENERIC_VALUE}"`, + category: "skills", + }); + expect(finding?.verdict).toBe("manual"); + expect(finding?.reasonCode).toBe("possible_secret_assignment"); + expect(finding?.summary).toContain("line 2"); + }); + it("hard-closes on a Voyage or Firecrawl API key (#4604)", () => { for (const line of ["config: pa-" + "aK9xQ2mZw7Ln4Rv8Pt3B", "key: fc-" + "aK9xQ2mZw7Ln4Rv8"]) { const finding = scanSubmissionContent({ content: `intro line\n${line}`, category: "skills" }); @@ -234,14 +248,15 @@ describe("scanSubmissionContent", () => { } }); - it("hard-closes on a MULTILINE generic secret assignment whose value wraps to the next line (auto-close parity)", () => { - // generic_secret_assignment is the one HARD kind whose keyword-to-value span can wrap. scanForSecrets over the - // whole blob catches it; scanSubmissionContent must too, or a wrapped secret bypasses the auto-close gate the - // PR-diff gate (whole-blob) would block. Built from separate literals so this file embeds no contiguous secret. + it("routes a MULTILINE generic secret assignment whose value wraps to the next line to MANUAL (#5346)", () => { + // generic_secret_assignment's keyword-to-value span can wrap. scanForSecrets over the whole blob catches + // it; scanSubmissionContent must too, or a wrapped hit bypasses this signal entirely — but it still routes + // to MANUAL (never close), matching the single-line case above. Built from separate literals so this file + // embeds no contiguous secret. const content = `intro line\nclient_secret =\n"${GENERIC_VALUE}"`; const finding = scanSubmissionContent({ content, category: "guides" }); - expect(finding?.verdict).toBe("close"); - expect(finding?.reasonCode).toBe("embedded_secret"); + expect(finding?.verdict).toBe("manual"); + expect(finding?.reasonCode).toBe("possible_secret_assignment"); expect(finding?.summary).toContain("line 3"); // cited where the wrapped match completes (the value line) }); @@ -260,6 +275,12 @@ describe("scanLinkedBodiesForSecrets", () => { expect(finding?.reasonCode).toBe("embedded_secret"); }); + it("flags a generic_secret_assignment-only hit in a LINKED body as MANUAL too (#5346)", () => { + const finding = scanLinkedBodiesForSecrets(["clean body", `client_secret = "${GENERIC_VALUE}"`]); + expect(finding?.verdict).toBe("manual"); + expect(finding?.reasonCode).toBe("possible_secret_assignment"); + }); + it("returns null when no linked body leaks", () => { expect(scanLinkedBodiesForSecrets(["clean", "also clean"])).toBeNull(); }); diff --git a/test/unit/safety-wiring.test.ts b/test/unit/safety-wiring.test.ts index 55971bae14..be9f24e7a8 100644 --- a/test/unit/safety-wiring.test.ts +++ b/test/unit/safety-wiring.test.ts @@ -352,27 +352,28 @@ 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", () => { + it("surfaces a lowercase-hyphenated password assignment as an advisory (never a hard block, #5346)", () => { 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?.code).toBe("possible_secret_assignment"); + expect(finding?.severity).toBe("warning"); expect(finding?.title).toContain("generic_secret_assignment"); expect(finding?.detail).toContain("config/prod.env:1"); }); - it("blocks mixed-case mock-tokenized generic credentials", () => { + it("surfaces a mixed-case mock-tokenized generic credential as an advisory (never a hard block, #5346)", () => { const diff = [ "### config/prod.env (modified) +1/-0", "@@ -0,0 +1 @@", '+password = "prod-mock-aK9xQ2mZw7Ln4Rv8Pt3Bh6"', ].join("\n"); const finding = secretLeakFinding(diff); - expect(finding?.code).toBe("secret_leak"); + expect(finding?.code).toBe("possible_secret_assignment"); + expect(finding?.severity).toBe("warning"); expect(finding?.title).toContain("generic_secret_assignment"); expect(finding?.detail).toContain("config/prod.env:1"); }); @@ -466,15 +467,16 @@ describe("gate treats secret_leak as a hard blocker", () => { 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. + // #2553: the concrete-format widened kinds (google_api_key, jwt) hard-block exactly like the original + // five — same secretLeakFinding -> evaluateGateCheck path, no separate opt-in. generic_secret_assignment is + // deliberately EXCLUDED from this table (see the dedicated describe block below, #5346): it is a + // keyword-plus-quoted-value SHAPE heuristic, not a concrete format, so it no longer hard-blocks. 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"}"`], ["voyage_api_key", `### src/config.ts (modified) +1/-0\n@@\n+const voyage = "${"pa-" + "aK9xQ2mZw7Ln4Rv8Pt3B"}";`], ["firecrawl_api_key", `### src/config.ts (modified) +1/-0\n@@\n+const firecrawl = "${"fc-" + "aK9xQ2mZw7Ln4Rv8"}";`], ])("hard-blocks a %s finding", (kind, diff) => { @@ -492,6 +494,43 @@ describe("gate treats secret_leak as a hard blocker", () => { }); }); +// #5346: gittensory PR #5346 was auto-closed on a "possible leaked secret" that was really two inert +// test-fixture strings matching the generic_secret_assignment SHAPE heuristic. This heuristic (unlike every +// concrete-format kind above) is no longer an unconditional hard blocker — it surfaces as an advisory finding +// that a human/AI reviewer can verify, matching how REES's own copy of this rule already rates it +// "medium confidence". +describe("generic_secret_assignment is advisory-only, never a hard blocker (#5346)", () => { + const diff = `### src/config.ts (modified) +1/-0\n@@\n+secret = "${"sk_live_" + "aK9xQ2mZw7Ln4Rv8Pt3Bh6"}"`; + + it("secretLeakFinding returns a warning-severity possible_secret_assignment finding, not secret_leak", () => { + const finding = secretLeakFinding(diff); + expect(finding?.code).toBe("possible_secret_assignment"); + expect(finding?.severity).toBe("warning"); + expect(finding?.title).toContain("generic_secret_assignment"); + }); + + it("does not fail the gate — it surfaces only as a non-blocking warning", () => { + const finding = secretLeakFinding(diff)!; + const gate = evaluateGateCheck(advisory([finding]), { confirmedContributor: true }); + expect(gate.conclusion).toBe("success"); + expect(gate.blockers).toEqual([]); + expect(gate.warnings.map((w) => w.code)).toContain("possible_secret_assignment"); + }); + + it("a concrete kind alongside a generic-only kind still hard-blocks on the concrete kind (secret_leak wins)", () => { + const mixedDiff = [ + "### src/config.ts (modified) +2/-0", + "@@ -0,0 +1,2 @@", + `+const token = "${FAKE_GH_TOKEN}";`, + '+password = "alpha-bravo-charlie-delta"', + ].join("\n"); + const finding = secretLeakFinding(mixedDiff); + expect(finding?.code).toBe("secret_leak"); + expect(finding?.severity).toBe("critical"); + expect(finding?.title).toContain("github_token"); + }); +}); + describe("secretLeakFinding scans only ADDED lines", () => { // Assembled at runtime so THIS test file's source carries no contiguous scannable token (the gate scans the // PR diff and a literal fixture here would block this very PR). diff --git a/test/unit/secret-patterns.test.ts b/test/unit/secret-patterns.test.ts index 657df7c1bf..81b802e626 100644 --- a/test/unit/secret-patterns.test.ts +++ b/test/unit/secret-patterns.test.ts @@ -1,10 +1,12 @@ import { describe, expect, it } from "vitest"; import { + ADVISORY_ONLY_SECRET_KINDS, GENERIC_SECRET_ASSIGNMENT_PATTERN, HARD_SECRET_KINDS, hasGenericSecretAssignment, hasLongSequentialRun, isPlaceholderSecretValue, + looksLikeDescriptivePlaceholderPhrase, SECRET_PATTERNS, } from "../../src/review/secret-patterns"; @@ -22,21 +24,33 @@ describe("secret-patterns — shared secret-detection primitives (#4608)", () => expect(new Set(names).size).toBe(names.length); }); - it("HARD_SECRET_KINDS excludes the weak seed-phrase/bittensor-key heuristics", () => { + it("HARD_SECRET_KINDS excludes the weak seed-phrase/bittensor-key heuristics and generic_secret_assignment", () => { expect(HARD_SECRET_KINDS.has("seed_or_mnemonic")).toBe(false); expect(HARD_SECRET_KINDS.has("bittensor_key")).toBe(false); - expect(HARD_SECRET_KINDS.has("generic_secret_assignment")).toBe(true); + // generic_secret_assignment is a keyword-plus-quoted-value SHAPE heuristic, not a concrete credential + // format -- gittensory PR #5346 auto-closed a legitimate contributor PR over two inert test-fixture + // strings that matched this shape but weren't real secrets. Split out into ADVISORY_ONLY_SECRET_KINDS + // below (regression guard for that incident). + expect(HARD_SECRET_KINDS.has("generic_secret_assignment")).toBe(false); }); - it("every non-generic HARD_SECRET_KINDS entry is a real SECRET_PATTERNS name", () => { + it("every HARD_SECRET_KINDS entry is a real SECRET_PATTERNS name", () => { const patternNames = new Set(SECRET_PATTERNS.map((pattern) => pattern.name)); for (const kind of HARD_SECRET_KINDS) { - if (kind === "generic_secret_assignment") continue; expect(patternNames.has(kind)).toBe(true); } }); }); + describe("ADVISORY_ONLY_SECRET_KINDS", () => { + it("contains exactly generic_secret_assignment, disjoint from HARD_SECRET_KINDS", () => { + expect([...ADVISORY_ONLY_SECRET_KINDS]).toEqual(["generic_secret_assignment"]); + for (const kind of ADVISORY_ONLY_SECRET_KINDS) { + expect(HARD_SECRET_KINDS.has(kind)).toBe(false); + } + }); + }); + describe("hasLongSequentialRun", () => { it("returns false when the value is too short to reach the threshold", () => { expect(hasLongSequentialRun("")).toBe(false); @@ -112,6 +126,39 @@ describe("secret-patterns — shared secret-detection primitives (#4608)", () => it("does NOT flag a genuinely high-entropy credential-shaped value", () => { expect(isPlaceholderSecretValue("aK9xQ2mZw7Ln4Rv8Pt3Bh6")).toBe(false); }); + + it("flags the two real false-positive fixture values from gittensory PR #5346/#5341 (regression)", () => { + // Neither contains a PLACEHOLDER_VALUE_PATTERN keyword (no "fake"/"dummy"/"sample"/etc.), so before + // looksLikeDescriptivePlaceholderPhrase these both slipped through as "looks like a secret" and + // hard-closed a legitimate contributor PR twice in a row. + expect(isPlaceholderSecretValue("present-value-not-a-real-token")).toBe(true); + expect(isPlaceholderSecretValue("test-value-should-never-appear-in-doctor-output")).toBe(true); + }); + }); + + describe("looksLikeDescriptivePlaceholderPhrase", () => { + it("flags the two real false-positive fixture values from gittensory PR #5346/#5341 (regression)", () => { + expect(looksLikeDescriptivePlaceholderPhrase("present-value-not-a-real-token")).toBe(true); + expect(looksLikeDescriptivePlaceholderPhrase("test-value-should-never-appear-in-doctor-output")).toBe(true); + }); + + it("does NOT flag a short (<5-segment) phrase even if it contains a function word", () => { + expect(looksLikeDescriptivePlaceholderPhrase("is-not-real")).toBe(false); + }); + + it("does NOT flag a 5+ segment phrase with a non-lowercase-alpha segment (digits/mixed case)", () => { + // "abc123" fails the pure-lowercase-letters check even though the rest of the phrase reads as prose -- + // a real high-entropy token embedded in a longer string must stay flagged. + expect(looksLikeDescriptivePlaceholderPhrase("abc123-is-not-a-real-token")).toBe(false); + }); + + it("does NOT flag a 5+ all-lowercase-word phrase with no function word (a genuine diceware-style passphrase)", () => { + expect(looksLikeDescriptivePlaceholderPhrase("correct-horse-battery-staple-secret")).toBe(false); + }); + + it("flags a 5+ all-lowercase-word phrase containing a function word, using underscores instead of hyphens", () => { + expect(looksLikeDescriptivePlaceholderPhrase("this_is_not_a_real_secret")).toBe(true); + }); }); describe("GENERIC_SECRET_ASSIGNMENT_PATTERN", () => { @@ -139,6 +186,13 @@ describe("secret-patterns — shared secret-detection primitives (#4608)", () => expect(hasGenericSecretAssignment('benign prose first.\nsecret = "aK9xQ2mZw7Ln4Rv8Pt3Bh6"')).toBe(true); }); + it("returns false for the two real false-positive assignments from gittensory PR #5346/#5341 (regression)", () => { + expect(hasGenericSecretAssignment('GITHUB_TOKEN: "present-value-not-a-real-token"')).toBe(false); + expect( + hasGenericSecretAssignment('const secretToken = "test-value-should-never-appear-in-doctor-output";'), + ).toBe(false); + }); + it("resets the shared regex's lastIndex on every call, so a prior call cannot corrupt the next scan", () => { // GENERIC_SECRET_ASSIGNMENT_PATTERN is a module-level /g regex; a call that returns true early leaves // lastIndex at the END of that match. This first call's match runs to the end of a 33-char string.