diff --git a/android/app/src/main/java/com/fabledsword/minstrel/api/AuthCookieInterceptor.kt b/android/app/src/main/java/com/fabledsword/minstrel/api/AuthCookieInterceptor.kt index e1a87926..172d767b 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/api/AuthCookieInterceptor.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/api/AuthCookieInterceptor.kt @@ -7,11 +7,23 @@ import javax.inject.Inject import javax.inject.Singleton /** - * Attaches the session cookie from [AuthStore] to every request, - * captures Set-Cookie from successful responses (login flow), and - * clears the store on 401 so downstream code can react to logout. + * Attaches the session cookie from [AuthStore] to Minstrel-server + * requests, captures Set-Cookie from successful responses (login + * flow), and clears the store on 401 so downstream code can react + * to logout. * * Mirrors the Flutter Dio interceptor pattern. + * + * Scoped to the [BaseUrlInterceptor.PLACEHOLDER_HOST] sentinel host + * the same way BaseUrlInterceptor is. Drift #568 / #569 caught two + * leaks here: (1) the session cookie was being attached to every + * external request the shared OkHttpClient services — including + * Coil image fetches to artwork.musicbrainz.org / coverartarchive.org + * / Lidarr's /MediaCover endpoints — exposing the session + * identifier to third-party logging; (2) a 401 from any of those + * external hosts silently wiped the user's Minstrel session. + * Restricting both attach + clear to placeholder-host requests + * closes both gaps. */ @Singleton class AuthCookieInterceptor @Inject constructor( @@ -19,8 +31,15 @@ class AuthCookieInterceptor @Inject constructor( ) : Interceptor { override fun intercept(chain: Interceptor.Chain): Response { + val original = chain.request() + if (original.url.host != BaseUrlInterceptor.PLACEHOLDER_HOST) { + // External request — no Minstrel session cookie attached, + // and a 401 from this host does NOT clear the user's + // session. Pass through untouched. + return chain.proceed(original) + } val cookie = authStore.sessionCookie.value - val request = chain.request().newBuilder().apply { + val request = original.newBuilder().apply { if (!cookie.isNullOrEmpty()) header("Cookie", cookie) }.build() diff --git a/android/app/src/main/java/com/fabledsword/minstrel/home/ui/HomeScreen.kt b/android/app/src/main/java/com/fabledsword/minstrel/home/ui/HomeScreen.kt index ae5a0ed9..63467dad 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/home/ui/HomeScreen.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/home/ui/HomeScreen.kt @@ -288,7 +288,15 @@ class HomeViewModel @Inject constructor( poolMessages.trySend("Mix isn't ready yet - try again in a moment") return@launch } - val source = if (playlist.refreshable) "playlist:${playlist.systemVariant}" else null + // Drift #564: send the BARE systemVariant string, not + // "playlist:" — the server's rotation matcher + // (internal/playevents/writer.go systemPlaylistSources) + // keys on the bare variant. Web sends the bare form too + // (web/src/lib/components/PlaylistCard.svelte:83), so this + // brings Android into alignment. Wrong prefix here meant + // system-mix plays from Android Home never advanced the + // rotation. + val source = if (playlist.refreshable) playlist.systemVariant else null player.setQueue(tracks, initialIndex = 0, source = source) }.join() } diff --git a/android/app/src/main/java/com/fabledsword/minstrel/player/PlayerController.kt b/android/app/src/main/java/com/fabledsword/minstrel/player/PlayerController.kt index 31dac4b2..9fc44bb7 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/player/PlayerController.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/player/PlayerController.kt @@ -72,10 +72,19 @@ class PlayerController @Inject constructor( * failure (decoder, transport, EOS), and once when the player * reaches STATE_READY with a duration of zero / TIME_UNSET — the * "track loaded but has no audio" case the user sees as the - * player sitting frozen on a track. Conflated channel so back- - * pressure can't stall the player loop. + * player sitting frozen on a track. + * + * Drift #561: this was originally Channel.CONFLATED, which silently + * dropped every emission except the latest each time the reporter + * loop wasn't actively reading. A network blip that failed 5 + * tracks back-to-back would surface only the last failure to the + * snackbar (the "Skipped 5 unplayable tracks" coalescing path was + * dead code) AND only POST one playback_errors row to the admin + * inbox instead of 5. Buffered so every burst event reaches the + * reporter; default capacity is 64 which is well above any real + * burst rate. */ - private val playbackErrorEventsChannel = Channel(Channel.CONFLATED) + private val playbackErrorEventsChannel = Channel(Channel.BUFFERED) val playbackErrorEvents: Flow = playbackErrorEventsChannel.receiveAsFlow() /** diff --git a/android/app/src/main/java/com/fabledsword/minstrel/playlists/ui/PlaylistDetailScreen.kt b/android/app/src/main/java/com/fabledsword/minstrel/playlists/ui/PlaylistDetailScreen.kt index 99250266..db8c3529 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/playlists/ui/PlaylistDetailScreen.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/playlists/ui/PlaylistDetailScreen.kt @@ -227,7 +227,16 @@ class PlaylistDetailViewModel @Inject constructor( val refs = tracks.toPlayableTrackRefs() if (refs.isEmpty()) return val startIndex = refs.indexOfFirst { it.id == startTrackId }.coerceAtLeast(0) - player.setQueue(refs, initialIndex = startIndex, source = "playlist:$playlistId") + // Drift #564: for refreshable system playlists, send the bare + // systemVariant string so the rotation matcher + // (systemPlaylistSources in playevents/writer.go) sees the + // play. User playlists keep the "playlist:" tag for + // attribution but it doesn't trigger rotation (intentional — + // rotation only applies to system mixes). + val playlist = (internal.value as? PlaylistDetailUiState.Success)?.detail?.playlist + val source = playlist?.systemVariant?.takeIf { playlist.refreshable } + ?: "playlist:$playlistId" + player.setQueue(refs, initialIndex = startIndex, source = source) } } diff --git a/android/app/src/test/java/com/fabledsword/minstrel/api/AuthCookieInterceptorTest.kt b/android/app/src/test/java/com/fabledsword/minstrel/api/AuthCookieInterceptorTest.kt index 61a3c582..039e8a8a 100644 --- a/android/app/src/test/java/com/fabledsword/minstrel/api/AuthCookieInterceptorTest.kt +++ b/android/app/src/test/java/com/fabledsword/minstrel/api/AuthCookieInterceptorTest.kt @@ -45,12 +45,26 @@ class AuthCookieInterceptorTest { } authStore = AuthStore(dao, TestScope(UnconfinedTestDispatcher())) + // BaseUrlInterceptor rewrites placeholder.invalid → mock server. + // AuthCookieInterceptor scopes its attach + clear behavior to + // the placeholder host so external Coil image fetches don't get + // the Minstrel session cookie attached and don't trigger a + // session-clear on 401. Tests issue requests to + // http://placeholder.invalid/... so the auth interceptor sees + // the in-scope host, and BaseUrlInterceptor (running AFTER, so + // the auth interceptor sees the original host) rewrites the URL + // before transport. + authStore.setBaseUrl(server.url("/").toString()) client = OkHttpClient.Builder() .addInterceptor(AuthCookieInterceptor(authStore)) + .addInterceptor(BaseUrlInterceptor(authStore)) .build() } + private fun placeholderUrl(path: String): String = + "http://${BaseUrlInterceptor.PLACEHOLDER_HOST}$path" + @AfterEach fun teardown() { server.shutdown() @@ -61,7 +75,7 @@ class AuthCookieInterceptorTest { authStore.setSessionCookie("session=abc123") server.enqueue(MockResponse().setResponseCode(200)) - client.newCall(Request.Builder().url(server.url("/api/test")).build()).execute() + client.newCall(Request.Builder().url(placeholderUrl("/api/test")).build()).execute() val recorded = server.takeRequest() assertEquals("session=abc123", recorded.getHeader("Cookie")) @@ -72,7 +86,7 @@ class AuthCookieInterceptorTest { authStore.setSessionCookie("session=expired") server.enqueue(MockResponse().setResponseCode(401)) - client.newCall(Request.Builder().url(server.url("/api/test")).build()).execute() + client.newCall(Request.Builder().url(placeholderUrl("/api/test")).build()).execute() assertNull(authStore.sessionCookie.value) } @@ -85,7 +99,7 @@ class AuthCookieInterceptorTest { .addHeader("Set-Cookie", "session=newvalue; Path=/; HttpOnly"), ) - client.newCall(Request.Builder().url(server.url("/api/login")).build()).execute() + client.newCall(Request.Builder().url(placeholderUrl("/api/login")).build()).execute() assertEquals("session=newvalue", authStore.sessionCookie.value) } @@ -95,8 +109,41 @@ class AuthCookieInterceptorTest { authStore.setSessionCookie("session=existing") server.enqueue(MockResponse().setResponseCode(200)) - client.newCall(Request.Builder().url(server.url("/api/anything")).build()).execute() + client.newCall(Request.Builder().url(placeholderUrl("/api/anything")).build()).execute() assertEquals("session=existing", authStore.sessionCookie.value) } + + // Drift #568 regression guard: requests to external hosts must NOT + // carry the Minstrel session cookie even when the auth store has + // one. The shared OkHttpClient is also used by Coil for image + // fetches to artwork.musicbrainz.org and the like; leaking the + // session cookie to those hosts is a posture violation. + @Test + fun `does NOT attach cookie to non-placeholder host`() { + authStore.setSessionCookie("session=secret") + server.enqueue(MockResponse().setResponseCode(200)) + + // Request goes directly to the mock server's host:port — NOT + // through the placeholder sentinel — so the auth interceptor + // should pass it through untouched. + client.newCall(Request.Builder().url(server.url("/external/image.jpg")).build()).execute() + + val recorded = server.takeRequest() + assertNull(recorded.getHeader("Cookie")) + } + + // Drift #569 regression guard: a 401 from an external host must + // NOT wipe the user's Minstrel session. A misbehaving CDN or a + // Lidarr that's 401-ing on /MediaCover URLs used to silently sign + // the user out. + @Test + fun `does NOT clear cookie on 401 from non-placeholder host`() { + authStore.setSessionCookie("session=keep") + server.enqueue(MockResponse().setResponseCode(401)) + + client.newCall(Request.Builder().url(server.url("/external/image.jpg")).build()).execute() + + assertEquals("session=keep", authStore.sessionCookie.value) + } } diff --git a/internal/playevents/writer.go b/internal/playevents/writer.go index f523a3b4..9759d578 100644 --- a/internal/playevents/writer.go +++ b/internal/playevents/writer.go @@ -69,11 +69,32 @@ func (w *Writer) RecordPlayStarted( } // systemPlaylistSources are the play_events.source values that count -// against a system playlist's rotation (Fable #415). Add future -// system-playlist kinds here as they ship (deep_cuts, rediscover, …). +// against a system playlist's rotation (Fable #415). Mirrors the +// `playlists_kind_variant_consistent` CHECK in migration +// 0028_discovery_mix_variants.up.sql — every variant in that CHECK +// list that ships as a refreshable system mix needs to be here so +// the rotation reporter sees Android + web plays from that surface. +// +// Drift #563: this map drifted behind the migrations. It had only +// for_you + discover but migrations 0021 + 0028 added 6 more +// variants. Plays from Rediscover / Deep Cuts / Songs Like X / +// New for You / On This Day / First Listens didn't advance the +// per-user rotation, so "unplayed first" ordering staled on those +// mixes — same tracks kept surfacing. +// +// Future variant additions should update this map AND the CHECK in +// the matching migration in the same change; the +// `db_check_constraint_for_new_variants` standing rule covers the +// CHECK half. var systemPlaylistSources = map[string]bool{ - "for_you": true, - "discover": true, + "for_you": true, + "discover": true, + "deep_cuts": true, + "rediscover": true, + "new_for_you": true, + "on_this_day": true, + "first_listens": true, + "songs_like_artist": true, } // RecordPlayStartedWithSource is RecordPlayStarted plus a `source`