From ad01b708f85c210b1bff7cacaafd4c7bbf8a4c33 Mon Sep 17 00:00:00 2001 From: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz> Date: Wed, 30 Sep 2026 11:05:14 -0600 Subject: [PATCH] feat(buzz-agent): persist the OAuth token cache owner-only on Windows On Windows the token cache was memory-only because std has no way to set an owner-only DACL, and the Win32 calls need `unsafe`, which buzz-agent forbids. Each process therefore ran its own sign-in, and Databricks stayed gated off on Windows in the desktop app. The new buzz-private-file crate holds that FFI behind a safe API, so buzz-agent keeps `forbid(unsafe_code)` on every platform: - `create_new` creates a file with the protected DACL `D:P(A;;FA;;;OW)` set atomically at creation, the Windows counterpart of `mode(0o600)`. - `open_owned` opens without following a final link, refuses anything but a regular file owned by the process's default owner, and then re-applies that DACL on the open handle, like the Unix `O_NOFOLLOW` + `fchmod` repair. auth.rs now persists and reads the cache on Windows through these, with the same temp-file + rename lifecycle as Unix. Tests that were Unix-only only because Windows had no disk cache now run on both, and Windows CI runs the helper crate (including its elevated planted-owner and symlink tests) plus the buzz-agent token-cache unit tests. Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz> --- .github/workflows/_ci-rust.yml | 25 +- Cargo.lock | 9 + Cargo.toml | 1 + crates/buzz-agent/Cargo.toml | 5 + crates/buzz-agent/DATABRICKS_REUSE.md | 5 +- crates/buzz-agent/src/auth.rs | 184 ++++----- crates/buzz-agent/src/databricks.rs | 4 +- .../buzz-agent/src/databricks_test_support.rs | 8 - crates/buzz-agent/src/databricks_tests.rs | 13 - .../tests/databricks_auth_coordinator.rs | 247 +----------- crates/buzz-private-file/Cargo.toml | 20 + crates/buzz-private-file/src/lib.rs | 365 ++++++++++++++++++ desktop/src-tauri/Cargo.lock | 8 + 13 files changed, 507 insertions(+), 387 deletions(-) create mode 100644 crates/buzz-private-file/Cargo.toml create mode 100644 crates/buzz-private-file/src/lib.rs diff --git a/.github/workflows/_ci-rust.yml b/.github/workflows/_ci-rust.yml index e25639d3ac..38daad4327 100644 --- a/.github/workflows/_ci-rust.yml +++ b/.github/workflows/_ci-rust.yml @@ -154,20 +154,23 @@ jobs: - name: Test (ACP launch-prefix platform contract) # Execute the non-Unix refusal branch; a cross-compile cannot test it. run: cargo test -p buzz-acp --target $env:TARGET --lib acp::launch::platform_tests + - name: Test (owner-only private files) + # The Win32 DACL code behind buzz-agent's token cache. The ignored + # tests plant a foreign-owned file and a symlink, which needs the + # runner's elevated shell. + run: cargo test -p buzz-private-file --target $env:TARGET -- --include-ignored - name: Test (buzz-agent auth coordinator) # The auth coordinator single-flights on an OS advisory lock, which is - # LockFileEx on Windows; this integration suite drives real second - # processes on the same lock file, so it only exercises the Windows - # lock runtime if it runs ON Windows. Every other job compiles it but - # never executes it. Tests exercised on Windows: lock serialization - # (two coordinators race for the same key), cooldown sidecar sharing - # across processes, attempt-sidecar adoption (UserInitiated waiter - # adopts a predecessor's denial), and the in-process single-flight for - # same-key coalescing. Tests that are UNIX-ONLY and NOT executed here: - # crash-release (flock drop on SIGKILL, guarded by #[cfg(unix)]) and - # cross-process cache success/race (on-disk token handoff, also - # #[cfg(unix)]). + # LockFileEx on Windows, and hands tokens across processes through the + # owner-only disk cache. This integration suite drives real second + # processes on the same lock and cache files, so it only exercises the + # Windows runtime if it runs ON Windows. Every other job compiles it but + # never executes it. run: cargo test -p buzz-agent --target $env:TARGET --test databricks_auth_coordinator + - name: Test (buzz-agent token cache) + # Disk cache reuse, joiner recovery and concurrent savers through the + # Windows owner-only cache path. + run: cargo test -p buzz-agent --target $env:TARGET --lib -- auth::tests databricks::tests # Smoke-test the new host-prereq contract: Git for Windows (which provides # bash) is available on the runner, a shell command round-trips, and bash # does NOT resolve from System32 (so WSL's launcher is never picked up). diff --git a/Cargo.lock b/Cargo.lock index c587e0abe0..49c00b388f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1018,6 +1018,7 @@ dependencies = [ "async-trait", "axum", "base64 0.22.1", + "buzz-private-file", "dirs", "fs2", "getrandom 0.4.3", @@ -1361,6 +1362,14 @@ dependencies = [ "thiserror 2.0.18", ] +[[package]] +name = "buzz-private-file" +version = "0.1.0" +dependencies = [ + "tempfile", + "windows-sys 0.61.2", +] + [[package]] name = "buzz-pubsub" version = "0.1.0" diff --git a/Cargo.toml b/Cargo.toml index d27c63d234..6b71adb085 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -12,6 +12,7 @@ members = [ "crates/buzz-audit", "crates/buzz-acp", "crates/buzz-agent", + "crates/buzz-private-file", "crates/sprig", "crates/buzz-test-client", "crates/buzz-ws-client", diff --git a/crates/buzz-agent/Cargo.toml b/crates/buzz-agent/Cargo.toml index 91a024a38b..a00d419eee 100644 --- a/crates/buzz-agent/Cargo.toml +++ b/crates/buzz-agent/Cargo.toml @@ -72,6 +72,11 @@ fs2 = "0.4" [target.'cfg(unix)'.dependencies] nix = { version = "0.31", default-features = false, features = ["signal", "process"] } +# Owner-only token cache files on Windows; keeps the Win32 FFI (and its +# `unsafe`) out of this crate. +[target.'cfg(windows)'.dependencies] +buzz-private-file = { path = "../buzz-private-file" } + [dev-dependencies] # Synthetic TLS OAuth/catalog servers exercise the strict HTTPS production path. rcgen = { version = "0.14", default-features = false, features = ["crypto", "ring"] } diff --git a/crates/buzz-agent/DATABRICKS_REUSE.md b/crates/buzz-agent/DATABRICKS_REUSE.md index 42bed686da..96ddf8846e 100644 --- a/crates/buzz-agent/DATABRICKS_REUSE.md +++ b/crates/buzz-agent/DATABRICKS_REUSE.md @@ -18,8 +18,9 @@ without running an agent, ACP, a CLI subprocess or any runtime configuration. - The caller chooses and exclusively owns its cache root BEFORE construction. `databricks-strict` separates strict state from legacy `databricks` caches and single-flight coordination even if roots coincide. It is not credential - migration. Existing host/client/scopes hash and Unix owner-only atomic token - persistence remain. Non-Unix token persistence remains disabled. + migration. Existing host/client/scopes hash and owner-only atomic token + persistence remain: mode `0600` on Unix, a protected owner-only DACL on + Windows. - Keep secrets native. The supplied opener must not log authorization URLs or its failure details. Drop the operation future to cancel actual requests and the callback listener. Set an app-level overall deadline, generation-fence diff --git a/crates/buzz-agent/src/auth.rs b/crates/buzz-agent/src/auth.rs index fc177a271d..aa69efd77b 100644 --- a/crates/buzz-agent/src/auth.rs +++ b/crates/buzz-agent/src/auth.rs @@ -516,20 +516,8 @@ impl PkceOAuthTokenSource { /// Persist a token to disk and the in-memory cell. /// /// The cache holds both the access and refresh tokens, so the on-disk - /// file is written owner-only (`0o600` on Unix) via an atomic - /// inode-swapping rename — see [`write_private_cache`]. - /// - /// On non-Unix platforms the token is stored in-memory only: the - /// `write_private_cache` path creates files with default ACLs, which do - /// not enforce owner-only access. Disk persistence is intentionally - /// disabled until a Windows-specific owner-only DACL is implemented (see - /// the `create_private_temp_file` non-Unix branch). The cost is that each - /// process performs its own acquisition on non-Unix — cross-process - /// *success* handoff requires the shared on-disk cache, so processes - /// serialize through the lock but the loser repeats the flow rather than - /// reading the winner's token. Cross-process *failure* adoption still works - /// because it uses the attempt sidecar (no token bytes). Correct and - /// safe until owner-only DACL persistence exists. + /// file is written owner-only (`0o600` on Unix, an owner-only DACL on + /// Windows) via an atomic rename — see [`write_private_cache`]. fn save(&self, state: &mut Option, token: CachedToken) -> Result<(), AgentError> { self.persist(&token)?; *state = Some(token); @@ -538,23 +526,12 @@ impl PkceOAuthTokenSource { /// Write `token` to the on-disk cache. Split out of [`save`](Self::save) so /// the 401 neutralization path can rewrite the disk layer without clobbering - /// a distinct in-memory entry. No-op on non-Unix (see [`save`](Self::save)). + /// a distinct in-memory entry. fn persist(&self, token: &CachedToken) -> Result<(), AgentError> { - #[cfg(unix)] - { - let body = serde_json::to_vec_pretty(token) - .map_err(|e| AgentError::Llm(format!("oauth cache serialize: {e}")))?; - write_private_cache(&self.cache_path, &body).map_err(|e| { - AgentError::Llm(format!("oauth cache write {:?}: {e}", self.cache_path)) - })?; - } - #[cfg(not(unix))] - { - // Disk persistence disabled on non-Unix: owner-only file - // permissions require a DACL that is not yet implemented. - let _ = token; - } - Ok(()) + let body = serde_json::to_vec_pretty(token) + .map_err(|e| AgentError::Llm(format!("oauth cache serialize: {e}")))?; + write_private_cache(&self.cache_path, &body) + .map_err(|e| AgentError::Llm(format!("oauth cache write {:?}: {e}", self.cache_path))) } /// Neutralize the matching rejected credential in B's own in-memory `state` @@ -1840,23 +1817,19 @@ fn read_private_cache(path: &Path) -> io::Result> { Ok(body) } -/// Non-Unix: token persistence and reading are both disabled until a -/// Windows-specific owner-only DACL is implemented. Any legacy token file -/// left by an older build (written with default ACLs) is deleted -/// opportunistically so the exposed artifact cannot be served by new builds. -/// Returns an error so [`read_cache`] yields `None`, giving a consistent -/// memory-only cache on non-Unix. -#[cfg(not(unix))] +/// Open the cache without following a final link, refuse it unless it is a +/// regular file owned by the current user, re-apply the owner-only DACL on +/// the open handle, and return its bytes. +/// +/// The Windows counterpart of the Unix `O_NOFOLLOW` + `fchmod` repair; see +/// [`buzz_private_file::open_owned`]. +#[cfg(windows)] fn read_private_cache(path: &Path) -> io::Result> { - // Best-effort removal of any legacy file. Errors are ignored — either the - // file does not exist (normal case) or it cannot be removed (no worse - // than before — the DACL story is still broken, but that is the pre-fix - // state we are trying to retire). - let _ = fs::remove_file(path); - Err(io::Error::new( - io::ErrorKind::Unsupported, - "token disk cache disabled on non-Unix (no owner-only DACL)", - )) + use std::io::Read; + + let mut body = Vec::new(); + buzz_private_file::open_owned(path)?.read_to_end(&mut body)?; + Ok(body) } /// Removes a temp file on drop unless it was already renamed away. Keeps a @@ -1894,10 +1867,9 @@ fn unique_suffix() -> String { /// world/other readable, write and fsync it, then rename over the /// destination. The rename swaps the inode/entry wholesale, so a pre-existing /// cache file with loose permissions is *replaced* by the new private one; -/// its old mode never survives. `fs::rename` maps to -/// `MOVEFILE_REPLACE_EXISTING` on Windows, so the atomic replace holds on -/// both platforms; the Windows owner-only DACL is pending the unsafe-FFI -/// decision noted at the seam. +/// its old mode or DACL never survives. `fs::rename` replaces an existing +/// destination on Windows too, so the atomic replace holds on both +/// platforms. fn write_private_cache(path: &Path, body: &[u8]) -> io::Result<()> { let parent = path.parent().ok_or_else(|| { io::Error::new( @@ -1938,22 +1910,11 @@ fn create_private_temp_file(tmp: &Path) -> io::Result { .open(tmp) } -/// Non-Unix fallback: create the temp file if it does not already exist. -/// -/// On Windows the owner-only equivalent is an explicit DACL set at creation -/// (`CreateFileW` with SDDL `D:P(A;;FA;;;OW)`, matching goose's -/// `private_file.rs`), but that FFI needs `unsafe`, which this crate forbids. -/// Reconciling the two — an isolated helper crate, a vetted safe dependency, -/// or descoping Windows — is an open decision escalated to the maintainer, so -/// this interim relies on the default per-user ACLs and drops the owner-only -/// implementation in behind this seam once the decision lands. `create_new` -/// fails if the file already exists. -#[cfg(not(unix))] +/// Windows: the same, with a protected owner-only DACL set atomically at +/// creation instead of a mode. +#[cfg(windows)] fn create_private_temp_file(tmp: &Path) -> io::Result { - fs::OpenOptions::new() - .write(true) - .create_new(true) - .open(tmp) + buzz_private_file::create_new(tmp) } /// Parse a token-endpoint JSON response. Fails loudly when `access_token` @@ -2327,7 +2288,6 @@ mod tests { assert!(token_from_response(&v, None).is_err()); } - #[cfg(unix)] // Disk adoption relies on `write_private_cache`; non-Unix disables disk persistence. #[tokio::test] async fn test_bearer_reuses_disk_token_after_expiry() { let dir = tempfile::tempdir().unwrap(); @@ -2381,10 +2341,6 @@ mod tests { /// exercises the joiner recovery branch rather than the initial fast-path /// `cached_hit`. Removing the joiner disk-recovery branch must make the /// test return Err(RefreshRejected) rather than Ok("sibling-replacement"). - /// - /// Disk-dependent: the replacement lives on disk, so `write_private_cache` - /// must be available (i.e. Unix only). - #[cfg(unix)] #[tokio::test] async fn test_joiner_shared_failure_recovers_disk_replacement() { use std::future::Future as _; @@ -2487,7 +2443,6 @@ mod tests { /// "disk unchanged" assertion fails — proving the unfenced write is exactly /// the race that would overwrite any concurrent C write that landed between /// A's failure and B's reconciliation. - #[cfg(unix)] #[tokio::test] async fn test_joiner_failure_does_not_write_disk() { use std::future::Future as _; @@ -2766,11 +2721,39 @@ mod tests { // ---- private atomic cache write -------------------------------------- - #[cfg(unix)] + /// Mode `0600` on Unix; on Windows a protected DACL whose only entry + /// grants the owner full access. + fn assert_owner_only(path: &Path, why: &str) { + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mode = fs::metadata(path).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o600, "{why}: got {mode:o}"); + } + #[cfg(windows)] + { + // SDDL, unlike `icacls` output, is not localized. + let out = std::process::Command::new("powershell") + .args([ + "-NoProfile", + "-Command", + "(Get-Acl -LiteralPath $env:ACL_PATH).Sddl", + ]) + .env("ACL_PATH", path) + .output() + .unwrap(); + let sddl = String::from_utf8_lossy(&out.stdout); + let dacl = sddl.trim().split_once("D:").map_or("", |(_, dacl)| dacl); + let (flags, aces) = dacl.split_once('(').unwrap_or_default(); + assert!( + flags.contains('P') && aces == "A;;FA;;;OW)", + "{why}: got {sddl}" + ); + } + } + #[tokio::test] async fn test_save_writes_owner_only_cache_file() { - use std::os::unix::fs::PermissionsExt; - let dir = tempfile::tempdir().unwrap(); let cfg = PkceOAuthConfig { discovery_url: "https://example.com/.well-known".into(), @@ -2795,16 +2778,7 @@ mod tests { .unwrap(); } - let mode = fs::metadata(&source.cache_path) - .unwrap() - .permissions() - .mode(); - assert_eq!( - mode & 0o777, - 0o600, - "cache file must be owner-only, got {:o}", - mode & 0o777 - ); + assert_owner_only(&source.cache_path, "cache file must be owner-only"); // No temp file left behind. let leftovers: Vec<_> = fs::read_dir(dir.path().join("test")) .unwrap() @@ -2860,11 +2834,8 @@ mod tests { assert_ne!(meta.ino(), old_inode, "cache inode was not replaced"); } - #[cfg(unix)] #[test] fn test_write_private_cache_concurrent_savers_all_succeed() { - use std::os::unix::fs::PermissionsExt; - // Unique tmp suffixes must let many concurrent writers to the SAME // destination each land a complete, private file — no tmp collision, // no failed rename. The fixed `*.json.tmp` name this replaces would @@ -2889,12 +2860,11 @@ mod tests { .expect("concurrent write_private_cache failed"); } - // Destination exists, is valid (one writer's complete body), and 0600. + // Destination exists and is valid (one writer's complete body). let contents = fs::read_to_string(path.as_ref()).unwrap(); let v: serde_json::Value = serde_json::from_str(&contents).unwrap(); assert!(v.get("n").is_some(), "cache body was corrupted: {contents}"); - let mode = fs::metadata(path.as_ref()).unwrap().permissions().mode(); - assert_eq!(mode & 0o777, 0o600); + assert_owner_only(&path, "concurrent saves must stay owner-only"); // No temp files leaked. let leftovers: Vec<_> = fs::read_dir(dir.path()) @@ -2907,14 +2877,12 @@ mod tests { // ---- owner-only is a load-time invariant, not just a save-time one ---- - #[cfg(unix)] #[tokio::test] async fn test_bearer_cache_hit_tightens_preexisting_loose_mode_file() { - use std::os::unix::fs::{MetadataExt, PermissionsExt}; - // A world-readable cache left by an older buzz-agent must be tightened // the moment we load it — on the plain cache-hit path, with no refresh // or save. Otherwise the pre-bug population stays exposed indefinitely. + // On Windows a plain `fs::write` already inherits the directory's ACL. let dir = tempfile::tempdir().unwrap(); let cfg = PkceOAuthConfig { discovery_url: "https://example.com/.well-known".into(), @@ -2939,26 +2907,32 @@ mod tests { expires_at: Some(future_exp), }; fs::write(&cache_path, serde_json::to_vec_pretty(&token).unwrap()).unwrap(); - fs::set_permissions(&cache_path, fs::Permissions::from_mode(0o644)).unwrap(); - let old_inode = fs::metadata(&cache_path).unwrap().ino(); + #[cfg(unix)] + let old_inode = { + use std::os::unix::fs::{MetadataExt, PermissionsExt}; + fs::set_permissions(&cache_path, fs::Permissions::from_mode(0o644)).unwrap(); + fs::metadata(&cache_path).unwrap().ino() + }; // Constructing the source loads the cache — repair happens here. let source = PkceOAuthTokenSource::new(cfg).unwrap(); let bearer = source.bearer().await.unwrap(); assert_eq!(bearer, "loose-but-valid"); - let meta = fs::metadata(&cache_path).unwrap(); - assert_eq!( - meta.permissions().mode() & 0o777, - 0o600, - "existing loose-mode cache was not tightened on load" + assert_owner_only( + &cache_path, + "existing loose cache was not tightened on load", ); // Repaired in place on the open fd — same inode, no rewrite/rename. - assert_eq!( - meta.ino(), - old_inode, - "load-time repair should fchmod in place, not replace the inode" - ); + #[cfg(unix)] + { + use std::os::unix::fs::MetadataExt; + assert_eq!( + fs::metadata(&cache_path).unwrap().ino(), + old_inode, + "load-time repair should fchmod in place, not replace the inode" + ); + } } #[cfg(unix)] diff --git a/crates/buzz-agent/src/databricks.rs b/crates/buzz-agent/src/databricks.rs index e695b24273..4c9e457d0d 100644 --- a/crates/buzz-agent/src/databricks.rs +++ b/crates/buzz-agent/src/databricks.rs @@ -36,8 +36,8 @@ impl DatabricksConnection { /// `workspace` must be an HTTPS origin (optional trailing slash), without /// userinfo, query or fragment. `cache_root` must be absolute and owned by /// this app; a separate `databricks-strict` namespace prevents accidental - /// coalescing with legacy auth. Unix token files retain owner-only atomic - /// persistence; on non-Unix the existing engine keeps credentials in memory. + /// coalescing with legacy auth. Token files keep owner-only atomic + /// persistence. /// The opener must not log the authorization URL or its own error details. pub fn new( workspace: &str, diff --git a/crates/buzz-agent/src/databricks_test_support.rs b/crates/buzz-agent/src/databricks_test_support.rs index 39b8b53995..ed19c9f6a5 100644 --- a/crates/buzz-agent/src/databricks_test_support.rs +++ b/crates/buzz-agent/src/databricks_test_support.rs @@ -28,16 +28,13 @@ impl Request { } pub(super) enum Reply { Json(u16, serde_json::Value), - #[cfg(unix)] Raw(String), - #[cfg(unix)] Stall, } pub(super) struct Server { pub base: String, pub cert: Option, pub requests: Arc>>, - #[cfg_attr(not(unix), allow(dead_code))] pub active: Arc, task: tokio::task::JoinHandle<()>, } @@ -124,7 +121,6 @@ impl Server { .filter(|r| r.path.starts_with(path)) .count() } - #[cfg(unix)] pub async fn wait_for(&self, path: &str) { tokio::time::timeout(Duration::from_secs(3), async { while self.count(path) == 0 { @@ -203,9 +199,7 @@ async fn serve( let body = body.to_string(); format!("HTTP/1.1 {status} Test\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{body}", body.len()) } - #[cfg(unix)] Reply::Raw(raw) => raw, - #[cfg(unix)] Reply::Stall => { // Send headers, then stall the body until the client cancels. let _ = stream @@ -268,7 +262,6 @@ pub(super) fn discovery(base: &str) -> Reply { serde_json::json!({"authorization_endpoint": format!("{base}/authorize"), "token_endpoint": format!("{base}/token")}), ) } -#[cfg(unix)] pub(super) fn seed(root: &Path, base: &str, namespace: &str, token: &str, expired: bool) { use sha2::{Digest, Sha256}; let key = format!( @@ -278,7 +271,6 @@ pub(super) fn seed(root: &Path, base: &str, namespace: &str, token: &str, expire std::fs::create_dir_all(&dir).unwrap(); std::fs::write(dir.join(format!("{}.json", hex::encode(Sha256::digest(key)))), serde_json::json!({"access_token": token, "refresh_token": "synthetic-refresh", "expires_at": if expired { 1 } else { 4_000_000_000u64 }}).to_string()).unwrap(); } -#[cfg(unix)] pub(super) fn chunked(status: u16, body: &str) -> Reply { Reply::Raw(format!("HTTP/1.1 {status} Test\r\nTransfer-Encoding: chunked\r\nConnection: close\r\n\r\n{:x}\r\n{body}\r\n0\r\n\r\n", body.len())) } diff --git a/crates/buzz-agent/src/databricks_tests.rs b/crates/buzz-agent/src/databricks_tests.rs index 02517a8e12..e7cdaf40c3 100644 --- a/crates/buzz-agent/src/databricks_tests.rs +++ b/crates/buzz-agent/src/databricks_tests.rs @@ -1,8 +1,6 @@ use super::*; -#[cfg(unix)] use crate::auth::TokenSource; use serde_json::json; -#[cfg(unix)] use std::sync::atomic::Ordering; #[path = "databricks_test_support.rs"] @@ -12,7 +10,6 @@ use support::*; fn strict(server: &Server, root: &Path, opener: Arc) -> DatabricksConnection { DatabricksConnection::build(&server.base, root, opener, server.builder()).unwrap() } -#[cfg(unix)] fn legacy(server: &Server, root: &Path, opener: Arc) -> Arc { PkceOAuthTokenSource::new_with( crate::llm::databricks_pkce_config(&server.base, Some(root.into())), @@ -74,7 +71,6 @@ async fn strict_headless_empty_never_networks_or_browses() { assert!(opener.urls.lock().unwrap().is_empty()); } -#[cfg(unix)] #[tokio::test] async fn strict_rejects_discovered_endpoints_before_grants_or_browser() { let sink = Server::start(true, |_, _| { @@ -204,7 +200,6 @@ async fn strict_connect_code_exchange_catalog_and_headless_401_refresh() { ); } -#[cfg(unix)] #[tokio::test] async fn strict_never_follows_discovery_token_or_catalog_redirects() { let sink = Server::start(true, |_, _| { @@ -274,7 +269,6 @@ async fn strict_never_follows_discovery_token_or_catalog_redirects() { assert_eq!(sink.count("/control"), 1); } -#[cfg(unix)] #[tokio::test] async fn strict_catalog_preserves_empty_filter_partial_and_paging_semantics() { for scenario in ["empty", "filtered-empty", "partial", "pages"] { @@ -323,7 +317,6 @@ async fn strict_catalog_preserves_empty_filter_partial_and_paging_semantics() { } } -#[cfg(unix)] #[tokio::test] async fn strict_cancellation_drops_real_http_work_and_allows_retry() { for stage in ["discovery", "refresh", "exchange", "catalog"] { @@ -432,7 +425,6 @@ async fn strict_browser_wait_cancellation_releases_callback_listener() { .unwrap(); } -#[cfg(unix)] #[tokio::test] async fn legacy_different_origin_endpoints_and_redirects_remain_supported() { let sink = Server::start(false, |_, r| { @@ -462,7 +454,6 @@ async fn legacy_different_origin_endpoints_and_redirects_remain_supported() { assert_eq!(sink.count("/token"), 2); } -#[cfg(unix)] #[tokio::test] async fn strict_host_root_and_legacy_namespace_isolation() { let server = Server::start(true, |_, _| panic!("should not reach network")).await; @@ -500,7 +491,6 @@ async fn strict_host_root_and_legacy_namespace_isolation() { assert_eq!(c.source.bearer_no_browser().await.unwrap(), "strict-a"); } -#[cfg(unix)] #[tokio::test] async fn shared_oauth_limits_bound_chunked_and_declared_bodies_in_both_paths() { use crate::auth_http::{MAX_OAUTH_ERROR_BYTES, MAX_OAUTH_RESPONSE_BYTES}; @@ -573,7 +563,6 @@ async fn shared_oauth_limits_bound_chunked_and_declared_bodies_in_both_paths() { } } -#[cfg(unix)] #[tokio::test] async fn shared_exact_limit_responses_and_grant_classifications_are_preserved() { use crate::auth_http::{MAX_OAUTH_ERROR_BYTES, MAX_OAUTH_RESPONSE_BYTES}; @@ -646,7 +635,6 @@ async fn shared_exact_limit_responses_and_grant_classifications_are_preserved() } } -#[cfg(unix)] #[tokio::test] async fn shared_diagnostics_never_log_oauth_bodies_urls_opener_or_callback_details() { use std::{io::Write, sync::Mutex}; @@ -790,7 +778,6 @@ async fn strict_malformed_token_expiry_is_a_safe_infrastructure_failure() { ); } -#[cfg(unix)] #[tokio::test] async fn strict_catalog_errors_redact_provider_text_and_urls() { let server = Server::start(true, |_, _| { diff --git a/crates/buzz-agent/tests/databricks_auth_coordinator.rs b/crates/buzz-agent/tests/databricks_auth_coordinator.rs index 0937cc87c1..82edb69a0a 100644 --- a/crates/buzz-agent/tests/databricks_auth_coordinator.rs +++ b/crates/buzz-agent/tests/databricks_auth_coordinator.rs @@ -146,35 +146,19 @@ enum RefreshMode { Reject, /// `500` — a provider-side fault, transient rather than a credential /// decision. - /// - /// Used only by Unix-only tests (refresh-error classification). - /// Gated to suppress dead-code warnings on Windows. - #[cfg(unix)] ServerError, /// A 4xx with the given OAuth `error` code in the body. Lets a test assert /// the coordinator treats `invalid_grant` (any 4xx) as a dead grant, but /// every other error code — and any non-`invalid_grant` status like `429` /// — as infrastructural rather than a credential rejection. - /// - /// Used only by Unix-only tests (refresh-error classification). - /// Gated to suppress dead-code warnings on Windows. - #[cfg(unix)] ClientError(axum::http::StatusCode, &'static str), /// Sleep `d` before answering, so the caller's per-request HTTP timeout /// elapses first (a transport timeout, not a verdict from the provider). - /// - /// Used only by Unix-only tests (refresh-timeout classification). - /// Gated to suppress dead-code warnings on Windows. - #[cfg(unix)] Hang(Duration), /// `200` returning the same fixed access token on every grant, regardless /// of how many are served. Models a provider that re-issues an identical /// access token, so a bounded rerun can hand back the exact bytes the /// caller already reported 401-rejected. - /// - /// Used only by Unix-only tests (rejected-token neutralization, sticky - /// reissuance). Gated to suppress dead-code warnings on Windows. - #[cfg(unix)] SucceedSticky(&'static str), } @@ -201,10 +185,6 @@ enum ExchangeMode { /// exchange. Models a provider that re-issues an identical access token, so /// a browser sign-in (reached after a dead refresh) can hand back the exact /// bytes the caller reported 401-rejected. - /// - /// Used only by Unix-only tests (sticky browser exchange after dead refresh). - /// Gated to suppress dead-code warnings on Windows. - #[cfg(unix)] SucceedSticky(&'static str), } @@ -274,7 +254,6 @@ async fn spawn_stub_with_modes(refresh: RefreshMode, exchange: ExchangeMode) -> // A hang delays the answer so the caller's per-request // HTTP timeout can elapse first (transport timeout, not // a credential decision). - #[cfg(unix)] if let RefreshMode::Hang(d) = refresh { tokio::time::sleep(d).await; } @@ -283,12 +262,10 @@ async fn spawn_stub_with_modes(refresh: RefreshMode, exchange: ExchangeMode) -> axum::http::StatusCode::UNAUTHORIZED, Json(json!({ "error": "invalid_grant" })), ), - #[cfg(unix)] RefreshMode::ServerError => ( axum::http::StatusCode::INTERNAL_SERVER_ERROR, Json(json!({ "error": "temporarily_unavailable" })), ), - #[cfg(unix)] RefreshMode::ClientError(status, error) => { (status, Json(json!({ "error": error }))) } @@ -300,7 +277,6 @@ async fn spawn_stub_with_modes(refresh: RefreshMode, exchange: ExchangeMode) -> "expires_in": 3600, })), ), - #[cfg(unix)] RefreshMode::Hang(_) => ( axum::http::StatusCode::OK, Json(json!({ @@ -309,7 +285,6 @@ async fn spawn_stub_with_modes(refresh: RefreshMode, exchange: ExchangeMode) -> "expires_in": 3600, })), ), - #[cfg(unix)] RefreshMode::SucceedSticky(tok) => ( axum::http::StatusCode::OK, Json(json!({ @@ -343,7 +318,6 @@ async fn spawn_stub_with_modes(refresh: RefreshMode, exchange: ExchangeMode) -> axum::http::StatusCode::OK, Json(json!({ "token_type": "bearer" })), ), - #[cfg(unix)] ExchangeMode::SucceedSticky(tok) => ( axum::http::StatusCode::OK, Json(json!({ @@ -383,7 +357,6 @@ async fn spawn_stub_with_modes(refresh: RefreshMode, exchange: ExchangeMode) -> /// deterministic ordering: the parent waits for `request_received` (proves A /// holds the lock and is mid-refresh), then spawns B, waits for B's snapshot /// marker, and finally calls `release()` before joining both workers. -#[cfg(unix)] struct RefreshGate { /// Notified by the stub once it has received the first refresh request. request_received: Arc, @@ -391,7 +364,6 @@ struct RefreshGate { proceed: Arc, } -#[cfg(unix)] impl RefreshGate { /// Asynchronously wait until the stub has received A's refresh request. async fn wait_for_request(&self) { @@ -408,7 +380,6 @@ impl RefreshGate { /// /// - `Sticky(tok)` — every refresh returns `200 OK` with `access_token: tok`. /// - `Reject` — every refresh returns `401 Unauthorized` with `invalid_grant`. -#[cfg(unix)] enum HeldRefreshResponse { Sticky(&'static str), Reject, @@ -422,7 +393,6 @@ enum HeldRefreshResponse { /// /// Returns the stub (for `refresh_grants` / `code_grants` assertions) and the /// control gate. Used by the cross-process held-refresh tests. -#[cfg(unix)] async fn spawn_stub_with_held_refresh(response: HeldRefreshResponse) -> (Stub, RefreshGate) { let code_grants = Arc::new(AtomicU64::new(0)); let refresh_grants = Arc::new(AtomicU64::new(0)); @@ -569,7 +539,6 @@ fn attempt_sidecar_path(cfg: &PkceOAuthConfig, cache_dir: &std::path::Path) -> s /// coordinator's `append_ext(cache_path, "lock")`. Used to point the /// out-of-process lock-holder helper at the exact file the coordinator /// contends on. -#[cfg(unix)] fn lock_file_path(cfg: &PkceOAuthConfig, cache_dir: &std::path::Path) -> std::path::PathBuf { let mut p = cache_file_path(cfg, cache_dir).into_os_string(); p.push(".lock"); @@ -780,7 +749,6 @@ async fn test_browser_open_failure_is_typed_and_retryable_by_user() { assert_eq!(approve_opener.call_count(), 1); } -#[cfg(unix)] #[tokio::test] async fn test_headless_dead_refresh_returns_refresh_rejected_without_browser() { let stub = spawn_stub(true).await; // refresh grants 401 @@ -816,7 +784,6 @@ async fn test_headless_dead_refresh_returns_refresh_rejected_without_browser() { ); } -#[cfg(unix)] #[tokio::test] async fn test_interactive_dead_refresh_converts_to_browser() { let stub = spawn_stub(true).await; // refresh grants 401 @@ -847,7 +814,6 @@ async fn test_interactive_dead_refresh_converts_to_browser() { assert_eq!(stub.code_grants.load(Ordering::SeqCst), 1); } -#[cfg(unix)] #[tokio::test] async fn test_headless_expired_token_live_refresh_recovers_silently() { let stub = spawn_stub(false).await; // refresh succeeds @@ -875,7 +841,6 @@ async fn test_headless_expired_token_live_refresh_recovers_silently() { assert_eq!(stub.refresh_grants.load(Ordering::SeqCst), 1); } -#[cfg(unix)] #[tokio::test] async fn test_interactive_login_reuses_valid_cache_without_browser() { let stub = spawn_stub(false).await; @@ -918,7 +883,6 @@ async fn test_interactive_login_reuses_valid_cache_without_browser() { /// Seed a not-yet-expired access token with a (dead) refresh token and return /// the access token so the caller can pass it as `rejected`. -#[cfg(unix)] fn seed_fresh_rejectable(cfg: &PkceOAuthConfig, cache_dir: &std::path::Path) -> String { let access = "fresh-but-rejected"; seed_cache( @@ -933,7 +897,6 @@ fn seed_fresh_rejectable(cfg: &PkceOAuthConfig, cache_dir: &std::path::Path) -> access.to_string() } -#[cfg(unix)] #[tokio::test] async fn test_auto_rejected_fresh_bearer_with_dead_refresh_launches_browser() { let stub = spawn_stub(true).await; // refresh grants 401 @@ -957,7 +920,6 @@ async fn test_auto_rejected_fresh_bearer_with_dead_refresh_launches_browser() { assert_eq!(stub.code_grants.load(Ordering::SeqCst), 1); } -#[cfg(unix)] #[tokio::test] async fn test_headless_rejected_fresh_bearer_with_dead_refresh_returns_refresh_rejected() { let stub = spawn_stub(true).await; // refresh grants 401 @@ -989,7 +951,6 @@ async fn test_headless_rejected_fresh_bearer_with_dead_refresh_returns_refresh_r // `RefreshRejected`, which would misreport a transient fault as a rotated // token and (for interactive intents) prompt a needless sign-in. -#[cfg(unix)] #[tokio::test] async fn test_refresh_timeout_is_network_unavailable_not_rejected() { // The token endpoint hangs far longer than the injected per-request HTTP @@ -1043,7 +1004,6 @@ async fn test_refresh_timeout_is_network_unavailable_not_rejected() { ); } -#[cfg(unix)] #[tokio::test] async fn test_refresh_server_error_is_network_unavailable_not_rejected() { let stub = spawn_stub_with(RefreshMode::ServerError).await; // refresh 500s @@ -1091,7 +1051,6 @@ async fn test_refresh_server_error_is_network_unavailable_not_rejected() { // and never pop a browser. The classifier keys on the OAuth error body, not // the bare status class. -#[cfg(unix)] #[tokio::test] async fn test_refresh_400_invalid_grant_is_dead_grant_not_network() { // A 400 (not just 401) carrying `invalid_grant` is still a dead refresh @@ -1127,7 +1086,6 @@ async fn test_refresh_400_invalid_grant_is_dead_grant_not_network() { assert_eq!(stub.refresh_grants.load(Ordering::SeqCst), 1); } -#[cfg(unix)] #[tokio::test] async fn test_refresh_non_invalid_grant_4xx_is_network_unavailable_not_rejected() { // Every 4xx whose OAuth body is NOT `invalid_grant` is a request/config or @@ -1300,7 +1258,6 @@ async fn test_userinitiated_joiner_does_not_inherit_auto_cooldown_result() { // dead. The joiner must instead detect the collision and run its own bounded // acquisition, obtaining a token that differs from its `rejected`. -#[cfg(unix)] #[tokio::test] async fn test_joiner_never_receives_its_own_rejected_token() { // Two concurrent `Headless` 401-recovery callers on one key, each rejecting @@ -1384,7 +1341,6 @@ async fn test_joiner_never_receives_its_own_rejected_token() { // this rerun terminate with a typed auth error before caching the rejected // token rather than returning it. -#[cfg(unix)] #[tokio::test] async fn test_joiner_rerun_reissuing_rejected_token_fails_typed_not_loop() { // A sticky provider returns ONE fixed access token on every refresh. Leader @@ -1456,7 +1412,6 @@ async fn test_joiner_rerun_reissuing_rejected_token_fails_typed_not_loop() { // Concrete scenario: A rejected X, refresh re-issues X → A gets RefreshRejected. // B rejected Y (different), refresh would yield X for B → B succeeds. -#[cfg(unix)] #[tokio::test] async fn test_joiner_with_different_rejected_does_not_inherit_leaders_rejection_failure() { // Sticky provider always returns "X" on every refresh grant. @@ -1525,16 +1480,10 @@ async fn test_joiner_with_different_rejected_does_not_inherit_leaders_rejection_ // token X rather than the just-acquired Y (memory won over disk). // - failure: B's matching rejected X remained live; its next `bearer()` still // served it. -// - no-persistence (Windows): B's state stayed empty; its next headless read -// returned `NoCredential` instead of Y and a second browser opened. // -// All three tests exercise the full `finish()` → `acquire_locked()` → +// Both tests exercise the full `finish()` → `acquire_locked()` → // `acquire_leader()` → `LeaderGuard::complete()` → joiner wiring. -// Unix-specific: the seed provides a live refresh token. The non-Unix constructor -// does not read the disk cache, so without a seed in memory A's headless path -// returns NoCredential rather than RefreshRejected. -#[cfg(unix)] #[tokio::test] async fn test_inprocess_joiner_reconciles_stale_state_after_shared_success() { // Scenario: A and B both loaded a locally-fresh-but-401'd token X. A leads, @@ -1618,8 +1567,6 @@ async fn test_inprocess_joiner_reconciles_stale_state_after_shared_success() { ); } -// Unix-specific: refresh token is required for a headless rejection path. -#[cfg(unix)] #[tokio::test] async fn test_inprocess_joiner_neutralizes_rejected_on_matching_shared_failure() { // Scenario: A and B both carry unexpired X as their rejected token. A leads, @@ -1686,90 +1633,6 @@ async fn test_inprocess_joiner_neutralizes_rejected_on_matching_shared_failure() ); } -// Non-Unix-specific: disk persistence is disabled on Windows, so the only way -// for B to retain Y after joining is in-memory state reconciliation. On Unix -// the disk can provide Y as a fallback, masking a reconciliation failure. -#[cfg(not(unix))] -#[tokio::test] -async fn test_inprocess_joiner_populates_empty_state_no_second_acquisition() { - // Scenario: A and B both start with empty state (no disk token on non-Unix). - // A leads, opens a browser, exchanges the code for Y. B joins A's slot and - // wakes to Ok(Y). Without reconciliation, B.state stays None. B's next - // headless acquire returns NoCredential instead of Y, and a second browser - // would open if UserInitiated. - // - // Mutation check (no state reconciliation): B.state stays None. The - // subsequent headless acquire on B returns Err(NoCredential) instead of - // Ok("browser-token-1") — the assertion FAILS. - let stub = spawn_stub(false).await; - let cache = TempDir::new().unwrap(); - let approve = ScriptedOpener::new(Script::Approve); - - let a = PkceOAuthTokenSource::new_with( - config(&stub, "/disco/a", cache.path()), - Arc::new(approve.clone()), - ) - .unwrap(); - let b = PkceOAuthTokenSource::new_with( - config(&stub, "/disco/a", cache.path()), - Arc::new(approve.clone()), - ) - .unwrap(); - - // Both start with empty state — UserInitiated falls through to a browser. - let (ra, rb) = tokio::join!( - a.acquire_with_intent(AuthIntent::UserInitiated, None), - b.acquire_with_intent(AuthIntent::UserInitiated, None), - ); - - assert_eq!( - ra, - Ok("browser-token-1".to_string()), - "leader (A) gets the browser token" - ); - assert_eq!( - rb, - Ok("browser-token-1".to_string()), - "joiner (B) shares the leader's browser token" - ); - assert_eq!( - approve.call_count(), - 1, - "exactly one browser opened — B joined rather than launching its own" - ); - assert_eq!( - stub.code_grants.load(Ordering::SeqCst), - 1, - "exactly one authorization-code exchange" - ); - - // B's subsequent headless acquire must return Y from in-memory state without - // a second browser. Without reconciliation, B.state is None and headless - // returns NoCredential (no disk fallback on non-Unix). - let rb_next = b - .acquire_with_intent(AuthIntent::Headless, None) - .await - .expect( - "B subsequent headless read must return Y from in-memory state, not NoCredential — \ - mutation check: fails if joiner state was not reconciled (bearer-only publication)", - ); - assert_eq!( - rb_next, "browser-token-1", - "B retains Y in memory for subsequent headless reads" - ); - // No second browser: B's subsequent read hit the in-memory cache. - assert_eq!( - approve.call_count(), - 1, - "no second browser opened — B's subsequent headless read hit the in-memory cache" - ); - assert_eq!( - stub.code_grants.load(Ordering::SeqCst), - 1, - "no second code exchange" - ); -} - // ---- a browser success that re-issues the rejected bytes must fail typed --- // // The 401-recovery invariant lives at `finish`'s persistence boundary, so it @@ -1781,7 +1644,6 @@ async fn test_inprocess_joiner_populates_empty_state_no_second_acquisition() { // bearer. A single interactive leader exercises the path; the colliding-joiner // rerun routes through the same boundary. -#[cfg(unix)] #[tokio::test] async fn test_interactive_browser_reissuing_rejected_token_fails_typed_not_loop() { // Refresh 401s (dead), so an interactive intent falls through to the @@ -1843,7 +1705,6 @@ async fn test_interactive_browser_reissuing_rejected_token_fails_typed_not_loop( // prove the cache is untouched after the typed failure, on both the refresh and // the browser re-issue paths. -#[cfg(unix)] #[tokio::test] async fn test_sticky_refresh_rejection_does_not_poison_cache_for_later_callers() { // A sticky provider re-issues `sticky-token` on every refresh. A caller that @@ -1901,7 +1762,6 @@ async fn test_sticky_refresh_rejection_does_not_poison_cache_for_later_callers() ); } -#[cfg(unix)] #[tokio::test] async fn test_sticky_browser_rejection_does_not_poison_cache_for_later_callers() { // Refresh is dead, so an interactive caller browses; the exchange stickily @@ -1976,7 +1836,6 @@ async fn test_sticky_browser_rejection_does_not_poison_cache_for_later_callers() // it rejected, so no future caller and no fresh process can serve it, while the // refresh token — not rejected, and the engine of recovery — stays intact. -#[cfg(unix)] #[tokio::test] async fn test_rejected_fresh_token_is_neutralized_for_a_fresh_process() { // The cached access token `A` is locally UNEXPIRED, and the provider @@ -2046,7 +1905,6 @@ async fn test_rejected_fresh_token_is_neutralized_for_a_fresh_process() { ); } -#[cfg(unix)] #[tokio::test] async fn test_rejected_fresh_token_is_neutralized_for_the_same_source() { // The in-memory layer of the same neutralization: after the SAME source @@ -2092,7 +1950,6 @@ async fn test_rejected_fresh_token_is_neutralized_for_the_same_source() { ); } -#[cfg(unix)] #[tokio::test] async fn test_rejected_fresh_token_neutralized_when_recovery_browses() { // The browser variant: `A` is unexpired but its refresh token is dead, so @@ -2282,7 +2139,6 @@ async fn test_rejected_token_disk_neutralization_neutralizes_in_place_when_paren ); } -#[cfg(unix)] #[tokio::test] async fn test_rejected_token_in_memory_neutralized_when_disk_neutralization_skipped() { // When `expire_rejected()` cannot read a matching disk entry (e.g. the cache @@ -2358,7 +2214,6 @@ async fn test_rejected_token_in_memory_neutralized_when_disk_neutralization_skip // must NOT be served as the replacement: doing so would skip the refresh the // 401 demanded and hand back a token the provider will also reject. -#[cfg(unix)] #[tokio::test] async fn test_rejected_recovery_skips_expired_sibling_and_refreshes() { let stub = spawn_stub(false).await; // refresh succeeds @@ -2540,7 +2395,6 @@ async fn test_exchange_timeout_is_network_unavailable_not_cooldown() { // crash mid-flow, and the kernel's release of the advisory lock is what lets // the coordinator's successor proceed with no PID files and no lock breaking. -#[cfg(unix)] #[tokio::test] async fn test_crossprocess_lock_holder_blocks_then_crash_release_lets_successor_proceed() { let stub = spawn_stub(false).await; // refresh succeeds once the lock is free @@ -2634,7 +2488,6 @@ struct Worker { #[derive(Deserialize)] struct WorkerOutcome { result: String, - #[cfg(unix)] bearer: Option, launches: u64, } @@ -2761,7 +2614,6 @@ async fn test_crossprocess_userinitiated_denial_shared_with_waiting_auto() { ); } -#[cfg(unix)] #[tokio::test] async fn test_crossprocess_two_coordinators_race_to_one_grant_and_cache() { // Two real coordinator processes race on one key from a cold cache. They @@ -2852,7 +2704,6 @@ async fn test_crossprocess_two_coordinators_race_to_one_grant_and_cache() { // second process detect that the predecessor completed while it was waiting // and adopt its failure directly. -#[cfg(unix)] #[tokio::test] async fn test_crossprocess_waiting_headless_adopts_predecessor_refresh_rejected() { // Two real headless processes on one key. The cache holds an expired @@ -3088,9 +2939,6 @@ async fn test_crossprocess_post_failure_userinitiated_runs_own_attempt() { // 6. C (UserInitiated, approve-scripted) starts fresh. C's snapshot == gen // on disk (1 with fix, 2 with mutation). In both cases C sees no advance // and runs its own browser flow. code_grants increments by 1 for C. -// -// This test is cache-free (no seed_cache / disk-token assertions) so it runs -// on Windows as well as Unix. #[tokio::test] async fn test_crossprocess_adopter_does_not_advance_generation() { @@ -3215,7 +3063,6 @@ async fn test_crossprocess_adopter_does_not_advance_generation() { // Mutation check (no digest gating): B adopts A's RefreshRejected → // refresh_grants stays at 1 → `refresh_grants == 2` assertion FAILS. -#[cfg(unix)] #[tokio::test] async fn test_crossprocess_waiter_with_different_rejected_does_not_adopt_leaders_failure() { // Stub stickily returns "X" but holds each response until released. @@ -3316,95 +3163,3 @@ async fn test_crossprocess_waiter_with_different_rejected_does_not_adopt_leaders "both workers run their own refresh — digest mismatch prevented adoption" ); } - -// ---- P1-3 non-Unix read path disabled ----------------------------------- -// -// On non-Unix platforms (Windows) token files written by older builds with -// default ACLs should not be consumed by new builds. `read_private_cache` -// returns an error on non-Unix (and opportunistically removes the legacy -// file), so `read_cache` yields `None` and the source behaves as if no -// cached token exists — memory-only cache on non-Unix. -// -// This test uses a cfg-gated stub: on Unix it only exercises the Unix read -// path (as a sanity check); the Windows behavior is proved by the -// `#[cfg(not(unix))]` branch of `read_private_cache` and verified by the -// Windows CI build + manual testing on the Windows runner. The test is written -// to compile on all platforms and asserts the platform-appropriate invariant. - -#[tokio::test] -async fn test_non_unix_does_not_serve_legacy_on_disk_token() { - // Seed a token that would be served from disk on Unix (unexpired, valid). - let stub = spawn_stub(false).await; // fresh token on refresh/browser - let cache = TempDir::new().unwrap(); - let opener = ScriptedOpener::new(Script::Approve); - let cfg = config(&stub, "/disco/a", cache.path()); - - seed_cache( - &cfg, - cache.path(), - json!({ - "access_token": "legacy-windows-token", - "refresh_token": "legacy-refresh", - "expires_at": future_secs(), - }), - ); - - let src = PkceOAuthTokenSource::new_with(cfg.clone(), Arc::new(opener.clone())).unwrap(); - - #[cfg(unix)] - { - // On Unix the cache is read and served directly from disk — this is the - // expected behavior on a secured platform. - let token = src - .acquire_with_intent(AuthIntent::Headless, None) - .await - .expect("Unix serves the seeded token from disk"); - assert_eq!(token, "legacy-windows-token", "Unix: disk token served"); - assert_eq!( - stub.refresh_grants.load(Ordering::SeqCst), - 0, - "Unix: no refresh — the disk token was served directly" - ); - // The seeded file is still on disk (not removed on Unix). - assert!( - cache_file_path(&cfg, cache.path()).exists(), - "Unix: the cache file is preserved" - ); - } - - #[cfg(not(unix))] - { - // On non-Unix `read_private_cache` refuses to read the legacy file and - // attempts to remove it. Construction and bearer() behave as if no cache - // exists — the source falls through to a browser flow. - let token = src - .acquire_with_intent(AuthIntent::Auto, None) - .await - .expect("non-Unix: browser flow succeeds (no disk token served)"); - assert_ne!( - token, "legacy-windows-token", - "non-Unix: legacy token must not be served from disk" - ); - assert_eq!( - stub.code_grants.load(Ordering::SeqCst), - 1, - "non-Unix: browser flow ran — disk token was not served" - ); - assert_eq!( - stub.refresh_grants.load(Ordering::SeqCst), - 0, - "non-Unix: no refresh grant — the source went straight to the browser flow" - ); - // The legacy file should have been removed by read_private_cache. - assert!( - !cache_file_path(&cfg, cache.path()).exists(), - "non-Unix: legacy cache file is removed by read_private_cache" - ); - // No new token file was written (persist is a no-op on non-Unix). - // (The token is held in memory only.) - assert!( - !cache_file_path(&cfg, cache.path()).exists(), - "non-Unix: no new cache file created (memory-only)" - ); - } -} diff --git a/crates/buzz-private-file/Cargo.toml b/crates/buzz-private-file/Cargo.toml new file mode 100644 index 0000000000..3d2ce5fcc4 --- /dev/null +++ b/crates/buzz-private-file/Cargo.toml @@ -0,0 +1,20 @@ +[package] +name = "buzz-private-file" +version.workspace = true +edition.workspace = true +rust-version.workspace = true +license.workspace = true +repository.workspace = true +description = "Owner-only files on Windows, keeping Win32 security FFI out of buzz-agent" + +[target.'cfg(windows)'.dependencies] +windows-sys = { version = "0.61", features = [ + "Win32_Foundation", + "Win32_Security", + "Win32_Security_Authorization", + "Win32_Storage_FileSystem", + "Win32_System_Threading", +] } + +[target.'cfg(windows)'.dev-dependencies] +tempfile = "3" diff --git a/crates/buzz-private-file/src/lib.rs b/crates/buzz-private-file/src/lib.rs new file mode 100644 index 0000000000..4370935317 --- /dev/null +++ b/crates/buzz-private-file/src/lib.rs @@ -0,0 +1,365 @@ +//! Owner-only files on Windows. +//! +//! `buzz-agent` forbids `unsafe`, but keeping its OAuth token cache private on +//! Windows takes Win32 security calls. They live here behind a safe API so that +//! crate keeps its guarantee. Unix gets the same property from file modes in +//! std, so this crate is empty there. +#![cfg(windows)] +#![deny(unsafe_code)] + +use std::ffi::c_void; +use std::fs::{File, OpenOptions}; +use std::io; +use std::os::windows::ffi::OsStrExt; +use std::os::windows::fs::OpenOptionsExt; +use std::os::windows::io::{AsRawHandle, FromRawHandle, OwnedHandle}; +use std::path::Path; +use std::ptr::{null, null_mut}; + +use windows_sys::Win32::Foundation::{ + LocalFree, ERROR_SUCCESS, GENERIC_WRITE, INVALID_HANDLE_VALUE, WIN32_ERROR, +}; +use windows_sys::Win32::Security::Authorization::{ + ConvertStringSecurityDescriptorToSecurityDescriptorW, GetSecurityInfo, SetSecurityInfo, + SDDL_REVISION_1, SE_FILE_OBJECT, +}; +use windows_sys::Win32::Security::{ + EqualSid, GetSecurityDescriptorDacl, GetTokenInformation, TokenOwner, + DACL_SECURITY_INFORMATION, OWNER_SECURITY_INFORMATION, PROTECTED_DACL_SECURITY_INFORMATION, + SECURITY_ATTRIBUTES, TOKEN_OWNER, TOKEN_QUERY, +}; +use windows_sys::Win32::Storage::FileSystem::{ + CreateFileW, CREATE_NEW, FILE_ATTRIBUTE_NORMAL, FILE_FLAG_OPEN_REPARSE_POINT, + FILE_GENERIC_READ, FILE_SHARE_DELETE, FILE_SHARE_READ, FILE_SHARE_WRITE, WRITE_DAC, +}; +use windows_sys::Win32::System::Threading::{GetCurrentProcess, OpenProcessToken}; + +/// Full access for the file's owner and nobody else; `P` blocks inherited ACEs. +const OWNER_ONLY: &str = "D:P(A;;FA;;;OW)"; + +/// Create `path` for writing, owner-only from the moment it exists. +/// +/// Fails if `path` exists. The DACL is set atomically at creation, so no other +/// user can open the file even briefly. Read, write and delete sharing match +/// std, so another process can still rename a new file over this path. +#[allow(unsafe_code)] +pub fn create_new(path: &Path) -> io::Result { + let descriptor = owner_only()?; + let path = verbatim(path)?; + let attributes = SECURITY_ATTRIBUTES { + nLength: size_of::() as u32, + lpSecurityDescriptor: descriptor.0, + bInheritHandle: 0, + }; + // SAFETY: `path` is NUL-terminated and `attributes` points at a descriptor + // that outlives the call. + let handle = unsafe { + CreateFileW( + path.as_ptr(), + GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, + &attributes, + CREATE_NEW, + FILE_ATTRIBUTE_NORMAL, + null_mut(), + ) + }; + if handle == INVALID_HANDLE_VALUE { + return Err(io::Error::last_os_error()); + } + // SAFETY: `handle` is a valid file handle that nothing else owns. + Ok(unsafe { File::from_raw_handle(handle) }) +} + +/// Open an existing owner-only file for reading. +/// +/// Does not follow a final symlink or junction, and refuses anything but a +/// regular file owned by this process's default owner, which is the owner +/// [`create_new`] assigns. It then re-applies the owner-only DACL on the open +/// handle, so a file with inherited permissions is tightened before use. +pub fn open_owned(path: &Path) -> io::Result { + let file = OpenOptions::new() + .access_mode(FILE_GENERIC_READ | WRITE_DAC) + .custom_flags(FILE_FLAG_OPEN_REPARSE_POINT) + .open(path)?; + if !file.metadata()?.is_file() { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + "not a regular file", + )); + } + if !owned_by_current_process(&file)? { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "file is not owned by the current user", + )); + } + restrict_to_owner(&file)?; + Ok(file) +} + +/// Memory Windows allocated with `LocalAlloc`, freed on drop. +struct Local(*mut c_void); + +impl Drop for Local { + #[allow(unsafe_code)] + fn drop(&mut self) { + // SAFETY: Windows allocated this pointer, and only this drop frees it. + unsafe { LocalFree(self.0) }; + } +} + +/// [`OWNER_ONLY`] as a security descriptor. +#[allow(unsafe_code)] +fn owner_only() -> io::Result { + let sddl: Vec = OWNER_ONLY.encode_utf16().chain(Some(0)).collect(); + let mut descriptor = null_mut(); + // SAFETY: `sddl` is NUL-terminated; on success Windows allocates `descriptor`. + let ok = unsafe { + ConvertStringSecurityDescriptorToSecurityDescriptorW( + sddl.as_ptr(), + SDDL_REVISION_1, + &mut descriptor, + null_mut(), + ) + }; + if ok == 0 { + return Err(io::Error::last_os_error()); + } + Ok(Local(descriptor)) +} + +/// Replace the DACL of a handle opened with `WRITE_DAC` by [`OWNER_ONLY`]. +#[allow(unsafe_code)] +fn restrict_to_owner(file: &File) -> io::Result<()> { + let descriptor = owner_only()?; + let (mut present, mut dacl, mut defaulted) = (0, null_mut(), 0); + // SAFETY: `descriptor` is valid, and `dacl` points into it. + if unsafe { GetSecurityDescriptorDacl(descriptor.0, &mut present, &mut dacl, &mut defaulted) } + == 0 + { + return Err(io::Error::last_os_error()); + } + // SAFETY: the handle is open, and `dacl` stays valid while `descriptor` lives. + win32(unsafe { + SetSecurityInfo( + file.as_raw_handle(), + SE_FILE_OBJECT, + DACL_SECURITY_INFORMATION | PROTECTED_DACL_SECURITY_INFORMATION, + null_mut(), + null_mut(), + dacl, + null(), + ) + }) +} + +/// Whether the file's owner is this process token's default owner. +#[allow(unsafe_code)] +fn owned_by_current_process(file: &File) -> io::Result { + let (mut owner, mut descriptor) = (null_mut(), null_mut()); + // SAFETY: the handle is open with `READ_CONTROL`; on success Windows + // allocates `descriptor` and points `owner` into it. + win32(unsafe { + GetSecurityInfo( + file.as_raw_handle(), + SE_FILE_OBJECT, + OWNER_SECURITY_INFORMATION, + &mut owner, + null_mut(), + null_mut(), + null_mut(), + &mut descriptor, + ) + })?; + let _descriptor = Local(descriptor); + + let mut token = null_mut(); + // SAFETY: `GetCurrentProcess` returns a pseudo-handle; on success `token` + // is a new handle. + if unsafe { OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &mut token) } == 0 { + return Err(io::Error::last_os_error()); + } + // SAFETY: `token` is a valid handle that nothing else owns. + let token = unsafe { OwnedHandle::from_raw_handle(token) }; + let mut len = 0; + // SAFETY: a null, zero-length buffer only queries the required size. + unsafe { GetTokenInformation(token.as_raw_handle(), TokenOwner, null_mut(), 0, &mut len) }; + // `usize` elements keep the buffer aligned for `TOKEN_OWNER`'s pointer. + let mut buffer = vec![0usize; (len as usize).div_ceil(size_of::())]; + // SAFETY: `buffer` is aligned and at least `len` bytes long. + let ok = unsafe { + GetTokenInformation( + token.as_raw_handle(), + TokenOwner, + buffer.as_mut_ptr().cast(), + len, + &mut len, + ) + }; + if ok == 0 { + return Err(io::Error::last_os_error()); + } + // SAFETY: Windows wrote a `TOKEN_OWNER` whose SID lives in `buffer`, and + // `owner` lives in `_descriptor`; both outlive the comparison. + Ok(unsafe { EqualSid(owner, (*buffer.as_ptr().cast::()).Owner) } != 0) +} + +fn win32(status: WIN32_ERROR) -> io::Result<()> { + if status == ERROR_SUCCESS { + Ok(()) + } else { + Err(io::Error::from_raw_os_error(status as i32)) + } +} + +/// `path` as a NUL-terminated wide string. Drive paths get the `\\?\` prefix +/// that std's own file APIs add, so long paths work here too. +fn verbatim(path: &Path) -> io::Result> { + let wide: Vec = std::path::absolute(path)? + .as_os_str() + .encode_wide() + .collect(); + if wide.contains(&0) { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "path contains NUL", + )); + } + let drive = matches!(wide.as_slice(), [_, colon, slash, ..] + if *colon == u16::from(b':') && *slash == u16::from(b'\\')); + let prefix = if drive { r"\\?\" } else { "" }; + Ok(prefix.encode_utf16().chain(wide).chain(Some(0)).collect()) +} + +#[cfg(test)] +mod tests { + use super::*; + use std::io::{Read, Write}; + use std::process::Command; + use windows_sys::Win32::Security::Authorization::ConvertSecurityDescriptorToStringSecurityDescriptorW; + + /// The file's DACL as SDDL, such as `D:P(A;;FA;;;OW)`. + #[allow(unsafe_code)] + fn dacl(file: &File) -> String { + let mut descriptor = null_mut(); + // SAFETY: the handle is open; on success Windows allocates `descriptor`. + let status = unsafe { + GetSecurityInfo( + file.as_raw_handle(), + SE_FILE_OBJECT, + DACL_SECURITY_INFORMATION, + null_mut(), + null_mut(), + null_mut(), + null_mut(), + &mut descriptor, + ) + }; + win32(status).unwrap(); + let descriptor = Local(descriptor); + let (mut sddl, mut len) = (null_mut(), 0); + // SAFETY: `descriptor` is valid; on success Windows allocates `sddl` + // holding `len` UTF-16 units. + let ok = unsafe { + ConvertSecurityDescriptorToStringSecurityDescriptorW( + descriptor.0, + SDDL_REVISION_1, + DACL_SECURITY_INFORMATION, + &mut sddl, + &mut len, + ) + }; + assert_ne!(ok, 0, "{}", io::Error::last_os_error()); + let _sddl = Local(sddl.cast()); + // SAFETY: `sddl` holds `len` UTF-16 units. + let units = unsafe { std::slice::from_raw_parts(sddl, len as usize) }; + String::from_utf16_lossy(units) + .trim_end_matches('\0') + .to_owned() + } + + fn assert_owner_only(file: &File) { + let sddl = dacl(file); + let (flags, aces) = sddl + .strip_prefix("D:") + .and_then(|rest| rest.split_once('(')) + .unwrap_or_else(|| panic!("unexpected DACL {sddl}")); + assert!(flags.contains('P'), "DACL is not protected: {sddl}"); + assert_eq!(aces, "A;;FA;;;OW)", "DACL is not owner-only: {sddl}"); + } + + fn read(mut file: File) -> String { + let mut body = String::new(); + file.read_to_string(&mut body).unwrap(); + body + } + + #[test] + fn create_new_is_owner_only_and_exclusive() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("token.json"); + let mut file = create_new(&path).unwrap(); + file.write_all(b"secret").unwrap(); + assert_owner_only(&file); + drop(file); + + let err = create_new(&path).unwrap_err(); + assert_eq!(err.kind(), io::ErrorKind::AlreadyExists); + assert_eq!(read(open_owned(&path).unwrap()), "secret"); + } + + #[test] + fn create_new_accepts_long_paths() { + let dir = tempfile::tempdir().unwrap(); + let parent = dir.path().join("a".repeat(120)).join("b".repeat(120)); + std::fs::create_dir_all(&parent).unwrap(); + let path = parent.join("token.json"); + create_new(&path).unwrap(); + assert_owner_only(&open_owned(&path).unwrap()); + } + + #[test] + fn open_owned_tightens_inherited_permissions() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("token.json"); + std::fs::write(&path, "legacy").unwrap(); + let file = open_owned(&path).unwrap(); + assert_owner_only(&file); + assert_eq!(read(file), "legacy"); + } + + fn icacls(path: &Path, args: &[&str]) { + let status = Command::new("icacls") + .arg(path) + .args(args) + .status() + .unwrap(); + assert!(status.success(), "icacls {args:?} failed; run elevated"); + } + + #[test] + #[ignore = "needs an elevated shell; CI runs it"] + fn open_owned_refuses_a_file_owned_by_someone_else() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("token.json"); + std::fs::write(&path, "planted").unwrap(); + // Everyone may read and rewrite it, but SYSTEM, not us, owns it. + icacls(&path, &["/grant", "*S-1-1-0:F"]); + icacls(&path, &["/setowner", "*S-1-5-18"]); + let err = open_owned(&path).unwrap_err(); + assert_eq!(err.kind(), io::ErrorKind::PermissionDenied); + } + + #[test] + #[ignore = "needs an elevated shell; CI runs it"] + fn open_owned_does_not_follow_symlinks() { + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("elsewhere.json"); + let link = dir.path().join("token.json"); + std::fs::write(&target, "elsewhere").unwrap(); + std::os::windows::fs::symlink_file(&target, &link).unwrap(); + let err = open_owned(&link).unwrap_err(); + assert_eq!(err.kind(), io::ErrorKind::InvalidData); + } +} diff --git a/desktop/src-tauri/Cargo.lock b/desktop/src-tauri/Cargo.lock index 728d636b93..1531939933 100644 --- a/desktop/src-tauri/Cargo.lock +++ b/desktop/src-tauri/Cargo.lock @@ -1076,6 +1076,7 @@ dependencies = [ "async-trait", "axum", "base64 0.22.1", + "buzz-private-file", "dirs", "fs2", "getrandom 0.4.3", @@ -1252,6 +1253,13 @@ dependencies = [ "thiserror 2.0.18", ] +[[package]] +name = "buzz-private-file" +version = "0.1.0" +dependencies = [ + "windows-sys 0.61.2", +] + [[package]] name = "buzz-sdk" version = "0.1.0"