feat: add configurable rate limiting for file operations#81
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Introduces token-bucket rate limiting for file upload/download/listing endpoints, enforced via a Spring Security filter and configurable via cloudpage.rate-limit.*.
Changes:
- Add
RateLimitFilter+RateLimitService+ token-bucket implementation with per-client and optional global tiers. - Wire the filter into the Spring Security chain and add default rate-limit configuration properties.
- Add unit tests for the limiter and filter; update controller tests to mock the new filter.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/src/main/java/cloudpage/ratelimit/TokenBucket.java | Implements a synchronized token bucket with deterministic clock injection. |
| backend/src/main/java/cloudpage/ratelimit/RateLimitService.java | Creates/caches buckets and enforces global + per-client admission. |
| backend/src/main/java/cloudpage/ratelimit/RateLimitFilter.java | Applies rate limiting to selected endpoints and returns 429 responses. |
| backend/src/main/java/cloudpage/ratelimit/RateLimitProperties.java | Adds configuration model for rate-limit tiers and policies. |
| backend/src/main/java/cloudpage/ratelimit/RateLimitDecision.java | Defines the result shape for limiter decisions. |
| backend/src/main/java/cloudpage/ratelimit/RateLimitCategory.java | Defines rate-limit categories (UPLOAD/DOWNLOAD/LISTING). |
| backend/src/main/java/cloudpage/security/SecurityConfig.java | Registers the rate-limit filter in the security filter chain. |
| backend/src/main/resources/application.properties | Adds default rate-limit settings and documentation comments. |
| backend/src/test/java/cloudpage/ratelimit/RateLimitServiceTest.java | Tests token bucket behavior via the service API. |
| backend/src/test/java/cloudpage/ratelimit/RateLimitFilterTest.java | Tests filter behavior (headers, 429 behavior, keying). |
| backend/src/test/java/cloudpage/controller/FolderControllerTest.java | Mocks RateLimitFilter to keep controller tests isolated. |
| backend/src/test/java/cloudpage/controller/FileControllerTest.java | Mocks RateLimitFilter to keep controller tests isolated. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Addressed the Copilot review in 6c6ede6:
Left the per-client bucket cache unbounded for now — with per-IP keys bound to real remote addresses the cardinality stays limited for a self-hosted instance, but happy to add TTL/size-bounded eviction (e.g. Caffeine) if you'd prefer. |
There was a problem hiding this comment.
Really like this overall ! clean token-bucket approach, no extra deps, and the tests are thorough. One thing I'd want fixed before merging though:
perClientBuckets grows without bound. We computeIfAbsent a bucket per (category, clientKey) but never remove anything, and for unauthenticated traffic the key is the client IP. So every distinct IP that ever hits an upload/download/listing endpoint leaves a permanent entry behind — the map only ever grows. It's a slow leak, but it's a bit ironic for an anti-abuse filter to have its own unbounded-growth surface.
Could you add some cleanup for stale clients? Two options:
- a periodic
@Scheduledsweep that drops buckets which have refilled back to full (i.e. the client's been idle longer than a refill period - re-creating it later is identical, so it's safe to evict). Keeps things dependency-free, matches the rest of the PR. - or swap the map for a Caffeine cache with
expireAfterAccess, if you'd rather not hand-roll it.
Everything else looks good to me - happy to approve once the buckets can't grow forever. @GabrielBBaldez
Add a per-user/per-IP (and optional instance-wide) token-bucket rate limiter for the upload, download and listing endpoints. Requests over budget get 429 with Retry-After and X-Rate-Limit-Remaining headers. Limits are configurable per category via cloudpage.rate-limit.* properties (overridable by env vars); implemented as a Spring filter with no new dependency.
Evaluate the per-client tier before the global one so a per-client-throttled client cannot drain the shared global budget (with a regression test); key per-IP limiting on getRemoteAddr() only (X-Forwarded-For is client-controlled and spoofable); match listing on an exact /api/folders or /api/folders/ prefix; drop throttle logs to debug without the client key; and omit X-Rate-Limit-Remaining when a category is unlimited.
Add a scheduled sweep that drops per-client buckets once they have refilled back to full, bounding the cache to currently-active clients (re-creating a bucket later is identical, so eviction is safe). Dependency-free, as suggested. Also mock RateLimitFilter in TrashControllerTest, which now loads the rate-limit-aware SecurityConfig.
|
Done — added a scheduled sweep that drops per-client buckets once they've refilled back to full, so the cache stays bounded to currently-active clients. Went with the dependency-free option you suggested (re-creating a bucket on the next request is identical, so evicting an idle one is safe). Also rebased onto main. |
6c6ede6 to
1de5bdf
Compare
DenizAltunkapan
left a comment
There was a problem hiding this comment.
@GabrielBBaldez Thanks!
Closes #35.
Adds configurable rate limiting for the file upload, download and listing endpoints to prevent abuse and smooth out load.
Approach
A token-bucket limiter implemented as a Spring
OncePerRequestFilter, with no new dependency. The issue suggested "Bucket4j or Spring-based filters"; I went with the latter since the per-user / per-IP / global policies are easy to express directly and it keeps the dependency surface flat (relevant for a self-hosted, security-focused app). The bucket reads time from an injectable clock, so the tests are deterministic (noThread.sleep). It runs right afterJwtAuthFilter, keying buckets by username when authenticated and falling back to the client IP.What it does
UPLOAD(POST /api/files/upload),DOWNLOAD(/api/files/{download,view,content}) andLISTING(GET /api/folders/**).429 Too Many RequestswithRetry-AfterandX-Rate-Limit-Remainingheaders; allowed responses also carry the remaining count.cloudpage.ratelimit.throttledcounter (via the existing Micrometer/actuator registry, when present) and a warn log on each throttle.cloudpage.rate-limit.*properties, overridable by environment variables. A non-positive capacity disables limiting for that category.Tests
RateLimitServiceTestcovers capacity/throttling, refill after the period, per-client and per-category isolation, the global tier binding across clients, and the disabled case.RateLimitFilterTestcovers category mapping, the429+ headers, user-vs-IP keying, unmatched paths passing through, and the masterenabled=falseswitch. Full suite green locally (104 tests); the existing@WebMvcTestcontroller tests gained a@MockitoBean RateLimitFilter, matching how they already mockJwtAuthFilter.Notes
The defaults (30 / 120 / 240 per minute) are conservative starting points — happy to tune. Buckets are kept in-memory per instance; a shared store would be an easy follow-up if you ever run multiple replicas.