From 8a9cf04a41e4279d186dc81748aacbad30bf1e9a Mon Sep 17 00:00:00 2001 From: CoffeeKnyte <67730400+CoffeeKnyte@users.noreply.github.com> Date: Thu, 30 Jul 2026 08:37:22 +0000 Subject: [PATCH] docs(playback): score the serve-path fixes and record the settled decisions The two coverage matrices and the plan's as-built deltas described GAP-10..GAP-15 as open and asserted things the code did not do. Rescored against the code as it now stands, and made the remaining overclaims explicit rather than leaving them to be discovered by the next reviewer. Marked resolved with the reasoning that produced each fix: GAP-10 (ABS Unwrap), GAP-11 (ebook observability), GAP-12 (rolling-deadline cut latch, pulled forward from the revocation batch because it makes GAP-10's fix inert), GAP-13 (BytesServed merged as a max). GAP-14 and GAP-15 stay open with their batch and, for GAP-14, decision A7 attached. Two claims are called out as STILL FALSE wherever they appear, so the blanket phrasing does not creep back: the kill switch does not "keep a stream dead" (an over-cap kill reopens after 5m until A1 lands) and monitoring does not "never trust client progress" (the LastActivityAt fallback survives until A5 lands). Documents a bound the plan never stated: the in-flight watcher polls, so a cut lands within one interval -- 5s in production -- not instantly. Every "cut" claim in these docs now says so. Corrects two pieces of stale guidance that would have misled the next implementer. Follow-up 0b said to reuse guardRevocationCut for the ebook routes: that wrapper keys on a session_id URL param and an ?st= token those routes do not carry, so it compiles and silently guards nothing. Follow-up 0c suggested a sticky flag on RollingDeadlineWriter: unreachable as written, because the rolling writer is constructed inside the serve helpers and wraps the writer the watcher holds, so Unwrap() -- which walks toward the socket -- can never reach it. Records decisions A1-A8 in one table so the follow-up issues inherit them, plus one new finding for the A3 batch: streamRequestIdentity verifies a stream token's signature but never checks its SessionID against the URL's session_id. Expands the AI-use disclosure to the standard docs/ai-contributions.md requires: tool, exact model IDs, involvement classification, and the adversarial findings and resolution from both directions of the cross-model review -- including the five defects found in the AI implementation and the one found in the AI review of it. Also states plainly that pnpm is absent on the host used for this round, so frontend evidence must come from CI, and that the jellycompat test package does not compile on main, so the compat changes here are verified by reading only. Part of #305. --- .../playback-paths-monitoring-kill-matrix.md | 170 ++++++++++-- docs/architecture/stream-abuse-matrix.md | 260 +++++++++++++++++- ...07-04-stream-monitoring-and-kill-switch.md | 35 +++ 3 files changed, 434 insertions(+), 31 deletions(-) diff --git a/docs/architecture/playback-paths-monitoring-kill-matrix.md b/docs/architecture/playback-paths-monitoring-kill-matrix.md index e1a87e4b..ced21978 100644 --- a/docs/architecture/playback-paths-monitoring-kill-matrix.md +++ b/docs/architecture/playback-paths-monitoring-kill-matrix.md @@ -21,6 +21,31 @@ > running series of fix commits. (This doc references sibling commits by role, not > SHA — squash/rebase rewrites hashes, so a SHA citation goes stale the moment the > branch is amended.) +> +> **Update 2026-07-29 — two-model review round (Claude Opus 5 + Codex gpt-5.6-sol).** +> A second adversarial pass over the rebased branch found **six open gaps**, none of +> which the branch's own tests caught: GAP-10 (ABS in-flight cuts no-op because +> `statusRecorder` lacks `Unwrap`), GAP-11 (ebook/comic/PDF serving is neither +> observed nor killable), GAP-12 (the rolling write deadline can erase a revocation +> cut), GAP-13 (`mergeStreams` discards edge `BytesServed`), GAP-14 +> (transfer-registry saturation serves unmonitored), GAP-15 (edge transcode liveness +> is request-observed, not byte-observed). GAP-10 and GAP-11 in particular mean the +> previous blanket claim that enforcement holds "on every serve surface" was **wrong**. +> +> **Update 2026-07-30 — serve-path batch landed.** **GAP-10, GAP-11, GAP-12 and +> GAP-13 are RESOLVED**; each is scored inline below. GAP-12 was pulled forward from +> the revocation batch because it makes the GAP-10 fix inert (see its entry). +> **GAP-14 and GAP-15 remain open** — GAP-14 to the liveness/replica batch under +> decision A7 (fail closed + connection cap), GAP-15 to the tracker-lifecycle batch. +> +> Two claims elsewhere in this doc are therefore **still not true** and are called out +> where they appear: the kill switch does not "keep a stream dead" (an over-cap kill +> still reopens after 5m until the revocation batch lands), and monitoring does not +> "never trust client progress" (the `LastActivityAt` fallback survives until decision +> A5 lands in the liveness batch). Do not restore the blanket phrasing. +> +> Note also that every "cut" in this document means **cut within ~5s**, not +> immediately: the in-flight watcher polls on a 5s interval. ## Dimensions @@ -68,6 +93,8 @@ Legend: ✅ covered · ⚠️ covered with caveat · ❌ gap. | ABS | authenticated file/download | `handleFileStream` → `ServeDirectPlay` | same integrated handler | | ABS | public playback track | `handlePublicTrack` → `ServeDirectPlay` | same integrated handler | | ABS | public RSS feed file | `handlePublicFeedFile` → `ServeFile` | same integrated handler | +| ebook | original file | `serveEbookInline` → `ServeContent` (`ebook_reader.go:626`) | same integrated handler | +| ebook | converted EPUB | `serveConvertedEpub` → `ServeContent` (`ebook_convert_serve.go:160`) | same integrated handler | ### Monitoring (server-observed existence, reported to central) @@ -87,8 +114,53 @@ bytes but withholds/falsifies progress must still be counted. | ABS | authenticated file/download | ✅ separate in-memory transfer record; cap-exempt | same process only | | ABS | public playback track | ✅ native-session transport marker + metered bytes | same | | ABS | public RSS feed file | ✅ separate in-memory transfer record; cap-exempt | same process only | +| ebook | original / converted EPUB | ✅ transfer record + meter (cap-exempt per A4) | same | -- **Existence is server-observed on every cell.** Integrated: a session is +- **GAP-11 — RESOLVED (serve-path batch).** The + `/api/v1/ebooks/{content_id}/files/{file_id}/read` route group applied only + `apimw.RequireProfile`, and both serve paths wrapped in `RollingDeadlineWriter` + alone: no metered writer, no transfer record, no `Refuse`, no `WatchAndCut`. Not a + text-file-sized path — `ebookReaderFormat` accepts `epub, pdf, mobi, azw, azw3, cbz, + cbr, fb2, fbz`, and CBZ/CBR archives and scanned PDFs routinely run 100 MB–1 GB+, so + a revoked user kept pulling the whole comic library at full speed while the admin view + showed nothing. + + Now metered, registered in the transfer registry, refused on entry and cut in flight, + following the ABS file-handler idiom. **Cap-exempt per decision A4** — admission is + untouched and reading consumes no video stream slot. + + ⚠️ **Correction to the earlier follow-up guidance, which said to "reuse + `guardRevocationCut`" here — do not.** That wrapper keys on a `session_id` URL param + this route does not have (it would pass `""`) and takes identity from + `streamRequestIdentity`, which looks for a `?st=` stream token this route never + carries. It compiles and silently guards nothing. The identity has to come from the + profile/auth context instead. +- **GAP-13 — RESOLVED (serve-path batch).** `streammonitor.mergeStreams` picked the + record with the later `LastServedAt` *wholesale* and backfilled only + `UserID`/`ProfileID`/`MediaFileID`, `Route`, `ClientIP`, `ClientName`, `HWAccel` and + `Position` — **`BytesServed` was not merged**, so when the central record was fresher + a stream whose bytes were all poured at an edge reported `0`. Now merged as a **max, + not a sum**: the two records are two observers of one pour, so summing would + double-count. `DedupeSessionInfos` had the identical hole and feeds the admin view; + fixed there too. +- ⚠️ **GAP-14 — registry saturation serves unmonitored (open).** The transfer + registry is capped at `defaultMaxEntries = 10_000` + (`transfers/registry.go:16`). Past that, `Begin` returns `ErrRegistryFull` and all + four call sites (`downloads/service.go:1103`, `jellycompat/streams.go:249`, + `abs/file_handler.go:150`, `abs/rss_feeds_handler.go:260`) log at **Debug** and + serve anyway; byte updates for the unregistered pour are discarded + (`registry.go:122`). Since there is no connection cap anywhere (abuse matrix E28) + and the compat surface is unthrottled (E25/E27), an attacker can hold the registry + at its ceiling and blind download-class monitoring for every other user. The + registry's own once-per-minute `Warn` is the only operator signal. + + **Decision A7 — still open, scheduled for the liveness/replica batch: fail closed, + plus a connection cap.** A per-user/credential concurrent-connection cap makes + saturation unreachable by one actor, *and* download-class pours are refused rather + than served unobserved if it saturates anyway. Until that lands, "no invisible + streams" silently stops being true under load — do not claim otherwise. +- **Existence is server-observed on every cell** *except the ebook rows above*. + Integrated: a session is unreapable while `activeTransportCount > 0`, and every byte-serving path now holds that marker — direct/remux for the whole pour, transcode for each segment serve (the segment markers were added to close a hidden-stream hole where a slow @@ -103,7 +175,19 @@ bytes but withholds/falsifies progress must still be counted. advanced only by server-observed bytes or transport begin/end. Client progress can still advance `LastActivityAt`, preserving reaping semantics, but cannot influence the enforcer's victim ordering. Integrated `BytesServed` and - `LastServedAt` are mapped alongside the edge equivalents. + `LastServedAt` are mapped alongside the edge equivalents. On the central side this + holds exactly: `Session.LastServedAt` is written only by `BeginTransport`, + `EndTransport` and `AddServedBytes` (`playback/session.go:1132,1151,1169`) — no + progress-report path touches it. + ⚠️ **One edge exception (GAP-15, open):** the proxy's transcode path calls + `touchTranscodeSession` immediately after token verification and *before* proxying + to the node (`proxy/server.go:308,317`), so `Touch` advances the edge record's + `LastServedAt` even when the node returns a 404 and no media byte is served. Edge + transcode liveness is therefore **request-observed, not byte-observed**. The blast + radius is bounded: the enforcer groups by user and keeps the freshest sessions + (`streamenforcer/enforcer.go:140`), so a client hammering dead segment URLs can + only change *which of its own* over-cap streams gets trimmed — it cannot exceed the + cap or shield another account. Worth fixing so the claim above is literally true. - Report-to-central: **C** = async via Redis (edge writes `silo:sessions:*`, `streammonitor.RedisSource` reads). **A/B** = in-process SessionManager IS central; read via `streammonitor.FuncSource`. MultiSource unions both, deduped @@ -154,10 +238,12 @@ Captured per live stream (`streammonitor.LiveStream` / `nodesessions.SessionInfo records while a pour is active. Native download quotas run only when a download row is created; neither `ServeDownload` nor `ServeDirect` gates a pour, and compat/ABS routes have no shared download quota. All routes arm the shared - `WatchAndCut`, so a - per-user stream revocation cuts an in-flight download pour, and the same hook - deletes every compat login so reconnects need re-auth. Documented at the - handlers. + `WatchAndCut`, and the same hook deletes every compat login so reconnects need + re-auth. **Arming it was previously not the same as it working:** on ABS routes the + cut could not reach the socket at all (GAP-10) and on the native download routes it + was undone by the rolling write deadline (GAP-12). Both are now fixed, so "a per-user + stream revocation cuts an in-flight download pour" holds on every one of these routes + — within one ~5s watch tick. ### Kill switch (revocation enforced on the serve path) @@ -169,18 +255,70 @@ Captured per live stream (`streammonitor.LiveStream` / `nodesessions.SessionInfo | jellycompat | direct | ✅ `Refuse` + in-flight cut† (GAP-1/3 fixed) | ✅ via proxy; ✅ local fallback guarded | | jellycompat | remux | ✅ `Refuse` + in-flight cut† | ✅ via proxy; ✅ local fallback guarded | | jellycompat | transcode | ✅ `Refuse` per segment (GAP-1 fixed) | ✅ via proxy/node; ✅ local fallback guarded | -| ABS | authenticated file/download | ✅ bearer JWT `iat` owner cutoff + in-flight cut | same | -| ABS | public playback track | ✅ native session id + session `StartedAt` owner cutoff + cut | same | -| ABS | public RSS feed file | ✅ feed `CreatedAt` owner cutoff + in-flight cut | same | +| ABS | authenticated file/download | ✅ bearer JWT `iat` owner cutoff refuses on entry + in-flight cut† (GAP-10 fixed) | same | +| ABS | public playback track | ✅ native session id + session `StartedAt` owner cutoff + cut† (GAP-10 fixed) | same | +| ABS | public RSS feed file | ✅ feed `CreatedAt` owner cutoff refuses on entry + cut† (GAP-10 fixed) | same | +| ebook | original file / converted EPUB | ✅ `Refuse` on entry + in-flight cut† (GAP-11 fixed) | same | +| native | subtitles | ✅ `guardRevocationCut` (Refuse + cut†) | ✅ verifyToken + cutOnRevocation | +| jellycompat | subtitles | ✅ `Refuse` + cut† (delivery only — extraction is buffered) | same | † In-flight cut uses `streamrevoke.Store.WatchAndCut` (SetWriteDeadline, checked on -entry then every 5s). Works at the edge (the proxy's metered writer implements -`Unwrap`) **and** integrated: the native (`statusWriter`, `requestStatusWriter`) -and compat (`loggingResponseWriter`, `compatImageProxyTagResponseWriter`, -`debugResponseWriter`) middleware writers now implement `Unwrap()`, so -`http.NewResponseController` reaches the socket instead of no-oping (see GAP-3). A -live-client smoke test is still worthwhile, but the previously-guaranteed -integrated no-op is fixed; worst case it still degrades to a next-request `Refuse`. +entry then every `Options.WatchInterval` — **5s in production**, so a cut lands within +~5s, not instantly). Works at the edge (the proxy's metered writer implements +`Unwrap`) **and** on the native/compat integrated surfaces: the native +(`statusWriter`, `requestStatusWriter`) and compat (`loggingResponseWriter`, +`compatImageProxyTagResponseWriter`, `debugResponseWriter`) middleware writers +implement `Unwrap()`, so `http.NewResponseController` reaches the socket instead of +no-oping (see GAP-3). chi's own `middleware.Compress` wrapper also implements +`Unwrap()` (chi v5.2.5 `middleware/compress.go:374`), so the globally-mounted +compressor does not break the chain either. ABS's `statusRecorder` now implements it +too (GAP-10), and a **latch** on the request context stops any +`RollingDeadlineWriter` downstream of the cut from re-arming the socket (GAP-12). + +**The audit that produced that list originally missed the ABS surface — see GAP-10.** +Any new middleware that wraps a byte-serving route must implement `Unwrap()`, and the check +belongs in a test that exercises the *mounted* router, not the handler in isolation. + +**GAP-10 — RESOLVED (serve-path batch).** Every ABS route is wrapped by `accessLog` +(`audiobooks/abs/handler.go:334`), whose `statusRecorder` +(`audiobooks/abs/access_log.go`) implemented `Write`, `WriteHeader`, `Hijack` and +`Flush` but **not** `Unwrap()`. The chain `SessionMeteredWriter → statusRecorder` +dead-ended, `http.NewResponseController(...).SetWriteDeadline` returned +`ErrNotSupported`, and `WatchAndCut` discarded that error and silently stopped +watching — a multi-GB ABS pour started before a `RevokeUser` ran to completion. +Fixed by adding `Unwrap()`. `WatchAndCut` now **logs** a failed `SetWriteDeadline` +(once per watcher) instead of discarding it, so the next wrapper of this shape is +loud rather than invisible. + +The old test passed throughout because `audiobooks/abs/revocation_test.go` called the +handlers directly and never saw the middleware. The replacement drives the **mounted** +router over a **real socket**; it and the compile-time `Unwrap` assertion were both +confirmed to fail when `Unwrap()` is removed again, so this cannot regress silently. + +**GAP-12 — RESOLVED (serve-path batch).** Was scoped to Batch 3, but pulled forward +because it makes the GAP-10 fix *inert*: adding `Unwrap()` is what makes +`SetWriteDeadline` start succeeding, which is also what makes +`RollingDeadlineWriter.bump()` start succeeding — and `bump()` pushed the deadline +back out to `now + StallWindow` (180s) once `bumpStep` (15s) had elapsed, plus once +more from its own constructor. Fixing GAP-10 alone would have left the ABS kill switch +broken, just differently. + +The obvious fix does not work and was rejected: the rolling writer is constructed +*inside* `ServeDirectPlay`/`ServeRemux` and **wraps** the writer `WatchAndCut` holds, +so it sits *above* the watcher. `Unwrap()` walks toward the socket, so the watcher can +never reach it by writer introspection. The cut has to travel by a side channel. + +Fixed with `httpstream.CutLatch`, carried on the request context and consulted by +`bump()`. Once latched the writer never extends the deadline again. `bump()` re-checks +the latch *after* setting a future deadline so a concurrent cut cannot be lost to the +check/set race, and `WatchAndCut` keeps re-applying the deadline each tick rather than +returning after the first cut, as belt-and-braces for any topology the latch misses. + +**Known bound, by design:** the watcher polls, so a revoked pour keeps delivering for +up to one watch interval — **5s in production** — before the cut lands. On a fast link +that is a meaningful amount of data. Read every "cut" claim below as "cut within ~5s", +not "cut immediately". `Options.WatchInterval` makes the interval injectable so +real-socket tests do not have to wait it out. ‡ The transcode node's serve-path check is session-only (`refuseIfRevoked` passes `userID = 0`), so a per-*user* kill is enforced by the fronting proxy's `verifyToken` diff --git a/docs/architecture/stream-abuse-matrix.md b/docs/architecture/stream-abuse-matrix.md index 644dbaf1..46db3b04 100644 --- a/docs/architecture/stream-abuse-matrix.md +++ b/docs/architecture/stream-abuse-matrix.md @@ -39,11 +39,18 @@ admission refusal (`ErrTooManyStreams`, `playback/session.go`) is **immediate**. --- -## Three corrections the investigation turned up (read first) +## Corrections the investigations turned up (read first) -The branch docs oversell three things. The stories below are scored against the +The branch docs oversell several things. The stories below are scored against the **code**, not the plan. +> **Round 2 — 2026-07-29, two-model review (Claude Opus 5 + Codex gpt-5.6-sol).** +> Corrections 4–8 below were found by a second adversarial pass over the rebased +> branch and are **open defects, not doc drift**. They matter because they hit the +> two stated priorities directly: #1 "see all bytes, no invisible streams" +> (corrections 5, 7, 8) and #2 "kill any stream" (corrections 4, 6). Affected rows in +> the summary matrix have been downgraded accordingly. + 1. **The async enforcer uses the effective, group-merged limit.** An earlier audit found it reading raw `users.max_streams`, which is zero ("inherit") for standard accounts. That is no longer true: its limit function calls @@ -64,6 +71,75 @@ The branch docs oversell three things. The stories below are scored against the ffmpeg semaphore (`reconstructSem`, sized to `NumCPU`) guards **only** the restart-reconstruct path in `transcodenode/server.go`, **not** fresh starts. +> **Update 2026-07-30 — serve-path batch landed.** Corrections **4, 5, 6 and 7 below +> are now FIXED** (GAP-10, GAP-11, GAP-12, GAP-13); they are kept here with their +> original wording because the summary-matrix rows and the follow-up list still +> reference them, and because the *reason* each was invisible to the test suite is the +> durable lesson. Correction **8 (GAP-14) remains open**, now with decision A7 +> attached. See the follow-up list at the end for what is left. +> +> Two claims this document makes elsewhere are **still false** and must not be +> restored to blanket phrasing: the kill switch does not "keep a stream dead" (an +> over-cap kill still reopens after 5m until the revocation batch lands), and +> monitoring does not "never trust client progress" (the `LastActivityAt` fallback +> survives until decision A5 lands). Also read every "cut" as "cut within ~5s" — the +> in-flight watcher polls on a 5s interval. + +4. **The ABS in-flight kill switch does not work.** *(FIXED — `Unwrap()` added; the + replacement test drives the mounted router over a real socket and was confirmed to + fail without the fix.)* Every ABS route is wrapped by + `accessLog` (`audiobooks/abs/handler.go:334`), whose `statusRecorder` + (`audiobooks/abs/access_log.go:83`) implements `Write`, `WriteHeader` and `Hijack` + but **not** `Unwrap()`. `WatchAndCut` walks + `SessionMeteredWriter → statusRecorder` and dead-ends, so + `SetWriteDeadline` returns `ErrNotSupported` — an error `WatchAndCut` discards + (`streamrevoke/store.go:187`). The watcher then silently stops. A multi-GB + audiobook pour started before a `RevokeUser` runs to completion. + `audiobooks/abs/revocation_test.go` calls handlers directly, bypassing the + middleware, so the test suite reports this as working. (GAP-10) + +5. **Ebook, comic and PDF serving is invisible and un-killable.** *(FIXED — now + metered, registered in the transfer registry, refused on entry and cut in flight; + cap-exempt per decision A4. Note the fix does **not** reuse `guardRevocationCut` as + originally suggested — that wrapper keys on a `session_id` param and `?st=` token + this route does not carry, so it would have silently guarded nothing.)* The + `/api/v1/ebooks/{content_id}/files/{file_id}/read` group (`api/router.go:2417`) + carries only `apimw.RequireProfile`. Both `serveEbookInline` + (`ebook_reader.go:626`) and `serveConvertedEpub` (`ebook_convert_serve.go:160`) + pour whole files with no meter, no transfer record, no `Refuse` and no + `WatchAndCut`. `ebookReaderFormat` (`ebook_reader.go:675`) accepts `cbz`/`cbr` + comic archives and `pdf` — routinely 100 MB–1 GB+ each. (GAP-11) + +6. **The rolling write deadline can erase a revocation cut.** *(FIXED — a `CutLatch` + on the request context stops `bump()` from re-arming a cut socket, and the watcher + keeps re-applying. Pulled forward from the revocation batch because it makes + correction 4's fix inert: adding `Unwrap()` is what makes `SetWriteDeadline` start + working, which is also what makes `bump()` start working.)* `WatchAndCut` sets the + socket deadline to *now* once, then returns. On routes wrapped in + `httpstream.RollingDeadlineWriter` — native managed and direct downloads + (`api/handlers/downloads.go:447`) — `bump()` + (`httpstream/rolling_deadline.go:95`) runs before every write slice and, once the + 15s `bumpStep` has elapsed, pushes the deadline back out to `now + 180s`. Nothing + re-arms the watcher. The cut is therefore reliable against a *stalled* pour and + unreliable against a *fast-draining* one — weakest against the ripping case it + exists to stop. (GAP-12) + +7. **Merged snapshots discard edge byte totals.** *(FIXED — merged as a max, not a sum, + since the two records are two observers of one pour; `DedupeSessionInfos` had the + same hole and was fixed too.)* `streammonitor.mergeStreams` takes + the record with the later `LastServedAt` wholesale and backfills owner, route, + client, HWAccel and position — but **not `BytesServed`**. When the central record + is fresher, a stream whose bytes were all poured at an edge reports `0` bytes to + the operator. (GAP-13) + +8. **Transfer-registry saturation serves unmonitored.** The registry caps at + `defaultMaxEntries = 10_000` (`transfers/registry.go:16`); past that `Begin` + returns `ErrRegistryFull` and all four call sites log at **Debug** and serve + anyway, discarding byte updates (`registry.go:122`). With no connection cap + anywhere (E28) and an unthrottled compat surface (E25/E27), an attacker can pin + the registry at its ceiling and blind download-class monitoring for everyone + else. (GAP-14) + One thing the docs *under*-sell: `main` already ships a real general API rate limiter (`internal/ratelimit/`), enabled by default — but it is mounted only on the authenticated native surface + specific auth endpoints, and the entire @@ -91,7 +167,7 @@ Jellyfin-compat surface is unthrottled (see Category E). | # | Abuse story | Detection | Enforcement | One-line verdict | |---|---|---|---|---| | 9 | Admin **terminates** a specific abusive live session | ✅ | ✅ | Terminate writes revocation first, keyed on sid; 202 even if no local session | -| 10 | Admin **bans a user** (revoke all their streams) | ✅ | ✅ | `RevokeUser` cutoff kills pre-ban streams incl. in-flight | +| 10 | Admin **bans a user** (revoke all their streams) | ✅ | ✅ | Cutoff refuses every *next* request; in-flight cut now lands on every surface within ~5s (GAP-10/GAP-12 fixed) | | 11 | Killed stream **reconnects** with the same token | ✅ | ✅ | Revocation outlives token; every reconnect `Refuse`d | | 12 | Killed stream **survives a server restart** | ✅ | ✅ | Durable Postgres mirror re-warms the kill list; monotonic expiry | | 13 | Kill issued **during an edge Redis outage** | ⚠️ | ⚠️ | Edge fails **open**: new kills don't reach edge until Redis recovers | @@ -108,13 +184,14 @@ Jellyfin-compat surface is unthrottled (see Category E). | # | Abuse story | Detection | Enforcement | One-line verdict | |---|---|---|---|---| -| 17 | Rip whole library via **native download** endpoints | ⚠️ | ⚠️ | Active pours/bytes visible; creation-time gate only; **no volume cap** | +| 17 | Rip whole library via **native download** endpoints | ⚠️ | ⚠️ | Active pours/bytes visible *until the registry saturates* (GAP-14); creation-time gate only; **no volume cap** | | 18 | Rip via **compat `/Items/{id}/Download`** (Infuse) | ⚠️ | ❌ | Active pour visible, but **no quota at all** | -| 18a | Rip via authenticated **ABS file/download** route | ⚠️ | ⚠️ | Active pour visible and user-cuttable; no quota | +| 18a | Rip via authenticated **ABS file/download** route | ✅ | ⚠️ | Active pour visible and now cuttable in flight (GAP-10 fixed); still **no volume quota** | | 18b | Pull an **ABS public playback track** | ✅ | ✅ | Native session id is monitored/metered; session and owner kills land | -| 18c | Pull a **public ABS RSS feed file** | ⚠️ | ⚠️ | Active pour visible; owner cutoff uses feed creation time | +| 18c | Pull a **public ABS RSS feed file** | ✅ | ⚠️ | Active pour visible; owner cutoff uses feed creation time and the in-flight cut now lands (GAP-10 fixed); closing a feed still does not cut its current pour | +| 18d | Rip via the **ebook/comic/PDF reader** route | ✅ | ⚠️ | Now metered, transfer-rowed, refused on entry and cut in flight (GAP-11 fixed); cap-exempt per A4 and still no volume quota | | 19 | Rip via **sequential direct-play GETs** (one at a time) | ⚠️ | ❌ | Cap counts concurrency, not volume; stays at 1 forever | -| 20 | Admin **stops an in-progress rip** mid-transfer | — | ⚠️ | Branch adds `WatchAndCut` on downloads (best-effort); admin must issue the revoke | +| 20 | Admin **stops an in-progress rip** mid-transfer | — | ✅ | `WatchAndCut` now lands on every download-class route within ~5s (GAP-10/GAP-12 fixed); admin must still issue the revoke | ### Category E — API / DB load abuse (non-stream; branch is orthogonal) @@ -168,6 +245,14 @@ This *fails* as an evasion. Existence and liveness are **byte-observed** `LastServedAt`), never derived from client progress. Falsifying `Position` only corrupts a secondary display field and, at worst, changes which of the user's *own* over-cap sessions `selectVictims` trims first. **Detection ✅ / Enforcement ✅.** +*Two round-2 caveats, both bounded:* (a) at the **edge**, `touchTranscodeSession` +fires before the node is proxied (`proxy/server.go:308,317`), so hammering dead +segment URLs advances `LastServedAt` without serving a byte — liveness there is +request-observed, not byte-observed (GAP-15). Because the enforcer groups by user, +this only lets an attacker choose *which of its own* streams gets trimmed. (b) the +merged snapshot could report `0` bytes for an edge-served stream (GAP-13) — **fixed**; +`BytesServed` is now merged as a max in both `mergeStreams` and `DedupeSessionInfos`, +so the byte signal is sound as well as the existence signal. **A6. Rapid session churn between ticks.** No rate limit exists on `/start` or `/transcode/start` (the `ratelimit` @@ -274,6 +359,25 @@ pour and served bytes are visible in the process-local admin transfer registry, but there is no durable history or automated consumer. **Detection ⚠️ / Enforcement ❌.** +**D18d. Rip via the ebook/comic/PDF reader route. — SECOND-WIDEST HOLE.** +`GET /api/v1/ebooks/{content_id}/files/{file_id}/read` (`api/router.go:2420`) is +guarded only by `apimw.RequireProfile`. `serveEbookInline` (`ebook_reader.go:626`) +opens the original file and hands it to `http.ServeContent` wrapped in nothing but a +`RollingDeadlineWriter`; `serveConvertedEpub` (`ebook_convert_serve.go:160`) does the +same for the converted EPUB. There is **no metered writer, no transfer registry +entry, no `Refuse`, and no `WatchAndCut`** — none of the `guardRevocation*` wrappers +used by `/stream/{session_id}` (`api/router.go:2541`) are applied. Full Range support +is inherited from `ServeContent`. The accepted formats include `cbz`, `cbr` and `pdf` +(`ebook_reader.go:675`), so this is a large-file path, not a text path. A user +revoked for ripping previously kept pulling the entire comic/ebook library at full +speed while the admin transfer list showed nothing and `BytesServed` never moved. + +**FIXED (GAP-11):** the route now registers a transfer record, meters its bytes, +refuses a revoked reader on entry and cuts an in-flight pour within ~5s. It stays +**cap-exempt** per decision A4, so reading never consumes a video stream slot, and +there is still no *volume* quota — a patient single-threaded rip remains possible. +**Detection ✅ / Enforcement ⚠️.** + **D19. Rip via sequential direct-play GETs.** Playing items one at a time keeps `activeCountLocked == 1`, always under the cap. Because the cap counts concurrency and never cumulative bytes, sequential @@ -369,11 +473,20 @@ visible, no CPU/process signal) / Enforcement ❌ (no aggregate cap).** ## What the branch genuinely delivers vs. what it does not **Delivers (and it's solid):** -- Authoritative, **byte-observed** stream existence that a client cannot hide by - lying about or withholding progress (A5). +- Authoritative, **byte-observed** stream existence on the *playback* surfaces that a + client cannot hide by lying about or withholding progress (A5) — with the two + bounded caveat GAP-15 noted there, and now **including** ebooks (GAP-11 fixed) and + subtitle pours. GAP-13 is fixed, so merged byte totals are trustworthy too. Still + **not** guaranteed under transfer-registry saturation (GAP-14, open). - A durable, restart-surviving, reconnect-proof **kill switch** for a *specific - session* or a *user*, enforced on every serve surface (native, compat, edge, - transcode node) via one shared `Refuse` + `WatchAndCut` (B9–B12, D20). + session* or a *user*, via one shared `Refuse` + `WatchAndCut` (B9–B12, D20). + `Refuse` — the next-request half — holds everywhere it is mounted. The + `WatchAndCut` half was weaker than previously documented — it no-op'd on all ABS + routes (GAP-10), was racy on rolling-deadline routes (GAP-12) and was absent from + ebooks (GAP-11) — and **all three are now fixed**, so it lands on every mounted + surface. Two honest bounds remain: the cut takes up to one **~5s** watch tick, and it + does **not** "keep a stream dead" — an over-cap kill still reopens after 5m until + decision A1 lands. - Immediate **synchronous admission** refusal of over-cap starts, and an async over-cap reconciler for the multi-node picture (A1, A2, C15) — *when a per-user cap is set*. @@ -390,8 +503,73 @@ visible, no CPU/process signal) / Enforcement ❌ (no aggregate cap).** (E25–E28) — rate limiter has real coverage gaps. - **CPU/transcode exhaustion** (E29) — no aggregate cap. +## Settled design decisions (2026-07-30) + +These were open forks blocking the remaining batches. All are now decided, so follow-up +issues inherit them rather than re-litigating. + +| # | Question | Decision | +|---|---|---| +| **A1** | Over-cap enforcement model | **Long revocation** matching the token's reconstructable lifetime, so an over-cap kill stops reopening every 5m. **Must not ship before the tracker-lifecycle batch** — it removes the self-healing property that currently limits the damage of a wrong count, and every known source of a wrong count is in that batch. | +| **A2** | Revocation state model | **Durable tombstones** — an independent `RevokedAt`/`ExpiresAt` merge *plus* a durable tombstone so an un-ban survives a restart and cannot be re-`Upsert`ed by a stale replica. One Goose migration. | +| **A3** | Credential identity for the user cutoff | **Token `iat` everywhere.** Replace the fresh `time.Now()` that compat and native fallback paths pass at request entry with the token's issued-at, so a pre-cutoff login cannot look post-cutoff and escape the kill. Per-login logout cuts stay out of scope (they need the authorization-generation option). | +| **A4** | What counts as a "stream" | **Observe + make killable, keep cap-exempt** for the ABS bare file route and ebook/comic/PDF reading. Neither consumes a video stream slot. **Implemented.** | +| **A5** | Liveness source of truth | **Separate server-observed liveness entirely.** Client progress becomes UI metadata and never feeds enforcement or reaping; the `LastActivityAt` fallback for `LastServedAt` is removed. This is what makes "never trusts client progress" true — it is **not** true today. | +| **A6** | Multi-replica visibility | **Publish every integrated stream to Redis** so all replica enforcers share one picture. Fixes the incomplete-input root cause; snapshot staleness handled by re-reading at kill time rather than a distributed lock. | +| **A7** | Fail-open vs fail-closed monitoring | **Fail closed + connection cap.** See follow-up 0e. | +| **A8** | Scope | **Split.** The serve-path, capability and docs work lands first; tracker lifecycle → revocation state → liveness/replicas → re-stream heuristic follow in that dependency order. | + +A new finding recorded while implementing A4, for the A3 batch: `streamRequestIdentity` +verifies a stream token's signature but never checks that the token's `SessionID` +matches the `session_id` in the URL, so a valid token for one session can supply +identity on another session's route. It belongs with A3 rather than being fragmented +across two batches. + ## Recommended follow-ups (prioritized by exposure) +**Round-2 defects come first — these are open bugs in shipped behavior, not +enhancements.** In rough fix-cost order: + +0a. ~~**Add `Unwrap()` to `abs.statusRecorder`**~~ — **DONE.** Plus `WatchAndCut` now + logs (once per watcher) rather than discarding a failed `SetWriteDeadline`, and the + replacement test drives the *mounted* router over a real socket. Verified + non-vacuous: it fails if `Unwrap()` is removed again. (GAP-10) +0b. ~~**Wire the ebook routes into the stream kill switch and the transfer registry**~~ + — **DONE**, cap-exempt per decision A4. ⚠️ **The original advice in this line — + "reuse `guardRevocationCut`" — was wrong and was not followed.** That wrapper reads + `chi.URLParam(r, "session_id")`, which the ebook route does not have (it would pass + `""`), and takes identity from `streamRequestIdentity`, which looks for a `?st=` + stream token the route never carries. It compiles and silently guards nothing. The + ebook fix takes identity from the profile/auth context and otherwise follows the ABS + file-handler idiom. (GAP-11) +0c. ~~**Make the revocation cut survive the rolling deadline**~~ — **DONE** via a + `CutLatch` on the request context. Note the sticky-flag-on-`RollingDeadlineWriter` + option suggested here could not be implemented as written: the rolling writer is + constructed *inside* `ServeDirectPlay`/`ServeRemux` and wraps the writer the watcher + holds, so it sits *above* the watcher and `Unwrap()` (which walks toward the socket) + can never reach it. Hence the context side channel. The re-applying watcher was also + kept, as belt-and-braces. (GAP-12) +0d. ~~**Merge `BytesServed` as a max, not wholesale**~~ — **DONE** in both + `mergeStreams` and `DedupeSessionInfos` (GAP-13). **Still open:** advance the edge + transcode record from real bytes rather than request entry (GAP-15) — deferred to + the tracker-lifecycle batch, which also fixes the overlapping-request defect that + makes edge record lifecycle unsafe to touch piecemeal. +0e. **Decide the registry-saturation policy explicitly** (GAP-14) — **DECIDED (A7), + not yet implemented:** fail *closed* for download-class pours once the registry is + full, **and** add a per-user/credential concurrent-connection cap (E28) so + saturation is unreachable by a single actor in the first place. Scheduled for the + liveness/replica batch. Today it still fails open, logging at Debug at the call + sites. +0f. **Bound the kill-list propagation lock.** `RevokeWithWarnings` holds `opMu` + across durable-Postgres and Redis I/O and strips the caller's deadline with + `context.WithoutCancel` (`streamrevoke/store.go:313,330`). A hung Redis or an + exhausted PG pool blocks every subsequent revoke/unrevoke indefinitely. The hot + read path (`IsRevoked`) uses a different lock and stays fast, so playback is + unaffected — but the operator's ability to issue *new* kills is not. Give the + detached propagation its own bounded context. + +Then, as originally scoped: + 1. **Download controls phase 2:** add per-download kill and a standing per-user download block. The existing user revocation is a cutoff, not a ban: it cuts older pours but deliberately allows new ones. @@ -414,7 +592,59 @@ visible, no CPU/process signal) / Enforcement ❌ (no aggregate cap).** ## AI-use disclosure -This assessment was produced with AI assistance (Claude), from a read-only audit of -the `feat/sauron-async-enforcer` branch and current `main`. No code behavior was -changed. Findings cite the code as of the audit; the three corrections above are -where the branch's own plan/coverage docs overstate what the shipped code does. +This assessment was produced with AI assistance, from a read-only audit of the +`feat/sauron-async-enforcer` branch and current `main`. No code behavior was changed. +Findings cite the code as of the audit; the corrections above are where the branch's +own plan/coverage docs overstate what the shipped code does. + +Round 1 (corrections 1–3) was produced with Claude. Round 2 (corrections 4–8, dated +2026-07-29) was a **two-model** pass: Claude Opus 5 and Codex `gpt-5.6-sol` reviewed +the rebased branch independently, and every finding recorded here was re-verified +against the code by hand before being written down. Corrections 4, 6, 7 and the +`opMu` follow-up (0f) originated with Codex; corrections 5 and 8 were found by both +models independently. Two Codex claims were **rejected** on verification and are +deliberately not recorded above: that chi's `middleware.Compress` wrapper breaks the +`Unwrap` chain (chi v5.2.5 implements `Unwrap()` at `middleware/compress.go:374`), +and that client progress reports advance the central `Session.LastServedAt` (only +`BeginTransport`/`EndTransport`/`AddServedBytes` write it — the real mechanism is the +edge `Touch`, recorded as GAP-15). + +Round 3 (2026-07-30) implemented corrections 4–7 as a **cross-model relay**: Claude +Opus 5 (`claude-opus-5[1m]`) planned, reconciled and reviewed; Codex `gpt-5.6-sol` at +medium effort adversarially reviewed the plan, implemented it, and took one remediation +round. Involvement classification: **AI-assisted implementation with human-directed +scope and AI cross-review**; every design fork (A1–A8) was decided by the maintainer, +not by either model. + +Adversarial findings and their resolution, recorded because the disclosure standard +requires it: + +- Codex's plan review found the decisive issue: fixing GAP-10 alone is **inert**, + because the same `Unwrap()` that makes `SetWriteDeadline` work also makes + `RollingDeadlineWriter.bump()` work, and `bump()` erases the cut. GAP-12 was pulled + forward from a later batch as a result. **Accepted.** +- Codex correctly refuted two claims in the Claude plan: that native subtitle URLs carry + a `?st=` stream token (they do not — `subtitleStreamURL` emits only `file_id`), and + that the `transfers` capability belonged on `/admin/sessions/capabilities` (it belongs + to `/admin/node-sessions`, which is separately gated). It also correctly refuted a + claimed "latent nil bug" in the ABS transfer defer — `Registry.End` guards a nil + receiver. **All accepted; the plan was corrected before implementation.** +- Claude's review of the Codex implementation found five defects, all confirmed by + reading or by running the tests: a **failing real-socket ebook test** (Codex's sandbox + could not run `httptest` listeners, so every real-socket test it wrote was + unexecuted); **GAP-12 left unfixed on the proxy edge remux path**; Warn-log spam every + 5s from the now-perpetual watcher; `HandleVideoStream` still using three separate + `time.Now()` values — the exact race Codex itself had raised and fixed only in the + subtitle path; and subtitle metering counting error-response bodies. **All five fixed + in one remediation round and re-verified here.** +- Claude's Batch-5 capability code was then reviewed by Codex, which found that the + vocabulary drift test was not genuinely bidirectional — it sampled rejected strings, + so a kind newly *accepted* by the parser could go unadvertised. **Accepted:** the + parser and the advertisement now read one shared map. + +Verification note: `pnpm` is absent on the host used for this round, so `make build`, +`pnpm run lint` and `pnpm run format:check` could not be run locally and must come from +CI. No frontend files were changed. The `internal/access` and `internal/jellycompat` +test packages **do not compile on `main`** (stale `UserStore` doubles missing +`GetOnboardingState`), so the compat changes in this round are **verified by reading +only** — CI cannot exercise them either until that is fixed separately. diff --git a/docs/superpowers/plans/2026-07-04-stream-monitoring-and-kill-switch.md b/docs/superpowers/plans/2026-07-04-stream-monitoring-and-kill-switch.md index 500545be..ac937ab9 100644 --- a/docs/superpowers/plans/2026-07-04-stream-monitoring-and-kill-switch.md +++ b/docs/superpowers/plans/2026-07-04-stream-monitoring-and-kill-switch.md @@ -208,6 +208,41 @@ coverage matrix remains authoritative. Still deferred, and deliberately so: multi-replica enforcement (VERIFY-3), the transcode-node ghost-record sweep, and a shared quota for the three download-class routes. +### Post-plan hardening round 2 — 2026-07-30 (serve-path batch) + +A two-model adversarial review found six gaps the branch's own tests did not catch +(GAP-10..GAP-15, all recorded in the companion coverage matrices). Four are now closed; +the fixes changed two things this plan asserted: + +- **"Every byte-serving path is observed" was false**, and is now closer to true. + Ebook/comic/PDF reading had no meter, no transfer record, no `Refuse` and no in-flight + cut (GAP-11); it now has all four, cap-exempt per decision A4. Subtitle pours on all + three surfaces were entry-gated only and now carry a transport span, a meter and a + watcher. The no-proxy remote transcode hop now meters its media bodies. It is still not + literally *every* path: transfer-registry saturation (GAP-14) can still blind + download-class monitoring, and edge transcode liveness is still request-observed + (GAP-15). +- **"The in-flight cut works" was false on the ABS surface and racy elsewhere.** ABS's + `statusRecorder` lacked `Unwrap()`, so `SetWriteDeadline` returned `ErrNotSupported` + and the cut was a silent no-op on every ABS route (GAP-10) — and the existing test + passed throughout because it called handlers directly, bypassing the middleware that + broke it. Fixing that alone was **not sufficient**: the same `Unwrap()` makes + `RollingDeadlineWriter.bump()` start working, and `bump()` pushed the cut deadline back + out (GAP-12). Both are fixed, the latter with a request-context `CutLatch`, because the + rolling writer is built *inside* the serve helpers and wraps the writer the watcher + holds — so it cannot be reached by `Unwrap()`, which walks the other way. + +**A bound this plan never stated:** the in-flight watcher polls, so a cut lands within +one interval (**5s in production**), not instantly. Every "cuts the stream" claim in this +plan should be read that way. + +Two of this plan's other claims remain **not yet true** and are tracked to later batches: +an over-cap kill still reopens after 5m (decision A1), and monitoring still consults +client-progress `LastActivityAt` as a liveness fallback, so "never trusts client +progress" awaits decision A5. + ## AI-use disclosure This plan was drafted with AI assistance (Claude), based on a read-only audit of the current codebase. No behavior was changed by writing it. The Status banner and As-built deltas section were added after implementation. + +The 2026-07-30 hardening round was a cross-model relay: Claude Opus 5 (`claude-opus-5[1m]`) planned, reconciled and reviewed; Codex `gpt-5.6-sol` at medium effort adversarially reviewed the plan, implemented it, and took one remediation round. Every design fork was decided by the maintainer. The adversarial findings on both sides and their resolution are recorded in [`stream-abuse-matrix.md`](../../architecture/stream-abuse-matrix.md) under "AI-use disclosure"; commands assume the repository root is the cwd.