From f5018bdcbc1bb745de48c512e49502dd9a786753 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 22 Sep 2026 01:59:03 +0200 Subject: [PATCH] fix(uploads): read the body after authenticating, not before it `POST /api/downloads/upload` declared `file: UploadFile = File(...)`, and FastAPI parses and spools a declared body before the first dependency runs -- `require_human` included. An unauthenticated caller could therefore make the process write the whole per-file allowance (500 MiB by default) to temp, once per request. SEC-20's ceiling bounded that number but could not stop the writing, which is why E27 parked it rather than closing it. The route now takes no body parameter. `spool_upload` names `require_human` first and only then streams `request.stream()` to the staging temp file, so a stranger is refused before a byte moves -- the shape the screenshot route has used since E16. Filename and display name travel as percent-encoded headers (latin-1 on the wire, so a Hungarian filename needs the encoding) instead of multipart parts. `capped_stream_to` joins `capped_body` in routes/_common.py: the same counting rule, written to a file descriptor rather than accumulated, because holding 500 MiB would trade a disk problem for a memory one. Its refusal is the caller's, so the upload route keeps answering 422 `upload_too_large` with the admin knob's own number. With no envelope to allow for, the edge is now exactly that knob, and MULTIPART_ENVELOPE_ALLOWANCE is gone. The invariant stops being prose: test_body_limit.py asks FastAPI itself (`route.dependant.body_params`) and fails the gate if any route with a raised ceiling declares a body. A comment could not have prevented the defect it described; that test would have. Both new tests were seen red for the right reason -- before the fix the anonymous probe got its 401 only after the server had read 4236 bytes of a multipart body, and re-declaring the File parameter turns the invariant test red naming the route and the parameter. --- backend/app/bodylimit.py | 41 +++--- backend/app/routes/_common.py | 26 +++- backend/app/routes/downloads.py | 136 ++++++++++++++------ backend/tests/conftest.py | 22 ++++ backend/tests/test_body_limit.py | 53 +++++++- backend/tests/test_public_unlock_limit.py | 4 +- backend/tests/test_upload.py | 149 ++++++++++++++++++---- frontend/src/lib/api/core.test.ts | 28 +++- frontend/src/lib/api/core.ts | 31 +++-- frontend/src/lib/api/downloads.ts | 15 ++- 10 files changed, 396 insertions(+), 109 deletions(-) diff --git a/backend/app/bodylimit.py b/backend/app/bodylimit.py index 0008ee62..9bf8ce33 100644 --- a/backend/app/bodylimit.py +++ b/backend/app/bodylimit.py @@ -53,31 +53,32 @@ DEFAULT_MAX_BODY_BYTES = 1024 * 1024 # # These are RAISED CEILINGS, NOT EXEMPTIONS, and the difference is the whole point. An exemption # would mean "this route is counted by something else" — which is only true when the route's own -# limit applies to the bytes as they ARRIVE. That holds for the screenshot route (no declared body -# parameter: `capped_body` streams it, and the owner check resolves first) and it does NOT hold for -# the upload route: `file: UploadFile = File(...)` is a declared body, so FastAPI parses and spools -# the whole multipart part to a temp file BEFORE the first dependency runs — `require_human` -# included. Its in-handler cap therefore measures bytes that have already landed. Left uncounted, -# an anonymous caller could stream without bound into the container's tmpfs; counted here, the -# process refuses at the same number the route itself enforces, before any of it is stored. +# limit applies to the bytes as they ARRIVE. That, in turn, is only true of a route with NO DECLARED +# BODY PARAMETER: FastAPI parses and spools a declared body BEFORE the first dependency runs, so a +# route that declares one has already stored the bytes by the time its own cap — or its auth — gets +# a say. +# +# Both listed routes now satisfy that. The screenshot route always did (`capped_body` streams it, +# and the owner check resolves first). The upload route did NOT until P1: it declared +# `file: UploadFile = File(...)`, so an anonymous caller could make the process write the whole +# per-file allowance to temp before `require_human` refused them — this middleware bounded that +# number but could not stop the writing. It now reads its own body from a dependency declared after +# the auth one, so its in-handler cap measures bytes as they arrive, like everything else here. +# +# That rule is no longer only written here: `test_body_limit.py` parses the routes and fails the +# gate if any route in this dict declares a body parameter. A comment could not have stopped the +# defect it describes; the test can. # # A route belongs here ONLY IF ITS OWN CAP IS LARGER THAN THE DEFAULT. That is why # `POST /api/diagnostics/events` is absent: its `capped_body` limit is 512 KiB, already below the # default, so listing it could only widen what it accepts. -# What a multipart envelope costs on top of the file itself: the boundary lines, the part headers -# and the sibling `display_name` field. The edge counts RAW BODY bytes while the upload route counts -# the DECODED file, so without this allowance a file of exactly the configured maximum — the number -# the admin page advertises, and the first thing anyone testing the limit reaches for — is refused -# one layer before the route that would have accepted it. Generous on purpose: its job is to keep -# the two layers from disagreeing at the boundary, not to be a second limit. -MULTIPART_ENVELOPE_ALLOWANCE = 64 * 1024 - ROUTE_MAX_BODY_BYTES: dict[tuple[str, str], int] = { - # Ingesting a local video file. `sysconfig`'s `download_upload_max_bytes` is bounded so it can - # never exceed this (see sysconfig.SPECS), which is what stops an admin from raising the route's - # own cap past the edge and collecting 413s from a layer the admin page never mentions. - ("POST", "/api/downloads/upload"): settings.download_upload_max_bytes - + MULTIPART_ENVELOPE_ALLOWANCE, + # Ingesting a local file. The body IS the file since P1 — no multipart envelope to allow for — + # so this is exactly the number the route enforces. `sysconfig`'s `download_upload_max_bytes` is + # bounded so it can never exceed it (see sysconfig.SPECS), which is what stops an admin from + # raising the route's own cap past the edge and collecting 413s from a layer the admin page + # never mentions. + ("POST", "/api/downloads/upload"): settings.download_upload_max_bytes, ("POST", "/api/debug-sessions/{sid}/screenshots"): debugsession.SCREENSHOT_MAX_BYTES, } diff --git a/backend/app/routes/_common.py b/backend/app/routes/_common.py index b7e99a55..19205df3 100644 --- a/backend/app/routes/_common.py +++ b/backend/app/routes/_common.py @@ -6,7 +6,7 @@ and "fetch a row this user owns or 404" idioms have one definition, one status c one existence-leak policy. """ -from typing import Any, TypeVar +from typing import IO, Any, TypeVar from fastapi import HTTPException, Request from sqlalchemy.orm import Session @@ -79,6 +79,30 @@ async def capped_body(request: Request, limit: int) -> bytes: return bytes(body) +async def capped_stream_to( + request: Request, out: IO[bytes], limit: int, refusal: HTTPException +) -> int: + """`capped_body`'s sibling for a body too large to hold: the same counting rule, written + straight to `out` instead of accumulated, returning the size. + + Two deliberate differences from `capped_body`. It never builds a `bytearray` — the upload route + admits 500 MiB by default, so holding the body would trade a disk problem for a memory one. And + the refusal is the CALLER'S, passed in rather than assumed: `capped_body`'s flat 413 is right + for a screenshot, while the upload route answers 422 `upload_too_large` carrying the admin + knob's own number, which is what its dialog shows the user. One counting rule, two answers, + neither of them a second copy of the loop. + + The caller owns `out` and owns cleaning it up: a body refused half-way has already written what + arrived before the limit was passed.""" + size = 0 + async for chunk in request.stream(): + size += len(chunk) + if size > limit: + raise refusal + out.write(chunk) + return size + + def issue_refused(exc: gitea.GiteaError) -> HTTPException: """The refusal an issue-tracker failure (E16) answers as — the send, the preview and the admin's connection test alike. Literal reasons, one per case: the frontend's contract test reads diff --git a/backend/app/routes/downloads.py b/backend/app/routes/downloads.py index 90e36197..c71380b4 100644 --- a/backend/app/routes/downloads.py +++ b/backend/app/routes/downloads.py @@ -9,11 +9,13 @@ serves finished files (range-aware, with the user's custom display name), and sh import os import re import tempfile +from collections.abc import AsyncIterator +from dataclasses import dataclass from datetime import datetime, timedelta, timezone from pathlib import Path -from urllib.parse import parse_qs, urlparse +from urllib.parse import parse_qs, unquote, urlparse -from fastapi import APIRouter, Depends, File, Form, HTTPException, Request, UploadFile +from fastapi import APIRouter, Depends, HTTPException, Request from fastapi.responses import FileResponse from pydantic import BaseModel from sqlalchemy import func, select @@ -38,7 +40,7 @@ from app.models import ( MediaAsset, User, ) -from app.routes._common import get_or_404, not_found, owned_or_404 +from app.routes._common import capped_stream_to, get_or_404, not_found, owned_or_404 from app.security import hash_password from app.userscope import is_messageable_user, messageable_clauses from app.utils import iso @@ -406,61 +408,113 @@ def enqueue_download( return _serialize_job(db, job) -# Read the upload in 1 MiB chunks so a large file never sits fully in memory, and the size cap is -# enforced on the ACTUAL bytes read (a client-set Content-Length can't be trusted to gate it). -_UPLOAD_CHUNK = 1024 * 1024 +# The upload's filename and display name travel as HEADERS, not as multipart parts. That is not a +# style choice: a DECLARED body parameter (`file: UploadFile = File(...)`) is read and spooled by +# FastAPI *before the first dependency runs*, so `require_human` could not gate it and an anonymous +# caller could make the process write the whole per-file allowance to temp, per request (P1; SEC-20 +# bounded the number but could not stop the writing). Reading the body ourselves, from a dependency +# declared AFTER the auth one, is the only shape that closes it — the same one +# `POST /api/debug-sessions/{sid}/screenshots` uses, and `test_body_limit.py` now enforces it for +# every route with a raised ceiling rather than leaving it to a comment like this one. +# +# Headers are latin-1 on the wire, so both are percent-encoded by the client and unquoted here; a +# Hungarian filename would otherwise be unsendable. +UPLOAD_FILENAME_HEADER = "x-upload-filename" +UPLOAD_DISPLAY_NAME_HEADER = "x-upload-display-name" -@router.post("/upload") -def upload_download( - file: UploadFile = File(...), - display_name: str | None = Form(None), +@dataclass(frozen=True) +class SpooledUpload: + """What the body dependency hands the handler: the bytes already on disk, plus the two names + that came alongside them.""" + + path: Path + size: int + user_id: int + filename: str + display_name: str | None + + +def _header_text(request: Request, name: str) -> str: + """One percent-encoded header, decoded. A header that is not valid UTF-8 once unquoted is the + caller's doing, not a server fault — it degrades to empty rather than 500ing, and the route's + own fallback names the file.""" + raw = request.headers.get(name, "") + try: + return unquote(raw, errors="strict").strip() + except (UnicodeDecodeError, ValueError): + # silent-ok: a mis-encoded header is the caller's own malformed request, and this one is + # cosmetic — it names the file. Refusing the upload over it would fail a 500 MiB transfer on + # a label; logging it would hand any client a free log line per request. + return "" + + +async def spool_upload( + request: Request, user: User = Depends(require_human), db: Session = Depends(get_db), -) -> dict: - """Ingest a local file into the caller's library (E7 S5). Any file type is accepted; media - (video/audio) gets the full function set, a generic file is download-only. The file is streamed - to a temp path under the download volume (enforcing the per-file cap as it goes), then handed to - the non-yt-dlp ingest which probes + places it and returns an already-`done` job. +) -> AsyncIterator[SpooledUpload]: + """Stream the request body to a staging temp file, enforcing the per-file cap as it arrives. - A sync (threadpool) handler on purpose: the ingest does blocking work — an ffprobe of up to 30s - plus sync DB writes — so it must NOT run on the event loop. `file.file` is the sync file object - Starlette spools the multipart body into.""" + `user` is taken but unused: it is the whole point. Dependencies resolve in declaration order, so + naming `require_human` here makes the auth check a precondition of reading the body at all — a + stranger is refused before a byte moves. The `yield` shape owns the temp file's life: ingest + MOVES it into place on success (the unlink is then a no-op), and on any other exit — cap hit, + empty, quota, probe or move error, a client that vanished mid-upload — it is removed.""" cap = sysconfig.get_int(db, "download_upload_max_bytes") staging = Path(settings.download_root) / ".staging" / "uploads" staging.mkdir(parents=True, exist_ok=True) fd, tmp = tempfile.mkstemp(dir=str(staging)) tmp_path = Path(tmp) try: - size = 0 with os.fdopen(fd, "wb") as out: - while chunk := file.file.read(_UPLOAD_CHUNK): - size += len(chunk) - if cap and size > cap: - raise HTTPException( - status_code=422, detail={"code": "upload_too_large", "limit": cap} - ) - out.write(chunk) - if size == 0: - raise HTTPException(status_code=400, detail={"code": "upload_empty"}) - try: - job = uploadmod.ingest_upload( - db, - user.id, - filename=file.filename or "upload", - src_path=tmp_path, - size=size, - display_name=(display_name or "").strip() or None, + size = await capped_stream_to( + request, + out, + cap, + HTTPException(status_code=422, detail={"code": "upload_too_large", "limit": cap}), ) - except quota.QuotaExceeded as e: - raise HTTPException(status_code=422, detail=_quota_detail(e)) - return _serialize_job(db, job) + yield SpooledUpload( + path=tmp_path, + size=size, + user_id=user.id, + filename=_header_text(request, UPLOAD_FILENAME_HEADER) or "upload", + display_name=_header_text(request, UPLOAD_DISPLAY_NAME_HEADER) or None, + ) finally: - # Ingest MOVES the temp file into place on success (unlink is then a no-op); on any early exit - # — cap hit, empty, quota, probe/move error — this removes the abandoned staging file. tmp_path.unlink(missing_ok=True) +@router.post("/upload") +def upload_download( + upload: SpooledUpload = Depends(spool_upload), + db: Session = Depends(get_db), +) -> dict: + """Ingest a local file into the caller's library (E7 S5). Any file type is accepted; media + (video/audio) gets the full function set, a generic file is download-only. The body IS the file + (see the header note above); `spool_upload` has already authenticated the caller and streamed it + to a temp path under the download volume, so what is left here is the non-yt-dlp ingest which + probes + places it and returns an already-`done` job. + + A sync (threadpool) handler on purpose: the ingest does blocking work — an ffprobe of up to 30s + plus sync DB writes — so it must NOT run on the event loop. The streaming half is async because + reading the body is; keeping them in separate callables is what lets each run where it belongs.""" + if upload.size == 0: + raise HTTPException(status_code=400, detail={"code": "upload_empty"}) + try: + job = uploadmod.ingest_upload( + db, + upload.user_id, + filename=upload.filename, + src_path=upload.path, + size=upload.size, + display_name=upload.display_name, + ) + except quota.QuotaExceeded as e: + raise HTTPException(status_code=422, detail=_quota_detail(e)) + return _serialize_job(db, job) + + class EnqueueEditIn(BaseModel): source_job_id: int = 0 edit_spec: dict diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py index 1c817f11..c25be444 100644 --- a/backend/tests/conftest.py +++ b/backend/tests/conftest.py @@ -15,6 +15,7 @@ tests that COMMIT (the whole point of this lane — transaction-ownership behavi import os import shutil +from urllib.parse import quote import pytest from alembic import command @@ -248,6 +249,27 @@ def as_user(db, override_dep, _configured): return _as +@pytest.fixture +def upload_headers(): + """Build the headers `POST /api/downloads/upload` takes since P1. + + Shared, because two test modules drive that route and a second copy of this is exactly the drift + that lets the wire shape change in one place only. The body IS the file now; the names ride as + percent-encoded headers (`quote` mirrors the client's `encodeURIComponent`, since headers are + latin-1 and a Hungarian filename would not survive raw).""" + + def _headers(filename: str, display_name: str | None = None) -> dict[str, str]: + headers = { + "content-type": "application/octet-stream", + "x-upload-filename": quote(filename), + } + if display_name is not None: + headers["x-upload-display-name"] = quote(display_name) + return headers + + return _headers + + @pytest.fixture(autouse=True) def _reset_rate_limiters(): """Every RateLimiter is a module-level singleton keyed by client IP, and TestClient always diff --git a/backend/tests/test_body_limit.py b/backend/tests/test_body_limit.py index 05bebba7..d19bf2ec 100644 --- a/backend/tests/test_body_limit.py +++ b/backend/tests/test_body_limit.py @@ -251,14 +251,57 @@ def test_the_upload_ceiling_and_the_admin_knob_cannot_drift_apart(): constant. If the knob could be set above the edge, an admin raising it would silently start getting 413s from a layer the admin page never mentions. - The relation is `<=`, not `==`, and the gap is the point: the edge counts RAW body bytes while - the route counts the DECODED file, so the edge has to allow for the multipart envelope or a file - of exactly the advertised maximum is refused one layer before the route that would take it. + Since P1 the two count the SAME bytes — the body is the file, with no multipart envelope + wrapped round it — so the old allowance is gone and the edge is the knob's own maximum, + exactly. A gap reappearing means someone re-wrapped the upload in an envelope without saying so. """ spec = next(s for s in sysconfig.SPECS if s.key == "download_upload_max_bytes") edge = bodylimit.ROUTE_MAX_BODY_BYTES[("POST", "/api/downloads/upload")] - assert spec.max is not None and spec.max <= edge - assert edge - spec.max == bodylimit.MULTIPART_ENVELOPE_ALLOWANCE + assert spec.max == edge + + +def _route_objects(root): + """`_declared_routes`' sibling, keeping the route OBJECT so its FastAPI dependant can be read.""" + found: dict[tuple[str, str], object] = {} + stack = [root] + while stack: + node = stack.pop() + for route in getattr(node, "routes", ()) or (): + inner = getattr(route, "original_router", None) + if inner is not None: + stack.append(inner) + continue + for method in getattr(route, "methods", None) or (): + found[(method, route.path)] = route + return found + + +@pytest.mark.parametrize(("method", "template"), sorted(bodylimit.ROUTE_MAX_BODY_BYTES)) +def test_no_route_with_a_raised_ceiling_declares_a_body_parameter(method, template): + """P1's invariant, and the reason this is a test rather than a comment. + + A raised ceiling is only honest when the route's own cap applies to the bytes AS THEY ARRIVE. + FastAPI reads and spools a DECLARED body (`= File(...)`, `= Form(...)`, a Pydantic model) + BEFORE the first dependency resolves — so on such a route this middleware's ceiling is the only + thing between an anonymous caller and the whole allowance, `require_human` never gets a say, and + the route's in-handler cap measures bytes that have already landed. That is exactly what P1 + closed on the upload route, which sat in `ROUTE_MAX_BODY_BYTES` with a 500 MiB ceiling and a + `file: UploadFile = File(...)` body for one release. + + Asked of FastAPI itself (`route.dependant.body_params`) rather than parsed or grepped: the + router already knows which parameters it will read the body for, and a shape a regex or an AST + walk guesses wrong is precisely how the next route slips through.""" + from app.main import app + + route = _route_objects(app).get((method, template)) + assert route is not None, f"{method} {template} is not a route any more" + body_params = [p.name for p in getattr(route.dependant, "body_params", ())] + assert not body_params, ( + f"{method} {template} declares a body parameter ({', '.join(body_params)}), so FastAPI " + "spools it before the first dependency runs and the raised ceiling is all that stands " + "between an anonymous caller and the whole allowance. Read the body yourself, from a " + "dependency declared AFTER the auth one — see routes/downloads.py:spool_upload." + ) def test_a_stored_override_outside_the_bounds_falls_back_on_READ(db): diff --git a/backend/tests/test_public_unlock_limit.py b/backend/tests/test_public_unlock_limit.py index ffbdebd0..816575a2 100644 --- a/backend/tests/test_public_unlock_limit.py +++ b/backend/tests/test_public_unlock_limit.py @@ -18,12 +18,12 @@ _PW = "open sesame" @pytest.fixture -def unlock_url(as_user, make_user, tmp_path, monkeypatch): +def unlock_url(as_user, make_user, tmp_path, monkeypatch, upload_headers): monkeypatch.setattr(settings, "download_root", str(tmp_path)) monkeypatch.setattr(probemod, "probe_media", lambda _p: ("h264", "aac", 320, 240, 5)) owner = as_user(make_user()) job = owner.post( - "/api/downloads/upload", files={"file": ("clip.mp4", b"fake-video", "video/mp4")} + "/api/downloads/upload", content=b"fake-video", headers=upload_headers("clip.mp4") ) assert job.status_code == 200, job.text link = owner.post(f"/api/downloads/{job.json()['id']}/links", json={"password": _PW}) diff --git a/backend/tests/test_upload.py b/backend/tests/test_upload.py index d1e8c9c2..4e983b5a 100644 --- a/backend/tests/test_upload.py +++ b/backend/tests/test_upload.py @@ -123,15 +123,106 @@ def test_ingest_rejects_over_footprint_cap_before_writing(db, make_user, _root, assert db.query(MediaAsset).count() == 0 # and no orphan asset row +# --- route: the auth-first body (P1) -------------------------------------------------------- + + +def _counting_body(chunks: int = 4, size: int = 1024): + """A request body that records how much of itself the server actually pulled. + + httpx's ASGI transport feeds the app lazily, one `http.request` message per yielded chunk, so a + chunk that was never yielded is a chunk the app never asked for. That is the whole assertion + below: it is not "the upload was refused", it is "the refusal happened before the bytes moved". + """ + pulled: list[int] = [] + + def body(): + for _ in range(chunks): + pulled.append(size) + yield b"x" * size + + return body(), pulled + + +_BOUNDARY = "p1boundary" + + +def _counting_multipart(chunks: int = 4, size: int = 1024): + """The same counter, wrapped in a real multipart envelope. + + The content-type is the load-bearing part. Starlette's `request.form()` returns an EMPTY + FormData without touching the stream when the content-type is not multipart/urlencoded — so a + raw-body probe cannot tell "the route stopped reading strangers" apart from "there was nothing + here to parse". Only a well-formed multipart body makes the old shape actually spool. + """ + pulled: list[int] = [] + head = ( + f"--{_BOUNDARY}\r\n" + 'Content-Disposition: form-data; name="file"; filename="clip.mp4"\r\n' + "Content-Type: application/octet-stream\r\n\r\n" + ).encode() + tail = f"\r\n--{_BOUNDARY}--\r\n".encode() + + def body(): + pulled.append(len(head)) + yield head + for _ in range(chunks): + pulled.append(size) + yield b"x" * size + pulled.append(len(tail)) + yield tail + + return body(), pulled + + +def test_an_anonymous_upload_is_refused_before_a_byte_of_it_is_read(client): + """P1 — the defect E27 bounded but did not close. + + While the route declared `file: UploadFile = File(...)`, FastAPI parsed and spooled the body + BEFORE the first dependency ran, so `require_human` could not gate it: an unauthenticated + caller could make the server write the whole per-file allowance (500 MiB by default) to temp, + per request. SEC-20's ceiling capped the number; it could not stop the writing. + + The route now reads its own body from a dependency declared AFTER the auth one, the shape + `POST /api/debug-sessions/{sid}/screenshots` already uses. The claim under test is the strong + one — a stranger's bytes are never read WHATEVER they claim to send, so the probe sends the + multipart the old route would have spooled, not the raw body the new one expects.""" + body, pulled = _counting_multipart() + r = client.post( + "/api/downloads/upload", + content=body, + headers={"content-type": f"multipart/form-data; boundary={_BOUNDARY}"}, + ) + assert r.status_code == 401 + assert pulled == [], f"the server read {sum(pulled)} bytes from an anonymous caller" + + +def test_an_authenticated_upload_still_has_its_body_read(as_user, make_user, _root, monkeypatch): + """The mutation check for the test above: a route that read NOBODY's body would pass it just as + well as the fixed one. The same counting body, signed in, must be pulled in full.""" + _probe(monkeypatch, (None, None, None, None, None)) + body, pulled = _counting_body() + c = as_user(make_user()) + r = c.post( + "/api/downloads/upload", + content=body, + headers={"content-type": "application/octet-stream", "x-upload-filename": "clip.bin"}, + ) + assert r.status_code == 200, r.text + assert sum(pulled) == 4096 + + # --- route ---------------------------------------------------------------------------------- -def test_upload_route_generic_forces_download(as_user, make_user, _root, monkeypatch): +def test_upload_route_generic_forces_download( + as_user, make_user, _root, monkeypatch, upload_headers +): _probe(monkeypatch, (None, None, None, None, None)) c = as_user(make_user()) r = c.post( "/api/downloads/upload", - files={"file": ("report.html", b"", "text/html")}, + content=b"", + headers=upload_headers("report.html"), ) assert r.status_code == 200, r.text job = r.json() @@ -145,7 +236,7 @@ def test_upload_route_generic_forces_download(as_user, make_user, _root, monkeyp def test_upload_route_media_html_extension_not_served_as_html( - as_user, make_user, _root, monkeypatch + as_user, make_user, _root, monkeypatch, upload_headers ): """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 @@ -156,7 +247,8 @@ def test_upload_route_media_html_extension_not_served_as_html( c = as_user(make_user()) r = c.post( "/api/downloads/upload", - files={"file": ("evil.html", b"", "text/html")}, + content=b"", + headers=upload_headers("evil.html"), ) assert r.status_code == 200, r.text job = r.json() @@ -177,7 +269,9 @@ def test_upload_route_media_html_extension_not_served_as_html( assert "attachment" in pr.headers["content-disposition"].lower() -def test_public_watch_file_forces_download_for_generic_file(as_user, make_user, _root, monkeypatch): +def test_public_watch_file_forces_download_for_generic_file( + as_user, make_user, _root, monkeypatch, upload_headers +): """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 @@ -186,7 +280,8 @@ def test_public_watch_file_forces_download_for_generic_file(as_user, make_user, c = as_user(make_user()) job = c.post( "/api/downloads/upload", - files={"file": ("notes.html", b"", "text/html")}, + content=b"", + headers=upload_headers("notes.html"), ).json() assert job["asset"]["media_kind"] == "file" link = c.post(f"/api/downloads/{job['id']}/links", json={"allow_download": True}).json() @@ -197,7 +292,7 @@ def test_public_watch_file_forces_download_for_generic_file(as_user, make_user, def test_public_watch_file_serves_media_inline_with_real_type( - as_user, make_user, _root, monkeypatch + as_user, make_user, _root, monkeypatch, upload_headers ): """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 → @@ -206,7 +301,8 @@ def test_public_watch_file_serves_media_inline_with_real_type( c = as_user(make_user()) job = c.post( "/api/downloads/upload", - files={"file": ("clip.mp4", b"fake-mp4-bytes", "video/mp4")}, + content=b"fake-mp4-bytes", + headers=upload_headers("clip.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") @@ -215,7 +311,9 @@ def test_public_watch_file_serves_media_inline_with_real_type( assert "inline" in pr.headers["content-disposition"].lower() -def test_upload_route_enforces_per_file_cap(as_user, make_user, db, _root, monkeypatch): +def test_upload_route_enforces_per_file_cap( + as_user, make_user, db, _root, monkeypatch, upload_headers +): from app import sysconfig sysconfig.set_value(db, "download_upload_max_bytes", 5) @@ -223,47 +321,48 @@ def test_upload_route_enforces_per_file_cap(as_user, make_user, db, _root, monke c = as_user(make_user()) r = c.post( "/api/downloads/upload", - files={"file": ("big.bin", b"way over the cap", "application/octet-stream")}, + content=b"way over the cap", + headers=upload_headers("big.bin"), ) assert r.status_code == 422 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.""" + """download_upload_max_bytes floors at 1, and set_value must enforce it. + + The falsy-0 footgun this originally guarded is gone — the route's old `if cap and size > cap` + read a stored 0 as "no cap at all", while `capped_stream_to` compares unconditionally and a 0 + would refuse everything instead. Refusing everything is the safe direction, but it is still + wrong, and the knob is also the number the admin page and the 422 `limit` advertise. 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): +def test_upload_route_rejects_empty_file(as_user, make_user, _root, monkeypatch, upload_headers): c = as_user(make_user()) - r = c.post( - "/api/downloads/upload", files={"file": ("empty.bin", b"", "application/octet-stream")} - ) + r = c.post("/api/downloads/upload", content=b"", headers=upload_headers("empty.bin")) assert r.status_code == 400 assert r.json()["detail"]["code"] == "upload_empty" -def test_redownload_rejects_an_upload(as_user, make_user, _root, monkeypatch): +def test_redownload_rejects_an_upload(as_user, make_user, _root, monkeypatch, upload_headers): _probe(monkeypatch, (None, None, None, None, None)) c = as_user(make_user()) - job = c.post( - "/api/downloads/upload", files={"file": ("a.bin", b"data", "application/octet-stream")} - ).json() + job = c.post("/api/downloads/upload", content=b"data", headers=upload_headers("a.bin")).json() r = c.post(f"/api/downloads/{job['id']}/redownload") assert r.status_code == 409 -def test_edit_rejects_a_generic_upload_source(as_user, make_user, _root, monkeypatch): +def test_edit_rejects_a_generic_upload_source( + as_user, make_user, _root, monkeypatch, upload_headers +): _probe(monkeypatch, (None, None, None, None, None)) c = as_user(make_user()) - job = c.post( - "/api/downloads/upload", files={"file": ("a.bin", b"data", "application/octet-stream")} - ).json() + job = c.post("/api/downloads/upload", content=b"data", headers=upload_headers("a.bin")).json() r = c.post( "/api/downloads/edit", json={"source_job_id": job["id"], "edit_spec": {"trim": {"start_s": 0, "end_s": 1}}}, diff --git a/frontend/src/lib/api/core.test.ts b/frontend/src/lib/api/core.test.ts index 2f23ca1f..ff9bf1de 100644 --- a/frontend/src/lib/api/core.test.ts +++ b/frontend/src/lib/api/core.test.ts @@ -23,6 +23,7 @@ vi.mock("../activeAccount", () => ({ })); import { setDebugSession } from "../log"; +import { downloadsApi } from "./downloads"; import { beacon, req, @@ -46,6 +47,7 @@ class FakeXHR { onerror: (() => void) | null = null; onabort: (() => void) | null = null; sent = false; + body: unknown = null; constructor() { FakeXHR.last = this; } @@ -53,8 +55,9 @@ class FakeXHR { setRequestHeader(k: string, v: string) { this.headers[k] = v; } - send() { + send(body?: unknown) { this.sent = true; + this.body = body ?? null; } abort() { this.onabort?.(); @@ -384,3 +387,26 @@ describe("the debug session a tab records into", () => { expect(sentHeaders().has("X-Debug-Session")).toBe(false); }); }); + +// P1 — the upload's wire shape is a security boundary, not a detail. A multipart part is a DECLARED +// body, which the server reads and spools BEFORE its auth dependency runs; the raw body is what lets +// `require_human` resolve first. A revert to FormData here would silently re-open that, and the +// backend's own test only sees the shape the client actually sends. +describe("uploadDownload's wire shape", () => { + it("sends the File as the raw body, with the names as percent-encoded headers", () => { + const file = new File(["abc"], "nyaralás videó.mp4", { type: "video/mp4" }); + void downloadsApi.uploadDownload(file, { displayName: "Nyaralás 2026" }); + const x = FakeXHR.last; + expect(x.body).toBe(file); + expect(x.body).not.toBeInstanceOf(FormData); + expect(x.headers["X-Upload-Filename"]).toBe(encodeURIComponent("nyaralás videó.mp4")); + expect(x.headers["X-Upload-Display-Name"]).toBe(encodeURIComponent("Nyaralás 2026")); + // Content-Type is the browser's to set from the File — an explicit one here would be a guess. + expect(x.headers["Content-Type"]).toBeUndefined(); + }); + + it("omits the display-name header when there is no display name", () => { + void downloadsApi.uploadDownload(new File(["abc"], "clip.mp4")); + expect(FakeXHR.last.headers["X-Upload-Display-Name"]).toBeUndefined(); + }); +}); diff --git a/frontend/src/lib/api/core.ts b/frontend/src/lib/api/core.ts index 5d8f69d3..f567802a 100644 --- a/frontend/src/lib/api/core.ts +++ b/frontend/src/lib/api/core.ts @@ -372,16 +372,27 @@ export async function req( } } -// Multipart upload with progress. `req` rides `fetch`, which can't report upload progress, so a -// file upload uses XHR instead: the caller gets a 0–100% callback as the body streams and an -// optional AbortSignal to cancel a large upload. Everything else mirrors `req` — the per-tab -// account header, credentials, the structured-detail localization, the global error modal for -// 4xx/5xx, and an `HttpError` throw — so callers handle failures exactly as they do a `req` call. -// The browser sets the multipart Content-Type (with its boundary), so we deliberately don't. +// Upload with progress. `req` rides `fetch`, which can't report upload progress, so a file upload +// uses XHR instead: the caller gets a 0–100% callback as the body streams and an optional +// AbortSignal to cancel a large upload. Everything else mirrors `req` — the per-tab account header, +// credentials, the structured-detail localization, the global error modal for 4xx/5xx, and an +// `HttpError` throw — so callers handle failures exactly as they do a `req` call. +// +// `body` is a `Blob`/`File` for a raw upload or `FormData` for a multipart one. The raw shape is +// what `/api/downloads/upload` uses since P1, and it is not a preference: a multipart part is a +// DECLARED body, which FastAPI spools before the route's auth dependency runs. `cfg.headers` is how +// such a caller sends the metadata that used to be sibling form fields. +// +// Content-Type is never set here. The browser derives it — the multipart boundary for FormData, the +// Blob's own type otherwise — and overriding it would break the boundary. export function uploadReq( url: string, - form: FormData, - cfg: { onProgress?: (pct: number) => void; signal?: AbortSignal } = {}, + body: Blob | FormData, + cfg: { + onProgress?: (pct: number) => void; + signal?: AbortSignal; + headers?: Record; + } = {}, ): Promise { return new Promise((resolve, reject) => { const xhr = new XMLHttpRequest(); @@ -390,7 +401,7 @@ export function uploadReq( const requestId = newRequestId(); const started = performance.now(); xhr.setRequestHeader(REQUEST_ID_HEADER, requestId); - for (const [name, value] of Object.entries(identityHeaders())) + for (const [name, value] of Object.entries({ ...identityHeaders(), ...cfg.headers })) xhr.setRequestHeader(name, value); if (cfg.onProgress) { xhr.upload.onprogress = (e) => { @@ -443,6 +454,6 @@ export function uploadReq( logRequest(requestId, "POST", url, started, { failed: "aborted" }); reject(new HttpError(0, ABORTED)); }; - xhr.send(form); + xhr.send(body); }); } diff --git a/frontend/src/lib/api/downloads.ts b/frontend/src/lib/api/downloads.ts index 1e07e265..f0f8584e 100644 --- a/frontend/src/lib/api/downloads.ts +++ b/frontend/src/lib/api/downloads.ts @@ -187,16 +187,23 @@ export const downloadsApi = { // Ingest a local file into the library (E7 S5). Multipart via XHR so the dialog can show upload // progress; any file type is accepted (media gets the full function set, a generic file is // download-only). Returns the already-`done` job the ingest creates. + // The body IS the file, and the two names ride as headers. Not a style choice: a multipart part is + // a DECLARED body, which FastAPI reads and spools BEFORE the route's `require_human` dependency + // runs — so with the old shape an unauthenticated caller could make the server write the whole + // per-file allowance to temp (P1). Headers are latin-1 on the wire, hence `encodeURIComponent`; + // the server percent-decodes both. Content-Type is left to the browser (the File's own type). uploadDownload: ( file: File, opts: { displayName?: string; onProgress?: (pct: number) => void; signal?: AbortSignal } = {}, ): Promise => { - const form = new FormData(); - form.append("file", file); - if (opts.displayName) form.append("display_name", opts.displayName); - return uploadReq("/api/downloads/upload", form, { + const headers: Record = { + "X-Upload-Filename": encodeURIComponent(file.name || "upload"), + }; + if (opts.displayName) headers["X-Upload-Display-Name"] = encodeURIComponent(opts.displayName); + return uploadReq("/api/downloads/upload", file, { onProgress: opts.onProgress, signal: opts.signal, + headers, }); }, enqueueEdit: (body: {