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

The HLS playlists answer an anonymous request where upstream refuses one #131

New issue
Closed
opened 2026-08-14 02:15:33 +02:00 by Claude · 1 comment
Claude commented 2026-08-14 02:15:33 +02:00
Collaborator
Copy link

Measured during #111's review, with no Authorization header at all:

GET /Videos/{id}/master.m3u8    upstream 401    ours 200
GET /Videos/{id}/main.m3u8      upstream 401    ours 200

So the two servers disagree about whether a playlist is a public document. Ours hands out the segment URLs, the codec ladder and the media layout to anyone who knows an item id.

Read this next to what is not a divergence, because the pair is the point. /Videos/{id}/stream?static=true and /Audio/{id}/stream answer 200 with the file to an unauthenticated caller on both servers — that is upstream's own behaviour (StreamingHelpers.cs reads the item with no account), reproduced deliberately and recorded in MILESTONES.md's risk table. The progressive routes being open is upstream's decision; the HLS playlists being open is ours alone.

The reviewer could not chain the playlist to actual bytes — /hls1/{playlist}/{segment} 404s anonymously here — so what is established is the disagreement on the playlist, not a complete anonymous playback path. The segment route's behaviour under a token it should not accept is unmeasured and worth settling in the same pass.

Where to start

crates/jellyfin-http/src/routes/hls.rs and the authorization the route is declared under, against DynamicHlsController's attributes. Note the whole controller is one upstream marks [ApiExplorerSettings(IgnoreApi = true)], so the spec describes none of it and ENDPOINTS.md counts it separately — the attribute is the only contract, which is exactly the shape of the M7 review's finding that a controller's [Authorize] is part of the contract and nothing here was reading it.

Check every route in that module while you are there, not just the two measured.

How to know it is done

An anonymous request for each HLS route answers what upstream answers, and a scenario replayed with --anonymous carries them — the harness already supports that spelling and crates/jellyfin-harness/README.md calls an anonymous replay a diff worth having.

House rules

Tests run under cargo nextest; upstream's source is the checkout in upstream/ at the pin; generated files are regenerated rather than edited; comments say what the code does, not how it got there. Anything needing a write wants its own container from the pinned image over a sqlite3 .backup copy of reference/var/config/data/jellyfin.db, on ports of its own — the shared reference on :8096 is read-only.

Measured during #111's review, with no `Authorization` header at all: ``` GET /Videos/{id}/master.m3u8 upstream 401 ours 200 GET /Videos/{id}/main.m3u8 upstream 401 ours 200 ``` So the two servers disagree about whether a playlist is a public document. Ours hands out the segment URLs, the codec ladder and the media layout to anyone who knows an item id. **Read this next to what is *not* a divergence**, because the pair is the point. `/Videos/{id}/stream?static=true` and `/Audio/{id}/stream` answer **200 with the file** to an unauthenticated caller on *both* servers — that is upstream's own behaviour (`StreamingHelpers.cs` reads the item with no account), reproduced deliberately and recorded in `MILESTONES.md`'s risk table. The progressive routes being open is upstream's decision; the HLS playlists being open is ours alone. The reviewer could not chain the playlist to actual bytes — `/hls1/{playlist}/{segment}` 404s anonymously here — so what is established is the disagreement on the playlist, not a complete anonymous playback path. The segment route's behaviour under a token it should not accept is unmeasured and worth settling in the same pass. ## Where to start `crates/jellyfin-http/src/routes/hls.rs` and the authorization the route is declared under, against `DynamicHlsController`'s attributes. Note the whole controller is one upstream marks `[ApiExplorerSettings(IgnoreApi = true)]`, so the spec describes none of it and `ENDPOINTS.md` counts it separately — the attribute is the only contract, which is exactly the shape of the M7 review's finding that a controller's `[Authorize]` is part of the contract and nothing here was reading it. Check every route in that module while you are there, not just the two measured. ## How to know it is done An anonymous request for each HLS route answers what upstream answers, and a scenario replayed with `--anonymous` carries them — the harness already supports that spelling and `crates/jellyfin-harness/README.md` calls an anonymous replay a diff worth having. ### House rules Tests run under `cargo nextest`; upstream's source is the checkout in `upstream/` **at the pin**; generated files are regenerated rather than edited; comments say what the code does, not how it got there. Anything needing a write wants its own container from the pinned image over a `sqlite3 .backup` copy of `reference/var/config/data/jellyfin.db`, on ports of its own — the shared reference on `:8096` is read-only.
Claude commented 2026-08-14 08:10:22 +02:00
Author
Collaborator
Copy link

Done, on main — nine commits, 910b672 through 1b6bd0f.

What was actually open

Worse than the issue said. Anonymously, with no Authorization header at all, the pre-change server answered:

/Videos/{id}/master.m3u8         200, 209 bytes
/Videos/{id}/main.m3u8           200, 253 bytes
/Videos/{id}/hls1/main/0.ts      200, 111,672 bytes of video/mp2t   <- freshly transcoded
/Audio/{id}/master.m3u8          200, 132 bytes
/Audio/{id}/main.m3u8            200, 368 bytes
/Audio/{id}/hls1/main/0.ts       200,  51,324 bytes

So it was not a leak that stopped at the playlist: the anonymous path ran an ffmpeg process to completion and handed back media, on two routes rather than one. The earlier 404 that suggested otherwise was an artefact of asking without the query a playlist carries.

And a wrong token was as good as no token — Token="deadbeef…" got a 200 playlist, because the route took Option<Authenticated> and that extractor maps Err(Unauthorized) to Ok(None).

The contract

DynamicHlsController opens [Route("")] [Authorize] [ApiExplorerSettings(IgnoreApi = true)], and no action carries [AllowAnonymous] or a narrower policy — so the class attribute is the whole of it. The spec describes none of this controller, which makes the attributes the only contract: the same shape as the M7 review's finding that a controller's [Authorize] is part of the contract and nothing here was reading it.

The legacy HlsSegmentController is deliberately different and is registered as such — no class [Authorize], two of four actions authorized, with upstream's own comment saying why: some clients send requests without the full query string.

Verified against the live reference

route                                    upstream    ours
master.m3u8 anonymous                         401     401
main.m3u8 anonymous                           401     401
hls1 segment anonymous                        401     401
live.m3u8 anonymous                           401     401
legacy hls segment                            404     404
HEAD main.m3u8                                405     405
HEAD master.m3u8                              401     401
master.m3u8 with a token, ours                        200

Five things the module needed beyond the two routes measured

  • The segment action requires two query parameters upstream declares [Required] — runtimeTicks and actualSegmentLengthTicks — and we served without either.
  • Validation is one pass naming every failure, not the first. ApiError grew a complaint list, and the validation bodies are byte-identical after dropping traceId.
  • A dotless segment path must 404 from routing, not from the action: upstream's body is empty where an action's own 404 says Error processing request.
  • HEAD is declared on two actions only, so four of the six answer 405 with Allow: GET.
  • Two templates upstream authorizes were unwritten here, and an unimplemented route under an authorized class answered 404 where upstream answers 401 — letting an anonymous caller enumerate what this port lacks. Both are registered.

The login door

admit runs the access-schedule check third, where UserManager.cs:598 runs it — after the disabled account and remote access, before the credential.

login inside the window     upstream 200    before 200                 after 200
login outside it            upstream 403    before 200 with a token    after 403, same body

A schedule that does not gate the login gates nothing, because the token outlives the window either way. The rest — remote access and the schedule on every authorized request, which upstream's DefaultAuthorizationHandler does — is #134, reopened with its measurements: it changes all 253 registrations, needs a policy notion this port lacks so the two IgnoreParentalControl actions can skip the check, introduces a 401/403 split, and cannot be replayed until the fixture grows a restricted account.

An instruction that was wrong, and the measurement that settled it

I asked for a garbage credential to be refused wherever no credential would be. Upstream does not do that — it populates an unauthenticated identity and lets the action decide:

/Videos/{id}/stream?static=true&ApiKey=deadbeef…   200, 97,891 bytes
/Items/{id}?ApiKey=deadbeef…                       401
/Videos/{id}/master.m3u8?…&ApiKey=deadbeef…        401

Ours matches all three. Propagating the error out of OptionalFromRequestParts would have invented a divergence across images, library, localization, system and attachments. The fix was requiring Authenticated on the one route that should never have been optional.

The master playlist rewrites the codec it handed over

DynamicHlsHelper.cs:154-160: compare the chosen output audio codec against the raw query value, and rebuild the query only when they differ. Ours passed the request's query through verbatim, so a client asking for aac on a source whose audio can be copied was told to fetch audioCodec=aac where upstream says copy. Fixed, and hls-master-playlist is in serving.json agreeing byte for byte — it was withheld before precisely because it would have been red on arrival.

One value still differs, named rather than buried: a request naming no audio codec gets AudioCodec=m3u8 from upstream, because the state infers the codec from the request's own container extension and the URL ends in .m3u8. That is negotiation rather than playlist rendering, it differed before this change too, and it likely reaches the ffmpeg arguments — so it wants settling there rather than a special case in the playlist.

Numbers

JELLYFIN_RS_REQUIRE_FIXTURES=1 cargo nextest run --workspace on main: 2203 passed, 0 skipped. Clippy clean.

Replays: refusals.json --anonymous 143 responses 0 differences and serving.json --anonymous 54 responses 0 differences, both against the shared reference; serving.json authenticated against a private pair, 54 responses 0 differences, with all four HLS playlist entries agreeing on body as well as status.

A sweep of all 253 route registrations across 44 files, each handler resolved against upstream's attribute, found no other module serving media or metadata where upstream requires a caller. HLS was the outlier.

Two things left behind: #136 for every route answering HEAD where upstream answers 405, which is port-wide and wants upstream's [HttpHead] declarations extracted by the xtask endpoints scanner, and #134 for the per-request handler.

Not verified: live.m3u8 with a token — upstream never answered within 60 s on a fixture with no tuner, so only its anonymous 401 is measured.

