fix(supply-chain): treat only == / <= as version pins in requirements.txt (#294) - #302
fix(supply-chain): treat only == / <= as version pins in requirements.txt (#294)#302Mark2Mac wants to merge 2 commits into
Conversation
rng1995
left a comment
There was a problem hiding this comment.
[Automated SkillSpector Review]
Requesting changes because the new guard still treats non-exact constraints as concrete installed versions. <=8.1.0 admits every earlier release, and ==1.* is also a wildcard range, so either can still be sent to the vulnerability lookup as a version the dependency may not install. Please retain only truly exact pins, or model ranges explicitly, and add regressions for both cases.
| # version makes a floor like "pillow>=10.0.0" report as "pillow==10.0.0" and | ||
| # attributes that release's CVEs to an unpinned dependency. Mirrors the guard | ||
| # already used by _extract_packages_from_setup_py. | ||
| version = m.group(3) if m.group(2) in ("==", "<=") else None |
There was a problem hiding this comment.
<= is still a range, not a concrete release: pkg<=8.1.0 can install any earlier version, so keeping 8.1.0 here continues the same false CVE attribution. The regex also accepts wildcard equality such as pkg==1.*, which is not exact either. Please retain only a non-wildcard exact equality (or model ranges explicitly), and update both extractors consistently.
|
Gentle ping — happy to rebase or adjust if the approach needs changing. Re-checked against Field data since opening it: scanning 65 skill/plugin units, every SC4 finding in the corpus came from a range being read as a pin (or from the version-less variant I just filed as #318). No manifest actually pinned a vulnerable release. |
….txt
`_extract_packages_from_requirements` kept the captured version for any operator,
so a floor like `pillow>=10.0.0` was recorded as the exact release `10.0.0` and
the OSV/CVE lookup attributed that version's vulnerabilities to an unpinned
dependency — a false CRITICAL on a requirements file that pins nothing.
Only `==` and `<=` bound the dependency to a concrete, CVE-checkable release;
`>=`, `>`, `!=`, `~=` are floors/ranges. This mirrors the guard already present
in `_extract_packages_from_setup_py` (`m.group(2) in ("==", "<=")`), so the two
extractors now agree.
Added a regression test asserting `>=`, `~=`, `!=` and bare names yield
version=None while `==` / `<=` keep the version.
Fixes NVIDIA#294
Signed-off-by: Mark2Mac <Mark2Mac@users.noreply.github.com>
Addresses the review on NVIDIA#302: the previous guard still admitted non-exact constraints. `<=8.1.0` matches every earlier release and `==1.*` is a wildcard, so both were handed to the vulnerability lookup as a version the dependency may never install. A vulnerability lookup answers "is THIS release affected?", which is only meaningful when the manifest admits exactly one release. That predicate is now explicit and shared instead of being re-derived at each call site: - `_pinned_version` (PEP 440): only `==` with a fully concrete version. Floors, caps, exclusions, compatible releases and wildcard equality yield None. - `_pinned_npm_version` (semver): only a bare `x.y.z`. npm defaults to caret ranges, so `"^1.8.3"` was being stripped into the concrete release `1.8.3`. Applied to all three extractors — requirements.txt, pyproject.toml and package.json — because the objection in the review holds verbatim for the two that were not touched by the original patch. Note for the maintainer: dropping these specifiers moves more dependencies to version=None, which NVIDIA#318 shows is currently reported as CRITICAL carrying the package's worst-ever advisory. The two fixes are complementary; happy to send the severity side as a separate PR. Regressions cover both cases named in the review (`<=` and `==1.*`) plus the npm caret/tilde/wildcard/range forms. Signed-off-by: Mark2Mac <Mark2Mac@users.noreply.github.com>
f97651a to
15788af
Compare
|
Rebased onto What changed. The predicate is now explicit and shared instead of being re-derived at each call site:
Applied to all three extractors — One thing worth flagging before this lands, because I measured it. On its own, this PR does not remove a single false positive — it only changes their label. With the patch applied: The fake pin is gone, the CRITICAL is not. Dropping the specifier moves the dependency to I have opened that as a separate PR. Measured together on the same inputs:
The last row is the one I care about most: real version matches are untouched. Happy to squash, split, or reorder these two in whatever way is easiest to review. |
|
Cross-PR note, from merging all my open PRs together locally and running them on real corpora. 1. #323 touches the same function. 2. This PR and #319 interact, in both directions. Worth knowing before either lands. A real example from the corpus:
So the false pin was accidentally producing silence here, and the pair turns it into an honest low-severity note. Measured across four real units: 776 findings → 768, with one unit gaining exactly this LOW entry and another losing eight (that one is #322, unrelated). |
rng1995
left a comment
There was a problem hiding this comment.
[Automated SkillSpector Review]
Re-review: approved. The prior blocker is resolved. _pinned_version() now accepts only concrete non-wildcard == pins, rejecting caps, floors, exclusions, compatible ranges, and wildcard equality; npm resolution likewise accepts only a bare exact semver. Requirements, pyproject, and package.json paths use the shared predicates, with regressions for the specific <= and ==1.* cases plus npm ranges. The focused suite passes (244 tests).
What
_extract_packages_from_requirementsrecorded the captured version for any operator, so a floor likepillow>=10.0.0was stored as the exact release10.0.0. The OSV/CVE lookup then attributed that release's vulnerabilities to a dependency that pins nothing — a false CRITICAL supply-chain finding.Only
==and<=bind a dependency to a concrete, CVE-checkable release.>=,>,!=,~=are floors/ranges. The fix applies the same guard already used by_extract_packages_from_setup_py(m.group(2) in ("==", "<=")), so the two extractors agree.Reproduction (before)
Test
Added
test_extract_packages_requirements_specifier_is_not_a_pin: asserts==/<=keep the version while>=,~=,!=and bare names yieldNone. Verified red before the fix (pillowcame back'10.0.0'), green after. Full existing suite for the analyzer stays green (320 passed).Fixes #294