Harden CI: stop the screenshot job persisting the job credential - #5338
Conversation
Both checkouts in pr-screenshots.yml are read-only source trees: they are
rendered and screenshotted, never pushed from. The publish step does not use
either working copy -- it git-inits a fresh temp repo and pushes to an
explicit https://x-access-token:${GH_TOKEN}@... remote.
So the credential actions/checkout leaves in .git/config has no consumer here.
It only sits readable for the rest of a job that builds and executes code from
the pull request head, including on the base checkout.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Wb9DAnFEFQS6NvPsqqz7Kq
✅ Drift Bot (ClawMetry): no drift detectedDrift Bot analyzed the changed files against this project's blueprints and requirements and found no drift. |
Visual diffComparing 13 of 66 comparison(s) flagged (>1% pixel diff).
Folder: f08398b0c28d. Full PNGs also attached as a workflow artefact. Generated by visual-diff bot. Pixel diffs >1% flagged; eyeball the table before merging. This check is non-blocking — fail = bot bug, not a code problem. |
✅ Drift Bot (ClawMetry): no drift detectedDrift Bot analyzed the changed files against this project's blueprints and requirements and found no drift. |
|
✨ auto-fixed: merged latest main into branch (branch was BEHIND) Generated by Claude Code |
Ignore the visual-diff table on this PR — it is measuring its own noiseFlagging this because the bot's comment is edited in place, so a reviewer only ever sees the latest table, and "38 of 66 flagged" on a PR that cannot change a pixel looks alarming without the context. This PR's diff is 9 added lines in The two runs make that concrete. Same diff, and views swing in both directions between "identical" and "completely different":
A view cannot go from 0.01% to 100.00% because of a YAML key that was already present in the first run. The totals moved 43/66 → 38/66 for the same reason. Consistent with the bot's own footer: it is non-blocking, and both runs concluded One thing the runs did confirm positively: the publish step pushed to Generated by Claude Code |
|
✅ Drift Bot (ClawMetry): no drift detectedDrift Bot analyzed the changed files against this project's blueprints and requirements and found no drift. |
|
✨ auto-fixed: merged main (14572bc) into branch to resolve BEHIND state Generated by Claude Code |
✅ Drift Bot (ClawMetry): no drift detectedDrift Bot analyzed the changed files against this project's blueprints and requirements and found no drift. |
|
✨ auto-fixed: merged main into branch (was BEHIND at 14572bc) Generated by Claude Code |
1 participant
Product record: No-PRD: CI-only change confined to
.github/, a PRD-exempt path.Risk: Low. One workflow, two
actions/checkoutsteps, nine added lines of which seven are a comment. Nothing in the shipped package changes. Undone by reverting one commit.Summary
Continues the
persist-credentials: falsesweep (#5312, #5321, #5327 merged; #5330, #5331 open) with the one remaining workflow whose checkouts are provably not the credential's consumer.actions/checkoutleaves a credential in.git/configby default.pr-screenshots.ymlis the job where that matters most and was least necessary:git inits a new repo there, and pushes to an explicithttps://x-access-token:${GH_TOKEN}@github.com/${GITHUB_REPOSITORY}.gitremote. The credential the checkouts persist is simply never read..git/configis readable for the remainder of that job — including from thebasecheckout, which exists only to render "before" images.Both checkouts are read-only source trees, so
persist-credentials: falseremoves the credential without removing anything that uses it.Why this workflow and not the other four remaining ones
The sweep is deliberately not finished in this PR. Four checkouts still persist their credential, and each is a different question rather than more of the same:
auto-quarantine.ymlgit push -u origin "$BRANCH"— it is the consumer. Needs the push rewritten to an explicit remote first, which is a behaviour change, not a hardening one.release-on-merge.ymlgit push origin "refs/tags/$NEW_TAG",git push origin "$BRANCH"), on the release pipeline. Same rewrite, higher blast radius.i18n-autotranslate.yml,i18n-docs-autotranslate.ymlpeter-evans/create-pull-request, which authenticates from its owntokeninput. That very likely toleratespersist-credentials: false, but the action's docs do not state it and this repo has no workflow pairing the two, so there is no evidence here either way. Not worth a silently-broken translation bot to find out in passing.The second
auto-deploy-cloud.ymlcheckout stays as-is by design — #5331 documents that its credential is the one the force-push uses.Test plan
yaml.safe_loadover all 34 workflow files → parseyaml.safe_loadover all composite action files → parsevisual-diffcheckout steps reportpersist-credentials: Falsewithpathandrefintactpython3 scripts/check_action_refs.py→ OK, 17 distinct references, pinning unaffectedpytest tests/test_workflow_yaml_valid.py tests/test_ci_workflow_invocations_are_real.py→ 189 passed, 2 skipped (unchanged from baseline)mktemp -d+git init+ an explicit token remote, not either checkoutNote for reviewers
The one thing worth a second opinion is the claim that nothing reads the persisted credential. The publish step is the only
git pushin the file and it runs entirely inside$(mktemp -d); the screenshot steps read fromhead/andbase/as plain source trees. If there is a path I missed that expects an authenticated remote in either checkout, that is the thing to catch here.Generated by Claude Code