mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix: rate limit by user instead of IP for authenticated requests (#22049)
## Problem Rate limiting by user is broken (#20857). The rate limit middleware runs before API key extraction, so user ID is never in the request context. This causes: - Rate limiting falls back to IP address for all requests - `X-Coder-Bypass-Ratelimit` header for Owners is ignored (can't verify role without identity) ## Solution Adds `PrecheckAPIKey`, a **root-level middleware** that fully validates the API key on every request (expiry, OIDC refresh, DB updates, role lookup) and stores the result in context. Added **once** at the root router — not duplicated per route group. ### Architecture ``` Request → Root middleware stack: → ExtractRealIP, Logger, ... → PrecheckAPIKey(...) ← validates key, stores result, never rejects → HandleSubdomain(apiRateLimiter) ← workspace apps now also benefit → CORS, CSRF → /api/v2 or /api/experimental: → apiRateLimiter ← reads prechecked result from context → route handlers: → ExtractAPIKeyMW ← reuses prechecked data, adds route-specific logic → handler ``` ### Key design decisions | Decision | Rationale | |---|---| | **Full validation, not lightweight** | Spike's review: "the whole idea of a 'lightweight' extraction that skips security checks is fundamentally flawed." Only fully validated keys are used for rate limiting — expired/invalid keys fall back to IP. | | **Structured error results** | `ValidateAPIKeyError` has a `Hard` flag that maps to `write` vs `optionalWrite`. Hard errors (5xx, OAuth refresh failures) surface even on optional-auth routes. Soft errors (missing/expired token) are swallowed on optional routes. | | **Added once at the root** | Spike's review: "Why can't we add it once at the root?" Root placement means workspace app rate limiters also benefit. | | **Skip prechecked when `SessionTokenFunc != nil`** | `workspaceapps/db.go` uses a custom `SessionTokenFunc` that extracts from `issueReq.SessionToken`. The prechecked result may have validated a different token. Falls back to `ValidateAPIKey` with the custom func. | | **User status check stays in `ExtractAPIKey`** | Dormant activation is route-specific — `ValidateAPIKey` stores status but doesn't enforce it. | | **Audience validation stays in `ExtractAPIKey`** | Depends on `cfg.AccessURL` and request path, uses `optionalWrite(403)` which depends on route config. | ### Changes - **`coderd/httpmw/apikey.go`**: - New `ValidateAPIKey` function — extracted core validation logic, returns structured errors instead of writing HTTP responses - New `PrecheckAPIKey` middleware — calls `ValidateAPIKey`, stores result in `apiKeyPrecheckedContextKey`, never rejects - New types: `ValidateAPIKeyConfig`, `ValidateAPIKeyResult`, `ValidateAPIKeyError`, `APIKeyPrechecked` - Refactored `ExtractAPIKey` — consumes prechecked result from context (skipping redundant validation), falls back to `ValidateAPIKey` when no precheck available - Removed `ExtractAPIKeyForRateLimit` and `preExtractedAPIKey` - **`coderd/httpmw/ratelimit.go`**: Rate limiter checks `apiKeyPrecheckedContextKey` first, then `apiKeyContextKey` fallback (for unit tests / workspace apps), then IP - **`coderd/coderd.go`**: Added `PrecheckAPIKey` once at root `r.Use(...)` block, removed `ExtractAPIKeyForRateLimit` from `/api/v2` and `/api/experimental` - **`coderd/coderd_test.go`**: `TestRateLimitByUser` regression test with `BypassOwner` subtest Fixes #20857
This commit is contained in:
@@ -416,3 +416,91 @@ func TestDERPMetrics(t *testing.T) {
|
||||
assert.Contains(t, names, "coder_derp_server_packets_dropped_reason_total",
|
||||
"expected coder_derp_server_packets_dropped_reason_total to be registered")
|
||||
}
|
||||
|
||||
// TestRateLimitByUser verifies that rate limiting keys by user ID when
|
||||
// an authenticated session is present, rather than falling back to IP.
|
||||
// This is a regression test for https://github.com/coder/coder/issues/20857
|
||||
func TestRateLimitByUser(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
const rateLimit = 5
|
||||
|
||||
ownerClient := coderdtest.New(t, &coderdtest.Options{
|
||||
APIRateLimit: rateLimit,
|
||||
})
|
||||
firstUser := coderdtest.CreateFirstUser(t, ownerClient)
|
||||
|
||||
t.Run("HitsLimit", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
ctx := testutil.Context(t, testutil.WaitLong)
|
||||
|
||||
// Make rateLimit requests — they should all succeed.
|
||||
for i := 0; i < rateLimit; i++ {
|
||||
req, err := http.NewRequestWithContext(ctx, http.MethodGet,
|
||||
ownerClient.URL.String()+"/api/v2/buildinfo", nil)
|
||||
require.NoError(t, err)
|
||||
req.Header.Set(codersdk.SessionTokenHeader, ownerClient.SessionToken())
|
||||
|
||||
resp, err := ownerClient.HTTPClient.Do(req)
|
||||
require.NoError(t, err)
|
||||
resp.Body.Close()
|
||||
require.Equal(t, http.StatusOK, resp.StatusCode,
|
||||
"request %d should succeed", i+1)
|
||||
}
|
||||
|
||||
// The next request should be rate-limited.
|
||||
req, err := http.NewRequestWithContext(ctx, http.MethodGet,
|
||||
ownerClient.URL.String()+"/api/v2/buildinfo", nil)
|
||||
require.NoError(t, err)
|
||||
req.Header.Set(codersdk.SessionTokenHeader, ownerClient.SessionToken())
|
||||
|
||||
resp, err := ownerClient.HTTPClient.Do(req)
|
||||
require.NoError(t, err)
|
||||
resp.Body.Close()
|
||||
require.Equal(t, http.StatusTooManyRequests, resp.StatusCode,
|
||||
"request should be rate limited")
|
||||
})
|
||||
|
||||
t.Run("BypassOwner", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
ctx := testutil.Context(t, testutil.WaitLong)
|
||||
|
||||
// Owner with bypass header should not be rate-limited.
|
||||
for i := 0; i < rateLimit+5; i++ {
|
||||
req, err := http.NewRequestWithContext(ctx, http.MethodGet,
|
||||
ownerClient.URL.String()+"/api/v2/buildinfo", nil)
|
||||
require.NoError(t, err)
|
||||
req.Header.Set(codersdk.SessionTokenHeader, ownerClient.SessionToken())
|
||||
req.Header.Set(codersdk.BypassRatelimitHeader, "true")
|
||||
|
||||
resp, err := ownerClient.HTTPClient.Do(req)
|
||||
require.NoError(t, err)
|
||||
resp.Body.Close()
|
||||
require.Equal(t, http.StatusOK, resp.StatusCode,
|
||||
"owner bypass request %d should succeed", i+1)
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("MemberCannotBypass", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
memberClient, _ := coderdtest.CreateAnotherUser(t, ownerClient, firstUser.OrganizationID)
|
||||
ctx := testutil.Context(t, testutil.WaitLong)
|
||||
|
||||
// A member requesting the bypass header should be rejected
|
||||
// with 428 Precondition Required — only owners may bypass.
|
||||
req, err := http.NewRequestWithContext(ctx, http.MethodGet,
|
||||
memberClient.URL.String()+"/api/v2/buildinfo", nil)
|
||||
require.NoError(t, err)
|
||||
req.Header.Set(codersdk.SessionTokenHeader, memberClient.SessionToken())
|
||||
req.Header.Set(codersdk.BypassRatelimitHeader, "true")
|
||||
|
||||
resp, err := memberClient.HTTPClient.Do(req)
|
||||
require.NoError(t, err)
|
||||
resp.Body.Close()
|
||||
require.Equal(t, http.StatusPreconditionRequired, resp.StatusCode,
|
||||
"member should not be able to bypass rate limit")
|
||||
})
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user