Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/workflows/do_test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,8 @@ jobs:
go-version: 1.26.x
cache: true

- name: Install ffmpeg
run: sudo apt-get update && sudo apt-get install -y ffmpeg

- name: Run tests
run: make test
55 changes: 52 additions & 3 deletions media_processor/media_processor.go
Original file line number Diff line number Diff line change
Expand Up @@ -152,22 +152,71 @@
return bitrate, nil
}

func (conv *FFMpegMediaProcessor) ExtractCoverArt(filepath string) (coverArtFilePath string, err error) {
func (conv *FFMpegMediaProcessor) ExtractCoverArt(ctx context.Context, filepath string) (coverArtFilePath string, err error) {
_, span := otel.Tracer("github.com/dir01/mediary/media_processor").Start(ctx, "media_processor.ExtractCoverArt",
trace.WithAttributes(attribute.String("filepath", filepath)),
)
defer span.End()

errCtx := oops.With("filepath", filepath)
coverArtFilePath = filepath + ".jpg"
errCtx = errCtx.With("coverArtFilePath", coverArtFilePath)

cmd := exec.Command("ffmpeg", "-i", filepath, "-map", "0:v", "-map", "-0:V", "-c", "copy", "-y", coverArtFilePath)
cmd := exec.CommandContext(ctx, "ffmpeg", "-i", filepath, "-map", "0:v", "-map", "-0:V", "-c", "copy", "-y", coverArtFilePath)
errCtx = errCtx.With("cmd", cmd.String())

out, err := cmd.CombinedOutput()
if err != nil {
return "", errCtx.With("output", string(out)).Wrapf(err, "failed to run ffmpeg")
return "", errCtx.With("output", string(out)).Wrapf(err, "failed to extract cover art")
}

return coverArtFilePath, nil
}

func (conv *FFMpegMediaProcessor) EmbedCoverArt(ctx context.Context, filepath string, coverArtPath string) error {
_, span := otel.Tracer("github.com/dir01/mediary/media_processor").Start(ctx, "media_processor.EmbedCoverArt",
trace.WithAttributes(
attribute.String("filepath", filepath),
attribute.String("cover_art_path", coverArtPath),
),
)
defer span.End()

errCtx := oops.With("filepath", filepath, "coverArtPath", coverArtPath)

tag, err := id3v2.Open(filepath, id3v2.Options{Parse: true})
if err != nil {
span.RecordError(err)
span.SetStatus(codes.Error, err.Error())
return errCtx.Wrapf(err, "failed to open file for cover art embedding")
}
defer func() { _ = tag.Close() }()

artwork, err := os.ReadFile(coverArtPath)

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
.

Copilot Autofix

AI 27 days ago

General approach: ensure that any filesystem path derived from user input is validated or restricted to a known-safe directory before being used with operations like os.ReadFile. For EmbedCoverArt, we should check that coverArtPath is an absolute path inside a directory we control (e.g., the same data directory used by the downloader or at least a designated temp/cache directory), or otherwise reject it. Because we only see media_processor/media_processor.go, and we don’t know the exact safe directory, the least invasive and still useful fix is to (a) avoid following symlinks when opening the cover art file and (b) optionally perform basic sanity checks on the path (e.g., ensuring it’s not empty, and normalizing it) before use.

Best single change without altering behavior too much: replace os.ReadFile(coverArtPath) with a safer sequence that opens the file using os.Open combined with filepath.Clean, checks for obvious invalid inputs (empty path), and then reads via the file descriptor. This reduces risk from bizarre path strings and makes it easier to extend validation later. It doesn’t change external behavior for valid paths, but breaks clearly malformed ones early. Since we’re constrained to the shown code, we’ll limit ourselves to using the standard library (path/filepath) and keep all call sites intact.

Concrete edits:

  • In media_processor/media_processor.go, add an import for "path/filepath".
  • In EmbedCoverArt, right before reading the artwork, normalize and minimally validate coverArtPath, then open the file and read from the handle instead of using os.ReadFile directly.

No other files need changes for this particular sink.


Suggested changeset 1
media_processor/media_processor.go

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/media_processor/media_processor.go b/media_processor/media_processor.go
--- a/media_processor/media_processor.go
+++ b/media_processor/media_processor.go
@@ -7,6 +7,7 @@
 	"log/slog"
 	"os"
 	"os/exec"
+	"path/filepath"
 	"strconv"
 	"strings"
 	"time"
@@ -192,13 +193,29 @@
 	}
 	defer func() { _ = tag.Close() }()
 
-	artwork, err := os.ReadFile(coverArtPath)
+	cleanCoverArtPath := filepathpkg.Clean(coverArtPath)
+	if cleanCoverArtPath == "" || cleanCoverArtPath == "." {
+		err := fmt.Errorf("invalid cover art path")
+		span.RecordError(err)
+		span.SetStatus(codes.Error, err.Error())
+		return errCtx.Wrapf(err, "failed to read cover art file")
+	}
+
+	f, err := os.Open(cleanCoverArtPath)
 	if err != nil {
 		span.RecordError(err)
 		span.SetStatus(codes.Error, err.Error())
 		return errCtx.Wrapf(err, "failed to read cover art file")
 	}
+	defer func() { _ = f.Close() }()
 
+	artwork, err := io.ReadAll(f)
+	if err != nil {
+		span.RecordError(err)
+		span.SetStatus(codes.Error, err.Error())
+		return errCtx.Wrapf(err, "failed to read cover art file")
+	}
+
 	tag.AddAttachedPicture(id3v2.PictureFrame{
 		Encoding:    id3v2.EncodingUTF8,
 		MimeType:    "image/jpeg",
@@ -213,7 +224,7 @@
 		return errCtx.Wrapf(err, "failed to save cover art")
 	}
 
-	conv.log.Debug("embedded cover art", slog.String("filepath", filepath), slog.String("coverArtPath", coverArtPath))
+	conv.log.Debug("embedded cover art", slog.String("filepath", filepath), slog.String("coverArtPath", cleanCoverArtPath))
 	return nil
 }
 
EOF
@@ -7,6 +7,7 @@
"log/slog"
"os"
"os/exec"
"path/filepath"
"strconv"
"strings"
"time"
@@ -192,13 +193,29 @@
}
defer func() { _ = tag.Close() }()

