0
Fork 0
mirror of https://codeberg.org/forgejo/forgejo.git synced 2024-12-22 23:33:15 -05:00

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 266e0b2ce9)
This commit is contained in:
Gusted 2024-10-23 00:48:46 +02:00 committed by forgejo-backport-action
parent d77096071d
commit 618eb8e72a
7 changed files with 139 additions and 37 deletions

View file

@ -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_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.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.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) 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)

1
release-notes/5718.md Normal file
View file

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

View file

@ -28,6 +28,7 @@ import (
"code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/setting"
api "code.gitea.io/gitea/modules/structs" api "code.gitea.io/gitea/modules/structs"
"code.gitea.io/gitea/modules/timeutil" "code.gitea.io/gitea/modules/timeutil"
"code.gitea.io/gitea/modules/util"
"code.gitea.io/gitea/modules/web" "code.gitea.io/gitea/modules/web"
"code.gitea.io/gitea/routers/api/v1/utils" "code.gitea.io/gitea/routers/api/v1/utils"
asymkey_service "code.gitea.io/gitea/services/asymkey" 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) log.Trace("Pull request merged: %d", pr.ID)
if form.DeleteBranchAfterMerge { 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 var headRepo *git.Repository
if ctx.Repo != nil && ctx.Repo.Repository != nil && ctx.Repo.Repository.ID == pr.HeadRepoID && ctx.Repo.GitRepo != nil { if ctx.Repo != nil && ctx.Repo.Repository != nil && ctx.Repo.Repository.ID == pr.HeadRepoID && ctx.Repo.GitRepo != nil {
headRepo = ctx.Repo.GitRepo headRepo = ctx.Repo.GitRepo
@ -1032,27 +1022,20 @@ func MergePullRequest(ctx *context.APIContext) {
} }
defer headRepo.Close() defer headRepo.Close()
} }
if err := pull_service.RetargetChildrenOnMerge(ctx, ctx.Doer, pr); err != nil {
ctx.Error(http.StatusInternalServerError, "RetargetChildrenOnMerge", err) if err := repo_service.DeleteBranchAfterMerge(ctx, ctx.Doer, pr, headRepo); err != nil {
return
}
if err := repo_service.DeleteBranch(ctx, ctx.Doer, pr.HeadRepo, headRepo, pr.HeadBranch); err != nil {
switch { switch {
case git.IsErrBranchNotExist(err):
ctx.NotFound(err)
case errors.Is(err, repo_service.ErrBranchIsDefault): 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): 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: default:
ctx.Error(http.StatusInternalServerError, "DeleteBranch", err) ctx.Error(http.StatusInternalServerError, "DeleteBranchAfterMerge", err)
} }
return 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) ctx.Status(http.StatusOK)

View file

@ -1389,21 +1389,11 @@ func MergePullRequest(ctx *context.Context) {
log.Trace("Pull request merged: %d", pr.ID) log.Trace("Pull request merged: %d", pr.ID)
if form.DeleteBranchAfterMerge { 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 var headRepo *git.Repository
if ctx.Repo != nil && ctx.Repo.Repository != nil && pr.HeadRepoID == ctx.Repo.Repository.ID && ctx.Repo.GitRepo != nil { if ctx.Repo != nil && ctx.Repo.Repository != nil && pr.HeadRepoID == ctx.Repo.Repository.ID && ctx.Repo.GitRepo != nil {
headRepo = ctx.Repo.GitRepo headRepo = ctx.Repo.GitRepo
} else { } else {
var err error
headRepo, err = gitrepo.OpenRepository(ctx, pr.HeadRepo) headRepo, err = gitrepo.OpenRepository(ctx, pr.HeadRepo)
if err != nil { if err != nil {
ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.FullName()), err) ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.FullName()), err)
@ -1411,7 +1401,22 @@ func MergePullRequest(ctx *context.Context) {
} }
defer headRepo.Close() 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()) ctx.JSONRedirect(issue.Link())

View file

@ -14,7 +14,9 @@ import (
"code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/db"
git_model "code.gitea.io/gitea/models/git" git_model "code.gitea.io/gitea/models/git"
issues_model "code.gitea.io/gitea/models/issues" issues_model "code.gitea.io/gitea/models/issues"
"code.gitea.io/gitea/models/perm/access"
repo_model "code.gitea.io/gitea/models/repo" repo_model "code.gitea.io/gitea/models/repo"
"code.gitea.io/gitea/models/unit"
user_model "code.gitea.io/gitea/models/user" user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/git"
"code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/gitrepo"
@ -24,8 +26,10 @@ import (
"code.gitea.io/gitea/modules/queue" "code.gitea.io/gitea/modules/queue"
repo_module "code.gitea.io/gitea/modules/repository" repo_module "code.gitea.io/gitea/modules/repository"
"code.gitea.io/gitea/modules/timeutil" "code.gitea.io/gitea/modules/timeutil"
"code.gitea.io/gitea/modules/util"
webhook_module "code.gitea.io/gitea/modules/webhook" webhook_module "code.gitea.io/gitea/modules/webhook"
notify_service "code.gitea.io/gitea/services/notify" 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" files_service "code.gitea.io/gitea/services/repository/files"
"xorm.io/builder" "xorm.io/builder"
@ -475,6 +479,41 @@ func DeleteBranch(ctx context.Context, doer *user_model.User, repo *repo_model.R
return nil 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 { type BranchSyncOptions struct {
RepoID int64 RepoID int64
} }

View file

@ -7,6 +7,8 @@ import (
"fmt" "fmt"
"io" "io"
"net/http" "net/http"
"net/url"
"strings"
"testing" "testing"
auth_model "code.gitea.io/gitea/models/auth" auth_model "code.gitea.io/gitea/models/auth"
@ -17,6 +19,7 @@ import (
user_model "code.gitea.io/gitea/models/user" user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/setting"
api "code.gitea.io/gitea/modules/structs" api "code.gitea.io/gitea/modules/structs"
"code.gitea.io/gitea/modules/test"
"code.gitea.io/gitea/services/forms" "code.gitea.io/gitea/services/forms"
issue_service "code.gitea.io/gitea/services/issue" issue_service "code.gitea.io/gitea/services/issue"
"code.gitea.io/gitea/tests" "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)
})
}

View file

@ -37,6 +37,7 @@ import (
"code.gitea.io/gitea/modules/test" "code.gitea.io/gitea/modules/test"
"code.gitea.io/gitea/modules/translation" "code.gitea.io/gitea/modules/translation"
"code.gitea.io/gitea/services/automerge" "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/forms"
"code.gitea.io/gitea/services/pull" "code.gitea.io/gitea/services/pull"
commitstatus_service "code.gitea.io/gitea/services/repository/commitstatus" 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}) 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)
})
}