fix(selftest): parse the workflow audits, and sweep the last .yml-only discovery sites - #719
Merged
Merged
Conversation
…HA-pin audits Closes #716. Three sites still globbed `*.yml` only, so a `.yaml` workflow bypassed each silently: `run-permissions-docs-tests.py`'s `read_only_workflows` discovery, and `_selftest.yml`'s SUBMODULES_TOKEN and SHA-pin audits. Discovery for the two shell audits is factored into a shared `list-workflow-files.sh` rather than duplicated, and it fails closed on an empty or missing directory --- a caller that received no paths would grep no input and pass having examined nothing.
CLAUDE.md's Tests section gains the `run-list-workflow-files-tests.sh` paragraph, the command-substitution constraint the two audits depend on, and the two new permissions-docs cases. Changelog fragment added.
Addresses all three findings from the pre-push adversarial review (codex). 1. Both audits read grep's exit status as three values, not two. The inline forms used grep as an `if` condition and ended a pipeline in `|| true`, so an unreadable or vanished file reported clean --- contradicting the fail-closed guarantee this PR adds. 2. The audits themselves are now exercised offline, each against a fixture whose violation lives in a `.yaml` file. Testing only the discovery helper left both consumers free to revert to a `*.yml` glob with every check still green, since this repo's real tree carries no .yaml workflow. 3. `expect_output` asserts the helper's exit status, not only its text. Files gha#720 for the pre-existing `- uses:` blind spot the pins fixtures surfaced; pinned as current behaviour rather than widened, since the one-character widening false-positives on a heredoc fixture.
Round 2 of the pre-push adversarial review raised two findings; both dissolve by walking parsed YAML rather than matching workflow text. 1. The SHA-pin audit was blind to the `- uses:` list form, so five real references were exempt (gha#720). Deferring that was wrong for a check whose success line asserts every action is pinned. A parsed walk sees both spellings, and cannot see a `uses:` written inside a `run:` heredoc -- which is why widening the regex was not the fix. 2. `mapfile` needs bash 4, and macOS ships 3.2 at /bin/bash, so both shell audits exited 127 for a maintainer running them directly. Discovery is now one `workflow_discovery` module shared by both audits and by the permissions-docs suite, rather than a glob repeated three times. An unparsable workflow is an error rather than a clean file, which is the same distinction the shell version drew from grep's exit 2.
…tions
Round 3 of the pre-push adversarial review raised three findings.
1. The parsed walk skipped an absent or wrongly-typed `jobs`, `steps`, `with`,
or step entry, so an empty or structurally invalid workflow reported clean
having been walked not at all -- the parsed-walk version of the grep exit-2
conflation round 2 fixed. Each shape is now an error.
2. `startswith("Morrison-Lab/gha")` exempted `Morrison-Lab/gha-evil` from
SHA-pinning: a different repository, under an exemption meaning "ours".
3. `SUBMODULES_TOKEN in value` matched `NOT_SUBMODULES_TOKEN`, blocking a
valid workflow. The test is now the identifier on word boundaries.
Eleven mutations confirmed red across the suite, four of them new here.
… count Round 5 of the pre-push adversarial review raised four findings, all valid. 1. Moving the YAML import to module load broke `--help` on a machine without PyYAML, undoing the lazy-import guarantee the permissions-docs suite had. It is imported inside `load_workflow` again, so the discovery half stays importable without it. 2. The pin test accepted only a 40-character Git commit, so `docker://image@sha256:<digest>` -- a form GitHub supports, and as immutable as a commit -- read as unpinned. A digest-pinned docker ref now passes; a tag-pinned one still fails. 3. The token audit's module docstring and its selftest step name still described a checkout-only check after round 4 deliberately kept it broad. 4. "five real references" was wrong. Five `- uses:` LINES were invisible to the old anchor; two of them sit inside `_selftest.yml` heredocs and are not references. The real count is three, all in altdoc-multiversion-docs.yml. Corrected in the script, CLAUDE.md, and the changelog; gha#720 carries the same error and is corrected there too. Thirteen mutations confirmed red.
Round 6 of the pre-push adversarial review raised two findings, both valid. 1. The parsed token audit visited only `steps[].with.token`, while the regex it replaced matched any line-leading `token:` and so also covered a reusable-workflow caller's job-level `with:` and `secrets:` blocks. That is a coverage regression wearing a refactor's clothes -- the text scan's reach was never written down, so nothing flagged the loss. Both blocks are walked now; `secrets: inherit` is a string and is skipped, not refused. 2. The pin test accepted a commit SHA or an image digest for every reference, so `actions/checkout@sha256:...` and `docker://alpine@<40hex>` both read as pinned though neither resolves. The reference is classified first. Fifteen mutations confirmed red.
…herit Round 7's single finding: skipping any string-valued job-level `with:` or `secrets:` was too broad. `secrets: inherit` is the one legitimate scalar there; a scalar `with:`, or a `secrets:` naming anything else, is malformed, and skipping it left a block the audit never examined reported as clean. A `token:` whose value is a list or mapping is refused on the same reasoning, while a number or boolean -- which an input may legitimately be -- is not.
Round 8 raised two findings, both valid. 1. GitHub's runner resolves an action's inputs case-insensitively, so a `Token:` was the same input while reading as a different key here -- a one-keystroke bypass of the whole audit. Both the job-level and step-level lookups normalize case now. `Submodules-Token` is still a different key, pinned by its own case. 2. CLAUDE.md still said widening the old regex produces "one new hit" two paragraphs after correcting the count to five lines / three references.
d-morrison
changed the title
d-morrison
marked this pull request as ready for review
August 28, 2026 19:23
lint-markdown's list-item splice detector flags a new `- ` item that follows a continuation line with no blank line between them.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…ing "checkout" Addresses both findings from the review of 4a0e9df. 1. The prose said "Eighteen" over a 16-item list, and the real number is 19. It is now a numbered enumeration so the count is checkable against the list rather than asserted over it -- which is what this file's own rule about completeness claims asks for, and which the previous wording failed. 2. The token audit's closing message still said the token "must not gate a checkout", after the audit was deliberately kept broad. It now says no action may take the secret as a general-purpose token, and names the checkout remedy only for the checkout case.
Contributor
|
Claude finished review — View run All 48 tests pass at the PR head, and the closing message now correctly reads "Where the step is a checkout, use..." rather than unconditionally describing the flagged step as a checkout — confirming both prior-round findings are fixed. Review of #719 (round 2)This round's diff ( Verification performed
FindingsNone. Both issues raised in the prior round are resolved by this diff, and I found no new issues in the changed lines — the code logic ( VerdictReady for merge — the prior round's "Needs minor work" status is resolved: the mutation-count documentation defect is fixed (19 claimed, 19 enumerated, all verified against real tests) and the token-audit error message no longer overstates its scope to non-checkout violations. No new issues found in this round's diff. Reviewed commit: 575cf86 |
Contributor
This was referenced Aug 28, 2026
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
1 participant
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.
Closes #716. Closes #720.
Three sites still globbed
*.ymlonly, so a.yamlworkflow bypassed eachsilently:
run-permissions-docs-tests.py's read-only classification, and_selftest.yml'sSUBMODULES_TOKENand SHA-pin audits. The sibling of thejob-guard gap #712 fixed.
What changed
Both shell audits moved out of
_selftest.ymlinto Python scripts that walkparsed YAML rather than matching workflow text, and all four consumers of
the discovery rule -- these two, the permissions-docs suite, and the job-guard
suite #712 fixed separately -- now share one
workflow_discoverymodule.Parsing rather than grepping was not gold-plating; it is what closes #720. A
line-anchored
^\s*uses:sees the continuation form and not- uses: ...,which left three real references exempt from the pin audit (all in
altdoc-multiversion-docs.yml, measured 2026-08-28 againstmainat7719d04). Widening the anchor is not the fix either: two of the five lines it
newly matches are
_selftest.yml's heredoc-written fixture workflows, whichare text inside a
run:block. A parsed walk sees both spellings and cannotreach heredoc content.
Review history
Eight adversarial review rounds ran before this was pushed. Each found real
defects; all are addressed, each with a test and a confirmed mutation:
grepexit 2 read as "clean"; the audits themselves untested; a test ignoring exit status- uses:blind spot;mapfileneeds bash 4, macOS ships 3.2jobs/steps/withshapes skipped;startswith("Morrison-Lab/gha")exemptinggha-evil;SUBMODULES_TOKENmatchingNOT_SUBMODULES_TOKENuses:skipped; the token audit's message overstating its scopedocker://...@sha256:read as unpinned; stale docstrings; "five real references" was wrong -- it is threewith:/secrets:unwalked (a coverage regression against the old grep); crossed pin forms acceptedsecrets: inheritToken:bypassed the auditOne finding was rebutted rather than addressed: round 4 asked to scope the
token audit to
actions/checkout. Narrowing a security audit to one actionname would miss a fork, a wrapper composite, or a rename, and the errors are
asymmetric -- a false negative ships a broken checkout to a consumer, a false
positive costs a PR comment. The misleading message was corrected instead, and
a test now pins the breadth so narrowing it later is a decision rather than a
silent regression.
Tests
run-workflow-audit-tests.py, 48 cases. The ones to keep if it is evertrimmed are the
.yaml-carried violations -- this repo's tree is all.yml,so a discovery regression leaves every other check green -- and the refusals:
an unparsable or structurally malformed workflow is an error, not a clean
file.
Eighteen mutations were confirmed to turn the suites red rather than assumed
to.