Repository navigation
sec(api): add "register" rate-limit bucket and fix CSRF prefix match (#1016, #1017) - #1038
Conversation
Issue #1016: "register" was missing from getDefaultRateLimits(), causing checkRateLimit() to silently fall through to api_general (300/min). The public /api/register endpoint writes to the DB and triggers a synchronous admin-notification email per request, making 300/min a viable table-flooding and inbox-spam vector. Add a dedicated "register" bucket matching setup_admin severity (5 per 15 min per IP). Add assertRateLimitKeysKnown(), called from NewHandler with every key used in checkRateLimit call sites. Panics at startup if a key is missing from the config map, catching this class of bug before production traffic hits. Issue #1017: csrfExemptAlways used strings.HasPrefix on "/api/register", which also exempted /api/registrations/<uuid>/approve, /reject, and DELETE (all AuthAdmin, state-changing). Approving a registration creates an enabled cloud account and stores credentials. Split the exempt list into a prefix list for /api/auth/* paths and an exact match for /api/register, mirroring the existing isPublicEndpoint() handling. Regression tests: - TestGetDefaultRateLimits_RegisterHasDedicatedBucket - TestAssertRateLimitKeysKnown_PanicsOnUnknownKey - TestAssertRateLimitKeysKnown_NoopForKnownKeys - TestRequiresCSRFValidation_RegistrationsAdminRoutesAreNotExempt Closes #1016, Closes #1017
cristim
added
triaged
|
Warning Review limit reached
More reviews will be available in 32 minutes and 58 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughAdds a dedicated 5/15min rate-limit bucket for ChangesAPI Rate Limiting and CSRF Security Hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/api/inmemory_rate_limiter_test.go (1)
269-288: ⚡ Quick winLock the register regression test to the exact security contract.
At Line 279-287, this test only checks “stricter than
api_general” and non-zero window. A weak value like299/minwould still pass. Assert the exact5 attempts / 15 minutescontract from#1016.Proposed diff
func TestGetDefaultRateLimits_RegisterHasDedicatedBucket(t *testing.T) { limits := getDefaultRateLimits() cfg, ok := limits["register"] if !ok { t.Fatal("register key missing from default rate limits (issue `#1016`): add it to getDefaultRateLimits()") } + // Contract from `#1016`: 5 attempts per 15 minutes. + assert.Equal(t, 5, cfg.MaxAttempts, "register max attempts must be 5 per issue `#1016`") + assert.Equal(t, 15*time.Minute, cfg.Window, "register window must be 15 minutes per issue `#1016`") + // register must be far stricter than api_general (300/min). apiGeneral := limits["api_general"] if cfg.MaxAttempts >= apiGeneral.MaxAttempts { t.Errorf("register limit (%d) is not stricter than api_general (%d); regression of `#1016`", cfg.MaxAttempts, apiGeneral.MaxAttempts) } // Window must be non-zero. if cfg.Window == 0 { t.Error("register rate limit has zero window duration") } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/inmemory_rate_limiter_test.go` around lines 269 - 288, The test TestGetDefaultRateLimits_RegisterHasDedicatedBucket is too loose; update it to assert the exact security contract from `#1016` by checking the "register" entry returned by getDefaultRateLimits() has MaxAttempts == 5 and Window == 15*time.Minute (instead of only being stricter than api_general and non-zero). Locate the "register" cfg in the test and replace/add assertions that compare cfg.MaxAttempts to 5 and cfg.Window to 15*time.Minute so the test fails on any regression in getDefaultRateLimits().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/api/inmemory_rate_limiter_test.go`:
- Around line 269-288: The test
TestGetDefaultRateLimits_RegisterHasDedicatedBucket is too loose; update it to
assert the exact security contract from `#1016` by checking the "register" entry
returned by getDefaultRateLimits() has MaxAttempts == 5 and Window ==
15*time.Minute (instead of only being stricter than api_general and non-zero).
Locate the "register" cfg in the test and replace/add assertions that compare
cfg.MaxAttempts to 5 and cfg.Window to 15*time.Minute so the test fails on any
regression in getDefaultRateLimits().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d26d6e80-1a51-4e5e-92ea-8b2e415d3b79
📒 Files selected for processing (5)
internal/api/handler.gointernal/api/inmemory_rate_limiter_test.gointernal/api/middleware.gointernal/api/middleware_test.gointernal/api/rate_limiter.go
Lock the register regression test to the precise security contract from issue #1016 instead of only checking "stricter than api_general" and a non-zero window. A weak value like 299/min would have passed the old assertions while still allowing registration flooding. Assert the exact 5 attempts / 15 minutes contract so any future relaxation is caught. Addresses CodeRabbit nitpick on PR #1038.
|
Addressed CR review findings:
@coderabbitai review |
|
🧠 Learnings used✅ Action performedReview finished.
|
…L1, 02-L4) 02-L1: update the forgot_password handler comment from "3 attempts per email per hour" to match the actual config (10 attempts / 5 minutes in getDefaultRateLimits). 02-L4: health.go checkConfigStore exposed err.Error() in the public /health response body (AuthPublic route). Log the detail server-side only and return a static "config store unavailable" message to callers. Update tests to assert the error string does not appear in the body. Closes #1064
…sions (02-N1, 02-N2) 02-N1: remove legacy RateLimiter/newRateLimiter/rateLimitEntry from types.go and rate_limiter.go. Production paths wire InMemoryRateLimiter or DBRateLimiter (both implement RateLimiterInterface). The dead code carried the same unbounded-GC bug as 02-M3 and was a maintenance trap. Remove matching tests; redirect rate_limiter_test.go to a comment noting where the config-function tests now live. 02-N2: change toAPIPermissions from []any (silent-drop type-assertion) to []Permission (typed, total). Replace the apologetic "in production this would use proper conversion" comment with a clear copy-contract doc. Update tests: replace the now-impossible NonPermissionItems test with a ReturnsCopy test that asserts the mutation-isolation contract. Closes #1064
…on (02-M3) The previous GC only removed already-expired entries above 1000 keys, allowing an attacker rotating source IPs to grow the map without bound (expired entries are harmless, so an adversary is never pruned). The fix lowers the hard cap to 500, evicts all expired entries first, then evicts the oldest live entries (by resetTime) down to 80% of cap if the map is still at or above the cap. Add a regression test (TestInMemoryRateLimiter_EntriesCapEnforced) that injects cap+2 unique IPs, confirms that one eviction cycle fires and the final map is strictly below cap. Previously this test would reach cap+2. Closes #1064
The existing opportunistic cleanup path (triggered when count==1) never fires for perpetually-denied keys: under sustained abuse the abusing key's count keeps climbing without ever resetting to 1, so its row escapes the 24-hour eviction window indefinitely. Add StartCleanupWorker(ctx) which launches a background goroutine that runs cleanup() on a fixed schedule (default 10 minutes), independent of traffic. Wire it in server/app.go immediately after NewDBRateLimiter so Lambda deployments benefit on startup. The integer width of count is Go's default int (64-bit on all supported platforms), so unbounded growth is cosmetically undesirable but not an overflow risk. Add TestDBRateLimiter_StartCleanupWorker_StopsOnContextCancel: overrides the ticker interval to 5ms, confirms the worker exits cleanly on cancel. Closes #1064
The global checkRateLimit swallows limiter errors and allows requests through (fail-open). For data/general endpoints this is intentional, but for the credential brute-force surface (login, setup_admin, reset_password, change_password) it means any DB pressure that causes the rate limiter to error silently disables protection. An attacker who can induce DB pressure also unlocks unlimited password guessing. Add checkRateLimitStrict (fail-closed): returns 503 on limiter error with a high-severity ALERT log so monitoring can detect and alert on the fail-closed window. Switch login, setup_admin, reset_password (both sites), and change_password to checkRateLimitStrict. For forgot_password (email-keyed, inline call): emit an ALERT log on error but keep fail-open because failing closed here would return 503 vs 200, leaking infrastructure state to an enumerating attacker; the enumeration risk is the higher concern. For createAPIKey (authenticated, user-keyed): upgrade the warn log to an error ALERT so the fail-open window is observable, but keep fail-open because the endpoint is behind requirePermission. Add three regression tests: - TestHandler_checkRateLimitStrict_FailsClosedOnError: 503 on db error - TestHandler_checkRateLimit_FailsOpenOnError: nil (allow) on db error - TestHandler_checkRateLimitStrict_Returns429WhenDenied: normal deny path Closes #1064
|
Added 5 commits for FOLD-1038 bucket (report 02, Med/Low/Nit, closes #1064): 02-L1 ( 02-L4 ( 02-N1, 02-N2 ( 02-M3 ( 02-M2 ( 02-M1 ( Verification: @coderabbitai review |
|
🧠 Learnings used✅ Action performedReview finished.
|
Allow in InMemoryRateLimiter had cyclomatic complexity 13, over the pre-commit gocyclo threshold of 10. Extract two helpers: - evictIfAtCap: cap-guard + expired-entry purge - evictOldest: oldest-first eviction down to 80% of cap Allow drops to CC=5; helpers are CC=5 and CC=4. No logic change. Also fix gofmt on rate_limiter.go (trailing blank line) and pkg/exchange/reshape_crossfamily_test.go (alignment in struct literals).
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
"register"was absent fromgetDefaultRateLimits(), socheckRateLimit(ctx, req, "register")silently fell through toapi_general(300/min). At that rate an attacker can bulk-create pending-registration rows (each triggers a DB write + synchronous admin-notification email fan-out). Fixed by adding a dedicated"register"bucket at 5 req/15 min/IP (matchingsetup_adminseverity). AddedassertRateLimitKeysKnown(), called fromNewHandler, that panics at startup if any key used in acheckRateLimitcall is absent from the config map -- catches this class of bug before production traffic.csrfExemptAlwaysinrequiresCSRFValidationmatched"/api/register"withstrings.HasPrefix, which also exemptedPOST /api/registrations/<id>/approve,/reject, andDELETE /api/registrations/<id>(allAuthAdmin, all state-changing). Approving a registration creates an enabled cloud account and stores credentials. Fixed by splitting the exempt list into a prefix slice for/api/auth/*paths and an exactpath == "/api/register"check, mirroring the existingisPublicEndpointlogic.Files changed
internal/api/rate_limiter.go: add"register"bucket; addassertRateLimitKeysKnown()internal/api/handler.go: callassertRateLimitKeysKnown(...)fromNewHandlerwith all active key stringsinternal/api/middleware.go: splitcsrfExemptAlways-- prefix list for/api/auth/*, exact match for/api/registerinternal/api/inmemory_rate_limiter_test.go: 3 regression tests for sec(api): public /api/register has no rate limit ("register" key missing → 300/min fallback) #1016internal/api/middleware_test.go: 1 regression test for sec(api): CSRF exemption uses HasPrefix — /api/register exempts /api/registrations/*/approve|reject|delete #1017Test plan
go build ./...passes (confirmed)go vet ./internal/api/...clean (confirmed)go test ./internal/api/...1496 tests pass (confirmed)TestGetDefaultRateLimits_RegisterHasDedicatedBucket-- asserts"register"key exists and is stricter thanapi_generalTestAssertRateLimitKeysKnown_PanicsOnUnknownKey-- asserts startup assertion fires on missing keyTestAssertRateLimitKeysKnown_NoopForKnownKeys-- asserts assertion is silent for known keysTestRequiresCSRFValidation_RegistrationsAdminRoutesAreNotExempt-- asserts all three admin/api/registrations/*routes require CSRFTestRequiresCSRFValidation_AlwaysExemptPaths-- asserts/api/registerexact path remains exemptCloses #1016
Closes #1017
Scope expanded
Added 7 remaining findings from FOLD-1038 bucket (report 02 Medium/Low/Nit) as additional commits.
Commits added
c33654ad4796dc7a512d691b7413aa4550e1130bd15ccAdditional test plan
go build ./...cleango vet ./internal/api/...cleango test ./internal/api/...1494 tests passTestInMemoryRateLimiter_EntriesCapEnforced-- cap+2 IPs, map stays below cap after evictionTestDBRateLimiter_StartCleanupWorker_StopsOnContextCancel-- worker exits on cancelTestHandler_checkRateLimitStrict_FailsClosedOnError-- 503 on limiter errorTestHandler_checkRateLimit_FailsOpenOnError-- nil (allow) on limiter errorTestToAPIPermissions_ReturnsCopy-- mutation isolation after typed conversionAlso closes #1064