From eb3ea5e4a53d509c61d70d9db5219048a55ac672 Mon Sep 17 00:00:00 2001 From: Kemal Zebari Date: Sun, 18 Jan 2026 13:05:52 -0800 Subject: [PATCH 1/3] Restrict branch naming when new change matches with protection rules --- options/locale/locale_en-US.json | 2 +- routers/api/v1/repo/branch.go | 2 +- services/repository/branch.go | 9 +++++++++ tests/integration/api_branch_test.go | 27 ++++++++++++++++++++++++++- 4 files changed, 37 insertions(+), 3 deletions(-) diff --git a/options/locale/locale_en-US.json b/options/locale/locale_en-US.json index f6c2aeb3cb53d..1c1891004c3dd 100644 --- a/options/locale/locale_en-US.json +++ b/options/locale/locale_en-US.json @@ -2663,7 +2663,7 @@ "repo.branch.new_branch_from": "Create new branch from \"%s\"", "repo.branch.renamed": "Branch %s was renamed to %s.", "repo.branch.rename_default_or_protected_branch_error": "Only admins can rename default or protected branches.", - "repo.branch.rename_protected_branch_failed": "This branch is protected by glob-based protection rules.", + "repo.branch.rename_protected_branch_failed": "Failed to rename branch due to branch protection rules.", "repo.branch.commits_divergence_from": "Commit divergence: %[1]d behind and %[2]d ahead of %[3]s", "repo.branch.commits_no_divergence": "The same as branch %[1]s", "repo.tag.create_tag": "Create tag %s", diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go index 4624d7e738c9e..9bdc0c76b8bb1 100644 --- a/routers/api/v1/repo/branch.go +++ b/routers/api/v1/repo/branch.go @@ -515,7 +515,7 @@ func RenameBranch(ctx *context.APIContext) { case repo_model.IsErrUserDoesNotHaveAccessToRepo(err): ctx.APIError(http.StatusForbidden, "User must be a repo or site admin to rename default or protected branches.") case errors.Is(err, git_model.ErrBranchIsProtected): - ctx.APIError(http.StatusForbidden, "Branch is protected by glob-based protection rules.") + ctx.APIError(http.StatusForbidden, "Failed to rename branch due to branch protection rules.") default: ctx.APIErrorInternal(err) } diff --git a/services/repository/branch.go b/services/repository/branch.go index 142073eabe7d9..c33c4bc4bd224 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -443,6 +443,15 @@ func RenameBranch(ctx context.Context, repo *repo_model.Repository, doer *user_m } } + // We also need to check if "to" matches with a protected branch rule. + isToBranchProtected, err := git_model.IsBranchProtected(ctx, repo.ID, to) + if err != nil { + return "", err + } + if isToBranchProtected && !perm.IsAdmin() { + return "", git_model.ErrBranchIsProtected + } + if err := git_model.RenameBranch(ctx, repo, from, to, func(ctx context.Context, isDefault bool) error { err2 := gitrepo.RenameBranch(ctx, repo, from, to) if err2 != nil { diff --git a/tests/integration/api_branch_test.go b/tests/integration/api_branch_test.go index 043aa10c7fb32..c8eacfb7b3a94 100644 --- a/tests/integration/api_branch_test.go +++ b/tests/integration/api_branch_test.go @@ -237,7 +237,32 @@ func TestAPIRenameBranch(t *testing.T) { MakeRequest(t, req, http.StatusCreated) resp := testAPIRenameBranch(t, "user2", "user2", "repo1", from, "new-branch-name", http.StatusForbidden) - assert.Contains(t, resp.Body.String(), "Branch is protected by glob-based protection rules.") + assert.Contains(t, resp.Body.String(), "Failed to rename branch due to branch protection rules.") + }) + t.Run("RenameBranchToMatchProtectionRulesWithAdminDoer", func(t *testing.T) { + // allow an admin (the owner in this case) to rename non-protected branch to one that matches a protected branch rule + repoName := "repo1" + ownerName := "user2" + from := "regular-branch-1" + ctx := NewAPITestContext(t, ownerName, repoName, auth_model.AccessTokenScopeWriteRepository) + testAPICreateBranch(t, ctx.Session, ownerName, repoName, "", from, http.StatusCreated) + + // NOTE: The protected/** branch protection rule was created in a previous test. + testAPIRenameBranch(t, ownerName, ownerName, repoName, from, "protected/2", http.StatusNoContent) + }) + t.Run("RenameBranchToMatchProtectionRulesWithNonAdminDoer", func(t *testing.T) { + // don't allow a non-admin to rename non-protected branch to one that matches a protected branch rule + repoName := "repo1" + ownerName := "user2" + from := "regular-branch-2" + ctx := NewAPITestContext(t, ownerName, repoName, auth_model.AccessTokenScopeWriteRepository) + testAPICreateBranch(t, ctx.Session, ownerName, repoName, "", from, http.StatusCreated) + + // NOTE: The protected/** branch protection rule was created in a previous test. + unprivilegedUser := "user40" + resp := testAPIRenameBranch(t, unprivilegedUser, ownerName, repoName, from, "protected/3", http.StatusForbidden) + + assert.Contains(t, resp.Body.String(), "Failed to rename branch due to branch protection rules.") }) t.Run("RenameBranchNormalScenario", func(t *testing.T) { testAPIRenameBranch(t, "user2", "user2", "repo1", "branch2", "new-branch-name", http.StatusNoContent) From b022698bc222d5db9fca4399aafcc41bbfb08036 Mon Sep 17 00:00:00 2001 From: Kemal Zebari Date: Sun, 18 Jan 2026 19:26:09 -0800 Subject: [PATCH 2/3] Utilize existing branch protection features for access control --- services/repository/branch.go | 4 ++-- tests/integration/api_branch_test.go | 22 +++++++++++++++------- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/services/repository/branch.go b/services/repository/branch.go index c33c4bc4bd224..4cafc5c3a34c3 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -444,11 +444,11 @@ func RenameBranch(ctx context.Context, repo *repo_model.Repository, doer *user_m } // We also need to check if "to" matches with a protected branch rule. - isToBranchProtected, err := git_model.IsBranchProtected(ctx, repo.ID, to) + rule, err := git_model.GetFirstMatchProtectedBranchRule(ctx, repo.ID, to) if err != nil { return "", err } - if isToBranchProtected && !perm.IsAdmin() { + if rule != nil && !rule.CanUserPush(ctx, doer) { return "", git_model.ErrBranchIsProtected } diff --git a/tests/integration/api_branch_test.go b/tests/integration/api_branch_test.go index c8eacfb7b3a94..0eec6930f6513 100644 --- a/tests/integration/api_branch_test.go +++ b/tests/integration/api_branch_test.go @@ -239,28 +239,36 @@ func TestAPIRenameBranch(t *testing.T) { resp := testAPIRenameBranch(t, "user2", "user2", "repo1", from, "new-branch-name", http.StatusForbidden) assert.Contains(t, resp.Body.String(), "Failed to rename branch due to branch protection rules.") }) - t.Run("RenameBranchToMatchProtectionRulesWithAdminDoer", func(t *testing.T) { - // allow an admin (the owner in this case) to rename non-protected branch to one that matches a protected branch rule + t.Run("RenameBranchToMatchProtectionRulesWithAllowedUser", func(t *testing.T) { + // allow an admin (the owner in this case) to rename a non-protected branch to one that matches a protected branch rule repoName := "repo1" ownerName := "user2" from := "regular-branch-1" ctx := NewAPITestContext(t, ownerName, repoName, auth_model.AccessTokenScopeWriteRepository) testAPICreateBranch(t, ctx.Session, ownerName, repoName, "", from, http.StatusCreated) - // NOTE: The protected/** branch protection rule was created in a previous test. + // NOTE: The protected/** branch protection rule was created in a previous test, with push enabled. testAPIRenameBranch(t, ownerName, ownerName, repoName, from, "protected/2", http.StatusNoContent) }) - t.Run("RenameBranchToMatchProtectionRulesWithNonAdminDoer", func(t *testing.T) { - // don't allow a non-admin to rename non-protected branch to one that matches a protected branch rule + t.Run("RenameBranchToMatchProtectionRulesWithUserWithUnauthorizedUser", func(t *testing.T) { + // don't allow an user not in the push whitelist to rename a non-protected branch to one that matches a protected branch rule repoName := "repo1" ownerName := "user2" + pushWhitelist := []string{ownerName} + token := getUserToken(t, "user2", auth_model.AccessTokenScopeWriteRepository) + req := NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/repos/%s/%s/branch_protections", ownerName, repoName), + &api.BranchProtection{ + RuleName: "owner-protected/**", + PushWhitelistUsernames: pushWhitelist, + }).AddTokenAuth(token) + + MakeRequest(t, req, http.StatusCreated) from := "regular-branch-2" ctx := NewAPITestContext(t, ownerName, repoName, auth_model.AccessTokenScopeWriteRepository) testAPICreateBranch(t, ctx.Session, ownerName, repoName, "", from, http.StatusCreated) - // NOTE: The protected/** branch protection rule was created in a previous test. unprivilegedUser := "user40" - resp := testAPIRenameBranch(t, unprivilegedUser, ownerName, repoName, from, "protected/3", http.StatusForbidden) + resp := testAPIRenameBranch(t, unprivilegedUser, ownerName, repoName, from, "owner-protected/1", http.StatusForbidden) assert.Contains(t, resp.Body.String(), "Failed to rename branch due to branch protection rules.") }) From 343e3d05d0a02ffa51efe79cb726058f1368436c Mon Sep 17 00:00:00 2001 From: Kemal Zebari Date: Mon, 19 Jan 2026 20:01:25 -0800 Subject: [PATCH 3/3] Fix up wording in test comments --- tests/integration/api_branch_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/integration/api_branch_test.go b/tests/integration/api_branch_test.go index 0eec6930f6513..1163c1e8c9c09 100644 --- a/tests/integration/api_branch_test.go +++ b/tests/integration/api_branch_test.go @@ -240,7 +240,7 @@ func TestAPIRenameBranch(t *testing.T) { assert.Contains(t, resp.Body.String(), "Failed to rename branch due to branch protection rules.") }) t.Run("RenameBranchToMatchProtectionRulesWithAllowedUser", func(t *testing.T) { - // allow an admin (the owner in this case) to rename a non-protected branch to one that matches a protected branch rule + // allow an admin (the owner in this case) to rename a regular branch to one that matches a branch protection rule repoName := "repo1" ownerName := "user2" from := "regular-branch-1" @@ -250,8 +250,8 @@ func TestAPIRenameBranch(t *testing.T) { // NOTE: The protected/** branch protection rule was created in a previous test, with push enabled. testAPIRenameBranch(t, ownerName, ownerName, repoName, from, "protected/2", http.StatusNoContent) }) - t.Run("RenameBranchToMatchProtectionRulesWithUserWithUnauthorizedUser", func(t *testing.T) { - // don't allow an user not in the push whitelist to rename a non-protected branch to one that matches a protected branch rule + t.Run("RenameBranchToMatchProtectionRulesWithUnauthorizedUser", func(t *testing.T) { + // don't allow renaming a regular branch to a protected branch if the doer is not in the push whitelist repoName := "repo1" ownerName := "user2" pushWhitelist := []string{ownerName} @@ -261,8 +261,8 @@ func TestAPIRenameBranch(t *testing.T) { RuleName: "owner-protected/**", PushWhitelistUsernames: pushWhitelist, }).AddTokenAuth(token) - MakeRequest(t, req, http.StatusCreated) + from := "regular-branch-2" ctx := NewAPITestContext(t, ownerName, repoName, auth_model.AccessTokenScopeWriteRepository) testAPICreateBranch(t, ctx.Session, ownerName, repoName, "", from, http.StatusCreated)