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.