Upgrade zizmor to the latest version in CI #15649
Conversation
AlexWaygood
added
red-knot
|
.github/zizmor.yml
Outdated
| excessive-permissions: | ||
| ignore: | ||
| - build-docker.yml | ||
| - publish-playground.yml | ||
| - publish-docs.yml |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The permissions related to jobs in the release workflow are in Cargo.toml here:
Lines 309 to 310 in 4cfa355
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.
There was a problem hiding this comment.
Aah, thank you for pointing that out!
So, I think it's just
build-docker.ymlthat 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_TOKENpermissions 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?
There was a problem hiding this comment.
The
GITHUB_TOKENpermissions 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 thatpublish-wasm.yml's more restricted permissions would downgrade the permissions passed to it byrelease.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.
There was a problem hiding this comment.
Otherwise, we'll need to revert the
build-binaries.ymlupdate in this PR as that's also a nested workflow called byrelease.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!
.github/zizmor.yml
Outdated
| excessive-permissions: | ||
| ignore: | ||
| - build-docker.yml | ||
| - publish-playground.yml | ||
| - publish-docs.yml |
There was a problem hiding this comment.
The permissions related to jobs in the release workflow are in Cargo.toml here:
Lines 309 to 310 in 4cfa355
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.
| @@ -10,3 +10,8 @@ rules: | |||
| ignore: | |||
| - build-docker.yml | |||
| - publish-playground.yml | |||
| excessive-permissions: | |||
| ignore: | |||
There was a problem hiding this comment.
It might be helpful to add a short comment explaining why they ignored. Ideally, this wouldn't be necessary (maybe add a todo?)
There was a problem hiding this comment.
there was already a TODO on line 4, but I added a comment closer to these lines explaining exactly why they are ignored :-)
* main: [red-knot] MDTests: Do not depend on precise public-symbol type inference (#15691) [red-knot] Make `infer.rs` unit tests independent of public symbol inference (#15690) Tidy knot CLI tests (#15685) [red-knot] Port comprehension tests to Markdown (#15688) Create Unknown rule diagnostics with a source range (#15648) [red-knot] Port 'deferred annotations' unit tests to Markdown (#15686) [red-knot] Support custom typeshed Markdown tests (#15683) Don't run the linter ecosystem check on PRs that only touch red-knot crates (#15687) Add `rules` table to configuration (#15645) [red-knot] Make `Diagnostic::file` optional (#15640) [red-knot] Add test for nested attribute access (#15684) [red-knot] Anchor relative paths in configurations (#15634) [`pyupgrade`] Handle multiple base classes for PEP 695 generics (`UP046`) (#15659) [`pyflakes`] Treat arguments passed to the `default=` parameter of `TypeVar` as type expressions (`F821`) (#15679) Upgrade zizmor to the latest version in CI (#15649) [`pyupgrade`] Add rules to use PEP 695 generics in classes and functions (`UP046`, `UP047`) (#15565) [red-knot] Ensure a gradual type can always be assigned to itself (#15675)
Summary
The latest version of zizmor points out that we have the default permissions set in several of our GitHub workflows that don't really need any permissions at all. These can be made more secure by restricting the permissions for these workflows.
Test Plan
CI on this PR. All workflows being modified here are fully run as part of normal PR CI.