From 21d5952ed39155e6d74592d8ba1ea31823cf23b8 Mon Sep 17 00:00:00 2001 From: Mathias Picker <48158184+MathiasWP@users.noreply.github.com> Date: Mon, 24 Aug 2026 15:34:54 +0200 Subject: [PATCH 1/2] Keep the cookies when the sign-in page hasn't loaded yet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clicking "Pick credential…" without a sign-in window already open reported "timed out reading the sign-in window", while holding a perfectly good session cookie. Opening the window first and then picking worked, which is the tell. `read_session` starts by reading `localStorage`, which means evaluating script in the page — and that fails until the page has loaded. The `?` propagated the failure, throwing away the cookie sweep with it. But cookies come from the Rust side rather than from the page: they need no script and are readable the moment the window exists. The one part that was working was being discarded because a different part wasn't ready. So the eval failure is now held rather than propagated, and only reported if the cookie sweep also comes back empty — nothing to show is still worth saying, and it was the honest answer all along for that case only. The retry loop needed a matching change, or this would have traded one bug for another. It stopped at the first non-empty read; with cookies arriving before the page does, that would hand back a cookies-only snapshot the instant the window opened, and miss a credential kept in `localStorage` entirely. It now settles on a *complete* read and keeps a cookies-only one as the fallback, so neither source loses to the other's timing. Also drops the worst case from ~43s (eight attempts each waiting out a 5s eval timeout) to returning as soon as the page answers. --- .../cookies-survive-an-unloaded-page.md | 9 ++ src-tauri/src/browser.rs | 112 ++++++++++++++---- 2 files changed, 95 insertions(+), 26 deletions(-) create mode 100644 .changeset/cookies-survive-an-unloaded-page.md diff --git a/.changeset/cookies-survive-an-unloaded-page.md b/.changeset/cookies-survive-an-unloaded-page.md new file mode 100644 index 0000000..f4e828c --- /dev/null +++ b/.changeset/cookies-survive-an-unloaded-page.md @@ -0,0 +1,9 @@ +--- +'fiber': patch +--- + +Stop "Pick credential…" timing out when no sign-in window is open yet. + +Reading `localStorage` means evaluating script in the page, which fails until the page has loaded. That failure was propagated, discarding the cookies along with it — even though cookies are read from the Rust side, need no script, and are there immediately. So opening the picker straight from a closed window reported "timed out reading the sign-in window" while the session cookie sat in hand; opening the window first, then picking, worked. + +Cookies are now kept when the page can't be read, and the timeout is only reported when there is genuinely nothing to show. The retry loop still waits for a complete read before settling, so a credential kept in `localStorage` isn't missed by returning the cookies-only snapshot the instant the window opens. diff --git a/src-tauri/src/browser.rs b/src-tauri/src/browser.rs index 40bbe08..74775f5 100644 --- a/src-tauri/src/browser.rs +++ b/src-tauri/src/browser.rs @@ -206,24 +206,37 @@ pub async fn snapshot(app: &AppHandle, section: &Section) -> Result open(app, section, false)?, }; - let found = if borrowed { - read_session(&window, section).await - } else { - // Freshly opened: give the page time to load before believing an empty - // result, but stop as soon as there's anything there. - let mut last = Ok(Snapshot::default()); - for _ in 0..LOAD_ATTEMPTS { - tokio::time::sleep(LOAD_INTERVAL).await; - last = read_session(&window, section).await; - if matches!(&last, Ok(found) if !found.is_empty()) { - break; + if borrowed { + return read_session(&window, section) + .await + .map(|reading| reading.snapshot); + } + + // Freshly opened: give the page time to load before believing an empty + // result, but stop as soon as there's a complete one. + // + // "Complete" rather than "non-empty", because cookies arrive before the + // page does. Stopping at the first non-empty read would hand back a + // cookies-only snapshot the moment the window opened — fine for a session + // cookie, and missing the credential entirely for anything kept in + // `localStorage`. A cookies-only read is still kept, so if the page never + // answers we return what we have rather than nothing. + let mut last = Ok(Snapshot::default()); + for _ in 0..LOAD_ATTEMPTS { + tokio::time::sleep(LOAD_INTERVAL).await; + match read_session(&window, section).await { + Ok(reading) => { + let settled = reading.complete && !reading.snapshot.is_empty(); + last = Ok(reading.snapshot); + if settled { + break; + } } + Err(failure) => last = Err(failure), } - let _ = window.close(); - last - }; - - found + } + let _ = window.close(); + last } /// Reading IndexedDB is asynchronous, and a script that returns a Promise is no @@ -463,18 +476,63 @@ fn merge_decrypted(entries: &mut Vec, decrypted: Vec } /// Reads `localStorage` and every cookie visible to an open window. -async fn read_session(window: &WebviewWindow, section: &Section) -> Result { +async fn read_session(window: &WebviewWindow, section: &Section) -> Result { let (login_url, ..) = browser_config(section)?; - let mut local_storage = read_local_storage(window).await?; - // MSAL entries arrive as ciphertext; put the plaintext in their place. - merge_decrypted( - &mut local_storage, - read_parked(window, MSAL_DECRYPT_JS).await, - ); + // Best-effort, deliberately. + // + // Reading `localStorage` means evaluating script in the page, which fails + // until the page has loaded — so on a window we just opened, the first + // attempts time out. Propagating that discarded the cookies with it, and + // cookies are read from *here* rather than from the page: they need no + // script and are available immediately. So opening the picker without a + // sign-in window already up reported "timed out reading the sign-in + // window" while holding a perfectly good session cookie. + // + // The failure is kept rather than dropped: if nothing at all can be read it + // is still the honest answer, and it is reported below. + let eval_failure = match read_local_storage(window).await { + Ok(mut local_storage) => { + // MSAL entries arrive as ciphertext; put the plaintext in their place. + merge_decrypted( + &mut local_storage, + read_parked(window, MSAL_DECRYPT_JS).await, + ); + let snapshot = read_cookies(window, section, login_url, local_storage).await?; + return Ok(Reading { + snapshot, + complete: true, + }); + } + Err(failure) => failure, + }; - // Cookies for both origins: the API we'll be calling, and the identity - // provider we just signed in to. Often they differ. + let snapshot = read_cookies(window, section, login_url, Vec::new()).await?; + if snapshot.cookies.is_empty() { + return Err(eval_failure); + } + Ok(Reading { + snapshot, + complete: false, + }) +} + +/// A read of the session, and whether the page itself answered. +struct Reading { + snapshot: Snapshot, + /// False when evaluating script failed — the page had not loaded — so this + /// holds cookies only. Worth using, and worth retrying: a credential kept + /// in `localStorage` would not be in it yet. + complete: bool, +} + +/// The cookie half of a snapshot, which needs nothing of the page. +async fn read_cookies( + window: &WebviewWindow, + section: &Section, + login_url: &str, + local_storage: Vec, +) -> Result { // The whole cookie store, not just the two origins we happen to know about. // A sign-in commonly ends up setting the session cookie on a host that is // neither the API base nor the login URL — a staging subdomain, an apex @@ -666,7 +724,9 @@ pub async fn silent_recapture(app: &AppHandle, section: &Section) -> Result Date: Mon, 24 Aug 2026 15:43:26 +0200 Subject: [PATCH 2/2] Give every loader request its own cancel handle, and name the redirect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more things from the same investigation, both of which were making the real problem harder to see. "request cancelled" — `HttpState` keys in-flight requests by `RequestSpec::id`, and `HashMap::insert` under an existing key drops the old cancel sender. Dropping it is exactly what the receiver is waiting for, so the earlier request returns `Cancelled`. Every loader request for a section shared one id, so any two that overlapped killed each other: a "Fetch a sample" while a background refresh was in flight, and which one died came down to timing. Nothing cancels a loader request by id — only the window does that, for requests a person sent — so the id had no reason to be predictable in the first place. Second, a rejected manifest now reports where the response came from when it left the origin the request was aimed at. Compared by origin rather than by whole URL: following a redirect within the same host is ordinary and says nothing, while leaving the host is what silently sheds the credential (`reqwest_strips` lists `cookie` and `authorization`, so those ride reqwest's own policy, which drops them cross-host). "403" and "403, having ended up on a different host" are different problems wearing the same status, and only one of them is the API's fault. `LoaderResponse` carries the resolved request URL for that comparison, because the loader itself only ever holds the relative form. The sidebar's loader error is now selectable and has a Copy button. The first thing anyone does with an error they can't act on is try to send it to someone who can, and "I'm not sure how to give you the exact error" is a bug report about this app, not about the person writing it. --- ...der-requests-stop-cancelling-each-other.md | 11 ++ src-tauri/src/lib.rs | 10 +- src-tauri/src/loader.rs | 127 +++++++++++++++++- src-tauri/src/mcp.rs | 11 +- src/lib/components/Sidebar.svelte | 14 +- 5 files changed, 156 insertions(+), 17 deletions(-) create mode 100644 .changeset/loader-requests-stop-cancelling-each-other.md diff --git a/.changeset/loader-requests-stop-cancelling-each-other.md b/.changeset/loader-requests-stop-cancelling-each-other.md new file mode 100644 index 0000000..22102f3 --- /dev/null +++ b/.changeset/loader-requests-stop-cancelling-each-other.md @@ -0,0 +1,11 @@ +--- +'fiber': patch +--- + +Stop two loader requests cancelling each other, and say when a rejection came from somewhere else. + +Every loader request for a section used the same id, and `HttpState` keys in-flight requests by that id — inserting a second under a key already there drops the first's cancel sender, which *is* the cancel signal. So a "Fetch a sample" while a background refresh was out came back "request cancelled", and which of the two died depended on timing. Loader requests now get a unique handle each; nothing cancels them by id, so there was never a reason for it to be predictable. + +A rejected manifest now also reports where the response actually came from, when that left the origin the request was aimed at. A Cookie or Authorization credential is dropped on a cross-host redirect, so "403" and "403, having ended up on a different host" are different problems wearing the same status — and only one of them is your API's fault. + +The sidebar's loader error is selectable and has a Copy button, because the first thing anyone does with an error they can't act on is send it to someone who can. diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index 1065118..1f08b71 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -387,8 +387,9 @@ mod gui { Box::pin(async move { let url = store::join_url_scoped(§ion.base_url, &request.url) .map_err(|err| format!("loader URL rejected: {err}"))?; + let requested_url = url.clone(); let spec = RequestSpec { - id: format!("loader:{}", section.id), + id: loader::request_id(§ion.id), request_id: format!("loader:{}", section.id), section_id: Some(section.id.clone()), method: request.method, @@ -420,6 +421,8 @@ mod gui { Ok(loader::LoaderResponse { status: response.status, body: response.body, + requested_url, + final_url: response.final_url, }) }) }) @@ -509,10 +512,7 @@ mod gui { .map_err(LoaderError::Fetch)?; if !(200..300).contains(&response.status) { - return Err(LoaderError::Status { - status: response.status, - detail: loader::detail_from(&response.body), - }); + return Err(loader::rejected(&response)); } serde_json::from_str(&response.body).map_err(|err| LoaderError::NotJson(err.to_string())) } diff --git a/src-tauri/src/loader.rs b/src-tauri/src/loader.rs index 1cada92..20f8f7d 100644 --- a/src-tauri/src/loader.rs +++ b/src-tauri/src/loader.rs @@ -21,6 +21,7 @@ //! server can run a loader headlessly. use std::collections::{BTreeMap, HashSet}; +use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::Arc; use std::time::Duration; @@ -193,6 +194,25 @@ pub struct LoaderRun { pub pages: usize, } +/// A cancellation handle no other request can collide with. +/// +/// `HttpState` keys in-flight requests by `RequestSpec::id`, and inserting a +/// second one under a key already there drops the first's cancel sender — +/// which *is* the cancel signal. Every loader request for a section used the +/// same id, so any two that overlapped killed each other: a "Fetch a sample" +/// while a background refresh was out came back "request cancelled", and which +/// of the two died depended on timing. +/// +/// Nothing cancels a loader request by id — only the window does that, for +/// requests a person sent — so the id has no reason to be predictable. +pub fn request_id(section_id: &str) -> String { + static NEXT: AtomicU64 = AtomicU64::new(0); + format!( + "loader:{section_id}:{}", + NEXT.fetch_add(1, Ordering::Relaxed) + ) +} + /// A request the loader makes, before the section's base URL and auth apply. #[derive(Debug, Clone)] pub struct LoaderRequest { @@ -200,10 +220,20 @@ pub struct LoaderRequest { pub method: String, } -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Default)] pub struct LoaderResponse { pub status: u16, pub body: String, + /// The absolute URL the request was aimed at, once the section's base URL + /// had been applied. The loader itself only ever holds the relative form, + /// so the comparison below has to be made against what actually went out. + pub requested_url: String, + /// Where the response actually came from, after any redirects. Reported on + /// a rejection when it left the origin the request was aimed at: a Cookie + /// or Authorization credential is dropped on a cross-host hop, so "403" + /// and "403, and by the way you ended up somewhere else" are different + /// problems with the same status. + pub final_url: String, } /// How the host performs the loader's request. Injected so this module stays @@ -443,14 +473,50 @@ async fn fetch_json( .map_err(LoaderError::Fetch)?; if !(200..300).contains(&response.status) { - return Err(LoaderError::Status { - status: response.status, - detail: detail_from(&response.body), - }); + return Err(rejected(&response)); } serde_json::from_str(&response.body).map_err(|err| LoaderError::NotJson(err.to_string())) } +/// The error for a non-2xx manifest response, body and redirect included. +pub(crate) fn rejected(response: &LoaderResponse) -> LoaderError { + let detail = match ( + detail_from(&response.body), + redirect_note(&response.requested_url, &response.final_url), + ) { + (Some(body), Some(note)) => Some(format!("{body} ({note})")), + (Some(body), None) => Some(body), + (None, note) => note, + }; + + LoaderError::Status { + status: response.status, + detail, + } +} + +/// "redirected to ", when the response came from somewhere else. +/// +/// Compared by origin rather than by whole URL: following a redirect within the +/// same host is ordinary and says nothing, while leaving the host is the thing +/// that silently drops the credential. +fn redirect_note(requested: &str, final_url: &str) -> Option { + if final_url.trim().is_empty() { + return None; + } + let (from, to) = (origin_of(requested)?, origin_of(final_url)?); + (from != to).then(|| format!("redirected to {to}, which the credential is not sent to")) +} + +fn origin_of(url: &str) -> Option { + let parsed = reqwest::Url::parse(url.trim()).ok()?; + Some(format!( + "{}://{}", + parsed.scheme(), + parsed.host_str().unwrap_or_default() + )) +} + /// How much of a rejected manifest body is worth showing. const DETAIL_LIMIT: usize = 200; @@ -643,6 +709,7 @@ mod tests { Ok(LoaderResponse { status: 200, body: body.replace("{{url}}", &request.url), + ..Default::default() }) }) }) @@ -759,6 +826,7 @@ mod tests { Ok(LoaderResponse { status: 200, body: body.to_string(), + ..Default::default() }) }) }); @@ -786,6 +854,7 @@ mod tests { status: 200, body: r#"{"routes":[{"verb":"GET","url":"/loop"}],"links":{"next":"/again"}}"# .to_string(), + ..Default::default() }) }) }); @@ -947,6 +1016,7 @@ mod tests { Ok(LoaderResponse { status: 403, body: String::new(), + ..Default::default() }) }) }); @@ -968,6 +1038,7 @@ mod tests { Ok(LoaderResponse { status: 403, body: r#"{"detail": "CSRF token missing"}"#.to_string(), + ..Default::default() }) }) }); @@ -996,6 +1067,51 @@ mod tests { assert_eq!(detail_from(" "), None); } + /// Leaving the origin is what silently drops a Cookie or Authorization + /// credential, so a 403 that arrived from somewhere else has to say so — + /// otherwise it is indistinguishable from the API refusing you outright. + #[test] + fn a_rejection_after_a_cross_host_redirect_says_where_it_ended_up() { + let response = LoaderResponse { + status: 403, + body: r#"{"message": "Token is empty"}"#.into(), + requested_url: "https://staging.example.com/openapi.json".into(), + final_url: "https://login.example.com/signin".into(), + }; + + assert_eq!( + rejected(&response).to_string(), + "the manifest request returned 403: Token is empty (redirected to \ + https://login.example.com, which the credential is not sent to)" + ); + } + + /// A redirect that stays put is ordinary and says nothing worth saying. + #[test] + fn a_same_origin_redirect_is_not_worth_mentioning() { + let response = LoaderResponse { + status: 403, + body: String::new(), + requested_url: "https://api.example.com/openapi.json".into(), + final_url: "https://api.example.com/v2/openapi.json".into(), + }; + + assert_eq!( + rejected(&response).to_string(), + "the manifest request returned 403" + ); + } + + /// Two loader requests must not share a cancellation handle: inserting the + /// second under the first's key drops its sender, which reads as a cancel. + #[test] + fn every_loader_request_gets_its_own_cancel_handle() { + let first = request_id("sec-1"); + let second = request_id("sec-1"); + assert_ne!(first, second); + assert!(first.starts_with("loader:sec-1:")); + } + #[tokio::test] async fn reports_a_response_that_is_not_json() { let html: Fetcher = Arc::new(|_| { @@ -1003,6 +1119,7 @@ mod tests { Ok(LoaderResponse { status: 200, body: "login".to_string(), + ..Default::default() }) }) }); diff --git a/src-tauri/src/mcp.rs b/src-tauri/src/mcp.rs index 8ee9e4c..e402b82 100644 --- a/src-tauri/src/mcp.rs +++ b/src-tauri/src/mcp.rs @@ -396,8 +396,9 @@ impl FiberMcp { Box::pin(async move { let url = store::join_url_scoped(§ion.base_url, &request.url) .map_err(|err| format!("loader URL rejected: {err}"))?; + let requested_url = url.clone(); let spec = RequestSpec { - id: format!("mcp-loader:{}", section.id), + id: loader::request_id(§ion.id), request_id: format!("loader:{}", section.id), section_id: Some(section.id.clone()), method: request.method, @@ -430,6 +431,8 @@ impl FiberMcp { Ok(loader::LoaderResponse { status: response.status, body: response.body, + requested_url, + final_url: response.final_url, }) }) }) @@ -466,11 +469,7 @@ impl FiberMcp { .map_err(|err| McpError::internal_error(err, None))?; if !(200..300).contains(&response.status) { return Err(McpError::internal_error( - loader::LoaderError::Status { - status: response.status, - detail: loader::detail_from(&response.body), - } - .to_string(), + loader::rejected(&response).to_string(), None, )); } diff --git a/src/lib/components/Sidebar.svelte b/src/lib/components/Sidebar.svelte index 259c9dc..5823e58 100644 --- a/src/lib/components/Sidebar.svelte +++ b/src/lib/components/Sidebar.svelte @@ -1227,11 +1227,23 @@ {#if collections.loaderFailure} {@const failure = collections.loaderFailure}
-

+ +

{failure.sectionName} · {failure.message}

+ {#if failure.canSignIn}