Add zizmor to pre-commit; address GH workflow issues raised #7662

stefanv · 2025-01-15T23:55:31Z

No description provided.

jni · 2025-01-16T12:45:16Z

@stefanv do you maybe want to update the description to add some more info?

how did you run zizmor?
what were the issues found? (the diff offers clues but no explanations)

stefanv · 2025-01-16T18:29:45Z

(cd .github/workflows && zizmor *)

warning[artipacked]: credential persistence through GitHub Actions artifacts
  --> /home/stefan/src/scikit-image/.github/workflows/benchmarks.yml:31:9
   |
31 |         - uses: actions/checkout@v4
   |  _________-
32 | |         with:
33 | |           fetch-depth: 0
   | |________________________- does not set persist-credentials: false
   |
   = note: audit confidence → Low

error[template-injection]: code injection via template expansion
  --> /home/stefan/src/scikit-image/.github/workflows/benchmarks.yml:66:9
   |
66 |         - name: Run benchmarks
   |           ^^^^^^^^^^^^^^^^^^^^ this step
67 |           shell: bash -l {0}
...
77 |             CONTENDER_LABEL: "${{ github.event.pull_request.head.label }}"
78 | /         run: |
79 | |           set -x
...  |
97 | |               exit 1
98 | |           fi
   | |____________^ github.event.pull_request.base.sha may expand into attacker-controllable code
   |
   = note: audit confidence → High

error[template-injection]: code injection via template expansion
  --> /home/stefan/src/scikit-image/.github/workflows/benchmarks.yml:66:9
   |
66 |         - name: Run benchmarks
   |           ^^^^^^^^^^^^^^^^^^^^ this step
67 |           shell: bash -l {0}
...
77 |             CONTENDER_LABEL: "${{ github.event.pull_request.head.label }}"
78 | /         run: |
79 | |           set -x
...  |
97 | |               exit 1
98 | |           fi
   | |____________^ github.event.pull_request.base.sha may expand into attacker-controllable code
   |
   = note: audit confidence → High

warning[artipacked]: credential persistence through GitHub Actions artifacts
  --> /home/stefan/src/scikit-image/.github/workflows/build_docs.yml:18:9
   |
18 |         - name: Checkout code
   |  _________-
19 | |         uses: actions/checkout@v4
   | |_________________________________- does not set persist-credentials: false
   |
   = note: audit confidence → Low

warning[artipacked]: credential persistence through GitHub Actions artifacts
  --> /home/stefan/src/scikit-image/.github/workflows/emscripten.yml:33:9
   |
33 |         - name: Checkout scikit-image
   |  _________-
34 | |         uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
   | |________________________________________________________________________________- does not set persist-credentials: false
   |
   = note: audit confidence → Low

warning[artipacked]: credential persistence through GitHub Actions artifacts
  --> /home/stefan/src/scikit-image/.github/workflows/test_nightlies_on_main.yml:33:9
   |
33 |       - uses: actions/checkout@v4
   |         ------------------------- does not set persist-credentials: false
   |
   = note: audit confidence → Low

warning[artipacked]: credential persistence through GitHub Actions artifacts
  --> /home/stefan/src/scikit-image/.github/workflows/tests.yml:69:9
   |
69 |         - name: Checkout scikit-image
   |  _________-
70 | |         uses: actions/checkout@v4
   | |_________________________________- does not set persist-credentials: false
   |
   = note: audit confidence → Low

warning[artipacked]: credential persistence through GitHub Actions artifacts
   --> /home/stefan/src/scikit-image/.github/workflows/tests.yml:120:9
    |
120 |         - name: Checkout scikit-image
    |  _________-
121 | |         uses: actions/checkout@v4
122 | |
123 | |       # TODO: replace with setup-python when there is support
    | |_____________________________________________________________- does not set persist-credentials: false
    |
    = note: audit confidence → Low

warning[artipacked]: credential persistence through GitHub Actions artifacts
   --> /home/stefan/src/scikit-image/.github/workflows/tests.yml:163:9
    |
163 |         - name: Checkout scikit-image
    |  _________-
164 | |         uses: actions/checkout@v4
    | |_________________________________- does not set persist-credentials: false
    |
    = note: audit confidence → Low

warning[artipacked]: credential persistence through GitHub Actions artifacts
  --> /home/stefan/src/scikit-image/.github/workflows/wheel_tests_and_release.yml:32:9
   |
32 |         - uses: actions/checkout@v4
   |  _________-
