fix(security): harden media decoding, file actions, updater and Windows open-in-place

[Lolle2000la]

Summary

Security hardening from a full audit of malformed-media, filesystem and update-supply-chain attack surfaces, followed by two review rounds (Copilot PR reviews — regular and suppressed comments — plus manual GUI testing). 30 commits, all SSH-signed. The main exploits have regression tests; the accompanying PoC suite (poc-suite.zip, attached) documents and reproduces each vulnerability against the pre-fix code.

Findings fixed

Sev	Finding	Fix
Critical	is_animated_gif decoded GIF frames with Limits::no_limits() on the UI thread — a 15-byte GIF claiming 65535² forced a ~16 GiB allocation	header-only scan counting image descriptors; no allocation
High	turbojpeg allocated the scaled buffer from SOF header dims (268 MB per tiny JPEG, ×parallel grid)	16384px dimension cap + checked_mul before allocating
High	no image::Limits anywhere; crafted PNG/GIF/WebP headers drove GB-scale transient allocations (incl. audio cover art)	image_decode_limits() (16384px / 256 MiB) on every decode path, via shared decode_path_with_limits/decode_bytes_with_limits entry points; turbojpeg max_alloc guard at the allocation site
Med	MP4 box-walker u64 overflow → infinite loop (release) / panic (debug); re-parsed the container every second during playback	checked_add + EOF bound + 10k box cap; rotation detected once per load + one-shot property-only re-probe (zero file I/O)
Med	Move/Copy/Rename of a symlink acted on the link's TARGET (canonicalize); Move silently overwrote existing destinations	refuse symlink sources (SourceIsSymlink, checked before AND after canonicalize to close the swap window), TargetExists enforced at construction and inside execute()/rollback(), CopyAction re-checks the source immediately before fs::copy, Windows trash keeps the link, status banner + i18n for refusals
Med	ffmpeg subprocess had no timeout; --prefixed names misparsed options	30s timeout + kill, probesize/analyzeduration bounds, guard (incl. non-UTF8 dash-prefixed paths via raw OS-byte check)
Med (Win)	cmd /C start "" <path> fed unquoted metacharacters to cmd's re-tokenizer	open crate with shellexecute-on-windows (ShellExecuteExW — no cmd layer); verified on a Windows VM
High (Win, supply chain)	velopack extracted the nupkg's Squirrel.exe over live Update.exe before PGP verification; a failed update left a persistent malicious updater	download + size-check + PGP-verify the package ourselves first; velopack's download_updates then early-returns (verified content only); re-verify before apply; delta file names validated; CI signing action pinned to commit SHA

Post-audit review rounds

Every valid review comment (regular and suppressed) was fixed; claims that were empirically wrong were compile-verified and rejected with rationale.

Updater (supply chain, further hardened)

