Package scanners: cut false positives and make the CI gate blocking#6355
Conversation
scan_packages.py and scan_npm_packages.py red-failed on legitimate library code, so the security-audit steps were left advisory. Reduce the false positives at the source and flip both gates to blocking. scan_packages.py: - Scan code only: blank comments and bare docstrings/doctests before matching (line numbers preserved), so prose and >>> examples cannot trip a finding. - Drop the platform.system() branch from the anti-analysis regex (under DOTALL it matched across the whole file, so every cross-platform library tripped it) and fix the dead /proc/self/status alternative. - Add a reviewed baseline allowlist (scan_packages_baseline.json) keyed on (package, basename, check): only non-baselined CRITICAL/HIGH exit 1, and a new kind of finding in a listed file still fails. - sdist fallback: when --with-deps cannot resolve a shard (a sdist-only package or a version conflict), drop to per-spec and fetch the raw sdist from the PyPI JSON API (no pip build, no setup.py), so every package is still scanned and no shard exits 2. scan_npm_packages.py: - Mirror the code-only JS/TS scanning (blank // and /* */ comments, string/template/regex aware) and the baseline allowlist. The npm corpus is clean today, so the baseline is empty. security-audit.yml: - Flip both scan steps to blocking (SCAN_ENFORCE=1), capturing the scanner exit via PIPESTATUS so tee does not mask it. tests/security: add coverage for the strip, baseline and sdist paths.
for more information, see https://pre-commit.ci
![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 introduces code-only scanning (comment and docstring stripping) and baseline allowlist support to both the Python and NPM package scanners to significantly reduce false positives. It also implements a secure, direct sdist fallback download mechanism from PyPI to preserve scanning coverage when bulk pip downloads fail, alongside comprehensive unit tests. The reviewer suggested improving error handling in the Python comment stripper by logging tokenization exceptions at a debug level rather than silently catching them.
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.
|
|
||
| spans: list[tuple[int, int, int, int]] = [] # (srow, scol, erow, ecol) |
There was a problem hiding this comment.
Avoid using broad, silent exception handlers. When catching Exception to ensure the scanner continues, log the exception (at least at a debug level) to aid in future debugging and troubleshooting.
| spans: list[tuple[int, int, int, int]] = [] # (srow, scol, erow, ecol) | |
| except Exception as e: | |
| logger.debug("Failed to tokenize file, falling back to raw content: %s", e) | |
| return content |
References
- Avoid using broad, silent exception handlers like
except Exception: pass. Instead, log the exception, even if at a debug level, to aid in future debugging.
![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: cfadefd267
ℹ️ 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".
| if nd.returncode == 0: | ||
| print( | ||
| f" [INFO] {name}: full tree unresolvable; scanned the package " | ||
| f"alone (--no-deps).", | ||
| file = sys.stderr, | ||
| ) | ||
| continue |
There was a problem hiding this comment.
When a top-level package has a wheel but one of its transitive dependencies is sdist-only, the per-spec pip download --with-deps --only-binary :all: call fails, then this branch downloads only the top-level package and marks the spec successful. I checked pip download --help, and --no-deps means “Don't install package dependencies,” so the sdist-only transitive dependency that caused the failure is never scanned and no download_errors entry is produced; in the now-enforcing CI gate this can report a clean shard while skipping the exact dependency class the fallback was meant to cover.
Useful? React with 👍 / 👎.
| continue # optional extra; default install would not pull it | ||
| m = _RE_NAME.match(head.strip()) | ||
| if m: | ||
| names.append(m.group(1)) |
There was a problem hiding this comment.
When a directly fetched sdist declares constrained deps, such as Requires-Dist: payload==1.2.3 or <2, this helper appends only the package name. The follow-up loop later downloads that bare name, so it scans whatever version is current on PyPI rather than the version the sdist would install; a malicious pinned transitive version can be missed while the enforcing CI reports success.
Useful? React with 👍 / 👎.
| """ | ||
| return (_norm_pkg(f.package), os.path.basename(f.filename), f.check) | ||
|
|
||
|
|
There was a problem hiding this comment.
The committed Python baseline is now active in CI, but this key collapses every path in a package to only basename(filename). If a future version of a baselined package adds the same check in another file with a common name such as utils.py or __init__.py, _partition_baseline treats it as already reviewed and exits clean; stripping just the archive root while keeping the package-relative path would preserve version stability without suppressing unrelated files.
Useful? React with 👍 / 👎.
| while j < n and toks[j].type in (tokenize.COMMENT, tokenize.NL): | ||
| j += 1 | ||
| if j < n and toks[j].type == tokenize.NEWLINE: | ||
| spans.append((*tok.start, *tok.end)) |
There was a problem hiding this comment.
This blanks every bare STRING token before pattern matching. In supported Python 3.10/3.11, a bare f-string such as f"{__import__('os').system('id')}" is tokenized as STRING and executes at import time, and ordinary docstrings can also be executed later with exec(__doc__); after this blanking the scanner sees only exec(__doc__) and can return clean even though the removed string held the network/exec payload. Please only strip truly inert docstrings, or rescan original strings when they are referenced/executable, so malicious packages cannot hide payloads in bare strings.
Useful? React with 👍 / 👎.
- Do not blank f-strings during code-only scanning (they evaluate at import); and when a file uses exec/eval, rescan the original for payload carriers hidden in a docstring/string so exec(__doc__) style payloads stay visible. - sdist fallback: recover transitive deps with their version specifier (fetch the pinned version, not latest), and recover deps in the --no-deps branch too so a sdist-only transitive dependency is still scanned instead of silently skipped. - Baseline: key by package-relative path, not basename, so a future same-named file in another directory is not auto-suppressed. Regenerated the baseline accordingly.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dabccce55f
ℹ️ 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".
| _HIDDEN_PAYLOAD_PATTERNS = ( | ||
| (RE_LARGE_BLOB, "large base64 blob"), | ||
| (RE_EMBEDDED_KEYS, "embedded key material"), | ||
| (RE_MAY12_IOC, "Shai-Hulud IOC string"), | ||
| (RE_OBFUSCATION, "marshal/compile/obfuscation"), |
There was a problem hiding this comment.
If a package stores a plain Python payload in a docstring and then runs exec(__doc__), _strip_noncode removes the docstring before the normal network/subprocess checks, and this new hidden-payload path only restores coverage for blobs, keys, IOCs, or obfuscation. Fresh evidence from the current patch is that an exec(__doc__) file whose docstring contains urllib.request.urlopen(...) plus os.system(...) still produces no findings, so an enforcing shard can report clean while the import-time payload is skipped.
Useful? React with 👍 / 👎.
| else: | ||
| print(f" [WARN] could not resolve indirect dep {dep}; skipping", file = sys.stderr) |
There was a problem hiding this comment.
When a recovered dependency is itself a wheel package whose dependency tree fails to resolve because one of its own transitive deps is sdist-only, this branch only prints a warning and does not download the wheel with --no-deps, direct-fetch the sdist child, or add a download_errors entry. Fresh evidence is the new follow-up loop: a direct package A can be scanned, but its dependency B and B's sdist-only child C are silently omitted while the enforcing CI shard can still exit 0.
Useful? React with 👍 / 👎.
| info = meta.get("info", {}) or {} | ||
| reqs = info.get("requires_dist") or [] |
There was a problem hiding this comment.
For a pinned sdist fallback, _download_sdist_direct fetches the requested version, but dependency recovery reads meta["info"]["requires_dist"] from the project-level JSON document, which describes the latest release rather than the pinned archive. If oldpkg==1.0.0 is sdist-only and pins a malicious payload==1.0.0, while the latest oldpkg has different or no dependencies, this follows the wrong tree and the enforcing scan can exit clean without ever scanning the payload.
Useful? React with 👍 / 👎.
| if ";" in r: | ||
| head, marker = r.split(";", 1) | ||
| if "extra" in marker: | ||
| continue # optional extra; default install would not pull it |
There was a problem hiding this comment.
This skips every Requires-Dist marker containing the word extra, but not every such marker is optional for the default install: extra != 'dev' or python_version >= '3.8' or extra == 'dev' can still be true when no extra is requested. A malicious sdist can put its pinned payload behind one of those default-true markers; pip would install it, but the fallback never adds it to sdist_dep_followups, so the enforcing shard can report clean with that dependency unscanned.
Useful? React with 👍 / 👎.
| def _finding_key(f: Finding) -> tuple[str, str, str]: | ||
| """Stable allowlist key: normalized package, file basename, pattern.""" | ||
| return (_norm_pkg_name(f.package), os.path.basename(f.filename), f.pattern) |
There was a problem hiding this comment.
Once a non-empty npm baseline is generated, this key collapses every path in a package to only basename(filename). If a reviewed package has a finding in dist/index.js and a later tarball adds the same pattern in src/index.js or another common basename, _partition_baseline suppresses the new file too and the enforcing npm gate can exit clean; keeping the package-relative path would avoid the over-suppression while remaining stable across version bumps.
Useful? React with 👍 / 👎.
| if version: | ||
| files = meta.get("releases", {}).get(version) | ||
| if files: | ||
| return files | ||
| return meta.get("urls", []) or [] |
There was a problem hiding this comment.
When a spec pins a version that is absent from PyPI metadata or has no files, this helper falls back to meta["urls"], i.e. the latest release. In the sdist fallback path, pkg==bad-or-yanked-version can therefore be treated as successfully scanned after downloading a different latest sdist, so the enforcing scan exits clean even though the requested pinned artifact was never examined; pinned versions should fail closed when their release file list is missing or empty.
Useful? React with 👍 / 👎.
1 participant
Summary
The Python and npm package scanners (
scripts/scan_packages.py,scripts/scan_npm_packages.py) red-failed on legitimate library code, so theirsecurity-audit.ymlsteps were left advisory. This reduces the false positives at the source, adds a reviewed baseline allowlist for the residual genuine-library findings, and flips both gates to blocking.scan_packages.py
>>>doctests no longer trip a finding. This was the dominant false-positive source: pandas doctests, httpx usage examples, advisories quoted in comments.platform.system() ... Linux/Windows/Darwinbranch: underre.DOTALLit matched across the whole file, so every cross-platform library tripped it. Also fix the dead/proc/self/statusalternative (a leading\bnext to/is unsatisfiable).scripts/scan_packages_baseline.json). Reviewed known-good CRITICAL/HIGH findings, matched on(package, basename, check). Only non-baselined CRITICAL/HIGH cause exit 1, and a new kind of finding in a listed file is a differentcheckand still fails. New flags:--baseline,--no-baseline,--write-baseline.--only-binary :all:never builds an sdist, but a wheel-less project then cannot be fetched at all, and one such package failed the entire--with-depsresolve (exit 2). When the bulk resolve fails, the scanner now drops to per-spec resolution and fetches the raw sdist tarball directly from the PyPI JSON API (no pip build, nosetup.py) for static scanning, so every package is covered and no shard exits 2. Genuine transport failures still exit 2.scan_npm_packages.py
//and/* */comments while tracking string, template-literal and regex context so a//inside"http://..."is never treated as a comment. Assigned string literals are never blanked, since droppers hide payloads there.security-audit.yml
SCAN_ENFORCE=1), capturing the scanner exit viaPIPESTATUSsoteedoes not mask it.Tests
tests/security/test_scan_packages.pyandtests/security/test_scan_npm_packages.pygain coverage for the strip, baseline and sdist paths. 52 passing.Baseline review
The 165 entries in
scan_packages_baseline.jsonare all attested benign across 52 trusted dependencies: Hugging Face auth headers, retry loops, matplotlib's Sphinxexec, langid loading its own model via marshal/eval, vendored test fixtures, and similar. The npm baseline is empty.