Skip to content

feat(tasks): enhance delete task functionality with account resolution and access control - #487

Merged
sweetmantech merged 4 commits into
testfrom
feat/api-tasks-delete-enhance
Apr 28, 2026
Merged

feat(tasks): enhance delete task functionality with account resolution and access control#487
sweetmantech merged 4 commits into
testfrom
feat/api-tasks-delete-enhance

Conversation

@pradipthaadhi

@pradipthaadhipradipthaadhi commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator
  • Updated registerDeleteTaskTool to include account ID resolution using resolveAccountId, ensuring only authorized users can delete tasks.
  • Modified deleteTask to accept a resolvedAccountId parameter, enforcing access control by checking task ownership before deletion.
  • Introduced TASK_ACCESS_DENIED_MESSAGE for clearer error handling when access is denied.
  • Updated deleteTaskHandler to handle access denial responses appropriately.
  • Adjusted validation schema for task deletion to ensure proper UUID format.

…n and access control
- Updated `registerDeleteTaskTool` to include account ID resolution using `resolveAccountId`, ensuring only authorized users can delete tasks.
- Modified `deleteTask` to accept a `resolvedAccountId` parameter, enforcing access control by checking task ownership before deletion.
- Introduced `TASK_ACCESS_DENIED_MESSAGE` for clearer error handling when access is denied.
- Updated `deleteTaskHandler` to handle access denial responses appropriately.
- Adjusted validation schema for task deletion to ensure proper UUID format.
This implementation improves security and user feedback during task deletion operations.
@vercel

vercelBot commented Apr 28, 2026

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

ProjectDeploymentActionsUpdated (UTC)
apiReadyReadyPreviewApr 28, 2026 9:23pm

Request Review

@coderabbitai

coderabbitaiBot commented Apr 28, 2026

Copy link
Copy Markdown

Warning

Rate limit exceeded

@sweetmantech has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 39 minutes and 22 seconds before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: c4153669-104d-4e41-9c98-f2543b7111c5

📥 Commits

Reviewing files that changed from the base of the PR and between a5eff42 and e2333b9.

