diff --git a/package.json b/package.json index c8a077f..2ec1c68 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "omnideck-desktop", "private": true, - "version": "0.5.0-alpha.3", + "version": "0.5.0-alpha4", "type": "module", "scripts": { "dev": "vite", diff --git a/src-tauri/Cargo.lock b/src-tauri/Cargo.lock index 0e42fb4..b11b3c6 100644 --- a/src-tauri/Cargo.lock +++ b/src-tauri/Cargo.lock @@ -2269,7 +2269,7 @@ dependencies = [ [[package]] name = "omnideck-desktop" -version = "0.5.0-alpha.3" +version = "0.5.0-alpha4" dependencies = [ "serde", "serde_json", diff --git a/src-tauri/Cargo.toml b/src-tauri/Cargo.toml index e0ba9d2..fb73834 100644 --- a/src-tauri/Cargo.toml +++ b/src-tauri/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "omnideck-desktop" -version = "0.5.0-alpha.3" +version = "0.5.0-alpha4" description = "Omnideck desktop app — a thin GUI shell over the omnideck CLI" authors = ["Omnideck"] edition = "2021" diff --git a/src-tauri/src/bootstrap.rs b/src-tauri/src/bootstrap.rs index 3129f2c..8983500 100644 --- a/src-tauri/src/bootstrap.rs +++ b/src-tauri/src/bootstrap.rs @@ -15,23 +15,38 @@ //! **No separate window** (a real change from an earlier version of this //! module, not the original design): onboarding was originally an isolated //! window, hidden until needed, mirroring the sibling's `hosted-app`/setup -//! window split. That caused a real, confirmed bug — creating two GTK/ -//! WebKit windows at startup (one hidden) failed EGL/GPU-driver -//! initialization on at least one real Intel/Mesa combination, with a -//! blank white dashboard as the symptom, and was independently reproduced -//! in the sibling repo's own build too (same two-window-at-startup -//! pattern). Fixed by dropping the second window entirely: `bootstrap`/ -//! `begin_setup`/`run_action` now run from the single `"main"` window, and +//! window split. That was suspected (wrongly, see below) to have caused a +//! real, confirmed bug — an `EGL_BAD_PARAMETER` failure at startup on at +//! least one real Intel/Mesa combination, blank white dashboard as the +//! symptom. Fixed by dropping the second window: `bootstrap`/`begin_setup`/ +//! `run_action` now run from the single `"main"` window, and //! `src/components/OnboardingView.tsx` / `DashboardView.tsx` are just two //! screens React swaps between based on the pushed [`SetupState`] — no -//! window to show or hide, so no `open_dashboard` command either. This is -//! a real, knowingly-accepted security tradeoff: these commands are no -//! longer isolated from the dashboard's own broader command surface the -//! way a separate window's capability grant would enforce. What's kept: -//! the state machine itself, and the server-side offered-actions allowlist -//! (`run_action` only accepts what the *last pushed state* actually -//! offered) — see `reference/desktop-hardening-migration-PLAN.md`'s -//! "Decisions from review" for the full history of this decision. +//! window to show or hide, so no `open_dashboard` command either. +//! +//! **No `Channel`, no `WebviewWindow` parameter either** (a second, later +//! correction): the single-window fix above shipped and still reproduced +//! the identical `EGL_BAD_PARAMETER` crash on the same real hardware. That +//! ruled out window *count* as the cause — the actual variable was that the +//! very first "confirmed fix" test never exercised `bootstrap` at all +//! (disabling the second window's creation also meant its JS, the only +//! thing that called `bootstrap`, never ran). Two mechanisms in this module +//! were — before this fix — unique in the whole codebase: `tauri::ipc:: +//! Channel` for pushing [`SetupState`] (every other stream, `add`/`update`/ +//! `remove`'s progress, uses plain `app.emit()` + frontend `listen()`, +//! proven to work in production), and a `WebviewWindow` injected command +//! parameter (every other command takes only `AppHandle`). Rather than +//! guess further, this module was converted to match the rest of the app +//! exactly: `app.emit("setup-state", ...)` instead of a `Channel`, and +//! `AppHandle`-only command signatures — dropping the manual +//! `window.label() == "main"` check along with it, since the capability +//! grant's own `"windows": ["main"]` scoping (`capabilities/default.json`) +//! already enforces the same thing at the Tauri-core level, exactly as it +//! does for every other command here; the manual check was redundant +//! defense-in-depth, never the only guard. If a future test confirms this +//! *also* wasn't the cause, the next thing to suspect is the CLI subprocess +//! spawn itself (`cli_bridge::runtime_status`) happening during initial +//! webview mount — untested in isolation as of this writing. //! //! Scope boundary: this module's job ends once the shared runtime is ready. //! Creating a Deck (pulling the omnideck image, provisioning a container) is @@ -57,7 +72,15 @@ use std::{ Arc, RwLock, }, }; -use tauri::{ipc::Channel, AppHandle, WebviewWindow}; +use tauri::{AppHandle, Emitter}; + +/// The single event name every pushed [`SetupState`] goes out under — +/// deliberately one fixed name, not per-call-site names the way `add`/ +/// `update`/`remove`'s progress events are (`"add-progress"` etc.): there's +/// only ever one onboarding screen listening, and only one bootstrap flow +/// running at a time (`BootstrapState::setup_running` already enforces +/// that), so there's nothing for a second name to disambiguate. +const SETUP_STATE_EVENT: &str = "setup-state"; /// Phases in weighted-progress order, matching `runtime ensure`'s own stage /// vocabulary exactly (`engine.SetupStageSoftware`/`SetupStageEnvironment` @@ -90,11 +113,12 @@ pub struct Diagnostic { pub status: String, } -/// Pushed from Rust to the dashboard's React app over a Tauri [`Channel`]. -/// `OnboardingView`'s `render(state)`-style logic fully re-derives what it -/// shows from each pushed state; `App.tsx` decides whether to render -/// `OnboardingView` or `DashboardView` based on `stage`/`canOpen`. Modeled -/// on the sibling's `SetupState` in `parity.rs`. +/// Pushed from Rust to the dashboard's React app as a `"setup-state"` Tauri +/// event (`app.emit`), the same mechanism `add`/`update`/`remove`'s +/// progress already use. `OnboardingView`'s `render(state)`-style logic +/// fully re-derives what it shows from each pushed state; `App.tsx` decides +/// whether to render `OnboardingView` or `DashboardView` based on +/// `stage`/`canOpen`. Modeled on the sibling's `SetupState` in `parity.rs`. #[derive(Clone, Debug, Serialize, PartialEq)] #[serde(rename_all = "camelCase")] pub struct SetupState { @@ -269,16 +293,11 @@ fn error_state(error: &CliError, reached_phase: usize) -> SetupState { /// Every failure mode of the 3 bootstrap commands themselves — distinct /// from [`CliError`] (which is specifically about CLI subprocess failures), -/// since these also cover the origin and action-allowlist checks that have -/// nothing to do with the CLI. +/// since these also cover the action-allowlist check and event emission, +/// which have nothing to do with the CLI. #[derive(Debug, Serialize)] #[serde(tag = "kind", rename_all = "snake_case")] pub enum BootstrapError { - /// The caller wasn't the `"main"` window. With no separate onboarding - /// window left to enforce this via a capability grant, this check is - /// the only remaining guard — see this module's doc comment for why - /// that's a real, accepted tradeoff, not an oversight. - OriginDenied, Cli(CliError), /// The requested `run_action` wasn't in the *current* state's offered /// actions — stops a compromised/buggy webview invoking something the @@ -294,13 +313,6 @@ impl From for BootstrapError { } } -fn authorize_main(window: &WebviewWindow) -> Result<(), BootstrapError> { - if window.label() != "main" { - return Err(BootstrapError::OriginDenied); - } - Ok(()) -} - #[derive(Clone)] pub struct BootstrapState { setup_running: Arc, @@ -317,8 +329,8 @@ impl Default for BootstrapState { } fn send_state( + app: &AppHandle, state: &BootstrapState, - channel: &Channel, setup_state: SetupState, ) -> Result<(), BootstrapError> { let mut actions = state @@ -329,8 +341,7 @@ fn send_state( actions.extend(setup_state.primary_action.iter().cloned()); actions.extend(setup_state.secondary_action.iter().cloned()); drop(actions); - channel - .send(setup_state) + app.emit(SETUP_STATE_EVENT, setup_state) .map_err(|_| BootstrapError::StateDeliveryFailed) } @@ -386,26 +397,22 @@ fn debug_forced_state() -> Option { #[tauri::command] pub async fn bootstrap( app: AppHandle, - window: WebviewWindow, state: tauri::State<'_, BootstrapState>, - on_event: Channel, ) -> Result<(), BootstrapError> { - authorize_main(&window)?; - if let Some(forced) = debug_forced_state() { - send_state(&state, &on_event, forced)?; + send_state(&app, &state, forced)?; return Ok(()); } match cli_bridge::runtime_status(&app).await { Ok(status) if status.ready => { - send_state(&state, &on_event, ready_state())?; + send_state(&app, &state, ready_state())?; } Ok(_) => { - send_state(&state, &on_event, welcome_state())?; + send_state(&app, &state, welcome_state())?; } Err(error) => { - send_state(&state, &on_event, error_state(&error, 0))?; + send_state(&app, &state, error_state(&error, 0))?; } } Ok(()) @@ -417,18 +424,15 @@ pub async fn bootstrap( #[tauri::command] pub async fn begin_setup( app: AppHandle, - window: WebviewWindow, state: tauri::State<'_, BootstrapState>, - on_event: Channel, ) -> Result<(), BootstrapError> { - authorize_main(&window)?; if state.setup_running.swap(true, Ordering::AcqRel) { return Ok(()); } - send_state(&state, &on_event, preparing_state(None, 0.0, None))?; + send_state(&app, &state, preparing_state(None, 0.0, None))?; - let progress_channel = on_event.clone(); + let progress_app = app.clone(); let progress_state = state.inner().clone(); let result = cli_bridge::runtime_ensure(&app, move |event: RuntimeSetupEvent| { let index = phase_index(&event.stage); @@ -438,8 +442,8 @@ pub async fn begin_setup( // computer ready…") — falling back to `activity`. let activity = event.detail.or(event.activity); let _ = send_state( + &progress_app, &progress_state, - &progress_channel, preparing_state(index, fraction, activity), ); }) @@ -449,7 +453,7 @@ pub async fn begin_setup( match result { Ok(status) if status.ready => { - send_state(&state, &on_event, ready_state())?; + send_state(&app, &state, ready_state())?; } Ok(status) => { let error = CliError::Cli(Box::new(cli_bridge::CliErrorBody { @@ -463,18 +467,18 @@ pub async fn begin_setup( action_value: None, instances: None, })); - send_state(&state, &on_event, error_state(&error, PHASES.len()))?; + send_state(&app, &state, error_state(&error, PHASES.len()))?; } Err(error) => { let reached = phase_index("environment").unwrap_or(PHASES.len() - 1); - send_state(&state, &on_event, error_state(&error, reached))?; + send_state(&app, &state, error_state(&error, reached))?; } } Ok(()) } -/// Runs a recovery action, but only one the *last state pushed to this -/// window* actually offered (checked via `offered_actions`) — see +/// Runs a recovery action, but only one the *last state pushed* actually +/// offered (checked via `offered_actions`) — see /// [`BootstrapError::ActionDenied`]'s doc comment for why. Deliberately a /// small action set for now: `"retry"` re-runs `begin_setup` (the frontend /// just calls that command directly — no server-side action needed for it, @@ -486,11 +490,9 @@ pub async fn begin_setup( #[tauri::command] pub fn run_action( app: AppHandle, - window: WebviewWindow, state: tauri::State<'_, BootstrapState>, action: String, ) -> Result<(), BootstrapError> { - authorize_main(&window)?; if !state .offered_actions .read() diff --git a/src-tauri/tauri.conf.json b/src-tauri/tauri.conf.json index 528206a..2ca4db8 100644 --- a/src-tauri/tauri.conf.json +++ b/src-tauri/tauri.conf.json @@ -1,7 +1,7 @@ { "$schema": "https://schema.tauri.app/config/2", "productName": "Omnideck", - "version": "0.5.0-alpha.3", + "version": "0.5.0-alpha4", "identifier": "dev.omnideck.desktop", "build": { "beforeDevCommand": "npm run dev", diff --git a/src/hooks/useBootstrap.ts b/src/hooks/useBootstrap.ts index 4511af5..a7b54df 100644 --- a/src/hooks/useBootstrap.ts +++ b/src/hooks/useBootstrap.ts @@ -1,7 +1,10 @@ import { useCallback, useEffect, useRef, useState } from "react"; -import { Channel, invoke } from "@tauri-apps/api/core"; +import { invoke } from "@tauri-apps/api/core"; +import { listen, type UnlistenFn } from "@tauri-apps/api/event"; import type { SetupState } from "../types/setup"; +const SETUP_STATE_EVENT = "setup-state"; + export interface BootstrapController { state: SetupState | null; /** True once the initial `bootstrap` check has resolved and found the @@ -28,48 +31,68 @@ function errorMessage(error: unknown): string { * commands (`bootstrap`/`begin_setup`/`run_action`) an earlier, isolated * onboarding window used, now called from the dashboard's own "main" * window (see bootstrap.rs's doc comment for why there's no window to - * show/hide anymore). `enabled` gates the initial `bootstrap` call so it - * only fires once useCliVersion has confirmed the CLI itself is reachable - * — a broken CLI already produces its own, more specific BlockingScreen; - * this hook is only meaningful once talking to the CLI already works. */ + * show/hide anymore). + * + * Listens for `"setup-state"` events the same way `NewDeckForm.tsx` listens + * for `"add-progress"` — not a `Channel`, which this hook used to use. + * That was a real, unique-to-this-hook mechanism that turned out to be a + * plausible cause of a startup crash on some hardware (see bootstrap.rs's + * doc comment); this now matches the proven pattern everywhere else in the + * app instead. + * + * `enabled` gates the initial `bootstrap` call so it only fires once + * useCliVersion has confirmed the CLI itself is reachable — a broken CLI + * already produces its own, more specific BlockingScreen; this hook is only + * meaningful once talking to the CLI already works. */ export function useBootstrap(enabled: boolean): BootstrapController { const [state, setState] = useState(null); const [initiallyReady, setInitiallyReady] = useState(false); const [actionError, setActionError] = useState(null); const [actionPending, setActionPending] = useState(false); const startedRef = useRef(false); + // True until the first "setup-state" event arrives, so that event (and + // only that one) can be used to decide `initiallyReady` — later events + // (from begin_setup mid-flow) must not retroactively flip it. + const awaitingInitialRef = useRef(true); + // `listen()` is async — resolves once the event subscription is actually + // registered on the Tauri side. Calling `invoke("bootstrap")` before that + // resolves risks losing the very first "setup-state" event to a race, so + // the mount effect below awaits this before invoking. + const listenerReadyRef = useRef | null>(null); - const invokeWithChannel = useCallback( - async (command: string, onFirst?: (first: SetupState) => void) => { - const channel = new Channel(); - let seenFirst = false; - channel.onmessage = (next) => { - setState(next); - if (!seenFirst) { - seenFirst = true; - onFirst?.(next); - } - }; - await invoke(command, { onEvent: channel }); - }, - [], - ); + useEffect(() => { + let cancelled = false; + const promise = listen(SETUP_STATE_EVENT, (event) => { + setState(event.payload); + if (awaitingInitialRef.current) { + awaitingInitialRef.current = false; + if (event.payload.stage === "ready") setInitiallyReady(true); + } + }); + listenerReadyRef.current = promise; + return () => { + cancelled = true; + void promise.then((fn) => { + if (!cancelled) fn(); + }); + }; + }, []); useEffect(() => { if (!enabled || startedRef.current) return; startedRef.current = true; - void invokeWithChannel("bootstrap", (first) => { - if (first.stage === "ready") setInitiallyReady(true); - }).catch((error) => setActionError(errorMessage(error))); - }, [enabled, invokeWithChannel]); + void (listenerReadyRef.current ?? Promise.resolve()) + .then(() => invoke("bootstrap")) + .catch((error) => setActionError(errorMessage(error))); + }, [enabled]); const beginSetup = useCallback(() => { setActionPending(true); setActionError(null); - invokeWithChannel("begin_setup") + invoke("begin_setup") .catch((error) => setActionError(errorMessage(error))) .finally(() => setActionPending(false)); - }, [invokeWithChannel]); + }, []); const runAction = useCallback((action: string) => { setActionPending(true); diff --git a/tests/policy.test.mjs b/tests/policy.test.mjs index 08acb9b..89e0814 100644 --- a/tests/policy.test.mjs +++ b/tests/policy.test.mjs @@ -9,13 +9,18 @@ // Onboarding used to run from a second, isolated "onboarding" window with // its own capability grant — reverted to a single "main" window (React // screen-swap instead of a window swap) after a real EGL/GPU-driver bug was -// root-caused to the two-window-at-startup pattern itself (see -// bootstrap.rs's doc comment and reference/desktop-hardening-migration-PLAN.md's -// "Decisions from review" for the full history). The onboarding-specific -// isolation tests that used to live here no longer apply — there's no -// second window or capability left to isolate — but the dashboard-bridge -// allowlist assertions below now also cover the 3 bootstrap commands folded -// into it. +// suspected to be the two-window-at-startup pattern itself. That reversal +// alone didn't actually fix it (the bug reappeared in the next real-hardware +// test) — the working theory now is `bootstrap.rs`'s use of `tauri::ipc:: +// Channel` and a `WebviewWindow` command parameter, both unique to this +// module and both since replaced with the same `app.emit()`/`listen()` + +// `AppHandle`-only pattern every other command in this app already uses +// successfully. See bootstrap.rs's doc comment and +// reference/desktop-hardening-migration-PLAN.md's "Decisions from review" +// for the full history. The onboarding-specific isolation tests that used +// to live here no longer apply — there's no second window or capability +// left to isolate — but the dashboard-bridge allowlist assertions below now +// also cover the 3 bootstrap commands folded into it. // // These assertions exist so a future PR can't silently widen the attack // surface (a broader capability grant, a new command added to an allowlist @@ -108,35 +113,51 @@ test("dashboard permission's command list matches lib.rs's invoke_handler exactl }); test("the frontend only calls bootstrap's three commands, never a shell/process API", () => { - const invoked = [...useBootstrapTs.matchAll(/invoke(?:WithChannel)?\(\s*"([^"]+)"/g)].map((m) => m[1]); + const invoked = [...useBootstrapTs.matchAll(/invoke\(\s*"([^"]+)"/g)].map((m) => m[1]); assert.deepEqual([...new Set(invoked)].sort(), ["begin_setup", "bootstrap", "run_action"]); assert.doesNotMatch(useBootstrapTs, /plugin-shell|Command\.sidecar|executable|argv|workingDirectory/); }); -test("every bootstrap command authorizes window.label() == \"main\"", () => { - assert.match(bootstrapRust, /fn authorize_main\(window: &WebviewWindow\)/); - assert.match(bootstrapRust, /window\.label\(\) != "main"/); +test("bootstrap.rs's commands are AppHandle-only, matching every other command in this app", () => { for (const command of ["bootstrap", "begin_setup", "run_action"]) { const fn = bootstrapRust.match(new RegExp(`pub (?:async )?fn ${command}\\([\\s\\S]*?\\n\\}`)); assert.ok(fn, `expected to find ${command}()`); - assert.match(fn[0], /authorize_main\(&window\)\?/, `${command} must call authorize_main`); + assert.doesNotMatch(fn[0], /WebviewWindow/, `${command} should not take a WebviewWindow parameter`); } }); -test("bootstrap.rs no longer manages window visibility", () => { - // The regression this guards against is the one this repo actually hit: a - // second, initially-hidden window created at startup broke EGL/GPU-driver - // init on at least one real Intel/Mesa combination. Asserting the - // window-management surface is gone (not just unused) keeps it from - // creeping back in as part of some future onboarding tweak. +test("bootstrap.rs pushes state via app.emit, not a Channel", () => { + assert.match(bootstrapRust, /const SETUP_STATE_EVENT: &str = "setup-state"/); + assert.match(bootstrapRust, /app\.emit\(SETUP_STATE_EVENT/); +}); + +test("bootstrap.rs no longer manages window visibility or uses Channel/WebviewWindow", () => { + // Two regressions this guards against, in order: (1) a second, + // initially-hidden window created at startup, suspected of breaking + // EGL/GPU-driver init on at least one real Intel/Mesa combination — fixed + // by removing the second window; (2) that fix alone didn't hold on a + // second real-hardware test, and `tauri::ipc::Channel` / + // `WebviewWindow` turned out to be the only mechanisms unique to this + // module versus the rest of the app's proven-working IPC patterns. + // Asserting both are gone (not just unused) keeps either from creeping + // back in as part of some future onboarding tweak. Doc comments (`//!`/ + // `///`) are excluded — they're allowed, and expected, to keep discussing + // this history in prose; only actual code is checked. + const code = bootstrapRust + .split("\n") + .filter((line) => !/^\s*(\/\/!|\/\/\/)/.test(line)) + .join("\n"); for (const symbol of [ "show_onboarding", "show_dashboard", "create_onboarding_window", "WebviewWindowBuilder", "WebviewUrl", + "WebviewWindow", + "ipc::Channel", + "Channel<", ]) { - assert.doesNotMatch(bootstrapRust, new RegExp(symbol), `${symbol} should not reappear in bootstrap.rs`); + assert.doesNotMatch(code, new RegExp(symbol), `${symbol} should not reappear in bootstrap.rs's code`); } });