fix: cap admin per_page at 500#445
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds pagination request validation to the gotrue API. The implementation introduces a Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/pagination.go (1)
61-68:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReject
per_page=0to avoid divide-by-zero downstream.Line 62 accepts
per_page=0, which can flow intocalculateTotalPages(perPage, ...)and panic on division. Please enforce a lower bound together with the new max check.Suggested fix
if queryPerPage != "" { perPage, err = strconv.ParseUint(queryPerPage, 10, 64) if err != nil { return nil, err } - if perPage > maxPerPage { - return nil, badRequestError("per_page must not exceed %d", maxPerPage) + if perPage == 0 || perPage > maxPerPage { + return nil, badRequestError("per_page must be between 1 and %d", maxPerPage) } }🤖 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 `@api/pagination.go` around lines 61 - 68, The handler currently accepts queryPerPage values including 0 which leads to division-by-zero in calculateTotalPages; after parsing queryPerPage into perPage (using strconv.ParseUint) add a lower-bound check: if perPage == 0 return badRequestError("per_page must be at least 1") (and keep the existing perPage > maxPerPage check), ensuring calculateTotalPages never receives a zero divisor.
🧹 Nitpick comments (1)
api/pagination_test.go (1)
20-27: ⚡ Quick winAdd a
per_page=0error test case.Please add a table row for
per_page=0expecting an error, so the lower-bound contract is explicitly covered.Suggested test row
{"per_page at max", fmt.Sprintf("per_page=%d", maxPerPage), 1, maxPerPage, false}, + {"per_page zero", "per_page=0", 0, 0, true}, {"per_page above max", fmt.Sprintf("per_page=%d", maxPerPage+1), 0, 0, true},🤖 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 `@api/pagination_test.go` around lines 20 - 27, Add a test row to the pagination test table in api/pagination_test.go to cover the lower-bound contract: insert an entry like {"per_page zero", "per_page=0", 0, 0, true} alongside the other cases so the parser/validator for per_page rejects 0; reference the same test table where entries use defaultPerPage and maxPerPage to ensure consistency with existing expectations.
🤖 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.
Outside diff comments:
In `@api/pagination.go`:
- Around line 61-68: The handler currently accepts queryPerPage values including
0 which leads to division-by-zero in calculateTotalPages; after parsing
queryPerPage into perPage (using strconv.ParseUint) add a lower-bound check: if
perPage == 0 return badRequestError("per_page must be at least 1") (and keep the
existing perPage > maxPerPage check), ensuring calculateTotalPages never
receives a zero divisor.
---
Nitpick comments:
In `@api/pagination_test.go`:
- Around line 20-27: Add a test row to the pagination test table in
api/pagination_test.go to cover the lower-bound contract: insert an entry like
{"per_page zero", "per_page=0", 0, 0, true} alongside the other cases so the
parser/validator for per_page rejects 0; reference the same test table where
entries use defaultPerPage and maxPerPage to ensure consistency with existing
expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9573baff-ee80-4ee3-b4f6-475cfe8ac0d1
📒 Files selected for processing (2)
api/pagination.goapi/pagination_test.go
2 participants
- Summary
per_pageon/admin/userswasn't bounded, so a caller could ask for999999999records and pull the whole user table in one query. This caps it at 500 and returns a 400 if you go over, so misbehaving clients fail loudly instead of silently getting partial pages.- Test plan
- Description for the changelog
Cap
/admin/usersper_pagequery parameter at 500.- A picture of a cute animal (not mandatory but encouraged)