From 618eb8e72ab6521694453639abac1cedcf9c4f6f Mon Sep 17 00:00:00 2001 From: Gusted Date: Wed, 23 Oct 2024 00:48:46 +0200 Subject: [PATCH] security: add permission check to 'delete branch after merge' - Add a permission check that the doer has write permissions to the head repository if the the 'delete branch after merge' is enabled when merging a pull request. - Unify the checks in the web and API router to `DeleteBranchAfterMerge`. - Added integration tests. (cherry picked from commit 266e0b2ce91ebd18858071d7702589971384402c) --- options/locale/locale_en-US.ini | 4 +++ release-notes/5718.md | 1 + routers/api/v1/repo/pull.go | 33 ++++++----------------- routers/web/repo/pull.go | 29 ++++++++++++--------- services/repository/branch.go | 39 ++++++++++++++++++++++++++++ tests/integration/api_pull_test.go | 38 +++++++++++++++++++++++++++ tests/integration/pull_merge_test.go | 32 +++++++++++++++++++++++ 7 files changed, 139 insertions(+), 37 deletions(-) create mode 100644 release-notes/5718.md diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 389d36a2a6..661dafd235 100644 --- a/options/locale/locale_en-US.ini +++ b/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) diff --git a/release-notes/5718.md b/release-notes/5718.md new file mode 100644 index 0000000000..f44178d1fe --- /dev/null +++ b/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. diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index bcb6ef2581..f491fd2735 100644 --- a/routers/api/v1/repo/pull.go +++ b/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) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index bc85012700..14b03b9fa4 100644 --- a/routers/web/repo/pull.go +++ b/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()) diff --git a/services/repository/branch.go b/services/repository/branch.go index f0e7120926..7d92053178 100644 --- a/services/repository/branch.go +++ b/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 } diff --git a/tests/integration/api_pull_test.go b/tests/integration/api_pull_test.go index 51c25fb75f..7b95d441dd 100644 --- a/tests/integration/api_pull_test.go +++ b/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) + }) +} diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 0f25bdebac..caf100144d 100644 --- a/tests/integration/pull_merge_test.go +++ b/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) + }) +}