• Staged, verify-first download: package and .sig land at per-run unique temp paths and are PGP-verified against the staged files before being promoted onto the final *.nupkg/*.nupkg.sig paths. An unverified package can no longer sit at a final path — previously a crash between write and verification (or a transient signature failure) clobbered a previously staged, verified package or triggered a whole-dir purge at next startup.
• Streamed download: the package streams chunk-by-chunk into its staged temp (expected-size/cap enforced incrementally, then fsynced) — never buffered whole, so a near-1 GiB package does not cost 1 GiB of RAM.
• Promotion rollback: if the signature rename fails after the package rename, the package rename is rolled back — error paths end with neither file promoted (a new .nupkg next to a stale .sig would be a mismatched pair that startup verification purges the whole dir for).
• Whole-packages-dir purges happen only on PGP verification failures; transient failures clean up only this run's uniquely-named temp artifacts.
• File-name/version validation rejects URL-reserved characters (%, #, ?) and whitespace, not just path traversal.

Settings persistence

• atomic_write now derives a unique temp name (pid + counter) per write, so two app instances cannot truncate each other's in-flight config temp and rename a partial/corrupt file into place; the temp is removed on a failed rename instead of littering the config dir.

Media decoding & thumbnails

• Metadata "Dimensions" read header-only (decode_image_dimensions) — ~1900× faster, no full pixel decode.
• Typed ThumbnailError (Decode vs Transient): queue/ffmpeg timeouts no longer permanently badge slow-but-valid videos as failed; real decode failures still get the persistent badge.

Video (mpv-utils)

• rotate_rgba R0 fast-path slices its result to the declared frame size (no oversized pooled buffer leaking into the frame) and skips the allocation entirely; frame-size math is checked, undersized sources rejected even for R0.

GUI bug fixes (manual testing)

• Folder-switch panic: with a card selected, switching from a folder with thousands of entries to a small one panicked (range start index 4399 out of range for slice of length 8) because the first render after the switch sliced the new, smaller list with the old folder's scroll snapshot. The virtualization math is now the shared, unit-tested viewport_window helper (used by both the grid view and the thumbnail tracker), which clamps both bounds to the list length; open_folder also resets the scroll snapshot. Search queries shrinking the list are covered too.
• Create-folder now uses create_dir and surfaces an existing folder via the status-target-exists banner instead of silently reporting success on a no-op.

Housekeeping

• Comments cleaned up across all crates (refactor: improve comments).

Verification

• cargo build --workspace && cargo test --workspace — all green (21 suites, 420 tests; GUI updater suite with the velopack feature: 171 tests; benchmarks crate: 21 tests incl. correctness tests for every variant)
• cargo clippy --workspace --all-targets -- -D warnings, cargo fmt --all -- — clean (enforced by pre-commit hooks on every commit)
• cargo bench -p benchmarks re-run — no regressions (GIF header scan is strictly cheaper than decode)
• Windows VM: old cmd /C start construction vs new ShellExecuteExW — hostile &-filenames inert after the fix
• poc-suite.zip contains payload generators, the standalone harness, and the Windows verification scripts used during the audit

Note

One original audit claim was refined during verification: cmd /C start "" <path>&cmd does not execute the trailing command (start swallows the rest of the line — verified empirically), but %VAR% expansion and any non-start first command remain dangerous, so the cmd layer was removed entirely.

poc-suite.zip

[Lolle2000la]

PoC suite

The suite is attached in the PR description: poc-suite.zip

Contents: payload generators (GIF/JPEG/MP4 bombs, malicious nupkg), the standalone harness with vendored pre-fix code paths, the Windows VM verification scripts, and poc/README.md with the full findings/fixes matrix.

[Copilot]

Pull request overview

This PR delivers broad security hardening across media decoding, filesystem actions, the updater supply chain, and external-open behavior (notably on Windows), with added regression tests for key exploit classes.

Changes:

• Add decode-time resource limits and header-only parsing paths to prevent allocation/CPU bombs (GIF/JPEG/PNG/MP4, ffmpeg pipe).
• Refuse symbolic-link sources in file actions and surface refusals via a transient in-app status banner (with i18n strings).
• Harden the velopack update flow by downloading + size-checking + PGP-verifying before velopack extraction, and pin the GPG import action by commit SHA.

Reviewed changes

Copilot reviewed 34 out of 36 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
resources/locale/ja/main.ftl	Adds localized status banner strings for new refusal/hardening UX.
resources/locale/en/main.ftl	Adds localized status banner strings for new refusal/hardening UX.
resources/locale/de/main.ftl	Adds localized status banner strings for new refusal/hardening UX.
crates/mpv-utils/src/worker.rs	Hardens RGBA rotation against degenerate/overflow dimensions.
crates/mpv-utils/src/rotation.rs	Hardens MP4 box walking and adds overflow regression test.
crates/mpv-utils/src/mpv_context.rs	Adjusts render callback drop ordering to reduce UAF risk window.
crates/media-sort-gui/src/view/overlay.rs	Adds non-blocking transient status toast UI element.
crates/media-sort-gui/src/view/main_layout.rs	Renders status toast in the main view stack.
crates/media-sort-gui/src/updater.rs	Reorders update download/verify vs velopack extraction; validates filenames; adds purge helper.
crates/media-sort-gui/src/update/tasks.rs	Applies image decode limits for audio cover; replaces Windows cmd-open with open crate; hardens Linux reveal URI encoding.
crates/media-sort-gui/src/update/folder.rs	Validates create-folder names with shared rename validation + status banner.
crates/media-sort-gui/src/update/drag_drop.rs	Refuses symlinks on drop and reports refusals via status banner.
crates/media-sort-gui/src/update.rs	Adds status banner expiry handling on Tick.
crates/media-sort-gui/src/subscriptions/prefetch.rs	Bounds thumbnail work queues and adds recv timeouts to prevent hangs/unbounded growth.
crates/media-sort-gui/src/state/media_errors.rs	Caps tracked decode errors to bound memory growth.
crates/media-sort-gui/src/state.rs	Adds transient status banner state + helper to set it.
crates/media-sort-gui/Cargo.toml	Adds open dependency for safer cross-platform “open externally”.
crates/media-sort-core/src/settings/store.rs	Makes config save atomic and attempts to preserve symlinked config paths.
crates/media-sort-core/src/path_utils.rs	Scopes EXDEV detection to unix platforms.
crates/media-sort-core/src/actions/reversible.rs	Introduces symlink-source rejection helper and error variant.
crates/media-sort-core/src/actions/rename_action.rs	Validates ./.. and rejects symlink sources.
crates/media-sort-core/src/actions/move_action.rs	Rejects symlink sources and refuses overwriting existing destination; adds security tests.
crates/media-sort-core/src/actions/copy_action.rs	Rejects symlink sources.
crates/media-sort-backend/tests/thumbnail_tests.rs	Adds regression test for turbojpeg SOF dimension bomb.
crates/media-sort-backend/tests/metadata_tests.rs	Adds regression test for GIF bomb handling via header scan.
crates/media-sort-backend/src/media/thumbnail.rs	Applies decode limits when decoding embedded audio cover art.
crates/media-sort-backend/src/media/image_decoder.rs	Adds shared decode limits + header-only GIF animation scan + sub-block skipper.
crates/media-sort-backend/src/media/format_pipeline.rs	Applies decode limits across format pipeline + adds JPEG dimension cap and checked allocation math.
crates/media-sort-backend/src/media/ffmpeg_pipe.rs	Adds ffmpeg timeout, option-guard for - names, probe bounds, and PNG decode limits.
crates/media-sort-backend/src/filesystem/trash.rs	Avoids canonicalizing symlinks on Windows trash to prevent trashing link targets.
crates/iced-mpv/src/widget/shader.rs	Adds buffer-size validation before GPU upload to avoid wgpu panics.
crates/iced-mpv/src/state.rs	Validates FrameReady buffer length vs dimensions to avoid panics.
Cargo.lock	Locks new dependency graph entries (notably open and its deps).
AGENTS.md	Documents new security invariants and architectural details.
.gitignore	Ignores PoC suite artifacts and harness target directory.
.github/workflows/release.yml	Pins GPG import GitHub Action to a specific commit SHA.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 34 out of 37 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

crates/media-sort-backend/src/media/ffmpeg_pipe.rs:132

• On the ffmpeg timeout path, the function returns without joining stdout_thread / stderr_thread, detaching threads per timeout. If multiple corrupt inputs time out, this can briefly accumulate many orphan threads and extra resource usage.

            None if std::time::Instant::now() >= deadline => {
                let _ = child.kill();
                let _ = child.wait();
                return Err(format!("ffmpeg timed out after {FFMPEG_TIMEOUT:?}"));
            }

crates/media-sort-backend/src/media/image_decoder.rs:153

• skip_sub_blocks uses SeekFrom::Current for every sub-block. This runs on the UI thread (via poll_background_channels calling is_animated_gif) and can turn a crafted GIF with many tiny sub-blocks into a large number of OS seeks / syscalls and visible UI stalls.

    loop {
        chunks += 1;
        if chunks > 1_000_000 {
            return false;
        }

crates/media-sort-gui/src/update/drag_drop.rs:80

• Building names by collecting and joining all symlink file names is unbounded. Drag-dropping many symlinks can allocate a very large string and render an extremely wide toast, impacting UI responsiveness.

                let names = symlink_paths
                    .iter()
                    .filter_map(|p| p.file_name().map(|n| n.to_string_lossy().into_owned()))
                    .collect::<Vec<_>>()
                    .join(", ");

[Copilot]

Pull request overview

Copilot reviewed 34 out of 37 changed files in this pull request and generated no new comments.

Suppressed comments (1)

crates/mpv-utils/src/worker.rs:101

• rotate_rgba allocates the destination buffer solely from the provided dimensions. If a caller ever passes inconsistent inputs (e.g. hostile/buggy src_w/src_h with a much smaller src slice), this will allocate dst_w*dst_h*4 bytes even though there isn't enough source data to justify that size. Consider validating src.len() against src_w*src_h*4 and bailing out before allocating when the buffer is undersized.

[Copilot]

Pull request overview

Copilot reviewed 34 out of 37 changed files in this pull request and generated no new comments.

Suppressed comments (4)

crates/media-sort-gui/src/updater.rs:304

• The fallback path after a failed rename always tries remove_file(package_path). If fs::rename failed for a reason other than “destination exists” (e.g. permission/IO error) and the destination file is not present, remove_file returns NotFound and masks the original error. It can also turn a transient rename failure into a hard failure even though a second rename attempt might have worked without deleting anything.

            match fs::rename(&partial_path, &package_path) {
                Ok(()) => Ok(()),
                Err(_) => {
                    fs::remove_file(&package_path).map_err(|e| e.to_string())?;
                    fs::rename(&partial_path, &package_path).map_err(|e| e.to_string())
                }
            }

crates/media-sort-gui/src/updater.rs:277

• response.bytes().await reads the entire .nupkg into memory before writing it to disk. Update packages can be large, so this can cause significant peak RSS (or OOM) during updates. Consider streaming the response body directly into the .partial file (while enforcing the expected size / a hard cap) instead of buffering the whole package in RAM.

    let package_bytes = response.bytes().await.map_err(|e| e.to_string())?;
    if package_bytes.len() as u64 != info.TargetFullRelease.Size {
        purge_packages_dir(&packages_dir).await;

crates/media-sort-core/src/settings/store.rs:151

• In the symlink-save branch, read_link failures fall back to path.clone(). If that happens (race, permission issue, etc.), the subsequent temp-file rename will replace the symlink with a regular file — the opposite of what the surrounding comment promises. It’s safer to propagate the read_link error so we never clobber the user’s symlink.

            let link_target = std::fs::read_link(&path).unwrap_or_else(|_| path.clone());

crates/media-sort-gui/src/subscriptions/prefetch.rs:181

• On response_rx.recv_timeout(FFMPEG_RESPONSE_TIMEOUT) timeout (and also on a disconnected ffmpeg worker pool), the function returns an error immediately and never attempts the mpv fallback. That makes the thumbnail pipeline less resilient: if ffmpeg hangs on a particular file (or the pool is dead), mpv may still be able to extract a frame, but we won’t try it.

            Err(std::sync::mpsc::RecvTimeoutError::Timeout) => {
                return Err(format!(
                    "ffmpeg thumbnail request timed out after {FFMPEG_RESPONSE_TIMEOUT:?}"
                ));
            }

[Copilot]

Pull request overview

Copilot reviewed 34 out of 37 changed files in this pull request and generated no new comments.

Suppressed comments (1)

crates/media-sort-gui/src/updater.rs:304

• In the atomic package write path, if fs::rename(partial, package) fails for a reason other than an existing destination (e.g. transient Windows error), the fallback unconditionally calls fs::remove_file(package_path) and propagates its error. If the destination file does not exist, this returns NotFound and aborts the update even though a simple retry of rename would be fine.

                Ok(()) => Ok(()),
                Err(_) => {
                    fs::remove_file(&package_path).map_err(|e| e.to_string())?;
                    fs::rename(&partial_path, &package_path).map_err(|e| e.to_string())
                }

[Copilot]

Pull request overview

Copilot reviewed 34 out of 37 changed files in this pull request and generated no new comments.

Suppressed comments (2)

crates/media-sort-gui/src/update/drag_drop.rs:88

• The toast text is intended to be capped, but this currently collects all symlink file names into a Vec before truncating to MAX_SHOWN_NAMES. A drop containing thousands of symlinks can still allocate an unbounded amount of memory here.

Only collect/join up to MAX_SHOWN_NAMES names and use symlink_paths.len() to decide whether to append an ellipsis.

                let names = symlink_paths
                    .iter()
                    .filter_map(|p| p.file_name().map(|n| n.to_string_lossy().into_owned()))
                    .collect::<Vec<_>>();
                let names = if names.len() > MAX_SHOWN_NAMES {

crates/media-sort-gui/src/updater.rs:279

• The comment says this avoids buffering the whole response, but the implementation still accumulates the entire .nupkg in package_bytes before writing it to disk. This can mislead future readers about peak RSS behavior for large packages.

Update the comment to reflect the current behavior (incremental read with size caps, but full in-memory accumulation), or stream directly to the temp file if keeping RSS low is a requirement.

    // Stream the package into memory with a hard cap instead of buffering
    // the whole response: the feed controls content length, so a lying or
    // compromised feed must not be able to drive unbounded RSS. The
    // expected size is also enforced incrementally (abort as soon as the
    // download exceeds it) and once more at the end.

[Copilot]

Pull request overview

Copilot reviewed 34 out of 37 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 34 out of 37 changed files in this pull request and generated no new comments.

Suppressed comments (3)

crates/media-sort-backend/src/media/image_decoder.rs:130

• After the change to early-return on trailer, the function should return None when the scan ends for reasons other than a valid GIF trailer (e.g., malformed input, block cap exceeded, unexpected EOF). Returning None preserves the original contract of Option<bool> (unknown/unparseable vs. known-static).

    }
    Some(images >= 2)

crates/media-sort-backend/src/media/image_decoder.rs:85

• is_animated_gif_reader currently returns Some(false) when the scan terminates due to malformed/truncated data (any non-trailer break), which conflates “static GIF” with “could not parse GIF”. That can cause callers to treat a corrupt/unknown GIF as a non-animated GIF. Consider returning None unless the scan reaches the GIF trailer (0x3B) or positively detects a second image descriptor.

This issue also appears on line 129 of the same file.

        match block[0] {
            0x3B => break, // trailer: end of image data
            0x2C => {

crates/media-sort-core/src/actions/move_action.rs:214

• The temp directory helper for the security tests uses a timestamp-derived rand() (subsec_nanos) which can collide when called multiple times in quick succession, making these tests potentially flaky due to shared directories/files. Prefer a monotonic per-process counter (or tempfile) to guarantee uniqueness.

    fn temp_subdir() -> std::path::PathBuf {
        let dir = temp_dir().join(format!("sub-{}", rand()));
        std::fs::create_dir_all(&dir).ok();
        dir
    }

[Copilot]

Pull request overview

Copilot reviewed 34 out of 37 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 38 out of 40 changed files in this pull request and generated no new comments.

Suppressed comments (1)

crates/media-sort-backend/src/media/ffmpeg_pipe.rs:72

• The --prefixed-path guard only checks path.to_str(), so on Unix a non-UTF8 path starting with - will bypass the check and still be interpreted by ffmpeg as an option. Since this is a security hardening guard, it should operate on the raw OS string on Unix.

    if path.to_str().is_some_and(|s| s.starts_with('-')) {
        return Err(format!(
            "cannot extract frame: path {:?} starts with '-'",
            path
        ));

[Copilot]

Pull request overview

Copilot reviewed 38 out of 40 changed files in this pull request and generated no new comments.

Suppressed comments (4)

crates/mpv-utils/src/worker.rs:509

• The late-rotation recheck block claims it is “property-only” and does “no file I/O”, but the paths_match guard calls current_p.canonicalize(), which performs filesystem I/O. This reintroduces per-load file access in the tick path and contradicts the comment.
  crates/media-sort-gui/src/updater.rs:190
• validate_release_file_name is used to build a GitHub release download URL later, but it doesn’t reject URL-reserved characters like %, #, or ? (or whitespace). A feed-controlled filename containing %2F could be interpreted as a path separator by the HTTP layer/server. Consider rejecting these characters here (or percent-encoding path segments when constructing URLs).
  crates/media-sort-gui/src/updater.rs:212
• validate_version blocks path separators and whitespace, but still allows URL-reserved characters like %, #, and ?. Since version is interpolated into the download URL, allowing %2F-style sequences can cause the HTTP request path to differ from the intended literal version string.
  crates/mpv-utils/src/worker.rs:117
• rotate_rgba always allocates dst (and computes dst_size) even when rotation is Rotation::R0, but the R0 match arm returns src.to_vec() and discards the allocation. This doubles the per-frame allocation/copy cost for the common no-rotation case.

[Copilot]

Pull request overview

Copilot reviewed 38 out of 40 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

crates/mpv-utils/src/worker.rs:113

• In the Rotation::R0 fast-path, src.to_vec() copies the entire slice, not just the validated src_w * src_h * 4 frame. If a caller ever passes a buffer larger than the frame (e.g. a pooled buffer with extra capacity/unused tail), this returns an oversized RGBA buffer that no longer matches the returned (src_w, src_h) dimensions, which can break downstream consumers that assume len == w*h*4. Since you already computed src_size, slice to that exact length for the R0 case.

[Copilot]

Pull request overview

Copilot reviewed 38 out of 40 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 38 out of 40 changed files in this pull request and generated no new comments.

Suppressed comments (3)

crates/media-sort-core/src/settings/store.rs:166

• The temp path for the atomic config write is always <config>.toml.tmp. If two Media Sort instances save concurrently (or the config is saved from two threads in the future), they can race on the same temp file and produce lost/partial writes. Using a per-process-unique temp filename (at least including pid) avoids cross-process collisions while keeping the rename atomic within the destination directory.

        let tmp = target.with_extension("toml.tmp");
        path_utils::atomic_write(&tmp, &target, data.as_bytes()).map_err(SettingsError::Io)?;

crates/media-sort-gui/src/update/folder.rs:143

• SubmitCreate uses std::fs::create_dir_all for a single-level folder name. create_dir_all succeeds when the directory already exists, so the UI treats "create" as successful even though nothing changed and no conflict is surfaced to the user. Using create_dir and reporting AlreadyExists via the existing status-target-exists banner makes the outcome unambiguous.

                        let new_dir = parent.join(&folder_name);
                        if let Err(e) = std::fs::create_dir_all(&new_dir) {
                            tracing::error!("Failed to create folder: {e}");
                        } else if state.folder.current_folder.is_some() {
                            state.build_folder_tree();
                        }

crates/media-sort-gui/src/updater.rs:471

• The update package is fully buffered into memory (collect_capped -> Vec<u8>) before being written to disk. Even with the 1 GiB hard cap, this can cause large RSS spikes or OOM crashes on lower-memory machines during updates. A safer approach is to stream chunks directly into the staged temp file while enforcing the same expected/cap limits (and then verify the signature against the staged file), avoiding the full in-memory buffer.

[Copilot]

Pull request overview

Copilot reviewed 39 out of 41 changed files in this pull request and generated no new comments.

Suppressed comments (2)

crates/media-sort-gui/src/updater.rs:351

• promote_staged_package renames the package into its final path before renaming the signature. If the signature rename fails, this returns Err but can leave the packages dir in an inconsistent state (new .nupkg promoted while the matching .sig is still missing/at a temp path). That can break later verification/apply logic and makes failure recovery harder. Consider promoting with a rollback strategy (or backing up existing finals) so you either end up with both files in place or neither.
  crates/media-sort-core/src/path_utils.rs:81
• atomic_write leaves the uniquely-named temp file behind if the final rename fails. Over time this can litter the config directory with *.tmp.<pid>.<n> files and may confuse users or tooling. Consider removing the temp file on rename failure.

pub fn atomic_write(dest: &Path, bytes: &[u8]) -> io::Result<()> {
    let tmp = unique_temp_path(dest, "tmp");
    {
        let mut file = std::fs::File::create(&tmp)?;
        file.write_all(bytes)?;
        file.sync_all()?;
    }
    std::fs::rename(&tmp, dest)
}

[Copilot]

Pull request overview

Copilot reviewed 39 out of 41 changed files in this pull request and generated 1 comment.