fix: email comments are removed from email addresses (#9074)

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9074
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Reviewed-by: 0ko <0ko@noreply.codeberg.org>

modules/validation/email.go

@@ -72,16 +72,23 @@ func validateEmailBasic(email string) error {
 }
 
 func validateEmailDomain(email string) error {
-	if !IsEmailDomainAllowed(email) {
+	if _, ok := IsEmailDomainAllowed(email); !ok {
 		return ErrEmailInvalid{email}
 	}
 
 	return nil
 }
 
-func IsEmailDomainAllowed(email string) bool {
-	return isEmailDomainAllowedInternal(
-		email,
+func IsEmailDomainAllowed(email string) (validEmail, ok bool) {
+	// Normalized the address. This strips for example comments which could be
+	// used to smuggle a different domain
+	parsedAddress, err := mail.ParseAddress(email)
+	if err != nil {
+		return false, false
+	}
+
+	return true, isEmailDomainAllowedInternal(
+		parsedAddress.Address,
 		setting.Service.EmailDomainAllowList,
 		setting.Service.EmailDomainBlockList)
 }

modules/validation/email_test.go

@@ -67,8 +67,3 @@ func TestEmailAddressValidate(t *testing.T) {
 		})
 	}
 }
-
-func TestEmailDomainAllowList(t *testing.T) {
-	res := IsEmailDomainAllowed("someuser@localhost.localdomain")
-	assert.True(t, res)
-}

routers/api/v1/admin/user.go

@@ -149,7 +149,7 @@ func CreateUser(ctx *context.APIContext) {
 		return
 	}
 
-	if !validation.IsEmailDomainAllowed(u.Email) {
+	if _, ok := validation.IsEmailDomainAllowed(u.Email); !ok {
 		ctx.Resp.Header().Add("X-Gitea-Warning", fmt.Sprintf("the domain of user email %s conflicts with EMAIL_DOMAIN_ALLOWLIST or EMAIL_DOMAIN_BLOCKLIST", u.Email))
 	}
 
@@ -235,7 +235,7 @@ func EditUser(ctx *context.APIContext) {
 			return
 		}
 
-		if !validation.IsEmailDomainAllowed(*form.Email) {
+		if _, ok := validation.IsEmailDomainAllowed(*form.Email); !ok {
 			ctx.Resp.Header().Add("X-Gitea-Warning", fmt.Sprintf("the domain of user email %s conflicts with EMAIL_DOMAIN_ALLOWLIST or EMAIL_DOMAIN_BLOCKLIST", *form.Email))
 		}
 	}

routers/web/admin/users.go

@@ -201,7 +201,7 @@ func NewUserPost(ctx *context.Context) {
 		return
 	}
 
-	if !validation.IsEmailDomainAllowed(u.Email) {
+	if _, ok := validation.IsEmailDomainAllowed(u.Email); !ok {
 		ctx.Flash.Warning(ctx.Tr("form.email_domain_is_not_allowed", u.Email))
 	}
 
@@ -421,7 +421,7 @@ func EditUserPost(ctx *context.Context) {
 			}
 			return
 		}
-		if !validation.IsEmailDomainAllowed(form.Email) {
+		if _, ok := validation.IsEmailDomainAllowed(form.Email); !ok {
 			ctx.Flash.Warning(ctx.Tr("form.email_domain_is_not_allowed", form.Email))
 		}
 	}

routers/web/auth/auth.go

@@ -453,7 +453,10 @@ func SignUpPost(ctx *context.Context) {
 		return
 	}
 
-	if !form.IsEmailDomainAllowed() {
+	if emailValid, ok := form.IsEmailDomainAllowed(); !emailValid {
+		ctx.RenderWithErr(ctx.Tr("form.email_invalid"), tplSignUp, form)
+		return
+	} else if !ok {
 		ctx.RenderWithErr(ctx.Tr("auth.email_domain_blacklisted"), tplSignUp, &form)
 		return
 	}

routers/web/auth/linkaccount.go

@@ -226,7 +226,10 @@ func LinkAccountPostRegister(ctx *context.Context) {
 		}
 	}
 
-	if !form.IsEmailDomainAllowed() {
+	if emailValid, ok := form.IsEmailDomainAllowed(); !emailValid {
+		ctx.RenderWithErr(ctx.Tr("form.email_invalid"), tplSignUp, form)
+		return
+	} else if !ok {
 		ctx.RenderWithErr(ctx.Tr("auth.email_domain_blacklisted"), tplLinkAccount, &form)
 		return
 	}

services/forms/user_form.go

@@ -109,7 +109,7 @@ func (f *RegisterForm) Validate(req *http.Request, errs binding.Errors) binding.
 // The email is marked as allowed if it matches any of the
 // domains in the whitelist or if it doesn't match any of
 // domains in the blocklist, if any such list is not empty.
-func (f *RegisterForm) IsEmailDomainAllowed() bool {
+func (f *RegisterForm) IsEmailDomainAllowed() (validEmail, ok bool) {
 	return validation.IsEmailDomainAllowed(f.Email)
 }
 

services/forms/user_form_test.go

@@ -20,7 +20,9 @@ func TestRegisterForm_IsDomainAllowed_Empty(t *testing.T) {
 
 	form := RegisterForm{}
 
-	assert.True(t, form.IsEmailDomainAllowed())
+	emailValid, ok := form.IsEmailDomainAllowed()
+	assert.False(t, emailValid)
+	assert.False(t, ok)
 }
 
 func TestRegisterForm_IsDomainAllowed_InvalidEmail(t *testing.T) {
@@ -36,7 +38,8 @@ func TestRegisterForm_IsDomainAllowed_InvalidEmail(t *testing.T) {
 	for _, v := range tt {
 		form := RegisterForm{Email: v.email}
 
-		assert.False(t, form.IsEmailDomainAllowed())
+		_, ok := form.IsEmailDomainAllowed()
+		assert.False(t, ok)
 	}
 }
 
@@ -59,7 +62,8 @@ func TestRegisterForm_IsDomainAllowed_AllowedEmail(t *testing.T) {
 	for _, v := range tt {
 		form := RegisterForm{Email: v.email}
 
-		assert.Equal(t, v.valid, form.IsEmailDomainAllowed())
+		_, ok := form.IsEmailDomainAllowed()
+		assert.Equal(t, v.valid, ok)
 	}
 }
 
@@ -72,7 +76,6 @@ func TestRegisterForm_IsDomainAllowed_BlockedEmail(t *testing.T) {
 	}{
 		{"security@gitea.io", false},
 		{"security@gitea.example", true},
-		{"invalid", true},
 
 		{"user@my.block", false},
 		{"user@my.block1", true},
@@ -81,7 +84,8 @@ func TestRegisterForm_IsDomainAllowed_BlockedEmail(t *testing.T) {
 	for _, v := range tt {
 		form := RegisterForm{Email: v.email}
 
-		assert.Equal(t, v.valid, form.IsEmailDomainAllowed())
+		_, ok := form.IsEmailDomainAllowed()
+		assert.Equal(t, v.valid, ok)
 	}
 }
 

tests/integration/email_block_allowlist_test.go

@@ -0,0 +1,66 @@
+// Copyright 2025 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package integration
+
+import (
+	"testing"
+
+	"forgejo.org/modules/setting"
+	"forgejo.org/modules/test"
+	"forgejo.org/modules/validation"
+	"forgejo.org/tests"
+
+	"github.com/gobwas/glob"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestEmailBlocklist(t *testing.T) {
+	defer test.MockVariableValue(
+		&setting.Service.EmailDomainBlockList,
+		[]glob.Glob{glob.MustCompile("evil")},
+	)()
+
+	defer tests.PrepareTestEnv(t)()
+
+	emailValid, ok := validation.IsEmailDomainAllowed("🐸@pond")
+	assert.True(t, emailValid)
+	assert.True(t, ok)
+
+	emailValid, ok = validation.IsEmailDomainAllowed("🐸@pond (what-is-this@evil)")
+	assert.True(t, emailValid)
+	assert.True(t, ok)
+
+	emailValid, ok = validation.IsEmailDomainAllowed("jomo@evil")
+	assert.True(t, emailValid)
+	assert.False(t, ok)
+
+	emailValid, ok = validation.IsEmailDomainAllowed("jomo@evil (but-does-it@break)")
+	assert.True(t, emailValid)
+	assert.False(t, ok)
+}
+
+func TestEmailAllowlist(t *testing.T) {
+	defer test.MockVariableValue(
+		&setting.Service.EmailDomainAllowList,
+		[]glob.Glob{glob.MustCompile("pond")},
+	)()
+
+	defer tests.PrepareTestEnv(t)()
+
+	emailValid, ok := validation.IsEmailDomainAllowed("🐸@pond")
+	assert.True(t, emailValid)
+	assert.True(t, ok)
+
+	emailValid, ok = validation.IsEmailDomainAllowed("🐸@pond (what-is-this@evil)")
+	assert.True(t, emailValid)
+	assert.True(t, ok)
+
+	emailValid, ok = validation.IsEmailDomainAllowed("jomo@evil")
+	assert.True(t, emailValid)
+	assert.False(t, ok)
+
+	emailValid, ok = validation.IsEmailDomainAllowed("jomo@evil (but-does-it@break)")
+	assert.True(t, emailValid)
+	assert.False(t, ok)
+}