ci: pin action hashes and escape variables with minimum permission #100
Conversation
zimeg
added
code health
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #100 +/- ##
=======================================
Coverage 63.32% 63.32%
=======================================
Files 211 211
Lines 22282 22282
=======================================
Hits 14110 14110
Misses 7085 7085
Partials 1087 1087 ☔ 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 on reasons that certain lines were changed for the most curious and kind reviewers!
| # See https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows#pull_request_target | ||
| on: | ||
| pull_request_target: | ||
| pull_request_target: # zizmor: ignore[dangerous-triggers] |
There was a problem hiding this comment.
Heh the comments above explain the reason we use pull_request_target here but this comment makes a hopeful assumption that zizmor can be added to pipelines for future audits 🤖 ✨
| # Number of commits to fetch. 0 indicates all history for all branches and tags. | ||
| # Default: 1 | ||
| # TODO - We should not fetch all history. | ||
| # But we need to fetch the latest tag to run `make test` and `make build`. | ||
| # This is a workaround until we fetch the latest tag using the Makefile. | ||
| fetch-depth: 0 |
There was a problem hiding this comment.
🪓 In this file we do not need to checkout all things!
| # Number of commits to fetch. 0 indicates all history for all branches and tags. | ||
| # Default: 1 | ||
| # TODO - We should not fetch all history. | ||
| # But we need to fetch the latest tag to run `make test` and `make build`. | ||
| # This is a workaround until we fetch the latest tag using the Makefile. |
There was a problem hiding this comment.
🪓 In this file we do need to fetch tags from past commits. This might be a lot of commits, and this TODO can be deceiving...
mwbrooks
left a comment
mwbrooks
left a comment
There was a problem hiding this comment.
✅ LGTM! Thanks for helping to boost our security.
❓ I left one question around dependabot and how we want to handle its PRs going forward, but it's non-blocking to merging this one!
| - name: Gather credentials | ||
| id: credentials | ||
| uses: actions/[email protected] | ||
| uses: actions/create-github-app-token@df432ceedc7162793a195dd1713ff69aefc7379e # v2.0.6 |
There was a problem hiding this comment.
note: Whoa, this is intense! I understand from a security point-of-view (pinning to a dash instead of a tag). Thanks for adding the comment!
question: How will dependabot handle this going forward? Will it try to use a tag?
There was a problem hiding this comment.
@mwbrooks I had the exact same questions! It's nice that we can keep this comment inline for somewhat reasonable understandings - I cannot remember hashes so well... 🔍
How will dependabot handle this going forward? Will it try to use a tag?
AFAICT tags from official releases will be used in these updates and the comment that follows will match. This should match the current updating events, but instead with commit details!
An example update has shown this to be alright and this blog post shared a few more detail 🎃
| rsync -av --delete ./docs/ "./docs_repo/content/$REPO/" | ||
| env: | ||
| REPO: ${{ github.event.repository.name }} |
There was a problem hiding this comment.
📝 Quick note, I find this surprisingly more portable too!
I don't believe the handlebar notation is supported in bash, but I do know environment variables are 😉
|
@mwbrooks Thanks for the quick and curious review! 🔏 ✨ Explorations with tags in this project has now made me warried of using these as immutable values... But we can guard against that with this change! I will merge this now. |
Summary
This PR uses the wonderful
zizmortool to audit our own workflows andpinactfor pinned versioning 👾Reviewers
A similar audit can be performed with the zizmor tool:
Notes
Most changes I hope are repetitive, but I will comment on the more significant ones!
Requirements