Repository navigation
fix(rules): close four gaps Codex found in #942 after it merged - #947
Conversation
- RE005: drop only a trap's quoted handler, not the whole line, so `trap '…' EXIT; cargo test || true` is still reported. - Job extraction: a comment between jobs (` # Publish`) no longer ends the `jobs:` block, so later jobs are judged too. This affects WH013 and every rule built on extract_job_blocks/1. - WH013: a step-level `continue-on-error: true` on a write counts as a mask. Job-level continue-on-error does not, because it is a deliberate, visible setting (standards' github-backup-mirror-reusable.yml documents and surfaces it). - open-data-pr.sh: the branch name carries GITHUB_RUN_ATTEMPT, so a re-run does not collide with a branch an earlier attempt pushed. Tests: 4 new cases; 110/110 pass, and 3 fail against main. The full WorkflowHardening scan, WH013 and RE005 report the same findings on hypatia, standards and echidna before and after. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CYR5bf5FBLS7cXQK72aVNM
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing: 5
✨ Finishing Touches
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. |
![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: 30d1434cd0
ℹ️ 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".
| Enum.any?(@write_masks, &Regex.match?(&1, content)) or | ||
| (step_continue_on_error?(content) and performs_contents_write?(content)) |
There was a problem hiding this comment.
When any non-writing sibling step has continue-on-error: true, this job-wide conjunction also treats an unmasked git push or gh release create in another step as masked. WH013 then emits the unmask-credentialed-write or grant-contents-write-to-writing-job-and-unmask recipe even though removing the sibling flag cannot affect the write; associate the flag with the step that actually performs the write.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #950. continue-on-error now counts only on the step that performs the write. A test checks that a sibling step's flag leaves an unmasked credentialed release unreported.
Generated by Claude Code
| has_or_true? = | ||
| stripped_body | ||
| |> String.replace(~r/^[ \t]*trap[ \t].*$/m, "") | ||
| |> String.replace(~r/\btrap[ \t]+(?:'[^'\n]*'|"[^"\n]*")/, "trap") |
There was a problem hiding this comment.
For a double-quoted handler containing escaped quotes, such as trap "kill \"$PID\" 2>/dev/null || true" EXIT, the pattern stops at the first escaped " and leaves the handler's || true in the scanned text. A normally failing test step with only this cleanup trap is therefore reported as swallowing its exit; the handler matcher needs to consume shell escapes while still preserving commands after the closing quote.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #950. The double-quoted handler matcher now consumes backslash escapes, and a test covers trap "kill \"$PID\" 2>/dev/null || true" EXIT.
Generated by Claude Code
| defp step_continue_on_error?(content) do | ||
| lines = String.split(content, "\n") | ||
|
|
||
| case Enum.find(lines, &Regex.match?(~r/^[ \t]*steps:[ \t]*$/, &1)) do |
There was a problem hiding this comment.
In valid workflows using steps: # publication steps, this exact-line match never finds the steps block, so a credentialed write under a literal step-level continue-on-error: true is not reported as masked at all; for token writes it also selects the less accurate starvation-only recipe. Allow a trailing YAML comment here, as the job extractor already does for jobs:.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #950 (this PR has merged). The steps: matcher now accepts a trailing comment, and there's a test for it.
Generated by Claude Code
| # A comment between jobs (` # Publish`) does not end `jobs:`. | ||
| line == "" or indent_of(line) >= jobs_indent + 1 or | ||
| String.starts_with?(String.trim_leading(line), "#") -> | ||
| {acc, true, jobs_indent, append_line(current, line)} |
There was a problem hiding this comment.
A comment at the jobs indentation now keeps extraction active but is also appended to the current job. Because WH013 scans job bodies as raw text, a separator such as # The publisher used to git push directly makes the preceding non-writing job appear to perform a write and can produce a spurious permission-starvation finding. Ignore such comments for the current body while retaining the in-jobs parser state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #950. A comment between jobs keeps the parser in jobs: but no longer joins the job above. A test checks that # … git push origin main after a read-only job gives no finding.
Generated by Claude Code
| Enum.any?(@write_masks, &Regex.match?(&1, content)) or | ||
| (step_continue_on_error?(content) and performs_contents_write?(content)) |
There was a problem hiding this comment.
When continue-on-error: true is the only mask on the writing step, this new branch routes to findings whose reason says the mask is 2>/dev/null, || true, or || echo and whose fix only tells users to remove those shell constructs. The report therefore describes syntax that is absent and does not tell the user to remove the actual step flag; include this mechanism in both WH013 messages and fixes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #950. Both masked WH013 findings now name the step's continue-on-error: true in their reason and fix, and the test asserts both strings.
Generated by Claude Code
## 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>
## Summary Follow-up to #947. Codex's review of #947 posted 5 findings shortly before it merged. CodeRabbit's review of this PR added 3 more, fixed in the second commit. | # | Finding | Fix | |---|---|---| | 1 | (Codex) `continue-on-error: true` on **any** step made the whole job's writes count as masked | Counted only on the **step that performs the write** (`step_bodies/1` splits each `steps:` list into items) | | 2 | (Codex) `steps: # comment` wasn't recognised, so a step-level flag under it was missed | The `steps:` key matcher accepts a trailing comment | | 3 | (Codex) WH013's masked reasons and fixes named only `2>/dev/null` / `\|\| true` / `\|\| echo` | Both masked findings also name the step's `continue-on-error: true` | | 4 | (Codex) A comment between jobs was **appended to the job above**, so `# … git push origin main` read as a write | Comments never join a job body, at any indent (CodeRabbit: an indented one still did) | | 5 | (Codex) `trap "kill \"$PID\" … \|\| true" EXIT` stopped matching at the escaped quote | The handler matcher consumes backslash escapes | | 6 | (CodeRabbit) `continue-on-error: true` inside a step's `run: \|` script was read as the step's flag | The flag counts only on the step's `- ` line or at its key column | | 7 | (CodeRabbit) A step opened by a bare `-` line wasn't split out | A trimmed `-` starts a step | ## Type of change - [x] 🐛 Bug fix (scanner precision: false positives and negatives) ## How has this been verified? hex.pm is unreachable here, so the touched rule modules were compiled standalone (Elixir 1.14) with `scanner_suppression`, `cicd_rules`, `security_errors` and `actions_lock`: - `test/workflow_hardening_test.exs` + `test/research_extensions_test.exs`: **117/117** pass. - Control: 4 of the 5 Codex-finding tests fail against `main`: escaped-quote trap, commented `steps:`, sibling-step flag, inter-job comment. The fifth (wording) is asserted inside the commented-`steps:` test. All 3 CodeRabbit-finding tests fail against the first commit (3133a5e). - `mix format --check-formatted` passes on all four files. - **Blast radius:** the full `WorkflowHardening.scan/1` gives **identical** findings on hypatia, standards and echidna before and after the second commit (6 / 7 / 6). With RE005 included, the first commit was also identical to `main`. ## 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). - [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
Follow-up to #942. Codex's second review of #942 posted 4 findings around the time it merged, so the fixes land here.
trap, sotrap '…' EXIT; cargo test || truewent unreportedtrap '…'/trap "…")extract_job_blocks/1stopped at a comment between jobs (# Publish), so later jobs were never judgedjobs:block. This affects WH013 and every rule built on the extractorcontinue-on-error: truewas not seen as maskedcontinue-on-error: trueon a write now counts as a mask (see note)open-data-pr.shreusedinbox-steward/<run_id>-…on a re-run, so the push was non-fast-forwardGITHUB_RUN_ATTEMPTNote on #3: a first cut counted job-level
continue-on-errortoo. 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
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_errorsandactions_lockstandalone (Elixir 1.14) and rantest/workflow_hardening_test.exswithtest/research_extensions_test.exs:main(the trap line, the inter-job comment and step-levelcontinue-on-error). The job-level test passes on both.mix format --check-formattedpasses 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/1andre005_test_swallows_exit/1. The output is identical for all three: no finding added or removed. The only remaining one is echidnalive-provers.yml:475RE005, which is a real finding.Checklist
masked_write?/1@doc; script header).Generated by Claude Code