Repository navigation
Give token refresh its own rate limit - #136
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (9)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe refresh endpoint now has a dedicated per-IP limit of 60 requests per minute, separate from the login limit. The limiter is wired into the refresh route and covered by limiter and handler tests. ChangesRefresh rate limiting
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Client
participant RefreshRoute
participant handleRefreshToken
participant RefreshRateLimiter
Client->>RefreshRoute: POST /api/v1/auth/refresh
RefreshRoute->>handleRefreshToken: dispatch request
handleRefreshToken->>RefreshRateLimiter: Allow(ip)
Suggested reviewers: Merge Risk: ⚪ Minimal · up to Refresh requests have their own rate-limit budget without consuming the login budget. No merge-blocking issue is established by the supplied evidence. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change preserves login protections and refresh-token validation while preventing refresh bursts from exhausting login allowances. No introduced authentication bypass was identified, but deployment-wide capacity for the higher refresh allowance remains unverified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 58.62% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 29 functions across 10 files. (2 skipped: 2 unsupported.)
✨ 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 |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code |
|
Thanks, Divesh. This is good on the merits. A separate refresh budget keyed on the same Now that #135 has landed on
Once it's mergeable it's good to go. No re-review needed unless the resolution changes behavior. |
5693c47 to
0310eec
Compare
Code review (re-review)Earlier blockers:
No issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code |
aaronbrethorst
left a comment
aaronbrethorst
left a comment
There was a problem hiding this comment.
Giving refresh its own per-IP budget is the right call. A refresh token can't be guessed, so this costs login nothing, and the new mux test proves a depot that uses up its refresh budget can still log in. Thanks for carrying #135's docs through the rebase.
2 participants
/api/v1/auth/refresh shared login's per-IP limiter, 10 attempts a minute. That is harmless while access tokens last 24h, but not at 15m, which is where ACCESS_TOKEN_TTL goes once the Android app refreshes on 401. Drivers who log in at the same pull-out get tokens that expire at the same moment, so a depot's drivers would all refresh within seconds of each other, and they usually share one public IP (depot WiFi or carrier NAT). With 50 drivers that is 50 refreshes in one window against a budget of 10, and because the budget was shared, logins from that depot would be blocked for the same minute.
The old comments called sharing deliberate, so that an address gets one allowance across both endpoints instead of one each. That defends against credential guessing, and refresh tokens can't be guessed: they are 32 random bytes, 256 bits. A separate refresh budget gives an attacker nothing to use against login, and login's budget is unchanged. What a refresh limit actually guards against is flooding, and that needs a generous cap, not login's tight one.
RefreshRateLimiter follows RegistrationRateLimiter: one per-IP map, built on allowInWindow, fails closed at capacity, and a cleanup goroutine that main stops on shutdown. That makes three limiters with the same shape. Extracting a shared per-IP limiter would make a good follow-up, but doing it here would also refactor the rider code.
refreshIPLimit is 60 a minute, which covers a 50-driver depot's burst with about 20% left for retries and still caps a flood from one IP. A bigger depot behind one IP would still hit it. It's a constant like loginIPLimit and registrationIPLimit, and an env var can come when an agency needs one. Jittering token expiry would spread the burst out at the source, but that changes how tokens are issued, so I left it out.
TestRefresh_DoesNotSpendLoginBudget goes through the real newMux. It uses up an IP's refresh budget, then logs in from the same IP and expects a 200. Wiring loginLimiter back into the refresh route makes it fail with a 429, and leaving the refresh route unlimited fails its precondition. I deleted the test that pinned the shared budget, since it asserted the behavior this PR reverses.
newMux and newHandler gained a parameter, so 15 test call sites changed, three of them in map_handlers_test.go from #138. go build skips _test.go files and didn't catch them; go vet did. The second commit deletes LoginRateLimiter.AllowIP, which nothing calls anymore.
#135 landed the other half of #118 (revoking tokens on replay), so this PR completes the issue. It's rebased onto main, keeping #135's handler doc comment and 500 response text.
go build, vet, fmt and test are clean, and with DATABASE_URL set the store and refresh tests ran (136 passed, 0 skipped).
Fixes #118
Summary by CodeRabbit
New Features
Documentation