fix(api): add limit/offset pagination to GET workflows#3443
fix(api): add limit/offset pagination to GET workflows#3443Siddhartha-singh01 wants to merge 1 commit intosimstudioai:mainfrom
Conversation
PR SummaryLow Risk Overview The workflow listing logic was refactored to build a shared Written by Cursor Bugbot for commit faf058b. This will update automatically on new commits. Configure here. |
|
@Siddhartha-singh01 is attempting to deploy a commit to the Sim Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR adds page/limit query-parameter pagination to the Key observations:
Confidence Score: 2/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Client: GET /api/workflows] --> B[Parse page + limit params]
B --> C{workspaceId present?}
C -- Yes --> D[Verify workspace exists]
D --> E[Verify user membership]
E --> F[Build query for single workspace]
C -- No --> G[Fetch permitted workspace IDs]
G --> H{Any workspaceIds?}
H -- No --> I[Return empty array]
H -- Yes --> J[Build query for all permitted workspaces]
F --> K{limit param valid?}
J --> K
K -- Yes --> L[Apply .limit]
L --> M{page param > 1?}
M -- Yes --> N[Apply .offset]
M -- No --> O[Execute query]
N --> O
K -- No --> O
O --> P[Return paginated workflows]
Last reviewed commit: faf058b |
| if (limit && !isNaN(limit) && limit > 0) { | ||
| baseQuery = baseQuery.limit(limit) | ||
| if (page && !isNaN(page) && page > 1) { | ||
| baseQuery = baseQuery.offset((page - 1) * limit) | ||
| } | ||
| } |
There was a problem hiding this comment.
No upper bound on limit undermines the fix
The entire motivation for this PR is to prevent OOM crashes from unbounded queries. However, because there is no cap on the limit value, an authenticated caller can still pass ?limit=999999999 and fetch the entire table in a single request — the original vulnerability is not fully closed.
A maximum limit (e.g. 100 or 500) should be enforced before the query is built:
| if (limit && !isNaN(limit) && limit > 0) { | |
| baseQuery = baseQuery.limit(limit) | |
| if (page && !isNaN(page) && page > 1) { | |
| baseQuery = baseQuery.offset((page - 1) * limit) | |
| } | |
| } | |
| if (limit && !isNaN(limit) && limit > 0) { | |
| const safeLimitMax = 500 | |
| const safeLimit = Math.min(limit, safeLimitMax) | |
| baseQuery = baseQuery.limit(safeLimit) | |
| if (page && !isNaN(page) && page > 1) { | |
| baseQuery = baseQuery.offset((page - 1) * safeLimit) | |
| } | |
| } |
| let workflows | ||
| const orderByClause = [asc(workflow.sortOrder), asc(workflow.createdAt), asc(workflow.id)] | ||
|
|
||
| let baseQuery: any |
There was a problem hiding this comment.
baseQuery: any bypasses Drizzle's type safety
Drizzle ORM queries are strongly typed end-to-end. Using any here silently drops those guarantees — e.g. .limit() / .offset() return types are no longer checked, and any future misuse won't be caught at compile time.
A practical alternative is to union the two concrete query types, or extract the shared query-building logic into a typed helper. At minimum, prefer unknown over any so the compiler forces explicit narrowing before use.
| const pageParam = url.searchParams.get('page') | ||
| const limitParam = url.searchParams.get('limit') | ||
| const page = pageParam ? parseInt(pageParam, 10) : undefined | ||
| const limit = limitParam ? parseInt(limitParam, 10) : undefined |
There was a problem hiding this comment.
Missing input validation for negative / zero page numbers
Negative page numbers silently fall through: e.g. page=-5 passes the page > 1 check as false, so the offset clause is never applied — callers get page-1 results regardless. Consider adding an explicit guard:
| const pageParam = url.searchParams.get('page') | |
| const limitParam = url.searchParams.get('limit') | |
| const page = pageParam ? parseInt(pageParam, 10) : undefined | |
| const limit = limitParam ? parseInt(limitParam, 10) : undefined | |
| const pageParam = url.searchParams.get('page') | |
| const limitParam = url.searchParams.get('limit') | |
| const page = pageParam ? Math.max(1, parseInt(pageParam, 10)) : undefined | |
| const limit = limitParam ? parseInt(limitParam, 10) : undefined |
This ensures that any page value below 1 is clamped to 1 rather than silently returning the first page without surfacing an error to the caller.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| if (page && !isNaN(page) && page > 1) { | ||
| baseQuery = baseQuery.offset((page - 1) * limit) | ||
| } | ||
| } |
There was a problem hiding this comment.
No upper bound on user-supplied limit parameter
Medium Severity
The limit query parameter is user-controlled with no maximum cap, so a request like ?limit=999999999 still fetches all rows — the exact OOM scenario this PR aims to fix. The codebase already enforces MAX_LIMIT = 250 via parsePaginationParams in apps/sim/app/api/v1/admin/types.ts; the same safeguard is missing here.


Summary
Fixes a bug where fetching workspaces drops the server due to excessive memory exhaustion.
The
GET /api/workflowsendpoint was basically fetching every single workflow without any bounds or limits. On larger workspaces, this risks causing OOM crashes or freezing the frontend.I just added standard offset/limit pagination pulling directly from the URL parameters so that the UI can paginate gracefully.
Fixes #3435
Type of Change
Testing
Tested pulling workflows with and without the URL limit/offset parameters locally. The query gracefully degrades to the default fetch if no params are provided, so it doesn't break any existing UI logic anywhere else.
Checklist