From 892d2983b0468b58c45568d5c247e56484236d74 Mon Sep 17 00:00:00 2001 From: Parvesh Saini <97528080+parveshsaini@users.noreply.github.com> Date: Thu, 18 Jun 2026 18:30:07 +0530 Subject: [PATCH] Fix: scan_packages.py --fix crash on download_packages() tuple return (#6413) * Fix scan_packages.py --fix crash on download_packages() tuple return `download_packages()` returns `(results, download_errors)`, but the two `--fix`-path call sites still treated the return value as the bare results list. `find_safe_version` did `downloaded = download_packages(...)` followed by `if not downloaded:` (always false: a 2-tuple is truthy) and `for _, archive_path in downloaded:`, which unpacked the results list into two variables -> ValueError in the normal single-archive `--no-deps` case. `_run_fix` indexed `downloaded[0][1]`, i.e. the second archive of the results list instead of the first archive's path -> IndexError. So `--fix` crashed exactly when a CRITICAL finding needed remediation. The main scan path already unpacks the tuple; this aligns the two `--fix` sites with it. Adds CPU-only regression tests for both sites. Closes #6412 Co-Authored-By: Claude Opus 4.8 * Update scripts/scan_packages.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update scripts/scan_packages.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --------- Co-authored-by: Claude Opus 4.8 Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --- scripts/scan_packages.py | 9 ++++- tests/security/test_scan_packages.py | 58 ++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 2 deletions(-) diff --git a/scripts/scan_packages.py b/scripts/scan_packages.py index 9f22035001..afa1189864 100644 --- a/scripts/scan_packages.py +++ b/scripts/scan_packages.py @@ -2154,8 +2154,10 @@ def find_safe_version( scan_dir = os.path.join(tmpdir, f"{name}_{ver}") os.makedirs(scan_dir, exist_ok = True) - downloaded = download_packages([spec], scan_dir) + downloaded, download_errors = download_packages([spec], scan_dir) if not downloaded: + for err in download_errors: + print(f" [WARN] {err}", file = sys.stderr) continue clean = True @@ -2288,9 +2290,12 @@ def _run_fix(critical_pkgs: set[str], entries: list[dict], max_search: int) -> N # If no pinned version, download to find what pip resolves dl_dir = os.path.join(tmpdir, f"resolve_{pkg_name}") os.makedirs(dl_dir, exist_ok = True) - downloaded = download_packages([pkg_name], dl_dir) + downloaded, download_errors = download_packages([pkg_name], dl_dir) if downloaded: current_ver = get_downloaded_version(downloaded[0][1]) + else: + for err in download_errors: + print(f" [WARN] {err}", file = sys.stderr) shutil.rmtree(dl_dir, ignore_errors = True) if not current_ver: diff --git a/tests/security/test_scan_packages.py b/tests/security/test_scan_packages.py index c9b64a9da4..19205a2842 100644 --- a/tests/security/test_scan_packages.py +++ b/tests/security/test_scan_packages.py @@ -548,3 +548,61 @@ def test_per_spec_sdist_only_is_not_error(tmp_path, monkeypatch): sp._resolve_per_spec_with_deps(["x==1.0.0"], str(tmp_path), {}, errors) assert errors == [] # sdist-only handled, not an exit-2 failure assert any(p.name.endswith(".tar.gz") for p in tmp_path.iterdir()) + + +# --------------------------------------------------------------------------- +# --fix path: download_packages() returns (results, download_errors); both +# --fix call sites must unpack the tuple, not treat it as the results list. +# --------------------------------------------------------------------------- + + +def test_find_safe_version_handles_download_tuple(monkeypatch): + # One downloaded archive, returned as the real (results, download_errors) tuple. + monkeypatch.setattr(sp, "fetch_pypi_versions", lambda name: ["0.9.0", "1.0.0"]) + monkeypatch.setattr( + sp, + "download_packages", + lambda specs, dest, **kw: ([("foo==0.9.0", "/tmp/foo-0.9.0.whl")], []), + ) + monkeypatch.setattr(sp, "scan_archive", lambda archive_path, name: []) # clean + monkeypatch.setattr(sp.os, "makedirs", lambda *a, **k: None) + monkeypatch.setattr(sp.os, "remove", lambda *a, **k: None) + monkeypatch.setattr(sp.shutil, "rmtree", lambda *a, **k: None) + + # bad_ver 1.0.0 -> the only older candidate is 0.9.0, which is clean. + result = sp.find_safe_version("foo", "1.0.0", "/tmp/ignored", max_search = 10) + assert result == "0.9.0" + + +def test_run_fix_uses_first_archive_path(monkeypatch): + monkeypatch.setattr( + sp, + "download_packages", + lambda specs, dest, **kw: ([("foo", "/tmp/foo-1.2.3.whl")], []), + ) + seen = {} + + def fake_get_downloaded_version(path): + seen["path"] = path + return "1.2.3" + + monkeypatch.setattr(sp, "get_downloaded_version", fake_get_downloaded_version) + monkeypatch.setattr(sp, "find_safe_version", lambda *a, **k: None) + monkeypatch.setattr(sp.os, "makedirs", lambda *a, **k: None) + monkeypatch.setattr(sp.shutil, "rmtree", lambda *a, **k: None) + + # CRITICAL package with no pinned version -> must download to resolve it, + # reaching downloaded[0][1] (the first archive's path). + entries = [ + { + "name": "foo", + "is_git": False, + "spec": "foo", + "source_file": None, + "raw_line": "foo", + "line_num": 1, + } + ] + sp._run_fix({"foo"}, entries, max_search = 10) # must not raise + + assert seen.get("path") == "/tmp/foo-1.2.3.whl"