From ac2eaf5de4e0b534a0bd066c8624f3428e735d7a Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Wed, 16 Sep 2026 18:51:56 +0200 Subject: [PATCH] fix: the four defects the client campaigns left open MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nothing here is a regression — all four shipped in `2.0.0-beta.0` — and all four are cleared before the tag rather than carried into it. **#224, a role separator cutting inside parentheses.** `split_on` applied its separators in passes over the whole value, with no idea where it was. A `COMPOSER` reading `Kobee (Melange / INHOUSE), Holy M (Melange / INHOUSE)` names two people; the two slashes made three entities, each carrying a parenthesis it never opened, and they reached the catalogue as artists that search answered with. It is one pass now, counting parenthesis depth and cutting only at zero. The rule the reference asks for is untouched: `Bach/Gounod` is still two people and `AC/DC` is still one band. Depth is clamped at zero so a stray `)` cannot make the rest of a value uncuttable, and a `(` that never closes protects only the tail it opened. **#225, `song` where the schema says `entry`.** `getPlayQueue` and `getNowPlaying` named their children as if the rename that applies inside a playlist did not apply to them. Three campaigns passed over it because every client that met it was lenient; one that decodes against the schema reads an empty queue. `playQueue` also gains the `username` the schema requires beside `changed` and `changedBy`. This is an observable wire change, made deliberately now: the freeze protects the clients that were validated, not a divergence from the specification, and a beta is what it is for. **#226, a 416 announcing a length of zero.** Two refusals wear that status and they do not mean the same thing. A complete file knows its length, so it says which range would have been satisfiable — and it accepts ranges, which every other answer for that same file already says, where this one claimed `none`. A transcode still being produced knows how many bytes it has written, never how many it will; `bytes */0` did not say "unknown", it said "empty", which a client may believe and never ask again. That header is now left out rather than filled with a falsehood. **#219, a refusal that outlived its session.** A sign-out and a sign-in fit inside one round trip, so a 401 raised for one account could arrive after another had signed in — renewing spent the new account's rotating refresh token for a request the old one made, and the replay went out under the new account's token. Five routes did this, not the four the issue names: the scan event stream renews the same way and was missed. One `renewFor` now holds the shape, checked before the renewal and again after it, because the renewal is itself a round trip a sign-out can happen inside. Every test here was run against the defect as well as against the fix. Three of them fail with the parenthesis guard removed or made too broad; the contract test fails on the rename and, separately, on `username` alone; the range assertions fail on `bytes */0` and on the wrong `Accept-Ranges`; the session test fails with the generation check neutralised, and it is the only one of the nineteen that does, which is what makes it new coverage rather than a second opinion. Two review findings were not taken. Rechecking the generation a second time between `renewFor` resolving and the replay closes a window one microtask wide that nothing in the module can write to — every writer sits behind an await, and a user gesture is a task, which cannot preempt a microtask drain; no scheduling distinguishes it, which is this repository's own bar for keeping a guard. And making the empty `getPlayQueue` schema-conforming would mean omitting the element, a third wire change on the one call every client makes at startup, with no observed harm and none of it asked for. That shape is pinned by a test instead, and left for #225 to settle. Signed-off-by: InstaZDLL --- docs/subsonic-compatibility.md | 8 ++ src/media.rs | 47 +++++++++--- src/subsonic/protocol.rs | 9 ++- src/subsonic/userdata.rs | 25 +++++-- src/tags.rs | 113 ++++++++++++++++++++++++++-- tests/media.rs | 38 ++++++++++ tests/subsonic_contract.rs | 133 ++++++++++++++++++++++++++++++++- webapp/src/api.ts | 37 +++++++-- webapp/src/session.test.ts | 56 ++++++++++++++ 9 files changed, 433 insertions(+), 33 deletions(-) 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);