From 47d69b7749689a7a7570ac20bdfd30d336daf7c1 Mon Sep 17 00:00:00 2001
From: Kemal Zebari <60799661+kemzeb@users.noreply.github.com>
Date: Sat, 7 Jun 2025 01:25:08 -0700
Subject: [PATCH] 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>
---
 modules/label/label.go                        | 14 ++-
 modules/test/utils.go                         | 10 ++
 options/locale/locale_en-US.ini               |  2 +-
 routers/web/org/org_labels.go                 | 36 ++++---
 routers/web/repo/issue_label.go               | 38 ++++----
 routers/web/repo/issue_label_test.go          | 94 +++++++++++++------
 routers/web/shared/label/label.go             | 26 +++++
 .../repo/issue/labels/label_edit_modal.tmpl   |  5 +-
 web_src/js/features/comp/LabelEdit.ts         |  2 +-
 9 files changed, 151 insertions(+), 76 deletions(-)
 create mode 100644 routers/web/shared/label/label.go

diff --git a/modules/label/label.go b/modules/label/label.go
index ce028aa9f3..3e68c4d26e 100644
--- a/modules/label/label.go
+++ b/modules/label/label.go
@@ -7,10 +7,10 @@ import (
 	"fmt"
 	"regexp"
 	"strings"
-)
+	"sync"
 
