Drift audit 2026-06-02 — 26 findings shipped #75

Merged
bvandeusen merged 11 commits from dev into main 2026-06-02 19:21:53 -04:00
6 changed files with 130 additions and 17 deletions
Showing only changes of commit fb3116d640 - Show all commits
@@ -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()
@@ -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:<variant>" — 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()
}
@@ -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<PlaybackErrorEvent>(Channel.CONFLATED)
private val playbackErrorEventsChannel = Channel<PlaybackErrorEvent>(Channel.BUFFERED)
val playbackErrorEvents: Flow<PlaybackErrorEvent> = playbackErrorEventsChannel.receiveAsFlow()
/**
@@ -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:<id>" 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)
}
}
@@ -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)
}
}
+25 -4
View File
@@ -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`