mirror of
https://github.com/openai/codex.git
synced 2026-11-13 14:46:14 +00:00
Preserve Windows sandbox identity during credential retry (#29624)
## Summary - recognize stale Windows sandbox credentials from both runner logon and child startup failures - refresh credentials once without changing the original command, permissions, file rules, desktop mode, or managed-network identity - add a Windows regression test that forces error 1312 and inspects the real retry arguments ## Why Elevated unified exec starts commands in two steps: ```text Codex -> sandbox command runner -> requested command ``` Either process start can fail when Windows invalidates the sandbox logon session. The child-side failure was previously returned as text, so the parent could not reliably recognize Windows error 1312. The existing retry also refreshed credentials with `proxy_enforced = false`, even when the original request used managed networking. That could change the selected Windows sandbox identity from offline to online during the retry. ## How - carry the failure stage and numeric Windows error code through the command-runner IPC protocol - preserve native `CreateProcessAsUserW` error codes instead of parsing error messages - keep every retry-sensitive field in one request and use it for both attempts - retry exactly once after refreshing credentials, then return the second failure - share the retry rule with the elevated capture path The Windows test injects error 1312 on both attempts and verifies: - two spawn attempts and one credential refresh - stale credentials are replaced by refreshed credentials - both attempts receive the same command, environment, cwd, permissions, roots, deny paths, TTY settings, and private-desktop mode - credential refresh receives the original `proxy_enforced` value ## Tests - `just test -p codex-windows-sandbox` - the new Windows-only regression test is included in the Windows nextest CI archive
This commit is contained in:
9 files changed
+488
-103
No files matched your search
@@ -26,7 +26,7 @@ use std::path::PathBuf;
|
||||
const MAX_FRAME_LEN: usize = 8 * 1024 * 1024;
|
||||
|
||||
/// Protocol version shared by the parent process and elevated command runner.
|
||||
pub const IPC_PROTOCOL_VERSION: u8 = 3;
|
||||
pub const IPC_PROTOCOL_VERSION: u8 = 4;
|
||||
|
||||
/// Length-prefixed, JSON-encoded frame.
|
||||
#[derive(Debug, Serialize, Deserialize, Clone)]
|
||||
@@ -118,7 +118,17 @@ pub struct ExitPayload {
|
||||
#[derive(Debug, Serialize, Deserialize, Clone)]
|
||||
pub struct ErrorPayload {
|
||||
pub message: String,
|
||||
pub code: String,
|
||||
pub stage: ErrorStage,
|
||||
pub windows_error_code: Option<u32>,
|
||||
}
|
||||
|
||||
/// Runner startup stage that produced an error.
|
||||
#[derive(Debug, Serialize, Deserialize, Clone, Copy, PartialEq, Eq)]
|
||||
#[serde(rename_all = "snake_case")]
|
||||
pub enum ErrorStage {
|
||||
ReadSpawnRequest,
|
||||
SpawnChild,
|
||||
WriteSpawnReady,
|
||||
}
|
||||
|
||||
/// Empty payload for control messages.
|
||||
@@ -236,4 +246,32 @@ mod tests {
|
||||
assert_eq!(PermissionProfile::read_only(), payload.permission_profile);
|
||||
assert_eq!(workspace_roots, payload.workspace_roots);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn error_payload_serializes_stage_and_windows_error_code() {
|
||||
let msg = FramedMessage {
|
||||
version: IPC_PROTOCOL_VERSION,
|
||||
message: Message::Error {
|
||||
payload: ErrorPayload {
|
||||
message: "CreateProcessAsUserW failed".to_string(),
|
||||
stage: ErrorStage::SpawnChild,
|
||||
windows_error_code: Some(1312),
|
||||
},
|
||||
},
|
||||
};
|
||||
|
||||
let encoded = serde_json::to_value(&msg).expect("serialize");
|
||||
assert_eq!(
|
||||
serde_json::json!({
|
||||
"version": IPC_PROTOCOL_VERSION,
|
||||
"type": "error",
|
||||
"payload": {
|
||||
"message": "CreateProcessAsUserW failed",
|
||||
"stage": "spawn_child",
|
||||
"windows_error_code": 1312,
|
||||
}
|
||||
}),
|
||||
encoded
|
||||
);
|
||||
}
|
||||
}
|
||||
@@ -1,4 +1,6 @@
|
||||
use crate::identity::SandboxCreds;
|
||||
use crate::ipc_framed::ErrorPayload;
|
||||
use crate::ipc_framed::ErrorStage;
|
||||
use crate::ipc_framed::FramedMessage;
|
||||
use crate::ipc_framed::IPC_PROTOCOL_VERSION;
|
||||
use crate::ipc_framed::Message;
|
||||
@@ -29,6 +31,7 @@ use windows_sys::Win32::Foundation::CloseHandle;
|
||||
use windows_sys::Win32::Foundation::DUPLICATE_SAME_ACCESS;
|
||||
use windows_sys::Win32::Foundation::DuplicateHandle;
|
||||
use windows_sys::Win32::Foundation::ERROR_LOGON_FAILURE;
|
||||
use windows_sys::Win32::Foundation::ERROR_NO_SUCH_LOGON_SESSION;
|
||||
use windows_sys::Win32::Foundation::ERROR_NOT_FOUND;
|
||||
use windows_sys::Win32::Foundation::GetLastError;
|
||||
use windows_sys::Win32::Foundation::HANDLE;
|
||||
@@ -63,14 +66,69 @@ impl std::fmt::Display for RunnerLogonError {
|
||||
|
||||
impl std::error::Error for RunnerLogonError {}
|
||||
|
||||
#[derive(Debug)]
|
||||
pub(crate) struct RunnerStartupError {
|
||||
payload: ErrorPayload,
|
||||
}
|
||||
|
||||
impl RunnerStartupError {
|
||||
pub(crate) fn new(payload: ErrorPayload) -> Self {
|
||||
Self { payload }
|
||||
}
|
||||
}
|
||||
|
||||
impl std::fmt::Display for RunnerStartupError {
|
||||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
|
||||
write!(
|
||||
f,
|
||||
"runner failed during {:?}: {}",
|
||||
self.payload.stage, self.payload.message
|
||||
)?;
|
||||
if let Some(code) = self.payload.windows_error_code {
|
||||
write!(f, " (Windows error {code})")?;
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
impl std::error::Error for RunnerStartupError {}
|
||||
|
||||
pub(crate) struct RunnerTransport {
|
||||
pipe_write: File,
|
||||
pipe_read: File,
|
||||
}
|
||||
|
||||
pub(crate) fn is_stale_sandbox_creds_error(err: &anyhow::Error) -> bool {
|
||||
err.downcast_ref::<RunnerLogonError>()
|
||||
.is_some_and(|err| err.code == ERROR_LOGON_FAILURE)
|
||||
fn is_refreshable_windows_error(code: u32) -> bool {
|
||||
matches!(code, ERROR_LOGON_FAILURE | ERROR_NO_SUCH_LOGON_SESSION)
|
||||
}
|
||||
|
||||
pub(crate) fn is_refreshable_sandbox_creds_error(err: &anyhow::Error) -> bool {
|
||||
if err
|
||||
.downcast_ref::<RunnerLogonError>()
|
||||
.is_some_and(|err| is_refreshable_windows_error(err.code))
|
||||
{
|
||||
return true;
|
||||
}
|
||||
|
||||
err.downcast_ref::<RunnerStartupError>().is_some_and(|err| {
|
||||
err.payload.stage == ErrorStage::SpawnChild
|
||||
&& err
|
||||
.payload
|
||||
.windows_error_code
|
||||
.is_some_and(is_refreshable_windows_error)
|
||||
})
|
||||
}
|
||||
|
||||
pub(crate) fn retry_runner_spawn_once<T>(
|
||||
sandbox_creds: SandboxCreds,
|
||||
mut spawn: impl FnMut(SandboxCreds) -> Result<T>,
|
||||
refresh: impl FnOnce() -> Result<SandboxCreds>,
|
||||
) -> Result<T> {
|
||||
match spawn(sandbox_creds) {
|
||||
Ok(result) => Ok(result),
|
||||
Err(err) if is_refreshable_sandbox_creds_error(&err) => spawn(refresh()?),
|
||||
Err(err) => Err(err),
|
||||
}
|
||||
}
|
||||
|
||||
impl RunnerTransport {
|
||||
@@ -90,7 +148,7 @@ impl RunnerTransport {
|
||||
.ok_or_else(|| anyhow::anyhow!("runner pipe closed before spawn_ready"))?;
|
||||
match msg.message {
|
||||
Message::SpawnReady { .. } => Ok(()),
|
||||
Message::Error { payload } => Err(anyhow::anyhow!("runner error: {}", payload.message)),
|
||||
Message::Error { payload } => Err(RunnerStartupError::new(payload).into()),
|
||||
other => Err(anyhow::anyhow!(
|
||||
"expected spawn_ready from runner, got {other:?}"
|
||||
)),
|
||||
@@ -416,20 +474,46 @@ fn wait_for_complete_frame(pipe_read: &File, timeout: Duration) -> Result<()> {
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::RunnerLogonError;
|
||||
use super::is_stale_sandbox_creds_error;
|
||||
use super::RunnerStartupError;
|
||||
use super::is_refreshable_sandbox_creds_error;
|
||||
use crate::ipc_framed::ErrorPayload;
|
||||
use crate::ipc_framed::ErrorStage;
|
||||
use pretty_assertions::assert_eq;
|
||||
use windows_sys::Win32::Foundation::ERROR_LOGON_FAILURE;
|
||||
use windows_sys::Win32::Foundation::ERROR_NO_SUCH_LOGON_SESSION;
|
||||
use windows_sys::Win32::Foundation::ERROR_NOT_FOUND;
|
||||
|
||||
#[test]
|
||||
fn stale_sandbox_creds_error_recognizes_logon_failures() {
|
||||
fn refreshable_sandbox_creds_error_recognizes_credential_and_child_start_failures() {
|
||||
assert_eq!(
|
||||
[ERROR_LOGON_FAILURE, ERROR_NOT_FOUND].map(|code| {
|
||||
[
|
||||
ERROR_LOGON_FAILURE,
|
||||
ERROR_NO_SUCH_LOGON_SESSION,
|
||||
ERROR_NOT_FOUND,
|
||||
]
|
||||
.map(|code| {
|
||||
let err =
|
||||
anyhow::Error::new(RunnerLogonError { code }).context("runner launch failed");
|
||||
is_stale_sandbox_creds_error(&err)
|
||||
is_refreshable_sandbox_creds_error(&err)
|
||||
}),
|
||||
[true, false]
|
||||
[true, true, false]
|
||||
);
|
||||
|
||||
assert_eq!(
|
||||
[
|
||||
(ErrorStage::SpawnChild, ERROR_NO_SUCH_LOGON_SESSION),
|
||||
(ErrorStage::SpawnChild, ERROR_NOT_FOUND),
|
||||
(ErrorStage::ReadSpawnRequest, ERROR_NO_SUCH_LOGON_SESSION),
|
||||
]
|
||||
.map(|(stage, windows_error_code)| {
|
||||
let err = anyhow::Error::new(RunnerStartupError::new(ErrorPayload {
|
||||
message: "runner startup failed".to_string(),
|
||||
stage,
|
||||
windows_error_code: Some(windows_error_code),
|
||||
}));
|
||||
is_refreshable_sandbox_creds_error(&err)
|
||||
}),
|
||||
[true, false, false]
|
||||
);
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user