Scanner: exclude comments from hidden-payload check, flag missing pin 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.

Author: danielhanchen

scripts/scan_packages.py

@@ -534,12 +534,14 @@ def _is_fstring(tok_string: str) -> bool:
     return q > 0 and "f" in tok_string[:q].lower()
 
 
-def _strip_noncode(content: str) -> str:
+def _strip_noncode(content: str, blank_comments: bool = True) -> str:
     """Blank comments and bare docstrings so IOC patterns see code only.
 
     Removed regions become spaces (newlines kept) so line numbers stay exact for
     _extract_evidence. Fails open on tokenizer errors (the raw text is still
-    fully scanned, so a real detection is never lost).
+    fully scanned, so a real detection is never lost). ``blank_comments=False``
+    keeps comments (only strings/docstrings blanked) to isolate the span that
+    exec() could actually run.
     """
     try:
         toks = list(tokenize.generate_tokens(io.StringIO(content).readline))
@@ -552,8 +554,9 @@ def _strip_noncode(content: str) -> str:
     for i, tok in enumerate(toks):
         ttype = tok.type
         if ttype == tokenize.COMMENT:
-            spans.append((*tok.start, *tok.end))
-            continue  # do not advance prev_significant; comments are transparent
+            if blank_comments:
+                spans.append((*tok.start, *tok.end))
+            continue  # transparent; never advances prev_significant
         if (
             ttype == tokenize.STRING
             and prev_significant in _LINE_START_TOKENS
@@ -619,10 +622,11 @@ def _hidden_payload_findings(
     scanning yet ``exec(__doc__)`` / ``exec(<str>)`` could still run it."""
     if not RE_EXEC_EVAL.search(stripped):
         return []
-    # The text code-only scanning removed (docstrings/comments/strings), with real
-    # code spaced out. _strip_noncode blanks in place and preserves length, so a
-    # char was removed exactly where stripped differs from original.
-    removed = "".join(o if o != s else " " for o, s in zip(original, stripped))
+    # Only docstrings/strings run via exec(__doc__)/exec(<str>); comments cannot.
+    # Isolate that span: keep comments as real code, take what string-blanking
+    # removed (length-preserved, so offsets stay exact for _extract_evidence).
+    code = _strip_noncode(original, blank_comments = False)
+    removed = "".join(o if o != s else " " for o, s in zip(original, code))
     out = []
 
     def _hidden(pat):
@@ -1705,20 +1709,23 @@ def _requires_dist_names(meta: dict) -> list[str]:
     return specs
 
 
-def _requires_dist_for(name: str, version: str | None, project_meta: dict) -> list[str]:
+def _requires_dist_for(
+    name: str, version: str | None, project_meta: dict, errors: list[str] | None = None
+) -> list[str]:
     """Declared deps for the pinned version, read from that release's metadata
     (its ``requires_dist`` can differ from latest). Unpinned uses the
     project-level (latest) document. A pinned version whose own metadata cannot
-    be fetched returns [] rather than substituting latest's deps, which would
-    scan the wrong tree."""
+    be fetched returns [] (never latest's deps) and, when ``errors`` is given,
+    records an incomplete-scan error so a partial tree is not read as "no deps"."""
     if not version:
         return _requires_dist_names(project_meta)
     vmeta = _pypi_json(name, version)
     if vmeta is None:
-        print(
-            f"  [WARN] no metadata for pinned {name}=={version}; not substituting latest deps",
-            file = sys.stderr,
-        )
+        msg = f"metadata fetch failed for pinned {name}=={version}; dependency scan incomplete"
+        if errors is None:
+            print(f"  [WARN] {msg}", file = sys.stderr)
+        else:
+            errors.append(msg)
         return []
     return _requires_dist_names(vmeta)
 
@@ -1842,7 +1849,7 @@ def _resolve_per_spec_with_deps(
             if fpath is None:
                 download_errors.append(serr or f"sdist fetch failed for {name}")
                 continue
-            sdist_dep_followups.extend(_requires_dist_for(name, version, meta))
+            sdist_dep_followups.extend(_requires_dist_for(name, version, meta, download_errors))
             continue
         # Has a wheel but the full transitive tree won't co-resolve
         # (ResolutionImpossible) -- typically a package the requirement file
@@ -1876,7 +1883,7 @@ def _resolve_per_spec_with_deps(
             # which --no-deps skips. Recover the declared deps so that class is
             # still scanned (each is fetched as a wheel or direct sdist below).
             if meta is not None:
-                sdist_dep_followups.extend(_requires_dist_for(name, version, meta))
+                sdist_dep_followups.extend(_requires_dist_for(name, version, meta, download_errors))
             continue
         # --no-deps also failed: last-ditch sdist fetch at the pinned version.
         if meta is not None:

tests/security/test_scan_packages.py

@@ -447,6 +447,19 @@ def test_hidden_payload_survives_visible_decoy():
     assert any("hidden network+exec payload" in f.check for f in findings)
 
 
+def test_comment_only_network_exec_not_flagged():
+    # Tokens only in comments are not executable by exec(); the hidden network+exec
+    # check inspects strings/docstrings (not comments), so this must stay clean.
+    src = (
+        "code = 'x = 1'\n"
+        "exec(code)\n"
+        "# urllib.request.urlopen('http://host/p').read()\n"
+        "# subprocess.run(['sh', '-c', 'id'])\n"
+    )
+    findings = sp.check_py_file(src, "pkg/ex.py", "pkg")
+    assert not any("hidden network+exec payload" in f.check for f in findings)
+
+
 def test_baseline_suppresses_listed_but_not_new_check(tmp_path):
     bl = tmp_path / "bl.json"
     listed = _mk(sp.CRITICAL, "fastapi", "fastapi/routing.py", "C2 polling/beaconing loop detected")
@@ -608,6 +621,16 @@ def test_requires_dist_for_uses_pinned_release(monkeypatch):
     assert "harmless>=1" not in specs
 
 
+def test_requires_dist_for_records_incomplete_scan_error(monkeypatch):
+    # Missing pinned metadata must surface an incomplete-scan error, not a silent
+    # [] that a caller cannot tell apart from a genuine no-deps release.
+    project = _meta([], requires = ["latestdep==9.9.9"])
+    monkeypatch.setattr(sp, "_pypi_json", lambda name, version = None: None if version else project)
+    errors: list[str] = []
+    assert sp._requires_dist_for("oldpkg", "1.0.0", project, errors) == []
+    assert errors and "incomplete" in errors[0]
+
+
 def test_release_files_pinned_missing_fails_closed():
     # A pin absent from metadata must NOT fall back to the latest artifact.
     meta = _meta(