diff --git a/backend/app/bodylimit.py b/backend/app/bodylimit.py index 9bf8ce33..d2847387 100644 --- a/backend/app/bodylimit.py +++ b/backend/app/bodylimit.py @@ -74,10 +74,25 @@ DEFAULT_MAX_BODY_BYTES = 1024 * 1024 # default, so listing it could only widen what it accepts. ROUTE_MAX_BODY_BYTES: dict[tuple[str, str], int] = { # 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. + # so this ceiling and the route's own cap count exactly the same bytes. `sysconfig`'s + # `download_upload_max_bytes` is bounded by this same constant (see sysconfig.SPECS), so an + # admin can only ever LOWER the route's cap below the edge, never raise it past one. + # + # WHICH LAYER ANSWERS, measured rather than assumed (deep-review round 1): + # * knob LOWERED (an override below the default) — the route's cap is the smaller number, so + # `capped_stream_to` trips first and answers 422 `upload_too_large` carrying the admin's own + # figure, after draining the rest of the body so the client can read it. + # * knob at the DEFAULT — the two numbers are EQUAL, and the edge always wins: a declared + # Content-Length over the cap is refused here before a byte is read, and an undeclared body + # trips this counter on the same chunk the route would have tripped on. The route's 422 is + # unreachable in that configuration, and no amount of headroom would change it — the + # declared-length check fires first for any body big enough to matter, and the route's drain + # pushes the count past the edge for the rest. + # + # So this 413 is a refusal a real user meets on a default install, not a theoretical edge, and + # it has to be as readable as the route's own: the client localizes `too_large` (`errors.tooLarge`) + # exactly as it does `upload_too_large`, and the upload dialog refuses an over-cap file before + # sending it at all, off the `upload_max_bytes` that `GET /api/downloads/usage` reports. ("POST", "/api/downloads/upload"): settings.download_upload_max_bytes, ("POST", "/api/debug-sessions/{sid}/screenshots"): debugsession.SCREENSHOT_MAX_BYTES, } diff --git a/backend/app/downloads/service.py b/backend/app/downloads/service.py index 896d65ac..c10676a9 100644 --- a/backend/app/downloads/service.py +++ b/backend/app/downloads/service.py @@ -31,6 +31,26 @@ class EditError(Exception): super().__init__(reason) +# The width of every user-supplied display string on a job (`DownloadJob.display_name`, +# `display_uploader`): `String(255)`. `test_upload.py` holds this against the columns +# themselves so the two cannot drift — a number in a comment is exactly how that drift happens. +DISPLAY_TEXT_MAX = 255 + + +def display_text(raw: str | None) -> str | None: + """A user-supplied display string on its way into one of those columns: stripped, truncated, + blank → None. + + One definition because there are three writers and they had drifted: the metadata PATCH + truncated, the upload ingest did not, and since P1 the upload's name arrives in a REQUEST HEADER + where nothing bounds its length — so a long enough one reached Postgres as a `DataError`, i.e. a + 500 after the whole file had already been transferred. Truncating is the right answer rather + than refusing: the name is a label on a file the user has already sent, and losing the tail of it + is not worth failing the upload over. + """ + return (raw or "").strip()[:DISPLAY_TEXT_MAX] or None + + def source_url(source_kind: str, source_ref: str) -> str: """The URL yt-dlp should fetch. YouTube stores the 11-char id (the adapter reconstructs the watch URL); ad-hoc sources store the URL verbatim.""" diff --git a/backend/app/downloads/upload.py b/backend/app/downloads/upload.py index f9219d0c..70acf329 100644 --- a/backend/app/downloads/upload.py +++ b/backend/app/downloads/upload.py @@ -99,7 +99,7 @@ def ingest_upload( source_kind="upload", source_ref=ref, profile_snapshot={}, - display_name=(display_name or "").strip() or None, + display_name=service.display_text(display_name), status="done", progress=100, queue_pos=service.next_queue_pos(db), diff --git a/backend/app/routes/downloads.py b/backend/app/routes/downloads.py index c71380b4..dc50f71e 100644 --- a/backend/app/routes/downloads.py +++ b/backend/app/routes/downloads.py @@ -462,6 +462,18 @@ async def spool_upload( 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") + user_id = user.id + # Hand the pooled connection back BEFORE the transfer starts. Resolving `user` and reading the + # cap were both SELECTs, and SQLAlchemy 2.0 autobegins a transaction on the first one — so + # without this the session sits `idle in transaction` holding one of the twenty pooled + # connections for as long as the client cares to keep sending. That is the invariant `db.py` + # sizes the pool on, and `plex/playback.py` records the production incident that taught it; + # reading the body here instead of letting FastAPI spool it (P1) is what put this route at risk + # of breaking it. Auth only reads, so ending the read transaction loses nothing, and the + # handler's first query autobegins a fresh one — one ROLLBACK round-trip against a minute of + # held connection. Everything this dependency still needs is already an int in hand: touching + # an ORM attribute below would reload `user` and check a connection straight back out. + db.rollback() staging = Path(settings.download_root) / ".staging" / "uploads" staging.mkdir(parents=True, exist_ok=True) fd, tmp = tempfile.mkstemp(dir=str(staging)) @@ -477,9 +489,9 @@ async def spool_upload( yield SpooledUpload( path=tmp_path, size=size, - user_id=user.id, + user_id=user_id, filename=_header_text(request, UPLOAD_FILENAME_HEADER) or "upload", - display_name=_header_text(request, UPLOAD_DISPLAY_NAME_HEADER) or None, + display_name=service.display_text(_header_text(request, UPLOAD_DISPLAY_NAME_HEADER)), ) finally: tmp_path.unlink(missing_ok=True) @@ -570,7 +582,20 @@ def list_downloads( @router.get("/usage") def my_usage(user: User = Depends(require_human), db: Session = Depends(get_db)) -> dict: - return quota.usage(db, user.id) + """The caller's download quota, plus the per-file upload cap. + + The cap rides here rather than in `quota.usage` because it is not a quota value — it is a + `sysconfig` knob about one route — and this is the route layer, where composing the two is the + job. The upload dialog needs it to refuse an over-cap file BEFORE sending it: both server-side + caps answer too late to be pleasant (the edge ceiling refuses the request outright, see + `bodylimit.ROUTE_MAX_BODY_BYTES`), and making the user push 600 MB up the wire to be told the + limit is 500 is a bad answer even when it is a correct one. It is a convenience, not the + boundary: the caps stay where they are and still refuse a client that ignores this. + """ + return { + **quota.usage(db, user.id), + "upload_max_bytes": sysconfig.get_int(db, "download_upload_max_bytes"), + } # Priority when a source has several jobs (e.g. a done one plus a fresh queued one). @@ -665,9 +690,9 @@ def update_download_meta( job = owned_or_404(db, DownloadJob, job_id, user, what="download") sent = body.model_fields_set if "display_name" in sent: - job.display_name = ((body.display_name or "").strip())[:255] or None + job.display_name = service.display_text(body.display_name) if "display_uploader" in sent: - job.display_uploader = ((body.display_uploader or "").strip())[:255] or None + job.display_uploader = service.display_text(body.display_uploader) if "display_uploader_url" in sent: job.display_uploader_url = _clean_url(body.display_uploader_url) if "extra_links" in sent: diff --git a/backend/app/sysconfig.py b/backend/app/sysconfig.py index 7d72a817..d8e3f407 100644 --- a/backend/app/sysconfig.py +++ b/backend/app/sysconfig.py @@ -137,13 +137,18 @@ SPECS: tuple[ConfigSpec, ...] = ( min=1, max=100_000, ), - # min=1, NOT 0: the per-file cap is enforced as `if cap and size > cap`, so a stored 0 would read - # as falsy and SILENTLY disable the limit (an unbounded streamed write to the volume). Floor it at - # 1 so the cap is always active; the overall footprint is separately bounded by the byte quota. - # `max` is not cosmetic: `bodylimit.ROUTE_MAX_BODY_BYTES` refuses an upload body over roughly + # min=1, NOT 0: a stored 0 used to read as "no per-file cap" (the cap was enforced as + # `if cap and size > cap`, so a falsy value disabled it). Since P1 the route passes this number + # straight to `capped_stream_to`, where 0 would refuse every upload instead — the opposite + # failure, and the reason `config.download_upload_max_bytes` carries its own `ge=1`. Either way + # a stored 0 is wrong, so it is floored at 1; the overall footprint is separately bounded by the + # byte quota. + # `max` is not cosmetic: `bodylimit.ROUTE_MAX_BODY_BYTES` refuses an upload body over exactly # this number at the edge, before the route's own check ever sees it. An admin allowed to raise # the knob past the edge would get 413s from a layer the admin page never mentions, so the two - # are pinned together here and `tests/test_body_limit.py` holds them that way. `_coerce` also + # are pinned together here and `tests/test_body_limit.py` holds them that way. Below the edge is + # the useful direction and the one the knob exists for: a lowered knob is what makes the route's + # own 422 — the one carrying this figure — the answer the user gets. `_coerce` also # REJECTS a stored value outside these bounds on read (falling back to the env default, with a # warning) — neither bound has always been what it is now, so a legal-when-written override can # be sitting in the database already. diff --git a/backend/tests/test_body_limit.py b/backend/tests/test_body_limit.py index d19bf2ec..e6d4149a 100644 --- a/backend/tests/test_body_limit.py +++ b/backend/tests/test_body_limit.py @@ -204,23 +204,51 @@ def test_the_default_ceiling_clears_the_largest_body_a_default_route_takes(): ) -# Every `capped_body` call site in the routers, as (module, limit constant). A route that reads its -# own body declares the size it MEANS to accept; if that is above the global default and its route -# is not in ROUTE_MAX_BODY_BYTES, this middleware refuses a request the route would have taken — -# silently, in production, on a path nobody tested. The point of the list is the third entry. +# Every body-reading call site in the routers, as (module, function, limit expression). A route that +# reads its own body declares the size it MEANS to accept; if that is above the global default and +# its route is not in ROUTE_MAX_BODY_BYTES, this middleware refuses a request the route would have +# taken — silently, in production, on a path nobody tested. The point of the list is the third entry. +# +# BOTH helpers, not just `capped_body`: P1 added `capped_stream_to` as its sibling and the registry +# did not follow, so the upload route — the one this whole invariant was written for — was the one +# site the drift guard could not see (deep-review round 1). The names come from `_common.py` itself +# below, so a THIRD sibling cannot be added without this list noticing either. CAPPED_BODY_SITES = { - ("diagnostics.py", "BODY_MAX_BYTES"), - ("debug_sessions.py", "debugsession.SCREENSHOT_MAX_BYTES"), + ("diagnostics.py", "capped_body", "BODY_MAX_BYTES"), + ("debug_sessions.py", "capped_body", "debugsession.SCREENSHOT_MAX_BYTES"), + ("downloads.py", "capped_stream_to", "cap"), } -def test_every_capped_body_site_is_known_and_fits_under_its_ceiling(): +def _body_readers() -> set[str]: + """The names of the body-reading helpers `_common.py` exports — asked of the module rather than + listed here, so adding a fourth one cannot quietly leave the registry below behind.""" + common = pathlib.Path(__file__).resolve().parents[1] / "app" / "routes" / "_common.py" + tree = ast.parse(common.read_text("utf-8"), filename=str(common)) + return { + node.name + for node in tree.body + if isinstance(node, ast.AsyncFunctionDef) and node.name.startswith("capped_") + } + + +def test_the_registry_covers_every_body_reading_helper(): + """The mutation guard for the test below: if a helper were renamed or added, the site list would + still match itself while the new one went uncovered.""" + assert _body_readers() == {"capped_body", "capped_stream_to"}, ( + "routes/_common.py grew or renamed a body-reading helper — add its call sites to " + "CAPPED_BODY_SITES and this expectation." + ) + + +def test_every_body_reading_site_is_known_and_fits_under_its_ceiling(): """Parsed, not grepped. A regex over the call text has to guess how the arguments are spelled, and the shapes it guesses wrong — a bare `4 * 1024 * 1024`, a parameter not named `request` — are the natural ways the NEXT site gets written, so a miss reads as "no new sites" rather than as a failure. `ast` sees every call whatever it looks like.""" + readers = _body_readers() routes_dir = pathlib.Path(__file__).resolve().parents[1] / "app" / "routes" - found: set[tuple[str, str]] = set() + found: set[tuple[str, str, str]] = set() for py in routes_dir.rglob("*.py"): tree = ast.parse(py.read_text("utf-8"), filename=str(py)) for node in ast.walk(tree): @@ -228,22 +256,27 @@ def test_every_capped_body_site_is_known_and_fits_under_its_ceiling(): continue fn = node.func name = fn.attr if isinstance(fn, ast.Attribute) else getattr(fn, "id", None) - if name != "capped_body": + if name not in readers: continue limit = next( (kw.value for kw in node.keywords if kw.arg == "limit"), - node.args[1] if len(node.args) > 1 else None, + node.args[2] if name == "capped_stream_to" and len(node.args) > 2 else None, ) - found.add((py.name, ast.unparse(limit) if limit is not None else "")) + if limit is None and name != "capped_stream_to": + limit = node.args[1] if len(node.args) > 1 else None + found.add((py.name, name, ast.unparse(limit) if limit is not None else "")) assert found == CAPPED_BODY_SITES, ( - f"a capped_body call site appeared, moved or changed shape: {found ^ CAPPED_BODY_SITES}. " + f"a body-reading call site appeared, moved or changed shape: {found ^ CAPPED_BODY_SITES}. " "Each one declares a body size; check it against bodylimit.DEFAULT_MAX_BODY_BYTES and, if " "it is larger, add its route to bodylimit.ROUTE_MAX_BODY_BYTES — then update this list." ) - # And the two known ones really do satisfy the rule. + # And the known ones really do satisfy the rule. assert diagnostics_routes.BODY_MAX_BYTES <= DEFAULT_MAX_BODY_BYTES assert debugsession.SCREENSHOT_MAX_BYTES > DEFAULT_MAX_BODY_BYTES assert ("POST", "/api/debug-sessions/{sid}/screenshots") in bodylimit.ROUTE_MAX_BODY_BYTES + # The upload site's limit is the sysconfig knob, which is bounded BY the edge ceiling rather + # than being a constant to compare against — that pairing is its own test below. + assert ("POST", "/api/downloads/upload") in bodylimit.ROUTE_MAX_BODY_BYTES def test_the_upload_ceiling_and_the_admin_knob_cannot_drift_apart(): @@ -254,6 +287,12 @@ def test_the_upload_ceiling_and_the_admin_knob_cannot_drift_apart(): 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. + + Equality also means the EDGE is what answers on a default install, where no override has lowered + the knob: the declared-length check refuses before a byte is read. That is not a gap to close + with headroom (measured: headroom only moves the boundary by its own size, and the route's drain + pushes the count back over it) — it is a refusal the client has to be able to read, which is why + `errors.tooLarge` exists and why the dialog checks the file against `upload_max_bytes` first. """ 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")] @@ -288,19 +327,28 @@ def test_no_route_with_a_raised_ceiling_declares_a_body_parameter(method, templa 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.""" + Asked of FastAPI itself 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. + + `route.body_field`, NOT `route.dependant.body_params`, and the difference is the whole test. + `body_params` holds only the parameters of the route's OWN callable; `body_field` is built from + `_get_flat_body_params`, which recurses into sub-dependencies — and that is also what FastAPI + itself consults when it decides to read the body. Measured on the pinned FastAPI: a body declared + inside a sub-dependency leaves `body_params` EMPTY while the body is still read in full before + auth, so the first version of this test was green over a reproduction of the very defect it + exists to forbid (deep-review round 1). The upload route's own idiom — a body read from a + `yield` dependency — is exactly the shape that would have slipped 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." + assert route.body_field is None, ( + f"{method} {template} declares a body ({route.body_field.name}) on the route or in one of " + "its dependencies, so FastAPI reads 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." ) diff --git a/backend/tests/test_upload.py b/backend/tests/test_upload.py index 33f6ad83..d3a230b2 100644 --- a/backend/tests/test_upload.py +++ b/backend/tests/test_upload.py @@ -21,10 +21,12 @@ from fastapi import HTTPException from app.config import settings from app.routes._common import capped_stream_to -from app.downloads import mediakind, quota, storage +from sqlalchemy import select + +from app.downloads import mediakind, quota, service, storage from app.downloads import probe as probemod from app.downloads import upload as uploadmod -from app.models import DownloadQuota, MediaAsset +from app.models import DownloadJob, DownloadQuota, MediaAsset # --- pure: the kind invariant --------------------------------------------------------------- @@ -369,6 +371,149 @@ def test_upload_route_enforces_per_file_cap( assert r.json()["detail"]["code"] == "upload_too_large" +def test_the_cap_admits_a_body_of_EXACTLY_the_cap_and_refuses_one_byte_more( + as_user, make_user, db, _root, monkeypatch, upload_headers +): + """The boundary, which the test above cannot see: it sends 16 bytes against a cap of 5, so a + `>` silently loosened to `>=` refuses it just the same and the mutation survives the whole + suite. The cap is `size > limit`, so a file of exactly the cap must be ACCEPTED. + """ + from app import sysconfig + + _probe(monkeypatch, (None, None, None, None, None)) + payload = b"x" * 64 + sysconfig.set_value(db, "download_upload_max_bytes", len(payload)) + db.commit() + c = as_user(make_user()) + + at_the_cap = c.post( + "/api/downloads/upload", content=payload, headers=upload_headers("exact.bin") + ) + assert at_the_cap.status_code == 200, at_the_cap.text + + over = c.post( + "/api/downloads/upload", content=payload + b"x", headers=upload_headers("over.bin") + ) + assert over.status_code == 422 + assert over.json()["detail"] == {"code": "upload_too_large", "limit": len(payload)} + + +def test_a_non_ascii_filename_survives_the_header_round_trip( + as_user, make_user, db, _root, monkeypatch, upload_headers +): + """The percent-decoding, which every other test in this file leaves vacuous. + + Headers are latin-1 on the wire, so the client `encodeURIComponent`s the name and the route + `unquote`s it. Every filename the suite sends is pure ASCII — and `quote()` is the IDENTITY + function on ASCII — so deleting the `unquote` call changes nothing anywhere else. A Hungarian + name is the only input that can tell the two apart: without the decode the sanitizer would be + handed the literal `nyaral%C3%A1s.bin` and would keep the percent-escapes in the stored name. + """ + _probe(monkeypatch, (None, None, None, None, None)) + c = as_user(make_user()) + r = c.post( + "/api/downloads/upload", content=b"data", headers=upload_headers("nyaralás ősszel.bin") + ) + assert r.status_code == 200, r.text + asset = db.execute(select(MediaAsset).order_by(MediaAsset.id.desc())).scalars().first() + assert asset is not None + stored = Path(asset.rel_path).name + assert "%" not in stored and "c3" not in stored.lower(), stored + assert stored.startswith("nyaralas_osszel"), stored + + +def test_the_display_text_bound_matches_the_columns_it_guards(): + """A number living in two places is how the `cancelled`/`canceled` bug happened. `display_text` + truncates to `DISPLAY_TEXT_MAX`; the columns it feeds are `String(255)`. Held against the model + itself, so widening one without the other fails here rather than as a DataError in production.""" + for name in ("display_name", "display_uploader"): + assert DownloadJob.__table__.c[name].type.length == service.DISPLAY_TEXT_MAX + + +def test_the_display_name_header_reaches_the_job_and_is_bounded( + as_user, make_user, _root, monkeypatch, upload_headers +): + """The second header, which had no backend test at all — and the length nothing bounded. + + Since P1 the display name arrives in a REQUEST HEADER rather than as a form field, so the only + limit on it was the header size. The column is varchar(255), so a longer one reached Postgres as + a DataError: a 500 AFTER the whole file had been transferred. It is truncated now, by the same + helper the metadata PATCH uses. + """ + _probe(monkeypatch, (None, None, None, None, None)) + c = as_user(make_user()) + + ok = c.post( + "/api/downloads/upload", + content=b"data", + headers=upload_headers("clip.bin", "Nyári vakáció"), + ) + assert ok.status_code == 200, ok.text + assert ok.json()["display_name"] == "Nyári vakáció" + + long_name = "n" * 300 + bounded = c.post( + "/api/downloads/upload", content=b"data2", headers=upload_headers("clip2.bin", long_name) + ) + assert bounded.status_code == 200, bounded.text + assert bounded.json()["display_name"] == "n" * service.DISPLAY_TEXT_MAX + + +def test_the_upload_holds_no_db_transaction_while_the_body_streams( + as_user, make_user, db, _root, monkeypatch, upload_headers +): + """P1 moved the body read behind the auth dependency, which put the whole transfer INSIDE the + request's database session: resolving the caller and reading the cap are SELECTs, SQLAlchemy 2.0 + autobegins on the first one, and the session then holds one of the twenty pooled connections — + `idle in transaction` to Postgres, with no `idle_in_transaction_session_timeout` to end it — for + as long as the client keeps sending. `db.py` sizes the pool on that not happening and + `plex/playback.py` records the production incident behind the rule. + + Asserted on the session rather than on `engine.pool.checkedout()`: the transaction is the cause, + the checkout is its consequence (measured), and the pool counter is shared with whatever else + the test process is holding. + """ + from app.routes import downloads as downloads_routes + + _probe(monkeypatch, (None, None, None, None, None)) + seen: dict[str, bool] = {} + real = downloads_routes.capped_stream_to + + async def spy(request, out, limit, refusal): + seen["in_transaction"] = bool(db.in_transaction()) + return await real(request, out, limit, refusal) + + monkeypatch.setattr(downloads_routes, "capped_stream_to", spy) + c = as_user(make_user()) + r = c.post("/api/downloads/upload", content=b"data", headers=upload_headers("held.bin")) + assert r.status_code == 200, r.text + assert seen["in_transaction"] is False, ( + "the request's session was inside a transaction when the transfer began, so a pooled " + "connection is pinned for the whole upload" + ) + + +def test_a_refused_upload_leaves_nothing_in_the_staging_directory( + as_user, make_user, db, _root, monkeypatch, upload_headers +): + """`spool_upload` writes the body to a staging temp file as it arrives, so a refusal happens + with a partial file already on disk — its `finally` is what removes it. Nothing asserted that: + deleting the unlink left the whole suite green while every refused upload leaked its partial + bytes onto the volume, one file per attempt, with no job to ever clean them up. + """ + from app import sysconfig + + sysconfig.set_value(db, "download_upload_max_bytes", 8) + db.commit() + staging = Path(settings.download_root) / ".staging" / "uploads" + c = as_user(make_user()) + + r = c.post("/api/downloads/upload", content=b"x" * 4096, headers=upload_headers("leaky.bin")) + assert r.status_code == 422 + assert staging.exists(), "the dependency did not get as far as creating the staging dir" + assert list(staging.iterdir()) == [], "a refused upload left its partial file behind" + + def test_upload_cap_cannot_be_disabled_with_zero(db): """download_upload_max_bytes floors at 1, and set_value must enforce it. diff --git a/frontend/src/components/UploadDialog.dom.test.tsx b/frontend/src/components/UploadDialog.dom.test.tsx index ccbecf77..28e7a671 100644 --- a/frontend/src/components/UploadDialog.dom.test.tsx +++ b/frontend/src/components/UploadDialog.dom.test.tsx @@ -11,6 +11,9 @@ import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; const h = vi.hoisted(() => ({ uploadDownload: vi.fn(async (_f: File, _o: unknown) => ({ id: 42 })), + // The dialog reads the per-file upload cap so it can refuse an over-cap file before spending the + // user's bandwidth on it. Generous by default; the cap test below lowers it. + downloadUsage: vi.fn(async () => ({ upload_max_bytes: 1_000_000 })), notify: vi.fn(), })); @@ -18,7 +21,9 @@ vi.mock("react-i18next", async (importOriginal) => { const actual = await importOriginal(); return { ...actual, useTranslation: () => ({ t: (key: string) => key }) }; }); -vi.mock("../lib/api", () => ({ api: { uploadDownload: h.uploadDownload } })); +vi.mock("../lib/api", () => ({ + api: { uploadDownload: h.uploadDownload, downloadUsage: h.downloadUsage }, +})); vi.mock("../lib/downloads", () => ({ invalidateDownloads: vi.fn() })); // The rest of the module stays real: the dialog is a ui/Modal, which hosts its own toasts. vi.mock("../lib/notifications", async (importOriginal) => ({ @@ -44,6 +49,8 @@ const fileInput = () => document.querySelector('input[type="file"]') as HTMLInpu beforeEach(() => { h.uploadDownload.mockClear(); h.notify.mockClear(); + h.downloadUsage.mockClear(); + h.downloadUsage.mockResolvedValue({ upload_max_bytes: 1_000_000 }); }); afterEach(cleanup); @@ -76,4 +83,31 @@ describe("UploadDialog", () => { const btn = screen.getByText("downloads.upload.submit").closest("button")!; expect(btn.disabled).toBe(true); }); + + it("refuses an over-cap file here, naming the limit, instead of sending it", async () => { + // Both server-side caps answer only after the transfer — and on a default install the one that + // answers is the edge ceiling, whose refusal carries no number at all. So the dialog checks the + // size itself: the user learns the limit before a byte moves. + h.downloadUsage.mockResolvedValue({ upload_max_bytes: 4 }); + wrap(); + await waitFor(() => expect(h.downloadUsage).toHaveBeenCalled()); + fireEvent.change(fileInput(), { + target: { files: [new File(["far too many bytes"], "big.bin")] }, + }); + + await waitFor(() => expect(screen.getByText("downloads.errors.upload.tooLarge")).toBeTruthy()); + const btn = screen.getByText("downloads.upload.submit").closest("button")!; + expect(btn.disabled).toBe(true); + fireEvent.click(btn); + expect(h.uploadDownload).not.toHaveBeenCalled(); + }); + + it("sends a file that fits, so the check above is not simply refusing everything", async () => { + h.downloadUsage.mockResolvedValue({ upload_max_bytes: 1_000_000 }); + wrap(); + await waitFor(() => expect(h.downloadUsage).toHaveBeenCalled()); + fireEvent.change(fileInput(), { target: { files: [new File(["small"], "ok.bin")] } }); + fireEvent.click(screen.getByText("downloads.upload.submit")); + await waitFor(() => expect(h.uploadDownload).toHaveBeenCalledTimes(1)); + }); }); diff --git a/frontend/src/components/UploadDialog.tsx b/frontend/src/components/UploadDialog.tsx index c6670db4..99f80fae 100644 --- a/frontend/src/components/UploadDialog.tsx +++ b/frontend/src/components/UploadDialog.tsx @@ -1,10 +1,11 @@ import { useEffect, useRef, useState } from "react"; import { useTranslation } from "react-i18next"; -import { useQueryClient } from "@tanstack/react-query"; +import { useQuery, useQueryClient } from "@tanstack/react-query"; import { UploadCloud, File as FileIcon } from "lucide-react"; import Modal from "./ui/Modal"; import { Button } from "./ui/Button"; import { api } from "../lib/api"; +import { qk } from "../lib/queryKeys"; import { isAborted } from "../lib/api/core"; import { notifyActionError } from "../lib/actionErrors"; import { invalidateDownloads } from "../lib/downloads"; @@ -31,6 +32,14 @@ export default function UploadDialog({ onClose }: { onClose: () => void }) { // plain boolean flickers. Count enter vs leave and only clear the highlight at depth 0. const dragDepth = useRef(0); const busy = pct !== null; + // The per-file cap, so an over-cap file is refused HERE instead of after the user has pushed it up + // the wire. Both server-side caps are still the boundary — this is only the pleasant half of it. + // On a default install the one that answers is the edge ceiling (`bodylimit`), whose refusal + // carries no number, so without this the user would be told "too large" and not what the limit is. + // A cap that has not loaded yet blocks nothing — the server still refuses, honestly. + const cap = useQuery({ queryKey: qk.downloadUsage(), queryFn: api.downloadUsage }).data + ?.upload_max_bytes; + const tooBig = !!file && !!cap && file.size > cap; // Closing the dialog mid-upload (X / ESC / backdrop / browser Back all unmount us) must abort the // in-flight XHR — otherwise it keeps streaming in the background and still toasts success. Only the @@ -40,7 +49,7 @@ export default function UploadDialog({ onClose }: { onClose: () => void }) { const pick = () => inputRef.current?.click(); const submit = async () => { - if (!file || busy) return; + if (!file || busy || tooBig) return; const ac = new AbortController(); abortRef.current = ac; setPct(0); @@ -133,6 +142,12 @@ export default function UploadDialog({ onClose }: { onClose: () => void }) { }} /> + {tooBig && ( +

