Validate hex colors when creating/editing labels (#34623)

Resolves #34618.

---------

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

routers/web/repo/issue_label.go

@@ -4,6 +4,7 @@
 package repo
 
 import (
+	"errors"
 	"net/http"
 
 	"code.gitea.io/gitea/models/db"
@@ -13,7 +14,9 @@ import (
 	"code.gitea.io/gitea/modules/log"
 	repo_module "code.gitea.io/gitea/modules/repository"
 	"code.gitea.io/gitea/modules/templates"
+	"code.gitea.io/gitea/modules/util"
 	"code.gitea.io/gitea/modules/web"
+	shared_label "code.gitea.io/gitea/routers/web/shared/label"
 	"code.gitea.io/gitea/services/context"
 	"code.gitea.io/gitea/services/forms"
 	issue_service "code.gitea.io/gitea/services/issue"
@@ -100,13 +103,8 @@ func RetrieveLabelsForList(ctx *context.Context) {
 
 // NewLabel create new label for repository
 func NewLabel(ctx *context.Context) {
-	form := web.GetForm(ctx).(*forms.CreateLabelForm)
-	ctx.Data["Title"] = ctx.Tr("repo.labels")
-	ctx.Data["PageIsLabels"] = true
-
-	if ctx.HasError() {
-		ctx.Flash.Error(ctx.Data["ErrorMsg"].(string))
-		ctx.Redirect(ctx.Repo.RepoLink + "/labels")
+	form := shared_label.GetLabelEditForm(ctx)
+	if ctx.Written() {
 		return
 	}
 
@@ -122,34 +120,36 @@ func NewLabel(ctx *context.Context) {
 		ctx.ServerError("NewLabel", err)
 		return
 	}
-	ctx.Redirect(ctx.Repo.RepoLink + "/labels")
+	ctx.JSONRedirect(ctx.Repo.RepoLink + "/labels")
 }
 
 // UpdateLabel update a label's name and color
 func UpdateLabel(ctx *context.Context) {
-	form := web.GetForm(ctx).(*forms.CreateLabelForm)
-	l, err := issues_model.GetLabelInRepoByID(ctx, ctx.Repo.Repository.ID, form.ID)
-	if err != nil {
-		switch {
-		case issues_model.IsErrRepoLabelNotExist(err):
-			ctx.HTTPError(http.StatusNotFound)
-		default:
-			ctx.ServerError("UpdateLabel", err)
-		}
+	form := shared_label.GetLabelEditForm(ctx)
+	if ctx.Written() {
 		return
 	}
+
+	l, err := issues_model.GetLabelInRepoByID(ctx, ctx.Repo.Repository.ID, form.ID)
+	if errors.Is(err, util.ErrNotExist) {
+		ctx.JSONErrorNotFound()
+		return
+	} else if err != nil {
+		ctx.ServerError("GetLabelInRepoByID", err)
+		return
+	}
+
 	l.Name = form.Title
 	l.Exclusive = form.Exclusive
 	l.ExclusiveOrder = form.ExclusiveOrder
 	l.Description = form.Description
 	l.Color = form.Color
-
 	l.SetArchived(form.IsArchived)
 	if err := issues_model.UpdateLabel(ctx, l); err != nil {
 		ctx.ServerError("UpdateLabel", err)
 		return
 	}
-	ctx.Redirect(ctx.Repo.RepoLink + "/labels")
+	ctx.JSONRedirect(ctx.Repo.RepoLink + "/labels")
 }
 
 // DeleteLabel delete a label

routers/web/repo/issue_label_test.go

@@ -6,10 +6,12 @@ package repo
 import (
 	"net/http"
 	"strconv"
+	"strings"
 	"testing"
 
 	issues_model "code.gitea.io/gitea/models/issues"
 	"code.gitea.io/gitea/models/unittest"
+	"code.gitea.io/gitea/modules/base"
 	"code.gitea.io/gitea/modules/repository"
 	"code.gitea.io/gitea/modules/test"
 	"code.gitea.io/gitea/modules/web"
@@ -19,19 +21,20 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-func int64SliceToCommaSeparated(a []int64) string {
-	s := ""
-	for i, n := range a {
-		if i > 0 {
-			s += ","
-		}
-		s += strconv.Itoa(int(n))
-	}
-	return s
+func TestIssueLabel(t *testing.T) {
+	unittest.PrepareTestEnv(t)
+	t.Run("RetrieveLabels", testRetrieveLabels)
+	t.Run("NewLabel", testNewLabel)
+	t.Run("NewLabelInvalidColor", testNewLabelInvalidColor)
+	t.Run("UpdateLabel", testUpdateLabel)
+	t.Run("UpdateLabelInvalidColor", testUpdateLabelInvalidColor)
+	t.Run("UpdateIssueLabelClear", testUpdateIssueLabelClear)
+	t.Run("UpdateIssueLabelToggle", testUpdateIssueLabelToggle)
+	t.Run("InitializeLabels", testInitializeLabels)
+	t.Run("DeleteLabel", testDeleteLabel)
 }
 
-func TestInitializeLabels(t *testing.T) {
-	unittest.PrepareTestEnv(t)
+func testInitializeLabels(t *testing.T) {
 	assert.NoError(t, repository.LoadRepoConfig())
 	ctx, _ := contexttest.MockContext(t, "user2/repo1/labels/initialize")
 	contexttest.LoadUser(t, ctx, 2)
@@ -47,8 +50,7 @@ func TestInitializeLabels(t *testing.T) {
 	assert.Equal(t, "/user2/repo2/labels", test.RedirectURL(ctx.Resp))
 }
 
-func TestRetrieveLabels(t *testing.T) {
-	unittest.PrepareTestEnv(t)
+func testRetrieveLabels(t *testing.T) {
 	for _, testCase := range []struct {
 		RepoID           int64
 		Sort             string
@@ -74,9 +76,8 @@ func TestRetrieveLabels(t *testing.T) {
 	}
 }
 
-func TestNewLabel(t *testing.T) {
-	unittest.PrepareTestEnv(t)
-	ctx, _ := contexttest.MockContext(t, "user2/repo1/labels/edit")
+func testNewLabel(t *testing.T) {
+	ctx, respWriter := contexttest.MockContext(t, "user2/repo1/labels/edit")
 	contexttest.LoadUser(t, ctx, 2)
 	contexttest.LoadRepo(t, ctx, 1)
 	web.SetForm(ctx, &forms.CreateLabelForm{
@@ -84,17 +85,32 @@ func TestNewLabel(t *testing.T) {
 		Color: "#abcdef",
 	})
 	NewLabel(ctx)
-	assert.Equal(t, http.StatusSeeOther, ctx.Resp.WrittenStatus())
+	assert.Equal(t, http.StatusOK, ctx.Resp.WrittenStatus())
 	unittest.AssertExistsAndLoadBean(t, &issues_model.Label{
 		Name:  "newlabel",
 		Color: "#abcdef",
 	})
-	assert.Equal(t, "/user2/repo1/labels", test.RedirectURL(ctx.Resp))
+	assert.Equal(t, "/user2/repo1/labels", test.RedirectURL(respWriter))
 }
 
-func TestUpdateLabel(t *testing.T) {
-	unittest.PrepareTestEnv(t)
-	ctx, _ := contexttest.MockContext(t, "user2/repo1/labels/edit")
+func testNewLabelInvalidColor(t *testing.T) {
+	ctx, respWriter := contexttest.MockContext(t, "user2/repo1/labels/edit")
+	contexttest.LoadUser(t, ctx, 2)
+	contexttest.LoadRepo(t, ctx, 1)
+	web.SetForm(ctx, &forms.CreateLabelForm{
+		Title: "newlabel-x",
+		Color: "bad-label-code",
+	})
+	NewLabel(ctx)
+	assert.Equal(t, http.StatusBadRequest, ctx.Resp.WrittenStatus())
+	assert.Equal(t, "repo.issues.label_color_invalid", test.ParseJSONError(respWriter.Body.Bytes()).ErrorMessage)
+	unittest.AssertNotExistsBean(t, &issues_model.Label{
+		Name: "newlabel-x",
+	})
+}
+
+func testUpdateLabel(t *testing.T) {
+	ctx, respWriter := contexttest.MockContext(t, "user2/repo1/labels/edit")
 	contexttest.LoadUser(t, ctx, 2)
 	contexttest.LoadRepo(t, ctx, 1)
 	web.SetForm(ctx, &forms.CreateLabelForm{
@@ -104,17 +120,37 @@ func TestUpdateLabel(t *testing.T) {
 		IsArchived: true,
 	})
 	UpdateLabel(ctx)
-	assert.Equal(t, http.StatusSeeOther, ctx.Resp.WrittenStatus())
+	assert.Equal(t, http.StatusOK, ctx.Resp.WrittenStatus())
 	unittest.AssertExistsAndLoadBean(t, &issues_model.Label{
 		ID:    2,
 		Name:  "newnameforlabel",
 		Color: "#abcdef",
 	})
-	assert.Equal(t, "/user2/repo1/labels", test.RedirectURL(ctx.Resp))
+	assert.Equal(t, "/user2/repo1/labels", test.RedirectURL(respWriter))
 }
 
-func TestDeleteLabel(t *testing.T) {
-	unittest.PrepareTestEnv(t)
+func testUpdateLabelInvalidColor(t *testing.T) {
+	ctx, respWriter := contexttest.MockContext(t, "user2/repo1/labels/edit")
+	contexttest.LoadUser(t, ctx, 2)
+	contexttest.LoadRepo(t, ctx, 1)
+	web.SetForm(ctx, &forms.CreateLabelForm{
+		ID:    1,
+		Title: "label1",
+		Color: "bad-label-code",
+	})
+
+	UpdateLabel(ctx)
+
+	assert.Equal(t, http.StatusBadRequest, ctx.Resp.WrittenStatus())
+	assert.Equal(t, "repo.issues.label_color_invalid", test.ParseJSONError(respWriter.Body.Bytes()).ErrorMessage)
+	unittest.AssertExistsAndLoadBean(t, &issues_model.Label{
+		ID:    1,
+		Name:  "label1",
+		Color: "#abcdef",
+	})
+}
+
+func testDeleteLabel(t *testing.T) {
 	ctx, _ := contexttest.MockContext(t, "user2/repo1/labels/delete")
 	contexttest.LoadUser(t, ctx, 2)
 	contexttest.LoadRepo(t, ctx, 1)
@@ -126,8 +162,7 @@ func TestDeleteLabel(t *testing.T) {
 	assert.EqualValues(t, ctx.Tr("repo.issues.label_deletion_success"), ctx.Flash.SuccessMsg)
 }
 
-func TestUpdateIssueLabel_Clear(t *testing.T) {
-	unittest.PrepareTestEnv(t)
+func testUpdateIssueLabelClear(t *testing.T) {
 	ctx, _ := contexttest.MockContext(t, "user2/repo1/issues/labels")
 	contexttest.LoadUser(t, ctx, 2)
 	contexttest.LoadRepo(t, ctx, 1)
@@ -140,7 +175,7 @@ func TestUpdateIssueLabel_Clear(t *testing.T) {
 	unittest.CheckConsistencyFor(t, &issues_model.Label{})
 }
 
-func TestUpdateIssueLabel_Toggle(t *testing.T) {
+func testUpdateIssueLabelToggle(t *testing.T) {
 	for _, testCase := range []struct {
 		Action      string
 		IssueIDs    []int64
@@ -156,7 +191,8 @@ func TestUpdateIssueLabel_Toggle(t *testing.T) {
 		ctx, _ := contexttest.MockContext(t, "user2/repo1/issues/labels")
 		contexttest.LoadUser(t, ctx, 2)
 		contexttest.LoadRepo(t, ctx, 1)
-		ctx.Req.Form.Set("issue_ids", int64SliceToCommaSeparated(testCase.IssueIDs))
+
+		ctx.Req.Form.Set("issue_ids", strings.Join(base.Int64sToStrings(testCase.IssueIDs), ","))
 		ctx.Req.Form.Set("action", testCase.Action)
 		ctx.Req.Form.Set("id", strconv.Itoa(int(testCase.LabelID)))
 		UpdateIssueLabel(ctx)