diff --git a/docs/subsonic-compatibility.md b/docs/subsonic-compatibility.md index e1841840..096910b3 100644 --- a/docs/subsonic-compatibility.md +++ b/docs/subsonic-compatibility.md @@ -19,6 +19,14 @@ Automated protocol coverage is enforced by the `subsonic_contract`, `subsonic_br > Nothing the 2026-09-16 campaign found is a regression. Everything it surfaced > is reachable at `v2.0.0-beta.0` too, and is filed as #224, #225 and #226. > +> **The rows record the runs, not today’s server.** #224, #225 and #226 were +> fixed after these campaigns, so two observations below have since stopped +> being true: `getPlayQueue` and `getNowPlaying` named their children `song` +> when DSub and Juliet met them, and now name them `entry`, with the `username` +> the schema requires beside them. That is an **observable wire change**, made +> deliberately during a beta rather than carried into a stable tag — the freeze +> protects the clients that were validated, not a divergence from the schema. +> > **How the four runs were told apart.** They shared one server and one log, and > the trace records path and status but never query strings, so no request names > its sender. Each run is bounded by a burst in the request rate, corroborated diff --git a/src/media.rs b/src/media.rs index f964242d..26e0e346 100644 --- a/src/media.rs +++ b/src/media.rs @@ -96,7 +96,10 @@ pub enum MediaError { Unauthorized, NotFound, InvalidRequest, - RangeNotSatisfiable(u64), + /// The range could not be honoured. The length is `None` when the + /// representation does not have one yet — a transcode still being produced + /// knows how many bytes it has written, never how many it will. + RangeNotSatisfiable(Option), /// A quota the caller cannot spend past. Distinct from a refusal of the /// bytes themselves: nothing is wrong with what was sent, there is only no /// room for it. @@ -209,7 +212,7 @@ impl MediaService { // Feishin with `bytes=0-`, then Juliet with the `bytes=0-1` probe iOS // sends before anything else. if range.is_some_and(|value| !starts_at_the_first_byte(value)) { - return Err(MediaError::RangeNotSatisfiable(0)); + return Err(MediaError::RangeNotSatisfiable(None)); } if query.offset_ms > 0 { @@ -1278,15 +1281,35 @@ impl IntoResponse for MediaError { (StatusCode::TOO_MANY_REQUESTS, [(header::RETRY_AFTER, "1")]).into_response() } Self::RangeNotSatisfiable(size) => { - let content_range = format!("bytes */{size}"); - ( - StatusCode::RANGE_NOT_SATISFIABLE, - [ - (header::CONTENT_RANGE, content_range.as_str()), - (header::ACCEPT_RANGES, "none"), - ], - ) - .into_response() + // Two refusals wear this status, and they do not mean the same + // thing. + // + // A complete file knows its length, so it can say which range + // would have been satisfiable — and it accepts ranges, which + // every other answer `serve_file` gives for that same resource + // already says. Only the range asked for was wrong. + // + // A transcode still being produced knows how many bytes it has + // written, never how many it will, and takes no range until it + // is whole. `bytes */0` would not say "length unknown": it + // would say the resource is empty, which a client is entitled + // to believe and never ask about again. The unsatisfied-range + // form has nowhere to put an unknown length, so the header is + // left out instead of filled with a falsehood. + let mut response = StatusCode::RANGE_NOT_SATISFIABLE.into_response(); + let headers = response.headers_mut(); + match size { + Some(size) => { + headers.insert(header::ACCEPT_RANGES, HeaderValue::from_static("bytes")); + if let Ok(value) = HeaderValue::from_str(&format!("bytes */{size}")) { + headers.insert(header::CONTENT_RANGE, value); + } + } + None => { + headers.insert(header::ACCEPT_RANGES, HeaderValue::from_static("none")); + } + } + response } Self::Internal => StatusCode::INTERNAL_SERVER_ERROR.into_response(), } @@ -1371,7 +1394,7 @@ async fn serve_file( match range { Some(range) => { let (start, end) = - parse_range(range, size).ok_or(MediaError::RangeNotSatisfiable(size))?; + parse_range(range, size).ok_or(MediaError::RangeNotSatisfiable(Some(size)))?; serve_partial(file, start, end, size, mime).await } None => { diff --git a/src/subsonic/protocol.rs b/src/subsonic/protocol.rs index 94f37f8a..e337c348 100644 --- a/src/subsonic/protocol.rs +++ b/src/subsonic/protocol.rs @@ -210,8 +210,8 @@ pub(super) fn json_array_field(parent: &str, name: &str) -> bool { | ("playlist", "entry") | ("bookmarks", "bookmark") | ("starred2" | "starred", "artist" | "album" | "song") - | ("nowPlaying", "song") - | ("playQueue", "song") + | ("nowPlaying", "entry") + | ("playQueue", "entry") | ("shares", "share") | ("share", "entry") | ("users", "user") @@ -220,8 +220,9 @@ pub(super) fn json_array_field(parent: &str, name: &str) -> bool { | ("lyricsList", "structuredLyrics") | ("structuredLyrics", "line") // A media item is rendered as `song`, and renamed to `entry` inside - // a playlist or share and to `child` inside a directory. Its - // OpenSubsonic relations are arrays under all three names. + // a playlist, a share, a play queue or the now-playing list, and to + // `child` inside a directory. Its OpenSubsonic relations are arrays + // under all three names. | ("song" | "entry" | "child" | "album", "artists" | "genres") // `getMusicDirectory` renders an album as `child`, so its arrays // have to keep their shape under that name too — otherwise one diff --git a/src/subsonic/userdata.rs b/src/subsonic/userdata.rs index a8cb53d1..c401f3c0 100644 --- a/src/subsonic/userdata.rs +++ b/src/subsonic/userdata.rs @@ -185,10 +185,15 @@ pub(super) async fn now_playing( .map_err(internal)?; Ok( Node::new("nowPlaying").children(entries.iter().map(|(username, song, started)| { - song_node(song).attr("username", username.clone()).attr( - "minutesAgo", - ((chrono::Utc::now().timestamp_millis() - started) / 60_000).max(0), - ) + // `entry`, not `song`: the schema renames a media item inside this + // container exactly as it does inside a playlist. + song_node(song) + .renamed("entry") + .attr("username", username.clone()) + .attr( + "minutesAgo", + ((chrono::Utc::now().timestamp_millis() - started) / 60_000).max(0), + ) })), ) } @@ -198,14 +203,24 @@ pub(super) async fn get_queue( principal: &Principal, ) -> Result { let Some(queue) = state.services.queue(principal.id).await.map_err(internal)? else { + // Nothing has been saved. The empty container carries no `username` + // either, because `changed` and `changedBy` would have to be invented + // beside it, and a queue that does not exist has no such facts. return Ok(Node::new("playQueue")); }; Ok(Node::new("playQueue") + .attr("username", principal.username.clone()) .maybe_attr("current", queue.current.map(|id| id.to_string())) .attr("position", queue.position_ms) .maybe_attr("changedBy", queue.changed_by) .attr("changed", iso_time(queue.updated_at)) - .children(queue.songs.iter().map(song_node))) + // `entry`, not `song` — see `nowPlaying` above. + .children( + queue + .songs + .iter() + .map(|song| song_node(song).renamed("entry")), + )) } pub(super) async fn save_queue( diff --git a/src/tags.rs b/src/tags.rs index beba8846..7a242444 100644 --- a/src/tags.rs +++ b/src/tags.rs @@ -113,14 +113,49 @@ const ARTIST_SEPARATORS: [&str; 6] = [" / ", " feat. ", " feat ", " ft. ", " ft /// composer's name contains a slash. const ROLE_SEPARATORS: [&str; 2] = ["/", ";"]; +/// Cuts `value` on any of `separators`, but never inside parentheses. +/// +/// A separator nested in a parenthesis does not divide two credits: it divides +/// what qualifies one. `Kobee (Melange / INHOUSE), Holy M (Melange / INHOUSE)` +/// names two people, and cutting at those slashes made three entities, each +/// carrying a parenthesis it never opened — `Kobee (Melange`, +/// `INHOUSE), Holy M (Melange`. They reached the catalogue as artists, and +/// search answered with them. +/// +/// The separators are tried in the order given at every position, which is what +/// keeps ` feat. ` ahead of ` feat `. Depth is clamped at zero so a stray +/// closing parenthesis cannot make the rest of the value uncuttable, and an +/// opening one that is never closed simply protects the tail it opened. fn split_on(value: &str, separators: &[&str]) -> Vec { - let mut parts = vec![value.to_owned()]; - for separator in separators { - parts = parts - .into_iter() - .flat_map(|part| part.split(separator).map(str::to_owned).collect::>()) - .collect(); + let mut parts = Vec::new(); + let mut depth: usize = 0; + let mut start = 0; + let mut cursor = 0; + while cursor < value.len() { + let rest = &value[cursor..]; + if depth == 0 { + if let Some(separator) = separators + .iter() + .find(|separator| rest.starts_with(**separator)) + { + parts.push(&value[start..cursor]); + cursor += separator.len(); + start = cursor; + continue; + } + } + let character = rest + .chars() + .next() + .expect("the cursor only ever lands on a character boundary"); + match character { + '(' => depth += 1, + ')' => depth = depth.saturating_sub(1), + _ => {} + } + cursor += character.len_utf8(); } + parts.push(&value[start..]); parts .into_iter() .map(|part| part.trim().to_owned()) @@ -386,6 +421,72 @@ mod tests { ); } + /// A separator inside parentheses qualifies one credit; it does not divide + /// two. The real tag that exposed this named two people and produced three + /// entities, each carrying a parenthesis it never opened. + #[test] + fn a_role_separator_inside_parentheses_does_not_cut() { + let raw = RawCredits { + artist: vec!["ITZY".into()], + roles: vec![( + Role::Composer, + vec!["Kobee (Melange / INHOUSE), Holy M (Melange / INHOUSE)".into()], + )], + ..RawCredits::default() + }; + assert_eq!( + names_of(&credits(&raw), Role::Composer), + vec!["Kobee (Melange / INHOUSE), Holy M (Melange / INHOUSE)"] + ); + } + + /// The parenthesis rule must not cost the cut it was added beside: a + /// separator outside one still divides, in the same value. + #[test] + fn a_role_separator_outside_parentheses_still_cuts() { + let raw = RawCredits { + artist: vec!["Nobody".into()], + roles: vec![( + Role::Composer, + vec!["Bach (arr. Gounod)/Liszt (after Bach)".into()], + )], + ..RawCredits::default() + }; + assert_eq!( + names_of(&credits(&raw), Role::Composer), + vec!["Bach (arr. Gounod)", "Liszt (after Bach)"] + ); + } + + /// A tag whose parentheses never balance must not swallow the whole value + /// or hang the scan. The opening one protects only the tail it opened. + #[test] + fn an_unbalanced_parenthesis_protects_only_its_tail() { + let unclosed = RawCredits { + artist: vec!["Nobody".into()], + roles: vec![( + Role::Composer, + vec!["Bach/Gounod (Melange / INHOUSE".into()], + )], + ..RawCredits::default() + }; + assert_eq!( + names_of(&credits(&unclosed), Role::Composer), + vec!["Bach", "Gounod (Melange / INHOUSE"] + ); + + // A stray closing parenthesis must not make the rest uncuttable. + let stray = RawCredits { + artist: vec!["Nobody".into()], + roles: vec![(Role::Composer, vec!["INHOUSE)/Bach".into()])], + ..RawCredits::default() + }; + assert_eq!( + names_of(&credits(&stray), Role::Composer), + vec!["INHOUSE)", "Bach"] + ); + } + /// A role tag is cut on the bare separators, where an artist tag is not. #[test] fn a_role_tag_is_cut_where_an_artist_tag_would_not_be() { diff --git a/tests/media.rs b/tests/media.rs index 9c54a322..2763dc84 100644 --- a/tests/media.rs +++ b/tests/media.rs @@ -102,6 +102,26 @@ async fn media_streaming_ranges_transcodes_caches_and_isolates_tenants() { .await .unwrap(); assert_eq!(unsatisfiable.status(), StatusCode::RANGE_NOT_SATISFIABLE); + // The file has a length, so the refusal states which range would have been + // satisfiable — and states the real one, read from the fixture rather than + // written down, so the assertion cannot drift away from the file. + let length = std::fs::metadata(music.join("Range.wav")).unwrap().len(); + assert_eq!( + unsatisfiable + .headers() + .get("content-range") + .and_then(|value| value.to_str().ok()), + Some(format!("bytes */{length}").as_str()) + ); + // And it still accepts ranges: only the range asked for was wrong. Saying + // `none` here would contradict every other answer this file gives. + assert_eq!( + unsatisfiable + .headers() + .get("accept-ranges") + .and_then(|value| value.to_str().ok()), + Some("bytes") + ); let hidden = router .clone() @@ -300,6 +320,24 @@ async fn media_streaming_ranges_transcodes_caches_and_isolates_tenants() { .await .unwrap(); assert_eq!(cold_seek.status(), StatusCode::RANGE_NOT_SATISFIABLE); + // Nothing has been produced yet, so there is no complete length to give. + // The unsatisfied-range form has nowhere to put "unknown", and `bytes */0` + // is not unknown — it is a claim that the resource is empty, which a client + // is entitled to believe and never ask again. The header is omitted. + assert!( + cold_seek.headers().get("content-range").is_none(), + "a refusal with no known length must not state one: {:?}", + cold_seek.headers().get("content-range") + ); + // This one genuinely takes no range until the transcode is whole, which is + // the other half of the same distinction. + assert_eq!( + cold_seek + .headers() + .get("accept-ranges") + .and_then(|value| value.to_str().ok()), + Some("none") + ); let live_seek = router .clone() diff --git a/tests/subsonic_contract.rs b/tests/subsonic_contract.rs index 6c872545..4d47c782 100644 --- a/tests/subsonic_contract.rs +++ b/tests/subsonic_contract.rs @@ -1094,7 +1094,7 @@ async fn subsonic_xml_json_auth_catalog_and_user_data_are_compatible() { .unwrap(); assert_eq!(listener_now_playing.status(), StatusCode::OK); let all_now_playing = subsonic_json(&router, "getNowPlaying", api_key, "").await; - assert!(all_now_playing["subsonic-response"]["nowPlaying"]["song"] + assert!(all_now_playing["subsonic-response"]["nowPlaying"]["entry"] .as_array() .unwrap() .iter() @@ -1585,3 +1585,134 @@ async fn subsonic_blurs_foreign_catalog_and_rate_limits_failed_authentication() assert_eq!(unknown.status(), wrong.status()); assert_eq!(body_text(unknown).await, body_text(wrong).await); } + +/// `getPlayQueue` and `getNowPlaying` name their children `entry`, never `song`. +/// +/// The schema renames a media item inside these two containers exactly as it +/// does inside a playlist, and this server sent `song` in both until +/// 2026-09-16. Three client campaigns passed over it because every client that +/// met it was lenient; a client that decodes against the schema reads an empty +/// queue and an empty now-playing list. +/// +/// Asserted in XML *and* JSON, and asserted negatively as well: the point is +/// not that `entry` appears, it is that `song` does not come back. Without the +/// negative half, emitting both names would pass. +#[tokio::test] +async fn play_queue_and_now_playing_name_their_children_entry() { + let (_temp, config, state) = test_app().await; + let web_hash = security::hash_password("web-password-for-test").unwrap(); + let owner = state + .db + .create_account("queue-owner", &web_hash, AccountRole::Admin, now_ms()) + .await + .unwrap(); + state + .db + .set_subsonic_credential( + owner, + owner, + &state.secret_box.encrypt(b"queue-password").unwrap(), + &security::token_hash("wfsk_queue"), + now_ms(), + ) + .await + .unwrap(); + let music = config.data_dir.join("queue-subsonic"); + std::fs::create_dir_all(&music).unwrap(); + write_test_wav(&music.join("Queued.wav")); + let root = std::fs::canonicalize(&music).unwrap(); + let library = state + .db + .create_library( + owner, + "Queue Subsonic", + &root, + LibraryVisibility::Private, + now_ms(), + ) + .await + .unwrap(); + run_scan( + &state, + owner, + LibraryRecord { + id: library, + name: "Queue Subsonic".into(), + root_path: root, + }, + ) + .await; + let song = state.db.list_tracks_for_user(owner, library).await.unwrap()[0].id; + let router = waveflow_server::app(&config, state); + + // Nothing is saved yet, and this pins what that answers today: a bare + // `playQueue`, carrying none of `username`, `changed` or `changedBy`. + // + // It is not schema-conforming, and it is deliberately left alone here. The + // schema has no way to say "empty queue" — the reference omits the element + // entirely — so conforming would mean a *third* observable change, on the + // one call every client makes at startup, with no client harm observed and + // none of it asked for. Pinned rather than corrected, and recorded on #225. + let unsaved = subsonic_json(&router, "getPlayQueue", "wfsk_queue", "").await; + let unsaved = &unsaved["subsonic-response"]["playQueue"]; + assert!(unsaved.is_object(), "{unsaved}"); + assert!(unsaved["entry"].is_null()); + assert!(unsaved["username"].is_null()); + + // Something has to be in each container, or an assertion that `song` is + // absent would hold on an empty answer and prove nothing. + subsonic_json( + &router, + "savePlayQueue", + "wfsk_queue", + &format!("&id={song}¤t={song}&position=25"), + ) + .await; + subsonic_json( + &router, + "scrobble", + "wfsk_queue", + &format!("&id={song}&submission=false"), + ) + .await; + + let queue = subsonic_json(&router, "getPlayQueue", "wfsk_queue", "").await; + let queue = &queue["subsonic-response"]["playQueue"]; + assert_eq!(queue["entry"][0]["id"], song.to_string()); + assert!(queue["song"].is_null(), "playQueue still carries `song`"); + // Required by the schema beside `changed` and `changedBy`, and absent until + // the same change: a queue belongs to somebody, and said so nowhere. + assert_eq!(queue["username"], "queue-owner"); + + let playing = subsonic_json(&router, "getNowPlaying", "wfsk_queue", "").await; + let playing = &playing["subsonic-response"]["nowPlaying"]; + assert_eq!(playing["entry"][0]["id"], song.to_string()); + assert!(playing["song"].is_null(), "nowPlaying still carries `song`"); + + // The XML half. A JSON-only assertion would miss a renaming applied to the + // array table alone, which is where the JSON shape is decided. + for (method, container) in [ + ("getPlayQueue", "playQueue"), + ("getNowPlaying", "nowPlaying"), + ] { + let response = router + .clone() + .oneshot( + Request::get(format!( + "/rest/{method}.view?apiKey=wfsk_queue&v=1.16.1&c=golden" + )) + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(response.status(), StatusCode::OK); + let xml = body_text(response).await; + assert!(xml.contains(&format!("<{container}")), "{method}: {xml}"); + assert!(xml.contains(" { + if (generation !== sessionGeneration) return false; + if (!(await refresh())) return false; + return generation === sessionGeneration; +} + function refresh(): Promise { // Shared only with callers of the same session. A renewal that outlived the // account it was started for now answers `false` on purpose, and handing that @@ -386,7 +406,7 @@ async function call( // whoever just arrived back on the sign-in screen. const generation = sessionGeneration; const response = await fetch(path, { ...init, headers }); - if (response.status === 401 && retry && (await refresh())) { + if (response.status === 401 && retry && (await renewFor(generation))) { return call(path, init, false, true); } // Refused again, on the attempt that a renewal had already paid for. A @@ -665,12 +685,14 @@ export async function putUploadChunk( ): Promise { const headers = new Headers({ "content-type": "application/octet-stream" }); if (session) headers.set("authorization", `Bearer ${session.access_token}`); + // Whose session this attempt speaks for; see `renewFor`. + const generation = sessionGeneration; const response = await fetch(`/api/v2/uploads/${id}/chunks/${index}`, { method: "PUT", headers, body: bytes, }); - if (response.status === 401 && retry && (await refresh())) { + if (response.status === 401 && retry && (await renewFor(generation))) { return putUploadChunk(id, index, bytes, false); } if (!response.ok) { @@ -721,7 +743,7 @@ export async function placeCanvas( // Whose session this attempt speaks for; see the same line in `call`. const generation = sessionGeneration; const response = await fetch(path, { method: "PUT", headers, body: file }); - if (response.status === 401 && retry && (await refresh())) { + if (response.status === 401 && retry && (await renewFor(generation))) { return placeCanvas(trackId, file, false, true); } // The same dead end `call` handles, on the one route that cannot go through @@ -807,10 +829,12 @@ async function loadArtworkUrl( ): Promise { const headers = new Headers(); if (session) headers.set("authorization", `Bearer ${session.access_token}`); + // Whose session this attempt speaks for; see `renewFor`. + const generation = sessionGeneration; const response = await fetch(`/api/v2/artwork/${encodeURIComponent(id)}`, { headers, }); - if (response.status === 401 && retry && (await refresh())) { + if (response.status === 401 && retry && (await renewFor(generation))) { return loadArtworkUrl(id, false); } // The same distinction `canvasUrl` makes above, and for the same reason: a @@ -960,8 +984,11 @@ export function watchScan( // repeat what `call` does about an expired access token. Without it an // admin who left the tab open watched "waiting for the first reading" // for as long as the scan took, and then for ever. + // Whose session this stream speaks for; see `renewFor`. This route is + // the fifth of the kind and was not named in the issue. + const generation = sessionGeneration; let response = await open(); - if (response.status === 401 && (await refresh())) { + if (response.status === 401 && (await renewFor(generation))) { response = await open(); } if (!response.ok || !response.body) { diff --git a/webapp/src/session.test.ts b/webapp/src/session.test.ts index e4af9b4a..857419b9 100644 --- a/webapp/src/session.test.ts +++ b/webapp/src/session.test.ts @@ -246,6 +246,62 @@ describe("session refresh", () => { expect(navigate).not.toHaveBeenCalled(); }); + it("neither renews nor replays a refusal that outlived its session", async () => { + const api = await freshApi(); + await signIn(api); + let releaseRefusal = () => {}; + const heldRefusal = new Promise((resolve) => { + releaseRefusal = () => resolve(jsonResponse({}, 401)); + }); + let renewals = 0; + let reads = 0; + fetchStub.mockImplementation(async (input: RequestInfo | URL) => { + const url = String(input); + if (url.includes("/auth/refresh")) { + renewals += 1; + return jsonResponse({ + access_token: "renewed", + user: { id: "u2", username: "other", role: "admin" }, + device_id: "d2", + }); + } + if (url.includes("/auth/login")) + return jsonResponse({ + access_token: "second-token", + user: { id: "u2", username: "other", role: "admin" }, + device_id: "d2", + }); + if (url.includes("/auth/logout")) + return new Response(null, { status: 204 }); + reads += 1; + // Held open on purpose. Hoping to land inside the window instead of + // holding it open is how this test passes with the guard removed. + return heldRefusal; + }); + + const refusedForTheFirst = api.getTrack("t1"); + await vi.waitFor(() => expect(reads).toBe(1)); + + // The whole race, made deliberate: the account the request belongs to signs + // out and another signs in while its refusal is still in the air. + await api.logout(); + await api.login("other", "correct horse battery staple"); + expect(api.hasSession()).toBe(true); + + releaseRefusal(); + await expect(refusedForTheFirst).rejects.toThrow(); + + // Renewing here would spend the **new** account's rotating refresh token + // for a request the previous one made, and the replay would go out under + // the new account's access token, because a retry rebuilds its headers + // from the current session. Nobody asked for that request, and its answer + // would be delivered to a consumer that has been torn down. + expect(renewals).toBe(0); + expect(reads).toBe(1); + expect(api.hasSession()).toBe(true); + expect(navigate).not.toHaveBeenCalled(); + }); + it("leaves it alone for a caller that never asked for a renewal", async () => { const api = await freshApi(); await signIn(api);