fix: cap API pagination bounds to prevent potential DoS#1066
fix: cap API pagination bounds to prevent potential DoS#1066aniruddhaadak80 wants to merge 1 commit intof:mainfrom
Conversation
📝 WalkthroughWalkthroughThree API routes now enforce pagination parameter bounds, replacing direct parsing with clamped values: page is constrained to at least 1, and limit/perPage to between 1 and 100 maximum. Comments explain the caps prevent database overload. Changes
Possibly related issues
Poem
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
Pull request overview
This PR adds upper bounds to API pagination parameters to prevent excessive database reads (potential DoS) by capping perPage/limit at 100 and ensuring page is at least 1 across public and admin list endpoints.
Changes:
- Clamp public prompts pagination (
page,perPage) to safe ranges (min 1, max 100 forperPage). - Clamp admin users pagination (
page,limit) to safe ranges (min 1, max 100 forlimit). - Clamp admin prompts pagination (
page,limit) to safe ranges (min 1, max 100 forlimit).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/app/api/prompts/route.ts | Adds clamped page/perPage before Prisma skip/take on the public prompts list endpoint. |
| src/app/api/admin/users/route.ts | Adds clamped page/limit before computing skip/take on the admin users list endpoint. |
| src/app/api/admin/prompts/route.ts | Adds clamped page/limit before computing skip/take on the admin prompts list endpoint. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const requestedPage = parseInt(searchParams.get("page") || "1"); | ||
| const requestedPerPage = parseInt(searchParams.get("perPage") || "24"); | ||
|
|
||
| // Cap pagination to prevent database overload | ||
| const page = Math.max(1, requestedPage); | ||
| const perPage = Math.min(Math.max(1, requestedPerPage), 100); |
There was a problem hiding this comment.
parseInt(...) can return NaN for inputs like ?page=abc or ?perPage=. Math.max(1, NaN) / Math.min(Math.max(1, NaN), 100) will still be NaN, which then makes skip/take invalid for Prisma and can also produce totalPages: NaN. Consider parsing with radix 10 and falling back to defaults when the parsed value is not a finite integer (e.g., Number.isFinite(...) ? ... : default).
| const requestedPage = parseInt(searchParams.get("page") || "1"); | ||
| const requestedLimit = parseInt(searchParams.get("limit") || "20"); | ||
|
|
||
| // Cap pagination to prevent database overload | ||
| const page = Math.max(1, requestedPage); | ||
| const limit = Math.min(Math.max(1, requestedLimit), 100); |
There was a problem hiding this comment.
parseInt(...) may return NaN (e.g. ?page=abc), and Math.max(1, NaN) / the later validPage/validLimit calculations will still be NaN. That would make skip/take invalid for Prisma. Consider falling back to defaults when the parsed value is not a finite integer (and optionally pass radix 10 to parseInt).
| const requestedPage = parseInt(searchParams.get("page") || "1"); | ||
| const requestedLimit = parseInt(searchParams.get("limit") || "20"); | ||
|
|
||
| // Cap pagination to prevent database overload | ||
| const page = Math.max(1, requestedPage); | ||
| const limit = Math.min(Math.max(1, requestedLimit), 100); |
There was a problem hiding this comment.
Same parsing issue as the users admin route: parseInt(...) can yield NaN, and Math.max(1, NaN) keeps it NaN, which then flows into skip/take. Please add a NaN/non-finite fallback to the default page/limit values (and consider parseInt(..., 10)).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/api/admin/prompts/route.ts (1)
27-41: 🛠️ Refactor suggestion | 🟠 MajorRemove redundant double-clamping of pagination values.
The clamping logic is applied twice: first at lines 31-32 (
page/limit), then again at lines 39-40 (validPage/validLimit). Sincepageandlimitare already clamped, the second pass is redundant and adds unnecessary complexity.Additionally,
parseInt()returnsNaNfor non-numeric input, which propagates throughMath.max/Math.min.♻️ Proposed fix to remove redundancy and handle NaN
const { searchParams } = new URL(request.url); - const requestedPage = parseInt(searchParams.get("page") || "1"); - const requestedLimit = parseInt(searchParams.get("limit") || "20"); - - // Cap pagination to prevent database overload - const page = Math.max(1, requestedPage); - const limit = Math.min(Math.max(1, requestedLimit), 100); + const requestedPage = parseInt(searchParams.get("page") || "1") || 1; + const requestedLimit = parseInt(searchParams.get("limit") || "20") || 20; const search = searchParams.get("search") || ""; const sortBy = searchParams.get("sortBy") || "createdAt"; const sortOrder = searchParams.get("sortOrder") || "desc"; const filter = searchParams.get("filter") || "all"; - // Validate pagination - const validPage = Math.max(1, page); - const validLimit = Math.min(Math.max(1, limit), 100); + // Cap pagination to prevent database overload + const validPage = Math.max(1, requestedPage); + const validLimit = Math.min(Math.max(1, requestedLimit), 100); const skip = (validPage - 1) * validLimit;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/admin/prompts/route.ts` around lines 27 - 41, The pagination values are being clamped twice and parseInt can produce NaN; remove the redundant second clamp (delete/stop using validPage and validLimit) and compute a single sanitized page/limit using requestedPage/requestedLimit: parse as integers, if Number.isNaN(...) substitute defaults (1 for page, 20 for limit), then clamp once with Math.max/Math.min to enforce bounds, set skip = (page - 1) * limit; update any references that used validPage/validLimit to use page/limit instead (look for symbols requestedPage, requestedLimit, page, limit, validPage, validLimit, skip).src/app/api/admin/users/route.ts (1)
27-41: 🛠️ Refactor suggestion | 🟠 MajorRemove redundant double-clamping of pagination values (same issue as admin/prompts).
Identical to
src/app/api/admin/prompts/route.ts: lines 31-32 clamp topage/limit, then lines 39-40 re-clamp tovalidPage/validLimit. The second pass is redundant. Also,parseInt()can returnNaNfor invalid input.♻️ Proposed fix to remove redundancy and handle NaN
const { searchParams } = new URL(request.url); - const requestedPage = parseInt(searchParams.get("page") || "1"); - const requestedLimit = parseInt(searchParams.get("limit") || "20"); - - // Cap pagination to prevent database overload - const page = Math.max(1, requestedPage); - const limit = Math.min(Math.max(1, requestedLimit), 100); + const requestedPage = parseInt(searchParams.get("page") || "1") || 1; + const requestedLimit = parseInt(searchParams.get("limit") || "20") || 20; const search = searchParams.get("search") || ""; const sortBy = searchParams.get("sortBy") || "createdAt"; const sortOrder = searchParams.get("sortOrder") || "desc"; const filter = searchParams.get("filter") || "all"; - // Validate pagination - const validPage = Math.max(1, page); - const validLimit = Math.min(Math.max(1, limit), 100); + // Cap pagination to prevent database overload + const validPage = Math.max(1, requestedPage); + const validLimit = Math.min(Math.max(1, requestedLimit), 100); const skip = (validPage - 1) * validLimit;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/admin/users/route.ts` around lines 27 - 41, The code double-clamps pagination: you parse requestedPage/requestedLimit, clamp into page/limit, then re-clamp into validPage/validLimit — remove the redundant second clamp and compute skip from the already-clamped values; also guard against parseInt returning NaN by converting/parsing then replacing NaN with the defaults before clamping (use Number.isNaN to detect NaN). Update references: requestedPage, requestedLimit, page, limit, and skip (remove validPage/validLimit and their use).
🧹 Nitpick comments (1)
src/app/api/admin/users/route.ts (1)
27-32: Consider extracting pagination clamping to a shared utility.The pagination clamping pattern is now duplicated across three routes (
/api/prompts,/api/admin/prompts,/api/admin/users). Extracting this to a shared helper would improve maintainability and ensure consistent behavior.💡 Example shared utility
Create a utility in
src/lib/pagination.ts:export interface PaginationParams { page: number; limit: number; } export function parsePagination( searchParams: URLSearchParams, defaults: { page?: number; limit?: number } = {} ): PaginationParams { const { page: defaultPage = 1, limit: defaultLimit = 20 } = defaults; const requestedPage = parseInt(searchParams.get("page") || String(defaultPage)) || defaultPage; const requestedLimit = parseInt(searchParams.get("limit") || searchParams.get("perPage") || String(defaultLimit)) || defaultLimit; return { page: Math.max(1, requestedPage), limit: Math.min(Math.max(1, requestedLimit), 100), }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/admin/users/route.ts` around lines 27 - 32, Extract the repeated pagination clamping into a shared helper (e.g., parsePagination) and replace the inline logic that computes requestedPage/requestedLimit and the clamped page/limit with a single call to that helper; implement a parsePagination(searchParams, defaults?) function that returns { page, limit } (use names PaginationParams and parsePagination to locate/implement it) and ensure it handles parsing defaults, NaN fallback, page >= 1 and limit between 1 and 100, then update usages in functions that currently set page and limit to call parsePagination instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/api/prompts/route.ts`:
- Around line 312-317: The current parsing of pagination params can yield NaN
(e.g., parseInt("abc")), which breaks the Math.max/Math.min logic and downstream
calculations; update the logic around requestedPage/requestedPerPage to detect
NaN and fall back to safe defaults before applying caps: after obtaining
requestedPage and requestedPerPage, coerce them to valid numbers (e.g., use
Number.isNaN or isNaN checks) and set page = Math.max(1, isNaN(requestedPage) ?
1 : requestedPage) and perPage = Math.min(Math.max(1, isNaN(requestedPerPage) ?
24 : requestedPerPage), 100) so (page - 1) * perPage is always a valid number.
Ensure you modify the variables named requestedPage, requestedPerPage, page, and
perPage in route.ts.
---
Outside diff comments:
In `@src/app/api/admin/prompts/route.ts`:
- Around line 27-41: The pagination values are being clamped twice and parseInt
can produce NaN; remove the redundant second clamp (delete/stop using validPage
and validLimit) and compute a single sanitized page/limit using
requestedPage/requestedLimit: parse as integers, if Number.isNaN(...) substitute
defaults (1 for page, 20 for limit), then clamp once with Math.max/Math.min to
enforce bounds, set skip = (page - 1) * limit; update any references that used
validPage/validLimit to use page/limit instead (look for symbols requestedPage,
requestedLimit, page, limit, validPage, validLimit, skip).
In `@src/app/api/admin/users/route.ts`:
- Around line 27-41: The code double-clamps pagination: you parse
requestedPage/requestedLimit, clamp into page/limit, then re-clamp into
validPage/validLimit — remove the redundant second clamp and compute skip from
the already-clamped values; also guard against parseInt returning NaN by
converting/parsing then replacing NaN with the defaults before clamping (use
Number.isNaN to detect NaN). Update references: requestedPage, requestedLimit,
page, limit, and skip (remove validPage/validLimit and their use).
---
Nitpick comments:
In `@src/app/api/admin/users/route.ts`:
- Around line 27-32: Extract the repeated pagination clamping into a shared
helper (e.g., parsePagination) and replace the inline logic that computes
requestedPage/requestedLimit and the clamped page/limit with a single call to
that helper; implement a parsePagination(searchParams, defaults?) function that
returns { page, limit } (use names PaginationParams and parsePagination to
locate/implement it) and ensure it handles parsing defaults, NaN fallback, page
>= 1 and limit between 1 and 100, then update usages in functions that currently
set page and limit to call parsePagination instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8fdd8954-ee75-43c5-a694-ba9ebff7dc6a
📒 Files selected for processing (3)
src/app/api/admin/prompts/route.tssrc/app/api/admin/users/route.tssrc/app/api/prompts/route.ts
| const requestedPage = parseInt(searchParams.get("page") || "1"); | ||
| const requestedPerPage = parseInt(searchParams.get("perPage") || "24"); | ||
|
|
||
| // Cap pagination to prevent database overload | ||
| const page = Math.max(1, requestedPage); | ||
| const perPage = Math.min(Math.max(1, requestedPerPage), 100); |
There was a problem hiding this comment.
Handle NaN from invalid pagination input.
parseInt() returns NaN for non-numeric strings (e.g., ?page=abc). Since Math.max(1, NaN) and Math.min(NaN, 100) both return NaN, the subsequent skip calculation (page - 1) * perPage will produce NaN, which Prisma may reject or handle unexpectedly.
🛡️ Proposed fix to handle NaN
- const requestedPage = parseInt(searchParams.get("page") || "1");
- const requestedPerPage = parseInt(searchParams.get("perPage") || "24");
+ const requestedPage = parseInt(searchParams.get("page") || "1") || 1;
+ const requestedPerPage = parseInt(searchParams.get("perPage") || "24") || 24;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/api/prompts/route.ts` around lines 312 - 317, The current parsing of
pagination params can yield NaN (e.g., parseInt("abc")), which breaks the
Math.max/Math.min logic and downstream calculations; update the logic around
requestedPage/requestedPerPage to detect NaN and fall back to safe defaults
before applying caps: after obtaining requestedPage and requestedPerPage, coerce
them to valid numbers (e.g., use Number.isNaN or isNaN checks) and set page =
Math.max(1, isNaN(requestedPage) ? 1 : requestedPage) and perPage =
Math.min(Math.max(1, isNaN(requestedPerPage) ? 24 : requestedPerPage), 100) so
(page - 1) * perPage is always a valid number. Ensure you modify the variables
named requestedPage, requestedPerPage, page, and perPage in route.ts.
Description
Currently, the page and limit/perPage parameters are parsed directly from search parameters and passed to Prisma without any maximum constraint. A malicious actor could request perPage=1000000, causing large database payload extraction and potential DoS.
This PR implements a strict maximum 100 results per page threshold and ensures the requested page defaults sensibly across the public prompts list and the admin routes.
Changes
Summary by CodeRabbit