fix(supply-chain): exclude pyproject metadata keys from dependency extraction#28
Conversation
rng1995
left a comment
There was a problem hiding this comment.
This is a clean, well-targeted fix for a real and embarrassing false positive (flagging the requires-python metadata key as the malicious PyPI package of the same name). I checked it against the existing analyzer and it holds up.
Correctness — verified
- The root cause is addressed properly: instead of line-parsing
pyproject.tomlwith the requirements.txt parser, dependencies are now read only from PEP 621[project].dependencies/optional-dependenciesand PEP 735[dependency-groups]. Metadata keys (requires-python,name,version, …) are structurally never considered, so the false HIGH disappears. - The PEP 508 parsing regex and the
version = … if op in ("==", "<=")logic are identical to_extract_packages_from_requirements, so extraction semantics stay consistent with the rest of the analyzer and no new behavior is introduced for the name/version handling. Reusing the established convention is the right call here. - Routing (
"pyproject.toml" in lower_path→ new extractor, everything else → existing one) keeps the change scoped to the reported case;requirements.txt,setup.py, andPipfileare untouched. - Using stdlib
tomllibadds no dependency (project targets 3.12+), andTOMLDecodeError→[]is the fail-safe direction for a false-positive fix. - The
isinstance(..., str)guards correctly skip PEP 735{include-group = ...}table entries and any non-string list items.
Tests
Genuinely thorough for the size of the change: metadata-key exclusion, optional + dependency-group extraction, malformed TOML, no-[project] table, include-group / non-PEP 508 skipping, the vanilla-project regression for the original issue, and confirmation that a real vulnerable dependency in pyproject.toml is still flagged via SC4. That last test is the important one and it's present.
Minor / optional (non-blocking)
- Malformed TOML is silently swallowed to
[]. That's the right default for avoiding false positives, but alogger.debug(orwarning) noting that apyproject.tomlfailed to parse would help operators notice a genuinely broken/obfuscated manifest rather than have it silently contribute zero dependencies. - Inherited from the shared regex (not introduced here, so just noting): package names beginning with a digit won't match, and only
==/<=pins record a version. Fine for name-based OSV/advisory lookups; if version-range precision is ever tightened for>=/~=deps, both extractors would want to evolve together.
Nice work — approving.
|
@CharmingGroot - Please resolve the conflicts, minor issues and merge the PR. |
…traction SC4 reported a HIGH "Known Vulnerable Dependency" on the `requires-python` key of pyproject.toml, matching it to the malicious PyPI package literally named `requires-python` (MAL-2025-41747). pyproject.toml was parsed with the requirements.txt line extractor, which treats any `key = value` line as a package, so PEP 621 metadata keys (`requires-python`, `name`, `version`, ...) were looked up as packages — a false HIGH on essentially every Python project. Add `_extract_packages_from_pyproject` (using stdlib `tomllib`) that pulls package names only from PEP 621 `dependencies` / `optional-dependencies` and PEP 735 `dependency-groups`, and route pyproject.toml to it. requirements.txt, setup.py, and Pipfile keep the existing extractor. Add tests for metadata-key exclusion, optional/group dependency extraction, malformed TOML, and that real vulnerable deps in pyproject are still flagged. Signed-off-by: CharmingGroot <ohyes9711@gmail.com>
f7a7757 to
9ba05b5
Compare
|
Conflicts resolved and force-pushed. Ready to merge. |
… parser improvements Upstream main (PR NVIDIA#28) landed an independent pyproject.toml fix that added PEP 735 [dependency-groups] support but dropped [build-system].requires. Merge resolution: - Remove the duplicate _extract_packages_from_pyproject definition - Add [build-system].requires back to the upstream version - Add empty-content early-return guard Also carry forward fixes from the NVIDIA#67 branch: - AS1 patterns now match absolute paths (open("/Users/x/.claude/...")) - Per-(rule_id, line) deduplication in agent snooping analyze() - Remove duplicate TestRunStaticPatternsAgentSnooping class - Remove unused rule_ids variable in test_patterns_new.py 880 tests pass, ruff clean. Signed-off-by: Lalit Shrotriya <shrotriya.lalit@outlook.com>
Fixes #2.
What this fixes
The Supply Chain analyzer (SC4) reported a HIGH "Known Vulnerable Dependency" on the
requires-pythonkey ofpyproject.toml, matching it to the malicious PyPI advisory MAL-2025-41747 (a real malicious package literally namedrequires-python).Root cause:
pyproject.tomlwas parsed with_extract_packages_from_requirements, the requirements.txt line parser. It treats any line beginning with a name as a package, so PEP 621[project]metadata keys (requires-python,name,version, …) were extracted as package names and looked up in OSV.requires-pythonappears in essentially every modern Python project, so this produced a false HIGH (and inflated risk scores towardDO_NOT_INSTALL) on almost anypyproject.toml.Reproduced on v2.1.3 with a vanilla project:
The fix
Add
_extract_packages_from_pyproject(using stdlibtomllib, no new dependency — the project already targets 3.12+) that collects package names only from PEP 621dependencies/optional-dependenciesand PEP 735dependency-groups, parsing PEP 508 requirement strings._analyze_dependenciesroutespyproject.tomlto it;requirements.txt,setup.py, andPipfilekeep the existing extractor, so this change is scoped to the false positive in the issue.After the fix,
requires-python(and other metadata keys) are no longer treated as packages, while real dependencies are still extracted and evaluated.Testing
ruff check src/ tests/andruff format --check src/ tests/pass.pytest -m 'not integration'passes (607 passed, 11 skipped). The integration suite needs an LLM endpoint and is unaffected.[project]tables, non-PEP 508 / include-group entries, and that a real vulnerable dependency in pyproject is still flagged (SC4 via static fallback).