diff --git a/src/queue/patchless-secret-scan.ts b/src/queue/patchless-secret-scan.ts index c8de75401a..0916c04dd8 100644 --- a/src/queue/patchless-secret-scan.ts +++ b/src/queue/patchless-secret-scan.ts @@ -53,9 +53,24 @@ export function shouldAttemptPatchLessSecretScan( baseSha?: string | null | undefined, ): boolean { if (status === "removed") return false; - if (status === "modified") return Boolean(baseSha?.trim()); + if (status === "added") return true; if (status === "renamed") return Boolean(baseSha?.trim() && file.previousFilename?.trim()); - return status === "added"; + // GitHub's Pull Request Files API `status` can also be `copied` | `changed` | `unchanged` + // (diff_entry OpenAPI schema). `copied`/`changed` can introduce new committed content relative + // to base; `unchanged` is still usable in merge-commit contexts where content can differ from + // what a local diff assumes. Treat all three like `modified`: attempt the base/head multiset + // scan when `baseSha` is known. Falling through to "never scan" for these statuses used to + // silently bypass both the content-fetch fallback and the fail-closed `secretScanIncomplete` + // advisory (#5947). + if ( + status === "modified" || + status === "copied" || + status === "changed" || + status === "unchanged" + ) { + return Boolean(baseSha?.trim()); + } + return false; } export function hasPatchLessSecretScanCandidates( @@ -159,9 +174,11 @@ async function mapPatchLessSecretScanFilesWithConcurrency( /** When GitHub omits inline `patch` (binary/large files), fetch post-change content and synthesize `+` lines so * the unconditional `secret_leak` hard blocker can still inspect committed credentials. Added files scan only - * genuinely new lines; modified/renamed files multiset-diff against base when `baseSha` is known. Unfetchable - * or baseline-unknown content leaves the file header-only so pre-existing secrets are not mis-flagged; content - * over the per-file cap is marked incomplete so the gate fails closed instead of scanning a truncated prefix. + * genuinely new lines; modified/copied/changed/unchanged/renamed files multiset-diff against base when `baseSha` + * is known (`copied`/`changed`/`unchanged` are treated like `modified` — #5947 — so they are never silently + * skipped). Unfetchable or baseline-unknown content leaves the file header-only so pre-existing secrets are not + * mis-flagged; content over the per-file cap is marked incomplete so the gate fails closed instead of scanning a + * truncated prefix. */ export async function enrichSecretScanFilesWithPatchFallback( files: PullRequestFileRecord[], diff --git a/test/unit/patchless-secret-scan.test.ts b/test/unit/patchless-secret-scan.test.ts index 5aa5e7626c..81031d05df 100644 --- a/test/unit/patchless-secret-scan.test.ts +++ b/test/unit/patchless-secret-scan.test.ts @@ -105,6 +105,69 @@ describe("enrichSecretScanFilesWithPatchFallback", () => { expect(secretLeakFinding(buildSecretScanDiff(enriched))?.code).toBe("secret_leak"); }); + it.each(["copied", "changed", "unchanged"] as const)( + "scans patch-less %s-status files via the modified-style base/head fallback (#5947)", + async (status) => { + const fetcher: FileFetcher = { + async getFileContent(path, ref) { + if (path !== "src/config.ts") return null; + if (ref === "base-sha") return "const existing = 1;\n"; + if (ref === "head-sha") return `const existing = 1;\nconst token = "${fakeToken}";\n`; + return null; + }, + }; + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "src/config.ts", + status, + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }, + ]; + const enriched = await enrichSecretScanFilesWithPatchFallback(files, { + headSha: "head-sha", + baseSha: "base-sha", + fetcher, + }); + expect(secretLeakFinding(buildSecretScanDiff(enriched))?.code).toBe("secret_leak"); + expect(enriched[0]?.payload?.patch).toContain(`+const token = "${fakeToken}";`); + }, + ); + + it.each(["copied", "changed", "unchanged"] as const)( + "leaves a patch-less %s-status file unscannable when baseSha is unknown (#5947)", + async (status) => { + const fetcher: FileFetcher = { + async getFileContent() { + return `const token = "${fakeToken}";\n`; + }, + }; + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "src/config.ts", + status, + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }, + ]; + const enriched = await enrichSecretScanFilesWithPatchFallback(files, { + headSha: "head-sha", + fetcher, + }); + expect(secretLeakFinding(buildSecretScanDiff(enriched))).toBeNull(); + expect(enriched[0]?.payload?.patch).toBeUndefined(); + expect(enriched[0]?.payload?.secretScanIncomplete).toBeUndefined(); + }, + ); + it("leaves a patch-less modified file unscannable when baseSha is unknown", async () => { const fetcher: FileFetcher = { async getFileContent() { @@ -969,7 +1032,16 @@ describe("patchlessSecretScanInternals", () => { expect(shouldAttemptPatchLessSecretScan({}, "modified", null)).toBe(false); expect(shouldAttemptPatchLessSecretScan({}, "modified", " ")).toBe(false); expect(shouldAttemptPatchLessSecretScan({}, "removed", "base-sha")).toBe(false); - expect(shouldAttemptPatchLessSecretScan({}, "copied", "base-sha")).toBe(false); + // copied/changed/unchanged are attemptable like modified when baseSha is known (#5947) — + // previously they fell through to `status === "added"` and were silently skipped. + expect(shouldAttemptPatchLessSecretScan({}, "copied", "base-sha")).toBe(true); + expect(shouldAttemptPatchLessSecretScan({}, "copied", null)).toBe(false); + expect(shouldAttemptPatchLessSecretScan({}, "copied", " ")).toBe(false); + expect(shouldAttemptPatchLessSecretScan({}, "changed", "base-sha")).toBe(true); + expect(shouldAttemptPatchLessSecretScan({}, "changed", null)).toBe(false); + expect(shouldAttemptPatchLessSecretScan({}, "unchanged", "base-sha")).toBe(true); + expect(shouldAttemptPatchLessSecretScan({}, "unchanged", null)).toBe(false); + expect(shouldAttemptPatchLessSecretScan({}, "unknown-status", "base-sha")).toBe(false); expect(shouldAttemptPatchLessSecretScan({}, "added", "base-sha")).toBe(true); expect( shouldAttemptPatchLessSecretScan({ previousFilename: "old.env" }, "renamed", "base-sha"), @@ -1008,11 +1080,18 @@ describe("patchlessSecretScanInternals", () => { payload: {}, }; const modified = { ...added, path: "modified.env", status: "modified" }; + const copied = { ...added, path: "copied.env", status: "copied" }; + const changed = { ...added, path: "changed.env", status: "changed" }; + const unchanged = { ...added, path: "unchanged.env", status: "unchanged" }; const inline = { ...added, path: "inline.env", payload: { patch: "+ok" } }; const removed = { ...added, path: "removed.env", status: "removed" }; expect(patchLessSecretScanFetchCost(added, null)).toBe(1); expect(patchLessSecretScanFetchCost(modified, "base-sha")).toBe(2); expect(patchLessSecretScanFetchCost(modified, null)).toBe(0); + expect(patchLessSecretScanFetchCost(copied, "base-sha")).toBe(2); + expect(patchLessSecretScanFetchCost(copied, null)).toBe(0); + expect(patchLessSecretScanFetchCost(changed, "base-sha")).toBe(2); + expect(patchLessSecretScanFetchCost(unchanged, "base-sha")).toBe(2); expect(patchLessSecretScanFetchCost(inline, null)).toBe(0); expect(patchLessSecretScanFetchCost(removed, "base-sha")).toBe(0); expect(patchLessSecretScanFetchCostExceedsBudget([added], null)).toBe(false); @@ -1133,6 +1212,45 @@ describe("maybeAddSecretLeakFinding patch-less fallback wiring", () => { expect(adv.findings.map((f) => f.code)).toContain("secret_leak"); }); + it("scans a patch-less copied-status file with a committed secret (#5947)", async () => { + const env = createTestEnv(); + const adv = advisory(); + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "copied.env", + status: "copied", + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }, + ]; + const groundingWire = await import("../../src/review/grounding-wire"); + const fetcher: FileFetcher = { + async getFileContent(path, ref) { + if (path !== "copied.env") return null; + if (ref === "base-sha") return "EXISTING_VALUE=1\n"; + if (ref === "head-sha") return `EXISTING_VALUE=1\nTOKEN=${fakeToken}\n`; + return null; + }, + }; + const spy = vi.spyOn(groundingWire, "makeGithubFileFetcher").mockResolvedValue(fetcher); + await maybeAddSecretLeakFinding(env, { + advisory: adv, + repoFullName: "acme/widgets", + pullNumber: 7, + files, + installationId: 1, + headSha: "head-sha", + baseSha: "base-sha", + }); + spy.mockRestore(); + expect(adv.findings.map((f) => f.code)).toContain("secret_leak"); + expect(adv.findings.some((f) => f.title.includes("could not be fully scanned"))).toBe(false); + }); + it("falls back to inline patches when patch-less enrichment rejects", async () => { const env = createTestEnv(); const adv = advisory();