Merge pull request 'add permission check to 'delete branch after merge'' (#5718) from earl-warren/forgejo:wip-delete-branch into forgejo

+4

options/locale/locale_en-US.ini

··· 1979 1979 pulls.auto_merge_newly_scheduled_comment = `scheduled this pull request to auto merge when all checks succeed %[1]s` 1980 1980 pulls.auto_merge_canceled_schedule_comment = `canceled auto merging this pull request when all checks succeed %[1]s` 1981 1981 1982 + pulls.delete_after_merge.head_branch.is_default = The head branch you want to delete is the default branch and cannot be deleted. 1983 + pulls.delete_after_merge.head_branch.is_protected = The head branch you want to delete is a protected branch and cannot be deleted. 1984 + pulls.delete_after_merge.head_branch.insufficient_branch = You don't have permission to delete the head branch. 1985 + 1982 1986 pulls.delete.title = Delete this pull request? 1983 1987 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) 1984 1988

+1

release-notes/5718.md

··· 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.

+8 -25

routers/api/v1/repo/pull.go

··· 28 28 "code.gitea.io/gitea/modules/setting" 29 29 api "code.gitea.io/gitea/modules/structs" 30 30 "code.gitea.io/gitea/modules/timeutil" 31 + "code.gitea.io/gitea/modules/util" 31 32 "code.gitea.io/gitea/modules/web" 32 33 "code.gitea.io/gitea/routers/api/v1/utils" 33 34 asymkey_service "code.gitea.io/gitea/services/asymkey" ··· 1034 1035 log.Trace("Pull request merged: %d", pr.ID) 1035 1036 1036 1037 if form.DeleteBranchAfterMerge { 1037 - // Don't cleanup when there are other PR's that use this branch as head branch. 1038 - exist, err := issues_model.HasUnmergedPullRequestsByHeadInfo(ctx, pr.HeadRepoID, pr.HeadBranch) 1039 - if err != nil { 1040 - ctx.ServerError("HasUnmergedPullRequestsByHeadInfo", err) 1041 - return 1042 - } 1043 - if exist { 1044 - ctx.Status(http.StatusOK) 1045 - return 1046 - } 1047 - 1048 1038 var headRepo *git.Repository 1049 1039 if ctx.Repo != nil && ctx.Repo.Repository != nil && ctx.Repo.Repository.ID == pr.HeadRepoID && ctx.Repo.GitRepo != nil { 1050 1040 headRepo = ctx.Repo.GitRepo ··· 1056 1046 } 1057 1047 defer headRepo.Close() 1058 1048 } 1059 - if err := pull_service.RetargetChildrenOnMerge(ctx, ctx.Doer, pr); err != nil { 1060 - ctx.Error(http.StatusInternalServerError, "RetargetChildrenOnMerge", err) 1061 - return 1062 - } 1063 - if err := repo_service.DeleteBranch(ctx, ctx.Doer, pr.HeadRepo, headRepo, pr.HeadBranch); err != nil { 1049 + 1050 + if err := repo_service.DeleteBranchAfterMerge(ctx, ctx.Doer, pr, headRepo); err != nil { 1064 1051 switch { 1065 - case git.IsErrBranchNotExist(err): 1066 - ctx.NotFound(err) 1067 1052 case errors.Is(err, repo_service.ErrBranchIsDefault): 1068 - ctx.Error(http.StatusForbidden, "DefaultBranch", fmt.Errorf("can not delete default branch")) 1053 + ctx.Error(http.StatusForbidden, "DefaultBranch", fmt.Errorf("the head branch is the default branch")) 1069 1054 case errors.Is(err, git_model.ErrBranchIsProtected): 1070 - ctx.Error(http.StatusForbidden, "IsProtectedBranch", fmt.Errorf("branch protected")) 1055 + ctx.Error(http.StatusForbidden, "IsProtectedBranch", fmt.Errorf("the head branch is protected")) 1056 + case errors.Is(err, util.ErrPermissionDenied): 1057 + ctx.Error(http.StatusForbidden, "HeadBranch", fmt.Errorf("insufficient permission to delete head branch")) 1071 1058 default: 1072 - ctx.Error(http.StatusInternalServerError, "DeleteBranch", err) 1059 + ctx.Error(http.StatusInternalServerError, "DeleteBranchAfterMerge", err) 1073 1060 } 1074 1061 return 1075 - } 1076 - if err := issues_model.AddDeletePRBranchComment(ctx, ctx.Doer, pr.BaseRepo, pr.Issue.ID, pr.HeadBranch); err != nil { 1077 - // Do not fail here as branch has already been deleted 1078 - log.Error("DeleteBranch: %v", err) 1079 1062 } 1080 1063 } 1081 1064

+17 -12

routers/web/repo/pull.go

··· 1389 1389 log.Trace("Pull request merged: %d", pr.ID) 1390 1390 1391 1391 if form.DeleteBranchAfterMerge { 1392 - // Don't cleanup when other pr use this branch as head branch 1393 - exist, err := issues_model.HasUnmergedPullRequestsByHeadInfo(ctx, pr.HeadRepoID, pr.HeadBranch) 1394 - if err != nil { 1395 - ctx.ServerError("HasUnmergedPullRequestsByHeadInfo", err) 1396 - return 1397 - } 1398 - if exist { 1399 - ctx.JSONRedirect(issue.Link()) 1400 - return 1401 - } 1402 - 1403 1392 var headRepo *git.Repository 1404 1393 if ctx.Repo != nil && ctx.Repo.Repository != nil && pr.HeadRepoID == ctx.Repo.Repository.ID && ctx.Repo.GitRepo != nil { 1405 1394 headRepo = ctx.Repo.GitRepo 1406 1395 } else { 1396 + var err error 1407 1397 headRepo, err = gitrepo.OpenRepository(ctx, pr.HeadRepo) 1408 1398 if err != nil { 1409 1399 ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.FullName()), err) ··· 1411 1401 } 1412 1402 defer headRepo.Close() 1413 1403 } 1414 - deleteBranch(ctx, pr, headRepo) 1404 + 1405 + if err := repo_service.DeleteBranchAfterMerge(ctx, ctx.Doer, pr, headRepo); err != nil { 1406 + switch { 1407 + case errors.Is(err, repo_service.ErrBranchIsDefault): 1408 + ctx.Flash.Error(ctx.Tr("repo.pulls.delete_after_merge.head_branch.is_default")) 1409 + case errors.Is(err, git_model.ErrBranchIsProtected): 1410 + ctx.Flash.Error(ctx.Tr("repo.pulls.delete_after_merge.head_branch.is_protected")) 1411 + case errors.Is(err, util.ErrPermissionDenied): 1412 + ctx.Flash.Error(ctx.Tr("repo.pulls.delete_after_merge.head_branch.insufficient_branch")) 1413 + default: 1414 + ctx.ServerError("DeleteBranchAfterMerge", err) 1415 + } 1416 + 1417 + ctx.JSONRedirect(issue.Link()) 1418 + return 1419 + } 1415 1420 } 1416 1421 1417 1422 ctx.JSONRedirect(issue.Link())

+39

services/repository/branch.go

··· 14 14 "code.gitea.io/gitea/models/db" 15 15 git_model "code.gitea.io/gitea/models/git" 16 16 issues_model "code.gitea.io/gitea/models/issues" 17 + "code.gitea.io/gitea/models/perm/access" 17 18 repo_model "code.gitea.io/gitea/models/repo" 19 + "code.gitea.io/gitea/models/unit" 18 20 user_model "code.gitea.io/gitea/models/user" 19 21 "code.gitea.io/gitea/modules/git" 20 22 "code.gitea.io/gitea/modules/gitrepo" ··· 24 26 "code.gitea.io/gitea/modules/queue" 25 27 repo_module "code.gitea.io/gitea/modules/repository" 26 28 "code.gitea.io/gitea/modules/timeutil" 29 + "code.gitea.io/gitea/modules/util" 27 30 webhook_module "code.gitea.io/gitea/modules/webhook" 28 31 notify_service "code.gitea.io/gitea/services/notify" 32 + pull_service "code.gitea.io/gitea/services/pull" 29 33 files_service "code.gitea.io/gitea/services/repository/files" 30 34 31 35 "xorm.io/builder" ··· 470 474 RepoName: repo.Name, 471 475 }); err != nil { 472 476 log.Error("Update: %v", err) 477 + } 478 + 479 + return nil 480 + } 481 + 482 + // DeleteBranchAfterMerge deletes the head branch after a PR was merged assiociated with the head branch. 483 + func DeleteBranchAfterMerge(ctx context.Context, doer *user_model.User, pr *issues_model.PullRequest, headRepo *git.Repository) error { 484 + // Don't cleanup when there are other PR's that use this branch as head branch. 485 + exist, err := issues_model.HasUnmergedPullRequestsByHeadInfo(ctx, pr.HeadRepoID, pr.HeadBranch) 486 + if err != nil { 487 + return err 488 + } 489 + if exist { 490 + return nil 491 + } 492 + 493 + // Ensure the doer has write permissions to the head repository of the branch it wants to delete. 494 + perm, err := access.GetUserRepoPermission(ctx, pr.HeadRepo, doer) 495 + if err != nil { 496 + return err 497 + } 498 + if !perm.CanWrite(unit.TypeCode) { 499 + return util.NewPermissionDeniedErrorf("Must have write permission to the head repository") 500 + } 501 + 502 + if err := pull_service.RetargetChildrenOnMerge(ctx, doer, pr); err != nil { 503 + return err 504 + } 505 + if err := DeleteBranch(ctx, doer, pr.HeadRepo, headRepo, pr.HeadBranch); err != nil { 506 + return err 507 + } 508 + 509 + if err := issues_model.AddDeletePRBranchComment(ctx, doer, pr.BaseRepo, pr.Issue.ID, pr.HeadBranch); err != nil { 510 + // Do not fail here as branch has already been deleted 511 + log.Error("DeleteBranchAfterMerge: %v", err) 473 512 } 474 513 475 514 return nil

