feat(security): add per-IP rate limiting on guest-order endpoints (#19) #29
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/guest-order-rate-limiting"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Why
Issue #19:
POST /api/guest-ordersis fully public (permitAll) with no throttling or IP-based limiting. An attacker can flood the orders table (DB bloat/DoS) and mass-generate self-confirmed paid orders.Changes
@Component,OncePerRequestFilter): in-memory sliding-window rate limiterapp.rate-limit.guest-create)app.rate-limit.guest-default)X-Forwarded-ForandX-Real-IPheadersreset()in@BeforeEachPhase 0 interim: in-memory, per-JVM, resets on restart. For production, use Bucket4j + Redis or nginx
limit_req.Test plan
./gradlew :backend:test— BUILD SUCCESSFULCloses #19
POST /api/guest-orders is fully public (permitAll) with no captcha, throttling, or IP-based limiting. An attacker can flood the orders table (DB bloat / DoS) and mass-generate self-confirmed paid orders. Changes: - GuestOrderRateLimitFilter: in-memory sliding-window rate limiter (OncePerRequestFilter + @Component). Per-IP limits: POST /api/guest-orders: 5 req/min (configurable) Other /api/guest-orders/**: 20 req/min (configurable) Returns 429 with Swedish JSON error when exceeded. Respects X-Forwarded-For and X-Real-IP headers. - SecurityConfig: register the filter before JWT filter - GuestOrderControllerTest: set high limits (1000) and reset filter state in @BeforeEach to avoid cross-test interference Config via application properties: app.rate-limit.guest-create (default 5) app.rate-limit.guest-default (default 20) Phase 0 interim: in-memory, per-JVM, resets on restart. For production, use Bucket4j + Redis or nginx limit_req. Closes #19Advisory review: ship with follow-up tracked for Phase 0 hardening.
Solid implementation of a sliding-window per-IP rate limiter. Clean code, good Javadoc, correct filter ordering. A few items below are worth addressing before this sees real traffic.
Warnings
X-Forwarded-For trust is unconditional
extractClientIptrustsX-Forwarded-For/X-Real-IPby default. If the app is ever directly reachable (no reverse proxy in front, or proxy doesn't strip client-supplied headers), an attacker can rotate the header value per request to bypass rate limiting entirely. Consider a config flag likeapp.rate-limit.trust-forwarded-headers(defaultfalse) that only trusts forwarded headers when explicitly enabled. The PR description mentions "respects" these headers but doesn't note the spoofing risk.Unbounded
storemap growthConcurrentHashMap<String, Deque<Instant>>never removes entries. Stale IP keys (where the deque has been pruned to empty) persist forever. Combined with X-Forwarded-For spoofing, an attacker can cause unbounded memory growth by sending requests with unique header values. A scheduled cleanup (evict entries with empty deques every N minutes) or a bounded cache (Caffeine withmaximumSize) would address this. Acceptable for Phase 0 but should be tracked.Suggestions
No dedicated rate-limit tests
The test changes only raise limits to 1000 and add
reset(). There are no tests that actually verify the rate-limiting behavior (e.g., firecreateLimit + 1requests, assert the last returns 429 with the expected body). For a security control, the core logic should be directly tested, not just bypassed. Consider a separateGuestOrderRateLimitFilterTestwith low configured limits.Use existing
ErrorResponseDTO instead of hardcoded JSONThe codebase already has
ErrorResponse(String message)andSecurityConfigusesobjectMapper.writeValueAsString(new ErrorResponse(message))for all other error responses. The filter handcrafts a raw JSON string instead ("{\"message\":\"...\"}"). InjectObjectMapperand follow the existing pattern for consistency and to avoid escape fragility.Add
Retry-Afterheader on 429RFC 6585 recommends including
Retry-Afteron 429 responses. Since the window is known (60s), addingresponse.setHeader("Retry-After", "60")is trivial and helps well-behaved clients back off.isCreatePathdoesn't handle trailing slashpath.equals("/api/guest-orders")won't match/api/guest-orders/. Spring typically normalizes this, but some clients or proxies may not. Considerpath.equals("/api/guest-orders") || path.equals("/api/guest-orders/")or normalizing first.Looks Good
computeIfAbsent+synchronizedon the deque is the right pattern@Componentauto-detection,@Valueconfiguration, andreset()for test isolation are all well done🟡 Unbounded map growth. Stale IP keys (pruned deques) are never removed from the map. With X-Forwarded-For spoofing, an attacker can cause unbounded memory growth by sending unique header values. Consider a scheduled cleanup task or a bounded cache (Caffeine
maximumSize).💡 Add
Retry-Afterheader.response.setHeader("Retry-After", "60")is trivial and helps well-behaved clients back off. RFC 6585 recommends it for 429 responses.💡 Use the existing
ErrorResponseDTO. The codebase hasErrorResponse(String message)and SecurityConfig usesobjectMapper.writeValueAsString(new ErrorResponse(message))for all error responses. InjectObjectMapperand follow the same pattern to avoid hardcoded JSON and keep responses consistent.🟡 X-Forwarded-For is trusted unconditionally. If the app is directly reachable (no stripping reverse proxy), an attacker can rotate this header per request to bypass rate limiting entirely. Consider a config flag like
app.rate-limit.trust-forwarded-headers(defaultfalse).💡 The rate limiter is wired in but never actually tested. No test fires
createLimit + 1requests to verify a 429 is returned. For a security control, consider a dedicatedGuestOrderRateLimitFilterTestwith low limits that exercises the actual rejection path.View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.Merge
Merge the changes and update on Forgejo.Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.