Skip to content

Commit

Permalink
fix: apply authorized email restriction to non-admin routes (#1778)
Browse files Browse the repository at this point in the history
## What kind of change does this PR introduce?
* Move the email restriction validation to the middleware rather than
doing it in the `validateEmail` function
* Excludes requests made to the `/admin` endpoints and any `GET` and
`DELETE` requests

## What is the current behavior?

Please link any relevant issues here.

## What is the new behavior?

Feel free to include screenshots if it includes visual changes.

## Additional context

Add any other context or screenshots.
  • Loading branch information
kangmingtay authored Sep 27, 2024
1 parent ba00f75 commit 1af203f
Show file tree
Hide file tree
Showing 6 changed files with 91 additions and 54 deletions.
1 change: 1 addition & 0 deletions internal/api/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ 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)

Expand Down
3 changes: 3 additions & 0 deletions internal/api/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,9 @@ type RequestParams interface {
struct {
Email string `json:"email"`
Phone string `json:"phone"`
} |
struct {
Email string `json:"email"`
}
}

Expand Down
19 changes: 0 additions & 19 deletions internal/api/mail.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@ package api

import (
"net/http"
"regexp"
"strings"
"time"

"github.com/didip/tollbooth/v5"
Expand Down Expand Up @@ -550,8 +548,6 @@ func (a *API) sendEmailChange(r *http.Request, tx *storage.Connection, u *models
return nil
}

var emailLabelPattern = regexp.MustCompile("[+][^@]+@")

func (a *API) validateEmail(email string) (string, error) {
if email == "" {
return "", badRequestError(ErrorCodeValidationFailed, "An email address is required")
Expand All @@ -563,21 +559,6 @@ func (a *API) validateEmail(email string) (string, error) {
return "", badRequestError(ErrorCodeValidationFailed, "Unable to validate email address: "+err.Error())
}

email = strings.ToLower(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 email, nil
}
}

return "", badRequestError(ErrorCodeEmailAddressNotAuthorized, "Email address %q cannot be used as it is not authorized", email)
}

return email, nil
}

Expand Down
35 changes: 0 additions & 35 deletions internal/api/mail_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,41 +48,6 @@ func (ts *MailTestSuite) SetupTest() {
require.NoError(ts.T(), ts.API.db.Create(u), "Error saving new user")
}

func (ts *MailTestSuite) TestValidateEmailAuthorizedAddresses() {
ts.Config.External.Email.AuthorizedAddresses = []string{"someone-a@example.com", "someone-b@example.com"}
defer func() {
ts.Config.External.Email.AuthorizedAddresses = nil
}()

positiveExamples := []string{
"someone-a@example.com",
"someone-b@example.com",
"someone-a+test-1@example.com",
"someone-b+test-2@example.com",
"someone-A@example.com",
"someone-B@example.com",
"someone-a@Example.com",
"someone-b@Example.com",
}

negativeExamples := []string{
"someone@example.com",
"s.omeone@example.com",
"someone-a+@example.com",
"someone+a@example.com",
}

for _, example := range positiveExamples {
_, err := ts.API.validateEmail(example)
require.NoError(ts.T(), err)
}

for _, example := range negativeExamples {
_, err := ts.API.validateEmail(example)
require.Error(ts.T(), err)
}
}

func (ts *MailTestSuite) TestGenerateLink() {
// create admin jwt
claims := &AccessTokenClaims{
Expand Down
38 changes: 38 additions & 0 deletions internal/api/middleware.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"fmt"
"net/http"
"net/url"
"regexp"
"strings"
"sync"
"time"
Expand Down Expand Up @@ -170,6 +171,43 @@ func isIgnoreCaptchaRoute(req *http.Request) bool {
return false
}

var emailLabelPattern = regexp.MustCompile("[+][^@]+@")

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 strings.HasPrefix(req.URL.Path, "/admin") || 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
Expand Down
49 changes: 49 additions & 0 deletions internal/api/middleware_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -515,3 +515,52 @@ func (ts *MiddlewareTestSuite) TestLimitHandlerWithSharedLimiter() {
})
}
}

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

0 comments on commit 1af203f

Please sign in to comment.