-
Notifications
You must be signed in to change notification settings - Fork 15
Security: enforce auth and ownership checks on chat fallback DELETE /api/scheduled-actions/delete #1640
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Security: enforce auth and ownership checks on chat fallback DELETE /api/scheduled-actions/delete #1640
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,124 @@ | ||
| import { beforeEach, describe, expect, it, vi } from "vitest"; | ||
| import { NextRequest } from "next/server"; | ||
| import { DELETE } from "../route"; | ||
| import { validateHeaders } from "@/lib/chat/validateHeaders"; | ||
| import { checkAccountArtistAccess } from "@/lib/supabase/account_artist_ids/checkAccountArtistAccess"; | ||
| import { deleteScheduledActionById } from "@/lib/supabase/scheduled_actions/deleteScheduledActionById"; | ||
| import { selectScheduledActionById } from "@/lib/supabase/scheduled_actions/selectScheduledActionById"; | ||
|
|
||
| vi.mock("@/lib/chat/validateHeaders", () => ({ | ||
| validateHeaders: vi.fn(), | ||
| })); | ||
|
|
||
| vi.mock("@/lib/supabase/account_artist_ids/checkAccountArtistAccess", () => ({ | ||
| checkAccountArtistAccess: vi.fn(), | ||
| })); | ||
|
|
||
| vi.mock("@/lib/supabase/scheduled_actions/selectScheduledActionById", () => ({ | ||
| selectScheduledActionById: vi.fn(), | ||
| })); | ||
|
|
||
| vi.mock("@/lib/supabase/scheduled_actions/deleteScheduledActionById", () => ({ | ||
| deleteScheduledActionById: vi.fn(), | ||
| })); | ||
|
|
||
| function makeRequest(body: unknown): NextRequest { | ||
| return new Request("http://localhost/api/scheduled-actions/delete", { | ||
| method: "DELETE", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify(body), | ||
| }) as NextRequest; | ||
| } | ||
|
|
||
| describe("DELETE /api/scheduled-actions/delete", () => { | ||
| const mockValidateHeaders = vi.mocked(validateHeaders); | ||
| const mockCheckAccountArtistAccess = vi.mocked(checkAccountArtistAccess); | ||
| const mockSelectScheduledActionById = vi.mocked(selectScheduledActionById); | ||
| const mockDeleteScheduledActionById = vi.mocked(deleteScheduledActionById); | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| it("returns 401 when caller is unauthenticated", async () => { | ||
| mockValidateHeaders.mockResolvedValueOnce({}); | ||
|
|
||
| const response = await DELETE(makeRequest({ id: "1e632dc8-94aa-4f85-9f85-241213d0d2f9" })); | ||
| const data = await response.json(); | ||
|
|
||
| expect(response.status).toBe(401); | ||
| expect(data).toEqual({ error: "Unauthorized" }); | ||
| expect(mockSelectScheduledActionById).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("returns 400 when id is missing or invalid", async () => { | ||
| mockValidateHeaders.mockResolvedValueOnce({ accountId: "account-123" }); | ||
|
|
||
| const response = await DELETE(makeRequest({ id: "not-a-uuid" })); | ||
| const data = await response.json(); | ||
|
|
||
| expect(response.status).toBe(400); | ||
| expect(data.error).toContain("UUID"); | ||
| expect(mockSelectScheduledActionById).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("returns 404 when task does not exist", async () => { | ||
| mockValidateHeaders.mockResolvedValueOnce({ accountId: "account-123" }); | ||
| mockSelectScheduledActionById.mockResolvedValueOnce({ | ||
| data: null, | ||
| error: null, | ||
| } as Awaited<ReturnType<typeof selectScheduledActionById>>); | ||
|
|
||
| const response = await DELETE(makeRequest({ id: "1e632dc8-94aa-4f85-9f85-241213d0d2f9" })); | ||
| const data = await response.json(); | ||
|
|
||
| expect(response.status).toBe(404); | ||
| expect(data).toEqual({ error: "Task not found" }); | ||
| }); | ||
|
|
||
| it("returns 403 when caller is not owner and lacks artist access", async () => { | ||
| mockValidateHeaders.mockResolvedValueOnce({ accountId: "account-123" }); | ||
| mockCheckAccountArtistAccess.mockResolvedValueOnce(false); | ||
| mockSelectScheduledActionById.mockResolvedValueOnce({ | ||
| data: { | ||
| id: "1e632dc8-94aa-4f85-9f85-241213d0d2f9", | ||
| account_id: "other-account", | ||
| artist_account_id: "artist-456", | ||
| }, | ||
| error: null, | ||
| } as Awaited<ReturnType<typeof selectScheduledActionById>>); | ||
|
|
||
| const response = await DELETE(makeRequest({ id: "1e632dc8-94aa-4f85-9f85-241213d0d2f9" })); | ||
| const data = await response.json(); | ||
|
|
||
| expect(response.status).toBe(403); | ||
| expect(data).toEqual({ error: "Forbidden" }); | ||
| expect(mockDeleteScheduledActionById).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("returns 200 and deletes when caller owns the task", async () => { | ||
| mockValidateHeaders.mockResolvedValueOnce({ accountId: "account-123" }); | ||
| mockSelectScheduledActionById.mockResolvedValueOnce({ | ||
| data: { | ||
| id: "1e632dc8-94aa-4f85-9f85-241213d0d2f9", | ||
| account_id: "account-123", | ||
| artist_account_id: "artist-456", | ||
| }, | ||
| error: null, | ||
| } as Awaited<ReturnType<typeof selectScheduledActionById>>); | ||
| mockDeleteScheduledActionById.mockResolvedValueOnce({ | ||
| error: null, | ||
| } as Awaited<ReturnType<typeof deleteScheduledActionById>>); | ||
|
|
||
| const response = await DELETE(makeRequest({ id: "1e632dc8-94aa-4f85-9f85-241213d0d2f9" })); | ||
| const data = await response.json(); | ||
|
|
||
| expect(response.status).toBe(200); | ||
| expect(data).toEqual({ success: true }); | ||
| expect(mockCheckAccountArtistAccess).not.toHaveBeenCalled(); | ||
| expect(mockDeleteScheduledActionById).toHaveBeenCalledTimes(1); | ||
| expect(mockDeleteScheduledActionById).toHaveBeenCalledWith( | ||
| "1e632dc8-94aa-4f85-9f85-241213d0d2f9" | ||
| ); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,18 +1,60 @@ | ||
| import { NextRequest, NextResponse } from "next/server"; | ||
| import supabase from "@/lib/supabase/serverClient"; | ||
| import { z } from "zod"; | ||
| import { validateHeaders } from "@/lib/chat/validateHeaders"; | ||
| import { checkAccountArtistAccess } from "@/lib/supabase/account_artist_ids/checkAccountArtistAccess"; | ||
| import { deleteScheduledActionById } from "@/lib/supabase/scheduled_actions/deleteScheduledActionById"; | ||
| import { selectScheduledActionById } from "@/lib/supabase/scheduled_actions/selectScheduledActionById"; | ||
|
|
||
| const deleteScheduledActionBodySchema = z.object({ | ||
| id: z.string().uuid("id must be a valid UUID"), | ||
| }); | ||
|
|
||
| export async function DELETE(req: NextRequest) { | ||
| const { id } = await req.json(); | ||
| const auth = await validateHeaders(req); | ||
| if (auth instanceof Response) { | ||
| return auth; | ||
| } | ||
|
Comment on lines
+13
to
+16
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are you updating the |
||
|
|
||
| if (!id) { | ||
| return NextResponse.json({ error: "Missing task id" }, { status: 400 }); | ||
| const accountId = auth.accountId; | ||
| if (!accountId) { | ||
| return NextResponse.json({ error: "Unauthorized" }, { status: 401 }); | ||
| } | ||
|
|
||
| let body: unknown; | ||
| try { | ||
| const { error } = await supabase | ||
| .from("scheduled_actions") | ||
| .delete() | ||
| .eq("id", id); | ||
| body = await req.json(); | ||
| } catch { | ||
| return NextResponse.json({ error: "Invalid JSON body" }, { status: 400 }); | ||
| } | ||
|
|
||
| const parsed = deleteScheduledActionBodySchema.safeParse(body); | ||
| if (!parsed.success) { | ||
| return NextResponse.json({ error: parsed.error.issues[0]?.message ?? "Invalid id" }, { status: 400 }); | ||
| } | ||
|
|
||
| try { | ||
| const { id } = parsed.data; | ||
|
|
||
| const { data: scheduledAction, error: selectError } = await selectScheduledActionById(id); | ||
|
|
||
| if (selectError) { | ||
| throw new Error(`Failed to load task: ${selectError.message}`); | ||
| } | ||
|
|
||
| if (!scheduledAction) { | ||
| return NextResponse.json({ error: "Task not found" }, { status: 404 }); | ||
| } | ||
|
|
||
| const canDeleteAsOwner = scheduledAction.account_id === accountId; | ||
| const canDeleteAsArtistAccess = canDeleteAsOwner | ||
| ? true | ||
| : await checkAccountArtistAccess(accountId, scheduledAction.artist_account_id); | ||
|
|
||
| if (!canDeleteAsOwner && !canDeleteAsArtistAccess) { | ||
| return NextResponse.json({ error: "Forbidden" }, { status: 403 }); | ||
| } | ||
|
|
||
| const { error } = await deleteScheduledActionById(id); | ||
|
|
||
| if (error) { | ||
| throw new Error(`Failed to delete task: ${error.message}`); | ||
|
|
@@ -28,4 +70,3 @@ export async function DELETE(req: NextRequest) { | |
| } | ||
|
|
||
| export const dynamic = "force-dynamic"; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import { useState } from "react"; | ||
| import { toast } from "react-toastify"; | ||
| import { useQueryClient } from "@tanstack/react-query"; | ||
| import { usePrivy } from "@privy-io/react-auth"; | ||
| import { deleteTask } from "@/lib/tasks/deleteTask"; | ||
|
|
||
| interface DeleteScheduledActionParams { | ||
|
|
@@ -12,6 +13,7 @@ interface DeleteScheduledActionParams { | |
| export const useDeleteScheduledAction = () => { | ||
| const [isLoading, setIsLoading] = useState(false); | ||
| const queryClient = useQueryClient(); | ||
| const { getAccessToken } = usePrivy(); | ||
|
|
||
| const deleteAction = async ({ | ||
| actionId, | ||
|
|
@@ -20,7 +22,12 @@ export const useDeleteScheduledAction = () => { | |
| }: DeleteScheduledActionParams) => { | ||
| setIsLoading(true); | ||
| try { | ||
| await deleteTask({ id: actionId }); | ||
| const accessToken = await getAccessToken(); | ||
| if (!accessToken) { | ||
| throw new Error("Please sign in to delete scheduled actions"); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: The auth error message "Please sign in to delete scheduled actions" is swallowed by the catch block's generic Prompt for AI agents |
||
| } | ||
|
|
||
| await deleteTask({ id: actionId, accessToken }); | ||
|
|
||
| onSuccess?.(); | ||
| toast.success(successMessage); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| import supabase from "@/lib/supabase/serverClient"; | ||
|
|
||
| export async function deleteScheduledActionById(id: string) { | ||
| return supabase.from("scheduled_actions").delete().eq("id", id); | ||
|
Comment on lines
+3
to
+4
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make the delete authorization atomic.
As per coding guidelines, "Implement built-in security practices for authentication and data handling." 🤖 Prompt for AI Agents |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| import supabase from "@/lib/supabase/serverClient"; | ||
| import type { Tables } from "@/types/database.types"; | ||
|
|
||
| export type ScheduledActionAuthRow = Pick< | ||
| Tables<"scheduled_actions">, | ||
| "id" | "account_id" | "artist_account_id" | ||
| >; | ||
|
|
||
| export async function selectScheduledActionById(id: string) { | ||
| return supabase | ||
| .from("scheduled_actions") | ||
| .select("id, account_id, artist_account_id") | ||
| .eq("id", id) | ||
| .maybeSingle<ScheduledActionAuthRow>(); | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3,6 +3,7 @@ import { TASKS_API_URL } from "@/lib/consts"; | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| export interface DeleteTaskParams { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| id: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| accessToken: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| const SCHEDULE_NOT_FOUND_MSG = "Schedule not found"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -18,10 +19,13 @@ function isScheduleNotFoundError(errorText: string): boolean { | |||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Delete task record from database when scheduler deletion isn't possible | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| async function deleteTaskFromDatabase(taskId: string): Promise<void> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| async function deleteTaskFromDatabase(taskId: string, accessToken: string): Promise<void> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| await fetch("/api/scheduled-actions/delete", { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The fallback route now enforces auth and ownership checks, but this fetch result is ignored, so Useful? React with 👍 / 👎. |
||||||||||||||||||||||||||||||||||||||||||||||||||||
| method: "DELETE", | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| headers: { "Content-Type": "application/json" }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| headers: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| "Content-Type": "application/json", | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| Authorization: `Bearer ${accessToken}`, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| body: JSON.stringify({ id: taskId }), | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+22
to
29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: The fetch response in Prompt for AI agents
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+22
to
30
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do not swallow fallback delete failures.
Suggested fix async function deleteTaskFromDatabase(taskId: string, accessToken: string): Promise<void> {
- await fetch("/api/scheduled-actions/delete", {
+ const response = await fetch("/api/scheduled-actions/delete", {
method: "DELETE",
headers: {
"Content-Type": "application/json",
Authorization: `Bearer ${accessToken}`,
},
body: JSON.stringify({ id: taskId }),
});
+
+ if (!response.ok) {
+ const errorText = await response.text();
+ throw new Error(`HTTP ${response.status}: ${errorText}`);
+ }
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -32,10 +36,15 @@ async function deleteTaskFromDatabase(taskId: string): Promise<void> { | |||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| export async function deleteTask(params: DeleteTaskParams): Promise<void> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!params.accessToken) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new Error("Please sign in to delete scheduled actions"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| const response = await fetch(TASKS_API_URL, { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| method: "DELETE", | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| headers: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| "Content-Type": "application/json", | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| Authorization: `Bearer ${params.accessToken}`, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| body: JSON.stringify({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| id: params.id, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -46,7 +55,7 @@ export async function deleteTask(params: DeleteTaskParams): Promise<void> { | |||||||||||||||||||||||||||||||||||||||||||||||||||
| const errorText = await response.text(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (isScheduleNotFoundError(errorText)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| await deleteTaskFromDatabase(params.id); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| await deleteTaskFromDatabase(params.id, params.accessToken); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.