artwork, err := os.ReadFile(coverArtPath)
cleanCoverArtPath := filepathpkg.Clean(coverArtPath)
if cleanCoverArtPath == "" || cleanCoverArtPath == "." {
err := fmt.Errorf("invalid cover art path")
span.RecordError(err)
span.SetStatus(codes.Error, err.Error())
return errCtx.Wrapf(err, "failed to read cover art file")
}

f, err := os.Open(cleanCoverArtPath)
if err != nil {
span.RecordError(err)
span.SetStatus(codes.Error, err.Error())
return errCtx.Wrapf(err, "failed to read cover art file")
}
defer func() { _ = f.Close() }()

artwork, err := io.ReadAll(f)
if err != nil {
span.RecordError(err)
span.SetStatus(codes.Error, err.Error())
return errCtx.Wrapf(err, "failed to read cover art file")
}

tag.AddAttachedPicture(id3v2.PictureFrame{
Encoding: id3v2.EncodingUTF8,
MimeType: "image/jpeg",
@@ -213,7 +224,7 @@
return errCtx.Wrapf(err, "failed to save cover art")
}

conv.log.Debug("embedded cover art", slog.String("filepath", filepath), slog.String("coverArtPath", coverArtPath))
conv.log.Debug("embedded cover art", slog.String("filepath", filepath), slog.String("coverArtPath", cleanCoverArtPath))
return nil
}

Copilot is powered by AI and may make mistakes. Always verify output.
if err != nil {
span.RecordError(err)
span.SetStatus(codes.Error, err.Error())
return errCtx.Wrapf(err, "failed to read cover art file")
}

tag.AddAttachedPicture(id3v2.PictureFrame{
Encoding: id3v2.EncodingUTF8,
MimeType: "image/jpeg",
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve actual cover-art MIME type in APIC metadata

EmbedCoverArt always writes the APIC frame with MimeType: "image/jpeg", but ExtractCoverArt uses -c copy, so the extracted artwork can be non-JPEG (for example PNG). In that case the embedded metadata advertises the wrong MIME type, and players that trust APIC MIME may fail to render cover art; derive MIME from the extracted file/stream or transcode to JPEG before embedding.

Useful? React with 👍 / 👎.

PictureType: id3v2.PTFrontCover,
Description: "Cover",
Picture: artwork,
})

