From 2e8a0c8d0c7ef9f8486d748e5e45a4696997bd1b Mon Sep 17 00:00:00 2001 From: ghost <49853598+JSONbored@users.noreply.github.com> Date: Fri, 26 Jun 2026 13:26:53 -0700 Subject: [PATCH] fix(enrichment): bound dependency scans --- .../src/analyzers/dependency-scan.ts | 35 +++++++- review-enrichment/src/brief.ts | 22 +++-- review-enrichment/test/enrichment.test.ts | 80 +++++++++++++++++++ 3 files changed, 129 insertions(+), 8 deletions(-) diff --git a/review-enrichment/src/analyzers/dependency-scan.ts b/review-enrichment/src/analyzers/dependency-scan.ts index 3f31207919..038f31de1c 100644 --- a/review-enrichment/src/analyzers/dependency-scan.ts +++ b/review-enrichment/src/analyzers/dependency-scan.ts @@ -10,6 +10,21 @@ interface DepChange { to: string; } +const MAX_MANIFEST_FILES = 20; +const MAX_PATCH_LINES_PER_FILE = 500; +const MAX_DEPENDENCY_QUERIES = 25; + +interface ScanLimits { + maxManifestFiles?: number; + maxPatchLinesPerFile?: number; + maxDependencyQueries?: number; +} + +interface ScanOptions { + signal?: AbortSignal; + limits?: ScanLimits; +} + // 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][^"]*)"/; @@ -43,16 +58,23 @@ const ECOSYSTEM: Record = { /** Extract added/changed (not removed) dependency versions from the changed manifests in the diff. Pure. */ export function extractDependencyChanges( files: NonNullable, + limits: ScanLimits = {}, ): DepChange[] { const byKey = new Map< string, { ecosystem: string; package: string; added?: string; removed?: string } >(); + const maxManifestFiles = limits.maxManifestFiles ?? MAX_MANIFEST_FILES; + const maxPatchLinesPerFile = + limits.maxPatchLinesPerFile ?? MAX_PATCH_LINES_PER_FILE; + let manifestFiles = 0; for (const file of files) { const manifest = file.path.split("/").pop() ?? file.path; const ecosystem = ECOSYSTEM[manifest]; if (!ecosystem || !file.patch) continue; - for (const line of file.patch.split("\n")) { + manifestFiles += 1; + if (manifestFiles > maxManifestFiles) break; + for (const line of file.patch.split("\n", maxPatchLinesPerFile)) { const sign = line[0]; if ( (sign !== "+" && sign !== "-") || @@ -131,11 +153,14 @@ export async function queryOsv( name: string, version: string, fetchImpl: typeof fetch = fetch, + signal?: AbortSignal, ): Promise { + if (signal?.aborted) return []; const response = await fetchImpl("https://api.osv.dev/v1/query", { method: "POST", headers: { "content-type": "application/json" }, body: JSON.stringify({ package: { name, ecosystem }, version }), + signal, }); if (!response.ok) return []; const data = (await response.json()) as { vulns?: OsvVuln[] }; @@ -153,15 +178,21 @@ export async function queryOsv( export async function scanDependencies( req: EnrichRequest, fetchImpl: typeof fetch = fetch, + options: ScanOptions = {}, ): Promise { - const changes = extractDependencyChanges(req.files ?? []); + const changes = extractDependencyChanges(req.files ?? [], options.limits).slice( + 0, + options.limits?.maxDependencyQueries ?? MAX_DEPENDENCY_QUERIES, + ); const findings: DependencyFinding[] = []; for (const change of changes) { + if (options.signal?.aborted) break; const cves = await queryOsv( change.ecosystem, change.package, change.to, fetchImpl, + options.signal, ); if (cves.length) { findings.push({ diff --git a/review-enrichment/src/brief.ts b/review-enrichment/src/brief.ts index 54eda8355e..e09e5c7a8f 100644 --- a/review-enrichment/src/brief.ts +++ b/review-enrichment/src/brief.ts @@ -14,21 +14,28 @@ import { scanInstallScripts } from "./analyzers/install-scripts.js"; import { scanActionPins } from "./analyzers/actions-pin.js"; import { renderBrief } from "./render.js"; -type AnalyzerFn = (req: EnrichRequest) => Promise; +type AnalyzerFn = (req: EnrichRequest, signal: AbortSignal) => Promise; // The analyzer registry. More land behind this same shape: license (#1475), secret (#1476), static (#1477), history (#1478). const ANALYZERS: Record = { - dependency: (req) => scanDependencies(req), + dependency: (req, signal) => scanDependencies(req, fetch, { signal }), secret: (req) => scanSecrets(req), license: (req) => scanLicenses(req), installScript: (req) => scanInstallScripts(req), actionPin: (req) => scanActionPins(req), }; -function withTimeout(promise: Promise, ms: number): Promise { +function runWithTimeout( + run: (signal: AbortSignal) => Promise, + ms: number, +): Promise { + const controller = new AbortController(); return new Promise((resolve, reject) => { - const timer = setTimeout(() => reject(new Error("analyzer_timeout")), ms); - promise.then( + const timer = setTimeout(() => { + controller.abort(); + reject(new Error("analyzer_timeout")); + }, ms); + run(controller.signal).then( (value) => { clearTimeout(timer); resolve(value); @@ -56,7 +63,10 @@ export async function buildBrief(req: EnrichRequest): Promise { await Promise.all( requested.map(async (name) => { try { - const result = await withTimeout(ANALYZERS[name](req), budgetMs); + const result = await runWithTimeout( + (signal) => ANALYZERS[name](req, signal), + budgetMs, + ); findings[name] = result as never; analyzerStatus[name] = "ok"; } catch { diff --git a/review-enrichment/test/enrichment.test.ts b/review-enrichment/test/enrichment.test.ts index e2606badbd..d9b3f75e2d 100644 --- a/review-enrichment/test/enrichment.test.ts +++ b/review-enrichment/test/enrichment.test.ts @@ -469,3 +469,83 @@ test("buildBrief: action-pin analyzer runs (pure, no network)", async () => { globalThis.fetch = realFetch; } }); + +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 }, + ); + + assert.deepEqual( + changes.map((change) => change.package), + ["first"], + ); +}); + +test("scanDependencies: caps OSV queries and forwards abort signals", async () => { + const seenSignals = []; + const files = Array.from({ length: 3 }, (_, index) => ({ + path: "package.json", + patch: `+ "pkg-${index}": "1.0.0",`, + })); + + const controller = new AbortController(); + const findings = await scanDependencies( + { repoFullName: "o/r", prNumber: 1, files }, + async (_url, init) => { + seenSignals.push(init.signal); + return { ok: true, json: async () => ({ vulns: [] }) }; + }, + { signal: controller.signal, limits: { maxDependencyQueries: 2 } }, + ); + + assert.equal(findings.length, 0); + assert.equal(seenSignals.length, 2); + assert.ok(seenSignals.every((signal) => signal instanceof AbortSignal)); +}); + +test("buildBrief: timeout aborts dependency scan so OSV work stops", async () => { + const realFetch = globalThis.fetch; + const signals = []; + let fetchCount = 0; + globalThis.fetch = async (_url, init) => { + fetchCount += 1; + signals.push(init.signal); + return await new Promise((_resolve, reject) => { + init.signal.addEventListener("abort", () => reject(new Error("aborted")), { + once: true, + }); + }); + }; + + try { + const brief = await buildBrief({ + repoFullName: "o/r", + prNumber: 10, + analyzers: ["dependency"], + budget: { timeoutMs: 1 }, + files: Array.from({ length: 5 }, (_, index) => ({ + path: "package.json", + patch: `+ "pkg-${index}": "1.0.0",`, + })), + }); + + assert.equal(brief.partial, true); + assert.equal(brief.analyzerStatus.dependency, "degraded"); + assert.equal(fetchCount, 1); + assert.equal(signals.length, 1); + assert.equal(signals[0].aborted, true); + await new Promise((resolve) => setTimeout(resolve, 10)); + assert.equal(fetchCount, 1); + } finally { + globalThis.fetch = realFetch; + } +});