diff options
15 files changed, 299 insertions, 146 deletions
diff --git a/.coverage-baseline b/.coverage-baseline index f7407d8..eb0f3cd 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -1 +1 @@ -94.12 +94.06 diff --git a/app/src/main/java/org/terst/listen/data/MediaUrl.kt b/app/src/main/java/org/terst/listen/data/MediaUrl.kt deleted file mode 100644 index 2269888..0000000 --- a/app/src/main/java/org/terst/listen/data/MediaUrl.kt +++ /dev/null @@ -1,23 +0,0 @@ -package org.terst.listen.data - -/** - * The URL a remote episode is actually opened with. - * - * Android refuses cleartext HTTP by default from API 28 onward and this app declares no - * exemption, so an `http://` enclosure fails with ERROR_CODE_IO_CLEARTEXT_NOT_PERMITTED as soon - * as it is streamed rather than downloaded. Older podcast episodes are still published that way. - * - * Upgrading the scheme is the right fix rather than permitting cleartext. Podcast CDNs serve the - * same path over TLS, and an exemption would weaken every other request the app makes for the - * sake of a handful of old enclosures. A host that genuinely has no TLS ends up no worse off, - * since cleartext was refused already. - */ -object MediaUrl { - - private const val CLEARTEXT = "http://" - private const val SECURE = "https://" - - /** The same URL over TLS, if it was a cleartext one. Anything else is returned unchanged. */ - fun secured(url: String): String = - if (url.startsWith(CLEARTEXT, ignoreCase = true)) SECURE + url.substring(CLEARTEXT.length) else url -} 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 9aca940..cd990cf 100644 --- a/app/src/main/java/org/terst/listen/download/EpisodeDownloader.kt +++ b/app/src/main/java/org/terst/listen/download/EpisodeDownloader.kt @@ -245,6 +245,13 @@ class EpisodeDownloader( } catch (e: IOException) { partial.delete() Result.failure(e) + } catch (e: IllegalArgumentException) { + // A feed can say anything, and OkHttp throws on a URL it cannot parse rather + // than returning an error. Every play goes through a fetch now, so letting this + // escape would mean one junk enclosure stopped any episode starting -- not just + // the broken one. The row ends FAILED, which is visible in the list. + partial.delete() + Result.failure(e) } } diff --git a/app/src/main/java/org/terst/listen/feed/FeedFailureText.kt b/app/src/main/java/org/terst/listen/feed/FeedFailureText.kt index becbffe..78acfee 100644 --- a/app/src/main/java/org/terst/listen/feed/FeedFailureText.kt +++ b/app/src/main/java/org/terst/listen/feed/FeedFailureText.kt @@ -1,5 +1,7 @@ package org.terst.listen.feed +import okhttp3.HttpUrl.Companion.toHttpUrlOrNull + /** * Why a feed could not be refreshed, in words the listener can act on. * @@ -41,4 +43,15 @@ object FeedFailureText { /** It parsed, but as something with no show in it -- a parked domain, a login wall. */ const val NOT_A_FEED = "That address is not a podcast feed." + + /** + * A feed URL cut down to what is safe to write in a log: its scheme and host. + * + * Not cosmetic. A private feed carries its token in the query, some carry credentials in the + * userinfo, and logcat is readable by anything with the permission. The host is the only part + * that makes a log line useful anyway, and an address that will not parse is named as such + * rather than printed -- there is no safe way to redact something with no structure. + */ + fun redact(url: String): String = + url.toHttpUrlOrNull()?.let { "${it.scheme}://${it.host}" } ?: "an unparsable address" } diff --git a/app/src/main/java/org/terst/listen/feed/FeedFetcher.kt b/app/src/main/java/org/terst/listen/feed/FeedFetcher.kt index 5790c63..0f42b37 100644 --- a/app/src/main/java/org/terst/listen/feed/FeedFetcher.kt +++ b/app/src/main/java/org/terst/listen/feed/FeedFetcher.kt @@ -46,10 +46,14 @@ class FeedFetcher(private val client: OkHttpClient) { } } catch (e: IOException) { // The cause is for the log, not the library screen; see FeedFailureText. - Log.w(TAG, "could not reach $url", e) + Log.w(TAG, "could not reach ${FeedFailureText.redact(url)}", e) FeedFetchResult.Failed(FeedFailureText.UNREACHABLE) - } catch (e: IllegalArgumentException) { - Log.w(TAG, "not a usable feed address: $url", e) + } catch (@Suppress("SwallowedException") e: IllegalArgumentException) { + // Swallowed on purpose, exception and all. The address did not parse, so it + // cannot be redacted to a safe host; and this exception's message is the + // address, so logging the cause would put back exactly what redacting removes. + // A private feed's URL carries a token, and logcat is not private. + Log.w(TAG, "not a usable feed address") FeedFetchResult.Failed(FeedFailureText.NOT_AN_ADDRESS) } } @@ -77,6 +81,9 @@ class FeedFetcher(private val client: OkHttpClient) { private companion object { const val NOT_MODIFIED = 304 + + const val TAG = "ListenFeed" + /** * Sent on every outbound request. * @@ -86,7 +93,6 @@ class FeedFetcher(private val client: OkHttpClient) { * proof-of-origin token or a borrowed login, which Listen still doesn't do. Listen/1.0 stays on * the end so the traffic is still attributable in a server log. */ - const val TAG = "ListenFeed" const val USER_AGENT = "Mozilla/5.0 (Linux; Android 14) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/120.0.0.0 Mobile Safari/537.36 Listen/1.0" } diff --git a/app/src/main/java/org/terst/listen/playback/ListenSessionCallback.kt b/app/src/main/java/org/terst/listen/playback/ListenSessionCallback.kt index 2d44a28..c5ffe59 100644 --- a/app/src/main/java/org/terst/listen/playback/ListenSessionCallback.kt +++ b/app/src/main/java/org/terst/listen/playback/ListenSessionCallback.kt @@ -3,6 +3,7 @@ package org.terst.listen.playback import android.content.Context import android.os.Bundle import androidx.annotation.OptIn +import android.util.Log import androidx.media3.common.MediaItem import androidx.media3.common.Player import androidx.media3.common.util.UnstableApi @@ -173,12 +174,21 @@ class ListenSessionCallback( ): ListenableFuture<MutableList<MediaItem>> = scope.future { mediaItems.mapNotNull { item -> val episodeId = LibraryTree.episodeId(item.mediaId) ?: return@mapNotNull item - val episode = graph.episodes.getById(episodeId) ?: return@mapNotNull null - val show = graph.subscriptions.getById(episode.subscriptionId) - episode.toMediaItem( - showTitle = show?.title, - author = show?.author, - showImageUrl = show?.imageUrl, + // Through the advancer, which fetches the audio if there is none. This used to read + // the row and build an item straight off it, so an undownloaded episode was handed + // to the player as its publisher's URL and streamed -- in the car, where the + // resulting misplaced skips are least likely to be noticed and most annoying. + val ready = graph.advancer.startFetchingIfNeeded(episodeId) as? StartOutcome.Ready + if (ready == null) { + // Nothing playable, and deliberately not the publisher's copy. Logged because + // there is no banner in a car. + Log.w("ListenPlayback", "cannot start $episodeId from outside the app") + return@mapNotNull null + } + ready.next.episode.toMediaItem( + showTitle = ready.next.show?.title, + author = ready.next.show?.author, + showImageUrl = ready.next.show?.imageUrl, ) }.toMutableList() } @@ -210,7 +220,7 @@ class ListenSessionCallback( val index = BrowsedStart.index(startIndex, mediaItems.size) val resumeMs = mediaItems.getOrNull(index) ?.let { LibraryTree.episodeId(it.mediaId) } - ?.let { graph.advancer.start(it) } + ?.let { graph.advancer.startFetchingIfNeeded(it) } ?.let { (it as? StartOutcome.Ready)?.next?.startPositionMs } ?: 0L diff --git a/app/src/main/java/org/terst/listen/playback/PlaybackAdvancer.kt b/app/src/main/java/org/terst/listen/playback/PlaybackAdvancer.kt index f40adef..9189d78 100644 --- a/app/src/main/java/org/terst/listen/playback/PlaybackAdvancer.kt +++ b/app/src/main/java/org/terst/listen/playback/PlaybackAdvancer.kt @@ -2,6 +2,7 @@ package org.terst.listen.playback import org.terst.listen.data.BreakVerdictEntity import org.terst.listen.data.EpisodeDao +import kotlinx.coroutines.CancellationException import org.terst.listen.data.EpisodeEntity import org.terst.listen.data.EpisodeMidrollEntity import org.terst.listen.data.QueueRepository @@ -168,11 +169,45 @@ class PlaybackAdvancer( * FAILED download state is visible in the list, and looping on a video YouTube will not * serve gets nowhere. */ - suspend fun fetchAndStart(episode: EpisodeEntity, kind: SubscriptionKind): StartOutcome = - sources.fetchAudio(episode, kind).fold( + @Suppress("TooGenericExceptionCaught") + suspend fun fetchAndStart(episode: EpisodeEntity, kind: SubscriptionKind): StartOutcome { + // Deliberately broad, and deliberately not around the cancellation. A fetch reaches + // arbitrary bytes at a URL a publisher chose; folding its Result caught everything it + // *returned* and nothing it threw, and since every play goes through a fetch now, one + // malformed enclosure would stop episodes starting rather than failing its own. + // CancellationException is rethrown, or a cancelled playback start would report as a + // failed one -- the same care WaitingDownloads takes. + val fetched = try { + sources.fetchAudio(episode, kind) + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { + Result.failure(e) + } + return fetched.fold( onSuccess = { start(episode.id) }, onFailure = { StartOutcome.FetchFailed(it.message ?: it.javaClass.simpleName) }, ) + } + + /** + * Begins [episodeId], fetching its audio first if there is none, in one call. + * + * For callers with nobody to tell. The in-app paths take [start], show a label while the + * fetch runs and then resolve again; Android Auto, a Bluetooth button and system resumption + * all arrive at the session callback with no screen to put a label on, and before this they + * skipped the question entirely -- a browse id became a media item carrying the publisher's + * URL, and the car streamed it. + * + * A failed fetch stays failed. Falling back to the publisher is the thing being removed: + * its copy carries different advertising, so the breaks measured on the server's copy land + * in the wrong place, and silence that can be retried beats skipping into the programme. + */ + suspend fun startFetchingIfNeeded(episodeId: String): StartOutcome = + when (val outcome = start(episodeId)) { + is StartOutcome.NeedsFetch -> fetchAndStart(outcome.episode, outcome.kind) + else -> outcome + } /** What it would take to begin [episodeId]. */ suspend fun start(episodeId: String): StartOutcome { @@ -191,9 +226,12 @@ class PlaybackAdvancer( // for. And nothing in playback checks `audioFromServer`, so the breaks the server // measured on its own copy were being applied to a copy with different advertising in // different places -- skips landing in the wrong spot, with no way to tell why. + // No file: fetch it, or -- with no show left to say how to fetch it -- give up. Giving + // up is the honest answer and it is what lets toMediaItem stop carrying a remote + // fallback at all. An orphaned episode that still has its file plays from it. val kind = show?.kind - if (episode.localPath == null && kind != null) { - return StartOutcome.NeedsFetch(episode, kind) + if (episode.localPath == null) { + return kind?.let { StartOutcome.NeedsFetch(episode, it) } ?: StartOutcome.Gone } return StartOutcome.Ready( diff --git a/app/src/main/java/org/terst/listen/playback/PlaybackService.kt b/app/src/main/java/org/terst/listen/playback/PlaybackService.kt index 7af4e0a..f7e3235 100644 --- a/app/src/main/java/org/terst/listen/playback/PlaybackService.kt +++ b/app/src/main/java/org/terst/listen/playback/PlaybackService.kt @@ -20,7 +20,6 @@ import kotlinx.coroutines.launch import org.terst.listen.data.AppDatabase import org.terst.listen.data.Artwork import org.terst.listen.data.EpisodeEntity -import org.terst.listen.data.MediaUrl import org.terst.listen.data.Graph /** @@ -368,10 +367,13 @@ fun EpisodeEntity.toMediaItem( MediaItem.Builder() // The episode id, so whatever comes back from the player can be tied to a row. .setMediaId(id) - // A downloaded file is a path and is used as it stands; a remote enclosure is upgraded - // to TLS, because Android refuses cleartext and older episodes are still published over - // http -- which fails as ERROR_CODE_IO_CLEARTEXT_NOT_PERMITTED the moment it is streamed. - .setUri(localPath ?: MediaUrl.secured(mediaUrl)) + // The app's own copy, always. There used to be a fallback to the enclosure URL when + // there was no file, and it was reachable from every play -- so pressing play on an + // undownloaded episode streamed it from the publisher, whose copy carries different + // advertising from the one the server measured the skips on. PlaybackAdvancer now + // fetches first and reports Gone when it cannot, so there is nothing left to fall back + // for. An empty uri here would mean a caller bypassed it. + .setUri(localPath.orEmpty()) .setMimeType(mediaType) .setMediaMetadata( androidx.media3.common.MediaMetadata.Builder() diff --git a/app/src/test/java/org/terst/listen/OutboundRequestsTest.kt b/app/src/test/java/org/terst/listen/OutboundRequestsTest.kt index 85f179b..8b40d06 100644 --- a/app/src/test/java/org/terst/listen/OutboundRequestsTest.kt +++ b/app/src/test/java/org/terst/listen/OutboundRequestsTest.kt @@ -78,34 +78,30 @@ class OutboundRequestsTest { } /** - * The app plays its own copy of an episode, not the publisher's. + * The app plays its own copy of an episode, and there is no longer a way not to. * - * `toMediaItem` falls back to the enclosure URL when there is no file, and that fallback was - * how an unasked-for request reached a publisher's CDN -- and how the server's measured - * breaks came to be applied to a copy with different advertising in it. `PlaybackAdvancer` - * now fetches first, so the app's own play paths never reach the fallback. + * `toMediaItem` used to fall back to the enclosure URL when there was no file, and that + * fallback was reachable from every play: pressing play on an undownloaded episode streamed + * it from the publisher's CDN. Unasked-for, and wrong -- that copy carries different + * advertising from the one the server measured the breaks on. * - * It survives for one caller: Android Auto hands back a browse id, `onAddMediaItems` - * resolves it straight to a media item, and a car that silently plays nothing is worse than - * a car that streams. Fixing that properly means fetching inside the session callback. - * - * Until then this pins it to one line, so it cannot spread back into the paths it was just - * taken out of. + * `PlaybackAdvancer` fetches first and reports Gone when it cannot, Android Auto included, + * so the fallback had no callers left and was deleted. This is the ratchet: zero, not one. */ @Test - fun `the publisher stream fallback exists in exactly one place`() { - val fallbacks = sources().flatMap { file -> + fun `nothing builds a media item out of a remote url`() { + val streaming = sources().flatMap { file -> linesOf(file).mapIndexedNotNull { index, line -> - "${file.name}:${index + 1}".takeIf { line.contains("MediaUrl.secured(") } + "${file.name}:${index + 1} ${line.trim()}".takeIf { + line.contains("setUri(") && !line.contains("localPath") + } } } - assertEquals( - "Streaming an episode from its publisher plays different audio from the one the " + - "skips were measured on. If this grew, something is bypassing PlaybackAdvancer:\n" + - fallbacks.joinToString("\n"), - 1, - fallbacks.size, + assertTrue( + "An episode played from anywhere but the app's own copy has the wrong advertising " + + "in it, so the measured skips land in the wrong place:\n" + streaming.joinToString("\n"), + streaming.isEmpty(), ) } } diff --git a/app/src/test/java/org/terst/listen/data/MediaUrlTest.kt b/app/src/test/java/org/terst/listen/data/MediaUrlTest.kt deleted file mode 100644 index 8b0be16..0000000 --- a/app/src/test/java/org/terst/listen/data/MediaUrlTest.kt +++ /dev/null @@ -1,76 +0,0 @@ -package org.terst.listen.data - -import org.junit.Assert.assertEquals -import org.junit.Test - -/** - * Streaming an episode whose enclosure is cleartext. - * - * Android has blocked cleartext HTTP by default since API 28, and the app declares no exemption, - * so an `http://` enclosure fails with ERROR_CODE_IO_CLEARTEXT_NOT_PERMITTED the moment it is - * played without being downloaded first. Dan Carlin's older episodes are published that way -- - * `http://dts.podtrac.com/...` -- while the recent ones are not, which is why this went unnoticed. - * - * Upgrading the scheme is the fix rather than permitting cleartext: podcast CDNs serve the same - * path over TLS (that exact URL answers 200 and a range request returns 206 audio/mpeg), and an - * exemption would weaken every other request the app makes. A host that genuinely has no TLS is - * no worse off, because cleartext was already refused. - */ -class MediaUrlTest { - - @Test - fun `upgrades a cleartext url so playback is allowed to open it`() { - assertEquals( - "https://dts.podtrac.com/redirect.mp3/traffic.libsyn.com/dancarlinhh/dchha62.mp3", - MediaUrl.secured("http://dts.podtrac.com/redirect.mp3/traffic.libsyn.com/dancarlinhh/dchha62.mp3"), - ) - } - - @Test - fun `leaves a url that is already secure alone`() { - val url = "https://cdn.example/episode.mp3" - assertEquals(url, MediaUrl.secured(url)) - } - - /** Feeds are hand-written often enough that the scheme's case cannot be relied on. */ - @Test - fun `upgrades regardless of how the scheme is capitalised`() { - assertEquals("https://cdn.example/a.mp3", MediaUrl.secured("HTTP://cdn.example/a.mp3")) - assertEquals("https://cdn.example/b.mp3", MediaUrl.secured("Http://cdn.example/b.mp3")) - } - - /** - * A downloaded episode is played from a path, not a URL, and a local file has no scheme to - * upgrade. Rewriting one would point the player at nothing. - */ - @Test - fun `leaves a local file path untouched`() { - val path = "/data/user/0/org.terst.listen/files/library/abc.mp3" - assertEquals(path, MediaUrl.secured(path)) - assertEquals("file:///tmp/a.mp3", MediaUrl.secured("file:///tmp/a.mp3")) - assertEquals("content://media/external/audio/1", MediaUrl.secured("content://media/external/audio/1")) - } - - /** Only the scheme changes: a query string carrying a signature must survive intact. */ - @Test - fun `changes nothing but the scheme`() { - assertEquals( - "https://cdn.example/a.mp3?Signature=ab%2Fcd&Expires=1791465291", - MediaUrl.secured("http://cdn.example/a.mp3?Signature=ab%2Fcd&Expires=1791465291"), - ) - } - - /** A host that happens to contain the scheme as text must not be mangled. */ - @Test - fun `only rewrites the scheme at the front of the url`() { - assertEquals( - "https://cdn.example/redirect?to=http://elsewhere/a.mp3", - MediaUrl.secured("http://cdn.example/redirect?to=http://elsewhere/a.mp3"), - ) - } - - @Test - fun `an empty url is left as it is`() { - assertEquals("", MediaUrl.secured("")) - } -} 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 f615eec..cc33a26 100644 --- a/app/src/test/java/org/terst/listen/download/EpisodeDownloaderTest.kt +++ b/app/src/test/java/org/terst/listen/download/EpisodeDownloaderTest.kt @@ -93,6 +93,32 @@ class EpisodeDownloaderTest { assertTrue(library.walkTopDown().none { it.name.endsWith(".part") }) } + /* + A feed can say anything, including something that is not a URL. + + OkHttp's Request.Builder throws IllegalArgumentException rather than returning an error, and + only IOException was caught -- so the exception escaped the Result the caller folds. That was + survivable while this ran from a worker. It is not now: every play goes through a fetch since + PlaybackAdvancer stopped streaming, so one junk enclosure in one feed could take out starting + any episode. + */ + @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 } + + val result = downloader.download(ep, SubscriptionKind.PODCAST) + + assertTrue("must be a failed Result, not a thrown exception", result.isFailure) + assertEquals(DownloadState.FAILED, dao.rows.single().downloadState) + } + + @Test + fun `an enclosure with a scheme http cannot fetch fails without throwing`() = runTest { + val ep = episode(url = "ftp://example.invalid/a.mp3").also { dao.rows += it } + + assertTrue(downloader.download(ep, SubscriptionKind.PODCAST).isFailure) + } + @Test fun `an unreachable host fails without throwing`() = runTest { val url = server.url("/a.mp3").toString() diff --git a/app/src/test/java/org/terst/listen/playback/PlaybackAdvancerTest.kt b/app/src/test/java/org/terst/listen/playback/PlaybackAdvancerTest.kt index 04b31ff..28fbc42 100644 --- a/app/src/test/java/org/terst/listen/playback/PlaybackAdvancerTest.kt +++ b/app/src/test/java/org/terst/listen/playback/PlaybackAdvancerTest.kt @@ -329,6 +329,109 @@ class PlaybackAdvancerTest { assertEquals(SubscriptionKind.PODCAST, (advancer.start("a") as? StartOutcome.NeedsFetch)?.kind) } + /* + Starting from outside the app, where there is nobody to show a "fetching" label to. + + Android Auto, a Bluetooth button and system resumption all reach the session callback, which + resolves a browse id straight to a media item. It never consulted the advancer about whether + the audio existed, so an undownloaded episode was handed to the player as its publisher's URL + and streamed -- in the car, which is where most of this library is actually listened to, and + the one place the wrong-copy skips would be least obvious. + + The in-app paths show a label and fetch in two steps. A car cannot, so this composes the two. + */ + @Test + fun `starting from outside the app fetches the audio first`() = runTest { + show() + episode("a") + val fetching = PlaybackAdvancer( + episodes, subscriptions, queue, + sources = FakeSources(onFetch = { target -> + episodes.updateDownload(target.id, DownloadState.DOWNLOADED, "/audio/a.mp3", now) + Result.success(Unit) + }), + ) { now } + + val outcome = fetching.startFetchingIfNeeded("a") + + assertEquals("/audio/a.mp3", (outcome as? StartOutcome.Ready)?.next?.episode?.localPath) + } + + /** Already downloaded: no fetch, because the common case must not pay for the rare one. */ + @Test + fun `starting from outside the app does not refetch what is already there`() = runTest { + show() + episode("a", localPath = "/audio/a.mp3") + var fetches = 0 + val fetching = PlaybackAdvancer( + episodes, subscriptions, queue, + sources = FakeSources(onFetch = { fetches++; Result.success(Unit) }), + ) { now } + + assertTrue(fetching.startFetchingIfNeeded("a") is StartOutcome.Ready) + assertEquals(0, fetches) + } + + /** + * A fetch that fails reports it rather than falling back to the publisher. + * + * Playing the publisher's copy is what this whole change is removing: it carries different + * advertising, so the breaks measured on the server's copy land in the wrong place. Silence + * the listener can retry beats skips that quietly cut the programme. + */ + @Test + fun `a fetch that fails from outside the app does not fall back to the publisher`() = runTest { + show() + episode("a") + val fetching = PlaybackAdvancer( + episodes, subscriptions, queue, + sources = FakeSources(onFetch = { Result.failure(IllegalStateException("waiting for the server")) }), + ) { now } + + assertTrue(fetching.startFetchingIfNeeded("a") is StartOutcome.FetchFailed) + } + + /** + * A fetch that throws is a failure, not a crash. + * + * fetchAndStart folds a Result, so anything the fetch *throws* went straight past it. Every + * play goes through here now, so one feed with a malformed enclosure would stop episodes + * starting at all rather than failing the one that is broken. + */ + /** + * An orphaned episode with no file has nothing to play and no way to get one. + * + * `start` reported Ready when the show was gone, because an unsubscribed show's episode + * still plays from a file it already has. With no file there is no kind to fetch with + * either, and the old answer was to stream the publisher. Nothing is the honest one, and it + * is what lets toMediaItem stop carrying a remote fallback at all. + */ + @Test + fun `an episode whose show is gone and whose file is gone is not playable`() = runTest { + episode("a", subscriptionId = "vanished") + + assertTrue(advancer.start("a") is StartOutcome.Gone) + } + + @Test + fun `an episode whose show is gone still plays from the file it has`() = runTest { + episode("a", subscriptionId = "vanished", localPath = "/audio/a.mp3") + + assertTrue(advancer.start("a") is StartOutcome.Ready) + } + + @Test + fun `a fetch that throws is reported rather than escaping`() = runTest { + show() + episode("a") + val throwing = PlaybackAdvancer( + episodes, subscriptions, queue, + sources = FakeSources(onFetch = { throw IllegalArgumentException("not a url") }), + ) { now } + + assertTrue(throwing.startFetchingIfNeeded("a") is StartOutcome.FetchFailed) + } + @Test fun `fetching then starting plays from the file yt-dlp wrote`() = runTest { show(kind = SubscriptionKind.YOUTUBE_CHANNEL) @@ -455,7 +558,9 @@ class PlaybackAdvancerTest { /** An episode whose show was unsubscribed should still play rather than blocking the queue. */ @Test fun `an episode with no surviving show still starts`() = runTest { - episode("a", subscriptionId = "gone") + // With its file. Without one there is no show left to say how to fetch it -- see + // `an episode whose show is gone and whose file is gone is not playable`. + episode("a", subscriptionId = "gone", localPath = "/audio/a.mp3") val start = advancer.start("a").ready() diff --git a/server/internal/api/api.go b/server/internal/api/api.go index d716bb4..2d2e279 100644 --- a/server/internal/api/api.go +++ b/server/internal/api/api.go @@ -278,8 +278,10 @@ func (s *Server) transcriptNoteFor(e *store.Episode) *transcriptNote { Note: e.TranscriptNote, } if e.HasTranscript() { - note.URL = "/api/transcript?episode=" + url.QueryEscape(e.ID) + - "&token=" + url.QueryEscape(s.opts.Token) + // No token. The phone sends Authorization: Bearer on this request anyway and tokenOK + // takes either, so putting it in the URL bought nothing and cost a copy of the token in + // every line of Apache's access log -- which keeps fourteen rotations. + note.URL = "/api/transcript?episode=" + url.QueryEscape(e.ID) if held, err := (transcript.Store{Dir: s.opts.TranscriptDir}).Get(e.ID); err == nil { note.Ads = len(held.Ads) for _, ad := range held.Ads { diff --git a/server/internal/api/api_test.go b/server/internal/api/api_test.go index 8a29b33..b50454b 100644 --- a/server/internal/api/api_test.go +++ b/server/internal/api/api_test.go @@ -441,3 +441,47 @@ func TestAskingForAnEpisodeMarksItWanted(t *testing.T) { t.Error("asking for a mirrored episode left it unwanted, so a poll may still drop it") } } + +/* +The transcript URL carries no token. + +Apache logs the request line, so every /api/transcript fetch wrote the server's token into +listen.terst.org-access.log in plaintext, across fourteen rotations. The token was redundant +there in the first place: the phone sends Authorization: Bearer on this request anyway, through +OkHttpSkipTransport, and tokenOK accepts either. + +The audio URL still carries one. That one is not redundant -- the downloader fetches it with no +credentials of its own -- and removing it needs the app to send the header first, so it goes in +a later change. +*/ +func TestTheTranscriptURLDoesNotCarryTheToken(t *testing.T) { + s, st, _, audioDir := newServer(t) + seedMirrored(t, st, audioDir, "audio-bytes") + e := st.Match("https://f", "g1", "", "") + e.TranscriptSource, e.TranscriptCues = "asr", 12 + + rec := get(t, s, "/api/episode?feed=https://f&guid=g1") + + var got episodeResponse + if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil { + t.Fatal(err) + } + if got.Transcript == nil || got.Transcript.URL == "" { + t.Fatalf("no transcript url in %s", rec.Body.String()) + } + if strings.Contains(got.Transcript.URL, "token") { + t.Errorf("the token is in the transcript url, and so in every access log line: %s", got.Transcript.URL) + } +} + +// And the request still works, because the header is what authorises it. +func TestTheTranscriptEndpointTakesTheHeader(t *testing.T) { + s, st, _, audioDir := newServer(t) + seedMirrored(t, st, audioDir, "audio-bytes") + + rec := get(t, s, "/api/transcript?episode="+episodeID(st)) + + if rec.Code == http.StatusUnauthorized || rec.Code == http.StatusForbidden { + t.Errorf("a bearer header must authorise this; got %d", rec.Code) + } +} diff --git a/server/internal/api/transcript_test.go b/server/internal/api/transcript_test.go index e6da3ff..dff4d54 100644 --- a/server/internal/api/transcript_test.go +++ b/server/internal/api/transcript_test.go @@ -106,9 +106,12 @@ func TestTheResponseSaysWhereTheWordsAre(t *testing.T) { if got.Transcript == nil || !strings.Contains(got.Transcript.URL, e.ID) { t.Fatalf("got %+v, want a URL naming the episode", got.Transcript) } - // Usable as given, like the audio URL: the phone appends nothing. - if !strings.Contains(got.Transcript.URL, "token=") { - t.Errorf("url = %q, want it to carry the token", got.Transcript.URL) + // Not usable as given, deliberately. The phone authorises this with a bearer header, which + // it was sending anyway, and a token in the URL was a copy of the secret in every line of + // Apache's access log. The audio URL still carries one because the downloader has no + // credentials of its own; see TestTheTranscriptURLDoesNotCarryTheToken. + if strings.Contains(got.Transcript.URL, "token=") { + t.Errorf("url = %q, want no token in it", got.Transcript.URL) } } |