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/.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/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 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}