From 18033f49ba8f00695dd9f885360664a383610df1 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 15 Mar 2022 01:39:54 +0800 Subject: [PATCH] Restrict email address validation (#17688) This didn't follow the RFC but it's a subset of that. I think we should narrow the allowed chars at first and discuss more possibility in future PRs. --- models/user/email_address.go | 30 +++++++++++++++- models/user/email_address_test.go | 55 +++++++++++++++++++++++++++++ models/user/user.go | 14 +++++--- models/user/user_test.go | 2 +- routers/api/v1/admin/user.go | 5 ++- routers/api/v1/user/email.go | 3 +- routers/web/admin/users.go | 6 +++- routers/web/auth/auth.go | 3 ++ routers/web/user/setting/account.go | 3 +- 9 files changed, 110 insertions(+), 11 deletions(-) diff --git a/models/user/email_address.go b/models/user/email_address.go index 726af7b3b4..564d018dac 100644 --- a/models/user/email_address.go +++ b/models/user/email_address.go @@ -10,6 +10,7 @@ import ( "errors" "fmt" "net/mail" + "regexp" "strings" "code.gitea.io/gitea/models/db" @@ -22,7 +23,22 @@ import ( ) // ErrEmailNotActivated e-mail address has not been activated error -var ErrEmailNotActivated = errors.New("E-mail address has not been activated") +var ErrEmailNotActivated = errors.New("e-mail address has not been activated") + +// 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) +} // ErrEmailInvalid represents an error where the email address does not comply with RFC 5322 type ErrEmailInvalid struct { @@ -106,12 +122,24 @@ func (email *EmailAddress) BeforeInsert() { } } +var 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])?)*$") + // ValidateEmail check if email is a allowed address func ValidateEmail(email string) error { if len(email) == 0 { return nil } + if !emailRegexp.MatchString(email) { + return ErrEmailCharIsNotSupported{email} + } + + if !(email[0] >= 'a' && email[0] <= 'z') && + !(email[0] >= 'A' && email[0] <= 'Z') && + !(email[0] >= '0' && email[0] <= '9') { + return ErrEmailInvalid{email} + } + if _, err := mail.ParseAddress(email); err != nil { return ErrEmailInvalid{email} } diff --git a/models/user/email_address_test.go b/models/user/email_address_test.go index 4a539e150a..7eeb469b26 100644 --- a/models/user/email_address_test.go +++ b/models/user/email_address_test.go @@ -252,3 +252,58 @@ func TestListEmails(t *testing.T) { assert.Len(t, emails, 5) assert.Greater(t, count, int64(len(emails))) } + +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`: ErrEmailCharIsNotSupported{`first;last@iana.org`}, + ".233@qq.com": ErrEmailInvalid{".233@qq.com"}, + "!233@qq.com": ErrEmailInvalid{"!233@qq.com"}, + "#233@qq.com": ErrEmailInvalid{"#233@qq.com"}, + "$233@qq.com": ErrEmailInvalid{"$233@qq.com"}, + "%233@qq.com": ErrEmailInvalid{"%233@qq.com"}, + "&233@qq.com": ErrEmailInvalid{"&233@qq.com"}, + "'233@qq.com": ErrEmailInvalid{"'233@qq.com"}, + "*233@qq.com": ErrEmailInvalid{"*233@qq.com"}, + "+233@qq.com": ErrEmailInvalid{"+233@qq.com"}, + "/233@qq.com": ErrEmailInvalid{"/233@qq.com"}, + "=233@qq.com": ErrEmailInvalid{"=233@qq.com"}, + "?233@qq.com": ErrEmailInvalid{"?233@qq.com"}, + "^233@qq.com": ErrEmailInvalid{"^233@qq.com"}, + "`233@qq.com": ErrEmailInvalid{"`233@qq.com"}, + "{233@qq.com": ErrEmailInvalid{"{233@qq.com"}, + "|233@qq.com": ErrEmailInvalid{"|233@qq.com"}, + "}233@qq.com": ErrEmailInvalid{"}233@qq.com"}, + "~233@qq.com": ErrEmailInvalid{"~233@qq.com"}, + ";233@qq.com": ErrEmailCharIsNotSupported{";233@qq.com"}, + "Foo ": ErrEmailCharIsNotSupported{"Foo "}, + string([]byte{0xE2, 0x84, 0xAA}): ErrEmailCharIsNotSupported{string([]byte{0xE2, 0x84, 0xAA})}, + } + for kase, err := range kases { + t.Run(kase, func(t *testing.T) { + assert.EqualValues(t, err, ValidateEmail(kase)) + }) + } +} diff --git a/models/user/user.go b/models/user/user.go index 3eabf4808c..a3094a13ce 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -644,6 +644,15 @@ func CreateUser(u *User, overwriteDefault ...*CreateUserOverwriteOptions) (err e u.Visibility = overwriteDefault[0].Visibility } + // validate data + if err := validateUser(u); err != nil { + return err + } + + if err := ValidateEmail(u.Email); err != nil { + return err + } + ctx, committer, err := db.TxContext() if err != nil { return err @@ -652,11 +661,6 @@ func CreateUser(u *User, overwriteDefault ...*CreateUserOverwriteOptions) (err e sess := db.GetEngine(ctx) - // validate data - if err := validateUser(u); err != nil { - return err - } - isExist, err := isUserExist(sess, 0, u.Name) if err != nil { return err diff --git a/models/user/user_test.go b/models/user/user_test.go index a5f47172ee..335537aa13 100644 --- a/models/user/user_test.go +++ b/models/user/user_test.go @@ -232,7 +232,7 @@ func TestCreateUserInvalidEmail(t *testing.T) { err := CreateUser(user) assert.Error(t, err) - assert.True(t, IsErrEmailInvalid(err)) + assert.True(t, IsErrEmailCharIsNotSupported(err)) } func TestCreateUserEmailAlreadyUsed(t *testing.T) { diff --git a/routers/api/v1/admin/user.go b/routers/api/v1/admin/user.go index 0ecebad5d7..1d3854df9b 100644 --- a/routers/api/v1/admin/user.go +++ b/routers/api/v1/admin/user.go @@ -119,6 +119,7 @@ func CreateUser(ctx *context.APIContext) { user_model.IsErrEmailAlreadyUsed(err) || db.IsErrNameReserved(err) || db.IsErrNameCharsNotAllowed(err) || + user_model.IsErrEmailCharIsNotSupported(err) || user_model.IsErrEmailInvalid(err) || db.IsErrNamePatternNotAllowed(err) { ctx.Error(http.StatusUnprocessableEntity, "", err) @@ -265,7 +266,9 @@ func EditUser(ctx *context.APIContext) { } if err := user_model.UpdateUser(u, emailChanged); err != nil { - if user_model.IsErrEmailAlreadyUsed(err) || user_model.IsErrEmailInvalid(err) { + if user_model.IsErrEmailAlreadyUsed(err) || + user_model.IsErrEmailCharIsNotSupported(err) || + user_model.IsErrEmailInvalid(err) { ctx.Error(http.StatusUnprocessableEntity, "", err) } else { ctx.Error(http.StatusInternalServerError, "UpdateUser", err) diff --git a/routers/api/v1/user/email.go b/routers/api/v1/user/email.go index 6887c306cc..ed79723c60 100644 --- a/routers/api/v1/user/email.go +++ b/routers/api/v1/user/email.go @@ -80,7 +80,8 @@ func AddEmail(ctx *context.APIContext) { if err := user_model.AddEmailAddresses(emails); err != nil { if user_model.IsErrEmailAlreadyUsed(err) { ctx.Error(http.StatusUnprocessableEntity, "", "Email address has been used: "+err.(user_model.ErrEmailAlreadyUsed).Email) - } else if user_model.IsErrEmailInvalid(err) { + } else if user_model.IsErrEmailCharIsNotSupported(err) || + user_model.IsErrEmailInvalid(err) { errMsg := fmt.Sprintf("Email address %s invalid", err.(user_model.ErrEmailInvalid).Email) ctx.Error(http.StatusUnprocessableEntity, "", errMsg) } else { diff --git a/routers/web/admin/users.go b/routers/web/admin/users.go index 1f304297c0..a7d7d62d9a 100644 --- a/routers/web/admin/users.go +++ b/routers/web/admin/users.go @@ -171,6 +171,9 @@ func NewUserPost(ctx *context.Context) { case user_model.IsErrEmailAlreadyUsed(err): ctx.Data["Err_Email"] = true ctx.RenderWithErr(ctx.Tr("form.email_been_used"), tplUserNew, &form) + case user_model.IsErrEmailCharIsNotSupported(err): + ctx.Data["Err_Email"] = true + ctx.RenderWithErr(ctx.Tr("form.email_invalid"), tplUserNew, &form) case user_model.IsErrEmailInvalid(err): ctx.Data["Err_Email"] = true ctx.RenderWithErr(ctx.Tr("form.email_invalid"), tplUserNew, &form) @@ -386,7 +389,8 @@ func EditUserPost(ctx *context.Context) { if user_model.IsErrEmailAlreadyUsed(err) { ctx.Data["Err_Email"] = true ctx.RenderWithErr(ctx.Tr("form.email_been_used"), tplUserEdit, &form) - } else if user_model.IsErrEmailInvalid(err) { + } else if user_model.IsErrEmailCharIsNotSupported(err) || + user_model.IsErrEmailInvalid(err) { ctx.Data["Err_Email"] = true ctx.RenderWithErr(ctx.Tr("form.email_invalid"), tplUserEdit, &form) } else { diff --git a/routers/web/auth/auth.go b/routers/web/auth/auth.go index ce8ec8a1e3..9209b7335e 100644 --- a/routers/web/auth/auth.go +++ b/routers/web/auth/auth.go @@ -573,6 +573,9 @@ func createUserInContext(ctx *context.Context, tpl base.TplName, form interface{ case user_model.IsErrEmailAlreadyUsed(err): ctx.Data["Err_Email"] = true ctx.RenderWithErr(ctx.Tr("form.email_been_used"), tpl, form) + case user_model.IsErrEmailCharIsNotSupported(err): + ctx.Data["Err_Email"] = true + ctx.RenderWithErr(ctx.Tr("form.email_invalid"), tpl, form) case user_model.IsErrEmailInvalid(err): ctx.Data["Err_Email"] = true ctx.RenderWithErr(ctx.Tr("form.email_invalid"), tpl, form) diff --git a/routers/web/user/setting/account.go b/routers/web/user/setting/account.go index b73122fa12..492b4f82c8 100644 --- a/routers/web/user/setting/account.go +++ b/routers/web/user/setting/account.go @@ -188,7 +188,8 @@ func EmailPost(ctx *context.Context) { ctx.RenderWithErr(ctx.Tr("form.email_been_used"), tplSettingsAccount, &form) return - } else if user_model.IsErrEmailInvalid(err) { + } else if user_model.IsErrEmailCharIsNotSupported(err) || + user_model.IsErrEmailInvalid(err) { loadAccountData(ctx) ctx.RenderWithErr(ctx.Tr("form.email_invalid"), tplSettingsAccount, &form)