From 2a44db04d8f07d02653ad2cbd9ac204269b76ae0 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 11 Jul 2026 05:41:43 +0200 Subject: [PATCH] =?UTF-8?q?chore(downloads):=20C1+C2=20=E2=80=94=20drop=20?= =?UTF-8?q?dead=20target=5Fext;=20centralize=20path-traversal=20guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - C1: remove downloads/formats.py target_ext() — defined but never called (the worker derives the real extension from the produced file's suffix). - C2: the download-root containment+existence guard was copy-pasted 6× across the file-serving endpoints (routes/downloads.py ×3, routes/public.py ×3). Extract storage.safe_abs_path(root, rel) -> Path|None so this security-sensitive check lives in one place; behavior identical (same containment test + messages). The extraction also made `pathlib.Path` unused in both route modules (removed). --- backend/app/downloads/formats.py | 8 -------- backend/app/downloads/storage.py | 11 +++++++++++ backend/app/routes/downloads.py | 16 ++++++---------- backend/app/routes/public.py | 16 ++++++---------- 4 files changed, 23 insertions(+), 28 deletions(-) diff --git a/backend/app/downloads/formats.py b/backend/app/downloads/formats.py index ef38ef9..609d9f9 100644 --- a/backend/app/downloads/formats.py +++ b/backend/app/downloads/formats.py @@ -80,14 +80,6 @@ def format_sig(spec: dict) -> str: return hashlib.sha256(payload.encode()).hexdigest()[:24] -def target_ext(spec: dict) -> str: - """Best-effort final extension for the produced file (also used to name the staging output).""" - s = normalize(spec) - if s["mode"] == "a": - return s["audio_format"] or "m4a" - return s["container"] or "mp4" - - def build_ydl_opts( spec: dict, outtmpl: str, diff --git a/backend/app/downloads/storage.py b/backend/app/downloads/storage.py index d15bb3a..7697c7e 100644 --- a/backend/app/downloads/storage.py +++ b/backend/app/downloads/storage.py @@ -100,6 +100,17 @@ def abs_path(download_root: str, rel: str) -> Path: return Path(download_root) / rel +def safe_abs_path(download_root: str, rel: str) -> Path | None: + """The resolved absolute path for `rel`, but ONLY if it stays inside DOWNLOAD_ROOT and exists on + disk — otherwise None. Centralizes the path-traversal + existence guard every file-serving endpoint + shares, so the containment check lives in exactly one place.""" + root = Path(download_root).resolve() + path = abs_path(download_root, rel).resolve() + if root not in path.parents or not path.exists(): + return None + return path + + def place_file(staging_file: Path, download_root: str, rel: str) -> None: """Move a completed staging file into its final location under DOWNLOAD_ROOT.""" dest = abs_path(download_root, rel) diff --git a/backend/app/routes/downloads.py b/backend/app/routes/downloads.py index 153f00c..f2075e0 100644 --- a/backend/app/routes/downloads.py +++ b/backend/app/routes/downloads.py @@ -7,7 +7,6 @@ serves finished files (range-aware, with the user's custom display name), and sh """ import re from datetime import datetime, timedelta, timezone -from pathlib import Path from urllib.parse import parse_qs, urlparse from fastapi import APIRouter, Depends, HTTPException, Request @@ -547,9 +546,8 @@ def download_file( asset = db.get(MediaAsset, job.asset_id) if job.asset_id else None if asset is None or asset.status != "ready" or not asset.rel_path: raise HTTPException(status_code=409, detail="This download isn't ready.") - path = storage.abs_path(settings.download_root, asset.rel_path).resolve() - root = Path(settings.download_root).resolve() - if root not in path.parents or not path.exists(): + path = storage.safe_abs_path(settings.download_root, asset.rel_path) + if path is None: raise HTTPException(status_code=404, detail="File is no longer available.") ext = asset.container or path.suffix.lstrip(".") @@ -601,9 +599,8 @@ def poster_image( asset = db.get(MediaAsset, job.asset_id) if job.asset_id else None if asset is None or not asset.poster_path: raise HTTPException(status_code=404, detail="No poster.") - path = storage.abs_path(settings.download_root, asset.poster_path).resolve() - root = Path(settings.download_root).resolve() - if root not in path.parents or not path.exists(): + path = storage.safe_abs_path(settings.download_root, asset.poster_path) + if path is None: raise HTTPException(status_code=404, detail="No poster.") return FileResponse(path, media_type="image/jpeg") @@ -616,9 +613,8 @@ def storyboard_image( asset = db.get(MediaAsset, job.asset_id) if job.asset_id else None if asset is None or not asset.storyboard_path: raise HTTPException(status_code=404, detail="No filmstrip.") - path = storage.abs_path(settings.download_root, asset.storyboard_path).resolve() - root = Path(settings.download_root).resolve() - if root not in path.parents or not path.exists(): + path = storage.safe_abs_path(settings.download_root, asset.storyboard_path) + if path is None: raise HTTPException(status_code=404, detail="No filmstrip.") return FileResponse(path, media_type="image/jpeg") diff --git a/backend/app/routes/public.py b/backend/app/routes/public.py index b36f1e9..53952a6 100644 --- a/backend/app/routes/public.py +++ b/backend/app/routes/public.py @@ -5,7 +5,6 @@ credential (a capability URL). They only ever expose one file's stream + minimal the app or any account data. Password-protected links exchange the password once at `/unlock` for a short-lived signed grant that the media URL carries (a `