[v14.0/forgejo] fix: verify PR author has write access to head to support allow maintainers edit (#12294)

Backport: https://codeberg.org/forgejo/forgejo/pulls/12292

When a pull request is opened, the author is able to mark that pull request to "Allow edits from maintainers", which grants the maintainers of the pull request's repo access to edit the pull request branch contents.  It is possible to create a pull request where the pull request author does not have the ability to edit the pull request branch.  Due to a missing security check for this case, maintainers of the pull request repo would be granted the ability to edit the pull request branch, even if the author of the pull request did not have that ability.  By exploiting this missing security check, a user can edit any branch in a repository if they're able to fork that repository.  The issue is being fixed by restricting the scope of "Allow edits from maintainers" to only grant that access if the pull request author also had access to edit the branch.

Thanks to Arvin Shivram of Brutecat Security for discovering and responsibly disclosing the vulnerability.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12294
Reviewed-by: 0ko <0ko@noreply.codeberg.org>

models/db/context.go

@@ -6,6 +6,8 @@ package db
 import (
 	"context"
 	"database/sql"
+	"fmt"
+	"slices"
 
 	"xorm.io/builder"
 	"xorm.io/xorm"
@@ -268,6 +270,48 @@ func GetByID[T any](ctx context.Context, id int64) (object *T, exist bool, err e
 	return &bean, true, nil
 }
 
+// Retrieves multiple objects with database queries similar to an xorm `.In(idField, idList)`. idField must be a unique
+// field on the database table, as a map[id]obj is returned and the usage of a non-unique field would result in objects
+// being overwritten in the map.
+//
+// The length of the IN list is constrained to DefaultMaxInSize for each database query, resulting in multiple database
+// queries if the length of the idList exceeds that setting; this constraint prevents exceeding bind parameter
+// limitations or query length limitations in the database engine.
+func GetByIDs[Bean any, Id comparable](ctx context.Context, idField string, idList []Id, bean *Bean) (map[Id]*Bean, error) {
+	retval := make(map[Id]*Bean, len(idList))
+	if len(idList) == 0 {
+		return retval, nil
+	}
+
+	table, err := TableInfo(bean)
+	if err != nil {
+		return nil, fmt.Errorf("unable to fetch table info for bean %v: %w", bean, err)
+	}
+
+	var structFieldName string
+	for _, c := range table.Columns() {
+		if c.Name == idField {
+			structFieldName = c.FieldName
+			break
+		}
+	}
+	if structFieldName == "" {
+		return nil, fmt.Errorf("unable to identify struct field for id field %s", idField)
+	}
+
+	for idChunk := range slices.Chunk(idList, DefaultMaxInSize) {
+		beans := make([]*Bean, 0, len(idChunk))
+		if err := GetEngine(ctx).In(idField, idChunk).Find(&beans); err != nil {
+			return nil, err
+		}
+		for _, bean := range beans {
+			retval[extractFieldValue(bean, structFieldName).(Id)] = bean
+		}
+	}
+
+	return retval, nil
+}
+
 func Exist[T any](ctx context.Context, cond builder.Cond) (bool, error) {
 	if !cond.IsValid() {
 		panic("cond is invalid in db.Exist(ctx, cond). This should not be possible.")

models/issues/pull_list.go

@@ -63,7 +63,7 @@ func GetUnmergedPullRequestsByHeadInfoMax(ctx context.Context, repoID, olderThan
 }
 
 // GetUnmergedPullRequestsByHeadInfo returns all pull requests that are open and has not been merged
-func GetUnmergedPullRequestsByHeadInfo(ctx context.Context, repoID int64, branch string) ([]*PullRequest, error) {
+func GetUnmergedPullRequestsByHeadInfo(ctx context.Context, repoID int64, branch string) (PullRequestList, error) {
 	prs := make([]*PullRequest, 0, 2)
 	sess := db.GetEngine(ctx).
 		Join("INNER", "issue", "issue.id = pull_request.issue_id").
@@ -82,18 +82,44 @@ func CanMaintainerWriteToBranch(ctx context.Context, p access_model.Permission,
 	}
 
 	prs, err := GetUnmergedPullRequestsByHeadInfo(ctx, p.Units[0].RepoID, branch)
+	// All these error cases return `false` to defer to the safer choice of not allowing write access on an error.
 	if err != nil {
+		log.Error("GetUnmergedPullRequestsByHeadInfo failed: %s", err)
+		return false
+	} else if issues, err := prs.LoadIssues(ctx); err != nil {
+		log.Error("LoadIssues failed: %s", err)
+		return false
+	} else if err := issues.LoadPosters(ctx); err != nil {
+		log.Error("LoadPosters failed: %s", err)
+		return false
+	} else if err := prs.LoadHeadRepos(ctx); err != nil {
+		log.Error("LoadHeadRepos failed: %s", err)
 		return false
 	}
 
 	for _, pr := range prs {
 		if pr.AllowMaintainerEdit {
+			// PR Poster must have write access to the head, so that when they turned on "AllowMaintainerEdit" they
+			// delegated that write access to the maintainers of the PR base.  If they don't currently have write
+			// access, they can't delegate that access.
+			poster := pr.Issue.Poster
+			posterHeadPerm, err := access_model.GetUserRepoPermission(ctx, pr.HeadRepo, poster)
+			if err != nil {
+				log.Error("GetUserRepoPermission failed: %s", err)
+				continue
+			}
+			if !posterHeadPerm.CanWrite(unit.TypeCode) {
+				continue
+			}
+
 			err = pr.LoadBaseRepo(ctx)
 			if err != nil {
+				log.Error("LoadBaseRepo failed: %s", err)
 				continue
 			}
 			prPerm, err := access_model.GetUserRepoPermission(ctx, pr.BaseRepo, user)
 			if err != nil {
+				log.Error("GetUserRepoPermission failed: %s", err)
 				continue
 			}
 			if prPerm.CanWrite(unit.TypeCode) {
@@ -240,6 +266,25 @@ func (prs PullRequestList) LoadIssues(ctx context.Context) (IssueList, error) {
 	return issueList, nil
 }
 
+func (prs PullRequestList) LoadHeadRepos(ctx context.Context) error {
+	repoIDs := []int64{}
+	for _, pr := range prs {
+		repoIDs = append(repoIDs, pr.HeadRepoID)
+	}
+	repos, err := db.GetByIDs(ctx, "id", repoIDs, &repo_model.Repository{})
+	if err != nil {
+		return err
+	}
+	for _, pr := range prs {
+		repo, ok := repos[pr.HeadRepoID]
+		if !ok {
+			return fmt.Errorf("unable to find repo %d", pr.HeadRepoID)
+		}
+		pr.HeadRepo = repo
+	}
+	return nil
+}
+
 // GetIssueIDs returns all issue ids
 func (prs PullRequestList) GetIssueIDs() []int64 {
 	return container.FilterSlice(prs, func(pr *PullRequest) (int64, bool) {

services/pull/pull.go

@@ -539,7 +539,7 @@ func CloseBranchPulls(ctx context.Context, doer *user_model.User, repoID int64,
 	}
 
 	prs = append(prs, prs2...)
-	if err := issues_model.PullRequestList(prs).LoadAttributes(ctx); err != nil {
+	if err := prs.LoadAttributes(ctx); err != nil {
 		return err
 	}
 
@@ -569,7 +569,7 @@ func CloseRepoBranchesPulls(ctx context.Context, doer *user_model.User, repo *re
 			return err
 		}
 
-		if err = issues_model.PullRequestList(prs).LoadAttributes(ctx); err != nil {
+		if err = prs.LoadAttributes(ctx); err != nil {
 			return err
 		}
 

tests/integration/db_query_test.go

@@ -0,0 +1,47 @@
+// Copyright 2026 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: GPL-3.0-or-later
+
+package integration
+
+import (
+	"fmt"
+	"testing"
+
+	actions_model "forgejo.org/models/actions"
+	"forgejo.org/models/db"
+	"forgejo.org/tests"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// These are basically unit tests, but by running them in the integration test suite they are tested against all
+// supported database types.
+
+func TestDatabaseDefaultMaxInSize(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+
+	// Ensure there are more than db.DefaultMaxInSize objects in a table:
+	targetCount := db.DefaultMaxInSize * 2
+	for i := range targetCount {
+		_, err := actions_model.InsertVariable(t.Context(), 2, 2, fmt.Sprintf("VAR_%d", i), fmt.Sprintf("Value %d", i))
+		require.NoError(t, err)
+	}
+
+	t.Run("GetByIDs", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+
+		allActionVariables := make([]*actions_model.ActionVariable, 0, targetCount)
+		err := db.GetEngine(t.Context()).Find(&allActionVariables)
+		require.NoError(t, err)
+
+		allIDs := make([]int64, len(allActionVariables))
+		for i := range allActionVariables {
+			allIDs[i] = allActionVariables[i].ID
+		}
+
+		allActionVariablesAgain, err := db.GetByIDs(t.Context(), "id", allIDs, &actions_model.ActionVariable{})
+		require.NoError(t, err)
+		assert.Len(t, allActionVariablesAgain, len(allActionVariables))
+	})
+}

tests/integration/pull_update_test.go

@@ -4,6 +4,7 @@
 package integration
 
 import (
+	"encoding/base64"
 	"fmt"
 	"net/http"
 	"net/url"
@@ -130,6 +131,107 @@ func TestAPIPullUpdateBranchProtection(t *testing.T) {
 	})
 }
 
+func TestAPIPullAllowMaintainerEditRestrictedHead(t *testing.T) {
+	onApplicationRun(t, func(t *testing.T, giteaURL *url.URL) {
+		baseRepoOwner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4})
+
+		realBaseRepo, _, cleanup := tests.CreateDeclarativeRepo(t, baseRepoOwner, "base-repo", nil, nil, nil)
+		defer cleanup()
+
+		forkUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+		forkRepo, err := repo_service.ForkRepositoryAndUpdates(t.Context(), forkUser, forkUser, repo_service.ForkRepoOptions{
+			BaseRepo:    realBaseRepo,
+			Name:        "repo-pr-update",
+			Description: "desc",
+		})
+		require.NoError(t, err)
+		assert.NotNil(t, forkRepo)
+
+		_, err = files_service.ChangeRepoFiles(git.DefaultContext, forkRepo, forkUser, &files_service.ChangeRepoFilesOptions{
+			Files: []*files_service.ChangeRepoFile{
+				{
+					Operation:     "create",
+					TreePath:      "File_B",
+					ContentReader: strings.NewReader("File B"),
+				},
+			},
+			Message:   "Add File on PR branch",
+			OldBranch: "main",
+			NewBranch: "main",
+			Author: &files_service.IdentityOptions{
+				Name:  forkUser.Name,
+				Email: forkUser.Email,
+			},
+			Committer: &files_service.IdentityOptions{
+				Name:  forkUser.Name,
+				Email: forkUser.Email,
+			},
+			Dates: &files_service.CommitDateOptions{
+				Author:    time.Now(),
+				Committer: time.Now(),
+			},
+		})
+		require.NoError(t, err)
+
+		// Create a pull request that is unexpectedly backwards -- normally we'd request a branch from the fork to be
+		// merged into the original base repo. But there's nothing preventing us from creating a pull request for the
+		// base to be pulled into the fork:
+
+		pullIssue := &issues_model.Issue{
+			RepoID:   forkRepo.ID,
+			Title:    "Pull Base into Fork",
+			PosterID: forkUser.ID,
+			Poster:   forkUser,
+			IsPull:   true,
+		}
+		pullRequest := &issues_model.PullRequest{
+			HeadRepoID:          realBaseRepo.ID,
+			BaseRepoID:          forkRepo.ID,
+			HeadBranch:          "main",
+			BaseBranch:          "main",
+			HeadRepo:            realBaseRepo,
+			BaseRepo:            forkRepo,
+			Type:                issues_model.PullRequestGitea,
+			AllowMaintainerEdit: true,
+		}
+		err = pull_service.NewPullRequest(git.DefaultContext, forkRepo, pullIssue, nil, nil, pullRequest, nil)
+		require.NoError(t, err)
+
+		// forkUser is the owner of forkRepo, and therefore a maintainer of forkRepo.  `AllowMaintainerEdit` should
+		// allow forkUser to edit the head of the PR... except that, in this case, the owner of the PR delegated that
+		// edit access but they never had that edit access.  Try to modify the head repo & branch to see.
+		session := loginUser(t, forkUser.LoginName)
+		token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository)
+
+		// Attempt modification by editing the realBase's main branch via API:
+		req := NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/repos/%s/%s/contents/File_A", realBaseRepo.OwnerName, realBaseRepo.Name),
+			&api.CreateFileOptions{
+				FileOptions: api.FileOptions{
+					BranchName:    "main",
+					NewBranchName: "main",
+					Message:       "illegal change",
+					Author: api.Identity{
+						Name:  forkUser.FullName,
+						Email: forkUser.Email,
+					},
+					Committer: api.Identity{
+						Name:  forkUser.FullName,
+						Email: forkUser.Email,
+					},
+				},
+				ContentBase64: base64.StdEncoding.EncodeToString([]byte("Some content.")),
+			}).
+			AddTokenAuth(token)
+		MakeRequest(t, req, http.StatusForbidden)
+
+		// Modify by "updating" the PR, which would pull the base into the head, even though we don't have access to
+		// write the head:
+		req = NewRequestf(t, "POST", "/api/v1/repos/%s/%s/pulls/%d/update", forkRepo.OwnerName, forkRepo.Name, pullIssue.Index).
+			AddTokenAuth(token)
+		MakeRequest(t, req, http.StatusInternalServerError) // probably should be Forbidden
+	})
+}
+
 func TestAPIViewUpdateSettings(t *testing.T) {
 	onApplicationRun(t, func(t *testing.T, giteaURL *url.URL) {
 		// Create PR to test