fix(server): isolate pre-auth rate-limit cardinality#61
Conversation
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
✨ 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 |
![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 a new FixedWindowSketchRateLimiter class to handle untrusted, high-cardinality keys using a count-min sketch algorithm, preventing memory exhaustion from API key spray attacks. This pre-auth rate limiter is integrated into the API key authentication middleware and server configuration, with corresponding tests added. The review feedback suggests minor code simplifications in the new rate limiter by removing redundant nullish coalescing operators and fallback checks where array values are guaranteed to be defined.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (row[index] !== 0xffff_ffff) { | ||
| row[index] = (row[index] ?? 0) + 1; | ||
| } |
There was a problem hiding this comment.
Since row is a Uint32Array and index is guaranteed to be within bounds, row[index] will always be a number and never undefined. The nullish coalescing operator ?? 0 is redundant here and can be simplified.
| if (row[index] !== 0xffff_ffff) { | |
| row[index] = (row[index] ?? 0) + 1; | |
| } | |
| if (row[index] !== 0xffff_ffff) { | |
| row[index] += 1; | |
| } |
There was a problem hiding this comment.
Addressed in 0203821: the typed-array counter now increments directly under the validated in-bounds invariant.
| private estimate(key: string): number { | ||
| let estimate = Number.POSITIVE_INFINITY; | ||
| for (let rowIndex = 0; rowIndex < this.rows.length; rowIndex += 1) { | ||
| const row = this.rows[rowIndex]; | ||
| if (!row) { | ||
| continue; | ||
| } | ||
| estimate = Math.min(estimate, row[this.hashIndex(key, rowIndex)] ?? 0); | ||
| } | ||
| return Number.isFinite(estimate) ? estimate : 0; | ||
| } |
There was a problem hiding this comment.
Since this.rows is initialized with a length of depth (which is validated to be >= 1), the loop is guaranteed to run at least once and estimate will always be updated to a finite number from the Uint32Array. Therefore, both the ?? 0 fallback and the Number.isFinite check are redundant and can be simplified.
| private estimate(key: string): number { | |
| let estimate = Number.POSITIVE_INFINITY; | |
| for (let rowIndex = 0; rowIndex < this.rows.length; rowIndex += 1) { | |
| const row = this.rows[rowIndex]; | |
| if (!row) { | |
| continue; | |
| } | |
| estimate = Math.min(estimate, row[this.hashIndex(key, rowIndex)] ?? 0); | |
| } | |
| return Number.isFinite(estimate) ? estimate : 0; | |
| } | |
| private estimate(key: string): number { | |
| let estimate = Number.POSITIVE_INFINITY; | |
| for (let rowIndex = 0; rowIndex < this.rows.length; rowIndex += 1) { | |
| const row = this.rows[rowIndex]; | |
| if (!row) { | |
| continue; | |
| } | |
| estimate = Math.min(estimate, row[this.hashIndex(key, rowIndex)]); | |
| } | |
| return estimate; | |
| } |
There was a problem hiding this comment.
Addressed in 0203821: construction guarantees at least one initialized row, so estimate now returns the loop result directly.
1 participant
Summary
Context
This is the late Cubic P2 follow-up from #59. The operator merged #59 before the final review comment was addressed, so this PR carries only commit
16a0c365rebased onto currentmain. It should land before the real npm publish associated with #60.Validation
16a0c365treeDo not auto-merge; operator verification is required.