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 7ccea1f..422411f 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/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 e0bc2ed..9b3cd0c 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 3fcd027..05944d9 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 1937029..808c17a 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 c6d7103..ed64286 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/account.go b/services/api/internal/server/handlers/account.go index 30a1bdc..62ee0a2 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 728eb1f..fc43b36 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 3a19aa0..bc1e5de 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) diff --git a/services/api/internal/server/handlers/proxy.go b/services/api/internal/server/handlers/proxy.go index fed3359..30face7 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 0406893..17ed02a 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") + } +}