Skip to content

Do not grant Windows sandbox ACLs on USERPROFILE #18443

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
efrazer-oai merged 2 commits into from
Apr 19, 2026
+151 −3
Merged

Do not grant Windows sandbox ACLs on USERPROFILE #18443

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2f71016
fix: block Windows sandbox profile root ACL grants
efrazer-oai Apr 18, 2026
f335d02
codex: address PR review feedback (#18443)
efrazer-oai Apr 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
154 changes: 151 additions & 3 deletions codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,10 @@ pub const ONLINE_USERNAME: &str = "CodexSandboxOnline";
const ERROR_CANCELLED: u32 = 1223;
const SECURITY_BUILTIN_DOMAIN_RID: u32 = 0x0000_0020;
const DOMAIN_ALIAS_RID_ADMINS: u32 = 0x0000_0220;
const USERPROFILE_READ_ROOT_EXCLUSIONS: &[&str] = &[
const USERPROFILE_ROOT_EXCLUSIONS: &[&str] = &[
".ssh",
".tsh",
".brev",
".gnupg",
".aws",
".azure",
Expand Down Expand Up @@ -333,7 +335,7 @@ fn profile_read_roots(user_profile: &Path) -> Vec<PathBuf> {
.map(|entry| (entry.file_name(), entry.path()))
.filter(|(name, _)| {
let name = name.to_string_lossy();
!USERPROFILE_READ_ROOT_EXCLUSIONS
!USERPROFILE_ROOT_EXCLUSIONS
.iter()
.any(|excluded| name.eq_ignore_ascii_case(excluded))
})
Expand Down Expand Up @@ -787,6 +789,9 @@ fn build_payload_roots(
request.env_map,
)
};
let write_roots = expand_user_profile_root(write_roots);
let write_roots = filter_user_profile_root(write_roots);
Copy link
Copy Markdown
Contributor

@chatgpt-codex-connector chatgpt-codex-connector bot Apr 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve USERPROFILE when it is the working-directory root

build_payload_roots always applies filter_user_profile_root to write roots. If command_cwd == USERPROFILE, gather_write_roots intentionally includes CWD, but this filter removes it. The payload then grants only existing child entries, so writes that require permission on the home root itself (e.g., creating a new top-level file/dir) can fail when running from home.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

@efrazer-oai efrazer-oai Apr 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is intended. Tradeoff is necessary to not break things like SSH as perms granted at top level percolate downward.

let write_roots = filter_user_profile_root_exclusions(write_roots);
let write_roots = filter_sensitive_write_roots(write_roots, request.codex_home);
let mut read_roots = if let Some(roots) = overrides.read_roots.as_deref() {
// An explicit override is the split policy's complete readable set. Keep only the
Expand All @@ -804,6 +809,9 @@ fn build_payload_roots(
} else {
gather_read_roots(request.command_cwd, request.policy, request.codex_home)
};
read_roots = expand_user_profile_root(read_roots);
read_roots = filter_user_profile_root(read_roots);
read_roots = filter_user_profile_root_exclusions(read_roots);
let write_root_set: HashSet<PathBuf> = write_roots.iter().cloned().collect();
read_roots.retain(|root| !write_root_set.contains(root));
(read_roots, write_roots)
Expand Down Expand Up @@ -834,6 +842,67 @@ fn build_payload_deny_write_paths(
deny_write_paths
}

fn expand_user_profile_root(roots: Vec<PathBuf>) -> Vec<PathBuf> {
let Ok(user_profile) = std::env::var("USERPROFILE") else {
return roots;
};
expand_user_profile_root_for(roots, Path::new(&user_profile))
}

fn expand_user_profile_root_for(roots: Vec<PathBuf>, user_profile: &Path) -> Vec<PathBuf> {
let user_profile_key = canonical_path_key(user_profile);
let mut expanded = Vec::new();
for root in roots {
if canonical_path_key(&root) == user_profile_key {
expanded.extend(profile_read_roots(user_profile));
} else {
expanded.push(root);
}
}

expanded.sort_by_key(|root| canonical_path_key(root));
expanded.dedup_by(|a, b| canonical_path_key(a.as_path()) == canonical_path_key(b.as_path()));
expanded
}

fn filter_user_profile_root(mut roots: Vec<PathBuf>) -> Vec<PathBuf> {
let Ok(user_profile) = std::env::var("USERPROFILE") else {
return roots;
};
let user_profile_key = canonical_path_key(Path::new(&user_profile));
roots.retain(|root| canonical_path_key(root) != user_profile_key);
roots
}

fn filter_user_profile_root_exclusions(mut roots: Vec<PathBuf>) -> Vec<PathBuf> {
let Ok(user_profile) = std::env::var("USERPROFILE") else {
return roots;
};
let user_profile = Path::new(&user_profile);
roots.retain(|root| !is_user_profile_root_exclusion(root, user_profile));
roots
}

fn is_user_profile_root_exclusion(root: &Path, user_profile: &Path) -> bool {
let root_key = canonical_path_key(root);
let profile_key = canonical_path_key(user_profile);
let profile_prefix = format!("{}/", profile_key.trim_end_matches('/'));
let Some(relative_key) = root_key.strip_prefix(&profile_prefix) else {
return false;
};
let Some(child_name) = relative_key
.split('/')
.next()
.filter(|name| !name.is_empty())
else {
return false;
};

USERPROFILE_ROOT_EXCLUSIONS
.iter()
.any(|excluded| child_name.eq_ignore_ascii_case(excluded))
}

fn filter_sensitive_write_roots(mut roots: Vec<PathBuf>, codex_home: &Path) -> Vec<PathBuf> {
// Never grant capability write access to CODEX_HOME or anything under CODEX_HOME/.sandbox,
// CODEX_HOME/.sandbox-bin, or CODEX_HOME/.sandbox-secrets. These locations contain sandbox
Expand Down Expand Up @@ -1029,13 +1098,15 @@ mod tests {
let tmp = TempDir::new().expect("tempdir");
let user_profile = tmp.path();
let allowed_dir = user_profile.join("Documents");
let allowed_file = user_profile.join(".gitconfig");
let allowed_file = user_profile.join("settings.json");
let excluded_dir = user_profile.join(".ssh");
let excluded_tsh = user_profile.join(".tsh");
let excluded_case_variant = user_profile.join(".AWS");

fs::create_dir_all(&allowed_dir).expect("create allowed dir");
fs::write(&allowed_file, "safe").expect("create allowed file");
fs::create_dir_all(&excluded_dir).expect("create excluded dir");
fs::create_dir_all(&excluded_tsh).expect("create excluded tsh dir");
fs::create_dir_all(&excluded_case_variant).expect("create excluded case variant");

let roots = profile_read_roots(user_profile);
Expand All @@ -1055,6 +1126,83 @@ mod tests {
assert_eq!(vec![missing_profile], roots);
}

#[test]
fn is_user_profile_root_exclusion_blocks_configured_children() {
let tmp = TempDir::new().expect("tempdir");
let user_profile = tmp.path().join("user-profile");
let documents = user_profile.join("Documents");
let app_data = user_profile.join("AppData");
let ssh_child = user_profile.join(".ssh").join("config");
let tsh_child = user_profile.join(".tsh").join("keys");
let other_root = tmp.path().join("other-root");
fs::create_dir_all(&documents).expect("create documents");
fs::create_dir_all(&app_data).expect("create app data");
fs::create_dir_all(&ssh_child).expect("create ssh child");
fs::create_dir_all(&tsh_child).expect("create tsh child");
fs::create_dir_all(&other_root).expect("create other root");

assert!(!super::is_user_profile_root_exclusion(
&documents,
&user_profile
));
assert!(!super::is_user_profile_root_exclusion(
&app_data,
&user_profile
));
assert!(super::is_user_profile_root_exclusion(
&ssh_child,
&user_profile
));
assert!(super::is_user_profile_root_exclusion(
&tsh_child,
&user_profile
));
assert!(!super::is_user_profile_root_exclusion(
&other_root,
&user_profile
));
}

#[test]
fn expand_user_profile_root_for_replaces_profile_root_with_children() {
let tmp = TempDir::new().expect("tempdir");
let user_profile = tmp.path().join("user-profile");
let documents = user_profile.join("Documents");
let excluded = user_profile.join(".local");
let other_root = tmp.path().join("other-root");
fs::create_dir_all(&documents).expect("create documents");
fs::create_dir_all(&excluded).expect("create excluded dir");
fs::create_dir_all(&other_root).expect("create other root");

let roots = super::expand_user_profile_root_for(
vec![user_profile.clone(), other_root.clone()],
&user_profile,
);
let actual: HashSet<PathBuf> = roots.into_iter().collect();
let expected: HashSet<PathBuf> = [documents, excluded, other_root].into_iter().collect();

assert_eq!(expected, actual);
}

#[test]
fn expanded_write_roots_still_drop_protected_codex_home() {
let tmp = TempDir::new().expect("tempdir");
let user_profile = tmp.path().join("user-profile");
let codex_home = user_profile.join("CodexHome");
let documents = user_profile.join("Documents");
fs::create_dir_all(&codex_home).expect("create codex home");
fs::create_dir_all(&documents).expect("create documents");

let mut roots =
super::expand_user_profile_root_for(vec![user_profile.clone()], &user_profile);
let user_profile_key = super::canonical_path_key(&user_profile);
roots.retain(|root| super::canonical_path_key(root) != user_profile_key);
roots.retain(|root| !super::is_user_profile_root_exclusion(root, &user_profile));
let roots = super::filter_sensitive_write_roots(roots, &codex_home);

assert_eq!(vec![documents], roots);
}

#[test]
fn gather_read_roots_includes_helper_bin_dir() {
let tmp = TempDir::new().expect("tempdir");
Expand Down
Loading