diff --git a/CHANGELOG.md b/CHANGELOG.md index 005bd4d7..7188ac56 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,23 @@ edit the release notes and re-run the generator. Upgrading, pinning a version and rolling back: [docs/self-hosting.md](docs/self-hosting.md#upgrading). +## 0.95.0 — 2026-09-23 + +A file that is too big to upload says so before it is sent, instead of failing as if the server were down - and the server no longer writes half a gigabyte to disk for someone who never signed in. + +### Fixes + +- Picking a file over the size limit used to end in "the server couldn't be reached" - after the whole file had been sent. The dialog now checks the size the moment you choose the file, tells you what the limit is, and does not start. If a refusal does come from the server, it reads as a sentence about the size rather than as a network failure. +- A long name on a download no longer breaks it. Anything typed past the length of the field used to fail the whole operation with a server error - on an upload, after the entire file had already transferred. Names are simply shortened now, on every screen where one can be set. +- While the file you picked is too large, the drop zone still lights up when you drag a replacement onto it. It used to stay red, which read as though drag-and-drop had stopped working - in exactly the moment you were trying to fix the problem. + +### Internal + +- An upload is now read only after the server knows who is sending it. Until now anyone at all could make the server write up to 500 MB to disk and only then be told they were not allowed to upload, because the file arrived before the password was ever checked. It is read a piece at a time, straight to disk, and stops the moment it passes the limit. +- An upload no longer holds a database connection open while the file is transferring. One slow upload used to occupy a connection for its entire duration, and enough of them at once would have starved everything else on the server. +- The parser for the old upload format is gone from the app entirely, along with the size allowance it required. +- Three adversarial review rounds closed the work: 13 findings, then 28, then 8. The last round found almost nothing in the app itself - its findings were in the tests the round before had written, which is why it was the last. Four of its claims were disproved rather than fixed, one of them a proposed change that would have broken emoji in names. Every regression test was proven to fail with its own fix taken out. + ## 0.94.0 — 2026-09-22 No new features, on purpose: a release spent entirely on the machinery underneath, so that whole classes of fault stop being possible rather than being fixed one at a time. diff --git a/VERSION b/VERSION index c5c73510..5f8cbfdb 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.94.0 +0.95.0 diff --git a/backend/app/bodylimit.py b/backend/app/bodylimit.py index 0008ee62..d2847387 100644 --- a/backend/app/bodylimit.py +++ b/backend/app/bodylimit.py @@ -53,31 +53,47 @@ 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 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/db.py b/backend/app/db.py index 51a06301..aba4339d 100644 --- a/backend/app/db.py +++ b/backend/app/db.py @@ -1,6 +1,6 @@ from collections.abc import Iterator -from sqlalchemy import create_engine +from sqlalchemy import create_engine, text from sqlalchemy.orm import DeclarativeBase, Session, sessionmaker from app.config import settings @@ -34,3 +34,50 @@ def get_db() -> Iterator[Session]: yield db finally: db.close() + + +def release_read_connection(db: Session) -> None: + """End a request's READ transaction so its pooled connection goes back BEFORE a long await. + + SQLAlchemy 2.0 autobegins on the first SELECT, so a request that has merely authenticated is + already holding one of the connections above — `idle in transaction` to Postgres, and nothing + times it out. A route that then awaits something slow (a request body still arriving, an + external fetch) pins it for the whole wait, which is precisely the invariant the pool is sized + on in the comment above. Measured: the next checkout after this call reuses the very backend + that was released, and a 1.8 s upload goes from one sustained `idle in transaction` to none. + + Every route that reads its own body from a dependency needs this, and `test_body_limit.py` + holds that — the three that do are the ones with a raised (or self-imposed) body ceiling, and + they got here one at a time, which is how the upload route ended up the only one fixed. + + The caller must have NOTHING PENDING, and that is checked rather than described. The session is + shared with every dependency that ran before, so a rollback here would silently discard a write + one of them staged — `service.enqueue` already learned that lesson the expensive way and uses a + SAVEPOINT for it. CLAUDE.md: a rule stated in a comment must also be enforced in code. + + TWO checks, because the obvious one is blind to the shapes this codebase actually writes in. + `db.new`/`deleted`/`dirty` hold only UNFLUSHED unit-of-work: the moment a helper calls + `db.flush()` — CLAUDE.md's canonical split is "the handler commits; helpers flush" — the object + is persistent with no unflushed history and all three sets are empty again, and Core DML + (`db.execute(update(...))`) never enters them at all. Measured: after `add`+`flush` the sets are + empty, the old check passed, and the rollback lost the row. So the transaction is asked + directly. Postgres assigns a transaction id on the first WRITE and never for a read, so + `pg_current_xact_id_if_assigned()` is NULL for exactly the state this function is safe in — and + it sees a raw `text("UPDATE …")` too, which no session-level bookkeeping would. + """ + if db.new or db.deleted or db.dirty: + raise RuntimeError( + "release_read_connection() with pending work: something ahead of this dependency " + "staged a write, which ending the transaction would discard. Commit it first, or do " + "it after the await." + ) + if ( + db.in_transaction() + and db.execute(text("SELECT pg_current_xact_id_if_assigned()")).scalar() is not None + ): + raise RuntimeError( + "release_read_connection() on a transaction that has already WRITTEN: something ahead " + "of this dependency flushed a row or ran an UPDATE/INSERT/DELETE, which ending the " + "transaction would discard. Commit it first, or do it after the await." + ) + db.rollback() diff --git a/backend/app/downloads/pipeline.py b/backend/app/downloads/pipeline.py index d61e99b6..22c7c1ac 100644 --- a/backend/app/downloads/pipeline.py +++ b/backend/app/downloads/pipeline.py @@ -25,7 +25,7 @@ from app.downloads.jobdb import ( ) from app.downloads.stated import StatedFailure from app.downloads.status import JobStatus -from app.models import DownloadJob, MediaAsset +from app.models import DISPLAY_TEXT_MAX, DownloadJob, MediaAsset from app.notifications import create_notification from app.titles import normalize_title @@ -597,7 +597,7 @@ def _download( } if job and not job.display_name and meta.title: # display_name is VARCHAR(255) — clip a long title (see _fill_asset_meta_early). - fields["display_name"] = meta.title[:255] + fields["display_name"] = meta.title[:DISPLAY_TEXT_MAX] # Atomic terminal write: finalizes ONLY while the row is still running, so a child that # finishes just after a route cancel/pause can't resurrect it (see finalize_if_running). finalized = service.finalize_if_running(db, job_id, **fields) diff --git a/backend/app/downloads/service.py b/backend/app/downloads/service.py index 896d65ac..f643d04d 100644 --- a/backend/app/downloads/service.py +++ b/backend/app/downloads/service.py @@ -20,7 +20,7 @@ from app.downloads import credentials from app.downloads import edit as editmod from app.downloads import formats, mediakind, quota, storage from app.downloads.status import HOLDING, JobStatus -from app.models import Channel, DownloadJob, MediaAsset, Video +from app.models import DISPLAY_TEXT_MAX, Channel, DownloadJob, MediaAsset, Video class EditError(Exception): @@ -179,7 +179,7 @@ def enqueue( profile_id=profile_id, profile_snapshot=spec, display_name=display_name - or (asset.title[:255] if asset.status == "ready" and asset.title else None), + or (asset.title[:DISPLAY_TEXT_MAX] if asset.status == "ready" and asset.title else None), status="queued", queue_pos=next_queue_pos(db), ) @@ -198,10 +198,13 @@ def enqueue( def _clip_title(source_title: str | None, display_name: str | None) -> str: + # Clipped to DISPLAY_TEXT_MAX, not to a literal: this feeds `DownloadJob.display_name` through + # `enqueue_edit`, the same column `display_text` guards. A machine-made title, so it is clipped + # rather than run through `display_text` — blank here means "no title", not None. if display_name: - return display_name[:255] + return display_name[:DISPLAY_TEXT_MAX] base = source_title or "Clip" - return f"{base} (clip)"[:255] + return f"{base} (clip)"[:DISPLAY_TEXT_MAX] def enqueue_edit( @@ -255,7 +258,7 @@ def enqueue_edit( source_asset_id=src_asset.id, edit_spec=spec, profile_snapshot={}, - display_name=display_name or asset.title, + display_name=display_name or asset.title[:DISPLAY_TEXT_MAX], status="queued", queue_pos=next_queue_pos(db), ) diff --git a/backend/app/downloads/upload.py b/backend/app/downloads/upload.py index f9219d0c..1c91421e 100644 --- a/backend/app/downloads/upload.py +++ b/backend/app/downloads/upload.py @@ -25,7 +25,7 @@ from sqlalchemy.orm import Session from app.config import settings from app.downloads import edit as editmod from app.downloads import mediakind, probe, quota, service, storage -from app.models import DownloadJob, MediaAsset +from app.models import DownloadJob, MediaAsset, display_text def _title_from_filename(filename: str) -> str: @@ -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=display_text(display_name), status="done", progress=100, queue_pos=service.next_queue_pos(db), diff --git a/backend/app/logs.py b/backend/app/logs.py index ce42b3dd..64746cf5 100644 --- a/backend/app/logs.py +++ b/backend/app/logs.py @@ -466,7 +466,6 @@ _THIRD_PARTY_LEVELS = { "httpx": "WARNING", "httpcore": "WARNING", "asyncio": "WARNING", - "multipart": "WARNING", "PIL": "WARNING", "alembic": "INFO", # uvicorn's own startup/shutdown/WebSocket lines are worth keeping; its ACCESS log is not — diff --git a/backend/app/models.py b/backend/app/models.py index 3700f58a..5677c594 100644 --- a/backend/app/models.py +++ b/backend/app/models.py @@ -23,6 +23,34 @@ from sqlalchemy.dialects.postgresql import JSONB, TSVECTOR from sqlalchemy.orm import Mapped, mapped_column, relationship, validates from app.db import Base + +# Every user-supplied display string in this schema shares one width, and it lives HERE so the +# columns below are declared with it rather than with a literal: a name a person typed goes into +# `User.display_name` (their Google profile name), `DownloadJob.display_name` and +# `DownloadJob.display_uploader`. A value wider than the column is a psycopg DataError, which +# `main.py` re-raises for everything but the NUL case — i.e. a bare 500 for a name somebody pasted. +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 SIX writers and they had drifted: `POST /api/downloads`, + `POST /api/downloads/edit`, `POST /api/downloads/upload`, the metadata PATCH's two fields, and + the Google sign-in callback. Only the PATCH truncated. Truncating is the right answer rather + than refusing: the name is a label on work the user has already done — a file already + transferred, an account already authenticated — and losing its tail is not worth failing the + request over. + + `test_upload.py` holds both halves: that every route assigning one of these fields goes through + this helper (parsed, not grepped), and that each of those routes truncates rather than 500s. A + sweep is exactly the kind of thing that gets done five-sixths of the way, which is how this + ended up being written twice. + """ + return (raw or "").strip()[:DISPLAY_TEXT_MAX] or None + + from app.utils import canonical_email @@ -114,7 +142,7 @@ class User(Base, TimestampMixin, CanonicalEmailMixin): # not cleared eagerly — it simply stops counting). diagnostics_level: Mapped[str | None] = mapped_column(String(8)) diagnostics_until: Mapped[datetime | None] = mapped_column(DateTime(timezone=True)) - display_name: Mapped[str | None] = mapped_column(String(255)) + display_name: Mapped[str | None] = mapped_column(String(DISPLAY_TEXT_MAX)) avatar_url: Mapped[str | None] = mapped_column(String(1024)) role: Mapped[str] = mapped_column(String(16), default="user", server_default="user") # The user's own YouTube channel id, resolved lazily (channels.list?mine=true) and @@ -953,11 +981,11 @@ class DownloadJob(Base, TimestampMixin, UpdatedAtMixin): ForeignKey("download_profiles.id", ondelete="SET NULL") ) profile_snapshot: Mapped[dict] = mapped_column(JSON) - display_name: Mapped[str | None] = mapped_column(String(255)) + display_name: Mapped[str | None] = mapped_column(String(DISPLAY_TEXT_MAX)) # Per-user display overrides + extras (like display_name — metadata only, never the file). # `display_uploader`/`display_uploader_url` override the asset's auto channel name/link; # `extra_links` is an optional list of extra reference URLs shown on the card + watch page. - display_uploader: Mapped[str | None] = mapped_column(String(255)) + display_uploader: Mapped[str | None] = mapped_column(String(DISPLAY_TEXT_MAX)) display_uploader_url: Mapped[str | None] = mapped_column(Text) extra_links: Mapped[list | None] = mapped_column(JSON) status: Mapped[str] = mapped_column( diff --git a/backend/app/routes/_common.py b/backend/app/routes/_common.py index b7e99a55..3526248f 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,47 @@ 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. + + **The refusal DRAINS the rest of the body before it is raised, and that is not politeness.** + Answering mid-upload and closing the connection is what the client sees as a network failure, + not as a refusal: the browser's XHR fires `onerror` before it can read the response, so a file + over the limit reported itself as "the server couldn't be reached" instead of naming the limit — + measured in a real browser, not reasoned about. Reading the remainder to nowhere lets the client + finish sending, read the 422 and say the true thing. + + The bytes are discarded, never written, and the whole request is already bounded by + `bodylimit`'s per-route ceiling — so the worst case is bandwidth an authenticated caller was + sending anyway, against the old behaviour which wrote exactly those bytes to disk.""" + size = 0 + over = False + async for chunk in request.stream(): + size += len(chunk) + if over: + continue # draining — see the docstring; counted, not stored + if size > limit: + over = True + continue + out.write(chunk) + if over: + raise refusal + 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/auth.py b/backend/app/routes/auth.py index 405acb02..2fbca25b 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -39,7 +39,7 @@ from app.auth import ( ) from app.config import settings from app.db import SessionLocal, get_db -from app.models import AuthToken, DemoWhitelist, Invite, OAuthToken, User +from app.models import AuthToken, DemoWhitelist, Invite, OAuthToken, User, display_text from app.ratelimit import RateLimiter, client_key from app.security import encrypt, hash_password, hash_token, verify_password from app.utils import canonical_email, valid_email @@ -472,7 +472,7 @@ async def callback( if email: user.google_email = email _sync_google_email(db, user, email) - user.display_name = userinfo.get("name") + user.display_name = display_text(userinfo.get("name")) user.avatar_url = userinfo.get("picture") # ADMIN_EMAILS (env) is the bootstrap admin list and always wins, so the configured admin can't # be locked out. Everyone else's role is managed in the admin UI and stored in the DB, so a @@ -594,7 +594,7 @@ def _complete_link( if linked_email: user.google_email = linked_email # see the sign-in callback: the other half of the identity if not user.display_name: - user.display_name = userinfo.get("name") + user.display_name = display_text(userinfo.get("name")) if not user.avatar_url: user.avatar_url = userinfo.get("picture") _store_token(db, user, token) diff --git a/backend/app/routes/debug_sessions.py b/backend/app/routes/debug_sessions.py index 3054364f..76345ea1 100644 --- a/backend/app/routes/debug_sessions.py +++ b/backend/app/routes/debug_sessions.py @@ -22,7 +22,7 @@ from sqlalchemy.orm import Session from app import audit, debugsession, logs, logsink from app.audit import AuditAction from app.auth import require_human -from app.db import get_db +from app.db import get_db, release_read_connection from app.issues import bundle, gitea from app.ratelimit import RateLimiter from app.refusal import refused @@ -335,7 +335,13 @@ def _upload_target( return owned_or_404(db, DebugSession, sid, user, what="debug_session") -async def _image_body(request: Request) -> bytes: +async def _image_body(request: Request, db: Session = Depends(get_db)) -> bytes: + # The owner check above has already run, and it was a SELECT — so this session is holding a + # pooled connection `idle in transaction` and would hold it for the whole 10 MiB upload, from a + # client that may be on a phone. Hand it back first; the handler's own work re-takes one. The + # `session` object the handler holds is expired by this and reloads on its next attribute read, + # which is the correct moment to touch the database again. + release_read_connection(db) return await capped_body(request, debugsession.SCREENSHOT_MAX_BYTES) diff --git a/backend/app/routes/diagnostics.py b/backend/app/routes/diagnostics.py index bdcfc9b9..8a7d4ddf 100644 --- a/backend/app/routes/diagnostics.py +++ b/backend/app/routes/diagnostics.py @@ -31,7 +31,7 @@ from sqlalchemy.orm import Session from app import debugsession, diagnostics, logs, logsink from app.auth import current_user -from app.db import get_db +from app.db import get_db, release_read_connection from app.models import DebugSession, LogEvent, User from app.ratelimit import RateLimiter from app.routes._common import capped_body @@ -73,7 +73,7 @@ class ClientEventsIn(BaseModel): events: list[ClientEvent] = Field(max_length=EVENTS_MAX) -async def _parsed_events(request: Request) -> ClientEventsIn: +async def _parsed_events(request: Request, db: Session = Depends(get_db)) -> ClientEventsIn: """The body as the model — parsed only after the size cap, which is why this is a dependency and not a plain body parameter (FastAPI reads a declared body in full before anything else runs). @@ -83,6 +83,10 @@ async def _parsed_events(request: Request) -> ClientEventsIn: media_type = request.headers.get("content-type", "").split(";")[0].strip().lower() if media_type != "application/json" and not media_type.endswith("+json"): raise HTTPException(status_code=415, detail={"code": "json_required"}) + # `current_user` resolved above on a SELECT, so this session is holding a pooled connection. + # Give it back before awaiting the body — the sink is a background beacon and its sender may be + # on a link far slower than the 512 KiB suggests. See `db.release_read_connection`. + release_read_connection(db) raw = await capped_body(request, BODY_MAX_BYTES) try: return ClientEventsIn.model_validate_json(raw) diff --git a/backend/app/routes/downloads.py b/backend/app/routes/downloads.py index 90e36197..d96a95fd 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 @@ -22,7 +24,7 @@ from sqlalchemy.orm import Session from app import audit, sysconfig from app.auth import admin_user, require_human from app.config import settings -from app.db import get_db +from app.db import get_db, release_read_connection from app.downloads import credentials as dlcreds from app.downloads import edit as editmod from app.downloads import links as linksmod @@ -37,8 +39,9 @@ from app.models import ( DownloadShare, MediaAsset, User, + display_text, ) -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 @@ -398,7 +401,7 @@ def enqueue_download( status_code=422, detail={"code": "url_blocked", "reason": e.reason} ) from e spec, profile_id = _resolve_spec(db, user, body.profile_id, body.spec) - display_name = (body.display_name or "").strip() or None + display_name = display_text(body.display_name) try: job = service.enqueue(db, user.id, source_kind, source_ref, spec, profile_id, display_name) except quota.QuotaExceeded as e: @@ -406,61 +409,119 @@ 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") + user_id = user.id + # Hand the pooled connection back BEFORE the transfer starts — see `db.release_read_connection` + # for why, and for the precondition it enforces. Everything this dependency still needs is + # already an int or a header in hand: touching an ORM attribute below would reload `user` and + # check a connection straight back out. + release_read_connection(db) 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=display_text(_header_text(request, UPLOAD_DISPLAY_NAME_HEADER)), + ) 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 @@ -480,7 +541,7 @@ def enqueue_edit( if source_job.status != "done" or source_job.asset_id is None: raise refused(409, "edit_unfinished") spec = body.edit_spec - display_name = (body.display_name or "").strip() or None + display_name = display_text(body.display_name) try: job = service.enqueue_edit(db, user.id, source_job, spec, display_name) except quota.QuotaExceeded as e: @@ -516,7 +577,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). @@ -611,9 +685,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 = display_text(body.display_name) if "display_uploader" in sent: - job.display_uploader = ((body.display_uploader or "").strip())[:255] or None + job.display_uploader = 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/app/worker.py b/backend/app/worker.py index 1aac2d5a..c5bd2ced 100644 --- a/backend/app/worker.py +++ b/backend/app/worker.py @@ -43,7 +43,7 @@ from app.downloads.jobdb import ( _staging_dir, ) from app.downloads.status import STOPPED_BY_ROUTE, JobStatus -from app.models import DownloadJob, MediaAsset +from app.models import DISPLAY_TEXT_MAX, DownloadJob, MediaAsset log = logs.get_logger(__name__) _stop = threading.Event() @@ -562,7 +562,7 @@ def _finish_from_cache(job_id: int, asset_id: int) -> None: fields = {"status": JobStatus.DONE, "progress": 100, "phase": None} if not job.display_name and asset.title: # display_name is VARCHAR(255); a title can be longer (e.g. a recipe in the title). - fields["display_name"] = asset.title[:255] + fields["display_name"] = asset.title[:DISPLAY_TEXT_MAX] asset.last_access_at = datetime.now(timezone.utc) # Atomic finalize: only marks the job done while it is still running, so a concurrent # cancel/pause is never resurrected (see service.finalize_if_running). diff --git a/backend/requirements.lock b/backend/requirements.lock index 737de225..91c17790 100644 --- a/backend/requirements.lock +++ b/backend/requirements.lock @@ -47,7 +47,6 @@ pydantic-settings==2.15.0 pydantic==2.13.4 pydantic_core==2.46.4 python-dotenv==1.2.3 -python-multipart==0.0.32 requests==2.34.2 rich==15.0.0 starlette==1.6.0 diff --git a/backend/requirements.txt b/backend/requirements.txt index c562cfff..6622eaf2 100644 --- a/backend/requirements.txt +++ b/backend/requirements.txt @@ -1,7 +1,4 @@ fastapi>=0.115,<1.0 -# Multipart form parsing — required by Starlette/FastAPI to accept file uploads (UploadFile/Form). -# The download center's local-file Upload (E7) is the only multipart endpoint in the app. -python-multipart>=0.0.9,<1.0 uvicorn[standard]>=0.30,<1.0 sqlalchemy>=2.0,<2.1 psycopg[binary]>=3.2,<4.0 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..d967643d 100644 --- a/backend/tests/test_body_limit.py +++ b/backend/tests/test_body_limit.py @@ -204,46 +204,271 @@ 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 — and a route that +# skips the helpers altogether and awaits `request.body()` lands here as `` +# rather than vanishing (round 3). +# +# The module is the path relative to `app/routes/`, so a site under `plex/` is spelled +# `plex/admin.py` and cannot collide with the top-level `admin.py`. CAPPED_BODY_SITES = { - ("diagnostics.py", "BODY_MAX_BYTES"), - ("debug_sessions.py", "debugsession.SCREENSHOT_MAX_BYTES"), + ("diagnostics.py", "_parsed_events", "capped_body", "BODY_MAX_BYTES"), + ("debug_sessions.py", "_image_body", "capped_body", "debugsession.SCREENSHOT_MAX_BYTES"), + ("downloads.py", "spool_upload", "capped_stream_to", "cap"), } -def test_every_capped_body_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.""" +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, ast.FunctionDef)) + and node.name.startswith("capped_") + } + + +def test_the_registry_covers_every_body_reading_helper(): + """The mutation guard for the tests 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." + ) + + +# The ways a route can read its own body WITHOUT one of `_common.py`'s helpers. Every `Request` +# parameter under `app/routes/` is named `request`, so the base name is an exact filter and not a +# guess — the only other `.json()` in the package is `exc.json()` on a ValidationError. +# +# Without this set the three guards below know only the `capped_*` naming convention, which means +# they are blind to the shape `_common.py` itself is written in. A route spelling `await +# request.body()` inline declares no ceiling, hands no connection back, and would be invisible to +# all three — reading as "no new sites" rather than as a failure, which is the exact way a drift +# guard dies quietly (deep-review round 3). +RAW_BODY_READS = {"body", "stream", "form", "json"} + + +def _call_name(node: ast.Call) -> str | None: + if isinstance(node.func, ast.Attribute): + return node.func.attr + return getattr(node.func, "id", None) + + +def _body_read_calls(fn: ast.AST, readers: set[str]) -> list[tuple[ast.Call, str]]: + """Every call inside `fn` that reads the request body, as (node, name) in source order.""" + out: list[tuple[ast.Call, str]] = [] + for node in ast.walk(fn): + if not isinstance(node, ast.Call): + continue + if ( + isinstance(node.func, ast.Attribute) + and isinstance(node.func.value, ast.Name) + and node.func.value.id == "request" + and node.func.attr in RAW_BODY_READS + ): + out.append((node, f"request.{node.func.attr}")) + elif _call_name(node) in readers: + out.append((node, _call_name(node) or "")) + return sorted(out, key=lambda pair: (pair[0].lineno, pair[0].col_offset)) + + +def _body_reading_functions() -> dict[tuple[str, str], dict]: + """Every function under `app/routes/` that reads a request body itself, keyed by + (module, function). + + 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. + + Keyed by the ENCLOSING FUNCTION, not just the module: a set keyed by module and limit + expression collapses two call sites that spell their limit the same way, which is exactly what + a second route copied from the first would do. + + The module half of the key is the path RELATIVE to `app/routes/`, not the basename: the scan + recurses, and `admin.py`, `discover.py` and `playlists.py` each exist both at the top level and + under `plex/`. Keying by basename would let the second of a colliding pair overwrite the first + and take its `releases` flag out of the comparison with it — the same entry-collapse this + registry was widened to close. + + `_common.py` is skipped: it is where the helpers live, so its own `request.stream()` loops are + the implementation of the rule rather than a violation of it. + """ + readers = _body_readers() routes_dir = pathlib.Path(__file__).resolve().parents[1] / "app" / "routes" - found: set[tuple[str, str]] = set() - for py in routes_dir.rglob("*.py"): + found: dict[tuple[str, str], dict] = {} + for py in sorted(routes_dir.rglob("*.py")): + if py.name == "_common.py": + continue + module = py.relative_to(routes_dir).as_posix() tree = ast.parse(py.read_text("utf-8"), filename=str(py)) - for node in ast.walk(tree): - if not isinstance(node, ast.Call): + for fn in ast.walk(tree): + if not isinstance(fn, (ast.AsyncFunctionDef, ast.FunctionDef)): continue - fn = node.func - name = fn.attr if isinstance(fn, ast.Attribute) else getattr(fn, "id", None) - if name != "capped_body": + calls = _body_read_calls(fn, readers) + if not calls: continue - limit = next( - (kw.value for kw in node.keywords if kw.arg == "limit"), - node.args[1] if len(node.args) > 1 else None, - ) - found.add((py.name, ast.unparse(limit) if limit is not None else "")) + names = [name for _, name in calls] + limits = [] + for node, name in zip([n for n, _ in calls], names): + if name.startswith("request."): + limits.append("") + continue + limit = next((kw.value for kw in node.keywords if kw.arg == "limit"), None) + if limit is None: + pos = 2 if name == "capped_stream_to" else 1 + limit = node.args[pos] if len(node.args) > pos else None + limits.append(ast.unparse(limit) if limit is not None else "") + # POSITION, not presence. `any(...)` over the function accepted a release that ran + # AFTER the await it exists to precede — the natural spelling for someone reading the + # name as "release it when you're done" — while pinning a connection for the whole + # transfer. The first body read is what the release has to beat. + releases = [ + node + for node in ast.walk(fn) + if isinstance(node, ast.Call) and _call_name(node) == "release_read_connection" + ] + first_read = calls[0][0].lineno + found[(module, fn.name)] = { + "helpers": names, + "limits": limits, + "releases": any(node.lineno < first_read for node in releases), + "releases_late": bool(releases) + and not any(node.lineno < first_read for node in releases), + } + return found + + +def test_every_body_reading_site_is_known_and_fits_under_its_ceiling(): + found = { + (module, fn, site["helpers"][i], site["limits"][i]) + for (module, fn), site in _body_reading_functions().items() + for i in range(len(site["helpers"])) + } 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_release_read_connection_ends_the_transaction_it_was_called_for(db): + """The helper itself: a SELECT has autobegun, and afterwards it has not.""" + from sqlalchemy import select + + from app.db import release_read_connection + + db.execute(select(SystemConfig).limit(1)) + assert db.in_transaction(), "nothing autobegan, so this test would pass vacuously" + release_read_connection(db) + assert not db.in_transaction() + + +def test_release_read_connection_refuses_to_discard_pending_work(db): + """The precondition, ENFORCED rather than described. + + The session belongs to the whole request, not to the dependency calling this — so if something + ahead of it ever stages a write, ending the transaction would throw that write away with + nothing in the log. `service.enqueue` already learned that a whole-session rollback takes the + caller's pending work with it, which is why it uses a SAVEPOINT. A comment cannot stop the next + dependency from being added in the wrong place; this can. + """ + from app.db import release_read_connection + + db.add(SystemConfig(key="probe_pending_write", value="1")) + with pytest.raises(RuntimeError, match="pending work"): + release_read_connection(db) + db.rollback() # the staged row is this test's own + + +def test_release_read_connection_refuses_to_discard_a_FLUSHED_write(db): + """The half the unit-of-work check cannot see. + + `db.new`/`dirty`/`deleted` empty out the moment a helper flushes, and CLAUDE.md's canonical + split is "the handler commits; helpers flush" — so the single most likely future violator is + the one shape the obvious check is blind to. Measured before the fix: all three sets empty, the + guard silent, the row gone after the rollback. + """ + from app.db import release_read_connection + + db.add(SystemConfig(key="probe_flushed_write", value="1")) + db.flush() + assert not (db.new or db.dirty or db.deleted), ( + "the flush did not clear the unit-of-work, so this test would pass for the wrong reason" + ) + with pytest.raises(RuntimeError, match="already WRITTEN"): + release_read_connection(db) + db.rollback() + + +def test_release_read_connection_refuses_to_discard_a_core_dml_write(db): + """The other half: `db.execute(update(...))` never enters the unit-of-work at all, and + `downloads/service.py` writes that way on purpose (a conditional `UPDATE … WHERE status=…` is + the honest-states pattern).""" + from sqlalchemy import insert + + from app.db import release_read_connection + + db.execute(insert(SystemConfig).values(key="probe_core_dml", value="1")) + assert not (db.new or db.dirty or db.deleted), ( + "Core DML entered the unit-of-work, so this test would pass for the wrong reason" + ) + with pytest.raises(RuntimeError, match="already WRITTEN"): + release_read_connection(db) + db.rollback() + + +def test_every_body_reading_site_hands_its_pooled_connection_back_first(): + """Reading the body yourself is what makes a request SLOW inside its own database session. + + The auth (or ownership) dependency that must resolve first is a SELECT, so SQLAlchemy has + autobegun and the session is holding one of the pool's connections; awaiting the body then pins + it for as long as the client keeps sending — one connection per in-flight upload, `idle in + transaction`, with nothing to time it out. `db.release_read_connection` ends that transaction, + and every one of these sites needs it. + + Held generically because the three sites arrived one at a time and only the newest was fixed: + a fourth body-reading route gets the same requirement automatically. + + BEFORE the read, not merely somewhere in the function. A release that runs after the await + satisfies the letter of the rule and none of its purpose — the connection was held for exactly + the span the rule exists to protect — so `releases_late` is reported as its own, louder case. + """ + sites = _body_reading_functions() + late = [f"{module}:{fn}" for (module, fn), site in sites.items() if site["releases_late"]] + missing = [ + f"{module}:{fn}" + for (module, fn), site in sites.items() + if not site["releases"] and not site["releases_late"] + ] + assert not late, ( + "a route calls db.release_read_connection AFTER it reads the body, which holds the pooled " + f"connection for exactly the transfer it was supposed to free: {late}. Move the call above " + "the await." + ) + assert not missing, ( + "a route reads its own request body while still inside its read transaction, pinning a " + f"pooled connection for the whole transfer: {missing}. Call " + "db.release_read_connection(db) before the await — see routes/downloads.py:spool_upload." + ) def test_the_upload_ceiling_and_the_admin_knob_cannot_drift_apart(): @@ -251,14 +476,72 @@ 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. + + 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")] - 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 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" + 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." + ) 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..4a3d2160 100644 --- a/backend/tests/test_upload.py +++ b/backend/tests/test_upload.py @@ -12,15 +12,23 @@ parsing (image → file, cover-art skip, dims, duration) is covered directly by below, which stubs one level down (`run_ffprobe_json`) and exercises the real `probe_media`. """ +import ast +import asyncio +import io +import pathlib from pathlib import Path import pytest +from fastapi import HTTPException from app.config import settings +from app.routes._common import capped_stream_to +from sqlalchemy import select + from app.downloads import mediakind, quota, 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 DISPLAY_TEXT_MAX, DownloadJob, DownloadQuota, MediaAsset # --- pure: the kind invariant --------------------------------------------------------------- @@ -123,15 +131,143 @@ 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 + + +def test_capped_stream_to_drains_the_rest_before_refusing(): + """Measured in a real browser, then pinned HERE rather than at the route. + + The cap now fires while the client is still sending, and answering + closing mid-upload is what + a browser reports as a NETWORK failure: the dialog said "the server couldn't be reached" + instead of naming the limit, because XHR never got to read the 422. `capped_stream_to` reads + the remainder to nowhere before raising, so the client finishes sending and reads the real + answer. + + It is asserted against the helper and not through TestClient on purpose — httpx's ASGI + transport pumps the request body whether or not the app reads it, so a route-level version of + this test passes just as happily with the drain deleted. It was written that way first and + caught by its own mutation check. + """ + pulled = [] + + async def stream(): + for i in range(8): + pulled.append(i) + yield b"x" * 1024 + + class _Req: + def stream(self): + return stream() + + out = io.BytesIO() + refusal = HTTPException(status_code=422, detail={"code": "upload_too_large", "limit": 2048}) + # `asyncio.run` rather than an async test: the suite has no async plugin, and adding one to + # pin four lines of helper would mean a new dev dependency and a lock regeneration. + with pytest.raises(HTTPException) as exc: + asyncio.run(capped_stream_to(_Req(), out, 2048, refusal)) + assert exc.value is refusal + assert len(pulled) == 8, "the body was abandoned mid-stream, not drained" + # Drained means DISCARDED: nothing past the limit is written. + assert len(out.getvalue()) <= 2048 + + # --- 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 +281,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 +292,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 +314,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 +325,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 +337,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 +346,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 +356,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 +366,391 @@ 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_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_a_long_display_name_is_truncated_on_EVERY_route_that_takes_one( + db, as_user, make_user, _root, monkeypatch, upload_headers +): + """The sweep, held end to end rather than at one site. + + `display_text` was introduced for the upload path and applied to three of the five writers of + this varchar(255) column; `POST /api/downloads` and `POST /api/downloads/edit` kept the old + `(x or "").strip() or None` and would still have answered a bare 500 (a psycopg DataError, + which main.py re-raises for everything but the NUL case) to a name a user simply pasted. Found + by four independent readers in deep-review round 2 — a sweep is exactly the kind of thing that + gets done four-fifths of the way, so this holds all of it at once. + """ + _probe(monkeypatch, ("h264", "aac", 640, 480, 60)) + monkeypatch.setattr(uploadmod.editmod, "ensure_poster", lambda _a, _root: None) + long_name = "n" * 300 + bound = "n" * DISPLAY_TEXT_MAX + c = as_user(make_user()) + + # 1. the upload route — the name arrives in a request header. + up = c.post( + "/api/downloads/upload", content=b"video", headers=upload_headers("a.mp4", long_name) + ) + assert up.status_code == 200, up.text + assert up.json()["display_name"] == bound + + # 2. the enqueue route — a plain JSON field, from the Add-download dialog. An inline `spec` + # rather than a profile id, so the test does not have to seed the builtin profile table. + enq = c.post( + "/api/downloads", + json={"source": "dQw4w9WgXcQ", "spec": {"kind": "video"}, "display_name": long_name}, + ) + assert enq.status_code == 200, enq.text + assert enq.json()["display_name"] == bound + + # 3. the edit route — the same field, from the editor. + edit = c.post( + "/api/downloads/edit", + json={ + "source_job_id": up.json()["id"], + "edit_spec": {"trim": {"start_s": 0, "end_s": 1}}, + "display_name": long_name, + }, + ) + assert edit.status_code == 200, edit.text + assert edit.json()["display_name"] == bound + + # 4 + 5. the metadata PATCH's two fields. + patched = c.patch( + f"/api/downloads/{up.json()['id']}", + json={"display_name": long_name, "display_uploader": long_name}, + ) + assert patched.status_code == 200, patched.text + assert patched.json()["display_name"] == bound + assert patched.json()["display_uploader"] == bound + + +def _display_write_sites() -> dict[tuple[str, str], set[str]]: + """Every place under `backend/app/` that puts a value into a display field, as + (module relative to `app/`, enclosing function) -> the set of value expressions written there. + + TWO SHAPES, because the first version of this guard walked only `ast.Assign` and was therefore + blind to the shape the upload route itself uses — `SpooledUpload(..., display_name=...)` is an + `ast.keyword`, not an assignment, so the one site with an unbounded REQUEST HEADER behind it was + the site the guard could not see (deep-review round 3). + + ANNOTATED assignments are excluded on purpose: `display_name: Mapped[str | None] = + mapped_column(...)` in `models.py` and `display_name: str | None = None` in a Pydantic model are + declarations of the field, not writes into it. + + The whole of `app/`, not just `app/routes/`: a job row is constructed in `downloads/service.py` + and `downloads/upload.py`, so a walk stopping at the HTTP boundary misses the writers that + actually reach the column. + """ + app_dir = pathlib.Path(__file__).resolve().parents[1] / "app" + found: dict[tuple[str, str], set[str]] = {} + for py in sorted(app_dir.rglob("*.py")): + module = py.relative_to(app_dir).as_posix() + tree = ast.parse(py.read_text("utf-8"), filename=str(py)) + enclosing: dict[int, str] = {} + for fn in ast.walk(tree): + if isinstance(fn, (ast.AsyncFunctionDef, ast.FunctionDef)): + for node in ast.walk(fn): + enclosing.setdefault(id(node), fn.name) + for node in ast.walk(tree): + if isinstance(node, ast.Assign): + names = { + t.id if isinstance(t, ast.Name) else t.attr + for t in node.targets + if isinstance(t, (ast.Name, ast.Attribute)) + } + value = node.value + elif isinstance(node, ast.keyword): + names = {node.arg} if node.arg else set() + value = node.value + else: + continue + if not names & {"display_name", "display_uploader"}: + continue + key = (module, enclosing.get(id(node), "")) + found.setdefault(key, set()).add(ast.unparse(value)) + return found + + +# Every write into a display field, with the expression each one writes. A varchar(255) column with +# a request header behind it turns a too-long name into a psycopg DataError — a 500 AFTER the whole +# file has transferred — so each of these has to be bounded, and each one here has been read. +# +# Bounded by `display_text` unless noted: +# auth.get_or_create_demo_user a literal. +# service.redownload CLEARS the field so the fresh metadata wins — `None` is the +# one value that needs no bound, and a clear is still a write. +# service.enqueue / enqueue_edit `display_name` is already bounded by its caller; the fallback +# slices the YouTube title to the same width. +# downloads.upload_download passes through `spool_upload`'s value, bounded there. +# +# Held as EXACT EQUALITY rather than as a list of offenders. The offender form could only fail +# upward: refactoring the writers into a shape the walker did not recognise left it asserting +# `not []` and enforcing nothing, which is how a drift guard dies quietly (round 3). This one fails +# when a site appears, when one disappears, and when one changes what it writes. +DISPLAY_WRITE_SITES = { + ("auth.py", "get_or_create_demo_user"): {"'Demo'"}, + ("downloads/service.py", "redownload"): {"None"}, + ("downloads/service.py", "enqueue"): { + "display_name or (asset.title[:DISPLAY_TEXT_MAX] " + "if asset.status == 'ready' and asset.title else None)" + }, + ("downloads/service.py", "enqueue_edit"): {"display_name or asset.title[:DISPLAY_TEXT_MAX]"}, + ("downloads/upload.py", "ingest_upload"): {"display_text(display_name)"}, + ("routes/auth.py", "callback"): {"display_text(userinfo.get('name'))"}, + ("routes/auth.py", "_complete_link"): {"display_text(userinfo.get('name'))"}, + ("routes/downloads.py", "enqueue_download"): {"display_text(body.display_name)"}, + ("routes/downloads.py", "enqueue_edit"): {"display_text(body.display_name)"}, + ("routes/downloads.py", "update_download_meta"): { + "display_text(body.display_name)", + "display_text(body.display_uploader)", + }, + ("routes/downloads.py", "upload_download"): {"upload.display_name"}, + ("routes/downloads.py", "spool_upload"): { + "display_text(_header_text(request, UPLOAD_DISPLAY_NAME_HEADER))" + }, +} + + +def test_no_code_path_assigns_a_display_field_without_the_shared_helper(): + """The drift guard for the test above: it holds five routes, this holds the rule. + + Parsed, not grepped. Every value that reaches a display field takes a string a user typed (or + YouTube supplied) and puts it on its way to a varchar(255) column, so every one of them must be + bounded. A writer added tomorrow fails here instead of 500ing in production. + """ + found = _display_write_sites() + moved = set(found) ^ set(DISPLAY_WRITE_SITES) + changed = { + key: (found[key], DISPLAY_WRITE_SITES[key]) + for key in set(found) & set(DISPLAY_WRITE_SITES) + if found[key] != DISPLAY_WRITE_SITES[key] + } + assert found == DISPLAY_WRITE_SITES, ( + "a display-field write appeared, moved or changed shape. Every value reaching " + "display_name/display_uploader must be bounded to models.DISPLAY_TEXT_MAX — normally by " + "models.display_text — or a name longer than the column is a 500 rather than a truncation. " + f"Sites that appeared or vanished: {moved}. Sites that changed: {changed}." + ) + + +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 == 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" * 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_stream = downloads_routes.capped_stream_to + real_release = downloads_routes.release_read_connection + + def spy_release(session): + # The PRECONDITION, so this test cannot pass vacuously. Asserting only "no transaction + # while streaming" would stay green if the auth/cap reads stopped opening one at all — + # and under `as_user` the auth SELECT is already overridden away, so that is a live risk + # rather than a theoretical one. + seen["before"] = bool(session.in_transaction()) + real_release(session) + seen["after_release"] = bool(session.in_transaction()) + + async def spy_stream(request, out, limit, refusal): + seen["while_streaming"] = bool(db.in_transaction()) + return await real_stream(request, out, limit, refusal) + + monkeypatch.setattr(downloads_routes, "release_read_connection", spy_release) + monkeypatch.setattr(downloads_routes, "capped_stream_to", spy_stream) + 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["before"] is True, ( + "nothing had opened a transaction before the release, so this test proves nothing — the " + "reads it is meant to follow must have moved or been stubbed away" + ) + assert seen["after_release"] is False + assert seen["while_streaming"] 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_the_usage_route_reports_the_per_file_upload_cap(as_user, make_user, db): + """The dialog refuses an over-cap file off this number, and it is the only layer that produces + a refusal carrying the limit on a default install — so the field is load-bearing, not + decorative. Nothing held it: the route had no test at all, and the frontend's `DownloadUsage` + is an unvalidated cast, so dropping the key would have been green everywhere (round 2). + """ + from app import sysconfig + + c = as_user(make_user()) + r = c.get("/api/downloads/usage") + assert r.status_code == 200, r.text + assert r.json()["upload_max_bytes"] == sysconfig.get_int(db, "download_upload_max_bytes") + + # And it is the KNOB, not a constant: a lowered override must show through. + sysconfig.set_value(db, "download_upload_max_bytes", 4096) + db.commit() + assert c.get("/api/downloads/usage").json()["upload_max_bytes"] == 4096 + + +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: 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/components/UploadDialog.dom.test.tsx b/frontend/src/components/UploadDialog.dom.test.tsx index ccbecf77..a02de9b8 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,98 @@ describe("UploadDialog", () => { const btn = screen.getByText("downloads.upload.submit").closest("button")!; expect(btn.disabled).toBe(true); }); + + // `file.size > cap`, on the boundary in both directions — the cap is inclusive server-side + // (backend test_upload.py pins that a body of EXACTLY the cap is accepted), so a dialog that + // refused it would make a legal file un-uploadable with no way round. Two sizes either side of + // one cap, because a single over-cap case passes just as happily with `>` loosened to `>=`. + const CAP = 8; + const pickFile = (bytes: number, name = "f.bin") => + fireEvent.change(fileInput(), { + target: { files: [new File(["x".repeat(bytes)], name)] }, + }); + + it("refuses a file one byte over the cap, 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 and may not even reach + // the browser. So the dialog checks the size itself: the user learns the limit before a byte + // moves. + h.downloadUsage.mockResolvedValue({ upload_max_bytes: CAP }); + wrap(); + await waitFor(() => expect(h.downloadUsage).toHaveBeenCalled()); + pickFile(CAP + 1, "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); + // The drop zone carries the reason, because a natively disabled Submit is not in the tab order + // to be asked for it. + const zone = screen.getByText("big.bin").closest("button")!; + expect(zone.getAttribute("aria-invalid")).toBe("true"); + expect(zone.getAttribute("aria-describedby")).toBe(screen.getByRole("alert").id); + }); + + it("still highlights as a drop target while the picked file is over the cap", async () => { + // The state a user is in when they see the refusal is "replace this file". With a file + // already picked the zone's label shows its NAME rather than `dropActive`, so the border is + // the only feedback that a drag is over a live target — and a danger border that outranks the + // drag state reads as "drag-and-drop is off now", sending the user hunting for a control that + // does not exist. The drop would have worked. + h.downloadUsage.mockResolvedValue({ upload_max_bytes: CAP }); + wrap(); + await waitFor(() => expect(h.downloadUsage).toHaveBeenCalled()); + pickFile(CAP + 1, "big.bin"); + const zone = screen.getByText("big.bin").closest("button")!; + expect(zone.className).toContain("border-danger"); + + fireEvent.dragEnter(zone); + expect(zone.className).toContain("border-accent"); + expect(zone.className).not.toContain("border-danger"); + }); + + it("swallows a Submit press that arrives while the button still holds focus", async () => { + // The OTHER half of "never sent": the test above covers the natively disabled button (not + // focused), this one covers the soft-disabled one. `ui/Button` is focus-safe, so a button that + // turns unavailable while it HOLDS focus stays in the tab order and stays clickable, and it is + // the primitive's own click guard that refuses — measured: deleting the `tooBig` term from + // `submit()` changes nothing here, which is why that term is documented as belt and braces + // rather than as the mechanism. What this pins is the user-visible contract: pressing Upload + // after dropping a bigger file does not start a transfer the server will refuse. + h.downloadUsage.mockResolvedValue({ upload_max_bytes: CAP }); + wrap(); + await waitFor(() => expect(h.downloadUsage).toHaveBeenCalled()); + pickFile(CAP, "fits.bin"); + const btn = screen.getByText("downloads.upload.submit").closest("button")!; + btn.focus(); + fireEvent.focus(btn); + pickFile(CAP + 1, "toobig.bin"); + await waitFor(() => expect(screen.getByRole("alert")).toBeTruthy()); + expect(btn.disabled).toBe(false); // soft-disabled, still focusable — and still clickable + fireEvent.click(btn); + expect(h.uploadDownload).not.toHaveBeenCalled(); + }); + + it("sends a file of EXACTLY the cap, which the server accepts", async () => { + h.downloadUsage.mockResolvedValue({ upload_max_bytes: CAP }); + wrap(); + await waitFor(() => expect(h.downloadUsage).toHaveBeenCalled()); + pickFile(CAP, "exact.bin"); + expect(screen.queryByRole("alert")).toBeNull(); + fireEvent.click(screen.getByText("downloads.upload.submit")); + await waitFor(() => expect(h.uploadDownload).toHaveBeenCalledTimes(1)); + }); + + it("re-announces the refusal when a SECOND over-cap file is picked", async () => { + // Failing the same way again has to be announced again. The message names the CAP, not the + // file, so two over-cap picks leave the node and its text identical — and a live region that + // is never touched says nothing. See formAlerts.dom.test.tsx, which holds the same rule for + // the three other alerts in this app that can repeat. + h.downloadUsage.mockResolvedValue({ upload_max_bytes: CAP }); + wrap(); + await waitFor(() => expect(h.downloadUsage).toHaveBeenCalled()); + pickFile(CAP + 1, "a.bin"); + const first = await screen.findByRole("alert"); + pickFile(CAP + 2, "b.bin"); + await waitFor(() => expect(screen.getByRole("alert")).not.toBe(first)); + }); }); diff --git a/frontend/src/components/UploadDialog.tsx b/frontend/src/components/UploadDialog.tsx index c6670db4..1b6a762e 100644 --- a/frontend/src/components/UploadDialog.tsx +++ b/frontend/src/components/UploadDialog.tsx @@ -1,16 +1,18 @@ -import { useEffect, useRef, useState } from "react"; +import { useEffect, useId, 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"; import { notify } from "../lib/notifications"; import { navigateTo } from "../lib/nav"; import { formatBytes } from "../lib/format"; +import { DISPLAY_NAME_MAX } from "../lib/api/downloads"; import { inputCls } from "./ui/form"; import { FocusSafeButton } from "./ui/FocusSafeButton"; @@ -31,6 +33,26 @@ 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; + const refusalId = useId(); + // Every pick gets a new number, so the refusal below REMOUNTS rather than being reconciled. Two + // over-cap picks in a row leave the node and its text identical — the message names the cap, not + // the file — and a live region that is not touched is not announced, so the second refusal was + // silent to a screen reader. `KeyGate`, `SetupWizard` and `Welcome` all key their alerts for the + // same reason, and `formAlerts.dom.test.tsx` is the test that exists because of it. + const [pickSeq, setPickSeq] = useState(0); + const pick_ = (f: File) => { + setFile(f); + setName(""); + setPickSeq((n) => n + 1); + }; // 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 +62,11 @@ export default function UploadDialog({ onClose }: { onClose: () => void }) { const pick = () => inputRef.current?.click(); const submit = async () => { - if (!file || busy) return; + // `tooBig` here is belt and braces, NOT the mechanism: `ui/Button` is focus-safe, so while the + // button holds focus it swallows the activation itself, and while it does not it is natively + // disabled. Measured — deleting this term breaks nothing, which is exactly why it must not be + // described as the guard. It is here for the next caller of `submit`, not for the only one. + if (!file || busy || tooBig) return; const ac = new AbortController(); abortRef.current = ac; setPct(0); @@ -94,14 +120,28 @@ export default function UploadDialog({ onClose }: { onClose: () => void }) { dragDepth.current = 0; setDragOver(false); const f = e.dataTransfer.files?.[0]; - if (f) { - setFile(f); - setName(""); - } + if (f) pick_(f); }} disabled={busy} + // The refusal is tied to the control it invalidates, not left to a one-shot announcement: + // the drop zone reads as invalid and points at the sentence, so a reader who tabs back to + // it hears WHY. (Measured, because `aria-invalid` is not in `button`'s ARIA-supported + // list: Chromium's accessibility node for a native button does carry `invalid` from this + // attribute.) The signal that is NOT colour is the sentence below, visible to everyone — + // the border only reinforces it. + aria-invalid={tooBig} + aria-describedby={tooBig ? refusalId : undefined} + // `dragOver` outranks `tooBig`: a drag is transient and the refusal repaints the instant + // the drop lands, whereas a zone that stays red under the pointer tells the user their + // drop target is dead — in the one state where they are trying to replace the file. With + // a file picked, the label below shows its name rather than `dropActive`, so this border + // is the ONLY feedback that the zone will accept the drop. className={`w-full flex flex-col items-center justify-center gap-2 rounded-xl border-2 border-dashed px-4 py-8 text-center transition disabled:opacity-60 ${ - dragOver ? "border-accent bg-accent/5" : "border-border hover:border-fg/40" + dragOver + ? "border-accent bg-accent/5" + : tooBig + ? "border-danger" + : "border-border hover:border-fg/40" }`} > {file ? ( @@ -125,22 +165,31 @@ export default function UploadDialog({ onClose }: { onClose: () => void }) { className="hidden" onChange={(e) => { const f = e.target.files?.[0]; - if (f) { - setFile(f); - setName(""); - } + if (f) pick_(f); e.target.value = ""; // let re-picking the same file fire onChange again }} /> -