diff --git a/src-tauri/src/properties_window.rs b/src-tauri/src/properties_window.rs index c5a4337..549de19 100644 --- a/src-tauri/src/properties_window.rs +++ b/src-tauri/src/properties_window.rs @@ -12,6 +12,8 @@ const PROPERTIES_WINDOW_TITLE: &str = "Properties - Firelink"; const PROPERTIES_WINDOW_READY_EVENT: &str = "properties-window-ready"; const PROPERTIES_WINDOW_ACTION_REQUEST_EVENT: &str = "properties-window-action-request"; const MAX_PROPERTIES_ACTION_PAYLOAD_BYTES: usize = 64 * 1024; +const MAX_PROPERTIES_SESSION_ID_BYTES: usize = 128; +const MAX_PROPERTIES_REQUEST_ID: u64 = 9_007_199_254_740_991; #[derive(Default)] pub struct PropertiesWindowRegistry { @@ -23,6 +25,7 @@ struct RegistryState { by_download: HashMap, by_window: HashMap, ready_windows: HashSet, + sessions_by_window: HashMap, } #[derive(Clone, Serialize)] @@ -30,6 +33,7 @@ struct RegistryState { struct PropertiesWindowReadyEvent { window_label: String, download_id: String, + session_id: String, } #[derive(Clone, Serialize)] @@ -37,6 +41,7 @@ struct PropertiesWindowReadyEvent { struct PropertiesWindowActionEvent { window_label: String, download_id: String, + session_id: String, request_id: u64, action: String, payload: Option, @@ -75,6 +80,7 @@ impl PropertiesWindowRegistry { .map_err(|_| "Properties window registry is unavailable".to_string())?; let download_id = state.by_window.remove(label); state.ready_windows.remove(label); + state.sessions_by_window.remove(label); if let Some(download_id) = &download_id { state.by_download.remove(download_id); } @@ -90,6 +96,7 @@ impl PropertiesWindowRegistry { if let Some(label) = &label { state.by_window.remove(label); state.ready_windows.remove(label); + state.sessions_by_window.remove(label); } Ok(label) } @@ -116,6 +123,34 @@ impl PropertiesWindowRegistry { Ok(()) } + pub fn register_session(&self, label: &str, session_id: &str) -> Result<(), String> { + let mut state = self + .state + .lock() + .map_err(|_| "Properties window registry is unavailable".to_string())?; + if !state.by_window.contains_key(label) { + return Err("Properties window is no longer registered".to_string()); + } + state + .sessions_by_window + .insert(label.to_string(), session_id.to_string()); + Ok(()) + } + + pub fn session_for_window(&self, label: &str) -> Result, String> { + Ok(self + .state + .lock() + .map_err(|_| "Properties window registry is unavailable".to_string())? + .sessions_by_window + .get(label) + .cloned()) + } + + pub fn session_matches(&self, label: &str, session_id: &str) -> Result { + Ok(self.session_for_window(label)?.as_deref() == Some(session_id)) + } + pub fn is_ready(&self, label: &str) -> Result { Ok(self .state @@ -211,6 +246,25 @@ fn validate_download_id(download_id: &str) -> Result<(), String> { Ok(()) } +fn validate_properties_session_id(session_id: &str) -> Result<(), String> { + if session_id.is_empty() + || session_id.len() > MAX_PROPERTIES_SESSION_ID_BYTES + || !session_id + .bytes() + .all(|byte| byte.is_ascii_alphanumeric() || matches!(byte, b'-' | b'_')) + { + return Err("Invalid Properties window session".to_string()); + } + Ok(()) +} + +fn validate_properties_request_id(request_id: u64) -> Result<(), String> { + if request_id == 0 || request_id > MAX_PROPERTIES_REQUEST_ID { + return Err("Invalid Properties action request ID".to_string()); + } + Ok(()) +} + #[tauri::command] pub fn open_download_properties_window( app: tauri::AppHandle, @@ -282,14 +336,18 @@ pub fn properties_window_send_ready( caller: tauri::WebviewWindow, app: tauri::AppHandle, registry: tauri::State<'_, PropertiesWindowRegistry>, + session_id: String, ) -> Result<(), String> { + validate_properties_session_id(&session_id)?; let download_id = registered_download_for_caller(&caller, ®istry)?; + registry.register_session(caller.label(), &session_id)?; app.emit_to( MAIN_WINDOW_LABEL, PROPERTIES_WINDOW_READY_EVENT, PropertiesWindowReadyEvent { window_label: caller.label().to_string(), download_id, + session_id, }, ) .map_err(|error| error.to_string()) @@ -311,10 +369,13 @@ pub fn properties_window_send_action( caller: tauri::WebviewWindow, app: tauri::AppHandle, registry: tauri::State<'_, PropertiesWindowRegistry>, + session_id: String, request_id: u64, action: String, payload: Option, ) -> Result<(), String> { + validate_properties_session_id(&session_id)?; + validate_properties_request_id(request_id)?; if !is_properties_action(&action) || action.len() > 64 || action.chars().any(char::is_control) @@ -330,12 +391,16 @@ pub fn properties_window_send_action( } } let download_id = registered_download_for_caller(&caller, ®istry)?; + if !registry.session_matches(caller.label(), &session_id)? { + return Err("Properties window session is no longer current".to_string()); + } app.emit_to( MAIN_WINDOW_LABEL, PROPERTIES_WINDOW_ACTION_REQUEST_EVENT, PropertiesWindowActionEvent { window_label: caller.label().to_string(), download_id, + session_id, request_id, action, payload, @@ -350,17 +415,26 @@ pub fn validate_properties_window_request( registry: tauri::State<'_, PropertiesWindowRegistry>, window_label: String, download_id: String, + session_id: String, + request_id: Option, ) -> Result<(), String> { if caller.label() != MAIN_WINDOW_LABEL { return Err("Only the main window can validate Properties requests".to_string()); } validate_download_id(&download_id)?; + validate_properties_session_id(&session_id)?; + if let Some(request_id) = request_id { + validate_properties_request_id(request_id)?; + } if !is_properties_window_label(&window_label) { return Err("Invalid Properties window label".to_string()); } if registry.download_for_window(&window_label)?.as_deref() != Some(download_id.as_str()) { return Err("Properties window request does not match its registered download".to_string()); } + if !registry.session_matches(&window_label, &session_id)? { + return Err("Properties window session is no longer current".to_string()); + } Ok(()) } @@ -442,6 +516,27 @@ mod tests { assert!(validate_download_id("").is_err()); assert!(validate_download_id("\n").is_err()); assert!(validate_download_id("valid-id").is_ok()); + assert!(validate_properties_session_id("session-1").is_ok()); + assert!(validate_properties_session_id("").is_err()); + assert!(validate_properties_session_id("bad session").is_err()); + assert!(validate_properties_request_id(1).is_ok()); + assert!(validate_properties_request_id(0).is_err()); + } + + #[test] + fn registering_a_new_session_invalidates_the_previous_session() { + let registry = PropertiesWindowRegistry::default(); + let label = registry.allocate("download-a").unwrap(); + + registry.register_session(&label, "session-old").unwrap(); + assert!(registry.session_matches(&label, "session-old").unwrap()); + + registry.register_session(&label, "session-new").unwrap(); + assert!(!registry.session_matches(&label, "session-old").unwrap()); + assert!(registry.session_matches(&label, "session-new").unwrap()); + + registry.remove_window(&label).unwrap(); + assert!(!registry.session_matches(&label, "session-new").unwrap()); } #[test] diff --git a/src/components/PropertiesWindowApp.tsx b/src/components/PropertiesWindowApp.tsx index 93c6349..575804e 100644 --- a/src/components/PropertiesWindowApp.tsx +++ b/src/components/PropertiesWindowApp.tsx @@ -50,6 +50,7 @@ export const PropertiesWindowApp = () => { const { t } = useTranslation(); const currentWindow = useMemo(() => getCurrentWindow(), []); const windowLabel = currentWindow.label; + const sessionId = useMemo(() => crypto.randomUUID(), []); const [downloadId, setDownloadId] = useState(null); const [snapshot, setSnapshot] = useState(null); const [activeTab, setActiveTab] = useState('overview'); @@ -190,7 +191,9 @@ export const PropertiesWindowApp = () => { if (cancelled) return; setDownloadId(id); unlistenSnapshot = await listen(PROPERTIES_WINDOW_SNAPSHOT, async event => { - if (event.payload.windowLabel !== windowLabel || event.payload.downloadId !== id) return; + if (event.payload.windowLabel !== windowLabel + || event.payload.downloadId !== id + || event.payload.sessionId !== sessionId) return; if (event.payload.revision <= latestSnapshotRevisionRef.current) return; latestSnapshotRevisionRef.current = event.payload.revision; await changeAppLocale(event.payload.snapshot.appearance.locale); @@ -218,7 +221,9 @@ export const PropertiesWindowApp = () => { } }); unlistenResult = await listen(PROPERTIES_WINDOW_ACTION_RESULT, event => { - if (event.payload.windowLabel !== windowLabel || event.payload.downloadId !== id) return; + if (event.payload.windowLabel !== windowLabel + || event.payload.downloadId !== id + || event.payload.sessionId !== sessionId) return; if (event.payload.requestId !== requestIdRef.current) return; setIsSaving(false); const completedAction = pendingActionRef.current; @@ -253,7 +258,7 @@ export const PropertiesWindowApp = () => { setNotice(t($ => $.downloadTable.noDownloads)); } }); - await sendPropertiesReady(); + await sendPropertiesReady(sessionId); if (cancelled) return; // Tauri event listeners are registered asynchronously. If the main // bridge was still installing its listener, the first ready event can @@ -261,7 +266,7 @@ export const PropertiesWindowApp = () => { // the handshake rather than leaving a permanently hidden window. readyRetryTimer = window.setInterval(() => { if (cancelled || hasRevealedWindowRef.current) return; - void sendPropertiesReady().catch(() => undefined); + void sendPropertiesReady(sessionId).catch(() => undefined); }, 500); } catch (error) { if (!cancelled) setErrorMessage(errorText(error)); @@ -277,7 +282,7 @@ export const PropertiesWindowApp = () => { appearanceCleanupRef.current?.(); appearanceCleanupRef.current = null; }; - }, [currentWindow, hydrateDraft, t, windowLabel]); + }, [currentWindow, hydrateDraft, sessionId, t, windowLabel]); useEffect(() => { if (!snapshot || draftTab !== null) return; @@ -331,6 +336,7 @@ export const PropertiesWindowApp = () => { await sendPropertiesActionRequest({ windowLabel, downloadId, + sessionId, requestId, action, payload, @@ -342,7 +348,7 @@ export const PropertiesWindowApp = () => { switchAfterSaveRef.current = null; setErrorMessage(errorText(error)); } - }, [downloadId, windowLabel]); + }, [downloadId, sessionId, windowLabel]); const applyActiveTab = useCallback(async () => { if (!snapshot || !isEditableStatus(snapshot.status)) { diff --git a/src/components/PropertiesWindowBridgeHost.tsx b/src/components/PropertiesWindowBridgeHost.tsx index 85da7a2..aeff1f4 100644 --- a/src/components/PropertiesWindowBridgeHost.tsx +++ b/src/components/PropertiesWindowBridgeHost.tsx @@ -16,13 +16,16 @@ import { applySecretPatch, beginExclusivePropertiesAction, createFrameCoalescer, + enqueuePropertiesAction, getPropertiesLifecycleAction, sanitizePropertiesSnapshot, sendPropertiesActionResult, sendPropertiesRemoved, sendPropertiesSnapshot, + shouldAcceptPropertiesActionRequest, type PropertiesActionRequest, type PropertiesPatch, + type PropertiesWindowRegistration, type PropertiesWindowReady, } from '../propertiesBridge'; import { invokeCommand as invoke } from '../ipc'; @@ -108,25 +111,28 @@ const copyEditablePatch = (rawPatch: PropertiesPatch): Partial => export const PropertiesWindowBridgeHost = () => { useEffect(() => { - const windows = new Map(); + const windows = new Map(); const snapshotRevisions = new Map(); const actionsInFlight = new Set(); + const actionChains = new Map>(); let disposed = false; let unlistenReady: UnlistenFn | undefined; let unlistenAction: UnlistenFn | undefined; let unlistenClosed: UnlistenFn | undefined; const snapshotCoalescer = createFrameCoalescer( windowLabel => { - const downloadId = windows.get(windowLabel); - if (downloadId) void sendFor(windowLabel, downloadId).catch(() => undefined); + const registration = windows.get(windowLabel); + if (registration) void sendFor(windowLabel, registration.downloadId).catch(() => undefined); }, callback => window.requestAnimationFrame(callback), handle => window.cancelAnimationFrame(handle), ); const sendFor = async (windowLabel: string, downloadId: string) => { + const registration = windows.get(windowLabel); + if (!registration || registration.downloadId !== downloadId || disposed) return false; const item = useDownloadStore.getState().downloads.find(download => download.id === downloadId); - if (!item || disposed) return false; + if (!item) return false; const settings = useSettingsStore.getState(); const progress = useDownloadProgressStore.getState(); const revision = (snapshotRevisions.get(windowLabel) ?? 0) + 1; @@ -134,6 +140,7 @@ export const PropertiesWindowBridgeHost = () => { await sendPropertiesSnapshot(windowLabel, { windowLabel, downloadId, + sessionId: registration.sessionId, revision, snapshot: sanitizePropertiesSnapshot(item, { theme: settings.theme, @@ -149,6 +156,25 @@ export const PropertiesWindowBridgeHost = () => { return true; }; + const synchronizeRegistration = ( + windowLabel: string, + downloadId: string, + sessionId: string, + ) => { + const previous = windows.get(windowLabel); + const sessionChanged = previous?.downloadId !== downloadId || previous.sessionId !== sessionId; + windows.set(windowLabel, { + downloadId, + sessionId, + latestRequestId: sessionChanged ? 0 : (previous?.latestRequestId ?? 0), + }); + if (sessionChanged) { + snapshotRevisions.set(windowLabel, 0); + } else if (!snapshotRevisions.has(windowLabel)) { + snapshotRevisions.set(windowLabel, 0); + } + }; + const handleReady = async (payload: PropertiesWindowReady) => { try { await invoke('validate_properties_window_request', payload); @@ -157,8 +183,7 @@ export const PropertiesWindowBridgeHost = () => { await sendPropertiesRemoved(payload.windowLabel, payload.downloadId); return; } - windows.set(payload.windowLabel, payload.downloadId); - if (!snapshotRevisions.has(payload.windowLabel)) snapshotRevisions.set(payload.windowLabel, 0); + synchronizeRegistration(payload.windowLabel, payload.downloadId, payload.sessionId); await sendFor(payload.windowLabel, payload.downloadId); } catch { // The child will show its own unavailable state. Do not log bridge @@ -166,14 +191,17 @@ export const PropertiesWindowBridgeHost = () => { } }; - const handleAction = async (request: PropertiesActionRequest) => { + const processAction = async (request: PropertiesActionRequest) => { let ok = false; let error: string | undefined; const actionKey = `${request.windowLabel}:${request.downloadId}`; let releaseAction: (() => void) | undefined; try { await invoke('validate_properties_window_request', request); - if (windows.get(request.windowLabel) !== request.downloadId) { + const registration = windows.get(request.windowLabel); + if (!registration + || registration.downloadId !== request.downloadId + || registration.sessionId !== request.sessionId) { throw new Error('Properties window is no longer registered'); } releaseAction = beginExclusivePropertiesAction(actionsInFlight, actionKey); @@ -285,6 +313,7 @@ export const PropertiesWindowBridgeHost = () => { await sendPropertiesActionResult(request.windowLabel, { windowLabel: request.windowLabel, downloadId: request.downloadId, + sessionId: request.sessionId, requestId: request.requestId, ok, ...(error ? { error } : {}), @@ -295,18 +324,44 @@ export const PropertiesWindowBridgeHost = () => { } }; + const handleAction = async (request: PropertiesActionRequest) => { + const actionKey = `${request.windowLabel}:${request.downloadId}`; + try { + // The native command validates the caller, download binding, and + // renderer session. If a ready event is delayed or lost, this valid + // action can also establish the main-window registration. + await invoke('validate_properties_window_request', request); + synchronizeRegistration(request.windowLabel, request.downloadId, request.sessionId); + } catch { + // Stale renderer actions are deliberately ignored. The current child + // session cannot safely consume a result for a superseded renderer. + return; + } + + const registration = windows.get(request.windowLabel); + if (!registration) return; + if (!shouldAcceptPropertiesActionRequest(registration, request)) return; + registration.latestRequestId = request.requestId; + + // Preserve user order for accepted requests. This keeps a pause from an + // earlier request from running after a newer resume, while still + // allowing the newer request to run after an already-started operation. + await enqueuePropertiesAction(actionChains, actionKey, () => processAction(request)); + }; + void listen(PROPERTIES_WINDOW_READY, event => void handleReady(event.payload)).then(value => { unlistenReady = value; }); void listen(PROPERTIES_WINDOW_ACTION_REQUEST, event => void handleAction(event.payload)).then(value => { unlistenAction = value; }); void listen(PROPERTIES_WINDOW_CLOSED, event => { - const downloadId = windows.get(event.payload); + const registration = windows.get(event.payload); windows.delete(event.payload); snapshotRevisions.delete(event.payload); snapshotCoalescer.cancel(event.payload); - if (downloadId) actionsInFlight.delete(`${event.payload}:${downloadId}`); + if (registration) actionsInFlight.delete(`${event.payload}:${registration.downloadId}`); }).then(value => { unlistenClosed = value; }); const unsubscribeStore = useDownloadStore.subscribe((state, previous) => { - for (const [windowLabel, downloadId] of windows) { + for (const [windowLabel, registration] of windows) { + const { downloadId } = registration; const next = state.downloads.find(download => download.id === downloadId); const before = previous.downloads.find(download => download.id === downloadId); if (!next) { @@ -321,7 +376,8 @@ export const PropertiesWindowBridgeHost = () => { } }); const unsubscribeProgress = useDownloadProgressStore.subscribe((state, previous) => { - for (const [windowLabel, downloadId] of windows) { + for (const [windowLabel, registration] of windows) { + const { downloadId } = registration; if (state.progressMap[downloadId] !== previous.progressMap[downloadId] || state.moveProgressMap[downloadId] !== previous.moveProgressMap[downloadId]) { snapshotCoalescer.schedule(windowLabel); diff --git a/src/ipc.ts b/src/ipc.ts index e8e2f99..42983be 100644 --- a/src/ipc.ts +++ b/src/ipc.ts @@ -157,10 +157,10 @@ type CommandMap = { remove_from_queue: { args: { id: string }; result: boolean }; open_download_properties_window: { args: { id: string }; result: string }; get_properties_window_download_id: { args: undefined; result: string }; - properties_window_send_ready: { args: undefined; result: void }; + properties_window_send_ready: { args: { sessionId: string }; result: void }; properties_window_reveal: { args: undefined; result: void }; - properties_window_send_action: { args: { requestId: number; action: string; payload?: unknown }; result: void }; - validate_properties_window_request: { args: { windowLabel: string; downloadId: string }; result: void }; + properties_window_send_action: { args: { sessionId: string; requestId: number; action: string; payload?: unknown }; result: void }; + validate_properties_window_request: { args: { windowLabel: string; downloadId: string; sessionId: string; requestId?: number }; result: void }; close_download_properties_window: { args: { id: string }; result: void }; properties_window_registry_remove_for_download: { args: { id: string }; result: void }; }; diff --git a/src/propertiesBridge.test.ts b/src/propertiesBridge.test.ts index 0b94515..379526e 100644 --- a/src/propertiesBridge.test.ts +++ b/src/propertiesBridge.test.ts @@ -14,9 +14,11 @@ import { applySecretPatch, beginExclusivePropertiesAction, createFrameCoalescer, + enqueuePropertiesAction, getPropertiesLifecycleAction, isExpectedPropertiesDiagnosticUnavailable, sanitizePropertiesSnapshot, + shouldAcceptPropertiesActionRequest, } from './propertiesBridge'; describe('Properties window bridge', () => { @@ -157,6 +159,72 @@ describe('Properties window bridge', () => { expect(inFlight.has('window:download')).toBe(false); }); + it('rejects actions from a superseded renderer session and older request IDs', () => { + const registration = { + downloadId: 'download-1', + sessionId: 'session-new', + latestRequestId: 4, + }; + + expect(shouldAcceptPropertiesActionRequest(registration, { + downloadId: 'download-1', + sessionId: 'session-old', + requestId: 99, + })).toBe(false); + expect(shouldAcceptPropertiesActionRequest(registration, { + downloadId: 'download-1', + sessionId: 'session-new', + requestId: 4, + })).toBe(false); + expect(shouldAcceptPropertiesActionRequest(registration, { + downloadId: 'download-1', + sessionId: 'session-new', + requestId: 5, + })).toBe(true); + }); + + it('rejects a request whose download binding does not match the window', () => { + expect(shouldAcceptPropertiesActionRequest({ + downloadId: 'download-1', + sessionId: 'session-1', + latestRequestId: 0, + }, { + downloadId: 'download-2', + sessionId: 'session-1', + requestId: 1, + })).toBe(false); + }); + + it('serializes actions per window and continues after an earlier action fails', async () => { + const chains = new Map>(); + const events: string[] = []; + let releaseFirst!: () => void; + let markFirstStarted!: () => void; + const firstGate = new Promise(resolve => { releaseFirst = resolve; }); + const firstStarted = new Promise(resolve => { markFirstStarted = resolve; }); + + const first = enqueuePropertiesAction(chains, 'window:download', async () => { + events.push('first-start'); + markFirstStarted(); + await firstGate; + events.push('first-end'); + throw new Error('first action failed'); + }); + const second = enqueuePropertiesAction(chains, 'window:download', async () => { + events.push('second'); + }); + + await firstStarted; + expect(events).toEqual(['first-start']); + releaseFirst(); + const results = await Promise.allSettled([first, second]); + + expect(results[0].status).toBe('rejected'); + expect(results[1].status).toBe('fulfilled'); + expect(events).toEqual(['first-start', 'first-end', 'second']); + expect(chains.size).toBe(0); + }); + it('coalesces repeated snapshot requests to one callback per animation frame', () => { const frames = new Map(); const delivered: string[] = []; diff --git a/src/propertiesBridge.ts b/src/propertiesBridge.ts index c3045e8..b062399 100644 --- a/src/propertiesBridge.ts +++ b/src/propertiesBridge.ts @@ -154,11 +154,13 @@ export const beginExclusivePropertiesAction = ( export type PropertiesWindowReady = { windowLabel: string; downloadId: string; + sessionId: string; }; export type PropertiesActionRequest = { windowLabel: string; downloadId: string; + sessionId: string; requestId: number; action: PropertiesAction; payload?: PropertiesPatch | { limit: string | null } | { maxPeers: string | null; peerSpeedLimit: string | null }; @@ -167,6 +169,7 @@ export type PropertiesActionRequest = { export type PropertiesActionResult = { windowLabel: string; downloadId: string; + sessionId: string; requestId: number; ok: boolean; error?: string; @@ -175,10 +178,41 @@ export type PropertiesActionResult = { export type PropertiesSnapshotEvent = { windowLabel: string; downloadId: string; + sessionId: string; revision: number; snapshot: PropertiesSnapshot; }; +export type PropertiesWindowRegistration = { + downloadId: string; + sessionId: string; + latestRequestId: number; +}; + +export const shouldAcceptPropertiesActionRequest = ( + registration: PropertiesWindowRegistration | undefined, + request: Pick, +): boolean => registration !== undefined + && registration.downloadId === request.downloadId + && registration.sessionId === request.sessionId + && Number.isSafeInteger(request.requestId) + && request.requestId > registration.latestRequestId; + +export const enqueuePropertiesAction = ( + chains: Map>, + key: string, + action: () => Promise, +): Promise => { + const previous = chains.get(key) ?? Promise.resolve(); + const operation = previous.catch(() => undefined).then(action); + let tracked: Promise; + tracked = operation.finally(() => { + if (chains.get(key) === tracked) chains.delete(key); + }); + chains.set(key, tracked); + return tracked; +}; + const copyWithoutSecrets = ( item: DownloadItem, appearance: DocumentAppearance, @@ -272,11 +306,12 @@ export const createFrameCoalescer = ( export const openPropertiesWindow = (downloadId: string): Promise => invoke('open_download_properties_window', { id: downloadId }); -export const sendPropertiesReady = (): Promise => - invoke('properties_window_send_ready'); +export const sendPropertiesReady = (sessionId: string): Promise => + invoke('properties_window_send_ready', { sessionId }); export const sendPropertiesActionRequest = (payload: PropertiesActionRequest): Promise => invoke('properties_window_send_action', { + sessionId: payload.sessionId, requestId: payload.requestId, action: payload.action, payload: payload.payload,