33 | |         with:
34 | |           fetch-depth: 0
   | |________________________- does not set persist-credentials: false
   |
   = note: audit confidence → Low

warning[artipacked]: credential persistence through GitHub Actions artifacts
  --> /home/stefan/src/scikit-image/.github/workflows/wheels_recipe.yml:43:9
   |
43 |         - uses: actions/checkout@v4
   |  _________-
44 | |         with:
45 | |           fetch-depth: 0
   | |________________________- does not set persist-credentials: false
   |
   = note: audit confidence → Low

warning[artipacked]: credential persistence through GitHub Actions artifacts
  --> /home/stefan/src/scikit-image/.github/workflows/wheels_recipe.yml:89:9
   |
89 |         - uses: actions/checkout@v4
   |  _________-
90 | |         with:
91 | |           fetch-depth: 0
   | |________________________- does not set persist-credentials: false
   |
   = note: audit confidence → Low

warning[artipacked]: credential persistence through GitHub Actions artifacts
   --> /home/stefan/src/scikit-image/.github/workflows/wheels_recipe.yml:131:9
    |
131 |         - uses: actions/checkout@v4
    |  _________-
132 | |         with:
133 | |           fetch-depth: 0
    | |________________________- does not set persist-credentials: false
    |
    = note: audit confidence → Low

warning[artipacked]: credential persistence through GitHub Actions artifacts
   --> /home/stefan/src/scikit-image/.github/workflows/wheels_recipe.yml:218:9
    |
218 |         - uses: actions/checkout@v4
    |  _________-
219 | |         with:
220 | |           fetch-depth: 0
    | |________________________- does not set persist-credentials: false
    |
    = note: audit confidence → Low

49 findings (1 ignored, 34 suppressed): 0 unknown, 0 informational, 0 low, 12 medium, 2 high

stefanv · 2025-01-16T18:30:17Z

In our case, I don't think these issues are serious, but they are also easy enough to work around.

stefanv · 2025-01-16T18:35:31Z

I added zizmor as a pre-commit hook, otherwise we'd have no way of catching these problems as they occur. We could also / alternatively enforce it in CI. I also noticed that we don't currently enforce pre-commit hooks in our CI?

mkcor · 2025-01-16T22:52:42Z

I also noticed that we don't currently enforce pre-commit hooks in our CI?

I'm not sure how helpful vs. confusing it would be to (new) contributors. Some contributors expect it (based on other projects?), but the remote branch would then diverge from the local one... more often than just from edits by maintainers when allowed.

stefanv · 2025-01-16T23:05:26Z

I also noticed that we don't currently enforce pre-commit hooks in our CI?

I'm not sure how helpful vs. confusing it would be to (new) contributors. Some contributors expect it (based on other projects?), but the remote branch would then diverge from the local one... more often than just from edits by maintainers when allowed.

Sorry, I'm not following. All code in the repo has to be compliant with the linters. This can be enforced locally, by running pre-commit (typically as a hook). But we should definitely not commit code that fails the linter.

The linter will not modify code, just report that there's a problem—which would have been noticed locally, if the committer had pre-commit enabled, as requested in the development guide.

jni · 2025-01-17T11:19:52Z

Sorry, I'm not following. All code in the repo has to be compliant with the linters. This can be enforced locally, by running pre-commit (typically as a hook). But we should definitely not commit code that fails the linter.

@mkcor is referencing the fact that some projects, including napari (against my wishes but it turned out to save my butt a couple of times 😂), configure pre-commit.ci to push automatic fixes when the PR fails the linter.

mkcor · 2025-01-17T12:11:25Z

Yes, that's exactly what I meant; thank you, @jni!

configure pre-commit.ci to push automatic fixes when the PR fails the linter

I thought that's what you meant, @stefanv, by enforcing pre-commit hooks in our CI. I guess I didn't get your original question ("we don't currently enforce pre-commit hooks in our CI?").

But we should definitely not commit code that fails the linter.

Of course; currently, we do this by policy: We refrain from merging a PR for which the lint check fails (and we fix the PR until it passes the link check, as we should), but technically we could merge it (so we could, hypothetically, have non-compliant code land in main).

Personally, I find that our current policy is good enough, safe enough, and well enforced, but I'm open to further steps (especially if that's becoming a standard in the ecosystem).

The linter will not modify code, just report that there's a problem—which would have been noticed locally, if the committer had pre-commit enabled, as requested in the development guide.

In my understanding, this is exactly / already the current situation.

