From 15b51f8d2d13c145cd18df030571a2db0abf4988 Mon Sep 17 00:00:00 2001 From: zhom <2717306+zhom@users.noreply.github.com> Date: Thu, 27 Aug 2026 09:16:25 +0400 Subject: [PATCH] refactor: cleanup --- .github/workflows/docker-sync.yml | 43 + .node-version | 2 +- .nvmrc | 2 +- donut-sync/docker-compose.yml | 11 + donut-sync/src/sync/sync.service.ts | 44 +- donut-sync/test/presign-endpoint.e2e-spec.ts | 71 +- package.json | 2 +- src-tauri/Cargo.lock | 2 +- src-tauri/Cargo.toml | 6 +- src-tauri/src/app_auto_updater.rs | 44 +- src-tauri/src/cloud_auth.rs | 52 +- src-tauri/src/default_browser.rs | 778 +++++++++++++++---- src-tauri/src/lib.rs | 151 ++-- src-tauri/src/sync/client.rs | 52 +- src-tauri/src/sync/engine.rs | 111 ++- src-tauri/src/sync/mod.rs | 93 +++ src-tauri/src/sync/preflight.rs | 71 ++ src-tauri/src/sync/scheduler.rs | 168 +++- src-tauri/tauri.conf.json | 2 +- src/components/settings-dialog.tsx | 32 +- src/i18n/locales/en.json | 6 +- src/i18n/locales/es.json | 6 +- src/i18n/locales/fr.json | 6 +- src/i18n/locales/ja.json | 6 +- src/i18n/locales/ko.json | 6 +- src/i18n/locales/pt.json | 6 +- src/i18n/locales/ru.json | 6 +- src/i18n/locales/tr.json | 6 +- src/i18n/locales/vi.json | 6 +- src/i18n/locales/zh.json | 6 +- src/types.ts | 13 + 31 files changed, 1471 insertions(+), 339 deletions(-) diff --git a/.github/workflows/docker-sync.yml b/.github/workflows/docker-sync.yml index 2bf3fc7..48f6cfb 100644 --- a/.github/workflows/docker-sync.yml +++ b/.github/workflows/docker-sync.yml @@ -31,7 +31,50 @@ env: IMAGE_NAME: donutbrowser/donut-sync jobs: + # donut-sync's own end-to-end suite covers which host it signs into presigned + # URLs. That is the whole of the self-hosted sync failure in issue 534: sign + # against an address only the server can reach and every client transfer dies + # at connect while /health and /readyz stay green. The suite existed and was + # never run by anything, so the guard was decorative. Run it here, before the + # image ships, because an image with broken presigning is the thing that + # reaches users. + # + # Ubuntu only, and separate from the Rust and Node matrices, because it needs + # Docker for MinIO and a POSIX env-var prefix in the package script. + test: + runs-on: ubuntu-latest + steps: + - name: Checkout repository + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 #v7.0.1 + + - name: Set up pnpm package manager + uses: pnpm/action-setup@0977fd99725f1db4007ccb2928dbb4e90d06cc86 #v6.0.10 + with: + run_install: false + + - name: Set up Node.js + uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 #v7.0.0 + with: + node-version-file: .node-version + cache: "pnpm" + + - name: Install dependencies + run: pnpm install --frozen-lockfile + + # Publishes MinIO on 8987, which is the port test/test-env.ts pins. + - name: Start test storage + run: docker compose -f donut-sync/docker-compose.yml up -d --wait + + - name: Run donut-sync end-to-end tests + working-directory: ./donut-sync + run: pnpm test:e2e + + - name: Stop test storage + if: always() + run: docker compose -f donut-sync/docker-compose.yml down -v + build-and-push: + needs: test runs-on: ubuntu-latest steps: - name: Checkout repository diff --git a/.node-version b/.node-version index 853058d..e63d7ea 100644 --- a/.node-version +++ b/.node-version @@ -1,2 +1,2 @@ -23 +24 diff --git a/.nvmrc b/.nvmrc index 03ff60b..fce7a4d 100644 --- a/.nvmrc +++ b/.nvmrc @@ -1 +1 @@ -23 \ No newline at end of file +24 \ No newline at end of file diff --git a/donut-sync/docker-compose.yml b/donut-sync/docker-compose.yml index 222b590..98b527a 100644 --- a/donut-sync/docker-compose.yml +++ b/donut-sync/docker-compose.yml @@ -1,3 +1,14 @@ +# Storage for developing and testing donut-sync itself. It runs MinIO only, and +# the sync server is expected to run on the host beside it (`pnpm start:dev`), +# which is why MinIO is published and why the port matches the one pinned in +# test/test-env.ts. +# +# This is NOT the self-hosting compose file. That one runs donut-sync in a +# container too, and it must set S3_PUBLIC_ENDPOINT, because a server that signs +# presigned URLs against a compose-internal host such as `http://minio:9000` +# hands every device a URL it cannot open, while /health and /readyz stay green. +# Take the self-hosting compose from https://donutbrowser.com/docs/self-hosting +# rather than from here. services: minio: image: minio/minio:latest diff --git a/donut-sync/src/sync/sync.service.ts b/donut-sync/src/sync/sync.service.ts index a78f756..6da9f30 100644 --- a/donut-sync/src/sync/sync.service.ts +++ b/donut-sync/src/sync/sync.service.ts @@ -86,6 +86,11 @@ export class SyncService implements OnModuleInit { // `S3_PUBLIC_ENDPOINT` names a different, client-reachable address. private presignClient: S3Client; private publicEndpoint: string; + /** + * Whether an operator chose the public endpoint, or it fell back to the + * server's own storage address. The fallback is the shape that fails. + */ + private publicEndpointWasConfigured: boolean; private bucket: string; // Upper bound on presign batch array length (DoS guard). private static readonly MAX_BATCH_ITEMS = 1000; @@ -131,9 +136,11 @@ export class SyncService implements OnModuleInit { // network and nowhere else. Signing is bound to the host, so the presign // client is a second client pinned to the public address rather than a // string rewrite of the signed URL. - const publicEndpoint = - this.configService.get("S3_PUBLIC_ENDPOINT") || endpoint; + const configuredPublicEndpoint = + this.configService.get("S3_PUBLIC_ENDPOINT"); + const publicEndpoint = configuredPublicEndpoint || endpoint; this.publicEndpoint = publicEndpoint; + this.publicEndpointWasConfigured = Boolean(configuredPublicEndpoint); this.presignClient = publicEndpoint === endpoint ? this.s3Client @@ -191,14 +198,33 @@ export class SyncService implements OnModuleInit { const isSingleLabel = !host.includes(".") && !host.includes(":") && host !== "localhost"; - if (!isSingleLabel) return; - this.logger.warn( - `Storage endpoint '${this.publicEndpoint}' uses the container-only host '${host}'. ` + - "Presigned URLs built from it cannot be reached by Donut Browser, so every " + - "transfer will fail while /health and /readyz stay green. Set S3_PUBLIC_ENDPOINT " + - "to an address your devices can reach (and publish that port).", - ); + if (isSingleLabel) { + this.logger.warn( + `Storage endpoint '${this.publicEndpoint}' uses the container-only host '${host}'. ` + + "Presigned URLs built from it cannot be reached by Donut Browser, so every " + + "transfer will fail while /health and /readyz stay green. Set S3_PUBLIC_ENDPOINT " + + "to an address your devices can reach (and publish that port).", + ); + return; + } + + // A dotted host proves nothing. With `S3_PUBLIC_ENDPOINT` unset, clients are + // handed whatever address this server uses for storage itself, and a + // reachable-looking name such as `storage.internal`, or a private address on + // a network the devices are not on, fails in exactly the same way while + // saying nothing at all. This server cannot test the endpoint for them, + // because it does not know where its clients are, so state what it does + // know and leave the judgement to the operator. + if (!this.publicEndpointWasConfigured) { + this.logger.log( + `S3_PUBLIC_ENDPOINT is not set, so presigned URLs will name '${this.publicEndpoint}', ` + + "the address this server uses for storage itself. Transfers go straight from each " + + "device to that address, and this server cannot verify a device can reach it. If " + + "transfers fail while /health and /readyz stay green, set S3_PUBLIC_ENDPOINT to an " + + "address your devices can reach and publish that port.", + ); + } } private async ensureBucketExists(): Promise { diff --git a/donut-sync/test/presign-endpoint.e2e-spec.ts b/donut-sync/test/presign-endpoint.e2e-spec.ts index 368d7c7..8f72a25 100644 --- a/donut-sync/test/presign-endpoint.e2e-spec.ts +++ b/donut-sync/test/presign-endpoint.e2e-spec.ts @@ -1,4 +1,4 @@ -import { INestApplication } from "@nestjs/common"; +import { INestApplication, Logger } from "@nestjs/common"; import { ConfigModule } from "@nestjs/config"; import { Test, TestingModule } from "@nestjs/testing"; import request from "supertest"; @@ -199,3 +199,72 @@ describe("presigned URL host", () => { }); }); }); + +// The server cannot test whether a device can reach the endpoint it signs, so +// the only honest thing it can do is say what it is handing out. Without this, +// the one configuration that breaks every transfer boots completely silently. +describe("boot message about the presign endpoint", () => { + let logs: string[]; + let warnings: string[]; + let logSpy: jest.SpyInstance; + let warnSpy: jest.SpyInstance; + + beforeEach(() => { + logs = []; + warnings = []; + logSpy = jest + .spyOn(Logger.prototype, "log") + .mockImplementation((message: unknown) => { + logs.push(String(message)); + }); + warnSpy = jest + .spyOn(Logger.prototype, "warn") + .mockImplementation((message: unknown) => { + warnings.push(String(message)); + }); + }); + + afterEach(() => { + logSpy.mockRestore(); + warnSpy.mockRestore(); + }); + + it("says which host clients will be handed when S3_PUBLIC_ENDPOINT is unset", async () => { + const app = await bootstrap(undefined); + try { + const spoken = [...logs, ...warnings].join("\n"); + expect(spoken).toContain("S3_PUBLIC_ENDPOINT"); + expect(spoken).toContain(TEST_S3_ENDPOINT); + } finally { + await app.close(); + } + }); + + // A single-label host is the documented compose default and cannot work for + // any client, so it earns a warning rather than a note. + it("warns loudly about a container-only host", async () => { + const app = await bootstrap("http://minio:9000"); + try { + const spoken = warnings.join("\n"); + expect(spoken).toContain("minio"); + expect(spoken).toContain("S3_PUBLIC_ENDPOINT"); + } finally { + delete process.env.S3_PUBLIC_ENDPOINT; + await app.close(); + } + }); + + // An operator who set the variable made a choice. Repeating the note at them + // would train them to ignore it, and the warning above is for the value that + // provably cannot work, not for every value the server cannot verify. + it("stays quiet when an operator has chosen a routable endpoint", async () => { + const app = await bootstrap(PUBLIC_ENDPOINT); + try { + const spoken = [...logs, ...warnings].join("\n"); + expect(spoken).not.toContain("S3_PUBLIC_ENDPOINT is not set"); + } finally { + delete process.env.S3_PUBLIC_ENDPOINT; + await app.close(); + } + }); +}); diff --git a/package.json b/package.json index a8c3378..8f70079 100644 --- a/package.json +++ b/package.json @@ -2,7 +2,7 @@ "name": "donutbrowser", "private": true, "license": "AGPL-3.0", - "version": "0.29.6", + "version": "0.30.0", "type": "module", "scripts": { "predev": "pnpm licenses:generate", diff --git a/src-tauri/Cargo.lock b/src-tauri/Cargo.lock index dd7119f..cce72ee 100644 --- a/src-tauri/Cargo.lock +++ b/src-tauri/Cargo.lock @@ -1735,7 +1735,7 @@ dependencies = [ [[package]] name = "donutbrowser" -version = "0.29.6" +version = "0.30.0" dependencies = [ "aes 0.9.2", "aes-gcm 0.11.1", diff --git a/src-tauri/Cargo.toml b/src-tauri/Cargo.toml index 83f8bc6..1cd41be 100644 --- a/src-tauri/Cargo.toml +++ b/src-tauri/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "donutbrowser" -version = "0.29.6" +version = "0.30.0" description = "Simple Yet Powerful Anti-Detect Browser" authors = ["zhom@github"] edition = "2021" @@ -147,6 +147,10 @@ windows = { version = "0.62", features = [ "Win32_Security", "Win32_Storage_FileSystem", "Win32_System_Registry", + # CoInitializeEx, so the `ms-settings:` hand-off in default_browser.rs has a + # COM apartment. ShellExecuteW activates the URI through a shell extension, + # and it runs on a `spawn_blocking` thread that has no apartment of its own. + "Win32_System_Com", "Win32_UI_Shell", # SendMessageTimeoutW, for the association-change broadcast in # default_browser.rs. Going through the crate rather than a hand-written diff --git a/src-tauri/src/app_auto_updater.rs b/src-tauri/src/app_auto_updater.rs index 399f499..0594758 100644 --- a/src-tauri/src/app_auto_updater.rs +++ b/src-tauri/src/app_auto_updater.rs @@ -1872,36 +1872,34 @@ rm "{}" parameters ); - // windows-sys is not a direct dep, so use the raw FFI via the - // windows crate that Tauri pulls in. ShellExecuteW returns an - // HINSTANCE > 32 on success. - #[link(name = "shell32")] - extern "system" { - fn ShellExecuteW( - hwnd: *mut std::ffi::c_void, - operation: *const u16, - file: *const u16, - parameters: *const u16, - directory: *const u16, - show_cmd: i32, - ) -> isize; - } - const SW_SHOWNORMAL: i32 = 1; - let open: Vec = "open\0".encode_utf16().collect(); + // Take the binding from the `windows` crate rather than writing the + // declaration here. A hand-written one is what put the wrong width on + // `SendMessageTimeoutA`'s out-parameter in `default_browser.rs`, and + // that killed the process on every click of "Set as default browser". + // No compiler and no lint can see such a mistake. The generated binding + // cannot drift from the real ABI, so there is nothing to get wrong. + use windows::core::{w, PCWSTR}; + use windows::Win32::UI::Shell::ShellExecuteW; + use windows::Win32::UI::WindowsAndMessaging::SW_SHOWNORMAL; let result = unsafe { ShellExecuteW( - std::ptr::null_mut(), - open.as_ptr(), - file_w.as_ptr(), - params_w.as_ptr(), - std::ptr::null(), + None, + w!("open"), + PCWSTR(file_w.as_ptr()), + PCWSTR(params_w.as_ptr()), + PCWSTR::null(), SW_SHOWNORMAL, ) }; - if result as usize <= 32 { - return Err(format!("ShellExecuteW failed with code {result}").into()); + // ShellExecuteW reports success as a value above 32. Anything at or + // below that is an error code wearing a handle's type. Read it as a + // signed value: the old `as usize` turned every negative code into a + // very large number, which read as success. + let code = result.0 as isize; + if code <= 32 { + return Err(format!("ShellExecuteW failed with code {code}").into()); } } else { // No pending installer — just restart the app. Use a minimal diff --git a/src-tauri/src/cloud_auth.rs b/src-tauri/src/cloud_auth.rs index 4269dac..4292577 100644 --- a/src-tauri/src/cloud_auth.rs +++ b/src-tauri/src/cloud_auth.rs @@ -11,7 +11,6 @@ use serde::{Deserialize, Serialize}; use sha2::{Digest, Sha256}; use std::fs; use std::path::PathBuf; -use std::sync::Arc; use tokio::sync::Mutex; use crate::browser::ProxySettings; @@ -1618,54 +1617,11 @@ pub async fn cloud_get_proxy_usage() -> Result, String> #[tauri::command] pub async fn restart_sync_service(app_handle: tauri::AppHandle) -> Result<(), String> { - // Stop existing scheduler - if let Some(scheduler) = sync::get_global_scheduler() { - scheduler.stop(); - } - - // Restart sync pipeline - let app_handle_sync = app_handle.clone(); + // Rebuilding the pipeline reaches the network, so do it off the command and + // let the caller's dialog close. `start_pipeline` retires the previous + // scheduler and the previous subscription itself. tauri::async_runtime::spawn(async move { - let mut subscription_manager = sync::SubscriptionManager::new(); - let work_rx = subscription_manager.take_work_receiver(); - - if let Err(e) = subscription_manager.start(app_handle_sync.clone()).await { - log::warn!("Failed to start sync subscription: {e}"); - return; - } - - if let Some(work_rx) = work_rx { - let scheduler = Arc::new(sync::SyncScheduler::new()); - sync::set_global_scheduler(scheduler.clone()); - - scheduler.sync_all_enabled_profiles(&app_handle_sync).await; - - match sync::SyncEngine::create_from_settings(&app_handle_sync).await { - Ok(engine) => { - if let Err(e) = engine - .check_for_missing_synced_profiles(&app_handle_sync) - .await - { - log::warn!("Failed to check for missing profiles: {}", e); - } - if let Err(e) = engine - .check_for_missing_synced_entities(&app_handle_sync) - .await - { - log::warn!("Failed to check for missing entities: {}", e); - } - } - Err(e) => { - log::warn!("Sync not configured, skipping missing profile check: {}", e); - } - } - - scheduler - .clone() - .start(app_handle_sync.clone(), work_rx) - .await; - log::info!("Sync scheduler restarted"); - } + sync::start_pipeline(app_handle).await; }); Ok(()) diff --git a/src-tauri/src/default_browser.rs b/src-tauri/src/default_browser.rs index 426e69c..3b10e83 100644 --- a/src-tauri/src/default_browser.rs +++ b/src-tauri/src/default_browser.rs @@ -1,7 +1,31 @@ +use serde::Serialize; use tauri::command; pub struct DefaultBrowser {} +/// What happened when the user asked Donut to become the default browser. +/// +/// macOS and Linux let a program make the change itself. Windows does not. The +/// registry value that decides the handler carries a signature only the shell +/// can produce, so the most a program may do is register itself and open the +/// page where the user makes the choice. Without this distinction the caller +/// reports a change that has not happened yet, which is what the Windows path +/// used to do. +/// +/// Each platform builds exactly one of these, so on any single target the other +/// one reads as never constructed. That is what the allow is for: the variant is +/// live, just not on the host being compiled. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize)] +#[serde(rename_all = "camelCase", tag = "status")] +#[allow(dead_code)] +pub enum SetDefaultOutcome { + /// Donut is the default browser now. Nothing is left for the user to do. + Set, + /// Registration is complete and the system settings page is open. The user + /// makes the final choice there. + AwaitingSystemSettings, +} + impl DefaultBrowser { fn new() -> Self { Self {} @@ -21,7 +45,7 @@ impl DefaultBrowser { // Linux answers this by running `xdg-mime`, a shell script that forks // further. That is blocking work with no upper bound, and this command // runs on the same async runtime as every other command, the REST API and - // the sync scheduler — so doing it inline occupies a worker for as long as + // the sync scheduler, so doing it inline occupies a worker for as long as // the desktop takes to answer. The Settings page polls this on a timer. #[cfg(target_os = "linux")] return blocking(linux::is_default_browser).await; @@ -30,16 +54,22 @@ impl DefaultBrowser { Err("Unsupported platform".to_string()) } - pub async fn set_as_default_browser(&self) -> Result<(), String> { + pub async fn set_as_default_browser(&self) -> Result { #[cfg(target_os = "macos")] - return macos::set_as_default_browser(); + return macos::set_as_default_browser().map(|()| SetDefaultOutcome::Set); + // Windows writes several registry trees, broadcasts `WM_SETTINGCHANGE` to + // every top-level window on the desktop and then hands off to the shell. + // The broadcast alone costs about 130 ms on an idle desktop and seconds on + // a busy one, so this does not belong on a runtime worker either. #[cfg(target_os = "windows")] - return windows::set_as_default_browser(); + return blocking(windows::set_as_default_browser).await; // Same reasoning, and this one additionally sleeps 500ms before verifying. #[cfg(target_os = "linux")] - return blocking(linux::set_as_default_browser).await; + return blocking(linux::set_as_default_browser) + .await + .map(|()| SetDefaultOutcome::Set); #[cfg(not(any(target_os = "macos", target_os = "windows", target_os = "linux")))] Err("Unsupported platform".to_string()) @@ -47,7 +77,7 @@ impl DefaultBrowser { } /// Run blocking work off the async runtime's worker threads. -#[cfg(target_os = "linux")] +#[cfg(any(target_os = "linux", target_os = "windows"))] async fn blocking(work: F) -> Result where F: FnOnce() -> Result + Send + 'static, @@ -124,18 +154,44 @@ mod macos { #[cfg(target_os = "windows")] #[allow(clippy::needless_borrows_for_generic_args)] mod windows { + use super::SetDefaultOutcome; use std::path::Path; use winreg::enums::*; use winreg::RegKey; + /// The key Windows knows us by. Never shown to a person. const APP_NAME: &str = "DonutBrowser"; + /// The name Windows shows in "Default apps" and in "Open with". + const DISPLAY_NAME: &str = "Donut Browser"; + const DESCRIPTION: &str = "Donut Browser - Simple Yet Powerful Anti-Detect Browser"; const PROG_ID: &str = "DonutBrowser.HTML"; - pub fn is_default_browser() -> Result { - let schemes = ["http", "https"]; + /// A web browser registers under `StartMenuInternet`, and + /// `RegisteredApplications` points at the `Capabilities` subkey of that + /// entry. Edge, Chrome and Firefox all do exactly this, and the shell reads + /// the capability data from there. + /// + /// The previous layout invented its own key at `Software\DonutBrowser` and + /// pointed `RegisteredApplications` at the parent instead of at + /// `Capabilities`. Every other entry on a normal machine ends in + /// `Capabilities`. The shell found no capability data, so Donut was never + /// offered as a browser and the button appeared to do nothing. + const CLIENT_KEY: &str = r"Software\Clients\StartMenuInternet\DonutBrowser"; + /// The value written into `RegisteredApplications`. + const CAPABILITIES_KEY: &str = r"Software\Clients\StartMenuInternet\DonutBrowser\Capabilities"; + /// The layout earlier builds wrote. Removed on every run, so a machine that + /// ran one of those does not keep stale capability data claiming http. + const LEGACY_APP_KEY: &str = r"Software\DonutBrowser"; - for scheme in schemes { - // Check if our browser is set as the default handler for this scheme + const URL_SCHEMES: [&str; 2] = ["http", "https"]; + /// The file types a browser is asked to open from Explorer. The ProgId + /// command passes the path through as `%1`, and `urls_from_args` in `lib.rs` + /// turns a path into a `file://` URL, so every extension listed here can + /// actually be serviced. Do not add one that cannot. + const FILE_EXTENSIONS: [&str; 4] = [".htm", ".html", ".shtml", ".xhtml"]; + + pub fn is_default_browser() -> Result { + for scheme in URL_SCHEMES { if !is_default_for_scheme(scheme)? { return Ok(false); } @@ -144,44 +200,42 @@ mod windows { Ok(true) } - pub fn set_as_default_browser() -> Result<(), String> { - // Get the current executable path - let exe_path = std::env::current_exe() - .map_err(|e| format!("Failed to get current executable path: {}", e))?; + pub fn set_as_default_browser() -> Result { + let exe_path = + std::env::current_exe().map_err(|e| format!("Failed to get current executable path: {e}"))?; - let exe_path_str = exe_path + let exe_path = exe_path .to_str() .ok_or("Failed to convert executable path to string")?; - // Verify the executable exists - if !Path::new(exe_path_str).exists() { - return Err(format!("Executable not found at: {}", exe_path_str)); + if !Path::new(exe_path).exists() { + return Err(format!("Executable not found at: {exe_path}")); } - // Register the application - register_application(exe_path_str)?; + let hkcu = RegKey::predef(HKEY_CURRENT_USER); + remove_legacy_registration(&hkcu); + register_prog_id(&hkcu, exe_path)?; + register_client(&hkcu, exe_path)?; + register_file_extensions(&hkcu)?; + register_application(&hkcu)?; - // Set as default for HTTP and HTTPS - set_default_for_scheme("http")?; - set_default_for_scheme("https")?; - - // Register file associations for HTML files - register_html_file_association(exe_path_str)?; - - // Notify the system of changes notify_system_of_changes(); - Ok(()) + open_default_apps_settings()?; + + Ok(SetDefaultOutcome::AwaitingSystemSettings) + } + + /// Wrap a path in the quotes the shell expects around a command or an icon. + fn quoted(value: &str) -> String { + format!(r#""{value}""#) } fn is_default_for_scheme(scheme: &str) -> Result { let hkcu = RegKey::predef(HKEY_CURRENT_USER); - // Check Software\Microsoft\Windows\Shell\Associations\UrlAssociations\{scheme}\UserChoice - let path = format!( - "Software\\Microsoft\\Windows\\Shell\\Associations\\UrlAssociations\\{}\\UserChoice", - scheme - ); + let path = + format!(r"Software\Microsoft\Windows\Shell\Associations\UrlAssociations\{scheme}\UserChoice"); match hkcu.open_subkey(&path) { Ok(key) => match key.get_value::("ProgId") { @@ -192,167 +246,276 @@ mod windows { } } - fn register_application(exe_path: &str) -> Result<(), String> { - let hkcu = RegKey::predef(HKEY_CURRENT_USER); + /// Delete the layout earlier builds wrote. + /// + /// Nothing else in this application has ever written under that key, so + /// removing it cannot lose anything a user cares about. Leaving it would + /// leave a second `Capabilities` block claiming http and https from a key the + /// shell no longer reads. + /// + /// The old code also wrote the ProgId into the default value of + /// `Software\Classes\.html` and `.htm`. That value is the association itself, + /// and it was never ours to take. Give it back, but only where it still holds + /// the ProgId we wrote. Any other value is the user's own choice and is left + /// alone. + fn remove_legacy_registration(root: &RegKey) { + match root.delete_subkey_all(LEGACY_APP_KEY) { + Ok(()) => log::debug!("Removed the superseded default-browser registration key"), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(e) => log::debug!("Could not remove the superseded registration key: {e}"), + } - // Register in Software\RegisteredApplications - let (registered_apps, _) = hkcu - .create_subkey("Software\\RegisteredApplications") - .map_err(|e| format!("Failed to create RegisteredApplications key: {}", e))?; + for extension in [".htm", ".html"] { + let path = format!(r"Software\Classes\{extension}"); + let Ok(key) = root.open_subkey_with_flags(&path, KEY_READ | KEY_SET_VALUE) else { + continue; + }; - registered_apps - .set_value(APP_NAME, &format!("Software\\{}", APP_NAME)) - .map_err(|e| format!("Failed to set registered application: {}", e))?; + let ours = key + .get_value::("") + .map(|value| value == PROG_ID) + .unwrap_or(false); - // Create application key - let (app_key, _) = hkcu - .create_subkey(&format!("Software\\{}", APP_NAME)) - .map_err(|e| format!("Failed to create application key: {}", e))?; - - // Set application properties - app_key - .set_value("ApplicationName", &APP_NAME) - .map_err(|e| format!("Failed to set ApplicationName: {}", e))?; - - app_key - .set_value( - "ApplicationDescription", - &"Donut Browser - Simple Yet Powerful Anti-Detect Browser", - ) - .map_err(|e| format!("Failed to set ApplicationDescription: {}", e))?; - - app_key - .set_value("ApplicationIcon", &format!("\"{}\",0", exe_path)) - .map_err(|e| format!("Failed to set ApplicationIcon: {}", e))?; - - // Create Capabilities key - let (capabilities, _) = app_key - .create_subkey("Capabilities") - .map_err(|e| format!("Failed to create Capabilities key: {}", e))?; - - capabilities - .set_value( - "ApplicationDescription", - &"Donut Browser - Simple Yet Powerful Anti-Detect Browser", - ) - .map_err(|e| format!("Failed to set Capabilities description: {}", e))?; - - // Set URL associations - let (url_assoc, _) = capabilities - .create_subkey("URLAssociations") - .map_err(|e| format!("Failed to create URLAssociations key: {}", e))?; - - url_assoc - .set_value("http", &PROG_ID) - .map_err(|e| format!("Failed to set http association: {}", e))?; - - url_assoc - .set_value("https", &PROG_ID) - .map_err(|e| format!("Failed to set https association: {}", e))?; - - // Set file associations - let (file_assoc, _) = capabilities - .create_subkey("FileAssociations") - .map_err(|e| format!("Failed to create FileAssociations key: {}", e))?; - - file_assoc - .set_value(".html", &PROG_ID) - .map_err(|e| format!("Failed to set .html association: {}", e))?; - - file_assoc - .set_value(".htm", &PROG_ID) - .map_err(|e| format!("Failed to set .htm association: {}", e))?; - - // Register the ProgID - register_prog_id(exe_path)?; - - Ok(()) + if ours { + match key.delete_value("") { + Ok(()) => log::debug!("Released the {extension} association taken by an older build"), + Err(e) => log::debug!("Could not release the {extension} association: {e}"), + } + } + } } - fn register_prog_id(exe_path: &str) -> Result<(), String> { - let hkcu = RegKey::predef(HKEY_CURRENT_USER); - - // Create ProgID key - let (prog_id_key, _) = hkcu - .create_subkey(&format!("Software\\Classes\\{}", PROG_ID)) - .map_err(|e| format!("Failed to create ProgID key: {}", e))?; + /// Describe the document type Donut opens, and how to open one. + fn register_prog_id(root: &RegKey, exe_path: &str) -> Result<(), String> { + let (prog_id_key, _) = root + .create_subkey(format!(r"Software\Classes\{PROG_ID}")) + .map_err(|e| format!("Failed to create ProgID key: {e}"))?; prog_id_key .set_value("", &"Donut Browser Document") - .map_err(|e| format!("Failed to set ProgID default value: {}", e))?; + .map_err(|e| format!("Failed to set ProgID default value: {e}"))?; prog_id_key .set_value("FriendlyTypeName", &"Donut Browser Document") - .map_err(|e| format!("Failed to set FriendlyTypeName: {}", e))?; + .map_err(|e| format!("Failed to set FriendlyTypeName: {e}"))?; + + // The shell reads this block to put a name and an icon beside the ProgId in + // the "Open with" list. Without it the entry shows as the raw ProgId. + let (application, _) = prog_id_key + .create_subkey("Application") + .map_err(|e| format!("Failed to create ProgID Application key: {e}"))?; + + application + .set_value("ApplicationName", &DISPLAY_NAME) + .map_err(|e| format!("Failed to set ProgID ApplicationName: {e}"))?; + + application + .set_value("ApplicationIcon", &format!("{},0", quoted(exe_path))) + .map_err(|e| format!("Failed to set ProgID ApplicationIcon: {e}"))?; - // Create DefaultIcon key let (icon_key, _) = prog_id_key .create_subkey("DefaultIcon") - .map_err(|e| format!("Failed to create DefaultIcon key: {}", e))?; + .map_err(|e| format!("Failed to create DefaultIcon key: {e}"))?; icon_key - .set_value("", &format!("\"{}\",0", exe_path)) - .map_err(|e| format!("Failed to set default icon: {}", e))?; + .set_value("", &format!("{},0", quoted(exe_path))) + .map_err(|e| format!("Failed to set default icon: {e}"))?; - // Create shell\open\command key let (command_key, _) = prog_id_key - .create_subkey("shell\\open\\command") - .map_err(|e| format!("Failed to create command key: {}", e))?; + .create_subkey(r"shell\open\command") + .map_err(|e| format!("Failed to create command key: {e}"))?; command_key - .set_value("", &format!("\"{}\" \"%1\"", exe_path)) - .map_err(|e| format!("Failed to set command: {}", e))?; + .set_value("", &format!(r#"{} "%1""#, quoted(exe_path))) + .map_err(|e| format!("Failed to set command: {e}"))?; Ok(()) } - fn set_default_for_scheme(scheme: &str) -> Result<(), String> { - let hkcu = RegKey::predef(HKEY_CURRENT_USER); + /// The `StartMenuInternet` entry: the shape the shell reads for a web + /// browser. A display name, an icon, the command that starts it, the + /// `InstallInfo` block the default-programs page expects, and the capability + /// lists that say which schemes and file types it handles. + fn register_client(root: &RegKey, exe_path: &str) -> Result<(), String> { + let (client, _) = root + .create_subkey(CLIENT_KEY) + .map_err(|e| format!("Failed to create browser client key: {e}"))?; - // Set in Software\Microsoft\Windows\CurrentVersion\Explorer\FileExts\.html\UserChoice - // Note: On Windows 10+, this might require elevated permissions or user interaction - // through the Settings app due to security restrictions + client + .set_value("", &DISPLAY_NAME) + .map_err(|e| format!("Failed to set client display name: {e}"))?; - // Try to set the association in the user's choice - let user_choice_path = format!( - "Software\\Microsoft\\Windows\\Shell\\Associations\\UrlAssociations\\{}\\UserChoice", - scheme - ); + let (icon, _) = client + .create_subkey("DefaultIcon") + .map_err(|e| format!("Failed to create client DefaultIcon key: {e}"))?; - // Note: Setting UserChoice directly may not work on Windows 10+ due to hash verification - // The user may need to manually set the default browser through Windows Settings - match hkcu.create_subkey(&user_choice_path) { - Ok((user_choice, _)) => { - // Attempt to set the ProgId - if user_choice.set_value("ProgId", &PROG_ID).is_err() { - // If we can't set UserChoice, that's expected on newer Windows versions - // The registration is still valuable for the "Open with" menu - } - } - Err(_) => { - // Expected on newer Windows versions - user must set manually - } + icon + .set_value("", &format!("{},0", quoted(exe_path))) + .map_err(|e| format!("Failed to set client icon: {e}"))?; + + let (command, _) = client + .create_subkey(r"shell\open\command") + .map_err(|e| format!("Failed to create client command key: {e}"))?; + + // No `%1` here. This entry is how the shell starts the browser with no + // document, for example from the Start menu. + command + .set_value("", "ed(exe_path)) + .map_err(|e| format!("Failed to set client command: {e}"))?; + + // The shell reads the icons-visible state from here, so the block has to + // exist. It also understands `ReinstallCommand`, `HideIconsCommand` and + // `ShowIconsCommand`, and Edge and Chrome advertise all three. Donut does + // not, because it does not act on `--make-default-browser`, `--hide-icons` + // or `--show-icons`. Advertising a command the program ignores is the same + // empty claim as registering a file type nothing can open. Add them here on + // the day the flags do something. + let (install_info, _) = client + .create_subkey("InstallInfo") + .map_err(|e| format!("Failed to create InstallInfo key: {e}"))?; + + install_info + .set_value("IconsVisible", &1u32) + .map_err(|e| format!("Failed to set IconsVisible: {e}"))?; + + let (capabilities, _) = client + .create_subkey("Capabilities") + .map_err(|e| format!("Failed to create Capabilities key: {e}"))?; + + // `ApplicationName` belongs inside `Capabilities`. The old code wrote it one + // level up, where the shell does not look, so the entry had no name. + capabilities + .set_value("ApplicationName", &DISPLAY_NAME) + .map_err(|e| format!("Failed to set ApplicationName: {e}"))?; + + capabilities + .set_value("ApplicationDescription", &DESCRIPTION) + .map_err(|e| format!("Failed to set ApplicationDescription: {e}"))?; + + capabilities + .set_value("ApplicationIcon", &format!("{},0", quoted(exe_path))) + .map_err(|e| format!("Failed to set ApplicationIcon: {e}"))?; + + let (url_assoc, _) = capabilities + .create_subkey("URLAssociations") + .map_err(|e| format!("Failed to create URLAssociations key: {e}"))?; + + for scheme in URL_SCHEMES { + url_assoc + .set_value(scheme, &PROG_ID) + .map_err(|e| format!("Failed to set {scheme} association: {e}"))?; + } + + let (file_assoc, _) = capabilities + .create_subkey("FileAssociations") + .map_err(|e| format!("Failed to create FileAssociations key: {e}"))?; + + for extension in FILE_EXTENSIONS { + file_assoc + .set_value(extension, &PROG_ID) + .map_err(|e| format!("Failed to set {extension} association: {e}"))?; } Ok(()) } - fn register_html_file_association(_exe_path: &str) -> Result<(), String> { - let hkcu = RegKey::predef(HKEY_CURRENT_USER); + /// Offer Donut in the "Open with" list for the HTML file types, without + /// taking the association away from whatever the user already chose. + /// + /// The old code wrote the ProgId into the default value of + /// `Software\Classes\.html`, which is the association itself. That replaced + /// the user's choice without asking, was never undone on uninstall, and did + /// not even take effect, because the per-user `FileExts` choice outranks it. + /// `OpenWithProgids` is the additive form: it adds Donut to the list and + /// displaces nothing. + fn register_file_extensions(root: &RegKey) -> Result<(), String> { + for extension in FILE_EXTENSIONS { + let (open_with, _) = root + .create_subkey(format!(r"Software\Classes\{extension}\OpenWithProgids")) + .map_err(|e| format!("Failed to create OpenWithProgids key for {extension}: {e}"))?; - // Register .html and .htm file associations - for ext in &[".html", ".htm"] { - let ext_path = format!("Software\\Classes\\{}", ext); + // Only the value name matters here. The payload is a marker. + open_with + .set_value(PROG_ID, &"") + .map_err(|e| format!("Failed to register the {extension} handler: {e}"))?; + } - match hkcu.create_subkey(&ext_path) { - Ok((ext_key, _)) => { - // Set the default value to our ProgID - let _ = ext_key.set_value("", &PROG_ID); - } - Err(_) => { - // Continue if we can't set the file association - } - } + Ok(()) + } + + /// Point `RegisteredApplications` at the capability data. This is what puts + /// Donut in the list Windows offers under "Default apps". + fn register_application(root: &RegKey) -> Result<(), String> { + let (registered_apps, _) = root + .create_subkey(r"Software\RegisteredApplications") + .map_err(|e| format!("Failed to create RegisteredApplications key: {e}"))?; + + registered_apps + .set_value(APP_NAME, &CAPABILITIES_KEY) + .map_err(|e| format!("Failed to set registered application: {e}")) + } + + /// Open the page where the user chooses the default browser. + /// + /// Windows does not let a program make itself the default. The value that + /// decides the handler, the `UserChoice` key under `UrlAssociations`, carries + /// a hash over the user's SID, the ProgId and a timestamp, and only the shell + /// can produce it. Windows 11 also ships UCPD.sys, which blocks writes to + /// those keys outright. + /// + /// The old code wrote `ProgId` there with no hash and discarded every error, + /// then reported success. The registry never changed, the Settings page went + /// on saying "Inactive", and the user was told nothing. Registration is the + /// part a program is allowed to do. The choice belongs to the user, so open + /// the page where they can make it and let the caller say so. + fn open_default_apps_settings() -> Result<(), String> { + use windows::core::{HSTRING, PCWSTR}; + use windows::Win32::System::Com::{ + CoInitializeEx, CoUninitialize, COINIT_APARTMENTTHREADED, COINIT_DISABLE_OLE1DDE, + }; + use windows::Win32::UI::Shell::ShellExecuteW; + use windows::Win32::UI::WindowsAndMessaging::SW_SHOWNORMAL; + + // `registeredAppUser` makes the page open on our entry rather than at the + // top of the list. It is the name just written into + // `RegisteredApplications`, so it only resolves because registration ran + // first. + let target = HSTRING::from(format!( + "ms-settings:defaultapps?registeredAppUser={APP_NAME}" + )); + let operation = HSTRING::from("open"); + + // ShellExecuteW hands the URI to a shell extension, and shell extensions + // are COM objects. This runs on a `spawn_blocking` thread, which has no + // apartment of its own, so give it one. An error means the thread already + // had an apartment in another mode, and in that case it is not ours to + // tear down. + let com_status = + unsafe { CoInitializeEx(None, COINIT_APARTMENTTHREADED | COINIT_DISABLE_OLE1DDE) }; + let owns_com = com_status.is_ok(); + + let result = unsafe { + ShellExecuteW( + None, + PCWSTR(operation.as_ptr()), + PCWSTR(target.as_ptr()), + PCWSTR::null(), + PCWSTR::null(), + SW_SHOWNORMAL, + ) + }; + + if owns_com { + unsafe { CoUninitialize() }; + } + + // ShellExecuteW reports success as a value above 32. Anything at or below + // that is an error code wearing a handle's type. + let code = result.0 as isize; + if code <= 32 { + return Err(format!( + "Donut Browser is registered, but Windows Settings did not open (code {code}). Open Settings, then Apps, then Default apps, find Donut Browser and set it for HTTP and HTTPS." + )); } Ok(()) @@ -399,6 +562,199 @@ mod windows { ); } } + + #[cfg(test)] + mod registration_tests { + use super::*; + + /// A scratch key that stands in for HKCU, so the test writes a real tree + /// through the real code without touching the tree Windows actually reads. + /// Deleted on the way out, including when an assertion fails. + struct ScratchRoot { + key: RegKey, + path: String, + } + + const SCRATCH_PARENT: &str = r"Software\DonutBrowserTests"; + + impl ScratchRoot { + fn new(name: &str) -> Self { + let hkcu = RegKey::predef(HKEY_CURRENT_USER); + let path = format!(r"{SCRATCH_PARENT}\{name}"); + let _ = hkcu.delete_subkey_all(&path); + let (key, _) = hkcu.create_subkey(&path).expect("create the scratch root"); + Self { key, path } + } + + fn value(&self, subkey: &str, name: &str) -> Option { + self + .key + .open_subkey(subkey) + .ok()? + .get_value::(name) + .ok() + } + } + + impl Drop for ScratchRoot { + fn drop(&mut self) { + let hkcu = RegKey::predef(HKEY_CURRENT_USER); + let _ = hkcu.delete_subkey_all(&self.path); + // Take the shared parent too, so a test run leaves nothing at all in + // the user's registry. `delete_subkey` refuses a key that still has + // children, which is exactly the guard needed while tests run in + // parallel: whoever finishes last removes it. + let _ = hkcu.delete_subkey(SCRATCH_PARENT); + } + } + + const EXE: &str = r"C:\Program Files\Donut Browser\donutbrowser.exe"; + + #[test] + fn registration_writes_the_shape_the_shell_reads() { + let root = ScratchRoot::new("registration"); + register_prog_id(&root.key, EXE).expect("register the ProgId"); + register_client(&root.key, EXE).expect("register the client"); + register_file_extensions(&root.key).expect("register the file types"); + register_application(&root.key).expect("register the application"); + + // The bug that made the button do nothing: this pointed at the + // application key instead of at its `Capabilities` subkey, so the shell + // read no capabilities and never offered Donut as a browser. Every other + // entry on a working machine ends in `Capabilities`. + let registered = root + .value(r"Software\RegisteredApplications", APP_NAME) + .expect("RegisteredApplications entry"); + assert_eq!(registered, CAPABILITIES_KEY); + assert!( + registered.ends_with(r"\Capabilities"), + "RegisteredApplications must name the Capabilities subkey, got {registered}" + ); + assert!( + root.key.open_subkey(®istered).is_ok(), + "RegisteredApplications names {registered}, which does not exist" + ); + + // The second bug: `ApplicationName` sat one level above `Capabilities`, + // where the shell does not look, so the entry had no name to show. + assert_eq!( + root.value(CAPABILITIES_KEY, "ApplicationName").as_deref(), + Some(DISPLAY_NAME) + ); + assert_eq!( + root + .value(CAPABILITIES_KEY, "ApplicationDescription") + .as_deref(), + Some(DESCRIPTION) + ); + + // Every scheme and file type the capability lists claim. + for scheme in URL_SCHEMES { + assert_eq!( + root + .value(&format!(r"{CAPABILITIES_KEY}\URLAssociations"), scheme) + .as_deref(), + Some(PROG_ID), + "{scheme} is not claimed" + ); + } + for extension in FILE_EXTENSIONS { + assert_eq!( + root + .value(&format!(r"{CAPABILITIES_KEY}\FileAssociations"), extension) + .as_deref(), + Some(PROG_ID), + "{extension} is not claimed" + ); + } + + // The rest of the StartMenuInternet entry. + assert_eq!(root.value(CLIENT_KEY, "").as_deref(), Some(DISPLAY_NAME)); + assert_eq!( + root.value(&format!(r"{CLIENT_KEY}\shell\open\command"), ""), + Some(quoted(EXE)) + ); + assert!(root + .key + .open_subkey(format!(r"{CLIENT_KEY}\InstallInfo")) + .is_ok()); + + // The ProgId command has to carry `%1`. Without it the shell starts the + // browser and never says which page to open. + let prog_id_command = root + .value( + &format!(r"Software\Classes\{PROG_ID}\shell\open\command"), + "", + ) + .expect("ProgId command"); + assert_eq!(prog_id_command, format!(r#"{} "%1""#, quoted(EXE))); + + // The file types are offered, not seized. Taking the default value of + // `Software\Classes\.html` is what the old code did, and that value + // belongs to whatever the user chose. + for extension in FILE_EXTENSIONS { + assert_eq!( + root + .value( + &format!(r"Software\Classes\{extension}\OpenWithProgids"), + PROG_ID + ) + .as_deref(), + Some(""), + "{extension} should offer the handler" + ); + assert!( + root + .value(&format!(r"Software\Classes\{extension}"), "") + .is_none(), + "{extension} default value must be left alone" + ); + } + } + + #[test] + fn the_association_an_older_build_took_is_given_back() { + let root = ScratchRoot::new("legacy"); + + // Recreate what the old code left behind: its own application key, and + // the ProgId written straight into the association for one file type. + let (legacy, _) = root + .key + .create_subkey(format!(r"{LEGACY_APP_KEY}\Capabilities\URLAssociations")) + .expect("legacy key"); + legacy.set_value("http", &PROG_ID).expect("legacy claim"); + + let (html, _) = root + .key + .create_subkey(r"Software\Classes\.html") + .expect("html class"); + html.set_value("", &PROG_ID).expect("legacy association"); + + // A file type the user pointed somewhere else. This one is not ours and + // must survive untouched. + let (htm, _) = root + .key + .create_subkey(r"Software\Classes\.htm") + .expect("htm class"); + htm.set_value("", &"ChromeHTML").expect("user association"); + + remove_legacy_registration(&root.key); + + assert!( + root.key.open_subkey(LEGACY_APP_KEY).is_err(), + "the superseded application key should be gone" + ); + assert!( + root.value(r"Software\Classes\.html", "").is_none(), + "the association we took should have been released" + ); + assert_eq!( + root.value(r"Software\Classes\.htm", "").as_deref(), + Some("ChromeHTML"), + "a choice that is not ours must not be touched" + ); + } + } } #[cfg(target_os = "linux")] @@ -551,7 +907,105 @@ pub async fn is_default_browser() -> Result { } #[command] -pub async fn set_as_default_browser() -> Result<(), String> { +pub async fn set_as_default_browser() -> Result { let default_browser = DefaultBrowser::instance(); default_browser.set_as_default_browser().await } + +#[cfg(test)] +mod tests { + /// The type system now prevents the mistake behind the crash on Windows. + /// `SendMessageTimeoutW` comes from the `windows` crate, and its + /// out-parameter is typed `Option<*mut usize>`, so a four byte slot no longer + /// compiles. That guarantee holds only while the call goes through the crate. + /// A hand-written declaration would bring back the whole class of bug in a + /// form no compiler and no lint can see, so refuse one here. + /// + /// This looks at the Windows module on every platform, because the module is + /// compiled out everywhere else and would otherwise go unchecked on the + /// runners that do most of the work. + #[test] + fn the_windows_module_declares_no_foreign_functions_by_hand() { + const SOURCE: &str = include_str!("default_browser.rs"); + + let start = SOURCE + .find("mod windows {") + .expect("the Windows module was renamed; update this guard"); + let end = SOURCE + .find("mod linux {") + .expect("the Linux module was renamed; update this guard"); + assert!( + start < end, + "the module order changed; update this guard so it still reads the Windows module" + ); + + assert!( + !SOURCE[start..end].contains(r#"extern ""#), + "The Windows module declares a foreign function by hand. Do not. A \ + hand-written declaration of SendMessageTimeoutA, with its out-parameter \ + typed *mut u32 instead of the real PDWORD_PTR, is what made Windows \ + write four bytes past a stack slot and kill the process every time a \ + user set Donut as their default browser. Take the binding from the \ + `windows` crate, which cannot drift from the real ABI, and add the \ + feature it needs to Cargo.toml." + ); + } + + /// Show why the out-parameter has to be pointer sized. + /// + /// This does not try to reproduce the crash. Whether the four byte overrun is + /// fatal depends on the frame the optimiser happens to build, so a crash test + /// passes under one profile and fails under another. It measures the thing + /// that is always true instead: the call writes eight bytes. + #[cfg(target_os = "windows")] + #[test] + fn send_message_timeout_writes_a_pointer_sized_result() { + use windows::Win32::Foundation::{HWND, LPARAM, WPARAM}; + use windows::Win32::UI::WindowsAndMessaging::{SendMessageTimeoutW, SMTO_ABORTIFHUNG, WM_NULL}; + + /// A four byte slot with a marker behind it, laid out the way the old code + /// laid out its `u32`. Eight bytes in total and eight byte aligned, so a + /// pointer sized write lands entirely inside the struct. Nothing outside it + /// is touched and the test is not itself undefined behaviour. + #[repr(C, align(8))] + struct Probe { + result: u32, + canary: u32, + } + + const SENTINEL: u32 = 0xDEAD_BEEF; + + let mut probe = Probe { + result: SENTINEL, + canary: SENTINEL, + }; + + // The window handle is deliberately not a window. USER32 clears the + // out-parameter before it looks at the target, so this measures the write + // width without creating a window, without a message loop and without + // sending anything to another process. The test is hermetic. + unsafe { + SendMessageTimeoutW( + HWND(0xDEAD_0000_usize as *mut core::ffi::c_void), + WM_NULL, + WPARAM(0), + LPARAM(0), + SMTO_ABORTIFHUNG, + 50, + Some(&mut probe as *mut Probe as *mut usize), + ); + } + + assert_eq!( + probe.result, 0, + "SendMessageTimeoutW did not write the out-parameter at all, so this test \ + no longer measures anything. Check the call before trusting it." + ); + assert_ne!( + probe.canary, SENTINEL, + "SendMessageTimeoutW wrote only four bytes. If Windows has really narrowed \ + lpdwResult to a DWORD then notify_system_of_changes may use a u32. Until \ + then the out-parameter stays pointer sized." + ); + } +} diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index a4f8a4e..fe1a45e 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -1745,6 +1745,44 @@ fn setup_system_tray(app: &tauri::AppHandle) -> Result<(), Box(args: impl IntoIterator) -> Vec { + args + .into_iter() + .skip(1) + .filter_map(|arg| { + if arg.starts_with("http://") || arg.starts_with("https://") { + return Some(arg.clone()); + } + + let path = std::path::Path::new(arg); + if !path.is_file() { + return None; + } + + let absolute = if path.is_absolute() { + path.to_path_buf() + } else { + env::current_dir().ok()?.join(path) + }; + + url::Url::from_file_path(absolute) + .ok() + .map(|url| url.to_string()) + }) + .collect() +} + #[cfg_attr(mobile, tauri::mobile_entry_point)] pub fn run() { run_with_builder(|builder| builder); @@ -1755,7 +1793,7 @@ pub fn run_with_builder( configure_builder: impl FnOnce(tauri::Builder) -> tauri::Builder, ) { let args: Vec = env::args().collect(); - let startup_url = args.iter().find(|arg| arg.starts_with("http")).cloned(); + let startup_url = urls_from_args(args.iter()).into_iter().next(); if let Some(url) = startup_url.clone() { log::info!("Found startup URL in command line"); @@ -1819,6 +1857,20 @@ pub fn run_with_builder( let _ = window.set_focus(); let _ = window.unminimize(); } + + // A second launch is how the desktop hands a running browser its next + // link. The shell starts the executable with the target in argv, this + // callback receives that argv, and the second process exits. The callback + // used to log the arguments and drop them, so clicking a link did nothing + // whenever Donut was already open, which is every time after the first. + for url in urls_from_args(args.iter()) { + let handle = app_handle.clone(); + tauri::async_runtime::spawn(async move { + if let Err(e) = handle_url_open(handle, url).await { + log::error!("Failed to handle a forwarded URL: {e}"); + } + }); + } }, )); @@ -2620,51 +2672,7 @@ pub fn run_with_builder( // Start sync subscription and scheduler if configured let app_handle_sync = app.handle().clone(); tauri::async_runtime::spawn(async move { - use std::sync::Arc; - - let mut subscription_manager = sync::SubscriptionManager::new(); - let work_rx = subscription_manager.take_work_receiver(); - - if let Err(e) = subscription_manager.start(app_handle_sync.clone()).await { - log::warn!("Failed to start sync subscription: {e}"); - } - - if let Some(work_rx) = work_rx { - let scheduler = Arc::new(sync::SyncScheduler::new()); - - // Set the global scheduler so commands can access it - sync::set_global_scheduler(scheduler.clone()); - - // Start initial sync for all enabled profiles - scheduler.sync_all_enabled_profiles(&app_handle_sync).await; - - // Check for missing synced profiles (deleted locally but exist remotely) - match sync::SyncEngine::create_from_settings(&app_handle_sync).await { - Ok(engine) => { - if let Err(e) = engine - .check_for_missing_synced_profiles(&app_handle_sync) - .await - { - log::warn!("Failed to check for missing profiles: {}", e); - } - if let Err(e) = engine - .check_for_missing_synced_entities(&app_handle_sync) - .await - { - log::warn!("Failed to check for missing entities: {}", e); - } - } - Err(e) => { - log::warn!("Sync not configured, skipping missing profile check: {}", e); - } - } - - scheduler - .clone() - .start(app_handle_sync.clone(), work_rx) - .await; - log::info!("Sync scheduler started"); - } + sync::start_pipeline(app_handle_sync).await; }); // Start cloud auth background refresh loop @@ -2965,6 +2973,57 @@ pub fn run_with_builder( mod tests { use std::fs; + #[test] + fn a_command_line_yields_the_links_and_files_it_carries() { + let exe = "C:/Program Files/Donut Browser/donutbrowser.exe".to_string(); + + // The executable's own path leads every command line and is not a target, + // even on a machine where that path happens to exist. + assert!(super::urls_from_args([&exe]).is_empty()); + + let link = "https://example.com/a?b=c".to_string(); + let insecure = "http://example.com".to_string(); + assert_eq!( + super::urls_from_args([&exe, &link, &insecure]), + vec![link.clone(), insecure] + ); + + // Flags and stray words are not links. The old filter took anything + // starting with "http", which is looser than it looks. + let flag = "--headless".to_string(); + let near_miss = "httpsomething".to_string(); + assert_eq!( + super::urls_from_args([&exe, &flag, &near_miss]), + Vec::::new() + ); + + // A path that is not there is ignored rather than guessed at. + let missing = "C:/no/such/page.html".to_string(); + assert!(super::urls_from_args([&exe, &missing]).is_empty()); + + // Explorer hands a browser a bare path, not a URL, because the registered + // command passes `%1` straight through. Turning it into a `file://` URL + // here is what makes the .html association in `default_browser.rs` a real + // claim rather than an empty one. + let directory = tempfile::tempdir().expect("temp dir"); + let page = directory.path().join("page.html"); + fs::write(&page, "").expect("write the page"); + let page_arg = page.to_string_lossy().to_string(); + + let found = super::urls_from_args([&exe, &page_arg]); + assert_eq!(found.len(), 1, "the file should have produced one URL"); + assert!( + found[0].starts_with("file:///"), + "expected a file URL, got {}", + found[0] + ); + assert!( + found[0].ends_with("page.html"), + "expected the page's own name, got {}", + found[0] + ); + } + #[test] fn backend_error_helpers_preserve_codes_and_structure_diagnostics() { let coded = super::backend_error("PROFILE_NOT_FOUND"); diff --git a/src-tauri/src/sync/client.rs b/src-tauri/src/sync/client.rs index 954355f..a7fe162 100644 --- a/src-tauri/src/sync/client.rs +++ b/src-tauri/src/sync/client.rs @@ -1,5 +1,27 @@ use super::types::*; use reqwest::Client; +use std::time::Duration; + +/// How long to wait for a storage host to accept a connection. +/// +/// This client had no timeouts at all. A host that neither accepts nor refuses, +/// which is what a dropping firewall or a black-holed address looks like, held +/// every attempt for the operating system's own connect backoff: measured at +/// 21 s on Windows and 134 s on Linux. With `MAX_FILE_RETRIES` and its backoff +/// that is minutes for one file, and a profile of two hundred files reports +/// nothing for most of an hour. +/// +/// Matches the pre-flight probe, so a host that fails the check fails a +/// transfer the same way and in the same time. +const CONNECT_TIMEOUT: Duration = Duration::from_secs(8); + +/// How long a transfer may make no progress at all. +/// +/// Deliberately an inactivity timeout and not a deadline on the whole request. +/// Profile files run to tens of megabytes and a slow link is not a broken one, +/// so a total timeout would start failing syncs that were working. This fires +/// only when nothing arrives for a full minute. +const READ_TIMEOUT: Duration = Duration::from_secs(60); #[derive(Clone)] pub struct SyncClient { @@ -11,7 +33,15 @@ pub struct SyncClient { impl SyncClient { pub fn new(base_url: String, token: String) -> Self { Self { - client: Client::new(), + client: Client::builder() + .connect_timeout(CONNECT_TIMEOUT) + .read_timeout(READ_TIMEOUT) + .build() + // A builder failure here means the TLS backend did not start. The + // default client cannot transfer either, so fall back and let the first + // real request report it, rather than making this constructor fallible + // for a condition no caller can act on. + .unwrap_or_default(), base_url: base_url.trim_end_matches('/').to_string(), token, } @@ -235,14 +265,13 @@ impl SyncClient { } // The storage host here comes from the presigned URL, so on a self-hosted - // server it is whatever the server signed against — frequently an address + // server it is whatever the server signed against, frequently an address // only the server can resolve. `reqwest`'s own Display collapses that to // "error sending request", which is why this failure used to be - // undiagnosable; report the innermost cause instead. - let response = req - .send() - .await - .map_err(|e| SyncError::NetworkError(super::preflight::transport_reason(&e)))?; + // undiagnosable; report the innermost cause and the host it names. + let response = req.send().await.map_err(|e| { + SyncError::NetworkError(super::preflight::transport_reason_for(presigned_url, &e)) + })?; if !response.status().is_success() { let status = response.status(); @@ -256,12 +285,9 @@ impl SyncClient { } pub async fn download_bytes(&self, presigned_url: &str) -> SyncResult> { - let response = self - .client - .get(presigned_url) - .send() - .await - .map_err(|e| SyncError::NetworkError(super::preflight::transport_reason(&e)))?; + let response = self.client.get(presigned_url).send().await.map_err(|e| { + SyncError::NetworkError(super::preflight::transport_reason_for(presigned_url, &e)) + })?; if !response.status().is_success() { return Err(SyncError::NetworkError(format!( diff --git a/src-tauri/src/sync/engine.rs b/src-tauri/src/sync/engine.rs index ca3a889..4a0fb41 100644 --- a/src-tauri/src/sync/engine.rs +++ b/src-tauri/src/sync/engine.rs @@ -146,9 +146,14 @@ fn critical_failure_message(action: &str, failures: &[(String, String)]) -> Stri /// Transfers go straight to the storage host named in the presigned URL, not /// through the sync server, so a self-hosted server that signs URLs against an /// address only it can resolve fails every file here while its own `/health` -/// and `/readyz` stay green. The cause string already carries the host; without -/// this line it still reads as an unexplained network fault, and the setting -/// that fixes it lives on the server, where the user is not looking. +/// and `/readyz` stay green. The cause string names the host, which says which +/// address is wrong; this line says where to change it, because the setting +/// lives on the server, where the user is not looking. +/// +/// The host only started appearing in that string when the transfer path moved +/// to `transport_reason_for`. Before that this comment claimed a host that was +/// never there, and every report of this bug arrived with a list of file names +/// and nothing to act on. fn storage_endpoint_hint(cause: &str) -> String { let lowered = cause.to_ascii_lowercase(); let is_transport_failure = [ @@ -4321,6 +4326,106 @@ pub async fn rollover_encryption_for_all_entities( mod tests { use super::*; + /// The whole of issue 534, at the only place the user ever sees it. + /// + /// A self-hosted server signs every presigned URL against the address it uses + /// for storage itself. In the documented compose file that is a Docker + /// service name, so the server is healthy, `/health` and `/readyz` are green, + /// and the client cannot open a single one of the URLs it is handed. The + /// reporters got a list of file names, no host and no setting, and there was + /// nothing in it to act on. + /// + /// The message has to carry three things: which files, which host refused + /// them, and which setting fixes it. + #[test] + fn a_transfer_failure_names_the_host_and_the_setting_that_fixes_it() { + // Exactly the text the transfer path now produces. The trailing host comes + // from `preflight::transport_reason_for`, which the two transfer call sites + // in `client.rs` use. + let cause = "connection failed: No such host is known. (os error 11001) \ + (storage host minio:9000)"; + let failures = vec![ + ("profile/Default/Cookies".to_string(), cause.to_string()), + ("profile/Default/Login Data".to_string(), cause.to_string()), + ("profile/Local State".to_string(), cause.to_string()), + ]; + + let message = critical_failure_message("upload", &failures); + + assert!( + message.contains("minio:9000"), + "the reader has to learn which host refused the transfer: {message}" + ); + assert!( + message.contains("S3_PUBLIC_ENDPOINT"), + "the setting that fixes it lives on the server, so the message has to \ + name it: {message}" + ); + assert!( + message.contains("profile/Default/Cookies"), + "the affected files still belong in the message: {message}" + ); + } + + /// The same guarantee, but driven through the real transfer path instead of a + /// hand-written cause string. + /// + /// The test above pins the message builder. This one pins the join: that an + /// upload which cannot reach its host actually produces a cause carrying that + /// host. Dropping back to a reason that omits the host, which is how this + /// shipped for months, breaks this test and not the one above. + /// + /// No server is involved. `.invalid` never resolves (RFC 2606), so the + /// failure is the real one, offline and deterministic. + #[tokio::test] + async fn an_unreachable_storage_host_survives_the_whole_way_to_the_message() { + let client = SyncClient::new("http://127.0.0.1:1".to_string(), "unused".to_string()); + let presigned = "http://donut-storage.invalid:9000/bucket/profiles/p1/Cookies\ + ?X-Amz-Signature=deadbeef"; + + let error = client + .upload_bytes(presigned, b"payload", None) + .await + .expect_err("a host that cannot resolve must not report a successful upload"); + + let message = critical_failure_message( + "upload", + &[("profile/Default/Cookies".to_string(), error.to_string())], + ); + + assert!( + message.contains("donut-storage.invalid:9000"), + "the host has to survive from the transfer to the message: {message}" + ); + assert!( + message.contains("S3_PUBLIC_ENDPOINT"), + "an unreachable storage host has one fix, and it is on the server: {message}" + ); + assert!( + !message.contains("X-Amz-Signature"), + "the signature must never reach the message: {message}" + ); + } + + /// The hint is for a transfer that never connected. A server that answered + /// and refused is a different problem with a different fix, and pointing that + /// user at their storage endpoint would send them the wrong way. + #[test] + fn a_rejected_transfer_is_not_blamed_on_the_storage_endpoint() { + let failures = vec![( + "profile/Default/Cookies".to_string(), + "Upload failed with status 403 Forbidden: SignatureDoesNotMatch".to_string(), + )]; + + let message = critical_failure_message("upload", &failures); + + assert!(message.contains("SignatureDoesNotMatch"), "{message}"); + assert!( + !message.contains("S3_PUBLIC_ENDPOINT"), + "a 403 is not an unreachable host: {message}" + ); + } + #[test] fn test_critical_failure_message_carries_the_cause() { // A self-hosted server that hands out unreachable presigned URLs fails diff --git a/src-tauri/src/sync/mod.rs b/src-tauri/src/sync/mod.rs index b8482af..171d384 100644 --- a/src-tauri/src/sync/mod.rs +++ b/src-tauri/src/sync/mod.rs @@ -31,6 +31,99 @@ pub use scheduler::{get_global_scheduler, set_global_scheduler, SyncScheduler}; pub use subscription::{SubscriptionManager, SyncWorkItem}; pub use types::{SyncError, SyncResult}; +/// The live subscription, held so it can be stopped. +/// +/// It used to be a local inside whichever task built the pipeline. Dropping a +/// `SubscriptionManager` does not end its work: `SyncSubscription::start` +/// spawns a task holding clones of the running flag and the work sender, so the +/// task outlived the handle and nothing could reach it. Every restart added one +/// more live SSE connection, each with its own poll loop on the server, and +/// disconnecting left an authenticated stream open to a server the user had +/// just removed. +static GLOBAL_SUBSCRIPTION: std::sync::Mutex> = + std::sync::Mutex::new(None); + +/// Held for the whole of `start_pipeline`, so only one pipeline is ever being +/// assembled at a time. +static PIPELINE_LOCK: tokio::sync::Mutex<()> = tokio::sync::Mutex::const_new(()); + +/// Retire the running pipeline, both halves of it. +pub fn stop_pipeline() { + if let Some(scheduler) = get_global_scheduler() { + scheduler.stop(); + } + if let Ok(mut guard) = GLOBAL_SUBSCRIPTION.lock() { + if let Some(subscription) = guard.as_mut() { + subscription.stop(); + } + *guard = None; + } +} + +/// Build and start the sync pipeline. Safe to call again to restart it. +/// +/// Startup and `restart_sync_service` each held their own copy of this, and the +/// copies had drifted. The restart copy stopped the old scheduler first and then +/// returned early if the subscription failed to start, so it left the +/// application holding a scheduler whose task had already exited. Everything +/// queued afterwards went into `pending_profiles` and was never drained, and +/// sync was silently dead until the app was restarted. One function cannot +/// drift from itself. +pub async fn start_pipeline(app_handle: tauri::AppHandle) { + // Two restarts arriving together would otherwise interleave: the second + // retires what the first has not published yet, then both start, and one + // scheduler is left ticking with nothing able to reach it. Building the + // pipeline is rare and already awaits the network, so serialising it costs + // nothing worth measuring. + let _building = PIPELINE_LOCK.lock().await; + + stop_pipeline(); + + let mut subscription_manager = SubscriptionManager::new(); + let Some(work_rx) = subscription_manager.take_work_receiver() else { + log::error!("Sync pipeline has no work receiver; not starting"); + return; + }; + + // A subscription failure costs live updates from other devices. It does not + // stop this device syncing its own changes on the timer, so carry on. The + // restart path used to give up here, which turned a token hiccup into sync + // being dead until the next launch. + if let Err(e) = subscription_manager.start(app_handle.clone()).await { + log::warn!("Failed to start sync subscription, continuing without live updates: {e}"); + } + if let Ok(mut guard) = GLOBAL_SUBSCRIPTION.lock() { + *guard = Some(subscription_manager); + } + + let scheduler = std::sync::Arc::new(SyncScheduler::new()); + // Published before the loop starts, because the checks below await the + // network and anything queued in the meantime has to land in this scheduler. + // `stop()` marks it cancelled, so a restart arriving during that window still + // retires it and `start` below becomes a no-op. + set_global_scheduler(scheduler.clone()); + + scheduler.sync_all_enabled_profiles(&app_handle).await; + + match SyncEngine::create_from_settings(&app_handle).await { + Ok(engine) => { + if let Err(e) = engine.check_for_missing_synced_profiles(&app_handle).await { + log::warn!("Failed to check for missing profiles: {e}"); + } + if let Err(e) = engine.check_for_missing_synced_entities(&app_handle).await { + log::warn!("Failed to check for missing entities: {e}"); + } + } + Err(e) => { + log::warn!("Sync not configured, skipping missing profile check: {e}"); + } + } + + if scheduler.clone().start(app_handle, work_rx).await { + log::info!("Sync scheduler started"); + } +} + /// Queue a profile sync if the profile has sync enabled. No-op otherwise. /// /// Called from profile metadata update paths so a rename / tag edit / proxy diff --git a/src-tauri/src/sync/preflight.rs b/src-tauri/src/sync/preflight.rs index 48cb64a..b8c076a 100644 --- a/src-tauri/src/sync/preflight.rs +++ b/src-tauri/src/sync/preflight.rs @@ -173,6 +173,38 @@ pub(crate) fn transport_reason(error: &reqwest::Error) -> String { } } +/// The same reason, naming the host that would not answer. +/// +/// A transfer goes straight to the host inside the presigned URL, and that host +/// is chosen by the server, not by this device. It is therefore the one fact the +/// user has never seen and the only one that points at the fix. Leaving it out +/// is what produced reports of "connection failed" with nothing to act on. +/// +/// `reqwest::Error::url()` is empty for connect-stage failures, which are +/// exactly the ones that matter here, so take the host from the URL the caller +/// already holds. +pub(crate) fn transport_reason_for(url: &str, error: &reqwest::Error) -> String { + let reason = transport_reason(error); + match storage_host(url) { + Some(host) => format!("{reason} (storage host {host})"), + None => reason, + } +} + +/// Host and port, and nothing else. +/// +/// A presigned URL carries the signature and the object key in its query, and +/// this string reaches log files and toasts. Only the authority is safe to +/// repeat, and it is the whole of what the reader needs. +fn storage_host(url: &str) -> Option { + let parsed = url::Url::parse(url).ok()?; + let host = parsed.host_str()?; + match parsed.port() { + Some(port) => Some(format!("{host}:{port}")), + None => Some(host.to_string()), + } +} + /// Pre-flight a sync server before saving it, and before trusting it to sync. #[tauri::command] pub async fn check_sync_server_connection(server_url: String) -> Result { @@ -260,4 +292,43 @@ mod tests { // The bare reqwest Display is what this exists to avoid. assert_ne!(error, "error sending request"); } + + #[test] + fn a_presigned_url_yields_only_its_authority() { + // The query carries the signature and the key. Neither may reach a log. + let signed = "http://minio:9000/donut/profiles/p1/profile/Default/Cookies\ + ?X-Amz-Signature=deadbeef&X-Amz-Credential=minioadmin"; + assert_eq!(storage_host(signed).as_deref(), Some("minio:9000")); + + assert_eq!( + storage_host("https://storage.example.com/bucket/key").as_deref(), + Some("storage.example.com") + ); + assert_eq!(storage_host("not a url").as_deref(), None); + } + + #[tokio::test] + async fn a_failed_transfer_names_the_host_that_refused_it() { + // The whole point of the message. Issue 534 reporters saw a list of file + // names and a bare "connection failed", and could not tell that the host + // their server had signed into every URL was one only the server could + // resolve. + let url = "http://minio.invalid:9000/donut/profiles/p1/Cookies?X-Amz-Signature=abc"; + let error = probe_client() + .put(url) + .body(b"payload".to_vec()) + .send() + .await + .expect_err("an unresolvable host must not succeed"); + + let message = transport_reason_for(url, &error); + assert!( + message.contains("minio.invalid:9000"), + "the failure has to name the storage host, got: {message}" + ); + assert!( + !message.contains("X-Amz-Signature"), + "the signature must never reach the message, got: {message}" + ); + } } diff --git a/src-tauri/src/sync/scheduler.rs b/src-tauri/src/sync/scheduler.rs index a001438..1a38ed1 100644 --- a/src-tauri/src/sync/scheduler.rs +++ b/src-tauri/src/sync/scheduler.rs @@ -8,7 +8,6 @@ use std::sync::Arc; use std::time::{Duration, Instant}; use tokio::sync::mpsc; use tokio::sync::Mutex; -use tokio::time::sleep; static GLOBAL_SCHEDULER: std::sync::Mutex>> = std::sync::Mutex::new(None); @@ -22,6 +21,17 @@ pub fn set_global_scheduler(scheduler: Arc) { } } +/// What `start` should do, given the flags. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum StartDecision { + /// Nothing is running and nothing retired it. Spawn the loop. + Start, + /// A loop is already ticking on this scheduler. + AlreadyRunning, + /// `stop` was called on it, possibly before it ever ran. + Retired, +} + #[derive(Debug, Clone)] struct ProfileStopTime { #[allow(dead_code)] @@ -31,6 +41,15 @@ struct ProfileStopTime { pub struct SyncScheduler { running: Arc, + /// Set by `stop()` and never cleared. A scheduler is one-shot. + /// + /// The pipeline publishes a scheduler before it starts its loop, because work + /// queued during the network checks in between has to land somewhere. That + /// left a window where `stop()` cleared a `running` flag that was still + /// false, so it did nothing, and the scheduler then started anyway and ticked + /// forever with no way to reach it. `running` cannot express "retired before + /// it ever ran", so this does. + cancelled: Arc, pending_profiles: Arc>>, pending_proxies: Arc>>, pending_groups: Arc>>, @@ -52,6 +71,7 @@ impl SyncScheduler { pub fn new() -> Self { Self { running: Arc::new(AtomicBool::new(false)), + cancelled: Arc::new(AtomicBool::new(false)), pending_profiles: Arc::new(Mutex::new(HashMap::new())), pending_proxies: Arc::new(Mutex::new(HashSet::new())), pending_groups: Arc::new(Mutex::new(HashSet::new())), @@ -68,7 +88,12 @@ impl SyncScheduler { self.running.load(Ordering::SeqCst) } + /// Retire this scheduler for good. + /// + /// Order matters: mark it cancelled before clearing `running`, so a `start()` + /// racing this call cannot slip between the two and begin ticking. pub fn stop(&self) { + self.cancelled.store(true, Ordering::SeqCst); self.running.store(false, Ordering::SeqCst); } @@ -334,35 +359,93 @@ impl SyncScheduler { } } + /// The decision `start` makes before it spawns anything. + /// + /// Split out so it can be tested. `start` needs a `tauri::AppHandle`, which a + /// unit test cannot build, and the retirement rule is the part worth pinning + /// down. The `running` check stays a `swap` so two concurrent starts cannot + /// both win. + fn claim_start_slot(&self) -> StartDecision { + if self.cancelled.load(Ordering::SeqCst) { + return StartDecision::Retired; + } + if self.running.swap(true, Ordering::SeqCst) { + return StartDecision::AlreadyRunning; + } + StartDecision::Start + } + + /// Begin ticking. Returns whether a loop was actually started, so the caller + /// can log the truth instead of assuming. pub async fn start( self: Arc, app_handle: tauri::AppHandle, mut work_rx: mpsc::UnboundedReceiver, - ) { - if self.running.swap(true, Ordering::SeqCst) { - return; + ) -> bool { + match self.claim_start_slot() { + StartDecision::Retired => { + // Retired while the pipeline was still assembling it. Starting now + // would leave a task nothing can stop, because the handle in the global + // has already been replaced. + log::info!("Sync scheduler was retired before it started; not starting it"); + return false; + } + StartDecision::AlreadyRunning => { + log::warn!("Sync scheduler is already running; ignoring the second start"); + return false; + } + StartDecision::Start => {} } let scheduler = self.clone(); let app_handle_clone = app_handle.clone(); tokio::spawn(async move { + // A fresh `sleep` inside the `select!` restarts from zero on every + // iteration, so a steady stream of work items kept resetting it and + // `process_pending` never ran: queued profiles sat there for as long as + // the stream lasted. An interval keeps its own schedule regardless of how + // often the other arm fires. `Delay` rather than `Burst` so a slow + // `process_pending` does not come back to a pile of missed ticks and run + // itself back to back. + let mut ticker = tokio::time::interval(Duration::from_millis(2000)); + ticker.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Delay); + // The first tick of an interval resolves immediately. The old shape + // always waited 2000 ms before its first pass, so consume it here and + // keep that behaviour. + ticker.tick().await; + + // Once the senders are gone `recv()` resolves instantly and forever, so + // the arm has to be disabled or the loop spins hot on a dead channel. + let mut work_channel_open = true; + while scheduler.running.load(Ordering::SeqCst) { tokio::select! { - Some(work_item) = work_rx.recv() => { - match work_item { - SyncWorkItem::Profile(id) => scheduler.queue_profile_sync(id).await, - SyncWorkItem::Proxy(id) => scheduler.queue_proxy_sync(id).await, - SyncWorkItem::Group(id) => scheduler.queue_group_sync(id).await, - SyncWorkItem::Vpn(id) => scheduler.queue_vpn_sync(id).await, - SyncWorkItem::Extension(id) => scheduler.queue_extension_sync(id).await, - SyncWorkItem::ExtensionGroup(id) => scheduler.queue_extension_group_sync(id).await, - SyncWorkItem::Tombstone(entity_type, entity_id) => { - scheduler.queue_tombstone(entity_type, entity_id).await + received = work_rx.recv(), if work_channel_open => { + match received { + Some(work_item) => match work_item { + SyncWorkItem::Profile(id) => scheduler.queue_profile_sync(id).await, + SyncWorkItem::Proxy(id) => scheduler.queue_proxy_sync(id).await, + SyncWorkItem::Group(id) => scheduler.queue_group_sync(id).await, + SyncWorkItem::Vpn(id) => scheduler.queue_vpn_sync(id).await, + SyncWorkItem::Extension(id) => scheduler.queue_extension_sync(id).await, + SyncWorkItem::ExtensionGroup(id) => scheduler.queue_extension_group_sync(id).await, + SyncWorkItem::Tombstone(entity_type, entity_id) => { + scheduler.queue_tombstone(entity_type, entity_id).await + } + }, + None => { + // The subscription is gone, so no more live updates from other + // devices. Local changes and the timer still work, so keep + // ticking rather than ending the scheduler. + log::warn!( + "Sync work channel closed; continuing on the timer without live updates" + ); + work_channel_open = false; } } } - _ = sleep(Duration::from_millis(2000)) => { + _ = ticker.tick() => { scheduler.process_pending(&app_handle_clone).await; } } @@ -370,6 +453,8 @@ impl SyncScheduler { log::info!("Sync scheduler stopped"); }); + + true } async fn process_pending(&self, app_handle: &tauri::AppHandle) { @@ -853,3 +938,56 @@ impl SyncScheduler { } } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn a_fresh_scheduler_starts_once() { + let scheduler = SyncScheduler::new(); + assert_eq!(scheduler.claim_start_slot(), StartDecision::Start); + assert!(scheduler.is_running()); + assert_eq!( + scheduler.claim_start_slot(), + StartDecision::AlreadyRunning, + "a second start must not spawn a second loop on the same scheduler" + ); + } + + #[test] + fn a_scheduler_retired_before_it_ran_never_starts() { + // The pipeline publishes a scheduler, then awaits two network checks, then + // starts the loop. A restart landing in that window calls `stop()` on a + // scheduler that has not started yet. `running` was already false, so the + // old `stop()` did nothing at all, the loop started afterwards, and it + // ticked forever with the global already pointing elsewhere. + let scheduler = SyncScheduler::new(); + assert!(!scheduler.is_running()); + + scheduler.stop(); + + assert_eq!( + scheduler.claim_start_slot(), + StartDecision::Retired, + "a scheduler stopped before starting must stay stopped" + ); + assert!( + !scheduler.is_running(), + "refusing to start must not leave the running flag set" + ); + } + + #[test] + fn stopping_a_running_scheduler_retires_it_for_good() { + let scheduler = SyncScheduler::new(); + assert_eq!(scheduler.claim_start_slot(), StartDecision::Start); + + scheduler.stop(); + assert!(!scheduler.is_running()); + + // A scheduler is one-shot. Restarting the pipeline builds a new one, so a + // retired instance coming back to life could only ever be a duplicate. + assert_eq!(scheduler.claim_start_slot(), StartDecision::Retired); + } +} diff --git a/src-tauri/tauri.conf.json b/src-tauri/tauri.conf.json index edaca05..62cf6b1 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": "Donut", - "version": "0.29.6", + "version": "0.30.0", "identifier": "com.donutbrowser", "build": { "beforeDevCommand": "pnpm copy-proxy-binary && pnpm dev", diff --git a/src/components/settings-dialog.tsx b/src/components/settings-dialog.tsx index 60ec2c2..436114a 100644 --- a/src/components/settings-dialog.tsx +++ b/src/components/settings-dialog.tsx @@ -62,6 +62,7 @@ import { } from "@/lib/themes"; import { showErrorToast, showSuccessToast } from "@/lib/toast-utils"; import { cn } from "@/lib/utils"; +import type { SetDefaultBrowserOutcome } from "@/types"; import { RippleButton } from "./ui/ripple"; interface AppSettings { @@ -401,14 +402,39 @@ export function SettingsDialog({ const handleSetDefaultBrowser = useCallback(async () => { setIsSettingDefault(true); try { - await invoke("set_as_default_browser"); + // Windows keeps the final choice for its own settings page, so a call + // that succeeded does not always mean Donut is the default yet. Say which + // of the two happened. Saying nothing at all is what left the user + // watching the badge stay "Inactive" with no explanation. + const outcome = await invoke( + "set_as_default_browser", + ); await checkDefaultBrowserStatus(); + + if (outcome.status === "awaitingSystemSettings") { + showSuccessToast(t("settings.defaultBrowser.finishInSystemSettings"), { + description: t( + "settings.defaultBrowser.finishInSystemSettingsDescription", + ), + duration: 8000, + }); + } else { + showSuccessToast(t("settings.defaultBrowser.setSuccess")); + } } catch (error) { - console.error("Failed to set as default browser:", error); + showErrorToast(t("settings.defaultBrowser.setFailed"), { + description: + error instanceof Error + ? error.message + : typeof error === "string" + ? error + : t("common.errors.unknown"), + duration: 8000, + }); } finally { setIsSettingDefault(false); } - }, [checkDefaultBrowserStatus]); + }, [checkDefaultBrowserStatus, t]); const handleClearTraffic = useCallback(async () => { setIsClearingTraffic(true); diff --git a/src/i18n/locales/en.json b/src/i18n/locales/en.json index 22e2d90..2c112fe 100644 --- a/src/i18n/locales/en.json +++ b/src/i18n/locales/en.json @@ -134,7 +134,11 @@ "title": "Default Browser", "setAsDefault": "Set as Default Browser", "alreadyDefault": "Already Default Browser", - "description": "When set as default, Donut Browser will handle web links and allow you to choose which profile to use." + "description": "When set as default, Donut Browser will handle web links and allow you to choose which profile to use.", + "setSuccess": "Donut Browser is now your default browser", + "setFailed": "Could not set the default browser", + "finishInSystemSettings": "Finish in Windows Settings", + "finishInSystemSettingsDescription": "Donut Browser is registered. Windows Settings is open: choose Donut Browser under Web browser to finish." }, "permissions": { "title": "System Permissions", diff --git a/src/i18n/locales/es.json b/src/i18n/locales/es.json index eb12bc5..5d7f31e 100644 --- a/src/i18n/locales/es.json +++ b/src/i18n/locales/es.json @@ -134,7 +134,11 @@ "title": "Navegador Predeterminado", "setAsDefault": "Establecer como Navegador Predeterminado", "alreadyDefault": "Ya es el Navegador Predeterminado", - "description": "Cuando se establece como predeterminado, Donut Browser manejará los enlaces web y te permitirá elegir qué perfil usar." + "description": "Cuando se establece como predeterminado, Donut Browser manejará los enlaces web y te permitirá elegir qué perfil usar.", + "setSuccess": "Donut Browser ya es tu navegador predeterminado", + "setFailed": "No se pudo establecer el navegador predeterminado", + "finishInSystemSettings": "Termina en la Configuración de Windows", + "finishInSystemSettingsDescription": "Donut Browser está registrado. Se abrió la Configuración de Windows: elige Donut Browser en Navegador web para terminar." }, "permissions": { "title": "Permisos del Sistema", diff --git a/src/i18n/locales/fr.json b/src/i18n/locales/fr.json index d9000be..46aac8a 100644 --- a/src/i18n/locales/fr.json +++ b/src/i18n/locales/fr.json @@ -134,7 +134,11 @@ "title": "Navigateur par défaut", "setAsDefault": "Définir comme navigateur par défaut", "alreadyDefault": "Déjà le navigateur par défaut", - "description": "Lorsqu'il est défini par défaut, Donut Browser gérera les liens web et vous permettra de choisir quel profil utiliser." + "description": "Lorsqu'il est défini par défaut, Donut Browser gérera les liens web et vous permettra de choisir quel profil utiliser.", + "setSuccess": "Donut Browser est maintenant votre navigateur par défaut", + "setFailed": "Impossible de définir le navigateur par défaut", + "finishInSystemSettings": "Terminez dans les Paramètres Windows", + "finishInSystemSettingsDescription": "Donut Browser est enregistré. Les Paramètres Windows sont ouverts : choisissez Donut Browser sous Navigateur web pour terminer." }, "permissions": { "title": "Permissions système", diff --git a/src/i18n/locales/ja.json b/src/i18n/locales/ja.json index 78fddb8..b7f00b2 100644 --- a/src/i18n/locales/ja.json +++ b/src/i18n/locales/ja.json @@ -134,7 +134,11 @@ "title": "デフォルトブラウザ", "setAsDefault": "デフォルトブラウザに設定", "alreadyDefault": "既にデフォルトブラウザです", - "description": "デフォルトに設定すると、Donut Browser がウェブリンクを処理し、使用するプロファイルを選択できます。" + "description": "デフォルトに設定すると、Donut Browser がウェブリンクを処理し、使用するプロファイルを選択できます。", + "setSuccess": "Donut Browser が既定のブラウザーになりました", + "setFailed": "既定のブラウザーを設定できませんでした", + "finishInSystemSettings": "Windows の設定で完了してください", + "finishInSystemSettingsDescription": "Donut Browser を登録しました。Windows の設定が開いています。「Web ブラウザー」で Donut Browser を選ぶと完了します。" }, "permissions": { "title": "システム権限", diff --git a/src/i18n/locales/ko.json b/src/i18n/locales/ko.json index 84b405c..ab53482 100644 --- a/src/i18n/locales/ko.json +++ b/src/i18n/locales/ko.json @@ -134,7 +134,11 @@ "title": "기본 브라우저", "setAsDefault": "기본 브라우저로 설정", "alreadyDefault": "이미 기본 브라우저입니다", - "description": "기본 브라우저로 설정하면 Donut Browser가 웹 링크를 처리하고 사용할 프로필을 선택할 수 있습니다." + "description": "기본 브라우저로 설정하면 Donut Browser가 웹 링크를 처리하고 사용할 프로필을 선택할 수 있습니다.", + "setSuccess": "이제 Donut Browser가 기본 브라우저입니다", + "setFailed": "기본 브라우저를 설정하지 못했습니다", + "finishInSystemSettings": "Windows 설정에서 완료하세요", + "finishInSystemSettingsDescription": "Donut Browser가 등록되었습니다. Windows 설정이 열려 있습니다. '웹 브라우저'에서 Donut Browser를 선택하면 완료됩니다." }, "permissions": { "title": "시스템 권한", diff --git a/src/i18n/locales/pt.json b/src/i18n/locales/pt.json index 0764f74..72840d4 100644 --- a/src/i18n/locales/pt.json +++ b/src/i18n/locales/pt.json @@ -134,7 +134,11 @@ "title": "Navegador Padrão", "setAsDefault": "Definir como Navegador Padrão", "alreadyDefault": "Já é o Navegador Padrão", - "description": "Quando definido como padrão, o Donut Browser lidará com links da web e permitirá que você escolha qual perfil usar." + "description": "Quando definido como padrão, o Donut Browser lidará com links da web e permitirá que você escolha qual perfil usar.", + "setSuccess": "O Donut Browser agora é o seu navegador padrão", + "setFailed": "Não foi possível definir o navegador padrão", + "finishInSystemSettings": "Conclua nas Configurações do Windows", + "finishInSystemSettingsDescription": "O Donut Browser está registrado. As Configurações do Windows foram abertas: escolha o Donut Browser em Navegador da web para concluir." }, "permissions": { "title": "Permissões do Sistema", diff --git a/src/i18n/locales/ru.json b/src/i18n/locales/ru.json index ad0c97b..e323136 100644 --- a/src/i18n/locales/ru.json +++ b/src/i18n/locales/ru.json @@ -134,7 +134,11 @@ "title": "Браузер по умолчанию", "setAsDefault": "Установить браузером по умолчанию", "alreadyDefault": "Уже браузер по умолчанию", - "description": "При установке по умолчанию Donut Browser будет обрабатывать веб-ссылки и позволит выбрать профиль для использования." + "description": "При установке по умолчанию Donut Browser будет обрабатывать веб-ссылки и позволит выбрать профиль для использования.", + "setSuccess": "Donut Browser теперь браузер по умолчанию", + "setFailed": "Не удалось назначить браузер по умолчанию", + "finishInSystemSettings": "Завершите в параметрах Windows", + "finishInSystemSettingsDescription": "Donut Browser зарегистрирован. Параметры Windows открыты: выберите Donut Browser в разделе «Веб-браузер», чтобы завершить." }, "permissions": { "title": "Системные разрешения", diff --git a/src/i18n/locales/tr.json b/src/i18n/locales/tr.json index 212236e..dbf30ec 100644 --- a/src/i18n/locales/tr.json +++ b/src/i18n/locales/tr.json @@ -134,7 +134,11 @@ "title": "Varsayılan Tarayıcı", "setAsDefault": "Varsayılan Tarayıcı Olarak Ayarla", "alreadyDefault": "Zaten Varsayılan Tarayıcı", - "description": "Varsayılan olarak ayarlandığında, Donut Browser web bağlantılarını yönetir ve hangi profilin kullanılacağını seçmenize olanak tanır." + "description": "Varsayılan olarak ayarlandığında, Donut Browser web bağlantılarını yönetir ve hangi profilin kullanılacağını seçmenize olanak tanır.", + "setSuccess": "Donut Browser artık varsayılan tarayıcınız", + "setFailed": "Varsayılan tarayıcı ayarlanamadı", + "finishInSystemSettings": "Windows Ayarları'nda tamamlayın", + "finishInSystemSettingsDescription": "Donut Browser kaydedildi. Windows Ayarları açıldı: tamamlamak için Web tarayıcısı bölümünden Donut Browser'ı seçin." }, "permissions": { "title": "Sistem İzinleri", diff --git a/src/i18n/locales/vi.json b/src/i18n/locales/vi.json index 8609373..86ac5cf 100644 --- a/src/i18n/locales/vi.json +++ b/src/i18n/locales/vi.json @@ -134,7 +134,11 @@ "title": "Trình duyệt mặc định", "setAsDefault": "Đặt làm trình duyệt mặc định", "alreadyDefault": "Đã là trình duyệt mặc định", - "description": "Khi được đặt làm mặc định, Donut Browser sẽ xử lý các liên kết web và cho phép bạn chọn hồ sơ để sử dụng." + "description": "Khi được đặt làm mặc định, Donut Browser sẽ xử lý các liên kết web và cho phép bạn chọn hồ sơ để sử dụng.", + "setSuccess": "Donut Browser hiện là trình duyệt mặc định của bạn", + "setFailed": "Không thể đặt trình duyệt mặc định", + "finishInSystemSettings": "Hoàn tất trong Cài đặt Windows", + "finishInSystemSettingsDescription": "Donut Browser đã được đăng ký. Cài đặt Windows đang mở: chọn Donut Browser trong mục Trình duyệt web để hoàn tất." }, "permissions": { "title": "Quyền hệ thống", diff --git a/src/i18n/locales/zh.json b/src/i18n/locales/zh.json index f926b79..a3f5917 100644 --- a/src/i18n/locales/zh.json +++ b/src/i18n/locales/zh.json @@ -134,7 +134,11 @@ "title": "默认浏览器", "setAsDefault": "设为默认浏览器", "alreadyDefault": "已是默认浏览器", - "description": "设为默认后，Donut Browser 将处理网页链接并允许您选择使用哪个配置文件。" + "description": "设为默认后，Donut Browser 将处理网页链接并允许您选择使用哪个配置文件。", + "setSuccess": "Donut Browser 现在是您的默认浏览器", + "setFailed": "无法设置默认浏览器", + "finishInSystemSettings": "请在 Windows 设置中完成", + "finishInSystemSettingsDescription": "Donut Browser 已注册。Windows 设置已打开：在“Web 浏览器”中选择 Donut Browser 即可完成。" }, "permissions": { "title": "系统权限", diff --git a/src/types.ts b/src/types.ts index 7ca596b..040bcce 100644 --- a/src/types.ts +++ b/src/types.ts @@ -818,3 +818,16 @@ export interface PreLaunchChecks { exit_measurement_unreliable: boolean; consent_token: string | null; } + +/** + * What happened when the user asked Donut to become the default browser. + * + * macOS and Linux let a program make the change itself, so the answer there is + * always "set". Windows reserves the final choice for its own settings page: + * the app registers itself, Windows Settings opens, and the user finishes the + * job. Treating that case as plain success is how the button used to report a + * change that had not happened. + */ +export type SetDefaultBrowserOutcome = + | { status: "set" } + | { status: "awaitingSystemSettings" };