Skip to content

feat(security): make auth-flow user scope independently enforceable (#506) #522

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ericfitz merged 1 commit into from
Jul 4, 2026
+121 −50
Merged

feat(security): make auth-flow user scope independently enforceable (#506) #522

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 64 additions & 32 deletions api/auth_flow_rate_limiter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()

Expand All @@ -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
Expand All @@ -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
Expand Down
50 changes: 41 additions & 9 deletions api/rate_limit_middleware_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -661,17 +661,17 @@ 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()
router.ServeHTTP(w, req)
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()
Expand Down Expand Up @@ -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)
})
}

Expand Down Expand Up @@ -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),
Expand All @@ -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",
Expand All @@ -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()
Expand Down
25 changes: 16 additions & 9 deletions test/integration/workflows/auth_flow_rate_limit_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
Loading