⛔ Files ignored due to path filters (2)
  • lib/tasks/__tests__/deleteTask.test.ts is excluded by !**/*.test.*, !**/__tests__/** and included by lib/**
  • lib/tasks/__tests__/validateDeleteTaskRequest.test.ts is excluded by !**/*.test.*, !**/__tests__/** and included by lib/**
📒 Files selected for processing (5)
  • lib/mcp/tools/tasks/registerDeleteTaskTool.ts
  • lib/tasks/deleteTask.ts
  • lib/tasks/deleteTaskHandler.ts
  • lib/tasks/validateDeleteTaskBody.ts
  • lib/tasks/validateDeleteTaskRequest.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/api-tasks-delete-enhance

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@cubic-dev-aicubic-dev-aiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 7 files

Confidence score: 3/5

  • There is a concrete error-handling gap in lib/mcp/tools/tasks/registerDeleteTaskTool.ts: exceptions from deleteTask can escape as uncaught handler errors instead of structured tool results.
  • Because this is a medium-severity (6/10) issue with high confidence, it introduces real user-facing failure risk in delete-task flows, so merge risk is moderate rather than minimal.
  • Pay close attention to lib/mcp/tools/tasks/registerDeleteTaskTool.ts - ensure deleteTask failures are caught and mapped to getToolResultError(...) for consistent tool responses.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/mcp/tools/tasks/registerDeleteTaskTool.ts">
<violation number="1" location="lib/mcp/tools/tasks/registerDeleteTaskTool.ts:45">
P2: Handle `deleteTask` exceptions in the tool callback and return `getToolResultError(...)` so failures are returned as tool results instead of uncaught handler errors.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client
participant Handler as Task Handler (API/MCP)
participant Auth as Auth Service
participant Logic as deleteTask Logic
participant DB as Database (Supabase)
participant Trigger as Trigger.dev (External)
Note over Client,Trigger: Task Deletion Flow with Access Control
Client->>Handler: DELETE /api/tasks (id)
Handler->>Auth: NEW: resolveAccountId(request/authInfo)
alt Auth Success
Auth-->>Handler: accountId
else Auth Failure
Auth-->>Handler: Error (401/403)
Handler-->>Client: Error Response
end
Handler->>Handler: CHANGED: validate id as UUID
Handler->>Logic: CHANGED: deleteTask(id, resolvedAccountId)
Logic->>DB: Fetch task by id
DB-->>Logic: task record (including account_id)
alt Task Not Found
Logic-->>Handler: Error: Task not found
Handler-->>Client: 404 Not Found
else Task Found
Logic->>Logic: NEW: Check ownership (task.account_id === resolvedAccountId)
alt NEW: Access Denied
Logic-->>Handler: Error: TASK_ACCESS_DENIED_MESSAGE
Handler-->>Client: 403 Forbidden
else Access Granted
Note over Logic,Trigger: Parallel Deletion
par Logic->>DB: deleteScheduledAction(id)
and opt task has trigger_schedule_id
Logic->>Trigger: deleteSchedule(trigger_id)
end
end
Logic-->>Handler: Success
Handler-->>Client: 200 OK / Tool Result Success
end
end
Loading

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment threadlib/mcp/tools/tasks/registerDeleteTaskTool.ts Outdated

@cubic-dev-aicubic-dev-aiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 1 file (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/mcp/tools/tasks/registerDeleteTaskTool.ts">
<violation number="1" location="lib/mcp/tools/tasks/registerDeleteTaskTool.ts:50">
P1: Do not return raw exception messages from this catch block; it can leak internal error details to clients. Log the original error server-side and return a generic message.
(Based on your team's feedback about avoiding raw exception text in 500 responses.) [FEEDBACK_USED]</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment threadlib/mcp/tools/tasks/registerDeleteTaskTool.ts Outdated
- Enhanced the error handling in `registerDeleteTaskTool` to return specific error messages for task not found and access denied scenarios.
- Introduced a new constant `TASK_NOT_FOUND_MESSAGE` for clearer error reporting.
- Updated the catch block to log unexpected errors and return a generic internal server error message.
This change improves user feedback during task deletion operations and ensures better handling of specific error cases.

@cubic-dev-aicubic-dev-aiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 issues found across 1 file (changes from recent commits).

Requires human review: This PR introduces security-related changes by implementing authorization and ownership checks for task deletion, which requires human verification.

@sweetmantech

Copy link
Copy Markdown
Contributor

Preview deployment test results

Exercised the preview at https://api-git-feat-api-tasks-delete-enhance-recoupable-ad724970.vercel.app.

✅ Validation + auth gates (all working)

#CaseStatusResult
1No auth401"Exactly one of x-api-key or Authorization must be provided"
2Invalid JSON body400"Invalid JSON body"
3Missing id400missing_fields: ["id"]
4Bad UUID400"id must be a valid UUID"
8OPTIONS preflight200CORS handler responds

✅ Happy path + ownership

#CaseStatusResult
10Create throwaway task200id: 4e4c51d2-45c3-4b38-ac60-841a763dca12
11DELETE the throwaway with valid auth200{"status":"success"}
12DELETE same id again (already gone)404"Task not found"

⚠️ Findings — divergences from PATCH (PR #486)

These aren't blockers for the PR's core goal, but they're inconsistencies between sibling endpoints (PATCH /api/tasks vs DELETE /api/tasks) that are worth deciding consciously.

1. Missing .strict() on deleteTaskBodySchema

curl -X DELETE ... -d '{"id":"00000000-0000-0000-0000-000000000000","bogus":"x"}'# → HTTP 404 (lookup proceeded; "bogus" was silently dropped)

PATCH PR #486 added .strict() and rejects unknown keys with "Unrecognized key: \"bogus\"". DELETE silently ignores them.

2. Missing body account_id override path

In lib/tasks/validateDeleteTaskRequest.ts:43:

constauthContext=awaitvalidateAuthContext(request);// no override

PATCH passes { accountId: validationResult.data.account_id } so org keys can scope deletes to a member account. DELETE doesn't, which means an org key cannot delete tasks owned by its member accounts — only its own. Either intentional (DELETE is irreversible, restrict by default) or an oversight worth fixing for parity.

🚨 Heads-up — accidental deletion during testing

While testing what I expected to be a cross-account 403 scenario:

curl -X DELETE ... -d '{"id":"d2510ffe-5e82-45a0-bd5e-51fea2214f22","account_id":"a1234567-89ab-4def-8123-456789abcdef"}'# → HTTP 200 {"status":"success"}

I expected this to mirror PATCH PR #486's test 15b which returned 403. But because (a) the body schema lacks .strict() and (b) validateAuthContext is called without the override, the spurious account_id was silently dropped, auth resolved to the API key's actual account (the real owner of the task), and the delete proceeded as a legitimate owner-deleting-own-task.

Net: task d2510ffe-5e82-45a0-bd5e-51fea2214f22 ("hello world" / Justin Bieber's weekly Monday 9am task) is gone. Not a security bug, but the lack of .strict() made the test misleading — a body field that the user thought would be honored was silently ignored, and the destructive action proceeded.

Verdict

Core PR objective — auth-derived resolvedAccountId + row-ownership guard on DELETE /api/tasks — works. Both cubic comments addressed in earlier commits. Two divergences from PATCH worth reconciling for consistency:

  • Add .strict() to deleteTaskBodySchema
  • Decide whether org-key body account_id override should work for DELETE (PATCH supports it)

Both are small follow-up patches; can also fold into this PR if you want sibling consistency.

@sweetmantech
sweetmantech merged commit 12175dc into testApr 28, 2026
6 checks passed
@sweetmantech
sweetmantech deleted the feat/api-tasks-delete-enhance branch April 28, 2026 21:29
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@pradipthaadhi@sweetmantech