mirror of
https://github.com/openai/codex.git
synced 2026-10-29 19:08:43 +00:00
Keep Windows sandbox private desktops alive across helper exits (#44658)
## Why Private desktops owned by short-lived sandbox wrappers disappear when those wrappers exit, preventing reuse across filesystem helper requests. ## What changed - Select and cache private desktops in the calling process, keeping desktops separate for different sandbox permissions. - Pass the desktop name through the wrapper to the restricted-token and elevated backends so helpers reuse the selected desktop. - Separate sandbox account preparation from filesystem ACL refresh so desktop selection does not perform the wrapper's refresh. - Propagate desktop preparation errors and require a desktop name when the wrapper's private desktop flag is set. ## Testing Add a Windows filesystem regression test covering desktop survival and reuse across reads, writes, metadata queries, and streaming reads, plus separate read-only permissions and rejected writes. Extend wrapper argument tests to cover named desktops and rejection of a missing desktop name. GitOrigin-RevId: 05a1cb829a902732248bfa7f4ad7470d911fa9f6
This commit is contained in:
@@ -76,10 +76,10 @@ pub(crate) async fn create_file_system_context(
|
||||
|
||||
#[cfg(windows)]
|
||||
pub(crate) fn is_unsupported_restricted_token_host<T>(result: &std::io::Result<T>) -> bool {
|
||||
result.as_ref().err().is_some_and(|err| {
|
||||
err.to_string()
|
||||
.contains("windows sandbox failed: CreateRestrictedToken failed: 87")
|
||||
})
|
||||
result
|
||||
.as_ref()
|
||||
.err()
|
||||
.is_some_and(|err| err.to_string().contains("CreateRestrictedToken failed: 87"))
|
||||
}
|
||||
|
||||
pub(crate) fn absolute_path(path: std::path::PathBuf) -> AbsolutePathBuf {
|
||||
|
||||
@@ -8,6 +8,8 @@ mod shared;
|
||||
#[path = "file_system/support.rs"]
|
||||
mod support;
|
||||
|
||||
use std::collections::BTreeSet;
|
||||
use std::ffi::c_void;
|
||||
use std::path::Path;
|
||||
use std::process::Command;
|
||||
use std::time::Duration;
|
||||
@@ -22,10 +24,13 @@ use codex_exec_server::WriteFileOptions;
|
||||
use codex_protocol::config_types::WindowsSandboxLevel;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use futures::TryStreamExt;
|
||||
use pretty_assertions::assert_eq;
|
||||
use test_case::test_case;
|
||||
use tokio::net::windows::named_pipe::ServerOptions;
|
||||
use tokio::time::timeout;
|
||||
use uuid::Uuid;
|
||||
use windows_sys::Win32::System::Threading::GetCurrentProcess;
|
||||
|
||||
use crate::support::FileSystemImplementation;
|
||||
use crate::support::create_file_system_context;
|
||||
@@ -387,6 +392,169 @@ async fn file_system_remote_fs_helper_respects_windows_sandbox_write_policy() ->
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn file_system_private_desktop_survives_helper_exits_and_separates_permissions() -> Result<()>
|
||||
{
|
||||
let context = create_file_system_context(FileSystemImplementation::Local).await?;
|
||||
let file_system = context.file_system;
|
||||
let tmp = tempfile::TempDir::new()?;
|
||||
let path = tmp.path().join("contents.txt");
|
||||
std::fs::write(&path, b"initial")?;
|
||||
let uri = PathUri::from_host_native_path(&path)?;
|
||||
let mut sandbox = workspace_write_sandbox(tmp.path().to_path_buf());
|
||||
sandbox.windows_sandbox_private_desktop = true;
|
||||
let before = process_private_desktops()?;
|
||||
let read = file_system
|
||||
.read_file(&uri, ReadFileOptions::default(), Some(&sandbox))
|
||||
.await;
|
||||
if is_unsupported_restricted_token_host(&read) {
|
||||
eprintln!("Skipping private desktop reuse: this host cannot create restricted tokens");
|
||||
return Ok(());
|
||||
}
|
||||
assert_eq!(read?, b"initial");
|
||||
|
||||
// Ownership must outlive each helper; a desktop held only by the helper disappears here.
|
||||
let warmed = process_private_desktops()?;
|
||||
assert_eq!(warmed.difference(&before).count(), 1);
|
||||
for contents in ["updated", "updated again"] {
|
||||
file_system
|
||||
.write_file(
|
||||
&uri,
|
||||
contents.as_bytes().to_vec(),
|
||||
WriteFileOptions::default(),
|
||||
Some(&sandbox),
|
||||
)
|
||||
.await?;
|
||||
assert_eq!(process_private_desktops()?, warmed);
|
||||
assert_eq!(
|
||||
file_system
|
||||
.read_file(&uri, ReadFileOptions::default(), Some(&sandbox))
|
||||
.await?,
|
||||
contents.as_bytes()
|
||||
);
|
||||
assert_eq!(process_private_desktops()?, warmed);
|
||||
assert!(
|
||||
file_system
|
||||
.get_metadata(&uri, GetMetadataOptions::default(), Some(&sandbox))
|
||||
.await?
|
||||
.is_file
|
||||
);
|
||||
assert_eq!(process_private_desktops()?, warmed);
|
||||
let chunks = file_system
|
||||
.read_file_stream(&uri, Some(&sandbox))
|
||||
.await?
|
||||
.try_collect::<Vec<_>>()
|
||||
.await?;
|
||||
assert_eq!(chunks.concat(), contents.as_bytes());
|
||||
assert_eq!(process_private_desktops()?, warmed);
|
||||
}
|
||||
|
||||
let mut readonly = read_only_sandbox_for_cwd(tmp.path().to_path_buf())?;
|
||||
readonly.windows_sandbox_level = WindowsSandboxLevel::RestrictedToken;
|
||||
readonly.windows_sandbox_private_desktop = true;
|
||||
assert_eq!(
|
||||
file_system
|
||||
.read_file(&uri, ReadFileOptions::default(), Some(&readonly))
|
||||
.await?,
|
||||
b"updated again"
|
||||
);
|
||||
let separated = process_private_desktops()?;
|
||||
assert!(warmed.is_subset(&separated));
|
||||
assert_eq!(separated.difference(&warmed).count(), 1);
|
||||
file_system
|
||||
.write_file(
|
||||
&uri,
|
||||
b"blocked".to_vec(),
|
||||
WriteFileOptions::default(),
|
||||
Some(&readonly),
|
||||
)
|
||||
.await
|
||||
.expect_err("read-only filesystem requests must reject writes");
|
||||
assert_eq!(std::fs::read(&path)?, b"updated again");
|
||||
assert_eq!(process_private_desktops()?, separated);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn process_private_desktops() -> Result<BTreeSet<String>> {
|
||||
// Query this process so other tests' private desktops cannot affect the assertions.
|
||||
// Native layout: https://github.com/winsiderss/phnt/blob/master/ntpsapi.h
|
||||
#[repr(C)]
|
||||
struct HandleEntry {
|
||||
handle: isize,
|
||||
_handle_count: usize,
|
||||
_pointer_count: usize,
|
||||
_granted_access: u32,
|
||||
_object_type_index: u32,
|
||||
_handle_attributes: u32,
|
||||
_reserved: u32,
|
||||
}
|
||||
#[link(name = "ntdll")]
|
||||
unsafe extern "system" {
|
||||
fn NtQueryInformationProcess(
|
||||
process: isize,
|
||||
class: u32,
|
||||
information: *mut c_void,
|
||||
length: u32,
|
||||
return_length: *mut u32,
|
||||
) -> i32;
|
||||
}
|
||||
#[link(name = "user32")]
|
||||
unsafe extern "system" {
|
||||
fn GetUserObjectInformationW(
|
||||
object: isize,
|
||||
index: i32,
|
||||
information: *mut c_void,
|
||||
length: u32,
|
||||
length_needed: *mut u32,
|
||||
) -> i32;
|
||||
}
|
||||
let mut snapshot = vec![0usize; 8192];
|
||||
let bytes = std::mem::size_of_val(snapshot.as_slice());
|
||||
let status = unsafe {
|
||||
NtQueryInformationProcess(
|
||||
GetCurrentProcess(),
|
||||
/*class*/ 51,
|
||||
snapshot.as_mut_ptr().cast(),
|
||||
bytes as u32,
|
||||
std::ptr::null_mut(),
|
||||
)
|
||||
};
|
||||
anyhow::ensure!(status >= 0, "process handle query failed: {status:#x}");
|
||||
let count = snapshot[0];
|
||||
anyhow::ensure!(
|
||||
count <= (bytes - 2 * size_of::<usize>()) / size_of::<HandleEntry>(),
|
||||
"process handle snapshot exceeds its buffer"
|
||||
);
|
||||
let entries = unsafe {
|
||||
std::slice::from_raw_parts(snapshot.as_ptr().add(2).cast::<HandleEntry>(), count)
|
||||
};
|
||||
let mut desktops = BTreeSet::new();
|
||||
for entry in entries {
|
||||
let mut name = [0u16; 64];
|
||||
let mut length_needed = 0;
|
||||
if unsafe {
|
||||
GetUserObjectInformationW(
|
||||
entry.handle,
|
||||
/*index*/ 2,
|
||||
name.as_mut_ptr().cast(),
|
||||
std::mem::size_of_val(&name) as u32,
|
||||
&mut length_needed,
|
||||
)
|
||||
} != 0
|
||||
{
|
||||
let end = name
|
||||
.iter()
|
||||
.position(|&unit| unit == 0)
|
||||
.unwrap_or(name.len());
|
||||
let name = String::from_utf16(&name[..end])?;
|
||||
if name.starts_with("CodexSandboxDesktop-") {
|
||||
desktops.insert(name);
|
||||
}
|
||||
}
|
||||
}
|
||||
Ok(desktops)
|
||||
}
|
||||
|
||||
fn read_only_sandbox_for_cwd(cwd: std::path::PathBuf) -> Result<FileSystemSandboxContext> {
|
||||
Ok(FileSystemSandboxContext::from_legacy_sandbox_policy(
|
||||
SandboxPolicy::new_read_only_policy(),
|
||||
|
||||
Reference in New Issue
Block a user