feat(api): enhance task run retrieval with account access control#523
Open
pradipthaadhi wants to merge 8 commits intotestfrom
Open
feat(api): enhance task run retrieval with account access control#523pradipthaadhi wants to merge 8 commits intotestfrom
pradipthaadhi wants to merge 8 commits intotestfrom
Conversation
- Updated the GET /api/tasks/runs endpoint to support two modes: - Retrieve mode: returns a single task run when `runId` is provided. - List mode: returns recent task runs for the authorized account when `runId` is omitted. - Added optional query parameters for `account_id` and `limit` to control access and pagination. - Implemented account access validation to ensure users can only retrieve their own task runs, returning a 403 error for unauthorized access. - Updated related validation and test cases to reflect these changes.
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughValidator now returns a resolved accountId for retrieve mode. The single-run handler checks the retrieved run's tags for Account-based access control enforcement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 5 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Architecture diagram
sequenceDiagram
participant Client
participant Route as GET /api/tasks/runs
participant Auth as AuthContext
participant Validator as validateGetTaskRunQuery
participant Admin as checkIsAdmin
participant Handler as getTaskRunHandler
participant TriggerDev as Trigger.dev API
Client->>Route: GET /api/tasks/runs?runId=xxx&account_id=yyy&limit=20
Route->>Auth: validateAuthContext(request)
Auth-->>Route: { accountId, orgId, authToken }
Route->>Validator: validateGetTaskRunQuery(request, authResult)
Validator->>Validator: Parse query params via Zod schema
alt runId present (Retrieve mode)
alt account_id override provided
Validator->>Admin: checkIsAdmin(authResult)
alt is admin
Validator->>Validator: Use provided account_id as targetAccountId
else not admin
Validator-->>Route: 403 response
end
else no override
Validator->>Validator: Use authResult.accountId as targetAccountId
end
Validator-->>Route: { mode: "retrieve", runId, accountId }
else no runId (List mode)
alt account_id override provided
Validator->>Admin: checkIsAdmin(authResult)
alt is admin
Validator->>Validator: Use provided account_id
else not admin
Validator-->>Route: 403 response
end
else no override
Validator->>Validator: Use authResult.accountId
end
Validator-->>Route: { mode: "list", accountId, limit }
end
Route->>Handler: getTaskRunHandler(request) with validated query
alt Retrieve mode
Handler->>TriggerDev: retrieveTaskRun(runId)
TriggerDev-->>Handler: taskRun data
alt run not found
Handler-->>Route: 404 { status: "error", error: "Run not found" }
else run found
Handler->>Handler: Check runs.tags includes `account:{validatedQuery.accountId}`
alt tag matches
Handler-->>Route: 200 { status: "success", runs: [taskRun] }
else tag mismatch
Handler-->>Route: 403 { status: "error", error: "Access denied to this task run" }
end
end
else List mode
Handler->>TriggerDev: listTaskRuns(accountId, limit)
TriggerDev-->>Handler: taskRun[]
Handler-->>Route: 200 { status: "success", runs: taskRun[] }
end
Route-->>Client: NextResponse with CORS headers
Requires human review: Changes introduce access control logic to a task run retrieval API, modifying core business logic and authentication. Even if AI found no issues, the blast radius includes potential data access bugs
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/tasks/getTaskRunHandler.ts (1)
8-16:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale JSDoc no longer reflects the handler's full response surface.
Line 10 still reads "Always returns
{ status: 'success', runs: [...] }", but the handler can now return HTTP 403, 404, and 500 error shapes too.📝 Suggested JSDoc update
/** * Handles GET /api/tasks/runs requests. - * Always returns { status: "success", runs: [...] }. - * When runId is provided, runs contains a single element. - * When omitted, runs contains recent runs for the authenticated account. + * Supports two modes: + * - Retrieve mode (runId provided): validates account ownership via run tags, + * returns `{ status: "success", runs: [run] }` or 403/404 on failure. + * - List mode (runId omitted): returns recent runs for the authenticated account. * * `@param` request - The NextRequest object - * `@returns` A NextResponse with the runs array + * `@returns` A NextResponse with the runs array, or an error response (400/403/404/500) */🤖 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 `@lib/tasks/getTaskRunHandler.ts` around lines 8 - 16, Update the JSDoc for the GET handler in getTaskRunHandler.ts to reflect the full response surface: mention that it can return 200 with { status: "success", runs: [...] } (single run when runId provided or recent runs otherwise) and also 403 (authorization error), 404 (not found), and 500 (internal error) shapes; reference the handler function name (getTaskRunHandler or default export handling GET) and include brief descriptions of each error response body so the doc matches the actual behavior and aids consumers and maintainers.lib/tasks/validateGetTaskRunQuery.ts (1)
30-32:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winJSDoc still documents the old
{ mode: "retrieve", runId }shape —accountIdis missing.The retrieve branch now returns
{ mode: "retrieve", runId, accountId }, but the JSDoc on line 31 still shows the old two-field shape.📝 Suggested JSDoc update
- * - `{ mode: "retrieve", runId }` when runId is provided + * - `{ mode: "retrieve", runId, accountId }` when runId is provided🤖 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 `@lib/tasks/validateGetTaskRunQuery.ts` around lines 30 - 32, Update the JSDoc describing the discriminated union to include accountId in the retrieve shape: change the documented branch from `{ mode: "retrieve", runId }` to `{ mode: "retrieve", runId, accountId }` so it matches the actual return shape produced by validateGetTaskRunQuery (and any related type aliases in this file); ensure the list branch remains `{ mode: "list", accountId, limit }` and adjust the comment text accordingly.
🧹 Nitpick comments (2)
lib/tasks/getTaskRunHandler.ts (1)
44-47: ⚡ Quick winSimplify the
tagsextraction — the?? []fallback in the truthy branch is unreachable dead code.When
Array.isArray(x)returnstrue,xis guaranteed non-null/undefined, so the?? []on line 45 can never execute. The two separate casts onresultare also unnecessarily noisy — collapse to a single intermediate variable.♻️ Proposed refactor
- const tags = Array.isArray((result as { tags?: unknown }).tags) - ? ((result as { tags?: unknown[] }).tags ?? []) - : []; - const hasAccountAccess = tags.includes(`account:${validatedQuery.accountId}`); + const rawTags = (result as { tags?: unknown }).tags; + const tags: unknown[] = Array.isArray(rawTags) ? rawTags : []; + const hasAccountAccess = tags.includes(`account:${validatedQuery.accountId}`);🤖 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 `@lib/tasks/getTaskRunHandler.ts` around lines 44 - 47, The tags extraction contains unreachable `?? []` and duplicated casts; simplify by extracting tags once into a properly typed local (e.g., const tagsRaw = (result as { tags?: unknown }).tags), then set const tags = Array.isArray(tagsRaw) ? tagsRaw as unknown[] : [] and keep hasAccountAccess = tags.includes(`account:${validatedQuery.accountId}`); remove the redundant null-coalescing and duplicate casts in getTaskRunHandler (use the single tagsRaw variable and the tags const).lib/tasks/validateGetTaskRunQuery.ts (1)
42-99: ⚖️ Poor tradeoff
validateGetTaskRunQueryexceeds the 50-line limit — extract account resolution into a helper.The function is ~58 lines. The account resolution block (lines 72–92) — including the admin bypass and
validateAccountIdOverridebranching — is a distinct concern that could live in its own helper (e.g.resolveTargetAccountId.ts), leaving the validation function focused purely on parsing and schema validation.♻️ Sketch of the extracted helper
// lib/tasks/resolveTargetAccountId.ts export async function resolveTargetAccountId( authAccountId: string, accountIdOverride?: string, ): Promise<string | NextResponse> { if (!accountIdOverride) return authAccountId; const isAdmin = await checkIsAdmin(authAccountId); if (isAdmin) return accountIdOverride; const overrideResult = await validateAccountIdOverride({ currentAccountId: authAccountId, targetAccountId: accountIdOverride, }); if (overrideResult instanceof NextResponse) return overrideResult; return overrideResult.accountId; }Then in
validateGetTaskRunQuery:- // Resolve the target account ID - let targetAccountId = authResult.accountId; - - if (result.data.account_id) { - const isAdmin = await checkIsAdmin(authResult.accountId); - if (isAdmin) { - targetAccountId = result.data.account_id; - } else { - const overrideResult = await validateAccountIdOverride({ ... }); - if (overrideResult instanceof NextResponse) return overrideResult; - targetAccountId = overrideResult.accountId; - } - } + const targetAccountId = await resolveTargetAccountId( + authResult.accountId, + result.data.account_id, + ); + if (targetAccountId instanceof NextResponse) return targetAccountId;As per coding guidelines,
lib/**/*.tsfunctions should be kept under 50 lines, with a file naming rule requiring the new file to be named after its exported function.🤖 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 `@lib/tasks/validateGetTaskRunQuery.ts` around lines 42 - 99, The account resolution block in validateGetTaskRunQuery should be extracted into a new helper resolveTargetAccountId to keep validateGetTaskRunQuery under 50 lines; create export async function resolveTargetAccountId(authAccountId: string, accountIdOverride?: string): Promise<string | NextResponse> that implements the current logic: return authAccountId if no override, call checkIsAdmin(authAccountId) and return accountIdOverride if admin, otherwise call validateAccountIdOverride({ currentAccountId: authAccountId, targetAccountId: accountIdOverride }) and propagate a NextResponse if returned or return overrideResult.accountId; replace the in-function block in validateGetTaskRunQuery with a call to resolveTargetAccountId and use its result for targetAccountId.
🤖 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.
Inline comments:
In `@app/api/tasks/runs/route.ts`:
- Line 27: The docs for the query param account_id are too restrictive; update
the inline comment that currently reads "admin/org-authorized only" to list all
allowed cases: admin users, org-authorized users, and personal API keys when the
provided account_id matches the key owner's account (as enforced by
validateGetTaskRunQuery -> validateAccountIdOverride). Locate the comment near
the account_id line in the route file and change the description to mention
those three permitted cases so it matches the actual validation logic.
In `@lib/tasks/getTaskRunHandler.ts`:
- Around line 48-53: The current branch that returns NextResponse.json({ status:
"error", error: "Access denied to this task run" }, { status: 403, headers:
getCorsHeaders() }) leaks existence of the run; change this to return a 404
matching the "Task run not found" response instead. Replace the 403 response in
the hasAccountAccess check with NextResponse.json({ status: "error", error:
"Task run not found" }, { status: 404, headers: getCorsHeaders() }) (i.e., use
404 and the same error text/headers used for missing runs) so callers without
access cannot enumerate run IDs.
---
Outside diff comments:
In `@lib/tasks/getTaskRunHandler.ts`:
- Around line 8-16: Update the JSDoc for the GET handler in getTaskRunHandler.ts
to reflect the full response surface: mention that it can return 200 with {
status: "success", runs: [...] } (single run when runId provided or recent runs
otherwise) and also 403 (authorization error), 404 (not found), and 500
(internal error) shapes; reference the handler function name (getTaskRunHandler
or default export handling GET) and include brief descriptions of each error
response body so the doc matches the actual behavior and aids consumers and
maintainers.
In `@lib/tasks/validateGetTaskRunQuery.ts`:
- Around line 30-32: Update the JSDoc describing the discriminated union to
include accountId in the retrieve shape: change the documented branch from `{
mode: "retrieve", runId }` to `{ mode: "retrieve", runId, accountId }` so it
matches the actual return shape produced by validateGetTaskRunQuery (and any
related type aliases in this file); ensure the list branch remains `{ mode:
"list", accountId, limit }` and adjust the comment text accordingly.
---
Nitpick comments:
In `@lib/tasks/getTaskRunHandler.ts`:
- Around line 44-47: The tags extraction contains unreachable `?? []` and
duplicated casts; simplify by extracting tags once into a properly typed local
(e.g., const tagsRaw = (result as { tags?: unknown }).tags), then set const tags
= Array.isArray(tagsRaw) ? tagsRaw as unknown[] : [] and keep hasAccountAccess =
tags.includes(`account:${validatedQuery.accountId}`); remove the redundant
null-coalescing and duplicate casts in getTaskRunHandler (use the single tagsRaw
variable and the tags const).
In `@lib/tasks/validateGetTaskRunQuery.ts`:
- Around line 42-99: The account resolution block in validateGetTaskRunQuery
should be extracted into a new helper resolveTargetAccountId to keep
validateGetTaskRunQuery under 50 lines; create export async function
resolveTargetAccountId(authAccountId: string, accountIdOverride?: string):
Promise<string | NextResponse> that implements the current logic: return
authAccountId if no override, call checkIsAdmin(authAccountId) and return
accountIdOverride if admin, otherwise call validateAccountIdOverride({
currentAccountId: authAccountId, targetAccountId: accountIdOverride }) and
propagate a NextResponse if returned or return overrideResult.accountId; replace
the in-function block in validateGetTaskRunQuery with a call to
resolveTargetAccountId and use its result for targetAccountId.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 02d5ab20-c215-4891-b409-725dd9140b94
⛔ Files ignored due to path filters (2)
lib/tasks/__tests__/getTaskRunHandler.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/tasks/__tests__/validateGetTaskRunQuery.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (3)
app/api/tasks/runs/route.tslib/tasks/getTaskRunHandler.tslib/tasks/validateGetTaskRunQuery.ts
- Updated documentation for the `account_id` query parameter to clarify its usage for account scoping. - Changed error response from 403 to 404 when access to a task run is denied, improving clarity for clients. - Modified the validation response to include `accountId` in the retrieve mode, aligning with the updated query structure. - Updated tests to reflect changes in error handling and response structure, ensuring consistency across the API.
…recoupable/api into feat/api-tasks-runs-enhancement
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)
app/api/tasks/runs/route.ts (1)
31-33:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the
@returnstext to reflect runs payload, not status-only wording.The current return description still reads like a single status response and is now out of sync with the documented two-mode
runsoutput.As per coding guidelines, “Use meaningful comments, not obvious ones” and “Write self-documenting code.”
🤖 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 `@app/api/tasks/runs/route.ts` around lines 31 - 33, Update the JSDoc `@returns` line for the API handler in route.ts to describe the runs payload instead of a status-only response; specifically, change the text for the function handling GET (exported GET / handler) to say it returns a NextResponse containing the runs payload (including both detailed run data or status-only output depending on the selected mode), so the comment matches the two-mode `runs` output.
🧹 Nitpick comments (1)
lib/tasks/getTaskRunHandler.ts (1)
17-69: ⚡ Quick winSplit
getTaskRunHandlerinto mode-specific helpers to reduce scope and size.This handler now mixes validation flow, list retrieval, retrieve authorization, and error mapping in one long function. Extract list-mode and retrieve-mode branches (plus shared response helpers) to keep it easier to maintain and within repo limits.
♻️ Refactor sketch
+async function handleListMode(/* ... */): Promise<NextResponse> { + // fetchTriggerRuns + success response +} + +async function handleRetrieveMode(/* ... */): Promise<NextResponse> { + // retrieveTaskRun + tag-based account check + success/not-found responses +} + export async function getTaskRunHandler(request: NextRequest): Promise<NextResponse> { const validatedQuery = await validateGetTaskRunQuery(request); if (validatedQuery instanceof NextResponse) return validatedQuery; try { - if (validatedQuery.mode === "list") { - // list logic - } - // retrieve logic + return validatedQuery.mode === "list" + ? handleListMode(/* ... */) + : handleRetrieveMode(/* ... */); } catch (error) { // shared error mapping } }As per coding guidelines, “Flag functions longer than 20 lines”, “Keep functions under 50 lines”, and “Apply Single Responsibility Principle (SRP): one exported function per file; each file should do one thing well.”
🤖 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 `@lib/tasks/getTaskRunHandler.ts` around lines 17 - 69, Split getTaskRunHandler into small mode-specific helpers to reduce scope: create a handleListMode(validatedQuery) that wraps fetchTriggerRuns and returns a NextResponse JSON with {status:"success", runs} and CORS headers, and create a handleRetrieveMode(validatedQuery) that calls retrieveTaskRun, checks null, extracts tags safely, enforces account access (tags.includes(`account:${validatedQuery.accountId}`)), and returns the appropriate NextResponse for success, not-found, or unauthorized; factor the repeated NextResponse.json(..., {headers: getCorsHeaders()}) into a small helper like makeCorsResponse(body, status) to avoid duplication, then have getTaskRunHandler only perform validation via validateGetTaskRunQuery and delegate to the two helpers inside the try/catch while preserving existing error handling.
🤖 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 `@app/api/tasks/runs/route.ts`:
- Around line 31-33: Update the JSDoc `@returns` line for the API handler in
route.ts to describe the runs payload instead of a status-only response;
specifically, change the text for the function handling GET (exported GET /
handler) to say it returns a NextResponse containing the runs payload (including
both detailed run data or status-only output depending on the selected mode), so
the comment matches the two-mode `runs` output.
---
Nitpick comments:
In `@lib/tasks/getTaskRunHandler.ts`:
- Around line 17-69: Split getTaskRunHandler into small mode-specific helpers to
reduce scope: create a handleListMode(validatedQuery) that wraps
fetchTriggerRuns and returns a NextResponse JSON with {status:"success", runs}
and CORS headers, and create a handleRetrieveMode(validatedQuery) that calls
retrieveTaskRun, checks null, extracts tags safely, enforces account access
(tags.includes(`account:${validatedQuery.accountId}`)), and returns the
appropriate NextResponse for success, not-found, or unauthorized; factor the
repeated NextResponse.json(..., {headers: getCorsHeaders()}) into a small helper
like makeCorsResponse(body, status) to avoid duplication, then have
getTaskRunHandler only perform validation via validateGetTaskRunQuery and
delegate to the two helpers inside the try/catch while preserving existing error
handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6840f806-2f64-40f8-9ec8-7b1b77178be0
⛔ Files ignored due to path filters (1)
lib/tasks/__tests__/getTaskRunHandler.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (3)
app/api/tasks/runs/route.tslib/tasks/getTaskRunHandler.tslib/tasks/validateGetTaskRunQuery.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/tasks/validateGetTaskRunQuery.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
runIdis provided.runIdis omitted.account_idandlimitto control access and pagination.Summary by CodeRabbit
New Features
Bug Fixes