diff --git a/review-enrichment/src/analyzers/dependency-scan.ts b/review-enrichment/src/analyzers/dependency-scan.ts index 038f31de1c..eac9f9bedf 100644 --- a/review-enrichment/src/analyzers/dependency-scan.ts +++ b/review-enrichment/src/analyzers/dependency-scan.ts @@ -25,6 +25,13 @@ interface ScanOptions { limits?: ScanLimits; } +export class DependencyScanTruncatedError extends Error { + constructor(reason: string) { + super(`dependency_scan_truncated:${reason}`); + this.name = "DependencyScanTruncatedError"; + } +} + // Per-manifest line parsers. Each returns [name, version] for a `+`/`-` diff line, or null. Heuristic (line-based, // not a full manifest parse) — good enough to flag the deps a PR adds/bumps without resolving the whole tree. const NPM_RE = /^"([^"]+)"\s*:\s*"([\^~>=<\s]*[0-9][^"]*)"/; @@ -73,8 +80,12 @@ export function extractDependencyChanges( const ecosystem = ECOSYSTEM[manifest]; if (!ecosystem || !file.patch) continue; manifestFiles += 1; - if (manifestFiles > maxManifestFiles) break; - for (const line of file.patch.split("\n", maxPatchLinesPerFile)) { + if (manifestFiles > maxManifestFiles) + throw new DependencyScanTruncatedError("manifest_file_limit"); + const patchLines = file.patch.split("\n"); + if (patchLines.length > maxPatchLinesPerFile) + throw new DependencyScanTruncatedError("patch_line_limit"); + for (const line of patchLines) { const sign = line[0]; if ( (sign !== "+" && sign !== "-") || @@ -180,10 +191,11 @@ export async function scanDependencies( fetchImpl: typeof fetch = fetch, options: ScanOptions = {}, ): Promise { - const changes = extractDependencyChanges(req.files ?? [], options.limits).slice( - 0, - options.limits?.maxDependencyQueries ?? MAX_DEPENDENCY_QUERIES, - ); + const changes = extractDependencyChanges(req.files ?? [], options.limits); + const maxDependencyQueries = + options.limits?.maxDependencyQueries ?? MAX_DEPENDENCY_QUERIES; + if (changes.length > maxDependencyQueries) + throw new DependencyScanTruncatedError("dependency_query_limit"); const findings: DependencyFinding[] = []; for (const change of changes) { if (options.signal?.aborted) break; diff --git a/review-enrichment/test/enrichment.test.ts b/review-enrichment/test/enrichment.test.ts index cd80458637..a47a7a7cec 100644 --- a/review-enrichment/test/enrichment.test.ts +++ b/review-enrichment/test/enrichment.test.ts @@ -1,6 +1,7 @@ import { test } from "node:test"; import assert from "node:assert/strict"; import { + DependencyScanTruncatedError, extractDependencyChanges, queryOsv, scanDependencies, @@ -133,6 +134,80 @@ test("scanDependencies: only deps with vulns are returned", async () => { assert.equal(findings[0].cves[0].severity, "critical"); }); +test("scanDependencies: truncation fails closed instead of hiding later vulnerable deps", async () => { + const files = [ + { + path: "package.json", + patch: Array.from( + { length: 26 }, + (_, i) => `+ "pkg-${i}": "1.0.0",`, + ).join("\n"), + }, + ]; + let calls = 0; + await assert.rejects( + scanDependencies( + { repoFullName: "o/r", prNumber: 1, files }, + async () => { + calls += 1; + return { ok: true, json: async () => ({ vulns: [] }) }; + }, + ), + DependencyScanTruncatedError, + ); + assert.equal(calls, 0); +}); + +test("scanDependencies: manifest and patch-line truncation fail closed", () => { + const manifestFiles = Array.from({ length: 21 }, (_, i) => ({ + path: `pkg-${i}/package.json`, + patch: `+ "pkg-${i}": "1.0.0",`, + })); + assert.throws( + () => extractDependencyChanges(manifestFiles), + DependencyScanTruncatedError, + ); + + assert.throws( + () => + extractDependencyChanges([ + { + path: "package.json", + patch: Array.from({ length: 501 }, (_, i) => + i === 500 ? '+ "lodash": "4.17.20",' : " context", + ).join("\n"), + }, + ]), + DependencyScanTruncatedError, + ); +}); + +test("buildBrief: truncated dependency scan marks the brief degraded", async () => { + const realFetch = globalThis.fetch; + globalThis.fetch = okFetch([]); + try { + const brief = await buildBrief({ + repoFullName: "o/r", + prNumber: 9, + analyzers: ["dependency"], + files: [ + { + path: "package.json", + patch: Array.from( + { length: 26 }, + (_, i) => `+ "pkg-${i}": "1.0.0",`, + ).join("\n"), + }, + ], + }); + assert.equal(brief.partial, true); + assert.equal(brief.analyzerStatus.dependency, "degraded"); + assert.equal(brief.findings.dependency, undefined); + } finally { + globalThis.fetch = realFetch; + } +}); + test("renderBrief: sorts by severity, empty when no findings", () => { const empty = renderBrief({}); assert.equal(empty.promptSection, ""); @@ -597,27 +672,37 @@ test("buildBrief: action-pin analyzer runs (pure, no network)", async () => { } }); -test("extractDependencyChanges: caps manifest files and patch lines", () => { - const changes = extractDependencyChanges( - [ - { - path: "package.json", - patch: ['+ "first": "1.0.0",', '+ "second": "1.0.0",'].join( - "\n", - ), - }, - { path: "nested/package.json", patch: '+ "third": "1.0.0",' }, - ], - { maxManifestFiles: 1, maxPatchLinesPerFile: 1 }, +test("extractDependencyChanges: rejects truncated manifest files and patch lines", () => { + assert.throws( + () => + extractDependencyChanges( + [ + { path: "package.json", patch: '+ "first": "1.0.0",' }, + { path: "nested/package.json", patch: '+ "second": "1.0.0",' }, + ], + { maxManifestFiles: 1 }, + ), + DependencyScanTruncatedError, ); - assert.deepEqual( - changes.map((change) => change.package), - ["first"], + assert.throws( + () => + extractDependencyChanges( + [ + { + path: "package.json", + patch: ['+ "first": "1.0.0",', '+ "second": "1.0.0",'].join( + "\n", + ), + }, + ], + { maxPatchLinesPerFile: 1 }, + ), + DependencyScanTruncatedError, ); }); -test("scanDependencies: caps OSV queries and forwards abort signals", async () => { +test("scanDependencies: forwards abort signals without truncation", async () => { const seenSignals = []; const files = Array.from({ length: 3 }, (_, index) => ({ path: "package.json", @@ -631,11 +716,11 @@ test("scanDependencies: caps OSV queries and forwards abort signals", async () = seenSignals.push(init.signal); return { ok: true, json: async () => ({ vulns: [] }) }; }, - { signal: controller.signal, limits: { maxDependencyQueries: 2 } }, + { signal: controller.signal, limits: { maxDependencyQueries: 3 } }, ); assert.equal(findings.length, 0); - assert.equal(seenSignals.length, 2); + assert.equal(seenSignals.length, 3); assert.ok(seenSignals.every((signal) => signal instanceof AbortSignal)); });