feat(lint): require security suppressions to be reviewed in an allowlist

[kovtcharov-amd]

Why this matters

The hub tar-slip (CWE-22, #2342) shipped even though bandit flagged it — because a # noqa: S202 - hub artifacts are trusted silenced the warning, and both bandit and the weekly audit read that suppression as a settled, accepted decision. Any # noqa: S<n> / # nosec is a place where a human waved off a security finding with no required review.

This adds a lint gate that makes every security suppression an explicit, justified entry in .security-suppressions.json. A suppression in the code that isn't listed there fails python util/lint.py, forcing the author to add a justification the reviewer sees in the PR diff. It doesn't judge whether a suppression is correct — it guarantees a human signed off on each one, which is exactly the step that was missing for the tar-slip.

Only real bandit security codes count — SLF001 and other non-security S-prefixed codes are ignored. Entries key on (path, rule), never a line number, so the allowlist survives edits instead of re-firing on every line shift.

Threads

• Suppression review gate (check_suppressions, lint step 7) — the new gate this PR adds; seeded with the 6 suppressions already in the tree (S603×3, B104, B602×2).
• Sits alongside the bandit HIGH gate that landed on main — after merging main, both gates live in util/check_security_gates.py and both run from util/lint.py. This PR does not change the HIGH gate; it only adds the suppression-review gate next to it.

Test plan

• pytest tests/unit/test_check_security_gates.py — 30 pass, incl. a backtest that the gate flags the tar-slip's # noqa: S202
• python util/check_security_gates.py — suppression gate reports all 6 suppressions reviewed
• black / isort clean on the changed files
• Merge conflicts with main resolved (both gates unified in one module; lint steps renumbered to 11)

[github-actions]

Verdict: Approve ✅

This adds a lint gate that forces every # noqa: S… / # nosec security suppression to be listed with a justification in .security-suppressions.json, so a human signs off on each one in the PR diff — directly closing the hole that let the hub tar-slip through. It's well-tested (including a backtest that re-runs the tar-slip suppression through the gate), keyed on (path, rule) so it survives line shifts, and fails loudly when the allowlist is missing or unjustified. Nothing here blocks merge.

Two small gaps worth a follow-up (not blockers): a blanket # noqa with no code still silences the bandit S-rules and slips past this gate, and the scan is a line-regex so a # nosec written inside a string/docstring in src//hub/ would be flagged as a real suppression. Also a cosmetic duplicate step label in the lint output.

🔍 Technical details

🟢 Minor / nits

1. Duplicate [7/10] step label (util/lint.py:327 and :344)
The new check prints [7/10] and so does the existing imports check — the run now has 11 steps but the labels stop at [7/10]…[10/10] with 7 appearing twice. Purely cosmetic console output, but easy to bump:

    print("\n[7/11] Checking security suppressions are reviewed...")

(and renumber imports→[8/11], agents→[9/11], dependabot→[10/11], docs→[11/11]).

2. Blanket # noqa (no code) evades the gate (util/check_security_gates.py:254)
_NOQA requires noqa: followed by codes, so a bare tf.extractall(d) # noqa — which flake8 treats as suppress-everything, including flake8-bandit S-rules — isn't detected and needs no allowlist entry. Since check_bandit is warning-only (lint.py:301), that's the same evasion path the gate is meant to close, just spelled without a code. Detecting it cleanly is hard (a blanket noqa legitimately covers style rules too), so scoping it out is defensible — worth a one-line note in the module docstring and the planned "new bandit HIGH" follow-up, rather than a code change here.

3. String/docstring literals are scanned too (check_security_gates.py:286)
find_suppressions is a per-line regex over raw text, so a # nosec or # noqa: S202 appearing inside a string literal or docstring in src//hub/ would be reported as a real suppression and require an allowlist entry. The tree happens to be clean today (SCAN_DIRS excludes util/, where this module's own docstring mentions both forms), so nothing breaks now — just a latent false-positive if such a literal ever lands. Fine to leave; noting for awareness.

Strengths

• Test coverage is the standout — the backtest (test_backtest_flags_the_tarslip_suppression) proves the gate would have caught the exact # noqa: S202 that shipped, and test_repo_suppressions_all_reviewed keeps the committed allowlist honest against the real tree.
• Correct non-security-code discrimination — _BANDIT_NOQA_CODE = S\d{3} with test_ignores_non_security_noqa_codes covering SLF001/E402; I confirmed the tree's SLF001 suppressions are correctly ignored while the 4 S603/B104 entries match the allowlist.
• Fails loudly, per CLAUDE.md — missing allowlist → FileNotFoundError, unjustified entry → ValueError; the gate is blocking (is_warning=False), not warning-only, and the FAIL message names what to do.
• Keyed on (path, rule), not line number — the allowlist survives edits instead of re-firing on every line shift; good design call, called out in both the module and the JSON _comment.