fix(ci): the hardening gate was blind to the commonest way to write a step - #129
Merged
Merged
Conversation
… step Found by an adversarial audit of this repo's own guards, then reproduced as BLIND -> CAUGHT pairs. Four ways an unsafe workflow passed the gate: 1. ``USES_RE`` was ``^\s*uses:``, which does not match the list form ``- uses:``. Seven of 27 refs in this tree were invisible to the gate, every ``actions/checkout`` among them. They are pinned today by discipline, not by this check: a PR moving one to ``@main`` passed. 2. ``docker://`` was exempt outright, so a mutable third-party tag (``docker://org/img:latest``) was waved through. 3. ``permissions`` was checked for PRESENCE only, so ``permissions: write-all`` -- a declaration of total access -- counted as evidence of least privilege. 4. The ``pull_request_target`` rationale was a substring test over the raw file, so writing the waiver's NAME inside a ``#`` comment satisfied it. Also removes the ``github.com/`` exemption: ``uses:`` does not accept that prefix and a GitHub owner name cannot contain a dot, so no such org can exist. It was dead code, and it was the subject of a CodeQL incomplete-substring alert -- which is how the file came under scrutiny at all. CodeQL flagged the harmless line and missed the regex two lines above that actually broke the gate. Fixing (2) needed a second pass: ``_check_uses_pin`` splits the ref at ``@`` before consulting ``_is_third_party_uses``, so a digest test there ran on the truncated string and a digest-pinned container was then rejected by the 40-char git-SHA rule. Container digests and git SHAs are different pin shapes and are now checked separately. That regression was caught by an over-correction control, not by review. The gate had no test at all -- it was the guard nobody guarded. Added, as pairs: each unsafe workflow next to a control that must still pass, plus a digest-pinned container that must remain allowed, plus a check that the real tree passes its own gate. Evidence: 7 passed. Four BLIND -> CAUGHT pairs against the pre-fix script, with a clean workflow green under both versions (so the gate did not simply become stricter about everything). Not addressed here, tracked separately: ``quality contract`` is not a required status check, so this gate does not block a merge even when red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JsDFcKo97HuF5ZLjxa3rRa
Both suppressions were justified when written and are now justified by nothing measurable. `impostor-commit` was disabled because the audit crashed on the private cross-repo pin `coworkerz-ci`. That pin is gone: the only two occurrences of that name left in the tree are the comments explaining the suppression. It is the one audit that checks whether a 40-character SHA actually exists upstream, so with it off the entire pinning strategy was unverified. Counter-check before re-enabling, with online audits active and a token so they really ran: `No findings to report`, all 25 pins authentic. `advanced-security: false` was justified with "private repo without GHAS". The repository is public -- measured, not assumed. While that flag was false, zizmor findings were never uploaded to Code Scanning: they were absent there for want of an upload, not for want of findings. Neither change alters what zizmor checks; they stop discarding what it already found. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JsDFcKo97HuF5ZLjxa3rRa
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Two mistakes of mine, both caught by CI rather than locally, both from running a piece of the chain instead of the chain. 1. The zizmor action refuses to start with both `advanced-security: true` and `annotations: true`. I switched the first and left the second. Running zizmor directly does not exercise the action's own option validation, so the local check could not have caught it -- the action wrapper is part of the contract and was never run. `annotations: false` is the right resolution rather than reverting: inline annotations vanish with the run, a Code Scanning alert persists and can be triaged. Uploading them is the point of enabling advanced-security. 2. An extraneous `f` prefix on a string literal with no placeholder (`tests/test_workflow_hardening_gate.py`). Ruff catches it; I had run only pytest locally, not `ruff check src tests benchmarks`. Local chain now run as the job runs it: Lint `All checks passed!`, 7 passed, and the gate green on the real tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JsDFcKo97HuF5ZLjxa3rRa
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
AGENTS.md Definition of Done item 7 requires a CHANGELOG entry or an explicit rationale for omitting one. Recording the four fail-open holes, the two re-enabled zizmor audits, the over-correction that closing the docker:// hole first caused, and the known gap that quality contract is still not a required status check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JsDFcKo97HuF5ZLjxa3rRa
marcohost33-maker
added a commit
that referenced
this pull request
Aug 29, 2026
…del vocabulary CHANGELOG entries appended to the existing [Unreleased] Fixed section (which already carries both #127 and #129 items -- added, not reordered), including the explicit correction of my earlier report that the Prony seed change was not a regression: the reviewer's counter-example holds as a class. layers-and-taxonomy.md still documented residual_model as a two-value field. That line was found by turning the very defect class of finding 2 -- a label asserting what should have happened rather than what did -- against my own result, one layer up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JsDFcKo97HuF5ZLjxa3rRa
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found by an adversarial audit of this repository's own guards, then reproduced as BLIND -> CAUGHT pairs before anything was changed.
Four ways an unsafe workflow passed the gate
.github/scripts/check_workflow_hardening.pyenforces AGENTS.md section 4 on every workflow here. It reported green while all four of these walked through:- uses:)USES_REwas^\s*uses:, which does not match a list itemdocker://org/img:latestdocker://was exempt outright, mutable tag includedpermissions: write-allpull_request_targetwaived in a#commentHole 1 is the serious one: 7 of 27 refs in this tree were invisible to the gate, every
actions/checkoutamong them, inci-python-local.yml,ci-qutip.yml,ci.yml,encoding-guard.ymlandpypi.yml. They are correctly pinned today — by discipline, not by this check. A PR moving one of them to@mainpassed.Hole 3 is the one worth pausing on: a declaration of total access counted as evidence of least privilege, because the check asked whether the word appeared, not what it granted.
Also removed: the
github.com/exemption.uses:does not accept that prefix and a GitHub owner name cannot contain a dot, so no such org can exist — dead code. It was the subject of a CodeQL incomplete-substring alert, which is the only reason this file came under scrutiny. CodeQL flagged the harmless line and missed the regex two lines above it that actually broke the gate.Two zizmor suppressions whose reasons no longer hold
impostor-commitwas disabled because the audit crashed on the private cross-repo pincoworkerz-ci. That pin is gone — the only two occurrences of the name left in the tree are the comments explaining the suppression. It is the one audit that verifies a 40-character SHA actually exists upstream, so the entire pinning strategy was unverified without it. Counter-checked before re-enabling, with a token so the online audits really ran:No findings to report, all 25 pins authentic.advanced-security: falsewas justified with "private repo without GHAS". This repository is public — measured, not assumed. While that flag was false, zizmor findings were never uploaded to Code Scanning: absent for want of an upload, not for want of findings.The fix needed a second pass
Closing hole 2 initially broke digest-pinned containers:
_check_uses_pinsplits the ref at@before consulting_is_third_party_uses, so a digest test there ran on the truncated string, and the digest then failed the 40-char git-SHA rule. Container digests and git SHAs are different pin shapes and are now checked separately.That regression was caught by an over-correction control, not by review — which is the argument for the tests below.
Tests
The gate had none. It was the guard nobody guarded, which is why it could report green for four classes of unsafe workflow.
Added as pairs: each unsafe workflow next to a control that must still pass. Rejection alone would be satisfied by a gate that fails everything, which is exactly as useless as one that passes everything. Plus a digest-pinned container that must remain allowed, and a check that the real tree passes its own gate.
Evidence: 7 passed. Four BLIND -> CAUGHT pairs measured against the pre-fix script, with a clean workflow green under both versions.
Not fixed here
quality contractis not a required status check onmain. Onlytest (3.10-3.14)andqutip-cross-checkare. Five security workflows run and none of them block a merge, so the comment inzizmor.ymlclaiming the step "GATET die CI (rot = blockt merge)" is factually wrong. Making it required is the right next step — but after this fix lands, not before, or a blind gate becomes mandatory.Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com