fix(ci): remove GHCR-write/OIDC credentials from docker.yml's pull_request path

[benmitchell11]

Summary

Splits docker.yml's build job into two: a permission-free build-pr job for pull_request events, and the original credentialed build job restricted (if: github.event_name != 'pull_request') to push/tag/workflow_dispatch. A pull_request run — same-repo or fork — now never holds packages:write/id-token:write/attestations:write or a GHCR login by default, closing the concrete gap #1454 describes.

Related issue

Closes #1454

Issue type

Bug

---

Agent provenance

Field	Value
Harness / provider	Claude Code
Model	claude-sonnet-5
Session reference	N/A - not exposed by this harness
Initiating human	@benmitchell11

Objective

docker.yml's pull_request-triggered execution path holds no GHCR-write or OIDC-attestation credentials, regardless of what the triggering PR's own diff to the file says about push flags.

Impacted components

.github/workflows/docker.yml

Approach and rejected alternatives

The bug as filed: build's per-job permissions block grants packages:write/id-token:write/attestations:write unconditionally on every trigger, and the "Log in to GHCR" step already authenticates for same-repo PRs (its if only excludes forks). The single remaining gate before an actual publish was the build step's own push=${{ github.event_name != 'pull_request' }} output flag — a boolean living inside the same file a same-repo PR's diff controls.

Rejected: leave one job, add a stronger if: condition around the push step. Considered gating the push/login steps more tightly with a bigger conditional expression. Rejected because it doesn't change the actual security property — a same-repo PR can edit an if: condition exactly as easily as it can edit the push= flag it's already able to edit today. The property that actually matters is not "how many characters does the exploit require," it's "does the job that executes on pull_request hold live credentials at all, by default, without the PR touching anything." A conditional inside a credentialed job can never guarantee that; only removing the credentials from that job's permissions block can.

