From f70bf89006e0d962629fd9a6651459704ea45afb Mon Sep 17 00:00:00 2001 From: Kayshen-X Date: Sat, 8 Aug 2026 16:29:41 +0800 Subject: [PATCH] fix(web): unblock session node creation, isolate account switches, persist online tenants MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review blockers: - the session push gate refused every id absent from the daemon snapshot, which included all ids the namespace allocator mints — so nodes created during a session never synced and were erased by the next pull. Unknown ids now pass when they parse into this peer's namespace (parsed comparison, not prefix match); with no allocator the old fail-closed behaviour stands - a same-tab account switch kept the previous account's document, sync baseline, and stored credentials. An identity epoch keyed on the auth subject (tri-state, so sign-out then sign-in is not mistaken for first contact) resets the sync client, gate, document, collab state and latches; 401/403 suppress pushes until the reset; browser storage partitions by subject with legacy keys deliberately not adopted - online tenants lived only in memory under the default deploy: compose now mounts a data volume, startup fails closed when eviction is enabled without a store (OPENPENCIL_ONLINE_EPHEMERAL=1 is the loud demo bypass), a dedicated sweeper replaces the connection-triggered sweep that never ran while idle, and shutdown flushes every resident tenant --- Cargo.lock | 1 + .../src/web_canvas_server/online_run_loop.rs | 136 ++++++++- .../web_canvas_server/online_share_tests.rs | 92 ++++++ .../src/web_canvas_server/tenant.rs | 29 ++ crates/op-host-web/Cargo.toml | 4 + crates/op-host-web/src/collab_sync.rs | 110 ++++---- .../op-host-web/src/collab_sync_push_gate.rs | 179 ++++++++++++ crates/op-host-web/src/identity_epoch.rs | 265 ++++++++++++++++++ crates/op-host-web/src/lib.rs | 4 + crates/op-host-web/src/live_sync_conflict.rs | 20 ++ .../op-host-web/src/live_sync_controller.rs | 87 ++++++ crates/op-host-web/src/live_sync_glue.rs | 125 +++------ crates/op-host-web/src/live_sync_identity.rs | 67 +++++ crates/op-host-web/src/web_auth_sync.rs | 7 + crates/op-host-web/src/web_settings.rs | 25 ++ .../src/web_settings_acp_scrub_tests.rs | 34 +-- .../src/web_settings_lossless_tests.rs | 4 +- .../op-host-web/src/web_settings_storage.rs | 32 ++- crates/op-host-web/src/web_settings_tests.rs | 12 +- 19 files changed, 1038 insertions(+), 195 deletions(-) create mode 100644 crates/op-host-web/src/collab_sync_push_gate.rs create mode 100644 crates/op-host-web/src/identity_epoch.rs create mode 100644 crates/op-host-web/src/live_sync_controller.rs create mode 100644 crates/op-host-web/src/live_sync_identity.rs diff --git a/Cargo.lock b/Cargo.lock index 62ceae40c..a33e7df65 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4184,6 +4184,7 @@ dependencies = [ "op-figma", "op-html", "op-pen-loader", + "op-util", "serde", "serde_json", "ttf-parser", diff --git a/crates/op-host-services/src/web_canvas_server/online_run_loop.rs b/crates/op-host-services/src/web_canvas_server/online_run_loop.rs index b5c1bac53..4e59b0c66 100644 --- a/crates/op-host-services/src/web_canvas_server/online_run_loop.rs +++ b/crates/op-host-services/src/web_canvas_server/online_run_loop.rs @@ -39,13 +39,11 @@ use super::tenant::{now_unix, TenantLimits, TenantRegistry}; use super::tenant_auth::{IdentityVerifier, PresentedCredentials, StaticVerifier}; use super::*; -/// How often the accept loop sweeps for idle tenants. -/// -/// Eviction is cheap (a map scan under one lock) and the deadline it enforces -/// is measured in minutes, so a sweep tied to connection arrivals would be -/// both too eager under load and never under idle. This is the floor between -/// sweeps, checked as connections arrive. -const EVICT_SWEEP_INTERVAL_SECS: u64 = 60; +/// Longest gap between idle sweeps, however long the idle deadline is. +const MAX_SWEEP_INTERVAL_SECS: u64 = 300; + +/// Opt-in to running with no persistence at all. Demo use only. +pub const EPHEMERAL_ENV: &str = "OPENPENCIL_ONLINE_EPHEMERAL"; /// Run the multi-account web-canvas daemon. /// @@ -63,6 +61,23 @@ pub fn run_online_web_canvas(options: ServeWebOptions) -> Result<()> { ); } let limits = TenantLimits::from_env(); + let store = super::tenant_store::TenantStore::from_env(); + // Eviction without persistence silently destroys documents: a tenant goes + // idle, is reclaimed to free memory, and the account's work is simply + // gone. That is defensible for a demo and indefensible for a deployment, + // and the two are indistinguishable from inside the process — so the + // operator has to say which one this is. + check_persistence_configured( + store.is_enabled(), + ephemeral_opt_in(), + limits.idle_evict_secs, + )?; + if !store.is_enabled() { + eprintln!( + "openpencil --serve-web --online: {EPHEMERAL_ENV} is set — evicted accounts lose \ + their documents. Never use this for a deployment." + ); + } let allow_origins = online_policy::allowed_origins_from_env(); if allow_origins.is_empty() { // Not fatal: a same-origin deployment behind a reverse proxy needs no @@ -100,10 +115,15 @@ pub fn run_online_web_canvas(options: ServeWebOptions) -> Result<()> { ), } - let registry = Arc::new(TenantRegistry::new(bound, limits, allow_origins)); + let registry = Arc::new(TenantRegistry::with_store( + bound, + limits, + allow_origins, + store, + )); let conn_count = Arc::new(AtomicUsize::new(0)); let shutdown = Arc::new(AtomicBool::new(false)); - let mut last_sweep = now_unix(); + spawn_sweeper(®istry, &shutdown); for stream in listener.incoming() { if shutdown.load(Ordering::Acquire) { @@ -116,11 +136,6 @@ pub fn run_online_web_canvas(options: ServeWebOptions) -> Result<()> { continue; } }; - let now = now_unix(); - if now.saturating_sub(last_sweep) >= EVICT_SWEEP_INTERVAL_SECS { - last_sweep = now; - registry.evict_idle(now); - } if conn_count.load(Ordering::Acquire) >= limits.max_conns { let _ = s.set_write_timeout(Some(IO_TIMEOUT)); let _ = crate::mcp_serve::write_mcp_http_response( @@ -154,10 +169,101 @@ pub fn run_online_web_canvas(options: ServeWebOptions) -> Result<()> { conn_count.fetch_sub(1, Ordering::AcqRel); } } - eprintln!("openpencil --serve-web --online: shutdown requested; exiting"); + // The sweeper observes the same flag and retires on its next wake. + shutdown.store(true, Ordering::Release); + let flushed = registry.flush_all(); + eprintln!( + "openpencil --serve-web --online: shutdown requested; flushed {flushed} account(s); \ + exiting" + ); Ok(()) } +/// Refuse to start a deployment that evicts accounts but keeps nothing. +/// +/// Pure so the decision is testable without a socket or the environment. The +/// two states this separates — "a demo that intentionally forgets" and "a +/// deployment whose data directory was left unset" — look identical from +/// inside the process, and only one of them is acceptable. So the operator +/// has to say which, rather than the daemon guessing and silently destroying +/// documents on the first idle timer. +pub(super) fn check_persistence_configured( + store_enabled: bool, + ephemeral: bool, + idle_evict_secs: u64, +) -> Result<()> { + if store_enabled || ephemeral { + return Ok(()); + } + Err(WebCanvasError::Config(format!( + "--online evicts idle accounts after {idle_evict_secs}s but no {} is configured, so \ + their documents would be discarded. Set {} to persist them, or set {EPHEMERAL_ENV}=1 \ + to accept that this deployment keeps nothing.", + super::tenant_store::DATA_DIR_ENV, + super::tenant_store::DATA_DIR_ENV, + ))) +} + +/// Whether the operator accepted a deployment that persists nothing. +fn ephemeral_opt_in() -> bool { + std::env::var(EPHEMERAL_ENV) + .ok() + .map(|value| value.trim().to_string()) + .is_some_and(|value| matches!(value.as_str(), "1" | "true" | "yes" | "on")) +} + +/// Run the idle sweep on its own clock. +/// +/// Eviction used to piggyback on connection arrivals, which meant the one +/// state it exists for — a daemon nobody is talking to — was exactly the state +/// it never ran in: idle accounts stayed resident indefinitely and were never +/// written to disk. This thread observes `shutdown` on every wake, so it +/// retires within one interval of the daemon being asked to stop. +fn spawn_sweeper(registry: &Arc, shutdown: &Arc) { + let registry = Arc::clone(registry); + let shutdown = Arc::clone(shutdown); + let interval = sweep_interval_secs(registry.limits().idle_evict_secs); + let spawned = thread::Builder::new() + .name("op-serve-web-online-sweeper".into()) + .spawn(move || { + while !shutdown.load(Ordering::Acquire) { + std::thread::sleep(std::time::Duration::from_secs(interval)); + if shutdown.load(Ordering::Acquire) { + return; + } + let evicted = registry.evict_idle(now_unix()); + if evicted > 0 { + eprintln!( + "openpencil --serve-web --online: reclaimed {evicted} idle account(s)" + ); + } + } + }); + if spawned.is_err() { + eprintln!( + "openpencil --serve-web --online: could not start the idle sweeper; accounts will \ + stay resident until a connection triggers a sweep" + ); + } +} + +/// How often to sweep, given the idle deadline. +/// +/// A quarter of the deadline bounds how long past its timer a tenant can +/// linger, and the ceiling keeps a very long deadline from meaning the daemon +/// effectively never sweeps. The floor keeps a short test deadline from +/// spinning the thread. +pub(super) const fn sweep_interval_secs(idle_evict_secs: u64) -> u64 { + let quarter = idle_evict_secs / 4; + if quarter < 1 { + 1 + } else if quarter > MAX_SWEEP_INTERVAL_SECS { + MAX_SWEEP_INTERVAL_SECS + } else { + quarter + } +} + /// Pick the identity verifier this deployment runs. /// /// The hub is the production answer. `StaticVerifier` stays reachable so the diff --git a/crates/op-host-services/src/web_canvas_server/online_share_tests.rs b/crates/op-host-services/src/web_canvas_server/online_share_tests.rs index 783b6a9ec..d677f27dd 100644 --- a/crates/op-host-services/src/web_canvas_server/online_share_tests.rs +++ b/crates/op-host-services/src/web_canvas_server/online_share_tests.rs @@ -407,3 +407,95 @@ fn a_tenant_that_cannot_be_written_stays_resident_rather_than_losing_its_documen let _ = std::fs::remove_file(&temp.root); } + +// --------------------------------------------------------------------------- +// The persistence lifecycle: sweep cadence, shutdown flush, fail-closed start. +// --------------------------------------------------------------------------- + +#[test] +fn the_sweep_interval_tracks_the_idle_deadline_within_bounds() { + use crate::web_canvas_server::online_run_loop::sweep_interval_secs; + // A quarter of the deadline bounds how long past its timer a tenant can + // linger — but the production default's quarter (450 s) exceeds the + // ceiling, so it clamps. + assert_eq!(sweep_interval_secs(1800), 300); + assert_eq!(sweep_interval_secs(400), 100); + // …with a ceiling, so a very long deadline still sweeps regularly… + assert_eq!(sweep_interval_secs(86_400), 300); + // …and a floor, so a short test deadline does not spin the thread. + for tiny in [0, 1, 2, 3] { + assert_eq!(sweep_interval_secs(tiny), 1, "{tiny}"); + } +} + +#[test] +fn an_idle_account_is_reclaimed_without_any_new_connection() { + // The regression: eviction used to run only when a connection arrived, so + // the one state it exists for — an idle daemon — was the state it never + // ran in, and nothing was ever written to disk. + let temp = PersistentRegistry::new("no-traffic"); + serve( + &temp.registry, + &verifier(), + Request::json("POST", "/api/mcp/document", SYNC_BODY).with_bearer("tokA"), + ); + assert_eq!(temp.registry.tenant_count(), 1); + + // No further requests — exactly what the sweeper thread calls. + assert_eq!(temp.registry.evict_idle(now_unix() + 3600), 1); + assert_eq!(temp.registry.tenant_count(), 0); + assert!(temp.registry.store().has_document("userA")); +} + +#[test] +fn a_controlled_shutdown_flushes_every_resident_account() { + // Without this, every account active at the moment of a deploy loses + // whatever had not happened to be evicted. + let temp = PersistentRegistry::new("flush"); + let verifier = verifier(); + for token in ["tokA", "tokB"] { + serve( + &temp.registry, + &verifier, + Request::json("POST", "/api/mcp/document", SYNC_BODY).with_bearer(token), + ); + } + assert_eq!(temp.registry.tenant_count(), 2); + assert!(!temp.registry.store().has_document("userA")); + + assert_eq!(temp.registry.flush_all(), 2); + assert!(temp.registry.store().has_document("userA")); + assert!(temp.registry.store().has_document("userB")); + // Flushing does not evict: requests still draining must keep working. + assert_eq!(temp.registry.tenant_count(), 2); +} + +#[test] +fn flushing_a_deployment_that_persists_nothing_is_a_no_op() { + let registry = registry(); + serve( + ®istry, + &verifier(), + Request::json("POST", "/api/mcp/document", SYNC_BODY).with_bearer("tokA"), + ); + assert_eq!(registry.flush_all(), 0); +} + +#[test] +fn a_deployment_that_evicts_but_persists_nothing_refuses_to_start() { + use crate::web_canvas_server::online_run_loop::check_persistence_configured; + + // The dangerous default: eviction on, data directory unset. Starting here + // means every idle account's document is destroyed on its first timer. + let refused = check_persistence_configured(false, false, 1800).unwrap_err(); + let message = refused.to_string(); + assert!(message.contains("OPENPENCIL_ONLINE_DATA_DIR"), "{message}"); + assert!(message.contains("OPENPENCIL_ONLINE_EPHEMERAL"), "{message}"); + + // Configured persistence is the normal deployment. + assert!(check_persistence_configured(true, false, 1800).is_ok()); + // An explicit opt-in is the demo, and says so. + assert!(check_persistence_configured(false, true, 1800).is_ok()); + // Both is fine — the data directory simply wins. + assert!(check_persistence_configured(true, true, 1800).is_ok()); +} diff --git a/crates/op-host-services/src/web_canvas_server/tenant.rs b/crates/op-host-services/src/web_canvas_server/tenant.rs index 1c89b01c3..aa654cfe2 100644 --- a/crates/op-host-services/src/web_canvas_server/tenant.rs +++ b/crates/op-host-services/src/web_canvas_server/tenant.rs @@ -459,6 +459,35 @@ impl TenantRegistry { } } + /// Write every resident tenant to disk. + /// + /// Called on controlled shutdown. Eviction is the only other writer, and + /// a daemon that is asked to stop has by definition not waited out anyone's + /// idle timer — so without this, every account that was active at the + /// moment of a deploy loses whatever it had not had evicted. + /// + /// Returns how many were written. Tenants are NOT removed: the process is + /// going away regardless, and removing them would only race the requests + /// still draining. + pub fn flush_all(&self) -> usize { + if !self.store.is_enabled() { + return 0; + } + let tenants = self.lock(); + let mut written = 0; + for (id, tenant) in tenants.iter() { + let guard = tenant.state.lock().unwrap_or_else(|p| p.into_inner()); + match self.store.save(id, &guard.editor, &tenant.shared_with()) { + Ok(()) => written += 1, + Err(error) => eprintln!( + "openpencil --serve-web --online: could not flush a tenant on shutdown \ + ({error})" + ), + } + } + written + } + /// Persist a tenant's access list, if persistence is on. pub fn persist_acl(&self, user_id: &str, tenant: &Tenant) { if !self.store.is_enabled() { diff --git a/crates/op-host-web/Cargo.toml b/crates/op-host-web/Cargo.toml index e6c78c14b..61eb8fcbb 100644 --- a/crates/op-host-web/Cargo.toml +++ b/crates/op-host-web/Cargo.toml @@ -29,6 +29,10 @@ op-editor-ui = { path = "../op-editor-ui" } # compiled through the unconditional op-editor-ui dep, and the baseline # `daemon_base` module reads its shared MCP-port/daemon-origin consts. op-editor-core = { path = "../op-editor-core" } + +# The canonical collaboration id grammar. Dependency-free and wasm32-clean — +# the push gate parses namespaced ids with it rather than matching prefixes. +op-util = { path = "../op-util" } # These stay optional and are pulled only by `canvaskit`, so the wasm32-clean # stub baseline (`--no-default-features --features web`) never compiles the # full editor pipeline. `op-pen-loader` disables `skia-measure` below; the web diff --git a/crates/op-host-web/src/collab_sync.rs b/crates/op-host-web/src/collab_sync.rs index 0d9bf3d5c..20454ed43 100644 --- a/crates/op-host-web/src/collab_sync.rs +++ b/crates/op-host-web/src/collab_sync.rs @@ -75,7 +75,16 @@ thread_local! { /// /// The browser mints ids from a local sequential counter, which is exactly /// what an active session cannot accept — see [`push_blocked_by_session`]. - static DAEMON_NODE_IDS: RefCell>> = + pub(super) static DAEMON_NODE_IDS: RefCell>> = + const { RefCell::new(None) }; + + /// The namespace this peer is currently minting ids under, when the + /// owner-assigned allocator is enabled. + /// + /// Set by `sync_id_allocation` at the moment the allocator is installed + /// and cleared the moment it is taken away, so it is exactly "the ids + /// this peer is entitled to invent" — see `push_blocked_by_session`. + pub(super) static SESSION_NAMESPACE: RefCell> = const { RefCell::new(None) }; } @@ -95,42 +104,11 @@ pub(crate) fn note_daemon_document(state: &op_editor_core::EditorState) { DAEMON_NODE_IDS.with(|ids| *ids.borrow_mut() = Some(document_node_ids(state))); } -/// Whether a live session forbids pushing the current local document. -/// -/// The browser has no owner-assigned id namespace: it mints `n` from a -/// local sequential allocator, and two peers creating a node in the same moment -/// would mint the same id. The collaboration protocol replays those ids -/// verbatim, so a colliding pair silently forks the document — the failure this -/// refuses to produce. -/// -/// The check is deliberately whole-document rather than per-gesture: draw, -/// duplicate, paste, group and import all mint through the same counter, and -/// gating the single push covers every one of them without a guard at each -/// call site. Any id the daemon has not seen blocks the push; edits to existing -/// nodes (move, restyle, delete) carry no new ids and go through untouched. -/// -/// The local node stays on screen until the next pull replaces it with the -/// daemon's document, so the divergence is bounded and self-healing. -pub(crate) fn push_blocked_by_session(state: &op_editor_core::EditorState) -> bool { - if state.editor_ui.collab.phase != CollabConnectionPhase::Active { - return false; - } - DAEMON_NODE_IDS.with(|known| { - let known = known.borrow(); - let Some(known) = known.as_ref() else { - // No daemon document seen yet in this session; refuse rather than - // guess, since the pull that would settle it is one tick away. - return true; - }; - document_node_ids(state) - .iter() - .any(|id| !known.contains(id)) - }) -} - /// Node ids in a document, through `op-editor-core`'s own walker so pages and /// nodes share the one collision domain the allocator uses. -fn document_node_ids(state: &op_editor_core::EditorState) -> HashSet { +pub(super) fn document_node_ids( + state: &op_editor_core::EditorState, +) -> HashSet { op_editor_core::collect_document_ids(&state.doc) } @@ -363,15 +341,21 @@ fn sync_id_allocation(host: &mut crate::widget_host::WidgetHost, wire: &CollabSt Some(namespace) if !host.collaboration_ids_enabled() => { match op_editor_core::PeerNamespace::parse(namespace) { Ok(namespace) => { + let enabled = namespace.clone(); if let Err(error) = host.enable_collaboration_ids(namespace) { // The document already carries ids this namespace // cannot resume above. Staying on the standalone // counter keeps the canvas usable; the push gate is // what keeps those ids off the wire. let _ = error; + } else { + set_session_namespace(Some(enabled)); } } - Err(_) => host.disable_collaboration_ids(), + Err(_) => { + host.disable_collaboration_ids(); + set_session_namespace(None); + } } } Some(_) => {} @@ -379,10 +363,28 @@ fn sync_id_allocation(host: &mut crate::widget_host::WidgetHost, wire: &CollabSt if host.collaboration_ids_enabled() { host.disable_collaboration_ids(); } + set_session_namespace(None); } } } +/// Forget everything scoped to the previous account. +/// +/// The daemon snapshot and the minting namespace both belong to the session +/// the old account was in; carrying them into a new account's tab would let +/// its push gate answer from another account's document. +pub(crate) fn reset_for_new_identity() { + DAEMON_NODE_IDS.with(|ids| *ids.borrow_mut() = None); + SESSION_NAMESPACE.with(|slot| *slot.borrow_mut() = None); + APPLIED_SEQ.set(None); + SESSION_LIVE.set(false); +} + +/// Record (or clear) the namespace this peer mints under. +fn set_session_namespace(namespace: Option) { + SESSION_NAMESPACE.with(|slot| *slot.borrow_mut() = namespace); +} + fn maybe_push_presence(inner: &Rc>, base: &str) { if !SESSION_LIVE.get() { return; @@ -428,7 +430,7 @@ fn now_ms() -> u64 { mod tests { use super::*; - fn doc_with(ids: &[&str]) -> op_editor_core::EditorState { + pub(super) fn doc_with(ids: &[&str]) -> op_editor_core::EditorState { let children: Vec = ids .iter() .map(|id| { @@ -447,8 +449,13 @@ mod tests { state } - fn reset_latches() { + pub(super) fn reset_latches() { DAEMON_NODE_IDS.with(|ids| *ids.borrow_mut() = None); + SESSION_NAMESPACE.with(|slot| *slot.borrow_mut() = None); + } + + pub(super) fn namespace(value: &str) -> op_editor_core::PeerNamespace { + op_editor_core::PeerNamespace::parse(value.to_string()).expect("valid namespace") } #[test] @@ -466,29 +473,6 @@ mod tests { ); } - #[test] - fn an_active_session_blocks_a_push_that_invents_a_node_id() { - reset_latches(); - let mut state = doc_with(&["n100"]); - note_daemon_document(&state); - state - .editor_ui - .collab - .set_phase(CollabConnectionPhase::Active); - - // Editing what the daemon already knows is fine. - assert!(!push_blocked_by_session(&state)); - - // Minting a new local id is not: the browser has no owner-assigned - // namespace, so this id could collide with a peer's. - let mut grown = doc_with(&["n100", "n101"]); - grown - .editor_ui - .collab - .set_phase(CollabConnectionPhase::Active); - assert!(push_blocked_by_session(&grown)); - } - #[test] fn deleting_a_node_during_a_session_is_not_blocked() { reset_latches(); @@ -690,3 +674,7 @@ fn note_stream_failure() { )); } } + +#[path = "collab_sync_push_gate.rs"] +mod push_gate; +pub(crate) use push_gate::push_blocked_by_session; diff --git a/crates/op-host-web/src/collab_sync_push_gate.rs b/crates/op-host-web/src/collab_sync_push_gate.rs new file mode 100644 index 000000000..91989fc92 --- /dev/null +++ b/crates/op-host-web/src/collab_sync_push_gate.rs @@ -0,0 +1,179 @@ +//! The session push gate: which local document may reach the daemon. +//! +//! Split out of `collab_sync.rs` at the 800-line cap. It is one decision, and +//! the decision is load-bearing: too strict and every node created during a +//! session is stuck forever (and deleted by the next pull); too loose and this +//! peer forks the shared document with ids another peer may also mint. + +use op_editor_core::CollabConnectionPhase; + +use super::{document_node_ids, DAEMON_NODE_IDS, SESSION_NAMESPACE}; + +/// Whether a live session forbids pushing the current local document. +/// +/// The browser has no owner-assigned id namespace: it mints `n` from a +/// local sequential allocator, and two peers creating a node in the same moment +/// would mint the same id. The collaboration protocol replays those ids +/// verbatim, so a colliding pair silently forks the document — the failure this +/// refuses to produce. +/// +/// The check is deliberately whole-document rather than per-gesture: draw, +/// duplicate, paste, group and import all mint through the same counter, and +/// gating the single push covers every one of them without a guard at each +/// call site. Edits to existing nodes (move, restyle, delete) carry no new ids +/// and go through untouched. +/// +/// ## Which unknown ids are allowed +/// +/// An id the daemon has not seen is only dangerous when this peer had no +/// right to invent it. Once the owner grants a namespace and the allocator is +/// installed, `c__` ids ARE this peer's to mint — the +/// namespace is what makes them collision-free — so blocking them would mean +/// every node created during a session is stuck forever, and the next pull +/// would quietly delete it. So the gate refuses an id only when it is both +/// unknown to the daemon AND outside this peer's namespace. +/// +/// With no namespace (an older daemon, or a session that has not reached +/// `Active`) the allocator is not installed and every unknown id is refused, +/// exactly as before: the browser is minting bare `n` ids that could +/// collide with a peer's. +/// +/// The local node stays on screen until the next pull replaces it with the +/// daemon's document, so the divergence is bounded and self-healing. +pub(crate) fn push_blocked_by_session(state: &op_editor_core::EditorState) -> bool { + if state.editor_ui.collab.phase != CollabConnectionPhase::Active { + return false; + } + DAEMON_NODE_IDS.with(|known| { + let known = known.borrow(); + let Some(known) = known.as_ref() else { + // No daemon document seen yet in this session; refuse rather than + // guess, since the pull that would settle it is one tick away. + return true; + }; + SESSION_NAMESPACE.with(|namespace| { + let namespace = namespace.borrow(); + document_node_ids(state) + .iter() + .any(|id| !known.contains(id) && !minted_by_this_peer(id, namespace.as_ref())) + }) + }) +} + +/// Whether `id` is one this peer was entitled to mint. +/// +/// Parsed through the shared `op-util` grammar rather than a hand-rolled +/// prefix test: `c_`/`_` are structural, a namespace has its own character +/// rules, and a substring check would accept `c_teamA-evil_7` as belonging to +/// `teamA`. Comparing parsed namespaces is what makes that impossible. +fn minted_by_this_peer( + id: &op_editor_core::NodeId, + namespace: Option<&op_editor_core::PeerNamespace>, +) -> bool { + let Some(namespace) = namespace else { + return false; + }; + op_util::collab_id::NamespacedId::parse(id.as_str()) + .is_ok_and(|parsed| parsed.namespace() == namespace) +} + +#[cfg(test)] +mod tests { + use super::super::note_daemon_document; + use super::super::tests::{doc_with, namespace, reset_latches}; + use super::*; + + #[test] + fn an_active_session_blocks_an_id_this_peer_had_no_right_to_invent() { + reset_latches(); + let mut state = doc_with(&["n100"]); + note_daemon_document(&state); + state + .editor_ui + .collab + .set_phase(CollabConnectionPhase::Active); + + // Editing what the daemon already knows is fine. + assert!(!push_blocked_by_session(&state)); + + // Minting a bare local id is not: with no owner-assigned namespace + // installed, this id could collide with a peer's. + let mut grown = doc_with(&["n100", "n101"]); + grown + .editor_ui + .collab + .set_phase(CollabConnectionPhase::Active); + assert!(push_blocked_by_session(&grown)); + } + + #[test] + fn a_namespaced_id_this_peer_minted_is_pushed() { + // The regression this replaces: every node created during a session + // was blocked forever, because an id the peer had just been granted + // the right to mint is by definition not in the daemon's snapshot. + reset_latches(); + let seed = doc_with(&["n100"]); + note_daemon_document(&seed); + SESSION_NAMESPACE.with(|slot| *slot.borrow_mut() = Some(namespace("teamA"))); + + for created in [ + // create, duplicate and paste all mint through the same counter. + vec!["n100", "c_teamA_1"], + vec!["n100", "c_teamA_1", "c_teamA_2"], + vec!["n100", "c_teamA_4294967296"], + ] { + let mut state = doc_with(&created); + state + .editor_ui + .collab + .set_phase(CollabConnectionPhase::Active); + assert!( + !push_blocked_by_session(&state), + "a node this peer was entitled to create must reach the daemon: {created:?}" + ); + } + } + + #[test] + fn an_id_from_another_peers_namespace_is_still_blocked() { + reset_latches(); + let seed = doc_with(&["n100"]); + note_daemon_document(&seed); + SESSION_NAMESPACE.with(|slot| *slot.borrow_mut() = Some(namespace("teamA"))); + + for hostile in [ + "c_teamB_1", // another peer's namespace + "c_teamA-evil_1", // a substring match a prefix test would accept + "c_teamAevil_1", // ditto, no separator + "n101", // a bare local id + "c_teamA", // no counter + "teamA_1", // no prefix + ] { + let mut state = doc_with(&["n100", hostile]); + state + .editor_ui + .collab + .set_phase(CollabConnectionPhase::Active); + assert!( + push_blocked_by_session(&state), + "{hostile} is not this peer's to mint" + ); + } + } + + #[test] + fn a_namespaced_id_is_blocked_when_no_allocator_is_installed() { + // Fail-closed: without the allocator the peer is minting bare ids, so + // a namespaced-looking id in the document was not minted here. + reset_latches(); + let seed = doc_with(&["n100"]); + note_daemon_document(&seed); + + let mut state = doc_with(&["n100", "c_teamA_1"]); + state + .editor_ui + .collab + .set_phase(CollabConnectionPhase::Active); + assert!(push_blocked_by_session(&state)); + } +} diff --git a/crates/op-host-web/src/identity_epoch.rs b/crates/op-host-web/src/identity_epoch.rs new file mode 100644 index 000000000..98a1b582a --- /dev/null +++ b/crates/op-host-web/src/identity_epoch.rs @@ -0,0 +1,265 @@ +//! Which account this tab currently belongs to, and what happens when that +//! changes. +//! +//! A browser tab outlives a sign-in. Sign out of A and into B in the same tab +//! and, without this, B inherits A's document (the shell keeps whatever was +//! last applied), A's provider credentials (same-origin storage key), and A's +//! sync baseline — and B's own document, starting at a LOWER daemon version, +//! cannot displace it, because the sync client only accepts a version higher +//! than the one it has applied. So the leak is not transient; it is what the +//! tab shows until it is reloaded. +//! +//! The fix is an epoch. Every `/api/auth/status` answer carries a subject; a +//! change of subject — including signed-in → signed-out → signed-in-as-someone +//! -else — bumps the epoch, and everything keyed to an account is dropped. +//! +//! ## Why the subject and not the display name +//! +//! `username` is the account handle the daemon reports for the session. +//! `display_name` is user-editable and `primary_email` can be absent, so +//! neither identifies an account across a switch. + +use std::cell::RefCell; + +/// Storage partition used before anyone signs in. +/// +/// A real subject can never collide with it: the partition key is prefixed, +/// and this value is not a legal account handle. +pub const ANONYMOUS_SUBJECT: &str = "anon"; + +thread_local! { + /// The subject this tab is currently showing. + /// + /// Three states, and the distinction matters: the outer `None` means no + /// status answer has arrived yet, `Some(None)` means observed and signed + /// out, and `Some(Some(subject))` means signed in. Collapsing the first + /// two would make a sign-in after a sign-out look like a tab's very first + /// answer — and that is exactly the A → out → B switch this exists to + /// catch. + static SUBJECT: RefCell>> = const { RefCell::new(None) }; + /// Bumped on every observed identity change. + static EPOCH: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + +/// The account partition in force, for storage keys. +pub fn current_subject() -> String { + SUBJECT.with(|slot| { + slot.borrow() + .clone() + .flatten() + .unwrap_or_else(|| ANONYMOUS_SUBJECT.to_string()) + }) +} + +/// How many identity changes this tab has seen. +pub fn epoch() -> u64 { + EPOCH.with(std::cell::Cell::get) +} + +/// Record the subject an `/api/auth/status` answer reported. +/// +/// Returns `true` when the identity CHANGED and the caller must therefore +/// drop everything keyed to the previous account. The first observation is +/// not a change: the tab has shown nothing yet. +pub fn observe_subject(subject: Option<&str>) -> bool { + let next = subject + .map(str::trim) + .filter(|value| !value.is_empty()) + .map(str::to_string); + SUBJECT.with(|slot| { + let mut slot = slot.borrow_mut(); + let previous = slot.replace(next.clone()); + match previous { + // The tab's first status answer. Whatever it says is what this + // tab has always been showing, so there is nothing to discard — + // but the storage partition is now known, so the epoch moves. + None => { + EPOCH.with(|epoch| epoch.set(epoch.get().saturating_add(1))); + false + } + // A repeat of the same answer: the common case, and it must not + // churn state or every poll would reset the tab. + Some(before) if before == next => false, + // A genuine change — sign-in, sign-out, or a switch between two + // accounts. All three must drop the previous account's state. + Some(_) => { + EPOCH.with(|epoch| epoch.set(epoch.get().saturating_add(1))); + true + } + } + }) +} + +/// Read the account subject out of an `/api/auth/status` body. +/// +/// `None` for a signed-out or unparseable answer, which is the anonymous +/// partition — the safe direction, since it shares nothing with a real one. +pub fn subject_from_status(body: &str) -> Option { + let parsed: serde_json::Value = serde_json::from_str(body).ok()?; + if !parsed["signed_in"].as_bool().unwrap_or(false) { + return None; + } + parsed["username"] + .as_str() + .map(str::trim) + .filter(|value| !value.is_empty()) + .map(str::to_string) +} + +/// Reset the epoch state. Tests only — a tab never goes back to "no answer". +#[cfg(test)] +pub fn reset_for_test() { + SUBJECT.with(|slot| *slot.borrow_mut() = None); + EPOCH.with(|epoch| epoch.set(0)); +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn the_first_anonymous_observation_is_not_a_change() { + reset_for_test(); + assert!(!observe_subject(None)); + assert_eq!(current_subject(), ANONYMOUS_SUBJECT); + } + + #[test] + fn the_first_answer_of_a_tab_is_never_a_reset() { + reset_for_test(); + // Whatever the first answer says is what this tab has always shown. + assert!(!observe_subject(Some("alice"))); + assert_eq!(current_subject(), "alice"); + assert!(epoch() > 0); + } + + #[test] + fn signing_in_after_an_observed_sign_out_is_a_reset() { + // Distinct from the case above: the tab HAS shown the anonymous + // state, so signing in replaces what was on screen. + reset_for_test(); + assert!(!observe_subject(None)); + assert!(observe_subject(Some("alice"))); + assert_eq!(current_subject(), "alice"); + } + + #[test] + fn switching_accounts_reports_a_change() { + reset_for_test(); + observe_subject(Some("alice")); + let before = epoch(); + assert!( + observe_subject(Some("bob")), + "a different account must reset the tab" + ); + assert_eq!(current_subject(), "bob"); + assert!(epoch() > before); + } + + #[test] + fn signing_out_reports_a_change() { + reset_for_test(); + observe_subject(Some("alice")); + assert!(observe_subject(None)); + assert_eq!(current_subject(), ANONYMOUS_SUBJECT); + } + + #[test] + fn the_sign_out_then_in_path_is_a_change_at_each_step() { + // The leak this exists for: A → anonymous → B in one tab. + reset_for_test(); + observe_subject(Some("alice")); + assert!(observe_subject(None)); + assert!(observe_subject(Some("bob"))); + assert_eq!(current_subject(), "bob"); + } + + #[test] + fn the_same_account_repeated_is_never_a_change() { + reset_for_test(); + observe_subject(Some("alice")); + let epoch_after_sign_in = epoch(); + for _ in 0..5 { + assert!(!observe_subject(Some("alice"))); + } + assert_eq!(epoch(), epoch_after_sign_in, "a poll must not churn state"); + } + + #[test] + fn a_subject_is_read_out_of_a_signed_in_status_body() { + assert_eq!( + subject_from_status(r#"{"signed_in":true,"username":"alice"}"#).as_deref(), + Some("alice") + ); + } + + #[test] + fn a_signed_out_or_unusable_status_body_has_no_subject() { + for body in [ + r#"{"signed_in":false,"username":"alice"}"#, + r#"{"signed_in":true}"#, + r#"{"signed_in":true,"username":" "}"#, + "not json", + "{}", + ] { + assert_eq!(subject_from_status(body), None, "{body:?}"); + } + } + + #[test] + fn whitespace_around_a_subject_does_not_create_a_second_partition() { + reset_for_test(); + observe_subject(Some("alice")); + assert!(!observe_subject(Some(" alice "))); + } +} + +// The storage partitions only exist in the build that has a settings store. +#[cfg(all(test, feature = "canvaskit"))] +mod partition_tests { + use super::*; + + #[test] + fn two_accounts_get_two_storage_partitions() { + reset_for_test(); + observe_subject(Some("alice")); + let alice_settings = crate::web_settings::settings_storage_key(); + let alice_credentials = crate::web_settings::credential_storage_key(); + + observe_subject(Some("bob")); + let bob_settings = crate::web_settings::settings_storage_key(); + let bob_credentials = crate::web_settings::credential_storage_key(); + + assert_ne!( + alice_settings, bob_settings, + "two accounts sharing a browser must not share a settings blob" + ); + assert_ne!( + alice_credentials, bob_credentials, + "one account's provider API keys must not be readable by the next" + ); + } + + #[test] + fn signing_out_returns_to_the_anonymous_partition() { + reset_for_test(); + observe_subject(Some("alice")); + let signed_in = crate::web_settings::credential_storage_key(); + observe_subject(None); + let anonymous = crate::web_settings::credential_storage_key(); + assert_ne!(signed_in, anonymous); + assert!(anonymous.ends_with(ANONYMOUS_SUBJECT), "{anonymous}"); + } + + #[test] + fn a_partition_key_is_never_the_bare_legacy_key() { + // The unpartitioned keys may hold a different account's credentials + // than the one now signed in, so they are never read. + reset_for_test(); + for subject in [None, Some("alice")] { + observe_subject(subject); + assert!(crate::web_settings::settings_storage_key().contains("::")); + assert!(crate::web_settings::credential_storage_key().contains("::")); + } + } +} diff --git a/crates/op-host-web/src/lib.rs b/crates/op-host-web/src/lib.rs index 17a244632..58dc24b0e 100644 --- a/crates/op-host-web/src/lib.rs +++ b/crates/op-host-web/src/lib.rs @@ -66,6 +66,10 @@ mod document_json; mod dom_io; #[cfg(feature = "canvaskit")] mod file_actions; +// Which account this tab belongs to. Lives with the daemon-facing modules +// because it is driven by the device-login status poll. +#[cfg(feature = "canvaskit")] +pub mod identity_epoch; #[cfg(all(test, feature = "canvaskit"))] mod prompt_center_file_actions_tests; // Short-lived Worker-side Figma converter. The exported class is instantiated diff --git a/crates/op-host-web/src/live_sync_conflict.rs b/crates/op-host-web/src/live_sync_conflict.rs index 4da0edb79..d66fe573a 100644 --- a/crates/op-host-web/src/live_sync_conflict.rs +++ b/crates/op-host-web/src/live_sync_conflict.rs @@ -217,3 +217,23 @@ mod server_authority_tests { } } } + +#[cfg(test)] +mod auth_latch_tests { + use crate::live_sync_glue::{auth_is_invalid, clear_auth_invalid, note_auth_invalid}; + + #[test] + fn a_refused_credential_stops_the_tab_pushing() { + clear_auth_invalid(); + assert!(!auth_is_invalid(), "a healthy tab pushes"); + + // The document on screen belongs to whoever WAS signed in; pushing it + // after a switch would write one account's work into another's tenant. + note_auth_invalid(); + assert!(auth_is_invalid()); + + // Only the identity reset lifts it, once the tab has been rebuilt. + clear_auth_invalid(); + assert!(!auth_is_invalid()); + } +} diff --git a/crates/op-host-web/src/live_sync_controller.rs b/crates/op-host-web/src/live_sync_controller.rs new file mode 100644 index 000000000..313e1446d --- /dev/null +++ b/crates/op-host-web/src/live_sync_controller.rs @@ -0,0 +1,87 @@ +//! The per-editor sync controller and the identity pairs every gating +//! decision is keyed on. +//! +//! Split out of `live_sync_glue.rs` at the 800-line cap — pure code motion. + +use std::cell::RefCell; +use std::rc::{Rc, Weak}; + +use op_editor_core::sync_gate::SyncGate; +use op_editor_core::web_sync::WebSyncClient; + +use crate::repaint_ctx::RepaintContext; + +use super::ACTIVE_SYNC; + +/// Shared sync state for one mounted editor. +/// +/// The gate and the client must see the same document identity, so they live +/// in one struct rather than as two independently-owned pieces. +pub(crate) struct SyncController { + pub gate: SyncGate, + pub client: WebSyncClient, + pub push_busy: bool, + /// A document identity already measured above the periodic push limit. + /// WASM linear memory does not shrink after a giant temporary JSON string, + /// so do not rebuild the same oversized snapshot every two seconds. + pub(super) oversize_identity: Option<(u64, u64, u64)>, +} + +impl SyncController { + pub(crate) fn new() -> Self { + Self { + gate: SyncGate::default(), + client: WebSyncClient::new(), + push_busy: false, + oversize_identity: None, + } + } +} + +pub(crate) type SharedSync = Rc>; + +/// Commit a successful daemon Save as a sync acknowledgement. +/// +/// The daemon has already installed exactly the saved snapshot. Recording its +/// version prevents the next probe from downloading and replacing the same +/// potentially huge document, while the snapshot pair reopens the pull gate. +/// A later local edit still differs from this pair and remains eligible for a +/// normal push (or another explicit Save for oversized documents). +pub(crate) fn acknowledge_daemon_save( + version: u64, + generation: u64, + revision: u64, + active_page_index: usize, + preserve_authored_geometry: bool, +) { + ACTIVE_SYNC.with(|slot| { + let Some(sync) = slot.borrow().as_ref().and_then(Weak::upgrade) else { + return; + }; + let mut sync = sync.borrow_mut(); + sync.client.mark_applied(version); + sync.client + .note_applied_snapshot_without_hash(active_page_index, preserve_authored_geometry); + sync.gate.note_synced(generation, revision); + }); +} + +/// The document-identity pair every gating decision is keyed on. Read fresh +/// from the live editor state at each decision point — never cached — so an +/// edit that lands between a tick firing and its async response landing is +/// always observed. +pub(super) fn current_pair(b: &C) -> (u64, u64) { + let s = b.host().editor_state(); + (s.document_generation(), s.document_revision()) +} + +pub(super) fn current_oversize_identity(b: &C) -> (u64, u64, u64) { + let host = b.host(); + let state = host.editor_state(); + let doc = state; + ( + host.document_epoch(), + doc.document_generation(), + doc.document_revision(), + ) +} diff --git a/crates/op-host-web/src/live_sync_glue.rs b/crates/op-host-web/src/live_sync_glue.rs index c11257a95..eb847fcf8 100644 --- a/crates/op-host-web/src/live_sync_glue.rs +++ b/crates/op-host-web/src/live_sync_glue.rs @@ -120,83 +120,7 @@ fn serialize_sync_document(doc: &op_editor_core::PenDocument) -> SyncDocumentJso /// once in `mount_ck` and handed to both this module's ticks and the /// postMessage bridge, so both sides observe and mutate the exact same /// `SyncGate` instance (a v1 defect — the bridge couldn't reach a local -/// `WebSyncClient` — is fixed by sharing this one struct). -pub(crate) struct SyncController { - pub gate: SyncGate, - pub client: WebSyncClient, - pub push_busy: bool, - /// A document identity already measured above the periodic push limit. - /// WASM linear memory does not shrink after a giant temporary JSON string, - /// so do not rebuild the same oversized snapshot every two seconds. - oversize_identity: Option<(u64, u64, u64)>, -} - -impl SyncController { - pub(crate) fn new() -> Self { - Self { - gate: SyncGate::default(), - client: WebSyncClient::new(), - push_busy: false, - oversize_identity: None, - } - } -} - -pub(crate) type SharedSync = Rc>; - -thread_local! { - /// The mounted editor's sync controller, exposed weakly so File → Save can - /// consume the daemon's returned version without creating an ownership - /// cycle or threading sync state through every DOM file-action callback. - static ACTIVE_SYNC: RefCell>>> = const { RefCell::new(None) }; -} - -/// Commit a successful daemon Save as a sync acknowledgement. -/// -/// The daemon has already installed exactly the saved snapshot. Recording its -/// version prevents the next probe from downloading and replacing the same -/// potentially huge document, while the snapshot pair reopens the pull gate. -/// A later local edit still differs from this pair and remains eligible for a -/// normal push (or another explicit Save for oversized documents). -pub(crate) fn acknowledge_daemon_save( - version: u64, - generation: u64, - revision: u64, - active_page_index: usize, - preserve_authored_geometry: bool, -) { - ACTIVE_SYNC.with(|slot| { - let Some(sync) = slot.borrow().as_ref().and_then(Weak::upgrade) else { - return; - }; - let mut sync = sync.borrow_mut(); - sync.client.mark_applied(version); - sync.client - .note_applied_snapshot_without_hash(active_page_index, preserve_authored_geometry); - sync.gate.note_synced(generation, revision); - }); -} - -/// The document-identity pair every gating decision is keyed on. Read fresh -/// from the live editor state at each decision point — never cached — so an -/// edit that lands between a tick firing and its async response landing is -/// always observed. -fn current_pair(b: &C) -> (u64, u64) { - let s = b.host().editor_state(); - (s.document_generation(), s.document_revision()) -} - -fn current_oversize_identity(b: &C) -> (u64, u64, u64) { - let host = b.host(); - let state = host.editor_state(); - ( - host.document_epoch(), - state.document_generation(), - state.document_revision(), - ) -} - -/// Content changes advance the gate's `(generation, revision)` pair, while +//// Content changes advance the gate's `(generation, revision)` pair, while /// active-page switches intentionally do not. Treat an editor-metadata delta /// as an additional push reason without weakening the conflict latch. #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -310,7 +234,15 @@ fn poll_version( let base_owned = base.to_string(); let fetch_busy = fetch_busy.clone(); let last_selection_key = last_selection_key.clone(); - let on_version: Rc = Rc::new(move |body: String| { + let on_version: Rc = Rc::new(move |status: u16, body: String| { + // A refused credential is the signal that this tab no longer speaks + // for the account whose document it is showing. Treating it as "no + // version" — which is what an untyped body read does — is what let the + // previous account's document sit on screen indefinitely. + if matches!(status, 401 | 403) { + note_auth_invalid(); + return; + } // The daemon answers both counters here. Hand the collaboration one to // its own loop rather than opening a second probe; a `collabSeq` bump // must never reach the document fetch below. @@ -340,7 +272,7 @@ fn poll_version( fetch_busy.set(false); } }); - let _ = live_sync::get(&format!("{base}/api/mcp/version"), on_version); + let _ = live_sync::get_with_status(&format!("{base}/api/mcp/version"), on_version); } /// Auto-resolve a latched push conflict, but only inside a live session. @@ -583,6 +515,13 @@ fn push_document_if_changed( // from the scalar baseline, not mistaken for a content change. reasons.editor_meta }; + // The daemon refused this tab's credential. Whatever is on screen belongs + // to whoever was signed in before, so pushing it now would write one + // account's work into whichever tenant the new credential resolves to. + // Held until the identity reset rebuilds the tab. + if auth_is_invalid() { + return; + } // An active collaboration session cannot sequence a node id this browser // minted from its local counter. Hold the push rather than fork the shared // document; the next pull replaces the local node with the daemon's copy. @@ -770,6 +709,34 @@ mod tests { } } +thread_local! { + /// Raised when the daemon answered a sync request 401/403. + /// + /// A tab whose credential stopped being accepted must stop pushing: the + /// document on screen belongs to whoever WAS signed in, and pushing it + /// after a switch would write one account's work into another's tenant. + /// Cleared by [`clear_auth_invalid`], which the identity reset calls once + /// the tab has been rebuilt for the new account. + pub(super) static AUTH_INVALID: std::cell::Cell = const { std::cell::Cell::new(false) }; + + /// The mounted editor's sync controller, exposed weakly so File → Save can + /// consume the daemon's returned version without creating an ownership + /// cycle or threading sync state through every DOM file-action callback. + pub(super) static ACTIVE_SYNC: RefCell>>> = const { RefCell::new(None) }; +} + +#[path = "live_sync_controller.rs"] +mod live_sync_controller; +pub(crate) use live_sync_controller::{acknowledge_daemon_save, SharedSync, SyncController}; +// Spine-local: the two identity pairs every gating decision here is keyed on. +use live_sync_controller::{current_oversize_identity, current_pair}; + #[path = "live_sync_conflict.rs"] mod live_sync_conflict; +#[path = "live_sync_identity.rs"] +mod live_sync_identity; +pub(crate) use live_sync_identity::{auth_is_invalid, note_auth_invalid, reset_for_new_identity}; +// Only the latch's own test lifts it; the reset clears it internally. use live_sync_conflict::{auto_resolve_is_safe, probe_serve_mode, server_is_authoritative}; +#[cfg(test)] +pub(crate) use live_sync_identity::clear_auth_invalid; diff --git a/crates/op-host-web/src/live_sync_identity.rs b/crates/op-host-web/src/live_sync_identity.rs new file mode 100644 index 000000000..7bed0e68b --- /dev/null +++ b/crates/op-host-web/src/live_sync_identity.rs @@ -0,0 +1,67 @@ +//! What happens when the tab stops speaking for the account it is showing. +//! +//! Split out of `live_sync_glue.rs` at the 800-line cap. Two related things +//! live here: the latch that stops a tab pushing once the daemon has refused +//! its credential, and the reset that rebuilds the tab when +//! `/api/auth/status` reports a different account. +//! +//! Together they close the same hole — a browser tab outlives a sign-in, so +//! without them account B inherits account A's document, sync baseline and +//! session state, and cannot displace them. + +use std::cell::RefCell; +use std::rc::{Rc, Weak}; + +use op_editor_core::sync_gate::SyncGate; +use op_editor_core::web_sync::WebSyncClient; + +use crate::repaint_ctx::RepaintContext; + +use super::{ACTIVE_SYNC, AUTH_INVALID}; + +/// Whether the daemon has refused this tab's credential. +pub(crate) fn auth_is_invalid() -> bool { + AUTH_INVALID.with(std::cell::Cell::get) +} + +/// Latch the tab out of pushing after a 401/403. +pub(crate) fn note_auth_invalid() { + AUTH_INVALID.with(|flag| flag.set(true)); +} + +/// Release the latch. Only the identity reset calls this. +pub(crate) fn clear_auth_invalid() { + AUTH_INVALID.with(|flag| flag.set(false)); +} + +/// Drop everything this tab was showing for the previous account. +/// +/// Called when `/api/auth/status` reports a different subject. Without it the +/// tab keeps the previous account's document (the shell simply never replaced +/// it), its sync baseline, and — because the sync client only accepts a +/// version HIGHER than the one it applied — the new account's own document, +/// which starts lower, can never displace it. So this is not tidy-up; it is +/// the only thing that makes the switch take effect at all. +pub(crate) fn reset_for_new_identity(inner: &Rc>) { + ACTIVE_SYNC.with(|slot| { + if let Some(sync) = slot.borrow().as_ref().and_then(Weak::upgrade) { + let mut sync = sync.borrow_mut(); + // A fresh client has applied no version, so the next probe + // downloads the new account's document whatever its number. + sync.client = WebSyncClient::new(); + sync.gate = SyncGate::default(); + sync.push_busy = false; + } + }); + if let Ok(mut context) = inner.try_borrow_mut() { + let state = context.host_mut().editor_state_mut(); + // Back to the same starter a fresh tab paints, so nothing of the + // previous account survives on screen. + state.replace_document(op_editor_core::EditorState::starter().doc); + state.editor_ui.collab = op_editor_core::CollabUiState::default(); + context.host_mut().mark_editor_state_dirty(); + let _ = context.repaint(); + } + crate::collab_sync::reset_for_new_identity(); + clear_auth_invalid(); +} diff --git a/crates/op-host-web/src/web_auth_sync.rs b/crates/op-host-web/src/web_auth_sync.rs index beaedc22d..c6bd1a3d2 100644 --- a/crates/op-host-web/src/web_auth_sync.rs +++ b/crates/op-host-web/src/web_auth_sync.rs @@ -175,6 +175,13 @@ fn fetch_status(inner: &Rc>, base: &str) let Ok(parsed) = serde_json::from_str::(&body) else { return; }; + // Identity first: everything below paints for an account, so the + // account has to be settled before any of it runs. + if crate::identity_epoch::observe_subject( + crate::identity_epoch::subject_from_status(&body).as_deref(), + ) { + crate::live_sync_glue::reset_for_new_identity(&inner); + } sync_account_avatar(&inner, parsed["avatar_revision"].as_str()); let mut b = inner.borrow_mut(); let ui = &mut b.host_mut().editor_state_mut().editor_ui; diff --git a/crates/op-host-web/src/web_settings.rs b/crates/op-host-web/src/web_settings.rs index 23efe25af..fa40269c6 100644 --- a/crates/op-host-web/src/web_settings.rs +++ b/crates/op-host-web/src/web_settings.rs @@ -30,6 +30,31 @@ const CREDENTIAL_PAYLOAD_VERSION: u32 = 2; const STORAGE_KEY: &str = "openpencil-rust-web-settings"; const CREDENTIAL_STORAGE_KEY: &str = "openpencil-rust-web-credentials"; +/// Per-account storage keys. +/// +/// The base keys above are same-origin and carry no account dimension, so two +/// accounts using one browser shared one settings blob — and one account's +/// provider API keys were readable by the next. Every read and write goes +/// through these instead, partitioned by the signed-in subject +/// (`identity_epoch::current_subject`, `"anon"` before sign-in). +/// +/// The old unpartitioned keys are deliberately NOT migrated. They may hold a +/// different account's credentials than the one now signed in, and there is no +/// way to tell whose they were — so adopting them is exactly the leak this +/// closes. They are left in place (untouched, unread) rather than deleted, so +/// a user who downgrades does not lose their settings. +pub(crate) fn settings_storage_key() -> String { + partitioned(STORAGE_KEY) +} + +pub(crate) fn credential_storage_key() -> String { + partitioned(CREDENTIAL_STORAGE_KEY) +} + +fn partitioned(base: &str) -> String { + format!("{base}::{}", crate::identity_epoch::current_subject()) +} + #[path = "web_settings_legacy.rs"] mod legacy; #[path = "web_settings_storage.rs"] diff --git a/crates/op-host-web/src/web_settings_acp_scrub_tests.rs b/crates/op-host-web/src/web_settings_acp_scrub_tests.rs index 27c5767d4..0881b7c13 100644 --- a/crates/op-host-web/src/web_settings_acp_scrub_tests.rs +++ b/crates/op-host-web/src/web_settings_acp_scrub_tests.rs @@ -41,7 +41,7 @@ fn legacy_acp_in_separate_credentials_is_removed_without_losing_builtin_keys() { ); assert!(state.editor_ui.agent_settings.acp_agents.is_empty()); assert_eq!(writes.len(), 1); - assert_eq!(writes[0].0, CREDENTIAL_STORAGE_KEY); + assert_eq!(writes[0].0, super::credential_storage_key()); assert!(writes[0].1.contains("sk-must-survive-acp-removal")); assert!(!writes[0].1.contains("acp_agents")); assert!(!writes[0].1.contains("acp-command-secret")); @@ -91,7 +91,7 @@ fn separate_acp_scrub_does_not_touch_future_general_settings() { assert!(load.unsupported_version); assert!(load.initial_settings_fingerprint(&state).is_none()); assert_eq!(writes.len(), 1); - assert_eq!(writes[0].0, CREDENTIAL_STORAGE_KEY); + assert_eq!(writes[0].0, super::credential_storage_key()); assert!(writes[0].1.contains("sk-must-survive-acp-removal")); assert!(!writes[0].1.contains("acp_agents")); assert!(!writes[0].1.contains("acp-command-secret")); @@ -143,8 +143,8 @@ fn acp_is_scrubbed_from_general_and_separate_snapshots_in_one_ordered_migration( assert!(load.loaded); assert!(!load.write_pending); assert_eq!(writes.len(), 2); - assert_eq!(writes[0].0, CREDENTIAL_STORAGE_KEY); - assert_eq!(writes[1].0, STORAGE_KEY); + assert_eq!(writes[0].0, super::credential_storage_key()); + assert_eq!(writes[1].0, super::settings_storage_key()); for (_, json) in &writes { assert!(!json.contains("acp_agents")); assert!(!json.contains("general-acp-secret")); @@ -181,7 +181,7 @@ fn legacy_acp_configuration_is_removed_without_loading_or_migrating_it() { assert!(!load.loaded); assert!(!load.write_pending); assert_eq!(writes.len(), 1); - assert_eq!(writes[0].0, STORAGE_KEY); + assert_eq!(writes[0].0, super::settings_storage_key()); assert!(!writes[0].1.contains("acp_agents")); assert!(!writes[0].1.contains("connected")); assert!(!writes[0].1.contains("legacy-command-secret")); @@ -214,7 +214,7 @@ fn failed_supported_acp_only_scrub_retries_without_authoritative_empty_credentia assert!(!load.unsupported_version); assert!(load.write_pending); assert_eq!(initial_writes.len(), 1); - assert_eq!(initial_writes[0].0, STORAGE_KEY); + assert_eq!(initial_writes[0].0, super::settings_storage_key()); let mut baseline = load.initial_fingerprint(&state); let mut retry_writes = Vec::new(); @@ -226,7 +226,7 @@ fn failed_supported_acp_only_scrub_retries_without_authoritative_empty_credentia assert!(saved.is_none()); assert!(!credential_migration_pending(&baseline)); assert_eq!(retry_writes.len(), 1); - assert_eq!(retry_writes[0].0, STORAGE_KEY); + assert_eq!(retry_writes[0].0, super::settings_storage_key()); assert!(!retry_writes[0].1.contains("acp_agents")); assert!(!retry_writes[0].1.contains("connected")); assert!(!retry_writes[0].1.contains("supported-acp-secret")); @@ -247,7 +247,7 @@ fn failed_supported_acp_only_scrub_retries_without_authoritative_empty_credentia .is_some() ); assert_eq!(user_writes.len(), 1); - assert_eq!(user_writes[0].0, CREDENTIAL_STORAGE_KEY); + assert_eq!(user_writes[0].0, super::credential_storage_key()); assert!(user_writes[0].1.contains("sk-user-edit")); } @@ -274,7 +274,7 @@ fn failed_read_only_credential_scrub_retries_without_enabling_user_writes() { assert!(load.unsupported_version); assert!(load.write_pending); assert_eq!(initial_writes.len(), 1); - assert_eq!(initial_writes[0].0, CREDENTIAL_STORAGE_KEY); + assert_eq!(initial_writes[0].0, super::credential_storage_key()); let mut baseline = load.initial_fingerprint(&state); let mut retry_writes = Vec::new(); @@ -286,7 +286,7 @@ fn failed_read_only_credential_scrub_retries_without_enabling_user_writes() { assert!(saved.is_none()); assert!(!credential_migration_pending(&baseline)); assert_eq!(retry_writes.len(), 1); - assert_eq!(retry_writes[0].0, CREDENTIAL_STORAGE_KEY); + assert_eq!(retry_writes[0].0, super::credential_storage_key()); assert!(!retry_writes[0].1.contains("acp_agents")); assert!(!retry_writes[0].1.contains("acp-secret")); assert!(retry_writes[0].1.contains("future-secret-must-survive")); @@ -329,7 +329,7 @@ fn failed_read_only_general_scrub_retries_without_creating_credential_snapshot() assert!(load.unsupported_version); assert!(load.write_pending); assert_eq!(initial_writes.len(), 1); - assert_eq!(initial_writes[0].0, STORAGE_KEY); + assert_eq!(initial_writes[0].0, super::settings_storage_key()); let mut baseline = load.initial_fingerprint(&state); let mut retry_writes = Vec::new(); @@ -341,7 +341,7 @@ fn failed_read_only_general_scrub_retries_without_creating_credential_snapshot() assert!(saved.is_none()); assert!(!credential_migration_pending(&baseline)); assert_eq!(retry_writes.len(), 1); - assert_eq!(retry_writes[0].0, STORAGE_KEY); + assert_eq!(retry_writes[0].0, super::settings_storage_key()); assert!(!retry_writes[0].1.contains("acp_agents")); assert!(!retry_writes[0].1.contains("connected")); assert!(!retry_writes[0].1.contains("general-acp-secret")); @@ -380,21 +380,21 @@ fn partial_read_only_scrub_retry_clears_only_the_successful_write() { assert!(load.write_pending); assert_eq!(initial_writes.len(), 1); - assert_eq!(initial_writes[0].0, CREDENTIAL_STORAGE_KEY); + assert_eq!(initial_writes[0].0, super::credential_storage_key()); let mut baseline = load.initial_fingerprint(&state); let mut first_retry = Vec::new(); assert!( save_credentials_if_changed_with(&state, &mut baseline, |key, json| { first_retry.push((key.to_string(), json.to_string())); - key == CREDENTIAL_STORAGE_KEY + *key == super::credential_storage_key() }) .is_none() ); assert!(credential_migration_pending(&baseline)); assert_eq!(first_retry.len(), 2); - assert_eq!(first_retry[0].0, CREDENTIAL_STORAGE_KEY); - assert_eq!(first_retry[1].0, STORAGE_KEY); + assert_eq!(first_retry[0].0, super::credential_storage_key()); + assert_eq!(first_retry[1].0, super::settings_storage_key()); assert!(first_retry[0].1.contains("future-credential-secret")); assert!(!first_retry[0].1.contains("credential-acp-secret")); @@ -408,7 +408,7 @@ fn partial_read_only_scrub_retry_clears_only_the_successful_write() { ); assert!(!credential_migration_pending(&baseline)); assert_eq!(second_retry.len(), 1); - assert_eq!(second_retry[0].0, STORAGE_KEY); + assert_eq!(second_retry[0].0, super::settings_storage_key()); assert!(second_retry[0].1.contains("future-general-secret")); assert!(!second_retry[0].1.contains("general-acp-secret")); } diff --git a/crates/op-host-web/src/web_settings_lossless_tests.rs b/crates/op-host-web/src/web_settings_lossless_tests.rs index 57d9f9e7c..e77fc180a 100644 --- a/crates/op-host-web/src/web_settings_lossless_tests.rs +++ b/crates/op-host-web/src/web_settings_lossless_tests.rs @@ -238,7 +238,7 @@ fn acp_scrub_preserves_unknown_general_fields_and_leaves_snapshot_read_only() { assert!(!load.loaded); assert!(load.unsupported_version); assert_eq!(writes.len(), 1); - assert_eq!(writes[0].0, STORAGE_KEY); + assert_eq!(writes[0].0, super::settings_storage_key()); assert!(writes[0].1.contains("future_setting")); assert!(writes[0].1.contains("future-secret")); assert!(!writes[0].1.contains("acp_agents")); @@ -266,7 +266,7 @@ fn acp_scrub_preserves_unknown_credential_fields_and_disables_future_writes() { assert!(!load.loaded); assert!(load.unsupported_version); assert_eq!(writes.len(), 1); - assert_eq!(writes[0].0, CREDENTIAL_STORAGE_KEY); + assert_eq!(writes[0].0, super::credential_storage_key()); assert!(writes[0].1.contains("future_credentials")); assert!(writes[0].1.contains("future-credential-secret")); assert!(!writes[0].1.contains("acp_agents")); diff --git a/crates/op-host-web/src/web_settings_storage.rs b/crates/op-host-web/src/web_settings_storage.rs index 59abcfb2c..f2b9d6d99 100644 --- a/crates/op-host-web/src/web_settings_storage.rs +++ b/crates/op-host-web/src/web_settings_storage.rs @@ -35,8 +35,8 @@ impl CredentialLoad { } pub(crate) fn load_into(state: &mut EditorState) -> CredentialLoad { - let settings_raw = storage_get(STORAGE_KEY); - let credential_raw = storage_get(CREDENTIAL_STORAGE_KEY); + let settings_raw = storage_get(&super::settings_storage_key()); + let credential_raw = storage_get(&super::credential_storage_key()); let load = load_into_with( state, settings_raw.as_deref(), @@ -68,7 +68,7 @@ where let sanitized = stored.sanitized_settings_json; let settings_saved = sanitized .as_deref() - .is_none_or(|json| persist(STORAGE_KEY, json)); + .is_none_or(|json| persist(&super::settings_storage_key(), json)); CredentialLoad { loaded: true, write_pending: !settings_saved, @@ -85,11 +85,11 @@ where let credential = stored.sanitized_credential_json.or(canonical_credential); let credential_saved = credential .as_deref() - .is_some_and(|json| persist(CREDENTIAL_STORAGE_KEY, json)); + .is_some_and(|json| persist(&super::credential_storage_key(), json)); let settings_saved = credential_saved && sanitized .as_deref() - .is_none_or(|json| persist(STORAGE_KEY, json)); + .is_none_or(|json| persist(&super::settings_storage_key(), json)); let write_pending = !credential_saved || !settings_saved; CredentialLoad { loaded: true, @@ -106,7 +106,7 @@ where let sanitized = stored.sanitized_settings_json; let settings_saved = sanitized .as_deref() - .is_none_or(|json| persist(STORAGE_KEY, json)); + .is_none_or(|json| persist(&super::settings_storage_key(), json)); return CredentialLoad { loaded: false, write_pending: !settings_saved, @@ -121,11 +121,11 @@ where let credential = credentials_json(state); let credential_saved = credential .as_deref() - .is_some_and(|json| persist(CREDENTIAL_STORAGE_KEY, json)); + .is_some_and(|json| persist(&super::credential_storage_key(), json)); let settings_saved = credential_saved && sanitized .as_deref() - .is_none_or(|json| persist(STORAGE_KEY, json)); + .is_none_or(|json| persist(&super::settings_storage_key(), json)); let write_pending = !credential_saved || !settings_saved; CredentialLoad { // Both legacy credentials and a healed invalid separate @@ -146,12 +146,12 @@ where let credential = stored.sanitized_credential_json; let credential_saved = credential .as_deref() - .is_none_or(|json| persist(CREDENTIAL_STORAGE_KEY, json)); + .is_none_or(|json| persist(&super::credential_storage_key(), json)); let sanitized = stored.sanitized_settings_json; let settings_saved = credential_saved && sanitized .as_deref() - .is_none_or(|json| persist(STORAGE_KEY, json)); + .is_none_or(|json| persist(&super::settings_storage_key(), json)); CredentialLoad { loaded: false, write_pending: !credential_saved || !settings_saved, @@ -166,7 +166,7 @@ where let sanitized = stored.sanitized_settings_json; let settings_saved = sanitized .as_deref() - .is_none_or(|json| persist(STORAGE_KEY, json)); + .is_none_or(|json| persist(&super::settings_storage_key(), json)); CredentialLoad { loaded: false, write_pending: !settings_saved, @@ -306,7 +306,9 @@ fn clear_local_credentials(state: &mut EditorState) { } pub(crate) fn save_if_changed(state: &EditorState, before: &mut Fingerprint) -> bool { - let saved = save_if_changed_with(state, before, |json| storage_set_checked(STORAGE_KEY, json)); + let saved = save_if_changed_with(state, before, |json| { + storage_set_checked(&super::settings_storage_key(), json) + }); if saved { clear_storage_failure(); } else if fingerprint(state) != *before { @@ -376,7 +378,7 @@ where return None; } let json = credentials_json(state)?; - if !persist(CREDENTIAL_STORAGE_KEY, &json) { + if !persist(&super::credential_storage_key(), &json) { return None; } *before = next; @@ -388,13 +390,13 @@ where F: FnMut(&str, &str) -> bool, { if let Some(json) = before.pending_credential_json.clone() { - if !persist(CREDENTIAL_STORAGE_KEY, &json) { + if !persist(&super::credential_storage_key(), &json) { return false; } before.pending_credential_json = None; } if let Some(json) = before.pending_settings_json.clone() { - if !persist(STORAGE_KEY, &json) { + if !persist(&super::settings_storage_key(), &json) { return false; } before.pending_settings_json = None; diff --git a/crates/op-host-web/src/web_settings_tests.rs b/crates/op-host-web/src/web_settings_tests.rs index 62dd59c6b..a755cb24c 100644 --- a/crates/op-host-web/src/web_settings_tests.rs +++ b/crates/op-host-web/src/web_settings_tests.rs @@ -82,7 +82,7 @@ fn legacy_mcp_flags_drop_gemini_without_shifting_google_antigravity() { ); let rewritten = writes .iter() - .find_map(|(key, json)| (key == STORAGE_KEY).then_some(json)) + .find_map(|(key, json)| (*key == super::settings_storage_key()).then_some(json)) .expect("legacy positional settings should be rewritten"); let value: serde_json::Value = serde_json::from_str(rewritten).expect("rewritten settings"); assert_eq!( @@ -380,7 +380,7 @@ fn future_credential_snapshot_remains_read_only_after_a_provider_edit() { ); let mut writes = 0; let saved = save_credentials_if_changed_with(&state, &mut baseline, |key, _| { - if key == CREDENTIAL_STORAGE_KEY { + if *key == super::credential_storage_key() { writes += 1; } true @@ -487,7 +487,7 @@ fn corrupt_separate_snapshot_queues_its_persisted_empty_replacement_for_server_s assert!(!load.write_pending); let credential_write = writes .iter() - .find(|(key, _)| key == CREDENTIAL_STORAGE_KEY) + .find(|(key, _)| *key == super::credential_storage_key()) .expect("the corrupt credential snapshot is replaced"); let value: serde_json::Value = serde_json::from_str(&credential_write.1).expect("replacement is valid JSON"); @@ -680,7 +680,7 @@ fn failed_legacy_credential_write_stays_loaded_and_retries_before_sanitizing() { assert!(load.loaded); assert!(load.write_pending); assert_eq!(first_writes.len(), 1); - assert_eq!(first_writes[0].0, CREDENTIAL_STORAGE_KEY); + assert_eq!(first_writes[0].0, super::credential_storage_key()); assert!(first_writes[0].1.contains("sk-retry-migration")); let mut baseline = load.initial_fingerprint(&state); @@ -695,8 +695,8 @@ fn failed_legacy_credential_write_stays_loaded_and_retries_before_sanitizing() { // sync. Retrying local persistence must not emit a second sync payload. assert!(saved.is_none()); assert_eq!(retry_writes.len(), 2); - assert_eq!(retry_writes[0].0, CREDENTIAL_STORAGE_KEY); - assert_eq!(retry_writes[1].0, STORAGE_KEY); + assert_eq!(retry_writes[0].0, super::credential_storage_key()); + assert_eq!(retry_writes[1].0, super::settings_storage_key()); assert!(!retry_writes[1].1.contains("sk-retry-migration")); assert_eq!(baseline, credential_fingerprint(&state)); }