fix(repo): reject unparsable required approvals in branch protection (#40213)

Saving a branch protection rule with a "Required approvals" value that
can't be parsed as an integer (like `99999999999999999999` or `abc`)
showed the success message and stored 0, because the binding error was
never checked. The form now uses `form-fetch-action` with
`GetFetchActionForm`, so invalid input is rejected and nothing is saved.
Dev mode still answers unparsable numbers with a 500 until
`TypeCastError` gets a message, see
https://github.com/go-gitea/gitea/pull/40212.

- Validation errors show on the page and highlight the offending field,
keeping the form input
- Negative approvals are rejected before any rule lookup, and the input
gets `min="0"`

Fixes https://github.com/go-gitea/gitea/issues/40211

Assisted-by: Claude Code:claude-opus-5-5

---------

Co-authored-by: silverwind <me@silverwind.io>

options/locale/locale_en-US.json

@@ -2487,7 +2487,6 @@
   "repo.settings.choose_branch": "Choose a branch…",
   "repo.settings.no_protected_branch": "There are no protected branches.",
   "repo.settings.edit_protected_branch": "Edit",
-  "repo.settings.protected_branch_required_rule_name": "Required rule name",
   "repo.settings.protected_branch_duplicate_rule_name": "Duplicate rule name",
   "repo.settings.protected_branch_required_approvals_min": "Required approvals cannot be negative.",
   "repo.settings.tags": "Tags",

routers/web/repo/setting/protected_branch.go

@@ -5,9 +5,7 @@ package setting
 
 import (
 	"errors"
-	"fmt"
 	"net/http"
-	"net/url"
 	"strconv"
 	"strings"
 	"time"
@@ -124,14 +122,15 @@ func SettingsProtectedBranch(c *context.Context) {
 
 // SettingsProtectedBranchPost updates the protected branch settings
 func SettingsProtectedBranchPost(ctx *context.Context) {
-	f := web.GetForm[*forms.ProtectBranchForm](ctx)
-	var protectBranch *git_model.ProtectedBranch
-	if f.RuleName == "" {
-		ctx.Flash.Error(ctx.Tr("repo.settings.protected_branch_required_rule_name"))
-		ctx.Redirect(ctx.Repo.RepoLink + "/settings/branches/edit")
+	f := context.GetFetchActionForm[*forms.ProtectBranchForm](ctx)
+	if f == nil {
 		return
 	}
-
+	if f.RequiredApprovals < 0 {
+		ctx.JSONErrorWithField(ctx.Tr("repo.settings.protected_branch_required_approvals_min"), "required_approvals")
+		return
+	}
+	var protectBranch *git_model.ProtectedBranch
 	var err error
 	if f.RuleID > 0 {
 		// If the RuleID isn't 0, it must be an edit operation. So we get rule by id.
@@ -149,8 +148,7 @@ func SettingsProtectedBranchPost(ctx *context.Context) {
 				return
 			}
 			if sameNameProtectBranch != nil {
-				ctx.Flash.Error(ctx.Tr("repo.settings.protected_branch_duplicate_rule_name"))
-				ctx.Redirect(fmt.Sprintf("%s/settings/branches/edit?rule_name=%s", ctx.Repo.RepoLink, protectBranch.RuleName))
+				ctx.JSONErrorWithField(ctx.Tr("repo.settings.protected_branch_duplicate_rule_name"), "rule_name")
 				return
 			}
 		}
@@ -176,11 +174,6 @@ func SettingsProtectedBranchPost(ctx *context.Context) {
 
 	var whitelistUsers, whitelistTeams, forcePushAllowlistUsers, forcePushAllowlistTeams, deletionAllowlistUsers, deletionAllowlistTeams, mergeWhitelistUsers, mergeWhitelistTeams, approvalsWhitelistUsers, approvalsWhitelistTeams, bypassAllowlistUsers, bypassAllowlistTeams []int64
 	protectBranch.RuleName = f.RuleName
-	if f.RequiredApprovals < 0 {
-		ctx.Flash.Error(ctx.Tr("repo.settings.protected_branch_required_approvals_min"))
-		ctx.Redirect(fmt.Sprintf("%s/settings/branches/edit?rule_name=%s", ctx.Repo.RepoLink, f.RuleName))
-		return
-	}
 
 	switch f.EnablePush {
 	case "all":
@@ -271,16 +264,14 @@ func SettingsProtectedBranchPost(ctx *context.Context) {
 				continue
 			}
 			if _, err := glob.Compile(trimmed); err != nil {
-				ctx.Flash.Error(ctx.Tr("repo.settings.protect_invalid_status_check_pattern", pattern))
-				ctx.Redirect(fmt.Sprintf("%s/settings/branches/edit?rule_name=%s", ctx.Repo.RepoLink, url.QueryEscape(protectBranch.RuleName)))
+				ctx.JSONErrorWithField(ctx.Tr("repo.settings.protect_invalid_status_check_pattern", pattern), "status_check_contexts")
 				return
 			}
 			validPatterns = append(validPatterns, trimmed)
 		}
 		if len(validPatterns) == 0 {
 			// if status check is enabled, patterns slice is not allowed to be empty
-			ctx.Flash.Error(ctx.Tr("repo.settings.protect_no_valid_status_check_patterns"))
-			ctx.Redirect(fmt.Sprintf("%s/settings/branches/edit?rule_name=%s", ctx.Repo.RepoLink, url.QueryEscape(protectBranch.RuleName)))
+			ctx.JSONErrorWithField(ctx.Tr("repo.settings.protect_no_valid_status_check_patterns"), "status_check_contexts")
 			return
 		}
 		protectBranch.StatusCheckContexts = validPatterns
@@ -334,7 +325,7 @@ func SettingsProtectedBranchPost(ctx *context.Context) {
 	}
 
 	ctx.Flash.Success(ctx.Tr("repo.settings.update_protect_branch_success", protectBranch.RuleName))
-	ctx.Redirect(fmt.Sprintf("%s/settings/branches?rule_name=%s", ctx.Repo.RepoLink, protectBranch.RuleName))
+	ctx.JSONRedirect(ctx.Repo.RepoLink + "/settings/branches")
 }
 
 // DeleteProtectedBranchRulePost delete protected branch rule by id

routers/web/web.go

@@ -1181,7 +1181,7 @@ func registerWebRoutes(m *web.Router, webAuth *AuthMiddleware) {
 		m.Group("/branches", func() {
 			m.Get("/", repo_setting.ProtectedBranchRules)
 			m.Combo("/edit").Get(repo_setting.SettingsProtectedBranch).
-				Post(web.Bind[*forms.ProtectBranchForm](), context.RepoMustNotBeArchived(), repo_setting.SettingsProtectedBranchPost)
+				Post(context.RepoMustNotBeArchived(), repo_setting.SettingsProtectedBranchPost)
 			m.Post("/{id}/delete", repo_setting.DeleteProtectedBranchRulePost)
 			m.Post("/priority", context.RepoMustNotBeArchived(), repo_setting.UpdateBranchProtectionPriories)
 		})

templates/repo/settings/protected_branch.tmpl

@@ -1,6 +1,6 @@
 {{template "repo/settings/layout_head" (dict "pageClass" "repository settings branches")}}
 	<div class="repo-setting-content">
-		<form class="ui form" action="{{.Link}}" method="post">
+		<form class="ui form form-fetch-action" action="{{.Link}}" method="post">
 			<h4 class="ui top attached header">
 				{{if .Rule.RuleName}}
 					{{ctx.Locale.Tr "repo.settings.branch_protection" .Rule.RuleName}}
@@ -145,7 +145,7 @@
 				<h5 class="ui dividing header">{{ctx.Locale.Tr "repo.settings.event_pull_request_approvals"}}</h5>
 				<div class="field">
 					<label>{{ctx.Locale.Tr "repo.settings.protect_required_approvals"}}</label>
-					<input name="required_approvals" type="number" value="{{.Rule.RequiredApprovals}}">
+					<input name="required_approvals" type="number" min="0" value="{{.Rule.RequiredApprovals}}">
 					<p class="help tw-ml-0">{{ctx.Locale.Tr "repo.settings.protect_required_approvals_desc"}}</p>
 				</div>
 				<div class="grouped fields">

tests/integration/actions_scoped_workflow_test.go

@@ -252,7 +252,7 @@ jobs:
 				pbValues["enable_status_check"] = "true"
 				pbValues["status_check_contexts"] = "ci/manual"
 			}
-			user2Session.MakeRequest(t, NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s/settings/branches/edit", consumer.OwnerName, consumer.Name), pbValues), http.StatusSeeOther)
+			user2Session.MakeRequest(t, NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s/settings/branches/edit", consumer.OwnerName, consumer.Name), pbValues), http.StatusOK)
 
 			prFile := &api.CreateFileOptions{
 				FileOptions: api.FileOptions{

tests/integration/editor_test.go

@@ -91,12 +91,15 @@ func testCreateFile(t *testing.T, session *TestSession, user, repo, baseBranchNa
 
 func testEditorProtectedBranch(t *testing.T) {
 	session := loginUser(t, "user2")
+	assert.JSONEq(t, `{"errorMessage":"Required approvals cannot be negative.","renderFormat":"html","errorFields":["required_approvals"]}`,
+		session.MakeRequest(t, NewRequestWithValues(t, "POST", "/user2/repo1/settings/branches/edit", map[string]string{"rule_name": "master", "required_approvals": "-1"}), http.StatusBadRequest).Body.String())
+
 	// Change the "master" branch to "protected"
 	req := NewRequestWithValues(t, "POST", "/user2/repo1/settings/branches/edit", map[string]string{
 		"rule_name":   "master",
 		"enable_push": "true",
 	})
-	session.MakeRequest(t, req, http.StatusSeeOther)
+	session.MakeRequest(t, req, http.StatusOK)
 	flashMsg := session.GetCookieFlashMessage()
 	assert.Equal(t, `Branch protection for rule "master" has been updated.`, flashMsg.SuccessMsg)
 
@@ -112,7 +115,7 @@ func testEditorProtectedBranch(t *testing.T) {
 		"unprotected_file_patterns": "docs/*.md",
 		"enable_push":               "true",
 	})
-	session.MakeRequest(t, req, http.StatusSeeOther)
+	session.MakeRequest(t, req, http.StatusOK)
 	flashMsg = session.GetCookieFlashMessage()
 	assert.Equal(t, `Branch protection for rule "master" has been updated.`, flashMsg.SuccessMsg)
 

tests/integration/git_general_test.go

@@ -553,8 +553,8 @@ func doProtectBranchExt(ctx APITestContext, ruleName string, opts doProtectBranc
 		}
 
 		// Send the request to update branch protection settings
-		req := NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s/settings/branches/edit", url.PathEscape(ctx.Username), url.PathEscape(ctx.Reponame)), formData)
-		ctx.Session.MakeRequest(t, req, http.StatusSeeOther)
+		link := fmt.Sprintf("/%s/%s/settings/branches", url.PathEscape(ctx.Username), url.PathEscape(ctx.Reponame))
+		assert.Equal(t, link, test.RedirectURL(ctx.Session.MakeRequest(t, NewRequestWithValues(t, "POST", link+"/edit", formData), http.StatusOK)))
 
 		// Check if the "master" branch has been locked successfully
 		flashMsg := ctx.Session.GetCookieFlashMessage()

tests/integration/pull_merge_test.go

@@ -829,7 +829,7 @@ func TestPullAutoMergeAfterCommitStatusSucceed(t *testing.T) {
 			"enable_status_check":   "true",
 			"status_check_contexts": "gitea/actions",
 		})
-		session.MakeRequest(t, req, http.StatusSeeOther)
+		session.MakeRequest(t, req, http.StatusOK)
 
 		addToQueuePullChan := make(chan automergequeue.AutoMergeItem, 1)
 		resetAutoMergeQueueMock := test.MockVariableValue(&automergequeue.AddToQueue, func(item automergequeue.AutoMergeItem) { addToQueuePullChan <- item })
@@ -911,7 +911,7 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApproval(t *testing.T) {
 			"status_check_contexts": "gitea/actions",
 			"required_approvals":    "1",
 		})
-		baseSession.MakeRequest(t, req, http.StatusSeeOther)
+		baseSession.MakeRequest(t, req, http.StatusOK)
 
 		// first time insert automerge record, return true
 		scheduled, err := automerge.ScheduleAutoMerge(t.Context(), baseUser, pr, repo_model.MergeStyleMerge, "auto merge test", false)
@@ -1023,7 +1023,7 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApprovalForAgitFlow(t *testing.
 			"status_check_contexts": "gitea/actions",
 			"required_approvals":    "1",
 		})
-		session.MakeRequest(t, req, http.StatusSeeOther)
+		session.MakeRequest(t, req, http.StatusOK)
 
 		user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
 		// first time insert automerge record, return true
@@ -1098,7 +1098,7 @@ func TestPullNonMergeForAdminWithBranchProtection(t *testing.T) {
 			"status_check_contexts":      "gitea/actions",
 			"block_admin_merge_override": "true",
 		})
-		session.MakeRequest(t, pbCreateReq, http.StatusSeeOther)
+		session.MakeRequest(t, pbCreateReq, http.StatusOK)
 
 		token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository)
 
@@ -1144,7 +1144,7 @@ func TestPullForceMergeForBypassAllowlistUser(t *testing.T) {
 			"enable_bypass_allowlist":    "on",
 			"bypass_allowlist_users":     strconv.FormatInt(bypassUser.ID, 10),
 		})
-		ownerSession.MakeRequest(t, pbCreateReq, http.StatusSeeOther)
+		ownerSession.MakeRequest(t, pbCreateReq, http.StatusOK)
 		defer testAPIDeleteBranchProtection(t, "master", http.StatusNoContent)
 
 		token := getTokenForLoggedInUser(t, bypassSession, auth_model.AccessTokenScopeWriteRepository)