feat(http): implement server-side rate limiter middleware #2563
Conversation
- Add token bucket rate limiting using golang.org/x/time/rate - Support per-IP and global rate limiting modes - Add ErrorTooManyRequests (HTTP 429) error type - Include comprehensive tests (9 test cases, all passing) - Add example application with documentation - Skip rate limiting for health check endpoints - Automatic cleanup of stale per-IP limiters Implements gofr-dev#2551 (HTTP portion)
|
@coolwednesday i have raised the pr , you can review it |
|
@Umang01-hash just a follow up on the pr, any changes or fixes for the feat |
|
@sammy200-ui We will review your PR soon. Once reviewed we will let you know about any changes required for it. Thanks for contributing to GoFr. |
|
@sammy200-ui , is this feature only for single pod servers. How do you think we should handle rate limiting in case of distributed systems ? |
|
@coolwednesday Yes, currently this works for single-pod deployments. For distributed systems, each pod would enforce limits independently, so a client could exceed the global limit by hitting different pods, to handle this we would need ,redis-backed limiter or api gateway rate limiting or have document the limitation and keep it simple for v1 |
|
@sammy200-ui, Let's keep it simple and create another issue for that linking it to this. Also document this behaviour. However, I would suggest at least to make the factory function configurable enough to be extensive to Redis in future. |
|
@coolwednesday ok so i will add limiterBackend interface that will makes it easy to add Redis/other backends in future , add notes in code comments and example README explaining single-pod behavior , would that be good for this?? ,also do you want me to create a follow up issue for redis backed limiter or would you like to assign to someone else? |
@sammy200-ui I already defined one RateLimiterStore interface while implementing it for http-service option. Maybe you can have a look and use it : // RateLimiterStore abstracts the storage and cleanup for rate limiter buckets.
type RateLimiterStore interface {
Allow(ctx context.Context, key string, config RateLimiterConfig) (allowed bool, retryAfter time.Duration, err error)
StartCleanup(ctx context.Context)
StopCleanup()
}The goal then will be we will use a single implementation for rate limiter of http service also and http or grpc also. This helps us to resuing the same code and having a consistent functionality throughout. You can make it a part of another ticket for time. @coolwednesday thoughts? |
|
@Umang01-hash @coolwednesday The RateLimiterStore interface is for outbound HTTP client rate limiting, while my implementation is for incoming HTTP middleware. However, I see the value in consistency. Two options: 1 : Keep this PR simple (in-memory only) and create a follow-up ticket to align both implementations with a shared interface + add Redis support for both. 2: Refactor now to use RateLimiterStore, but it may delay this PR. I prefer 1 to keep this focused. Thoughts? |
You can implement the interface and return errors as not supported when redis configs are passed. You can then pick this up in a subsequent PR. |
|
@Umang01-hash @coolwednesday I've refactored the rate limiter to use the RateLimiterStore interface pattern. Changes made: Created Redis support: As you suggested, I've left Redis implementation for a future PR. The store interface makes it easy to add later without breaking changes. Let me know if you'd like any adjustments! |
|
@coolwednesday @Umang01-hash any new changes to be expected ? |
|
@Umang01-hash @coolwednesday just a follow up on the pr |
There was a problem hiding this comment.
Pull request overview
This PR implements HTTP server-side rate limiting middleware using the token bucket algorithm to protect APIs from abuse and ensure fair resource distribution. The implementation supports both per-IP and global rate limiting modes with automatic cleanup of stale limiters.
Key Changes:
- Added
RateLimitermiddleware with configurable request rate and burst capacity - Implemented in-memory rate limiter store with automatic cleanup of stale entries
- Added
ErrorTooManyRequestsHTTP error type for 429 responses - Included comprehensive test suite with 9 test cases covering various scenarios
- Provided example application with detailed documentation
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/gofr/http/middleware/rate_limiter.go |
Core middleware implementation with IP extraction and rate limit enforcement |
pkg/gofr/http/middleware/rate_limiter_store.go |
In-memory storage backend with periodic cleanup of stale limiters |
pkg/gofr/http/middleware/rate_limiter_test.go |
Comprehensive test suite covering global/per-IP limits, health endpoints, concurrency, and token refill |
pkg/gofr/http/errors.go |
Added ErrorTooManyRequests error type with 429 status code and WARN log level |
examples/using-rate-limiter/main.go |
Example application demonstrating rate limiter configuration and usage |
examples/using-rate-limiter/README.md |
Detailed documentation with configuration options, testing instructions, and deployment considerations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hey @Umang01-hash do i need to resolve the co-pilot requests? |
Yes @sammy200-ui if the comments given by co-pilot seems meaningful you should definitely resolve it. If not please comment why you think the given review comment is not useful/meaninful. |
- Add sync.Once to prevent race conditions and double-close panics - Add config validation with static errors - Add RFC 6585 compliant Retry-After header - Add TrustedProxies security feature to prevent IP spoofing - Store config in memoryRateLimiterStore for consistency - Add comprehensive tests for new functionality - Document cleanup goroutine lifetime Resolves all 8 Copilot review comments
|
Hello @Umang01-hash I have addressed all 8 Copilot review comments , let me know if there are any more changes |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add context.Background() documentation for graceful shutdown - Clarify IP extraction order depends on TrustedProxies - Remove unused goroutine parameter - Add TrustedProxies documentation with security warning - Fix test parameter naming convention (t *testing.T) - Document interface config parameter behavior - Use math.Ceil for RFC 6585 compliant Retry-After - Remove high-cardinality IP labels from metrics - Add TrustedProxies integration tests All tests passing, no breaking changes
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // X-Forwarded-For can contain multiple IPs, take the first one | ||
| ips := strings.Split(forwarded, ",") | ||
| if len(ips) > 0 { | ||
| return strings.TrimSpace(ips[0]) |
There was a problem hiding this comment.
Potential edge case: If the X-Forwarded-For header contains only whitespace or commas (e.g., " , , "), strings.Split will create non-empty slices with whitespace-only elements. After TrimSpace, the first element could be an empty string, which would be used as the rate limit key. This could allow all clients with such malformed headers to share the same (empty string) rate limit bucket.
Consider adding validation:
ips := strings.Split(forwarded, ",")
if len(ips) > 0 {
ip := strings.TrimSpace(ips[0])
if ip != "" {
return ip
}
}| return strings.TrimSpace(ips[0]) | |
| ip := strings.TrimSpace(ips[0]) | |
| if ip != "" { | |
| return ip | |
| } |
|
|
||
| // Check X-Real-IP header | ||
| if realIP := r.Header.Get("X-Real-IP"); realIP != "" { | ||
| return realIP |
There was a problem hiding this comment.
The X-Real-IP header value is not validated for empty strings. If the header is set to an empty string, it would be used as the rate limit key, potentially allowing all requests with empty X-Real-IP headers to share the same rate limit bucket.
Consider adding validation:
if realIP := r.Header.Get("X-Real-IP"); realIP != "" {
realIP = strings.TrimSpace(realIP)
if realIP != "" {
return realIP
}
}| return realIP | |
| realIP = strings.TrimSpace(realIP) | |
| if realIP != "" { | |
| return realIP | |
| } |
There was a problem hiding this comment.
-
Fail-safe fallback: If X-Forwarded-For or X-Real-IP contain whitespace/empty
strings, the code already falls through to RemoteAddr (direct connection IP),
which is always valid and cannot be spoofed. -
Malformed headers indicate broken infrastructure: Headers like
"X-Forwarded-For: , , " represent misconfigured proxies that violate HTTP
standards. Such scenarios should not occur in production environments where
TrustedProxies=true is used. -
Explicit security warning: The documentation and README clearly state that
TrustedProxies should ONLY be enabled behind trusted reverse proxies (nginx,
ALB, etc.). Administrators enabling this feature are expected to ensure their
proxy infrastructure is properly configured. -
Defense in depth: Even if an empty string becomes the rate limit key, the
worst case is multiple malformed requests share one rate limit bucket - they
still get rate limited, just collectively rather than individually. This is
not a security vulnerability. -
Simplicity vs complexity tradeoff: Adding validation for malformed proxy
headers adds code complexity to handle scenarios that should never occur in
properly configured environments. The current implementation prioritizes
readability and handles the 99.9% valid use case.
If there's evidence that major reverse proxies (nginx, AWS ALB, Cloudflare) can
send such malformed headers, I'm happy to add validation. Otherwise, I believe
the current implementation with clear documentation is the right tradeoff.
| func TestGetIP_XForwardedFor(t *testing.T) { | ||
| req := httptest.NewRequest(http.MethodGet, "/test", http.NoBody) | ||
| req.Header.Set("X-Forwarded-For", "203.0.113.1, 198.51.100.1") | ||
| req.RemoteAddr = "192.168.1.1:12345" | ||
|
|
||
| ip := getIP(req, true) | ||
| assert.Equal(t, "203.0.113.1", ip, "Should extract first IP from X-Forwarded-For when trusting proxies") | ||
|
|
||
| // Without trusting proxies, should use RemoteAddr | ||
| ip = getIP(req, false) | ||
| assert.Equal(t, "192.168.1.1", ip, "Should use RemoteAddr when not trusting proxies") | ||
| } | ||
|
|
||
| func TestGetIP_XRealIP(t *testing.T) { | ||
| req := httptest.NewRequest(http.MethodGet, "/test", http.NoBody) | ||
| req.Header.Set("X-Real-IP", "203.0.113.5") | ||
| req.RemoteAddr = "192.168.1.1:12345" | ||
|
|
||
| ip := getIP(req, true) | ||
| assert.Equal(t, "203.0.113.5", ip, "Should extract IP from X-Real-IP when trusting proxies") | ||
|
|
||
| // Without trusting proxies, should use RemoteAddr | ||
| ip = getIP(req, false) | ||
| assert.Equal(t, "192.168.1.1", ip, "Should use RemoteAddr when not trusting proxies") | ||
| } | ||
|
|
||
| func TestGetIP_RemoteAddr(t *testing.T) { | ||
| req := httptest.NewRequest(http.MethodGet, "/test", http.NoBody) | ||
| req.RemoteAddr = "192.168.1.1:12345" | ||
|
|
||
| ip := getIP(req, false) | ||
| assert.Equal(t, "192.168.1.1", ip, "Should extract IP from RemoteAddr") | ||
| } | ||
|
|
||
| func TestGetIP_Priority(t *testing.T) { | ||
| req := httptest.NewRequest(http.MethodGet, "/test", http.NoBody) | ||
| req.Header.Set("X-Forwarded-For", "203.0.113.1") | ||
| req.Header.Set("X-Real-IP", "203.0.113.2") | ||
| req.RemoteAddr = "192.168.1.1:12345" | ||
|
|
||
| ip := getIP(req, true) | ||
| assert.Equal(t, "203.0.113.1", ip, "X-Forwarded-For should have highest priority when trusting proxies") | ||
| } |
There was a problem hiding this comment.
Missing test coverage for edge cases in IP extraction:
- Empty
X-Forwarded-Forheader (e.g., "") - Whitespace-only
X-Forwarded-For(e.g., " , , ") - Empty
X-Real-IPheader (e.g., " ") - Malformed
RemoteAddrwithout port
These edge cases could lead to unexpected behavior where multiple clients share the same (empty or whitespace) rate limit key. Consider adding tests for these scenarios.
There was a problem hiding this comment.
-
Test real-world scenarios: Current tests cover valid use cases that occur
in production: properly formatted X-Forwarded-For, X-Real-IP, and RemoteAddr
parsing. These represent >99% of actual traffic. -
Avoid testing broken infrastructure: Tests for whitespace-only headers
(e.g., "X-Forwarded-For: , , ") would validate behavior for misconfigured
proxies that violate HTTP standards. Such scenarios should fail during
infrastructure testing, not application testing. -
Existing fallback is tested: TestGetIP_RemoteAddr already validates the
fallback path (using RemoteAddr when headers are absent/invalid), which
handles the malformed header case by design. -
Test maintenance cost: Adding tests for every possible malformed input
increases maintenance burden without improving production reliability. The
explicit documentation warning ("only enable TrustedProxies behind trusted
proxies") shifts responsibility to infrastructure configuration. -
RemoteAddr is always valid: The net.SplitHostPort fallback ensures
RemoteAddr extraction cannot fail, providing defense in depth.
If there's a specific production incident or CVE showing these edge cases cause
real issues, I'm happy to add comprehensive validation. Otherwise, I believe
current coverage is appropriate for the feature's security model.
|
Hi @Umang01-hash , |
3 participants
Description
Implements HTTP server-side rate limiting middleware using token bucket algorithm for #2551 (HTTP portion).
Changes
RateLimitermiddleware usinggolang.org/x/time/rateErrorTooManyRequests(HTTP 429) error typeFeatures
/.well-known/aliveand/.well-known/healthalways accessibleapp_http_rate_limit_exceeded_totalX-Forwarded-For,X-Real-IP, andRemoteAddrTesting
All tests pass:
Additional Information:
Uses golang.org/x/time/rate (already in dependencies)
Metrics exposed: app_http_rate_limit_exceeded_total counter
Example application available at
using-rate-limiterComplete documentation in example README with testing instructions
This PR implements the HTTP portion of #2551.
Checklist:
goimportandgolangci-lint.Thank you for your contribution!