Skip to content

fix: read exact versions from Python lockfiles for OSV#263

Open
Root-Aamir wants to merge 1 commit into
NVIDIA:mainfrom
Root-Aamir:fix/osv-query-with-version
Open

fix: read exact versions from Python lockfiles for OSV#263
Root-Aamir wants to merge 1 commit into
NVIDIA:mainfrom
Root-Aamir:fix/osv-query-with-version

Conversation

@Root-Aamir

Copy link
Copy Markdown

Fixes #233.
This adds support for reading exact package versions from Python TOML lockfiles such as uv.lock and poetry.lock, so OSV queries use the installed package version instead of falling back to package-name-only lookups.
Changes:

  • Detect uv.lock and poetry.lock as Python dependency files.
  • Parse [[package]] entries and their exact versions.
  • Pass extracted versions through the existing OSV query flow.
  • Add regression tests for uv.lock and poetry.lock version handling.
    Test:
  • ..venv\Scripts\python.exe -m pytest tests\nodes\analyzers\test_static_patterns_supply_chain_lockfiles.py -q

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Automated SkillSpector Review]

This PR adds a _extract_packages_from_toml_lock() parser for uv.lock/poetry.lock, registers those filenames in the three dependency-file gates in static_patterns_supply_chain.py (SC1 analyze(), _analyze_dependencies(), and node()), and routes lockfiles through the existing SC4 OSV flow with exact versions. Two regression tests are added. The core parsing logic is sound (I verified both new tests pass, TOML decode errors are handled, and non-dict/malformed entries are skipped), and all three gate lists were updated consistently.

Blockers

  1. CI lint/format will fail. Verified locally against the repo's ruff config (ruff 0.15): ruff check fails with I001 on the new test file, and ruff format --check would reformat both changed files (extra blank line before / only one blank line after _extract_packages_from_toml_lock in the source file; missing blank lines between top-level defs in the test file). CI runs both ruff check src/ tests/ and ruff format --check src/ tests/, so this cannot merge as-is. The test file also starts with a UTF-8 BOM (EF BB BF) and uses a one-line comment instead of the repo's standard 14-line SPDX/Apache header used by every other test file.
  2. DCO check will fail. The single commit (2667e97) has no Signed-off-by: line; CI explicitly greps every commit in the PR range for it.
  3. Does not actually fix the #233 repro. Issue #233 is "OSV analyzer reports historical CVEs even when fixed version installed." In that scenario the project has pyproject.toml (or requirements) entries without exact pins plus a lockfile. This PR adds a parallel scan of the lockfile with exact versions, but the pyproject/requirements pass is unchanged: I verified _extract_packages_from_pyproject still yields ('mlx', None) for mlx>=0.31, which produces a version-less OSV query returning all historical CVEs — so the spurious CRITICAL SC4 finding from #233 still fires from pyproject.toml. To fix #233, lockfile versions should be used to resolve/refine unpinned entries from manifest files (or at minimum the finding from the manifest suppressed when the locked version is clean). If the intended scope is only "also scan lockfiles," the PR should not claim Fixes #233.

Non-blocking

  • Wrong line attribution for findings. content.find(f'name = "{name}"') matches the first occurrence, which in uv.lock is often a dependencies = [{ name = "..." }] entry of an earlier package, not the package's own [[package]] block. Verified: in a realistic uv.lock, requests was attributed to the line inside another package's dependencies list. Consider scanning for the [[package]] block position instead.
  • SC1 gate on lockfiles is semantically odd. Lockfiles are fully pinned by definition, so running "Unpinned Dependencies" patterns over them adds scan cost and future FP surface for no signal. Suggest limiting lockfiles to the SC4–SC6 path only.
  • Scale/noise consideration. uv.lock contains the full transitive graph (often hundreds of packages, including the project itself). query_batch sends one unchunked POST (OSV caps at 1000 queries/request) and _fetch_vuln_details issues up to 10 GETs per vulnerable package; SC5/SC6 will now also run over every transitive dep, which may add typosquat/abandoned noise for dependencies the skill author doesn't control. Worth a comment or a cap.
  • Test coverage is happy-path only. No tests for malformed TOML, missing version field, non-dict package entries, or the node()-level gating; no assertion on reported line numbers.


version_value = version.strip() if isinstance(version, str) and version.strip() else None
marker = f'name = "{name}"'
idx = content.find(marker)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

content.find(marker) returns the first occurrence of name = "<pkg>", which in uv.lock is frequently a dependencies = [{ name = "..." }] entry of an earlier package rather than this package's own [[package]] block, so SC4/SC5/SC6 findings get anchored to the wrong line. Verified with a realistic uv.lock: requests was attributed to another package's dependencies list. Consider locating the enclosing [[package]] block (e.g., regex over \[\[package\]\]\s*\nname = "...") or tracking positions while parsing.

is_dep_file = any(
n in file_path.lower()
for n in ["requirements", "package.json", "pyproject.toml", "setup.py", "pipfile"]
for n in ["requirements", "package.json", "pyproject.toml", "setup.py", "pipfile", "uv.lock", "poetry.lock"]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding uv.lock/poetry.lock to the SC1 ("Unpinned Dependencies") gate is semantically inverted — lockfiles are fully pinned by definition, so these patterns can only ever produce false positives or wasted scanning here. Suggest keeping lockfiles out of the SC1 gate and only routing them through the SC4–SC6 dependency analysis.

lower_path = file_path.lower()
is_python_dep = any(
n in lower_path for n in ["requirements", "pyproject.toml", "setup.py", "pipfile"]
n in lower_path for n in ["requirements", "pyproject.toml", "setup.py", "pipfile", "uv.lock", "poetry.lock"]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds a parallel lockfile scan, but the pyproject.toml/requirements pass still extracts unpinned entries as (name, None) and queries OSV without a version, returning all historical CVEs — the exact false-positive reported in #233 will still fire from the manifest file even when the lockfile pins a clean version. To fix #233, prefer the locked version for packages that also appear in manifest files (or suppress the manifest finding when the locked version has no advisories).

@@ -0,0 +1,38 @@
# Licensed under the Apache License, Version 2.0 (the "License");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file starts with a UTF-8 BOM (EF BB BF) and uses a single truncated comment line instead of the repo's standard SPDX/Apache-2.0 header (see any sibling test file). It also fails ruff check (I001) and ruff format --check (missing blank lines between top-level defs), both of which run in CI over tests/. Please strip the BOM, add the full header, and run ruff format.

name = "requests"
version = "2.31.0"
"""
supply_chain._analyze_dependencies(content, "uv.lock")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coverage is happy-path only. Consider adding cases for: malformed TOML (returns [] rather than raising), a [[package]] entry missing version (name passed with version=None), and the node()-level gate so a uv.lock component actually reaches _analyze_dependencies end-to-end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OSV analyzer reports historical CVEs even when fixed version installed

2 participants