From c0c5b23728c8fb633dae23aa4b29ed60e2691a2b Mon Sep 17 00:00:00 2001 From: Stojan Dimitrovski Date: Tue, 15 Oct 2024 19:19:53 +0200 Subject: [PATCH] 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 | 1 - internal/api/mail.go | 53 ++++++++++++++++++++++++++- internal/api/middleware.go | 41 --------------------- internal/api/middleware_test.go | 63 --------------------------------- 4 files changed, 52 insertions(+), 106 deletions(-) diff --git a/internal/api/api.go b/internal/api/api.go index 580280aa5..aafcff22f 100644 --- a/internal/api/api.go +++ b/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) diff --git a/internal/api/mail.go b/internal/api/mail.go index 440e04c90..5b31f4878 100644 --- a/internal/api/mail.go +++ b/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,6 +581,25 @@ 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() @@ -576,6 +607,26 @@ func (a *API) sendEmail(r *http.Request, tx *storage.Connection, u *models.User, 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( diff --git a/internal/api/middleware.go b/internal/api/middleware.go index 1ad8e8687..7103c29a6 100644 --- a/internal/api/middleware.go +++ b/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 diff --git a/internal/api/middleware_test.go b/internal/api/middleware_test.go index 4d529068b..ced249f6c 100644 --- a/internal/api/middleware_test.go +++ b/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) - } - }) - } -}