diff --git a/lib/songs/__tests__/authorizeCatalogAccess.test.ts b/lib/songs/__tests__/authorizeCatalogAccess.test.ts new file mode 100644 index 00000000..1bfb8bd2 --- /dev/null +++ b/lib/songs/__tests__/authorizeCatalogAccess.test.ts @@ -0,0 +1,67 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { NextResponse } from "next/server"; +import { authorizeCatalogAccess } from "@/lib/songs/authorizeCatalogAccess"; +import { selectAccountCatalogs } from "@/lib/supabase/account_catalogs/selectAccountCatalogs"; + +vi.mock("@/lib/supabase/account_catalogs/selectAccountCatalogs", () => ({ + selectAccountCatalogs: vi.fn(), +})); +vi.mock("@/lib/networking/getCorsHeaders", () => ({ + getCorsHeaders: () => ({ "Access-Control-Allow-Origin": "*" }), +})); + +describe("authorizeCatalogAccess", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("allows a catalog the account owns", async () => { + vi.mocked(selectAccountCatalogs).mockResolvedValue([{ id: "cat_1" }] as never); + + expect(await authorizeCatalogAccess("acc_1", ["cat_1"])).toBeNull(); + }); + + it("rejects a catalog the account does not own", async () => { + vi.mocked(selectAccountCatalogs).mockResolvedValue([{ id: "mine" }] as never); + + const result = await authorizeCatalogAccess("acc_1", ["someone_elses"]); + + expect(result).toBeInstanceOf(NextResponse); + expect((result as NextResponse).status).toBe(403); + }); + + // A write can name several catalogs in one body; authorizing only the first + // would let one owned catalog carry edits into catalogs the caller does not own. + it("rejects when only one of several catalogs is unowned", async () => { + vi.mocked(selectAccountCatalogs).mockResolvedValue([{ id: "mine" }] as never); + + const result = await authorizeCatalogAccess("acc_1", ["mine", "theirs"]); + + expect((result as NextResponse).status).toBe(403); + }); + + // Review finding (cubic, 2026-07-30). A bulk body naming many catalogs used + // to fan out into one query per catalog, which can exhaust the connection + // pool before the write even runs. + it("reads the caller's catalogs once regardless of how many are named", async () => { + vi.mocked(selectAccountCatalogs).mockResolvedValue([ + { id: "a" }, + { id: "b" }, + { id: "c" }, + ] as never); + + await authorizeCatalogAccess("acc_1", ["a", "b", "c", "a", "b"]); + + expect(selectAccountCatalogs).toHaveBeenCalledTimes(1); + expect(selectAccountCatalogs).toHaveBeenCalledWith("acc_1"); + }); + + // Review finding (cubic, 2026-07-30). selectAccountCatalogs throws on a query + // failure, and that must surface as a 500 from the handler rather than being + // swallowed into a false "does not belong" 403 that clients will not retry. + it("propagates a database failure rather than reporting it as forbidden", async () => { + vi.mocked(selectAccountCatalogs).mockRejectedValue(new Error("connection reset")); + + await expect(authorizeCatalogAccess("acc_1", ["cat_1"])).rejects.toThrow("connection reset"); + }); +}); diff --git a/lib/songs/__tests__/validateCatalogSongsRequest.test.ts b/lib/songs/__tests__/validateCatalogSongsRequest.test.ts new file mode 100644 index 00000000..c0a73a70 --- /dev/null +++ b/lib/songs/__tests__/validateCatalogSongsRequest.test.ts @@ -0,0 +1,93 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { NextRequest, NextResponse } from "next/server"; +import { validateCatalogSongsRequest } from "@/lib/songs/validateCatalogSongsRequest"; +import { validateAuthContext } from "@/lib/auth/validateAuthContext"; +import { authorizeCatalogAccess } from "@/lib/songs/authorizeCatalogAccess"; + +vi.mock("@/lib/auth/validateAuthContext", () => ({ validateAuthContext: vi.fn() })); +vi.mock("@/lib/songs/authorizeCatalogAccess", () => ({ authorizeCatalogAccess: vi.fn() })); +vi.mock("@/lib/networking/getCorsHeaders", () => ({ + getCorsHeaders: () => ({ "Access-Control-Allow-Origin": "*" }), +})); + +const post = (body: unknown) => + new NextRequest("https://api.test/api/catalogs/songs", { + method: "POST", + body: JSON.stringify(body), + headers: { "Content-Type": "application/json" }, + }); + +describe("validateCatalogSongsRequest", () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(authorizeCatalogAccess).mockResolvedValue(null); + }); + + /** + * chat#1912 row 6. The order is the contract: 401 before 400 before 403. + * A malformed body from an unauthenticated caller previously returned 400, + * and that 400-without-credentials is exactly what proved the endpoint had + * no auth layer. Body validation must never run first. + */ + it("returns 401 for a malformed body when the caller has no credentials", async () => { + vi.mocked(validateAuthContext).mockResolvedValue( + NextResponse.json({ status: "error" }, { status: 401 }) as never, + ); + + const result = await validateCatalogSongsRequest(post({})); + + expect((result as NextResponse).status).toBe(401); + expect(authorizeCatalogAccess).not.toHaveBeenCalled(); + }); + + it("returns 400 for a malformed body once the caller is authenticated", async () => { + vi.mocked(validateAuthContext).mockResolvedValue({ accountId: "acc_1" } as never); + + const result = await validateCatalogSongsRequest(post({})); + + expect((result as NextResponse).status).toBe(400); + // Ownership is meaningless without a valid body, so it must not be consulted. + expect(authorizeCatalogAccess).not.toHaveBeenCalled(); + }); + + it("returns the authorization failure for someone else's catalog", async () => { + vi.mocked(validateAuthContext).mockResolvedValue({ accountId: "acc_1" } as never); + vi.mocked(authorizeCatalogAccess).mockResolvedValue( + NextResponse.json({ status: "error" }, { status: 403 }) as never, + ); + + const result = await validateCatalogSongsRequest( + post({ songs: [{ catalog_id: "theirs", isrc: "X" }] }), + ); + + expect((result as NextResponse).status).toBe(403); + }); + + it("returns the validated body plus the authenticated account on success", async () => { + vi.mocked(validateAuthContext).mockResolvedValue({ accountId: "acc_1" } as never); + + const result = await validateCatalogSongsRequest( + post({ songs: [{ catalog_id: "mine", isrc: "X" }] }), + ); + + expect(result).toMatchObject({ + accountId: "acc_1", + songs: [{ catalog_id: "mine", isrc: "X" }], + }); + }); + + it("checks every catalog named in the body, not just the first", async () => { + vi.mocked(validateAuthContext).mockResolvedValue({ accountId: "acc_1" } as never); + + await validateCatalogSongsRequest( + post({ + songs: [ + { catalog_id: "mine", isrc: "X" }, + { catalog_id: "theirs", isrc: "Y" }, + ], + }), + ); + + expect(authorizeCatalogAccess).toHaveBeenCalledWith("acc_1", ["mine", "theirs"]); + }); +}); diff --git a/lib/songs/authorizeCatalogAccess.ts b/lib/songs/authorizeCatalogAccess.ts new file mode 100644 index 00000000..a45f8112 --- /dev/null +++ b/lib/songs/authorizeCatalogAccess.ts @@ -0,0 +1,39 @@ +import { NextResponse } from "next/server"; +import { getCorsHeaders } from "@/lib/networking/getCorsHeaders"; +import { selectAccountCatalogs } from "@/lib/supabase/account_catalogs/selectAccountCatalogs"; + +/** + * Ownership half of the `/api/catalogs/songs` gate: every catalog the operation + * touches must belong to `accountId`. + * + * Authentication is deliberately **not** done here. Callers run + * `validateAuthContext` before parsing or validating the request, so a caller + * with no credentials gets 401 rather than a body-validation 400 — the exact + * confusion that hid this hole in the first place (chat#1912 row 6). + * + * Reads the caller's catalogs in **one** query and checks membership in memory, + * so a bulk body naming many catalogs cannot fan out into many simultaneous + * queries. `selectAccountCatalogs` throws on a query failure, so a database + * outage surfaces as a 500 rather than a false "does not belong" 403. + * + * @param accountId - The authenticated account + * @param catalogIds - Every catalog the operation touches + * @returns A 403 NextResponse when any catalog is not the caller's, else null + */ +export async function authorizeCatalogAccess( + accountId: string, + catalogIds: string[], +): Promise { + const owned = await selectAccountCatalogs(accountId); + const ownedIds = new Set(owned.map(catalog => catalog.id)); + + if (catalogIds.every(id => ownedIds.has(id))) return null; + + return NextResponse.json( + { + status: "error", + error: "This catalog does not belong to the authenticated account", + }, + { status: 403, headers: getCorsHeaders() }, + ); +} diff --git a/lib/songs/createCatalogSongsHandler.ts b/lib/songs/createCatalogSongsHandler.ts index 0b6171ee..b4ff2370 100644 --- a/lib/songs/createCatalogSongsHandler.ts +++ b/lib/songs/createCatalogSongsHandler.ts @@ -20,10 +20,7 @@ import { validateCatalogSongsRequest } from "@/lib/songs/validateCatalogSongsReq */ export async function createCatalogSongsHandler(request: NextRequest): Promise { try { - const body = await request.json(); - - // Validate request body - const validatedBody = validateCatalogSongsRequest(body); + const validatedBody = await validateCatalogSongsRequest(request); if (validatedBody instanceof NextResponse) { return validatedBody; } @@ -85,7 +82,9 @@ export async function createCatalogSongsHandler(request: NextRequest): Promise { try { - const body = await request.json(); - - // Validate request body - const validatedBody = validateCatalogSongsRequest(body); + const validatedBody = await validateCatalogSongsRequest(request); if (validatedBody instanceof NextResponse) { return validatedBody; } @@ -58,7 +55,9 @@ export async function deleteCatalogSongsHandler(request: NextRequest): Promise { try { - const { searchParams } = new URL(request.url); - - const validatedQuery = validateCatalogSongsQuery(searchParams); + const validatedQuery = await validateCatalogSongsQuery(request); if (validatedQuery instanceof NextResponse) { return validatedQuery; } @@ -56,7 +54,9 @@ export async function getCatalogSongsHandler(request: NextRequest): Promise; +export type ValidatedCatalogSongsQuery = CatalogSongsQuery & { accountId: string }; + /** - * Validates catalog songs query parameters. + * Validates a catalog songs read: credentials, then query shape, then that the + * catalog belongs to the caller. + * + * The order is the contract (chat#1912 row 6, recoupable/docs#282). Auth runs + * before the query is parsed so a caller with no credentials gets 401 rather + * than a validation 400 — a 400 without credentials is precisely what proved + * this endpoint had no auth layer at all. * - * @param searchParams - The URL search parameters to validate. - * @returns A NextResponse with an error if validation fails, or the validated query parameters if validation passes. + * @param request - The incoming request, carrying `x-api-key` or a bearer token + * @returns A NextResponse (401/400/403), or the validated query plus the + * authenticated account */ -export function validateCatalogSongsQuery( - searchParams: URLSearchParams, -): NextResponse | CatalogSongsQuery { +export async function validateCatalogSongsQuery( + request: NextRequest, +): Promise { + const auth = await validateAuthContext(request); + if (auth instanceof NextResponse) return auth; + + const { searchParams } = new URL(request.url); const params = Object.fromEntries(searchParams.entries()); const validationResult = catalogSongsQuerySchema.safeParse(params); @@ -49,5 +64,10 @@ export function validateCatalogSongsQuery( ); } - return validationResult.data; + const forbidden = await authorizeCatalogAccess(auth.accountId, [ + validationResult.data.catalog_id, + ]); + if (forbidden) return forbidden; + + return { ...validationResult.data, accountId: auth.accountId }; } diff --git a/lib/songs/validateCatalogSongsRequest.ts b/lib/songs/validateCatalogSongsRequest.ts index 85da95cb..63f11b8a 100644 --- a/lib/songs/validateCatalogSongsRequest.ts +++ b/lib/songs/validateCatalogSongsRequest.ts @@ -1,5 +1,8 @@ -import { NextResponse } from "next/server"; +import { NextRequest, NextResponse } from "next/server"; import { getCorsHeaders } from "@/lib/networking/getCorsHeaders"; +import { safeParseJson } from "@/lib/networking/safeParseJson"; +import { validateAuthContext } from "@/lib/auth/validateAuthContext"; +import { authorizeCatalogAccess } from "@/lib/songs/authorizeCatalogAccess"; import { z } from "zod"; const catalogSongInputSchema = z.object({ @@ -17,13 +20,30 @@ export const catalogSongsRequestSchema = z.object({ export type CatalogSongsRequest = z.infer; +export type ValidatedCatalogSongsRequest = CatalogSongsRequest & { accountId: string }; + /** - * Validates a catalog songs request body. + * Validates a catalog songs write (POST and DELETE share it): credentials, + * then body shape, then that every catalog named belongs to the caller. + * + * The order is the contract (chat#1912 row 6, recoupable/docs#282). Auth runs + * before the body is read so a caller with no credentials gets 401 rather than + * a validation 400 — a 400 without credentials is precisely what proved this + * endpoint had no auth layer at all. A write can name several catalogs, and + * every one is checked: authorizing only the first would let one owned catalog + * carry edits into catalogs the caller does not own. * - * @param body - The request body to validate. - * @returns A NextResponse with an error if validation fails, or the validated body if validation passes. + * @param request - The incoming request, carrying `x-api-key` or a bearer token + * @returns A NextResponse (401/400/403), or the validated body plus the + * authenticated account */ -export function validateCatalogSongsRequest(body: unknown): NextResponse | CatalogSongsRequest { +export async function validateCatalogSongsRequest( + request: NextRequest, +): Promise { + const auth = await validateAuthContext(request); + if (auth instanceof NextResponse) return auth; + + const body = await safeParseJson(request); const validationResult = catalogSongsRequestSchema.safeParse(body); if (!validationResult.success) { @@ -41,5 +61,11 @@ export function validateCatalogSongsRequest(body: unknown): NextResponse | Catal ); } - return validationResult.data; + const forbidden = await authorizeCatalogAccess( + auth.accountId, + validationResult.data.songs.map(song => song.catalog_id), + ); + if (forbidden) return forbidden; + + return { ...validationResult.data, accountId: auth.accountId }; }