Skip to content
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.

Sign up for GitHub

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

Upgrade zizmor to the latest version in CI #15649

Merged
merged 4 commits into from
Jan 22, 2025
Merged

Upgrade zizmor to the latest version in CI #15649

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
85ac91c
Upgrade zizmor to the latest version in CI
AlexWaygood Jan 20, 2025
9b1226b
what happens if nothing has any permissions
AlexWaygood Jan 21, 2025
094615f
Merge branch 'main' into alex/upgrade-zizmor
AlexWaygood Jan 22, 2025
afc0a26
better comment
AlexWaygood Jan 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/build-binaries.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: true

permissions: {}

env:
PACKAGE_NAME: ruff
MODULE_NAME: ruff
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/ci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
name: CI

permissions: {}

on:
push:
branches: [main]
Expand Down
5 changes: 5 additions & 0 deletions .github/zizmor.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,8 @@ rules:
ignore:
- build-docker.yml
- publish-playground.yml
excessive-permissions:
ignore:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be helpful to add a short comment explaining why they ignored. Ideally, this wouldn't be necessary (maybe add a todo?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there was already a TODO on line 4, but I added a comment closer to these lines explaining exactly why they are ignored :-)

- build-docker.yml
- publish-playground.yml
- publish-docs.yml
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't make modifications to these files because they don't run as part of normal PR CI (only during releases, usually) and it wasn't obvious to me how to figure out what permissions each workflow needs. I didn't want to break the release workflow, and it didn't seem worth investing a large amount of time into trying to investigate exactly what permissions each workflow needed, so I left it for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The permissions related to jobs in the release workflow are in Cargo.toml here:

ruff/Cargo.toml

Lines 309 to 310 in 4cfa355

# Custom permissions for GitHub Jobs
github-custom-job-permissions = { "build-docker" = { packages = "write", contents = "read" }, "publish-wasm" = { contents = "read", id-token = "write", packages = "write" } }

And, can also be referred in the release.yml.

So, I think it's just build-docker.yml that requires specific permission while the other two jobs don't require any.

But, I'm fine leaving it as it is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aah, thank you for pointing that out!

So, I think it's just build-docker.yml that requires specific permission while the other two jobs don't require any.

I think if we put permissions: {} in e.g. publish-wasm.yml, it would override the permissions passed to the workflow by release.yml -- the docs here state:

The GITHUB_TOKEN permissions passed from the caller workflow can be only downgraded (not elevated) by the called workflow.

And the called workflow here would be publish-wasm.yml, so that would mean that publish-wasm.yml's more restricted permissions would downgrade the permissions passed to it by release.yml, breaking the release workflow.

I think that means that each of these workflows needs to have exactly the same permissions in their individual workflow files as they have passed to them by release.yml -- does that sound right to you?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GITHUB_TOKEN permissions passed from the caller workflow can be only downgraded (not elevated) by the called workflow.

And the called workflow here would be publish-wasm.yml, so that would mean that publish-wasm.yml's more restricted permissions would downgrade the permissions passed to it by release.yml, breaking the release workflow.

I think that means that each of these workflows needs to have exactly the same permissions in their individual workflow files as they have passed to them by release.yml -- does that sound right to you?

Interesting, doesn't this mean that the nested workflow (e.g., publish-wasm.yml) can have permissions: {} because that's more restrictive as compared to the parent workflow (release.yml) ?

Otherwise, we'll need to revert the build-binaries.yml update in this PR as that's also a nested workflow called by release.yml.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise, we'll need to revert the build-binaries.yml update in this PR as that's also a nested workflow called by release.yml.

I'm pretty confident that the build-binaries.yml change is okay because, unlike the other workflows, that workflow is run exactly the same way as part of PR CI as it is when it's called from release.yml. (And the CI on this PR is green.)

I'm just going to leave this as-is for now, but I'm very happy if somebody else wants to experiment with removing these ignores and actually fixing the issues zizmor is complaining about!

2 changes: 1 addition & 1 deletion .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ repos:
# zizmor detects security vulnerabilities in GitHub Actions workflows.
# Additional configuration for the tool is found in `.github/zizmor.yml`
- repo: https://github.com/woodruffw/zizmor-pre-commit
rev: v1.1.1
rev: v1.2.2
hooks:
- id: zizmor

Expand Down
Loading