fix(downloads): harden upload serving + ingest against stored-XSS and leaks (E7 review)
Deep-review remediation for the E7 upload path, all findings adversarially verified: - Serve every library file with a Content-Type chosen from the ffprobe-verified media_kind + a strict container allow-list (storage.served_media_type), never guessed from the user-controlled file extension. Closes a stored-XSS vector where a media/HTML polyglot uploaded as "evil.html" was served inline as text/html on the login-free /watch surface. Applied to both the authenticated /file and the public watch_file endpoints. - Classify uploaded still images (jpg/png/gif/webp/…) as media_kind='file' so they are force-downloaded and don't get a broken in-app video player. - Roll back the on-disk side of an upload when the ingest transaction fails after place_file (unlink the placed file + its deterministic poster path), so a failed commit can't orphan bytes in the GC-skipped .uploads tree. - Floor download_upload_max_bytes at min=1 so a stored 0 can't silently disable the per-file cap. - Dedupe the ffprobe-JSON invocation into edit.run_ffprobe_json (shared by probe_codecs and upload._probe) and the upload extension into storage._clean_upload_ext; drop the now-unused parse_probe_streams. Tests: public + authenticated force-download and the polyglot-not-served-as-HTML guard, the real _probe classifier (image/cover-art/dims/duration), the ingest rollback/cleanup path, the max_jobs branch, the cap-floor invariant, and run_ffprobe_json error handling.
This commit is contained in:
@@ -6,10 +6,14 @@ size known up front, the per-file cap enforced while streaming, filename sanitiz
|
||||
guards that keep an upload out of paths that assume a re-fetchable source (edit / redownload).
|
||||
|
||||
DB-backed (real Postgres) — skipped when no database is reachable. `settings.download_root` is
|
||||
pointed at a per-test tmp dir so ingest's on-disk moves are isolated; `_probe` is monkeypatched so
|
||||
the tests don't depend on ffprobe (its real behaviour is exercised by the download/edit suites).
|
||||
pointed at a per-test tmp dir so ingest's on-disk moves are isolated. The ingest/route tests
|
||||
monkeypatch `_probe` so they don't shell out to ffprobe; the classifier's OWN parsing (image →
|
||||
file, cover-art skip, dims, duration) is covered directly by `TestProbeClassifier` below, which
|
||||
stubs one level down (`run_ffprobe_json`) and exercises the real `_probe`.
|
||||
"""
|
||||
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
from app.config import settings
|
||||
@@ -137,6 +141,77 @@ def test_upload_route_generic_forces_download(as_user, make_user, _root, monkeyp
|
||||
assert "attachment" in fr.headers["content-disposition"].lower()
|
||||
|
||||
|
||||
def test_upload_route_media_html_extension_not_served_as_html(
|
||||
as_user, make_user, _root, monkeypatch
|
||||
):
|
||||
"""The XSS guard: a MEDIA-classified upload (real codecs) named with an executable extension must
|
||||
NOT be served with a guessed text/html type. The served Content-Type comes from the verified
|
||||
media_kind + a container allow-list ('html' isn't one), so it force-downloads as octet-stream —
|
||||
on both the authenticated file endpoint AND the login-free public watch endpoint. Without the fix
|
||||
the public inline branch let a media/HTML polyglot named evil.html run as stored XSS."""
|
||||
_probe(monkeypatch, ("h264", "aac", 320, 240, 5)) # ffprobe says it's video…
|
||||
c = as_user(make_user())
|
||||
r = c.post(
|
||||
"/api/downloads/upload",
|
||||
files={"file": ("evil.html", b"<script>alert(1)</script>", "text/html")},
|
||||
)
|
||||
assert r.status_code == 200, r.text
|
||||
job = r.json()
|
||||
assert job["asset"]["media_kind"] == "video" and job["asset"]["container"] == "html"
|
||||
|
||||
# Authenticated endpoint: never text/html.
|
||||
fr = c.get(f"/api/downloads/{job['id']}/file")
|
||||
assert fr.status_code == 200
|
||||
assert fr.headers["content-type"].startswith("application/octet-stream")
|
||||
assert "attachment" in fr.headers["content-disposition"].lower()
|
||||
|
||||
# Public login-free endpoint via a STREAM-ONLY link (the inline branch) — still not text/html.
|
||||
link = c.post(f"/api/downloads/{job['id']}/links", json={"allow_download": False}).json()
|
||||
pr = c.get(f"/api/public/watch/{link['token']}/file")
|
||||
assert pr.status_code == 200
|
||||
assert not pr.headers["content-type"].startswith("text/html")
|
||||
assert pr.headers["content-type"].startswith("application/octet-stream")
|
||||
assert "attachment" in pr.headers["content-disposition"].lower()
|
||||
|
||||
|
||||
def test_public_watch_file_forces_download_for_generic_file(as_user, make_user, _root, monkeypatch):
|
||||
"""The login-free public serve of a generic upload (media_kind='file') must be a neutral
|
||||
force-download attachment — the branch that stops uploaded HTML/SVG executing on the public
|
||||
origin. This is a SEPARATE code path from the authenticated /file route, so it needs its own
|
||||
route-level assertion (previously only the authenticated branch was tested)."""
|
||||
_probe(monkeypatch, (None, None, None, None, None))
|
||||
c = as_user(make_user())
|
||||
job = c.post(
|
||||
"/api/downloads/upload",
|
||||
files={"file": ("notes.html", b"<script>alert(1)</script>", "text/html")},
|
||||
).json()
|
||||
assert job["asset"]["media_kind"] == "file"
|
||||
link = c.post(f"/api/downloads/{job['id']}/links", json={"allow_download": True}).json()
|
||||
pr = c.get(f"/api/public/watch/{link['token']}/file")
|
||||
assert pr.status_code == 200
|
||||
assert pr.headers["content-type"].startswith("application/octet-stream")
|
||||
assert "attachment" in pr.headers["content-disposition"].lower()
|
||||
|
||||
|
||||
def test_public_watch_file_serves_media_inline_with_real_type(
|
||||
as_user, make_user, _root, monkeypatch
|
||||
):
|
||||
"""A recognised media container IS still served inline with its real audio/video type (so it
|
||||
plays in the watch page) — the fix must not break legitimate playback. A stream-only link →
|
||||
inline; the type is video/mp4 (from the container allow-list), never text/* and never a guess."""
|
||||
_probe(monkeypatch, ("h264", "aac", 640, 360, 12))
|
||||
c = as_user(make_user())
|
||||
job = c.post(
|
||||
"/api/downloads/upload",
|
||||
files={"file": ("clip.mp4", b"fake-mp4-bytes", "video/mp4")},
|
||||
).json()
|
||||
link = c.post(f"/api/downloads/{job['id']}/links", json={"allow_download": False}).json()
|
||||
pr = c.get(f"/api/public/watch/{link['token']}/file")
|
||||
assert pr.status_code == 200
|
||||
assert pr.headers["content-type"].startswith("video/mp4")
|
||||
assert "inline" in pr.headers["content-disposition"].lower()
|
||||
|
||||
|
||||
def test_upload_route_enforces_per_file_cap(as_user, make_user, db, _root, monkeypatch):
|
||||
from app import sysconfig
|
||||
|
||||
@@ -151,6 +226,16 @@ def test_upload_route_enforces_per_file_cap(as_user, make_user, db, _root, monke
|
||||
assert r.json()["detail"]["code"] == "upload_too_large"
|
||||
|
||||
|
||||
def test_upload_cap_cannot_be_disabled_with_zero(db):
|
||||
"""download_upload_max_bytes floors at 1: a stored 0 would read falsy in the route's
|
||||
`if cap and size > cap` guard and SILENTLY disable the per-file cap, so set_value must reject it.
|
||||
Pins the min=1 invariant so a revert to min=0 can't pass the gate."""
|
||||
from app import sysconfig
|
||||
|
||||
with pytest.raises(ValueError):
|
||||
sysconfig.set_value(db, "download_upload_max_bytes", 0)
|
||||
|
||||
|
||||
def test_upload_route_rejects_empty_file(as_user, make_user, _root, monkeypatch):
|
||||
c = as_user(make_user())
|
||||
r = c.post(
|
||||
@@ -182,3 +267,128 @@ def test_edit_rejects_a_generic_upload_source(as_user, make_user, _root, monkeyp
|
||||
)
|
||||
assert r.status_code == 400
|
||||
assert r.json()["detail"]["reason"] == "not_media"
|
||||
|
||||
|
||||
def test_ingest_rejects_over_max_jobs(db, make_user, _root, monkeypatch):
|
||||
"""The other check_upload rejection branch (holdings count, not bytes): once the user is at
|
||||
max_jobs, a further upload is refused before any disk write."""
|
||||
_probe(monkeypatch, (None, None, None, None, None))
|
||||
u = make_user()
|
||||
db.add(
|
||||
DownloadQuota(user_id=u.id, max_bytes=10**12, max_concurrent=2, max_jobs=1, unlimited=False)
|
||||
)
|
||||
db.commit()
|
||||
src1 = _root / "a.bin"
|
||||
src1.write_bytes(b"a")
|
||||
uploadmod.ingest_upload(db, u.id, filename="a.bin", src_path=src1, size=1) # now 1 holding job
|
||||
src2 = _root / "b.bin"
|
||||
src2.write_bytes(b"b")
|
||||
with pytest.raises(quota.QuotaExceeded) as e:
|
||||
uploadmod.ingest_upload(db, u.id, filename="b.bin", src_path=src2, size=1)
|
||||
assert e.value.reason == "max_jobs"
|
||||
assert src2.exists() # rejected before the move — nothing landed
|
||||
|
||||
|
||||
def test_ingest_cleans_up_placed_files_on_commit_failure(db, make_user, _root, monkeypatch):
|
||||
"""A failure AFTER storage.place_file has moved the file onto disk must roll back AND unlink the
|
||||
placed file + any (even partial) poster — otherwise the orphan is permanent (the per-user delete
|
||||
keys off the rolled-back asset.rel_path, and the GC orphan sweep skips the dot-prefixed .uploads/
|
||||
tree). Exercises the rollback/cleanup branch that IS the leak remediation, incl. the poster case
|
||||
where ensure_poster left a partial file but returned None."""
|
||||
_probe(monkeypatch, ("h264", "aac", 320, 240, 5)) # video → the poster path runs
|
||||
u = make_user()
|
||||
|
||||
# Simulate ffmpeg leaving a partial poster on disk, then failing (ensure_poster returns None).
|
||||
def _partial_poster(asset, root):
|
||||
storage.abs_path(root, str(Path(asset.rel_path).with_suffix(".jpg"))).write_bytes(
|
||||
b"partial"
|
||||
)
|
||||
return None
|
||||
|
||||
monkeypatch.setattr(uploadmod.editmod, "ensure_poster", _partial_poster)
|
||||
|
||||
# Force the commit to fail after the file (and partial poster) are on disk.
|
||||
def _boom():
|
||||
raise RuntimeError("db down")
|
||||
|
||||
monkeypatch.setattr(db, "commit", _boom)
|
||||
|
||||
src = _root / "clip.mp4"
|
||||
src.write_bytes(b"fake-video-bytes")
|
||||
with pytest.raises(RuntimeError):
|
||||
uploadmod.ingest_upload(db, u.id, filename="clip.mp4", src_path=src, size=16)
|
||||
|
||||
db.rollback() # drop the monkeypatched-commit session state before querying
|
||||
uploads = _root / ".uploads"
|
||||
leftover = [p for p in uploads.rglob("*") if p.is_file()] if uploads.exists() else []
|
||||
assert leftover == [], f"orphaned files left on disk: {leftover}"
|
||||
assert db.query(MediaAsset).count() == 0 # the flushed asset rolled back — no dangling row
|
||||
|
||||
|
||||
# --- classifier: the REAL _probe parsing (stubs ffprobe one level down) ----------------------
|
||||
# The ingest/route tests above stub _probe wholesale; these exercise _probe's OWN logic — image
|
||||
# detection, the cover-art skip, dimension/duration parsing — by faking only run_ffprobe_json, so a
|
||||
# regression in the classifier the force-download guarantee leans on is actually caught.
|
||||
|
||||
|
||||
class TestProbeClassifier:
|
||||
def _probe_with(self, monkeypatch, data):
|
||||
monkeypatch.setattr(uploadmod.editmod, "run_ffprobe_json", lambda *a, **k: data)
|
||||
return uploadmod._probe(Path("x"))
|
||||
|
||||
def test_probe_failure_is_a_generic_file(self, monkeypatch):
|
||||
# run_ffprobe_json returns None (ffprobe failed / not media) → all None → derive() == FILE.
|
||||
v, a, w, h, d = self._probe_with(monkeypatch, None)
|
||||
assert (v, a, w, h, d) == (None, None, None, None, None)
|
||||
assert mediakind.derive(v, a) == mediakind.FILE
|
||||
|
||||
@pytest.mark.parametrize("fmt", ["image2", "png_pipe", "jpeg_pipe", "webp_pipe", "gif"])
|
||||
def test_still_image_classifies_as_file_not_video(self, monkeypatch, fmt):
|
||||
# A standalone image exposes a real video stream, but its container format is an image demuxer
|
||||
# — it must NOT become media_kind='video' (broken player + inline serving). Stays a file.
|
||||
data = {
|
||||
"format": {"format_name": fmt},
|
||||
"streams": [{"codec_type": "video", "codec_name": "png", "width": 8, "height": 8}],
|
||||
}
|
||||
v, a, w, h, d = self._probe_with(monkeypatch, data)
|
||||
assert v is None and mediakind.derive(v, a) == mediakind.FILE
|
||||
|
||||
def test_cover_art_video_stream_is_skipped_audio_stays_audio(self, monkeypatch):
|
||||
# Album art embedded in an audio file is a video stream flagged attached_pic — skip it so the
|
||||
# asset classifies as audio, not video.
|
||||
data = {
|
||||
"format": {"format_name": "mp3", "duration": "212.5"},
|
||||
"streams": [
|
||||
{
|
||||
"codec_type": "video",
|
||||
"codec_name": "mjpeg",
|
||||
"disposition": {"attached_pic": 1},
|
||||
"width": 600,
|
||||
"height": 600,
|
||||
},
|
||||
{"codec_type": "audio", "codec_name": "mp3"},
|
||||
],
|
||||
}
|
||||
v, a, w, h, d = self._probe_with(monkeypatch, data)
|
||||
assert (v, a, d) == (None, "mp3", 212)
|
||||
assert mediakind.derive(v, a) == mediakind.AUDIO
|
||||
|
||||
def test_real_video_records_codecs_dims_duration(self, monkeypatch):
|
||||
data = {
|
||||
"format": {"format_name": "mov,mp4,m4a", "duration": "60.0"},
|
||||
"streams": [
|
||||
{"codec_type": "video", "codec_name": "h264", "width": 1920, "height": 1080},
|
||||
{"codec_type": "audio", "codec_name": "aac"},
|
||||
],
|
||||
}
|
||||
v, a, w, h, d = self._probe_with(monkeypatch, data)
|
||||
assert (v, a, w, h, d) == ("h264", "aac", 1920, 1080, 60)
|
||||
assert mediakind.derive(v, a) == mediakind.VIDEO
|
||||
|
||||
def test_unparseable_duration_is_none(self, monkeypatch):
|
||||
data = {
|
||||
"format": {"format_name": "wav", "duration": "N/A"},
|
||||
"streams": [{"codec_type": "audio", "codec_name": "pcm_s16le"}],
|
||||
}
|
||||
v, a, w, h, d = self._probe_with(monkeypatch, data)
|
||||
assert d is None and mediakind.derive(v, a) == mediakind.AUDIO
|
||||
|
||||
Reference in New Issue
Block a user