diff --git a/.github/actions/rust-setup/action.yml b/.github/actions/rust-setup/action.yml index 87f01bb5..1ba1509b 100644 --- a/.github/actions/rust-setup/action.yml +++ b/.github/actions/rust-setup/action.yml @@ -37,8 +37,23 @@ inputs: runs: using: composite steps: + # Pinned to a commit SHA, not `@master`. Every other action in this repo is + # referenced by a `@vN` tag, which is not expected to move; `@master` is a + # BRANCH that advances on every upstream commit, so each run silently + # resolved to whatever HEAD happened to be — no review window, no signal + # that anything changed. That matters disproportionately here: this + # composite is consumed by 12 of the repo's 19 checkouts, including + # `release.yml` (`contents: write`, builds the shipped binaries) and + # `web.yml` (`pages: write` + `id-token: write`), and it is the action that + # installs the compiler — a position where a compromise is very hard to + # detect from the build output. + # + # The trailing `# v1` is the convention Dependabot's `github-actions` + # ecosystem (already enabled in `.github/dependabot.yml`) reads to keep the + # pin current, so this does not trade a supply-chain risk for a stale-action + # one. Verified: `refs/tags/v1` -> e97e2d8c at time of pinning. - name: Install Rust toolchain - uses: dtolnay/rust-toolchain@master + uses: dtolnay/rust-toolchain@e97e2d8cc328f1b50210efc529dca0028893a2d9 # v1 with: toolchain: ${{ inputs.toolchain }} targets: ${{ inputs.targets }} diff --git a/.github/workflows/release-auto.yml b/.github/workflows/release-auto.yml index 276bf0ff..b1e99fa6 100644 --- a/.github/workflows/release-auto.yml +++ b/.github/workflows/release-auto.yml @@ -69,39 +69,22 @@ jobs: # Build the release from the exact commit CI went green on. A shallow # checkout suffices: we only read plain text files (Cargo.toml, # CHANGELOG.md, and the optional `.github/release-notes/vX.Y.Z.md` - # override), and the tag existence check uses `git ls-remote` (no - # local tag history needed). + # override), and the tag existence check is an API call, not a Git + # operation. ref: ${{ github.event.workflow_run.head_sha }} - # - # DELIBERATELY the one checkout in this repo that KEEPS persisted Git - # credentials (every other one sets `persist-credentials: false`; see - # issue #318). The `git ls-remote --tags origin` below is the only - # operation in this repo that needs THIS checkout's `origin` - # credential. (It is not the only Git-over-network call in `.github/` - # — `fastlane match` in `ios.yml` clones the signing repository — but - # that one authenticates with its own `MATCH_GIT_*` secrets against a - # different remote, so it is unaffected by this setting.) This job is - # also unreachable from untrusted input — `workflow_run`, further - # gated to `event == 'push'` above — and executes no repository code - # at all: it only reads the text files named above. So hardening buys - # nothing here while risking a release path that is only exercised at - # a version cut. - # - # If a future sweep wants uniformity, first replace the `ls-remote` - # with `gh api repos/$GITHUB_REPOSITORY/git/ref/tags/$tag` (which uses - # GH_TOKEN and is unaffected by credential persistence), THEN drop the - # credentials. That swap also fixes a latent bug: the `>/dev/null 2>&1` - # below makes a transient network/auth failure indistinguishable from - # "tag does not exist", so a blip sends the workflow down the - # should_release=true path for a version that is already tagged. - # `gh release create` then either fails outright (if a release already - # exists for that tag) or publishes a release against the pre-existing - # tag — noisy or wrong, but not silently duplicated. Fail-closed - # handling belongs with that change, not here. + # Completes the #318 sweep — this was the last checkout in the repo + # still persisting credentials, held back only because the tag check + # used `git ls-remote origin`. That check is now `gh api` (see the + # step below), which authenticates with GH_TOKEN and needs nothing + # from `.git/config`, so the exception no longer has a reason to + # exist and all 19 checkouts are uniform. + persist-credentials: false - name: Decide whether a new version needs releasing id: decide shell: bash + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | set -euo pipefail # Workspace version from [workspace.package] in Cargo.toml. @@ -113,13 +96,46 @@ jobs: tag="v${version}" echo "tag=${tag}" >> "$GITHUB_OUTPUT" echo "version=${version}" >> "$GITHUB_OUTPUT" - if git ls-remote --exit-code --tags origin "refs/tags/${tag}" >/dev/null 2>&1; then - echo "Tag ${tag} already exists - nothing to release." - echo "should_release=false" >> "$GITHUB_OUTPUT" - else - echo "Version ${version} has no ${tag} tag yet - will release." - echo "should_release=true" >> "$GITHUB_OUTPUT" - fi + + # Tag-existence check, FAIL-CLOSED. The previous implementation was + # git ls-remote --exit-code --tags origin "refs/tags/$tag" >/dev/null 2>&1 + # which collapsed three distinct outcomes into two: tag present, tag + # absent, and *lookup failed* all became a simple true/false, with any + # non-zero exit read as "absent". A transient network or auth blip + # therefore pushed an already-released version down the + # should_release=true path. This decides the entire release, so + # guessing is the one thing it must not do. + # + # `git/matching-refs` is used rather than `git/ref/tags/$tag` because + # it answers "absent" with HTTP 200 and an empty array instead of a + # 404 — so a genuine miss never looks like an error, and no error-body + # parsing is needed to tell them apart. It matches by PREFIX, so + # `tags/v2.2.1` would also return `v2.2.10`; the jq filter compares the + # full ref for exactness. + # + # Everything here fails closed under `set -euo pipefail`: a gh/API + # failure, malformed JSON, or an unexpected count aborts the job + # rather than resolving to a release decision. It also needs no Git + # credentials, which is what let the checkout above join the rest of + # the #318 sweep. + refs_json="$(gh api "repos/${GITHUB_REPOSITORY}/git/matching-refs/tags/${tag}")" + count="$(printf '%s\n' "$refs_json" \ + | jq --arg r "refs/tags/${tag}" '[.[] | select(.ref == $r)] | length')" + case "$count" in + 0) + echo "Version ${version} has no ${tag} tag yet - will release." + echo "should_release=true" >> "$GITHUB_OUTPUT" + ;; + 1) + echo "Tag ${tag} already exists - nothing to release." + echo "should_release=false" >> "$GITHUB_OUTPUT" + ;; + *) + echo "::error::Unexpected match count (${count}) for refs/tags/${tag} - refusing to guess." + printf '%s\n' "$refs_json" >&2 + exit 1 + ;; + esac - name: Resolve release notes + title if: steps.decide.outputs.should_release == 'true' diff --git a/AGENTS.md b/AGENTS.md index b0359fa7..f63682df 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -189,6 +189,10 @@ These cross-cutting decisions span multiple files. Reading individual chip docs - **GitHub Wiki Initialization:** When assisting with GitHub Wiki deployments for the first time, instruct the user to click "Create the first page" in the GitHub UI to provision the `.wiki.git` repository. If the Wiki is cloned locally inside the main repository, ensure its folder (e.g., `RustyNES.wiki/`) is added to `.gitignore`. - **Symlinked Agent Configs:** Ensure symlinked agent files (like `GEMINI.md` -> `AGENTS.md`) are explicitly removed from `.gitignore` so they are correctly tracked by version control. - **Libretro buildbot CI (issue #311) shipped in PR #312 (2026-07-19/20, `b49dd1e0`) — issue #311 stays OPEN by design.** `.gitlab-ci.yml` + a `[lib] name = "rustynes"` naming-collision fix + `[workspace] default-members` + RA memory-maps + an FDS load-path fix/disk-control + native Game Genie cheats all shipped. TAS and Netplay needed no new libretro-side wiring — RetroArch's own rollback/movie machinery already rides the existing `on_serialize`/`on_unserialize` hooks. Companion upstream PRs `libretro/docs#1164` and `libretro/libretro-super#2021` are open but not yet merged by the libretro team; the remaining step (mirroring the repo onto `git.libretro.com` + enabling the buildbot) is entirely on their side. **Never close #311** with a "Closes #311" commit-body keyword — it already auto-closed prematurely once this way and had to be reopened; only close it once the upstream team confirms the buildbot is live. +- **GENERAL RULE (learned twice, the hard way): never put a GitHub magic closing keyword — `Closes #N` / `Fixes #N` / `Resolves #N` — in a commit or PR body when the issue tracks ANY work the change does not finish.** GitHub closes the issue the instant the PR merges, marks it `stateReason: COMPLETED`, and the tracker then reports finished work that was never done. This has now fired twice: #311 (PR #312), and again on #318, which auto-closed one second after PR #319 merged (`02:42:15Z` merge → `02:42:16Z` close) even though #319's own body and review replies explicitly deferred two of its items. Write "addresses #N" / "partially addresses #N" in prose instead, and close the issue by hand only once its last item actually lands. When an issue *looks* completed, verify against the tree (`git log`, grep the code) rather than trusting the CLOSED/COMPLETED state — a keyword-closed issue is indistinguishable from a genuinely finished one in the UI. +- **CI security hardening (PRs #319 + #320, 2026-07-21, merged `85ee20db` / `a69200ef`).** `persist-credentials: false` on **all 19** `actions/checkout` sites (18 in #319; the last one, `release-auto.yml`, once its tag check stopped needing Git credentials — see the next bullet), because build scripts / proc macros / test binaries / Gradle scripts / MkDocs all execute unreviewed PR code that could read the token out of `.git/config`. Facts worth not re-deriving: `.github/actions/rust-setup` performs **no checkout of its own** (so call-site hardening is complete coverage); `persist-credentials` does **not** affect the `gh` CLI or API calls, only git network ops using the stored credential — which is why `gh release create`, `softprops/action-gh-release`, and `fastlane match` (a *different* repo, own `MATCH_GIT_*` secrets) all look like they need it and don't; and the highest-exposure job is **`web.yml`'s `build`**, not any `ci.yml` job, because `web.yml` declares `pages: write` + `id-token: write` at *workflow* level. There are now **no exceptions**: `release-auto.yml`'s `prepare` was the last holdout (it needed `git ls-remote origin` for the tag check), and that check is now a `gh api` call, so its checkout joined the sweep. +- **The release tag-existence check is FAIL-CLOSED — keep it that way.** `release-auto.yml`'s `decide` step queries `gh api repos/$GITHUB_REPOSITORY/git/matching-refs/tags/`, NOT `git/ref/tags/`: `matching-refs` answers "absent" with HTTP 200 + an empty array, so a genuine miss can never be confused with a lookup failure and no error-body parsing is needed. It matches by **prefix**, so the exact ref is compared in `jq` — this is load-bearing, not defensive: `v2.2` prefix-matches two real tags (`v2.2.0`, `v2.2.1`) while exact-matching none. Under `shell: bash` + `set -euo pipefail` both a `gh` failure and a non-array body abort the job (verified: exit 1 and exit 5 respectively). The old `git ls-remote ... >/dev/null 2>&1` read *any* non-zero exit as "tag absent", so a blip would try to re-release a shipped version. **Never reintroduce a form where a failed lookup is indistinguishable from "absent."** (Note when testing shell behavior locally: this harness's shell is zsh, whose `set -e` semantics for `var="$(cmd)"` differ from bash's — test with `bash -c` or you will get a false result.) +- **`pre-commit run --all-files` REWRITES vendored/immutable trees — use `--files ` or a single named hook instead.** `trailing-whitespace` / `end-of-file-fixer` / `mixed-line-ending` *modify* files and, before PR #320, had no `exclude` at all: one `--all-files` run silently reformatted **41 files** across the vendored TriCNES C#, vendored rcheevos C, `ref-docs/`, an upstream font licence, and upstream test-ROM READMEs — destroying exactly the byte-identical-to-upstream property those trees exist for. `.markdownlintignore` covered them for markdownlint only. #320 added a shared `exclude` anchor across the three rewriting hooks, scoped deliberately **narrower** than `.markdownlintignore`: only content we did not author. Frozen-but-ours trees (`docs/archive/`, `to-dos/plans/`, `docs/monetization/`) stay in scope, since the invariant is "don't rewrite what we didn't write". If it happens anyway, revert **only** the unintended paths (never a blanket `git checkout`, and never including your own edits). - **The libretro buildbot is a THIRD CI system with its own rules — and the pinned toolchain fights it.** The first real run (pipeline #91899, 2026-07-20) passed 1 of 10 jobs. `rust-toolchain.toml`'s `channel = "1.96.0"` makes rustup install a *fresh* toolchain inside libretro's build image, bypassing the image's pre-provisioned cross targets, so 8 jobs died on `E0463: can't find crate for core`; each job in `.gitlab-ci.yml` now runs `rustup target add ${RUST_TARGET}` (NOT added to `rust-toolchain.toml`'s `targets` — that would cost every contributor and GH Actions job ~8 extra `rust-std` downloads). The Apple jobs must use `!reference` rather than `extends` for that, because GitLab's `extends` REPLACES array keys and would silently drop the templates' `SDKROOT`/`STRIP`/`CC`/`CXX` exports. tvOS is special twice over: its template hardcodes `cargo +nightly build -Zbuild-std`, which bypasses our channel pin onto the image's stale nightly (below our MSRV) *and* omits `panic_abort` that `[profile.release] panic = "abort"` needs — handled by refreshing nightly + `CARGO_PROFILE_RELEASE_PANIC=unwind` in that job (`CARGO_UNSTABLE_BUILD_STD` does NOT work: the CLI `-Z` flag wins). **A green GitHub Actions run does not imply a green buildbot** — the new `libretro-cross` CI job (one triple for each buildbot ABI family a Linux runner can model — MinGW-Windows and Android/NDK; the Apple families are deliberately excluded, as bindgen needs a real per-target sysroot and there is no Apple SDK on a Linux runner) is the early-warning gate; before touching anything libretro-related, cross-check `cargo check --release -p rustynes-libretro --target ` locally. - **`rust-libretro 0.3.2` is unmaintained (no commit since 2023-02) and has a MinGW bug we work around.** It casts a keycode with `cfg(target_family = "windows")`, but C enum signedness follows the *ABI*: only **MSVC** gives plain enums `int` — under **MinGW** (`x86_64-pc-windows-gnu`, what the buildbot builds) bindgen emits `c_uint` and the crate fails `E0308`. `.cargo/config.toml`'s `[env] BINDGEN_EXTRA_CLANG_ARGS_x86_64_pc_windows_gnu = "--target=x86_64-pc-windows-msvc"` fixes it; the generated-bindings diff is 28 lines, all enum signedness. Don't "clean up" that env var without rebuilding for `x86_64-pc-windows-gnu`. - **CodeRabbit is now a 3rd automated PR review bot** (`.coderabbit.yaml`, added 2026-07-20 in PR #316), alongside gemini-code-assist and copilot-pull-request-reviewer — same reply-and-resolve-every-thread ceremony applies before any merge. Configured `profile: assertive` (not the "chill" default) and a `tools{}`/`path_instructions`/custom-checks set audited against this repo's actual file footprint, not guessed. `tone_instructions` has a hard 250-character schema limit that fails validation silently on the CodeRabbit side — after editing `.coderabbit.yaml`, verify with a `@coderabbitai configuration` PR comment and confirm every changed field shows `Source: Repository YAML (base)`. diff --git a/CHANGELOG.md b/CHANGELOG.md index e7f36d91..b3865d53 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,35 +38,53 @@ cycle-accurate core later replaced. ### Security -- **`persist-credentials: false` on every CI checkout that compiles or runs - repository code (closes #318).** `actions/checkout` defaults to writing the - workflow `GITHUB_TOKEN` into `.git/config`, where any code the job then - executes from the checkout — Cargo build scripts, proc macros, test - binaries, Gradle build scripts, `scripts/*.sh`, the MkDocs build — can read - it. On a pull request that tree is by definition unreviewed code, and nearly - every CI job compiles or runs it — the exceptions being the `audit` / `deny` - jobs, which install prebuilt binaries and only parse `Cargo.lock`. Applied to - **18 of the 19** checkouts in - `.github/`. Highest-exposure site was not a `ci.yml` job but `web.yml`'s - `build`, whose workflow-level `permissions:` grant `pages: write` + - `id-token: write` and which is PR-reachable while running `trunk`, - `cargo doc`, `pip install`, and `mkdocs build`. +- **`persist-credentials: false` on all 19 CI checkouts, plus a fail-closed + release-tag check and a pinned toolchain action (closes #318).** + `actions/checkout` defaults to writing the workflow `GITHUB_TOKEN` into + `.git/config`, where any code the job then executes from the checkout — + Cargo build scripts, proc macros, test binaries, Gradle build scripts, + `scripts/*.sh`, the MkDocs build — can read it. On a pull request that tree + is by definition unreviewed code, and nearly every CI job compiles or runs + it (the exceptions being the `audit` / `deny` jobs, which install prebuilt + binaries and only parse `Cargo.lock`). Audited rather than applied blanket: `.github/actions/rust-setup` performs no checkout of its own, so call-site hardening is complete coverage; and - **every hardened job was confirmed not to need the checkout credential** — - nothing pushes commits, tags, or branches, there are no submodules, GitHub - Pages uses the OIDC flow (not a `gh-pages` push), and `gh release create` / + **no job was found to need the checkout credential** — nothing pushes + commits, tags, or branches, there are no submodules, GitHub Pages uses the + OIDC flow (not a `gh-pages` push), and `gh release create` / `softprops/action-gh-release` authenticate by API token while `fastlane match` clones a different remote with its own `MATCH_GIT_*` - credentials. The single deliberate - exception is `release-auto.yml`'s `prepare`, the one job whose work needs - that credential (`git ls-remote --tags origin`); it is - unreachable from untrusted input and executes no repository code, so - hardening would buy nothing while risking a release path exercised only at a - version cut. Its checkout now carries an inline comment recording that, plus - the safe migration order for anyone wanting uniformity later. Purely - additive CI configuration — no source, build output, or emulation behavior - changes. + credentials. Highest-exposure site was not a `ci.yml` job but `web.yml`'s + `build`, whose workflow-level `permissions:` grant `pages: write` + + `id-token: write` and which is PR-reachable while running `trunk`, + `cargo doc`, `pip install`, and `mkdocs build`. + Two related hardening items landed alongside it: + - `release-auto.yml`'s tag-existence check was + `git ls-remote --exit-code --tags origin "refs/tags/$tag" >/dev/null 2>&1`, + which collapsed *tag present*, *tag absent*, and *lookup failed* into a + two-way answer, reading any non-zero exit as "absent" — so a transient + network or auth blip pushed an already-released version down the + `should_release=true` path. It is now a `gh api` call against + `git/matching-refs/tags/`, chosen over `git/ref/tags/` because it + answers "absent" with HTTP 200 and an empty array rather than a 404, 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, not theoretical: `v2.2` prefix-matches two real + tags while exact-matching none. Every failure path now aborts the job under + `set -euo pipefail` instead of resolving to a release decision. Removing + the last Git operation is also what let that job's checkout — the only one + that had needed the credential — join the sweep. + - `.github/actions/rust-setup` pinned `dtolnay/rust-toolchain` from `@master` + to commit `e97e2d8c` (`# v1`). `@master` is a **branch** that advances on + every upstream commit, unlike the `@vN` tags used everywhere else, and this + composite feeds 12 of the 19 checkouts — including `release.yml` + (`contents: write`) and `web.yml` (`pages: write` + `id-token: write`) — + while being the action that installs the compiler. The `# v1` trailing + comment is what Dependabot's already-enabled `github-actions` ecosystem + reads to keep the pin current, so this does not trade a supply-chain risk + for a stale-action one. + Purely additive CI configuration — no source, build output, or emulation + behavior changes. ### Changed