diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index aafac77717..aacbc6e1bf 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2468,6 +2468,7 @@ dependencies = [ "codex-utils-cargo-bin", "codex-utils-cli", "codex-utils-path", + "codex-utils-pty", "codex-windows-sandbox", "crossterm", "flate2", @@ -2997,7 +2998,6 @@ dependencies = [ "tracing-test", "url", "uuid", - "which 8.0.0", "wiremock", "zip", ] @@ -4769,6 +4769,7 @@ dependencies = [ "dunce", "pretty_assertions", "tempfile", + "windows-sys 0.52.0", ] [[package]] diff --git a/codex-rs/cli/Cargo.toml b/codex-rs/cli/Cargo.toml index bad12f5594..7e174f8375 100644 --- a/codex-rs/cli/Cargo.toml +++ b/codex-rs/cli/Cargo.toml @@ -115,6 +115,7 @@ app_test_support = { workspace = true } assert_cmd = { workspace = true } assert_matches = { workspace = true } codex-utils-cargo-bin = { workspace = true } +codex-utils-pty = { workspace = true } flate2 = { workspace = true } futures = { workspace = true } insta = { workspace = true } diff --git a/codex-rs/cli/src/doctor.rs b/codex-rs/cli/src/doctor.rs index 0e0e06b3e5..be6a02dbdc 100644 --- a/codex-rs/cli/src/doctor.rs +++ b/codex-rs/cli/src/doctor.rs @@ -4,7 +4,8 @@ //! configuration, authentication, terminal, state paths, and bounded reachability //! probes without attempting repair or starting long-lived services. Each check //! returns a redacted, serializable row so the same data can back the human -//! summary and `--json` support report. +//! summary and `--json` support report. PATH entries are untrusted data: checks +//! may inspect them, but must not execute the programs they select. //! //! A failing check should describe the problem and remediation, but it should not //! mutate user state. That keeps the command safe to run before filing a support @@ -21,7 +22,6 @@ use std::io::Read; use std::net::IpAddr; use std::path::Path; use std::path::PathBuf; -use std::process::Command; use std::sync::Arc; use std::time::Duration; use std::time::Instant; @@ -133,10 +133,6 @@ const COLOR_ENV_VARS: &[&str] = &[ const TERMINAL_DIMENSION_ENV_VARS: &[&str] = &["COLUMNS", "LINES"]; const TERMINFO_ENV_VARS: &[&str] = &["TERMINFO", "TERMINFO_DIRS"]; const LOCALE_ENV_VARS: &[&str] = &["LC_ALL", "LC_CTYPE", "LANG"]; -#[cfg(windows)] -const NPM_COMMAND: &str = "npm.cmd"; -#[cfg(not(windows))] -const NPM_COMMAND: &str = "npm"; const REMOTE_TERMINAL_ENV_VARS: &[&str] = &[ "SSH_TTY", "SSH_CONNECTION", @@ -150,13 +146,6 @@ const REMOTE_TERMINAL_ENV_VARS: &[&str] = &[ "DISPLAY", "WT_SESSION", ]; -const TMUX_OPTION_NAMES: &[&str] = &[ - "extended-keys", - "xterm-keys", - "allow-passthrough", - "set-clipboard", - "focus-events", -]; const NARROW_TERMINAL_COLUMNS: u16 = 80; const NARROW_TERMINAL_ROWS: u16 = 24; @@ -430,7 +419,7 @@ async fn build_report( ), }) }, - async { run_sync_check("updates", progress.clone(), || updates_check(config)) }, + run_async_check("updates", progress.clone(), updates_check(config)), async { run_sync_check("network", progress.clone(), || network::check(Some(config))) }, @@ -450,7 +439,9 @@ async fn build_report( terminal_check(command.no_color) }) }, - run_async_check("git", progress.clone(), git_check(config.cwd.as_path())), + async { + run_sync_check("git", progress.clone(), || git_check(config.cwd.as_path())) + }, async { run_sync_check("terminal title", progress.clone(), || { terminal_title_check(config) @@ -528,7 +519,7 @@ async fn build_report( terminal_check(command.no_color) }) }, - run_async_check("git", progress.clone(), git_check(&cwd)), + async { run_sync_check("git", progress.clone(), || git_check(&cwd)) }, async { run_sync_check("state", progress.clone(), fallback_state_check) }, run_async_check( "provider reachability", @@ -907,41 +898,15 @@ fn installation_check(show_details: bool) -> DoctorCheck { } if doctor_managed_by_npm(current_exe.as_deref()) { - match npm_global_root_check() { - NpmRootCheck::Match { package_root } => { - details.push(format!("npm update target: {}", package_root.display())); - } - NpmRootCheck::Mismatch { - running_package_root, - npm_package_root, - } => { - status = CheckStatus::Fail; - summary = - "npm install -g @openai/codex would update a different install".to_string(); - remediation = Some(format!( - "Fix PATH or npm prefix so the running package root ({}) matches the npm global package root ({}).", - running_package_root.display(), - npm_package_root.display() - )); - details.push(format!( - "running package root: {}", - running_package_root.display() - )); - details.push(format!("npm package root: {}", npm_package_root.display())); - } - NpmRootCheck::MissingPackageRoot => { - status = status.max(CheckStatus::Warning); - summary = "npm-managed launch is missing package-root provenance".to_string(); - remediation = Some( - "Reinstall or update Codex so the JS shim provides CODEX_MANAGED_PACKAGE_ROOT." - .to_string(), - ); - } - NpmRootCheck::NpmUnavailable(error) => { - status = status.max(CheckStatus::Warning); - summary = "npm-managed launch could not inspect npm global root".to_string(); - details.push(format!("npm root -g failed: {error}")); - } + details + .push("npm update target: not inspected (PATH helpers are not executed)".to_string()); + if env::var_os("CODEX_MANAGED_PACKAGE_ROOT").is_none() { + status = status.max(CheckStatus::Warning); + summary = "npm-managed launch is missing package-root provenance".to_string(); + remediation = Some( + "Reinstall or update Codex so the JS shim provides CODEX_MANAGED_PACKAGE_ROOT." + .to_string(), + ); } } @@ -1063,62 +1028,6 @@ fn display_optional_path(path: Option<&Path>) -> String { .unwrap_or_else(|| "none".to_string()) } -#[derive(Debug, PartialEq, Eq)] -enum NpmRootCheck { - Match { - package_root: PathBuf, - }, - Mismatch { - running_package_root: PathBuf, - npm_package_root: PathBuf, - }, - MissingPackageRoot, - NpmUnavailable(String), -} - -fn npm_global_root_check() -> NpmRootCheck { - let Some(running_package_root) = env::var_os("CODEX_MANAGED_PACKAGE_ROOT").map(PathBuf::from) - else { - return NpmRootCheck::MissingPackageRoot; - }; - - let output = match run_command(NPM_COMMAND, ["root", "-g"]) { - Ok(output) => output, - Err(err) => return NpmRootCheck::NpmUnavailable(err), - }; - let Some(npm_root) = output.lines().map(str::trim).find(|line| !line.is_empty()) else { - return NpmRootCheck::NpmUnavailable("empty output from npm root -g".to_string()); - }; - - compare_npm_package_roots(&running_package_root, &PathBuf::from(npm_root)) -} - -fn compare_npm_package_roots(running_package_root: &Path, npm_root: &Path) -> NpmRootCheck { - let npm_package_root = npm_root.join("@openai").join("codex"); - let running = normalize_path_for_compare(running_package_root); - let target = normalize_path_for_compare(&npm_package_root); - if running == target { - NpmRootCheck::Match { - package_root: npm_package_root, - } - } else { - NpmRootCheck::Mismatch { - running_package_root: running_package_root.to_path_buf(), - npm_package_root, - } - } -} - -fn normalize_path_for_compare(path: &Path) -> String { - let canonical = path.canonicalize().unwrap_or_else(|_| path.to_path_buf()); - let raw = canonical.to_string_lossy().replace('\\', "/"); - if cfg!(windows) { - raw.to_ascii_lowercase() - } else { - raw - } -} - fn display_list>(items: &[T]) -> String { if items.is_empty() { "none".to_string() @@ -1132,39 +1041,16 @@ fn display_list>(items: &[T]) -> String { } fn codex_path_entries() -> Vec { - #[cfg(windows)] - let result = run_command("where", ["codex"]); - #[cfg(not(windows))] - let result = run_command("which", ["-a", "codex"]); - - result - .unwrap_or_default() - .lines() - .map(str::trim) - .filter(|line| !line.is_empty()) - .map(str::to_string) + let Ok(candidates) = which::which_all("codex") else { + return Vec::new(); + }; + let mut seen = BTreeSet::new(); + candidates + .filter(|candidate| seen.insert(candidate.clone())) + .map(|path| path.display().to_string()) .collect() } -fn run_command(program: &str, args: I) -> Result -where - I: IntoIterator, - S: AsRef, -{ - let output = Command::new(program) - .args(args) - .output() - .map_err(|err| err.to_string())?; - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string(); - if stderr.is_empty() { - return Err(format!("exited with status {}", output.status)); - } - return Err(stderr); - } - Ok(String::from_utf8_lossy(&output.stdout).to_string()) -} - fn config_check(config: &Config) -> DoctorCheck { let mut details = Vec::new(); details.push(format!("CODEX_HOME: {}", config.codex_home.display())); @@ -1748,7 +1634,7 @@ impl TerminalCheckInputs { let terminal_size = crossterm::terminal::size().map_err(|err| err.to_string()); let info = terminal_info(); let tmux_details = if matches!(info.multiplexer, Some(Multiplexer::Tmux { .. })) { - tmux_diagnostic_details() + vec!["tmux options: not inspected (PATH helpers are not executed)".to_string()] } else { Vec::new() }; @@ -2147,50 +2033,6 @@ fn terminal_size_issues(inputs: &TerminalCheckInputs) -> Vec { issues } -fn tmux_diagnostic_details() -> Vec { - let mut details = Vec::new(); - push_tmux_display_detail(&mut details, "tmux client termtype", "#{client_termtype}"); - push_tmux_display_detail(&mut details, "tmux client termname", "#{client_termname}"); - for option in TMUX_OPTION_NAMES { - let value = tmux_option_value(option).unwrap_or_else(|| "unavailable".to_string()); - details.push(format!("tmux {option}: {value}")); - } - details -} - -fn push_tmux_display_detail(details: &mut Vec, label: &str, format: &str) { - if let Some(value) = tmux_display_message(format) { - details.push(format!("{label}: {value}")); - } -} - -fn tmux_option_value(option: &str) -> Option { - let output = Command::new("tmux") - .args(["show-options", "-gqv", option]) - .output() - .ok()?; - if !output.status.success() { - return None; - } - non_empty_trimmed(String::from_utf8(output.stdout).ok()?) -} - -fn tmux_display_message(format: &str) -> Option { - let output = Command::new("tmux") - .args(["display-message", "-p", format]) - .output() - .ok()?; - if !output.status.success() { - return None; - } - non_empty_trimmed(String::from_utf8(output.stdout).ok()?) -} - -fn non_empty_trimmed(value: String) -> Option { - let value = value.trim().to_string(); - if value.is_empty() { None } else { Some(value) } -} - async fn state_check(config: &Config, command: &DoctorCommand) -> DoctorCheck { let mut details = Vec::new(); path_readiness(&mut details, "CODEX_HOME", &config.codex_home); @@ -3250,31 +3092,6 @@ mod tests { ); } - #[test] - fn compare_npm_package_roots_detects_match() { - let running = PathBuf::from("/prefix/lib/node_modules/@openai/codex"); - let npm_root = PathBuf::from("/prefix/lib/node_modules"); - assert_eq!( - compare_npm_package_roots(&running, &npm_root), - NpmRootCheck::Match { - package_root: npm_root.join("@openai").join("codex") - } - ); - } - - #[test] - fn compare_npm_package_roots_detects_mismatch() { - let running = PathBuf::from("/old/lib/node_modules/@openai/codex"); - let npm_root = PathBuf::from("/new/lib/node_modules"); - assert_eq!( - compare_npm_package_roots(&running, &npm_root), - NpmRootCheck::Mismatch { - running_package_root: running, - npm_package_root: npm_root.join("@openai").join("codex"), - } - ); - } - #[test] fn startup_warning_counts_group_known_sources() { let warnings = vec![ @@ -4366,7 +4183,7 @@ mod tests { } #[test] - fn terminal_check_keeps_tmux_probe_failures_non_fatal() { + fn terminal_check_allows_missing_tmux_details() { let mut inputs = terminal_inputs(); inputs.info.multiplexer = Some(Multiplexer::Tmux { version: None }); diff --git a/codex-rs/cli/src/doctor/git.rs b/codex-rs/cli/src/doctor/git.rs index fa94d97320..55739ceb58 100644 --- a/codex-rs/cli/src/doctor/git.rs +++ b/codex-rs/cli/src/doctor/git.rs @@ -1,62 +1,33 @@ +//! Inspects Git executable locations and repository metadata without running Git. + use std::collections::BTreeSet; use std::path::Path; use std::path::PathBuf; -use std::process::Output; -use std::process::Stdio; -use std::time::Duration; use codex_git_utils::get_git_repo_root; -use tokio::process::Command; -use tokio::time::timeout; use super::CheckStatus; use super::DoctorCheck; use super::DoctorIssue; -const GIT_COMMAND_TIMEOUT: Duration = Duration::from_secs(/*secs*/ 2); - #[derive(Clone, Debug, Default, Eq, PartialEq)] struct GitCheckInputs { selected_git: Option, git_candidates: Vec, - git_version: Option, - git_exec_path: Option, - git_build_options: Option, repo_root: Option, git_entry: Option, - branch: Option, - core_fsmonitor: Option, } -pub(super) async fn git_check(cwd: &Path) -> DoctorCheck { +pub(super) fn git_check(cwd: &Path) -> DoctorCheck { let selected_git = which::which("git").ok(); let git_candidates = git_candidates(); let repo_root = get_git_repo_root(cwd); - let (git_version, git_exec_path, git_build_options, branch, core_fsmonitor) = - if let Some(git_path) = selected_git.as_deref() { - let (version, exec_path, build_options, branch, fsmonitor) = tokio::join!( - git_output(git_path, cwd, &["--version"]), - git_output(git_path, cwd, &["--exec-path"]), - git_output(git_path, cwd, &["version", "--build-options"]), - git_output(git_path, cwd, &["rev-parse", "--abbrev-ref", "HEAD"]), - git_output(git_path, cwd, &["config", "--get", "core.fsmonitor"]), - ); - (version, exec_path, build_options, branch, fsmonitor) - } else { - (None, None, None, None, None) - }; - git_check_from_inputs(GitCheckInputs { selected_git, git_candidates, - git_version, - git_exec_path, - git_build_options, git_entry: repo_root.as_deref().map(git_entry_summary), repo_root, - branch, - core_fsmonitor, }) } @@ -70,17 +41,6 @@ fn git_check_from_inputs(inputs: GitCheckInputs) -> DoctorCheck { for (index, path) in inputs.git_candidates.iter().enumerate() { details.push(format!("PATH git #{}: {}", index + 1, path.display())); } - push_optional_detail(&mut details, "git version", inputs.git_version.as_deref()); - push_optional_detail( - &mut details, - "git exec path", - inputs.git_exec_path.as_deref(), - ); - push_optional_detail( - &mut details, - "git build options", - inputs.git_build_options.as_deref(), - ); match inputs.repo_root.as_deref() { Some(root) => { details.push("repo detected: true".to_string()); @@ -88,43 +48,24 @@ fn git_check_from_inputs(inputs: GitCheckInputs) -> DoctorCheck { } None => details.push("repo detected: false".to_string()), } - push_optional_detail(&mut details, ".git entry", inputs.git_entry.as_deref()); - push_optional_detail( - &mut details, - "git branch", - normalized_branch(inputs.branch.as_deref()), - ); - push_optional_detail( - &mut details, - "core.fsmonitor", - inputs - .core_fsmonitor - .as_deref() - .filter(|value| !value.is_empty()), - ); + if let Some(entry) = inputs.git_entry.as_deref() { + details.push(format!(".git entry: {entry}")); + } + details.push("git execution: not inspected (PATH helpers are not executed)".to_string()); let mut check = DoctorCheck::new( "git.environment", "git", CheckStatus::Ok, - git_summary(&inputs), + if inputs.selected_git.is_some() { + "git executable found; execution not verified" + } else { + "git executable not found" + }, ) .details(details); - if inputs.selected_git.is_some() && inputs.git_version.is_none() { - check.status = CheckStatus::Warning; - check.summary = "Git executable found but could not be run".to_string(); - check = check.issue( - DoctorIssue::new( - CheckStatus::Warning, - "Git executable was found on PATH but did not return a version", - ) - .expected("git --version succeeds") - .remedy("Fix the selected Git executable or PATH so Codex can inspect Git metadata.") - .field("git version") - .field("selected git"), - ); - } else if inputs.selected_git.is_none() && inputs.repo_root.is_some() { + if inputs.selected_git.is_none() && inputs.repo_root.is_some() { check.status = CheckStatus::Warning; check.summary = "Git repository detected but git executable was not found".to_string(); check = check.issue( @@ -136,50 +77,11 @@ fn git_check_from_inputs(inputs: GitCheckInputs) -> DoctorCheck { .remedy("Install Git or fix PATH so Codex can inspect repository metadata.") .field("selected git"), ); - } else if let Some(cause) = - old_windows_git_warning(inputs.git_version.as_deref(), cfg!(windows)) - { - check.status = CheckStatus::Warning; - check.summary = cause.clone(); - check = check.issue( - DoctorIssue::new(CheckStatus::Warning, cause) - .measured(inputs.git_version.unwrap_or_else(|| "unknown".to_string())) - .expected("current Git for Windows") - .remedy( - "Update Git for Windows or the bundled Git executable Codex resolves first.", - ) - .field("git version") - .field("selected git"), - ); } check } -fn git_summary(inputs: &GitCheckInputs) -> String { - match inputs.git_version.as_deref() { - Some(version) => version.to_string(), - None if inputs.selected_git.is_some() => { - "git executable found; version unavailable".to_string() - } - None => "git executable not found".to_string(), - } -} - -fn push_optional_detail(details: &mut Vec, label: &str, value: Option<&str>) { - if let Some(value) = value { - details.push(format!("{label}: {value}")); - } -} - -fn normalized_branch(branch: Option<&str>) -> Option<&str> { - match branch { - Some("HEAD") => Some("detached HEAD"), - Some(value) if !value.is_empty() => Some(value), - _ => None, - } -} - fn git_candidates() -> Vec { let Ok(candidates) = which::which_all("git") else { return Vec::new(); @@ -190,40 +92,6 @@ fn git_candidates() -> Vec { .collect() } -async fn git_output(git_path: &Path, cwd: &Path, args: &[&str]) -> Option { - let mut command = Command::new(git_path); - command - .env("GIT_OPTIONAL_LOCKS", "0") - .args(["-c", codex_git_utils::SAFE_BARE_REPOSITORY_CONFIG]) - .args(args) - .current_dir(cwd) - .stdin(Stdio::null()) - .kill_on_drop(true); - let output = timeout(GIT_COMMAND_TIMEOUT, command.output()) - .await - .ok()? - .ok()?; - command_output_text(output) -} - -fn command_output_text(output: Output) -> Option { - if !output.status.success() { - return None; - } - let stdout = String::from_utf8_lossy(&output.stdout); - let normalized = stdout - .lines() - .map(str::trim) - .filter(|line| !line.is_empty()) - .collect::>() - .join("; "); - if normalized.is_empty() { - None - } else { - Some(normalized) - } -} - fn git_entry_summary(repo_root: &Path) -> String { let entry = repo_root.join(".git"); match std::fs::metadata(&entry) { @@ -243,96 +111,12 @@ fn git_entry_summary(repo_root: &Path) -> String { } } -fn old_windows_git_warning(version: Option<&str>, is_windows: bool) -> Option { - if !is_windows { - return None; - } - let version = version?; - if version.to_ascii_lowercase().contains("msysgit") { - return Some("old msysgit installation may corrupt Windows TUI rendering".to_string()); - } - let parsed = parse_git_version(version)?; - if parsed.major < 2 || (parsed.major == 2 && parsed.minor <= 34) { - return Some("old Git for Windows may corrupt Windows TUI rendering".to_string()); - } - None -} - -#[derive(Clone, Copy, Debug, Eq, PartialEq)] -struct ParsedGitVersion { - major: u32, - minor: u32, - patch: u32, -} - -fn parse_git_version(version: &str) -> Option { - let version = version.strip_prefix("git version ")?; - let numeric = version - .split_whitespace() - .next()? - .split(".windows.") - .next() - .unwrap_or(version); - let mut parts = numeric.split('.'); - Some(ParsedGitVersion { - major: parts.next()?.parse().ok()?, - minor: parts.next()?.parse().ok()?, - patch: parts.next().unwrap_or("0").parse().ok()?, - }) -} - #[cfg(test)] mod tests { use pretty_assertions::assert_eq; use super::*; - #[test] - fn parses_git_for_windows_version() { - assert_eq!( - parse_git_version("git version 2.34.1.windows.1"), - Some(ParsedGitVersion { - major: 2, - minor: 34, - patch: 1, - }) - ); - assert_eq!( - parse_git_version("git version 2.54.0.windows.1"), - Some(ParsedGitVersion { - major: 2, - minor: 54, - patch: 0, - }) - ); - } - - #[test] - fn classifies_old_windows_git() { - assert_eq!( - old_windows_git_warning( - Some("git version 2.34.1.windows.1"), - /*is_windows*/ true - ) - .as_deref(), - Some("old Git for Windows may corrupt Windows TUI rendering") - ); - assert_eq!( - old_windows_git_warning( - Some("git version 2.54.0.windows.1"), - /*is_windows*/ true - ), - None - ); - assert_eq!( - old_windows_git_warning( - Some("git version 2.34.1.windows.1"), - /*is_windows*/ false - ), - None - ); - } - #[test] fn warns_when_git_repo_has_no_git_executable() { let check = git_check_from_inputs(GitCheckInputs { @@ -347,35 +131,30 @@ mod tests { ); } - #[test] - fn warns_when_selected_git_cannot_report_version() { - let check = git_check_from_inputs(GitCheckInputs { - selected_git: Some(PathBuf::from("/usr/bin/git")), - repo_root: Some(PathBuf::from("/repo")), - ..GitCheckInputs::default() - }); - - assert_eq!(check.status, CheckStatus::Warning); - assert_eq!(check.summary, "Git executable found but could not be run"); - } - #[test] fn reports_git_candidates_and_repo_metadata() { let check = git_check_from_inputs(GitCheckInputs { selected_git: Some(PathBuf::from("/usr/bin/git")), git_candidates: vec![PathBuf::from("/usr/bin/git"), PathBuf::from("/opt/bin/git")], - git_version: Some("git version 2.54.0".to_string()), - git_exec_path: Some("/usr/libexec/git-core".to_string()), repo_root: Some(PathBuf::from("/repo")), git_entry: Some("directory".to_string()), - branch: Some("main".to_string()), - core_fsmonitor: Some("false".to_string()), - ..GitCheckInputs::default() }); - assert_eq!(check.status, CheckStatus::Ok); - assert!(check.details.contains(&"PATH git entries: 2".to_string())); - assert!(check.details.contains(&"git branch: main".to_string())); - assert!(check.details.contains(&"core.fsmonitor: false".to_string())); + let report = super::super::DoctorReport { + schema_version: 1, + generated_at: "2026-01-01T00:00:00Z".to_string(), + overall_status: check.status, + codex_version: "test".to_string(), + checks: vec![check], + }; + insta::assert_snapshot!(super::super::render_human_report( + &report, + super::super::HumanOutputOptions { + show_details: true, + show_all: false, + ascii: true, + color_enabled: false, + }, + )); } } diff --git a/codex-rs/cli/src/doctor/output.rs b/codex-rs/cli/src/doctor/output.rs index d3d7dfd914..c4c0087259 100644 --- a/codex-rs/cli/src/doctor/output.rs +++ b/codex-rs/cli/src/doctor/output.rs @@ -642,7 +642,6 @@ fn display_summary(check: &DoctorCheck, options: HumanOutputOptions) -> String { "runtime" => runtime_summary(check), "install" if check.status == CheckStatus::Ok => "consistent".to_string(), "search" => search_summary(check), - "git" => git_summary(check), "terminal" => terminal_summary(check), "title" => title_summary(check, options), "state" => state_summary(check), @@ -681,12 +680,6 @@ fn search_summary(check: &DoctorCheck) -> String { } } -fn git_summary(check: &DoctorCheck) -> String { - detail::detail_value(check, "git version") - .or_else(|| detail::detail_value(check, "selected git")) - .unwrap_or_else(|| check.summary.clone()) -} - fn terminal_summary(check: &DoctorCheck) -> String { let mut parts = Vec::new(); if let Some(terminal) = detail::detail_value(check, "terminal") { diff --git a/codex-rs/cli/src/doctor/runtime.rs b/codex-rs/cli/src/doctor/runtime.rs index 2eb176189a..b012a66ec9 100644 --- a/codex-rs/cli/src/doctor/runtime.rs +++ b/codex-rs/cli/src/doctor/runtime.rs @@ -6,7 +6,6 @@ //! package files or from PATH. use std::env; -use std::process::Command; use codex_install_context::InstallContext; use codex_install_context::InstallMethod; @@ -48,7 +47,7 @@ pub(super) fn runtime_check() -> DoctorCheck { .details(details) } -/// Verifies that the search command selected by the install context is usable. +/// Inspects the search command selected by the install context without executing it. /// /// Package-layout installs should point at a bundled ripgrep binary, while local /// installs without that layout usually resolve rg from PATH. A warning here @@ -80,23 +79,12 @@ pub(super) fn search_check() -> DoctorCheck { } } } else { - match Command::new(&rg_command).arg("--version").output() { - Ok(output) if output.status.success() => { - let version = String::from_utf8_lossy(&output.stdout) - .lines() - .next() - .unwrap_or("rg version unknown") - .to_string(); - details.push(format!("search command readiness: {version}")); + match which::which(&rg_command) { + Ok(path) => { + details.push(format!("search command path: {}", path.display())); + details.push("search command readiness: found; execution not verified".to_string()); CheckStatus::Ok } - Ok(output) => { - details.push(format!( - "search command readiness: exited with status {}", - output.status - )); - CheckStatus::Warning - } Err(err) => { details.push(format!("search command readiness: {err}")); CheckStatus::Warning @@ -105,7 +93,7 @@ pub(super) fn search_check() -> DoctorCheck { }; let summary = match status { - CheckStatus::Ok => format!("search is OK ({provider})"), + CheckStatus::Ok => format!("search command found ({provider}); execution not verified"), CheckStatus::Warning => "search command could not be verified".to_string(), CheckStatus::Fail => unreachable!(), }; diff --git a/codex-rs/cli/src/doctor/snapshots/codex__doctor__git__tests__reports_git_candidates_and_repo_metadata.snap b/codex-rs/cli/src/doctor/snapshots/codex__doctor__git__tests__reports_git_candidates_and_repo_metadata.snap new file mode 100644 index 0000000000..16f810edeb --- /dev/null +++ b/codex-rs/cli/src/doctor/snapshots/codex__doctor__git__tests__reports_git_candidates_and_repo_metadata.snap @@ -0,0 +1,22 @@ +--- +source: cli/src/doctor/git.rs +assertion_line: 150 +expression: "super::super::render_human_report(&report, super::super::HumanOutputOptions\n{ show_details: true, show_all: false, ascii: true, color_enabled: false, },)" +--- +Codex Doctor vtest + +Environment + [ok] git git executable found; execution not verified + selected git /usr/bin/git + repo detected true + repo root /repo + .git entry directory + PATH entries (2) /usr/bin/git + /opt/bin/git + git execution not inspected (PATH helpers are not executed) + +------------------------------------------------------------- +1 ok | 0 warn | 0 fail ok + +--summary compact output --all expand truncated lists +--json redacted report diff --git a/codex-rs/cli/src/doctor/updates.rs b/codex-rs/cli/src/doctor/updates.rs index 08d0d3d276..fbad7d91ce 100644 --- a/codex-rs/cli/src/doctor/updates.rs +++ b/codex-rs/cli/src/doctor/updates.rs @@ -1,25 +1,19 @@ //! Diagnoses whether Codex update paths target the running installation. //! //! Update diagnostics combine cached version metadata, install-channel hints, -//! and bounded latest-version probes. For npm-managed launches, this module also -//! verifies that npm install -g would update the package root that launched the -//! current process, which catches PATH and prefix mismatches before the user runs -//! an update command. +//! and bounded latest-version HTTP probes. It never executes package managers or +//! other helpers selected by PATH; npm update targets are not verified. use std::path::Path; #[cfg(target_os = "macos")] use std::path::PathBuf; -#[cfg(any(target_os = "macos", target_os = "windows"))] use std::time::Duration; use codex_core::config::Config; -#[cfg(any(target_os = "macos", target_os = "windows"))] use codex_http_client::ClientRouteClass; -#[cfg(any(target_os = "macos", target_os = "windows"))] use codex_http_client::RouteAwareClientPool; use codex_install_context::InstallContext; use codex_install_context::InstallMethod; -#[cfg(any(target_os = "macos", target_os = "windows"))] use http::Method; use serde::Deserialize; #[cfg(target_os = "macos")] @@ -29,15 +23,14 @@ use super::CheckStatus; use super::DoctorCheck; #[cfg(any(target_os = "macos", target_os = "windows"))] use super::DoctorIssue; -use super::NpmRootCheck; #[cfg(any(target_os = "macos", target_os = "windows"))] use super::desktop::platform::InstalledApp; use super::doctor_install_context; use super::doctor_managed_by_npm; #[cfg(any(target_os = "macos", target_os = "windows"))] use super::network; -use super::npm_global_root_check; -use super::run_command; + +const MAX_VERSION_RESPONSE_BYTES: usize = 1024 * 1024; const VERSION_FILE_NAME: &str = "version.json"; const GITHUB_LATEST_RELEASE_URL: &str = "https://api.github.com/repos/openai/codex/releases/latest"; @@ -57,7 +50,7 @@ const DESKTOP_UPDATE_URL: &str = /// Network failures while fetching latest-version metadata degrade the row to a /// warning instead of failing doctor outright; update freshness is useful /// support context but should not mask more direct install/config failures. -pub(super) fn updates_check(config: &Config) -> DoctorCheck { +pub(super) async fn updates_check(config: &Config) -> DoctorCheck { let current_exe = std::env::current_exe().ok(); let install_context = doctor_install_context(current_exe.as_deref()); let mut details = vec![ @@ -71,48 +64,18 @@ pub(super) fn updates_check(config: &Config) -> DoctorCheck { push_cached_version_details(&mut details, &version_file); let mut status = CheckStatus::Ok; - let mut summary = "update configuration is locally consistent".to_string(); - let mut remediation = None; + let summary = "update configuration is locally consistent".to_string(); if doctor_managed_by_npm(current_exe.as_deref()) { - match npm_global_root_check() { - NpmRootCheck::Match { package_root } => { - details.push(format!("npm update target: {}", package_root.display())); - } - NpmRootCheck::Mismatch { - running_package_root, - npm_package_root, - } => { - status = CheckStatus::Fail; - summary = "update would target a different npm install".to_string(); - details.push(format!( - "running package root: {}", - running_package_root.display() - )); - details.push(format!("npm package root: {}", npm_package_root.display())); - remediation = Some(format!( - "Fix PATH or npm prefix so the running package root ({}) matches the npm global package root ({}).", - running_package_root.display(), - npm_package_root.display() - )); - } - NpmRootCheck::MissingPackageRoot => { - status = status.max(CheckStatus::Warning); - summary = "npm update target could not be proven".to_string(); - remediation = Some( - "Reinstall or update Codex so the JS shim provides CODEX_MANAGED_PACKAGE_ROOT." - .to_string(), - ); - } - NpmRootCheck::NpmUnavailable(error) => { - status = status.max(CheckStatus::Warning); - summary = "npm update target could not be inspected".to_string(); - details.push(format!("npm root -g failed: {error}")); - } - } + details + .push("npm update target: not inspected (PATH helpers are not executed)".to_string()); } + let client = RouteAwareClientPool::new_without_request_logging( + config.http_client_factory(), + ClientRouteClass::Other, + ); - match fetch_latest_version(&install_context) { + match fetch_latest_version(&client, &install_context).await { Ok(latest_version) => { details.push(format!("latest version: {latest_version}")); if is_newer(&latest_version, env!("CARGO_PKG_VERSION")) == Some(true) { @@ -127,11 +90,7 @@ pub(super) fn updates_check(config: &Config) -> DoctorCheck { } } - let mut check = DoctorCheck::new("updates.status", "updates", status, summary).details(details); - if let Some(remediation) = remediation { - check = check.remediation(remediation); - } - check + DoctorCheck::new("updates.status", "updates", status, summary).details(details) } #[cfg(any(target_os = "macos", target_os = "windows"))] @@ -440,46 +399,75 @@ fn update_action_label(context: &InstallContext) -> &'static str { } } -fn fetch_latest_version(context: &InstallContext) -> Result { +async fn fetch_latest_version( + client: &RouteAwareClientPool, + context: &InstallContext, +) -> Result { match &context.method { - InstallMethod::Brew => fetch_homebrew_cask_version(), + InstallMethod::Brew => fetch_homebrew_cask_version(client).await, InstallMethod::Npm | InstallMethod::Bun | InstallMethod::VitePlus | InstallMethod::Pnpm | InstallMethod::Standalone { .. } - | InstallMethod::Other => fetch_latest_github_release_version(), + | InstallMethod::Other => fetch_latest_github_release_version(client).await, } } -fn fetch_latest_github_release_version() -> Result { +async fn fetch_latest_github_release_version( + client: &RouteAwareClientPool, +) -> Result { #[derive(Deserialize)] struct ReleaseInfo { tag_name: String, } - let info = http_get_json::(GITHUB_LATEST_RELEASE_URL)?; + let info = http_get_json::(client, GITHUB_LATEST_RELEASE_URL).await?; info.tag_name .strip_prefix("rust-v") .map(str::to_string) .ok_or_else(|| format!("failed to parse latest tag {}", info.tag_name)) } -fn fetch_homebrew_cask_version() -> Result { +async fn fetch_homebrew_cask_version(client: &RouteAwareClientPool) -> Result { #[derive(Deserialize)] struct HomebrewCaskInfo { version: String, } - http_get_json::(HOMEBREW_CASK_API_URL).map(|info| info.version) + http_get_json::(client, HOMEBREW_CASK_API_URL) + .await + .map(|info| info.version) } -fn http_get_json(url: &str) -> Result +async fn http_get_json(client: &RouteAwareClientPool, url: &str) -> Result where T: for<'de> Deserialize<'de>, { - let body = run_command("curl", ["-fsSL", "--max-time", "5", url])?; - serde_json::from_str::(&body).map_err(|err| err.to_string()) + tokio::time::timeout(Duration::from_secs(/*secs*/ 5), async { + let mut response = client + .request(Method::GET, url) + .header( + http::header::USER_AGENT, + concat!("codex-doctor/", env!("CARGO_PKG_VERSION")), + ) + .send() + .await + .map_err(|err| err.to_string())?; + if !response.status().is_success() { + return Err(format!("HTTP {}", response.status())); + } + let mut body = Vec::new(); + while let Some(chunk) = response.chunk().await.map_err(|err| err.to_string())? { + if chunk.len() > MAX_VERSION_RESPONSE_BYTES.saturating_sub(body.len()) { + return Err("version response exceeds size limit".to_string()); + } + body.extend_from_slice(&chunk); + } + serde_json::from_slice(&body).map_err(|err| err.to_string()) + }) + .await + .map_err(|_| "version request timed out".to_string())? } fn is_newer(latest: &str, current: &str) -> Option { @@ -509,6 +497,74 @@ struct VersionInfo { #[cfg(test)] mod tests { use super::*; + use pretty_assertions::assert_eq; + + #[tokio::test] + async fn version_http_probe_decodes_json_and_rejects_invalid_responses() { + use codex_http_client::HttpClientFactory; + use codex_http_client::OutboundProxyPolicy; + use wiremock::Mock; + use wiremock::MockServer; + use wiremock::ResponseTemplate; + use wiremock::matchers::method; + use wiremock::matchers::path; + + let server = MockServer::start().await; + let client = RouteAwareClientPool::new_without_request_logging( + HttpClientFactory::new(OutboundProxyPolicy::ReqwestDefault), + ClientRouteClass::Other, + ); + for (endpoint, response) in [ + ( + "valid", + ResponseTemplate::new(/*s*/ 200) + .set_body_json(serde_json::json!({"version": "1.2.3"})), + ), + ( + "invalid", + ResponseTemplate::new(/*s*/ 200).set_body_string("not JSON"), + ), + ("unavailable", ResponseTemplate::new(/*s*/ 503)), + ("proxy_auth_required", ResponseTemplate::new(/*s*/ 407)), + ( + "redirect", + ResponseTemplate::new(/*s*/ 302) + .insert_header("Location", format!("{}/valid", server.uri())), + ), + ( + "timeout", + ResponseTemplate::new(/*s*/ 200).set_delay(Duration::from_secs(/*secs*/ 6)), + ), + ( + "oversized", + ResponseTemplate::new(/*s*/ 200) + .set_body_bytes(vec![b' '; MAX_VERSION_RESPONSE_BYTES + 1]), + ), + ] { + Mock::given(method("GET")) + .and(path(format!("/{endpoint}"))) + .respond_with(response) + .mount(&server) + .await; + let result = http_get_json::( + &client, + &format!("{}/{endpoint}", server.uri()), + ) + .await; + if matches!(endpoint, "valid" | "redirect") { + assert_eq!(result, Ok(serde_json::json!({"version": "1.2.3"}))); + } else if endpoint == "timeout" { + assert_eq!(result, Err("version request timed out".to_string())); + } else if endpoint == "proxy_auth_required" { + assert_eq!( + result, + Err("HTTP 407 Proxy Authentication Required".to_string()) + ); + } else { + assert!(result.is_err(), "{endpoint} must not be accepted"); + } + } + } #[cfg(target_os = "macos")] #[test] diff --git a/codex-rs/cli/tests/doctor_path_safety.rs b/codex-rs/cli/tests/doctor_path_safety.rs new file mode 100644 index 0000000000..cce1addf42 --- /dev/null +++ b/codex-rs/cli/tests/doctor_path_safety.rs @@ -0,0 +1,334 @@ +//! Black-box coverage for diagnostics under a repository-controlled PATH. + +use std::ffi::OsString; +use std::path::PathBuf; +use std::time::Duration; + +use anyhow::Result; +use app_test_support::TestAppServer; +use codex_app_server_protocol::RequestId; +use pretty_assertions::assert_eq; +use serde_json::Value; +use serde_json::json; +use tempfile::TempDir; +use tokio::io::AsyncBufReadExt; +use tokio::io::AsyncWriteExt; +use tokio::io::BufReader; +use tokio::net::TcpListener; +use tokio::time::timeout; + +struct Fixture { + root: TempDir, + program: PathBuf, + workspace: PathBuf, + home: PathBuf, + marker: PathBuf, + path: OsString, +} + +impl Fixture { + fn new() -> Result { + let root = TempDir::new()?; + // Cargo-built paths deliberately ignore npm provenance. Launch outside + // target/ so this fixture also exercises the packaged-install checks. + let program = root + .path() + .join(format!("codex{}", std::env::consts::EXE_SUFFIX)); + let source = codex_utils_cargo_bin::cargo_bin("codex")?; + if std::fs::hard_link(&source, &program).is_err() { + std::fs::copy(&source, &program)?; + } + let workspace = root.path().join("workspace"); + let bin = workspace.join("node_modules/.bin"); + let home = root.path().join("home"); + let marker = root.path().join("helper-ran"); + std::fs::create_dir_all(&bin)?; + std::fs::create_dir_all(workspace.join(".git"))?; + std::fs::create_dir(&home)?; + std::fs::write( + home.join("config.toml"), + r#" +cli_auth_credentials_store = "file" +check_for_update_on_startup = false +model_provider = "local" +[analytics] +enabled = false +[model_providers.local] +name = "local test" +base_url = "http://127.0.0.1:9/v1" +wire_api = "responses" +"#, + )?; + for name in [ + "zellij", "tmux", "which", "where", "npm", "rg", "git", "curl", "cmd.exe", + ] { + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let executable = bin.join(name); + std::fs::write( + &executable, + "#!/bin/sh\nprintf 'helper ran\\n' >> \"$CODEX_TEST_HELPER_MARKER\"\nexit 0\n", + )?; + std::fs::set_permissions( + executable, + std::fs::Permissions::from_mode(/*mode*/ 0o755), + )?; + } + #[cfg(windows)] + std::fs::write( + bin.join(format!("{name}.cmd")), + "@echo helper ran>>\"%CODEX_TEST_HELPER_MARKER%\"\r\n@exit /b 0\r\n", + )?; + } + // Windows selects rg.exe explicitly, so rg.cmd cannot satisfy discovery. + // An invalid image still counts as found: doctor must not execute it. + #[cfg(windows)] + std::fs::write(bin.join("rg.exe"), "not an executable image")?; + let path = std::env::join_paths(std::iter::once(bin).chain(std::env::split_paths( + &std::env::var_os("PATH").unwrap_or_default(), + )))?; + Ok(Self { + root, + program, + workspace, + home, + marker, + path, + }) + } + + fn command(&self) -> Result { + let mut command = assert_cmd::Command::new(&self.program); + command + .current_dir(&self.workspace) + .env("CODEX_HOME", &self.home) + .env("HOME", self.root.path()) + .env("PATH", &self.path) + .env("CODEX_TEST_HELPER_MARKER", &self.marker) + .env("CODEX_MANAGED_BY_NPM", "1") + .env( + "CODEX_MANAGED_PACKAGE_ROOT", + self.workspace.join("node_modules/@openai/codex"), + ) + .env( + "CODEX_APP_SERVER_MANAGED_CONFIG_PATH", + self.home.join("managed_config.toml"), + ) + .env("HTTPS_PROXY", "http://127.0.0.1:9") + .env("HTTP_PROXY", "http://127.0.0.1:9") + .env("ALL_PROXY", "http://127.0.0.1:9") + .env("https_proxy", "http://127.0.0.1:9") + .env("http_proxy", "http://127.0.0.1:9") + .env("all_proxy", "http://127.0.0.1:9") + .env("NO_PROXY", "127.0.0.1,localhost") + .env("no_proxy", "127.0.0.1,localhost") + .env_remove("TMUX") + .env_remove("TMUX_PANE") + .env_remove("ZELLIJ_VERSION") + .env_remove("ZELLIJ_SESSION_NAME") + .env_remove("TERM_PROGRAM") + .env("TERM", "dumb") + .env("ZELLIJ", "0") + .timeout(Duration::from_secs(/*secs*/ 45)); + Ok(command) + } +} + +#[test] +fn startup_and_doctor_do_not_execute_path_helpers() -> Result<()> { + let fixture = Fixture::new()?; + // Non-TTY dumb-terminal startup exits immediately after terminal detection. + fixture.command()?.assert().failure(); + assert!(!fixture.marker.exists(), "startup executed a PATH helper"); + + for args in [ + vec!["doctor", "--json"], + vec!["doctor", "--json", "--feedback"], + ] { + let mut command = fixture.command()?; + command + .args(args) + .env("TMUX", "test-tmux") + .env("TERM_PROGRAM", "tmux"); + let output = command.output()?; + let report: Value = serde_json::from_slice(&output.stdout)?; + assert_eq!( + report["checks"]["installation"]["details"]["managed by npm"], + "true" + ); + assert_eq!(report["checks"]["runtime.search"]["status"], "ok"); + assert_eq!( + report["checks"]["runtime.search"]["details"]["search command path"], + fixture + .workspace + .join("node_modules/.bin") + .join(format!("rg{}", std::env::consts::EXE_SUFFIX)) + .display() + .to_string() + ); + assert_eq!( + report["checks"]["git.environment"]["summary"], + "git executable found; execution not verified" + ); + assert!(!fixture.marker.exists(), "doctor executed a PATH helper"); + } + Ok(()) +} + +#[cfg(unix)] +#[tokio::test] +async fn interactive_tmux_startup_does_not_execute_workspace_helpers() -> Result<()> { + let fixture = Fixture::new()?; + let command = fixture.command()?; + let mut env: std::collections::HashMap = std::env::vars().collect(); + for (key, value) in command.get_envs() { + let key = key.to_string_lossy().into_owned(); + if let Some(value) = value { + env.insert(key, value.to_string_lossy().into_owned()); + } else { + env.remove(&key); + } + } + env.insert("TERM".to_string(), "xterm-256color".to_string()); + env.insert("TERM_PROGRAM".to_string(), "tmux".to_string()); + env.insert("TMUX".to_string(), "test-tmux".to_string()); + env.insert( + "CODEX_TUI_DISABLE_KEYBOARD_ENHANCEMENT".to_string(), + "0".to_string(), + ); + env.insert( + "GHOSTTY_RESOURCES_DIR".to_string(), + "/test/ghostty".to_string(), + ); + let spawned = codex_utils_pty::spawn_pty_process( + fixture.program.to_str().unwrap(), + &[], + &fixture.workspace, + &env, + /*arg0*/ &None, + codex_utils_pty::TerminalSize { + rows: 40, + cols: 120, + }, + &[], + ) + .await?; + let session = spawned.session; + let mut output_rx = spawned.stdout_rx; + let writer = session.writer_sender(); + let mut output = String::new(); + let ansi = regex_lite::Regex::new(r"\x1b\[[0-?]*[ -/]*[@-~]")?; + let ready = timeout(Duration::from_secs(/*secs*/ 45), async { + while let Some(bytes) = output_rx.recv().await { + let chunk = String::from_utf8_lossy(&bytes); + output.push_str(&chunk); + if chunk.contains("\x1b[6n") { + writer.send(b"\x1b[1;1R".to_vec()).await?; + } + if chunk.contains("\x1b[c") { + writer.send(b"\x1b[?1;2c".to_vec()).await?; + } + // Ratatui can position over spaces instead of writing them. + let text: String = ansi + .replace_all(&output, "") + .chars() + .filter(|character| !character.is_whitespace()) + .collect(); + if text.contains("Doyoutrustthecontentsofthisdirectory?") { + return Ok::<_, anyhow::Error>(()); + } + } + anyhow::bail!("TUI exited before trust prompt") + }) + .await; + session.terminate(); + let _ = timeout(Duration::from_secs(/*secs*/ 10), spawned.exit_rx).await?; + ready.map_err(|_| { + anyhow::anyhow!( + "trust prompt timed out: {}", + output.chars().take(/*n*/ 4000).collect::() + ) + })??; + assert!( + output.contains("\x1b[>5u"), + "expected safe Ghostty/tmux keyboard flags: {output}" + ); + assert!( + !fixture.marker.exists(), + "interactive startup executed a workspace helper" + ); + Ok(()) +} + +#[tokio::test] +async fn feedback_with_logs_does_not_execute_path_helpers() -> Result<()> { + let fixture = Fixture::new()?; + let proxy = TcpListener::bind("127.0.0.1:0").await?; + let proxy_uri = format!("http://{}", proxy.local_addr()?); + // Reject all outbound traffic locally, including the final feedback upload. + let proxy_task = tokio::spawn(async move { + while let Ok((stream, _)) = proxy.accept().await { + let mut stream = BufReader::new(stream); + let mut request = String::new(); + if stream.read_line(&mut request).await.is_ok() { + let _ = stream.get_mut().write_all(b"HTTP/1.1 503 Service Unavailable\r\nContent-Length: 0\r\nConnection: close\r\n\r\n").await; + } + } + }); + let path = fixture.path.to_string_lossy(); + let marker = fixture.marker.to_string_lossy(); + let home = fixture.root.path().to_string_lossy(); + let package_root = fixture.workspace.join("node_modules/@openai/codex"); + let package_root = package_root.to_string_lossy(); + let mut app_server = TestAppServer::builder() + .with_program(&fixture.program) + .with_codex_home(&fixture.home) + // The CLI does not accept the standalone app-server's test-only flag. + // Disable plugins through real config; their safety test joins a full sync. + .with_plugin_startup_tasks() + .with_args(&["-c", "features.plugins=false", "app-server"]) + .with_env_overrides(&[ + ("PATH", Some(path.as_ref())), + ("HOME", Some(home.as_ref())), + ("CODEX_TEST_HELPER_MARKER", Some(marker.as_ref())), + ("CODEX_MANAGED_BY_NPM", Some("1")), + ("CODEX_MANAGED_PACKAGE_ROOT", Some(package_root.as_ref())), + ("ZELLIJ", Some("0")), + ("ZELLIJ_VERSION", None), + ("TMUX", None), + ("TMUX_PANE", None), + ("HTTP_PROXY", Some(&proxy_uri)), + ("HTTPS_PROXY", Some(&proxy_uri)), + ("ALL_PROXY", Some(&proxy_uri)), + ("http_proxy", Some(&proxy_uri)), + ("https_proxy", Some(&proxy_uri)), + ("all_proxy", Some(&proxy_uri)), + ("NO_PROXY", Some("127.0.0.1,localhost")), + ("no_proxy", Some("127.0.0.1,localhost")), + ]) + .build_initialized_with_timeout(Duration::from_secs(/*secs*/ 30)) + .await?; + let request_id = app_server + .send_raw_request( + "feedback/upload", + Some(json!({ + "classification": "bug", "includeLogs": true + })), + ) + .await?; + let error = timeout( + Duration::from_secs(/*secs*/ 45), + app_server.read_stream_until_error_message(RequestId::Integer(request_id)), + ) + .await??; + assert!(error.error.message.contains("failed to upload feedback")); + assert!(!fixture.marker.exists(), "feedback executed a PATH helper"); + timeout( + Duration::from_secs(/*secs*/ 10), + app_server.shutdown_gracefully(), + ) + .await??; + proxy_task.abort(); + Ok(()) +} diff --git a/codex-rs/core-plugins/Cargo.toml b/codex-rs/core-plugins/Cargo.toml index 0899163acc..f5e5160707 100644 --- a/codex-rs/core-plugins/Cargo.toml +++ b/codex-rs/core-plugins/Cargo.toml @@ -57,9 +57,6 @@ url = { workspace = true } uuid = { workspace = true, features = ["v4"] } zip = { workspace = true } -[target.'cfg(target_os = "macos")'.dependencies] -which = { workspace = true } - [dev-dependencies] codex-exec-server-test-support = { workspace = true } libc = { workspace = true } diff --git a/codex-rs/core-plugins/src/startup_sync.rs b/codex-rs/core-plugins/src/startup_sync.rs index fe0e5de88b..dd7eff45d9 100644 --- a/codex-rs/core-plugins/src/startup_sync.rs +++ b/codex-rs/core-plugins/src/startup_sync.rs @@ -78,14 +78,12 @@ pub fn sync_openai_plugins_repo( codex_home: &Path, http_client_factory: HttpClientFactory, ) -> Result { + // Keep Git-only egress working without trusting workspace PATH entries. + let git_binary = codex_utils_path::system_executable("git"); + // Apple's /usr/bin/git is an installer shim when developer tools are absent. + // The resolver prefers the real CLT/Xcode executable when installed. #[cfg(target_os = "macos")] - let git_binary = match which::which("git") { - Ok(git_path) => macos_git_binary_from_path(git_path, apple_developer_tools_available()), - Err(_) => None, - }; - #[cfg(not(target_os = "macos"))] - let git_binary = Some(PathBuf::from("git")); - + let git_binary = git_binary.filter(|path| path != Path::new("/usr/bin/git")); sync_openai_plugins_repo_with_transport_overrides( codex_home, git_binary.as_deref(), @@ -106,7 +104,7 @@ fn sync_openai_plugins_repo_with_transport_overrides( let git_sync_result = match git_binary { Some(git_binary) => sync_openai_plugins_repo_via_git(codex_home, git_binary), - None => Err("git executable is unavailable".to_string()), + None => Err("no Git executable found in trusted installation directories".to_string()), }; match git_sync_result { @@ -116,11 +114,13 @@ fn sync_openai_plugins_repo_with_transport_overrides( Ok(remote_sha) } Err(err) => { - emit_curated_plugins_startup_sync_metric("git", "failure"); - warn!( - error = %err, - "git sync failed for curated plugin sync; falling back to GitHub HTTP" - ); + if git_binary.is_some() { + emit_curated_plugins_startup_sync_metric("git", "failure"); + warn!( + error = %err, + "git sync failed for curated plugin sync; falling back to GitHub HTTP" + ); + } match sync_openai_plugins_repo_via_http(codex_home, api_base_url, http_client_factory) { Ok(remote_sha) => { emit_curated_plugins_startup_sync_metric("http", "success"); @@ -266,7 +266,7 @@ fn fetch_curated_plugins_commit_from( context: &str, ) -> Result<(), String> { let fetch_refspec = format!("+{source_revision}:{CURATED_PLUGINS_FETCH_REF}"); - let mut command = git_command(git_binary); + let mut command = git_command(git_binary)?; command .arg("-C") .arg(repo_path) @@ -298,7 +298,7 @@ fn run_git_in_repo( args: &[&str], context: &str, ) -> Result<(), String> { - let mut command = git_command(git_binary); + let mut command = git_command(git_binary)?; command.arg("-C").arg(repo_path).args(args); let output = run_git_command_with_timeout(&mut command, context, CURATED_PLUGINS_GIT_TIMEOUT)?; ensure_git_success(&output, context) @@ -608,9 +608,10 @@ fn read_local_git_or_sha_file( } fn git_ls_remote_head_sha(codex_home: &Path, git_binary: &Path) -> Result { - let mut command = git_command(git_binary); + let mut command = git_command(git_binary)?; let _trusted_repository = crate::configure_trusted_git_repository(&mut command, codex_home)?; command + .current_dir(codex_home) .arg("ls-remote") .arg(OPENAI_PLUGINS_GIT_URL) .arg("HEAD"); @@ -637,7 +638,7 @@ fn git_ls_remote_head_sha(codex_home: &Path, git_binary: &Path) -> Result Result { - let output = git_command(git_binary) + let output = git_command(git_binary)? .arg("-C") .arg(repo_path) .arg("rev-parse") @@ -661,31 +662,22 @@ fn git_head_sha(repo_path: &Path, git_binary: &Path) -> Result { Ok(sha) } -fn git_command(git_binary: &Path) -> Command { - crate::PluginGitMode::Automatic.command(git_binary) -} - -#[cfg(any(target_os = "macos", test))] -fn macos_git_binary_from_path( - git_path: PathBuf, - apple_developer_tools_available: bool, -) -> Option { - if git_path == Path::new("/usr/bin/git") && !apple_developer_tools_available { - None - } else { - Some(git_path) - } -} - -#[cfg(target_os = "macos")] -fn apple_developer_tools_available() -> bool { - Command::new("/usr/bin/xcode-select") - .arg("-p") - .stdin(Stdio::null()) - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .status() - .is_ok_and(|status| status.success()) +fn git_command(git_binary: &Path) -> Result { + let mut command = crate::PluginGitMode::Automatic.command(git_binary); + // Git launches transports and credential helpers too. Selecting a trusted + // main executable alone does not prevent a workspace PATH helper from running. + command + .env( + "PATH", + codex_utils_path::system_path() + .map_err(|err| format!("failed to construct trusted Git PATH: {err}"))?, + ) + .env_remove("GIT_EXEC_PATH") + .env_remove("GIT_TEMPLATE_DIR") + .env_remove("DEVELOPER_DIR") + .env("GIT_TERMINAL_PROMPT", "0") + .env("NoDefaultCurrentDirectoryInExePath", "1"); + Ok(command) } fn run_git_command_with_timeout( diff --git a/codex-rs/core-plugins/src/startup_sync_tests.rs b/codex-rs/core-plugins/src/startup_sync_tests.rs index 8d272c95a0..de3e306de1 100644 --- a/codex-rs/core-plugins/src/startup_sync_tests.rs +++ b/codex-rs/core-plugins/src/startup_sync_tests.rs @@ -21,6 +21,106 @@ use zip::write::SimpleFileOptions; const TEST_CURATED_PLUGIN_SHA: &str = "0123456789abcdef0123456789abcdef01234567"; +#[cfg(unix)] +#[test] +fn pretrust_startup_sync_uses_installed_git_with_hostile_path() { + const CHILD_HOME: &str = "CODEX_TEST_CURATED_SYNC_HOME"; + if let Some(home) = std::env::var_os(CHILD_HOME) { + // Call the public entry point, synchronously. The parent joins this + // process before checking the marker, so no detached work can escape it. + sync_openai_plugins_repo( + Path::new(&home), + crate::test_support::test_http_client_factory(), + ) + .expect("Git-only startup sync should succeed"); + return; + } + let fixture = tempdir().expect("fixture"); + let source = fixture.path().join("remote"); + let home = fixture.path().join("home"); + let workspace = fixture.path().join("workspace"); + let bin = workspace.join("node_modules/.bin"); + let marker = fixture.path().join("helper-ran"); + std::fs::create_dir_all(&source).unwrap(); + std::fs::create_dir_all(&bin).unwrap(); + std::fs::create_dir(&home).unwrap(); + run_git(&source, &["init", "--quiet"]); + for name in ["git", "git-upload-pack", "git-remote-https"] { + write_executable_script( + &bin.join(name), + &format!("#!/bin/sh\nprintf ran > '{}'\nexit 1\n", marker.display()), + ); + } + let config = fixture.path().join("gitconfig"); + std::fs::write( + &config, + format!( + "[url \"file://{}\"]\n insteadOf = {OPENAI_PLUGINS_GIT_URL}\n", + source.display() + ), + ) + .unwrap(); + + for plugins in [vec!["gmail"], vec!["gmail", "linear"]] { + write_openai_curated_marketplace(&source, &plugins); + run_git(&source, &["add", "."]); + run_git( + &source, + &[ + "-c", + "user.name=Test", + "-c", + "user.email=test@example.com", + "commit", + "-qm", + "update", + ], + ); + let expected_sha = run_git(&source, &["rev-parse", "HEAD"]); + let output = Command::new(std::env::current_exe().unwrap()) + .args([ + "--exact", + "startup_sync::tests::pretrust_startup_sync_uses_installed_git_with_hostile_path", + "--nocapture", + ]) + .current_dir(&workspace) + .env(CHILD_HOME, &home) + .env("PATH", &bin) + .env("GIT_EXEC_PATH", &bin) + .env("GIT_CONFIG_GLOBAL", &config) + .env("GIT_CONFIG_SYSTEM", "/dev/null") + .env("HTTP_PROXY", "http://127.0.0.1:9") + .env("HTTPS_PROXY", "http://127.0.0.1:9") + .env("http_proxy", "http://127.0.0.1:9") + .env("https_proxy", "http://127.0.0.1:9") + .output() + .expect("join sync subprocess"); + assert!( + output.status.success(), + "{}\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + assert_eq!( + read_curated_plugins_sha(&home), + Some( + String::from_utf8(expected_sha.stdout) + .unwrap() + .trim() + .to_string() + ) + ); + assert!(!marker.exists(), "sync executed a workspace helper"); + for plugin in plugins { + assert!( + curated_plugins_repo_path(&home) + .join(format!("plugins/{plugin}/.codex-plugin/plugin.json")) + .is_file() + ); + } + } +} + #[tokio::test] async fn github_http_routes_repository_ref_and_zipball_urls() { let server = MockServer::start().await; @@ -93,7 +193,7 @@ async fn backup_archive_routes_metadata_and_backend_supplied_download_urls() { #[test] fn git_command_sanitizes_ambient_repository_environment() { - let command = git_command(Path::new("git")); + let command = git_command(Path::new("git")).expect("trusted Git command"); assert_eq!( command.get_args().collect::>(), @@ -260,6 +360,7 @@ async fn ordinary_clone_rejects_tracked_embedded_bare_repository() { std::fs::remove_file(&marker).expect("remove unguarded execution marker"); let guarded = git_command(Path::new("git")) + .expect("trusted Git command") .args(["status", "--short"]) .current_dir(&nested) .output() @@ -289,6 +390,7 @@ async fn ordinary_clone_rejects_tracked_embedded_bare_repository() { ); let explicit_git_dir = git_command(Path::new("git")) + .expect("trusted Git command") .arg("--git-dir") .arg(&nested) .args(["rev-parse", "--git-dir"]) @@ -936,31 +1038,6 @@ async fn sync_openai_plugins_repo_falls_back_to_http_when_git_is_unavailable() { assert_eq!(read_curated_plugins_sha(tmp.path()).as_deref(), Some(sha)); } -#[test] -fn apple_git_without_developer_tools_is_unavailable() { - assert_eq!( - macos_git_binary_from_path( - PathBuf::from("/usr/bin/git"), - /*apple_developer_tools_available*/ false, - ), - None - ); - assert_eq!( - macos_git_binary_from_path( - PathBuf::from("/usr/bin/git"), - /*apple_developer_tools_available*/ true, - ), - Some(PathBuf::from("/usr/bin/git")) - ); - assert_eq!( - macos_git_binary_from_path( - PathBuf::from("/opt/homebrew/bin/git"), - /*apple_developer_tools_available*/ false, - ), - Some(PathBuf::from("/opt/homebrew/bin/git")) - ); -} - #[tokio::test] async fn sync_openai_plugins_repo_uses_http_without_git_transport() { let tmp = tempdir().expect("tempdir"); diff --git a/codex-rs/terminal-detection/src/lib.rs b/codex-rs/terminal-detection/src/lib.rs index 5e9b197138..2c172ee778 100644 --- a/codex-rs/terminal-detection/src/lib.rs +++ b/codex-rs/terminal-detection/src/lib.rs @@ -1,7 +1,8 @@ //! Terminal detection utilities. //! //! This module feeds terminal metadata into OpenTelemetry user-agent logging and into -//! terminal-specific configuration choices in the TUI. +//! terminal-specific configuration choices in the TUI. Detection only reads the +//! environment; it must not execute helpers selected by an untrusted PATH. use std::sync::OnceLock; @@ -70,22 +71,6 @@ pub enum Multiplexer { }, } -/// tmux client terminal identification captured via `tmux display-message`. -/// -/// `termtype` corresponds to `#{client_termtype}` and typically reflects the -/// underlying terminal program (for example, `ghostty` or `wezterm`) with an -/// optional version suffix. `termname` comes from `#{client_termname}` and -/// preserves the TERM capability string exposed by the client (for example, -/// `xterm-256color`). -/// -/// This information is only available when running under tmux and lets us -/// attribute the session to the underlying terminal rather than to tmux itself. -#[derive(Clone, Debug, Default, Eq, PartialEq)] -struct TmuxClientInfo { - termtype: Option, - termname: Option, -} - impl TerminalInfo { /// Creates terminal metadata from detected fields. fn new( @@ -120,17 +105,6 @@ impl TerminalInfo { ) } - /// Creates terminal metadata from a `TERM_PROGRAM` match plus a `TERM` value. - fn from_term_program_and_term( - name: TerminalName, - term_program: String, - version: Option, - term: Option, - multiplexer: Option, - ) -> Self { - Self::new(name, Some(term_program), version, term, multiplexer) - } - /// Creates terminal metadata from a known terminal name and optional version. fn from_name( name: TerminalName, @@ -238,14 +212,6 @@ trait Environment { fn has_non_empty(&self, name: &str) -> bool { self.var_non_empty(name).is_some() } - - /// Returns tmux client details when available. - fn tmux_client_info(&self) -> TmuxClientInfo; - - /// Returns Zellij version details when available. - fn zellij_version(&self) -> Option { - self.var_non_empty("ZELLIJ_VERSION") - } } /// Reads environment variables from the running process. @@ -262,15 +228,6 @@ impl Environment for ProcessEnvironment { } } } - - fn tmux_client_info(&self) -> TmuxClientInfo { - tmux_client_info() - } - - fn zellij_version(&self) -> Option { - self.var_non_empty("ZELLIJ_VERSION") - .or_else(zellij_version_from_command) - } } /// Returns a sanitized terminal identifier for User-Agent strings. @@ -288,34 +245,26 @@ pub fn terminal_info() -> TerminalInfo { /// Detects structured terminal metadata from an injectable environment. /// /// Detection order favors explicit identifiers before falling back to capability strings: -/// - If `TERM_PROGRAM=tmux`, the tmux client term type/name are used instead. The client term -/// type is split on whitespace to extract a program name plus optional version (for example, -/// `ghostty 1.2.3`), while the client term name becomes the `TERM` capability string. -/// - Otherwise, `TERM_PROGRAM` (plus `TERM_PROGRAM_VERSION`) drives the detected terminal name. +/// - `TERM_PROGRAM` (plus `TERM_PROGRAM_VERSION`) drives the detected terminal name, +/// except when it identifies tmux rather than the underlying terminal. /// This means `TERM_PROGRAM` can mask later probes (for example `WT_SESSION`). /// - Next, terminal-specific variables (WEZTERM, iTerm2, Apple Terminal, kitty, etc.) are checked. /// - Finally, `TERM` is used as the capability fallback. -/// -/// tmux client term info is only consulted when a tmux multiplexer is detected, and it is -/// derived from `tmux display-message` to surface the underlying terminal program instead of -/// reporting tmux itself. fn detect_terminal_info_from_env(env: &dyn Environment) -> TerminalInfo { let multiplexer = detect_multiplexer(env); - if let Some(term_program) = env.var_non_empty("TERM_PROGRAM") { - if is_tmux_term_program(&term_program) - && matches!(multiplexer, Some(Multiplexer::Tmux { .. })) - && let Some(terminal) = - terminal_from_tmux_client_info(env.tmux_client_info(), multiplexer.clone()) - { - return terminal; - } - + if let Some(term_program) = env.var_non_empty("TERM_PROGRAM") + && !is_tmux_term_program(&term_program) + { let version = env.var_non_empty("TERM_PROGRAM_VERSION"); let name = terminal_name_from_term_program(&term_program).unwrap_or(TerminalName::Unknown); return TerminalInfo::from_term_program(name, term_program, version, multiplexer); } + if env.has_non_empty("GHOSTTY_RESOURCES_DIR") { + return TerminalInfo::from_name(TerminalName::Ghostty, /*version*/ None, multiplexer); + } + if env.has("WEZTERM_VERSION") { let version = env.var_non_empty("WEZTERM_VERSION"); return TerminalInfo::from_name(TerminalName::WezTerm, version, multiplexer); @@ -400,7 +349,7 @@ fn detect_multiplexer(env: &dyn Environment) -> Option { || env.has_non_empty("ZELLIJ_VERSION") { return Some(Multiplexer::Zellij { - version: env.zellij_version(), + version: env.var_non_empty("ZELLIJ_VERSION"), }); } @@ -411,30 +360,6 @@ fn is_tmux_term_program(value: &str) -> bool { value.eq_ignore_ascii_case("tmux") } -fn terminal_from_tmux_client_info( - client_info: TmuxClientInfo, - multiplexer: Option, -) -> Option { - let termtype = client_info.termtype.and_then(none_if_whitespace); - let termname = client_info.termname.and_then(none_if_whitespace); - - if let Some(termtype) = termtype.as_ref() { - let (program, version) = split_term_program_and_version(termtype); - let name = terminal_name_from_term_program(&program).unwrap_or(TerminalName::Unknown); - return Some(TerminalInfo::from_term_program_and_term( - name, - program, - version, - termname, - multiplexer, - )); - } - - termname - .as_ref() - .map(|termname| TerminalInfo::from_term(termname.to_string(), multiplexer)) -} - fn tmux_version_from_env(env: &dyn Environment) -> Option { let term_program = env.var("TERM_PROGRAM")?; if !is_tmux_term_program(&term_program) { @@ -444,60 +369,6 @@ fn tmux_version_from_env(env: &dyn Environment) -> Option { env.var_non_empty("TERM_PROGRAM_VERSION") } -fn split_term_program_and_version(value: &str) -> (String, Option) { - let mut parts = value.split_whitespace(); - let program = parts.next().unwrap_or_default().to_string(); - let version = parts.next().map(ToString::to_string); - (program, version) -} - -fn tmux_client_info() -> TmuxClientInfo { - let termtype = tmux_display_message("#{client_termtype}"); - let termname = tmux_display_message("#{client_termname}"); - - TmuxClientInfo { termtype, termname } -} - -fn tmux_display_message(format: &str) -> Option { - let output = std::process::Command::new("tmux") - .args(["display-message", "-p", format]) - .output() - .ok()?; - - if !output.status.success() { - return None; - } - - let value = String::from_utf8(output.stdout).ok()?; - none_if_whitespace(value.trim().to_string()) -} - -fn zellij_version_from_command() -> Option { - // Best-effort fallback: missing or broken zellij binaries should not affect - // terminal detection. - let output = std::process::Command::new("zellij") - .arg("--version") - .output() - .ok()?; - if !output.status.success() { - return None; - } - - let stdout = String::from_utf8(output.stdout).ok()?; - parse_zellij_version(stdout.trim()) -} - -fn parse_zellij_version(value: &str) -> Option { - let value = none_if_whitespace(value.to_string())?; - let mut parts = value.split_whitespace(); - match (parts.next(), parts.next()) { - (Some(command), Some(version)) if command.eq_ignore_ascii_case("zellij") => { - Some(version.to_string()) - } - _ => Some(value), - } -} - /// Sanitizes a terminal token for use in User-Agent headers. /// /// Invalid header characters are replaced with underscores. diff --git a/codex-rs/terminal-detection/src/terminal_tests.rs b/codex-rs/terminal-detection/src/terminal_tests.rs index 06f41090ab..b761a767cc 100644 --- a/codex-rs/terminal-detection/src/terminal_tests.rs +++ b/codex-rs/terminal-detection/src/terminal_tests.rs @@ -4,16 +4,12 @@ use std::collections::HashMap; struct FakeEnvironment { vars: HashMap, - tmux_client_info: TmuxClientInfo, - zellij_version: Option, } impl FakeEnvironment { fn new() -> Self { Self { vars: HashMap::new(), - tmux_client_info: TmuxClientInfo::default(), - zellij_version: None, } } @@ -21,35 +17,12 @@ impl FakeEnvironment { self.vars.insert(key.to_string(), value.to_string()); self } - - fn with_tmux_client_info(mut self, termtype: Option<&str>, termname: Option<&str>) -> Self { - self.tmux_client_info = TmuxClientInfo { - termtype: termtype.map(ToString::to_string), - termname: termname.map(ToString::to_string), - }; - self - } - - fn with_zellij_version(mut self, version: &str) -> Self { - self.zellij_version = Some(version.to_string()); - self - } } impl Environment for FakeEnvironment { fn var(&self, name: &str) -> Option { self.vars.get(name).cloned() } - - fn tmux_client_info(&self) -> TmuxClientInfo { - self.tmux_client_info.clone() - } - - fn zellij_version(&self) -> Option { - self.zellij_version - .clone() - .or_else(|| self.var_non_empty("ZELLIJ_VERSION")) - } } fn terminal_info( @@ -313,22 +286,24 @@ fn detects_tmux_multiplexer() { let env = FakeEnvironment::new() .with_var("TMUX", "/tmp/tmux-1000/default,123,0") .with_var("TERM_PROGRAM", "tmux") - .with_tmux_client_info(Some("xterm-256color"), Some("screen-256color")); + .with_var("TERM_PROGRAM_VERSION", "3.5a"); let terminal = detect_terminal_info_from_env(&env); assert_eq!( terminal, terminal_info( TerminalName::Unknown, - Some("xterm-256color"), + /*term_program*/ None, /*version*/ None, - Some("screen-256color"), - Some(Multiplexer::Tmux { version: None }), + /*term*/ None, + Some(Multiplexer::Tmux { + version: Some("3.5a".to_string()) + }), ), "tmux_multiplexer_info" ); assert_eq!( terminal.user_agent_token(), - "xterm-256color", + "unknown", "tmux_multiplexer_user_agent" ); } @@ -369,115 +344,6 @@ fn detects_zellij_multiplexer_version() { ); } -#[test] -fn detects_zellij_multiplexer_command_version() { - let env = FakeEnvironment::new() - .with_var("ZELLIJ", "1") - .with_zellij_version("0.44.1"); - let terminal = detect_terminal_info_from_env(&env); - assert_eq!( - terminal, - terminal_info( - TerminalName::Unknown, - /*term_program*/ None, - /*version*/ None, - /*term*/ None, - Some(Multiplexer::Zellij { - version: Some("0.44.1".to_string()), - }), - ), - "zellij_multiplexer_command_version" - ); -} - -#[test] -fn parses_zellij_version_output() { - assert_eq!( - parse_zellij_version("zellij 0.44.1"), - Some("0.44.1".to_string()) - ); - assert_eq!(parse_zellij_version("0.44.1"), Some("0.44.1".to_string())); - assert_eq!(parse_zellij_version(""), None); -} - -#[test] -fn detects_tmux_client_termtype() { - let env = FakeEnvironment::new() - .with_var("TMUX", "/tmp/tmux-1000/default,123,0") - .with_var("TERM_PROGRAM", "tmux") - .with_tmux_client_info(Some("WezTerm"), /*termname*/ None); - let terminal = detect_terminal_info_from_env(&env); - assert_eq!( - terminal, - terminal_info( - TerminalName::WezTerm, - Some("WezTerm"), - /*version*/ None, - /*term*/ None, - Some(Multiplexer::Tmux { version: None }), - ), - "tmux_client_termtype_info" - ); - assert_eq!( - terminal.user_agent_token(), - "WezTerm", - "tmux_client_termtype_user_agent" - ); -} - -#[test] -fn detects_tmux_client_termname() { - let env = FakeEnvironment::new() - .with_var("TMUX", "/tmp/tmux-1000/default,123,0") - .with_var("TERM_PROGRAM", "tmux") - .with_tmux_client_info(/*termtype*/ None, Some("xterm-256color")); - let terminal = detect_terminal_info_from_env(&env); - assert_eq!( - terminal, - terminal_info( - TerminalName::Unknown, - /*term_program*/ None, - /*version*/ None, - Some("xterm-256color"), - Some(Multiplexer::Tmux { version: None }) - ), - "tmux_client_termname_info" - ); - assert_eq!( - terminal.user_agent_token(), - "xterm-256color", - "tmux_client_termname_user_agent" - ); -} - -#[test] -fn detects_tmux_term_program_uses_client_termtype() { - let env = FakeEnvironment::new() - .with_var("TMUX", "/tmp/tmux-1000/default,123,0") - .with_var("TERM_PROGRAM", "tmux") - .with_var("TERM_PROGRAM_VERSION", "3.6a") - .with_tmux_client_info(Some("ghostty 1.2.3"), Some("xterm-ghostty")); - let terminal = detect_terminal_info_from_env(&env); - assert_eq!( - terminal, - terminal_info( - TerminalName::Ghostty, - Some("ghostty"), - Some("1.2.3"), - Some("xterm-ghostty"), - Some(Multiplexer::Tmux { - version: Some("3.6a".to_string()), - }), - ), - "tmux_term_program_client_termtype_info" - ); - assert_eq!( - terminal.user_agent_token(), - "ghostty/1.2.3", - "tmux_term_program_client_termtype_user_agent" - ); -} - #[test] fn detects_wezterm() { let env = FakeEnvironment::new().with_var("WEZTERM_VERSION", "2024.2"); @@ -961,3 +827,50 @@ fn detects_term_fallbacks() { ); assert_eq!(terminal.user_agent_token(), "unknown", "unknown_user_agent"); } + +#[test] +fn tmux_preserves_safe_underlying_terminal_identifiers() { + for (variable, value, name, version) in [ + ( + "WEZTERM_VERSION", + "2024.2", + TerminalName::WezTerm, + Some("2024.2"), + ), + ("ITERM_SESSION_ID", "w0t1p0", TerminalName::Iterm2, None), + ("KITTY_WINDOW_ID", "1", TerminalName::Kitty, None), + ("WT_SESSION", "session", TerminalName::WindowsTerminal, None), + ( + "ALACRITTY_SOCKET", + "/tmp/alacritty", + TerminalName::Alacritty, + None, + ), + ( + "GHOSTTY_RESOURCES_DIR", + "/Applications/Ghostty.app/Contents/Resources/ghostty", + TerminalName::Ghostty, + None, + ), + ] { + let env = FakeEnvironment::new() + .with_var("TMUX", "/tmp/tmux-1000/default,123,0") + .with_var("TERM_PROGRAM", "tmux") + .with_var("TERM_PROGRAM_VERSION", "3.5a") + .with_var("TERM", "screen-256color") + .with_var(variable, value); + assert_eq!( + detect_terminal_info_from_env(&env), + terminal_info( + name, + /*term_program*/ None, + version, + /*term*/ None, + Some(Multiplexer::Tmux { + version: Some("3.5a".to_string()) + }), + ), + "{variable}" + ); + } +} diff --git a/codex-rs/tui/src/tui/keyboard_modes.rs b/codex-rs/tui/src/tui/keyboard_modes.rs index f6c08fcbb0..66e8ea7976 100644 --- a/codex-rs/tui/src/tui/keyboard_modes.rs +++ b/codex-rs/tui/src/tui/keyboard_modes.rs @@ -64,6 +64,9 @@ fn running_in_wsl() -> bool { } pub(super) fn running_in_vscode_terminal() -> bool { + if term_program_is_vscode(std::env::var("TERM_PROGRAM").ok().as_deref()) { + return true; + } vscode_terminal_detected( std::env::var("TERM_PROGRAM").ok().as_deref(), windows_term_program().as_deref(), @@ -99,7 +102,11 @@ fn windows_term_program() -> Option { #[cfg(target_os = "linux")] fn read_windows_term_program() -> Option { - let output = std::process::Command::new("cmd.exe") + if !running_in_wsl() { + return None; + } + let executable = codex_utils_path::system_executable("cmd.exe")?; + let output = std::process::Command::new(executable) .args(["/d", "/s", "/c", "set TERM_PROGRAM"]) .stdin(std::process::Stdio::null()) .stderr(std::process::Stdio::null()) @@ -137,6 +144,7 @@ pub(super) fn enable_keyboard_enhancement() { DisableModifyOtherKeys, PushKeyboardEnhancementFlags(keyboard_enhancement_flags( terminal_info().name, + running_in_tmux_session, tmux_extended_keys_format.as_deref() )) ); @@ -151,6 +159,7 @@ pub(super) fn enable_keyboard_enhancement() { fn keyboard_enhancement_flags( terminal_name: TerminalName, + running_in_tmux_session: bool, tmux_extended_keys_format: Option<&str>, ) -> KeyboardEnhancementFlags { let flags = KeyboardEnhancementFlags::DISAMBIGUATE_ESCAPE_CODES @@ -158,9 +167,10 @@ fn keyboard_enhancement_flags( // iTerm and Ghostty can leak shortcut release events that the terminal consumes. // tmux's xterm key format also loses Shift-Enter when event types are - // reported. Preserve repeat classification on transports that support it. + // reported. An unavailable/unrecognized tmux probe must take the same safe + // fallback. Preserve repeat classification on confirmed csi-u transports. if matches!(terminal_name, TerminalName::Ghostty | TerminalName::Iterm2) - || matches!(tmux_extended_keys_format, Some("xterm")) + || (running_in_tmux_session && !matches!(tmux_extended_keys_format, Some("csi-u"))) { flags } else { @@ -190,11 +200,14 @@ fn tmux_should_enable_modify_other_keys_for( } fn read_tmux_extended_keys_format() -> Option { + let executable = codex_utils_path::system_executable("tmux")?; + let path = codex_utils_path::system_path().ok()?; for args in [ ["display-message", "-p", "#{extended-keys-format}"], ["show-options", "-gqv", "extended-keys-format"], ] { - let output = std::process::Command::new("tmux") + let output = std::process::Command::new(&executable) + .env("PATH", &path) .args(args) .stdin(std::process::Stdio::null()) .stderr(std::process::Stdio::null()) @@ -327,6 +340,7 @@ mod tests { assert_eq!( ansi_for(PushKeyboardEnhancementFlags(keyboard_enhancement_flags( TerminalName::Iterm2, + /*running_in_tmux_session*/ false, /*tmux_extended_keys_format*/ None ))), "\x1b[>5u" @@ -338,6 +352,7 @@ mod tests { assert_eq!( ansi_for(PushKeyboardEnhancementFlags(keyboard_enhancement_flags( TerminalName::Ghostty, + /*running_in_tmux_session*/ false, /*tmux_extended_keys_format*/ None ))), "\x1b[>5u" @@ -349,6 +364,7 @@ mod tests { assert_eq!( ansi_for(PushKeyboardEnhancementFlags(keyboard_enhancement_flags( TerminalName::Kitty, + /*running_in_tmux_session*/ false, /*tmux_extended_keys_format*/ None ))), "\x1b[>7u" @@ -360,6 +376,7 @@ mod tests { assert_eq!( ansi_for(PushKeyboardEnhancementFlags(keyboard_enhancement_flags( TerminalName::Kitty, + /*running_in_tmux_session*/ true, Some("csi-u") ))), "\x1b[>7u" @@ -371,6 +388,7 @@ mod tests { assert_eq!( ansi_for(PushKeyboardEnhancementFlags(keyboard_enhancement_flags( TerminalName::Kitty, + /*running_in_tmux_session*/ true, Some("xterm") ))), "\x1b[>5u" @@ -382,12 +400,25 @@ mod tests { assert_eq!( ansi_for(PushKeyboardEnhancementFlags(keyboard_enhancement_flags( TerminalName::Unknown, + /*running_in_tmux_session*/ false, /*tmux_extended_keys_format*/ None ))), "\x1b[>7u" ); } + #[test] + fn keyboard_enhancement_uses_conservative_flags_when_tmux_format_is_unknown() { + assert_eq!( + ansi_for(PushKeyboardEnhancementFlags(keyboard_enhancement_flags( + TerminalName::Unknown, + /*running_in_tmux_session*/ true, + /*tmux_extended_keys_format*/ None, + ))), + "\x1b[>5u" + ); + } + #[test] fn keyboard_enhancement_env_flag_parses_common_values() { assert_eq!(parse_bool_env(Some("1")), Some(true)); diff --git a/codex-rs/utils/path-utils/Cargo.toml b/codex-rs/utils/path-utils/Cargo.toml index d1503fd060..6757b3f69c 100644 --- a/codex-rs/utils/path-utils/Cargo.toml +++ b/codex-rs/utils/path-utils/Cargo.toml @@ -12,6 +12,13 @@ codex-utils-absolute-path = { workspace = true } dunce = { workspace = true } tempfile = { workspace = true } +[target.'cfg(windows)'.dependencies] +windows-sys = { version = "0.52", features = [ + "Win32_Foundation", + "Win32_System_Com", + "Win32_UI_Shell", +] } + [dev-dependencies] pretty_assertions = { workspace = true } tempfile = { workspace = true } diff --git a/codex-rs/utils/path-utils/src/lib.rs b/codex-rs/utils/path-utils/src/lib.rs index e09042ba01..067c75e90c 100644 --- a/codex-rs/utils/path-utils/src/lib.rs +++ b/codex-rs/utils/path-utils/src/lib.rs @@ -2,6 +2,9 @@ pub(crate) mod env; pub use env::is_wsl; +mod system_commands; +pub use system_commands::system_executable; +pub use system_commands::system_path; use codex_utils_absolute_path::AbsolutePathBuf; use std::collections::HashSet; diff --git a/codex-rs/utils/path-utils/src/system_commands.rs b/codex-rs/utils/path-utils/src/system_commands.rs new file mode 100644 index 0000000000..86bc6d90bd --- /dev/null +++ b/codex-rs/utils/path-utils/src/system_commands.rs @@ -0,0 +1,156 @@ +//! Installed helper discovery for automatic work before workspace trust. +//! +//! Only OS and conventional package-manager locations are trusted, never ambient +//! PATH or the working directory. Custom installations fall back to no helper. +//! This assumes the installed software directories themselves are trusted; it +//! does not defend against an attacker who can modify system installations. + +use std::ffi::OsString; +use std::path::Path; +use std::path::PathBuf; + +/// Finds an installed helper without consulting PATH, PATHEXT, or the working directory. +pub fn system_executable(name: &str) -> Option { + executable_in_directories(name, &system_directories()) +} + +/// Child-process PATH for automatic helpers, not for user-requested tools. +pub fn system_path() -> std::io::Result { + let directories = system_directories(); + if directories.is_empty() { + return Err(std::io::Error::other("no trusted executable directories")); + } + std::env::join_paths(directories).map_err(std::io::Error::other) +} + +fn executable_in_directories(name: &str, directories: &[PathBuf]) -> Option { + // Do not allow callers to accidentally escape the installation directories. + if Path::new(name).file_name() != Some(std::ffi::OsStr::new(name)) { + return None; + } + let name = format!("{name}{}", std::env::consts::EXE_SUFFIX); + directories.iter().find_map(|directory| { + let executable = dunce::canonicalize(directory.join(&name)).ok()?; + // A symlink into a workspace is not an installed executable. Package + // manager links may target sibling libexec/Cellar directories. + let installed = directories.iter().any(|root| executable.starts_with(root)) + || installation_roots() + .iter() + .any(|root| executable.starts_with(root)); + if !installed || !executable.is_file() { + return None; + } + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + if executable.metadata().ok()?.permissions().mode() & 0o111 == 0 { + return None; + } + } + Some(executable) + }) +} + +#[cfg(unix)] +fn installation_roots() -> Vec { + [ + "/usr/bin", + "/bin", + "/usr/sbin", + "/sbin", + "/usr/local", + "/opt/homebrew", + "/opt/local", + "/Library/Developer/CommandLineTools", + "/Applications/Xcode.app/Contents/Developer", + "/nix/store", + "/mnt/c/Windows/System32", + ] + .into_iter() + .map(PathBuf::from) + .collect() +} + +fn system_directories() -> Vec { + #[cfg(unix)] + let directories = [ + "/opt/homebrew/bin", + "/usr/local/bin", + "/opt/local/bin", + "/Library/Developer/CommandLineTools/usr/bin", + "/Applications/Xcode.app/Contents/Developer/usr/bin", + "/usr/bin", + "/bin", + "/usr/sbin", + "/sbin", + ] + .into_iter() + .map(PathBuf::from); + #[cfg(target_os = "linux")] + let directories = + directories.chain(crate::is_wsl().then(|| PathBuf::from("/mnt/c/Windows/System32"))); + #[cfg(windows)] + let directories = installation_roots().into_iter().flat_map(|root| { + vec![ + root.join("Git/cmd"), + root.join("Git/mingw64/bin"), + root.join("Git/usr/bin"), + root, + ] + }); + let roots = installation_roots(); + directories + .filter_map(|directory| dunce::canonicalize(directory).ok()) + .filter(|directory| roots.iter().any(|root| directory.starts_with(root))) + .collect() +} + +#[cfg(windows)] +fn installation_roots() -> Vec { + use std::os::windows::ffi::OsStringExt; + use windows_sys::Win32::System::Com::CoTaskMemFree; + use windows_sys::Win32::UI::Shell::FOLDERID_ProgramFiles; + use windows_sys::Win32::UI::Shell::FOLDERID_ProgramFilesX86; + use windows_sys::Win32::UI::Shell::FOLDERID_System; + use windows_sys::Win32::UI::Shell::KF_FLAG_DEFAULT_PATH; + use windows_sys::Win32::UI::Shell::SHGetKnownFolderPath; + + [ + FOLDERID_ProgramFiles, + FOLDERID_ProgramFilesX86, + FOLDERID_System, + ] + .iter() + .filter_map(|id| { + let mut path = std::ptr::null_mut(); + // SAFETY: the API writes a CoTaskMem-allocated, NUL-terminated UTF-16 path. + let result = unsafe { + SHGetKnownFolderPath( + id, + KF_FLAG_DEFAULT_PATH as u32, + /*htoken*/ 0, + &mut path, + ) + }; + if result != 0 || path.is_null() { + return None; + } + // SAFETY: success guarantees the string; free it after copying. + let directory = unsafe { + let mut len = 0; + while *path.add(len) != 0 { + len += 1; + } + let directory = + PathBuf::from(OsString::from_wide(std::slice::from_raw_parts(path, len))); + CoTaskMemFree(path.cast()); + directory + }; + dunce::canonicalize(directory).ok() + }) + .collect() +} + +#[cfg(test)] +#[path = "system_commands_tests.rs"] +mod tests; diff --git a/codex-rs/utils/path-utils/src/system_commands_tests.rs b/codex-rs/utils/path-utils/src/system_commands_tests.rs new file mode 100644 index 0000000000..d964a8a283 --- /dev/null +++ b/codex-rs/utils/path-utils/src/system_commands_tests.rs @@ -0,0 +1,31 @@ +use super::*; +use pretty_assertions::assert_eq; + +#[test] +fn resolves_only_executables_inside_installation_directories() { + let root = tempfile::tempdir().unwrap(); + let bin = root.path().join("bin"); + std::fs::create_dir(&bin).unwrap(); + let executable = bin.join(format!("helper{}", std::env::consts::EXE_SUFFIX)); + std::fs::write(&executable, "installed helper").unwrap(); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&executable, std::fs::Permissions::from_mode(/*mode*/ 0o755)) + .unwrap(); + } + let directories = vec![dunce::canonicalize(bin).unwrap()]; + assert_eq!( + executable_in_directories("helper", &directories), + Some(dunce::canonicalize(&executable).unwrap()) + ); + assert_eq!(executable_in_directories("../helper", &directories), None); + assert_eq!(executable_in_directories("missing", &directories), None); + #[cfg(unix)] + { + let outside = root.path().join("outside"); + std::fs::rename(&executable, &outside).unwrap(); + std::os::unix::fs::symlink(&outside, &executable).unwrap(); + assert_eq!(executable_in_directories("helper", &directories), None); + } +}