mirror of
https://github.com/openai/codex.git
synced 2026-05-22 03:54:18 +00:00
Fix app-server isolated runtime capability escapes
This commit is contained in:
@@ -222,6 +222,10 @@ impl ConfigManager {
|
||||
typesafe_overrides: ConfigOverrides,
|
||||
fallback_cwd: Option<PathBuf>,
|
||||
) -> std::io::Result<Config> {
|
||||
let file_system = self
|
||||
.runtime_capabilities
|
||||
.require_local_filesystem("load app-server config")
|
||||
.map_err(std::io::Error::other)?;
|
||||
let merged_cli_overrides = cli_overrides
|
||||
.iter()
|
||||
.cloned()
|
||||
@@ -242,7 +246,7 @@ impl ConfigManager {
|
||||
.fallback_cwd(fallback_cwd)
|
||||
.cloud_requirements(self.current_cloud_requirements())
|
||||
.thread_config_loader(self.current_thread_config_loader())
|
||||
.build()
|
||||
.build_with_file_system(file_system.as_ref())
|
||||
.await?;
|
||||
self.apply_runtime_feature_enablement(&mut config);
|
||||
self.apply_arg0_paths(&mut config);
|
||||
|
||||
@@ -388,6 +388,7 @@ impl MessageProcessor {
|
||||
Arc::clone(&thread_manager),
|
||||
outgoing.clone(),
|
||||
config_manager.clone(),
|
||||
Arc::clone(&runtime_capabilities),
|
||||
);
|
||||
let plugin_processor = PluginRequestProcessor::new(
|
||||
auth_manager.clone(),
|
||||
@@ -413,6 +414,7 @@ impl MessageProcessor {
|
||||
arg0_paths.clone(),
|
||||
Arc::clone(&config),
|
||||
config_manager.clone(),
|
||||
Arc::clone(&runtime_capabilities),
|
||||
Arc::clone(&thread_store),
|
||||
Arc::clone(&pending_thread_unloads),
|
||||
thread_state_manager.clone(),
|
||||
|
||||
@@ -8,6 +8,7 @@ pub(crate) struct McpRequestProcessor {
|
||||
thread_manager: Arc<ThreadManager>,
|
||||
outgoing: Arc<OutgoingMessageSender>,
|
||||
config_manager: ConfigManager,
|
||||
runtime_capabilities: Arc<RuntimeCapabilities>,
|
||||
}
|
||||
|
||||
impl McpRequestProcessor {
|
||||
@@ -16,12 +17,14 @@ impl McpRequestProcessor {
|
||||
thread_manager: Arc<ThreadManager>,
|
||||
outgoing: Arc<OutgoingMessageSender>,
|
||||
config_manager: ConfigManager,
|
||||
runtime_capabilities: Arc<RuntimeCapabilities>,
|
||||
) -> Self {
|
||||
Self {
|
||||
auth_manager,
|
||||
thread_manager,
|
||||
outgoing,
|
||||
config_manager,
|
||||
runtime_capabilities,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -204,18 +207,12 @@ impl McpRequestProcessor {
|
||||
.to_mcp_config(self.thread_manager.plugins_manager().as_ref())
|
||||
.await;
|
||||
let auth = self.auth_manager.auth().await;
|
||||
let environment_manager = self.thread_manager.environment_manager();
|
||||
let runtime_environment = match environment_manager.default_environment() {
|
||||
Some(environment) => {
|
||||
// Status listing has no turn cwd. This fallback is used only
|
||||
// by executor-backed stdio MCPs whose config omits `cwd`.
|
||||
McpRuntimeEnvironment::new(environment, config.cwd.to_path_buf())
|
||||
}
|
||||
None => McpRuntimeEnvironment::new(
|
||||
environment_manager.local_environment(),
|
||||
config.cwd.to_path_buf(),
|
||||
),
|
||||
};
|
||||
let runtime_environment = runtime_environment_without_thread(
|
||||
self.thread_manager.environment_manager().as_ref(),
|
||||
&self.runtime_capabilities,
|
||||
config.cwd.to_path_buf(),
|
||||
"list MCP server status without thread",
|
||||
)?;
|
||||
|
||||
tokio::spawn(async move {
|
||||
Self::list_mcp_server_status_task(
|
||||
@@ -369,15 +366,12 @@ impl McpRequestProcessor {
|
||||
.to_mcp_config(self.thread_manager.plugins_manager().as_ref())
|
||||
.await;
|
||||
let auth = self.auth_manager.auth().await;
|
||||
let runtime_environment = {
|
||||
let environment_manager = self.thread_manager.environment_manager();
|
||||
let environment = environment_manager
|
||||
.default_environment()
|
||||
.unwrap_or_else(|| environment_manager.local_environment());
|
||||
// Resource reads without a thread have no turn cwd. This fallback
|
||||
// is used only by executor-backed stdio MCPs whose config omits `cwd`.
|
||||
McpRuntimeEnvironment::new(environment, config.cwd.to_path_buf())
|
||||
};
|
||||
let runtime_environment = runtime_environment_without_thread(
|
||||
self.thread_manager.environment_manager().as_ref(),
|
||||
&self.runtime_capabilities,
|
||||
config.cwd.to_path_buf(),
|
||||
"read MCP resource without thread",
|
||||
)?;
|
||||
let request_id = request_id.clone();
|
||||
|
||||
tokio::spawn(async move {
|
||||
@@ -435,6 +429,64 @@ impl McpRequestProcessor {
|
||||
}
|
||||
}
|
||||
|
||||
fn runtime_environment_without_thread(
|
||||
environment_manager: &EnvironmentManager,
|
||||
runtime_capabilities: &RuntimeCapabilities,
|
||||
cwd: PathBuf,
|
||||
local_fallback_operation: &str,
|
||||
) -> Result<McpRuntimeEnvironment, JSONRPCErrorError> {
|
||||
let environment = match environment_manager.default_environment() {
|
||||
Some(environment) => environment,
|
||||
None => runtime_capabilities
|
||||
.require_local_environment(local_fallback_operation)
|
||||
.map_err(|err| internal_error(err.to_string()))?,
|
||||
};
|
||||
// Threadless MCP requests have no turn cwd. This fallback is used only
|
||||
// by executor-backed stdio MCPs whose config omits `cwd`.
|
||||
Ok(McpRuntimeEnvironment::new(environment, cwd))
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::runtime_environment_without_thread;
|
||||
use codex_core::RuntimeCapabilities;
|
||||
use codex_exec_server::EnvironmentManager;
|
||||
use codex_exec_server::ExecServerRuntimePaths;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
#[test]
|
||||
fn threadless_mcp_local_fallback_rejects_isolated_runtime() {
|
||||
let environment_manager = EnvironmentManager::disabled_for_tests(test_runtime_paths());
|
||||
let error = runtime_environment_without_thread(
|
||||
&environment_manager,
|
||||
&RuntimeCapabilities::isolated(),
|
||||
test_cwd(),
|
||||
"list MCP server status without thread",
|
||||
)
|
||||
.expect_err("isolated runtime should reject local MCP fallback");
|
||||
|
||||
assert_eq!(
|
||||
error.message,
|
||||
"list MCP server status without thread requires ambient worker-local environment"
|
||||
);
|
||||
}
|
||||
|
||||
fn test_runtime_paths() -> ExecServerRuntimePaths {
|
||||
ExecServerRuntimePaths::new(
|
||||
std::env::current_exe().expect("current exe"),
|
||||
/*codex_linux_sandbox_exe*/ None,
|
||||
)
|
||||
.expect("runtime paths")
|
||||
}
|
||||
|
||||
fn test_cwd() -> std::path::PathBuf {
|
||||
AbsolutePathBuf::current_dir()
|
||||
.expect("current dir")
|
||||
.to_path_buf()
|
||||
}
|
||||
}
|
||||
|
||||
fn with_mcp_tool_call_thread_id_meta(
|
||||
meta: Option<serde_json::Value>,
|
||||
thread_id: &str,
|
||||
|
||||
@@ -330,6 +330,7 @@ pub(crate) struct ThreadRequestProcessor {
|
||||
pub(super) arg0_paths: Arg0DispatchPaths,
|
||||
pub(super) config: Arc<Config>,
|
||||
pub(super) config_manager: ConfigManager,
|
||||
pub(super) runtime_capabilities: Arc<RuntimeCapabilities>,
|
||||
pub(super) thread_store: Arc<dyn ThreadStore>,
|
||||
pub(super) pending_thread_unloads: Arc<Mutex<HashSet<ThreadId>>>,
|
||||
pub(super) thread_state_manager: ThreadStateManager,
|
||||
@@ -350,6 +351,7 @@ impl ThreadRequestProcessor {
|
||||
arg0_paths: Arg0DispatchPaths,
|
||||
config: Arc<Config>,
|
||||
config_manager: ConfigManager,
|
||||
runtime_capabilities: Arc<RuntimeCapabilities>,
|
||||
thread_store: Arc<dyn ThreadStore>,
|
||||
pending_thread_unloads: Arc<Mutex<HashSet<ThreadId>>>,
|
||||
thread_state_manager: ThreadStateManager,
|
||||
@@ -366,6 +368,7 @@ impl ThreadRequestProcessor {
|
||||
arg0_paths,
|
||||
config,
|
||||
config_manager,
|
||||
runtime_capabilities,
|
||||
thread_store,
|
||||
pending_thread_unloads,
|
||||
thread_state_manager,
|
||||
@@ -654,10 +657,16 @@ impl ThreadRequestProcessor {
|
||||
.map(|response| Some(response.into()))
|
||||
}
|
||||
|
||||
async fn instruction_sources_from_config(config: &Config) -> Vec<AbsolutePathBuf> {
|
||||
codex_core::AgentsMdManager::new(config)
|
||||
.instruction_sources(LOCAL_FS.as_ref())
|
||||
.await
|
||||
async fn instruction_sources_from_config(
|
||||
runtime_capabilities: &RuntimeCapabilities,
|
||||
config: &Config,
|
||||
) -> Result<Vec<AbsolutePathBuf>, JSONRPCErrorError> {
|
||||
let file_system = runtime_capabilities
|
||||
.require_local_filesystem("load thread instruction sources")
|
||||
.map_err(|err| internal_error(err.to_string()))?;
|
||||
Ok(codex_core::AgentsMdManager::new(config)
|
||||
.instruction_sources(file_system.as_ref())
|
||||
.await)
|
||||
}
|
||||
|
||||
async fn load_thread(
|
||||
@@ -1006,9 +1015,14 @@ impl ThreadRequestProcessor {
|
||||
&& config.active_project.trust_level.is_none()
|
||||
&& (requested_permissions_trust_project || effective_permissions_trust_project)
|
||||
{
|
||||
let trust_target = resolve_root_git_project_for_trust(LOCAL_FS.as_ref(), &config.cwd)
|
||||
.await
|
||||
.unwrap_or_else(|| config.cwd.clone());
|
||||
let file_system = self
|
||||
.runtime_capabilities
|
||||
.require_local_filesystem("trust requested thread cwd")
|
||||
.map_err(|err| internal_error(err.to_string()))?;
|
||||
let trust_target =
|
||||
resolve_root_git_project_for_trust(file_system.as_ref(), &config.cwd)
|
||||
.await
|
||||
.unwrap_or_else(|| config.cwd.clone());
|
||||
let current_cli_overrides = config_manager.current_cli_overrides();
|
||||
let cli_overrides_with_trust;
|
||||
let cli_overrides_for_reload = if let Err(err) =
|
||||
@@ -1055,7 +1069,8 @@ impl ThreadRequestProcessor {
|
||||
.map_err(|err| config_load_error(&err))?;
|
||||
}
|
||||
|
||||
let instruction_sources = Self::instruction_sources_from_config(&config).await;
|
||||
let instruction_sources =
|
||||
Self::instruction_sources_from_config(&self.runtime_capabilities, &config).await?;
|
||||
let environments = environments.unwrap_or_else(|| {
|
||||
listener_task_context
|
||||
.thread_manager
|
||||
@@ -2439,7 +2454,8 @@ impl ThreadRequestProcessor {
|
||||
}
|
||||
};
|
||||
|
||||
let instruction_sources = Self::instruction_sources_from_config(&config).await;
|
||||
let instruction_sources =
|
||||
Self::instruction_sources_from_config(&self.runtime_capabilities, &config).await?;
|
||||
let response_history = thread_history.clone();
|
||||
|
||||
match self
|
||||
@@ -2724,8 +2740,11 @@ impl ThreadRequestProcessor {
|
||||
thread_summary.session_id = existing_thread.session_configured().session_id.to_string();
|
||||
let mut config_for_instruction_sources = self.config.as_ref().clone();
|
||||
config_for_instruction_sources.cwd = config_snapshot.cwd.clone();
|
||||
let instruction_sources =
|
||||
Self::instruction_sources_from_config(&config_for_instruction_sources).await;
|
||||
let instruction_sources = Self::instruction_sources_from_config(
|
||||
&self.runtime_capabilities,
|
||||
&config_for_instruction_sources,
|
||||
)
|
||||
.await?;
|
||||
|
||||
let listener_command_tx = {
|
||||
let thread_state = thread_state.lock().await;
|
||||
@@ -3095,7 +3114,8 @@ impl ThreadRequestProcessor {
|
||||
.map_err(|err| config_load_error(&err))?;
|
||||
|
||||
let fallback_model_provider = config.model_provider_id.clone();
|
||||
let instruction_sources = Self::instruction_sources_from_config(&config).await;
|
||||
let instruction_sources =
|
||||
Self::instruction_sources_from_config(&self.runtime_capabilities, &config).await?;
|
||||
|
||||
let NewThread {
|
||||
thread_id,
|
||||
|
||||
@@ -59,6 +59,8 @@ mod thread_processor_behavior_tests {
|
||||
use codex_config::SessionThreadConfig;
|
||||
use codex_config::StaticThreadConfigLoader;
|
||||
use codex_config::ThreadConfigSource;
|
||||
use codex_core::RuntimeCapabilities;
|
||||
use codex_core::config::ConfigBuilder;
|
||||
use codex_model_provider_info::ModelProviderInfo;
|
||||
use codex_model_provider_info::WireApi;
|
||||
use codex_protocol::ThreadId;
|
||||
@@ -88,6 +90,28 @@ mod thread_processor_behavior_tests {
|
||||
use std::sync::Arc;
|
||||
use tempfile::TempDir;
|
||||
|
||||
#[tokio::test]
|
||||
async fn instruction_sources_reject_isolated_runtime_without_local_filesystem() {
|
||||
let temp_dir = TempDir::new().expect("tempdir");
|
||||
let config = ConfigBuilder::without_managed_config_for_tests()
|
||||
.codex_home(temp_dir.path().to_path_buf())
|
||||
.build()
|
||||
.await
|
||||
.expect("build config");
|
||||
|
||||
let error = ThreadRequestProcessor::instruction_sources_from_config(
|
||||
&RuntimeCapabilities::isolated(),
|
||||
&config,
|
||||
)
|
||||
.await
|
||||
.expect_err("isolated runtime should reject instruction source load");
|
||||
|
||||
assert_eq!(
|
||||
error.message,
|
||||
"load thread instruction sources requires ambient worker-local filesystem"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn validate_dynamic_tools_rejects_unsupported_input_schema() {
|
||||
let tools = vec![ApiDynamicToolSpec {
|
||||
|
||||
@@ -1069,11 +1069,18 @@ impl ConfigBuilder {
|
||||
}
|
||||
|
||||
pub async fn build(self) -> std::io::Result<Config> {
|
||||
// Keep the large config-loading future off small runtime thread stacks.
|
||||
Box::pin(self.build_inner()).await
|
||||
self.build_with_file_system(LOCAL_FS.as_ref()).await
|
||||
}
|
||||
|
||||
async fn build_inner(self) -> std::io::Result<Config> {
|
||||
pub async fn build_with_file_system(
|
||||
self,
|
||||
file_system: &dyn ExecutorFileSystem,
|
||||
) -> std::io::Result<Config> {
|
||||
// Keep the large config-loading future off small runtime thread stacks.
|
||||
Box::pin(self.build_inner(file_system)).await
|
||||
}
|
||||
|
||||
async fn build_inner(self, file_system: &dyn ExecutorFileSystem) -> std::io::Result<Config> {
|
||||
let Self {
|
||||
codex_home,
|
||||
cli_overrides,
|
||||
@@ -1098,7 +1105,7 @@ impl ConfigBuilder {
|
||||
};
|
||||
harness_overrides.cwd = Some(cwd.to_path_buf());
|
||||
let config_layer_stack = load_config_layers_state(
|
||||
LOCAL_FS.as_ref(),
|
||||
file_system,
|
||||
&codex_home,
|
||||
Some(cwd),
|
||||
&cli_overrides,
|
||||
@@ -1159,7 +1166,7 @@ impl ConfigBuilder {
|
||||
config_layer_stack.requirements_toml().clone(),
|
||||
)?;
|
||||
let mut config = Config::load_config_with_layer_stack(
|
||||
LOCAL_FS.as_ref(),
|
||||
file_system,
|
||||
lock_config_toml,
|
||||
harness_overrides,
|
||||
codex_home,
|
||||
@@ -1173,7 +1180,7 @@ impl ConfigBuilder {
|
||||
return Ok(config);
|
||||
}
|
||||
Config::load_config_with_layer_stack(
|
||||
LOCAL_FS.as_ref(),
|
||||
file_system,
|
||||
config_toml,
|
||||
harness_overrides,
|
||||
codex_home,
|
||||
@@ -2425,10 +2432,9 @@ impl Config {
|
||||
guardian_policy_config_source: _,
|
||||
} = config_layer_stack.requirements().clone();
|
||||
|
||||
let user_instructions =
|
||||
AgentsMdManager::load_global_instructions(LOCAL_FS.as_ref(), Some(&codex_home))
|
||||
.await
|
||||
.map(|loaded| loaded.contents);
|
||||
let user_instructions = AgentsMdManager::load_global_instructions(fs, Some(&codex_home))
|
||||
.await
|
||||
.map(|loaded| loaded.contents);
|
||||
let mut startup_warnings = config_layer_stack
|
||||
.startup_warnings()
|
||||
.unwrap_or_default()
|
||||
|
||||
Reference in New Issue
Block a user