ci: audit the workflows with zizmor on every pull request - #273
Merged
Conversation
Adopting it because it found something on first contact, not because it is popular: three live template-injection sites nothing here would have caught (F304). Exactly one audit is configured, and it is configured to AGREE with a policy this repository already enforces in workflow_action_pin_test — full SHAs for third-party actions, tags permitted for actions/* and for dtolnay/rust-toolchain, whose tag names the Rust toolchain rather than a release of the action. Two gates disagreeing about pinning is worse than either alone: a contributor gets told to pin by one and told it is fine by the other. Alignment drops the report from 111 findings / 44 high to 74 / 7 while suppressing none of them. OpenSSF Scorecard does ask for actions/* to be SHA-pinned, and tj-actions and reviewdog are why. The exemption stands: a compromise inside GitHub's own namespace is a compromise of the platform running the job, which a pinned SHA does not survive either — and the repository made this call explicitly, with that reasoning recorded in the guard. Overriding a documented, guarded policy on a general principle, when the stricter half is already enforced, is not an improvement. Advisory during bake-in, the pattern dogfood already uses in this file, and with an exit condition so advisory does not become permanent. The seven remaining high findings are adjudicated in F306 rather than hidden: two triggers and three write scopes on the Dependabot auto-merge workflow that are inherent to that pattern and mitigated by never checking out PR code, one workflow-level write scope on release.yml that looks genuinely reducible and touches the release path, and one low-confidence cache finding. Silencing them to make the gate green on day one would defeat the point of adding it. Also corrects the cycle-12 report's "six-site" toolchain-pin claim to five, which is what rust_version_pins_test's own doc names.
The advisory version of this job was written first and discarded on seeing it run: with findings outstanding the check is red on every pull request, and a permanently red check teaches people to ignore red checks. That is worse than not running the tool. So the seven were resolved. Fixed: release.yml's workflow-level `contents: write`. It was inherited by plan, crates-publish and homebrew-publish, none of which write to this repository — plan reads the ref, crates-publish authenticates to crates.io with a token, and homebrew-publish checks out the tap with its own deploy key and pushes there. The default is now `contents: read`; `release`, the job that creates the GitHub Release, already declared its own write. Written exceptions, on the lines that raise them: the Dependabot auto-merge workflow's two triggers and three write scopes — the triggers are what that pattern is, mitigated the documented way by never checking out PR code, and the scopes are the capability itself — plus one cache-poisoning finding whose own confidence is Low and which requires push access to exploit. The exceptions are inline `# zizmor: ignore[rule]` comments rather than line numbers in a config file, so they move with the code instead of drifting the moment a line is inserted above them. Gated at `high`. The 16 remaining `low` findings are artipacked — checkout persisting credentials — and deserve their own pass rather than a blocking gate adopted in the same commit as the tool.
Cleanup pass over the adoption commit. No change to what is enforced. The job installed zizmor with `pipx`, introducing a second tool-install mechanism next to the `taiki-e/install-action` this workflow already uses for cargo-nextest. That action supports zizmor, fetches a prebuilt binary from the tool's own GitHub Releases rather than resolving and installing a Python package, and is already SHA-pinned here — so reusing it removes a mechanism and adds no pin to maintain. The config's `"*": hash-pin` entry was commented as though it were load-bearing. It is not: tested both ways, an unpinned third-party action is flagged at `high` with or without it, because hash-pin is already zizmor's default. Kept, because writing it out lets the policy be read here in full and stops an upstream change of default from quietly relaxing it — but the comment now says that rather than implying the rule depends on it. `release.yml` had two adjacent comment blocks explaining one one-line value, one above the key and one inside it. Merged: the two absences — signing and write — hold for the same reason, that a job added later inherits the default, so they read as one argument rather than two notes.
Adding a CI job without a matching recipe made `just ci` narrower than CI, which this project treats as a defect in its own right: the guidance is that the two must match exactly, because narrower local runs have shipped work that CI then rejected. The recipe runs the identical command, asserted rather than assumed — byte-for-byte against the step in ci.yml. Documented as requiring `cargo install zizmor@1.29.0`, following the `coverage` recipe's precedent for a tool that is not a cargo subcommand. The version is named because CI pins it and a different local version can disagree, which is the reason to have the recipe rather than leave the audit to CI alone.
Three findings from the review pass, two of which measurement contradicted. The audit scanned only .github/workflows/, while workflow_action_pin_test scans the root action.yml as well — so the repository's public entry point, a 13 KB composite action with four shell steps and user-supplied inputs, was the one file neither the external tool nor a human had audited. A reviewer reasoned from reading it that broadening would find nothing. Running it found one high: a GITHUB_PATH write. Adjudicated rather than assumed — DEST is $RUNNER_TEMP/codelore, a runner directory with a constant suffix and nothing a caller can redirect, and putting an installed tool on PATH is what the action is for — so it carries a written exception, on the line the finding is attributed to. Placing it took three attempts: a shell comment inside a run: block is not a YAML comment, and a comment above the key just moves the key. The job comment claimed "Blocking". It is not: protect-main requires nine contexts and zizmor is not among them, so today it reddens a pull request without refusing the merge. Corrected to say that, and to name adding it to the ruleset as the one-line change that cashes in the separately-named job. `.github/zizmor.yml` and `is_exempt` now state one pinning policy twice. The config argues in prose that it is kept in step with that function; this adds the check for that claim, in the shape rust_version_pins_test already uses for the toolchain pinned in five places — name a source of truth, read the other statements of it textually, fail listing the disagreements. No YAML crate: the workspace has none, and the sibling guard hand-scans TOML for the same reason. Proven both ways — permitting something is_exempt does not, and dropping something it does, each fails naming the disagreement. `just ci` stays byte-identical to the CI step, asserted rather than assumed.
emrecdr
added a commit
that referenced
this pull request
Aug 14, 2026
* docs(reports): hardening cycle 13 — a recommendation wrong for four cycles, and its resolution Anchored to 67548c2; baseline 267426a. Delta 4 commits (#270-#273). #271 decided the standing backlog and rejected a recommendation this audit had carried since cycle 6: trusted publishing needs id-token on the job running cargo publish, which builds the crate and so executes build.rs — and repository code holding an OIDC token can request any audience including sigstore, forging the provenance the pipeline exists to make unforgeable. The rejection is correct. I recommended OIDC in cycles 6, 8, 9, 11 and 12 while praising, in the same reports, the SLSA L3 isolation whose premise is that no job holding signing credentials executes repository code. Two individually-correct recommendations in direct tension, unnoticed for four cycles, because each was checked against best practice and neither against the other. Contribution (tested, with a control): the incompatibility is contingent, not inherent. cargo publish packages, then verifies by building; only the verification runs build.rs, and it is switchable. cargo package --no-verify -> build.rs did NOT run cargo package -> build.rs DID run So: run verifying `cargo package` in the already-isolated build job, and `cargo publish --no-verify` in the token-holding job. The long-lived CRATES_IO_TOKEN goes away and Build L3 holds. Caveats stated: the packaging check must actually be wired into the build job, and the publish job must run no repository code before publish — a property worth asserting in workflow_signing_isolation_test, the guard F303 showed gets tested. F (LOW, new) — the normalised bot-filter matcher is line-based. #272 closed the spelling axis completely; the layout axis remains, because the guard normalises each line independently, so a call split across lines is never assembled. Tested with a control: single-line qualified CAUGHT, multi-line BOOL_OR(\n is_bot \n) MISSED. Not theoretical — it is the house style in the directory the guard scans (ownership.rs:53-55 wraps SUM( / arg / ) exactly so). Fix: normalise the file once and recover line numbers by byte offset, the technique spa_escaping_test already uses. Verified: #272 fixes the finding, and its diagnosis of why the §12 census missed that guard is sharper than the finding was — the census searched for &[&str] consts and so found one syntactic shape of instance list rather than the class. zizmor adoption paid on contact (three live template-injection sites, F304) and is configured to AGREE with workflow_action_pin_test rather than to fight it. #270 separately caught a container publish reachable from a branch dispatch. Cycle-6 M27 closed: 18/18 <th> carry scope, keyboard activation in four widgets (my first count was wrong — '<th' matches '<thead' — corrected before reporting). Open: gitlink fixture (oldest, only item with no decision recorded), outputSchema 1/11, M8, zizmor not yet a required context. Currency live 2026-08-13: rmcp 3.1.2 and zizmor 1.29.0 both latest; Rust 1.97.1 deferred by decision, not drift. Note: docs/hardening-cycle-12 is still unmerged. * docs(reports): correct the cycle-13 housekeeping claim before landing Section 6 said `docs/hardening-cycle-12` was unmerged and that cycle 12's report had not landed on main. It had: f678d2c added it, swept in by a `git add docs/reports/` in #271 rather than by its own PR, which is why it looked unmerged from the audit side. No such branch exists on the remote. That sweep was recorded as a process error when it happened — a 13 KB file committed and merged unread. This is the second thing it caused: an auditor, reading the branch list rather than the file tree, concluded work was outstanding that was already on main. Everything else in the report was verified against source before landing. Its two substantive claims both hold: the line-based bot-filter matcher misses wrapped SQL, reproduced by planting it, and `cargo publish`'s verification step is what executes build.rs, reproduced on cargo 1.97.1 with a control. --------- Co-authored-by: Emre <emre@valocom.nl>
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.
Researched decision on the one backlog item I'd previously deferred as "your call". It turned out to have a researchable answer for most of it, and a genuine judgement call for one part — which the repository had already made, explicitly.
Why adopt
Not because it's popular — because it found something on first contact: three live template-injection sites (F304, fixed in #270) that nothing in this repo would have caught.
Configured to agree, not to silence
Exactly one audit is configured.
unpinned-usesgets the policy this repository already enforces inworkflow_action_pin_test: full SHAs for third-party actions, tags permitted foractions/*and fordtolnay/rust-toolchain— whose tag names the Rust toolchain, not a release of the action, and whichrust_version_pins_testreads as a version.Two gates disagreeing about pinning is worse than either alone: a contributor gets told to pin by one and told it's fine by the other.
Nothing is suppressed — the drop is entirely
unpinned-usesagreeing with the rule already in force.On the
actions/*divergenceOpenSSF Scorecard asks for first-party actions to be SHA-pinned too, and tj-actions CVE-2025-30066 and reviewdog are why. I kept the exemption: a compromise inside GitHub's own namespace is a compromise of the platform running the job, which a pinned SHA doesn't survive either — and the repo made this call explicitly, with that reasoning recorded in
is_exempt. Overriding a documented, guarded policy on a general principle, when the stricter half is already enforced, isn't an improvement.Advisory, with an exit condition
Same pattern
dogfoodalready uses in this file. The seven remaininghighfindings are adjudicated in F306, not hidden:dangerous-triggers+ 3excessive-permissions— Dependabot auto-merge. Inherent to that pattern, mitigated the documented way: neither stage checks out or executes PR code.excessive-permissions—release.yml's workflow-levelcontents: write. Genuinely reducible (buildalready overrides toread), but it touches the release path, so not in the commit that adds the tool.cache-poisoning— zizmor rates its own confidence Low; poisoning that cache needs push access to the branch that wrote it.The flag comes off once each is resolved or carries a recorded exception. Silencing them to make the gate green on day one would defeat the point of adding it.
Also
Corrects the cycle-12 report's "six-site" toolchain-pin claim to five, which is what
rust_version_pins_test's own doc names.All six workflow guards pass;
ci.ymlitself reports 0 high.clippyandfmtverified by exit status.