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 <[email protected]>
Co-authored-by: Lunny Xiao <[email protected]>
Co-authored-by: techknowlogick <[email protected]>

* 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 <[email protected]>
Co-authored-by: zeripath <[email protected]>
Co-authored-by: Lunny Xiao <[email protected]>
Co-authored-by: techknowlogick <[email protected]>

Author: 5 people

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":                "[email protected]\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)
+}

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
+	}{
+		{"[email protected]\r\n", http.StatusOK, i18n.Tr("en", "form.email_invalid", nil)},
+		{"[email protected]\r", http.StatusOK, i18n.Tr("en", "form.email_invalid", nil)},
+		{"[email protected]\n", http.StatusOK, i18n.Tr("en", "form.email_invalid", nil)},
+		{"[email protected]", 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()),
+			)
+		}
+	}
+}

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

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.

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 {

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:              "[email protected]\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

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:

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)
 		}

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 := "[email protected]\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)
+}

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)