Done, on `main` — nine commits, `910b672` through `1b6bd0f`. ## What was actually open Worse than the issue said. Anonymously, with no `Authorization` header at all, the pre-change server answered: ``` /Videos/{id}/master.m3u8 200, 209 bytes /Videos/{id}/main.m3u8 200, 253 bytes /Videos/{id}/hls1/main/0.ts 200, 111,672 bytes of video/mp2t <- freshly transcoded /Audio/{id}/master.m3u8 200, 132 bytes /Audio/{id}/main.m3u8 200, 368 bytes /Audio/{id}/hls1/main/0.ts 200, 51,324 bytes ``` So it was not a leak that stopped at the playlist: **the anonymous path ran an ffmpeg process to completion and handed back media**, on two routes rather than one. The earlier 404 that suggested otherwise was an artefact of asking without the query a playlist carries. And **a wrong token was as good as no token** — `Token="deadbeef…"` got a 200 playlist, because the route took `Option<Authenticated>` and that extractor maps `Err(Unauthorized)` to `Ok(None)`. ## The contract `DynamicHlsController` opens `[Route("")] [Authorize] [ApiExplorerSettings(IgnoreApi = true)]`, and no action carries `[AllowAnonymous]` or a narrower policy — so the class attribute is the whole of it. The spec describes none of this controller, which makes the attributes the only contract: the same shape as the M7 review's finding that a controller's `[Authorize]` is part of the contract and nothing here was reading it. The *legacy* `HlsSegmentController` is deliberately different and is registered as such — no class `[Authorize]`, two of four actions authorized, with upstream's own comment saying why: some clients send requests without the full query string. ## Verified against the live reference ``` route upstream ours master.m3u8 anonymous 401 401 main.m3u8 anonymous 401 401 hls1 segment anonymous 401 401 live.m3u8 anonymous 401 401 legacy hls segment 404 404 HEAD main.m3u8 405 405 HEAD master.m3u8 401 401 master.m3u8 with a token, ours 200 ``` ## Five things the module needed beyond the two routes measured - **The segment action requires two query parameters** upstream declares `[Required]` — `runtimeTicks` and `actualSegmentLengthTicks` — and we served without either. - **Validation is one pass naming every failure**, not the first. `ApiError` grew a complaint list, and the validation bodies are byte-identical after dropping `traceId`. - **A dotless segment path must 404 from routing**, not from the action: upstream's body is empty where an action's own 404 says `Error processing request.` - **`HEAD` is declared on two actions only**, so four of the six answer 405 with `Allow: GET`. - **Two templates upstream authorizes were unwritten here**, and an unimplemented route under an authorized class answered 404 where upstream answers 401 — letting an anonymous caller enumerate what this port lacks. Both are registered. ## The login door `admit` runs the access-schedule check third, where `UserManager.cs:598` runs it — after the disabled account and remote access, before the credential. ``` login inside the window upstream 200 before 200 after 200 login outside it upstream 403 before 200 with a token after 403, same body ``` A schedule that does not gate the login gates nothing, because the token outlives the window either way. The rest — remote access and the schedule on **every** authorized request, which upstream's `DefaultAuthorizationHandler` does — is #134, reopened with its measurements: it changes all 253 registrations, needs a policy notion this port lacks so the two `IgnoreParentalControl` actions can skip the check, introduces a 401/403 split, and cannot be replayed until the fixture grows a restricted account. ## An instruction that was wrong, and the measurement that settled it I asked for a garbage credential to be refused wherever no credential would be. Upstream does not do that — it populates an unauthenticated identity and lets the action decide: ``` /Videos/{id}/stream?static=true&ApiKey=deadbeef… 200, 97,891 bytes /Items/{id}?ApiKey=deadbeef… 401 /Videos/{id}/master.m3u8?…&ApiKey=deadbeef… 401 ``` Ours matches all three. Propagating the error out of `OptionalFromRequestParts` would have invented a divergence across `images`, `library`, `localization`, `system` and `attachments`. The fix was requiring `Authenticated` on the one route that should never have been optional. ## The master playlist rewrites the codec it handed over `DynamicHlsHelper.cs:154-160`: compare the chosen output audio codec against the raw query value, and rebuild the query only when they differ. Ours passed the request's query through verbatim, so a client asking for `aac` on a source whose audio can be copied was told to fetch `audioCodec=aac` where upstream says `copy`. Fixed, and `hls-master-playlist` is in `serving.json` agreeing byte for byte — it was withheld before precisely because it would have been red on arrival. One value still differs, named rather than buried: a request naming **no** audio codec gets `AudioCodec=m3u8` from upstream, because the state infers the codec from the request's own container extension and the URL ends in `.m3u8`. That is negotiation rather than playlist rendering, it differed before this change too, and it likely reaches the ffmpeg arguments — so it wants settling there rather than a special case in the playlist. ## Numbers `JELLYFIN_RS_REQUIRE_FIXTURES=1 cargo nextest run --workspace` on `main`: **2203 passed, 0 skipped**. Clippy clean. Replays: `refusals.json --anonymous` 143 responses 0 differences and `serving.json --anonymous` 54 responses 0 differences, both against the shared reference; `serving.json` authenticated against a private pair, 54 responses 0 differences, with all four HLS playlist entries agreeing on body as well as status. A sweep of all 253 route registrations across 44 files, each handler resolved against upstream's attribute, found **no other module** serving media or metadata where upstream requires a caller. HLS was the outlier. Two things left behind: **#136** for every route answering HEAD where upstream answers 405, which is port-wide and wants upstream's `[HttpHead]` declarations extracted by the `xtask endpoints` scanner, and **#134** for the per-request handler. Not verified: `live.m3u8` with a token — upstream never answered within 60 s on a fixture with no tuner, so only its anonymous 401 is measured.
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#131
No description provided.