[hashcatHitman]

• I did not use an LLM to create a change in this PR.

---

In pursuit of getting more zizmor lints enforced across rust-lang repos through crabwatch, this PR fixes most zizmor findings as of zizmor@1.31.1, using the auditor persona. The lints that actually fired per-persona level are:

regular persona findings:

• Fixed in this PR:
  • artipacked
  • excessive-permissions (See also: Solve the excessive-permissions lint crabwatch#97)
  • template-injection
• Not fixed in this PR:
  • None

pedantic persona findings:

• Everything from regular
• Fixed in this PR:
  • 1 additional instance of excessive-permissions (See also: Solve the excessive-permissions lint crabwatch#97)
  • 7 additional instances of template-injection
• Not fixed in this PR:
  • anonymous-definition (Because coming up with names is hard and there is minimal benefit to this)
  • concurrency-limits (Due to concerns about special cases. See also: Determine special cases for concurrency-limits crabwatch#107)

auditor persona findings:

• Everything from pedantic
• Not fixed in this PR:
  • None

I also wanted to mention something not marked by zizmor. You manually install rustup like so:

rustfmt/.github/workflows/test.yml

Lines 69 to 74 in 97738df

	- name: install rustup
	if: ${{ matrix.os != 'windows-latest' }}
	run: |
	curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs > rustup-init.sh
	sh rustup-init.sh -y --default-toolchain none
	rustup target add ${{ matrix.target }}

Are you aware this isn't necessary unless you absolutely must have the newest rustup? The standard GitHub runners ubuntu-latest, windows-latest, and macos-latest all come with rustup preinstalled.

[ytmimi]

I'd like to familiarize myself with zizmor before we move forward. Marking this as on-hold for now.

[hashcatHitman]

Sure! Also see the crabwatch repository, which is the wrapper around zizmor used by t-infra. Feel free to ask me questions here or on Zulip, and I'll do my best to answer them.

[jieyouxu]

Thanks, GHA changes look good to me.

View changes since this review

[jieyouxu]

Is there a way we can require zizmor checks to pass as opposed to monitor for now? I know we're still in the gradually-cover-all-the-repo phase.

(Or wait, is it just some rules are not enforced org-wide yet?)

[hashcatHitman]

Is there a way we can require zizmor checks to pass as opposed to monitor for now?

I'm not sure, I've never dealt with a workflow that mysteriously imposes itself from another repo like this before. If you have access to branch rulesets, my best guess would be to add a ruleset that requires status checks to pass, but I'm not sure if Crabwatch will actually show up in that list. (I don't have such access over surveys, which would have been my place to check, so I'm guessing you probably don't have such access?)

Alternatively, you could just re-run zizmor yourself in this repository and require it the same way you would any other workflow. It'd be a bit redundant, but it is an option.

I know we're still in the gradually-cover-all-the-repo phase.

This seems to imply that you think once we get out of that phase we're going to start requiring crabwatch to pass. While that was the original plan, I believe we've decided against doing that for the foreseeable future due to several lints being capable of failing due to factors outside of our control (originally, this referred to archived-uses, but I believe I made a more complete list in this issue: rust-lang/crabwatch#92). This change of plans was announced here: #t-infra/announcements > GitHub Actions linter @ 💬.

(Or wait, is it just some rules are not enforced org-wide yet?)

We maintain a policy here: https://github.com/rust-lang/crabwatch/blob/main/zizmor-policy.yml

Not all rules are enabled by default. We're enabling more rules over time, but before we do that, we have to fix them so that they pass. The policy allows defining overrides for individual repos, so the workflow looks like one of two things, generally:

• A:
  • 1. Pick a lint that is not yet enabled
  • 2. Pick ${repo} from the repos that lint fires for
  • 3. Fix that lint in repository rust-lang/${repo}
  • 4. Enable that lint just for rust-lang/${repo}
  • 5. Repeat 2-4 until all repos have the lint solved
  • 6. Enable the lint by default and remove the overrides
• B:
  • 1. Pick ${repo} from the repos that any lint fires for
  • 2. Fix as many of the lints in repository rust-lang/${repo} as possible
  • 3. Enable all of the lints that are fixed just for rust-lang/${repo}
  • 4. Repeat 1-3 until all repos have some large set of lints solved
  • 5. Enable all those lints by default and remove the overrides

I started by doing something like the first approach, where I'd specifically target ~3 lints at a time, but I've found it isn't much more work to just tackle almost all of them at once. Fewer PRs to bother each repository with, and saying "I'm here to fix all/almost all of the lints" makes it less surprising for me to enable lints that were already solved for a given repository.

I've been using the script found here: https://github.com/hashcatHitman/.zizmor_meta/blob/514f583f97e1fda98861582cfdd4bebfe7f639f8/zizmor_blast.bash

To do bulk scans which I manually cache to pick through later. I'm not sure if we actually use crabwatch on any repos that are archived, not public, or forks, but I've started by prioritizing the public, non-archived, non-forks. In general, the crabwatch custom property is what decides if we run crabwatch on a repo. You'd probably get more accurate results by running the actual crabwatch wrapper, but I'm stubborn and don't feel like downloading yq just for this, yet.

[jieyouxu]

Ah okay, thanks for the context.

[jieyouxu]

Hm yeah alright, I didn't consider the external factor possibility (i.e. an action can get archived and now if you hard-require zizmor nothing can be merged). That makes sense 👍

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[ytmimi]

Thanks @hashcatHitman and @jieyouxu. I haven't had the time to fully look into zizmore, but I trust that these GHA changes are good.

View changes since this review