Freeze plugin roots in MCP tool attribution (#41117)

## Why

Plugin MCP trust decisions must describe the exact host-discovered server
registration. Looking up a plugin again by ID can resolve a different root.

## What changed

- Retain the host plugin root in MCP catalog attribution and connection identity.
- Reconnect an MCP server when its attributed plugin root changes.
- Pass the retained root through tool lifecycle attribution and use it directly
  when building trusted tool context.

## Testing

Add coverage for catalog identity, connection reconciliation, lifecycle
attribution, and rejecting a different root associated with the same plugin ID.

GitOrigin-RevId: bc92743dd40ab44065a07c3ea972b4956ef8351f
This commit is contained in:
felixxia-oai
2026-08-27 11:27:50 +00:00
committed by copyberry
parent 6c59264b14
commit aa89cf62bb
11 changed files with 279 additions and 38 deletions
+15
View File
@@ -7,6 +7,7 @@ use codex_config::McpServerConfig;
use codex_config::McpServerDisabledReason;
use codex_config::RequirementSource;
use codex_protocol::mcp_policy::EnvironmentMcpPolicy;
use codex_utils_path_uri::PathUri;
use crate::CODEX_APPS_MCP_SERVER_NAME;
@@ -16,6 +17,7 @@ pub struct McpPluginAttribution {
plugin_id: String,
display_name: String,
agent_plugin: bool,
host_root: Option<PathUri>,
}
impl McpPluginAttribution {
@@ -24,6 +26,7 @@ impl McpPluginAttribution {
plugin_id,
display_name,
agent_plugin: false,
host_root: None,
}
}
@@ -32,9 +35,16 @@ impl McpPluginAttribution {
plugin_id,
display_name,
agent_plugin: true,
host_root: None,
}
}
/// Records the exact host-discovered plugin root.
pub fn with_host_root(mut self, host_root: PathUri) -> Self {
self.host_root = Some(host_root);
self
}
pub fn plugin_id(&self) -> &str {
&self.plugin_id
}
@@ -46,6 +56,11 @@ impl McpPluginAttribution {
pub fn is_agent_plugin(&self) -> bool {
self.agent_plugin
}
/// Returns the host-discovered root captured with this server registration.
pub fn host_root(&self) -> Option<&PathUri> {
self.host_root.as_ref()
}
}
/// The component that declared an MCP server registration.
+29
View File
@@ -9,6 +9,7 @@ use codex_config::McpServerToolConfig;
use codex_config::McpServerTransportConfig;
use codex_protocol::mcp_policy::EnvironmentMcpPolicy;
use codex_protocol::mcp_policy::PluginMcpRequirements;
use codex_utils_path_uri::PathUri;
use pretty_assertions::assert_eq;
use crate::CODEX_APPS_MCP_SERVER_NAME;
@@ -20,6 +21,7 @@ use super::McpServerConflictAction;
use super::McpServerRegistration;
use super::McpServerSource;
use super::ResolvedMcpCatalog;
use super::ResolvedMcpServer;
fn server(url: &str) -> McpServerConfig {
McpServerConfig {
@@ -85,6 +87,33 @@ fn remove(source: McpServerSource) -> McpServerConflictAction {
McpServerConflictAction::Remove(source)
}
#[test]
fn plugin_host_root_is_retained_in_catalog_identity() {
let original_root = PathUri::parse("file:///plugins/original").expect("valid plugin root URI");
let replacement_root =
PathUri::parse("file:///plugins/replacement").expect("valid plugin root URI");
let catalog_for_root = |root| {
let mut builder = ResolvedMcpCatalog::builder();
builder.register(McpServerRegistration::from_plugin(
"docs".to_string(),
plugin("plugin@test").with_host_root(root),
/*plugin_order*/ 0,
server("https://plugin.example/mcp"),
));
builder.build()
};
let original = catalog_for_root(original_root.clone());
let replacement = catalog_for_root(replacement_root);
let Some(McpServerSource::Plugin(attribution)) =
original.server("docs").map(ResolvedMcpServer::source)
else {
panic!("expected host-discovered plugin registration");
};
assert_eq!(attribution.host_root(), Some(&original_root));
assert!(!original.has_same_servers(&replacement));
}
#[test]
fn source_precedence_preserves_the_winning_registration() {
let extension = server("https://extension.example/mcp");
+15 -8
View File
@@ -30,6 +30,7 @@ use std::sync::atomic::Ordering;
use std::time::Duration;
use crate::binding::call_tool_result_from_rmcp;
use crate::catalog::McpServerSource;
use crate::elicitation::ElicitationRequestManager;
use crate::elicitation::ElicitationRequestRouter;
use crate::mcp::CODEX_APPS_MCP_SERVER_NAME;
@@ -284,14 +285,19 @@ impl McpConnectionSet {
.into_iter()
.filter(|(_, server)| server.enabled())
{
let is_host_owned_codex_apps = config
.mcp_server_catalog
.server(&server_name)
.is_some_and(|server| {
server
.source()
.is_host_owned_apps(&server_name, server.config())
});
let registration = config.mcp_server_catalog.server(&server_name);
let is_host_owned_codex_apps = registration.is_some_and(|server| {
server
.source()
.is_host_owned_apps(&server_name, server.config())
});
let host_plugin_root = registration.and_then(|server| match server.source() {
McpServerSource::Plugin(plugin) => plugin.host_root(),
McpServerSource::SelectedPlugin(_)
| McpServerSource::Config
| McpServerSource::Compatibility { .. }
| McpServerSource::Extension { .. } => None,
});
let catalog_item_limit = if is_host_owned_codex_apps {
MAX_CODEX_APPS_TOOL_CATALOG_ITEMS
} else {
@@ -348,6 +354,7 @@ impl McpConnectionSet {
let connection_identity = McpServerConnectionIdentity::new(
&server_name,
&server,
host_plugin_root,
store_mode,
keyring_backend_kind,
&resolved_environment,
@@ -58,6 +58,7 @@ use codex_rmcp_client::InProcessTransportFactory;
use codex_rmcp_client::McpAuthState;
use codex_rmcp_client::McpLoginRequirement;
use codex_rmcp_client::RmcpClient;
use codex_utils_path_uri::PathUri;
use futures::FutureExt;
use futures::future::BoxFuture;
use pretty_assertions::assert_eq;
@@ -3854,6 +3855,7 @@ async fn executor_owned_chatgpt_mcp_accepts_only_safe_explicit_authorization() -
McpServerConnectionIdentity::new(
"fake-first-party",
remote_server,
/*host_plugin_root*/ None,
OAuthCredentialsStoreMode::File,
keyring_backend_kind,
&resolved_environment,
@@ -4354,6 +4356,7 @@ fn reusable_server_identity(
McpServerConnectionIdentity::new(
"docs",
&server,
/*host_plugin_root*/ None,
OAuthCredentialsStoreMode::default(),
AuthKeyringBackendKind::default(),
&resolved_environment,
@@ -4406,16 +4409,29 @@ async fn reconcile_reusable_server(
config: McpServerConfig,
runtime_context: McpRuntimeContext,
) -> McpConnectionSet {
let (tx_event, _rx_event) = async_channel::unbounded();
let codex_home = tempdir().expect("tempdir");
reconcile_reusable_server_with_mcp_config(
previous,
config,
runtime_context,
crate::mcp::tests::test_mcp_config(codex_home.path().to_path_buf()),
)
.await
}
async fn reconcile_reusable_server_with_mcp_config(
previous: &McpConnectionSet,
config: McpServerConfig,
runtime_context: McpRuntimeContext,
mcp_config: crate::McpConfig,
) -> McpConnectionSet {
let (tx_event, _rx_event) = async_channel::unbounded();
McpConnectionSet::new(
Some(previous),
McpPublicationGate::already_published(),
McpRuntimeInput {
startup_policy: McpStartupPolicy::Eager,
config: Arc::new(crate::mcp::tests::test_mcp_config(
codex_home.path().to_path_buf(),
)),
config: Arc::new(mcp_config),
plugins_available: false,
ready_selected_capability_roots: Vec::new(),
mcp_servers: HashMap::from([(
@@ -4738,6 +4754,7 @@ fn connection_identity_uses_effective_authorization_headers() {
McpServerConnectionIdentity::new(
"docs",
&server,
/*host_plugin_root*/ None,
OAuthCredentialsStoreMode::File,
keyring_backend_kind,
&Ok(None),
@@ -5118,6 +5135,81 @@ async fn reconciliation_reconnects_when_connection_identity_changes() {
assert!(!previous.shares_test_connection_with(&reconciled, "docs"));
}
#[tokio::test]
async fn reconciliation_reconnects_when_host_plugin_root_changes() {
let runtime_context = reusable_server_runtime_context();
let server_config = reusable_server_config("http://127.0.0.1:1");
let original_root = PathUri::parse("file:///plugins/original").expect("valid plugin root URI");
let replacement_root =
PathUri::parse("file:///plugins/replacement").expect("valid plugin root URI");
let mut previous = manager_with_reusable_ready_server(
&server_config,
&runtime_context,
vec![create_test_tool("docs", "search")],
)
.await;
let server = EffectiveMcpServer::configured(server_config.clone());
let resolved_environment = runtime_context.resolve_server_environment("docs", &server_config);
let original_identity = McpServerConnectionIdentity::new(
"docs",
&server,
Some(&original_root),
OAuthCredentialsStoreMode::default(),
AuthKeyringBackendKind::default(),
&resolved_environment,
&runtime_context,
/*runtime_auth_provider*/ None,
/*auth*/ None,
/*codex_apps_cache_identity*/ None,
ElicitationCapability::default(),
ClientMcpExtensions::default(),
/*previous_identity*/ None,
);
Arc::get_mut(
&mut previous
.servers
.get_mut("docs")
.expect("test server should exist")
.connection,
)
.expect("test server should have one connection owner")
.identity = Some(original_identity);
let codex_home = tempdir().expect("tempdir");
let config_for_root = |root| {
let mut config = crate::mcp::tests::test_mcp_config(codex_home.path().to_path_buf());
let mut catalog = crate::ResolvedMcpCatalog::builder();
catalog.register(crate::McpServerRegistration::from_plugin(
"docs".to_string(),
crate::McpPluginAttribution::new("docs@test".to_string(), "Docs".to_string())
.with_host_root(root),
/*plugin_order*/ 0,
server_config.clone(),
));
config.mcp_server_catalog = catalog.build();
config
};
let unchanged = reconcile_reusable_server_with_mcp_config(
&previous,
server_config.clone(),
runtime_context.clone(),
config_for_root(original_root),
)
.await;
assert!(previous.shares_test_connection_with(&unchanged, "docs"));
let replacement_config = config_for_root(replacement_root);
let replacement = reconcile_reusable_server_with_mcp_config(
&unchanged,
server_config,
runtime_context,
replacement_config,
)
.await;
assert!(!unchanged.shares_test_connection_with(&replacement, "docs"));
}
#[tokio::test]
async fn connection_identity_distinguishes_accounts_with_the_same_token() -> anyhow::Result<()> {
let runtime_context = reusable_server_runtime_context();
@@ -5139,6 +5231,7 @@ async fn connection_identity_distinguishes_accounts_with_the_same_token() -> any
McpServerConnectionIdentity::new(
"docs",
&server,
/*host_plugin_root*/ None,
OAuthCredentialsStoreMode::default(),
AuthKeyringBackendKind::default(),
&Ok(None),
@@ -5191,6 +5284,7 @@ async fn connection_identity_distinguishes_agent_account_runtime_and_task() -> a
McpServerConnectionIdentity::new(
CODEX_APPS_MCP_SERVER_NAME,
&server,
/*host_plugin_root*/ None,
OAuthCredentialsStoreMode::default(),
AuthKeyringBackendKind::default(),
&Ok(None),
+5
View File
@@ -17,6 +17,7 @@ use codex_login::CodexAuth;
use codex_protocol::mcp::ClientMcpExtensions;
use codex_rmcp_client::StoredOAuthCredentialSnapshot;
use codex_rmcp_client::StoredOAuthTokens;
use codex_utils_path_uri::PathUri;
use rmcp::model::ElicitationCapability;
use tracing::warn;
@@ -96,6 +97,7 @@ pub(crate) struct McpServerConnectionIdentity {
auth: McpServerAuth,
transport: McpServerTransportConfig,
environment_id: String,
host_plugin_root: Option<PathUri>,
oauth_store: Option<(OAuthCredentialsStoreMode, AuthKeyringBackendKind)>,
oauth_credentials: Result<Option<StoredOAuthCredentialSnapshot>, String>,
pub(crate) oauth_store_was_contended: bool,
@@ -115,6 +117,7 @@ impl McpServerConnectionIdentity {
pub(crate) fn new(
server_name: &str,
server: &EffectiveMcpServer,
host_plugin_root: Option<&PathUri>,
store_mode: OAuthCredentialsStoreMode,
keyring_backend_kind: AuthKeyringBackendKind,
resolved_environment: &Result<Option<Arc<Environment>>, String>,
@@ -210,6 +213,7 @@ impl McpServerConnectionIdentity {
auth: config.auth.clone(),
transport: config.transport.clone(),
environment_id: config.environment_id.clone(),
host_plugin_root: host_plugin_root.cloned(),
oauth_store: stored_oauth_url
.is_some()
.then_some((store_mode, keyring_backend_kind)),
@@ -244,6 +248,7 @@ impl McpServerConnectionIdentity {
self.auth == other.auth
&& self.transport == other.transport
&& self.environment_id == other.environment_id
&& self.host_plugin_root == other.host_plugin_root
&& self.oauth_store == other.oauth_store
&& same_resolved_environment(&self.resolved_environment, &other.resolved_environment)
&& self.local_stdio_fallback_cwd == other.local_stdio_fallback_cwd
+2 -1
View File
@@ -5533,7 +5533,8 @@ async fn rebuild_preserving_session_layers_refreshes_plugin_derived_mcp_config()
.plugin_attributions_by_server_name(),
HashMap::from([(
"sample".to_string(),
McpPluginAttribution::new("sample@test".to_string(), "sample".to_string()),
McpPluginAttribution::new("sample@test".to_string(), "sample".to_string())
.with_host_root(PathUri::from_host_native_path(&plugin_root)?),
)])
);
+2 -1
View File
@@ -1702,7 +1702,8 @@ impl Config {
plugin.config_name.clone(),
plugin.display_name().to_string(),
)
};
}
.with_host_root(PathUri::from_abs_path(&plugin.root));
for (name, plugin_server) in plugin_mcp_servers {
catalog.register(McpServerRegistration::from_plugin(
name,
@@ -18,6 +18,7 @@ use codex_extension_api::ToolLifecycleFuture;
use codex_extension_api::ToolStartInput;
use codex_mcp::CODEX_APPS_MCP_SERVER_NAME;
use codex_protocol::models::ContentItem;
use codex_utils_path_uri::PathUri;
use core_test_support::apps_test_server::AppsTestServer;
use core_test_support::apps_test_server::SEARCH_CALENDAR_LIST_TOOL;
use core_test_support::apps_test_server::SEARCH_CALENDAR_NAMESPACE;
@@ -285,6 +286,70 @@ async fn tool_start_receives_executed_mcp_call_for_connector(
Ok(())
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn tool_start_receives_frozen_host_plugin_root() -> Result<()> {
skip_if_no_network!(Ok(()));
skip_if_wine_exec!(Ok(()), "requires a native test_stdio_server");
let server = responses::start_mock_server().await;
let codex_home = Arc::new(tempfile::tempdir()?);
let plugin_root = super::plugins::write_sample_plugin_manifest_and_config(codex_home.as_ref());
let server_config = json!({
"command": super::rmcp_client::remote_aware_stdio_server_bin()?,
"environment_id": super::rmcp_client::remote_aware_environment_id(),
});
fs::write(
plugin_root.join(".mcp.json"),
serde_json::to_vec(&json!({"mcpServers": {"sample": server_config}}))?,
)?;
let recorder = Arc::new(ConversationHistoryRecorder::default());
let mut extensions = ExtensionRegistryBuilder::<Config>::new();
extensions.tool_lifecycle_contributor(recorder.clone());
let test = test_codex()
.with_home(codex_home)
.with_extensions(Arc::new(extensions.build()))
.with_model_info_override("gpt-5.4", |model| model.supports_search_tool = false)
.build_with_auto_env(&server)
.await?;
wait_for_mcp_server(&test.codex, "sample").await?;
responses::mount_sse_sequence(
&server,
vec![
responses::sse(vec![
responses::ev_function_call_with_namespace(
"plugin-lifecycle-call",
"mcp__sample",
"echo",
r#"{"message":"ping"}"#,
),
responses::ev_completed("first-response"),
]),
responses::sse(vec![responses::ev_completed("second-response")]),
],
)
.await;
test.submit_text_turn("Call the sample echo tool.").await?;
let histories = recorder
.histories
.lock()
.unwrap_or_else(std::sync::PoisonError::into_inner);
let [history] = histories.as_slice() else {
panic!("expected one tool start, got {}", histories.len());
};
let expected_source = McpToolSource::Plugin {
id: "sample@test".to_string(),
root: PathUri::from_host_native_path(&plugin_root)?,
};
assert_eq!(
history.mcp_tool,
Some(("sample".to_string(), None, expected_source)),
);
Ok(())
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn tool_start_receives_rewritten_payload_and_post_hook_history() -> Result<()> {
skip_if_no_network!(Ok(()));
@@ -5,9 +5,11 @@ use std::sync::Arc;
use codex_config::McpServerConfig;
use codex_mcp::McpServerSource;
use codex_mcp::PreparedMcpCall;
use codex_mcp::ResolvedMcpServer;
use codex_tools::ToolCallSource;
use codex_tools::ToolName;
use codex_tools::ToolPayload;
use codex_utils_path_uri::PathUri;
use crate::ConversationHistorySnapshot;
use crate::ExtensionData;
@@ -47,6 +49,8 @@ pub enum McpToolSource {
Plugin {
/// Identifier of the plugin that owns this MCP server.
id: String,
/// Host-local plugin root captured with the exact server registration.
root: PathUri,
},
/// An executor-selected plugin whose root has not been attested by the host.
SelectedPlugin,
@@ -71,21 +75,24 @@ impl McpToolContext {
configured_server: Option<&McpServerConfig>,
) -> Self {
let tool = call.tool_info().clone();
let registration = call.config().mcp_server_catalog.server(call.server_name());
let source = if tool.connector_id.is_some() && call.is_host_owned_apps() {
McpToolSource::Connector
} else if call.is_selected_plugin_server() {
McpToolSource::SelectedPlugin
} else if let Some(id) = call.plugin_id() {
McpToolSource::Plugin { id: id.to_owned() }
} else if call
.config()
.mcp_server_catalog
.server(call.server_name())
.is_some_and(|server| {
matches!(server.source(), McpServerSource::Config)
&& configured_server.is_some_and(|configured| server.config() == configured)
})
} else if let Some(McpServerSource::Plugin(plugin)) =
registration.map(ResolvedMcpServer::source)
&& Some(plugin.plugin_id()) == call.plugin_id()
&& let Some(root) = plugin.host_root()
{
McpToolSource::Plugin {
id: plugin.plugin_id().to_owned(),
root: root.clone(),
}
} else if registration.is_some_and(|server| {
matches!(server.source(), McpServerSource::Config)
&& configured_server.is_some_and(|configured| server.config() == configured)
}) {
McpToolSource::Config
} else {
McpToolSource::Other
@@ -68,13 +68,13 @@ pub(crate) async fn trusted_tool_context(
) -> Option<GuardianTrustedToolFragment> {
let codex_home = config.codex_home.as_path().canonicalize().ok()?;
let plugins = match source {
McpToolSource::Connector | McpToolSource::Plugin { .. } => Some(
McpToolSource::Connector => Some(
manager
.plugins_manager()
.plugins_for_config(&config.plugins_config_input())
.await,
),
McpToolSource::Config => None,
McpToolSource::Config | McpToolSource::Plugin { .. } => None,
McpToolSource::SelectedPlugin | McpToolSource::Other => return None,
};
@@ -98,18 +98,16 @@ pub(crate) async fn trusted_tool_context(
trusted_user_config_source(config, "apps", connector_id, &codex_home)?
}
}
McpToolSource::Plugin { id: plugin_id } => {
let plugin = plugins.as_ref()?.plugins().iter().find(|plugin| {
plugin.is_active()
&& plugin.config_name == plugin_id.as_str()
&& plugin.mcp_servers.contains_key(&tool.server_name)
&& is_home_owned_plugin_capability(
plugin.root.as_path(),
&codex_home,
PluginCapability::Mcp,
)
})?;
plugin.root.as_path().display().to_string()
McpToolSource::Plugin { root, .. } => {
let plugin_root = root.to_abs_path().ok()?;
if !is_home_owned_plugin_capability(
plugin_root.as_path(),
&codex_home,
PluginCapability::Mcp,
) {
return None;
}
plugin_root.as_path().display().to_string()
}
McpToolSource::Config => {
trusted_user_config_source(config, "mcp_servers", &tool.server_name, &codex_home)?
@@ -149,6 +149,11 @@ async fn trusts_connector_declared_by_home_owned_plugin() -> Result<()> {
&mcp,
&McpToolSource::Plugin {
id: "trusted@test".to_string(),
root: test
.config
.codex_home
.join("plugins/cache/test/trusted/local")
.into(),
},
&test.thread_manager,
&test.config,
@@ -156,6 +161,20 @@ async fn trusts_connector_declared_by_home_owned_plugin() -> Result<()> {
.await
.expect("home-owned plugin MCP server should be trusted");
assert_eq!(mcp_context.metadata, expected_context(&mcp, &plugin_root));
// A trusted cached plugin with the same ID must not replace the frozen outside-home root.
assert_eq!(
trusted_tool_context(
&mcp,
&McpToolSource::Plugin {
id: "trusted@test".to_string(),
root: test.config.cwd.clone().into(),
},
&test.thread_manager,
&test.config,
)
.await,
None,
);
assert_eq!(
trusted_tool_context(
&mcp,