From 7a2be3e6f62d4b92501cd28d94ef8af4cc351c16 Mon Sep 17 00:00:00 2001 From: Jeremy Wharton Date: Mon, 21 Nov 2022 12:58:42 -0600 Subject: [PATCH] private/web,satellite/console/.../consoleapi: serve rate limiting errors as JSON This change causes rate limiting errors to be returned to the client as JSON objects rather than plain text to prevent the satellite UI from encountering issues when trying to parse them. Resolves storj/customer-issues#88 Change-Id: I11abd19068927a22f1c28d18fc99e7dad8461834 --- private/web/error.go | 45 +++++++++++++++++++ private/web/ratelimiter.go | 18 +++++--- private/web/ratelimiter_test.go | 3 +- .../consoleweb/consoleapi/abtesting.go | 9 ++-- .../consoleweb/consoleapi/analytics.go | 3 +- .../console/consoleweb/consoleapi/apikeys.go | 3 +- .../console/consoleweb/consoleapi/auth.go | 4 +- .../console/consoleweb/consoleapi/buckets.go | 3 +- .../console/consoleweb/consoleapi/common.go | 36 --------------- .../console/consoleweb/consoleapi/payments.go | 3 +- .../console/consoleweb/consoleapi/projects.go | 3 +- .../consoleweb/consoleapi/usagelimits.go | 3 +- satellite/console/consoleweb/server.go | 10 ++--- web/satellite/src/api/auth.ts | 18 ++++---- 14 files changed, 94 insertions(+), 67 deletions(-) create mode 100644 private/web/error.go diff --git a/private/web/error.go b/private/web/error.go new file mode 100644 index 000000000..6e90418ed --- /dev/null +++ b/private/web/error.go @@ -0,0 +1,45 @@ +// Copyright (C) 2022 Storj Labs, Inc. +// See LICENSE for copying information. + +package web + +import ( + "encoding/json" + "net/http" + + "go.uber.org/zap" +) + +// ServeJSONError writes a JSON error to the response output stream. +func ServeJSONError(log *zap.Logger, w http.ResponseWriter, status int, err error) { + ServeCustomJSONError(log, w, status, err, err.Error()) +} + +// ServeCustomJSONError writes a JSON error with a custom message to the response output stream. +func ServeCustomJSONError(log *zap.Logger, w http.ResponseWriter, status int, err error, msg string) { + fields := []zap.Field{ + zap.Int("code", status), + zap.String("message", msg), + zap.Error(err), + } + switch status { + case http.StatusNoContent: + return + case http.StatusInternalServerError: + log.Error("returning error to client", fields...) + case http.StatusBadRequest: + log.Debug("returning error to client", fields...) + case http.StatusTooManyRequests: + default: + log.Info("returning error to client", fields...) + } + + w.WriteHeader(status) + + err = json.NewEncoder(w).Encode(map[string]string{ + "error": msg, + }) + if err != nil { + log.Error("failed to write json error response", zap.Error(err)) + } +} diff --git a/private/web/ratelimiter.go b/private/web/ratelimiter.go index ff2d630d1..be93f9327 100644 --- a/private/web/ratelimiter.go +++ b/private/web/ratelimiter.go @@ -11,9 +11,16 @@ import ( "sync" "time" + "github.com/zeebo/errs" + "go.uber.org/zap" "golang.org/x/time/rate" ) +const ( + internalServerErrMsg = "An internal server error has occurred." + rateLimitErrMsg = "You've exceeded your request limit. Please try again later." +) + // RateLimiterConfig configures a RateLimiter. type RateLimiterConfig struct { Duration time.Duration `help:"the rate at which request are allowed" default:"5m"` @@ -24,6 +31,7 @@ type RateLimiterConfig struct { // RateLimiter imposes a rate limit per key. type RateLimiter struct { config RateLimiterConfig + log *zap.Logger mu sync.Mutex limits map[string]*userLimit keyFunc func(*http.Request) (string, error) @@ -36,12 +44,12 @@ type userLimit struct { } // NewIPRateLimiter constructs a RateLimiter that limits based on IP address. -func NewIPRateLimiter(config RateLimiterConfig) *RateLimiter { - return NewRateLimiter(config, GetRequestIP) +func NewIPRateLimiter(config RateLimiterConfig, log *zap.Logger) *RateLimiter { + return NewRateLimiter(config, log, GetRequestIP) } // NewRateLimiter constructs a RateLimiter. -func NewRateLimiter(config RateLimiterConfig, keyFunc func(*http.Request) (string, error)) *RateLimiter { +func NewRateLimiter(config RateLimiterConfig, log *zap.Logger, keyFunc func(*http.Request) (string, error)) *RateLimiter { return &RateLimiter{ config: config, limits: make(map[string]*userLimit), @@ -79,12 +87,12 @@ func (rl *RateLimiter) Limit(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { key, err := rl.keyFunc(r) if err != nil { - http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError) + ServeCustomJSONError(rl.log, w, http.StatusInternalServerError, err, internalServerErrMsg) return } limit := rl.getUserLimit(key) if !limit.Allow() { - http.Error(w, http.StatusText(http.StatusTooManyRequests), http.StatusTooManyRequests) + ServeJSONError(rl.log, w, http.StatusTooManyRequests, errs.New(rateLimitErrMsg)) return } next.ServeHTTP(w, r) diff --git a/private/web/ratelimiter_test.go b/private/web/ratelimiter_test.go index 0deb7b764..34d6166c6 100644 --- a/private/web/ratelimiter_test.go +++ b/private/web/ratelimiter_test.go @@ -12,6 +12,7 @@ import ( "github.com/spf13/pflag" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/zap/zaptest" "storj.io/common/testcontext" "storj.io/private/cfgstruct" @@ -23,7 +24,7 @@ func TestNewIPRateLimiter(t *testing.T) { config := web.RateLimiterConfig{} cfgstruct.Bind(&pflag.FlagSet{}, &config, cfgstruct.UseDevDefaults()) config.NumLimits = 2 - rateLimiter := web.NewIPRateLimiter(config) + rateLimiter := web.NewIPRateLimiter(config, zaptest.NewLogger(t)) // run ratelimiter cleanup until end of test ctx := testcontext.New(t) diff --git a/satellite/console/consoleweb/consoleapi/abtesting.go b/satellite/console/consoleweb/consoleapi/abtesting.go index 610ef8ce6..78f0327a7 100644 --- a/satellite/console/consoleweb/consoleapi/abtesting.go +++ b/satellite/console/consoleweb/consoleapi/abtesting.go @@ -11,6 +11,7 @@ import ( "github.com/zeebo/errs" "go.uber.org/zap" + "storj.io/storj/private/web" "storj.io/storj/satellite/abtesting" "storj.io/storj/satellite/console" ) @@ -40,13 +41,13 @@ func (a *ABTesting) GetABValues(w http.ResponseWriter, r *http.Request) { user, err := console.GetUser(ctx) if err != nil { - ServeJSONError(a.log, w, http.StatusUnauthorized, err) + web.ServeJSONError(a.log, w, http.StatusUnauthorized, err) return } values, err := a.service.GetABValues(ctx, *user) if err != nil { - ServeJSONError(a.log, w, http.StatusInternalServerError, err) + web.ServeJSONError(a.log, w, http.StatusInternalServerError, err) } w.Header().Set("Content-Type", "application/json") @@ -65,13 +66,13 @@ func (a *ABTesting) SendHit(w http.ResponseWriter, r *http.Request) { action := mux.Vars(r)["action"] if action == "" { - ServeJSONError(a.log, w, http.StatusBadRequest, errs.New("parameter 'action' can't be empty")) + web.ServeJSONError(a.log, w, http.StatusBadRequest, errs.New("parameter 'action' can't be empty")) return } user, err := console.GetUser(ctx) if err != nil { - ServeJSONError(a.log, w, http.StatusUnauthorized, err) + web.ServeJSONError(a.log, w, http.StatusUnauthorized, err) return } diff --git a/satellite/console/consoleweb/consoleapi/analytics.go b/satellite/console/consoleweb/consoleapi/analytics.go index 838bc8f74..3489351df 100644 --- a/satellite/console/consoleweb/consoleapi/analytics.go +++ b/satellite/console/consoleweb/consoleapi/analytics.go @@ -11,6 +11,7 @@ import ( "github.com/zeebo/errs" "go.uber.org/zap" + "storj.io/storj/private/web" "storj.io/storj/satellite/analytics" "storj.io/storj/satellite/console" ) @@ -101,5 +102,5 @@ func (a *Analytics) PageEventTriggered(w http.ResponseWriter, r *http.Request) { // serveJSONError writes JSON error to response output stream. func (a *Analytics) serveJSONError(w http.ResponseWriter, status int, err error) { - ServeJSONError(a.log, w, status, err) + web.ServeJSONError(a.log, w, status, err) } diff --git a/satellite/console/consoleweb/consoleapi/apikeys.go b/satellite/console/consoleweb/consoleapi/apikeys.go index aa8d2cd5d..020367822 100644 --- a/satellite/console/consoleweb/consoleapi/apikeys.go +++ b/satellite/console/consoleweb/consoleapi/apikeys.go @@ -10,6 +10,7 @@ import ( "go.uber.org/zap" "storj.io/common/uuid" + "storj.io/storj/private/web" "storj.io/storj/satellite/console" ) @@ -71,5 +72,5 @@ func (keys *APIKeys) DeleteByNameAndProjectID(w http.ResponseWriter, r *http.Req // serveJSONError writes JSON error to response output stream. func (keys *APIKeys) serveJSONError(w http.ResponseWriter, status int, err error) { - ServeJSONError(keys.log, w, status, err) + web.ServeJSONError(keys.log, w, status, err) } diff --git a/satellite/console/consoleweb/consoleapi/auth.go b/satellite/console/consoleweb/consoleapi/auth.go index 4eadd627b..3a8389b51 100644 --- a/satellite/console/consoleweb/consoleapi/auth.go +++ b/satellite/console/consoleweb/consoleapi/auth.go @@ -103,7 +103,7 @@ func (a *Auth) Token(w http.ResponseWriter, r *http.Request) { tokenInfo, err := a.service.Token(ctx, tokenRequest) if err != nil { if console.ErrMFAMissing.Has(err) { - serveCustomJSONError(a.log, w, http.StatusOK, err, a.getUserErrorMessage(err)) + web.ServeCustomJSONError(a.log, w, http.StatusOK, err, a.getUserErrorMessage(err)) } else { a.log.Info("Error authenticating token request", zap.String("email", tokenRequest.Email), zap.Error(ErrAuthAPI.Wrap(err))) a.serveJSONError(w, err) @@ -838,7 +838,7 @@ func (a *Auth) RefreshSession(w http.ResponseWriter, r *http.Request) { // serveJSONError writes JSON error to response output stream. func (a *Auth) serveJSONError(w http.ResponseWriter, err error) { status := a.getStatusCode(err) - serveCustomJSONError(a.log, w, status, err, a.getUserErrorMessage(err)) + web.ServeCustomJSONError(a.log, w, status, err, a.getUserErrorMessage(err)) } // getStatusCode returns http.StatusCode depends on console error class. diff --git a/satellite/console/consoleweb/consoleapi/buckets.go b/satellite/console/consoleweb/consoleapi/buckets.go index eb95763fc..367c1771b 100644 --- a/satellite/console/consoleweb/consoleapi/buckets.go +++ b/satellite/console/consoleweb/consoleapi/buckets.go @@ -11,6 +11,7 @@ import ( "go.uber.org/zap" "storj.io/common/uuid" + "storj.io/storj/private/web" "storj.io/storj/satellite/console" ) @@ -68,5 +69,5 @@ func (b *Buckets) AllBucketNames(w http.ResponseWriter, r *http.Request) { // serveJSONError writes JSON error to response output stream. func (b *Buckets) serveJSONError(w http.ResponseWriter, status int, err error) { - ServeJSONError(b.log, w, status, err) + web.ServeJSONError(b.log, w, status, err) } diff --git a/satellite/console/consoleweb/consoleapi/common.go b/satellite/console/consoleweb/consoleapi/common.go index db766fa26..1c00b3153 100644 --- a/satellite/console/consoleweb/consoleapi/common.go +++ b/satellite/console/consoleweb/consoleapi/common.go @@ -5,12 +5,9 @@ package consoleapi import ( "context" - "encoding/json" - "net/http" "sync" "github.com/zeebo/errs" - "go.uber.org/zap" ) var ( @@ -18,39 +15,6 @@ var ( ErrUtils = errs.Class("console api utils") ) -// ServeJSONError writes a JSON error to the response output stream. -func ServeJSONError(log *zap.Logger, w http.ResponseWriter, status int, err error) { - serveCustomJSONError(log, w, status, err, err.Error()) -} - -// serveCustomJSONError writes a JSON error with a custom message to the response output stream. -func serveCustomJSONError(log *zap.Logger, w http.ResponseWriter, status int, err error, msg string) { - fields := []zap.Field{ - zap.Int("code", status), - zap.String("message", msg), - zap.Error(err), - } - switch status { - case http.StatusNoContent: - return - case http.StatusInternalServerError: - log.Error("returning error to client", fields...) - case http.StatusBadRequest: - log.Debug("returning error to client", fields...) - default: - log.Info("returning error to client", fields...) - } - - w.WriteHeader(status) - - err = json.NewEncoder(w).Encode(map[string]string{ - "error": msg, - }) - if err != nil { - log.Error("failed to write json error response", zap.Error(ErrUtils.Wrap(err))) - } -} - // ContextChannel is a generic, context-aware channel. type ContextChannel struct { mu sync.Mutex diff --git a/satellite/console/consoleweb/consoleapi/payments.go b/satellite/console/consoleweb/consoleapi/payments.go index b7130f1b1..2c6a1da02 100644 --- a/satellite/console/consoleweb/consoleapi/payments.go +++ b/satellite/console/consoleweb/consoleapi/payments.go @@ -15,6 +15,7 @@ import ( "github.com/zeebo/errs" "go.uber.org/zap" + "storj.io/storj/private/web" "storj.io/storj/satellite/console" "storj.io/storj/satellite/payments/stripecoinpayments" ) @@ -390,5 +391,5 @@ func (p *Payments) WalletPayments(w http.ResponseWriter, r *http.Request) { // serveJSONError writes JSON error to response output stream. func (p *Payments) serveJSONError(w http.ResponseWriter, status int, err error) { - ServeJSONError(p.log, w, status, err) + web.ServeJSONError(p.log, w, status, err) } diff --git a/satellite/console/consoleweb/consoleapi/projects.go b/satellite/console/consoleweb/consoleapi/projects.go index 871c9593d..953ba4533 100644 --- a/satellite/console/consoleweb/consoleapi/projects.go +++ b/satellite/console/consoleweb/consoleapi/projects.go @@ -13,6 +13,7 @@ import ( "go.uber.org/zap" "storj.io/common/uuid" + "storj.io/storj/private/web" "storj.io/storj/satellite/console" ) @@ -65,5 +66,5 @@ func (p *Projects) GetSalt(w http.ResponseWriter, r *http.Request) { // serveJSONError writes JSON error to response output stream. func (p *Projects) serveJSONError(w http.ResponseWriter, status int, err error) { - ServeJSONError(p.log, w, status, err) + web.ServeJSONError(p.log, w, status, err) } diff --git a/satellite/console/consoleweb/consoleapi/usagelimits.go b/satellite/console/consoleweb/consoleapi/usagelimits.go index c65f71c49..89186ffab 100644 --- a/satellite/console/consoleweb/consoleapi/usagelimits.go +++ b/satellite/console/consoleweb/consoleapi/usagelimits.go @@ -14,6 +14,7 @@ import ( "go.uber.org/zap" "storj.io/common/uuid" + "storj.io/storj/private/web" "storj.io/storj/satellite/accounting" "storj.io/storj/satellite/console" ) @@ -155,5 +156,5 @@ func (ul *UsageLimits) DailyUsage(w http.ResponseWriter, r *http.Request) { // serveJSONError writes JSON error to response output stream. func (ul *UsageLimits) serveJSONError(w http.ResponseWriter, status int, err error) { - ServeJSONError(ul.log, w, status, err) + web.ServeJSONError(ul.log, w, status, err) } diff --git a/satellite/console/consoleweb/server.go b/satellite/console/consoleweb/server.go index 315ed803c..7edb29841 100644 --- a/satellite/console/consoleweb/server.go +++ b/satellite/console/consoleweb/server.go @@ -217,8 +217,8 @@ func NewServer(logger *zap.Logger, config Config, service *console.Service, oidc analytics: analytics, abTesting: abTesting, stripePublicKey: stripePublicKey, - ipRateLimiter: web.NewIPRateLimiter(config.RateLimit), - userIDRateLimiter: NewUserIDRateLimiter(config.RateLimit), + ipRateLimiter: web.NewIPRateLimiter(config.RateLimit, logger), + userIDRateLimiter: NewUserIDRateLimiter(config.RateLimit, logger), nodeURL: nodeURL, pricing: pricing, } @@ -548,7 +548,7 @@ func (server *Server) withAuth(handler http.Handler) http.Handler { defer func() { if err != nil { - consoleapi.ServeJSONError(server.log, w, http.StatusUnauthorized, console.ErrUnauthorized.Wrap(err)) + web.ServeJSONError(server.log, w, http.StatusUnauthorized, console.ErrUnauthorized.Wrap(err)) server.cookieAuth.RemoveTokenCookie(w) } }() @@ -991,8 +991,8 @@ func (server *Server) parseTemplates() (_ *templates, err error) { } // NewUserIDRateLimiter constructs a RateLimiter that limits based on user ID. -func NewUserIDRateLimiter(config web.RateLimiterConfig) *web.RateLimiter { - return web.NewRateLimiter(config, func(r *http.Request) (string, error) { +func NewUserIDRateLimiter(config web.RateLimiterConfig, log *zap.Logger) *web.RateLimiter { + return web.NewRateLimiter(config, log, func(r *http.Request) (string, error) { user, err := console.GetUser(r.Context()) if err != nil { return "", err diff --git a/web/satellite/src/api/auth.ts b/web/satellite/src/api/auth.ts index 3b4cc3ae6..3884a1b25 100644 --- a/web/satellite/src/api/auth.ts +++ b/web/satellite/src/api/auth.ts @@ -15,7 +15,6 @@ import { HttpClient } from '@/utils/httpClient'; export class AuthHttpApi implements UsersApi { private readonly http: HttpClient = new HttpClient(); private readonly ROOT_PATH: string = '/api/v0/auth'; - private readonly rateLimitErrMsg = 'You\'ve exceeded limit of attempts, try again in 5 minutes'; /** * Used to resend an registration confirmation email. @@ -30,11 +29,14 @@ export class AuthHttpApi implements UsersApi { return; } - if (response.status == 429) { - throw new ErrorTooManyRequests(this.rateLimitErrMsg); + const result = await response.json(); + const errMsg = result.error || 'Failed to send email'; + switch (response.status) { + case 429: + throw new ErrorTooManyRequests(errMsg); + default: + throw new Error(errMsg); } - - throw new Error('Failed to send email'); } /** @@ -75,7 +77,7 @@ export class AuthHttpApi implements UsersApi { case 401: throw new ErrorUnauthorized(errMsg); case 429: - throw new ErrorTooManyRequests(this.rateLimitErrMsg); + throw new ErrorTooManyRequests(errMsg); default: throw new Error(errMsg); } @@ -125,7 +127,7 @@ export class AuthHttpApi implements UsersApi { case 404: throw new ErrorUnauthorized(errMsg); case 429: - throw new ErrorTooManyRequests(this.rateLimitErrMsg); + throw new ErrorTooManyRequests(errMsg); default: throw new Error(errMsg); } @@ -279,7 +281,7 @@ export class AuthHttpApi implements UsersApi { case 401: throw new ErrorUnauthorized(errMsg); case 429: - throw new ErrorTooManyRequests(this.rateLimitErrMsg); + throw new ErrorTooManyRequests(errMsg); default: throw new Error(errMsg); }