From ba3ca37897c12d2d285f48e8a429d47b40773df5 Mon Sep 17 00:00:00 2001 From: Peter Stone Date: Fri, 6 Nov 2026 09:06:36 +0000 Subject: 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 Claude-Session: https://claude.ai/code/session_018AwnALZL6bD4VBtATDVv3B --- .coverage-baseline | 2 +- .../org/terst/listen/download/EpisodeDownloader.kt | 15 ++- .../java/org/terst/listen/server/ServerBearer.kt | 38 +++++++ .../java/org/terst/listen/server/ServerSkips.kt | 10 ++ .../java/org/terst/listen/server/SkipServer.kt | 3 + .../terst/listen/download/EpisodeDownloaderTest.kt | 40 +++++++ .../listen/download/WaitForServerWiringTest.kt | 22 ++++ .../org/terst/listen/server/ServerBearerTest.kt | 118 +++++++++++++++++++++ 8 files changed, 246 insertions(+), 2 deletions(-) create mode 100644 app/src/main/java/org/terst/listen/server/ServerBearer.kt create mode 100644 app/src/test/java/org/terst/listen/server/ServerBearerTest.kt diff --git a/.coverage-baseline b/.coverage-baseline index eb0f3cd..e11965c 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -1 +1 @@ -94.06 +94.09 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 @@ -47,6 +47,16 @@ interface ServerAudio { /** Records the server's offsets, once its audio is actually the file on disk. */ 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. * @@ -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? = 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")) + } +} -- cgit v1.2.3