Let a fork's records survive their own squash (#723) - #730
Merged
Conversation
`preserve` ran on `pull_request`, which hands a fork's workflow a read-only token. So the job read the branch's commits, built the inherited note correctly, and could not publish it:
merge-type=squash records=4 pushed=false
→ the record was attached on the runner and is about to be discarded with it
That is what happened to #720. Four records were lost at the merge and recovered by hand afterwards, in the repository whose whole claim is that records survive a squash. `contents: write` was already declared; it cannot grant what the event does not carry.
`pull_request_target` carries it. That event is better known for handing an attacker a write token, and the mechanism is specific: it checks out the base by default, and the hole opens when a workflow points the checkout at the fork's head and then runs it.
This job never has. It checks out `base.ref` -- for an unrelated reason, because `refs/pull/<n>/merge` is gone once the pull request closes -- installs and builds from that tree, and brings the fork's commits in as `refs/commitlore/pr-head`, which is read for trailer blocks and nothing else. Data, never scripts. The change is one line; what makes it safe was already true.
Being already true is not the same as staying true, so the two rules are asserted rather than requested: the checkout must name the base branch, and every command mentioning the fork ref must be a `git fetch`. Reverting the event, pointing the checkout at the head, or running anything out of that ref each fail a named case.
Limit: this runs on close, so it publishes rather than gates -- a merge that could not carry its records still happens first, and the report is the only thing standing there
Limit: the fetched ref is trusted as data by every consumer downstream; a future step that treats a commit message as a command would reopen this by a different door
Ruled-out: make preserve a required check | it runs on the close event, so it cannot be a merge gate at all
Ruled-out: a maintainer runs squash-preserve after each fork merge | it is what I did for #720, and a manual step nobody is reminded of fails on the fifth occurrence rather than the first
Blast: system
Undo: easy
Certainty: firm
Record-Id: r-forkpreserve723
Provenance: authored
Verified: three mutations, each caught by the case whose name claims it -- reverting to `pull_request` fails the trigger case; pointing the checkout at `head.sha` fails the base-branch case; adding `git checkout refs/commitlore/pr-head -- .` before `npm ci` fails three, including the one asserting every mention of that ref is a fetch. Six cases pass on the file as it stands; the assertions read the workflow with comment lines stripped, so a comment cannot satisfy them.
CommitLore-Version: 2.0.0
CommitLore — record lintTrailers: clean — 3 commits in Trailer violations fail this check. Active constraints are informational — they are what the repository already decided, not a verdict on this PR. |
`action-preserve` asserted `on.pull_request.types` and #723 moved the workflow to `pull_request_target`, so the case died on `Cannot read properties of undefined` rather than on anything it was checking. What it was checking is unchanged: the job fires on the closed event and nothing else. It now reads whichever key carries that. Which key it should be, and the two rules that keep the stronger event safe, are asserted in `test/preserve-workflow-safety.test.ts` -- a property belongs in one place, and that place is the file named after it. Limit: this is the third guard this week whose subject moved out from under it; the pattern is a case pinned to a spelling rather than to the thing spelled Blast: local Undo: easy Certainty: firm Record-Id: r-trigkey723 Provenance: authored Verified: 37 cases pass across action-preserve and preserve-workflow-safety together; before this the check jobs failed with `Cannot read properties of undefined (reading 'types')`, which named the spelling and not the property CommitLore-Version: 2.0.0
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #723.
preserveran onpull_request, which hands a fork's workflow a read-only token. The job read the branch's commits, built the inherited note correctly, and could not publish it:That is what happened to #720 — four records lost at the merge, recovered by hand afterwards, in the repository whose whole claim is that records survive a squash.
contents: writewas already declared; it cannot grant what the event does not carry.Why
pull_request_targetis safe hereThat event is better known for handing an attacker a write token, and the mechanism is specific: it checks out the base by default, and the hole opens when a workflow points the checkout at the fork's head and then runs it.
This job never has. It checks out
base.ref— for an unrelated reason, becauserefs/pull/<n>/mergeis gone once the PR closes — installs and builds from that tree, and brings the fork's commits in asrefs/commitlore/pr-head, read for trailer blocks and nothing else. Data, never scripts.The change is one line. What makes it safe was already true.
Being already true is not staying true
So the two rules are asserted rather than requested.
test/preserve-workflow-safety.test.tsreads the workflow with comment lines stripped — a comment cannot satisfy it — and requires that the checkout names the base branch and that every command mentioning the fork ref is agit fetch.pull_requestref: github.event.pull_request.head.shagit checkout refs/commitlore/pr-head -- . && npm ciWhat this does not do
It runs on close, so it publishes rather than gates: a merge that cannot carry its records still happens first, and the report is the only thing standing there. Making
preservea required check cannot fix that — the close event is after the merge by construction.