From 13780388fb3e1f1bebe7bc50ddc6f08cd532e7c1 Mon Sep 17 00:00:00 2001 From: PathGao Date: Wed, 5 Aug 2026 07:15:53 +0800 Subject: [PATCH] test(tabs): run a transfer end to end instead of inspecting the gate #366 gated claim/complete/cancel on the caller being window-. That label exists only for a window create_transfer_window just built, so offer_tab_to_window's destination -- an already-open window labelled main or window- -- was refused 100% of the time. #452 fixed it by recording the target on the pending entry and accepting either label. It reached master, not a release. Nothing in the suite failed. The Rust test asserted the predicate behaves as written, and it did: the gate was self-consistently wrong. The script test asserted that the invoke('offer_tab_to_window', ...) call appears in the source, and it still appeared. Both confirmed an implementation exists and is internally consistent, which is precisely what a reachable path becoming unreachable leaves undisturbed. Four tests now drive the broker from stage to complete: * a transfer to an existing window -- stage from main, record the target the way offer_tab_to_window does, claim as that window, complete. This is the path that was dead. * a transfer to a new window -- claim as window- with no target recorded, so the fix cannot trade one path for the other. * a bystander window refused at claim, complete and cancel, with the transfer left intact for its real destination afterwards. * the recorded target releasing its own claim, the only rollback a claimed transfer has: the source's timeout is deliberately inert once a claim exists, so a refusal here would strand the tab in both windows. The authorisation decision moves out of the three #[tauri::command] bodies onto the broker as claim_as/complete_as/cancel_as, which take the caller's label as &str; each command is now one line passing window.label(). A unit test cannot build a tauri::Window, so the gate was otherwise reachable only through the predicate it was written against. Same peek, same order, same error strings. Reverting the recorded-target arm of is_destination_authority and restoring the two predicate tests to their #366 wording leaves the suite at 3 failed / 151 passed -- all three of them these tests. Co-authored-by: Claude Opus 5 --- src-tauri/src/tab_transfer.rs | 242 +++++++++++++++++++++++++++------- 1 file changed, 198 insertions(+), 44 deletions(-) diff --git a/src-tauri/src/tab_transfer.rs b/src-tauri/src/tab_transfer.rs index 7dc2c80d..b6a2461d 100644 --- a/src-tauri/src/tab_transfer.rs +++ b/src-tauri/src/tab_transfer.rs @@ -244,6 +244,73 @@ impl TabTransferBroker { }, } } + + /// The gate in front of `claim` and `complete`. A window is a destination + /// if the token names it (`create_transfer_window` built it) or if + /// `offer_tab_to_window` recorded it as the target; an unknown token has + /// no destination at all, so it is refused here rather than later. + fn authorize_destination( + &self, + caller_label: &str, + token: &str, + action: &str, + ) -> Result<(), String> { + let authorized = self.peek(token).is_some_and(|entry| { + is_destination_authority(caller_label, token, entry.target_label.as_deref()) + }); + if authorized { + Ok(()) + } else { + Err(format!( + "TRANSFER_FORBIDDEN: window '{caller_label}' may not {action} this tab transfer" + )) + } + } + + fn claim_as( + &self, + caller_label: &str, + token: &str, + now: Instant, + ) -> Result, String> { + self.authorize_destination(caller_label, token, "claim")?; + Ok(self.claim(token, now).map(|transfer| transfer.payload)) + } + + fn complete_as(&self, caller_label: &str, token: &str) -> Result { + self.authorize_destination(caller_label, token, "complete")?; + self.take(token).ok_or_else(|| { + "TRANSFER_NOT_FOUND: the staged tab is gone; the source window kept it".to_string() + }) + } + + fn cancel_as( + &self, + caller_label: &str, + token: &str, + now: Instant, + ) -> Result { + let Some(entry) = self.peek(token) else { + // Already completed, cancelled or never staged: cancelling is a + // no-op, so stay idempotent rather than erroring on a late timeout. + return Ok(CancelOutcome { + cancelled: false, + claimed: false, + }); + }; + match cancel_authority( + caller_label, + &entry.source_label, + token, + entry.target_label.as_deref(), + ) { + CancelAuthority::Source => Ok(self.cancel_from_source(token, now)), + CancelAuthority::Destination => Ok(self.cancel_from_destination(token)), + CancelAuthority::Forbidden => Err(format!( + "TRANSFER_FORBIDDEN: window '{caller_label}' may not cancel this tab transfer" + )), + } + } } #[tauri::command] @@ -261,20 +328,7 @@ pub fn claim_detached_tab( state: State<'_, TabTransferBroker>, token: String, ) -> Result, String> { - let entry = state.peek(&token); - let is_authorized = entry - .as_ref() - .map_or(false, |e| is_destination_authority(window.label(), &token, e.target_label.as_deref())); - - if !is_authorized { - return Err(format!( - "TRANSFER_FORBIDDEN: window '{}' may not claim this tab transfer", - window.label() - )); - } - Ok(state - .claim(&token, Instant::now()) - .map(|transfer| transfer.payload)) + state.claim_as(window.label(), &token, Instant::now()) } #[tauri::command] @@ -284,20 +338,7 @@ pub fn complete_detached_tab( state: State<'_, TabTransferBroker>, token: String, ) -> Result<(), String> { - let entry = state.peek(&token); - let is_authorized = entry - .as_ref() - .map_or(false, |e| is_destination_authority(window.label(), &token, e.target_label.as_deref())); - - if !is_authorized { - return Err(format!( - "TRANSFER_FORBIDDEN: window '{}' may not complete this tab transfer", - window.label() - )); - } - let transfer = state.take(&token).ok_or_else(|| { - "TRANSFER_NOT_FOUND: the staged tab is gone; the source window kept it".to_string() - })?; + let transfer = state.complete_as(window.label(), &token)?; app.emit_to(transfer.source_label.as_str(), "tab-transfer-claimed", token) .map_err(|e| format!("TRANSFER_HANDOFF_FAILED: {e}")) } @@ -308,22 +349,7 @@ pub fn cancel_detached_tab( state: State<'_, TabTransferBroker>, token: String, ) -> Result { - let Some(entry) = state.peek(&token) else { - // Already completed, cancelled or never staged: cancelling is a - // no-op, so stay idempotent rather than erroring on a late timeout. - return Ok(CancelOutcome { - cancelled: false, - claimed: false, - }); - }; - match cancel_authority(window.label(), &entry.source_label, &token, entry.target_label.as_deref()) { - CancelAuthority::Source => Ok(state.cancel_from_source(&token, Instant::now())), - CancelAuthority::Destination => Ok(state.cancel_from_destination(&token)), - CancelAuthority::Forbidden => Err(format!( - "TRANSFER_FORBIDDEN: window '{}' may not cancel this tab transfer", - window.label() - )), - } + state.cancel_as(window.label(), &token, Instant::now()) } #[cfg(test)] @@ -597,4 +623,132 @@ mod tests { assert!(broker.peek(&tokens[0]).is_none()); assert!(broker.peek(&tokens[MAX_PENDING_TRANSFERS]).is_some()); } + + // The tests above take the broker and the authorisation predicates apart. + // These four put them back together and run a transfer from `stage` to + // `complete`, because the defect #452 fixed was in the composition: the + // gate matched the predicate it was written against, the broker held the + // payload it was asked to hold, and the path between them was closed. + + /// A window that already exists when the transfer starts. Either `main` + /// or, as here, a window some earlier transfer created — so its label + /// derives from a *different* token. `window-` for the token being + /// transferred is the one label such a window can never have. + const EXISTING_WINDOW: &str = "window-1f0e9d8c7b6a5948372615043f2e1d0c"; + + // Move to Window > (an open window). `offer_tab_to_window` records the + // target before it notifies it; without that record the destination is + // not `window-` and every step below is TRANSFER_FORBIDDEN. + #[test] + fn a_transfer_to_an_existing_window_runs_end_to_end() { + let broker = TabTransferBroker::new(); + let token = broker.stage("{\"tab\":\"notes.md\"}".to_string(), "main".to_string()); + broker + .set_target_label(&token, EXISTING_WINDOW.to_string()) + .expect("a staged transfer should accept the target it is offered to"); + + let payload = broker + .claim_as(EXISTING_WINDOW, &token, Instant::now()) + .expect("the window offer_tab_to_window named is a destination and may claim") + .expect("the staged payload should still be there"); + assert_eq!(payload, "{\"tab\":\"notes.md\"}"); + + let transfer = broker + .complete_as(EXISTING_WINDOW, &token) + .expect("the window that claimed the payload must be able to complete the hand-off"); + assert_eq!( + transfer.source_label, "main", + "completion names the source so it is the window told to drop its tab" + ); + assert!( + broker.peek(&token).is_none(), + "a completed transfer must leave nothing behind for a second claim" + ); + } + + // Move to New Window. `create_transfer_window` builds `window-` + // and nothing records a target, so this path must keep working off the + // derived label alone. + #[test] + fn a_transfer_to_a_new_window_runs_end_to_end() { + let broker = TabTransferBroker::new(); + let token = broker.stage("{\"tab\":\"draft.md\"}".to_string(), "main".to_string()); + let destination = destination_label(&token); + + let payload = broker + .claim_as(&destination, &token, Instant::now()) + .expect("the window the token names is a destination and may claim") + .expect("the staged payload should still be there"); + assert_eq!(payload, "{\"tab\":\"draft.md\"}"); + + let transfer = broker + .complete_as(&destination, &token) + .expect("the window the token names must be able to complete the hand-off"); + assert_eq!(transfer.source_label, "main"); + assert!(broker.peek(&token).is_none()); + } + + // Widening the gate to the recorded target must not widen it to everyone: + // the payload is a document the source window had open. + #[test] + fn a_third_party_window_is_refused_at_every_step() { + const BYSTANDER: &str = "window-00000000000000000000000000000000"; + + let broker = TabTransferBroker::new(); + let token = broker.stage("{\"tab\":\"private.md\"}".to_string(), "main".to_string()); + broker + .set_target_label(&token, EXISTING_WINDOW.to_string()) + .expect("a staged transfer should accept the target it is offered to"); + + for refusal in [ + broker.claim_as(BYSTANDER, &token, Instant::now()).err(), + broker.complete_as(BYSTANDER, &token).err(), + broker.cancel_as(BYSTANDER, &token, Instant::now()).err(), + ] { + let message = refusal.expect( + "neither the source, nor window-, nor the recorded target: refuse it", + ); + assert!(message.starts_with("TRANSFER_FORBIDDEN"), "{message}"); + } + + // The refusals are inert: the transfer is untouched and its actual + // destination can still take it. + assert_eq!( + broker + .claim_as(EXISTING_WINDOW, &token, Instant::now()) + .expect("the real destination must not be affected by the refusals") + .as_deref(), + Some("{\"tab\":\"private.md\"}") + ); + } + + // A claim makes the source's timeout a no-op, so the window holding it is + // the only party that can end it. If the recorded target were refused + // here, a failed render would strand the tab in both windows. + #[test] + fn the_recorded_destination_may_release_its_own_claim() { + let broker = TabTransferBroker::new(); + let token = broker.stage("{\"tab\":\"notes.md\"}".to_string(), "main".to_string()); + broker + .set_target_label(&token, EXISTING_WINDOW.to_string()) + .expect("a staged transfer should accept the target it is offered to"); + broker + .claim_as(EXISTING_WINDOW, &token, Instant::now()) + .expect("the recorded target may claim"); + + let outcome = broker + .cancel_as(EXISTING_WINDOW, &token, Instant::now()) + .expect("the window holding the claim must be able to release it"); + assert_eq!( + outcome, + CancelOutcome { + cancelled: true, + claimed: true + } + ); + assert!( + broker.peek(&token).is_none(), + "a released claim must free the transfer, not leave it claimed forever" + ); + } }