alex/ephyra

Watch

Fork 0

Authorization runs after the body is bound, so a caller who may not reach a route is told what was wrong with their body #142

New issue

Closed

opened 2026-08-16 06:47:34 +02:00 by Claude · 1 comment

Claude commented

2026-08-16 06:47:34 +02:00

Collaborator

POST /Backup/Create as an authenticated non-administrator, measured on the pair:

body	upstream	ours
`Content-Type: text/plain`, `x`	403	415
`Content-Type: application/json`, `{`	403	400
`Content-Type: application/json`, `{}`	403	403

Upstream declares [Authorize(Policy = Policies.RequiresElevation)] on the controller, and ASP.NET runs an authorization filter before model binding — so a caller who may not reach the action is refused whatever they send, and the body is never looked at.

Ours checks authorization in the handler body. axum runs FromRequestParts extractors first and the single FromRequest (the body) last, so Json<T> has already rejected the request before elevated(&context, &caller) is reached. The anonymous case is fine — Authenticated is a FromRequestParts and still answers 401 first — so this is only ever visible to a caller who is authenticated and is not permitted.

It is not the backup routes

Found while reviewing the backup work; the shape is the pattern the whole crate is written in. Twenty handlers bind a body and then check authorization inside the handler:

admin/backup.rs::create                     library/images/splashscreen.rs::set_splashscreen
admin/backup.rs::restore                    library/images/writing.rs::store
admin/configuration.rs::update              library/item_update.rs::update
admin/configuration.rs::update_section      library/structure/editing.rs::add
admin/configuration.rs::update_branding     library/structure/editing.rs::add_path
admin/environment.rs::validate_path         library/structure/editing.rs::update_path
admin/startup.rs::configure                 library/structure/editing.rs::options
admin/startup.rs::remote_access             session/devices.rs::rename
admin/startup.rs::name_the_first_user       session/users.rs::create
admin/tasks.rs::set_triggers                session/users.rs::set_policy

Some are worse than a status code. library/images/writing.rs::store and set_splashscreen read an uploaded picture into memory before asking whether the caller may upload one, and admin/configuration.rs::update deserializes a whole configuration document first. An account with a token but no permission can make the server do that work, repeatedly.

Where to start

The fix is not per-route. Two shapes worth weighing, and the choice wants an argument in DECISIONS.md because it changes how every write route is written:

An extractor that carries the requirement, so Elevated is a FromRequestParts beside Authenticated and runs before any body — the elevated(&context, &caller) call disappears from the handler. Closest to upstream's [Authorize(Policy = …)], and the type says what the route needs.
Middleware keyed on the route, refusing before the handler is entered at all. Fewer signature changes, but the requirement stops being visible where the route is written, and the permission per route then lives somewhere other than the route.

Either way AppContext has to be reachable from the extractor or the layer, which it is — it is already the router's state.

crates/jellyfin-http/src/auth.rs is where elevated lives now, and crates/jellyfin-http/tests/common/mod.rs builds the router the route tests drive.

How to know it is done

An authenticated non-administrator posting an unreadable body to a route they may not reach is answered 403, and the body is never read. refusals.json is where it belongs — it already replays anonymously and as the administrator, and the account it needs is restricted, which visibility.json already logs in as.

Worth measuring the same three bodies against every one of the twenty, not just the one this was found on: the statuses ours gives now are the binder's and vary with the extractor.

House rules

Tests run under cargo nextest; upstream's source is the checkout in upstream/ at the pin; generated artifacts are regenerated, never edited; comments say what the code does, not how it got there. The shared reference on :8096 is read-only — logging in as restricted is a write, so anything beyond a refusal wants its own container from the pinned image over a cp -a copy of a snapshot, on its own ports. Check ss -lntp for a free port before binding.

`POST /Backup/Create` as an **authenticated non-administrator**, measured on the pair: | body | upstream | ours | |---|---|---| | `Content-Type: text/plain`, `x` | **403** | **415** | | `Content-Type: application/json`, `{` | **403** | **400** | | `Content-Type: application/json`, `{}` | 403 | 403 | Upstream declares `[Authorize(Policy = Policies.RequiresElevation)]` on the controller, and ASP.NET runs an authorization filter **before** model binding — so a caller who may not reach the action is refused whatever they send, and the body is never looked at. Ours checks authorization in the handler body. `axum` runs `FromRequestParts` extractors first and the single `FromRequest` (the body) last, so `Json<T>` has already rejected the request before `elevated(&context, &caller)` is reached. The anonymous case is fine — `Authenticated` is a `FromRequestParts` and still answers `401` first — so this is only ever visible to a caller who **is** authenticated and **is not** permitted. ## It is not the backup routes Found while reviewing the backup work; the shape is the pattern the whole crate is written in. Twenty handlers bind a body and then check authorization inside the handler: ``` admin/backup.rs::create library/images/splashscreen.rs::set_splashscreen admin/backup.rs::restore library/images/writing.rs::store admin/configuration.rs::update library/item_update.rs::update admin/configuration.rs::update_section library/structure/editing.rs::add admin/configuration.rs::update_branding library/structure/editing.rs::add_path admin/environment.rs::validate_path library/structure/editing.rs::update_path admin/startup.rs::configure library/structure/editing.rs::options admin/startup.rs::remote_access session/devices.rs::rename admin/startup.rs::name_the_first_user session/users.rs::create admin/tasks.rs::set_triggers session/users.rs::set_policy ``` Some are worse than a status code. `library/images/writing.rs::store` and `set_splashscreen` read an uploaded picture into memory before asking whether the caller may upload one, and `admin/configuration.rs::update` deserializes a whole configuration document first. An account with a token but no permission can make the server do that work, repeatedly. ## Where to start The fix is not per-route. Two shapes worth weighing, and the choice wants an argument in `DECISIONS.md` because it changes how every write route is written: - **An extractor that carries the requirement**, so `Elevated` is a `FromRequestParts` beside `Authenticated` and runs before any body — the `elevated(&context, &caller)` call disappears from the handler. Closest to upstream's `[Authorize(Policy = …)]`, and the type says what the route needs. - **Middleware keyed on the route**, refusing before the handler is entered at all. Fewer signature changes, but the requirement stops being visible where the route is written, and the permission per route then lives somewhere other than the route. Either way `AppContext` has to be reachable from the extractor or the layer, which it is — it is already the router's state. `crates/jellyfin-http/src/auth.rs` is where `elevated` lives now, and `crates/jellyfin-http/tests/common/mod.rs` builds the router the route tests drive. ## How to know it is done An authenticated non-administrator posting an unreadable body to a route they may not reach is answered `403`, and the body is never read. `refusals.json` is where it belongs — it already replays anonymously and as the administrator, and the account it needs is `restricted`, which `visibility.json` already logs in as. Worth measuring the same three bodies against every one of the twenty, not just the one this was found on: the statuses ours gives now are the binder's and vary with the extractor. ### House rules Tests run under `cargo nextest`; upstream's source is the checkout in `upstream/` **at the pin**; generated artifacts are regenerated, never edited; comments say what the code does, not how it got there. The shared reference on `:8096` is read-only — logging in as `restricted` is a write, so anything beyond a refusal wants its own container from the pinned image over a `cp -a` copy of a snapshot, on its own ports. Check `ss -lntp` for a free port before binding.

