peter/siftlode
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(security): SEC-20's exemptions become raised ceilings, and the 413 is dressed

Browse Source 
The module admitted its two opt-outs under a stated rule — "both enforce their own
limit on the bytes they actually read, which is what makes opting them out safe" —
and that rule is false for one of them. `POST /api/downloads/upload` declares
`file: UploadFile = File(...)`, 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 measures bytes that have already landed. Measured on the
installed fastapi 0.141.1 / starlette 1.6.0 — starlette's max_part_size guards
non-file parts only, so an anonymous chunked upload was bounded by nothing. In the
shipped self-host compose that lands in a RAM-backed tmpfs.

The exposure predates SEC-20 (no route was capped before it); what this branch
added was the claim. So the list becomes a mapping of per-route CEILINGS instead
of a set of exemptions: every request now gets a finite number, the upload route's
being the same one the route itself enforces. `sysconfig`'s knob is bounded by it
so an admin cannot raise the handler's cap past the edge and start collecting 413s
from a layer the admin page never mentions — held by a test, not by prose.

Two more things the review caught in the same module:

The 413 was not byte-identical to `capped_body`'s after all. `json.dumps` puts
spaces where JSONResponse does not, and the test that promised to hold them
together compared PARSED JSON, so it could never have seen it. Built through the
same JSONResponse now, and compared as raw bytes.

And the middleware sat outside CORS and the security headers, so its 413 carried
neither — on the split-origin dev setup the browser drops it and the user gets a
generic network failure instead of the translated too_large, while the identical
refusal from capped_body one layer down renders fine. It moves to just inside
CORS: still ahead of the session, the setup gate and every auth dependency.

Also corrected: the docblock's "largest non-exempt body is 64 KiB, ~16x headroom"
(it is 512 KiB and 2x — the diagnostics sink, named eleven lines further down),
and CLAUDE.md's Request SIZE row, which forbade the one correct use of
capped_body that is actually in the tree.
This commit is contained in:
peter
2026-09-21 23:32:49 +02:00
parent bf518223e5
commit 05121fbc99
5 changed files with 172 additions and 76 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/tests/test_body_limit.py
+79 -23
View File
@@ -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