Repository navigation
feat(security): per-IP rate limits on sync/meta/presence POST endpoints - #37
Conversation
- sync had no limit despite being the most expensive route (tx-less calls fan out over every chain); 10/min per IP - meta is unsigned by design, so the IP bucket is the only spam defense; 20/min per IP before validation - presence hashes caller-controlled UA into a DB row per variant; 30/min per IP after the bot short-circuit - write-limits.test.ts pins the wiring (429 shape, limit-before-work order)
|
Warning Review limit reachedNext included review available in 26 minutes. View limit detailsLimit details: You’ve used the included review currently available. This review ran on the open-source allowance, not this organization's plan, because the pull request author doesn't have an assigned seat. Waiting won't change this — ask an organization admin to assign them a seat, or add seats in Billing if every seat is already assigned, then retry. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Comment |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
This is a useful, focused improvement. The limits run before the protected sync, metadata and presence work, and normal presence polling stays comfortably below the threshold.
The five submitted tests passed. Isolated route checks using the actual limiter also confirmed the 10/20/30 limits, next-request 429s before protected work, independent IP buckets and unchanged sync validation. External work was stubbed; this was not a production load test. Current app and contract CI are green.
Approving for the current Fly setup. One non-blocking limitation worth documenting: these counters are per process, not a global limit across machines. Alternate proxy deployments also need their own trusted-client-IP handling.
|
Thanks for the fixes, @Ayush7614, and for working through the earlier feedback. For the next batch, could you prioritize the outstanding review comments and group small changes that solve the same issue? Separate PRs are welcome for independent fixes; linking related work and adding a reproduction and regression test will help us review them faster. |
kevincodex1
left a comment
kevincodex1
left a comment
There was a problem hiding this comment.
looks great! thanks Ayush
3 participants
Three unauthenticated write endpoints had no rate limit while every sibling does: sync is the most expensive route in the app (10/min), meta is unsigned by design so the IP bucket is the only spam defense (20/min), and presence hashes caller-controlled UA into a DB row per variant (30/min after the bot short-circuit). All follow the existing edit/posts 429 slow-down pattern. New write-limits.test.ts pins the wiring; limiter behavior is covered in editServer.test.ts. Verified: full app suite 338/338, lint, typecheck, build.