diff --git a/BUILDING.md b/BUILDING.md index 2d3580a..d43c786 100644 --- a/BUILDING.md +++ b/BUILDING.md @@ -71,13 +71,29 @@ npm ci npx tauri build ``` +Linux ships as **AppImage only**. To match what CI produces, pass the bundle +explicitly: + +```bash +npx tauri build --bundles appimage +``` + +The `.deb` and `.rpm` bundles were dropped — two more artifacts to build and +publish for an audience the AppImage already serves, and neither could +self-update. A bare `npx tauri build` still emits them, because +`tauri.conf.json` keeps `"targets": "all"` so that macOS and Windows are +untouched; they are not released and not tested. + Build artifacts are located in `app/src-tauri/target/release/bundle/`: -| Format | Path | -|------------|-------------------------------| -| AppImage | `appimage/*.AppImage` | -| Debian pkg | `deb/*.deb` | -| RPM pkg | `rpm/*.rpm` | +| Format | Path | Released | +|------------|-------------------------------|----------| +| AppImage | `appimage/*.AppImage` | yes | +| Debian pkg | `deb/*.deb` | no | +| RPM pkg | `rpm/*.rpm` | no | + +`scripts/finalize-appimage.sh` post-processes the AppImage; see the Packaging +section of `CLAUDE.md` for why both of its steps are load-bearing. ## macOS diff --git a/app/src-tauri/Cargo.lock b/app/src-tauri/Cargo.lock index d663f61..de2e13a 100644 --- a/app/src-tauri/Cargo.lock +++ b/app/src-tauri/Cargo.lock @@ -5306,6 +5306,7 @@ dependencies = [ "tauri-plugin-opener", "tokio", "tower-http", + "url", "uuid", "zeroize", ] diff --git a/app/src-tauri/Cargo.toml b/app/src-tauri/Cargo.toml index ebc9648..30558d5 100644 --- a/app/src-tauri/Cargo.toml +++ b/app/src-tauri/Cargo.toml @@ -39,6 +39,10 @@ local-ip-address = "0.6" argon2 = "0.5" aes-gcm = "0.10" zeroize = "1" +# WHATWG URL parsing for `url_open`'s re-validation of URLs arriving from the +# container. Already in the tree transitively (reqwest), and the point of +# using it rather than hand-rolling is parity with the frontend's `new URL()`. +url = "2" [dev-dependencies] # `test-util` (not part of tokio's `full`) lets the auto-start retry tests run diff --git a/app/src-tauri/capabilities/default.json b/app/src-tauri/capabilities/default.json index fbb9ace..63e7777 100644 --- a/app/src-tauri/capabilities/default.json +++ b/app/src-tauri/capabilities/default.json @@ -1,16 +1,12 @@ { "identifier": "default", - "description": "Default capabilities for Triple-C. Every entry here is an IPC command a compromised webview can call directly, so the set is an enumeration of what `app/src` actually invokes — verified against tauri 2.11.0's `PLUGINS` table in `build.rs`, not assumed from a plugin's `default` set. `core:default` in particular is NOT used: it is an alias for `core:{path,event,window,webview,app,image,resources,menu,tray}:default`, and `core:image:default` carries `allow-from-path`, whose handler (`tauri-2.11.0/src/image/plugin.rs:41` → `src/image/mod.rs:96`) is a bare `std::fs::read(path)` with no scope mechanism of any kind. Nothing imports `@tauri-apps/api/image`, so the whole plugin is dropped rather than scoped — there is nothing to scope it with. `core:menu` and `core:tray` are dropped for the same reason (no menu, no tray icon); `core:window` and `core:path` because nothing imports them; `core:resources:allow-close` because no frontend value is a `Resource`; and `core:event`'s `allow-emit`/`allow-emit-to` because the frontend only ever *listens* — every emit in this app originates in Rust. Three notes on what is deliberately kept or accepted: (1) `core:webview:allow-internal-toggle-devtools` is not called by `app/src` at all — it is called by Tauri's own injected `toggle-devtools.js`, which binds Ctrl/Cmd+Shift+I. Both that script and the command behind it are `#[cfg(any(debug_assertions, feature = \"devtools\"))]`, so this grant is a `tauri dev` convenience that does not exist in a release bundle. (2) `opener:allow-open-url` cannot be narrowed by host. `TerminalView`'s `WebLinksAddon` opens links Claude printed inside the container, which are arbitrary by construction, so a host allowlist here would delete the feature rather than bound it. What *is* bounded: `opener:default` is not used, so `open_path` and `reveal_item_in_dir` are absent; the scope's two entries restrict the scheme to http/https (`file:`, `mailto:`, `tel:`, `smb:` are all refused by `Scope::is_url_allowed`); and because each entry leaves `app` at its serde default of `Application::Default`, which matches only `with == None`, `openUrl(url, \"/bin/sh\")` is refused — the `with` argument is not a usable exec primitive. The call sites re-validate through `sanitizeRelayUrl` (scheme allowlist, no embedded credentials, length cap) before anything reaches the opener. Accepted residual risk: a compromised webview can make the OS open an attacker-chosen http(s) URL, which is an outbound channel. Recorded here rather than fixed. (3) `drag:allow-start-drag` is **gone**, together with the OS drag-out it existed for. It could not be scoped — `tauri-plugin-drag` takes the item paths from the caller and has no scope mechanism, so a compromised webview could call `startDrag({ item: ['~/.ssh/id_rsa'] })` against any host path the user can read — and it was carried as an accepted residual risk for one gesture. Drag-out was held back for separate hardening (see branch `hold/disk-and-dragout`) and the plugin is no longer a dependency. Getting a file *out* of a container is either \"Back up container\" on the project's Overview tab, which archives a tree through the Docker API, or the Files tab's per-row \"Save to host…\", which copies one file; getting one *in* is a drop on the Terminal or the Files tab's \"Upload…\". None of the four touches this permission. The Files tab's two are worth separating out here, because they are the only host-path commands in the app whose dialog is opened by **Rust** rather than by the webview — `pick_save_path` and `pick_files_to_upload` in `commands/file_commands.rs` drive `tauri-plugin-dialog` from the backend, so a compromised webview can ask for a picker and nothing more: it cannot name a host path as an *input* to either command. Be precise about the limit of that claim — host paths do still travel outward in error text (`Failed to create /home/j/Documents/x.txt.triple-c-part-1a2b3c4d: Permission denied`), including canonicalized ones, which disclose symlink targets. That is accepted; the app already hands the webview the project paths. What is closed is the direction that mattered — the webview naming where bytes go. That is the shape an earlier revision of this file named as the honest one if the Files tab ever regained host I/O, and it is the shape it regained it in. The `dialog:allow-open` / `dialog:allow-save` grants below are therefore *not* what those two use; they remain for the frontend pickers in Add Project, the Config tab's workspace and access sections, the CA-certificate field and Backup. Two commands still take a host path over IPC as a string — the terminal drop and `download_container_backup` — and for those `validate_host_path` is the boundary rather than defence in depth. This file is the reviewed threat model of record, so keep this census accurate: a stale reference here is worse than none. Note that dragging files *into* the app is unaffected: `dragDropEnabled` and `onDragDropEvent` are core webview behaviour and need no grant. Historical note kept because it is easy to re-introduce: the `store:*` grants were removed — nothing in `app/src` uses `@tauri-apps/plugin-store`, and the plugin's `resolve_store_path` is a `PathBuf::push` against AppData, which `push` discards outright when handed an absolute path, so the grant was an arbitrary host-file read/write primitive (`plugin:store|load` + `set` + `save` on `~/.claude/settings.json` is host code execution). On the CSP side: `app.security.csp` in `tauri.conf.json` covers the shipped bundle, and there is deliberately no `devCsp`. `npm run tauri dev` loads the main document straight from Vite at `build.devUrl` (`http://localhost:1420`), and Tauri only attaches a CSP to documents it serves itself — `protocol/tauri.rs:217` sets the header on `tauri://` assets, and the dev server is proxied through that protocol only when `PROXY_DEV_SERVER`, which is `cfg!(all(dev, mobile))` and therefore false for every desktop build. A `devCsp` here would be inert config that reads as protection, which is worse than its absence. If a CSP in dev is wanted, the only place that can set one is the Vite dev server's own `server.headers` in `app/vite.config.ts`; it is not set today, and dev is not the shipped configuration.", + "description": "Default capabilities for Triple-C. Every entry here is an IPC command a compromised webview can call directly, so the set is an enumeration of what `app/src` actually invokes — verified against tauri 2.11.0's `PLUGINS` table in `build.rs`, not assumed from a plugin's `default` set. `core:default` in particular is NOT used: it is an alias for `core:{path,event,window,webview,app,image,resources,menu,tray}:default`, and `core:image:default` carries `allow-from-path`, whose handler (`tauri-2.11.0/src/image/plugin.rs:41` → `src/image/mod.rs:96`) is a bare `std::fs::read(path)` with no scope mechanism of any kind. Nothing imports `@tauri-apps/api/image`, so the whole plugin is dropped rather than scoped — there is nothing to scope it with. `core:menu` and `core:tray` are dropped for the same reason (no menu, no tray icon); `core:window` and `core:path` because nothing imports them; `core:resources:allow-close` because no frontend value is a `Resource`; and `core:event`'s `allow-emit`/`allow-emit-to` because the frontend only ever *listens* — every emit in this app originates in Rust. Three notes on what is deliberately kept or accepted: (1) `core:webview:allow-internal-toggle-devtools` is not called by `app/src` at all — it is called by Tauri's own injected `toggle-devtools.js`, which binds Ctrl/Cmd+Shift+I. Both that script and the command behind it are `#[cfg(any(debug_assertions, feature = \"devtools\"))]`, so this grant is a `tauri dev` convenience that does not exist in a release bundle. (2) `opener:allow-open-url` is **gone**. It could not be narrowed by host \u2014 `TerminalView`'s `WebLinksAddon` opens links Claude printed inside the container, which are arbitrary by construction, so a host allowlist would have deleted the feature rather than bounded it \u2014 and it was carried here as an accepted residual risk: a compromised webview could make the OS open an attacker-chosen http(s) URL, an outbound channel. That risk is now closed rather than recorded. Every host-browser open in the app goes through the `open_url_external` command in `url_open.rs`, which exists because the AppImage environment leaks into a cold-launched browser on Linux (triple-c#34) and which re-validates the URL in Rust \u2014 scheme allowlist, no embedded credentials, no control characters, length cap, ASCII asserted before `execvp`. On macOS and Windows that command reaches the same plugin as before, via `OpenerExt::open_url`, whose desktop implementation calls `crate::open::open` directly and is therefore not gated by this file at all (`tauri-plugin-opener-2.5.3/src/lib.rs:60`). The plugin stays a dependency for exactly that reason; what is removed is the webview's ability to reach it without passing the Rust validation. (3) `drag:allow-start-drag` is **gone**, together with the OS drag-out it existed for. It could not be scoped — `tauri-plugin-drag` takes the item paths from the caller and has no scope mechanism, so a compromised webview could call `startDrag({ item: ['~/.ssh/id_rsa'] })` against any host path the user can read — and it was carried as an accepted residual risk for one gesture. Drag-out was held back for separate hardening (see branch `hold/disk-and-dragout`) and the plugin is no longer a dependency. Getting a file *out* of a container is either \"Back up container\" on the project's Overview tab, which archives a tree through the Docker API, or the Files tab's per-row \"Save to host…\", which copies one file; getting one *in* is a drop on the Terminal or the Files tab's \"Upload…\". None of the four touches this permission. The Files tab's two are worth separating out here, because they are the only host-path commands in the app whose dialog is opened by **Rust** rather than by the webview — `pick_save_path` and `pick_files_to_upload` in `commands/file_commands.rs` drive `tauri-plugin-dialog` from the backend, so a compromised webview can ask for a picker and nothing more: it cannot name a host path as an *input* to either command. Be precise about the limit of that claim — host paths do still travel outward in error text (`Failed to create /home/j/Documents/x.txt.triple-c-part-1a2b3c4d: Permission denied`), including canonicalized ones, which disclose symlink targets. That is accepted; the app already hands the webview the project paths. What is closed is the direction that mattered — the webview naming where bytes go. That is the shape an earlier revision of this file named as the honest one if the Files tab ever regained host I/O, and it is the shape it regained it in. The `dialog:allow-open` / `dialog:allow-save` grants below are therefore *not* what those two use; they remain for the frontend pickers in Add Project, the Config tab's workspace and access sections, the CA-certificate field and Backup. Two commands still take a host path over IPC as a string — the terminal drop and `download_container_backup` — and for those `validate_host_path` is the boundary rather than defence in depth. This file is the reviewed threat model of record, so keep this census accurate: a stale reference here is worse than none. Note that dragging files *into* the app is unaffected: `dragDropEnabled` and `onDragDropEvent` are core webview behaviour and need no grant. Historical note kept because it is easy to re-introduce: the `store:*` grants were removed — nothing in `app/src` uses `@tauri-apps/plugin-store`, and the plugin's `resolve_store_path` is a `PathBuf::push` against AppData, which `push` discards outright when handed an absolute path, so the grant was an arbitrary host-file read/write primitive (`plugin:store|load` + `set` + `save` on `~/.claude/settings.json` is host code execution). On the CSP side: `app.security.csp` in `tauri.conf.json` covers the shipped bundle, and there is deliberately no `devCsp`. `npm run tauri dev` loads the main document straight from Vite at `build.devUrl` (`http://localhost:1420`), and Tauri only attaches a CSP to documents it serves itself — `protocol/tauri.rs:217` sets the header on `tauri://` assets, and the dev server is proxied through that protocol only when `PROXY_DEV_SERVER`, which is `cfg!(all(dev, mobile))` and therefore false for every desktop build. A `devCsp` here would be inert config that reads as protection, which is worse than its absence. If a CSP in dev is wanted, the only place that can set one is the Vite dev server's own `server.headers` in `app/vite.config.ts`; it is not set today, and dev is not the shipped configuration.", "windows": ["main"], "permissions": [ "core:event:allow-listen", "core:event:allow-unlisten", "core:webview:allow-internal-toggle-devtools", "dialog:allow-open", - "dialog:allow-save", - { - "identifier": "opener:allow-open-url", - "allow": [{ "url": "http://*" }, { "url": "https://*" }] - } + "dialog:allow-save" ] } diff --git a/app/src-tauri/gen/schemas/capabilities.json b/app/src-tauri/gen/schemas/capabilities.json index 44533eb..4691a66 100644 --- a/app/src-tauri/gen/schemas/capabilities.json +++ b/app/src-tauri/gen/schemas/capabilities.json @@ -1 +1 @@ -{"default":{"identifier":"default","description":"Default capabilities for Triple-C. Every entry here is an IPC command a compromised webview can call directly, so the set is an enumeration of what `app/src` actually invokes — verified against tauri 2.11.0's `PLUGINS` table in `build.rs`, not assumed from a plugin's `default` set. `core:default` in particular is NOT used: it is an alias for `core:{path,event,window,webview,app,image,resources,menu,tray}:default`, and `core:image:default` carries `allow-from-path`, whose handler (`tauri-2.11.0/src/image/plugin.rs:41` → `src/image/mod.rs:96`) is a bare `std::fs::read(path)` with no scope mechanism of any kind. Nothing imports `@tauri-apps/api/image`, so the whole plugin is dropped rather than scoped — there is nothing to scope it with. `core:menu` and `core:tray` are dropped for the same reason (no menu, no tray icon); `core:window` and `core:path` because nothing imports them; `core:resources:allow-close` because no frontend value is a `Resource`; and `core:event`'s `allow-emit`/`allow-emit-to` because the frontend only ever *listens* — every emit in this app originates in Rust. Three notes on what is deliberately kept or accepted: (1) `core:webview:allow-internal-toggle-devtools` is not called by `app/src` at all — it is called by Tauri's own injected `toggle-devtools.js`, which binds Ctrl/Cmd+Shift+I. Both that script and the command behind it are `#[cfg(any(debug_assertions, feature = \"devtools\"))]`, so this grant is a `tauri dev` convenience that does not exist in a release bundle. (2) `opener:allow-open-url` cannot be narrowed by host. `TerminalView`'s `WebLinksAddon` opens links Claude printed inside the container, which are arbitrary by construction, so a host allowlist here would delete the feature rather than bound it. What *is* bounded: `opener:default` is not used, so `open_path` and `reveal_item_in_dir` are absent; the scope's two entries restrict the scheme to http/https (`file:`, `mailto:`, `tel:`, `smb:` are all refused by `Scope::is_url_allowed`); and because each entry leaves `app` at its serde default of `Application::Default`, which matches only `with == None`, `openUrl(url, \"/bin/sh\")` is refused — the `with` argument is not a usable exec primitive. The call sites re-validate through `sanitizeRelayUrl` (scheme allowlist, no embedded credentials, length cap) before anything reaches the opener. Accepted residual risk: a compromised webview can make the OS open an attacker-chosen http(s) URL, which is an outbound channel. Recorded here rather than fixed. (3) `drag:allow-start-drag` is **gone**, together with the OS drag-out it existed for. It could not be scoped — `tauri-plugin-drag` takes the item paths from the caller and has no scope mechanism, so a compromised webview could call `startDrag({ item: ['~/.ssh/id_rsa'] })` against any host path the user can read — and it was carried as an accepted residual risk for one gesture. Drag-out was held back for separate hardening (see branch `hold/disk-and-dragout`) and the plugin is no longer a dependency. Getting a file *out* of a container is either \"Back up container\" on the project's Overview tab, which archives a tree through the Docker API, or the Files tab's per-row \"Save to host…\", which copies one file; getting one *in* is a drop on the Terminal or the Files tab's \"Upload…\". None of the four touches this permission. The Files tab's two are worth separating out here, because they are the only host-path commands in the app whose dialog is opened by **Rust** rather than by the webview — `pick_save_path` and `pick_files_to_upload` in `commands/file_commands.rs` drive `tauri-plugin-dialog` from the backend, so a compromised webview can ask for a picker and nothing more: it cannot name a host path as an *input* to either command. Be precise about the limit of that claim — host paths do still travel outward in error text (`Failed to create /home/j/Documents/x.txt.triple-c-part-1a2b3c4d: Permission denied`), including canonicalized ones, which disclose symlink targets. That is accepted; the app already hands the webview the project paths. What is closed is the direction that mattered — the webview naming where bytes go. That is the shape an earlier revision of this file named as the honest one if the Files tab ever regained host I/O, and it is the shape it regained it in. The `dialog:allow-open` / `dialog:allow-save` grants below are therefore *not* what those two use; they remain for the frontend pickers in Add Project, the Config tab's workspace and access sections, the CA-certificate field and Backup. Two commands still take a host path over IPC as a string — the terminal drop and `download_container_backup` — and for those `validate_host_path` is the boundary rather than defence in depth. This file is the reviewed threat model of record, so keep this census accurate: a stale reference here is worse than none. Note that dragging files *into* the app is unaffected: `dragDropEnabled` and `onDragDropEvent` are core webview behaviour and need no grant. Historical note kept because it is easy to re-introduce: the `store:*` grants were removed — nothing in `app/src` uses `@tauri-apps/plugin-store`, and the plugin's `resolve_store_path` is a `PathBuf::push` against AppData, which `push` discards outright when handed an absolute path, so the grant was an arbitrary host-file read/write primitive (`plugin:store|load` + `set` + `save` on `~/.claude/settings.json` is host code execution). On the CSP side: `app.security.csp` in `tauri.conf.json` covers the shipped bundle, and there is deliberately no `devCsp`. `npm run tauri dev` loads the main document straight from Vite at `build.devUrl` (`http://localhost:1420`), and Tauri only attaches a CSP to documents it serves itself — `protocol/tauri.rs:217` sets the header on `tauri://` assets, and the dev server is proxied through that protocol only when `PROXY_DEV_SERVER`, which is `cfg!(all(dev, mobile))` and therefore false for every desktop build. A `devCsp` here would be inert config that reads as protection, which is worse than its absence. If a CSP in dev is wanted, the only place that can set one is the Vite dev server's own `server.headers` in `app/vite.config.ts`; it is not set today, and dev is not the shipped configuration.","local":true,"windows":["main"],"permissions":["core:event:allow-listen","core:event:allow-unlisten","core:webview:allow-internal-toggle-devtools","dialog:allow-open","dialog:allow-save",{"identifier":"opener:allow-open-url","allow":[{"url":"http://*"},{"url":"https://*"}]}]}} \ No newline at end of file +{"default":{"identifier":"default","description":"Default capabilities for Triple-C. Every entry here is an IPC command a compromised webview can call directly, so the set is an enumeration of what `app/src` actually invokes — verified against tauri 2.11.0's `PLUGINS` table in `build.rs`, not assumed from a plugin's `default` set. `core:default` in particular is NOT used: it is an alias for `core:{path,event,window,webview,app,image,resources,menu,tray}:default`, and `core:image:default` carries `allow-from-path`, whose handler (`tauri-2.11.0/src/image/plugin.rs:41` → `src/image/mod.rs:96`) is a bare `std::fs::read(path)` with no scope mechanism of any kind. Nothing imports `@tauri-apps/api/image`, so the whole plugin is dropped rather than scoped — there is nothing to scope it with. `core:menu` and `core:tray` are dropped for the same reason (no menu, no tray icon); `core:window` and `core:path` because nothing imports them; `core:resources:allow-close` because no frontend value is a `Resource`; and `core:event`'s `allow-emit`/`allow-emit-to` because the frontend only ever *listens* — every emit in this app originates in Rust. Three notes on what is deliberately kept or accepted: (1) `core:webview:allow-internal-toggle-devtools` is not called by `app/src` at all — it is called by Tauri's own injected `toggle-devtools.js`, which binds Ctrl/Cmd+Shift+I. Both that script and the command behind it are `#[cfg(any(debug_assertions, feature = \"devtools\"))]`, so this grant is a `tauri dev` convenience that does not exist in a release bundle. (2) `opener:allow-open-url` is **gone**. It could not be narrowed by host — `TerminalView`'s `WebLinksAddon` opens links Claude printed inside the container, which are arbitrary by construction, so a host allowlist would have deleted the feature rather than bounded it — and it was carried here as an accepted residual risk: a compromised webview could make the OS open an attacker-chosen http(s) URL, an outbound channel. That risk is now closed rather than recorded. Every host-browser open in the app goes through the `open_url_external` command in `url_open.rs`, which exists because the AppImage environment leaks into a cold-launched browser on Linux (triple-c#34) and which re-validates the URL in Rust — scheme allowlist, no embedded credentials, no control characters, length cap, ASCII asserted before `execvp`. On macOS and Windows that command reaches the same plugin as before, via `OpenerExt::open_url`, whose desktop implementation calls `crate::open::open` directly and is therefore not gated by this file at all (`tauri-plugin-opener-2.5.3/src/lib.rs:60`). The plugin stays a dependency for exactly that reason; what is removed is the webview's ability to reach it without passing the Rust validation. (3) `drag:allow-start-drag` is **gone**, together with the OS drag-out it existed for. It could not be scoped — `tauri-plugin-drag` takes the item paths from the caller and has no scope mechanism, so a compromised webview could call `startDrag({ item: ['~/.ssh/id_rsa'] })` against any host path the user can read — and it was carried as an accepted residual risk for one gesture. Drag-out was held back for separate hardening (see branch `hold/disk-and-dragout`) and the plugin is no longer a dependency. Getting a file *out* of a container is either \"Back up container\" on the project's Overview tab, which archives a tree through the Docker API, or the Files tab's per-row \"Save to host…\", which copies one file; getting one *in* is a drop on the Terminal or the Files tab's \"Upload…\". None of the four touches this permission. The Files tab's two are worth separating out here, because they are the only host-path commands in the app whose dialog is opened by **Rust** rather than by the webview — `pick_save_path` and `pick_files_to_upload` in `commands/file_commands.rs` drive `tauri-plugin-dialog` from the backend, so a compromised webview can ask for a picker and nothing more: it cannot name a host path as an *input* to either command. Be precise about the limit of that claim — host paths do still travel outward in error text (`Failed to create /home/j/Documents/x.txt.triple-c-part-1a2b3c4d: Permission denied`), including canonicalized ones, which disclose symlink targets. That is accepted; the app already hands the webview the project paths. What is closed is the direction that mattered — the webview naming where bytes go. That is the shape an earlier revision of this file named as the honest one if the Files tab ever regained host I/O, and it is the shape it regained it in. The `dialog:allow-open` / `dialog:allow-save` grants below are therefore *not* what those two use; they remain for the frontend pickers in Add Project, the Config tab's workspace and access sections, the CA-certificate field and Backup. Two commands still take a host path over IPC as a string — the terminal drop and `download_container_backup` — and for those `validate_host_path` is the boundary rather than defence in depth. This file is the reviewed threat model of record, so keep this census accurate: a stale reference here is worse than none. Note that dragging files *into* the app is unaffected: `dragDropEnabled` and `onDragDropEvent` are core webview behaviour and need no grant. Historical note kept because it is easy to re-introduce: the `store:*` grants were removed — nothing in `app/src` uses `@tauri-apps/plugin-store`, and the plugin's `resolve_store_path` is a `PathBuf::push` against AppData, which `push` discards outright when handed an absolute path, so the grant was an arbitrary host-file read/write primitive (`plugin:store|load` + `set` + `save` on `~/.claude/settings.json` is host code execution). On the CSP side: `app.security.csp` in `tauri.conf.json` covers the shipped bundle, and there is deliberately no `devCsp`. `npm run tauri dev` loads the main document straight from Vite at `build.devUrl` (`http://localhost:1420`), and Tauri only attaches a CSP to documents it serves itself — `protocol/tauri.rs:217` sets the header on `tauri://` assets, and the dev server is proxied through that protocol only when `PROXY_DEV_SERVER`, which is `cfg!(all(dev, mobile))` and therefore false for every desktop build. A `devCsp` here would be inert config that reads as protection, which is worse than its absence. If a CSP in dev is wanted, the only place that can set one is the Vite dev server's own `server.headers` in `app/vite.config.ts`; it is not set today, and dev is not the shipped configuration.","local":true,"windows":["main"],"permissions":["core:event:allow-listen","core:event:allow-unlisten","core:webview:allow-internal-toggle-devtools","dialog:allow-open","dialog:allow-save"]}} \ No newline at end of file diff --git a/app/src-tauri/src/browser_view/commands.rs b/app/src-tauri/src/browser_view/commands.rs index 0b3a58e..1b4a5af 100644 --- a/app/src-tauri/src/browser_view/commands.rs +++ b/app/src-tauri/src/browser_view/commands.rs @@ -15,6 +15,12 @@ use crate::AppState; /// non-`Running` status carrying an explanation rather than an error, so the /// pane always has something specific to say. This is host-side only — no /// container recreation is involved either way. +/// +/// Either way the choice is persisted, so it survives an app restart. This is +/// the only caller allowed to write `false`: every other path to +/// [`BrowserViewManager::stop`](crate::browser_view::BrowserViewManager::stop) +/// is a teardown rather than the user changing their mind. Enabling persists +/// inside `start`, which is the single funnel for it. #[tauri::command] pub async fn set_browser_view_enabled( project_id: String, @@ -23,9 +29,32 @@ pub async fn set_browser_view_enabled( state: State<'_, AppState>, ) -> Result { if !enabled { + // Persist first, then tear down: the supervisor's own teardown emit + // reads this flag back out of the store, and reading it mid-stop would + // announce a view that is going away as still enabled. + // + // But the write's outcome is a *value*, not a branch. A `?` here meant + // that a store with no such project record returned early and + // `manager().stop()` never ran, leaving the supervisor, the proxy and + // the host port up for a project that, as far as the user is concerned, + // just had its view switched off. That state is not hypothetical while + // a session is live — the supervisor's own `store.get()` check in + // [`crate::browser_view`] exists because a record can go away + // underneath it — and before the flag was persisted at all, turning the + // view off always tore the session down. + let persisted = state + .projects_store + .set_browser_view_enabled(&project_id, false); // Awaits the supervisor, so the host port is released before we return. - manager().stop(&project_id).await; - return Ok(manager().status(&project_id).await); + // + // A failed write is still reported rather than logged and swallowed. + // The resources are gone either way by this point, so surfacing it + // costs nothing that matters, and the failure it describes is one the + // user needs: the stored flag still says *enabled*, so the view comes + // back by itself on the next launch. Returning `Ok` would be a claim + // about persistence that isn't true. + tear_down_then_report(persisted, manager().stop(&project_id)).await?; + return Ok(manager().status(&project_id, false).await); } let container_id = running_container(&state, &project_id, "opening the browser view").await?; @@ -40,10 +69,31 @@ pub async fn set_browser_view_enabled( .await } -/// Current status. Cheap: reads in-process state only, never the container. +/// Await `teardown`, then report `persisted`. +/// +/// Trivial on purpose, and split out for one reason: it is the whole rule the +/// disable path of [`set_browser_view_enabled`] has to obey — the teardown is +/// unconditional, and a failed persist surfaces only after it has run — and as +/// a free function that rule can be tested without a live `AppState`. +async fn tear_down_then_report( + persisted: Result<(), String>, + teardown: impl std::future::Future, +) -> Result<(), String> { + teardown.await; + persisted +} + +/// Current status. Cheap: the session map in this process plus the stored flag, +/// never the container. +/// +/// The two are independent on purpose — this is what the pane reads on mount, +/// and after an app restart the honest answer is "enabled, nothing running". #[tauri::command] -pub async fn get_browser_view_status(project_id: String) -> Result { - Ok(manager().status(&project_id).await) +pub async fn get_browser_view_status( + project_id: String, + state: State<'_, AppState>, +) -> Result { + Ok(manager().status(&project_id, enabled_for(&state, &project_id)).await) } /// Probe the container for Playwright without starting anything. @@ -110,7 +160,9 @@ pub async fn open_browser_view_popout( app_handle: AppHandle, state: State<'_, AppState>, ) -> Result<(), String> { - let status = manager().status(&project_id).await; + let status = manager() + .status(&project_id, enabled_for(&state, &project_id)) + .await; let (BrowserViewState::Running, Some(url)) = (status.state, status.url.as_deref()) else { return Err( "The browser view isn't running. Start it before opening it in its own window." @@ -209,7 +261,9 @@ pub async fn open_page_in_container_browser( // the user to go and press Start in the Browser tab themselves — and from // the terminal's URL prompt, with no indication that was even needed. // Asking for a page *is* asking to watch it, so the viewer comes up too. - let status = manager().status(&project_id).await; + let status = manager() + .status(&project_id, enabled_for(&state, &project_id)) + .await; if status.state != BrowserViewState::Running { crate::commands::project_commands::emit_progress( &app_handle, @@ -229,7 +283,9 @@ pub async fn open_page_in_container_browser( // From the terminal there is no pane on screen to fill, so the page needs a // window of its own or it lands somewhere the user isn't looking. if show_window { - let status = manager().status(&project_id).await; + let status = manager() + .status(&project_id, enabled_for(&state, &project_id)) + .await; if let Some(url) = status.url.as_deref() { let name = state .projects_store @@ -311,6 +367,20 @@ pub async fn get_browser_view_match_window(project_id: String) -> Result, project_id: &str) -> bool { + state + .projects_store + .get(project_id) + .is_some_and(|p| p.browser_view_enabled) +} + /// The project's container, or a sentence saying why there isn't one. /// /// Every command here needs a *running* container, and every one of them used @@ -344,3 +414,43 @@ async fn running_container( } Ok(container_id) } + +#[cfg(test)] +mod tests { + use super::*; + use std::sync::atomic::{AtomicBool, Ordering}; + + /// The regression: turning the view off must not leave the supervisor, the + /// proxy and the host port running just because the project record could + /// not be written — which is exactly what a missing record did. + #[tokio::test] + async fn a_failed_persist_does_not_skip_the_teardown() { + let torn_down = AtomicBool::new(false); + let result = tear_down_then_report(Err("Project x not found".to_string()), async { + torn_down.store(true, Ordering::SeqCst); + }) + .await; + + assert!( + torn_down.load(Ordering::SeqCst), + "the session must be torn down even when the store write failed" + ); + assert_eq!( + result.err().as_deref(), + Some("Project x not found"), + "and the write failure must still reach the caller, not be swallowed" + ); + } + + #[tokio::test] + async fn a_successful_persist_reports_success_after_the_teardown() { + let torn_down = AtomicBool::new(false); + let result = tear_down_then_report(Ok(()), async { + torn_down.store(true, Ordering::SeqCst); + }) + .await; + + assert!(torn_down.load(Ordering::SeqCst)); + assert!(result.is_ok()); + } +} diff --git a/app/src-tauri/src/browser_view/mod.rs b/app/src-tauri/src/browser_view/mod.rs index b816749..dd35dcb 100644 --- a/app/src-tauri/src/browser_view/mod.rs +++ b/app/src-tauri/src/browser_view/mod.rs @@ -34,14 +34,22 @@ //! //! ## Lifecycle //! -//! Off by default and per-project opt-in, exactly like `auth_bridge_enabled`. +//! Off by default and per-project opt-in. The opt-in itself is +//! [`Project::browser_view_enabled`](crate::models::Project), persisted like +//! `auth_bridge_enabled` and read from the store on demand rather than cached +//! here — so the pane comes back the way it was left. What does *not* persist +//! is the session: nothing starts a viewer on app start, so a project left +//! enabled reports `enabled: true` with a state of `Off` until the pane asks +//! for one. That is deliberate, and the reason the flag and the session are +//! separate ideas — see [`BrowserViewManager::status`]. +//! //! One supervisor task per session owns the proxy and the viewer process, and it //! is the only thing that tears them down, so every way a session can end funnels //! through one code path: //! //! | Trigger | Path | //! |---|---| -//! | Turned off in the UI | `set_browser_view_enabled(false)` → [`BrowserViewManager::stop`] | +//! | Turned off in the UI | `set_browser_view_enabled(false)` → persist `false`, then [`BrowserViewManager::stop`] | //! | Container stopped, by the UI or otherwise | supervisor's `is_container_running` check | //! | Project deleted | supervisor's `store.get()` check | //! | Container rebuilt | old container stops → supervisor exits; the new one is not auto-started | @@ -59,7 +67,10 @@ //! orphan is reachable on container loopback only: the host-side port dies with //! the app, and [`crate::auth_bridge::RESERVED_CONTAINER_PORTS`] is a constant //! precisely so the bridge will not mirror an orphan the next time the app -//! starts. The next [`BrowserViewManager::start`] reclaims it. +//! starts. The next [`BrowserViewManager::start`] reclaims it — and since the +//! opt-in is now durable, the restarted app says `enabled` with nothing running, +//! which is exactly the state that invites the user to press the button that +//! reclaims it. Nothing reclaims it on its own, because nothing auto-starts. pub mod commands; pub mod detect; @@ -134,7 +145,10 @@ pub enum BrowserViewState { #[derive(Debug, Clone, Serialize)] pub struct BrowserViewStatus { - /// The per-project opt-in. Off by default. + /// The per-project opt-in, read from the persisted project record. Off by + /// default, and true without a `Running` state whenever the view is turned + /// on but has nothing up — a stopped container, or an app that has just + /// restarted and does not auto-start viewers. pub enabled: bool, pub state: BrowserViewState, /// Fully-formed, token-bearing URL for the pane's iframe. Loopback only. @@ -201,17 +215,20 @@ struct Session { type SessionMap = Arc>>; +/// Live sessions, and nothing else. +/// +/// The per-project opt-in deliberately is **not** a field here. It lives on +/// the project record as +/// [`browser_view_enabled`](crate::models::Project::browser_view_enabled) and +/// is read from [`ProjectsStore`] at each use, exactly as +/// [`crate::auth_bridge::AuthBridgeManager`] treats `auth_bridge_enabled`: +/// one copy, durable across a restart, and impossible to get out of step with +/// what the Config tab shows. A cached copy here was the previous design and +/// its only observable behaviour was forgetting the user's choice on every +/// app start. #[derive(Default)] pub struct BrowserViewManager { sessions: SessionMap, - /// The per-project opt-in. - /// - /// NOTE: in memory only, so it does not survive an app restart. The durable - /// home for this is a `browser_view_enabled: bool` field on - /// `models::Project` (see the report) — `models/project.rs` is out of scope - /// for this change, so the flag lives here and the wiring is otherwise - /// identical to `auth_bridge_enabled`. - enabled: Mutex>, next_epoch: AtomicU64, } @@ -226,22 +243,15 @@ pub fn manager() -> &'static Arc { } impl BrowserViewManager { - pub async fn is_enabled(&self, project_id: &str) -> bool { - self.enabled.lock().await.contains(project_id) - } - - async fn set_enabled(&self, project_id: &str, enabled: bool) { - let mut set = self.enabled.lock().await; - if enabled { - set.insert(project_id.to_string()); - } else { - set.remove(project_id); - } - } - /// Current status without touching the container. - pub async fn status(&self, project_id: &str) -> BrowserViewStatus { - let enabled = self.is_enabled(project_id).await; + /// + /// `enabled` is passed in rather than looked up, the way + /// [`crate::auth_bridge::AuthBridgeManager::status`] takes it: the flag is + /// the caller's to read from the store, and keeping it out of here is what + /// stops a second copy of it appearing. A project whose view is enabled but + /// whose container is stopped — or whose app has just restarted — reports + /// `enabled: true` with a state of `Off`, which is the honest answer. + pub async fn status(&self, project_id: &str, enabled: bool) -> BrowserViewStatus { match self.sessions.lock().await.get(project_id) { Some(session) => BrowserViewStatus { enabled, @@ -261,6 +271,14 @@ impl BrowserViewManager { /// /// Idempotent: a call while a live session exists returns that session's /// status untouched, so re-opening the tab does not restart the dashboard. + /// + /// This is the single funnel for turning the view **on**, so it is also + /// where the durable flag is written — both call sites (the toggle and + /// `open_page_in_container_browser`, which opens a page and then shows it) + /// mean "on", and neither can forget. The **off** direction is not + /// symmetric and must not be: [`Self::stop`] is reached by teardown paths + /// that are not the user changing their mind, so the command owns that + /// write. See [`Self::stop`]. pub async fn start( &self, project_id: String, @@ -268,7 +286,7 @@ impl BrowserViewManager { app: AppHandle, store: Arc, ) -> Result { - self.set_enabled(&project_id, true).await; + store.set_browser_view_enabled(&project_id, true)?; // Bind the answer before acting on it: `status()` takes the same lock, // and this mutex is not reentrant. @@ -279,7 +297,7 @@ impl BrowserViewManager { .get(&project_id) .is_some_and(|s| !s.supervisor.is_finished()); if already_live { - return Ok(self.status(&project_id).await); + return Ok(self.status(&project_id, true).await); } let detection = detect::detect(&container_id).await?; @@ -364,14 +382,21 @@ impl BrowserViewManager { }, ); - let status = self.status(&project_id).await; + let status = self.status(&project_id, true).await; emit(&app, &project_id, &status); Ok(status) } /// Stop one project's view and wait until its host port has been released. + /// + /// Tears the *session* down and deliberately leaves the durable flag alone. + /// Most callers are not the user turning the feature off — a migration + /// removes the container out from under a running view + /// (`migration_commands`), and the container can stop for any other reason + /// — and persisting `false` for those would quietly opt the project out of + /// a feature it never asked to lose. `set_browser_view_enabled(false)` is + /// the one caller that means it, and it writes the flag itself first. pub async fn stop(&self, project_id: &str) { - self.set_enabled(project_id, false).await; // Remove under the lock, then release it before awaiting: the // supervisor takes the same lock to deregister itself on exit. let session = self.sessions.lock().await.remove(project_id); @@ -483,7 +508,12 @@ async fn supervise( // longer exists. The session owns it, and this is where the session ends. let _ = popout::close(&app, &project_id); - let enabled = manager().is_enabled(&project_id).await; + // Straight from the store, like the auth bridge's own teardown emit: the + // session is over, but the project may well still be opted in — a stopped + // container is not a changed mind, and the pane has to show the difference. + let enabled = store + .get(&project_id) + .is_some_and(|p| p.browser_view_enabled); emit(&app, &project_id, &BrowserViewStatus::off(enabled)); } @@ -915,6 +945,25 @@ mod tests { assert!(s.url.is_none()); } + #[tokio::test] + async fn the_opt_in_and_the_live_session_are_separate_answers() { + let manager = BrowserViewManager::default(); + + // Exactly what the pane reads on mount after an app restart of a + // project that was left enabled: the durable flag says on, and nothing + // auto-starts, so the state is honestly `Off`. The old in-memory flag + // could not express this — it came back `false` and the pane silently + // showed the feature as never having been turned on. + let status = manager.status("p1", true).await; + assert!(status.enabled); + assert_eq!(status.state, BrowserViewState::Off); + assert!(status.url.is_none()); + + // The flag belongs to the caller, read from the store. The manager + // keeps no copy, so it has nothing to contradict it with. + assert!(!manager.status("p1", false).await.enabled); + } + #[test] fn an_unavailable_status_keeps_the_detail_the_user_needs() { let mut d = PlaywrightDetection::default(); diff --git a/app/src-tauri/src/commands/project_commands.rs b/app/src-tauri/src/commands/project_commands.rs index ac93bfe..e82d011 100644 --- a/app/src-tauri/src/commands/project_commands.rs +++ b/app/src-tauri/src/commands/project_commands.rs @@ -1036,7 +1036,6 @@ fn pending_cleanup_is_stale(recorded_at: &str, now: chrono::DateTime, ) -> Result { // Taken as raw JSON, then deserialised, for one reason: a secret field that @@ -1098,37 +1097,57 @@ pub async fn update_project( // [`crate::models::validate_env_vars_update`]. crate::models::validate_env_vars_update(&stored.custom_env_vars, &project.custom_env_vars)?; - project.container_id = stored.container_id; - project.status = stored.status; - project.created_at = stored.created_at; + restore_store_owned_fields(&mut project, &stored); project.updated_at = chrono::Utc::now().to_rfc3339(); store_secrets_for_project(&project, &explicitly_cleared)?; - let updated = state.projects_store.update(project)?; - // `auth_bridge_enabled` can arrive through this generic save as well as - // through `set_auth_bridge_enabled`, so reconcile the running bridge with - // whatever was just persisted. `start` is idempotent and `stop` is a no-op - // when nothing is running, so this is safe on every project save. - if updated.auth_bridge_enabled { - if let Some(ref container_id) = updated.container_id { - if docker::is_container_running(container_id).await.unwrap_or(false) { - state - .auth_bridge - .start( - updated.id.clone(), - container_id.clone(), - app_handle, - state.projects_store.clone(), - ) - .await; - } - } - } else { - state.auth_bridge.stop(&updated.id).await; - } + // Nothing reconciles the *running* auth bridge here any more, and there is + // nothing left for such a step to do. This command can no longer change + // `auth_bridge_enabled` at all (see [`restore_store_owned_fields`]), so a + // reconcile could only ever re-assert what was already true. The paths that + // do change it each own their own side effect: `set_auth_bridge_enabled` + // starts or stops the bridge itself, [`start_project_container`] arms it + // when the container comes up, and `reconcile_project_statuses` re-arms it + // for every already-running container at launch. The version of this that + // re-asserted on every save is what turned a stale flag in a payload into a + // restarted bridge. + state.projects_store.update(project) +} - Ok(updated) +/// Restore onto `project` the fields whose value belongs to the store rather +/// than to whoever is saving the project. See the comment above `stored` in +/// [`update_project`] for `container_id`, `status` and `created_at`. +/// +/// **Both feature flags are in here, for one reason that covers them equally: +/// neither ever arrives through this command as an edit.** Each has a +/// dedicated setter — [`crate::browser_view::commands::set_browser_view_enabled`] +/// and [`crate::commands::auth_bridge_commands::set_auth_bridge_enabled`] — +/// and that setter is the only control the UI offers for it. Neither is wired +/// into the Config tab's `save`: the browser view's toggle lives in the Browser +/// tab, and `AuthBridgeRow`'s switch calls `set_auth_bridge_enabled` directly +/// even though it is rendered *in* the Config tab, because that tab's editors +/// are disabled while the container runs and the bridge is precisely the thing +/// a user needs to flip while a login is hanging. +/// +/// So the flags in an incoming payload are never a choice — they are whatever +/// the frontend was told when it loaded the project, and the setters do not +/// write their new value back into frontend app state. Every unrelated save +/// (a renamed session, an env var, a mount name) carries that snapshot back. +/// Taking it would silently undo a toggle made since. +/// +/// This restored only `browser_view_enabled` before, on the stated belief that +/// the Config tab edited `auth_bridge_enabled` through this save. It does not. +/// The consequence was specific: a user turns the bridge off — having been told +/// a bridged port is unauthenticated and reachable by any local process — then +/// closes a renamed terminal tab, and the stale `true` in that save re-persisted +/// and restarted the bridge. +fn restore_store_owned_fields(project: &mut Project, stored: &Project) { + project.container_id = stored.container_id.clone(); + project.status = stored.status.clone(); + project.browser_view_enabled = stored.browser_view_enabled; + project.auth_bridge_enabled = stored.auth_bridge_enabled; + project.created_at = stored.created_at.clone(); } #[tauri::command] @@ -2186,4 +2205,89 @@ mod tests { // Changing it to a different root is a change, and refused. assert!(validate_mounted_host_path("x", Some("/"), Some("C:\\")).is_err()); } + // ── Fields a generic save does not get to write ─────────────────────── + + /// A project as the store holds it, plus the copy the frontend is about to + /// save back: same record, one unrelated edit, and the flags as they were + /// when the frontend last loaded it. + fn stored_and_stale_payload() -> (Project, Project) { + let mut stored = Project::new("demo".to_string(), Vec::new()); + stored.container_id = Some("abc123".to_string()); + stored.status = ProjectStatus::Running; + + let mut payload = stored.clone(); + payload.container_id = None; + payload.status = ProjectStatus::Stopped; + payload + .renamed_session_names + .insert("s1".to_string(), "build".to_string()); + + (stored, payload) + } + + /// The regression. The user turns the auth bridge off — the switch calls + /// `set_auth_bridge_enabled`, which persists `false` and stops the bridge, + /// and writes nothing back into the frontend's copy of the project. Every + /// holder of that copy still has `auth_bridge_enabled: true`, and the next + /// unrelated save (closing a renamed terminal tab) posts it back. That save + /// must not re-enable the bridge. + #[test] + fn a_stale_auth_bridge_flag_in_a_save_cannot_re_enable_a_disabled_bridge() { + let (mut stored, mut payload) = stored_and_stale_payload(); + stored.auth_bridge_enabled = false; + payload.auth_bridge_enabled = true; + + restore_store_owned_fields(&mut payload, &stored); + + assert!( + !payload.auth_bridge_enabled, + "a save must not be able to turn the bridge back on: the stored value is the user's" + ); + // The edit the save was actually for still goes through. + assert_eq!( + payload.renamed_session_names.get("s1").map(String::as_str), + Some("build") + ); + } + + /// The mirror image, and the reason the serde default going to `true` + /// made this worse: a pre-existing record with no `auth_bridge_enabled` + /// key reads as enabled, so the stale payload is `true` for every project + /// that predates the field. A user who has *not* turned the bridge off is + /// equally entitled to have the store's answer win. + #[test] + fn an_enabled_bridge_is_left_enabled_by_the_same_rule() { + let (mut stored, mut payload) = stored_and_stale_payload(); + stored.auth_bridge_enabled = true; + payload.auth_bridge_enabled = false; + + restore_store_owned_fields(&mut payload, &stored); + + assert!(payload.auth_bridge_enabled); + } + + /// The flag that was already restored, kept under test beside the one that + /// was not — the two are owned by their setters for the same reason and + /// must not drift apart again. + #[test] + fn a_stale_browser_view_flag_cannot_undo_the_panes_toggle_either() { + let (mut stored, mut payload) = stored_and_stale_payload(); + stored.browser_view_enabled = true; + payload.browser_view_enabled = false; + + restore_store_owned_fields(&mut payload, &stored); + + assert!(payload.browser_view_enabled); + } + + #[test] + fn the_container_handle_status_and_creation_time_still_come_from_the_store() { + let (stored, mut payload) = stored_and_stale_payload(); + + restore_store_owned_fields(&mut payload, &stored); + + assert_eq!(payload.container_id.as_deref(), Some("abc123")); + assert_eq!(payload.status, ProjectStatus::Running); + assert_eq!(payload.created_at, stored.created_at); + } } diff --git a/app/src-tauri/src/lib.rs b/app/src-tauri/src/lib.rs index dc62069..3e38a2e 100644 --- a/app/src-tauri/src/lib.rs +++ b/app/src-tauri/src/lib.rs @@ -7,6 +7,7 @@ mod logging; mod models; mod project_lock; mod storage; +pub mod url_open; pub mod web_terminal; use std::sync::atomic::{AtomicBool, Ordering}; @@ -552,6 +553,9 @@ pub fn run() { commands::update_commands::check_image_update, // Help commands::help_commands::get_help_content, + // Opening a link in the host browser (see `url_open` for why this + // is not `@tauri-apps/plugin-opener` on Linux) + url_open::open_url_external, // Install helper commands::install_helper_commands::detect_install_options, commands::install_helper_commands::run_docker_install, @@ -934,7 +938,6 @@ mod tests { "core:webview:allow-internal-toggle-devtools", "dialog:allow-open", "dialog:allow-save", - "opener:allow-open-url", ]; expected.sort(); assert_eq!( diff --git a/app/src-tauri/src/main.rs b/app/src-tauri/src/main.rs index baca4b5..b345fde 100644 --- a/app/src-tauri/src/main.rs +++ b/app/src-tauri/src/main.rs @@ -63,6 +63,12 @@ /// URL; most non-WebKitGTK browsers ignore the variable entirely), but /// worth knowing before chasing the "links don't open" half of triple-c#34 /// as a separate, unrelated cause. +/// +/// That leak is now plugged rather than merely documented: `url_open` hands +/// the opener a child environment with this variable (and the AppImage's own +/// `LD_LIBRARY_PATH`/`GTK_PATH`/... ) restored or removed. Setting it here +/// stays process-wide because GTK/WebKitGTK need it; what changed is that the +/// children no longer inherit it. #[cfg(target_os = "linux")] const DMABUF_VAR: &str = "WEBKIT_DISABLE_DMABUF_RENDERER"; @@ -138,6 +144,12 @@ mod tests { } fn main() { + // Before *any* `std::env::set_var` — `url_open` hands a child process the + // environment this app was started with, and the workaround below is one + // of the things that must not leak into it (see triple-c#34). Anything + // added here that mutates the environment belongs after this line. + triple_c_lib::url_open::capture_pristine_environment(); + #[cfg(target_os = "linux")] apply_webkit_wayland_workaround(); diff --git a/app/src-tauri/src/models/project.rs b/app/src-tauri/src/models/project.rs index 5c22e15..70905fd 100644 --- a/app/src-tauri/src/models/project.rs +++ b/app/src-tauri/src/models/project.rs @@ -132,6 +132,26 @@ fn default_use_shared_auth_token() -> bool { true } +/// `auth_bridge_enabled` defaults to **on**, and the default is what makes +/// `claude login` work at all. +/// +/// The login flow binds a *random* ephemeral loopback port inside the +/// container and then sends the host's browser to `127.0.0.1:`. +/// On the host nothing is listening there, so the callback lands on a closed +/// port and the CLI waits for a redirect that can never arrive. The bridge +/// mirrors the container's loopback listeners onto the same host port, which +/// is the only thing that closes that loop — so off-by-default made a hang the +/// out-of-the-box experience. +/// +/// Returning `true` from a `#[serde(default)]` helper (rather than flipping the +/// constructor alone) is deliberate: existing `projects.json` records were +/// written before this field existed, or while it was off, and an absent key is +/// what the default is read for. A project that wants the old behaviour turns +/// the toggle off, which persists an explicit `false`. +fn default_auth_bridge_enabled() -> bool { + true +} + /// How much autonomy Claude Code is granted inside the container. /// /// Maps onto Claude Code CLI flags — see [`PermissionMode::cli_args`], which is @@ -336,17 +356,30 @@ pub struct Project { pub sandbox_mode_enabled: bool, #[serde(default)] pub mission_control_enabled: bool, - /// Opt in to the auth bridge: while the container runs, its loopback - /// listeners are mirrored onto the host's loopback so browser OAuth - /// callbacks (`claude login`, `fly login`, `aws sso login`) can reach them. + /// The auth bridge: while the container runs, its loopback listeners are + /// mirrored onto the host's loopback so browser OAuth callbacks + /// (`claude login`, `fly login`, `aws sso login`) can reach them. /// Purely host-side — it deliberately has no container-recreation label, /// because toggling it changes nothing about the container itself. - #[serde(default)] + /// + /// **On by default**, and opt-*out* rather than opt-in — see + /// [`default_auth_bridge_enabled`] for why the default is the feature. + #[serde(default = "default_auth_bridge_enabled")] pub auth_bridge_enabled: bool, /// Opt in to the browser-view pane, which watches and takes over the /// browser Claude drives with Playwright inside the container. Purely /// host-side like `auth_bridge_enabled`, so it likewise has no /// container-recreation label. + /// + /// This is the *durable* home of the flag: `BrowserViewManager` reads it + /// rather than keeping its own copy, so the pane comes back the way it was + /// left. Off by default, and unlike the auth bridge it stays that way — a + /// view costs a container exec, a Node daemon and a host port, and a + /// container without Playwright cannot serve one at all. + /// + /// Durable does **not** mean auto-started: nothing brings a viewer up on + /// app start, so a project left enabled reports `enabled` with a state of + /// `Off` until the pane (or `open_page_in_container_browser`) asks for one. #[serde(default)] pub browser_view_enabled: bool, /// Grant the container what a VPN client needs to build a tunnel: @@ -639,7 +672,7 @@ impl Project { allow_docker_access: false, sandbox_mode_enabled: false, mission_control_enabled: false, - auth_bridge_enabled: false, + auth_bridge_enabled: default_auth_bridge_enabled(), browser_view_enabled: false, vpn_support_enabled: false, use_shared_auth_token: default_use_shared_auth_token(), @@ -885,4 +918,69 @@ mod tests { let round_tripped: ClaudeCodeSettings = serde_json::from_str(&json).unwrap(); assert_eq!(round_tripped, partial); } + + // ── The host-side per-project toggles ───────────────────────────────── + + #[test] + fn a_project_stored_before_the_auth_bridge_existed_gets_it_turned_on() { + // The whole point of the serde default: `MAIN_SHAPE_PROJECT` is a real + // record written by a shipped binary and has no `auth_bridge_enabled` + // key at all. Without this, every existing project keeps hanging on + // `claude login` until its owner finds the toggle. + assert!(!MAIN_SHAPE_PROJECT.contains("auth_bridge_enabled")); + let project: Project = serde_json::from_str(MAIN_SHAPE_PROJECT).unwrap(); + assert!(project.auth_bridge_enabled); + + // The browser view is the other way round and must stay so: it costs a + // Node daemon, a container exec loop and a host port, and most + // containers have no Playwright to serve it with. + assert!(!project.browser_view_enabled); + } + + #[test] + fn turning_the_auth_bridge_off_survives_the_default() { + // Opt-out has to be expressible, or the toggle does nothing across a + // restart. An explicit `false` in the file beats the default. + let json = r#"{ "auth_bridge_enabled": false }"#; + #[derive(Deserialize)] + struct JustTheFlag { + #[serde(default = "default_auth_bridge_enabled")] + auth_bridge_enabled: bool, + } + let parsed: JustTheFlag = serde_json::from_str(json).unwrap(); + assert!(!parsed.auth_bridge_enabled); + + // And a saved project always writes the key, so the choice is pinned + // rather than re-defaulted on the next load. + let mut p = Project::new("demo".to_string(), Vec::new()); + p.auth_bridge_enabled = false; + let round_tripped: Project = + serde_json::from_str(&serde_json::to_string(&p).unwrap()).unwrap(); + assert!(!round_tripped.auth_bridge_enabled); + } + + #[test] + fn a_new_project_starts_with_the_bridge_on_and_the_view_off() { + let p = Project::new("demo".to_string(), Vec::new()); + assert!(p.auth_bridge_enabled); + assert!(!p.browser_view_enabled); + } + + #[test] + fn the_path_migration_never_writes_the_flags_and_so_cannot_defeat_the_default() { + // `ProjectsStore::new` runs every record through this before + // deserialising. If it inserted either key — even as `false` — the + // serde default above would never be consulted for an existing project + // and this change would be a no-op on exactly the projects it is for. + let legacy = serde_json::json!({ + "id": "p1", + "name": "demo", + "path": "/home/u/demo", + }); + let migrated = Project::migrate_from_value(legacy); + let obj = migrated.as_object().unwrap(); + assert!(obj.contains_key("paths"), "the migration should still do its own job"); + assert!(!obj.contains_key("auth_bridge_enabled")); + assert!(!obj.contains_key("browser_view_enabled")); + } } diff --git a/app/src-tauri/src/storage/projects_store.rs b/app/src-tauri/src/storage/projects_store.rs index e3ed311..338da50 100644 --- a/app/src-tauri/src/storage/projects_store.rs +++ b/app/src-tauri/src/storage/projects_store.rs @@ -241,6 +241,21 @@ impl ProjectsStore { } } + /// Granular setter for the browser view's opt-in, for the same reason + /// [`Self::set_auth_bridge_enabled`] has one: the pane toggles this while + /// the Config tab may be holding an older copy of the whole record. + pub fn set_browser_view_enabled(&self, project_id: &str, enabled: bool) -> Result<(), String> { + let mut projects = self.lock(); + if let Some(p) = projects.iter_mut().find(|p| p.id == project_id) { + p.browser_view_enabled = enabled; + p.updated_at = chrono::Utc::now().to_rfc3339(); + self.save(&projects)?; + Ok(()) + } else { + Err(format!("Project {} not found", project_id)) + } + } + pub fn set_container_id(&self, project_id: &str, container_id: Option) -> Result<(), String> { let mut projects = self.lock(); if let Some(p) = projects.iter_mut().find(|p| p.id == project_id) { @@ -338,4 +353,61 @@ mod tests { fs::remove_dir_all(&dir).ok(); } + + /// A store over a temp file. `new()` insists on `dirs::data_dir()`, which + /// is the real user's; the fields are right here, so the granular setters + /// can be exercised against a directory the test owns. + fn store_over(dir: &Path, projects: Vec) -> ProjectsStore { + ProjectsStore { + projects: Mutex::new(projects), + file_path: dir.join("projects.json"), + } + } + + #[test] + fn the_browser_view_flag_is_written_to_disk_and_read_back() { + // The point of the whole exercise: before this the flag lived in a + // `HashSet` in `BrowserViewManager` and an app restart forgot it. + let dir = temp_dir("browser-view"); + let project = Project::new("demo".to_string(), Vec::new()); + let id = project.id.clone(); + let store = store_over(&dir, vec![project]); + + assert!(!store.get(&id).unwrap().browser_view_enabled); + store.set_browser_view_enabled(&id, true).unwrap(); + assert!(store.get(&id).unwrap().browser_view_enabled); + + // Durable, not merely in memory — this is what a restart reads. + let on_disk: Vec = + serde_json::from_str(&fs::read_to_string(dir.join("projects.json")).unwrap()).unwrap(); + assert!(on_disk[0].browser_view_enabled); + + store.set_browser_view_enabled(&id, false).unwrap(); + assert!(!store.get(&id).unwrap().browser_view_enabled); + + assert!(store.set_browser_view_enabled("no-such-project", true).is_err()); + + fs::remove_dir_all(&dir).ok(); + } + + #[test] + fn a_granular_toggle_leaves_every_other_field_alone() { + // Why these setters exist at all: the Config tab can be holding an + // older copy of the whole record while the pane flips one flag. + let dir = temp_dir("granular"); + let mut project = Project::new("demo".to_string(), Vec::new()); + project.claude_instructions = Some("keep me".to_string()); + let id = project.id.clone(); + let store = store_over(&dir, vec![project]); + + store.set_browser_view_enabled(&id, true).unwrap(); + store.set_auth_bridge_enabled(&id, false).unwrap(); + + let saved = store.get(&id).unwrap(); + assert_eq!(saved.claude_instructions.as_deref(), Some("keep me")); + assert!(saved.browser_view_enabled); + assert!(!saved.auth_bridge_enabled); + + fs::remove_dir_all(&dir).ok(); + } } diff --git a/app/src-tauri/src/url_open.rs b/app/src-tauri/src/url_open.rs new file mode 100644 index 0000000..7372386 --- /dev/null +++ b/app/src-tauri/src/url_open.rs @@ -0,0 +1,791 @@ +//! Opening a URL in the *host's* browser — the half of triple-c#34 where +//! "Open" appeared to do nothing on Linux. +//! +//! # Why this module exists rather than `openUrl` from `@tauri-apps/plugin-opener` +//! +//! The plugin's Linux path shells out to `xdg-open`, and the child inherits +//! this process's environment verbatim. Inside an AppImage that environment is +//! not the user's — it is the AppImage's, and it is actively hostile to any +//! program that is not the one the bundle was built for: +//! +//! - linuxdeploy's `AppRun`/`AppRun.wrapped` prepends the bundle's own +//! directories to `LD_LIBRARY_PATH`, `PATH`, `XDG_DATA_DIRS`, `PYTHONPATH`, +//! `PERLLIB`, `QT_PLUGIN_PATH` and `GSETTINGS_SCHEMA_DIR`. +//! - `linuxdeploy-plugin-gtk`'s hook adds `GTK_PATH`, `GTK_EXE_PREFIX`, +//! `GTK_DATA_PREFIX`, `GTK_IM_MODULE_FILE`, `GIO_MODULE_DIR` and +//! `GDK_PIXBUF_MODULE_FILE`. +//! - `scripts/finalize-appimage.sh` installs one more hook of our own +//! (`triple-c-wayland-fallback.sh`) that can prepend +//! `$APPDIR/usr/lib/wayland-fallback` to `LD_LIBRARY_PATH`. +//! - `main.rs` sets `WEBKIT_DISABLE_DMABUF_RENDERER` process-wide, and the +//! comment there has flagged this leak for a while: it reaches whatever the +//! app spawns afterwards. +//! +//! A browser that is *already running* is unaffected — `xdg-open` just hands +//! the URL to the existing instance over D-Bus/IPC and the new process exits. +//! A **cold-launched** browser loads our bundled GTK/glib/pixbuf stack against +//! the host's, aborts before it ever paints, and `xdg-open` has already +//! returned 0. From the app's point of view the click did nothing. That is the +//! reported symptom, and it is why the bug only reproduces for some people. +//! +//! # What this does instead +//! +//! `open_url_external` re-validates the URL (see below) and spawns the opener +//! with a **sanitized child environment**. Sanitizing is +//! [`sanitize_child_env`], a pure function over two maps so it can be tested +//! without touching process-wide state: +//! +//! 1. If the AppImage saved the pre-launch value under a `*_ORIG` / +//! `APPIMAGE_ORIGINAL_*` name, restore that. Restoring a saved original is +//! strictly better than unsetting, because the user may genuinely have had +//! an `LD_LIBRARY_PATH` of their own. +//! 2. Otherwise, if the variable differs from the value this process started +//! with, restore the start-up value. That is what undoes *our own* +//! `std::env::set_var` — `main.rs` snapshots the environment via +//! [`capture_pristine_environment`] before any mutation runs. +//! 3. Otherwise, drop only the entries that point inside `$APPDIR`, keeping +//! the rest of the list intact. Blanket-unsetting would also discard +//! whatever the user's session had set; this removes exactly the +//! bundle's own contribution. +//! +//! Nothing is invented: a variable the pristine environment did not have and +//! that does not point into `$APPDIR` is left alone, so outside an AppImage +//! (`cargo tauri dev`, a distro build) this is very close to a no-op. +//! +//! # Portal vs. `xdg-open` +//! +//! `org.freedesktop.portal.OpenURI` would sidestep both the environment leak +//! *and* a missing `x-scheme-handler/https` association, but reaching it means +//! a D-Bus client — `zbus` and its async stack — as a new dependency for one +//! call, on the only platform where we ship a single self-contained binary. +//! It also only helps where a portal is running, which is precisely the +//! desktop-environment case in which `xdg-open` already works once the +//! environment is clean. The environment *is* the bug here, so the cheap fix +//! is the complete one. `gio open` is kept as a second candidate because it +//! goes through GIO's own handler lookup rather than `xdg-open`'s shell +//! heuristics, which covers most of what the portal would have covered. +//! +//! # Security +//! +//! The URL reaching this command originates in an **untrusted container** (see +//! `app/src/lib/urlRelay.ts`). The frontend validates with `sanitizeRelayUrl`, +//! but a compromised webview can call this command directly, so the rules are +//! mirrored here and enforced again: `http`/`https` only, a non-empty host, no +//! embedded credentials, no control characters or whitespace, and a length +//! cap. The URL is never passed through a shell — `std::process::Command` with +//! explicit arguments, so there is no word-splitting, no globbing and no +//! metacharacter to escape. + +use std::collections::BTreeMap; +use std::sync::OnceLock; + +use url::Url; + +/// Hard cap on a URL we will hand to the OS. Mirrors `MAX_RELAY_URL_LENGTH` +/// in `app/src/lib/urlRelay.ts`. +const MAX_URL_LEN: usize = 8192; + +/// The environment this process was started with, captured before anything +/// mutates it. See [`capture_pristine_environment`]. +// Only the Linux spawn path reads these; the macOS/Windows path delegates to +// the opener plugin. Kept unconditional (rather than `#[cfg(linux)]`) so the +// tests and the documentation stay in one piece on every platform. +#[cfg_attr(not(target_os = "linux"), allow(dead_code))] +static PRISTINE_ENV: OnceLock> = OnceLock::new(); + +/// Record the environment as it was at process start. +/// +/// Must be called from `main()` **before** any `std::env::set_var` — today +/// that means before `apply_webkit_wayland_workaround()`, which is the only +/// mutation in the tree. Calling it twice is harmless; the first call wins. +/// +/// This is the only reliable source of truth for "what did the user actually +/// have?" for variables *we* set. It cannot recover what `AppRun` overwrote +/// before `main()` ran — that is what the `*_ORIG` and `$APPDIR` rules in +/// [`sanitize_child_env`] are for. +pub fn capture_pristine_environment() { + let _ = PRISTINE_ENV.set(std::env::vars().collect()); +} + +/// Variables an AppImage launcher is known to override, and that break a +/// cold-launched child that is not this app. +/// +/// `PATH` is in the list for the same reason as the rest: `AppRun` prepends +/// `$APPDIR/usr/bin`, and resolving `xdg-open` (or anything the browser's own +/// wrapper script calls) out of the bundle is its own failure mode. +// Only the Linux spawn path reads these; the macOS/Windows path delegates to +// the opener plugin. Kept unconditional (rather than `#[cfg(linux)]`) so the +// tests and the documentation stay in one piece on every platform. +#[cfg_attr(not(target_os = "linux"), allow(dead_code))] +const SANITIZED_VARS: &[&str] = &[ + "GDK_PIXBUF_MODULEDIR", + "GDK_PIXBUF_MODULE_FILE", + "GIO_MODULE_DIR", + "GSETTINGS_SCHEMA_DIR", + "GTK_DATA_PREFIX", + "GTK_EXE_PREFIX", + "GTK_IM_MODULE_FILE", + "GTK_PATH", + "LD_LIBRARY_PATH", + "PATH", + "PERLLIB", + "PYTHONPATH", + "QT_PLUGIN_PATH", + "XDG_DATA_DIRS", + // Set by `main.rs`, not by AppRun — rule 2 (the pristine snapshot) is what + // removes it, since the pristine environment almost never has it. + "WEBKIT_DISABLE_DMABUF_RENDERER", +]; + +/// What to do to one variable in the child: `Some(value)` sets it, `None` +/// removes it. +// Only the Linux spawn path reads these; the macOS/Windows path delegates to +// the opener plugin. Kept unconditional (rather than `#[cfg(linux)]`) so the +// tests and the documentation stay in one piece on every platform. +#[cfg_attr(not(target_os = "linux"), allow(dead_code))] +type EnvChange = (String, Option); + +/// True when `entry` is `appdir` itself or a path inside it. +// Only the Linux spawn path reads these; the macOS/Windows path delegates to +// the opener plugin. Kept unconditional (rather than `#[cfg(linux)]`) so the +// tests and the documentation stay in one piece on every platform. +#[cfg_attr(not(target_os = "linux"), allow(dead_code))] +fn is_inside(entry: &str, appdir: &str) -> bool { + let appdir = appdir.trim_end_matches('/'); + if appdir.is_empty() { + return false; + } + entry == appdir || entry.strip_prefix(appdir).is_some_and(|r| r.starts_with('/')) +} + +/// Drop the `$APPDIR` entries from a colon-separated list, keeping order and +/// keeping everything else. +/// +/// Single-valued variables (`GDK_PIXBUF_MODULE_FILE`, say) are just lists of +/// one, so they need no separate case: a value inside `$APPDIR` filters down +/// to nothing and the variable is removed. +// Only the Linux spawn path reads these; the macOS/Windows path delegates to +// the opener plugin. Kept unconditional (rather than `#[cfg(linux)]`) so the +// tests and the documentation stay in one piece on every platform. +#[cfg_attr(not(target_os = "linux"), allow(dead_code))] +fn strip_appdir_entries(value: &str, appdir: &str) -> Option { + let kept: Vec<&str> = value + .split(':') + .filter(|entry| !entry.is_empty() && !is_inside(entry, appdir)) + .collect(); + if kept.is_empty() { + None + } else { + Some(kept.join(":")) + } +} + +/// Compute the changes that turn `current` into an environment safe to hand a +/// cold-launched host program. +/// +/// Pure on purpose — `current` and `pristine` are passed in rather than read +/// from the process, so the rules can be tested without a global mutex around +/// the environment. Returns changes sorted by variable name so assertions are +/// deterministic. +// Only the Linux spawn path reads these; the macOS/Windows path delegates to +// the opener plugin. Kept unconditional (rather than `#[cfg(linux)]`) so the +// tests and the documentation stay in one piece on every platform. +#[cfg_attr(not(target_os = "linux"), allow(dead_code))] +fn sanitize_child_env( + current: &BTreeMap, + pristine: &BTreeMap, + appdir: Option<&str>, +) -> Vec { + let mut changes: Vec = Vec::new(); + + for var in SANITIZED_VARS { + let now = current.get(*var); + + // 1. A saved original always wins. Both spellings are checked because + // which one exists depends on the launcher: linuxdeploy's AppRun + // and the various `AppRun.wrapped` generations have used each. + // An empty saved value means "it was unset", not "set it to empty". + let saved = current + .get(&format!("{var}_ORIG")) + .or_else(|| current.get(&format!("APPIMAGE_ORIGINAL_{var}"))); + if let Some(saved) = saved { + let restored = if saved.is_empty() { + None + } else { + Some(saved.clone()) + }; + if restored.as_ref() != now { + changes.push((var.to_string(), restored)); + } + continue; + } + + // 2. We changed it ourselves after start-up — put back what was there. + let at_start = pristine.get(*var); + if at_start != now { + changes.push((var.to_string(), at_start.cloned())); + continue; + } + + // 3. Polluted before `main()` ran, with nothing saved. Remove the + // bundle's own entries and keep the user's. + let (Some(now), Some(appdir)) = (now, appdir) else { + continue; + }; + let stripped = strip_appdir_entries(now, appdir); + if stripped.as_deref() != Some(now.as_str()) { + changes.push((var.to_string(), stripped)); + } + } + + changes.sort_by(|a, b| a.0.cmp(&b.0)); + changes +} + +/// Whether `candidate` holds a character that disqualifies it before parsing. +/// +/// Mirrors `hasForbiddenChar` in `app/src/lib/urlRelay.ts`, and for the same +/// reasons: C0/C1 controls and whitespace are invisible in the UI and are +/// stripped rather than rejected by some URL parsers, and quote characters are +/// illegal in a URL per RFC 3986 while being exactly what an argument-splitting +/// opener downstream would act on. Written as a scan over code points rather +/// than a regex so the control ranges cannot be mangled by an editing tool. +fn has_forbidden_char(candidate: &str) -> bool { + candidate.chars().any(|ch| { + let code = ch as u32; + code <= 0x20 + || code == 0x7f + || (0x80..=0x9f).contains(&code) + || ch == '"' + || ch == '\'' + || ch == '`' + || ch.is_whitespace() + }) +} + +/// Validate a URL an untrusted source asked the host to open. +/// +/// Returns the normalized URL, or a message safe to show the user. The message +/// never echoes the input: it is the input that is untrusted, and this error +/// is rendered in a toast. +fn validate_external_url(raw: &str) -> Result { + // Rust's `trim` strips slightly more than JavaScript's (NEL, U+0085, for + // one), so a string the frontend would have rejected can reach the parser + // here with its edges shaved. That only ever removes outer whitespace — + // everything that survives still has to pass every check below — so the + // divergence cannot widen what gets opened. + let candidate = raw.trim(); + + if candidate.is_empty() { + return Err("Refused to open an empty URL.".to_string()); + } + if candidate.len() > MAX_URL_LEN { + return Err(format!( + "Refused to open a URL longer than {MAX_URL_LEN} characters." + )); + } + if has_forbidden_char(candidate) { + return Err( + "Refused to open a URL containing whitespace, quotes or control characters." + .to_string(), + ); + } + + let parsed = Url::parse(candidate).map_err(|_| "Refused to open a malformed URL.".to_string())?; + + // Scheme allowlist. Nothing else, ever — `file:`, `javascript:`, `data:` + // and every registered protocol handler stay out of reach of the + // container. The scheme is safe to interpolate: the parser restricts it to + // ASCII alphanumerics, `+`, `-` and `.`. + if parsed.scheme() != "http" && parsed.scheme() != "https" { + return Err(format!( + "Refused to open a {}: URL — only http and https are allowed.", + parsed.scheme() + )); + } + if parsed.host_str().is_none_or(str::is_empty) { + return Err("Refused to open a URL with no host.".to_string()); + } + // `https://claude.ai@evil.tld/x` reads as claude.ai anywhere the string is + // truncated, and navigates to evil.tld. + if !parsed.username().is_empty() || parsed.password().is_some() { + return Err("Refused to open a URL containing embedded credentials.".to_string()); + } + + let normalized = parsed.to_string(); + if normalized.len() > MAX_URL_LEN { + return Err(format!( + "Refused to open a URL longer than {MAX_URL_LEN} characters." + )); + } + // A normalized http(s) URL is ASCII by construction — the host is + // punycoded and everything after it is percent-encoded. Asserting it means + // nothing non-ASCII can reach an `execvp` argument, whatever the parser + // decides to do in a future version. + if !normalized.is_ascii() { + return Err("Refused to open a URL with non-ASCII characters.".to_string()); + } + + Ok(normalized) +} + +/// Openers to try, in order, each as (program, leading arguments). +/// +/// `xdg-open` first because it is what the desktop expects to be asked and +/// honours the user's `mimeapps.list`. `gio open` second: it is present +/// wherever glib is (which, for a GTK app's host, is everywhere) and resolves +/// the handler through GIO rather than `xdg-open`'s shell heuristics, so it +/// still works when the `x-scheme-handler/https` association `xdg-open` looks +/// for is missing or points at something broken. +#[cfg(target_os = "linux")] +const OPENERS: &[(&str, &[&str])] = &[("xdg-open", &[]), ("gio", &["open"])]; + +/// How long a candidate opener is given to fail before it is assumed to have +/// worked. +/// +/// `xdg-open` usually returns immediately (it hands the URL to a running +/// browser and exits), but in its generic fallback mode it *is* the browser's +/// parent and stays alive for the session. So "still running" cannot be read +/// as failure, and "exited non-zero quickly" is the only negative signal there +/// is — though not, on its own, a trustworthy one. See +/// [`exit_code_means_nothing_was_launched`]. +#[cfg(target_os = "linux")] +const OPENER_GRACE: std::time::Duration = std::time::Duration::from_millis(400); + +/// Whether a non-zero exit says the opener certainly launched nothing, and so +/// that the next candidate can be tried without risking a second tab. +/// +/// The loop used to treat every quick non-zero exit as "it did nothing" and +/// fall through. That is safe for most of `xdg-open`'s documented codes — 1 +/// (syntax), 2 (file not found) and 3 (a required tool could not be found) are +/// all statements that it never got as far as launching a handler, and 3 is the +/// missing-association case `gio open` is in [`OPENERS`] for. 127 is the same +/// statement made by a shell, which is how a `$BROWSER` or `x-www-browser` +/// wrapper naming a program that does not exist comes back. +/// +/// Code 4 is the one that cannot be read that way, and it is the catch-all: +/// "the action failed" also covers a handler that *was* launched and then +/// returned non-zero. A browser that takes the URL, opens the tab in an already +/// running instance and exits non-zero for its own reasons ends up here, as +/// does a wrapper script that does its job and then returns the exit status of +/// something else. Falling through on that hands the same URL to a second +/// opener: two tabs for one click, and for an OAuth link two authorize +/// requests. +/// +/// So anything not recognised below — 4, an unfamiliar code, or a death by +/// signal (`code()` is `None`) — ends the loop rather than continuing it. The +/// caller is told the opener failed, which is the honest report of an +/// ambiguous outcome, and no second request is made on the user's behalf. Note +/// what this costs: an opener that genuinely failed with code 4 no longer falls +/// through to `gio`, so a user whose `xdg-open` fails that way sees an error +/// where they previously might have got a tab. +/// +/// This is reasoning from `xdg-open`'s documented exit codes, not from an +/// observed double-open in this app. +#[cfg(target_os = "linux")] +fn exit_code_means_nothing_was_launched(code: Option) -> bool { + matches!(code, Some(1 | 2 | 3 | 127)) +} + +/// Spawn `url` with an opener, under a sanitized environment. +#[cfg(target_os = "linux")] +fn spawn_with_clean_env(url: &str) -> Result<(), String> { + let current: BTreeMap = std::env::vars().collect(); + let pristine = PRISTINE_ENV.get().cloned().unwrap_or_else(|| current.clone()); + let appdir = current.get("APPDIR").cloned(); + let changes = sanitize_child_env(¤t, &pristine, appdir.as_deref()); + + let mut failures: Vec = Vec::new(); + + for (program, leading) in OPENERS { + let mut command = std::process::Command::new(program); + command.args(*leading).arg(url); + // The bundle's own identity is not the child's business either, and a + // browser that re-execs itself through a wrapper script can pick these + // up. + for var in ["APPDIR", "APPIMAGE", "ARGV0", "OWD"] { + command.env_remove(var); + } + for (key, value) in &changes { + match value { + Some(value) => command.env(key, value), + None => command.env_remove(key), + }; + } + // Detached: the opener must not inherit our stdio, or a browser + // writing to stderr keeps a pipe to us open for the session. + command + .stdin(std::process::Stdio::null()) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()); + + // A spawn failure — `ErrorKind::NotFound` for an opener that is not + // installed, `PermissionDenied` for one that cannot be executed — is + // the unambiguous case: nothing ran, so nothing was opened, and the + // next candidate is free to try. + let mut child = match command.spawn() { + Ok(child) => child, + Err(err) => { + failures.push(format!("{program}: {err}")); + continue; + } + }; + + std::thread::sleep(OPENER_GRACE); + match child.try_wait() { + Ok(Some(status)) if !status.success() => { + failures.push(format!("{program} exited with {status}")); + // A program that *ran* is not a program that did nothing. + if !exit_code_means_nothing_was_launched(status.code()) { + return Err(format!( + "Could not confirm the link opened. Tried: {}. It may have opened anyway \ + — check your browser before trying again.", + failures.join("; ") + )); + } + continue; + } + Ok(_) => {} + Err(err) => { + failures.push(format!("{program}: could not be waited on: {err}")); + continue; + } + } + + // Still running (it is the browser's parent) — reap it off-thread so it + // does not become a zombie for the life of the app. + std::thread::spawn(move || { + let _ = child.wait(); + }); + return Ok(()); + } + + Err(format!( + "Could not open the link. Tried: {}. Check that xdg-utils is installed and that a default browser is set.", + failures.join("; ") + )) +} + +/// Open `url` in the user's browser. +/// +/// On Linux this goes through [`spawn_with_clean_env`] rather than +/// `@tauri-apps/plugin-opener`, for the AppImage reasons in this module's +/// documentation (triple-c#34). macOS and Windows keep the plugin's path — +/// neither has the environment problem, and `open`/`ShellExecute` are the +/// right calls there — but they are reached through this same command so the +/// frontend has one call site with one set of validation rules. +/// +/// Errors are returned rather than logged-and-swallowed: "Open" silently doing +/// nothing is the bug being fixed, so the failure has to be something the UI +/// can show. +#[tauri::command] +pub async fn open_url_external(app: tauri::AppHandle, url: String) -> Result<(), String> { + let validated = validate_external_url(&url)?; + + #[cfg(target_os = "linux")] + { + let _ = &app; + tauri::async_runtime::spawn_blocking(move || spawn_with_clean_env(&validated)) + .await + .map_err(|err| format!("Could not open the link: {err}"))? + } + + #[cfg(not(target_os = "linux"))] + { + use tauri_plugin_opener::OpenerExt; + app.opener() + .open_url(validated, None::<&str>) + .map_err(|err| format!("Could not open the link: {err}")) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn map(pairs: &[(&str, &str)]) -> BTreeMap { + pairs + .iter() + .map(|(k, v)| (k.to_string(), v.to_string())) + .collect() + } + + // ── URL re-validation ──────────────────────────────────────────────── + + #[test] + fn plain_http_and_https_urls_are_accepted() { + for url in [ + "https://claude.ai/", + "http://localhost:1420/callback?code=abc", + "https://example.com/path#frag", + ] { + assert!(validate_external_url(url).is_ok(), "{url} should be allowed"); + } + } + + #[test] + fn urls_are_returned_normalized() { + assert_eq!( + validate_external_url("https://Example.COM").unwrap(), + "https://example.com/" + ); + } + + #[test] + fn only_http_and_https_survive() { + for url in [ + "file:///etc/passwd", + "javascript:alert(1)", + "data:text/html,