fix(security): sanitize api pagination parameters#36
Conversation
Category: Security Priority: P1 What: Added explicit `isNaN` validation and lower bound checks for `limit` and `offset` query parameters across transaction and payment link API endpoints (`/api/v1/payment-links`, `/api/v1/transactions`, `/api/transactions`, `/api/transactions/history`). Why: Passing unvalidated `NaN` or negative values to Supabase `.range()` or `.limit()` caused unhandled errors, crashing the route and creating DoS vulnerability vectors. Impact: Improved API resilience by safely falling back to defaults (limit 10/50, offset 0) and preventing database crashes from malformed inputs. Verification: Evaluated `npm run lint` and `npm run build` which passed successfully. 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 robust input validation for ⚡ Powered by Flash Review · Report Issue |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const { searchParams } = new URL(req.url) | ||
| const limit = Math.min(parseInt(searchParams.get('limit') || '10'), 100) | ||
| const offset = parseInt(searchParams.get('offset') || '0') | ||
| const parsedLimit = parseInt(searchParams.get('limit') || '10') |
There was a problem hiding this comment.
⚡ Flash Review
🚨 Security: The previous logic for limit could result in NaN if a non-numeric string was provided, potentially leading to unexpected database query behavior or errors. This robust input validation, including isNaN checks and clamping limit to a minimum of 1 and maximum of 100, significantly improves the API's resilience against malformed requests.
Fix: No further fix needed, this change correctly addresses the potential issue.
| const { searchParams } = new URL(req.url) | ||
| const paymentLinkId = searchParams.get('payment_link_id') | ||
| const limit = Math.min(parseInt(searchParams.get('limit') || '50'), 100) | ||
| const parsedLimit = parseInt(searchParams.get('limit') || '50') |
There was a problem hiding this comment.
⚡ Flash Review
🚨 Security: While the limit parsing logic is improved to handle NaN and negative values, relying on manual parseInt and isNaN checks for API query parameters can be less robust and consistent than using a dedicated validation library.
Fix: Implement a Zod schema to validate all incoming query parameters for this API route, including payment_link_id and limit. This ensures consistent, declarative validation, better type safety, and prevents unexpected values from reaching the database.
import { z } from 'zod'
const querySchema = z.object({
payment_link_id: z.string().uuid('Invalid payment link ID format'),
limit: z.preprocess(
(val) => parseInt(z.string().parse(val), 10),
z.number().int().min(1).max(100)
).default(50).optional()
})
const { payment_link_id, limit } = querySchema.parse(Object.fromEntries(searchParams))
|
Closing as duplicate of #31 which has been merged. |
1 participant
Category: Security
Priority: P1
What: Added explicit
isNaNvalidation and lower bound checks forlimitandoffsetquery parameters across transaction and payment link API endpoints (/api/v1/payment-links,/api/v1/transactions,/api/transactions,/api/transactions/history).Why: Passing unvalidated
NaNor negative values to Supabase.range()or.limit()caused unhandled errors, crashing the route and creating DoS vulnerability vectors.Impact: Improved API resilience by safely falling back to defaults (limit 10/50, offset 0) and preventing database crashes from malformed inputs.
Verification: Evaluated
npm run lintandnpm run buildwhich passed successfully.PR created automatically by Jules for task 14301886515865622520 started by @Shreyassp002