Feature/rate limiting filter#88
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new per‑IP RateLimitingFilter (Bucket4j, 10 tokens, refill 1/10s) with background idle‑bucket cleanup and testing hooks; integrates the filter at startup in ConnectionHandler and treats HTTP 429 like 400/403 for early short‑circuiting; adds SC_TOO_MANY_REQUESTS and unit tests. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Conn as ConnectionHandler
participant RL as RateLimitingFilter
participant Store as BucketStore
participant Chain as FilterChain
participant App as Application
Client->>Conn: HTTP request
Conn->>RL: doFilter(request, response, chain)
RL->>RL: resolve client IP
RL->>Store: get/create bucket for IP
alt token available
Store->>Store: consume token
RL->>Chain: proceed with chain
Chain->>App: handle request
App-->>Chain: response
Chain-->>Conn: response
Conn-->>Client: HTTP 200/other
else token exhausted
Store->>RL: token unavailable
RL-->>Client: HTTP 429 Too Many Requests
Conn->>Conn: treat 429 as early return (stop processing)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/java/org/example/filter/RateLimitingFilter.java (1)
74-76: Prefer package‑privateclearBuckets()to limit API surface.
It’s only used by tests in the same package; making it non‑public keeps production API tighter.♻️ Suggested visibility change
- public void clearBuckets(){ - buckets.clear(); - } + void clearBuckets() { + buckets.clear(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/filter/RateLimitingFilter.java` around lines 74 - 76, Change the visibility of the clearBuckets method on RateLimitingFilter from public to package-private to reduce the public API surface; locate the public void clearBuckets() method (which calls buckets.clear()) and remove the public modifier so it has default (package) visibility, ensuring tests in the same package can still call it and production code outside the package cannot.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/filter/RateLimitingFilter.java`:
- Around line 26-31: The static ConcurrentHashMap named buckets in
RateLimitingFilter currently never evicts per‑IP Bucket instances; replace it
with a bounded, expiring cache (e.g., a Caffeine or Guava Cache) configured with
maximumSize and expireAfterAccess to avoid unbounded growth, and update any
accesses that use buckets (lookups/compute) to use the cache's
get/computeIfAbsent semantics so Bucket creation still uses capacity,
refillTokens and refillPeriod; alternatively implement explicit lastAccess
timestamp on Bucket and run a periodic cleanup to remove idle entries, but do
not leave the static buckets map unbounded.
- Around line 42-47: In RateLimitingFilter.doFilter, guard the retrieval of
request.getAttribute("clientIp") before casting and using it as a map key:
verify the attribute is non-null and an instance of String, and if not, set an
explicit error on the HttpResponseBuilder (e.g., 400 Bad Request or a clear
error body) and return early instead of calling buckets.computeIfAbsent; use the
same clientIp variable when calling createNewBucket and downstream chain only
when the check passes.
---
Nitpick comments:
In `@src/main/java/org/example/filter/RateLimitingFilter.java`:
- Around line 74-76: Change the visibility of the clearBuckets method on
RateLimitingFilter from public to package-private to reduce the public API
surface; locate the public void clearBuckets() method (which calls
buckets.clear()) and remove the public modifier so it has default (package)
visibility, ensuring tests in the same package can still call it and production
code outside the package cannot.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/org/example/ConnectionHandler.javasrc/main/java/org/example/filter/RateLimitingFilter.javasrc/main/java/org/example/http/HttpResponseBuilder.javasrc/test/java/org/example/filter/RateLimitingFilterTest.java
| @Override | ||
| public void doFilter(HttpRequest request, HttpResponseBuilder response, FilterChain chain) { | ||
|
|
||
| String clientIp = (String) request.getAttribute("clientIp"); |
There was a problem hiding this comment.
Since at the moment the attribute clientIp is the socket connections remote ip it will be the same for all clients when we are running the application behind a gateway. When doing so we can configure the gateway to set a header with the real client ip.
X-Forwarded-For: 83.218.80.21, 192.168.1.109
Here the real ip for the client is 83.218.80.21 and our gateway has ip 192.168.1.109. If this header exists we should take the ip number one step before our trusted gateways last occurance.
Preferable this should be a configurable setting so we can change our trusted gateway.
There was a problem hiding this comment.
I have fixed this in my last commits.
|
Koden är ren, försåelig och möter de upsatta kraven men behöver finslipning och safe guards.
Så snart som de nuvarande problemen lösts, FilterChain är implementerad och filtret har testats i körning med Insomnia requests borde en merge gå att göra. Snyggt jobbat! |
…tsCount and ageBucketsForTesting
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/org/example/filter/RateLimitingFilter.java (1)
45-52: ConfirmclientIpis derived with trusted-proxy logic upstream.This limiter is only as correct as the producer of
clientIp. In proxied deployments, using socket IP collapses clients; using rawX-Forwarded-Forwithout trust policy allows spoofing.#!/bin/bash set -euo pipefail echo "== Find where clientIp attribute is written/read ==" rg -nP --type=java -C3 'setAttribute\(\s*"clientIp"|getAttribute\(\s*"clientIp"' echo echo "== Find proxy-header parsing and trust handling ==" rg -nP --type=java -C3 'X-Forwarded-For|Forwarded|trusted proxy|proxy'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/filter/RateLimitingFilter.java` around lines 45 - 52, The RateLimitingFilter currently reads clientIp via request.getAttribute("clientIp") — confirm and enforce that the attribute is produced upstream using trusted-proxy logic (i.e., a component that parses X-Forwarded-For/Forwarded and applies a trust policy) rather than raw socket IP or untrusted headers; locate the code that calls request.setAttribute("clientIp") and the proxy-header parsing code, verify it implements trust checks for known proxies, and if missing either (a) move header parsing/trust logic into a dedicated TrustedProxyResolver used before RateLimitingFilter or (b) add validation in RateLimitingFilter to reject or fallback when the attribute appears derived from untrusted headers. Ensure references: request.getAttribute("clientIp"), request.setAttribute("clientIp"), RateLimitingFilter, and any proxy/header parsing utilities are used to find and fix the upstream trust handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/filter/RateLimitingFilter.java`:
- Around line 34-37: The cleanup worker started in init() via
startCleanupThread() currently has no lifecycle control and can be started
multiple times or leaked because destroy() never stops it; modify
RateLimitingFilter to hold a Thread field (e.g., cleanupThread) and a volatile
boolean or AtomicBoolean (e.g., cleanupRunning), make startCleanupThread()
synchronized to no-op if cleanupRunning is true, have the cleanup Runnable check
cleanupRunning and respond to interrupts, and implement destroy() to set
cleanupRunning=false, interrupt cleanupThread, and join it (or wait with
timeout) to ensure the background loop stops cleanly; update any cleanupRunnable
references and thread creation in startCleanupThread()/init()/destroy()
accordingly.
---
Duplicate comments:
In `@src/main/java/org/example/filter/RateLimitingFilter.java`:
- Around line 45-52: The RateLimitingFilter currently reads clientIp via
request.getAttribute("clientIp") — confirm and enforce that the attribute is
produced upstream using trusted-proxy logic (i.e., a component that parses
X-Forwarded-For/Forwarded and applies a trust policy) rather than raw socket IP
or untrusted headers; locate the code that calls
request.setAttribute("clientIp") and the proxy-header parsing code, verify it
implements trust checks for known proxies, and if missing either (a) move header
parsing/trust logic into a dedicated TrustedProxyResolver used before
RateLimitingFilter or (b) add validation in RateLimitingFilter to reject or
fallback when the attribute appears derived from untrusted headers. Ensure
references: request.getAttribute("clientIp"), request.setAttribute("clientIp"),
RateLimitingFilter, and any proxy/header parsing utilities are used to find and
fix the upstream trust handling.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/example/filter/RateLimitingFilter.javasrc/test/java/org/example/filter/RateLimitingFilterTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/org/example/filter/RateLimitingFilterTest.java
AnnaZiafar
left a comment
AnnaZiafar
left a comment
There was a problem hiding this comment.
Väldigt välskriven och välstrukturerad kod! Bra att du städar i alla trådar och undviker användningen av magic numbers. Snyggt att du har hög docstring coverage också, något jag själv bör tänka på heh. Tycker egentligen att den är redo för merge, enda jag har att peta på är Loggern. Jag har precis skapat en ny pr så den är ju inte mergad än, men där skapar jag en centralicerad logger. Man kanske vill vänta på att den mergeas så att du kan byta ut denna loggern mot den nya? Så att alla ser likadana ut:)
eeebbaandersson
left a comment
eeebbaandersson
left a comment
There was a problem hiding this comment.
Fin och välskriven kod med tydligt dokumentation som gör den lättläst för oss andra😊
Kan instämma med det som Anna nämnde tidigare gällande loggern, så om hennes kod mergas först och din därefter?
Verkligen snyggt jobbat för övrigt!
|
Ja absolut! Vi kan merga Annas pr först :) @eeebbaandersson @AnnaZiafar |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main/java/org/example/filter/RateLimitingFilter.java (3)
29-31: Consider makingrefillPeriodastatic finalconstant with SCREAMING_SNAKE_CASE.
CAPACITYandREFILL_TOKENSarestatic finalwith SCREAMING_SNAKE_CASE, butrefillPeriodis an instance field with camelCase. Since it's a fixed configuration value, it should be consistent with the other constants.♻️ Proposed fix
private static final long CAPACITY = 10; private static final long REFILL_TOKENS = 1; - private final Duration refillPeriod = Duration.ofSeconds(10); + private static final Duration REFILL_PERIOD = Duration.ofSeconds(10); private static final int MAX_BUCKETS_THRESHOLD = 1000;Then update the usage in
createNewBucket():- .refillGreedy(REFILL_TOKENS, refillPeriod) + .refillGreedy(REFILL_TOKENS, REFILL_PERIOD)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/filter/RateLimitingFilter.java` around lines 29 - 31, Make refillPeriod a static final constant using SCREAMING_SNAKE_CASE (e.g., REFILL_PERIOD) to match CAPACITY and REFILL_TOKENS, update its declaration to static final Duration REFILL_PERIOD = Duration.ofSeconds(10), and update any usage in createNewBucket() (and elsewhere) to reference REFILL_PERIOD instead of refillPeriod so all fixed configuration values are consistent.
151-160: Consider restricting visibility of testing methods.
getBucketsCount()andageBucketsForTesting()are public test helpers in production code. While functional, this exposes internal state. Consider making them package-private or documenting them as test-only with a naming convention (e.g., suffix withForTesting).♻️ Suggested visibility change
- public int getBucketsCount() { + // Package-private for testing + int getBucketsCount() { return buckets.size(); } - public void ageBucketsForTesting(long millisToSubtract) { + // Package-private for testing + void ageBucketsForTesting(long millisToSubtract) { for (BucketWrapper wrapper : buckets.values()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/filter/RateLimitingFilter.java` around lines 151 - 160, The two test helpers in RateLimitingFilter—getBucketsCount() and ageBucketsForTesting(long)—should not be public; change their visibility to package-private (remove the public modifier) or alternatively annotate/document them as test-only and/or rename to include a ForTesting suffix to signal limited use. Update any unit tests in the same package or test access strategy (e.g., move tests into the same package or use reflection/ testing utilities) to avoid breaking callers after making getBucketsCount() and ageBucketsForTesting(...) non-public.
117-121: X-Forwarded-For parsing takes the first IP, which may need refinement.The current implementation extracts the first IP from the
X-Forwarded-Forheader. While this is often correct (leftmost = original client), it doesn't handle the "trusted gateway" scenario mentioned in the PR comments where you'd want to take the IP one step before your trusted proxy.For now, this is a reasonable simplification. If you deploy behind multiple proxies or want configurable trusted gateways, consider enhancing this later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/filter/RateLimitingFilter.java` around lines 117 - 121, The code in RateLimitingFilter currently takes the first IP from the X-Forwarded-For header (variable xForwardedFor → clientIp), which isn't sufficient when you sit behind trusted proxies; update the X-Forwarded-For parsing in RateLimitingFilter to handle trusted proxies by splitting the header into addresses, iterating from left-to-right and selecting the first IP that is not in a configurable trustedProxies set (or first non-private address if you prefer), and fall back to the current first-entry behavior if no candidate is found; expose trustedProxies as a constructor/config property and update the logic in the method that sets clientIp to use this new filtering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/test/java/org/example/filter/RateLimitingFilterIpTest.java`:
- Around line 37-46: Remove the redundant stub that returns Map.of() and replace
it with a stub for request.getHeaders() so resolveClientIp won't NPE; i.e., keep
a single when(request.getAttribute("clientIp")).thenReturn("10.0.0.5") and add a
when(request.getHeaders()).thenReturn(Map.of()) (or appropriate empty headers
object) so resolveClientIp(request, response) can safely check headers before
falling back to the attribute.
---
Nitpick comments:
In `@src/main/java/org/example/filter/RateLimitingFilter.java`:
- Around line 29-31: Make refillPeriod a static final constant using
SCREAMING_SNAKE_CASE (e.g., REFILL_PERIOD) to match CAPACITY and REFILL_TOKENS,
update its declaration to static final Duration REFILL_PERIOD =
Duration.ofSeconds(10), and update any usage in createNewBucket() (and
elsewhere) to reference REFILL_PERIOD instead of refillPeriod so all fixed
configuration values are consistent.
- Around line 151-160: The two test helpers in
RateLimitingFilter—getBucketsCount() and ageBucketsForTesting(long)—should not
be public; change their visibility to package-private (remove the public
modifier) or alternatively annotate/document them as test-only and/or rename to
include a ForTesting suffix to signal limited use. Update any unit tests in the
same package or test access strategy (e.g., move tests into the same package or
use reflection/ testing utilities) to avoid breaking callers after making
getBucketsCount() and ageBucketsForTesting(...) non-public.
- Around line 117-121: The code in RateLimitingFilter currently takes the first
IP from the X-Forwarded-For header (variable xForwardedFor → clientIp), which
isn't sufficient when you sit behind trusted proxies; update the X-Forwarded-For
parsing in RateLimitingFilter to handle trusted proxies by splitting the header
into addresses, iterating from left-to-right and selecting the first IP that is
not in a configurable trustedProxies set (or first non-private address if you
prefer), and fall back to the current first-entry behavior if no candidate is
found; expose trustedProxies as a constructor/config property and update the
logic in the method that sets clientIp to use this new filtering.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/org/example/ConnectionHandler.javasrc/main/java/org/example/filter/RateLimitingFilter.javasrc/main/java/org/example/http/HttpResponseBuilder.javasrc/test/java/org/example/filter/RateLimitingFilterIpTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/example/ConnectionHandler.java
Min är mergad nu. Om du bara byter ut loggern så tycker jag att din kod är redo för merge. Snyggt jobbat! |
Martin-E-Karlsson
left a comment
Martin-E-Karlsson
left a comment
There was a problem hiding this comment.
Riktigt bra och strukturerat arbete!
Jag gillar alla de tillagda testerna, användningen av statiska variabler och extraheringen av metoden du gjorde. Koden är renare och mer lättläst än senast jag läste den!
Kathify
left a comment
Kathify
left a comment
There was a problem hiding this comment.
Riktigt snyggt jobbat med hela lösningen! Det märks att du har lagt ner mycket tanke på både funktionalitet och stabilitet.
6 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests