diff --git a/.coverage b/.coverage new file mode 100644 index 0000000..c8a8c6c Binary files /dev/null and b/.coverage differ diff --git a/.jules/bolt.md b/.jules/bolt.md index 63607c3..348310f 100644 --- a/.jules/bolt.md +++ b/.jules/bolt.md @@ -60,3 +60,6 @@ ## 2026-06-25 - [Optimize Path.exists() when paired with stat()] **Learning:** Checking `Path.exists()` before `Path.stat()` introduces a redundant system call because `exists()` internally uses `stat()`. **Action:** Rely on catching the `OSError` from `Path.stat()` to simultaneously check for existence and retrieve file attributes, saving measurable I/O overhead on large filesystems. +## 2024-05-15 - Fast Absolute Paths for Subprocesses +**Learning:** In Python, `pathlib.Path.resolve()` incurs heavy disk I/O because it traverses the filesystem to resolve symlinks and normalize the path. This can be a significant bottleneck when called repeatedly (e.g. in loops or large batches of files). +**Action:** When passing file paths to command-line tools like `ffmpeg` or `subprocess.run` to ensure they are absolute and avoid argument injection, use `pathlib.Path.absolute()` instead of `.resolve()`. It performs a purely lexical path join (adding the current working directory if relative) and avoids expensive system calls, as the underlying OS syscalls handle symlink resolution automatically anyway. diff --git a/.jules/sentinel.md b/.jules/sentinel.md index 467e052..36349a3 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -36,3 +36,7 @@ **Vulnerability:** Argument Injection via relative paths starting with a hyphen in command-line utilities. **Learning:** Even when `ffmpeg` inputs are protected by `-i`, the output paths, as well as arguments to other utilities like `brctl` and `SetFile`, can be maliciously crafted to start with `-` and be interpreted as options if relative paths are used. **Prevention:** Resolve file paths before passing them to `subprocess.run` when a tool does not support an explicit input flag or `--` delimiter. Absolute paths use a root, drive, or UNC prefix rather than a leading hyphen, so they cannot be parsed as command-line options. +## 2026-06-26 - Command injection in subprocess.run via file metadata +**Vulnerability:** The `media_shrinker.py` script passed a user-controlled creation date directly to `subprocess.run(["setfile", "-d", creation_date, ...])` without validating its format. +**Learning:** File metadata (like creation time or EXIF data) can be manipulated by attackers to contain shell metacharacters or other malicious payloads. If this metadata is later passed to command-line utilities without validation, it can lead to command injection. +**Prevention:** Always validate user-controlled inputs, including file metadata, against an expected format (e.g. using a regular expression like `^\d{2}/\d{2}/\d{4} \d{2}:\d{2}:\d{2}$` for a date) before passing them to subprocesses. diff --git a/media_shrinker.py b/media_shrinker.py index 3eede1d..9647270 100644 --- a/media_shrinker.py +++ b/media_shrinker.py @@ -132,9 +132,9 @@ def command( raise MediaShrinkerError( "ffmpeg argument template is missing '-i'" ) from exc - args[input_index] = str(input_path.resolve()) + args[input_index] = str(input_path.absolute()) if output_path is not None: - args[-1] = str(output_path.resolve()) + args[-1] = str(output_path.absolute()) if overwrite: args = ["-y" if arg == "-n" else arg for arg in args] @@ -633,7 +633,7 @@ def build_icloud_download_command( ) -> list[str]: """Build a safe iCloud download command for source_path.""" - return [brctl_path, "download", str(source_path.resolve())] + return [brctl_path, "download", str(source_path.absolute())] def download_from_icloud(source_path: Path, *, brctl_path: str = "brctl") -> None: @@ -1237,6 +1237,7 @@ def _execute_conversions( protected_sources = [c[0] for c in candidates] def process_candidate(candidate_tuple: tuple[Path, int]) -> list[ConversionResult]: + """Convert a single candidate inside the thread pool.""" candidate, size = candidate_tuple try: return convert_file( @@ -1553,7 +1554,7 @@ def _execute_plan( _ensure_not_source_path(source, temp_output) try: - command = plan.command( + command_args = plan.command( ffmpeg_path=ffmpeg_path, input_path=source, output_path=temp_output, @@ -1561,7 +1562,7 @@ def _execute_plan( ) try: completed = subprocess.run( - command, check=False, capture_output=True, text=True + command_args, check=False, capture_output=True, text=True, shell=False ) except FileNotFoundError as exc: raise MediaShrinkerError(f"ffmpeg not found: {ffmpeg_path}") from exc @@ -1629,11 +1630,15 @@ def _copy_macos_creation_time( creation_date = datetime.fromtimestamp(float(birthtime)).strftime( "%m/%d/%Y %H:%M:%S" ) + import re as _re + if not _re.match(r"^\d{2}/\d{2}/\d{4} \d{2}:\d{2}:\d{2}$", creation_date): + return subprocess.run( - [setfile_path, "-d", creation_date, str(dest.resolve())], + [setfile_path, "-d", creation_date, str(dest.absolute())], check=False, capture_output=True, text=True, + shell=False, ) diff --git a/tests/test_media_shrinker.py b/tests/test_media_shrinker.py index 1b202d2..a726ed6 100644 --- a/tests/test_media_shrinker.py +++ b/tests/test_media_shrinker.py @@ -280,9 +280,9 @@ def test_conversion_command_resolves_input_and_output_overrides(self) -> None: self.assertEqual( command[command.index("-i") + 1], - str(Path("-input.wav").resolve()), + str(Path("-input.wav").absolute()), ) - self.assertEqual(command[-1], str(Path("-output.flac").resolve())) + self.assertEqual(command[-1], str(Path("-output.flac").absolute())) def test_lossy_audio_uses_highest_opus_bitrate_that_fits_target_with_safety_margin( self, @@ -1042,7 +1042,7 @@ def test_build_icloud_download_command_uses_argument_list_for_paths_with_spaces( self.assertEqual( command, - ["brctl", "download", str(Path("folder/file with spaces.m4a").resolve())], + ["brctl", "download", str(Path("folder/file with spaces.m4a").absolute())], )