A feed must not be able to spend the server's token

The security review of the last commit found a confused deputy in it, and it was right. The
bearer was attached on origin alone, and `episode.mediaUrl` comes out of a feed -- so a publisher
could set an enclosure URL on the skip server's own origin, match the origin check exactly, and
have the app issue an authenticated GET to a path of their choosing, storing whatever came back
as that episode's audio. Confirmed by writing the test before the fix: it failed.

Provenance is the first gate now and origin is the second. `authenticated = true` is passed only
for the URL `ServerAudio.resolve` returned, never for one that arrived in a feed, and the origin
check still has to agree -- so a server answer naming some other host is fetched without
credentials rather than sending them there. Both gates have a test, including the case where they
disagree, and OutboundRequestsTest pins that exactly one call site in the app is authenticated.

**Chasing the coverage gate honestly turned up a real bug**, which is the second time this week
it has earned its keep. Rather than file a third COVERAGE_EXCEPTION I went to see what the
uncovered lines in EpisodeDownloader actually were, and wrote a test for the empty-response
branch. It failed: OkHttp hands back an empty body rather than null for a 204, so the existing
`?: run { ... }` guard never fires, the copy writes nothing, and a **zero-byte file is renamed
into place and recorded as DOWNLOADED**. That episode then fails to play and reads as the
publisher's fault. The length is checked after the copy now, which catches a 204, an empty 200
and anything else that arrives with no bytes.

Also tested while there: the media-type to file-extension mapping, which had none. Not cosmetic
-- Media3 picks an extractor partly from the extension, so a .audio file holding mp4 bytes is a
plausible way to get an episode that will not play. Parameters and capitals included, because
feeds send "audio/mpeg; charset=utf-8" and "AUDIO/MPEG".

1059 tests, detekt 0, lint clean, coverage 94.09% -> 94.28% with no exception.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018AwnALZL6bD4VBtATDVv3B

3 files changed, 186 insertions, 4 deletions

