fix(catalogs): catalog songs require authentication and ownership (chat#1912 row 6) - #802
Conversation
…at#1912 row 6) 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) <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughCatalog song handlers now use asynchronous request validation with authentication, catalog ownership authorization, validated account context, and fixed internal error responses. ChangesCatalog access authorization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant NextRequest
participant CatalogValidator
participant validateAuthContext
participant authorizeCatalogAccess
participant CatalogSongsHandler
NextRequest->>CatalogSongsHandler: catalog song request
CatalogSongsHandler->>CatalogValidator: validate request or query
CatalogValidator->>validateAuthContext: authenticate request
validateAuthContext-->>CatalogValidator: accountId
CatalogValidator->>authorizeCatalogAccess: requested catalog IDs
authorizeCatalogAccess-->>CatalogValidator: authorization result
CatalogValidator-->>CatalogSongsHandler: validated data or response
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files
Confidence score: 3/5
- In
lib/songs/createCatalogSongsHandler.ts, bulk authenticated requests can trigger unbounded concurrent ownership checks, which may exhaust the Supabase connection pool and cause catalog-song creation to fail or time out under load—cap request/catalog size or add explicit concurrency limits for the ownership query fan-out.
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/songs/createCatalogSongsHandler.ts">
<violation number="1" location="lib/songs/createCatalogSongsHandler.ts:36">
P2: A large authenticated bulk request can fan out into an unbounded number of simultaneous ownership queries, exhausting the Supabase connection pool before the write runs. Bound the request/catalog count or replace the per-catalog Promise.all lookup with a single set-based ownership query.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client
participant Handler as API Handler (GET/POST/DELETE /api/catalogs/songs)
participant Validate as validateCatalogSongsBodyOrQuery
participant AuthGate as authorizeCatalogAccess
participant Auth as validateAuthContext
participant SelectCat as selectAccountCatalog (Supabase)
participant DB as Database
Note over Client,DB: NEW: Authorization gate for all catalog-songs operations
Client->>Handler: Request (GET/POST/DELETE)
Handler->>Validate: Validate body or query schema
alt Invalid body/query
Validate->>Handler: 400 response
Handler->>Client: 400
else Valid
Validate->>Handler: validated data (includes catalog IDs)
Handler->>AuthGate: authorizeCatalogAccess(request, catalogIds)
AuthGate->>Auth: authenticate request (bearer / x-api-key)
alt No credentials or invalid
Auth->>AuthGate: 401 NextResponse
AuthGate->>Handler: 401 response
Handler->>Client: 401
else Authenticated
Auth->>AuthGate: { accountId }
Note over AuthGate,SelectCat: Check every unique catalog_id in the operation
loop For each unique catalogId
AuthGate->>SelectCat: selectAccountCatalog({ accountId, catalogId })
SelectCat->>DB: Query account_catalogs table
DB-->>SelectCat: row or null
SelectCat-->>AuthGate: link or null
end
alt Any catalog not owned (null)
AuthGate->>Handler: 403 response (catalog does not belong to account)
Handler->>Client: 403
else All owned
AuthGate->>Handler: { accountId }
Note over Handler,DB: Existing handler logic (unchanged)
alt GET
Handler->>DB: selectCatalogSongsWithArtists(...)
DB-->>Handler: songs
Handler->>Client: 200 + songs
else POST
Handler->>DB: processSongsInput + insert
DB-->>Handler: created records
Handler->>Client: 200
else DELETE
Handler->>DB: deleteCatalogSongs(...)
DB-->>Handler: affected rows
Handler->>Client: 200
end
end
end
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // row 6). This write previously enforced nothing at all. | ||
| const authorized = await authorizeCatalogAccess( | ||
| request, | ||
| validatedBody.songs.map(song => song.catalog_id).filter((id): id is string => !!id), |
There was a problem hiding this comment.
P2: A large authenticated bulk request can fan out into an unbounded number of simultaneous ownership queries, exhausting the Supabase connection pool before the write runs. Bound the request/catalog count or replace the per-catalog Promise.all lookup with a single set-based ownership query.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/songs/createCatalogSongsHandler.ts, line 36:
<comment>A large authenticated bulk request can fan out into an unbounded number of simultaneous ownership queries, exhausting the Supabase connection pool before the write runs. Bound the request/catalog count or replace the per-catalog Promise.all lookup with a single set-based ownership query.</comment>
<file context>
@@ -28,6 +29,14 @@ export async function createCatalogSongsHandler(request: NextRequest): Promise<N
+ // row 6). This write previously enforced nothing at all.
+ const authorized = await authorizeCatalogAccess(
+ request,
+ validatedBody.songs.map(song => song.catalog_id).filter((id): id is string => !!id),
+ );
+ if (authorized instanceof NextResponse) return authorized;
</file context>
…e query 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) <noreply@anthropic.com>
Preview verification — 2026-07-30Preview https://api-c3tzvslpp-recoup.vercel.app, built from head Fixtures: catalog A = Results
Rows 2, 3 and 9 together are the ordering proof: with no credentials a malformed body now returns 401, and only once authenticated does the same body return 400. That is Row 6 was checked against the database, not just the response. After the cross-account Review findings — all four valid, all fixed
Blast radius, restated after verificationRow 1 is a behaviour change on a currently-open read. Anything calling Suggested merge order from here: this → chat#1917, so the copy stops promising the songs tab as soon as it starts refusing. Full suite 4296 passed / 792 files, Test residueThe verification created one preview account ( |
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Confidence score: 4/5
- In
lib/songs/createCatalogSongsHandler.ts, the 113-line handler exceeds the maintainability guideline, which raises the chance of future regressions because validation, orchestration, and response logic are harder to reason about in one place—extract focused helper functions (or service-layer calls) to keep the route handler lean.
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/songs/createCatalogSongsHandler.ts">
<violation number="1" location="lib/songs/createCatalogSongsHandler.ts:9">
P2: Custom agent: **Enforce Clear Code Style and Maintainability Practices**
This handler file is 113 lines, exceeding the 100-line limit set by our maintainability guidelines. Keeping route handlers lean makes them easier to scan and test. Consider moving the data transformation, bulk insertion, and result filtering steps into one or more dedicated helpers so the handler stays focused on orchestration and auth.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| import { getCorsHeaders } from "@/lib/networking/getCorsHeaders"; | ||
| import { insertCatalogSongs } from "@/lib/supabase/catalog_songs/insertCatalogSongs"; | ||
| import { selectCatalogSongsWithArtists } from "@/lib/supabase/catalog_songs/selectCatalogSongsWithArtists"; | ||
| import { processSongsInput } from "@/lib/songs/processSongsInput"; |
There was a problem hiding this comment.
P2: Custom agent: Enforce Clear Code Style and Maintainability Practices
This handler file is 113 lines, exceeding the 100-line limit set by our maintainability guidelines. Keeping route handlers lean makes them easier to scan and test. Consider moving the data transformation, bulk insertion, and result filtering steps into one or more dedicated helpers so the handler stays focused on orchestration and auth.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/songs/createCatalogSongsHandler.ts, line 9:
<comment>This handler file is 113 lines, exceeding the 100-line limit set by our maintainability guidelines. Keeping route handlers lean makes them easier to scan and test. Consider moving the data transformation, bulk insertion, and result filtering steps into one or more dedicated helpers so the handler stays focused on orchestration and auth.</comment>
<file context>
@@ -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";
/**
</file context>
| @@ -28,6 +35,14 @@ | |||
| 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; | |||
There was a problem hiding this comment.
SRP / OCP
- actual: 2 distinct auth / validation calls in the handler function.
- required: move both validateAuthContext and authorizeCatalogAccess calls to a standalone validate function.
There was a problem hiding this comment.
Done in a8098fab. Both calls moved into validateCatalogSongsRequest, which POST and DELETE already shared, so there is one standalone validate function rather than a new one per verb.
It now takes the request and runs credentials → body shape → ownership, returning either the 401/400/403 response or the validated body plus the authenticated accountId. Body parsing moved in with it, via the shared safeParseJson.
The handler is back to one validation call followed by business logic:
const validatedBody = await validateCatalogSongsRequest(request);
if (validatedBody instanceof NextResponse) return validatedBody;Keeping the order inside the validator also makes it testable as a unit, which it now is: 401 before 400, ownership never consulted for an invalid body, and every catalog named in the body checked rather than only the first.
| @@ -26,6 +33,14 @@ export async function deleteCatalogSongsHandler(request: NextRequest): Promise<N | |||
| 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), | |||
| ); | |||
There was a problem hiding this comment.
SRP / OCP
- actual: 2 distinct auth / validation calls in the handler function.
- required: move both validateAuthContext and authorizeCatalogAccess calls to a standalone validate function.
There was a problem hiding this comment.
Done in a8098fab. Both calls moved into validateCatalogSongsRequest, which POST and DELETE already shared, so there is one standalone validate function rather than a new one per verb.
It now takes the request and runs credentials → body shape → ownership, returning either the 401/400/403 response or the validated body plus the authenticated accountId. Body parsing moved in with it, via the shared safeParseJson.
The handler is back to one validation call followed by business logic:
const validatedBody = await validateCatalogSongsRequest(request);
if (validatedBody instanceof NextResponse) return validatedBody;Keeping the order inside the validator also makes it testable as a unit, which it now is: 401 before 400, ownership never consulted for an invalid body, and every catalog named in the body checked rather than only the first.
| // 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); | ||
| 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; |
There was a problem hiding this comment.
SRP / OCP
- actual: 2 distinct auth / validation calls in the handler function.
- required: move both validateAuthContext and authorizeCatalogAccess calls to validateCatalogSongsQuery.
There was a problem hiding this comment.
Done in a8098fab. Both moved into validateCatalogSongsQuery as asked.
It takes the request rather than URLSearchParams and runs credentials → query shape → ownership, returning either the 401/400/403 response or the validated query plus the authenticated accountId. The handler is one call again:
const validatedQuery = await validateCatalogSongsQuery(request);
if (validatedQuery instanceof NextResponse) return validatedQuery;Signature change is contained: this validator had no callers outside the GET handler.
…view) 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) <noreply@anthropic.com>
Re-verified after the refactor — 2026-07-30Preview https://api-hh8sa62lq-recoup.vercel.app, built from head
Row 5 is new and worth calling out: malformed JSON with no credentials is now 401. Previously the body was parsed first, so Row 11 was checked in the database rather than inferred from the 403. What changed since the last runHandlers each made two auth/validation calls; both now live in the validate function the handler already used, per review. Added Full suite 4301 passed / 793 files, Test residue: one preview account and one empty catalog with a single song. No customer data modified, per row 11. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
lib/songs/createCatalogSongsHandler.ts (2)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a shared sanitized-error-response helper. All three handlers now duplicate the same catch-all
NextResponse.json({ status: "error", error: "Internal server error" }, { status: 500, headers: getCorsHeaders() })block — one root cause (no shared helper) repeated three times.
lib/songs/createCatalogSongsHandler.ts#L80-94: replace this catch block's response construction with a sharedinternalErrorResponse()helper call.lib/songs/deleteCatalogSongsHandler.ts#L53-67: replace this catch block's response construction with the same shared helper.lib/songs/getCatalogSongsHandler.ts#L52-66: replace this catch block's response construction with the same shared helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/songs/createCatalogSongsHandler.ts` at line 1, Extract a shared internalErrorResponse() helper that returns the existing sanitized 500 NextResponse with getCorsHeaders(), then replace the duplicated catch-block response construction in the handlers’ catch paths with calls to this helper.
80-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSanitized 500 response duplicated across handlers.
The catch-all
"Internal server error"response body/status/headers block is now identical increateCatalogSongsHandler.ts,deleteCatalogSongsHandler.ts, andgetCatalogSongsHandler.ts. Extracting a shared helper (e.g.internalErrorResponse()) would keep the sanitization fix in one place going forward.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/songs/createCatalogSongsHandler.ts` around lines 80 - 94, Extract the duplicated sanitized 500 response into a shared internalErrorResponse helper and update createCatalogSongsHandler, deleteCatalogSongsHandler, and getCatalogSongsHandler to return it from their catch blocks. Preserve the existing generic error body, 500 status, and CORS headers, while keeping detailed errors limited to server-side logging.lib/songs/deleteCatalogSongsHandler.ts (1)
53-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSanitized 500 response duplicated (see createCatalogSongsHandler.ts).
Same catch-all error block as the other two handlers — candidate for a shared helper.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/songs/deleteCatalogSongsHandler.ts` around lines 53 - 67, Extract the duplicated sanitized 500 response from the catch block in the catalog song handlers into a shared helper, then reuse that helper from deleteCatalogSongsHandler and the corresponding create handler. Preserve the existing internal error logging, generic client-facing message, 500 status, and CORS headers.lib/songs/getCatalogSongsHandler.ts (1)
52-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSanitized 500 response duplicated (see createCatalogSongsHandler.ts).
Same catch-all error block as the other two handlers — candidate for a shared helper.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/songs/getCatalogSongsHandler.ts` around lines 52 - 66, Extract the duplicated sanitized 500 response construction from the catch block in the catalog songs handler into a shared helper, reusing the existing equivalent helper or adding one alongside createCatalogSongsHandler. Update this handler and the other matching handlers to call it while preserving the Internal server error message, 500 status, and CORS headers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/songs/validateCatalogSongsRequest.ts`:
- Around line 23-46: Rename the validator file to validateCatalogSongsBody.ts
and rename the exported function validateCatalogSongsRequest to
validateCatalogSongsBody. Update both call sites and all imports/references to
use the new filename and symbol, preserving the existing validation behavior and
API.
---
Nitpick comments:
In `@lib/songs/createCatalogSongsHandler.ts`:
- Line 1: Extract a shared internalErrorResponse() helper that returns the
existing sanitized 500 NextResponse with getCorsHeaders(), then replace the
duplicated catch-block response construction in the handlers’ catch paths with
calls to this helper.
- Around line 80-94: Extract the duplicated sanitized 500 response into a shared
internalErrorResponse helper and update createCatalogSongsHandler,
deleteCatalogSongsHandler, and getCatalogSongsHandler to return it from their
catch blocks. Preserve the existing generic error body, 500 status, and CORS
headers, while keeping detailed errors limited to server-side logging.
In `@lib/songs/deleteCatalogSongsHandler.ts`:
- Around line 53-67: Extract the duplicated sanitized 500 response from the
catch block in the catalog song handlers into a shared helper, then reuse that
helper from deleteCatalogSongsHandler and the corresponding create handler.
Preserve the existing internal error logging, generic client-facing message, 500
status, and CORS headers.
In `@lib/songs/getCatalogSongsHandler.ts`:
- Around line 52-66: Extract the duplicated sanitized 500 response construction
from the catch block in the catalog songs handler into a shared helper, reusing
the existing equivalent helper or adding one alongside
createCatalogSongsHandler. Update this handler and the other matching handlers
to call it while preserving the Internal server error message, 500 status, and
CORS headers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 207b2781-1456-457b-826d-17479dfc7657
⛔ Files ignored due to path filters (2)
lib/songs/__tests__/authorizeCatalogAccess.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/songs/__tests__/validateCatalogSongsRequest.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (6)
lib/songs/authorizeCatalogAccess.tslib/songs/createCatalogSongsHandler.tslib/songs/deleteCatalogSongsHandler.tslib/songs/getCatalogSongsHandler.tslib/songs/validateCatalogSongsQuery.tslib/songs/validateCatalogSongsRequest.ts
| 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); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
File/function naming violates validate*.ts convention.
This file validates the request body but is named validateCatalogSongsRequest.ts / exports validateCatalogSongsRequest, not validateCatalogSongsBody.ts / validateCatalogSongsBody as the path instruction for lib/**/validate*.ts requires.
As per path instructions, lib/**/validate*.ts files must "Follow naming: validateBody.ts or validateQuery.ts".
♻️ Suggested rename
-export type ValidatedCatalogSongsRequest = CatalogSongsRequest & { accountId: string };
+export type ValidatedCatalogSongsBody = CatalogSongsRequest & { accountId: string };
-export async function validateCatalogSongsRequest(
+export async function validateCatalogSongsBody(
request: NextRequest,
-): Promise<NextResponse | ValidatedCatalogSongsRequest> {
+): Promise<NextResponse | ValidatedCatalogSongsBody> {Also rename the file to validateCatalogSongsBody.ts and update its two call sites.
#!/bin/bash
# Find all call sites/imports of validateCatalogSongsRequest to confirm rename scope
rg -nP '\bvalidateCatalogSongsRequest\b' --type=ts🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/songs/validateCatalogSongsRequest.ts` around lines 23 - 46, Rename the
validator file to validateCatalogSongsBody.ts and rename the exported function
validateCatalogSongsRequest to validateCatalogSongsBody. Update both call sites
and all imports/references to use the new filename and symbol, preserving the
existing validation behavior and API.
Source: Path instructions
…1912 row 6) (#1917) * fix(catalog): cross-account copy stops promising the songs tab (chat#1912 row 6) Third link of the row 6 chain, after recoupable/docs#282 (contract) and recoupable/api#802 (enforcement). The other-account state said "The songs are listed under Manage songs, but the play counts and valuation belong to the account that measured them." Once catalog songs are account-scoped, that tab returns 403 for a non-owner, so the copy would be pointing them at something that now fails. Rewords to state what is true either way and offers an in-app next step instead. The existing guards still hold: no em dashes, and no CTA off to recoupable.dev. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(catalog): give the cross-account state a way forward, not just text Review (@sweetmantech): the state should offer a happy path rather than a paragraph and a dead end. It already had a button, but "Go to your catalogs" is only a happy path for someone who has catalogs. A stranger following a shared catalog link usually has none, so that button landed them on an empty page — a second dead end. The action now adapts: viewers with catalogs still go to theirs, and viewers with none get "Value your catalog" pointing at /setup/artists, the step that creates one. Body copy leads into the action instead of explaining the refusal twice. useOwnsCatalog already loads the viewer's catalog list for the ownership check, so the flag costs no extra request. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Implements the contract in recoupable/docs#282 — row 6 of chat#1912.
Why
All three catalog-songs operations enforced nothing. Verified on prod 2026-07-30 with no credentials at all:
The writes returning 400 rather than 401 is the tell: the request reached body validation, so it had already passed the auth layer — there wasn't one. Anyone holding a catalog id could read its tracks, add songs, or delete them. I stopped at the invalid-body probe rather than mutating real data.
Meanwhile
GET /api/catalogs/{catalogId}/measurementsreturns 401 for that same catalog. The catalog report page was built on that asymmetry: it tells a signed-in stranger "the play counts and valuation belong to the account that measured them" on the very page that lists the catalog's songs.What changed
New
lib/songs/authorizeCatalogAccess.ts, gating all three handlers:validateAuthContext→ 401 without credentialsselectAccountCatalogownership check → 403 for a catalog the caller does not ownWrites can name several catalogs in one body, so every distinct catalog id in
songs[]is checked. Authorizing only the first would let one owned catalog carry edits into catalogs the caller does not own — there is a test for exactly that.Blast radius worth reviewing
This closes a read that is currently open, so any caller relying on unauthenticated
GET /api/catalogs/songswill start getting 401. Known consumer: chat's catalog report Manage songs tab, which already sends a Privy bearer, so it is unaffected for the owner. A non-owner viewing a shared catalog URL will now see the songs tab fail — which is the intended consequence of the decision, and the reason row 6 also carries a chat-side copy change (the current cross-account wording promises songs that will no longer render).Tests
TDD red→green.
lib/songs/__tests__/authorizeCatalogAccess.test.ts— 5 cases: no credentials → 401 (and ownership is never queried), unowned catalog → 403, owned →{ accountId }, one unowned among several → 403, and ownership always checked against the authenticated account rather than a supplied one.Full suite 4296 passed / 792 files,
lint:checkandformat:checkclean,tsc --noEmitat the repo's existing 236-error baseline with none in the touched files.Verification
Preview verification against row 6's Works-when (unauthenticated parity between
/catalogs/songsand/measurements, plus an owner regression check) will be posted as a PR comment.🤖 Generated with Claude Code
Summary by cubic
Require authentication and account ownership for GET, POST, and DELETE on
/api/catalogs/songs, with a single validator per handler that runs auth before parsing. This closes the open read and aligns with recoupable/docs#282 (chat#1912 row 6).validateCatalogSongsQueryandvalidateCatalogSongsRequestnow takeNextRequest, runvalidateAuthContextfirst, parse (safeParseJson), validate input, thenauthorizeCatalogAccess(401 → 400 → 403). They return the validated input plus the authenticatedaccountId.accountId, verify every catalog id, and read catalogs once viaselectAccountCatalogs; DB failures now surface as 500, not a false 403./api/catalogs/{id}/measurements), and 500 responses use a generic message.Written for commit a8098fa. Summary will update on new commits.
Summary by CodeRabbit
Security
Bug Fixes