fix: use IP-based rate limiting to prevent infinite bucket attacks[ISSUE #100]#126
fix: use IP-based rate limiting to prevent infinite bucket attacks[ISSUE #100]#126Yashaswini-V21 wants to merge 2 commits intoAnkanMisra:mainfrom
Conversation
- Changed getRateLimitKey to always use IP address instead of nonce - Nonce-based keying created new buckets for every request (nonces must be unique) - This fixes infinite rate limits and memory leaks for authenticated users - Updated TestGetRateLimitKey to expect IP-based keys in all scenarios Fixes AnkanMisra#100
📝 WalkthroughWalkthroughThe rate-limiting strategy was changed to always key off client IP. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Gateway as Gateway
participant RL as RateLimiter
Client->>Gateway: HTTP request (any headers, nonce varies)
Gateway->>Gateway: getRateLimitKey() -> "ip:" + ClientIP
Gateway->>RL: check/consume bucket for "ip:<client-ip>"
RL-->>Gateway: Allow / Deny (429)
Gateway-->>Client: Response (200 or 429)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // getRateLimitKey determines the key for rate limiting (always uses IP) | ||
| func getRateLimitKey(c *gin.Context) string { | ||
| signature := c.GetHeader("X-402-Signature") | ||
| nonce := c.GetHeader("X-402-Nonce") | ||
|
|
||
| // Only use nonce-based key if BOTH signature and nonce are present | ||
| // This prevents attackers from bypassing IP rate limits with fake nonces | ||
| if signature != "" && nonce != "" { | ||
| hash := sha256.Sum256([]byte(nonce)) | ||
| // Use 32 hex chars (128 bits) for better collision resistance | ||
| return "nonce:" + hex.EncodeToString(hash[:])[:32] | ||
| } | ||
|
|
||
| // REMOVED: Nonce-based keying | ||
| // Nonces must be unique per request (replay attack prevention), | ||
| // which creates infinite buckets and memory leaks. | ||
| // ALWAYS use IP for now to prevent infinite-bucket attacks | ||
| return "ip:" + c.ClientIP() | ||
| } |
There was a problem hiding this comment.
This creates a critical bug: all authenticated users sharing the same IP now share the same rate limit bucket.
Users behind corporate NAT, VPNs, or proxies will unfairly share rate limits. For example, if 10 employees at a company all authenticate with different wallets, they'd collectively hit the 60 RPM limit instead of each getting 60 RPM.
The original nonce-based approach failed because nonces are unique per request. However, you have access to recovered_address from signature verification (gateway/main.go:434, gateway/main.go:51). Consider using wallet address as the rate limit key for authenticated requests:
- Anonymous:
"ip:" + c.ClientIP() - Authenticated:
"wallet:" + recoveredAddress
This requires either:
- Moving signature verification before rate limiting (adds latency)
- Using a lightweight address recovery in the rate limiter
- Accepting IP-based limits only for anonymous tier, and using wallet-based for authenticated tiers
Prompt To Fix With AI
This is a comment left during a code review.
Path: gateway/main.go
Line: 727-734
Comment:
This creates a critical bug: **all authenticated users sharing the same IP now share the same rate limit bucket**.
Users behind corporate NAT, VPNs, or proxies will unfairly share rate limits. For example, if 10 employees at a company all authenticate with different wallets, they'd collectively hit the 60 RPM limit instead of each getting 60 RPM.
The original nonce-based approach failed because nonces are unique per request. However, you have access to `recovered_address` from signature verification (gateway/main.go:434, gateway/main.go:51). Consider using wallet address as the rate limit key for authenticated requests:
- Anonymous: `"ip:" + c.ClientIP()`
- Authenticated: `"wallet:" + recoveredAddress`
This requires either:
1. Moving signature verification before rate limiting (adds latency)
2. Using a lightweight address recovery in the rate limiter
3. Accepting IP-based limits only for anonymous tier, and using wallet-based for authenticated tiers
How can I resolve this? If you propose a fix, please make it concise.| // Nonces must be unique per request (replay attack prevention), | ||
| // which creates infinite buckets and memory leaks. | ||
| // ALWAYS use IP for now to prevent infinite-bucket attacks | ||
| return "ip:" + c.ClientIP() |
There was a problem hiding this comment.
Gin's ClientIP() without trusted proxy configuration may return incorrect IPs behind load balancers/proxies.
In main() (line 205), there's no call to gin.Engine.SetTrustedProxies(). If this gateway runs behind a reverse proxy (nginx, AWS ALB, etc.), ClientIP() will return the proxy's IP, not the user's real IP, causing all users to share one rate limit bucket.
Check if running behind a proxy and configure trusted proxies if needed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: gateway/main.go
Line: 733
Comment:
Gin's `ClientIP()` without trusted proxy configuration may return incorrect IPs behind load balancers/proxies.
In `main()` (line 205), there's no call to `gin.Engine.SetTrustedProxies()`. If this gateway runs behind a reverse proxy (nginx, AWS ALB, etc.), `ClientIP()` will return the proxy's IP, not the user's real IP, causing all users to share one rate limit bucket.
Check if running behind a proxy and configure trusted proxies if needed.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
This test still passes because both simulated users come from the same test client IP in the test environment, making the test accidentally valid but for the wrong reason. The test no longer validates what it claims to validate. Either update the test name/comments to reflect IP-based bucketing, or update the test to actually simulate different IPs using Prompt To Fix With AIThis is a comment left during a code review.
Path: gateway/main_test.go
Line: 157-208
Comment:
Test name and comment claim to verify "different users have separate rate limit buckets" (line 158), but after this PR, all authenticated users from the same IP share the same bucket.
This test still passes because both simulated users come from the same test client IP in the test environment, making the test accidentally valid but for the wrong reason. The test no longer validates what it claims to validate.
Either update the test name/comments to reflect IP-based bucketing, or update the test to actually simulate different IPs using `req.Header.Set("X-Forwarded-For", ...)` if trusted proxies are configured.
How can I resolve this? If you propose a fix, please make it concise. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical rate limiting vulnerability (issue #100) where nonce-based keying created infinite rate limit buckets, effectively bypassing rate limits and causing memory leaks. The fix changes the rate limiting strategy to use IP-based keys exclusively, preventing the infinite bucket attack.
Changes:
- Replaced nonce-based rate limiting keys with IP-based keys in
getRateLimitKey() - Updated test to validate IP-based behavior across all scenarios (with/without auth headers)
- Removed nonce hashing logic to simplify the implementation
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| gateway/main.go | Modified getRateLimitKey() to always return IP-based keys, removing previous nonce-based logic that created unlimited buckets |
| gateway/main_test.go | Updated TestGetRateLimitKey to expect IP-based keys in all test cases and simplified validation logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Nonces must be unique per request (replay attack prevention), | ||
| // which creates infinite buckets and memory leaks. | ||
| // ALWAYS use IP for now to prevent infinite-bucket attacks | ||
| return "ip:" + c.ClientIP() |
There was a problem hiding this comment.
The use of c.ClientIP() without configuring trusted proxies in Gin makes this vulnerable to IP spoofing via X-Forwarded-For headers. An attacker can bypass rate limits by sending different X-Forwarded-For values with each request.
Gin's ClientIP() method returns the first untrusted IP from X-Forwarded-For by default when no trusted proxies are configured. This means any client can set arbitrary X-Forwarded-For headers to create new rate limit buckets.
To fix this:
- Configure trusted proxies using
router.SetTrustedProxies()if deployed behind a reverse proxy/load balancer - OR use
c.Request.RemoteAddrdirectly to get the actual TCP connection IP - OR explicitly disable proxy headers with
router.SetTrustedProxies(nil)
The application appears to be deployed directly (see docker-compose.yml port 3000:3000) without a reverse proxy, so using c.Request.RemoteAddr or disabling proxy headers is recommended.
| // REMOVED: Nonce-based keying | ||
| // Nonces must be unique per request (replay attack prevention), | ||
| // which creates infinite buckets and memory leaks. | ||
| // ALWAYS use IP for now to prevent infinite-bucket attacks | ||
| return "ip:" + c.ClientIP() |
There was a problem hiding this comment.
While IP-based rate limiting resolves the infinite bucket attack, it introduces operational concerns for legitimate users behind shared IPs (corporate NAT, carrier-grade NAT, VPNs, etc.). All users behind the same NAT will share a single rate limit bucket.
Consider documenting this limitation in the code comments or README, especially:
- Users on mobile networks may share IPs with many others
- Corporate users will be rate limited collectively
- The rate limits (10 RPM anonymous, 60 RPM standard) may be too restrictive for shared IPs
Potential improvements for future consideration:
- Hybrid approach: Use wallet address (from signature recovery) + IP for authenticated requests
- Implement a "burst allowance" multiplier for authenticated users to compensate for shared IP scenarios
- Add monitoring/logging to detect when single IPs are hitting limits (could indicate shared IP vs actual attack)
| // REMOVED: Nonce-based keying | ||
| // Nonces must be unique per request (replay attack prevention), | ||
| // which creates infinite buckets and memory leaks. | ||
| // ALWAYS use IP for now to prevent infinite-bucket attacks |
There was a problem hiding this comment.
The comment "ALWAYS use IP for now" suggests this is a temporary fix. Consider clarifying whether this is intended to be permanent or if there are plans for a more sophisticated approach (e.g., wallet address + IP hybrid for authenticated users).
Adding a TODO or more specific comment about future plans would help maintainers understand the long-term strategy. For example, the issue mentioned a "Tentative" rate limit based on X-Wallet-Address header as a potential alternative.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gateway/main_test.go (1)
157-208:⚠️ Potential issue | 🔴 Critical
TestRateLimitMiddleware_DifferentKeyswill fail — test was not updated for IP-based keyingBoth "users" are created with
http.NewRequestwhich leavesreq.RemoteAddrempty (""). Gin'sc.ClientIP()returns""for both, sogetRateLimitKeyproduces the same key"ip:"for every request in this test. After User 1 exhausts theRATE_LIMIT_STANDARD_BURST=2quota, User 2's request hits the same empty bucket and receives 429 — but the assertion at line 205 expects 200, causing a test failure.Fix: differentiate users via
RemoteAddr.🐛 Proposed fix — assign distinct RemoteAddr values
// User 1 exhausts their limit for i := 0; i < 2; i++ { req, _ := http.NewRequest("GET", "/test", nil) + req.RemoteAddr = "192.168.1.1:1234" req.Header.Set("X-402-Signature", "sig1") - req.Header.Set("X-402-Nonce", "user1-11111111") // Different first 8 chars + req.Header.Set("X-402-Nonce", "user1-11111111") ... } // User 1 should now be rate limited req1, _ := http.NewRequest("GET", "/test", nil) +req1.RemoteAddr = "192.168.1.1:1234" req1.Header.Set("X-402-Signature", "sig1") req1.Header.Set("X-402-Nonce", "user1-11111111") ... // User 2 should still be allowed (different IP bucket) req2, _ := http.NewRequest("GET", "/test", nil) +req2.RemoteAddr = "192.168.1.2:1234" req2.Header.Set("X-402-Signature", "sig2") -req2.Header.Set("X-402-Nonce", "user2-22222222") // Different first 8 chars +req2.Header.Set("X-402-Nonce", "user2-22222222")Note: this fix only works correctly once
SetTrustedProxies(nil)is applied inmain.go(as flagged above), otherwise Gin would still readX-Forwarded-Foreven in tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/main_test.go` around lines 157 - 208, The test TestRateLimitMiddleware_DifferentKeys is failing because requests created with http.NewRequest have empty RemoteAddr so getRateLimitKey (via c.ClientIP()) returns the same "ip:" key; fix by assigning distinct RemoteAddr values on each http.Request used in this test (the two loop requests for user1, the subsequent req1, and the req2 for user2) so RateLimitMiddleware/getRateLimitKey produce different buckets per user; also ensure Gin trusted proxies are set appropriately (SetTrustedProxies(nil) in main.go) so ClientIP uses RemoteAddr in tests.
🧹 Nitpick comments (1)
gateway/main_test.go (1)
243-265:expectedKeytable field is unused — assertion hardcodes"ip:"The
expectedKey stringfield is declared in the test struct and populated in every case, but the assertion at Line 263 hardcodes"ip:"instead of usingtt.expectedKey. The field is dead code.Either drive the check from the table, or drop the field entirely.
♻️ Proposed refactor
- if !strings.HasPrefix(key, "ip:") { - t.Errorf("Expected IP-based key, got '%s'", key) + if !strings.HasPrefix(key, tt.expectedKey) { + t.Errorf("Expected key with prefix '%s', got '%s'", tt.expectedKey, key) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/main_test.go` around lines 243 - 265, The test table declares an unused expectedKey field but the assertion hardcodes "ip:"; update the test in gateway/main_test.go to either use tt.expectedKey in the assertion (e.g., check strings.HasPrefix(key, tt.expectedKey) or compare key to tt.expectedKey as intended) or remove the expectedKey field from the test struct and table rows and keep the hardcoded "ip:" check—locate the test that calls getRateLimitKey(c) and modify the t.Errorf/HasPrefix check accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gateway/main.go`:
- Around line 727-734: getRateLimitKey currently relies on c.ClientIP(), which
can be spoofed via X-Forwarded-For unless Gin's trusted proxies are set; update
main() (after r := gin.Default()) to call r.SetTrustedProxies with a safe value
— either nil to disable proxy header parsing (Engine.SetTrustedProxies(nil)) if
no reverse proxy is used, or the specific proxy CIDR(s) (e.g. "10.0.0.0/8") if
behind a known reverse proxy — so ClientIP() returns a reliable source and
prevents per-request unique IP spoofing; leave getRateLimitKey unchanged to
continue using c.ClientIP() once trusted proxies are properly configured.
---
Outside diff comments:
In `@gateway/main_test.go`:
- Around line 157-208: The test TestRateLimitMiddleware_DifferentKeys is failing
because requests created with http.NewRequest have empty RemoteAddr so
getRateLimitKey (via c.ClientIP()) returns the same "ip:" key; fix by assigning
distinct RemoteAddr values on each http.Request used in this test (the two loop
requests for user1, the subsequent req1, and the req2 for user2) so
RateLimitMiddleware/getRateLimitKey produce different buckets per user; also
ensure Gin trusted proxies are set appropriately (SetTrustedProxies(nil) in
main.go) so ClientIP uses RemoteAddr in tests.
---
Nitpick comments:
In `@gateway/main_test.go`:
- Around line 243-265: The test table declares an unused expectedKey field but
the assertion hardcodes "ip:"; update the test in gateway/main_test.go to either
use tt.expectedKey in the assertion (e.g., check strings.HasPrefix(key,
tt.expectedKey) or compare key to tt.expectedKey as intended) or remove the
expectedKey field from the test struct and table rows and keep the hardcoded
"ip:" check—locate the test that calls getRateLimitKey(c) and modify the
t.Errorf/HasPrefix check accordingly.
| // getRateLimitKey determines the key for rate limiting (always uses IP) | ||
| func getRateLimitKey(c *gin.Context) string { | ||
| signature := c.GetHeader("X-402-Signature") | ||
| nonce := c.GetHeader("X-402-Nonce") | ||
|
|
||
| // Only use nonce-based key if BOTH signature and nonce are present | ||
| // This prevents attackers from bypassing IP rate limits with fake nonces | ||
| if signature != "" && nonce != "" { | ||
| hash := sha256.Sum256([]byte(nonce)) | ||
| // Use 32 hex chars (128 bits) for better collision resistance | ||
| return "nonce:" + hex.EncodeToString(hash[:])[:32] | ||
| } | ||
|
|
||
| // REMOVED: Nonce-based keying | ||
| // Nonces must be unique per request (replay attack prevention), | ||
| // which creates infinite buckets and memory leaks. | ||
| // ALWAYS use IP for now to prevent infinite-bucket attacks | ||
| return "ip:" + c.ClientIP() | ||
| } |
There was a problem hiding this comment.
c.ClientIP() can be spoofed via X-Forwarded-For, recreating the infinite-bucket attack
TrustedProxies is enabled by default and trusts all proxies by default. Because no SetTrustedProxies call is made in main() (r := gin.Default() at Line 205), ClientIP() will try to parse the headers X-Forwarded-For and X-Real-IP. A client sending X-Forwarded-For: <unique-IP-per-request> generates a distinct key on every call — the same infinite-bucket root cause this PR aims to fix, just through a different attack header.
Gin's own documentation warns: "Gin trust all proxies by default if you don't specify a trusted proxy using the function above, this is NOT safe. At the same time, if you don't use any proxy, you can disable this feature by using Engine.SetTrustedProxies(nil), then Context.ClientIP() will return the remote address directly."
The cleanest remediation is to configure trusted proxies in main():
🛡️ Proposed fix — restrict trusted proxies in main()
r := gin.Default()
+// Only trust the direct connection IP (RemoteAddr).
+// If behind a known reverse-proxy (e.g. "10.0.0.1"), list it explicitly.
+r.SetTrustedProxies(nil)If the gateway is legitimately deployed behind a reverse proxy, restrict to the actual proxy CIDR instead of nil (e.g. r.SetTrustedProxies([]string{"10.0.0.0/8"})) so the real client IP is still retrieved correctly without allowing arbitrary spoofing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gateway/main.go` around lines 727 - 734, getRateLimitKey currently relies on
c.ClientIP(), which can be spoofed via X-Forwarded-For unless Gin's trusted
proxies are set; update main() (after r := gin.Default()) to call
r.SetTrustedProxies with a safe value — either nil to disable proxy header
parsing (Engine.SetTrustedProxies(nil)) if no reverse proxy is used, or the
specific proxy CIDR(s) (e.g. "10.0.0.0/8") if behind a known reverse proxy — so
ClientIP() returns a reliable source and prevents per-request unique IP
spoofing; leave getRateLimitKey unchanged to continue using c.ClientIP() once
trusted proxies are properly configured.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gateway/main_test.go (1)
157-208:⚠️ Potential issue | 🟠 Major
TestRateLimitMiddleware_DifferentKeysis semantically broken after the IP-based keying switch and will fail in CIThe test distinguishes two "users" purely by their
X-402-Signature/X-402-Nonceheaders — no distinct client IPs are set. With IP-based keying,getRateLimitKeycallsc.ClientIP(), which in turn readsreq.RemoteAddr(empty forhttp.NewRequest) and theX-Forwarded-For/X-Real-IPheaders (neither is set here). Gin resolves both "users" to the same IP, placing them in the same rate-limit bucket.Consequence: after User 1 exhausts
RATE_LIMIT_STANDARD_BURST=2, User 2's first request hits the same depleted bucket and receives a429— not the200asserted at line 205 — causing a CI failure.The comment on line 181 (
// Different first 8 chars) is also a stale artifact of the old nonce-key prefix slicing logic.Fix: set distinct
X-Forwarded-Foraddresses for each simulated user so they actually map to different IP-based buckets.🐛 Proposed fix
- // User 1 exhausts their limit - for i := 0; i < 2; i++ { - req, _ := http.NewRequest("GET", "/test", nil) - req.Header.Set("X-402-Signature", "sig1") - req.Header.Set("X-402-Nonce", "user1-11111111") // Different first 8 chars - w := httptest.NewRecorder() + // User 1 (IP 10.0.0.1) exhausts their limit + for i := 0; i < 2; i++ { + req, _ := http.NewRequest("GET", "/test", nil) + req.Header.Set("X-Forwarded-For", "10.0.0.1") + w := httptest.NewRecorder() r.ServeHTTP(w, req) if w.Code != 200 { t.Errorf("User 1 request %d should succeed", i+1) } } // User 1 should now be rate limited req1, _ := http.NewRequest("GET", "/test", nil) - req1.Header.Set("X-402-Signature", "sig1") - req1.Header.Set("X-402-Nonce", "user1-11111111") + req1.Header.Set("X-Forwarded-For", "10.0.0.1") w1 := httptest.NewRecorder() r.ServeHTTP(w1, req1) if w1.Code != 429 { t.Error("User 1 should be rate limited") } - // User 2 should still be allowed (different bucket) + // User 2 (IP 10.0.0.2) should still be allowed (different IP bucket) req2, _ := http.NewRequest("GET", "/test", nil) - req2.Header.Set("X-402-Signature", "sig2") - req2.Header.Set("X-402-Nonce", "user2-22222222") // Different first 8 chars + req2.Header.Set("X-Forwarded-For", "10.0.0.2") w2 := httptest.NewRecorder() r.ServeHTTP(w2, req2) if w2.Code != 200 { t.Error("User 2 should not be rate limited (separate bucket)") }Verify what
c.ClientIP()returns in gin whenreq.RemoteAddris unset and no forwarding headers are present:#!/bin/bash # Check gin's ClientIP resolution when RemoteAddr is empty and no forwarding headers are set. rg -n "ClientIP\|RemoteIP\|RemoteAddr\|TrustedProxies\|ForwardedByClientIP" --type go -A3 -B3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/main_test.go` around lines 157 - 208, The test TestRateLimitMiddleware_DifferentKeys fails after switching to IP-based keying because getRateLimitKey uses c.ClientIP(), and the http.NewRequest calls don't set RemoteAddr or forwarding headers so both requests map to the same IP; update the test to set distinct X-Forwarded-For (or X-Real-IP) headers on the two simulated users' requests (e.g., "1.2.3.4" vs "5.6.7.8") so each maps to a different rate-limit bucket and the assertions expecting separate behavior pass.
🧹 Nitpick comments (1)
gateway/main_test.go (1)
243-265:expectedKeyfield is dead code — assertion ignores itThe test struct at lines 247–252 declares and populates
expectedKey, but the loop body (line 263) hardcodes the prefix checkstrings.HasPrefix(key, "ip:")instead of usingtt.expectedKey. The field has no effect on test outcome. Either use it in the assertion or drop it from the struct entirely.♻️ Use `tt.expectedKey` in the assertion (and drop the struct field if it stays constant)
key := getRateLimitKey(c) - - // After fix: All keys should be IP-based to prevent - // infinite bucket attacks from unique nonces - if !strings.HasPrefix(key, "ip:") { - t.Errorf("Expected IP-based key, got '%s'", key) + if !strings.HasPrefix(key, tt.expectedKey) { + t.Errorf("Expected key with prefix '%s', got '%s'", tt.expectedKey, key) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/main_test.go` around lines 243 - 265, The test struct defines expectedKey but the assertion always hardcodes "ip:", so expectedKey is unused; update the test loop to assert against tt.expectedKey (e.g., replace strings.HasPrefix(key, "ip:") with strings.HasPrefix(key, tt.expectedKey) or an equality check as appropriate) when calling getRateLimitKey, and if expectedKey is the same for all cases you can remove the field and keep a single literal; reference the tests slice and getRateLimitKey to locate where to change the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@gateway/main_test.go`:
- Around line 157-208: The test TestRateLimitMiddleware_DifferentKeys fails
after switching to IP-based keying because getRateLimitKey uses c.ClientIP(),
and the http.NewRequest calls don't set RemoteAddr or forwarding headers so both
requests map to the same IP; update the test to set distinct X-Forwarded-For (or
X-Real-IP) headers on the two simulated users' requests (e.g., "1.2.3.4" vs
"5.6.7.8") so each maps to a different rate-limit bucket and the assertions
expecting separate behavior pass.
---
Nitpick comments:
In `@gateway/main_test.go`:
- Around line 243-265: The test struct defines expectedKey but the assertion
always hardcodes "ip:", so expectedKey is unused; update the test loop to assert
against tt.expectedKey (e.g., replace strings.HasPrefix(key, "ip:") with
strings.HasPrefix(key, tt.expectedKey) or an equality check as appropriate) when
calling getRateLimitKey, and if expectedKey is the same for all cases you can
remove the field and keep a single literal; reference the tests slice and
getRateLimitKey to locate where to change the assertion.
1 participant
Closes #100
Problem
The current rate limiter used nonce-based keying for authenticated requests. Since nonces must be unique per request (to prevent replay attacks), every request created a new rate limit bucket, resulting in:
Solution
Changed to IP-based rate limiting for all requests to prevent infinite-bucket attacks.
Changes
getRateLimitKey()to always return IP-based keysTestGetRateLimitKeyto validate IP-based behaviorTesting
Summary by CodeRabbit
Bug Fixes
Tests
Greptile Summary
This PR fixes the infinite bucket creation bug by switching from nonce-based to IP-based rate limiting, but introduces a new critical issue where authenticated users sharing the same IP (corporate NAT, VPNs, proxies) now share rate limit buckets instead of having individual limits.
Key Changes:
getRateLimitKey()to always return IP-based keysCritical Issues Found:
ClientIP()withoutSetTrustedProxies()may return proxy IP, causing all users behind load balancer to share one bucketTestRateLimitMiddleware_DifferentKeysclaims to test separate user buckets but doesn't validate this after the changeRecommended Solution:
Use wallet address (from
RecoveredAddress) as the rate limit key for authenticated requests, keeping IP-based limits only for anonymous users.Confidence Score: 1/5
getRateLimitKey()function needs wallet-based keying for authenticated usersImportant Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Request Arrives] --> B[RateLimitMiddleware] B --> C[getRateLimitKey] C --> D{Has Signature<br/>and Nonce?} D -->|Before: Yes| E[BEFORE: nonce-based key<br/>Creates NEW bucket<br/>per request] D -->|After: Yes/No| F[AFTER: IP-based key<br/>c.ClientIP] E --> G[selectRateLimitTier] F --> G G --> H{Authenticated?} H -->|Yes| I[standard tier<br/>60 RPM] H -->|No| J[anonymous tier<br/>10 RPM] I --> K[limiters map standard] J --> L[limiters map anonymous] K --> M[Check bucket<br/>limiter.Allow key] L --> M M --> N{Allowed?} N -->|Yes| O[Continue] N -->|No| P[429 Rate Limited] style E fill:#ffcccc style F fill:#ccffcc style M fill:#ffffccLast reviewed commit: fb674b0
(2/5) Greptile learns from your feedback when you react with thumbs up/down!