diff --git a/backend/app/sysconfig.py b/backend/app/sysconfig.py index 0b91f355..7d72a817 100644 --- a/backend/app/sysconfig.py +++ b/backend/app/sysconfig.py @@ -143,9 +143,10 @@ SPECS: tuple[ConfigSpec, ...] = ( # `max` is not cosmetic: `bodylimit.ROUTE_MAX_BODY_BYTES` refuses an upload body over roughly # this 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. `_coerce` clamps - # a stored value to this on READ too — this bound did not always exist, so a legal-when-written - # override can be sitting in the database already. + # are pinned together here and `tests/test_body_limit.py` holds them that way. `_coerce` also + # REJECTS a stored value outside these bounds on read (falling back to the env default, with a + # warning) — neither bound has always been what it is now, so a legal-when-written override can + # be sitting in the database already. ConfigSpec( "download_upload_max_bytes", "int", @@ -249,19 +250,28 @@ def _choice(s: ConfigSpec, raw: object) -> str: return option +class OutOfRange(ValueError): + """A stored int override that no longer satisfies its spec's bounds.""" + + def _coerce(s: ConfigSpec, raw: str): if s.type == "int": - # CLAMPED, not merely validated. `set_value` rejects an out-of-range int, but bounds get - # ADDED to a spec (download_upload_max_bytes gained a max when SEC-20 started enforcing the - # same number at the edge) and env defaults get LOWERED, and either leaves a stored value - # that was legal when it was written. Serving it unclamped is how one layer ends up acting - # on a number another layer refuses — with the admin page happily showing a value outside - # the range it renders. Clamping keeps the promise the bounds make rather than restating it. + # Bounds are checked on READ, not only in `set_value`. Bounds get ADDED to a spec + # (download_upload_max_bytes gained a `max` when SEC-20 started enforcing the same number at + # the edge; its `min` went 0 → 1 during E7) and env defaults get LOWERED, and either leaves + # a stored value that was legal when it was written. Serving it is how one layer ends up + # acting on a number another layer refuses, with the admin page showing a value outside the + # range it renders. + # + # It raises rather than CLAMPING, and that distinction cost a round: clamping is a silent + # rewrite, and silently rewriting is exactly what this module refuses to do elsewhere. The + # case that settles it is a stored 0 for download_upload_max_bytes — legal when `min` was 0, + # and meaning "no per-file cap" to the route that reads it. Clamped to `min=1` it becomes a + # ONE-BYTE cap and uploads stop working with nothing in the log. Raising drops it into the + # same fallback the uncoercible path uses: the env default, and one warning. v = int(raw) - if s.min is not None: - v = max(v, s.min) - if s.max is not None: - v = min(v, s.max) + if (s.min is not None and v < s.min) or (s.max is not None and v > s.max): + raise OutOfRange(f"{v} is outside [{s.min}, {s.max}]") return v if s.type == "bool": return raw == "true" @@ -315,14 +325,19 @@ def get(db: Session, key: str): return _coerce(s, raw) except (TypeError, ValueError) as exc: # The admin set this in the UI and it is quietly not in force: the app runs on the env - # default while the settings page goes on showing the value that was saved. + # default while the settings page goes on showing the value that was saved. An OutOfRange is + # the same situation arrived at from the other direction — the value was fine when it was + # written and the spec moved under it — so it gets the same treatment and its own code. + event = ( + "sysconfig.override_out_of_range" + if isinstance(exc, OutOfRange) + else "sysconfig.override_uncoercible" + ) if (s.key, raw) not in _coerce_warned: _coerce_warned.add((s.key, raw)) log.warning( - "a stored setting could not be coerced; the default is used instead", - extra=logs.event( - "sysconfig.override_uncoercible", key=s.key, reason=type(exc).__name__ - ), + "a stored setting is not usable; the default is used instead", + extra=logs.event(event, key=s.key, reason=type(exc).__name__), ) return _default(s) diff --git a/backend/tests/test_body_limit.py b/backend/tests/test_body_limit.py index f46421a0..c11d4187 100644 --- a/backend/tests/test_body_limit.py +++ b/backend/tests/test_body_limit.py @@ -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 "")) 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(): diff --git a/frontend/src/components/TagFilterChips.dom.test.tsx b/frontend/src/components/TagFilterChips.dom.test.tsx index 926c067e..8a12615f 100644 --- a/frontend/src/components/TagFilterChips.dom.test.tsx +++ b/frontend/src/components/TagFilterChips.dom.test.tsx @@ -67,24 +67,27 @@ describe("TagFilterChips — the overflow menu", () => { }); it("does not come back when the chip set SHRINKS under the open menu", () => { - // The nastier half of the same bug. `useChipOverflowFit` holds `visible` in state and re-decides - // in a layout effect, so the render in which the set shrinks still carries the PREVIOUS - // measurement — `chips.length - visible` goes negative there, and a `hidden === 0` guard reading - // an unclamped value never fires. The flag then survives the one window it exists to close. - const four = [...chips, chip("delta")]; - fit.visible = 1; - const { rerender } = render(); + // The nastier half of the same bug, and it needs the hook's real timing to reproduce. + // `useChipOverflowFit` keeps `visible` in state and only rewrites it from a LAYOUT EFFECT, so + // the render in which the set shrinks still carries the measurement taken for the PREVIOUS, + // longer set. `length - visible` is negative in that render, a `hidden === 0` guard reading it + // unclamped never fires, and the flag survives the one window it exists to close. + // + // So `fit.visible` is left ALONE across the shrink — that staleness is the precondition, not a + // convenience of the mock. Five chips of which three fit, then three of them are deleted in the + // tag manager: 3 - 5 = -2. + const five = ["alpha", "beta", "gamma", "delta", "epsilon"].map(chip); + fit.visible = 3; + const { rerender } = render(); fireEvent.click(moreBtn()!); expect(menu()).toBeTruthy(); - // Three tags are deleted in the tag manager. `visible` is still the stale 3. - fit.visible = 3; - rerender(); + rerender(); // `visible` is still the stale 3 expect(menu()).toBeNull(); - // …and now the layout effect lands: two long names, only one fits. + // …and now the layout effect lands: two long names, only one of them fits. fit.visible = 1; - rerender(); + rerender(); expect(menu()).toBeNull(); expect(moreBtn()).toHaveProperty("ariaExpanded", "false"); });