fix(review): detect cross-line split credentials in gate secret scan (#2454)

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",

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<string>();
+  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];
+}

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");
+  });
 });