diff --git a/CLAUDE.md b/CLAUDE.md index 84c8184f..e7a6f481 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -57,7 +57,7 @@ how it gets built. Concretely: `routes/*` (HTTP only: validation, auth-dependency, serialization) → domain packages `sync/`, `plex/`, `downloads/`, `youtube/`, `issues/` (shared by routes + scheduler + worker) → flat core (`auth`, `quota`, -`sysconfig`, `state`, `audit`, `notifications`, `userscope`, `realtime`, `ratelimit`, `progress`, +`sysconfig`, `state`, `audit`, `notifications`, `userscope`, `realtime`, `ratelimit`, `bodylimit`, `progress`, `security`, `logs`, `logsink`, `diagnostics`, `debugsession`) → `models/db/config`. **Dependency direction is one-way: core never imports routes.** Frontend: `main.tsx` provider tree → `App.tsx` shell → `moduleRegistry` route-chunks → module components → the **platform layer `components/ui/`** (domain-free primitives: the popover engine and @@ -78,6 +78,7 @@ CQRS, a repository/DI framework, a router library, a generic CRUD-router factory | ----------------------- | ------------------------------------------------------------ | ------------------------------------------------------ | | ORM | SQLAlchemy 2.0 `select()` | `db.query(...)` | | Request body | a Pydantic model | `payload: dict` | +| Request SIZE | nothing — `bodylimit` caps everything (SEC-20) | `capped_body`, except for a LARGER per-route cap | | Ownership check | `owned_or_404` / `get_or_404` | ad-hoc `_own_playlist`/`_owned`/`_own_job` | | Admin gate import | `app.auth` | the `app.routes.admin` re-export | | Upstream failure status | `422` with JSON detail | `502` (reserve 502 for raw byte-proxies) | diff --git a/backend/app/bodylimit.py b/backend/app/bodylimit.py new file mode 100644 index 00000000..68ffd36c --- /dev/null +++ b/backend/app/bodylimit.py @@ -0,0 +1,183 @@ +"""The global request-body cap (SEC-20). + +**The hole.** FastAPI reads a declared body IN FULL before auth or any other dependency runs — the +body parameter has to be parsed to be passed in. So `POST /auth/password-login` drains a 32 MiB +upload from an anonymous caller and only then answers 401. 70 of the 72 body-declaring routes behave +the same way: one unauthenticated client can spend the process's memory and time at will. + +**Why a middleware and not `capped_body` on every route.** `routes/_common.py:capped_body` is the +right tool where a route genuinely takes bulk bytes, and the two routes that use it use it +correctly. But putting it on all 70 others would (a) fight the canonical "Request body → a Pydantic +model" row, since capping means taking the body as raw bytes and parsing it by hand, and (b) leave +the NEXT route added uncovered, because nothing would remind its author. A default that has to be +opted into is not a default. This module inverts it: the cap applies to everything, and the few +routes that need more opt OUT, in one list, next to the code that reads it. + +**Why raw ASGI and not `@app.middleware("http")`.** Starlette's BaseHTTPMiddleware cannot wrap +`receive`, so it can only read `Content-Length` — which a client may omit entirely (chunked) or +simply understate. The bytes have to be counted as they arrive, which is the same rule `capped_body` +already applies per-route, and counting needs the receive channel. + +**Why the refusal is byte-identical to `capped_body`'s.** Two shapes for one fact is how the +`cancelled`/`canceled` bug happened. The 413 body here and the 413 body from `capped_body` are the +same JSON, and `tests/test_body_limit.py` compares them against each other rather than against a +literal, so they cannot drift apart. + +**No admin knob, deliberately.** The ceiling is a constant, not a `sysconfig` value: an attacker- +facing limit that an admin can raise from the UI is a limit that can be raised by whoever gets the +admin session, and nobody has asked to tune it. If a self-hoster ever needs a different ceiling it +belongs in `config.py` (env, set before the process starts), not in the database. +""" + +import json +import re + +from starlette.types import ASGIApp, Message, Receive, Scope, Send + +# 1 MiB. The largest body any non-exempt route legitimately takes is 64 KiB (the debug-session form, +# `routes/debug_sessions.py:FORM_MAX_BYTES`), so this leaves ~16x headroom: a ceiling that refuses a +# legitimate request would be a worse bug than the one this module fixes. +DEFAULT_MAX_BODY_BYTES = 1024 * 1024 + +# The routes that opt out, as (method, path TEMPLATE) — the template, not a concrete path, so the +# list reads like the route decorators it mirrors and `test_body_limit.py` can assert that each one +# still resolves to a real route. A renamed route therefore fails the gate instead of silently +# losing its exemption in prod. +# +# A route belongs here ONLY IF ITS OWN CAP IS LARGER THAN THE GLOBAL ONE. That is the whole +# admission rule, and it is why `POST /api/diagnostics/events` is absent: its `capped_body` limit is +# 512 KiB, already below this ceiling, so the global cap never fires on it and an exemption would +# only widen what it accepts. Both entries below enforce their own limit on the bytes they actually +# read, which is what makes opting them out safe rather than merely convenient: +# /api/downloads/upload streams in 1 MiB chunks, caps on the real byte count (E7 S5); +# ingesting a local video file, so it is legitimately unbounded here +# /api/debug-sessions/{sid}/screenshots capped_body at SCREENSHOT_MAX_BYTES (10 MiB), and the +# owner check runs as a dependency BEFORE the body is read +EXEMPT_ROUTES: frozenset[tuple[str, str]] = frozenset( + { + ("POST", "/api/downloads/upload"), + ("POST", "/api/debug-sessions/{sid}/screenshots"), + } +) + +# The refusal, in the exact shape `capped_body` raises — `HTTPException(413, {"code": "too_large"})` +# renders as this. Built once; the test asserts it against a real route's response. +TOO_LARGE_BODY = json.dumps({"detail": {"code": "too_large"}}).encode() + +_PATH_PARAM = re.compile(r"\{[^/{}]+\}") + + +def template_to_regex(template: str) -> re.Pattern[str]: + """`/api/debug-sessions/{sid}/screenshots` → a regex matching one concrete path. + + A path parameter matches `[^/]+` — one segment, never across a `/`, so `{sid}` cannot swallow a + deeper path and hand an exemption to a route that was never granted one. + """ + literals = [re.escape(part) for part in _PATH_PARAM.split(template)] + return re.compile("^" + "[^/]+".join(literals) + "$") + + +def _declared_length(scope: Scope) -> int | None: + """The `Content-Length` header as an int, or None when absent or unparseable (a chunked body has + none, and a malformed one is not a number we may trust — both fall through to counting).""" + for name, value in scope.get("headers", ()): + if name == b"content-length": + try: + return int(value) + except ValueError: + # silent-ok: a malformed Content-Length is the caller's doing, not a fault of ours, + # and it is about to be answered anyway — returning None drops the request into the + # counting path, which refuses on the real bytes. Logging it would hand an anonymous + # client a free line in our log for every request it cares to malform. + return None + return None + + +async def _send_too_large(send: Send) -> None: + await send( + { + "type": "http.response.start", + "status": 413, + "headers": [ + (b"content-type", b"application/json"), + (b"content-length", str(len(TOO_LARGE_BODY)).encode()), + ], + } + ) + await send({"type": "http.response.body", "body": TOO_LARGE_BODY}) + + +class BodyLimitMiddleware: + """Refuses any request body over `limit` bytes, before the application sees it. + + Registered second-outermost in `main.py` — inside the access log so a refusal is still logged + with its request id, outside everything else so the session, CORS, setup-gate and auth layers + never run for a request that is already too large. + """ + + def __init__( + self, + app: ASGIApp, + *, + limit: int = DEFAULT_MAX_BODY_BYTES, + exempt: frozenset[tuple[str, str]] = EXEMPT_ROUTES, + ) -> None: + self.app = app + self.limit = limit + self._exempt = tuple((method, template_to_regex(path)) for method, path in exempt) + + def is_exempt(self, method: str, path: str) -> bool: + return any(m == method and rx.match(path) for m, rx in self._exempt) + + async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: + if scope["type"] != "http" or self.is_exempt( + scope.get("method", ""), scope.get("path", "") + ): + await self.app(scope, receive, send) + return + + declared = _declared_length(scope) + if declared is not None and declared > self.limit: + # Refused on the declaration alone: nothing downstream is called, and not one byte of + # the body is read. + await _send_too_large(send) + return + + seen = 0 + refused = False + started = False + + async def counting_receive() -> Message: + nonlocal seen, refused + message = await receive() + if message["type"] != "http.request": + return message + seen += len(message.get("body", b"")) + if seen > self.limit: + # The app is blocked on this receive, so it has not answered yet. Telling it the + # client went away is the only in-band way to stop it reading: `request.stream()` + # turns an `http.disconnect` into ClientDisconnect, which unwinds the handler. Our + # own 413 goes out below, once the app has finished unwinding. + refused = True + return {"type": "http.disconnect"} + return message + + async def guarded_send(message: Message) -> None: + nonlocal started + if refused and not started: + return # discard the app's answer to the disconnect; the 413 below is the response + started = True + await send(message) + + try: + await self.app(scope, counting_receive, guarded_send) + except Exception: # noqa: BLE001 — only swallowed when WE caused it, see below + # ClientDisconnect (or whatever a handler raises on a truncated read) is the expected + # consequence of the disconnect we just faked, and the caller must get the 413 rather + # than a 500. Anything raised when we did NOT refuse is a real failure and is re-raised + # untouched. + if not refused: + raise + + if refused and not started: + await _send_too_large(send) diff --git a/backend/app/main.py b/backend/app/main.py index c3856349..457b2742 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -23,6 +23,7 @@ from fastapi.staticfiles import StaticFiles from sqlalchemy.exc import DataError from starlette.middleware.sessions import SessionMiddleware +from app import bodylimit from app import logsink from app import state from app import sysconfig @@ -257,6 +258,14 @@ async def security_headers(request, call_next): return response +# SEC-20 — the global request-body cap (app/bodylimit.py). Registered SECOND-TO-LAST, i.e. directly +# INSIDE the access log: a refusal is still logged with its request id and duration, but everything +# below this line — the session, CORS, the setup gate, and every auth dependency under the router — +# never runs for a body that is already too large. FastAPI reads a declared body in full before the +# first dependency, so without this the anonymous login route is a free memory/time sink. +app.add_middleware(bodylimit.BodyLimitMiddleware) + + # The access log (app/logs.py): request id, route template, status, duration — one line, never a # query string. Registered LAST so it is the OUTERMOST http middleware and therefore times the whole # request, including what the three above do. Being outermost also puts it OUTSIDE SessionMiddleware, diff --git a/backend/tests/test_body_limit.py b/backend/tests/test_body_limit.py new file mode 100644 index 00000000..d8a03ade --- /dev/null +++ b/backend/tests/test_body_limit.py @@ -0,0 +1,235 @@ +"""SEC-20 — the global request-body cap (`app/bodylimit.py`). + +Two halves, deliberately kept apart: + +* the MIDDLEWARE tests drive a toy ASGI app, so they pin the mechanism (declared length, counted + bytes, the exemption match) with no router, no auth and no database in the way; +* the APP tests drive the real application, so they pin that the middleware is actually wired in, + sits ahead of the auth dependencies, and refuses in the same shape `capped_body` does. + +The pair that matters most is "an oversized body is refused" together with "a normal body still +gets through": the first alone would pass just as well if the cap refused *everything*. +""" + +import json + +import pytest +from fastapi.testclient import TestClient +from starlette.requests import Request + +from app import bodylimit +from app.bodylimit import DEFAULT_MAX_BODY_BYTES, BodyLimitMiddleware +from app.routes import diagnostics as diagnostics_routes + +SMALL = b"x" * 16 +OVER = b"x" * (DEFAULT_MAX_BODY_BYTES + 1) + + +async def _reading_app(scope, receive, send): + """A toy app that reads the whole body the way a route does, then reports its length. + + It uses a real Starlette `Request`, so the faked `http.disconnect` reaches it as the real + ClientDisconnect a real handler would see — the middleware's exception path is exercised, not + stubbed around. + """ + body = await Request(scope, receive).body() + payload = json.dumps({"read": len(body)}).encode() + await send( + { + "type": "http.response.start", + "status": 200, + "headers": [(b"content-type", b"application/json")], + } + ) + await send({"type": "http.response.body", "body": payload}) + + +def _client(**kwargs) -> TestClient: + return TestClient(BodyLimitMiddleware(_reading_app, **kwargs)) + + +def _chunks(total: int, size: int = 64 * 1024): + """A body with NO Content-Length: httpx sends an iterator chunked, which is exactly the shape a + declared-length check cannot see.""" + sent = 0 + while sent < total: + n = min(size, total - sent) + yield b"x" * n + sent += n + + +# --- the mechanism ------------------------------------------------------------------------------ + + +def test_a_declared_length_over_the_cap_is_refused_without_reading_the_body(): + r = _client().post("/anything", content=OVER) + assert r.status_code == 413 + assert r.content == bodylimit.TOO_LARGE_BODY + + +def test_a_body_under_the_cap_is_passed_through_untouched(): + # The mutation guard for the test above: if the cap refused everything, this would fail. + r = _client().post("/anything", content=SMALL) + assert r.status_code == 200 + assert r.json() == {"read": len(SMALL)} + + +def test_a_chunked_body_is_refused_on_the_COUNTED_bytes(): + # No Content-Length at all, so the only thing that can stop this is counting as it arrives. + r = _client().post("/anything", content=_chunks(DEFAULT_MAX_BODY_BYTES + 64 * 1024)) + assert r.status_code == 413 + assert r.content == bodylimit.TOO_LARGE_BODY + + +def test_a_chunked_body_under_the_cap_still_arrives_whole(): + size = DEFAULT_MAX_BODY_BYTES - 1 + r = _client().post("/anything", content=_chunks(size)) + assert r.status_code == 200 + assert r.json() == {"read": size} + + +def test_an_understated_content_length_does_not_buy_a_pass(): + # The declared-length check is the cheap half; a client that lies must still be counted out. + r = _client().post("/anything", content=OVER, headers={"content-length": "10"}) + assert r.status_code == 413 + + +def test_exactly_at_the_cap_is_allowed(): + size = DEFAULT_MAX_BODY_BYTES + r = _client(limit=size).post("/anything", content=b"x" * size) + assert r.status_code == 200 + assert r.json() == {"read": size} + + +# --- the exemption list ------------------------------------------------------------------------- + + +def test_an_exempt_route_may_send_a_body_over_the_global_cap(): + client = _client(exempt=frozenset({("POST", "/api/things/{tid}/blob")})) + r = client.post("/api/things/abc/blob", content=OVER) + assert r.status_code == 200 + assert r.json() == {"read": len(OVER)} + + +def test_the_exemption_is_scoped_to_its_method_and_its_path(): + client = _client(exempt=frozenset({("POST", "/api/things/{tid}/blob")})) + # Same path, different method. + assert client.put("/api/things/abc/blob", content=OVER).status_code == 413 + # A neighbour that merely starts the same way. + assert client.post("/api/things/abc/blob/extra", content=OVER).status_code == 413 + + +def test_a_path_parameter_cannot_swallow_a_slash(): + # `{tid}` matching greedily across `/` would hand the exemption to routes that never got one. + mw = BodyLimitMiddleware(_reading_app, exempt=frozenset({("POST", "/a/{tid}/b")})) + assert mw.is_exempt("POST", "/a/one/b") + assert not mw.is_exempt("POST", "/a/one/two/b") + + +def _declared_routes(root) -> set[tuple[str, str]]: + """Every (method, path template) the app serves. + + It has to RECURSE: since FastAPI 0.141 / Starlette 1.6 an `include_router` leaves an opaque + `_IncludedRouter` in `app.routes` and keeps the real routes under its `original_router`, instead + of flattening them into the parent as older versions did. Reading `app.routes` alone finds 35 + entries and none of ours; walking the tree finds all 252. + """ + found: set[tuple[str, str]] = set() + 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.add((method, route.path)) + return found + + +@pytest.mark.parametrize(("method", "template"), sorted(bodylimit.EXEMPT_ROUTES)) +def test_every_exempt_template_still_resolves_to_a_real_route(method, template): + """The drift guard. The exemption list names route templates that live in another file; a + renamed route must fail the gate here, not lose its exemption silently in production.""" + from app.main import app + + assert (method, template) in _declared_routes(app) + + +def test_a_route_that_caps_itself_below_the_global_ceiling_is_NOT_exempt(): + """The admission rule, enforced rather than described: an exemption is for routes whose own cap + is LARGER than the global one. The client log sink caps itself at 512 KiB, so the global ceiling + never fires on it and exempting it would only widen what it accepts.""" + assert diagnostics_routes.BODY_MAX_BYTES <= DEFAULT_MAX_BODY_BYTES + assert not any(path.startswith("/api/diagnostics") for _, path in bodylimit.EXEMPT_ROUTES) + + +def test_the_screenshot_exemption_is_justified_by_its_own_larger_cap(): + from app import debugsession + + assert debugsession.SCREENSHOT_MAX_BYTES > DEFAULT_MAX_BODY_BYTES + assert ("POST", "/api/debug-sessions/{sid}/screenshots") in bodylimit.EXEMPT_ROUTES + + +# --- the real application ----------------------------------------------------------------------- + + +def test_the_login_route_is_refused_before_a_single_dependency_runs(client, monkeypatch): + """`/auth/password-login` is the reason SEC-20 exists: it is anonymous, it declares a body, and + FastAPI reads that body in full before the first dependency. `_login_limiter.allow` is the very + first statement of the handler, so it is the sharpest available proof that nothing ran.""" + from app.routes import auth as auth_routes + + reached = False + + def _record(*args, **kwargs): + nonlocal reached + reached = True + return True + + monkeypatch.setattr(auth_routes._login_limiter, "allow", _record) + + r = client.post("/auth/password-login", content=OVER, headers={"content-type": "text/plain"}) + assert r.status_code == 413 + assert not reached, "the handler ran — the cap is inside the router instead of ahead of it" + + +def test_a_normal_login_body_still_reaches_the_handler(client, monkeypatch): + """The mutation check for the test above. If the cap refused every body, the 413 test would + pass while the app was broken; this one would not.""" + from app.routes import auth as auth_routes + + reached = False + + def _record(*args, **kwargs): + nonlocal reached + reached = True + return True + + monkeypatch.setattr(auth_routes._login_limiter, "allow", _record) + + r = client.post("/auth/password-login", json={"email": "nobody@example.com", "password": "x"}) + assert r.status_code == 401 + assert reached + + +def test_the_global_refusal_is_byte_identical_to_the_per_route_one(db, as_user, make_user): + """Two shapes for one fact is how the `cancelled`/`canceled` bug happened. The log sink caps + itself with `capped_body`; its 413 and the middleware's must be the same bytes. + + Signed in on purpose: `current_user` resolves before the body dependency, so an anonymous call + would be answered 401 and never reach the per-route cap this compares against. The body is over + the SINK's 512 KiB but under the global 1 MiB, so it is `capped_body` that refuses it. + """ + user = make_user() + db.commit() + over_the_sink = b"x" * (diagnostics_routes.BODY_MAX_BYTES + 1) + assert len(over_the_sink) < DEFAULT_MAX_BODY_BYTES + r = as_user(user).post( + "/api/diagnostics/events", + content=over_the_sink, + headers={"content-type": "application/json"}, + ) + assert r.status_code == 413 + assert r.json() == json.loads(bodylimit.TOO_LARGE_BODY)