From 80cdf221b2d38a3162c40393a229a6adbdd67e59 Mon Sep 17 00:00:00 2001 From: Yeongseon Choe Date: Sat, 24 Oct 2026 21:57:20 +0900 Subject: [PATCH 1/9] ci(security): add a guard that fails on an empty or partial Bandit report Bandit exits 0 for an empty directory, a missing path and a file it cannot parse; it only lists them as skipped. scripts/check_bandit_report.py reads Bandit's JSON report and exits 1 unless some code was analysed (loc > 0), no file was skipped (errors), every .py file under the declared scope appears in the report (catches a scope narrowed with -x or a .bandit file) and there are no findings. It prints one `bandit: ...` line per problem, and fails closed on a missing or unparsable report. tests/test_check_bandit_report.py runs real Bandit with the flags the Makefile uses on scratch trees: empty directory, only empty files, missing directory, unparsable file, a file excluded by .bandit and a new shell=True call must fail with the matching reason; a clean tree, a 60-file tree (Bandit 1.9.4 draws a progress bar above 50 files) and a finding suppressed inline with `# nosec Bxxx - reason` must pass. Disabling any single guard check turns at least one case red. Refs #578 Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- scripts/check_bandit_report.py | 44 ++++++++++++++++ tests/test_check_bandit_report.py | 83 +++++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+) create mode 100644 scripts/check_bandit_report.py create mode 100644 tests/test_check_bandit_report.py 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/tests/test_check_bandit_report.py b/tests/test_check_bandit_report.py new file mode 100644 index 0000000..35d1fe6 --- /dev/null +++ b/tests/test_check_bandit_report.py @@ -0,0 +1,83 @@ +"""The Bandit gate must fail instead of passing vacuously (#578), proven with real Bandit. + +Bandit exits 0 for most of these scopes, so each case builds a scratch tree, runs Bandit with +the flags of ``make security`` and hands its JSON report to the guard. +""" + +from __future__ import annotations + +from pathlib import Path +import subprocess +import sys + +import pytest + +from scripts.check_bandit_report import main + +_SHELL_TRUE = "import subprocess\nsubprocess.call('echo', shell=True)\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 From 7b99fbd8d2ff0bb26b3fbfb3406e5ec99cc9c875 Mon Sep 17 00:00:00 2001 From: Yeongseon Choe Date: Sat, 24 Oct 2026 21:57:27 +0900 Subject: [PATCH 2/9] ci(security): scan examples, scripts and tools and run the guard in make security make security ran `bandit -r src` and trusted its exit status. It now deletes the old reports, scans `src examples` with every test and `scripts tools` with B404/B603/B607 skipped (B602 shell=True stays on), each with `-q -f json -o test-results/bandit/.json`, runs scripts/check_bandit_report.py on each report with the same scope variable, and fails if Bandit or the guard did. -q keeps the progress bar Bandit 1.9.4 prints on stdout above 50 files out of the log. Deleting the reports first means a stale clean report can never be read when Bandit does not run. The `security` Hatch script (`python -m bandit -r src`, no guard) is removed: nothing calls it any more except docs/security.md, so it would stay a second, vacuous way to run the scan. test-results/ is ignored. Refs #578 Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- .gitignore | 1 + Makefile | 19 ++++++++++++++++++- pyproject.toml | 1 - 3 files changed, 19 insertions(+), 2 deletions(-) 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..e061096 100644 --- a/Makefile +++ b/Makefile @@ -58,9 +58,26 @@ 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 each JSON report; the reports are deleted first so a stale one can never pass. +# -q: Bandit 1.9.4 prints a progress bar to stdout above 50 files. +# The helpers run git, ruff and pip with argument lists, so B404/B603/B607 are skipped for +# them only (B602 shell=True stays on). Suppress anything else inline: `# nosec Bxxx - reason`. +BANDIT_DIR := test-results/bandit +BANDIT_STRICT := src examples +BANDIT_HELPERS := 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)/strict.json -r $(BANDIT_STRICT) || status=1; \ + $(BANDIT_CHECK) $(BANDIT_DIR)/strict.json $(BANDIT_STRICT) || status=1; \ + $(BANDIT) -o $(BANDIT_DIR)/helpers.json -s B404,B603,B607 -r $(BANDIT_HELPERS) || status=1; \ + $(BANDIT_CHECK) $(BANDIT_DIR)/helpers.json $(BANDIT_HELPERS) || status=1; \ + exit $$status .PHONY: check-schema-hash check-schema-hash: ensure-hatch 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 From c0301775cb043c41335af97967e373f2f1397ac0 Mon Sep 17 00:00:00 2001 From: Yeongseon Choe Date: Sat, 24 Oct 2026 21:57:32 +0900 Subject: [PATCH 3/9] ci(security): upload the Bandit reports from the bandit job Upload test-results/bandit/ with if: always() so the JSON behind a red or green required check can be inspected, with 14-day retention. actions/upload-artifact is pinned to the v7.0.1 SHA that sbom.yml already uses (node24); the v4 pin used by the release workflows runs on node20 and would put a deprecation annotation on a required check. The artifact name carries run_attempt because v4+ artifacts are immutable per workflow run. The job id, permissions, triggers and existing steps are unchanged. Refs #578 Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- .github/workflows/security.yml | 8 ++++++++ 1 file changed, 8 insertions(+) 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: From de77db07f9c8c9c8048fe06f4c45e61aa69eb7ed Mon Sep 17 00:00:00 2001 From: Yeongseon Choe Date: Sat, 24 Oct 2026 21:57:32 +0900 Subject: [PATCH 4/9] docs(security): document the Bandit scope, guard and suppression convention docs/security.md still described `hatch run security` and `python -m bandit -r src`. Describe the real scope, the report guard and the single suppression mechanism (`# nosec Bxxx - `, no allow-list or baseline file), and say in docs/development.md that the pre-commit Bandit hook is narrower than make security. Refs #578 Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- docs/development.md | 3 ++- docs/security.md | 14 ++++++-------- 2 files changed, 8 insertions(+), 9 deletions(-) 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..bb5f326 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` and `examples` with every test enabled, and `scripts` and `tools` with the subprocess tests B404, B603 and B607 skipped (those helpers run `git`, `ruff` and `pip` with argument lists; B602, `shell=True`, stays on). 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 reports to `test-results/bandit/`, 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 false positive inline with `# nosec Bxxx - `, naming the test ID and the reason. There is no separate allow-list or baseline file. ### 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 reports are uploaded as a workflow artifact. ### Pre-commit Hook From cf810eb4a2c27e7d327891b6f7c0acbba1b519e6 Mon Sep 17 00:00:00 2001 From: Yeongseon Choe Date: Sat, 24 Oct 2026 22:15:31 +0900 Subject: [PATCH 5/9] ci(security): document a quiet inline suppression form that Bandit honours The convention written in the previous commits, `# nosec Bxxx - `, works but makes Bandit 1.9.4 print a `Test in comment: is not a test name or id` warning for every word of the reason, and a comma-separated list such as `# nosec B404,B602` registers only its last ID (NOSEC_COMMENT_TESTS captures the last repetition of its group). The ID list ends at a second `#`, so `# nosec Bxxx # ` is silent (measured: stdout and stderr empty) and several IDs are separated by spaces. Document that form in docs/security.md and the Makefile comment, and use it in the test fixture so the documented form is the one proven to suppress the finding. Refs #578 Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- Makefile | 2 +- docs/security.md | 2 +- tests/test_check_bandit_report.py | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Makefile b/Makefile index e061096..ac05f14 100644 --- a/Makefile +++ b/Makefile @@ -62,7 +62,7 @@ lint: ensure-hatch # also reads each JSON report; the reports are deleted first so a stale one can never pass. # -q: Bandit 1.9.4 prints a progress bar to stdout above 50 files. # The helpers run git, ruff and pip with argument lists, so B404/B603/B607 are skipped for -# them only (B602 shell=True stays on). Suppress anything else inline: `# nosec Bxxx - reason`. +# them only (B602 shell=True stays on). Suppress anything else inline: `# nosec Bxxx # reason`. BANDIT_DIR := test-results/bandit BANDIT_STRICT := src examples BANDIT_HELPERS := scripts tools diff --git a/docs/security.md b/docs/security.md index bb5f326..02f9e88 100644 --- a/docs/security.md +++ b/docs/security.md @@ -59,7 +59,7 @@ You can run a security scan locally with: make security ``` -Suppress a reviewed false positive inline with `# nosec Bxxx - `, naming the test ID and the reason. There is no separate allow-list or baseline file. +Suppress a reviewed false positive inline 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 separate allow-list or baseline file. ### CI/CD Integration diff --git a/tests/test_check_bandit_report.py b/tests/test_check_bandit_report.py index 35d1fe6..c75feea 100644 --- a/tests/test_check_bandit_report.py +++ b/tests/test_check_bandit_report.py @@ -16,8 +16,8 @@ _SHELL_TRUE = "import subprocess\nsubprocess.call('echo', shell=True)\n" _SUPPRESSED = ( - "import subprocess # nosec B404 - test fixture\n" - "subprocess.call('echo', shell=True) # nosec B602 B607 - test fixture\n" + "import subprocess # nosec B404 # test fixture\n" + "subprocess.call('echo', shell=True) # nosec B602 B607 # test fixture\n" ) _OK = "x = 1\n" From 3336611a310bc2adb0b0a1f79ff5abf65be96d41 Mon Sep 17 00:00:00 2001 From: Yeongseon Choe Date: Sat, 24 Oct 2026 23:32:25 +0900 Subject: [PATCH 6/9] ci(security): annotate the reviewed subprocess calls in check_wheel_schemas Bandit reports B404 on `import subprocess` and B603 on both `subprocess.run` calls in scripts/check_wheel_schemas.py (LOW severity, HIGH confidence). They run the python of the temporary venv the script has just created, as argument lists without a shell; the only non-literal argument is the absolute wheel path, and the second call runs a fixed code string. Each finding gets its own `# nosec Bxxx # ` marker so the directory-wide skip of those tests can go. The pip call is wrapped onto three lines only so its marker fits the line width; ruff format 0.16.8 and 0.17.1 both accept the file as written. `bandit --ignore-nosec -r scripts tools` reports the same eight findings before and after (this file's three move from lines 11, 71, 74 to 11, 71, 76), and dropping any single ID from a marker re-surfaces exactly that finding. Refs #578 Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- scripts/check_wheel_schemas.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) 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, From 57afe4effc85863bb991da3c821adc1a6d5f3213 Mon Sep 17 00:00:00 2001 From: Yeongseon Choe Date: Sat, 24 Oct 2026 23:32:25 +0900 Subject: [PATCH 7/9] ci(security): annotate the reviewed subprocess calls in check_pr_format tools/check_pr_format.py runs `git diff` and `ruff format --check` as fixed argument lists without a shell. Bandit reports B404 on the import and B603 plus B607 (executable found through PATH) on each call, all LOW severity. Each finding gets its own `# nosec Bxxx # ` marker so the directory-wide skip of those tests can go. The ruff call is wrapped onto three lines only so its marker fits the line width; ruff format 0.16.8 and 0.17.1 both accept the file as written, and tests/test_check_pr_format.py still passes. Refs #578 Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- tools/check_pr_format.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) 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__": From 28e74d5a1ad30f43a23c1a7cc16893ccc3aa062f Mon Sep 17 00:00:00 2001 From: Yeongseon Choe Date: Sat, 24 Oct 2026 23:32:25 +0900 Subject: [PATCH 8/9] ci(security): scan scripts and tools with every Bandit test and no skip list The helper scan skipped B404, B603 and B607 for all of scripts/ and tools/, so a new unreviewed subprocess call there passed make security: a seeded tools/zz.py with `subprocess.run([sys.argv[1]], check=True)` was green while an unskipped Bandit reports B404 and B603. That contradicts #578, which allows exceptions only as inline `# nosec` markers with reasons. The eight reviewed findings now carry their own markers, so drop `-s` and scan `src examples scripts tools` in one run: one report (test-results/bandit/bandit.json) and one guard call on the same scope variable, which leaves no way to give one directory different flags. tests/test_check_bandit_report.py gets four cases that run the real Makefile recipe on a scratch copy of the layout it scans (a pass-through script stands in for hatch, `true` for pip), with the same new code in scripts/ and tools/: clean (passes), an unannotated subprocess call (fails with B404 and B603), `shell=True` (fails with B602) and a call with reasoned inline markers (passes). The unannotated-call case fails against the previous recipe; re-adding `-s B404,B603,B607`, or dropping scripts or tools from the scope, turns cases red again. Refs #578 Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- Makefile | 15 +++----- tests/test_check_bandit_report.py | 63 ++++++++++++++++++++++++++++++- 2 files changed, 67 insertions(+), 11 deletions(-) diff --git a/Makefile b/Makefile index ac05f14..5a8c8b1 100644 --- a/Makefile +++ b/Makefile @@ -59,13 +59,12 @@ lint: ensure-hatch @$(HATCH) run lint # Bandit exits 0 for an empty, missing or unparsable scope, so scripts/check_bandit_report.py -# also reads each JSON report; the reports are deleted first so a stale one can never pass. +# 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. -# The helpers run git, ruff and pip with argument lists, so B404/B603/B607 are skipped for -# them only (B602 shell=True stays on). Suppress anything else inline: `# nosec Bxxx # reason`. +# 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_STRICT := src examples -BANDIT_HELPERS := scripts tools +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 @@ -73,10 +72,8 @@ BANDIT_CHECK := $(HATCH) run python scripts/check_bandit_report.py security: ensure-hatch @rm -rf $(BANDIT_DIR) && mkdir -p $(BANDIT_DIR) @status=0; \ - $(BANDIT) -o $(BANDIT_DIR)/strict.json -r $(BANDIT_STRICT) || status=1; \ - $(BANDIT_CHECK) $(BANDIT_DIR)/strict.json $(BANDIT_STRICT) || status=1; \ - $(BANDIT) -o $(BANDIT_DIR)/helpers.json -s B404,B603,B607 -r $(BANDIT_HELPERS) || status=1; \ - $(BANDIT_CHECK) $(BANDIT_DIR)/helpers.json $(BANDIT_HELPERS) || status=1; \ + $(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 diff --git a/tests/test_check_bandit_report.py b/tests/test_check_bandit_report.py index c75feea..d7dd853 100644 --- a/tests/test_check_bandit_report.py +++ b/tests/test_check_bandit_report.py @@ -1,12 +1,18 @@ """The Bandit gate must fail instead of passing vacuously (#578), proven with real Bandit. -Bandit exits 0 for most of these scopes, so each case builds a scratch tree, runs Bandit with -the flags of ``make security`` and hands its JSON report to the guard. +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 @@ -14,7 +20,14 @@ 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" @@ -81,3 +94,49 @@ def test_missing_report_fails_instead_of_reading_a_stale_one( # 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 From 07a6b62fc920bbdaa4c26a1ff50a299dfa6f19f3 Mon Sep 17 00:00:00 2001 From: Yeongseon Choe Date: Sat, 24 Oct 2026 23:32:25 +0900 Subject: [PATCH 9/9] docs(security): describe the single Bandit scope and the inline-only suppressions docs/security.md still said scripts and tools run with B404, B603 and B607 skipped. Describe the one scope with every test enabled, the single report file, and the rule that each subprocess call in scripts/ and tools/ carries its own reasoned marker, so a new one is reported until someone reviews it. Refs #578 Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- docs/security.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/security.md b/docs/security.md index 02f9e88..ebe6390 100644 --- a/docs/security.md +++ b/docs/security.md @@ -49,9 +49,9 @@ 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. `make security` scans `src` and `examples` with every test enabled, and `scripts` and `tools` with the subprocess tests B404, B603 and B607 skipped (those helpers run `git`, `ruff` and `pip` with argument lists; B602, `shell=True`, stays on). Test files are not scanned. +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. -Bandit exits 0 for an empty directory, a missing path and a file it cannot parse. `make security` therefore writes Bandit's JSON reports to `test-results/bandit/`, 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. +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: @@ -59,11 +59,11 @@ You can run a security scan locally with: make security ``` -Suppress a reviewed false positive inline 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 separate allow-list or baseline file. +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. The Bandit JSON reports are uploaded as a workflow artifact. +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