fix(ci): check-action-pins matches quoted uses scalars, anchors SHA (#2669) - #2715
Conversation
…2669) The scanner only matched bare `uses:` scalars and accepted any 40-hex run inside the value, so a quoted pin was skipped entirely and `@<sha>oops` passed as a valid pin. - extract_pin() recognises bare, 'single-quoted' and "double-quoted" scalars - the ref must span the WHOLE scalar and end in exactly 40 hex chars - scan set now also covers action manifests tracked outside .github/ (dedupe makes the overlap harmless) - offline `--extract` mode exposes the matcher for tests (no gh, no network) Closes #2669
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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: 159bce46c8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if [[ $line =~ $uses_dquoted_re ]]; then | ||
| scalar="${BASH_REMATCH[1]}" | ||
| elif [[ $line =~ $uses_squoted_re ]]; then | ||
| scalar="${BASH_REMATCH[1]}" | ||
| elif [[ $line =~ $uses_bare_re ]]; then |
There was a problem hiding this comment.
When a bare uses: value has a trailing comment that itself contains a quoted uses: example, the double-quoted regex wins because it is searched across the whole line before the bare form. For example, uses: actual/bad@<sha> # previous uses: "comment/good@<sha>" checks only comment/good, so a resolvable historical reference can hide an unresolvable action that the workflow actually executes. Match the YAML key's immediate scalar before considering its quoting style.
Useful? React with 👍 / 👎.
| # treats quoted regex fragments as literals inside [[ =~ ]]. | ||
| uses_dquoted_re='uses:[[:space:]]*"([^"]*)"' | ||
| uses_squoted_re="uses:[[:space:]]*'([^']*)'" | ||
| uses_bare_re='uses:[[:space:]]*([^[:space:]#]+)' |
There was a problem hiding this comment.
When a workflow uses a valid YAML flow mapping such as - { uses: owner/repo@<sha>, name: Checkout }, this regex includes the comma in scalar, causing the anchored pin regex to reject the reference and silently skip its resolvability check. The previous unanchored matcher did extract such pins, so the bare-scalar boundary should also account for YAML flow delimiters such as ,, }, and ].
Useful? React with 👍 / 👎.
Two regressions in the #2669 matcher, both from matching the whole line: - a quoted pin inside a trailing comment shadowed the real bare pin, because the unanchored uses: patterns re-matched at the comment; - a flow-mapping step (`- { uses: owner/repo@<sha>, name: X }`) was silently skipped, because the comma stayed glued to the bare scalar and the anchored pin_re then rejected it. Cut to the first `uses:` key, ltrim, and match the three scalar spellings anchored on that remainder; bare scalars now also terminate at `,` and `}`. Covered by two regression tests that fail on the pre-fix script. bash 3.2 compatible; the live run still reports the same 10 ok pins.
1 participant
Closes #2669 (follow-up deferred out of PR #2660, CodeRabbit Major on
scripts/check-action-pins.sh:20).Problem
The scanner matched exactly one YAML spelling and never anchored the SHA:
uses: 'owner/repo@<sha>'anduses: "owner/repo@<sha>"were skipped — a quoted pin was invisible to the resolvability gate.{40}accepted any 40-hex prefix, souses: owner/repo@<sha>oops"resolved" against the first 40 hex chars while the workflow itself would fail at runtime withUnable to resolve action.No current exposure (all 10 pins in
.github/workflowsare bare and resolve), so this is pure hardening of theAction Pin Resolvabilitygate before someone quotes a pin.Change
scripts/check-action-pins.shextract_pin()picks the scalar out of all three spellings — bare,'single',"double"— then requires the ref to match^owner/repo(/subpath)?@[0-9a-f]{40}$, i.e. the SHA must terminate the scalar. Trailing# v5comments and CRLF endings still parse; local (./…),docker://, tag and branch refs are still ignored.action.yml/action.yamlanywhere in the tree (issue point 3), not just.github/workflows+.github/actions. Overlap is harmless — pins are deduped as before.--extractmode reads YAML lines from stdin and prints the pins the matcher accepts. Nogh, no network — this is what makes the regex layer testable.tests/integration/check-action-pins-matcher.test.ts(new, 9 tests / 151 assertions)Feeds fixture lines through
--extract: accepts bare/single/double-quoted, subpath (owner/repo/sub@sha→owner/repo@sha), trailing comments, CRLF; rejects@<sha>deadbeef,@<sha>oopsin both quote styles, 39-hex,@v5,@main,./…,docker://…. A final case replays every SHA-pinneduses:line from the real.github/workflowsand asserts each one still extracts, so the parity with today's behaviour is locked.Evidence
Caveat
--extractdeliberately reportsowner/repo@shafor subpath actions, dropping the subpath: the commit being resolved lives in the parent repository, which is whatgh api repos/<repo>/commits/<sha>needs and what the dedupe key has always used.