diff --git a/README.md b/README.md index 9ba6fc0..3ff4d6a 100644 --- a/README.md +++ b/README.md @@ -304,7 +304,7 @@ If you run this action from more than one workflow on the same PR with `comment_ ## How blocking works -The job fails (non-zero exit) when **either** tool finds issues above its configured threshold. +The job fails (non-zero exit) when **either** tool finds issues above its configured threshold. It also fails when pip-audit cannot run, for example when the export fails or the requirements file is missing. **Bandit threshold** (`bandit_severity_threshold`): findings at or above the threshold block the job. @@ -330,7 +330,7 @@ The job fails (non-zero exit) when **either** tool finds issues above its config | `bandit_scan_dirs` | `.` | Comma-separated directories for bandit to scan (relative to `working_directory`) | | `bandit_severity_threshold` | `high` | Minimum severity that blocks the job: `high`, `medium`, or `low` | | `pip_audit_block_on` | `fixable` | When pip-audit findings block the job: `fixable`, `all`, or `none` | -| `package_manager` | `requirements` | How to resolve deps for pip-audit: `uv`, `pip`, `poetry`, `pipenv`, `requirements` | +| `package_manager` | `requirements` | How to resolve deps for pip-audit: `uv`, `poetry`, `pipenv`, `requirements`. For a pip environment, run `pip freeze > requirements.txt` first and use `requirements` | | `requirements_file` | `requirements.txt` | Path to requirements file when `package_manager=requirements` | | `working_directory` | `.` | Directory to run the audit from (useful for monorepos) | | `comment_on` | `never` | When to post a PR comment: `never`, `blocking` (only when issues block the job), or `always` | diff --git a/action.yml b/action.yml index d25f35f..74b4e90 100644 --- a/action.yml +++ b/action.yml @@ -16,7 +16,7 @@ inputs: description: When to block on pip-audit findings — fixable, all, or none default: fixable package_manager: - description: How to resolve dependencies for pip-audit (uv, pip, poetry, pipenv, requirements) + description: How to resolve dependencies for pip-audit (uv, poetry, pipenv, requirements) default: requirements requirements_file: description: Path to requirements file when package_manager=requirements @@ -63,7 +63,7 @@ runs: - name: Run Bandit (static security analysis) if: contains(inputs.tools, 'bandit') continue-on-error: true - uses: lhoupert/bandit-action@18022d5292d04b21fae1bfa44597b94402ba7365 + uses: lhoupert/bandit-action@e2d48932beda5cc8c50cb45ed39f8d873ef2d365 with: targets: ${{ steps.resolve-targets.outputs.targets }} diff --git a/integration-tests/tests/test_validate_results.py b/integration-tests/tests/test_validate_results.py index cee24ab..09ca751 100644 --- a/integration-tests/tests/test_validate_results.py +++ b/integration-tests/tests/test_validate_results.py @@ -4,6 +4,7 @@ - TestParseSarif - TestParsePipAudit - TestValidateTest (pure logic, no I/O) + - TestCheckAudited - TestGenerateReport - TestMain (integration, uses tmp_path + monkeypatch) """ @@ -248,6 +249,46 @@ def test_expected_pip_audit_package_absent_returns_error(self) -> None: assert any("requests" in e for e in errors) +# --------------------------------------------------------------------------- +# check_audited +# --------------------------------------------------------------------------- + + +class TestCheckAudited: + """A pip-audit case without a report, or with nothing audited, must not pass as clean.""" + + def _write(self, tmp_path: Path, deps: list[dict[str, Any]]) -> Path: + path = tmp_path / "pip-audit-report.json" + path.write_text(json.dumps({"dependencies": deps, "fixes": []})) + return path + + def test_missing_report_returns_error(self) -> None: + errors = vr.check_audited({"expected_conclusion": "success"}, None) + + assert any("did not run" in e for e in errors) + + @pytest.mark.parametrize( + "deps", + [[], [{"name": "my-app", "skip_reason": "Dependency not found on PyPI"}]], + ) + def test_zero_audited_dependencies_returns_error( + self, tmp_path: Path, deps: list[dict[str, Any]] + ) -> None: + errors = vr.check_audited({"expected_conclusion": "success"}, self._write(tmp_path, deps)) + + assert any("did not run" in e for e in errors) + + def test_audited_dependency_returns_no_error(self, tmp_path: Path) -> None: + path = self._write(tmp_path, [{"name": "requests", "version": "2.32.0", "vulns": []}]) + + assert vr.check_audited({"expected_conclusion": "success"}, path) == [] + + def test_pip_audit_disabled_needs_no_report(self) -> None: + expected = {"expected_conclusion": "failure", "pip_audit_disabled": True} + + assert vr.check_audited(expected, None) == [] + + # --------------------------------------------------------------------------- # generate_report # --------------------------------------------------------------------------- @@ -314,16 +355,20 @@ def test_happy_path_returns_zero(self, tmp_path: Path, monkeypatch: pytest.Monke } self._write_expected_yml(tmp_path, tests) - # Test 01: clean SARIF, no pip-audit artifact + clean_audit = {"dependencies": [{"name": "requests", "version": "2.32.0", "vulns": []}]} + + # Test 01: clean SARIF, clean pip-audit report art01 = tmp_path / "artifacts" / "security-audit-01" art01.mkdir(parents=True) (art01 / "results.sarif").write_text(json.dumps({"runs": [{"results": []}]})) + (art01 / "pip-audit-report.json").write_text(json.dumps(clean_audit)) - # Test 02: B602 finding in SARIF + # Test 02: B602 finding in SARIF; pip-audit report nested, as artifact upload does art02 = tmp_path / "artifacts" / "security-audit-02" - art02.mkdir(parents=True) + (art02 / "02-case").mkdir(parents=True) sarif02 = {"runs": [{"results": [{"ruleId": "B602", "level": "error"}]}]} (art02 / "results.sarif").write_text(json.dumps(sarif02)) + (art02 / "02-case" / "pip-audit-report.json").write_text(json.dumps(clean_audit)) needs = {"test-01": {"result": "success"}, "test-02": {"result": "failure"}} monkeypatch.setenv("NEEDS_JSON", json.dumps(needs)) @@ -335,6 +380,32 @@ def test_happy_path_returns_zero(self, tmp_path: Path, monkeypatch: pytest.Monke assert vr.main() == 0 + def test_missing_pip_audit_report_returns_one( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + tests = { + "01": { + "name": "clean", + "expected_conclusion": "success", + "bandit_findings": [], + "pip_audit_findings": [], + }, + } + self._write_expected_yml(tmp_path, tests) + art01 = tmp_path / "artifacts" / "security-audit-01" + art01.mkdir(parents=True) + (art01 / "results.sarif").write_text(json.dumps({"runs": [{"results": []}]})) + + monkeypatch.setenv("NEEDS_JSON", json.dumps({"test-01": {"result": "success"}})) + monkeypatch.setattr(vr, "EXPECTED_COUNT", 1) + monkeypatch.setattr(vr, "ARTIFACTS_DIR", tmp_path / "artifacts") + monkeypatch.setattr(vr, "EXPECTED_RESULTS_PATH", tmp_path / "expected_results.yml") + monkeypatch.delenv("GITHUB_STEP_SUMMARY", raising=False) + monkeypatch.chdir(tmp_path) + + assert vr.main() == 1 + assert "did not run" in (tmp_path / "validation-report.md").read_text() + def test_conclusion_mismatch_returns_one( self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: diff --git a/integration-tests/validate_results.py b/integration-tests/validate_results.py index 6b2d740..ac7dd47 100644 --- a/integration-tests/validate_results.py +++ b/integration-tests/validate_results.py @@ -139,6 +139,20 @@ def validate_test( return errors +def check_audited(expected: dict[str, Any], pip_audit_path: Path | None) -> list[str]: + """Error when a pip-audit case has no report or audited zero dependencies. + + The action used to report a crashed or empty audit as clean, so a case + without a report would otherwise pass as "no vulns". + """ + if expected.get("pip_audit_disabled", False): + return [] + deps = json.loads(pip_audit_path.read_bytes())["dependencies"] if pip_audit_path else [] + if not any("skip_reason" not in dep for dep in deps): + return ["pip-audit: no report or zero dependencies audited (the audit did not run)"] + return [] + + # --------------------------------------------------------------------------- # Report generation # --------------------------------------------------------------------------- @@ -251,21 +265,18 @@ def main() -> int: if sarif_path.exists(): bandit_findings = parse_sarif(sarif_path) - pip_audit_path = artifact_dir / "pip-audit-report.json" - if pip_audit_path.exists(): + # Artifact upload uses least common ancestor, so the file may be nested + # e.g. artifacts/security-audit-08/08-poetry-src-both/pip-audit-report.json + pip_audit_path = next(artifact_dir.rglob("pip-audit-report.json"), None) + if pip_audit_path: pip_audit_findings = parse_pip_audit(pip_audit_path) - else: - # Artifact upload uses least common ancestor, so the file may be nested - # e.g. artifacts/security-audit-08/08-poetry-src-both/pip-audit-report.json - nested = next(artifact_dir.rglob("pip-audit-report.json"), None) - if nested: - pip_audit_findings = parse_pip_audit(nested) all_bandit[num] = bandit_findings all_pip_audit[num] = pip_audit_findings conclusion = conclusions.get(num, "missing") errors = validate_test(num, exp, conclusion, bandit_findings, pip_audit_findings) + errors += check_audited(exp, pip_audit_path) all_errors[num] = errors status = "✅" if not errors else "❌" diff --git a/src/python_security_auditing/__main__.py b/src/python_security_auditing/__main__.py index 71f9c98..06bf994 100644 --- a/src/python_security_auditing/__main__.py +++ b/src/python_security_auditing/__main__.py @@ -9,7 +9,13 @@ from .annotations import emit_annotations from .pr_comment import upsert_pr_comment from .report import build_markdown, check_thresholds, write_step_summary -from .runners import generate_requirements, read_bandit_sarif, run_pip_audit +from .runners import ( + PIP_AUDIT_REPORT, + AuditError, + generate_requirements, + read_bandit_sarif, + run_pip_audit, +) from .settings import Settings @@ -21,6 +27,7 @@ def main() -> None: bandit_report: dict[str, Any] = {} pip_audit_report: list[dict[str, Any]] = [] + pip_audit_error = "" if "bandit" in settings.enabled_tools: if settings.debug: @@ -39,23 +46,31 @@ def main() -> None: f"[debug] generating requirements for package_manager={settings.package_manager}", file=sys.stderr, ) - requirements_path = generate_requirements(settings) - if settings.debug: - print(f"[debug] running pip-audit on {requirements_path}", file=sys.stderr) - pip_audit_report = run_pip_audit(requirements_path, settings) + PIP_AUDIT_REPORT.unlink(missing_ok=True) # never upload an earlier run's report + try: + requirements_path = generate_requirements(settings) + if settings.debug: + print(f"[debug] running pip-audit on {requirements_path}", file=sys.stderr) + pip_audit_report = run_pip_audit(requirements_path, settings) + except (AuditError, FileNotFoundError) as exc: + pip_audit_error = str(exc) if settings.debug: print(f"[debug] pip-audit findings: {len(pip_audit_report)}", file=sys.stderr) - markdown = build_markdown(bandit_report, pip_audit_report, settings) + markdown = build_markdown(bandit_report, pip_audit_report, settings, pip_audit_error) write_step_summary(markdown, settings) - emit_annotations(bandit_report, pip_audit_report, settings) + emit_annotations(bandit_report, pip_audit_report, settings, pip_audit_error) - has_blocking = check_thresholds(bandit_report, pip_audit_report, settings) + has_blocking = bool(pip_audit_error) or check_thresholds( + bandit_report, pip_audit_report, settings + ) if settings.github_token and settings.comment_on != "never": if settings.comment_on == "always" or has_blocking: upsert_pr_comment(markdown, settings) + if pip_audit_error: + sys.exit(f"pip-audit did NOT run, so dependencies were NOT audited.\n{pip_audit_error}") if has_blocking: sys.exit(1) diff --git a/src/python_security_auditing/annotations.py b/src/python_security_auditing/annotations.py index a6b0469..fb83df1 100644 --- a/src/python_security_auditing/annotations.py +++ b/src/python_security_auditing/annotations.py @@ -14,10 +14,16 @@ _SEVERITY_ORDER = {"HIGH": 0, "MEDIUM": 1, "LOW": 2} +def _escape(text: str) -> str: + """Escape workflow-command message data.""" + return text.replace("%", "%25").replace("\r", "%0D").replace("\n", "%0A") + + def emit_annotations( bandit_report: dict[str, Any], pip_audit_report: list[dict[str, Any]], settings: Settings, + pip_audit_error: str = "", ) -> None: """Print GitHub Actions workflow commands to stdout. @@ -37,15 +43,14 @@ def _sort_key(r: dict[str, Any]) -> int: fname = result.get("filename", "") line = result.get("line_number", 0) test_id = result.get("test_id", "") - text = ( - result.get("issue_text", "") - .replace("%", "%25") - .replace("\r", "%0D") - .replace("\n", "%0A") - ) + text = _escape(result.get("issue_text", "")) print(f"::{level} file={fname},line={line}::[{test_id}] {text}") if "pip-audit" in settings.enabled_tools: + if pip_audit_error: + print(f"::error::pip-audit did NOT run: {_escape(pip_audit_error)}") + elif not any("skip_reason" not in pkg for pkg in pip_audit_report): + print("::warning::pip-audit audited 0 dependencies, so nothing was checked") for pkg in pip_audit_report: if not pkg.get("vulns"): continue diff --git a/src/python_security_auditing/report.py b/src/python_security_auditing/report.py index 30ae842..b4e303a 100644 --- a/src/python_security_auditing/report.py +++ b/src/python_security_auditing/report.py @@ -13,6 +13,7 @@ def build_markdown( bandit_report: dict[str, Any], pip_audit_report: list[dict[str, Any]], settings: Settings, + pip_audit_error: str = "", ) -> str: """Build a full markdown security report.""" sections: list[str] = ["# Security Audit Report\n"] @@ -29,9 +30,9 @@ def build_markdown( sections.append(_bandit_section(bandit_report, settings)) if "pip-audit" in settings.enabled_tools: - sections.append(_pip_audit_section(pip_audit_report, settings)) + sections.append(_pip_audit_section(pip_audit_report, settings, pip_audit_error)) - blocking = check_thresholds(bandit_report, pip_audit_report, settings) + blocking = check_thresholds(bandit_report, pip_audit_report, settings) or bool(pip_audit_error) sections.append("---\n") if blocking: sections.append("**Result: ❌ Blocking issues found — see details above.**\n") @@ -103,7 +104,7 @@ def _bandit_section(report: dict[str, Any], settings: Settings) -> str: return "\n".join(lines) -def _pip_audit_section(report: list[dict[str, Any]], settings: Settings) -> str: +def _pip_audit_section(report: list[dict[str, Any]], settings: Settings, error: str) -> str: vulnerable = [pkg for pkg in report if pkg.get("vulns")] security_url = ( f"https://github.com/{settings.github_repository}/security/dependabot" @@ -117,6 +118,18 @@ def _pip_audit_section(report: list[dict[str, Any]], settings: Settings) -> str: ) lines = [heading] + if error: + lines.append( + f"❌ pip-audit did NOT run, so dependencies were NOT audited.\n\n```\n{error}\n```\n" + ) + return "\n".join(lines) + + audited = sum("skip_reason" not in pkg for pkg in report) + lines.append(f"_Dependencies audited: {audited}, skipped: {len(report) - audited}._\n") + if not audited: + lines.append("⚠️ No dependencies were audited.\n") + return "\n".join(lines) + if not vulnerable: lines.append("✅ No vulnerabilities found.\n") return "\n".join(lines) diff --git a/src/python_security_auditing/runners.py b/src/python_security_auditing/runners.py index 8accbe2..32976c6 100644 --- a/src/python_security_auditing/runners.py +++ b/src/python_security_auditing/runners.py @@ -3,6 +3,7 @@ from __future__ import annotations import json +import os import shutil import subprocess import sys @@ -12,6 +13,12 @@ from .settings import Settings +PIP_AUDIT_REPORT = Path("pip-audit-report.json") + + +class AuditError(Exception): + """The dependency list or the pip-audit report could not be produced.""" + def _resolve_exe(name: str) -> str: """Resolve an executable name to its full path via PATH, raising if not found.""" @@ -24,26 +31,46 @@ def _resolve_exe(name: str) -> str: def generate_requirements(settings: Settings) -> Path: """Return a requirements.txt Path suitable for pip-audit. - For package managers that don't produce a file directly (pip, pipenv), + For package managers that don't produce a file directly (pipenv), captures stdout into a temp file. For 'requirements', returns the - configured path unchanged. + configured path unchanged. Raises AuditError when no list can be produced. """ pm = settings.package_manager - if pm == "requirements": - return Path(settings.requirements_file) + if pm == "pip": + # `pip freeze` here would list this action's own environment, not the caller's. + raise AuditError( + "package_manager 'pip' is not supported: run `pip freeze > requirements.txt` " + "in your job and use `package_manager: requirements`." + ) + if pm == "requirements": + path = Path(settings.requirements_file) + if not path.is_file(): + raise AuditError(f"requirements file not found: {path}") + return path + + if pm == "pipenv": + # pipenv keeps the lock at .lock. Without it, `pipenv requirements` + # exits 0 and prints no packages. + lock = Path(os.environ.get("PIPENV_PIPFILE", "Pipfile") + ".lock") + if not lock.is_file(): + raise AuditError(f"{lock} not found: commit it or run `pipenv lock` first.") + + # Check everything that can fail before creating the temp file, so it is not leaked. + exe = _resolve_exe(pm) tmp = tempfile.NamedTemporaryFile(suffix="-requirements.txt", delete=False, mode="w") tmp.close() out_path = Path(tmp.name) if pm == "uv": cmd = [ - _resolve_exe("uv"), + exe, "export", "--format", "requirements-txt", "--no-hashes", + "--no-emit-project", "-o", str(out_path), ] @@ -57,27 +84,17 @@ def generate_requirements(settings: Settings) -> Path: text=True, ) except subprocess.CalledProcessError as exc: - print( - f"uv export failed (no lockfile?): {exc.stderr.strip()}", - file=sys.stderr, - ) - return out_path + out_path.unlink() + raise AuditError(f"uv export failed: {exc.stderr.strip()}") from exc if settings.debug: print( f"[debug] generated requirements ({out_path}):\n{out_path.read_text()}", file=sys.stderr, ) - elif pm == "pip": - result = subprocess.run( # nosec B603,B605 -- list args, full path via _resolve_exe() - [_resolve_exe("pip"), "freeze"], capture_output=True, text=True, check=True - ) - out_path.write_text(result.stdout) - if settings.debug: - print(f"[debug] pip freeze output ({out_path}):\n{result.stdout}", file=sys.stderr) elif pm == "poetry": # poetry-plugin-export is bundled in Poetry 1.8+; ignore failure here subprocess.run( # nosec B603,B605 -- list args, full path via _resolve_exe() - [_resolve_exe("poetry"), "self", "add", "poetry-plugin-export"], + [exe, "self", "add", "poetry-plugin-export"], check=False, capture_output=True, text=True, @@ -85,7 +102,7 @@ def generate_requirements(settings: Settings) -> Path: try: subprocess.run( # nosec B603,B605 -- list args, full path via _resolve_exe() [ - _resolve_exe("poetry"), + exe, "export", "--format", "requirements.txt", @@ -98,11 +115,8 @@ def generate_requirements(settings: Settings) -> Path: text=True, ) except subprocess.CalledProcessError as exc: - print( - f"poetry export failed (no lockfile?): {exc.stderr.strip()}", - file=sys.stderr, - ) - return out_path + out_path.unlink() + raise AuditError(f"poetry export failed: {exc.stderr.strip()}") from exc if settings.debug: print( f"[debug] poetry export output ({out_path}):\n{out_path.read_text()}", @@ -111,15 +125,12 @@ def generate_requirements(settings: Settings) -> Path: elif pm == "pipenv": try: result = subprocess.run( # nosec B603,B605 -- list args, full path via _resolve_exe() - [_resolve_exe("pipenv"), "requirements"], capture_output=True, text=True, check=True + [exe, "requirements"], capture_output=True, text=True, check=True ) out_path.write_text(result.stdout) except subprocess.CalledProcessError as exc: - print( - f"pipenv requirements failed (no lockfile?): {exc.stderr.strip()}", - file=sys.stderr, - ) - return out_path + out_path.unlink() + raise AuditError(f"pipenv requirements failed: {exc.stderr.strip()}") from exc if settings.debug: print( f"[debug] pipenv requirements output ({out_path}):\n{result.stdout}", @@ -174,20 +185,20 @@ def read_bandit_sarif(sarif_path: Path) -> dict[str, Any]: def run_pip_audit( requirements_path: Path, settings: Settings | None = None ) -> list[dict[str, Any]]: - """Run pip-audit, write pip-audit-report.json, return parsed report.""" - output_file = Path("pip-audit-report.json") + """Run pip-audit, write pip-audit-report.json, return parsed report. + + Raises AuditError when pip-audit does not produce a JSON report. + """ cmd = [_resolve_exe("pip-audit"), "-r", str(requirements_path), "--no-deps", "-f", "json"] + if settings and settings.package_manager != "requirements": + # Exports are fully pinned: audit them as written. Without --disable-pip, + # pip-audit resolves them with pip (building the project) and ignores --no-deps. + cmd.append("--disable-pip") if settings and settings.debug: print(f"[debug] pip-audit command: {cmd}", file=sys.stderr) result = subprocess.run(cmd, capture_output=True, text=True) # nosec B603,B605 -- list args, full path via _resolve_exe() - # pip-audit exits 1 when vulnerabilities are found — that is expected - if result.returncode not in (0, 1): - print( - f"pip-audit exited with unexpected code {result.returncode}:\n{result.stderr}", - file=sys.stderr, - ) if settings and settings.debug: print( @@ -196,12 +207,13 @@ def run_pip_audit( file=sys.stderr, ) - raw = result.stdout.strip() - if raw: - parsed: Any = json.loads(raw) - output_file.write_text(raw) - # pip-audit 2.7+ wraps output in {"dependencies": [...], "fixes": [...]} - if isinstance(parsed, dict): - return list(parsed.get("dependencies", [])) - return list(parsed) - return [] + # pip-audit exits 1 both when it finds vulnerabilities and when it fails, + # so only a JSON report proves that the audit ran. + try: + dependencies = list(json.loads(result.stdout)["dependencies"]) + except (ValueError, KeyError, TypeError) as exc: + raise AuditError( + f"pip-audit failed (exit {result.returncode}):\n{result.stderr.strip()}" + ) from exc + PIP_AUDIT_REPORT.write_text(result.stdout) + return dependencies diff --git a/tests/fixtures/pip_audit_clean.json b/tests/fixtures/pip_audit_clean.json index fe51488..adbf1f9 100644 --- a/tests/fixtures/pip_audit_clean.json +++ b/tests/fixtures/pip_audit_clean.json @@ -1 +1 @@ -[] +[{"name": "requests", "version": "2.32.0", "vulns": []}] diff --git a/tests/test_annotations.py b/tests/test_annotations.py index fbdce69..641c3d0 100644 --- a/tests/test_annotations.py +++ b/tests/test_annotations.py @@ -104,6 +104,25 @@ def test_pip_clean_emits_nothing( assert capsys.readouterr().out == "" +@pytest.mark.parametrize( + "report", + [[], [{"name": "my-app", "version": "0.1.0", "skip_reason": "Dependency not found on PyPI"}]], +) +def test_pip_zero_audited_emits_warning( + bandit_clean: dict[str, Any], report: list[Any], capsys: pytest.CaptureFixture[str] +) -> None: + emit_annotations(bandit_clean, report, Settings()) + assert capsys.readouterr().out.startswith("::warning::pip-audit audited 0 dependencies") + + +def test_pip_audit_error_emits_escaped_error( + bandit_clean: dict[str, Any], capsys: pytest.CaptureFixture[str] +) -> None: + emit_annotations(bandit_clean, [], Settings(), pip_audit_error="uv failed\n100% broken") + out = capsys.readouterr().out + assert out == "::error::pip-audit did NOT run: uv failed%0A100%25 broken\n" + + def test_bandit_only_tool_skips_pip( bandit_clean: dict[str, Any], pip_fixable: list[Any], diff --git a/tests/test_main.py b/tests/test_main.py index bbf6eab..c4919ee 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -12,13 +12,12 @@ FIXTURES = Path(__file__).parent / "fixtures" -def _make_sarif_mock(sarif_content: str, pip_stdout: str = "[]") -> object: - """Return a mock_subprocess factory that feeds a SARIF file and pip-audit output.""" +CLEAN_REPORT = '{"dependencies": [{"name": "requests", "version": "2.32.0", "vulns": []}]}' - def mock_subprocess(cmd: list[str], **kwargs: object) -> MagicMock: - return MagicMock(returncode=0, stderr="", stdout=pip_stdout) - return mock_subprocess +def _clean_audit(cmd: list[str], **kwargs: object) -> MagicMock: + """uv export succeeds and pip-audit reports no vulnerabilities.""" + return MagicMock(returncode=0, stderr="", stdout=CLEAN_REPORT) def test_comment_on_never_never_calls_upsert( @@ -33,16 +32,9 @@ def test_comment_on_never_never_calls_upsert( monkeypatch.setenv("GITHUB_TOKEN", "tok") monkeypatch.chdir(tmp_path) - uv_exc = subprocess.CalledProcessError(2, "uv", stderr="No uv.lock found") - - def mock_subprocess(cmd: list[str], **kwargs: object) -> MagicMock: - if cmd[0] == "uv": - raise uv_exc - return MagicMock(returncode=0, stderr="", stdout="[]") - with ( patch("python_security_auditing.runners.shutil.which", side_effect=lambda exe: exe), - patch("python_security_auditing.runners.subprocess.run", side_effect=mock_subprocess), + patch("python_security_auditing.runners.subprocess.run", side_effect=_clean_audit), patch("python_security_auditing.__main__.emit_annotations"), patch("python_security_auditing.__main__.upsert_pr_comment") as mock_comment, ): @@ -65,16 +57,9 @@ def test_comment_on_blocking_calls_upsert_when_blocking( monkeypatch.setenv("COMMENT_ON", "blocking") monkeypatch.chdir(tmp_path) - uv_exc = subprocess.CalledProcessError(2, "uv", stderr="No uv.lock found") - - def mock_subprocess(cmd: list[str], **kwargs: object) -> MagicMock: - if cmd[0] == "uv": - raise uv_exc - return MagicMock(returncode=0, stderr="", stdout="[]") - with ( patch("python_security_auditing.runners.shutil.which", side_effect=lambda exe: exe), - patch("python_security_auditing.runners.subprocess.run", side_effect=mock_subprocess), + patch("python_security_auditing.runners.subprocess.run", side_effect=_clean_audit), patch("python_security_auditing.__main__.emit_annotations"), patch("python_security_auditing.__main__.upsert_pr_comment") as mock_comment, ): @@ -96,16 +81,9 @@ def test_comment_on_blocking_skips_upsert_when_clean( monkeypatch.setenv("COMMENT_ON", "blocking") monkeypatch.chdir(tmp_path) - uv_exc = subprocess.CalledProcessError(2, "uv", stderr="No uv.lock found") - - def mock_subprocess(cmd: list[str], **kwargs: object) -> MagicMock: - if cmd[0] == "uv": - raise uv_exc - return MagicMock(returncode=0, stderr="", stdout="[]") - with ( patch("python_security_auditing.runners.shutil.which", side_effect=lambda exe: exe), - patch("python_security_auditing.runners.subprocess.run", side_effect=mock_subprocess), + patch("python_security_auditing.runners.subprocess.run", side_effect=_clean_audit), patch("python_security_auditing.__main__.emit_annotations"), patch("python_security_auditing.__main__.upsert_pr_comment") as mock_comment, ): @@ -113,54 +91,69 @@ def mock_subprocess(cmd: list[str], **kwargs: object) -> MagicMock: mock_comment.assert_not_called() -def test_main_succeeds_when_uv_lockfile_missing_and_no_bandit_issues( - monkeypatch: pytest.MonkeyPatch, tmp_path: Path +def test_main_fails_closed_when_uv_export_fails( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path, capsys: pytest.CaptureFixture[str] ) -> None: + """A failed export must fail the step and say so, not report 'No vulnerabilities found'.""" sarif_path = tmp_path / "results.sarif" sarif_path.write_text((FIXTURES / "bandit_clean.sarif").read_text()) + summary_path = tmp_path / "summary.md" monkeypatch.setenv("PACKAGE_MANAGER", "uv") monkeypatch.setenv("TOOLS", "bandit,pip-audit") monkeypatch.setenv("BANDIT_SARIF_PATH", str(sarif_path)) + monkeypatch.setenv("GITHUB_STEP_SUMMARY", str(summary_path)) monkeypatch.chdir(tmp_path) + # a report left by an earlier run must not be uploaded as this run's result + (tmp_path / "pip-audit-report.json").write_text('{"dependencies": [], "fixes": []}') - uv_exc = subprocess.CalledProcessError(2, "uv", stderr="No uv.lock found") - - def mock_subprocess(cmd: list[str], **kwargs: object) -> MagicMock: - if cmd[0] == "uv": - raise uv_exc - return MagicMock(returncode=0, stderr="", stdout="[]") - + uv_exc = subprocess.CalledProcessError(2, "uv", stderr="No uv.lock found\nsecond line") with ( patch("python_security_auditing.runners.shutil.which", side_effect=lambda exe: exe), - patch("python_security_auditing.runners.subprocess.run", side_effect=mock_subprocess), - patch("python_security_auditing.__main__.emit_annotations"), + patch("python_security_auditing.runners.subprocess.run", side_effect=uv_exc), ): - main() # should return normally without calling sys.exit(1) + with pytest.raises(SystemExit) as exc_info: + main() + message = str(exc_info.value.code) # a str code exits with status 1 + assert "pip-audit did NOT run" in message + assert "uv export failed: No uv.lock found" in message + summary = summary_path.read_text() + assert "pip-audit did NOT run" in summary + assert "No vulnerabilities found" not in summary + out = capsys.readouterr().out + assert "::error::pip-audit did NOT run: uv export failed: No uv.lock found%0Asecond line" in out + assert not (tmp_path / "pip-audit-report.json").exists() -def test_main_fails_when_bandit_blocks_despite_missing_lockfile( - monkeypatch: pytest.MonkeyPatch, tmp_path: Path + +def test_main_reports_bandit_and_comments_when_pip_audit_cannot_run( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path, capsys: pytest.CaptureFixture[str] ) -> None: + """A failed audit must not hide bandit results or leave a stale PR comment.""" sarif_path = tmp_path / "results.sarif" sarif_path.write_text((FIXTURES / "bandit_issues.sarif").read_text()) + summary_path = tmp_path / "summary.md" monkeypatch.setenv("PACKAGE_MANAGER", "uv") monkeypatch.setenv("TOOLS", "bandit,pip-audit") monkeypatch.setenv("BANDIT_SARIF_PATH", str(sarif_path)) monkeypatch.setenv("BANDIT_SEVERITY_THRESHOLD", "high") + monkeypatch.setenv("GITHUB_STEP_SUMMARY", str(summary_path)) + monkeypatch.setenv("GITHUB_TOKEN", "tok") + monkeypatch.setenv("COMMENT_ON", "blocking") monkeypatch.chdir(tmp_path) uv_exc = subprocess.CalledProcessError(2, "uv", stderr="No uv.lock found") - - def mock_subprocess(cmd: list[str], **kwargs: object) -> MagicMock: - if cmd[0] == "uv": - raise uv_exc - return MagicMock(returncode=0, stderr="", stdout="[]") - with ( patch("python_security_auditing.runners.shutil.which", side_effect=lambda exe: exe), - patch("python_security_auditing.runners.subprocess.run", side_effect=mock_subprocess), - patch("python_security_auditing.__main__.emit_annotations"), + patch("python_security_auditing.runners.subprocess.run", side_effect=uv_exc), + patch("python_security_auditing.__main__.upsert_pr_comment") as mock_comment, ): with pytest.raises(SystemExit) as exc_info: main() - assert exc_info.value.code == 1 + + assert exc_info.value.code not in (None, 0) + summary = summary_path.read_text() + assert "B404" in summary # bandit section still rendered + assert "pip-audit did NOT run" in summary + assert "::error file=src/app.py,line=2::[B404]" in capsys.readouterr().out + mock_comment.assert_called_once() + assert "pip-audit did NOT run" in mock_comment.call_args[0][0] diff --git a/tests/test_report.py b/tests/test_report.py index 90ba923..42ac3f2 100644 --- a/tests/test_report.py +++ b/tests/test_report.py @@ -189,6 +189,36 @@ def test_markdown_pip_table(bandit_clean: dict[str, Any], pip_fixable: list[Any] assert "GHSA-j8r2-6x86-q33q" in md +def test_markdown_pip_counts_audited_and_skipped(bandit_clean: dict[str, Any]) -> None: + report: list[dict[str, Any]] = [ + {"name": "requests", "version": "2.32.0", "vulns": []}, + {"name": "my-app", "version": "0.1.0", "skip_reason": "Dependency not found on PyPI"}, + ] + md = build_markdown(bandit_clean, report, Settings()) + assert "Dependencies audited: 1, skipped: 1" in md + assert "No vulnerabilities found" in md + + +@pytest.mark.parametrize( + "report", + [[], [{"name": "my-app", "version": "0.1.0", "skip_reason": "Dependency not found on PyPI"}]], +) +def test_markdown_zero_audited_is_not_reported_clean( + bandit_clean: dict[str, Any], report: list[Any] +) -> None: + md = build_markdown(bandit_clean, report, Settings()) + assert "No vulnerabilities found" not in md + assert "No dependencies were audited" in md + + +def test_markdown_pip_audit_error(bandit_issues: dict[str, Any]) -> None: + md = build_markdown(bandit_issues, [], Settings(), pip_audit_error="uv export failed: boom") + assert "B404" in md # bandit results are still shown + assert "pip-audit did NOT run" in md + assert "uv export failed: boom" in md + assert "No vulnerabilities found" not in md + + def test_markdown_run_url( bandit_clean: dict[str, Any], pip_clean: list[Any], monkeypatch: pytest.MonkeyPatch ) -> None: diff --git a/tests/test_runners.py b/tests/test_runners.py index 4c0e22d..6002a76 100644 --- a/tests/test_runners.py +++ b/tests/test_runners.py @@ -4,14 +4,21 @@ import json import subprocess +import tempfile from pathlib import Path from unittest.mock import MagicMock, patch import pytest -from python_security_auditing.runners import generate_requirements, read_bandit_sarif, run_pip_audit +from python_security_auditing.runners import ( + AuditError, + generate_requirements, + read_bandit_sarif, + run_pip_audit, +) from python_security_auditing.settings import Settings FIXTURES = Path(__file__).parent / "fixtures" +CLEAN_REPORT = json.dumps({"dependencies": [], "fixes": []}) # --------------------------------------------------------------------------- @@ -19,13 +26,26 @@ # --------------------------------------------------------------------------- -def test_requirements_mode_returns_configured_path(monkeypatch: pytest.MonkeyPatch) -> None: +def test_requirements_mode_returns_configured_path( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + monkeypatch.chdir(tmp_path) + (tmp_path / "custom-requirements.txt").write_text("requests==2.31.0\n") monkeypatch.setenv("PACKAGE_MANAGER", "requirements") monkeypatch.setenv("REQUIREMENTS_FILE", "custom-requirements.txt") s = Settings() assert generate_requirements(s) == Path("custom-requirements.txt") +def test_requirements_mode_raises_when_file_missing( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + monkeypatch.chdir(tmp_path) + monkeypatch.setenv("PACKAGE_MANAGER", "requirements") + with pytest.raises(AuditError, match="requirements file not found: requirements.txt"): + generate_requirements(Settings()) + + def test_uv_mode_calls_uv_export(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: monkeypatch.setenv("PACKAGE_MANAGER", "uv") s = Settings() @@ -42,24 +62,18 @@ def test_uv_mode_calls_uv_export(monkeypatch: pytest.MonkeyPatch, tmp_path: Path assert "export" in cmd assert "--format" in cmd assert "requirements-txt" in cmd + # the caller's own project is not on PyPI: emitting it makes pip-audit build it + assert "--no-emit-project" in cmd assert str(result).endswith("-requirements.txt") -def test_pip_mode_calls_pip_freeze(monkeypatch: pytest.MonkeyPatch) -> None: +def test_pip_mode_is_rejected(monkeypatch: pytest.MonkeyPatch) -> None: + """`pip freeze` would list the action's own environment, so pip mode must fail.""" monkeypatch.setenv("PACKAGE_MANAGER", "pip") - s = Settings() - - freeze_output = "requests==2.31.0\n" - with ( - patch("python_security_auditing.runners.shutil.which", side_effect=lambda exe: exe), - patch("python_security_auditing.runners.subprocess.run") as mock_run, - ): - mock_run.return_value = MagicMock(returncode=0, stdout=freeze_output) - result = generate_requirements(s) - - cmd = mock_run.call_args[0][0] - assert cmd == ["pip", "freeze"] - assert result.read_text() == freeze_output + with patch("python_security_auditing.runners.subprocess.run") as mock_run: + with pytest.raises(AuditError, match=r"pip freeze > requirements\.txt"): + generate_requirements(Settings()) + mock_run.assert_not_called() def test_poetry_mode_calls_poetry_export(monkeypatch: pytest.MonkeyPatch) -> None: @@ -82,7 +96,11 @@ def test_poetry_mode_calls_poetry_export(monkeypatch: pytest.MonkeyPatch) -> Non assert str(result).endswith("-requirements.txt") -def test_pipenv_mode_calls_pipenv_requirements(monkeypatch: pytest.MonkeyPatch) -> None: +def test_pipenv_mode_calls_pipenv_requirements( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + monkeypatch.chdir(tmp_path) + (tmp_path / "Pipfile.lock").write_text("{}") monkeypatch.setenv("PACKAGE_MANAGER", "pipenv") s = Settings() @@ -181,19 +199,27 @@ def test_run_pip_audit_parses_json(tmp_path: Path, monkeypatch: pytest.MonkeyPat assert (tmp_path / "pip-audit-report.json").exists() -def test_run_pip_audit_returns_empty_on_no_output( - tmp_path: Path, monkeypatch: pytest.MonkeyPatch +@pytest.mark.parametrize( + ("returncode", "stdout"), + [(1, ""), (0, ""), (1, "Traceback (most recent call last):"), (0, "[]"), (2, "{}")], +) +def test_run_pip_audit_raises_without_json_report( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, returncode: int, stdout: str ) -> None: + """pip-audit exits 1 for findings AND for fatal errors: only a JSON report counts.""" monkeypatch.chdir(tmp_path) with ( patch("python_security_auditing.runners.shutil.which", side_effect=lambda exe: exe), patch("python_security_auditing.runners.subprocess.run") as mock_run, ): - mock_run.return_value = MagicMock(returncode=0, stderr="", stdout="") - report = run_pip_audit(Path("requirements.txt")) + mock_run.return_value = MagicMock( + returncode=returncode, stderr="ERROR:pip_audit._cli:boom", stdout=stdout + ) + with pytest.raises(AuditError, match=rf"exit {returncode}\):\nERROR:pip_audit\._cli:boom"): + run_pip_audit(Path("requirements.txt")) - assert report == [] + assert not (tmp_path / "pip-audit-report.json").exists() def test_run_pip_audit_uses_requirements_path( @@ -206,7 +232,7 @@ def test_run_pip_audit_uses_requirements_path( patch("python_security_auditing.runners.shutil.which", side_effect=lambda exe: exe), patch("python_security_auditing.runners.subprocess.run") as mock_run, ): - mock_run.return_value = MagicMock(returncode=0, stderr="", stdout="[]") + mock_run.return_value = MagicMock(returncode=0, stderr="", stdout=CLEAN_REPORT) run_pip_audit(req_path) cmd = mock_run.call_args[0][0] @@ -215,85 +241,82 @@ def test_run_pip_audit_uses_requirements_path( assert "json" in cmd -def test_run_pip_audit_command_includes_no_deps( - tmp_path: Path, monkeypatch: pytest.MonkeyPatch +@pytest.mark.parametrize( + ("package_manager", "disable_pip"), + [("uv", True), ("poetry", True), ("pipenv", True), ("requirements", False)], +) +def test_run_pip_audit_disables_pip_for_pinned_exports( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, package_manager: str, disable_pip: bool ) -> None: + """Exports are fully pinned; a requirements file may not be, so pip resolves it.""" monkeypatch.chdir(tmp_path) + monkeypatch.setenv("PACKAGE_MANAGER", package_manager) with ( patch("python_security_auditing.runners.shutil.which", side_effect=lambda exe: exe), patch("python_security_auditing.runners.subprocess.run") as mock_run, ): - mock_run.return_value = MagicMock(returncode=0, stderr="", stdout="[]") - run_pip_audit(Path("requirements.txt")) + mock_run.return_value = MagicMock(returncode=0, stderr="", stdout=CLEAN_REPORT) + run_pip_audit(Path("requirements.txt"), Settings()) cmd = mock_run.call_args[0][0] assert "--no-deps" in cmd + assert ("--disable-pip" in cmd) is disable_pip # --------------------------------------------------------------------------- -# generate_requirements — missing lockfile handling +# generate_requirements — export failures must fail, not return an empty file # --------------------------------------------------------------------------- -def test_generate_requirements_uv_returns_empty_on_missing_lockfile( - monkeypatch: pytest.MonkeyPatch, +@pytest.mark.parametrize("package_manager", ["uv", "poetry", "pipenv"]) +def test_generate_requirements_raises_when_export_fails( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path, package_manager: str ) -> None: - monkeypatch.setenv("PACKAGE_MANAGER", "uv") - s = Settings() - exc = subprocess.CalledProcessError(2, "uv", stderr="No uv.lock found") + monkeypatch.chdir(tmp_path) + (tmp_path / "Pipfile.lock").write_text("{}") + monkeypatch.setenv("PACKAGE_MANAGER", package_manager) + tmpdir = tmp_path / "tmp" + tmpdir.mkdir() + monkeypatch.setattr(tempfile, "tempdir", str(tmpdir)) + exc = subprocess.CalledProcessError(2, package_manager, stderr="lockfile is broken") + # poetry first runs `poetry self add`, which may fail without consequence + side_effect = [MagicMock(returncode=1), exc] if package_manager == "poetry" else exc with ( patch("python_security_auditing.runners.shutil.which", side_effect=lambda exe: exe), - patch("python_security_auditing.runners.subprocess.run", side_effect=exc), + patch("python_security_auditing.runners.subprocess.run", side_effect=side_effect), ): - result = generate_requirements(s) - assert result.exists() - assert result.stat().st_size == 0 + with pytest.raises(AuditError, match=f"{package_manager} .*failed: lockfile is broken"): + generate_requirements(Settings()) + assert list(tmpdir.iterdir()) == [] # no leaked temp requirements file -def test_generate_requirements_poetry_returns_empty_on_missing_lockfile( - monkeypatch: pytest.MonkeyPatch, -) -> None: - monkeypatch.setenv("PACKAGE_MANAGER", "poetry") - s = Settings() - export_exc = subprocess.CalledProcessError(1, "poetry", stderr="poetry.lock not found") - with ( - patch("python_security_auditing.runners.shutil.which", side_effect=lambda exe: exe), - patch("python_security_auditing.runners.subprocess.run") as mock_run, - ): - # self add uses check=False so a non-zero return is silently ignored; - # export raises CalledProcessError to simulate a missing lockfile - mock_run.side_effect = [MagicMock(returncode=1), export_exc] - result = generate_requirements(s) - assert result.exists() - assert result.stat().st_size == 0 - - -def test_generate_requirements_pipenv_returns_empty_on_missing_lockfile( - monkeypatch: pytest.MonkeyPatch, +def test_generate_requirements_pipenv_raises_without_lockfile( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path ) -> None: + """`pipenv requirements` exits 0 with no packages when Pipfile.lock is missing.""" + monkeypatch.chdir(tmp_path) monkeypatch.setenv("PACKAGE_MANAGER", "pipenv") - s = Settings() - exc = subprocess.CalledProcessError(1, "pipenv", stderr="Pipfile.lock not found") - with ( - patch("python_security_auditing.runners.shutil.which", side_effect=lambda exe: exe), - patch("python_security_auditing.runners.subprocess.run", side_effect=exc), - ): - result = generate_requirements(s) - assert result.exists() - assert result.stat().st_size == 0 + with patch("python_security_auditing.runners.subprocess.run") as mock_run: + with pytest.raises(AuditError, match="Pipfile.lock not found"): + generate_requirements(Settings()) + mock_run.assert_not_called() -def test_generate_requirements_uv_warns_on_missing_lockfile( - monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +def test_generate_requirements_pipenv_uses_lockfile_next_to_pipenv_pipfile( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path ) -> None: - monkeypatch.setenv("PACKAGE_MANAGER", "uv") - s = Settings() - exc = subprocess.CalledProcessError(2, "uv", stderr="No uv.lock found") + """pipenv keeps the lock at .lock, not in the current directory.""" + monkeypatch.chdir(tmp_path) + (tmp_path / "sub").mkdir() + (tmp_path / "sub" / "Pipfile.lock").write_text("{}") + monkeypatch.setenv("PIPENV_PIPFILE", str(tmp_path / "sub" / "Pipfile")) + monkeypatch.setenv("PACKAGE_MANAGER", "pipenv") with ( patch("python_security_auditing.runners.shutil.which", side_effect=lambda exe: exe), - patch("python_security_auditing.runners.subprocess.run", side_effect=exc), + patch("python_security_auditing.runners.subprocess.run") as mock_run, ): - generate_requirements(s) - assert "uv export failed" in capsys.readouterr().err + mock_run.return_value = MagicMock(returncode=0, stdout="requests==2.31.0\n") + result = generate_requirements(Settings()) + assert result.read_text() == "requests==2.31.0\n" # ---------------------------------------------------------------------------