From 2d3ad4a530f822a274f036851d938d3e4b0fc041 Mon Sep 17 00:00:00 2001 From: Ross Golder Date: Fri, 4 Sep 2026 07:28:58 +0700 Subject: [PATCH] enhance: Improve validation errors for secrets/variables (#39221) Signed-off-by: Ross Golder Co-authored-by: wxiaoguang --- modules/util/error.go | 70 +++++++++++-------- modules/util/error_test.go | 17 +++++ options/locale/locale_en-US.json | 5 +- routers/api/v1/org/action.go | 48 ++----------- routers/api/v1/repo/action.go | 42 ++--------- routers/api/v1/user/action.go | 48 ++----------- routers/web/admin/config.go | 7 +- routers/web/org/members.go | 4 +- routers/web/repo/pull.go | 10 +-- routers/web/repo/release.go | 6 +- routers/web/shared/actions/variables.go | 8 +-- routers/web/shared/secrets/secrets.go | 3 +- services/context/api.go | 19 ++--- services/context/context.go | 24 +++++-- services/context/upload/upload.go | 5 ++ services/secrets/validation.go | 16 +++-- services/secrets/validation_test.go | 26 ++++--- templates/shared/secrets/add_list.tmpl | 4 +- templates/shared/variables/variable_list.tmpl | 4 +- 19 files changed, 148 insertions(+), 218 deletions(-) diff --git a/modules/util/error.go b/modules/util/error.go index d503cadf288..a24ccbddcd9 100644 --- a/modules/util/error.go +++ b/modules/util/error.go @@ -9,44 +9,65 @@ import ( "html/template" ) -// Common Errors forming the base of our error system -// -// Many Errors returned by Gitea can be tested against these errors using "errors.Is". -var ( - ErrInvalidArgument = errors.New("invalid argument") // also implies HTTP 400 - ErrPermissionDenied = errors.New("permission denied") // also implies HTTP 403 - ErrNotExist = errors.New("resource does not exist") // also implies HTTP 404 - ErrAlreadyExist = errors.New("resource already exists") // also implies HTTP 409 - ErrContentTooLarge = errors.New("content exceeds limit") // also implies HTTP 413 +// This file defines common errors forming the base of our error system. +// These errors can be used to classify errors and to provide a common, +// safe (no server-side sensitive information), and translatable error message for end users. - // ErrUnprocessableContent implies HTTP 422, the syntax of the request content is correct, - // but the server is unable to process the contained instructions - ErrUnprocessableContent = errors.New("unprocessable content") +// ErrorTranslatable wraps an error with translation information +type ErrorTranslatable interface { + error + Unwrap() error + Translate(ErrorLocaleTranslator) template.HTML +} + +var ( + ErrInvalidArgument = errorForUser{400, "invalid argument"} + ErrPermissionDenied = errorForUser{403, "permission denied"} + ErrNotExist = errorForUser{404, "resource does not exist"} + ErrAlreadyExist = errorForUser{409, "resource already exists"} // 409 Conflict + ErrContentTooLarge = errorForUser{413, "content exceeds limit"} // 413 Request Entity Too Large + + // ErrUnprocessableContent means request content is correct, but the server is unable to process the contained instructions + ErrUnprocessableContent = errorForUser{422, "unprocessable content"} // 422 Unprocessable Entity ) +type errorForUser struct { + code int // implies HTTP status code + msg string +} + +func (w errorForUser) Error() string { + return w.msg +} + +func ErrorUnwrapForUser(err error) (string, int) { + if e, ok := errors.AsType[errorForUser](err); ok { + return err.Error(), e.code + } + return "", 0 +} + // errorWrapper provides a simple wrapper for a wrapped error where the wrapped error message plays no part in the error message -// Especially useful for "untyped" errors created with "errors.New(…)" that can be classified as 'invalid argument', 'permission denied', 'exists already', or 'does not exist' type errorWrapper struct { - Message string - Err error + msg string + err error } -// Error returns the message func (w errorWrapper) Error() string { - return w.Message + return w.msg } -// Unwrap returns the underlying error func (w errorWrapper) Unwrap() error { - return w.Err + return w.err } // ErrorWrap returns an error that formats as the given text but unwraps as the provided error +// The message should be safe (no sensitive information) to be shown to end users func ErrorWrap(unwrap error, message string, args ...any) error { if len(args) == 0 { - return errorWrapper{Message: message, Err: unwrap} + return errorWrapper{msg: message, err: unwrap} } - return errorWrapper{Message: fmt.Sprintf(message, args...), Err: unwrap} + return errorWrapper{msg: fmt.Sprintf(message, args...), err: unwrap} } // NewInvalidArgumentErrorf returns an error that formats as the given text but unwraps as an ErrInvalidArgument @@ -69,13 +90,6 @@ func NewNotExistErrorf(message string, args ...any) error { return ErrorWrap(ErrNotExist, message, args...) } -// ErrorTranslatable wraps an error with translation information -type ErrorTranslatable interface { - error - Unwrap() error - Translate(ErrorLocaleTranslator) template.HTML -} - type errorTranslatableWrapper struct { err error trKey string diff --git a/modules/util/error_test.go b/modules/util/error_test.go index d4f541052c4..b52c063f6d1 100644 --- a/modules/util/error_test.go +++ b/modules/util/error_test.go @@ -4,6 +4,7 @@ package util import ( + "fmt" "io" "testing" @@ -32,3 +33,19 @@ func TestErrorTranslatable(t *testing.T) { require.True(t, ok) assert.Equal(t, "key", wrapped.trKey) } + +func TestErrorUnwrapForUser(t *testing.T) { + err := NewNotExistErrorf("test msg") + msg, code := ErrorUnwrapForUser(err) + assert.Equal(t, "test msg", msg) + assert.Equal(t, 404, code) + + err = fmt.Errorf("other wrapper: %w", err) + msg, code = ErrorUnwrapForUser(err) + assert.Equal(t, "other wrapper: test msg", msg) + assert.Equal(t, 404, code) + + msg, code = ErrorUnwrapForUser(io.EOF) + assert.Equal(t, "", msg) + assert.Equal(t, 0, code) +} diff --git a/options/locale/locale_en-US.json b/options/locale/locale_en-US.json index 1d947b6deb8..c3ec1353a06 100644 --- a/options/locale/locale_en-US.json +++ b/options/locale/locale_en-US.json @@ -3751,11 +3751,10 @@ "secrets.description": "Secrets will be passed to certain actions and cannot be read otherwise.", "secrets.none": "There are no secrets yet.", "secrets.creation.description": "Description", - "secrets.creation.name_placeholder": "case-insensitive, alphanumeric characters or underscores only, cannot start with GITEA_ or GITHUB_", + "secrets.creation.name_pattern": "Name must start with a letter or underscore, contain only letters, numbers, and underscores, and must not start with GITEA_ or GITHUB_", "secrets.creation.value_placeholder": "Input any content. Whitespace at the start and end will be omitted.", "secrets.creation.description_placeholder": "Enter short description (optional).", "secrets.save_success": "The secret \"%s\" has been saved.", - "secrets.save_failed": "Failed to save secret.", "secrets.add_secret": "Add secret", "secrets.edit_secret": "Edit secret", "secrets.deletion": "Remove secret", @@ -3932,9 +3931,7 @@ "actions.variables.edit": "Edit Variable", "actions.variables.deletion.failed": "Failed to remove variable.", "actions.variables.deletion.success": "The variable has been removed.", - "actions.variables.creation.failed": "Failed to add variable.", "actions.variables.creation.success": "The variable \"%s\" has been added.", - "actions.variables.update.failed": "Failed to edit variable.", "actions.variables.update.success": "The variable has been edited.", "actions.logs.always_auto_scroll": "Always auto scroll logs", "actions.logs.always_expand_running": "Always expand running logs", diff --git a/routers/api/v1/org/action.go b/routers/api/v1/org/action.go index e90bce5d57a..55ec043c048 100644 --- a/routers/api/v1/org/action.go +++ b/routers/api/v1/org/action.go @@ -110,13 +110,7 @@ func (Action) CreateOrUpdateSecret(ctx *context.APIContext) { _, created, err := secret_service.CreateOrUpdateSecret(ctx, ctx.Org.Organization.ID, 0, ctx.PathParam("secretname"), opt.Data, opt.Description) if err != nil { - if errors.Is(err, util.ErrInvalidArgument) { - ctx.APIError(http.StatusBadRequest, err.Error()) - } else if errors.Is(err, util.ErrNotExist) { - ctx.APIError(http.StatusNotFound, err.Error()) - } else { - ctx.APIErrorInternal(err) - } + ctx.APIErrorAuto(err) return } @@ -157,13 +151,7 @@ func (Action) DeleteSecret(ctx *context.APIContext) { err := secret_service.DeleteSecretByName(ctx, ctx.Org.Organization.ID, 0, ctx.PathParam("secretname")) if err != nil { - if errors.Is(err, util.ErrInvalidArgument) { - ctx.APIError(http.StatusBadRequest, err.Error()) - } else if errors.Is(err, util.ErrNotExist) { - ctx.APIError(http.StatusNotFound, err.Error()) - } else { - ctx.APIErrorInternal(err) - } + ctx.APIErrorAuto(err) return } @@ -276,11 +264,7 @@ func (Action) GetVariable(ctx *context.APIContext) { Name: ctx.PathParam("variablename"), }) if err != nil { - if errors.Is(err, util.ErrNotExist) { - ctx.APIError(http.StatusNotFound, err.Error()) - } else { - ctx.APIErrorInternal(err) - } + ctx.APIErrorAuto(err) return } @@ -326,13 +310,7 @@ func (Action) DeleteVariable(ctx *context.APIContext) { // "$ref": "#/responses/notFound" if err := actions_service.DeleteVariableByName(ctx, ctx.Org.Organization.ID, 0, ctx.PathParam("variablename")); err != nil { - if errors.Is(err, util.ErrInvalidArgument) { - ctx.APIError(http.StatusBadRequest, err.Error()) - } else if errors.Is(err, util.ErrNotExist) { - ctx.APIError(http.StatusNotFound, err.Error()) - } else { - ctx.APIErrorInternal(err) - } + ctx.APIErrorAuto(err) return } @@ -392,11 +370,7 @@ func (Action) CreateVariable(ctx *context.APIContext) { } if _, err := actions_service.CreateVariable(ctx, ownerID, 0, variableName, opt.Value, opt.Description); err != nil { - if errors.Is(err, util.ErrInvalidArgument) { - ctx.APIError(http.StatusBadRequest, err.Error()) - } else { - ctx.APIErrorInternal(err) - } + ctx.APIErrorAuto(err) return } @@ -444,11 +418,7 @@ func (Action) UpdateVariable(ctx *context.APIContext) { Name: ctx.PathParam("variablename"), }) if err != nil { - if errors.Is(err, util.ErrNotExist) { - ctx.APIError(http.StatusNotFound, err.Error()) - } else { - ctx.APIErrorInternal(err) - } + ctx.APIErrorAuto(err) return } @@ -461,11 +431,7 @@ func (Action) UpdateVariable(ctx *context.APIContext) { v.Description = opt.Description if _, err := actions_service.UpdateVariableNameData(ctx, v); err != nil { - if errors.Is(err, util.ErrInvalidArgument) { - ctx.APIError(http.StatusBadRequest, err.Error()) - } else { - ctx.APIErrorInternal(err) - } + ctx.APIErrorAuto(err) return } diff --git a/routers/api/v1/repo/action.go b/routers/api/v1/repo/action.go index ceef3d54661..a9813d11ff1 100644 --- a/routers/api/v1/repo/action.go +++ b/routers/api/v1/repo/action.go @@ -139,13 +139,7 @@ func (Action) CreateOrUpdateSecret(ctx *context.APIContext) { _, created, err := secret_service.CreateOrUpdateSecret(ctx, 0, repo.ID, ctx.PathParam("secretname"), opt.Data, opt.Description) if err != nil { - if errors.Is(err, util.ErrInvalidArgument) { - ctx.APIError(http.StatusBadRequest, err.Error()) - } else if errors.Is(err, util.ErrNotExist) { - ctx.APIError(http.StatusNotFound, err.Error()) - } else { - ctx.APIErrorInternal(err) - } + ctx.APIErrorAuto(err) return } @@ -193,13 +187,7 @@ func (Action) DeleteSecret(ctx *context.APIContext) { err := secret_service.DeleteSecretByName(ctx, 0, repo.ID, ctx.PathParam("secretname")) if err != nil { - if errors.Is(err, util.ErrInvalidArgument) { - ctx.APIError(http.StatusBadRequest, err.Error()) - } else if errors.Is(err, util.ErrNotExist) { - ctx.APIError(http.StatusNotFound, err.Error()) - } else { - ctx.APIErrorInternal(err) - } + ctx.APIErrorAuto(err) return } @@ -296,13 +284,7 @@ func (Action) DeleteVariable(ctx *context.APIContext) { // "$ref": "#/responses/notFound" if err := actions_service.DeleteVariableByName(ctx, 0, ctx.Repo.Repository.ID, ctx.PathParam("variablename")); err != nil { - if errors.Is(err, util.ErrInvalidArgument) { - ctx.APIError(http.StatusBadRequest, err.Error()) - } else if errors.Is(err, util.ErrNotExist) { - ctx.APIError(http.StatusNotFound, err.Error()) - } else { - ctx.APIErrorInternal(err) - } + ctx.APIErrorAuto(err) return } @@ -365,11 +347,7 @@ func (Action) CreateVariable(ctx *context.APIContext) { } if _, err := actions_service.CreateVariable(ctx, 0, repoID, variableName, opt.Value, opt.Description); err != nil { - if errors.Is(err, util.ErrInvalidArgument) { - ctx.APIError(http.StatusBadRequest, err.Error()) - } else { - ctx.APIErrorInternal(err) - } + ctx.APIErrorAuto(err) return } @@ -420,11 +398,7 @@ func (Action) UpdateVariable(ctx *context.APIContext) { Name: ctx.PathParam("variablename"), }) if err != nil { - if errors.Is(err, util.ErrNotExist) { - ctx.APIError(http.StatusNotFound, err.Error()) - } else { - ctx.APIErrorInternal(err) - } + ctx.APIErrorAuto(err) return } @@ -437,11 +411,7 @@ func (Action) UpdateVariable(ctx *context.APIContext) { v.Description = opt.Description if _, err := actions_service.UpdateVariableNameData(ctx, v); err != nil { - if errors.Is(err, util.ErrInvalidArgument) { - ctx.APIError(http.StatusBadRequest, err.Error()) - } else { - ctx.APIErrorInternal(err) - } + ctx.APIErrorAuto(err) return } diff --git a/routers/api/v1/user/action.go b/routers/api/v1/user/action.go index 38414e4a4d3..0974c7a2306 100644 --- a/routers/api/v1/user/action.go +++ b/routers/api/v1/user/action.go @@ -52,13 +52,7 @@ func CreateOrUpdateSecret(ctx *context.APIContext) { _, created, err := secret_service.CreateOrUpdateSecret(ctx, ctx.Doer.ID, 0, ctx.PathParam("secretname"), opt.Data, opt.Description) if err != nil { - if errors.Is(err, util.ErrInvalidArgument) { - ctx.APIError(http.StatusBadRequest, err.Error()) - } else if errors.Is(err, util.ErrNotExist) { - ctx.APIError(http.StatusNotFound, err.Error()) - } else { - ctx.APIErrorInternal(err) - } + ctx.APIErrorAuto(err) return } @@ -94,13 +88,7 @@ func DeleteSecret(ctx *context.APIContext) { err := secret_service.DeleteSecretByName(ctx, ctx.Doer.ID, 0, ctx.PathParam("secretname")) if err != nil { - if errors.Is(err, util.ErrInvalidArgument) { - ctx.APIError(http.StatusBadRequest, err.Error()) - } else if errors.Is(err, util.ErrNotExist) { - ctx.APIError(http.StatusNotFound, err.Error()) - } else { - ctx.APIErrorInternal(err) - } + ctx.APIErrorAuto(err) return } @@ -153,11 +141,7 @@ func CreateVariable(ctx *context.APIContext) { } if _, err := actions_service.CreateVariable(ctx, ownerID, 0, variableName, opt.Value, opt.Description); err != nil { - if errors.Is(err, util.ErrInvalidArgument) { - ctx.APIError(http.StatusBadRequest, err.Error()) - } else { - ctx.APIErrorInternal(err) - } + ctx.APIErrorAuto(err) return } @@ -200,11 +184,7 @@ func UpdateVariable(ctx *context.APIContext) { Name: ctx.PathParam("variablename"), }) if err != nil { - if errors.Is(err, util.ErrNotExist) { - ctx.APIError(http.StatusNotFound, err.Error()) - } else { - ctx.APIErrorInternal(err) - } + ctx.APIErrorAuto(err) return } @@ -217,11 +197,7 @@ func UpdateVariable(ctx *context.APIContext) { v.Description = opt.Description if _, err := actions_service.UpdateVariableNameData(ctx, v); err != nil { - if errors.Is(err, util.ErrInvalidArgument) { - ctx.APIError(http.StatusBadRequest, err.Error()) - } else { - ctx.APIErrorInternal(err) - } + ctx.APIErrorAuto(err) return } @@ -252,13 +228,7 @@ func DeleteVariable(ctx *context.APIContext) { // "$ref": "#/responses/notFound" if err := actions_service.DeleteVariableByName(ctx, ctx.Doer.ID, 0, ctx.PathParam("variablename")); err != nil { - if errors.Is(err, util.ErrInvalidArgument) { - ctx.APIError(http.StatusBadRequest, err.Error()) - } else if errors.Is(err, util.ErrNotExist) { - ctx.APIError(http.StatusNotFound, err.Error()) - } else { - ctx.APIErrorInternal(err) - } + ctx.APIErrorAuto(err) return } @@ -291,11 +261,7 @@ func GetVariable(ctx *context.APIContext) { Name: ctx.PathParam("variablename"), }) if err != nil { - if errors.Is(err, util.ErrNotExist) { - ctx.APIError(http.StatusNotFound, err.Error()) - } else { - ctx.APIErrorInternal(err) - } + ctx.APIErrorAuto(err) return } diff --git a/routers/web/admin/config.go b/routers/web/admin/config.go index d7c11843d5b..74124b19990 100644 --- a/routers/web/admin/config.go +++ b/routers/web/admin/config.go @@ -5,7 +5,6 @@ package admin import ( - "errors" "net/http" system_model "gitea.dev/models/system" @@ -161,11 +160,7 @@ loop: err := validateConfigKeyValue(key, value) if err != nil { - if errors.Is(err, util.ErrInvalidArgument) { - ctx.JSONError(err.Error()) - } else { - ctx.JSONError(ctx.Tr("admin.config.set_setting_failed", key)) - } + ctx.JSONErrorAuto(err) break loop } configSettings[key] = value diff --git a/routers/web/org/members.go b/routers/web/org/members.go index 79c7e20d72b..181593af91f 100644 --- a/routers/web/org/members.go +++ b/routers/web/org/members.go @@ -10,7 +10,6 @@ import ( "gitea.dev/models/organization" user_model "gitea.dev/models/user" - "gitea.dev/modules/log" "gitea.dev/modules/setting" "gitea.dev/modules/templates" "gitea.dev/modules/util" @@ -130,6 +129,5 @@ func MembersAction(ctx *context.Context) { return } - log.Error("Action(%s): %v", ctx.PathParam("action"), err) - ctx.JSONError(err.Error()) // FIXME: legacy logic, errors are handled together, it's not right, need to distinguish between different errors + ctx.JSONErrorAuto(err) } diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 4ea89b7d84b..ba92e5a8a7f 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1319,14 +1319,8 @@ func CompareAndPullRequestPost(ctx *context.Context) { repo := ctx.Repo.Repository comparePageInfo := newComparePageInfo() err := comparePageInfo.parseCompareInfo(ctx, ctx.PathParam("*")) - if errors.Is(err, util.ErrNotExist) { - ctx.JSONErrorNotFound() - return - } else if errors.Is(err, util.ErrInvalidArgument) { - ctx.JSONError(err.Error()) - return - } else if err != nil { - ctx.ServerError("ParseCompareInfo", err) + if err != nil { + ctx.JSONErrorAuto(err) return } ci := comparePageInfo.compareInfo diff --git a/routers/web/repo/release.go b/routers/web/repo/release.go index 712c1311e9a..edf029f5b67 100644 --- a/routers/web/repo/release.go +++ b/routers/web/repo/release.go @@ -623,11 +623,7 @@ func EditReleasePost(ctx *context.Context) { rel.IsPrerelease = form.Prerelease if err = release_service.UpdateRelease(ctx, ctx.Doer, ctx.Repo.GitRepo, rel, addAttachmentUUIDs, delAttachmentUUIDs, editAttachments); err != nil { - if upload.IsErrFileTypeForbidden(err) { - ctx.JSONError(err.Error()) - } else { - ctx.ServerError("UpdateRelease", err) - } + ctx.JSONErrorAuto(err) return } ctx.JSONRedirect(ctx.Repo.RepoLink + "/releases") diff --git a/routers/web/shared/actions/variables.go b/routers/web/shared/actions/variables.go index de3940d317f..d999cac0050 100644 --- a/routers/web/shared/actions/variables.go +++ b/routers/web/shared/actions/variables.go @@ -126,8 +126,7 @@ func VariableCreate(ctx *context.Context) { v, err := actions_service.CreateVariable(ctx, vCtx.OwnerID, vCtx.RepoID, form.Name, form.Data, form.Description) if err != nil { - log.Error("CreateVariable: %v", err) - ctx.JSONError(ctx.Tr("actions.variables.creation.failed")) + ctx.JSONErrorAuto(err) return } @@ -159,9 +158,8 @@ func VariableUpdate(ctx *context.Context) { variable.Data = form.Data variable.Description = form.Description - if ok, err := actions_service.UpdateVariableNameData(ctx, variable); err != nil || !ok { - log.Error("UpdateVariable: %v", err) - ctx.JSONError(ctx.Tr("actions.variables.update.failed")) + if _, err := actions_service.UpdateVariableNameData(ctx, variable); err != nil { + ctx.JSONErrorAuto(err) return } ctx.Flash.Success(ctx.Tr("actions.variables.update.success")) diff --git a/routers/web/shared/secrets/secrets.go b/routers/web/shared/secrets/secrets.go index 88078a35436..6ad48492d44 100644 --- a/routers/web/shared/secrets/secrets.go +++ b/routers/web/shared/secrets/secrets.go @@ -31,8 +31,7 @@ func PerformSecretsPost(ctx *context.Context, ownerID, repoID int64, redirectURL s, _, err := secret_service.CreateOrUpdateSecret(ctx, ownerID, repoID, form.Name, util.NormalizeStringEOL(form.Data), form.Description) if err != nil { - log.Error("CreateOrUpdateSecret failed: %v", err) - ctx.JSONError(ctx.Tr("secrets.save_failed")) + ctx.JSONErrorAuto(err) return } diff --git a/services/context/api.go b/services/context/api.go index 47e1cc43431..5affd8b2778 100644 --- a/services/context/api.go +++ b/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{} diff --git a/services/context/context.go b/services/context/context.go index 6cc1fe0e984..f13d51c55f1 100644 --- a/services/context/context.go +++ b/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) diff --git a/services/context/upload/upload.go b/services/context/upload/upload.go index f5282b64dd6..9ac37b4a7a7 100644 --- a/services/context/upload/upload.go +++ b/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 diff --git a/services/secrets/validation.go b/services/secrets/validation.go index 6b711f63d82..f062e22f07a 100644 --- a/services/secrets/validation.go +++ b/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 } diff --git a/services/secrets/validation_test.go b/services/secrets/validation_test.go index 6ee47513dc7..cc8a54aa142 100644 --- a/services/secrets/validation_test.go +++ b/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) + } } } diff --git a/templates/shared/secrets/add_list.tmpl b/templates/shared/secrets/add_list.tmpl index 3e4a86a1183..c6ed1c31627 100644 --- a/templates/shared/secrets/add_list.tmpl +++ b/templates/shared/secrets/add_list.tmpl @@ -78,9 +78,9 @@
diff --git a/templates/shared/variables/variable_list.tmpl b/templates/shared/variables/variable_list.tmpl index aa8b561f0e6..0387ad3fc34 100644 --- a/templates/shared/variables/variable_list.tmpl +++ b/templates/shared/variables/variable_list.tmpl @@ -76,9 +76,9 @@