-
Notifications
You must be signed in to change notification settings - Fork 6.5k
more world-writable warning improvements #6389
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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. " | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor nit: It's a bit unusual and inconsistent to use "We" in an error message like this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ahh yeah, I agree. Lemme land this and update in my next PR (it should be rare for folks to ever see this message) |
||
| .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, | ||
| ) { | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new guard in
spawn_world_writable_scanusesif let Ok(ref paths) = result && !paths.is_empty(), which relies on thelet_chainsfeature (codex-rs/tui/src/app.rslines 654‑656). This syntax is still unstable on the Rust channel used by the repo, so the crate will not compile (rustcemits E0658: "letexpressions in this position are unstable"). Please split the guard into a normalif letfollowed by an explicitif !paths.is_empty()(or equivalent) so the code builds on stable Rust.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codex The workspace pins rust-toolchain.toml:2 to 1.90.0, where let-chains are stable. This compiles just fine, all CI checks pass. Not going to change this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary
failed_scanflag when the sandbox audit cannot complete. tui/src/app.rsL640-L688windows_world_writable_flaggedtreats scan failures as unsafe by usingmap_or(true, …)and documenting why errors still trigger the warning path. tui/src/chatwidget.rsL2142-L2155Testing
cargo test -p codex-tuiView task →