Commit `f84bca7807`

f84bca780788661af127cba684ebc78194cb7fcb

parent: 319867a8f1

Verified · cmc ci/build: success ci/sonar: success ci/test: success ci/vuln: success

cmc <hello@cleberg.net> · 2026-09-05 01:05 UTC

mr: only a writer's review decides a merge gate

mr review resolves with CanRead and applies no further check, and
reviewGates counted every fresh verdict. On a public repository that let
anyone with an account satisfy require_approvals — defeating four-eyes
review by holding two accounts, which on an open-registration instance
means defeating it outright — and equally let a passer-by block a merge
the owner wanted, with no override and only a force-push to clear it.
Both confirmed against a running instance before this was written.

Reviewing stays open to every reader. An outside opinion on a public
change is worth having, and discarding it would be the wrong fix; it just
does not decide the gate. What decides the gate is write access, because
that is the same question the gates already answer — a person who could
push this change themselves is the person whose approval means the
repository accepts it.

Someone named in CODEOWNERS without write access is a misconfiguration
for an owner to fix rather than a case to special-case: they could not
merge what they approved.

mr show and the merge request page mark an uncounted review as advisory.
A page showing an approval the gate ignores differs from the gate, and
the difference would surface only when a merge was refused. Both read the
same ReviewersWhoCount, so the two cannot drift.

Closes #147

Layout: unified · split

e2e/reviewstanding_test.go added +111

		@@ -0,0 +1,111 @@
	1	package e2e
	2
	3	import (
	4	"os"
	5	"path/filepath"
	6	"strings"
	7	"testing"
	8	)
	9
	10	// TestReviewsCountOnlyFromWriters: a merge gate is a repository's own
	11	// rule, so only people the repository trusts can decide it.
	12	//
	13	// mr review resolves with CanRead and applies no further check, and
	14	// reviewGates counted every fresh verdict. On a public repository that
	15	// let anyone with an account satisfy require-approvals — defeating
	16	// four-eyes review by having two accounts, which on an open-registration
	17	// instance means defeating it outright — and equally let them block a
	18	// merge the owner wanted (#147).
	19	//
	20	// Reviewing stays open to everyone: an outside opinion on a public change
	21	// is worth having. It just does not decide the gate.
	22	func TestReviewsCountOnlyFromWriters(t *testing.T) {
	23	inst := startInstance(t)
	24	ownerKey := inst.newKey(t, "owner")
	25	writerKey := inst.newKey(t, "writer")
	26	strangerKey := inst.newKey(t, "stranger")
	27	inst.admin(t, "admin", "user", "create", "owner", "--key", ownerKey+".pub",
	28	"--email", "owner@example.test", "--verified")
	29	inst.admin(t, "admin", "user", "create", "writer", "--key", writerKey+".pub")
	30	inst.admin(t, "admin", "user", "create", "stranger", "--key", strangerKey+".pub")
	31
	32	// Public, so the stranger can read it and open a review at all.
	33	if _, errOut, code := inst.ssh(t, ownerKey, "", "repo", "create", "owner/pub"); code != 0 {
	34	t.Fatalf("repo create: %s", errOut)
	35	}
	36	if _, _, code := inst.ssh(t, ownerKey, "", "repo", "access", "grant", "owner/pub", "writer", "write"); code != 0 {
	37	t.Fatal("grant failed")
	38	}
	39	if _, _, code := inst.ssh(t, ownerKey, "", "repo", "settings", "require-approvals", "owner/pub", "1"); code != 0 {
	40	t.Fatal("require-approvals failed")
	41	}
	42
	43	env := inst.gitEnv(ownerKey)
	44	work := t.TempDir()
	45	mustGit(t, work, env, "clone", inst.sshURL("owner/pub"), "w")
	46	dir := filepath.Join(work, "w")
	47	os.WriteFile(filepath.Join(dir, "a.txt"), []byte("a\n"), 0o644)
	48	mustGit(t, dir, env, "checkout", "-q", "-b", "main")
	49	mustGit(t, dir, env, "add", ".")
	50	mustGit(t, dir, env, "commit", "-q", "-m", "base")
	51	mustGit(t, dir, env, "push", "-q", "origin", "main")
	52	newMR := func(branch string) {
	53	t.Helper()
	54	mustGit(t, dir, env, "checkout", "-q", "-b", branch, "main")
	55	mustGit(t, dir, env, "commit", "-q", "--allow-empty", "-m", branch)
	56	mustGit(t, dir, env, "push", "-q", "origin", branch)
	57	if _, errOut, code := inst.ssh(t, ownerKey, "", "mr", "create", "owner/pub",
	58	"--source", branch, "--target", "main", "--title", "'"+branch+"'"); code != 0 {
	59	t.Fatalf("mr create %s: %s", branch, errOut)
	60	}
	61	}
	62
	63	// !1 — a stranger's approval must not satisfy the gate.
	64	newMR("feat1")
	65	if _, errOut, code := inst.ssh(t, strangerKey, "", "mr", "review", "owner/pub", "1", "--approve"); code != 0 {
	66	t.Fatalf("a stranger should still be able to review a public MR: %s", errOut)
	67	}
	68	_, errOut, code := inst.ssh(t, ownerKey, "", "mr", "merge", "owner/pub", "1")
	69	if code != 4 \|\| !strings.Contains(errOut, "fresh approval") {
	70	t.Fatalf("a stranger's approval satisfied require-approvals: exit %d, %s", code, errOut)
	71	}
	72	// A writer's approval does.
	73	if _, errOut, code := inst.ssh(t, writerKey, "", "mr", "review", "owner/pub", "1", "--approve"); code != 0 {
	74	t.Fatalf("writer review: %s", errOut)
	75	}
	76	if _, errOut, code := inst.ssh(t, ownerKey, "", "mr", "merge", "owner/pub", "1"); code != 0 {
	77	t.Fatalf("a writer's approval did not satisfy the gate: %s", errOut)
	78	}
	79
	80	// !2 — a stranger's objection must not block the owner.
	81	newMR("feat2")
	82	if _, _, code := inst.ssh(t, writerKey, "", "mr", "review", "owner/pub", "2", "--approve"); code != 0 {
	83	t.Fatal("writer approve failed")
	84	}
	85	if _, errOut, code := inst.ssh(t, strangerKey, "", "mr", "review", "owner/pub", "2", "--request-changes"); code != 0 {
	86	t.Fatalf("stranger request-changes: %s", errOut)
	87	}
	88	if _, errOut, code := inst.ssh(t, ownerKey, "", "mr", "merge", "owner/pub", "2"); code != 0 {
	89	t.Fatalf("a stranger blocked the owner's merge: %s", errOut)
	90	}
	91
	92	// !3 — a writer's objection still blocks, or the gate means nothing.
	93	newMR("feat3")
	94	if _, _, code := inst.ssh(t, writerKey, "", "mr", "review", "owner/pub", "3", "--request-changes"); code != 0 {
	95	t.Fatal("writer request-changes failed")
	96	}
	97	_, errOut, code = inst.ssh(t, ownerKey, "", "mr", "merge", "owner/pub", "3")
	98	if code != 4 \|\| !strings.Contains(errOut, "writer requested changes") {
	99	t.Fatalf("a writer's objection did not block: exit %d, %s", code, errOut)
	100	}
	101
	102	// The review is still recorded and visible either way — it is the
	103	// gate that ignores it, not the conversation.
	104	out, _, _ := inst.ssh(t, ownerKey, "", "mr", "show", "owner/pub", "2", "--json")
	105	if !strings.Contains(out, "stranger") {
	106	t.Fatalf("the stranger's review was discarded rather than recorded:\n%s", out)
	107	}
	108	if !strings.Contains(out, `"counts":false`) {
	109	t.Fatalf("mr show does not say the review is not counted:\n%s", out)
	110	}
	111	}

