Repository navigation
fix(rules): judge WH013 per job, and skip writes made with their own credential - #942
Conversation
Governance's Validate Hypatia Baseline and the Hypatia Security Scan build the scanner at the reusables' default hypatia-ref, 51ab649. That commit's ActionsLock parser accepts only v0.0.2, so this repo's tool-owned v0.0.3 actions.lock fails closed as two high invalid_actions_lock findings on every run. Pass hypatia-ref aa34a4c (main after #938, which adds v0.0.3 support) as an input. The `uses:` refs are unchanged, so actions.lock is untouched. This is the value standards#1315 moves the defaults to; drop the input once the standards callers are re-pinned past that PR. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CYR5bf5FBLS7cXQK72aVNM
…credential WH013 reported a write as starved whenever the file had no `contents: write` grant, including jobs that write with their own credential. That grant only governs GITHUB_TOKEN, so a PAT or app-token write cannot be starved by a read-only grant. Measured false positives: - hypatia estate-rescan.yml: PAT clone of verisimdb-data and push (high) - hypatia inbox-steward-intake.yml: checkout with FARM_PAT (high) - standards github-backup-mirror-reusable.yml: PAT push to the backup repo Writes are now judged per job. A job is credentialed when `token:`, `GH_TOKEN:` or `GITHUB_TOKEN:` is set to a secret other than secrets.GITHUB_TOKEN, or to a step-minted outputs.token. Starvation and its masked :high arm consider only GITHUB_TOKEN jobs. A credentialed write that is masked (2>/dev/null, || true, || echo) is still reported, at :warn with fix recipe unmask-credentialed-write, since a credential that expires would fail it silently. Tests: 5 new cases; 61/61 pass, and 3 of the new cases fail against the previous module. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CYR5bf5FBLS7cXQK72aVNM
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 30 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
…943) ## Summary PR 3 of the baseline-gate stack, stacked on #942. Fixes the true positive left after WH013's precision fix: four real defects in `.github/workflows/inbox-steward-intake.yml`. | Defect | Effect | Fix | |---|---|---| | `update-ruleset` appended ` :: TODO: Apply rule update: …` lines to **`.hypatia-baseline.json`** | Corrupts the JSON the Governance baseline gate reads, on every critical/high suggestion | Suggestions are surfaced as `::notice` annotations only. The `data/ruleset-updates/*.json` record (`applied: false`) is unchanged. | | Every `git commit` / `git push origin HEAD` was masked (`2>/dev/null \|\| true`, `\|\| echo "::warning::…"`) and went **straight to main** | A refused write stayed green and bypassed review and signing | New `scripts/open-data-pr.sh`: branch, PR, squash auto-merge (the `estate-rescan.yml` pattern). Every failure exits non-zero. An empty change is skipped with `git diff --cached --quiet`. | | `record-report` committed only when `inputs.dry_run == "false"`; the input is **empty on `repository_dispatch`** | Every real dispatch ran as a dry run, while `update-ruleset` (`!= 'true'`) did write | Only an explicit `'true'` is a dry run, matching `update-ruleset`. | | The `.git-private-farm` dispatch discarded stderr and turned a refusal into a warning | A failed hand-off stayed green | Fails the job. | Context: none of this has produced data. `data/inbox-steward-reports/` and `data/ruleset-updates/` don't exist on main, and the last 17 runs (to 2026-07-05) all failed. ## Type of change - [x] 🐛 Bug fix - [x] 🔧 Build / CI / tooling ## How has this been verified? - `wh013_permission_starved_write/1` (with #942) on this tree returns `[]`, down from 1 `:warn` `unmask-credentialed-write` finding. - `actionlint` 1.7.7: the same 14 messages before and after (diffed). All 14 are pre-existing and sit in the `summary` job; see notes. - `shellcheck scripts/open-data-pr.sh`: clean. - I sandbox-ran `open-data-pr.sh` against a bare local remote with a stub `gh`: - publish pushes `inbox-steward/123-report` and calls `gh pr create … --base main` then `gh pr merge 77 --squash --auto --delete-branch` (rc 0); - with nothing to commit it exits 0 without a PR; - an unreachable remote gives rc 128. ## Checklist - [ ] My commits are **signed**. The session's signing shim is platform-mediated. - [x] I ran the project's own checks/tests locally and they pass. - [x] New files carry the correct SPDX (`scripts/open-data-pr.sh`: MPL-2.0). - [x] Docs are updated, and no public claim now overstates what the code does. - [x] I have not introduced a soundness hole. ## Notes for reviewers - **Needs:** `FARM_PAT` with Contents **and** Pull requests write on this repo, plus *Allow auto-merge* enabled. If either is missing, the job now fails visibly instead of warning. - **Pre-existing, not fixed here:** the `summary` job builds its step summary with `${{ … && echo '…' || echo '…' }}`. That is shell inside an expression, and actionlint rejects it ("parser did not reach end of input"). Left out to keep this PR to the write path. --- _Generated by [Claude Code](https://claude.ai/code/session_01CYR5bf5FBLS7cXQK72aVNM)_ Co-authored-by: Claude <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb15dcec17
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- A file whose jobs the extractor cannot split is judged as one body and
is never credentialed. One job's PAT can no longer excuse another job's
write.
- `jobs: # comment` now splits jobs, so that fallback is rarer.
- A workflow-level `env:` credential (`GH_TOKEN: ${{ secrets.X }}`)
credentials every job.
- `secrets['NAME']` counts as an own credential;
`secrets['GITHUB_TOKEN']` does not.
- masked_write?/1 also sees `gh release create|upload|edit`.
Tests: 4 new cases; 64/64 pass, and the 4 fail against the previous
commit. WH013 on hypatia, standards and echidna: no findings, and no
workflow there has a commented `jobs:` header.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CYR5bf5FBLS7cXQK72aVNM
) ## Summary PR 4 of the baseline-gate stack, stacked on #942 (which now also carries #943). It clears all 3 RE005 findings at their source: one false positive in the rule, and two masks removed from workflows. | Finding | Verdict | Fix | |---|---|---| | `tests.yml:763` `trap 'kill "$STRESS_PID" 2>/dev/null \|\| true' EXIT` | **Rule false positive.** The EXIT trap runs after the step's status is decided. Under `set -e`, an *unmasked* failing `kill` turns a **passing** step red (measured: rc 1). The `\|\| true` is what keeps it honest. | RE005 skips `trap …` lines (`lib/rules/research_extensions.ex`). The same false positive is in echidna `server-boot-gate.yml:40`. | | `tests.yml:192` `$(find … -exec grep … \| grep -v … \|\| true)` | **Real, small mask.** It also swallowed grep's exit **2**: with a missing directory the old step reports PASS. | Rewritten as `grep -r --include … --exclude-dir=rules` with an explicit status test (1 = no match = pass, >1 fails). Comment lines are removed with `sed`, which can't exit 1 on "all filtered". | | `build-gossamer-gui.yml:189` `ls … 2>/dev/null \|\| true` right before `exit 1` | The mask did nothing except hide the diagnosis | Mask dropped | ## Type of change - [x] 🐛 Bug fix (a rule false positive, plus two CI masks) ## How has this been verified? - **Rule:** I compiled `research_extensions.ex` with `scanner_suppression.ex`, `cicd_rules.ex` and `security_errors.ex` standalone (Elixir 1.14; hex.pm is unreachable here) and ran `test/research_extensions_test.exs`: - **42/42** pass; - control: the new trap test fails against the previous module, and the "a trap does not hide a real swallow" test passes on both; - `mix format --check-formatted` passes. - **Blast radius (`re005_test_swallows_exit/1`, old → new):** - hypatia: 3 → **0**; - standards: 0 → 0; - echidna: 2 → 1 (the trap in `server-boot-gate.yml` clears; `live-provers.yml:475` is a real finding and remains). - **The `tests.yml` step**, extracted with `yq` and run on this tree: - clean → rc 0 PASS; - planted `believe_me` in `src/` → rc 1 FAIL; - planted in `lib/rules/` → PASS (excluded, as before); - `-- believe_me` comment → PASS; - a missing search directory → **rc 2 FAIL**, where the old step reports PASS. - `actionlint` 1.7.7: identical message sets before and after on both files (6 pre-existing in `tests.yml`, 0 in `build-gossamer-gui.yml`). ## Checklist - [ ] My commits are **signed**. The session's signing shim is platform-mediated. - [x] I ran the project's own checks/tests locally and they pass (scoped as above). - [x] New files carry the correct SPDX (none added). - [x] Docs are updated (the RE005 `@doc` states the trap rule). - [x] I have not introduced a soundness hole. Only whole `trap …` lines are skipped, and a real swallow elsewhere in the same step still fires (test). --- _Generated by [Claude Code](https://claude.ai/code/session_01CYR5bf5FBLS7cXQK72aVNM)_ Co-authored-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2523093176
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| exit 0 | ||
| fi | ||
|
|
||
| branch="inbox-steward/${GITHUB_RUN_ID}-${suffix}" |
There was a problem hiding this comment.
When a workflow attempt pushes this branch but fails before successfully queuing or completing the PR, rerunning it reuses the same GITHUB_RUN_ID. The fresh checkout creates a different local history under the identical branch name, so the subsequent push is rejected as non-fast-forward and the rerun cannot recover automatically. Include GITHUB_RUN_ATTEMPT in the branch name or explicitly resume/update the existing branch and PR.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #947. The branch is now inbox-steward/<run_id>-<run_attempt>-<suffix>.
Generated by Claude Code
| # turn a PASSING step red, so the `|| true` is what keeps it honest. | ||
| has_or_true? = | ||
| stripped_body | ||
| |> String.replace(~r/^[ \t]*trap[ \t].*$/m, "") |
There was a problem hiding this comment.
For a multiline run block containing a valid same-line sequence such as trap 'cleanup' EXIT; cargo test || true, this replacement deletes the entire physical line rather than only the trap handler, so RE005 returns no finding even though the test failure is swallowed. Strip only the quoted trap command, or retain and scan commands following the trap registration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #947 (this PR has merged). Only the quoted trap handler is dropped now, so trap '…' EXIT; cargo test || true is reported. There's a test for it.
Generated by Claude Code
| [] -> | ||
| if performs_contents_write?(content), do: [{content, false}], else: [] | ||
|
|
||
| jobs -> |
There was a problem hiding this comment.
When a valid YAML comment appears between job definitions, extract_job_blocks/1 stops at that comment and returns only the jobs preceding it. Because this nonempty partial result is accepted here as complete, any later job is omitted from WH013 entirely; for example, a read-only test job followed by # Publish and then a permission-starved git push job produces no finding. Make the extractor skip comments between jobs or detect incomplete extraction and use a safe fallback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #947. A comment between jobs no longer ends the jobs: block, and a test covers your exact shape (a test job, # Publish, then a starved push job). The full WorkflowHardening scan of hypatia, standards and echidna is unchanged.
Generated by Claude Code
| masked_credentialed = | ||
| if Enum.any?(credentialed, &masked_write?/1), | ||
| do: [wh013_masked_credentialed_finding(rel)], |
There was a problem hiding this comment.
When a credentialed write step uses continue-on-error: true, a failed push or release leaves the workflow green, but masked_write?/1 only recognizes shell-level redirection and || patterns. The job is removed from token_writes as credentialed and this check returns false, so WH013 emits no warning at all when the PAT expires or loses scope. Include step-level continue-on-error: true when determining whether a credentialed write is masked.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #947, for step-level continue-on-error: true. Job-level isn't counted, because it's a deliberate, visible job setting. Standards' github-backup-mirror-reusable.yml documents one and surfaces refusals itself, and counting it would have added a warning there. Both cases have tests.
Generated by Claude Code
## Summary Follow-up to #942. Codex's second review of #942 posted 4 findings around the time it merged, so the fixes land here. | # | Finding (Codex, on #942) | Fix | |---|---|---| | 1 | RE005 dropped the **whole line** after `trap`, so `trap '…' EXIT; cargo test \|\| true` went unreported | Drop only the trap's quoted handler (`trap '…'` / `trap "…"`) | | 2 | `extract_job_blocks/1` **stopped at a comment between jobs** (` # Publish`), so later jobs were never judged | Comment lines no longer end the `jobs:` block. This affects WH013 and every rule built on the extractor | | 3 | A credentialed write under `continue-on-error: true` was not seen as masked | **Step-level** `continue-on-error: true` on a write now counts as a mask (see note) | | 4 | `open-data-pr.sh` reused `inbox-steward/<run_id>-…` on a re-run, so the push was non-fast-forward | Branch name includes `GITHUB_RUN_ATTEMPT` | **Note on #3:** a first cut counted job-level `continue-on-error` too. That produced a new warning on standards' `github-backup-mirror-reusable.yml`, where the job-level flag is deliberate: the file's header documents it and the job surfaces refusals itself. Codex's finding named step-level, so job-level is left out, and a test covers that case. ## Type of change - [x] 🐛 Bug fix (scanner false negatives) ## How has this been verified? hex.pm is still unreachable from this session. I compiled the touched rule modules with `scanner_suppression`, `cicd_rules`, `security_errors` and `actions_lock` standalone (Elixir 1.14) and ran `test/workflow_hardening_test.exs` with `test/research_extensions_test.exs`: - **110/110** pass; - control: 3 of the 4 new tests fail against `main` (the trap line, the inter-job comment and step-level `continue-on-error`). The job-level test passes on both. `mix format --check-formatted` passes on all four Elixir files. **Blast radius:** I compared old and new on hypatia, standards and echidna, using `WorkflowHardening.scan/1` (every WH rule), `wh013_permission_starved_write/1` and `re005_test_swallows_exit/1`. The output is **identical** for all three: no finding added or removed. The only remaining one is echidna `live-provers.yml:475` RE005, which is a real finding. ## Checklist - [ ] My commits are **signed**. The session's signing shim is platform-mediated. - [x] I ran the project's own checks/tests locally and they pass (scoped as above). - [x] New files carry the correct SPDX (none added). - [x] Docs are updated (`masked_write?/1` @doc; script header). - [x] I have not introduced a soundness hole. --- _Generated by [Claude Code](https://claude.ai/code/session_01CYR5bf5FBLS7cXQK72aVNM)_ Co-authored-by: Claude <noreply@anthropic.com>
## Summary This is the last step of the baseline-gate stack. `governance.yml` and `hypatia-scan.yml` pass `hypatia-ref: aa34a4c` (#940), which predates the rule fixes that clear this repo's own false positives. This PR moves both to **hypatia `main` at `23dc74d`**, which contains: - #938: ActionsLock accepts v0.0.3 locks (already in `aa34a4c`); - #942: WH013 judges writes per job and skips PAT/app-token writes. This clears `estate-rescan.yml`; `inbox-steward-intake.yml` was also repaired in #943; - #947: RE005 skips `trap` handlers, and job extraction survives comments between jobs. `uses:` refs are unchanged, so `actions.lock` is untouched. ## What this PR's governance run will show This PR's own `governance / Validate Hypatia Baseline` run is the measurement: it builds the scanner at `23dc74d`. The jump from `aa34a4c` also brings #931's new rules (WF026, WF027, PIN-003), which haven't been measured on this repo yet. hex.pm is blocked in my session, so I can't build the escript to pre-run them. **Expected remaining findings, by class:** - **RE001 (harden-runner):** most of the original 18 jobs, including the 2 dispatched label workflows, which must stay action-free. - These need an owner decision; the harden-runner steps were removed again in #948 because the lock has no record. - **WH013, RE004, RE005, `invalid_actions_lock`:** expected 0. I'll post the measured list here once the run completes. ## Type of change - [x] 🔧 Build / CI / tooling ## How has this been verified? - `actionlint` 1.7.7 is clean on both files. - `23dc74d` is `origin/main` at the time of writing. `git log aa34a4c..23dc74d` contains #942, #947, #945, #931, #941 and #936. ## Checklist - [ ] My commits are **signed**. The session's signing shim is platform-mediated. - [x] I ran the project's own checks/tests locally and they pass. - [x] New files carry the correct SPDX (none added). - [x] Docs are updated (the pin comment names what it carries). - [x] I have not introduced a soundness hole. --- _Generated by [Claude Code](https://claude.ai/code/session_01CYR5bf5FBLS7cXQK72aVNM)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
2 participants
Summary
PR 2 of the baseline-gate stack, stacked on #940.
WH013 ("permission-starved write") fired whenever a workflow writes and the file grants no
contents: write. That grant only governsGITHUB_TOKEN, so it fired on jobs that write with their own credential. Measured false positives:estate-rescan.ymlHYPATIA_DISPATCH_PATin a clone URL, pushing to verisimdb-datainbox-steward-intake.ymlactions/checkouttoken: FARM_PATunmask-credentialed-writegithub-backup-mirror-reusable.ymlBACKUP_MIRROR_TOKENPAT push to the backup repoChanges:
extract_job_blocks/1). A file the extractor can't split falls back to whole-file judgment, as before.token:,GH_TOKEN:orGITHUB_TOKEN:is set to a secret other thansecrets.GITHUB_TOKEN, or to a step-mintedsteps.<id>.outputs.token(GitHub App).own_credential_job?/1is public and has unit asserts.:higharm consider onlyGITHUB_TOKENjobs. A PAT in one job never excuses aGITHUB_TOKENpush in another; there is a test for that.2>/dev/null,|| true,|| echo) is still reported, at:warnwith its own recipe. A credential that expires or loses scope fails silently, but no grant is missing.The remaining inbox-steward warn is a real defect, fixed in the next PR of the stack.
Type of change
How has this been verified?
mix deps.getcan't reach hex.pm from this session, so I compiledactions_lock.exandworkflow_hardening.exstandalone with Elixir 1.14 and rantest/workflow_hardening_test.exsunder ExUnit (seed 0):own_credential_job?cases). The other two (secrets.GITHUB_TOKENis not a credential; a PAT in one job doesn't excuse another job) hold on both, as they should.mix format --check-formattedpasses on both files. The formatter changed only the edited region.wh013_permission_starved_write/1against the real hypatia, standards and echidna trees with the old and new modules; that produced the table above.CI's
mix testis the first full-suite run.Checklist
GITHUB_TOKENanyway; this is documented onown_credential_job?/1.Notes for reviewers
The gate on this PR still runs the pinned scanner (
aa34a4cfrom #940), so this change won't show in its own baseline result. A final pin bump after the rule PRs merge picks it up.Generated by Claude Code