more world-writable warning improvements (#6389)

3 improvements:
1. show up to 3 actual paths that are world-writable
2. do the scan/warning for Read-Only mode too, because it also applies
there
3. remove the "Cancel" option since it doesn't always apply (like on
startup)
This commit is contained in:
iceweasel-oai
2025-11-08 11:35:43 -08:00
committed by GitHub
parent 5beb6167c8
commit a47181e471
5 changed files with 155 additions and 64 deletions

View File

@@ -174,13 +174,14 @@ impl App {
skip_world_writable_scan_once: false, skip_world_writable_scan_once: false,
}; };
// On startup, if Auto mode (workspace-write) is active, warn about world-writable dirs on Windows. // On startup, if Auto mode (workspace-write) or ReadOnly is active, warn about world-writable dirs on Windows.
#[cfg(target_os = "windows")] #[cfg(target_os = "windows")]
{ {
let should_check = codex_core::get_platform_sandbox().is_some() let should_check = codex_core::get_platform_sandbox().is_some()
&& matches!( && matches!(
app.config.sandbox_policy, app.config.sandbox_policy,
codex_core::protocol::SandboxPolicy::WorkspaceWrite { .. } codex_core::protocol::SandboxPolicy::WorkspaceWrite { .. }
| codex_core::protocol::SandboxPolicy::ReadOnly
) )
&& !app && !app
.config .config
@@ -192,7 +193,7 @@ impl App {
let env_map: std::collections::HashMap<String, String> = std::env::vars().collect(); let env_map: std::collections::HashMap<String, String> = std::env::vars().collect();
let tx = app.app_event_tx.clone(); let tx = app.app_event_tx.clone();
let logs_base_dir = app.config.codex_home.clone(); let logs_base_dir = app.config.codex_home.clone();
Self::spawn_world_writable_scan(cwd, env_map, logs_base_dir, tx, false); Self::spawn_world_writable_scan(cwd, env_map, logs_base_dir, tx);
} }
} }
@@ -386,9 +387,18 @@ impl App {
AppEvent::OpenFullAccessConfirmation { preset } => { AppEvent::OpenFullAccessConfirmation { preset } => {
self.chat_widget.open_full_access_confirmation(preset); self.chat_widget.open_full_access_confirmation(preset);
} }
AppEvent::OpenWorldWritableWarningConfirmation { preset } => { AppEvent::OpenWorldWritableWarningConfirmation {
self.chat_widget preset,
.open_world_writable_warning_confirmation(preset); sample_paths,
extra_count,
failed_scan,
} => {
self.chat_widget.open_world_writable_warning_confirmation(
preset,
sample_paths,
extra_count,
failed_scan,
);
} }
AppEvent::OpenFeedbackNote { AppEvent::OpenFeedbackNote {
category, category,
@@ -449,14 +459,15 @@ impl App {
} }
AppEvent::UpdateSandboxPolicy(policy) => { AppEvent::UpdateSandboxPolicy(policy) => {
#[cfg(target_os = "windows")] #[cfg(target_os = "windows")]
let policy_is_workspace_write = matches!( let policy_is_workspace_write_or_ro = matches!(
policy, policy,
codex_core::protocol::SandboxPolicy::WorkspaceWrite { .. } codex_core::protocol::SandboxPolicy::WorkspaceWrite { .. }
| codex_core::protocol::SandboxPolicy::ReadOnly
); );
self.chat_widget.set_sandbox_policy(policy); self.chat_widget.set_sandbox_policy(policy);
// If sandbox policy becomes workspace-write, run the Windows world-writable scan. // If sandbox policy becomes workspace-write or read-only, run the Windows world-writable scan.
#[cfg(target_os = "windows")] #[cfg(target_os = "windows")]
{ {
// One-shot suppression if the user just confirmed continue. // One-shot suppression if the user just confirmed continue.
@@ -466,7 +477,7 @@ impl App {
} }
let should_check = codex_core::get_platform_sandbox().is_some() let should_check = codex_core::get_platform_sandbox().is_some()
&& policy_is_workspace_write && policy_is_workspace_write_or_ro
&& !self.chat_widget.world_writable_warning_hidden(); && !self.chat_widget.world_writable_warning_hidden();
if should_check { if should_check {
let cwd = self.config.cwd.clone(); let cwd = self.config.cwd.clone();
@@ -474,7 +485,7 @@ impl App {
std::env::vars().collect(); std::env::vars().collect();
let tx = self.app_event_tx.clone(); let tx = self.app_event_tx.clone();
let logs_base_dir = self.config.codex_home.clone(); let logs_base_dir = self.config.codex_home.clone();
Self::spawn_world_writable_scan(cwd, env_map, logs_base_dir, tx, false); Self::spawn_world_writable_scan(cwd, env_map, logs_base_dir, tx);
} }
} }
} }
@@ -628,28 +639,48 @@ impl App {
env_map: std::collections::HashMap<String, String>, env_map: std::collections::HashMap<String, String>,
logs_base_dir: PathBuf, logs_base_dir: PathBuf,
tx: AppEventSender, tx: AppEventSender,
apply_preset_on_continue: bool,
) { ) {
#[inline]
fn normalize_windows_path_for_display(p: &std::path::Path) -> String {
let canon = dunce::canonicalize(p).unwrap_or_else(|_| p.to_path_buf());
canon.display().to_string().replace('/', "\\")
}
tokio::task::spawn_blocking(move || { tokio::task::spawn_blocking(move || {
if codex_windows_sandbox::preflight_audit_everyone_writable( let result = codex_windows_sandbox::preflight_audit_everyone_writable(
&cwd, &cwd,
&env_map, &env_map,
Some(logs_base_dir.as_path()), Some(logs_base_dir.as_path()),
) );
.is_err() if let Ok(ref paths) = result
&& !paths.is_empty()
{ {
if apply_preset_on_continue { let as_strings: Vec<String> = paths
if let Some(preset) = codex_common::approval_presets::builtin_approval_presets() .iter()
.into_iter() .map(|p| normalize_windows_path_for_display(p))
.find(|p| p.id == "auto") .collect();
{ let sample_paths: Vec<String> = as_strings.iter().take(3).cloned().collect();
tx.send(AppEvent::OpenWorldWritableWarningConfirmation { let extra_count = if as_strings.len() > sample_paths.len() {
preset: Some(preset), as_strings.len() - sample_paths.len()
});
}
} else { } else {
tx.send(AppEvent::OpenWorldWritableWarningConfirmation { preset: None }); 0
} };
tx.send(AppEvent::OpenWorldWritableWarningConfirmation {
preset: None,
sample_paths,
extra_count,
failed_scan: false,
});
} else if result.is_err() {
// Scan failed: still warn, but with no examples and mark as failed.
let sample_paths: Vec<String> = Vec::new();
let extra_count = 0usize;
tx.send(AppEvent::OpenWorldWritableWarningConfirmation {
preset: None,
sample_paths,
extra_count,
failed_scan: true,
});
} }
}); });
} }

View File

@@ -79,6 +79,12 @@ pub(crate) enum AppEvent {
#[cfg_attr(not(target_os = "windows"), allow(dead_code))] #[cfg_attr(not(target_os = "windows"), allow(dead_code))]
OpenWorldWritableWarningConfirmation { OpenWorldWritableWarningConfirmation {
preset: Option<ApprovalPreset>, preset: Option<ApprovalPreset>,
/// Up to 3 sample world-writable directories to display in the warning.
sample_paths: Vec<String>,
/// If there are more than `sample_paths`, this carries the remaining count.
extra_count: usize,
/// True when the scan failed (e.g. ACL query error) and protections could not be verified.
failed_scan: bool,
}, },
/// Show Windows Subsystem for Linux setup instructions for auto mode. /// Show Windows Subsystem for Linux setup instructions for auto mode.

View File

@@ -2045,9 +2045,48 @@ impl ChatWidget {
&& self.windows_world_writable_flagged() && self.windows_world_writable_flagged()
{ {
let preset_clone = preset.clone(); let preset_clone = preset.clone();
// Compute sample paths for the warning popup.
let mut env_map: std::collections::HashMap<String, String> =
std::collections::HashMap::new();
for (k, v) in std::env::vars() {
env_map.insert(k, v);
}
let (sample_paths, extra_count, failed_scan) =
match codex_windows_sandbox::preflight_audit_everyone_writable(
&self.config.cwd,
&env_map,
Some(self.config.codex_home.as_path()),
) {
Ok(paths) if !paths.is_empty() => {
fn normalize_windows_path_for_display(
p: &std::path::Path,
) -> String {
let canon = dunce::canonicalize(p)
.unwrap_or_else(|_| p.to_path_buf());
canon.display().to_string().replace('/', "\\")
}
let as_strings: Vec<String> = paths
.iter()
.map(|p| normalize_windows_path_for_display(p))
.collect();
let samples: Vec<String> =
as_strings.iter().take(3).cloned().collect();
let extra = if as_strings.len() > samples.len() {
as_strings.len() - samples.len()
} else {
0
};
(samples, extra, false)
}
Err(_) => (Vec::new(), 0, true),
_ => (Vec::new(), 0, false),
};
vec![Box::new(move |tx| { vec![Box::new(move |tx| {
tx.send(AppEvent::OpenWorldWritableWarningConfirmation { tx.send(AppEvent::OpenWorldWritableWarningConfirmation {
preset: Some(preset_clone.clone()), preset: Some(preset_clone.clone()),
sample_paths: sample_paths.clone(),
extra_count,
failed_scan,
}); });
})] })]
} else { } else {
@@ -2111,7 +2150,7 @@ impl ChatWidget {
&env_map, &env_map,
Some(self.config.codex_home.as_path()), Some(self.config.codex_home.as_path()),
) { ) {
Ok(()) => false, Ok(paths) => !paths.is_empty(),
Err(_) => true, Err(_) => true,
} }
} }
@@ -2184,31 +2223,66 @@ impl ChatWidget {
pub(crate) fn open_world_writable_warning_confirmation( pub(crate) fn open_world_writable_warning_confirmation(
&mut self, &mut self,
preset: Option<ApprovalPreset>, preset: Option<ApprovalPreset>,
sample_paths: Vec<String>,
extra_count: usize,
failed_scan: bool,
) { ) {
let (approval, sandbox) = match &preset { let (approval, sandbox) = match &preset {
Some(p) => (Some(p.approval), Some(p.sandbox.clone())), Some(p) => (Some(p.approval), Some(p.sandbox.clone())),
None => (None, None), None => (None, None),
}; };
let mut header_children: Vec<Box<dyn Renderable>> = Vec::new(); let mut header_children: Vec<Box<dyn Renderable>> = Vec::new();
let title_line = Line::from("Auto mode has unprotected directories").bold(); let mode_label = match self.config.sandbox_policy {
let info_line = Line::from(vec![ SandboxPolicy::WorkspaceWrite { .. } => "Auto mode",
"Some important directories on this system are world-writable. ".into(), SandboxPolicy::ReadOnly => "Read-Only mode",
"The Windows sandbox cannot protect writes to these locations in Auto mode." _ => "Auto mode",
};
let title_line = Line::from("Unprotected directories found").bold();
let info_line = if failed_scan {
Line::from(vec![
"We couldn't complete the world-writable scan, so protections cannot be verified. "
.into(),
format!("The Windows sandbox cannot guarantee protection in {mode_label}.")
.fg(Color::Red),
])
} else {
Line::from(vec![
"Some important directories on this system are world-writable. ".into(),
format!(
"The Windows sandbox cannot protect writes to these locations in {mode_label}."
)
.fg(Color::Red), .fg(Color::Red),
]); ])
};
header_children.push(Box::new(title_line)); header_children.push(Box::new(title_line));
header_children.push(Box::new( header_children.push(Box::new(
Paragraph::new(vec![info_line]).wrap(Wrap { trim: false }), Paragraph::new(vec![info_line]).wrap(Wrap { trim: false }),
)); ));
if !sample_paths.is_empty() {
// Show up to three examples and optionally an "and X more" line.
let mut lines: Vec<Line> = Vec::new();
lines.push(Line::from("Examples:").bold());
for p in &sample_paths {
lines.push(Line::from(format!(" - {p}")));
}
if extra_count > 0 {
lines.push(Line::from(format!("and {extra_count} more")));
}
header_children.push(Box::new(Paragraph::new(lines).wrap(Wrap { trim: false })));
}
let header = ColumnRenderable::with(header_children); let header = ColumnRenderable::with(header_children);
// Build actions ensuring acknowledgement happens before applying the new sandbox policy, // Build actions ensuring acknowledgement happens before applying the new sandbox policy,
// so downstream policy-change hooks don't re-trigger the warning. // so downstream policy-change hooks don't re-trigger the warning.
let mut accept_actions: Vec<SelectionAction> = Vec::new(); let mut accept_actions: Vec<SelectionAction> = Vec::new();
// Suppress the immediate re-scan once after user confirms continue. // Suppress the immediate re-scan only when a preset will be applied (i.e., via /approvals),
accept_actions.push(Box::new(|tx| { // to avoid duplicate warnings from the ensuing policy change.
tx.send(AppEvent::SkipNextWorldWritableScan); if preset.is_some() {
})); accept_actions.push(Box::new(|tx| {
tx.send(AppEvent::SkipNextWorldWritableScan);
}));
}
if let (Some(approval), Some(sandbox)) = (approval, sandbox.clone()) { if let (Some(approval), Some(sandbox)) = (approval, sandbox.clone()) {
accept_actions.extend(Self::approval_preset_actions(approval, sandbox)); accept_actions.extend(Self::approval_preset_actions(approval, sandbox));
} }
@@ -2222,36 +2296,21 @@ impl ChatWidget {
accept_and_remember_actions.extend(Self::approval_preset_actions(approval, sandbox)); accept_and_remember_actions.extend(Self::approval_preset_actions(approval, sandbox));
} }
let deny_actions: Vec<SelectionAction> = if preset.is_some() {
vec![Box::new(|tx| {
tx.send(AppEvent::OpenApprovalsPopup);
})]
} else {
Vec::new()
};
let items = vec![ let items = vec![
SelectionItem { SelectionItem {
name: "Continue".to_string(), name: "Continue".to_string(),
description: Some("Apply Auto mode for this session".to_string()), description: Some(format!("Apply {mode_label} for this session")),
actions: accept_actions, actions: accept_actions,
dismiss_on_select: true, dismiss_on_select: true,
..Default::default() ..Default::default()
}, },
SelectionItem { SelectionItem {
name: "Continue and don't warn again".to_string(), name: "Continue and don't warn again".to_string(),
description: Some("Enable Auto mode and remember this choice".to_string()), description: Some(format!("Enable {mode_label} and remember this choice")),
actions: accept_and_remember_actions, actions: accept_and_remember_actions,
dismiss_on_select: true, dismiss_on_select: true,
..Default::default() ..Default::default()
}, },
SelectionItem {
name: "Cancel".to_string(),
description: Some("Go back without enabling Auto mode".to_string()),
actions: deny_actions,
dismiss_on_select: true,
..Default::default()
},
]; ];
self.bottom_pane.show_selection_view(SelectionViewParams { self.bottom_pane.show_selection_view(SelectionViewParams {
@@ -2266,6 +2325,9 @@ impl ChatWidget {
pub(crate) fn open_world_writable_warning_confirmation( pub(crate) fn open_world_writable_warning_confirmation(
&mut self, &mut self,
_preset: Option<ApprovalPreset>, _preset: Option<ApprovalPreset>,
_sample_paths: Vec<String>,
_extra_count: usize,
_failed_scan: bool,
) { ) {
} }

View File

@@ -1,6 +1,5 @@
use crate::token::world_sid; use crate::token::world_sid;
use crate::winutil::to_wide; use crate::winutil::to_wide;
use anyhow::anyhow;
use anyhow::Result; use anyhow::Result;
use std::collections::HashSet; use std::collections::HashSet;
use std::ffi::c_void; use std::ffi::c_void;
@@ -177,7 +176,7 @@ pub fn audit_everyone_writable(
cwd: &Path, cwd: &Path,
env: &std::collections::HashMap<String, String>, env: &std::collections::HashMap<String, String>,
logs_base_dir: Option<&Path>, logs_base_dir: Option<&Path>,
) -> Result<()> { ) -> Result<Vec<PathBuf>> {
let start = Instant::now(); let start = Instant::now();
let mut flagged: Vec<PathBuf> = Vec::new(); let mut flagged: Vec<PathBuf> = Vec::new();
let mut seen: HashSet<String> = HashSet::new(); let mut seen: HashSet<String> = HashSet::new();
@@ -265,14 +264,7 @@ pub fn audit_everyone_writable(
), ),
logs_base_dir, logs_base_dir,
); );
let mut list_err = String::new(); return Ok(flagged);
for p in flagged {
list_err.push_str(&format!("\n - {}", p.display()));
}
return Err(anyhow!(
"Refusing to run: found directories writable by Everyone: {}",
list_err
));
} }
// Log success once if nothing flagged // Log success once if nothing flagged
crate::logging::log_note( crate::logging::log_note(
@@ -281,7 +273,7 @@ pub fn audit_everyone_writable(
), ),
logs_base_dir, logs_base_dir,
); );
Ok(()) Ok(Vec::new())
} }
// Fast mask-based check: does the DACL contain any ACCESS_ALLOWED ACE for // Fast mask-based check: does the DACL contain any ACCESS_ALLOWED ACE for
// Everyone that includes generic or specific write bits? Skips inherit-only // Everyone that includes generic or specific write bits? Skips inherit-only

View File

@@ -172,7 +172,7 @@ mod windows_impl {
cwd: &Path, cwd: &Path,
env_map: &HashMap<String, String>, env_map: &HashMap<String, String>,
logs_base_dir: Option<&Path>, logs_base_dir: Option<&Path>,
) -> Result<()> { ) -> Result<Vec<PathBuf>> {
audit::audit_everyone_writable(cwd, env_map, logs_base_dir) audit::audit_everyone_writable(cwd, env_map, logs_base_dir)
} }
@@ -438,7 +438,7 @@ mod stub {
_cwd: &Path, _cwd: &Path,
_env_map: &HashMap<String, String>, _env_map: &HashMap<String, String>,
_logs_base_dir: Option<&Path>, _logs_base_dir: Option<&Path>,
) -> Result<()> { ) -> Result<Vec<std::path::PathBuf>> {
bail!("Windows sandbox is only available on Windows") bail!("Windows sandbox is only available on Windows")
} }