From c94ec7d9193bea9bd84a24d367de987dbe6f019a Mon Sep 17 00:00:00 2001 From: Caleb Bae Date: Wed, 2 Sep 2026 12:28:59 -0500 Subject: [PATCH] fix: key rate limiter by session user with per-IP fallback --- .claude/skills/backend/references/testing.md | 3 +- .env.example | 8 +- claude.md | 5 +- cmd/api/api.go | 23 ++- cmd/api/errors.go | 4 +- cmd/api/main.go | 19 +- cmd/api/middlewares.go | 54 +++++- cmd/api/middlewares_test.go | 189 +++++++++++++++---- cmd/api/test_utils_test.go | 32 +++- internal/ratelimiter/fixed-window.go | 12 +- internal/ratelimiter/ratelimiter.go | 10 +- 11 files changed, 282 insertions(+), 77 deletions(-) diff --git a/.claude/skills/backend/references/testing.md b/.claude/skills/backend/references/testing.md index adcd6720d..f775c146c 100644 --- a/.claude/skills/backend/references/testing.md +++ b/.claude/skills/backend/references/testing.md @@ -10,7 +10,8 @@ All in `cmd/api/test_utils_test.go`. Use these instead of duplicating setup. | Helper | What it does | |--------|-------------| -| `newTestApplication(t)` | Build `*application` with mock store, no-op zap logger, mock GCS, mock mailer, real fixed-window rate limiter (20/5s), basic auth `testuser:testpass`, public API key `test-api-key`. | +| `newTestApplication(t)` | Build `*application` with mock store, no-op zap logger, mock GCS, mock mailer, real fixed-window rate limiters (20/5s per user, 200/5s per IP) with the real SuperTokens session resolver (no cookie → IP fallback, no core contact), basic auth `testuser:testpass`, public API key `test-api-key`. | +| `headerSessionUserID` | Drop-in for `app.sessionUserID` that reads the session user from the `X-Test-Session-User` header (`testSessionUserHeader`), so rate limiter tests can vary the user per request without a running core. | | `executeRequest(req, mux)` | Run req through handler/mux, return `*httptest.ResponseRecorder`. | | `checkResponseCode(t, expected, actual)` | Assert status code with descriptive failure. | | `addBasicAuth(req)` | Set `Authorization: Basic ` header. | diff --git a/.env.example b/.env.example index cbf585226..82fa37d3a 100644 --- a/.env.example +++ b/.env.example @@ -151,9 +151,15 @@ VAPID_SUBJECT=noreply@example.com RATE_LIMITER_ENABLED=true -# Requests per IP per five-second window. +# Requests per signed-in user per five-second window. Applies to /v1/* only. RATELIMITER_REQUESTS_COUNT=20 +# Requests per client IP per five-second window for /v1/* calls that do not +# carry a verified session (login page lookups, the first request after an +# access token expires). Larger than the per-user budget because every hacker +# at the venue typically shares one IP. +RATELIMITER_IP_REQUESTS_COUNT=200 + # ── Apple Wallet passes (optional) ─────────────────────────────────────────── diff --git a/claude.md b/claude.md index 3c79349bb..fd2e67edc 100644 --- a/claude.md +++ b/claude.md @@ -55,7 +55,8 @@ Note: `air` runs `task gen-docs` as a pre-command on every rebuild, so `swag` CL - **Entry point:** `cmd/api/main.go` — loads config, `cmd/api/api.go` — Chi router setup in `mount()` - **Database:** PostgreSQL 16.3, raw SQL (no ORM), repository pattern in `internal/store/` - **Auth:** SuperTokens (Passwordless magic link + Google OAuth), initialized in `internal/auth/` -- **Middleware chain:** RequestID → RealIP → Logger → Recoverer → CORS → SuperTokens → RateLimiter → AuthRequired → RequireRole +- **Middleware chain:** RequestID → RealIP → Logger → Recoverer → CORS → SuperTokens → RateLimiter (`/v1` only) → AuthRequired → RequireRole +- **Rate limiting:** keyed by SuperTokens user ID when the request carries a verified session (`RATELIMITER_REQUESTS_COUNT`), falling back to client IP otherwise (`RATELIMITER_IP_REQUESTS_COUNT`, larger because a whole venue shares one NAT). Static assets and `/auth/*` are never limited. - **Roles (hierarchical):** `hacker` (1) < `admin` (2) < `super_admin` (3) - **JSON envelope:** Success: `{"data": ...}`, Error: `{"error": "..."}` - **Pagination:** Cursor-based with base64-encoded JSON cursors @@ -100,7 +101,7 @@ Tests live in `cmd/api/` (`_test.go` files, same package as handlers): - `internal/db/` — PostgreSQL connection setup (pgx) - `internal/mailer/` — SendGrid email with embedded Go templates - `internal/auth/` — SuperTokens init, user creation from session -- `internal/ratelimiter/` — fixed-window rate limiter +- `internal/ratelimiter/` — fixed-window rate limiter (one instance per user-ID bucket, one per IP bucket) - `internal/logger/` — Zap logger (dev/prod modes based on `ENV`) ### Frontend (React 19 + TypeScript + Vite) diff --git a/cmd/api/api.go b/cmd/api/api.go index 48ce80efa..679d1202a 100644 --- a/cmd/api/api.go +++ b/cmd/api/api.go @@ -29,8 +29,15 @@ type application struct { mailer mailer.Client gcsClient gcs.Client appleWalletPasses appleWalletPassGenerator - rateLimiter ratelimiter.Limiter - dispatcherCancel context.CancelFunc + // rateLimiter buckets requests by verified session user ID; ipRateLimiter + // is the fallback for requests without one. Split so the shared-IP budget + // (a venue full of hackers behind one NAT) can be tuned independently. + rateLimiter ratelimiter.Limiter + ipRateLimiter ratelimiter.Limiter + // sessionUserID resolves the SuperTokens user ID for a request without + // requiring a session. Injected so tests can stub it. + sessionUserID sessionUserIDResolver + dispatcherCancel context.CancelFunc } type config struct { @@ -136,12 +143,14 @@ func (app *application) mount() http.Handler { // Applied at root level so it intercepts /auth/* requests. r.Use(supertokens.Middleware) - // Ratelimiter - if app.config.rateLimiter.Enabled { - r.Use(app.RateLimiterMiddleware) - } - r.Route("/v1", func(r chi.Router) { + // Ratelimiter. Scoped to the API so the SPA shell and static assets + // (served from /*) are never throttled; /auth/* is handled by the + // SuperTokens middleware above and never reaches this router. + if app.config.rateLimiter.Enabled { + r.Use(app.RateLimiterMiddleware) + } + // Public API (key auth) r.Route("/public", func(r chi.Router) { r.Use(app.APIKeyMiddleware) diff --git a/cmd/api/errors.go b/cmd/api/errors.go index f5685cac0..0f926b4ec 100644 --- a/cmd/api/errors.go +++ b/cmd/api/errors.go @@ -58,8 +58,8 @@ func (app *application) unauthorizedBasicErrorResponse(w http.ResponseWriter, r writeJSONError(w, http.StatusUnauthorized, "unauthorized") } -func (app *application) rateLimiterExceededResponse(w http.ResponseWriter, r *http.Request, retryAfter string) { - app.logger.Warnw("rate limit exceeded", "method", r.Method, "path", r.URL.Path) +func (app *application) rateLimiterExceededResponse(w http.ResponseWriter, r *http.Request, key, retryAfter string) { + app.logger.Warnw("rate limit exceeded", "method", r.Method, "path", r.URL.Path, "key", key) w.Header().Set("Retry-After", retryAfter) diff --git a/cmd/api/main.go b/cmd/api/main.go index a98ddc49f..a695a9652 100644 --- a/cmd/api/main.go +++ b/cmd/api/main.go @@ -89,10 +89,13 @@ func main() { publicAPIKey: env.GetString("PUBLIC_API_KEY", ""), }, rateLimiter: ratelimiter.Config{ - // Limit 20 requests every 5 seconds per IP - RequestPerTimeFrame: env.GetInt("RATELIMITER_REQUESTS_COUNT", 20), - TimeFrame: time.Second * 5, - Enabled: env.GetBool("RATE_LIMITER_ENABLED", true), + // Limit 20 requests every 5 seconds per signed-in user. Requests + // without a verified session fall back to a per-IP bucket with a + // larger budget, since a whole venue can sit behind one NAT. + RequestPerTimeFrame: env.GetInt("RATELIMITER_REQUESTS_COUNT", 20), + IPRequestPerTimeFrame: env.GetInt("RATELIMITER_IP_REQUESTS_COUNT", 200), + TimeFrame: time.Second * 5, + Enabled: env.GetBool("RATE_LIMITER_ENABLED", true), }, frontendURL: frontendURL, publicCORSOrigin: env.GetString("PUBLIC_CORS_ORIGIN", ""), @@ -199,11 +202,15 @@ func main() { logger.Infow("gcs client initialized", "bucket", cfg.gcs.bucketName) } - // Init rate limiter + // Init rate limiters rateLimiter := ratelimiter.NewFixedWindowLimiter( cfg.rateLimiter.RequestPerTimeFrame, cfg.rateLimiter.TimeFrame, ) + ipRateLimiter := ratelimiter.NewFixedWindowLimiter( + cfg.rateLimiter.IPRequestPerTimeFrame, + cfg.rateLimiter.TimeFrame, + ) // Apple Wallet signing is optional. If explicitly enabled, invalid or // incomplete signing material is a deployment error. @@ -224,6 +231,8 @@ func main() { gcsClient: gcsClient, appleWalletPasses: appleWalletPasses, rateLimiter: rateLimiter, + ipRateLimiter: ipRateLimiter, + sessionUserID: supertokensSessionUserID, } // Metrics collected diff --git a/cmd/api/middlewares.go b/cmd/api/middlewares.go index 1d25029b2..ec0b3a175 100644 --- a/cmd/api/middlewares.go +++ b/cmd/api/middlewares.go @@ -6,18 +6,42 @@ import ( "encoding/base64" "errors" "fmt" + "net" "net/http" "strings" "github.com/hackutd/harp/internal/auth" + "github.com/hackutd/harp/internal/ratelimiter" "github.com/hackutd/harp/internal/store" "github.com/supertokens/supertokens-golang/recipe/session" + "github.com/supertokens/supertokens-golang/recipe/session/sessmodels" ) type contextKey string const userContextKey contextKey = "user" +// sessionUserIDResolver reports the SuperTokens user ID behind a request, or +// false when the request carries no verifiable session. +type sessionUserIDResolver func(w http.ResponseWriter, r *http.Request) (string, bool) + +// supertokensSessionUserID reads the session without requiring one. Only a +// signature-verified access token yields a user ID, so a forged token cannot +// mint its own rate-limit bucket. Missing, expired, or invalid tokens report +// false and the caller falls back to the client IP. Verification is local +// (cached JWKS); no call to the SuperTokens core is made. +func supertokensSessionUserID(w http.ResponseWriter, r *http.Request) (string, bool) { + optional := false + sess, err := session.GetSession(r, w, &sessmodels.VerifySessionOptions{ + SessionRequired: &optional, + AntiCsrfCheck: &optional, + }) + if err != nil || sess == nil { + return "", false + } + return sess.GetUserID(), true +} + // Validates HTTP Basic authentication credentials func (app *application) BasicAuthMiddleware(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -49,17 +73,41 @@ func (app *application) BasicAuthMiddleware(next http.Handler) http.Handler { }) } -// Rate limits per IP +// Rate limits per signed-in user, falling back to the client IP for requests +// without a verified session. Per-user buckets keep one venue NAT full of +// hackers from exhausting a single shared budget. func (app *application) RateLimiterMiddleware(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if allow, retryAfter := app.rateLimiter.Allow(r.RemoteAddr); !allow { - app.rateLimiterExceededResponse(w, r, retryAfter.String()) + limiter, key := app.rateLimiterFor(w, r) + if allow, retryAfter := limiter.Allow(key); !allow { + app.rateLimiterExceededResponse(w, r, key, retryAfter.String()) return } next.ServeHTTP(w, r) }) } +// Picks the limiter and bucket key for a request: the per-user limiter keyed +// by SuperTokens user ID when a session verifies, else the per-IP limiter. +func (app *application) rateLimiterFor(w http.ResponseWriter, r *http.Request) (ratelimiter.Limiter, string) { + if app.sessionUserID != nil { + if userID, ok := app.sessionUserID(w, r); ok { + return app.rateLimiter, "user:" + userID + } + } + return app.ipRateLimiter, "ip:" + clientIP(r) +} + +// middleware.RealIP rewrites RemoteAddr to the bare forwarded IP behind a +// proxy, but without one RemoteAddr keeps its port, which would make every +// connection its own bucket. +func clientIP(r *http.Request) string { + if host, _, err := net.SplitHostPort(r.RemoteAddr); err == nil { + return host + } + return r.RemoteAddr +} + // Verifies the SuperTokens session and loads the user into context func (app *application) AuthRequiredMiddleware(next http.Handler) http.Handler { return session.VerifySession(nil, func(w http.ResponseWriter, r *http.Request) { diff --git a/cmd/api/middlewares_test.go b/cmd/api/middlewares_test.go index 106509948..a1590653a 100644 --- a/cmd/api/middlewares_test.go +++ b/cmd/api/middlewares_test.go @@ -3,6 +3,7 @@ package main import ( "fmt" "net/http" + "net/http/httptest" "testing" "time" @@ -175,73 +176,183 @@ func TestRequireRoleMiddleware(t *testing.T) { } func TestRateLimiterMiddleware(t *testing.T) { - t.Run("should allow requests under the limit", func(t *testing.T) { - app := newTestApplication(t) - app.rateLimiter = ratelimiter.NewFixedWindowLimiter(5, 5*time.Second) - - handler := app.RateLimiterMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - })) + ok := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + // remoteAddr is the client IP (with port, as net/http reports it without a + // proxy); user, when non-empty, is the session user the stub resolver reports. + newRequest := func(t *testing.T, remoteAddr, user string) *http.Request { + t.Helper() req, err := http.NewRequest(http.MethodGet, "/", nil) require.NoError(t, err) - req.RemoteAddr = "192.168.1.1:1234" + req.RemoteAddr = remoteAddr + if user != "" { + req.Header.Set(testSessionUserHeader, user) + } + return req + } - rr := executeRequest(req, handler) + t.Run("should allow anonymous requests under the IP limit", func(t *testing.T) { + app := newTestApplication(t) + app.ipRateLimiter = ratelimiter.NewFixedWindowLimiter(5, 5*time.Second) + handler := app.RateLimiterMiddleware(ok) + + rr := executeRequest(newRequest(t, "192.168.1.1:1234", ""), handler) checkResponseCode(t, http.StatusOK, rr.Code) }) - t.Run("should return 429 when limit exceeded", func(t *testing.T) { + t.Run("should return 429 when the IP limit is exceeded", func(t *testing.T) { app := newTestApplication(t) - app.rateLimiter = ratelimiter.NewFixedWindowLimiter(2, 5*time.Second) - - handler := app.RateLimiterMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - })) + app.ipRateLimiter = ratelimiter.NewFixedWindowLimiter(2, 5*time.Second) + handler := app.RateLimiterMiddleware(ok) - // hit limit for i := 0; i < 2; i++ { - req, err := http.NewRequest(http.MethodGet, "/", nil) - require.NoError(t, err) - req.RemoteAddr = "10.0.0.1:1234" - - rr := executeRequest(req, handler) + rr := executeRequest(newRequest(t, "10.0.0.1:1234", ""), handler) checkResponseCode(t, http.StatusOK, rr.Code) } - req, err := http.NewRequest(http.MethodGet, "/", nil) - require.NoError(t, err) - req.RemoteAddr = "10.0.0.1:1234" + rr := executeRequest(newRequest(t, "10.0.0.1:1234", ""), handler) + checkResponseCode(t, http.StatusTooManyRequests, rr.Code) + assert.NotEmpty(t, rr.Header().Get("Retry-After")) + }) + + t.Run("should track IP buckets independently", func(t *testing.T) { + app := newTestApplication(t) + app.ipRateLimiter = ratelimiter.NewFixedWindowLimiter(1, 5*time.Second) + handler := app.RateLimiterMiddleware(ok) + rr := executeRequest(newRequest(t, "10.0.0.2:1234", ""), handler) + checkResponseCode(t, http.StatusOK, rr.Code) + + rr = executeRequest(newRequest(t, "10.0.0.3:1234", ""), handler) + checkResponseCode(t, http.StatusOK, rr.Code) + }) + + t.Run("should ignore the client port when keying by IP", func(t *testing.T) { + app := newTestApplication(t) + app.ipRateLimiter = ratelimiter.NewFixedWindowLimiter(1, 5*time.Second) + handler := app.RateLimiterMiddleware(ok) + + rr := executeRequest(newRequest(t, "10.0.0.4:1111", ""), handler) + checkResponseCode(t, http.StatusOK, rr.Code) + + // same host, new ephemeral port: still the same bucket + rr = executeRequest(newRequest(t, "10.0.0.4:2222", ""), handler) + checkResponseCode(t, http.StatusTooManyRequests, rr.Code) + + // RealIP leaves a bare address; that must also share the bucket + rr = executeRequest(newRequest(t, "10.0.0.4", ""), handler) + checkResponseCode(t, http.StatusTooManyRequests, rr.Code) + }) + + t.Run("should fall back to IP when the access token cookie is not a valid JWT", func(t *testing.T) { + // Exercises the real SuperTokens resolver: an unparseable token must be + // treated as no session, never as an error or a 5xx. + app := newTestApplication(t) + app.ipRateLimiter = ratelimiter.NewFixedWindowLimiter(1, 5*time.Second) + handler := app.RateLimiterMiddleware(ok) + + req := newRequest(t, "10.0.0.5:1234", "") + req.AddCookie(&http.Cookie{Name: "sAccessToken", Value: "not-a-jwt"}) rr := executeRequest(req, handler) + checkResponseCode(t, http.StatusOK, rr.Code) + + req = newRequest(t, "10.0.0.5:1234", "") + req.AddCookie(&http.Cookie{Name: "sAccessToken", Value: "not-a-jwt"}) + rr = executeRequest(req, handler) + checkResponseCode(t, http.StatusTooManyRequests, rr.Code) + }) + + t.Run("should treat a missing resolver as no session", func(t *testing.T) { + app := newTestApplication(t) + app.sessionUserID = nil + app.ipRateLimiter = ratelimiter.NewFixedWindowLimiter(1, 5*time.Second) + handler := app.RateLimiterMiddleware(ok) + + rr := executeRequest(newRequest(t, "10.0.0.6:1234", ""), handler) + checkResponseCode(t, http.StatusOK, rr.Code) + + rr = executeRequest(newRequest(t, "10.0.0.6:1234", ""), handler) checkResponseCode(t, http.StatusTooManyRequests, rr.Code) - assert.NotEmpty(t, rr.Header().Get("Retry-After")) }) - t.Run("should track limits per IP independently", func(t *testing.T) { + t.Run("should key signed-in requests by user regardless of IP", func(t *testing.T) { app := newTestApplication(t) + app.sessionUserID = headerSessionUserID app.rateLimiter = ratelimiter.NewFixedWindowLimiter(1, 5*time.Second) + handler := app.RateLimiterMiddleware(ok) - handler := app.RateLimiterMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - })) + rr := executeRequest(newRequest(t, "10.0.0.7:1234", "st-user-1"), handler) + checkResponseCode(t, http.StatusOK, rr.Code) - // first IP hits limit - req1, err := http.NewRequest(http.MethodGet, "/", nil) - require.NoError(t, err) - req1.RemoteAddr = "10.0.0.2:1234" + // same user from a different network still shares the bucket + rr = executeRequest(newRequest(t, "10.0.0.8:1234", "st-user-1"), handler) + checkResponseCode(t, http.StatusTooManyRequests, rr.Code) + }) + + t.Run("should give signed-in users behind one IP independent buckets", func(t *testing.T) { + // The venue case: many hackers behind a single NAT address. + app := newTestApplication(t) + app.sessionUserID = headerSessionUserID + app.rateLimiter = ratelimiter.NewFixedWindowLimiter(1, 5*time.Second) + handler := app.RateLimiterMiddleware(ok) - rr := executeRequest(req1, handler) + for _, user := range []string{"st-user-1", "st-user-2", "st-user-3"} { + rr := executeRequest(newRequest(t, "203.0.113.10:1234", user), handler) + checkResponseCode(t, http.StatusOK, rr.Code) + } + + rr := executeRequest(newRequest(t, "203.0.113.10:1234", "st-user-1"), handler) + checkResponseCode(t, http.StatusTooManyRequests, rr.Code) + }) + + t.Run("should keep user and IP budgets separate", func(t *testing.T) { + app := newTestApplication(t) + app.sessionUserID = headerSessionUserID + app.rateLimiter = ratelimiter.NewFixedWindowLimiter(1, 5*time.Second) + app.ipRateLimiter = ratelimiter.NewFixedWindowLimiter(1, 5*time.Second) + handler := app.RateLimiterMiddleware(ok) + + // signed-in user exhausts their own bucket + rr := executeRequest(newRequest(t, "203.0.113.20:1234", "st-user-1"), handler) checkResponseCode(t, http.StatusOK, rr.Code) + rr = executeRequest(newRequest(t, "203.0.113.20:1234", "st-user-1"), handler) + checkResponseCode(t, http.StatusTooManyRequests, rr.Code) - // different IP should be allowed - req2, err := http.NewRequest(http.MethodGet, "/", nil) - require.NoError(t, err) - req2.RemoteAddr = "10.0.0.3:1234" + // an anonymous request from the same IP has not been charged + rr = executeRequest(newRequest(t, "203.0.113.20:1234", ""), handler) + checkResponseCode(t, http.StatusOK, rr.Code) + rr = executeRequest(newRequest(t, "203.0.113.20:1234", ""), handler) + checkResponseCode(t, http.StatusTooManyRequests, rr.Code) - rr = executeRequest(req2, handler) + // and a different signed-in user on that IP is unaffected by either + rr = executeRequest(newRequest(t, "203.0.113.20:1234", "st-user-2"), handler) checkResponseCode(t, http.StatusOK, rr.Code) }) + + t.Run("should throttle /v1 routes but never static assets", func(t *testing.T) { + app := newTestApplication(t) + app.ipRateLimiter = ratelimiter.NewFixedWindowLimiter(1, 5*time.Second) + mux := app.mount() + + get := func(path string) *httptest.ResponseRecorder { + req, err := http.NewRequest(http.MethodGet, path, nil) + require.NoError(t, err) + req.RemoteAddr = "10.0.0.9:1234" + return executeRequest(req, mux) + } + + // /v1/health needs Basic auth, so 401 proves the limiter let it through + checkResponseCode(t, http.StatusUnauthorized, get("/v1/health").Code) + checkResponseCode(t, http.StatusTooManyRequests, get("/v1/health").Code) + + // the SPA shell and its assets are served from /* and never counted, + // even though this IP's bucket is exhausted + for _, path := range []string{"/", "/assets/index-abc123.js", "/dashboard", "/auth/verify"} { + assert.NotEqual(t, http.StatusTooManyRequests, get(path).Code, "expected %s to bypass the rate limiter", path) + } + }) } func TestApplicationsEnabledMiddleware(t *testing.T) { diff --git a/cmd/api/test_utils_test.go b/cmd/api/test_utils_test.go index 701c14e1d..1a31387be 100644 --- a/cmd/api/test_utils_test.go +++ b/cmd/api/test_utils_test.go @@ -62,6 +62,7 @@ func newTestApplication(t *testing.T) *application { mockStore := store.NewMockStore() rateLimiter := ratelimiter.NewFixedWindowLimiter(20, 5*time.Second) + ipRateLimiter := ratelimiter.NewFixedWindowLimiter(200, 5*time.Second) return &application{ config: config{ @@ -74,19 +75,34 @@ func newTestApplication(t *testing.T) *application { publicAPIKey: "test-api-key", }, rateLimiter: ratelimiter.Config{ - RequestPerTimeFrame: 20, - TimeFrame: 5 * time.Second, - Enabled: true, + RequestPerTimeFrame: 20, + IPRequestPerTimeFrame: 200, + TimeFrame: 5 * time.Second, + Enabled: true, }, }, - store: mockStore, - logger: logger, - mailer: &mailer.MockClient{}, - gcsClient: &gcs.MockClient{}, - rateLimiter: rateLimiter, + store: mockStore, + logger: logger, + mailer: &mailer.MockClient{}, + gcsClient: &gcs.MockClient{}, + rateLimiter: rateLimiter, + ipRateLimiter: ipRateLimiter, + // Real resolver: with no session cookie it reports false without + // contacting the core, so tests exercise the IP fallback by default. + sessionUserID: supertokensSessionUserID, } } +// Stand-in for SuperTokens session resolution so rate limiter tests can vary +// the user per request without a running core. Requests without the header +// behave like requests with no verifiable session. +const testSessionUserHeader = "X-Test-Session-User" + +func headerSessionUserID(_ http.ResponseWriter, r *http.Request) (string, bool) { + id := r.Header.Get(testSessionUserHeader) + return id, id != "" +} + func executeRequest(req *http.Request, mux http.Handler) *httptest.ResponseRecorder { rr := httptest.NewRecorder() mux.ServeHTTP(rr, req) diff --git a/internal/ratelimiter/fixed-window.go b/internal/ratelimiter/fixed-window.go index e5000f422..5c9cd3d4e 100644 --- a/internal/ratelimiter/fixed-window.go +++ b/internal/ratelimiter/fixed-window.go @@ -20,18 +20,18 @@ func NewFixedWindowLimiter(limit int, window time.Duration) *FixedWindowLimiter } } -func (rl *FixedWindowLimiter) Allow(ip string) (bool, time.Duration) { +func (rl *FixedWindowLimiter) Allow(key string) (bool, time.Duration) { rl.RLock() - count, exists := rl.clients[ip] + count, exists := rl.clients[key] rl.RUnlock() if !exists || count < rl.limit { rl.Lock() if !exists { - go rl.resetCount(ip) + go rl.resetCount(key) } - rl.clients[ip]++ + rl.clients[key]++ rl.Unlock() return true, 0 @@ -40,9 +40,9 @@ func (rl *FixedWindowLimiter) Allow(ip string) (bool, time.Duration) { return false, rl.window } -func (rl *FixedWindowLimiter) resetCount(ip string) { +func (rl *FixedWindowLimiter) resetCount(key string) { time.Sleep(rl.window) rl.Lock() - delete(rl.clients, ip) + delete(rl.clients, key) rl.Unlock() } diff --git a/internal/ratelimiter/ratelimiter.go b/internal/ratelimiter/ratelimiter.go index 35655e157..55fa0af57 100644 --- a/internal/ratelimiter/ratelimiter.go +++ b/internal/ratelimiter/ratelimiter.go @@ -3,11 +3,15 @@ package ratelimiter import "time" type Limiter interface { - Allow(ip string) (bool, time.Duration) + Allow(key string) (bool, time.Duration) } type Config struct { + // Budget for requests that carry a verified session, keyed by user ID. RequestPerTimeFrame int - TimeFrame time.Duration - Enabled bool + // Budget for requests without a verified session, keyed by client IP. + // Kept separate because many attendees share one IP at the venue. + IPRequestPerTimeFrame int + TimeFrame time.Duration + Enabled bool }