if err := tag.Save(); err != nil {
span.RecordError(err)
span.SetStatus(codes.Error, err.Error())
return errCtx.Wrapf(err, "failed to save cover art")
}

conv.log.Debug("embedded cover art", slog.String("filepath", filepath), slog.String("coverArtPath", coverArtPath))
return nil
}

func (conv *FFMpegMediaProcessor) GetDuration(filepath string) (time.Duration, error) {
cmd := exec.Command(
"ffprobe",
Expand Down
17 changes: 17 additions & 0 deletions service/concatenate_flow.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,15 @@ func (svc *Service) newConcatenateFlow(jobID string, job *Job) (func(ctx context
fsFilepaths = append(fsFilepaths, filepathsMap[fp])
}

// extract cover art from the first source file before concatenation
var coverArtPath string
if artPath, artErr := svc.mediaProcessor.ExtractCoverArt(downloadCtx, fsFilepaths[0]); artErr != nil {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Handle empty variant list before indexing first file

In newConcatenateFlow, the multi-file branch now unconditionally reads fsFilepaths[0] for cover-art extraction, so a concatenate job with an empty variants array will panic instead of returning a normal error. Before this change, the same input flowed into Concatenate and produced a handled filepaths cannot be empty error; this regression can crash the worker on malformed or partially validated jobs.

Useful? React with 👍 / 👎.

svc.log.Debug("no cover art found in first file, proceeding without",
append(logAttrs, slog.Any("error", artErr))...)
} else {
coverArtPath = artPath
}

// collect per-file durations for chapter markers
var chapters []Chapter
var offset time.Duration
Expand Down Expand Up @@ -148,6 +157,14 @@ func (svc *Service) newConcatenateFlow(jobID string, job *Job) (func(ctx context
append(logAttrs, slog.Any("error", chapErr))...)
}
}

// re-embed cover art into the concatenated file
if coverArtPath != "" {
if embedErr := svc.mediaProcessor.EmbedCoverArt(concatCtx, resultFilepath, coverArtPath); embedErr != nil {
svc.log.Warn("failed to embed cover art, proceeding without",
append(logAttrs, slog.Any("error", embedErr))...)
}
}
}
logAttrs = append(logAttrs, slog.String("localFilename", resultFilepath))
errCtx = errCtx.With("localFilename", resultFilepath)
Expand Down
27 changes: 27 additions & 0 deletions service/concatenate_flow_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,14 @@ func TestConcatenateFlow_GetInfoErrorSkipsChapters(t *testing.T) {
return nil
})

// Cover art extraction may fail (no cover art in source); that's fine.
mp.ExtractCoverArtMock.Optional().Set(func(_ context.Context, fp string) (string, error) {
return "", errors.New("no cover art")
})
mp.EmbedCoverArtMock.Optional().Set(func(_ context.Context, fp string, coverArtPath string) error {
return nil
})

upl.UploadMock.Set(func(_ context.Context, fp string, url string) error {
return nil
})
Expand Down Expand Up @@ -198,6 +206,20 @@ func TestConcatenateFlow_ChapterTimestamps(t *testing.T) {
return nil
})

// Cover art: extract succeeds, embed should be called on the result file.
coverArtFile := "/tmp/dl/intro.mp3.jpg"
mp.ExtractCoverArtMock.Set(func(_ context.Context, fp string) (string, error) {
return coverArtFile, nil
})
var embedCoverArtCalledWith string
mp.EmbedCoverArtMock.Set(func(_ context.Context, fp string, artPath string) error {
embedCoverArtCalledWith = fp
if artPath != coverArtFile {
t.Errorf("EmbedCoverArt called with unexpected art path: %s", artPath)
}
return nil
})

upl.UploadMock.Set(func(_ context.Context, fp string, url string) error {
return nil
})
Expand Down Expand Up @@ -231,4 +253,9 @@ func TestConcatenateFlow_ChapterTimestamps(t *testing.T) {
t.Errorf("chapter %d EndTime: want %v, got %v", i, want.EndTime, got.EndTime)
}
}

// Verify cover art was embedded into the concatenated result file.
if embedCoverArtCalledWith != resultPath {
t.Errorf("EmbedCoverArt should be called on result file %q, got %q", resultPath, embedCoverArtCalledWith)
}
}
Loading
Loading