Add zizmor to pre-commit and fix most findings #127749
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -61,6 +61,8 @@ jobs: | |
| - run: >- | ||
| echo '${{ github.event_name }}' | ||
| - uses: actions/checkout@v4 | ||
| with: | ||
| persist-credentials: false | ||
|
There was a problem hiding this comment. There's a There was a problem hiding this comment. I don't think it needs to be an authenticated git fetch? It ran okay here: https://github.com/python/cpython/actions/runs/12225521755/job/34099604804 There was a problem hiding this comment. Yep, There was a problem hiding this comment. That makes sense, but if this is the case, does it also mean that as long as we are just reading from public repos, there are no credentials used so there's nothing to share with the other jobs? IOW, (It might still be better to explicitly add There was a problem hiding this comment.
Nope, unfortunately the default means that a credential is persisted, even though it isn't necessary. So There was a problem hiding this comment. Thanks for your reply! I'm still not sure I understand if there is an actual issue with reading (i.e. with no writing/mutating operations) public repos though. I tried digging a bit deeper to understand and this is what I found. The # Personal access token (PAT) used to fetch the repository. The PAT is configured
# with the local git config, which enables your scripts to run authenticated git
# commands. The post-job step removes the PAT.
#
# We recommend using a service account with the least permissions necessary. Also
# when generating a new PAT, select the least scopes necessary.
#
# [Learn more about creating and using encrypted secrets](https://help.github.com/en/actions/automating-your-workflow-with-github-actions/creating-and-using-encrypted-secrets)
#
# Default: ${{ github.token }}
token: ''
# SSH key used to fetch the repository. The SSH key is configured with the local
# git config, which enables your scripts to run authenticated git commands. The
# post-job step removes the SSH key.
#
# We recommend using a service account with the least permissions necessary.
#
# [Learn more about creating and using encrypted secrets](https://help.github.com/en/actions/automating-your-workflow-with-github-actions/creating-and-using-encrypted-secrets)
ssh-key: ''The comment for # Whether to configure the token or SSH key with the local git config
# Default: true
persist-credentials: ''So my understanding is that if I explicitly set a token/key with If I don't explicitly specify any token/key (i.e. what we are doing in our workflows),
If no credentials are passed explicitly by using
Footnotes
There was a problem hiding this comment. No problem! Thanks for your detailed response as well.
Yep: the risk with this is that it's easy to inadvertently upload/log/otherwise persist that local filesystem credential. This can't happen with the in-memory token (i.e. For example, until recently, this would cause a token disclosure via a public artifact: uses: actions/checkout
# other steps
uses: actions/upload-artifact
with:
path: . # uploads the entire repo, including the persisted tokenThis post has some interesting/elucidative examples of that: https://unit42.paloaltonetworks.com/github-repo-artifacts-leak-tokens/
Yeah, I believe this will result in an empty string being saved on the filesystem for the token. I'm not 100% sure but I think this will cause GitHub API errors in some cases. (I think There was a problem hiding this comment.
Thanks for sharing, this is really interesting! Now that I have all the pieces of the puzzle it finally makes sense:
There are a few more caveats here and there, but this is already more than enough to justify the use of There was a problem hiding this comment. No problem! Your summary is great and matches my understanding perfectly 🙂 |
||
| - name: Check for source changes | ||
| id: check | ||
| run: | | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,12 +22,14 @@ jobs: | |
| env: | ||
| branch_base: 'origin/${{ github.event.pull_request.base.ref }}' | ||
| branch_pr: 'origin/${{ github.event.pull_request.head.ref }}' | ||
| commits: ${{ github.event.pull_request.commits }} | ||
| refspec_base: '+${{ github.event.pull_request.base.sha }}:remotes/origin/${{ github.event.pull_request.base.ref }}' | ||
| refspec_pr: '+${{ github.event.pull_request.head.sha }}:remotes/origin/${{ github.event.pull_request.head.ref }}' | ||
| steps: | ||
| - name: 'Check out latest PR branch commit' | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| persist-credentials: false | ||
hugovk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ref: >- | ||
| ${{ | ||
| github.event_name == 'pull_request' | ||
|
|
@@ -39,7 +41,7 @@ jobs: | |
| if: github.event_name == 'pull_request' | ||
| run: | | ||
| # Fetch enough history to find a common ancestor commit (aka merge-base): | ||
| git fetch origin ${{ env.refspec_pr }} --depth=$(( ${{ env.commits }} + 1 )) \ | ||
|
There was a problem hiding this comment. not an argument against doing this, just a howl of frustration at GitHub: it feels so silly that we have to do this to make our workflow secure There was a problem hiding this comment. Yeah, it's definitely not ideal 🙃. In this particular case however, this might be a false positive: contextually it looks like (And as a separate note: There was a problem hiding this comment. Yup, https://docs.github.com/en/webhooks/webhook-events-and-payloads#pull_request documents that
I'm very ignorant about all this, but I assumed from your docs at https://woodruffw.github.io/zizmor/audits/#template-injection that using an environment variable here would be safer:
There was a problem hiding this comment. I guess it's saf-er but not truly safe? :) There was a problem hiding this comment. Ah, I think that phrasing is confusing! What I was trying to say in those docs is that lines like: run: foo ${{ env.bar }}should be ideally replaced with: run: foo "${BAR}"
env:
BAR: ${{ env.bar }}In other words: in the best case I can improve those docs to make that more clear 🙂 There was a problem hiding this comment. (Technically I think There was a problem hiding this comment. Just to make sure I understand correctly, if I have something like: run: echo "Issue title: ${{ github.event.issue.title }}"and the attacker sets the title to something like run: echo "Issue title: "; rm -rf /; echo ""and execute the 3 commands separately ( Whereas if I use run: echo "Issue title: ${TITLE}"
env:
TITLE: ${{ github.event.issue.title }}when run: echo "Issue title: \"; rm -rf /; echo \""which will only execute a single Also what is the difference -- once the env var is set -- between using:
Are they all equivalent or do different escaping/expansions rules apply? There was a problem hiding this comment.
Aha, thanks! I've updated the PR. Although this isn't working in Windows' pwsh.exe shell, what's the right syntax there? https://github.com/python/cpython/actions/runs/12238891899/job/34138074592?pr=127749 There was a problem hiding this comment.
You could try using There was a problem hiding this comment.
The first two examples are shell interpolations, and have the same expansion rules applied to them. But note: this is only because of the surrounding quotes: if you had instead written The third example is a template interpolation, which bypasses the shell interpolation rules. It's not safe to do in the general case. TL;DR: you almost always want some variant of
Yep, this is the easiest fix -- the alternative is to use pwsh's |
||
| --no-tags --prune --no-recurse-submodules | ||
|
|
||
| # This should get the oldest commit in the local fetched history (which may not be the commit the PR branched from): | ||
|
|
@@ -81,6 +83,8 @@ jobs: | |
| timeout-minutes: 60 | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| with: | ||
| persist-credentials: false | ||
| - name: 'Set up Python' | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
|
|
@@ -99,6 +103,8 @@ jobs: | |
| timeout-minutes: 60 | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| with: | ||
| persist-credentials: false | ||
| - uses: actions/cache@v4 | ||
| with: | ||
| path: ~/.cache/pip | ||
|
|
||
hugovk marked this conversation as resolved.
Show resolved
Hide resolved
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| rules: | ||
| dangerous-triggers: | ||
| ignore: | ||
| - documentation-links.yml |
Uh oh!
There was an error while loading. Please reload this page.