+ {t("downloads.errors.upload.tooLarge", { size: formatBytes(cap ?? 0) })} +

+ )} + @@ -141,6 +156,9 @@ export default function UploadDialog({ onClose }: { onClose: () => void }) { onChange={(e) => setName(e.target.value)} placeholder={t("downloads.upload.namePlaceholder")} disabled={busy} + // The column behind it is varchar(255) and the name rides in a REQUEST HEADER, where nothing + // else bounds it; the server truncates to the same number (downloads.service.display_text). + maxLength={255} className={`${inputCls} mb-5`} /> @@ -175,7 +193,7 @@ export default function UploadDialog({ onClose }: { onClose: () => void }) { > {t("downloads.actions.cancel")} - diff --git a/frontend/src/i18n/locales/en/errors.json b/frontend/src/i18n/locales/en/errors.json index c7ccd936..afdf28b7 100644 --- a/frontend/src/i18n/locales/en/errors.json +++ b/frontend/src/i18n/locales/en/errors.json @@ -3,6 +3,7 @@ "generic": "The server couldn't carry out that action. Please try again.", "demo": "This isn't available in the demo account.", "validation": "Some of the values you entered weren't valid. Please check them and try again.", + "tooLarge": "That was too large for the server to accept.", "server": "Server error", "syncCooldown": "You've synced a few times just now — give it a few minutes. The YouTube budget is shared with everyone on this instance, and background syncing keeps running either way.", "notFound": { diff --git a/frontend/src/i18n/locales/hu/errors.json b/frontend/src/i18n/locales/hu/errors.json index d571b979..a4a86166 100644 --- a/frontend/src/i18n/locales/hu/errors.json +++ b/frontend/src/i18n/locales/hu/errors.json @@ -3,6 +3,7 @@ "generic": "A szerver nem tudta végrehajtani a műveletet. Próbáld újra.", "demo": "Ez a bemutató fiókban nem érhető el.", "validation": "Néhány megadott érték érvénytelen volt. Ellenőrizd, majd próbáld újra.", + "tooLarge": "Ez túl nagy, a szerver nem fogadta el.", "server": "Szerverhiba", "syncCooldown": "Az imént többször is szinkronizáltál — várj pár percet. A YouTube-keret közös az instance összes felhasználójával, a háttérszinkron pedig így is fut tovább.", "notFound": { diff --git a/frontend/src/lib/api/core.test.ts b/frontend/src/lib/api/core.test.ts index ff9bf1de..af43a201 100644 --- a/frontend/src/lib/api/core.test.ts +++ b/frontend/src/lib/api/core.test.ts @@ -74,11 +74,14 @@ beforeEach(() => { vi.stubGlobal("XMLHttpRequest", FakeXHR as unknown as typeof XMLHttpRequest); }); -const form = () => new FormData(); +// The upload body is a Blob and only a Blob: `FormData` was accepted until P1 and is gone, because +// a multipart part is a DECLARED body that FastAPI spools before the route's auth dependency runs — +// and nothing on the backend can parse one any more (`python-multipart` was dropped with it). +const body = () => new Blob(["x"]); describe("uploadReq", () => { it("resolves with the parsed JSON on a 2xx", async () => { - const p = uploadReq<{ id: number }>("/u", form()); + const p = uploadReq<{ id: number }>("/u", body()); const x = FakeXHR.last; x.status = 200; x.responseText = '{"id":7}'; @@ -93,14 +96,14 @@ describe("uploadReq", () => { it("rejects immediately if the signal is already aborted (never sends)", async () => { const ac = new AbortController(); ac.abort(); - const p = uploadReq("/u", form(), { signal: ac.signal }); + const p = uploadReq("/u", body(), { signal: ac.signal }); await expect(p).rejects.toSatisfy(isAborted); expect(FakeXHR.last?.sent).not.toBe(true); }); it("aborts the XHR and rejects when the signal fires", async () => { const ac = new AbortController(); - const p = uploadReq("/u", form(), { signal: ac.signal }); + const p = uploadReq("/u", body(), { signal: ac.signal }); ac.abort(); // → our abort listener calls xhr.abort() → onabort → reject await expect(p).rejects.toSatisfy(isAborted); }); @@ -108,7 +111,7 @@ describe("uploadReq", () => { it("calls the unauthorized handler on 401", async () => { const onUnauth = vi.fn(); setUnauthorizedHandler(onUnauth); - const p = uploadReq("/u", form()); + const p = uploadReq("/u", body()); const x = FakeXHR.last; x.status = 401; x.onload!(); @@ -118,7 +121,7 @@ describe("uploadReq", () => { }); it("localizes a structured 422 upload_too_large and surfaces the error", async () => { - const p = uploadReq("/u", form()); + const p = uploadReq("/u", body()); const x = FakeXHR.last; x.status = 422; x.responseText = '{"detail":{"code":"upload_too_large","limit":5}}'; @@ -128,8 +131,39 @@ describe("uploadReq", () => { expect(reportError).toHaveBeenCalledWith("downloads.errors.upload.tooLarge"); }); + it("localizes the edge ceiling's 413 too_large, which carries no limit", async () => { + // On a DEFAULT install this — not the route's numbered 422 above — is what an over-cap upload + // gets: the ceiling and the route's cap are the same figure and the edge answers first. It fell + // through to the generic "couldn't carry out that action" until the branch existed. + const p = uploadReq("/u", body()); + const x = FakeXHR.last; + x.status = 413; + x.responseText = '{"detail":{"code":"too_large"}}'; + x.onload!(); + const err = (await p.catch((e: unknown) => e)) as HttpError; + expect(err).toBeInstanceOf(HttpError); + expect(err.detail).toBe("errors.tooLarge"); + }); + + it("lets its own identity headers win over a caller's", () => { + // Who a request is from is this layer's to state, not its caller's. Merged the other way round, + // a caller's header of the same name would silently re-attribute the upload — to another + // account, or (here) into someone else's debug session recording. + setDebugSession("0123456789abcdef"); + try { + void uploadReq("/u", body(), { + headers: { "X-Debug-Session": "deadbeefdeadbeef", "X-Upload-Filename": "a.bin" }, + }); + expect(FakeXHR.last.headers["X-Debug-Session"]).toBe("0123456789abcdef"); + // ...while a header that is genuinely the caller's still rides. + expect(FakeXHR.last.headers["X-Upload-Filename"]).toBe("a.bin"); + } finally { + setDebugSession(null); + } + }); + it("offers no string detail as the reason on the upload path either", async () => { - const p = uploadReq("/u", form()); + const p = uploadReq("/u", body()); const x = FakeXHR.last; x.status = 405; x.responseText = '{"detail":"Method Not Allowed"}'; @@ -140,7 +174,7 @@ describe("uploadReq", () => { }); it("reports a connectivity failure on a network error", async () => { - const p = uploadReq("/u", form()); + const p = uploadReq("/u", body()); FakeXHR.last.onerror!(); await expect(p).rejects.toMatchObject({ status: 0 }); // The soft self-clearing "connection lost" toast (via notify), not a blocking modal. @@ -329,14 +363,14 @@ describe("req — what it has already put in front of the user", () => { }); it("marks what the upload path reported the same way", async () => { - const dead = uploadReq("/u", form()); + const dead = uploadReq("/u", body()); FakeXHR.last.onerror!(); expect(alreadyReported(await dead.catch((e: unknown) => e))).toBe(false); - const refused = uploadReq("/u", form()); + const refused = uploadReq("/u", body()); FakeXHR.last.status = 500; FakeXHR.last.onload!(); expect(alreadyReported(await refused.catch((e: unknown) => e))).toBe(true); - const gone = uploadReq("/u", form()); + const gone = uploadReq("/u", body()); FakeXHR.last.status = 404; FakeXHR.last.onload!(); expect(alreadyReported(await gone.catch((e: unknown) => e))).toBe(false); @@ -350,7 +384,7 @@ describe("req — what it has already put in front of the user", () => { await req("/api/x", { method: "POST" }).catch(() => {}); // a notice is up getActiveToasts.mockReturnValue([{ id: 9 }]); remove.mockClear(); - const p = uploadReq("/u", form()); + const p = uploadReq("/u", body()); FakeXHR.last.status = 502; FakeXHR.last.onload!(); await expect(p).rejects.toMatchObject({ status: 502 }); @@ -378,7 +412,7 @@ describe("the debug session a tab records into", () => { expect(sentHeaders().get("X-Debug-Session")).toBe("0123456789abcdef"); beacon("/api/y", {}); expect(sentHeaders().get("X-Debug-Session")).toBe("0123456789abcdef"); - void uploadReq("/u", form()); + void uploadReq("/u", body()); expect(FakeXHR.last.headers["X-Debug-Session"]).toBe("0123456789abcdef"); } finally { setDebugSession(null); diff --git a/frontend/src/lib/api/core.ts b/frontend/src/lib/api/core.ts index f567802a..5780261a 100644 --- a/frontend/src/lib/api/core.ts +++ b/frontend/src/lib/api/core.ts @@ -119,6 +119,13 @@ function localizeDetail(d: unknown): string { if (o?.code === "upload_too_large") return t("downloads.errors.upload.tooLarge", { size: formatBytes(o.limit) }); if (o?.code === "upload_empty") return t("downloads.errors.upload.empty"); + // The EDGE ceiling's refusal (`bodylimit.TOO_LARGE_BODY`), shared by every route and carrying no + // number. Not a theoretical case: on a DEFAULT install it is what an over-cap upload actually + // gets, because the ceiling and the route's own cap are the same figure and the edge answers + // first — the route's numbered `upload_too_large` above is the answer only once an admin has + // LOWERED the knob (see bodylimit.ROUTE_MAX_BODY_BYTES). Without this branch that refusal fell + // through to the generic "couldn't carry out that action", the one message no retry can fix. + if (o?.code === "too_large") return t("errors.tooLarge"); // SEC-12: a download URL the server refuses to fetch. The reasons are deliberately distinct — // "we could not look that host up" is a different thing to tell someone than "we will not fetch // from that address" — so an unknown reason falls back to the generic refusal rather than a key. @@ -378,16 +385,17 @@ export async function req( // 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. +// `body` is a `Blob`/`File` — the bytes themselves, and nothing else. `FormData` was the other +// accepted shape until P1 and is deliberately gone: a multipart part is a DECLARED body, which +// FastAPI spools to disk before the route's auth dependency runs, and the backend dropped +// `python-multipart` altogether, so there is no longer anything on the other end that could parse +// one. `cfg.headers` is how a caller sends the metadata that used to ride as 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. +// Content-Type is never set here: the browser derives it from the Blob's own type, and a wrong one +// is worse than none. export function uploadReq( url: string, - body: Blob | FormData, + body: Blob, cfg: { onProgress?: (pct: number) => void; signal?: AbortSignal; @@ -401,7 +409,10 @@ export function uploadReq( const requestId = newRequestId(); const started = performance.now(); xhr.setRequestHeader(REQUEST_ID_HEADER, requestId); - for (const [name, value] of Object.entries({ ...identityHeaders(), ...cfg.headers })) + // `identityHeaders()` LAST: who the request is from is this layer's to state, not the caller's. + // Merged the other way round, a caller's header of the same name would silently re-attribute + // the upload to another account — the one thing in here that must not be overridable. + for (const [name, value] of Object.entries({ ...cfg.headers, ...identityHeaders() })) xhr.setRequestHeader(name, value); if (cfg.onProgress) { xhr.upload.onprogress = (e) => { diff --git a/frontend/src/lib/api/downloads.ts b/frontend/src/lib/api/downloads.ts index f0f8584e..790d3f02 100644 --- a/frontend/src/lib/api/downloads.ts +++ b/frontend/src/lib/api/downloads.ts @@ -124,6 +124,10 @@ export interface DownloadUsage { max_concurrent: number; max_jobs: number; unlimited: boolean; + // The per-file cap on an upload, so the dialog can refuse an over-cap file before spending the + // user's bandwidth on it. Not the same thing as `max_bytes`, which is the whole library + // footprint: this one is a single request's ceiling and is enforced twice server-side anyway. + upload_max_bytes: number; } export interface AdminStorage { @@ -184,9 +188,9 @@ export const downloadsApi = { spec?: DownloadSpec; display_name?: string; }): Promise => req("/api/downloads", { method: "POST", body: JSON.stringify(body) }), - // 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. + // Ingest a local file into the library (E7 S5). Sent via XHR rather than `req` 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