From 8f64688a20845ddb3607144afc4ef2dd3235df2a Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Tue, 18 Aug 2026 23:59:35 +0200 Subject: [PATCH 1/2] fix(auth): a narrowed credential can no longer mint an unrestricted one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `write`-scoped API token on an administrator's account could reach `Admin` in two requests. Every link verified against the code: POST /api/v2/oauth/authorize took Access::Write, so a token issued ["write"] — deliberately without admin — passed. authorize_native_client requires no registered client: any non-empty client_id, a loopback redirect and an S256 challenge mint a code, and the authorization row records no scopes. POST /api/v2/oauth/token is unauthenticated by design. The session it returns is built with an empty scope list, and an empty list is unrestricted, so Access::Admin opened. The category error was treating "mutate user data" and "issue a reference to this account" as one authority. They are not: the second hands out a credential, and the credential handed out is broader than the one that asked. `Access::Unrestricted` names that. It is a restriction, not a role — only a credential carrying no scope list at all may mint one — so an ordinary account still pairs its own devices from its own session, which is what the flow is for. What it refuses is doing so on behalf of a credential that was deliberately narrowed. A read-only token could never start the chain, since the route already required a write, and the Subsonic facade is unaffected: it authenticates on `subsonic_credential`, not on `api_token`. The durable fix is to carry the scopes through the grant, so a session is never broader than what issued it. That needs a column on `oauth_authorization` and on `session`; this closes the path today without a migration, and both the enum and the RFC say so. Also corrected: the doc comment on `authenticated` claimed "being an administrator does not widen a token". That was the false half. The token widened itself, and the sentence now says which half held. The test builds the whole chain and asserts it stops at the first step, then asserts the refusal belongs to the narrowing and not to the account: the same account's session still grants, and the same token still writes what it was given. It fails if the route is put back to Access::Write. Signed-off-by: InstaZDLL --- docs/api-v2-guide.md | 10 ++++- docs/rfcs/RFC-002-waveflow-server-v2.md | 4 +- src/http.rs | 41 ++++++++++++++++++--- tests/v2_foundations.rs | 49 +++++++++++++++++++++++++ 4 files changed, 95 insertions(+), 9 deletions(-) diff --git a/docs/api-v2-guide.md b/docs/api-v2-guide.md index 9d133f9..be45847 100644 --- a/docs/api-v2-guide.md +++ b/docs/api-v2-guide.md @@ -525,10 +525,16 @@ scopes are checked: | Scope | Admits | |---|---| -| `write` | any mutation: playlists, favorites, ratings, the queue, bookmarks, shares, scrobbles, scans, issuing an OAuth code | +| `write` | any mutation: playlists, favorites, ratings, the queue, bookmarks, shares, scrobbles, scans | | `admin` | the administrative routes, and everything `write` admits | -Reading needs no scope, so a token naming neither is read-only. **A scope this +Reading needs no scope, so a token naming neither is read-only. **Minting a +credential needs no scope either — it needs the absence of one**: the +authorization code flow at `POST /api/v2/oauth/authorize` is refused to any +token carrying a scope list at all, because the session it returns is +unrestricted and nothing on the grant records what asked for it. A narrowed +credential must not be able to mint a broader one. Pairing a device is done +from a session, which is what the flow is for. **A scope this server does not know grants nothing**, which is why `catalog:read` reads and does no more: there is no vocabulary to learn, only these two names to use. diff --git a/docs/rfcs/RFC-002-waveflow-server-v2.md b/docs/rfcs/RFC-002-waveflow-server-v2.md index 3e68046..298392f 100644 --- a/docs/rfcs/RFC-002-waveflow-server-v2.md +++ b/docs/rfcs/RFC-002-waveflow-server-v2.md @@ -70,7 +70,9 @@ Genre matching is one rule across every method. `getGenres` groups by `genre.can `getArtistInfo` and `getArtistInfo2` resolve the requested artist through tenant-scoped catalogue access and return their standard empty containers until artist biography enrichment is implemented. This preserves compatibility with clients such as DSub without fabricating biography or similar-artist metadata. `getAlbumInfo` and `getAlbumInfo2` behave the same way for albums, for the same reason: Feishin and Symfonium call them as soon as an album page opens, and an unimplemented-method error there reads to the client as a broken album rather than as absent enrichment. They carry one real value, the album's release identifier, as their `musicBrainzId` element; notes and biography images stay absent because WaveFlow queries no remote source. `AlbumInfo` predates the presence rule and its members are elements rather than attributes, so an album with no release id omits the element instead of sending it empty. -API tokens are restricted by their scopes on every route, not only on the administrative ones. `Access` names what a route needs — `Read`, `Write` or `Admin` — and is chosen at the single point where the caller is resolved, so a route cannot exist without answering the question. An empty scope list is unrestricted, which is what sessions, OAuth grants and tokens issued without scopes carry; a non-empty list grants only what it names, and a name the server does not know grants nothing, so no vocabulary has to be enumerated. `admin` implies `write`, because a credential trusted to create accounts is not usefully barred from creating a playlist. Issuing a token stays administrative on both surfaces: a token carries the authority of the account it belongs to, so who may mint one is a question about the instance. +API tokens are restricted by their scopes on every route, not only on the administrative ones. `Access` names what a route needs — `Read`, `Write`, `Admin` or `Unrestricted` — and is chosen at the single point where the caller is resolved, so a route cannot exist without answering the question. An empty scope list is unrestricted, which is what sessions, OAuth grants and tokens issued without scopes carry; a non-empty list grants only what it names, and a name the server does not know grants nothing, so no vocabulary has to be enumerated. `admin` implies `write`, because a credential trusted to create accounts is not usefully barred from creating a playlist. Issuing a token stays administrative on both surfaces: a token carries the authority of the account it belongs to, so who may mint one is a question about the instance. + +Minting a credential is `Unrestricted`, which is a restriction and not a role: only a credential carrying no scope list at all may do it. The authorization code flow returns a session with the account's whole authority, and neither the authorization row nor the session records what asked for it, so a `write` token could reach `POST /api/v2/oauth/authorize` and come back holding a session that answered to `Admin` — an escalation in two requests, from a token deliberately issued without `admin`. The category error was treating "mutate user data" and "issue a reference to this account" as one authority. The durable fix is to carry the scopes through the grant, so that a session is never broader than what issued it; that needs a column on `oauth_authorization` and on `session`, and until it exists this closes the path without a migration. An ordinary account still pairs its own devices from its own session, which is what the flow is for. `playlist.owner` and `share.username` carry the authenticated username. Both collections are read scoped to their owner, so no other name is reachable; the empty string previously emitted made Feishin treat every playlist as another account's and refuse to edit it. diff --git a/src/http.rs b/src/http.rs index 3049540..8de950c 100644 --- a/src/http.rs +++ b/src/http.rs @@ -1109,7 +1109,11 @@ pub async fn oauth_authorize( ) -> Result, ApiError> { // The browser session is the proof of identity; the consent screen is a // route of the embedded client, so this is a JSON call rather than a form. - let user = authenticated(&state, &headers, Access::Write).await?; + // + // Unrestricted rather than Write: this mints a credential, and the one it + // mints carries the account's whole authority. A `write` token reaching + // here came back holding a session that answered to `Admin`. + let user = authenticated(&state, &headers, Access::Unrestricted).await?; let redirect_to = state .services .authorize_native_client( @@ -2237,11 +2241,30 @@ fn bearer_token(headers: &HeaderMap) -> Option<&str> { pub(crate) enum Access { /// Reads the caller's own catalogue and user data. Read, - /// Writes on the caller's behalf: playlists, favorites, ratings, the queue, - /// bookmarks, shares, scrobbles, scans, and issuing an OAuth code. + /// Writes on the caller's behalf: playlists, favorites, ratings, the + /// queue, bookmarks, shares, scrobbles and scans. Write, /// Acts on the instance: accounts, libraries, memberships, credentials. Admin, + /// Mints a new credential from this one. + /// + /// Only an unrestricted credential may, because the credential minted is + /// itself unrestricted: a session issued through the authorization code + /// flow carries the account's whole authority, and nothing on the grant + /// records what asked for it. Letting a narrowed credential mint one is + /// how a token widens itself — `write` was enough to reach the code flow, + /// and the session that came back answered to `Admin`. + /// + /// This is a restriction, not a role: an ordinary account pairs its own + /// devices from its own session, which is what the flow is for. What it + /// refuses is doing so on behalf of a credential that was deliberately + /// narrowed. + /// + /// The durable fix is to carry the scopes through the grant, so a session + /// is never broader than what issued it. That needs a column on + /// `oauth_authorization` and on `session`; until then, this closes the + /// path without a migration. + Unrestricted, } /// The scope that admits the administrative routes. @@ -2270,6 +2293,9 @@ impl Access { Self::Read => true, Self::Write => holds(WRITE_SCOPE) || holds(ADMIN_SCOPE), Self::Admin => holds(ADMIN_SCOPE), + // Unreachable: the early return above already answered for every + // empty list, and a non-empty one is by definition restricted. + Self::Unrestricted => false, } } } @@ -2278,9 +2304,12 @@ impl Access { /// what the route is about to do. /// /// Both halves of administrative authority live here: an active administrator, -/// on a credential that has not been narrowed away from it. Being an -/// administrator does not widen a token, and a token cannot promote an -/// ordinary account. +/// on a credential that has not been narrowed away from it. A token cannot +/// promote an ordinary account, and an administrator's token is not widened by +/// whose account it belongs to. +/// +/// It could widen itself, once. Minting a credential is [`Access::Unrestricted`] +/// for that reason, and not merely a write. pub(crate) async fn authenticated( state: &AppState, headers: &HeaderMap, diff --git a/tests/v2_foundations.rs b/tests/v2_foundations.rs index a5bcdd4..7ba4237 100644 --- a/tests/v2_foundations.rs +++ b/tests/v2_foundations.rs @@ -7954,6 +7954,55 @@ async fn pkce_authorization_grants_a_native_session_exactly_once() { StatusCode::UNAUTHORIZED ); + // A narrowed credential cannot mint an unrestricted one. The session the + // code flow returns carries the account's whole authority and records + // nothing about what asked for it, so a `write` token reaching this route + // came back holding a session that answered to `Admin`: two requests from + // a token deliberately issued without `admin`. + let scoped = json_body( + router + .clone() + .oneshot( + Request::post("/api/v2/admin/users/pkce-user/tokens") + .header("authorization", format!("Bearer {token}")) + .header("content-type", "application/json") + .body(Body::from( + serde_json::json!({"name": "agent", "scopes": ["write"]}).to_string(), + )) + .unwrap(), + ) + .await + .unwrap(), + ) + .await; + let scoped = scoped["secret"].as_str().expect("the secret").to_owned(); + assert_eq!( + authorize(grant.clone(), Some(scoped.clone())) + .await + .status(), + StatusCode::FORBIDDEN + ); + // It is the narrowing that refuses, not the account: the same account's + // session still grants, and the token still writes what it was given. + let writes = router + .clone() + .oneshot( + Request::builder() + .method(Method::PUT) + .uri("/api/v2/ratings/track/00000000-0000-4000-8000-000000000000") + .header("authorization", format!("Bearer {scoped}")) + .header("content-type", "application/json") + .body(Body::from(serde_json::json!({"rating": 3}).to_string())) + .unwrap(), + ) + .await + .unwrap(); + assert_ne!( + writes.status(), + StatusCode::FORBIDDEN, + "the write scope should still write" + ); + // A redirect that could carry the code off the machine is refused. let mut remote = grant.clone(); remote["redirect_uri"] = "http://evil.example.com/cb".into(); From 8b09a7b996ff3b02db5e7b143d03cf94abed9569 Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Wed, 19 Aug 2026 00:08:20 +0200 Subject: [PATCH 2/2] test(auth): prove the write scope writes, not that it was not refused MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The escalation test asserted the scoped token was `!= FORBIDDEN` on a rating against a fabricated UUID. That distinguished the scope gate — a `catalog:read` token answers 403 there — but it proved nothing beyond it: the request 404'd, and the assertion would have held just as well if the handler had become a no-op. The fixture now indexes one track, so the second half of the claim is a write: `204`, and the rating is read back from the catalogue afterwards. Both directions are checked, by breaking each in turn. Putting the authorization route back to `Access::Write` fails it at the escalation (`left: 200, right: 403`). Dropping `write` from what `Access::Write` accepts fails it at the write (`left: 403, right: 204`). One test, two opposite regressions, which is what the assertion is for: the refusal belongs to the narrowing, not to the account. Signed-off-by: InstaZDLL --- tests/v2_foundations.rs | 56 +++++++++++++++++++++++++++++++++++------ 1 file changed, 48 insertions(+), 8 deletions(-) diff --git a/tests/v2_foundations.rs b/tests/v2_foundations.rs index 7ba4237..a4714d1 100644 --- a/tests/v2_foundations.rs +++ b/tests/v2_foundations.rs @@ -7899,12 +7899,50 @@ async fn pkce_authorization_grants_a_native_session_exactly_once() { let (_temp, config, state) = test_app().await; let password = "correct horse battery staple"; let hash = security::hash_password(password).unwrap(); - state + let user = state .db .create_account("pkce-user", &hash, AccountRole::Admin, now_ms()) .await .unwrap(); - let router = waveflow_server::app(&config, state); + // One indexed track, so the scoped token can be shown writing rather + // than merely not being refused. + let music = config.data_dir.join("pkce-music"); + std::fs::create_dir_all(&music).unwrap(); + let library = state + .db + .create_library( + user, + "Pkce", + &std::fs::canonicalize(&music).unwrap(), + LibraryVisibility::Private, + now_ms(), + ) + .await + .unwrap(); + let scan = state + .db + .create_scan_job(library, Some(user), "manual") + .await + .unwrap(); + state.db.start_scan_job(scan, 1).await.unwrap(); + let mut input = browse_input(700, "Paired", "Handshake", "Loopback", Some(1), Some(1)); + input.relative_path = "pkce-0.flac".into(); + input.quick_hash = format!("{:064x}", 71_000); + input.full_hash = format!("{:064x}", 72_000); + state + .db + .apply_catalog_track(library, scan, &input, None, false) + .await + .unwrap(); + state.db.finish_scan_job(scan, 0).await.unwrap(); + let track = state + .services + .catalog_snapshot(user, &[]) + .await + .unwrap() + .songs[0] + .id; + let router = waveflow_server::app(&config, state.clone()); let token = login_token(&router, "pkce-user", password).await; let verifier = "H1r8mQ2xY7pL4vC0nB6zK9tW3sD5gJ8fA2eR7uI1oP4"; @@ -7989,7 +8027,7 @@ async fn pkce_authorization_grants_a_native_session_exactly_once() { .oneshot( Request::builder() .method(Method::PUT) - .uri("/api/v2/ratings/track/00000000-0000-4000-8000-000000000000") + .uri(format!("/api/v2/ratings/track/{track}")) .header("authorization", format!("Bearer {scoped}")) .header("content-type", "application/json") .body(Body::from(serde_json::json!({"rating": 3}).to_string())) @@ -7997,11 +8035,13 @@ async fn pkce_authorization_grants_a_native_session_exactly_once() { ) .await .unwrap(); - assert_ne!( - writes.status(), - StatusCode::FORBIDDEN, - "the write scope should still write" - ); + assert_eq!(writes.status(), StatusCode::NO_CONTENT); + // And the write landed: a route that had quietly become a no-op would + // still answer without refusing. + let ratings = state.services.ratings(user).await.unwrap(); + assert_eq!(ratings.len(), 1); + assert_eq!(ratings[0].entity_id, track); + assert_eq!(ratings[0].rating, 3); // A redirect that could carry the code off the machine is refused. let mut remote = grant.clone();