fix(merge gate): bind pr_merge and tbd_ship merges to the head commit the gate checked (TOCTOU)
The gate evaluates CI for one head sha; a push to the PR branch between that check and the merge would have merged unchecked code. MergePRArgs now carries head_commit_id (documented in Gitea 1.27.1's MergePullRequestOption) and both tools send the sha they evaluated. Tests for both tools; each removal flip-tested red. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SZkhi49hfeSqBp5aZJNXsM
This commit is contained in:
@@ -133,6 +133,8 @@ all fail closed: nothing is merged, the result is `merged:false` with a `reason`
|
|||||||
Unclassified is not low. Above `risk/low` a human reviews and merges in the Gitea UI (which this gate does not touch).
|
Unclassified is not low. Above `risk/low` a human reviews and merges in the Gitea UI (which this gate does not touch).
|
||||||
`pr_merge` additionally fails with an error, merging nothing, when the PR or the base's branch-protection state cannot
|
`pr_merge` additionally fails with an error, merging nothing, when the PR or the base's branch-protection state cannot
|
||||||
be read. A merge conflict is still an error.
|
be read. A merge conflict is still an error.
|
||||||
|
Both tools bind the merge to the head commit whose CI the gate evaluated (`head_commit_id`), so a push to the PR
|
||||||
|
branch after the check makes Gitea refuse the merge instead of merging unchecked code.
|
||||||
|
|
||||||
Limits: the gate enforces the tier of the issue the caller names; it cannot prove that issue is the one the change
|
Limits: the gate enforces the tier of the issue the caller names; it cannot prove that issue is the one the change
|
||||||
belongs to, so the link is the caller's claim (local-dev rule 5's convention still applies). Not gated: merging in the
|
belongs to, so the link is the caller's claim (local-dev rule 5's convention still applies). Not gated: merging in the
|
||||||
|
|||||||
@@ -109,6 +109,8 @@ type MergePRArgs struct {
|
|||||||
Do string `json:"Do"`
|
Do string `json:"Do"`
|
||||||
Title string `json:"merge_message_title,omitempty"`
|
Title string `json:"merge_message_title,omitempty"`
|
||||||
Body string `json:"merge_message_field,omitempty"`
|
Body string `json:"merge_message_field,omitempty"`
|
||||||
|
// HeadCommitID binds the merge to the commit whose CI was checked: if the PR branch moved, Gitea refuses.
|
||||||
|
HeadCommitID string `json:"head_commit_id,omitempty"`
|
||||||
}
|
}
|
||||||
|
|
||||||
func (c *Client) MergePullRequest(ctx context.Context, owner, repo string, index int, args MergePRArgs) error {
|
func (c *Client) MergePullRequest(ctx context.Context, owner, repo string, index int, args MergePRArgs) error {
|
||||||
|
|||||||
@@ -97,9 +97,10 @@ func (t *PRMerge) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag
|
|||||||
}
|
}
|
||||||
|
|
||||||
if err := t.c.MergePullRequest(ctx, args.Owner, args.Repo, args.Number, gitea.MergePRArgs{
|
if err := t.c.MergePullRequest(ctx, args.Owner, args.Repo, args.Number, gitea.MergePRArgs{
|
||||||
Do: style,
|
Do: style,
|
||||||
Title: args.Title,
|
Title: args.Title,
|
||||||
Body: args.Body,
|
Body: args.Body,
|
||||||
|
HeadCommitID: pr.Head.Sha,
|
||||||
}); err != nil {
|
}); err != nil {
|
||||||
return nil, err
|
return nil, err
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -145,6 +145,19 @@ func TestPRMergeDefaultsToMergeStyleAndPassesStyleThrough(t *testing.T) {
|
|||||||
assert.Equal(t, "squash", payload["Do"])
|
assert.Equal(t, "squash", payload["Do"])
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TOCTOU: the gate read CI for ONE head commit. The merge must be bound to that commit, so a push to the PR branch
|
||||||
|
// after the check makes Gitea refuse the merge instead of merging code nobody checked.
|
||||||
|
func TestPRMerge_TheMergeIsBoundToTheCommitTheGateChecked(t *testing.T) {
|
||||||
|
f := newPRMergeFake(t, prMergeFakeOpts{})
|
||||||
|
|
||||||
|
_, err := callMerge(t, f, mergeArgs)
|
||||||
|
|
||||||
|
require.NoError(t, err)
|
||||||
|
var payload map[string]any
|
||||||
|
require.NoError(t, json.Unmarshal(f.mergeBody, &payload))
|
||||||
|
assert.Equal(t, "abc", payload["head_commit_id"], "the head sha the CI check used")
|
||||||
|
}
|
||||||
|
|
||||||
// A merge conflict is still an error, as before the gate existed.
|
// A merge conflict is still an error, as before the gate existed.
|
||||||
func TestPRMergeConflictReturnsError(t *testing.T) {
|
func TestPRMergeConflictReturnsError(t *testing.T) {
|
||||||
f := newPRMergeFake(t, prMergeFakeOpts{mergeStatus: http.StatusConflict})
|
f := newPRMergeFake(t, prMergeFakeOpts{mergeStatus: http.StatusConflict})
|
||||||
|
|||||||
@@ -320,7 +320,7 @@ func (t *TBDShip) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag
|
|||||||
}
|
}
|
||||||
|
|
||||||
// Green + unprotected → squash-merge, then delete the short-lived branch.
|
// Green + unprotected → squash-merge, then delete the short-lived branch.
|
||||||
if err := t.c.MergePullRequest(ctx, args.Owner, args.Repo, pr.Number, gitea.MergePRArgs{Do: "squash"}); err != nil {
|
if err := t.c.MergePullRequest(ctx, args.Owner, args.Repo, pr.Number, gitea.MergePRArgs{Do: "squash", HeadCommitID: pr.Head.Sha}); err != nil {
|
||||||
if errors.Is(err, gitea.ErrConflict) {
|
if errors.Is(err, gitea.ErrConflict) {
|
||||||
result["reason"] = "base moved and the merge is not clean — PR left open, not merged; resolve the conflict and merge manually"
|
result["reason"] = "base moved and the merge is not clean — PR left open, not merged; resolve the conflict and merge manually"
|
||||||
return textOK(result)
|
return textOK(result)
|
||||||
|
|||||||
@@ -3,6 +3,7 @@ package tools_test
|
|||||||
import (
|
import (
|
||||||
"context"
|
"context"
|
||||||
"encoding/json"
|
"encoding/json"
|
||||||
|
"io"
|
||||||
"net/http"
|
"net/http"
|
||||||
"net/http/httptest"
|
"net/http/httptest"
|
||||||
"strings"
|
"strings"
|
||||||
@@ -63,6 +64,8 @@ func shipFakeFull(t *testing.T, runsJSON string, requiredApprovals, mergeStatus
|
|||||||
case r.Method == http.MethodGet && strings.Contains(p, "/actions/runs"):
|
case r.Method == http.MethodGet && strings.Contains(p, "/actions/runs"):
|
||||||
_, _ = w.Write([]byte(`{"total_count":0,"workflow_runs":` + runsJSON + `}`))
|
_, _ = w.Write([]byte(`{"total_count":0,"workflow_runs":` + runsJSON + `}`))
|
||||||
case r.Method == http.MethodPost && strings.HasSuffix(p, "/pulls/7/merge"):
|
case r.Method == http.MethodPost && strings.HasSuffix(p, "/pulls/7/merge"):
|
||||||
|
body, _ := io.ReadAll(r.Body)
|
||||||
|
shipLastMergeBody.Store(string(body))
|
||||||
merged.Store(true)
|
merged.Store(true)
|
||||||
w.WriteHeader(mergeStatus)
|
w.WriteHeader(mergeStatus)
|
||||||
_, _ = w.Write([]byte(`{}`))
|
_, _ = w.Write([]byte(`{}`))
|
||||||
@@ -216,6 +219,23 @@ func TestTBDShip_Resume_ExistingBranchAndPR(t *testing.T) {
|
|||||||
assert.True(t, merged.Load())
|
assert.True(t, merged.Load())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// shipLastMergeBody is the body of the most recent merge request the fake received.
|
||||||
|
var shipLastMergeBody atomic.Value
|
||||||
|
|
||||||
|
// TOCTOU: the merge is bound to the head commit whose CI the gate evaluated (gitea-mcp#74).
|
||||||
|
func TestTBDShip_TheMergeIsBoundToTheCommitTheGateChecked(t *testing.T) {
|
||||||
|
var merged, deleted atomic.Bool
|
||||||
|
srv := shipFake(t, shipGreen, &merged, &deleted)
|
||||||
|
defer srv.Close()
|
||||||
|
|
||||||
|
res := callShip(t, srv.URL, shipArgs)
|
||||||
|
|
||||||
|
require.Equal(t, true, res["merged"])
|
||||||
|
var payload map[string]any
|
||||||
|
require.NoError(t, json.Unmarshal([]byte(shipLastMergeBody.Load().(string)), &payload))
|
||||||
|
assert.Equal(t, "abc", payload["head_commit_id"], "the head sha the CI check used")
|
||||||
|
}
|
||||||
|
|
||||||
// ---- gitea-mcp#63: the risk tier is an input to the gate. Only exactly one risk/low label merges. ----
|
// ---- gitea-mcp#63: the risk tier is an input to the gate. Only exactly one risk/low label merges. ----
|
||||||
const (
|
const (
|
||||||
shipGreen = `[{"id":1,"status":"completed","conclusion":"success","head_sha":"abc"}]`
|
shipGreen = `[{"id":1,"status":"completed","conclusion":"success","head_sha":"abc"}]`
|
||||||
|
|||||||
Reference in New Issue
Block a user