Preserve cover art when concatenating audio files#205
Conversation
Extract cover art from the first source file before concatenation and re-embed it into the result using an ID3v2 APIC frame. Both extraction and embedding fail gracefully so concatenation still succeeds when no cover art is present. https://claude.ai/code/session_01YcR3uyXEmKKEnAagvuvCkM
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81e3eac343
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| // extract cover art from the first source file before concatenation | ||
| var coverArtPath string | ||
| if artPath, artErr := svc.mediaProcessor.ExtractCoverArt(downloadCtx, fsFilepaths[0]); artErr != nil { |
There was a problem hiding this comment.
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 👍 / 👎.
|
|
||
| tag.AddAttachedPicture(id3v2.PictureFrame{ | ||
| Encoding: id3v2.EncodingUTF8, | ||
| MimeType: "image/jpeg", |
There was a problem hiding this comment.
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 👍 / 👎.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
| defer func() { _ = tag.Close() }() | ||
|
|
||
| artwork, err := os.ReadFile(coverArtPath) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 23 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 validatecoverArtPath, then open the file and read from the handle instead of usingos.ReadFiledirectly.
No other files need changes for this particular sink.
| @@ -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 | ||
| } | ||
|
|
… available The test depends on ffmpeg to generate test MP3 files. In environments where ffmpeg is not installed, the test fails with a confusing error. Add an exec.LookPath guard to skip gracefully. https://claude.ai/code/session_01YcR3uyXEmKKEnAagvuvCkM
Instead of skipping TestConcatenate_OutputSizeNotLargerThanInputs when ffmpeg is missing, install ffmpeg via apt in the test workflow so the test actually exercises the bitrate-matching logic. https://claude.ai/code/session_01YcR3uyXEmKKEnAagvuvCkM
3 participants
Extract cover art from the first source file before concatenation and
re-embed it into the result using an ID3v2 APIC frame. Both extraction
and embedding fail gracefully so concatenation still succeeds when no
cover art is present.
https://claude.ai/code/session_01YcR3uyXEmKKEnAagvuvCkM