fix: round 3 — a vacuous ordering test, a clamp that was a silent rewrite, a regex that missed
Round 3 over round 2's fixes. The trend is 22 → 13 → 6, and the character has
shifted: these are mostly defects in the REVIEW's own work, not in the epic.
The ordering test I added to hold the middleware's position could not fail for
either arrangement it names. All four `@app.middleware("http")` layers — the
access log, the two header decorators AND the setup gate that must be inside —
register as the same `BaseHTTPMiddleware`, so asserting on the class passes for
the pre-move stack and for a cap moved back outside CORS. It names the layers by
their dispatch function now, which is the only thing that distinguishes them.
The `sysconfig` bounds check CLAMPED, and clamping is a silent rewrite — the thing
this module refuses to do everywhere else, where an unusable override falls back
to the env default and logs once. On the max side the difference is cosmetic; on
the min side it is a live bug. `download_upload_max_bytes` had `min=0` before E7,
and a stored 0 means "no per-file cap" to the route that reads it. Clamped to 1 it
becomes a ONE-BYTE cap: uploads dead, nothing in the log, the admin page showing
"1". It raises `OutOfRange` now and takes the existing fallback, with its own
event code.
The `capped_body` sweep grepped for an identifier as the limit argument, so a site
written `capped_body(request, 4 * 1024 * 1024)` — or with the parameter not named
`request` — was MISSED, and a miss reads as "no new sites". Parsed with `ast`
instead: every call is seen whatever it looks like, and an unfamiliar argument
shape fails the equality instead of vanishing.
Frontend: the shrink test drove the mock in a way the real hook cannot produce
(it moved `visible` in the same step as the shrink, and the comment miscounted the
tags), so it reproduced the right END STATE by accident rather than the sequence.
`visible` is now left alone across the shrink, which IS the precondition.
Ops: the baseline guard answered "did this run rewrite it?" and therefore went
green on the second run over an already-tightened, uncommitted baseline — the
exact state its own comment says marks the localdev stack stale ever after. It
asks both questions now. And `.dockerignore` joins BAKED_PATHS: it decides what
the COPY lines actually take, so it is an input to the build context exactly as
the copied trees are.
This commit is contained in:
@@ -11,15 +11,16 @@ The pair that matters most is "an oversized body is refused" together with "a no
|
||||
gets through": the first alone would pass just as well if the cap refused *everything*.
|
||||
"""
|
||||
|
||||
import ast
|
||||
import json
|
||||
import pathlib
|
||||
import re
|
||||
|
||||
import pytest
|
||||
from fastapi.testclient import TestClient
|
||||
from starlette.requests import Request
|
||||
|
||||
from app import bodylimit, debugsession, sysconfig
|
||||
from app.config import settings
|
||||
from app.models import SystemConfig
|
||||
from app.bodylimit import DEFAULT_MAX_BODY_BYTES, BodyLimitMiddleware
|
||||
from app.routes import diagnostics as diagnostics_routes
|
||||
@@ -214,18 +215,30 @@ CAPPED_BODY_SITES = {
|
||||
|
||||
|
||||
def test_every_capped_body_site_is_known_and_fits_under_its_ceiling():
|
||||
"""Parsed, not grepped. A regex over the call text has to guess how the arguments are spelled,
|
||||
and the shapes it guesses wrong — a bare `4 * 1024 * 1024`, a parameter not named `request` —
|
||||
are the natural ways the NEXT site gets written, so a miss reads as "no new sites" rather than
|
||||
as a failure. `ast` sees every call whatever it looks like."""
|
||||
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")
|
||||
)
|
||||
}
|
||||
found: set[tuple[str, str]] = set()
|
||||
for py in routes_dir.rglob("*.py"):
|
||||
tree = ast.parse(py.read_text("utf-8"), filename=str(py))
|
||||
for node in ast.walk(tree):
|
||||
if not isinstance(node, ast.Call):
|
||||
continue
|
||||
fn = node.func
|
||||
name = fn.attr if isinstance(fn, ast.Attribute) else getattr(fn, "id", None)
|
||||
if name != "capped_body":
|
||||
continue
|
||||
limit = next(
|
||||
(kw.value for kw in node.keywords if kw.arg == "limit"),
|
||||
node.args[1] if len(node.args) > 1 else None,
|
||||
)
|
||||
found.add((py.name, ast.unparse(limit) if limit is not None else "<none>"))
|
||||
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."
|
||||
f"a capped_body call site appeared, moved or changed shape: {found ^ CAPPED_BODY_SITES}. "
|
||||
"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
|
||||
@@ -248,14 +261,30 @@ def test_the_upload_ceiling_and_the_admin_knob_cannot_drift_apart():
|
||||
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."""
|
||||
def test_a_stored_override_outside_the_bounds_falls_back_on_READ(db):
|
||||
"""`set_value` refuses an out-of-range int, but neither bound has always been what it is now —
|
||||
the upload knob had no `max` before SEC-20 and its `min` was 0 before E7 — so a value that was
|
||||
legal when it was written can be sitting in the database. Served as-is it is exactly the drift
|
||||
the test above forbids: the route would accept bytes the edge refuses.
|
||||
|
||||
The fallback is the env default and a warning, NOT a clamp. A clamp is a silent rewrite, and on
|
||||
the min side it would be a wrong one: a stored 0 meant "no per-file cap" to the route, and
|
||||
clamped to 1 it becomes a one-byte cap that kills uploads with nothing in the log.
|
||||
"""
|
||||
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)))
|
||||
assert spec.max is not None and spec.min is not None
|
||||
default = settings.download_upload_max_bytes
|
||||
|
||||
row = SystemConfig(key="download_upload_max_bytes", value=str(spec.max * 4))
|
||||
db.add(row)
|
||||
db.commit()
|
||||
assert sysconfig.get_int(db, "download_upload_max_bytes") == default
|
||||
|
||||
row.value = "0" # the legacy "no per-file cap", now below min
|
||||
db.commit()
|
||||
assert sysconfig.get_int(db, "download_upload_max_bytes") == default
|
||||
|
||||
row.value = str(spec.max) # still in range — an override that is fine is still honoured
|
||||
db.commit()
|
||||
assert sysconfig.get_int(db, "download_upload_max_bytes") == spec.max
|
||||
|
||||
@@ -265,20 +294,32 @@ def test_the_cap_sits_inside_the_response_decorators_and_outside_the_setup_gate(
|
||||
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
|
||||
The layers are named, not counted by class: all four `@app.middleware("http")` decorators —
|
||||
including the setup gate, which must be INSIDE — register as the same `BaseHTTPMiddleware`, so a
|
||||
class-shaped assertion passes for every arrangement anyone would plausibly get wrong.
|
||||
|
||||
Outside the cap: only 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.
|
||||
request while an instance is unconfigured, which 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]
|
||||
def layer(m) -> str:
|
||||
dispatch = m.kwargs.get("dispatch") if m.kwargs else None
|
||||
return dispatch.__name__ if dispatch is not None else getattr(m.cls, "__name__", str(m.cls))
|
||||
|
||||
names = [layer(m) for m in app.user_middleware] # outer → inner
|
||||
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])
|
||||
assert set(names[:i]) <= {
|
||||
"log_requests",
|
||||
"security_headers",
|
||||
"static_cache_headers",
|
||||
"CORSMiddleware",
|
||||
}, f"something that is not a response decorator sits outside the cap: {names[:i]}"
|
||||
assert "log_requests" in names[:i], "the access log must stay outside, so a 413 is logged"
|
||||
assert "setup_gate" in names[i + 1 :], "the setup gate must not run for an oversized body"
|
||||
assert "SessionMiddleware" in names[i + 1 :]
|
||||
|
||||
|
||||
def test_the_screenshot_ceiling_matches_the_cap_the_route_itself_enforces():
|
||||
|
||||
Reference in New Issue
Block a user