Skip to content

Zizmor audit - #29251

Merged
timhoffm merged 4 commits into
matplotlib:mainfrom
QuLogic:zizmor-audit
Dec 9, 2024
Merged

timhoffm merged 4 commits into
matplotlib:mainfrom
QuLogic:zizmor-audit

Conversation

@QuLogic

@QuLogic QuLogic commented Dec 7, 2024

Copy link
Copy Markdown
Member

PR summary

This applies suggestions from zizmor to increase the security of the Actions workflows. Currently, we can almost pass the "auditor" persona (the strictest) except for:

  • "expression may expand into a self-hosted runner", but we don't have any self-hosted runners, so this is a false positive.
  • "pull_request_target is almost always used insecurely", but we do not a) use user-controlled target substitutions, nor b) run any code from the checkout. So I believe these are safe as nothing user-controlled can be injected.

I do not believe we have any glaring security holes here, but this PR just avoids any cause for concern from auditing.

PR checklist

We only have a public repo, and should have locked down permissions on
the token, but it's best practice not to leak these out into other steps
of the job.
Moved the permissions to the jobs that need them, though this is
probably not a big change for the reviewdog workflow. Also drop the
`pull-request` permission from the reviewdog workflow, as it's not in
the mypy-stubtest one, and still seems to work.
I don't believe `do_no_merge.yml` is unsafe, but there's no need to echo
the environment variable (it'll either pass or fail based on the value
anyway.)

I also don't think the `circleci.yml` context variable is vulnerable,
but zizmor warns about it, and it's easy to avoid if turns out to be
vulnerable.
@github-actions github-actions Bot added CI: Run cibuildwheel Run wheel building tests on a PR CI: Run cygwin Run cygwin tests on a PR topic: pyplot API labels Dec 7, 2024
@QuLogic

QuLogic commented Dec 7, 2024

Copy link
Copy Markdown
Member Author

I pushed a commit to break linting and confirm that reviewdog was still working, and it seems to have posted the failures correctly without the pull-request permission, so I'll drop that commit now.

@tacaswell tacaswell added this to the v3.10.0 milestone Dec 9, 2024
@timhoffm
timhoffm merged commit f0ecacc into matplotlib:main Dec 9, 2024
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Dec 9, 2024
@QuLogic
QuLogic deleted the zizmor-audit branch December 9, 2024 22:53
greglucas added a commit that referenced this pull request Dec 9, 2024
…251-on-v3.10.x

Backport PR #29251 on branch v3.10.x (Zizmor audit)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI: Run cibuildwheel Run wheel building tests on a PR CI: Run cygwin Run cygwin tests on a PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants