alex/ephyra
Watch
1
0
Fork
You've already forked ephyra
 0
Code Issues 2 Pull requests Projects Releases Packages 1 Wiki Activity Actions

A read by id ignores what the caller is allowed to see #111

New issue
Closed
opened 2026-08-13 22:59:43 +02:00 by Claude · 1 comment
Claude commented 2026-08-13 22:59:43 +02:00
Collaborator
Copy link

Named in MILESTONES.md's risk table and not yet actionable anywhere else, so filing it as work.

Upstream reaches an item through GetItemById<T>(id, userId), which applies IsVisibleStandalone: blocked tags, blocked media folders, and the parental rating. This port has no equivalent at any of its by-id call sites — visibility exists only inside ItemQuery and in which libraries a view offers. A caller who knows an id reads the row whatever their account allows, including its media source and therefore its stream.

Three pieces, in the order they matter:

  1. The parental rating on a standalone read. InheritedParentalRatingValue is populated since M11 and the query filter honours it; extending it to a by-id read is the smallest half and closes the case an operator most expects to work — a child account opening a link to an adult film.
  2. Blocked tags are not modelled at all.
  3. Blocked media folders are not modelled at all.

This is the one item on the outstanding list that is a security property rather than a feature, and it is worth a test per call site class rather than a single guard somebody can forget to call — the M7 review found four routes running under a weaker policy than upstream's for exactly that reason.

Note the separate, deliberate divergence already recorded in the same table: an anonymous GET /Genres/<name>/Images/Primary files a row, which is upstream's own behaviour and is named rather than fixed. Do not conflate the two.

Where to start

MILESTONES.md's risk table has the statement of the problem; this is the work.

Upstream reaches an item through GetItemById<T>(id, userId) and the rule is
BaseItem.IsVisibleStandalone — blocked tags, blocked media folders, and the
parental rating. Read that method and the User policy fields it consults
before touching anything here.

Ours: the filters exist only inside crates/jellyfin-core/src/query.rs and in
which libraries a view offers. The by-id call sites are found with a grep for
the item-by-id helpers in crates/jellyfin-http/src/routes/ and
crates/jellyfin-app/ — there were 96 at the time this was written, so the
shape of the fix matters more than the fix: a guard somebody has to remember
to call will be forgotten, and the M7 review already found four routes running
under a weaker policy than upstream's for exactly that reason. Prefer making the
unchecked read impossible to express — the user is a parameter of the fetch, not
a check after it.

Order: the parental rating first (it is populated since M11 and the query filter
already honours it, so this is the smallest half and closes the case an operator
most expects to work), then blocked tags, then blocked media folders.

How to know it is done

  • A restricted account is refused by id on everything the same account is not
    shown in a listing — and the streaming routes are in that set, not just the
    metadata ones.
  • A scenario replayed as that account diffs clean against the reference. This is
    a case the fixture has to grow a user for.
  • Do not conflate this with the neighbouring risk-table entry: an anonymous
    GET /Genres/<name>/Images/Primary filing a row is upstream's own behaviour,
    reproduced on purpose.

House rules

CLAUDE.md is the short version and these are the parts that bite: tests run
under cargo nextest; upstream's source is the checkout in upstream/ at the
pin (v12.0-rc5, 6d501ba4) and not the clone beside the repository;
generated files are regenerated rather than edited; a comment says what the code
does, not how it got there; and Dn references stay out of code and comments.
ARCHITECTURE.md decides which crate a thing goes in and which dependency edges
are illegal, DECISIONS.md says why a rule exists.

The reference pair is cargo xtask reference up (and reference provision,
which is idempotent); a run is
cargo run -p jellyfin-harness -- replay --upstream URL --ours URL --scenario FILE --out DIR,
with diff-db for the two databases. crates/jellyfin-harness/README.md says
what a diff means and what an ignore entry has to carry — every entry needs a
reason and an expiry milestone, and an adds-only history is the smell the risk
table names.

Named in `MILESTONES.md`'s risk table and not yet actionable anywhere else, so filing it as work. Upstream reaches an item through `GetItemById<T>(id, userId)`, which applies `IsVisibleStandalone`: blocked tags, blocked media folders, and the parental rating. This port has no equivalent at any of its by-id call sites — visibility exists only inside `ItemQuery` and in which libraries a view offers. **A caller who knows an id reads the row whatever their account allows**, including its media source and therefore its stream. Three pieces, in the order they matter: 1. **The parental rating on a standalone read.** `InheritedParentalRatingValue` is populated since M11 and the query filter honours it; extending it to a by-id read is the smallest half and closes the case an operator most expects to work — a child account opening a link to an adult film. 2. **Blocked tags** are not modelled at all. 3. **Blocked media folders** are not modelled at all. This is the one item on the outstanding list that is a security property rather than a feature, and it is worth a test per call site class rather than a single guard somebody can forget to call — the M7 review found four routes running under a weaker policy than upstream's for exactly that reason. Note the separate, deliberate divergence already recorded in the same table: an anonymous `GET /Genres/<name>/Images/Primary` files a row, which is upstream's own behaviour and is named rather than fixed. Do not conflate the two. --- ## Where to start `MILESTONES.md`'s risk table has the statement of the problem; this is the work. Upstream reaches an item through `GetItemById<T>(id, userId)` and the rule is `BaseItem.IsVisibleStandalone` — blocked tags, blocked media folders, and the parental rating. Read that method and the `User` policy fields it consults before touching anything here. Ours: the filters exist only inside `crates/jellyfin-core/src/query.rs` and in which libraries a view offers. The by-id call sites are found with a grep for the item-by-id helpers in `crates/jellyfin-http/src/routes/` and `crates/jellyfin-app/` — there were 96 at the time this was written, so **the shape of the fix matters more than the fix**: a guard somebody has to remember to call will be forgotten, and the M7 review already found four routes running under a weaker policy than upstream's for exactly that reason. Prefer making the unchecked read impossible to express — the user is a parameter of the fetch, not a check after it. Order: the parental rating first (it is populated since M11 and the query filter already honours it, so this is the smallest half and closes the case an operator most expects to work), then blocked tags, then blocked media folders. ## How to know it is done - A restricted account is refused by id on everything the same account is not shown in a listing — and the streaming routes are in that set, not just the metadata ones. - A scenario replayed as that account diffs clean against the reference. This is a case the fixture has to grow a user for. - Do **not** conflate this with the neighbouring risk-table entry: an anonymous `GET /Genres/<name>/Images/Primary` filing a row is upstream's own behaviour, reproduced on purpose. ### House rules `CLAUDE.md` is the short version and these are the parts that bite: tests run under `cargo nextest`; upstream's source is the checkout in `upstream/` **at the pin** (`v12.0-rc5`, `6d501ba4`) and not the clone beside the repository; generated files are regenerated rather than edited; a comment says what the code does, not how it got there; and `Dn` references stay out of code and comments. `ARCHITECTURE.md` decides which crate a thing goes in and which dependency edges are illegal, `DECISIONS.md` says why a rule exists. The reference pair is `cargo xtask reference up` (and `reference provision`, which is idempotent); a run is `cargo run -p jellyfin-harness -- replay --upstream URL --ours URL --scenario FILE --out DIR`, with `diff-db` for the two databases. `crates/jellyfin-harness/README.md` says what a diff means and what an ignore entry has to carry — every entry needs a reason and an expiry milestone, and an adds-only history is the smell the risk table names.
Claude commented 2026-08-14 02:54:34 +02:00
Author
Collaborator
Copy link

Done, on main — twelve commits, d44b0e1 through f7226dd.

The shape

ItemRepository::by_id(id, seen_by: &Viewer). Viewer is either an account — rating limit, cleaned blocked tags, refused libraries — or Viewer::anyone(), the read with no account behind it. Roughly sixty call sites each had to say whose read it is, and anyone() is greppable: 43 production sites, every one checked against where upstream passes no user. Refusal is absence rather than a check bolted on afterwards, so every site got upstream's status and body for free — the same 162-byte problem document, byte for byte.

by_ids takes a Viewer too, after review: ARCHITECTURE.md claimed an unchecked by-id read could not be expressed, and that method was one, with six callers.

Measured, not argued

Three restricted accounts — MaxParentalRating, BlockedTags, EnabledFolders — against an upstream container stood up from the pinned image over a copy of the reference database, because the shared reference has one account and creating another there is a write. 23 by-id routes × 3 accounts agree with upstream on status and body, images included:

ratingkid  /Items/{Alien}                404 = 404      (identical ProblemDetails)
tagkid     /Items/{Night Signal}         404 = 404
folderkid  /Items/{episode}              404 = 404
folderkid  /Items?parentId={shows}       401 = 401      "folderkid is not permitted to access Library shows."
anonymous  /Items/{Alien}/Images/Primary 200 = 200      277,313 bytes both

What the review found, and what it cost

A listing aimed at a playlist or a box set applied none of the three restrictions. The linked-contents branch skipped the narrowing entirely, so a PG-capped account opening a collection an adult had added a restricted film to received that film's whole BaseItemDto where upstream answers 401. The change's own doc says a row reached by its id is a row the account would have been listed; this inverted it. Fixed, and ItemQuery grew blocked_libraries for that one listing — what a set names can sit in any library, where an ordinary listing is already narrowed to what it may select from, which is why /Items?ids= keeps #91's behaviour untouched. The recursive form keeps the folder check off, because that is upstream's own exception.

/Items/Latest returned R-rated titles to a capped account while a doc comment added by this change claimed it narrowed. Now identical to upstream in titles and order for all four accounts.

A box set in a refused library was over-refused — upstream 200, ours 404. Fixed rather than exempted: upstream's IsLegacyBoxSet is a path test, so a box set with no path, a path under %AppDataPath%, or linked children is not measured by the library its row sits in. Both kinds are pinned by a test.

The scenario was almost all refusals — 18 of 26 requests 404-from-both, and one intended positive control vacuous because that fixture row has no image. A regression returning None for every restricted account would have diffed clean across most of it, which is M5's lesson exactly. It is 38 requests now: 19 refusals and 19 answered with a body through the library the account keeps.

Two things deliberately not done

The progressive stream routes are not guarded, and that is correct: GET /Videos/{id}/stream?static=true and /Audio/{id}/stream answer 200 with the file to a caller holding no token at all — verified on the reference with no Authorization header, and it is StreamingHelpers.cs reading the item with no account. A check there would protect nothing while the anonymous door stands open, and it would break the "same status and same body" bar this change is measured by. /Videos/{id}/master.m3u8 answering anonymously is ours rather than upstream's, and is filed as #131.

#91's hole stays open. A listing naming ids still reaches rows outside the caller's libraries on both servers — confirmed both ways, rating and tags do narrow it on both, folders do not on either.

The risk row

Not retired — restored, with Half-corrected at M11. appended in the M8 style. What reaches a by-id read is closed; what is left is #128 (every listing other than /Items and /Items/Latest still ignores the account's limits) and #129 (AllowedTags and BlockUnratedItems are modelled nowhere). An account can still read what it must not see, through those listings, and the record says so.

Cost

/Items/{id} went 20 → 23 queries for an ordinary account and 26 for one refused four libraries, after batching the folders behind refused libraries into one query and reading the account's rows once per viewer instead of four times. Interleaved A/B over 800 requests: +0.07 ms ordinary, +0.23 ms restricted. Every added read is a primary-key lookup or one indexed AncestorIds query — O(1) in library size, against report.md's 22 ms median on 56,337 items. A cache was considered and rejected: it would need invalidating on every policy and library write.

Numbers

JELLYFIN_RS_REQUIRE_FIXTURES=1 cargo nextest run --workspace on main: 2167 passed, 0 skipped. Clippy clean. visibility.json 38/38 agreed, 0 differences. Against a control run of the pre-change binary: refusals 21 → 9 differences, and browsing, items, portable, collections, serving, recommendations, handshake and playback all unchanged.

Browser: folderkid sees only the movies library, and a direct URL to an item in a refused library never renders a detail page. jellyfin is unaffected — all four libraries, artwork, item detail with stream info.

One caveat about running the new scenario. visibility.json needs a restricted account that only cargo xtask reference provision creates, and that provisioning change is in this landing. Until the reference is re-provisioned the scenario fails loudly — exit 1, no output directory, zero requests sent — which is the right failure, and was checked rather than assumed.

Four defects this turned up are filed: #128, #129, #130, #131.

Done, on `main` — twelve commits, `d44b0e1` through `f7226dd`. ## The shape `ItemRepository::by_id(id, seen_by: &Viewer)`. `Viewer` is either an account — rating limit, cleaned blocked tags, refused libraries — or `Viewer::anyone()`, the read with no account behind it. Roughly sixty call sites each had to say whose read it is, and `anyone()` is greppable: 43 production sites, every one checked against where upstream passes no user. Refusal is absence rather than a check bolted on afterwards, so every site got upstream's status **and body** for free — the same 162-byte problem document, byte for byte. `by_ids` takes a `Viewer` too, after review: `ARCHITECTURE.md` claimed an unchecked by-id read could not be expressed, and that method was one, with six callers. ## Measured, not argued Three restricted accounts — `MaxParentalRating`, `BlockedTags`, `EnabledFolders` — against an upstream container stood up from the pinned image over a copy of the reference database, because the shared reference has one account and creating another there is a write. **23 by-id routes × 3 accounts agree with upstream on status and body**, images included: ``` ratingkid /Items/{Alien} 404 = 404 (identical ProblemDetails) tagkid /Items/{Night Signal} 404 = 404 folderkid /Items/{episode} 404 = 404 folderkid /Items?parentId={shows} 401 = 401 "folderkid is not permitted to access Library shows." anonymous /Items/{Alien}/Images/Primary 200 = 200 277,313 bytes both ``` ## What the review found, and what it cost **A listing aimed at a playlist or a box set applied none of the three restrictions.** The linked-contents branch skipped the narrowing entirely, so a PG-capped account opening a collection an adult had added a restricted film to received that film's whole `BaseItemDto` where upstream answers 401. The change's own doc says a row reached by its id is a row the account would have been listed; this inverted it. Fixed, and `ItemQuery` grew `blocked_libraries` for that one listing — what a set *names* can sit in any library, where an ordinary listing is already narrowed to what it may select from, which is why `/Items?ids=` keeps #91's behaviour untouched. The recursive form keeps the folder check off, because that is upstream's own exception. **`/Items/Latest` returned R-rated titles to a capped account** while a doc comment added by this change claimed it narrowed. Now identical to upstream in titles and order for all four accounts. **A box set in a refused library was over-refused** — upstream 200, ours 404. Fixed rather than exempted: upstream's `IsLegacyBoxSet` is a path test, so a box set with no path, a path under `%AppDataPath%`, or linked children is not measured by the library its row sits in. Both kinds are pinned by a test. **The scenario was almost all refusals** — 18 of 26 requests 404-from-both, and one intended positive control vacuous because that fixture row has no image. A regression returning `None` for every restricted account would have diffed clean across most of it, which is M5's lesson exactly. It is 38 requests now: 19 refusals and **19 answered with a body** through the library the account keeps. ## Two things deliberately not done **The progressive stream routes are not guarded**, and that is correct: `GET /Videos/{id}/stream?static=true` and `/Audio/{id}/stream` answer **200 with the file to a caller holding no token at all** — verified on the reference with no `Authorization` header, and it is `StreamingHelpers.cs` reading the item with no account. A check there would protect nothing while the anonymous door stands open, and it would break the "same status and same body" bar this change is measured by. `/Videos/{id}/master.m3u8` answering anonymously is **ours** rather than upstream's, and is filed as #131. **#91's hole stays open.** A listing naming `ids` still reaches rows outside the caller's libraries on both servers — confirmed both ways, rating and tags do narrow it on both, folders do not on either. ## The risk row Not retired — restored, with `Half-corrected at M11.` appended in the M8 style. What reaches a by-id read is closed; what is left is #128 (every listing other than `/Items` and `/Items/Latest` still ignores the account's limits) and #129 (`AllowedTags` and `BlockUnratedItems` are modelled nowhere). **An account can still read what it must not see**, through those listings, and the record says so. ## Cost `/Items/{id}` went 20 → 23 queries for an ordinary account and 26 for one refused four libraries, after batching the folders behind refused libraries into one query and reading the account's rows once per viewer instead of four times. Interleaved A/B over 800 requests: **+0.07 ms** ordinary, **+0.23 ms** restricted. Every added read is a primary-key lookup or one indexed `AncestorIds` query — O(1) in library size, against `report.md`'s 22 ms median on 56,337 items. A cache was considered and rejected: it would need invalidating on every policy and library write. ## Numbers `JELLYFIN_RS_REQUIRE_FIXTURES=1 cargo nextest run --workspace` on `main`: **2167 passed, 0 skipped**. Clippy clean. `visibility.json` 38/38 agreed, 0 differences. Against a control run of the pre-change binary: `refusals` **21 → 9** differences, and `browsing`, `items`, `portable`, `collections`, `serving`, `recommendations`, `handshake` and `playback` all unchanged. Browser: `folderkid` sees only the movies library, and a direct URL to an item in a refused library never renders a detail page. `jellyfin` is unaffected — all four libraries, artwork, item detail with stream info. **One caveat about running the new scenario.** `visibility.json` needs a `restricted` account that only `cargo xtask reference provision` creates, and that provisioning change is in this landing. Until the reference is re-provisioned the scenario **fails loudly** — exit 1, no output directory, zero requests sent — which is the right failure, and was checked rather than assumed. Four defects this turned up are filed: #128, #129, #130, #131.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
No results found.
Labels
Clear labels   
correctness
Diverges from upstream

  
encoding
Transcoding, ffmpeg, and the arg strings

  
http
Route behaviour

  
jobs
Scheduled tasks

  
operations
Installing a server and keeping one running

  
p1
Do this first

  
p2
After the p1s

  
p3
Real, and not urgent

  
performance
Speed or resource use

  
providers
Metadata from somewhere other than the file

  
scanner
The library scan

  
security
Who is allowed to see what

  
testing
Test coverage and harness

No labels
correctness
encoding
http
jobs
operations
p1
p2
p3
performance
providers
scanner
security
testing
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
alex/ephyra#111
No description provided.