From 0fc866b43bae35ff4bc6d667db5b2163a2a955ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Houpert?= <10154151+lhoupert@users.noreply.github.com> Date: Tue, 6 Oct 2026 16:12:34 +0100 Subject: [PATCH 1/8] fix: run bandit in the action's Python module instead of the bandit-action fork The lhoupert/bandit-action fork had three problems: - It installed an unpinned bandit on Python 3.9 (bandit 1.8.6), so files using 3.10+ syntax were skipped silently. - It ran bandit with `|| true` under continue-on-error, so a crash or a missing SARIF read as a clean scan. - It passed a comma-separated bandit_scan_dirs through unchanged when working_directory was '.', so bandit scanned nothing. run_bandit() now runs bandit[sarif]>=1.9.4 from the action's own environment (Python 3.13), on each comma-separated scan dir. It runs from the repository root, so SARIF paths stay repo-relative. Finding paths are percent-decoded, and annotation property values are escaped. It fails closed, as pip-audit does since the previous commits. Each of these cases is reported as "bandit did NOT run", and pip-audit results are still reported: - a missing scan dir; - a non-zero exit (with --exit-zero, findings exit 0, so anything else is a crash or a usage error); - a missing or invalid SARIF; - a file bandit could not parse, listed (at most 20) with how to exclude it on purpose in a `.bandit` file; - no Python file scanned; - an OSError while running bandit. In every case results.sarif is deleted, so neither the artifact nor Code Scanning gets an empty or partial report: that would close open alerts. A SARIF without bandit's file metrics only means the scanned file count is unknown, which is a warning. action.yml: - Removes an earlier results.sarif before the audit. - Uploads it to Code Scanning, as the fork did, with its own pinned upload-sarif step. The step runs before the artifact upload and has continue-on-error, so security-events: write stays optional. Co-Authored-By: Claude Opus 5.5 (1M context) --- CLAUDE.md | 4 +- README.md | 10 +- action.yml | 38 +-- pyproject.toml | 1 + src/python_security_auditing/__main__.py | 30 ++- src/python_security_auditing/annotations.py | 14 +- src/python_security_auditing/report.py | 13 +- src/python_security_auditing/runners.py | 89 ++++++- src/python_security_auditing/settings.py | 6 +- tests/conftest.py | 7 + tests/fixtures/bandit_clean.sarif | 4 + tests/fixtures/bandit_issues.sarif | 4 + tests/test_annotations.py | 24 ++ tests/test_main.py | 92 +++++-- tests/test_report.py | 9 + tests/test_runners.py | 276 ++++++++++++++++++-- uv.lock | 100 ++++++- 17 files changed, 619 insertions(+), 102 deletions(-) create mode 100644 tests/conftest.py diff --git a/CLAUDE.md b/CLAUDE.md index f9f5641..a52809a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -30,7 +30,7 @@ src/python_security_auditing/ - **Build system:** Hatch (`hatchling`) - **Python:** ≥ 3.13 -- **Dependencies:** `pydantic-settings`, `pip-audit` +- **Dependencies:** `pydantic-settings`, `pip-audit`, `bandit[sarif]` - **Dev deps:** `pytest`, `pytest-mock`, `mypy` (strict), `ruff` ### Common Commands @@ -68,7 +68,7 @@ uv run ruff format src/ tests/ ## Key Design Decisions -- **SARIF input for bandit:** Bandit runs in a separate composite step (`lhoupert/bandit-action`). This package only reads the SARIF output file — it does not invoke bandit directly. +- **Bandit runs in-process:** `run_bandit()` runs bandit from the package's own environment, writes `results.sarif` at the workspace root (artifact and Code Scanning upload), and reads it back. It fails closed: a crash, a skipped file or no file scanned raises `AuditError`. - **PR comment is idempotent:** Uses a hidden HTML marker (``) to find and update the same comment on subsequent pushes. - **Threshold logic:** `check_thresholds()` in `report.py` returns a boolean; the orchestrator translates that to `sys.exit(1)`. - **Package manager adapters:** `generate_requirements()` normalizes all package managers to a `requirements.txt` file before passing to `pip-audit`. diff --git a/README.md b/README.md index 3ff4d6a..d3cda50 100644 --- a/README.md +++ b/README.md @@ -161,7 +161,7 @@ permissions: security-events: write ``` -If you don't need Code Scanning integration, `contents: read` alone is sufficient. +If you don't need Code Scanning integration, `contents: read` alone is sufficient: the SARIF upload then fails without failing the job. ## Usage examples @@ -212,6 +212,10 @@ When your source code spans more than one directory, pass a comma-separated list bandit_scan_dirs: 'src/,scripts/' ``` +### Configuring bandit + +Bandit honours a `.bandit` file (INI, `[bandit]` section, for example `exclude` or `skips`) placed in a scanned directory. `[tool.bandit]` in `pyproject.toml` is not read. Bandit runs on Python 3.13, and a file it cannot parse fails the job, because its findings would otherwise go unreported. To skip such a file on purpose, list it under `exclude`. + ### Project in a subdirectory (monorepo) Set `working_directory` to the project root within the repo. All relative paths (scan dirs, requirements file) are resolved from there: @@ -343,8 +347,8 @@ The job fails (non-zero exit) when **either** tool finds issues above its config - **Annotations** — always emitted. Bandit findings appear as inline annotations on the PR "Files changed" tab (keyed to file and line). pip-audit findings appear as summary-level annotations. No email notifications are generated. - **Step summary** — the full report is written to the workflow run summary, visible under the "Summary" tab. - **PR comment** — opt-in via `comment_on: blocking` or `comment_on: always`. Created on first run, updated in place on every subsequent run. The comment is keyed on a hidden `` marker, so multiple workflows on the same PR each maintain their own separate comment. -- **Artifact** — `pip-audit-report.json` and `results.sarif` uploaded under the name set by `artifact_name` (default: `security-audit-reports`) for download or downstream steps. The `results.sarif` file is the bandit SARIF report; it is also uploaded to GitHub Code Scanning automatically by the underlying `lhoupert/bandit-action` step, making findings visible in the repository's Security tab when the job has `security-events: write` permission. -- **Exit code** — non-zero when blocking issues are found, so the job fails and branch protections can enforce it. +- **Artifact** — `pip-audit-report.json` and `results.sarif` uploaded under the name set by `artifact_name` (default: `security-audit-reports`) for download or downstream steps. The `results.sarif` file is the bandit SARIF report; it is also uploaded to GitHub Code Scanning with `github/codeql-action/upload-sarif`, making findings visible in the repository's Security tab when the job has `security-events: write` permission. +- **Exit code** — non-zero when blocking issues are found, or when a tool could not run fully (for bandit: a missing scan dir, a file it cannot parse, or no Python file to scan), so the job fails and branch protections can enforce it. ## Development diff --git a/action.yml b/action.yml index 74b4e90..3207a04 100644 --- a/action.yml +++ b/action.yml @@ -40,32 +40,11 @@ inputs: runs: using: composite steps: - - name: Resolve bandit targets + # A report left by an earlier job (self-hosted runner) must not be uploaded as this one's. + - name: Remove an earlier bandit report if: contains(inputs.tools, 'bandit') - id: resolve-targets shell: bash - env: - WORKING_DIRECTORY: ${{ inputs.working_directory }} - BANDIT_SCAN_DIRS: ${{ inputs.bandit_scan_dirs }} - run: | - if [[ "$WORKING_DIRECTORY" == "." ]]; then - echo "targets=$BANDIT_SCAN_DIRS" >> "$GITHUB_OUTPUT" - else - resolved="" - IFS=',' read -ra parts <<< "$BANDIT_SCAN_DIRS" - for part in "${parts[@]}"; do - [[ "$part" == "." ]] && t="$WORKING_DIRECTORY" || t="$WORKING_DIRECTORY/$part" - resolved="${resolved:+$resolved }$t" - done - echo "targets=$resolved" >> "$GITHUB_OUTPUT" - fi - - - name: Run Bandit (static security analysis) - if: contains(inputs.tools, 'bandit') - continue-on-error: true - uses: lhoupert/bandit-action@e2d48932beda5cc8c50cb45ed39f8d873ef2d365 - with: - targets: ${{ steps.resolve-targets.outputs.targets }} + run: rm -f -- "$GITHUB_WORKSPACE/results.sarif" - name: Set up uv uses: astral-sh/setup-uv@c18668ad3cf93ea998bef934396af7bb5c839dc7 # v10.2.0 @@ -79,6 +58,7 @@ runs: working-directory: ${{ inputs.working_directory }} env: TOOLS: ${{ inputs.tools }} + BANDIT_SCAN_DIRS: ${{ inputs.bandit_scan_dirs }} BANDIT_SEVERITY_THRESHOLD: ${{ inputs.bandit_severity_threshold }} BANDIT_SARIF_PATH: ${{ github.workspace }}/results.sarif PIP_AUDIT_BLOCK_ON: ${{ inputs.pip_audit_block_on }} @@ -91,6 +71,16 @@ runs: RUNNER_DEBUG: ${{ runner.debug }} run: uv run --no-project --with "$GITHUB_ACTION_PATH" python -m python_security_auditing + # results.sarif exists only if bandit scanned every file in this job: the first step removes + # an earlier one, and the audit deletes it when bandit fails (an empty report would close + # every open alert). Runs before the artifact upload, so a failed upload cannot skip it. + - name: Upload bandit SARIF to Code Scanning + if: contains(inputs.tools, 'bandit') && hashFiles('results.sarif') != '' + continue-on-error: true # needs `security-events: write`; the audit result stands without it + uses: github/codeql-action/upload-sarif@2892aa5e19bbd11bc0cff5427e3b750a04d9e3c2 # v4.38.2 + with: + sarif_file: ${{ github.workspace }}/results.sarif + - name: Upload ${{ inputs.artifact_name }} if: always() uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 diff --git a/pyproject.toml b/pyproject.toml index de9e9c8..dbf0087 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -11,6 +11,7 @@ requires-python = ">=3.13" dependencies = [ "pydantic-settings>=2.14.2", "pip-audit>=2.7", + "bandit[sarif]>=1.9.4", ] [project.optional-dependencies] diff --git a/src/python_security_auditing/__main__.py b/src/python_security_auditing/__main__.py index 06bf994..1283d64 100644 --- a/src/python_security_auditing/__main__.py +++ b/src/python_security_auditing/__main__.py @@ -3,7 +3,6 @@ from __future__ import annotations import sys -from pathlib import Path from typing import Any from .annotations import emit_annotations @@ -13,7 +12,7 @@ PIP_AUDIT_REPORT, AuditError, generate_requirements, - read_bandit_sarif, + run_bandit, run_pip_audit, ) from .settings import Settings @@ -27,14 +26,14 @@ def main() -> None: bandit_report: dict[str, Any] = {} pip_audit_report: list[dict[str, Any]] = [] + bandit_error = "" pip_audit_error = "" if "bandit" in settings.enabled_tools: - if settings.debug: - print( - f"[debug] reading bandit SARIF from {settings.bandit_sarif_path}", file=sys.stderr - ) - bandit_report = read_bandit_sarif(Path(settings.bandit_sarif_path)) + try: + bandit_report = run_bandit(settings) + except (AuditError, FileNotFoundError) as exc: + bandit_error = str(exc) if settings.debug: print( f"[debug] bandit findings: {len(bandit_report.get('results', []))}", file=sys.stderr @@ -57,11 +56,13 @@ def main() -> None: 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, pip_audit_error) + markdown = build_markdown( + bandit_report, pip_audit_report, settings, pip_audit_error, bandit_error + ) write_step_summary(markdown, settings) - emit_annotations(bandit_report, pip_audit_report, settings, pip_audit_error) + emit_annotations(bandit_report, pip_audit_report, settings, pip_audit_error, bandit_error) - has_blocking = bool(pip_audit_error) or check_thresholds( + has_blocking = bool(bandit_error or pip_audit_error) or check_thresholds( bandit_report, pip_audit_report, settings ) @@ -69,8 +70,15 @@ def main() -> None: if settings.comment_on == "always" or has_blocking: upsert_pr_comment(markdown, settings) + failures = [] + if bandit_error: + failures.append(f"bandit did NOT run, so the code was NOT scanned.\n{bandit_error}") if pip_audit_error: - sys.exit(f"pip-audit did NOT run, so dependencies were NOT audited.\n{pip_audit_error}") + failures.append( + f"pip-audit did NOT run, so dependencies were NOT audited.\n{pip_audit_error}" + ) + if failures: + sys.exit("\n".join(failures)) if has_blocking: sys.exit(1) diff --git a/src/python_security_auditing/annotations.py b/src/python_security_auditing/annotations.py index fb83df1..501ee2f 100644 --- a/src/python_security_auditing/annotations.py +++ b/src/python_security_auditing/annotations.py @@ -19,11 +19,17 @@ def _escape(text: str) -> str: return text.replace("%", "%25").replace("\r", "%0D").replace("\n", "%0A") +def _escape_property(text: str) -> str: + """Escape a workflow-command property value (file=...), where ':' and ',' are separators.""" + return _escape(text).replace(":", "%3A").replace(",", "%2C") + + def emit_annotations( bandit_report: dict[str, Any], pip_audit_report: list[dict[str, Any]], settings: Settings, pip_audit_error: str = "", + bandit_error: str = "", ) -> None: """Print GitHub Actions workflow commands to stdout. @@ -32,6 +38,12 @@ def emit_annotations( No email notifications are generated by annotations. """ if "bandit" in settings.enabled_tools: + if bandit_error: + print(f"::error::bandit did NOT run: {_escape(bandit_error)}") + elif bandit_report.get("files_read", 0) is None: + print( + "::warning::bandit reported no file metrics, so the scanned file count is unknown" + ) results: list[dict[str, Any]] = bandit_report.get("results", []) def _sort_key(r: dict[str, Any]) -> int: @@ -40,7 +52,7 @@ def _sort_key(r: dict[str, Any]) -> int: for result in sorted(results, key=_sort_key): sev = result.get("issue_severity", "LOW") level = _SEVERITY_TO_LEVEL.get(sev, "notice") - fname = result.get("filename", "") + fname = _escape_property(result.get("filename", "")) line = result.get("line_number", 0) test_id = result.get("test_id", "") text = _escape(result.get("issue_text", "")) diff --git a/src/python_security_auditing/report.py b/src/python_security_auditing/report.py index b4e303a..0525ad6 100644 --- a/src/python_security_auditing/report.py +++ b/src/python_security_auditing/report.py @@ -14,6 +14,7 @@ def build_markdown( pip_audit_report: list[dict[str, Any]], settings: Settings, pip_audit_error: str = "", + bandit_error: str = "", ) -> str: """Build a full markdown security report.""" sections: list[str] = ["# Security Audit Report\n"] @@ -27,12 +28,14 @@ def build_markdown( sections.append(f"[View workflow run]({run_url})\n") if "bandit" in settings.enabled_tools: - sections.append(_bandit_section(bandit_report, settings)) + sections.append(_bandit_section(bandit_report, settings, bandit_error)) if "pip-audit" in settings.enabled_tools: sections.append(_pip_audit_section(pip_audit_report, settings, pip_audit_error)) - blocking = check_thresholds(bandit_report, pip_audit_report, settings) or bool(pip_audit_error) + blocking = check_thresholds(bandit_report, pip_audit_report, settings) or bool( + bandit_error or pip_audit_error + ) sections.append("---\n") if blocking: sections.append("**Result: ❌ Blocking issues found — see details above.**\n") @@ -42,7 +45,7 @@ def build_markdown( return "\n".join(sections) -def _bandit_section(report: dict[str, Any], settings: Settings) -> str: +def _bandit_section(report: dict[str, Any], settings: Settings, error: str) -> str: results: list[dict[str, Any]] = report.get("results", []) security_url = ( f"https://github.com/{settings.github_repository}/security/code-scanning" @@ -56,6 +59,10 @@ def _bandit_section(report: dict[str, Any], settings: Settings) -> str: ) lines = [heading] + if error: + lines.append(f"❌ bandit did NOT run, so the code was NOT scanned.\n\n```\n{error}\n```\n") + return "\n".join(lines) + if not results: lines.append("✅ No issues found.\n") return "\n".join(lines) diff --git a/src/python_security_auditing/runners.py b/src/python_security_auditing/runners.py index 32976c6..bef237a 100644 --- a/src/python_security_auditing/runners.py +++ b/src/python_security_auditing/runners.py @@ -2,6 +2,7 @@ from __future__ import annotations +import contextlib import json import os import shutil @@ -10,6 +11,7 @@ import tempfile from pathlib import Path from typing import Any +from urllib.parse import unquote from .settings import Settings @@ -17,7 +19,7 @@ class AuditError(Exception): - """The dependency list or the pip-audit report could not be produced.""" + """A tool could not run, so its report could not be produced.""" def _resolve_exe(name: str) -> str: @@ -149,14 +151,27 @@ def generate_requirements(settings: Settings) -> Path: def read_bandit_sarif(sarif_path: Path) -> dict[str, Any]: - """Read results.sarif produced by lhoupert/bandit-action, return bandit-style report dict.""" - if not sarif_path.exists(): - return {"results": [], "errors": []} + """Read bandit's SARIF report, return a bandit-style report dict. + + "errors" lists the files bandit skipped. "files_read" counts the files bandit read, or is + None when the report has no per-file metrics. Raises AuditError when the report is + missing or is not SARIF. + """ + try: + run: dict[str, Any] = json.loads(sarif_path.read_text())["runs"][0] + # bandit details, which a release could drop: it lists the files it could not open or + # parse as notifications, and keeps metrics for every file it read, plus "_totals". + errors = [] + for notice in (run.get("invocations") or [{}])[0].get("toolConfigurationNotifications", []): + phys = (notice.get("locations") or [{}])[0].get("physicalLocation", {}) + uri = phys.get("artifactLocation", {}).get("uri", "") + errors.append({"filename": unquote(uri), "reason": notice["message"]["text"]}) + metrics = run.get("properties", {}).get("metrics") + except (OSError, ValueError, LookupError, TypeError, AttributeError) as exc: + raise AuditError(f"bandit wrote no valid SARIF report to {sarif_path}: {exc!r}") from exc - sarif: dict[str, Any] = json.loads(sarif_path.read_text()) - sarif_results: list[dict[str, Any]] = sarif.get("runs", [{}])[0].get("results", []) results: list[dict[str, Any]] = [] - for sarif_result in sarif_results: + for sarif_result in run.get("results", []): props: dict[str, Any] = sarif_result.get("properties", {}) severity = props.get("issue_severity") or _SARIF_LEVEL_TO_SEVERITY.get( sarif_result.get("level", "none"), "LOW" @@ -166,7 +181,7 @@ def read_bandit_sarif(sarif_path: Path) -> dict[str, Any]: line_number = 0 if locations: phys = locations[0].get("physicalLocation", {}) - filename = phys.get("artifactLocation", {}).get("uri", "") + filename = unquote(phys.get("artifactLocation", {}).get("uri", "")) # bandit %-encodes line_number = phys.get("region", {}).get("startLine", 0) results.append( { @@ -179,7 +194,63 @@ def read_bandit_sarif(sarif_path: Path) -> dict[str, Any]: } ) - return {"results": results, "errors": []} + files_read = None if metrics is None else len(set(metrics) - {"_totals"}) + return {"results": results, "errors": errors, "files_read": files_read} + + +def run_bandit(settings: Settings) -> dict[str, Any]: + """Run bandit on bandit_scan_dirs, write its SARIF report, return the parsed report. + + Raises AuditError, and leaves no SARIF report, when bandit cannot run, fails, skips a + file or reads none: an empty or partial report would close open Code Scanning alerts. + """ + sarif_path = Path(settings.bandit_sarif_path).resolve() + try: + sarif_path.unlink(missing_ok=True) # never upload an earlier run's report + dirs = [d.strip() for d in settings.bandit_scan_dirs.split(",") if d.strip()] + missing = [d for d in dirs if not Path(d).exists()] + if missing: + raise AuditError(f"bandit_scan_dirs not found in {Path.cwd()}: {', '.join(missing)}") + # Run from the repository root, so that SARIF paths (annotations, Code Scanning) + # stay relative to it whatever the working directory. + root = Path(settings.github_workspace or ".").resolve() + targets = [os.path.relpath(Path(d).resolve(), root) for d in dirs] + # With --exit-zero findings exit 0 too, so any other exit is a crash or a usage error. + cmd = [_resolve_exe("bandit"), "-r", *targets, "-f", "sarif", "-o", str(sarif_path)] + cmd += ["--exit-zero"] + + if settings.debug: + print(f"[debug] bandit command (cwd={root}): {cmd}", file=sys.stderr) + + result = subprocess.run(cmd, cwd=root, capture_output=True, text=True) # nosec B603 -- list args, full path via _resolve_exe() + + if settings.debug: + print( + f"[debug] bandit exit={result.returncode} stderr={result.stderr!r}", file=sys.stderr + ) + + if result.returncode: + raise AuditError(f"bandit failed (exit {result.returncode}):\n{result.stderr.strip()}") + report = read_bandit_sarif(sarif_path) + if skipped := [f"{e['filename']}: {e['reason']}" for e in report["errors"]]: + count = len(skipped) + if count > 20: # the list ends up in the PR comment, which GitHub caps at 65,536 chars + skipped[20:] = [f"… and {count - 20} more"] + raise AuditError( + f"bandit could not scan {count} file(s), so they were NOT checked:\n" + + "\n".join(skipped) + + "\nFix them, or skip them on purpose: list them under `exclude` in the [bandit] " + "section of a `.bandit` file in a scanned directory." + ) + if report["files_read"] == 0: + raise AuditError(f"bandit found no Python file to scan in {targets}") + except (AuditError, OSError) as exc: + with contextlib.suppress(OSError): + sarif_path.unlink(missing_ok=True) + if isinstance(exc, AuditError): + raise + raise AuditError(f"bandit could not run: {exc}") from exc + return report def run_pip_audit( diff --git a/src/python_security_auditing/settings.py b/src/python_security_auditing/settings.py index c5e72fb..29e21c8 100644 --- a/src/python_security_auditing/settings.py +++ b/src/python_security_auditing/settings.py @@ -34,8 +34,9 @@ def debug(self) -> bool: # Tool selection tools: str = "bandit,pip-audit" - # Bandit config — scan dirs and threshold are passed directly to lhoupert/bandit-action; - # the Python module only reads the SARIF output and uses the threshold for reporting. + # Bandit config — comma-separated scan dirs, relative to the working directory. Bandit + # reports every finding; the threshold only decides which ones block the job. + bandit_scan_dirs: str = "." bandit_severity_threshold: Literal["high", "medium", "low"] = "high" bandit_sarif_path: str = "results.sarif" @@ -99,6 +100,7 @@ def _validate_head_ref(cls, v: str) -> str: github_workflow: str = "" # Name of the running workflow github_step_summary: str = "" # Path to step summary file + github_workspace: str = "" # Repository root: bandit reports paths relative to it @property def enabled_tools(self) -> list[str]: diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 0000000..d931456 --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,7 @@ +import pytest + + +@pytest.fixture(autouse=True) +def _no_github_workspace(monkeypatch: pytest.MonkeyPatch) -> None: + """CI sets GITHUB_WORKSPACE; tests that need it set it themselves.""" + monkeypatch.delenv("GITHUB_WORKSPACE", raising=False) diff --git a/tests/fixtures/bandit_clean.sarif b/tests/fixtures/bandit_clean.sarif index daa825c..6066007 100644 --- a/tests/fixtures/bandit_clean.sarif +++ b/tests/fixtures/bandit_clean.sarif @@ -7,6 +7,10 @@ "name": "Bandit" } }, + "invocations": [{ "executionSuccessful": true }], + "properties": { + "metrics": { "_totals": { "loc": 5 }, "./src/app.py": { "loc": 5 } } + }, "results": [] } ] diff --git a/tests/fixtures/bandit_issues.sarif b/tests/fixtures/bandit_issues.sarif index a0aaa9e..3a17236 100644 --- a/tests/fixtures/bandit_issues.sarif +++ b/tests/fixtures/bandit_issues.sarif @@ -7,6 +7,10 @@ "name": "Bandit" } }, + "invocations": [{ "executionSuccessful": true }], + "properties": { + "metrics": { "_totals": { "loc": 5 }, "./src/app.py": { "loc": 5 } } + }, "results": [ { "ruleId": "B404", diff --git a/tests/test_annotations.py b/tests/test_annotations.py index 641c3d0..ffe1443 100644 --- a/tests/test_annotations.py +++ b/tests/test_annotations.py @@ -123,6 +123,30 @@ def test_pip_audit_error_emits_escaped_error( assert out == "::error::pip-audit did NOT run: uv failed%0A100%25 broken\n" +def test_bandit_error_emits_escaped_error( + pip_clean: list[Any], capsys: pytest.CaptureFixture[str] +) -> None: + emit_annotations({}, pip_clean, Settings(), bandit_error="bandit failed (exit 2):\nusage") + out = capsys.readouterr().out + assert out == "::error::bandit did NOT run: bandit failed (exit 2):%0Ausage\n" + + +def test_bandit_file_property_is_escaped( + pip_clean: list[Any], capsys: pytest.CaptureFixture[str] +) -> None: + """In a property value ':' and ',' are separators, so they must be %-encoded too.""" + result = {"issue_severity": "HIGH", "filename": "a,b:c%.py", "line_number": 3, "test_id": "B1"} + emit_annotations({"results": [result]}, pip_clean, Settings()) + assert capsys.readouterr().out == "::error file=a%2Cb%3Ac%25.py,line=3::[B1] \n" + + +def test_bandit_without_file_metrics_warns( + bandit_clean: dict[str, Any], pip_clean: list[Any], capsys: pytest.CaptureFixture[str] +) -> None: + emit_annotations({**bandit_clean, "files_read": None}, pip_clean, Settings()) + assert capsys.readouterr().out.startswith("::warning::bandit reported no file metrics") + + 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 c4919ee..2066894 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -4,6 +4,7 @@ import subprocess from pathlib import Path +from typing import Any from unittest.mock import MagicMock, patch import pytest @@ -15,26 +16,34 @@ CLEAN_REPORT = '{"dependencies": [{"name": "requests", "version": "2.32.0", "vulns": []}]}' -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 _fake_tools(bandit_sarif: str, uv_error: Exception | None = None) -> Any: + """subprocess.run stand-in: bandit writes a fixture SARIF; uv export and pip-audit are clean.""" + + def run(cmd: list[str], **kwargs: object) -> MagicMock: + if cmd[0] == "bandit": + Path(cmd[cmd.index("-o") + 1]).write_text((FIXTURES / bandit_sarif).read_text()) + elif cmd[0] == "uv" and uv_error: + raise uv_error + return MagicMock(returncode=0, stderr="", stdout=CLEAN_REPORT) + + return run def test_comment_on_never_never_calls_upsert( monkeypatch: pytest.MonkeyPatch, tmp_path: Path ) -> None: """comment_on=never (default) must never call upsert_pr_comment, even with a token.""" - 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("GITHUB_TOKEN", "tok") monkeypatch.chdir(tmp_path) with ( patch("python_security_auditing.runners.shutil.which", side_effect=lambda exe: exe), - patch("python_security_auditing.runners.subprocess.run", side_effect=_clean_audit), + patch( + "python_security_auditing.runners.subprocess.run", + side_effect=_fake_tools("bandit_issues.sarif"), + ), patch("python_security_auditing.__main__.emit_annotations"), patch("python_security_auditing.__main__.upsert_pr_comment") as mock_comment, ): @@ -47,11 +56,8 @@ def test_comment_on_blocking_calls_upsert_when_blocking( monkeypatch: pytest.MonkeyPatch, tmp_path: Path ) -> None: """comment_on=blocking must call upsert_pr_comment when blocking issues exist.""" - 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("GITHUB_TOKEN", "tok") monkeypatch.setenv("COMMENT_ON", "blocking") @@ -59,7 +65,10 @@ def test_comment_on_blocking_calls_upsert_when_blocking( with ( patch("python_security_auditing.runners.shutil.which", side_effect=lambda exe: exe), - patch("python_security_auditing.runners.subprocess.run", side_effect=_clean_audit), + patch( + "python_security_auditing.runners.subprocess.run", + side_effect=_fake_tools("bandit_issues.sarif"), + ), patch("python_security_auditing.__main__.emit_annotations"), patch("python_security_auditing.__main__.upsert_pr_comment") as mock_comment, ): @@ -72,18 +81,18 @@ def test_comment_on_blocking_skips_upsert_when_clean( monkeypatch: pytest.MonkeyPatch, tmp_path: Path ) -> None: """comment_on=blocking must not call upsert_pr_comment when no blocking issues.""" - 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("GITHUB_TOKEN", "tok") monkeypatch.setenv("COMMENT_ON", "blocking") monkeypatch.chdir(tmp_path) with ( patch("python_security_auditing.runners.shutil.which", side_effect=lambda exe: exe), - patch("python_security_auditing.runners.subprocess.run", side_effect=_clean_audit), + patch( + "python_security_auditing.runners.subprocess.run", + side_effect=_fake_tools("bandit_clean.sarif"), + ), patch("python_security_auditing.__main__.emit_annotations"), patch("python_security_auditing.__main__.upsert_pr_comment") as mock_comment, ): @@ -95,12 +104,9 @@ 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 @@ -109,7 +115,10 @@ def test_main_fails_closed_when_uv_export_fails( 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=uv_exc), + patch( + "python_security_auditing.runners.subprocess.run", + side_effect=_fake_tools("bandit_clean.sarif", uv_exc), + ), ): with pytest.raises(SystemExit) as exc_info: main() @@ -129,12 +138,9 @@ 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") @@ -144,7 +150,10 @@ def test_main_reports_bandit_and_comments_when_pip_audit_cannot_run( uv_exc = subprocess.CalledProcessError(2, "uv", stderr="No uv.lock found") with ( patch("python_security_auditing.runners.shutil.which", side_effect=lambda exe: exe), - patch("python_security_auditing.runners.subprocess.run", side_effect=uv_exc), + patch( + "python_security_auditing.runners.subprocess.run", + side_effect=_fake_tools("bandit_issues.sarif", uv_exc), + ), patch("python_security_auditing.__main__.upsert_pr_comment") as mock_comment, ): with pytest.raises(SystemExit) as exc_info: @@ -157,3 +166,40 @@ def test_main_reports_bandit_and_comments_when_pip_audit_cannot_run( 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] + + +def test_main_reports_pip_audit_when_bandit_cannot_run( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """A failed bandit run must fail the step and say so, and still report pip-audit.""" + summary_path = tmp_path / "summary.md" + monkeypatch.setenv("PACKAGE_MANAGER", "uv") + monkeypatch.setenv("TOOLS", "bandit,pip-audit") + monkeypatch.setenv("GITHUB_STEP_SUMMARY", str(summary_path)) + monkeypatch.chdir(tmp_path) + + def run(cmd: list[str], **kwargs: object) -> MagicMock: + if cmd[0] == "bandit": + return MagicMock(returncode=2, stderr="ERROR\tMultiple .bandit files found", stdout="") + return MagicMock(returncode=0, stderr="", stdout=CLEAN_REPORT) + + with ( + patch("python_security_auditing.runners.shutil.which", side_effect=lambda exe: exe), + patch("python_security_auditing.runners.subprocess.run", side_effect=run), + ): + with pytest.raises(SystemExit) as exc_info: + main() + + assert str(exc_info.value.code) == ( + "bandit did NOT run, so the code was NOT scanned.\n" + "bandit failed (exit 2):\nERROR\tMultiple .bandit files found" + ) + summary = summary_path.read_text() + assert "bandit did NOT run" in summary + assert "No issues found" not in summary + assert "_Dependencies audited: 1, skipped: 0._" in summary # pip-audit still reported + assert "Blocking issues found" in summary + out = capsys.readouterr().out + assert "::error::bandit did NOT run: bandit failed (exit 2):%0AERROR" in out + assert (tmp_path / "pip-audit-report.json").exists() + assert not (tmp_path / "results.sarif").exists() diff --git a/tests/test_report.py b/tests/test_report.py index 42ac3f2..00f89d8 100644 --- a/tests/test_report.py +++ b/tests/test_report.py @@ -219,6 +219,15 @@ def test_markdown_pip_audit_error(bandit_issues: dict[str, Any]) -> None: assert "No vulnerabilities found" not in md +def test_markdown_bandit_error(pip_fixable: list[Any]) -> None: + md = build_markdown({}, pip_fixable, Settings(), bandit_error="bandit failed (exit 2)") + assert "bandit did NOT run" in md + assert "bandit failed (exit 2)" in md + assert "No issues found" not in md + assert "requests" in md # pip-audit results are still shown + assert "Blocking issues found" 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 6002a76..8801991 100644 --- a/tests/test_runners.py +++ b/tests/test_runners.py @@ -6,6 +6,7 @@ import subprocess import tempfile from pathlib import Path +from typing import Any from unittest.mock import MagicMock, patch import pytest @@ -13,6 +14,7 @@ AuditError, generate_requirements, read_bandit_sarif, + run_bandit, run_pip_audit, ) from python_security_auditing.settings import Settings @@ -122,6 +124,28 @@ def test_pipenv_mode_calls_pipenv_requirements( # --------------------------------------------------------------------------- +def _bandit_sarif( + results: tuple[dict[str, Any], ...] = (), + files: tuple[str, ...] = ("./app.py",), + skipped: tuple[tuple[str, str], ...] = (), +) -> str: + """A SARIF report shaped like bandit's: metrics per file read, skipped files as notices.""" + notices = [ + { + "level": "error", + "message": {"text": reason}, + "locations": [{"physicalLocation": {"artifactLocation": {"uri": uri}}}], + } + for uri, reason in skipped + ] + run = { + "results": list(results), + "invocations": [{"executionSuccessful": True, "toolConfigurationNotifications": notices}], + "properties": {"metrics": {name: {"loc": 1} for name in ("_totals", *files)}}, + } + return json.dumps({"version": "2.1.0", "runs": [run]}) + + def test_read_bandit_sarif_parses_findings(tmp_path: Path) -> None: sarif_path = tmp_path / "results.sarif" sarif_path.write_text((FIXTURES / "bandit_issues.sarif").read_text()) @@ -136,43 +160,249 @@ def test_read_bandit_sarif_parses_findings(tmp_path: Path) -> None: assert report["results"][1]["issue_severity"] == "MEDIUM" -def test_read_bandit_sarif_returns_empty_on_missing_file(tmp_path: Path) -> None: - report = read_bandit_sarif(tmp_path / "results.sarif") - assert report["results"] == [] - assert report["errors"] == [] +def test_read_bandit_sarif_decodes_paths(tmp_path: Path) -> None: + """bandit %-encodes SARIF URIs; annotations and the report need the real path.""" + sarif_path = tmp_path / "results.sarif" + location = {"physicalLocation": {"artifactLocation": {"uri": "src/my%20app.py"}}} + sarif_path.write_text(_bandit_sarif(results=({"ruleId": "B602", "locations": [location]},))) + assert read_bandit_sarif(sarif_path)["results"][0]["filename"] == "src/my app.py" + + +@pytest.mark.parametrize("content", [None, "", "not json", "[]", '{"runs": []}']) +def test_read_bandit_sarif_raises_on_missing_or_invalid_file( + tmp_path: Path, content: str | None +) -> None: + sarif_path = tmp_path / "results.sarif" + if content is not None: + sarif_path.write_text(content) + with pytest.raises(AuditError, match="bandit wrote no valid SARIF report"): + read_bandit_sarif(sarif_path) def test_read_bandit_sarif_returns_empty_on_clean_sarif(tmp_path: Path) -> None: sarif_path = tmp_path / "results.sarif" sarif_path.write_text((FIXTURES / "bandit_clean.sarif").read_text()) report = read_bandit_sarif(sarif_path) - assert report["results"] == [] + assert report == {"results": [], "errors": [], "files_read": 1} def test_read_bandit_sarif_falls_back_to_level_mapping(tmp_path: Path) -> None: + sarif_path = tmp_path / "results.sarif" + result = {"ruleId": "B999", "level": "warning", "message": {"text": "test issue"}} + sarif_path.write_text(_bandit_sarif(results=(result,))) + report = read_bandit_sarif(sarif_path) + assert report["results"][0]["issue_severity"] == "MEDIUM" + + +def test_read_bandit_sarif_lists_skipped_files(tmp_path: Path) -> None: sarif_path = tmp_path / "results.sarif" sarif_path.write_text( - json.dumps( - { - "version": "2.1.0", - "runs": [ - { - "results": [ - { - "ruleId": "B999", - "level": "warning", - "message": {"text": "test issue"}, - "locations": [], - "properties": {}, - } - ] - } - ], - } + _bandit_sarif( + files=("./src/app.py", "./src/new syntax.py"), + skipped=(("src/new%20syntax.py", "syntax error while parsing AST from file"),), ) ) report = read_bandit_sarif(sarif_path) - assert report["results"][0]["issue_severity"] == "MEDIUM" + assert report["errors"] == [ + {"filename": "src/new syntax.py", "reason": "syntax error while parsing AST from file"} + ] + assert report["files_read"] == 2 + + +def test_read_bandit_sarif_tolerates_missing_bandit_details(tmp_path: Path) -> None: + """Metrics and invocations are bandit details a release may drop: not a failure.""" + sarif_path = tmp_path / "results.sarif" + sarif_path.write_text(json.dumps({"runs": [{"results": [{"ruleId": "B602"}]}]})) + report = read_bandit_sarif(sarif_path) + assert [r["test_id"] for r in report["results"]] == ["B602"] + assert report["errors"] == [] + assert report["files_read"] is None # unknown + + +# --------------------------------------------------------------------------- +# run_bandit +# --------------------------------------------------------------------------- + + +def _fake_bandit(sarif: str | None, returncode: int = 0, stderr: str = "") -> Any: + """subprocess.run stand-in: bandit writes `sarif` (if any) to its -o path.""" + + def run(cmd: list[str], **kwargs: object) -> MagicMock: + if sarif is not None: + Path(cmd[cmd.index("-o") + 1]).write_text(sarif) + return MagicMock(returncode=returncode, stderr=stderr, stdout="") + + return run + + +def test_run_bandit_builds_command_from_workspace_root( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Comma-separated dirs become separate targets, relative to the repository root.""" + (tmp_path / "proj/src").mkdir(parents=True) + (tmp_path / "proj/scripts").mkdir() + monkeypatch.chdir(tmp_path / "proj") # working_directory + sarif_path = tmp_path / "results.sarif" + monkeypatch.setenv("GITHUB_WORKSPACE", str(tmp_path)) + monkeypatch.setenv("BANDIT_SCAN_DIRS", "src/, scripts,.") + monkeypatch.setenv("BANDIT_SARIF_PATH", str(sarif_path)) + + with ( + patch("python_security_auditing.runners.shutil.which", side_effect=lambda exe: exe), + patch( + "python_security_auditing.runners.subprocess.run", + side_effect=_fake_bandit(_bandit_sarif()), + ) as mock_run, + ): + run_bandit(Settings()) + + cmd = mock_run.call_args[0][0] + assert cmd == [ + "bandit", + "-r", + "proj/src", + "proj/scripts", + "proj", + "-f", + "sarif", + "-o", + str(sarif_path.resolve()), + "--exit-zero", + ] + assert mock_run.call_args.kwargs["cwd"] == tmp_path.resolve() + + +def test_run_bandit_defaults_to_the_working_directory( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.chdir(tmp_path) + with ( + patch("python_security_auditing.runners.shutil.which", side_effect=lambda exe: exe), + patch( + "python_security_auditing.runners.subprocess.run", + side_effect=_fake_bandit(_bandit_sarif()), + ) as mock_run, + ): + run_bandit(Settings()) + assert mock_run.call_args[0][0][1:3] == ["-r", "."] + assert mock_run.call_args.kwargs["cwd"] == tmp_path.resolve() + + +@pytest.mark.parametrize( + "sarif", + [ + (FIXTURES / "bandit_issues.sarif").read_text(), + # no metrics: the file count is unknown, so the exit code and the SARIF decide + json.dumps({"runs": [{"results": [{"ruleId": "B404"}, {"ruleId": "B602"}]}]}), + ], +) +def test_run_bandit_returns_findings( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, sarif: str +) -> None: + """With --exit-zero, bandit exits 0 when it finds issues; the SARIF is kept for upload.""" + monkeypatch.chdir(tmp_path) + with ( + patch("python_security_auditing.runners.shutil.which", side_effect=lambda exe: exe), + patch("python_security_auditing.runners.subprocess.run", side_effect=_fake_bandit(sarif)), + ): + report = run_bandit(Settings()) + assert [r["test_id"] for r in report["results"]] == ["B404", "B602"] + assert (tmp_path / "results.sarif").read_text() == sarif + + +@pytest.mark.parametrize( + ("sarif", "returncode", "stderr", "message"), + [ + # usage error: argparse has already created an empty -o file + ("", 2, "bandit: error: unrecognized arguments", r"exit 2\):\nbandit: error: unrec"), + # crash: an uncaught exception exits 1 and writes no report + (None, 1, "Traceback (most recent call last):", r"exit 1\):\nTraceback"), + (None, 0, "", "bandit wrote no valid SARIF report"), + ("not json", 0, "", "bandit wrote no valid SARIF report"), + (_bandit_sarif(files=()), 0, "", r"no Python file to scan in \['.'\]"), + # a skipped file would drop its findings, and close its Code Scanning alerts + ( + _bandit_sarif( + files=("./app.py", "./new.py"), + skipped=(("new.py", "syntax error while parsing AST from file"),), + ), + 0, + "", + r"could not scan 1 file\(s\), so they were NOT checked:\n" + r"new.py: syntax error while parsing AST from file\n.*`exclude`.*`.bandit`", + ), + ], +) +def test_run_bandit_fails_closed( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + sarif: str | None, + returncode: int, + stderr: str, + message: str, +) -> None: + """A failed, partial or empty scan raises, and leaves no SARIF to upload.""" + monkeypatch.chdir(tmp_path) + (tmp_path / "results.sarif").write_text(_bandit_sarif()) # an earlier run's report + with ( + patch("python_security_auditing.runners.shutil.which", side_effect=lambda exe: exe), + patch( + "python_security_auditing.runners.subprocess.run", + side_effect=_fake_bandit(sarif, returncode, stderr), + ), + ): + with pytest.raises(AuditError, match=message): + run_bandit(Settings()) + assert not (tmp_path / "results.sarif").exists() + + +def test_run_bandit_caps_the_skipped_file_list( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The list ends up in the PR comment, which GitHub caps at 65,536 characters.""" + monkeypatch.chdir(tmp_path) + skipped = tuple((f"f{i}.py", "syntax error while parsing AST from file") for i in range(25)) + sarif = _bandit_sarif(files=tuple(f"./f{i}.py" for i in range(25)), skipped=skipped) + with ( + patch("python_security_auditing.runners.shutil.which", side_effect=lambda exe: exe), + patch("python_security_auditing.runners.subprocess.run", side_effect=_fake_bandit(sarif)), + ): + with pytest.raises(AuditError) as exc_info: + run_bandit(Settings()) + message = str(exc_info.value) + assert "could not scan 25 file(s)" in message + assert "f19.py:" in message + assert "f20.py:" not in message + assert "… and 5 more" in message + + +def test_run_bandit_rejects_missing_scan_dirs( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A misspelt dir next to a valid one would otherwise go unscanned with a warning.""" + monkeypatch.chdir(tmp_path) + (tmp_path / "src").mkdir() + (tmp_path / "results.sarif").write_text(_bandit_sarif()) # an earlier run's report + monkeypatch.setenv("BANDIT_SCAN_DIRS", "src, scirpts,gone/") + with patch("python_security_auditing.runners.subprocess.run") as mock_run: + with pytest.raises(AuditError, match="bandit_scan_dirs not found in .*: scirpts, gone/"): + run_bandit(Settings()) + mock_run.assert_not_called() + assert not (tmp_path / "results.sarif").exists() + + +def test_run_bandit_wraps_os_errors(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """An OSError must not crash the run before pip-audit and the summary.""" + monkeypatch.chdir(tmp_path) + with ( + patch("python_security_auditing.runners.shutil.which", side_effect=lambda exe: exe), + patch( + "python_security_auditing.runners.subprocess.run", + side_effect=PermissionError(13, "Permission denied"), + ), + ): + with pytest.raises(AuditError, match="bandit could not run: .*Permission denied"): + run_bandit(Settings()) # --------------------------------------------------------------------------- diff --git a/uv.lock b/uv.lock index 7010546..4dd2047 100644 --- a/uv.lock +++ b/uv.lock @@ -79,6 +79,36 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/f1/f4/b54123680025c0b7253117418f023d1b2487f1102552acbdd9d8ee96b622/ast_serialize-0.12.1-cp39-abi3-win_arm64.whl", hash = "sha256:610a41351de68199de9a1434499083b4256c0df7658ec1cfc0a0a7b20b08d317", size = 1136560, upload-time = "2026-10-03T12:24:58.689Z" }, ] +[[package]] +name = "attrs" +version = "26.1.0" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/9a/8e/82a0fe20a541c03148528be8cac2408564a6c9a0cc7e9171802bc1d26985/attrs-26.1.0.tar.gz", hash = "sha256:d03ceb89cb322a8fd706d4fb91940737b6642aa36998fe130a9bc96c985eff32", size = 952055, upload-time = "2026-03-19T14:22:25.026Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/64/b4/17d4b0b2a2dc85a6df63d1157e028ed19f90d4cd97c36717afef2bc2f395/attrs-26.1.0-py3-none-any.whl", hash = "sha256:c647aa4a12dfbad9333ca4e71fe62ddc36f4e63b2d260a37a8b83d2f043ac309", size = 67548, upload-time = "2026-03-19T14:22:23.645Z" }, +] + +[[package]] +name = "bandit" +version = "1.9.4" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "colorama", marker = "sys_platform == 'win32'" }, + { name = "pyyaml" }, + { name = "rich" }, + { name = "stevedore" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/aa/c3/0cb80dfe0f3076e5da7e4c5ad8e57bac6ac357ff4a6406205501cade4965/bandit-1.9.4.tar.gz", hash = "sha256:b589e5de2afe70bd4d53fa0c1da6199f4085af666fde00e8a034f152a52cd628", size = 4242677, upload-time = "2026-02-25T06:44:15.503Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/05/a4/a26d5b25671d27e03afb5401a0be5899d94ff8fab6a698b1ac5be3ec29ef/bandit-1.9.4-py3-none-any.whl", hash = "sha256:f89ffa663767f5a0585ea075f01020207e966a9c0f2b9ef56a57c7963a3f6f8e", size = 134741, upload-time = "2026-02-25T06:44:13.694Z" }, +] + +[package.optional-dependencies] +sarif = [ + { name = "jschema-to-python" }, + { name = "sarif-om" }, +] + [[package]] name = "boolean-py" version = "5.0" @@ -290,6 +320,29 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/cb/b1/3846dd7f199d53cb17f49cba7e651e9ce294d8497c8c150530ed11865bb8/iniconfig-2.3.0-py3-none-any.whl", hash = "sha256:f631c04d2c48c52b84d0d0549c99ff3859c98df65b3101406327ecc7d53fbf12", size = 7484, upload-time = "2025-10-18T21:55:41.639Z" }, ] +[[package]] +name = "jschema-to-python" +version = "1.2.3" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "attrs" }, + { name = "jsonpickle" }, + { name = "pbr" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/1d/7f/5ae3d97ddd86ec33323231d68453afd504041efcfd4f4dde993196606849/jschema_to_python-1.2.3.tar.gz", hash = "sha256:76ff14fe5d304708ccad1284e4b11f96a658949a31ee7faed9e0995279549b91", size = 10061, upload-time = "2019-10-05T20:02:39.657Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/31/9e/1b6819a87c3f59170406163ba17bc55b0abe18ae552f53d2b0a2025f9c63/jschema_to_python-1.2.3-py3-none-any.whl", hash = "sha256:8a703ca7604d42d74b2815eecf99a33359a8dccbb80806cce386d5e2dd992b05", size = 10400, upload-time = "2019-10-05T20:02:37.948Z" }, +] + +[[package]] +name = "jsonpickle" +version = "4.1.3" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/d7/65/8b589546fe473fd262aebdfead994c7ba237caaa8fa4ca8855d0888933b7/jsonpickle-4.1.3.tar.gz", hash = "sha256:de234c1d1ed2c5313833e608e2a2467202f48ae24ea7a2afa2d238d8999aa4b5", size = 317678, upload-time = "2026-10-02T15:47:07.496Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/00/8e/20ccb39e89f350a11bfb3e43728bdb4614fd9f3e69058036218e403566e8/jsonpickle-4.1.3-py3-none-any.whl", hash = "sha256:99822e77b593462c1f67361b0571e54323db43f1285706a63da5b5759714c9df", size = 49180, upload-time = "2026-10-02T15:47:06.189Z" }, +] + [[package]] name = "librt" version = "0.16.0" @@ -559,6 +612,18 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/f1/d9/7fb5aa316bc299258e68c73ba3bddbc499654a07f151cba08f6153988714/pathspec-1.1.1-py3-none-any.whl", hash = "sha256:a00ce642f577bf7f473932318056212bc4f8bfdf53128c78bbd5af0b9b20b189", size = 57328, upload-time = "2026-04-27T01:46:07.06Z" }, ] +[[package]] +name = "pbr" +version = "7.0.3" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "setuptools" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/5e/ab/1de9a4f730edde1bdbbc2b8d19f8fa326f036b4f18b2f72cfbea7dc53c26/pbr-7.0.3.tar.gz", hash = "sha256:b46004ec30a5324672683ec848aed9e8fc500b0d261d40a3229c2d2bbfcedc29", size = 135625, upload-time = "2025-11-03T17:04:56.274Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/c0/db/61efa0d08a99f897ef98256b03e563092d36cc38dc4ebe4a85020fe40b31/pbr-7.0.3-py2.py3-none-any.whl", hash = "sha256:ff223894eb1cd271a98076b13d3badff3bb36c424074d26334cd25aebeecea6b", size = 131898, upload-time = "2025-11-03T17:04:54.875Z" }, +] + [[package]] name = "pip" version = "26.2.1" @@ -786,9 +851,10 @@ wheels = [ [[package]] name = "python-security-auditing" -version = "0.6.0" +version = "0.6.1" source = { editable = "." } dependencies = [ + { name = "bandit", extra = ["sarif"] }, { name = "pip-audit" }, { name = "pydantic-settings" }, ] @@ -806,6 +872,7 @@ dev = [ [package.metadata] requires-dist = [ + { name = "bandit", extras = ["sarif"], specifier = ">=1.9.4" }, { name = "mypy", marker = "extra == 'dev'", specifier = ">=1.0" }, { name = "pip-audit", specifier = ">=2.7" }, { name = "pydantic-settings", specifier = ">=2.14.2" }, @@ -907,6 +974,28 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/51/60/5fb1a39dbb5ae314d5f59bc7348a63c1d5c20f3cd83914c4b5cb0be31d2d/ruff-0.16.9-py3-none-win_arm64.whl", hash = "sha256:ed1a252039200f57a59eebc063b54beabea67bfbaaca0eeaa7f54b5fbcda2284", size = 10458649, upload-time = "2026-09-24T20:37:46.882Z" }, ] +[[package]] +name = "sarif-om" +version = "1.0.4" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "attrs" }, + { name = "pbr" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/ba/de/bbdd93fe456d4011500784657c5e4a31e3f4fcbb276255d4db1213aed78c/sarif_om-1.0.4.tar.gz", hash = "sha256:cd5f416b3083e00d402a92e449a7ff67af46f11241073eea0461802a3b5aef98", size = 28847, upload-time = "2019-10-05T20:11:23.338Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/82/7c/1d3d0467565aa8b3e77ab8712042a09dd1158056826f45783f3d2b34adf1/sarif_om-1.0.4-py3-none-any.whl", hash = "sha256:539ef47a662329b1c8502388ad92457425e95dc0aaaf995fe46f4984c4771911", size = 30193, upload-time = "2019-10-05T20:11:21.577Z" }, +] + +[[package]] +name = "setuptools" +version = "84.0.0" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/6d/44/f5da03a8ef95d369145c5bb53050e7877c9f3d312e128605fd9504829143/setuptools-84.0.0.tar.gz", hash = "sha256:f4695c21257f0d9b537ec2692c941d02ee143b7cc1276941349a546573b2ef73", size = 1168449, upload-time = "2026-08-08T18:27:58.365Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/95/9c/c510029fc6ef33a6275cd2c5d3cecd6613dfd6aa401d57c54f1c18852ccf/setuptools-84.0.0-py3-none-any.whl", hash = "sha256:51a52592b3b99e102b609654876bd65f19f999935166d1352678931132b0c670", size = 818216, upload-time = "2026-08-08T18:27:56.719Z" }, +] + [[package]] name = "sortedcontainers" version = "2.4.0" @@ -916,6 +1005,15 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/32/46/9cb0e58b2deb7f82b84065f37f3bffeb12413f947f9388e4cac22c4621ce/sortedcontainers-2.4.0-py2.py3-none-any.whl", hash = "sha256:a163dcaede0f1c021485e957a39245190e74249897e2ae4b2aa38595db237ee0", size = 29575, upload-time = "2021-05-16T22:03:41.177Z" }, ] +[[package]] +name = "stevedore" +version = "5.9.1" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/db/a1/3b8ed9c1fc3aa6eebb57732d924ddaa0500ecc3b638d0454816320994383/stevedore-5.9.1.tar.gz", hash = "sha256:e97a2667923efda926e8713fde6a73616df68210a3cbc6f02b48967b676fd8bf", size = 518111, upload-time = "2026-08-20T15:25:14.754Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/c5/97/bba6e7ec2f5498b9dcb7b1b6400086b80ae5a8ebaff4b25e8c8add75f439/stevedore-5.9.1-py3-none-any.whl", hash = "sha256:5c8ff3a9f336cc1a06ac0f597bc79d11a2f950bfd32e290ca56b5a301fafafbf", size = 54931, upload-time = "2026-08-20T15:25:13.602Z" }, +] + [[package]] name = "tomli" version = "2.4.1" From 746dfaa2924c41667a3e6493e247cbf52705cce4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Houpert?= <10154151+lhoupert@users.noreply.github.com> Date: Tue, 6 Oct 2026 16:53:00 +0100 Subject: [PATCH 2/8] test: isolate the unit tests from the CI environment Settings reads GITHUB_STEP_SUMMARY, GITHUB_WORKSPACE, RUNNER_DEBUG and the action inputs from the environment, and CI sets several of them. The tests then wrote to the real job summary (1,945 bytes in a local repro) and ran with debug output. Clear every variable Settings reads, plus PIPENV_PIPFILE. Co-Authored-By: Claude Opus 5.5 (1M context) --- tests/conftest.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index d931456..f6685d4 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,7 +1,15 @@ +import os + import pytest +from python_security_auditing.settings import Settings + +# Settings reads these case-insensitively; runners.py also reads PIPENV_PIPFILE. +_READ_FROM_ENV = {*Settings.model_fields, "pipenv_pipfile"} @pytest.fixture(autouse=True) -def _no_github_workspace(monkeypatch: pytest.MonkeyPatch) -> None: - """CI sets GITHUB_WORKSPACE; tests that need it set it themselves.""" - monkeypatch.delenv("GITHUB_WORKSPACE", raising=False) +def _isolated_env(monkeypatch: pytest.MonkeyPatch) -> None: + """CI sets GITHUB_STEP_SUMMARY, GITHUB_WORKSPACE, RUNNER_DEBUG...: tests set what they need.""" + for name in list(os.environ): + if name.lower() in _READ_FROM_ENV: + monkeypatch.delenv(name) From c11ed2fa001b990d432d2f6826480516dc4a33b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Houpert?= <10154151+lhoupert@users.noreply.github.com> Date: Tue, 6 Oct 2026 16:55:47 +0100 Subject: [PATCH 3/8] fix: keep bandit findings when some files cannot be scanned A file bandit could not parse or read used to discard the whole report and say "bandit did NOT run", hiding the other files' findings. What happens now: - The findings are kept in the summary, the annotations and the PR comment. - The skipped files are a blocking error that lists them (at most 20). - The SARIF is still deleted, because a partial report would close those files' Code Scanning alerts. The message says so. - The "did NOT run" path is now only for a crash, a usage error, or a missing or malformed SARIF. The message also explains how to exclude files on purpose. A `.bandit` `exclude` list replaces bandit's default excludes, so the example repeats them. It uses `*/name/*` globs because bandit compares the patterns with paths such as `./.venv/...` when it scans `.`, and plain names like `.venv` then do not match: checked with bandit 1.9.4 and this invocation. When a skipped file is under a virtual environment or node_modules, the message says so. The bandit run itself is harder to break: - bandit runs as `sys.executable -m bandit`, so it is always the bandit of the action's environment. - Overlapping scan dirs (for example `src,.`) are scanned once: repeated and nested dirs are dropped, otherwise bandit reports their files twice. - bandit's stderr is capped at its last 2,000 characters. - Any malformed SARIF value is an AuditError, instead of a TypeError that crashed the run before pip-audit and left the file to be uploaded. - When the file count is unknown, the summary now says so too, not only an annotation. - The dead FileNotFoundError catch around run_bandit is removed. The pip-audit one stays: _resolve_exe still raises it there. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/python_security_auditing/__main__.py | 2 +- src/python_security_auditing/annotations.py | 5 +- src/python_security_auditing/report.py | 26 ++++ src/python_security_auditing/runners.py | 102 ++++++------- tests/test_annotations.py | 10 ++ tests/test_main.py | 39 ++++- tests/test_report.py | 50 ++++++- tests/test_runners.py | 155 ++++++++++++-------- 8 files changed, 273 insertions(+), 116 deletions(-) diff --git a/src/python_security_auditing/__main__.py b/src/python_security_auditing/__main__.py index 1283d64..04ed765 100644 --- a/src/python_security_auditing/__main__.py +++ b/src/python_security_auditing/__main__.py @@ -32,7 +32,7 @@ def main() -> None: if "bandit" in settings.enabled_tools: try: bandit_report = run_bandit(settings) - except (AuditError, FileNotFoundError) as exc: + except AuditError as exc: bandit_error = str(exc) if settings.debug: print( diff --git a/src/python_security_auditing/annotations.py b/src/python_security_auditing/annotations.py index 501ee2f..7a09ecd 100644 --- a/src/python_security_auditing/annotations.py +++ b/src/python_security_auditing/annotations.py @@ -4,6 +4,7 @@ from typing import Any +from .report import bandit_skipped_message from .settings import Settings _SEVERITY_TO_LEVEL: dict[str, str] = { @@ -40,7 +41,9 @@ def emit_annotations( if "bandit" in settings.enabled_tools: if bandit_error: print(f"::error::bandit did NOT run: {_escape(bandit_error)}") - elif bandit_report.get("files_read", 0) is None: + if bandit_report.get("errors"): + print(f"::error::{_escape(bandit_skipped_message(bandit_report['errors']))}") + if bandit_report.get("files_read", 0) is None: print( "::warning::bandit reported no file metrics, so the scanned file count is unknown" ) diff --git a/src/python_security_auditing/report.py b/src/python_security_auditing/report.py index 0525ad6..61fd5ff 100644 --- a/src/python_security_auditing/report.py +++ b/src/python_security_auditing/report.py @@ -2,11 +2,30 @@ from __future__ import annotations +from pathlib import PurePath from typing import Any from .settings import Settings _SEVERITY_ICON = {"HIGH": "🔴", "MEDIUM": "🟡", "LOW": "🟢"} +_ENV_DIRS = {".venv", "venv", "site-packages", "node_modules"} + + +def bandit_skipped_message(skipped: list[dict[str, Any]]) -> str: + """Explain which files bandit could not scan; list at most 20 (the PR comment is capped).""" + lines = [f"{e['filename']}: {e['reason']}" for e in skipped[:20]] + if len(skipped) > 20: + lines.append(f"… and {len(skipped) - 20} more") + if any(_ENV_DIRS.intersection(PurePath(e["filename"]).parts) for e in skipped): + lines.append("Some are in a virtual environment or node_modules inside bandit_scan_dirs.") + return ( + f"bandit could not scan {len(skipped)} file(s), so their findings are missing and the " + "results were NOT uploaded to Code Scanning:\n" + + "\n".join(lines) + + "\nTo skip files on purpose, list them under `exclude` in your `.bandit` file. That " + "list replaces bandit's default excludes, so repeat them, for example: " + "`exclude = */.git/*,*/__pycache__/*,*/.tox/*,*/.eggs/*,*.egg,*/.venv/*,path/to/file.py`" + ) def build_markdown( @@ -63,6 +82,11 @@ def _bandit_section(report: dict[str, Any], settings: Settings, error: str) -> s lines.append(f"❌ bandit did NOT run, so the code was NOT scanned.\n\n```\n{error}\n```\n") return "\n".join(lines) + if skipped := report.get("errors", []): + lines.append(f"❌ The scan is incomplete.\n\n```\n{bandit_skipped_message(skipped)}\n```\n") + if report.get("files_read", 0) is None: + lines.append("⚠️ Number of files scanned unknown: bandit reported no file metrics.\n") + if not results: lines.append("✅ No issues found.\n") return "\n".join(lines) @@ -167,6 +191,8 @@ def check_thresholds( ) -> bool: """Return True if any blocking issues were found.""" if "bandit" in settings.enabled_tools: + if bandit_report.get("errors"): # files bandit could not scan + return True for result in bandit_report.get("results", []): if result.get("issue_severity") in settings.blocking_severities: return True diff --git a/src/python_security_auditing/runners.py b/src/python_security_auditing/runners.py index bef237a..290fc47 100644 --- a/src/python_security_auditing/runners.py +++ b/src/python_security_auditing/runners.py @@ -155,7 +155,7 @@ def read_bandit_sarif(sarif_path: Path) -> dict[str, Any]: "errors" lists the files bandit skipped. "files_read" counts the files bandit read, or is None when the report has no per-file metrics. Raises AuditError when the report is - missing or is not SARIF. + missing or malformed. """ try: run: dict[str, Any] = json.loads(sarif_path.read_text())["runs"][0] @@ -167,62 +167,68 @@ def read_bandit_sarif(sarif_path: Path) -> dict[str, Any]: uri = phys.get("artifactLocation", {}).get("uri", "") errors.append({"filename": unquote(uri), "reason": notice["message"]["text"]}) metrics = run.get("properties", {}).get("metrics") + files_read = None if metrics is None else len(set(metrics) - {"_totals"}) + + results: list[dict[str, Any]] = [] + for sarif_result in run.get("results", []): + props: dict[str, Any] = sarif_result.get("properties", {}) + severity = props.get("issue_severity") or _SARIF_LEVEL_TO_SEVERITY.get( + sarif_result.get("level", "none"), "LOW" + ) + locations: list[dict[str, Any]] = sarif_result.get("locations", []) + filename = "" + line_number = 0 + if locations: + phys = locations[0].get("physicalLocation", {}) + filename = unquote(phys.get("artifactLocation", {}).get("uri", "")) # %-encoded + line_number = phys.get("region", {}).get("startLine", 0) + results.append( + { + "issue_severity": severity, + "issue_confidence": props.get("issue_confidence", ""), + "issue_text": sarif_result.get("message", {}).get("text", ""), + "filename": filename, + "line_number": line_number, + "test_id": sarif_result.get("ruleId", ""), + } + ) except (OSError, ValueError, LookupError, TypeError, AttributeError) as exc: raise AuditError(f"bandit wrote no valid SARIF report to {sarif_path}: {exc!r}") from exc - results: list[dict[str, Any]] = [] - for sarif_result in run.get("results", []): - props: dict[str, Any] = sarif_result.get("properties", {}) - severity = props.get("issue_severity") or _SARIF_LEVEL_TO_SEVERITY.get( - sarif_result.get("level", "none"), "LOW" - ) - locations: list[dict[str, Any]] = sarif_result.get("locations", []) - filename = "" - line_number = 0 - if locations: - phys = locations[0].get("physicalLocation", {}) - filename = unquote(phys.get("artifactLocation", {}).get("uri", "")) # bandit %-encodes - line_number = phys.get("region", {}).get("startLine", 0) - results.append( - { - "issue_severity": severity, - "issue_confidence": props.get("issue_confidence", ""), - "issue_text": sarif_result.get("message", {}).get("text", ""), - "filename": filename, - "line_number": line_number, - "test_id": sarif_result.get("ruleId", ""), - } - ) - - files_read = None if metrics is None else len(set(metrics) - {"_totals"}) return {"results": results, "errors": errors, "files_read": files_read} def run_bandit(settings: Settings) -> dict[str, Any]: """Run bandit on bandit_scan_dirs, write its SARIF report, return the parsed report. - Raises AuditError, and leaves no SARIF report, when bandit cannot run, fails, skips a - file or reads none: an empty or partial report would close open Code Scanning alerts. + Raises AuditError when bandit cannot run, fails or reads no file. The SARIF report is + kept only when bandit scanned every file: an empty or partial report would close open + Code Scanning alerts. """ sarif_path = Path(settings.bandit_sarif_path).resolve() + complete = False try: sarif_path.unlink(missing_ok=True) # never upload an earlier run's report dirs = [d.strip() for d in settings.bandit_scan_dirs.split(",") if d.strip()] missing = [d for d in dirs if not Path(d).exists()] if missing: raise AuditError(f"bandit_scan_dirs not found in {Path.cwd()}: {', '.join(missing)}") + # bandit reports the files of overlapping dirs twice: drop repeated and nested dirs. + paths = list(dict.fromkeys(Path(d).resolve() for d in dirs)) + paths = [p for p in paths if not any(p != q and p.is_relative_to(q) for q in paths)] # Run from the repository root, so that SARIF paths (annotations, Code Scanning) # stay relative to it whatever the working directory. root = Path(settings.github_workspace or ".").resolve() - targets = [os.path.relpath(Path(d).resolve(), root) for d in dirs] - # With --exit-zero findings exit 0 too, so any other exit is a crash or a usage error. - cmd = [_resolve_exe("bandit"), "-r", *targets, "-f", "sarif", "-o", str(sarif_path)] - cmd += ["--exit-zero"] + targets = [os.path.relpath(p, root) for p in paths] + # The bandit of this environment, not one on PATH. With --exit-zero, findings exit 0 + # too, so any other exit is a crash or a usage error. + cmd = [sys.executable, "-m", "bandit", "-r", *targets] + cmd += ["-f", "sarif", "-o", str(sarif_path), "--exit-zero"] if settings.debug: print(f"[debug] bandit command (cwd={root}): {cmd}", file=sys.stderr) - result = subprocess.run(cmd, cwd=root, capture_output=True, text=True) # nosec B603 -- list args, full path via _resolve_exe() + result = subprocess.run(cmd, cwd=root, capture_output=True, text=True) # nosec B603 -- list args, sys.executable if settings.debug: print( @@ -230,27 +236,21 @@ def run_bandit(settings: Settings) -> dict[str, Any]: ) if result.returncode: - raise AuditError(f"bandit failed (exit {result.returncode}):\n{result.stderr.strip()}") + stderr = result.stderr.strip() + if len(stderr) > 2000: # it ends up in the PR comment, capped at 65,536 characters + stderr = "… (truncated)\n" + stderr[-2000:] + raise AuditError(f"bandit failed (exit {result.returncode}):\n{stderr}") report = read_bandit_sarif(sarif_path) - if skipped := [f"{e['filename']}: {e['reason']}" for e in report["errors"]]: - count = len(skipped) - if count > 20: # the list ends up in the PR comment, which GitHub caps at 65,536 chars - skipped[20:] = [f"… and {count - 20} more"] - raise AuditError( - f"bandit could not scan {count} file(s), so they were NOT checked:\n" - + "\n".join(skipped) - + "\nFix them, or skip them on purpose: list them under `exclude` in the [bandit] " - "section of a `.bandit` file in a scanned directory." - ) - if report["files_read"] == 0: + if report["files_read"] == 0 and not report["errors"]: raise AuditError(f"bandit found no Python file to scan in {targets}") - except (AuditError, OSError) as exc: - with contextlib.suppress(OSError): - sarif_path.unlink(missing_ok=True) - if isinstance(exc, AuditError): - raise + complete = not report["errors"] + return report + except OSError as exc: raise AuditError(f"bandit could not run: {exc}") from exc - return report + finally: + if not complete: + with contextlib.suppress(OSError): + sarif_path.unlink(missing_ok=True) def run_pip_audit( diff --git a/tests/test_annotations.py b/tests/test_annotations.py index ffe1443..6de5e14 100644 --- a/tests/test_annotations.py +++ b/tests/test_annotations.py @@ -147,6 +147,16 @@ def test_bandit_without_file_metrics_warns( assert capsys.readouterr().out.startswith("::warning::bandit reported no file metrics") +def test_bandit_skipped_files_emit_error( + pip_clean: list[Any], capsys: pytest.CaptureFixture[str] +) -> None: + report = {"results": [], "errors": [{"filename": "src/new.py", "reason": "syntax error"}]} + emit_annotations(report, pip_clean, Settings()) + out = capsys.readouterr().out + assert out.startswith("::error::bandit could not scan 1 file(s)") + assert "%0Asrc/new.py: syntax error%0A" in out + + 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 2066894..4f3e51d 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -2,6 +2,7 @@ from __future__ import annotations +import json import subprocess from pathlib import Path from typing import Any @@ -20,7 +21,7 @@ def _fake_tools(bandit_sarif: str, uv_error: Exception | None = None) -> Any: """subprocess.run stand-in: bandit writes a fixture SARIF; uv export and pip-audit are clean.""" def run(cmd: list[str], **kwargs: object) -> MagicMock: - if cmd[0] == "bandit": + if cmd[1:3] == ["-m", "bandit"]: Path(cmd[cmd.index("-o") + 1]).write_text((FIXTURES / bandit_sarif).read_text()) elif cmd[0] == "uv" and uv_error: raise uv_error @@ -179,7 +180,7 @@ def test_main_reports_pip_audit_when_bandit_cannot_run( monkeypatch.chdir(tmp_path) def run(cmd: list[str], **kwargs: object) -> MagicMock: - if cmd[0] == "bandit": + if cmd[1:3] == ["-m", "bandit"]: return MagicMock(returncode=2, stderr="ERROR\tMultiple .bandit files found", stdout="") return MagicMock(returncode=0, stderr="", stdout=CLEAN_REPORT) @@ -203,3 +204,37 @@ def run(cmd: list[str], **kwargs: object) -> MagicMock: assert "::error::bandit did NOT run: bandit failed (exit 2):%0AERROR" in out assert (tmp_path / "pip-audit-report.json").exists() assert not (tmp_path / "results.sarif").exists() + + +def test_main_fails_but_reports_findings_when_bandit_skips_files( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """Files bandit could not scan block the job; the other files' findings are still shown.""" + summary_path = tmp_path / "summary.md" + monkeypatch.setenv("TOOLS", "bandit") + monkeypatch.setenv("GITHUB_STEP_SUMMARY", str(summary_path)) + monkeypatch.chdir(tmp_path) + sarif = json.loads((FIXTURES / "bandit_issues.sarif").read_text()) + notice = { + "message": {"text": "syntax error while parsing AST from file"}, + "locations": [{"physicalLocation": {"artifactLocation": {"uri": "src/new.py"}}}], + } + sarif["runs"][0]["invocations"][0]["toolConfigurationNotifications"] = [notice] + + def run(cmd: list[str], **kwargs: object) -> MagicMock: + Path(cmd[cmd.index("-o") + 1]).write_text(json.dumps(sarif)) + return MagicMock(returncode=0, stderr="", stdout="") + + with patch("python_security_auditing.runners.subprocess.run", side_effect=run): + with pytest.raises(SystemExit) as exc_info: + main() + + assert exc_info.value.code == 1 + summary = summary_path.read_text() + assert "bandit could not scan 1 file(s)" in summary + assert "B404" in summary + assert "did NOT run" not in summary + out = capsys.readouterr().out + assert "::error::bandit could not scan 1 file(s)" in out + assert "::error file=src/app.py,line=2::[B404]" in out + assert not (tmp_path / "results.sarif").exists() diff --git a/tests/test_report.py b/tests/test_report.py index 00f89d8..863ba65 100644 --- a/tests/test_report.py +++ b/tests/test_report.py @@ -7,7 +7,12 @@ from typing import Any, cast import pytest -from python_security_auditing.report import build_markdown, check_thresholds, write_step_summary +from python_security_auditing.report import ( + bandit_skipped_message, + build_markdown, + check_thresholds, + write_step_summary, +) from python_security_auditing.settings import Settings FIXTURES = Path(__file__).parent / "fixtures" @@ -228,6 +233,49 @@ def test_markdown_bandit_error(pip_fixable: list[Any]) -> None: assert "Blocking issues found" in md +SKIPPED = [{"filename": "src/new.py", "reason": "syntax error while parsing AST from file"}] + + +def test_markdown_bandit_skipped_files_keep_findings_and_block( + bandit_issues: dict[str, Any], pip_clean: list[Any] +) -> None: + md = build_markdown({**bandit_issues, "errors": SKIPPED}, pip_clean, Settings()) + assert "bandit could not scan 1 file(s)" in md + assert "NOT uploaded to Code Scanning" in md + assert "src/new.py: syntax error while parsing AST from file" in md + assert "B404" in md # the other files' findings are still reported + assert "Blocking issues found" in md + + +def test_bandit_skipped_files_block_below_threshold( + bandit_clean: dict[str, Any], pip_clean: list[Any] +) -> None: + assert check_thresholds({**bandit_clean, "errors": SKIPPED}, pip_clean, Settings()) is True + + +def test_bandit_skipped_message_is_capped_and_points_at_a_venv() -> None: + skipped = [{"filename": f".venv/lib/f{i}.py", "reason": "syntax error"} for i in range(25)] + message = bandit_skipped_message(skipped) + assert "could not scan 25 file(s)" in message + assert ".venv/lib/f19.py: syntax error" in message + assert ".venv/lib/f20.py" not in message + assert "… and 5 more" in message + assert "virtual environment" in message + # bandit's own default excludes are replaced by an `exclude` list, so it repeats them + assert "exclude = */.git/*,*/__pycache__/*,*/.tox/*,*/.eggs/*,*.egg,*/.venv/*," in message + + +def test_bandit_skipped_message_without_venv_has_no_venv_hint() -> None: + assert "virtual environment" not in bandit_skipped_message(SKIPPED) + + +def test_markdown_bandit_unknown_file_count( + bandit_clean: dict[str, Any], pip_clean: list[Any] +) -> None: + md = build_markdown({**bandit_clean, "files_read": None}, pip_clean, Settings()) + assert "⚠️ Number of files scanned unknown" 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 8801991..506a919 100644 --- a/tests/test_runners.py +++ b/tests/test_runners.py @@ -4,6 +4,7 @@ import json import subprocess +import sys import tempfile from pathlib import Path from typing import Any @@ -219,6 +220,23 @@ def test_read_bandit_sarif_tolerates_missing_bandit_details(tmp_path: Path) -> N assert report["files_read"] is None # unknown +@pytest.mark.parametrize( + "run", + [ + {"results": None}, + {"results": [1]}, + {"results": [{"locations": [{"physicalLocation": {"artifactLocation": {"uri": 5}}}]}]}, + {"results": [], "properties": {"metrics": 5}}, + ], +) +def test_read_bandit_sarif_raises_on_malformed_values(tmp_path: Path, run: dict[str, Any]) -> None: + """A malformed report must not crash the run before pip-audit, nor reach Code Scanning.""" + sarif_path = tmp_path / "results.sarif" + sarif_path.write_text(json.dumps({"runs": [run]})) + with pytest.raises(AuditError, match="bandit wrote no valid SARIF report"): + read_bandit_sarif(sarif_path) + + # --------------------------------------------------------------------------- # run_bandit # --------------------------------------------------------------------------- @@ -244,25 +262,23 @@ def test_run_bandit_builds_command_from_workspace_root( monkeypatch.chdir(tmp_path / "proj") # working_directory sarif_path = tmp_path / "results.sarif" monkeypatch.setenv("GITHUB_WORKSPACE", str(tmp_path)) - monkeypatch.setenv("BANDIT_SCAN_DIRS", "src/, scripts,.") + monkeypatch.setenv("BANDIT_SCAN_DIRS", "src/, scripts") monkeypatch.setenv("BANDIT_SARIF_PATH", str(sarif_path)) - with ( - patch("python_security_auditing.runners.shutil.which", side_effect=lambda exe: exe), - patch( - "python_security_auditing.runners.subprocess.run", - side_effect=_fake_bandit(_bandit_sarif()), - ) as mock_run, - ): + with patch( + "python_security_auditing.runners.subprocess.run", + side_effect=_fake_bandit(_bandit_sarif()), + ) as mock_run: run_bandit(Settings()) - cmd = mock_run.call_args[0][0] - assert cmd == [ + # the bandit of the action's own environment, never one found on PATH + assert mock_run.call_args[0][0] == [ + sys.executable, + "-m", "bandit", "-r", "proj/src", "proj/scripts", - "proj", "-f", "sarif", "-o", @@ -272,19 +288,40 @@ def test_run_bandit_builds_command_from_workspace_root( assert mock_run.call_args.kwargs["cwd"] == tmp_path.resolve() +@pytest.mark.parametrize( + ("dirs", "targets"), + [ + ("src, ., ./src/, scripts", ["."]), + ("src/sub, src, scripts, src/", ["src", "scripts"]), + ], +) +def test_run_bandit_scans_overlapping_dirs_once( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, dirs: str, targets: list[str] +) -> None: + """bandit would report the files of overlapping dirs twice, under different paths.""" + (tmp_path / "src/sub").mkdir(parents=True) + (tmp_path / "scripts").mkdir() + monkeypatch.chdir(tmp_path) + monkeypatch.setenv("BANDIT_SCAN_DIRS", dirs) + with patch( + "python_security_auditing.runners.subprocess.run", + side_effect=_fake_bandit(_bandit_sarif()), + ) as mock_run: + run_bandit(Settings()) + cmd = mock_run.call_args[0][0] + assert cmd[4 : cmd.index("-f")] == targets + + def test_run_bandit_defaults_to_the_working_directory( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: monkeypatch.chdir(tmp_path) - with ( - patch("python_security_auditing.runners.shutil.which", side_effect=lambda exe: exe), - patch( - "python_security_auditing.runners.subprocess.run", - side_effect=_fake_bandit(_bandit_sarif()), - ) as mock_run, - ): + with patch( + "python_security_auditing.runners.subprocess.run", + side_effect=_fake_bandit(_bandit_sarif()), + ) as mock_run: run_bandit(Settings()) - assert mock_run.call_args[0][0][1:3] == ["-r", "."] + assert mock_run.call_args[0][0][3:5] == ["-r", "."] assert mock_run.call_args.kwargs["cwd"] == tmp_path.resolve() @@ -301,15 +338,33 @@ def test_run_bandit_returns_findings( ) -> None: """With --exit-zero, bandit exits 0 when it finds issues; the SARIF is kept for upload.""" monkeypatch.chdir(tmp_path) - with ( - patch("python_security_auditing.runners.shutil.which", side_effect=lambda exe: exe), - patch("python_security_auditing.runners.subprocess.run", side_effect=_fake_bandit(sarif)), - ): + with patch("python_security_auditing.runners.subprocess.run", side_effect=_fake_bandit(sarif)): report = run_bandit(Settings()) assert [r["test_id"] for r in report["results"]] == ["B404", "B602"] assert (tmp_path / "results.sarif").read_text() == sarif +def test_run_bandit_keeps_findings_when_files_are_skipped( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The findings of the other files are reported; the partial SARIF is not uploaded.""" + monkeypatch.chdir(tmp_path) + issues = json.loads((FIXTURES / "bandit_issues.sarif").read_text())["runs"][0]["results"] + sarif = _bandit_sarif( + results=tuple(issues), + files=("./src/app.py", "./new.py"), + skipped=(("new.py", "syntax error while parsing AST from file"),), + ) + with patch("python_security_auditing.runners.subprocess.run", side_effect=_fake_bandit(sarif)): + report = run_bandit(Settings()) + assert [r["test_id"] for r in report["results"]] == ["B404", "B602"] + assert report["errors"] == [ + {"filename": "new.py", "reason": "syntax error while parsing AST from file"} + ] + # a partial report would close the skipped file's Code Scanning alerts + assert not (tmp_path / "results.sarif").exists() + + @pytest.mark.parametrize( ("sarif", "returncode", "stderr", "message"), [ @@ -319,18 +374,8 @@ def test_run_bandit_returns_findings( (None, 1, "Traceback (most recent call last):", r"exit 1\):\nTraceback"), (None, 0, "", "bandit wrote no valid SARIF report"), ("not json", 0, "", "bandit wrote no valid SARIF report"), + ('{"runs": [{"results": null}]}', 0, "", "bandit wrote no valid SARIF report"), (_bandit_sarif(files=()), 0, "", r"no Python file to scan in \['.'\]"), - # a skipped file would drop its findings, and close its Code Scanning alerts - ( - _bandit_sarif( - files=("./app.py", "./new.py"), - skipped=(("new.py", "syntax error while parsing AST from file"),), - ), - 0, - "", - r"could not scan 1 file\(s\), so they were NOT checked:\n" - r"new.py: syntax error while parsing AST from file\n.*`exclude`.*`.bandit`", - ), ], ) def test_run_bandit_fails_closed( @@ -341,39 +386,32 @@ def test_run_bandit_fails_closed( stderr: str, message: str, ) -> None: - """A failed, partial or empty scan raises, and leaves no SARIF to upload.""" + """A failed or empty scan raises, and leaves no SARIF to upload.""" monkeypatch.chdir(tmp_path) (tmp_path / "results.sarif").write_text(_bandit_sarif()) # an earlier run's report - with ( - patch("python_security_auditing.runners.shutil.which", side_effect=lambda exe: exe), - patch( - "python_security_auditing.runners.subprocess.run", - side_effect=_fake_bandit(sarif, returncode, stderr), - ), + with patch( + "python_security_auditing.runners.subprocess.run", + side_effect=_fake_bandit(sarif, returncode, stderr), ): with pytest.raises(AuditError, match=message): run_bandit(Settings()) assert not (tmp_path / "results.sarif").exists() -def test_run_bandit_caps_the_skipped_file_list( - tmp_path: Path, monkeypatch: pytest.MonkeyPatch -) -> None: - """The list ends up in the PR comment, which GitHub caps at 65,536 characters.""" +def test_run_bandit_truncates_long_stderr(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """stderr ends up in the PR comment, which GitHub caps at 65,536 characters.""" monkeypatch.chdir(tmp_path) - skipped = tuple((f"f{i}.py", "syntax error while parsing AST from file") for i in range(25)) - sarif = _bandit_sarif(files=tuple(f"./f{i}.py" for i in range(25)), skipped=skipped) - with ( - patch("python_security_auditing.runners.shutil.which", side_effect=lambda exe: exe), - patch("python_security_auditing.runners.subprocess.run", side_effect=_fake_bandit(sarif)), + stderr = "x" * 10_000 + "\nValueError: the real cause" + with patch( + "python_security_auditing.runners.subprocess.run", + side_effect=_fake_bandit(None, 1, stderr), ): with pytest.raises(AuditError) as exc_info: run_bandit(Settings()) message = str(exc_info.value) - assert "could not scan 25 file(s)" in message - assert "f19.py:" in message - assert "f20.py:" not in message - assert "… and 5 more" in message + assert len(message) < 2_100 + assert "… (truncated)" in message + assert message.endswith("ValueError: the real cause") def test_run_bandit_rejects_missing_scan_dirs( @@ -394,12 +432,9 @@ def test_run_bandit_rejects_missing_scan_dirs( def test_run_bandit_wraps_os_errors(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: """An OSError must not crash the run before pip-audit and the summary.""" monkeypatch.chdir(tmp_path) - with ( - patch("python_security_auditing.runners.shutil.which", side_effect=lambda exe: exe), - patch( - "python_security_auditing.runners.subprocess.run", - side_effect=PermissionError(13, "Permission denied"), - ), + with patch( + "python_security_auditing.runners.subprocess.run", + side_effect=PermissionError(13, "Permission denied"), ): with pytest.raises(AuditError, match="bandit could not run: .*Permission denied"): run_bandit(Settings()) From 6db14012631e7b8a4648b2c4ba769a9ba113b809 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Houpert?= <10154151+lhoupert@users.noreply.github.com> Date: Tue, 6 Oct 2026 16:56:14 +0100 Subject: [PATCH 4/8] fix: normalize the tools input and reject unknown tools action.yml uses contains(inputs.tools, 'bandit'), which is case-insensitive, while enabled_tools compared exact strings. With `tools: Bandit`, the action deleted results.sarif and tried the upload, but the module skipped bandit, so the job passed with nothing scanned. Tools are now lowercased and trimmed. An unknown or empty tool list fails with a clear error instead of scanning nothing. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/python_security_auditing/settings.py | 9 +++++++++ tests/test_settings.py | 14 ++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/src/python_security_auditing/settings.py b/src/python_security_auditing/settings.py index 29e21c8..12d6aba 100644 --- a/src/python_security_auditing/settings.py +++ b/src/python_security_auditing/settings.py @@ -102,6 +102,15 @@ def _validate_head_ref(cls, v: str) -> str: github_step_summary: str = "" # Path to step summary file github_workspace: str = "" # Repository root: bandit reports paths relative to it + @field_validator("tools", mode="after") + @classmethod + def _known_tools(cls, v: str) -> str: + # Lowercase, as action.yml's contains(inputs.tools, 'bandit') is case-insensitive. + names = [t.strip().lower() for t in v.split(",") if t.strip()] + if not names or set(names) - {"bandit", "pip-audit"}: + raise ValueError(f"tools must list bandit and/or pip-audit, got: {v!r}") + return ",".join(names) + @property def enabled_tools(self) -> list[str]: return [t.strip() for t in self.tools.split(",") if t.strip()] diff --git a/tests/test_settings.py b/tests/test_settings.py index 15d7425..9ed1bb4 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -34,6 +34,20 @@ def test_enabled_tools_custom(monkeypatch: pytest.MonkeyPatch) -> None: assert s.enabled_tools == ["pip-audit"] +def test_enabled_tools_are_normalized(monkeypatch: pytest.MonkeyPatch) -> None: + """action.yml's contains() is case-insensitive, so `Bandit` must run bandit here too.""" + monkeypatch.setenv("TOOLS", " Bandit , PIP-AUDIT ") + assert Settings().enabled_tools == ["bandit", "pip-audit"] + + +@pytest.mark.parametrize("tools", ["bandit,banditt", "", " , "]) +def test_unknown_or_no_tools_are_rejected(monkeypatch: pytest.MonkeyPatch, tools: str) -> None: + """A typo would otherwise scan nothing and pass.""" + monkeypatch.setenv("TOOLS", tools) + with pytest.raises(ValidationError, match="tools"): + Settings() + + def test_enabled_tools_whitespace(monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.setenv("TOOLS", " bandit , pip-audit ") s = Settings() From 0db16e8270cfb1dd3cdf87882babae8148f961c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Houpert?= <10154151+lhoupert@users.noreply.github.com> Date: Tue, 6 Oct 2026 16:56:34 +0100 Subject: [PATCH 5/8] fix: upload each working directory's bandit SARIF under its own category upload-sarif aborts a second upload with the same category in the same job ("only one run of the codeql/analyze or codeql/upload-sarif actions is allowed per job per tool/category"). So in a monorepo job that runs the action twice, the second project's results were never uploaded. A working_directory other than '.' now uploads under the category `bandit/`. For '.', the category stays empty, which upload-sarif treats as unset (getOptionalInput maps "" to undefined). That is what the bandit fork did, so existing alerts keep matching. Co-Authored-By: Claude Opus 5.5 (1M context) --- action.yml | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/action.yml b/action.yml index 3207a04..61baa6a 100644 --- a/action.yml +++ b/action.yml @@ -72,14 +72,19 @@ runs: run: uv run --no-project --with "$GITHUB_ACTION_PATH" python -m python_security_auditing # results.sarif exists only if bandit scanned every file in this job: the first step removes - # an earlier one, and the audit deletes it when bandit fails (an empty report would close - # every open alert). Runs before the artifact upload, so a failed upload cannot skip it. + # an earlier one, and the audit deletes it when bandit fails or skips files (an empty or + # partial report would close open alerts). Runs before the artifact upload, so a failed + # upload cannot skip it. - name: Upload bandit SARIF to Code Scanning if: contains(inputs.tools, 'bandit') && hashFiles('results.sarif') != '' continue-on-error: true # needs `security-events: write`; the audit result stands without it uses: github/codeql-action/upload-sarif@2892aa5e19bbd11bc0cff5427e3b750a04d9e3c2 # v4.38.2 with: sarif_file: ${{ github.workspace }}/results.sarif + # A job may upload one SARIF per category, so each working_directory gets its own. + # For '.' it stays empty (= no category, the default analysis), as the bandit fork's + # upload did, so existing alerts keep matching. + category: ${{ inputs.working_directory != '.' && format('bandit/{0}', inputs.working_directory) || '' }} - name: Upload ${{ inputs.artifact_name }} if: always() From d024d7d1213281dda650bf7bf73edaf0eb409300 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Houpert?= <10154151+lhoupert@users.noreply.github.com> Date: Tue, 6 Oct 2026 16:57:16 +0100 Subject: [PATCH 6/8] test: fail the integration validation when a bandit case has no SARIF A missing results.sarif was read as "no bandit findings". The action now deletes the file when bandit did not run or skipped files, so every case (all of them run bandit) must produce one, like the pip-audit "no report" check. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../tests/test_validate_results.py | 20 +++++++++++++++++++ integration-tests/validate_results.py | 12 +++++++++++ 2 files changed, 32 insertions(+) diff --git a/integration-tests/tests/test_validate_results.py b/integration-tests/tests/test_validate_results.py index 09ca751..66bb43a 100644 --- a/integration-tests/tests/test_validate_results.py +++ b/integration-tests/tests/test_validate_results.py @@ -289,6 +289,26 @@ def test_pip_audit_disabled_needs_no_report(self) -> None: assert vr.check_audited(expected, None) == [] +# --------------------------------------------------------------------------- +# check_scanned +# --------------------------------------------------------------------------- + + +class TestCheckScanned: + """The action deletes results.sarif when bandit did not run or skipped files.""" + + def test_missing_sarif_returns_error(self, tmp_path: Path) -> None: + errors = vr.check_scanned(tmp_path / "results.sarif") + + assert any("no results.sarif" in e for e in errors) + + def test_present_sarif_returns_no_error(self, tmp_path: Path) -> None: + path = tmp_path / "results.sarif" + path.write_text(json.dumps({"runs": [{"results": []}]})) + + assert vr.check_scanned(path) == [] + + # --------------------------------------------------------------------------- # generate_report # --------------------------------------------------------------------------- diff --git a/integration-tests/validate_results.py b/integration-tests/validate_results.py index ac7dd47..1de6efb 100644 --- a/integration-tests/validate_results.py +++ b/integration-tests/validate_results.py @@ -153,6 +153,17 @@ def check_audited(expected: dict[str, Any], pip_audit_path: Path | None) -> list return [] +def check_scanned(sarif_path: Path) -> list[str]: + """Error when a case (all of them run bandit) has no results.sarif. + + The action deletes it when bandit did not run or skipped files, so a case + without one would otherwise pass as "no findings". + """ + if sarif_path.exists(): + return [] + return ["bandit: no results.sarif (bandit did not run, or skipped files)"] + + # --------------------------------------------------------------------------- # Report generation # --------------------------------------------------------------------------- @@ -277,6 +288,7 @@ def main() -> int: conclusion = conclusions.get(num, "missing") errors = validate_test(num, exp, conclusion, bandit_findings, pip_audit_findings) errors += check_audited(exp, pip_audit_path) + errors += check_scanned(sarif_path) all_errors[num] = errors status = "✅" if not errors else "❌" From 8d4a54124e832fc0075b2ae251c721b4f4256820 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Houpert?= <10154151+lhoupert@users.noreply.github.com> Date: Tue, 6 Oct 2026 16:58:00 +0100 Subject: [PATCH 7/8] docs: document .bandit handling, venv excludes and Code Scanning permissions README: - Only one `.bandit` file is read, and only inside the scanned directories. bandit fails when it finds more than one. - An `exclude` list replaces bandit's default excludes, so the example repeats them as `*/name/*` globs. A plain `.venv` does not match when bandit scans `.`. - With the default `.`, a virtual environment or node_modules is scanned too. - In private repositories the upload needs `actions: read`: upload-sarif calls GET /repos/{owner}/{repo}/actions/runs/{run_id}. The upload is best effort. CLAUDE.md: bandit runs as a subprocess (`python -m bandit`), not in-process. Co-Authored-By: Claude Opus 5.5 (1M context) --- CLAUDE.md | 6 +++--- README.md | 11 +++++++++-- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index a52809a..862e6c0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -13,7 +13,7 @@ action.yml ← GitHub Action entry point (composite steps) src/python_security_auditing/ __main__.py ← Orchestrator: settings → runners → report → comment → exit settings.py ← Pydantic-based config from env vars (GitHub Action inputs) - runners.py ← Tool invocation: SARIF parsing, pip-audit, package manager adapters + runners.py ← Tool invocation: bandit and its SARIF, pip-audit, package manager adapters report.py ← Markdown report builder and threshold checker pr_comment.py ← Upsert PR comment via `gh` CLI ``` @@ -22,7 +22,7 @@ src/python_security_auditing/ **Key boundaries:** - `settings.py` — input/config boundary (reads env vars, validates via Pydantic) -- `runners.py` — external tool boundary (subprocess calls to bandit SARIF, pip-audit, package managers) +- `runners.py` — external tool boundary (subprocess calls to bandit, pip-audit, package managers) - `report.py` — pure logic (markdown generation, threshold checking — no I/O except step summary) - `pr_comment.py` — GitHub API boundary (subprocess calls to `gh` CLI) @@ -68,7 +68,7 @@ uv run ruff format src/ tests/ ## Key Design Decisions -- **Bandit runs in-process:** `run_bandit()` runs bandit from the package's own environment, writes `results.sarif` at the workspace root (artifact and Code Scanning upload), and reads it back. It fails closed: a crash, a skipped file or no file scanned raises `AuditError`. +- **Bandit runs as a subprocess:** `run_bandit()` runs the bandit CLI as a subprocess (`python -m bandit`, from the package's own environment), writes `results.sarif` at the workspace root (artifact and Code Scanning upload), and reads it back. It fails closed: a crash or no file scanned raises `AuditError`; skipped files block the job and their SARIF is not uploaded. - **PR comment is idempotent:** Uses a hidden HTML marker (``) to find and update the same comment on subsequent pushes. - **Threshold logic:** `check_thresholds()` in `report.py` returns a boolean; the orchestrator translates that to `sys.exit(1)`. - **Package manager adapters:** `generate_requirements()` normalizes all package managers to a `requirements.txt` file before passing to `pip-audit`. diff --git a/README.md b/README.md index d3cda50..32a8fee 100644 --- a/README.md +++ b/README.md @@ -161,7 +161,7 @@ permissions: security-events: write ``` -If you don't need Code Scanning integration, `contents: read` alone is sufficient: the SARIF upload then fails without failing the job. +In a private repository, the Code Scanning upload also needs `actions: read`, because `upload-sarif` reads the workflow run. The upload is best effort: without these permissions it fails without failing the job, and findings still appear in the annotations and the summary. If you don't need Code Scanning integration, `contents: read` alone is sufficient. ## Usage examples @@ -214,7 +214,14 @@ When your source code spans more than one directory, pass a comma-separated list ### Configuring bandit -Bandit honours a `.bandit` file (INI, `[bandit]` section, for example `exclude` or `skips`) placed in a scanned directory. `[tool.bandit]` in `pyproject.toml` is not read. Bandit runs on Python 3.13, and a file it cannot parse fails the job, because its findings would otherwise go unreported. To skip such a file on purpose, list it under `exclude`. +Bandit reads a `.bandit` file (INI, `[bandit]` section, for example `exclude` or `skips`). It does not read `[tool.bandit]` in `pyproject.toml`. Bandit only looks for `.bandit` inside the scanned directories, and it fails if it finds more than one. So keep a single `.bandit`: at the working-directory root when you scan the default `.`, otherwise in one of the `bandit_scan_dirs`. + +Bandit runs on Python 3.13. A file it cannot parse fails the job, because its findings would otherwise be missing; the other files' findings are still reported. With the default `bandit_scan_dirs: '.'`, this includes files in a virtual environment or `node_modules` inside the working directory. To skip files on purpose, list them under `exclude`. That list replaces bandit's default excludes, so repeat them. Use `*/name/*` globs for directories: when bandit scans `.`, a plain name such as `.venv` does not match. + +```ini +[bandit] +exclude = */.git/*,*/__pycache__/*,*/.tox/*,*/.eggs/*,*.egg,*/.venv/*,path/to/file.py +``` ### Project in a subdirectory (monorepo) From e50f338c78913e98d453996f027e98d151bb7c4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Houpert?= <10154151+lhoupert@users.noreply.github.com> Date: Tue, 6 Oct 2026 17:00:47 +0100 Subject: [PATCH 8/8] refactor: read enabled_tools from the normalized tools value The tools validator already lowercases, trims and drops empty names. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/python_security_auditing/settings.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/python_security_auditing/settings.py b/src/python_security_auditing/settings.py index 12d6aba..d25b302 100644 --- a/src/python_security_auditing/settings.py +++ b/src/python_security_auditing/settings.py @@ -113,7 +113,7 @@ def _known_tools(cls, v: str) -> str: @property def enabled_tools(self) -> list[str]: - return [t.strip() for t in self.tools.split(",") if t.strip()] + return self.tools.split(",") # normalized by _known_tools @property def blocking_severities(self) -> list[str]: