fix: pin 6 actions to commit SHA, extract 12 expressions to env vars#34380
fix: pin 6 actions to commit SHA, extract 12 expressions to env vars#34380dagecko wants to merge 7 commits into
Conversation
📝 WalkthroughWalkthroughMultiple GitHub Actions workflows are updated to pin external action versions to specific commit SHAs instead of floating semantic version tags, and to refactor secrets and GitHub context references from inline interpolation into step-level environment variables. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
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 |
Did some research into the CodeQL envvar-injection-critical guidance (https://codeql.github.com/codeql-query-help/actions/actions-envvar-injection-critical/) and wanted to add this additional change to prevent shell injection through attacker-controllable values like ref names and workflow inputs, and to prevent unexpected behavior from special characters in secret values. Before: echo ${REF_NAME} After: echo "${REF_NAME}"
huang-julien
left a comment
huang-julien
left a comment
There was a problem hiding this comment.
Hi 👋 nice catch. It's indeed better to switch to sha commit, thanks.
I left you a small comment.
|
@huang-julien just circling back — the - Chris |
| @@ -26,7 +28,9 @@ jobs: | |||
| steps: | |||
| - uses: actions/checkout@v4 | |||
| @@ -1 +1 @@ | |||
| # | |||
There was a problem hiding this comment.
This is an auto-generated file. It probably should not be touched, the fix should happen upstream. Otherwise, next generation will erase this PRs changes.
There was a problem hiding this comment.
Same for code-simplifier.lock.yml
|
Hey @huang-julien, good call on the lock files. You're right that those are gh-aw generated and the fix should happen upstream. We filed a report directly with the gh-aw team (github/gh-aw#24743) and they were great to work with, resolved it the same day. The fix is in the code generator itself so all downstream lock files will be hardened on the next generation. They also incorporated our tool Runner Guard into gh-aw's pipeline (github/gh-aw#24749) as an additional layer of continuous analysis. If you're using gh-aw you can add Happy to drop the 2 lock files from this PR so the other 7 workflow hardening fixes can go through cleanly. Let me know.
|
|
Yup, please revert the changes on the gh aw files and then this should be good to merge. Thank you ! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/handle-release-branches.yml (1)
51-53: Extraction of inline expressions is incomplete.Several
run:blocks still interpolate GitHub context /needs.*outputs directly into shell (line 52 usessteps.next-version.outputs.prop; lines 68–71 and 89 useneeds.*.outputs.branch). For consistency with the rest of this PR's hardening and to keep the same defense-in-depth posture the PR aims for, consider moving these to step-levelenv:as well. The values flow from internal outputs (git refs / package.json version), so risk is low, but the extraction pattern should be uniform.Also applies to: 66-71, 87-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/handle-release-branches.yml around lines 51 - 53, The run block currently interpolates steps.next-version.outputs.prop inline (used to build NEXT_RELEASE_BRANCH) and other run blocks reference needs.*.outputs.branch directly; extract these outputs to step-level environment variables instead (e.g., set an env entry like NEXT_RELEASE_PROP: ${{ steps.next-version.outputs.prop }} and branch outputs like SOME_BRANCH: ${{ needs.some_job.outputs.branch }}), then reference the env vars inside the run scripts (use $NEXT_RELEASE_PROP / $SOME_BRANCH) so all GitHub context/output interpolation is done in env:, updating the code that computes NEXT_RELEASE_BRANCH and the run blocks that use needs.*.outputs.branch accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/handle-release-branches.yml:
- Around line 51-53: The run block currently interpolates
steps.next-version.outputs.prop inline (used to build NEXT_RELEASE_BRANCH) and
other run blocks reference needs.*.outputs.branch directly; extract these
outputs to step-level environment variables instead (e.g., set an env entry like
NEXT_RELEASE_PROP: ${{ steps.next-version.outputs.prop }} and branch outputs
like SOME_BRANCH: ${{ needs.some_job.outputs.branch }}), then reference the env
vars inside the run scripts (use $NEXT_RELEASE_PROP / $SOME_BRANCH) so all
GitHub context/output interpolation is done in env:, updating the code that
computes NEXT_RELEASE_BRANCH and the run blocks that use needs.*.outputs.branch
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 622f2c0a-7f52-444c-aa9d-a0030a3051b5
📒 Files selected for processing (1)
.github/workflows/handle-release-branches.yml
|
@huang-julien Apologies for the delay in getting back to you, pushing the fixes now:
Should be good to go from our end. Thanks again for the patience on this one. - Chris |
huang-julien
left a comment
huang-julien
left a comment
There was a problem hiding this comment.
Thank you for this PR !
3 participants
Re-submission of #34350. Had a problem with my fork and had to delete it, which closed the original PR. Apologies for the noise.
Summary
This PR pins all GitHub Actions to immutable commit SHAs instead of mutable version tags and extracts expressions from
run:blocks intoenv:mappings.Changes by file
A note on internal action pinning
This PR pins all actions including org-owned ones. Best practice is to pin everything — the tj-actions/changed-files attack was an internally maintained action that was compromised, and every repo referencing it by tag silently executed attacker code. That said, it's your codebase. If you'd prefer to leave org-owned actions unpinned, let us know and we'll adjust the PR.
How to verify
Review the diff — each change is mechanical and preserves workflow behavior:
action@v3becomesaction@abc123 # v3— original version preserved as comment${{ expr }}inrun:moves toenv:block, referenced as$ENV_VARin the scriptI put up some research on this on Twitter and a research site if you want more context. I wrote a scanner called Runner Guard and open sourced it here.
If you have any questions, reach out. I'll be monitoring comms.
- Chris Nyhuis (dagecko)
Summary by CodeRabbit