chore(downloads): C1+C2 — drop dead target_ext; centralize path-traversal guard

- 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).
This commit is contained in:
npeter83 2026-07-11 05:41:43 +02:00
parent c2a2c98f16
commit 2a44db04d8
4 changed files with 23 additions and 28 deletions

View file

@ -80,14 +80,6 @@ def format_sig(spec: dict) -> str:
return hashlib.sha256(payload.encode()).hexdigest()[:24] 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( def build_ydl_opts(
spec: dict, spec: dict,
outtmpl: str, outtmpl: str,

View file

@ -100,6 +100,17 @@ def abs_path(download_root: str, rel: str) -> Path:
return Path(download_root) / rel 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: def place_file(staging_file: Path, download_root: str, rel: str) -> None:
"""Move a completed staging file into its final location under DOWNLOAD_ROOT.""" """Move a completed staging file into its final location under DOWNLOAD_ROOT."""
dest = abs_path(download_root, rel) dest = abs_path(download_root, rel)

View file

@ -7,7 +7,6 @@ serves finished files (range-aware, with the user's custom display name), and sh
""" """
import re import re
from datetime import datetime, timedelta, timezone from datetime import datetime, timedelta, timezone
from pathlib import Path
from urllib.parse import parse_qs, urlparse from urllib.parse import parse_qs, urlparse
from fastapi import APIRouter, Depends, HTTPException, Request 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 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: if asset is None or asset.status != "ready" or not asset.rel_path:
raise HTTPException(status_code=409, detail="This download isn't ready.") raise HTTPException(status_code=409, detail="This download isn't ready.")
path = storage.abs_path(settings.download_root, asset.rel_path).resolve() path = storage.safe_abs_path(settings.download_root, asset.rel_path)
root = Path(settings.download_root).resolve() if path is None:
if root not in path.parents or not path.exists():
raise HTTPException(status_code=404, detail="File is no longer available.") raise HTTPException(status_code=404, detail="File is no longer available.")
ext = asset.container or path.suffix.lstrip(".") 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 asset = db.get(MediaAsset, job.asset_id) if job.asset_id else None
if asset is None or not asset.poster_path: if asset is None or not asset.poster_path:
raise HTTPException(status_code=404, detail="No poster.") raise HTTPException(status_code=404, detail="No poster.")
path = storage.abs_path(settings.download_root, asset.poster_path).resolve() path = storage.safe_abs_path(settings.download_root, asset.poster_path)
root = Path(settings.download_root).resolve() if path is None:
if root not in path.parents or not path.exists():
raise HTTPException(status_code=404, detail="No poster.") raise HTTPException(status_code=404, detail="No poster.")
return FileResponse(path, media_type="image/jpeg") 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 asset = db.get(MediaAsset, job.asset_id) if job.asset_id else None
if asset is None or not asset.storyboard_path: if asset is None or not asset.storyboard_path:
raise HTTPException(status_code=404, detail="No filmstrip.") raise HTTPException(status_code=404, detail="No filmstrip.")
path = storage.abs_path(settings.download_root, asset.storyboard_path).resolve() path = storage.safe_abs_path(settings.download_root, asset.storyboard_path)
root = Path(settings.download_root).resolve() if path is None:
if root not in path.parents or not path.exists():
raise HTTPException(status_code=404, detail="No filmstrip.") raise HTTPException(status_code=404, detail="No filmstrip.")
return FileResponse(path, media_type="image/jpeg") return FileResponse(path, media_type="image/jpeg")

View file

@ -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 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 `<video src>` can't send a header). a short-lived signed grant that the media URL carries (a `<video src>` can't send a header).
""" """
from pathlib import Path
from fastapi import APIRouter, Depends, HTTPException from fastapi import APIRouter, Depends, HTTPException
from fastapi.responses import FileResponse from fastapi.responses import FileResponse
@ -42,9 +41,8 @@ def _ready_target(db: Session, link: DownloadLink) -> tuple[DownloadJob, MediaAs
raise HTTPException(status_code=404, detail="This video is no longer available.") raise HTTPException(status_code=404, detail="This video is no longer available.")
# Verify the file is actually on disk here (not just flagged ready), so the watch page never # Verify the file is actually on disk here (not just flagged ready), so the watch page never
# loads a player for a missing file — meta and file agree. # loads a player for a missing file — meta and file agree.
path = storage.abs_path(settings.download_root, asset.rel_path).resolve() path = storage.safe_abs_path(settings.download_root, asset.rel_path)
root = Path(settings.download_root).resolve() if path is None:
if root not in path.parents or not path.exists():
raise HTTPException(status_code=404, detail="This video is no longer available.") raise HTTPException(status_code=404, detail="This video is no longer available.")
return job, asset return job, asset
@ -116,9 +114,8 @@ def watch_file(token: str, g: str | None = None, db: Session = Depends(get_db)):
if link.password_hash and not linksmod.check_grant(token, g): if link.password_hash and not linksmod.check_grant(token, g):
raise HTTPException(status_code=403, detail="This link needs a password.") raise HTTPException(status_code=403, detail="This link needs a password.")
asset = _ready_asset(db, link) asset = _ready_asset(db, link)
path = storage.abs_path(settings.download_root, asset.rel_path).resolve() path = storage.safe_abs_path(settings.download_root, asset.rel_path)
root = Path(settings.download_root).resolve() if path is None:
if root not in path.parents or not path.exists():
raise HTTPException(status_code=404, detail="This video is no longer available.") raise HTTPException(status_code=404, detail="This video is no longer available.")
ext = asset.container or path.suffix.lstrip(".") ext = asset.container or path.suffix.lstrip(".")
@ -141,8 +138,7 @@ def watch_poster(token: str, g: str | None = None, db: Session = Depends(get_db)
job, asset = _ready_target(db, link) job, asset = _ready_target(db, link)
if not asset.poster_path: if not asset.poster_path:
raise HTTPException(status_code=404, detail="No poster.") raise HTTPException(status_code=404, detail="No poster.")
path = storage.abs_path(settings.download_root, asset.poster_path).resolve() path = storage.safe_abs_path(settings.download_root, asset.poster_path)
root = Path(settings.download_root).resolve() if path is None:
if root not in path.parents or not path.exists():
raise HTTPException(status_code=404, detail="No poster.") raise HTTPException(status_code=404, detail="No poster.")
return FileResponse(path, media_type="image/jpeg") return FileResponse(path, media_type="image/jpeg")