From 090f73210cf572b52720d98e9d799c600b42c261 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Tue, 20 Oct 2026 01:20:47 +0100 Subject: [PATCH] sec(api): cap forgot-password requests per client IP POST /api/auth/forgot-password only had the per-email bucket, so one client could walk any number of addresses, one request each, with no total volume cap. Add a forgot_password_ip bucket (10 per 15 minutes per IP, the same budget as reset_password) checked before the body is parsed. It keys on the request's SourceIP like every other per-IP bucket (rightmost, proxy-appended X-Forwarded-For entry on the HTTP server) and fails open like the per-email bucket (02-M1). The comment/config mismatch from the issue was already reconciled to 10 per 5 minutes in #1038; the handler comment now names both buckets. Closes #13 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler.go | 2 + internal/api/handler_auth.go | 13 +- .../handler_forgot_password_ratelimit_test.go | 180 ++++++++++++++++++ internal/api/rate_limiter.go | 5 + .../app_rate_limiter_integration_test.go | 38 +++- 5 files changed, 236 insertions(+), 2 deletions(-) create mode 100644 internal/api/handler_forgot_password_ratelimit_test.go diff --git a/internal/api/handler.go b/internal/api/handler.go index 5deb120ef..a5a1df9e6 100644 --- a/internal/api/handler.go +++ b/internal/api/handler.go @@ -227,6 +227,8 @@ func NewHandler(cfg HandlerConfig) *Handler { // not under production load. assertRateLimitKeysKnown( "login", + "forgot_password", + "forgot_password_ip", "setup_admin", "reset_password", "change_password", diff --git a/internal/api/handler_auth.go b/internal/api/handler_auth.go index 045786c11..14ad4859e 100644 --- a/internal/api/handler_auth.go +++ b/internal/api/handler_auth.go @@ -332,12 +332,23 @@ func (h *Handler) forgotPassword(ctx context.Context, req *events.LambdaFunction return nil, fmt.Errorf("authentication service not configured") } + // Per-IP cap: 10 attempts per IP per 15 minutes, checked before the body + // is parsed so malformed requests spend the budget too. Without it one + // client could walk any number of addresses, one request each, past the + // per-email bucket below (#13). The client IP is the request's SourceIP, + // the same key every other per-IP bucket uses. Fail-open like the + // per-email bucket (02-M1); the 429 depends only on this caller's own + // request count, never on whether an account exists. + if err := h.checkRateLimit(ctx, req, "forgot_password_ip"); err != nil { + return nil, err + } + var pwdReq PasswordResetRequest if err := json.Unmarshal([]byte(req.Body), &pwdReq); err != nil { return nil, NewClientError(400, "invalid request body") } - // Rate limiting: 10 attempts per email per 5 minutes to prevent enumeration attacks. + // Per-email cap: 10 attempts per email per 5 minutes to prevent enumeration attacks. // On rate-limiter error: emit a high-severity alert (02-M1) but continue with the // request. Failing closed here would reveal infrastructure state (503 vs 200) to // an enumerating attacker and harm legitimate users more than the enumeration risk. diff --git a/internal/api/handler_forgot_password_ratelimit_test.go b/internal/api/handler_forgot_password_ratelimit_test.go new file mode 100644 index 000000000..638a7dd8c --- /dev/null +++ b/internal/api/handler_forgot_password_ratelimit_test.go @@ -0,0 +1,180 @@ +package api + +import ( + "context" + "errors" + "fmt" + "sync" + "testing" + + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +const forgotPasswordOK = "if the email exists, a reset link has been sent" + +func forgotReqFrom(ip, body string) *events.LambdaFunctionURLRequest { + req := &events.LambdaFunctionURLRequest{Body: body} + req.RequestContext.HTTP.SourceIP = ip + return req +} + +func forgotBody(email string) string { + return fmt.Sprintf(`{"email":%q}`, email) +} + +func requireForgotOK(t *testing.T, result any, err error, msgAndArgs ...any) { + t.Helper() + require.NoError(t, err, msgAndArgs...) + resp, ok := result.(map[string]string) + require.True(t, ok, "unexpected result type %T", result) + assert.Equal(t, forgotPasswordOK, resp["status"], msgAndArgs...) +} + +// The per-IP and per-email limits on forgot-password are separate buckets. +// Pinning both keeps the handler comment and the config in step (#13). +func TestGetDefaultRateLimits_ForgotPasswordBuckets(t *testing.T) { + limits := getDefaultRateLimits() + assert.Equal(t, NewRateLimitConfig(10, 5*60), limits["forgot_password"], "per-email bucket") + assert.Equal(t, NewRateLimitConfig(10, 15*60), limits["forgot_password_ip"], "per-IP bucket") + assert.NotPanics(t, func() { assertRateLimitKeysKnown("forgot_password", "forgot_password_ip") }) +} + +// Issue #13: one client walking many addresses (one request per address) used +// to be allowed without limit because only the per-email bucket existed. +func TestHandler_forgotPassword_PerIPCapAcrossDistinctEmails(t *testing.T) { + limit := getDefaultRateLimits()["forgot_password_ip"].MaxAttempts + require.Less(t, limit, 100) + + mockAuth := new(MockAuthService) + mockAuth.On("RequestPasswordReset", mock.Anything, mock.Anything).Return(nil) + h := &Handler{auth: mockAuth, rateLimiter: NewInMemoryRateLimiter()} + ctx := context.Background() + + for i := 0; i <= limit; i++ { + result, err := h.forgotPassword(ctx, forgotReqFrom("203.0.113.7", forgotBody(fmt.Sprintf("probe%d@example.com", i)))) + if i < limit { + requireForgotOK(t, result, err, "attempt %d", i+1) + continue + } + assert.Nil(t, result) + requireClientCode(t, err, 429, "attempt %d must be throttled", i+1) + } + mockAuth.AssertNumberOfCalls(t, "RequestPasswordReset", limit) + + // A different client keeps its own budget. + result, err := h.forgotPassword(ctx, forgotReqFrom("203.0.113.8", forgotBody("someone@example.com"))) + requireForgotOK(t, result, err) + mockAuth.AssertNumberOfCalls(t, "RequestPasswordReset", limit+1) +} + +// The bucket key is the request's SourceIP, which the HTTP server derives from +// the rightmost (proxy-appended) X-Forwarded-For entry. A forwarded-for header +// that reaches the handler must not open a fresh bucket. +func TestHandler_forgotPassword_PerIPCapIgnoresForwardedHeaders(t *testing.T) { + limit := getDefaultRateLimits()["forgot_password_ip"].MaxAttempts + + mockAuth := new(MockAuthService) + mockAuth.On("RequestPasswordReset", mock.Anything, mock.Anything).Return(nil) + h := &Handler{auth: mockAuth, rateLimiter: NewInMemoryRateLimiter()} + + for i := 0; i <= limit; i++ { + req := forgotReqFrom("203.0.113.9", forgotBody(fmt.Sprintf("spoof%d@example.com", i))) + req.Headers = map[string]string{ + "x-forwarded-for": fmt.Sprintf("198.51.100.%d", i+1), + "x-real-ip": fmt.Sprintf("198.51.100.%d", i+1), + } + result, err := h.forgotPassword(context.Background(), req) + if i < limit { + requireForgotOK(t, result, err, "attempt %d", i+1) + } else { + requireClientCode(t, err, 429, "attempt %d must be throttled", i+1) + } + } +} + +// The per-IP check runs before the body is parsed, so a flood of malformed +// bodies spends the same budget as well-formed ones. +func TestHandler_forgotPassword_MalformedBodiesSpendIPBudget(t *testing.T) { + limit := getDefaultRateLimits()["forgot_password_ip"].MaxAttempts + + mockAuth := new(MockAuthService) + h := &Handler{auth: mockAuth, rateLimiter: NewInMemoryRateLimiter()} + + for i := 0; i < limit; i++ { + _, err := h.forgotPassword(context.Background(), forgotReqFrom("203.0.113.10", "{not json")) + requireClientCode(t, err, 400, "attempt %d", i+1) + } + _, err := h.forgotPassword(context.Background(), forgotReqFrom("203.0.113.10", forgotBody("user@example.com"))) + requireClientCode(t, err, 429) + mockAuth.AssertNotCalled(t, "RequestPasswordReset", mock.Anything, mock.Anything) +} + +// A request refused by the per-IP bucket must not spend the target address's +// per-email budget or reach the reset service. +func TestHandler_forgotPassword_PerIPDeniedSkipsEmailBucket(t *testing.T) { + mockAuth := new(MockAuthService) + rl := new(MockRateLimiter) + t.Cleanup(func() { rl.AssertExpectations(t) }) + rl.On("AllowWithIP", mock.Anything, "203.0.113.11", "forgot_password_ip").Return(false, nil).Once() + h := &Handler{auth: mockAuth, rateLimiter: rl} + + result, err := h.forgotPassword(context.Background(), forgotReqFrom("203.0.113.11", forgotBody("victim@example.com"))) + assert.Nil(t, result) + requireClientCode(t, err, 429) + rl.AssertNotCalled(t, "AllowWithEmail", mock.Anything, mock.Anything, mock.Anything) + mockAuth.AssertNotCalled(t, "RequestPasswordReset", mock.Anything, mock.Anything) +} + +// Like the per-email bucket (02-M1), the per-IP bucket fails open on a limiter +// error so the endpoint's response does not expose limiter health. +func TestHandler_forgotPassword_PerIPLimiterErrorFailsOpen(t *testing.T) { + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + mockAuth.On("RequestPasswordReset", mock.Anything, "user@example.com").Return(nil).Once() + rl := new(MockRateLimiter) + t.Cleanup(func() { rl.AssertExpectations(t) }) + rl.On("AllowWithIP", mock.Anything, "203.0.113.12", "forgot_password_ip"). + Return(false, errors.New("pool exhausted")).Once() + rl.On("AllowWithEmail", mock.Anything, "user@example.com", "forgot_password").Return(true, nil).Once() + h := &Handler{auth: mockAuth, rateLimiter: rl} + + result, err := h.forgotPassword(context.Background(), forgotReqFrom("203.0.113.12", forgotBody("user@example.com"))) + requireForgotOK(t, result, err) +} + +// Concurrent requests from one client admit exactly the bucket size. +func TestHandler_forgotPassword_PerIPCapConcurrent(t *testing.T) { + limit := getDefaultRateLimits()["forgot_password_ip"].MaxAttempts + const callers = 40 + + mockAuth := new(MockAuthService) + mockAuth.On("RequestPasswordReset", mock.Anything, mock.Anything).Return(nil) + h := &Handler{auth: mockAuth, rateLimiter: NewInMemoryRateLimiter()} + + var wg sync.WaitGroup + var mu sync.Mutex + codes := map[int]int{} + for i := range callers { + wg.Add(1) + go func() { + defer wg.Done() + _, err := h.forgotPassword(context.Background(), forgotReqFrom("203.0.113.13", forgotBody(fmt.Sprintf("c%d@example.com", i)))) + code := 200 + if ce, ok := IsClientError(err); ok { + code = ce.code + } else if err != nil { + code = -1 + } + mu.Lock() + codes[code]++ + mu.Unlock() + }() + } + wg.Wait() + + assert.Equal(t, map[int]int{200: limit, 429: callers - limit}, codes) + mockAuth.AssertNumberOfCalls(t, "RequestPasswordReset", limit) +} diff --git a/internal/api/rate_limiter.go b/internal/api/rate_limiter.go index e4f723ddd..fc57b4501 100644 --- a/internal/api/rate_limiter.go +++ b/internal/api/rate_limiter.go @@ -36,6 +36,11 @@ func getDefaultRateLimits() map[string]RateLimitConfig { "api_general": NewRateLimitConfig(300, 60), // 300 requests / minute / IP "admin": NewRateLimitConfig(30, 60), // 30 / minute / user "approve_cancel_public": NewRateLimitConfig(30, 60), // 30 / minute / IP for public approve/cancel/reject + // forgot_password_ip caps one client's total volume across addresses: + // the per-email forgot_password bucket lets a caller walk many + // addresses, one request each (#13). Same budget as reset_password, + // the next step of the same flow. + "forgot_password_ip": NewRateLimitConfig(10, 15*60), // 10 attempts / 15 minutes / IP (#13) // setup_admin should fire at most once per deployment. Issue #411 specifies // 5 attempts per 15 minutes so a cold-start burst cannot brute-force the // first-run endpoint before the DB-backed limiter is available. diff --git a/internal/server/app_rate_limiter_integration_test.go b/internal/server/app_rate_limiter_integration_test.go index 89eeedf5d..a08eb69a6 100644 --- a/internal/server/app_rate_limiter_integration_test.go +++ b/internal/server/app_rate_limiter_integration_test.go @@ -67,7 +67,8 @@ func TestApplicationRateLimiter(t *testing.T) { ip := func(bucket int) string { return fmt.Sprintf("192.0.2.%d", mode*10+bucket) } request := func(replica int, path, sourceIP, body string) (int, error) { method := http.MethodGet - if path == "/api/auth/login" || strings.HasPrefix(path, "/api/purchases/") { + if path == "/api/auth/login" || path == "/api/auth/forgot-password" || + strings.HasPrefix(path, "/api/purchases/") { method = http.MethodPost } req, reqErr := http.NewRequestWithContext(ctx, method, servers[replica].URL+path, strings.NewReader(body)) @@ -130,10 +131,45 @@ func TestApplicationRateLimiter(t *testing.T) { } checkCount(ip(3), "approve_cancel_public", 32) + // Forgot-password per-IP cap (#13): distinct addresses from one + // client share one bucket across replicas, and a client-supplied + // X-Forwarded-For entry left of the proxy-appended one opens no + // fresh bucket. + forgot := func(replica, i int, xff string) (int, error) { + return request(replica, "/api/auth/forgot-password", xff, + fmt.Sprintf(`{"email":"probe%d@example.test"}`, i)) + } + for i := range 12 { + want := http.StatusOK + if i >= 10 { + want = http.StatusTooManyRequests + } + status, reqErr := forgot(i%2, i, fmt.Sprintf("198.51.100.%d, %s", i+1, ip(7))) + require.NoError(t, reqErr) + assert.Equal(t, want, status, "forgot-password attempt %d", i+1) + } + checkCount(ip(7), "forgot_password_ip", 12) + type outcome struct { status int err error } + forgotResults := make(chan outcome, 15) + for i := range 15 { + go func() { + status, reqErr := forgot(i%2, 100+i, ip(8)) + forgotResults <- outcome{status, reqErr} + }() + } + forgotStatuses := make(map[int]int) + for range 15 { + result := <-forgotResults + require.NoError(t, result.err) + forgotStatuses[result.status]++ + } + assert.Equal(t, map[int]int{http.StatusOK: 10, http.StatusTooManyRequests: 5}, forgotStatuses) + checkCount(ip(8), "forgot_password_ip", 15) + results := make(chan outcome, 12) for i := range 12 { go func() {