A second review round over the first round's fixes, which is where the remaining real bugs usually are. All four are the same failure as the round it reviewed: a claim stated in a comment that the code does not hold. The middleware moved inside CORS, but the setup gate is registered LATER and is therefore OUTSIDE it — so the comment's "the setup gate never runs for a body that is already too large" was false the moment it was written. On an unconfigured instance that gate opens a DB session per request, which is exactly the anonymous pre-auth work SEC-20 exists to remove. The cap now sits between them, and the order is asserted against `app.user_middleware` rather than described: prose about a stack that `add_middleware` builds inside-out is how this got backwards. The edge ceiling and the route's own cap were pinned EQUAL, but they measure different things — the edge counts raw body bytes, the route counts the decoded file. A file of exactly the advertised maximum was refused one layer before the route that would have taken it. The edge now allows for the multipart envelope, and the test asserts the relation and the gap instead of equality. `sysconfig`'s new `max` was enforced on write only. That bound did not exist before this epic, so a value that was legal when it was written can be sitting in the database — served unclamped it produces exactly the drift the comment says is impossible, with the admin page showing a number outside its own range. Int overrides are clamped on read now, which also survives an env default being lowered later. And `DOWNLOAD_UPLOAD_MAX_BYTES=0` meant "no per-file cap" to the route but a hard zero to the edge: every upload refused, including the admin page's own way back, recoverable only by editing env and restarting. The setting takes `ge=1`. Two test gaps from the same round: the header assertion that is the ONLY check of the middleware's position was DB-gated and skipped on a host without Postgres, and the headroom "ratio" the docblock calls load-bearing was held as `<=`, i.e. no ratio at all. Both fixed, plus a sweep that every `capped_body` call site in the routers is known and fits under its ceiling — the point of that list is the third entry, not the two that exist today.
216 lines
11 KiB
Python
216 lines
11 KiB
Python
"""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 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
|
|
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 re
|
|
|
|
from starlette.responses import JSONResponse
|
|
from starlette.types import ASGIApp, Message, Receive, Scope, Send
|
|
|
|
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 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.
|
|
#
|
|
# 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.
|
|
# 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,
|
|
("POST", "/api/debug-sessions/{sid}/screenshots"): debugsession.SCREENSHOT_MAX_BYTES,
|
|
}
|
|
|
|
# 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"\{[^/{}]+\}")
|
|
|
|
|
|
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 its route's ceiling, before the application sees it.
|
|
|
|
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__(
|
|
self,
|
|
app: ASGIApp,
|
|
*,
|
|
limit: int = DEFAULT_MAX_BODY_BYTES,
|
|
routes: dict[tuple[str, str], int] | None = None,
|
|
) -> None:
|
|
self.app = app
|
|
self.limit = limit
|
|
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 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":
|
|
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 > 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 > 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)
|