Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions codex-rs/core/src/mcp/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ fn codex_apps_mcp_server_config(config: &Config, auth: Option<&CodexAuth>) -> Mc
tool_timeout_sec: None,
enabled_tools: None,
disabled_tools: None,
scopes: None,
}
}

Expand Down
13 changes: 13 additions & 0 deletions codex-rs/core/src/windows_sandbox.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,19 @@ pub fn sandbox_setup_is_complete(_codex_home: &Path) -> bool {
false
}

#[cfg(target_os = "windows")]
pub fn elevated_setup_failure_details(err: &anyhow::Error) -> Option<(String, String)> {
let failure = codex_windows_sandbox::extract_setup_failure(err)?;
let code = failure.code.as_str().to_string();
let message = codex_windows_sandbox::sanitize_setup_metric_tag_value(&failure.message);
Some((code, message))
}

#[cfg(not(target_os = "windows"))]
pub fn elevated_setup_failure_details(_err: &anyhow::Error) -> Option<(String, String)> {
None
}

#[cfg(target_os = "windows")]
pub fn run_elevated_setup(
policy: &SandboxPolicy,
Expand Down
19 changes: 18 additions & 1 deletion codex-rs/tui/src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1612,10 +1612,27 @@ impl App {
}
}
Err(err) => {
let mut code_tag: Option<String> = None;
let mut message_tag: Option<String> = None;
if let Some((code, message)) =
codex_core::windows_sandbox::elevated_setup_failure_details(
&err,
)
{
code_tag = Some(code);
message_tag = Some(message);
}
let mut tags: Vec<(&str, &str)> = Vec::new();
if let Some(code) = code_tag.as_deref() {
tags.push(("code", code));
}
if let Some(message) = message_tag.as_deref() {
tags.push(("message", message));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

non-blocking but would be great to include in core

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes - one of the things we'll address when we implement the NUX in the vsce is moving all the metrics stuff to core, so it gets included

otel_manager.counter(
"codex.windows_sandbox.elevated_setup_failure",
1,
&[],
&tags,
);
tracing::error!(
error = %err,
Expand Down
128 changes: 99 additions & 29 deletions codex-rs/windows-sandbox-rs/src/firewall.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ use windows::Win32::System::Com::CoUninitialize;
use windows::Win32::System::Com::CLSCTX_INPROC_SERVER;
use windows::Win32::System::Com::COINIT_APARTMENTTHREADED;

use codex_windows_sandbox::SetupErrorCode;
use codex_windows_sandbox::SetupFailure;

// This is the stable identifier we use to find/update the rule idempotently.
// It intentionally does not change between installs.
const OFFLINE_BLOCK_RULE_NAME: &str = "codex_sandbox_offline_block_outbound";
Expand All @@ -33,16 +36,27 @@ pub fn ensure_offline_outbound_block(offline_sid: &str, log: &mut File) -> Resul

let hr = unsafe { CoInitializeEx(None, COINIT_APARTMENTTHREADED) };
if hr.is_err() {
return Err(anyhow::anyhow!("CoInitializeEx failed: {hr:?}"));
return Err(anyhow::Error::new(SetupFailure::new(
SetupErrorCode::HelperFirewallComInitFailed,
format!("CoInitializeEx failed: {hr:?}"),
)));
}

let result = unsafe {
(|| -> Result<()> {
let policy: INetFwPolicy2 = CoCreateInstance(&NetFwPolicy2, None, CLSCTX_INPROC_SERVER)
.map_err(|e| anyhow::anyhow!("CoCreateInstance NetFwPolicy2: {e:?}"))?;
let rules = policy
.Rules()
.map_err(|e| anyhow::anyhow!("INetFwPolicy2::Rules: {e:?}"))?;
.map_err(|err| {
anyhow::Error::new(SetupFailure::new(
SetupErrorCode::HelperFirewallPolicyAccessFailed,
format!("CoCreateInstance NetFwPolicy2 failed: {err:?}"),
))
})?;
let rules = policy.Rules().map_err(|err| {
anyhow::Error::new(SetupFailure::new(
SetupErrorCode::HelperFirewallPolicyAccessFailed,
format!("INetFwPolicy2::Rules failed: {err:?}"),
))
})?;

// Block all outbound IP protocols for this user.
ensure_block_rule(
Expand Down Expand Up @@ -75,14 +89,28 @@ fn ensure_block_rule(
) -> Result<()> {
let name = BSTR::from(internal_name);
let rule: INetFwRule3 = match unsafe { rules.Item(&name) } {
Ok(existing) => existing
.cast()
.map_err(|e| anyhow::anyhow!("cast existing firewall rule to INetFwRule3: {e:?}"))?,
Ok(existing) => existing.cast().map_err(|err| {
anyhow::Error::new(SetupFailure::new(
SetupErrorCode::HelperFirewallRuleCreateOrAddFailed,
format!("cast existing firewall rule to INetFwRule3 failed: {err:?}"),
))
})?,
Err(_) => {
let new_rule: INetFwRule3 =
unsafe { CoCreateInstance(&NetFwRule, None, CLSCTX_INPROC_SERVER) }
.map_err(|e| anyhow::anyhow!("CoCreateInstance NetFwRule: {e:?}"))?;
unsafe { new_rule.SetName(&name) }.map_err(|e| anyhow::anyhow!("SetName: {e:?}"))?;
unsafe { CoCreateInstance(&NetFwRule, None, CLSCTX_INPROC_SERVER) }.map_err(
|err| {
anyhow::Error::new(SetupFailure::new(
SetupErrorCode::HelperFirewallRuleCreateOrAddFailed,
format!("CoCreateInstance NetFwRule failed: {err:?}"),
))
},
)?;
unsafe { new_rule.SetName(&name) }.map_err(|err| {
anyhow::Error::new(SetupFailure::new(
SetupErrorCode::HelperFirewallRuleCreateOrAddFailed,
format!("SetName failed: {err:?}"),
))
})?;
// Set all properties before adding the rule so we don't leave half-configured rules.
configure_rule(
&new_rule,
Expand All @@ -91,7 +119,12 @@ fn ensure_block_rule(
local_user_spec,
offline_sid,
)?;
unsafe { rules.Add(&new_rule) }.map_err(|e| anyhow::anyhow!("Rules::Add: {e:?}"))?;
unsafe { rules.Add(&new_rule) }.map_err(|err| {
anyhow::Error::new(SetupFailure::new(
SetupErrorCode::HelperFirewallRuleCreateOrAddFailed,
format!("Rules::Add failed: {err:?}"),
))
})?;
new_rule
}
};
Expand All @@ -117,29 +150,66 @@ fn configure_rule(
) -> Result<()> {
unsafe {
rule.SetDescription(&BSTR::from(friendly_desc))
.map_err(|e| anyhow::anyhow!("SetDescription: {e:?}"))?;
rule.SetDirection(NET_FW_RULE_DIR_OUT)
.map_err(|e| anyhow::anyhow!("SetDirection: {e:?}"))?;
rule.SetAction(NET_FW_ACTION_BLOCK)
.map_err(|e| anyhow::anyhow!("SetAction: {e:?}"))?;
rule.SetEnabled(VARIANT_TRUE)
.map_err(|e| anyhow::anyhow!("SetEnabled: {e:?}"))?;
rule.SetProfiles(NET_FW_PROFILE2_ALL.0)
.map_err(|e| anyhow::anyhow!("SetProfiles: {e:?}"))?;
rule.SetProtocol(protocol)
.map_err(|e| anyhow::anyhow!("SetProtocol: {e:?}"))?;
.map_err(|err| {
anyhow::Error::new(SetupFailure::new(
SetupErrorCode::HelperFirewallRuleCreateOrAddFailed,
format!("SetDescription failed: {err:?}"),
))
})?;
rule.SetDirection(NET_FW_RULE_DIR_OUT).map_err(|err| {
anyhow::Error::new(SetupFailure::new(
SetupErrorCode::HelperFirewallRuleCreateOrAddFailed,
format!("SetDirection failed: {err:?}"),
))
})?;
rule.SetAction(NET_FW_ACTION_BLOCK).map_err(|err| {
anyhow::Error::new(SetupFailure::new(
SetupErrorCode::HelperFirewallRuleCreateOrAddFailed,
format!("SetAction failed: {err:?}"),
))
})?;
rule.SetEnabled(VARIANT_TRUE).map_err(|err| {
anyhow::Error::new(SetupFailure::new(
SetupErrorCode::HelperFirewallRuleCreateOrAddFailed,
format!("SetEnabled failed: {err:?}"),
))
})?;
rule.SetProfiles(NET_FW_PROFILE2_ALL.0).map_err(|err| {
anyhow::Error::new(SetupFailure::new(
SetupErrorCode::HelperFirewallRuleCreateOrAddFailed,
format!("SetProfiles failed: {err:?}"),
))
})?;
rule.SetProtocol(protocol).map_err(|err| {
anyhow::Error::new(SetupFailure::new(
SetupErrorCode::HelperFirewallRuleCreateOrAddFailed,
format!("SetProtocol failed: {err:?}"),
))
})?;
rule.SetLocalUserAuthorizedList(&BSTR::from(local_user_spec))
.map_err(|e| anyhow::anyhow!("SetLocalUserAuthorizedList: {e:?}"))?;
.map_err(|err| {
anyhow::Error::new(SetupFailure::new(
SetupErrorCode::HelperFirewallRuleCreateOrAddFailed,
format!("SetLocalUserAuthorizedList failed: {err:?}"),
))
})?;
}

// Read-back verification: ensure we actually wrote the expected SID scope.
let actual = unsafe { rule.LocalUserAuthorizedList() }
.map_err(|e| anyhow::anyhow!("LocalUserAuthorizedList (read-back): {e:?}"))?;
let actual = unsafe { rule.LocalUserAuthorizedList() }.map_err(|err| {
anyhow::Error::new(SetupFailure::new(
SetupErrorCode::HelperFirewallRuleVerifyFailed,
format!("LocalUserAuthorizedList (read-back) failed: {err:?}"),
))
})?;
let actual_str = actual.to_string();
if !actual_str.contains(offline_sid) {
anyhow::bail!(
"offline firewall rule user scope mismatch: expected SID {offline_sid}, got {actual_str}"
);
return Err(anyhow::Error::new(SetupFailure::new(
SetupErrorCode::HelperFirewallRuleVerifyFailed,
format!(
"offline firewall rule user scope mismatch: expected SID {offline_sid}, got {actual_str}"
),
)));
}
Ok(())
}
Expand Down
17 changes: 17 additions & 0 deletions codex-rs/windows-sandbox-rs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ mod setup;
#[cfg(target_os = "windows")]
mod elevated_impl;

#[cfg(target_os = "windows")]
mod setup_error;

#[cfg(target_os = "windows")]
pub use acl::allow_null_device;
#[cfg(target_os = "windows")]
Expand Down Expand Up @@ -67,6 +70,20 @@ pub use setup::sandbox_secrets_dir;
#[cfg(target_os = "windows")]
pub use setup::SETUP_VERSION;
#[cfg(target_os = "windows")]
pub use setup_error::extract_failure as extract_setup_failure;
#[cfg(target_os = "windows")]
pub use setup_error::sanitize_tag_value as sanitize_setup_metric_tag_value;
#[cfg(target_os = "windows")]
pub use setup_error::setup_error_path;
#[cfg(target_os = "windows")]
pub use setup_error::write_setup_error_report;
#[cfg(target_os = "windows")]
pub use setup_error::SetupErrorCode;
#[cfg(target_os = "windows")]
pub use setup_error::SetupErrorReport;
#[cfg(target_os = "windows")]
pub use setup_error::SetupFailure;
#[cfg(target_os = "windows")]
pub use token::convert_string_sid_to_sid;
#[cfg(target_os = "windows")]
pub use token::create_readonly_token_with_cap_from;
Expand Down
80 changes: 70 additions & 10 deletions codex-rs/windows-sandbox-rs/src/sandbox_users.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ use codex_windows_sandbox::sandbox_dir;
use codex_windows_sandbox::sandbox_secrets_dir;
use codex_windows_sandbox::string_from_sid_bytes;
use codex_windows_sandbox::to_wide;
use codex_windows_sandbox::SetupErrorCode;
use codex_windows_sandbox::SetupFailure;
use codex_windows_sandbox::SETUP_VERSION;

pub const SANDBOX_USERS_GROUP: &str = "CodexSandboxUsers";
Expand Down Expand Up @@ -122,9 +124,10 @@ pub fn ensure_local_user(name: &str, password: &str, log: &mut File) -> Result<(
);
if upd != NERR_Success {
super::log_line(log, &format!("NetUserSetInfo failed for {name} code {upd}"))?;
return Err(anyhow::anyhow!(
"failed to create/update user {name}, code {status}/{upd}"
));
return Err(anyhow::Error::new(SetupFailure::new(
SetupErrorCode::HelperUserCreateOrUpdateFailed,
format!("failed to create/update user {name}, code {status}/{upd}"),
)));
}
}

Expand Down Expand Up @@ -174,7 +177,10 @@ pub fn ensure_local_group(name: &str, comment: &str, log: &mut File) -> Result<(
log,
&format!("NetLocalGroupAdd failed for {name} code {status} parm_err={parm_err}"),
)?;
anyhow::bail!("failed to create local group {name}, code {status}");
return Err(anyhow::Error::new(SetupFailure::new(
SetupErrorCode::HelperUsersGroupCreateFailed,
format!("failed to create local group {name}, code {status}"),
)));
}
}
Ok(())
Expand Down Expand Up @@ -394,11 +400,37 @@ fn write_secrets(
online_pwd: &str,
) -> Result<()> {
let sandbox_dir = sandbox_dir(codex_home);
std::fs::create_dir_all(&sandbox_dir)?;
std::fs::create_dir_all(&sandbox_dir).map_err(|err| {
anyhow::Error::new(SetupFailure::new(
SetupErrorCode::HelperUsersFileWriteFailed,
format!(
"failed to create sandbox dir {}: {err}",
sandbox_dir.display()
),
))
})?;
let secrets_dir = sandbox_secrets_dir(codex_home);
std::fs::create_dir_all(&secrets_dir)?;
let offline_blob = dpapi_protect(offline_pwd.as_bytes())?;
let online_blob = dpapi_protect(online_pwd.as_bytes())?;
std::fs::create_dir_all(&secrets_dir).map_err(|err| {
anyhow::Error::new(SetupFailure::new(
SetupErrorCode::HelperUsersFileWriteFailed,
format!(
"failed to create secrets dir {}: {err}",
secrets_dir.display()
),
))
})?;
let offline_blob = dpapi_protect(offline_pwd.as_bytes()).map_err(|err| {
anyhow::Error::new(SetupFailure::new(
SetupErrorCode::HelperDpapiProtectFailed,
format!("dpapi protect failed for offline user: {err}"),
))
})?;
let online_blob = dpapi_protect(online_pwd.as_bytes()).map_err(|err| {
anyhow::Error::new(SetupFailure::new(
SetupErrorCode::HelperDpapiProtectFailed,
format!("dpapi protect failed for online user: {err}"),
))
})?;
let users = SandboxUsersFile {
version: SETUP_VERSION,
offline: SandboxUserRecord {
Expand All @@ -420,7 +452,35 @@ fn write_secrets(
};
let users_path = secrets_dir.join("sandbox_users.json");
let marker_path = sandbox_dir.join("setup_marker.json");
std::fs::write(users_path, serde_json::to_vec_pretty(&users)?)?;
std::fs::write(marker_path, serde_json::to_vec_pretty(&marker)?)?;
let users_json = serde_json::to_vec_pretty(&users).map_err(|err| {
anyhow::Error::new(SetupFailure::new(
SetupErrorCode::HelperUsersFileWriteFailed,
format!("serialize sandbox users failed: {err}"),
))
})?;
std::fs::write(&users_path, users_json).map_err(|err| {
anyhow::Error::new(SetupFailure::new(
SetupErrorCode::HelperUsersFileWriteFailed,
format!(
"write sandbox users file {} failed: {err}",
users_path.display()
),
))
})?;
let marker_json = serde_json::to_vec_pretty(&marker).map_err(|err| {
anyhow::Error::new(SetupFailure::new(
SetupErrorCode::HelperSetupMarkerWriteFailed,
format!("serialize setup marker failed: {err}"),
))
})?;
std::fs::write(&marker_path, marker_json).map_err(|err| {
anyhow::Error::new(SetupFailure::new(
SetupErrorCode::HelperSetupMarkerWriteFailed,
format!(
"write setup marker file {} failed: {err}",
marker_path.display()
),
))
})?;
Ok(())
}
Loading
Loading