5f42ff61c832babb35ba451be84e0962acdd38bd
8
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 peter
|
4aaaaa7860 |
chore(comments): drop epic, sprint and review-round ids from the code
The code cited its plan instead of saying why: `E28 review J4`, `(R6 S3b)`, `S1 justified…`,
`deep-review round 5`. That plan lives in the private ops repo, so for every reader of this one the
citation was noise, and a sentence whose subject was the plan step ("round 2 showed…") hid its
lesson behind an id. Each one is gone and the "why" kept: a bare citation is deleted, a sentence
carrying a lesson is reworded around it ("a later attempt showed…").
Measured (`scripts/audit_metrics.py`, product code): 652 lines in 270 files carried an epic code by
the definition the next commit holds (507 / 241 by the old, two-digit-only one) -> 0. Across all
code, tests, scripts and configs: ~1100 epic-code lines plus ~320 sprint/round ids.
Behaviour unchanged, checked mechanically: every changed .py file has the same AST once docstrings
are blanked; css/sh/toml/yml/json files are identical once comments are stripped; the only
non-comment changes in TS/JS are test titles, one description string in a contract table, and five
ESLint messages, whose text is all the gates snapshot diff holds. Not touched: Markdown (docs and
the changelog keep their history), Alembic migrations, TV episode notation (`S2 · E5`), DESIGN.md
ids (`C1`, `B4`, `S3h`), tracker ids (`SEC-16`, `DX-17`).
One stale claim fixed on the way: test_hls_reaper's docstring said the player checkpoints "every
10 s whether or not it is playing", which plex/stream.py calls wrong; it now says what the code does.
|
||
|
peter
|
6d8b7be6d8 |
feat!: remove the shared demo account
The demo account — one shared user that whitelisted emails entered from the login page, reset on a schedule — goes end to end (owner decision, E34): demo.py and the reset job, users.is_demo and demo_whitelist (migration 0098 deletes the demo user, its audit rows, the reset job's override and notices, then drops the column and table; the downgrade restores the empty schema), the /auth/demo entry, the admin whitelist and reset, the Demo tab, the banner, the login page's "Try the demo", and every is_demo branch in routes, userscope, scheduler and the UI. require_human only ever refused the demo account, so its 87 uses become current_user (owner-approved); require_write_scope keeps its own check. The login page's "or" divider now shows only beside the Google button: without the demo entry under it, an instance without Google would have said "or" to nothing. A contract test (removedFeatures) fails on the word anywhere in the backend, frontend, e2e or scripts — seen red on a planted hit. Size ceilings tightened for the modules that shrank; admin.py is back under the limit. |
||
|
peter
|
3c16012524 |
build(backend): close the lint gaps the E32 audit measured
ruff gains RUF100, PLW1510 and flake8-bugbear, every hit fixed rather than ignored. B008 stays on with FastAPI's Depends/Query declared immutable. basedpyright moves to standard mode, pytest to --strict-markers, and a hand run of pytest now requires the database unless SIFTLODE_TEST_REQUIRE_DB=0. PLW1510 found a real bug: the three ffprobe probes in plex/stream.py never looked at the exit status, so a probe that failed was read as the file's truth (an HDR source sent down the HW-safe path, a multi-audio file collapsed to one track, a subtitle offset taken from garbage) and the warning each except branch promises never fired. They now run with check=True. |
||
|
peter
|
efd7b217ca |
fix(uploads): make both caps answerable, and stop holding a connection
The epic-end deep review found three defects in P1 and four tests that did not hold what they claimed. They land together because they share the upload route and its test module; splitting by hunk would have produced commits that do not individually run green. * The edge ceiling and the route's own cap are the SAME number at the default configuration, so the edge always answers first: a declared Content-Length over the cap is refused before a byte is read, and an undeclared body trips the middleware on the same chunk the route would have tripped on. The route's numbered 422 -- and the drain written for it last week -- are reachable only once an admin LOWERS the knob, which is the one shape the browser UAT tested. Measured: headroom does not fix this, because the declared-length check fires first for any body big enough to matter and the drain pushes the count back over the edge for the rest. So the edge's own refusal is made readable instead: `errors.tooLarge` localizes `too_large` the way `upload_too_large` was already localized, and the dialog refuses an over-cap file BEFORE sending it, off the `upload_max_bytes` that GET /api/downloads/usage now reports. Measured in Chrome against the API directly: that 413 arrives as `onload` with a readable body in 43 ms. * Reading the body behind the auth dependency put the whole transfer inside the request's database session -- the auth SELECT autobegins a transaction, so one of the twenty pooled connections sat `idle in transaction`, with nothing to time it out, for as long as the client cared to keep sending. That is the invariant db.py sizes the pool on and plex/playback.py records an incident for. The read transaction now ends before the await. Measured on the dev server: an 1818 ms upload shows no idle-in-transaction sample while the bytes move, only afterwards, when the handler actually writes. * The display name moved into a request header, where nothing bounds its length, while the column stayed varchar(255) -- so a long enough name was a DataError 500 after the whole file had already transferred. One `display_text` helper now serves all three writers (the metadata PATCH already truncated, the upload ingest did not), and the dialog stops at the same number. * The gate invariant asked `route.dependant.body_params`, which holds only the route's OWN callable's parameters: a body declared in a SUB-DEPENDENCY -- the upload route's own idiom -- leaves it empty while FastAPI still reads the body before auth. It asks `route.body_field` now, which is what FastAPI itself consults. The AST registry of body-reading call sites had also never followed `capped_stream_to`, so the one route the whole invariant exists for was the single site the drift guard could not see. Four tests that passed over their own mutation are fixed: the cap boundary (a `>` loosened to `>=` survived the entire suite), the percent-decoding (every filename the suite sends is ASCII, where quote() is the identity function), the display-name header (no test at all), and the staging cleanup (deleting the unlink left the suite green while every refused upload leaked its partial bytes onto the volume). Every test added or changed here was mutation-checked. |
||
|
peter
|
f5018bdcbc |
fix(uploads): read the body after authenticating, not before it
`POST /api/downloads/upload` declared `file: UploadFile = File(...)`, and FastAPI parses and spools a declared body before the first dependency runs -- `require_human` included. An unauthenticated caller could therefore make the process write the whole per-file allowance (500 MiB by default) to temp, once per request. SEC-20's ceiling bounded that number but could not stop the writing, which is why E27 parked it rather than closing it. The route now takes no body parameter. `spool_upload` names `require_human` first and only then streams `request.stream()` to the staging temp file, so a stranger is refused before a byte moves -- the shape the screenshot route has used since E16. Filename and display name travel as percent-encoded headers (latin-1 on the wire, so a Hungarian filename needs the encoding) instead of multipart parts. `capped_stream_to` joins `capped_body` in routes/_common.py: the same counting rule, written to a file descriptor rather than accumulated, because holding 500 MiB would trade a disk problem for a memory one. Its refusal is the caller's, so the upload route keeps answering 422 `upload_too_large` with the admin knob's own number. With no envelope to allow for, the edge is now exactly that knob, and MULTIPART_ENVELOPE_ALLOWANCE is gone. The invariant stops being prose: test_body_limit.py asks FastAPI itself (`route.dependant.body_params`) and fails the gate if any route with a raised ceiling declares a body. A comment could not have prevented the defect it described; that test would have. Both new tests were seen red for the right reason -- before the fix the anonymous probe got its 401 only after the server had read 4236 bytes of a multipart body, and re-declaring the File parameter turns the invariant test red naming the route and the parameter. |
||
|
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.
|