diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index 6f76dbd..c114406 100644 --- a/.github/workflows/security.yml +++ b/.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: diff --git a/.gitignore b/.gitignore index 2ff99df..7a001a7 100644 --- a/.gitignore +++ b/.gitignore @@ -15,6 +15,7 @@ env/ .coverage coverage.xml htmlcov/ +test-results/ # Build artifacts build/ diff --git a/Makefile b/Makefile index c781c5f..5a8c8b1 100644 --- a/Makefile +++ b/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 # `, 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 diff --git a/docs/development.md b/docs/development.md index f0a6e9e..f0ed710 100644 --- a/docs/development.md +++ b/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 | diff --git a/docs/security.md b/docs/security.md index 651a608..ebe6390 100644 --- a/docs/security.md +++ b/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 # `. 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 diff --git a/pyproject.toml b/pyproject.toml index 8b102cc..9197699 100644 --- a/pyproject.toml +++ b/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 diff --git a/scripts/check_bandit_report.py b/scripts/check_bandit_report.py new file mode 100644 index 0000000..a0e9c49 --- /dev/null +++ b/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 ...") + raise SystemExit(main(sys.argv[1], sys.argv[2:])) diff --git a/scripts/check_wheel_schemas.py b/scripts/check_wheel_schemas.py index 6a6691e..84b9f43 100644 --- a/scripts/check_wheel_schemas.py +++ b/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, diff --git a/tests/test_check_bandit_report.py b/tests/test_check_bandit_report.py new file mode 100644 index 0000000..d7dd853 --- /dev/null +++ b/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 diff --git a/tools/check_pr_format.py b/tools/check_pr_format.py index 6c3051c..a51ed08 100644 --- a/tools/check_pr_format.py +++ b/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__":