From 71600021b40dc746ba14b3fda0ef863263d1e753 Mon Sep 17 00:00:00 2001 From: Timo <6156589+Shik3i@users.noreply.github.com> Date: Sat, 15 Aug 2026 17:03:07 +0200 Subject: [PATCH 1/2] Fix episode types, proxy credential relay and two player state leaks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second pass of the audit, over the areas the first pass never opened: the Android player module, the RSS parser, and the rest of the image/audio proxy. itunes:episodeType was parsed and thrown away parseRSS computed the value, defaulted it to "full", and then left the field out of the struct literal — so every episode ever ingested stored an empty episode_type. The Inbox's "hide specials" filter keys on trailer/bonus and had nothing to key on, degrading to guessing from the title: a properly tagged trailer with a neutral title was never filtered. Atom carries no equivalent tag, so an entry is "full" by the same rule the RSS branch already applied to untagged items. Existing rows heal on the next feed refresh, which already writes episode_type on update. The proxies relayed credentials embedded in a URL Both proxy endpoints take a URL straight out of a publisher's feed. The SSRF-safe dialer resolves and vets every address, on the initial request and on each redirect, but it never looks at the userinfo component — so `https://user:secret@host/` in an artwork or enclosure tag was presented upstream by the instance itself. Only that gap is closed here: repeating the dialer's address policy in the handler would also reject the loopback targets the transport is deliberately allowed to reach. A sleep timer outlived the episode it was set on "Stop at the end of this chapter" is a media position, and only the natural end of an episode cleared it. Skipping to the next track carried the target across and paused the new episode at a position that meant nothing there. The service and the UI state both reset it on an episode change now. The per-podcast outro setting was cached until the podcast changed, so a change made while that show was playing never took effect; it is re-read on the same transition. Verified on an emulator: playback runs, position (34.4 s) and the listening session (34.5 s wall clock, matching audio time at 1.0x) both persist, no crashes. Go and both Android unit-test tasks pass. Co-Authored-By: Claude Opus 5 --- .../koalacast/core/player/PlaybackService.kt | 11 +++ .../koalacast/core/player/PlayerConnection.kt | 10 +++ services/api/internal/rss/parser.go | 20 ++++-- services/api/internal/rss/parser_test.go | 67 +++++++++++++++++++ .../api/internal/server/handlers/proxy.go | 36 ++++++++-- .../internal/server/handlers/proxy_test.go | 37 ++++++++++ 6 files changed, 171 insertions(+), 10 deletions(-) diff --git a/apps/android/core/player/src/main/kotlin/net/koalastuff/koalacast/core/player/PlaybackService.kt b/apps/android/core/player/src/main/kotlin/net/koalastuff/koalacast/core/player/PlaybackService.kt index e0bc2ed6..9b3cd0c9 100644 --- a/apps/android/core/player/src/main/kotlin/net/koalastuff/koalacast/core/player/PlaybackService.kt +++ b/apps/android/core/player/src/main/kotlin/net/koalastuff/koalacast/core/player/PlaybackService.kt @@ -653,6 +653,17 @@ class PlaybackService : MediaLibraryService() { transitionOldPositionMs = null outroHandledEpisodeId = null automaticSeekTargetMs = null + // A "stop at 42:00" timer is about the episode it was set on. Only + // the natural end of an episode cleared it, so skipping to the next + // track carried the target across and paused the new episode at a + // position that meant nothing there. The per-podcast outro setting + // is re-read for the same reason: it was cached until the *podcast* + // changed, so a change made while a show was playing never took. + if (previousTrack != null && previousTrack.episodeId != nextTrack?.episodeId) { + sleepAtPositionMs = null + sleepAtEpisodeEnd = false + } + outroSettingsPodcastId = null if (player.isPlaying) { TrackMediaItem.toTrack(mediaItem)?.let { track -> recorder.start( diff --git a/apps/android/core/player/src/main/kotlin/net/koalastuff/koalacast/core/player/PlayerConnection.kt b/apps/android/core/player/src/main/kotlin/net/koalastuff/koalacast/core/player/PlayerConnection.kt index 3fcd0276..05944d93 100644 --- a/apps/android/core/player/src/main/kotlin/net/koalastuff/koalacast/core/player/PlayerConnection.kt +++ b/apps/android/core/player/src/main/kotlin/net/koalastuff/koalacast/core/player/PlayerConnection.kt @@ -453,6 +453,14 @@ class PlayerConnection @Inject constructor( .takeIf { it != C.TIME_UNSET && it > 0 } ?: track?.durationMs ?: 0L + // "Stop at the end of this chapter/episode" is about the episode it + // was set on. The service clears its own target on a track change; + // without the same reset here the control kept showing an armed + // timer against an episode that had already been left behind. + val previous = _state.value.track + val episodeChanged = + track != null && previous != null && track.episodeId != previous.episodeId + if (episodeChanged) sleepTargetPositionMs = null _state.update { it.copy( track = track, @@ -467,6 +475,8 @@ class PlayerConnection @Inject constructor( controller.playerError?.errorCodeName }, isOfflineSource = TrackMediaItem.isOffline(currentItem), + sleepAtChapterEnd = !episodeChanged && it.sleepAtChapterEnd, + sleepAtEpisodeEnd = !episodeChanged && it.sleepAtEpisodeEnd, ) } } diff --git a/services/api/internal/rss/parser.go b/services/api/internal/rss/parser.go index 19370290..808c17a5 100644 --- a/services/api/internal/rss/parser.go +++ b/services/api/internal/rss/parser.go @@ -324,10 +324,16 @@ func parseRSS(buf []byte) (*ParsedFeed, error) { ArtworkURL: strings.TrimSpace(item.ItunesImage.Href), EpisodeNumber: item.ItunesEpisode, SeasonNumber: item.ItunesSeason, - Explicit: epExplicit, - Link: strings.TrimSpace(item.Link), - ChaptersURL: strings.TrimSpace(item.PodcastChapters.URL), - Transcripts: transcripts, + // Computed above and then dropped on the floor: the field was never + // assigned, so every episode ever ingested stored an empty + // episode_type. The Inbox's "hide specials" filter keys on + // trailer/bonus and had nothing to key on, leaving it to guess from + // the title. + EpisodeType: epType, + Explicit: epExplicit, + Link: strings.TrimSpace(item.Link), + ChaptersURL: strings.TrimSpace(item.PodcastChapters.URL), + Transcripts: transcripts, } feed.Episodes = append(feed.Episodes, ep) @@ -438,7 +444,11 @@ func parseAtom(buf []byte) (*ParsedFeed, error) { EnclosureURL: strings.TrimSpace(enclosureURL), EnclosureType: strings.TrimSpace(enclosureType), EnclosureLength: enclosureLength, - Link: strings.TrimSpace(epLink), + // Atom carries no itunes:episodeType. "full" is what the RSS branch + // falls back to for an untagged item, and consumers compare against + // trailer/bonus, so an Atom entry is a full episode by the same rule. + EpisodeType: "full", + Link: strings.TrimSpace(epLink), } feed.Episodes = append(feed.Episodes, ep) diff --git a/services/api/internal/rss/parser_test.go b/services/api/internal/rss/parser_test.go index c6d71036..ed64286e 100644 --- a/services/api/internal/rss/parser_test.go +++ b/services/api/internal/rss/parser_test.go @@ -293,3 +293,70 @@ func TestParseFeedXML_Chapters(t *testing.T) { t.Errorf("expected no chapters URL on the plain episode, got %q", got) } } + +// itunes:episodeType is how a publisher marks a trailer or a bonus item, and it +// is what the Inbox's "hide specials" filter keys on. The parser computed the +// value and then left it out of the struct literal, so every episode ever +// ingested stored an empty type and the filter was reduced to guessing from the +// title. +func TestParseRSS_KeepsEpisodeType(t *testing.T) { + feed := ` + + + Show + + Season one is coming + ep-trailer + trailer + + + Behind the scenes + ep-bonus + Bonus + + + Episode one + ep-plain + + +` + + parsed, err := ParseFeedXML(strings.NewReader(feed)) + if err != nil { + t.Fatalf("parse: %v", err) + } + if len(parsed.Episodes) != 3 { + t.Fatalf("expected 3 episodes, got %d", len(parsed.Episodes)) + } + want := []string{"trailer", "Bonus", "full"} + for i, expected := range want { + if got := parsed.Episodes[i].EpisodeType; got != expected { + t.Errorf("episode %d: EpisodeType = %q, want %q", i, got, expected) + } + } +} + +// Atom has no equivalent tag, so an entry is a full episode rather than an +// untyped one — consumers compare against trailer/bonus. +func TestParseAtom_DefaultsEpisodeTypeToFull(t *testing.T) { + feed := ` + + Show + + entry-1 + Episode one + + +` + + parsed, err := ParseFeedXML(strings.NewReader(feed)) + if err != nil { + t.Fatalf("parse: %v", err) + } + if len(parsed.Episodes) != 1 { + t.Fatalf("expected 1 episode, got %d", len(parsed.Episodes)) + } + if got := parsed.Episodes[0].EpisodeType; got != "full" { + t.Errorf("EpisodeType = %q, want \"full\"", got) + } +} diff --git a/services/api/internal/server/handlers/proxy.go b/services/api/internal/server/handlers/proxy.go index fed3359a..30face75 100644 --- a/services/api/internal/server/handlers/proxy.go +++ b/services/api/internal/server/handlers/proxy.go @@ -16,6 +16,7 @@ import ( "io" "math/big" "net/http" + "net/url" "regexp" "strconv" "strings" @@ -130,17 +131,17 @@ func NewProxyHandler(audioProxyEnabled bool) *ProxyHandler { // 40 MP comfortably covers legitimate podcast artwork (typically <=3000x3000). const maxDecodedPixels = 40 * 1000 * 1000 -// Cold publisher/CDN connections routinely exceed one second. A 1.2 s deadline -// made the proxy return its temporary placeholder before otherwise healthy -// artwork arrived, forcing users to reload until a request happened to be fast. -// Singleflight still coalesces identical requests while this bounded deadline -// gives DNS, TLS and the first upstream response a realistic window. // acceptedImageFormats mirrors the decoders registered by this package's // imports (JPEG, PNG, GIF, WebP). Content negotiation is a promise about what // the client can read; advertising a format with no decoder turns every // negotiating CDN into a broken image. const acceptedImageFormats = "image/webp,image/jpeg,image/png,image/gif;q=0.8,*/*;q=0.5" +// Cold publisher/CDN connections routinely exceed one second. A 1.2 s deadline +// made the proxy return its temporary placeholder before otherwise healthy +// artwork arrived, forcing users to reload until a request happened to be fast. +// Singleflight still coalesces identical requests while this bounded deadline +// gives DNS, TLS and the first upstream response a realistic window. const imageProxyTimeout = 8 * time.Second const maxAudioDownloadBytes = int64(2 * 1024 * 1024 * 1024) const maxAudioStreamDuration = 4 * time.Hour @@ -164,6 +165,23 @@ func writeImageFallback(w http.ResponseWriter) { _, _ = w.Write(imageFallbackWebP) } +// proxyTargetAllowed rejects what the SSRF-safe transport cannot see. +// +// Address policy belongs to the dialer, which resolves and checks every IP at +// connect time and again on each redirect — repeating it here would also reject +// the loopback targets the transport is deliberately allowed to reach in tests. +// What the dialer never looks at is the userinfo component, and both proxy +// endpoints take a fully attacker-controlled URL: without this the server +// presents whatever `https://user:pass@host/` a feed put in an artwork or +// enclosure tag, turning the instance into a credential relay. +func proxyTargetAllowed(rawURL string) bool { + parsed, err := url.Parse(rawURL) + if err != nil || parsed.Host == "" || parsed.User != nil { + return false + } + return true +} + // GetAudioProxy streams an enclosure through the listener's KoalaCast instance. // The browser uses it only as an explicitly enabled fallback when publisher CORS // blocks direct downloads or Web Audio effects. Ordinary playback stays direct. @@ -177,6 +195,10 @@ func (h *ProxyHandler) GetAudioProxy(w http.ResponseWriter, r *http.Request) { http.Error(w, `{"error":"valid http/https url required"}`, http.StatusBadRequest) return } + if !proxyTargetAllowed(rawURL) { + http.Error(w, `{"error":"valid http/https url required"}`, http.StatusBadRequest) + return + } rangeHeader := strings.TrimSpace(r.Header.Get("Range")) if rangeHeader != "" && !validAudioRange(rangeHeader) { w.Header().Set("Content-Range", "bytes */"+strconv.FormatInt(maxAudioDownloadBytes, 10)) @@ -400,6 +422,10 @@ func (h *ProxyHandler) GetImageProxy(w http.ResponseWriter, r *http.Request) { http.Error(w, `{"error":"valid http/https url required"}`, http.StatusBadRequest) return } + if !proxyTargetAllowed(rawURL) { + http.Error(w, `{"error":"valid http/https url required"}`, http.StatusBadRequest) + return + } targetW := 300 if wStr := r.URL.Query().Get("w"); wStr != "" { diff --git a/services/api/internal/server/handlers/proxy_test.go b/services/api/internal/server/handlers/proxy_test.go index 0406893b..17ed02ac 100644 --- a/services/api/internal/server/handlers/proxy_test.go +++ b/services/api/internal/server/handlers/proxy_test.go @@ -9,6 +9,7 @@ import ( "image/jpeg" "net/http" "net/http/httptest" + "net/url" "strings" "testing" @@ -415,3 +416,39 @@ func TestProxyHandler_ImageRequestSendsNegotiatedAccept(t *testing.T) { t.Fatalf("upstream saw Accept %q, want %q", seenAccept, acceptedImageFormats) } } + +// Both proxy endpoints take a URL straight out of a publisher's feed. The +// dialer resolves and vets every address, but it never looks at the userinfo +// component, so credentials embedded in an artwork or enclosure URL would be +// presented upstream by the instance itself. +func TestProxyHandler_RejectsEmbeddedCredentials(t *testing.T) { + proxy := NewProxyHandler(true) + proxy.httpClient = rss.NewSafeHTTPClient(rss.SafeTransportConfig{AllowLoopback: true}) + proxy.streamClient = rss.NewSafeHTTPClient(rss.SafeTransportConfig{AllowLoopback: true}) + + const hostile = "https://user:secret@cdn.example/cover.jpg" + for name, call := range map[string]func(http.ResponseWriter, *http.Request){ + "image": proxy.GetImageProxy, + "audio": proxy.GetAudioProxy, + } { + rec := httptest.NewRecorder() + call(rec, httptest.NewRequest(http.MethodGet, "/p?url="+url.QueryEscape(hostile), nil)) + if rec.Code != http.StatusBadRequest { + t.Errorf("%s proxy accepted embedded credentials: got %d, want 400", name, rec.Code) + } + } +} + +// The loopback targets the test transport is allowed to reach must keep working: +// address policy belongs to the dialer, not to this check. +func TestProxyTargetAllowedLeavesAddressPolicyToTheDialer(t *testing.T) { + if !proxyTargetAllowed("http://127.0.0.1:8080/cover.jpg") { + t.Error("a loopback target must not be rejected here") + } + if proxyTargetAllowed("https://user@host/x") { + t.Error("userinfo must be rejected") + } + if proxyTargetAllowed("https:///no-host") { + t.Error("a missing host must be rejected") + } +} From 5e9fda64d6e83988d44eef51c944e050a37d5ad6 Mon Sep 17 00:00:00 2001 From: Timo <6156589+Shik3i@users.noreply.github.com> Date: Sat, 15 Aug 2026 17:16:54 +0200 Subject: [PATCH 2/2] Harden the last-admin guard and stop truncated reads passing as complete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third pass of the audit, over the remaining server handlers and the Android data layer. An instance could be left with no administrator. SuspendUser counted the active admins outside its write transaction, so two administrators suspending each other at the same time both saw two, both passed the guard, and the result was an instance nobody could administer — recoverable only by editing the database by hand. The count moves inside the transaction, which is BEGIN IMMEDIATE, so the second request waits and sees the first one's effect. Two list endpoints ignored rows.Err(). A read that stops early leaves a short slice that looks like a complete answer: - ListSessions is what a listener uses to spot a session they do not recognise, and the one row missing from a truncated read could be exactly that one. - ExportAccount is offered as the listener's complete copy of their own data. A file that is quietly missing rows is worse than a failed request. Both now fail the request instead. Also corrects the AutoDownloadWorker scheduling comment, which claimed the job runs on an unmetered network while the constraint says CONNECTED. The behaviour is right — that pass only reads feed metadata, and "download over Wi-Fi only" becomes an UNMETERED constraint in DownloadRepository.enqueue, on the transfer that actually spends the listener's data — but the comment described a guarantee the code does not make. Co-Authored-By: Claude Opus 5 --- .../data/repository/AutoDownloadWorker.kt | 8 +++- .../api/internal/server/handlers/account.go | 8 ++++ .../api/internal/server/handlers/admin.go | 37 +++++++++++-------- services/api/internal/server/handlers/auth.go | 9 +++++ 4 files changed, 46 insertions(+), 16 deletions(-) diff --git a/apps/android/core/data/src/main/kotlin/net/koalastuff/koalacast/core/data/repository/AutoDownloadWorker.kt b/apps/android/core/data/src/main/kotlin/net/koalastuff/koalacast/core/data/repository/AutoDownloadWorker.kt index 7ccea1fa..422411fe 100644 --- a/apps/android/core/data/src/main/kotlin/net/koalastuff/koalacast/core/data/repository/AutoDownloadWorker.kt +++ b/apps/android/core/data/src/main/kotlin/net/koalastuff/koalacast/core/data/repository/AutoDownloadWorker.kt @@ -148,9 +148,15 @@ class AutoDownloadWorker @AssistedInject constructor( private const val MAX_CONCURRENT_FEEDS = 4 /** - * Every six hours, on unmetered network. WorkManager will not run a periodic + * Every six hours, on any connection. WorkManager will not run a periodic * job more often than every 15 minutes anyway, and a podcast feed that * updates faster than six hours is not a thing worth burning battery on. + * + * Deliberately not UNMETERED: this pass only reads feed metadata to decide + * what is worth fetching. The audio itself is enqueued through + * [DownloadRepository.enqueue], which is where "download over Wi-Fi only" + * turns into an UNMETERED constraint on the transfer that actually spends + * the listener's data. */ fun schedule(context: Context) { val request = PeriodicWorkRequestBuilder(6, TimeUnit.HOURS) diff --git a/services/api/internal/server/handlers/account.go b/services/api/internal/server/handlers/account.go index 30a1bdc2..62ee0a2d 100644 --- a/services/api/internal/server/handlers/account.go +++ b/services/api/internal/server/handlers/account.go @@ -259,7 +259,15 @@ func (h *AuthHandler) ExportAccount(w http.ResponseWriter, r *http.Request) { } collected = append(collected, map[string]any{"id": id, "created_at": created}) } + // An export is the listener's copy of their own data, and it is offered as + // a complete one. A read that stopped early has to fail the request rather + // than hand back a shorter file that looks whole. + err = rows.Err() rows.Close() + if err != nil { + http.Error(w, `{"error":"failed to export account"}`, http.StatusInternalServerError) + return + } export[table.key] = collected } diff --git a/services/api/internal/server/handlers/admin.go b/services/api/internal/server/handlers/admin.go index 728eb1f1..fc43b36c 100644 --- a/services/api/internal/server/handlers/admin.go +++ b/services/api/internal/server/handlers/admin.go @@ -140,21 +140,10 @@ func (h *AdminHandler) SuspendUser(w http.ResponseWriter, r *http.Request) { return } - if req.Suspend { - // Guard against an admin locking themselves out, or removing the last admin. - if authUser != nil && targetID == authUser.ID { - http.Error(w, `{"error":"you cannot suspend your own account"}`, http.StatusBadRequest) - return - } - if targetRole == "admin" { - var activeAdmins int - _ = h.DB.SQL.QueryRowContext(r.Context(), - "SELECT COUNT(*) FROM users WHERE role = 'admin' AND is_suspended = 0").Scan(&activeAdmins) - if activeAdmins <= 1 { - http.Error(w, `{"error":"cannot suspend the last active admin"}`, http.StatusBadRequest) - return - } - } + if req.Suspend && authUser != nil && targetID == authUser.ID { + // Guard against an admin locking themselves out. + http.Error(w, `{"error":"you cannot suspend your own account"}`, http.StatusBadRequest) + return } suspendInt := 0 @@ -169,6 +158,24 @@ func (h *AdminHandler) SuspendUser(w http.ResponseWriter, r *http.Request) { } defer tx.Rollback() + // Counted inside the write transaction. Read outside it, two administrators + // suspending each other at the same time both saw two active admins, both + // passed, and the instance was left with none — recoverable only by editing + // the database by hand. Every transaction here is BEGIN IMMEDIATE, so the + // second one waits and sees the first one's effect. + if req.Suspend && targetRole == "admin" { + var activeAdmins int + if err := tx.QueryRowContext(r.Context(), + "SELECT COUNT(*) FROM users WHERE role = 'admin' AND is_suspended = 0").Scan(&activeAdmins); err != nil { + http.Error(w, `{"error":"database error"}`, http.StatusInternalServerError) + return + } + if activeAdmins <= 1 { + http.Error(w, `{"error":"cannot suspend the last active admin"}`, http.StatusBadRequest) + return + } + } + if _, err := tx.ExecContext(r.Context(), "UPDATE users SET is_suspended = ? WHERE id = ?", suspendInt, targetID); err != nil { http.Error(w, `{"error":"failed to update user status"}`, http.StatusInternalServerError) return diff --git a/services/api/internal/server/handlers/auth.go b/services/api/internal/server/handlers/auth.go index 3a19aa00..bc1e5dee 100644 --- a/services/api/internal/server/handlers/auth.go +++ b/services/api/internal/server/handlers/auth.go @@ -484,6 +484,7 @@ func (h *AuthHandler) ListSessions(w http.ResponseWriter, r *http.Request) { sessions = append(sessions, item) } } + rowsErr := rows.Err() rows.Close() // Native device credentials (Bearer tokens). @@ -508,7 +509,15 @@ func (h *AuthHandler) ListSessions(w http.ResponseWriter, r *http.Request) { sessions = append(sessions, item) } } + dRowsErr := dRows.Err() dRows.Close() + // This list is what a listener uses to spot a session they do not recognise. + // A truncated read must not quietly present a shorter list as the whole + // picture — the one session missing from it could be the one that matters. + if rowsErr != nil || dRowsErr != nil { + http.Error(w, `{"error":"database error"}`, http.StatusInternalServerError) + return + } w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusOK)