From 191db95bf06f41e4bd320d748e655dfa19f9e5d0 Mon Sep 17 00:00:00 2001 From: RealDiligent Date: Sat, 4 Jul 2026 11:21:37 +0800 Subject: [PATCH] fix(review): detect cross-line split credentials in gate secret scan (#2454) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The unconditional secret_leak hard blocker scanned added lines in isolation, so credentials split across adjacent added string literals evaded every per-line regex while REES enrichment already joined them (#2454). Add scanPrDiffForSecretKinds with bounded cross-line literal join, hunk/context boundaries, consecutive-run generic assignment detection, and in-hunk +++ content handling. Wire secretLeakFinding through the new walker. Only skip the cross-line join when the current line matched a gate-blocking kind on its own — a soft heuristic alone (e.g. coldkey:) must not suppress joining literals that complete a split concrete credential (#2877 review blocker). Export GATE_BLOCKING_SECRET_KINDS as the single source of truth shared between the diff walker and secretLeakFinding filter. Supersedes closed #2877. Co-authored-by: Cursor --- src/review/safety.ts | 49 ++----- src/review/secrets-scan.ts | 129 +++++++++++++++++++ test/unit/safety-wiring.test.ts | 26 ++++ test/unit/secrets-scan.test.ts | 219 +++++++++++++++++++++++++++++++- 4 files changed, 381 insertions(+), 42 deletions(-) diff --git a/src/review/safety.ts b/src/review/safety.ts index 9faf479b4..c02fb583f 100644 --- a/src/review/safety.ts +++ b/src/review/safety.ts @@ -7,33 +7,11 @@ import type { AdvisoryFinding } from "../types"; import { neutralizePromptInjection, safeReviewTitle } from "./prompt-injection"; -import { scanForSecrets } from "./secrets-scan"; +import { GATE_BLOCKING_SECRET_KINDS, scanPrDiffForSecretKinds } 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 -// "mnemonic" in a .toml, .github/workflows/**, or wrangler/workers config is NOT a leaked credential, but it -// matches those two patterns — on these Bittensor repos that wrongly hard-blocked owner config/workflow PRs -// (RC6: #1505/#1495/#1485). A real-format token IS a leak regardless of the file it lives in, so we keep the -// concrete formats as hard blockers and ignore only the ambiguous heuristics. This mirrors the same gate the -// content lane already uses (src/review/content-lane/security-scan.ts). -// -// #2553: widened to match review-enrichment/src/analyzers/secret-scan.ts's richer, higher-recall rule set. -// google_api_key/jwt are as format-precise as the original five (near-zero false-positive risk). -// generic_secret_assignment is the one keyword-shaped pattern here — secrets-scan.ts already excludes -// placeholder/type-declaration/schema-shaped matches (see isPlaceholderSecretValue there) before this kind -// is ever produced, so it is safe to treat as an unconditional hard blocker like the rest. -const HARD_SECRET_KINDS = new Set([ - "github_token", - "github_pat", - "private_key_block", - "aws_access_key", - "slack_token", - "google_api_key", - "gitlab_token", - "npm_token", - "jwt", - "generic_secret_assignment", -]); +// false-positive on legitimate config/workflow content. See {@link GATE_BLOCKING_SECRET_KINDS} in +// secrets-scan.ts (single source of truth shared with scanPrDiffForSecretKinds cross-line join logic). /** True when the safety scan is enabled. Flag-OFF (default) → every helper below is a no-op pass-through. */ export function isSafetyEnabled(env: { @@ -86,7 +64,7 @@ export function defangReviewInput(input: SafetyReviewInput): { * 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 + * ({@link GATE_BLOCKING_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 / @@ -98,21 +76,10 @@ export function secretLeakFinding(diff: string): AdvisoryFinding | null { // 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), - ); + // scanPrDiffForSecretKinds walks the diff line-by-line (with a bounded cross-line literal join on consecutive + // added lines, #2454) instead of joining all `+` lines into one blob — that join would miss a credential + // split across adjacent assignments and would also ignore hunk/context boundaries the gate must respect. + const kinds = scanPrDiffForSecretKinds(diff).filter((kind) => GATE_BLOCKING_SECRET_KINDS.has(kind)); if (kinds.length === 0) return null; return { code: "secret_leak", diff --git a/src/review/secrets-scan.ts b/src/review/secrets-scan.ts index 560348180..e6e9cf432 100644 --- a/src/review/secrets-scan.ts +++ b/src/review/secrets-scan.ts @@ -27,6 +27,24 @@ const SECRET_PATTERNS: Array<{ name: string; re: RegExp }> = [ { name: "bittensor_key", re: /\b(?:hot|cold)key\b\s*[:=]/i }, ]; +/** Concrete credential formats the gate hard-blocks — NOT weak heuristics (`seed_or_mnemonic` / `bittensor_key`). */ +export const GATE_BLOCKING_SECRET_KINDS = new Set([ + "github_token", + "github_pat", + "private_key_block", + "aws_access_key", + "slack_token", + "google_api_key", + "gitlab_token", + "npm_token", + "jwt", + "generic_secret_assignment", +]); + +function isGateBlockingKind(kind: string): boolean { + return GATE_BLOCKING_SECRET_KINDS.has(kind); +} + // Deliberately NOT in SECRET_PATTERNS above: unlike the format-specific patterns (a real GitHub token/AWS key // ALWAYS matches its exact character format, so a bare .test() is precise enough), a keyword-plus-quoted-value // SHAPE also matches plenty of non-secrets -- a Zod schema field (`password: z.string()`), a TypeScript type @@ -90,3 +108,114 @@ export function scanForSecrets(text: string): SecretScanResult { if (hasGenericSecretAssignment(text)) kinds.push("generic_secret_assignment"); return { found: kinds.length > 0, kinds }; } + +/** Extract quoted string-literal inner text from one source line (for cross-line join below). */ +function extractStringLiteralContents(line: string): string[] { + const literals: string[] = []; + const re = /"(?:[^"\\]|\\.)*"|'(?:[^'\\]|\\.)*'|`(?:[^`\\]|\\.)*`/g; + let match: RegExpExecArray | null; + while ((match = re.exec(line)) !== null) literals.push(match[0].slice(1, -1)); + return literals; +} + +function formatSecretKindsFromText(text: string): string[] { + const kinds = SECRET_PATTERNS.filter((pattern) => pattern.re.test(text)).map((pattern) => pattern.name); + if (hasGenericSecretAssignment(text)) kinds.push("generic_secret_assignment"); + return kinds; +} + +/** True for unified-diff file headers (`+++ b/path`, `--- a/path`), not added content like `+++const`. + * Mirrors review-enrichment/src/analyzers/diff-lines.ts `isDiffFileHeaderLine`. */ +function isUnifiedDiffFileHeaderLine(line: string): boolean { + return /^(?:\+\+\+|---) (?:[ab]\/|\/dev\/null)/.test(line); +} + +/** Scan a PR diff for secret kinds introduced on added lines (and added/renamed file headers). Per-line regex + * first; then a bounded cross-line join of consecutive added lines' adjacent string literals (#2454) so a + * credential split across `const a = "AKIA…"; const b = "REST";` still trips the unconditional gate. Context, + * removed, hunk, and file-section boundaries reset both the literal-join window and generic-assignment runs. */ +export function scanPrDiffForSecretKinds(diff: string): string[] { + const found = new Set(); + let inFileSection = false; + let inHunk = false; + let previousLiterals: string[] = []; + let addedRun: string[] = []; + + const resetJoinState = (): void => { + previousLiterals = []; + addedRun = []; + }; + + const noteGenericFromAddedRun = (): void => { + if (found.has("generic_secret_assignment") || addedRun.length === 0) return; + if (hasGenericSecretAssignment(addedRun.join("\n"))) { + found.add("generic_secret_assignment"); + } + }; + + for (const line of diff.split("\n")) { + if (line === "") { + inFileSection = false; + inHunk = false; + resetJoinState(); + continue; + } + if (/^### .+ \(.+\) /.test(line)) { + inFileSection = true; + inHunk = false; + resetJoinState(); + if (/^### .+ \((?:added|renamed)\) /.test(line)) { + for (const kind of formatSecretKindsFromText(line)) found.add(kind); + } + continue; + } + if (line.startsWith("@@")) { + inHunk = true; + resetJoinState(); + continue; + } + if (line.startsWith("+")) { + if (!inFileSection) continue; + // Skip pre-hunk file headers only; inside a hunk `+++…` is added content, not a header. + if (!inHunk && isUnifiedDiffFileHeaderLine(line)) continue; + const content = line.slice(1); + addedRun.push(content); + let matchedBlocking = false; + for (const kind of formatSecretKindsFromText(content)) { + found.add(kind); + if (isGateBlockingKind(kind)) matchedBlocking = true; + } + noteGenericFromAddedRun(); + const currentLiterals = extractStringLiteralContents(content); + const lastPrevious = previousLiterals.at(-1); + const firstCurrent = currentLiterals[0]; + // Only skip the cross-line join when this line already matched a gate-blocking kind on its own. + // A soft heuristic alone (e.g. `coldkey:` in config) must not suppress joining literals that + // complete a split concrete credential (#2877 / Gittensory review blocker). + if (!matchedBlocking && lastPrevious !== undefined && firstCurrent !== undefined) { + const joined = lastPrevious + firstCurrent; + for (const pattern of SECRET_PATTERNS) { + if (!isGateBlockingKind(pattern.name)) continue; + if (pattern.re.test(joined)) { + found.add(pattern.name); + break; + } + } + if (!found.has("generic_secret_assignment") && hasGenericSecretAssignment(joined)) { + found.add("generic_secret_assignment"); + } + } + previousLiterals = currentLiterals; + continue; + } + if (line.startsWith("-")) { + resetJoinState(); + continue; + } + if (inHunk && line.startsWith(" ")) { + resetJoinState(); + } + } + + return [...found]; +} diff --git a/test/unit/safety-wiring.test.ts b/test/unit/safety-wiring.test.ts index 750fb39d6..1b78a31ad 100644 --- a/test/unit/safety-wiring.test.ts +++ b/test/unit/safety-wiring.test.ts @@ -435,4 +435,30 @@ describe("secretLeakFinding scans only ADDED lines", () => { const diff = `### fixtures/${fakeToken}.txt (removed) +0/-1\n@@\n-const unrelated = 1;`; expect(secretLeakFinding(diff)).toBeNull(); }); + + it("flags a credential split across consecutive added lines (#2454)", () => { + const awsKeyFragmentA = "AKIA" + "IOSFODNN7"; + const awsKeyFragmentB = "EXAMPLE"; + const diff = [ + "### src/config.ts (modified) +2/-0", + "@@ -1,0 +1,2 @@", + `+const part1 = "${awsKeyFragmentA}";`, + `+const part2 = "${awsKeyFragmentB}";`, + ].join("\n"); + expect(secretLeakFinding(diff)?.code).toBe("secret_leak"); + expect(secretLeakFinding(diff)?.title).toContain("aws_access_key"); + }); + + it("blocks split credentials when the second line matches only a non-blocking heuristic (#2877)", () => { + const awsKeyFragmentA = "AKIA" + "IOSFODNN7"; + const awsKeyFragmentB = "EXAMPLE"; + const diff = [ + "### src/config.ts (modified) +2/-0", + "@@ -1,0 +1,2 @@", + `+const part1 = "${awsKeyFragmentA}";`, + `+coldkey = "${awsKeyFragmentB}";`, + ].join("\n"); + expect(secretLeakFinding(diff)?.code).toBe("secret_leak"); + expect(secretLeakFinding(diff)?.title).toContain("aws_access_key"); + }); }); diff --git a/test/unit/secrets-scan.test.ts b/test/unit/secrets-scan.test.ts index 04f30230a..6211d98f7 100644 --- a/test/unit/secrets-scan.test.ts +++ b/test/unit/secrets-scan.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { scanForSecrets } from "../../src/review/secrets-scan"; +import { scanForSecrets, scanPrDiffForSecretKinds } from "../../src/review/secrets-scan"; describe("scanForSecrets — deterministic secret-pattern scanner", () => { it("returns no findings for empty / benign text", () => { @@ -107,3 +107,220 @@ describe("scanForSecrets — deterministic secret-pattern scanner", () => { expect(scanForSecrets('token = "short12345"').kinds).not.toContain("generic_secret_assignment"); }); }); + +describe("scanPrDiffForSecretKinds — cross-line split credentials (#2454)", () => { + const awsKeyFragmentA = "AKIA" + "IOSFODNN7"; + const awsKeyFragmentB = "EXAMPLE"; + + it("catches an AWS key split across two adjacent added lines via string literals", () => { + const diff = [ + "### src/config.ts (modified) +2/-0", + "@@ -1,0 +1,2 @@", + `+const part1 = "${awsKeyFragmentA}";`, + `+const part2 = "${awsKeyFragmentB}";`, + ].join("\n"); + expect(scanPrDiffForSecretKinds(diff)).toContain("aws_access_key"); + }); + + it("does not join literals across a context line inside the hunk", () => { + const diff = [ + "### src/config.ts (modified) +2/-0", + "@@ -1,1 +1,3 @@", + `+const part1 = "${awsKeyFragmentA}";`, + ' const unrelated = "context line";', + `+const part2 = "${awsKeyFragmentB}";`, + ].join("\n"); + expect(scanPrDiffForSecretKinds(diff)).not.toContain("aws_access_key"); + }); + + it("does not join literals across a hunk boundary", () => { + const diff = [ + "### src/config.ts (modified) +2/-0", + "@@ -1,0 +1,1 @@", + `+const part1 = "${awsKeyFragmentA}";`, + "@@ -10,0 +11,1 @@", + `+const part2 = "${awsKeyFragmentB}";`, + ].join("\n"); + expect(scanPrDiffForSecretKinds(diff)).not.toContain("aws_access_key"); + }); + + it("does not join unrelated short literals into a false positive", () => { + const diff = [ + "### src/app.ts (modified) +2/-0", + "@@ -1,0 +1,2 @@", + '+const a = "hello";', + '+const b = "world";', + ].join("\n"); + expect(scanPrDiffForSecretKinds(diff)).toEqual([]); + }); + + it("still flags a single-line secret on an added line", () => { + const fakeToken = "ghp_" + "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"; + const diff = `### src/config.ts (modified) +1/-0\n@@\n+const token = "${fakeToken}";`; + expect(scanPrDiffForSecretKinds(diff)).toContain("github_token"); + }); + + it("does not flag secrets on removed or context lines", () => { + const fakeToken = "ghp_" + "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"; + const removed = `### src/config.ts (modified) +0/-1\n@@\n-const token = "${fakeToken}";`; + const context = `### src/config.ts (modified) +1/-0\n@@\n const token = "${fakeToken}";\n+const unrelated = 1;`; + expect(scanPrDiffForSecretKinds(removed)).toEqual([]); + expect(scanPrDiffForSecretKinds(context)).toEqual([]); + }); + + it("scans patch-less synthetic + lines without a hunk header", () => { + const fakeToken = "ghp_" + "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"; + const diff = `### secrets.env (modified) +1/-0\n+const token = "${fakeToken}";`; + expect(scanPrDiffForSecretKinds(diff)).toContain("github_token"); + }); + + it("flags secrets embedded in added/renamed file path headers", () => { + const fakeToken = "ghp_" + "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"; + const added = `### fixtures/${fakeToken}.txt (added) +1/-0\n+clean content`; + const renamed = `### fixtures/${fakeToken}.txt (renamed) +0/-0\n+clean content`; + expect(scanPrDiffForSecretKinds(added)).toContain("github_token"); + expect(scanPrDiffForSecretKinds(renamed)).toContain("github_token"); + }); + + it("ignores orphan + lines that appear before any file header", () => { + const fakeToken = "ghp_" + "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"; + expect(scanPrDiffForSecretKinds(`+const token = "${fakeToken}";`)).toEqual([]); + }); + + it("resets cross-line join state across blank lines between file sections", () => { + const fakeToken = "ghp_" + "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"; + const diff = [ + "### src/a.ts (modified) +1/-0", + `+const part1 = "${awsKeyFragmentA}";`, + "", + "### src/b.ts (modified) +1/-0", + `+const part2 = "${awsKeyFragmentB}";`, + ].join("\n"); + expect(scanPrDiffForSecretKinds(diff)).not.toContain("aws_access_key"); + expect( + scanPrDiffForSecretKinds( + [ + "### src/b.ts (modified) +1/-0", + `+const token = "${fakeToken}";`, + ].join("\n"), + ), + ).toContain("github_token"); + }); + + it("resets cross-line join state when a removed line breaks the added-line run", () => { + const diff = [ + "### src/config.ts (modified) +2/-0", + "@@ -1,0 +1,2 @@", + `+const part1 = "${awsKeyFragmentA}";`, + "-const removed = 1;", + `+const part2 = "${awsKeyFragmentB}";`, + ].join("\n"); + expect(scanPrDiffForSecretKinds(diff)).not.toContain("aws_access_key"); + }); + + it("recovers generic_secret_assignment when keyword and value span adjacent added lines", () => { + const fakeSecret = "sk_live_" + "aK9xQ2mZw7Ln4Rv8Pt3Bh6"; + const diff = [ + "### src/config.ts (modified) +2/-0", + "@@", + "+client_secret =", + `+"${fakeSecret}"`, + ].join("\n"); + expect(scanPrDiffForSecretKinds(diff)).toContain("generic_secret_assignment"); + }); + + it("does not join generic keyword and value across context, removed, hunk, or file boundaries", () => { + const fakeSecret = "sk_live_" + "aK9xQ2mZw7Ln4Rv8Pt3Bh6"; + + const contextSplit = [ + "### src/config.ts (modified) +2/-0", + "@@", + "+client_secret =", + " const unrelated = 1;", + `+"${fakeSecret}"`, + ].join("\n"); + expect(scanPrDiffForSecretKinds(contextSplit)).not.toContain("generic_secret_assignment"); + + const removedSplit = [ + "### src/config.ts (modified) +2/-0", + "@@", + "+client_secret =", + "-const gone = 1;", + `+"${fakeSecret}"`, + ].join("\n"); + expect(scanPrDiffForSecretKinds(removedSplit)).not.toContain("generic_secret_assignment"); + + const hunkSplit = [ + "### src/config.ts (modified) +2/-0", + "@@ -1,0 +1,1 @@", + "+client_secret =", + "@@ -10,0 +11,1 @@", + `+"${fakeSecret}"`, + ].join("\n"); + expect(scanPrDiffForSecretKinds(hunkSplit)).not.toContain("generic_secret_assignment"); + + const fileSplit = [ + "### src/a.ts (modified) +1/-0", + "@@", + "+client_secret =", + "", + "### src/b.ts (modified) +1/-0", + "@@", + `+"${fakeSecret}"`, + ].join("\n"); + expect(scanPrDiffForSecretKinds(fileSplit)).not.toContain("generic_secret_assignment"); + }); + + it("still joins literals when the second line matches only a non-blocking heuristic (#2877)", () => { + const diff = [ + "### src/config.ts (modified) +2/-0", + "@@ -1,0 +1,2 @@", + `+const part1 = "${awsKeyFragmentA}";`, + `+coldkey = "${awsKeyFragmentB}";`, + ].join("\n"); + expect(scanPrDiffForSecretKinds(diff)).toContain("aws_access_key"); + expect(scanPrDiffForSecretKinds(diff)).toContain("bittensor_key"); + }); + + it("does not double-join when the first added line already matches on its own", () => { + const fakeAwsKey = awsKeyFragmentA + awsKeyFragmentB; + const diff = [ + "### src/config.ts (modified) +2/-0", + "@@", + `+const key = "${fakeAwsKey}";`, + `+const tail = "${awsKeyFragmentB}";`, + ].join("\n"); + expect(scanPrDiffForSecretKinds(diff)).toEqual(["aws_access_key"]); + }); + + it("scans in-hunk added content that begins with ++ (rendered +++…)", () => { + const fakeToken = "ghp_" + "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"; + const diff = [ + "### src/config.ts (modified) +1/-0", + "@@ -1,0 +1,1 @@", + `+++const token = "${fakeToken}";`, + ].join("\n"); + expect(scanPrDiffForSecretKinds(diff)).toContain("github_token"); + }); + + it("skips unified-diff file headers before the first hunk", () => { + const fakeToken = "ghp_" + "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"; + const diff = [ + "### src/config.ts (modified) +1/-0", + "+++ b/src/config.ts", + "--- a/src/config.ts", + "@@ -1,0 +1,1 @@", + "+const ok = 1;", + ].join("\n"); + expect(scanPrDiffForSecretKinds(diff)).toEqual([]); + expect( + scanPrDiffForSecretKinds( + [ + "### src/config.ts (modified) +1/-0", + "+++ b/src/config.ts", + `+const token = "${fakeToken}";`, + ].join("\n"), + ), + ).toContain("github_token"); + }); +});