Audit part 2: episode types, proxy credential relay, player state leaks

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<AutoDownloadWorker>(6, TimeUnit.HOURS)

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(

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,
                 )
             }
         }

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)

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 := `<?xml version="1.0"?>
+<rss version="2.0" xmlns:itunes="http://www.itunes.com/dtds/podcast-1.0.dtd">
+  <channel>
+    <title>Show</title>
+    <item>
+      <title>Season one is coming</title>
+      <guid>ep-trailer</guid>
+      <itunes:episodeType>trailer</itunes:episodeType>
+    </item>
+    <item>
+      <title>Behind the scenes</title>
+      <guid>ep-bonus</guid>
+      <itunes:episodeType>Bonus</itunes:episodeType>
+    </item>
+    <item>
+      <title>Episode one</title>
+      <guid>ep-plain</guid>
+    </item>
+  </channel>
+</rss>`
+
+	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 := `<?xml version="1.0"?>
+<feed xmlns="http://www.w3.org/2005/Atom">
+  <title>Show</title>
+  <entry>
+    <id>entry-1</id>
+    <title>Episode one</title>
+    <link rel="enclosure" href="https://cdn.example/a.mp3" type="audio/mpeg"/>
+  </entry>
+</feed>`
+
+	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)
+	}
+}

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
 	}
 

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

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)

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 != "" {

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")
+	}
+}