internal/control/mr.go +47 −4

		@@ -505,8 +505,9 @@ func runMRShow(c *Ctx, args []string) int {
505	505	cs = append(cs, commentOut{cm.Author, cm.Body, cm.BodyFormat, cm.CreatedAt})
506	506	}
507	507	var rs []ReviewOut
	508	counts := ReviewersWhoCount(c.Store, repo, reviews)
508	509	for _, r := range reviews {
509		rs = append(rs, ReviewOut{r.Reviewer, r.Verdict, r.Stale, r.CreatedAt})
	510	rs = append(rs, ReviewOut{r.Reviewer, r.Verdict, r.Stale, counts[r.Reviewer], r.CreatedAt})
510	511	}
511	512	// The commits this MR carries: base..head, the diff's range.
512	513	var commits []CommitOut
		@@ -572,7 +573,11 @@ func runMRShow(c *Ctx, args []string) int {
572	573	if r.Stale {
573	574	stale = " (stale)"
574	575	}
575		fmt.Fprintf(w, "review: %s %s%s at %s\n", r.Reviewer, r.Verdict, stale, r.CreatedAt)
	576	advisory := ""
	577	if !r.Counts {
	578	advisory = " (advisory: no write access)"
	579	}
	580	fmt.Fprintf(w, "review: %s %s%s%s at %s\n", r.Reviewer, r.Verdict, stale, advisory, r.CreatedAt)
576	581	}
577	582	for _, cm := range cs {
578	583	fmt.Fprintf(w, "\n--- %s at %s\n%s\n", cm.Author, cm.CreatedAt, cm.Body)
		@@ -1112,10 +1117,16 @@ func (c *Ctx) reviewGates(repo store.Repo, mr store.MR, dir, targetSHA, headSHA
1112	1117	if err != nil {
1113	1118	return c.fail(protocol.ExitFailure, "%v", err)
1114	1119	}
1115		// Latest fresh review per reviewer decides their stance.
	1120	// Latest fresh review per reviewer decides their stance — but only
	1121	// from someone the repository trusts to write to it. Reviewing is
	1122	// open to any reader, which is what makes an outside opinion on a
	1123	// public change possible; deciding a merge gate is not the same
	1124	// thing, and counting every verdict let anyone with an account
	1125	// satisfy require_approvals or block a merge indefinitely (#147).
	1126	counts := ReviewersWhoCount(c.Store, repo, reviews)
1116	1127	latest := map[string]string{}
1117	1128	for _, r := range reviews {
1118		if r.Stale \|\| r.Reviewer == mr.Author {
	1129	if r.Stale \|\| r.Reviewer == mr.Author \|\| !counts[r.Reviewer] {
1119	1130	continue
1120	1131	}
1121	1132	latest[r.Reviewer] = r.Verdict
		@@ -1442,3 +1453,35 @@ func runMRRangeDiff(c *Ctx, args []string) int {
1442	1453	}
1443	1454	return protocol.ExitOK
1444	1455	}
	1456
	1457	// reviewersWhoCount is the set of reviewers whose verdict decides a merge
	1458	// gate: those with write access to the repository.
	1459	//
	1460	// Write, rather than a separate reviewer role, because it is the same
	1461	// question the gates already answer — a person who could push this change
	1462	// themselves is the person whose approval means the repository accepts
	1463	// it. Someone named in CODEOWNERS without write is a misconfiguration the
	1464	// owner should fix rather than a case to special-case here: they could
	1465	// not merge what they approved.
	1466	// Exported because the web renders the same distinction: a page that
	1467	// showed an approval the gate ignores would differ from the gate, and the
	1468	// difference would only surface when a merge was refused.
	1469	func ReviewersWhoCount(st *store.Store, repo store.Repo, reviews []store.MRReview) map[string]bool {
	1470	counts := map[string]bool{}
	1471	for _, r := range reviews {
	1472	if _, done := counts[r.Reviewer]; done {
	1473	continue
	1474	}
	1475	counts[r.Reviewer] = false
	1476	u, err := st.UserByUsername(r.Reviewer)
	1477	if err != nil {
	1478	continue
	1479	}
	1480	grant, err := st.AccessRole(repo.ID, u.ID)
	1481	if err != nil {
	1482	continue
	1483	}
	1484	counts[r.Reviewer] = policy.CanWrite(u, repo, grant)
	1485	}
	1486	return counts
	1487	}

internal/control/output.go +9 −3

		@@ -37,9 +37,15 @@ type MRShow struct {
37	37
38	38	// ReviewOut is one review on a merge request.
39	39	type ReviewOut struct {
40		Reviewer string `json:"reviewer"`
41		Verdict string `json:"verdict"`
42		Stale bool `json:"stale"`
	40	Reviewer string `json:"reviewer"`
	41	Verdict string `json:"verdict"`
	42	Stale bool `json:"stale"`
	43	// Counts reports whether this verdict decides the merge gates. A
	44	// reader may review a public merge request; only someone who can
	45	// write to the repository decides whether it merges. Without this the
	46	// page would show an approval the gate ignores, and the difference
	47	// would be invisible until a merge was refused (#147).
	48	Counts bool `json:"counts"`
43	49	CreatedAt string `json:"created_at"`
44	50	}
45	51

internal/httpd/mrpage_test.go +8 −2

		@@ -20,7 +20,7 @@ type mrPageData struct {
20	20	Checks []store.Check
21	21	Combined string
22	22	Comments []renderedComment
23		Reviews []store.MRReview
	23	Reviews []reviewRow
24	24	DiffFiles []diffFile
25	25	Stat diffStat
26	26	Commits []struct{}
		@@ -34,11 +34,17 @@ type mrPageData struct {
34	34	}
35	35
36	36	func renderMR(t *testing.T, m store.MR, reviews []store.MRReview, checks []store.Check) string {
	37	rows := make([]reviewRow, 0, len(reviews))
	38	for _, r := range reviews {
	39	// The page test renders reviews that count; whether a given
	40	// reviewer's does is decided by access, which e2e covers.
	41	rows = append(rows, reviewRow{MRReview: r, Counts: true})
	42	}
37	43	t.Helper()
38	44	var sb strings.Builder
39	45	if err := web.Render(&sb, "mr.html", mrPageData{
40	46	repoPage: testRepoPage(), MR: m, View: "conversation",
41		Reviews: reviews, Checks: checks, Combined: "",
	47	Reviews: rows, Checks: checks, Combined: "",
42	48	}); err != nil {
43	49	t.Fatalf("render: %v", err)
44	50	}

internal/httpd/web.go +17 −2

		@@ -1746,6 +1746,13 @@ func (s Server) mr(w http.ResponseWriter, r http.Request) {
1746	1746	}
1747	1747	comments, _ := s.st.ListMRComments(m.ID)
1748	1748	reviews, _ := s.st.ListMRReviews(m.ID)
	1749	// The same rule the merge gates apply, so the page cannot show an
	1750	// approval the gate ignores (#147).
	1751	reviewCounts := control.ReviewersWhoCount(s.st, p.Repo, reviews)
	1752	reviewRows := make([]reviewRow, 0, len(reviews))
	1753	for _, r := range reviews {
	1754	reviewRows = append(reviewRows, reviewRow{MRReview: r, Counts: reviewCounts[r.Reviewer]})
	1755	}
1749	1756	checks, combined, _ := s.st.ChecksForCommit(p.Repo.ID, m.HeadSHA)
1750	1757	// The viewer sees their own unsubmitted review comments and nobody
1751	1758	// else's.
		@@ -1833,7 +1840,7 @@ func (s Server) mr(w http.ResponseWriter, r http.Request) {
1833	1840	Checks []store.Check
1834	1841	Combined string
1835	1842	Comments []renderedComment
1836		Reviews []store.MRReview
	1843	Reviews []reviewRow
1837	1844	DiffFiles []diffFile
1838	1845	DiffTruncated bool
1839	1846	Stat diffStat
		@@ -1849,7 +1856,7 @@ func (s Server) mr(w http.ResponseWriter, r http.Request) {
1849	1856	StackedOn *store.MR
1850	1857	Stacked []store.MR
1851	1858	}{p, m, view, md(m.Body, m.BodyFormat), checks, combined, renderComments(comments, md),
1852		reviews, files, diffTruncated, stat, commits, commitsTotal, branches, s.canEditItem(r, p.Repo, m.Author),
	1859	reviewRows, files, diffTruncated, stat, commits, commitsTotal, branches, s.canEditItem(r, p.Repo, m.Author),
1853	1860	canWrite, unresolved, revisions, s.takeFlash(w, r), detachedThreads, stackedOn, stacked})
1854	1861	}
1855	1862
		@@ -1895,3 +1902,11 @@ func policyCanAdmin(u store.User, repo store.Repo, grant string) bool {
1895	1902	func policyCanRead(u store.User, repo store.Repo, grant string) bool {
1896	1903	return policy.CanRead(u, repo, grant)
1897	1904	}
	1905
	1906	// reviewRow is a review with whether the merge gates count it, which
	1907	// depends on the reviewer's access and so is not a property of the
	1908	// review row itself.
	1909	type reviewRow struct {
	1910	store.MRReview
	1911	Counts bool
	1912	}

internal/web/templates/mr.html +1 −1

		@@ -128,7 +128,7 @@
128	128	{{end}}
129	129	<div class="grp">
130	130	<h2>Reviews</h2>
131		{{range .Reviews}}<p class="row"><span class="dot {{if eq .Verdict "approve"}}ok{{else}}pend{{end}}"></span><a href="/{{.Reviewer}}">{{.Reviewer}}</a> {{.Verdict}}{{if .Stale}} <span class="chip chip-stale">stale</span>{{end}}<span class="sub">{{when .CreatedAt}}</span></p>
	131	{{range .Reviews}}<p class="row"><span class="dot {{if eq .Verdict "approve"}}ok{{else}}pend{{end}}"></span><a href="/{{.Reviewer}}">{{.Reviewer}}</a> {{.Verdict}}{{if .Stale}} <span class="chip chip-stale">stale</span>{{end}}{{if not .Counts}} <span class="chip chip-neutral" title="This reviewer has no write access, so the merge gates do not count it">advisory</span>{{end}}<span class="sub">{{when .CreatedAt}}</span></p>
132	132	{{else}}<p class="none">No reviews yet</p>{{end}}
133	133	{{if gt (len .Revisions) 1}}<p class="row none">{{len .Revisions}} revisions pushed. What changed between the last two:
134	134	<code>gitbay mr range-diff {{.Repo.OwnerName}}/{{.Repo.Name}} {{.MR.Number}}</code></p>{{end}}

Commit f84bca7807

Commit `f84bca7807`