fix(security): sanitize API pagination parameters#31
Conversation
Fixed pagination security in API routes by safely parsing 'limit' and 'offset' query parameters. Previous implementation parsed them using 'parseInt' without 'isNaN' checks, allowing 'NaN' or negative numbers to be passed to Supabase's '.range()' and '.limit()' functions, risking runtime errors. This fix enforces safe bounds and default values. Co-authored-by: Shreyassp002 <96625037+Shreyassp002@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
⚡ Flash Review
🚨 Critical (must fix before merge)
✅ What's good: The PR correctly identifies and addresses a critical security vulnerability by adding explicit validation for pagination parameters, significantly improving API robustness. ⚡ Powered by Flash Review · Report Issue |
| const { searchParams } = new URL(req.url) | ||
| const limit = Math.min(parseInt(searchParams.get('limit') || '50'), 100) | ||
|
|
||
| const rawLimit = parseInt(searchParams.get('limit') || '50') |
There was a problem hiding this comment.
⚡ Flash Review
🚨 Security: While the manual parsing and clamping of the limit parameter is an improvement, for critical API routes handling real money, all user-provided query parameters should be validated using a robust schema library like Zod. This ensures comprehensive type safety, consistency, and better error handling for invalid inputs, preventing potential edge-case vulnerabilities.
Fix: Define a Zod schema for the query parameters and parse them, e.g., z.object({ limit: z.coerce.number().int().min(1).max(100).default(50) }).
|
|
||
| const rawOffset = parseInt(searchParams.get('offset') || '0') | ||
| const offset = isNaN(rawOffset) || rawOffset < 0 ? 0 : rawOffset | ||
|
|
There was a problem hiding this comment.
⚡ Flash Review
🚨 Security: This added validation for the limit parameter is crucial. Previously, malformed input (e.g., limit=abc) would result in NaN being passed to downstream logic, potentially leading to unexpected database query behavior or errors. The explicit isNaN and bounds checks now ensure limit is always a valid, positive integer, significantly hardening the API against invalid requests and preventing potential service disruption.
Category: Security
Priority: P0
What: Fixed pagination security in API routes (
src/app/api/v1/payment-links/route.ts,src/app/api/v1/transactions/route.ts,src/app/api/transactions/route.ts,src/app/api/transactions/history/route.ts) by properly sanitizinglimitandoffsetquery parameters.Why: Previous implementation used
parseIntwithout checking forNaN. Providing non-numeric or negative values could result inNaNor negative boundaries being passed directly to Supabase.range()and.limit(), causing runtime errors or unexpected behavior.Impact: Prevents potential DoS or runtime database exceptions from maliciously crafted query parameters.
Verification: Ran
npm run lintandNEXT_PUBLIC_WALLET_CONNECT_PROJECT_ID=dummy npm run buildsuccessfully. Verified that default safe boundaries are applied whenNaNor negative values are provided.PR created automatically by Jules for task 6495204142204813755 started by @Shreyassp002