From 23f28aac669c8aebea67a8e7aeb1f8a9c1a653fd Mon Sep 17 00:00:00 2001 From: Jiho Andrew Lee Date: Sat, 19 Sep 2026 20:37:25 +0900 Subject: [PATCH] fix(podcast): implement missing SSRF validation and fix migration boot failure Two blocking bugs found while reviewing this branch: 1. Compile error: validateURL() was called in fetchAndParse/doDownload (added while applying Strix's SSRF suggestions) but the function itself was never committed - only "Add validateURL and isReservedIP helper functions" was left as a plain-text suggestion with no one-click apply, and it got missed. Implemented validateURL/isReservedIP plus a safeHTTPTransport whose DialContext re-resolves and re-checks the target IP at actual connection time (not just once via a URL pre-check), so a DNS answer that changes between the check and the request (DNS rebinding) can't reach a reserved address - this also covers HTTP redirect targets for free, since redirects reuse the same Transport. Added AllowLoopbackHTTPForTests() so the existing httptest-based suite (which binds to 127.0.0.1) still passes without weakening the guard for any other address. 2. Migration boot failure: the podcast migrations were dated 2026-04-27/28 (when the feature was actually developed), but goose.UpContext (as this project calls it, no WithAllowMissing) hard-errors on any pending migration older than the DB's already-applied max version. Any install already past April on current master would fail to start entirely on upgrade. Renumbered all 5 podcast migrations to 2026-09-02 (after everything currently on master). This also meant the podcast_* columns added to the already-shipped uniform_canonical_ids migration's idColumns were dead code for any install that had already run that migration - editing an applied migration's Go source doesn't make it re-run. Reverted that edit and split the podcast id canonicalization into its own, later migration (20260902000005) that reuses the same buildIDMap/ applyIDMap machinery. Verified both fresh-install and existing-install upgrade paths end-to-end against real sqlite DBs: no boot error, and legacy-shaped podcast ids (plus their FK references) get correctly rewritten to canonical form. Verified: full build clean, core/podcasts + server/nativeapi + db/migrations test suites all pass, gofmt clean. --- core/podcasts/podcasts.go | 95 ++++++++++++++++++- core/podcasts/podcasts_suite_test.go | 7 ++ .../20260720015443_uniform_canonical_ids.go | 17 ++-- ...dcast.go => 20260902000000_add_podcast.go} | 8 ++ ...902000001_add_podcast_downloaded_bytes.go} | 0 ...t20.go => 20260902000002_add_podcast20.go} | 0 ...go => 20260902000003_add_podcast_tier3.go} | 0 ...=> 20260902000004_add_podcast_metadata.go} | 0 ...902000005_podcast_uniform_canonical_ids.go | 57 +++++++++++ db/migrations/uniform_canonical_ids_test.go | 8 -- 10 files changed, 171 insertions(+), 21 deletions(-) rename db/migrations/{20260427165650_add_podcast.go => 20260902000000_add_podcast.go} (74%) rename db/migrations/{20260427184047_add_podcast_downloaded_bytes.go => 20260902000001_add_podcast_downloaded_bytes.go} (100%) rename db/migrations/{20260428000000_add_podcast20.go => 20260902000002_add_podcast20.go} (100%) rename db/migrations/{20260428120000_add_podcast_tier3.go => 20260902000003_add_podcast_tier3.go} (100%) rename db/migrations/{20260428200000_add_podcast_metadata.go => 20260902000004_add_podcast_metadata.go} (100%) create mode 100644 db/migrations/20260902000005_podcast_uniform_canonical_ids.go diff --git a/core/podcasts/podcasts.go b/core/podcasts/podcasts.go index 8714c5d2d..8ffe4eb0d 100644 --- a/core/podcasts/podcasts.go +++ b/core/podcasts/podcasts.go @@ -316,7 +316,7 @@ func (s *podcastService) doDownload(ctx context.Context, ep *model.PodcastEpisod s.setEpisodeError(ctx, ep, fmt.Errorf("invalid enclosure URL: %w", err)) return } - httpClient := &http.Client{Timeout: 30 * time.Second} + httpClient := &http.Client{Timeout: 30 * time.Second, Transport: safeHTTPTransport} resp, err := httpClient.Get(ep.EnclosureURL) //nolint:gosec if err != nil { s.setEpisodeError(ctx, ep, err) @@ -535,11 +535,102 @@ func (pw *progressWriter) Write(p []byte) (int, error) { return n, err } +// validateURL rejects any URL that is not a plain http/https request to a +// named host. It exists to prevent SSRF: without it, an authenticated user +// could point the preview/download endpoints at internal services, cloud +// metadata endpoints (e.g. 169.254.169.254), or any other host only +// reachable from the server itself. This is a cheap, fast-failing check on +// the URL's shape - the actual IP-level check happens per-connection in +// safeHTTPTransport below, since the host a URL names and the IP it +// resolves to at request time aren't guaranteed to be the same thing. +func validateURL(rawURL string) error { + u, err := url.Parse(rawURL) + if err != nil { + return fmt.Errorf("parsing URL: %w", err) + } + if u.Scheme != "http" && u.Scheme != "https" { + return fmt.Errorf("unsupported URL scheme %q, only http/https are allowed", u.Scheme) + } + if u.Hostname() == "" { + return fmt.Errorf("URL has no host") + } + return nil +} + +// isReservedIP reports whether ip is a loopback, private, link-local, +// multicast, or otherwise non-routable/internal address. Cloud metadata +// endpoints (e.g. AWS/GCP/Azure's 169.254.169.254) fall under the +// link-local range, so they're covered without a special case. +// +// This is a var, not a plain func, only so AllowLoopbackHTTPForTests (below) +// can narrow it for test binaries - production code never reassigns it. +var isReservedIP = func(ip net.IP) bool { + return ip.IsLoopback() || + ip.IsPrivate() || + ip.IsLinkLocalUnicast() || + ip.IsLinkLocalMulticast() || + ip.IsInterfaceLocalMulticast() || + ip.IsMulticast() || + ip.IsUnspecified() +} + +// AllowLoopbackHTTPForTests relaxes safeHTTPTransport's SSRF guard to permit +// loopback addresses (127.0.0.0/8, ::1) - every other reserved/private/ +// link-local range (including cloud metadata endpoints) is still refused. +// It exists because httptest.Server always binds to loopback, so the podcast +// test suite needs a way to point the service at one without disabling the +// guard entirely. Not for production use. +func AllowLoopbackHTTPForTests() { + strict := isReservedIP + isReservedIP = func(ip net.IP) bool { + if ip.IsLoopback() { + return false + } + return strict(ip) + } +} + +// safeHTTPTransport is shared by every outbound podcast HTTP request (RSS +// feed fetch and episode download). Its DialContext resolves the host and +// checks isReservedIP at the moment of connection, not just once via +// validateURL up front - so a DNS answer that changes between the URL +// check and the actual TCP connect (DNS rebinding) can't be used to reach +// a reserved address that validateURL alone would have caught. +var safeHTTPTransport = &http.Transport{ + DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) { + host, port, err := net.SplitHostPort(addr) + if err != nil { + return nil, fmt.Errorf("parsing address %q: %w", addr, err) + } + ips, err := net.DefaultResolver.LookupIPAddr(ctx, host) + if err != nil { + return nil, fmt.Errorf("resolving host %q: %w", host, err) + } + if len(ips) == 0 { + return nil, fmt.Errorf("host %q did not resolve to any address", host) + } + var dialer net.Dialer + var lastErr error + for _, ip := range ips { + if isReservedIP(ip.IP) { + lastErr = fmt.Errorf("host %q resolves to a reserved/internal address (%s), refusing to connect", host, ip.IP) + continue + } + conn, dialErr := dialer.DialContext(ctx, network, net.JoinHostPort(ip.IP.String(), port)) + if dialErr == nil { + return conn, nil + } + lastErr = dialErr + } + return nil, lastErr + }, +} + func fetchAndParse(rssURL string) (*rssFeed, error) { if err := validateURL(rssURL); err != nil { return nil, fmt.Errorf("invalid RSS feed URL: %w", err) } - httpClient := &http.Client{Timeout: 15 * time.Second} + httpClient := &http.Client{Timeout: 15 * time.Second, Transport: safeHTTPTransport} resp, err := httpClient.Get(rssURL) //nolint:gosec if err != nil { return nil, fmt.Errorf("fetching RSS feed: %w", err) diff --git a/core/podcasts/podcasts_suite_test.go b/core/podcasts/podcasts_suite_test.go index afa5922ea..09d31bb39 100644 --- a/core/podcasts/podcasts_suite_test.go +++ b/core/podcasts/podcasts_suite_test.go @@ -3,6 +3,7 @@ package podcasts_test import ( "testing" + "github.com/navidrome/navidrome/core/podcasts" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" @@ -12,6 +13,12 @@ import ( func TestPodcasts(t *testing.T) { tests.Init(t, false) log.SetLevel(log.LevelFatal) + // This suite's specs fetch RSS feeds/episodes from an httptest.Server, which + // always binds to loopback - safeHTTPTransport's SSRF guard would otherwise + // refuse every request the suite makes. See AllowLoopbackHTTPForTests's own + // doc comment: every other reserved/private/link-local address is still + // refused, so this doesn't disable the guard, just narrows it for this run. + podcasts.AllowLoopbackHTTPForTests() RegisterFailHandler(Fail) RunSpecs(t, "Podcasts Suite") } diff --git a/db/migrations/20260720015443_uniform_canonical_ids.go b/db/migrations/20260720015443_uniform_canonical_ids.go index c488ec8d8..21a70da14 100644 --- a/db/migrations/20260720015443_uniform_canonical_ids.go +++ b/db/migrations/20260720015443_uniform_canonical_ids.go @@ -77,14 +77,6 @@ var idColumns = []struct{ table, col string }{ {"media_file_artists", "media_file_id"}, {"media_file_artists", "artist_id"}, {"album_artists", "album_id"}, {"album_artists", "artist_id"}, {"library_tag", "tag_id"}, - {"podcast_channel", "id"}, - {"podcast_episode", "id"}, {"podcast_episode", "channel_id"}, {"podcast_episode", "stream_id"}, - {"podcast_transcript", "id"}, {"podcast_transcript", "episode_id"}, - {"podcast_person", "id"}, {"podcast_person", "channel_id"}, {"podcast_person", "episode_id"}, - {"podcast_podroll", "id"}, {"podcast_podroll", "channel_id"}, - {"podcast_live_item", "id"}, {"podcast_live_item", "channel_id"}, - {"podcast_funding", "id"}, {"podcast_funding", "channel_id"}, - {"podcast_image", "id"}, {"podcast_image", "channel_id"}, {"podcast_image", "episode_id"}, } // embeddedIDColumns holds ids nested inside a larger value; the id-columns guard checks this @@ -100,7 +92,7 @@ var embeddedIDColumns = []struct { } func upUniformCanonicalIds(ctx context.Context, tx *sql.Tx) error { - if err := buildIDMap(ctx, tx); err != nil { + if err := buildIDMap(ctx, tx, idColumns); err != nil { return err } for _, tc := range idColumns { @@ -137,7 +129,10 @@ func rotateSessionSecret(ctx context.Context, tx *sql.Tx) error { } // buildIDMap stages old->new pairs for every id that changes, indexed for the update joins. -func buildIDMap(ctx context.Context, tx *sql.Tx) error { +// columns is a parameter (not always the package-level idColumns) so a later migration can +// reuse this same collect-and-rewrite machinery for a different, disjoint set of columns - see +// podcast_uniform_canonical_ids.go, which does exactly that for tables idColumns predates. +func buildIDMap(ctx context.Context, tx *sql.Tx, columns []struct{ table, col string }) error { _, err := tx.ExecContext(ctx, "CREATE TEMP TABLE _id_map (old_id TEXT PRIMARY KEY, new_id TEXT NOT NULL) WITHOUT ROWID") if err != nil { @@ -148,7 +143,7 @@ func buildIDMap(ctx context.Context, tx *sql.Tx) error { return err } defer ins.Close() - for _, tc := range idColumns { + for _, tc := range columns { if err := collectColumn(ctx, tx, ins, tc.table, tc.col); err != nil { return fmt.Errorf("collecting %s.%s: %w", tc.table, tc.col, err) } diff --git a/db/migrations/20260427165650_add_podcast.go b/db/migrations/20260902000000_add_podcast.go similarity index 74% rename from db/migrations/20260427165650_add_podcast.go rename to db/migrations/20260902000000_add_podcast.go index 7bd8f3171..eabffeecf 100644 --- a/db/migrations/20260427165650_add_podcast.go +++ b/db/migrations/20260902000000_add_podcast.go @@ -7,6 +7,14 @@ import ( "github.com/pressly/goose/v3" ) +// This file (and the 4 that follow it, add_podcast_downloaded_bytes/podcast20/podcast_tier3/ +// podcast_metadata) were originally timestamped 2026-04-27/28, matching when the podcast +// feature was actually developed. They were renumbered to 2026-09-02 (after every migration +// already on master as of this PR) before merging: goose.UpContext (as navidrome calls it, with +// no WithAllowMissing) hard-errors and refuses to start if it finds a pending migration whose +// version is lower than the DB's already-applied max version - which every one of these files +// would have been, for any install that had already migrated past April on current master. +// Keep new migrations timestamped at-or-after merge time, not authoring time. func init() { goose.AddMigrationContext(upAddPodcast, downAddPodcast) } diff --git a/db/migrations/20260427184047_add_podcast_downloaded_bytes.go b/db/migrations/20260902000001_add_podcast_downloaded_bytes.go similarity index 100% rename from db/migrations/20260427184047_add_podcast_downloaded_bytes.go rename to db/migrations/20260902000001_add_podcast_downloaded_bytes.go diff --git a/db/migrations/20260428000000_add_podcast20.go b/db/migrations/20260902000002_add_podcast20.go similarity index 100% rename from db/migrations/20260428000000_add_podcast20.go rename to db/migrations/20260902000002_add_podcast20.go diff --git a/db/migrations/20260428120000_add_podcast_tier3.go b/db/migrations/20260902000003_add_podcast_tier3.go similarity index 100% rename from db/migrations/20260428120000_add_podcast_tier3.go rename to db/migrations/20260902000003_add_podcast_tier3.go diff --git a/db/migrations/20260428200000_add_podcast_metadata.go b/db/migrations/20260902000004_add_podcast_metadata.go similarity index 100% rename from db/migrations/20260428200000_add_podcast_metadata.go rename to db/migrations/20260902000004_add_podcast_metadata.go diff --git a/db/migrations/20260902000005_podcast_uniform_canonical_ids.go b/db/migrations/20260902000005_podcast_uniform_canonical_ids.go new file mode 100644 index 000000000..69ba9faf2 --- /dev/null +++ b/db/migrations/20260902000005_podcast_uniform_canonical_ids.go @@ -0,0 +1,57 @@ +package migrations + +import ( + "context" + "database/sql" + "fmt" + + "github.com/pressly/goose/v3" +) + +func init() { + goose.AddMigrationContext(upPodcastUniformCanonicalIds, downPodcastUniformCanonicalIds) +} + +// podcastIDColumns lists every Navidrome-id-bearing podcast_* column, the same inventory +// uniform_canonical_ids (20260720015443) keeps for every other table - see this file's own +// upPodcastUniformCanonicalIds doc comment for why podcast ids need their own, later migration +// instead of just being added to that one's idColumns. +var podcastIDColumns = []struct{ table, col string }{ + {"podcast_channel", "id"}, + {"podcast_episode", "id"}, {"podcast_episode", "channel_id"}, {"podcast_episode", "stream_id"}, + {"podcast_transcript", "id"}, {"podcast_transcript", "episode_id"}, + {"podcast_person", "id"}, {"podcast_person", "channel_id"}, {"podcast_person", "episode_id"}, + {"podcast_podroll", "id"}, {"podcast_podroll", "channel_id"}, + {"podcast_live_item", "id"}, {"podcast_live_item", "channel_id"}, + {"podcast_funding", "id"}, {"podcast_funding", "channel_id"}, + {"podcast_image", "id"}, {"podcast_image", "channel_id"}, {"podcast_image", "episode_id"}, +} + +// upPodcastUniformCanonicalIds rewrites podcast_* ids to the same canonical 22-char base62 +// encoding uniform_canonical_ids (20260720015443) already applied to every other table. +// +// It has to be a separate, later migration rather than an addition to that one's idColumns, +// for two independent reasons: +// 1. The podcast_* tables don't exist yet when 20260720015443 runs - the add_podcast* migrations +// that create them are timestamped after it (2026-09-02, see add_podcast.go's own comment on +// why) - so a SELECT against them there would fail outright, even on a fresh install. +// 2. Editing an already-applied migration's Go source has no runtime effect on any install that +// already ran it: goose tracks migrations as applied-or-not by version, not by re-diffing +// their source on every startup. An install that ran 20260720015443 before this feature +// existed would never re-run it, no matter what idColumns says today. +func upPodcastUniformCanonicalIds(ctx context.Context, tx *sql.Tx) error { + if err := buildIDMap(ctx, tx, podcastIDColumns); err != nil { + return err + } + for _, tc := range podcastIDColumns { + if err := applyIDMap(ctx, tx, tc.table, tc.col); err != nil { + return fmt.Errorf("canonicalizing %s.%s: %w", tc.table, tc.col, err) + } + } + _, err := tx.ExecContext(ctx, "DROP TABLE _id_map") + return err +} + +func downPodcastUniformCanonicalIds(ctx context.Context, tx *sql.Tx) error { + return nil // irreversible data migration +} diff --git a/db/migrations/uniform_canonical_ids_test.go b/db/migrations/uniform_canonical_ids_test.go index 05d796c83..ced75cb7e 100644 --- a/db/migrations/uniform_canonical_ids_test.go +++ b/db/migrations/uniform_canonical_ids_test.go @@ -65,14 +65,6 @@ var _ = Describe("upUniformCanonicalIds", func() { CREATE TABLE library_tag (tag_id text, library_id integer); CREATE TABLE plugin (id text, users text); CREATE TABLE property (id text primary key, value text); - CREATE TABLE podcast_channel (id text); - CREATE TABLE podcast_episode (id text, channel_id text, stream_id text); - CREATE TABLE podcast_transcript (id text, episode_id text); - CREATE TABLE podcast_person (id text, channel_id text, episode_id text); - CREATE TABLE podcast_podroll (id text, channel_id text); - CREATE TABLE podcast_live_item (id text, channel_id text); - CREATE TABLE podcast_funding (id text, channel_id text); - CREATE TABLE podcast_image (id text, channel_id text, episode_id text); `) Expect(err).ToNot(HaveOccurred())