From cc88b0dcac71751d3d6907b9696b7dfd46bb7c26 Mon Sep 17 00:00:00 2001 From: Nehanth Date: Wed, 21 Oct 2026 13:41:07 -0400 Subject: [PATCH] fix(osv): address review findings in the dependency-CVE adapter Setup state and INFO.md handling: - Remember the last explicit --osv / --no-osv choice in setup state, and carry --osv in the printed resume command. Previously a plain resume or a later bare init/setup flipped the info checkpoint digest, re-ran the analysis without dependency context, and dropped the reachability verdicts. --no-osv is new and is the way to turn the step back off. - Keep a hand-edited INFO.md on every re-run. Only the agent's "### Dependency reachability" block is refreshed (spliced into the Threat model section), kept as-is when osv produced nothing, and removed when osv is off. - Stop the osv section strip at the next "## " heading instead of cutting to end of file. - Record the dependency matcher pass only after it completes, keyed to a digest of the vulnerable-dependency set, so a failed pass is retried and a newly found dependency gets its own pass. - Ignore a leftover osv-findings.json when osv is disabled. Scanning: - Resolve ranges with --resolution lowest-direct instead of lowest, so only declared dependencies are floored. Plain lowest also pinned transitive dependencies to versions no real install picks; on fastapi this cut the report from 3 packages / 22 advisories to starlette / 14. - Capture osv-scanner stderr so "No package sources found" (exit 128, empty stdout) is treated as a clean result and clears stale findings. - Match vendored trees against the path relative to the project root, so a project checked out under a vendor/ directory is not filtered out. - Warn when uv or syft fails to resolve a manifest instead of reporting it as clean. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/dependency-cves.md | 13 +- .../deepsec/src/__tests__/osv-enrich.test.ts | 83 +++++++++++ .../src/__tests__/setup-coordinator.test.ts | 95 +++++++++--- packages/deepsec/src/cli.ts | 6 +- packages/deepsec/src/setup/coordinator.ts | 107 +++++++++----- packages/deepsec/src/setup/osv-enrich.ts | 135 ++++++++++++++++-- packages/deepsec/src/setup/state.ts | 11 +- 7 files changed, 372 insertions(+), 78 deletions(-) diff --git a/docs/dependency-cves.md b/docs/dependency-cves.md index 08291e9..95761d8 100644 --- a/docs/dependency-cves.md +++ b/docs/dependency-cves.md @@ -7,7 +7,7 @@ deepsec can surface known-vulnerable dependencies (CVEs) alongside its usual cod ## Enable it -Turn it on per run with the flag: +Turn it on with the flag: ```bash deepsec setup --osv @@ -23,7 +23,7 @@ export default defineConfig({ }); ``` -Either form enables the step; the default remains off because it runs external tools that reach the network during setup. Enabling it invalidates only that project's threat-model checkpoint, so the next `setup` re-runs the `info` phase once to incorporate the advisories. +Either form enables the step; the default remains off because it runs external tools that reach the network during setup. Once `--osv` has been passed, the workspace remembers it, so a later `deepsec init` or `deepsec setup` (including the resume command setup prints when it pauses) keeps the step on. To turn it off again, pass `--no-osv`; that choice is remembered too, and the next run drops the dependency verdicts from `INFO.md`. Enabling it invalidates only that project's threat-model checkpoint, so the next `setup` re-runs the `info` phase once to incorporate the advisories. ## Required tools @@ -39,7 +39,7 @@ The step calls external binaries and degrades cleanly when they are absent (it l **Lockfiles.** Any lockfile `osv-scanner` already understands (`package-lock.json`, `go.mod`, a pinned `requirements.txt`, and so on) is scanned directly during the recursive source walk. -**Python ranges.** `osv-scanner` can't read a `pyproject.toml` that declares version ranges with no lockfile. When `uv` is available, deepsec resolves such a manifest to both its **lowest** and **highest** allowed versions with `uv pip compile` and scans those. The lowest version is the worst case a `>=` constraint permits; the highest is what a fresh install gets today. Manifests that already have a pinned lockfile (`uv.lock`, `poetry.lock`, `Pipfile.lock`, `pdm.lock`) are left to the direct scan. +**Python ranges.** `osv-scanner` can't read a `pyproject.toml` that declares version ranges with no lockfile. When `uv` is available, deepsec resolves such a manifest twice with `uv pip compile` and scans both: once with each declared dependency at the **lowest** version its range allows (`--resolution lowest-direct`), and once at the **highest** (what a fresh install gets today). Only the declared dependencies are pinned low; transitive dependencies resolve normally, so the report isn't flooded with advisories for ancient transitive versions no real install would pick. If `uv` can't resolve a manifest, setup logs a warning rather than reporting it as clean. Manifests that already have a pinned lockfile (`uv.lock`, `poetry.lock`, `Pipfile.lock`, `pdm.lock`) are left to the direct scan. **Java/Maven.** `osv-scanner` can't read a bare `pom.xml` either. When `syft` is available, deepsec resolves the Maven project into a CycloneDX SBOM and scans that. Gradle projects without a lockfile are not resolved. @@ -47,8 +47,9 @@ The step calls external binaries and degrades cleanly when they are absent (it l - `data//setup/osv-findings.json` records the vulnerable packages and their advisory IDs. This file and the resolver's evidence files live under the gitignored `setup/` directory. - The threat model (`INFO.md`) gains a **Known-vulnerable dependencies** section. Rows resolved from a version range are labeled as such, since that version may not be the one installed. -- That section requires a **Dependency reachability** verdict with one graded line per dependency (`HIGH`/`MEDIUM`/`LOW`, the vulnerable call site as `file:line` or "not called", and the consequence if reached). Fixing this structure keeps a reachable advisory from being summarized away as prose, so the same dependency is graded the same way across runs. -- Matcher generation receives the threat model, so an advisory graded reachable gets a matcher aimed at its call site and becomes a scan candidate. The investigation phase then confirms reachability and writes the finding. +- The threat model requires a **Dependency reachability** verdict with one graded line per dependency (`HIGH`/`MEDIUM`/`LOW`, the vulnerable call site as `file:line` or "not called", and the consequence if reached). Fixing this structure keeps a reachable advisory from being summarized away as prose, so the same dependency is graded the same way across runs. +- Matcher generation receives the threat model, so an advisory graded reachable gets a matcher aimed at its call site and becomes a scan candidate. The investigation phase then confirms reachability and writes the finding. The dependency matcher pass runs again whenever the set of vulnerable dependencies changes. +- If you edit `INFO.md` by hand, re-runs keep your edits and only refresh the **Dependency reachability** verdicts. ## Security notes @@ -56,5 +57,5 @@ The resolver treats the manifest as untrusted input. `uv` runs with `--no-build` ## Limitations -- Range resolution checks the lowest and highest allowed versions, not every version in between, so an advisory that affects only a middle version (introduced and fixed inside the range) can be missed. +- Range resolution checks the lowest and highest allowed versions of each declared dependency, not every version in between, so an advisory that affects only a middle version (introduced and fixed inside the range) can be missed. - A resolved version is hypothetical: it is what the range permits, not necessarily what is installed. Findings from resolved versions are applicable-if-resolved, not confirmed-present, which is why the threat model labels them. diff --git a/packages/deepsec/src/__tests__/osv-enrich.test.ts b/packages/deepsec/src/__tests__/osv-enrich.test.ts index c830148..6d7cf04 100644 --- a/packages/deepsec/src/__tests__/osv-enrich.test.ts +++ b/packages/deepsec/src/__tests__/osv-enrich.test.ts @@ -4,6 +4,7 @@ import path from "node:path"; import { describe, expect, it } from "vitest"; import { buildContext, + extractReachabilityBlock, findManifest, findManifests, findPom, @@ -11,6 +12,7 @@ import { mergeInfoWithOsvContext, OSV_CONTEXT_HEADING, parseOsv, + spliceReachabilityBlock, } from "../setup/osv-enrich.js"; // osv-scanner's JSON groups packages under results[].packages[], each result @@ -305,3 +307,84 @@ describe("buildContext", () => { expect(ctx).toContain("PYSEC-2024-60: ReDoS"); }); }); + +describe("mergeInfoWithOsvContext (section boundaries)", () => { + it("removes only the osv section, keeping curated sections that follow it", () => { + const info = `# app\n\n## What this codebase does\nApp.\n\n${OSV_CONTEXT_HEADING}\n\n- \`x@1\`\n - CVE-1\n\n## Threat model\nCurated threats.\n`; + const out = mergeInfoWithOsvContext(info, ""); + expect(out).not.toContain(OSV_CONTEXT_HEADING); + expect(out).not.toContain("CVE-1"); + expect(out).toContain("## Threat model\nCurated threats."); + expect(out).toContain("## What this codebase does\nApp."); + }); +}); + +describe("dependency reachability splice", () => { + const curated = + "# app\n\n## Threat model\nCurated threats.\n\n## Project-specific patterns to flag\nRoutes.\n"; + const block = + "### Dependency reachability\n- starlette@0.46.0 — HIGH reachability — r.py:1 — DoS"; + + it("extracts the agent's block and stops at the next section", () => { + const fresh = `# app\n\n## Threat model\nX.\n\n${block}\n\n## Project-specific patterns to flag\nY.\n`; + expect(extractReachabilityBlock(fresh)).toBe(block); + expect(extractReachabilityBlock(curated)).toBeUndefined(); + }); + + it("inserts the block at the end of the Threat model section", () => { + const out = spliceReachabilityBlock(curated, block); + expect(out).toContain("Curated threats."); + expect(out.indexOf("Curated threats.")).toBeLessThan(out.indexOf(block)); + expect(out.indexOf(block)).toBeLessThan(out.indexOf("## Project-specific patterns to flag")); + }); + + it("replaces a previous block instead of accumulating", () => { + const once = spliceReachabilityBlock(curated, block); + const newer = + "### Dependency reachability\n- anyio@4.0.0 — LOW reachability — not called — n/a"; + const twice = spliceReachabilityBlock(once, newer); + expect(twice.match(/### Dependency reachability/g)).toHaveLength(1); + expect(twice).toContain("anyio@4.0.0"); + expect(twice).not.toContain("starlette@0.46.0"); + }); + + it("removes stale verdicts when given no block", () => { + const out = spliceReachabilityBlock(spliceReachabilityBlock(curated, block), undefined); + expect(out).not.toContain("Dependency reachability"); + expect(out).toContain("## Project-specific patterns to flag\nRoutes."); + }); +}); + +describe("parseOsv vendored-tree filter", () => { + const result = (p: string) => ({ + source: { path: p }, + packages: [ + { + package: { name: "requests", version: "2.0.0", ecosystem: "PyPI" }, + vulnerabilities: [{ id: "CVE-X" }], + }, + ], + }); + + it("does not drop a project that itself lives under a vendor/ directory", () => { + const root = "/home/ci/vendor/acme-api"; + const raw = JSON.stringify({ results: [result(`${root}/requirements.txt`)] }); + expect(parseOsv(raw, undefined, root)).toHaveLength(1); + }); + + it("still drops vendored trees inside the project", () => { + const root = "/home/ci/acme-api"; + const raw = JSON.stringify({ + results: [ + result(`${root}/node_modules/x/requirements.txt`), + result(`${root}/.deepsec/a.txt`), + ], + }); + expect(parseOsv(raw, undefined, root)).toEqual([]); + }); + + it("keeps resolved lockfiles that live outside the project root", () => { + const raw = JSON.stringify({ results: [result("/tmp/deepsec-osv-1/requirements.txt")] }); + expect(parseOsv(raw, undefined, "/home/ci/acme-api")).toHaveLength(1); + }); +}); diff --git a/packages/deepsec/src/__tests__/setup-coordinator.test.ts b/packages/deepsec/src/__tests__/setup-coordinator.test.ts index c772e05..f810b9e 100644 --- a/packages/deepsec/src/__tests__/setup-coordinator.test.ts +++ b/packages/deepsec/src/__tests__/setup-coordinator.test.ts @@ -259,26 +259,85 @@ describe("one-shot setup coordinator", () => { curatedInfo, ); - // Enabling osv on a project that already has a complete INFO.md must NOT - // discard the freshly analyzed reachability verdicts by restoring the old - // copy with the raw grading directive appended. The fresh analysis wins. + // Enabling osv on a project with a curated INFO.md keeps the curation and + // splices in only the fresh "### Dependency reachability" verdicts the new + // analysis produced; the raw grading directive is never persisted. fs.writeFileSync(path.join(workspace, "data", "app", "INFO.md"), curatedInfo); - const infoAfterOsv = await (async () => { - await runSetupWorkflow({ - workspaceDir: workspace, - projectId: "app", - projectRoot: project, - through: "threat-model", - osv: true, - services, - onLog: () => undefined, - }); - return fs.readFileSync(path.join(workspace, "data", "app", "INFO.md"), "utf8"); - })(); - expect(analyze).toHaveBeenCalledTimes(3); // osv flag invalidated the info checkpoint + analyze.mockImplementationOnce(async () => ({ + infoMarkdown: + "# app\n\n## What this codebase does\nFresh.\n\n## Auth shape\nNone.\n\n## Threat model\nFresh threats.\n\n### Dependency reachability\n- starlette@0.46.0 — HIGH reachability — src/route.ts:1 — multipart DoS\n\n## Project-specific patterns to flag\nRoutes.\n\n## Known false-positives\nNone.", + surfaces: [ + { + id: "http-routes", + kind: "http" as const, + description: "HTTP routes", + fileGlobs: ["src/**/*.ts"], + representativeFiles: ["src/route.ts"], + exposure: "public" as const, + }, + ], + inspectedPaths: ["src/route.ts"], + })); + await runSetupWorkflow({ + workspaceDir: workspace, + projectId: "app", + projectRoot: project, + through: "threat-model", + osv: true, + services, + onLog: () => undefined, + }); + const infoAfterOsv = fs.readFileSync(path.join(workspace, "data", "app", "INFO.md"), "utf8"); + expect(analyze).toHaveBeenCalledTimes(3); // enabling osv invalidated the info checkpoint + expect(infoAfterOsv).toContain("Reject