diff --git a/backend/app/downloads/edit.py b/backend/app/downloads/edit.py index a4a37b8d..2ae70af6 100644 --- a/backend/app/downloads/edit.py +++ b/backend/app/downloads/edit.py @@ -438,18 +438,42 @@ _BROWSER_SAFE_VCODEC = ("h264",) _BROWSER_SAFE_ACODEC = ("aac", "mp3") -def parse_probe_streams(payload: str) -> tuple[str | None, str | None]: - """(video_codec, audio_codec) out of ffprobe's JSON stream list, lowercased. - - JSON, not the flat `default=nw=1` output: there the fields arrive in ffprobe's own struct - order, which puts `codec_name` BEFORE `codec_type`, so a line-by-line "remember the last - codec_type" parser attributes every name to the PREVIOUS stream. Measured on real downloads: - an ordinary H.264+AAC mp4 probed as `v=aac a=None` — the browser-playability check then - believed the video stream was AAC and re-encoded a perfectly fine file on every download.""" +def run_ffprobe_json( + path: Path, + *, + show_streams: bool = False, + show_format: bool = False, + entries: str | None = None, +) -> dict | None: + """One ffprobe call → its parsed JSON dict, or None on any failure (timeout, non-zero exit, + missing binary, unparseable output). The SINGLE ffprobe-JSON-invocation point in the downloads + package: the browser-playability codec probe here and the upload-ingest probe (`upload._probe`) + both go through it, so the subprocess flags/timeout/error handling live in one place instead of + two drifting copies.""" + args = ["ffprobe", "-v", "error"] + if show_streams: + args.append("-show_streams") + if show_format: + args.append("-show_format") + if entries: + args += ["-show_entries", entries] + args += ["-of", "json", str(path)] try: - streams = json.loads(payload or "{}").get("streams") or [] - except ValueError: - return None, None + out = subprocess.run(args, capture_output=True, text=True, timeout=30, check=True).stdout + return json.loads(out) + except (subprocess.TimeoutExpired, subprocess.CalledProcessError, OSError, ValueError): + return None + + +def _codecs_from_streams(streams: list) -> tuple[str | None, str | None]: + """(video_codec, audio_codec) lowercased from an ffprobe JSON stream list — the first of each kind. + + Parsed from the JSON `streams` list, NOT ffprobe's flat `default=nw=1` output: there the fields + arrive in ffprobe's own struct order, which puts `codec_name` BEFORE `codec_type`, so a line-by-line + "remember the last codec_type" parser attributes every name to the PREVIOUS stream. Measured on real + downloads: an ordinary H.264+AAC mp4 probed as `v=aac a=None` — the browser-playability check then + believed the video stream was AAC and re-encoded a perfectly fine file on every download. Reading each + stream dict by its own `codec_type` is immune to that ordering.""" vcodec = acodec = None for s in streams: name = (s.get("codec_name") or "").lower() or None @@ -464,26 +488,10 @@ def parse_probe_streams(payload: str) -> tuple[str | None, str | None]: def probe_codecs(path: Path) -> tuple[str | None, str | None]: """Return (video_codec, audio_codec) lowercased via one ffprobe call; None for a missing stream or if ffprobe fails (a failed probe is treated as 'leave it alone' upstream).""" - try: - out = subprocess.run( - [ - "ffprobe", - "-v", - "error", - "-show_entries", - "stream=codec_type,codec_name", - "-of", - "json", - str(path), - ], - capture_output=True, - text=True, - timeout=30, - check=True, - ).stdout - except (subprocess.TimeoutExpired, subprocess.CalledProcessError, OSError): + data = run_ffprobe_json(path, entries="stream=codec_type,codec_name") + if data is None: return None, None - return parse_probe_streams(out) + return _codecs_from_streams(data.get("streams") or []) def probe_duration(path: Path) -> int: diff --git a/backend/app/downloads/storage.py b/backend/app/downloads/storage.py index b24f95ca..88ee8577 100644 --- a/backend/app/downloads/storage.py +++ b/backend/app/downloads/storage.py @@ -19,6 +19,8 @@ from datetime import date from pathlib import Path from xml.sax.saxutils import escape +from app.downloads import mediakind + # Reserve room for the "_id.ext" suffix + parent dirs within the 255-char component limit. _MAX_TITLE = 120 # User-facing (Content-Disposition) names can be longer than an on-disk component. @@ -147,6 +149,14 @@ def edit_rel_path(user_id: int, asset_id: int, ext: str) -> str: return f".edits/{user_id}/clip_{asset_id}.{ext}" +def _clean_upload_ext(filename: str) -> str: + """The upload's extension reduced to a shell-safe, lowercased, bounded token (``""`` when the + upload has no extension). The SINGLE definition feeding both the on-disk suffix and the stored + `container` — they must agree (the served Content-Disposition suffix has to match the container), + so the sanitize/truncation rule lives in one place, not two drifting copies.""" + return _UNSAFE.sub("", Path(filename).suffix.lstrip(".")).lower()[:16] + + def upload_rel_path(user_id: int, asset_id: int, filename: str) -> str: """On-disk path (relative to DOWNLOAD_ROOT) for a locally-uploaded file. @@ -157,16 +167,61 @@ def upload_rel_path(user_id: int, asset_id: int, filename: str) -> str: preserved (shell-safe, bounded) so the served file + its Content-Disposition carry a correct suffix; an extensionless upload stays extensionless.""" stem = sanitize(Path(filename).stem, 80) - ext = _UNSAFE.sub("", Path(filename).suffix.lstrip(".")).lower()[:16] + ext = _clean_upload_ext(filename) suffix = f".{ext}" if ext else "" return f".uploads/{user_id}/{stem}_a{asset_id}{suffix}" def upload_ext(filename: str) -> str | None: """The stored `container` for an uploaded file: its shell-safe lowercased extension, or None when - the upload has no extension (a bare name).""" - ext = _UNSAFE.sub("", Path(filename).suffix.lstrip(".")).lower()[:16] - return ext or None + the upload has no extension (a bare name). Shares `_clean_upload_ext` with `upload_rel_path` so the + stored container and the on-disk suffix can never diverge.""" + return _clean_upload_ext(filename) or None + + +# The Content-Type used to serve an inline-playable media asset, keyed by its stored `container`. +# DELIBERATELY a strict allow-list of known audio/video containers: the served type is chosen from +# the asset's ffprobe-verified media_kind + this table, NEVER guessed from the raw (user-controlled) +# upload extension. Without this, an uploaded media/HTML polyglot named "evil.html" — a real MP3/MP4 +# that ffprobe classifies as media, so it dodges the generic-file force-download — would be served +# with a Content-Type Starlette guesses from ".html" (text/html) and execute as stored XSS on the +# serving origin. A media asset whose container isn't in this table falls back to a neutral +# force-download attachment (see `served_media_type`), so nothing script-executable is ever inline. +_MEDIA_MIME: dict[str, str] = { + # video containers + "mp4": "video/mp4", + "m4v": "video/mp4", + "mov": "video/quicktime", + "webm": "video/webm", + "mkv": "video/x-matroska", + "ogv": "video/ogg", + "avi": "video/x-msvideo", + # audio containers + "m4a": "audio/mp4", + "mp3": "audio/mpeg", + "aac": "audio/aac", + "ogg": "audio/ogg", + "oga": "audio/ogg", + "opus": "audio/ogg", + "weba": "audio/webm", + "wav": "audio/wav", + "flac": "audio/flac", +} + + +def served_media_type(media_kind: str | None, container: str | None) -> str | None: + """The Content-Type a library file MUST be served with — a real audio/video MIME for a recognised + media container, else ``None`` meaning "serve as a neutral force-download attachment". + + Returns None for a generic ``file`` kind AND for any media whose container we don't positively + recognise as safe. Callers serve None as ``application/octet-stream`` + ``attachment`` so the + served Content-Type is NEVER derived from the (attacker-controlled) file extension — the guard + that stops uploaded HTML/SVG or a media/HTML polyglot from executing inline on the serving + origin. A ``None`` media_kind (a legacy asset before the discriminator was backfilled) is treated + as media so an old download keeps its inline player.""" + if media_kind == mediakind.FILE: + return None + return _MEDIA_MIME.get((container or "").lower()) def download_filename(display_name: str, container: str | None, path: Path) -> str: diff --git a/backend/app/downloads/upload.py b/backend/app/downloads/upload.py index 1440c1f4..c863742d 100644 --- a/backend/app/downloads/upload.py +++ b/backend/app/downloads/upload.py @@ -16,9 +16,7 @@ source to re-download), so it doesn't age out on the retention clock the way a r does — it lives until the user deletes it. """ -import json import secrets -import subprocess from datetime import datetime, timezone from pathlib import Path @@ -29,32 +27,43 @@ from app.downloads import edit as editmod from app.downloads import mediakind, quota, service, storage from app.models import DownloadJob, MediaAsset +# ffprobe container formats that mean "this is a still image / image sequence", NOT playable media. +# A standalone jpg/png/gif/webp/bmp reports a real video stream (codec mjpeg/png/…) with no +# `attached_pic` flag, so without this guard it would classify as media_kind='video' and get a Play +# button + a broken in-app