diff --git a/cmd/gate/docs/FOLLOWUPS.md b/cmd/gate/docs/FOLLOWUPS.md index 174788b1..03593ea2 100644 --- a/cmd/gate/docs/FOLLOWUPS.md +++ b/cmd/gate/docs/FOLLOWUPS.md @@ -95,16 +95,17 @@ wiring `gate` into the merge tail. 5 of 7 real PRs park, with no notification, so they park silently. Emit something on park (stdout/file/console to start) so a parked run surfaces instead of waiting to be noticed. -- [ ] **Pin the primary diff path to the evaluated head.** +- [x] **Pin the primary diff path to the evaluated head.** Surfaced by the evidence-local-diff skeptic panel (2026-07-16). The oversized-PR fallback refuses unless `pulls.head == view.headRefOid`, but the primary path (`gh pr diff `) fetches by PR number with no head pin: a force-push to an innocent head between the view read and the diff read records that head's diff, and a force-push back before merge still satisfies `--match-head-commit`. Window is a sub-call race needing push access + green CI on the decoy, but - gate's threat model includes adversarial agents with push access. Fix: after a successful - `gh pr diff`, re-read `pulls/` and refuse unless `head.sha == view.headRefOid` (shrinks the - window to a sub-call race); airtight variant fetches the under-cap diff SHA-pinned via the - `compare` endpoint. The fallback path already has this property. + gate's threat model includes adversarial agents with push access. **Landed:** the primary path + reads `pulls/`, refuses unless `head.sha == view.headRefOid`, then fetches the merge-base diff + through the SHA-pinned `compare/...` endpoint; the recorded evidence carries that + verified head. Deterministic moved-head and A→B→A mutants prove that mismatched or substituted diff + bytes never become recordable evidence. The fallback path already had this property. - [x] **Refuse to reseal a mismatched anchor as crash recovery.** Surfaced by codex on the tenant-move review (workbench#59, 2026-07-17); the gate judge blocked diff --git a/cmd/gate/internal/evidence/diff_test.go b/cmd/gate/internal/evidence/diff_test.go new file mode 100644 index 00000000..e144737d --- /dev/null +++ b/cmd/gate/internal/evidence/diff_test.go @@ -0,0 +1,96 @@ +package evidence + +import ( + "encoding/json" + "strings" + "testing" +) + +func TestFetchPrimaryDiffPinsViewedHead(t *testing.T) { + const ( + base = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + head = "1111111111111111111111111111111111111111" + ) + result, err := fetchPrimaryDiff(PRRef{Repo: "o/r", Number: 7}, head, primaryDiffFetchers{ + pull: func(PRRef) (json.RawMessage, error) { + return pullHeads(base, head), nil + }, + compare: func(_ PRRef, gotBase, gotHead string) (json.RawMessage, error) { + if gotBase != base || gotHead != head { + t.Fatalf("compare pair = %s...%s, want %s...%s", gotBase, gotHead, base, head) + } + return []byte("the diff"), nil + }, + }) + if err != nil { + t.Fatalf("fetchPrimaryDiff: %v", err) + } + if result.Diff != "the diff" || result.Head != head { + t.Fatalf("result = %+v, want diff and exact viewed head", result) + } +} + +// This is the moved-head mutant: the pull read reports a different head than +// the view. No diff fetch may run and no bytes may escape as evidence. +func TestFetchPrimaryDiffRefusesMovedHead(t *testing.T) { + const ( + viewed = "1111111111111111111111111111111111111111" + moved = "2222222222222222222222222222222222222222" + ) + compareCalled := false + result, err := fetchPrimaryDiff(PRRef{Repo: "o/r", Number: 7}, viewed, primaryDiffFetchers{ + pull: func(PRRef) (json.RawMessage, error) { + return pullHeads("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", moved), nil + }, + compare: func(PRRef, string, string) (json.RawMessage, error) { + compareCalled = true + return []byte("diff for the moved head"), nil + }, + }) + if err == nil { + t.Fatalf("moved head returned recordable evidence: %+v", result) + } + if !strings.Contains(err.Error(), "pr head moved during gather") || + !strings.Contains(err.Error(), viewed) || !strings.Contains(err.Error(), moved) { + t.Fatalf("moved-head refusal lost its evidence: %v", err) + } + if result != (diffResult{}) { + t.Fatalf("moved head returned partial evidence: %+v", result) + } + if compareCalled { + t.Fatal("moved head reached the compare diff fetch") + } +} + +// This is the double-force-push mutant from review: the PR begins at A, moves +// to B during the diff read, then returns to A. The compare fetch can still +// receive only A's immutable commit pair. +func TestFetchPrimaryDiffPinsABARaceToCommitPair(t *testing.T) { + const ( + base = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + viewed = "1111111111111111111111111111111111111111" + moved = "2222222222222222222222222222222222222222" + ) + liveHead := viewed + result, err := fetchPrimaryDiff(PRRef{Repo: "o/r", Number: 7}, viewed, primaryDiffFetchers{ + pull: func(PRRef) (json.RawMessage, error) { return pullHeads(base, liveHead), nil }, + compare: func(_ PRRef, gotBase, gotHead string) (json.RawMessage, error) { + liveHead = moved + defer func() { liveHead = viewed }() + if gotBase != base || gotHead != viewed { + t.Fatalf("mutable pair reached compare: %s...%s", gotBase, gotHead) + } + return []byte("diff for immutable A"), nil + }, + }) + if err != nil { + t.Fatalf("fetchPrimaryDiff under A-B-A race: %v", err) + } + if liveHead != viewed || result.Head != viewed || result.Diff != "diff for immutable A" { + t.Fatalf("A-B-A result = %+v, live head %s", result, liveHead) + } +} + +func pullHeads(base, head string) json.RawMessage { + return []byte(`{"base":{"sha":"` + base + `"},"head":{"sha":"` + head + `"}}`) +} diff --git a/cmd/gate/internal/evidence/evidence.go b/cmd/gate/internal/evidence/evidence.go index 769a729d..8711f6ab 100644 --- a/cmd/gate/internal/evidence/evidence.go +++ b/cmd/gate/internal/evidence/evidence.go @@ -40,7 +40,7 @@ type diffBody struct { PR PRRef `json:"pr"` Diff string `json:"diff"` // Provenance — reconstructable from state alone which path produced the - // diff and which commits it spans. "api" = GitHub's merge-base diff; + // diff and which commits it spans. "api" = GitHub's SHA-pinned compare diff; // "local-merge-base" = the oversized-PR fallback. Method string `json:"method,omitempty"` Head string `json:"head,omitempty"` @@ -115,6 +115,11 @@ type reviewFetchers struct { panel func(PRRef, string, []rawComment, []Comment) reviewpanel.Evidence } +type primaryDiffFetchers struct { + pull func(PRRef) (json.RawMessage, error) + compare func(PRRef, string, string) (json.RawMessage, error) +} + // Gather records view, diff, and comments evidence for a PR and returns their ids. func Gather(st *state.Store, run string, pr PRRef) (Bundle, error) { viewID, view, err := View(st, run, pr) @@ -160,23 +165,20 @@ func GatherFrom(st *state.Store, run string, pr PRRef, viewID string, view json. return b, fmt.Errorf("evidence: parse PR head: %w", err) } - // method "api" records only that GitHub served the diff — not head/merge_base: - // gh pr diff reads by PR number and doesn't report which head it rendered, so - // stamping the view's head would claim a span this path never verified. The - // fallback path controls exact SHAs and stamps them. + // The primary path reads the pull's immutable commit pair, checks that its + // head matches the view, then asks GitHub for that pair's compare diff. The + // fallback controls the same exact SHAs locally when GitHub rejects the diff + // as oversized. body := diffBody{PR: pr, Method: "api"} - diff, err := gh("pr", "diff", fmt.Sprint(pr.Number), "-R", pr.Repo) + r, err := primaryDiff(pr, viewed.HeadRefOid) if tooLarge(err) { - var r diffResult r, err = fallbackDiff(pr, view) - body.Diff, body.Method, body.MergeBase, body.Head = r.Diff, "local-merge-base", r.MergeBase, r.Head + body.Method = "local-merge-base" } if err != nil { return b, err } - if body.Method == "api" { - body.Diff = string(diff) - } + body.Diff, body.MergeBase, body.Head = r.Diff, r.MergeBase, r.Head a, err := st.Append(state.KindEvidence, run, nil, body) if err != nil { return b, err @@ -211,6 +213,47 @@ func GatherFrom(st *state.Store, run string, pr PRRef, viewID string, view json. return b, nil } +func primaryDiff(pr PRRef, viewHead string) (diffResult, error) { + return fetchPrimaryDiff(pr, viewHead, primaryDiffFetchers{ + pull: func(pr PRRef) (json.RawMessage, error) { + return gh("api", fmt.Sprintf("repos/%s/pulls/%d", pr.Repo, pr.Number)) + }, + compare: func(pr PRRef, base, head string) (json.RawMessage, error) { + return gh("api", "-H", "Accept: application/vnd.github.v3.diff", + fmt.Sprintf("repos/%s/compare/%s...%s", pr.Repo, base, head)) + }, + }) +} + +// fetchPrimaryDiff binds the primary diff to an immutable commit pair. The +// pull read must still match the view evidence, and the diff is then fetched by +// those exact base/head SHAs rather than by mutable PR number. An A→B→A +// force-push during the fetch cannot substitute B's bytes for A's evidence. +func fetchPrimaryDiff(pr PRRef, viewHead string, fetchers primaryDiffFetchers) (diffResult, error) { + pull, err := fetchers.pull(pr) + if err != nil { + return diffResult{}, err + } + base, head, err := parsePullHeads(pull) + if err != nil { + return diffResult{}, err + } + if !reSHA.MatchString(base) || !reSHA.MatchString(head) { + return diffResult{}, fmt.Errorf("evidence: non-hex commit id from api (base=%q head=%q)", base, head) + } + if head != viewHead { + return diffResult{}, fmt.Errorf("evidence: pr head moved during gather: view %s, pulls %s", viewHead, head) + } + diff, err := fetchers.compare(pr, base, head) + if err != nil { + return diffResult{}, err + } + if len(diff) == 0 { + return diffResult{}, fmt.Errorf("evidence: empty diff at head %s", head) + } + return diffResult{Diff: string(diff), Head: head}, nil +} + // decisiveReviewState reports whether a submission state states a position on // whether the PR may merge. APPROVED and CHANGES_REQUESTED do; COMMENTED does // NOT — GitHub is unambiguous that commenting after approving does not withdraw