From 083655240eb904f3952e13d40dc7d143e482802f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Houpert?= <10154151+lhoupert@users.noreply.github.com> Date: Sat, 28 Mar 2026 23:43:58 +0000 Subject: [PATCH 1/6] chore: add additional test pip-audit --- src/python_security_auditing/runners.py | 83 ++++++++++++++++--------- tests/test_main.py | 59 ++++++++++++++++++ tests/test_runners.py | 64 +++++++++++++++++++ uv.lock | 2 +- 4 files changed, 176 insertions(+), 32 deletions(-) create mode 100644 tests/test_main.py diff --git a/src/python_security_auditing/runners.py b/src/python_security_auditing/runners.py index e1eed65..b628599 100644 --- a/src/python_security_auditing/runners.py +++ b/src/python_security_auditing/runners.py @@ -32,12 +32,19 @@ def generate_requirements(settings: Settings) -> Path: cmd = ["uv", "export", "--format", "requirements-txt", "--no-hashes", "-o", str(out_path)] if settings.debug: print(f"[debug] uv export command: {cmd}", file=sys.stderr) - subprocess.run( - cmd, - check=True, - capture_output=True, - text=True, - ) + try: + subprocess.run( + cmd, + check=True, + capture_output=True, + text=True, + ) + except subprocess.CalledProcessError as exc: + print( + f"uv export failed (no lockfile?): {exc.stderr.strip()}", + file=sys.stderr, + ) + return out_path if settings.debug: print( f"[debug] generated requirements ({out_path}):\n{out_path.read_text()}", @@ -49,36 +56,50 @@ def generate_requirements(settings: Settings) -> Path: if settings.debug: print(f"[debug] pip freeze output ({out_path}):\n{result.stdout}", file=sys.stderr) elif pm == "poetry": - subprocess.run( - ["poetry", "self", "add", "poetry-plugin-export"], - check=True, - capture_output=True, - text=True, - ) - subprocess.run( - [ - "poetry", - "export", - "--format", - "requirements.txt", - "--without-hashes", - "-o", - str(out_path), - ], - check=True, - capture_output=True, - text=True, - ) + try: + subprocess.run( + ["poetry", "self", "add", "poetry-plugin-export"], + check=True, + capture_output=True, + text=True, + ) + subprocess.run( + [ + "poetry", + "export", + "--format", + "requirements.txt", + "--without-hashes", + "-o", + str(out_path), + ], + check=True, + capture_output=True, + text=True, + ) + except subprocess.CalledProcessError as exc: + print( + f"poetry export failed (no lockfile?): {exc.stderr.strip()}", + file=sys.stderr, + ) + return out_path if settings.debug: print( f"[debug] poetry export output ({out_path}):\n{out_path.read_text()}", file=sys.stderr, ) elif pm == "pipenv": - result = subprocess.run( - ["pipenv", "requirements"], capture_output=True, text=True, check=True - ) - out_path.write_text(result.stdout) + try: + result = subprocess.run( + ["pipenv", "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 if settings.debug: print( f"[debug] pipenv requirements output ({out_path}):\n{result.stdout}", @@ -135,7 +156,7 @@ def run_pip_audit( ) -> list[dict[str, Any]]: """Run pip-audit, write pip-audit-report.json, return parsed report.""" output_file = Path("pip-audit-report.json") - cmd = ["pip-audit", "-r", str(requirements_path), "-f", "json"] + cmd = ["pip-audit", "-r", str(requirements_path), "--no-deps", "-f", "json"] if settings and settings.debug: print(f"[debug] pip-audit command: {cmd}", file=sys.stderr) diff --git a/tests/test_main.py b/tests/test_main.py new file mode 100644 index 0000000..3e9b320 --- /dev/null +++ b/tests/test_main.py @@ -0,0 +1,59 @@ +"""Tests for __main__.py orchestrator.""" + +from __future__ import annotations + +import subprocess +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest +from python_security_auditing.__main__ import main + +FIXTURES = Path(__file__).parent / "fixtures" + + +def test_main_succeeds_when_uv_lockfile_missing_and_no_bandit_issues( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + sarif_path = tmp_path / "results.sarif" + sarif_path.write_text((FIXTURES / "bandit_clean.sarif").read_text()) + monkeypatch.setenv("PACKAGE_MANAGER", "uv") + monkeypatch.setenv("TOOLS", "bandit,pip-audit") + monkeypatch.setenv("BANDIT_SARIF_PATH", str(sarif_path)) + monkeypatch.setenv("POST_PR_COMMENT", "false") + 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.subprocess.run", side_effect=mock_subprocess): + main() # should return normally without calling sys.exit(1) + + +def test_main_fails_when_bandit_blocks_despite_missing_lockfile( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + sarif_path = tmp_path / "results.sarif" + sarif_path.write_text((FIXTURES / "bandit_issues.sarif").read_text()) + 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("POST_PR_COMMENT", "false") + 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.subprocess.run", side_effect=mock_subprocess): + with pytest.raises(SystemExit) as exc_info: + main() + assert exc_info.value.code == 1 diff --git a/tests/test_runners.py b/tests/test_runners.py index 5b18c98..e75cb9c 100644 --- a/tests/test_runners.py +++ b/tests/test_runners.py @@ -3,6 +3,7 @@ from __future__ import annotations import json +import subprocess from pathlib import Path from unittest.mock import MagicMock, patch @@ -191,3 +192,66 @@ def test_run_pip_audit_uses_requirements_path( assert str(req_path) in cmd assert "-f" in cmd assert "json" in cmd + + +def test_run_pip_audit_command_includes_no_deps( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.chdir(tmp_path) + with 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")) + cmd = mock_run.call_args[0][0] + assert "--no-deps" in cmd + + +# --------------------------------------------------------------------------- +# generate_requirements — missing lockfile handling +# --------------------------------------------------------------------------- + + +def test_generate_requirements_uv_returns_empty_on_missing_lockfile( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setenv("PACKAGE_MANAGER", "uv") + s = Settings() + exc = subprocess.CalledProcessError(2, "uv", stderr="No uv.lock found") + with patch("python_security_auditing.runners.subprocess.run", side_effect=exc): + result = generate_requirements(s) + assert result.exists() + assert result.stat().st_size == 0 + + +def test_generate_requirements_poetry_returns_empty_on_missing_lockfile( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setenv("PACKAGE_MANAGER", "poetry") + s = Settings() + exc = subprocess.CalledProcessError(1, "poetry", stderr="poetry.lock not found") + with patch("python_security_auditing.runners.subprocess.run", side_effect=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, +) -> None: + monkeypatch.setenv("PACKAGE_MANAGER", "pipenv") + s = Settings() + exc = subprocess.CalledProcessError(1, "pipenv", stderr="Pipfile.lock not found") + with patch("python_security_auditing.runners.subprocess.run", side_effect=exc): + result = generate_requirements(s) + assert result.exists() + assert result.stat().st_size == 0 + + +def test_generate_requirements_uv_warns_on_missing_lockfile( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + monkeypatch.setenv("PACKAGE_MANAGER", "uv") + s = Settings() + exc = subprocess.CalledProcessError(2, "uv", stderr="No uv.lock found") + with patch("python_security_auditing.runners.subprocess.run", side_effect=exc): + generate_requirements(s) + assert "uv export failed" in capsys.readouterr().err diff --git a/uv.lock b/uv.lock index 849f544..f14620d 100644 --- a/uv.lock +++ b/uv.lock @@ -566,7 +566,7 @@ wheels = [ [[package]] name = "python-security-auditing" -version = "0.4.2" +version = "0.4.3" source = { editable = "." } dependencies = [ { name = "pip-audit" }, From 1791bce99627a60fd1a0985c0e8a6169ffc19005 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Houpert?= <10154151+lhoupert@users.noreply.github.com> Date: Sun, 29 Mar 2026 00:11:00 +0000 Subject: [PATCH 2/6] fix: make poetry self add non-fatal (plugin bundled in Poetry 1.8+) --- src/python_security_auditing/runners.py | 13 +++++++------ tests/test_runners.py | 7 +++++-- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/python_security_auditing/runners.py b/src/python_security_auditing/runners.py index b628599..a2ae85f 100644 --- a/src/python_security_auditing/runners.py +++ b/src/python_security_auditing/runners.py @@ -56,13 +56,14 @@ def generate_requirements(settings: Settings) -> Path: 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( + ["poetry", "self", "add", "poetry-plugin-export"], + check=False, + capture_output=True, + text=True, + ) try: - subprocess.run( - ["poetry", "self", "add", "poetry-plugin-export"], - check=True, - capture_output=True, - text=True, - ) subprocess.run( [ "poetry", diff --git a/tests/test_runners.py b/tests/test_runners.py index e75cb9c..066bc4e 100644 --- a/tests/test_runners.py +++ b/tests/test_runners.py @@ -227,8 +227,11 @@ def test_generate_requirements_poetry_returns_empty_on_missing_lockfile( ) -> None: monkeypatch.setenv("PACKAGE_MANAGER", "poetry") s = Settings() - exc = subprocess.CalledProcessError(1, "poetry", stderr="poetry.lock not found") - with patch("python_security_auditing.runners.subprocess.run", side_effect=exc): + export_exc = subprocess.CalledProcessError(1, "poetry", stderr="poetry.lock not found") + with 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 From 649f973adf758245f56f522d692fa98a45387090 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Houpert?= <10154151+lhoupert@users.noreply.github.com> Date: Sun, 29 Mar 2026 23:48:18 +0100 Subject: [PATCH 3/6] fix: secure executable and files paths --- src/python_security_auditing/__main__.py | 2 +- src/python_security_auditing/pr_comment.py | 17 +++-- src/python_security_auditing/runners.py | 31 ++++++-- src/python_security_auditing/settings.py | 24 +++++++ tests/test_main.py | 10 ++- tests/test_pr_comment.py | 44 ++++++++---- tests/test_runners.py | 84 ++++++++++++++++++---- tests/test_settings.py | 74 +++++++++++++++++++ 8 files changed, 249 insertions(+), 37 deletions(-) diff --git a/src/python_security_auditing/__main__.py b/src/python_security_auditing/__main__.py index c4a4e8f..1910a3b 100644 --- a/src/python_security_auditing/__main__.py +++ b/src/python_security_auditing/__main__.py @@ -16,7 +16,7 @@ def main() -> None: settings = Settings() if settings.debug: - print(f"[debug] settings: {settings.model_dump()}", file=sys.stderr) + print(f"[debug] settings: {settings.model_dump(exclude={'github_token'})}", file=sys.stderr) bandit_report: dict[str, Any] = {} pip_audit_report: list[dict[str, Any]] = [] diff --git a/src/python_security_auditing/pr_comment.py b/src/python_security_auditing/pr_comment.py index f4077ff..b8a4d4c 100644 --- a/src/python_security_auditing/pr_comment.py +++ b/src/python_security_auditing/pr_comment.py @@ -3,12 +3,21 @@ from __future__ import annotations import json +import shutil import subprocess import sys from .settings import Settings +def _resolve_exe(name: str) -> str: + """Resolve an executable name to its full path via PATH, raising if not found.""" + resolved = shutil.which(name) + if resolved is None: + raise FileNotFoundError(f"Required tool not found on PATH: {name!r}") + return resolved + + def comment_marker(workflow: str) -> str: if workflow: return f"" @@ -25,7 +34,7 @@ def resolve_pr_number(settings: Settings) -> int | None: result = subprocess.run( [ - "gh", + _resolve_exe("gh"), "pr", "list", "--head", @@ -65,7 +74,7 @@ def upsert_pr_comment(markdown: str, settings: Settings) -> None: # Find an existing comment with our marker existing_id: int | None = None list_result = subprocess.run( - ["gh", "api", f"repos/{repo}/issues/{pr_number}/comments"], + [_resolve_exe("gh"), "api", f"repos/{repo}/issues/{pr_number}/comments"], capture_output=True, text=True, ) @@ -78,7 +87,7 @@ def upsert_pr_comment(markdown: str, settings: Settings) -> None: if existing_id is not None: subprocess.run( [ - "gh", + _resolve_exe("gh"), "api", "--method", "PATCH", @@ -90,6 +99,6 @@ def upsert_pr_comment(markdown: str, settings: Settings) -> None: ) else: subprocess.run( - ["gh", "pr", "comment", str(pr_number), "--body", body, "--repo", repo], + [_resolve_exe("gh"), "pr", "comment", str(pr_number), "--body", body, "--repo", repo], check=True, ) diff --git a/src/python_security_auditing/runners.py b/src/python_security_auditing/runners.py index a2ae85f..bedae19 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 shutil import subprocess import sys import tempfile @@ -12,6 +13,14 @@ from .settings import Settings +def _resolve_exe(name: str) -> str: + """Resolve an executable name to its full path via PATH, raising if not found.""" + resolved = shutil.which(name) + if resolved is None: + raise FileNotFoundError(f"Required tool not found on PATH: {name!r}") + return resolved + + def generate_requirements(settings: Settings) -> Path: """Return a requirements.txt Path suitable for pip-audit. @@ -29,7 +38,15 @@ def generate_requirements(settings: Settings) -> Path: out_path = Path(tmp.name) if pm == "uv": - cmd = ["uv", "export", "--format", "requirements-txt", "--no-hashes", "-o", str(out_path)] + cmd = [ + _resolve_exe("uv"), + "export", + "--format", + "requirements-txt", + "--no-hashes", + "-o", + str(out_path), + ] if settings.debug: print(f"[debug] uv export command: {cmd}", file=sys.stderr) try: @@ -51,14 +68,16 @@ def generate_requirements(settings: Settings) -> Path: file=sys.stderr, ) elif pm == "pip": - result = subprocess.run(["pip", "freeze"], capture_output=True, text=True, check=True) + result = subprocess.run( + [_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( - ["poetry", "self", "add", "poetry-plugin-export"], + [_resolve_exe("poetry"), "self", "add", "poetry-plugin-export"], check=False, capture_output=True, text=True, @@ -66,7 +85,7 @@ def generate_requirements(settings: Settings) -> Path: try: subprocess.run( [ - "poetry", + _resolve_exe("poetry"), "export", "--format", "requirements.txt", @@ -92,7 +111,7 @@ def generate_requirements(settings: Settings) -> Path: elif pm == "pipenv": try: result = subprocess.run( - ["pipenv", "requirements"], capture_output=True, text=True, check=True + [_resolve_exe("pipenv"), "requirements"], capture_output=True, text=True, check=True ) out_path.write_text(result.stdout) except subprocess.CalledProcessError as exc: @@ -157,7 +176,7 @@ def run_pip_audit( ) -> list[dict[str, Any]]: """Run pip-audit, write pip-audit-report.json, return parsed report.""" output_file = Path("pip-audit-report.json") - cmd = ["pip-audit", "-r", str(requirements_path), "--no-deps", "-f", "json"] + cmd = [_resolve_exe("pip-audit"), "-r", str(requirements_path), "--no-deps", "-f", "json"] if settings and settings.debug: print(f"[debug] pip-audit command: {cmd}", file=sys.stderr) diff --git a/src/python_security_auditing/settings.py b/src/python_security_auditing/settings.py index 2d8c254..9726fd9 100644 --- a/src/python_security_auditing/settings.py +++ b/src/python_security_auditing/settings.py @@ -2,6 +2,7 @@ from __future__ import annotations +from pathlib import Path from typing import Literal from pydantic import field_validator @@ -61,6 +62,29 @@ def _empty_str_to_none(cls, v: object) -> object: return None return v + @field_validator("bandit_sarif_path", "requirements_file", "github_step_summary", mode="after") + @classmethod + def _no_path_traversal(cls, v: str) -> str: + if v and ".." in Path(v).parts: + raise ValueError(f"Path traversal not allowed: {v!r}") + return v + + @field_validator("github_repository", mode="after") + @classmethod + def _validate_repository_format(cls, v: str) -> str: + if not v: + return v + if ".." in v or v.startswith("/") or v.count("/") != 1: + raise ValueError(f"github_repository must be 'owner/repo' format, got: {v!r}") + return v + + @field_validator("github_run_id", mode="after") + @classmethod + def _validate_run_id(cls, v: str) -> str: + if v and not v.isdigit(): + raise ValueError(f"github_run_id must be numeric, got: {v!r}") + return v + github_head_ref: str = "" # Branch name for PRs github_workflow: str = "" # Name of the running workflow github_step_summary: str = "" # Path to step summary file diff --git a/tests/test_main.py b/tests/test_main.py index 3e9b320..d365574 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -30,7 +30,10 @@ def mock_subprocess(cmd: list[str], **kwargs: object) -> MagicMock: raise uv_exc return MagicMock(returncode=0, stderr="", stdout="[]") - with patch("python_security_auditing.runners.subprocess.run", side_effect=mock_subprocess): + with ( + patch("python_security_auditing.runners.shutil.which", side_effect=lambda exe: exe), + patch("python_security_auditing.runners.subprocess.run", side_effect=mock_subprocess), + ): main() # should return normally without calling sys.exit(1) @@ -53,7 +56,10 @@ def mock_subprocess(cmd: list[str], **kwargs: object) -> MagicMock: raise uv_exc return MagicMock(returncode=0, stderr="", stdout="[]") - with patch("python_security_auditing.runners.subprocess.run", side_effect=mock_subprocess): + with ( + patch("python_security_auditing.runners.shutil.which", side_effect=lambda exe: exe), + patch("python_security_auditing.runners.subprocess.run", side_effect=mock_subprocess), + ): with pytest.raises(SystemExit) as exc_info: main() assert exc_info.value.code == 1 diff --git a/tests/test_pr_comment.py b/tests/test_pr_comment.py index 20f494b..5b36edb 100644 --- a/tests/test_pr_comment.py +++ b/tests/test_pr_comment.py @@ -64,10 +64,13 @@ def test_upsert_creates_new_comment(monkeypatch: pytest.MonkeyPatch) -> None: s = Settings() list_response = MagicMock(returncode=0, stdout=json.dumps([])) - with patch( - "python_security_auditing.pr_comment.subprocess.run", - side_effect=[list_response, MagicMock()], - ) as mock_run: + with ( + patch("python_security_auditing.pr_comment.shutil.which", side_effect=lambda exe: exe), + patch( + "python_security_auditing.pr_comment.subprocess.run", + side_effect=[list_response, MagicMock()], + ) as mock_run, + ): upsert_pr_comment("# Report", s) create_call = mock_run.call_args_list[1] @@ -87,10 +90,13 @@ def test_upsert_updates_existing_comment(monkeypatch: pytest.MonkeyPatch) -> Non existing = [{"id": 99, "body": "\nold"}] list_response = MagicMock(returncode=0, stdout=json.dumps(existing)) - with patch( - "python_security_auditing.pr_comment.subprocess.run", - side_effect=[list_response, MagicMock()], - ) as mock_run: + with ( + patch("python_security_auditing.pr_comment.shutil.which", side_effect=lambda exe: exe), + patch( + "python_security_auditing.pr_comment.subprocess.run", + side_effect=[list_response, MagicMock()], + ) as mock_run, + ): upsert_pr_comment("# Report", s) patch_call = mock_run.call_args_list[1] @@ -110,13 +116,27 @@ def test_upsert_does_not_match_different_workflow(monkeypatch: pytest.MonkeyPatc other_workflow_comment = [{"id": 99, "body": "\nold"}] list_response = MagicMock(returncode=0, stdout=json.dumps(other_workflow_comment)) - with patch( - "python_security_auditing.pr_comment.subprocess.run", - side_effect=[list_response, MagicMock()], - ) as mock_run: + with ( + patch("python_security_auditing.pr_comment.shutil.which", side_effect=lambda exe: exe), + patch( + "python_security_auditing.pr_comment.subprocess.run", + side_effect=[list_response, MagicMock()], + ) as mock_run, + ): upsert_pr_comment("# Report", s) # Must create a new comment, not PATCH the existing one create_call = mock_run.call_args_list[1] cmd: list[str] = create_call[0][0] assert cmd[:3] == ["gh", "pr", "comment"] + + +def test_upsert_raises_when_gh_not_found(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("POST_PR_COMMENT", "true") + monkeypatch.setenv("GITHUB_TOKEN", "tok") + monkeypatch.setenv("GITHUB_REPOSITORY", "org/repo") + monkeypatch.setenv("PR_NUMBER", "42") + s = Settings() + with patch("python_security_auditing.pr_comment.shutil.which", return_value=None): + with pytest.raises(FileNotFoundError, match="gh"): + upsert_pr_comment("# Report", s) diff --git a/tests/test_runners.py b/tests/test_runners.py index 066bc4e..4c0e22d 100644 --- a/tests/test_runners.py +++ b/tests/test_runners.py @@ -30,7 +30,10 @@ def test_uv_mode_calls_uv_export(monkeypatch: pytest.MonkeyPatch, tmp_path: Path monkeypatch.setenv("PACKAGE_MANAGER", "uv") s = Settings() - with patch("python_security_auditing.runners.subprocess.run") as mock_run: + 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) result = generate_requirements(s) @@ -47,7 +50,10 @@ def test_pip_mode_calls_pip_freeze(monkeypatch: pytest.MonkeyPatch) -> None: s = Settings() freeze_output = "requests==2.31.0\n" - with patch("python_security_auditing.runners.subprocess.run") as mock_run: + 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) @@ -60,7 +66,10 @@ def test_poetry_mode_calls_poetry_export(monkeypatch: pytest.MonkeyPatch) -> Non monkeypatch.setenv("PACKAGE_MANAGER", "poetry") s = Settings() - with patch("python_security_auditing.runners.subprocess.run") as mock_run: + 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) result = generate_requirements(s) @@ -78,7 +87,10 @@ def test_pipenv_mode_calls_pipenv_requirements(monkeypatch: pytest.MonkeyPatch) s = Settings() pipenv_output = "requests==2.31.0\n" - with patch("python_security_auditing.runners.subprocess.run") as mock_run: + 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=pipenv_output) result = generate_requirements(s) @@ -156,7 +168,10 @@ def test_run_pip_audit_parses_json(tmp_path: Path, monkeypatch: pytest.MonkeyPat deps = json.loads((FIXTURES / "pip_audit_fixable.json").read_text()) fixture_text = json.dumps({"dependencies": deps, "fixes": []}) - with patch("python_security_auditing.runners.subprocess.run") as mock_run: + 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=1, stderr="", stdout=fixture_text) report = run_pip_audit(Path("requirements.txt")) @@ -171,7 +186,10 @@ def test_run_pip_audit_returns_empty_on_no_output( ) -> None: monkeypatch.chdir(tmp_path) - with patch("python_security_auditing.runners.subprocess.run") as mock_run: + 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")) @@ -184,7 +202,10 @@ def test_run_pip_audit_uses_requirements_path( monkeypatch.chdir(tmp_path) req_path = tmp_path / "custom-reqs.txt" - with patch("python_security_auditing.runners.subprocess.run") as mock_run: + 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(req_path) @@ -198,7 +219,10 @@ def test_run_pip_audit_command_includes_no_deps( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: monkeypatch.chdir(tmp_path) - with patch("python_security_auditing.runners.subprocess.run") as mock_run: + 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")) cmd = mock_run.call_args[0][0] @@ -216,7 +240,10 @@ def test_generate_requirements_uv_returns_empty_on_missing_lockfile( monkeypatch.setenv("PACKAGE_MANAGER", "uv") s = Settings() exc = subprocess.CalledProcessError(2, "uv", stderr="No uv.lock found") - with patch("python_security_auditing.runners.subprocess.run", side_effect=exc): + 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 @@ -228,7 +255,10 @@ def test_generate_requirements_poetry_returns_empty_on_missing_lockfile( monkeypatch.setenv("PACKAGE_MANAGER", "poetry") s = Settings() export_exc = subprocess.CalledProcessError(1, "poetry", stderr="poetry.lock not found") - with patch("python_security_auditing.runners.subprocess.run") as mock_run: + 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] @@ -243,7 +273,10 @@ def test_generate_requirements_pipenv_returns_empty_on_missing_lockfile( monkeypatch.setenv("PACKAGE_MANAGER", "pipenv") s = Settings() exc = subprocess.CalledProcessError(1, "pipenv", stderr="Pipfile.lock not found") - with patch("python_security_auditing.runners.subprocess.run", side_effect=exc): + 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 @@ -255,6 +288,33 @@ def test_generate_requirements_uv_warns_on_missing_lockfile( monkeypatch.setenv("PACKAGE_MANAGER", "uv") s = Settings() exc = subprocess.CalledProcessError(2, "uv", stderr="No uv.lock found") - with patch("python_security_auditing.runners.subprocess.run", side_effect=exc): + with ( + patch("python_security_auditing.runners.shutil.which", side_effect=lambda exe: exe), + patch("python_security_auditing.runners.subprocess.run", side_effect=exc), + ): generate_requirements(s) assert "uv export failed" in capsys.readouterr().err + + +# --------------------------------------------------------------------------- +# generate_requirements / run_pip_audit — tool-not-found handling +# --------------------------------------------------------------------------- + + +def test_generate_requirements_raises_when_tool_not_found( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setenv("PACKAGE_MANAGER", "uv") + s = Settings() + with patch("python_security_auditing.runners.shutil.which", return_value=None): + with pytest.raises(FileNotFoundError, match="uv"): + generate_requirements(s) + + +def test_run_pip_audit_raises_when_tool_not_found( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.chdir(tmp_path) + with patch("python_security_auditing.runners.shutil.which", return_value=None): + with pytest.raises(FileNotFoundError, match="pip-audit"): + run_pip_audit(Path("requirements.txt")) diff --git a/tests/test_settings.py b/tests/test_settings.py index 66a9c09..e5f18cd 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -1,6 +1,7 @@ """Tests for settings.py — env var parsing and computed properties.""" import pytest +from pydantic import ValidationError from python_security_auditing.settings import Settings @@ -96,3 +97,76 @@ def test_github_workflow(monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.setenv("GITHUB_WORKFLOW", "CI") s = Settings() assert s.github_workflow == "CI" + + +# --------------------------------------------------------------------------- +# Path traversal validation +# --------------------------------------------------------------------------- + + +def test_bandit_sarif_path_rejects_traversal(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("BANDIT_SARIF_PATH", "../etc/passwd") + with pytest.raises(ValidationError): + Settings() + + +def test_requirements_file_rejects_traversal(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("REQUIREMENTS_FILE", "../../etc/shadow") + with pytest.raises(ValidationError): + Settings() + + +def test_github_step_summary_rejects_traversal(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("GITHUB_STEP_SUMMARY", "/tmp/../../etc/hosts") + with pytest.raises(ValidationError): + Settings() + + +# --------------------------------------------------------------------------- +# github_repository format validation +# --------------------------------------------------------------------------- + + +def test_github_repository_accepts_valid(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("GITHUB_REPOSITORY", "my-org/my-repo") + s = Settings() + assert s.github_repository == "my-org/my-repo" + + +def test_github_repository_rejects_traversal(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("GITHUB_REPOSITORY", "../../secret-org/secret-repo") + with pytest.raises(ValidationError): + Settings() + + +def test_github_repository_rejects_extra_slashes(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("GITHUB_REPOSITORY", "org/repo/extra") + with pytest.raises(ValidationError): + Settings() + + +def test_github_repository_empty_is_allowed() -> None: + s = Settings() + assert s.github_repository == "" + + +# --------------------------------------------------------------------------- +# github_run_id numeric validation +# --------------------------------------------------------------------------- + + +def test_github_run_id_accepts_numeric(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("GITHUB_RUN_ID", "12345678") + s = Settings() + assert s.github_run_id == "12345678" + + +def test_github_run_id_rejects_non_numeric(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("GITHUB_RUN_ID", "123; rm -rf /") + with pytest.raises(ValidationError): + Settings() + + +def test_github_run_id_empty_is_allowed() -> None: + s = Settings() + assert s.github_run_id == "" From 4e77eea0bc352f6c808dbe75ec486a1fee9ac637 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Houpert?= <10154151+lhoupert@users.noreply.github.com> Date: Mon, 30 Mar 2026 00:12:35 +0100 Subject: [PATCH 4/6] chore: add branch validation name --- src/python_security_auditing/pr_comment.py | 8 +++---- src/python_security_auditing/runners.py | 12 +++++----- src/python_security_auditing/settings.py | 11 +++++++++ tests/test_settings.py | 28 ++++++++++++++++++++++ 4 files changed, 49 insertions(+), 10 deletions(-) diff --git a/src/python_security_auditing/pr_comment.py b/src/python_security_auditing/pr_comment.py index b8a4d4c..f5fcf1f 100644 --- a/src/python_security_auditing/pr_comment.py +++ b/src/python_security_auditing/pr_comment.py @@ -32,7 +32,7 @@ def resolve_pr_number(settings: Settings) -> int | None: if not settings.github_head_ref or not settings.github_repository: return None - result = subprocess.run( + result = subprocess.run( # nosec B603,B605 -- list args, full path via _resolve_exe() [ _resolve_exe("gh"), "pr", @@ -73,7 +73,7 @@ def upsert_pr_comment(markdown: str, settings: Settings) -> None: # Find an existing comment with our marker existing_id: int | None = None - list_result = subprocess.run( + list_result = subprocess.run( # nosec B603,B605 -- list args, full path via _resolve_exe() [_resolve_exe("gh"), "api", f"repos/{repo}/issues/{pr_number}/comments"], capture_output=True, text=True, @@ -85,7 +85,7 @@ def upsert_pr_comment(markdown: str, settings: Settings) -> None: break if existing_id is not None: - subprocess.run( + subprocess.run( # nosec B603,B605 -- list args, full path via _resolve_exe() [ _resolve_exe("gh"), "api", @@ -98,7 +98,7 @@ def upsert_pr_comment(markdown: str, settings: Settings) -> None: check=True, ) else: - subprocess.run( + subprocess.run( # nosec B603,B605 -- list args, full path via _resolve_exe() [_resolve_exe("gh"), "pr", "comment", str(pr_number), "--body", body, "--repo", repo], check=True, ) diff --git a/src/python_security_auditing/runners.py b/src/python_security_auditing/runners.py index bedae19..8accbe2 100644 --- a/src/python_security_auditing/runners.py +++ b/src/python_security_auditing/runners.py @@ -50,7 +50,7 @@ def generate_requirements(settings: Settings) -> Path: if settings.debug: print(f"[debug] uv export command: {cmd}", file=sys.stderr) try: - subprocess.run( + subprocess.run( # nosec B603,B605 -- list args, full path via _resolve_exe() cmd, check=True, capture_output=True, @@ -68,7 +68,7 @@ def generate_requirements(settings: Settings) -> Path: file=sys.stderr, ) elif pm == "pip": - result = subprocess.run( + 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) @@ -76,14 +76,14 @@ def generate_requirements(settings: Settings) -> Path: 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( + subprocess.run( # nosec B603,B605 -- list args, full path via _resolve_exe() [_resolve_exe("poetry"), "self", "add", "poetry-plugin-export"], check=False, capture_output=True, text=True, ) try: - subprocess.run( + subprocess.run( # nosec B603,B605 -- list args, full path via _resolve_exe() [ _resolve_exe("poetry"), "export", @@ -110,7 +110,7 @@ def generate_requirements(settings: Settings) -> Path: ) elif pm == "pipenv": try: - result = subprocess.run( + result = subprocess.run( # nosec B603,B605 -- list args, full path via _resolve_exe() [_resolve_exe("pipenv"), "requirements"], capture_output=True, text=True, check=True ) out_path.write_text(result.stdout) @@ -181,7 +181,7 @@ def run_pip_audit( if settings and settings.debug: print(f"[debug] pip-audit command: {cmd}", file=sys.stderr) - result = subprocess.run(cmd, capture_output=True, text=True) + 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( diff --git a/src/python_security_auditing/settings.py b/src/python_security_auditing/settings.py index 9726fd9..bb447f7 100644 --- a/src/python_security_auditing/settings.py +++ b/src/python_security_auditing/settings.py @@ -2,6 +2,7 @@ from __future__ import annotations +import re from pathlib import Path from typing import Literal @@ -86,6 +87,16 @@ def _validate_run_id(cls, v: str) -> str: return v github_head_ref: str = "" # Branch name for PRs + + @field_validator("github_head_ref", mode="after") + @classmethod + def _validate_head_ref(cls, v: str) -> str: + if v and not re.fullmatch(r"[a-zA-Z0-9._/\-]+", v): + raise ValueError( + f"github_head_ref contains invalid characters for a branch name: {v!r}" + ) + return v + github_workflow: str = "" # Name of the running workflow github_step_summary: str = "" # Path to step summary file diff --git a/tests/test_settings.py b/tests/test_settings.py index e5f18cd..5ff0828 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -170,3 +170,31 @@ def test_github_run_id_rejects_non_numeric(monkeypatch: pytest.MonkeyPatch) -> N def test_github_run_id_empty_is_allowed() -> None: s = Settings() assert s.github_run_id == "" + + +# --------------------------------------------------------------------------- +# github_head_ref branch name validation +# --------------------------------------------------------------------------- + + +def test_github_head_ref_accepts_dependabot_branch(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("GITHUB_HEAD_REF", "dependabot/pip/requests-2.32.0") + s = Settings() + assert s.github_head_ref == "dependabot/pip/requests-2.32.0" + + +def test_github_head_ref_empty_is_allowed() -> None: + s = Settings() + assert s.github_head_ref == "" + + +def test_github_head_ref_rejects_semicolon(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("GITHUB_HEAD_REF", "main; rm -rf /") + with pytest.raises(ValidationError): + Settings() + + +def test_github_head_ref_rejects_newline(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("GITHUB_HEAD_REF", "feature/branch\ninjected") + with pytest.raises(ValidationError): + Settings() From 14cd0948eb61dd4b4339303e5454b8ddc03cf16a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Houpert?= <10154151+lhoupert@users.noreply.github.com> Date: Mon, 30 Mar 2026 00:25:31 +0100 Subject: [PATCH 5/6] test: fix test issue when running on gha --- tests/test_settings.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/test_settings.py b/tests/test_settings.py index 5ff0828..07d9089 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -145,7 +145,8 @@ def test_github_repository_rejects_extra_slashes(monkeypatch: pytest.MonkeyPatch Settings() -def test_github_repository_empty_is_allowed() -> None: +def test_github_repository_empty_is_allowed(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("GITHUB_REPOSITORY", "") s = Settings() assert s.github_repository == "" @@ -167,7 +168,8 @@ def test_github_run_id_rejects_non_numeric(monkeypatch: pytest.MonkeyPatch) -> N Settings() -def test_github_run_id_empty_is_allowed() -> None: +def test_github_run_id_empty_is_allowed(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("GITHUB_RUN_ID", "") s = Settings() assert s.github_run_id == "" @@ -183,7 +185,8 @@ def test_github_head_ref_accepts_dependabot_branch(monkeypatch: pytest.MonkeyPat assert s.github_head_ref == "dependabot/pip/requests-2.32.0" -def test_github_head_ref_empty_is_allowed() -> None: +def test_github_head_ref_empty_is_allowed(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("GITHUB_HEAD_REF", "") s = Settings() assert s.github_head_ref == "" From 194c81defb76f04721dd16c8d903469895a59c92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Houpert?= <10154151+lhoupert@users.noreply.github.com> Date: Mon, 30 Mar 2026 00:36:29 +0100 Subject: [PATCH 6/6] ci: add dedicated zizmor action --- .github/workflows/ci.yml | 16 ---------------- .github/workflows/zizmor.yml | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 16 deletions(-) create mode 100644 .github/workflows/zizmor.yml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 135612b..bc66bf4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -21,7 +21,6 @@ jobs: - uses: astral-sh/setup-uv@d4b2f3b6ecc6e67c4457f6d3e41ec42d3d0fcb86 # v5.3.1 - run: uv pip install --system pre-commit==4.2.0 - run: pre-commit run --all-files --show-diff-on-failure --color=always - test: runs-on: ubuntu-latest @@ -38,21 +37,6 @@ jobs: - name: Run tests run: pytest - zizmor: - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - with: - persist-credentials: false - - uses: actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v6.2.0 - with: - python-version: '3.13' - - uses: astral-sh/setup-uv@d4b2f3b6ecc6e67c4457f6d3e41ec42d3d0fcb86 # v5.3.1 - - name: Install zizmor - run: uv pip install --system zizmor==1.6.0 - - name: Run zizmor - run: zizmor --min-severity medium .github/ - security: runs-on: ubuntu-latest permissions: diff --git a/.github/workflows/zizmor.yml b/.github/workflows/zizmor.yml new file mode 100644 index 0000000..1fb002a --- /dev/null +++ b/.github/workflows/zizmor.yml @@ -0,0 +1,32 @@ +--- +# https://github.com/woodruffw/zizmor +name: Zizmor + +on: + push: + branches: [main] + pull_request: + branches: ["**"] + +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + +permissions: {} + +jobs: + zizmor: + name: Run zizmor + runs-on: ubuntu-latest + permissions: + security-events: write # Required for upload-sarif (used by zizmor-action) to upload SARIF files. + steps: + - name: Checkout repository + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + persist-credentials: false + + - name: Run zizmor + uses: zizmorcore/zizmor-action@71321a20a9ded102f6e9ce5718a2fcec2c4f70d8 # v0.5.2 + with: + persona: pedantic