stefanv · 2025-01-17T19:34:26Z

Got it, thanks for the clarification, Juan: I don't mean to reformat PRs automatically, simply to check that they are compliant.

We are currently in the (bad) situation that our CI is failing almost constantly, so we have to merge with CI failing. This makes it very easy for lint failures to slip through, if we're not careful. This is just an extra safety net.

mkcor · 2025-01-18T07:30:10Z

We are currently in the (bad) situation that our CI is failing almost constantly, so we have to merge with CI failing.

That's right, and I admit I'm always uncomfortable merging PRs with some CI failing even when it's unrelated (including when it's patently unrelated, i.e., investigating literally took 30 seconds).

This makes it very easy for lint failures to slip through, if we're not careful.

In my experience, it's not 'very easy.' First of all, we are careful. Whenever CI is failing, we look into the list of checks (we click all the failing checks and read the error messages and troubleshoot and so on) and, honestly, the [edit: ~~link~~] lint check stands out:

it's at the very end of the list, so you can't miss it when you scroll down;
it has a different thumbnail (with that "P" logo), so it draws your attention.

Whether all CI checks are passing:

or some are failing:

we don't miss that "P pre-commit.ci - pr" line. Even (mostly?) new contributors immediately pick it up when the check fails, and they ask what they should be doing.

This is just an extra safety net.

Sorry, I'm missing what the safety net is here exactly. To be an extra safety net, shouldn't the zizmor block go under ci: in .pre-commit-config.yaml?

stefanv · 2025-01-27T23:44:07Z

@mkcor Not sure I follow, the ci section has the following:

ci:
  autofix_prs: false
  autofix_commit_msg: |
    '[pre-commit.ci 🤖] Apply code format tools to PR'
  autoupdate_schedule: quarterly

This pre-commit check will only affect people modifying the CI workflows, which won't be new contributors.

mkcor · 2025-01-28T14:04:41Z

Oh, I get it now! I apologize for the confusion, @stefanv. The 'zizmor block' I was referring to is adding zizmor to our pre-commit hooks, period.

mkcor

I could reproduce successfully. Note that, before pip installing zizmor==1.1.1, I casually pip installed zizmor, so I got version 1.2.2, which resulted in:

$ cd .github/workflows && zizmor .
2025-01-28T14:11:59.938385Z  WARN collect_inputs:collect_from_repo_dir{top_dir="." current_dir="."}: zizmor: ./.github/workflows not found while collecting workflows
no inputs collected

lagru · 2025-01-28T16:50:57Z

.github/workflows/milestone-merged-prs.yaml

When checking with zizmor v1.2.2 I get additional warnings relating to this file and label-check.yaml:16:3.

Details

warning[excessive-permissions]: overly broad permissions --> milestone-merged-prs.yaml:11:3 | 11 | / milestone_pr: 12 | | name: attach to PR ... | 17 | | token: ${{ secrets.MILESTONE_LABELER_TOKEN }} 18 | | force: true | | - | |______________________| | this job | default permissions used due to no permissions: block | = note: audit confidence → Medium warning[excessive-permissions]: overly broad permissions --> label-check.yaml:16:3 | 16 | / check-type-label: 17 | | name: ensure type label ... | 24 | | exit 1 25 | | fi | | - | |_____________| | this job | default permissions used due to no permissions: block | = note: audit confidence → Medium 38 findings (1 ignored, 35 suppressed): 0 unknown, 0 informational, 0 low, 2 medium, 0 high

Do we care about addressing these?

Also I don't really understand the warning. GitHub's worflow syntax doesn't mention block as a valid value for permissions.

I think it means the permissions block is missing.

lagru · 2025-02-03T16:54:21Z

.pre-commit-config.yaml

@@ -43,6 +43,11 @@ repos:
        entry: python tools/generate_requirements.py
        files: "pyproject.toml|requirements/.*\\.txt|tools/generate_requirements.py"

+  - repo: https://github.com/woodruffw/zizmor-pre-commit
+    rev: 4dcc98ccce605605fd6d5202bf803a8c8bc34e8d # v1.1.1


Any value in already updating to most recent v1.3.0?

Yes, let me do that.

lagru

Looks good otherwise.

jarrodmillman

Thanks! It looks like there is one or two minor pending changes (e.g., updating to 1.3), so I will let Stefan merge once he's finished.

stefanv · 2025-02-03T20:55:18Z

Darn, forgot to push

