mirror of
https://github.com/openai/codex.git
synced 2026-11-03 19:53:25 +00:00
app-server: reject nonportable environment cwd
This commit is contained in:
@@ -682,7 +682,7 @@ Turns attach user input (text or images) to a thread and trigger Codex generatio
|
||||
- `{"type":"image","url":"https://…png"}`
|
||||
- `{"type":"localImage","path":"/tmp/screenshot.png"}`
|
||||
|
||||
You can optionally specify config overrides on the new turn. If specified, these settings become the default for subsequent turns on the same thread. `outputSchema` applies only to the current turn. Experimental `environments` is turn-scoped: omit it to inherit the thread's sticky environments, pass `[]` to run the turn with no environments, or pass explicit environment ids to override the sticky selection for this turn only. Each environment `cwd` must be absolute and use that environment's native path convention, so a Windows environment uses a value such as `C:\\workspace` even when app-server runs on Linux.
|
||||
You can optionally specify config overrides on the new turn. If specified, these settings become the default for subsequent turns on the same thread. `outputSchema` applies only to the current turn. Experimental `environments` is turn-scoped: omit it to inherit the thread's sticky environments, pass `[]` to run the turn with no environments, or pass explicit environment ids to override the sticky selection for this turn only. Each environment `cwd` must be absolute and use that environment's native path convention, so a Windows environment uses a value such as `C:\\workspace` even when app-server runs on Linux. The path must also be portable to the selected executor; app-server rejects malformed paths and Windows device names before starting the thread or turn.
|
||||
|
||||
`approvalsReviewer` accepts:
|
||||
|
||||
|
||||
@@ -532,11 +532,13 @@ impl ThreadManager {
|
||||
"failed to resolve shell for environment `{environment_id}`: {err}"
|
||||
))
|
||||
})?;
|
||||
let cwd = cwd.to_path_uri(info.path_convention).map_err(|err| {
|
||||
CodexErr::InvalidRequest(format!(
|
||||
"invalid cwd for environment `{environment_id}`: {err}"
|
||||
))
|
||||
})?;
|
||||
let cwd = cwd
|
||||
.to_normalized_path_uri(info.path_convention)
|
||||
.map_err(|err| {
|
||||
CodexErr::InvalidRequest(format!(
|
||||
"invalid cwd for environment `{environment_id}`: {err}"
|
||||
))
|
||||
})?;
|
||||
selections.push(TurnEnvironmentSelection {
|
||||
environment_id,
|
||||
cwd,
|
||||
|
||||
@@ -415,7 +415,26 @@ async fn native_environment_selections_reject_duplicate_ids() {
|
||||
assert_eq!(
|
||||
error.to_string(),
|
||||
format!(
|
||||
"invalid cwd for environment `local`: path `relative` is not absolute using {} path syntax",
|
||||
"invalid cwd for environment `local`: path `relative` is not a valid absolute {} path",
|
||||
PathConvention::native()
|
||||
)
|
||||
);
|
||||
|
||||
let invalid_native_cwd = match PathConvention::native() {
|
||||
PathConvention::Posix => "/workspace/bad\0name",
|
||||
PathConvention::Windows => r"C:\workspace\NUL.txt",
|
||||
};
|
||||
let error = manager
|
||||
.resolve_native_environment_selections([(
|
||||
"local".to_string(),
|
||||
ApiPathString::new(invalid_native_cwd),
|
||||
)])
|
||||
.await
|
||||
.expect_err("nonportable cwd must fail at the environment boundary");
|
||||
assert_eq!(
|
||||
error.to_string(),
|
||||
format!(
|
||||
"invalid cwd for environment `local`: path `{invalid_native_cwd}` is not a valid absolute {} path",
|
||||
PathConvention::native()
|
||||
)
|
||||
);
|
||||
|
||||
@@ -15,6 +15,7 @@ use codex_app_server_protocol::AskForApproval;
|
||||
use codex_app_server_protocol::CommandExecutionStatus;
|
||||
use codex_app_server_protocol::ItemCompletedNotification;
|
||||
use codex_app_server_protocol::ItemStartedNotification;
|
||||
use codex_app_server_protocol::JSONRPCError;
|
||||
use codex_app_server_protocol::JSONRPCResponse;
|
||||
use codex_app_server_protocol::RequestId;
|
||||
use codex_app_server_protocol::SandboxPolicy;
|
||||
@@ -134,6 +135,34 @@ async fn exercise_app_server(websocket_url: String) -> Result<()> {
|
||||
.await?;
|
||||
timeout(APP_SERVER_TIMEOUT, app_server.initialize()).await??;
|
||||
|
||||
let invalid_cwd = r"C:\workspace\NUL.txt";
|
||||
let invalid_request_id = app_server
|
||||
.send_thread_start_request(ThreadStartParams {
|
||||
model: Some("mock-model".to_string()),
|
||||
environments: Some(vec![TurnEnvironmentParams {
|
||||
environment_id: REMOTE_ENVIRONMENT_ID.to_string(),
|
||||
cwd: ApiPathString::new(invalid_cwd),
|
||||
}]),
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
let invalid_response: JSONRPCError = timeout(
|
||||
APP_SERVER_TIMEOUT,
|
||||
app_server.read_stream_until_error_message(RequestId::Integer(invalid_request_id)),
|
||||
)
|
||||
.await??;
|
||||
assert_eq!(invalid_response.id, RequestId::Integer(invalid_request_id));
|
||||
assert_eq!(
|
||||
invalid_response.error.message,
|
||||
format!(
|
||||
"invalid cwd for environment `{REMOTE_ENVIRONMENT_ID}`: path `{invalid_cwd}` is not a valid absolute Windows path"
|
||||
)
|
||||
);
|
||||
assert!(
|
||||
response_mock.requests().is_empty(),
|
||||
"invalid environment cwd must fail before model inference"
|
||||
);
|
||||
|
||||
let environment = remote_windows_environment();
|
||||
let thread_request_id = app_server
|
||||
.send_thread_start_request(ThreadStartParams {
|
||||
|
||||
@@ -105,6 +105,22 @@ impl ApiPathString {
|
||||
})
|
||||
}
|
||||
|
||||
/// Parses this API string as an absolute, lexically normalized path URI.
|
||||
///
|
||||
/// Unlike [`Self::to_path_uri`], this rejects opaque fallback forms and
|
||||
/// Windows components that cannot name ordinary filesystem entries. Use
|
||||
/// this at process-execution boundaries where the resulting URI must be
|
||||
/// portable across hosts and renderable by the target executor.
|
||||
pub fn to_normalized_path_uri(
|
||||
&self,
|
||||
convention: PathConvention,
|
||||
) -> Result<PathUri, ApiPathStringError> {
|
||||
match convention {
|
||||
PathConvention::Posix => parse_resolved_posix_path(&self.0),
|
||||
PathConvention::Windows => parse_resolved_windows_path(&self.0),
|
||||
}
|
||||
}
|
||||
|
||||
pub fn as_str(&self) -> &str {
|
||||
&self.0
|
||||
}
|
||||
@@ -126,15 +142,7 @@ fn parse_posix_path(path: &str) -> Option<PathUri> {
|
||||
|
||||
fn parse_windows_path(path: &str) -> Option<PathUri> {
|
||||
let bytes = path.as_bytes();
|
||||
let uses_namespace = matches!(
|
||||
bytes,
|
||||
[first, second, namespace @ (b'.' | b'?'), separator, ..]
|
||||
if is_windows_separator_byte(*first)
|
||||
&& is_windows_separator_byte(*second)
|
||||
&& is_windows_separator_byte(*separator)
|
||||
&& matches!(*namespace, b'.' | b'?')
|
||||
);
|
||||
if uses_namespace || path.contains('\0') {
|
||||
if uses_windows_namespace(path) || path.contains('\0') {
|
||||
return Some(windows_opaque_path_uri(path));
|
||||
}
|
||||
|
||||
@@ -205,23 +213,24 @@ fn resolve_windows_path(base: &PathUri, path: &str) -> Result<PathUri, ApiPathSt
|
||||
}
|
||||
|
||||
fn parse_resolved_posix_path(path: &str) -> Result<PathUri, ApiPathStringError> {
|
||||
let native_path = path;
|
||||
let Some(path) = path.strip_prefix('/') else {
|
||||
return Err(invalid_native_path(path, PathConvention::Posix));
|
||||
};
|
||||
if path.contains('\0') {
|
||||
return Err(invalid_native_path(path, PathConvention::Posix));
|
||||
return Err(invalid_native_path(native_path, PathConvention::Posix));
|
||||
}
|
||||
build_resolved_path_uri(
|
||||
/*host*/ None,
|
||||
path.split('/'),
|
||||
/*protected_segments*/ 0,
|
||||
path,
|
||||
native_path,
|
||||
PathConvention::Posix,
|
||||
)
|
||||
}
|
||||
|
||||
fn parse_resolved_windows_path(path: &str) -> Result<PathUri, ApiPathStringError> {
|
||||
if path.contains('\0') {
|
||||
if uses_windows_namespace(path) || path.contains('\0') {
|
||||
return Err(invalid_native_path(path, PathConvention::Windows));
|
||||
}
|
||||
|
||||
@@ -233,6 +242,9 @@ fn parse_resolved_windows_path(path: &str) -> Result<PathUri, ApiPathStringError
|
||||
let Some(share) = segments.next().filter(|share| !share.is_empty()) else {
|
||||
return Err(invalid_native_path(path, PathConvention::Windows));
|
||||
};
|
||||
if !is_valid_windows_component(host) || !is_valid_windows_component(share) {
|
||||
return Err(invalid_native_path(path, PathConvention::Windows));
|
||||
}
|
||||
return build_resolved_path_uri(
|
||||
Some(host),
|
||||
std::iter::once(share).chain(segments),
|
||||
@@ -259,6 +271,17 @@ fn parse_resolved_windows_path(path: &str) -> Result<PathUri, ApiPathStringError
|
||||
)
|
||||
}
|
||||
|
||||
fn uses_windows_namespace(path: &str) -> bool {
|
||||
matches!(
|
||||
path.as_bytes(),
|
||||
[first, second, namespace @ (b'.' | b'?'), separator, ..]
|
||||
if is_windows_separator_byte(*first)
|
||||
&& is_windows_separator_byte(*second)
|
||||
&& is_windows_separator_byte(*separator)
|
||||
&& matches!(*namespace, b'.' | b'?')
|
||||
)
|
||||
}
|
||||
|
||||
fn build_resolved_path_uri<'a>(
|
||||
host: Option<&str>,
|
||||
segments: impl IntoIterator<Item = &'a str>,
|
||||
@@ -569,7 +592,7 @@ pub enum ApiPathStringError {
|
||||
path: String,
|
||||
convention: PathConvention,
|
||||
},
|
||||
#[error("path `{path}` is not absolute using {convention} path syntax")]
|
||||
#[error("path `{path}` is not a valid absolute {convention} path")]
|
||||
InvalidNativePath {
|
||||
path: String,
|
||||
convention: PathConvention,
|
||||
|
||||
@@ -356,6 +356,61 @@ fn rejects_relative_api_paths() {
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn normalized_native_paths_are_host_independent() {
|
||||
for (raw_path, convention, expected_uri) in [
|
||||
(
|
||||
"/workspace/../space #/%/é",
|
||||
PathConvention::Posix,
|
||||
"file:///space%20%23/%25/%C3%A9",
|
||||
),
|
||||
(
|
||||
r"C:\workspace\..\space #\100%\é",
|
||||
PathConvention::Windows,
|
||||
"file:///C:/space%20%23/100%25/%C3%A9",
|
||||
),
|
||||
(
|
||||
r"\\server\share\..\folder",
|
||||
PathConvention::Windows,
|
||||
"file://server/share/folder",
|
||||
),
|
||||
] {
|
||||
assert_eq!(
|
||||
ApiPathString::new(raw_path).to_normalized_path_uri(convention),
|
||||
Ok(PathUri::parse(expected_uri).expect("expected path URI")),
|
||||
"normalizing {raw_path:?}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn normalized_native_paths_reject_nonportable_execution_paths() {
|
||||
for (raw_path, convention) in [
|
||||
("relative", PathConvention::Posix),
|
||||
("/workspace/bad\0name", PathConvention::Posix),
|
||||
(r"C:relative", PathConvention::Windows),
|
||||
(r"\root-relative", PathConvention::Windows),
|
||||
(r"\\server", PathConvention::Windows),
|
||||
(r"\\server\..\folder", PathConvention::Windows),
|
||||
(r"\\server\.\folder", PathConvention::Windows),
|
||||
(r"\\server\NUL\folder", PathConvention::Windows),
|
||||
(r"C:\workspace\NUL.txt", PathConvention::Windows),
|
||||
(r"C:\workspace\trailing.", PathConvention::Windows),
|
||||
("C:\\workspace\\bad\0name", PathConvention::Windows),
|
||||
(r"\\?\C:\workspace", PathConvention::Windows),
|
||||
("/workspace", PathConvention::Windows),
|
||||
] {
|
||||
assert_eq!(
|
||||
ApiPathString::new(raw_path).to_normalized_path_uri(convention),
|
||||
Err(ApiPathStringError::InvalidNativePath {
|
||||
path: raw_path.to_string(),
|
||||
convention,
|
||||
}),
|
||||
"normalizing {raw_path:?}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn renders_an_absolute_path_using_the_explicit_convention() {
|
||||
#[cfg(unix)]
|
||||
@@ -504,6 +559,8 @@ fn native_resolution_rejects_reserved_windows_device_names() {
|
||||
r"C:\COM9.log",
|
||||
r"C:\lpt1",
|
||||
r"child\NUL.txt",
|
||||
r"\\server\..\folder",
|
||||
r"\\server\.\folder",
|
||||
] {
|
||||
assert!(
|
||||
matches!(
|
||||
|
||||
Reference in New Issue
Block a user