Claude added the

correctness

http

security

labels

2026-08-16 06:47:43 +02:00

Claude referenced this issue

2026-08-16 08:41:50 +02:00

A disabled account's token still works: the check is at login only #143

alex referenced this issue from a commit

2026-08-16 09:28:23 +02:00

auth: make a route's policy an extractor, and run the checks upstream runs

alex closed this issue

2026-08-16 09:28:23 +02:00

Claude commented

2026-08-16 09:29:02 +02:00

Author

Collaborator

Closed by c9137c8, with the review's findings in 551d87d.

The extractor won, and the argument is DECISIONS.md D32. Every policy
upstream declares is a type implementing FromRequestParts — Authenticated,
Anonymous, Elevated, FirstTimeSetupOrElevated, FirstTimeSetupOrDefault,
IgnoreParentalControl, LocalOrElevated, CollectionManagement, Download,
SubtitleManagement — and a handler names the one its action is declared under
and calls nothing.

Two things the issue did not anticipate:

A rejected policy writes no body. Measured: POST /Backup/Create as
restricted is 403 with no content type at all, where the 403 an action
throws through SecurityException carries upstream's text/plain "Error processing request.". That is a second error variant, ForbiddenEmpty, and
six other sites were on the wrong one — every action returning Forbid().
The ordering bug has a second level. A route parameter that can fail to
bind is also a FromRequestParts, so Path<(String, String, i32)> declared
ahead of the policy answered 400 for a non-numeric image index before the
caller was looked at. Four handlers had it.

Beyond the twenty: /System/ActivityLog/Entries was answering any caller where
upstream is RequiresElevation, Items/{id}/Download was the last unconverted
UserPermissionRequirement, and the subtitle upload refused an administrator
that upstream admits — the permission handler alone would refuse one, and the
default handler beside it succeeds the same requirement for the administrator
role first.

Measured: backup.json replays the issue's three bodies as restricted —
13 responses, 0 differences, all three 403. refusals.json went from 8
differences to 2, and the two left are the pre-existing
/MusicGenres/InstantMix content type.

Closed by `c9137c8`, with the review's findings in `551d87d`. **The extractor won**, and the argument is `DECISIONS.md` D32. Every policy upstream declares is a type implementing `FromRequestParts` — `Authenticated`, `Anonymous`, `Elevated`, `FirstTimeSetupOrElevated`, `FirstTimeSetupOrDefault`, `IgnoreParentalControl`, `LocalOrElevated`, `CollectionManagement`, `Download`, `SubtitleManagement` — and a handler names the one its action is declared under and calls nothing. Two things the issue did not anticipate: - **A rejected policy writes no body.** Measured: `POST /Backup/Create` as `restricted` is `403` with no content type at all, where the `403` an action throws through `SecurityException` carries upstream's `text/plain "Error processing request."`. That is a second error variant, `ForbiddenEmpty`, and six other sites were on the wrong one — every action returning `Forbid()`. - **The ordering bug has a second level.** A route parameter that can fail to bind is also a `FromRequestParts`, so `Path<(String, String, i32)>` declared ahead of the policy answered `400` for a non-numeric image index before the caller was looked at. Four handlers had it. Beyond the twenty: `/System/ActivityLog/Entries` was answering any caller where upstream is `RequiresElevation`, `Items/{id}/Download` was the last unconverted `UserPermissionRequirement`, and the subtitle upload refused an administrator that upstream admits — the permission handler alone would refuse one, and the default handler beside it succeeds the same requirement for the administrator role first. **Measured**: `backup.json` replays the issue's three bodies as `restricted` — 13 responses, 0 differences, all three `403`. `refusals.json` went from 8 differences to 2, and the two left are the pre-existing `/MusicGenres/InstantMix` content type.

Claude referenced this issue

2026-08-16 09:29:46 +02:00

Per-request authorization stops at the token: neither remote access nor the access schedule is checked #134