Per-request authorization stops at the token: neither remote access nor the access schedule is checked #134
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Upstream runs
DefaultAuthorizationHandleron every[Authorize]request, and it fails the request in two cases beyond "the token is valid":EnableRemoteAccessand the request did not come from the local network, andThis port resolves the token to a session and stops (
crates/jellyfin-http/src/auth.rs).EnableRemoteAccessis checked only at login (crates/jellyfin-app/src/session/auth/mod.rs:116), and the access schedule is checked nowhere at all.Two concrete consequences, on every authorized route rather than any one of them:
AccessSchedulesis one of the 31 tables this project binds, and M7 wrote the user-management routes that maintain it, so the rows exist and are served — nothing reads them when a request arrives.Read off upstream's source rather than measured. Confirming it needs a restricted account on the reference, which is a write to the shared fixture; #111 established the pattern for that — stand up a private container from the pinned image and make the accounts there.
Where to start
crates/jellyfin-http/src/auth.rsis where a request becomes a caller, and it is the one place that can enforce this without 96 call sites remembering to. Upstream:Jellyfin.Api/Auth/DefaultAuthorizationPolicy/DefaultAuthorizationHandler.cs, and theNetworkManagercall it uses to decide what "local" means — which this port models thinly, so what counts as the local network is its own question worth settling fromIsInLocalNetworkrather than assumed to be loopback.Note the shape #111 chose for by-id reads and prefer it here: make the unchecked path impossible to express rather than adding a guard each route must remember.
How to know it is done
A restricted account's token, replayed from outside the local network and outside its schedule, is refused with the same status and body upstream gives — measured on a private pair, and carried by a scenario so a replay holds it.
House rules
Tests run under
cargo nextest; upstream's source is the checkout inupstream/at the pin; generated artifacts are regenerated, never edited; comments say what the code does, not how it got there. The shared reference on:8096is read-only — a login is a write, so anything needing an account wants its own container from the pinned image over asqlite3 .backupcopy ofreference/var/config/data/jellyfin.db. Checkss -lntpfor a free port before binding; stale servers from earlier work are still listening.Folding this into #131 rather than tracking it separately — it came out of that work, it is the same module, and the fix belongs in the same branch. Reopening only if it turns out to be larger than that branch should carry.
Reopening. It was folded into #131 on the reasoning that it was the same module; it is not — it changes the answer of every authorized route, and #131's implementer made the case with measurements rather than an opinion.
Measured, on a private pair
A token issued while inside an access schedule, with the window then moved away:
GET /Users/MeGET /Items?limit=1GET /SessionsGET /System/Info/System/Infoagreeing is not luck: it is[Authorize(Policy = Policies.FirstTimeSetupOrIgnoreParentalControl)](SystemController.cs:68), one of exactly two actions upstream exempts from the parental check. The other isUserController.cs:128.Why it is not a small change
Authenticatedis uniform today; the two exempt actions have to skip the schedule check, which means the type has to carry which policy it was reached under.by_id,permissions,access_schedules) where upstream reads a cached user. #111 paid +0.07 ms for three primary-key lookups on a by-id read and argued the cost; this wants the same argument with its own number.What is already done, and not part of this
The login door is closed in #131's branch: upstream refuses a login outside the schedule with 403 (
UserManager.cs:598) where we answered 200, andcrates/jellyfin-app/src/session/auth/mod.rsalready refusesIsDisabledand remote access in that same place. That was the self-contained half.jellyfin_core::config::is_in_local_networkalready exists and is whatauth.rsand the login path use, so what "local network" means is settled and does not need re-deciding here.The order to do it in
DefaultAuthorizationHandlerinAuthenticated— remote access, the administrator short-circuit, the parental schedule, and the twoIgnoreParentalControlactions modelled. Upstream:Jellyfin.Api/Auth/DefaultAuthorizationPolicy/DefaultAuthorizationHandler.cs.account(#111 added it) andxtask reference provisionalready creates arestricteduser; what is missing is one whose schedule and remote-access permission are set.Prefer the shape #111 chose: make the unchecked path impossible to express rather than adding a guard each route must remember.
Closed by
c9137c8— the same commit as #142, which is what the batch note saidwould happen: apart, that extractor gets written twice.
Both checks now run where a request becomes a caller, in
admitted()— upstream'sDefaultAuthorizationHandler, in its own order: anapi key is unrestricted, a caller with no account decides nothing, the
remote-access check, the administrator short-circuit, then the parental
schedule.
jellyfin_app::session::auth::admitted_atis reused for the schedule,and
jellyfin_core::config::is_in_local_networkfor what "local" means, as thisissue said it should be.
The policy set is not uniform, which is why each is a type. From upstream's
registrations:
RequiresElevationis a role claim alone and runs neither check;the
FirstTimeSetup*policies run remote access and skip the schedule;IgnoreParentalControlandFirstTimeSetupOrIgnoreParentalControlare the twoactions this issue counted as exempt from the parental check, and they are
GET /Users/{userId}andGET /System/Info. A single flag could not have saidthat.
The cost this issue asked to be argued. Permissions are read once as the
request becomes a caller and carried on
Authenticated, soadministers()costsno query where it used to cost one — the default policy's net addition is one
read of
AccessSchedules, for a non-administrator only, plus one primary-keyread of the account added later by #144. Timed over a thousand sequential
/Users/Merequests: 0.464 ms median against 0.470 ms without the account read,which is below the floor of that measurement. Recorded in D32, with the note
that it wants retaking on a library whose tables are not small.
The restricted-account fixture this issue asked for, item 2 of its own
ordering, is not done: the checks are held by route tests in
crates/jellyfin-http/tests/authorization.rs, twelve of them, which build theaccount and the schedule they need in memory. A replayable fixture would want an
account whose schedule and remote-access permission are set, and that is a write
to the shared reference — worth its own issue if a scenario ever needs it.