Rejected: gate via GitHub Environment with required reviewers. This is the structurally sound fix — an environment's required-reviewer rule lives in repository settings, not the workflow file, so a PR's own diff can't bypass it even if it adds a whole new job. Rejected for this PR only because creating a protected Environment needs Maintain/Admin role, which I don't have (same constraint as #1420). Filed as #1456 instead of blocking this PR on access I don't have.

Chosen: split into two jobs, build-pr (pull_request, contents: read only) and build (everything else, unchanged permissions, now gated if: github.event_name != 'pull_request'). This is the largest fix deliverable without Environment access: it removes the live credentials from the default, unedited PR-execution path entirely, rather than relying on a same-job conditional. It does not defend against a same-repo PR that deliberately adds a new job or a new trigger to redeclare those permissions from scratch — nothing expressed purely in this file can defend against that, which is exactly why #1456 exists.

Kept build-pr's job name: identical to build's (Build (${{ matrix.platform }})) rather than a distinguishing name, deliberately — the two are mutually exclusive per run, and matching names means whichever job runs reports under the same status-check name branch protection may already require. A differently-named PR-only job risked permanently blocking merges if "Build (linux/amd64)"/"Build (linux/arm64)" turn out to be required checks (unconfirmed — see #1420 on why the protection config isn't visible via API here). Kept build-pr without a GHCR login step at all, so cache-from is an anonymous registry read; if the buildcache package isn't public this is a cache miss (cold build), not a failure, per buildx's documented behavior for an unreachable cache-from source. Traded away: same-repo PRs lose the registry-warmed cache build still gets on real releases, in exchange for never holding a push-capable token during a PR run.

Verification

Command run:

python3 -c "import yaml; yaml.safe_load(open('.github/workflows/docker.yml')); print('YAML valid')"

Raw output:

YAML valid

Command run (confirming the exact permission/login lines the bug report cited, to verify the split actually removed them from the PR-triggered job):

grep -n "packages: write\|id-token: write\|attestations: write\|Log in to GHCR\|^  build" .github/workflows/docker.yml

Raw output:

93:  build-pr:
159:  build:
170:      packages: write    # push to GHCR
171:      id-token: write    # OIDC for build provenance attestation
172:      attestations: write
209:      - name: Log in to GHCR
321:      packages: write    # push the merged manifest
322:      id-token: write    # OIDC for provenance attestation on the manifest
323:      attestations: write
344:      - name: Log in to GHCR
440:      packages: write
470:      - name: Log in to GHCR
519:      packages: write
520:      id-token: write
521:      attestations: write
531:      - name: Log in to GHCR

build-pr (line 93 on, ends before build at line 159): only contents: read in its permissions block, no login step anywhere in it. build (line 159 on): permissions and login unchanged from before this PR, but the job now carries if: github.event_name != 'pull_request' so it never executes for a pull_request event. The remaining hits (321-344, 440-531) are merge, push-gateway-build, and push-gateway-merge — untouched by this PR; merge already carried if: github.event_name != 'pull_request' before this change, and the push-gateway-* jobs already carry if: github.repository == 'block/buzz', which is never true in this fork.

Command run (credential sweep):

grep -niE "(-----BEGIN|password\s*=|secret\s*=|api[_-]?key|token\s*=|AKIA[0-9A-Z]{16}|ghp_[A-Za-z0-9]{20,}|xox[baprs]-)" .github/workflows/docker.yml

Raw output: (no matches, exit code 1)

• Tests or checks were run and the raw output is pasted above
• The diff is confined to the scope of the linked issue
• No secrets, keys, tokens or hostnames were added to tracked files

Not verified

The real push/tag publish path is unverified by this PR. I did not create a relay-v* tag or push to launchpad directly (both would trigger a real image publish), since that's a live production action outside a PR's own scope and not something I should do unilaterally. The build job's own steps are byte-for-byte unchanged from before this PR — only a job-level if: was added — so the risk of this PR silently breaking a real release is limited to that one line, but it is still unverified against a real push/tag event. This PR's own CI run (on pull_request, exercising the new build-pr job and the merge/push-gateway-* jobs' unaffected if: skips) is the evidence available before merge; a real release is the first true end-to-end test of the unchanged build path after this merges.

Whether build-pr's anonymous cache-from degrades to a cold build or errors — reasoned from buildx's documented behavior (missing/unreachable cache source is a cache miss, not a failure), not confirmed against this specific package's visibility (I don't have read:packages scope to check ghcr.io/launchpad-26/buzz-buildcache's visibility directly). This PR's own CI run is the first real test; if it fails here rather than gracefully degrading, that's a signal to revisit before merge.

Whether "Build (linux/amd64)"/"Build (linux/arm64)" are configured as required status checks — unconfirmed, same reason as #1420 (the protection/ruleset API returns empty for this token). Kept the job name: identical specifically to avoid this being load-bearing either way, but the actual branch-protection configuration remains something only admin access can confirm.

Security implications

Reduces docker.yml's attack surface for pull_request runs: no GHCR-write, OIDC-attestation, or authenticated-registry-session credential is granted to a pull_request-triggered job by default, closing the gap #1454 describes for the ordinary case. Does not close the deeper case of a same-repo PR that deliberately adds a new job/trigger to redeclare those permissions — that requires Environment-based protection external to this file, tracked as #1456. No change to the push/tag/workflow_dispatch path's own steps, permissions, or security posture — only which trigger reaches them.

Escalations

Whether this PR should say Closes #1454 given the residual gap #1456 describes — raising rather than deciding unilaterally, mirroring the #146/#1010/#1011 split earlier in this project. I judged the literal, concretely-described bug (an unedited PR already holding live publish credentials) is genuinely fixed, and the deeper, environment-gated closure is a distinct, admin-gated piece of work rather than an unfinished part of the same fix — same reasoning as why #1420 and #1011 are separate issues rather than blocking the PRs that depend on them. If a reviewer disagrees with that split, the fix is to reopen #1454 rather than revert this PR, since the change itself is still a real improvement either way.

[benmitchell11]

CI confirms both things flagged as unverified in this PR's own body:

build-pr's anonymous cache-from degrades gracefully, doesn't fail. The buildcache package is confirmed private — build-pr's log shows:
```
#12 importing cache manifest from ghcr.io/launchpad-26/buzz-buildcache:amd64
#12 ERROR: failed to configure registry cache importer: failed to authorize: failed to fetch anonymous token: unexpected status from GET request to https://ghcr.io/token?scope=repository%3Alaunchpad-26%2Fbuzz-buildcache%3Apull&service=ghcr.io: 401 Unauthorized
```
— logged as an error-level line, but non-fatal: the job proceeded to a full cold build of both the release and debug targets and completed successfully (Build (linux/amd64): pass, 15m32s; Build (linux/arm64): pass, 12m6s). Confirms the traded-away cache warm-start is real (same-repo PRs now always cold-build) but doesn't break anything.

Job naming didn't disrupt required-status-check matching. gh pr checks shows Build (linux/amd64)/Build (linux/arm64) passing under those exact names (from build-pr, matching build's name deliberately), and the PR's merge-box is mergeable: MERGEABLE, blocked only on reviewDecision: REVIEW_REQUIRED — no stuck/missing check.

Ready for review.

[serina-mcfall]

Review — comments, no blockers

This is a good change and I think it should go in. The security property it claims is the one it actually delivers, and I verified that independently rather than taking the body's word for it.

The shape is right for the right reason. Gating a credentialed job with a tighter if: would have left the credentials present and the guarantee conditional; splitting the job means a pull_request run never holds packages:write/id-token:write/attestations:write at all, so there is no condition left to defeat. The PR body argues this explicitly and the argument is correct.

Worth noting for anyone reading the diff cold: 84 additions, 0 deletions on a file that already exists is the signature of a stale merge base hiding deletions — I checked, and it isn't one here. git merge-base origin/launchpad 7c7d7b591 equals the launchpad tip exactly, 0 commits behind, and the true diff against live launchpad is also 84 insertions(+). The title says "remove" and the diff deletes nothing because the removal is achieved by routing, not deletion. That is legitimate.

What I verified myself

• No pull_request_target. Triggers are push (launchpad, relay-v[0-9]*), path-filtered pull_request, and workflow_dispatch. This was the hole I most expected — github.event_name != 'pull_request' would be true for pull_request_target, which runs with base-repo context. It isn't in the file, so the guard is airtight.
• Mutual exclusivity holds. build-pr on pull_request, build on everything else. No event runs both; no event runs neither.
• Validation parity is intact — build-pr builds runtime and runtime-debug, on both linux/amd64 and linux/arm64, same context and Dockerfile. Nothing silently stopped being validated on PRs.
• merge is consistently gated (needs: build, same event guard), so it skips cleanly on PRs rather than hanging on a skipped dependency.
• PRs cannot poison the shared cache. build-pr has cache-from only — no cache-to, no login, no registry exporter. build retains cache-to. This is a real hardening benefit the body doesn't even claim.
• ADR-0005's provenance property is preserved. IMAGE_NAME remains ghcr.io/launchpad-26/buzz and build-pr never pushes, so nothing changes about what this fork publishes or from where. (docker.yml is one of §3's five settled image-provenance exception files — raising its existence as a §3 violation would be wrong, and I'm not.)
• Supply chain: added uses: are full SHA pins carried over from the existing job, and persist-credentials: false is present on checkout.
• The duplicate name: trick works. Live on this PR: Build (linux/amd64) and Build (linux/arm64) both pass from build-pr, and the skipped build reports as Build (${{ matrix.platform }}) — skipped before matrix expansion, so it leaves neither required matrix check pending. The reasoning in the body about not introducing a check name a PR could never satisfy is sound.

High — the premise of follow-up #1456 does not hold as written

Not a defect in this diff, which is why it isn't blocking — but it will be wrong the moment #1456 is acted on.

The body says a protected Environment is "the structurally sound fix — an environment's required-reviewer rule lives in repository settings, not the workflow file, so a PR's own diff can't bypass it even if it adds a whole new job."

Environment protection rules only gate a job that declares environment:. A same-repo PR adding a brand-new job that simply omits that key is not gated by them at all — permissions: and any environment association are job-local YAML, which is exactly the class of thing this PR correctly identifies as PR-controlled. So required reviewers alone do not close the "adds a whole new job" case the body says they close.

There is a version that works, and it is worth writing into #1456 so the follow-up doesn't get built against the weaker claim: Environment-scoped secrets genuinely are unavailable to a job that doesn't declare the environment. So the control holds only if publishing stops depending on GITHUB_TOKEN permissions and starts depending on a credential that lives in the environment — an App installation token or PAT held as an environment secret. A new undeclaring job can still request packages: write, but it would have nothing to authenticate with. Otherwise the durable answer is publishing from a trusted default-branch workflow that PR-head YAML cannot influence.

Credit where it is due: the body is already honest that this file cannot defend against a PR redeclaring permissions from scratch. The over-claim is specifically about what the Environment follow-up would buy.

Low — the header comment is now stale

.github/workflows/docker.yml:24 still reads:

#   - pull_request           → build only (no push), cache stays warm

The cache no longer stays warm on a PR. build-pr has cache-from and no cache-to (confirmed: zero cache-to occurrences in that job, one in build), so a PR consumes the cache without refreshing it — and with no registry login it only reads at all if the buildcache package is anonymously pullable. Suggest: "build only (no push); reads the cache when anonymously available, never writes it."

Observation, not a finding — the fix is scoped to build

push-gateway-build still carries packages: write with no job-level event guard, and its GHCR login step still uses the same github.event_name != 'pull_request' || head.repo.full_name == github.repository pattern that #1454 identifies as the problem. It is unreachable here — if: github.repository == 'block/buzz' is false in this fork — so this is not a defect for us and not something this PR needs to fix.

It does mean the Objective sentence, "docker.yml's pull_request-triggered execution path holds no GHCR-write or OIDC-attestation credentials", is true of the path that executes in this repository, not of the whole file. One clause would make it exact.

Not verified

• Whether ghcr.io/launchpad-26/buzz-buildcache is anonymously pullable. The API refuses without read:packages (HTTP 403), so I could not confirm whether PR builds now run warm or cold. The body flags this as a knowingly accepted trade-off; the first merged PR build's wall-clock will answer it.
• Whether Build (linux/amd64) / Build (linux/arm64) are in fact required checks. #1420 records that the protection config is not readable without admin, so the name-matching decision is defensive rather than confirmed — which is the right way round.
• That each pinned action SHA matches its adjacent version comment. Both this session and the cross-model pass were offline; the pins were carried over unchanged from the existing job, so the risk is inherited rather than introduced.

_{Reviewed by AI agents on behalf of @serina-mcfall: Claude Opus 5 orchestrating, and an independent Codex pass run read-only and offline over a pre-fetched copy of this PR and issues #1454/#1456/#1420. The two agreed on every security property above; the #1456 finding and the stale-comment finding came from Codex and were re-verified here before being included. I have not approved and will not approve this PR — per launchpad/AGENTS.md §5.1 that is not an agent's call.}

[serina-mcfall]

Approved and Reviewed

[benmitchell11]

Thank you for the thorough review — both real findings addressed:

High (#1456's premise wrong): confirmed and corrected. Rewrote #1456: required reviewers alone don't gate a job that never declares the protected environment in the first place. The issue now describes the two approaches that actually close it — an Environment-scoped publish credential (so a job that skips the environment reference has nothing to authenticate with) or moving publish to a workflow PR-head content can't influence (workflow_run against the completed push, or manual dispatch).

Low (stale header comment): fixed in #1458, since this PR was already merged.

Also, while acting on this: found and fixed a separate, unrelated problem — four issues I filed this session (including #1454 and #1456) had been silently failing buzz's own issue-body-check workflow since creation, because I'd been reusing buzz-infrastructure's issue format instead of this repo's actual per-type templates. All four now pass; noting it here since #1456 is one of the affected issues and its body has since been reformatted to match the Task template on top of the premise correction above.