From c89fd9f97aa907ee11cc4c9d21f19e97029f75a1 Mon Sep 17 00:00:00 2001 From: Mathias Picker <48158184+MathiasWP@users.noreply.github.com> Date: Wed, 30 Sep 2026 15:20:36 +0200 Subject: [PATCH] Refresh cached credentials when saved secrets change --- .changeset/fresh-mcp-credentials.md | 5 + CONTRIBUTING.md | 7 +- src-tauri/src/auth.rs | 180 ++++++++++++++++++++++--- src-tauri/src/mcp.rs | 9 +- src-tauri/src/secrets.rs | 78 +++++++++-- src-tauri/src/send.rs | 202 +++++++++++++++++++++++----- 6 files changed, 416 insertions(+), 65 deletions(-) create mode 100644 .changeset/fresh-mcp-credentials.md diff --git a/.changeset/fresh-mcp-credentials.md b/.changeset/fresh-mcp-credentials.md new file mode 100644 index 0000000..2341f2b --- /dev/null +++ b/.changeset/fresh-mcp-credentials.md @@ -0,0 +1,5 @@ +--- +"fiber": patch +--- + +Invalidate cached authentication when saved credentials change across Fiber processes, and refresh once for explicit token-expiry errors returned as HTTP 403. MCP sign-in advice now covers expired-token responses too. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 896a9cc..17add0a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -147,8 +147,11 @@ browser route. Once per app run, for a collection with credentials — not once per request. The keychain is read the first time a section's credential is needed and kept in -memory from then on, invalidated by a 401 or by the credential being replaced. A -browser session re-captured after a 401 stays in memory only: writing it back +memory from then on, invalidated by a 401, an explicit token-expiry 403, or a +saved credential change. Non-secret change markers let a running local MCP +server notice keychain writes and deletions made by Fiber without prompting on +every request. Credential files are checked by content before reusing a token. +A browser session re-captured after rejection stays in desktop memory only: writing it back needs authorization exactly as reading does, and a session that expires this often will be stale again long before the next launch, so the prompt bought nothing. Setting one up on purpose, through *Pick credential*, still writes. diff --git a/src-tauri/src/auth.rs b/src-tauri/src/auth.rs index b0be425..6a55d28 100644 --- a/src-tauri/src/auth.rs +++ b/src-tauri/src/auth.rs @@ -3,7 +3,7 @@ //! The point of this module is to kill "paste a fresh Bearer token every hour". //! A section describes how to *obtain* a token — usually by making a request — //! and the token is cached, injected, and re-fetched automatically when the API -//! answers 401. See §5 of the design doc. +//! rejects authentication (401 or an explicit token-expiry 403). //! //! Secrets are passed in by the caller rather than read here, so the logic is //! testable without touching the real keychain. @@ -131,8 +131,11 @@ pub enum AuthError { struct CachedToken { value: String, - /// `None` means it only expires when the API rejects it. + /// `None` means no time limit; credential changes still invalidate it. expires_at: Option, + revision: u64, + auth: AuthConfig, + base_url: String, } impl CachedToken { @@ -141,12 +144,37 @@ impl CachedToken { } } -#[derive(Default)] pub struct AuthState { tokens: Mutex>, + revisions_dir: std::path::PathBuf, + secrets_file: Option, +} + +impl Default for AuthState { + fn default() -> Self { + Self { + tokens: Mutex::default(), + revisions_dir: crate::secrets::revisions_dir(), + secrets_file: std::env::var_os("FIBER_SECRETS_FILE").map(Into::into), + } + } } impl AuthState { + #[cfg(test)] + pub(crate) fn with_secrets_file(path: std::path::PathBuf) -> Self { + Self { + secrets_file: Some(path), + ..Self::default() + } + } + + pub(crate) fn revision(&self, section: &Section) -> u64 { + section.auth.secret_ref().map_or(0, |reference| { + crate::secrets::revision(&self.revisions_dir, reference, self.secrets_file.as_deref()) + }) + } + /// Drops a section's token so the next send fetches a new one. pub fn invalidate(&self, section_id: &str) { // A poisoned lock is another thread's panic, not this cache's problem: @@ -157,24 +185,38 @@ impl AuthState { .remove(section_id); } - fn cached(&self, section_id: &str) -> Option { + fn cached(&self, section: &Section, revision: u64) -> Option { let tokens = self .tokens .lock() .unwrap_or_else(std::sync::PoisonError::into_inner); tokens - .get(section_id) - .filter(|token| token.valid()) + .get(§ion.id) + .filter(|token| { + token.valid() + && token.revision == revision + && token.auth == section.auth + && token.base_url == section.base_url + }) .map(|token| token.value.clone()) } - pub(crate) fn store(&self, section_id: &str, value: String, ttl_seconds: u64) { + pub(crate) fn store(&self, section: &Section, value: String, ttl_seconds: u64, revision: u64) { let expires_at = (ttl_seconds > 0).then(|| Instant::now() + Duration::from_secs(ttl_seconds)); self.tokens .lock() .unwrap_or_else(std::sync::PoisonError::into_inner) - .insert(section_id.to_string(), CachedToken { value, expires_at }); + .insert( + section.id.clone(), + CachedToken { + value, + expires_at, + revision, + auth: section.auth.clone(), + base_url: section.base_url.clone(), + }, + ); } } @@ -194,19 +236,21 @@ pub async fn header_for( where F: Fn(&str) -> Option, { + // Sample before reading the credential: a concurrent write must invalidate + // this entry on the next send, never bless an old value with a new revision. + let revision = state.revision(section); let fetch = || section.auth.secret_ref().and_then(lookup); match §ion.auth { AuthConfig::None => Ok(None), - // Cached like a login token, though nothing expires it but a 401 or the - // user replacing it — both of which already call `invalidate`. + // Cached until its source changes, including in another Fiber process. AuthConfig::Bearer { .. } => { - let token = match state.cached(§ion.id) { + let token = match state.cached(section, revision) { Some(token) => token, None => { let token = fetch().ok_or(AuthError::MissingSecret)?; - state.store(§ion.id, token.clone(), 0); + state.store(section, token.clone(), 0, revision); token } }; @@ -225,12 +269,12 @@ where ttl_seconds, .. } => { - let token = match state.cached(§ion.id) { + let token = match state.cached(section, revision) { Some(token) => token, None => { let body = fetch().ok_or(AuthError::MissingSecret)?; let token = log_in(http, section, method, url, &body, token_path).await?; - state.store(§ion.id, token.clone(), *ttl_seconds); + state.store(section, token.clone(), *ttl_seconds, revision); token } }; @@ -242,11 +286,11 @@ where // Re-capturing it needs a webview, which lives outside this module — // see `browser.rs`. Here we only compose what's already stored. AuthConfig::Browser { header, prefix, .. } => { - let captured = match state.cached(§ion.id) { + let captured = match state.cached(section, revision) { Some(value) => value, None => { let value = fetch().ok_or(AuthError::NotSignedIn)?; - state.store(§ion.id, value.clone(), 0); + state.store(section, value.clone(), 0, revision); value } }; @@ -613,6 +657,110 @@ mod tests { ); } + #[tokio::test] + async fn saved_browser_changes_reach_independent_caches_without_extra_keychain_reads() { + let dir = std::env::temp_dir().join(format!("fiber-revisions-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + let app = AuthState { + revisions_dir: dir.clone(), + ..Default::default() + }; + let mcp = AuthState { + revisions_dir: dir.clone(), + ..Default::default() + }; + let http = HttpState::default(); + let section = section( + AuthConfig::Browser { + login_url: "https://example.com/login".into(), + capture: CaptureKind::LocalStorage, + capture_key: "token".into(), + capture_path: String::new(), + header: "Authorization".into(), + prefix: "Bearer".into(), + ttl_seconds: 0, + secret_ref: "revision-test:auth".into(), + }, + "https://example.com", + ); + let stored = Mutex::new(Some("old-token".to_string())); + let reads = AtomicUsize::new(0); + let lookup = |_: &str| { + reads.fetch_add(1, Ordering::SeqCst); + stored.lock().unwrap().clone() + }; + for state in [&app, &mcp] { + for _ in 0..2 { + let header = header_for(state, &http, §ion, &lookup) + .await + .unwrap() + .unwrap(); + assert_eq!(header.value, "Bearer old-token"); + } + } + assert_eq!(reads.load(Ordering::SeqCst), 2); + crate::secrets::mark_changed(&dir, "unrelated:auth").unwrap(); + header_for(&mcp, &http, §ion, &lookup).await.unwrap(); + assert_eq!( + reads.load(Ordering::SeqCst), + 2, + "unrelated writes do not prompt" + ); + + *stored.lock().unwrap() = Some("new-token".into()); + crate::secrets::mark_changed(&dir, "revision-test:auth").unwrap(); + for state in [&app, &mcp] { + let header = header_for(state, &http, §ion, &lookup) + .await + .unwrap() + .unwrap(); + assert_eq!(header.value, "Bearer new-token"); + } + assert_eq!(reads.load(Ordering::SeqCst), 4); + + *stored.lock().unwrap() = None; + crate::secrets::mark_changed(&dir, "revision-test:auth").unwrap(); + for state in [&app, &mcp] { + assert!(matches!( + header_for(state, &http, §ion, &lookup).await, + Err(AuthError::NotSignedIn) + )); + } + std::fs::remove_dir_all(dir).unwrap(); + } + + #[tokio::test] + async fn changing_the_credential_reference_does_not_reuse_the_old_token() { + let state = AuthState::default(); + let http = HttpState::default(); + let mut section = section( + AuthConfig::Bearer { + secret_ref: "old".into(), + }, + "https://example.com", + ); + let lookup = |reference: &str| Some(reference.to_string()); + assert_eq!( + header_for(&state, &http, §ion, &lookup) + .await + .unwrap() + .unwrap() + .value, + "Bearer old" + ); + section.auth = AuthConfig::Bearer { + secret_ref: "new".into(), + }; + assert_eq!( + header_for(&state, &http, §ion, &lookup) + .await + .unwrap() + .unwrap() + .value, + "Bearer new" + ); + } + /// Serves `/login`, handing out a new token each time it's called, so tests /// can tell a cached token from a freshly fetched one. async fn login_server() -> (String, std::sync::Arc) { diff --git a/src-tauri/src/mcp.rs b/src-tauri/src/mcp.rs index 6f9d820..83126fc 100644 --- a/src-tauri/src/mcp.rs +++ b/src-tauri/src/mcp.rs @@ -1092,17 +1092,18 @@ impl FiberMcp { hints.push("The response could not be persisted, so query_response is unavailable."); log::warn!("could not record MCP response {id}: {err}"); } - // A 401 that survived the retry means re-authenticating did not help, + // An auth rejection that survived the retry means refreshing did not help, // and the reason depends on where the credential came from. The retry // re-reads it (`send::send_authenticated_streaming` invalidates first), // so a credential file that the app keeps current has already been // consulted — which is why the advice is no longer "restart the server". - if response.status == 401 && section.auth.secret_ref().is_some() { + if crate::send::authentication_rejected(&response) && section.auth.secret_ref().is_some() { let browser = matches!(section.auth, crate::auth::AuthConfig::Browser { .. }); hints.push(if browser { "Browser credentials cannot be re-captured headlessly. Sign in again in Fiber \ - — if this server reads a credential file the app keeps current, the next call \ - picks it up; otherwise re-export its secrets." + and capture the credential. Local servers pick up saved keychain changes on \ + the next call; servers using FIBER_SECRETS_FILE pick up file changes. \ + Servers using a FIBER_SECRETS snapshot need their secrets re-exported and a restart." } else { "Re-authenticating did not help, so the stored credential is being rejected. \ Check it in Section settings." diff --git a/src-tauri/src/secrets.rs b/src-tauri/src/secrets.rs index 5020617..ccf5bcd 100644 --- a/src-tauri/src/secrets.rs +++ b/src-tauri/src/secrets.rs @@ -41,9 +41,55 @@ fn entry(reference: &str) -> Result { pub fn set(reference: &str, value: &str) -> Result<(), SecretError> { entry(reference)?.set_password(value)?; + notify_changed(reference); Ok(()) } +/// Non-secret change markers let other Fiber processes drop cached keychain +/// values without reading the keychain (and prompting) on every request. +pub(crate) fn revisions_dir() -> std::path::PathBuf { + crate::mcp::app_data_dir().join("credential-revisions") +} + +fn revision_path(dir: &std::path::Path, reference: &str) -> std::path::PathBuf { + use base64::Engine as _; + dir.join(base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(reference)) +} + +pub(crate) fn mark_changed(dir: &std::path::Path, reference: &str) -> Result<(), String> { + // Random content, rather than mtime or size: rapid, same-length credential + // replacements must still be visible. This contains no credential material. + let revision = new_key()?; + std::fs::create_dir_all(dir).map_err(|err| err.to_string())?; + std::fs::write(revision_path(dir, reference), revision).map_err(|err| err.to_string()) +} + +fn notify_changed(reference: &str) { + if let Err(err) = mark_changed(&revisions_dir(), reference) { + log::warn!("could not publish credential change: {err}"); + } +} + +/// An in-memory fingerprint only; neither credentials nor their hashes are +/// written to the marker files. Read file contents to notice same-size writes +/// even on mounts with coarse timestamps. Environment snapshots never change. +pub(crate) fn revision( + dir: &std::path::Path, + reference: &str, + secrets_file: Option<&std::path::Path>, +) -> u64 { + use std::hash::{Hash, Hasher}; + let mut hash = std::collections::hash_map::DefaultHasher::new(); + std::fs::read(revision_path(dir, reference)) + .ok() + .hash(&mut hash); + if let Some(path) = secrets_file { + path.hash(&mut hash); + std::fs::read(path).ok().hash(&mut hash); + } + hash.finish() +} + /// Authenticated encryption for the credential file. /// /// The file lives in the mounted collections directory, which is the only @@ -194,14 +240,13 @@ fn env_result() -> &'static Result, String> { /// container held whatever was true when it started: the user signed in, the /// keychain got the new token, and the server went on presenting the expired /// one until someone re-exported the secrets and restarted the workload. A -/// mounted file changes under a running process, so re-reading it is half the -/// fix; the other half is `send::send_authenticated_streaming` dropping the -/// cached token on a 401, which is what sends anyone back here to look. +/// mounted file changes under a running process. `auth::header_for` checks its +/// content fingerprint before using a cached token, so changes reach the next send. /// /// Deliberately uncached, unlike the mtime+size stamp `mcp::all_sections` uses /// for collections. Two reasons. It is not hot: `auth::header_for` only reaches /// a lookup when `AuthState` has no live token, which is once per collection -/// per run plus each 401 — everything else is answered from memory. And a +/// per run, credential changes and authentication rejections. And a /// stamp would be *wrong* here in a way it is not for collections: one token /// replaced by another of the same length within a single mtime tick is the /// ordinary case for a refreshed JWT or session cookie, and a bind mount can @@ -211,15 +256,25 @@ fn file_secrets() -> Result, String> { let Some(path) = std::env::var_os("FIBER_SECRETS_FILE") else { return Ok(HashMap::new()); }; + read_file( + std::path::Path::new(&path), + std::env::var("FIBER_SECRETS_KEY").ok().as_deref(), + ) +} + +pub(crate) fn read_file( + path: &std::path::Path, + key: Option<&str>, +) -> Result, String> { // Absent is not an error. The app only writes the file once someone has set // a container up, and a server pointed at a path that isn't there yet // should say "no credentials", not refuse to start. - let raw = match std::fs::read_to_string(&path) { + let raw = match std::fs::read_to_string(path) { Ok(raw) => raw, Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(HashMap::new()), Err(err) => return Err(format!("could not read FIBER_SECRETS_FILE: {err}")), }; - decode_file(&raw, std::env::var("FIBER_SECRETS_KEY").ok().as_deref()) + decode_file(&raw, key) } /// The key is passed in rather than read here, so the format rules — including @@ -395,7 +450,10 @@ pub fn has(reference: &str) -> bool { pub fn delete(reference: &str) -> Result<(), SecretError> { match entry(reference)?.delete_credential() { - Ok(()) | Err(keyring::Error::NoEntry) => Ok(()), + Ok(()) | Err(keyring::Error::NoEntry) => { + notify_changed(reference); + Ok(()) + } Err(err) => Err(err.into()), } } @@ -503,11 +561,7 @@ mod tests { fn a_missing_secrets_file_is_not_an_error() { let path = std::env::temp_dir().join("fiber-secrets-absent.json"); let _ = std::fs::remove_file(&path); - // SAFETY: single-threaded test setup; nothing else reads this variable - // until `file_secrets` does, on the next line. - unsafe { std::env::set_var("FIBER_SECRETS_FILE", &path) }; - let found = file_secrets(); - unsafe { std::env::remove_var("FIBER_SECRETS_FILE") }; + let found = read_file(&path, None); assert!(found.unwrap().is_empty()); } } diff --git a/src-tauri/src/send.rs b/src-tauri/src/send.rs index 13f560b..cb1f1a5 100644 --- a/src-tauri/src/send.rs +++ b/src-tauri/src/send.rs @@ -1,4 +1,4 @@ -//! Authenticated send, with the 401 refresh-and-retry built in. +//! Authenticated send, with refresh-and-retry for rejected credentials. //! //! Split out of the Tauri command layer so it stays free of Tauri types: both //! the app and the headless MCP server drive the same logic, and the headless @@ -16,6 +16,55 @@ use crate::auth::{self, AuthConfig, AuthState}; use crate::http::{self, ChunkSink, HttpError, HttpState, RequestSpec, ResponseData}; use crate::store::Section; +/// A forbidden response normally means insufficient permissions. Only explicit +/// token-expiry errors share the 401 refresh path (some APIs, including Kvist, +/// report expired JWTs as 403). Keep this shared with MCP's sign-in advice. +pub(crate) fn authentication_rejected(response: &ResponseData) -> bool { + if response.status == 401 { + return true; + } + if response.status != 403 { + return false; + } + fn expired(message: &str) -> bool { + matches!( + message + .trim() + .trim_end_matches('.') + .to_ascii_lowercase() + .as_str(), + "invalid token: jwt expired" + | "jwt expired" + | "token expired" + | "token has expired" + | "access token expired" + | "access token has expired" + | "token_expired" + | "expired_token" + | "tokenexpirederror" + | "jwtexpired" + ) + } + fn error_value(value: &serde_json::Value) -> bool { + match value { + serde_json::Value::String(message) => expired(message), + serde_json::Value::Object(fields) => [ + "error", + "message", + "error_description", + "detail", + "code", + "name", + ] + .iter() + .any(|key| fields.get(*key).is_some_and(error_value)), + _ => false, + } + } + expired(&response.body) + || serde_json::from_str(&response.body).is_ok_and(|value| error_value(&value)) +} + /// Lifts a fresh browser-captured credential, for the one auth kind a replayed /// request can't refresh. Implemented by the GUI over a hidden webview; absent /// under the MCP server, where a browser-captured credential can be used but not @@ -28,7 +77,7 @@ pub(crate) trait Recapturer: Send + Sync { } /// Applies the section's auth, and — the whole point of auth-as-a-request — -/// treats a 401 as "the cached token aged out": drop it, log in again, retry +/// treats a 401 or explicit 403 token-expiry error as a stale token: log in and retry /// once. Exactly once, so a genuinely unauthorised request can't loop. /// /// `lookup` resolves a keychain (or injected) reference. Injected so the retry @@ -79,19 +128,11 @@ where let prepared = apply_auth(http_state, auth_state, section, spec, lookup).await?; let first = http::send_streaming(http_state, prepared, sink).await; - let rejected = matches!(&first, Ok(response) if response.status == 401); - - // A static token cannot be refreshed by replaying anything, so there is no - // retry to make — but where the credential comes from a source that changes - // underneath the process, the cached copy is still worth dropping so the - // *next* send reads the new one. That is a containerised server whose - // credential file the app has just rewritten: without this, a bearer - // collection would present the token it started with for the life of the - // workload, because nothing else ever expires a zero-TTL entry. - // - // Conditioned on there being such a source, because in the desktop app the - // same line would buy nothing and cost a keychain prompt per 401 — see - // `auth::header_for` on why reads are lazy. + let rejected = matches!(&first, Ok(response) if authentication_rejected(response)); + + // Static bearer tokens cannot be refreshed automatically. Drop rejected + // injected values for the next send as well; local keychain values stay + // cached until their change marker moves, avoiding repeated prompts. if rejected && retry_spec.is_none() && crate::secrets::has_injected_source() { auth_state.invalidate(§ion.id); } @@ -102,7 +143,10 @@ where } let spec = retry_spec.expect("should_retry implies can_refresh"); - log::info!("401 from {}, re-authenticating and retrying once", spec.url); + log::info!( + "authentication rejected by {}, re-authenticating and retrying once", + spec.url + ); auth_state.invalidate(§ion.id); // A browser-captured credential can't be re-fetched by replaying a request, @@ -110,6 +154,7 @@ where // the identity provider's own session is still alive this is invisible. The // MCP server passes no recapturer, so this branch is simply skipped there. if let (AuthConfig::Browser { .. }, Some(recapture)) = (§ion.auth, recapture) { + let revision = auth_state.revision(section); match recapture.recapture(section).await { Ok(value) => { // Kept in memory, deliberately not written back to the keychain. @@ -130,10 +175,10 @@ where // server reads, which is not the keychain and costs no prompt, // so the recapturer mirrors the new value into it before // returning. See `browser::BrowserRecapture`. - auth_state.store(§ion.id, value, 0); + auth_state.store(section, value, 0, revision); } // The window is now visible for the user to sign in; the original - // 401 is the honest answer for this request. + // rejection is the honest answer for this request. Err(err) => { log::info!("silent re-capture failed: {err}"); return first; @@ -141,11 +186,15 @@ where } } - match apply_auth(http_state, auth_state, section, spec, lookup).await { + let result = match apply_auth(http_state, auth_state, section, spec, lookup).await { Ok(retry) => http::send_streaming(http_state, retry, sink).await, - // Re-authentication failed, so the original 401 is the honest answer. + // Re-authentication failed, so return the original rejection. Err(_) => first, + }; + if matches!(&result, Ok(response) if authentication_rejected(response)) { + auth_state.invalidate(§ion.id); } + result } async fn apply_auth( @@ -377,6 +426,14 @@ mod tests { /// credential is not refreshed by replaying a request, it is *replaced on /// disk* by the desktop app when someone signs in again. async fn fixed_token_api(accepted: &'static str) -> (String, Arc) { + rejecting_token_api(accepted, "401 Unauthorized", "").await + } + + async fn rejecting_token_api( + accepted: &'static str, + rejected_status: &'static str, + body: &'static str, + ) -> (String, Arc) { let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); let addr = listener.local_addr().unwrap(); let calls = Arc::new(Calls { @@ -397,10 +454,15 @@ mod tests { let request = String::from_utf8_lossy(&buf[..read]).to_string(); counters.protected.fetch_add(1, Ordering::SeqCst); let ok = request.contains(&format!("Bearer {accepted}")); - let status = if ok { "200 OK" } else { "401 Unauthorized" }; + let status = if ok { "200 OK" } else { rejected_status }; + let body = if ok { "" } else { body }; let _ = socket .write_all( - format!("HTTP/1.1 {status}\r\nContent-Length: 0\r\n\r\n").as_bytes(), + format!( + "HTTP/1.1 {status}\r\nContent-Length: {}\r\n\r\n{body}", + body.len() + ) + .as_bytes(), ) .await; let _ = socket.flush().await; @@ -428,10 +490,6 @@ mod tests { let path = dir.join("secrets.json"); std::fs::write(&path, r#"{"sec-1:auth":"old-token"}"#).unwrap(); - // SAFETY: no other test reads this variable, and the reads it causes all - // happen on this task before it is removed at the end. - unsafe { std::env::set_var("FIBER_SECRETS_FILE", &path) }; - let section = Section { id: "sec-1".into(), name: "Test".into(), @@ -442,7 +500,13 @@ mod tests { ..Default::default() }; let http_state = HttpState::default(); - let auth_state = AuthState::default(); + let auth_state = AuthState::with_secrets_file(path.clone()); + let lookup = |reference: &str| { + crate::secrets::read_file(&path, None) + .unwrap() + .get(reference) + .cloned() + }; let send = || { send_authenticated( @@ -450,26 +514,102 @@ mod tests { &auth_state, Some(§ion), spec_for(&base), - &crate::secrets::get, + &lookup, None, ) }; - // The token the workload started with. Rejected, and now also dropped - // from the cache, which is the half that used to be missing. + // The token the workload started with is rejected. assert_eq!(send().await.unwrap().status, 401); + // Repopulate the cache before the disk write: the change must be + // noticed proactively, without needing another rejection first. + apply_auth(&http_state, &auth_state, §ion, spec_for(&base), &lookup) + .await + .unwrap(); + // Signing in again in Fiber. Same length, as a refreshed token usually // is — an mtime+size stamp would not have noticed this. std::fs::write(&path, r#"{"sec-1:auth":"new-token"}"#).unwrap(); let after = send().await.unwrap().status; - unsafe { std::env::remove_var("FIBER_SECRETS_FILE") }; let _ = std::fs::remove_dir_all(&dir); assert_eq!(after, 200, "the next send should use the rewritten token"); } + #[tokio::test] + async fn browser_expiry_on_403_refreshes_once_but_permission_denials_do_not() { + for (body, expired) in [ + ("Invalid token: jwt expired", true), + (r#"{"message":"Invalid token: jwt expired"}"#, true), + (r#"{"error":{"code":"token_expired"}}"#, true), + (r#"{"message":"Insufficient permissions"}"#, false), + (r#"{"error":"invalid_token"}"#, false), + (r#"{"data":"jwt expired","message":"Forbidden"}"#, false), + ] { + for refreshed in [true, false] { + let (base, calls) = rejecting_token_api("new-token", "403 Forbidden", body).await; + let http = HttpState::default(); + let auth = AuthState::default(); + let mut section = section_with_login(&base); + section.auth = AuthConfig::Browser { + login_url: format!("{base}/login"), + capture: crate::auth::CaptureKind::LocalStorage, + capture_key: "token".into(), + capture_path: String::new(), + header: "Authorization".into(), + prefix: "Bearer".into(), + ttl_seconds: 0, + secret_ref: "expiry-test:auth".into(), + }; + let reads = AtomicUsize::new(0); + let lookup = |_: &str| { + let count = reads.fetch_add(1, Ordering::SeqCst); + Some( + if refreshed && count > 0 { + "new-token" + } else { + "old-token" + } + .into(), + ) + }; + let response = send_authenticated( + &http, + &auth, + Some(§ion), + spec_for(&base), + &lookup, + None, + ) + .await + .unwrap(); + assert_eq!( + response.status, + if expired && refreshed { 200 } else { 403 }, + "{body}" + ); + assert_eq!( + calls.protected.load(Ordering::SeqCst), + if expired { 2 } else { 1 }, + "exactly one retry only for expiry: {body}" + ); + assert_eq!(reads.load(Ordering::SeqCst), if expired { 2 } else { 1 }); + if expired && !refreshed { + auth::header_for(&auth, &http, §ion, &lookup) + .await + .unwrap(); + assert_eq!( + reads.load(Ordering::SeqCst), + 3, + "a rejected retry must not stay cached" + ); + } + } + } + } + /// A genuine 401 must not loop: exactly one retry, then give up. #[tokio::test] async fn retries_at_most_once() {