From bf422a4facfa9f18217847271ada42f9914e5d7c Mon Sep 17 00:00:00 2001 From: Sweets Sweetman Date: Thu, 30 Jul 2026 15:21:49 -0500 Subject: [PATCH 1/3] fix(catalogs): catalog songs require authentication and ownership (chat#1912 row 6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements the contract in recoupable/docs#282. GET, POST and DELETE /api/catalogs/songs enforced nothing. Verified on prod 2026-07-30: all three reached query or body validation with no credentials (GET 200, POST and DELETE 400 on an empty body), so anyone holding a catalog id could read its tracks and ISRCs, add songs, or delete them — while the sibling /api/catalogs/{catalogId}/measurements returned 401 for that same catalog. The catalog report page relied on that asymmetry, telling a stranger the valuation belonged to another account on the page that listed its songs. New authorizeCatalogAccess gates all three on validateAuthContext plus an account_catalogs ownership check, returning 401 without credentials and 403 for a catalog the caller does not own. The account is always the authenticated one, never taken from the request. Writes can name several catalogs in one body, so every distinct catalog is checked; authorizing only the first would let one owned catalog carry edits to catalogs the caller does not own. Co-Authored-By: Claude Opus 5 (1M context) --- .../__tests__/authorizeCatalogAccess.test.ts | 81 +++++++++++++++++++ lib/songs/authorizeCatalogAccess.ts | 47 +++++++++++ lib/songs/createCatalogSongsHandler.ts | 9 +++ lib/songs/deleteCatalogSongsHandler.ts | 9 +++ lib/songs/getCatalogSongsHandler.ts | 7 ++ 5 files changed, 153 insertions(+) create mode 100644 lib/songs/__tests__/authorizeCatalogAccess.test.ts create mode 100644 lib/songs/authorizeCatalogAccess.ts diff --git a/lib/songs/__tests__/authorizeCatalogAccess.test.ts b/lib/songs/__tests__/authorizeCatalogAccess.test.ts new file mode 100644 index 000000000..aa21e6e63 --- /dev/null +++ b/lib/songs/__tests__/authorizeCatalogAccess.test.ts @@ -0,0 +1,81 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { NextRequest, NextResponse } from "next/server"; +import { authorizeCatalogAccess } from "@/lib/songs/authorizeCatalogAccess"; +import { validateAuthContext } from "@/lib/auth/validateAuthContext"; +import { selectAccountCatalog } from "@/lib/supabase/account_catalogs/selectAccountCatalog"; + +vi.mock("@/lib/auth/validateAuthContext", () => ({ validateAuthContext: vi.fn() })); +vi.mock("@/lib/supabase/account_catalogs/selectAccountCatalog", () => ({ + selectAccountCatalog: vi.fn(), +})); +vi.mock("@/lib/networking/getCorsHeaders", () => ({ + getCorsHeaders: () => ({ "Access-Control-Allow-Origin": "*" }), +})); + +const request = () => new NextRequest("https://api.test/api/catalogs/songs"); + +describe("authorizeCatalogAccess", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + // chat#1912 row 6. GET/POST/DELETE /api/catalogs/songs enforced no auth at + // all: on prod, all three reached body or query validation with no + // credentials, so anyone holding a catalog id could read, add or remove its + // songs while /measurements returned 401 for the same catalog. + it("rejects a caller with no credentials", async () => { + vi.mocked(validateAuthContext).mockResolvedValue( + NextResponse.json({ status: "error" }, { status: 401 }) as never, + ); + + const result = await authorizeCatalogAccess(request(), ["cat_1"]); + + expect(result).toBeInstanceOf(NextResponse); + expect((result as NextResponse).status).toBe(401); + expect(selectAccountCatalog).not.toHaveBeenCalled(); + }); + + it("rejects a catalog the caller does not own", async () => { + vi.mocked(validateAuthContext).mockResolvedValue({ accountId: "acc_1" } as never); + vi.mocked(selectAccountCatalog).mockResolvedValue(null); + + const result = await authorizeCatalogAccess(request(), ["someone_elses"]); + + expect((result as NextResponse).status).toBe(403); + }); + + it("returns the account id for a catalog the caller owns", async () => { + vi.mocked(validateAuthContext).mockResolvedValue({ accountId: "acc_1" } as never); + vi.mocked(selectAccountCatalog).mockResolvedValue({ + account: "acc_1", + catalog: "cat_1", + } as never); + + const result = await authorizeCatalogAccess(request(), ["cat_1"]); + + expect(result).toEqual({ accountId: "acc_1" }); + }); + + it("rejects when only one of several catalogs is unowned", async () => { + vi.mocked(validateAuthContext).mockResolvedValue({ accountId: "acc_1" } as never); + vi.mocked(selectAccountCatalog) + .mockResolvedValueOnce({ account: "acc_1" } as never) + .mockResolvedValueOnce(null); + + const result = await authorizeCatalogAccess(request(), ["mine", "theirs"]); + + expect((result as NextResponse).status).toBe(403); + }); + + it("checks ownership against the authenticated account, never a caller-supplied one", async () => { + vi.mocked(validateAuthContext).mockResolvedValue({ accountId: "acc_real" } as never); + vi.mocked(selectAccountCatalog).mockResolvedValue({ account: "acc_real" } as never); + + await authorizeCatalogAccess(request(), ["cat_1"]); + + expect(selectAccountCatalog).toHaveBeenCalledWith({ + accountId: "acc_real", + catalogId: "cat_1", + }); + }); +}); diff --git a/lib/songs/authorizeCatalogAccess.ts b/lib/songs/authorizeCatalogAccess.ts new file mode 100644 index 000000000..56c7783a9 --- /dev/null +++ b/lib/songs/authorizeCatalogAccess.ts @@ -0,0 +1,47 @@ +import { NextRequest, NextResponse } from "next/server"; +import { getCorsHeaders } from "@/lib/networking/getCorsHeaders"; +import { validateAuthContext } from "@/lib/auth/validateAuthContext"; +import { selectAccountCatalog } from "@/lib/supabase/account_catalogs/selectAccountCatalog"; + +/** + * Gate for every `/api/catalogs/songs` operation: the caller must be + * authenticated and the catalog must belong to them. + * + * All three operations previously enforced nothing, so anyone holding a + * catalog id could read, add or remove its songs while the sibling + * `/measurements` endpoint returned 401 for the same catalog (chat#1912 row 6, + * contract in recoupable/docs#282). + * + * The account is always the authenticated one, never a caller-supplied value. + * + * A write can name several catalogs in one body, so every distinct catalog is + * checked — authorizing only the first would let one owned catalog carry edits + * to catalogs the caller does not own. + * + * @param request - The incoming request, carrying `x-api-key` or a bearer token + * @param catalogIds - Every catalog the operation touches + * @returns `{ accountId }` when authorized, or a 401/403 NextResponse + */ +export async function authorizeCatalogAccess( + request: NextRequest, + catalogIds: string[], +): Promise<{ accountId: string } | NextResponse> { + const authResult = await validateAuthContext(request); + if (authResult instanceof NextResponse) return authResult; + + const { accountId } = authResult; + const links = await Promise.all( + [...new Set(catalogIds)].map(catalogId => selectAccountCatalog({ accountId, catalogId })), + ); + if (links.some(link => !link)) { + return NextResponse.json( + { + status: "error", + error: "This catalog does not belong to the authenticated account", + }, + { status: 403, headers: getCorsHeaders() }, + ); + } + + return { accountId }; +} diff --git a/lib/songs/createCatalogSongsHandler.ts b/lib/songs/createCatalogSongsHandler.ts index 0b6171ee7..777781566 100644 --- a/lib/songs/createCatalogSongsHandler.ts +++ b/lib/songs/createCatalogSongsHandler.ts @@ -5,6 +5,7 @@ import { selectCatalogSongsWithArtists } from "@/lib/supabase/catalog_songs/sele import { processSongsInput } from "@/lib/songs/processSongsInput"; import { SongInput } from "@/lib/songs/formatSongsInput"; import { validateCatalogSongsRequest } from "@/lib/songs/validateCatalogSongsRequest"; +import { authorizeCatalogAccess } from "@/lib/songs/authorizeCatalogAccess"; /** * Handler for creating catalog-song relationships. @@ -28,6 +29,14 @@ export async function createCatalogSongsHandler(request: NextRequest): Promise song.catalog_id).filter((id): id is string => !!id), + ); + if (authorized instanceof NextResponse) return authorized; + // Get unique ISRCs and create song records with CSV data preserved const dataByIsrc = validatedBody.songs.reduce((map, song) => { if (song.isrc) { diff --git a/lib/songs/deleteCatalogSongsHandler.ts b/lib/songs/deleteCatalogSongsHandler.ts index 55521322d..e5307df1f 100644 --- a/lib/songs/deleteCatalogSongsHandler.ts +++ b/lib/songs/deleteCatalogSongsHandler.ts @@ -3,6 +3,7 @@ import { getCorsHeaders } from "@/lib/networking/getCorsHeaders"; import { selectCatalogSongsWithArtists } from "@/lib/supabase/catalog_songs/selectCatalogSongsWithArtists"; import { deleteCatalogSongs } from "@/lib/supabase/catalog_songs/deleteCatalogSongs"; import { validateCatalogSongsRequest } from "@/lib/songs/validateCatalogSongsRequest"; +import { authorizeCatalogAccess } from "@/lib/songs/authorizeCatalogAccess"; /** * Handler for deleting catalog-song relationships. @@ -26,6 +27,14 @@ export async function deleteCatalogSongsHandler(request: NextRequest): Promise song.catalog_id).filter((id): id is string => !!id), + ); + if (authorized instanceof NextResponse) return authorized; + // Delete catalog_songs relationships const affectedCatalogIds = await deleteCatalogSongs(validatedBody.songs); diff --git a/lib/songs/getCatalogSongsHandler.ts b/lib/songs/getCatalogSongsHandler.ts index 46e57ab79..7c3cf57e2 100644 --- a/lib/songs/getCatalogSongsHandler.ts +++ b/lib/songs/getCatalogSongsHandler.ts @@ -2,6 +2,7 @@ import { NextRequest, NextResponse } from "next/server"; import { getCorsHeaders } from "@/lib/networking/getCorsHeaders"; import { selectCatalogSongsWithArtists } from "@/lib/supabase/catalog_songs/selectCatalogSongsWithArtists"; import { validateCatalogSongsQuery } from "@/lib/songs/validateCatalogSongsQuery"; +import { authorizeCatalogAccess } from "@/lib/songs/authorizeCatalogAccess"; /** * Handler for retrieving catalog songs with pagination. @@ -24,6 +25,12 @@ export async function getCatalogSongsHandler(request: NextRequest): Promise Date: Thu, 30 Jul 2026 15:57:15 -0500 Subject: [PATCH 2/3] fix(catalogs): authenticate before parsing, and check ownership in one query MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four review findings, all valid. Auth ran after body and query validation, so an unauthenticated malformed request still got a 400 rather than the promised 401 — the exact confusion this PR set out to remove, since a 400 without credentials is what proved there was no auth layer. All three handlers now call validateAuthContext before touching the request: 401 for no credentials, then 400 for a bad body, then 403 for someone else's catalog. authorizeCatalogAccess now takes the already-authenticated accountId rather than the request, which makes that ordering explicit at each call site, and reads the caller's catalogs in one query via selectAccountCatalogs instead of fanning out one query per catalog named in the body. That also fixes a false 403: selectAccountCatalog returns null on a query failure, so a database outage was being reported as "does not belong" and would not be retried. selectAccountCatalogs throws, so an outage now surfaces as a 500. The 500 body no longer echoes the exception text, which can now carry auth and database failure detail. The detail is still logged. Co-Authored-By: Claude Opus 5 (1M context) --- .../__tests__/authorizeCatalogAccess.test.ts | 84 ++++++++----------- lib/songs/authorizeCatalogAccess.ts | 60 ++++++------- lib/songs/createCatalogSongsHandler.ts | 20 +++-- lib/songs/deleteCatalogSongsHandler.ts | 20 +++-- lib/songs/getCatalogSongsHandler.ts | 20 +++-- 5 files changed, 103 insertions(+), 101 deletions(-) diff --git a/lib/songs/__tests__/authorizeCatalogAccess.test.ts b/lib/songs/__tests__/authorizeCatalogAccess.test.ts index aa21e6e63..1bfb8bd21 100644 --- a/lib/songs/__tests__/authorizeCatalogAccess.test.ts +++ b/lib/songs/__tests__/authorizeCatalogAccess.test.ts @@ -1,81 +1,67 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; -import { NextRequest, NextResponse } from "next/server"; +import { NextResponse } from "next/server"; import { authorizeCatalogAccess } from "@/lib/songs/authorizeCatalogAccess"; -import { validateAuthContext } from "@/lib/auth/validateAuthContext"; -import { selectAccountCatalog } from "@/lib/supabase/account_catalogs/selectAccountCatalog"; +import { selectAccountCatalogs } from "@/lib/supabase/account_catalogs/selectAccountCatalogs"; -vi.mock("@/lib/auth/validateAuthContext", () => ({ validateAuthContext: vi.fn() })); -vi.mock("@/lib/supabase/account_catalogs/selectAccountCatalog", () => ({ - selectAccountCatalog: vi.fn(), +vi.mock("@/lib/supabase/account_catalogs/selectAccountCatalogs", () => ({ + selectAccountCatalogs: vi.fn(), })); vi.mock("@/lib/networking/getCorsHeaders", () => ({ getCorsHeaders: () => ({ "Access-Control-Allow-Origin": "*" }), })); -const request = () => new NextRequest("https://api.test/api/catalogs/songs"); - describe("authorizeCatalogAccess", () => { beforeEach(() => { vi.clearAllMocks(); }); - // chat#1912 row 6. GET/POST/DELETE /api/catalogs/songs enforced no auth at - // all: on prod, all three reached body or query validation with no - // credentials, so anyone holding a catalog id could read, add or remove its - // songs while /measurements returned 401 for the same catalog. - it("rejects a caller with no credentials", async () => { - vi.mocked(validateAuthContext).mockResolvedValue( - NextResponse.json({ status: "error" }, { status: 401 }) as never, - ); - - const result = await authorizeCatalogAccess(request(), ["cat_1"]); + it("allows a catalog the account owns", async () => { + vi.mocked(selectAccountCatalogs).mockResolvedValue([{ id: "cat_1" }] as never); - expect(result).toBeInstanceOf(NextResponse); - expect((result as NextResponse).status).toBe(401); - expect(selectAccountCatalog).not.toHaveBeenCalled(); + expect(await authorizeCatalogAccess("acc_1", ["cat_1"])).toBeNull(); }); - it("rejects a catalog the caller does not own", async () => { - vi.mocked(validateAuthContext).mockResolvedValue({ accountId: "acc_1" } as never); - vi.mocked(selectAccountCatalog).mockResolvedValue(null); + it("rejects a catalog the account does not own", async () => { + vi.mocked(selectAccountCatalogs).mockResolvedValue([{ id: "mine" }] as never); - const result = await authorizeCatalogAccess(request(), ["someone_elses"]); + const result = await authorizeCatalogAccess("acc_1", ["someone_elses"]); + expect(result).toBeInstanceOf(NextResponse); expect((result as NextResponse).status).toBe(403); }); - it("returns the account id for a catalog the caller owns", async () => { - vi.mocked(validateAuthContext).mockResolvedValue({ accountId: "acc_1" } as never); - vi.mocked(selectAccountCatalog).mockResolvedValue({ - account: "acc_1", - catalog: "cat_1", - } as never); + // 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(request(), ["cat_1"]); + const result = await authorizeCatalogAccess("acc_1", ["mine", "theirs"]); - expect(result).toEqual({ accountId: "acc_1" }); + expect((result as NextResponse).status).toBe(403); }); - it("rejects when only one of several catalogs is unowned", async () => { - vi.mocked(validateAuthContext).mockResolvedValue({ accountId: "acc_1" } as never); - vi.mocked(selectAccountCatalog) - .mockResolvedValueOnce({ account: "acc_1" } as never) - .mockResolvedValueOnce(null); + // 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); - const result = await authorizeCatalogAccess(request(), ["mine", "theirs"]); + await authorizeCatalogAccess("acc_1", ["a", "b", "c", "a", "b"]); - expect((result as NextResponse).status).toBe(403); + expect(selectAccountCatalogs).toHaveBeenCalledTimes(1); + expect(selectAccountCatalogs).toHaveBeenCalledWith("acc_1"); }); - it("checks ownership against the authenticated account, never a caller-supplied one", async () => { - vi.mocked(validateAuthContext).mockResolvedValue({ accountId: "acc_real" } as never); - vi.mocked(selectAccountCatalog).mockResolvedValue({ account: "acc_real" } as never); - - await authorizeCatalogAccess(request(), ["cat_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")); - expect(selectAccountCatalog).toHaveBeenCalledWith({ - accountId: "acc_real", - catalogId: "cat_1", - }); + await expect(authorizeCatalogAccess("acc_1", ["cat_1"])).rejects.toThrow("connection reset"); }); }); diff --git a/lib/songs/authorizeCatalogAccess.ts b/lib/songs/authorizeCatalogAccess.ts index 56c7783a9..a45f81126 100644 --- a/lib/songs/authorizeCatalogAccess.ts +++ b/lib/songs/authorizeCatalogAccess.ts @@ -1,47 +1,39 @@ -import { NextRequest, NextResponse } from "next/server"; +import { NextResponse } from "next/server"; import { getCorsHeaders } from "@/lib/networking/getCorsHeaders"; -import { validateAuthContext } from "@/lib/auth/validateAuthContext"; -import { selectAccountCatalog } from "@/lib/supabase/account_catalogs/selectAccountCatalog"; +import { selectAccountCatalogs } from "@/lib/supabase/account_catalogs/selectAccountCatalogs"; /** - * Gate for every `/api/catalogs/songs` operation: the caller must be - * authenticated and the catalog must belong to them. + * Ownership half of the `/api/catalogs/songs` gate: every catalog the operation + * touches must belong to `accountId`. * - * All three operations previously enforced nothing, so anyone holding a - * catalog id could read, add or remove its songs while the sibling - * `/measurements` endpoint returned 401 for the same catalog (chat#1912 row 6, - * contract in recoupable/docs#282). + * 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). * - * The account is always the authenticated one, never a caller-supplied value. + * 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. * - * A write can name several catalogs in one body, so every distinct catalog is - * checked — authorizing only the first would let one owned catalog carry edits - * to catalogs the caller does not own. - * - * @param request - The incoming request, carrying `x-api-key` or a bearer token + * @param accountId - The authenticated account * @param catalogIds - Every catalog the operation touches - * @returns `{ accountId }` when authorized, or a 401/403 NextResponse + * @returns A 403 NextResponse when any catalog is not the caller's, else null */ export async function authorizeCatalogAccess( - request: NextRequest, + accountId: string, catalogIds: string[], -): Promise<{ accountId: string } | NextResponse> { - const authResult = await validateAuthContext(request); - if (authResult instanceof NextResponse) return authResult; +): Promise { + const owned = await selectAccountCatalogs(accountId); + const ownedIds = new Set(owned.map(catalog => catalog.id)); - const { accountId } = authResult; - const links = await Promise.all( - [...new Set(catalogIds)].map(catalogId => selectAccountCatalog({ accountId, catalogId })), - ); - if (links.some(link => !link)) { - return NextResponse.json( - { - status: "error", - error: "This catalog does not belong to the authenticated account", - }, - { status: 403, headers: getCorsHeaders() }, - ); - } + if (catalogIds.every(id => ownedIds.has(id))) return null; - return { accountId }; + 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 777781566..14ab962ea 100644 --- a/lib/songs/createCatalogSongsHandler.ts +++ b/lib/songs/createCatalogSongsHandler.ts @@ -6,6 +6,7 @@ import { processSongsInput } from "@/lib/songs/processSongsInput"; import { SongInput } from "@/lib/songs/formatSongsInput"; import { validateCatalogSongsRequest } from "@/lib/songs/validateCatalogSongsRequest"; import { authorizeCatalogAccess } from "@/lib/songs/authorizeCatalogAccess"; +import { validateAuthContext } from "@/lib/auth/validateAuthContext"; /** * Handler for creating catalog-song relationships. @@ -21,6 +22,11 @@ import { authorizeCatalogAccess } from "@/lib/songs/authorizeCatalogAccess"; */ export async function createCatalogSongsHandler(request: NextRequest): Promise { try { + // Authenticate before parsing the body, so a caller with no credentials + // gets 401 rather than a body-validation 400 (chat#1912 row 6). + const auth = await validateAuthContext(request); + if (auth instanceof NextResponse) return auth; + const body = await request.json(); // Validate request body @@ -29,13 +35,13 @@ export async function createCatalogSongsHandler(request: NextRequest): Promise song.catalog_id).filter((id): id is string => !!id), ); - if (authorized instanceof NextResponse) return authorized; + if (forbidden) return forbidden; // Get unique ISRCs and create song records with CSV data preserved const dataByIsrc = validatedBody.songs.reduce((map, song) => { @@ -94,7 +100,9 @@ export async function createCatalogSongsHandler(request: NextRequest): Promise { try { + // Authenticate before parsing the body, so a caller with no credentials + // gets 401 rather than a body-validation 400 (chat#1912 row 6). + const auth = await validateAuthContext(request); + if (auth instanceof NextResponse) return auth; + const body = await request.json(); // Validate request body @@ -27,13 +33,13 @@ export async function deleteCatalogSongsHandler(request: NextRequest): Promise song.catalog_id).filter((id): id is string => !!id), ); - if (authorized instanceof NextResponse) return authorized; + if (forbidden) return forbidden; // Delete catalog_songs relationships const affectedCatalogIds = await deleteCatalogSongs(validatedBody.songs); @@ -67,7 +73,9 @@ export async function deleteCatalogSongsHandler(request: NextRequest): Promise { try { + // Authenticate before touching the request, so a caller with no + // credentials gets 401 rather than a query-validation 400 (chat#1912 row 6). + const auth = await validateAuthContext(request); + if (auth instanceof NextResponse) return auth; + const { searchParams } = new URL(request.url); const validatedQuery = validateCatalogSongsQuery(searchParams); @@ -25,11 +31,11 @@ export async function getCatalogSongsHandler(request: NextRequest): Promise Date: Thu, 30 Jul 2026 16:20:30 -0500 Subject: [PATCH 3/3] refactor(catalogs): one validate call per handler (chat#1912 row 6 review) Review (@sweetmantech): each handler made two distinct auth/validation calls. Both now live in the validate function the handler already used. validateCatalogSongsQuery takes the request and does credentials, then query shape, then ownership. validateCatalogSongsRequest does the same for the body and is shared by POST and DELETE. Each returns the validated input plus the authenticated accountId, or the 401/400/403 response. Handlers are back to a single validation call followed by business logic, and body parsing moved into the validator via the shared safeParseJson. Adds validateCatalogSongsRequest tests covering the order that is the actual contract: 401 before 400 before 403, ownership never consulted for an invalid body, and every catalog in the body checked rather than only the first. Co-Authored-By: Claude Opus 5 (1M context) --- .../validateCatalogSongsRequest.test.ts | 93 +++++++++++++++++++ lib/songs/createCatalogSongsHandler.ts | 20 +--- lib/songs/deleteCatalogSongsHandler.ts | 20 +--- lib/songs/getCatalogSongsHandler.ts | 17 +--- lib/songs/validateCatalogSongsQuery.ts | 36 +++++-- lib/songs/validateCatalogSongsRequest.ts | 38 ++++++-- 6 files changed, 156 insertions(+), 68 deletions(-) create mode 100644 lib/songs/__tests__/validateCatalogSongsRequest.test.ts diff --git a/lib/songs/__tests__/validateCatalogSongsRequest.test.ts b/lib/songs/__tests__/validateCatalogSongsRequest.test.ts new file mode 100644 index 000000000..c0a73a702 --- /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/createCatalogSongsHandler.ts b/lib/songs/createCatalogSongsHandler.ts index 14ab962ea..b4ff23704 100644 --- a/lib/songs/createCatalogSongsHandler.ts +++ b/lib/songs/createCatalogSongsHandler.ts @@ -5,8 +5,6 @@ import { selectCatalogSongsWithArtists } from "@/lib/supabase/catalog_songs/sele import { processSongsInput } from "@/lib/songs/processSongsInput"; import { SongInput } from "@/lib/songs/formatSongsInput"; import { validateCatalogSongsRequest } from "@/lib/songs/validateCatalogSongsRequest"; -import { authorizeCatalogAccess } from "@/lib/songs/authorizeCatalogAccess"; -import { validateAuthContext } from "@/lib/auth/validateAuthContext"; /** * Handler for creating catalog-song relationships. @@ -22,27 +20,11 @@ import { validateAuthContext } from "@/lib/auth/validateAuthContext"; */ export async function createCatalogSongsHandler(request: NextRequest): Promise { try { - // Authenticate before parsing the body, so a caller with no credentials - // gets 401 rather than a body-validation 400 (chat#1912 row 6). - const auth = await validateAuthContext(request); - if (auth instanceof NextResponse) return auth; - - const body = await request.json(); - - // Validate request body - const validatedBody = validateCatalogSongsRequest(body); + const validatedBody = await validateCatalogSongsRequest(request); if (validatedBody instanceof NextResponse) { return validatedBody; } - // Every catalog named in the body must belong to the caller. This write - // previously enforced nothing at all. - const forbidden = await authorizeCatalogAccess( - auth.accountId, - validatedBody.songs.map(song => song.catalog_id).filter((id): id is string => !!id), - ); - if (forbidden) return forbidden; - // Get unique ISRCs and create song records with CSV data preserved const dataByIsrc = validatedBody.songs.reduce((map, song) => { if (song.isrc) { diff --git a/lib/songs/deleteCatalogSongsHandler.ts b/lib/songs/deleteCatalogSongsHandler.ts index 0fc25d830..5ab268e92 100644 --- a/lib/songs/deleteCatalogSongsHandler.ts +++ b/lib/songs/deleteCatalogSongsHandler.ts @@ -3,8 +3,6 @@ import { getCorsHeaders } from "@/lib/networking/getCorsHeaders"; import { selectCatalogSongsWithArtists } from "@/lib/supabase/catalog_songs/selectCatalogSongsWithArtists"; import { deleteCatalogSongs } from "@/lib/supabase/catalog_songs/deleteCatalogSongs"; import { validateCatalogSongsRequest } from "@/lib/songs/validateCatalogSongsRequest"; -import { authorizeCatalogAccess } from "@/lib/songs/authorizeCatalogAccess"; -import { validateAuthContext } from "@/lib/auth/validateAuthContext"; /** * Handler for deleting catalog-song relationships. @@ -20,27 +18,11 @@ import { validateAuthContext } from "@/lib/auth/validateAuthContext"; */ export async function deleteCatalogSongsHandler(request: NextRequest): Promise { try { - // Authenticate before parsing the body, so a caller with no credentials - // gets 401 rather than a body-validation 400 (chat#1912 row 6). - const auth = await validateAuthContext(request); - if (auth instanceof NextResponse) return auth; - - const body = await request.json(); - - // Validate request body - const validatedBody = validateCatalogSongsRequest(body); + const validatedBody = await validateCatalogSongsRequest(request); if (validatedBody instanceof NextResponse) { return validatedBody; } - // Every catalog named in the body must belong to the caller. This write - // previously enforced nothing at all. - const forbidden = await authorizeCatalogAccess( - auth.accountId, - validatedBody.songs.map(song => song.catalog_id).filter((id): id is string => !!id), - ); - if (forbidden) return forbidden; - // Delete catalog_songs relationships const affectedCatalogIds = await deleteCatalogSongs(validatedBody.songs); diff --git a/lib/songs/getCatalogSongsHandler.ts b/lib/songs/getCatalogSongsHandler.ts index c9fa029c8..8dbfc4ae0 100644 --- a/lib/songs/getCatalogSongsHandler.ts +++ b/lib/songs/getCatalogSongsHandler.ts @@ -2,8 +2,6 @@ import { NextRequest, NextResponse } from "next/server"; import { getCorsHeaders } from "@/lib/networking/getCorsHeaders"; import { selectCatalogSongsWithArtists } from "@/lib/supabase/catalog_songs/selectCatalogSongsWithArtists"; import { validateCatalogSongsQuery } from "@/lib/songs/validateCatalogSongsQuery"; -import { authorizeCatalogAccess } from "@/lib/songs/authorizeCatalogAccess"; -import { validateAuthContext } from "@/lib/auth/validateAuthContext"; /** * Handler for retrieving catalog songs with pagination. @@ -19,24 +17,11 @@ import { validateAuthContext } from "@/lib/auth/validateAuthContext"; */ export async function getCatalogSongsHandler(request: NextRequest): Promise { try { - // Authenticate before touching the request, so a caller with no - // credentials gets 401 rather than a query-validation 400 (chat#1912 row 6). - const auth = await validateAuthContext(request); - if (auth instanceof NextResponse) return auth; - - const { searchParams } = new URL(request.url); - - const validatedQuery = validateCatalogSongsQuery(searchParams); + const validatedQuery = await validateCatalogSongsQuery(request); if (validatedQuery instanceof NextResponse) { return validatedQuery; } - // Catalog songs are account-scoped (contract in recoupable/docs#282): this - // read was previously open to anyone holding a catalog id, while the - // sibling /measurements endpoint required credentials. - const forbidden = await authorizeCatalogAccess(auth.accountId, [validatedQuery.catalog_id]); - if (forbidden) return forbidden; - // Fetch catalog songs with pagination const result = await selectCatalogSongsWithArtists({ catalogId: validatedQuery.catalog_id, diff --git a/lib/songs/validateCatalogSongsQuery.ts b/lib/songs/validateCatalogSongsQuery.ts index bc36bd44e..dde892502 100644 --- a/lib/songs/validateCatalogSongsQuery.ts +++ b/lib/songs/validateCatalogSongsQuery.ts @@ -1,5 +1,7 @@ -import { NextResponse } from "next/server"; +import { NextRequest, NextResponse } from "next/server"; import { getCorsHeaders } from "@/lib/networking/getCorsHeaders"; +import { validateAuthContext } from "@/lib/auth/validateAuthContext"; +import { authorizeCatalogAccess } from "@/lib/songs/authorizeCatalogAccess"; import { z } from "zod"; export const catalogSongsQuerySchema = z.object({ @@ -21,15 +23,28 @@ export const catalogSongsQuerySchema = z.object({ export type CatalogSongsQuery = z.infer; +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 85da95cba..63f11b8ac 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 }; }