diff --git a/app/src/main/java/org/terst/listen/download/EpisodeDownloader.kt b/app/src/main/java/org/terst/listen/download/EpisodeDownloader.kt
index 652f494..1745157 100644
--- a/app/src/main/java/org/terst/listen/download/EpisodeDownloader.kt
+++ b/app/src/main/java/org/terst/listen/download/EpisodeDownloader.kt
@@ -209,7 +209,7 @@ class EpisodeDownloader(
         val serverUrl = info?.audioUrl?.takeIf { info.ready }
             ?: return Result.failure(WaitingForServerException())
 
-        val result = downloadOverHttp(episode, serverUrl)
+        val result = downloadOverHttp(episode, serverUrl, authenticated = true)
         if (result.isFailure) return Result.failure(WaitingForServerException(result.exceptionOrNull()))
 
         server.record(episode, info)
@@ -222,7 +222,15 @@ class EpisodeDownloader(
         return runCatching { source.download(episode.mediaUrl, libraryDir) }
     }
 
-    private suspend fun downloadOverHttp(episode: EpisodeEntity, url: String): Result<File> =
+    /**
+     * [authenticated] is true only for a URL the skip server itself returned, never for one that
+     * arrived in a feed. See the header block below for why origin alone is not enough.
+     */
+    private suspend fun downloadOverHttp(
+        episode: EpisodeEntity,
+        url: String,
+        authenticated: Boolean = false,
+    ): Result<File> =
         withContext(Dispatchers.IO) {
             val target = targetFile(episode)
             // Written to a partial name and renamed on success. A truncated file recorded as
@@ -232,8 +240,16 @@ class EpisodeDownloader(
 
             try {
                 val request = Request.Builder().url(url).header("User-Agent", USER_AGENT)
-                    // Only ever for the skip server's own origin; see ServerAudio.bearerFor.
-                    .apply { serverAudio?.bearerFor(url)?.let { header("Authorization", "Bearer $it") } }
+                    // Provenance first, then origin. [authenticated] says the server chose this
+                    // URL; bearerFor says it really is the server's. Both are required, because
+                    // either alone has a hole: an enclosure URL comes from the feed, so a
+                    // publisher could point one at the server's own origin and have the app
+                    // spend the token on a path of their choosing.
+                    .apply {
+                        if (authenticated) {
+                            serverAudio?.bearerFor(url)?.let { header("Authorization", "Bearer $it") }
+                        }
+                    }
                     .build()
                 client.newCall(request).execute().use { response ->
                     if (!response.isSuccessful) {
@@ -249,6 +265,14 @@ class EpisodeDownloader(
                     body.byteStream().use { input ->
                         partial.outputStream().use { output -> input.copyTo(output) }
                     }
+                    // A 204, or a 200 with nothing in it, reaches here: OkHttp hands back an
+                    // empty body rather than null, so the check above never fires and the copy
+                    // writes no bytes. Renaming that into place records a zero-byte episode as
+                    // DOWNLOADED -- it fails to play, and reads as the publisher's fault.
+                    if (partial.length() == 0L) {
+                        partial.delete()
+                        return@withContext Result.failure(IOException("Empty response"))
+                    }
                 }
                 if (!partial.renameTo(target)) {
                     partial.delete()
diff --git a/app/src/test/java/org/terst/listen/OutboundRequestsTest.kt b/app/src/test/java/org/terst/listen/OutboundRequestsTest.kt
index 8b40d06..3d59071 100644
--- a/app/src/test/java/org/terst/listen/OutboundRequestsTest.kt
+++ b/app/src/test/java/org/terst/listen/OutboundRequestsTest.kt
@@ -104,4 +104,31 @@ class OutboundRequestsTest {
             streaming.isEmpty(),
         )
     }
+
+    /**
+     * Exactly one request in the app carries the skip server's token.
+     *
+     * `downloadOverHttp` fetches both the server's copy of an episode and the publisher's, and
+     * only the first may be authenticated. Origin matching alone does not make that safe: an
+     * enclosure URL comes out of a feed, so a publisher can point one at the server's own origin
+     * and have the app spend the token on a path they chose. Provenance is the first gate and
+     * `authenticated = true` is how it is spelled, so there should be one of it -- the call that
+     * fetches what `ServerAudio.resolve` returned.
+     */
+    @Test
+    fun `only one call site sends the server token`() {
+        val authenticated = sources().flatMap { file ->
+            linesOf(file).mapIndexedNotNull { index, line ->
+                "${file.name}:${index + 1}  ${line.trim()}".takeIf { line.contains("authenticated = true") }
+            }
+        }
+
+        assertEquals(
+            "Each of these fetches with Peter's server credentials. A feed chooses an episode's " +
+                "mediaUrl, so any call that might carry one is a way to spend his token:\n" +
+                authenticated.joinToString("\n"),
+            1,
+            authenticated.size,
+        )
+    }
 }
diff --git a/app/src/test/java/org/terst/listen/download/EpisodeDownloaderTest.kt b/app/src/test/java/org/terst/listen/download/EpisodeDownloaderTest.kt
index 4f697e7..03cbd56 100644
--- a/app/src/test/java/org/terst/listen/download/EpisodeDownloaderTest.kt
+++ b/app/src/test/java/org/terst/listen/download/EpisodeDownloaderTest.kt
@@ -123,6 +123,58 @@ class EpisodeDownloaderTest {
         assertEquals("Bearer s3cret", server.takeRequest().getHeader("Authorization"))
     }
 
+    /*
+    A feed cannot talk the app into spending its token.
+
+    The origin check alone was not enough, and the hole is that `episode.mediaUrl` comes from the
+    feed. A publisher who set an enclosure URL on the skip server's own origin would match the
+    origin check exactly, and the app would issue an authenticated GET to a path of the
+    publisher's choosing -- storing whatever came back as that episode's audio.
+
+    So the bearer needs provenance as well as origin: it goes only on the URL the *server* handed
+    back from resolve, never on one that arrived in a feed. The origin check stays as the second
+    gate, in case the server's own answer is ever wrong.
+    */
+    /**
+     * The second gate, doing its job.
+     *
+     * Provenance says the server chose this URL, so the request is authenticated in principle --
+     * but if the URL is not actually on the server's origin, the origin check still refuses the
+     * token. That covers the server being misconfigured or its answer being tampered with: the
+     * download goes ahead without credentials rather than sending them somewhere else.
+     */
+    @Test
+    fun `a server url that is not on the server's origin is fetched without the token`() = runTest {
+        server.enqueue(MockResponse().setBody("elsewhere-bytes"))
+        val ready = ServerEpisode(
+            ready = true, audioUrl = server.url("/audio/abc").toString(),
+            durationMs = 1, introMs = null, outroMs = null, detected = true,
+        )
+        val ep = episode(url = "https://publisher.example/original.mp3").also { dao.rows += it }
+
+        // bearer = null: provenance says yes, the origin check says no.
+        withServer(FakeServerAudio(ready, bearer = null)).download(ep, SubscriptionKind.PODCAST)
+
+        assertNull(server.takeRequest().getHeader("Authorization"))
+    }
+
+    @Test
+    fun `an enclosure pointing at the server's own origin still gets no token`() = runTest {
+        val onServersOrigin = server.url("/audio/someone-elses").toString()
+        val ep = episode(url = onServersOrigin).also { dao.rows += it }
+        server.enqueue(MockResponse().setBody("publisher-bytes"))
+
+        // Configured, and it would hand over a token for any URL -- but this is the publisher
+        // path, because the episode's own mediaUrl is what is being fetched.
+        withServer(FakeServerAudio(null, configured = false, bearer = "s3cret"))
+            .download(ep, SubscriptionKind.PODCAST)
+
+        assertNull(
+            "a feed must not be able to spend the server token on a URL it chose",
+            server.takeRequest().getHeader("Authorization"),
+        )
+    }
+
     @Test
     fun `a publisher's audio is fetched with no token at all`() = runTest {
         server.enqueue(MockResponse().setBody("publisher-bytes"))
@@ -138,6 +190,85 @@ class EpisodeDownloaderTest {
         )
     }
 
+    /**
+     * A 200 with no body is a failure, not an empty episode.
+     *
+     * Pre-existing and untested, found while checking what this file's uncovered lines actually
+     * were. Writing a zero-byte file and calling it downloaded would give a silent, unplayable
+     * episode -- the same class of thing as the truncated-download guard beside it.
+     */
+    /*
+    The downloaded file is named for what it holds.
+
+    Untested until now, found while working out which lines in this file were uncovered. The
+    extension is not cosmetic: Media3 picks an extractor partly from it, and a .audio file whose
+    bytes are an mp4 is a plausible way to get an episode that will not play.
+    */
+    @Test
+    fun `the file extension follows the media type`() = runTest {
+        for ((type, extension) in listOf(
+            "audio/mpeg" to ".mp3",
+            "audio/mp4" to ".m4a",
+            "audio/x-m4a" to ".m4a",
+            "audio/aac" to ".m4a",
+            "audio/opus" to ".opus",
+            "audio/ogg" to ".ogg",
+            "audio/flac" to ".flac",
+            "audio/wav" to ".wav",
+            "audio/x-wav" to ".wav",
+            "video/mp4" to ".mp4",
+        )) {
+            dao.rows.clear()
+            server.enqueue(MockResponse().setBody("bytes"))
+            val ep = episode(url = server.url("/a").toString()).copy(id = "e-$extension", mediaType = type)
+                .also { dao.rows += it }
+
+            val file = downloader.download(ep, SubscriptionKind.PODCAST).getOrThrow()
+
+            assertTrue("$type should give $extension, got ${file.name}", file.name.endsWith(extension))
+        }
+    }
+
+    /** A type nobody listed still has to produce a file, rather than one with no name. */
+    @Test
+    fun `an unknown media type still gets an extension`() = runTest {
+        server.enqueue(MockResponse().setBody("bytes"))
+        val ep = episode(url = server.url("/a").toString()).copy(mediaType = "application/octet-stream")
+            .also { dao.rows += it }
+
+        assertTrue(downloader.download(ep, SubscriptionKind.PODCAST).getOrThrow().name.endsWith(".audio"))
+    }
+
+    /** Feeds write "audio/mpeg; charset=utf-8" and "AUDIO/MPEG"; both are mp3. */
+    @Test
+    fun `a media type with parameters or capitals is still recognised`() = runTest {
+        for (type in listOf("audio/mpeg; charset=utf-8", "AUDIO/MPEG", " audio/mpeg ")) {
+            dao.rows.clear()
+            server.enqueue(MockResponse().setBody("bytes"))
+            val ep = episode(url = server.url("/a").toString()).copy(id = "e${type.hashCode()}", mediaType = type)
+                .also { dao.rows += it }
+
+            val file = downloader.download(ep, SubscriptionKind.PODCAST).getOrThrow()
+
+            assertTrue("$type should give .mp3, got ${file.name}", file.name.endsWith(".mp3"))
+        }
+    }
+
+    @Test
+    fun `a response with no body fails and leaves nothing behind`() = runTest {
+        server.enqueue(MockResponse().setResponseCode(204))
+        val ep = episode(url = server.url("/empty.mp3").toString()).also { dao.rows += it }
+
+        val result = downloader.download(ep, SubscriptionKind.PODCAST)
+
+        assertTrue("a bodyless response must not count as a download: $result", result.isFailure)
+        assertEquals("state", DownloadState.FAILED, dao.rows.single().downloadState)
+        assertEquals(
+            "files left: ${library.listFiles()?.map { "${it.name}=${it.length()}" }}",
+            0, library.listFiles()?.size ?: 0,
+        )
+    }
+
     @Test
     fun `an enclosure that is not a url fails without throwing`() = runTest {
         val ep = episode(url = "not a url at all").also { dao.rows += it }