fix(security): round 2 — the SEC-20 fix had four defects of its own
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.
This commit is contained in:
@@ -12,12 +12,15 @@ gets through": the first alone would pass just as well if the cap refused *every
|
||||
"""
|
||||
|
||||
import json
|
||||
import pathlib
|
||||
import re
|
||||
|
||||
import pytest
|
||||
from fastapi.testclient import TestClient
|
||||
from starlette.requests import Request
|
||||
|
||||
from app import bodylimit, sysconfig
|
||||
from app import bodylimit, debugsession, sysconfig
|
||||
from app.models import SystemConfig
|
||||
from app.bodylimit import DEFAULT_MAX_BODY_BYTES, BodyLimitMiddleware
|
||||
from app.routes import diagnostics as diagnostics_routes
|
||||
|
||||
@@ -186,27 +189,99 @@ def test_every_listed_template_still_resolves_to_a_real_route(method, template):
|
||||
|
||||
|
||||
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 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
|
||||
the global ceiling never fires on it and raising it could only widen what it accepts.
|
||||
|
||||
`<=` alone would not hold the claim. The docblock calls the 2x margin load-bearing precisely
|
||||
because a ceiling that refuses a legitimate request is a worse bug than the one this module
|
||||
fixes, and a margin of 1x is no margin at all.
|
||||
"""
|
||||
assert diagnostics_routes.BODY_MAX_BYTES * 2 <= DEFAULT_MAX_BODY_BYTES
|
||||
assert not any(
|
||||
path.startswith("/api/diagnostics") for _, path in bodylimit.ROUTE_MAX_BODY_BYTES
|
||||
)
|
||||
|
||||
|
||||
# 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.
|
||||
CAPPED_BODY_SITES = {
|
||||
("diagnostics.py", "BODY_MAX_BYTES"),
|
||||
("debug_sessions.py", "debugsession.SCREENSHOT_MAX_BYTES"),
|
||||
}
|
||||
|
||||
|
||||
def test_every_capped_body_site_is_known_and_fits_under_its_ceiling():
|
||||
routes_dir = pathlib.Path(__file__).resolve().parents[1] / "app" / "routes"
|
||||
found = {
|
||||
(py.name, m.group(1))
|
||||
for py in routes_dir.rglob("*.py")
|
||||
for m in re.finditer(
|
||||
r"capped_body\(\s*request\s*,\s*([A-Za-z_][\w.]*)", py.read_text("utf-8")
|
||||
)
|
||||
}
|
||||
assert found == CAPPED_BODY_SITES, (
|
||||
"a capped_body call site appeared or moved. 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.
|
||||
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
|
||||
|
||||
|
||||
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."""
|
||||
"""The upload route's in-handler cap is a `sysconfig` value an admin can change; the edge is a
|
||||
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.
|
||||
"""
|
||||
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")]
|
||||
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
|
||||
|
||||
|
||||
def test_a_stored_override_above_the_max_is_clamped_on_READ(db):
|
||||
"""`set_value` refuses an out-of-range int, but this bound did not always exist — before SEC-20
|
||||
the upload knob had no `max` at all — so a value that was legal when it was written can be
|
||||
sitting in the database. Served unclamped it is exactly the drift the test above forbids: the
|
||||
route would accept bytes the edge refuses."""
|
||||
spec = next(s for s in sysconfig.SPECS if s.key == "download_upload_max_bytes")
|
||||
assert spec.max is not None
|
||||
db.add(SystemConfig(key="download_upload_max_bytes", value=str(spec.max * 4)))
|
||||
db.commit()
|
||||
assert sysconfig.get_int(db, "download_upload_max_bytes") == spec.max
|
||||
|
||||
|
||||
def test_the_cap_sits_inside_the_response_decorators_and_outside_the_setup_gate():
|
||||
"""The registration order, held in code rather than in the comment beside it. `add_middleware`
|
||||
inverts the reading order (last registered = OUTERMOST), which is exactly the kind of thing a
|
||||
prose description gets backwards — it already did once.
|
||||
|
||||
Outside the cap: the response decorators, so the 413 is dressed like any other answer. Inside
|
||||
it: everything that does work on the caller's behalf — the setup gate opens a DB session per
|
||||
request while an instance is unconfigured, and that is the anonymous pre-auth work SEC-20 exists
|
||||
to remove.
|
||||
"""
|
||||
from app.main import app
|
||||
|
||||
names = [getattr(m.cls, "__name__", str(m.cls)) for m in app.user_middleware]
|
||||
i = names.index("BodyLimitMiddleware")
|
||||
# Outer→inner, so a smaller index is further out.
|
||||
assert i > 0, "the cap is the outermost middleware — the access log must stay outside it"
|
||||
assert names[i + 1 :] and "SessionMiddleware" in names[i + 1 :]
|
||||
# The setup gate and every auth dependency are inside; only decorators are outside.
|
||||
assert all(n == "BaseHTTPMiddleware" or n == "CORSMiddleware" for n in names[:i])
|
||||
|
||||
|
||||
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 (
|
||||
bodylimit.ROUTE_MAX_BODY_BYTES[("POST", "/api/debug-sessions/{sid}/screenshots")]
|
||||
@@ -217,12 +292,22 @@ def test_the_screenshot_ceiling_matches_the_cap_the_route_itself_enforces():
|
||||
# --- the real application -----------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_the_global_refusal_carries_the_baseline_security_headers(client):
|
||||
def test_the_global_refusal_carries_the_baseline_security_headers():
|
||||
"""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"})
|
||||
generic error instead of the translated `too_large`.
|
||||
|
||||
A bare TestClient on purpose, not the `client` fixture: this is the only test that exercises the
|
||||
middleware ORDER, and that fixture is DB-gated — so on a host without Postgres the one assertion
|
||||
holding the move would have skipped while the file still reported green. The refusal never
|
||||
reaches a database; it is answered above the router.
|
||||
"""
|
||||
from app.main import app
|
||||
|
||||
r = TestClient(app).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
|
||||
|
||||
Reference in New Issue
Block a user