diff --git a/models/user/email_address.go b/models/user/email_address.go index 7365f5374e0..6f5ff8fdaa1 100644 --- a/models/user/email_address.go +++ b/models/user/email_address.go @@ -7,7 +7,6 @@ package user import ( "context" "fmt" - "net/mail" "strings" "time" @@ -22,45 +21,6 @@ import ( "xorm.io/builder" ) -// ErrEmailCharIsNotSupported e-mail address contains unsupported character -type ErrEmailCharIsNotSupported struct { - Email string -} - -// IsErrEmailCharIsNotSupported checks if an error is an ErrEmailCharIsNotSupported -func IsErrEmailCharIsNotSupported(err error) bool { - _, ok := err.(ErrEmailCharIsNotSupported) - return ok -} - -func (err ErrEmailCharIsNotSupported) Error() string { - return fmt.Sprintf("e-mail address contains unsupported character [email: %s]", err.Email) -} - -func (err ErrEmailCharIsNotSupported) Unwrap() error { - return util.ErrInvalidArgument -} - -// ErrEmailInvalid represents an error where the email address does not comply with RFC 5322 -// or has a leading '-' character -type ErrEmailInvalid struct { - Email string -} - -// IsErrEmailInvalid checks if an error is an ErrEmailInvalid -func IsErrEmailInvalid(err error) bool { - _, ok := err.(ErrEmailInvalid) - return ok -} - -func (err ErrEmailInvalid) Error() string { - return fmt.Sprintf("e-mail invalid [email: %s]", err.Email) -} - -func (err ErrEmailInvalid) Unwrap() error { - return util.ErrInvalidArgument -} - // ErrEmailAlreadyUsed represents a "EmailAlreadyUsed" kind of error. type ErrEmailAlreadyUsed struct { Email string @@ -147,18 +107,34 @@ func InsertEmailAddress(ctx context.Context, email *EmailAddress) (*EmailAddress return email, nil } +type ErrEmailInvalid string + +func (err ErrEmailInvalid) Error() string { + return string(err) +} + +func (err ErrEmailInvalid) Unwrap() error { + return util.ErrInvalidArgument +} + // ValidateEmail check if email is a valid & allowed address func ValidateEmail(email string) error { - if err := validateEmailBasic(email); err != nil { - return err + if !validation.IsEmailAddressValid(email) { + return ErrEmailInvalid("email address is invalid: " + email) } - return validateEmailDomain(email) + if !IsEmailDomainAllowed(email) { + return ErrEmailInvalid("email domain is not allowed: " + email) + } + return nil } // ValidateEmailForAdmin check if email is a valid address when admins manually add or edit users func ValidateEmailForAdmin(email string) error { - return validateEmailBasic(email) // In this case we do not need to check the email domain + if !validation.IsEmailAddressValid(email) { + return ErrEmailInvalid("email address is invalid: " + email) + } + return nil } func GetEmailAddressByEmail(ctx context.Context, email string) (*EmailAddress, error) { @@ -491,37 +467,11 @@ func ActivateUserEmail(ctx context.Context, userID int64, email string, activate }) } -// validateEmailBasic checks whether the email complies with the rules -func validateEmailBasic(email string) error { - if len(email) == 0 { - return ErrEmailInvalid{email} - } - - if !globalVars().emailRegexp.MatchString(email) { - return ErrEmailCharIsNotSupported{email} - } - - if email[0] == '-' { - return ErrEmailInvalid{email} - } - - if _, err := mail.ParseAddress(email); err != nil { - return ErrEmailInvalid{email} - } - - return nil -} - -// validateEmailDomain checks whether the email domain is allowed or blocked -func validateEmailDomain(email string) error { - if !IsEmailDomainAllowed(email) { - return ErrEmailInvalid{email} - } - - return nil -} - func IsEmailDomainAllowed(email string) bool { + localPart, _, _ := strings.CutLast(email, "@") + if strings.ContainsAny(localPart, "%!") && (len(setting.Service.EmailDomainAllowList) > 0 || len(setting.Service.EmailDomainBlockList) > 0) { + return false // percent-hack and bang-path local parts can route mail to a domain other than the listed one + } if len(setting.Service.EmailDomainAllowList) == 0 { return !validation.IsEmailDomainListed(setting.Service.EmailDomainBlockList, email) } diff --git a/models/user/email_address_test.go b/models/user/email_address_test.go index 06ab883779d..4ced7c7f3d5 100644 --- a/models/user/email_address_test.go +++ b/models/user/email_address_test.go @@ -150,58 +150,22 @@ func TestListEmails(t *testing.T) { } func TestEmailAddressValidate(t *testing.T) { - kases := map[string]error{ - "abc@gmail.com": nil, - "132@hotmail.com": nil, - "1-3-2@test.org": nil, - "1.3.2@test.org": nil, - "a_123@test.org.cn": nil, - `first.last@iana.org`: nil, - `first!last@iana.org`: nil, - `first#last@iana.org`: nil, - `first$last@iana.org`: nil, - `first%last@iana.org`: nil, - `first&last@iana.org`: nil, - `first'last@iana.org`: nil, - `first*last@iana.org`: nil, - `first+last@iana.org`: nil, - `first/last@iana.org`: nil, - `first=last@iana.org`: nil, - `first?last@iana.org`: nil, - `first^last@iana.org`: nil, - "first`last@iana.org": nil, - `first{last@iana.org`: nil, - `first|last@iana.org`: nil, - `first}last@iana.org`: nil, - `first~last@iana.org`: nil, - `first;last@iana.org`: user_model.ErrEmailCharIsNotSupported{`first;last@iana.org`}, - ".233@qq.com": user_model.ErrEmailInvalid{".233@qq.com"}, - "!233@qq.com": nil, - "#233@qq.com": nil, - "$233@qq.com": nil, - "%233@qq.com": nil, - "&233@qq.com": nil, - "'233@qq.com": nil, - "*233@qq.com": nil, - "+233@qq.com": nil, - "-233@qq.com": user_model.ErrEmailInvalid{"-233@qq.com"}, - "/233@qq.com": nil, - "=233@qq.com": nil, - "?233@qq.com": nil, - "^233@qq.com": nil, - "_233@qq.com": nil, - "`233@qq.com": nil, - "{233@qq.com": nil, - "|233@qq.com": nil, - "}233@qq.com": nil, - "~233@qq.com": nil, - ";233@qq.com": user_model.ErrEmailCharIsNotSupported{";233@qq.com"}, - "Foo ": user_model.ErrEmailCharIsNotSupported{"Foo "}, - string([]byte{0xE2, 0x84, 0xAA}): user_model.ErrEmailCharIsNotSupported{string([]byte{0xE2, 0x84, 0xAA})}, + cases := map[string]bool{ + "": false, + "root@localhost": true, + "user@[192.168.1.2]": true, + "@a": false, + "abc@gmail.com": true, + "abc@gmail.com\n": false, + "Foo ": false, + "abc@gmail.com (x)": false, + "jürgen@example.com": false, + "a@foo_bar.com": false, } - for kase, err := range kases { - t.Run(kase, func(t *testing.T) { - assert.Equal(t, err, user_model.ValidateEmail(kase)) + for tc, isValid := range cases { + t.Run(tc, func(t *testing.T) { + err := user_model.ValidateEmail(tc) + assert.Equal(t, err == nil, isValid) }) } } diff --git a/models/user/user.go b/models/user/user.go index 09358e7093c..c30be783a37 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -553,7 +553,6 @@ type globalVarsStruct struct { transformDiacritics transform.Transformer replaceCharsHyphenRE *regexp.Regexp emailToReplacer *strings.Replacer - emailRegexp *regexp.Regexp systemUserNewFuncs map[int64]func() *User systemUserNameIdMap map[string]int64 } @@ -577,7 +576,6 @@ var globalVars = sync.OnceValue(func() *globalVarsStruct { ":", "", ";", "", ), - emailRegexp: regexp.MustCompile("^[a-zA-Z0-9.!#$%&'*+-/=?^_`{|}~]*@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*$"), } userFuncs := []func() *User{NewGhostUser, NewActionsUser, NewDeployKeyUser, NewCliUser, NewAuthSourceUser} diff --git a/models/user/user_test.go b/models/user/user_test.go index 2bf32a50382..49eec8e5deb 100644 --- a/models/user/user_test.go +++ b/models/user/user_test.go @@ -297,21 +297,6 @@ func TestDisplayName(t *testing.T) { } } -func TestCreateUserInvalidEmail(t *testing.T) { - user := &user_model.User{ - Name: "GiteaBot", - Email: "GiteaBot@gitea.io\r\n", - Passwd: ";p['////..-++']", - IsAdmin: false, - Theme: setting.UI.DefaultTheme, - MustChangePassword: false, - } - - err := user_model.CreateUser(t.Context(), user, &user_model.Meta{}) - assert.Error(t, err) - assert.True(t, user_model.IsErrEmailCharIsNotSupported(err)) -} - func TestCreateUserEmailAlreadyUsed(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) diff --git a/modules/validation/binding.go b/modules/validation/binding.go index 18903423e5d..0cd92fbff11 100644 --- a/modules/validation/binding.go +++ b/modules/validation/binding.go @@ -48,8 +48,21 @@ func newFieldError(field reflect.StructField, cls, msg string) *BindingError { return &BindingError{[]string{field.Name}, cls, msg} //nolint:govet // make sure no missing fields } +func AddValidationError(errs BindingErrors, fieldName, errorMsg string) BindingErrors { + errs.Add([]string{fieldName}, ErrCustomMessage, errorMsg) + return errs +} + // AddBindingRules adds additional binding rules func AddBindingRules(b *binding.Binder) { + b.ClearRules("Email") + b.AddRuleNonZero("Email", func(_ context.Context, f *binding.ValidationField) *binding.Error { + if !IsEmailAddressValid(f.ValueMustString()) { + return newFieldError(f.StructField, binding.ERR_EMAIL, "invalid email") + } + return nil + }) + b.AddRuleNonZero("GitRefName", func(ctx context.Context, f *binding.ValidationField) *binding.Error { if !git.IsValidRefPattern(f.ValueMustString()) { return newFieldError(f.StructField, ErrGitRefName, "GitRefName") diff --git a/modules/validation/binding_test.go b/modules/validation/binding_test.go index e3b0187a577..90a72c95e0a 100644 --- a/modules/validation/binding_test.go +++ b/modules/validation/binding_test.go @@ -21,9 +21,16 @@ type ( URL string `form:"ValidUrl" binding:"ValidUrl"` GlobPattern string `form:"GlobPattern" binding:"GlobPattern"` RegexPattern string `form:"RegexPattern" binding:"RegexPattern"` + Email string `form:"Email" binding:"Email"` } ) func performValidationTest(t *testing.T, testCase validationTestCase) { assert.Equal(t, testCase.expectedErrors, Binder().Validate(t.Context(), testCase.data)) } + +func TestEmailValidation(t *testing.T) { + assert.Nil(t, Binder().Validate(t.Context(), &TestForm{Email: "b@a"})) + assert.Equal(t, BindingErrors{{FieldNames: []string{"Email"}, Classification: "EmailError", Message: "invalid email"}}, + Binder().Validate(t.Context(), &TestForm{Email: "abc"})) +} diff --git a/modules/validation/helpers.go b/modules/validation/helpers.go index a84d08ca97e..30b8484d0d7 100644 --- a/modules/validation/helpers.go +++ b/modules/validation/helpers.go @@ -4,14 +4,18 @@ package validation import ( + "net/mail" "net/url" "regexp" "slices" "strings" "sync" + "unicode/utf8" "gitea.dev/modules/glob" "gitea.dev/modules/setting" + + "golang.org/x/net/idna" ) type globalVarsStruct struct { @@ -108,3 +112,22 @@ func IsValidBadgeSlug(slug string) bool { vars := globalVars() return vars.validBadgeSlugPattern.MatchString(slug) && !vars.invalidBadgeSlugPattern.MatchString(slug) } + +func IsEmailAddressValid(email string) bool { + if strings.ContainsFunc(email, func(r rune) bool { return r >= utf8.RuneSelf }) { + // At the moment, we don't support UTF8 email address. To support it, need to correctly handle IDN/punycode + return false + } + addr, err := mail.ParseAddress(email) + if err != nil || addr.Address != email { + // email must be parseable, and the "email" string must be the address, no other parts + return false + } + _, domain, _ := strings.Cut(email, "@") + if strings.HasPrefix(domain, "[") { + // address like "foo@[192.168.1.2]" + return true + } + _, err = idna.Registration.ToASCII(domain) + return err == nil +} diff --git a/modules/web/middleware/binding.go b/modules/web/middleware/binding.go index 1f9321cf786..e19a309ea96 100644 --- a/modules/web/middleware/binding.go +++ b/modules/web/middleware/binding.go @@ -88,11 +88,6 @@ func getRuleBody(field reflect.StructField, ruleName string) string { return "" } -func AddValidationError(errs validation.BindingErrors, fieldName, errorMsg string) validation.BindingErrors { - errs.Add([]string{fieldName}, validation.ErrCustomMessage, errorMsg) - return errs -} - func getFieldDisplayNameForMessage(f any, l translation.Locale, fieldNames []string) (field reflect.StructField, ok bool, displayName string) { if len(fieldNames) == 0 { return field, false, "" diff --git a/routers/api/v1/admin/user.go b/routers/api/v1/admin/user.go index ce7bb0896f8..9dc359500bb 100644 --- a/routers/api/v1/admin/user.go +++ b/routers/api/v1/admin/user.go @@ -74,6 +74,8 @@ func CreateUser(ctx *context.APIContext) { // "$ref": "#/responses/error" // "403": // "$ref": "#/responses/forbidden" + // "409": + // "$ref": "#/responses/error" // "422": // "$ref": "#/responses/validationError" @@ -136,17 +138,7 @@ func CreateUser(ctx *context.APIContext) { } if err := user_model.AdminCreateUser(ctx, u, &user_model.Meta{}, overwriteDefault); err != nil { - if user_model.IsErrUserAlreadyExist(err) || - user_model.IsErrEmailAlreadyUsed(err) || - db.IsErrNameReserved(err) || - db.IsErrNameCharsNotAllowed(err) || - user_model.IsErrEmailCharIsNotSupported(err) || - user_model.IsErrEmailInvalid(err) || - db.IsErrNamePatternNotAllowed(err) { - ctx.APIError(http.StatusUnprocessableEntity, err.Error()) - } else { - ctx.APIErrorInternal(err) - } + ctx.APIErrorAuto(err) return } @@ -191,6 +183,8 @@ func EditUser(ctx *context.APIContext) { // "$ref": "#/responses/error" // "403": // "$ref": "#/responses/forbidden" + // "409": + // "$ref": "#/responses/error" // "422": // "$ref": "#/responses/validationError" @@ -219,17 +213,7 @@ func EditUser(ctx *context.APIContext) { if form.Email != nil { if err := user_service.ReplacePrimaryEmailAddress(ctx, ctx.ContextUser, *form.Email); err != nil { - switch { - case user_model.IsErrEmailCharIsNotSupported(err), user_model.IsErrEmailInvalid(err): - if !user_model.IsEmailDomainAllowed(*form.Email) { - err = fmt.Errorf("the domain of user email %s conflicts with EMAIL_DOMAIN_ALLOWLIST or EMAIL_DOMAIN_BLOCKLIST", *form.Email) - } - ctx.APIError(http.StatusBadRequest, err.Error()) - case user_model.IsErrEmailAlreadyUsed(err): - ctx.APIError(http.StatusBadRequest, err.Error()) - default: - ctx.APIErrorInternal(err) - } + ctx.APIErrorAuto(err) return } } diff --git a/routers/api/v1/user/email.go b/routers/api/v1/user/email.go index 3d92a82dca0..b642e81e06a 100644 --- a/routers/api/v1/user/email.go +++ b/routers/api/v1/user/email.go @@ -4,7 +4,6 @@ package user import ( - "fmt" "net/http" user_model "gitea.dev/models/user" @@ -55,6 +54,10 @@ func AddEmail(ctx *context.APIContext) { // responses: // '201': // "$ref": "#/responses/EmailList" + // "400": + // "$ref": "#/responses/error" + // "409": + // "$ref": "#/responses/error" // "422": // "$ref": "#/responses/validationError" @@ -70,22 +73,7 @@ func AddEmail(ctx *context.APIContext) { } if err := user_service.AddEmailAddresses(ctx, ctx.Doer, form.Emails); err != nil { - if errEmailAlreadyUsed, ok := err.(user_model.ErrEmailAlreadyUsed); ok { - ctx.APIError(http.StatusUnprocessableEntity, "Email address has been used: "+errEmailAlreadyUsed.Email) - } else if user_model.IsErrEmailCharIsNotSupported(err) || user_model.IsErrEmailInvalid(err) { - email := "" - if typedError, ok := err.(user_model.ErrEmailInvalid); ok { - email = typedError.Email - } - if typedError, ok := err.(user_model.ErrEmailCharIsNotSupported); ok { - email = typedError.Email - } - - errMsg := fmt.Sprintf("Email address %q invalid", email) - ctx.APIError(http.StatusUnprocessableEntity, errMsg) - } else { - ctx.APIErrorInternal(err) - } + ctx.APIErrorAuto(err) return } diff --git a/routers/web/admin/users.go b/routers/web/admin/users.go index fadbe4692b0..7d6f86975d9 100644 --- a/routers/web/admin/users.go +++ b/routers/web/admin/users.go @@ -23,6 +23,7 @@ import ( "gitea.dev/modules/optional" "gitea.dev/modules/setting" "gitea.dev/modules/templates" + "gitea.dev/modules/util" "gitea.dev/modules/web" "gitea.dev/routers/web/explore" user_setting "gitea.dev/routers/web/user/setting" @@ -176,6 +177,7 @@ func NewUserPost(ctx *context.Context) { var errNameReserved db.ErrNameReserved var errNamePatternNotAllowed db.ErrNamePatternNotAllowed var errNameCharsNotAllowed db.ErrNameCharsNotAllowed + var errEmailInvalid user_model.ErrEmailInvalid switch { case user_model.IsErrUserAlreadyExist(err): ctx.Data["Err_UserName"] = true @@ -183,7 +185,7 @@ func NewUserPost(ctx *context.Context) { case user_model.IsErrEmailAlreadyUsed(err): ctx.Data["Err_Email"] = true ctx.RenderWithErrDeprecated(ctx.Tr("form.email_been_used"), tplUserNew, &form) - case user_model.IsErrEmailInvalid(err), user_model.IsErrEmailCharIsNotSupported(err): + case errors.As(err, &errEmailInvalid): ctx.Data["Err_Email"] = true ctx.RenderWithErrDeprecated(ctx.Tr("form.email_invalid"), tplUserNew, &form) case errors.As(err, &errNameReserved): @@ -409,12 +411,12 @@ func EditUserPost(ctx *context.Context) { if form.Email != "" { if err := user_service.ReplacePrimaryEmailAddress(ctx, u, form.Email); err != nil { switch { - case user_model.IsErrEmailCharIsNotSupported(err), user_model.IsErrEmailInvalid(err): - ctx.Data["Err_Email"] = true - ctx.RenderWithErrDeprecated(ctx.Tr("form.email_invalid"), tplUserEdit, &form) case user_model.IsErrEmailAlreadyUsed(err): ctx.Data["Err_Email"] = true ctx.RenderWithErrDeprecated(ctx.Tr("form.email_been_used"), tplUserEdit, &form) + case errors.Is(err, util.ErrInvalidArgument): + ctx.Data["Err_Email"] = true + ctx.RenderWithErrDeprecated(ctx.Tr("form.email_invalid"), tplUserEdit, &form) default: ctx.ServerError("AddOrSetPrimaryEmailAddress", err) } diff --git a/routers/web/auth/auth.go b/routers/web/auth/auth.go index e29d61eff95..54ab556d986 100644 --- a/routers/web/auth/auth.go +++ b/routers/web/auth/auth.go @@ -659,6 +659,7 @@ func createUserInContext(ctx *context.Context, tpl templates.TplName, form any, var errNameReserved db.ErrNameReserved var errNamePatternNotAllowed db.ErrNamePatternNotAllowed var errNameCharsNotAllowed db.ErrNameCharsNotAllowed + var errEmailInvalid user_model.ErrEmailInvalid switch { case user_model.IsErrUserAlreadyExist(err): ctx.Data["Err_UserName"] = true @@ -666,10 +667,7 @@ func createUserInContext(ctx *context.Context, tpl templates.TplName, form any, case user_model.IsErrEmailAlreadyUsed(err): ctx.Data["Err_Email"] = true ctx.RenderWithErrDeprecated(ctx.Tr("form.email_been_used"), tpl, form) - case user_model.IsErrEmailCharIsNotSupported(err): - ctx.Data["Err_Email"] = true - ctx.RenderWithErrDeprecated(ctx.Tr("form.email_invalid"), tpl, form) - case user_model.IsErrEmailInvalid(err): + case errors.As(err, &errEmailInvalid): ctx.Data["Err_Email"] = true ctx.RenderWithErrDeprecated(ctx.Tr("form.email_invalid"), tpl, form) case errors.As(err, &errNameReserved): diff --git a/routers/web/user/setting/account.go b/routers/web/user/setting/account.go index 97e6e43b687..4d610ac8f1d 100644 --- a/routers/web/user/setting/account.go +++ b/routers/web/user/setting/account.go @@ -19,6 +19,7 @@ import ( "gitea.dev/modules/setting" "gitea.dev/modules/templates" "gitea.dev/modules/timeutil" + "gitea.dev/modules/util" "gitea.dev/modules/web" "gitea.dev/services/auth" "gitea.dev/services/auth/source/db" @@ -187,7 +188,7 @@ func EmailPost(ctx *context.Context) { loadAccountData(ctx) ctx.RenderWithErrDeprecated(ctx.Tr("form.email_been_used"), tplSettingsAccount, &form) - } else if user_model.IsErrEmailCharIsNotSupported(err) || user_model.IsErrEmailInvalid(err) { + } else if errors.Is(err, util.ErrInvalidArgument) { loadAccountData(ctx) ctx.RenderWithErrDeprecated(ctx.Tr("form.email_invalid"), tplSettingsAccount, &form) diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go index 6bbdcccda18..3fdbb5b2cf2 100644 --- a/services/forms/repo_form.go +++ b/services/forms/repo_form.go @@ -264,7 +264,7 @@ type NewSlackHookForm struct { func (f *NewSlackHookForm) Validate(ctx *middleware.ValidateContext, errs validation.BindingErrors) validation.BindingErrors { if !webhook.IsValidSlackChannel(strings.TrimSpace(f.Channel)) { - errs = middleware.AddValidationError(errs, "Channel", ctx.Locale.TrString("repo.settings.add_webhook.invalid_channel_name")) + errs = validation.AddValidationError(errs, "Channel", ctx.Locale.TrString("repo.settings.add_webhook.invalid_channel_name")) } return errs } diff --git a/services/forms/user_form.go b/services/forms/user_form.go index bcf28db8173..3f111df9d06 100644 --- a/services/forms/user_form.go +++ b/services/forms/user_form.go @@ -276,7 +276,7 @@ func DetectInvalidOAuth2ApplicationRedirectURI(uris []string) (invalidURL string func (f *EditOAuth2ApplicationForm) Validate(ctx *middleware.ValidateContext, errs validation.BindingErrors) validation.BindingErrors { invalidURI := DetectInvalidOAuth2ApplicationRedirectURI(util.SplitTrimSpace(f.RedirectURIs, "\n")) if invalidURI != "" { - errs = middleware.AddValidationError(errs, "RedirectURIs", "RedirectURIs: "+ctx.Locale.TrString("form.url_error", `"`+invalidURI+`"`)) + errs = validation.AddValidationError(errs, "RedirectURIs", "RedirectURIs: "+ctx.Locale.TrString("form.url_error", `"`+invalidURI+`"`)) } return errs } diff --git a/services/forms/user_form_test.go b/services/forms/user_form_test.go index d62571c0d8d..214cbd05ed2 100644 --- a/services/forms/user_form_test.go +++ b/services/forms/user_form_test.go @@ -46,6 +46,7 @@ func TestRegisterForm_IsDomainAllowed_AllowedEmail(t *testing.T) { }{ {"security@gitea.io", true}, {"security@gITea.io", true}, + {"hack%evil.example@gitea.io", false}, {"invalid", false}, {"seee@example.com", false}, @@ -69,6 +70,7 @@ func TestRegisterForm_IsDomainAllowed_BlockedEmail(t *testing.T) { }{ {"security@gitea.io", false}, {"security@gitea.example", true}, + {"gitea.io!hack@gitea.example", false}, {"invalid", true}, {"user@my.block", false}, diff --git a/services/mailer/sender/sendmail.go b/services/mailer/sender/sendmail.go index ce438a78ea6..bb09845b172 100644 --- a/services/mailer/sender/sendmail.go +++ b/services/mailer/sender/sendmail.go @@ -30,15 +30,10 @@ func (s *SendmailSender) Send(from string, to []string, msg io.WriterTo) error { envelopeFrom = setting.MailService.EnvelopeFrom } - args := []string{"-f", envelopeFrom, "-i"} + // Use "-t" to extract recipients from message headers, don't add email addresses to the command line. + // Because email address can start with "-" which can lead to injected command line argument (RCE) + args := []string{"-f", envelopeFrom, "-i", "-t"} args = append(args, setting.MailService.SendmailArgs...) - for _, recipient := range to { - smtpTo, err := sanitizeEmailAddress(recipient) - if err != nil { - return fmt.Errorf("invalid recipient address %q: %w", recipient, err) - } - args = append(args, smtpTo) - } log.Trace("Sending with: %s %v", setting.MailService.SendmailPath, args) desc := fmt.Sprintf("SendMail: %s %v", setting.MailService.SendmailPath, args) diff --git a/templates/swagger/v1-openapi3.generated.json b/templates/swagger/v1-openapi3.generated.json index bea1be6261c..8da43734261 100644 --- a/templates/swagger/v1-openapi3.generated.json +++ b/templates/swagger/v1-openapi3.generated.json @@ -12058,6 +12058,9 @@ "403": { "$ref": "#/components/responses/forbidden" }, + "409": { + "$ref": "#/components/responses/error" + }, "422": { "$ref": "#/components/responses/validationError" } @@ -12142,6 +12145,9 @@ "403": { "$ref": "#/components/responses/forbidden" }, + "409": { + "$ref": "#/components/responses/error" + }, "422": { "$ref": "#/components/responses/validationError" } @@ -34999,6 +35005,12 @@ "201": { "$ref": "#/components/responses/EmailList" }, + "400": { + "$ref": "#/components/responses/error" + }, + "409": { + "$ref": "#/components/responses/error" + }, "422": { "$ref": "#/components/responses/validationError" } diff --git a/templates/swagger/v1-swagger.generated.json b/templates/swagger/v1-swagger.generated.json index 41559dab8b5..a4f9c7ade98 100644 --- a/templates/swagger/v1-swagger.generated.json +++ b/templates/swagger/v1-swagger.generated.json @@ -945,6 +945,9 @@ "403": { "$ref": "#/responses/forbidden" }, + "409": { + "$ref": "#/responses/error" + }, "422": { "$ref": "#/responses/validationError" } @@ -1029,6 +1032,9 @@ "403": { "$ref": "#/responses/forbidden" }, + "409": { + "$ref": "#/responses/error" + }, "422": { "$ref": "#/responses/validationError" } @@ -22273,6 +22279,12 @@ "201": { "$ref": "#/responses/EmailList" }, + "400": { + "$ref": "#/responses/error" + }, + "409": { + "$ref": "#/responses/error" + }, "422": { "$ref": "#/responses/validationError" } diff --git a/tests/integration/api_admin_test.go b/tests/integration/api_admin_test.go index 0d23ff248b4..dfcacdf0687 100644 --- a/tests/integration/api_admin_test.go +++ b/tests/integration/api_admin_test.go @@ -209,7 +209,7 @@ func TestAPIEditUser(t *testing.T) { errMap := make(map[string]any) json.Unmarshal(resp.Body.Bytes(), &errMap) - assert.Equal(t, "e-mail invalid [email: ]", errMap["message"]) + assert.Equal(t, "email address is invalid: ", errMap["message"]) user2 = unittest.AssertExistsAndLoadBean(t, &user_model.User{LoginName: "user2"}) assert.False(t, user2.IsRestricted) @@ -355,7 +355,7 @@ func TestAPIEditUser_NotAllowedEmailDomain(t *testing.T) { resp := MakeRequest(t, req, http.StatusBadRequest) errMap := make(map[string]string) assert.NoError(t, json.Unmarshal(resp.Body.Bytes(), &errMap)) - assert.Equal(t, "the domain of user email user2@example1.com conflicts with EMAIL_DOMAIN_ALLOWLIST or EMAIL_DOMAIN_BLOCKLIST", errMap["message"]) + assert.Equal(t, "email domain is not allowed: user2@example1.com", errMap["message"]) req = NewRequestWithJSON(t, "PATCH", urlStr, api.EditUserOption{Email: new("user2@example.org")}).AddTokenAuth(token) MakeRequest(t, req, http.StatusOK) diff --git a/tests/integration/api_user_email_test.go b/tests/integration/api_user_email_test.go index 4dca49d4779..bc61a1d3939 100644 --- a/tests/integration/api_user_email_test.go +++ b/tests/integration/api_user_email_test.go @@ -76,7 +76,7 @@ func TestAPIAddEmail(t *testing.T) { req := NewRequestWithJSON(t, "POST", "/api/v1/user/emails", &opts). AddTokenAuth(token) - MakeRequest(t, req, http.StatusUnprocessableEntity) + MakeRequest(t, req, http.StatusConflict) opts = api.CreateEmailOption{ Emails: []string{"user2-3@example.com"}, @@ -109,7 +109,7 @@ func TestAPIAddEmail(t *testing.T) { } req = NewRequestWithJSON(t, "POST", "/api/v1/user/emails", &opts). AddTokenAuth(token) - MakeRequest(t, req, http.StatusUnprocessableEntity) + MakeRequest(t, req, http.StatusBadRequest) } func TestAPIDeleteEmail(t *testing.T) {