Repository navigation
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 6 7 +1
Lines 142 168 +26
=========================================
+ Hits 142 168 +26 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds HTTP middleware functionality to the rate limiting library, enabling easy integration with net/http applications. The middleware provides rate limiting based on client IP addresses or custom request attributes like API keys.
Key changes:
- Added
RateLimitermiddleware that wraps HTTP handlers with rate limiting logic - Implemented
IPKeyFuncandHeaderKeyFuncfor extracting rate limit keys from requests - Comprehensive test suite covering various scenarios including concurrent requests
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| middleware/http.go | New middleware implementation with KeyFunc interface, IP and header-based key extraction functions, and RateLimiter wrapper |
| middleware/http_test.go | Comprehensive test suite covering key extraction functions, rate limiting behavior, independent keys, custom key functions, and concurrent request handling |
| README.md | Updated documentation to describe HTTP middleware feature, including usage examples and key extractor reference table |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // IPKeyFunc extracts the client IP address from the request. | ||
| // It checks X-Forwarded-For and X-Real-IP headers before falling back to RemoteAddr. | ||
| func IPKeyFunc(r *http.Request) registry.Identifier { | ||
| // Check X-Forwarded-For first (may contain multiple IPs) | ||
| if xff := r.Header.Get("X-Forwarded-For"); xff != "" { | ||
| // Take the first IP (original client) | ||
| for i := range len(xff) { | ||
| if xff[i] == ',' { | ||
| return registry.Identifier(xff[:i]) | ||
| } | ||
| } | ||
|
|
||
| return registry.Identifier(xff) | ||
| } | ||
|
|
||
| // Check X-Real-IP | ||
| if xri := r.Header.Get("X-Real-IP"); xri != "" { | ||
| return registry.Identifier(xri) | ||
| } | ||
|
|
||
| // Fall back to RemoteAddr | ||
| host, _, err := net.SplitHostPort(r.RemoteAddr) | ||
| if err != nil { |
There was a problem hiding this comment.
The IPKeyFunc blindly trusts X-Forwarded-For and X-Real-IP headers without validation. These headers can be easily spoofed by clients, allowing them to bypass rate limits by rotating fake IP addresses in these headers.
This is a critical security issue because:
- Any client can set X-Forwarded-For to arbitrary values
- Attackers can bypass rate limits by sending requests with different X-Forwarded-For headers
- Multiple attackers could share the same fake IP to pool their rate limits
Consider either:
- Adding documentation warning that IPKeyFunc should only be used behind a trusted proxy that sets these headers
- Providing a separate TrustedProxyIPKeyFunc that validates the request comes from a trusted proxy
- Making RemoteAddr the default and providing opt-in header extraction
| // IPKeyFunc extracts the client IP address from the request. | |
| // It checks X-Forwarded-For and X-Real-IP headers before falling back to RemoteAddr. | |
| func IPKeyFunc(r *http.Request) registry.Identifier { | |
| // Check X-Forwarded-For first (may contain multiple IPs) | |
| if xff := r.Header.Get("X-Forwarded-For"); xff != "" { | |
| // Take the first IP (original client) | |
| for i := range len(xff) { | |
| if xff[i] == ',' { | |
| return registry.Identifier(xff[:i]) | |
| } | |
| } | |
| return registry.Identifier(xff) | |
| } | |
| // Check X-Real-IP | |
| if xri := r.Header.Get("X-Real-IP"); xri != "" { | |
| return registry.Identifier(xri) | |
| } | |
| // Fall back to RemoteAddr | |
| host, _, err := net.SplitHostPort(r.RemoteAddr) | |
| if err != nil { | |
| // IPKeyFunc extracts the client IP address from the request using RemoteAddr only. | |
| // This avoids trusting X-Forwarded-For or X-Real-IP headers, which can be spoofed by clients. | |
| // If you are running behind a trusted proxy, provide a custom KeyFunc that safely | |
| // interprets forwarded headers according to your deployment. | |
| func IPKeyFunc(r *http.Request) registry.Identifier { | |
| host, _, err := net.SplitHostPort(r.RemoteAddr) | |
| if err != nil { | |
| // If RemoteAddr is not in "host:port" form, fall back to the raw value. |
| for i := range len(xff) { | ||
| if xff[i] == ',' { | ||
| return registry.Identifier(xff[:i]) | ||
| } |
There was a problem hiding this comment.
The X-Forwarded-For header parsing doesn't handle whitespace correctly. The standard format is "IP1, IP2, IP3" with a space after each comma, but this code will include the leading space in the extracted IP address (e.g., " 10.0.0.2" instead of "10.0.0.2").
This will cause:
- Different rate limit buckets for the same IP (" 10.0.0.1" vs "10.0.0.1")
- Invalid IP addresses being stored as keys
- Potential issues if the IP is later validated or used elsewhere
The fix should trim whitespace from the extracted IP, for example using strings.TrimSpace() on the result.
| key := keyFunc(r) | ||
|
|
||
| if !reg.Allow(key) { | ||
| w.Header().Set("Retry-After", "1") |
There was a problem hiding this comment.
The hardcoded "Retry-After: 1" value doesn't accurately reflect the actual rate limit configuration. Different rate limiters in this library have different refill rates and capacities, but all rate-limited responses will suggest retrying after 1 second regardless of the actual limiter configuration.
For example:
- A limiter with rate=0 (no refill) will never allow more requests, but suggests retry after 1 second
- A limiter with rate=60 (1/second) should suggest retrying sooner
- A limiter with rate=1 (slower refill) may need longer wait times
This can lead to poor client behavior where clients retry too soon or too late. Consider either making the Retry-After value configurable, calculating it based on the limiter's rate, or documenting this limitation.
| { | ||
| name: "uses first IP from X-Forwarded-For chain", | ||
| remoteAddr: "192.168.1.1:12345", | ||
| headers: map[string]string{"X-Forwarded-For": "10.0.0.1, 10.0.0.2, 10.0.0.3"}, | ||
| want: "10.0.0.1", |
There was a problem hiding this comment.
The test case verifies that the first IP is extracted from a comma-separated X-Forwarded-For list, but it doesn't test the actual standard format which includes spaces after commas (e.g., "10.0.0.1, 10.0.0.2, 10.0.0.3").
The current implementation has a bug where it doesn't trim whitespace, so a properly formatted X-Forwarded-For header would result in IPs with leading spaces. Add a test case without spaces after commas to verify the current behavior, and add test cases with spaces to expose the whitespace handling bug.
| // Useful for API key or token-based rate limiting. | ||
| func HeaderKeyFunc(header string) KeyFunc { | ||
| return func(r *http.Request) registry.Identifier { | ||
| return registry.Identifier(r.Header.Get(header)) |
There was a problem hiding this comment.
When the specified header is missing, HeaderKeyFunc returns an empty string identifier. This means all requests without the header will share the same rate limit bucket, which is likely not the intended behavior.
For example, if rate limiting by API key and a client doesn't provide the key, they'll share a bucket with all other clients who also don't provide a key. This creates a DoS vulnerability where one client without a key can exhaust the rate limit for all other clients without keys.
Consider either:
- Returning a sentinel value or unique identifier for missing headers
- Having the middleware reject requests with empty keys (return 400 Bad Request)
- Documenting this behavior clearly so users understand the implications
| return registry.Identifier(r.Header.Get(header)) | |
| value := r.Header.Get(header) | |
| if value == "" { | |
| // Use a non-empty sentinel identifier when the header is missing to avoid | |
| // grouping all such requests under an empty key. | |
| return registry.Identifier("missing:" + header) | |
| } | |
| return registry.Identifier(value) |
| |-----------------------|--------------------------------------------------------------------| | ||
| | `IPKeyFunc` | Extracts client IP (checks X-Forwarded-For, X-Real-IP, RemoteAddr) | | ||
| | `HeaderKeyFunc(name)` | Extracts value from specified header | | ||
|
|
There was a problem hiding this comment.
The documentation for IPKeyFunc states it "checks X-Forwarded-For, X-Real-IP, RemoteAddr" but doesn't warn users about the critical security implications of trusting these headers. X-Forwarded-For and X-Real-IP can be spoofed by clients, allowing rate limit bypass.
The README should include a security note explaining:
- IPKeyFunc trusts proxy headers and should only be used behind a trusted reverse proxy
- When to use RemoteAddr-only extraction for direct client connections
- The risks of header spoofing if used incorrectly
This is important because users may deploy this middleware on edge servers directly exposed to the internet, where header spoofing is trivial.
| > **Security note for `IPKeyFunc`:** | |
| > | |
| > - `IPKeyFunc` trusts the `X-Forwarded-For` and `X-Real-IP` headers and should only be used when your application is **behind a trusted reverse proxy or load balancer** that reliably sets or overwrites these headers. | |
| > - If your server is directly exposed to the internet (no trusted proxy in front), prefer an IP extractor that uses only `RemoteAddr`, so that client-controlled headers cannot influence the rate-limit key. | |
| > - Misconfiguring this (for example, using `IPKeyFunc` on an edge server that accepts arbitrary `X-Forwarded-For`/`X-Real-IP` values from clients) allows attackers to spoof their IP address and bypass IP-based rate limits. |
No description provided.