From bbda08c691cf7ff9d55a45621a4a7239d851f8bc Mon Sep 17 00:00:00 2001 From: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz> Date: Wed, 30 Sep 2026 15:17:56 -0600 Subject: [PATCH] test(buzz-private-file): make the repair tests start from a loose DACL On Windows the load-repair tests relied on the temp directory's inherited ACL to make the file loose. Grant Everyone read and check that it took before loading, so skipping the repair fails both tests. Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz> --- crates/buzz-agent/src/auth.rs | 3 ++- crates/buzz-private-file/Cargo.toml | 2 +- crates/buzz-private-file/src/lib.rs | 38 +++++++++++++++++++++-------- 3 files changed, 31 insertions(+), 12 deletions(-) diff --git a/crates/buzz-agent/src/auth.rs b/crates/buzz-agent/src/auth.rs index f6825f822c..5a803a211e 100644 --- a/crates/buzz-agent/src/auth.rs +++ b/crates/buzz-agent/src/auth.rs @@ -2864,7 +2864,6 @@ mod tests { // 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(), @@ -2895,6 +2894,8 @@ mod tests { fs::set_permissions(&cache_path, fs::Permissions::from_mode(0o644)).unwrap(); fs::metadata(&cache_path).unwrap().ino() }; + #[cfg(windows)] + buzz_private_file::grant_everyone_read(&cache_path); // Constructing the source loads the cache — repair happens here. let source = PkceOAuthTokenSource::new(cfg).unwrap(); diff --git a/crates/buzz-private-file/Cargo.toml b/crates/buzz-private-file/Cargo.toml index d5ffb36869..4f8e145c53 100644 --- a/crates/buzz-private-file/Cargo.toml +++ b/crates/buzz-private-file/Cargo.toml @@ -8,7 +8,7 @@ repository.workspace = true description = "Owner-only files on Windows, keeping Win32 security FFI out of buzz-agent" [features] -# `assert_owner_only`, for other crates' tests. +# `assert_owner_only` and `grant_everyone_read`, for other crates' tests. test-support = [] [target.'cfg(windows)'.dependencies] diff --git a/crates/buzz-private-file/src/lib.rs b/crates/buzz-private-file/src/lib.rs index f604735803..0db761eff2 100644 --- a/crates/buzz-private-file/src/lib.rs +++ b/crates/buzz-private-file/src/lib.rs @@ -49,11 +49,11 @@ const OWNER_ONLY: &str = "D:P(A;;FA;;;OW)"; /// removes. #[allow(unsafe_code)] pub fn create_new(path: &Path) -> io::Result { - let descriptor = owner_only()?; + let owner_only = descriptor(OWNER_ONLY)?; let path = verbatim(path)?; let attributes = SECURITY_ATTRIBUTES { nLength: size_of::() as u32, - lpSecurityDescriptor: descriptor.0, + lpSecurityDescriptor: owner_only.0, bInheritHandle: 0, }; // SAFETY: `path` is NUL-terminated and `attributes` points at a descriptor @@ -102,7 +102,7 @@ pub fn open_owned(path: &Path) -> io::Result { "file is not owned by the current user", )); } - restrict_to_owner(&file)?; + set_dacl(&file, OWNER_ONLY)?; Ok(file) } @@ -117,10 +117,10 @@ impl Drop for Local { } } -/// [`OWNER_ONLY`] as a security descriptor. +/// An SDDL string as a security descriptor. #[allow(unsafe_code)] -fn owner_only() -> io::Result { - let sddl: Vec = OWNER_ONLY.encode_utf16().chain(Some(0)).collect(); +fn descriptor(sddl: &str) -> io::Result { + let sddl: Vec = sddl.encode_utf16().chain(Some(0)).collect(); let mut descriptor = null_mut(); // SAFETY: `sddl` is NUL-terminated; on success Windows allocates `descriptor`. let ok = unsafe { @@ -137,10 +137,11 @@ fn owner_only() -> io::Result { Ok(Local(descriptor)) } -/// Replace the DACL of a handle opened with `WRITE_DAC` by [`OWNER_ONLY`]. +/// Replace the DACL of a handle opened with `WRITE_DAC` by the protected DACL +/// in `sddl`. #[allow(unsafe_code)] -fn restrict_to_owner(file: &File) -> io::Result<()> { - let descriptor = owner_only()?; +fn set_dacl(file: &File, sddl: &str) -> io::Result<()> { + let descriptor = descriptor(sddl)?; 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) } @@ -332,6 +333,22 @@ pub fn assert_owner_only(file: &File, why: &str) { assert_eq!(aces, "A;;FA;;;OW)", "{why}: DACL is not owner-only: {sddl}"); } +/// Test support: also let Everyone read `path`, and check that it took, so a +/// test can show that [`open_owned`] repairs a loose file. +#[cfg(any(test, feature = "test-support"))] +#[track_caller] +pub fn grant_everyone_read(path: &Path) { + use windows_sys::Win32::Storage::FileSystem::READ_CONTROL; + + let file = OpenOptions::new() + .access_mode(READ_CONTROL | WRITE_DAC) + .open(path) + .unwrap(); + set_dacl(&file, "D:P(A;;FA;;;OW)(A;;FR;;;WD)").unwrap(); + let sddl = dacl(&file); + assert!(sddl.contains(";;;WD)"), "Everyone cannot read: {sddl}"); +} + #[cfg(test)] mod tests { use super::*; @@ -369,10 +386,11 @@ mod tests { } #[test] - fn open_owned_tightens_inherited_permissions() { + fn open_owned_tightens_loose_permissions() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("token.json"); std::fs::write(&path, "legacy").unwrap(); + grant_everyone_read(&path); let file = open_owned(&path).unwrap(); assert_owner_only(&file, "tightened file"); assert_eq!(read(file), "legacy");