From 62c99747094a641cee29703de4f9622d59457d8f Mon Sep 17 00:00:00 2001 From: Gianni Carlo Date: Tue, 29 Sep 2026 11:37:49 -0500 Subject: [PATCH 1/5] fix: send Jellyfin auth in the standard Authorization header Jellyfin 12 turned legacy authorization off by default: it ignores the X-Emby-Authorization header every Jellyfin call sent. Sign-in and Quick Connect reached the server with no client/device info and failed with 400 ("Authentication failed: Bad Request" in 1.1.3, "The server responded with 400" since the connection-flow redesign), and token calls made by already-connected installs came back 401. Every endpoint now sends the same MediaBrowser value in `Authorization`, which Jellyfin has always accepted (checked against 10.8.13, 10.9.11, 10.10.7, 10.11.11 and 12.1.0). The progress push had its own copy of the header builder that reported "Android" / 1.0.0; it now uses JellyfinService's, so the server sees one identity per install. Its custom headers are sanitized like the service's: with the token in `Authorization`, a custom header of that name would otherwise replace it. --- .../audiobookplayer/logic/CoreProcessors.kt | 42 ++-------- .../network/services/JellyfinApi.kt | 25 +++--- .../network/services/JellyfinService.kt | 57 +++++++------- .../logic/ExternalUpdateProcessorTest.kt | 78 +++++++++++++++++++ .../network/JellyfinFileExtensionsTest.kt | 2 +- .../network/JellyfinProbeTest.kt | 17 +++- .../JellyfinQuickConnectServiceTest.kt | 4 +- 7 files changed, 148 insertions(+), 77 deletions(-) create mode 100644 core/src/test/java/com/tortugapower/audiobookplayer/logic/ExternalUpdateProcessorTest.kt diff --git a/core/src/main/java/com/tortugapower/audiobookplayer/logic/CoreProcessors.kt b/core/src/main/java/com/tortugapower/audiobookplayer/logic/CoreProcessors.kt index abbbd2da..7d574f20 100644 --- a/core/src/main/java/com/tortugapower/audiobookplayer/logic/CoreProcessors.kt +++ b/core/src/main/java/com/tortugapower/audiobookplayer/logic/CoreProcessors.kt @@ -996,7 +996,10 @@ class SetExternalResourceToDownloadProcessor : TaskProcessor { } class ExternalUpdateProcessor( - private val context: Context + private val context: Context, + // Through the repository, not the DAO (see process()). Overridable so tests can swap the Keystore cipher. + private val serverRepository: ExternalServerRepository = + ExternalServerRepository(AppDatabase.getDatabase(context).externalServerDao()), ) : TaskProcessor { private val gson = Gson() @@ -1008,7 +1011,9 @@ class ExternalUpdateProcessor( private fun buildApiClient(sanitizedUrl: String, customHeaders: Map?): retrofit2.Retrofit { val okHttpClientBuilder = baseHttpClient.newBuilder() - customHeaders?.forEach { (key, value) -> + // Sanitized like JellyfinService's client: a custom `Authorization` entry would replace the + // provider's own auth header, and an illegal name/value throws at request time. + ExternalServiceUtils.sanitizeCustomHeaders(customHeaders)?.forEach { (key, value) -> okHttpClientBuilder.addInterceptor { chain -> val request = chain.request().newBuilder().header(key, value).build() chain.proceed(request) @@ -1034,34 +1039,6 @@ class ExternalUpdateProcessor( return permanent } - private fun getDeviceId(): String { - return try { - if (!com.tortugapower.audiobookplayer.core.CoreContext.isInitialized()) return "BookPlayerAndroidID" - val appCtx = com.tortugapower.audiobookplayer.core.CoreContext.appContext - val prefs = appCtx.getSharedPreferences("jellyfin_prefs", Context.MODE_PRIVATE) - var id = prefs.getString("device_id", null) - if (id == null) { - id = java.util.UUID.randomUUID().toString() - prefs.edit().putString("device_id", id).apply() - } - id - } catch (e: Exception) { - "BookPlayerAndroidID" - } - } - - private fun getJellyfinAuthHeader(token: String? = null): String { - val device = "Android" - val deviceId = getDeviceId() - val client = "BookPlayer" - val version = "1.0.0" - var header = "MediaBrowser Client=\"$client\", Device=\"$device\", DeviceId=\"$deviceId\", Version=\"$version\"" - if (token != null) { - header += ", Token=\"$token\"" - } - return header - } - override suspend fun process(task: SyncTaskEntity): Boolean { val payloadType = object : TypeToken>() {}.type val payload: Map = gson.fromJson(task.payload, payloadType) @@ -1075,13 +1052,10 @@ class ExternalUpdateProcessor( val percentCompleted = (payload["percentCompleted"] as? Double) ?: 0.0 val isFinished = (payload["isFinished"] as? Boolean) ?: false - val db = AppDatabase.getDatabase(context) - // Resolve through THE shared resolver (stable-id contract + decrypted credentials): the // old inline rowid lookup read the DAO directly, so the token below was ciphertext and // the provider rejected it with 401; it also stopped matching once hostIds became // GUIDs/URL keys, silently discarding every progress push. - val serverRepository = com.tortugapower.audiobookplayer.repository.ExternalServerRepository(db.externalServerDao()) val server = ExternalServiceUtils.serverForResource( serverRepository, com.tortugapower.audiobookplayer.database.entities.ExternalResourceEntity( @@ -1121,7 +1095,7 @@ class ExternalUpdateProcessor( val api = buildApiClient(sanitizedUrl, customHeaders) .create(com.tortugapower.audiobookplayer.network.services.JellyfinApi::class.java) - val authHeader = getJellyfinAuthHeader(token) + val authHeader = com.tortugapower.audiobookplayer.network.services.JellyfinService.getAuthHeader(token) val response = api.updateUserData(authHeader, providerId, requestBody) handleResponse(providerName, response) } diff --git a/core/src/main/java/com/tortugapower/audiobookplayer/network/services/JellyfinApi.kt b/core/src/main/java/com/tortugapower/audiobookplayer/network/services/JellyfinApi.kt index 4a6763aa..ef1dde9a 100644 --- a/core/src/main/java/com/tortugapower/audiobookplayer/network/services/JellyfinApi.kt +++ b/core/src/main/java/com/tortugapower/audiobookplayer/network/services/JellyfinApi.kt @@ -4,10 +4,13 @@ import com.google.gson.annotations.SerializedName import retrofit2.Response import retrofit2.http.* +// Every call carries the MediaBrowser scheme in the standard `Authorization` header. Jellyfin 12 turned +// legacy auth off by default: it ignores `X-Emby-Authorization`, so sign-in arrived with no client/device +// info (400) and token calls came back 401. interface JellyfinApi { @POST("Users/AuthenticateByName") suspend fun authenticate( - @Header("X-Emby-Authorization") authHeader: String, + @Header("Authorization") authHeader: String, @Body request: JellyfinAuthRequest ): Response @@ -19,32 +22,32 @@ interface JellyfinApi { // client-identity header (no token) like every other pre-auth Jellyfin call. @GET("QuickConnect/Enabled") suspend fun getQuickConnectEnabled( - @Header("X-Emby-Authorization") authHeader: String + @Header("Authorization") authHeader: String ): Response // Quick Connect: start a request (server returns the user-facing Code + our Secret) … @POST("QuickConnect/Initiate") suspend fun initiateQuickConnect( - @Header("X-Emby-Authorization") authHeader: String + @Header("Authorization") authHeader: String ): Response // … poll until the user approves it from the web UI (Authenticated flips to true; 404 once the secret expired) … @GET("QuickConnect/Connect") suspend fun getQuickConnectState( - @Header("X-Emby-Authorization") authHeader: String, + @Header("Authorization") authHeader: String, @Query("secret") secret: String ): Response // … then exchange the approved secret for a session, same shape as a password sign-in. @POST("Users/AuthenticateWithQuickConnect") suspend fun authenticateWithQuickConnect( - @Header("X-Emby-Authorization") authHeader: String, + @Header("Authorization") authHeader: String, @Body request: JellyfinQuickConnectRequest ): Response @GET("Items") suspend fun getItems( - @Header("X-Emby-Authorization") authHeader: String, + @Header("Authorization") authHeader: String, @Query("IncludeItemTypes") itemTypes: String = "Audiobook", @Query("Recursive") recursive: Boolean = true, @Query("Fields") fields: String = "PrimaryImageAspectRatio,BasicSyncInfo,Path,Genres,ArtistItems", @@ -61,7 +64,7 @@ interface JellyfinApi { */ @GET("Items") suspend fun getItemsByIds( - @Header("X-Emby-Authorization") authHeader: String, + @Header("Authorization") authHeader: String, @Query("Ids") ids: String, @Query("Fields") fields: String = "MediaSources,Path" ): Response @@ -69,23 +72,23 @@ interface JellyfinApi { // The authenticated user's top-level views (libraries); the user is inferred from the token. @GET("UserViews") suspend fun getUserViews( - @Header("X-Emby-Authorization") authHeader: String + @Header("Authorization") authHeader: String ): Response @GET("System/Info") suspend fun getSystemInfo( - @Header("X-Emby-Authorization") authHeader: String + @Header("Authorization") authHeader: String ): Response // Revokes the session behind the supplied token. @POST("Sessions/Logout") suspend fun logout( - @Header("X-Emby-Authorization") authHeader: String + @Header("Authorization") authHeader: String ): Response @POST("Users/me/Items/{itemId}/UserData") suspend fun updateUserData( - @Header("X-Emby-Authorization") authHeader: String, + @Header("Authorization") authHeader: String, @Path("itemId") itemId: String, @Body request: JellyfinUserDataRequest ): Response diff --git a/core/src/main/java/com/tortugapower/audiobookplayer/network/services/JellyfinService.kt b/core/src/main/java/com/tortugapower/audiobookplayer/network/services/JellyfinService.kt index bfa249ae..6b7421e2 100644 --- a/core/src/main/java/com/tortugapower/audiobookplayer/network/services/JellyfinService.kt +++ b/core/src/main/java/com/tortugapower/audiobookplayer/network/services/JellyfinService.kt @@ -42,34 +42,6 @@ class JellyfinService : ExternalService, QuickConnectCapable { .create(JellyfinApi::class.java) } - private fun getDeviceId(): String { - return try { - if (!com.tortugapower.audiobookplayer.core.CoreContext.isInitialized()) return "BookPlayerAndroidID" - val context = com.tortugapower.audiobookplayer.core.CoreContext.appContext - val prefs = context.getSharedPreferences("jellyfin_prefs", android.content.Context.MODE_PRIVATE) - var id = prefs.getString("device_id", null) - if (id == null) { - id = java.util.UUID.randomUUID().toString() - prefs.edit().putString("device_id", id).apply() - } - id - } catch (e: Exception) { - "BookPlayerAndroidID" - } - } - - // The MediaBrowser scheme Jellyfin requires on every call, token or not. Client/Device/Version come - // from ClientIdentity (injected by the host at startup) — they are what Jellyfin shows in its Quick - // Connect approval and Devices dashboard, so a hardcoded version would misreport every install. - private fun getAuthHeader(token: String? = null): String { - val deviceId = getDeviceId() - var header = "MediaBrowser Client=\"${ClientIdentity.appName}\", Device=\"${ClientIdentity.deviceName}\", DeviceId=\"$deviceId\", Version=\"${ClientIdentity.appVersion}\"" - if (token != null) { - header += ", Token=\"$token\"" - } - return header - } - override suspend fun probe(url: String, headers: Map?): ProbeResult { return try { val api = getApi(url, headers) @@ -301,6 +273,35 @@ class JellyfinService : ExternalService, QuickConnectCapable { companion object { private const val HYDRATION_CHUNK = 100 + private fun getDeviceId(): String { + return try { + if (!com.tortugapower.audiobookplayer.core.CoreContext.isInitialized()) return "BookPlayerAndroidID" + val context = com.tortugapower.audiobookplayer.core.CoreContext.appContext + val prefs = context.getSharedPreferences("jellyfin_prefs", android.content.Context.MODE_PRIVATE) + var id = prefs.getString("device_id", null) + if (id == null) { + id = java.util.UUID.randomUUID().toString() + prefs.edit().putString("device_id", id).apply() + } + id + } catch (e: Exception) { + "BookPlayerAndroidID" + } + } + + // The MediaBrowser scheme Jellyfin requires on every call, token or not. Client/Device/Version come + // from ClientIdentity (injected by the host at startup) — they are what Jellyfin shows in its Quick + // Connect approval and Devices dashboard, so a hardcoded version would misreport every install. + // Shared with the progress push so the server never sees this install under two identities. + internal fun getAuthHeader(token: String? = null): String { + val deviceId = getDeviceId() + var header = "MediaBrowser Client=\"${ClientIdentity.appName}\", Device=\"${ClientIdentity.deviceName}\", DeviceId=\"$deviceId\", Version=\"${ClientIdentity.appVersion}\"" + if (token != null) { + header += ", Token=\"$token\"" + } + return header + } + /** * The item's REAL audio extension in iOS's order of trust: the first media source's container * (first entry of a comma list), else the extension of its file path. Null when the server reports diff --git a/core/src/test/java/com/tortugapower/audiobookplayer/logic/ExternalUpdateProcessorTest.kt b/core/src/test/java/com/tortugapower/audiobookplayer/logic/ExternalUpdateProcessorTest.kt new file mode 100644 index 00000000..4566d81f --- /dev/null +++ b/core/src/test/java/com/tortugapower/audiobookplayer/logic/ExternalUpdateProcessorTest.kt @@ -0,0 +1,78 @@ +package com.tortugapower.audiobookplayer.logic + +import android.content.Context +import androidx.test.core.app.ApplicationProvider +import com.tortugapower.audiobookplayer.database.AppDatabase +import com.tortugapower.audiobookplayer.database.entities.ExternalServerEntity +import com.tortugapower.audiobookplayer.database.entities.ExternalServiceType +import com.tortugapower.audiobookplayer.database.entities.SyncTaskEntity +import com.tortugapower.audiobookplayer.repository.ExternalServerRepository +import com.tortugapower.audiobookplayer.repository.TokenCipher +import kotlinx.coroutines.runBlocking +import okhttp3.mockwebserver.MockResponse +import okhttp3.mockwebserver.MockWebServer +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * Covers the Jellyfin progress push against a MockWebServer: it must authenticate in the standard + * `Authorization` header (Jellyfin 12 ignores `X-Emby-Authorization`) and never let a custom header + * replace that auth. + */ +@RunWith(RobolectricTestRunner::class) +class ExternalUpdateProcessorTest { + + private val context = ApplicationProvider.getApplicationContext() + private val server = MockWebServer() + + @Before fun setUp() { + server.start() + // The AppDatabase singleton persists across test methods — start each test from empty tables. + runBlocking(kotlinx.coroutines.Dispatchers.IO) { AppDatabase.getDatabase(context).clearAllTables() } + } + + @After fun tearDown() = server.shutdown() + + // Reversible stand-in for the Keystore cipher (same pattern as StreamFileUploadProcessorTest) — the + // stored row is ciphertext, and the push must authenticate with the DECRYPTED token. + private val fakeCipher = object : TokenCipher { + override fun encrypt(plaintext: String) = "ENC($plaintext)" + override fun decrypt(stored: String) = stored.removePrefix("ENC(").removeSuffix(")") + } + + private fun serverRepository() = + ExternalServerRepository(AppDatabase.getDatabase(context).externalServerDao(), fakeCipher) + + private fun task() = SyncTaskEntity( + id = "row-1", taskID = "book-1_jf-9", queueKey = "jellyfin", + jobType = SyncTaskFactory.JOB_EXTERNAL_UPDATE, position = 0, + payload = """{"uuid":"book-1","providerName":"jellyfin","providerId":"jf-9","hostId":"srv-guid","currentTime":12.5,"percentCompleted":0.25,"isFinished":false}""", + ) + + @Test fun `jellyfin progress push authenticates in the standard Authorization header`() = runBlocking { + serverRepository().saveServer( + ExternalServerEntity( + id = 1, name = "jf", type = ExternalServiceType.JELLYFIN, url = server.url("/").toString(), + token = "tok", stableId = "srv-guid", + customHeaders = mapOf("Authorization" to "Basic custom", "CF-Access-Client-Id" to "cf-id"), + ), + ) + server.enqueue(MockResponse().setResponseCode(200)) + + val handled = ExternalUpdateProcessor(context, serverRepository()).process(task()) + + assertTrue(handled) + val push = server.takeRequest() + assertEquals("POST", push.method) + val auth = push.getHeader("Authorization")!! + assertTrue(auth, auth.startsWith("MediaBrowser Client=\"")) + assertTrue(auth, auth.contains("Token=\"tok\"")) + assertEquals("cf-id", push.getHeader("CF-Access-Client-Id")) + assertTrue(push.body.readUtf8().contains("\"PlaybackPositionTicks\":125000000")) + } +} diff --git a/core/src/test/java/com/tortugapower/audiobookplayer/network/JellyfinFileExtensionsTest.kt b/core/src/test/java/com/tortugapower/audiobookplayer/network/JellyfinFileExtensionsTest.kt index f7283449..61b25cf1 100644 --- a/core/src/test/java/com/tortugapower/audiobookplayer/network/JellyfinFileExtensionsTest.kt +++ b/core/src/test/java/com/tortugapower/audiobookplayer/network/JellyfinFileExtensionsTest.kt @@ -64,7 +64,7 @@ class JellyfinFileExtensionsTest { val request = requests.single() assertEquals("MediaSources,Path", request.requestUrl!!.queryParameter("Fields")) assertEquals("1", request.getHeader("X-Test")) - assertTrue(request.getHeader("X-Emby-Authorization")!!.contains("Token=\"tok\"")) + assertTrue(request.getHeader("Authorization")!!.contains("Token=\"tok\"")) } @Test fun `ids are chunked so a whole-folder import cannot overflow the URL`() { diff --git a/core/src/test/java/com/tortugapower/audiobookplayer/network/JellyfinProbeTest.kt b/core/src/test/java/com/tortugapower/audiobookplayer/network/JellyfinProbeTest.kt index fac799bf..55674b46 100644 --- a/core/src/test/java/com/tortugapower/audiobookplayer/network/JellyfinProbeTest.kt +++ b/core/src/test/java/com/tortugapower/audiobookplayer/network/JellyfinProbeTest.kt @@ -11,6 +11,7 @@ import org.junit.After import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test @@ -65,7 +66,7 @@ class JellyfinProbeTest { found() val requests = generateSequence { server.takeRequest(1, java.util.concurrent.TimeUnit.SECONDS) }.toList() val qc = requests.first { it.path == "/QuickConnect/Enabled" } - val header = qc.getHeader("X-Emby-Authorization") + val header = qc.getHeader("Authorization") assertNotNull(header) assertTrue(header!!.startsWith("MediaBrowser Client=\"")) assertTrue(header.contains("DeviceId=\"")) @@ -122,6 +123,20 @@ class JellyfinProbeTest { assertEquals("Home", result.name) } + /** + * Jellyfin 12 ignores the legacy `X-Emby-Authorization` header by default, so a sign-in that sent only + * that one reached the server with no client/device info and failed with 400 (#119). + */ + @Test fun `sign-in sends the client identity in the standard Authorization header`() { + runBlocking { service.connect(url(), "hana", "pw") } + val requests = generateSequence { server.takeRequest(1, java.util.concurrent.TimeUnit.SECONDS) }.toList() + val signIn = requests.first { it.path == "/Users/AuthenticateByName" } + assertTrue(signIn.getHeader("Authorization")!!.startsWith("MediaBrowser Client=\"")) + assertNull(signIn.getHeader("X-Emby-Authorization")) + val info = requests.first { it.path == "/System/Info" } + assertTrue(info.getHeader("Authorization")!!.contains("Token=\"tok\"")) + } + @Test fun `wrong credentials map to the unauthorized copy`() { authenticate = MockResponse().setResponseCode(401) val failure = runBlocking { service.connect(url(), "hana", "wrong") } as ConnectionResult.Failure diff --git a/core/src/test/java/com/tortugapower/audiobookplayer/network/JellyfinQuickConnectServiceTest.kt b/core/src/test/java/com/tortugapower/audiobookplayer/network/JellyfinQuickConnectServiceTest.kt index 450bc1da..923d39b0 100644 --- a/core/src/test/java/com/tortugapower/audiobookplayer/network/JellyfinQuickConnectServiceTest.kt +++ b/core/src/test/java/com/tortugapower/audiobookplayer/network/JellyfinQuickConnectServiceTest.kt @@ -62,7 +62,7 @@ class JellyfinQuickConnectServiceTest { val request = requests().single { it.path == "/QuickConnect/Initiate" } assertEquals("POST", request.method) - val header = request.getHeader("X-Emby-Authorization")!! + val header = request.getHeader("Authorization")!! assertTrue(header.startsWith("MediaBrowser Client=\"")) assertFalse(header.contains("Token=")) } @@ -113,7 +113,7 @@ class JellyfinQuickConnectServiceTest { val exchange = requests().single { it.path == "/Users/AuthenticateWithQuickConnect" } assertEquals("""{"Secret":"s3cr3t"}""", exchange.body.readUtf8()) - assertFalse(exchange.getHeader("X-Emby-Authorization")!!.contains("Token=")) + assertFalse(exchange.getHeader("Authorization")!!.contains("Token=")) } @Test fun `exchange failures map like password sign-in`() = runBlocking { From 01f78f1964117829867da365e0a14496c17a8ad5 Mon Sep 17 00:00:00 2001 From: Gianni Carlo Date: Tue, 29 Sep 2026 11:38:00 -0500 Subject: [PATCH 2/5] fix: authenticate media-server downloads with headers, not a URL token Downloading a streamed Jellyfin book for offline use sent only the `?api_key=` URL, which Jellyfin 12 rejects (401), and never sent the user's custom headers (Cloudflare Access etc.). The download processor now attaches the provider's auth header plus the custom headers, resolved per run from the saved server, so tokens stay out of the task table and a re-auth's fresh token applies to a queued download. Only URLs on that server get them: a BookPlayer-cloud presigned URL goes out bare, since S3 rejects a request that carries a second auth mechanism. With every consumer on header auth (playback through PlaybackManager's host registry, the stream-to-cloud pipe, and now downloads), the Jellyfin download URL drops its `api_key` query token, which only leaked the token into logs and persisted task payloads. AudiobookShelf keeps its `token` query param. --- .../audiobookplayer/logic/CoreProcessors.kt | 22 ++++- .../logic/ExternalServiceUtils.kt | 29 +++++- .../audiobookplayer/logic/PlaybackManager.kt | 2 +- .../logic/DownloadFileProcessorTest.kt | 96 +++++++++++++++++++ .../logic/ExternalStreamUrlTest.kt | 7 +- .../logic/StreamFileUploadProcessorTest.kt | 6 +- 6 files changed, 148 insertions(+), 14 deletions(-) diff --git a/core/src/main/java/com/tortugapower/audiobookplayer/logic/CoreProcessors.kt b/core/src/main/java/com/tortugapower/audiobookplayer/logic/CoreProcessors.kt index 7d574f20..a6006811 100644 --- a/core/src/main/java/com/tortugapower/audiobookplayer/logic/CoreProcessors.kt +++ b/core/src/main/java/com/tortugapower/audiobookplayer/logic/CoreProcessors.kt @@ -553,7 +553,13 @@ class StreamFileUploadProcessor( } } -class DownloadFileProcessor(private val context: Context) : TaskProcessor { +class DownloadFileProcessor( + private val context: Context, + // Through the repository, not the DAO: stored credentials are encrypted at rest, and media-server + // downloads authenticate with this token. Overridable so tests can swap the Keystore cipher. + private val serverRepository: ExternalServerRepository = + ExternalServerRepository(AppDatabase.getDatabase(context).externalServerDao()), +) : TaskProcessor { override suspend fun process(task: SyncTaskEntity): Boolean { val gson = Gson() val payloadType = object : TypeToken>() {}.type @@ -586,10 +592,11 @@ class DownloadFileProcessor(private val context: Context) : TaskProcessor { destFile.parentFile?.mkdirs() val client = okhttp3.OkHttpClient() - val request = okhttp3.Request.Builder().url(remoteURL).build() return try { - val response = client.newCall(request).execute() + val request = okhttp3.Request.Builder().url(remoteURL) + mediaServerHeaders(taskId, remoteURL)?.forEach { (k, v) -> request.addHeader(k, v) } + val response = client.newCall(request.build()).execute() if (!response.isSuccessful) { Log.e("DownloadFileProcessor", "❌ Download failed: ${response.code}") return false @@ -653,6 +660,15 @@ class DownloadFileProcessor(private val context: Context) : TaskProcessor { } } + // Resolved per run, not stored in the payload: tokens stay out of the task table, and a re-auth's + // fresh token applies to an already-queued download. The resource pick mirrors externalStreamUrlFor. + private suspend fun mediaServerHeaders(uuid: String, url: String): Map? { + val resource = AppDatabase.getDatabase(context).libraryDao().getExternalResourcesForBookSync(uuid) + .find { it.syncStatus == ExternalResourceEntity.STATUS_STREAM || it.syncStatus == ExternalResourceEntity.STATUS_DOWNLOADED } + ?: return null + return ExternalServiceUtils.downloadHeadersFor(serverRepository, resource, url) + } + override fun canHandle(jobType: String): Boolean { return jobType == SyncTaskFactory.JOB_DOWNLOAD_FILE } diff --git a/core/src/main/java/com/tortugapower/audiobookplayer/logic/ExternalServiceUtils.kt b/core/src/main/java/com/tortugapower/audiobookplayer/logic/ExternalServiceUtils.kt index 5577a093..0896e564 100644 --- a/core/src/main/java/com/tortugapower/audiobookplayer/logic/ExternalServiceUtils.kt +++ b/core/src/main/java/com/tortugapower/audiobookplayer/logic/ExternalServiceUtils.kt @@ -136,16 +136,37 @@ object ExternalServiceUtils { } /** - * The provider's direct-download URL for [resource] on [server] (query-token auth, so it needs no - * extra headers), or null for an unknown provider. Pure counterpart of the URL rebuild in - * `resolveStreamingUrl`, also used to GET the source file for the stream-to-cloud pipe. + * The provider's direct-download URL for [resource] on [server], or null for an unknown provider. + * Pure counterpart of the URL rebuild in `resolveStreamingUrl`, also used to GET the source file for + * the stream-to-cloud pipe. The Jellyfin URL carries no token — Jellyfin 12 ignores `api_key`, and a + * URL token leaks into logs and the task table — so every request for it needs the provider's header + * auth: playback via PlaybackManager's host registry, the pipe and downloads via [downloadHeadersFor]. + * ABS keeps its `token` query param; its consumers send the Bearer header on top of it. */ fun downloadUrlFor(server: ExternalServerEntity, resource: ExternalResourceEntity): String? { val path = when (serviceTypeFor(resource.providerName)) { - ExternalServiceType.JELLYFIN -> "Items/${resource.providerId}/Download?api_key=${server.token ?: ""}" + ExternalServiceType.JELLYFIN -> "Items/${resource.providerId}/Download" ExternalServiceType.AUDIOBOOKSHELF -> "api/items/${resource.providerId}/download?token=${server.token ?: ""}" null -> return null } return "${sanitizeUrl(server.url)}$path" } + + /** + * The headers a download of [url] must carry when it comes from the saved server behind [resource]: + * the provider's Authorization header plus the user's custom headers, like playback and the pipe. + * The query token alone isn't enough — Jellyfin 12 rejects it (401), as do newer ABS versions. + * Null when [url] is anywhere else: a BookPlayer-cloud presigned URL must go out bare, since S3 + * rejects a request that carries a second auth mechanism. + */ + suspend fun downloadHeadersFor( + servers: ExternalServerRepository, + resource: ExternalResourceEntity, + url: String, + ): Map? { + val server = serverForResource(servers, resource) ?: return null + if (!url.startsWith(sanitizeUrl(server.url))) return null + val type = serviceTypeFor(resource.providerName) ?: return null + return playbackHeaders(type, server.token, sanitizeCustomHeaders(server.customHeaders)) + } } diff --git a/core/src/main/java/com/tortugapower/audiobookplayer/logic/PlaybackManager.kt b/core/src/main/java/com/tortugapower/audiobookplayer/logic/PlaybackManager.kt index 50710545..e98c5332 100644 --- a/core/src/main/java/com/tortugapower/audiobookplayer/logic/PlaybackManager.kt +++ b/core/src/main/java/com/tortugapower/audiobookplayer/logic/PlaybackManager.kt @@ -707,7 +707,7 @@ object PlaybackManager { * Best-effort audio file extension for picking the chapter parser, from most to least reliable: * the item's `relativePath`, then its `originalFileName` (set for external items whose relativePath * is null, e.g. AudiobookShelf), then the remote URL's last path segment with any query/fragment - * stripped. A streaming URL like `Items//Download?api_key=...` yields no extension → we fall + * stripped. A streaming URL like `Items//Download` yields no extension → we fall * through rather than mis-detecting. Pure (no Android APIs) so it's unit-tested. Lowercased, no dot. */ internal fun audioExtensionFor(item: LibraryItemEntity, url: String): String { diff --git a/core/src/test/java/com/tortugapower/audiobookplayer/logic/DownloadFileProcessorTest.kt b/core/src/test/java/com/tortugapower/audiobookplayer/logic/DownloadFileProcessorTest.kt index 257b39eb..aa48e082 100644 --- a/core/src/test/java/com/tortugapower/audiobookplayer/logic/DownloadFileProcessorTest.kt +++ b/core/src/test/java/com/tortugapower/audiobookplayer/logic/DownloadFileProcessorTest.kt @@ -3,12 +3,23 @@ package com.tortugapower.audiobookplayer.logic import android.content.Context import androidx.test.core.app.ApplicationProvider import com.tortugapower.audiobookplayer.database.AppDatabase +import com.tortugapower.audiobookplayer.database.entities.ExternalResourceEntity +import com.tortugapower.audiobookplayer.database.entities.ExternalServerEntity +import com.tortugapower.audiobookplayer.database.entities.ExternalServiceType import com.tortugapower.audiobookplayer.database.entities.ItemType import com.tortugapower.audiobookplayer.database.entities.LibraryItemEntity import com.tortugapower.audiobookplayer.database.entities.SyncTaskEntity +import com.tortugapower.audiobookplayer.repository.ExternalServerRepository +import com.tortugapower.audiobookplayer.repository.TokenCipher import kotlinx.coroutines.runBlocking +import okhttp3.mockwebserver.MockResponse +import okhttp3.mockwebserver.MockWebServer +import org.junit.After +import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull import org.junit.Assert.assertTrue +import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner @@ -19,11 +30,35 @@ import java.io.File * (no backing file; its stored remoteURL 404s), so the processor must report it done — deleting it — * instead of failing and blocking the serial file queue with infinite retries. Legacy library taps used to * enqueue the container itself; [OfflineDownloadManager] now fans containers out into BOOK-file tasks. + * + * Also covers media-server auth: a download from the saved Jellyfin/ABS server carries the same header + * auth as playback (Jellyfin 12 rejects the URL's query token alone), while a BookPlayer-cloud URL for the + * same item goes out without it. */ @RunWith(RobolectricTestRunner::class) class DownloadFileProcessorTest { private val context = ApplicationProvider.getApplicationContext() + private val mediaServer = MockWebServer() + private val cloud = MockWebServer() + private val uuid = "jf-book-1" + private val relativePath = "Book One.m4b" + + @Before fun setUp() { + mediaServer.start() + cloud.start() + // The AppDatabase singleton persists across test methods — start each test from empty tables. + runBlocking(kotlinx.coroutines.Dispatchers.IO) { AppDatabase.getDatabase(context).clearAllTables() } + // Robolectric's StatFs reports no free space; the storage guard would refuse every download. + StorageMonitor.availableBytesProvider = { Long.MAX_VALUE / 2 } + } + + @After fun tearDown() { + mediaServer.shutdown() + cloud.shutdown() + StorageMonitor.availableBytesProvider = { ctx -> android.os.StatFs(ctx.filesDir.path).availableBytes } + OfflineDownloadManager.processedFile(context, relativePath).delete() + } private fun containerItem(uuid: String, type: ItemType) = LibraryItemEntity( uuid = uuid, title = "002", author = null, duration = 0.0, currentTime = 0.0, @@ -38,6 +73,40 @@ class DownloadFileProcessorTest { payload = """{"uuid":"$uuid","title":"002","relativePath":"002","remoteURL":"https://example.invalid/002_"}""", ) + private fun bookDownloadTask(remoteURL: String) = SyncTaskEntity( + id = "row-$uuid", taskID = uuid, queueKey = SyncTaskFactory.QUEUE_FILE, + jobType = SyncTaskFactory.JOB_DOWNLOAD_FILE, position = 0, + payload = """{"uuid":"$uuid","title":"Book One","relativePath":"$relativePath","remoteURL":"$remoteURL"}""", + ) + + // Reversible stand-in for the Keystore cipher (same pattern as StreamFileUploadProcessorTest) — the + // stored row is ciphertext, and the download must authenticate with the DECRYPTED token. + private val fakeCipher = object : TokenCipher { + override fun encrypt(plaintext: String) = "ENC($plaintext)" + override fun decrypt(stored: String) = stored.removePrefix("ENC(").removeSuffix(")") + } + + private fun serverRepository() = + ExternalServerRepository(AppDatabase.getDatabase(context).externalServerDao(), fakeCipher) + + private suspend fun insertJellyfinBook() { + AppDatabase.getDatabase(context).libraryDao().insertItemWithExternalResource( + LibraryItemEntity(uuid = uuid, title = "Book One", relativePath = relativePath, type = ItemType.BOOK), + ExternalResourceEntity( + providerName = "jellyfin", providerId = "jf-9", + syncStatus = ExternalResourceEntity.STATUS_STREAM, libraryItemUuid = uuid, hostId = "srv-guid", + ), + ) + serverRepository().saveServer( + ExternalServerEntity( + id = 1, name = "jf", type = ExternalServiceType.JELLYFIN, url = mediaServer.url("/").toString(), + token = "tok", stableId = "srv-guid", customHeaders = mapOf("CF-Access-Client-Id" to "cf-id"), + ), + ) + } + + private fun processor() = DownloadFileProcessor(context, serverRepository()) + @Test fun `container download task is dropped as done, not retried`() = runBlocking { AppDatabase.getDatabase(context).libraryDao() .insertItem(containerItem("bound-1", ItemType.BOUND)) @@ -49,4 +118,31 @@ class DownloadFileProcessorTest { // The guard bails before any network/disk write — no stray Processed file for the container path. assertFalse(File(File(context.filesDir, "Processed"), "002").exists()) } + + @Test fun `media-server download carries the server's auth and custom headers`() = runBlocking { + insertJellyfinBook() + mediaServer.enqueue(MockResponse().setBody("audio-bytes")) + + val handled = processor().process(bookDownloadTask(mediaServer.url("/Items/jf-9/Download").toString())) + + assertTrue(handled) + val get = mediaServer.takeRequest() + assertEquals("/Items/jf-9/Download", get.path) + assertEquals("MediaBrowser Token=\"tok\"", get.getHeader("Authorization")) + assertEquals("cf-id", get.getHeader("CF-Access-Client-Id")) + assertEquals("audio-bytes", OfflineDownloadManager.processedFile(context, relativePath).readText()) + } + + @Test fun `cloud download of a media-server item goes out without media-server auth`() = runBlocking { + insertJellyfinBook() + cloud.enqueue(MockResponse().setBody("audio-bytes")) + + val handled = processor().process(bookDownloadTask(cloud.url("/presigned/book.m4b?X-Amz-Signature=sig").toString())) + + assertTrue(handled) + val get = cloud.takeRequest() + // S3 rejects a presigned request that also carries an Authorization header. + assertNull(get.getHeader("Authorization")) + assertNull(get.getHeader("CF-Access-Client-Id")) + } } diff --git a/core/src/test/java/com/tortugapower/audiobookplayer/logic/ExternalStreamUrlTest.kt b/core/src/test/java/com/tortugapower/audiobookplayer/logic/ExternalStreamUrlTest.kt index f6f455a5..fbf0822b 100644 --- a/core/src/test/java/com/tortugapower/audiobookplayer/logic/ExternalStreamUrlTest.kt +++ b/core/src/test/java/com/tortugapower/audiobookplayer/logic/ExternalStreamUrlTest.kt @@ -22,9 +22,10 @@ class ExternalStreamUrlTest { syncStatus = ExternalResourceEntity.STATUS_STREAM, libraryItemUuid = "b1", hostId = "1", ) - @Test fun `jellyfin download URL carries the api_key query token`() { + // No token in the URL: Jellyfin 12 ignores `api_key`, so consumers authenticate with the header. + @Test fun `jellyfin download URL carries no token`() { assertEquals( - "https://media.example.com/Items/item-9/Download?api_key=tok-1", + "https://media.example.com/Items/item-9/Download", ExternalServiceUtils.downloadUrlFor(server(ExternalServiceType.JELLYFIN), resource("jellyfin")), ) } @@ -38,7 +39,7 @@ class ExternalStreamUrlTest { @Test fun `trailing-slash server URL does not double the slash`() { assertEquals( - "https://media.example.com/Items/item-9/Download?api_key=tok-1", + "https://media.example.com/Items/item-9/Download", ExternalServiceUtils.downloadUrlFor( server(ExternalServiceType.JELLYFIN, url = "https://media.example.com/"), resource("jellyfin"), ), diff --git a/core/src/test/java/com/tortugapower/audiobookplayer/logic/StreamFileUploadProcessorTest.kt b/core/src/test/java/com/tortugapower/audiobookplayer/logic/StreamFileUploadProcessorTest.kt index da691dc5..459ab982 100644 --- a/core/src/test/java/com/tortugapower/audiobookplayer/logic/StreamFileUploadProcessorTest.kt +++ b/core/src/test/java/com/tortugapower/audiobookplayer/logic/StreamFileUploadProcessorTest.kt @@ -101,9 +101,9 @@ class StreamFileUploadProcessorTest { assertTrue(handled) val get = server.takeRequest() assertEquals("GET", get.method) - // Query-token download URL derived from the saved server + resource... - assertEquals("/Items/jf-9/Download?api_key=tok", get.path) - // ...PLUS header auth, like playback: newer ABS versions 401 on query-string tokens. + // Download URL derived from the saved server + resource, with no token in it... + assertEquals("/Items/jf-9/Download", get.path) + // ...so the header carries the auth, like playback (Jellyfin 12 and newer ABS 401 on query tokens). assertEquals("MediaBrowser Token=\"tok\"", get.getHeader("Authorization")) val put = server.takeRequest() assertEquals("PUT", put.method) From 5f7165eb652822c2213ea9f30cefe9a497ad03a1 Mon Sep 17 00:00:00 2001 From: Gianni Carlo Date: Tue, 29 Sep 2026 12:13:56 -0500 Subject: [PATCH 3/5] fix: address review feedback (round 1) DownloadFileProcessor reads the response inside `use`, so an error status or a refused (no room) download no longer leaks the connection on each retry; with header auth an expired token is an ordinary 401. The stream-to-cloud pipe sanitizes custom headers like the download does: a persisted illegal header threw on addHeader and failed every attempt. --- .../audiobookplayer/logic/CoreProcessors.kt | 100 +++++++++--------- .../logic/DownloadFileProcessorTest.kt | 13 +++ .../logic/StreamFileUploadProcessorTest.kt | 18 +++- 3 files changed, 81 insertions(+), 50 deletions(-) diff --git a/core/src/main/java/com/tortugapower/audiobookplayer/logic/CoreProcessors.kt b/core/src/main/java/com/tortugapower/audiobookplayer/logic/CoreProcessors.kt index a6006811..c7244456 100644 --- a/core/src/main/java/com/tortugapower/audiobookplayer/logic/CoreProcessors.kt +++ b/core/src/main/java/com/tortugapower/audiobookplayer/logic/CoreProcessors.kt @@ -449,10 +449,11 @@ class StreamFileUploadProcessor( Log.w("StreamFileUploadProcessor", "⚠️ No saved server can serve ${item.title} — will retry") return false } - // Header auth on top of the query token, like playback and the artwork backfill: newer ABS - // versions reject query-string tokens (401) and only accept the Authorization header. + // Header auth, like playback and the artwork backfill: the Jellyfin URL carries no token, and + // newer ABS versions reject query-string tokens (401). Custom headers are sanitized like the + // download's: a persisted illegal name/value throws on addHeader and would wedge the pipe. val headers = ExternalServiceUtils.serviceTypeFor(resource.providerName) - ?.let { ExternalServiceUtils.playbackHeaders(it, server.token, server.customHeaders) } + ?.let { ExternalServiceUtils.playbackHeaders(it, server.token, ExternalServiceUtils.sanitizeCustomHeaders(server.customHeaders)) } return try { val getRequest = okhttp3.Request.Builder().url(sourceUrl) @@ -596,57 +597,60 @@ class DownloadFileProcessor( return try { val request = okhttp3.Request.Builder().url(remoteURL) mediaServerHeaders(taskId, remoteURL)?.forEach { (k, v) -> request.addHeader(k, v) } - val response = client.newCall(request.build()).execute() - if (!response.isSuccessful) { - Log.e("DownloadFileProcessor", "❌ Download failed: ${response.code}") - return false - } - - val body = response.body ?: return false - val contentLength = body.contentLength() - // Refuse up front when the file can't fit with headroom to spare: a download that fills the - // disk takes the database down with it. The engine holds downloads until storage recovers. - if (contentLength > 0 && !StorageMonitor.hasRoomFor(context, contentLength)) { - StorageMonitor.noteTransferDoesNotFit(context, contentLength) - Log.w("DownloadFileProcessor", "⛔ Not enough storage for $relativePath ($contentLength bytes)") - return false - } - var bytesRead = 0L - var cancelled = false + // `use` closes the response on every path: the early returns below (error status, no room) would + // otherwise leak the connection on each retry. + client.newCall(request.build()).execute().use { response -> + if (!response.isSuccessful) { + Log.e("DownloadFileProcessor", "❌ Download failed: ${response.code}") + return false + } - body.byteStream().use { input: java.io.InputStream -> - FileOutputStream(destFile).use { output: FileOutputStream -> - val buffer = ByteArray(8 * 1024) - var read: Int - while (input.read(buffer).also { read = it } != -1) { - // Cooperative cancellation: abort mid-stream if the user cancelled this download. - if (SyncStatusManager.isCancelRequested(taskId)) { - cancelled = true - break - } - output.write(buffer, 0, read) - bytesRead += read - if (contentLength > 0) { - val progress = bytesRead.toDouble() / contentLength - SyncStatusManager.updateTaskProgress(taskId, progress) + val body = response.body ?: return false + val contentLength = body.contentLength() + // Refuse up front when the file can't fit with headroom to spare: a download that fills the + // disk takes the database down with it. The engine holds downloads until storage recovers. + if (contentLength > 0 && !StorageMonitor.hasRoomFor(context, contentLength)) { + StorageMonitor.noteTransferDoesNotFit(context, contentLength) + Log.w("DownloadFileProcessor", "⛔ Not enough storage for $relativePath ($contentLength bytes)") + return false + } + var bytesRead = 0L + var cancelled = false + + body.byteStream().use { input: java.io.InputStream -> + FileOutputStream(destFile).use { output: FileOutputStream -> + val buffer = ByteArray(8 * 1024) + var read: Int + while (input.read(buffer).also { read = it } != -1) { + // Cooperative cancellation: abort mid-stream if the user cancelled this download. + if (SyncStatusManager.isCancelRequested(taskId)) { + cancelled = true + break + } + output.write(buffer, 0, read) + bytesRead += read + if (contentLength > 0) { + val progress = bytesRead.toDouble() / contentLength + SyncStatusManager.updateTaskProgress(taskId, progress) + } } + output.flush() } - output.flush() } - } - SyncStatusManager.clearTaskProgress(taskId) - if (cancelled) { - Log.d("DownloadFileProcessor", "🚫 Download cancelled: $relativePath") - if (destFile.exists()) destFile.delete() - // Leave the cancel flag SET on purpose: TaskConcurrencyManager reads it on this false - // return to make the task terminal (delete, no retry) and then clears it. Clearing here - // would let the failure path re-queue the task and silently re-download it to completion. - return false + SyncStatusManager.clearTaskProgress(taskId) + if (cancelled) { + Log.d("DownloadFileProcessor", "🚫 Download cancelled: $relativePath") + if (destFile.exists()) destFile.delete() + // Leave the cancel flag SET on purpose: TaskConcurrencyManager reads it on this false + // return to make the task terminal (delete, no retry) and then clears it. Clearing here + // would let the failure path re-queue the task and silently re-download it to completion. + return false + } + SyncStatusManager.clearCancel(taskId) + Log.d("DownloadFileProcessor", "✅ Download complete: $relativePath") + true } - SyncStatusManager.clearCancel(taskId) - Log.d("DownloadFileProcessor", "✅ Download complete: $relativePath") - true } catch (e: Exception) { StorageMonitor.reportFailure(context, e) // ENOSPC mid-write: the storage state holds further downloads Log.e("DownloadFileProcessor", "💥 Exception during download: ${e.message}", e) diff --git a/core/src/test/java/com/tortugapower/audiobookplayer/logic/DownloadFileProcessorTest.kt b/core/src/test/java/com/tortugapower/audiobookplayer/logic/DownloadFileProcessorTest.kt index aa48e082..19cea402 100644 --- a/core/src/test/java/com/tortugapower/audiobookplayer/logic/DownloadFileProcessorTest.kt +++ b/core/src/test/java/com/tortugapower/audiobookplayer/logic/DownloadFileProcessorTest.kt @@ -133,6 +133,19 @@ class DownloadFileProcessorTest { assertEquals("audio-bytes", OfflineDownloadManager.processedFile(context, relativePath).readText()) } + // An expired token is now an ordinary failure: retryable (false), nothing written, and the response is + // closed (the processor reads it inside `use`) so repeated retries don't leak connections. + @Test fun `rejected media-server download is retryable and writes nothing`() = runBlocking { + insertJellyfinBook() + mediaServer.enqueue(MockResponse().setResponseCode(401).setBody("expired")) + + val handled = processor().process(bookDownloadTask(mediaServer.url("/Items/jf-9/Download").toString())) + + assertFalse(handled) + assertEquals(1, mediaServer.requestCount) + assertFalse(OfflineDownloadManager.processedFile(context, relativePath).exists()) + } + @Test fun `cloud download of a media-server item goes out without media-server auth`() = runBlocking { insertJellyfinBook() cloud.enqueue(MockResponse().setBody("audio-bytes")) diff --git a/core/src/test/java/com/tortugapower/audiobookplayer/logic/StreamFileUploadProcessorTest.kt b/core/src/test/java/com/tortugapower/audiobookplayer/logic/StreamFileUploadProcessorTest.kt index 459ab982..7148cde8 100644 --- a/core/src/test/java/com/tortugapower/audiobookplayer/logic/StreamFileUploadProcessorTest.kt +++ b/core/src/test/java/com/tortugapower/audiobookplayer/logic/StreamFileUploadProcessorTest.kt @@ -80,9 +80,9 @@ class StreamFileUploadProcessorTest { AppDatabase.getDatabase(context).externalServerDao(), fakeCipher, ) - private suspend fun insertServer() { + private suspend fun insertServer(customHeaders: Map? = null) { serverRepository().saveServer( - ExternalServerEntity(id = 1, name = "jf", type = ExternalServiceType.JELLYFIN, url = server.url("/").toString(), token = "tok", stableId = "srv-guid"), + ExternalServerEntity(id = 1, name = "jf", type = ExternalServiceType.JELLYFIN, url = server.url("/").toString(), token = "tok", stableId = "srv-guid", customHeaders = customHeaders), ) } @@ -122,6 +122,20 @@ class StreamFileUploadProcessorTest { assertEquals(1.0, SyncStatusManager.taskProgress.value["row-1"]!!, 0.0001) } + // A persisted illegal header (BOOKPLAYER-B: a Cyrillic name) throws on addHeader; unsanitized it + // failed every attempt and wedged the pipe. The legal custom header still rides along. + @Test fun `illegal custom headers are dropped instead of failing the source GET`() = runBlocking { + insertStreamItem(); insertServer(mapOf("Заголовок" to "x", "CF-Access-Client-Id" to "cf-id")) + server.enqueue(MockResponse().setBody("audio")) // Jellyfin GET + server.enqueue(MockResponse()) // S3 PUT + + assertTrue(processor(putUrl = server.url("/s3-put").toString()).process(task())) + + val get = server.takeRequest() + assertEquals("MediaBrowser Token=\"tok\"", get.getHeader("Authorization")) + assertEquals("cf-id", get.getHeader("CF-Access-Client-Id")) + } + @Test fun `unknown source length stages through cache and still PUTs a fixed-length body`() = runBlocking { insertStreamItem(); insertServer() val audio = "chunked-audio-payload" From 8aba9c8e7c69a91eeb5b3dbe84cb6fc4c01d0fa3 Mon Sep 17 00:00:00 2001 From: Gianni Carlo Date: Tue, 29 Sep 2026 12:25:38 -0500 Subject: [PATCH 4/5] fix: address review feedback (round 2) Downloads and the pipe's source GET attach media-server headers through a network interceptor that adds them only to hops on the server's origin (scheme, host, port). OkHttp drops Authorization on a cross-host redirect but keeps custom headers, which are often Cloudflare Access secrets; playback already pins its headers to the server's host the same way. --- .../audiobookplayer/logic/CoreProcessors.kt | 22 ++++++++++++------- .../logic/ExternalServiceUtils.kt | 21 ++++++++++++++++++ .../logic/DownloadFileProcessorTest.kt | 20 +++++++++++++++++ .../logic/StreamFileUploadProcessorTest.kt | 22 +++++++++++++++++++ 4 files changed, 77 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/com/tortugapower/audiobookplayer/logic/CoreProcessors.kt b/core/src/main/java/com/tortugapower/audiobookplayer/logic/CoreProcessors.kt index c7244456..3612012f 100644 --- a/core/src/main/java/com/tortugapower/audiobookplayer/logic/CoreProcessors.kt +++ b/core/src/main/java/com/tortugapower/audiobookplayer/logic/CoreProcessors.kt @@ -456,9 +456,12 @@ class StreamFileUploadProcessor( ?.let { ExternalServiceUtils.playbackHeaders(it, server.token, ExternalServiceUtils.sanitizeCustomHeaders(server.customHeaders)) } return try { - val getRequest = okhttp3.Request.Builder().url(sourceUrl) - headers?.forEach { (k, v) -> getRequest.addHeader(k, v) } - client.newCall(getRequest.build()).execute().use { response -> + // Headers ride only the hops that stay on the media server (a redirect elsewhere must not get + // them); newBuilder() shares the pipe client's connection pool. + val sourceClient = headers?.let { + client.newBuilder().addNetworkInterceptor(ExternalServiceUtils.originPinnedHeaders(sourceUrl, it)).build() + } ?: client + sourceClient.newCall(okhttp3.Request.Builder().url(sourceUrl).build()).execute().use { response -> val body = response.body if (!response.isSuccessful || body == null) { Log.e("StreamFileUploadProcessor", "❌ Source GET failed (${response.code}) for ${item.title}") @@ -592,14 +595,17 @@ class DownloadFileProcessor( // Ensure parent directories exist destFile.parentFile?.mkdirs() - val client = okhttp3.OkHttpClient() - return try { - val request = okhttp3.Request.Builder().url(remoteURL) - mediaServerHeaders(taskId, remoteURL)?.forEach { (k, v) -> request.addHeader(k, v) } + // Media-server headers ride only the hops that stay on that server: OkHttp would carry custom + // headers (often Cloudflare Access secrets) across a redirect to another host. + val client = okhttp3.OkHttpClient.Builder().apply { + mediaServerHeaders(taskId, remoteURL)?.let { + addNetworkInterceptor(ExternalServiceUtils.originPinnedHeaders(remoteURL, it)) + } + }.build() // `use` closes the response on every path: the early returns below (error status, no room) would // otherwise leak the connection on each retry. - client.newCall(request.build()).execute().use { response -> + client.newCall(okhttp3.Request.Builder().url(remoteURL).build()).execute().use { response -> if (!response.isSuccessful) { Log.e("DownloadFileProcessor", "❌ Download failed: ${response.code}") return false diff --git a/core/src/main/java/com/tortugapower/audiobookplayer/logic/ExternalServiceUtils.kt b/core/src/main/java/com/tortugapower/audiobookplayer/logic/ExternalServiceUtils.kt index 0896e564..4d4e9b45 100644 --- a/core/src/main/java/com/tortugapower/audiobookplayer/logic/ExternalServiceUtils.kt +++ b/core/src/main/java/com/tortugapower/audiobookplayer/logic/ExternalServiceUtils.kt @@ -5,6 +5,8 @@ import com.tortugapower.audiobookplayer.database.entities.ExternalServerEntity import com.tortugapower.audiobookplayer.database.entities.ExternalServiceType import com.tortugapower.audiobookplayer.repository.ExternalServerRepository import kotlinx.coroutines.flow.first +import okhttp3.HttpUrl.Companion.toHttpUrlOrNull +import okhttp3.Interceptor object ExternalServiceUtils { fun sanitizeUrl(url: String): String { @@ -169,4 +171,23 @@ object ExternalServiceUtils { val type = serviceTypeFor(resource.providerName) ?: return null return playbackHeaders(type, server.token, sanitizeCustomHeaders(server.customHeaders)) } + + /** + * A network interceptor that adds [headers] to each hop of a request only while it stays on [url]'s + * origin (scheme, host and port — the rule OkHttp applies to `Authorization` on redirects). OkHttp + * keeps every other header across a cross-host redirect, and custom headers are often Cloudflare + * Access secrets; playback pins its headers to the server's host the same way. + */ + fun originPinnedHeaders(url: String, headers: Map): Interceptor { + val origin = url.toHttpUrlOrNull() + return Interceptor { chain -> + val request = chain.request() + val sameOrigin = origin != null && request.url.scheme == origin.scheme && + request.url.host == origin.host && request.url.port == origin.port + if (!sameOrigin) return@Interceptor chain.proceed(request) + val pinned = request.newBuilder() + headers.forEach { (name, value) -> pinned.header(name, value) } + chain.proceed(pinned.build()) + } + } } diff --git a/core/src/test/java/com/tortugapower/audiobookplayer/logic/DownloadFileProcessorTest.kt b/core/src/test/java/com/tortugapower/audiobookplayer/logic/DownloadFileProcessorTest.kt index 19cea402..e89f92d8 100644 --- a/core/src/test/java/com/tortugapower/audiobookplayer/logic/DownloadFileProcessorTest.kt +++ b/core/src/test/java/com/tortugapower/audiobookplayer/logic/DownloadFileProcessorTest.kt @@ -146,6 +146,26 @@ class DownloadFileProcessorTest { assertFalse(OfflineDownloadManager.processedFile(context, relativePath).exists()) } + // OkHttp drops Authorization on a cross-host redirect but keeps custom headers, which are often + // Cloudflare Access secrets: the headers are pinned to the media server's origin, hop by hop. + @Test fun `a redirect off the media server gets none of its headers`() = runBlocking { + insertJellyfinBook() + mediaServer.enqueue(MockResponse().setResponseCode(302).setHeader("Location", cloud.url("/elsewhere/book.m4b"))) + cloud.enqueue(MockResponse().setBody("audio-bytes")) + + val handled = processor().process(bookDownloadTask(mediaServer.url("/Items/jf-9/Download").toString())) + + assertTrue(handled) + val first = mediaServer.takeRequest() + assertEquals("MediaBrowser Token=\"tok\"", first.getHeader("Authorization")) + assertEquals("cf-id", first.getHeader("CF-Access-Client-Id")) + val redirected = cloud.takeRequest() + assertEquals("/elsewhere/book.m4b", redirected.path) + assertNull(redirected.getHeader("Authorization")) + assertNull(redirected.getHeader("CF-Access-Client-Id")) + assertEquals("audio-bytes", OfflineDownloadManager.processedFile(context, relativePath).readText()) + } + @Test fun `cloud download of a media-server item goes out without media-server auth`() = runBlocking { insertJellyfinBook() cloud.enqueue(MockResponse().setBody("audio-bytes")) diff --git a/core/src/test/java/com/tortugapower/audiobookplayer/logic/StreamFileUploadProcessorTest.kt b/core/src/test/java/com/tortugapower/audiobookplayer/logic/StreamFileUploadProcessorTest.kt index 7148cde8..078004eb 100644 --- a/core/src/test/java/com/tortugapower/audiobookplayer/logic/StreamFileUploadProcessorTest.kt +++ b/core/src/test/java/com/tortugapower/audiobookplayer/logic/StreamFileUploadProcessorTest.kt @@ -136,6 +136,28 @@ class StreamFileUploadProcessorTest { assertEquals("cf-id", get.getHeader("CF-Access-Client-Id")) } + // Custom headers (often Cloudflare Access secrets) survive a cross-host redirect in OkHttp; the source + // GET pins them to the media server's origin so another host never receives them. + @Test fun `a source redirect off the media server gets none of its headers`() = runBlocking { + val elsewhere = MockWebServer().apply { start() } + try { + insertStreamItem(); insertServer(mapOf("CF-Access-Client-Id" to "cf-id")) + server.enqueue(MockResponse().setResponseCode(302).setHeader("Location", elsewhere.url("/cdn/book.m4b"))) + elsewhere.enqueue(MockResponse().setBody("audio")) + server.enqueue(MockResponse()) // S3 PUT + + assertTrue(processor(putUrl = server.url("/s3-put").toString()).process(task())) + + assertEquals("cf-id", server.takeRequest().getHeader("CF-Access-Client-Id")) + val redirected = elsewhere.takeRequest() + assertNull(redirected.getHeader("Authorization")) + assertNull(redirected.getHeader("CF-Access-Client-Id")) + assertEquals("audio", server.takeRequest().body.readUtf8()) + } finally { + elsewhere.shutdown() + } + } + @Test fun `unknown source length stages through cache and still PUTs a fixed-length body`() = runBlocking { insertStreamItem(); insertServer() val audio = "chunked-audio-payload" From 0b62d8a4b0a0d2f65393d634882e1182217b344b Mon Sep 17 00:00:00 2001 From: Gianni Carlo Date: Tue, 29 Sep 2026 12:43:36 -0500 Subject: [PATCH 5/5] fix: address review feedback (round 3) DownloadFileProcessor keeps one base OkHttpClient and derives the per-server variant from it with newBuilder(), like the progress push and the pipe, so queued downloads and their retries share one connection pool and dispatcher instead of building a new client each time. --- .../audiobookplayer/logic/CoreProcessors.kt | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/com/tortugapower/audiobookplayer/logic/CoreProcessors.kt b/core/src/main/java/com/tortugapower/audiobookplayer/logic/CoreProcessors.kt index 3612012f..0507960b 100644 --- a/core/src/main/java/com/tortugapower/audiobookplayer/logic/CoreProcessors.kt +++ b/core/src/main/java/com/tortugapower/audiobookplayer/logic/CoreProcessors.kt @@ -564,6 +564,12 @@ class DownloadFileProcessor( private val serverRepository: ExternalServerRepository = ExternalServerRepository(AppDatabase.getDatabase(context).externalServerDao()), ) : TaskProcessor { + companion object { + // One base client for every download (and retry); per-server variants derive via newBuilder(), + // which shares this client's connection pool and dispatcher threads. + private val baseHttpClient by lazy { okhttp3.OkHttpClient() } + } + override suspend fun process(task: SyncTaskEntity): Boolean { val gson = Gson() val payloadType = object : TypeToken>() {}.type @@ -598,11 +604,9 @@ class DownloadFileProcessor( return try { // Media-server headers ride only the hops that stay on that server: OkHttp would carry custom // headers (often Cloudflare Access secrets) across a redirect to another host. - val client = okhttp3.OkHttpClient.Builder().apply { - mediaServerHeaders(taskId, remoteURL)?.let { - addNetworkInterceptor(ExternalServiceUtils.originPinnedHeaders(remoteURL, it)) - } - }.build() + val client = mediaServerHeaders(taskId, remoteURL)?.let { + baseHttpClient.newBuilder().addNetworkInterceptor(ExternalServiceUtils.originPinnedHeaders(remoteURL, it)).build() + } ?: baseHttpClient // `use` closes the response on every path: the early returns below (error status, no room) would // otherwise leak the connection on each retry. client.newCall(okhttp3.Request.Builder().url(remoteURL).build()).execute().use { response ->