From 33431fcbd3e5c4a6c6bf18db3339ae331efdf3f4 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sun, 22 Nov 2020 18:31:35 +0100 Subject: [PATCH] Validate email before inserting/updating (#13475) (#13666) * Add email validity check (#13475) * Improve error feedback for duplicate deploy keys Instead of a generic HTTP 500 error page, a flash message is rendered with the deploy key page template so inform the user that a key with the intended title already exists. * API returns 422 error when key with name exists * Add email validity checking Add email validity checking for the following routes: [Web interface] 1. User registration 2. User creation by admin 3. Adding an email through user settings [API] 1. POST /admin/users 2. PATCH /admin/users/:username 3. POST /user/emails * Add further tests * Add signup email tests * Add email validity check for linking existing account * Address PR comments * Remove unneeded DB session * Move email check to updateUser Co-authored-by: zeripath Co-authored-by: Lunny Xiao Co-authored-by: techknowlogick * skip email validation on empty string (#13627) - move validation into its own function - use a session for UpdateUserSetting * rm TODO for backport Co-authored-by: Chris Shyi Co-authored-by: zeripath Co-authored-by: Lunny Xiao Co-authored-by: techknowlogick --- integrations/api_admin_test.go | 19 +++++++++++++++++ integrations/signup_test.go | 38 +++++++++++++++++++++++++++++++++ models/error.go | 15 +++++++++++++ models/user.go | 26 +++++++++++++++++----- models/user_mail.go | 21 ++++++++++++++++++ models/user_test.go | 15 +++++++++++++ options/locale/locale_en-US.ini | 1 + routers/admin/users.go | 6 ++++++ routers/admin/users_test.go | 30 ++++++++++++++++++++++++++ routers/api/v1/admin/user.go | 3 ++- routers/api/v1/user/email.go | 4 ++++ routers/user/auth.go | 6 ++++++ routers/user/setting/account.go | 5 +++++ 13 files changed, 183 insertions(+), 6 deletions(-) diff --git a/integrations/api_admin_test.go b/integrations/api_admin_test.go index 9ff9d71493f..80d6b522895 100644 --- a/integrations/api_admin_test.go +++ b/integrations/api_admin_test.go @@ -144,3 +144,22 @@ func TestAPIListUsersNonAdmin(t *testing.T) { req := NewRequestf(t, "GET", "/api/v1/admin/users?token=%s", token) session.MakeRequest(t, req, http.StatusForbidden) } + +func TestAPICreateUserInvalidEmail(t *testing.T) { + defer prepareTestEnv(t)() + adminUsername := "user1" + session := loginUser(t, adminUsername) + token := getTokenForLoggedInUser(t, session) + urlStr := fmt.Sprintf("/api/v1/admin/users?token=%s", token) + req := NewRequestWithValues(t, "POST", urlStr, map[string]string{ + "email": "invalid_email@domain.com\r\n", + "full_name": "invalid user", + "login_name": "invalidUser", + "must_change_password": "true", + "password": "password", + "send_notify": "true", + "source_id": "0", + "username": "invalidUser", + }) + session.MakeRequest(t, req, http.StatusUnprocessableEntity) +} diff --git a/integrations/signup_test.go b/integrations/signup_test.go index 02262ec8537..5208a42ce59 100644 --- a/integrations/signup_test.go +++ b/integrations/signup_test.go @@ -5,10 +5,14 @@ package integrations import ( + "fmt" "net/http" + "strings" "testing" "code.gitea.io/gitea/modules/setting" + "github.com/stretchr/testify/assert" + "github.com/unknwon/i18n" ) func TestSignup(t *testing.T) { @@ -28,3 +32,37 @@ func TestSignup(t *testing.T) { req = NewRequest(t, "GET", "/exampleUser") MakeRequest(t, req, http.StatusOK) } + +func TestSignupEmail(t *testing.T) { + defer prepareTestEnv(t)() + + setting.Service.EnableCaptcha = false + + tests := []struct { + email string + wantStatus int + wantMsg string + }{ + {"exampleUser@example.com\r\n", http.StatusOK, i18n.Tr("en", "form.email_invalid", nil)}, + {"exampleUser@example.com\r", http.StatusOK, i18n.Tr("en", "form.email_invalid", nil)}, + {"exampleUser@example.com\n", http.StatusOK, i18n.Tr("en", "form.email_invalid", nil)}, + {"exampleUser@example.com", http.StatusFound, ""}, + } + + for i, test := range tests { + req := NewRequestWithValues(t, "POST", "/user/sign_up", map[string]string{ + "user_name": fmt.Sprintf("exampleUser%d", i), + "email": test.email, + "password": "examplePassword!1", + "retype": "examplePassword!1", + }) + resp := MakeRequest(t, req, test.wantStatus) + if test.wantMsg != "" { + htmlDoc := NewHTMLParser(t, resp.Body) + assert.Equal(t, + test.wantMsg, + strings.TrimSpace(htmlDoc.doc.Find(".ui.message").Text()), + ) + } + } +} diff --git a/models/error.go b/models/error.go index b2273f74c91..83354ff173d 100644 --- a/models/error.go +++ b/models/error.go @@ -193,6 +193,21 @@ func (err ErrEmailAlreadyUsed) Error() string { return fmt.Sprintf("e-mail already in use [email: %s]", err.Email) } +// ErrEmailInvalid represents an error where the email address does not comply with RFC 5322 +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) +} + // ErrOpenIDAlreadyUsed represents a "OpenIDAlreadyUsed" kind of error. type ErrOpenIDAlreadyUsed struct { OpenID string diff --git a/models/user.go b/models/user.go index 1ab417115f9..b2abdca964f 100644 --- a/models/user.go +++ b/models/user.go @@ -821,6 +821,10 @@ func CreateUser(u *User) (err error) { return ErrEmailAlreadyUsed{u.Email} } + if err = ValidateEmail(u.Email); err != nil { + return err + } + isExist, err = isEmailUsed(sess, u.Email) if err != nil { return err @@ -963,8 +967,12 @@ func checkDupEmail(e Engine, u *User) error { return nil } -func updateUser(e Engine, u *User) error { - _, err := e.ID(u.ID).AllCols().Update(u) +func updateUser(e Engine, u *User) (err error) { + u.Email = strings.ToLower(u.Email) + if err = ValidateEmail(u.Email); err != nil { + return err + } + _, err = e.ID(u.ID).AllCols().Update(u) return err } @@ -984,13 +992,21 @@ func updateUserCols(e Engine, u *User, cols ...string) error { } // UpdateUserSetting updates user's settings. -func UpdateUserSetting(u *User) error { +func UpdateUserSetting(u *User) (err error) { + sess := x.NewSession() + defer sess.Close() + if err = sess.Begin(); err != nil { + return err + } if !u.IsOrganization() { - if err := checkDupEmail(x, u); err != nil { + if err = checkDupEmail(sess, u); err != nil { return err } } - return updateUser(x, u) + if err = updateUser(sess, u); err != nil { + return err + } + return sess.Commit() } // deleteBeans deletes all given beans, beans should contain delete conditions. diff --git a/models/user_mail.go b/models/user_mail.go index 60354e23ffb..29a73f13f21 100644 --- a/models/user_mail.go +++ b/models/user_mail.go @@ -8,6 +8,7 @@ package models import ( "errors" "fmt" + "net/mail" "strings" "code.gitea.io/gitea/modules/log" @@ -32,6 +33,19 @@ type EmailAddress struct { IsPrimary bool `xorm:"-"` } +// ValidateEmail check if email is a allowed address +func ValidateEmail(email string) error { + if len(email) == 0 { + return nil + } + + if _, err := mail.ParseAddress(email); err != nil { + return ErrEmailInvalid{email} + } + + return nil +} + // GetEmailAddresses returns all email addresses belongs to given user. func GetEmailAddresses(uid int64) ([]*EmailAddress, error) { emails := make([]*EmailAddress, 0, 5) @@ -143,6 +157,10 @@ func addEmailAddress(e Engine, email *EmailAddress) error { return ErrEmailAlreadyUsed{email.Email} } + if err = ValidateEmail(email.Email); err != nil { + return err + } + _, err = e.Insert(email) return err } @@ -167,6 +185,9 @@ func AddEmailAddresses(emails []*EmailAddress) error { } else if used { return ErrEmailAlreadyUsed{emails[i].Email} } + if err = ValidateEmail(emails[i].Email); err != nil { + return err + } } if _, err := x.Insert(emails); err != nil { diff --git a/models/user_test.go b/models/user_test.go index d03ef4fad4d..b9fe99278c0 100644 --- a/models/user_test.go +++ b/models/user_test.go @@ -346,6 +346,21 @@ func TestCreateUser(t *testing.T) { assert.NoError(t, DeleteUser(user)) } +func TestCreateUserInvalidEmail(t *testing.T) { + user := &User{ + Name: "GiteaBot", + Email: "GiteaBot@gitea.io\r\n", + Passwd: ";p['////..-++']", + IsAdmin: false, + Theme: setting.UI.DefaultTheme, + MustChangePassword: false, + } + + err := CreateUser(user) + assert.Error(t, err) + assert.True(t, IsErrEmailInvalid(err)) +} + func TestCreateUser_Issue5882(t *testing.T) { // Init settings diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 18c621db67d..1f5e2a35d25 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -366,6 +366,7 @@ org_name_been_taken = The organization name is already taken. team_name_been_taken = The team name is already taken. team_no_units_error = Allow access to at least one repository section. email_been_used = The email address is already used. +email_invalid = The email address is invalid. openid_been_used = The OpenID address '%s' is already used. username_password_incorrect = Username or password is incorrect. password_complexity = Password does not pass complexity requirements: diff --git a/routers/admin/users.go b/routers/admin/users.go index 9fb758621b0..4382ee3877f 100644 --- a/routers/admin/users.go +++ b/routers/admin/users.go @@ -129,6 +129,9 @@ func NewUserPost(ctx *context.Context, form auth.AdminCreateUserForm) { case models.IsErrEmailAlreadyUsed(err): ctx.Data["Err_Email"] = true ctx.RenderWithErr(ctx.Tr("form.email_been_used"), tplUserNew, &form) + case models.IsErrEmailInvalid(err): + ctx.Data["Err_Email"] = true + ctx.RenderWithErr(ctx.Tr("form.email_invalid"), tplUserNew, &form) case models.IsErrNameReserved(err): ctx.Data["Err_UserName"] = true ctx.RenderWithErr(ctx.Tr("user.form.name_reserved", err.(models.ErrNameReserved).Name), tplUserNew, &form) @@ -277,6 +280,9 @@ func EditUserPost(ctx *context.Context, form auth.AdminEditUserForm) { if models.IsErrEmailAlreadyUsed(err) { ctx.Data["Err_Email"] = true ctx.RenderWithErr(ctx.Tr("form.email_been_used"), tplUserEdit, &form) + } else if models.IsErrEmailInvalid(err) { + ctx.Data["Err_Email"] = true + ctx.RenderWithErr(ctx.Tr("form.email_invalid"), tplUserEdit, &form) } else { ctx.ServerError("UpdateUser", err) } diff --git a/routers/admin/users_test.go b/routers/admin/users_test.go index 2b36b45d49c..a282507f56b 100644 --- a/routers/admin/users_test.go +++ b/routers/admin/users_test.go @@ -87,3 +87,33 @@ func TestNewUserPost_MustChangePasswordFalse(t *testing.T) { assert.Equal(t, email, u.Email) assert.False(t, u.MustChangePassword) } + +func TestNewUserPost_InvalidEmail(t *testing.T) { + + models.PrepareTestEnv(t) + ctx := test.MockContext(t, "admin/users/new") + + u := models.AssertExistsAndLoadBean(t, &models.User{ + IsAdmin: true, + ID: 2, + }).(*models.User) + + ctx.User = u + + username := "gitea" + email := "gitea@gitea.io\r\n" + + form := auth.AdminCreateUserForm{ + LoginType: "local", + LoginName: "local", + UserName: username, + Email: email, + Password: "abc123ABC!=$", + SendNotify: false, + MustChangePassword: false, + } + + NewUserPost(ctx, form) + + assert.NotEmpty(t, ctx.Flash.ErrorMsg) +} diff --git a/routers/api/v1/admin/user.go b/routers/api/v1/admin/user.go index dc095f3a135..c4b52e4bd63 100644 --- a/routers/api/v1/admin/user.go +++ b/routers/api/v1/admin/user.go @@ -101,6 +101,7 @@ func CreateUser(ctx *context.APIContext, form api.CreateUserOption) { models.IsErrEmailAlreadyUsed(err) || models.IsErrNameReserved(err) || models.IsErrNameCharsNotAllowed(err) || + models.IsErrEmailInvalid(err) || models.IsErrNamePatternNotAllowed(err) { ctx.Error(http.StatusUnprocessableEntity, "", err) } else { @@ -208,7 +209,7 @@ func EditUser(ctx *context.APIContext, form api.EditUserOption) { } if err := models.UpdateUser(u); err != nil { - if models.IsErrEmailAlreadyUsed(err) { + if models.IsErrEmailAlreadyUsed(err) || models.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 07fcde625e7..d848f5e58d8 100644 --- a/routers/api/v1/user/email.go +++ b/routers/api/v1/user/email.go @@ -5,6 +5,7 @@ package user import ( + "fmt" "net/http" "code.gitea.io/gitea/models" @@ -78,6 +79,9 @@ func AddEmail(ctx *context.APIContext, form api.CreateEmailOption) { if err := models.AddEmailAddresses(emails); err != nil { if models.IsErrEmailAlreadyUsed(err) { ctx.Error(http.StatusUnprocessableEntity, "", "Email address has been used: "+err.(models.ErrEmailAlreadyUsed).Email) + } else if models.IsErrEmailInvalid(err) { + errMsg := fmt.Sprintf("Email address %s invalid", err.(models.ErrEmailInvalid).Email) + ctx.Error(http.StatusUnprocessableEntity, "", errMsg) } else { ctx.Error(http.StatusInternalServerError, "AddEmailAddresses", err) } diff --git a/routers/user/auth.go b/routers/user/auth.go index 32b031fc741..ba6420967f6 100644 --- a/routers/user/auth.go +++ b/routers/user/auth.go @@ -964,6 +964,9 @@ func LinkAccountPostRegister(ctx *context.Context, cpt *captcha.Captcha, form au case models.IsErrEmailAlreadyUsed(err): ctx.Data["Err_Email"] = true ctx.RenderWithErr(ctx.Tr("form.email_been_used"), tplLinkAccount, &form) + case models.IsErrEmailInvalid(err): + ctx.Data["Err_Email"] = true + ctx.RenderWithErr(ctx.Tr("form.email_invalid"), tplSignUp, &form) case models.IsErrNameReserved(err): ctx.Data["Err_UserName"] = true ctx.RenderWithErr(ctx.Tr("user.form.name_reserved", err.(models.ErrNameReserved).Name), tplLinkAccount, &form) @@ -1151,6 +1154,9 @@ func SignUpPost(ctx *context.Context, cpt *captcha.Captcha, form auth.RegisterFo case models.IsErrEmailAlreadyUsed(err): ctx.Data["Err_Email"] = true ctx.RenderWithErr(ctx.Tr("form.email_been_used"), tplSignUp, &form) + case models.IsErrEmailInvalid(err): + ctx.Data["Err_Email"] = true + ctx.RenderWithErr(ctx.Tr("form.email_invalid"), tplSignUp, &form) case models.IsErrNameReserved(err): ctx.Data["Err_UserName"] = true ctx.RenderWithErr(ctx.Tr("user.form.name_reserved", err.(models.ErrNameReserved).Name), tplSignUp, &form) diff --git a/routers/user/setting/account.go b/routers/user/setting/account.go index 99e20177bc9..9b72e2a31a2 100644 --- a/routers/user/setting/account.go +++ b/routers/user/setting/account.go @@ -179,6 +179,11 @@ func EmailPost(ctx *context.Context, form auth.AddEmailForm) { ctx.RenderWithErr(ctx.Tr("form.email_been_used"), tplSettingsAccount, &form) return + } else if models.IsErrEmailInvalid(err) { + loadAccountData(ctx) + + ctx.RenderWithErr(ctx.Tr("form.email_invalid"), tplSettingsAccount, &form) + return } ctx.ServerError("AddEmailAddress", err) return