Audit the workflows themselves; nothing was checking CI's own config - #98
Merged
Conversation
The pipeline gated the shell thoroughly but never looked at the two files driving it. Two consequences, both measured rather than assumed: ci.yaml had no `permissions:` block, and this repo's default_workflow_permissions is "write" -- so `bootstrap`, which pipes a curl'd script into bash, and `apply`, which executes the whole tree, each ran with a read/write GITHUB_TOKEN. Now set once at the workflow level to contents: read, with persist-credentials: false on every checkout. The actions were pinned to tags, which can be force-moved. CLAUDE.md already made that argument for .chezmoiexternal.yaml; the actions holding the token were the exception. They are now pinned to commits. Hash-pinning is not paperwork. Four zizmor audits inspect the SHA and none can run on a tag pin: impostor-commit (the commit came from that repo, not a fork in its network), ref-version-mismatch (the `# vX.Y.Z` comment matches the commit), stale-action-refs, known-vulnerable-actions. zizmor went from 19 findings to none. Adds a `workflows` job: actionlint for correctness -- it shellchecks `run:` blocks, ~200 lines nothing else saw -- and zizmor for security. Adds ruff to `lint`, which py_compile cannot substitute for. Those four audits are online and fail silently-open: with no GH_TOKEN zizmor reports nothing and exits 0, so a job that lost its token would stay green while checking none of this. tests/fixtures/zizmor/canary.yml pins a real commit under a deliberately wrong version comment, and the job fails unless ref-version-mismatch fires on it. bump-deps grows an ACTION pin class that rewrites the commit and its version comment together -- a comment left to rot into a lie is worse than a tag pin, since it is what a human reads. Its `files` is a list, which closes a pre-existing gap: only ci.yaml was registered, so an --apply would rewrite five pins there, leave deps.yaml behind, and then report "all pins current". Three things the linters found rather than I did: - ci.yaml ran `run: "$HOME/.../install.sh"`. YAML consumes the quotes, so bash received the path bare and split it on any space in $HOME. - Two dead imports in bin/bump-deps that py_compile cannot see. - Two SC2016 hits in deps.yaml, both intentional Markdown backticks, scoped to the compound command rather than silenced per line. And one I introduced: the new invariant first used re.search over the whole file, so it passed as long as one well-formed pin existed anywhere in it -- reverting one of six checkouts slipped through. Caught while proving the gate could fail, fixed, and pinned down by test_one_bad_pin_among_several_good_ones_is_caught. All five documented breakages were verified to turn the suite red.
…esult zizmor exits 13 when it has findings, and the canary fixture is built to produce one -- so under `bash -e` the step died on the scan itself, before the assertion that reads the JSON ever ran. zizmor's docs state that --format=json already suppresses the 11+ exit codes. As of v1.28.0 it does not. The real scan keeps its exit-code gate; only the canary opts out. My local check missed this because it ran zizmor and the assertion as two separate commands with no `set -e`, so zizmor's status was discarded. Re-verified through a faithful copy of the step under `bash -e`: passes with a token, and fails both when the token is absent (zizmor warns that it is "running in offline mode by default" and reports nothing) and when the fixture's version comment is corrected.
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
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.
Adds zizmor (security) and actionlint (correctness) over the two workflow files, plus ruff over the Python. Declines CodeQL — its supported languages don't include shell, which is ~95% of this repo's executable surface, and on workflows zizmor has far more audits.
Why
Two things, both measured rather than assumed:
ci.yamlhad nopermissions:block, and this repo'sdefault_workflow_permissionsiswrite. Sobootstrap(pipes a curl'd script into bash) andapply(executes the whole tree) each ran with a read/writeGITHUB_TOKEN.CLAUDE.mdalready makes that argument for.chezmoiexternal.yaml; the actions holding the token were the exception.zizmor on the original tree: 19 findings — 7 High
unpinned-uses, 6 Mediumexcessive-permissions, 6 Lowartipacked. After this change: none.Hash-pinning is the point, not the paperwork
Four zizmor audits inspect the commit, and none can run on a tag pin:
impostor-commitactions/checkout@<attacker-fork-sha>resolves and looks legitimate.ref-version-mismatch# vX.Y.Zcomment corresponds to that commit — which kills the usual objection that SHA pins turn the comment into an unverified lie.stale-action-refsknown-vulnerable-actionsThe canary
Those audits are online and fail silently-open: with no
GH_TOKEN, zizmor reports nothing and exits 0. A job that lost its token would stay green while checking none of this.tests/fixtures/zizmor/canary.ymlpins a real commit under a deliberately wrong version comment; the job fails unlessref-version-mismatchfires. Same reasoning as thetestjob's Assert nothing was skipped step.bump-depsgains anACTIONpin classRewrites the commit and its version comment together — a comment left to rot is worse than a tag pin, since it's what a human reads. Its
filesis a list, closing a pre-existing gap: onlyci.yamlwas registered, so--applywould rewrite five pins there, leavedeps.yamlbehind, and then report "all pins current".Found by the linters, not by me
run: "$HOME/.../install.sh"— YAML consumes the quotes, so bash received the path bare and split it on any space in$HOME.bin/bump-depsthatpy_compilecannot see.SC2016hits indeps.yaml, scoped to the compound command.And one I introduced: the new invariant first used
re.searchover the whole file, so it passed as long as one well-formed pin existed anywhere in it — reverting one of six checkouts slipped through. Caught while proving the gate could fail, and pinned down bytest_one_bad_pin_among_several_good_ones_is_caught.Verification
zizmor/actionlint/ruffclean;bump-deps --self-testand--checkpass;./tests/rungreen at 56 tests with nothing skipped. All five breakages now documented intests/README.mdwere verified to turn the suite red, including both canary failure modes (corrected fixture, and absent token).Two things left for you
disabled(free on a public repo) — complements gitleaks by blocking at push time rather than only auditing history. Optionally flipdefault_workflow_permissionstoreadas defense-in-depth behind the new explicit blocks.GITLEAKS_ENABLE_COMMENTS: falseis set so thesecretsjob stays atcontents: read. If you'd rather keep PR comments, that job needspull-requests: writeinstead.Pre-existing and untouched:
zsh-completionsis 2 commits behind in.chezmoiexternal.yaml, sobump-deps --checkstill exits 1. That's anEXTERNALpulling unread upstream code and wants a deliberate confirmation, so I left it alone.