From 5c8718c2d5f26d17446dcc034529904ec1dd284a Mon Sep 17 00:00:00 2001 From: "Michael C. Ferguson" Date: Sat, 18 Jul 2026 22:29:38 -0500 Subject: [PATCH 1/2] feat(ci): add a manual workflow_dispatch trigger to the hash sync MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Without this there is NO way to re-drive renovate-hash-sync.yml after fixing a bug in it. Three things block every other route, and all three held at once on run 29669946883, which executed the same broken kernel-URL code three times across two separate fixes: * pushing the fix to the branch triggers nothing — the paths: filter does not list the workflow file itself; * a maintainer merging the fix in is skipped by the actor == renovate[bot] gate; * and a re-run replays the workflow definition from the commit the ORIGINAL run was created against, so it faithfully re-executes the old code. Pulling attempt 3's log confirmed it ran the pre-#42 unanchored grep. Only Renovate moving the branch head, or a manual trigger, gets new code to run. The `branch` input is required and is NOT the ref you dispatch from: GitHub runs the workflow definition from the dispatch ref while this workflow checks out and pushes to inputs.branch. Dispatch from the default branch (fixed workflow), name the broken branch in the input. Dispatching *on* the stale branch would just re-run its stale copy — the exact trap this fixes. Manual runs refuse to target the default branch: this workflow commits and pushes, and an auto-generated hash commit must go through a PR. workflow_dispatch is already restricted to collaborators with write access, so the job gate does not widen who can drive this. Also resolves the pre-existing actionlint finding it was adjacent to: the push step interpolated github.event.pull_request.head.ref straight into the script body, a command-injection vector via an attacker-chosen branch name on a PR from an untrusted contributor. Both use sites now go through a job-level TARGET_BRANCH env var, and the push uses `--` so a branch named like an option is not parsed as one. actionlint's untrusted-expression warning is gone. Corrected two stale header claims while in here: the workflow HAS now run against a real Renovate PR (#41, 6.18.38 -> 6.18.39), and its warn-and-skip paths can report SUCCESS while doing nothing — which is exactly how the stale linux.hash went unnoticed. The github-package loop and the lzma-sdk and sdcard-payload steps remain unproven. actionlint and shellcheck clean. Guard logic tested: a renovate branch passes; the default branch and an empty input are both rejected. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01UFdgJXQKvrucXeKDami499 --- .github/workflows/renovate-hash-sync.yml | 133 +++++++++++++++++++---- docs/renovate.md | 36 +++++- 2 files changed, 143 insertions(+), 26 deletions(-) diff --git a/.github/workflows/renovate-hash-sync.yml b/.github/workflows/renovate-hash-sync.yml index 3658ccf..f8a3067 100644 --- a/.github/workflows/renovate-hash-sync.yml +++ b/.github/workflows/renovate-hash-sync.yml @@ -70,22 +70,44 @@ # the first two; xow-firmware pins opaque Microsoft Update .cab GUIDs, # not a version). See docs/renovate.md. # -# TRIGGER: only for PRs opened by Renovate itself (github.actor check below), -# only on branches in THIS repo (never a fork -- pushing back requires -# write access the default token does not get on a fork-originated PR), and -# only when a file this workflow actually knows how to fix has changed. -# Idempotent by design: it always recomputes from the PR's current file -# content and only commits if the recomputed hash differs from what's -# already there, so re-runs (e.g. a second Renovate push to the same PR) -# are harmless no-ops once the hash is already correct. +# TRIGGER: automatically, only for PRs opened by Renovate itself (github.actor +# check below), only on branches in THIS repo (never a fork -- pushing back +# requires write access the default token does not get on a fork-originated +# PR), and only when a file this workflow actually knows how to fix has +# changed. Idempotent by design: it always recomputes from the branch's current +# file content and only commits if the recomputed hash differs from what's +# already there, so re-runs (e.g. a second Renovate push to the same PR) are +# harmless no-ops once the hash is already correct. # -# UNVERIFIED: like renovate.json itself, this workflow has never been -# exercised against a real Renovate PR (Renovate is installed and live -# since 2026-07-14, but no custom manager has produced a real PR yet -- -# see docs/renovate.md's "Unverified / what to check on first run"). -# Treat the exact regexes/URLs below -- including the libchdr loop entry -# and the bespoke lzma-sdk and sdcard-payload steps -- as reviewed-by-hand, -# not proven. +# ...and manually, via workflow_dispatch with a `branch` input. That escape +# hatch exists because none of the automatic machinery can re-drive this +# workflow after a bug in it is fixed: +# +# * the paths: filter above does not list this file, so pushing a fix to it +# triggers nothing; +# * the actor gate skips the job when a human pushes the fix to the branch; +# * and a re-run replays the workflow definition from the commit the original +# run was created against, so it re-executes the OLD code. +# +# All three held simultaneously on run 29669946883, which ran the same broken +# kernel-URL code three times across two separate fixes. When dispatching, +# note that the ref you dispatch FROM (which supplies this file) and the +# `branch` you dispatch AT (which gets checked out and pushed to) are +# deliberately different things. +# +# PARTIALLY VERIFIED (updated 2026-07-19): the kernel step HAS now been +# exercised against a real Renovate PR (#41, kernel 6.18.38 -> 6.18.39) and +# works end to end. That first run also found two bugs this header used to +# understate: an unanchored defconfig grep that built a URL containing a +# newline (#42), and the fact that a fetch failure here is only a ::warning:: +# -- so the job reported SUCCESS three times while silently leaving +# linux.hash stale. Read every warn-and-skip path below as "this can go green +# without doing anything". +# +# Still UNPROVEN: the generic github-package loop (including the libchdr +# entry) and the bespoke lzma-sdk and sdcard-payload steps have never run +# against a real PR. Treat those regexes/URLs as reviewed-by-hand, not +# proven -- see docs/renovate.md's "Unverified / what to check on first run". # ################################################################################ @@ -111,8 +133,31 @@ on: - "package/lzma-sdk/lzma-sdk.mk" - "scripts/fetch-sdcard-payload.sh" + # Manual escape hatch. Needed because NOTHING else can re-drive this workflow + # after a fix lands: the pull_request trigger's `paths:` filter does not list + # this file (so pushing a fix to it triggers nothing), the job is gated on + # actor == renovate[bot] (so a maintainer's own push is skipped), and a + # re-run replays the workflow definition from the commit the original run was + # created against -- which is how run 29669946883 executed the same broken + # code three times across two fixes. + # + # IMPORTANT: dispatch from a branch whose copy of THIS FILE is the one you + # want to run (normally the default branch), and name the branch you want + # FIXED in the `branch` input. Those are two different things, and conflating + # them is the trap above: GitHub runs the workflow definition from the ref you + # dispatch on, while this workflow edits and pushes to `inputs.branch`. + workflow_dispatch: + inputs: + branch: + description: "Branch to refresh companion hashes on (e.g. renovate/kernel-longterm-6.18-6.x). NOT the branch you are dispatching from." + required: true + type: string + concurrency: - group: renovate-hash-sync-${{ github.event.pull_request.number }} + # pull_request runs key on the PR number; dispatch runs key on the target + # branch. Without the fallback every manual run would share one empty-suffix + # group and cancel the previous one. + group: renovate-hash-sync-${{ github.event.pull_request.number || inputs.branch }} cancel-in-progress: true permissions: @@ -120,24 +165,59 @@ permissions: jobs: hash-sync: - name: Refresh companion hashes for this PR's version bump(s) + name: Refresh companion hashes for this branch's version bump(s) runs-on: ubuntu-26.04 - # Only Renovate's own PRs, and never a fork (pushing back needs write - # access to the head branch, which a fork PR's default token never has). + # Automatic runs: only Renovate's own PRs, and never a fork (pushing back + # needs write access to the head branch, which a fork PR's default token + # never has). Manual runs: workflow_dispatch is already restricted to + # collaborators with write access, which is the same bar this gate enforces + # for the automatic path -- so it does not widen who can drive this. if: > - github.actor == 'renovate[bot]' && - github.event.pull_request.head.repo.full_name == github.repository + github.event_name == 'workflow_dispatch' || + (github.actor == 'renovate[bot]' && + github.event.pull_request.head.repo.full_name == github.repository) timeout-minutes: 10 + # Resolved once, here, rather than repeated at each use site. Passing it to + # the push step as a shell variable (not an inline ${{ }} expansion) also + # closes the script-injection vector actionlint flags for + # github.event.pull_request.head.ref -- a branch name is attacker-chosen + # text on a PR from an untrusted contributor. + env: + TARGET_BRANCH: ${{ github.event_name == 'workflow_dispatch' && inputs.branch || github.event.pull_request.head.ref }} + steps: + # A manual run pushes commits to whatever branch it is pointed at, so + # refuse the default branch outright: that would land an auto-generated + # hash commit on master with no review. The automatic path cannot hit + # this (a PR head branch is never the base), but a mistyped input can. + - name: Refuse to auto-commit to the default branch + if: github.event_name == 'workflow_dispatch' + env: + DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} + run: | + set -euo pipefail + if [ -z "$TARGET_BRANCH" ]; then + echo "::error::branch input resolved to empty -- nothing to check out." >&2 + exit 1 + fi + if [ "$TARGET_BRANCH" = "$DEFAULT_BRANCH" ]; then + echo "::error::refusing to run against the default branch ('$TARGET_BRANCH') -- this workflow commits and pushes hash fixes, which must go through a PR." >&2 + exit 1 + fi + echo "target branch: $TARGET_BRANCH" + # Same pin as every other workflow in this repo (build.yml et al.) -- # kept in sync so Renovate's own github-actions manager (renovate.json) # tracks exactly one actions/checkout dependency, not two drifting # copies. - - name: Checkout PR head branch + - name: Checkout the target branch uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 with: - ref: ${{ github.event.pull_request.head.ref }} + # The PR head branch, or the dispatch input -- NOT the ref this + # workflow definition came from. On a manual run those differ by + # design (see the workflow_dispatch note above). + ref: ${{ env.TARGET_BRANCH }} fetch-depth: 1 # --- 1. Github-sourced packages (drivers/firmware + libchdr) ----------- @@ -445,4 +525,9 @@ jobs: comment. That value must come from Buildroot's GPG-signed .sign manifest and is a manual step. COMMITMSG - git push origin "HEAD:${{ github.event.pull_request.head.ref }}" + # Shell variable, NOT a workflow-expression interpolation: a branch + # name is attacker-controlled text on a PR from an untrusted + # contributor, and expanding it into the script body before bash sees + # it is a command-injection vector (actionlint flags exactly this). + # "--" stops a branch named like an option from being parsed as one. + git push origin -- "HEAD:$TARGET_BRANCH" diff --git a/docs/renovate.md b/docs/renovate.md index 4a31443..981ad88 100644 --- a/docs/renovate.md +++ b/docs/renovate.md @@ -108,8 +108,40 @@ the one case where it deliberately does nothing. **Triggers on**: `pull_request` (opened/synchronize), only when `github.actor == 'renovate[bot]'` and the PR's head repo is this repo (never a fork — a fork PR's default token cannot push back to it anyway). It always -recomputes from the PR's current file content and only commits if the result -actually differs, so re-runs on an already-correct PR are harmless no-ops. +recomputes from the branch's current file content and only commits if the +result actually differs, so re-runs on an already-correct PR are harmless +no-ops. + +**...and manually**, via `workflow_dispatch` with a required `branch` input. + +This escape hatch is not a convenience — without it there is **no way at all** +to re-drive this workflow after fixing a bug in it. Three things block every +other route, and all three held at once on +[run 29669946883](https://github.com/mcfbytes/Buildroot_MiSTer/actions/runs/29669946883), +which executed the same broken kernel-URL code three times across two separate +fixes: + +| route | why it fails | +|---|---| +| push the fix to the branch | the `paths:` filter doesn't list the workflow file, so nothing triggers | +| a maintainer merges the fix in | the `renovate[bot]` actor gate skips the job | +| re-run the failed run | a re-run replays the workflow definition from the commit the *original* run was created against — so it re-executes the old code | + +Only Renovate itself moving the branch head, or this manual trigger, gets new +code to run. + +> **The ref you dispatch *from* and the branch you dispatch *at* are different +> things, deliberately.** GitHub runs the workflow definition from the ref you +> dispatch on, while this workflow checks out and pushes to `inputs.branch`. +> So dispatch **from the default branch** (which has the fixed workflow) and +> put the branch you want *repaired* — e.g. +> `renovate/kernel-longterm-6.18-6.x` — in the input. Dispatching *on* the +> stale branch would just run its stale copy again, which is the whole trap. + +Manual runs refuse to target the default branch: this workflow commits and +pushes, and an auto-generated hash commit must go through a PR. `workflow_dispatch` +is already restricted to collaborators with write access, so this does not +widen who can drive the workflow. **What it fixes automatically, and why each case is safe:** From 024699127be954d7d887f15d0d27a2c613d6ed4b Mon Sep 17 00:00:00 2001 From: "Michael C. Ferguson" Date: Sat, 18 Jul 2026 23:12:13 -0500 Subject: [PATCH 2/2] fix(ci): close a ref-spelling bypass of the default-branch guard From Copilot's review on #45. The guard compared $TARGET_BRANCH to the default branch as a plain string, so dispatching `refs/heads/master` slipped past it while actions/checkout and `git push` still resolved it to master. Verified with `git push --dry-run`, and the hole is wider than reported -- THREE spellings all reach master, two of which bypassed the check: master -> master (caught) refs/heads/master -> master (bypassed) heads/master -> master (bypassed; not in the review) So normalising away a `refs/heads/` prefix, as suggested, would still have left `heads/master` open. Rejecting all ref-namespace forms instead of normalising them, deliberately: TARGET_BRANCH is consumed by BOTH actions/checkout and the final git push, so the value validated here must be the exact value those steps use. Normalising would mean re-exporting through $GITHUB_ENV and depending on it overriding the job-level env: for subsequent steps -- a precedence subtlety not worth betting a push to the default branch on. Also added `git check-ref-format --branch` as a syntactic gate (rejects embedded spaces, "..", a leading "-", empty), and dropped the event_name == workflow_dispatch condition so the automatic path is validated too. That costs nothing and means a PR head branch is checked as well. Verified, 12 cases: legitimate names (renovate/..., feature/x) pass; master, refs/heads/master, heads/master, remotes/..., tags/..., -x, "a..b", "x y" and empty are all rejected, each with the message that explains which rule fired. shellcheck clean; actionlint clean apart from its stale runner-label list. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01UFdgJXQKvrucXeKDami499 --- .github/workflows/renovate-hash-sync.yml | 41 +++++++++++++++++++++--- docs/renovate.md | 10 ++++++ 2 files changed, 47 insertions(+), 4 deletions(-) diff --git a/.github/workflows/renovate-hash-sync.yml b/.github/workflows/renovate-hash-sync.yml index f8a3067..0208c19 100644 --- a/.github/workflows/renovate-hash-sync.yml +++ b/.github/workflows/renovate-hash-sync.yml @@ -149,7 +149,7 @@ on: workflow_dispatch: inputs: branch: - description: "Branch to refresh companion hashes on (e.g. renovate/kernel-longterm-6.18-6.x). NOT the branch you are dispatching from." + description: "Plain branch name to refresh companion hashes on (e.g. renovate/kernel-longterm-6.18-6.x). NOT the branch you are dispatching from, and NOT a ref -- refs/heads/x and heads/x are rejected." required: true type: string @@ -191,20 +191,53 @@ jobs: # refuse the default branch outright: that would land an auto-generated # hash commit on master with no review. The automatic path cannot hit # this (a PR head branch is never the base), but a mistyped input can. - - name: Refuse to auto-commit to the default branch - if: github.event_name == 'workflow_dispatch' + - name: Validate the target branch, and refuse the default branch env: DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} run: | set -euo pipefail + if [ -z "$TARGET_BRANCH" ]; then - echo "::error::branch input resolved to empty -- nothing to check out." >&2 + echo "::error::target branch resolved to empty -- nothing to check out." >&2 exit 1 fi + + # Syntactic validation first: rejects empty, embedded spaces, "..", + # a leading "-", and the other things git considers malformed. Runs + # before checkout, which is fine -- check-ref-format needs no repo. + if ! git check-ref-format --branch "$TARGET_BRANCH" >/dev/null 2>&1; then + echo "::error::'$TARGET_BRANCH' is not a valid git branch name." >&2 + exit 1 + fi + + # REJECT ref-namespace forms rather than normalising them away. git + # resolves several spellings to the same branch, so a plain string + # comparison against the default branch is only sound once these are + # excluded. Verified with `git push --dry-run`, all three reach master: + # + # master -> master (caught by the compare below) + # refs/heads/master -> master (bypasses a naive compare) + # heads/master -> master (bypasses it too) + # + # Rejecting is deliberate, not laziness: TARGET_BRANCH is consumed by + # BOTH actions/checkout and the final `git push`, so the value that + # gets validated here must be the exact value those steps use. + # Normalising would mean re-exporting through $GITHUB_ENV and relying + # on it overriding the job-level env: for later steps -- a subtlety + # not worth betting a force-push to the default branch on. + case "$TARGET_BRANCH" in + refs/*|heads/*|remotes/*|tags/*) + echo "::error::pass a plain branch name, not a ref: '$TARGET_BRANCH'." >&2 + echo "::error::git resolves refs/heads/ and heads/ to the same branch as , which would slip past the default-branch check below." >&2 + exit 1 + ;; + esac + if [ "$TARGET_BRANCH" = "$DEFAULT_BRANCH" ]; then echo "::error::refusing to run against the default branch ('$TARGET_BRANCH') -- this workflow commits and pushes hash fixes, which must go through a PR." >&2 exit 1 fi + echo "target branch: $TARGET_BRANCH" # Same pin as every other workflow in this repo (build.yml et al.) -- diff --git a/docs/renovate.md b/docs/renovate.md index 981ad88..a85b4aa 100644 --- a/docs/renovate.md +++ b/docs/renovate.md @@ -143,6 +143,16 @@ pushes, and an auto-generated hash commit must go through a PR. `workflow_dispat is already restricted to collaborators with write access, so this does not widen who can drive the workflow. +The input must be a **plain branch name**, not a ref. git resolves several +spellings to the same branch — `master`, `refs/heads/master` and `heads/master` +all push to `master` (verified with `git push --dry-run`) — so a naive string +comparison against the default branch is only sound once the ref forms are +excluded. They are rejected outright rather than normalised, because the same +value is consumed by both `actions/checkout` and the final `git push`: what gets +validated has to be exactly what those steps use. The name is also run through +`git check-ref-format --branch`, which rejects embedded spaces, `..`, a leading +`-`, and the rest. + **What it fixes automatically, and why each case is safe:** 1. **The 12 github-archive `.hash` files** (the 9 commit pins + munt +