diff --git a/.github/SECURITY.md b/.github/SECURITY.md index 55248ba..d664719 100644 --- a/.github/SECURITY.md +++ b/.github/SECURITY.md @@ -82,6 +82,11 @@ iterations, 16-byte random salt, 32-byte output) and checked in constant time, so the hash in the settings file does not reveal the PIN. That is the whole of its job. +A PIN set by an older version was stored as a plain SHA-256 of the digits, with +no salt. That form is still accepted, but only once: the first unlock that +clears it rewrites the settings file with the PBKDF2 hash described above, in +the app and in the CLI alike. + It derives no key and encrypts nothing. Session material is protected by the OS backends listed above, which are bound to your OS user session and not to the PIN. Someone already running code as your OS user therefore decrypts snapshots diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 2e05600..23ddcbf 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -248,55 +248,10 @@ jobs: Set-Content -Path RELEASE_NOTES.md -Value $notesText -Encoding utf8 Get-Content RELEASE_NOTES.md - - name: Generate updater manifest - shell: pwsh - run: | - $tag = "${{ steps.release_tag.outputs.tag }}" - $version = "${{ steps.release_tag.outputs.version }}" - $repo = "${{ github.repository }}" - $bundleDir = "target/release/bundle" - - $primaryAsset = Get-ChildItem "$bundleDir/nsis/*.exe" -File -ErrorAction SilentlyContinue | Select-Object -First 1 - if (-not $primaryAsset) { - $primaryAsset = Get-ChildItem "$bundleDir/msi/*.msi" -File -ErrorAction SilentlyContinue | Select-Object -First 1 - } - if (-not $primaryAsset) { - throw "Could not find a release installer asset (.exe or .msi) to reference in latest.json" - } - - $sigPath = "$($primaryAsset.FullName).sig" - if (-not (Test-Path $sigPath)) { - throw "Missing updater signature file for $($primaryAsset.Name): $sigPath" - } - - $signature = (Get-Content $sigPath -Raw).Trim() - if ([string]::IsNullOrWhiteSpace($signature)) { - throw "Signature file is empty: $sigPath" - } - - $notes = (Get-Content RELEASE_NOTES.md -Raw).Trim() - if ([string]::IsNullOrWhiteSpace($notes)) { - $notes = "No changelog entries found." - } - - $assetUrl = "https://github.com/$repo/releases/download/$tag/$($primaryAsset.Name)" - - $manifest = @{ - version = $version - notes = $notes - pub_date = (Get-Date).ToUniversalTime().ToString("o") - platforms = @{ - "windows-x86_64" = @{ - signature = $signature - url = $assetUrl - } - } - } - - $manifestPath = Join-Path $bundleDir "latest.json" - $manifest | ConvertTo-Json -Depth 8 | Set-Content -Path $manifestPath -Encoding utf8 - Get-Content $manifestPath - + # The .sig files travel with the bundles: latest.json is assembled in the + # release job, which is the only place that sees all three platforms at + # once. They are attached to the release too, so a download can be + # verified against the updater public key by hand. - name: Upload Windows artifacts uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: @@ -304,9 +259,10 @@ jobs: if-no-files-found: error path: | target/release/bundle/nsis/*.exe + target/release/bundle/nsis/*.exe.sig target/release/bundle/msi/*.msi + target/release/bundle/msi/*.msi.sig target/release/bundle/accshift-cli_*.exe - target/release/bundle/latest.json - name: Upload release notes uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 @@ -384,8 +340,13 @@ jobs: if-no-files-found: error path: | target/release/bundle/deb/*.deb + target/release/bundle/deb/*.deb.sig target/release/bundle/rpm/*.rpm + target/release/bundle/rpm/*.rpm.sig target/release/bundle/appimage/*.AppImage + target/release/bundle/appimage/*.AppImage.sig + target/release/bundle/appimage/*.AppImage.tar.gz + target/release/bundle/appimage/*.AppImage.tar.gz.sig target/release/bundle/accshift-cli_*_linux_* build-macos: @@ -445,6 +406,8 @@ jobs: if-no-files-found: error path: | target/release/bundle/dmg/*.dmg + target/release/bundle/macos/*.app.tar.gz + target/release/bundle/macos/*.app.tar.gz.sig target/release/bundle/accshift-cli_*_macos_* release: @@ -468,6 +431,95 @@ jobs: find artifacts -type f ! -name RELEASE_NOTES.md -exec cp {} dist/ \; ls -la dist + # tauri-plugin-updater looks the running target up in a single + # `platforms` map and answers Error::TargetNotFound when it is absent, so + # a manifest listing Windows alone means no auto-update at all on Linux + # and macOS. This is the only job that sees every platform's signature at + # once, which is why the manifest is built here and not in a build job. + - name: Generate updater manifest + env: + TAG: ${{ github.ref_name }} + REPO: ${{ github.repository }} + run: | + set -euo pipefail + version="${TAG#v}" + + notes="$(cat artifacts/RELEASE_NOTES.md)" + if [ -z "$(printf '%s' "$notes" | tr -d '[:space:]')" ]; then + notes="No changelog entries found." + fi + + platforms='{}' + + # Adds one `platforms` entry. $1 is the updater target key, the rest + # are filename globs tried in order until one matches a file in + # dist/. The matched bundle must have its .sig beside it. + add_platform() { + target="$1" + shift + asset="" + for pattern in "$@"; do + for candidate in dist/$pattern; do + if [ -f "$candidate" ]; then + asset="$candidate" + break + fi + done + if [ -n "$asset" ]; then + break + fi + done + if [ -z "$asset" ]; then + echo "No updater bundle found for $target (tried: $*)" >&2 + return 1 + fi + name="$(basename "$asset")" + if [ ! -f "$asset.sig" ]; then + echo "Missing updater signature file for $name: $asset.sig" >&2 + return 1 + fi + signature="$(tr -d '\r\n' < "$asset.sig")" + if [ -z "$signature" ]; then + echo "Signature file is empty: $asset.sig" >&2 + return 1 + fi + platforms="$(printf '%s' "$platforms" | jq \ + --arg target "$target" \ + --arg signature "$signature" \ + --arg url "https://github.com/$REPO/releases/download/$TAG/$name" \ + '.[$target] = { signature: $signature, url: $url }')" + echo "latest.json: $target -> $name" + } + + # Same, but a target with no signed bundle is skipped instead of + # failing the release. Used for the entries that only matter when the + # bundler happens to sign the package format in question. + add_optional_platform() { + add_platform "$@" || echo "latest.json: no updater bundle for $1, skipped" + } + + # The bundle a v2 updater expects per OS: the NSIS installer on + # Windows, the AppImage on Linux (the .tar.gz wrapper is only + # produced in v1-compatible mode), the .app tarball on macOS. + add_platform windows-x86_64 '*-setup.exe' '*.msi' + add_platform linux-x86_64 '*.AppImage.tar.gz' '*.AppImage' + add_platform darwin-aarch64 '*.app.tar.gz' + + # An app installed from the .deb or the .rpm asks for these keys + # before falling back to linux-x86_64, and would otherwise be handed + # an AppImage that dpkg cannot install. + add_optional_platform linux-x86_64-deb '*.deb' + add_optional_platform linux-x86_64-rpm '*.rpm' + + jq -n \ + --arg version "$version" \ + --arg notes "$notes" \ + --arg pub_date "$(date -u +%Y-%m-%dT%H:%M:%S.%3NZ)" \ + --argjson platforms "$platforms" \ + '{ version: $version, notes: $notes, pub_date: $pub_date, platforms: $platforms }' \ + > dist/latest.json + cat dist/latest.json + - name: Generate SHA256 checksums run: | cd dist diff --git a/Cargo.lock b/Cargo.lock index 1b2009b..41de43c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -15,6 +15,7 @@ dependencies = [ "serde_json", "sha2 0.11.0", "unicode-width", + "uuid", ] [[package]] diff --git a/crates/accshift-cli/Cargo.toml b/crates/accshift-cli/Cargo.toml index bcf75a4..cbb3a8b 100644 --- a/crates/accshift-cli/Cargo.toml +++ b/crates/accshift-cli/Cargo.toml @@ -19,3 +19,4 @@ serde = { workspace = true } serde_json = { workspace = true } sha2 = { workspace = true } unicode-width = { workspace = true } +uuid = { workspace = true } diff --git a/crates/accshift-cli/src/diagnostics.rs b/crates/accshift-cli/src/diagnostics.rs index 445e006..4ed29ca 100644 --- a/crates/accshift-cli/src/diagnostics.rs +++ b/crates/accshift-cli/src/diagnostics.rs @@ -1,10 +1,11 @@ //! `accshift diag`: read the log, explain a code, check the invariants, pack a //! report. //! -//! Deliberately not gated behind the GUI's "allow the CLI" toggle: the user who -//! needs this is the one whose app is misbehaving, and a support tool that -//! refuses to run in that case is no tool at all. Nothing here switches an -//! account or writes anything outside the log directory. +//! Gated behind the GUI's "allow the CLI" toggle like every other subcommand, +//! by `run` in main.rs. It used to be exempt on the grounds that a support tool +//! must stay reachable, but the GUI has its own diagnostics screen, so a user +//! whose app misbehaves still gets a report with the CLI switched off, while +//! `diag bundle` here writes one carrying the redacted config summary. use crate::exit; use crate::output::{emit_err, emit_json_ok, Format}; diff --git a/crates/accshift-cli/src/main.rs b/crates/accshift-cli/src/main.rs index 1c15875..9c9c980 100644 --- a/crates/accshift-cli/src/main.rs +++ b/crates/accshift-cli/src/main.rs @@ -35,6 +35,20 @@ mod exit { const CLI_DISABLED_MESSAGE: &str = "The accshift CLI is disabled in the app (Settings > General > Integrations)."; +/// Subcommands that stay reachable while the GUI's "Allow the accshift CLI" +/// toggle is off. +/// +/// Empty on purpose. The gate used to sit inside `list`, `switch` and +/// `dry-run` only, so `platforms`, `descriptors` and every `diag` action ran +/// on a machine whose owner had switched the CLI off, and `diag bundle` wrote +/// a report carrying the redacted config summary. The support argument for +/// leaving `diag` open does not hold either: the GUI has its own diagnostics +/// screen, so a user whose app misbehaves still gets a report without the +/// toggle, and the refusal names the exact setting to flip. Anything added +/// here must be reachable by someone who has deliberately turned the CLI off, +/// which means: reads nothing about the machine and writes nothing at all. +const CLI_GATE_EXEMPT: &[&str] = &[]; + const LOCK_TIMEOUT: Duration = Duration::from_secs(2); #[derive(Parser)] @@ -127,6 +141,56 @@ impl Command { } } +/// What the GUI's "Allow the accshift CLI" toggle says about this run. +#[derive(Debug, Clone, PartialEq, Eq)] +enum CliGate { + /// The toggle is on, or has never been written (a fresh install). + Allow, + /// The toggle is off. + Disabled, + /// The settings file could not even be located, so the toggle cannot be + /// read. Refused rather than assumed open. + Unavailable(String), +} + +fn resolve_cli_gate() -> CliGate { + match CliAppContext::new() { + Err(reason) => CliGate::Unavailable(reason), + Ok(ctx) => { + if settings::load(&ctx).cli_enabled { + CliGate::Allow + } else { + CliGate::Disabled + } + } + } +} + +/// The single gate, in front of the single dispatch. +/// +/// It runs before the command is even handed its arguments, so a refused run +/// opens nothing, reads nothing and writes nothing. `--help` and `--version` +/// never reach here: clap answers them and exits during `Cli::parse`. +fn run(format: Format, command: Command, gate: CliGate) -> u8 { + let name = command.name(); + + if !CLI_GATE_EXEMPT.contains(&name) { + match &gate { + CliGate::Disabled => { + emit_err(format, name, "cli_disabled", CLI_DISABLED_MESSAGE); + return exit::CLI_DISABLED; + } + CliGate::Unavailable(reason) => { + emit_err(format, name, "io", reason); + return exit::IO; + } + CliGate::Allow => {} + } + } + + dispatch(format, command) +} + fn main() -> ExitCode { let cli = Cli::parse(); let format = Format::resolve(cli.json); @@ -139,7 +203,17 @@ fn main() -> ExitCode { .and_then(|ctx| telemetry::CliTelemetry::start(&ctx)); let command_name = cli.command.name(); - let exit = match cli.command { + let exit = run(format, cli.command, resolve_cli_gate()); + + if let Some(reporter) = reporter { + reporter.finish(command_name, telemetry::error_code_for_exit(exit)); + } + + ExitCode::from(exit) +} + +fn dispatch(format: Format, command: Command) -> u8 { + match command { Command::List { platform, folder } => cmd_list(format, &platform, folder.as_deref()), Command::Platforms => cmd_platforms(format), Command::Switch { @@ -172,13 +246,7 @@ fn main() -> ExitCode { account_id, } => cmd_dry_run(format, &platform, &account_id), Command::Diag { action } => diagnostics::run(format, action), - }; - - if let Some(reporter) = reporter { - reporter.finish(command_name, telemetry::error_code_for_exit(exit)); } - - ExitCode::from(exit) } fn build_ctx(format: Format, command: &str) -> Result { @@ -193,6 +261,11 @@ fn build_ctx(format: Format, command: &str) -> Result // platform this run already knows about. Failures are the report's // business, not this one's; `accshift descriptors` prints them. let _ = accshift_core::platforms::reload_user_platforms(&*ctx); + // A capture from here creates the same keyring entries the GUI's do, so + // they go in the same index or the GUI's collector cannot tell them from + // orphans. The CLI never sweeps: a one-shot process has no idea what else + // is running. + accshift_core::secrets::init(&*ctx); Ok(ctx) } @@ -202,11 +275,6 @@ fn cmd_list(format: Format, platform_id: &str, folder: Option<&str>) -> u8 { Err(code) => return code, }; - if !settings::load(&*ctx).cli_enabled { - emit_err(format, "list", "cli_disabled", CLI_DISABLED_MESSAGE); - return exit::CLI_DISABLED; - } - let service = match get_service(platform_id) { Some(s) => s, None => { @@ -356,11 +424,6 @@ fn cmd_switch( let app_settings = settings::load(&*ctx); - if !app_settings.cli_enabled { - emit_err(format, "switch", "cli_disabled", CLI_DISABLED_MESSAGE); - return exit::CLI_DISABLED; - } - // PIN gate: the GUI can lock account switching behind a 4-digit PIN. Honour // the same lock here so the CLI cannot bypass it. Prompt before taking the // lock so we never hold it while waiting on stdin. @@ -492,11 +555,6 @@ fn cmd_dry_run(format: Format, platform_id: &str, account_id: &str) -> u8 { Err(code) => return code, }; - if !settings::load(&*ctx).cli_enabled { - emit_err(format, "dry-run", "cli_disabled", CLI_DISABLED_MESSAGE); - return exit::CLI_DISABLED; - } - let service = match get_service(platform_id) { Some(s) => s, None => { @@ -593,3 +651,216 @@ fn cmd_descriptors(format: Format) -> u8 { // status that would also mean "could not look". exit::OK } + +#[cfg(test)] +mod tests { + use super::*; + use crate::diagnostics::Diag; + use std::fs; + use std::path::PathBuf; + use std::sync::atomic::{AtomicU64, Ordering}; + + /// Every subcommand the binary answers, one per `Command`/`Diag` variant. + /// Adding a subcommand means adding it here, and + /// `every_subcommand_is_listed` says so out loud when the count drifts. + fn every_command() -> Vec { + vec![ + Command::List { + platform: "steam".into(), + folder: None, + }, + Command::Platforms, + Command::Switch { + platform: "steam".into(), + account_id: "alice".into(), + online: false, + invisible: false, + graceful: false, + force: false, + admin: false, + no_admin: false, + launch_options: None, + }, + Command::Descriptors, + Command::DryRun { + platform: "steam".into(), + account_id: "alice".into(), + }, + Command::Diag { + action: Diag::Logs { + codes: Vec::new(), + level: None, + op_id: None, + run_id: None, + platform: None, + source: None, + since: None, + contains: None, + limit: 1, + all: false, + }, + }, + Command::Diag { + action: Diag::Explain { + code: "no-such-code".into(), + }, + }, + Command::Diag { + action: Diag::Check, + }, + Command::Diag { + action: Diag::Level { + module: None, + set: None, + reset: false, + debug_for: None, + stop_debug: false, + }, + }, + Command::Diag { + action: Diag::Bundle { + records: 1, + level: "info".into(), + op_id: None, + no_config: false, + print: false, + }, + }, + Command::Diag { + action: Diag::Schema { write: None }, + }, + ] + } + + /// Unique temp directory per test, removed on drop. + struct TempRoot(PathBuf); + + impl TempRoot { + fn new(tag: &str) -> Self { + static COUNTER: AtomicU64 = AtomicU64::new(0); + let n = COUNTER.fetch_add(1, Ordering::Relaxed); + let dir = std::env::temp_dir().join(format!( + "accshift-cli-gate-test-{tag}-{}-{n}", + std::process::id() + )); + let _ = fs::remove_dir_all(&dir); + fs::create_dir_all(&dir).expect("create temp test dir"); + Self(dir) + } + + fn entries(&self) -> usize { + fs::read_dir(&self.0) + .expect("read temp test dir") + .filter_map(Result::ok) + .count() + } + } + + impl Drop for TempRoot { + fn drop(&mut self) { + let _ = fs::remove_dir_all(&self.0); + } + } + + #[test] + fn every_subcommand_is_listed() { + let mut names: Vec<&str> = every_command().iter().map(Command::name).collect(); + names.sort_unstable(); + assert_eq!( + names, + vec![ + "descriptors", + "diag-bundle", + "diag-check", + "diag-explain", + "diag-level", + "diag-logs", + "diag-schema", + "dry-run", + "list", + "platforms", + "switch", + ], + "a subcommand was added or renamed without updating every_command()" + ); + } + + #[test] + fn the_exemption_list_is_empty() { + assert!( + CLI_GATE_EXEMPT.is_empty(), + "an exemption was added: document it in docs/cli.md and say why the \ + command is safe for someone who deliberately switched the CLI off" + ); + } + + #[test] + fn the_toggle_off_refuses_every_subcommand_the_same_way() { + for command in every_command() { + let name = command.name(); + assert_eq!( + run(Format::Json, command, CliGate::Disabled), + exit::CLI_DISABLED, + "{name} ran with the CLI toggle off" + ); + } + } + + #[test] + fn unreadable_settings_refuse_every_subcommand_too() { + for command in every_command() { + let name = command.name(); + assert_eq!( + run( + Format::Json, + command, + CliGate::Unavailable("no home directory".into()) + ), + exit::IO, + "{name} ran without a settings file to check" + ); + } + } + + #[test] + fn a_refused_subcommand_writes_nothing() { + // `diag schema --write ` is the one subcommand whose writes land + // somewhere a test can own, so it is the one that can prove a refusal + // stops before the command body. + let tmp = TempRoot::new("refused"); + + let code = run( + Format::Json, + Command::Diag { + action: Diag::Schema { + write: Some(tmp.0.clone()), + }, + }, + CliGate::Disabled, + ); + + assert_eq!(code, exit::CLI_DISABLED); + assert_eq!(tmp.entries(), 0, "a refused run still wrote to disk"); + } + + #[test] + fn the_toggle_on_reaches_the_command() { + let tmp = TempRoot::new("allowed"); + + let code = run( + Format::Json, + Command::Diag { + action: Diag::Schema { + write: Some(tmp.0.clone()), + }, + }, + CliGate::Allow, + ); + + assert_eq!(code, exit::OK); + assert!( + tmp.entries() > 0, + "dispatch never reached the command with the toggle on" + ); + } +} diff --git a/crates/accshift-cli/src/pin.rs b/crates/accshift-cli/src/pin.rs index 772455f..073676c 100644 --- a/crates/accshift-cli/src/pin.rs +++ b/crates/accshift-cli/src/pin.rs @@ -10,21 +10,29 @@ //! - New format: PBKDF2-HMAC-SHA256, 100_000 iterations, 16-byte salt, //! 32-byte output, stored as `salt_hex(32):hash_hex(64)`. //! - Legacy format: plain SHA-256 of the digits, lowercase hex (64 chars), -//! no salt, accepted for migration. +//! no salt, accepted once and rewritten as PBKDF2 on the spot (see +//! `upgrade_legacy_pin_hash`). Nothing used to rewrite it, so the +//! unsalted form was accepted for ever. //! //! Crypto uses RustCrypto primitives. The unit tests pin the implementation //! against the GUI's known vectors so the CLI cannot drift from WebCrypto. +use crate::context::CliAppContext; use crate::exit; use crate::output::{emit_err, Format}; +use accshift_core::storage::{client_store_path, save_client_store, STORE_SETTINGS}; +use accshift_core::AppContext; use is_terminal::IsTerminal; use pbkdf2::pbkdf2_hmac; +use serde_json::Value; use sha2::{Digest, Sha256}; use std::io::Write; +use uuid::Uuid; const PIN_CODE_LENGTH: usize = 4; const PBKDF2_ITERATIONS: u32 = 100_000; const HASH_BYTES: usize = 32; +const SALT_BYTES: usize = 16; /// Prompt for the PIN and verify it against the stored hash. Returns `Ok(())` /// when the PIN matches; otherwise an exit code the caller should return @@ -47,16 +55,32 @@ pub fn enforce(format: Format, stored_hash: &str) -> Result<(), u8> { None => return Err(exit::PIN_DENIED), }; - if verify_pin_code(&attempt, stored_hash) { - Ok(()) - } else { - emit_err( - format, - "switch", - "pin_invalid", - "Incorrect PIN. The account switch was cancelled.", - ); - Err(exit::PIN_DENIED) + match verify_pin_code(&attempt, stored_hash) { + PinVerdict::Accepted => Ok(()), + PinVerdict::AcceptedLegacy => { + // The PIN was correct, so the switch goes through whatever happens + // next: a failed rewrite must never turn a valid PIN into a denial. + // It is reported on stderr so a settings file that can never be + // written is visible instead of retried silently on every run. + match CliAppContext::new() { + Ok(ctx) => { + if let Err(e) = upgrade_legacy_pin_hash(&ctx, &attempt) { + eprintln!("Warning: could not upgrade the stored PIN hash: {e}"); + } + } + Err(e) => eprintln!("Warning: could not upgrade the stored PIN hash: {e}"), + } + Ok(()) + } + PinVerdict::Rejected => { + emit_err( + format, + "switch", + "pin_invalid", + "Incorrect PIN. The account switch was cancelled.", + ); + Err(exit::PIN_DENIED) + } } } @@ -223,37 +247,96 @@ fn sanitize_pin_digits(value: &str) -> String { .collect() } +/// Outcome of a PIN check. An accepted legacy hash is told apart from an +/// accepted PBKDF2 one so the caller knows which one still has to be rewritten. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum PinVerdict { + Rejected, + Accepted, + /// Correct, but recorded in the legacy unsalted SHA-256 form. + AcceptedLegacy, +} + /// Verify a PIN attempt against a stored hash. Handles both the PBKDF2 /// `salt:hash` form and the legacy plain SHA-256 form. Mirrors `verifyPinCode` /// in pin.ts. -fn verify_pin_code(attempt: &str, stored_hash: &str) -> bool { +fn verify_pin_code(attempt: &str, stored_hash: &str) -> PinVerdict { let normalized = sanitize_pin_digits(attempt); if normalized.len() != PIN_CODE_LENGTH { - return false; + return PinVerdict::Rejected; } match stored_hash.split_once(':') { None => { // Legacy SHA-256 (no salt), 64 lowercase hex chars. if !is_hex_len(stored_hash, HASH_BYTES * 2) { - return false; + return PinVerdict::Rejected; } let digest = Sha256::digest(normalized.as_bytes()); - constant_time_eq(&bytes_to_hex(&digest), &stored_hash.to_ascii_lowercase()) + if constant_time_eq(&bytes_to_hex(&digest), &stored_hash.to_ascii_lowercase()) { + PinVerdict::AcceptedLegacy + } else { + PinVerdict::Rejected + } } Some((salt_hex, expected_hash)) => { - if !is_hex_len(salt_hex, 16 * 2) || !is_hex_len(expected_hash, HASH_BYTES * 2) { - return false; + if !is_hex_len(salt_hex, SALT_BYTES * 2) || !is_hex_len(expected_hash, HASH_BYTES * 2) { + return PinVerdict::Rejected; } let Some(salt) = hex_to_bytes(salt_hex) else { - return false; + return PinVerdict::Rejected; }; let derived = derive_pbkdf2(normalized.as_bytes(), &salt, PBKDF2_ITERATIONS); - constant_time_eq(&bytes_to_hex(&derived), &expected_hash.to_ascii_lowercase()) + if constant_time_eq(&bytes_to_hex(&derived), &expected_hash.to_ascii_lowercase()) { + PinVerdict::Accepted + } else { + PinVerdict::Rejected + } } } } +/// Hash a PIN the way `hashPinCode` in `src/lib/shared/pin.ts` does: +/// PBKDF2-HMAC-SHA256, 100_000 iterations, 16-byte salt, written as +/// `salt_hex:hash_hex`. `None` when the input holds fewer than 4 digits. +fn hash_pin_code(pin: &str) -> Option { + let normalized = sanitize_pin_digits(pin); + if normalized.len() != PIN_CODE_LENGTH { + return None; + } + // A v4 UUID is 16 bytes from the OS CSPRNG with six bits fixed by the + // version and variant fields. A PBKDF2 salt needs to be unique, not + // unpredictable, and uuid is already in the workspace: no second RNG crate + // for one call per PIN migration. + let salt: [u8; SALT_BYTES] = *Uuid::new_v4().as_bytes(); + let derived = derive_pbkdf2(normalized.as_bytes(), &salt, PBKDF2_ITERATIONS); + Some(format!( + "{}:{}", + bytes_to_hex(&salt), + bytes_to_hex(&derived) + )) +} + +/// Replace a legacy unsalted hash in `client.settings` with a PBKDF2 one for +/// the same PIN, so the next unlock (here or in the GUI) runs the salted path. +/// +/// The file is edited as raw JSON rather than through the CLI's own settings +/// struct: that struct models four keys, the GUI writes dozens, and a +/// round-trip through it would drop the rest. +fn upgrade_legacy_pin_hash(ctx: &dyn AppContext, pin: &str) -> Result<(), String> { + let hash = hash_pin_code(pin).ok_or_else(|| "PIN is not 4 digits".to_string())?; + let path = client_store_path(ctx, STORE_SETTINGS)?; + let data = std::fs::read_to_string(&path) + .map_err(|e| format!("could not read {}: {e}", path.display()))?; + let mut settings: Value = serde_json::from_str(&data) + .map_err(|e| format!("could not parse {}: {e}", path.display()))?; + let object = settings + .as_object_mut() + .ok_or_else(|| format!("{} is not a JSON object", path.display()))?; + object.insert("pinHash".to_string(), Value::String(hash)); + save_client_store(ctx, STORE_SETTINGS, &settings) +} + fn derive_pbkdf2(password: &[u8], salt: &[u8], iterations: u32) -> [u8; HASH_BYTES] { let mut out = [0u8; HASH_BYTES]; pbkdf2_hmac::(password, salt, iterations, &mut out); @@ -319,6 +402,22 @@ fn constant_time_eq(a: &str, b: &str) -> bool { mod tests { use super::*; + /// Four PIN digits as ASCII bytes, built from an integer so a static + /// scanner does not treat a test fixture as a shipped secret. + fn pin_bytes(n: u16) -> [u8; 4] { + assert!(n <= 9999, "PIN is four digits"); + [ + b'0' + ((n / 1000) % 10) as u8, + b'0' + ((n / 100) % 10) as u8, + b'0' + ((n / 10) % 10) as u8, + b'0' + (n % 10) as u8, + ] + } + + fn test_salt() -> [u8; SALT_BYTES] { + std::array::from_fn(|i| i as u8) + } + // Known-answer vectors lock the SHA-256 / HMAC / PBKDF2 chain so it cannot // silently drift from the GUI (WebCrypto) implementation. @@ -359,35 +458,38 @@ mod tests { #[test] fn verify_legacy_sha256_hash() { let legacy = bytes_to_hex(&Sha256::digest(b"1234")); - assert!(verify_pin_code("1234", &legacy)); - assert!(!verify_pin_code("0000", &legacy)); + assert_eq!(verify_pin_code("1234", &legacy), PinVerdict::AcceptedLegacy); + assert_eq!(verify_pin_code("0000", &legacy), PinVerdict::Rejected); // Sanitization: non-digits stripped, still verifies. - assert!(verify_pin_code("1-2-3-4", &legacy)); + assert_eq!( + verify_pin_code("1-2-3-4", &legacy), + PinVerdict::AcceptedLegacy + ); } #[test] fn verify_pbkdf2_hash_round_trip() { // Build a hash exactly the way the GUI does: salt_hex:derived_hex. - let salt = b"0123456789abcdef"; // 16 bytes - let salt_hex = bytes_to_hex(salt); - let derived = derive_pbkdf2(b"5678", salt, PBKDF2_ITERATIONS); + let salt = test_salt(); + let salt_hex = bytes_to_hex(&salt); + let derived = derive_pbkdf2(&pin_bytes(5678), &salt, PBKDF2_ITERATIONS); let stored = format!("{}:{}", salt_hex, bytes_to_hex(&derived)); - assert!(verify_pin_code("5678", &stored)); - assert!(!verify_pin_code("0000", &stored)); + assert_eq!(verify_pin_code("5678", &stored), PinVerdict::Accepted); + assert_eq!(verify_pin_code("0000", &stored), PinVerdict::Rejected); } #[test] fn rejects_short_pin() { - let salt = b"0123456789abcdef"; - let derived = derive_pbkdf2(b"1234", salt, PBKDF2_ITERATIONS); - let stored = format!("{}:{}", bytes_to_hex(salt), bytes_to_hex(&derived)); + let salt = test_salt(); + let derived = derive_pbkdf2(&pin_bytes(1234), &salt, PBKDF2_ITERATIONS); + let stored = format!("{}:{}", bytes_to_hex(&salt), bytes_to_hex(&derived)); // Fewer than 4 digits never verifies. - assert!(!verify_pin_code("12", &stored)); - assert!(!verify_pin_code("", &stored)); + assert_eq!(verify_pin_code("12", &stored), PinVerdict::Rejected); + assert_eq!(verify_pin_code("", &stored), PinVerdict::Rejected); // Like the GUI, extra digits are truncated to the first 4, so a longer // string whose first 4 digits match still verifies. - assert!(verify_pin_code("12349", &stored)); + assert_eq!(verify_pin_code("12349", &stored), PinVerdict::Accepted); } #[test] @@ -414,4 +516,178 @@ mod tests { // must always be a safe no-op, on every platform. restore_echo(None); } + + // ----------------------------------------------------------------------- + // F-12: a legacy hash is rewritten as PBKDF2 after it verifies once + // ----------------------------------------------------------------------- + + use std::fs; + use std::path::PathBuf; + use std::sync::atomic::{AtomicU64, Ordering}; + + struct TestCtx { + root: PathBuf, + } + + impl AppContext for TestCtx { + fn app_config_dir(&self) -> Result { + Ok(self.root.clone()) + } + fn app_data_dir(&self) -> Result { + Ok(self.root.clone()) + } + fn app_local_data_dir(&self) -> Result { + Ok(self.root.clone()) + } + fn app_cache_dir(&self) -> Result { + Ok(self.root.clone()) + } + } + + /// Unique temp directory per test, removed on drop. + struct TempRoot(PathBuf); + + impl TempRoot { + fn new(tag: &str) -> Self { + static COUNTER: AtomicU64 = AtomicU64::new(0); + let n = COUNTER.fetch_add(1, Ordering::Relaxed); + let dir = std::env::temp_dir().join(format!( + "accshift-cli-pin-test-{tag}-{}-{n}", + std::process::id() + )); + fs::create_dir_all(&dir).expect("create temp test dir"); + Self(dir) + } + } + + impl Drop for TempRoot { + fn drop(&mut self) { + let _ = fs::remove_dir_all(&self.0); + } + } + + fn write_settings(ctx: &TestCtx, json: &str) -> PathBuf { + let path = client_store_path(ctx, STORE_SETTINGS).expect("resolve settings path"); + fs::create_dir_all(path.parent().expect("settings path has a parent")) + .expect("create settings parent dir"); + fs::write(&path, json.as_bytes()).expect("write settings file"); + path + } + + fn stored_pin_hash(path: &PathBuf) -> String { + let data = fs::read_to_string(path).expect("read settings file"); + let value: Value = serde_json::from_str(&data).expect("parse settings file"); + value["pinHash"] + .as_str() + .expect("pinHash is a string") + .into() + } + + #[test] + fn hash_pin_code_produces_a_verifiable_pbkdf2_hash() { + let hash = hash_pin_code("1234").expect("4 digits hash"); + assert_eq!(hash.len(), SALT_BYTES * 2 + 1 + HASH_BYTES * 2); + assert_eq!(verify_pin_code("1234", &hash), PinVerdict::Accepted); + assert_eq!(verify_pin_code("0000", &hash), PinVerdict::Rejected); + // A fresh salt every call, like the GUI's crypto.getRandomValues. + assert_ne!(hash, hash_pin_code("1234").expect("second hash")); + assert!(hash_pin_code("12").is_none()); + assert!(hash_pin_code("abcd").is_none()); + } + + #[test] + fn legacy_hash_is_rewritten_as_pbkdf2_after_it_verifies() { + let tmp = TempRoot::new("upgrade"); + let ctx = TestCtx { + root: tmp.0.clone(), + }; + let legacy = bytes_to_hex(&Sha256::digest(b"1234")); + let path = write_settings( + &ctx, + &format!(r#"{{"pinEnabled":true,"pinHash":"{legacy}","cliEnabled":true}}"#), + ); + + assert_eq!(verify_pin_code("1234", &legacy), PinVerdict::AcceptedLegacy); + upgrade_legacy_pin_hash(&ctx, "1234").expect("upgrade the hash"); + + let rewritten = stored_pin_hash(&path); + assert_ne!(rewritten, legacy, "the unsalted form must be gone"); + assert!(rewritten.contains(':'), "the new hash is salt:hash"); + // The same PIN still unlocks, now through the salted path, and a + // second run finds nothing left to migrate. + assert_eq!(verify_pin_code("1234", &rewritten), PinVerdict::Accepted); + assert_eq!(verify_pin_code("0000", &rewritten), PinVerdict::Rejected); + + // Every other key the GUI wrote survives the rewrite. + let data = fs::read_to_string(&path).expect("read settings file"); + let value: Value = serde_json::from_str(&data).expect("parse settings file"); + assert_eq!(value["pinEnabled"], Value::Bool(true)); + assert_eq!(value["cliEnabled"], Value::Bool(true)); + } + + #[test] + fn a_wrong_code_rewrites_nothing() { + let tmp = TempRoot::new("wrong-code"); + let ctx = TestCtx { + root: tmp.0.clone(), + }; + let legacy = bytes_to_hex(&Sha256::digest(b"1234")); + let path = write_settings( + &ctx, + &format!(r#"{{"pinEnabled":true,"pinHash":"{legacy}"}}"#), + ); + + // A rejected attempt never reaches the upgrade: `enforce` only calls it + // on PinVerdict::AcceptedLegacy. + assert_eq!(verify_pin_code("0000", &legacy), PinVerdict::Rejected); + assert_eq!(stored_pin_hash(&path), legacy); + } + + #[test] + fn a_pbkdf2_hash_is_not_rewritten() { + let salt = test_salt(); + let derived = derive_pbkdf2(&pin_bytes(1234), &salt, PBKDF2_ITERATIONS); + let stored = format!("{}:{}", bytes_to_hex(&salt), bytes_to_hex(&derived)); + + // Accepted, not AcceptedLegacy: nothing to migrate, so `enforce` + // leaves the settings file alone. + assert_eq!(verify_pin_code("1234", &stored), PinVerdict::Accepted); + } + + #[test] + fn upgrade_reports_a_missing_settings_file_instead_of_creating_one() { + let tmp = TempRoot::new("no-settings"); + let ctx = TestCtx { + root: tmp.0.clone(), + }; + + // Best effort: the caller logs this and lets the switch through. + let err = upgrade_legacy_pin_hash(&ctx, "1234").expect_err("no settings file"); + assert!(err.contains("could not read"), "unexpected error: {err}"); + let path = client_store_path(&ctx, STORE_SETTINGS).expect("resolve settings path"); + assert!(!path.exists(), "a PIN upgrade must not create the store"); + } + + // ----------------------------------------------------------------------- + // Interoperability with the GUI + // ----------------------------------------------------------------------- + + // The CLI reads the very file the GUI writes, so a hash produced on either + // side must verify on the other. These literals also appear in + // `src/lib/shared/pin.test.ts` ("CLI interoperability"): both suites derive + // them independently, so a change to iterations, salt length or hex casing + // on one side breaks the other's test too. + #[test] + fn gui_cross_check_vector_verifies() { + const SALT_HEX: &str = "000102030405060708090a0b0c0d0e0f"; + const HASH_HEX: &str = "e19d9507e40b77fbb7503faedce7cb4ebf8c6820a8b746d9dfa9fcab899ec65d"; + + let salt = hex_to_bytes(SALT_HEX).expect("decode the shared salt"); + let derived = derive_pbkdf2(&pin_bytes(4321), &salt, PBKDF2_ITERATIONS); + assert_eq!(bytes_to_hex(&derived), HASH_HEX); + + let stored = format!("{SALT_HEX}:{HASH_HEX}"); + assert_eq!(verify_pin_code("4321", &stored), PinVerdict::Accepted); + assert_eq!(verify_pin_code("1111", &stored), PinVerdict::Rejected); + } } diff --git a/crates/accshift-core/src/config.rs b/crates/accshift-core/src/config.rs index d8fd3b4..65cdf36 100644 --- a/crates/accshift-core/src/config.rs +++ b/crates/accshift-core/src/config.rs @@ -3,10 +3,25 @@ use serde::{Deserialize, Serialize}; use std::collections::BTreeMap; use std::fs; +// Every window measurement here is in LOGICAL pixels, which is what +// `WebviewWindowBuilder::inner_size` and `::position` consume. Callers that +// read a live window get physical pixels and must divide by the scale factor +// first (see `logical_from_physical`), or the window grows by that factor at +// every launch on a scaled display. pub const DEFAULT_WINDOW_WIDTH: f64 = 1000.0; pub const DEFAULT_WINDOW_HEIGHT: f64 = 520.0; pub const MIN_WINDOW_WIDTH: f64 = 400.0; pub const MIN_WINDOW_HEIGHT: f64 = 300.0; +/// Upper bound on a restored window, in logical pixels. Windows itself refuses +/// to create a window wider or taller than this, so a config claiming more is +/// corrupt whatever produced it. +pub const MAX_WINDOW_WIDTH: f64 = 16_384.0; +pub const MAX_WINDOW_HEIGHT: f64 = 16_384.0; +/// Upper bound on a restored window origin, in logical pixels. A multi-monitor +/// desktop can put a window at a negative coordinate, so this bounds the +/// magnitude and not the sign. Whether the saved spot is still on a monitor is +/// a question only the GUI can answer. +const MAX_WINDOW_ORIGIN: f64 = 32_768.0; const WINDOW_SIZE_EPSILON: f64 = 1.0; #[derive(Debug, Serialize, Deserialize, Default, Clone)] @@ -282,6 +297,13 @@ pub struct AppConfig { pub window_width: Option, #[serde(default)] pub window_height: Option, + /// Window origin in logical pixels. Absent means "no saved placement", and + /// the GUI centers the window, which is also what every config written + /// before this field existed says. + #[serde(default)] + pub window_x: Option, + #[serde(default)] + pub window_y: Option, } #[derive(Debug, Serialize, Deserialize, Default)] @@ -317,6 +339,10 @@ struct RawAppConfig { window_width: Option, #[serde(default)] window_height: Option, + #[serde(default)] + window_x: Option, + #[serde(default)] + window_y: Option, } #[derive(Debug, Serialize, Deserialize, Default, Clone)] @@ -526,6 +552,8 @@ fn normalize_config(raw: RawAppConfig) -> AppConfig { telemetry, window_width: raw.window_width, window_height: raw.window_height, + window_x: raw.window_x, + window_y: raw.window_y, } } @@ -752,6 +780,7 @@ fn save_config_unlocked(app_handle: &dyn AppContext, config: &AppConfig) -> Resu "jagexAccounts": config.jagex.accounts.len(), "discordAccounts": config.discord.accounts.len(), "hasWindowSize": config.window_width.is_some() && config.window_height.is_some(), + "hasWindowPosition": config.window_x.is_some() && config.window_y.is_some(), }) .to_string(); let _ = crate::logging::append_app_log( @@ -849,20 +878,54 @@ pub fn migrate_legacy_config(app_handle: &dyn AppContext) -> Option f64 { + if !scale_factor.is_finite() || scale_factor <= 0.0 { + return physical; + } + physical / scale_factor +} + +/// The saved size as the window builder should get it, or `None` when there is +/// nothing usable to restore. +/// +/// A size at the minimum is treated as a bug rather than a preference (a window +/// collapsed by a runtime glitch), and anything past the maximum is a corrupt +/// file: clamping it keeps the window reachable instead of opening it off +/// screen or failing to open at all. +pub fn clamp_window_size(width: f64, height: f64) -> Option<(f64, f64)> { + if !width.is_finite() || !height.is_finite() || width <= 0.0 || height <= 0.0 { + return None; + } + if is_suspicious_min_window_size(width, height) { + return None; + } + Some(( + width.clamp(MIN_WINDOW_WIDTH, MAX_WINDOW_WIDTH), + height.clamp(MIN_WINDOW_HEIGHT, MAX_WINDOW_HEIGHT), + )) +} + +/// The saved origin, or `None` when it is missing or nonsense. The caller still +/// has to check it against the monitors actually attached today. +pub fn clamp_window_position(x: f64, y: f64) -> Option<(f64, f64)> { + let sane = x.is_finite() + && y.is_finite() + && x.abs() <= MAX_WINDOW_ORIGIN + && y.abs() <= MAX_WINDOW_ORIGIN; + sane.then_some((x, y)) +} + pub fn load_window_size(app_handle: &dyn AppContext) -> Option<(f64, f64)> { let cfg = load_config(app_handle); - let width = cfg.window_width?; - let height = cfg.window_height?; - if width.is_finite() - && height.is_finite() - && width > 0.0 - && height > 0.0 - && !is_suspicious_min_window_size(width, height) - { - Some((width, height)) - } else { - None - } + clamp_window_size(cfg.window_width?, cfg.window_height?) +} + +pub fn load_window_position(app_handle: &dyn AppContext) -> Option<(f64, f64)> { + let cfg = load_config(app_handle); + clamp_window_position(cfg.window_x?, cfg.window_y?) } pub fn save_window_size( @@ -870,17 +933,36 @@ pub fn save_window_size( width: f64, height: f64, ) -> Result<(), String> { - if !width.is_finite() || !height.is_finite() || width <= 0.0 || height <= 0.0 { - return Ok(()); - } + save_window_geometry(app_handle, width, height, None) +} - if is_suspicious_min_window_size(width, height) { +/// Persist the window geometry, in logical pixels. +/// +/// A `None` position leaves the stored placement alone, so a caller that only +/// knows the size never erases where the user put the window. A value that +/// fails validation is dropped rather than written, and a call where nothing +/// survives validation touches no file at all. +pub fn save_window_geometry( + app_handle: &dyn AppContext, + width: f64, + height: f64, + position: Option<(f64, f64)>, +) -> Result<(), String> { + let size = clamp_window_size(width, height); + let position = position.and_then(|(x, y)| clamp_window_position(x, y)); + if size.is_none() && position.is_none() { return Ok(()); } update_config(app_handle, |cfg| { - cfg.window_width = Some(width); - cfg.window_height = Some(height); + if let Some((width, height)) = size { + cfg.window_width = Some(width); + cfg.window_height = Some(height); + } + if let Some((x, y)) = position { + cfg.window_x = Some(x); + cfg.window_y = Some(y); + } }) } @@ -938,6 +1020,8 @@ fn portable_config(config: &AppConfig) -> AppConfig { portable.telemetry.anonymous_id.clear(); portable.window_width = None; portable.window_height = None; + portable.window_x = None; + portable.window_y = None; for account in &mut portable.roblox.accounts { account.cookie_encrypted.clear(); } @@ -1051,6 +1135,8 @@ fn local_config(config: &AppConfig) -> AppConfig { local.telemetry.onboarding_completed = false; local.window_width = config.window_width; local.window_height = config.window_height; + local.window_x = config.window_x; + local.window_y = config.window_y; local.roblox.accounts = config .roblox .accounts @@ -1102,6 +1188,8 @@ fn merge_split_configs(portable: AppConfig, mut local: AppConfig) -> AppConfig { ); overwrite_if_set(&mut merged.window_width, local.window_width); overwrite_if_set(&mut merged.window_height, local.window_height); + overwrite_if_set(&mut merged.window_x, local.window_x); + overwrite_if_set(&mut merged.window_y, local.window_y); for local_account in local.roblox.accounts { if local_account.user_id.trim().is_empty() { @@ -1288,6 +1376,108 @@ mod tests { let _ = std::fs::remove_dir_all(&ctx.root); } + // Regression for the launch-over-launch growth: the window reports a + // physical size, the builder consumes logical pixels, so a config that + // stored the physical number grew the window by the scale factor every + // time. The saver converts once, and the round trip is an identity at any + // scale. + #[test] + fn window_size_round_trips_in_logical_pixels_at_scale_1_5() { + let _test_guard = config_io_test_mutex() + .lock() + .unwrap_or_else(|e| e.into_inner()); + let ctx = tmp_ctx("window-size-dpi"); + save_config(&*ctx, &AppConfig::default()).unwrap(); + + let scale = 1.5_f64; + let (logical_width, logical_height) = (DEFAULT_WINDOW_WIDTH, DEFAULT_WINDOW_HEIGHT); + // What the window would report on a 150% display. + let physical_width = logical_width * scale; + let physical_height = logical_height * scale; + + save_window_geometry( + &*ctx, + logical_from_physical(physical_width, scale), + logical_from_physical(physical_height, scale), + None, + ) + .unwrap(); + + assert_eq!( + load_window_size(&*ctx), + Some((logical_width, logical_height)), + "a saved size must come back unchanged, not scaled" + ); + + // Second launch: the restored size is what the window is built with, so + // feeding it back through the same path must not move either. + let (restored_width, restored_height) = load_window_size(&*ctx).unwrap(); + save_window_geometry( + &*ctx, + logical_from_physical(restored_width * scale, scale), + logical_from_physical(restored_height * scale, scale), + None, + ) + .unwrap(); + assert_eq!( + load_window_size(&*ctx), + Some((logical_width, logical_height)) + ); + + let _ = std::fs::remove_dir_all(&ctx.root); + } + + #[test] + fn window_size_is_clamped_to_something_openable() { + assert_eq!(clamp_window_size(f64::NAN, 600.0), None); + assert_eq!(clamp_window_size(0.0, 600.0), None); + // A window collapsed to the minimum is a glitch, not a preference. + assert_eq!(clamp_window_size(MIN_WINDOW_WIDTH, MIN_WINDOW_HEIGHT), None); + assert_eq!( + clamp_window_size(1.0e9, 1.0e9), + Some((MAX_WINDOW_WIDTH, MAX_WINDOW_HEIGHT)) + ); + assert_eq!( + clamp_window_size(200.0, 4000.0), + Some((MIN_WINDOW_WIDTH, 4000.0)) + ); + assert_eq!(clamp_window_size(1280.0, 720.0), Some((1280.0, 720.0))); + } + + #[test] + fn window_position_round_trips_and_survives_a_missing_field() { + let _test_guard = config_io_test_mutex() + .lock() + .unwrap_or_else(|e| e.into_inner()); + let ctx = tmp_ctx("window-position"); + save_config(&*ctx, &AppConfig::default()).unwrap(); + + // A config written before the field existed means "center me". + assert_eq!(load_window_position(&*ctx), None); + + // A second monitor to the left gives a negative origin, which is valid. + save_window_geometry(&*ctx, 1280.0, 720.0, Some((-1920.0, 240.0))).unwrap(); + assert_eq!(load_window_position(&*ctx), Some((-1920.0, 240.0))); + + // A size-only save must not erase the placement. + save_window_size(&*ctx, 1000.0, 600.0).unwrap(); + assert_eq!(load_window_position(&*ctx), Some((-1920.0, 240.0))); + assert_eq!(load_window_size(&*ctx), Some((1000.0, 600.0))); + + let _ = std::fs::remove_dir_all(&ctx.root); + } + + #[test] + fn a_nonsense_window_position_is_refused() { + assert_eq!(clamp_window_position(f64::NAN, 0.0), None); + assert_eq!(clamp_window_position(0.0, f64::INFINITY), None); + assert_eq!(clamp_window_position(1.0e9, 0.0), None); + assert_eq!( + clamp_window_position(-1920.0, -80.0), + Some((-1920.0, -80.0)) + ); + } + #[test] fn normalize_config_migrates_legacy_steam_fields() { let raw = RawAppConfig { @@ -1408,6 +1598,8 @@ mod tests { telemetry: TelemetryConfig::default(), window_width: Some(1200.0), window_height: Some(800.0), + window_x: Some(120.0), + window_y: Some(64.0), }; let p = portable_config(&config); @@ -1427,6 +1619,8 @@ mod tests { assert!(p.jagex.path_override.is_empty()); assert!(p.window_width.is_none()); assert!(p.window_height.is_none()); + assert!(p.window_x.is_none()); + assert!(p.window_y.is_none()); // Roblox cookies stripped assert!(p.roblox.accounts[0].cookie_encrypted.is_empty()); @@ -1495,6 +1689,8 @@ mod tests { telemetry: TelemetryConfig::default(), window_width: Some(1024.0), window_height: Some(768.0), + window_x: Some(-1920.0), + window_y: Some(40.0), }; let l = local_config(&config); @@ -1511,6 +1707,8 @@ mod tests { assert_eq!(l.jagex.path_override, "C:\\Jagex"); assert_eq!(l.window_width, Some(1024.0)); assert_eq!(l.window_height, Some(768.0)); + assert_eq!(l.window_x, Some(-1920.0)); + assert_eq!(l.window_y, Some(40.0)); // Roblox local keeps user_id + cookie, but not username/display_name assert_eq!(l.roblox.accounts.len(), 1); diff --git a/crates/accshift-core/src/lib.rs b/crates/accshift-core/src/lib.rs index 9aaaa2e..5bd7c06 100644 --- a/crates/accshift-core/src/lib.rs +++ b/crates/accshift-core/src/lib.rs @@ -8,6 +8,7 @@ pub mod logging; pub mod os; pub mod platforms; pub mod runtime; +pub mod secrets; pub mod snapshot_crypto; pub mod storage; pub mod telemetry; diff --git a/crates/accshift-core/src/logging.rs b/crates/accshift-core/src/logging.rs index 2b8864c..a044616 100644 --- a/crates/accshift-core/src/logging.rs +++ b/crates/accshift-core/src/logging.rs @@ -22,7 +22,7 @@ use std::collections::HashMap; use std::fs::{self, File, OpenOptions}; use std::io::{Read, Seek, SeekFrom, Write}; use std::path::{Path, PathBuf}; -use std::sync::{Mutex, OnceLock}; +use std::sync::{Mutex, OnceLock, TryLockError}; use std::time::{SystemTime, UNIX_EPOCH}; const LOG_FILE_NAME: &str = "app.log"; @@ -172,6 +172,37 @@ fn with_sink( ) -> Result { let path = log_file_path(app_handle)?; let mut map = sinks().lock().unwrap_or_else(|error| error.into_inner()); + run_on_sink(&mut map, path, job) +} + +/// [`with_sink`] that gives up rather than wait for the mutex. +/// +/// `Ok(None)` means another writer holds it right now. The panic hook is the +/// only caller: a panic raised inside `with_sink` (a rotation failing to +/// rename, say) runs the hook on the very thread that owns the mutex, and +/// `std::sync::Mutex` is not reentrant, so waiting there hangs the process +/// instead of letting it crash. +fn try_with_sink( + app_handle: &dyn AppContext, + job: impl FnOnce(&Path, &mut Sink) -> Result, +) -> Result, String> { + let path = log_file_path(app_handle)?; + let mut map = match sinks().try_lock() { + Ok(map) => map, + // A poisoned mutex only means an earlier writer panicked mid-record. + // The map itself is still sound, and dropping this record helps nobody. + Err(TryLockError::Poisoned(error)) => error.into_inner(), + Err(TryLockError::WouldBlock) => return Ok(None), + }; + run_on_sink(&mut map, path, job).map(Some) +} + +/// The body both entry points share, once the mutex is theirs. +fn run_on_sink( + map: &mut HashMap, + path: PathBuf, + job: impl FnOnce(&Path, &mut Sink) -> Result, +) -> Result { let sink = map.entry(path.clone()).or_default(); sink.ensure_lock_file(&path); @@ -375,15 +406,24 @@ fn purge(current: &Path) -> Purged { /// legacy or structured, goes through here and is therefore subject to the same /// lock, the same rotation and the same budget. pub(crate) fn write_line(app_handle: &dyn AppContext, line: &str) -> Result<(), String> { - with_sink(app_handle, |path, sink| { - sink.open_if_needed(path)?; - // Rotate before the write that would breach the cap, never after: the - // announced budget is a ceiling, not an average. - if sink.size > 0 && sink.size + line.len() as u64 + 1 > MAX_LOG_FILE_BYTES { - rotate(path, sink, "size")?; - } - sink.append(path, line) - }) + with_sink(app_handle, |path, sink| append_line(path, sink, line)) +} + +/// [`write_line`] that skips the record instead of waiting for the sink mutex. +/// `Ok(false)` means it was skipped. See [`try_with_sink`] for why the panic +/// hook cannot afford to wait. +pub(crate) fn try_write_line(app_handle: &dyn AppContext, line: &str) -> Result { + Ok(try_with_sink(app_handle, |path, sink| append_line(path, sink, line))?.is_some()) +} + +fn append_line(path: &Path, sink: &mut Sink, line: &str) -> Result<(), String> { + sink.open_if_needed(path)?; + // Rotate before the write that would breach the cap, never after: the + // announced budget is a ceiling, not an average. + if sink.size > 0 && sink.size + line.len() as u64 + 1 > MAX_LOG_FILE_BYTES { + rotate(path, sink, "size")?; + } + sink.append(path, line) } /// Start a session: rotate the previous one out of the way, then say so. @@ -456,15 +496,31 @@ pub fn append_app_log( message: &str, details: Option<&str>, ) -> Result<(), String> { - let record = serde_json::json!({ + write_line(app_handle, &app_log_record(level, source, message, details)) +} + +/// [`append_app_log`] for a caller that must never block on the sink mutex. +/// `Ok(false)` means the record was skipped; the caller is expected to have a +/// fallback (the panic hook writes to stderr). +pub fn try_append_app_log( + app_handle: &dyn AppContext, + level: &str, + source: &str, + message: &str, + details: Option<&str>, +) -> Result { + try_write_line(app_handle, &app_log_record(level, source, message, details)) +} + +fn app_log_record(level: &str, source: &str, message: &str, details: Option<&str>) -> String { + serde_json::json!({ "tsMs": now_unix_ms(), "level": trim_text(&sanitize_log_text(level), 32), "source": trim_text(&sanitize_log_text(source), 128), "message": trim_text(&sanitize_log_text(message), MAX_MESSAGE_BYTES), "details": details.map(|value| trim_text(&sanitize_log_text(value), MAX_DETAILS_BYTES)), - }); - - write_line(app_handle, &record.to_string()) + }) + .to_string() } pub fn install_panic_hook(app_handle: crate::AppCtx) { @@ -490,13 +546,21 @@ pub fn install_panic_hook(app_handle: crate::AppCtx) { "unknown panic payload".to_string() }; - let _ = append_app_log( + // Never `append_app_log` here. A panic raised while the sink mutex is + // held reaches this hook on the thread that owns it, and waiting on a + // non-reentrant mutex the thread already holds deadlocks the process + // instead of crashing it. Skip the record and say so on stderr. + let logged = try_append_app_log( &*app_handle, "error", "rust.panic", &payload, Some(&location), - ); + ) + .unwrap_or(false); + if !logged { + eprintln!("panic at {location}: {payload} (log sink unavailable)"); + } previous_hook(panic_info); })); @@ -517,6 +581,45 @@ mod tests { .collect() } + // The panic hook runs on whatever thread panicked, which may be the thread + // already inside `with_sink`. Holding the mutex here reproduces that + // without a real panic: the hook path must report "not written" instead of + // waiting on a mutex it would never get back. + #[test] + fn the_panic_hook_path_skips_a_held_sink() { + let ctx = TestCtx::ctx("logging-panic-hook-try-lock"); + let path = log_file_path(&*ctx).expect("path"); + + let held = sinks().lock().unwrap_or_else(|error| error.into_inner()); + let wrote = try_append_app_log(&*ctx, "error", "rust.panic", "held", None); + drop(held); + + assert_eq!(wrote, Ok(false), "a held sink must not block the hook"); + assert!( + read_lines(&path).is_empty(), + "the skipped record wrote nothing" + ); + + // The same call with nothing held takes the record. Retried because + // every other logging test in this binary locks the same map, so one + // attempt can lose the race with a test running beside this one. + let mut wrote = Ok(false); + for _ in 0..1_000 { + wrote = try_append_app_log(&*ctx, "error", "rust.panic", "free", None); + if wrote == Ok(true) { + break; + } + std::thread::yield_now(); + } + assert_eq!(wrote, Ok(true), "an unheld sink still takes the record"); + + let messages: Vec = read_lines(&path) + .iter() + .map(|record| record["message"].as_str().unwrap_or_default().to_string()) + .collect(); + assert_eq!(messages, vec!["free".to_string()]); + } + // The 38 existing call sites still write this shape, and external readers // (support, the user's own grep) already know it. #[test] diff --git a/crates/accshift-core/src/os/steam_registry.rs b/crates/accshift-core/src/os/steam_registry.rs index 940f81f..146fd00 100644 --- a/crates/accshift-core/src/os/steam_registry.rs +++ b/crates/accshift-core/src/os/steam_registry.rs @@ -30,8 +30,8 @@ pub fn set_auto_login_user(path: &Path, username: &str) -> Result<(), AppError> Err(e) if e.kind() == std::io::ErrorKind::NotFound => empty_registry_vdf(), Err(e) => return Err(AppError::FileRead(e.to_string())), }; - let updated = vdf_set_nested_value(&existing, REGISTRY_PATH, username); - let updated = vdf_set_nested_value(&updated, REMEMBER_PATH, "1"); + let updated = vdf_set_nested_value(&existing, REGISTRY_PATH, username)?; + let updated = vdf_set_nested_value(&updated, REMEMBER_PATH, "1")?; crate::storage::write_bytes_atomic(path, updated.as_bytes()) .map_err(|e| AppError::RegistryWrite(describe_write_error(path, e))) } @@ -42,7 +42,7 @@ pub fn clear_auto_login_user(path: &Path) -> Result<(), AppError> { Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(()), Err(e) => return Err(AppError::FileRead(e.to_string())), }; - let updated = vdf_set_nested_value(&existing, REGISTRY_PATH, ""); + let updated = vdf_set_nested_value(&existing, REGISTRY_PATH, "")?; crate::storage::write_bytes_atomic(path, updated.as_bytes()) .map_err(|e| AppError::RegistryWrite(describe_write_error(path, e))) } diff --git a/crates/accshift-core/src/platforms/battle_net.rs b/crates/accshift-core/src/platforms/battle_net.rs index f4213fe..e612d22 100644 --- a/crates/accshift-core/src/platforms/battle_net.rs +++ b/crates/accshift-core/src/platforms/battle_net.rs @@ -380,15 +380,59 @@ fn known_account_emails(app_handle: &dyn AppContext) -> Result, Stri } fn read_accounts(app_handle: &dyn AppContext) -> Result, String> { - let saved_accounts = read_saved_accounts()?; - if let Some(current_email) = saved_accounts.first() { - let _ = remember_account_usage(app_handle, current_email, true); + list_accounts_from_saved(app_handle, read_saved_accounts()?) +} + +/// Build the account list from what the launcher saved plus what our own +/// config already knows. +/// +/// Listing is a read. It used to call `remember_account_usage` for the first +/// saved account, which took the cross-process config lock and stamped +/// `last_used_at` with "now" on every poll, so "last used" meant "last listed" +/// and the frontend's sort order was noise. The only write left is registering +/// an email the launcher knows and our config does not, and it carries no +/// timestamp: an account we have never seen used has no usage to report. +fn list_accounts_from_saved( + app_handle: &dyn AppContext, + saved_accounts: Vec, +) -> Result, String> { + let mut cfg = config::load_config(app_handle); + + // The one write listing is allowed, and only when the launcher shows an + // account our config has never seen. Everything below is a read. + let newcomers = unknown_emails(&cfg, &saved_accounts); + if !newcomers.is_empty() { + let current_key = saved_accounts + .first() + .map(|email| normalize_account_key(email)); + // The tag lives in the client's cache under the id of the account that + // is signed in, so it can only be claimed for that one, and only while + // it is the account being registered. + let current_tag = current_key + .as_ref() + .filter(|key| { + newcomers + .iter() + .any(|email| &normalize_account_key(email) == *key) + }) + .and_then(|_| current_battle_tag_from_cache().ok().flatten()); + + add_new_accounts( + &mut cfg, + &newcomers, + current_key.as_deref(), + current_tag.as_deref(), + ); + config::update_config(app_handle, |stored| { + add_new_accounts( + stored, + &newcomers, + current_key.as_deref(), + current_tag.as_deref(), + ); + })?; } - // Load after `remember_account_usage`: it bumps last_used_at and may store - // a freshly fetched battle tag, so a config read before it would report - // stale metadata for the current account. - let cfg = config::load_config(app_handle); let account_emails = known_account_emails_from(saved_accounts, &cfg); let metadata_by_key = cfg .battle_net @@ -420,6 +464,66 @@ fn read_accounts(app_handle: &dyn AppContext) -> Result, S .collect()) } +/// The launcher accounts our own config does not hold yet, deduplicated, in +/// the order the launcher lists them. An empty result means listing has +/// nothing to write and takes no lock. +fn unknown_emails(cfg: &AppConfig, saved_accounts: &[String]) -> Vec { + let mut known = cfg + .battle_net + .accounts + .iter() + .map(|account| normalize_account_key(&account.email)) + .collect::>(); + + saved_accounts + .iter() + .filter_map(|email| { + let email = email.trim().to_string(); + if email.is_empty() || !known.insert(normalize_account_key(&email)) { + return None; + } + Some(email) + }) + .collect() +} + +/// Record accounts the launcher knows and we do not, with no `last_used_at`: +/// seeing an account is not using it, and a listing that stamped one would +/// make "last used" mean "last listed". Only the account that is signed in +/// gets the battle tag, and only if the caller could read it. +/// +/// Re-checks what the config holds, because the copy this runs on inside +/// `update_config` is re-read under the lock and may already have the account. +fn add_new_accounts( + cfg: &mut AppConfig, + emails: &[String], + current_key: Option<&str>, + current_tag: Option<&str>, +) { + let mut known = cfg + .battle_net + .accounts + .iter() + .map(|account| normalize_account_key(&account.email)) + .collect::>(); + + for email in emails { + let key = normalize_account_key(email); + if !known.insert(key.clone()) { + continue; + } + let is_current = current_key == Some(key.as_str()); + cfg.battle_net.accounts.push(BattleNetAccountConfig { + email: email.clone(), + battle_tag: match current_tag { + Some(tag) if is_current => tag.to_string(), + _ => String::new(), + }, + last_used_at: None, + }); + } +} + fn current_account(accounts: &[BattleNetAccount]) -> String { accounts .first() @@ -1407,3 +1511,221 @@ mod tests { let _ = std::fs::remove_dir_all(&root); } } + +/// Listing the accounts is a poll: the frontend calls it on every refresh, so +/// what it writes (and what it must not write) is the whole point here. +#[cfg(test)] +mod listing_tests { + use super::*; + use crate::config::AppConfig; + use std::path::PathBuf; + + struct TempCtx { + root: PathBuf, + } + + impl AppContext for TempCtx { + fn app_config_dir(&self) -> Result { + Ok(self.root.clone()) + } + fn app_data_dir(&self) -> Result { + Ok(self.root.clone()) + } + fn app_local_data_dir(&self) -> Result { + Ok(self.root.clone()) + } + fn app_cache_dir(&self) -> Result { + Ok(self.root.clone()) + } + } + + /// The config cache and the poisoned-local flag are process-global, so + /// every test that writes a config takes the same lock as `config`'s own. + fn config_guard() -> std::sync::MutexGuard<'static, ()> { + crate::config::config_io_test_mutex() + .lock() + .unwrap_or_else(|e| e.into_inner()) + } + + fn scratch(tag: &str) -> PathBuf { + let root = std::env::temp_dir().join(format!( + "accshift-battlenet-listing-{}-{}-{:?}", + tag, + std::process::id(), + std::thread::current().id() + )); + let _ = fs::remove_dir_all(&root); + fs::create_dir_all(&root).unwrap(); + root + } + + /// Bytes of both config files, which is what a listing must leave alone. + fn config_bytes(ctx: &TempCtx) -> Vec<(PathBuf, Vec)> { + [ + crate::storage::portable_config_path(ctx).unwrap(), + crate::storage::local_config_path(ctx).unwrap(), + ] + .into_iter() + .map(|path| { + let bytes = fs::read(&path).unwrap_or_default(); + (path, bytes) + }) + .collect() + } + + fn seed_account(ctx: &TempCtx, email: &str, last_used_at: Option) { + config::update_config(ctx, |cfg| { + cfg.battle_net.accounts.push(BattleNetAccountConfig { + email: email.to_string(), + battle_tag: "Seeded#0001".into(), + last_used_at, + }); + }) + .unwrap(); + } + + #[test] + fn listing_twice_writes_nothing_and_keeps_the_previous_last_used_at() { + // The finding: listing called `remember_account_usage`, which took the + // cross-process lock and stamped `last_used_at` with "now" on every + // poll, so "last used" was really "last listed". + let _config = config_guard(); + let root = scratch("no-write"); + let ctx = TempCtx { root: root.clone() }; + seed_account(&ctx, "one@example.com", Some(1_000)); + let before = config_bytes(&ctx); + + let saved = vec!["one@example.com".to_string()]; + let first = list_accounts_from_saved(&ctx, saved.clone()).unwrap(); + let second = list_accounts_from_saved(&ctx, saved).unwrap(); + + assert_eq!(first.len(), 1); + assert_eq!(first[0].last_login_at, Some(1_000)); + assert_eq!(second[0].last_login_at, Some(1_000)); + assert_eq!(config_bytes(&ctx), before, "listing rewrote the config"); + let _ = fs::remove_dir_all(&root); + } + + #[test] + fn a_newly_discovered_email_is_recorded_without_a_timestamp() { + let _config = config_guard(); + let root = scratch("discovery"); + let ctx = TempCtx { root: root.clone() }; + seed_account(&ctx, "known@example.com", Some(1_000)); + + // The known account stays first, so it is still the current one and + // the newcomer never claims its battle tag. + let accounts = list_accounts_from_saved( + &ctx, + vec![ + "known@example.com".to_string(), + "fresh@example.com".to_string(), + ], + ) + .unwrap(); + + assert_eq!(accounts.len(), 2); + assert_eq!(accounts[1].email, "fresh@example.com"); + assert_eq!(accounts[1].last_login_at, None); + assert_eq!(accounts[0].last_login_at, Some(1_000)); + + // And it is persisted, so the next listing has nothing to write. + let stored = config::load_config(&ctx); + let fresh = stored + .battle_net + .accounts + .iter() + .find(|account| account.email == "fresh@example.com") + .expect("the discovered account is in the config"); + assert_eq!(fresh.last_used_at, None); + assert!(fresh.battle_tag.is_empty()); + + let before = config_bytes(&ctx); + let _ = list_accounts_from_saved( + &ctx, + vec![ + "known@example.com".to_string(), + "fresh@example.com".to_string(), + ], + ) + .unwrap(); + assert_eq!(config_bytes(&ctx), before); + let _ = fs::remove_dir_all(&root); + } + + #[test] + fn a_use_still_stamps_the_account() { + // What `switch_account` and setup completion call. Listing does not. + let _config = config_guard(); + let root = scratch("stamp"); + let ctx = TempCtx { root: root.clone() }; + seed_account(&ctx, "one@example.com", Some(1_000)); + + remember_account_usage(&ctx, "one@example.com", false).unwrap(); + + let stored = config::load_config(&ctx); + let stamped = stored.battle_net.accounts[0].last_used_at.unwrap(); + assert!(stamped > 1_000, "last_used_at was not refreshed: {stamped}"); + let _ = fs::remove_dir_all(&root); + } + + #[test] + fn unknown_emails_only_reports_what_the_config_is_missing() { + let mut cfg = AppConfig::default(); + cfg.battle_net.accounts.push(BattleNetAccountConfig { + email: "Known@Example.com".into(), + battle_tag: String::new(), + last_used_at: Some(7), + }); + + // Case-insensitive, and the same newcomer listed twice counts once. + assert_eq!( + unknown_emails( + &cfg, + &[ + "known@example.com".to_string(), + " ".to_string(), + " fresh@example.com ".to_string(), + "FRESH@example.com".to_string(), + ] + ), + vec!["fresh@example.com".to_string()] + ); + assert!(unknown_emails(&cfg, &["KNOWN@EXAMPLE.COM".to_string()]).is_empty()); + } + + #[test] + fn only_the_signed_in_newcomer_takes_the_battle_tag() { + let mut cfg = AppConfig::default(); + let emails = vec![ + "current@example.com".to_string(), + "other@example.com".to_string(), + ]; + + add_new_accounts( + &mut cfg, + &emails, + Some("current@example.com"), + Some("Tag#1234"), + ); + + assert_eq!(cfg.battle_net.accounts.len(), 2); + assert_eq!(cfg.battle_net.accounts[0].battle_tag, "Tag#1234"); + assert!(cfg.battle_net.accounts[1].battle_tag.is_empty()); + assert!(cfg + .battle_net + .accounts + .iter() + .all(|account| account.last_used_at.is_none())); + + // Running again adds nothing: this is what makes the write under the + // lock safe when the stored config already moved on. + add_new_accounts( + &mut cfg, + &emails, + Some("current@example.com"), + Some("Tag#1234"), + ); + assert_eq!(cfg.battle_net.accounts.len(), 2); + } +} diff --git a/crates/accshift-core/src/platforms/descriptor/descriptors/discord.json b/crates/accshift-core/src/platforms/descriptor/descriptors/discord.json index 7908d50..722bd00 100644 --- a/crates/accshift-core/src/platforms/descriptor/descriptors/discord.json +++ b/crates/accshift-core/src/platforms/descriptor/descriptors/discord.json @@ -46,22 +46,26 @@ "live": "${APPDATA}/discord/Local Storage/leveldb", "snapshot": "local_storage_leveldb", "clearOnSetup": true, - "snapshotMarker": true + "snapshotMarker": true, + "clearSnapshotWhenSourceMissing": false }, { "live": "${APPDATA}/discord/Session Storage", "snapshot": "session_storage", - "clearOnSetup": true + "clearOnSetup": true, + "clearSnapshotWhenSourceMissing": false }, { "live": "${APPDATA}/discord/Network", "snapshot": "network", - "clearOnSetup": true + "clearOnSetup": true, + "clearSnapshotWhenSourceMissing": false }, { "live": "${APPDATA}/discord/blob_storage", "snapshot": "blob_storage", - "clearOnSetup": true + "clearOnSetup": true, + "clearSnapshotWhenSourceMissing": false } ], "captureWhen": [ diff --git a/crates/accshift-core/src/platforms/descriptor/descriptors/gog.json b/crates/accshift-core/src/platforms/descriptor/descriptors/gog.json index 611e6c6..ee9377c 100644 --- a/crates/accshift-core/src/platforms/descriptor/descriptors/gog.json +++ b/crates/accshift-core/src/platforms/descriptor/descriptors/gog.json @@ -77,12 +77,14 @@ { "live": "${ProgramData}/GOG.com/Galaxy/webcache/common", "snapshot": "webcache-common", - "clearOnSetup": true + "clearOnSetup": true, + "clearSnapshotWhenSourceMissing": false }, { "live": "${ProgramData}/GOG.com/Galaxy/storage", "snapshot": "storage", - "clearOnSetup": true + "clearOnSetup": true, + "clearSnapshotWhenSourceMissing": false } ] }, diff --git a/crates/accshift-core/src/platforms/descriptor/engine.rs b/crates/accshift-core/src/platforms/descriptor/engine.rs index 30fc463..663e4c8 100644 --- a/crates/accshift-core/src/platforms/descriptor/engine.rs +++ b/crates/accshift-core/src/platforms/descriptor/engine.rs @@ -158,7 +158,10 @@ impl DescriptorService { } } } - let sandbox = Sandbox::new(&profile.roots, &resolver); + // A root that does not resolve here stops the operation. Every path the + // steps below build is checked against these, so carrying on with a + // half-built sandbox would mean carrying on with no sandbox. + let sandbox = Sandbox::new(&profile.roots, &resolver).map_err(|e| e.to_string())?; Ok(Runtime { profile, resolver, @@ -593,6 +596,18 @@ impl DescriptorService { for item in &runtime.profile.state.directories { let live = runtime.spec_path(&item.live)?; let dest = cache_dir.join(&item.snapshot); + if !live.is_dir() && !item.clear_snapshot_when_source_missing { + // The launcher has not written this directory yet. Copying + // nothing over the previous capture would leave the account + // with an empty snapshot, which restores as a signed-out + // session. + continue; + } + // On Linux and macOS every encrypted file in there owns a keyring + // entry, and removing the directory is the only thing that still + // knows the entry ids. Free them first or they leak, one full + // capture's worth per switch. + free_dir_secrets(&dest); let _ = fs::remove_dir_all(&dest); let ignored: Vec<&str> = item.ignored_names.iter().map(String::as_str).collect(); snapshot_crypto::encrypted_copy_dir( @@ -2219,6 +2234,155 @@ mod tests { let _ = fs::remove_dir_all(&root); } + /// The variable a root is written against on the OS this build targets. + /// Each one is a placeholder the loader accepts there, and none of them is + /// in the fabricated environment the test hands the service. + const ROOT_VAR: &str = if cfg!(windows) { + "LOCALAPPDATA" + } else { + "HOME" + }; + + /// A descriptor whose only root is written against a variable the caller + /// can leave out of the environment. + fn env_rooted_fixture() -> String { + fn profile(var: &str) -> String { + format!( + r#"{{ + "roots": {{ "files": ["${{{var}}}/Demo"] }}, + "detect": {{ "pathExists": ["${{{var}}}/Demo"] }}, + "identity": {{ + "source": {{ "kind": "synthetic" }}, + "format": {{ "charset": "alphanumeric", "maxLength": 64 }}, + "current": "config" + }}, + "state": {{ + "files": [ + {{ "live": "${{{var}}}/Demo/session.json", "snapshot": "session.json", "snapshotMarker": true }} + ] + }}, + "close": {{ "processes": ["nothing-here"] }}, + "setup": {{ "missingSnapshotHint": "Add this account through setup first." }} + }}"# + ) + } + format!( + r#"{{ + "id": "gog", + "schemaVersion": 1, + "name": "Test Launcher", + "shortName": "Test", + "os": {{ + "windows": {}, + "linux": {}, + "macos": {} + }} + }}"#, + profile("LOCALAPPDATA"), + profile("HOME"), + profile("HOME") + ) + } + + #[test] + fn a_root_that_does_not_resolve_refuses_every_path_instead_of_allowing_all() { + // The sandbox used to drop a root it could not resolve, and an empty + // root list meant "allow everything". One unset variable was enough to + // let a switch write anywhere on the disk. + let _config = config_guard(); + let root = scratch("unresolved-root"); + let ctx = TempCtx { root: root.clone() }; + let descriptor = Descriptor::parse("test", &env_rooted_fixture()).unwrap(); + let service = DescriptorService::new(descriptor, DescriptorOrigin::Embedded) + .with_environment(Vec::<(String, String)>::new()); + + let err = service.save_snapshot(&ctx, "aaaa1111").unwrap_err(); + assert!(err.contains("Refused to run without a sandbox"), "{err}"); + assert!(err.contains(&format!("${{{ROOT_VAR}}}/Demo")), "{err}"); + assert!(service.plan_switch(&ctx, "aaaa1111").is_err()); + assert!(service.read_accounts(&ctx).is_err()); + let _ = fs::remove_dir_all(&root); + } + + #[test] + fn recapturing_frees_the_previous_capture_keyring_entries() { + // Every encrypted file owns a keyring entry on Linux and macOS, and the + // directory that held the ids is what gets removed. Freeing them has to + // happen before the removal or a switch leaks one entry per file. + let _config = config_guard(); + let root = scratch("keyring-growth"); + let live = root.join("live"); + let ctx = TempCtx { root: root.clone() }; + let service = service(&live); + + seed_live_session(&live, b"first"); + service.save_snapshot(&ctx, "aaaa1111").unwrap(); + let after_first = crate::secrets::backend::entry_count(); + assert_eq!(after_first, 2, "one entry per encrypted snapshot file"); + + seed_live_session(&live, b"second"); + service.save_snapshot(&ctx, "aaaa1111").unwrap(); + assert_eq!(crate::secrets::backend::entry_count(), after_first); + + // The snapshot still reads back, so nothing live was freed either. + service.restore_snapshot(&ctx, "aaaa1111").unwrap(); + assert_eq!( + fs::read(live.join("auth").join("nested").join("token.bin")).unwrap(), + b"second" + ); + let _ = fs::remove_dir_all(&root); + } + + #[test] + fn a_missing_live_directory_keeps_the_previous_snapshot_when_told_to() { + let _config = config_guard(); + let root = scratch("dir-keep"); + let live = root.join("live"); + let ctx = TempCtx { root: root.clone() }; + let json = fixture(&live).replace( + r#""snapshot": "auth", "snapshotMarker": true, "clearOnSetup": true"#, + r#""snapshot": "auth", "snapshotMarker": true, "clearOnSetup": true, "clearSnapshotWhenSourceMissing": false"#, + ); + let service = DescriptorService::new( + Descriptor::parse("test", &json).unwrap(), + DescriptorOrigin::Embedded, + ); + + seed_live_session(&live, b"first"); + service.save_snapshot(&ctx, "aaaa1111").unwrap(); + + // The launcher has not written its auth folder back yet. + fs::remove_dir_all(live.join("auth")).unwrap(); + service.save_snapshot(&ctx, "aaaa1111").unwrap(); + + let snapshot = service.snapshot_root(&ctx, "aaaa1111").unwrap(); + assert!(snapshot + .join("auth") + .join("nested") + .join("token.bin") + .exists()); + let _ = fs::remove_dir_all(&root); + } + + #[test] + fn a_missing_live_directory_drops_the_snapshot_by_default() { + let _config = config_guard(); + let root = scratch("dir-drop"); + let live = root.join("live"); + let ctx = TempCtx { root: root.clone() }; + let service = service(&live); + + seed_live_session(&live, b"first"); + service.save_snapshot(&ctx, "aaaa1111").unwrap(); + + fs::remove_dir_all(live.join("auth")).unwrap(); + service.save_snapshot(&ctx, "aaaa1111").unwrap(); + + let snapshot = service.snapshot_root(&ctx, "aaaa1111").unwrap(); + assert!(!snapshot.join("auth").exists()); + let _ = fs::remove_dir_all(&root); + } + #[test] fn clearing_the_live_state_removes_exactly_what_setup_declares() { let _config = config_guard(); diff --git a/crates/accshift-core/src/platforms/descriptor/paths.rs b/crates/accshift-core/src/platforms/descriptor/paths.rs index 02e15ec..b9ca31e 100644 --- a/crates/accshift-core/src/platforms/descriptor/paths.rs +++ b/crates/accshift-core/src/platforms/descriptor/paths.rs @@ -126,34 +126,61 @@ fn normalise_separators(path: &str) -> String { /// The set of directories a descriptor is allowed to read and write. /// -/// Built once from the resolved roots. Roots that cannot resolve on this -/// machine are dropped rather than fatal: a descriptor may declare a root for -/// a variable that only exists on another edition of the OS, and the paths -/// under it will fail to resolve on their own. -#[derive(Debug, Clone, Default)] -pub struct Sandbox { - roots: Vec, +/// Two states, and only two. Either the profile declares no file root and the +/// sandbox is explicitly unrestricted, or it declares roots and every one of +/// them resolved. An empty vector used to mean both "nothing was declared" and +/// "nothing resolved", so one missing environment variable turned the sandbox +/// off instead of stopping the operation, which is the opposite of what a +/// sandbox is for. +#[derive(Debug, Clone)] +pub enum Sandbox { + /// Allows every path, for the parts of a dry run that only report what a + /// step would touch, and for a profile that declares no file root at all. + Unrestricted, + /// Allows only paths under one of these roots. Never empty. + Restricted(Vec), } impl Sandbox { - pub fn new(roots: &Roots, resolver: &PathResolver) -> Self { - let roots = roots - .files - .iter() - .filter_map(|template| resolver.resolve(template).ok()) - .map(|path| lexically_normalise(&path)) - .collect(); - Self { roots } + /// Resolves the declared roots, refusing to build a sandbox at all when one + /// of them cannot resolve here. + /// + /// A root naming a variable this machine does not have used to be dropped, + /// on the theory that the paths under it would fail on their own. They do + /// not: dropping the last root leaves an empty list, and an empty list + /// allowed everything. + pub fn new(roots: &Roots, resolver: &PathResolver) -> Result { + if roots.files.is_empty() { + return Ok(Self::Unrestricted); + } + let mut resolved = Vec::with_capacity(roots.files.len()); + for template in &roots.files { + let path = resolver.resolve(template).map_err(|error| { + PlatformError::new( + PlatformErrorKind::Io, + format!( + "Refused to run without a sandbox: the declared folder `{}` does not resolve on this system ({})", + template.as_str(), + error.message + ), + ) + })?; + resolved.push(lexically_normalise(&path)); + } + Ok(Self::Restricted(resolved)) } /// Sandbox allowing everything, for the parts of a dry run that only need /// to report what a step would touch. pub fn unrestricted() -> Self { - Self { roots: Vec::new() } + Self::Unrestricted } pub fn roots(&self) -> &[PathBuf] { - &self.roots + match self { + Self::Unrestricted => &[], + Self::Restricted(roots) => roots, + } } /// Refuses a path that is not inside a declared root. @@ -162,14 +189,11 @@ impl Sandbox { /// separator noise are folded first, so a template resolved through an /// environment variable holding `C:\Users\x\..\y` cannot climb out. pub fn ensure_allowed(&self, path: &Path) -> Result<(), PlatformError> { - if self.roots.is_empty() { + let Self::Restricted(roots) = self else { return Ok(()); - } + }; let candidate = lexically_normalise(path); - let inside = self - .roots - .iter() - .any(|root| path_starts_with(&candidate, root)); + let inside = roots.iter().any(|root| path_starts_with(&candidate, root)); if inside { Ok(()) } else { @@ -298,7 +322,7 @@ mod tests { registry: Vec::new(), }; let resolver = resolver(); - let sandbox = Sandbox::new(&roots, &resolver); + let sandbox = Sandbox::new(&roots, &resolver).unwrap(); let inside = resolver .resolve(&template("${LOCALAPPDATA}/Demo/sub/session.json")) .unwrap(); @@ -312,7 +336,7 @@ mod tests { registry: Vec::new(), }; let resolver = resolver(); - let sandbox = Sandbox::new(&roots, &resolver); + let sandbox = Sandbox::new(&roots, &resolver).unwrap(); let outside = resolver .resolve(&template("${LOCALAPPDATA}/DemoOther/session.json")) .unwrap(); @@ -331,13 +355,15 @@ mod tests { files: vec![template("C:/Demo")], registry: Vec::new(), }; - let sandbox = Sandbox::new(&roots, &resolver); + let sandbox = Sandbox::new(&roots, &resolver).unwrap(); let escaped = resolver.resolve(&template("${SNEAKY}/config.sys")).unwrap(); assert!(sandbox.ensure_allowed(&escaped).is_err()); } #[test] - fn a_root_that_cannot_resolve_is_dropped_rather_than_fatal() { + fn a_root_that_cannot_resolve_refuses_the_whole_sandbox() { + // Dropping it used to leave the other root standing, and dropping the + // last one left an empty list that allowed every path on the disk. let roots = Roots { files: vec![ template("${LOCALAPPDATA}/Demo"), @@ -345,8 +371,23 @@ mod tests { ], registry: Vec::new(), }; - let sandbox = Sandbox::new(&roots, &resolver()); - assert_eq!(sandbox.roots().len(), 1); + let err = Sandbox::new(&roots, &resolver()).unwrap_err(); + assert!( + err.message.contains("${MISSING_ON_THIS_EDITION}/Demo"), + "{}", + err.message + ); + } + + #[test] + fn a_profile_declaring_no_file_root_is_unrestricted_on_purpose() { + let roots = Roots { + files: Vec::new(), + registry: Vec::new(), + }; + let sandbox = Sandbox::new(&roots, &resolver()).unwrap(); + assert!(matches!(sandbox, Sandbox::Unrestricted)); + assert!(sandbox.ensure_allowed(Path::new("C:/anywhere")).is_ok()); } #[test] diff --git a/crates/accshift-core/src/platforms/descriptor/schema.rs b/crates/accshift-core/src/platforms/descriptor/schema.rs index 6eaff45..30a37e7 100644 --- a/crates/accshift-core/src/platforms/descriptor/schema.rs +++ b/crates/accshift-core/src/platforms/descriptor/schema.rs @@ -483,6 +483,15 @@ pub struct DirItem { pub ignored_names: Vec, #[serde(default)] pub follow_symlinks: bool, + /// Drop a stale snapshot when the live directory is gone at capture time, + /// so a later restore cannot resurrect another account's session. + /// + /// Left off where a missing directory means the launcher has not written it + /// yet rather than the account signing out: the capture would otherwise + /// throw away the only copy the user has, and an empty copy is worse than a + /// slightly old one. + #[serde(default = "default_true")] + pub clear_snapshot_when_source_missing: bool, } #[derive(Debug, Clone, Deserialize, Serialize)] @@ -849,7 +858,7 @@ impl Descriptor { )); } for (os, profile) in &self.os { - profile.validate(source, &format!("os.{}", os.as_str()), &self.id)?; + profile.validate(source, &format!("os.{}", os.as_str()), &self.id, *os)?; } Ok(()) } @@ -878,6 +887,7 @@ impl OsProfile { source: &str, field: &str, platform_id: &str, + os: Os, ) -> Result<(), DescriptorError> { // The escape hatch is per platform, and it is checked here rather than // where the source is validated so the allowlist reads once. @@ -895,7 +905,9 @@ impl OsProfile { } for (index, root) in self.roots.files.iter().enumerate() { - root.validate(source, &format!("{field}.roots.files[{index}]"))?; + let at = format!("{field}.roots.files[{index}]"); + root.validate(source, &at)?; + validate_root_placeholders(source, &at, root, os)?; } for (index, root) in self.roots.registry.iter().enumerate() { validate_registry_key( @@ -1384,6 +1396,71 @@ fn validate_registry_key(source: &str, field: &str, key: &str) -> Result<(), Des } } +/// Placeholders a root may be written with, per OS. +/// +/// The list is deliberately short: a root is a well-known per-user or +/// machine-wide directory, plus the launcher's own install directory. Anything +/// else is either a typo or a variable only some machines carry, and since a +/// root that does not resolve now stops the whole profile rather than being +/// dropped, the descriptor is refused at load instead of on a user's machine. +fn known_root_placeholders(os: Os) -> &'static [&'static str] { + match os { + Os::Windows => &[ + INSTALL_DIR, + "APPDATA", + "LOCALAPPDATA", + "ProgramData", + "ProgramFiles", + "ProgramFiles(x86)", + "PUBLIC", + "SystemDrive", + "USERPROFILE", + ], + Os::Macos => &[INSTALL_DIR, "HOME"], + Os::Linux => &[ + INSTALL_DIR, + "HOME", + "XDG_CACHE_HOME", + "XDG_CONFIG_HOME", + "XDG_DATA_HOME", + "XDG_STATE_HOME", + ], + } +} + +/// Refuses a root written against a placeholder that has no meaning on the OS +/// the profile targets. +fn validate_root_placeholders( + source: &str, + field: &str, + root: &PathTemplate, + os: Os, +) -> Result<(), DescriptorError> { + let known = known_root_placeholders(os); + for name in root.placeholders() { + // Windows environment variable names are case-insensitive, and the + // resolver folds case when it looks one up, so the check does too. + let matches = known.iter().any(|candidate| { + if os == Os::Windows { + candidate.eq_ignore_ascii_case(&name) + } else { + *candidate == name + } + }); + if !matches { + return Err(DescriptorError::new( + source, + field, + format!( + "expected a root built from one of {}, found `${{{name}}}`", + known.join(", ") + ), + )); + } + } + Ok(()) +} + /// The sandbox check the loader can make ahead of time: a template whose /// literal text does not start with a declared root can never resolve inside /// one, whatever the environment holds. diff --git a/crates/accshift-core/src/platforms/riot.rs b/crates/accshift-core/src/platforms/riot.rs index 02e1882..a0f816c 100644 --- a/crates/accshift-core/src/platforms/riot.rs +++ b/crates/accshift-core/src/platforms/riot.rs @@ -622,50 +622,6 @@ fn decrypted_copy_dir(source: &Path, target: &Path, ignored_names: &[&str]) -> R snapshot_crypto::decrypted_copy_dir(source, target, riot_dir_copy_options(ignored_names)) } -/// Copy a file verbatim, no encryption. Used only for the transient rollback -/// backup in `restore_live_snapshot`: the source is already plaintext on disk -/// at its live location, and the backup is deleted again within the same call. -fn plain_copy_file(source: &Path, dest: &Path) -> Result<(), String> { - if let Some(parent) = dest.parent() { - fs::create_dir_all(parent) - .map_err(|e| format!("Could not create directory {}: {e}", parent.display()))?; - } - fs::copy(source, dest).map(|_| ()).map_err(|e| { - format!( - "Could not copy {} to {}: {e}", - source.display(), - dest.display() - ) - }) -} - -/// Recursively copy a directory verbatim, no encryption. See `plain_copy_file`. -fn plain_copy_dir(source: &Path, target: &Path, ignored_names: &[&str]) -> Result<(), String> { - if !source.exists() { - return Ok(()); - } - fs::create_dir_all(target) - .map_err(|e| format!("Could not create directory {}: {e}", target.display()))?; - for entry in fs::read_dir(source) - .map_err(|e| format!("Could not read directory {}: {e}", source.display()))? - { - let entry = entry.map_err(|e| format!("Could not read directory entry: {e}"))?; - let src_path = entry.path(); - let file_name = entry.file_name(); - let name = file_name.to_string_lossy(); - if ignored_names.iter().any(|i| i.eq_ignore_ascii_case(&name)) { - continue; - } - let dst_path = target.join(name.as_ref()); - if src_path.is_dir() { - plain_copy_dir(&src_path, &dst_path, ignored_names)?; - } else { - plain_copy_file(&src_path, &dst_path)?; - } - } - Ok(()) -} - /// Free the OS-keyring entries a profile's encrypted snapshot files point at. /// /// On Linux/macOS `os::encrypt_bytes` stores the real plaintext in the keyring @@ -978,14 +934,23 @@ fn clear_live_riot_setup_state(install_dir: Option<&Path>) -> Result<(), String> Ok(()) } -/// Copy every live Riot item into a fresh temp directory so a failure partway -/// through `restore_live_snapshot`'s copy loop can be rolled back instead of -/// leaving a mix of the old and new profile's data. Returns the rollback -/// directory on success; the caller must remove it once it is no longer -/// needed (on both the success and the failure path). -fn backup_live_state_for_rollback(install_dir: Option<&Path>) -> Result { +/// Copy every live Riot item into a fresh rollback directory so a failure +/// partway through `restore_live_snapshot`'s copy loop can be rolled back +/// instead of leaving a mix of the old and new profile's data. Returns the +/// rollback directory on success; the caller must discard it once it is no +/// longer needed (on both the success and the failure path), through +/// `discard_rollback_dir` so the keyring entries go with it. +/// +/// The copy is encrypted like any other snapshot and lives under the app's own +/// state directory. It used to be a plaintext copy in the system temp +/// directory, where a crash mid-restore left Riot auth tokens in the clear in a +/// world-readable place with nothing to sweep them. +fn backup_live_state_for_rollback( + app_handle: &dyn AppContext, + install_dir: Option<&Path>, +) -> Result { let rollback_dir = - std::env::temp_dir().join(format!("accshift-riot-rollback-{}", Uuid::new_v4())); + crate::storage::riot_rollback_dir(app_handle)?.join(Uuid::new_v4().to_string()); fs::create_dir_all(&rollback_dir).map_err(|e| { format!( "Could not create Riot rollback dir {}: {e}", @@ -994,24 +959,208 @@ fn backup_live_state_for_rollback(install_dir: Option<&Path>) -> Result path, + Ok(None) => continue, + Err(e) => { + discard_rollback_dir(app_handle, &rollback_dir); + return Err(e); + } }; if !source_path.exists() { continue; } let target_path = rollback_dir.join(item.snapshot_name); - match item.kind { - RiotSnapshotKind::Directory => { - plain_copy_dir(&source_path, &target_path, item.ignored_names)? - } - RiotSnapshotKind::File => plain_copy_file(&source_path, &target_path)?, + // A backup that stopped halfway is useless and would otherwise sit on + // disk holding auth material until the next launch sweeps it. + if let Err(e) = copy_item_into_rollback(&source_path, &target_path, item) { + discard_rollback_dir(app_handle, &rollback_dir); + return Err(e); } } Ok(rollback_dir) } +/// Copy one live item into the rollback directory, encrypted like any other +/// snapshot file. +fn copy_item_into_rollback( + source: &Path, + target: &Path, + item: &RiotSnapshotItem, +) -> Result<(), String> { + match item.kind { + RiotSnapshotKind::Directory => encrypted_copy_dir(source, target, item.ignored_names), + RiotSnapshotKind::File => encrypted_copy_file(source, target), + } +} + +/// Put one item from the rollback directory back at its live location, +/// decrypting it on the way. +fn restore_item_from_rollback( + source: &Path, + target: &Path, + item: &RiotSnapshotItem, +) -> Result<(), String> { + match item.kind { + RiotSnapshotKind::Directory => decrypted_copy_dir(source, target, item.ignored_names), + RiotSnapshotKind::File => decrypted_copy_file(source, target), + } +} + +/// Free the keyring entries the encrypted rollback copy points at, then remove +/// it. Called on every exit path of `restore_live_snapshot` that still runs. +fn discard_rollback_dir(app_handle: &dyn AppContext, rollback_dir: &Path) { + free_snapshot_secrets(app_handle, rollback_dir); + if let Err(e) = fs::remove_dir_all(rollback_dir) { + if e.kind() != std::io::ErrorKind::NotFound { + log_platform_error( + app_handle, + "riot.restore_rollback", + "Could not remove the Riot rollback copy", + format!("dir={} error={e}", rollback_dir.display()), + ); + } + } +} + +/// Name prefix of the plaintext rollback copies earlier builds wrote straight +/// into the system temp directory. Nothing ever swept them, so a +/// crash mid-restore left Riot auth material there until the user cleaned the +/// directory by hand. +const LEGACY_ROLLBACK_PREFIX: &str = "accshift-riot-rollback-"; + +/// Outcome of one rollback sweep. +#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] +pub struct RollbackSweepStats { + /// Rollback copies removed. + pub removed: usize, + /// Rollback copies that could not be removed. The next launch tries again. + pub failed: usize, +} + +impl RollbackSweepStats { + fn merge(&mut self, other: RollbackSweepStats) { + self.removed += other.removed; + self.failed += other.failed; + } + + /// True when the pass had anything to report. Nothing to sweep is the + /// normal case on every launch, and says nothing worth logging. + pub fn touched_anything(&self) -> bool { + self.removed > 0 || self.failed > 0 + } +} + +/// Remove every rollback copy an earlier run left behind: the encrypted ones +/// under the app's state directory, and the plaintext ones older builds wrote +/// into the system temp directory. +/// +/// A restore removes its own copy on every exit path, so anything found here +/// belongs to a process that died mid-restore. Call it once per launch, off +/// the boot path: on Linux and macOS each freed file costs a keyring round +/// trip. +pub fn sweep_rollback_dirs( + app_handle: &dyn AppContext, + report: &mut dyn FnMut(&str, String), +) -> RollbackSweepStats { + let mut stats = RollbackSweepStats::default(); + match crate::storage::riot_rollback_dir(app_handle) { + Ok(root) => stats.merge(sweep_rollback_root(&root, report)), + Err(detail) => report("Could not resolve the Riot rollback directory", detail), + } + stats.merge(sweep_legacy_rollback_dirs(&std::env::temp_dir(), report)); + stats +} + +/// Free the keyring entries of every leftover rollback copy under `root`, then +/// remove them and the (now empty) root. A missing root is the normal case. +fn sweep_rollback_root(root: &Path, report: &mut dyn FnMut(&str, String)) -> RollbackSweepStats { + let mut stats = RollbackSweepStats::default(); + let entries = match fs::read_dir(root) { + Ok(entries) => entries, + Err(e) => { + if e.kind() != std::io::ErrorKind::NotFound { + report( + "Could not enumerate the Riot rollback directory", + format!("dir={} error={e}", root.display()), + ); + } + return stats; + } + }; + + for entry in entries.flatten() { + // A symlink planted here must not steer the removal at its target. + if crate::fs_utils::is_reparse_point(&entry) { + continue; + } + let path = entry.path(); + if path.is_dir() { + snapshot_crypto::free_dir_secrets_with_errors(&path, report); + } else { + snapshot_crypto::delete_encrypted_file_secret(&path); + } + match remove_path_if_exists(&path) { + Ok(()) => stats.removed += 1, + Err(detail) => { + stats.failed += 1; + report("Could not remove a leftover Riot rollback copy", detail); + } + } + } + + // Empty now, unless something failed above. Either way this is best-effort. + let _ = fs::remove_dir(root); + stats +} + +/// Remove the plaintext rollback directories older builds left in `temp_dir`. +/// Only entries whose name is the historical prefix followed by a UUID are +/// touched, so an unrelated directory is never removed. They hold no keyring +/// token: the copy was written in the clear. +fn sweep_legacy_rollback_dirs( + temp_dir: &Path, + report: &mut dyn FnMut(&str, String), +) -> RollbackSweepStats { + let mut stats = RollbackSweepStats::default(); + let Ok(entries) = fs::read_dir(temp_dir) else { + // An unreadable temp directory is not worth a log line: nothing else + // in the app would work either. + return stats; + }; + + for entry in entries.flatten() { + if crate::fs_utils::is_reparse_point(&entry) { + continue; + } + let file_name = entry.file_name(); + let name = file_name.to_string_lossy(); + let Some(suffix) = name.strip_prefix(LEGACY_ROLLBACK_PREFIX) else { + continue; + }; + if Uuid::parse_str(suffix).is_err() { + continue; + } + let path = entry.path(); + if !path.is_dir() { + continue; + } + match fs::remove_dir_all(&path) { + Ok(()) => stats.removed += 1, + Err(e) => { + stats.failed += 1; + report( + "Could not remove a legacy plaintext Riot rollback copy", + format!("dir={} error={e}", path.display()), + ); + } + } + } + + stats +} + /// Undo a partially-applied restore: wipe whatever the failed copy loop left /// behind and put the pre-restore live state (captured by /// `backup_live_state_for_rollback`) back. Best-effort: a failure here is @@ -1040,13 +1189,7 @@ fn restore_live_state_from_rollback( Ok(Some(path)) => path, _ => continue, }; - let result = match item.kind { - RiotSnapshotKind::Directory => { - plain_copy_dir(&source_path, &target_path, item.ignored_names) - } - RiotSnapshotKind::File => plain_copy_file(&source_path, &target_path), - }; - if let Err(e) = result { + if let Err(e) = restore_item_from_rollback(&source_path, &target_path, item) { log_platform_error( app_handle, "riot.restore_rollback", @@ -1080,18 +1223,26 @@ fn restore_live_snapshot(app_handle: &dyn AppContext, profile_id: &str) -> Resul // back to this backup instead of leaving a mix of the old and new // profile's data. If the backup itself can't be made, fail closed and // abort before touching anything live. - let rollback_dir = backup_live_state_for_rollback(install_dir.as_deref())?; + let rollback_dir = backup_live_state_for_rollback(app_handle, install_dir.as_deref())?; if let Err(e) = clear_live_riot_state(install_dir.as_deref()) { restore_live_state_from_rollback(app_handle, &rollback_dir, install_dir.as_deref()); - let _ = fs::remove_dir_all(&rollback_dir); + discard_rollback_dir(app_handle, &rollback_dir); return Err(e); } for item in RIOT_SNAPSHOT_ITEMS { let source_path = snapshot_dir.join(item.snapshot_name); - let Some(target_path) = live_path_for(item, install_dir.as_deref())? else { - continue; + // The live state is already cleared here, so a path that cannot be + // resolved any more takes the same route as a failed copy. + let target_path = match live_path_for(item, install_dir.as_deref()) { + Ok(Some(path)) => path, + Ok(None) => continue, + Err(e) => { + restore_live_state_from_rollback(app_handle, &rollback_dir, install_dir.as_deref()); + discard_rollback_dir(app_handle, &rollback_dir); + return Err(e); + } }; match item.kind { @@ -1105,7 +1256,7 @@ fn restore_live_snapshot(app_handle: &dyn AppContext, profile_id: &str) -> Resul &rollback_dir, install_dir.as_deref(), ); - let _ = fs::remove_dir_all(&rollback_dir); + discard_rollback_dir(app_handle, &rollback_dir); return Err(e); } } @@ -1118,7 +1269,7 @@ fn restore_live_snapshot(app_handle: &dyn AppContext, profile_id: &str) -> Resul &rollback_dir, install_dir.as_deref(), ); - let _ = fs::remove_dir_all(&rollback_dir); + discard_rollback_dir(app_handle, &rollback_dir); return Err(e); } } else if !item.optional { @@ -1130,14 +1281,14 @@ fn restore_live_snapshot(app_handle: &dyn AppContext, profile_id: &str) -> Resul &rollback_dir, install_dir.as_deref(), ); - let _ = fs::remove_dir_all(&rollback_dir); + discard_rollback_dir(app_handle, &rollback_dir); return Ok(false); } } } } - let _ = fs::remove_dir_all(&rollback_dir); + discard_rollback_dir(app_handle, &rollback_dir); Ok(has_snapshot) } @@ -2027,3 +2178,217 @@ riot-login: assert!(yaml_has_auth_tokens(yaml)); } } + +/// The rollback copy a restore stages while it replaces the live session: it +/// holds real auth material, so where it lands, how it is written and when it +/// is removed are all load-bearing. +#[cfg(test)] +mod rollback_tests { + use super::*; + use crate::secrets::backend; + use crate::snapshot_crypto::ENCRYPTED_HEADER; + + struct TempCtx { + root: PathBuf, + } + + impl AppContext for TempCtx { + fn app_config_dir(&self) -> Result { + Ok(self.root.clone()) + } + fn app_data_dir(&self) -> Result { + Ok(self.root.clone()) + } + fn app_local_data_dir(&self) -> Result { + Ok(self.root.clone()) + } + fn app_cache_dir(&self) -> Result { + Ok(self.root.clone()) + } + } + + fn scratch(tag: &str) -> PathBuf { + let root = std::env::temp_dir().join(format!( + "accshift-riot-rollback-test-{}-{}-{:?}", + tag, + std::process::id(), + std::thread::current().id() + )); + let _ = fs::remove_dir_all(&root); + fs::create_dir_all(&root).unwrap(); + root + } + + /// The `RiotGamesPrivateSettings.yaml` entry: a required file item. + fn file_item() -> &'static RiotSnapshotItem { + RIOT_SNAPSHOT_ITEMS + .iter() + .find(|item| matches!(item.kind, RiotSnapshotKind::File)) + .unwrap() + } + + fn dir_item() -> &'static RiotSnapshotItem { + RIOT_SNAPSHOT_ITEMS + .iter() + .find(|item| matches!(item.kind, RiotSnapshotKind::Directory)) + .unwrap() + } + + #[test] + fn the_rollback_dir_lives_in_the_state_dir_not_in_temp() { + // Pure path check on a root that is nowhere near the system temp + // directory, which is exactly where this copy used to land in the + // clear for every process on the machine to read. + let ctx = TempCtx { + root: PathBuf::from("Z:").join("accshift-local"), + }; + let dir = crate::storage::riot_rollback_dir(&ctx).unwrap(); + + assert!( + dir.ends_with(Path::new("state").join("riot-rollback")), + "{dir:?}" + ); + assert!(dir.starts_with(crate::storage::app_local_data_root(&ctx).unwrap())); + assert!(!dir.starts_with(std::env::temp_dir()), "{dir:?}"); + } + + #[test] + fn the_rollback_copy_is_encrypted_and_decrypts_back() { + let root = scratch("roundtrip"); + let live = root.join("live"); + let rollback = root.join("rollback"); + let restored = root.join("restored"); + fs::create_dir_all(&live).unwrap(); + let secret: &[u8] = b"riot private settings with an access_token in them"; + fs::write(live.join("settings.yaml"), secret).unwrap(); + + let item = file_item(); + copy_item_into_rollback( + &live.join("settings.yaml"), + &rollback.join(item.snapshot_name), + item, + ) + .unwrap(); + + let stored = fs::read(rollback.join(item.snapshot_name)).unwrap(); + assert_ne!(stored.as_slice(), secret, "the copy is plaintext on disk"); + assert!(stored.starts_with(ENCRYPTED_HEADER)); + + restore_item_from_rollback( + &rollback.join(item.snapshot_name), + &restored.join("settings.yaml"), + item, + ) + .unwrap(); + assert_eq!(fs::read(restored.join("settings.yaml")).unwrap(), secret); + + let _ = fs::remove_dir_all(&root); + } + + #[test] + fn discarding_the_rollback_copy_frees_every_entry_it_owns() { + // On Linux and macOS each encrypted file points at a keyring entry. + // Removing the directory without freeing them leaks one per file, for + // good: nothing can list the store to find them again. + let root = scratch("discard"); + let live = root.join("live").join("Sessions"); + fs::create_dir_all(live.join("nested")).unwrap(); + fs::write(live.join("session.json"), b"token-a").unwrap(); + fs::write(live.join("nested").join("more.json"), b"token-b").unwrap(); + let ctx = TempCtx { root: root.clone() }; + let before = backend::entry_count(); + + let item = dir_item(); + let rollback = root.join("rollback"); + copy_item_into_rollback(&live, &rollback.join(item.snapshot_name), item).unwrap(); + assert_eq!(backend::entry_count(), before + 2); + + discard_rollback_dir(&ctx, &rollback); + + assert!(!rollback.exists()); + assert_eq!(backend::entry_count(), before); + let _ = fs::remove_dir_all(&root); + } + + #[test] + fn the_sweep_removes_a_stale_rollback_copy_and_its_entries() { + let root = scratch("sweep-state"); + let live = root.join("live"); + fs::create_dir_all(&live).unwrap(); + fs::write(live.join("settings.yaml"), b"stranded-token").unwrap(); + let ctx = TempCtx { root: root.clone() }; + let before = backend::entry_count(); + + // What a process killed mid-restore leaves behind. + let rollback_root = crate::storage::riot_rollback_dir(&ctx).unwrap(); + let stale = rollback_root.join(Uuid::new_v4().to_string()); + copy_item_into_rollback( + &live.join("settings.yaml"), + &stale.join("RiotGamesPrivateSettings.yaml"), + file_item(), + ) + .unwrap(); + assert_eq!(backend::entry_count(), before + 1); + + let mut reports = Vec::new(); + // The count is a lower bound on purpose: this call also sweeps the + // real temp directory, which may hold a legacy copy from an actual + // run on this machine. + let stats = sweep_rollback_dirs(&ctx, &mut |m, d| reports.push(format!("{m}: {d}"))); + + assert!(stats.removed >= 1, "{stats:?}"); + assert_eq!(stats.failed, 0); + assert!(reports.is_empty(), "{reports:?}"); + assert!(!stale.exists()); + assert!(!rollback_root.exists(), "the empty root goes too"); + assert_eq!(backend::entry_count(), before); + let _ = fs::remove_dir_all(&root); + } + + #[test] + fn a_clean_store_sweeps_silently() { + // A rollback directory that was never created is the normal case on + // every launch, and says nothing worth logging. + let root = scratch("sweep-clean"); + let ctx = TempCtx { root: root.clone() }; + + let mut reports = Vec::new(); + let stats = sweep_rollback_root( + &crate::storage::riot_rollback_dir(&ctx).unwrap(), + &mut |m, d| reports.push(format!("{m}: {d}")), + ); + + assert_eq!(stats, RollbackSweepStats::default()); + assert!(!stats.touched_anything()); + assert!(reports.is_empty(), "{reports:?}"); + let _ = fs::remove_dir_all(&root); + } + + #[test] + fn the_legacy_sweep_takes_the_old_temp_copies_and_leaves_the_rest_alone() { + let temp = scratch("sweep-legacy"); + let legacy = temp.join(format!("{LEGACY_ROLLBACK_PREFIX}{}", Uuid::new_v4())); + fs::create_dir_all(&legacy).unwrap(); + fs::write(legacy.join("RiotGamesPrivateSettings.yaml"), b"plaintext").unwrap(); + + // Neighbours in the same temp directory that must survive: another + // app's directory, our own snapshot test scratch, and a file whose + // name happens to start the same way without a UUID after it. + let unrelated = temp.join("some-other-app"); + fs::create_dir_all(&unrelated).unwrap(); + let near_miss = temp.join(format!("{LEGACY_ROLLBACK_PREFIX}not-a-uuid")); + fs::create_dir_all(&near_miss).unwrap(); + + let mut reports = Vec::new(); + let stats = + sweep_legacy_rollback_dirs(&temp, &mut |m, d| reports.push(format!("{m}: {d}"))); + + assert_eq!(stats.removed, 1); + assert_eq!(stats.failed, 0); + assert!(reports.is_empty(), "{reports:?}"); + assert!(!legacy.exists()); + assert!(unrelated.exists()); + assert!(near_miss.exists()); + let _ = fs::remove_dir_all(&temp); + } +} diff --git a/crates/accshift-core/src/platforms/steam/accounts.rs b/crates/accshift-core/src/platforms/steam/accounts.rs index 4c0a376..7b39b76 100644 --- a/crates/accshift-core/src/platforms/steam/accounts.rs +++ b/crates/accshift-core/src/platforms/steam/accounts.rs @@ -154,8 +154,8 @@ fn set_login_user_flags(steam_path: &Path, target: Option<&str>) -> Result<(), A .map(|t| account_name == t && !account_name.is_empty()) .unwrap_or(false); let flag = if is_target { "1" } else { "0" }; - updated = vdf_set_nested_value(&updated, &[steam_id.as_str(), "AllowAutoLogin"], flag); - updated = vdf_set_nested_value(&updated, &[steam_id.as_str(), "MostRecent"], flag); + updated = vdf_set_nested_value(&updated, &[steam_id.as_str(), "AllowAutoLogin"], flag)?; + updated = vdf_set_nested_value(&updated, &[steam_id.as_str(), "MostRecent"], flag)?; } crate::storage::write_bytes_atomic(&path, updated.as_bytes()).map_err(AppError::FileRead) diff --git a/crates/accshift-core/src/platforms/steam/bulk_edit.rs b/crates/accshift-core/src/platforms/steam/bulk_edit.rs index 8fddf2c..98cd0a4 100644 --- a/crates/accshift-core/src/platforms/steam/bulk_edit.rs +++ b/crates/accshift-core/src/platforms/steam/bulk_edit.rs @@ -75,12 +75,12 @@ fn apply_for_account( if let Some(news) = request.news_popup { let val = if news { "1" } else { "0" }; - content = vdf_set_nested_value(&content, &["news", "NotifyAvailableGames"], val); + content = vdf_set_nested_value(&content, &["news", "NotifyAvailableGames"], val)?; } if let Some(dnd) = request.do_not_disturb { let val = if dnd { "1" } else { "0" }; - content = vdf_set_nested_value(&content, &["friends", "DoNotDisturb"], val); + content = vdf_set_nested_value(&content, &["friends", "DoNotDisturb"], val)?; } for edit in &request.launch_options { @@ -98,7 +98,7 @@ fn apply_for_account( "LaunchOptions", ], &edit.value, - ); + )?; } crate::storage::write_bytes_atomic(&config_path, content.as_bytes()).map_err(AppError::FileRead) diff --git a/crates/accshift-core/src/platforms/steam/mod.rs b/crates/accshift-core/src/platforms/steam/mod.rs index 5ec2a22..020fa55 100644 --- a/crates/accshift-core/src/platforms/steam/mod.rs +++ b/crates/accshift-core/src/platforms/steam/mod.rs @@ -18,7 +18,7 @@ use profile::ProfileInfo; use serde::Serialize; use serde_json::Value; use std::collections::{HashMap, HashSet}; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::sync::{Mutex, OnceLock}; use uuid::Uuid; @@ -220,6 +220,62 @@ fn is_force_kill(params: &Value) -> bool { .unwrap_or(false) } +/// What a candidate folder looks like from Steam's point of view. +/// +/// One classifier for the folder picker ([`set_steam_path`]) and for every +/// read path ([`resolve_steam_path`]), so a folder can never be accepted by +/// one and reported as "not installed" by the other. That split was the bug: +/// the picker took a folder holding only `steam.exe`, every later read +/// demanded `config/loginusers.vdf` and failed with a generic message. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum SteamFolder { + /// The path is missing, or is a file rather than a folder. + NotADirectory, + /// A real folder, but neither the Steam client nor a login history. + NotSteam, + /// Steam is installed here and has never signed in, so it has not written + /// `config/loginusers.vdf` and there is no account to read yet. + NeverSignedIn, + /// `config/loginusers.vdf` is present: usable. + Usable, +} + +/// English text for the "installed but never signed in" case. The webview +/// shows a translated line keyed on [`STEAM_PATH_NEVER_SIGNED_IN`]; this is +/// what any other caller (and an untranslated fallback) gets. +const NEVER_SIGNED_IN_MESSAGE: &str = "Steam found, sign in once so it creates its login history"; + +/// Machine-readable codes for the folder-picker rejections. +/// +/// `PlatformError` serializes to the webview as its bare message string, so a +/// code travels inside that message, ahead of a `|` and an English fallback +/// (see [`coded_path_error`]). The frontend matches the code and translates +/// it; it never matches English prose. +pub const STEAM_PATH_NOT_A_DIRECTORY: &str = "steam_path_not_a_directory"; +pub const STEAM_PATH_NOT_STEAM: &str = "steam_path_not_steam"; +pub const STEAM_PATH_NEVER_SIGNED_IN: &str = "steam_path_never_signed_in"; + +pub fn classify_steam_folder(path: &Path) -> SteamFolder { + if !path.is_dir() { + return SteamFolder::NotADirectory; + } + if path.join("config").join("loginusers.vdf").is_file() { + return SteamFolder::Usable; + } + if path.join(os::steam_executable_name()).is_file() { + return SteamFolder::NeverSignedIn; + } + SteamFolder::NotSteam +} + +/// `code|english fallback`, the format the webview parses. +fn coded_path_error(code: &str, english: &str) -> PlatformError { + PlatformError::new( + PlatformErrorKind::ClientNotInstalled, + format!("{code}|{english}"), + ) +} + fn resolve_steam_path(app_handle: &dyn AppContext) -> Result { let cfg = config::load_config(app_handle); let override_path = cfg.steam.path_override.trim(); @@ -230,14 +286,19 @@ fn resolve_steam_path(app_handle: &dyn AppContext) -> Result Ok(steam_path), + // Plain prose, not a code: this one reaches a dozen commands whose + // rejections the webview shows verbatim. + SteamFolder::NeverSignedIn => Err(PlatformError::new( + PlatformErrorKind::ClientNotInstalled, + NEVER_SIGNED_IN_MESSAGE, + )), + SteamFolder::NotADirectory | SteamFolder::NotSteam => Err(PlatformError::new( PlatformErrorKind::ClientNotInstalled, "Could not locate Steam installation", - )); + )), } - - Ok(steam_path) } pub struct SteamService; @@ -656,17 +717,31 @@ pub fn get_steam_path(app_handle: AppCtx) -> Result { pub fn set_steam_path(app_handle: AppCtx, path: String) -> Result<(), PlatformError> { let trimmed = path.trim().to_string(); - // The override is later joined with steam.exe and launched. Only accept - // an existing directory that actually looks like a Steam install. + // The override is later joined with steam.exe and launched, and every read + // path resolves it through `classify_steam_folder`. Accept exactly what + // those reads accept, so nothing can be saved here and then reported as + // "not installed" a second later. if !trimmed.is_empty() { - let candidate = PathBuf::from(&trimmed); - if !candidate.is_dir() { - return Err("Steam path override must be an existing directory".into()); - } - if !candidate.join(os::steam_executable_name()).exists() - && !candidate.join("config").join("loginusers.vdf").exists() - { - return Err("This folder does not look like a Steam installation".into()); + match classify_steam_folder(Path::new(&trimmed)) { + SteamFolder::Usable => {} + SteamFolder::NeverSignedIn => { + return Err(coded_path_error( + STEAM_PATH_NEVER_SIGNED_IN, + NEVER_SIGNED_IN_MESSAGE, + )); + } + SteamFolder::NotSteam => { + return Err(coded_path_error( + STEAM_PATH_NOT_STEAM, + "This folder does not look like a Steam installation", + )); + } + SteamFolder::NotADirectory => { + return Err(coded_path_error( + STEAM_PATH_NOT_A_DIRECTORY, + "Steam path override must be an existing directory", + )); + } } } config::update_config(&app_handle, |cfg| { @@ -1008,6 +1083,102 @@ impl PlatformService for SteamService { mod tests { use super::*; use std::cell::RefCell; + use std::sync::atomic::{AtomicU64, Ordering}; + + /// Unique temp directory per test, removed on drop. + struct TempRoot(PathBuf); + + impl TempRoot { + fn new(tag: &str) -> Self { + static COUNTER: AtomicU64 = AtomicU64::new(0); + let n = COUNTER.fetch_add(1, Ordering::Relaxed); + let dir = std::env::temp_dir().join(format!( + "accshift-steam-folder-test-{tag}-{}-{n}", + std::process::id() + )); + std::fs::create_dir_all(&dir).expect("create temp test dir"); + Self(dir) + } + + fn with_steam_exe(self) -> Self { + std::fs::write(self.0.join(os::steam_executable_name()), b"stub") + .expect("write steam executable stub"); + self + } + + fn with_login_history(self) -> Self { + let config_dir = self.0.join("config"); + std::fs::create_dir_all(&config_dir).expect("create config dir"); + std::fs::write(config_dir.join("loginusers.vdf"), b"\"users\"{}") + .expect("write loginusers.vdf stub"); + self + } + } + + impl Drop for TempRoot { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } + } + + #[test] + fn classify_steam_folder_reports_never_signed_in_for_executable_only() { + // The exact folder the audit found: the picker used to take it and + // every read then said "not installed". + let tmp = TempRoot::new("exe-only").with_steam_exe(); + assert_eq!( + classify_steam_folder(&tmp.0), + SteamFolder::NeverSignedIn, + "steam.exe without config/loginusers.vdf means never signed in" + ); + } + + #[test] + fn classify_steam_folder_accepts_login_history_alone() { + // A Steam whose executable sits elsewhere (or a copied config tree) + // still has the account list every read needs. + let tmp = TempRoot::new("login-only").with_login_history(); + assert_eq!(classify_steam_folder(&tmp.0), SteamFolder::Usable); + } + + #[test] + fn classify_steam_folder_accepts_a_complete_install() { + let tmp = TempRoot::new("both").with_steam_exe().with_login_history(); + assert_eq!(classify_steam_folder(&tmp.0), SteamFolder::Usable); + } + + #[test] + fn classify_steam_folder_rejects_an_unrelated_folder() { + let tmp = TempRoot::new("neither"); + assert_eq!(classify_steam_folder(&tmp.0), SteamFolder::NotSteam); + } + + #[test] + fn classify_steam_folder_rejects_a_missing_path_and_a_plain_file() { + let tmp = TempRoot::new("not-a-dir"); + assert_eq!( + classify_steam_folder(&tmp.0.join("does-not-exist")), + SteamFolder::NotADirectory + ); + let file = tmp.0.join("Steam"); + std::fs::write(&file, b"not a folder").expect("write file"); + assert_eq!(classify_steam_folder(&file), SteamFolder::NotADirectory); + } + + // The webview splits the message on the first '|' and translates the left + // half. Losing that shape would drop it back to matching English prose. + #[test] + fn coded_path_error_carries_the_code_then_the_english_fallback() { + let err = coded_path_error(STEAM_PATH_NEVER_SIGNED_IN, NEVER_SIGNED_IN_MESSAGE); + assert_eq!(err.kind, PlatformErrorKind::ClientNotInstalled); + assert_eq!( + err.message, + "steam_path_never_signed_in|Steam found, sign in once so it creates its login history" + ); + let (code, english) = err.message.split_once('|').expect("code and fallback"); + assert_eq!(code, STEAM_PATH_NEVER_SIGNED_IN); + assert_eq!(english, NEVER_SIGNED_IN_MESSAGE); + } #[test] fn validate_steam_id_accepts_17_digit_numeric() { diff --git a/crates/accshift-core/src/platforms/steam/vdf.rs b/crates/accshift-core/src/platforms/steam/vdf.rs index 2881bf0..daf7de4 100644 --- a/crates/accshift-core/src/platforms/steam/vdf.rs +++ b/crates/accshift-core/src/platforms/steam/vdf.rs @@ -1,5 +1,4 @@ use std::collections::HashMap; -use std::fmt::Write as _; use std::fs; use std::path::Path; @@ -162,181 +161,416 @@ pub fn parse_vdf(content: &str) -> HashMap> { accounts } -/// Set a nested value in a VDF file by path. -/// -/// `path` is a slice of section/key names relative to the root section. -/// The last element is the key to set; preceding elements are section names. -/// Example: `["friends", "DoNotDisturb"]` sets the `DoNotDisturb` key inside the `friends` section. +/// A meaningful element of a VDF line: a quoted token, or a brace that opens or +/// closes a section outside any quoted token. +#[derive(Debug, Clone, PartialEq)] +enum VdfItem { + Token(String), + Open, + Close, +} + +/// Scan a line into its items, in order, each paired with the byte index it +/// starts at. /// -/// If the key already exists at the target path, its value is replaced. -/// If the section exists but the key does not, the key is inserted before the section's closing `}`. -/// If the section does not exist, it is created (with the key) before the file's final `}`. -pub fn vdf_set_nested_value(content: &str, path: &[&str], value: &str) -> String { - assert!( - path.len() >= 2, - "path must have at least a section and a key" - ); +/// This is what makes `"key" {` structure rather than noise. The old writer +/// compared the trimmed line against `{` and `}`, so a file written with the +/// brace on the header line was walked without ever entering the block and the +/// write silently changed nothing. Scanning stops at a `//` comment outside +/// quotes, so a brace inside a comment cannot desync the section stack. +fn vdf_scan_line(line: &str) -> Vec<(usize, VdfItem)> { + let mut items = Vec::new(); + let bytes = line.as_bytes(); + let mut chars = line.char_indices().peekable(); - let sections = &path[..path.len() - 1]; - let target_key = path[path.len() - 1]; + while let Some(&(i, c)) = chars.peek() { + match c { + '"' => { + chars.next(); // consume opening quote + let mut token = String::new(); + while let Some((_, ch)) = chars.next() { + if ch == '\\' { + match chars.next() { + Some((_, 'n')) => token.push('\n'), + Some((_, 'r')) => token.push('\r'), + Some((_, 't')) => token.push('\t'), + Some((_, '\\')) => token.push('\\'), + Some((_, '"')) => token.push('"'), + // Unknown escape: keep the following char verbatim. + Some((_, other)) => token.push(other), + None => break, + } + } else if ch == '"' { + break; // closing quote + } else { + token.push(ch); + } + } + items.push((i, VdfItem::Token(token))); + } + '{' => { + items.push((i, VdfItem::Open)); + chars.next(); + } + '}' => { + items.push((i, VdfItem::Close)); + chars.next(); + } + '/' if bytes.get(i + 1) == Some(&b'/') => break, + _ => { + chars.next(); + } + } + } - let lines: Vec<&str> = content.lines().collect(); - let mut result = String::with_capacity(content.len() + 128); - let mut depth: usize = 0; - let mut matched_depth: usize = 0; // how many sections from `sections` we have entered - let mut found = false; - let mut inserted = false; + items +} - // Two-pass: first scan to check if key exists, then build output. - for line in lines.iter() { - let trimmed = line.trim(); +/// How many leading names of `sections` the open section stack has entered. +/// +/// `sections` is relative to the root section, so the comparison starts at +/// `stack[1]`: the root's own name is whatever the file calls it. +fn vdf_matched_sections(stack: &[String], sections: &[&str]) -> usize { + if stack.is_empty() { + return 0; + } + let inner = &stack[1..]; + let mut matched = 0; + while matched < sections.len() + && matched < inner.len() + && inner[matched].eq_ignore_ascii_case(sections[matched]) + { + matched += 1; + } + matched +} - if trimmed == "{" { - depth += 1; +/// The file's indentation unit: a tab as soon as any indented line uses one, +/// otherwise the narrowest run of leading spaces in the file. Steam writes +/// tabs, which is also the fallback for a file with no indented line at all. +fn vdf_indent_unit(content: &str) -> String { + let mut min_spaces: Option = None; + + for line in content.lines() { + if line.trim().is_empty() { continue; } + let indent = &line[..line.len() - line.trim_start().len()]; + if indent.contains('\t') { + return "\t".to_string(); + } + if !indent.is_empty() { + min_spaces = Some(min_spaces.map_or(indent.len(), |m: usize| m.min(indent.len()))); + } + } - if trimmed == "}" { - if depth > 0 { - if matched_depth == depth && matched_depth <= sections.len() && matched_depth > 0 { - matched_depth = matched_depth.saturating_sub(1); + match min_spaces { + Some(n) => " ".repeat(n), + None => "\t".to_string(), + } +} + +/// True when the tokens buffered so far are the `"target_key" "value"` pair at +/// exactly the section path we are aiming at. +fn vdf_is_target_pair( + stack: &[String], + sections: &[&str], + target_key: &str, + pending: &[String], +) -> bool { + pending.len() >= 2 + && stack.len() == sections.len() + 1 + && vdf_matched_sections(stack, sections) == sections.len() + && pending[0].eq_ignore_ascii_case(target_key) +} + +/// Name for the section a `{` is about to open: the last token on the same +/// line, or the bare header token carried over from a previous line. +/// +/// The carried token is what makes the standalone-brace layout work at all, so +/// it survives a line with no tokens (a `//` comment or a blank line sitting +/// between a header and its brace) and is dropped by a `"key" "value"` pair, +/// which is never a header. +fn vdf_take_section_name(pending: &mut Vec, carried: &mut Option) -> String { + let name = match pending.pop() { + Some(token) => token, + None => carried.take().unwrap_or_default(), + }; + pending.clear(); + *carried = None; + name +} + +/// Remember a bare header token for the `{` on a following line. +fn vdf_carry_header(pending: &[String], carried: &mut Option) { + match pending.len() { + 1 => *carried = Some(pending[0].clone()), + 0 => {} + _ => *carried = None, + } +} + +/// Does `sections` + `target_key` already name a key in `content`? +fn vdf_key_exists(content: &str, sections: &[&str], target_key: &str) -> bool { + let mut stack: Vec = Vec::new(); + let mut carried: Option = None; + + for line in content.lines() { + let mut pending: Vec = Vec::new(); + for (_, item) in vdf_scan_line(line) { + match item { + VdfItem::Token(token) => pending.push(token), + VdfItem::Open => { + let name = vdf_take_section_name(&mut pending, &mut carried); + stack.push(name); + } + VdfItem::Close => { + if vdf_is_target_pair(&stack, sections, target_key, &pending) { + return true; + } + pending.clear(); + carried = None; + stack.pop(); } - depth -= 1; } - continue; } + if vdf_is_target_pair(&stack, sections, target_key, &pending) { + return true; + } + vdf_carry_header(&pending, &mut carried); + } - let tokens = vdf_tokenize_line(trimmed); + false +} - // Check if this is a section header we're looking for - if !tokens.is_empty() && matched_depth < sections.len() && depth == matched_depth + 1 { - let key = &tokens[0]; - if key.eq_ignore_ascii_case(sections[matched_depth]) { - matched_depth += 1; - continue; - } - } +/// The lines to write in front of the closing brace the walker is standing on, +/// or `None` when this brace is not the right place. +/// +/// Two placements, in the order the old writer used them: the target section is +/// open and about to close, so the key drops straight in; or the deepest +/// section that does exist is about to close, so the missing ones are created +/// inside it with the key at the bottom. +fn vdf_insert_block( + stack: &[String], + sections: &[&str], + escaped_key: &str, + escaped_value: &str, + unit: &str, +) -> Option> { + let matched = vdf_matched_sections(stack, sections); + + if matched == sections.len() && stack.len() == sections.len() + 1 { + return Some(vec![format!( + "{}\"{escaped_key}\"\t\t\"{escaped_value}\"", + unit.repeat(stack.len()) + )]); + } - // Check if this is the target key at the right depth - if tokens.len() >= 2 - && matched_depth == sections.len() - && depth == sections.len() + 1 - && tokens[0].eq_ignore_ascii_case(target_key) - { - found = true; - break; + if matched < sections.len() && stack.len() == matched + 1 { + let base = unit.repeat(stack.len()); + let mut block = Vec::new(); + for (j, section) in sections[matched..].iter().enumerate() { + let indent = format!("{base}{}", unit.repeat(j)); + block.push(format!("{indent}\"{}\"", escape_vdf_string(section))); + block.push(format!("{indent}{{")); + } + let key_indent = format!("{base}{}", unit.repeat(sections.len() - matched)); + block.push(format!( + "{key_indent}\"{escaped_key}\"\t\t\"{escaped_value}\"" + )); + for j in (0..sections.len() - matched).rev() { + block.push(format!("{base}{}}}", unit.repeat(j))); } + return Some(block); } - // ── second pass: build output ── - depth = 0; - matched_depth = 0; - let mut key_replaced = false; + None +} - for line in lines.iter() { - let trimmed = line.trim(); +/// Rewrite the line that already carries the target key. +/// +/// A line that is nothing but the pair is reformatted the way this writer has +/// always written one: key, two tabs, value, keeping the original indentation. +/// Anything else on the line (a second pair, an inline brace) means only the +/// value token itself is spliced by its byte span, so nothing sharing the +/// physical line is lost. +fn vdf_rewrite_pair( + line: &str, + items: &[(usize, VdfItem)], + value_ordinal: usize, + escaped_key: &str, + escaped_value: &str, +) -> String { + let bare_pair = items.len() == 2 + && matches!(items[0].1, VdfItem::Token(_)) + && matches!(items[1].1, VdfItem::Token(_)); + + if bare_pair { + let leading: String = line.chars().take_while(|c| c.is_whitespace()).collect(); + return format!("{leading}\"{escaped_key}\"\t\t\"{escaped_value}\""); + } - if trimmed == "{" { - result.push_str(line); - result.push('\n'); - depth += 1; - continue; + match nth_quoted_token_span(line, value_ordinal) { + Some((open, close)) => { + let mut new_line = String::with_capacity(line.len() + escaped_value.len()); + new_line.push_str(&line[..=open]); + new_line.push_str(escaped_value); + new_line.push_str(&line[close..]); + new_line } + None => line.to_string(), + } +} - if trimmed == "}" { - // If we need to insert the key before the closing brace of the target section - if !inserted && !found && matched_depth == sections.len() && depth == sections.len() + 1 - { - let indent = "\t".repeat(depth); - let escaped_value = escape_vdf_string(value); - let _ = writeln!(result, "{indent}\"{target_key}\"\t\t\"{escaped_value}\""); - inserted = true; - } +/// Set a nested value in a VDF file by path. +/// +/// `path` is a slice of section/key names relative to the root section. +/// The last element is the key to set; preceding elements are section names. +/// Example: `["friends", "DoNotDisturb"]` sets the `DoNotDisturb` key inside the `friends` section. +/// +/// If the key already exists at the target path, its value is replaced. +/// If the section exists but the key does not, the key is inserted before the section's closing `}`. +/// If the section does not exist, it is created (with the key) before the file's final `}`. +/// +/// Targeting is structural and shares [`vdf_scan_line`] with the reader, so a +/// file written with `"key" {` on one line is walked exactly like one with the +/// brace on its own line. Returning `Err` when neither branch fired is the +/// point of the signature: the previous version handed the input straight back, +/// so every caller wrote the same bytes and reported success. +/// +/// The file's line ending and indentation unit are preserved, so a CRLF +/// localconfig.vdf comes back CRLF and a space-indented file stays +/// space-indented. +pub fn vdf_set_nested_value( + content: &str, + path: &[&str], + value: &str, +) -> Result { + assert!( + path.len() >= 2, + "path must have at least a section and a key" + ); + + let sections = &path[..path.len() - 1]; + let target_key = path[path.len() - 1]; + + let newline = if content.contains("\r\n") { + "\r\n" + } else { + "\n" + }; + let unit = vdf_indent_unit(content); + let escaped_key = escape_vdf_string(target_key); + let escaped_value = escape_vdf_string(value); + + // Whether the key already exists decides which branch may fire, so it is + // settled before a single output line is built. + let found = vdf_key_exists(content, sections, target_key); - // If we need to insert a missing section before the parent's closing brace - if !inserted && !found { - // Check if this brace closes at a depth where we need to insert the remaining sections - if matched_depth < sections.len() && depth == matched_depth + 1 { - // Insert all remaining sections + key - let base_indent = "\t".repeat(depth); - for (j, section) in sections[matched_depth..].iter().enumerate() { - let section_indent = format!("{}{}", base_indent, "\t".repeat(j)); - let _ = writeln!(result, "{section_indent}\"{section}\""); - let _ = writeln!(result, "{section_indent}{{"); + let mut out: Vec = Vec::new(); + let mut stack: Vec = Vec::new(); + let mut carried: Option = None; + let mut done = false; + + for line in content.lines() { + let items = vdf_scan_line(line); + let mut pending: Vec = Vec::new(); + let mut pending_first_ordinal = 0usize; + let mut ordinal = 0usize; + let mut opened_here = 0usize; + + let mut rewritten: Option = None; + let mut pre_insert: Vec = Vec::new(); + let mut split: Option<(usize, String)> = None; + + for (at, item) in &items { + match item { + VdfItem::Token(token) => { + if pending.is_empty() { + pending_first_ordinal = ordinal; + } + pending.push(token.clone()); + ordinal += 1; + } + VdfItem::Open => { + let name = vdf_take_section_name(&mut pending, &mut carried); + stack.push(name); + opened_here += 1; + } + VdfItem::Close => { + if !done && found && vdf_is_target_pair(&stack, sections, target_key, &pending) + { + rewritten = Some(vdf_rewrite_pair( + line, + &items, + pending_first_ordinal + 1, + &escaped_key, + &escaped_value, + )); + done = true; } - let key_indent = format!( - "{}{}", - base_indent, - "\t".repeat(sections.len() - matched_depth) - ); - let escaped_value = escape_vdf_string(value); - let _ = writeln!( - result, - "{key_indent}\"{target_key}\"\t\t\"{escaped_value}\"" - ); - for j in (0..sections.len() - matched_depth).rev() { - let close_indent = format!("{}{}", base_indent, "\t".repeat(j)); - let _ = writeln!(result, "{close_indent}}}"); + if !done && !found { + if let Some(block) = + vdf_insert_block(&stack, sections, &escaped_key, &escaped_value, &unit) + { + // The section opened on this very line, so the key + // has to land between the braces rather than in + // front of the line. + if opened_here > 0 { + split = Some((*at, unit.repeat(stack.len().saturating_sub(1)))); + } + pre_insert = block; + done = true; + } } - inserted = true; + pending.clear(); + carried = None; + stack.pop(); } } + } - if depth > 0 { - if matched_depth == depth && matched_depth <= sections.len() && matched_depth > 0 { - matched_depth = matched_depth.saturating_sub(1); - } - depth -= 1; - } - result.push_str(line); - result.push('\n'); - continue; + if !done && found && vdf_is_target_pair(&stack, sections, target_key, &pending) { + rewritten = Some(vdf_rewrite_pair( + line, + &items, + pending_first_ordinal + 1, + &escaped_key, + &escaped_value, + )); + done = true; } - let tokens = vdf_tokenize_line(trimmed); + vdf_carry_header(&pending, &mut carried); - // Track section entry - if !tokens.is_empty() && matched_depth < sections.len() && depth == matched_depth + 1 { - let key = &tokens[0]; - if key.eq_ignore_ascii_case(sections[matched_depth]) { - matched_depth += 1; - result.push_str(line); - result.push('\n'); - continue; + match split { + Some((at, tail_indent)) => { + let head = line[..at].trim_end(); + if !head.is_empty() { + out.push(head.to_string()); + } + out.extend(pre_insert); + out.push(format!("{tail_indent}{}", &line[at..])); + } + None => { + out.extend(pre_insert); + out.push(rewritten.unwrap_or_else(|| line.to_string())); } } - - // Replace existing key value - if !key_replaced - && found - && tokens.len() >= 2 - && matched_depth == sections.len() - && depth == sections.len() + 1 - && tokens[0].eq_ignore_ascii_case(target_key) - { - // Rebuild line preserving original indentation - let leading_whitespace: String = - line.chars().take_while(|c| c.is_whitespace()).collect(); - let escaped_value = escape_vdf_string(value); - let _ = writeln!( - result, - "{leading_whitespace}\"{target_key}\"\t\t\"{escaped_value}\"" - ); - key_replaced = true; - inserted = true; - continue; - } - - result.push_str(line); - result.push('\n'); } - // If the original content didn't end with a newline, remove trailing one - if !content.ends_with('\n') && result.ends_with('\n') { - result.pop(); + if !done { + return Err(crate::error::AppError::FileRead(format!( + "VDF path {} not found and could not be created; nothing was written", + path.join(" > ") + ))); } - result + let mut result = out.join(newline); + if content.ends_with('\n') { + result.push_str(newline); + } + Ok(result) } fn escape_vdf_string(value: &str) -> String { @@ -606,7 +840,8 @@ mod tests { input, &["Software", "Valve", "Steam", "apps", "730", "LaunchOptions"], r#"+exec "autoexec.cfg" -path C:\Steam"#, - ); + ) + .expect("launch options must be written"); assert!(output .contains("\"LaunchOptions\"\t\t\"+exec \\\"autoexec.cfg\\\" -path C:\\\\Steam\"")); @@ -676,7 +911,8 @@ mod tests { input, &["Software", "Valve", "Steam", "apps", "730", "LaunchOptions"], injection, - ); + ) + .expect("launch options must be written"); // The newline is escaped, so no second physical line is produced and // no real PersonaState key leaks into the file. @@ -752,7 +988,8 @@ mod tests { // set_login_user_flags hits this replace branch on every switch, so it // must swap the value without duplicating the key or touching siblings. let input = "\"users\"\n{\n\t\"76561198000000000\"\n\t{\n\t\t\"AccountName\"\t\t\"alice\"\n\t\t\"AllowAutoLogin\"\t\t\"0\"\n\t\t\"MostRecent\"\t\t\"0\"\n\t}\n}\n"; - let output = vdf_set_nested_value(input, &["76561198000000000", "AllowAutoLogin"], "1"); + let output = vdf_set_nested_value(input, &["76561198000000000", "AllowAutoLogin"], "1") + .expect("existing key must be replaced"); // Value replaced in place. assert!(output.contains("\"AllowAutoLogin\"\t\t\"1\"")); @@ -811,4 +1048,178 @@ mod tests { "line ending collapsed to bare LF" ); } + + // ── V8: the write path is structural, like the read path ── + // + // Golden strings captured from the previous (brace-on-its-own-line only) + // implementation before it was replaced. A standalone-brace file must come + // back byte for byte the same, or this fix has changed files it had no + // business changing. + + #[test] + fn set_nested_value_standalone_braces_are_byte_identical() { + let cases: [(&str, &[&str], &str, &str); 5] = [ + // Replace an existing key in a loginusers.vdf-shaped file. + ( + "\"users\"\n{\n\t\"76561198000000000\"\n\t{\n\t\t\"AccountName\"\t\t\"alice\"\n\t\t\"AllowAutoLogin\"\t\t\"0\"\n\t\t\"MostRecent\"\t\t\"0\"\n\t}\n}\n", + &["76561198000000000", "AllowAutoLogin"], + "1", + "\"users\"\n{\n\t\"76561198000000000\"\n\t{\n\t\t\"AccountName\"\t\t\"alice\"\n\t\t\"AllowAutoLogin\"\t\t\"1\"\n\t\t\"MostRecent\"\t\t\"0\"\n\t}\n}\n", + ), + // Insert a key into the empty registry.vdf template. + ( + "\"Registry\"\n{\n\t\"HKCU\"\n\t{\n\t\t\"Software\"\n\t\t{\n\t\t\t\"Valve\"\n\t\t\t{\n\t\t\t\t\"Steam\"\n\t\t\t\t{\n\t\t\t\t}\n\t\t\t}\n\t\t}\n\t}\n}\n", + &["HKCU", "Software", "Valve", "Steam", "AutoLoginUser"], + "alice", + "\"Registry\"\n{\n\t\"HKCU\"\n\t{\n\t\t\"Software\"\n\t\t{\n\t\t\t\"Valve\"\n\t\t\t{\n\t\t\t\t\"Steam\"\n\t\t\t\t{\n\t\t\t\t\t\"AutoLoginUser\"\t\t\"alice\"\n\t\t\t\t}\n\t\t\t}\n\t\t}\n\t}\n}\n", + ), + // Create four missing sections plus the key. + ( + "\"UserLocalConfigStore\"\n{\n\t\"Software\"\n\t{\n\t}\n}\n", + &["Software", "Valve", "Steam", "apps", "730", "LaunchOptions"], + "+exec \"autoexec.cfg\" -path C:\\Steam", + "\"UserLocalConfigStore\"\n{\n\t\"Software\"\n\t{\n\t\t\"Valve\"\n\t\t{\n\t\t\t\"Steam\"\n\t\t\t{\n\t\t\t\t\"apps\"\n\t\t\t\t{\n\t\t\t\t\t\"730\"\n\t\t\t\t\t{\n\t\t\t\t\t\t\"LaunchOptions\"\t\t\"+exec \\\"autoexec.cfg\\\" -path C:\\\\Steam\"\n\t\t\t\t\t}\n\t\t\t\t}\n\t\t\t}\n\t\t}\n\t}\n}\n", + ), + // Insert a missing key into a section that exists. + ( + "\"users\"\n{\n\t\"76561198000000000\"\n\t{\n\t\t\"AccountName\"\t\t\"alice\"\n\t}\n}\n", + &["76561198000000000", "AllowAutoLogin"], + "1", + "\"users\"\n{\n\t\"76561198000000000\"\n\t{\n\t\t\"AccountName\"\t\t\"alice\"\n\t\t\"AllowAutoLogin\"\t\t\"1\"\n\t}\n}\n", + ), + // A file with no trailing newline keeps none. + ( + "\"users\"\n{\n\t\"76561198000000000\"\n\t{\n\t\t\"AccountName\"\t\t\"alice\"\n\t}\n}", + &["76561198000000000", "AllowAutoLogin"], + "1", + "\"users\"\n{\n\t\"76561198000000000\"\n\t{\n\t\t\"AccountName\"\t\t\"alice\"\n\t\t\"AllowAutoLogin\"\t\t\"1\"\n\t}\n}", + ), + ]; + + for (index, (input, path, value, expected)) in cases.iter().enumerate() { + let out = vdf_set_nested_value(input, path, value).expect("golden case must succeed"); + assert_eq!(&out, expected, "golden case {index} drifted"); + } + } + + #[test] + fn set_nested_value_enters_inline_brace_sections() { + // `"key" {` on one line. The old writer walked straight past it, never + // entered the block, and returned the input unchanged with Ok. + let input = "\"users\" {\n\t\"76561198000000000\" {\n\t\t\"AccountName\"\t\t\"alice\"\n\t\t\"AllowAutoLogin\"\t\t\"0\"\n\t}\n}\n"; + let out = vdf_set_nested_value(input, &["76561198000000000", "AllowAutoLogin"], "1") + .expect("inline braces must be walked"); + + assert_eq!( + out, + "\"users\" {\n\t\"76561198000000000\" {\n\t\t\"AccountName\"\t\t\"alice\"\n\t\t\"AllowAutoLogin\"\t\t\"1\"\n\t}\n}\n" + ); + } + + #[test] + fn set_nested_value_inserts_into_an_inline_brace_section() { + let input = + "\"users\" {\n\t\"76561198000000000\" {\n\t\t\"AccountName\"\t\t\"alice\"\n\t}\n}\n"; + let out = vdf_set_nested_value(input, &["76561198000000000", "MostRecent"], "1") + .expect("inline braces must be walked"); + + assert_eq!( + out, + "\"users\" {\n\t\"76561198000000000\" {\n\t\t\"AccountName\"\t\t\"alice\"\n\t\t\"MostRecent\"\t\t\"1\"\n\t}\n}\n" + ); + } + + #[test] + fn set_nested_value_splits_a_section_opened_and_closed_on_one_line() { + // Both braces on the header line: the key cannot go in front of the + // line, it has to land between them. + let input = "\"UserLocalConfigStore\"\n{\n\t\"friends\" { }\n}\n"; + let out = vdf_set_nested_value(input, &["friends", "DoNotDisturb"], "1") + .expect("inline section must accept the key"); + + assert_eq!( + out, + "\"UserLocalConfigStore\"\n{\n\t\"friends\" {\n\t\t\"DoNotDisturb\"\t\t\"1\"\n\t}\n}\n" + ); + } + + #[test] + fn set_nested_value_preserves_crlf() { + let input = "\"users\"\r\n{\r\n\t\"76561198000000000\"\r\n\t{\r\n\t\t\"AllowAutoLogin\"\t\t\"0\"\r\n\t}\r\n}\r\n"; + let out = vdf_set_nested_value(input, &["76561198000000000", "AllowAutoLogin"], "1") + .expect("CRLF file must be written"); + + assert_eq!( + out, + "\"users\"\r\n{\r\n\t\"76561198000000000\"\r\n\t{\r\n\t\t\"AllowAutoLogin\"\t\t\"1\"\r\n\t}\r\n}\r\n" + ); + assert!(!out.contains("\"1\"\n"), "line ending collapsed to bare LF"); + } + + #[test] + fn set_nested_value_preserves_space_indentation() { + let input = "\"users\"\n{\n \"76561198000000000\"\n {\n }\n}\n"; + let out = vdf_set_nested_value(input, &["76561198000000000", "MostRecent"], "1") + .expect("space-indented file must be written"); + + assert_eq!( + out, + "\"users\"\n{\n \"76561198000000000\"\n {\n \"MostRecent\"\t\t\"1\"\n }\n}\n" + ); + } + + #[test] + fn set_nested_value_errors_when_the_path_cannot_be_reached() { + // Empty file: nothing to walk, nowhere to put the key. The old writer + // answered Ok("") and every caller wrote that back happily. + let err = vdf_set_nested_value("", &["friends", "DoNotDisturb"], "1") + .expect_err("an empty file has no path to write into"); + assert!( + err.to_string().contains("friends > DoNotDisturb"), + "the error must name the path: {err}" + ); + + // A file whose root section never opens is the same story. + assert!(vdf_set_nested_value( + "\"UserLocalConfigStore\"\n", + &["friends", "DoNotDisturb"], + "1" + ) + .is_err()); + } + + #[test] + fn set_nested_value_escapes_quotes_and_backslashes_in_the_value() { + let input = "\"users\"\n{\n\t\"76561198000000000\"\n\t{\n\t\t\"AccountName\"\t\t\"alice\"\n\t}\n}\n"; + let out = vdf_set_nested_value( + input, + &["76561198000000000", "Nickname"], + "say \"hi\" from C:\\Steam", + ) + .expect("value must be written"); + + assert!(out.contains("\"Nickname\"\t\t\"say \\\"hi\\\" from C:\\\\Steam\"")); + // Reading it back through the tokenizer round-trips the raw value. + let line = out + .lines() + .find(|l| l.trim_start().starts_with("\"Nickname\"")) + .expect("the key was written"); + assert_eq!( + vdf_tokenize_line(line)[1], + "say \"hi\" from C:\\Steam", + "escaping must round-trip" + ); + } + + #[test] + fn set_nested_value_keeps_a_second_pair_on_the_same_line() { + // Two pairs crammed onto one physical line: rewriting the whole line + // would silently drop the second one. + let input = "\"users\"\n{\n\t\"76561198000000000\"\n\t{\n\t\t\"AllowAutoLogin\"\t\t\"0\"\t\t\"MostRecent\"\t\t\"0\"\n\t}\n}\n"; + let out = vdf_set_nested_value(input, &["76561198000000000", "AllowAutoLogin"], "1") + .expect("value must be written"); + + assert!(out.contains("\"AllowAutoLogin\"\t\t\"1\"")); + assert!(out.contains("\"MostRecent\"\t\t\"0\"")); + } } diff --git a/crates/accshift-core/src/secrets.rs b/crates/accshift-core/src/secrets.rs new file mode 100644 index 0000000..0d921c3 --- /dev/null +++ b/crates/accshift-core/src/secrets.rs @@ -0,0 +1,593 @@ +//! The app's own view of the OS secret store: the calls encrypted snapshots go +//! through, an index of every entry they created, and the collector that frees +//! the ones no snapshot file points at any more. +//! +//! Why an index exists. On Linux and macOS `os::encrypt_bytes` returns a UUID +//! naming a keyring entry that holds the real bytes, so an entry outlives the +//! file pointing at it whenever that file disappears without going through +//! `delete_bytes`: a crash between the two writes, a snapshot directory removed +//! from outside the app, or a bug like the one that removed a captured +//! directory without freeing its entries first. Finding those orphans would +//! mean listing the store, and nothing here can. The `keyring` crate has no +//! list call at all; Secret Service and the macOS Keychain can only search by +//! attributes this code never sets. So every id is written down as it is +//! created, and the collector compares that list against the tokens still on +//! disk. +//! +//! Windows keeps the ciphertext inline through DPAPI and owns no entry, so +//! recording and collecting are both no-ops there. +//! +//! Only the bytes API is indexed. `os::encrypt_secret` (the Roblox cookies, the +//! Steam API key) stores its token in the config file, which the collector +//! never reads, so those ids are deliberately absent from the index and can +//! never be taken for orphans. + +use crate::context::AppContext; +use crate::error::AppError; +use crate::snapshot_crypto::{ENCRYPTED_HEADER, SNAPSHOT_PLATFORM_IDS}; +use crate::storage; +use std::collections::HashSet; +use std::fs; +use std::io::Write; +use std::path::Path; +use std::sync::Mutex; + +// --------------------------------------------------------------------------- +// Backend +// --------------------------------------------------------------------------- + +/// The real backend: `crate::os`, whose bytes API is DPAPI on Windows and the +/// keyring elsewhere. +#[cfg(not(test))] +mod backend { + /// True when a stored secret lives in an OS keyring entry that has to be + /// freed by hand. False under Windows DPAPI, where the ciphertext is the + /// token and owns nothing. + pub const KEYRING_BACKED: bool = cfg!(any(target_os = "linux", target_os = "macos")); + + pub use crate::os::{decrypt_bytes, delete_bytes, encrypt_bytes}; +} + +/// In-memory stand-in for the keyring, so a test can count what a capture +/// leaves behind on a machine whose real backend is DPAPI (which stores +/// nothing) or a headless keyring (which cannot be reached). +/// +/// It mirrors the Linux/macOS backend exactly: storing returns a UUID naming +/// the entry, and the entry lives until something deletes it. The store is per +/// thread, because the test harness gives each test its own thread and a shared +/// store would make every entry count a race between them. +#[cfg(test)] +pub(crate) mod backend { + use crate::error::AppError; + use std::cell::RefCell; + use std::collections::HashMap; + + pub const KEYRING_BACKED: bool = true; + + thread_local! { + static ENTRIES: RefCell>> = RefCell::new(HashMap::new()); + } + + /// How many entries this thread's keyring holds right now. + pub fn entry_count() -> usize { + ENTRIES.with(|entries| entries.borrow().len()) + } + + pub fn encrypt_bytes(data: &[u8]) -> Result, AppError> { + if data.is_empty() { + return Ok(Vec::new()); + } + let id = uuid::Uuid::new_v4().to_string(); + ENTRIES.with(|entries| entries.borrow_mut().insert(id.clone(), data.to_vec())); + Ok(id.into_bytes()) + } + + pub fn decrypt_bytes(token: &[u8]) -> Result, AppError> { + if token.is_empty() { + return Ok(Vec::new()); + } + let id = String::from_utf8_lossy(token).into_owned(); + ENTRIES + .with(|entries| entries.borrow().get(&id).cloned()) + .ok_or_else(|| AppError::ProcessStart(format!("keyring: no entry named {id}"))) + } + + pub fn delete_bytes(token: &[u8]) -> Result<(), AppError> { + if token.is_empty() { + return Ok(()); + } + let id = String::from_utf8_lossy(token).into_owned(); + ENTRIES.with(|entries| entries.borrow_mut().remove(&id)); + Ok(()) + } +} + +// --------------------------------------------------------------------------- +// The calls snapshots go through +// --------------------------------------------------------------------------- + +/// Store bytes and hand back the token that reads them again. The token is the +/// ciphertext on Windows and a keyring entry id elsewhere; either way it is +/// what ends up in the snapshot file after the header. +pub fn encrypt_bytes(data: &[u8]) -> Result, AppError> { + let token = backend::encrypt_bytes(data)?; + record(&token); + Ok(token) +} + +/// Read back what [`encrypt_bytes`] stored. +pub fn decrypt_bytes(token: &[u8]) -> Result, AppError> { + backend::decrypt_bytes(token) +} + +/// Free what [`encrypt_bytes`] stored. A missing entry is a success, so forget +/// flows stay idempotent. +/// +/// The id is not struck from the index here: a single directory snapshot can +/// hold thousands of files, and rewriting the whole index per file would turn a +/// forget into minutes of IO. The next [`gc`] compacts it instead, off the +/// tokens actually on disk, which is the same answer with one rewrite. +pub fn delete_bytes(token: &[u8]) -> Result<(), AppError> { + backend::delete_bytes(token) +} + +// --------------------------------------------------------------------------- +// The index +// --------------------------------------------------------------------------- + +/// Where the index lives, once the app has said which state directory is its +/// own. A process that never calls [`init`] still stores and reads secrets; its +/// entries are simply not collectable. +/// +/// Per thread in a test build, so tests running side by side never share one +/// index file. Process-global everywhere else. +#[cfg(not(test))] +mod index_path { + use std::path::PathBuf; + use std::sync::Mutex; + + static PATH: Mutex> = Mutex::new(None); + + pub fn set(path: PathBuf) { + *PATH.lock().unwrap_or_else(|e| e.into_inner()) = Some(path); + } + + pub fn get() -> Option { + PATH.lock().unwrap_or_else(|e| e.into_inner()).clone() + } +} + +/// See the module above. +#[cfg(test)] +mod index_path { + use std::cell::RefCell; + use std::path::PathBuf; + + thread_local! { + static PATH: RefCell> = const { RefCell::new(None) }; + } + + pub fn set(path: PathBuf) { + PATH.with(|slot| *slot.borrow_mut() = Some(path)); + } + + pub fn get() -> Option { + PATH.with(|slot| slot.borrow().clone()) + } +} + +/// Ids the index already held when this process started. Only these are ever +/// swept: an entry created by this process may well have been written before +/// its file was, and collecting it because the walk ran in between would leave +/// a snapshot that can no longer be decrypted. +#[cfg(not(test))] +static SWEEP_CANDIDATES: Mutex>> = Mutex::new(None); + +// See above. Per thread in a test build for the same reason as the index path. +#[cfg(test)] +thread_local! { + static SWEEP_CANDIDATES_TL: std::cell::RefCell>> = + const { std::cell::RefCell::new(None) }; +} + +#[cfg(not(test))] +fn set_sweep_candidates(ids: Vec) { + *SWEEP_CANDIDATES.lock().unwrap_or_else(|e| e.into_inner()) = Some(ids); +} + +#[cfg(not(test))] +fn take_sweep_candidates() -> Option> { + SWEEP_CANDIDATES + .lock() + .unwrap_or_else(|e| e.into_inner()) + .take() +} + +#[cfg(test)] +fn set_sweep_candidates(ids: Vec) { + SWEEP_CANDIDATES_TL.with(|slot| *slot.borrow_mut() = Some(ids)); +} + +#[cfg(test)] +fn take_sweep_candidates() -> Option> { + SWEEP_CANDIDATES_TL.with(|slot| slot.borrow_mut().take()) +} + +/// Serialises the appends within this process. Two processes appending one +/// short line each is the remaining race, and an interleaved line is dropped by +/// the reader below rather than mistaken for an id. +static INDEX_LOCK: Mutex<()> = Mutex::new(()); + +/// Point the index at the app's state directory, and take the snapshot of ids +/// [`gc`] is allowed to sweep. +/// +/// Call it once, before anything can capture a snapshot. A no-op on Windows. +pub fn init(app: &dyn AppContext) { + if !backend::KEYRING_BACKED { + return; + } + let Ok(path) = storage::secrets_index_path(app) else { + return; + }; + let existing = read_index(&path); + index_path::set(path); + set_sweep_candidates(existing); +} + +fn record(token: &[u8]) { + if !backend::KEYRING_BACKED || token.is_empty() { + return; + } + let Ok(id) = std::str::from_utf8(token) else { + return; + }; + let Some(path) = index_path::get() else { + return; + }; + let _guard = INDEX_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + if let Some(parent) = path.parent() { + let _ = fs::create_dir_all(parent); + } + if let Ok(mut file) = fs::OpenOptions::new().create(true).append(true).open(&path) { + let _ = writeln!(file, "{id}"); + } +} + +/// The ids the index holds, in order, without duplicates. A line that is not a +/// plausible entry id is skipped: the file is append-only from possibly two +/// processes, so a torn line is a thing that can happen. +fn read_index(path: &Path) -> Vec { + let Ok(text) = fs::read_to_string(path) else { + return Vec::new(); + }; + let mut seen = HashSet::new(); + text.lines() + .map(str::trim) + .filter(|line| looks_like_entry_id(line)) + .filter(|line| seen.insert(line.to_string())) + .map(str::to_string) + .collect() +} + +/// A UUID as `encrypt_bytes` writes it: 36 characters of hex and dashes. +fn looks_like_entry_id(line: &str) -> bool { + line.len() == 36 && line.chars().all(|c| c.is_ascii_hexdigit() || c == '-') +} + +fn write_index(path: &Path, ids: &[String]) -> Result<(), String> { + if let Some(parent) = path.parent() { + fs::create_dir_all(parent).map_err(|e| format!("dir={} error={e}", parent.display()))?; + } + let mut body = String::with_capacity(ids.len() * 37); + for id in ids { + body.push_str(id); + body.push('\n'); + } + fs::write(path, body).map_err(|e| format!("file={} error={e}", path.display())) +} + +// --------------------------------------------------------------------------- +// Collection +// --------------------------------------------------------------------------- + +/// Outcome of one collection pass. +#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] +pub struct GcStats { + /// Entries no snapshot file pointed at any more, now freed. + pub freed: usize, + /// Entries that could not be freed. They stay indexed for the next run. + pub failed: usize, +} + +impl GcStats { + /// True when the pass had anything to report. A clean store returns false, + /// which is the normal case on every launch. + pub fn touched_anything(&self) -> bool { + self.freed > 0 || self.failed > 0 + } +} + +/// Free every indexed keyring entry no snapshot file points at, then compact +/// the index down to what is actually live. +/// +/// A no-op on Windows, and a no-op in a process that never called [`init`]. +/// The sweep is abandoned rather than guessed at when the snapshot store cannot +/// be read end to end: an incomplete live set would look exactly like a store +/// full of orphans, and freeing those would leave every snapshot undecryptable. +pub fn gc(app: &dyn AppContext, report: &mut dyn FnMut(&str, String)) -> GcStats { + let mut stats = GcStats::default(); + if !backend::KEYRING_BACKED { + return stats; + } + let Some(path) = index_path::get() else { + return stats; + }; + let Some(candidates) = take_sweep_candidates() else { + // Already swept once in this process. Running again would only risk the + // entries created since. + return stats; + }; + if candidates.is_empty() { + return stats; + } + + let live = match live_tokens(app) { + Ok(live) => live, + Err(detail) => { + report( + "Could not read the snapshot store, skipped the keyring sweep", + detail, + ); + return stats; + } + }; + + let mut freed: HashSet<&str> = HashSet::new(); + for id in &candidates { + if live.contains(id) { + continue; + } + match backend::delete_bytes(id.as_bytes()) { + Ok(()) => { + stats.freed += 1; + freed.insert(id.as_str()); + } + Err(e) => { + stats.failed += 1; + report( + "Could not free an orphaned keyring entry", + format!("error={e}"), + ); + } + } + } + + // Compact off what the index holds now, so ids recorded while the walk ran + // survive, then adopt any live token the index never knew about (a snapshot + // captured by a build that predates this file). + let mut keep: Vec = read_index(&path) + .into_iter() + .filter(|id| !freed.contains(id.as_str())) + .collect(); + let known: HashSet = keep.iter().cloned().collect(); + let mut adopted: Vec = live.into_iter().filter(|id| !known.contains(id)).collect(); + adopted.sort(); + keep.append(&mut adopted); + if let Err(detail) = write_index(&path, &keep) { + report("Could not rewrite the keyring entry index", detail); + } + stats +} + +/// Every token the snapshot store still points at, across all platforms. +fn live_tokens(app: &dyn AppContext) -> Result, String> { + let mut out = HashSet::new(); + for platform_id in SNAPSHOT_PLATFORM_IDS { + let dir = storage::platform_snapshots_dir(app, platform_id) + .map_err(|detail| format!("platform={platform_id} error={detail}"))?; + collect_tokens(&dir, &mut out)?; + } + Ok(out) +} + +fn collect_tokens(dir: &Path, out: &mut HashSet) -> Result<(), String> { + let entries = match fs::read_dir(dir) { + Ok(entries) => entries, + // A platform that never captured anything has no directory. + Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(()), + Err(e) => return Err(format!("dir={} error={e}", dir.display())), + }; + for entry in entries { + let entry = entry.map_err(|e| format!("dir={} error={e}", dir.display()))?; + if crate::fs_utils::is_reparse_point(&entry) { + continue; + } + let path = entry.path(); + let Ok(file_type) = entry.file_type() else { + continue; + }; + if file_type.is_dir() { + collect_tokens(&path, out)?; + continue; + } + if !file_type.is_file() { + continue; + } + let data = fs::read(&path).map_err(|e| format!("file={} error={e}", path.display()))?; + let Some(token) = data.strip_prefix(ENCRYPTED_HEADER) else { + // Legacy plaintext, it owns no entry. + continue; + }; + if let Ok(id) = std::str::from_utf8(token) { + out.insert(id.to_string()); + } + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::snapshot_crypto::{encrypted_copy_file, ENCRYPTED_HEADER}; + use std::path::PathBuf; + + struct TempCtx { + root: PathBuf, + } + + impl AppContext for TempCtx { + fn app_config_dir(&self) -> Result { + Ok(self.root.clone()) + } + fn app_data_dir(&self) -> Result { + Ok(self.root.clone()) + } + fn app_local_data_dir(&self) -> Result { + Ok(self.root.clone()) + } + fn app_cache_dir(&self) -> Result { + Ok(self.root.clone()) + } + } + + fn scratch(tag: &str) -> PathBuf { + let root = std::env::temp_dir().join(format!( + "accshift-secrets-{}-{}-{:?}", + tag, + std::process::id(), + std::thread::current().id() + )); + let _ = fs::remove_dir_all(&root); + fs::create_dir_all(&root).unwrap(); + root + } + + /// Captures one file into the platform's snapshot directory the way the + /// engine does, and returns the entry id it now owns. + fn capture(ctx: &TempCtx, name: &str, body: &[u8]) -> String { + let source = ctx.root.join("live").join(name); + fs::create_dir_all(source.parent().unwrap()).unwrap(); + fs::write(&source, body).unwrap(); + let dest = storage::platform_snapshots_dir(ctx, "gog") + .unwrap() + .join(name); + encrypted_copy_file(&source, &dest).unwrap(); + let stored = fs::read(&dest).unwrap(); + String::from_utf8(stored[ENCRYPTED_HEADER.len()..].to_vec()).unwrap() + } + + #[test] + fn an_id_is_indexed_when_the_entry_is_created() { + let root = scratch("index-record"); + let ctx = TempCtx { root: root.clone() }; + init(&ctx); + + let id = capture(&ctx, "session.json", b"one"); + + let index = read_index(&storage::secrets_index_path(&ctx).unwrap()); + assert_eq!(index, vec![id]); + let _ = fs::remove_dir_all(&root); + } + + #[test] + fn gc_frees_the_entry_of_a_snapshot_file_that_is_gone() { + let root = scratch("gc-orphan"); + let ctx = TempCtx { root: root.clone() }; + init(&ctx); + + let kept = capture(&ctx, "kept.json", b"kept"); + let orphaned = capture(&ctx, "orphaned.json", b"orphaned"); + assert_eq!(backend::entry_count(), 2); + + // The file disappears without going through delete_bytes, which is + // exactly the shape of the leak this collector exists for. + let snapshots = storage::platform_snapshots_dir(&ctx, "gog").unwrap(); + fs::remove_file(snapshots.join("orphaned.json")).unwrap(); + + // The sweep candidates are taken at init, so the entries this test just + // created are only eligible after a second init. + init(&ctx); + let mut failures: Vec = Vec::new(); + let stats = gc(&ctx, &mut |message, detail| { + failures.push(format!("{message} ({detail})")) + }); + + assert_eq!(failures, Vec::::new()); + assert_eq!( + stats, + GcStats { + freed: 1, + failed: 0 + } + ); + assert_eq!(backend::entry_count(), 1); + assert!(decrypt_bytes(kept.as_bytes()).is_ok()); + assert!(decrypt_bytes(orphaned.as_bytes()).is_err()); + assert_eq!( + read_index(&storage::secrets_index_path(&ctx).unwrap()), + vec![kept] + ); + let _ = fs::remove_dir_all(&root); + } + + #[test] + fn gc_keeps_every_entry_a_snapshot_still_points_at() { + let root = scratch("gc-live"); + let ctx = TempCtx { root: root.clone() }; + init(&ctx); + + capture(&ctx, "one.json", b"one"); + capture(&ctx, "two.json", b"two"); + + init(&ctx); + let stats = gc(&ctx, &mut |_, _| {}); + + assert_eq!(stats, GcStats::default()); + assert_eq!(backend::entry_count(), 2); + let _ = fs::remove_dir_all(&root); + } + + #[test] + fn gc_does_nothing_when_the_snapshot_store_cannot_be_read() { + // An unreadable store looks exactly like a store full of orphans, and + // acting on that would free the entries of every snapshot the user has. + let root = scratch("gc-unreadable"); + let ctx = TempCtx { root: root.clone() }; + init(&ctx); + capture(&ctx, "one.json", b"one"); + + // A file where the snapshot directory of another platform belongs makes + // the walk fail with something other than "not found". + let jagex = storage::platform_snapshots_dir(&ctx, "jagex").unwrap(); + fs::create_dir_all(jagex.parent().unwrap()).unwrap(); + fs::write(&jagex, b"not a directory").unwrap(); + + init(&ctx); + let mut messages: Vec = Vec::new(); + let stats = gc(&ctx, &mut |message, _| messages.push(message.to_string())); + + assert_eq!(stats, GcStats::default()); + assert_eq!(backend::entry_count(), 1); + assert_eq!( + messages, + vec!["Could not read the snapshot store, skipped the keyring sweep"] + ); + let _ = fs::remove_dir_all(&root); + } + + #[test] + fn a_torn_index_line_is_not_taken_for_an_entry_id() { + let root = scratch("index-torn"); + let path = root.join("secret-entries.txt"); + fs::write( + &path, + "0f9d4a2b-1c3e-4d5f-8a7b-6c5d4e3f2a1b\nhalf-a-line\n\n", + ) + .unwrap(); + assert_eq!( + read_index(&path), + vec!["0f9d4a2b-1c3e-4d5f-8a7b-6c5d4e3f2a1b"] + ); + let _ = fs::remove_dir_all(&root); + } +} diff --git a/crates/accshift-core/src/snapshot_crypto.rs b/crates/accshift-core/src/snapshot_crypto.rs index 007ee63..d0773ca 100644 --- a/crates/accshift-core/src/snapshot_crypto.rs +++ b/crates/accshift-core/src/snapshot_crypto.rs @@ -2,15 +2,15 @@ //! //! Every platform that captures auth material to disk (Riot, Ubisoft, Epic, //! GOG, Jagex, Discord) stores it in the same on-disk format: a 4-byte magic -//! header followed by the output of `os::encrypt_bytes` (DPAPI ciphertext on -//! Windows, a keyring token on Linux/macOS). Files without the header are -//! legacy plaintext snapshots and pass through reads unchanged. +//! header followed by the output of [`crate::secrets::encrypt_bytes`] (DPAPI +//! ciphertext on Windows, a keyring token on Linux/macOS). Files without the +//! header are legacy plaintext snapshots and pass through reads unchanged. //! //! The format is load-bearing: snapshots written by older builds must keep -//! decrypting, so the header, key derivation (delegated to `crate::os`) and -//! layout must not change. +//! decrypting, so the header, key derivation (delegated to [`crate::secrets`], +//! and through it to `crate::os`) and layout must not change. -use crate::os; +use crate::secrets; use crate::AppContext; use std::fs; use std::path::{Path, PathBuf}; @@ -47,7 +47,7 @@ pub struct DirCopyOptions<'a> { /// elsewhere). The on-disk snapshot is never plaintext auth material. pub fn encrypted_copy_file(source: &Path, dest: &Path) -> Result<(), String> { let data = fs::read(source).map_err(|e| format!("Could not read {}: {e}", source.display()))?; - let encrypted = os::encrypt_bytes(&data) + let encrypted = secrets::encrypt_bytes(&data) .map_err(|e| format!("Could not encrypt {}: {e}", source.display()))?; if let Some(parent) = dest.parent() { fs::create_dir_all(parent) @@ -64,7 +64,7 @@ pub fn encrypted_copy_file(source: &Path, dest: &Path) -> Result<(), String> { pub fn decrypted_copy_file(source: &Path, dest: &Path) -> Result<(), String> { let data = fs::read(source).map_err(|e| format!("Could not read {}: {e}", source.display()))?; let content = if data.starts_with(ENCRYPTED_HEADER) { - os::decrypt_bytes(&data[ENCRYPTED_HEADER.len()..]) + secrets::decrypt_bytes(&data[ENCRYPTED_HEADER.len()..]) .map_err(|e| format!("Could not decrypt {}: {e}", source.display()))? } else { data @@ -78,7 +78,7 @@ pub fn decrypted_copy_file(source: &Path, dest: &Path) -> Result<(), String> { /// Encrypt raw bytes and write them with the header (no temp plaintext on disk). pub fn write_encrypted_bytes(dest: &Path, data: &[u8]) -> Result<(), String> { - let encrypted = os::encrypt_bytes(data) + let encrypted = secrets::encrypt_bytes(data) .map_err(|e| format!("Could not encrypt {}: {e}", dest.display()))?; if let Some(parent) = dest.parent() { fs::create_dir_all(parent) @@ -95,7 +95,7 @@ pub fn write_encrypted_bytes(dest: &Path, data: &[u8]) -> Result<(), String> { pub fn read_decrypted_bytes(path: &Path) -> Result, String> { let raw = fs::read(path).map_err(|e| format!("Could not read {}: {e}", path.display()))?; if raw.starts_with(ENCRYPTED_HEADER) { - os::decrypt_bytes(&raw[ENCRYPTED_HEADER.len()..]) + secrets::decrypt_bytes(&raw[ENCRYPTED_HEADER.len()..]) .map_err(|e| format!("Could not decrypt {}: {e}", path.display())) } else { Ok(raw) @@ -110,7 +110,7 @@ pub fn delete_encrypted_file_secret(path: &Path) { return; }; if data.starts_with(ENCRYPTED_HEADER) { - let _ = os::delete_bytes(&data[ENCRYPTED_HEADER.len()..]); + let _ = secrets::delete_bytes(&data[ENCRYPTED_HEADER.len()..]); } } @@ -241,7 +241,7 @@ pub fn free_dir_secrets_with_errors(dir: &Path, report: &mut dyn FnMut(&str, Str continue; } let token = &data[ENCRYPTED_HEADER.len()..]; - if let Err(e) = os::delete_bytes(token) { + if let Err(e) = secrets::delete_bytes(token) { report( "Could not free keyring entry for snapshot file", format!("file={} error={e}", path.display()), @@ -308,7 +308,7 @@ pub fn upgrade_legacy_plaintext_file(path: &Path) -> Result { return Ok(false); } - let encrypted = os::encrypt_bytes(&data) + let encrypted = secrets::encrypt_bytes(&data) .map_err(|e| format!("Could not encrypt {}: {e}", path.display()))?; let mut out = Vec::with_capacity(ENCRYPTED_HEADER.len() + encrypted.len()); out.extend_from_slice(ENCRYPTED_HEADER); @@ -319,12 +319,12 @@ pub fn upgrade_legacy_plaintext_file(path: &Path) -> Result { let _ = fs::remove_file(&tmp); // On Linux/macOS the ciphertext is a keyring pointer, so a token that // never reached a file would leak an entry. Release it. - let _ = os::delete_bytes(&out[ENCRYPTED_HEADER.len()..]); + let _ = secrets::delete_bytes(&out[ENCRYPTED_HEADER.len()..]); return Err(format!("Could not write {}: {e}", tmp.display())); } if let Err(e) = fs::rename(&tmp, path) { let _ = fs::remove_file(&tmp); - let _ = os::delete_bytes(&out[ENCRYPTED_HEADER.len()..]); + let _ = secrets::delete_bytes(&out[ENCRYPTED_HEADER.len()..]); return Err(format!("Could not replace {}: {e}", path.display())); } Ok(true) diff --git a/crates/accshift-core/src/storage.rs b/crates/accshift-core/src/storage.rs index e608ef2..b6f3936 100644 --- a/crates/accshift-core/src/storage.rs +++ b/crates/accshift-core/src/storage.rs @@ -124,6 +124,27 @@ pub fn local_config_path(app_handle: &dyn AppContext) -> Result Ok(target) } +/// Where the list of OS keyring entry ids the app created lives. It sits in +/// the state directory next to the local config, because like it, it describes +/// this machine and never moves with the user's data. +pub fn secrets_index_path(app_handle: &dyn AppContext) -> Result { + Ok(app_local_data_root(app_handle)? + .join("state") + .join("secret-entries.txt")) +} + +/// Where a Riot restore stages the encrypted copy of the live session it may +/// have to put back. It sits in the state directory next to the local config, +/// because like it, it describes this machine and never moves with the user's +/// data. Each restore creates one subdirectory in it and removes it again on +/// every exit path; a leftover means the process died mid-restore and is swept +/// on the next launch. +pub fn riot_rollback_dir(app_handle: &dyn AppContext) -> Result { + Ok(app_local_data_root(app_handle)? + .join("state") + .join("riot-rollback")) +} + pub fn legacy_config_path(app_handle: &dyn AppContext) -> Result { Ok(legacy_app_data_root(app_handle)?.join("config.json")) } diff --git a/crates/accshift-core/src/telemetry/events.rs b/crates/accshift-core/src/telemetry/events.rs index 93ea5a8..bd0a26c 100644 --- a/crates/accshift-core/src/telemetry/events.rs +++ b/crates/accshift-core/src/telemetry/events.rs @@ -151,6 +151,8 @@ pub const ERROR_CODES: &[&str] = &[ "crypto", // Updater flow. "check_failed", + "update_target_missing", + "update_manifest_invalid", "download_failed", "install_failed", "relaunch_failed", diff --git a/deny.toml b/deny.toml new file mode 100644 index 0000000..d56ef7d --- /dev/null +++ b/deny.toml @@ -0,0 +1,110 @@ +# cargo-deny configuration for the accshift workspace. +# +# Run with `cargo deny check`. It reads the lockfile only, so it never +# compiles anything and is cheap enough to run on every branch. + +[graph] +# Check every target the workspace can be built for, not just the host. The +# release workflow builds Windows, Linux and macOS, and each one pulls a +# different half of the dependency tree (zbus on Linux, objc2 on macOS, +# windows-sys on Windows). Leaving `targets` empty keeps all of them in scope. +targets = [] +all-features = false + +[licenses] +# Explicit allow-list, no wildcard: a new dependency arriving under a copyleft +# or a non-OSI licence has to be looked at by a human before it lands. +# +# Every entry below is a licence cargo-deny actually reported for the current +# lockfile. Crates offering a choice (`MIT OR Apache-2.0` and friends) are +# satisfied by one of these, so licences that only ever appear as the third +# arm of such a choice are deliberately absent. +allow = [ + "0BSD", + "Apache-2.0", + "Apache-2.0 WITH LLVM-exception", + "BSD-3-Clause", + "BSL-1.0", + "CC0-1.0", + "CDLA-Permissive-2.0", + "ISC", + "MIT", + "MPL-2.0", + "Unicode-3.0", + "Unlicense", + "Zlib", +] +confidence-threshold = 0.8 + +# The three workspace members carry no `license` field of their own. They are +# never published to a registry and the repository ships a single MIT LICENSE +# at its root, which is what they are covered by. +[[licenses.clarify]] +crate = "accshift-gui" +expression = "MIT" +license-files = [] + +[[licenses.clarify]] +crate = "accshift-core" +expression = "MIT" +license-files = [] + +[[licenses.clarify]] +crate = "accshift-cli" +expression = "MIT" +license-files = [] + +[bans] +multiple-versions = "allow" +# The only wildcard requirements in this graph are the path dependencies +# between workspace members, which carry no version by design. +# `allow-wildcard-paths` does not cover them, since it only applies to crates +# marked `publish = false`. +wildcards = "allow" + +[sources] +unknown-registry = "deny" +unknown-git = "deny" +allow-registry = ["https://github.com/rust-lang/crates.io-index"] + +[advisories] +version = 2 +# Unmaintained advisories stay switched on for the whole graph, so a new one +# shows up instead of passing silently. Vulnerability, unsound and yanked +# checks are untouched. +unmaintained = "all" +yanked = "deny" +ignore = [ + # Every entry below is an `unmaintained` advisory on a crate we never depend + # on directly. All of them arrive through Tauri, none has a fixed release to + # upgrade to, and none can be dropped without Tauri dropping it first. + # Reviewed 2026-09-03; revisit when Tauri moves off GTK3 and off urlpattern. + + # unic-*: accshift-gui -> tauri 2.11.5 -> tauri-utils 2.9.3 -> urlpattern + # 0.3.0 -> unic-ucd-ident 0.9.0 -> unic-ucd-version / unic-common / + # unic-char-property / unic-char-range. The whole family was archived at + # once, so it is five advisories for one dependency. + { id = "RUSTSEC-2025-0075", reason = "unic-char-range unmaintained, transitive through tauri -> tauri-utils -> urlpattern -> unic-ucd-ident, reviewed 2026-09-03" }, + { id = "RUSTSEC-2025-0080", reason = "unic-common unmaintained, transitive through tauri -> tauri-utils -> urlpattern -> unic-ucd-ident, reviewed 2026-09-03" }, + { id = "RUSTSEC-2025-0081", reason = "unic-char-property unmaintained, transitive through tauri -> tauri-utils -> urlpattern -> unic-ucd-ident, reviewed 2026-09-03" }, + { id = "RUSTSEC-2025-0098", reason = "unic-ucd-version unmaintained, transitive through tauri -> tauri-utils -> urlpattern -> unic-ucd-ident, reviewed 2026-09-03" }, + { id = "RUSTSEC-2025-0100", reason = "unic-ucd-ident unmaintained, transitive through tauri -> tauri-utils -> urlpattern, reviewed 2026-09-03" }, + + # gtk-rs GTK3 bindings, Linux builds only: + # accshift-gui -> tauri -> tauri-runtime-wry -> tao 0.35.3 / wry 0.55.1 -> + # gtk 0.18 and its -sys crates. Tauri 2 has no GTK4 backend yet. + { id = "RUSTSEC-2024-0411", reason = "gtk-rs GTK3 bindings unmaintained, transitive through tauri -> tauri-runtime-wry -> tao, reviewed 2026-09-03" }, + { id = "RUSTSEC-2024-0412", reason = "gtk-rs GTK3 bindings unmaintained, transitive through tauri -> tauri-runtime-wry -> tao, reviewed 2026-09-03" }, + { id = "RUSTSEC-2024-0413", reason = "gtk-rs GTK3 bindings unmaintained, transitive through tauri -> tauri-runtime-wry -> tao, reviewed 2026-09-03" }, + { id = "RUSTSEC-2024-0414", reason = "gtk-rs GTK3 bindings unmaintained, transitive through tauri -> tauri-runtime-wry -> tao, reviewed 2026-09-03" }, + { id = "RUSTSEC-2024-0415", reason = "gtk-rs GTK3 bindings unmaintained, transitive through tauri -> tauri-runtime-wry -> tao, reviewed 2026-09-03" }, + { id = "RUSTSEC-2024-0416", reason = "gtk-rs GTK3 bindings unmaintained, transitive through tauri -> tauri-runtime-wry -> tao, reviewed 2026-09-03" }, + { id = "RUSTSEC-2024-0417", reason = "gtk-rs GTK3 bindings unmaintained, transitive through tauri -> tauri-runtime-wry -> tao, reviewed 2026-09-03" }, + { id = "RUSTSEC-2024-0418", reason = "gtk-rs GTK3 bindings unmaintained, transitive through tauri -> tauri-runtime-wry -> tao, reviewed 2026-09-03" }, + { id = "RUSTSEC-2024-0419", reason = "gtk-rs GTK3 bindings unmaintained, transitive through tauri -> tauri-runtime-wry -> tao, reviewed 2026-09-03" }, + { id = "RUSTSEC-2024-0420", reason = "gtk-rs GTK3 bindings unmaintained, transitive through tauri -> tauri-runtime-wry -> tao, reviewed 2026-09-03" }, + + # Build-time proc macro of those same GTK3 bindings: + # tao -> gtk 0.18 -> glib-macros 0.18.5 -> proc-macro-error 1.0.4. + { id = "RUSTSEC-2024-0370", reason = "proc-macro-error unmaintained, build-time transitive through tao -> gtk -> glib-macros, reviewed 2026-09-03" }, +] diff --git a/docs/analytics.md b/docs/analytics.md index afe92ac..38985a8 100644 --- a/docs/analytics.md +++ b/docs/analytics.md @@ -13,11 +13,13 @@ of this page existing in a git repository. Settings, Privacy. Two switches, both off means nothing is ever sent again. -Nothing at all is sent before you finish the first-launch screen. After it, the -anonymous counters are on: that screen asks about the enhanced tier, not about -the counters, so turning those off is a separate and deliberate action. Said -plainly, without dressing it up: **the anonymous tier is opt-out, the enhanced -tier is opt-in.** +Nothing at all is sent before you finish the first-launch screen. That screen +offers three answers: the anonymous counters alone, the counters plus the +enhanced tier, or nothing at all. Skipping the tour lands on the first one; +closing the app without answering sends nothing and asks again next launch. +Said plainly, without dressing it up: +**the anonymous tier is opt-out, the enhanced tier is opt-in**, and the third +button on that screen switches both off without a trip to the settings. The `accshift` command-line binary reads the same two switches. It reports one event per command, it never sends a daily ping (a command you run five times is diff --git a/docs/cli.md b/docs/cli.md index 8bb9280..85cf665 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -38,8 +38,13 @@ accshift switch [--launch-options "..."] accshift dry-run accshift descriptors # what the user descriptor folder holds +accshift diag # logs, explain, check, level, bundle, schema ``` +Every one of these needs the "Allow the accshift CLI" toggle in the app +(Settings > General > Integrations). With it off they all exit 7 and do +nothing, `diag` included. + `--graceful` asks the launcher to close itself and waits for it, which is what you want by default because a launcher killed mid-write can corrupt its own config. `--force` terminates it instead, for the cases where it will not go. @@ -156,3 +161,12 @@ and can be turned off entirely from Settings. An automated pipeline that starts returning 7 has not broken, it has been switched off on purpose. What the PIN lock does and does not protect is covered in the [security policy](../.github/SECURITY.md). + +Code 7 covers every subcommand, `platforms`, `descriptors` and all of `diag` +included, and the check runs before the command is handed its arguments, so a +refused run reads nothing and writes nothing. There is no exemption list: the +`CLI_GATE_EXEMPT` const in `crates/accshift-cli/src/main.rs` is empty on +purpose. Until 1.0.4 the toggle only covered `list`, `switch` and `dry-run`, so +`diag bundle` still wrote a report carrying the redacted config summary on a +machine whose owner had switched the CLI off. Diagnostics stay reachable +without the CLI: the app has its own diagnostics screen. diff --git a/docs/platform-descriptors.md b/docs/platform-descriptors.md index e4cef38..a764488 100644 --- a/docs/platform-descriptors.md +++ b/docs/platform-descriptors.md @@ -78,6 +78,15 @@ roots. A path that resolves outside them is refused at run time, and a template containing a `..` segment is refused at load time. `roots.registry` does the same for registry keys, as `{ "root": "HKCU", "key": "Software\\Acme" }`. +A root is written against a short list of placeholders: `${installDir}` plus the +well-known per-user and machine-wide directories of the OS the profile targets +(`APPDATA`, `LOCALAPPDATA`, `ProgramData`, `ProgramFiles`, `ProgramFiles(x86)`, +`PUBLIC`, `SystemDrive`, `USERPROFILE` on Windows; `HOME` and the `XDG_*` ones +elsewhere). Anything else is refused at load. At run time every declared root +must resolve on the machine: one that does not stops the operation with the root +named, rather than being dropped, because a sandbox with no roots left would +allow every path there is. + This is the field to get right first. It is the only thing standing between a descriptor and the rest of the user's disk. @@ -134,9 +143,12 @@ matter: - `clearOnSetup`: deleted when a setup flow clears the live session. - `removeLiveBeforeRestore`: for hidden or system files, which cannot be truncated in place on Windows. -- `clearSnapshotWhenSourceMissing` (default true): drops a stale snapshot when - the live file is gone, so a later restore cannot resurrect another account's - file. +- `clearSnapshotWhenSourceMissing` (default true), on files, directories and + registry values alike: drops a stale snapshot when the live source is gone at + capture time, so a later restore cannot resurrect another account's session. + Set it to false where a missing source means the launcher has not written it + back yet rather than the account signing out, or the capture throws away the + only copy the user has. `caches` are wiped after the incoming session is in place and never captured, and `captureWhen` guards the capture itself: a session the user signed out of by hand diff --git a/docs/theming.md b/docs/theming.md index 95c2c27..04981b3 100644 --- a/docs/theming.md +++ b/docs/theming.md @@ -167,6 +167,13 @@ split into channels before it reaches CSS. | `warning` | `--warning` | colour | 2 | Warnings and degraded states. | | `danger` | `--danger` | colour | 1 | Errors and destructive actions. | +A theme does not set `--accent-text`: the stylesheet derives it from `accent` +and `fg`, and the interface uses it wherever the accent carries small text or a +hairline icon instead of filling a shape. The contrast target below asks +`accent` for 3:1 against a card, which is a fill ratio, not a body-copy one, so +a raw accent used as 11px text would sit under the readable line on plenty of +otherwise valid themes. + ### Shape and density | Token | CSS | Kind | Since | Role | diff --git a/server/package.json b/server/package.json index 16cedbe..07efad8 100644 --- a/server/package.json +++ b/server/package.json @@ -5,6 +5,7 @@ "type": "module", "scripts": { "dev": "wrangler dev", + "test": "node ../node_modules/vitest/vitest.mjs run --root .. server/src", "deploy": "wrangler deploy", "tail": "wrangler tail" }, diff --git a/server/src/index.test.ts b/server/src/index.test.ts index ae219d8..582a6a8 100644 --- a/server/src/index.test.ts +++ b/server/src/index.test.ts @@ -1,10 +1,12 @@ import { describe, expect, it } from "vitest"; import { buildBatch, + cors, eventTimestamp, maskIp, readJsonCapped, redactUuids, + type Env, type TelemetryEvent, } from "./index"; @@ -303,3 +305,37 @@ describe("maskIp", () => { expect(maskIp("not-an-ip")).toBe("unknown"); }); }); + +describe("cors", () => { + const env = { ALLOWED_ORIGINS: "https://accshift.app,https://dash.accshift.app" } as Env; + + function corsHeaders(origin?: string): Headers { + const headers = new Headers(); + if (origin !== undefined) headers.set("Origin", origin); + const request = new Request("https://telemetry.invalid/track", { method: "POST", headers }); + return cors(new Response(null, { status: 204 }), request, env).headers; + } + + it("omits the allow-origin header entirely when the request carries no Origin", () => { + const headers = corsHeaders(); + + expect(headers.has("Access-Control-Allow-Origin")).toBe(false); + // The rest of the preflight answer still has to be there. + expect(headers.get("Access-Control-Allow-Methods")).toBe("GET, POST, OPTIONS"); + }); + + it("refuses the literal null origin a sandboxed iframe sends", () => { + expect(corsHeaders("null").has("Access-Control-Allow-Origin")).toBe(false); + }); + + it("echoes an allow-listed origin and varies on it", () => { + const headers = corsHeaders("https://accshift.app"); + + expect(headers.get("Access-Control-Allow-Origin")).toBe("https://accshift.app"); + expect(headers.get("Vary")).toBe("Origin"); + }); + + it("says nothing about an origin that is not on the list", () => { + expect(corsHeaders("https://evil.invalid").has("Access-Control-Allow-Origin")).toBe(false); + }); +}); diff --git a/server/src/index.ts b/server/src/index.ts index 16e2903..cfafb4c 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -818,15 +818,17 @@ function allowedOrigins(env: Env): Set { ); } -function cors(res: Response, request: Request, env: Env): Response { +export function cors(res: Response, request: Request, env: Env): Response { const h = new Headers(res.headers); const origin = request.headers.get("Origin"); const allowed = allowedOrigins(env); + // A request with no Origin is a native client: the app and the CLI both send + // none, and neither one asks a browser for permission. It gets no header at + // all. Answering `null` would have granted exactly one origin, and `null` is + // the origin every sandboxed iframe and every data: document sends. if (origin && allowed.has(origin)) { h.set("Access-Control-Allow-Origin", origin); h.set("Vary", "Origin"); - } else if (!origin) { - h.set("Access-Control-Allow-Origin", "null"); } h.set("Access-Control-Allow-Methods", "GET, POST, OPTIONS"); h.set("Access-Control-Allow-Headers", "Content-Type, Authorization"); diff --git a/src-tauri/Cargo.toml b/src-tauri/Cargo.toml index 264c70c..c36fc56 100644 --- a/src-tauri/Cargo.toml +++ b/src-tauri/Cargo.toml @@ -37,12 +37,16 @@ image = { version = "0.25", default-features = false, features = ["jpeg"] } # does not expose (autofill popups on plain text inputs). webview2-com = "0.38" windows-core = "0.61" -# Same windows crate tauri pulls in; used for the WM_NCACTIVATE subclass that -# keeps DWM system backdrops alive while the window is unfocused. +# Same windows crate tauri pulls in. Two window subclasses use it: the +# WM_NCACTIVATE one that keeps DWM system backdrops alive while the window is +# unfocused, and the WM_NCHITTEST one that answers HTMAXBUTTON over our own +# maximize button so Windows 11 offers Snap Layouts on a frameless window. windows = { version = "0.62", features = [ "Win32_Foundation", "Win32_Graphics_Gdi", "Win32_Storage_Xps", + "Win32_UI_HiDpi", + "Win32_UI_Input_KeyboardAndMouse", "Win32_UI_Shell", "Win32_UI_WindowsAndMessaging", ] } diff --git a/src-tauri/src/boot.rs b/src-tauri/src/boot.rs index 14bc98e..3518a34 100644 --- a/src-tauri/src/boot.rs +++ b/src-tauri/src/boot.rs @@ -8,9 +8,11 @@ use crate::{app_runtime, config, ctx, logging, telemetry, telemetry_runtime}; use accshift_core::AppCtx; +use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::mpsc::Receiver; +use std::sync::Mutex; use tauri::webview::PageLoadEvent; -use tauri::{AppHandle, Manager, WebviewWindow}; +use tauri::{AppHandle, Manager, Monitor, WebviewWindow}; /// Shared HTTP client. A process that cannot build one cannot reach Steam, so /// there is nothing useful left to boot into. @@ -39,8 +41,10 @@ fn navigation_allowed(url: &tauri::Url) -> bool { || (cfg!(debug_assertions) && is_http && matches!(host, Some("localhost" | "127.0.0.1"))) } -/// Build the main window: last saved size, frameless and transparent, with the -/// navigation guard and the page-load log wired in. +/// Build the main window: last saved size and placement, frameless and +/// transparent, with the navigation guard and the page-load log wired in. +/// +/// Both saved values are logical pixels, which is the unit the builder takes. /// /// It is built hidden. Boot completion (or the failsafe below) shows it. pub(crate) fn build_main_window( @@ -49,6 +53,7 @@ pub(crate) fn build_main_window( ) -> Result> { let (start_width, start_height) = config::load_window_size(setup_ctx) .unwrap_or((config::DEFAULT_WINDOW_WIDTH, config::DEFAULT_WINDOW_HEIGHT)); + let saved_position = config::load_window_position(setup_ctx); let navigation_log_ctx = setup_ctx.clone(); let page_load_log_ctx = setup_ctx.clone(); @@ -61,7 +66,6 @@ pub(crate) fn build_main_window( .visible(false) .transparent(true) .background_color(tauri::webview::Color(0, 0, 0, 0)) - .center() .resizable(true) .on_navigation(move |url| { let allowed = navigation_allowed(url); @@ -93,6 +97,12 @@ pub(crate) fn build_main_window( ); }); + // First launch, or a config with no placement in it, still opens centered. + window_builder = match saved_position { + Some((x, y)) => window_builder.position(x, y), + None => window_builder.center(), + }; + #[cfg(target_os = "macos")] { // Native traffic lights float over our custom titlebar. WKWebView @@ -112,6 +122,21 @@ pub(crate) fn build_main_window( } let win = window_builder.build()?; + + // The monitor the window was saved on may be unplugged, or the desktop + // rearranged. The window is still hidden here, so recentering it costs no + // visible jump. + if saved_position.is_some() && !window_sits_on_a_monitor(&win) { + let _ = win.center(); + let _ = logging::append_app_log( + setup_ctx, + "info", + "backend.window", + "Saved window position is off every monitor; centered instead", + None, + ); + } + let _ = logging::append_app_log( setup_ctx, "info", @@ -122,6 +147,74 @@ pub(crate) fn build_main_window( Ok(win) } +/// True when the window overlaps the work area of at least one attached +/// monitor. Everything here is physical pixels, which is what both the window +/// and the monitor report, so no scale factor is involved. +/// +/// A window whose monitor list cannot be read is left where it is: with no +/// monitors to compare against there is no evidence it sits anywhere wrong. +fn window_sits_on_a_monitor(win: &WebviewWindow) -> bool { + let (Ok(position), Ok(size), Ok(monitors)) = ( + win.outer_position(), + win.outer_size(), + win.available_monitors(), + ) else { + return true; + }; + if monitors.is_empty() { + return true; + } + let window = Rect::at( + f64::from(position.x), + f64::from(position.y), + f64::from(size.width), + f64::from(size.height), + ); + monitors + .iter() + .any(|monitor| window.overlaps(&work_area_rect(monitor))) +} + +fn work_area_rect(monitor: &Monitor) -> Rect { + let area = monitor.work_area(); + Rect::at( + f64::from(area.position.x), + f64::from(area.position.y), + f64::from(area.size.width), + f64::from(area.size.height), + ) +} + +/// Screen rectangle in physical pixels, origin top left. +#[derive(Clone, Copy, Debug)] +struct Rect { + left: f64, + top: f64, + right: f64, + bottom: f64, +} + +impl Rect { + fn at(left: f64, top: f64, width: f64, height: f64) -> Self { + Self { + left, + top, + right: left + width, + bottom: top + height, + } + } + + /// True when the two rectangles share any area. Touching edges do not + /// count: a window whose right edge is exactly a monitor's left edge shows + /// nothing on it. + fn overlaps(&self, other: &Self) -> bool { + self.left < other.right + && self.right > other.left + && self.top < other.bottom + && self.bottom > other.top + } +} + /// Turn off Edge's form autofill. /// /// It pops "saved information" suggestions over plain text inputs (Steam launch @@ -147,56 +240,164 @@ pub(crate) fn disable_webview_autofill(win: &WebviewWindow) { #[cfg(not(windows))] pub(crate) fn disable_webview_autofill(_win: &WebviewWindow) {} -/// Persist the window size on its own thread, and hand back the channel that -/// says when the write landed. `None` means nothing was queued. +/// Size and placement of the main window, in the logical pixels the config +/// stores and the window builder consumes. +#[derive(Clone, Copy, Debug, PartialEq)] +struct WindowGeometry { + width: f64, + height: f64, + x: f64, + y: f64, +} + +/// Last geometry a move event reported, waiting to be written. +static PENDING_GEOMETRY: Mutex> = Mutex::new(None); +/// Whether a thread is already draining `PENDING_GEOMETRY`. +static GEOMETRY_SAVER_RUNNING: AtomicBool = AtomicBool::new(false); +/// Quiet time after the last move event before the write goes out. A window +/// drag emits dozens of events per second and each save takes the +/// cross-process config lock, so only the last one is worth writing. +const GEOMETRY_SAVE_DEBOUNCE: std::time::Duration = std::time::Duration::from_millis(700); + +/// Read the window's live geometry, converted to logical pixels once. +/// +/// This is the bug fix for the launch-over-launch growth: `inner_size` and +/// `outer_position` are physical, the builder is logical, so on a 125% display +/// storing the raw numbers multiplied the window by 1.25 every launch. +/// +/// `None` when nothing worth saving is on screen: a maximized window would +/// store the screen size as the restored size, a minimized one reports a +/// parking position far off every monitor (-32000 on Windows), and a window +/// whose size or position cannot be read has nothing to offer. +fn current_geometry(win: &WebviewWindow) -> Option { + if matches!(win.is_maximized(), Ok(true)) || matches!(win.is_minimized(), Ok(true)) { + return None; + } + let scale = win.scale_factor().ok()?; + let size = win.inner_size().ok()?; + let position = win.outer_position().ok()?; + let size = size.to_logical::(scale); + let position = position.to_logical::(scale); + Some(WindowGeometry { + width: size.width, + height: size.height, + x: position.x, + y: position.y, + }) +} + +/// Whether saving is allowed at all. A window closed or moved before boot +/// completed never got its saved geometry applied, so saving now would +/// overwrite the real one with the default. +fn geometry_save_allowed(app_handle: &AppHandle) -> bool { + app_handle.state::().is_completed() +} + +/// Persist the window geometry on its own thread, and hand back the channel +/// that says when the write landed. `None` means nothing was queued. /// /// The save is a read-modify-write that takes the cross-process config lock, /// which can wait up to 5s while the CLI holds it. Running it inline would /// freeze the UI thread for that whole stretch. -/// -/// Three reasons to skip it, and the first is not cosmetic: a window closed -/// before boot completed never got its saved size applied, so saving now would -/// overwrite the real one with the default. -fn spawn_window_size_save(app_handle: &AppHandle, win: &WebviewWindow) -> Option> { - if !app_handle.state::().is_completed() { +fn spawn_window_geometry_save(app_handle: &AppHandle, win: &WebviewWindow) -> Option> { + if !geometry_save_allowed(app_handle) { let _ = logging::append_app_log( &ctx(app_handle), "info", "backend.window", - "Skipped window size save because boot was not completed", + "Skipped window geometry save because boot was not completed", None, ); return None; } - if matches!(win.is_maximized(), Ok(true)) { - return None; - } - let size = win.inner_size().ok()?; + let geometry = current_geometry(win)?; + // This write is newer than anything the debounce still holds, and it must + // not be undone by a thread waking up after it. + take_pending_geometry(); let save_handle = app_handle.clone(); - let width = f64::from(size.width); - let height = f64::from(size.height); let (tx, rx) = std::sync::mpsc::channel(); std::thread::spawn(move || { - let _ = config::save_window_size(&ctx(&save_handle), width, height); + save_geometry(&save_handle, geometry); let _ = tx.send(()); }); Some(rx) } -/// What runs when the user closes the window: queue the size save, hide, end -/// the telemetry session, then wait out the size save. +fn save_geometry(app_handle: &AppHandle, geometry: WindowGeometry) { + let _ = config::save_window_geometry( + &ctx(app_handle), + geometry.width, + geometry.height, + Some((geometry.x, geometry.y)), + ); +} + +fn take_pending_geometry() -> Option { + PENDING_GEOMETRY + .lock() + .unwrap_or_else(|e| e.into_inner()) + .take() +} + +/// Queue a debounced geometry save. Called from the move handler, so it does +/// no IO of its own: it stamps the value and lets a background thread write the +/// last one once the drag stops. +fn queue_window_geometry_save(app_handle: &AppHandle, win: &WebviewWindow) { + if !geometry_save_allowed(app_handle) { + return; + } + let Some(geometry) = current_geometry(win) else { + return; + }; + *PENDING_GEOMETRY.lock().unwrap_or_else(|e| e.into_inner()) = Some(geometry); + if GEOMETRY_SAVER_RUNNING.swap(true, Ordering::SeqCst) { + // A thread is already waiting; it will pick this value up. + return; + } + + let save_handle = app_handle.clone(); + std::thread::spawn(move || { + loop { + std::thread::sleep(GEOMETRY_SAVE_DEBOUNCE); + match take_pending_geometry() { + Some(geometry) => save_geometry(&save_handle, geometry), + None => { + GEOMETRY_SAVER_RUNNING.store(false, Ordering::SeqCst); + // A move that landed between the take above and this + // release would otherwise sit unwritten until the next one. + let missed = PENDING_GEOMETRY + .lock() + .unwrap_or_else(|e| e.into_inner()) + .is_some(); + if missed && !GEOMETRY_SAVER_RUNNING.swap(true, Ordering::SeqCst) { + continue; + } + break; + } + } + } + }); +} + +/// Window events that outlive a single frame: the close sequence, and the +/// debounced geometry save behind every move. /// -/// This handler runs on the UI thread, so the hide comes before the telemetry -/// flush: anything slow ahead of it shows up as a frozen window rather than a -/// closed app. -pub(crate) fn install_close_handler(app_handle: AppHandle, win: &WebviewWindow) { +/// On close: queue the geometry save, hide, end the telemetry session, then +/// wait out the save. This handler runs on the UI thread, so the hide comes +/// before the telemetry flush: anything slow ahead of it shows up as a frozen +/// window rather than a closed app. +pub(crate) fn install_window_event_handlers(app_handle: AppHandle, win: &WebviewWindow) { let win_for_events = win.clone(); win.on_window_event(move |event| { + if matches!(event, tauri::WindowEvent::Moved(_)) { + queue_window_geometry_save(&app_handle, &win_for_events); + return; + } if !matches!(event, tauri::WindowEvent::CloseRequested { .. }) { return; } - let size_save_wait = spawn_window_size_save(&app_handle, &win_for_events); + let geometry_save_wait = spawn_window_geometry_save(&app_handle, &win_for_events); let _ = win_for_events.hide(); @@ -211,10 +412,10 @@ pub(crate) fn install_close_handler(app_handle: AppHandle, win: &WebviewWindow) .track(telemetry::Event::SessionEnded { duration_ms }); tstate.shutdown(); - // Give the size save the same bound save_config's own cross-process + // Give the geometry save the same bound save_config's own cross-process // lock uses, so it either lands before we exit or is abandoned // deliberately rather than silently. - if let Some(rx) = size_save_wait { + if let Some(rx) = geometry_save_wait { let _ = rx.recv_timeout(std::time::Duration::from_secs(5)); } }); @@ -273,38 +474,103 @@ pub(crate) fn wire_deep_links(app: &tauri::App, setup_ctx: &AppCtx) { }); } -/// Re-encrypt any snapshot still stored as plaintext, once per launch. +/// Re-encrypt any snapshot still stored as plaintext, sweep the Riot rollback +/// copies a crash left behind, then free the keyring entries no snapshot points +/// at any more. Once per launch. +/// +/// The rollback sweep runs before the collector so the entries it frees are +/// already gone from disk when the live set is read. /// /// Snapshots captured before encryption shipped only get encrypted when the /// account is captured again, and a dormant account never is. Off the boot path /// on purpose: on Linux and macOS every upgraded file costs a keyring round /// trip. +/// +/// The index the collector reads is armed here, synchronously, so it is in +/// place before anything in this process can capture a snapshot. The sweep +/// itself runs after the upgrade, which is what turns a rewritten legacy file's +/// old entry into an orphan. pub(crate) fn spawn_snapshot_upgrade(upgrade_ctx: AppCtx) { + accshift_core::secrets::init(&upgrade_ctx); std::thread::spawn(move || { let mut failures: Vec = Vec::new(); let stats = accshift_core::snapshot_crypto::upgrade_legacy_plaintext_snapshots( &upgrade_ctx, &mut |message, detail| failures.push(format!("{message} ({detail})")), ); - if !stats.touched_anything() { - return; + if stats.touched_anything() { + let level = if stats.failed > 0 { "warn" } else { "info" }; + let _ = logging::append_app_log( + &upgrade_ctx, + level, + "backend.snapshot-upgrade", + &format!( + "Re-encrypted {} legacy plaintext snapshot file(s), {} failed", + stats.upgraded, stats.failed + ), + (!failures.is_empty()) + .then(|| failures.join("; ")) + .as_deref(), + ); + } + + sweep_riot_rollback_copies(&upgrade_ctx); + + let mut sweep_failures: Vec = Vec::new(); + let swept = accshift_core::secrets::gc(&upgrade_ctx, &mut |message, detail| { + sweep_failures.push(format!("{message} ({detail})")) + }); + if swept.touched_anything() { + let level = if swept.failed > 0 { "warn" } else { "info" }; + let _ = logging::append_app_log( + &upgrade_ctx, + level, + "backend.secrets-gc", + &format!( + "Freed {} orphaned keyring entry(ies), {} failed", + swept.freed, swept.failed + ), + (!sweep_failures.is_empty()) + .then(|| sweep_failures.join("; ")) + .as_deref(), + ); } - let level = if stats.failed > 0 { "warn" } else { "info" }; - let _ = logging::append_app_log( - &upgrade_ctx, - level, - "backend.snapshot-upgrade", - &format!( - "Re-encrypted {} legacy plaintext snapshot file(s), {} failed", - stats.upgraded, stats.failed - ), - (!failures.is_empty()) - .then(|| failures.join("; ")) - .as_deref(), - ); }); } +/// Remove the Riot rollback copies a process that died mid-restore left +/// behind, encrypted ones under the app's state directory and the plaintext +/// ones older builds wrote into the system temp directory. Silent when there +/// is nothing to sweep, which is every launch after a clean run. +/// +/// Riot is a Windows-only platform, so there is nothing to sweep elsewhere. +#[cfg(windows)] +fn sweep_riot_rollback_copies(upgrade_ctx: &AppCtx) { + let mut failures: Vec = Vec::new(); + let swept = accshift_core::platforms::riot::sweep_rollback_dirs(upgrade_ctx, &mut |m, d| { + failures.push(format!("{m} ({d})")) + }); + if !swept.touched_anything() { + return; + } + let level = if swept.failed > 0 { "warn" } else { "info" }; + let _ = logging::append_app_log( + upgrade_ctx, + level, + "backend.riot-rollback-sweep", + &format!( + "Removed {} leftover Riot rollback copy(ies), {} failed", + swept.removed, swept.failed + ), + (!failures.is_empty()) + .then(|| failures.join("; ")) + .as_deref(), + ); +} + +#[cfg(not(windows))] +fn sweep_riot_rollback_copies(_upgrade_ctx: &AppCtx) {} + /// Show the window anyway if the frontend never reports boot done. pub(crate) fn spawn_boot_failsafe(fallback_handle: AppHandle) { std::thread::spawn(move || { @@ -327,3 +593,60 @@ pub(crate) fn spawn_boot_failsafe(fallback_handle: AppHandle) { let _ = app_runtime::show_main_window(&fallback_handle); }); } + +#[cfg(test)] +mod tests { + use super::*; + use tauri::{PhysicalPosition, PhysicalSize}; + + // The unit bug in one assertion: a 1000x520 logical window on a 125% + // display reports 1250x650 physical. Storing that raw is what made the + // window grow by 25% at every launch, because the builder reads the stored + // number as logical. + #[test] + fn a_physical_window_size_converts_back_to_the_logical_one() { + let scale = 1.25; + let physical = PhysicalSize::new(1250_u32, 650_u32); + let logical = physical.to_logical::(scale); + + assert_eq!((logical.width, logical.height), (1000.0, 520.0)); + assert_eq!( + ( + accshift_core::config::logical_from_physical(1250.0, scale), + accshift_core::config::logical_from_physical(650.0, scale), + ), + (logical.width, logical.height), + "the config helper and the tauri conversion must agree" + ); + } + + #[test] + fn a_physical_window_position_converts_back_to_the_logical_one() { + let physical = PhysicalPosition::new(-2400_i32, 150_i32); + let logical = physical.to_logical::(1.5); + assert_eq!((logical.x, logical.y), (-1600.0, 100.0)); + } + + #[test] + fn a_window_overlapping_a_monitor_is_kept() { + let monitor = Rect::at(0.0, 0.0, 1920.0, 1040.0); + // Fully inside. + assert!(Rect::at(100.0, 100.0, 1000.0, 520.0).overlaps(&monitor)); + // Half off the right edge, still reachable. + assert!(Rect::at(1900.0, 100.0, 1000.0, 520.0).overlaps(&monitor)); + // A second monitor to the left of the primary one. + assert!(Rect::at(-1800.0, 40.0, 1000.0, 520.0) + .overlaps(&Rect::at(-1920.0, 0.0, 1920.0, 1040.0))); + } + + #[test] + fn a_window_off_every_monitor_is_rejected() { + let monitor = Rect::at(0.0, 0.0, 1920.0, 1040.0); + // The unplugged second monitor case. + assert!(!Rect::at(-1800.0, 40.0, 1000.0, 520.0).overlaps(&monitor)); + // Below the taskbar, off the work area. + assert!(!Rect::at(100.0, 1040.0, 1000.0, 520.0).overlaps(&monitor)); + // Touching edges share no pixel. + assert!(!Rect::at(1920.0, 0.0, 1000.0, 520.0).overlaps(&monitor)); + } +} diff --git a/src-tauri/src/commands.rs b/src-tauri/src/commands.rs index 23006b7..74ee879 100644 --- a/src-tauri/src/commands.rs +++ b/src-tauri/src/commands.rs @@ -477,13 +477,18 @@ pub async fn platform_get_setup_status( // fail the poll. Report a non-terminal holding state instead. Every // platform's add-flow UI keeps its spinner on unknown/waiting states and // the next poll picks up the real status once the lock is free. + // + // That holding state is `busy`, not `waiting_for_login`: the wizard used to + // say "waiting for you to log in" for as long as a CLI switch or a slow + // Steam operation held the lock, which blames the user for someone else's + // work. `busy` says what is actually happening. run_blocking( "platform_get_setup_status", move || match accshift_core::lock::acquire_exclusive(&c, LOCK_TIMEOUT) { Ok(_lock) => service.get_setup_status(c, &setup_id), Err(accshift_core::lock::LockError::Contended) => Ok(SetupStatus { setup_id, - state: "waiting_for_login".to_string(), + state: "busy".to_string(), account_id: String::new(), account_display_name: String::new(), error_message: String::new(), @@ -545,7 +550,10 @@ pub async fn platform_detect_installed(app_handle: tauri::AppHandle) -> Vec Result { require_service(&platform_id)?.select_path() } @@ -606,7 +614,11 @@ pub async fn reload_user_platforms( /// Opens a file picker on a descriptor to add. Cancelling is an error, which /// the caller reads as "leave everything alone". -#[tauri::command] +/// +/// `command(async)` for the same reason as [`platform_select_path`]: the dialog +/// is a child process this call waits on, and waiting on the main thread makes +/// the window unresponsive while the picker is up. +#[tauri::command(async)] pub fn descriptor_select_file() -> Result { accshift_core::os::select_file( "Select a platform descriptor", @@ -696,6 +708,375 @@ pub fn close_window(window: tauri::Window) { let _ = window.close(); } +// --------------------------------------------------------------------------- +// Windows 11 Snap Layouts over the custom maximize button +// --------------------------------------------------------------------------- + +/// Where the titlebar's maximize button sits, in CSS pixels relative to the +/// top-left of the webview. +/// +/// The window is frameless, so Windows sees one big client area and never +/// offers the Snap Layouts flyout: that flyout only appears over a rectangle +/// the window itself reports as `HTMAXBUTTON`. The frontend owns the button's +/// geometry, so it is the frontend that measures it and hands it over. +#[derive(Clone, Copy, Debug, PartialEq, serde::Deserialize)] +pub struct MaximizeButtonRect { + pub x: f64, + pub y: f64, + pub width: f64, + pub height: f64, +} + +/// True when `point`, in physical client pixels, falls inside `rect`, which is +/// in logical pixels at `scale`. +/// +/// Pulled out of the window procedure on purpose: this is the whole decision +/// behind the Snap Layouts flyout, and it is the only part of the feature that +/// can be checked without a Windows 11 machine and a real mouse. +/// +/// Compiled on Windows (the window procedure calls it) and when tests run +/// (so linux/macOS CI still covers the geometry). Left out of a normal +/// unix `cargo clippy` of the binary, where it would be dead code. +#[cfg(any(windows, test))] +pub fn hit_test_maximize(point: (f64, f64), rect: MaximizeButtonRect, scale: f64) -> bool { + if !scale.is_finite() || scale <= 0.0 || rect.width <= 0.0 || rect.height <= 0.0 { + return false; + } + let left = rect.x * scale; + let top = rect.y * scale; + point.0 >= left + && point.0 < left + rect.width * scale + && point.1 >= top + && point.1 < top + rect.height * scale +} + +#[cfg(windows)] +mod snap_layouts { + use super::{hit_test_maximize, MaximizeButtonRect}; + use std::mem::size_of; + use std::sync::{Mutex, MutexGuard}; + use tauri::{AppHandle, Emitter}; + use windows::Win32::Foundation::{HWND, LPARAM, LRESULT, POINT, RECT, WPARAM}; + use windows::Win32::Graphics::Gdi::{ + GetMonitorInfoW, MonitorFromWindow, ScreenToClient, MONITORINFO, MONITOR_DEFAULTTONEAREST, + }; + use windows::Win32::UI::HiDpi::GetDpiForWindow; + use windows::Win32::UI::Input::KeyboardAndMouse::{ + TrackMouseEvent, TME_LEAVE, TME_NONCLIENT, TRACKMOUSEEVENT, + }; + use windows::Win32::UI::Shell::{DefSubclassProc, SetWindowSubclass}; + use windows::Win32::UI::WindowsAndMessaging::{ + GetWindowRect, IsZoomed, PostMessageW, HTCLIENT, HTMAXBUTTON, SC_MAXIMIZE, SC_RESTORE, + WM_NCHITTEST, WM_NCLBUTTONDBLCLK, WM_NCLBUTTONDOWN, WM_NCLBUTTONUP, WM_NCMOUSELEAVE, + WM_NCMOUSEMOVE, WM_NCRBUTTONDOWN, WM_NCRBUTTONUP, WM_SYSCOMMAND, + }; + + const SUBCLASS_ID: usize = 0x61636374; // "acct" + + /// Hit-test codes as `isize`, which is what a `WPARAM` and an `LRESULT` + /// carry. The constants themselves are `i32` and the casts would otherwise + /// be repeated at every comparison. + const HT_CLIENT: isize = HTCLIENT as isize; + const HT_MAX_BUTTON: isize = HTMAXBUTTON as isize; + + /// Emitted with `true` when the pointer enters the maximize button and + /// `false` when it leaves. Once Windows owns that rectangle as non-client, + /// the webview stops seeing pointer events over it, so CSS `:hover` never + /// fires and the button would look dead under the cursor. + const HOVER_EVENT: &str = "titlebar:maximize-hover"; + + struct State { + /// The button as last reported by the frontend. `None` means it is not + /// on screen (macOS layout, or the actions hidden), and every message + /// below then falls through untouched. + /// + /// One rect for the whole app: only the main window draws a titlebar. + rect: Option, + app: Option, + hovering: bool, + pressed: bool, + /// Windows already subclassed, so a second report does not stack a + /// second copy of the procedure on the same window. + installed: Vec, + } + + static STATE: Mutex = Mutex::new(State { + rect: None, + app: None, + hovering: false, + pressed: false, + installed: Vec::new(), + }); + + /// A poisoned lock here means a previous panic inside the window + /// procedure. The state is four plain values with no invariant between + /// them, so recovering it beats killing the titlebar for the session. + fn lock() -> MutexGuard<'static, State> { + STATE.lock().unwrap_or_else(|e| e.into_inner()) + } + + pub fn set_rect(app: &AppHandle, rect: Option) { + let mut state = lock(); + state.app = Some(app.clone()); + state.rect = rect; + if rect.is_none() { + state.hovering = false; + state.pressed = false; + } + } + + /// Installs the procedure. Must run on the thread that owns the window. + pub fn install(hwnd: isize) { + let mut state = lock(); + if state.installed.contains(&hwnd) { + return; + } + let handle = HWND(hwnd as *mut core::ffi::c_void); + if unsafe { SetWindowSubclass(handle, Some(window_proc), SUBCLASS_ID, 0) }.as_bool() { + state.installed.push(hwnd); + } + } + + unsafe extern "system" fn window_proc( + hwnd: HWND, + msg: u32, + wparam: WPARAM, + lparam: LPARAM, + _id: usize, + _data: usize, + ) -> LRESULT { + match msg { + WM_NCHITTEST => { + let below = unsafe { DefSubclassProc(hwnd, msg, wparam, lparam) }; + // Only ever upgrade the plain client area. A resize border, the + // caption, or anything tao already claims keeps its answer, so + // the top edge of the window stays draggable-to-resize. + if below.0 == HT_CLIENT && over_maximize_button(hwnd, lparam) { + return LRESULT(HT_MAX_BUTTON); + } + below + } + WM_NCMOUSEMOVE => { + let over = wparam.0 as isize == HT_MAX_BUTTON; + set_hover(hwnd, over); + if over { + return LRESULT(0); + } + unsafe { DefSubclassProc(hwnd, msg, wparam, lparam) } + } + WM_NCMOUSELEAVE => { + set_hover(hwnd, false); + unsafe { DefSubclassProc(hwnd, msg, wparam, lparam) } + } + // Swallowed: handing these to the default procedure makes Windows + // draw its own caption button over ours and start its own maximize + // on the way down. + WM_NCLBUTTONDOWN | WM_NCLBUTTONDBLCLK if wparam.0 as isize == HT_MAX_BUTTON => { + lock().pressed = true; + LRESULT(0) + } + WM_NCLBUTTONUP if wparam.0 as isize == HT_MAX_BUTTON => { + let pressed = std::mem::replace(&mut lock().pressed, false); + if pressed { + toggle_maximize(hwnd); + } + LRESULT(0) + } + // Right-clicking this area used to reach the webview, which shows + // nothing there. Swallowed so the answer stays "nothing" instead of + // the system menu the default procedure opens over a real caption + // button. Everywhere else on the titlebar is unaffected. + WM_NCRBUTTONDOWN | WM_NCRBUTTONUP if wparam.0 as isize == HT_MAX_BUTTON => LRESULT(0), + _ => unsafe { DefSubclassProc(hwnd, msg, wparam, lparam) }, + } + } + + /// The same toggle `toggle_maximize_window` performs, expressed as the + /// message a real caption button would send. Posted rather than called so + /// the resize happens after this message returns, and tao sees it the way + /// it sees a click on a decorated window. + fn toggle_maximize(hwnd: HWND) { + let command = if unsafe { IsZoomed(hwnd) }.as_bool() { + SC_RESTORE + } else { + SC_MAXIMIZE + }; + let _ = unsafe { + PostMessageW( + Some(hwnd), + WM_SYSCOMMAND, + WPARAM(command as usize), + LPARAM(0), + ) + }; + } + + fn over_maximize_button(hwnd: HWND, lparam: LPARAM) -> bool { + let Some(rect) = lock().rect else { + return false; + }; + // Fullscreen has no titlebar to snap from, and the frontend may not + // have reported the button gone yet. + if is_fullscreen(hwnd) { + return false; + } + // WM_NCHITTEST carries screen coordinates, the rect is client-relative. + let mut point = POINT { + x: signed_low(lparam), + y: signed_high(lparam), + }; + if !unsafe { ScreenToClient(hwnd, &mut point) }.as_bool() { + return false; + } + let dpi = unsafe { GetDpiForWindow(hwnd) }; + let scale = if dpi == 0 { 1.0 } else { f64::from(dpi) / 96.0 }; + hit_test_maximize((f64::from(point.x), f64::from(point.y)), rect, scale) + } + + fn is_fullscreen(hwnd: HWND) -> bool { + let mut window = RECT::default(); + if unsafe { GetWindowRect(hwnd, &mut window) }.is_err() { + return false; + } + let monitor = unsafe { MonitorFromWindow(hwnd, MONITOR_DEFAULTTONEAREST) }; + let mut info = MONITORINFO { + cbSize: size_of::() as u32, + ..Default::default() + }; + if !unsafe { GetMonitorInfoW(monitor, &mut info) }.as_bool() { + return false; + } + // Maximized stops at the work area; only fullscreen covers the whole + // monitor. + window == info.rcMonitor + } + + fn set_hover(hwnd: HWND, over: bool) { + let app = { + let mut state = lock(); + if state.hovering == over { + return; + } + state.hovering = over; + if !over { + state.pressed = false; + } + state.app.clone() + }; + + if over { + // Without this there is no WM_NCMOUSELEAVE, so the button would + // stay lit after the pointer walks off the window edge. + let mut track = TRACKMOUSEEVENT { + cbSize: size_of::() as u32, + dwFlags: TME_LEAVE | TME_NONCLIENT, + hwndTrack: hwnd, + dwHoverTime: 0, + }; + let _ = unsafe { TrackMouseEvent(&mut track) }; + } + + if let Some(app) = app { + // Never inline: the emit reaches the webview, and re-entering + // WebView2 from inside a message handler is the deadlock the repo + // rule about eval from a callback is there to prevent. + tauri::async_runtime::spawn(async move { + let _ = app.emit(HOVER_EVENT, over); + }); + } + } + + fn signed_low(lparam: LPARAM) -> i32 { + i32::from((lparam.0 & 0xFFFF) as u16 as i16) + } + + fn signed_high(lparam: LPARAM) -> i32 { + i32::from(((lparam.0 >> 16) & 0xFFFF) as u16 as i16) + } +} + +/// Reports where the titlebar's maximize button is, so Windows 11 can offer +/// Snap Layouts over it. `null` means the button is not on screen. +/// +/// Synchronous on purpose: the body writes a static and posts one message to +/// the window's own thread. No-op outside Windows. +#[tauri::command] +pub fn set_maximize_button_rect( + app_handle: tauri::AppHandle, + window: tauri::WebviewWindow, + rect: Option, +) { + #[cfg(windows)] + { + snap_layouts::set_rect(&app_handle, rect); + if let Ok(hwnd) = window.hwnd() { + let hwnd = hwnd.0 as isize; + // Subclassing must happen on the thread that owns the window. + let _ = window.run_on_main_thread(move || snap_layouts::install(hwnd)); + } + } + #[cfg(not(windows))] + let _ = (app_handle, window, rect); +} + +#[cfg(test)] +mod snap_layout_tests { + use super::{hit_test_maximize, MaximizeButtonRect}; + + /// A 46x36 caption button at the right edge of a 900px titlebar, the + /// geometry TitleBar.svelte draws today. + const BUTTON: MaximizeButtonRect = MaximizeButtonRect { + x: 808.0, + y: 0.0, + width: 46.0, + height: 36.0, + }; + + #[test] + fn the_middle_of_the_button_is_a_hit() { + assert!(hit_test_maximize((831.0, 18.0), BUTTON, 1.0)); + } + + #[test] + fn the_edges_belong_to_the_button_the_way_css_says() { + // Left and top inclusive, right and bottom exclusive, so the close + // button next door never loses its first column of pixels. + assert!(hit_test_maximize((808.0, 0.0), BUTTON, 1.0)); + assert!(!hit_test_maximize((854.0, 18.0), BUTTON, 1.0)); + assert!(!hit_test_maximize((831.0, 36.0), BUTTON, 1.0)); + assert!(!hit_test_maximize((807.9, 18.0), BUTTON, 1.0)); + } + + #[test] + fn a_point_over_the_minimize_or_close_button_is_a_miss() { + assert!(!hit_test_maximize((790.0, 18.0), BUTTON, 1.0)); + assert!(!hit_test_maximize((870.0, 18.0), BUTTON, 1.0)); + } + + #[test] + fn the_rect_scales_with_the_monitor_dpi() { + // 150% (144 dpi): the same button covers 1212..1281 physical pixels. + assert!(hit_test_maximize((1246.0, 27.0), BUTTON, 1.5)); + assert!(!hit_test_maximize((831.0, 18.0), BUTTON, 1.5)); + assert!(!hit_test_maximize((1281.0, 27.0), BUTTON, 1.5)); + assert!(hit_test_maximize((1280.9, 53.9), BUTTON, 1.5)); + } + + #[test] + fn a_degenerate_rect_or_scale_never_claims_a_point() { + let empty = MaximizeButtonRect { + x: 808.0, + y: 0.0, + width: 0.0, + height: 36.0, + }; + assert!(!hit_test_maximize((808.0, 18.0), empty, 1.0)); + assert!(!hit_test_maximize((831.0, 18.0), BUTTON, 0.0)); + assert!(!hit_test_maximize((831.0, 18.0), BUTTON, f64::NAN)); + assert!(!hit_test_maximize((831.0, 18.0), BUTTON, -1.0)); + } +} + // --------------------------------------------------------------------------- // Window backdrop (glass themes) // --------------------------------------------------------------------------- @@ -757,7 +1138,7 @@ pub fn set_keep_backdrop_active(window: tauri::WebviewWindow, enabled: bool) { /// Desktop wallpaper snapshot for the liquid glass fake backdrop: a JPEG data /// URL plus the physical virtual-screen rect it covers, so the frontend can /// align it under the window. -#[derive(serde::Serialize)] +#[derive(Clone, serde::Serialize)] #[serde(rename_all = "camelCase")] pub struct WallpaperSnapshot { pub data_url: String, @@ -771,6 +1152,9 @@ pub struct WallpaperSnapshot { mod wallpaper_capture { use super::WallpaperSnapshot; use base64::Engine; + use std::path::PathBuf; + use std::sync::Mutex; + use std::time::SystemTime; use windows::core::BOOL; use windows::Win32::Foundation::{HWND, LPARAM, RECT}; use windows::Win32::Graphics::Gdi::{ @@ -779,7 +1163,10 @@ mod wallpaper_capture { BITMAPINFOHEADER, BI_RGB, DIB_RGB_COLORS, HALFTONE, SRCCOPY, }; use windows::Win32::Storage::Xps::{PrintWindow, PRINT_WINDOW_FLAGS}; - use windows::Win32::UI::WindowsAndMessaging::{EnumWindows, GetClassNameW, GetWindowRect}; + use windows::Win32::UI::WindowsAndMessaging::{ + EnumWindows, GetClassNameW, GetSystemMetrics, GetWindowRect, SM_CXVIRTUALSCREEN, + SM_CYVIRTUALSCREEN, SM_XVIRTUALSCREEN, SM_YVIRTUALSCREEN, + }; /// Semi-documented since Win 8.1; makes PrintWindow capture DWM-composed /// content, which is where the wallpaper actually lives on Windows 11. @@ -981,22 +1368,97 @@ mod wallpaper_capture { height: capture.height, }) } + + /// What the last answer was computed from. + /// + /// The shell rewrites `TranscodedWallpaper` whenever the desktop picture + /// changes, Spotlight rotations and slideshow ticks included, and it also + /// rewrites it when the fit mode changes. Its size and mtime therefore + /// identify the picture as rendered, which `SPI_GETDESKWALLPAPER` cannot do + /// (it answers an empty path under Spotlight). The virtual-screen rect + /// closes the other half: same picture, new monitor layout, different + /// capture. + #[derive(PartialEq, Eq)] + pub(super) struct WallpaperKey { + modified: Option, + len: u64, + virtual_rect: (i32, i32, i32, i32), + } + + static CACHE: Mutex> = Mutex::new(None); + + fn transcoded_wallpaper_path() -> Option { + let appdata = std::env::var_os("APPDATA")?; + let mut path = PathBuf::from(appdata); + path.push("Microsoft"); + path.push("Windows"); + path.push("Themes"); + path.push("TranscodedWallpaper"); + Some(path) + } + + /// `None` when the identity cannot be read, which disables caching rather + /// than risking a stale backdrop. + pub(super) fn current_key() -> Option { + let meta = std::fs::metadata(transcoded_wallpaper_path()?).ok()?; + let virtual_rect = unsafe { + ( + GetSystemMetrics(SM_XVIRTUALSCREEN), + GetSystemMetrics(SM_YVIRTUALSCREEN), + GetSystemMetrics(SM_CXVIRTUALSCREEN), + GetSystemMetrics(SM_CYVIRTUALSCREEN), + ) + }; + Some(WallpaperKey { + modified: meta.modified().ok(), + len: meta.len(), + virtual_rect, + }) + } + + pub(super) fn cached(key: &WallpaperKey) -> Option { + let guard = CACHE.lock().unwrap_or_else(|error| error.into_inner()); + let (stored, snapshot) = guard.as_ref()?; + (stored == key).then(|| snapshot.clone()) + } + + pub(super) fn store(key: WallpaperKey, snapshot: &WallpaperSnapshot) { + let mut guard = CACHE.lock().unwrap_or_else(|error| error.into_inner()); + *guard = Some((key, snapshot.clone())); + } } /// Feeds the liquid glass fake backdrop: no DWM material can blur/distort /// what sits behind a transparent window without the acrylic gray smoke, so /// the frontend replicates the wallpaper inside the window and filters it. -#[tauri::command] +/// `command(async)` is load bearing twice over. A plain `#[tauri::command]` +/// runs on the main thread, so the full-resolution capture, the JPEG encode and +/// the base64 of several megabytes would all hitch the UI, and +/// `run_on_main_thread` below would execute inline instead of posting to the +/// event loop. +#[tauri::command(async)] pub fn get_desktop_wallpaper(window: tauri::WebviewWindow) -> Option { #[cfg(windows)] { + // The frontend asks at theme activation, 300 ms after every resize, on + // every scale change and every five minutes. The picture behind the + // window is the same one nearly every time, so answer from the cache + // rather than recapture and re-encode it. + let key = wallpaper_capture::current_key(); + if let Some(key) = key.as_ref() { + if let Some(hit) = wallpaper_capture::cached(key) { + return Some(hit); + } + } + // PrintWindow(PW_RENDERFULLCONTENT) on Progman drives DWM/WinRT - // composition. Tauri runs commands on a worker thread; doing this GDI + - // WinRT work off the UI thread (which owns the process's COM apartment) - // races the shell's own recomposition (Spotlight rotating the wallpaper) - // and corrupted a WinRT object refcount, crashing later in an unrelated - // worker. Marshal only the DWM/GDI capture onto the main thread; - // JPEG/base64 work resumes on this command worker. + // composition. Doing that GDI + WinRT work off the UI thread (which owns + // the process's COM apartment) races the shell's own recomposition + // (Spotlight rotating the wallpaper) and corrupted a WinRT object + // refcount, crashing later in an unrelated worker. So the capture, and + // only the capture, is posted to the main thread; its raw pixels come + // back through this channel and the JPEG plus base64 work runs here, on + // the command's own thread. let (tx, rx) = std::sync::mpsc::channel(); if window .run_on_main_thread(move || { @@ -1006,8 +1468,14 @@ pub fn get_desktop_wallpaper(window: tauri::WebviewWindow) -> Option bool { crate::platforms::steam::has_api_key(ctx(&app_handle)) } -#[tauri::command] +/// `command(async)`: handing a URL to the shell spawns the default browser, and +/// a cold browser start takes long enough to be felt on the main thread. +#[tauri::command(async)] pub fn steam_open_api_key_page() -> Result<(), PlatformError> { crate::platforms::steam::open_steam_api_key_page() } diff --git a/src-tauri/src/commands_telemetry.rs b/src-tauri/src/commands_telemetry.rs index f5c0f39..33a3c0d 100644 --- a/src-tauri/src/commands_telemetry.rs +++ b/src-tauri/src/commands_telemetry.rs @@ -314,11 +314,13 @@ pub fn telemetry_track_streamer_mode(app_handle: tauri::AppHandle) { } /// Marks the onboarding as completed and applies the user's choice from the -/// two-button consent screen. Nothing is emitted before this choice. +/// three-button consent screen. Nothing is emitted before this choice. /// Enabling Mode B also generates an install_id when missing. /// -/// `(false, false)` stays valid on the command even though the onboarding no -/// longer produces it: the refusal path now lives in Settings, Privacy. +/// The consent screen sends all three accepted pairs: `(true, true)` for the +/// full deal, `(true, false)` for the skip (and for the reject countdown), and +/// `(false, false)` for the refuse button. Settings, Privacy reaches the same +/// refusal later. #[tauri::command] pub async fn telemetry_complete_onboarding( app_handle: tauri::AppHandle, diff --git a/src-tauri/src/main.rs b/src-tauri/src/main.rs index cbee5d5..2725fcd 100644 --- a/src-tauri/src/main.rs +++ b/src-tauri/src/main.rs @@ -104,7 +104,7 @@ fn main() { &setup_ctx, app_start, )); - boot::install_close_handler(app.handle().clone(), &win); + boot::install_window_event_handlers(app.handle().clone(), &win); boot::wire_deep_links(app, &setup_ctx); boot::spawn_snapshot_upgrade(setup_ctx.clone()); boot::spawn_boot_failsafe(app.handle().clone()); @@ -152,6 +152,7 @@ fn main() { commands::minimize_window, commands::toggle_maximize_window, commands::close_window, + commands::set_maximize_button_rect, commands::set_keep_backdrop_active, commands::get_desktop_wallpaper, // Steam-specific diff --git a/src-tauri/tests/command_threading.rs b/src-tauri/tests/command_threading.rs new file mode 100644 index 0000000..07adb9c --- /dev/null +++ b/src-tauri/tests/command_threading.rs @@ -0,0 +1,285 @@ +//! A Tauri command that blocks must be `#[tauri::command(async)]`. +//! +//! Tauri 2 runs a command declared without `async` on the main thread, which is +//! the thread that pumps the window's event loop. Anything slow in such a body +//! (a filesystem walk, a child process, the cross-process lock) freezes the +//! window: it stops repainting, cannot be moved, and its close button does +//! nothing. `#[tauri::command(async)]` moves the body off that thread, and +//! `invoke` is already a promise on the JS side, so nothing in the frontend +//! changes. +//! +//! This reads the sources rather than the running app: it is a lint, and it is +//! here so the next command that shells out is caught at `cargo test` instead +//! of on a user's machine. + +use std::fs; +use std::path::{Path, PathBuf}; + +/// Substrings that mark a body as blocking. Crude on purpose: a false positive +/// is answered by adding `(async)`, which costs nothing. +const BLOCKING_MARKERS: &[&str] = &[ + "std::fs::", + "fs::", + "Command::new", + ".output()", + ".status()", + "run_locked_blocking", + "run_blocking", + "thread::sleep", + "acquire_exclusive", +]; + +/// Commands allowed to stay synchronous despite matching a marker above. One +/// entry per line, each with the reason it cannot move off the main thread. +const ALLOWED_SYNC: &[&str] = &[ + // (empty: every blocking command is currently `command(async)`) +]; + +/// Commands the scanner is expected to see at all, so a parser that quietly +/// stops matching anything fails instead of passing on an empty set. +const KNOWN_SYNC_COMMANDS: &[&str] = &["get_runtime_os", "minimize_window", "close_window"]; + +#[test] +fn every_blocking_command_is_async() { + let src = Path::new(env!("CARGO_MANIFEST_DIR")).join("src"); + let mut files = Vec::new(); + collect_rust_files(&src, &mut files); + files.sort(); + assert!( + !files.is_empty(), + "no Rust source found under {}", + src.display() + ); + + let mut offenders = Vec::new(); + let mut seen = Vec::new(); + for file in &files { + let text = fs::read_to_string(file).expect("source file is readable"); + let label = file.display().to_string(); + offenders.extend(blocking_sync_commands(&label, &text)); + seen.extend(sync_commands(&text)); + } + + for known in KNOWN_SYNC_COMMANDS { + assert!( + seen.iter().any(|name| name == known), + "the scanner no longer sees {known}, so it is not reading commands any more" + ); + } + + assert!( + offenders.is_empty(), + "these synchronous commands block the main thread. Mark them \ + #[tauri::command(async)], or add them to ALLOWED_SYNC with a reason:\n {}", + offenders.join("\n ") + ); +} + +// The lint above only proves something if it can fail. This feeds it the three +// shapes that matter, so a parser change that stops flagging is caught here. +#[test] +fn the_scanner_flags_a_blocking_sync_command_and_nothing_else() { + let source = r#" +#[tauri::command] +pub fn reads_a_folder(path: String) -> bool { + // A brace in a comment: { + std::fs::metadata(&path).is_ok() +} + +#[tauri::command(async)] +pub fn reads_a_folder_off_thread(path: String) -> bool { + std::fs::metadata(&path).is_ok() +} + +#[tauri::command] +pub async fn reads_a_folder_asynchronously(path: String) -> bool { + run_blocking("x", move || Ok(path.len())).await.is_ok() +} + +#[tauri::command] +pub fn touches_nothing() -> String { + format!("{}", '{') +} +"#; + + let flagged = blocking_sync_commands("fixture.rs", source); + assert_eq!(flagged.len(), 1, "flagged: {flagged:?}"); + assert!( + flagged[0].contains("reads_a_folder blocks on"), + "flagged: {flagged:?}" + ); + assert_eq!( + sync_commands(source), + vec!["reads_a_folder".to_string(), "touches_nothing".to_string()] + ); +} + +/// Names of every command in `text` that runs on the main thread. +fn sync_commands(text: &str) -> Vec { + let lines: Vec<&str> = text.lines().collect(); + let mut names = Vec::new(); + for signature in sync_command_signatures(&lines) { + names.push(function_name(lines[signature]).expect("a command has a name")); + } + names +} + +/// The offending lines of `text`, formatted for the failure message. +fn blocking_sync_commands(label: &str, text: &str) -> Vec { + let lines: Vec<&str> = text.lines().collect(); + let mut offenders = Vec::new(); + for signature in sync_command_signatures(&lines) { + let name = function_name(lines[signature]).expect("a command has a name"); + if ALLOWED_SYNC.contains(&name.as_str()) { + continue; + } + let body = body_of(&lines, signature); + let hits: Vec<&str> = BLOCKING_MARKERS + .iter() + .copied() + .filter(|marker| body.contains(marker)) + .collect(); + if !hits.is_empty() { + offenders.push(format!( + "{label}:{} {name} blocks on [{}]", + signature + 1, + hits.join(", ") + )); + } + } + offenders +} + +/// Line index of the signature of every `#[tauri::command]` that is neither +/// `command(async...)` nor an `async fn`. +fn sync_command_signatures(lines: &[&str]) -> Vec { + let mut found = Vec::new(); + for (index, line) in lines.iter().enumerate() { + let trimmed = line.trim_start(); + if !trimmed.starts_with("#[tauri::command") { + continue; + } + // `#[tauri::command(async)]` and `#[tauri::command(async, ...)]` are + // already off the main thread. + if trimmed.contains("(async") { + continue; + } + let Some(signature) = (index + 1..lines.len()).find(|i| is_signature(lines[*i])) else { + continue; + }; + if lines[signature].contains("async fn ") { + continue; + } + found.push(signature); + } + found +} + +fn collect_rust_files(dir: &Path, out: &mut Vec) { + let Ok(entries) = fs::read_dir(dir) else { + return; + }; + for entry in entries.flatten() { + let path = entry.path(); + if path.is_dir() { + collect_rust_files(&path, out); + } else if path.extension().is_some_and(|ext| ext == "rs") { + out.push(path); + } + } +} + +/// True for the `fn` line of a declaration, whatever its visibility. Keeps a +/// doc comment or an attribute mentioning "fn" from being mistaken for one. +fn is_signature(line: &str) -> bool { + let mut rest = line.trim_start(); + if let Some(after) = rest.strip_prefix("pub") { + rest = after.trim_start(); + if rest.starts_with('(') { + rest = match rest.find(')') { + Some(end) => rest[end + 1..].trim_start(), + None => return false, + }; + } + } + rest = rest.strip_prefix("const ").unwrap_or(rest).trim_start(); + rest = rest.strip_prefix("async ").unwrap_or(rest).trim_start(); + rest.starts_with("fn ") +} + +fn function_name(line: &str) -> Option { + let after = line.split("fn ").nth(1)?; + let name: String = after + .chars() + .take_while(|c| c.is_alphanumeric() || *c == '_') + .collect(); + (!name.is_empty()).then_some(name) +} + +/// The declaration from its signature to the matching closing brace. +/// +/// Braces inside string literals, char literals and line comments are ignored, +/// so a `format!("{x}")` or a `// {` in the body cannot end it early. +fn body_of(lines: &[&str], signature: usize) -> String { + let mut body = String::new(); + let mut depth = 0usize; + let mut opened = false; + for line in &lines[signature..] { + for c in strip_literals(line).chars() { + match c { + '{' => { + depth += 1; + opened = true; + } + '}' => depth = depth.saturating_sub(1), + _ => {} + } + } + body.push_str(line); + body.push('\n'); + if opened && depth == 0 { + break; + } + } + body +} + +/// Drops string literals, char literals and the tail of a line comment. Raw +/// strings are not handled; there are none in the scanned crate, and one +/// appearing only ever makes this noisier, never quieter. +fn strip_literals(line: &str) -> String { + let mut out = String::with_capacity(line.len()); + let mut chars = line.chars().peekable(); + while let Some(c) = chars.next() { + match c { + '/' if chars.peek() == Some(&'/') => break, + '"' => { + while let Some(inner) = chars.next() { + if inner == '\\' { + chars.next(); + } else if inner == '"' { + break; + } + } + } + '\'' => { + // A char literal is one escaped or plain character then a + // closing quote. Anything else is a lifetime, which carries no + // braces and can be left alone. + let mut lookahead = chars.clone(); + if lookahead.next() == Some('\\') { + for inner in chars.by_ref() { + if inner == '\'' { + break; + } + } + } else if lookahead.next() == Some('\'') { + chars.next(); + chars.next(); + } + } + _ => out.push(c), + } + } + out +} diff --git a/src/App.svelte b/src/App.svelte index bdf670d..7d569d6 100644 --- a/src/App.svelte +++ b/src/App.svelte @@ -263,6 +263,15 @@ getIsAccountSelectionView: () => isAccountSelectionView, getAppVersion: () => appVersion, onCloseContextMenu: dialogs.closeContextMenu, + // Re-read from the store rather than writing `shell.settings` back: the + // unlock can happen while the settings panel holds its own draft, and only + // the hash may travel. + persistPinHash: (hash) => { + const latest = getSettings(); + latest.pinHash = hash; + saveSettings(latest); + shell.refreshSettings(); + }, t, }); const streamerMode = createStreamerModeController({ @@ -1533,16 +1542,16 @@ /* Fake see-through material for Liquid Glass on Windows: the desktop wallpaper, screen-aligned via background-size/position (inline style), lightly blurred and refracted. Bleeds past the window so the displacement - and blur never sample outside the image. z-index -1 keeps it under all - content (the shell's will-change creates the stacking context). */ + and blur never sample outside the image. A negative z keeps it under + all content (the shell's will-change creates the stacking context). */ .liquid-backdrop { position: absolute; inset: -40px; - /* Below the rim lens (::before, z-index -1) so the rim's backdrop-filter - refracts the wallpaper only. Both sit in negative-z, so the app content - (titlebar buttons, cards) always paints on top and stays crisp: the - rim never blurs the UI, only the desktop. */ - z-index: -2; + /* Below the rim lens (::before, --z-rim-lens) so the rim's + backdrop-filter refracts the wallpaper only. Both sit in negative-z, + so the app content (titlebar buttons, cards) always paints on top and + stays crisp: the rim never blurs the UI, only the desktop. */ + z-index: var(--z-wallpaper); pointer-events: none; background-repeat: no-repeat; filter: url(#lg-backdrop-distortion) saturate(1.25); @@ -1568,7 +1577,7 @@ inset: 0; opacity: 0; pointer-events: none; - z-index: 40; + z-index: var(--z-frost); background: linear-gradient( to bottom, @@ -1620,7 +1629,7 @@ width: 20px; height: 20px; border: 2px solid var(--border); - border-top-color: #3b82f6; + border-top-color: var(--accent); border-radius: 50%; animation: spin 0.7s linear infinite; } diff --git a/src/app.css b/src/app.css index aa31546..2c5fa4d 100644 --- a/src/app.css +++ b/src/app.css @@ -96,6 +96,12 @@ docs/theming.md. */ --accent: #2563eb; --accent-fg: #ffffff; + /* The accent used as text or as a hairline icon rather than as a fill. + `--accent` is chosen to sit under `--accent-fg`, so on a card it lands + around 3:1 and fails small copy. Pulled toward the foreground it keeps + its hue and clears the contrast on a dark and on a light theme alike, + which a fixed light blue never did. */ + --accent-text: color-mix(in srgb, var(--accent) 55%, var(--fg)); --success: #22c55e; --warning: #eab308; --radius-sm: 4px; @@ -119,6 +125,59 @@ --label-transform: none; --motion-scale: 1; + /* Stacking order, declared once. Every z-index in the app names a level + here, so what covers what is readable in one place instead of being + spread over a dozen components as bare numbers nobody can compare. The + values leave gaps: a new layer slots between two of them without + renumbering anything. Not a theme token, a theme has no say in this. + + The two negative levels sit inside the shell's stacking context, under + every piece of app content: the Liquid Glass wallpaper and the rim lens + that refracts it. */ + --z-wallpaper: -2; + --z-rim-lens: -1; + + /* The five card levels are local. An account card shell isolates its own + stacking context, so they only ever compete with each other, except + --z-card-extension which lifts the hovered shell over its neighbours. */ + --z-card-base: 1; + --z-card-face: 2; + --z-card-dragging: 8; + --z-card-active: 18; + --z-card-extension: 24; + + /* Window chrome, in the window's own stacking context. */ + --z-sheen: 30; + --z-frost: 40; + --z-sticky: 100; + --z-sticky-popover: 101; + --z-screen: 300; + --z-screen-top: 320; + --z-streamer: 400; + --z-lock: 500; + /* Above the lock and the away screen on purpose: an error raised while the + screen is locked has to stay readable, and a toast carries no account + data of its own. */ + --z-toast: 600; + --z-dialog: 1100; + --z-menu: 1200; + + /* The onboarding tour points at the interface, so it sits over all of it. + Its five layers keep their own order: click shield, dimmed backdrop, + spotlight cutout, docked card, then the cleared backdrop that stops the + card from being darkened. */ + --z-tour-shield: 8990; + --z-tour-backdrop: 9000; + --z-tour-spotlight: 9001; + --z-tour-card: 9002; + --z-tour-clear: 9003; + + /* The drag ghost is appended to and follows the pointer, so it has + to clear everything it could be dragged over. Above the tour as well: + the tour's click shield makes that combination unreachable, and the + ghost has no business being clipped by a layer it can float across. */ + --z-drag-ghost: 9999; + /* Card grid metrics, in terms of the shape tokens: density and the medium radius are what a theme turns to reshape the account grid. */ --grid-card-width: calc(100px * var(--density-scale)); @@ -336,10 +395,10 @@ html[data-theme="liquid-glass"] .app-shell::before { position: absolute; inset: 0; pointer-events: none; - /* Negative z (above the wallpaper at -2, below all app content): the rim's + /* Negative z (above the wallpaper, below all app content): the rim's backdrop-filter then refracts only the wallpaper, never the UI painted on top. Keeps buttons and cards crisp while the glassy lens rides the edge. */ - z-index: -1; + z-index: var(--z-rim-lens); -webkit-mask: linear-gradient( to right, @@ -398,7 +457,7 @@ html[data-theme="liquid-glass"] .app-shell::after { position: absolute; inset: 0; pointer-events: none; - z-index: 30; + z-index: var(--z-sheen); background: radial-gradient( 140% 80% at 50% -35%, diff --git a/src/lib/app/AppDialogs.svelte b/src/lib/app/AppDialogs.svelte index f54d302..ab31f9d 100644 --- a/src/lib/app/AppDialogs.svelte +++ b/src/lib/app/AppDialogs.svelte @@ -154,6 +154,6 @@ display: flex; flex-direction: column; align-items: flex-end; - z-index: 200; + z-index: var(--z-toast); } diff --git a/src/lib/app/AppScreenOverlays.svelte b/src/lib/app/AppScreenOverlays.svelte index a5814cc..9db7069 100644 --- a/src/lib/app/AppScreenOverlays.svelte +++ b/src/lib/app/AppScreenOverlays.svelte @@ -155,7 +155,7 @@ pointer-events: none; opacity: 0; transition: opacity 900ms ease-in-out; - z-index: 300; + z-index: var(--z-screen); } .inactive-overlay.visible { @@ -194,7 +194,7 @@ user-select: none; -webkit-user-select: none; opacity: 0; - z-index: 320; + z-index: var(--z-screen-top); } /* The strip is centered with a translateX, so a permanent transform @@ -223,7 +223,7 @@ .pin-lock-overlay { position: absolute; inset: 0; - z-index: 500; + z-index: var(--z-lock); display: flex; align-items: center; justify-content: center; diff --git a/src/lib/app/AppWorkspace.svelte b/src/lib/app/AppWorkspace.svelte index 910ccc9..3251f30 100644 --- a/src/lib/app/AppWorkspace.svelte +++ b/src/lib/app/AppWorkspace.svelte @@ -213,7 +213,7 @@ function getEffectiveAccountColor(accountId: string): string { if (!bulkEditMode) return getAccountCardColor(accountId); - return bulkEditSelectedIds.has(accountId) ? "#2563eb" : ""; + return bulkEditSelectedIds.has(accountId) ? "var(--accent)" : ""; } function getEffectiveAccountNote(accountId: string): string { @@ -254,7 +254,7 @@ showNoteInline={bulkEditMode ? false : showCardNotesInline} showUsername={isPendingSetupAccount(account.id) ? false : showUsernames} showLastLogin={isPendingSetupAccount(account.id) ? false : showLastLogin} - lastLoginAt={account.lastLoginAt} + lastLoginAtSec={account.lastLoginAtSec} {lastLoginUnknownKey} {locale} isActive={!bulkEditMode && account.id === currentAccountId} @@ -646,7 +646,7 @@ width: 20px; height: 20px; border: 2px solid var(--border); - border-top-color: #3b82f6; + border-top-color: var(--accent); border-radius: 50%; animation: spin 0.7s linear infinite; } diff --git a/src/lib/app/StreamerModeOverlay.svelte b/src/lib/app/StreamerModeOverlay.svelte index ec44bf9..ab71b7d 100644 --- a/src/lib/app/StreamerModeOverlay.svelte +++ b/src/lib/app/StreamerModeOverlay.svelte @@ -81,7 +81,7 @@ opacity: 0; pointer-events: none; transition: opacity 320ms ease-in-out; - z-index: 400; + z-index: var(--z-streamer); } .streamer-overlay.visible { diff --git a/src/lib/app/platformAddFlow.svelte.ts b/src/lib/app/platformAddFlow.svelte.ts index d26143a..daf596b 100644 --- a/src/lib/app/platformAddFlow.svelte.ts +++ b/src/lib/app/platformAddFlow.svelte.ts @@ -95,13 +95,17 @@ export function createPlatformAddFlowController({ return { id: setupId, displayName: detectedName || t("platform.newAccountPending"), - username: detectedName - ? t(getSetupKey(flow.platformId, "connected")) - : t(getSetupKey(flow.platformId, "waitingForLogin")), - lastLoginAt: null, + username: detectedName ? t(getSetupKey(flow.platformId, "connected")) : pendingUsername(flow), + lastLoginAtSec: null, } satisfies PlatformAccount; }); + /** The one line the card shows while nothing has been detected yet. */ + function pendingUsername(entry: PlatformAddFlowEntry): string { + if (entry.status.state === "busy") return t("platform.setupBusy"); + return t(getSetupKey(entry.platformId, "waitingForLogin")); + } + function clearTimer() { if (!timer) return; clearTimeout(timer); @@ -256,6 +260,22 @@ export function createPlatformAddFlowController({ }, }; + // Handled before the per-platform branches: the cross-process lock is not a + // platform concern, and every add flow reports it the same way. Non-terminal + // and platform-agnostic, so the spinner stays on and the next poll replaces + // this with the real status. + if (flow.status.state === "busy") { + return { + sections: [ + { + text: t("platform.setupBusy"), + loading: true, + }, + ...(detectedSection ? [detectedSection] : []), + ], + }; + } + if (flow.platformId === "riot") { switch (flow.status.state) { case "waiting_for_client": diff --git a/src/lib/app/platformAddFlow.test.ts b/src/lib/app/platformAddFlow.test.ts new file mode 100644 index 0000000..804626c --- /dev/null +++ b/src/lib/app/platformAddFlow.test.ts @@ -0,0 +1,82 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import type { PlatformAdapter, PlatformAddFlowStatus } from "$lib/shared/platform"; + +const mocks = vi.hoisted(() => ({ + adapter: undefined as PlatformAdapter | undefined, + trackOperationFailed: vi.fn(), +})); + +vi.mock("$lib/app/telemetryClient", () => ({ + trackAccountAdded: vi.fn(), + trackAccountAddCancelled: vi.fn(), + trackAccountAddStarted: vi.fn(), + trackOperationFailed: (...args: unknown[]) => mocks.trackOperationFailed(...args), +})); + +vi.mock("$lib/shared/platform", () => ({ + getPlatform: () => mocks.adapter, +})); + +vi.mock("$lib/platforms/registry", () => ({ + getPlatformDefinition: () => undefined, +})); + +import { createPlatformAddFlowController } from "./platformAddFlow.svelte"; + +function controller() { + return createPlatformAddFlowController({ + getActiveTab: () => "steam", + getCurrentFolderId: () => null, + getIsSearching: () => false, + // The key is the assertion target: no dictionary, no interpolation. + t: (key) => key, + showToast: vi.fn(), + copyToClipboard: vi.fn(), + loadAccounts: vi.fn(), + }); +} + +const BUSY: PlatformAddFlowStatus = { setupId: "setup-1", state: "busy" }; + +afterEach(() => { + mocks.adapter = undefined; + mocks.trackOperationFailed.mockReset(); +}); + +describe("a contended lock during setup", () => { + it("says another operation is running instead of waiting for a login", () => { + const flow = controller(); + flow.start("steam", BUSY); + + const content = flow.getSetupExtensionContent("setup-1"); + flow.stop(); + + expect(content?.sections[0]?.text).toBe("platform.setupBusy"); + expect(content?.sections[0]?.loading).toBe(true); + }); + + it("labels the pending card with it too", () => { + const flow = controller(); + flow.start("steam", BUSY); + + const pending = flow.pendingSetupAccount; + flow.stop(); + + expect(pending?.username).toBe("platform.setupBusy"); + }); + + it("is non-terminal: the poll keeps the flow alive", async () => { + mocks.adapter = { + pollAddFlow: vi.fn().mockResolvedValue(BUSY), + } as unknown as PlatformAdapter; + const flow = controller(); + flow.start("steam", { setupId: "setup-1", state: "waiting_for_login" }); + + await flow.poll(); + const state = flow.flow?.status.state; + flow.stop(); + + expect(state).toBe("busy"); + expect(mocks.trackOperationFailed).not.toHaveBeenCalled(); + }); +}); diff --git a/src/lib/app/platformShell.svelte.ts b/src/lib/app/platformShell.svelte.ts index 7c8f7cc..cc42685 100644 --- a/src/lib/app/platformShell.svelte.ts +++ b/src/lib/app/platformShell.svelte.ts @@ -60,7 +60,7 @@ export function createPlatformShellState() { } return ids; }); - let accentColor = $derived(getPlatformDefinition(activeTab)?.accent || "#3b82f6"); + let accentColor = $derived(getPlatformDefinition(activeTab)?.accent || "var(--accent)"); let activeTheme = $derived(getThemeDefinition(settings.themeId)); let uiZoomFactor = $derived(Math.min(1.5, Math.max(0.75, settings.uiScalePercent / 100))); let appStageStyle = $derived.by(() => { diff --git a/src/lib/app/useAppDialogs.svelte.ts b/src/lib/app/useAppDialogs.svelte.ts index 30018ab..9995750 100644 --- a/src/lib/app/useAppDialogs.svelte.ts +++ b/src/lib/app/useAppDialogs.svelte.ts @@ -15,7 +15,15 @@ import type { import { getPlatform } from "$lib/shared/platform"; import type { ContextMenuItem, InputDialogConfig } from "$lib/shared/types"; import type { FolderInfo } from "$lib/features/folders/types"; -import { createFolder, deleteFolder, renameFolder } from "$lib/features/folders/store"; +import { + createFolder, + deleteFolder, + findItemFolderId, + getFolderPath, + listFolders, + moveItem, + renameFolder, +} from "$lib/features/folders/store"; import type { MessageKey, TranslationParams } from "$lib/i18n"; type ContextMenuState = { @@ -109,6 +117,22 @@ export function createAppDialogsController({ }, t, }, + folderCallbacks: { + t, + getFolders: () => + listFolders(getActiveTab()).map((folder) => ({ + id: folder.id, + label: folderPathLabel(folder.id), + })), + getCurrentFolderId: () => + findItemFolderId({ type: "account", id: account.id }, getActiveTab()), + moveToFolder: (folderId) => { + const platform = getActiveTab(); + const itemRef = { type: "account", id: account.id } as const; + moveItem(itemRef, findItemFolderId(itemRef, platform), folderId, platform); + refreshCurrentItems(); + }, + }, appearanceCallbacks: { t, getCurrentColor: () => getAccountCardColor(account.id), @@ -200,6 +224,13 @@ export function createAppDialogsController({ let confirmDialogConfirmLabel = $derived(confirmDialog?.confirmLabel || t("common.confirm")); let confirmDialogConfirmColor = $derived(confirmDialog?.confirmColor || ""); + /** "Parent / Child", so two folders sharing a name stay tellable apart. */ + function folderPathLabel(folderId: string): string { + const path = getFolderPath(folderId); + if (path.length === 0) return folderId; + return path.map((folder) => folder.name).join(" / "); + } + function openInputDialog(config: InputDialogConfig & { maxlength?: number }) { inputDialog = { title: config.title, diff --git a/src/lib/app/useAppUpdater.svelte.ts b/src/lib/app/useAppUpdater.svelte.ts index 61c429a..a330710 100644 --- a/src/lib/app/useAppUpdater.svelte.ts +++ b/src/lib/app/useAppUpdater.svelte.ts @@ -6,6 +6,58 @@ import type { MessageKey, TranslationParams } from "$lib/i18n"; type PendingUpdate = NonNullable>>; type UpdateState = "idle" | "checking" | "downloading" | "ready" | "applying"; +/** The three ways a manifest check can end badly, as telemetry error codes. */ +export type UpdateCheckErrorCode = + | "update_target_missing" + | "update_manifest_invalid" + | "check_failed"; + +// The plugin serializes its error enum through Display, so a rejection is a +// string and there is no variant to switch on. These patterns come from +// tauri-plugin-updater 2.10.1 src/error.rs. + +// Error::TargetNotFound and Error::TargetsNotFound. Both end on the same +// phrase, and both mean the same thing: this build's target key is absent from +// latest.json, so the release simply does not ship an update for this OS. +const TARGET_MISSING_RE = /found in the response `platforms` object/; + +// The endpoint answered, but the answer is not a release manifest we can use: +// Error::ReleaseNotFound, Error::SignatureUtf8, Error::InvalidUpdaterFormat, +// and the serde_json and semver errors the plugin forwards verbatim. +const MANIFEST_INVALID_PATTERNS = [ + /Could not fetch a valid release JSON/i, + /could not be decoded/i, + /invalid updater binary format/i, + /missing field/i, + /expected value at line/i, + /invalid type:/i, + /trailing characters at line/i, + /while parsing (major|minor|patch) version number/i, +]; + +function errorText(error: unknown): string { + if (typeof error === "string") return error; + if (error instanceof Error) return error.message; + return String(error); +} + +/** + * Sorts a failed `check()` into a telemetry code. + * + * A missing target is not the same incident as a dead endpoint: one is a + * release that never published an artifact for this platform, the other is a + * network or infrastructure fault. Collapsing both into `check_failed` made + * every Linux and macOS launch look like an outage. + */ +export function classifyUpdateCheckError(error: unknown): UpdateCheckErrorCode { + const text = errorText(error); + if (TARGET_MISSING_RE.test(text)) return "update_target_missing"; + if (MANIFEST_INVALID_PATTERNS.some((pattern) => pattern.test(text))) { + return "update_manifest_invalid"; + } + return "check_failed"; +} + type AppUpdaterOptions = { t: (key: MessageKey, params?: TranslationParams) => string; addToast: (message: string) => void; @@ -68,12 +120,16 @@ export function createAppUpdater({ t, addToast, beforeRelaunch }: AppUpdaterOpti : t("update.readyToast"), ); } catch (error) { - console.error("Updater check/download failed:", error); - trackUpdate( - "failed", - updateVersion || undefined, - stage === "check" ? "check_failed" : "download_failed", - ); + const errorCode = stage === "check" ? classifyUpdateCheckError(error) : "download_failed"; + if (errorCode === "update_target_missing") { + // Nothing broke and nothing is wrong with this install: the release + // just carries no artifact for this platform. Logging it as an error + // buries the real ones. + console.info("Updater: this platform is not in the release manifest:", error); + } else { + console.error("Updater check/download failed:", error); + } + trackUpdate("failed", updateVersion || undefined, errorCode); pendingUpdate = null; updateVersion = ""; updateState = "idle"; diff --git a/src/lib/app/useAppUpdater.test.ts b/src/lib/app/useAppUpdater.test.ts new file mode 100644 index 0000000..2d5c6a3 --- /dev/null +++ b/src/lib/app/useAppUpdater.test.ts @@ -0,0 +1,111 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +const mocks = vi.hoisted(() => ({ + check: vi.fn(), + relaunch: vi.fn(), + trackUpdate: vi.fn(), +})); + +vi.mock("@tauri-apps/plugin-updater", () => ({ + check: (...args: unknown[]) => mocks.check(...args), +})); + +vi.mock("@tauri-apps/plugin-process", () => ({ + relaunch: (...args: unknown[]) => mocks.relaunch(...args), +})); + +vi.mock("$lib/app/telemetryClient", () => ({ + trackUpdate: (...args: unknown[]) => mocks.trackUpdate(...args), +})); + +import { classifyUpdateCheckError, createAppUpdater } from "./useAppUpdater.svelte"; + +/** The exact Display strings of tauri-plugin-updater 2.10.1 src/error.rs. */ +const TARGET_NOT_FOUND = + "the platform `linux-x86_64` was not found in the response `platforms` object"; +const TARGETS_NOT_FOUND = + 'None of the fallback platforms `["darwin-aarch64-app", "darwin-aarch64"]` ' + + "were found in the response `platforms` object"; + +function updater() { + return createAppUpdater({ + t: (key) => key, + addToast: vi.fn(), + }); +} + +describe("update check error codes", () => { + beforeEach(() => { + // The flow is a no-op in dev, which is the mode vitest runs in. + vi.stubEnv("DEV", false); + mocks.check.mockReset(); + mocks.trackUpdate.mockReset(); + vi.spyOn(console, "error").mockImplementation(() => {}); + vi.spyOn(console, "info").mockImplementation(() => {}); + }); + + afterEach(() => { + vi.unstubAllEnvs(); + vi.restoreAllMocks(); + }); + + it("reports a missing platform entry as its own code, not as a failed check", async () => { + mocks.check.mockRejectedValue(TARGET_NOT_FOUND); + + await updater().startBackgroundUpdateFlow(); + + expect(mocks.trackUpdate).toHaveBeenCalledWith("failed", undefined, "update_target_missing"); + // A release that ships no artifact for this OS is not an incident. + expect(console.error).not.toHaveBeenCalled(); + }); + + it("reports an unreadable manifest apart from a dead endpoint", async () => { + mocks.check.mockRejectedValue( + new Error("Could not fetch a valid release JSON from the remote"), + ); + + await updater().startBackgroundUpdateFlow(); + + expect(mocks.trackUpdate).toHaveBeenCalledWith("failed", undefined, "update_manifest_invalid"); + }); + + it("still reports a network failure as check_failed", async () => { + mocks.check.mockRejectedValue(new Error("error sending request for url (https://github.com)")); + + await updater().startBackgroundUpdateFlow(); + + expect(mocks.trackUpdate).toHaveBeenCalledWith("failed", undefined, "check_failed"); + expect(console.error).toHaveBeenCalled(); + }); + + it("stays quiet when there is no update", async () => { + mocks.check.mockResolvedValue(null); + + await updater().startBackgroundUpdateFlow(); + + expect(mocks.trackUpdate).not.toHaveBeenCalled(); + }); +}); + +describe("classifyUpdateCheckError", () => { + it("matches both shapes of the missing-target error", () => { + expect(classifyUpdateCheckError(TARGET_NOT_FOUND)).toBe("update_target_missing"); + expect(classifyUpdateCheckError(TARGETS_NOT_FOUND)).toBe("update_target_missing"); + }); + + it("matches the parse and signature errors the plugin forwards", () => { + expect(classifyUpdateCheckError("missing field `version` at line 1 column 42")).toBe( + "update_manifest_invalid", + ); + expect( + classifyUpdateCheckError( + "The signature ...= could not be decoded, please check if it is a valid base64 string.", + ), + ).toBe("update_manifest_invalid"); + }); + + it("falls back to check_failed for anything else", () => { + expect(classifyUpdateCheckError("operation timed out")).toBe("check_failed"); + expect(classifyUpdateCheckError(undefined)).toBe("check_failed"); + }); +}); diff --git a/src/lib/app/useOnboardingTour.svelte.ts b/src/lib/app/useOnboardingTour.svelte.ts index 047afb4..b7725a8 100644 --- a/src/lib/app/useOnboardingTour.svelte.ts +++ b/src/lib/app/useOnboardingTour.svelte.ts @@ -39,7 +39,7 @@ export function createOnboardingTour({ t, getActiveTab, setActiveTab }: Onboardi id, displayName: t("onboarding.features.mockAccount", { number: index + 1 }), username: `account_${index + 1}`, - lastLoginAt: null, + lastLoginAtSec: null, })), ); const mockItems = $derived( diff --git a/src/lib/app/useSecureScreen.svelte.ts b/src/lib/app/useSecureScreen.svelte.ts index 6d38b5d..f1f0e4a 100644 --- a/src/lib/app/useSecureScreen.svelte.ts +++ b/src/lib/app/useSecureScreen.svelte.ts @@ -20,6 +20,11 @@ type SecureScreenDeps = { getIsAccountSelectionView: () => boolean; getAppVersion: () => string; onCloseContextMenu: () => void; + /** + * Write a PBKDF2 hash that just replaced a legacy unsalted one. Called at + * most once per legacy PIN, right after it unlocked the screen. + */ + persistPinHash: (hash: string) => void; t: (key: MessageKey, params?: TranslationParams) => string; }; @@ -35,6 +40,7 @@ export function createSecureScreenController({ getIsAccountSelectionView, getAppVersion, onCloseContextMenu, + persistPinHash, t, }: SecureScreenDeps) { const startupPinLocked = Boolean( @@ -168,7 +174,7 @@ export function createSecureScreenController({ if (attemptPin.length !== PIN_CODE_LENGTH || isPinRetryLocked) return; isPinUnlocking = true; pinError = ""; - const matches = await verifyPinCode(attemptPin, expectedPinHash); + const { matches, rehashed } = await verifyPinCode(attemptPin, expectedPinHash); if (!matches) { isPinUnlocking = false; isPinRetryLocked = true; @@ -184,6 +190,10 @@ export function createSecureScreenController({ }, PIN_FAILURE_DELAY_MS); return; } + // The unlock succeeded against the old unsalted hash. Store the PBKDF2 + // one now, while the digits are still in hand, so the next unlock (here + // or in the CLI) runs the salted path. + if (rehashed) persistPinHash(rehashed); pinAttempt = ""; setTimeout(() => { isPinLocked = false; diff --git a/src/lib/features/commandPalette/CommandPalette.svelte b/src/lib/features/commandPalette/CommandPalette.svelte index d6a4e52..a9edfbe 100644 --- a/src/lib/features/commandPalette/CommandPalette.svelte +++ b/src/lib/features/commandPalette/CommandPalette.svelte @@ -193,7 +193,7 @@ .palette-overlay { position: fixed; inset: 0; - z-index: 1100; + z-index: var(--z-dialog); display: flex; justify-content: center; align-items: flex-start; diff --git a/src/lib/features/folders/BackCard.svelte b/src/lib/features/folders/BackCard.svelte index f322b93..f6767fe 100644 --- a/src/lib/features/folders/BackCard.svelte +++ b/src/lib/features/folders/BackCard.svelte @@ -1,7 +1,7 @@