mirror of
https://github.com/navidrome/navidrome.git
synced 2026-10-08 10:27:08 +02:00
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 <guid> - Handle clipboard write failure in the podcast list
This commit is contained in:
parent
19385dc28b
commit
4bebfad0ed
10 changed files with 126 additions and 52 deletions
|
|
@ -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)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
||||
// <guid> 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,
|
||||
|
|
|
|||
|
|
@ -77,6 +77,15 @@ var _ = Describe("ParseRSSFeed", func() {
|
|||
Expect(feed.Episodes).To(HaveLen(2))
|
||||
})
|
||||
|
||||
It("falls back to the enclosure URL when <guid> is missing", func() {
|
||||
feed, err := podcasts.ParseRSSFeed([]byte(`<?xml version="1.0"?><rss version="2.0"><channel><title>T</title>
|
||||
<item><title>No guid</title><enclosure url="https://example.com/noguid.mp3" length="1" type="audio/mpeg"/></item>
|
||||
</channel></rss>`))
|
||||
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]
|
||||
|
|
|
|||
|
|
@ -267,6 +267,7 @@
|
|||
"downloadStarted": "다운로드 시작됨",
|
||||
"channelAdded": "채널 추가됨",
|
||||
"urlCopied": "피드 URL이 클립보드에 복사됨",
|
||||
"urlCopyFailed": "피드 URL을 복사할 수 없습니다",
|
||||
"alreadyExists": "이미 등록된 채널입니다.",
|
||||
"channelDeleted": "채널 삭제됨",
|
||||
"episodeDeleted": "에피소드 삭제됨"
|
||||
|
|
|
|||
|
|
@ -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
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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))
|
||||
}
|
||||
})
|
||||
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
|
|
|
|||
|
|
@ -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 (
|
||||
<span style={{ display: 'flex', alignItems: 'center', gap: 4 }}>
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue