diff --git a/CLAUDE.md b/CLAUDE.md index f9f5641..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) @@ -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 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 3ff4d6a..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. +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 @@ -212,6 +212,17 @@ When your source code spans more than one directory, pass a comma-separated list bandit_scan_dirs: 'src/,scripts/' ``` +### Configuring bandit + +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) Set `working_directory` to the project root within the repo. All relative paths (scan dirs, requirements file) are resolved from there: @@ -343,8 +354,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..61baa6a 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,21 @@ 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 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() uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 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 "❌" 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..04ed765 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 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..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] = { @@ -19,11 +20,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 +39,14 @@ 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)}") + 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" + ) results: list[dict[str, Any]] = bandit_report.get("results", []) def _sort_key(r: dict[str, Any]) -> int: @@ -40,7 +55,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..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( @@ -14,6 +33,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 +47,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 +64,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 +78,15 @@ 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 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) @@ -160,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 32976c6..290fc47 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,37 +151,106 @@ 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": []} - - 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: - 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 = phys.get("artifactLocation", {}).get("uri", "") - 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", ""), - } - ) + """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 malformed. + """ + 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") + 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 + + 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 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(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, sys.executable + + if settings.debug: + print( + f"[debug] bandit exit={result.returncode} stderr={result.stderr!r}", file=sys.stderr + ) - return {"results": results, "errors": []} + if result.returncode: + 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 report["files_read"] == 0 and not report["errors"]: + raise AuditError(f"bandit found no Python file to scan in {targets}") + complete = not report["errors"] + return report + except OSError as exc: + raise AuditError(f"bandit could not run: {exc}") from exc + finally: + if not complete: + with contextlib.suppress(OSError): + sarif_path.unlink(missing_ok=True) def run_pip_audit( diff --git a/src/python_security_auditing/settings.py b/src/python_security_auditing/settings.py index c5e72fb..d25b302 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,10 +100,20 @@ 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 + + @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()] + return self.tools.split(",") # normalized by _known_tools @property def blocking_severities(self) -> list[str]: diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 0000000..f6685d4 --- /dev/null +++ b/tests/conftest.py @@ -0,0 +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 _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) 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..6de5e14 100644 --- a/tests/test_annotations.py +++ b/tests/test_annotations.py @@ -123,6 +123,40 @@ 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_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 c4919ee..4f3e51d 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -2,8 +2,10 @@ from __future__ import annotations +import json import subprocess from pathlib import Path +from typing import Any from unittest.mock import MagicMock, patch import pytest @@ -15,26 +17,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[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 + 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 +57,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 +66,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 +82,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 +105,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 +116,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 +139,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 +151,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 +167,74 @@ 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[1:3] == ["-m", "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() + + +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 42ac3f2..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" @@ -219,6 +224,58 @@ 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 + + +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 6002a76..506a919 100644 --- a/tests/test_runners.py +++ b/tests/test_runners.py @@ -4,8 +4,10 @@ import json import subprocess +import sys import tempfile from pathlib import Path +from typing import Any from unittest.mock import MagicMock, patch import pytest @@ -13,6 +15,7 @@ AuditError, generate_requirements, read_bandit_sarif, + run_bandit, run_pip_audit, ) from python_security_auditing.settings import Settings @@ -122,6 +125,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 +161,283 @@ 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 + + +@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 +# --------------------------------------------------------------------------- + + +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.subprocess.run", + side_effect=_fake_bandit(_bandit_sarif()), + ) as mock_run: + run_bandit(Settings()) + + # 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", + "-f", + "sarif", + "-o", + str(sarif_path.resolve()), + "--exit-zero", + ] + 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.subprocess.run", + side_effect=_fake_bandit(_bandit_sarif()), + ) as mock_run: + run_bandit(Settings()) + assert mock_run.call_args[0][0][3:5] == ["-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.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"), + [ + # 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"), + ('{"runs": [{"results": null}]}', 0, "", "bandit wrote no valid SARIF report"), + (_bandit_sarif(files=()), 0, "", r"no Python file to scan in \['.'\]"), + ], +) +def test_run_bandit_fails_closed( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + sarif: str | None, + returncode: int, + stderr: str, + message: str, +) -> None: + """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.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_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) + 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 len(message) < 2_100 + assert "… (truncated)" in message + assert message.endswith("ValueError: the real cause") + + +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.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/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() 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"