more world-writable warning improvements (openai#6389)

iceweasel-oai · web-flow · commit a47181e471b6 · 2025-11-08T11:35:43.000-08:00
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)
diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs
@@ -174,13 +174,14 @@ impl App {
             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")]
         {
             let should_check = codex_core::get_platform_sandbox().is_some()
                 && matches!(
                     app.config.sandbox_policy,
                     codex_core::protocol::SandboxPolicy::WorkspaceWrite { .. }
+                        | codex_core::protocol::SandboxPolicy::ReadOnly
                 )
                 && !app
                     .config
@@ -192,7 +193,7 @@ impl App {
                 let env_map: std::collections::HashMap<String, String> = std::env::vars().collect();
                 let tx = app.app_event_tx.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 } => {
                 self.chat_widget.open_full_access_confirmation(preset);
             }
-            AppEvent::OpenWorldWritableWarningConfirmation { preset } => {
-                self.chat_widget
-                    .open_world_writable_warning_confirmation(preset);
+            AppEvent::OpenWorldWritableWarningConfirmation {
+                preset,
+                sample_paths,
+                extra_count,
+                failed_scan,
+            } => {
+                self.chat_widget.open_world_writable_warning_confirmation(
+                    preset,
+                    sample_paths,
+                    extra_count,
+                    failed_scan,
+                );
             }
             AppEvent::OpenFeedbackNote {
                 category,
@@ -449,14 +459,15 @@ impl App {
             }
             AppEvent::UpdateSandboxPolicy(policy) => {
                 #[cfg(target_os = "windows")]
-                let policy_is_workspace_write = matches!(
+                let policy_is_workspace_write_or_ro = matches!(
                     policy,
                     codex_core::protocol::SandboxPolicy::WorkspaceWrite { .. }
+                        | codex_core::protocol::SandboxPolicy::ReadOnly
                 );
 
                 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")]
                 {
                     // One-shot suppression if the user just confirmed continue.
@@ -466,15 +477,15 @@ impl App {
                     }
 
                     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();
                     if should_check {
                         let cwd = self.config.cwd.clone();
                         let env_map: std::collections::HashMap<String, String> =
                             std::env::vars().collect();
                         let tx = self.app_event_tx.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>,
         logs_base_dir: PathBuf,
         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 || {
-            if codex_windows_sandbox::preflight_audit_everyone_writable(
+            let result = codex_windows_sandbox::preflight_audit_everyone_writable(
                 &cwd,
                 &env_map,
                 Some(logs_base_dir.as_path()),
-            )
-            .is_err()
+            );
+            if let Ok(ref paths) = result
+                && !paths.is_empty()
             {
-                if apply_preset_on_continue {
-                    if let Some(preset) = codex_common::approval_presets::builtin_approval_presets()
-                        .into_iter()
-                        .find(|p| p.id == "auto")
-                    {
-                        tx.send(AppEvent::OpenWorldWritableWarningConfirmation {
-                            preset: Some(preset),
-                        });
-                    }
+                let as_strings: Vec<String> = paths
+                    .iter()
+                    .map(|p| normalize_windows_path_for_display(p))
+                    .collect();
+                let sample_paths: Vec<String> = as_strings.iter().take(3).cloned().collect();
+                let extra_count = if as_strings.len() > sample_paths.len() {
+                    as_strings.len() - sample_paths.len()
                 } 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,
+                });
             }
         });
     }
diff --git a/codex-rs/tui/src/app_event.rs b/codex-rs/tui/src/app_event.rs
@@ -79,6 +79,12 @@ pub(crate) enum AppEvent {
     #[cfg_attr(not(target_os = "windows"), allow(dead_code))]
     OpenWorldWritableWarningConfirmation {
         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.
diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs
@@ -2045,9 +2045,48 @@ impl ChatWidget {
                         && self.windows_world_writable_flagged()
                     {
                         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| {
                             tx.send(AppEvent::OpenWorldWritableWarningConfirmation {
                                 preset: Some(preset_clone.clone()),
+                                sample_paths: sample_paths.clone(),
+                                extra_count,
+                                failed_scan,
                             });
                         })]
                     } else {
@@ -2111,7 +2150,7 @@ impl ChatWidget {
             &env_map,
             Some(self.config.codex_home.as_path()),
         ) {
-            Ok(()) => false,
+            Ok(paths) => !paths.is_empty(),
             Err(_) => true,
         }
     }
@@ -2184,31 +2223,66 @@ impl ChatWidget {
     pub(crate) fn open_world_writable_warning_confirmation(
         &mut self,
         preset: Option<ApprovalPreset>,
+        sample_paths: Vec<String>,
+        extra_count: usize,
+        failed_scan: bool,
     ) {
         let (approval, sandbox) = match &preset {
             Some(p) => (Some(p.approval), Some(p.sandbox.clone())),
             None => (None, None),
         };
         let mut header_children: Vec<Box<dyn Renderable>> = Vec::new();
-        let title_line = Line::from("Auto mode has unprotected directories").bold();
-        let info_line = Line::from(vec![
-            "Some important directories on this system are world-writable. ".into(),
-            "The Windows sandbox cannot protect writes to these locations in Auto mode."
+        let mode_label = match self.config.sandbox_policy {
+            SandboxPolicy::WorkspaceWrite { .. } => "Auto mode",
+            SandboxPolicy::ReadOnly => "Read-Only 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),
-        ]);
+            ])
+        };
         header_children.push(Box::new(title_line));
         header_children.push(Box::new(
             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);
 
         // Build actions ensuring acknowledgement happens before applying the new sandbox policy,
         // so downstream policy-change hooks don't re-trigger the warning.
         let mut accept_actions: Vec<SelectionAction> = Vec::new();
-        // Suppress the immediate re-scan once after user confirms continue.
-        accept_actions.push(Box::new(|tx| {
-            tx.send(AppEvent::SkipNextWorldWritableScan);
-        }));
+        // Suppress the immediate re-scan only when a preset will be applied (i.e., via /approvals),
+        // to avoid duplicate warnings from the ensuing policy change.
+        if preset.is_some() {
+            accept_actions.push(Box::new(|tx| {
+                tx.send(AppEvent::SkipNextWorldWritableScan);
+            }));
+        }
         if let (Some(approval), Some(sandbox)) = (approval, sandbox.clone()) {
             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));
         }
 
-        let deny_actions: Vec<SelectionAction> = if preset.is_some() {
-            vec![Box::new(|tx| {
-                tx.send(AppEvent::OpenApprovalsPopup);
-            })]
-        } else {
-            Vec::new()
-        };
-
         let items = vec![
             SelectionItem {
                 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,
                 dismiss_on_select: true,
                 ..Default::default()
             },
             SelectionItem {
                 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,
                 dismiss_on_select: true,
                 ..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 {
@@ -2266,6 +2325,9 @@ impl ChatWidget {
     pub(crate) fn open_world_writable_warning_confirmation(
         &mut self,
         _preset: Option<ApprovalPreset>,
+        _sample_paths: Vec<String>,
+        _extra_count: usize,
+        _failed_scan: bool,
     ) {
     }
 
diff --git a/codex-rs/windows-sandbox-rs/src/audit.rs b/codex-rs/windows-sandbox-rs/src/audit.rs
@@ -1,6 +1,5 @@
 use crate::token::world_sid;
 use crate::winutil::to_wide;
-use anyhow::anyhow;
 use anyhow::Result;
 use std::collections::HashSet;
 use std::ffi::c_void;
@@ -177,7 +176,7 @@ pub fn audit_everyone_writable(
     cwd: &Path,
     env: &std::collections::HashMap<String, String>,
     logs_base_dir: Option<&Path>,
-) -> Result<()> {
+) -> Result<Vec<PathBuf>> {
     let start = Instant::now();
     let mut flagged: Vec<PathBuf> = Vec::new();
     let mut seen: HashSet<String> = HashSet::new();
@@ -265,14 +264,7 @@ pub fn audit_everyone_writable(
             ),
             logs_base_dir,
         );
-        let mut list_err = String::new();
-        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
-        ));
+        return Ok(flagged);
     }
     // Log success once if nothing flagged
     crate::logging::log_note(
@@ -281,7 +273,7 @@ pub fn audit_everyone_writable(
         ),
         logs_base_dir,
     );
-    Ok(())
+    Ok(Vec::new())
 }
 // Fast mask-based check: does the DACL contain any ACCESS_ALLOWED ACE for
 // Everyone that includes generic or specific write bits? Skips inherit-only
diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs
