From 8ad66b8f2115704eed56e59e2709c4eddf3cffac Mon Sep 17 00:00:00 2001 From: Lahfir <70215676+lahfir@users.noreply.github.com> Date: Sat, 25 Jul 2026 23:25:28 -0700 Subject: [PATCH] refactor!: remove speculative Win32 private-file layer from core, add real Windows/Linux test lanes (#106) --- .github/workflows/ci.yml | 65 ++++ CLAUDE.md | 17 +- Cargo.lock | 1 - Cargo.toml | 1 - crates/core/Cargo.toml | 10 - crates/core/src/adapter/system.rs | 8 +- crates/core/src/adapter/system_tests.rs | 11 + crates/core/src/commands/skills_tests.rs | 4 +- crates/core/src/private_file.rs | 128 +++----- crates/core/src/private_file_lock_tests.rs | 37 +++ crates/core/src/private_file_parent.rs | 60 ++-- crates/core/src/private_file_windows.rs | 299 ------------------ crates/core/src/private_file_windows_guard.rs | 84 ----- crates/core/src/private_file_windows_open.rs | 77 ----- crates/core/src/private_file_windows_path.rs | 247 --------------- .../core/src/private_file_windows_rename.rs | 114 ------- .../core/src/private_file_windows_security.rs | 241 -------------- crates/core/src/refs_lock.rs | 12 +- .../src/trace_read/html_screenshot_tests.rs | 11 +- crates/ffi/src/adapter.rs | 6 +- crates/ffi/tests/c_abi_passthrough.rs | 20 +- docs/phases.md | 20 +- ...ip-platform-code-that-ci-cannot-execute.md | 95 ++++++ rust-toolchain.toml | 1 + 24 files changed, 334 insertions(+), 1235 deletions(-) create mode 100644 crates/core/src/private_file_lock_tests.rs delete mode 100644 crates/core/src/private_file_windows.rs delete mode 100644 crates/core/src/private_file_windows_guard.rs delete mode 100644 crates/core/src/private_file_windows_open.rs delete mode 100644 crates/core/src/private_file_windows_path.rs delete mode 100644 crates/core/src/private_file_windows_rename.rs delete mode 100644 crates/core/src/private_file_windows_security.rs create mode 100644 docs/solutions/best-practices/never-ship-platform-code-that-ci-cannot-execute.md diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ab3af47..5af5db8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -195,6 +195,71 @@ jobs: - name: NPM wrapper smoke run: scripts/ci-npm-wrapper-smoke.sh + test-linux: + name: Test (Linux) + runs-on: ubuntu-latest + timeout-minutes: 20 + permissions: + contents: read + steps: + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + + - name: Install Rust toolchain + run: rustup show + + - name: Cache cargo registry + uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 + with: + path: | + ~/.cargo/registry/index/ + ~/.cargo/registry/cache/ + ~/.cargo/git/db/ + key: ${{ runner.os }}-cargo-${{ hashFiles('Cargo.lock', 'Cargo.toml', 'crates/**/Cargo.toml', 'src/Cargo.toml') }} + restore-keys: ${{ runner.os }}-cargo- + + - name: Core and Linux unit tests + run: scripts/cargo-test-isolated-home.sh test --locked -p agent-desktop-core -p agent-desktop-linux --lib + + test-windows: + name: Test (Windows) + runs-on: windows-latest + timeout-minutes: 20 + permissions: + contents: read + steps: + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + + - name: Install Rust toolchain + run: rustup show + + - name: Cache cargo registry + uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 + with: + path: | + ~/.cargo/registry/index/ + ~/.cargo/registry/cache/ + ~/.cargo/git/db/ + key: ${{ runner.os }}-cargo-${{ hashFiles('Cargo.lock', 'Cargo.toml', 'crates/**/Cargo.toml', 'src/Cargo.toml') }} + restore-keys: ${{ runner.os }}-cargo- + + # Core resolves its data dir via HOME then USERPROFILE (crates/core/src/refs.rs + # fn home_dir), so pointing HOME at a fresh runner-temp directory is enough to + # keep tests off the real profile — the bash isolation script the macOS/Linux + # lanes use doesn't apply here since this step runs under pwsh. CARGO_HOME and + # RUSTUP_HOME must be resolved from the real USERPROFILE first (rustup/cargo + # default them to %USERPROFILE%\.rustup and %USERPROFILE%\.cargo) so repointing + # USERPROFILE at the empty test home doesn't strand the installed toolchain. + - name: Core and Windows unit tests (isolated HOME) + shell: pwsh + run: | + $env:CARGO_HOME = if ($env:CARGO_HOME) { $env:CARGO_HOME } else { Join-Path $env:USERPROFILE '.cargo' } + $env:RUSTUP_HOME = if ($env:RUSTUP_HOME) { $env:RUSTUP_HOME } else { Join-Path $env:USERPROFILE '.rustup' } + $testHome = Join-Path $env:RUNNER_TEMP ("agent-desktop-test-home-" + [guid]::NewGuid()) + New-Item -ItemType Directory -Path $testHome | Out-Null + $env:HOME = $testHome + $env:USERPROFILE = $testHome + cargo test --locked -p agent-desktop-core -p agent-desktop-windows --lib + ffi-python-smoke: name: FFI Python Smoke runs-on: macos-latest diff --git a/CLAUDE.md b/CLAUDE.md index 4b79432..6193948 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -7,10 +7,14 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co ```bash cargo build # Debug build cargo build --release # Release build (<15MB target) -cargo test --lib --workspace # Run all unit tests +cargo test --workspace # Run every test (lib, binary, and integration targets) +cargo test --lib --workspace # Library unit tests only; SKIPS the agent-desktop binary crate +cargo test -p agent-desktop # Binary crate tests (CLI contract, dispatch, batch, policy) cargo test --lib -p agent-desktop-core # Test core crate only cargo test --lib -p agent-desktop-macos # Test macOS crate only cargo test test_name # Run a single test by name +cargo check -p agent-desktop-core --all-targets --target x86_64-pc-windows-msvc # Core must cross-compile +cargo check -p agent-desktop-core --all-targets --target x86_64-unknown-linux-gnu cargo clippy --all-targets -- -D warnings # Lint (must pass, zero warnings) cargo fmt --all -- --check # Format check cargo fmt --all # Auto-format @@ -375,11 +379,22 @@ for the actionability preflight (`get_live_*`), and `is_protected_process` ## CI Requirements - GitHub Actions macOS runner executes full test suite on every PR +- Windows and Linux runners execute the core unit tests plus their native platform crate on every PR - `cargo tree -p agent-desktop-core` must not contain platform crate names - `cargo clippy --all-targets -- -D warnings` - `cargo test --workspace` - Binary size check: fail if release binary exceeds 15MB +### Core platform-conditional code + +`agent-desktop-core` must stay executable on every supported OS. A `#[cfg]` branch in core +that no CI lane runs is a hypothesis, not shipped code — either add a lane that executes +it or keep it out of core. Platform hardening belongs behind `PlatformAdapter` or in the +platform crate, written on that platform against a lane that runs it. Core carried 1,062 +LOC of never-executed Win32 file I/O into v0.5.0; it failed 225 of 940 tests on first +contact with Windows and was deleted. See +`docs/solutions/best-practices/never-ship-platform-code-that-ci-cannot-execute.md`. + ## Commands 58 commands spanning App/Window, Observation, Interaction, Scroll, Keyboard, diff --git a/Cargo.lock b/Cargo.lock index 954edbd..782110b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -39,7 +39,6 @@ dependencies = [ "smallvec", "thiserror", "tracing", - "windows-sys", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index 7dabf7b..b8ec0b2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,7 +21,6 @@ rustc-hash = "2.1" libc = "0.2" smallvec = { version = "1.13", features = ["serde", "union"] } agent-desktop-core = { path = "crates/core" } -windows-sys = { version = "0.61.2" } [workspace.lints.rust] unsafe_op_in_unsafe_fn = "warn" diff --git a/crates/core/Cargo.toml b/crates/core/Cargo.toml index 4e3c101..053f2f0 100644 --- a/crates/core/Cargo.toml +++ b/crates/core/Cargo.toml @@ -15,15 +15,5 @@ base64.workspace = true libc.workspace = true smallvec.workspace = true -[target.'cfg(windows)'.dependencies] -windows-sys = { workspace = true, features = [ - "Win32_Foundation", - "Win32_Security", - "Win32_Security_Authorization", - "Win32_Storage_FileSystem", - "Win32_System_SystemServices", - "Win32_System_Threading", -] } - [lints] workspace = true diff --git a/crates/core/src/adapter/system.rs b/crates/core/src/adapter/system.rs index 5bbc96c..86e035d 100644 --- a/crates/core/src/adapter/system.rs +++ b/crates/core/src/adapter/system.rs @@ -15,11 +15,13 @@ pub trait SystemOps: Send + Sync { Err(AdapterError::not_supported("acquire_interaction_lease")) } + /// An adapter that never overrides this has not probed permissions at all, + /// which is not the same fact as a user denying one. `Unknown` routes through + /// `unknown_accessibility_means_unsupported` to `PLATFORM_NOT_SUPPORTED` + /// rather than the misleading `PERM_DENIED`. fn permission_report(&self, _deadline: Deadline) -> Result { Ok(PermissionReport { - accessibility: PermissionState::Denied { - suggestion: "Platform adapter not available".into(), - }, + accessibility: PermissionState::Unknown, screen_recording: PermissionState::Unknown, automation: PermissionState::NotRequired, }) diff --git a/crates/core/src/adapter/system_tests.rs b/crates/core/src/adapter/system_tests.rs index c48899b..f2840b1 100644 --- a/crates/core/src/adapter/system_tests.rs +++ b/crates/core/src/adapter/system_tests.rs @@ -18,6 +18,17 @@ fn default_surfaces_fail_closed() { assert!(DefaultOnly.supported_surfaces().is_empty()); } +#[test] +fn default_permission_report_is_unknown_not_denied() { + let report = DefaultOnly + .permission_report(crate::Deadline::standard().unwrap()) + .expect("default permission report must not error"); + + assert_eq!(report.accessibility, crate::PermissionState::Unknown); + assert!(!report.accessibility_denied()); + assert!(DefaultOnly.unknown_accessibility_means_unsupported()); +} + #[test] fn default_open_session_is_explicitly_unsupported() { let affinity = crate::SessionAffinity { diff --git a/crates/core/src/commands/skills_tests.rs b/crates/core/src/commands/skills_tests.rs index 0892da5..4943af5 100644 --- a/crates/core/src/commands/skills_tests.rs +++ b/crates/core/src/commands/skills_tests.rs @@ -30,7 +30,9 @@ fn get_full_inlines_references() { .expect("get full"); let content = v["content"].as_str().expect("string"); assert!(content.contains("--- references/workflows.md ---")); - assert!(content.contains("--- references/macos.md ---")); + if cfg!(target_os = "macos") { + assert!(content.contains("--- references/macos.md ---")); + } assert!(content.contains("@s8f3k2p9:e1")); assert!(content.contains("session start` does not activate later processes")); assert!(content.contains("session-owned ref still requires the same `--session`")); diff --git a/crates/core/src/private_file.rs b/crates/core/src/private_file.rs index 3b05389..248a182 100644 --- a/crates/core/src/private_file.rs +++ b/crates/core/src/private_file.rs @@ -1,5 +1,4 @@ use std::fs::File; -#[cfg(not(windows))] use std::fs::OpenOptions; use std::hash::{BuildHasher, RandomState}; use std::io::{Read, Write}; @@ -8,44 +7,29 @@ use std::sync::atomic::{AtomicU64, Ordering}; static TEMP_COUNTER: AtomicU64 = AtomicU64::new(0); -#[cfg(windows)] -#[path = "private_file_windows.rs"] -pub(crate) mod windows; - pub(crate) fn open_private_lock(path: &Path, create: bool) -> std::io::Result { - #[cfg(windows)] - { - windows::open_lock(path, create) - } - #[cfg(not(windows))] - { - let parent = path - .parent() - .ok_or_else(|| invalid_input("private file path has no parent"))?; - crate::private_file_parent::ensure_private(parent)?; - let mut options = OpenOptions::new(); - options.read(true).write(true).create(create); - configure_unix(&mut options, 0o600); - let file = options.open(path)?; - validate_private_regular(&file)?; - Ok(file) - } + let parent = path + .parent() + .ok_or_else(|| invalid_input("private file path has no parent"))?; + crate::private_file_parent::ensure_private(parent)?; + let mut options = OpenOptions::new(); + options.read(true).write(true).create(create); + configure_unix(&mut options, 0o600); + let file = options.open(path)?; + validate_private_regular(&file)?; + Ok(file) } +/// Read access is requested alongside append because callers lock the returned +/// handle, and Windows `LockFileEx` fails with `ERROR_ACCESS_DENIED` on a handle +/// opened for append only. pub(crate) fn open_private_append(path: &Path) -> std::io::Result { - #[cfg(windows)] - { - windows::open_append(path) - } - #[cfg(not(windows))] - { - let mut options = OpenOptions::new(); - options.create(true).append(true); - configure_unix(&mut options, 0o600); - let file = options.open(path)?; - validate_private_regular(&file)?; - Ok(file) - } + let mut options = OpenOptions::new(); + options.read(true).create(true).append(true); + configure_unix(&mut options, 0o600); + let file = options.open(path)?; + validate_private_regular(&file)?; + Ok(file) } pub(crate) fn read_private_bounded(path: &Path, max_bytes: u64) -> std::io::Result> { @@ -54,9 +38,6 @@ pub(crate) fn read_private_bounded(path: &Path, max_bytes: u64) -> std::io::Resu } pub(crate) fn read_regular_bounded(path: &Path, max_bytes: u64) -> std::io::Result> { - #[cfg(windows)] - let file = windows::open_regular_read(path)?; - #[cfg(not(windows))] let file = { let mut options = OpenOptions::new(); options.read(true); @@ -106,7 +87,7 @@ fn write_atomic_with( file.sync_all()?; #[cfg(test)] crash_before_rename_if_requested(path); - replace_atomic(&file, &temporary, path)?; + replace_atomic(&temporary, path)?; validate_private_regular(&open_private_read(path)?)?; sync_parent(parent) })(); @@ -158,8 +139,6 @@ pub(crate) fn validate_private_regular(file: &File) -> std::io::Result std::io::Result std::io::Result { - #[cfg(windows)] - { - windows::open_read(path) - } - #[cfg(not(windows))] - { - let mut options = OpenOptions::new(); - options.read(true); - configure_unix(&mut options, 0); - let file = options.open(path)?; - validate_private_regular(&file)?; - Ok(file) - } + let mut options = OpenOptions::new(); + options.read(true); + configure_unix(&mut options, 0); + let file = options.open(path)?; + validate_private_regular(&file)?; + Ok(file) } fn read_bounded(file: File, max_bytes: u64) -> std::io::Result> { @@ -216,26 +186,17 @@ fn create_temporary(path: &Path) -> std::io::Result<(PathBuf, File)> { std::time::SystemTime::now(), )); let temporary = path.with_file_name(format!(".{file_name}.{nonce:016x}.tmp")); - #[cfg(windows)] - match windows::create_new(&temporary) { - Ok(file) => return Ok((temporary, file)), + let mut options = OpenOptions::new(); + options.write(true).create_new(true); + configure_unix(&mut options, 0o600); + match options.open(&temporary) { + Ok(file) => { + validate_private_regular(&file)?; + return Ok((temporary, file)); + } Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => continue, Err(error) => return Err(error), } - #[cfg(not(windows))] - { - let mut options = OpenOptions::new(); - options.write(true).create_new(true); - configure_unix(&mut options, 0o600); - match options.open(&temporary) { - Ok(file) => { - validate_private_regular(&file)?; - return Ok((temporary, file)); - } - Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => continue, - Err(error) => return Err(error), - } - } } Err(std::io::Error::new( std::io::ErrorKind::AlreadyExists, @@ -270,13 +231,7 @@ fn sync_user_directory(path: &Path) -> std::io::Result<()> { OpenOptions::new().read(true).open(path)?.sync_all() } -#[cfg(windows)] -fn replace_atomic(file: &File, source: &Path, destination: &Path) -> std::io::Result<()> { - windows::replace_atomic(file, source, destination) -} - -#[cfg(not(windows))] -fn replace_atomic(_file: &File, source: &Path, destination: &Path) -> std::io::Result<()> { +fn replace_atomic(source: &Path, destination: &Path) -> std::io::Result<()> { std::fs::rename(source, destination) } @@ -288,7 +243,7 @@ fn configure_unix(options: &mut OpenOptions, mode: u32) { .custom_flags(libc::O_CLOEXEC | libc::O_NOFOLLOW | libc::O_NONBLOCK); } -#[cfg(not(any(unix, windows)))] +#[cfg(not(unix))] fn configure_unix(_options: &mut OpenOptions, _mode: u32) {} #[cfg(target_os = "macos")] @@ -322,12 +277,7 @@ fn validate_local_filesystem(file: &File) -> std::io::Result<()> { Ok(()) } -#[cfg(windows)] -fn validate_local_filesystem(file: &File) -> std::io::Result<()> { - windows::validate_local(file) -} - -#[cfg(not(any(target_os = "macos", target_os = "linux", windows)))] +#[cfg(not(any(target_os = "macos", target_os = "linux")))] fn validate_local_filesystem(_file: &File) -> std::io::Result<()> { Ok(()) } @@ -352,3 +302,7 @@ pub(super) fn permission_denied(message: &'static str) -> std::io::Error { #[cfg(test)] #[path = "private_file_tests.rs"] mod tests; + +#[cfg(test)] +#[path = "private_file_lock_tests.rs"] +mod lock_tests; diff --git a/crates/core/src/private_file_lock_tests.rs b/crates/core/src/private_file_lock_tests.rs new file mode 100644 index 0000000..d5e5d69 --- /dev/null +++ b/crates/core/src/private_file_lock_tests.rs @@ -0,0 +1,37 @@ +use super::*; + +fn scratch_dir(name: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!( + "agent-desktop-{name}-{}-{}", + std::process::id(), + crate::refs::new_snapshot_id() + )); + crate::private_file_parent::ensure_private(&dir).expect("private scratch directory"); + dir +} + +#[test] +fn appended_private_files_are_lockable() { + let path = scratch_dir("append-lock").join("trace.jsonl"); + + let file = open_private_append(&path).expect("append handle"); + file.lock() + .expect("append handles must be lockable on every platform"); + file.unlock().expect("unlock"); + + drop(file); + let _ = std::fs::remove_file(&path); +} + +#[test] +fn append_handles_reopen_and_relock_after_close() { + let path = scratch_dir("append-relock").join("trace.jsonl"); + + for _ in 0..2 { + let file = open_private_append(&path).expect("append handle"); + file.lock().expect("lock"); + file.unlock().expect("unlock"); + } + + let _ = std::fs::remove_file(&path); +} diff --git a/crates/core/src/private_file_parent.rs b/crates/core/src/private_file_parent.rs index 257cceb..1777bff 100644 --- a/crates/core/src/private_file_parent.rs +++ b/crates/core/src/private_file_parent.rs @@ -1,53 +1,39 @@ use std::path::Path; pub(super) fn ensure_private(path: &Path) -> std::io::Result<()> { - #[cfg(windows)] - { - super::private_file::windows::ensure_private_parent(path) + ensure_directory_path(path)?; + let metadata = std::fs::symlink_metadata(path)?; + if metadata.file_type().is_symlink() || !metadata.is_dir() { + return Err(super::private_file::permission_denied( + "private file parent must be a real directory", + )); } - #[cfg(not(windows))] + #[cfg(unix)] { - ensure_directory_path(path)?; - let metadata = std::fs::symlink_metadata(path)?; - if metadata.file_type().is_symlink() || !metadata.is_dir() { + use std::os::unix::fs::MetadataExt; + if metadata.uid() != unsafe { libc::geteuid() } { return Err(super::private_file::permission_denied( - "private file parent must be a real directory", + "private file parent is not owned by the effective user", )); } - #[cfg(unix)] - { - use std::os::unix::fs::MetadataExt; - if metadata.uid() != unsafe { libc::geteuid() } { - return Err(super::private_file::permission_denied( - "private file parent is not owned by the effective user", - )); - } - if metadata.mode() & 0o077 != 0 { - return Err(super::private_file::permission_denied( - "private file parent is accessible by group or other users", - )); - } + if metadata.mode() & 0o077 != 0 { + return Err(super::private_file::permission_denied( + "private file parent is accessible by group or other users", + )); } - Ok(()) } + Ok(()) } pub(super) fn ensure_user(path: &Path) -> std::io::Result<()> { - #[cfg(windows)] - { - super::private_file::windows::ensure_user_parent(path) - } - #[cfg(not(windows))] - { - ensure_directory_path(path)?; - let metadata = std::fs::metadata(path)?; - if !metadata.is_dir() { - return Err(super::private_file::permission_denied( - "user output parent must be a directory", - )); - } - Ok(()) + ensure_directory_path(path)?; + let metadata = std::fs::metadata(path)?; + if !metadata.is_dir() { + return Err(super::private_file::permission_denied( + "user output parent must be a directory", + )); } + Ok(()) } #[cfg(unix)] @@ -96,7 +82,7 @@ fn ensure_directory_path(path: &Path) -> std::io::Result<()> { Ok(()) } -#[cfg(not(any(unix, windows)))] +#[cfg(not(unix))] fn ensure_directory_path(path: &Path) -> std::io::Result<()> { std::fs::create_dir_all(path) } diff --git a/crates/core/src/private_file_windows.rs b/crates/core/src/private_file_windows.rs deleted file mode 100644 index 4f30433..0000000 --- a/crates/core/src/private_file_windows.rs +++ /dev/null @@ -1,299 +0,0 @@ -use std::fs::File; -use std::mem::size_of; -use std::os::windows::io::AsRawHandle; -use std::path::Path; -use std::ptr::null; -use windows_sys::Win32::Foundation::{ - ERROR_ALREADY_EXISTS, ERROR_INVALID_PARAMETER, GENERIC_READ, GENERIC_WRITE, GetLastError, - HANDLE, -}; -use windows_sys::Win32::Storage::FileSystem::{ - BY_HANDLE_FILE_INFORMATION, CREATE_NEW, CreateDirectoryW, DELETE, FILE_APPEND_DATA, - FILE_ATTRIBUTE_DIRECTORY, FILE_ATTRIBUTE_REPARSE_POINT, FILE_REMOTE_PROTOCOL_INFO, - FILE_SHARE_READ, FILE_SHARE_WRITE, FileRemoteProtocolInfo, GetFileInformationByHandle, - GetFileInformationByHandleEx, OPEN_ALWAYS, OPEN_EXISTING, READ_CONTROL, -}; - -#[path = "private_file_windows_guard.rs"] -mod guard; -#[path = "private_file_windows_open.rs"] -mod open; -#[path = "private_file_windows_path.rs"] -mod path; -#[path = "private_file_windows_rename.rs"] -mod rename; -#[path = "private_file_windows_security.rs"] -mod security; - -use open::FileOpen; -use security::PrivateSecurity; - -const LEAF_SHARING: u32 = FILE_SHARE_READ | FILE_SHARE_WRITE; -const GUARD_SHARING: u32 = FILE_SHARE_READ; -const TEMP_ACCESS: u32 = GENERIC_WRITE | READ_CONTROL | DELETE; - -pub(super) fn open_lock(path: &Path, create: bool) -> std::io::Result { - let creation = if create { OPEN_ALWAYS } else { OPEN_EXISTING }; - open_private( - path, - GENERIC_READ | GENERIC_WRITE | READ_CONTROL, - creation, - true, - ) -} - -pub(super) fn open_append(path: &Path) -> std::io::Result { - open_private(path, FILE_APPEND_DATA | READ_CONTROL, OPEN_ALWAYS, false) -} - -pub(super) fn open_read(path: &Path) -> std::io::Result { - open_private(path, GENERIC_READ | READ_CONTROL, OPEN_EXISTING, false) -} - -pub(super) fn open_regular_read(path: &Path) -> std::io::Result { - let path = path::normalized(path)?; - path::validate_file_name(&path)?; - with_leaf_parent(&path, false, None, || { - FileOpen::leaf(&path, GENERIC_READ, OPEN_EXISTING, null()).execute() - }) -} - -pub(super) fn create_new(path: &Path) -> std::io::Result { - open_private(path, TEMP_ACCESS, CREATE_NEW, false) -} - -pub(crate) fn ensure_private_parent(path: &Path) -> std::io::Result<()> { - let path = path::normalized(path)?; - let security = PrivateSecurity::new_directory()?; - guard::with_ancestor_guards(&path, true, Some(&security), |guards| { - security::validate_private_acl(guards.leaf()?) - }) -} - -pub(crate) fn ensure_user_parent(path: &Path) -> std::io::Result<()> { - let path = path::normalized(path)?; - let security = PrivateSecurity::new_directory()?; - guard::with_ancestor_guards(&path, true, Some(&security), |_| Ok(())) -} - -pub(super) fn validate_private(file: &File) -> std::io::Result<()> { - let info = file_information(file)?; - if info.dwFileAttributes & (FILE_ATTRIBUTE_DIRECTORY | FILE_ATTRIBUTE_REPARSE_POINT) != 0 { - return Err(super::permission_denied( - "private path must be a regular non-reparse file", - )); - } - if info.nNumberOfLinks != 1 { - return Err(super::permission_denied( - "private file must not be hard-linked", - )); - } - validate_local(file)?; - security::validate_private_acl(file) -} - -pub(super) fn validate_regular(file: &File) -> std::io::Result<()> { - let info = file_information(file)?; - if info.dwFileAttributes & (FILE_ATTRIBUTE_DIRECTORY | FILE_ATTRIBUTE_REPARSE_POINT) != 0 { - return Err(super::permission_denied( - "path must be a regular non-reparse file", - )); - } - validate_local(file) -} - -pub(super) fn validate_local(file: &File) -> std::io::Result<()> { - let mut remote = remote_protocol_query(); - let success = unsafe { - GetFileInformationByHandleEx( - raw_handle(file), - FileRemoteProtocolInfo, - (&raw mut remote).cast(), - size_of::() as u32, - ) - }; - let error = if success == 0 { - unsafe { GetLastError() } - } else { - 0 - }; - classify_locality(success != 0, error) -} - -fn classify_locality(success: bool, error: u32) -> std::io::Result<()> { - if success { - return Err(super::permission_denied( - "network filesystems are not accepted here", - )); - } - if error == ERROR_INVALID_PARAMETER { - Ok(()) - } else { - Err(std::io::Error::new( - std::io::ErrorKind::Unsupported, - "cannot verify that the Windows storage is local", - )) - } -} - -pub(super) fn replace_atomic( - source_file: &File, - source: &Path, - destination: &Path, -) -> std::io::Result<()> { - let source = path::normalized(source)?; - let destination = path::normalized(destination)?; - path::validate_file_name(&source)?; - path::validate_file_name(&destination)?; - let source_parent = parent(&source)?; - let destination_parent = parent(&destination)?; - if source_parent == destination_parent { - return guard::with_ancestor_guards(source_parent, false, None, |_guards| { - rename::replace(source_file, &destination) - }); - } - guard::with_ancestor_guards(source_parent, false, None, |_source_guards| { - guard::with_ancestor_guards(destination_parent, false, None, |_destination_guards| { - rename::replace(source_file, &destination) - }) - }) -} - -fn open_private( - path: &Path, - access: u32, - creation: u32, - create_parent: bool, -) -> std::io::Result { - let path = path::normalized(path)?; - path::validate_file_name(&path)?; - let (directory_security, file_security) = if create_parent { - let (directory, file) = PrivateSecurity::new_pair()?; - (Some(directory), file) - } else { - (None, PrivateSecurity::new_file()?) - }; - with_leaf_parent(&path, create_parent, directory_security.as_ref(), || { - let file = FileOpen::leaf(&path, access, creation, file_security.attributes()).execute()?; - validate_private(&file)?; - Ok(file) - }) -} - -fn with_leaf_parent( - path: &Path, - create_parent: bool, - security: Option<&PrivateSecurity>, - operation: impl FnOnce() -> std::io::Result, -) -> std::io::Result { - guard::with_ancestor_guards(parent(path)?, create_parent, security, |_guards| { - operation() - }) -} - -fn create_private_directory(path: &Path, security: &PrivateSecurity) -> std::io::Result<()> { - let path = path::wide_normalized(path)?; - if unsafe { CreateDirectoryW(path.as_ptr(), security.attributes()) } != 0 { - return Ok(()); - } - let code = unsafe { GetLastError() }; - if code == ERROR_ALREADY_EXISTS { - Ok(()) - } else { - Err(std::io::Error::from_raw_os_error(code as i32)) - } -} - -fn open_guarded_directory(path: &Path) -> std::io::Result { - FileOpen::guarded_directory(path, GENERIC_READ | READ_CONTROL, OPEN_EXISTING).execute() -} - -fn validate_directory(file: &File) -> std::io::Result<()> { - let info = file_information(file)?; - if info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY == 0 - || info.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT != 0 - { - return Err(super::permission_denied( - "private file parent must be a real non-reparse directory", - )); - } - validate_local(file) -} - -fn file_information(file: &File) -> std::io::Result { - let mut info = BY_HANDLE_FILE_INFORMATION::default(); - if unsafe { GetFileInformationByHandle(raw_handle(file), &raw mut info) } == 0 { - Err(std::io::Error::last_os_error()) - } else { - Ok(info) - } -} - -fn raw_handle(file: &File) -> HANDLE { - file.as_raw_handle() -} - -fn parent(path: &Path) -> std::io::Result<&Path> { - path.parent() - .filter(|parent| !parent.as_os_str().is_empty()) - .ok_or_else(|| invalid_input("private file path has no parent")) -} - -fn remote_protocol_query() -> FILE_REMOTE_PROTOCOL_INFO { - FILE_REMOTE_PROTOCOL_INFO { - StructureVersion: 2, - StructureSize: size_of::() as u16, - ..Default::default() - } -} - -fn invalid_input(message: &'static str) -> std::io::Error { - std::io::Error::new(std::io::ErrorKind::InvalidInput, message) -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn ancestor_guards_exclude_delete_sharing() { - assert_ne!( - windows_sys::Win32::Storage::FileSystem::FILE_FLAG_OPEN_REPARSE_POINT, - 0 - ); - assert_eq!(LEAF_SHARING, FILE_SHARE_READ | FILE_SHARE_WRITE); - assert_eq!( - LEAF_SHARING & windows_sys::Win32::Storage::FileSystem::FILE_SHARE_DELETE, - 0 - ); - assert_eq!(GUARD_SHARING, FILE_SHARE_READ); - assert_eq!(GUARD_SHARING & FILE_SHARE_WRITE, 0); - assert_eq!( - GUARD_SHARING & windows_sys::Win32::Storage::FileSystem::FILE_SHARE_DELETE, - 0 - ); - assert_ne!(TEMP_ACCESS & DELETE, 0); - } - - #[test] - fn remote_protocol_query_uses_the_current_contract_shape() { - let query = remote_protocol_query(); - - assert_eq!(query.StructureVersion, 2); - assert_eq!( - usize::from(query.StructureSize), - size_of::() - ); - assert_eq!(query.Protocol, 0); - assert_eq!(ERROR_INVALID_PARAMETER, 87); - assert!(classify_locality(false, ERROR_INVALID_PARAMETER).is_ok()); - assert_eq!( - classify_locality(true, 0).unwrap_err().kind(), - std::io::ErrorKind::PermissionDenied - ); - assert_eq!( - classify_locality(false, 5).unwrap_err().kind(), - std::io::ErrorKind::Unsupported - ); - } -} diff --git a/crates/core/src/private_file_windows_guard.rs b/crates/core/src/private_file_windows_guard.rs deleted file mode 100644 index a1f6743..0000000 --- a/crates/core/src/private_file_windows_guard.rs +++ /dev/null @@ -1,84 +0,0 @@ -use std::fs::File; -use std::path::{Component, Path, PathBuf}; - -use super::security::PrivateSecurity; - -pub(super) struct AncestorGuards { - handles: Vec, -} - -impl AncestorGuards { - fn acquire( - path: &Path, - create: bool, - security: Option<&PrivateSecurity>, - ) -> std::io::Result { - let absolute = super::path::normalized(path)?; - let mut current = PathBuf::new(); - let mut handles = Vec::new(); - for component in absolute.components() { - current.push(component); - if matches!(component, Component::Prefix(_)) { - continue; - } - let handle = match super::open_guarded_directory(¤t) { - Ok(handle) => handle, - Err(error) if create && error.kind() == std::io::ErrorKind::NotFound => { - let security = security.ok_or_else(|| { - std::io::Error::new( - std::io::ErrorKind::Unsupported, - "secure Windows directory creation requires a private descriptor", - ) - })?; - super::create_private_directory(¤t, security)?; - super::open_guarded_directory(¤t)? - } - Err(error) => return Err(error), - }; - super::validate_directory(&handle)?; - handles.push(handle); - } - if handles.is_empty() { - return Err(std::io::Error::new( - std::io::ErrorKind::PermissionDenied, - "Windows private path has no guardable directory", - )); - } - Ok(Self { handles }) - } - - pub(super) fn leaf(&self) -> std::io::Result<&File> { - self.handles.last().ok_or_else(|| { - std::io::Error::new( - std::io::ErrorKind::PermissionDenied, - "Windows private path lost its ancestor guards", - ) - }) - } -} - -pub(super) fn with_ancestor_guards( - path: &Path, - create: bool, - security: Option<&PrivateSecurity>, - operation: impl FnOnce(&AncestorGuards) -> std::io::Result, -) -> std::io::Result { - let guards = AncestorGuards::acquire(path, create, security)?; - operation(&guards) -} - -#[cfg(test)] -mod tests { - use super::*; - - fn lifetime_contract(guards: &AncestorGuards) -> std::io::Result<&File> { - guards.leaf() - } - - #[test] - fn operation_borrows_the_live_guard_chain() { - let contract: for<'a> fn(&'a AncestorGuards) -> std::io::Result<&'a File> = - lifetime_contract; - let _ = contract; - } -} diff --git a/crates/core/src/private_file_windows_open.rs b/crates/core/src/private_file_windows_open.rs deleted file mode 100644 index a10de35..0000000 --- a/crates/core/src/private_file_windows_open.rs +++ /dev/null @@ -1,77 +0,0 @@ -use std::fs::File; -use std::os::windows::io::FromRawHandle; -use std::path::Path; -use std::ptr::null_mut; -use windows_sys::Win32::Foundation::{HANDLE, INVALID_HANDLE_VALUE}; -use windows_sys::Win32::Security::SECURITY_ATTRIBUTES; -use windows_sys::Win32::Storage::FileSystem::{ - CreateFileW, FILE_ATTRIBUTE_NORMAL, FILE_FLAG_BACKUP_SEMANTICS, FILE_FLAG_OPEN_REPARSE_POINT, -}; - -pub(super) struct FileOpen<'a> { - path: &'a Path, - access: u32, - creation: u32, - directory: bool, - sharing: u32, - security: *const SECURITY_ATTRIBUTES, -} - -impl<'a> FileOpen<'a> { - pub(super) fn leaf( - path: &'a Path, - access: u32, - creation: u32, - security: *const SECURITY_ATTRIBUTES, - ) -> Self { - Self { - path, - access, - creation, - directory: false, - sharing: super::LEAF_SHARING, - security, - } - } - - pub(super) fn guarded_directory(path: &'a Path, access: u32, creation: u32) -> Self { - Self { - path, - access, - creation, - directory: true, - sharing: super::GUARD_SHARING, - security: std::ptr::null(), - } - } - - pub(super) fn execute(self) -> std::io::Result { - let path = super::path::wide_normalized(self.path)?; - let flags = FILE_FLAG_OPEN_REPARSE_POINT - | if self.directory { - FILE_FLAG_BACKUP_SEMANTICS - } else { - FILE_ATTRIBUTE_NORMAL - }; - let handle = unsafe { - CreateFileW( - path.as_ptr(), - self.access, - self.sharing, - self.security, - self.creation, - flags, - null_mut(), - ) - }; - file_from_handle(handle) - } -} - -fn file_from_handle(handle: HANDLE) -> std::io::Result { - if handle == INVALID_HANDLE_VALUE { - Err(std::io::Error::last_os_error()) - } else { - Ok(unsafe { File::from_raw_handle(handle) }) - } -} diff --git a/crates/core/src/private_file_windows_path.rs b/crates/core/src/private_file_windows_path.rs deleted file mode 100644 index 41fb758..0000000 --- a/crates/core/src/private_file_windows_path.rs +++ /dev/null @@ -1,247 +0,0 @@ -use std::os::windows::ffi::OsStrExt; -use std::path::{Component, Path, PathBuf, Prefix}; - -const VERBATIM: &[u16] = &[b'\\' as u16, b'\\' as u16, b'?' as u16, b'\\' as u16]; -const VERBATIM_UNC: &[u16] = &[ - b'\\' as u16, - b'\\' as u16, - b'?' as u16, - b'\\' as u16, - b'U' as u16, - b'N' as u16, - b'C' as u16, - b'\\' as u16, -]; - -pub(super) fn normalized(path: &Path) -> std::io::Result { - validate_path_nul(path)?; - validate_components(path)?; - let absolute = if path.is_absolute() { - path.to_path_buf() - } else { - if path.has_root() || matches!(path.components().next(), Some(Component::Prefix(_))) { - return Err(invalid_input( - "drive-relative and root-relative Windows paths are not accepted here", - )); - } - std::env::current_dir()?.join(path) - }; - let mut normalized = PathBuf::new(); - for component in absolute.components() { - match component { - Component::CurDir => {} - Component::ParentDir => { - return Err(invalid_input( - "Windows private paths must not contain parent traversal", - )); - } - _ => normalized.push(component), - } - } - if !normalized.is_absolute() { - return Err(invalid_input( - "Windows path did not normalize to an absolute path", - )); - } - validate_components(&normalized)?; - Ok(normalized) -} - -pub(super) fn wide_normalized(path: &Path) -> std::io::Result> { - validate_path_nul(path)?; - validate_components(path)?; - let absolute: Vec = path.as_os_str().encode_wide().collect(); - let mut verbatim = if absolute.starts_with(VERBATIM) { - absolute - } else if absolute.starts_with(&[b'\\' as u16, b'\\' as u16]) { - let mut path = Vec::with_capacity(VERBATIM_UNC.len() + absolute.len() - 2 + 1); - path.extend_from_slice(VERBATIM_UNC); - path.extend_from_slice(&absolute[2..]); - path - } else if is_drive_absolute(&absolute) { - let mut path = Vec::with_capacity(VERBATIM.len() + absolute.len() + 1); - path.extend_from_slice(VERBATIM); - path.extend_from_slice(&absolute); - path - } else { - return Err(invalid_input( - "Windows path did not normalize to an absolute path", - )); - }; - if verbatim.len() + 1 > 32_767 { - return Err(invalid_input("Windows verbatim path exceeds 32,767 units")); - } - verbatim.push(0); - Ok(verbatim) -} - -pub(super) fn validate_file_name(path: &Path) -> std::io::Result<()> { - let file_name = path - .file_name() - .ok_or_else(|| invalid_input("private path has no filename"))?; - validate_component(file_name) -} - -fn validate_components(path: &Path) -> std::io::Result<()> { - for component in path.components() { - match component { - Component::Prefix(prefix) => validate_prefix(prefix.kind())?, - Component::Normal(component) => validate_component(component)?, - Component::ParentDir => { - return Err(invalid_input( - "Windows private paths must not contain parent traversal", - )); - } - Component::RootDir | Component::CurDir => {} - } - } - Ok(()) -} - -fn validate_prefix(prefix: Prefix<'_>) -> std::io::Result<()> { - match prefix { - Prefix::Disk(_) - | Prefix::UNC(_, _) - | Prefix::VerbatimDisk(_) - | Prefix::VerbatimUNC(_, _) => Ok(()), - Prefix::DeviceNS(_) | Prefix::Verbatim(_) => Err(invalid_input( - "Windows device namespace paths are not accepted here", - )), - } -} - -fn validate_component(component: &std::ffi::OsStr) -> std::io::Result<()> { - let wide: Vec = component.encode_wide().collect(); - if wide.contains(&0) { - return Err(invalid_input("Windows paths must not contain NUL")); - } - if wide.contains(&(b':' as u16)) { - return Err(invalid_input( - "Windows alternate data streams are not accepted here", - )); - } - if matches!(wide.last(), Some(last) if *last == b'.' as u16 || *last == b' ' as u16) { - return Err(invalid_input( - "Windows private path components must not end in a dot or space", - )); - } - if is_dos_reserved(&wide) { - return Err(invalid_input( - "Windows DOS device basenames are not accepted here", - )); - } - Ok(()) -} - -fn is_dos_reserved(component: &[u16]) -> bool { - let base_end = component - .iter() - .position(|unit| *unit == b'.' as u16) - .unwrap_or(component.len()); - let mut base = &component[..base_end]; - while matches!(base.last(), Some(last) if *last == b' ' as u16 || *last == b'.' as u16) { - base = &base[..base.len() - 1]; - } - ascii_eq(base, b"NUL") - || ascii_eq(base, b"CON") - || ascii_eq(base, b"AUX") - || ascii_eq(base, b"PRN") - || ascii_eq(base, b"CLOCK$") - || numbered_device(base, b"COM") - || numbered_device(base, b"LPT") -} - -fn numbered_device(base: &[u16], prefix: &[u8; 3]) -> bool { - base.len() == 4 - && ascii_eq(&base[..3], prefix) - && matches!(base[3], unit if (b'1' as u16..=b'9' as u16).contains(&unit) || matches!(unit, 0x00b9 | 0x00b2 | 0x00b3)) -} - -fn ascii_eq(value: &[u16], expected: &[u8]) -> bool { - value.len() == expected.len() - && value.iter().zip(expected).all(|(actual, expected)| { - u8::try_from(*actual).is_ok_and(|actual| actual.to_ascii_uppercase() == *expected) - }) -} - -fn validate_path_nul(path: &Path) -> std::io::Result<()> { - if path.as_os_str().encode_wide().any(|unit| unit == 0) { - return Err(invalid_input("Windows paths must not contain NUL")); - } - Ok(()) -} - -fn is_drive_absolute(path: &[u16]) -> bool { - matches!(path, [drive, colon, slash, ..] if *drive != b'\\' as u16 && *colon == b':' as u16 && *slash == b'\\' as u16) -} - -fn invalid_input(message: &'static str) -> std::io::Error { - std::io::Error::new(std::io::ErrorKind::InvalidInput, message) -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn file_names_reject_windows_aliasing_syntax() { - assert!(validate_file_name(Path::new("state:stream")).is_err()); - assert!(validate_file_name(Path::new("state.")).is_err()); - assert!(validate_file_name(Path::new("state ")).is_err()); - assert!(validate_file_name(Path::new("state.json")).is_ok()); - } - - #[test] - fn dos_device_names_are_rejected_case_insensitively_with_extensions() { - for name in [ - "NUL", - "con.txt", - "AUX.json", - "prn", - "CLOCK$", - "COM1.log", - "com9", - "LPT1.txt", - "lpt9", - "COM¹.txt", - "LPT²", - "lpt³.json", - ] { - assert!(validate_file_name(Path::new(name)).is_err(), "{name}"); - } - for name in ["COM0", "COM10", "LPT0", "LPT10", "console", "auxiliary"] { - assert!(validate_file_name(Path::new(name)).is_ok(), "{name}"); - } - } - - #[test] - fn drive_absolute_detection_requires_a_root_separator() { - assert!(is_drive_absolute(&[ - b'C' as u16, - b':' as u16, - b'\\' as u16, - b'x' as u16 - ])); - assert!(!is_drive_absolute(&[b'C' as u16, b':' as u16, b'x' as u16])); - } - - #[test] - fn drive_and_unc_paths_receive_verbatim_prefixes() { - let drive = wide_normalized(Path::new(r"C:\state\refs.json")).unwrap(); - let unc = wide_normalized(Path::new(r"\\server\share\refs.json")).unwrap(); - - assert!(drive.starts_with(VERBATIM)); - assert!(unc.starts_with(VERBATIM_UNC)); - assert_eq!(drive.last(), Some(&0)); - assert_eq!(unc.last(), Some(&0)); - } - - #[test] - fn long_drive_path_is_encoded_without_legacy_max_path_truncation() { - let path = format!(r"C:\state\{}\refs.json", "a".repeat(300)); - let encoded = wide_normalized(Path::new(&path)).unwrap(); - - assert!(encoded.starts_with(VERBATIM)); - assert!(encoded.len() > 260); - } -} diff --git a/crates/core/src/private_file_windows_rename.rs b/crates/core/src/private_file_windows_rename.rs deleted file mode 100644 index 3d8769b..0000000 --- a/crates/core/src/private_file_windows_rename.rs +++ /dev/null @@ -1,114 +0,0 @@ -use std::fs::File; -use std::mem::{offset_of, size_of}; -use std::os::windows::io::AsRawHandle; -use std::path::Path; -use std::ptr::null_mut; -use windows_sys::Win32::Storage::FileSystem::{ - FILE_RENAME_INFO, FileRenameInfo, SetFileInformationByHandle, -}; - -pub(super) struct RenameBuffer { - words: Vec, - byte_len: u32, -} - -impl RenameBuffer { - fn new(destination: &Path) -> std::io::Result { - let name = super::path::wide_normalized(destination)?; - if name.last() != Some(&0) { - return Err(std::io::Error::new( - std::io::ErrorKind::InvalidInput, - "Windows rename target is not NUL-terminated", - )); - } - let name_units = name.len().checked_sub(1).ok_or_else(|| { - std::io::Error::new( - std::io::ErrorKind::InvalidInput, - "Windows rename target is not NUL-terminated", - ) - })?; - let name_bytes = name_units.checked_mul(size_of::()).ok_or_else(|| { - std::io::Error::new( - std::io::ErrorKind::InvalidInput, - "Windows rename target is too large", - ) - })?; - let stored_name_bytes = name.len().checked_mul(size_of::()).ok_or_else(|| { - std::io::Error::new( - std::io::ErrorKind::InvalidInput, - "Windows rename target is too large", - ) - })?; - let byte_len = offset_of!(FILE_RENAME_INFO, FileName) - .checked_add(stored_name_bytes) - .ok_or_else(|| { - std::io::Error::new( - std::io::ErrorKind::InvalidInput, - "Windows rename buffer is too large", - ) - })?; - let mut words = vec![0usize; byte_len.div_ceil(size_of::())]; - let info = words.as_mut_ptr().cast::(); - unsafe { - (*info).Anonymous.ReplaceIfExists = true; - (*info).RootDirectory = null_mut(); - (*info).FileNameLength = name_bytes as u32; - std::ptr::copy_nonoverlapping( - name.as_ptr(), - (&raw mut (*info).FileName).cast::(), - name.len(), - ); - } - Ok(Self { - words, - byte_len: byte_len as u32, - }) - } - - fn as_ptr(&self) -> *const std::ffi::c_void { - self.words.as_ptr().cast() - } -} - -pub(super) fn replace(source: &File, destination: &Path) -> std::io::Result<()> { - let rename = RenameBuffer::new(destination)?; - if unsafe { - SetFileInformationByHandle( - source.as_raw_handle(), - FileRenameInfo, - rename.as_ptr(), - rename.byte_len, - ) - } == 0 - { - return Err(std::io::Error::last_os_error()); - } - source.sync_all() -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn replacement_contract_requires_the_open_source_descriptor() { - let contract: fn(&File, &Path) -> std::io::Result<()> = replace; - let _ = contract; - } - - #[test] - fn rename_layout_declares_the_terminal_nul_for_one_unit_basename() { - let rename = RenameBuffer::new(Path::new(r"C:\x")).unwrap(); - let info = rename.as_ptr().cast::(); - let file_name_bytes = unsafe { (*info).FileNameLength } as usize; - let nul_index = file_name_bytes / size_of::(); - let nul = unsafe { *(&raw const (*info).FileName).cast::().add(nul_index) }; - - assert!(rename.byte_len as usize >= size_of::()); - assert!( - offset_of!(FILE_RENAME_INFO, FileName) + file_name_bytes + size_of::() - <= rename.byte_len as usize - ); - assert_eq!(nul, 0); - } -} diff --git a/crates/core/src/private_file_windows_security.rs b/crates/core/src/private_file_windows_security.rs deleted file mode 100644 index 4f1e1c7..0000000 --- a/crates/core/src/private_file_windows_security.rs +++ /dev/null @@ -1,241 +0,0 @@ -use std::ffi::c_void; -use std::fs::File; -use std::mem::{size_of, zeroed}; -use std::os::windows::io::AsRawHandle; -use std::ptr::null_mut; -use std::sync::Arc; -use windows_sys::Win32::Foundation::{CloseHandle, LocalFree}; -use windows_sys::Win32::Security::Authorization::{GetSecurityInfo, SE_FILE_OBJECT}; -use windows_sys::Win32::Security::{ - ACCESS_ALLOWED_ACE, ACL, ACL_REVISION, ACL_SIZE_INFORMATION, AclSizeInformation, - AddAccessAllowedAceEx, CONTAINER_INHERIT_ACE, DACL_SECURITY_INFORMATION, EqualSid, GetAce, - GetAclInformation, GetLengthSid, GetSecurityDescriptorControl, GetTokenInformation, - InitializeAcl, InitializeSecurityDescriptor, OBJECT_INHERIT_ACE, OWNER_SECURITY_INFORMATION, - PSID, SE_DACL_PROTECTED, SECURITY_ATTRIBUTES, SECURITY_DESCRIPTOR, - SetSecurityDescriptorControl, SetSecurityDescriptorDacl, SetSecurityDescriptorOwner, - TOKEN_QUERY, TOKEN_USER, TokenUser, -}; -use windows_sys::Win32::Storage::FileSystem::FILE_ALL_ACCESS; -use windows_sys::Win32::System::SystemServices::{ - ACCESS_ALLOWED_ACE_TYPE, SECURITY_DESCRIPTOR_REVISION, -}; -use windows_sys::Win32::System::Threading::{GetCurrentProcess, OpenProcessToken}; - -pub(super) fn validate_private_acl(file: &File) -> std::io::Result<()> { - let (current_sid, _) = current_user_sid()?; - let mut owner = null_mut(); - let mut dacl = null_mut(); - let mut descriptor = null_mut(); - let status = unsafe { - GetSecurityInfo( - file.as_raw_handle(), - SE_FILE_OBJECT, - OWNER_SECURITY_INFORMATION | DACL_SECURITY_INFORMATION, - &raw mut owner, - null_mut(), - &raw mut dacl, - null_mut(), - &raw mut descriptor, - ) - }; - if status != 0 { - return Err(std::io::Error::from_raw_os_error(status as i32)); - } - let current_sid_ptr = current_sid.as_ptr().cast_mut().cast(); - let result = if owner.is_null() || unsafe { EqualSid(owner, current_sid_ptr) } == 0 { - Err(permission_denied( - "private path is not owned by the current user", - )) - } else { - validate_owner_only_dacl(descriptor, dacl, current_sid_ptr) - }; - unsafe { LocalFree(descriptor) }; - result -} - -fn validate_owner_only_dacl( - descriptor: *mut c_void, - dacl: *mut ACL, - current_sid: PSID, -) -> std::io::Result<()> { - let mut control = 0; - let mut revision = 0; - if dacl.is_null() - || unsafe { GetSecurityDescriptorControl(descriptor, &raw mut control, &raw mut revision) } - == 0 - || control & SE_DACL_PROTECTED == 0 - { - return Err(permission_denied( - "private path must have a protected owner-only DACL", - )); - } - let mut acl_info = ACL_SIZE_INFORMATION::default(); - if unsafe { - GetAclInformation( - dacl, - (&raw mut acl_info).cast(), - size_of::() as u32, - AclSizeInformation, - ) - } == 0 - || acl_info.AceCount != 1 - { - return Err(permission_denied( - "private path DACL must contain exactly one owner entry", - )); - } - let mut raw_ace = null_mut(); - if unsafe { GetAce(dacl, 0, &raw mut raw_ace) } == 0 || raw_ace.is_null() { - return Err(std::io::Error::last_os_error()); - } - let ace = unsafe { &*raw_ace.cast::() }; - let ace_sid = (&raw const ace.SidStart).cast_mut().cast(); - if ace.Header.AceType != ACCESS_ALLOWED_ACE_TYPE as u8 - || ace.Mask & FILE_ALL_ACCESS != FILE_ALL_ACCESS - || unsafe { EqualSid(ace_sid, current_sid) } == 0 - { - return Err(permission_denied( - "private path grants access outside the current user", - )); - } - Ok(()) -} - -pub(super) struct PrivateSecurity { - _sid: Arc>, - _acl: Vec, - _descriptor: Box, - attributes: SECURITY_ATTRIBUTES, -} - -impl PrivateSecurity { - pub(super) fn new_directory() -> std::io::Result { - let (sid, sid_bytes) = current_user_sid()?; - Self::new_with_sid(Arc::new(sid), sid_bytes, true) - } - - pub(super) fn new_file() -> std::io::Result { - let (sid, sid_bytes) = current_user_sid()?; - Self::new_with_sid(Arc::new(sid), sid_bytes, false) - } - - pub(super) fn new_pair() -> std::io::Result<(Self, Self)> { - let (sid, sid_bytes) = current_user_sid()?; - let sid = Arc::new(sid); - let directory = Self::new_with_sid(Arc::clone(&sid), sid_bytes, true)?; - let file = Self::new_with_sid(sid, sid_bytes, false)?; - Ok((directory, file)) - } - - fn new_with_sid( - sid: Arc>, - sid_bytes: usize, - directory: bool, - ) -> std::io::Result { - let sid_ptr = sid.as_ref().as_ptr().cast_mut().cast(); - let acl_bytes = - size_of::() + size_of::() - size_of::() + sid_bytes; - let mut acl = aligned_words(acl_bytes); - let acl_ptr = acl.as_mut_ptr().cast::(); - if unsafe { InitializeAcl(acl_ptr, acl_bytes as u32, ACL_REVISION) } == 0 { - return Err(std::io::Error::last_os_error()); - } - let inheritance = if directory { - CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE - } else { - 0 - }; - if unsafe { - AddAccessAllowedAceEx(acl_ptr, ACL_REVISION, inheritance, FILE_ALL_ACCESS, sid_ptr) - } == 0 - { - return Err(std::io::Error::last_os_error()); - } - let mut descriptor = Box::new(unsafe { zeroed::() }); - let descriptor_ptr = (&raw mut *descriptor).cast(); - if unsafe { InitializeSecurityDescriptor(descriptor_ptr, SECURITY_DESCRIPTOR_REVISION) } - == 0 - || unsafe { SetSecurityDescriptorOwner(descriptor_ptr, sid_ptr, 0) } == 0 - || unsafe { SetSecurityDescriptorDacl(descriptor_ptr, 1, acl_ptr, 0) } == 0 - || unsafe { - SetSecurityDescriptorControl(descriptor_ptr, SE_DACL_PROTECTED, SE_DACL_PROTECTED) - } == 0 - { - return Err(std::io::Error::last_os_error()); - } - let attributes = SECURITY_ATTRIBUTES { - nLength: size_of::() as u32, - lpSecurityDescriptor: descriptor_ptr, - bInheritHandle: 0, - }; - Ok(Self { - _sid: sid, - _acl: acl, - _descriptor: descriptor, - attributes, - }) - } - - pub(super) fn attributes(&self) -> *const SECURITY_ATTRIBUTES { - &raw const self.attributes - } -} - -fn current_user_sid() -> std::io::Result<(Vec, usize)> { - let mut token = null_mut(); - if unsafe { OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &raw mut token) } == 0 { - return Err(std::io::Error::last_os_error()); - } - let result = (|| { - let mut bytes = 0; - unsafe { GetTokenInformation(token, TokenUser, null_mut(), 0, &raw mut bytes) }; - if bytes == 0 { - return Err(std::io::Error::last_os_error()); - } - let mut buffer = aligned_words(bytes as usize); - if unsafe { - GetTokenInformation( - token, - TokenUser, - buffer.as_mut_ptr().cast(), - bytes, - &raw mut bytes, - ) - } == 0 - { - return Err(std::io::Error::last_os_error()); - } - let sid = unsafe { (*buffer.as_ptr().cast::()).User.Sid }; - let sid_bytes = unsafe { GetLengthSid(sid) } as usize; - if sid_bytes == 0 { - return Err(std::io::Error::last_os_error()); - } - let mut owned = aligned_words(sid_bytes); - unsafe { - std::ptr::copy_nonoverlapping(sid.cast::(), owned.as_mut_ptr().cast(), sid_bytes) - }; - Ok((owned, sid_bytes)) - })(); - unsafe { CloseHandle(token) }; - result -} - -fn aligned_words(byte_len: usize) -> Vec { - vec![0; byte_len.div_ceil(size_of::())] -} - -fn permission_denied(message: &'static str) -> std::io::Error { - std::io::Error::new(std::io::ErrorKind::PermissionDenied, message) -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn private_acl_is_protected_and_owner_only_by_construction() { - assert_ne!(SE_DACL_PROTECTED, 0); - assert_ne!(FILE_ALL_ACCESS, 0); - assert_eq!(ACL_REVISION, 2); - } -} diff --git a/crates/core/src/refs_lock.rs b/crates/core/src/refs_lock.rs index f5a90f9..d68bc56 100644 --- a/crates/core/src/refs_lock.rs +++ b/crates/core/src/refs_lock.rs @@ -32,11 +32,13 @@ mod tests { use super::*; fn lock_path(name: &str) -> std::path::PathBuf { - std::env::temp_dir().join(format!( - "agent-desktop-{name}-{}-{}.lock", - std::process::id(), - crate::refs::new_snapshot_id() - )) + std::env::temp_dir() + .join(format!( + "agent-desktop-{name}-{}-{}", + std::process::id(), + crate::refs::new_snapshot_id() + )) + .join("store.lock") } #[test] diff --git a/crates/core/src/trace_read/html_screenshot_tests.rs b/crates/core/src/trace_read/html_screenshot_tests.rs index 1b0815b..344e442 100644 --- a/crates/core/src/trace_read/html_screenshot_tests.rs +++ b/crates/core/src/trace_read/html_screenshot_tests.rs @@ -90,10 +90,13 @@ fn absolute_screenshot_path_is_not_embedded() { write_segment( &trace_dir, "100-1000.jsonl", - &[&format!( - r#"{{"event":"action.artifacts","screenshot_pre":"{}","ts_ms":1,"seq":1}}"#, - sentinel.display() - )], + &[&serde_json::json!({ + "event": "action.artifacts", + "screenshot_pre": sentinel.to_string_lossy(), + "ts_ms": 1, + "seq": 1, + }) + .to_string()], ); let (html, stats) = export_html( &trace_dir, diff --git a/crates/ffi/src/adapter.rs b/crates/ffi/src/adapter.rs index 8f5c183..8c7aef8 100644 --- a/crates/ffi/src/adapter.rs +++ b/crates/ffi/src/adapter.rs @@ -81,9 +81,9 @@ pub(crate) use acquire_adapter; /// /// Every method delegates to the `PlatformAdapter` trait defaults, all of /// which return `not_supported()` errors. `permission_report()` returns -/// `Denied` via the trait default, so `ad_check_permissions` yields -/// `ErrPermDenied` on a stub build — the expected signal for a CI runner -/// that has no OS accessibility permission. +/// `Unknown` via the trait default, so `ad_check_permissions` yields +/// `ErrPlatformNotSupported` on a stub build — the honest signal that the +/// adapter never probed permissions, rather than that one was denied. #[cfg(feature = "stub-adapter")] struct StubAdapter; diff --git a/crates/ffi/tests/c_abi_passthrough.rs b/crates/ffi/tests/c_abi_passthrough.rs index 361dea5..f4b31b5 100644 --- a/crates/ffi/tests/c_abi_passthrough.rs +++ b/crates/ffi/tests/c_abi_passthrough.rs @@ -93,27 +93,27 @@ fn stub_ad_version_always_succeeds() { } } -/// `ad_check_permissions` maps the stub adapter's `Denied` permission state -/// to `ErrPermDenied (-1)`, not `ErrPlatformNotSupported (-8)`. This is the -/// documented exception. Cross-platform callers must treat both codes as -/// "adapter not operational here". +/// `ad_check_permissions` reports `ErrPlatformNotSupported (-8)` on an adapter +/// that never probes permissions. Not having measured a permission is a +/// different fact from a user having denied one, and `ErrPermDenied (-1)` sends +/// callers looking for a grant that does not exist on that platform. #[cfg(feature = "stub-adapter")] #[test] -fn stub_ad_check_permissions_returns_err_perm_denied() { +fn stub_ad_check_permissions_returns_err_platform_not_supported() { with_adapter(|adapter| unsafe { let rc = ad_check_permissions(adapter); assert_eq!( rc, - AdResult::ErrPermDenied, - "stub adapter permission_report() returns Denied → ErrPermDenied (-1), \ - not ErrPlatformNotSupported (-8). Both mean the adapter is not operational." + AdResult::ErrPlatformNotSupported, + "an adapter that does not probe permissions reports Unknown, which must \ + surface as ErrPlatformNotSupported (-8), never ErrPermDenied (-1)." ); let msg = ad_last_error_message(); assert!( !msg.is_null(), - "last-error message must be set on ErrPermDenied" + "last-error message must be set on ErrPlatformNotSupported" ); - assert_eq!(ad_last_error_code(), AdResult::ErrPermDenied); + assert_eq!(ad_last_error_code(), AdResult::ErrPlatformNotSupported); }); } diff --git a/docs/phases.md b/docs/phases.md index fe48942..ee30cf4 100644 --- a/docs/phases.md +++ b/docs/phases.md @@ -165,7 +165,7 @@ agent-desktop/ │ │ ├── snapshot_surface.rs # SnapshotSurface (predeclared cross-platform surface vocabulary) │ │ ├── snapshot.rs # SnapshotEngine (filter, allocate, serialize) │ │ ├── session/ # session start/end/list/gc, manifest, liveness -│ │ ├── private_file*.rs # 0600-equivalent private artifact writing (Unix perms + Windows ACL family) +│ │ ├── private_file*.rs # 0600-equivalent private artifact writing (portable std::fs on every target; Windows ACL hardening not yet built, see Phase 2.1) │ │ ├── trace.rs / trace_read/ # JSONL reliability trace, segment merge, HTML viewer │ │ ├── error_code.rs # ErrorCode enum │ │ ├── adapter_error.rs / app_error.rs # AdapterError, AppError @@ -952,13 +952,13 @@ Every sub-phase 2.0–2.15 below is held to the same definition of done, stated **Goal:** empirically map Windows accessibility reality with raw, no-Rust scripts before any adapter code exists, producing a committed evidence corpus the Rust sub-phases implement against — and feeding every contradiction back into this document. -**Scope:** a `probes/windows/` directory of raw scripts — PowerShell using .NET managed UIA (`System.Windows.Automation`, preinstalled with .NET Framework 4.8) plus small C# programs compiled with `csc.exe` where UIA3 COM specifics differ from the managed wrapper. The corpus must cover, each as a runnable script with captured JSON/text output committed beside it: (1) full-tree dumps of Notepad, Explorer, Settings, and one Electron app (VS Code or Slack) including every property read per node; (2) a pattern-availability census per ControlType (Invoke, Toggle, Value, RangeValue, ExpandCollapse, SelectionItem, Scroll, ScrollItem, Text, Window, LegacyIAccessible); (3) every interaction exercised raw — invoke, toggle, set value, select, expand/collapse, scroll via pattern AND wheel, text get/selection/caret/insert, focus; (4) SendInput synthesis experiments — keyboard incl. modifier chords and UTF-16 chunking limits, mouse click/move/wheel/drag; (5) `ElementFromPoint` hit-testing incl. deliberately occluded and zero-size targets; (6) `CacheRequest` batched reads timed against per-property reads; (7) AutomationId coverage census across Win32 / WinForms / WPF / Electron; (8) event-handler observations (which UIA events actually fire, ordering, MTA threading behavior); (9) elevation/UIPI behavior against an elevated process; (10) RDP-session and DPI/multi-monitor bounds behavior. Alongside the scripts: `probes/windows/FINDINGS.md` — a findings ledger mapping every experiment to observed behavior and a doc-alignment verdict (confirms this document / contradicts it / new edge case). +**Scope:** a `probes/windows/` directory of raw scripts — PowerShell using .NET managed UIA (`System.Windows.Automation`, preinstalled with .NET Framework 4.8) plus small C# programs compiled with `csc.exe` where UIA3 COM specifics differ from the managed wrapper. The corpus must cover, each as a runnable script with captured JSON/text output committed beside it: (1) full-tree dumps of Notepad, Explorer, Settings, and one Electron app (VS Code or Slack) including every property read per node; (2) a pattern-availability census per ControlType (Invoke, Toggle, Value, RangeValue, ExpandCollapse, SelectionItem, Scroll, ScrollItem, Text, Window, LegacyIAccessible); (3) every interaction exercised raw — invoke, toggle, set value, select, expand/collapse, scroll via pattern AND wheel, text get/selection/caret/insert, focus; (4) SendInput synthesis experiments — keyboard incl. modifier chords and UTF-16 chunking limits, mouse click/move/wheel/drag; (5) `ElementFromPoint` hit-testing incl. deliberately occluded and zero-size targets; (6) `CacheRequest` batched reads timed against per-property reads; (7) AutomationId coverage census across Win32 / WinForms / WPF / Electron; (8) event-handler observations (which UIA events actually fire, ordering, MTA threading behavior); (9) elevation/UIPI behavior against an elevated process; (10) RDP-session and DPI/multi-monitor bounds behavior; (11) private-file I/O primitives — whether atomic rename over a concurrently-open handle requires `FILE_SHARE_DELETE`, whether an elevated process owns new objects as `TokenOwner` (e.g. `BUILTIN\Administrators`) rather than `TokenUser`, whether `GetFileInformationByHandleEx(FileRemoteProtocolInfo)` reliably distinguishes local from remote volumes, and what ancestor-vs-leaf ACL validation contract parity with the unix leaf-only rule actually needs (see `docs/solutions/best-practices/never-ship-platform-code-that-ci-cannot-execute.md`). Alongside the scripts: `probes/windows/FINDINGS.md` — a findings ledger mapping every experiment to observed behavior and a doc-alignment verdict (confirms this document / contradicts it / new edge case). **Key APIs:** System.Windows.Automation, UIA3 COM (IUIAutomation) via csc.exe shims, SendInput, ElementFromPoint, CacheRequest. **Depends on:** nothing — this is the entry point; the dev VM needs only its preinstalled .NET, PowerShell, and git. -**Exit criteria:** the script corpus and captured outputs are committed and re-runnable on the dev VM; the findings ledger covers tree, patterns, interactions, input, hit-testing, batching, identity, events, elevation, and session behavior with no open "unknown" rows; every ledger entry that contradicts this document has a matching amendment to this document landed in the same PR (see the source-of-truth feedback rule in the Platform Delivery Model); no Rust adapter sub-phase (2.2 onward) starts until this exit gate is green. +**Exit criteria:** the script corpus and captured outputs are committed and re-runnable on the dev VM; the findings ledger covers tree, patterns, interactions, input, hit-testing, batching, identity, events, elevation, session, and private-file I/O behavior with no open "unknown" rows; every ledger entry that contradicts this document has a matching amendment to this document landed in the same PR (see the source-of-truth feedback rule in the Platform Delivery Model); no Rust adapter sub-phase (2.2 onward) starts until this exit gate is green. **Est. PR size:** ~1.5k lines (scripts + ledger; no Rust). @@ -972,15 +972,15 @@ Every sub-phase 2.0–2.15 below is held to the same definition of done, stated - `CoInitializeEx(NULL, COINIT_MULTITHREADED)` + `SetProcessDpiAwarenessContext(DPI_AWARENESS_CONTEXT_PER_MONITOR_AWARE_V2)` bootstrap at process start - `WindowsAdapterSession` implementing `AdapterSession` via `open_session` — owns COM apartment state so later sub-phases don't reinvent COM lifecycle - Re-verify the dependency pins below against crates.io + supply-chain policy (pinned at 2026-04 research time — see New Dependencies) -- Fix the `private_file_windows_security` `AceSize` validation gap (`crates/core/src/private_file_windows_security.rs`, currently cfg-gated dead code on non-Windows CI) so the private-artifact-writing path is real before any Windows code writes a refmap or trace file +- Implement Windows private-file hardening from scratch, behind `PlatformAdapter` or as a Windows-gated dependency of the `agent-desktop-windows` crate — never as unconditional `agent-desktop-core` surface (see `docs/solutions/best-practices/never-ship-platform-code-that-ci-cannot-execute.md`) — satisfying, with evidence from 2.0's probes: `FILE_SHARE_DELETE` on every concurrently-open handle across an atomic replace; owner validation against `TokenOwner`, not `TokenUser`; no locality inference from `FileRemoteProtocolInfo`; and an ancestor-vs-leaf validation contract decided deliberately, matching or explicitly diverging from the unix leaf-only rule, so the private-artifact-writing path is real before any Windows code writes a refmap or trace file -**Key APIs:** `CoInitializeEx`, `SetProcessDpiAwarenessContext`, Win32 ACL / `AceSize` validation (private-file security) +**Key APIs:** `CoInitializeEx`, `SetProcessDpiAwarenessContext`, Win32 ACL / `TokenOwner` validation, `FILE_SHARE_DELETE` (private-file hardening) **Depends on:** nothing (opening sub-phase) -**Exit criteria:** workspace green on Windows CI; `WindowsAdapter` constructs and satisfies the trait; every command returns honest `PLATFORM_NOT_SUPPORTED` on Windows; the permission probe is unit-tested against mocked COM security state. +**Exit criteria:** workspace green on Windows CI; `WindowsAdapter` constructs and satisfies the trait; every command returns honest `PLATFORM_NOT_SUPPORTED` on Windows; the permission probe is unit-tested against mocked COM security state; private-file hardening is unit-tested on the `windows-latest` CI lane, not merely `cargo check`-clean. -**Est. PR size:** ~0.8k LOC +**Est. PR size:** ~1.3k LOC (bootstrap ~0.8k + from-scratch private-file hardening ~0.5k) ### 2.2 — UIA Element Wrapper & Tree Walk @@ -1170,11 +1170,11 @@ Every sub-phase 2.0–2.15 below is held to the same definition of done, stated **Scope:** - `ScreenshotBackend::Legacy` first (`PrintWindow(hwnd, hdc, PW_RENDERFULLCONTENT)` — mitigates DWM black frames), then `ScreenshotBackend::Modern` via `windows-capture`/WGC where the session supports it (P2-O13) - `screenshot --screen` honest display targeting (pairs with `list_displays` from 2.4) -- Typed clipboard: `CF_UNICODETEXT` → `ClipboardContent::Text`, image via `CF_DIB`/PNG → `ClipboardContent::Image`, `CF_HDROP` file lists → `ClipboardContent::FileUrls`, all written through 0600-equivalent private files (see 2.1's ACL fix) +- Typed clipboard: `CF_UNICODETEXT` → `ClipboardContent::Text`, image via `CF_DIB`/PNG → `ClipboardContent::Image`, `CF_HDROP` file lists → `ClipboardContent::FileUrls`, all written through 0600-equivalent private files (see 2.1's private-file hardening) **Key APIs:** `PrintWindow`, `windows-capture`/`Windows.Graphics.Capture`, `OpenClipboard`/`GetClipboardData`/`SetClipboardData` -**Depends on:** 2.1 (private-file ACL fix), 2.4 (displays) +**Depends on:** 2.1 (private-file hardening), 2.4 (displays) **Exit criteria:** screenshot + clipboard e2e pass (clipboard tests hermetic, no real-desktop clipboard pollution); WGC gracefully degrades (falls back to `Legacy`, does not fail) in RDP-limited sessions. @@ -1652,7 +1652,7 @@ Same rendering shape, same [Cross-cutting sub-phase DoD](#cross-cutting-sub-phas **Key APIs:** `org.freedesktop.portal.ScreenCast`, `org.freedesktop.portal.RemoteDesktop`, `XGetImage`, `wl-clipboard`/`xclip` -**Depends on:** 3.1 (private-file handling — Unix permissions already real, unlike the Windows ACL gap), 3.4 (displays) +**Depends on:** 3.1 (private-file handling — Unix permissions already real; Windows private-file hardening is still to be built from scratch in 2.1), 3.4 (displays) **Exit criteria:** screenshot + clipboard e2e pass (clipboard tests hermetic); the PipeWire portal flow is proven — the user approves via the XDG portal dialog once, subsequent calls bypass the dialog within the session grant window. diff --git a/docs/solutions/best-practices/never-ship-platform-code-that-ci-cannot-execute.md b/docs/solutions/best-practices/never-ship-platform-code-that-ci-cannot-execute.md new file mode 100644 index 0000000..50a0961 --- /dev/null +++ b/docs/solutions/best-practices/never-ship-platform-code-that-ci-cannot-execute.md @@ -0,0 +1,95 @@ +--- +title: Never ship platform code that CI cannot execute +date: 2026-07-25 +category: best-practices +module: crates/core private file I/O +problem_type: process_gap +component: tooling +symptoms: + - "A platform-conditional branch in core compiles everywhere but has only ever run on one OS." + - "225 of 940 core unit tests fail the first time the suite is run on Windows." + - "status fails closed on a plain local NTFS disk with 'cannot verify that the Windows storage is local'." + - "A platform unit test compares two constants and can never fail." +root_cause: process_gap +resolution_type: code_removal +severity: high +tags: [platform-isolation, ci, windows, private-file, cfg, dead-code] +--- + +# Never ship platform code that CI cannot execute + +## Problem + +`agent-desktop-core` is defined as platform-agnostic. Its domain logic honoured that — +five `target_os` lines in the whole crate, every command test driven by `MockAdapter`. + +Its filesystem layer did not. `private_file*` carried 1,062 LOC of Windows-only `unsafe` +Win32: ACL construction, SID comparison, ancestor guard chains, reparse-point rejection, +remote-volume detection. The Windows adapter it served was a 76-LOC stub with no +`windows-sys` dependency and no implemented command — 14x more Windows code in +"platform-agnostic" core than in the Windows crate. + +CI ran `cargo check` on Windows and Linux, and `cargo test` on macOS alone. The Win32 +layer shipped in v0.5.0 type-checked and never once executed. Its first execution, on a +real Windows machine, failed 225 of 940 core tests across four clusters: + +- 122 sharing violations. `LEAF_SHARING` deliberately excluded `FILE_SHARE_DELETE`, so + `SetFileInformationByHandle(FileRenameInfo)` collided with the validation handle the + same code still held open. POSIX `rename(2)` ignores open descriptors; Windows does not. +- 69 owner-only-DACL rejections. Owner validation compared against `TokenUser`; elevated + tokens own new objects as `BUILTIN\Administrators`, which is how most CI runners execute. +- 9 trace-writer access denials and 25 cascading assertion failures. +- `status` dead on Windows: locality was inferred from + `GetFileInformationByHandleEx(FileRemoteProtocolInfo)` failing with exactly + `ERROR_INVALID_PARAMETER`. Local NTFS returns other codes, so the gate failed closed — + and it ran on every ancestor from the volume root down. + +The layer also diverged from the contract it claimed to implement. On unix, ownership and +mode are checked on the leaf directory only; ancestors merely have to be non-symlink +directories. The Windows path validated every ancestor and rejected any reparse point. +Nobody decided that. It drifted, and no test could notice. + +The single Windows behaviour test in the deleted code was: + +```rust +assert_eq!(LEAF_SHARING & FILE_SHARE_DELETE, 0); +``` + +It asserted the defect was correct, compared constants to constants, and could never fail. + +## Resolution + +Deleted all 1,062 LOC and removed `windows-sys` from core entirely. Windows now uses the +same portable `std::fs` path as every other non-unix target. This is honest: the hardening +guarded refmap, trace, and session artifacts on a platform where no command can produce +them. + +Added real `cargo test` lanes for Windows and Linux so every platform-conditional branch +in core is executed on every PR. That lane, not the deleted code, is the actual fix — it +is what stops the same mistake reaching the Linux adapter, whose +`validate_local_filesystem` had been equally unrun. + +## Rules + +- A `#[cfg]` branch that CI cannot execute is not shipped code, it is a hypothesis. Either + add a lane that runs it, or do not merge it. +- Do not write platform hardening ahead of the platform adapter it protects. Write it on + that platform, against a lane that runs it, informed by probes. +- A test that compares constants to constants is not a test. Every test must be able to + fail. +- When a platform layer needs a stricter policy than the shared contract, that is a + contract change to make deliberately and document — not a detail to bury in one OS. + +## Constraints for future Windows private-file hardening + +When hardened Windows I/O returns, it belongs behind `PlatformAdapter` or in a +Windows-gated dependency of the platform crate — never as unconditional core surface. It +must satisfy, with evidence: + +1. Atomic replace requires `FILE_SHARE_DELETE` on every concurrently-open handle to the + target. POSIX rename semantics do not transfer. +2. Owner validation compares against `TokenOwner`, not `TokenUser`. +3. `FileRemoteProtocolInfo` cannot establish volume locality. Use a verified primitive or + drop the gate rather than failing closed on local disks. +4. Ancestor-wide validation must match the unix leaf-only contract, or the contract must + change for both. diff --git a/rust-toolchain.toml b/rust-toolchain.toml index 6a5a047..ba0ffbd 100644 --- a/rust-toolchain.toml +++ b/rust-toolchain.toml @@ -1,3 +1,4 @@ [toolchain] channel = "1.89.0" profile = "minimal" +components = ["clippy", "rustfmt"]