Merge pull request '[v9.0/forgejo] add permission check to 'delete branch after merge'' (#5721) from bp-v9.0/forgejo-266e0b2 into v9.0/forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/5721
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>

options/locale/locale_en-US.ini

@@ -1972,6 +1972,10 @@ pulls.auto_merge_canceled_schedule = The auto merge was canceled for this pull r
 pulls.auto_merge_newly_scheduled_comment = `scheduled this pull request to auto merge when all checks succeed %[1]s`
 pulls.auto_merge_canceled_schedule_comment = `canceled auto merging this pull request when all checks succeed %[1]s`
 
+pulls.delete_after_merge.head_branch.is_default = The head branch you want to delete is the default branch and cannot be deleted.
+pulls.delete_after_merge.head_branch.is_protected = The head branch you want to delete is a protected branch and cannot be deleted.
+pulls.delete_after_merge.head_branch.insufficient_branch = You don't have permission to delete the head branch.
+
 pulls.delete.title = Delete this pull request?
 pulls.delete.text = Do you really want to delete this pull request? (This will permanently remove all content. Consider closing it instead, if you intend to keep it archived)
 

release-notes/5718.md

@@ -0,0 +1 @@
+Because of a missing permission check, the branch used to propose a pull request to a repository can always be deleted by the user performing the merge. It was fixed so that such a deletion is only allowed if the user performing the merge has write permission to the repository from which the pull request was made.

routers/api/v1/repo/pull.go

@@ -28,6 +28,7 @@ import (
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/modules/timeutil"
+	"code.gitea.io/gitea/modules/util"
 	"code.gitea.io/gitea/modules/web"
 	"code.gitea.io/gitea/routers/api/v1/utils"
 	asymkey_service "code.gitea.io/gitea/services/asymkey"
@@ -1010,17 +1011,6 @@ func MergePullRequest(ctx *context.APIContext) {
 	log.Trace("Pull request merged: %d", pr.ID)
 
 	if form.DeleteBranchAfterMerge {
-		// Don't cleanup when there are other PR's that use this branch as head branch.
-		exist, err := issues_model.HasUnmergedPullRequestsByHeadInfo(ctx, pr.HeadRepoID, pr.HeadBranch)
-		if err != nil {
-			ctx.ServerError("HasUnmergedPullRequestsByHeadInfo", err)
-			return
-		}
-		if exist {
-			ctx.Status(http.StatusOK)
-			return
-		}
-
 		var headRepo *git.Repository
 		if ctx.Repo != nil && ctx.Repo.Repository != nil && ctx.Repo.Repository.ID == pr.HeadRepoID && ctx.Repo.GitRepo != nil {
 			headRepo = ctx.Repo.GitRepo
@@ -1032,27 +1022,20 @@ func MergePullRequest(ctx *context.APIContext) {
 			}
 			defer headRepo.Close()
 		}
-		if err := pull_service.RetargetChildrenOnMerge(ctx, ctx.Doer, pr); err != nil {
-			ctx.Error(http.StatusInternalServerError, "RetargetChildrenOnMerge", err)
-			return
-		}
-		if err := repo_service.DeleteBranch(ctx, ctx.Doer, pr.HeadRepo, headRepo, pr.HeadBranch); err != nil {
+
+		if err := repo_service.DeleteBranchAfterMerge(ctx, ctx.Doer, pr, headRepo); err != nil {
 			switch {
-			case git.IsErrBranchNotExist(err):
-				ctx.NotFound(err)
 			case errors.Is(err, repo_service.ErrBranchIsDefault):
-				ctx.Error(http.StatusForbidden, "DefaultBranch", fmt.Errorf("can not delete default branch"))
+				ctx.Error(http.StatusForbidden, "DefaultBranch", fmt.Errorf("the head branch is the default branch"))
 			case errors.Is(err, git_model.ErrBranchIsProtected):
-				ctx.Error(http.StatusForbidden, "IsProtectedBranch", fmt.Errorf("branch protected"))
+				ctx.Error(http.StatusForbidden, "IsProtectedBranch", fmt.Errorf("the head branch is protected"))
+			case errors.Is(err, util.ErrPermissionDenied):
+				ctx.Error(http.StatusForbidden, "HeadBranch", fmt.Errorf("insufficient permission to delete head branch"))
 			default:
-				ctx.Error(http.StatusInternalServerError, "DeleteBranch", err)
+				ctx.Error(http.StatusInternalServerError, "DeleteBranchAfterMerge", err)
 			}
 			return
 		}
-		if err := issues_model.AddDeletePRBranchComment(ctx, ctx.Doer, pr.BaseRepo, pr.Issue.ID, pr.HeadBranch); err != nil {
-			// Do not fail here as branch has already been deleted
-			log.Error("DeleteBranch: %v", err)
-		}
 	}
 
 	ctx.Status(http.StatusOK)

routers/web/repo/pull.go

@@ -1389,21 +1389,11 @@ func MergePullRequest(ctx *context.Context) {
 	log.Trace("Pull request merged: %d", pr.ID)
 
 	if form.DeleteBranchAfterMerge {
-		// Don't cleanup when other pr use this branch as head branch
-		exist, err := issues_model.HasUnmergedPullRequestsByHeadInfo(ctx, pr.HeadRepoID, pr.HeadBranch)
-		if err != nil {
-			ctx.ServerError("HasUnmergedPullRequestsByHeadInfo", err)
-			return
-		}
-		if exist {
-			ctx.JSONRedirect(issue.Link())
-			return
-		}
-
 		var headRepo *git.Repository
 		if ctx.Repo != nil && ctx.Repo.Repository != nil && pr.HeadRepoID == ctx.Repo.Repository.ID && ctx.Repo.GitRepo != nil {
 			headRepo = ctx.Repo.GitRepo
 		} else {
+			var err error
 			headRepo, err = gitrepo.OpenRepository(ctx, pr.HeadRepo)
 			if err != nil {
 				ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.FullName()), err)
@@ -1411,7 +1401,22 @@ func MergePullRequest(ctx *context.Context) {
 			}
 			defer headRepo.Close()
 		}
-		deleteBranch(ctx, pr, headRepo)
+
+		if err := repo_service.DeleteBranchAfterMerge(ctx, ctx.Doer, pr, headRepo); err != nil {
+			switch {
+			case errors.Is(err, repo_service.ErrBranchIsDefault):
+				ctx.Flash.Error(ctx.Tr("repo.pulls.delete_after_merge.head_branch.is_default"))
+			case errors.Is(err, git_model.ErrBranchIsProtected):
+				ctx.Flash.Error(ctx.Tr("repo.pulls.delete_after_merge.head_branch.is_protected"))
+			case errors.Is(err, util.ErrPermissionDenied):
+				ctx.Flash.Error(ctx.Tr("repo.pulls.delete_after_merge.head_branch.insufficient_branch"))
+			default:
+				ctx.ServerError("DeleteBranchAfterMerge", err)
+			}
+
+			ctx.JSONRedirect(issue.Link())
+			return
+		}
 	}
 
 	ctx.JSONRedirect(issue.Link())

services/repository/branch.go

@@ -14,7 +14,9 @@ import (
 	"code.gitea.io/gitea/models/db"
 	git_model "code.gitea.io/gitea/models/git"
 	issues_model "code.gitea.io/gitea/models/issues"
+	"code.gitea.io/gitea/models/perm/access"
 	repo_model "code.gitea.io/gitea/models/repo"
+	"code.gitea.io/gitea/models/unit"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/gitrepo"
@@ -24,8 +26,10 @@ import (
 	"code.gitea.io/gitea/modules/queue"
 	repo_module "code.gitea.io/gitea/modules/repository"
 	"code.gitea.io/gitea/modules/timeutil"
+	"code.gitea.io/gitea/modules/util"
 	webhook_module "code.gitea.io/gitea/modules/webhook"
 	notify_service "code.gitea.io/gitea/services/notify"
+	pull_service "code.gitea.io/gitea/services/pull"
 	files_service "code.gitea.io/gitea/services/repository/files"
 
 	"xorm.io/builder"
@@ -475,6 +479,41 @@ func DeleteBranch(ctx context.Context, doer *user_model.User, repo *repo_model.R
 	return nil
 }
 
+// DeleteBranchAfterMerge deletes the head branch after a PR was merged assiociated with the head branch.
+func DeleteBranchAfterMerge(ctx context.Context, doer *user_model.User, pr *issues_model.PullRequest, headRepo *git.Repository) error {
+	// Don't cleanup when there are other PR's that use this branch as head branch.
+	exist, err := issues_model.HasUnmergedPullRequestsByHeadInfo(ctx, pr.HeadRepoID, pr.HeadBranch)
+	if err != nil {
+		return err
+	}
+	if exist {
+		return nil
+	}
+
+	// Ensure the doer has write permissions to the head repository of the branch it wants to delete.
+	perm, err := access.GetUserRepoPermission(ctx, pr.HeadRepo, doer)
+	if err != nil {
+		return err
+	}
+	if !perm.CanWrite(unit.TypeCode) {
+		return util.NewPermissionDeniedErrorf("Must have write permission to the head repository")
+	}
+
+	if err := pull_service.RetargetChildrenOnMerge(ctx, doer, pr); err != nil {
+		return err
+	}
+	if err := DeleteBranch(ctx, doer, pr.HeadRepo, headRepo, pr.HeadBranch); err != nil {
+		return err
+	}
+
+	if err := issues_model.AddDeletePRBranchComment(ctx, doer, pr.BaseRepo, pr.Issue.ID, pr.HeadBranch); err != nil {
+		// Do not fail here as branch has already been deleted
+		log.Error("DeleteBranchAfterMerge: %v", err)
+	}
+
+	return nil
+}
+
 type BranchSyncOptions struct {
 	RepoID int64
 }

tests/integration/api_pull_test.go

@@ -7,6 +7,8 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"net/url"
+	"strings"
 	"testing"
 
 	auth_model "code.gitea.io/gitea/models/auth"
@@ -17,6 +19,7 @@ import (
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
+	"code.gitea.io/gitea/modules/test"
 	"code.gitea.io/gitea/services/forms"
 	issue_service "code.gitea.io/gitea/services/issue"
 	"code.gitea.io/gitea/tests"
@@ -309,3 +312,38 @@ func doAPIGetPullFiles(ctx APITestContext, pr *api.PullRequest, callback func(*t
 		}
 	}
 }
+
+func TestAPIPullDeleteBranchPerms(t *testing.T) {
+	onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
+		user2Session := loginUser(t, "user2")
+		user4Session := loginUser(t, "user4")
+		testRepoFork(t, user4Session, "user2", "repo1", "user4", "repo1")
+		testEditFileToNewBranch(t, user2Session, "user2", "repo1", "master", "base-pr", "README.md", "Hello, World\n(Edited - base PR)\n")
+
+		req := NewRequestWithValues(t, "POST", "/user4/repo1/compare/master...user2/repo1:base-pr", map[string]string{
+			"_csrf": GetCSRF(t, user4Session, "/user4/repo1/compare/master...user2/repo1:base-pr"),
+			"title": "Testing PR",
+		})
+		resp := user4Session.MakeRequest(t, req, http.StatusOK)
+		elem := strings.Split(test.RedirectURL(resp), "/")
+
+		token := getTokenForLoggedInUser(t, user4Session, auth_model.AccessTokenScopeWriteRepository)
+		req = NewRequestWithValues(t, "POST", "/api/v1/repos/user4/repo1/pulls/"+elem[4]+"/merge", map[string]string{
+			"do":                        "merge",
+			"delete_branch_after_merge": "on",
+		}).AddTokenAuth(token)
+		resp = user4Session.MakeRequest(t, req, http.StatusForbidden)
+
+		type userResponse struct {
+			Message string `json:"message"`
+		}
+		var bodyResp userResponse
+		DecodeJSON(t, resp, &bodyResp)
+
+		assert.EqualValues(t, "insufficient permission to delete head branch", bodyResp.Message)
+
+		// Check that the branch still exist.
+		req = NewRequest(t, "GET", "/api/v1/repos/user2/repo1/branches/base-pr").AddTokenAuth(token)
+		user4Session.MakeRequest(t, req, http.StatusOK)
+	})
+}

tests/integration/pull_merge_test.go

@@ -37,6 +37,7 @@ import (
 	"code.gitea.io/gitea/modules/test"
 	"code.gitea.io/gitea/modules/translation"
 	"code.gitea.io/gitea/services/automerge"
+	forgejo_context "code.gitea.io/gitea/services/context"
 	"code.gitea.io/gitea/services/forms"
 	"code.gitea.io/gitea/services/pull"
 	commitstatus_service "code.gitea.io/gitea/services/repository/commitstatus"
@@ -1150,3 +1151,34 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApprovalForAgitFlow(t *testing.
 		unittest.AssertNotExistsBean(t, &pull_model.AutoMerge{PullID: pr.ID})
 	})
 }
+
+func TestPullDeleteBranchPerms(t *testing.T) {
+	onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
+		user2Session := loginUser(t, "user2")
+		user4Session := loginUser(t, "user4")
+		testRepoFork(t, user4Session, "user2", "repo1", "user4", "repo1")
+		testEditFileToNewBranch(t, user2Session, "user2", "repo1", "master", "base-pr", "README.md", "Hello, World\n(Edited - base PR)\n")
+
+		req := NewRequestWithValues(t, "POST", "/user4/repo1/compare/master...user2/repo1:base-pr", map[string]string{
+			"_csrf": GetCSRF(t, user4Session, "/user4/repo1/compare/master...user2/repo1:base-pr"),
+			"title": "Testing PR",
+		})
+		resp := user4Session.MakeRequest(t, req, http.StatusOK)
+		elem := strings.Split(test.RedirectURL(resp), "/")
+
+		req = NewRequestWithValues(t, "POST", "/user4/repo1/pulls/"+elem[4]+"/merge", map[string]string{
+			"_csrf":                     GetCSRF(t, user4Session, "/user4/repo1/pulls/"+elem[4]),
+			"do":                        "merge",
+			"delete_branch_after_merge": "on",
+		})
+		user4Session.MakeRequest(t, req, http.StatusOK)
+
+		flashCookie := user4Session.GetCookie(forgejo_context.CookieNameFlash)
+		assert.NotNil(t, flashCookie)
+		assert.EqualValues(t, "error%3DYou%2Bdon%2527t%2Bhave%2Bpermission%2Bto%2Bdelete%2Bthe%2Bhead%2Bbranch.", flashCookie.Value)
+
+		// Check that the branch still exist.
+		req = NewRequest(t, "GET", "/user2/repo1/src/branch/base-pr")
+		user4Session.MakeRequest(t, req, http.StatusOK)
+	})
+}