docs(catalogs): catalog songs require authentication (chat#1912 row 6) - #282
Merged
Conversation
GET /api/catalogs/songs is documented and implemented as unauthenticated
while GET /api/catalogs/{catalogId}/measurements requires credentials, so the
catalog report page tells a stranger the valuation belongs to another account
on the same screen that hands them every track and ISRC.
Verified on prod 2026-07-30 that POST and DELETE are unauthenticated too:
both reach body validation and return 400 with no credentials, so anyone
holding a catalog id can add or remove its songs.
This is the contract half of the decision, written first so it can be
reviewed before any code moves: a catalog is readable and writable only by
the accounts it belongs to. All three operations gain apiKeyAuth/bearerAuth
plus 401 and 403, matching the measurements endpoint.
If we would rather catalogs be readable by anyone holding the link, this is
the PR to reject — the api change follows from whatever lands here.
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe OpenAPI specification for ChangesCatalog songs API contract
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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}
This was referenced Jul 30, 2026
Collaborator
Author
Rendered verification — 2026-07-30
Results
Tab ordering was worth checking rather than assuming: a first pass at this edit inserted ScreenshotsGET — Authorizations section and the cURL sample now carrying the key:
GET — 401 selected, description renders:
POST:
DELETE — note the
DELETE — 403 selected:
What this does not verifyThis is a spec change. Rendering it proves the contract now says these operations require credentials; it does not make them require credentials. As of this comment, prod still answers unauthenticated Until api#802 lands, these pages describe intended behaviour rather than current behaviour. That is the normal state for a contract-first PR, but it is worth stating plainly rather than leaving a reader to assume the docs match the API today. Still the decision pointMerging this ratifies catalogs are account-scoped. The alternative (link-readable catalogs) is defensible for reads and coherent with the valuation funnel, but cannot justify unauthenticated writes. Reject this PR and I will invert the chain. |
sweetmantech
added a commit
to recoupable/api
that referenced
this pull request
Jul 30, 2026
…at#1912 row 6) (#802) * fix(catalogs): catalog songs require authentication and ownership (chat#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> * fix(catalogs): authenticate before parsing, and check ownership in one 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> * 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) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
sweetmantech
added a commit
to recoupable/chat
that referenced
this pull request
Jul 30, 2026
…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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Contract for row 6 of chat#1912. Docs first, deliberately — this PR is the decision, and the api change follows from whatever lands here.
Why
The two tabs of the same page answer the same question differently. Measured on prod 2026-07-30 against a real catalog:
GET /api/catalogs/{catalogId}/measurements?limit=1GET /api/catalogs/songs?catalog_id=…&page=1&limit=100So
/catalogs/{id}tells a signed-in stranger "the play counts and valuation belong to the account that measured them" while the Manage songs tab hands them every track and ISRC. We over-share the tracklist and under-share the valuation on one screen.It is worse than a read-side leak. The spec has
security: Noneon the write operations too, and prod confirms it — with no credentials at all:Both reach body validation, which means auth was never enforced. Anyone holding a catalog id can add or remove that catalog's songs. I stopped at the invalid-body probe rather than mutating real data, but that response is sufficient proof the request passed the auth layer.
The decision this encodes
A catalog is readable and writable only by the accounts it belongs to.
That is the conservative reading, and it matches what the product already tells users. The alternative is defensible — an ungated catalog is coherent with the valuation funnel, where the ungated number is the acquisition hook — but it would mean opening the measurements read rather than closing the songs read, and it cannot justify unauthenticated writes.
If you want link-readable catalogs, reject this PR and I will invert it. What is not defensible is the current state, where the page contradicts itself.
What changed
Purely additive to
api-reference/openapi/releases.json(84 insertions, 0 deletions):GET,POSTandDELETE /api/catalogs/songseach gainsecurity: [apiKeyAuth, bearerAuth], matching the measurements endpoint401— missing or invalid credentials403— the catalog does not belong to the authenticated accountBoth responses reuse the existing
CatalogSongsErrorResponseschema rather than introducing a new one. Responses are ordered200, 400, 401, 403[, 404]so the rendered tabs stay in numeric order.The file does not round-trip through
json.dumpsbyte-for-byte, so the edits were made as anchored text insertions at brace boundaries and the result re-validated withjson.load.Sequencing
docs → api → chat. This PR is step 1. The api change (enforce
validateAuthContexton all three operations, scope to the caller's catalogs) and any chat copy reconciliation follow it, and row 6's Works-when checks unauthenticated parity between the two endpoints.Not verified here
No preview run: this is a spec-only change and Mintlify renders auth from the spec. Happy to run
npx mintlify@latest devand post rendered screenshots of the three pages if you want that before merge.🤖 Generated with Claude Code
Summary by cubic
Require authentication for catalog songs endpoints and limit access to the owning account. Resolves chat#1912 row 6 by closing unauthenticated reads and writes on the catalog songs page.
apiKeyAuthandbearerAuthtoGET,POST, andDELETE /api/catalogs/songs.401and403responses usingCatalogSongsErrorResponse.Written for commit 1b0246f. Summary will update on new commits.