From 733a390ecd1609a72f4123c9ab5101b85f3437e9 Mon Sep 17 00:00:00 2001 From: Mathieu Fenniak Date: Wed, 29 Apr 2026 05:26:22 +0200 Subject: [PATCH] fix: verify PR author has write access to head to support allow maintainers edit (#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/12292 Reviewed-by: 0ko <0ko@noreply.codeberg.org> --- models/issues/pull_list.go | 47 +++++++++++- release-notes/12293.md | 1 + release-notes/12294.md | 1 + release-notes/12295.md | 1 + services/pull/pull.go | 4 +- tests/integration/pull_update_test.go | 102 ++++++++++++++++++++++++++ 6 files changed, 153 insertions(+), 3 deletions(-) create mode 100644 release-notes/12293.md create mode 100644 release-notes/12294.md create mode 100644 release-notes/12295.md diff --git a/models/issues/pull_list.go b/models/issues/pull_list.go index 861ccace81..10e39c6a49 100644 --- a/models/issues/pull_list.go +++ b/models/issues/pull_list.go @@ -73,7 +73,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"). @@ -92,18 +92,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) { @@ -250,6 +276,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) { diff --git a/release-notes/12293.md b/release-notes/12293.md new file mode 100644 index 0000000000..0052eced48 --- /dev/null +++ b/release-notes/12293.md @@ -0,0 +1 @@ +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. diff --git a/release-notes/12294.md b/release-notes/12294.md new file mode 100644 index 0000000000..0052eced48 --- /dev/null +++ b/release-notes/12294.md @@ -0,0 +1 @@ +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. diff --git a/release-notes/12295.md b/release-notes/12295.md new file mode 100644 index 0000000000..0052eced48 --- /dev/null +++ b/release-notes/12295.md @@ -0,0 +1 @@ +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. diff --git a/services/pull/pull.go b/services/pull/pull.go index 617d5d0806..7c078a0058 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -534,7 +534,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 } @@ -564,7 +564,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 } diff --git a/tests/integration/pull_update_test.go b/tests/integration/pull_update_test.go index 42f9e841e1..d013dfeb95 100644 --- a/tests/integration/pull_update_test.go +++ b/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