mirror of
https://github.com/openai/codex.git
synced 2026-11-01 16:17:58 +00:00
Trust hooks from materialized workspace plugins (#32301)
## What changed - Carry newly installed or updated remote plugin metadata through effective-plugin refresh callbacks, including coalesced refreshes. - After a successful refresh, record the current hook hashes for listed workspace plugins associated with the active account while preserving existing hook settings and unrelated state. - Serialize the background trust write with config mutations, and leave hooks untrusted if the write fails or the active account changes. ## Testing - Cover eligible plugin selection and escaped hook config keys. - Verify end-to-end trust for newly materialized plugin hooks, preservation of existing config, and fail-closed behavior when config cannot be written. GitOrigin-RevId: 0b150766415be6fccc117f7856a241e10ad456f2
This commit is contained in:
@@ -0,0 +1,181 @@
|
||||
use std::collections::BTreeSet;
|
||||
use std::sync::Arc;
|
||||
|
||||
use codex_app_server_protocol::ConfigBatchWriteParams;
|
||||
use codex_app_server_protocol::ConfigEdit;
|
||||
use codex_app_server_protocol::MergeStrategy;
|
||||
use codex_core::ThreadManager;
|
||||
use codex_core_plugins::EffectivePluginsChange;
|
||||
use codex_core_plugins::remote::RemotePluginMaterialization;
|
||||
use codex_core_plugins::remote::RemotePluginScope;
|
||||
use codex_core_plugins::remote::RemotePluginShareDiscoverability;
|
||||
use codex_login::AuthManager;
|
||||
use serde_json::json;
|
||||
use tracing::warn;
|
||||
|
||||
use crate::config_manager::ConfigManager;
|
||||
use crate::request_processors::ConfigRequestProcessor;
|
||||
use crate::request_serialization::RequestSerializationAccess;
|
||||
use crate::request_serialization::RequestSerializationQueueKey;
|
||||
use crate::request_serialization::RequestSerializationQueues;
|
||||
|
||||
/// Refresh plugin consumers and trust hooks from newly materialized Workspace + Listed bundles.
|
||||
pub(crate) fn effective_plugins_changed_callback(
|
||||
auth_manager: Arc<AuthManager>,
|
||||
thread_manager: Arc<ThreadManager>,
|
||||
config_manager: ConfigManager,
|
||||
config_processor: ConfigRequestProcessor,
|
||||
request_serialization_queues: RequestSerializationQueues,
|
||||
) -> Arc<dyn Fn(EffectivePluginsChange) + Send + Sync> {
|
||||
Arc::new(move |change| {
|
||||
thread_manager.plugins_manager().clear_cache();
|
||||
thread_manager.skills_service().clear_cache();
|
||||
|
||||
let refresh_thread_manager = Arc::clone(&thread_manager);
|
||||
let refresh_config_manager = config_manager.clone();
|
||||
tokio::spawn(async move {
|
||||
if refresh_thread_manager.list_thread_ids().await.is_empty() {
|
||||
return;
|
||||
}
|
||||
crate::mcp_refresh::queue_best_effort_refresh(
|
||||
&refresh_thread_manager,
|
||||
&refresh_config_manager,
|
||||
)
|
||||
.await;
|
||||
});
|
||||
|
||||
if change.materialized_remote_plugins.is_empty() {
|
||||
return;
|
||||
}
|
||||
|
||||
let trust_auth_manager = Arc::clone(&auth_manager);
|
||||
let trust_thread_manager = Arc::clone(&thread_manager);
|
||||
let trust_config_manager = config_manager.clone();
|
||||
let trust_config_processor = config_processor.clone();
|
||||
let trust_request_serialization_queues = request_serialization_queues.clone();
|
||||
tokio::spawn(async move {
|
||||
trust_request_serialization_queues
|
||||
.enqueue_background(
|
||||
RequestSerializationQueueKey::Global("config"),
|
||||
RequestSerializationAccess::Exclusive,
|
||||
async move {
|
||||
if let Err(err) = trust_materialized_plugin_hooks(
|
||||
change.materialized_remote_plugins,
|
||||
&trust_auth_manager,
|
||||
&trust_thread_manager,
|
||||
&trust_config_manager,
|
||||
&trust_config_processor,
|
||||
)
|
||||
.await
|
||||
{
|
||||
warn!(error = %err, "failed to trust materialized plugin hooks");
|
||||
}
|
||||
},
|
||||
)
|
||||
.await;
|
||||
});
|
||||
})
|
||||
}
|
||||
|
||||
fn workspace_listed_plugin_ids(
|
||||
materializations: Vec<RemotePluginMaterialization>,
|
||||
current_account_id: &str,
|
||||
) -> BTreeSet<String> {
|
||||
materializations
|
||||
.into_iter()
|
||||
.filter(|plugin| {
|
||||
plugin.scope == RemotePluginScope::Workspace
|
||||
&& plugin.discoverability == Some(RemotePluginShareDiscoverability::Listed)
|
||||
&& plugin.authenticated_account_id.as_deref() == Some(current_account_id)
|
||||
})
|
||||
.map(|plugin| plugin.plugin_id.as_key())
|
||||
.collect()
|
||||
}
|
||||
|
||||
fn hook_trusted_hash_edit(hook_key: &str, current_hash: &str) -> ConfigEdit {
|
||||
let escaped_hook_key = hook_key.replace('\\', "\\\\").replace('"', "\\\"");
|
||||
ConfigEdit {
|
||||
key_path: format!(r#"hooks.state."{escaped_hook_key}".trusted_hash"#),
|
||||
value: json!(current_hash),
|
||||
merge_strategy: MergeStrategy::Replace,
|
||||
}
|
||||
}
|
||||
|
||||
async fn trust_materialized_plugin_hooks(
|
||||
materializations: Vec<RemotePluginMaterialization>,
|
||||
auth_manager: &AuthManager,
|
||||
thread_manager: &ThreadManager,
|
||||
config_manager: &ConfigManager,
|
||||
config_processor: &ConfigRequestProcessor,
|
||||
) -> Result<(), String> {
|
||||
let Some(current_account_id) = auth_manager
|
||||
.auth_cached()
|
||||
.and_then(|auth| auth.get_account_id())
|
||||
else {
|
||||
return Ok(());
|
||||
};
|
||||
let plugin_ids = workspace_listed_plugin_ids(materializations, ¤t_account_id);
|
||||
if plugin_ids.is_empty() {
|
||||
return Ok(());
|
||||
}
|
||||
let config = config_manager
|
||||
.load_latest_config(/*fallback_cwd*/ None)
|
||||
.await
|
||||
.map_err(|err| format!("failed to reload config: {err}"))?;
|
||||
let plugin_outcome = thread_manager
|
||||
.plugins_manager()
|
||||
.plugins_for_config(&config.plugins_config_input())
|
||||
.await;
|
||||
let hooks = codex_hooks::list_hooks(codex_hooks::HooksConfig {
|
||||
feature_enabled: true,
|
||||
bypass_hook_trust: config.bypass_hook_trust,
|
||||
config_layer_stack: Some(config.config_layer_stack),
|
||||
plugin_hook_sources: plugin_outcome.effective_plugin_hook_sources(),
|
||||
plugin_hook_load_warnings: plugin_outcome.effective_plugin_hook_warnings(),
|
||||
..Default::default()
|
||||
});
|
||||
if !hooks.warnings.is_empty() {
|
||||
warn!(
|
||||
warnings = ?hooks.warnings,
|
||||
"hook discovery reported warnings while trusting materialized plugins"
|
||||
);
|
||||
}
|
||||
let edits = hooks
|
||||
.hooks
|
||||
.into_iter()
|
||||
.filter(|hook| {
|
||||
hook.plugin_id
|
||||
.as_ref()
|
||||
.is_some_and(|plugin_id| plugin_ids.contains(plugin_id))
|
||||
})
|
||||
.map(|hook| hook_trusted_hash_edit(&hook.key, &hook.current_hash))
|
||||
.collect::<Vec<_>>();
|
||||
if edits.is_empty() {
|
||||
return Ok(());
|
||||
}
|
||||
if auth_manager
|
||||
.auth_cached()
|
||||
.and_then(|auth| auth.get_account_id())
|
||||
.as_deref()
|
||||
!= Some(current_account_id.as_str())
|
||||
{
|
||||
warn!("skipping materialized plugin hook trust after account changed");
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
let params = ConfigBatchWriteParams {
|
||||
edits,
|
||||
file_path: None,
|
||||
expected_version: None,
|
||||
reload_user_config: true,
|
||||
};
|
||||
config_processor
|
||||
.batch_write(params)
|
||||
.await
|
||||
.map_err(|err| format!("failed to write hook trust: {}", err.message))?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "effective_plugin_change_tests.rs"]
|
||||
mod tests;
|
||||
@@ -0,0 +1,63 @@
|
||||
use super::*;
|
||||
use codex_plugin::PluginId;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
#[test]
|
||||
fn only_workspace_listed_materializations_are_eligible() {
|
||||
let materialization =
|
||||
|name: &str,
|
||||
scope: RemotePluginScope,
|
||||
discoverability: Option<RemotePluginShareDiscoverability>| {
|
||||
RemotePluginMaterialization {
|
||||
plugin_id: PluginId::new(name.to_string(), "test".to_string())
|
||||
.expect("valid plugin id"),
|
||||
scope,
|
||||
discoverability,
|
||||
authenticated_account_id: Some("account-123".to_string()),
|
||||
}
|
||||
};
|
||||
|
||||
let mut materializations = vec![
|
||||
materialization(
|
||||
"eligible",
|
||||
RemotePluginScope::Workspace,
|
||||
Some(RemotePluginShareDiscoverability::Listed),
|
||||
),
|
||||
materialization(
|
||||
"unlisted",
|
||||
RemotePluginScope::Workspace,
|
||||
Some(RemotePluginShareDiscoverability::Unlisted),
|
||||
),
|
||||
materialization(
|
||||
"private",
|
||||
RemotePluginScope::Workspace,
|
||||
Some(RemotePluginShareDiscoverability::Private),
|
||||
),
|
||||
materialization("workspace-missing", RemotePluginScope::Workspace, None),
|
||||
materialization("global", RemotePluginScope::Global, None),
|
||||
materialization("user", RemotePluginScope::User, None),
|
||||
];
|
||||
let mut wrong_account = materialization(
|
||||
"wrong-account",
|
||||
RemotePluginScope::Workspace,
|
||||
Some(RemotePluginShareDiscoverability::Listed),
|
||||
);
|
||||
wrong_account.authenticated_account_id = Some("account-456".to_string());
|
||||
materializations.push(wrong_account);
|
||||
|
||||
let plugin_ids = workspace_listed_plugin_ids(materializations, "account-123");
|
||||
|
||||
assert_eq!(plugin_ids, BTreeSet::from(["eligible@test".to_string()]));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn hook_trusted_hash_edit_targets_only_escaped_leaf() {
|
||||
assert_eq!(
|
||||
hook_trusted_hash_edit(r#"plugin."quoted"\path"#, "sha256:current"),
|
||||
ConfigEdit {
|
||||
key_path: r#"hooks.state."plugin.\"quoted\"\\path".trusted_hash"#.to_string(),
|
||||
value: serde_json::json!("sha256:current"),
|
||||
merge_strategy: MergeStrategy::Replace,
|
||||
}
|
||||
);
|
||||
}
|
||||
@@ -94,6 +94,7 @@ mod connection_cleanup;
|
||||
mod connection_rpc_gate;
|
||||
mod current_time;
|
||||
mod dynamic_tools;
|
||||
mod effective_plugin_change;
|
||||
mod error_code;
|
||||
mod extensions;
|
||||
mod external_auth;
|
||||
|
||||
@@ -313,6 +313,21 @@ impl MessageProcessor {
|
||||
let workspace_settings_cache =
|
||||
Arc::new(workspace_settings::WorkspaceSettingsCache::default());
|
||||
let app_list_shutdown_token = CancellationToken::new();
|
||||
let request_serialization_queues = RequestSerializationQueues::default();
|
||||
let config_processor = ConfigRequestProcessor::new(
|
||||
outgoing.clone(),
|
||||
config_manager.clone(),
|
||||
thread_manager.clone(),
|
||||
analytics_events_client.clone(),
|
||||
);
|
||||
let on_effective_plugins_changed =
|
||||
crate::effective_plugin_change::effective_plugins_changed_callback(
|
||||
auth_manager.clone(),
|
||||
Arc::clone(&thread_manager),
|
||||
config_manager.clone(),
|
||||
config_processor.clone(),
|
||||
request_serialization_queues.clone(),
|
||||
);
|
||||
let account_processor = AccountRequestProcessor::new(
|
||||
auth_manager.clone(),
|
||||
Arc::clone(&thread_manager),
|
||||
@@ -382,6 +397,7 @@ impl MessageProcessor {
|
||||
analytics_events_client.clone(),
|
||||
config_manager.clone(),
|
||||
workspace_settings_cache,
|
||||
on_effective_plugins_changed,
|
||||
);
|
||||
let remote_control_processor = RemoteControlRequestProcessor::new(remote_control_handle);
|
||||
let search_processor = SearchRequestProcessor::new(outgoing.clone());
|
||||
@@ -437,12 +453,6 @@ impl MessageProcessor {
|
||||
Some(on_effective_plugins_changed),
|
||||
);
|
||||
}
|
||||
let config_processor = ConfigRequestProcessor::new(
|
||||
outgoing.clone(),
|
||||
config_manager.clone(),
|
||||
thread_manager.clone(),
|
||||
analytics_events_client.clone(),
|
||||
);
|
||||
let external_agent_config_processor =
|
||||
ExternalAgentConfigRequestProcessor::new(ExternalAgentConfigRequestProcessorArgs {
|
||||
outgoing: outgoing.clone(),
|
||||
@@ -492,7 +502,7 @@ impl MessageProcessor {
|
||||
thread_processor,
|
||||
turn_processor,
|
||||
windows_sandbox_processor,
|
||||
request_serialization_queues: RequestSerializationQueues::default(),
|
||||
request_serialization_queues,
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -215,7 +215,7 @@ impl AccountRequestProcessor {
|
||||
.maybe_start_remote_plugin_caches_refresh(
|
||||
&config.plugins_config_input(),
|
||||
auth,
|
||||
Some(Arc::new(move || {
|
||||
Some(Arc::new(move |_change| {
|
||||
Self::spawn_effective_plugins_changed_task(
|
||||
Arc::clone(&refresh_thread_manager),
|
||||
refresh_config_manager.clone(),
|
||||
|
||||
@@ -36,6 +36,8 @@ pub(crate) struct PluginRequestProcessor {
|
||||
analytics_events_client: AnalyticsEventsClient,
|
||||
config_manager: ConfigManager,
|
||||
workspace_settings_cache: Arc<workspace_settings::WorkspaceSettingsCache>,
|
||||
on_effective_plugins_changed:
|
||||
Arc<dyn Fn(codex_core_plugins::EffectivePluginsChange) + Send + Sync>,
|
||||
}
|
||||
|
||||
fn plugin_skills_to_info(
|
||||
@@ -358,6 +360,9 @@ impl PluginRequestProcessor {
|
||||
analytics_events_client: AnalyticsEventsClient,
|
||||
config_manager: ConfigManager,
|
||||
workspace_settings_cache: Arc<workspace_settings::WorkspaceSettingsCache>,
|
||||
on_effective_plugins_changed: Arc<
|
||||
dyn Fn(codex_core_plugins::EffectivePluginsChange) + Send + Sync,
|
||||
>,
|
||||
) -> Self {
|
||||
Self {
|
||||
auth_manager,
|
||||
@@ -366,6 +371,7 @@ impl PluginRequestProcessor {
|
||||
analytics_events_client,
|
||||
config_manager,
|
||||
workspace_settings_cache,
|
||||
on_effective_plugins_changed,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -468,36 +474,14 @@ impl PluginRequestProcessor {
|
||||
.map(|response| Some(response.into()))
|
||||
}
|
||||
|
||||
pub(crate) fn effective_plugins_changed_callback(&self) -> Arc<dyn Fn() + Send + Sync> {
|
||||
let thread_manager = Arc::clone(&self.thread_manager);
|
||||
let config_manager = self.config_manager.clone();
|
||||
Arc::new(move || {
|
||||
Self::spawn_effective_plugins_changed_task(
|
||||
Arc::clone(&thread_manager),
|
||||
config_manager.clone(),
|
||||
);
|
||||
})
|
||||
pub(crate) fn effective_plugins_changed_callback(
|
||||
&self,
|
||||
) -> Arc<dyn Fn(codex_core_plugins::EffectivePluginsChange) + Send + Sync> {
|
||||
Arc::clone(&self.on_effective_plugins_changed)
|
||||
}
|
||||
|
||||
fn on_effective_plugins_changed(&self) {
|
||||
Self::spawn_effective_plugins_changed_task(
|
||||
Arc::clone(&self.thread_manager),
|
||||
self.config_manager.clone(),
|
||||
);
|
||||
}
|
||||
|
||||
fn spawn_effective_plugins_changed_task(
|
||||
thread_manager: Arc<ThreadManager>,
|
||||
config_manager: ConfigManager,
|
||||
) {
|
||||
tokio::spawn(async move {
|
||||
thread_manager.plugins_manager().clear_cache();
|
||||
thread_manager.skills_service().clear_cache();
|
||||
if thread_manager.list_thread_ids().await.is_empty() {
|
||||
return;
|
||||
}
|
||||
crate::mcp_refresh::queue_best_effort_refresh(&thread_manager, &config_manager).await;
|
||||
});
|
||||
(self.on_effective_plugins_changed)(Default::default());
|
||||
}
|
||||
|
||||
fn clear_plugin_related_caches(&self) {
|
||||
|
||||
@@ -104,7 +104,7 @@ impl RequestSerializationQueueKey {
|
||||
}
|
||||
|
||||
pub(crate) struct QueuedInitializedRequest {
|
||||
gate: Arc<ConnectionRpcGate>,
|
||||
gate: Option<Arc<ConnectionRpcGate>>,
|
||||
future: BoxFutureUnit,
|
||||
}
|
||||
|
||||
@@ -114,14 +114,24 @@ impl QueuedInitializedRequest {
|
||||
future: impl Future<Output = ()> + Send + 'static,
|
||||
) -> Self {
|
||||
Self {
|
||||
gate,
|
||||
gate: Some(gate),
|
||||
future: Box::pin(future),
|
||||
}
|
||||
}
|
||||
|
||||
fn new_background(future: impl Future<Output = ()> + Send + 'static) -> Self {
|
||||
Self {
|
||||
gate: None,
|
||||
future: Box::pin(future),
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) async fn run(self) {
|
||||
let Self { gate, future } = self;
|
||||
gate.run(future).await;
|
||||
match gate {
|
||||
Some(gate) => gate.run(future).await,
|
||||
None => future.await,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -136,6 +146,21 @@ pub(crate) struct RequestSerializationQueues {
|
||||
}
|
||||
|
||||
impl RequestSerializationQueues {
|
||||
/// Enqueue app-owned work alongside RPCs that mutate the same serialized resource.
|
||||
pub(crate) async fn enqueue_background(
|
||||
&self,
|
||||
key: RequestSerializationQueueKey,
|
||||
access: RequestSerializationAccess,
|
||||
future: impl Future<Output = ()> + Send + 'static,
|
||||
) {
|
||||
self.enqueue(
|
||||
key,
|
||||
access,
|
||||
QueuedInitializedRequest::new_background(future),
|
||||
)
|
||||
.await;
|
||||
}
|
||||
|
||||
pub(crate) async fn enqueue(
|
||||
&self,
|
||||
key: RequestSerializationQueueKey,
|
||||
|
||||
Reference in New Issue
Block a user