+38

tests/integration/api_pull_test.go

··· 7 7 "fmt" 8 8 "io" 9 9 "net/http" 10 + "net/url" 11 + "strings" 10 12 "testing" 11 13 12 14 auth_model "code.gitea.io/gitea/models/auth" ··· 17 19 user_model "code.gitea.io/gitea/models/user" 18 20 "code.gitea.io/gitea/modules/setting" 19 21 api "code.gitea.io/gitea/modules/structs" 22 + "code.gitea.io/gitea/modules/test" 20 23 "code.gitea.io/gitea/services/forms" 21 24 issue_service "code.gitea.io/gitea/services/issue" 22 25 "code.gitea.io/gitea/tests" ··· 309 312 } 310 313 } 311 314 } 315 + 316 + func TestAPIPullDeleteBranchPerms(t *testing.T) { 317 + onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { 318 + user2Session := loginUser(t, "user2") 319 + user4Session := loginUser(t, "user4") 320 + testRepoFork(t, user4Session, "user2", "repo1", "user4", "repo1") 321 + testEditFileToNewBranch(t, user2Session, "user2", "repo1", "master", "base-pr", "README.md", "Hello, World\n(Edited - base PR)\n") 322 + 323 + req := NewRequestWithValues(t, "POST", "/user4/repo1/compare/master...user2/repo1:base-pr", map[string]string{ 324 + "_csrf": GetCSRF(t, user4Session, "/user4/repo1/compare/master...user2/repo1:base-pr"), 325 + "title": "Testing PR", 326 + }) 327 + resp := user4Session.MakeRequest(t, req, http.StatusOK) 328 + elem := strings.Split(test.RedirectURL(resp), "/") 329 + 330 + token := getTokenForLoggedInUser(t, user4Session, auth_model.AccessTokenScopeWriteRepository) 331 + req = NewRequestWithValues(t, "POST", "/api/v1/repos/user4/repo1/pulls/"+elem[4]+"/merge", map[string]string{ 332 + "do": "merge", 333 + "delete_branch_after_merge": "on", 334 + }).AddTokenAuth(token) 335 + resp = user4Session.MakeRequest(t, req, http.StatusForbidden) 336 + 337 + type userResponse struct { 338 + Message string `json:"message"` 339 + } 340 + var bodyResp userResponse 341 + DecodeJSON(t, resp, &bodyResp) 342 + 343 + assert.EqualValues(t, "insufficient permission to delete head branch", bodyResp.Message) 344 + 345 + // Check that the branch still exist. 346 + req = NewRequest(t, "GET", "/api/v1/repos/user2/repo1/branches/base-pr").AddTokenAuth(token) 347 + user4Session.MakeRequest(t, req, http.StatusOK) 348 + }) 349 + }

+32

tests/integration/pull_merge_test.go

··· 37 37 "code.gitea.io/gitea/modules/test" 38 38 "code.gitea.io/gitea/modules/translation" 39 39 "code.gitea.io/gitea/services/automerge" 40 + forgejo_context "code.gitea.io/gitea/services/context" 40 41 "code.gitea.io/gitea/services/forms" 41 42 "code.gitea.io/gitea/services/pull" 42 43 commitstatus_service "code.gitea.io/gitea/services/repository/commitstatus" ··· 1150 1151 unittest.AssertNotExistsBean(t, &pull_model.AutoMerge{PullID: pr.ID}) 1151 1152 }) 1152 1153 } 1154 + 1155 + func TestPullDeleteBranchPerms(t *testing.T) { 1156 + onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { 1157 + user2Session := loginUser(t, "user2") 1158 + user4Session := loginUser(t, "user4") 1159 + testRepoFork(t, user4Session, "user2", "repo1", "user4", "repo1") 1160 + testEditFileToNewBranch(t, user2Session, "user2", "repo1", "master", "base-pr", "README.md", "Hello, World\n(Edited - base PR)\n") 1161 + 1162 + req := NewRequestWithValues(t, "POST", "/user4/repo1/compare/master...user2/repo1:base-pr", map[string]string{ 1163 + "_csrf": GetCSRF(t, user4Session, "/user4/repo1/compare/master...user2/repo1:base-pr"), 1164 + "title": "Testing PR", 1165 + }) 1166 + resp := user4Session.MakeRequest(t, req, http.StatusOK) 1167 + elem := strings.Split(test.RedirectURL(resp), "/") 1168 + 1169 + req = NewRequestWithValues(t, "POST", "/user4/repo1/pulls/"+elem[4]+"/merge", map[string]string{ 1170 + "_csrf": GetCSRF(t, user4Session, "/user4/repo1/pulls/"+elem[4]), 1171 + "do": "merge", 1172 + "delete_branch_after_merge": "on", 1173 + }) 1174 + user4Session.MakeRequest(t, req, http.StatusOK) 1175 + 1176 + flashCookie := user4Session.GetCookie(forgejo_context.CookieNameFlash) 1177 + assert.NotNil(t, flashCookie) 1178 + assert.EqualValues(t, "error%3DYou%2Bdon%2527t%2Bhave%2Bpermission%2Bto%2Bdelete%2Bthe%2Bhead%2Bbranch.", flashCookie.Value) 1179 + 1180 + // Check that the branch still exist. 1181 + req = NewRequest(t, "GET", "/user2/repo1/src/branch/base-pr") 1182 + user4Session.MakeRequest(t, req, http.StatusOK) 1183 + }) 1184 + }

Configure Feed

Configure Feed