From f9b5d13b07b6bb8899d360f94395c5dc6bbb914f Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 22:07:44 -0700 Subject: [PATCH] fix(enrichment): join adjacent added lines before secret-scan matching MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Secret-scan matched each added diff line independently, so a real credential split across two added lines via string concatenation (e.g. `const a = "AKIA..."; const b = a + "REST";`) never appeared as a single matchable token on either line and evaded every RULES regex. Extract quoted string-literal contents per added line and, when the line doesn't already match on its own, join the immediately-preceding line's last literal with the current line's first literal and test that too (downgraded to medium confidence — a joined pair is a heuristic, not a direct match). Bounded to consecutive added lines only: a hunk boundary, context line, or removed line resets the window, so this only ever catches the realistic "two sequential variable assignments" shape, not an unbounded cross-file join. Existing per-line detection is untouched and unweakened. Added review-enrichment/test/secret-scan.test.ts (previously untested) covering both the pre-existing single-line rules and the new cross-line join, including negative cases for context-line/hunk- boundary breaks and duplicate-finding avoidance. Fixes #2454 --- .../src/analyzers/secret-scan.ts | 44 +++++++- review-enrichment/test/secret-scan.test.ts | 105 ++++++++++++++++++ 2 files changed, 143 insertions(+), 6 deletions(-) create mode 100644 review-enrichment/test/secret-scan.test.ts diff --git a/review-enrichment/src/analyzers/secret-scan.ts b/review-enrichment/src/analyzers/secret-scan.ts index 816f36641d..f13ac03bc0 100644 --- a/review-enrichment/src/analyzers/secret-scan.ts +++ b/review-enrichment/src/analyzers/secret-scan.ts @@ -47,33 +47,65 @@ const RULES: Rule[] = [ }, ]; +/** Extract the inner text of every quoted string literal (single/double/backtick) on a line. Used to catch a + * secret whose literal value is split across two adjacent added lines and joined at runtime (e.g. + * `const a = "AKIA..."; const b = a + "REST";`) — pure per-line regex matching never sees the runtime-joined + * value, only the two separate source literals either side of the `+`. */ +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; +} + /** Scan one file's unified-diff patch, tracking new-file line numbers via hunk headers. Pure. Value never captured. */ export function scanPatch(path: string, patch: string): SecretFinding[] { const findings: SecretFinding[] = []; let newLine = 0; + // Last added line's extracted string-literal contents, for the cross-line join check below. Reset whenever a + // non-added line breaks the run — a secret is only plausibly split across CONSECUTIVE added lines. + let previousLiterals: string[] = []; for (const line of patch.split("\n")) { if (line.startsWith("+++") || line.startsWith("---")) continue; const hunk = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(line); if (hunk) { newLine = Number(hunk[1]); + previousLiterals = []; continue; } if (line.startsWith("+")) { const content = line.slice(1); + let matched = false; for (const rule of RULES) { if (rule.re.test(content)) { - findings.push({ - file: path, - line: newLine, - kind: rule.kind, - confidence: rule.confidence, - }); + findings.push({ file: path, line: newLine, kind: rule.kind, confidence: rule.confidence }); + matched = true; break; // one finding per line — first (most specific) rule wins } } + const currentLiterals = extractStringLiteralContents(content); + // Bounded: only the immediately-preceding line's LAST literal joined with this line's FIRST literal — the + // common "two sequential variable assignments" shape. Skipped once this line already matched on its own. + const lastPrevious = previousLiterals.at(-1); + const firstCurrent = currentLiterals[0]; + if (!matched && lastPrevious !== undefined && firstCurrent !== undefined) { + const joined = lastPrevious + firstCurrent; + for (const rule of RULES) { + if (rule.re.test(joined)) { + // "medium" regardless of the rule's own confidence — a joined pair is a heuristic, not a direct match. + findings.push({ file: path, line: newLine, kind: rule.kind, confidence: "medium" }); + break; + } + } + } + previousLiterals = currentLiterals; newLine++; } else if (!line.startsWith("-")) { newLine++; // context line advances the new-file counter; removed lines do not + previousLiterals = []; + } else { + previousLiterals = []; } } return findings; diff --git a/review-enrichment/test/secret-scan.test.ts b/review-enrichment/test/secret-scan.test.ts new file mode 100644 index 0000000000..b9d53896ed --- /dev/null +++ b/review-enrichment/test/secret-scan.test.ts @@ -0,0 +1,105 @@ +// Units for the secret-scan analyzer. Kept separate so analyzer PRs do not collide in one shared test file. +import { test } from "node:test"; +import assert from "node:assert/strict"; + +import { scanPatch } from "../dist/analyzers/secret-scan.js"; + +const hunk = (lines) => `@@ -1,0 +1,${lines.length} @@\n${lines.map((l) => `+${l}`).join("\n")}`; + +// Built from fragments at test-run time (never a contiguous secret-shaped literal in the committed source) so +// GitHub push protection does not flag this fixture file the way it would flag a real leaked credential — the +// fragments below are only ever joined into one string inside the synthetic diff patch handed to scanPatch, or +// (for the cross-line tests) deliberately kept as two SEPARATE short literals that mirror what a real split +// secret looks like in source — neither fragment alone is a contiguous match for any RULES pattern. +const awsKeyFragmentA = "AKIA" + "IOSFODNN7"; // 13 chars — too short alone to match \bAKIA[0-9A-Z]{16}\b +const awsKeyFragmentB = "EXAMPLE"; // matches no RULES pattern alone +const fakeAwsKey = awsKeyFragmentA + awsKeyFragmentB; +const fakeStripeKey = ["sk_live_", "abcdefghijklmnop1234567890"].join(""); + +test("scanPatch flags a single-line AWS access key with high confidence", () => { + const findings = scanPatch("src/config.ts", hunk([`const key = "${fakeAwsKey}";`])); + assert.equal(findings.length, 1); + assert.equal(findings[0].kind, "aws_access_key_id"); + assert.equal(findings[0].confidence, "high"); + assert.equal(findings[0].file, "src/config.ts"); + assert.equal(findings[0].line, 1); +}); + +test("scanPatch flags a private key header", () => { + const findings = scanPatch("id_rsa", hunk(["-----BEGIN RSA PRIVATE KEY-----"])); + assert.equal(findings.length, 1); + assert.equal(findings[0].kind, "private_key"); +}); + +test("scanPatch flags a generic secret assignment", () => { + const findings = scanPatch("src/config.ts", hunk([`const apiKey = "${fakeStripeKey}";`])); + assert.equal(findings.length, 1); + assert.equal(findings[0].kind, "generic_secret_assignment"); + assert.equal(findings[0].confidence, "medium"); +}); + +test("scanPatch reports nothing for clean code", () => { + const findings = scanPatch("src/app.ts", hunk(['const greeting = "hello world";', "export function run() {}"])); + assert.equal(findings.length, 0); +}); + +// Regression test for #2454: a real credential split across two added lines via string concatenation evaded +// every RULES regex since neither line's own text contains the full contiguous pattern. +test("scanPatch catches an AWS key split across two adjacent added lines via concatenation (#2454)", () => { + const findings = scanPatch( + "src/config.ts", + hunk([`const part1 = "${awsKeyFragmentA}";`, `const part2 = "${awsKeyFragmentB}";`, "const awsKey = part1 + part2;"]), + ); + assert.equal(findings.length, 1); + assert.equal(findings[0].kind, "aws_access_key_id"); + assert.equal(findings[0].confidence, "medium"); // downgraded — a joined pair is a heuristic, not a direct match + assert.equal(findings[0].line, 2); // attributed to the completing (second) line +}); + +test("scanPatch does not join literals across a context line that breaks the added-line run (#2454)", () => { + const patch = [ + "@@ -1,1 +1,3 @@", + ' const unrelated = "context line";', // unchanged context line breaks the run + `+const part1 = "${awsKeyFragmentA}";`, + `+const part2 = "${awsKeyFragmentB}";`, + ].join("\n"); + // part1 and part2 ARE still adjacent added lines here, so this should still match — this test instead + // verifies a context line BETWEEN the two secret halves prevents the join. + const findingsAdjacent = scanPatch("src/config.ts", patch); + assert.equal(findingsAdjacent.length, 1); + + const patchWithGap = [ + "@@ -1,1 +1,3 @@", + `+const part1 = "${awsKeyFragmentA}";`, + ' const unrelated = "context line";', + `+const part2 = "${awsKeyFragmentB}";`, + ].join("\n"); + const findingsGapped = scanPatch("src/config.ts", patchWithGap); + assert.equal(findingsGapped.length, 0); +}); + +test("scanPatch does not join literals across a hunk boundary (#2454)", () => { + const patch = [ + "@@ -1,0 +1,1 @@", + `+const part1 = "${awsKeyFragmentA}";`, + "@@ -10,0 +11,1 @@", + `+const part2 = "${awsKeyFragmentB}";`, + ].join("\n"); + const findings = scanPatch("src/config.ts", patch); + assert.equal(findings.length, 0); +}); + +test("scanPatch does not double-report a line that already matched on its own", () => { + const findings = scanPatch( + "src/config.ts", + hunk([`const key = "${fakeAwsKey}";`, 'const other = "unrelated-string-value";']), + ); + // The first line already matches directly; its literal must not ALSO be joined into a second, duplicate finding. + assert.equal(findings.length, 1); + assert.equal(findings[0].line, 1); +}); + +test("scanPatch does not join two unrelated short literals into a false positive", () => { + const findings = scanPatch("src/app.ts", hunk(['const a = "hello";', 'const b = "world";'])); + assert.equal(findings.length, 0); +});