Rate limit register and unregister - #29
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds a separate per-IP limiter for ChangesRegister Endpoint Rate Limiting
Estimated code review effort: 3 (Moderate) | ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
AndreaDiazCorreia
left a comment
There was a problem hiding this comment.
Nice work, the implementation looks solid and covers the acceptance criteria from #7. Just one thing before merging: could you update CLAUDE.md to mention that /api/register and /api/unregister can now return 429 with the shared rate_limited_response body? Same style as the 403 whitelist exception that's already documented there. The absolute upper bound on TokenStore from the issue is a different threat model (botnet/multi-IP), so probably better as a separate follow-up. Otherwise LGTM.
|
@juanfradb Are you still working on this? |
|
Keeping this one open. Unlike the other four in the batch, there is no code blocker here: the implementation is correct, it merges cleanly on current |
Constraint 3 declares the /api/register and /api/unregister bodies byte-identical to the pre-1.1 fixtures. The per-IP limiter introduces two new response shapes on those endpoints: 429 with the shared rate_limited_response body, and the 500 fail-closed path when the per-IP key cannot be extracted.
Summary
Security validation
Source/sink/control: unauthenticated /api/register and /api/unregister could be called repeatedly from one client IP and churn or grow the in-memory TokenStore without an HTTP rate limit. Both routes now share a per-IP limiter with 120/min and burst 100, while /api/health, /api/info, and /api/status remain unwrapped.
Original issue no longer reproduces:
Refs #7
Tests
Summary by CodeRabbit
New Features
/api/registerand/api/unregister(120 requests/minute, burst 100), returning HTTP 429 with retry guidance when limits are exceeded.Documentation