fix(accounts): authenticate POST /api/accounts/artists (P0a)#771
Conversation
Require validateAuthContext on the roster-link endpoint; derive the target account from the credential. The legacy email body field is now an optional override gated by checkAccountAccess. Fixes P0a in recoupable/chat#1860. Co-Authored-By: Claude Fable 5 <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.
|
|
Warning Review limit reached
Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (6)
✨ 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}
There was a problem hiding this comment.
3 issues found across 7 files
Confidence score: 3/5
lib/accounts/resolveAddArtistAccountId.tscurrently returns different outcomes for unknown vs inaccessible emails (404 vs 403), which lets authenticated callers infer whether an account/email exists; merging as-is keeps an account-enumeration side channel in place — return a single generic denial response for both cases before merging.lib/accounts/__tests__/addArtistToAccountHandler.test.tshighlights that handler errors can surface raw exception text (e.g.,"boom") in JSON, so internal messages could leak to clients and create avoidable information disclosure risk — sanitize error responses to a generic message and update the 400-path assertion to verify body content.app/api/accounts/artists/__tests__/route.test.tsis missing coverage for the allowedemailoverride path, so a key authorization/lookup flow can regress without test detection — add the accepted-path test to lock expected behavior before merge.
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/accounts/resolveAddArtistAccountId.ts">
<violation number="1" location="lib/accounts/resolveAddArtistAccountId.ts:27">
P2: The email override still exposes account existence to any authenticated caller: unknown emails return the resolver's 404, while existing inaccessible emails return 403. Consider returning the same generic denial response for unresolved and inaccessible emails so this endpoint cannot be used for account/email enumeration.</violation>
</file>
<file name="lib/accounts/__tests__/addArtistToAccountHandler.test.ts">
<violation number="1" location="lib/accounts/__tests__/addArtistToAccountHandler.test.ts:58">
P2: The 400-error test asserts `res.status` but doesn't verify the response body. The handler leaks raw error messages — `new Error("boom")` becomes `{ message: "boom" }` in the JSON response. As reported in team feedback, exception text (stack traces, DB errors, etc.) should never appear in API responses; the server should log the full error and return a hardcoded message.</violation>
</file>
<file name="app/api/accounts/artists/__tests__/route.test.ts">
<violation number="1" location="app/api/accounts/artists/__tests__/route.test.ts:80">
P2: Missing test for the email override accepted path. The test suite covers the `email` denied case (403) and the no-email case (resolver returns same accountId), but not the `email` accepted case where `resolveAddArtistAccountId` returns a *different* accountId than the auth context. Adding this would verify that the route correctly passes through the resolver's output to the handler rather than inadvertently using the auth accountId.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client
participant Route as POST /api/accounts/artists
participant Auth as validateAuthContext
participant Resolver as resolveAddArtistAccountId
participant Access as checkAccountAccess
participant DB as Database (Supabase)
participant Handler as addArtistToAccountHandler
Note over Client,Handler: NEW: Authenticated flow for linking artist to account
Client->>Route: POST { artistId, ?email }
Route->>Route: validateAddArtistBody()
alt Body invalid (missing or bad artistId)
Route-->>Client: 400 Bad Request
end
Route->>Auth: validateAuthContext(req)
alt No/ambiguous credential
Auth-->>Route: 401 NextResponse
Route-->>Client: 401 Unauthorized
else Credential valid
Auth-->>Route: { accountId, authToken }
end
Route->>Resolver: resolveAddArtistAccountId(accountId, email)
alt No email given
Resolver-->>Route: authenticatedAccountId (default)
else Email provided
Resolver->>DB: resolveAccountIdByEmail(email)
alt Email not found
DB-->>Resolver: 404 NextResponse
Resolver-->>Route: 404
Route-->>Client: 404 Not Found
else Email found
DB-->>Resolver: targetAccountId
alt targetAccountId == authenticatedAccountId
Resolver-->>Route: targetAccountId (self)
else Different account
Resolver->>Access: checkAccountAccess(authId, targetId)
alt Access granted
Access-->>Resolver: { hasAccess: true }
Resolver-->>Route: targetAccountId
else Access denied
Access-->>Resolver: { hasAccess: false }
Resolver-->>Route: 403 NextResponse
Route-->>Client: 403 Forbidden
end
end
end
end
Route->>Handler: addArtistToAccountHandler({ accountId, artistId })
Handler->>DB: getAccountArtistIds([accountId])
alt Artist already linked
DB-->>Handler: existing link
Handler-->>Route: 200 (idempotent)
else Artist not yet linked
DB-->>Handler: no duplicate
Handler->>DB: insertAccountArtistId(accountId, artistId)
alt Insert succeeds
DB-->>Handler: ok
Handler-->>Route: 200 Success
else DB error
DB-->>Handler: error
Handler-->>Route: 400 Bad Request
end
end
Route-->>Client: JSON response
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| const targetAccountId = await resolveAccountIdByEmail(email); | ||
| if (targetAccountId instanceof NextResponse) { | ||
| return targetAccountId; |
There was a problem hiding this comment.
P2: The email override still exposes account existence to any authenticated caller: unknown emails return the resolver's 404, while existing inaccessible emails return 403. Consider returning the same generic denial response for unresolved and inaccessible emails so this endpoint cannot be used for account/email enumeration.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/accounts/resolveAddArtistAccountId.ts, line 27:
<comment>The email override still exposes account existence to any authenticated caller: unknown emails return the resolver's 404, while existing inaccessible emails return 403. Consider returning the same generic denial response for unresolved and inaccessible emails so this endpoint cannot be used for account/email enumeration.</comment>
<file context>
@@ -0,0 +1,43 @@
+
+ const targetAccountId = await resolveAccountIdByEmail(email);
+ if (targetAccountId instanceof NextResponse) {
+ return targetAccountId;
+ }
+
</file context>
|
|
||
| const res = await addArtistToAccountHandler({ accountId: ACCOUNT_ID, artistId: ARTIST_ID }); | ||
|
|
||
| expect(res.status).toBe(400); |
There was a problem hiding this comment.
P2: The 400-error test asserts res.status but doesn't verify the response body. The handler leaks raw error messages — new Error("boom") becomes { message: "boom" } in the JSON response. As reported in team feedback, exception text (stack traces, DB errors, etc.) should never appear in API responses; the server should log the full error and return a hardcoded message.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/accounts/__tests__/addArtistToAccountHandler.test.ts, line 58:
<comment>The 400-error test asserts `res.status` but doesn't verify the response body. The handler leaks raw error messages — `new Error("boom")` becomes `{ message: "boom" }` in the JSON response. As reported in team feedback, exception text (stack traces, DB errors, etc.) should never appear in API responses; the server should log the full error and return a hardcoded message.</comment>
<file context>
@@ -0,0 +1,60 @@
+
+ const res = await addArtistToAccountHandler({ accountId: ACCOUNT_ID, artistId: ARTIST_ID });
+
+ expect(res.status).toBe(400);
+ });
+});
</file context>
| }); | ||
| }); | ||
|
|
||
| it("returns the resolver error when email override is denied", async () => { |
There was a problem hiding this comment.
P2: Missing test for the email override accepted path. The test suite covers the email denied case (403) and the no-email case (resolver returns same accountId), but not the email accepted case where resolveAddArtistAccountId returns a different accountId than the auth context. Adding this would verify that the route correctly passes through the resolver's output to the handler rather than inadvertently using the auth accountId.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/api/accounts/artists/__tests__/route.test.ts, line 80:
<comment>Missing test for the email override accepted path. The test suite covers the `email` denied case (403) and the no-email case (resolver returns same accountId), but not the `email` accepted case where `resolveAddArtistAccountId` returns a *different* accountId than the auth context. Adding this would verify that the route correctly passes through the resolver's output to the handler rather than inadvertently using the auth accountId.</comment>
<file context>
@@ -0,0 +1,95 @@
+ });
+ });
+
+ it("returns the resolver error when email override is denied", async () => {
+ vi.mocked(validateAuthContext).mockResolvedValue({
+ accountId: AUTH_ACCOUNT_ID,
</file context>
Preview verification — P0a: authenticate
|
| # | Path | Request | Documented | Actual | Verdict |
|---|---|---|---|---|---|
| 1 | Auth gate (the fix) | no credential, valid body {artistId} |
401 | HTTP 401 {"error":"Exactly one of x-api-key or Authorization must be provided"} |
✅ unauthenticated write closed |
| 2 | Auth gate | bad Bearer token | 401 | HTTP 401 {"message":"Failed to verify authentication token"} |
✅ |
| 3 | Validation | no auth, {} (missing artistId) |
400 | HTTP 400 missing_fields:["artistId"] |
✅ (body validation precedes auth) |
| 4 | Validation | {artistId:"not-a-uuid"} |
400 | HTTP 400 "artistId must be a valid UUID" |
✅ |
| 5 | Happy path | authed, {artistId:<already-rostered>} |
200 | HTTP 200 {"success":true} |
✅ |
| 6 | Email override, not found | authed, {artistId, email:<nonexistent>} |
404 | HTTP 404 {"error":"No account found for the provided email"} |
✅ |
| 7 | No mutation | GET /api/artists after row 5 |
roster unchanged | roster count: 1 → ['Lady Gaga'] |
✅ idempotent, no row added |
| 8 | Email override denied | non-staff caller, {artistId, email:<foreign account>} |
403 | HTTP 403 {"error":"Access denied to the account for the provided email"} |
✅ override gated by checkAccountAccess |
Notes
- Check ordering: body validation (400) runs before auth (401) — a malformed body 400s even unauthenticated (rows 3–4). Auth-then-resolve for valid bodies (rows 1, 5, 6, 8). Consistent with the diff.
- The
emailoverride survives only as an access-gated path: self / accessible account → resolves (row 5-style), foreign account → 403 (row 8), unknown email → 404 (row 6). Derives the target from the credential otherwise — never from unauthenticatedemail. - Per the issue, this needs a follow-up docs update (
add-artist.mdx: auth now required; document 401/403/404). Not in this PR's scope.
Verdict
The unauthenticated roster-write hole is closed (401), input validation and the access-gated email override behave as documented (400/403/404), and the happy path is idempotent with no unintended mutation. No regressions observed.
🤖 Generated with Claude Code
There was a problem hiding this comment.
SRP
- actual: handler code written in route.ts
- required: handler + validator functions to match other endpoint architecture.
There was a problem hiding this comment.
Fixed in c2e35716. Extracted the orchestration to match the connectors endpoint architecture:
route.ts→ one-line delegate:return addArtistToAccountHandler(req)addArtistToAccountHandler(request)→ thin request handlervalidateAddArtistRequest(request)→ validator (body + auth + target-account resolution →{accountId, artistId}or error)linkArtistToAccount({accountId, artistId})→ business/DB step
Behavior preserved (400 body-first → 401 → 403/404 → link). 60 accounts tests green; tsc + lint + prettier clean on touched files.
|
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. |
… validator (SRP)
Route was carrying orchestration (parse, body validation, auth, account
resolution) inline. Mirror the connectors endpoint architecture:
- route.ts -> one-line delegate to addArtistToAccountHandler(request)
- addArtistToAccountHandler(request): thin request handler
- validateAddArtistRequest(request): bundles body + auth + target-account
resolution, returns { accountId, artistId } or a NextResponse error
- linkArtistToAccount({ accountId, artistId }): the business/DB step
Behavior preserved (400 body-first, then 401, then 403/404, then link).
Also sanitize the link failure path to a generic 400 message + server log
(no raw exception text in the response). 60 accounts tests green.
Co-Authored-By: Claude Opus 4.8 <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. |
SRP refactor applied + re-verified liveAddressed the review note (handler code inline in
Mirrors Tests: 60 accounts tests green ( Re-verified live against the refactor preview
Remaining cubic P2 (not changed) — email enumeration (404 vs 403)cubic flagged that unknown email → 404 while inaccessible email → 403 lets an authenticated caller probe account existence. Left as-is deliberately: collapsing both to one generic denial is a behavior change to the documented contract (and would need the docs update to match). Flagging for a decision — happy to fold it in here or split to a follow-up. 🤖 Generated with Claude Code |
There was a problem hiding this comment.
2 issues found across 8 files (changes from recent commits).
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/accounts/linkArtistToAccount.ts">
<violation number="1" location="lib/accounts/linkArtistToAccount.ts:24">
P3: This existence check loads the account's full artist roster and nested artist/social details just to check one `(accountId, artistId)` pair. Consider using the existing `selectAccountArtistId(accountId, artistId)` helper or an equally narrow query to avoid unnecessary DB and network work on large rosters.</violation>
<violation number="2" location="lib/accounts/linkArtistToAccount.ts:32">
P2: Concurrent duplicate requests can break the endpoint's idempotency because both calls can observe no existing row before either insert commits. Prefer a DB-level upsert/on-conflict ignore or duplicate-key handling so repeated requests return success atomically.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
|
|
||
| // Add artist to account | ||
| await insertAccountArtistId(accountId, artistId); |
There was a problem hiding this comment.
P2: Concurrent duplicate requests can break the endpoint's idempotency because both calls can observe no existing row before either insert commits. Prefer a DB-level upsert/on-conflict ignore or duplicate-key handling so repeated requests return success atomically.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/accounts/linkArtistToAccount.ts, line 32:
<comment>Concurrent duplicate requests can break the endpoint's idempotency because both calls can observe no existing row before either insert commits. Prefer a DB-level upsert/on-conflict ignore or duplicate-key handling so repeated requests return success atomically.</comment>
<file context>
@@ -0,0 +1,42 @@
+ }
+
+ // Add artist to account
+ await insertAccountArtistId(accountId, artistId);
+
+ return NextResponse.json({ success: true }, { status: 200, headers: getCorsHeaders() });
</file context>
| }: AddArtistParams): Promise<NextResponse> { | ||
| try { | ||
| // Check if artist is already associated with account | ||
| const existingArtists = await getAccountArtistIds({ accountIds: [accountId] }); |
There was a problem hiding this comment.
P3: This existence check loads the account's full artist roster and nested artist/social details just to check one (accountId, artistId) pair. Consider using the existing selectAccountArtistId(accountId, artistId) helper or an equally narrow query to avoid unnecessary DB and network work on large rosters.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/accounts/linkArtistToAccount.ts, line 24:
<comment>This existence check loads the account's full artist roster and nested artist/social details just to check one `(accountId, artistId)` pair. Consider using the existing `selectAccountArtistId(accountId, artistId)` helper or an equally narrow query to avoid unnecessary DB and network work on large rosters.</comment>
<file context>
@@ -0,0 +1,42 @@
+}: AddArtistParams): Promise<NextResponse> {
+ try {
+ // Check if artist is already associated with account
+ const existingArtists = await getAccountArtistIds({ accountIds: [accountId] });
+ const alreadyExists = existingArtists.some(a => a.artist_id === artistId);
+
</file context>
1 participant
Summary
Closes P0a in recoupable/chat#1860.
POST /api/accounts/artistswas completely unauthenticated: the handler resolved the target account purely from a caller-suppliedemailand wroteaccount_artist_idsrows, letting anyone link any account to any canonical artist (and that roster row doubles as act-as-artist connector authority).Changes
app/api/accounts/artists/route.ts: now requiresvalidateAuthContext(x-api-key or Bearer), matching sibling authenticated routes. Returns 401 with no/ambiguous credential.lib/accounts/resolveAddArtistAccountId.ts(new): the target account defaults to the authenticated account. The legacyemailbody field is now optional and gated: it only works whencheckAccountAccess(authAccountId, target)passes (self / managed artist / owned workspace / member org); otherwise 403.lib/accounts/addArtistToAccountHandler.ts: takes a resolved{ accountId, artistId }; no longer trusts email input for account resolution.lib/accounts/validateAddArtistBody.ts:emailis now optional;artistIdstill a required UUID.Caller impact
Grepped the mono root (
chat,marketing,docs,skills,cli) foraccounts/artists: no code callers found in chat or marketing. The only external reference isdocs/api-reference/accounts/add-artist.mdx(OpenAPI page), which will need a follow-up docs update:emailis now optional, auth is required, and new 401/403 responses exist. Any out-of-tree caller sending only{email, artistId}without a credential will now get 401 (intended: that was the vulnerability).Test plan (TDD, red first)
pnpm exec vitest run lib/accounts app/api/accounts: 14 files, 60 tests passed (12 new tests: route 401/400/derive-from-credential/override-denied; resolver default/self/access-granted/403/404; handler insert/dedupe/db-error).pnpm exec tsc --noEmit: no errors in any touched file; pre-existing errors remain in unrelated test files (lib/trigger/__tests__,lib/admins/...,lib/accounts/__tests__/validateOverrideAccountId.test.ts) that also fail onmain.Preview verification pending — tracked in chat#1860.
🤖 Generated with Claude Code
Summary by cubic
Secure the roster-link endpoint:
POST /api/accounts/artistsnow requires auth and links artists to the authenticated account by default. The optionalemailoverride is access-gated, fixing P0a in recoupable/chat#1860.Bug Fixes
validateAuthContext; return 401 when missing/invalid.emailviacheckAccountAccess(403 on denial, 404 if not found).addArtistToAccountHandler,validateAddArtistRequest, andlinkArtistToAccount; route delegates to the handler. Added tests for route, handler, validator, resolver, and linking.Migration
artistId;emailis optional and access-gated. Update API docs for auth and new 401/403 responses.Written for commit c2e3571. Summary will update on new commits.