stefanv · 2025-02-04T01:04:09Z

@jarrodmillman Did I mess up the permissions for the milestone labeler? I was worried I might, so I checked :) The action ran successfully, but the milestone is not set, which is a problem in itself.

…ailure * origin/main: Add zizmor to pre-commit; address GH workflow issues raised (scikit-image#7662) Reenable test marked as flaky on Azure. (scikit-image#7694) Simultaneously resolve all dependencies; add pip caching (scikit-image#7690) Copy keypoints if necessary to preserve contiguity (scikit-image#7692) CI cleanup (scikit-image#7693) Port CI testing from Azure to GitHub (scikit-image#7687) Lower CI build verbosity Use pytest config in pyproject.toml in CI (scikit-image#7555) Improve docstring for rolling_ball function. (scikit-image#7682)

stefanv added the 🔧 type: Maintenance Refactoring and maintenance of internals label Jan 15, 2025

stefanv mentioned this pull request Jan 17, 2025

Add lint workflow #7663

Closed

mkcor approved these changes Jan 28, 2025

View reviewed changes

lagru reviewed Jan 28, 2025

View reviewed changes

Add zizmor as pre-commit hook

25cc275

stefanv changed the title ~~Address GH workflow security issues raised by zizmor~~ Add zizmor to pre-commit; address GH workflow issues raised Feb 3, 2025

Address zizmor-raised security concerns

eb1d543

stefanv force-pushed the zizmor-gh-workflows branch from e507127 to eb1d543 Compare February 3, 2025 04:26

lagru reviewed Feb 3, 2025

View reviewed changes

lagru approved these changes Feb 3, 2025

View reviewed changes

Update zizmor to 1.3.0

6670c08

jarrodmillman approved these changes Feb 3, 2025

View reviewed changes

stefanv merged commit ef21164 into scikit-image:main Feb 4, 2025
24 checks passed

stefanv added this to the 0.25.2 milestone Feb 4, 2025

stefanv mentioned this pull request Feb 4, 2025

Give milestone labeler necessary permissions #7695

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add zizmor to pre-commit; address GH workflow issues raised #7662

Add zizmor to pre-commit; address GH workflow issues raised #7662

stefanv commented Jan 15, 2025

jni commented Jan 16, 2025

stefanv commented Jan 16, 2025 •

edited

Loading

stefanv commented Jan 16, 2025

stefanv commented Jan 16, 2025

mkcor commented Jan 16, 2025

stefanv commented Jan 16, 2025

jni commented Jan 17, 2025

mkcor commented Jan 17, 2025

stefanv commented Jan 17, 2025

mkcor commented Jan 18, 2025 •

edited

Loading

stefanv commented Jan 27, 2025

mkcor commented Jan 28, 2025

mkcor left a comment

lagru Jan 28, 2025

lagru Jan 28, 2025

stefanv Feb 3, 2025

lagru Feb 3, 2025

stefanv Feb 3, 2025

lagru left a comment

jarrodmillman left a comment

stefanv commented Feb 3, 2025

stefanv commented Feb 4, 2025

Add zizmor to pre-commit; address GH workflow issues raised #7662

Add zizmor to pre-commit; address GH workflow issues raised #7662

Conversation

stefanv commented Jan 15, 2025

jni commented Jan 16, 2025

stefanv commented Jan 16, 2025 • edited Loading

stefanv commented Jan 16, 2025

stefanv commented Jan 16, 2025

mkcor commented Jan 16, 2025

stefanv commented Jan 16, 2025

jni commented Jan 17, 2025

mkcor commented Jan 17, 2025

stefanv commented Jan 17, 2025

mkcor commented Jan 18, 2025 • edited Loading

stefanv commented Jan 27, 2025

mkcor commented Jan 28, 2025

mkcor left a comment

Choose a reason for hiding this comment

lagru Jan 28, 2025

Choose a reason for hiding this comment

lagru Jan 28, 2025

Choose a reason for hiding this comment

stefanv Feb 3, 2025

Choose a reason for hiding this comment

lagru Feb 3, 2025

Choose a reason for hiding this comment

stefanv Feb 3, 2025

Choose a reason for hiding this comment

lagru left a comment

Choose a reason for hiding this comment

jarrodmillman left a comment

Choose a reason for hiding this comment

stefanv commented Feb 3, 2025

stefanv commented Feb 4, 2025

stefanv commented Jan 16, 2025 •

edited

Loading

mkcor commented Jan 18, 2025 •

edited

Loading