ci(security): fail the Bandit gate when it analysed nothing and scan examples, scripts and tools

.github/workflows/security.yml

@@ -31,6 +31,14 @@ jobs:
       - name: Run Bandit
         run: make security
 
+      - name: Upload Bandit reports
+        if: always()
+        uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
+        with:
+          name: bandit-reports-${{ github.run_attempt }}
+          path: test-results/bandit/
+          retention-days: 14
+
   semgrep:
     runs-on: ubuntu-latest
     steps:

.gitignore

@@ -15,6 +15,7 @@ env/
 .coverage
 coverage.xml
 htmlcov/
+test-results/
 
 # Build artifacts
 build/

Makefile

@@ -58,9 +58,23 @@ typecheck: ensure-hatch
 lint: ensure-hatch
 	@$(HATCH) run lint
 
+# Bandit exits 0 for an empty, missing or unparsable scope, so scripts/check_bandit_report.py
+# also reads its JSON report; the report is deleted first so a stale one can never pass.
+# -q: Bandit 1.9.4 prints a progress bar to stdout above 50 files.
+# Every Bandit test runs on every file: there is no skip list for any directory. Suppress a
+# reviewed finding on its own line, `# nosec Bxxx # <reason>`, never here.
+BANDIT_DIR := test-results/bandit
+BANDIT_SCOPE := src examples scripts tools
+BANDIT := $(HATCH) run python -m bandit -q -f json
+BANDIT_CHECK := $(HATCH) run python scripts/check_bandit_report.py
+
 .PHONY: security
 security: ensure-hatch
-	@$(HATCH) run security
+	@rm -rf $(BANDIT_DIR) && mkdir -p $(BANDIT_DIR)
+	@status=0; \
+	$(BANDIT) -o $(BANDIT_DIR)/bandit.json -r $(BANDIT_SCOPE) || status=1; \
+	$(BANDIT_CHECK) $(BANDIT_DIR)/bandit.json $(BANDIT_SCOPE) || status=1; \
+	exit $$status
 
 .PHONY: check-schema-hash
 check-schema-hash: ensure-hatch

docs/development.md

@@ -79,6 +79,7 @@ single source of truth, so this table does not restate the numbers.
 
 - Only scans `src/` directory
 - Skips `tests/`
+- This is the pre-commit hook; `make security` scans more (see [Security](security.md))
 
 ### Run Hooks Manually
 
@@ -121,7 +122,7 @@ Use these as the **golden commands** for local validation and CI parity. Prefer
 | `make format` | Format code (ruff) |
 | `make lint` | Run linter (ruff + mypy) |
 | `make typecheck` | Run mypy type checking |
-| `make security` | Run Bandit security scan |
+| `make security` | Run Bandit on `src`, `examples`, `scripts` and `tools`; fail on an empty or partial scan |
 | `make test` | Run pytest |
 | `make cov` | Run tests with coverage |
 | `make check` | Run lint + typecheck |

docs/security.md

@@ -49,23 +49,21 @@ We use automated tools to ensure the codebase remains secure and follows best pr
 
 ### Bandit: Static Analysis Security Scanner
 
-Bandit is used for static analysis security scanning of Python code. It is configured to scan the source code while skipping test files to minimize false positives.
+Bandit is used for static analysis security scanning of Python code. `make security` scans `src`, `examples`, `scripts` and `tools` in one run with every Bandit test enabled; no directory has a skip list. Test files are not scanned.
 
-You can run a security scan locally using the following commands:
+Bandit exits 0 for an empty directory, a missing path and a file it cannot parse. `make security` therefore writes Bandit's JSON report to `test-results/bandit/bandit.json`, and `scripts/check_bandit_report.py` fails the target when no code was analysed, a file was skipped, a Python file in scope was left unscanned, or any finding remains.
+
+You can run a security scan locally with:
 
 ```bash
-# Using make
 make security
-
-# Using hatch
-hatch run security
 ```
 
