mirror of
https://github.com/navidrome/navidrome.git
synced 2026-10-09 02:47:29 +02:00
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.
This commit is contained in:
parent
7338461efe
commit
23f28aac66
10 changed files with 171 additions and 21 deletions
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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")
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
}
|
||||
|
|
@ -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
|
||||
}
|
||||
|
|
@ -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())
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue