diff --git a/api/auth_middleware.go b/api/auth_middleware.go index 2060f207..e5015098 100644 --- a/api/auth_middleware.go +++ b/api/auth_middleware.go @@ -349,12 +349,28 @@ func (app *ApiServer) authMiddleware(c *fiber.Ctx) error { // Not authorized to act on behalf of myId. // - // Exception: /users/:userId/feed/for-you accepts user_id as a viewer hint - // used only for response decoration (has_current_user_reposted etc.); the - // path :userId — not user_id — controls what gets personalized. Treat the - // query user_id as advisory rather than authoritative on this route so - // the endpoint can be called like the other public read endpoints. - allowUnauthenticatedViewerId := strings.HasSuffix(c.Path(), "/feed/for-you") + // Exceptions: public discovery reads where user_id is purely a + // viewer hint used for response decoration + // (has_current_user_reposted, has_current_user_saved, etc.) with no + // permission semantics tied to it. Treat the query user_id as + // advisory rather than authoritative on these routes so logged-in + // SDK clients can pass it without forging signature headers. + // + // Anything in this list MUST satisfy two conditions: + // 1. Method is GET (no writes — writes must remain authoritative). + // 2. user_id is used only to populate has_current_user_* / similar + // decoration flags. It MUST NOT control content selection, + // permission, or row visibility — that responsibility lives on + // a path :userId param or an explicit auth middleware. + // + // Follow-up: the list below is the tactical patch for the most + // affected discovery surfaces. The longer-term fix is a per-route + // opt-in marker (e.g. an `advisoryUserId` middleware attached at + // route registration) so this allowlist doesn't have to grow with + // every new public read. Tracked in api#TBD. + path := c.Path() + allowUnauthenticatedViewerId := c.Method() == fiber.MethodGet && + isAdvisoryUserIdPath(path) if myId != 0 && !pkceAuthed && !allowUnauthenticatedViewerId && !app.isAuthorizedRequest(c.Context(), myId, wallet) { return fiber.NewError( @@ -382,6 +398,68 @@ func (app *ApiServer) authMiddleware(c *fiber.Ctx) error { return c.Next() } +// isAdvisoryUserIdPath matches request paths where ?user_id is treated as a +// viewer hint for response decoration only — no permission semantics. See the +// big comment in authMiddleware for the contract; this matcher must stay in +// sync with it. +// +// All entries are conceptual route patterns rewritten as a literal path-match +// or a small predicate. Anything load-bearing on user_id (e.g. /me, /account, +// any write) is intentionally absent. +func isAdvisoryUserIdPath(path string) bool { + // Normalize: drop the /v1 prefix so the matcher is version-agnostic. + stripped := strings.TrimPrefix(path, "/v1") + + switch stripped { + case "/playlists", + "/playlists/trending", + "/playlists/top", + "/playlists/new-releases", + "/playlists/by_permalink", + "/playlists/search", + "/tracks", + "/tracks/trending", + "/tracks/recommended", + "/tracks/trending/ids", + "/users", + "/users/search", + "/users/top", + "/users/genre/top": + return true + } + + // /users/:userId/feed/for-you and other /feed/for-you variants. + if strings.HasSuffix(stripped, "/feed/for-you") { + return true + } + + // Dynamic single-resource reads: /playlists/, /tracks/, + // /users/, /users/handle/. These are decorative — the path + // param controls the resource; user_id only personalizes flags. + segs := strings.Split(strings.TrimPrefix(stripped, "/"), "/") + if len(segs) >= 2 { + switch segs[0] { + case "playlists", "tracks": + // /playlists/, /tracks/ — but NOT /playlists//stream + // etc. (those endpoints may have their own semantics). Match + // only the two-segment case here; sub-resources stay strict. + if len(segs) == 2 { + return true + } + case "users": + // /users/, /users/handle/ — single user fetch. + // Their sub-resource reads (/users//tracks, /followers, + // etc.) are NOT in this tactical exemption; if needed, add + // them explicitly. + if len(segs) == 2 || (len(segs) == 3 && segs[1] == "handle") { + return true + } + } + } + + return false +} + // Middleware to require auth for the userId in the route params // Returns a 403 if the authedWallet is not authorized to act on behalf of the userId // Should be placed after authMiddleware diff --git a/api/auth_middleware_test.go b/api/auth_middleware_test.go index 6ad08207..6822df8b 100644 --- a/api/auth_middleware_test.go +++ b/api/auth_middleware_test.go @@ -435,3 +435,58 @@ func base64Encode(s string) string { func base64EncodeBytes(b []byte) string { return base64.StdEncoding.EncodeToString(b) } + +// TestIsAdvisoryUserIdPath documents the contract for which public read +// surfaces treat ?user_id as advisory (decoration only) vs authoritative +// (permission/selection). The list MUST stay narrow — anything that +// materially uses user_id beyond decoration should NOT be marked advisory. +func TestIsAdvisoryUserIdPath(t *testing.T) { + cases := []struct { + path string + want bool + }{ + // Documented exempt — viewer hint only. + {"/v1/playlists/trending", true}, + {"/v1/playlists/top", true}, + {"/v1/playlists/new-releases", true}, + {"/v1/playlists/by_permalink", true}, + {"/v1/playlists/search", true}, + {"/v1/tracks/trending", true}, + {"/v1/tracks/recommended", true}, + + // Single-resource reads — :id is the resource selector; user_id + // only personalizes has_current_user_*. + {"/v1/playlists/abc123", true}, + {"/v1/tracks/def456", true}, + {"/v1/users/oaM5J", true}, + {"/v1/users/handle/somebody", true}, + + // For You — the canonical example. + {"/v1/users/oaM5J/feed/for-you", true}, + + // NOT exempt — authoritative on myId. /me derives "who is the + // caller" from user_id, so impersonation here would be a + // security hole. (See big comment in authMiddleware.) + {"/v1/me", false}, + {"/v1/oauth/me", false}, + {"/v1/users/account/0xabc", false}, + + // NOT exempt — sub-resource reads that this tactical patch + // doesn't claim coverage for. They may need to be added in a + // follow-up; for now they stay strict (which is the existing + // pre-patch behavior). + {"/v1/playlists/abc/tracks", false}, + {"/v1/users/oaM5J/tracks", false}, + {"/v1/users/oaM5J/followers", false}, + + // Random paths should never match. + {"/v1/notifications", false}, + {"/v1/", false}, + {"/", false}, + } + + for _, tc := range cases { + got := isAdvisoryUserIdPath(tc.path) + assert.Equalf(t, tc.want, got, "isAdvisoryUserIdPath(%q)", tc.path) + } +} diff --git a/api/v1_playlists_trending_test.go b/api/v1_playlists_trending_test.go index 8d78a426..3355ed06 100644 --- a/api/v1_playlists_trending_test.go +++ b/api/v1_playlists_trending_test.go @@ -289,6 +289,23 @@ func TestGetTrendingPlaylists_Albums(t *testing.T) { }) } + // Regression: a signed-out SDK caller passes user_id as a viewer hint + // (purely for has_current_user_* decoration). The middleware must + // treat it as advisory and not 403 the request — otherwise every + // logged-in client gets an empty trending list. Same path serves both + // type=playlist and type=album, so one exemption covers both. + { + viewer := trashid.MustEncodeHashID(1) + status, body := testGet(t, + app, + "/v1/playlists/trending?limit=5&type=album&user_id="+viewer, + nil) + assert.Equal(t, 200, status) + jsonAssert(t, body, map[string]any{ + "data.#": 5, + }) + } + // Cache safety net: if an album becomes private after the qualified-ids // cache was populated (a stale entry), the response handler should still // drop it before returning. Flip album 1 to private and re-call — the