enhance: Improve validation errors for secrets/variables (#39221)

Signed-off-by: Ross Golder <ross@golder.org>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

services/context/api.go

@@ -170,22 +170,11 @@ func (ctx *APIContext) APIError(status int, msg string) {
 
 // APIErrorAuto use error check function to determine the response code
 func (ctx *APIContext) APIErrorAuto(err error) {
-	switch {
-	case errors.Is(err, util.ErrInvalidArgument):
-		ctx.APIError(http.StatusBadRequest, err.Error())
-	case errors.Is(err, util.ErrPermissionDenied):
-		ctx.APIError(http.StatusForbidden, err.Error())
-	case errors.Is(err, util.ErrNotExist):
-		ctx.APIError(http.StatusNotFound, err.Error())
-	case errors.Is(err, util.ErrAlreadyExist):
-		ctx.APIError(http.StatusConflict, err.Error())
-	case errors.Is(err, util.ErrContentTooLarge):
-		ctx.APIError(http.StatusRequestEntityTooLarge, err.Error())
-	case errors.Is(err, util.ErrUnprocessableContent):
-		ctx.APIError(http.StatusUnprocessableEntity, err.Error())
-	default:
-		ctx.apiErrorInternal(1, err)
+	if errMsg, code := util.ErrorUnwrapForUser(err); errMsg != "" {
+		ctx.APIError(code, errMsg)
+		return
 	}
+	ctx.apiErrorInternal(1, err)
 }
 
 type apiContextKeyType struct{}

services/context/context.go

@@ -18,6 +18,7 @@ import (
 	"gitea.dev/modules/cache"
 	"gitea.dev/modules/httpcache"
 	"gitea.dev/modules/httplib"
+	"gitea.dev/modules/log"
 	"gitea.dev/modules/reqctx"
 	"gitea.dev/modules/session"
 	"gitea.dev/modules/setting"
@@ -245,8 +246,8 @@ func (ctx *Context) JSONOK() {
 	ctx.JSON(http.StatusOK, map[string]any{"ok": true}) // this is only a dummy response, frontend seldom uses it
 }
 
-func buildJsonErrorMap(msg any) map[string]any {
-	switch v := msg.(type) {
+func buildJsonErrorMap[T string | template.HTML](msg T) map[string]any {
+	switch v := any(msg).(type) {
 	case string:
 		return map[string]any{"errorMessage": v, "renderFormat": "text"}
 	case template.HTML:
@@ -255,11 +256,26 @@ func buildJsonErrorMap(msg any) map[string]any {
 	panic(fmt.Sprintf("unsupported type: %T", msg))
 }
 
-func (ctx *Context) JSONError(msg any) {
+func (ctx *Context) JSONErrorAuto(err error) {
+	if errTr := util.ErrorAsTranslatable(err); errTr != nil {
+		msg := errTr.Translate(ctx.Locale)
+		ctx.JSON(http.StatusBadRequest, buildJsonErrorMap(msg))
+		return
+	}
+	errMsg, httpCode := util.ErrorUnwrapForUser(err)
+	if errMsg != "" {
+		ctx.JSON(httpCode, buildJsonErrorMap(errMsg))
+		return
+	}
+	log.ErrorWithSkip(1, "JSONErrorAuto: server internal error: %v", err)
+	ctx.JSON(http.StatusInternalServerError, buildJsonErrorMap(ctx.Locale.TrString("error.occurred")))
+}
+
+func (ctx *Context) JSONError[T string | template.HTML](msg T) {
 	ctx.JSON(http.StatusBadRequest, buildJsonErrorMap(msg))
 }
 
-func (ctx *Context) JSONErrorWithField(msg any, field string) {
+func (ctx *Context) JSONErrorWithField[T string | template.HTML](msg T, field string) {
 	m := buildJsonErrorMap(msg)
 	m["errorFields"] = []string{field}
 	ctx.JSON(http.StatusBadRequest, m)

services/context/upload/upload.go

@@ -15,6 +15,7 @@ import (
 	"gitea.dev/modules/log"
 	"gitea.dev/modules/reqctx"
 	"gitea.dev/modules/setting"
+	"gitea.dev/modules/util"
 	"gitea.dev/services/context"
 )
 
@@ -33,6 +34,10 @@ func (err ErrFileTypeForbidden) Error() string {
 	return "This file cannot be uploaded or modified due to a forbidden file extension or type."
 }
 
+func (err ErrFileTypeForbidden) Unwrap() error {
+	return util.ErrInvalidArgument
+}
+
 var wildcardTypeRe = regexp.MustCompile(`^[a-z]+/\*$`)
 
 // Verify validates whether a file is allowed to be uploaded. If buf is empty, it will just check if the file

services/secrets/validation.go

@@ -17,17 +17,21 @@ var globalVars = sync.OnceValue(func() (ret struct {
 	namePattern, forbiddenPrefixPattern *regexp.Regexp
 },
 ) {
-	ret.namePattern = regexp.MustCompile("(?i)^[A-Z_][A-Z0-9_]*$")
-	ret.forbiddenPrefixPattern = regexp.MustCompile("(?i)^GIT(EA|HUB)_")
+	ret.namePattern = regexp.MustCompile(`^[a-zA-Z_][a-zA-Z0-9_]*$`)
+	ret.forbiddenPrefixPattern = regexp.MustCompile("(?i)^(GITEA_|GITHUB_)")
 	return ret
 })
 
 func ValidateName(name string) error {
 	vars := globalVars()
-	if !vars.namePattern.MatchString(name) ||
-		vars.forbiddenPrefixPattern.MatchString(name) ||
-		strings.EqualFold(name, "CI") /* CI is always set to true in GitHub Actions*/ {
-		return util.NewInvalidArgumentErrorf("invalid variable or secret name")
+	if !vars.namePattern.MatchString(name) {
+		return util.NewInvalidArgumentErrorf("name must start with a letter or underscore and contain only letters, numbers, and underscores")
+	}
+	if vars.forbiddenPrefixPattern.MatchString(name) {
+		return util.NewInvalidArgumentErrorf("name cannot start with 'GITEA_' or 'GITHUB_'")
+	}
+	if strings.EqualFold(name, "CI") {
+		return util.NewInvalidArgumentErrorf("'CI' is a reserved name")
 	}
 	return nil
 }

services/secrets/validation_test.go

@@ -11,19 +11,25 @@ import (
 
 func TestValidateName(t *testing.T) {
 	cases := []struct {
-		name  string
-		valid bool
+		name   string
+		errMsg string // substring that should appear in error message
 	}{
-		{"FOO", true},
-		{"FOO1_BAR2", true},
-		{"_FOO", true}, // really? why support this
-		{"1FOO", false},
-		{"giteA_xx", false},
-		{"githuB_xx", false},
-		{"cI", false},
+		{"FOO", ""},
+		{"FOO1_BAR2", ""},
+		{"_Foo", ""},
+
+		{"FOO.BAR", "contain only letters, numbers, and underscores"},
+		{"1FOO", "name must start with a letter or underscore"},
+		{"giteA_xx", "name cannot start with"},
+		{"githuB_xx", "name cannot start with"},
+		{"cI", "is a reserved name"},
 	}
 	for _, c := range cases {
 		err := ValidateName(c.name)
-		assert.Equal(t, c.valid, err == nil, "ValidateName(%q)", c.name)
+		if c.errMsg == "" {
+			assert.NoError(t, err, "ValidateName(%q) should be valid", c.name)
+		} else {
+			assert.ErrorContains(t, err, c.errMsg, "ValidateName(%q) error message should mention %q", c.name, c.errMsg)
+		}
 	}
 }