Skip to content

Commit bc2a870

Browse files
tadasantclaude
andcommitted
Simplify rate limiting: always trust proxy headers
Since the registry is always deployed behind NGINX ingress with use-forwarded-headers=true, there's no need for a TrustProxy config option. The NGINX ingress is trusted infrastructure that correctly sets X-Forwarded-For headers. Removed: - TrustProxy config option and environment variable - Tests for TrustProxy enabled/disabled behavior The implementation now always checks X-Forwarded-For/X-Real-IP headers first, then falls back to RemoteAddr. This matches the deployment setup. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent 4914d98 commit bc2a870

File tree

5 files changed

+23
-115
lines changed

5 files changed

+23
-115
lines changed

.env.example

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,3 @@ MCP_REGISTRY_RATE_LIMIT_ENABLED=true
4646
MCP_REGISTRY_RATE_LIMIT_REQUESTS_PER_MINUTE=60
4747
# Maximum requests per hour per IP address (default: 1000)
4848
MCP_REGISTRY_RATE_LIMIT_REQUESTS_PER_HOUR=1000
49-
# Trust X-Forwarded-For and X-Real-IP headers from reverse proxy (default: false)
50-
# Only enable this when running behind a trusted reverse proxy (nginx, cloud load balancer)
51-
# WARNING: Setting this to true without a trusted proxy allows IP spoofing
52-
MCP_REGISTRY_RATE_LIMIT_TRUST_PROXY=false

internal/api/ratelimit/ratelimit.go

Lines changed: 19 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,6 @@ type Config struct {
2323
CleanupInterval time.Duration
2424
// SkipPaths are paths that should not be rate limited
2525
SkipPaths []string
26-
// TrustProxy determines if X-Forwarded-For and X-Real-IP headers should be trusted.
27-
// Set to true only when running behind a trusted reverse proxy (e.g., nginx, cloud load balancer).
28-
// When false, only the direct connection IP (RemoteAddr) is used, preventing IP spoofing.
29-
TrustProxy bool
3026
// MaxVisitors is the maximum number of visitor entries to track (memory protection).
3127
// When exceeded, oldest entries are evicted. Default: 100000.
3228
MaxVisitors int
@@ -39,7 +35,6 @@ func DefaultConfig() Config {
3935
RequestsPerHour: 1000,
4036
CleanupInterval: 10 * time.Minute,
4137
SkipPaths: []string{"/health", "/ping", "/metrics"},
42-
TrustProxy: false, // Secure default: don't trust proxy headers
4338
MaxVisitors: 100000,
4439
}
4540
}
@@ -198,32 +193,29 @@ func (rl *RateLimiter) shouldSkip(path string) bool {
198193
}
199194

