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.
This commit is contained in:
Gusted
Earl Warren
parent
00379db370
commit
266e0b2ce9
|
|
@ -1979,6 +1979,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)
|
||||
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
@ -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"
|
||||
|
|
@ -1034,17 +1035,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
|
||||
|
|
@ -1056,27 +1046,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)
|
||||
|
|
|
|||
|
|
@ -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())
|
||||
|
|
|
|||
|
|
@ -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
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
})
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
})
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue