fix: enforce authorized address checks on send email only (#1806)

Moves the authorized address only check in the flow that sends the
email, instead of in the email address validation flow.

internal/api/api.go

@@ -134,7 +134,6 @@ func NewAPIWithVersion(globalConfig *conf.GlobalConfiguration, db *storage.Conne
 
 	r.Route("/", func(r *router) {
 		r.Use(api.isValidExternalHost)
-		r.Use(api.isValidAuthorizedEmail)
 
 		r.Get("/settings", api.Settings)
 

internal/api/mail.go

@@ -2,6 +2,7 @@ package api
 
 import (
 	"net/http"
+	"regexp"
 	"strings"
 	"time"
 
@@ -23,7 +24,6 @@ import (
 
 var (
 	EmailRateLimitExceeded error = errors.New("email rate limit exceeded")
-	AddressNotAuthorized   error = errors.New("Destination email address not authorized")
 )
 
 type GenerateLinkParams struct {
@@ -320,6 +320,8 @@ func (a *API) sendConfirmation(r *http.Request, tx *storage.Connection, u *model
 		u.ConfirmationToken = oldToken
 		if errors.Is(err, EmailRateLimitExceeded) {
 			return tooManyRequestsError(ErrorCodeOverEmailSendRateLimit, EmailRateLimitExceeded.Error())
+		} else if herr, ok := err.(*HTTPError); ok {
+			return herr
 		}
 		return internalServerError("Error sending confirmation email").WithInternalError(err)
 	}
@@ -351,6 +353,8 @@ func (a *API) sendInvite(r *http.Request, tx *storage.Connection, u *models.User
 		u.ConfirmationToken = oldToken
 		if errors.Is(err, EmailRateLimitExceeded) {
 			return tooManyRequestsError(ErrorCodeOverEmailSendRateLimit, EmailRateLimitExceeded.Error())
+		} else if herr, ok := err.(*HTTPError); ok {
+			return herr
 		}
 		return internalServerError("Error sending invite email").WithInternalError(err)
 	}
@@ -390,6 +394,8 @@ func (a *API) sendPasswordRecovery(r *http.Request, tx *storage.Connection, u *m
 		u.RecoveryToken = oldToken
 		if errors.Is(err, EmailRateLimitExceeded) {
 			return tooManyRequestsError(ErrorCodeOverEmailSendRateLimit, EmailRateLimitExceeded.Error())
+		} else if herr, ok := err.(*HTTPError); ok {
+			return herr
 		}
 		return internalServerError("Error sending recovery email").WithInternalError(err)
 	}
@@ -428,6 +434,8 @@ func (a *API) sendReauthenticationOtp(r *http.Request, tx *storage.Connection, u
 		u.ReauthenticationToken = oldToken
 		if errors.Is(err, EmailRateLimitExceeded) {
 			return tooManyRequestsError(ErrorCodeOverEmailSendRateLimit, EmailRateLimitExceeded.Error())
+		} else if herr, ok := err.(*HTTPError); ok {
+			return herr
 		}
 		return internalServerError("Error sending reauthentication email").WithInternalError(err)
 	}
@@ -467,6 +475,8 @@ func (a *API) sendMagicLink(r *http.Request, tx *storage.Connection, u *models.U
 		u.RecoveryToken = oldToken
 		if errors.Is(err, EmailRateLimitExceeded) {
 			return tooManyRequestsError(ErrorCodeOverEmailSendRateLimit, EmailRateLimitExceeded.Error())
+		} else if herr, ok := err.(*HTTPError); ok {
+			return herr
 		}
 		return internalServerError("Error sending magic link email").WithInternalError(err)
 	}
@@ -517,6 +527,8 @@ func (a *API) sendEmailChange(r *http.Request, tx *storage.Connection, u *models
 	if err := a.sendEmail(r, tx, u, mail.EmailChangeVerification, otpCurrent, otpNew, u.EmailChangeTokenNew); err != nil {
 		if errors.Is(err, EmailRateLimitExceeded) {
 			return tooManyRequestsError(ErrorCodeOverEmailSendRateLimit, EmailRateLimitExceeded.Error())
+		} else if herr, ok := err.(*HTTPError); ok {
+			return herr
 		}
 		return internalServerError("Error sending email change email").WithInternalError(err)
 	}
@@ -569,13 +581,52 @@ func validateSentWithinFrequencyLimit(sentAt *time.Time, frequency time.Duration
 	return nil
 }
 
+var emailLabelPattern = regexp.MustCompile("[+][^@]+@")
+
+func (a *API) checkEmailAddressAuthorization(email string) bool {
+	if len(a.config.External.Email.AuthorizedAddresses) > 0 {
+		// allow labelled emails when authorization rules are in place
+		normalized := emailLabelPattern.ReplaceAllString(email, "@")
+
+		for _, authorizedAddress := range a.config.External.Email.AuthorizedAddresses {
+			if strings.EqualFold(normalized, authorizedAddress) {
+				return true
+			}
+		}
+
+		return false
+	}
+
+	return true
+}
+
 func (a *API) sendEmail(r *http.Request, tx *storage.Connection, u *models.User, emailActionType, otp, otpNew, tokenHashWithPrefix string) error {
 	mailer := a.Mailer()
 	ctx := r.Context()
 	config := a.config
 	referrerURL := utilities.GetReferrer(r, config)
 	externalURL := getExternalHost(ctx)
 
+	if emailActionType != mail.EmailChangeVerification {
+		if u.GetEmail() != "" && !a.checkEmailAddressAuthorization(u.GetEmail()) {
+			return badRequestError(ErrorCodeEmailAddressNotAuthorized, "Email address %q cannot be used as it is not authorized", u.GetEmail())
+		}
+	} else {
+		// first check that the user can update their address to the
+		// new one in u.EmailChange
+		if u.EmailChange != "" && !a.checkEmailAddressAuthorization(u.EmailChange) {
+			return badRequestError(ErrorCodeEmailAddressNotAuthorized, "Email address %q cannot be used as it is not authorized", u.EmailChange)
+		}
+
+		// if secure email change is enabled, check that the user
+		// account (which could have been created before the authorized
+		// address authorization restriction was enabled) can even
+		// receive the confirmation message to the existing address
+		if config.Mailer.SecureEmailChangeEnabled && u.GetEmail() != "" && !a.checkEmailAddressAuthorization(u.GetEmail()) {
+			return badRequestError(ErrorCodeEmailAddressNotAuthorized, "Email address %q cannot be used as it is not authorized", u.GetEmail())
+		}
+	}
+
 	// apply rate limiting before the email is sent out
 	if ok := a.limiterOpts.Email.Allow(); !ok {
 		emailRateLimitCounter.Add(

internal/api/middleware.go

@@ -7,7 +7,6 @@ import (
 	"fmt"
 	"net/http"
 	"net/url"
-	"regexp"
 	"strings"
 	"sync"
 	"time"
@@ -138,46 +137,6 @@ func isIgnoreCaptchaRoute(req *http.Request) bool {
 	return false
 }
 
-var emailLabelPattern = regexp.MustCompile("[+][^@]+@")
-
-// we don't need to enforce the check on these endpoints since they don't send emails
-var containsNonEmailSendingPath = regexp.MustCompile(`^/(admin|token|verify)`)
-
-func (a *API) isValidAuthorizedEmail(w http.ResponseWriter, req *http.Request) (context.Context, error) {
-	ctx := req.Context()
-
-	// skip checking for authorized email addresses if it's an admin request
-	if containsNonEmailSendingPath.MatchString(req.URL.Path) || req.Method == http.MethodGet || req.Method == http.MethodDelete {
-		return ctx, nil
-	}
-
-	var body struct {
-		Email string `json:"email"`
-	}
-
-	if err := retrieveRequestParams(req, &body); err != nil {
-		// let downstream handlers handle the error
-		return ctx, nil
-	}
-	if body.Email == "" {
-		return ctx, nil
-	}
-	email := strings.ToLower(body.Email)
-	if len(a.config.External.Email.AuthorizedAddresses) > 0 {
-		// allow labelled emails when authorization rules are in place
-		normalized := emailLabelPattern.ReplaceAllString(email, "@")
-
-		for _, authorizedAddress := range a.config.External.Email.AuthorizedAddresses {
-			if normalized == authorizedAddress {
-				return ctx, nil
-			}
-		}
-
-		return ctx, badRequestError(ErrorCodeEmailAddressNotAuthorized, "Email address %q cannot be used as it is not authorized", email)
-	}
-	return ctx, nil
-}
-
 func (a *API) isValidExternalHost(w http.ResponseWriter, req *http.Request) (context.Context, error) {
 	ctx := req.Context()
 	config := a.config

internal/api/middleware_test.go

@@ -341,66 +341,3 @@ func (ts *MiddlewareTestSuite) TestLimitHandler() {
 	ts.API.limitHandler(lmt).handler(okHandler).ServeHTTP(w, req)
 	require.Equal(ts.T(), http.StatusTooManyRequests, w.Code)
 }
-
-func (ts *MiddlewareTestSuite) TestIsValidAuthorizedEmail() {
-	ts.API.config.External.Email.AuthorizedAddresses = []string{"valid@example.com"}
-
-	cases := []struct {
-		desc    string
-		reqPath string
-		body    map[string]interface{}
-	}{
-		{
-			desc:    "bypass check for admin endpoints",
-			reqPath: "/admin",
-			body: map[string]interface{}{
-				"email": "test@example.com",
-			},
-		},
-		{
-			desc:    "bypass check for token endpoint",
-			reqPath: "/token",
-			body: map[string]interface{}{
-				"email": "valid@example.com",
-			},
-		},
-		{
-			desc:    "bypass check for verify endpoint",
-			reqPath: "/verify",
-			body: map[string]interface{}{
-				"email": "valid@example.com",
-			},
-		},
-		{
-			desc:    "bypass check if no email in request body",
-			reqPath: "/signup",
-			body:    map[string]interface{}{},
-		},
-		{
-			desc:    "email not in authorized list",
-			reqPath: "/signup",
-			body: map[string]interface{}{
-				"email": "invalid@example.com",
-			},
-		},
-		{
-			desc:    "email in authorized list",
-			reqPath: "/signup",
-			body: map[string]interface{}{
-				"email": "valid@example.com",
-			},
-		},
-	}
-
-	for _, c := range cases {
-		ts.Run(c.desc, func() {
-			var buffer bytes.Buffer
-			require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(c.body))
-			req := httptest.NewRequest(http.MethodPost, "http://localhost"+c.reqPath, &buffer)
-			w := httptest.NewRecorder()
-			if _, err := ts.API.isValidAuthorizedEmail(w, req); err != nil {
-				require.Equal(ts.T(), err.(*HTTPError).ErrorCode, ErrorCodeEmailAddressNotAuthorized)
-			}
-		})
-	}
-}