f839a461e674fa7feebdbf4a4caf573efca79545
5
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 peter
|
8525666913 |
test: round 4 — an assertion that could not fail, and a comment that did not add up
Two findings, both in the review's own work; nothing in the product code. That is the shift that ends the loop: 22 → 13 → 6 → 2, and the last two are a test and a comment rather than a defect. The "an override that is fine is still honoured" leg of the bounds-fallback test stored `spec.max` — which, for this key, IS the env default, because the spec's max is derived from it. So the assertion was satisfied by the fallback it existed to rule out: mutating `_coerce` to reject EVERY int override left all three legs green. It stores `spec.max - 1` now and asserts the value differs from the default, so the leg can actually fail. And the shrink test's comment claimed `3 - 5 = -2` for an expression that computes `chips.length - visible` = `2 - 3 = -1` — naming neither operand nor the result, in a comment that was rewritten one round earlier precisely for miscounting. |
||
|
peter
|
3f24248e01 |
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.
|
||
|
peter
|
aa51644f5f |
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. |
||
|
peter
|
05121fbc99 |
fix(security): SEC-20's exemptions become raised ceilings, and the 413 is dressed
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. |
||
|
peter
|
51daf3e388 |
feat(security): cap every request body before auth runs (SEC-20)
FastAPI reads a declared body IN FULL before the first dependency, so
`POST /auth/password-login` drained an arbitrary upload from an anonymous
caller and only then answered 401. 70 of the 72 body-declaring routes
behaved the same way: one unauthenticated client could spend the
process's memory and time at will.
A raw ASGI middleware, not `@app.middleware("http")` — BaseHTTPMiddleware
cannot wrap `receive`, so it could only trust `Content-Length`, which a
client may omit (chunked) or understate. This refuses an oversized
declaration outright and counts the bytes of everything else as they
arrive, the same rule `capped_body` already applies per-route. Registered
directly inside the access log: a refusal is still logged with its
request id, but the session, CORS, the setup gate and every auth
dependency never run for a body that is already too large.
Two routes opt out, under one stated rule: an exemption is ONLY for a
route whose own cap is larger than the global one. That is why
`/api/diagnostics/events` is absent — it caps itself at 512 KiB, below
the ceiling, so exempting it could only widen what it accepts. The list
names route templates and a test resolves each against the live route
table, so a rename fails the gate instead of silently dropping an
exemption in prod.
The 413 is byte-identical to `capped_body`'s and the test compares the
two against each other rather than against a literal — two shapes for one
fact is how the `cancelled`/`canceled` bug happened.
|