diff --git a/src/review/safety.ts b/src/review/safety.ts index f37bd55e99..4d0bed6408 100644 --- a/src/review/safety.ts +++ b/src/review/safety.ts @@ -7,7 +7,7 @@ import type { AdvisoryFinding } from "../types"; import { neutralizePromptInjection, safeReviewTitle } from "./prompt-injection"; -import { scanForSecrets } from "./secrets-scan"; +import { scanDiffForSecretsWithLocations } 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 @@ -85,6 +85,10 @@ export function defangReviewInput(input: SafetyReviewInput): { return { title, body, diff, changedFiles }; } +// #3041: cap the number of locations listed in a finding's `detail` so a single PR with dozens of hits still +// produces a readable comment; anything past the cap is summarized as an omitted count instead of listed. +const MAX_REPORTED_SECRET_LOCATIONS = 5; + /** * 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 @@ -94,34 +98,37 @@ export function defangReviewInput(input: SafetyReviewInput): { * 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). + * + * #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 + * added-only-text behavior) while also tracking each hit's file:line, so the finding can point a maintainer + * straight at the flagged content instead of forcing them to re-derive it from the whole diff. */ export function secretLeakFinding(diff: string): AdvisoryFinding | null { - // Scan ONLY additions — the secrets THIS change introduces. A token on a removed/context line is not being - // committed by the PR, so flagging it would wrongly block a change that merely REMOVES or refactors a - // secret-shaped string (e.g. deleting/defanging a test fixture, or rotating a credential out). Added/renamed - // file paths are also committed PR state, but buildSecretScanDiff carries them only in `### path (status)` - // headers, so keep those metadata lines while still dropping modified/removed headers and `+++` patch headers. - const added = diff - .split("\n") - .filter( - (line) => - (line.startsWith("+") && !line.startsWith("+++")) || - /^### .+ \((?:added|renamed)\) /.test(line), - ) - .join("\n"); // 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 kinds = scanForSecrets(added).kinds.filter((kind) => - HARD_SECRET_KINDS.has(kind), + 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}`, ); - if (kinds.length === 0) return null; + 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(", ")}. A committed credential must be rotated and removed from the change before merge.`, + 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.", }; diff --git a/src/review/secrets-scan.ts b/src/review/secrets-scan.ts index fef4f770fa..431ee135d2 100644 --- a/src/review/secrets-scan.ts +++ b/src/review/secrets-scan.ts @@ -63,12 +63,22 @@ function hasLongSequentialRun(value: string): boolean { return false; } +// #3041: a value made ENTIRELY of lowercase words joined by hyphens (2+ segments, e.g. the test-fixture +// literal "installation-token" used 351+ times across this repo's own test suite as a mock fetch-response +// token) reads as an ordinary English-word compound identifier -- a mock/fixture name -- not a generated +// credential. Real secrets/tokens are essentially always alphanumeric, mixed-case, or base64/hex; they are +// never a pure lowercase-hyphenated phrase. Require at least one hyphen (2+ segments) so this stays narrow +// and doesn't broaden into excluding arbitrary single lowercase words that could plausibly be real secrets. +const LOWERCASE_HYPHENATED_COMPOUND_PATTERN = /^[a-z]+(-[a-z]+)+$/; + /** 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. */ + * distinct characters (e.g. "xxxxxxxxxxxxxxxx", "----------------"), a long monotonic character-code run + * (e.g. "abcdefghijklmnop123"), or a lowercase-hyphenated word compound (e.g. "installation-token") — real + * high-entropy secrets never look like any of these. */ function isPlaceholderSecretValue(value: string): boolean { if (PLACEHOLDER_VALUE_PATTERN.test(value)) return true; if (new Set(value.toLowerCase()).size <= 2) return true; + if (LOWERCASE_HYPHENATED_COMPOUND_PATTERN.test(value)) return true; return hasLongSequentialRun(value); } @@ -85,14 +95,82 @@ function hasGenericSecretAssignment(text: string): boolean { return false; } +// #3041: the one place the pattern list (format-specific SECRET_PATTERNS + the generic keyword-assignment +// heuristic) is applied to a string. Both `scanForSecrets` (whole-text scan) and +// `scanDiffForSecretsWithLocations` (per-line diff scan, for file:line attribution) delegate here so there is +// exactly one implementation of "does this text contain secret-shaped content" to keep in sync. +function matchedKindsIn(text: string): string[] { + if (!text) return []; + const kinds = SECRET_PATTERNS.filter((pattern) => pattern.re.test(text)).map((pattern) => pattern.name); + if (hasGenericSecretAssignment(text)) kinds.push("generic_secret_assignment"); + return kinds; +} + export interface SecretScanResult { found: boolean; kinds: string[]; } 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"); + const kinds = matchedKindsIn(text); return { found: kinds.length > 0, kinds }; } + +/** One secret-pattern hit at a specific location in a diff, for surfacing file:line in a finding (#3041). A + * `line` of `0` means the match came from a file-header PATH itself (an added/renamed filename), not from + * diff content — there is no line number for that case. */ +export interface SecretScanLocationMatch { + kind: string; + path: string; + line: number; +} + +const DIFF_FILE_HEADER_PATTERN = /^### (.+) \(([a-z]+)\) \+\d+\/-\d+$/; +const DIFF_HUNK_HEADER_PATTERN = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/; + +/** + * Walk a `buildSecretScanDiff`-shaped diff (see src/queue/processors.ts) line by line, scanning only content + * this PR is INTRODUCING — added (`+`) lines and, for an added/renamed file, the path in its own section + * header — and return every pattern hit with its file path and 1-based line number in the new/post-change + * file. Context (` `) and removed (`-`) lines are tracked for line-number bookkeeping but never scanned: a + * removed or unchanged line is not something this PR is committing. This mirrors the added-only scanning + * `secretLeakFinding` used to do via string filtering, but keeps enough diff structure to report WHERE a hit + * lives instead of collapsing everything to a flat blob. + */ +export function scanDiffForSecretsWithLocations(diff: string): SecretScanLocationMatch[] { + const matches: SecretScanLocationMatch[] = []; + let currentPath = ""; + let currentNewLine = 0; + for (const line of diff.split("\n")) { + const fileHeader = DIFF_FILE_HEADER_PATTERN.exec(line); + if (fileHeader) { + currentPath = fileHeader[1]!; + currentNewLine = 0; + const status = fileHeader[2]!; + if (status === "added" || status === "renamed") { + for (const kind of matchedKindsIn(currentPath)) { + matches.push({ kind, path: currentPath, line: 0 }); + } + } + continue; + } + const hunkHeader = DIFF_HUNK_HEADER_PATTERN.exec(line); + if (hunkHeader) { + currentNewLine = Number(hunkHeader[1]) - 1; + continue; + } + if (line.startsWith("+") && !line.startsWith("+++")) { + currentNewLine += 1; + const content = line.slice(1); + for (const kind of matchedKindsIn(content)) { + matches.push({ kind, path: currentPath, line: currentNewLine }); + } + continue; + } + if (line.startsWith("-")) continue; + // Context line (single leading space) or a blank separator between file sections -- either way it isn't + // new content this PR introduces, but a genuine context line still occupies a line in the new file. + currentNewLine += 1; + } + return matches; +} diff --git a/test/unit/safety-wiring.test.ts b/test/unit/safety-wiring.test.ts index 750fb39d6c..6c81f2bcc7 100644 --- a/test/unit/safety-wiring.test.ts +++ b/test/unit/safety-wiring.test.ts @@ -436,3 +436,78 @@ describe("secretLeakFinding scans only ADDED lines", () => { expect(secretLeakFinding(diff)).toBeNull(); }); }); + +// #3041: the finding's `detail` must surface the exact file:line so a maintainer can jump straight to the +// flagged content instead of re-deriving it from the whole diff. +describe("secretLeakFinding surfaces file:line locations (#3041)", () => { + // Assembled at runtime so THIS test file's source carries no contiguous scannable token. + const fakeToken = "ghp_" + "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"; + + it("reports the exact file path and line number for a planted secret in a multi-file diff", () => { + const diff = [ + "### src/unrelated.ts (modified) +2/-0", + "@@ -1,2 +1,4 @@", + " const a = 1;", + "+const b = 2;", + "+const c = 3;", + "", + "### test/fixture.ts (modified) +3/-0", + "@@ -10,3 +10,6 @@", + " const before = true;", + "+const filler = 0;", + `+const token = "${fakeToken}";`, + "+const after = 1;", + ].join("\n"); + const finding = secretLeakFinding(diff); + expect(finding?.code).toBe("secret_leak"); + // Hunk starts at new-file line 10; " const before" is line 10 (context), "+const filler" is line 11, + // and the secret line is line 12. + expect(finding?.detail).toContain("test/fixture.ts:12"); + expect(finding?.detail).not.toContain("src/unrelated.ts"); + }); + + it("caps the reported locations at 5 and notes how many more were omitted (singular)", () => { + const lines = ["### src/many.ts (modified) +6/-0", "@@ -1,0 +1,6 @@"]; + for (let i = 0; i < 6; i += 1) { + lines.push(`+const secret${i} = "${fakeToken}";`); + } + const diff = lines.join("\n"); + const finding = secretLeakFinding(diff); + expect(finding?.code).toBe("secret_leak"); + // 6 distinct locations (lines 1-6) -> 5 shown + "1 more location" (singular) omitted note. + for (let line = 1; line <= 5; line += 1) { + expect(finding?.detail).toContain(`src/many.ts:${line}`); + } + expect(finding?.detail).not.toContain("src/many.ts:6"); + expect(finding?.detail).toContain("+1 more location)"); + expect(finding?.detail).not.toContain("+1 more locations)"); + }); + + it("caps the reported locations at 5 and notes how many more were omitted (plural)", () => { + const lines = ["### src/many.ts (modified) +8/-0", "@@ -1,0 +1,8 @@"]; + for (let i = 0; i < 8; i += 1) { + lines.push(`+const secret${i} = "${fakeToken}";`); + } + const diff = lines.join("\n"); + const finding = secretLeakFinding(diff); + expect(finding?.code).toBe("secret_leak"); + // 8 distinct locations (lines 1-8) -> 5 shown + "3 more locations" (plural) omitted note. + for (let line = 1; line <= 5; line += 1) { + expect(finding?.detail).toContain(`src/many.ts:${line}`); + } + expect(finding?.detail).not.toContain("src/many.ts:6"); + expect(finding?.detail).toContain("+3 more locations)"); + }); + + it("a removed line's secret-shaped content does not appear in the finding at all", () => { + const diff = `### src/config.ts (modified) +0/-1\n@@ -5,1 +5,0 @@\n-const token = "${fakeToken}";`; + expect(secretLeakFinding(diff)).toBeNull(); + }); + + it("an added file's secret-shaped filename is reported as a filename-level (line 0) location", () => { + const diff = `### fixtures/${fakeToken}.txt (added) +1/-0\n@@ -0,0 +1,1 @@\n+benign fixture content`; + const finding = secretLeakFinding(diff); + expect(finding?.code).toBe("secret_leak"); + expect(finding?.detail).toContain(`fixtures/${fakeToken}.txt (filename)`); + }); +}); diff --git a/test/unit/secrets-scan.test.ts b/test/unit/secrets-scan.test.ts index 2ee058d215..1a375e389a 100644 --- a/test/unit/secrets-scan.test.ts +++ b/test/unit/secrets-scan.test.ts @@ -128,4 +128,31 @@ describe("scanForSecrets — deterministic secret-pattern scanner", () => { it("does NOT flag a short value under the 16-character floor", () => { expect(scanForSecrets('token = "short12345"').kinds).not.toContain("generic_secret_assignment"); }); + + // #3041: PR #3036 was wrongly hard-blocked by the test-fixture literal "installation-token" (used 351+ + // times across this repo's own test suite as a mock fetch-response token) — a lowercase-hyphenated word + // compound reads as a fixture/mock name, not a generated credential. + it.each([ + ["installation-token", 'token: "installation-token"'], + ["access-token", 'token = "access-token"'], + ["some-mock-secret-value", 'secret: "some-mock-secret-value"'], + ])("does NOT flag a lowercase-hyphenated word compound: %s (#3041)", (_name, snippet) => { + expect(scanForSecrets(snippet).kinds).not.toContain("generic_secret_assignment"); + }); + + it("still flags a real-looking generic secret with digits and mixed case (regression guard for #3041)", () => { + // Same fixture as the "high-entropy value" test above — proves the new lowercase-hyphenated exclusion + // doesn't broaden past its intended narrow shape: this value has digits + mixed case, not a pure + // lowercase-hyphenated phrase, so it must still be flagged. + const fakeSecret = "sk_live_" + "aK9xQ2mZw7Ln4Rv8Pt3Bh6"; + expect(scanForSecrets(`fakeSecret = "${fakeSecret}"`).kinds).toContain("generic_secret_assignment"); + }); + + it("a single lowercase word with no hyphen is unaffected by the new hyphenated-compound exclusion (#3041)", () => { + // 20 lowercase letters, no repeats and no sequential run, so it isn't already caught by the entropy/ + // placeholder checks either -- proves LOWERCASE_HYPHENATED_COMPOUND_PATTERN specifically requires a + // hyphen (2+ segments) and does not accidentally match a single unhyphenated word. + const singleWord = "qwzxvbnmalskdjfhgpoiu"; + expect(scanForSecrets(`token = "${singleWord}"`).kinds).toContain("generic_secret_assignment"); + }); });