13b7577d9f01c97c05ac22a5a8939ce2b57557c4
14
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 peter
|
85aa917b38 |
feat(files): the Files page — the library in one aligned list, its folders in a rail
Files is the one library: every done file, the ones shared with me, or one folder's, picked in a side rail (sources, own folders, the storage meter) and mirrored to ?folder=. One table with every kind of value in its own column; Copy link, Share and Save in sight, every action on a right click: play, cut, rename in the row, move to an own folder or back home, keep, details, re-download, delete. The Downloads page keeps only the queue until the tray replaces it. The old /api/downloads library routes are gone: list shared, upload, the details PATCH, delete, the file and the usage now live under /api/files, and a listed file says which source it came from. The job's "expired" flag is gone with the state it described. |
||
|
peter
|
1a1665b8ec |
feat(files): the Files library API — folders, moving and keeping files, upload into a folder, the file's hash
/api/files is the library's home: list a user's done files (by folder, searched, sorted, or the ones shared with them), rename / move / keep / delete a file, make, rename and delete own folders (a deleted folder's files go back to their source folders), and set how long any folder keeps a file — done files shift by the difference, never closer than the GC's warning window. An upload can name its folder (an own folder or Uploads), checked before the body is read and again before the file lands; it is hashed while it streams, and any file can be hashed on first ask. The download says the hash (Repr-Digest) once it is known; a new file at an asset's path forgets it. Uploads, deletes, keeps, moves and folder changes are audited (ids only). The fileless "expired" rows of the old retention are deleted by migration 0103: an expired file now leaves the library. The shared route helpers moved to routes/files.py; the old /api/downloads library routes delegate to the new ones until the client moves over. |
||
|
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
|
321776480e |
fix(uploads): make the drift guards fail for the reasons they claim
Deep-review round 3, targeted at the surfaces round 2's fix commit changed. Five of the eight findings are the same shape: the guards written to stop half-done sweeps were themselves half-done. test_upload.py's display-field guard walked only `ast.Assign`, so the one writer with an unbounded REQUEST HEADER behind it — `spool_upload`'s `display_name=display_text(...)` keyword — was invisible to the guard meant to protect it. It also stopped at `app/routes/`, missing the four writers in `downloads/` that actually construct the row, and asserted `not offenders`, which enforces nothing once a refactor moves the writers out of its view. It now walks assignments AND keywords across `app/`, and holds exact set equality against a registry of all twelve write sites with each one's value. test_body_limit.py's release guard used `any(... ast.walk(fn))`, which accepts a `release_read_connection` that runs AFTER the await it exists to precede — satisfying the letter of the rule while pinning a connection for exactly the transfer it was supposed to free. Position is checked now, and a late release is its own, louder failure. The registry keyed sites by file basename while scanning recursively, and `admin.py`, `discover.py` and `playlists.py` each exist both at the top level and under `plex/` — the same entry-collapse the registry was widened to close. Keyed by relative path now. And all three guards recognised a body read only by the `capped_` naming convention, so a route spelling `await request.body()` inline declared no ceiling, released no connection, and was invisible to every one of them. `release_read_connection`'s precondition claimed the caller "must have NOTHING PENDING, and that is checked rather than described" while checking only UNFLUSHED ORM work. Measured: after `add`+`flush` all three collections are empty and the rollback loses the row, and Core DML never enters them at all — both of them this codebase's canonical write shapes. It now also asks the transaction directly via `pg_current_xact_id_if_assigned()`. `errors.tooLarge` was reworded to "try a smaller file", but that key answers every route's body cap: the issue composer's over-size JSON draft got advice about a file it does not have. Back to payload-neutral; the file-specific sentence keeps its own key. The upload dialog's new danger border outranked `dragOver`, so a user replacing an over-cap file got no sign the drop zone was live — and with a file picked the label shows its name rather than "drop it here", so the border was the only feedback left. Four further findings were refuted 3/3 and are not fixed: `aria-invalid` on the drop zone (measured in Chromium — the AX node does carry it on a native button), a bidi-override spoof through `display_text` (the served filename already strips those categories, and the proposed filter would have broken every ZWJ emoji), and `capped_body` not draining (the screenshot route's edge ceiling equals its own cap, so that refusal is unreachable, and the log sink is sent by beacon with nobody reading the answer). |
||
|
peter
|
7c0647f87a |
fix(uploads): finish the sweeps round 1 left half-done
Deep-review round 2, on round 1's own fix commit. It found MORE than round 1 did (28 raw, against 13), and most of it was mine -- a new piece of UI attracts findings a backend-only diff never does. The pattern in almost all of them is the same: a principle applied to the site that prompted it and not to its siblings. * `display_text` reached three of the FIVE writers of a user-supplied name. `POST /api/downloads` and `POST /api/downloads/edit` still handed Postgres an unbounded string, so a 300-character name pasted into the Add-download dialog or the editor was a bare 500 (main.py re-raises every DataError but the NUL case). Four of the seven reviewers found it independently. The bound now lives in `models.py`, the three columns are DECLARED with it, and the guard for the next writer is parsed rather than remembered: every `display_name` / `display_uploader` assignment under `app/routes/` must go through the helper. That guard immediately found two more -- the Google sign-in callback writes the profile name into the same 255-wide column -- so it is six, not five. * The connection release reached one of the THREE routes that read their own body. The screenshot route (10 MiB, from a phone) and the client log sink both still held a pooled connection idle-in-transaction for the length of the upload, which is the invariant db.py sizes the pool on. All three now call `db.release_read_connection`, whose precondition -- nothing pending on the shared session -- is CHECKED rather than described, because a whole-session rollback discarding a caller's staged write is a mistake this repo has already made once. `test_body_limit.py` holds the rule for the next such route. * The AST drift registry keyed its sites by (module, limit expression), so a second route copied from the first collapsed onto the same entry and went uncovered -- the exact hole the registry exists to close. Keyed per call site now, and it carries the release requirement too. * The dialog's own refusal was announced once and never again: two over-cap picks leave an identical node with identical text, and an untouched live region says nothing. It is keyed per pick now, the drop zone reads as invalid and points at the sentence, and the reason is reachable from the control rather than only from a one-shot announcement. * Two tests rebuilt the blind spot they were written to remove: the dialog's cap test sat nowhere near the boundary (a `>` loosened to `>=` survived it, which is the mutation round 1 fixed on the server side), and the header-precedence test only covered an exact-case duplicate while a differently-cased one would have been COMBINED onto the wire rather than replaced. Both fixed, and the merge now dedupes case-insensitively because header names are. Also: the upload dialog's unstubbed usage query was firing a real request from jsdom in a neighbouring suite and leaking a delayed "connection lost" toast across test boundaries; `GET /api/downloads/usage` had no test at all though the whole client-side check hangs off it; and the comment claiming the edge 413 is readable now says what was measured -- readable to 200 MiB over loopback, a bare `onerror` through a proxy -- so the pre-send check is named as the mechanism and that branch as the backstop it is. Every test added or changed here was mutation-checked. One mutation SURVIVED and is recorded rather than papered over: the `tooBig` term inside `submit()` cannot be reached, because `ui/Button` swallows the activation itself, so it is marked as belt and braces instead of being described as the guard. |
||
|
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
|
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.
|