fix(catalogs): catalog songs require authentication and ownership (chat#1912 row 6)

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");
+  });
+});

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"]);
+  });
+});

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<NextResponse | null> {
+  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() },
+  );
+}

lib/songs/createCatalogSongsHandler.ts

@@ -20,10 +20,7 @@ import { validateCatalogSongsRequest } from "@/lib/songs/validateCatalogSongsReq
  */
 export async function createCatalogSongsHandler(request: NextRequest): Promise<NextResponse> {
   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<N
     return NextResponse.json(
       {
         status: "error",
-        error: error instanceof Error ? error.message : "Internal server error",
+        // Fixed message: this path can now surface auth and database
+        // failures, whose text must not reach the caller.
+        error: "Internal server error",
       },
       {
         status: 500,

lib/songs/deleteCatalogSongsHandler.ts

@@ -18,10 +18,7 @@ import { validateCatalogSongsRequest } from "@/lib/songs/validateCatalogSongsReq
  */
 export async function deleteCatalogSongsHandler(request: NextRequest): Promise<NextResponse> {
   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<N
     return NextResponse.json(
       {
         status: "error",
-        error: error instanceof Error ? error.message : "Internal server error",
+        // Fixed message: this path can now surface auth and database
+        // failures, whose text must not reach the caller.
+        error: "Internal server error",
       },
       {
         status: 500,

lib/songs/getCatalogSongsHandler.ts

@@ -17,9 +17,7 @@ import { validateCatalogSongsQuery } from "@/lib/songs/validateCatalogSongsQuery
  */
 export async function getCatalogSongsHandler(request: NextRequest): Promise<NextResponse> {
   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<Next
     return NextResponse.json(
       {
         status: "error",
-        error: error instanceof Error ? error.message : "Internal server error",
+        // Fixed message: this path can now surface auth and database
+        // failures, whose text must not reach the caller.
+        error: "Internal server error",
       },
       {
         status: 500,

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<typeof catalogSongsQuerySchema>;
 
+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<NextResponse | ValidatedCatalogSongsQuery> {
+  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 };
 }

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<typeof catalogSongsRequestSchema>;
 
+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<NextResponse | ValidatedCatalogSongsRequest> {
+  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 };
 }