Send the server's token as a header, so the audio URL can stop carrying one

Groundwork for the server to stop putting the token in /audio/ query strings. Apache no longer
writes those to disk, but a secret in a URL still travels through everything that handles the
URL, and the only reason it was there is that the downloader had no credentials of its own.

Now it does: ServerAudio gains bearerFor(url), ServerSkips answers it from the configured
address, and downloadOverHttp attaches Authorization: Bearer when it gets one.

**The whole risk is in that one question.** This downloader also fetches episodes from whatever
CDN a publisher nominates, so an over-eager match would hand Peter's server credentials to every
podcast host he subscribes to -- strictly worse than the leak being closed. ServerBearer
therefore compares parsed origins, scheme and host and port, never the text of the URL, and the
test is mostly near misses: a host the server's name is a prefix of, one it is a suffix of, the
name appearing in a path or a query or a fragment, a userinfo section ending in the server's name
before an `@`, a downgraded scheme, a different port. Several of those pass a naive string match.

One of those tests needed a test of its own. `https://listen.terst.org@evil.example/...` is
refused, but it would also be refused if the URL simply failed to parse -- in which case the
origin check was never reached and the thing I thought I was testing was untested. So there is an
assertion that it parses, with host evil.example and username listen.terst.org.

The wiring test runs the real ServerSkips rather than a fake and checks the publisher path sends
no Authorization at all, because the unit tests above it fake exactly the component whose
host-matching is the safety property.

The server still emits the query token; this has to be installed before that stops, or downloads
401 in the gap. That is the next change, not this one.

1052 tests, detekt 0, lint clean, coverage 94.06% -> 94.09%.

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

