Skip to content

fix(auth): treat user_id as advisory on /playlists/trending#862

Open
dylanjeffers wants to merge 2 commits into
mainfrom
fix/trending-playlists-user-id
Open

fix(auth): treat user_id as advisory on /playlists/trending#862
dylanjeffers wants to merge 2 commits into
mainfrom
fix/trending-playlists-user-id

Conversation

@dylanjeffers
Copy link
Copy Markdown
Contributor

Bug

`GET /v1/playlists/trending?user_id=` returns HTTP 403 for any signed-out caller that passes a viewer hint:

```
{"code":403,"error":"You are not authorized to make this request authedWallet= myId=177451798"}
```

Same for `?type=album&user_id=` (same path, different query). Repros today against `api.audius.co` for any user_id.

Effect at the SDK layer: clients reading the response mask the error and surface an empty `data` array, so every logged-in caller's "Playlists for you" / trending strip silently renders nothing.

Root cause

The global `authMiddleware` (`app.Use(app.authMiddleware)` in `server.go:391`) rejects any request where the decoded `user_id` (`myId`) doesn't match the wallet resolved from auth headers (`auth_middleware.go:359`).

There's already an exemption for `/users/:userId/feed/for-you` — documented as advisory because user_id on that route is only used for response decoration (`has_current_user_reposted`, etc.) and has no permission semantics. The trending playlists endpoint has the same shape: ranking is global, user_id only personalizes decoration. But it wasn't on the exemption list.

The SDK's `getTrendingPlaylists` passes `user_id` whenever the caller is logged in (without forging signature headers, which it has no way to do for an unauthenticated SDK consumer). So the middleware 403'd every such call.

Fix

Extend the existing `allowUnauthenticatedViewerId` check to also match `/playlists/trending`. One suffix covers both `type=playlist` and `type=album` — they share the same path. Comment is updated to document the contract: the list must stay narrow and only include endpoints where `user_id` is purely decorative.

```go
allowUnauthenticatedViewerId := strings.HasSuffix(path, "/feed/for-you") ||
strings.HasSuffix(path, "/playlists/trending")
```

Test

Added a new case to `TestGetTrendingPlaylists_Albums` that hits `/v1/playlists/trending?type=album&user_id=` and asserts 200 + a populated `data` array. The new case fails on `main` (403) and passes with this change.

Out of scope (worth a follow-up)

The same 403 pattern affects other public discovery reads when passed `user_id`:

  • `GET /v1/playlists/?user_id=` — verified 403 on prod
  • Likely `/v1/playlists/top`, `/v1/playlists/new-releases`
  • Possibly others

