Skip to content

Commit

Permalink
Only allow admins to rename default/protected branches (#33276)
Browse files Browse the repository at this point in the history
Currently, anyone with write permissions to a repo are able to rename
default or protected branches.

This change follows
[GitHub's](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-branches-in-your-repository/renaming-a-branch)
design by only allowing repo/site admins to change these branches.
However, it also follows are current design for protected branches and
only allows admins to modify branch names == branch protection rule
names. Glob-based rules cannot be renamed by anyone (as was already the
case, but we now catch `ErrBranchIsProtected` which we previously did
not catch, throwing a 500).
  • Loading branch information
kemzeb authored Jan 15, 2025
1 parent 4b21a6c commit 2483a93
Show file tree
Hide file tree
Showing 7 changed files with 90 additions and 10 deletions.
6 changes: 6 additions & 0 deletions models/fixtures/access.yml
Original file line number Diff line number Diff line change
Expand Up @@ -171,3 +171,9 @@
user_id: 40
repo_id: 61
mode: 4

-
id: 30
user_id: 40
repo_id: 1
mode: 2
2 changes: 2 additions & 0 deletions options/locale/locale_en-US.ini
Original file line number Diff line number Diff line change
Expand Up @@ -2714,6 +2714,8 @@ branch.create_branch_operation = Create branch
branch.new_branch = Create new branch
branch.new_branch_from = Create new branch from "%s"
branch.renamed = Branch %s was renamed to %s.
branch.rename_default_or_protected_branch_error = Only admins can rename default or protected branches.
branch.rename_protected_branch_failed = This branch is protected by glob-based protection rules.
tag.create_tag = Create tag %s
tag.create_tag_operation = Create tag
Expand Down
10 changes: 9 additions & 1 deletion routers/api/v1/repo/branch.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"code.gitea.io/gitea/models/db"
git_model "code.gitea.io/gitea/models/git"
"code.gitea.io/gitea/models/organization"
repo_model "code.gitea.io/gitea/models/repo"
user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/git"
"code.gitea.io/gitea/modules/gitrepo"
Expand Down Expand Up @@ -443,7 +444,14 @@ func UpdateBranch(ctx *context.APIContext) {

msg, err := repo_service.RenameBranch(ctx, repo, ctx.Doer, ctx.Repo.GitRepo, oldName, opt.Name)
if err != nil {
ctx.Error(http.StatusInternalServerError, "RenameBranch", err)
switch {
case repo_model.IsErrUserDoesNotHaveAccessToRepo(err):
ctx.Error(http.StatusForbidden, "", "User must be a repo or site admin to rename default or protected branches.")
case errors.Is(err, git_model.ErrBranchIsProtected):
ctx.Error(http.StatusForbidden, "", "Branch is protected by glob-based protection rules.")
default:
ctx.Error(http.StatusInternalServerError, "RenameBranch", err)
}
return
}
if msg == "target_exist" {
Expand Down
8 changes: 8 additions & 0 deletions routers/web/repo/setting/protected_branch.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package setting

import (
"errors"
"fmt"
"net/http"
"net/url"
Expand All @@ -14,6 +15,7 @@ import (
"code.gitea.io/gitea/models/organization"
"code.gitea.io/gitea/models/perm"
access_model "code.gitea.io/gitea/models/perm/access"
repo_model "code.gitea.io/gitea/models/repo"
"code.gitea.io/gitea/modules/base"
"code.gitea.io/gitea/modules/templates"
"code.gitea.io/gitea/modules/web"
Expand Down Expand Up @@ -351,9 +353,15 @@ func RenameBranchPost(ctx *context.Context) {
msg, err := repository.RenameBranch(ctx, ctx.Repo.Repository, ctx.Doer, ctx.Repo.GitRepo, form.From, form.To)
if err != nil {
switch {
case repo_model.IsErrUserDoesNotHaveAccessToRepo(err):
ctx.Flash.Error(ctx.Tr("repo.branch.rename_default_or_protected_branch_error"))
ctx.Redirect(fmt.Sprintf("%s/branches", ctx.Repo.RepoLink))
case git_model.IsErrBranchAlreadyExists(err):
ctx.Flash.Error(ctx.Tr("repo.branch.branch_already_exists", form.To))
ctx.Redirect(fmt.Sprintf("%s/branches", ctx.Repo.RepoLink))
case errors.Is(err, git_model.ErrBranchIsProtected):
ctx.Flash.Error(ctx.Tr("repo.branch.rename_protected_branch_failed"))
ctx.Redirect(fmt.Sprintf("%s/branches", ctx.Repo.RepoLink))
default:
ctx.ServerError("RenameBranch", err)
}
Expand Down
23 changes: 23 additions & 0 deletions services/repository/branch.go
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,29 @@ func RenameBranch(ctx context.Context, repo *repo_model.Repository, doer *user_m
return "from_not_exist", nil
}

perm, err := access_model.GetUserRepoPermission(ctx, repo, doer)
if err != nil {
return "", err
}

isDefault := from == repo.DefaultBranch
if isDefault && !perm.IsAdmin() {
return "", repo_model.ErrUserDoesNotHaveAccessToRepo{
UserID: doer.ID,
RepoName: repo.LowerName,
}
}

// If from == rule name, admins are allowed to modify them.
if protectedBranch, err := git_model.GetProtectedBranchRuleByName(ctx, repo.ID, from); err != nil {
return "", err
} else if protectedBranch != nil && !perm.IsAdmin() {
return "", repo_model.ErrUserDoesNotHaveAccessToRepo{
UserID: doer.ID,
RepoName: repo.LowerName,
}
}

if err := git_model.RenameBranch(ctx, repo, from, to, func(ctx context.Context, isDefault bool) error {
err2 := gitRepo.RenameBranch(from, to)
if err2 != nil {
Expand Down
49 changes: 41 additions & 8 deletions tests/integration/api_branch_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -190,28 +190,61 @@ func testAPICreateBranch(t testing.TB, session *TestSession, user, repo, oldBran
func TestAPIUpdateBranch(t *testing.T) {
onGiteaRun(t, func(t *testing.T, _ *url.URL) {
t.Run("UpdateBranchWithEmptyRepo", func(t *testing.T) {
testAPIUpdateBranch(t, "user10", "repo6", "master", "test", http.StatusNotFound)
testAPIUpdateBranch(t, "user10", "user10", "repo6", "master", "test", http.StatusNotFound)
})
t.Run("UpdateBranchWithSameBranchNames", func(t *testing.T) {
resp := testAPIUpdateBranch(t, "user2", "repo1", "master", "master", http.StatusUnprocessableEntity)
resp := testAPIUpdateBranch(t, "user2", "user2", "repo1", "master", "master", http.StatusUnprocessableEntity)
assert.Contains(t, resp.Body.String(), "Cannot rename a branch using the same name or rename to a branch that already exists.")
})
t.Run("UpdateBranchThatAlreadyExists", func(t *testing.T) {
resp := testAPIUpdateBranch(t, "user2", "repo1", "master", "branch2", http.StatusUnprocessableEntity)
resp := testAPIUpdateBranch(t, "user2", "user2", "repo1", "master", "branch2", http.StatusUnprocessableEntity)
assert.Contains(t, resp.Body.String(), "Cannot rename a branch using the same name or rename to a branch that already exists.")
})
t.Run("UpdateBranchWithNonExistentBranch", func(t *testing.T) {
resp := testAPIUpdateBranch(t, "user2", "repo1", "i-dont-exist", "new-branch-name", http.StatusNotFound)
resp := testAPIUpdateBranch(t, "user2", "user2", "repo1", "i-dont-exist", "new-branch-name", http.StatusNotFound)
assert.Contains(t, resp.Body.String(), "Branch doesn't exist.")
})
t.Run("RenameBranchNormalScenario", func(t *testing.T) {
testAPIUpdateBranch(t, "user2", "repo1", "branch2", "new-branch-name", http.StatusNoContent)
t.Run("UpdateBranchWithNonAdminDoer", func(t *testing.T) {
// don't allow default branch renaming
resp := testAPIUpdateBranch(t, "user40", "user2", "repo1", "master", "new-branch-name", http.StatusForbidden)
assert.Contains(t, resp.Body.String(), "User must be a repo or site admin to rename default or protected branches.")

// don't allow protected branch renaming
token := getUserToken(t, "user2", auth_model.AccessTokenScopeWriteRepository)
req := NewRequestWithJSON(t, "POST", "/api/v1/repos/user2/repo1/branches", &api.CreateBranchRepoOption{
BranchName: "protected-branch",
}).AddTokenAuth(token)
MakeRequest(t, req, http.StatusCreated)
testAPICreateBranchProtection(t, "protected-branch", 1, http.StatusCreated)
resp = testAPIUpdateBranch(t, "user40", "user2", "repo1", "protected-branch", "new-branch-name", http.StatusForbidden)
assert.Contains(t, resp.Body.String(), "User must be a repo or site admin to rename default or protected branches.")
})
t.Run("UpdateBranchWithGlobedBasedProtectionRulesAndAdminAccess", func(t *testing.T) {
// don't allow branch that falls under glob-based protection rules to be renamed
token := getUserToken(t, "user2", auth_model.AccessTokenScopeWriteRepository)
req := NewRequestWithJSON(t, "POST", "/api/v1/repos/user2/repo1/branch_protections", &api.BranchProtection{
RuleName: "protected/**",
EnablePush: true,
}).AddTokenAuth(token)
MakeRequest(t, req, http.StatusCreated)

from := "protected/1"
req = NewRequestWithJSON(t, "POST", "/api/v1/repos/user2/repo1/branches", &api.CreateBranchRepoOption{
BranchName: from,
}).AddTokenAuth(token)
MakeRequest(t, req, http.StatusCreated)

resp := testAPIUpdateBranch(t, "user2", "user2", "repo1", from, "new-branch-name", http.StatusForbidden)
assert.Contains(t, resp.Body.String(), "Branch is protected by glob-based protection rules.")
})
t.Run("UpdateBranchNormalScenario", func(t *testing.T) {
testAPIUpdateBranch(t, "user2", "user2", "repo1", "branch2", "new-branch-name", http.StatusNoContent)
})
})
}

func testAPIUpdateBranch(t *testing.T, ownerName, repoName, from, to string, expectedHTTPStatus int) *httptest.ResponseRecorder {
token := getUserToken(t, ownerName, auth_model.AccessTokenScopeWriteRepository)
func testAPIUpdateBranch(t *testing.T, doerName, ownerName, repoName, from, to string, expectedHTTPStatus int) *httptest.ResponseRecorder {
token := getUserToken(t, doerName, auth_model.AccessTokenScopeWriteRepository)
req := NewRequestWithJSON(t, "PATCH", "api/v1/repos/"+ownerName+"/"+repoName+"/branches/"+from, &api.UpdateBranchRepoOption{
Name: to,
}).AddTokenAuth(token)
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/api_repo_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -735,5 +735,5 @@ func TestAPIRepoGetAssignees(t *testing.T) {
resp := MakeRequest(t, req, http.StatusOK)
var assignees []*api.User
DecodeJSON(t, resp, &assignees)
assert.Len(t, assignees, 1)
assert.Len(t, assignees, 2)
}

0 comments on commit 2483a93

Please sign in to comment.