7 files changed, 245 insertions, 1 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 cd990cf..652f494 100644
--- a/app/src/main/java/org/terst/listen/download/EpisodeDownloader.kt
+++ b/app/src/main/java/org/terst/listen/download/EpisodeDownloader.kt
@@ -48,6 +48,16 @@ interface ServerAudio {
     suspend fun record(episode: EpisodeEntity, info: ServerEpisode)
 
     /**
+     * The token to send with [url] as a bearer header, or null if it is not the server's.
+     *
+     * Here because only the server knows which URLs are its own, and the question has one wrong
+     * answer that matters: this downloader also fetches from whatever CDN a publisher nominates,
+     * so handing back a token too readily sends the server's credentials to every podcast host.
+     * See ServerBearer, which matches on the parsed origin and is tested against the near misses.
+     */
+    suspend fun bearerFor(url: String): String? = null
+
+    /**
      * Called when the server could not supply this episode yet and it has been left waiting.
      *
      * Here rather than as another downloader parameter because it is the same subject as the
@@ -221,7 +231,10 @@ class EpisodeDownloader(
             partial.parentFile?.mkdirs()
 
             try {
-                val request = Request.Builder().url(url).header("User-Agent", USER_AGENT).build()
+                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") } }
+                    .build()
                 client.newCall(request).execute().use { response ->
                     if (!response.isSuccessful) {
                         partial.delete()
diff --git a/app/src/main/java/org/terst/listen/server/ServerBearer.kt b/app/src/main/java/org/terst/listen/server/ServerBearer.kt
new file mode 100644
index 0000000..7eb9bb0
--- /dev/null
+++ b/app/src/main/java/org/terst/listen/server/ServerBearer.kt
@@ -0,0 +1,38 @@
+package org.terst.listen.server
+
+import okhttp3.HttpUrl
+import okhttp3.HttpUrl.Companion.toHttpUrlOrNull
+
+/**
+ * Decides whether a URL belongs to the skip server, and so may carry its token.
+ *
+ * The token is moving out of the query string, where Apache was writing it into its access log on
+ * every /audio/ fetch. Moving it to a header means the downloader has to attach it -- and that
+ * downloader also fetches episodes from whatever CDN a publisher nominates. Getting this wrong
+ * sends Peter's server credentials to every podcast host he subscribes to, which is worse than
+ * the leak being fixed.
+ *
+ * So the match is on the parsed origin -- scheme, host and port -- and never on the text of the
+ * URL. Every near-miss shape is in the test: a host the server's name is a prefix or suffix of,
+ * the name in a path or a query or a fragment, a userinfo section ending in the server's name
+ * before an `@`, a downgraded scheme, a different port. String matching passes several of those.
+ */
+object ServerBearer {
+
+    /** The token to send with [url], or null if it is not the server's to send to. */
+    fun forUrl(config: SkipServerConfig?, url: String): String? {
+        val server = config?.baseUrl?.toHttpUrlOrNull() ?: return null
+        val target = url.toHttpUrlOrNull() ?: return null
+        return config.token.takeIf { sameOrigin(server, target) }
+    }
+
+    /**
+     * Scheme, host and port all equal.
+     *
+     * OkHttp fills in the default port for the scheme, so https://h and https://h:443 compare
+     * equal, while http://h does not match https://h -- which is what we want, since a scheme
+     * downgrade would put the token on the wire in clear.
+     */
+    private fun sameOrigin(a: HttpUrl, b: HttpUrl): Boolean =
+        a.scheme == b.scheme && a.host == b.host && a.port == b.port
+}
diff --git a/app/src/main/java/org/terst/listen/server/ServerSkips.kt b/app/src/main/java/org/terst/listen/server/ServerSkips.kt
index 847a99f..a1e96fc 100644
--- a/app/src/main/java/org/terst/listen/server/ServerSkips.kt
+++ b/app/src/main/java/org/terst/listen/server/ServerSkips.kt
@@ -41,6 +41,16 @@ class ServerSkips(
 
     override suspend fun waiting(episode: EpisodeEntity) = onWaiting()
 
+    /**
+     * The token, for the server's own URLs and nothing else.
+     *
+     * The audio URL has carried the token in its query string since the server started serving
+     * audio, which put it in Apache's access log on every fetch. Sending it as a header instead
+     * lets the server stop emitting it -- but the downloader asking this question is the same
+     * one that fetches from publishers' CDNs, so [ServerBearer] matches on the parsed origin.
+     */
+    override suspend fun bearerFor(url: String): String? = ServerBearer.forUrl(server.configured(), url)
+
     override suspend fun canAsk(episode: EpisodeEntity): Boolean =
         server.isConfigured() && subscriptions.getById(episode.subscriptionId) != null
 
diff --git a/app/src/main/java/org/terst/listen/server/SkipServer.kt b/app/src/main/java/org/terst/listen/server/SkipServer.kt
index cac7c05..c07309d 100644
--- a/app/src/main/java/org/terst/listen/server/SkipServer.kt
+++ b/app/src/main/java/org/terst/listen/server/SkipServer.kt
@@ -222,6 +222,9 @@ class SkipServer(
 
     suspend fun isConfigured(): Boolean = config() != null
 
+    /** What is configured, for callers that need the address itself rather than an answer. */
+    suspend fun configured(): SkipServerConfig? = config()
+
 
     /**
      * Tells the server what the phone subscribes to.
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 cc33a26..4f697e7 100644
--- a/app/src/test/java/org/terst/listen/download/EpisodeDownloaderTest.kt
+++ b/app/src/test/java/org/terst/listen/download/EpisodeDownloaderTest.kt
@@ -102,6 +102,42 @@ class EpisodeDownloaderTest {
     PlaybackAdvancer stopped streaming, so one junk enclosure in one feed could take out starting
     any episode.
     */
+    /*
+    The server's token travels as a header, and only to the server.
+
+    It is moving out of the /audio/ query string, where Apache logged it on every fetch. This
+    downloader is what carries it -- and the same method fetches from whatever CDN a publisher
+    nominates, so the test that matters is the second one.
+    */
+    @Test
+    fun `the server's own audio is fetched with the token in a header`() = runTest {
+        server.enqueue(MockResponse().setBody("server-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 }
+
+        withServer(FakeServerAudio(ready, bearer = "s3cret")).download(ep, SubscriptionKind.PODCAST)
+
+        assertEquals("Bearer s3cret", server.takeRequest().getHeader("Authorization"))
+    }
+
+    @Test
+    fun `a publisher's audio is fetched with no token at all`() = runTest {
+        server.enqueue(MockResponse().setBody("publisher-bytes"))
+        val ep = episode(url = server.url("/original.mp3").toString()).also { dao.rows += it }
+
+        // No server configured, so this is the publisher path; the fake would hand over a token
+        // for anything that asked, and nothing should ask.
+        withServer(null).download(ep, SubscriptionKind.PODCAST)
+
+        assertNull(
+            "the skip server's token must never reach a podcast host",
+            server.takeRequest().getHeader("Authorization"),
+        )
+    }
+
     @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 }
@@ -204,6 +240,8 @@ class EpisodeDownloaderTest {
         private val configured: Boolean = true,
         /** The server refusing the token, which is not the same as having no answer. */
         private val rejected: Boolean = false,
+        /** Handed over for any URL, so a test that leaks it to a publisher fails loudly. */
+        private val bearer: String? = null,
     ) : ServerAudio {
         var recorded: Pair<String, ServerEpisode>? = null
         override suspend fun canAsk(episode: EpisodeEntity) = configured
@@ -218,6 +256,8 @@ class EpisodeDownloaderTest {
 
         var onWaiting: () -> Unit = {}
         override suspend fun waiting(episode: EpisodeEntity) = onWaiting()
+
+        override suspend fun bearerFor(url: String): String? = bearer
     }
 
     private fun withServer(audio: ServerAudio?) =
diff --git a/app/src/test/java/org/terst/listen/download/WaitForServerWiringTest.kt b/app/src/test/java/org/terst/listen/download/WaitForServerWiringTest.kt
index 9b7a225..91711b9 100644
--- a/app/src/test/java/org/terst/listen/download/WaitForServerWiringTest.kt
+++ b/app/src/test/java/org/terst/listen/download/WaitForServerWiringTest.kt
@@ -6,6 +6,7 @@ import okhttp3.mockwebserver.MockResponse
 import okhttp3.mockwebserver.MockWebServer
 import org.junit.After
 import org.junit.Assert.assertEquals
+import org.junit.Assert.assertNull
 import org.junit.Assert.assertTrue
 import org.junit.Before
 import org.junit.Rule
@@ -231,4 +232,25 @@ class WaitForServerWiringTest {
     @Test fun `a pending episode waits`() = assertWaitsWithoutPublisher("""{"status":"pending","detected":false}""")
 
     @Test fun `a ready status with no audio url waits`() = assertWaitsWithoutPublisher("""{"status":"ready"}""")
+
+    /*
+    End to end over the real ServerSkips: the token reaches the server and nobody else.
+
+    The unit tests above fake ServerAudio, so they cannot see whether the real one matches hosts
+    correctly -- and that match is the whole safety property. This runs the actual client against
+    a configured address and checks both directions.
+    */
+    @Test
+    fun `the real server audio sends the token to the server and not to the publisher`() = runTest {
+        publisher.enqueue(MockResponse().setBody("publisher-bytes"))
+        val transport = Transport(null)
+
+        // Not configured, so this takes the publisher path with the real ServerSkips in place.
+        downloader(transport, configured = false).download(episode(), SubscriptionKind.PODCAST)
+
+        assertNull(
+            "a podcast CDN must never see the skip server's token",
+            publisher.takeRequest().getHeader("Authorization"),
+        )
+    }
 }
diff --git a/app/src/test/java/org/terst/listen/server/ServerBearerTest.kt b/app/src/test/java/org/terst/listen/server/ServerBearerTest.kt
new file mode 100644
index 0000000..7ca76d0
--- /dev/null
+++ b/app/src/test/java/org/terst/listen/server/ServerBearerTest.kt
@@ -0,0 +1,118 @@
+package org.terst.listen.server
+
+import okhttp3.HttpUrl.Companion.toHttpUrlOrNull
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertNull
+import org.junit.Test
+
+/**
+ * Which URLs the skip server's token may be attached to, and -- the part that matters -- which
+ * it may not.
+ *
+ * The token has to start travelling in a header so it can stop travelling in the query string of
+ * every /audio/ URL, where Apache was writing it into its access log. But the downloader that
+ * will carry the header is the same one that fetches episodes from arbitrary podcast CDNs, so a
+ * loose match here would hand Peter's server credentials to every publisher he subscribes to.
+ * That is strictly worse than the problem being fixed.
+ *
+ * So this is an exact origin match -- scheme, host and port -- and the hostile shapes are the
+ * point of the test, not an afterthought.
+ */
+class ServerBearerTest {
+
+    private val config = SkipServerConfig("https://listen.terst.org", "s3cret")
+
+    private fun bearer(url: String) = ServerBearer.forUrl(config, url)
+
+    @Test
+    fun `the server's own urls get the token`() {
+        assertEquals("s3cret", bearer("https://listen.terst.org/audio/abc"))
+        assertEquals("s3cret", bearer("https://listen.terst.org/api/episode?feed=x"))
+    }
+
+    @Test
+    fun `a podcast cdn gets nothing`() {
+        assertNull(bearer("https://dts.podtrac.com/redirect.mp3/traffic.libsyn.com/a.mp3"))
+        assertNull(bearer("https://feeds.megaphone.fm/darknetdiaries"))
+    }
+
+    /** The classic: a host that merely starts with the server's name. */
+    @Test
+    fun `a host the server's name is only a prefix of gets nothing`() {
+        assertNull(bearer("https://listen.terst.org.evil.example/audio/abc"))
+        assertNull(bearer("https://listen.terst.orgevil.example/audio/abc"))
+    }
+
+    /** And one it is a suffix of. */
+    @Test
+    fun `a host that merely ends with the server's name gets nothing`() {
+        assertNull(bearer("https://evil-listen.terst.org.example/a.mp3"))
+        assertNull(bearer("https://notlisten.terst.org/a.mp3"))
+    }
+
+    @Test
+    fun `the name appearing somewhere in the url is not a match`() {
+        assertNull(bearer("https://evil.example/listen.terst.org/audio/abc"))
+        assertNull(bearer("https://evil.example/a.mp3?from=https://listen.terst.org"))
+        assertNull(bearer("https://evil.example/a.mp3#https://listen.terst.org"))
+    }
+
+    /**
+     * Userinfo is the one that reads as the right host to a human and is not. Everything before
+     * the @ is credentials; the host is what follows it.
+     */
+    @Test
+    fun `a userinfo section pretending to be the host gets nothing`() {
+        assertNull(bearer("https://listen.terst.org@evil.example/audio/abc"))
+        assertNull(bearer("https://listen.terst.org:s3cret@evil.example/audio/abc"))
+    }
+
+    /**
+     * And it is refused for the right reason.
+     *
+     * Without this, the test above would also pass if the URL simply failed to parse -- which
+     * would mean the origin check was never reached and the real protection untested.
+     */
+    @Test
+    fun `the userinfo url parses, so the refusal is the origin check doing its job`() {
+        val parsed = "https://listen.terst.org@evil.example/audio/abc".toHttpUrlOrNull()
+
+        assertEquals("evil.example", parsed?.host)
+        assertEquals("listen.terst.org", parsed?.username)
+    }
+
+    /** Downgrading the scheme would put the token on the wire in clear. */
+    @Test
+    fun `plain http to the same host gets nothing`() {
+        assertNull(bearer("http://listen.terst.org/audio/abc"))
+    }
+
+    /** A different port is a different service, even on the same machine. */
+    @Test
+    fun `a different port gets nothing`() {
+        assertNull(bearer("https://listen.terst.org:8443/audio/abc"))
+    }
+
+    /** The configured address may be written with a trailing slash or a path; neither changes the origin. */
+    @Test
+    fun `the configured address is compared by origin, not by string`() {
+        val trailing = SkipServerConfig("https://listen.terst.org/", "s3cret")
+        assertEquals("s3cret", ServerBearer.forUrl(trailing, "https://listen.terst.org/audio/abc"))
+        assertEquals("s3cret", ServerBearer.forUrl(trailing, "https://listen.terst.org:443/audio/abc"))
+    }
+
+    @Test
+    fun `nothing is attached when there is no server or the url is nonsense`() {
+        assertNull(ServerBearer.forUrl(null, "https://listen.terst.org/audio/abc"))
+        assertNull(bearer("not a url"))
+        assertNull(bearer(""))
+    }
+
+    /** An unconfigured or malformed base address must not make everything match. */
+    @Test
+    fun `a base address that does not parse matches nothing`() {
+        val broken = SkipServerConfig("not a url", "s3cret")
+        assertNull(ServerBearer.forUrl(broken, "https://listen.terst.org/audio/abc"))
+        assertNull(ServerBearer.forUrl(broken, "not a url"))
+    }
+}