ci: pin action hashes and escape variables with minimum permission#441
ci: pin action hashes and escape variables with minimum permission#441zimeg merged 2 commits intoslackapi:mainfrom
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #441 +/- ##
=======================================
Coverage 99.85% 99.85%
=======================================
Files 7 7
Lines 709 709
=======================================
Hits 708 708
Misses 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zimeg
left a comment
zimeg
left a comment
There was a problem hiding this comment.
💌 Notes for the kind reviewers!
These might be patterns common across other repos that I hope to follow up with, and I might reference this pull request elsewhere or perhaps write those comments separate. In either case, I'm looking forward to improving workflow securities around 🔏
| permissions: | ||
| contents: read |
There was a problem hiding this comment.
📝 These permissions limit the default $GITHUB_TOKEN but the generated app token used for opening a PR later has the scopes of the app.
If this does cause strangeness, I will be quick to follow up 🫡
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | ||
| with: | ||
| persist-credentials: false |
There was a problem hiding this comment.
📝 A similar note about this token not being required for more than a checkout here!
| rsync -av --delete ./docs/ "./docs_repo/content/$REPO/" | ||
| env: | ||
| REPO: ${{ github.event.repository.name }} |
There was a problem hiding this comment.
🗣️ This is a common pattern used to avoid script injections! We make user inputs an environment variable and also quote the variable when used for inline scripts.
| name: Tests | ||
| on: | ||
| pull_request_target: | ||
| pull_request_target: # zizmor: ignore[dangerous-triggers] |
There was a problem hiding this comment.
📝 To test the requested changes of a PR, pull_request_target is required.
🔗 https://docs.zizmor.sh/audits/#dangerous-triggers
I am hoping we can add the zizmor tool as a check of slackapi/slack-health-score soon, but also want to make it obvious that this pattern is known to exist here 🙏 ✨
| author: ${{ github.event.sender.login }} | ||
| channel_id: ${{ secrets.SLACK_CHANNEL_ID }} | ||
| event_url: ${{ env.EVENT_URL}} | ||
| event_url: ${{ steps.push.outputs.url || steps.pull_request.outputs.url }} |
There was a problem hiding this comment.
📣 TIL writing to $GITHUB_ENV is not a good practice!
🔗 https://docs.zizmor.sh/audits/#github-env
Some workaround was required to keep the noted step as a single step with various possible inputs, but this continues to match expectations:
push: https://github.com/zimeg/slack-github-action/actions/runs/15056293205/job/42322838794#step:12:8pull_request_target: https://github.com/zimeg/slack-github-action/actions/runs/15056529213/job/42323524886#step:12:8
Both links above use changes on a fork's main branch!
|
📣 I meant to note that the test changes of this PR are not reflected in the actual status with |
There was a problem hiding this comment.
This might be a hot take but I don't think we need to pin the hashes, I think the readability of the raw version outweighs the benefits of pinning hashes and leaving comments for the actual version, I'm concerned about how dependabot handles updating these dependencies 
Could you explain what are the security improvements gained by using the hash rather then referencing the version and share any documented cases of critical vulnerability in this area 🙏
It may also be worth considering what is at risk here, as far as I know these Github Actions don't have access to our package release keys, I don't think we should be building a "complex security vault" if we are leaving it empty
|
@hello-ashleyintech @WilliamBergamin Thank y'all both for reviewing this! 🔐 @WilliamBergamin I agree a tag is more simple, but tags can be changed which can result in supply chain problems if unexpected In this project, the permissions:
contents: writeWe're using trusted and reviewed actions at this time. But for the sake of this example if the current In a less theoretical scenario, CVE-2025-30066 shows this happening a few months ago elsewhere - writeup here 📚 Using a commit hash removes these possibilities altogether and is recommended as a best practice as well. IMHO we should consider recommending this within the example code samples of this project, but this might be a discussion for later. FWIW @dependabot is kind too and automates the hash update and also the version comment at the time of update! |
WilliamBergamin
left a comment
WilliamBergamin
left a comment
There was a problem hiding this comment.
Thanks for the detailed update, lets try it out
|
@WilliamBergamin This was a surprising case to explore for me, and I hope the caution keeps these actions more secure 👾 @hello-ashleyintech @WilliamBergamin Once more, thank you for the reviews! I believe a @codecov update is fanning out now and I'll report back on these updates soon! Similar updates to other repositories are also in the works. I plan to share these in following hours ⏳ ✨ |
|
Woah! This was so quick but is also showing a hopeful update: #443 😉 |
3 participants
Summary
This PR uses the wonderful
zizmortool to audit our own workflows andpinactfor pinned versioning 👾While not so simple to bump ourselves, the kind @dependabot can help keep these hashes updated with the changes of dependabot/dependabot-core#5951 having landed 🤖 ✨
Reviewers
A similar audit can be performed with the
zizmortool:Notes
Most changes I hope are repetitive, but I will comment on the more significant ones! FWIW the
developandtestworkflow continue to work with these changes:pushpull_requestRequirements