From 22bd8540f7687238ebc21c664a0b149c68d787b3 Mon Sep 17 00:00:00 2001 From: Cory Douthat <80133232+corydouthat-sq@users.noreply.github.com> Date: Tue, 15 Sep 2026 02:06:31 -0400 Subject: [PATCH 1/2] fix(builderbot-auth): default non-macOS session storage to a file The OS keyring backend for browser auth sessions is only implemented on macOS, yet default_session_storage_for_bb_home returned it on every platform and stored_session_credential_header_value short-circuited to Ok(None) off macOS unless BB_AUTH_STORAGE or BB_AUTH_STORAGE_FILE was set. On Windows and Linux that meant kgoose requests went out without the session header and Connections looked silently broken until each user exported BB_AUTH_STORAGE=file by hand. Default to the existing file store under bb home on non-macOS platforms, drop the early return, and advertise only the storage kinds a platform actually supports. macOS behaviour is unchanged. The file is protected by directory permissions rather than a keyring: 0600 on Unix, the user-profile ACL on Windows; the docs and the doctor warning now say so. Tests cover the platform default, the explicit file overrides, the macOS-only keyring refusal, header lookup through the default store, and the error text for unsupported values. Co-Authored-By: Claude Fable 5.1 --- bb-cli/docs/bb-auth-local-testing.md | 2 +- crates/builderbot-auth/src/auth_storage.rs | 264 +++++++++++++++++++-- scripts/windows/Doctor-Windows.ps1 | 2 +- 3 files changed, 251 insertions(+), 17 deletions(-) diff --git a/bb-cli/docs/bb-auth-local-testing.md b/bb-cli/docs/bb-auth-local-testing.md index 5e3228cb4..3739c5d44 100644 --- a/bb-cli/docs/bb-auth-local-testing.md +++ b/bb-cli/docs/bb-auth-local-testing.md @@ -63,7 +63,7 @@ Expected result: - kgoose validates the state cookie, exchanges the Auth0 code server-side, and redirects to the CLI loopback callback with a one-time exchange code - the CLI exchanges that code through kgoose and stores the returned session credential -By default, the CLI stores browser auth sessions in the OS keyring. For local debugging without touching keyring state, use the `BB_AUTH_STORAGE=file` command above. +By default, the CLI stores browser auth sessions in the OS keyring on macOS. On Windows and Linux the default is a JSON file, `/auth-sessions.json`, protected by directory permissions rather than a keyring (0600 on Unix, the user-profile ACL on Windows). For local debugging without touching the default store on any platform, use the `BB_AUTH_STORAGE=file` command above. ## Test In Staging diff --git a/crates/builderbot-auth/src/auth_storage.rs b/crates/builderbot-auth/src/auth_storage.rs index 69ed0c76a..ec8111981 100644 --- a/crates/builderbot-auth/src/auth_storage.rs +++ b/crates/builderbot-auth/src/auth_storage.rs @@ -2,7 +2,7 @@ use std::collections::BTreeMap; #[cfg(any(debug_assertions, test))] use std::collections::HashMap; use std::fs; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; #[cfg(any(debug_assertions, test))] use std::sync::Mutex; @@ -129,7 +129,16 @@ pub fn default_session_storage_for_bb_home( path, )))); } - Ok(Box::new(KeyringSessionCredentialStorage)) + // The OS keyring backend exists only on macOS. Elsewhere, default + // to the file store under bb home so Windows and Linux builds hold + // a session without every user first exporting BB_AUTH_STORAGE. + // That file is protected by directory permissions, not a keyring: + // 0600 on Unix, the user-profile ACL on Windows. + if cfg!(target_os = "macos") { + Ok(Box::new(KeyringSessionCredentialStorage)) + } else { + file_storage_from_env(&bb_home) + } } Err(error) => Err(anyhow!("read {BB_AUTH_STORAGE_ENV_VAR}: {error}")), } @@ -140,13 +149,6 @@ pub fn stored_session_credential_header_value( server_url: &str, bb_home: PathBuf, ) -> Result> { - #[cfg(not(target_os = "macos"))] - if std::env::var_os(BB_AUTH_STORAGE_ENV_VAR).is_none() - && std::env::var_os(BB_AUTH_STORAGE_FILE_ENV_VAR).is_none() - { - return Ok(None); - } - let storage = default_session_storage_for_bb_home(bb_home)?; let storage_key = SessionStorageKey::new(profile, server_url); Ok(storage @@ -182,10 +184,11 @@ pub fn kgoose_auth_storage_lookup_urls(base_url: &str, service_path: &str) -> Ve } fn supported_storage_values() -> &'static str { - if cfg!(debug_assertions) { - "keyring, memory, file, or file:" - } else { - "keyring, file, or file:" + match (cfg!(target_os = "macos"), cfg!(debug_assertions)) { + (true, true) => "keyring, memory, file, or file:", + (true, false) => "keyring, file, or file:", + (false, true) => "memory, file, or file: (keyring is macOS-only)", + (false, false) => "file or file: (keyring is macOS-only)", } } @@ -399,7 +402,7 @@ fn keyring_delete_legacy_compose_token(_key: &SessionStorageKey) -> Result #[cfg(not(target_os = "macos"))] fn unsupported_keyring_storage() -> Result { anyhow::bail!( - "OS keyring browser auth storage is currently only implemented on macOS; set {BB_AUTH_STORAGE_ENV_VAR}=file for local testing" + "OS keyring browser auth storage is only implemented on macOS; other platforms default to file storage under bb home, or set {BB_AUTH_STORAGE_ENV_VAR}=file:" ) } @@ -414,7 +417,7 @@ fn parse_stored_session(value: &str) -> Result { } } -fn restrict_permissions(path: &PathBuf) -> Result<()> { +fn restrict_permissions(path: &Path) -> Result<()> { #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; @@ -422,6 +425,10 @@ fn restrict_permissions(path: &PathBuf) -> Result<()> { fs::set_permissions(path, permissions) .with_context(|| format!("chmod 600 {}", path.display()))?; } + // Windows has no mode bits to tighten; the file inherits the ACL of the + // user's bb home directory. + #[cfg(not(unix))] + let _ = path; Ok(()) } @@ -429,6 +436,233 @@ fn restrict_permissions(path: &PathBuf) -> Result<()> { mod tests { use super::*; + /// `default_session_storage_for_bb_home` reads process-global environment + /// variables, so the tests that touch them take this lock and restore the + /// previous values when the guard drops. + static ENV_LOCK: Mutex<()> = Mutex::new(()); + + struct StorageEnv { + _guard: std::sync::MutexGuard<'static, ()>, + saved: Vec<(&'static str, Option)>, + } + + impl StorageEnv { + fn cleared() -> Self { + let guard = ENV_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + let names = [BB_AUTH_STORAGE_ENV_VAR, BB_AUTH_STORAGE_FILE_ENV_VAR]; + let saved = names + .iter() + .map(|name| (*name, std::env::var_os(name))) + .collect(); + for name in names { + std::env::remove_var(name); + } + Self { + _guard: guard, + saved, + } + } + + fn set(&self, name: &str, value: &str) { + std::env::set_var(name, value); + } + } + + impl Drop for StorageEnv { + fn drop(&mut self) { + for (name, value) in self.saved.drain(..) { + match value { + Some(value) => std::env::set_var(name, value), + None => std::env::remove_var(name), + } + } + } + } + + fn scratch_dir(label: &str) -> PathBuf { + std::env::temp_dir().join(format!( + "bb-auth-storage-{label}-{}", + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .expect("clock") + .as_nanos() + )) + } + + fn sample_credential() -> StoredSessionCredential { + StoredSessionCredential { + session_credential: "session-token".to_string(), + expires_at: None, + } + } + + const SAMPLE_SERVER_URL: &str = "https://kgoose.example.test/cash-app/goose"; + + #[test] + fn default_storage_without_env_matches_the_platform() { + let _env = StorageEnv::cleared(); + let bb_home = scratch_dir("platform-default"); + + let storage = default_session_storage_for_bb_home(bb_home.clone()).expect("storage"); + + let expected = if cfg!(target_os = "macos") { + "keyring" + } else { + "file" + }; + assert_eq!(storage.kind(), expected); + let _ = fs::remove_dir_all(bb_home); + } + + #[cfg(not(target_os = "macos"))] + #[test] + fn default_file_storage_lives_under_bb_home_and_round_trips() { + let _env = StorageEnv::cleared(); + let bb_home = scratch_dir("bb-home-default"); + let key = SessionStorageKey::new("default", SAMPLE_SERVER_URL); + + let storage = default_session_storage_for_bb_home(bb_home.clone()).expect("storage"); + storage + .set(&key, &sample_credential()) + .expect("store credential"); + + assert!( + bb_home.join("auth-sessions.json").is_file(), + "default file storage must be /auth-sessions.json" + ); + assert_eq!( + storage + .get(&key) + .expect("read credential") + .expect("credential") + .session_credential, + "session-token" + ); + assert!(storage.delete(&key).expect("delete credential")); + assert!(storage.get(&key).expect("read after delete").is_none()); + let _ = fs::remove_dir_all(bb_home); + } + + #[cfg(not(target_os = "macos"))] + #[test] + fn stored_header_value_reads_the_default_file_storage() { + let _env = StorageEnv::cleared(); + let bb_home = scratch_dir("header-default"); + let key = SessionStorageKey::new("default", SAMPLE_SERVER_URL); + + assert_eq!( + stored_session_credential_header_value("default", SAMPLE_SERVER_URL, bb_home.clone()) + .expect("lookup before storing"), + None + ); + default_session_storage_for_bb_home(bb_home.clone()) + .expect("storage") + .set(&key, &sample_credential()) + .expect("store credential"); + + assert_eq!( + stored_session_credential_header_value("default", SAMPLE_SERVER_URL, bb_home.clone()) + .expect("lookup after storing"), + Some("session-token".to_string()) + ); + let _ = fs::remove_dir_all(bb_home); + } + + #[cfg(not(target_os = "macos"))] + #[test] + fn explicit_keyring_request_is_refused_off_macos() { + let env = StorageEnv::cleared(); + env.set(BB_AUTH_STORAGE_ENV_VAR, "keyring"); + let key = SessionStorageKey::new("default", SAMPLE_SERVER_URL); + + let storage = + default_session_storage_for_bb_home(scratch_dir("keyring-refused")).expect("storage"); + + assert_eq!(storage.kind(), "keyring"); + let error = storage.get(&key).expect_err("keyring is macOS-only"); + assert!( + error.to_string().contains("only implemented on macOS"), + "unexpected error: {error}" + ); + } + + #[test] + fn explicit_file_path_override_wins_over_the_default() { + let env = StorageEnv::cleared(); + let directory = scratch_dir("file-path-override"); + let explicit = directory.join("explicit").join("sessions.json"); + env.set( + BB_AUTH_STORAGE_ENV_VAR, + &format!("file:{}", explicit.display()), + ); + let key = SessionStorageKey::new("default", SAMPLE_SERVER_URL); + + let storage = + default_session_storage_for_bb_home(directory.join("bb-home")).expect("storage"); + storage + .set(&key, &sample_credential()) + .expect("store credential"); + + assert_eq!(storage.kind(), "file"); + assert!( + explicit.is_file(), + "credential must land at the explicit path" + ); + assert!(!directory + .join("bb-home") + .join("auth-sessions.json") + .exists()); + let _ = fs::remove_dir_all(directory); + } + + #[test] + fn storage_file_env_var_wins_over_the_default() { + let env = StorageEnv::cleared(); + let directory = scratch_dir("file-env-override"); + let explicit = directory.join("from-env").join("sessions.json"); + env.set( + BB_AUTH_STORAGE_FILE_ENV_VAR, + &explicit.display().to_string(), + ); + let key = SessionStorageKey::new("default", SAMPLE_SERVER_URL); + + let storage = + default_session_storage_for_bb_home(directory.join("bb-home")).expect("storage"); + storage + .set(&key, &sample_credential()) + .expect("store credential"); + + assert_eq!(storage.kind(), "file"); + assert!( + explicit.is_file(), + "credential must land at BB_AUTH_STORAGE_FILE" + ); + let _ = fs::remove_dir_all(directory); + } + + #[test] + fn unsupported_storage_value_names_the_platform_choices() { + let env = StorageEnv::cleared(); + env.set(BB_AUTH_STORAGE_ENV_VAR, "cloud"); + + let error = default_session_storage_for_bb_home(scratch_dir("unsupported")) + .err() + .expect("unsupported storage must fail"); + + let message = error.to_string(); + assert!( + message.contains("file:"), + "unexpected error: {message}" + ); + assert_eq!( + message.contains("keyring, "), + cfg!(target_os = "macos"), + "keyring must only be advertised as a choice on macOS: {message}" + ); + } + #[test] fn file_storage_scopes_credentials_by_profile_and_server() { let directory = std::env::temp_dir().join(format!( diff --git a/scripts/windows/Doctor-Windows.ps1 b/scripts/windows/Doctor-Windows.ps1 index 2f7935482..7da3dfc23 100644 --- a/scripts/windows/Doctor-Windows.ps1 +++ b/scripts/windows/Doctor-Windows.ps1 @@ -200,7 +200,7 @@ try { Fail "Managed Goose" "$($_.Exception.Message). Run: just setup-windows" } -Warn "Native sign-in" "Berd native provider sign-in is not supported on Windows yet. Sign in on macOS or use explicit local credential/file storage for Windows verification." +Warn "Native sign-in" "Berd native provider sign-in is not supported on Windows yet. Browser auth sessions (bb, Connections) are stored in a file under bb home on Windows; no BB_AUTH_STORAGE setup is needed." Write-Host "" if ($script:Failures -gt 0) { From b038bdaa2d4d50b350a04b51f8e3b0f3160b73a1 Mon Sep 17 00:00:00 2001 From: Cory Douthat <80133232+corydouthat-sq@users.noreply.github.com> Date: Tue, 15 Sep 2026 19:50:00 -0400 Subject: [PATCH 2/2] fix(builderbot-auth): make the file session store process-safe and private on Windows Review feedback on the non-macOS file default: the store did an unlocked read-modify-write with an in-place truncating write, so the desktop app and the CLI could overwrite each other's entries or read a half-written document, and on Windows a process without HOME resolved bb home to a cwd-relative `.bb`. - Serialize writers with an exclusive OS lock on a sibling `.lock` file (`std::fs::File::lock`, so the crate's MSRV moves to 1.89) and replace the store atomically through a temp file plus rename. Readers never observe a partial document. - Resolve bb home from USERPROFILE on Windows when HOME is absent, so the credential file lands in an absolute per-user location. - On Windows, reset the credential file's ACL with icacls so only the current user has access (the 0600 equivalent) instead of relying on the inherited profile ACL. - Tests: concurrent writers in threads and in a second process keep every entry and leave no temp files; Windows-only tests for the USERPROFILE fallback and for the single-user ACL. Co-Authored-By: Claude Fable 5.1 --- bb-cli/docs/bb-auth-local-testing.md | 2 +- crates/builderbot-auth/Cargo.toml | 2 +- crates/builderbot-auth/src/auth_storage.rs | 329 +++++++++++++++++++-- crates/builderbot-auth/src/config.rs | 21 +- 4 files changed, 330 insertions(+), 24 deletions(-) diff --git a/bb-cli/docs/bb-auth-local-testing.md b/bb-cli/docs/bb-auth-local-testing.md index 3739c5d44..95bd19464 100644 --- a/bb-cli/docs/bb-auth-local-testing.md +++ b/bb-cli/docs/bb-auth-local-testing.md @@ -63,7 +63,7 @@ Expected result: - kgoose validates the state cookie, exchanges the Auth0 code server-side, and redirects to the CLI loopback callback with a one-time exchange code - the CLI exchanges that code through kgoose and stores the returned session credential -By default, the CLI stores browser auth sessions in the OS keyring on macOS. On Windows and Linux the default is a JSON file, `/auth-sessions.json`, protected by directory permissions rather than a keyring (0600 on Unix, the user-profile ACL on Windows). For local debugging without touching the default store on any platform, use the `BB_AUTH_STORAGE=file` command above. +By default, the CLI stores browser auth sessions in the OS keyring on macOS. On Windows and Linux the default is a JSON file, `/auth-sessions.json`, protected by file permissions rather than a keyring (mode 0600 on Unix; on Windows the file's ACL is reset so only the current user has access). Writes replace the file atomically and are serialized across the desktop app and the CLI through a sibling `auth-sessions.json.lock`. For local debugging without touching the default store on any platform, use the `BB_AUTH_STORAGE=file` command above. ## Test In Staging diff --git a/crates/builderbot-auth/Cargo.toml b/crates/builderbot-auth/Cargo.toml index e9ac8f260..c3bebd92c 100644 --- a/crates/builderbot-auth/Cargo.toml +++ b/crates/builderbot-auth/Cargo.toml @@ -2,7 +2,7 @@ name = "builderbot-auth" version = "0.1.0" edition = "2021" -rust-version = "1.88.0" +rust-version = "1.89.0" license = "Apache-2.0" publish = false diff --git a/crates/builderbot-auth/src/auth_storage.rs b/crates/builderbot-auth/src/auth_storage.rs index ec8111981..6fe8415ae 100644 --- a/crates/builderbot-auth/src/auth_storage.rs +++ b/crates/builderbot-auth/src/auth_storage.rs @@ -132,8 +132,10 @@ pub fn default_session_storage_for_bb_home( // The OS keyring backend exists only on macOS. Elsewhere, default // to the file store under bb home so Windows and Linux builds hold // a session without every user first exporting BB_AUTH_STORAGE. - // That file is protected by directory permissions, not a keyring: - // 0600 on Unix, the user-profile ACL on Windows. + // That file is protected by file permissions, not a keyring (0600 + // on Unix, a current-user-only ACL on Windows), replaced + // atomically, and locked across processes; see + // FileSessionCredentialStorage. if cfg!(target_os = "macos") { Ok(Box::new(KeyringSessionCredentialStorage)) } else { @@ -257,13 +259,76 @@ impl FileSessionCredentialStorage { serde_json::from_slice(&bytes).with_context(|| format!("parse {}", self.path.display())) } - fn write_entries(&self, entries: &BTreeMap) -> Result<()> { - if let Some(parent) = self.path.parent() { - fs::create_dir_all(parent).with_context(|| format!("create {}", parent.display()))?; + fn parent_dir(&self) -> PathBuf { + match self.path.parent() { + Some(parent) if !parent.as_os_str().is_empty() => parent.to_path_buf(), + _ => PathBuf::from("."), } + } + + /// Sibling lock file (`.lock`). It holds no secrets and stays on + /// disk; only the OS lock on it matters. + fn lock_path(&self) -> PathBuf { + let mut path = self.path.as_os_str().to_os_string(); + path.push(".lock"); + PathBuf::from(path) + } + + /// Runs `update` while holding an exclusive OS lock, so the desktop app and + /// the CLI cannot interleave their read-modify-write cycles and silently + /// drop each other's entries. + fn with_exclusive_lock(&self, update: impl FnOnce() -> Result) -> Result { + let parent = self.parent_dir(); + fs::create_dir_all(&parent).with_context(|| format!("create {}", parent.display()))?; + let lock_path = self.lock_path(); + let lock = fs::OpenOptions::new() + .create(true) + .write(true) + .truncate(false) + .open(&lock_path) + .with_context(|| format!("open {}", lock_path.display()))?; + lock.lock() + .with_context(|| format!("lock {}", lock_path.display()))?; + let result = update(); + lock.unlock() + .with_context(|| format!("unlock {}", lock_path.display()))?; + result + } + + /// Writes the full document to a temporary sibling, restricts it, then + /// renames it over the store. Readers therefore see either the previous or + /// the new complete document, never a truncated or half-written one. + fn write_entries(&self, entries: &BTreeMap) -> Result<()> { + let parent = self.parent_dir(); + fs::create_dir_all(&parent).with_context(|| format!("create {}", parent.display()))?; let json = serde_json::to_vec_pretty(entries).context("serialize auth session storage")?; - fs::write(&self.path, json).with_context(|| format!("write {}", self.path.display()))?; - restrict_permissions(&self.path) + let file_name = self + .path + .file_name() + .map(|name| name.to_string_lossy().into_owned()) + .unwrap_or_else(|| "auth-sessions.json".to_string()); + let nanos = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|elapsed| elapsed.as_nanos()) + .unwrap_or(0); + let temp_path = parent.join(format!(".{file_name}.tmp-{}-{nanos}", std::process::id())); + let written = self.replace_with(&temp_path, &json); + if written.is_err() { + let _ = fs::remove_file(&temp_path); + } + written + } + + fn replace_with(&self, temp_path: &Path, json: &[u8]) -> Result<()> { + fs::write(temp_path, json).with_context(|| format!("write {}", temp_path.display()))?; + restrict_permissions(temp_path)?; + fs::rename(temp_path, &self.path).with_context(|| { + format!( + "replace {} with {}", + self.path.display(), + temp_path.display() + ) + }) } fn legacy_purpose_tokens_path(&self) -> PathBuf { @@ -283,18 +348,22 @@ impl SessionCredentialStorage for FileSessionCredentialStorage { } fn set(&self, key: &SessionStorageKey, credential: &StoredSessionCredential) -> Result<()> { - let mut entries = self.read_entries()?; - entries.insert(key.hashed_id(), credential.clone()); - self.write_entries(&entries) + self.with_exclusive_lock(|| { + let mut entries = self.read_entries()?; + entries.insert(key.hashed_id(), credential.clone()); + self.write_entries(&entries) + }) } fn delete(&self, key: &SessionStorageKey) -> Result { - let mut entries = self.read_entries()?; - let removed = entries.remove(&key.hashed_id()).is_some(); - if removed { - self.write_entries(&entries)?; - } - Ok(removed) + self.with_exclusive_lock(|| { + let mut entries = self.read_entries()?; + let removed = entries.remove(&key.hashed_id()).is_some(); + if removed { + self.write_entries(&entries)?; + } + Ok(removed) + }) } fn delete_legacy_purpose_token_cache(&self, _key: &SessionStorageKey) -> Result { @@ -417,6 +486,11 @@ fn parse_stored_session(value: &str) -> Result { } } +/// Makes the credential file readable by the current user only: mode 0600 on +/// Unix; on Windows the ACL is reset so inherited entries (Administrators, +/// SYSTEM, anything granted on the parent) are dropped and only the current +/// user keeps access. Windows uses `icacls`, which ships with every supported +/// release, so no additional dependency is needed. fn restrict_permissions(path: &Path) -> Result<()> { #[cfg(unix)] { @@ -425,9 +499,35 @@ fn restrict_permissions(path: &Path) -> Result<()> { fs::set_permissions(path, permissions) .with_context(|| format!("chmod 600 {}", path.display()))?; } - // Windows has no mode bits to tighten; the file inherits the ACL of the - // user's bb home directory. - #[cfg(not(unix))] + #[cfg(windows)] + { + let user = match (std::env::var("USERDOMAIN"), std::env::var("USERNAME")) { + (Ok(domain), Ok(name)) if !domain.is_empty() && !name.is_empty() => { + format!("{domain}\\{name}") + } + (_, Ok(name)) if !name.is_empty() => name, + _ => anyhow::bail!( + "cannot restrict {}: USERNAME is not set, so the owner of the credential file is unknown", + path.display() + ), + }; + let output = std::process::Command::new("icacls") + .arg(path) + .arg("/inheritance:r") + .arg("/grant:r") + .arg(format!("{user}:F")) + .output() + .with_context(|| format!("run icacls for {}", path.display()))?; + if !output.status.success() { + anyhow::bail!( + "icacls could not restrict {}: {} {}", + path.display(), + String::from_utf8_lossy(&output.stdout).trim(), + String::from_utf8_lossy(&output.stderr).trim() + ); + } + } + #[cfg(not(any(unix, windows)))] let _ = path; Ok(()) } @@ -789,6 +889,197 @@ mod tests { ); } + fn credential(value: impl Into) -> StoredSessionCredential { + StoredSessionCredential { + session_credential: value.into(), + expires_at: None, + } + } + + #[test] + fn concurrent_writers_keep_every_entry() { + let directory = scratch_dir("concurrent-writers"); + let path = directory.join("sessions.json"); + let writers = 8; + let rounds = 5; + + let handles: Vec<_> = (0..writers) + .map(|index| { + let path = path.clone(); + std::thread::spawn(move || { + // Each writer opens its own handle, like a separate process. + let storage = FileSessionCredentialStorage::new(path); + let key = SessionStorageKey::new(format!("profile-{index}"), SAMPLE_SERVER_URL); + for round in 0..rounds { + storage + .set(&key, &credential(format!("token-{index}-{round}"))) + .expect("store credential"); + } + }) + }) + .collect(); + for handle in handles { + handle.join().expect("writer thread"); + } + + let storage = FileSessionCredentialStorage::new(path); + let entries = storage.read_entries().expect("merged file parses"); + assert_eq!(entries.len(), writers, "every writer's entry survives"); + for index in 0..writers { + let key = SessionStorageKey::new(format!("profile-{index}"), SAMPLE_SERVER_URL); + assert_eq!( + storage + .get(&key) + .expect("read credential") + .expect("credential") + .session_credential, + format!("token-{index}-{}", rounds - 1) + ); + } + let leftovers: Vec = fs::read_dir(&directory) + .expect("list directory") + .filter_map(Result::ok) + .map(|entry| entry.file_name().to_string_lossy().into_owned()) + .filter(|name| name.contains(".tmp-")) + .collect(); + assert!( + leftovers.is_empty(), + "temp files left behind: {leftovers:?}" + ); + let _ = fs::remove_dir_all(directory); + } + + const CHILD_WRITER_ENV: &str = "BB_AUTH_STORAGE_TEST_CHILD_PATH"; + + /// Second process for `concurrent_processes_keep_every_entry`. Runs as a + /// plain test but does nothing unless the parent set `CHILD_WRITER_ENV`. + #[test] + fn file_storage_child_writer() { + let Some(path) = std::env::var_os(CHILD_WRITER_ENV) else { + return; + }; + let storage = FileSessionCredentialStorage::new(PathBuf::from(path)); + let key = SessionStorageKey::new("child", SAMPLE_SERVER_URL); + for round in 0..25 { + storage + .set(&key, &credential(format!("child-{round}"))) + .expect("child stores credential"); + } + } + + #[test] + fn concurrent_processes_keep_every_entry() { + let directory = scratch_dir("concurrent-processes"); + let path = directory.join("sessions.json"); + let mut child = std::process::Command::new(std::env::current_exe().expect("test binary")) + .args([ + "--exact", + "auth_storage::tests::file_storage_child_writer", + "--test-threads=1", + ]) + .env(CHILD_WRITER_ENV, &path) + .stdout(std::process::Stdio::null()) + .spawn() + .expect("spawn child writer"); + + let storage = FileSessionCredentialStorage::new(path); + let parent_key = SessionStorageKey::new("parent", SAMPLE_SERVER_URL); + for round in 0..25 { + storage + .set(&parent_key, &credential(format!("parent-{round}"))) + .expect("parent stores credential"); + } + let status = child.wait().expect("wait for child writer"); + assert!(status.success(), "child writer failed: {status}"); + + let entries = storage.read_entries().expect("merged file parses"); + assert_eq!(entries.len(), 2, "both processes' entries survive"); + assert_eq!( + storage + .get(&parent_key) + .expect("read parent") + .expect("parent credential") + .session_credential, + "parent-24" + ); + assert_eq!( + storage + .get(&SessionStorageKey::new("child", SAMPLE_SERVER_URL)) + .expect("read child") + .expect("child credential") + .session_credential, + "child-24" + ); + let _ = fs::remove_dir_all(directory); + } + + #[cfg(windows)] + #[test] + fn windows_default_bb_home_uses_the_user_profile_when_home_is_absent() { + let _guard = ENV_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + let saved_home = std::env::var_os("HOME"); + let saved_profile = std::env::var_os("USERPROFILE"); + let profile = scratch_dir("userprofile"); + + std::env::remove_var("HOME"); + std::env::set_var("USERPROFILE", &profile); + let bb_home = crate::config::default_bb_home(); + + match saved_home { + Some(value) => std::env::set_var("HOME", value), + None => std::env::remove_var("HOME"), + } + match saved_profile { + Some(value) => std::env::set_var("USERPROFILE", value), + None => std::env::remove_var("USERPROFILE"), + } + + assert!( + bb_home.is_absolute(), + "bb home must not be cwd-relative: {}", + bb_home.display() + ); + assert_eq!(bb_home, profile.join(".bb")); + } + + #[cfg(windows)] + #[test] + fn windows_file_storage_grants_only_the_current_user() { + let directory = scratch_dir("acl"); + let path = directory.join("sessions.json"); + let storage = FileSessionCredentialStorage::new(path.clone()); + storage + .set( + &SessionStorageKey::new("default", SAMPLE_SERVER_URL), + &sample_credential(), + ) + .expect("store credential"); + + let output = std::process::Command::new("icacls") + .arg(&path) + .output() + .expect("icacls lists the ACL"); + let listing = String::from_utf8_lossy(&output.stdout); + let aces: Vec<&str> = listing.lines().filter(|line| line.contains(":(")).collect(); + let user = std::env::var("USERNAME").expect("USERNAME"); + assert_eq!( + aces.len(), + 1, + "exactly one access entry expected:\n{listing}" + ); + assert!( + aces[0].to_lowercase().contains(&user.to_lowercase()), + "the only access entry must be the current user:\n{listing}" + ); + assert!( + !listing.contains("BUILTIN\\") && !listing.contains("NT AUTHORITY\\"), + "inherited entries must be gone:\n{listing}" + ); + let _ = fs::remove_dir_all(directory); + } + #[cfg(target_os = "macos")] #[test] fn macos_keyring_item_shape_uses_legacy_service_and_account() { diff --git a/crates/builderbot-auth/src/config.rs b/crates/builderbot-auth/src/config.rs index 9e6f2ba6a..a6d8e9591 100644 --- a/crates/builderbot-auth/src/config.rs +++ b/crates/builderbot-auth/src/config.rs @@ -19,10 +19,25 @@ pub const BB_SKILLS_PROFILE_ENV_VAR: &str = "BB_SKILLS_PROFILE"; pub const DEFAULT_PROFILE_NAME: &str = "default"; pub const PREFERENCES_FILE_NAME: &str = "config.yaml"; +/// `/.bb`, where home is `HOME`, or `USERPROFILE` on Windows when a +/// native process has no `HOME` (Git Bash exports one; cmd, PowerShell, and +/// the desktop app do not). Only when neither exists does this fall back to a +/// working-directory-relative `.bb`, so bb state and the file-backed browser +/// auth store normally live in an absolute per-user location. pub fn default_bb_home() -> PathBuf { - env::var("HOME") - .map(|home| PathBuf::from(home).join(".bb")) - .unwrap_or_else(|_| PathBuf::from(".bb")) + let home = env::var_os("HOME") + .filter(|value| !value.is_empty()) + .or_else(|| { + if cfg!(windows) { + env::var_os("USERPROFILE").filter(|value| !value.is_empty()) + } else { + None + } + }); + match home { + Some(home) => PathBuf::from(home).join(".bb"), + None => PathBuf::from(".bb"), + } } pub fn default_preferences_path(bb_home: &Path) -> PathBuf {