200195
// getClientIP extracts the client IP from the request.
201-
// When TrustProxy is true, it considers X-Forwarded-For and X-Real-IP headers.
202-
// When TrustProxy is false, only RemoteAddr is used to prevent IP spoofing.
203-
func (rl *RateLimiter) getClientIP(r *http.Request) string {
204-
// Only trust proxy headers if explicitly configured
205-
if rl.config.TrustProxy {
206-
// Check X-Forwarded-For header (can contain multiple IPs)
207-
if xff := r.Header.Get("X-Forwarded-For"); xff != "" {
208-
// Take the first IP (original client)
209-
if idx := strings.Index(xff, ","); idx != -1 {
210-
xff = xff[:idx]
211-
}
212-
xff = strings.TrimSpace(xff)
213-
if ip := validateAndNormalizeIP(xff); ip != "" {
214-
return ip
215-
}
196+
// It considers X-Forwarded-For and X-Real-IP headers for reverse proxy scenarios,
197+
// as the registry is deployed behind NGINX ingress with use-forwarded-headers enabled.
198+
func getClientIP(r *http.Request) string {
199+
// Check X-Forwarded-For header (can contain multiple IPs)
200+
if xff := r.Header.Get("X-Forwarded-For"); xff != "" {
201+
// Take the first IP (original client)
202+
if idx := strings.Index(xff, ","); idx != -1 {
203+
xff = xff[:idx]
204+
}
205+
xff = strings.TrimSpace(xff)
206+
if ip := validateAndNormalizeIP(xff); ip != "" {
207+
return ip
216208
}
209+
}
217210

218-
// Check X-Real-IP header
219-
if xri := r.Header.Get("X-Real-IP"); xri != "" {
220-
if ip := validateAndNormalizeIP(strings.TrimSpace(xri)); ip != "" {
221-
return ip
222-
}
211+
// Check X-Real-IP header
212+
if xri := r.Header.Get("X-Real-IP"); xri != "" {
213+
if ip := validateAndNormalizeIP(strings.TrimSpace(xri)); ip != "" {
214+
return ip
223215
}
224216
}
225217

226-
// Fall back to RemoteAddr (always used when TrustProxy is false)
218+
// Fall back to RemoteAddr
227219
ip, _, err := net.SplitHostPort(r.RemoteAddr)
228220
if err != nil {
229221
// RemoteAddr might not have a port
@@ -265,7 +257,7 @@ func (rl *RateLimiter) Middleware(next http.Handler) http.Handler {
265257
return
266258
}
267259

268-
ip := rl.getClientIP(r)
260+
ip := getClientIP(r)
269261

270262
if !rl.Allow(ip) {
271263
w.Header().Set("Content-Type", "application/problem+json")

internal/api/ratelimit/ratelimit_test.go

Lines changed: 4 additions & 82 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ func TestRateLimiter_Allow(t *testing.T) {
2121
RequestsPerHour: 10,
2222
CleanupInterval: time.Hour, // Long interval to avoid cleanup during test
2323
SkipPaths: []string{"/health"},
24-
TrustProxy: false,
2524
MaxVisitors: 1000,
2625
}
2726
rl := ratelimit.New(cfg)
@@ -54,7 +53,6 @@ func TestRateLimiter_HourlyLimit(t *testing.T) {
5453
RequestsPerHour: 5,
5554
CleanupInterval: time.Hour,
5655
SkipPaths: []string{"/health"},
57-
TrustProxy: false,
5856
MaxVisitors: 1000,
5957
}
6058
rl := ratelimit.New(cfg)
@@ -81,7 +79,6 @@ func TestRateLimiter_Middleware(t *testing.T) {
8179
RequestsPerHour: 100,
8280
CleanupInterval: time.Hour,
8381
SkipPaths: []string{"/health", "/ping"},
84-
TrustProxy: false,
8582
MaxVisitors: 1000,
8683
}
8784
rl := ratelimit.New(cfg)
@@ -165,13 +162,12 @@ func TestRateLimiter_Middleware(t *testing.T) {
165162
})
166163
}
167164

168-
func TestGetClientIP_TrustProxyEnabled(t *testing.T) {
165+
func TestGetClientIP_WithHeaders(t *testing.T) {
169166
cfg := ratelimit.Config{
170167
RequestsPerMinute: 1,
171168
RequestsPerHour: 100,
172169
CleanupInterval: time.Hour,
173170
SkipPaths: []string{},
174-
TrustProxy: true, // Trust proxy headers
175171
MaxVisitors: 1000,
176172
}
177173
rl := ratelimit.New(cfg)
@@ -183,7 +179,7 @@ func TestGetClientIP_TrustProxyEnabled(t *testing.T) {
183179

184180
middleware := rl.Middleware(handler)
185181

186-
t.Run("uses X-Forwarded-For header when TrustProxy is true", func(t *testing.T) {
182+
t.Run("uses X-Forwarded-For header", func(t *testing.T) {
187183
// First request from this forwarded IP
188184
req := httptest.NewRequest(http.MethodGet, "/test", nil)
189185
req.RemoteAddr = testLocalAddr
@@ -230,7 +226,7 @@ func TestGetClientIP_TrustProxyEnabled(t *testing.T) {
230226
}
231227
})
232228

233-
t.Run("uses X-Real-IP header when TrustProxy is true", func(t *testing.T) {
229+
t.Run("uses X-Real-IP header", func(t *testing.T) {
234230
req := httptest.NewRequest(http.MethodGet, "/test", nil)
235231
req.RemoteAddr = testLocalAddr
236232
req.Header.Set("X-Real-IP", "203.0.113.3")
@@ -252,75 +248,8 @@ func TestGetClientIP_TrustProxyEnabled(t *testing.T) {
252248
t.Errorf("second request should be blocked, got status %d", w2.Code)
253249
}
254250
})
255-
}
256-
257-
func TestGetClientIP_TrustProxyDisabled(t *testing.T) {
258-
cfg := ratelimit.Config{
259-
RequestsPerMinute: 1,
260-
RequestsPerHour: 100,
261-
CleanupInterval: time.Hour,
262-
SkipPaths: []string{},
263-
TrustProxy: false, // Don't trust proxy headers (secure default)
264-
MaxVisitors: 1000,
265-
}
266-
rl := ratelimit.New(cfg)
267-
defer rl.Stop()
268251

269-
handler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
270-
w.WriteHeader(http.StatusOK)
271-
})
272-
273-
middleware := rl.Middleware(handler)
274-
275-
t.Run("ignores X-Forwarded-For when TrustProxy is false", func(t *testing.T) {
276-
// First request - uses RemoteAddr despite X-Forwarded-For
277-
req := httptest.NewRequest(http.MethodGet, "/test", nil)
278-
req.RemoteAddr = "10.0.0.10:12345"
279-
req.Header.Set("X-Forwarded-For", "203.0.113.1")
280-
w := httptest.NewRecorder()
281-
middleware.ServeHTTP(w, req)
282-
283-
if w.Code != http.StatusOK {
284-
t.Errorf("first request should be allowed, got status %d", w.Code)
285-
}
286-
287-
// Second request from same RemoteAddr (different X-Forwarded-For) should be blocked
288-
req2 := httptest.NewRequest(http.MethodGet, "/test", nil)
289-
req2.RemoteAddr = "10.0.0.10:12346"
290-
req2.Header.Set("X-Forwarded-For", "203.0.113.99") // Different spoofed IP
291-
w2 := httptest.NewRecorder()
292-
middleware.ServeHTTP(w2, req2)
293-
294-
if w2.Code != http.StatusTooManyRequests {
295-
t.Errorf("second request should be blocked (spoofing prevented), got status %d", w2.Code)
296-
}
297-
})
298-
299-
t.Run("ignores X-Real-IP when TrustProxy is false", func(t *testing.T) {
300-
// First request
301-
req := httptest.NewRequest(http.MethodGet, "/test", nil)
302-
req.RemoteAddr = "10.0.0.11:12345"
303-
req.Header.Set("X-Real-IP", "203.0.113.5")
304-
w := httptest.NewRecorder()
305-
middleware.ServeHTTP(w, req)
306-
307-
if w.Code != http.StatusOK {
308-
t.Errorf("first request should be allowed, got status %d", w.Code)
309-
}
310-
311-
// Second request from same RemoteAddr should be blocked
312-
req2 := httptest.NewRequest(http.MethodGet, "/test", nil)
313-
req2.RemoteAddr = "10.0.0.11:12346"
314-
req2.Header.Set("X-Real-IP", "203.0.113.99") // Different spoofed IP
315-
w2 := httptest.NewRecorder()
316-
middleware.ServeHTTP(w2, req2)
317-
318-
if w2.Code != http.StatusTooManyRequests {
319-
t.Errorf("second request should be blocked, got status %d", w2.Code)
320-
}
321-
})
322-
323-
t.Run("uses RemoteAddr correctly", func(t *testing.T) {
252+
t.Run("falls back to RemoteAddr when no headers", func(t *testing.T) {
324253
req := httptest.NewRequest(http.MethodGet, "/test", nil)
325254
req.RemoteAddr = "203.0.113.4:12345"
326255
w := httptest.NewRecorder()
@@ -348,7 +277,6 @@ func TestGetClientIP_InvalidIPs(t *testing.T) {
348277
RequestsPerHour: 1000,
349278
CleanupInterval: time.Hour,
350279
SkipPaths: []string{},
351-
TrustProxy: true,
352280
MaxVisitors: 1000,
353281
}
354282
rl := ratelimit.New(cfg)
@@ -404,7 +332,6 @@ func TestRateLimiter_MaxVisitors(t *testing.T) {
404332
RequestsPerHour: 1000,
405333
CleanupInterval: time.Hour,
406334
SkipPaths: []string{},
407-
TrustProxy: false,
408335
MaxVisitors: 3, // Very low limit for testing
409336
}
410337
rl := ratelimit.New(cfg)
@@ -431,7 +358,6 @@ func TestRateLimiter_Concurrency(_ *testing.T) {
431358
RequestsPerHour: 10000,
432359
CleanupInterval: time.Hour,
433360
SkipPaths: []string{},
434-
TrustProxy: false,
435361
MaxVisitors: 100,
436362
}
437363
rl := ratelimit.New(cfg)
@@ -471,10 +397,6 @@ func TestDefaultConfig(t *testing.T) {
471397
t.Errorf("expected CleanupInterval to be 10 minutes, got %v", cfg.CleanupInterval)
472398
}
473399

474-
if cfg.TrustProxy != false {
475-
t.Error("expected TrustProxy to default to false for security")
476-
}
477-
478400
if cfg.MaxVisitors != 100000 {
479401
t.Errorf("expected MaxVisitors to be 100000, got %d", cfg.MaxVisitors)
480402
}

internal/api/server.go

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,6 @@ func NewServer(cfg *config.Config, registryService service.RegistryService, metr
8080
RequestsPerHour: cfg.RateLimitRequestsPerHour,
8181
CleanupInterval: 10 * time.Minute,
8282
SkipPaths: []string{"/health", "/ping", "/metrics"},
83-
TrustProxy: cfg.RateLimitTrustProxy,
8483
MaxVisitors: 100000,
8584
}
8685
rateLimiter = ratelimit.New(rateLimitConfig)

internal/config/config.go

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@ type Config struct {
2929
RateLimitEnabled bool `env:"RATE_LIMIT_ENABLED" envDefault:"true"`
3030
RateLimitRequestsPerMinute int `env:"RATE_LIMIT_REQUESTS_PER_MINUTE" envDefault:"60"`
3131
RateLimitRequestsPerHour int `env:"RATE_LIMIT_REQUESTS_PER_HOUR" envDefault:"1000"`
32-
RateLimitTrustProxy bool `env:"RATE_LIMIT_TRUST_PROXY" envDefault:"false"`
3332
}
3433

3534
// NewConfig creates a new configuration with default values

0 commit comments

Comments
 (0)