diff --git a/CHANGELOG.md b/CHANGELOG.md index 810b0cd0..f3b3ae01 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,44 @@ ## Unreleased -(nothing yet) +### Security + +- **A DASH manifest or an IMF composition playlist given as media is refused, whatever it is + called.** The playlist guard knew HLS and ffconcat by name and first bytes only, so a DASH + manifest saved as `clip.mp4` (with a `dash:profile` or the 3GPP `PSS:profile:DASH1`) made every + tool follow its `BaseURL`: to another local file, which + the output then contained (reported completed and verified), or over HTTP, which FFmpeg 6.1's + `-protocol_whitelist file` did not stop. Both are now told the way ffmpeg's own probes tell + them, by content, and refused as `media_path_is_playlist`. +- **`subfile,:` is refused as a URL.** ffmpeg opens it through the subfile protocol; + the URL guard stopped reading the scheme at the comma and let it through, so the guards + described the named file while ffmpeg read another. +- **A path with a line break is refused where it would go into a concat list** (`loop.py`, + `join.py --copy`, `sequence.py`, multi-part cuts): `media_path_has_line_break`. Each line of a + concat list is a directive, so such a name added entries and pulled a file named nowhere on the + command line into the output. +- **The hidden temp files in the output directory are created exclusively.** The staging temp of + an `--overwrite` run, `place_output()`'s temp, `export.py`'s key/fill temp and `batch.py`'s + cross-filesystem delivery temp had fixed names and were opened through + whatever was already there, a planted symlink included: the encode was written into the link's + target and the output left as a symlink to it. A copy the tool makes itself is written + through the descriptor that created the temp, never by reopening its name, and a directory that + refuses the exclusive create fails the run (`output_dir_not_writable`) instead of handing ffmpeg + a name nobody reserved. A lock's default audit log is opened without following a symlink. +- **`render.py` refuses an output that is any file the project names.** The guard compared the + output with the clips and overlay videos only, so with `--overwrite` a project whose output was + its own music bed, brand file (or the brand's logo or font), overlay image or font, caption + file, beat source, or the project file itself replaced it. A `--stop-after` preview, which + never writes the output, is not refused. `audiogram.image` and an overlay's `font_file` are now passed + to their stages resolved against the project file, as every other project path is, instead of + against the caller's working directory. + +### Fixed + +- **A colour given to `caption.py`, `graphics.py` or a brand must be six hex digits.** The check + was on the length only: a six-letter name such as `yellow` or `orange` drew black text and was + reported verified, and a quote or comma reached the filter graph and failed as `kind: ffmpeg`. + It is now a `kind: input` refusal. ## 3.6.0 diff --git a/docs/contract.md b/docs/contract.md index 09792a56..bdde626f 100644 --- a/docs/contract.md +++ b/docs/contract.md @@ -897,7 +897,7 @@ here claims otherwise until real exit-code/stderr sniffing exists to back that u A refusal also carries `error.reason`, a stable snake_case code for *why* (`contract --json` → `json_output.error_reasons` lists them: `output_exists`, `output_is_input`, `output_is_directory`, `output_dir_missing`, `output_dir_not_writable`, `output_locked`, -`media_path_is_url`, `media_path_is_playlist` (an HLS or ffconcat playlist given as media), +`media_path_is_url` (`subfile,...:` included), `media_path_is_playlist` (an HLS, DASH, IMF or ffconcat playlist given as media, DASH and IMF told by content whatever the file is called), `media_path_has_line_break` (a path with a line break, which a concat list cannot hold), `output_looks_like_option` (an output starting with `-`, other than `-` for stdout), `output_path_taken` (two files one run writes share a path), `media_path_not_a_file` (a FIFO or device as input), `silence.py --timemap`'s `timemap_path_taken`, `join.py --copy`'s `join_copy_mismatch`, `render.py`'s `plan_*` reasons, `--codec av1`/`prores`'s `encoder_missing` and `encoder_list_unreadable`, and `--transcribe`'s `asr_engine_missing`, `no_speech`, `engine_failed` diff --git a/docs/design-decisions.md b/docs/design-decisions.md index db7cc6b0..3459ff92 100644 --- a/docs/design-decisions.md +++ b/docs/design-decisions.md @@ -407,7 +407,29 @@ exists. When a decision changes, edit the entry in the same PR. - **An existing output is written through a hidden sibling temp file and replaced only on success**, so a failed run never costs the caller the file that was there (FFmpeg 5.x truncates the output before a filter error). The temp name `..ffskill-` is expected in the - output directory during a run. Code: `_common._stage_existing_output()`. + output directory during a run; it is created exclusively (`O_EXCL`), so an entry already at that + name, a planted symlink included, is never written through, and the next free + `..ffskill--` is used instead. A copy the tool makes itself (`place_output`, + `batch.py`'s cross-filesystem delivery) writes the descriptor that created the temp + (`copy_to_sibling()`); ffmpeg opens its temp by name, so a process that may delete entries in + the output directory could still swap the file between the create and that open, which a + sticky shared directory (`/tmp`) and a directory only the caller can write both rule out. A lock's default audit log beside the output + is opened without following a symlink; a log named with `--audit-log` is opened as named. Code: + `_common._stage_existing_output()`, `runner.reserve_sibling()`, `emit.audit()`. Test: + `test_hidden_temp_names_are_never_written_through_a_planted_link`. +- **A file ffmpeg would read as a playlist is refused whatever it is called.** HLS and ffconcat + are told by name or their first bytes; a DASH manifest and an IMF composition playlist by name + or by the strings ffmpeg's own probes look for (``), anywhere before the first NUL byte, because ffmpeg probes the whole probe + buffer: a manifest saved as `clip.mp4` made every tool follow its BaseURL to another local file + or over HTTP, and FFmpeg 6.1's `-protocol_whitelist file` did not stop the HTTP request. A media + file has a NUL byte within its first bytes, so the scan stops there. `subfile,:` + is refused as a URL like any other protocol prefix. A path with a line break is refused where it + would be written into a concat list (`media_path_has_line_break`): the format has no escape for + one. Code: `runner._refuse_playlist()`, `runner.looks_like_url()`, `decision.concat_list_line()`. + Tests: `test_a_playlist_input_and_an_option_shaped_output_are_refused`, + `test_a_url_never_reaches_ffmpeg_as_an_input_or_an_output`, + `test_a_path_with_a_line_break_never_reaches_a_concat_list`. - **Subtitle/data tracks are kept where the picture's timeline is untouched and dropped where it is retimed; both cases report `dropped_non_av_streams`.** Tools that re-encode the picture or audio in place (`fit`, `color`, `graphics`, `overlay`, `audio`, `loudness`, `proxy`, diff --git a/scripts/_ass_overlay.py b/scripts/_ass_overlay.py index 5f934d37..ea66a9c4 100644 --- a/scripts/_ass_overlay.py +++ b/scripts/_ass_overlay.py @@ -54,7 +54,7 @@ def ass_time(sec: float) -> str: def ass_color(hex_rgb: str, alpha: int = 0) -> str: h = str(hex_rgb).lstrip("#") - if len(h) != 6: + if len(h) != 6 or any(c not in "0123456789abcdefABCDEF" for c in h): raise ValueError(f"colour must be RRGGBB hex, got '{hex_rgb}'") return f"&H{alpha:02X}{h[4:6]}{h[2:4]}{h[0:2]}".upper() diff --git a/scripts/_common/color.py b/scripts/_common/color.py index 7d0ceebe..0b61d580 100644 --- a/scripts/_common/color.py +++ b/scripts/_common/color.py @@ -312,13 +312,20 @@ def through(m: "re.Match[str]") -> str: return out[:at] + ["-vf", conv] + out[at:] +_HEX6 = re.compile(r"^[0-9A-Fa-f]{6}$") + + def color_hex(value: str) -> str: - """Normalise '#ffd200' / 'ffd200' / '0xFFD200' to 'FFD200'.""" + """Normalise '#ffd200' / 'ffd200' / '0xFFD200' to 'FFD200'. Six hex digits only: the result is + written into libass styles and drawtext/color options, so a six-letter colour name ("yellow") + used to pass the length check and draw black, reported verified, and a quote or comma reached + the filter graph.""" v = str(value).strip().lstrip("#") if v.lower().startswith("0x"): v = v[2:] - if len(v) != 6: - die(f"colour must be RRGGBB, got '{value}'") + if not _HEX6.match(v): + die(f"colour must be six hex digits RRGGBB (e.g. FFD200 or #ffd200), got '{value}'; colour names are not " + "accepted here") return v.upper() diff --git a/scripts/_common/decision.py b/scripts/_common/decision.py index 1296f82f..b8b0b8c5 100644 --- a/scripts/_common/decision.py +++ b/scripts/_common/decision.py @@ -148,7 +148,15 @@ def concat_list_line(path: str) -> str: """One `file '...'` line for the concat demuxer. The demuxer reads backslash as an escape inside the quoted form, so a Windows path (C:\\Users\\...\\part000.mp4) must be written with forward slashes -- ffmpeg opens either spelling on Windows -- and a single quote in the - name is closed, escaped and reopened. Shared by cut.py (multi-segment) and sequence.py.""" + name is closed, escaped and reopened. Shared by cut.py (multi-segment) and sequence.py. + + A line break has no escape in this format: the demuxer reads each line as its own directive, + so a name containing one would add `file` entries to the list and pull in files the caller + never named. Such a path is refused.""" + if "\n" in str(path) or "\r" in str(path): + refuse("input", "media_path_has_line_break", + f"{str(path)!r} contains a line break, which ffmpeg's concat list cannot hold (each line is a " + "directive of its own); rename the file", path=str(path)) # only on Windows: on POSIX the quoted form takes a backslash literally, and it is part of the name (#408) escaped = (str(path).replace("\\", "/") if _on_windows() else str(path)).replace("'", "'\\''") return f"file '{escaped}'" diff --git a/scripts/_common/emit.py b/scripts/_common/emit.py index 31919a41..fc57c00c 100644 --- a/scripts/_common/emit.py +++ b/scripts/_common/emit.py @@ -8,6 +8,7 @@ import json import math import os +import stat import sys from typing import Any, Dict, List, Optional, Sequence, Tuple from _common.runner import Context, ERROR_CODE, ERROR_RETRYABLE, STATE @@ -319,7 +320,28 @@ def audit(event: str, details: Dict[str, Any], *, default_path: Optional[str] = rec["plan_sha256"] = plan_sha256 rec["details"] = details try: - with open(path, "a", encoding="utf-8") as f: + # a default path is a hidden name in someone's output directory: never appended to through a + # symlink planted there. A log the caller named (--audit-log) is opened as named. + flags = os.O_WRONLY | os.O_APPEND | os.O_CREAT | (0 if ctx.audit_log else getattr(os, "O_NOFOLLOW", 0)) + checked = not ctx.audit_log and not hasattr(os, "O_NOFOLLOW") + if checked and os.path.islink(path): + raise OSError(f"{path} is a symbolic link") + fd = os.open(path, flags, 0o666) + if checked: + # no O_NOFOLLOW (Windows): a link that appeared between the look above and the open is + # caught after it -- the descriptor must be the file now at that name, not a link's + # target -- and nothing is written through it + try: + st, at = os.fstat(fd), os.lstat(path) + # st_nlink: a link swapped for a hard link to its target before the lstat would + # otherwise pass the identity check; a log this tool created has one name + if (stat.S_ISLNK(at.st_mode) or (st.st_dev, st.st_ino) != (at.st_dev, at.st_ino) + or not stat.S_ISREG(st.st_mode) or st.st_nlink != 1): + raise OSError(f"{path} was replaced by a link while it was opened") + except BaseException: + os.close(fd) + raise + with os.fdopen(fd, "a", encoding="utf-8") as f: f.write(json.dumps(json_safe(rec), ensure_ascii=False, sort_keys=True) + "\n") except OSError as exc: warn(f"could not append the {event} record to the audit log {path} ({exc}); the run is unaffected", diff --git a/scripts/_common/runner.py b/scripts/_common/runner.py index 015a3e33..33e972b6 100644 --- a/scripts/_common/runner.py +++ b/scripts/_common/runner.py @@ -775,21 +775,69 @@ def video_encoder_of(cmd: Sequence[str]) -> Optional[str]: _URL_SCHEME_POSIX = re.compile(r"^[A-Za-z][A-Za-z0-9+.-]*:") +def _is_subfile_url(value: str) -> bool: + """ffmpeg's url_find_protocol() also opens `subfile,:` through the subfile + protocol: the scheme stops at the comma, so the regexes above never see it.""" + return value.startswith("subfile,") and ":" in value[len("subfile,"):] + + def looks_like_url(value: str) -> bool: """True for a value ffmpeg would open through a protocol rather than as a local file.""" + if _is_subfile_url(value): + return True scheme = _URL_SCHEME if os.name == "nt" else _URL_SCHEME_POSIX return bool(scheme.match(value)) and not value.startswith("pipe:") -_PLAYLIST_SUFFIXES = (".m3u8", ".m3u", ".ffconcat") +def url_scheme(value: str) -> str: + """The protocol name ffmpeg would open `value` through (for a looks_like_url() value).""" + return "subfile" if _is_subfile_url(value) else value.split(":", 1)[0].lower() + + +_PLAYLIST_SUFFIXES = (".m3u8", ".m3u", ".ffconcat", ".mpd") _PLAYLIST_MAGIC = (b"#EXTM3U", b"ffconcat") +# ffmpeg probes a text format over the whole probe buffer (up to -probesize, 5,000,000 bytes), not +# over a fixed prefix, and reads that buffer as a C string, so the scan below stops at the first +# NUL byte: a real media file has one within its first few bytes, a manifest has none. +_MANIFEST_SCAN_LIMIT = 5_000_000 + + +def _manifest_kind(path: str) -> Optional[str]: + """"DASH" or "IMF" when ffmpeg's own probe would read `path` as that manifest, whatever its + name: dash_probe() looks for ``. Either one names other files, local or remote, that + ffmpeg then opens (a DASH BaseURL is followed over HTTP even under -protocol_whitelist file).""" + buf = bytearray() + try: + with open(path, "rb") as fh: + while len(buf) < _MANIFEST_SCAN_LIMIT: + chunk = fh.read(65536) + if not chunk: + break + nul = chunk.find(b"\0") + if nul >= 0: + buf += chunk[:nul] + break + buf += chunk + except OSError: + return None + text = bytes(buf[:_MANIFEST_SCAN_LIMIT]) + low = text.lower() + if b"" in text: + return "IMF" + return None def _refuse_playlist(value: str) -> None: - """Refuse an input that ffmpeg would read as a playlist (HLS .m3u8, an ffconcat list) rather - than as media. A playlist names other files by path, so a media file handed to the skill could - pull in any file the user can read. The skill's own concat lists pass `-f concat` and are not - checked here.""" + """Refuse an input that ffmpeg would read as a playlist (HLS .m3u8, a DASH .mpd, an IMF + composition playlist, an ffconcat list) rather than as media. A playlist names other files by + path or URL, so a media file handed to the skill could pull in any file the user can read, or + make ffmpeg fetch a URL. HLS and ffconcat are told by name or by their first bytes; DASH and IMF + by name or by the content ffmpeg's own probe looks for, anywhere before the first NUL byte. The + skill's own concat lists pass `-f concat` and are not checked here.""" if os.path.exists(value) and not os.path.isfile(value) and not os.path.isdir(value): # a FIFO or a device: ffmpeg would block on it or read whatever it streams refuse("input", "media_path_not_a_file", @@ -801,9 +849,14 @@ def _refuse_playlist(value: str) -> None: head = fh.read(16).lstrip(b"\xef\xbb\xbf \t\r\n") except OSError: pass + kind = None if value.lower().endswith(_PLAYLIST_SUFFIXES) or head.startswith(_PLAYLIST_MAGIC): + kind = "HLS, DASH or ffconcat" + elif os.path.isfile(value): + kind = _manifest_kind(value) + if kind: refuse("input", "media_path_is_playlist", - f"{value!r} is a playlist (HLS or ffconcat), which ffmpeg would follow to the files it names; " + f"{value!r} is a playlist ({kind}), which ffmpeg would follow to the files or URLs it names; " "this skill takes media files only. Pass the media file itself", path=value) @@ -836,9 +889,9 @@ def _refuse_url_media(cmd: Sequence[str]) -> None: for value in values: if looks_like_url(value): refuse("input", "media_path_is_url", - f"{value!r} would be opened by ffmpeg as a URL ({value.split(':', 1)[0]}:), not as a local file; " + f"{value!r} would be opened by ffmpeg as a URL ({url_scheme(value)}:), not as a local file; " "this skill reads and writes local files only. Pass a plain path (a local name containing " - "a colon works as ./name)", path=value, scheme=value.split(":", 1)[0].lower()) + "a colon works as ./name)", path=value, scheme=url_scheme(value)) def _check_no_overwrite_input(cmd: Sequence[str]) -> None: @@ -1243,10 +1296,73 @@ def _stage_existing_output(cmd: Sequence[str]) -> Tuple[List[str], Optional[str] return list(cmd), None, None d, base = os.path.split(output) stem, ext = os.path.splitext(base) - tmp = os.path.join(d, f".{stem}.ffskill-{os.getpid()}{ext}") + tmp = reserve_sibling(d, stem, ext) return list(cmd[:-1]) + [tmp], output, tmp +def _reserve(d: str, stem: str, ext: str, tag: str) -> "Tuple[str, Optional[int]]": + """(path, open descriptor) of a hidden sibling created exclusively; (path, None) when the + directory refuses the create (the write that follows then fails there).""" + flags = os.O_CREAT | os.O_EXCL | os.O_WRONLY | getattr(os, "O_NOFOLLOW", 0) | getattr(os, "O_BINARY", 0) + n = 1 + while True: + path = os.path.join(d, f".{stem}.ffskill-" + (f"{tag}-" if tag else "") + f"{os.getpid()}" + + (f"-{n}" if n > 1 else "") + ext) + try: + return path, os.open(path, flags, 0o666) + except FileExistsError: + n += 1 + except OSError: + return path, None + + +def reserve_sibling(d: str, stem: str, ext: str, tag: str = "") -> str: + """Create the hidden sibling `..ffskill-` (or `...--` when that + name is taken) exclusively, and return its path, for ffmpeg to write with -y. O_EXCL never + opens an existing entry, a symlink included, so a name planted in the output directory + beforehand is never written through. What it cannot stop is someone who may delete files in + that directory replacing this one between the create and ffmpeg's open: not possible in a + sticky shared directory (/tmp), nor in one only the caller can write. A copy this process makes + itself goes through copy_to_sibling(), which writes the descriptor it created.""" + path, fd = _reserve(d, stem, ext, tag) + if fd is None: + # fail closed: a name that was not created exclusively is never handed to ffmpeg + refuse("input", "output_dir_not_writable", f"cannot create a temporary file in {d or '.'!r}", + output=os.path.join(d, stem + ext), directory=d or ".") + os.close(fd) + return path + + +def copy_to_sibling(src: str, dst: str) -> str: + """Copy `src` into a hidden sibling of `dst` created exclusively, through the descriptor that + created it (never by reopening the name), and return the sibling's path for os.replace().""" + import shutil + d, base = os.path.split(dst) + stem, ext = os.path.splitext(base) + path, fd = _reserve(d, stem, ext, "") + if fd is None: + raise OSError(f"cannot create a temporary file beside {dst}") + try: + fout = os.fdopen(fd, "wb") + except BaseException: + os.close(fd) + try: + os.remove(path) + except OSError: + pass + raise + try: + with fout, open(src, "rb") as fin: + shutil.copyfileobj(fin, fout, 1 << 20) + except BaseException: + try: + os.remove(path) + except OSError: + pass + raise + return path + + HW_ENV = "FFMPEG_SKILL_HW" # set only by render.py/batch.py --hw/--no-hw for the stages they run: "the outer command chose # explicitly", so a child resolves $FFMPEG_SKILL_HW as a flag (export.py included), not a default @@ -1846,7 +1962,6 @@ def place_output(src: str, dst: str, move: bool = False) -> None: _check_existing_output(cmd) d, base = os.path.split(dst) stem, ext = os.path.splitext(base) - tmp = os.path.join(d, f".{stem}.ffskill-{os.getpid()}{ext}") with _OutputLock(dst): if move: try: @@ -1855,14 +1970,16 @@ def place_output(src: str, dst: str, move: bool = False) -> None: return except OSError: pass + tmp = None try: - shutil.copyfile(src, tmp) + tmp = copy_to_sibling(src, dst) os.replace(tmp, dst) except OSError as e: - try: - os.remove(tmp) - except OSError: - pass + if tmp: + try: + os.remove(tmp) + except OSError: + pass die(f"could not place {dst}: {e}", kind="output") _remember_output(cmd) diff --git a/scripts/_contract.py b/scripts/_contract.py index e12d1b64..a5f277d7 100644 --- a/scripts/_contract.py +++ b/scripts/_contract.py @@ -424,7 +424,8 @@ def input_schema(parser: argparse.ArgumentParser) -> Dict[str, Any]: "output_exists": "an output already exists and --overwrite was not given; details.outputs: [{path, resolved, size, mtime_ns}]", "output_is_input": "the output resolves to the same file as an input; details: {output, input, resolved}", "output_is_directory": "the output path is a directory; details: {output}", - "media_path_is_playlist": "an input is a playlist (HLS .m3u8/.m3u or an ffconcat list, by name or by its first bytes) that ffmpeg would follow to the files it names; the skill takes media files only; details: {path}", + "media_path_is_playlist": "an input is a playlist that ffmpeg would follow to the files or URLs it names: HLS .m3u8/.m3u or an ffconcat list (by name or by its first bytes), a DASH manifest or an IMF composition playlist (by name or by the content ffmpeg's own probe looks for, whatever the file is called); the skill takes media files only; details: {path}", + "media_path_has_line_break": "a path written into a concat list (loop.py, join.py --copy, sequence.py, multi-part cuts) contains a line break, which the concat format cannot hold: each line is a directive, so the name would add entries to the list; details: {path}", "media_path_not_a_file": "an input exists but is not a regular file (a FIFO or a device), which ffmpeg would block on or read as a stream; details: {path}", "join_copy_mismatch": "join.py --copy: a clip's streams do not match the first clip's, it hides pre-roll frames with an edit list, or its picture and sound end more than 50 ms apart; details: {clips: [{index?, path, differs}]}", "output_path_taken": "two files one run writes (a side file and the output, or two side files) resolve to the same path, so the second would replace the first; details: {output, other}", diff --git a/scripts/batch.py b/scripts/batch.py index abdb2358..a4a166a7 100755 --- a/scripts/batch.py +++ b/scripts/batch.py @@ -33,7 +33,7 @@ from typing import Any, Dict, List, Optional from _common import STATE, add_common, apply_common, child_args, die, emit, info, run_tool, MEDIA_EXT as _MEDIA_EXT -from _common.runner import HW_FORCED_ENV, _OutputLock, add_hw_orchestrator_args, env_hw, stop_children +from _common.runner import HW_FORCED_ENV, _OutputLock, add_hw_orchestrator_args, copy_to_sibling, env_hw, stop_children from _common.emit import absorb_stage_hw, audit, note, shown, steps_encoder_report, warn HERE = Path(__file__).resolve().parent @@ -265,16 +265,18 @@ def locked(msg: str, hint: str, details: Dict[str, Any]) -> None: except OSError: # another filesystem: copy beside the final path and rename, so a copy that fails part # way never leaves a truncated file where the caller's was - import shutil - tmp = final.with_name(f".{final.stem}.ffskill-{os.getpid()}-{threading.get_ident()}{final.suffix}") + # created exclusively and written through the descriptor that created it: never through + # an entry planted at the name, and two threads delivering side by side get two names + tmp = None try: - shutil.copyfile(src, tmp) + tmp = copy_to_sibling(str(src), str(final)) os.replace(tmp, final) except OSError as exc: - try: - os.remove(tmp) - except OSError: - pass + if tmp: + try: + os.remove(tmp) + except OSError: + pass raise ItemError({"step": None, "tool": None, "kind": "output", "message": f"could not deliver {src} to {final}: {exc}"}) log(f" delivered the last written file to {final}") diff --git a/scripts/caption.py b/scripts/caption.py index 67ed93f6..24fe3a2e 100755 --- a/scripts/caption.py +++ b/scripts/caption.py @@ -926,8 +926,8 @@ def mux_subtitle_codec(output: str) -> str: def ass_color(hex_rgb: str, alpha: int = 0) -> str: h = hex_rgb.lstrip("#") - if len(h) != 6: - die(f"colour must be RRGGBB hex, got '{hex_rgb}'") + if len(h) != 6 or any(c not in "0123456789abcdefABCDEF" for c in h): + die(f"colour must be six hex digits RRGGBB, got '{hex_rgb}'") r, g, b = h[0:2], h[2:4], h[4:6] return f"&H{alpha:02X}{b}{g}{r}".upper() diff --git a/scripts/export.py b/scripts/export.py index 8e6f8596..8692b32e 100755 --- a/scripts/export.py +++ b/scripts/export.py @@ -51,7 +51,7 @@ from _common.decision import hw_preset_video, restate_last_swap from _common.emit import note, record_fallback, record_skip, warn from _common.probe import square_pixels, square_pixels_note -from _common.runner import ffmpeg_encoders, guard_sidecar, remember_written, run_analysis as _run_analysis, subtitle_codec_for +from _common.runner import ffmpeg_encoders, guard_sidecar, remember_written, reserve_sibling, run_analysis as _run_analysis, subtitle_codec_for from _common.emit import file_identity from check import SPECS as PLATFORMS, measure_loudness from _platforms import ALIASES as _ALIASES, PLATFORMS as PLATFORM_TABLE, TP_TOLERANCE_DB, resolve as resolve_platform @@ -213,7 +213,7 @@ def _key_fill(args, p: Dict, meta: Dict, output: str, cmd: List[str], vf: List[s if not STATE.dry_run: d, base = os.path.split(output) stem, ext = os.path.splitext(base) - fill_tmp = os.path.join(d, f".{stem}.ffskill-fill-{os.getpid()}{ext}") + fill_tmp = reserve_sibling(d, stem, ext, "fill") # created exclusively: never through a planted link cmd += (p["audio"] if has_audio else ["-an"]) + [fill_tmp or output] cmd += ["-map", "[key]"] + p["video"] + tc_args + cfr_args(meta) + ["-an", key] try: diff --git a/scripts/render.py b/scripts/render.py index 514dfddd..53c03219 100755 --- a/scripts/render.py +++ b/scripts/render.py @@ -464,6 +464,64 @@ def check_overlay(ov: Any, label: str) -> None: die(f"{label}: scale sizes an image, logo or video layer; text size is font_size", kind="input") +def project_input_files(proj: Dict[str, Any], rel: Any) -> List[str]: + """Every file a project names as an input: clip sources, music/replace/effects, the brand, + caption files (a --text value that names an existing file), overlay images, videos and fonts, the + audiogram image, a beat grid's source and the chapters file. Values of the wrong type are left + to the stage that reads them.""" + found: List[str] = [] + + def add(value: Any, must_exist: bool = False) -> None: + if isinstance(value, str) and value: + p = rel(value) + if not must_exist or os.path.isfile(p): + found.append(p) + + for c in proj.get("clips") or []: + if isinstance(c, dict): + add(c.get("src")) + if isinstance(c.get("snap"), dict): + add(c["snap"].get("source")) + for key in ("audio", "captions", "audiogram", "snap"): + if not isinstance(proj.get(key), dict): + continue + obj = proj[key] + if key == "audio": + for k in ("music", "replace", "effects"): + add(obj.get(k)) + elif key == "captions": + add(obj.get("srt")) + add(obj.get("ass")) + add(obj.get("text"), must_exist=True) # inline cue text is not a path + elif key == "audiogram": + add(obj.get("image")) + else: + add(obj.get("source")) + add(proj.get("brand")) + brand = proj.get("brand") + if isinstance(brand, str) and brand: + # the brand's own logo and font_file are read by the overlay and caption stages, resolved + # against the brand file as load_brand() resolves them; an unreadable brand is left to them + try: + data = json.loads(Path(rel(brand)).read_text(encoding="utf-8")) + except (OSError, ValueError): + data = None + if isinstance(data, dict): + base = Path(rel(brand)).resolve().parent + for k in ("logo", "font_file"): + v = data.get(k) + if isinstance(v, str) and v and "://" not in v: + found.append(v if os.path.isabs(v) else str(base / v)) + add(proj.get("chapters"), must_exist=True) + overlays = proj.get("overlays") + for ov in (overlays if isinstance(overlays, list) else []): # any other type is the overlay stage's to refuse + if isinstance(ov, dict): + add(ov.get("image")) + add(ov.get("video")) + add(ov.get("font_file")) + return found + + def require_object(obj: Any, label: str) -> None: """Refuse a value that is not an object where the run reads it with .get(): "export": "reels" in a render, a string frame, a clip written as a bare path. 2.2.1 died there with a traceback @@ -1216,8 +1274,13 @@ def rel(p: Any) -> str: if not clips: die("project.clips is empty") output = rel(proj.get("output") or "final.mp4") - # the final stage is a copy from the work dir, so run()'s own guard never sees the sources (review 5) - refuse_output_is_input(output, *[rel(c.get("src")) for c in clips if c.get("src")]) + # the final stage is a copy from the work dir, so run()'s own guard never sees the sources + # (review 5): every file the project names is passed here, not only the clips, or --overwrite + # replaced a music bed or a brand file named as the output + if not args.stop_after or args.stop_after == "export": + # only a run that ends in the final copy writes `output`; a --stop-after preview writes + # its stage file inside the work directory and leaves `output` alone + refuse_output_is_input(output, *([os.path.abspath(args.project)] if args.project else []), *project_input_files(proj, rel)) if _reaches("overlays", args.stop_after): # #326: an overlay video is the caller's file, never an earlier stage's output, so a # missing one is refused here, under --dry-run too, not after every stage before it ran @@ -1396,7 +1459,12 @@ def rel(p: Any) -> str: ("color", "--color"), ("background", "--background"), ("width", "--width"), ("height", "--height"), ("fps", "--fps")): if ag.get(key) is not None: - argv += [flag, str(ag[key])] + value = ag[key] + if key == "image" and isinstance(value, str) and "://" not in value: + # against the project file, as every other path here and as the output guard + # checks it; it used to reach waveform.py as written, against the cwd + value = rel(value) + argv += [flag, str(value)] argv += brand_args sh("waveform.py", *argv, stage="audiogram") current = nxt @@ -1542,7 +1610,8 @@ def rel(p: Any) -> str: for k, flag in (("position", "--position"), ("start", "--start"), ("end", "--end"), ("fade", "--fade"), ("opacity", "--opacity"), ("scale", "--scale"), ("font_size", "--font-size"), ("font", "--font"), ("font_file", "--font-file"), ("margin", "--margin"), ("platform", "--platform"), ("chromakey", "--chromakey"), ("chromakey_similarity", "--chromakey-similarity"), ("chromakey_blend", "--chromakey-blend")): if ov.get(k) is not None: - argv += [flag, str(ov[k])] + # font_file is a path: the same one project_input_files() guards + argv += [flag, rel(ov[k]) if k == "font_file" and ov[k] else str(ov[k])] if ov.get("box"): argv.append("--box") # review 12: the overlay stage was the one stage that never heard which destination this diff --git a/tests/test_contract.py b/tests/test_contract.py index f4ceb464..8f51ae58 100644 --- a/tests/test_contract.py +++ b/tests/test_contract.py @@ -3915,6 +3915,37 @@ def test_a_playlist_input_and_an_option_shaped_output_are_refused(self): err = json.loads(proc.stdout)["error"] self.assertEqual((err["kind"], err["reason"]), ("input", "media_path_is_playlist"), (name, path, proc.stderr)) self.assertFalse((work / "out.mp4").exists()) + # DASH and IMF are told by content, as ffmpeg's own probes tell them: a manifest named .mp4 + # made every tool follow its BaseURL to another local file (and over HTTP). The case and a + # long comment before {self.src}\n') + dash = work / "dash.mp4" + dash.write_text('\n' + mpd, encoding="utf-8") + hidden = work / "dash_hidden.mov" + hidden.write_text("\n" + mpd.replace("", ""), encoding="utf-8") + named_mpd = work / "manifest.mpd" + named_mpd.write_text(mpd, encoding="utf-8") + # dash_probe() also accepts the 3GPP profile in place of dash:profile + dash3gpp = work / "dash_3gpp.mp4" + dash3gpp.write_text(mpd.replace("urn:mpeg:dash:profile:isoff-on-demand:2011", "urn:3GPP:PSS:profile:DASH1"), encoding="utf-8") + imf = work / "cpl.mp4" + imf.write_text('\n' + "urn:uuid:0x\n", encoding="utf-8") + self.assertEqual(sh("ffprobe", "-v", "error", "-show_entries", "format=format_name", "-of", "csv=p=0", dash).stdout.strip(), + "dash", "the fixture must be what ffprobe reads as DASH") + self.assertEqual(sh("ffprobe", "-v", "error", "-show_entries", "format=format_name", "-of", "csv=p=0", dash3gpp, + check=False).stdout.strip(), "dash", "ffprobe reads the 3GPP profile as DASH too") + for path in (dash, hidden, named_mpd, dash3gpp, imf): + for name, args in (("probe", [path]), ("cut", [path, "--start", "0", "--end", "0.5", "-o", work / "pulled.mp4"])): + proc = tool(name, *args, "--json", check=False) + err = json.loads(proc.stdout)["error"] + self.assertEqual((err["kind"], err["reason"]), ("input", "media_path_is_playlist"), (name, path, proc.stderr)) + self.assertFalse((work / "pulled.mp4").exists()) + # a real media file is not read past its first NUL byte, and is not refused + import importlib + self.assertIsNone(importlib.import_module("_common.runner")._manifest_kind(str(self.src))) if hasattr(os, "mkfifo"): # a pipe is refused before anything opens it (an open would block) fifo = work / "pipe.mp4" if not fifo.exists(): @@ -3926,6 +3957,191 @@ def test_a_playlist_input_and_an_option_shaped_output_are_refused(self): self.assertEqual((err["kind"], err["reason"]), ("input", "output_looks_like_option")) self.assertEqual([p.name for p in work.glob("ffmpeg-*.log")], [], "the output reached ffmpeg as -report") + def test_render_refuses_an_output_that_is_any_file_the_project_names(self): + """render.py's last stage is a copy from its work directory, so run()'s own guard never sees + the sources: the guard compared the output with the clips and overlay videos only, and + with --overwrite a project whose output was its own music bed replaced it.""" + work = self.out("render_guard"); work.mkdir(exist_ok=True) + bed = work / "bed.wav" + shutil.copy(self.wav, bed) + before = bed.read_bytes() + brand = work / "brand.json" + brand.write_text('{"name": "x"}', encoding="utf-8") + for key, value, output in (("audio", {"music": "bed.wav"}, "bed.wav"), + ("brand", "brand.json", "brand.json"), + ("overlays", [{"image": "bed.wav"}], "bed.wav"), + ("overlays", [{"text": "hi", "font_file": "brand.json"}], "brand.json")): + proj = work / f"p_{key}.json" + proj.write_text(json.dumps({"output": output, "clips": [{"src": str(self.src)}], key: value}), encoding="utf-8") + for extra in ([], ["--dry-run"]): + proc = tool("render", proj, "--overwrite", "--json", *extra, check=False) + err = json.loads(proc.stdout)["error"] + self.assertEqual(err["reason"], "output_is_input", (key, extra, proc.stderr)) + self.assertEqual(bed.read_bytes(), before) + self.assertEqual(brand.read_text(encoding="utf-8"), '{"name": "x"}') + # inline caption text is not a path: a cue that happens to read like the output is no refusal + import importlib + render = importlib.import_module("render") + found = render.project_input_files({"captions": {"text": "final.mp4"}, "clips": []}, lambda p: str(work / p)) + self.assertEqual(found, []) + # an overlays value of the wrong type is the overlay stage's to refuse, not a traceback here + self.assertEqual(render.project_input_files({"overlays": 42}, lambda p: str(work / p)), []) + # the audiogram image reaches waveform.py resolved against the project file, the path the + # guard checked, not against the caller's working directory + img = work / "cover.png" + ffmpeg("-f", "lavfi", "-i", "color=c=red:s=64x64", "-frames:v", "1", img) + ag = work / "p_audiogram.json" + ag.write_text(json.dumps({"output": "ag.mp4", "clips": [{"src": str(self.wav)}], + "audiogram": {"image": "cover.png"}}), encoding="utf-8") + proc = tool("render", ag, "--dry-run", "--json", cwd=self.work) + def forwarded_path(stage: str, flag: str) -> str: + line = next((l for l in proc.stderr.splitlines() if stage in l), "") + m = re.search(re.escape(flag) + r" (.+?) --", line) + self.assertTrue(m, proc.stderr) + return m.group(1) + # the same file, whatever spelling (a Windows 8.3 short name) the temp directory has + self.assertTrue(os.path.samefile(forwarded_path("waveform.py", "--image"), img), proc.stderr) + # an overlay font_file reaches overlay.py resolved the same way + ov = work / "p_font.json" + ov.write_text(json.dumps({"output": "f.mp4", "clips": [{"src": str(self.src)}], + "overlays": [{"text": "hi", "font_file": "brand.json"}]}), encoding="utf-8") + proc = tool("render", ov, "--dry-run", "--json", cwd=self.work) + self.assertTrue(os.path.samefile(forwarded_path("overlay.py", "--font-file"), brand), proc.stderr) + # and the project file itself is an input: its own output may not replace it + selfout = work / "selfout.json" + selfout.write_text(json.dumps({"output": "selfout.json", "clips": [{"src": str(self.src)}]}), encoding="utf-8") + text = selfout.read_text(encoding="utf-8") + proc = tool("render", selfout, "--overwrite", "--json", check=False) + self.assertEqual(json.loads(proc.stdout)["error"]["reason"], "output_is_input", proc.stderr) + self.assertEqual(selfout.read_text(encoding="utf-8"), text) + # the brand's own font file, resolved against the brand file, is an input too + font = work / "brandfont.ttf" + font.write_bytes(b"not really a font") + (work / "brand2.json").write_text('{"font_file": "brandfont.ttf"}', encoding="utf-8") + bp = work / "p_brandfont.json" + bp.write_text(json.dumps({"output": "brandfont.ttf", "clips": [{"src": str(self.src)}], "brand": "brand2.json"}), + encoding="utf-8") + proc = tool("render", bp, "--overwrite", "--dry-run", "--json", check=False) + self.assertEqual(json.loads(proc.stdout)["error"]["reason"], "output_is_input", proc.stderr) + self.assertEqual(font.read_bytes(), b"not really a font") + # a --stop-after preview never writes `output`, so naming an input there is no refusal + pv = work / "p_preview.json" + pv.write_text(json.dumps({"output": "bed.wav", "clips": [{"src": str(self.src)}], "audio": {"music": "bed.wav"}}), + encoding="utf-8") + proc = tool("render", pv, "--stop-after", "clips", "--overwrite", "--dry-run", "--json", check=False) + self.assertEqual(proc.returncode, 0, proc.stderr) + self.assertIsNone(json.loads(proc.stdout).get("error"), proc.stdout) + self.assertEqual(bed.read_bytes(), before) + + def test_a_colour_is_six_hex_digits_or_refused(self): + """color_hex() checked the length only: a six-letter name ("yellow", "orange") drew black text + reported verified, and a quote or a comma reached the filter graph as kind ffmpeg.""" + for name, args in (("caption", [self.src, "--text", "hi", "--color", "yellow"]), + ("caption", [self.src, "--text", "hi", "--outline-color", "'],abc"]), + ("graphics", [self.src, "--template", "title", "--title", "HI", "--text-color", "orange"])): + proc = tool(name, *args, "-o", self.out("colour.mp4"), "--dry-run", "--json", check=False) + err = json.loads(proc.stdout)["error"] + self.assertEqual(err["kind"], "input", (args, proc.stderr)) + self.assertIn("hex", err["message"], err) + import importlib + color = importlib.import_module("_common.color") + self.assertEqual(color.color_hex("#ffd200"), "FFD200") + self.assertEqual(color.color_hex("0xFFD200"), "FFD200") + + def test_a_path_with_a_line_break_never_reaches_a_concat_list(self): + """The concat demuxer reads each line as a directive and has no escape for a line break, so + a name containing one added `file` entries to the list: loop.py on "a\\nfile " + pulled , named nowhere on the command line, into the output.""" + if os.name == "nt": + self.skipTest("Windows file names cannot hold a line break") + work = self.out("line_break"); work.mkdir(exist_ok=True) + odd = work / "a\nfile b.mp4" + shutil.copy(self.src, odd) + for name, args in (("loop", [odd, "--times", "2", "-o", work / "looped.mp4"]), + ("join", ["--copy", "--transition", "none", odd, odd, "-o", work / "joined.mp4"])): + proc = tool(name, *args, "--json", check=False) + err = json.loads(proc.stdout)["error"] + self.assertEqual((err["kind"], err["reason"]), ("input", "media_path_has_line_break"), (name, proc.stderr)) + self.assertEqual([p.name for p in work.iterdir() if p.suffix == ".mp4" and "\n" not in p.name], []) + + def test_hidden_temp_names_are_never_written_through_a_planted_link(self): + """The staging temp, place_output's temp and a lock's default audit log are fixed hidden + names in the caller's output directory. A symlink planted at one was followed: an + --overwrite run wrote the encode into the link's target and left the output a symlink to + it. The temps are now created exclusively, and the default audit log is opened without + following a link.""" + if os.name == "nt": + self.skipTest("symlinks need privileges on Windows") + import importlib + runner = importlib.import_module("_common.runner") + emit = importlib.import_module("_common.emit") + work = self.out("planted"); work.mkdir(exist_ok=True) + victim = work / "victim.txt" + victim.write_text("precious\n", encoding="utf-8") + first = work / f".out.ffskill-{os.getpid()}.mp4" + if first.is_symlink() or first.exists(): + first.unlink() + first.symlink_to(victim) + out = work / "out.mp4" + out.write_bytes(b"old") + cmd, final, tmp = runner._stage_existing_output(["ffmpeg", "-i", str(self.src), str(out)]) + self.assertEqual(final, str(out)) + self.assertNotEqual(tmp, str(first)) + self.assertTrue(os.path.isfile(tmp) and not os.path.islink(tmp), tmp) + self.assertEqual(cmd[-1], tmp) + os.remove(tmp) + # place_output() and batch.py copy through the descriptor that created the temp + again = runner.copy_to_sibling(str(self.src), str(out)) + self.assertNotEqual(again, str(first)) + self.assertTrue(os.path.isfile(again) and not os.path.islink(again)) + self.assertEqual(Path(again).read_bytes(), Path(self.src).read_bytes()) + os.remove(again) + self.assertEqual(victim.read_text(encoding="utf-8"), "precious\n") + # a name that could not be created exclusively is never handed out + from unittest import mock + with mock.patch.object(runner, "_reserve", return_value=(str(work / ".x"), None)): + with self.assertRaises(SystemExit): + runner.reserve_sibling(str(work), "out", ".mp4") + with self.assertRaises(OSError): + runner.copy_to_sibling(str(self.src), str(out)) + log = work / ".out.mp4.ffskill-audit.jsonl" + if log.is_symlink() or log.exists(): + log.unlink() + log.symlink_to(victim) + with unittest.mock.patch.object(emit, "warn"): + res = emit.audit("stale_lock_taken_over", {"lock": "x"}, default_path=str(log)) + self.assertFalse(res["written"], res) + self.assertEqual(victim.read_text(encoding="utf-8"), "precious\n") + # without O_NOFOLLOW (Windows) a link at the name is refused, and so is one that appears + # between the look and the open + nofollow = os.O_NOFOLLOW + real_open = os.open + def swap_then_open(p, *a, **k): + if os.path.lexists(p): + os.unlink(p) + os.symlink(victim, p) + return real_open(p, *a, **k) + def swap_then_hardlink(p, *a, **k): + fd = swap_then_open(p, *a, **k) + os.unlink(p) + os.link(victim, p) # the identity check alone would now pass + return fd + try: + del os.O_NOFOLLOW + for opener in (real_open, swap_then_open, swap_then_hardlink): + if not log.is_symlink(): + if log.exists(): + log.unlink() + if opener is real_open: + log.symlink_to(victim) + with unittest.mock.patch.object(emit, "warn"), unittest.mock.patch.object(emit.os, "open", opener): + res = emit.audit("stale_lock_taken_over", {"lock": "x"}, default_path=str(log)) + self.assertFalse(res["written"], (opener, res)) + self.assertEqual(victim.read_text(encoding="utf-8"), "precious\n") + log.unlink() + finally: + os.O_NOFOLLOW = nofollow + def test_a_url_never_reaches_ffmpeg_as_an_input_or_an_output(self): """The path guards read -o as a path, ffmpeg reads a value with a protocol prefix as a URL. The skill reads and writes local files only, so such an input or output is refused as @@ -3934,8 +4150,14 @@ def test_a_url_never_reaches_ffmpeg_as_an_input_or_an_output(self): import importlib, socket runner = importlib.import_module("_common.runner") for url in ("tcp:127.0.0.1:9?x=.mkv", "http://127.0.0.1:9/../../x.mkv", "file:other.mp4", "FILE:x.mp4", - "rtmp://h/live", "concat:a.ts|b.ts", "data:video/mp4;base64,AA=="): + "rtmp://h/live", "concat:a.ts|b.ts", "data:video/mp4;base64,AA==", + # ffmpeg's url_find_protocol() reads `subfile,:` as the subfile + # protocol: the scheme stops at the comma, so the regex alone missed it + "subfile,,start,0,end,0,,:clip.mp4"): self.assertTrue(runner.looks_like_url(url), url) + self.assertEqual(runner.url_scheme("subfile,,start,0,end,0,,:clip.mp4"), "subfile") + self.assertFalse(runner.looks_like_url("subfile_clip.mp4")) + self.assertFalse(runner.looks_like_url("subfile,clip.mp4")) for path in ("./clip:1.mp4", "clips/a:b.mp4", "pipe:1", "-", "a.mp4", "/abs/a.mp4", "sine=frequency=440:sample_rate=48000"): self.assertFalse(runner.looks_like_url(path), path) @@ -7477,13 +7699,14 @@ def replace(a, b): raise OSError(18, "Invalid cross-device link") return real_replace(a, b) - def copyfile(a, b): - Path(b).write_bytes(b"ne") + def copyfileobj(fin, fout, *args): + # the copy now writes the descriptor that created the temp (never reopening its name) + fout.write(b"ne") raise OSError(28, "No space left on device") saved = (batch.STATE.dry_run, batch.STATE.overwrite) try: batch.STATE.dry_run, batch.STATE.overwrite = False, True - with mock.patch("os.replace", replace), mock.patch("shutil.copyfile", copyfile): + with mock.patch("os.replace", replace), mock.patch("shutil.copyfileobj", copyfileobj): with self.assertRaises(batch.ItemError): batch.deliver(str(src), final) finally: diff --git a/tests/test_cut_copy.py b/tests/test_cut_copy.py index 342d4ed8..cb443c25 100644 --- a/tests/test_cut_copy.py +++ b/tests/test_cut_copy.py @@ -2110,7 +2110,8 @@ def test_a_checked_join_is_renamed_into_place_under_the_outputs_lock(self): self.assertEqual(Path(dst).read_bytes(), b"theirs") lock.unlink() with mock.patch.object(cut.STATE, "overwrite", True), \ - mock.patch("shutil.copyfile", side_effect=AssertionError("copied")): + mock.patch("shutil.copyfile", side_effect=AssertionError("copied")), \ + mock.patch("shutil.copyfileobj", side_effect=AssertionError("copied")): cut.place_output(src, dst, move=True) self.assertEqual(Path(dst).read_bytes(), b"new") self.assertFalse(os.path.exists(src))