-// colorPattern is a regexp which can validate label color
-var colorPattern = regexp.MustCompile("^#?(?:[0-9a-fA-F]{6}|[0-9a-fA-F]{3})$")
+	"code.gitea.io/gitea/modules/util"
+)
 
 // Label represents label information loaded from template
 type Label struct {
@@ -21,6 +21,10 @@ type Label struct {
 	ExclusiveOrder int    `yaml:"exclusive_order,omitempty"`
 }
 
+var colorPattern = sync.OnceValue(func() *regexp.Regexp {
+	return regexp.MustCompile(`^#([\da-fA-F]{3}|[\da-fA-F]{6})$`)
+})
+
 // NormalizeColor normalizes a color string to a 6-character hex code
 func NormalizeColor(color string) (string, error) {
 	// normalize case
@@ -31,8 +35,8 @@ func NormalizeColor(color string) (string, error) {
 		color = "#" + color
 	}
 
-	if !colorPattern.MatchString(color) {
-		return "", fmt.Errorf("bad color code: %s", color)
+	if !colorPattern().MatchString(color) {
+		return "", util.NewInvalidArgumentErrorf("invalid color: %s", color)
 	}
 
 	// convert 3-character shorthand into 6-character version
diff --git a/modules/test/utils.go b/modules/test/utils.go
index 3051d3d286..53c6a3ed52 100644
--- a/modules/test/utils.go
+++ b/modules/test/utils.go
@@ -17,6 +17,7 @@ import (
 
 // RedirectURL returns the redirect URL of a http response.
 // It also works for JSONRedirect: `{"redirect": "..."}`
+// FIXME: it should separate the logic of checking from header and JSON body
 func RedirectURL(resp http.ResponseWriter) string {
 	loc := resp.Header().Get("Location")
 	if loc != "" {
@@ -34,6 +35,15 @@ func RedirectURL(resp http.ResponseWriter) string {
 	return ""
 }
 
+func ParseJSONError(buf []byte) (ret struct {
+	ErrorMessage string `json:"errorMessage"`
+	RenderFormat string `json:"renderFormat"`
+},
+) {
+	_ = json.Unmarshal(buf, &ret)
+	return ret
+}
+
 func IsNormalPageCompleted(s string) bool {
 	return strings.Contains(s, `<footer class="page-footer"`) && strings.Contains(s, `</html>`)
 }
diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini
index e795db7a68..8797ef4bc0 100644
--- a/options/locale/locale_en-US.ini
+++ b/options/locale/locale_en-US.ini
@@ -1433,7 +1433,6 @@ commitstatus.success = Success
 ext_issues = Access to External Issues
 ext_issues.desc = Link to an external issue tracker.
 
-projects = Projects
 projects.desc = Manage issues and pulls in projects.
 projects.description = Description (optional)
 projects.description_placeholder = Description
@@ -1653,6 +1652,7 @@ issues.save = Save
 issues.label_title = Name
 issues.label_description = Description
 issues.label_color = Color
+issues.label_color_invalid = Invalid color
 issues.label_exclusive = Exclusive
 issues.label_archive = Archive Label
 issues.label_archived_filter = Show archived labels
diff --git a/routers/web/org/org_labels.go b/routers/web/org/org_labels.go
index 456ed3f01e..2a4aa7f557 100644
--- a/routers/web/org/org_labels.go
+++ b/routers/web/org/org_labels.go
@@ -4,13 +4,15 @@
 package org
 
 import (
-	"net/http"
+	"errors"
 
 	"code.gitea.io/gitea/models/db"
 	issues_model "code.gitea.io/gitea/models/issues"
 	"code.gitea.io/gitea/modules/label"
 	repo_module "code.gitea.io/gitea/modules/repository"
+	"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"
 )
@@ -32,14 +34,8 @@ func RetrieveLabels(ctx *context.Context) {
 
 // NewLabel create new label for organization
 func NewLabel(ctx *context.Context) {
-	form := web.GetForm(ctx).(*forms.CreateLabelForm)
-	ctx.Data["Title"] = ctx.Tr("repo.labels")
-	ctx.Data["PageIsLabels"] = true
-	ctx.Data["PageIsOrgSettings"] = true
-
-	if ctx.HasError() {
-		ctx.Flash.Error(ctx.Data["ErrorMsg"].(string))
-		ctx.Redirect(ctx.Org.OrgLink + "/settings/labels")
+	form := shared_label.GetLabelEditForm(ctx)
+	if ctx.Written() {
 		return
 	}
 
@@ -55,20 +51,22 @@ func NewLabel(ctx *context.Context) {
 		ctx.ServerError("NewLabel", err)
 		return
 	}
-	ctx.Redirect(ctx.Org.OrgLink + "/settings/labels")
+	ctx.JSONRedirect(ctx.Org.OrgLink + "/settings/labels")
 }
 
 // UpdateLabel update a label's name and color
 func UpdateLabel(ctx *context.Context) {
-	form := web.GetForm(ctx).(*forms.CreateLabelForm)
+	form := shared_label.GetLabelEditForm(ctx)
+	if ctx.Written() {
+		return
+	}
+
 	l, err := issues_model.GetLabelInOrgByID(ctx, ctx.Org.Organization.ID, form.ID)
-	if err != nil {
-		switch {
-		case issues_model.IsErrOrgLabelNotExist(err):
-			ctx.HTTPError(http.StatusNotFound)
-		default:
-			ctx.ServerError("UpdateLabel", err)
-		}
+	if errors.Is(err, util.ErrNotExist) {
+		ctx.JSONErrorNotFound()
+		return
+	} else if err != nil {
+		ctx.ServerError("GetLabelInOrgByID", err)
 		return
 	}
 
@@ -82,7 +80,7 @@ func UpdateLabel(ctx *context.Context) {
 		ctx.ServerError("UpdateLabel", err)
 		return
 	}
-	ctx.Redirect(ctx.Org.OrgLink + "/settings/labels")
+	ctx.JSONRedirect(ctx.Org.OrgLink + "/settings/labels")
 }
 
 // DeleteLabel delete a label
diff --git a/routers/web/repo/issue_label.go b/routers/web/repo/issue_label.go
index f9c41adbcf..72a316e98d 100644
--- a/routers/web/repo/issue_label.go
+++ b/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
diff --git a/routers/web/repo/issue_label_test.go b/routers/web/repo/issue_label_test.go
index c3fba07034..f4eca26f8e 100644
--- a/routers/web/repo/issue_label_test.go
+++ b/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)
diff --git a/routers/web/shared/label/label.go b/routers/web/shared/label/label.go
new file mode 100644
index 0000000000..6968a318c4
--- /dev/null
+++ b/routers/web/shared/label/label.go
@@ -0,0 +1,26 @@
+// Copyright 2025 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package label
+
+import (
+	"code.gitea.io/gitea/modules/label"
+	"code.gitea.io/gitea/modules/web"
+	"code.gitea.io/gitea/services/context"
+	"code.gitea.io/gitea/services/forms"
+)
+
+func GetLabelEditForm(ctx *context.Context) *forms.CreateLabelForm {
+	form := web.GetForm(ctx).(*forms.CreateLabelForm)
+	if ctx.HasError() {
+		ctx.JSONError(ctx.Data["ErrorMsg"].(string))
+		return nil
+	}
+	var err error
+	form.Color, err = label.NormalizeColor(form.Color)
+	if err != nil {
+		ctx.JSONError(ctx.Tr("repo.issues.label_color_invalid"))
+		return nil
+	}
+	return form
+}
diff --git a/templates/repo/issue/labels/label_edit_modal.tmpl b/templates/repo/issue/labels/label_edit_modal.tmpl
index 364fc508f1..6837d66dce 100644
--- a/templates/repo/issue/labels/label_edit_modal.tmpl
+++ b/templates/repo/issue/labels/label_edit_modal.tmpl
@@ -5,7 +5,7 @@
 >
 	<div class="header"></div>
 	<div class="content">
-		<form class="ui form ignore-dirty" method="post">
+		<form class="ui form ignore-dirty form-fetch-action" method="post">
 			{{.CsrfTokenHtml}}
 			<input name="id" type="hidden">
 			<div class="required field">
@@ -50,7 +50,8 @@
 			<div class="field">
 				<label for="color">{{ctx.Locale.Tr "repo.issues.label_color"}}</label>
 				<div class="column js-color-picker-input">
-					<input name="color" value="#70c24a" placeholder="#c320f6" required maxlength="7">
+					<!-- the "#" is optional because backend NormalizeColor is able to handle it, API also accepts both formats, and it is easier for users to directly copy-paste a hex value -->
+					<input name="color" value="#70c24a" placeholder="#c320f6" required pattern="^#?([\dA-Fa-f]{3}|[\dA-Fa-f]{6})$" maxlength="7">
 					{{template "repo/issue/label_precolors"}}
 				</div>
 			</div>
diff --git a/web_src/js/features/comp/LabelEdit.ts b/web_src/js/features/comp/LabelEdit.ts
index 55351cd900..a5bb750cdb 100644
--- a/web_src/js/features/comp/LabelEdit.ts
+++ b/web_src/js/features/comp/LabelEdit.ts
@@ -70,7 +70,7 @@ export function initCompLabelEdit(pageSelector: string) {
           form.reportValidity();
           return false;
         }
-        form.submit();
+        form.dispatchEvent(new Event('submit', {bubbles: true}));
       },
     }).modal('show');
   };