From 860552be4f9a232aedf7cf5e10fdd8a1d929be7e Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 30 Jul 2026 23:56:16 -0400 Subject: [PATCH] ci(security): harden checkout credentials, pin actions, fail-closed tag check Three supply-chain items. The first two carry over from the sibling project's `v2.2.2` audit (logged in the lockstep checklist but never scoped to a rung); the third was raised by CodeRabbit reviewing #275 and declined there because pinning only the new job would have left the repo inconsistent while claiming to have addressed it. ## `persist-credentials: false` on 15 of 17 checkouts `actions/checkout` writes the workflow `GITHUB_TOKEN` into `.git/config`, where anything the job then executes from the checked-out tree -- build scripts, proc macros, test binaries, `scripts/*.sh`, the MkDocs build -- can read it. On a pull request that tree is by definition unreviewed code, and nearly every job here compiles or runs it. Highest exposure was not a `ci.yml` job but `web.yml`'s `build`, whose workflow-level `permissions:` grant `pages: write` + `id-token: write` while it runs `trunk`, `cargo doc`, `pip install`, and `mkdocs build`. Audited per site rather than applied blanket, and that distinction mattered: **`release-auto.yml`'s `prepare` job genuinely needs the credential.** It creates an annotated tag and `git push origin "$TAG"`, which authenticates through exactly that token. The sibling's audit concluded no job needed it -- that conclusion does NOT transfer here, and a blanket sweep would have silently broken every future release. It is now the only checkout in the repository that keeps the token, with the reason recorded inline so the exception reads as a decision. Its exposure is bounded by its trigger: `workflow_run` on a completed CI run of `main`, never a pull request, so the tree it executes is already-reviewed code. `antigravity-review.yml` already had the setting and is unchanged. ## The release-tag check now fails closed It was `git ls-remote --exit-code --tags origin "refs/tags/${tag}"`, which collapses THREE outcomes into two -- tag present (0), tag absent (2), and lookup failed (non-zero, various) -- reading any non-zero as "absent". A transient network blip, an auth hiccup, or a rate limit therefore sent an ALREADY-RELEASED version down the `should_release=true` path, re-tagging a release `to-dos/ROADMAP.md`'s ceremony treats as immutable. Now `gh api git/matching-refs/tags/`, chosen over `git/ref/tags/` because it answers "absent" with HTTP 200 and an empty array, so a genuine miss can never be confused with an error and no error-body parsing is needed. That endpoint matches by PREFIX, so the exact ref is compared in `jq`. Verified necessary rather than assumed: against this repository, `v1.2` prefix-matches SEVEN real tags while exact-matching none, so a naive `length > 0` would report an unreleased version as already released. Exercised all four cases against the live API -- existing tag, absent tag, prefix-only match, lookup failure. The step had no token of its own (it previously used only plain `git`), so `GH_TOKEN` is added to it; `gh` errors out without one rather than falling back to an ambient credential. ## All 34 action references pinned to commit SHAs Across all 8 workflows plus the composite `rust-setup` action. An upstream tag move could previously change what CI executes with no repository review. Each pin carries its original tag as a trailing comment so intent stays readable at a glance. Also aligned one `actions/upload-artifact@v4` -- introduced with the fuzz job in the previous commit -- to the `@v7` the rest of the repo already used, before pinning it. ## Verification `actionlint` reports the SAME three findings before and after (a self-hosted runner label, two pre-existing shellcheck notes), confirmed by linting the HEAD copies side by side -- no new lint surface. All nine files parse as YAML. The tag-check logic was exercised against the live GitHub API, including the prefix trap. Co-Authored-By: Claude Opus 5 (1M context) --- .github/actions/rust-setup/action.yml | 4 +-- .github/workflows/antigravity-review.yml | 2 +- .github/workflows/ci.yml | 30 +++++++++++----- .github/workflows/ios.yml | 4 ++- .github/workflows/pgo.yml | 12 ++++--- .github/workflows/release-auto.yml | 44 +++++++++++++++++++++--- .github/workflows/release.yml | 7 ++-- .github/workflows/security.yml | 22 +++++++----- .github/workflows/web.yml | 14 ++++---- CHANGELOG.md | 41 ++++++++++++++++++++++ 10 files changed, 143 insertions(+), 37 deletions(-) diff --git a/.github/actions/rust-setup/action.yml b/.github/actions/rust-setup/action.yml index f50e1873..6e4e244c 100644 --- a/.github/actions/rust-setup/action.yml +++ b/.github/actions/rust-setup/action.yml @@ -34,12 +34,12 @@ runs: # `@master`, which floats the ACTION's own implementation and can change CI behavior out from # under the pinned Rust toolchain input below without warning — caught in PR review). The # `toolchain:` input still wins over whatever default this ref's snapshot implies. - - uses: dtolnay/rust-toolchain@1.96 + - uses: dtolnay/rust-toolchain@2fe4ca74464c5902a4f6e302d0a619b4ea911ccc # 1.96 with: toolchain: ${{ inputs.toolchain }} components: ${{ inputs.components }} targets: ${{ inputs.targets }} - - uses: Swatinem/rust-cache@v2 + - uses: Swatinem/rust-cache@e18b497796c12c097a38f9edb9d0641fb99eee32 # v2 with: key: ${{ inputs.cache-key-suffix }} - name: Install Linux frontend system deps diff --git a/.github/workflows/antigravity-review.yml b/.github/workflows/antigravity-review.yml index fa3d9d0a..32ba8cf4 100644 --- a/.github/workflows/antigravity-review.yml +++ b/.github/workflows/antigravity-review.yml @@ -63,7 +63,7 @@ jobs: # branch throughout. Consequence worth knowing: a PR that edits the reviewer or the # style guide is reviewed by the version already on the default branch until it merges. - name: Check out repo (for the style guide + scripts) - uses: actions/checkout@v7 + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 with: ref: ${{ github.event.repository.default_branch }} # Not `fetch-depth: 0`: the large-diff fallback fetches exactly the two refs it diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b8713ffb..a9388d45 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -29,8 +29,10 @@ jobs: outputs: code: ${{ steps.filter.outputs.code }} steps: - - uses: actions/checkout@v7 - - uses: dorny/paths-filter@v3 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + persist-credentials: false + - uses: dorny/paths-filter@d1c1ffe0248fe513906c8e24db8ea791d46f8590 # v3 id: filter with: filters: | @@ -88,7 +90,9 @@ jobs: CARGO_HTTP_MULTIPLEXING: "false" runs-on: ubuntu-latest steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + persist-credentials: false # fmt/clippy only ever check the HOST target here (no `--target` is ever passed to either # command in this job) — installing the wasm32/thumbv7em cross targets bought nothing but # setup time on every single PR push. Only `no_std` (which actually cross-compiles) needs @@ -184,7 +188,9 @@ jobs: CARGO_HTTP_MULTIPLEXING: "false" runs-on: ubuntu-latest steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + persist-credentials: false - uses: ./.github/actions/rust-setup with: cache-key-suffix: test-light @@ -206,7 +212,9 @@ jobs: os: [ubuntu-latest, macos-latest, windows-latest] runs-on: ${{ matrix.os }} steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + persist-credentials: false # Same rationale as `lint`: neither fmt nor clippy nor `cargo test` cross-compiles here, so # no extra targets are installed. - uses: ./.github/actions/rust-setup @@ -265,7 +273,9 @@ jobs: - rustysnes-cart - rustysnes-core steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + persist-credentials: false - uses: ./.github/actions/rust-setup with: targets: thumbv7em-none-eabihf @@ -285,7 +295,9 @@ jobs: CARGO_HTTP_MULTIPLEXING: "false" runs-on: ubuntu-latest steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + persist-credentials: false # The `rustysnes-core` `headless_frame` bench is chip-stack-only (no wgpu/winit/egui), so # no Linux frontend system deps are needed here (unlike `lint`/`full-test`). - uses: ./.github/actions/rust-setup @@ -301,7 +313,9 @@ jobs: if: needs.setup.outputs.mode != 'skip' runs-on: ubuntu-latest steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + persist-credentials: false - uses: ./.github/actions/rust-setup with: cache-key-suffix: accuracysnes diff --git a/.github/workflows/ios.yml b/.github/workflows/ios.yml index 6b177d95..50415b1d 100644 --- a/.github/workflows/ios.yml +++ b/.github/workflows/ios.yml @@ -40,7 +40,9 @@ jobs: CARGO_NET_RETRY: "10" CARGO_HTTP_MULTIPLEXING: "false" steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + persist-credentials: false - uses: ./.github/actions/rust-setup with: # `dtolnay/rust-toolchain` ignores its OWN `toolchain:`/`components:`/`targets:` inputs diff --git a/.github/workflows/pgo.yml b/.github/workflows/pgo.yml index 86a7a32b..4610d6eb 100644 --- a/.github/workflows/pgo.yml +++ b/.github/workflows/pgo.yml @@ -73,7 +73,9 @@ jobs: promotable: ${{ steps.gate.outputs.promotable }} speedup_pct: ${{ steps.gate.outputs.speedup_pct }} steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + persist-credentials: false # `components: llvm-tools-preview` is listed for documentation, but the EFFECTIVE source is # `rust-toolchain.toml` (which now includes it) -- `dtolnay/rust-toolchain` silently ignores @@ -173,7 +175,7 @@ jobs: - name: Upload promoted PGO binary if: steps.gate.outputs.promotable == 'true' - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 with: name: rustysnes-pgo-${{ steps.triple.outputs.triple }} path: target/${{ steps.triple.outputs.triple }}/release/rustysnes @@ -194,7 +196,9 @@ jobs: permissions: contents: read steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + persist-credentials: false - uses: ./.github/actions/rust-setup with: components: llvm-tools-preview @@ -282,7 +286,7 @@ jobs: - name: Upload BOLT binary if: steps.bolt_probe.outputs.have_bolt == 'true' - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 with: name: rustysnes-bolt-${{ steps.triple.outputs.triple }} path: target/${{ steps.triple.outputs.triple }}/release/rustysnes diff --git a/.github/workflows/release-auto.yml b/.github/workflows/release-auto.yml index e1c88818..477b72ba 100644 --- a/.github/workflows/release-auto.yml +++ b/.github/workflows/release-auto.yml @@ -53,17 +53,34 @@ jobs: should_release: ${{ steps.decide.outputs.should_release }} tag: ${{ steps.decide.outputs.tag }} steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 with: # Build the release from the exact commit CI went green on. Full history so the - # annotated-tag creation and the `git ls-remote` existence check both have what they - # need (a shallow clone's tag/ref visibility isn't reliable here). + # annotated-tag creation and the tag-existence check both have what they need (a shallow + # clone's tag/ref visibility isn't reliable here). ref: ${{ github.event.workflow_run.head_sha }} fetch-depth: 0 + # DELIBERATELY NOT `persist-credentials: false`, and the only checkout in the repository + # that keeps the token. Every other one was hardened in `v1.26.0` because checkout writes + # the workflow `GITHUB_TOKEN` into `.git/config`, where anything the job then executes + # from the tree can read it. + # + # This job is the exception because it genuinely needs it: it creates an annotated tag and + # `git push origin "$TAG"` (below), which authenticates through exactly that credential. + # The sibling project's audit concluded no job needed the token; that conclusion does NOT + # transfer here, which is why this was audited per-site rather than applied blanket. + # + # The exposure is bounded by the trigger: `workflow_run` on a completed CI run of `main`, + # never a pull request, so the tree this job executes is already-reviewed code. - name: Decide whether a closed-out version needs releasing id: decide shell: bash + env: + # Required by the `gh api` tag-existence check below. `gh` errors out without it rather + # than falling back to the ambient credential, and this step previously used only plain + # `git`, so it had no token of its own. Same form as the release steps further down. + GH_TOKEN: ${{ github.token }} run: | set -euo pipefail # ANY non-blank content under [Unreleased] (not just a "- " bullet -- a stray heading @@ -93,7 +110,26 @@ jobs: echo "tag=${tag}" >> "$GITHUB_OUTPUT" echo "version=${version}" >> "$GITHUB_OUTPUT" echo "header=${header}" >> "$GITHUB_OUTPUT" - if git ls-remote --exit-code --tags origin "refs/tags/${tag}" >/dev/null 2>&1; then + # FAIL CLOSED. This was `git ls-remote --exit-code --tags origin "refs/tags/${tag}"`, + # which collapses THREE outcomes into two: tag present (0), tag absent (2), and lookup + # failed (non-zero, various). Reading any non-zero as "absent" means a transient network + # blip, an auth hiccup, or a rate limit sends an ALREADY-RELEASED version down the + # `should_release=true` path — re-tagging a published release, which + # `to-dos/ROADMAP.md`'s release ceremony treats as immutable. + # + # `git/matching-refs` is chosen over `git/ref/tags/` deliberately: it answers + # "absent" with HTTP 200 and an empty array, so a genuine miss can never be confused with + # an error and no error-body parsing is needed. It matches by PREFIX, though, so the exact + # ref is compared in `jq` — that is not theoretical, `v1.2` prefix-matches several real + # tags while exact-matching none. + # + # Every failure path aborts the job (the script runs under `set -euo pipefail`) rather + # than resolving to a release decision. + if ! matching="$(gh api "repos/${GITHUB_REPOSITORY}/git/matching-refs/tags/${tag}")"; then + echo "::error::Tag lookup for ${tag} failed. Refusing to guess whether it exists." + exit 1 + fi + if printf '%s' "$matching" | jq -e --arg ref "refs/tags/${tag}" 'any(.[]; .ref == $ref)' >/dev/null; then echo "Tag ${tag} already exists -- nothing to release." echo "should_release=false" >> "$GITHUB_OUTPUT" else diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index cc4b2878..15527b20 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -43,13 +43,14 @@ jobs: # `push` trigger, where `github.ref_name` is already the pushed tag). TAG: ${{ inputs.tag || github.ref_name }} steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 with: + persist-credentials: false ref: ${{ env.TAG }} - - uses: dtolnay/rust-toolchain@stable + - uses: dtolnay/rust-toolchain@4cda84d5c5c54efe2404f9d843567869ab1699d4 # stable with: targets: ${{ matrix.target }} - - uses: Swatinem/rust-cache@v2 + - uses: Swatinem/rust-cache@e18b497796c12c097a38f9edb9d0641fb99eee32 # v2 with: key: release-${{ matrix.target }} - name: Install Linux frontend system deps diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index 33b65ead..138433a1 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -51,12 +51,14 @@ jobs: name: Dependency Audit runs-on: ubuntu-latest steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + persist-credentials: false # Install the PREBUILT binary, never `cargo install` (build-from-source): the repo pins # rustc 1.96, but a from-source cargo-audit build can need a newer toolchain than that to # COMPILE. The prebuilt binary runs fine under any toolchain -- it only parses Cargo.lock, # it never compiles this project. - - uses: taiki-e/install-action@v2 + - uses: taiki-e/install-action@6a1bd70eaac3c8bdf093356838d7ee09fda951cf # v2 with: tool: cargo-audit - run: cargo audit @@ -74,10 +76,12 @@ jobs: name: Cargo Deny Check runs-on: ubuntu-latest steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + persist-credentials: false # Same rationale as the audit job: the prebuilt binary avoids needing a newer toolchain # than the repo's pinned 1.96 just to build the checker itself. Policy lives in `deny.toml`. - - uses: taiki-e/install-action@v2 + - uses: taiki-e/install-action@6a1bd70eaac3c8bdf093356838d7ee09fda951cf # v2 with: tool: cargo-deny - run: cargo deny check @@ -112,12 +116,14 @@ jobs: # indistinguishably from a real finding. timeout-minutes: 180 steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + persist-credentials: false # cargo-fuzz needs nightly (`-Z sanitizer=address`); the project pins 1.96 stable in # `rust-toolchain.toml`. Installing nightly alongside it, and invoking it explicitly as # `+nightly`, keeps the pin authoritative for every other job. - - uses: dtolnay/rust-toolchain@nightly - - uses: taiki-e/install-action@v2 + - uses: dtolnay/rust-toolchain@4fd1da8b0805d2d2e936788875a7d65dbd677dc2 # nightly + - uses: taiki-e/install-action@6a1bd70eaac3c8bdf093356838d7ee09fda951cf # v2 with: tool: cargo-fuzz # `fuzz/` depends on `rustysnes-frontend` (the patch/preset/config/symbols boundaries live @@ -162,7 +168,7 @@ jobs: # cargo +nightly fuzz run - name: Upload crashing inputs if: failure() - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 with: name: fuzz-artifacts path: | diff --git a/.github/workflows/web.yml b/.github/workflows/web.yml index fa3d1fed..b465ed32 100644 --- a/.github/workflows/web.yml +++ b/.github/workflows/web.yml @@ -65,7 +65,9 @@ jobs: name: build demo + docs runs-on: ubuntu-latest steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + persist-credentials: false # GOTCHA: keep crates/rustysnes-frontend/web/Trunk.toml's wasm-bindgen pin == Cargo.lock # library version. `linux-frontend-deps: "true"` because the rustdoc build below compiles # the whole workspace (incl. rustysnes-frontend) for the host, which needs the native @@ -81,7 +83,7 @@ jobs: # + its whole dependency tree from source on every single run — minutes of pure setup cost # for a tool whose own source never changes here). - name: Install trunk - uses: taiki-e/install-action@v2 + uses: taiki-e/install-action@6a1bd70eaac3c8bdf093356838d7ee09fda951cf # v2 with: tool: trunk @@ -112,7 +114,7 @@ jobs: run: cargo doc --workspace --exclude rustysnes-android --no-deps - name: Set up Python for MkDocs - uses: actions/setup-python@v5 + uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5 with: python-version: "3.12" @@ -148,11 +150,11 @@ jobs: - name: Configure Pages if: github.event_name != 'pull_request' - uses: actions/configure-pages@v5 + uses: actions/configure-pages@983d7736d9b0ae728b81ab479565c72886d7745b # v5 - name: Upload Pages artifact (demo + docs) if: github.event_name != 'pull_request' - uses: actions/upload-pages-artifact@v3 + uses: actions/upload-pages-artifact@56afc609e74202658d3ffba0e8f6dda462b719fa # v3 with: path: _site @@ -168,4 +170,4 @@ jobs: steps: - name: Deploy to GitHub Pages id: deployment - uses: actions/deploy-pages@v4 + uses: actions/deploy-pages@d6db90164ac5ed86f2b6aed7e0febac5b3c0c03e # v4 diff --git a/CHANGELOG.md b/CHANGELOG.md index aa2eab49..d6c3b52f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,47 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Security + +- **CI supply-chain hardening — every workflow credential, action pin, and the release gate.** + Three items, the first two carried over from the sibling project's `v2.2.2` audit and the third + raised in review of the fuzzing PR. + + **`persist-credentials: false` on 15 of 17 checkouts.** `actions/checkout` writes the workflow + `GITHUB_TOKEN` into `.git/config`, where anything the job then executes from the tree — build + scripts, proc macros, test binaries, `scripts/*.sh`, the MkDocs build — can read it. On a pull + request that tree is by definition unreviewed code, and nearly every job here compiles or runs + it. Highest exposure was `web.yml`'s `build`, which holds `pages: write` + `id-token: write` + while running `trunk`, `cargo doc`, `pip install`, and `mkdocs build`. + + Audited per site rather than applied blanket, and that mattered: **`release-auto.yml`'s `prepare` + job genuinely needs the credential** — it creates an annotated tag and `git push origin "$TAG"`, + which authenticates through exactly that token. The sibling's audit concluded no job needed it; + that conclusion does not transfer, and a blanket sweep would have broken every future release. + It is now the only checkout in the repository that keeps the token, with the reason recorded + inline, and its exposure is bounded by its trigger (`workflow_run` on a completed CI run of + `main`, never a pull request). + + **The release-tag existence check now fails closed.** It was + `git ls-remote --exit-code --tags origin "refs/tags/${tag}"`, which collapses three outcomes into + two — tag present, tag absent, and *lookup failed* — reading any non-zero exit as "absent". A + transient network blip, auth hiccup, or rate limit therefore sent an already-released version + down the `should_release=true` path, re-tagging a release the ceremony treats as immutable. Now a + `gh api git/matching-refs/tags/` call, chosen over `git/ref/tags/` because it answers + "absent" with HTTP 200 and an empty array, so a genuine miss can never be confused with an error. + That endpoint matches by **prefix**, so the exact ref is compared in `jq` — verified necessary, + not theoretical: `v1.2` prefix-matches **7** real tags while exact-matching none, and a naive + `length > 0` would have reported it as already released. Every failure path aborts the job. + + **All 34 third-party action references pinned to immutable commit SHAs**, across all 8 workflows + plus the composite `rust-setup` action. An upstream tag move could previously change what CI + executes without any repository review. Each pin carries its original tag as a trailing comment + so the intent stays readable. Also aligned one `actions/upload-artifact@v4` (introduced with the + fuzz job) to the `@v7` the rest of the repo already used. + + Verified: `actionlint` reports the same three findings before and after (a self-hosted runner + label and two pre-existing shellcheck notes) — no new lint surface; all nine files parse. + ### Added - **Fuzzing infrastructure (`fuzz/`) — fourteen `cargo-fuzz` targets, one per untrusted-input