From 4bebfad0ede2145b336b91010752b826fcda4dd0 Mon Sep 17 00:00:00 2001 From: Jiho Andrew Lee Date: Wed, 30 Sep 2026 15:42:14 +0900 Subject: [PATCH] fix(podcast): address CodeRabbit review feedback - Remove total HTTP client timeout from episode downloads (use transport phase timeouts instead) and delete partial files on failure - Keep SSE progress listener subscribed while the show view is mounted - Return podcastEpisode (with streamId) from getNewestPodcasts - Clean up dependent rows and MediaFiles in DeleteChannel within a tx - Limit RSS feed body size to 32 MiB - Block additional reserved IP ranges in the SSRF guard - Fall back to enclosure URL when an item has no - Handle clipboard write failure in the podcast list --- core/podcasts/podcasts.go | 91 ++++++++++++++++++++++++-- core/podcasts/rss.go | 24 ++++--- core/podcasts/rss_test.go | 9 +++ resources/i18n/ko.json | 1 + server/subsonic/podcasts.go | 20 +----- server/subsonic/podcasts_test.go | 10 +-- server/subsonic/responses/responses.go | 9 +-- ui/src/i18n/en.json | 1 + ui/src/podcast/PodcastList.jsx | 6 +- ui/src/podcast/PodcastShow.jsx | 7 +- 10 files changed, 126 insertions(+), 52 deletions(-) diff --git a/core/podcasts/podcasts.go b/core/podcasts/podcasts.go index 5ac6ed5a3..3630dbc34 100644 --- a/core/podcasts/podcasts.go +++ b/core/podcasts/podcasts.go @@ -316,8 +316,15 @@ 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, Transport: safeHTTPTransport} - resp, err := httpClient.Get(ep.EnclosureURL) //nolint:gosec + // No total Client.Timeout here: it would also cover reading the body and + // abort large episodes. Connection-phase limits live on safeHTTPTransport. + httpClient := &http.Client{Transport: safeHTTPTransport} + req, err := http.NewRequestWithContext(ctx, http.MethodGet, ep.EnclosureURL, nil) + if err != nil { + s.setEpisodeError(ctx, ep, err) + return + } + resp, err := httpClient.Do(req) //nolint:gosec if err != nil { s.setEpisodeError(ctx, ep, err) return @@ -337,6 +344,8 @@ func (s *podcastService) doDownload(ctx context.Context, ep *model.PodcastEpisod size, err := io.Copy(&progressWriter{ep: ep, ds: s.ds, broker: s.broker, ctx: ctx, w: f}, resp.Body) if err != nil { + f.Close() + _ = os.Remove(dest) // don't leave a partial file behind s.setEpisodeError(ctx, ep, err) return } @@ -499,7 +508,39 @@ func (s *podcastService) DeleteChannel(ctx context.Context, id string) error { _ = os.Remove(ep.Path) } } - return s.ds.PodcastChannel().Delete(ctx, id) + // The channel-/episode-scoped detail tables have no FK cascade, and the + // registered MediaFiles are not FK-linked either, so clean them up here. + return s.ds.WithTx(func(tx model.DataStore) error { + for _, ep := range episodes { + if ep.StreamID != "" { + if err := tx.MediaFile().Delete(ctx, ep.StreamID); err != nil { + return err + } + } + if err := tx.PodcastPerson().SaveForEpisode(ctx, ep.ID, nil); err != nil { + return err + } + if err := tx.PodcastImage().SaveForEpisode(ctx, ep.ID, nil); err != nil { + return err + } + } + if err := tx.PodcastPerson().SaveForChannel(ctx, id, nil); err != nil { + return err + } + if err := tx.PodcastFunding().SaveForChannel(ctx, id, nil); err != nil { + return err + } + if err := tx.PodcastImage().SaveForChannel(ctx, id, nil); err != nil { + return err + } + if err := tx.PodcastPodroll().SaveForChannel(ctx, id, nil); err != nil { + return err + } + if err := tx.PodcastLiveItem().DeleteByChannel(ctx, id); err != nil { + return err + } + return tx.PodcastChannel().Delete(ctx, id) + }) } // progressWriter wraps an io.Writer and periodically saves download progress to DB. @@ -571,7 +612,36 @@ var isReservedIP = func(ip net.IP) bool { ip.IsLinkLocalMulticast() || ip.IsInterfaceLocalMulticast() || ip.IsMulticast() || - ip.IsUnspecified() + ip.IsUnspecified() || + inReservedNets(ip) +} + +// reservedNets are special-purpose ranges that net.IP's helpers don't cover. +var reservedNets = func() []*net.IPNet { + var nets []*net.IPNet + for _, cidr := range []string{ + "0.0.0.0/8", // "this" network + "100.64.0.0/10", // carrier-grade NAT + "192.0.0.0/24", // IETF protocol assignments + "198.18.0.0/15", // benchmarking + "64:ff9b::/96", // NAT64 + } { + _, n, err := net.ParseCIDR(cidr) + if err != nil { + panic(err) + } + nets = append(nets, n) + } + return nets +}() + +func inReservedNets(ip net.IP) bool { + for _, n := range reservedNets { + if n.Contains(ip) { + return true + } + } + return false } // AllowLoopbackHTTPForTests relaxes safeHTTPTransport's SSRF guard to permit @@ -597,6 +667,8 @@ func AllowLoopbackHTTPForTests() { // 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{ + TLSHandshakeTimeout: 15 * time.Second, + ResponseHeaderTimeout: 30 * time.Second, DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) { host, port, err := net.SplitHostPort(addr) if err != nil { @@ -609,7 +681,7 @@ var safeHTTPTransport = &http.Transport{ if len(ips) == 0 { return nil, fmt.Errorf("host %q did not resolve to any address", host) } - var dialer net.Dialer + dialer := net.Dialer{Timeout: 15 * time.Second} var lastErr error for _, ip := range ips { if isReservedIP(ip.IP) { @@ -626,6 +698,8 @@ var safeHTTPTransport = &http.Transport{ }, } +const maxFeedSize = 32 << 20 // 32 MiB + func fetchAndParse(rssURL string) (*rssFeed, error) { if err := validateURL(rssURL); err != nil { return nil, fmt.Errorf("invalid RSS feed URL: %w", err) @@ -637,10 +711,15 @@ func fetchAndParse(rssURL string) (*rssFeed, error) { } defer resp.Body.Close() - data, err := io.ReadAll(resp.Body) + // Read at most one byte past the limit so an oversized feed is detected + // without buffering it entirely in memory. + data, err := io.ReadAll(io.LimitReader(resp.Body, maxFeedSize+1)) if err != nil { return nil, fmt.Errorf("reading RSS feed: %w", err) } + if len(data) > maxFeedSize { + return nil, fmt.Errorf("RSS feed exceeds %d bytes", maxFeedSize) + } return ParseRSSFeed(data) } diff --git a/core/podcasts/rss.go b/core/podcasts/rss.go index 70c618ab1..460d52c20 100644 --- a/core/podcasts/rss.go +++ b/core/podcasts/rss.go @@ -236,15 +236,15 @@ type rssItem struct { ItunesDur string `xml:"http://www.itunes.com/dtds/podcast-1.0.dtd duration"` // Podcasting 2.0 episode tags - PodcastChapters podcastChapters `xml:"https://podcastindex.org/namespace/1.0 chapters"` + PodcastChapters podcastChapters `xml:"https://podcastindex.org/namespace/1.0 chapters"` PodcastTranscripts []podcastTranscript `xml:"https://podcastindex.org/namespace/1.0 transcript"` - PodcastSeason podcastSeason `xml:"https://podcastindex.org/namespace/1.0 season"` - PodcastEpisodeNum podcastEpisodeNum `xml:"https://podcastindex.org/namespace/1.0 episode"` - PodcastSoundbite podcastSoundbite `xml:"https://podcastindex.org/namespace/1.0 soundbite"` - PodcastPersons []podcastPerson `xml:"https://podcastindex.org/namespace/1.0 person"` - PodcastLocation podcastLocation `xml:"https://podcastindex.org/namespace/1.0 location"` - PodcastLicense podcastLicense `xml:"https://podcastindex.org/namespace/1.0 license"` - PodcastImages []podcastImageTag `xml:"https://podcastindex.org/namespace/1.0 image"` + PodcastSeason podcastSeason `xml:"https://podcastindex.org/namespace/1.0 season"` + PodcastEpisodeNum podcastEpisodeNum `xml:"https://podcastindex.org/namespace/1.0 episode"` + PodcastSoundbite podcastSoundbite `xml:"https://podcastindex.org/namespace/1.0 soundbite"` + PodcastPersons []podcastPerson `xml:"https://podcastindex.org/namespace/1.0 person"` + PodcastLocation podcastLocation `xml:"https://podcastindex.org/namespace/1.0 location"` + PodcastLicense podcastLicense `xml:"https://podcastindex.org/namespace/1.0 license"` + PodcastImages []podcastImageTag `xml:"https://podcastindex.org/namespace/1.0 image"` } type enclosure struct { @@ -413,8 +413,14 @@ func ParseRSSFeed(data []byte) (*rssFeed, error) { pubDate, _ := parseRSSDate(item.PubDate) suffix := suffixFromMIME(item.Enclosure.Type, item.Enclosure.URL) + // is optional in RSS 2.0; fall back to the enclosure URL so + // episodes keep a unique identity for de-duplication on refresh. + guid := strings.TrimSpace(item.GUID) + if guid == "" { + guid = item.Enclosure.URL + } ep := model.PodcastEpisode{ - GUID: item.GUID, + GUID: guid, Title: item.Title, Description: desc, PublishDate: pubDate, diff --git a/core/podcasts/rss_test.go b/core/podcasts/rss_test.go index 41728ccb9..95b91b9ef 100644 --- a/core/podcasts/rss_test.go +++ b/core/podcasts/rss_test.go @@ -77,6 +77,15 @@ var _ = Describe("ParseRSSFeed", func() { Expect(feed.Episodes).To(HaveLen(2)) }) + It("falls back to the enclosure URL when is missing", func() { + feed, err := podcasts.ParseRSSFeed([]byte(`T +No guid +`)) + Expect(err).ToNot(HaveOccurred()) + Expect(feed.Episodes).To(HaveLen(1)) + Expect(feed.Episodes[0].GUID).To(Equal("https://example.com/noguid.mp3")) + }) + It("parses episode fields correctly", func() { feed, _ := podcasts.ParseRSSFeed([]byte(testRSSFeed)) ep := feed.Episodes[0] diff --git a/resources/i18n/ko.json b/resources/i18n/ko.json index 711e5acaf..56ac225c3 100644 --- a/resources/i18n/ko.json +++ b/resources/i18n/ko.json @@ -267,6 +267,7 @@ "downloadStarted": "다운로드 시작됨", "channelAdded": "채널 추가됨", "urlCopied": "피드 URL이 클립보드에 복사됨", + "urlCopyFailed": "피드 URL을 복사할 수 없습니다", "alreadyExists": "이미 등록된 채널입니다.", "channelDeleted": "채널 삭제됨", "episodeDeleted": "에피소드 삭제됨" diff --git a/server/subsonic/podcasts.go b/server/subsonic/podcasts.go index a56f9f67d..31b9a507f 100644 --- a/server/subsonic/podcasts.go +++ b/server/subsonic/podcasts.go @@ -225,25 +225,7 @@ func (api *Router) GetNewestPodcasts(r *http.Request) (*responses.Subsonic, erro resp := newResponse() resp.NewestPodcasts = &responses.NewestPodcasts{} for _, ep := range eps { - child := responses.Child{ - Id: ep.ID, - Title: ep.Title, - IsDir: false, - Parent: ep.ChannelID, - Duration: int32(ep.Duration), - Size: ep.Size, - BitRate: int32(ep.BitRate), - Suffix: ep.Suffix, - ContentType: ep.ContentType, - Type: "podcast", - ChannelId: ep.ChannelID, - Description: ep.Description, - Status: string(ep.Status), - } - if !ep.PublishDate.IsZero() { - child.PublishDate = ep.PublishDate.UTC().Format(time.RFC3339) - } - resp.NewestPodcasts.Episode = append(resp.NewestPodcasts.Episode, child) + resp.NewestPodcasts.Episode = append(resp.NewestPodcasts.Episode, buildPodcastEpisode(ep)) } return resp, nil } diff --git a/server/subsonic/podcasts_test.go b/server/subsonic/podcasts_test.go index e9cf997f2..df31798af 100644 --- a/server/subsonic/podcasts_test.go +++ b/server/subsonic/podcasts_test.go @@ -118,12 +118,12 @@ var _ = Describe("Podcasts", func() { BeforeEach(func() { now := time.Now() episodeRepo.Data = map[string]*model.PodcastEpisode{ - "ep-1": {ID: "ep-1", Title: "Ep1", ChannelID: "ch-1", PublishDate: now.Add(-time.Hour), Status: model.PodcastStatusCompleted}, - "ep-2": {ID: "ep-2", Title: "Ep2", ChannelID: "ch-1", PublishDate: now, Status: model.PodcastStatusNew}, + "ep-1": {ID: "ep-1", Title: "Ep1", ChannelID: "ch-1", StreamID: "stream-ep-1", PublishDate: now.Add(-time.Hour), Status: model.PodcastStatusCompleted}, + "ep-2": {ID: "ep-2", Title: "Ep2", ChannelID: "ch-1", StreamID: "stream-ep-2", PublishDate: now, Status: model.PodcastStatusNew}, } }) - It("returns episodes in Child format", func() { + It("returns episodes in podcastEpisode format", func() { r := httptest.NewRequest("GET", "/rest/getNewestPodcasts", nil) r = r.WithContext(userCtx) @@ -132,14 +132,14 @@ var _ = Describe("Podcasts", func() { Expect(resp.NewestPodcasts.Episode).To(HaveLen(2)) }) - It("sets type to podcast", func() { + It("includes streamId so clients can play the episode", func() { r := httptest.NewRequest("GET", "/rest/getNewestPodcasts", nil) r = r.WithContext(userCtx) resp, err := api.GetNewestPodcasts(r) Expect(err).ToNot(HaveOccurred()) for _, ep := range resp.NewestPodcasts.Episode { - Expect(ep.Type).To(Equal("podcast")) + Expect(ep.StreamId).To(Equal("stream-" + ep.ID)) } }) diff --git a/server/subsonic/responses/responses.go b/server/subsonic/responses/responses.go index 6e4ea63f4..69250aa56 100644 --- a/server/subsonic/responses/responses.go +++ b/server/subsonic/responses/responses.go @@ -169,12 +169,7 @@ type Child struct { SongCount int32 `xml:"songCount,attr,omitempty" json:"songCount,omitempty"` IsVideo bool `xml:"isVideo,attr,omitempty" json:"isVideo,omitempty"` BookmarkPosition int64 `xml:"bookmarkPosition,attr,omitempty" json:"bookmarkPosition,omitempty"` - // Podcast-specific fields (used in getNewestPodcasts) - ChannelId string `xml:"channelId,attr,omitempty" json:"channelId,omitempty"` - Description string `xml:"description,attr,omitempty" json:"description,omitempty"` - Status string `xml:"status,attr,omitempty" json:"status,omitempty"` - PublishDate string `xml:"publishDate,attr,omitempty" json:"publishDate,omitempty"` - *OpenSubsonicChild `xml:",omitempty" json:",omitempty"` + *OpenSubsonicChild `xml:",omitempty" json:",omitempty"` } type OpenSubsonicChild struct { @@ -706,7 +701,7 @@ type Podcasts struct { } type NewestPodcasts struct { - Episode []Child `xml:"episode,omitempty" json:"episode,omitempty"` + Episode []PodcastEpisode `xml:"episode,omitempty" json:"episode,omitempty"` } type PodcastFundingResp struct { diff --git a/ui/src/i18n/en.json b/ui/src/i18n/en.json index 2ac47f820..bbb57db70 100644 --- a/ui/src/i18n/en.json +++ b/ui/src/i18n/en.json @@ -299,6 +299,7 @@ "downloadStarted": "Download started", "channelAdded": "Channel added", "urlCopied": "Feed URL copied to clipboard", + "urlCopyFailed": "Could not copy the feed URL", "alreadyExists": "This channel is already registered.", "channelDeleted": "Channel deleted", "episodeDeleted": "Episode deleted" diff --git a/ui/src/podcast/PodcastList.jsx b/ui/src/podcast/PodcastList.jsx index d67984827..7e7dafe6e 100644 --- a/ui/src/podcast/PodcastList.jsx +++ b/ui/src/podcast/PodcastList.jsx @@ -70,8 +70,10 @@ const FeedUrlField = ({ record }) => { if (!record?.url) return null const handleCopy = (e) => { e.stopPropagation() - navigator.clipboard.writeText(record.url) - notify('resources.podcast.notifications.urlCopied') + navigator.clipboard + .writeText(record.url) + .then(() => notify('resources.podcast.notifications.urlCopied')) + .catch(() => notify('resources.podcast.notifications.urlCopyFailed', 'warning')) } return ( diff --git a/ui/src/podcast/PodcastShow.jsx b/ui/src/podcast/PodcastShow.jsx index 2a462effc..3e7c95e4d 100644 --- a/ui/src/podcast/PodcastShow.jsx +++ b/ui/src/podcast/PodcastShow.jsx @@ -124,10 +124,9 @@ const PodcastShow = (props) => { useEffect(loadEpisodes, [record?.id]) - // Subscribe to SSE progress only while episodes are downloading - const hasDownloading = episodes.some((ep) => ep.status === 'downloading') + // Stay subscribed to SSE progress while the view is mounted: a download + // started from here may not be flagged 'downloading' yet when we refresh. useEffect(() => { - if (!hasDownloading) return const handler = (e) => { const { episodeId, downloadedBytes, size, duration, status } = e.detail if (status === 'completed' || status === 'error') { @@ -145,7 +144,7 @@ const PodcastShow = (props) => { } window.addEventListener('podcastEpisodeProgress', handler) return () => window.removeEventListener('podcastEpisodeProgress', handler) - }, [hasDownloading]) + }, [record?.id]) if (!record) return null