From 77e291804910494de82403c46fd8676d7f3082a8 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 28 Apr 2025 10:39:58 -0700 Subject: [PATCH 1/2] fix: drop d as keyboard shortcut for scrolling in the TUI (#704) The existing `b` and `space` are sufficient and `d` and `u` default to half-page scrolling in `less`, so the way we supported `d` and `u` wasn't faithful to that, anyway: https://man7.org/linux/man-pages/man1/less.1.html If we decide to bring `d` and `u` back, they should probably match `less`? --- codex-rs/tui/src/conversation_history_widget.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/codex-rs/tui/src/conversation_history_widget.rs b/codex-rs/tui/src/conversation_history_widget.rs index de1dbba963..d8abb9f107 100644 --- a/codex-rs/tui/src/conversation_history_widget.rs +++ b/codex-rs/tui/src/conversation_history_widget.rs @@ -48,11 +48,11 @@ impl ConversationHistoryWidget { self.scroll_down(1); true } - KeyCode::PageUp | KeyCode::Char('b') | KeyCode::Char('u') | KeyCode::Char('U') => { + KeyCode::PageUp | KeyCode::Char('b') => { self.scroll_page_up(); true } - KeyCode::PageDown | KeyCode::Char(' ') | KeyCode::Char('d') | KeyCode::Char('D') => { + KeyCode::PageDown | KeyCode::Char(' ') => { self.scroll_page_down(); true } @@ -238,7 +238,7 @@ impl WidgetRef for ConversationHistoryWidget { fn render_ref(&self, area: Rect, buf: &mut Buffer) { let (title, border_style) = if self.has_input_focus { ( - "Messages (↑/↓ or j/k = line, b/u = PgUp, space/d = PgDn)", + "Messages (↑/↓ or j/k = line, b/space = page)", Style::default().fg(Color::LightYellow), ) } else { From decb0cc953897e877108e4727c75a2180403752c Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 28 Apr 2025 12:13:52 -0700 Subject: [PATCH 2/2] fix: tighten up check for /usr/bin/sandbox-exec --- .../src/utils/agent/handle-exec-command.ts | 28 +++++++++++-------- .../src/utils/agent/sandbox/macos-seatbelt.ts | 10 ++++++- codex-rs/core/src/exec.rs | 4 ++- 3 files changed, 28 insertions(+), 14 deletions(-) diff --git a/codex-cli/src/utils/agent/handle-exec-command.ts b/codex-cli/src/utils/agent/handle-exec-command.ts index 6cb48016ad..63f8d09229 100644 --- a/codex-cli/src/utils/agent/handle-exec-command.ts +++ b/codex-cli/src/utils/agent/handle-exec-command.ts @@ -11,8 +11,8 @@ import { exec, execApplyPatch } from "./exec.js"; import { ReviewDecision } from "./review.js"; import { isLoggingEnabled, log } from "../logger/log.js"; import { SandboxType } from "./sandbox/interface.js"; -import { access } from "fs/promises"; -import { execFile } from "node:child_process"; +import { PATH_TO_SEATBELT_EXECUTABLE } from "./sandbox/macos-seatbelt.js"; +import fs from "fs/promises"; // --------------------------------------------------------------------------- // Session‑level cache of commands that the user has chosen to always approve. @@ -218,7 +218,7 @@ async function execCommand( let { workdir } = execInput; if (workdir) { try { - await access(workdir); + await fs.access(workdir); } catch (e) { log(`EXEC workdir=${workdir} not found, use process.cwd() instead`); workdir = process.cwd(); @@ -275,14 +275,18 @@ async function execCommand( * Return `true` if the `sandbox-exec` binary can be located. This intentionally does **not** * spawn the binary – we only care about its presence. */ -export const isSandboxExecAvailable = (): Promise => - new Promise((res) => - execFile( - "command", - ["-v", "sandbox-exec"], - { signal: AbortSignal.timeout(200) }, - (err) => res(!err), // exit 0 ⇒ found - ), +const isSandboxExecAvailable: Promise = fs + .access(PATH_TO_SEATBELT_EXECUTABLE, fs.constants.X_OK) + .then( + () => true, + (err) => { + if (!["ENOENT", "ACCESS", "EPERM"].includes(err.code)) { + log( + `Unexpected error for \`stat ${PATH_TO_SEATBELT_EXECUTABLE}\`: ${err.message}`, + ); + } + return false; + }, ); async function getSandbox(runInSandbox: boolean): Promise { @@ -295,7 +299,7 @@ async function getSandbox(runInSandbox: boolean): Promise { // instance, inside certain CI images). Attempting to spawn a missing // binary makes Node.js throw an *uncaught* `ENOENT` error further down // the stack which crashes the whole CLI. - if (await isSandboxExecAvailable()) { + if (await isSandboxExecAvailable) { return SandboxType.MACOS_SEATBELT; } else { throw new Error( diff --git a/codex-cli/src/utils/agent/sandbox/macos-seatbelt.ts b/codex-cli/src/utils/agent/sandbox/macos-seatbelt.ts index 934056d9af..a01e2c63ee 100644 --- a/codex-cli/src/utils/agent/sandbox/macos-seatbelt.ts +++ b/codex-cli/src/utils/agent/sandbox/macos-seatbelt.ts @@ -12,6 +12,14 @@ function getCommonRoots() { ]; } +/** + * When working with `sandbox-exec`, only consider `sandbox-exec` in `/usr/bin` + * to defend against an attacker trying to inject a malicious version on the + * PATH. If /usr/bin/sandbox-exec has been tampered with, then the attacker + * already has root access. + */ +export const PATH_TO_SEATBELT_EXECUTABLE = "/usr/bin/sandbox-exec"; + export function execWithSeatbelt( cmd: Array, opts: SpawnOptions, @@ -57,7 +65,7 @@ export function execWithSeatbelt( ); const fullCommand = [ - "sandbox-exec", + PATH_TO_SEATBELT_EXECUTABLE, "-p", fullPolicy, ...policyTemplateParams, diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index 4ce07acf78..0edc96305b 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -35,6 +35,8 @@ const TIMEOUT_CODE: i32 = 64; const MACOS_SEATBELT_READONLY_POLICY: &str = include_str!("seatbelt_readonly_policy.sbpl"); +const MACOS_PATH_TO_SEATBELT_EXECUTABLE: &str = "/usr/bin/sandbox-exec"; + #[derive(Deserialize, Debug, Clone)] pub struct ExecParams { pub command: Vec, @@ -186,7 +188,7 @@ pub fn create_seatbelt_command( }; let mut seatbelt_command: Vec = vec![ - "sandbox-exec".to_string(), + MACOS_PATH_TO_SEATBELT_EXECUTABLE.to_string(), "-p".to_string(), full_policy.to_string(), ];