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

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/12293
Reviewed-by: 0ko <0ko@noreply.codeberg.org>

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/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