feat(security): rate limit the public endpoints an anonymous caller can drive - #14
Conversation
…an drive There was no rate limiting anywhere in the application. Every unauthenticated endpoint could be called in a tight loop, which made three things cheap: flooding the message upload endpoints with 20 MB bodies, spraying login and support registration, and burning LLM spend through the conversation endpoints. Add a small in-process fixed-window limiter and attach it to six public routes: login (20 per window), support registration (10), the widget session exchange (120), message attachment and image upload (30, shared between the two), and documentation feedback (20). The window is 60 seconds by default, and both it and the on/off switch are configurable. The limits are sized for a human driving a browser, not for a machine. The session exchange budget in particular has to absorb a whole support office loading the widget from behind one NAT address, which is why it sits an order of magnitude above the others. Deliberately not limited: - /api/third/* channel webhooks. A platform that receives a 429 from its webhook stops retrying and eventually disables the delivery, which takes an entire channel offline - a far worse outcome than the flood the limit was meant to stop. Those endpoints authenticate by signature instead. - /api/ws/* websockets, and /api/webhooks/* which are HMAC verified. - /api/dashboard/* - already behind AuthMiddleware, and staff sharing one office address would throttle each other. Rejections return 429 with a Retry-After header and an ordinary JsonResult body. The status is a real 429 rather than the 200-with-error-code the auth middleware uses, because web/lib/api/client.ts parses the payload before it inspects response.ok and surfaces payload.message, so the localized text still reaches the user - and Retry-After is only meaningful on a 429 or 503. The message is error.e0354 in both backend locales and does not disclose the limit or the remaining budget, which would only tell a caller how much room they have left. Retry-After rounds the remaining window up rather than down. Telling a caller to come back sooner than the window actually resets just earns another 429. Rejections are not logged separately. requestLogMiddleware already records path, status and client address for every request, so a 429 is visible there without giving a flood a second way to fill the log. The limiter keys on ctx.ClientIP(), which is only trustworthy because of the trusted-proxy configuration that landed immediately before this. Without it a caller would pick their own bucket with an X-Forwarded-For header. Counters are per process, and expired buckets are swept lazily from inside Allow rather than by a goroutine, so there is no background lifetime to own and no unbounded growth. Running several replicas gives each its own budget, weakening the bound by the replica count; config.example.yaml says so explicitly. These limits exist to make flooding expensive, not to meter a quota. Tests: seven for the limiter, including an exact-count check across 16 goroutines making 3200 calls against one key; three for the middleware, covering the 429 shape, per-address isolation and a nil limiter allowing everything; and five end to end against a server built by NewServer, including one that fires 1000 requests at the health, config, org-sync and two channel webhook routes and fails if any of them returns 429. The concurrency test could not be run under -race: this repository builds with CGO disabled and go test -race requires cgo. It still has teeth, because an unguarded concurrent map write panics rather than merely miscounting.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_86c1b6fd-b6c2-4d8a-97b4-83b4ff5eccd7) |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an in-process, fixed-window rate limiting mechanism to protect public, unauthenticated endpoints from abuse. It includes a new ratelimit package, a RateLimit middleware, configuration updates, and comprehensive tests. Feedback focuses on optimizing the rate limiter's memory and CPU usage during floods by recreating the map in sweep instead of using delete, and avoiding unnecessary map allocations in New when the limiter is disabled.
| func (l *Limiter) sweep(now time.Time) { | ||
| if !l.sweptAt.IsZero() && now.Sub(l.sweptAt) < l.window { | ||
| return | ||
| } | ||
| for key, current := range l.buckets { | ||
| if !now.Before(current.resetAt) { | ||
| delete(l.buckets, key) | ||
| } | ||
| } | ||
| l.sweptAt = now | ||
| } |
There was a problem hiding this comment.
In Go, deleting keys from a map using delete does not shrink the underlying bucket array or release its memory. If the server experiences a flood of requests from many unique IP addresses (e.g., during a DDoS or scanning event), the buckets map will grow extremely large. Even after the keys expire and are deleted by sweep, the map will retain its peak size, and subsequent iterations over the map during sweep will still scan all empty buckets, causing CPU spikes on the hot path of Allow.
To prevent memory bloat and slow iterations, we can recreate the map with only the active (non-expired) buckets during the sweep. Since sweep runs at most once per window, this overhead is highly amortized and keeps the map size bounded to only active clients.
| func (l *Limiter) sweep(now time.Time) { | |
| if !l.sweptAt.IsZero() && now.Sub(l.sweptAt) < l.window { | |
| return | |
| } | |
| for key, current := range l.buckets { | |
| if !now.Before(current.resetAt) { | |
| delete(l.buckets, key) | |
| } | |
| } | |
| l.sweptAt = now | |
| } | |
| func (l *Limiter) sweep(now time.Time) { | |
| if !l.sweptAt.IsZero() && now.Sub(l.sweptAt) < l.window { | |
| return | |
| } | |
| active := make(map[string]bucket) | |
| for key, current := range l.buckets { | |
| if now.Before(current.resetAt) { | |
| active[key] = current | |
| } | |
| } | |
| l.buckets = active | |
| l.sweptAt = now | |
| } |
| func New(limit int, window time.Duration) *Limiter { | ||
| if limit <= 0 || window <= 0 { | ||
| return &Limiter{limit: limit, window: window, buckets: make(map[string]bucket)} | ||
| } | ||
| return &Limiter{limit: limit, window: window, buckets: make(map[string]bucket)} | ||
| } |
There was a problem hiding this comment.
The two branches in New are identical and both allocate the buckets map. When limit <= 0 or window <= 0, the limiter is disabled and Allow returns early without ever accessing buckets. We can avoid this unnecessary allocation and simplify the code by only initializing the map when the limiter is actually enabled.
| func New(limit int, window time.Duration) *Limiter { | |
| if limit <= 0 || window <= 0 { | |
| return &Limiter{limit: limit, window: window, buckets: make(map[string]bucket)} | |
| } | |
| return &Limiter{limit: limit, window: window, buckets: make(map[string]bucket)} | |
| } | |
| func New(limit int, window time.Duration) *Limiter { | |
| if limit <= 0 || window <= 0 { | |
| return &Limiter{limit: limit, window: window} | |
| } | |
| return &Limiter{limit: limit, window: window, buckets: make(map[string]bucket)} | |
| } |
1 participant
Closes SEC-08 in
docs/CROVE_DESK_AUDIT.html. Depends on the trusted-proxy work in #13, which is why it comes second: the limiter keys onctx.ClientIP(), and until #13 that value was whatever the caller put inX-Forwarded-For.The gap
There was no rate limiting anywhere in the application. Every unauthenticated endpoint could be called in a tight loop, which made three things cheap: flooding the message upload endpoints with 20 MB bodies, spraying login and support registration, and burning LLM spend through the conversation endpoints.
What is limited
POST /api/auth/loginPOST /api/support/auth/registerPOST /api/customer/session_exchangePOST /api/message/upload_attachment+upload_imagePOST /api/support/doc-page/feedbackWindow is 60 seconds by default;
RATE_LIMIT_ENABLEDandRATE_LIMIT_WINDOW_SECONDSconfigure it. Limits are sized for a human driving a browser, not for a machine.What is deliberately NOT limited
/api/third/*channel webhooks. A platform that receives a 429 from its webhook stops retrying and eventually disables the delivery, which takes an entire channel offline - a far worse outcome than the flood the limit was meant to stop. Those endpoints authenticate by signature instead./api/ws/*websockets, and/api/webhooks/*which are HMAC verified./api/dashboard/*- already behindAuthMiddleware, and staff sharing one office address would throttle each other.TestNewServerLeavesWebhooksAndPublicReadsUnthrottledfires 1000 requests at the health, config, org-sync and two channel webhook routes and fails if any of them returns 429. That exemption is the part most likely to be broken by a future refactor, so it is the part with the loudest test.Response shape
429,
Retry-Afterin whole seconds rounded up, and an ordinaryJsonResultbody.A real 429 rather than the 200-with-error-code that
AuthMiddlewareuses, becauseweb/lib/api/client.tsparses the payload before it inspectsresponse.okand surfacespayload.message- so the localized text still reaches the user, andRetry-Afteris only meaningful on a 429 or 503.web/lib/api/im.tsuses the samerequesthelper, so the widget behaves identically.The message is
error.e0354in both backend locales and does not disclose the limit or the remaining budget, which would only tell a caller how much room they have left.Rejections are not logged separately:
requestLogMiddlewarealready records path, status and client address for every request, so a 429 is visible there without giving a flood a second way to fill the log.Design notes
Allowrather than by a goroutine, so there is no background lifetime to own and no unbounded growth. Running several replicas gives each its own budget, weakening the bound by the replica count.config.example.yamlsays so explicitly rather than leaving an operator to discover it. These limits exist to make flooding expensive, not to meter a quota.newPublicRateLimitsbuilds the limiters once per server inaddRouter, not per request.Tests
Fifteen new: seven for the limiter, three for the middleware, five end to end against a server built by
NewServer.TestLimiterCountsExactlyUnderConcurrencyruns 16 goroutines making 3200 calls against one key with a limit of 1000 and asserts exactly 1000 were allowed.One caveat, stated rather than hidden:
-racecould not be used, because this repository builds with CGO disabled andgo test -racerequires cgo. The concurrency test still has teeth - an unguarded concurrent map write panics rather than merely miscounting - but it is not a substitute for the race detector.Retry-Afterrounding was caught by my own test: the first implementation didint(seconds)+1, which returns 61 for a 60 second window whenever the clock has not ticked between requests. It now rounds up properly.Verification
go build -tags dev ./...andgo vet -tags dev ./...clean.go test -count=1 -tags dev ./internal/...passes across 49 packages - broader than what CI runs, which covers services, repositories, pkg, oidcclient, migration, builders, bootstrap and handlers.Not in this pull request
The upstream port. This is upstream code and belongs in
huabeitech/agent-desktoo, but it cannot be replayed the way the earlier upload fix was:internal/pkg/config/config.gocarries fork-only configuration ondev, andinternal/bootstrap/server.goondevalready contains the storage hardening still sitting in the open upstream PR huabeitech#40. Porting needs the hunks applied to upstream's versions by hand, which is one piece of work covering SEC-26, SEC-09 and SEC-08 together.Note
Medium Risk
Touches hot unauthenticated paths (login, uploads, widget session exchange); misconfigured trusted proxies would weaken or mis-key limits, and per-replica counters only raise the cost of abuse rather than enforcing a global quota.
Overview
Adds per-client-IP rate limiting on abuse-prone public routes, keyed off
ctx.ClientIP()(so it depends on trusted-proxy configuration being correct).What gets limited (separate in-process fixed-window budgets, default 60s window):
POST /api/auth/login, support registration, widgetsession_exchange, shared budget for message image/attachment uploads, and doc-page feedback. Limits are built once at router setup and applied via newmiddleware.RateLimit, which returns HTTP 429 withRetry-After(seconds, rounded up) and localizederror.e0354in the usual JSON envelope.Configuration: new
server.rateLimit/RATE_LIMIT_ENABLEDandRATE_LIMIT_WINDOW_SECONDS(on by default; disabling yields nil limiters so routes stay unchanged). Example env and YAML document that counters are per process (weaker with multiple replicas) and that channel webhooks,/api/webhooks, websockets, and the authenticated dashboard are intentionally exempt so platforms do not get 429s on webhook delivery.Implementation: new
internal/pkg/ratelimitin-memory limiter with lazy bucket sweep; bootstrap integration tests assert throttling, per-IP isolation, configurability, disable path, and that health/config/org-sync and third-party webhooks never return 429.Reviewed by Cursor Bugbot for commit 61832f4. Configure here.