The right long-term shape is probably a route-level opt-in flag (`advisoryUserId: true`) instead of a path-suffix allowlist that has to grow with every new read endpoint. Out of scope for this fix — keeping this PR scoped to the one path `feat/for-you-collections` (apps#14411) actually needs unblocked.

🤖 Generated with Claude Code

dylanjeffers and others added 2 commits May 27, 2026 17:14
GET /v1/playlists/trending?user_id=<id> was returning HTTP 403
"You are not authorized to make this request authedWallet= myId=…"
for every signed-out call that passed a viewer hint.

Root cause: the global authMiddleware (run via app.Use) rejects any
request where the decoded user_id (myId) doesn't match the wallet
resolved from auth headers. There's already a precedent exemption
for /users/:userId/feed/for-you (auth_middleware.go:357 prior),
where user_id is documented as advisory — only used for response
decoration like has_current_user_reposted, with no permission
semantics tied to it. The trending playlists endpoint has the same
shape: ranking is global, user_id only personalizes decoration.

But since the SDK's getTrendingPlaylists passes user_id whenever
the caller is logged in (and does not forge signature headers), the
middleware 403'd every logged-in caller. Net effect at the surface
level was an empty data array, since clients reading the SDK
response mask the error.

Extend the existing exemption to cover /playlists/trending (which
serves both type=playlist and type=album from the same path, so one
suffix covers both). Comment is updated to document the contract:
this list must stay narrow and only include endpoints where
user_id is purely decorative.

Test: added a new case to TestGetTrendingPlaylists_Albums that hits
trending with ?user_id=<id> and asserts 200 + a populated data
array. The new case fails without the middleware change.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After auditing all 173 unauthenticated GET routes in the API, the
suffix-match approach (one path: /playlists/trending) covers only one
of many surfaces affected by the same global-middleware 403. Other
public discovery reads with ?user_id in the SDK have the same bug:

- /v1/playlists/top, /playlists/new-releases, /playlists/by_permalink,
  /playlists/search
- /v1/tracks/trending, /tracks/recommended
- /v1/playlists/<id>, /v1/tracks/<id>
- /v1/users/<id>, /v1/users/handle/<handle>
- /v1/users (and /users/search, /users/top, /users/genre/top)

All of these treat user_id as a viewer hint (has_current_user_* flags
only) — they have no permission semantics on user_id. The path :id
controls what gets returned.

NOT extended:
- /v1/me, /v1/oauth/me: load-bearing on user_id (derive "who am I"
  from it). Must stay authoritative.
- /v1/users/account/:wallet: requireAuthMiddleware-protected; the
  wallet path controls access.
- Sub-resource reads under /users/:userId/<thing>: kept strict for
  this tactical patch. The pre-patch behavior was already strict, so
  leaving them strict isn't a regression. Can be added in a follow-up
  if needed.

Refactor: move the exemption logic to an isAdvisoryUserIdPath helper
with explicit cases + a small predicate for dynamic single-resource
paths. Document the contract inline. Add TestIsAdvisoryUserIdPath
covering both directions (exempt + still-strict).

The proper long-term fix is a per-route opt-in marker
(advisoryUserId middleware attached at route registration). That
removes the central allowlist entirely. Out of scope for this PR;
flagged in the inline comment.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@dylanjeffers
Copy link
Copy Markdown
Contributor Author

Broader audit + extended fix in `2fe7778`.

After auditing all 173 unauthenticated GET routes that call `app.getMyId(c)`, the original one-path suffix patch (`/playlists/trending`) covered only one of many discovery surfaces hit by the same global-middleware 403. Same root cause, same fix shape — just a wider exemption.

Surfaces now covered

  • `/v1/playlists/trending`, `/playlists/top`, `/playlists/new-releases`, `/playlists/by_permalink`, `/playlists/search`
  • `/v1/tracks/trending`, `/tracks/recommended`
  • `/v1/playlists/`, `/v1/tracks/` (single-resource reads — the path :id is the selector)
  • `/v1/users/`, `/v1/users/handle/`, `/v1/users`, `/users/search`, `/users/top`, `/users/genre/top`
  • `/v1/users//feed/for-you` (pre-existing exemption preserved)

All of these treat `user_id` as a viewer hint for `has_current_user_reposted` / `has_current_user_saved` decoration only. None use it to gate permission or select content.

Deliberately NOT extended (security-sensitive)

  • `/v1/me`, `/v1/oauth/me` — load-bearing on `user_id`. The handler does `userId := app.getMyId(c)` and returns that user's profile. Removing the strict check here would let an authenticated user pass `?user_id=X` and read user X's profile. Stays authoritative.
  • `/v1/users/account/:wallet` — already protected by `requireAuthMiddleware`. No change needed.
  • Sub-resource reads under `/users/:userId/` (followers, following, tracks, library, etc.) — kept strict to minimize blast radius. They have the same bug but pre-patch behavior was already strict, so leaving them strict isn't a new regression. Can be added in a follow-up once we verify each handler doesn't use `user_id` for selection.

Implementation

Moved the exemption logic to `isAdvisoryUserIdPath(path string) bool` with explicit cases + a small predicate for dynamic single-resource paths. Also gated the whole exemption on `c.Method() == GET` so it can never apply to writes. Added `TestIsAdvisoryUserIdPath` covering both directions — exempt paths AND the still-strict paths (`/me`, `/oauth/me`, sub-resource reads) — to lock the contract in.

The right long-term fix

The central allowlist still has to grow over time and is fragile to typos / new routes. Better shape: a per-route `advisoryUserId` middleware attached at route registration, e.g.

```go
g.Get("/playlists/trending", app.advisoryUserId, app.v1PlaylistsTrending)
```

That removes the central list entirely and puts the opt-in next to the route. Out of scope for this PR; left as a follow-up note in the inline comment.

🤖 Generated with Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant