-
-
Notifications
You must be signed in to change notification settings - Fork 32k
Add zizmor to pre-commit and fix most findings #127749
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
596c49e
7f5a8ec
6b7f89f
b6115ca
26c4d7d
bd9e472
c33dcba
a246790
e5ba3d3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,15 +4,14 @@ on: | |
pull_request: | ||
types: [opened, reopened, labeled, unlabeled, synchronize] | ||
|
||
permissions: | ||
issues: write | ||
pull-requests: write | ||
|
||
jobs: | ||
label-dnm: | ||
name: DO-NOT-MERGE | ||
if: github.repository_owner == 'python' | ||
runs-on: ubuntu-latest | ||
permissions: | ||
issues: write | ||
pull-requests: write | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These labels are only used on PRs, so I'm pretty sure the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, we can check with this separately. This PR isn't adding any extra permissions here. |
||
timeout-minutes: 10 | ||
|
||
steps: | ||
|
@@ -28,6 +27,9 @@ jobs: | |
name: Unresolved review | ||
if: github.repository_owner == 'python' | ||
runs-on: ubuntu-latest | ||
permissions: | ||
issues: write | ||
pull-requests: write | ||
timeout-minutes: 10 | ||
|
||
steps: | ||
|
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Nope, unfortunately the default means that a credential is persisted, even though it isn't necessary. So There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 token This 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
---|---|---|
|
@@ -20,6 +20,8 @@ jobs: | |
CROSS_BUILD_WASI: cross-build/wasm32-wasip1 | ||
steps: | ||
- uses: actions/checkout@v4 | ||
with: | ||
persist-credentials: false | ||
# No problem resolver registered as one doesn't currently exist for Clang. | ||
- name: "Install wasmtime" | ||
uses: bytecodealliance/actions/wasmtime/setup@v1 | ||
|
@@ -34,9 +36,9 @@ jobs: | |
- name: "Install WASI SDK" # Hard-coded to x64. | ||
if: steps.cache-wasi-sdk.outputs.cache-hit != 'true' | ||
run: | | ||
mkdir ${{ env.WASI_SDK_PATH }} && \ | ||
curl -s -S --location https://github.com/WebAssembly/wasi-sdk/releases/download/wasi-sdk-${{ env.WASI_SDK_VERSION }}/wasi-sdk-${{ env.WASI_SDK_VERSION }}.0-x86_64-linux.tar.gz | \ | ||
tar --strip-components 1 --directory ${{ env.WASI_SDK_PATH }} --extract --gunzip | ||
mkdir "${WASI_SDK_PATH}" && \ | ||
curl -s -S --location "https://github.com/WebAssembly/wasi-sdk/releases/download/wasi-sdk-${WASI_SDK_VERSION}/wasi-sdk-${WASI_SDK_VERSION}.0-x86_64-linux.tar.gz" | \ | ||
tar --strip-components 1 --directory "${WASI_SDK_PATH}" --extract --gunzip | ||
- name: "Configure ccache action" | ||
uses: hendrikmuhs/[email protected] | ||
with: | ||
|
@@ -72,6 +74,6 @@ jobs: | |
- name: "Make host" | ||
run: python3 Tools/wasm/wasi.py make-host | ||
- name: "Display build info" | ||
run: make --directory ${{ env.CROSS_BUILD_WASI }} pythoninfo | ||
run: make --directory "${CROSS_BUILD_WASI}" pythoninfo | ||
- name: "Test" | ||
run: make --directory ${{ env.CROSS_BUILD_WASI }} test | ||
run: make --directory "${CROSS_BUILD_WASI}" test |
hugovk marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
# Configuration for the zizmor static analysis tool, run via pre-commit in CI | ||
# https://woodruffw.github.io/zizmor/configuration/ | ||
rules: | ||
dangerous-triggers: | ||
ignore: | ||
- documentation-links.yml |
Uh oh!
There was an error while loading. Please reload this page.