From 58e7db89a17454f6af4688ab19ec9e44ebcc66e4 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Mon, 9 Dec 2024 00:42:06 +0000 Subject: [PATCH] Run zizmor in CI, and fix most warnings (#14844) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary A [recent exploit](https://github.com/advisories/GHSA-7x29-qqmq-v6qc) brought attention to how easy it can be for attackers to use template expansion in GitHub Actions workflows to inject arbitrary code into a repository. That vulnerability [would have been caught by the zizmor linter](https://blog.yossarian.net/2024/12/06/zizmor-ultralytics-injection), which looks for potential security vulnerabilities in GitHub Actions workflows. This PR adds [zizmor](https://github.com/woodruffw/zizmor) as a pre-commit hook and fixes the high- and medium-severity warnings flagged by the tool. All the warnings fixed in this PR are related to this zizmor check: https://woodruffw.github.io/zizmor/audits/#artipacked. The summary of the check is that `actions/checkout` will by default persist git configuration for the duration of the workflow, which can be insecure. It's unnecessary unless you actually need to do things with `git` later on in the workflow. None of our workflows do except for `publish-docs.yml` and `sync-typeshed.yml`, so I set `persist-credentials: true` for those two but `persist-credentials: false` for all other uses of `actions/checkout`. Unfortunately there are several warnings in `release.yml`, including four high-severity warnings. However, this is a generated workflow file, so I have deliberately excluded this file from the check. These are the findings in `release.yml`:
release.yml findings ``` warning[artipacked]: credential persistence through GitHub Actions artifacts --> /Users/alexw/dev/ruff/.github/workflows/release.yml:62:9 | 62 | - uses: actions/checkout@v4 | _________- 63 | | with: 64 | | submodules: recursive | |_______________________________- does not set persist-credentials: false | = note: audit confidence → Low warning[artipacked]: credential persistence through GitHub Actions artifacts --> /Users/alexw/dev/ruff/.github/workflows/release.yml:124:9 | 124 | - uses: actions/checkout@v4 | _________- 125 | | with: 126 | | submodules: recursive | |_______________________________- does not set persist-credentials: false | = note: audit confidence → Low warning[artipacked]: credential persistence through GitHub Actions artifacts --> /Users/alexw/dev/ruff/.github/workflows/release.yml:174:9 | 174 | - uses: actions/checkout@v4 | _________- 175 | | with: 176 | | submodules: recursive | |_______________________________- does not set persist-credentials: false | = note: audit confidence → Low warning[artipacked]: credential persistence through GitHub Actions artifacts --> /Users/alexw/dev/ruff/.github/workflows/release.yml:249:9 | 249 | - uses: actions/checkout@v4 | _________- 250 | | with: 251 | | submodules: recursive 252 | | # Create a GitHub Release while uploading all files to it | |_______________________________________________________________- does not set persist-credentials: false | = note: audit confidence → Low error[excessive-permissions]: overly broad workflow or job-level permissions --> /Users/alexw/dev/ruff/.github/workflows/release.yml:17:1 | 17 | / permissions: 18 | | "contents": "write" ... | 39 | | # If there's a prerelease-style suffix to the version, then the release(s) 40 | | # will be marked as a prerelease. | |_________________________________^ contents: write is overly broad at the workflow level | = note: audit confidence → High error[template-injection]: code injection via template expansion --> /Users/alexw/dev/ruff/.github/workflows/release.yml:80:9 | 80 | - id: plan | _________^ 81 | | run: | | |_________^ 82 | || dist ${{ (inputs.tag && inputs.tag != 'dry-run' && format('host --steps=create --tag={0}', inputs.tag)) || 'plan' }} --out... 83 | || echo "dist ran successfully" 84 | || cat plan-dist-manifest.json 85 | || echo "manifest=$(jq -c "." plan-dist-manifest.json)" >> "$GITHUB_OUTPUT" | ||__________________________________________________________________________________^ this step | ||__________________________________________________________________________________^ inputs.tag may expand into attacker-controllable code | = note: audit confidence → Low error[template-injection]: code injection via template expansion --> /Users/alexw/dev/ruff/.github/workflows/release.yml:80:9 | 80 | - id: plan | _________^ 81 | | run: | | |_________^ 82 | || dist ${{ (inputs.tag && inputs.tag != 'dry-run' && format('host --steps=create --tag={0}', inputs.tag)) || 'plan' }} --out... 83 | || echo "dist ran successfully" 84 | || cat plan-dist-manifest.json 85 | || echo "manifest=$(jq -c "." plan-dist-manifest.json)" >> "$GITHUB_OUTPUT" | ||__________________________________________________________________________________^ this step | ||__________________________________________________________________________________^ inputs.tag may expand into attacker-controllable code | = note: audit confidence → Low error[template-injection]: code injection via template expansion --> /Users/alexw/dev/ruff/.github/workflows/release.yml:80:9 | 80 | - id: plan | _________^ 81 | | run: | | |_________^ 82 | || dist ${{ (inputs.tag && inputs.tag != 'dry-run' && format('host --steps=create --tag={0}', inputs.tag)) || 'plan' }} --out... 83 | || echo "dist ran successfully" 84 | || cat plan-dist-manifest.json 85 | || echo "manifest=$(jq -c "." plan-dist-manifest.json)" >> "$GITHUB_OUTPUT" | ||__________________________________________________________________________________^ this step | ||__________________________________________________________________________________^ inputs.tag may expand into attacker-controllable code | = note: audit confidence → Low ```
## Test Plan `uvx pre-commit run -a` --- .github/workflows/build-binaries.yml | 8 +++++ .github/workflows/build-docker.yml | 1 + .github/workflows/ci.yaml | 38 ++++++++++++++++++++++++ .github/workflows/daily_fuzz.yaml | 2 ++ .github/workflows/publish-docs.yml | 1 + .github/workflows/publish-playground.yml | 2 ++ .github/workflows/publish-wasm.yml | 2 ++ .github/workflows/sync_typeshed.yaml | 2 ++ .pre-commit-config.yaml | 15 ++++++++++ 9 files changed, 71 insertions(+) diff --git a/.github/workflows/build-binaries.yml b/.github/workflows/build-binaries.yml index aa65037cae..e71cb3b0e2 100644 --- a/.github/workflows/build-binaries.yml +++ b/.github/workflows/build-binaries.yml @@ -40,6 +40,7 @@ jobs: - uses: actions/checkout@v4 with: submodules: recursive + persist-credentials: false - uses: actions/setup-python@v5 with: python-version: ${{ env.PYTHON_VERSION }} @@ -68,6 +69,7 @@ jobs: - uses: actions/checkout@v4 with: submodules: recursive + persist-credentials: false - uses: actions/setup-python@v5 with: python-version: ${{ env.PYTHON_VERSION }} @@ -109,6 +111,7 @@ jobs: - uses: actions/checkout@v4 with: submodules: recursive + persist-credentials: false - uses: actions/setup-python@v5 with: python-version: ${{ env.PYTHON_VERSION }} @@ -164,6 +167,7 @@ jobs: - uses: actions/checkout@v4 with: submodules: recursive + persist-credentials: false - uses: actions/setup-python@v5 with: python-version: ${{ env.PYTHON_VERSION }} @@ -216,6 +220,7 @@ jobs: - uses: actions/checkout@v4 with: submodules: recursive + persist-credentials: false - uses: actions/setup-python@v5 with: python-version: ${{ env.PYTHON_VERSION }} @@ -290,6 +295,7 @@ jobs: - uses: actions/checkout@v4 with: submodules: recursive + persist-credentials: false - uses: actions/setup-python@v5 with: python-version: ${{ env.PYTHON_VERSION }} @@ -354,6 +360,7 @@ jobs: - uses: actions/checkout@v4 with: submodules: recursive + persist-credentials: false - uses: actions/setup-python@v5 with: python-version: ${{ env.PYTHON_VERSION }} @@ -419,6 +426,7 @@ jobs: - uses: actions/checkout@v4 with: submodules: recursive + persist-credentials: false - uses: actions/setup-python@v5 with: python-version: ${{ env.PYTHON_VERSION }} diff --git a/.github/workflows/build-docker.yml b/.github/workflows/build-docker.yml index b4ba150656..c57b3a0c96 100644 --- a/.github/workflows/build-docker.yml +++ b/.github/workflows/build-docker.yml @@ -36,6 +36,7 @@ jobs: - uses: actions/checkout@v4 with: submodules: recursive + persist-credentials: false - uses: docker/setup-buildx-action@v3 diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 92556526f8..9d8c157715 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -38,6 +38,7 @@ jobs: - uses: actions/checkout@v4 with: fetch-depth: 0 + persist-credentials: false - uses: tj-actions/changed-files@v45 id: changed @@ -99,6 +100,8 @@ jobs: timeout-minutes: 10 steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - name: "Install Rust toolchain" run: rustup component add rustfmt - run: cargo fmt --all --check @@ -111,6 +114,8 @@ jobs: timeout-minutes: 20 steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - name: "Install Rust toolchain" run: | rustup component add clippy @@ -129,6 +134,8 @@ jobs: timeout-minutes: 20 steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - name: "Install Rust toolchain" run: rustup show - name: "Install mold" @@ -173,6 +180,8 @@ jobs: timeout-minutes: 20 steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - name: "Install Rust toolchain" run: rustup show - name: "Install mold" @@ -200,6 +209,8 @@ jobs: timeout-minutes: 20 steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - name: "Install Rust toolchain" run: rustup show - name: "Install cargo nextest" @@ -224,6 +235,8 @@ jobs: timeout-minutes: 10 steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - name: "Install Rust toolchain" run: rustup target add wasm32-unknown-unknown - uses: actions/setup-node@v4 @@ -251,6 +264,8 @@ jobs: timeout-minutes: 20 steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - name: "Install Rust toolchain" run: rustup show - name: "Install mold" @@ -267,6 +282,8 @@ jobs: timeout-minutes: 20 steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - uses: SebRollen/toml-action@v1.2.0 id: msrv with: @@ -299,6 +316,8 @@ jobs: timeout-minutes: 10 steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - name: "Install Rust toolchain" run: rustup show - uses: Swatinem/rust-cache@v2 @@ -325,6 +344,8 @@ jobs: FORCE_COLOR: 1 steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - uses: astral-sh/setup-uv@v4 - uses: actions/download-artifact@v4 name: Download Ruff binary to test @@ -355,6 +376,8 @@ jobs: timeout-minutes: 5 steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - name: "Install Rust toolchain" run: rustup component add rustfmt - uses: Swatinem/rust-cache@v2 @@ -379,6 +402,8 @@ jobs: timeout-minutes: 20 steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - uses: actions/setup-python@v5 with: python-version: ${{ env.PYTHON_VERSION }} @@ -489,6 +514,8 @@ jobs: if: ${{ needs.determine_changes.outputs.code == 'true' || github.ref == 'refs/heads/main' }} steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - uses: cargo-bins/cargo-binstall@main - run: cargo binstall --no-confirm cargo-shear - run: cargo shear @@ -499,6 +526,8 @@ jobs: timeout-minutes: 20 steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - uses: actions/setup-python@v5 with: python-version: ${{ env.PYTHON_VERSION }} @@ -524,6 +553,8 @@ jobs: timeout-minutes: 10 steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - uses: actions/setup-python@v5 with: python-version: ${{ env.PYTHON_VERSION }} @@ -555,6 +586,8 @@ jobs: MKDOCS_INSIDERS_SSH_KEY_EXISTS: ${{ secrets.MKDOCS_INSIDERS_SSH_KEY != '' }} steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - uses: actions/setup-python@v5 with: python-version: "3.13" @@ -595,6 +628,8 @@ jobs: timeout-minutes: 10 steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - name: "Install Rust toolchain" run: rustup show - name: "Cache rust" @@ -622,6 +657,7 @@ jobs: - uses: actions/checkout@v4 name: "Download ruff-lsp source" with: + persist-credentials: false repository: "astral-sh/ruff-lsp" - uses: actions/setup-python@v5 @@ -657,6 +693,8 @@ jobs: steps: - name: "Checkout Branch" uses: actions/checkout@v4 + with: + persist-credentials: false - name: "Install Rust toolchain" run: rustup show diff --git a/.github/workflows/daily_fuzz.yaml b/.github/workflows/daily_fuzz.yaml index e4f31e7790..a3a4e84d99 100644 --- a/.github/workflows/daily_fuzz.yaml +++ b/.github/workflows/daily_fuzz.yaml @@ -32,6 +32,8 @@ jobs: if: ${{ github.repository == 'astral-sh/ruff' || github.event_name != 'schedule' }} steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - uses: astral-sh/setup-uv@v4 - name: "Install Rust toolchain" run: rustup show diff --git a/.github/workflows/publish-docs.yml b/.github/workflows/publish-docs.yml index fe2dd5edcb..1192d54dcb 100644 --- a/.github/workflows/publish-docs.yml +++ b/.github/workflows/publish-docs.yml @@ -26,6 +26,7 @@ jobs: - uses: actions/checkout@v4 with: ref: ${{ inputs.ref }} + persist-credentials: true - uses: actions/setup-python@v5 with: diff --git a/.github/workflows/publish-playground.yml b/.github/workflows/publish-playground.yml index 8e64c55e50..648ef1da2a 100644 --- a/.github/workflows/publish-playground.yml +++ b/.github/workflows/publish-playground.yml @@ -25,6 +25,8 @@ jobs: CF_API_TOKEN_EXISTS: ${{ secrets.CF_API_TOKEN != '' }} steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - name: "Install Rust toolchain" run: rustup target add wasm32-unknown-unknown - uses: actions/setup-node@v4 diff --git a/.github/workflows/publish-wasm.yml b/.github/workflows/publish-wasm.yml index c33406f118..e2996eca29 100644 --- a/.github/workflows/publish-wasm.yml +++ b/.github/workflows/publish-wasm.yml @@ -30,6 +30,8 @@ jobs: fail-fast: false steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - name: "Install Rust toolchain" run: rustup target add wasm32-unknown-unknown - uses: jetli/wasm-pack-action@v0.4.0 diff --git a/.github/workflows/sync_typeshed.yaml b/.github/workflows/sync_typeshed.yaml index 4e74ca645f..30a472e028 100644 --- a/.github/workflows/sync_typeshed.yaml +++ b/.github/workflows/sync_typeshed.yaml @@ -25,11 +25,13 @@ jobs: name: Checkout Ruff with: path: ruff + persist-credentials: true - uses: actions/checkout@v4 name: Checkout typeshed with: repository: python/typeshed path: typeshed + persist-credentials: true - name: Setup git run: | git config --global user.name typeshedbot diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 6b2fd2d14d..3cf893cf5d 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -88,5 +88,20 @@ repos: - id: prettier types: [yaml] + - repo: https://github.com/woodruffw/zizmor-pre-commit + rev: v0.8.0 + hooks: + - id: zizmor + # `release.yml` is autogenerated by `dist`; security issues need to be fixed there + # (https://opensource.axo.dev/cargo-dist/) + exclude: .github/workflows/release.yml + # We could consider enabling the low-severity warnings, but they're noisy + args: [--min-severity=medium] + + - repo: https://github.com/python-jsonschema/check-jsonschema + rev: 0.29.4 + hooks: + - id: check-github-workflows + ci: skip: [cargo-fmt, dev-generate-all]