Package scanners: close fail-open gaps in the sdist fallback and hidden-payload paths#6359
Conversation
…en-payload paths Follow-up hardening on the now-blocking scanners so the enforcing gate cannot report clean while a malicious artifact goes unscanned. scan_packages.py - Hidden payload: also flag a network call AND an os/subprocess exec that live only in a blanked docstring/string of an exec/eval file (the fetch-then-run shape of an exec(__doc__) dropper). Either alone in real code was already covered; hidden together they are the payload. - Pinned releases fail closed: _release_files no longer falls back to the latest artifact when a pinned version is missing or empty, so a yanked/bad pin is an error instead of a different file being scanned in its place. - requires_dist is read from the pinned release's metadata, not the project-level (latest) document, so a sdist-only pin follows its own dependency tree. - Environment markers are evaluated (PEP 508) instead of dropping any marker that merely contains the word extra, so default-true markers like extra != 'dev' are kept; conservative fallback keeps a dep on any uncertainty. - Transitive recovery is a depth-bounded worklist: a wheel dependency whose own child is sdist-only is fetched (--no-deps) and scanned, then its children are recovered in turn, rather than being silently skipped. scan_npm_packages.py - Baseline keys use the package-relative path instead of the basename, so the same basename in a different directory is not over-suppressed. Tests cover each case; full scripts pass AST and ruff checks.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves package scanning security and dependency resolution. In scan_npm_packages.py, baseline allowlist matching is updated to use package-relative paths instead of basenames to prevent silent suppression of findings in different directories. In scan_packages.py, a new check is added to detect hidden network and execution payloads in docstrings, and dependency resolution is enhanced with depth-bounded transitive dependency recovery for sdist-only packages, PEP 508 marker evaluation, and fail-closed behavior for missing pinned releases. Corresponding unit tests are also added. The review feedback suggests two improvements: first, using default_environment() when evaluating PEP 508 markers to avoid UndefinedEnvironmentName errors for standard environment variables; second, ensuring that transitive dependencies are also chased in the fallback sdist download path.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| try: | ||
| from packaging.markers import Marker | ||
| return bool(Marker(m).evaluate({"extra": ""})) |
There was a problem hiding this comment.
When evaluating PEP 508 markers using packaging.markers.Marker.evaluate, passing only {"extra": ""} as the environment dictionary will cause UndefinedEnvironmentName to be raised for any marker that references other standard environment variables (such as python_version, sys_platform, etc.). This forces the evaluation to always fall back to the conservative regex check, resulting in unnecessary over-scanning of optional dependencies that use compound markers.
We can avoid this by using packaging.markers.default_environment() to obtain the full environment of the running interpreter, and then overriding only the extra key.
| try: | |
| from packaging.markers import Marker | |
| return bool(Marker(m).evaluate({"extra": ""})) | |
| try: | |
| from packaging.markers import Marker, default_environment | |
| env = default_environment() | |
| env["extra"] = "" | |
| return bool(Marker(m).evaluate(env)) |
| fpath, _serr = _download_sdist_direct(dep_name, dep_ver, dest, meta = meta) | ||
| if fpath is None: | ||
| print(f" [WARN] could not resolve indirect dep {dep}; skipping", file = sys.stderr) |
There was a problem hiding this comment.
In the fallback path where nd_cmd (the wheel download) fails, we fall back to downloading the sdist directly using _download_sdist_direct. However, if this direct sdist download succeeds, we do not chase its transitive dependencies. To ensure completeness and avoid silently skipping dependencies of such fallback sdists, we should also extend the worklist with its declared dependencies when depth < _MAX_DEP_FOLLOWUP_DEPTH, matching the behavior of the other sdist-only branch.
| fpath, _serr = _download_sdist_direct(dep_name, dep_ver, dest, meta = meta) | |
| if fpath is None: | |
| print(f" [WARN] could not resolve indirect dep {dep}; skipping", file = sys.stderr) | |
| fpath, _serr = _download_sdist_direct(dep_name, dep_ver, dest, meta = meta) | |
| if fpath is None: | |
| print(f" [WARN] could not resolve indirect dep {dep}; skipping", file = sys.stderr) | |
| elif depth < _MAX_DEP_FOLLOWUP_DEPTH: | |
| worklist.extend((d, depth + 1) for d in _requires_dist_for(dep_name, dep_ver, meta)) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16949a0d81
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def _hidden(pat): | ||
| return bool(pat.search(original)) and not pat.search(stripped) |
There was a problem hiding this comment.
When an exec(__doc__) payload hides both urllib.request.urlopen(...) and os.system(...) in the docstring, adding any benign visible network call (or visible subprocess call) makes _hidden return False because it searches the entire stripped file. In that scenario the executable payload still lives in the blanked region, but the new hidden network+exec check is skipped and the normal code-only checks can report nothing, so a malicious dropper can pass clean; this should detect matches in the removed spans rather than requiring the pattern to be absent from all real code.
Useful? React with 👍 / 👎.
| return True | ||
| try: | ||
| from packaging.markers import Marker | ||
| return bool(Marker(m).evaluate({"extra": ""})) |
There was a problem hiding this comment.
Because the security-audit scanner runs on ubuntu-latest/Python 3.12 (.github/workflows/security-audit.yml lines 756 and 797), evaluating every non-extra marker against the scanner process now drops dependencies such as payload; sys_platform == 'win32' or payload; python_version == '3.13'. The previous recovery path over-scanned non-extra markers, but after this change an sdist-only package can hide a malicious transitive dependency behind a marker for another supported install target and the CI scan will report clean; only pure extra-gated markers should be skipped, or the evaluation needs to cover all supported target environments.
Useful? React with 👍 / 👎.
| for e in data.get("entries", []): | ||
| try: | ||
| keys.add((_norm_pkg_name(e["package"]), os.path.basename(e["file"]), e["pattern"])) | ||
| keys.add((_norm_pkg_name(e["package"]), _relpath_in_package(e["file"]), e["pattern"])) |
There was a problem hiding this comment.
For baselines generated by the previous scanner, each entry stores only a basename (for example index.js). Loading those entries unchanged means a prior exception for package/dist/index.js is normalized to index.js, and a new finding in package/index.js also normalizes to index.js, so non-empty legacy baselines can still suppress a different same-named file despite this patch's package-relative keying. Since the baseline version was not changed, existing users are not forced to regenerate or review these ambiguous entries.
Useful? React with 👍 / 👎.
| (its ``requires_dist`` can differ from latest); falls back to the | ||
| project-level document when the pin has no separate metadata.""" | ||
| vmeta = _pypi_json(name, version) if version else None | ||
| return _requires_dist_names(vmeta or project_meta) |
There was a problem hiding this comment.
For a pinned sdist-only package, if the version-specific PyPI JSON request times out, 404s, or is otherwise unreadable, this falls back to the project-level document, whose requires_dist describes the latest release rather than the scanned pin. That reopens the old fail-open path for transitive recovery: the pinned artifact itself is scanned, but any dependencies unique to that pinned release are replaced by latest-release dependencies and can be missed; when a pin's metadata cannot be read, the scanner should fail closed or otherwise avoid substituting unrelated metadata.
Useful? React with 👍 / 👎.
…fail closed on missing pin metadata - Markers: keep any dep whose marker can hold on another install target (sys_platform == 'win32', python_version == '3.13'); only drop a marker that depends solely on extra and is false with no extra. A scanner runs on one target but must cover code installed on others. Pure-extra markers are evaluated against default_environment() with extra unset. - Hidden dropper: the network+exec docstring check now inspects the removed (blanked) span directly, so a benign visible network or subprocess call cannot mask a payload that still lives in a docstring. Carrier checks stay blanked-only (an in-code carrier is already caught by the normal check), so corpus findings are unchanged. - requires_dist: a pinned version whose own metadata cannot be fetched recovers nothing rather than substituting the latest release's dependency tree. - Transitive recovery: the last-ditch direct-sdist branch also chases the recovered package's declared deps, matching the other branches. - npm baseline: schema bumped to v2 (package-relative keys); a pre-v2 baseline with entries is ignored (fail closed) instead of mis-applying basename keys. Tests cover each case; scripts pass AST, ruff, and the import-hoist verifier.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80379b10c6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # live in the blanked region. Search the removed span directly (not "absent | ||
| # from real code") so a benign visible network/subprocess call cannot mask | ||
| # the docstring payload. | ||
| if RE_NETWORK.search(removed) and RE_SUBPROCESS.search(removed): |
There was a problem hiding this comment.
When a package file contains a legitimate exec() in real code and only comments/prose mention both a network call and subprocess.run, _strip_noncode blanks those comments and removed retains them, so this new condition raises a HIGH "hidden network+exec payload". Comments cannot be executed by exec(__doc__) or string execution, and the scanner otherwise strips comments to avoid prose false positives, so doc examples beside unrelated exec usage can now fail the gate.
Useful? React with 👍 / 👎.
| if any(v in m for v in _MARKER_ENV_VARS): | ||
| return True # also platform/python gated: true on some target -> scan |
There was a problem hiding this comment.
For metadata such as dep; extra == 'gpu' and sys_platform == 'linux', this branch returns True just because the marker mentions a platform variable. _requires_dist_names then treats that optional-extra dependency as part of the default dependency tree, so the --with-deps fallback downloads and scans packages users would not install unless they request the extra, producing spurious security failures; evaluate the marker with extra='' before keeping platform+extra conjuncts.
Useful? React with 👍 / 👎.
| f" [WARN] no metadata for pinned {name}=={version}; not substituting latest deps", | ||
| file = sys.stderr, | ||
| ) | ||
| return [] |
There was a problem hiding this comment.
When a pinned sdist-only package is recovered and the /pypi/<name>/<version>/json request fails or lacks metadata, this return [] makes _resolve_per_spec_with_deps record no download_errors; the root sdist is still scanned and the job can exit 0 even though none of that release's declared dependencies were enumerated. In the --with-deps CI path, a transient PyPI JSON failure therefore becomes a silent partial scan, so propagate an incomplete-scan error or only fall back to project_meta when it is known to describe the same version.
Useful? React with 👍 / 👎.
… metadata as incomplete Hidden network+exec detection now inspects only docstring/string spans (what exec(__doc__)/exec(<str>) can actually run), so a real exec() beside comments that mention a network and a subprocess call no longer false-positives. Missing pinned-release metadata in transitive recovery records a download_error so the --with-deps path fails closed instead of treating it as no dependencies. Adds regression tests for both.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Pushed a follow-up (d3b335e) closing the two remaining review points on the scanners:
Added regression tests for both cases. Full |
…lowup # Conflicts: # tests/security/test_scan_npm_packages.py # tests/security/test_scan_packages.py
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
for more information, see https://pre-commit.ci
1 participant
Follow-up hardening on the package scanners from #6355. Now that the CI gate is blocking, these close fail-open cases where a scan could report clean while a malicious artifact was never examined. All are pattern/logic fixes with no behavior change for legitimate packages.
scan_packages.py
exec(__doc__)file whose docstring carriesurllib.request.urlopen(...)plusos.system(...)produced no findings. The hidden-payload pass now flags a network call AND an os/subprocess exec that live only in a blanked region. Either alone in real code is already covered; hidden together they are the payload._release_filesno longer falls back to the latest artifact when a pinned version is absent or empty. A yanked or bad pin is now an error rather than a different file being scanned in its place.requires_distis read from the pinned release's metadata, not the project-level (latest) document, so a sdist-only pin follows its own dependency tree.extra. Default-true markers such asextra != 'dev'orpython_version >= '3.8' or extra == 'dev'are kept; on any parse or eval uncertainty the dependency is kept (over-scan, never silently skip).--no-depsand scanned, then its children are recovered in turn, instead of being skipped.scan_npm_packages.py
package/) instead of the basename, so the same basename in a different directory is not over-suppressed. Mirrors the Python scanner.Tests
Added coverage for each case (hidden network+exec combo, pinned-missing fail-closed, version-accurate
requires_dist, marker evaluation, npm path keying) and updated the existing sdist/baseline tests for the new behavior. Full security suite passes; both scripts pass AST andruff check.