diff --git a/CLAUDE.md b/CLAUDE.md index 1db1015c..c256bd4f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -79,7 +79,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 | +| Request SIZE | nothing — `bodylimit` caps every route (SEC-20); need more? add a ceiling to `ROUTE_MAX_BODY_BYTES` | `capped_body` on a route that does not read raw bytes itself | | 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 index 68ffd36c..37090fb7 100644 --- a/backend/app/bodylimit.py +++ b/backend/app/bodylimit.py @@ -19,9 +19,12 @@ simply understate. The bytes have to be counted as they arrive, which is the sam 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. +`cancelled`/`canceled` bug happened. The 413 body here is built through the same `JSONResponse` +FastAPI renders `HTTPException(413, {"code": "too_large"})` with — not a hand-rolled `json.dumps`, +whose default separators differ by two bytes — and `tests/test_body_limit.py` compares the RAW BYTES +of the two against each other rather than against a literal, so they cannot drift apart. The same +reasoning decides where the middleware is registered (`main.py`): just inside CORS, so this 413 +carries the same headers `capped_body`'s does. **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 @@ -29,40 +32,50 @@ admin session, and nobody has asked to tune it. If a self-hoster ever needs a di belongs in `config.py` (env, set before the process starts), not in the database. """ -import json import re +from starlette.responses import JSONResponse 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. +from app import debugsession +from app.config import settings + +# 1 MiB. The largest body any route on the default ceiling legitimately takes is 512 KiB (the client +# log sink, `routes/diagnostics.py:BODY_MAX_BYTES`), so this leaves 2x headroom — a ceiling that +# refuses a legitimate request would be a worse bug than the one this module fixes. That ratio is +# the load-bearing fact, so `test_body_limit.py` holds it rather than the figure above. 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. +# The routes that need MORE than the default, as (method, path TEMPLATE) → their own ceiling. 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 +# fails the gate instead of silently losing its allowance 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"), - } -) +# 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. +# +# 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. +ROUTE_MAX_BODY_BYTES: dict[tuple[str, str], int] = { + # Ingesting a local video file. `sysconfig`'s `download_upload_max_bytes` is bounded by this + # same setting (see sysconfig.SPECS), so the admin knob can never ask for more than the edge + # will pass — the two numbers cannot drift into refusing a legitimate upload. + ("POST", "/api/downloads/upload"): settings.download_upload_max_bytes, + ("POST", "/api/debug-sessions/{sid}/screenshots"): debugsession.SCREENSHOT_MAX_BYTES, +} -# 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() +# The refusal, byte-for-byte what `capped_body`'s `HTTPException(413, {"code": "too_large"})` renders +# as — built through the same JSONResponse so the separators match too, and asserted against a real +# route's raw bytes in `test_body_limit.py` rather than against a literal here. +TOO_LARGE_BODY = JSONResponse({"detail": {"code": "too_large"}}, status_code=413).body _PATH_PARAM = re.compile(r"\{[^/{}]+\}") @@ -108,11 +121,14 @@ async def _send_too_large(send: Send) -> None: class BodyLimitMiddleware: - """Refuses any request body over `limit` bytes, before the application sees it. + """Refuses any request body over its route's ceiling, 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. + Registered just inside CORS in `main.py` — so the 413 is decorated like every other response + and a browser can actually read it — but outside the session, the setup gate and every auth + dependency, none of which run for a request that is already too large. + + Every request gets a finite ceiling: `ROUTE_MAX_BODY_BYTES` raises it for the two routes that + genuinely take bulk bytes, and there is no path that skips counting altogether. """ def __init__( @@ -120,24 +136,31 @@ class BodyLimitMiddleware: app: ASGIApp, *, limit: int = DEFAULT_MAX_BODY_BYTES, - exempt: frozenset[tuple[str, str]] = EXEMPT_ROUTES, + routes: dict[tuple[str, str], int] | None = None, ) -> None: self.app = app self.limit = limit - self._exempt = tuple((method, template_to_regex(path)) for method, path in exempt) + self._routes = tuple( + (method, template_to_regex(path), cap) + for (method, path), cap in (ROUTE_MAX_BODY_BYTES if routes is None else routes).items() + ) - def is_exempt(self, method: str, path: str) -> bool: - return any(m == method and rx.match(path) for m, rx in self._exempt) + def limit_for(self, method: str, path: str) -> int: + """This request's ceiling: the route's own if it has one, else the global default. Every + request gets a finite number — there is no path through here that is simply not counted.""" + for m, rx, cap in self._routes: + if m == method and rx.match(path): + return cap + return self.limit 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", "") - ): + if scope["type"] != "http": await self.app(scope, receive, send) return + limit = self.limit_for(scope.get("method", ""), scope.get("path", "")) declared = _declared_length(scope) - if declared is not None and declared > self.limit: + if declared is not None and declared > limit: # Refused on the declaration alone: nothing downstream is called, and not one byte of # the body is read. await _send_too_large(send) @@ -153,7 +176,7 @@ class BodyLimitMiddleware: if message["type"] != "http.request": return message seen += len(message.get("body", b"")) - if seen > self.limit: + if seen > 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 diff --git a/backend/app/main.py b/backend/app/main.py index 457b2742..caba3f7f 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -190,6 +190,19 @@ app.add_middleware( https_only=settings.session_https_only, # Secure flag when served over HTTPS (prod) ) +# SEC-20 — the global request-body cap (app/bodylimit.py). FastAPI reads a declared body IN FULL +# before the first dependency, so without this the anonymous login route is a free memory/time sink. +# +# WHERE it sits is the whole design. Registered directly after SessionMiddleware, so — last +# registered being outermost — it ends up just INSIDE CORS and just OUTSIDE the session: the 413 is +# decorated by CORS and the security headers like every other response (a refusal a browser cannot +# read is no better than a hang, and `capped_body`'s 413 one layer down is decorated), while the +# session cookie, the setup gate and every auth dependency under the router still never run for a +# body that is already too large. Registering it outside CORS instead would answer a split-origin +# caller with a response the browser drops — two shapes for one fact, which is exactly what +# bodylimit.py exists to avoid. +app.add_middleware(bodylimit.BodyLimitMiddleware) + if settings.frontend_origin: app.add_middleware( CORSMiddleware, @@ -258,17 +271,10 @@ 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, +# request, including what every layer above does — the body cap included, so a 413 is still logged +# with its request id and duration. Being outermost also puts it OUTSIDE SessionMiddleware, # which is why the account comes from `request.state.user_id` (set by the auth dependency, the one # place a session is actually resolved into an account) rather than from `request.session`. app.middleware("http")(log_requests) diff --git a/backend/app/sysconfig.py b/backend/app/sysconfig.py index 1c9f1668..9b3d40fc 100644 --- a/backend/app/sysconfig.py +++ b/backend/app/sysconfig.py @@ -140,7 +140,18 @@ SPECS: tuple[ConfigSpec, ...] = ( # 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. - ConfigSpec("download_upload_max_bytes", "int", "downloads", "download_upload_max_bytes", min=1), + # `max` is not cosmetic: `bodylimit.ROUTE_MAX_BODY_BYTES` refuses an upload body over this same + # 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. + ConfigSpec( + "download_upload_max_bytes", + "int", + "downloads", + "download_upload_max_bytes", + min=1, + max=settings.download_upload_max_bytes, + ), ConfigSpec( "download_retention_days", "int", "downloads", "download_retention_days", min=0, max=3_650 ), diff --git a/backend/tests/test_body_limit.py b/backend/tests/test_body_limit.py index d8a03ade..bd9b00de 100644 --- a/backend/tests/test_body_limit.py +++ b/backend/tests/test_body_limit.py @@ -17,7 +17,7 @@ import pytest from fastapi.testclient import TestClient from starlette.requests import Request -from app import bodylimit +from app import bodylimit, sysconfig from app.bodylimit import DEFAULT_MAX_BODY_BYTES, BodyLimitMiddleware from app.routes import diagnostics as diagnostics_routes @@ -101,18 +101,36 @@ def test_exactly_at_the_cap_is_allowed(): assert r.json() == {"read": size} -# --- the exemption list ------------------------------------------------------------------------- +# --- the per-route ceilings --------------------------------------------------------------------- + +BIG = {("POST", "/api/things/{tid}/blob"): DEFAULT_MAX_BODY_BYTES * 4} -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) +def test_a_route_with_a_raised_ceiling_may_send_a_body_over_the_global_cap(): + r = _client(routes=BIG).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")})) +def test_a_raised_ceiling_is_still_a_ceiling(): + """The difference between this and an exemption, and the reason the list is a mapping: a route + that needs more bytes does not thereby get unlimited bytes. Left uncounted, the upload route + would let an ANONYMOUS caller stream without bound — FastAPI parses a declared body before the + first dependency, so `require_human` has not run yet when the bytes land.""" + over = b"x" * (BIG[("POST", "/api/things/{tid}/blob")] + 1) + r = _client(routes=BIG).post("/api/things/abc/blob", content=over) + assert r.status_code == 413 + assert r.content == bodylimit.TOO_LARGE_BODY + + +def test_a_raised_ceiling_counts_a_chunked_body_too(): + cap = BIG[("POST", "/api/things/{tid}/blob")] + r = _client(routes=BIG).post("/api/things/abc/blob", content=_chunks(cap + 64 * 1024)) + assert r.status_code == 413 + + +def test_the_raised_ceiling_is_scoped_to_its_method_and_its_path(): + client = _client(routes=BIG) # Same path, different method. assert client.put("/api/things/abc/blob", content=OVER).status_code == 413 # A neighbour that merely starts the same way. @@ -120,10 +138,20 @@ def test_the_exemption_is_scoped_to_its_method_and_its_path(): 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") + # `{tid}` matching greedily across `/` would hand the raised ceiling to routes without one. + mw = BodyLimitMiddleware(_reading_app, routes={("POST", "/a/{tid}/b"): 99}) + assert mw.limit_for("POST", "/a/one/b") == 99 + assert mw.limit_for("POST", "/a/one/two/b") == DEFAULT_MAX_BODY_BYTES + + +def test_every_request_gets_a_finite_ceiling(): + """There is no pass-through path. A route that is not listed gets the default; one that is + listed gets its own; nothing is simply not counted.""" + mw = BodyLimitMiddleware(_reading_app) + assert mw.limit_for("GET", "/api/anything") == DEFAULT_MAX_BODY_BYTES + for (method, template), cap in bodylimit.ROUTE_MAX_BODY_BYTES.items(): + assert cap > DEFAULT_MAX_BODY_BYTES # the admission rule: raised, never lowered + assert mw.limit_for(method, template.replace("{sid}", "abc")) == cap def _declared_routes(root) -> set[tuple[str, str]]: @@ -148,33 +176,58 @@ def _declared_routes(root) -> set[tuple[str, str]]: 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.""" +@pytest.mark.parametrize(("method", "template"), sorted(bodylimit.ROUTE_MAX_BODY_BYTES)) +def test_every_listed_template_still_resolves_to_a_real_route(method, template): + """The drift guard. The list names route templates that live in another file; a renamed route + must fail the gate here, not lose its allowance 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.""" +def test_the_default_ceiling_clears_the_largest_body_a_default_route_takes(): + """The headroom the module's docblock claims, held as a ratio rather than as a figure in prose. + The client log sink caps itself at 512 KiB — below the default, which is why it is not listed: + the global ceiling never fires on it and raising it could 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) + assert not any( + path.startswith("/api/diagnostics") for _, path in bodylimit.ROUTE_MAX_BODY_BYTES + ) -def test_the_screenshot_exemption_is_justified_by_its_own_larger_cap(): +def test_the_upload_ceiling_and_the_admin_knob_cannot_drift_apart(): + """The upload route's in-handler cap is a `sysconfig` value an admin can change; this middleware + is a constant. If the knob could be set above the ceiling, an admin raising it would silently + start getting 413s from a layer the admin page never mentions. The spec's `max` is what stops + that, and it is the same number.""" + spec = next(s for s in sysconfig.SPECS if s.key == "download_upload_max_bytes") + assert spec.max == bodylimit.ROUTE_MAX_BODY_BYTES[("POST", "/api/downloads/upload")] + + +def test_the_screenshot_ceiling_matches_the_cap_the_route_itself_enforces(): from app import debugsession assert debugsession.SCREENSHOT_MAX_BYTES > DEFAULT_MAX_BODY_BYTES - assert ("POST", "/api/debug-sessions/{sid}/screenshots") in bodylimit.EXEMPT_ROUTES + assert ( + bodylimit.ROUTE_MAX_BODY_BYTES[("POST", "/api/debug-sessions/{sid}/screenshots")] + == debugsession.SCREENSHOT_MAX_BYTES + ) # --- the real application ----------------------------------------------------------------------- +def test_the_global_refusal_carries_the_baseline_security_headers(client): + """The 413 is a response like any other, so it has to be dressed like one. The middleware sits + INSIDE the security-header and CORS layers for exactly this reason: a refusal a browser drops + for a missing header is indistinguishable from a network failure, and the user would be shown a + generic error instead of the translated `too_large`.""" + r = client.post("/auth/password-login", content=OVER, headers={"content-type": "text/plain"}) + assert r.status_code == 413 + assert r.headers.get("x-content-type-options") == "nosniff" + assert "referrer-policy" in r.headers + + 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 @@ -232,4 +285,7 @@ def test_the_global_refusal_is_byte_identical_to_the_per_route_one(db, as_user, headers={"content-type": "application/json"}, ) assert r.status_code == 413 - assert r.json() == json.loads(bodylimit.TOO_LARGE_BODY) + # RAW BYTES, not parsed JSON: the two used to differ by the two spaces `json.dumps` puts in by + # default and `JSONResponse` does not, and a structural comparison could not see it — which is + # the only kind of drift the docblock's promise was ever at risk from. + assert r.content == bodylimit.TOO_LARGE_BODY