From 5db4900bc9960df2016e4ccfd2c1bec7de0b9f20 Mon Sep 17 00:00:00 2001 From: Eric Fitzgerald Date: Sat, 4 Jul 2026 14:12:14 -0400 Subject: [PATCH] feat(security): make auth-flow user scope independently enforceable (#506) The multi-scope auth-flow rate limiter enforced session, IP, and user scopes at the same 100/60s limit and checked them session -> IP -> user. Because single-IP traffic accumulates the IP counter at least as fast as the user counter, the IP scope (checked first, equal limit) always tripped first and the user scope could never be the attributed blocker for real single-IP traffic -- it was effectively subsumed by the IP scope. Adopt options 1 + 2 from the issue: - Differentiate the limits: per-user limit is now 50/60s, half the per-IP limit (100/60s), so the user scope bites independently for single-account attacks (password-spray / account enumeration against one victim) from a single source IP. Lowering the user limit (rather than raising the IP limit) is strictly more restrictive and cannot lock out legitimate traffic: an interactive OAuth flow is a handful of requests, a client-credentials client with a 1h token needs ~1 request/hour, and ResetUserRateLimit clears the counter on a successful login. - Reorder to most-specific-first (session -> user -> IP) so the user scope is correctly attributed when it bites. Extracts the three limits into named constants and adds a regression test proving a single account from a single IP is blocked by the user scope at 50, not subsumed by the IP scope at 100. Unit and integration tests updated to the new per-user limit. Closes #506 Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Kk9GxWS9EpazjbwBKfMpUX --- api/auth_flow_rate_limiter.go | 96 ++++++++++++------- api/rate_limit_middleware_test.go | 50 ++++++++-- .../workflows/auth_flow_rate_limit_test.go | 25 +++-- 3 files changed, 121 insertions(+), 50 deletions(-) diff --git a/api/auth_flow_rate_limiter.go b/api/auth_flow_rate_limiter.go index 9acf3bf5..24a1b677 100644 --- a/api/auth_flow_rate_limiter.go +++ b/api/auth_flow_rate_limiter.go @@ -12,6 +12,33 @@ import ( const buildModeTest = "test" +// Auth-flow rate limit values (requests per 60s sliding window). +const ( + // authFlowSessionLimit bounds requests sharing one session identifier + // (OAuth state / SAML RelayState / token code). + authFlowSessionLimit = 100 + + // authFlowUserLimit bounds requests keyed on a single user identifier + // (login_hint, or client_id for the client-credentials grant). It is + // deliberately LOWER than the per-IP limit so the user scope is + // independently enforceable (issue #506): for single-account attacks such + // as password-spray or account enumeration against one victim from a single + // source IP, the user counter now trips before the shared-IP counter, which + // at an equal limit always tripped first and subsumed the user scope. 50/60s + // is far above any legitimate single-account cadence — an interactive OAuth + // flow is a handful of requests, and a client-credentials client with a + // 1-hour token needs roughly one token request per hour — and + // ResetUserRateLimit clears this counter on a successful login, so + // legitimate users are never locked out. + authFlowUserLimit = 50 + + // authFlowDefaultIPLimit bounds requests from a single source IP across the + // auth-flow and token endpoints. Higher than authFlowUserLimit so that + // shared egress IPs (corporate NAT) can carry many distinct users before the + // coarse IP scope engages. + authFlowDefaultIPLimit = 100 +) + // envForceAuthFlowRateLimiting, when set to "true", forces the auth-flow rate // limiter to enforce limits even in build_mode=test. It exists so the // integration suite can exercise the multi-scope limiter against a server that @@ -60,22 +87,26 @@ func (r *AuthFlowRateLimiter) ResetUserRateLimit(ctx context.Context, userIdenti } } -// CheckRateLimit checks all three scopes and returns the most restrictive result -// Scopes: session (100/min), IP (100/min), user identifier (100/min) +// CheckRateLimit checks all three scopes and returns the most restrictive result. +// Scopes are evaluated most-specific-first: session (100/min), user identifier +// (50/min), IP (100/min). // SEM@2ba330fcb59eb085d8f877fe8f75f90af9b69071: check all three auth-flow rate limit scopes and return the most restrictive result (reads DB) func (r *AuthFlowRateLimiter) CheckRateLimit(ctx context.Context, sessionID string, ipAddress string, userIdentifier string) (*RateLimitResult, error) { - return r.checkRateLimitWithIPLimit(ctx, sessionID, ipAddress, userIdentifier, 100) + return r.checkRateLimitWithIPLimit(ctx, sessionID, ipAddress, userIdentifier, authFlowDefaultIPLimit) } // CheckRateLimitForTokenEndpoint checks rate limits for the token endpoint -// Uses the same 100 requests/minute per IP limit as other auth endpoints +// Uses the same per-IP limit as other auth endpoints // SEM@c70d49ed2d6089c24d05f8bc287ba5711c73abde: check rate limits for the token endpoint using the standard per-IP limit (reads DB) func (r *AuthFlowRateLimiter) CheckRateLimitForTokenEndpoint(ctx context.Context, sessionID string, ipAddress string, userIdentifier string) (*RateLimitResult, error) { - return r.checkRateLimitWithIPLimit(ctx, sessionID, ipAddress, userIdentifier, 100) + return r.checkRateLimitWithIPLimit(ctx, sessionID, ipAddress, userIdentifier, authFlowDefaultIPLimit) } -// checkRateLimitWithIPLimit implements multi-scope rate limiting with a configurable IP limit -// SEM@c70d49ed2d6089c24d05f8bc287ba5711c73abde: check session, IP, and user rate limit scopes with a configurable IP limit (reads DB) +// checkRateLimitWithIPLimit implements multi-scope rate limiting with a configurable IP limit. +// Scopes are checked most-specific-first (session -> user -> IP) so that the +// most narrowly-scoped counter is the one attributed when several would trip, +// and so the lower per-user limit engages before the shared per-IP limit. +// SEM@40c0e38339277e6a54d03dc01d30025bc0ef663d: check session, user, then IP rate limit scopes with a configurable IP limit (reads DB) func (r *AuthFlowRateLimiter) checkRateLimitWithIPLimit(ctx context.Context, sessionID string, ipAddress string, userIdentifier string, ipLimit int) (*RateLimitResult, error) { logger := slogging.Get() @@ -96,63 +127,64 @@ func (r *AuthFlowRateLimiter) checkRateLimitWithIPLimit(ctx context.Context, ses return &RateLimitResult{Allowed: true}, nil } - // Check session scope (100 requests/minute) + // Check session scope (most specific: one OAuth state / SAML relay / code). if sessionID != "" { sessionKey := fmt.Sprintf("auth:ratelimit:session:60s:%s", sessionID) - allowed, retryAfter, err := r.CheckSlidingWindow(ctx, sessionKey, 100, 60) + allowed, retryAfter, err := r.CheckSlidingWindow(ctx, sessionKey, authFlowSessionLimit, 60) if err != nil { logger.Error("failed to check session rate limit: %v", err) return nil, fmt.Errorf("session rate limit check failed: %w", err) } if !allowed { - remaining, resetAt, _ := r.GetRateLimitInfo(ctx, sessionKey, 100, 60) + remaining, resetAt, _ := r.GetRateLimitInfo(ctx, sessionKey, authFlowSessionLimit, 60) return &RateLimitResult{ Allowed: false, BlockedByScope: "session", RetryAfter: retryAfter, - Limit: 100, + Limit: authFlowSessionLimit, Remaining: remaining, ResetAt: resetAt, }, nil } } - // Check IP scope (configurable requests/minute) - if ipAddress != "" { - ipKey := fmt.Sprintf("auth:ratelimit:ip:60s:%s", ipAddress) - allowed, retryAfter, err := r.CheckSlidingWindow(ctx, ipKey, ipLimit, 60) + // Check user identifier scope before IP so the lower per-user limit engages + // independently for single-account attacks from a shared source IP. + if userIdentifier != "" { + userKey := fmt.Sprintf("auth:ratelimit:user:60s:%s", userIdentifier) + allowed, retryAfter, err := r.CheckSlidingWindow(ctx, userKey, authFlowUserLimit, 60) if err != nil { - logger.Error("failed to check IP rate limit: %v", err) - return nil, fmt.Errorf("IP rate limit check failed: %w", err) + logger.Error("failed to check user identifier rate limit: %v", err) + return nil, fmt.Errorf("user identifier rate limit check failed: %w", err) } if !allowed { - remaining, resetAt, _ := r.GetRateLimitInfo(ctx, ipKey, ipLimit, 60) + remaining, resetAt, _ := r.GetRateLimitInfo(ctx, userKey, authFlowUserLimit, 60) return &RateLimitResult{ Allowed: false, - BlockedByScope: "ip", + BlockedByScope: "user", RetryAfter: retryAfter, - Limit: ipLimit, + Limit: authFlowUserLimit, Remaining: remaining, ResetAt: resetAt, }, nil } } - // Check user identifier scope (100 attempts/minute) - if userIdentifier != "" { - userKey := fmt.Sprintf("auth:ratelimit:user:60s:%s", userIdentifier) - allowed, retryAfter, err := r.CheckSlidingWindow(ctx, userKey, 100, 60) + // Check IP scope last (coarsest: a shared egress IP may carry many users). + if ipAddress != "" { + ipKey := fmt.Sprintf("auth:ratelimit:ip:60s:%s", ipAddress) + allowed, retryAfter, err := r.CheckSlidingWindow(ctx, ipKey, ipLimit, 60) if err != nil { - logger.Error("failed to check user identifier rate limit: %v", err) - return nil, fmt.Errorf("user identifier rate limit check failed: %w", err) + logger.Error("failed to check IP rate limit: %v", err) + return nil, fmt.Errorf("IP rate limit check failed: %w", err) } if !allowed { - remaining, resetAt, _ := r.GetRateLimitInfo(ctx, userKey, 100, 60) + remaining, resetAt, _ := r.GetRateLimitInfo(ctx, ipKey, ipLimit, 60) return &RateLimitResult{ Allowed: false, - BlockedByScope: "user", + BlockedByScope: "ip", RetryAfter: retryAfter, - Limit: 100, + Limit: ipLimit, Remaining: remaining, ResetAt: resetAt, }, nil @@ -164,15 +196,15 @@ func (r *AuthFlowRateLimiter) checkRateLimitWithIPLimit(ctx context.Context, ses var resetAt int64 if sessionID != "" { sessionKey := fmt.Sprintf("auth:ratelimit:session:60s:%s", sessionID) - remaining, resetAt, _ = r.GetRateLimitInfo(ctx, sessionKey, 100, 60) + remaining, resetAt, _ = r.GetRateLimitInfo(ctx, sessionKey, authFlowSessionLimit, 60) } else { - remaining = 100 + remaining = authFlowSessionLimit resetAt = time.Now().Unix() + 60 } return &RateLimitResult{ Allowed: true, - Limit: 100, + Limit: authFlowSessionLimit, Remaining: remaining, ResetAt: resetAt, }, nil diff --git a/api/rate_limit_middleware_test.go b/api/rate_limit_middleware_test.go index d75c5d64..6e44db2d 100644 --- a/api/rate_limit_middleware_test.go +++ b/api/rate_limit_middleware_test.go @@ -661,9 +661,9 @@ func TestAuthFlowRateLimitMiddleware(t *testing.T) { loginHint := "testuser@example.com" - // Make 100 requests to exhaust user limit (100/min) + // Make authFlowUserLimit requests to exhaust the user limit (50/min) // Use different state AND different IP each time to avoid session/IP limits - for i := range 100 { + for i := range authFlowUserLimit { req := httptest.NewRequest("GET", "/oauth2/authorize?state="+uuid.New().String()+"&login_hint="+loginHint, nil) req.RemoteAddr = fmt.Sprintf("10.%d.%d.%d:12345", i/65536%256, i/256%256, i%256+1) w := httptest.NewRecorder() @@ -671,7 +671,7 @@ func TestAuthFlowRateLimitMiddleware(t *testing.T) { assert.Equal(t, http.StatusOK, w.Code, "Request %d should be allowed", i+1) } - // 101st request should be blocked by user scope + // The next request should be blocked by user scope req := httptest.NewRequest("GET", "/oauth2/authorize?state="+uuid.New().String()+"&login_hint="+loginHint, nil) req.RemoteAddr = "10.99.99.99:12345" w := httptest.NewRecorder() @@ -1067,18 +1067,19 @@ func TestAuthFlowRateLimiter(t *testing.T) { limiter := NewAuthFlowRateLimiter(client) userIdentifier := "testuser@example.com" - // Make 100 requests to exhaust user limit (use different sessions and IPs) - for i := range 100 { + // Make authFlowUserLimit requests to exhaust the user limit (use different sessions and IPs) + for i := range authFlowUserLimit { result, err := limiter.CheckRateLimit(context.Background(), uuid.New().String(), uuid.New().String(), userIdentifier) require.NoError(t, err) assert.True(t, result.Allowed, "Request %d should be allowed", i+1) } - // 101st request should be blocked by user scope + // The next request should be blocked by user scope result, err := limiter.CheckRateLimit(context.Background(), uuid.New().String(), uuid.New().String(), userIdentifier) require.NoError(t, err) assert.False(t, result.Allowed) assert.Equal(t, "user", result.BlockedByScope) + assert.Equal(t, authFlowUserLimit, result.Limit) }) } @@ -1250,8 +1251,10 @@ func TestAuthFlowRateLimiterMultiScopeScenarios(t *testing.T) { limiter := NewAuthFlowRateLimiter(client) targetUser := "admin@example.com" - // Attacker probes one username 100 times from different IPs/sessions - for i := range 100 { + // Attacker probes one username authFlowUserLimit times from different IPs/sessions. + // The per-user limit (50) is lower than the per-IP limit (100), so the user + // scope bites independently even though each request also uses a fresh IP. + for i := range authFlowUserLimit { result, err := limiter.CheckRateLimit(context.Background(), uuid.New().String(), fmt.Sprintf("10.0.%d.%d", i/256, i%256), @@ -1260,7 +1263,7 @@ func TestAuthFlowRateLimiterMultiScopeScenarios(t *testing.T) { assert.True(t, result.Allowed, "Request %d should be allowed", i+1) } - // 101st attempt blocked by user scope + // The next attempt is blocked by user scope result, err := limiter.CheckRateLimit(context.Background(), uuid.New().String(), "172.16.0.1", @@ -1270,6 +1273,35 @@ func TestAuthFlowRateLimiterMultiScopeScenarios(t *testing.T) { assert.Equal(t, "user", result.BlockedByScope) }) + t.Run("single-account attack from one IP is blocked by user scope, not subsumed by IP (issue #506)", func(t *testing.T) { + client, mr := setupTestRedis(t) + defer mr.Close() + defer func() { _ = client.Close() }() + + limiter := NewAuthFlowRateLimiter(client) + targetUser := "victim@example.com" + attackerIP := "203.0.113.77" + + // A real single-IP attacker password-spraying one account: same user and + // same IP on every request (unique session per request so the session + // scope stays out of it). Because the per-user limit (50) is lower than + // the per-IP limit (100), the user scope must trip first — before the IP + // counter reaches its limit. Prior to #506 the equal limits and the + // session->IP->user order let the IP scope always trip first, so the user + // scope could never be the attributed blocker for single-IP traffic. + for i := range authFlowUserLimit { + result, err := limiter.CheckRateLimit(context.Background(), uuid.New().String(), attackerIP, targetUser) + require.NoError(t, err) + assert.True(t, result.Allowed, "Request %d should be allowed", i+1) + } + + result, err := limiter.CheckRateLimit(context.Background(), uuid.New().String(), attackerIP, targetUser) + require.NoError(t, err) + assert.False(t, result.Allowed) + assert.Equal(t, "user", result.BlockedByScope, "user scope must bite before the shared-IP scope for single-account attacks") + assert.Equal(t, authFlowUserLimit, result.Limit) + }) + t.Run("session replay: same session blocked at 100 regardless of other scopes", func(t *testing.T) { client, mr := setupTestRedis(t) defer mr.Close() diff --git a/test/integration/workflows/auth_flow_rate_limit_test.go b/test/integration/workflows/auth_flow_rate_limit_test.go index 513a0abe..232e40f2 100644 --- a/test/integration/workflows/auth_flow_rate_limit_test.go +++ b/test/integration/workflows/auth_flow_rate_limit_test.go @@ -8,12 +8,18 @@ import ( "github.com/ericfitz/tmi/test/integration/framework" ) -// authFlowLimit is the per-scope auth-flow rate limit (requests / 60s) enforced -// by api/auth_flow_rate_limiter.go for the session, IP, and user scopes. The -// token endpoint uses the same per-IP limit. Keep this in sync with the -// implementation (checkRateLimitWithIPLimit). +// authFlowLimit is the per-session and per-IP auth-flow rate limit (requests / +// 60s) enforced by api/auth_flow_rate_limiter.go. The token endpoint uses the +// same per-IP limit. Keep this in sync with the implementation +// (authFlowSessionLimit / authFlowDefaultIPLimit in checkRateLimitWithIPLimit). const authFlowLimit = 100 +// authFlowUserLimit is the per-user-identifier auth-flow rate limit (requests / +// 60s). It is deliberately lower than authFlowLimit so the user scope is +// independently enforceable for single-account attacks (issue #506). Keep this +// in sync with authFlowUserLimit in api/auth_flow_rate_limiter.go. +const authFlowUserLimit = 50 + // uniqueAuthFlowIP returns a distinct non-loopback test IP for index i. The // integration server runs with no trusted proxies, so it honors X-Forwarded-For // (extractIPAddress's manual path) — letting a test vary the perceived client IP @@ -148,19 +154,20 @@ func TestAuthFlowRateLimiting_MultiScope(t *testing.T) { framework.AssertNoError(t, err, "Failed to create client") // Same login_hint, but a unique state AND a unique IP per request so - // neither the session nor the IP scope accumulates — only the user scope. + // neither the session nor the IP scope accumulates — only the user scope, + // which enforces the lower authFlowUserLimit (issue #506). loginHint := "ratelimit-user-" + framework.UniqueUserID() - for i := 0; i < authFlowLimit; i++ { + for i := 0; i < authFlowUserLimit; i++ { state := fmt.Sprintf("user-state-%s-%d", framework.UniqueUserID(), i) resp := authFlowAuthorize(t, client, state, loginHint, uniqueAuthFlowIP(i)) if resp.StatusCode == 429 { - t.Fatalf("Request %d was rate limited before the limit of %d", i+1, authFlowLimit) + t.Fatalf("Request %d was rate limited before the limit of %d", i+1, authFlowUserLimit) } } - resp := authFlowAuthorize(t, client, "user-final-"+framework.UniqueUserID(), loginHint, uniqueAuthFlowIP(authFlowLimit)) + resp := authFlowAuthorize(t, client, "user-final-"+framework.UniqueUserID(), loginHint, uniqueAuthFlowIP(authFlowUserLimit)) if resp.StatusCode != 429 { - t.Errorf("Expected 429 on request %d with the same login_hint, got %d", authFlowLimit+1, resp.StatusCode) + t.Errorf("Expected 429 on request %d with the same login_hint, got %d", authFlowUserLimit+1, resp.StatusCode) } if scope := resp.Headers.Get("X-RateLimit-Scope"); scope != "user" { t.Errorf("Expected X-RateLimit-Scope=user, got %q", scope)