-The underlying command executed is `python -m bandit -r src`.
+Suppress a reviewed finding only inline, on the line Bandit reports, with `# nosec Bxxx # <reason>`. Separate several test IDs with spaces, not commas (Bandit 1.9.4 honours only the last ID of a comma list), and keep the reason behind the second `#`: Bandit warns about every other word after the IDs. There is no skip list, allow-list or baseline file: each subprocess call in `scripts/` and `tools/` (they run `git`, `ruff` and a temporary virtualenv's `python` as argument lists, without a shell) carries its own marker, so a new one is reported until someone reviews it and writes its own.
 
 ### CI/CD Integration
 
-Security scans are integrated into our continuous integration pipeline. Every push and pull request triggers the `security.yml` workflow via GitHub Actions, ensuring that no known security regressions are introduced into the main branch.
+Security scans are integrated into our continuous integration pipeline. Every push and pull request triggers the `security.yml` workflow via GitHub Actions, ensuring that no known security regressions are introduced into the main branch. The Bandit JSON report is uploaded as a workflow artifact.
 
 ### Pre-commit Hook
 

pyproject.toml

@@ -115,7 +115,6 @@ e2e-azure = "pytest -v -m e2e tests/e2e/ --no-cov"
 docs = "mkdocs serve"
 precommit = "pre-commit run --all-files"
 precommit-install = "pre-commit install"
-security = "python -m bandit -r src"
 
 [tool.ruff]
 line-length = 100

scripts/check_bandit_report.py

@@ -0,0 +1,44 @@
+"""Exit 1 unless Bandit analysed every Python file in the scope and reported nothing.
+
+Bandit exits 0 for an empty directory, a missing path and a file it cannot parse (it only
+lists them as skipped), so its exit status alone can pass without analysing anything.
+"""
+
+from __future__ import annotations
+
+import json
+from pathlib import Path
+import sys
+
+
+def main(report: str, scopes: list[str]) -> int:
+    try:
+        data = json.loads(Path(report).read_text(encoding="utf-8"))
+    except (OSError, ValueError) as error:
+        print(f"bandit: cannot read {report}: {error}", file=sys.stderr)
+        return 1
+    scanned = {Path(name).as_posix() for name in data["metrics"]}
+    loc = data["metrics"]["_totals"]["loc"]
+    problems = ["no code was analysed"] if loc == 0 else []
+    problems += [f"skipped {e['filename']}: {e['reason']}" for e in data["errors"]]
+    problems += [
+        f"not scanned: {path.as_posix()}"
+        for scope in scopes
+        for path in sorted(Path(scope).rglob("*.py"))
+        if path.as_posix() not in scanned
+    ]
+    problems += [
+        f"{r['filename']}:{r['line_number']} {r['test_id']} {r['issue_text']}"
+        for r in data["results"]
+    ]
+    for problem in problems:
+        print(f"bandit: {problem}", file=sys.stderr)
+    if not problems:
+        print(f"bandit: {' '.join(scopes)}: {loc} lines in {len(scanned) - 1} files, no findings")
+    return 1 if problems else 0
+
+
+if __name__ == "__main__":
+    if len(sys.argv) < 3:
+        raise SystemExit("Usage: python scripts/check_bandit_report.py <report.json> <scope>...")
+    raise SystemExit(main(sys.argv[1], sys.argv[2:]))

scripts/check_wheel_schemas.py

@@ -8,7 +8,7 @@
 import hashlib
 import os
 from pathlib import Path
-import subprocess
+import subprocess  # nosec B404 # runs only the temp venv's python as argument lists, no shell
 import sys
 import tempfile
 import venv
@@ -68,10 +68,12 @@ def _install_smoke(wheel: Path) -> None:
         environment = root / "venv"
         venv.EnvBuilder(with_pip=True).create(environment)
         python = environment / ("Scripts/python.exe" if os.name == "nt" else "bin/python")
-        subprocess.run([python, "-m", "pip", "install", str(wheel)], cwd=root, check=True)
+        subprocess.run(  # nosec B603 # temp-venv python, literal args plus the wheel path, no shell
+            [python, "-m", "pip", "install", str(wheel)], cwd=root, check=True
+        )
         process_environment = os.environ.copy()
         process_environment.pop("PYTHONPATH", None)
-        subprocess.run(
+        subprocess.run(  # nosec B603 # temp-venv python, fixed smoke_code literal, no shell
             [python, "-I", "-c", smoke_code],
             cwd=root,
             env=process_environment,

tests/test_check_bandit_report.py

@@ -0,0 +1,142 @@
+"""The Bandit gate must fail instead of passing vacuously (#578), proven with real Bandit.
+
+Bandit exits 0 for most of these scopes, so the first cases build a scratch tree, run Bandit
+with the flags of ``make security`` and hand its JSON report to the guard. The last cases run
+the real ``make security`` recipe on a scratch copy of the layout it scans, so a skip list added
+to that recipe turns them red.
+"""
+
+from __future__ import annotations
+
+import os
+from pathlib import Path
+import re
+import shlex
+import shutil
+import subprocess
+import sys
+
+import pytest
+
+from scripts.check_bandit_report import main
+
+_ROOT = Path(__file__).resolve().parents[1]
+_SHELL_TRUE = "import subprocess\nsubprocess.call('echo', shell=True)\n"
+_NEW_CALL = "import subprocess\nimport sys\n\nsubprocess.run([sys.argv[1]], check=True)\n"
+_REVIEWED_CALL = (
+    "import subprocess  # nosec B404 # test fixture\n"
+    "import sys\n\n"
+    "subprocess.run([sys.argv[1]], check=True)  # nosec B603 # test fixture\n"
+)
+_SUPPRESSED = (
+    "import subprocess  # nosec B404 # test fixture\n"
+    "subprocess.call('echo', shell=True)  # nosec B602 B607 # test fixture\n"
+)
+_OK = "x = 1\n"
+
+
+@pytest.mark.parametrize(
+    ("files", "exit_code", "reason"),
+    [
+        pytest.param({"ok.py": _OK}, 0, "", id="clean-tree"),
+        # Bandit 1.9.4 draws a progress bar on stdout above 50 files; -q keeps it out.
+        pytest.param({f"m{n}.py": _OK for n in range(60)}, 0, "", id="over-50-files"),
+        pytest.param({"ok.py": _SUPPRESSED}, 0, "", id="inline-nosec-with-reason"),
+        pytest.param({}, 1, "no code was analysed", id="empty-dir"),
+        pytest.param({"empty.py": ""}, 1, "no code was analysed", id="only-empty-files"),
+        pytest.param(None, 1, "skipped", id="missing-dir"),
+        pytest.param({"bad.py": "def (:\n"}, 1, "syntax error", id="unparsable-file"),
+        pytest.param(
+            {"a.py": _OK, "gone.py": _OK, ".bandit": "[bandit]\nexclude = gone.py\n"},
+            1,
+            "not scanned",
+            id="narrowed-scope",
+        ),
+        pytest.param({"bad.py": _SHELL_TRUE}, 1, "B602", id="shell-true-finding"),
+    ],
+)
+def test_bandit_gate_fails_unless_everything_was_analysed(
+    tmp_path: Path,
+    capsys: pytest.CaptureFixture[str],
+    files: dict[str, str] | None,
+    exit_code: int,
+    reason: str,
+) -> None:
+    # Given a scope (``None``: a path that does not exist) scanned like `make security` does
+    scope = tmp_path / "scope"
+    if files is not None:
+        scope.mkdir()
+        for name, text in files.items():
+            (scope / name).write_text(text, encoding="utf-8")
+    report = tmp_path / "bandit.json"
+    bandit = [sys.executable, "-m", "bandit", "-q", "-f", "json", "-o", str(report)]
+    subprocess.run([*bandit, "-r", str(scope)], check=False, capture_output=True)
+
+    # When the guard reads Bandit's report
+    result = main(str(report), [str(scope)])
+
+    # Then it passes only a complete, finding-free scan, and names why it failed otherwise
+    assert result == exit_code
+    assert reason in capsys.readouterr().err
+
+
+def test_missing_report_fails_instead_of_reading_a_stale_one(
+    tmp_path: Path, capsys: pytest.CaptureFixture[str]
+) -> None:
+    # Given a scope and a report path that Bandit never wrote
+    scope = tmp_path / "scope"
+    scope.mkdir()
+    (scope / "ok.py").write_text(_OK, encoding="utf-8")
+
+    # When the guard looks for the report
+    result = main(str(tmp_path / "bandit.json"), [str(scope)])
+
+    # Then it fails and says it could not read it
+    assert result == 1
+    assert "cannot read" in capsys.readouterr().err
+
+
+@pytest.mark.skipif(
+    sys.platform == "win32" or shutil.which("make") is None,
+    reason="runs the real `make security` recipe through /bin/sh",
+)
+@pytest.mark.parametrize(
+    ("helper_source", "failing_tests"),
+    [
+        pytest.param("x = 1\n", set(), id="clean-helpers"),
+        pytest.param(_NEW_CALL, {"B404", "B603"}, id="new-subprocess-call-without-nosec"),
+        pytest.param(_SHELL_TRUE, {"B602"}, id="shell-true"),
+        pytest.param(_REVIEWED_CALL, set(), id="subprocess-call-with-reasoned-nosec"),
+    ],
+)
+def test_make_security_holds_helper_dirs_to_every_bandit_test(
+    tmp_path: Path, helper_source: str, failing_tests: set[str]
+) -> None:
+    # Given the layout `make security` scans, with the same new code in both helper directories
+    for directory in ("src", "examples", "scripts", "tools"):
+        (tmp_path / directory).mkdir()
+        (tmp_path / directory / "ok.py").write_text("x = 1\n", encoding="utf-8")
+    for directory in ("scripts", "tools"):
+        (tmp_path / directory / "new_helper.py").write_text(helper_source, encoding="utf-8")
+    shutil.copy(_ROOT / "Makefile", tmp_path / "Makefile")
+    shutil.copy(_ROOT / "scripts" / "check_bandit_report.py", tmp_path / "scripts")
+    hatch = tmp_path / "hatch"  # `$(HATCH) run python ...` runs this interpreter, which has Bandit
+    hatch.write_text(f'#!/bin/sh\nshift 2\nexec {shlex.quote(sys.executable)} "$@"\n')
+    hatch.chmod(0o755)
+    environment = {k: v for k, v in os.environ.items() if not k.startswith(("MAKE", "MFLAGS"))}
+
+    # When the real recipe runs (`true` stands in for pip, so nothing is installed)
+    result = subprocess.run(
+        ["make", "security", f"HATCH={hatch}", "PIP=true", f"VENV_DIR={tmp_path}"],
+        cwd=tmp_path,
+        env=environment,
+        capture_output=True,
+        text=True,
+        check=False,
+    )
+
+    # Then it fails exactly when the new code is unreviewed, naming the test in each helper file
+    reported = set(re.findall(r"^bandit: (\S+?):\d+ (B\d+) ", result.stderr, re.MULTILINE))
+    expected = {(f"{d}/new_helper.py", t) for d in ("scripts", "tools") for t in failing_tests}
+    assert (result.returncode != 0) == bool(failing_tests), result.stderr
+    assert expected <= reported, result.stderr

tools/check_pr_format.py

@@ -2,12 +2,12 @@
 
 import os
 from pathlib import Path
-import subprocess
+import subprocess  # nosec B404 # runs only git and ruff, as argument lists, no shell
 import sys
 
 
 def changed_python_files(base: str, head: str) -> list[str]:
-    result = subprocess.run(
+    result = subprocess.run(  # nosec B603 B607 # arg list, no shell; PR revisions; git on PATH
         # Exclude deletions rather than allow-listing statuses: an allow-list of
         # ACMR silently drops T (type changed), so swapping a Python-named
         # symlink for a real .py file would skip the format check entirely.
@@ -34,7 +34,9 @@ def main(base: str, head: str) -> int:
     if not paths:
         print("No changed Python files to format-check.")
         return 0
-    return subprocess.run(["ruff", "format", "--check", "--", *paths], check=False).returncode
+    return subprocess.run(  # nosec B603 B607 # arg list, no shell; paths after --; ruff on PATH
+        ["ruff", "format", "--check", "--", *paths], check=False
+    ).returncode
 
 
 if __name__ == "__main__":