From 8819eee69673c653d0d9970a0227bd680ee7345a Mon Sep 17 00:00:00 2001 From: Nicolas Date: Mon, 15 Jun 2026 22:08:10 +0200 Subject: [PATCH] derive maintainer edit --- routers/private/hook_pre_receive.go | 21 +++++++++++++-------- routers/private/hook_pre_receive_test.go | 16 ++++++++++------ 2 files changed, 23 insertions(+), 14 deletions(-) diff --git a/routers/private/hook_pre_receive.go b/routers/private/hook_pre_receive.go index b973603756..65dbcfa89a 100644 --- a/routers/private/hook_pre_receive.go +++ b/routers/private/hook_pre_receive.go @@ -51,20 +51,25 @@ type preReceiveContext struct { } // CanWriteCode returns true if pusher can write code -func (ctx *preReceiveContext) CanWriteCode(ref git.RefName) bool { +func (ctx *preReceiveContext) CanWriteCode(refFullName git.RefName) bool { if !ctx.loadPusherAndPermission() { return false } - // Must not be cached: CanMaintainerWriteToBranch is evaluated against ctx.branchName, which - // differs for each ref in a batch push. Caching the first result would let a per-branch - // maintainer-edit grant on one ref authorize writes to every other ref in the same push. - // FIXME: the ref can be a branch, a tag, or something else? - return issues_model.CanMaintainerWriteToBranch(ctx, ctx.userPerm, branchName, ctx.user) || ctx.deployKeyAccessMode >= perm_model.AccessModeWrite + // The maintainer-edit grant is scoped to a single PR head branch, so it must be evaluated + // against the exact branch being pushed and only for branch refs. Tags and other refs can + // never match a PR head branch, so they pass an empty name. Deriving this per ref (instead of + // from shared mutable state) prevents a per-branch grant from authorizing writes to other refs + // batched into the same push. + maintainerEditBranch := "" + if refFullName.IsBranch() { + maintainerEditBranch = refFullName.BranchName() + } + return issues_model.CanMaintainerWriteToBranch(ctx, ctx.userPerm, maintainerEditBranch, ctx.user) || ctx.deployKeyAccessMode >= perm_model.AccessModeWrite } // AssertCanWriteCode returns true if pusher can write code -func (ctx *preReceiveContext) AssertCanWriteCode(ref git.RefName) bool { - if !ctx.CanWriteCode(ref) { +func (ctx *preReceiveContext) AssertCanWriteCode(refFullName git.RefName) bool { + if !ctx.CanWriteCode(refFullName) { if ctx.Written() { return false } diff --git a/routers/private/hook_pre_receive_test.go b/routers/private/hook_pre_receive_test.go index 2ee64f2e40..fb2401fba4 100644 --- a/routers/private/hook_pre_receive_test.go +++ b/routers/private/hook_pre_receive_test.go @@ -10,6 +10,7 @@ import ( "gitea.dev/models/perm/access" repo_model "gitea.dev/models/repo" "gitea.dev/models/unittest" + "gitea.dev/modules/git" "gitea.dev/services/contexttest" "github.com/stretchr/testify/assert" @@ -17,9 +18,9 @@ import ( ) // TestPreReceiveCanWriteCodePerBranch ensures the maintainer-edit write grant is evaluated against -// the current branch on every call, instead of being cached from the first ref of a batch push. +// the exact ref being pushed on every call, derived from that ref rather than shared mutable state. // Otherwise a per-branch grant (an open PR with "allow edits from maintainers") could be batched -// together with a protected branch to escalate into full repository write. +// together with a protected branch or a tag to escalate into full repository write. func TestPreReceiveCanWriteCodePerBranch(t *testing.T) { require.NoError(t, unittest.PrepareTestDatabase()) @@ -57,10 +58,13 @@ func TestPreReceiveCanWriteCodePerBranch(t *testing.T) { } // The granted branch must be writable... - ctx.branchName = "granted-branch" - assert.True(t, ctx.CanWriteCode()) + assert.True(t, ctx.CanWriteCode(git.RefNameFromBranch("granted-branch"))) // ...but another branch in the same push must NOT inherit that grant. - ctx.branchName = "master" - assert.False(t, ctx.CanWriteCode()) + assert.False(t, ctx.CanWriteCode(git.RefNameFromBranch("master"))) + + // ...and a tag sharing the granted branch's name must NOT inherit it either: the grant is + // scoped to PR head branches, so a non-branch ref can never match it. (A tag ref already + // yields an empty branch name, so this guards the per-ref evaluation, not the IsBranch check.) + assert.False(t, ctx.CanWriteCode(git.RefNameFromTag("granted-branch"))) }