fix: read exact versions from Python lockfiles for OSV#263
Conversation
rng1995
left a comment
There was a problem hiding this comment.
[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
- CI lint/format will fail. Verified locally against the repo's ruff config (ruff 0.15):
ruff checkfails withI001on the new test file, andruff format --checkwould reformat both changed files (extra blank line before / only one blank line after_extract_packages_from_toml_lockin the source file; missing blank lines between top-level defs in the test file). CI runs bothruff check src/ tests/andruff 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. - 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. - 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_pyprojectstill yields('mlx', None)formlx>=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 claimFixes #233.
Non-blocking
- Wrong line attribution for findings.
content.find(f'name = "{name}"')matches the first occurrence, which in uv.lock is often adependencies = [{ name = "..." }]entry of an earlier package, not the package's own[[package]]block. Verified: in a realistic uv.lock,requestswas 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_batchsends one unchunked POST (OSV caps at 1000 queries/request) and_fetch_vuln_detailsissues 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
versionfield, non-dictpackageentries, or thenode()-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) |
There was a problem hiding this comment.
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"] |
There was a problem hiding this comment.
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"] |
There was a problem hiding this comment.
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"); | |||
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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.
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:
Test: