mirror of
https://git.sr.ht/~joren/streamrip-go
synced 2026-10-16 22:07:30 +02:00
fix: resource leaks, cancellation, races and path traversal
Six correctness bugs that the existing tests could not see. The suite passed and `go test -race` was clean throughout, because nothing exercised concurrent token refresh, shared-folder artwork, or hostile metadata. main(): extract run() int main() was a single 1196-line function that registered 20 `defer mainApp.Close()` handlers and then called os.Exit 150 times, so none of them ever ran. Close() is what closes the SQLite handle, flushes the progress bars and removes the artwork temp dirs, so every failed rip leaked temp directories. main() is now a wrapper around run() int and the command bodies return exit codes. Ctrl-C now stops a rip ctx was threaded everywhere but never checked: zero ctx.Err()/ctx.Done() in internal/app and internal/download. Cancellation left Rip() iterating the whole remaining queue, and the retry ladder retried on a dead context, so one Ctrl-C cost three doomed attempts per remaining track. Adds checks to the dispatch loops, the worker-spawn loops and the retry ladder, and routes the MarkFailed/MarkDownloaded bookkeeping through a detached context so the records survive cancellation. Convert and TagFLAC now take a ctx and use exec.CommandContext, so a stalled ffmpeg/ffprobe no longer outlives Ctrl-C. Config race Provider token refresh mutated the shared *config.Config and called SaveFile() from inside download workers with no lock, racing the struct fields and issuing concurrent os.WriteFile against one path. Adds a mutex to config.Config plus UpdateTokens(), and routes all five refresh sites through it. Each provider client gets an authMu guarding loggedIn and its login/refresh path; beatport needed it most, since ensureToken is reached from apiRequest. Artwork cover race artwork.Prepare was check-then-write on a path shared by every worker in a playlist rip. Two workers both saw a missing cover.jpg, both created it, and the loser's cleanup deleted the winner's finished file. Adds a per-folder lock and downloads via a unique temp file + rename. The regression test fails against the old code with 15 of 16 workers getting no cover. Path traversal from remote metadata naming.CleanName stripped <>:"/\|?* and control chars but not "." , so a title of ".." survived into filepath.Join and escaped the download root. SoundCloud and Tidal playlist names are user-generated. Now rejects "." and "..", trims trailing dots, handles Windows device names, adds DEL to the invalid set and caps components at 255 bytes without splitting a rune. app.go gains safeJoin as a second line of defence. Credentials no longer world-readable The config holds plaintext provider passwords, Deezer ARL cookies and OAuth tokens and was written 0o644. Now 0o600, with 0o700 directories, and an existing 0o644 file is tightened on write. Also: deletes the 9 duplicated *-rip-smoke dev commands (~300 lines fully subsumed by `rip id`) and the unreachable second dev-help arm; registers image/png so PNG covers are downscaled instead of being silently saved as cover.jpg containing PNG bytes; adds the missing .gitignore, which was keeping a 21MB build artifact one `git add -A` away from the repo. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
19 files changed
+915
-562
No files matched your search
+127
-20
@@ -11,6 +11,7 @@ import (
|
||||
"strconv"
|
||||
"strings"
|
||||
"sync"
|
||||
"time"
|
||||
|
||||
"streamrip-go/internal/artwork"
|
||||
"streamrip-go/internal/audio/convert"
|
||||
@@ -87,7 +88,7 @@ var (
|
||||
)
|
||||
|
||||
type trackTagger interface {
|
||||
TagFLAC(path string, meta tag.Metadata, coverPath string) error
|
||||
TagFLAC(ctx context.Context, path string, meta tag.Metadata, coverPath string) error
|
||||
}
|
||||
|
||||
type videoDownloadableProvider interface {
|
||||
@@ -161,6 +162,79 @@ func (m *Main) logf(format string, args ...any) {
|
||||
fmt.Printf(format, args...)
|
||||
}
|
||||
|
||||
// bookkeepingTimeout bounds the detached store writes below. They must not
|
||||
// inherit a cancelled context, or a single Ctrl-C silently loses the record of
|
||||
// which tracks failed.
|
||||
const bookkeepingTimeout = 5 * time.Second
|
||||
|
||||
// markFailed records a failed download. It deliberately detaches from ctx so
|
||||
// the write still lands when the rip was cancelled, which is exactly when the
|
||||
// failure record matters most.
|
||||
func (m *Main) markFailed(ctx context.Context, source, mediaType, id string) {
|
||||
if m.Store == nil {
|
||||
return
|
||||
}
|
||||
writeCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), bookkeepingTimeout)
|
||||
defer cancel()
|
||||
if err := m.Store.MarkFailed(writeCtx, source, mediaType, id); err != nil {
|
||||
m.logf("warning: could not record failure for %s/%s/%s: %v\n", source, mediaType, id, err)
|
||||
}
|
||||
}
|
||||
|
||||
// markDownloaded records a completed download, detached from ctx for the same
|
||||
// reason as markFailed: the file is already on disk, so losing the record would
|
||||
// cause a needless re-download later.
|
||||
func (m *Main) markDownloaded(ctx context.Context, source, id string) error {
|
||||
if m.Store == nil {
|
||||
return nil
|
||||
}
|
||||
writeCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), bookkeepingTimeout)
|
||||
defer cancel()
|
||||
return m.Store.MarkDownloaded(writeCtx, source, id)
|
||||
}
|
||||
|
||||
// safeJoin joins base with parts and asserts the result stays inside the
|
||||
// configured downloads folder.
|
||||
//
|
||||
// naming.CleanName already rejects "." and ".." components, so this should
|
||||
// never fire; it is a second line of defence because every component here is
|
||||
// built from provider-supplied metadata, and writing outside the download root
|
||||
// is the one failure mode worth being paranoid about.
|
||||
func (m *Main) safeJoin(base string, parts ...string) string {
|
||||
joined := filepath.Join(append([]string{base}, parts...)...)
|
||||
|
||||
root := strings.TrimSpace(m.Config.Session.Downloads.Folder)
|
||||
if root == "" {
|
||||
return joined
|
||||
}
|
||||
if pathWithin(root, joined) {
|
||||
return joined
|
||||
}
|
||||
|
||||
m.logf("warning: refusing to write outside the download folder (%q); using %q instead\n", joined, root)
|
||||
return filepath.Join(root, "Unknown")
|
||||
}
|
||||
|
||||
// pathWithin reports whether target is root itself or sits beneath it.
|
||||
func pathWithin(root, target string) bool {
|
||||
absRoot, err := filepath.Abs(root)
|
||||
if err != nil {
|
||||
return false
|
||||
}
|
||||
absTarget, err := filepath.Abs(target)
|
||||
if err != nil {
|
||||
return false
|
||||
}
|
||||
rel, err := filepath.Rel(absRoot, absTarget)
|
||||
if err != nil {
|
||||
return false
|
||||
}
|
||||
if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) {
|
||||
return false
|
||||
}
|
||||
return true
|
||||
}
|
||||
|
||||
func (m *Main) GetLoggedInProvider(ctx context.Context, source string) (provider.Client, error) {
|
||||
p, ok := m.Providers[source]
|
||||
if !ok {
|
||||
@@ -184,7 +258,7 @@ func (m *Main) AddByID(ctx context.Context, source, mediaType, id string) error
|
||||
ResolveFn: func(ctx context.Context) (media.Media, error) {
|
||||
meta, err := p.GetMetadata(ctx, id, mediaType)
|
||||
if err != nil {
|
||||
_ = m.Store.MarkFailed(ctx, source, mediaType, id)
|
||||
m.markFailed(ctx, source, mediaType, id)
|
||||
return nil, err
|
||||
}
|
||||
|
||||
@@ -346,7 +420,7 @@ func (m *Main) ripTrackCollection(ctx context.Context, p provider.Client, source
|
||||
if m.Config.Session.Downloads.SourceSubdirectories {
|
||||
base = filepath.Join(base, jsonutil.TitleCase(source))
|
||||
}
|
||||
folder := filepath.Join(base, naming.CleanName(name, naming.Config{RestrictCharacters: m.Config.Session.Filepaths.RestrictCharacters, TruncateTo: m.Config.Session.Filepaths.TruncateTo}))
|
||||
folder := m.safeJoin(base, naming.CleanName(name, naming.Config{RestrictCharacters: m.Config.Session.Filepaths.RestrictCharacters, TruncateTo: m.Config.Session.Filepaths.TruncateTo}))
|
||||
tracksMap, ok := meta["tracks"].(map[string]any)
|
||||
if !ok {
|
||||
return fmt.Errorf("%s missing tracks data", strings.ToLower(kind))
|
||||
@@ -388,6 +462,9 @@ func (m *Main) ripTrackCollection(ctx context.Context, p provider.Client, source
|
||||
}
|
||||
if !m.Config.Session.Downloads.Concurrency || m.Config.Session.Downloads.MaxConnections == 1 {
|
||||
for i, trackID := range ids {
|
||||
if ctx.Err() != nil {
|
||||
break
|
||||
}
|
||||
runOne(i+1, trackID)
|
||||
}
|
||||
} else {
|
||||
@@ -399,6 +476,9 @@ func (m *Main) ripTrackCollection(ctx context.Context, p provider.Client, source
|
||||
var wg sync.WaitGroup
|
||||
var mu sync.Mutex
|
||||
for i, trackID := range ids {
|
||||
if ctx.Err() != nil {
|
||||
break
|
||||
}
|
||||
wg.Add(1)
|
||||
sem <- struct{}{}
|
||||
go func(pos int, tid string) {
|
||||
@@ -440,18 +520,18 @@ func (m *Main) ripVideo(ctx context.Context, p provider.Client, source, videoID
|
||||
|
||||
d, err := vp.GetVideoDownloadable(ctx, videoID)
|
||||
if err != nil {
|
||||
_ = m.Store.MarkFailed(ctx, source, "video", videoID)
|
||||
m.markFailed(ctx, source, "video", videoID)
|
||||
return fmt.Errorf("id=%s get_video_downloadable: %w", videoID, err)
|
||||
}
|
||||
|
||||
title := titleFromMetadata(meta, videoID)
|
||||
outPath := m.videoOutputPath(source, videoID, title, d.Extension)
|
||||
if err = m.DL.FileVideo(ctx, d.URL, outPath); err != nil {
|
||||
_ = m.Store.MarkFailed(ctx, source, "video", videoID)
|
||||
m.markFailed(ctx, source, "video", videoID)
|
||||
return fmt.Errorf("id=%s title=%q video download: %w", videoID, title, err)
|
||||
}
|
||||
|
||||
if err = m.Store.MarkDownloaded(ctx, source, videoID); err != nil {
|
||||
if err = m.markDownloaded(ctx, source, videoID); err != nil {
|
||||
return err
|
||||
}
|
||||
return nil
|
||||
@@ -631,11 +711,17 @@ func (m *Main) Rip(ctx context.Context) error {
|
||||
m.logf("Ripping %d media item(s)\n", len(m.Media))
|
||||
failures := 0
|
||||
for _, item := range m.Media {
|
||||
if err := ctx.Err(); err != nil {
|
||||
return err
|
||||
}
|
||||
if err := item.Rip(ctx); err != nil {
|
||||
failures++
|
||||
m.logf("media item failed: %v\n", err)
|
||||
}
|
||||
}
|
||||
if err := ctx.Err(); err != nil {
|
||||
return err
|
||||
}
|
||||
if failures > 0 {
|
||||
return fmt.Errorf("%d media item(s) failed", failures)
|
||||
}
|
||||
@@ -708,6 +794,9 @@ func (m *Main) ripAlbum(ctx context.Context, p provider.Client, source, albumID
|
||||
|
||||
if !m.Config.Session.Downloads.Concurrency || m.Config.Session.Downloads.MaxConnections == 1 {
|
||||
for i, trackID := range trackIDs {
|
||||
if ctx.Err() != nil {
|
||||
break
|
||||
}
|
||||
opts := ripTrackOptions{albumFolder: folder, albumEmbedCover: artRes.EmbedPath, albumArtist: albumArtist, index: i + 1, total: total, albumDiscTotal: discTotal}
|
||||
if i == 0 {
|
||||
opts.prefetched = prefetched
|
||||
@@ -731,6 +820,9 @@ func (m *Main) ripAlbum(ctx context.Context, p provider.Client, source, albumID
|
||||
var wg sync.WaitGroup
|
||||
errCh := make(chan error, len(trackIDs))
|
||||
for i, trackID := range trackIDs {
|
||||
if ctx.Err() != nil {
|
||||
break
|
||||
}
|
||||
wg.Add(1)
|
||||
sem <- struct{}{}
|
||||
go func(idx int, tid string) {
|
||||
@@ -770,7 +862,7 @@ func (m *Main) ripPlaylist(ctx context.Context, p provider.Client, source, playl
|
||||
if m.Config.Session.Downloads.SourceSubdirectories {
|
||||
base = filepath.Join(base, jsonutil.TitleCase(source))
|
||||
}
|
||||
folder := filepath.Join(base, naming.CleanName(name, naming.Config{
|
||||
folder := m.safeJoin(base, naming.CleanName(name, naming.Config{
|
||||
RestrictCharacters: m.Config.Session.Filepaths.RestrictCharacters,
|
||||
TruncateTo: m.Config.Session.Filepaths.TruncateTo,
|
||||
}))
|
||||
@@ -827,6 +919,9 @@ func (m *Main) ripPlaylist(ctx context.Context, p provider.Client, source, playl
|
||||
|
||||
if !m.Config.Session.Downloads.Concurrency || m.Config.Session.Downloads.MaxConnections == 1 {
|
||||
for i, id := range ids {
|
||||
if ctx.Err() != nil {
|
||||
break
|
||||
}
|
||||
runOne(i+1, id)
|
||||
}
|
||||
} else {
|
||||
@@ -838,6 +933,9 @@ func (m *Main) ripPlaylist(ctx context.Context, p provider.Client, source, playl
|
||||
var wg sync.WaitGroup
|
||||
var mu sync.Mutex
|
||||
for i, id := range ids {
|
||||
if ctx.Err() != nil {
|
||||
break
|
||||
}
|
||||
wg.Add(1)
|
||||
sem <- struct{}{}
|
||||
go func(pos int, tid string) {
|
||||
@@ -876,7 +974,7 @@ func (m *Main) ripPlaylistMixed(ctx context.Context, playlistID, name string, re
|
||||
}
|
||||
}
|
||||
|
||||
folder := filepath.Join(m.Config.Session.Downloads.Folder, naming.CleanName(name, naming.Config{
|
||||
folder := m.safeJoin(m.Config.Session.Downloads.Folder, naming.CleanName(name, naming.Config{
|
||||
RestrictCharacters: m.Config.Session.Filepaths.RestrictCharacters,
|
||||
TruncateTo: m.Config.Session.Filepaths.TruncateTo,
|
||||
}))
|
||||
@@ -938,6 +1036,9 @@ func (m *Main) requireSourceDownloadAuth(source string) error {
|
||||
}
|
||||
|
||||
func (m *Main) ripTrack(ctx context.Context, p provider.Client, source, id, fallbackTitle string, opts ripTrackOptions) error {
|
||||
if err := ctx.Err(); err != nil {
|
||||
return err
|
||||
}
|
||||
if !m.IgnoreDB {
|
||||
alreadyDownloaded, err := m.Store.IsDownloaded(ctx, source, id)
|
||||
if err == nil && alreadyDownloaded {
|
||||
@@ -952,7 +1053,7 @@ func (m *Main) ripTrack(ctx context.Context, p provider.Client, source, id, fall
|
||||
|
||||
meta, err := p.GetMetadata(ctx, id, "track")
|
||||
if err != nil {
|
||||
_ = m.Store.MarkFailed(ctx, source, "track", id)
|
||||
m.markFailed(ctx, source, "track", id)
|
||||
return fmt.Errorf("id=%s metadata: %w", id, err)
|
||||
}
|
||||
|
||||
@@ -969,7 +1070,7 @@ func (m *Main) ripTrack(ctx context.Context, p provider.Client, source, id, fall
|
||||
if d == nil {
|
||||
d, err = p.GetDownloadable(ctx, id, m.qualityForSource(source))
|
||||
if err != nil {
|
||||
_ = m.Store.MarkFailed(ctx, source, "track", id)
|
||||
m.markFailed(ctx, source, "track", id)
|
||||
return fmt.Errorf("id=%s title=%q get_downloadable: %w", id, title, err)
|
||||
}
|
||||
}
|
||||
@@ -992,9 +1093,15 @@ func (m *Main) ripTrack(ctx context.Context, p provider.Client, source, id, fall
|
||||
return m.DL.File(ctx, d.URL, outPath)
|
||||
}
|
||||
if err = downloadOnce(); err != nil {
|
||||
// Never spend retries or a provider fallback on a cancelled context:
|
||||
// otherwise one Ctrl-C costs three doomed attempts per remaining track.
|
||||
if ctxErr := ctx.Err(); ctxErr != nil {
|
||||
m.markFailed(ctx, source, "track", id)
|
||||
return fmt.Errorf("id=%s title=%q download: %w", id, title, err)
|
||||
}
|
||||
m.logf("retry: %s (%v)\n", filepath.Base(outPath), err)
|
||||
if err = downloadOnce(); err != nil {
|
||||
if fallbackProvider, ok := p.(trackFallbackDownloader); ok {
|
||||
if fallbackProvider, ok := p.(trackFallbackDownloader); ok && ctx.Err() == nil {
|
||||
m.logf("fallback: %s via provider backup flow\n", filepath.Base(outPath))
|
||||
if fbErr := fallbackProvider.DownloadTrackFallback(ctx, id, m.qualityForSource(source), outPath); fbErr == nil {
|
||||
goto downloaded
|
||||
@@ -1002,7 +1109,7 @@ func (m *Main) ripTrack(ctx context.Context, p provider.Client, source, id, fall
|
||||
m.logf("fallback failed: %s (%v)\n", filepath.Base(outPath), fbErr)
|
||||
}
|
||||
}
|
||||
_ = m.Store.MarkFailed(ctx, source, "track", id)
|
||||
m.markFailed(ctx, source, "track", id)
|
||||
return fmt.Errorf("id=%s title=%q download: %w", id, title, err)
|
||||
}
|
||||
}
|
||||
@@ -1040,24 +1147,24 @@ downloaded:
|
||||
coverPath = tag.CoverPathForTrack(outPath, opts.albumFolder)
|
||||
}
|
||||
}
|
||||
if err = m.Tagger.TagFLAC(outPath, tagMeta, coverPath); err != nil {
|
||||
if err = m.Tagger.TagFLAC(ctx, outPath, tagMeta, coverPath); err != nil {
|
||||
if isFFmpegMissingError(err) {
|
||||
_ = m.Store.MarkFailed(ctx, source, "track", id)
|
||||
m.markFailed(ctx, source, "track", id)
|
||||
return fmt.Errorf("id=%s title=%q tag: %w", id, title, err)
|
||||
}
|
||||
m.logf("warning: tag failed for %s: %v\n", filepath.Base(outPath), err)
|
||||
}
|
||||
|
||||
if m.Config.Session.Conversion.Enabled {
|
||||
convertedPath, convErr := convert.Convert(outPath, m.Config.Session.Conversion)
|
||||
convertedPath, convErr := convert.Convert(ctx, outPath, m.Config.Session.Conversion)
|
||||
if convErr != nil {
|
||||
_ = m.Store.MarkFailed(ctx, source, "track", id)
|
||||
m.markFailed(ctx, source, "track", id)
|
||||
return fmt.Errorf("id=%s title=%q convert: %w", id, title, convErr)
|
||||
}
|
||||
outPath = convertedPath
|
||||
}
|
||||
|
||||
return m.Store.MarkDownloaded(ctx, source, id)
|
||||
return m.markDownloaded(ctx, source, id)
|
||||
}
|
||||
|
||||
func (m *Main) qualityForSource(source string) int {
|
||||
@@ -1206,7 +1313,7 @@ func (m *Main) albumFolderPath(source, albumID, albumTitle, albumArtist, year st
|
||||
TruncateTo: m.Config.Session.Filepaths.TruncateTo,
|
||||
})
|
||||
|
||||
return filepath.Join(base, folderName)
|
||||
return m.safeJoin(base, folderName)
|
||||
}
|
||||
|
||||
func (m *Main) trackOutputPath(source, id, title, ext string, d *provider.Downloadable, trackMeta map[string]any, albumFolder string, albumDiscTotal int) string {
|
||||
@@ -1279,7 +1386,7 @@ func (m *Main) trackOutputPath(source, id, title, ext string, d *provider.Downlo
|
||||
RestrictCharacters: m.Config.Session.Filepaths.RestrictCharacters,
|
||||
TruncateTo: m.Config.Session.Filepaths.TruncateTo,
|
||||
})
|
||||
return filepath.Join(base, fileName+"."+ext)
|
||||
return m.safeJoin(base, fileName+"."+ext)
|
||||
}
|
||||
|
||||
func (m *Main) videoOutputPath(source, id, title, ext string) string {
|
||||
@@ -1297,7 +1404,7 @@ func (m *Main) videoOutputPath(source, id, title, ext string) string {
|
||||
if fileName == "" {
|
||||
fileName = id
|
||||
}
|
||||
return filepath.Join(base, fileName+"."+ext)
|
||||
return m.safeJoin(base, fileName+"."+ext)
|
||||
}
|
||||
|
||||
func titleFromMetadata(meta map[string]any, fallback string) string {
|
||||
|
||||
@@ -20,13 +20,13 @@ import (
|
||||
|
||||
type noopTagger struct{}
|
||||
|
||||
func (n noopTagger) TagFLAC(string, tag.Metadata, string) error { return nil }
|
||||
func (n noopTagger) TagFLAC(context.Context, string, tag.Metadata, string) error { return nil }
|
||||
|
||||
type failingTagger struct {
|
||||
err error
|
||||
}
|
||||
|
||||
func (f failingTagger) TagFLAC(string, tag.Metadata, string) error { return f.err }
|
||||
func (f failingTagger) TagFLAC(context.Context, string, tag.Metadata, string) error { return f.err }
|
||||
|
||||
type fakeProvider struct {
|
||||
url string
|
||||
|
||||
Reference in new issue
Block a user