agent/codex
2
0
Fork 0
mirror of https://github.com/pchuan98/codex.git synced 2026-07-01 00:31:56 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix: harden plugin feature gating (#15104)

Browse Source 
Resubmit https://github.com/openai/codex/pull/15020 with correct
content.

1. Use requirement-resolved config.features as the plugin gate.
2. Guard plugin/list, plugin/read, and related flows behind that gate.
3. Skip bad marketplace.json files instead of failing the whole list.
4. Simplify plugin state and caching.
This commit is contained in:
xl-openai
2026-03-18 17:03:37 -07:00
committed by GitHub
Unverified
parent 56d0c6bf67
commit dcd5e08269
13 changed files with 337 additions and 108 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core/src/plugins/manager.rs
+44 -68
View File
@@ -29,16 +29,12 @@ use crate::auth::CodexAuth;
use crate::config::Config;
use crate::config::ConfigService;
use crate::config::ConfigServiceError;
use crate::config::ConfigToml;
use crate::config::edit::ConfigEdit;
use crate::config::edit::ConfigEditsBuilder;
use crate::config::profile::ConfigProfile;
use crate::config::types::McpServerConfig;
use crate::config::types::PluginConfig;
use crate::config_loader::ConfigLayerStack;
use crate::features::Feature;
use crate::features::FeatureOverrides;
use crate::features::Features;
use crate::skills::SkillMetadata;
use crate::skills::loader::SkillRoot;
use crate::skills::loader::load_skills_from_roots;
@@ -421,7 +417,7 @@ impl From<RemotePluginFetchError> for PluginRemoteSyncError {
pub struct PluginsManager {
codex_home: PathBuf,
store: PluginStore,
cache_by_cwd: RwLock<HashMap<PathBuf, PluginLoadOutcome>>,
cached_enabled_outcome: RwLock<Option<PluginLoadOutcome>>,
analytics_events_client: RwLock<Option<AnalyticsEventsClient>>,
}
@@ -430,7 +426,7 @@ impl PluginsManager {
Self {
codex_home: codex_home.clone(),
store: PluginStore::new(codex_home),
cache_by_cwd: RwLock::new(HashMap::new()),
cached_enabled_outcome: RwLock::new(None),
analytics_events_client: RwLock::new(None),
}
}
@@ -444,49 +440,44 @@ impl PluginsManager {
}
pub fn plugins_for_config(&self, config: &Config) -> PluginLoadOutcome {
self.plugins_for_layer_stack(
&config.cwd,
&config.config_layer_stack,
/*force_reload*/ false,
)
self.plugins_for_config_with_force_reload(config, /*force_reload*/ false)
}
pub fn plugins_for_layer_stack(
pub(crate) fn plugins_for_config_with_force_reload(
&self,
cwd: &Path,
config_layer_stack: &ConfigLayerStack,
config: &Config,
force_reload: bool,
) -> PluginLoadOutcome {
if !plugins_feature_enabled_from_stack(config_layer_stack) {
if !config.features.enabled(Feature::Plugins) {
return PluginLoadOutcome::default();
}
if !force_reload && let Some(outcome) = self.cached_outcome_for_cwd(cwd) {
if !force_reload && let Some(outcome) = self.cached_enabled_outcome() {
return outcome;
}
let outcome = load_plugins_from_layer_stack(config_layer_stack, &self.store);
let outcome = load_plugins_from_layer_stack(&config.config_layer_stack, &self.store);
log_plugin_load_errors(&outcome);
let mut cache = match self.cache_by_cwd.write() {
let mut cache = match self.cached_enabled_outcome.write() {
Ok(cache) => cache,
Err(err) => err.into_inner(),
};
cache.insert(cwd.to_path_buf(), outcome.clone());
*cache = Some(outcome.clone());
outcome
}
pub fn clear_cache(&self) {
let mut cache_by_cwd = match self.cache_by_cwd.write() {
let mut cached_enabled_outcome = match self.cached_enabled_outcome.write() {
Ok(cache) => cache,
Err(err) => err.into_inner(),
};
cache_by_cwd.clear();
*cached_enabled_outcome = None;
}
fn cached_outcome_for_cwd(&self, cwd: &Path) -> Option<PluginLoadOutcome> {
match self.cache_by_cwd.read() {
Ok(cache) => cache.get(cwd).cloned(),
Err(err) => err.into_inner().get(cwd).cloned(),
fn cached_enabled_outcome(&self) -> Option<PluginLoadOutcome> {
match self.cached_enabled_outcome.read() {
Ok(cache) => cache.clone(),
Err(err) => err.into_inner().clone(),
}
}
@@ -634,6 +625,10 @@ impl PluginsManager {
config: &Config,
auth: Option<&CodexAuth>,
) -> Result<RemotePluginSyncResult, PluginRemoteSyncError> {
if !config.features.enabled(Feature::Plugins) {
return Ok(RemotePluginSyncResult::default());
}
info!("starting remote plugin sync");
let remote_plugins = fetch_remote_plugin_status(config, auth)
.await
@@ -817,7 +812,11 @@ impl PluginsManager {
config: &Config,
additional_roots: &[AbsolutePathBuf],
) -> Result<Vec<ConfiguredMarketplace>, MarketplaceError> {
let (installed_plugins, configured_plugins) = self.configured_plugin_states(config);
if !config.features.enabled(Feature::Plugins) {
return Ok(Vec::new());
}
let (installed_plugins, enabled_plugins) = self.configured_plugin_states(config);
let marketplaces = list_marketplaces(&self.marketplace_roots(additional_roots))?;
let mut seen_plugin_keys = HashSet::new();
@@ -840,10 +839,7 @@ impl PluginsManager {
// resolve to the first discovered source.
id: plugin_key.clone(),
installed: installed_plugins.contains(&plugin_key),
enabled: configured_plugins
.get(&plugin_key)
.copied()
.unwrap_or(false),
enabled: enabled_plugins.contains(&plugin_key),
name: plugin.name,
source: plugin.source,
policy: plugin.policy,
@@ -867,6 +863,10 @@ impl PluginsManager {
config: &Config,
request: &PluginReadRequest,
) -> Result<PluginReadOutcome, MarketplaceError> {
if !config.features.enabled(Feature::Plugins) {
return Err(MarketplaceError::PluginsDisabled);
}
let marketplace = load_marketplace(&request.marketplace_path)?;
let marketplace_name = marketplace.name.clone();
let plugin = marketplace
@@ -886,7 +886,7 @@ impl PluginsManager {
},
)?;
let plugin_key = plugin_id.as_key();
let (installed_plugins, configured_plugins) = self.configured_plugin_states(config);
let (installed_plugins, enabled_plugins) = self.configured_plugin_states(config);
let source_path = match &plugin.source {
MarketplacePluginSource::Local { path } => path.clone(),
};
@@ -927,10 +927,7 @@ impl PluginsManager {
policy: plugin.policy,
interface: plugin.interface,
installed: installed_plugins.contains(&plugin_key),
enabled: configured_plugins
.get(&plugin_key)
.copied()
.unwrap_or(false),
enabled: enabled_plugins.contains(&plugin_key),
skills,
apps,
mcp_server_names,
@@ -939,7 +936,7 @@ impl PluginsManager {
}
pub fn maybe_start_curated_repo_sync_for_config(self: &Arc<Self>, config: &Config) {
if plugins_feature_enabled_from_stack(&config.config_layer_stack) {
if config.features.enabled(Feature::Plugins) {
let mut configured_curated_plugin_ids =
configured_plugins_from_stack(&config.config_layer_stack)
.into_keys()
@@ -1005,25 +1002,22 @@ impl PluginsManager {
}
}
fn configured_plugin_states(
&self,
config: &Config,
) -> (HashSet<String>, HashMap<String, bool>) {
let installed_plugins = configured_plugins_from_stack(&config.config_layer_stack)
.into_keys()
fn configured_plugin_states(&self, config: &Config) -> (HashSet<String>, HashSet<String>) {
let configured_plugins = configured_plugins_from_stack(&config.config_layer_stack);
let installed_plugins = configured_plugins
.keys()
.filter(|plugin_key| {
PluginId::parse(plugin_key)
.ok()
.is_some_and(|plugin_id| self.store.is_installed(&plugin_id))
})
.cloned()
.collect::<HashSet<_>>();
let configured_plugins = self
.plugins_for_config(config)
.plugins()
.iter()
.map(|plugin| (plugin.config_name.clone(), plugin.enabled))
.collect::<HashMap<String, bool>>();
(installed_plugins, configured_plugins)
let enabled_plugins = configured_plugins
.into_iter()
.filter_map(|(plugin_key, plugin)| plugin.enabled.then_some(plugin_key))
.collect::<HashSet<_>>();
(installed_plugins, enabled_plugins)
}
fn marketplace_roots(&self, additional_roots: &[AbsolutePathBuf]) -> Vec<AbsolutePathBuf> {
@@ -1107,24 +1101,6 @@ impl PluginUninstallError {
}
}
fn plugins_feature_enabled_from_stack(config_layer_stack: &ConfigLayerStack) -> bool {
// Plugins are intentionally opt-in from the persisted user config only. Project config
// layers should not be able to enable plugin loading for a checkout.
let Some(user_layer) = config_layer_stack.get_user_layer() else {
return false;
};
let Ok(config_toml) = user_layer.config.clone().try_into::<ConfigToml>() else {
warn!("failed to deserialize config when checking plugin feature flag");
return false;
};
let config_profile = config_toml
.get_config_profile(config_toml.profile.clone())
.unwrap_or_else(|_| ConfigProfile::default());
let features =
Features::from_config(&config_toml, &config_profile, FeatureOverrides::default());
features.enabled(Feature::Plugins)
}
fn log_plugin_load_errors(outcome: &PluginLoadOutcome) {
for plugin in outcome
.plugins
@@ -1263,7 +1239,7 @@ fn refresh_curated_plugin_cache(
fn configured_plugins_from_stack(
config_layer_stack: &ConfigLayerStack,
) -> HashMap<String, PluginConfig> {
// Keep plugin entries aligned with the same user-layer-only semantics as the feature gate.
// Plugin entries remain persisted user config only.
let Some(user_layer) = config_layer_stack.get_user_layer() else {
return HashMap::new();
};
codex-rs/core/src/plugins/manager_tests.rs
+131 -18
View File
@@ -59,18 +59,8 @@ fn plugin_config_toml(enabled: bool, plugins_feature_enabled: bool) -> String {
fn load_plugins_from_config(config_toml: &str, codex_home: &Path) -> PluginLoadOutcome {
write_file(&codex_home.join(CONFIG_TOML_FILE), config_toml);
let stack = ConfigLayerStack::new(
vec![ConfigLayerEntry::new(
ConfigLayerSource::User {
file: AbsolutePathBuf::try_from(codex_home.join(CONFIG_TOML_FILE)).unwrap(),
},
toml::from_str(config_toml).expect("plugin test config should parse"),
)],
ConfigRequirements::default(),
ConfigRequirementsToml::default(),
)
.expect("config layer stack should build");
PluginsManager::new(codex_home.to_path_buf()).plugins_for_layer_stack(codex_home, &stack, false)
let config = load_config_blocking(codex_home, codex_home);
PluginsManager::new(codex_home.to_path_buf()).plugins_for_config(&config)
}
async fn load_config(codex_home: &Path, cwd: &Path) -> crate::config::Config {
@@ -82,6 +72,14 @@ async fn load_config(codex_home: &Path, cwd: &Path) -> crate::config::Config {
.expect("config should load")
}
fn load_config_blocking(codex_home: &Path, cwd: &Path) -> crate::config::Config {
tokio::runtime::Builder::new_current_thread()
.enable_all()
.build()
.expect("tokio runtime should build")
.block_on(load_config(codex_home, cwd))
}
#[test]
fn load_plugins_loads_default_skills_and_mcp_servers() {
let codex_home = TempDir::new().unwrap();
@@ -749,8 +747,13 @@ fn load_plugins_returns_empty_when_feature_disabled() {
&plugin_root.join("skills/sample-search/SKILL.md"),
"---\nname: sample-search\ndescription: search sample data\n---\n",
);
write_file(
&codex_home.path().join(CONFIG_TOML_FILE),
&plugin_config_toml(true, false),
);
let outcome = load_plugins_from_config(&plugin_config_toml(true, false), codex_home.path());
let config = load_config_blocking(codex_home.path(), codex_home.path());
let outcome = PluginsManager::new(codex_home.path().to_path_buf()).plugins_for_config(&config);
assert_eq!(outcome, PluginLoadOutcome::default());
}
@@ -1000,12 +1003,125 @@ enabled = false
);
}
#[tokio::test]
async fn list_marketplaces_returns_empty_when_feature_disabled() {
let tmp = tempfile::tempdir().unwrap();
let repo_root = tmp.path().join("repo");
fs::create_dir_all(repo_root.join(".git")).unwrap();
fs::create_dir_all(repo_root.join(".agents/plugins")).unwrap();
fs::write(
repo_root.join(".agents/plugins/marketplace.json"),
r#"{
"name": "debug",
"plugins": [
{
"name": "enabled-plugin",
"source": {
"source": "local",
"path": "./enabled-plugin"
}
}
]
}"#,
)
.unwrap();
write_file(
&tmp.path().join(CONFIG_TOML_FILE),
r#"[features]
plugins = false
[plugins."enabled-plugin@debug"]
enabled = true
"#,
);
let config = load_config(tmp.path(), &repo_root).await;
let marketplaces = PluginsManager::new(tmp.path().to_path_buf())
.list_marketplaces_for_config(&config, &[AbsolutePathBuf::try_from(repo_root).unwrap()])
.unwrap();
assert_eq!(marketplaces, Vec::new());
}
#[tokio::test]
async fn read_plugin_for_config_returns_plugins_disabled_when_feature_disabled() {
let tmp = tempfile::tempdir().unwrap();
let repo_root = tmp.path().join("repo");
fs::create_dir_all(repo_root.join(".git")).unwrap();
fs::create_dir_all(repo_root.join(".agents/plugins")).unwrap();
let marketplace_path =
AbsolutePathBuf::try_from(repo_root.join(".agents/plugins/marketplace.json")).unwrap();
fs::write(
marketplace_path.as_path(),
r#"{
"name": "debug",
"plugins": [
{
"name": "enabled-plugin",
"source": {
"source": "local",
"path": "./enabled-plugin"
}
}
]
}"#,
)
.unwrap();
write_file(
&tmp.path().join(CONFIG_TOML_FILE),
r#"[features]
plugins = false
[plugins."enabled-plugin@debug"]
enabled = true
"#,
);
let config = load_config(tmp.path(), &repo_root).await;
let err = PluginsManager::new(tmp.path().to_path_buf())
.read_plugin_for_config(
&config,
&PluginReadRequest {
plugin_name: "enabled-plugin".to_string(),
marketplace_path,
},
)
.unwrap_err();
assert!(matches!(err, MarketplaceError::PluginsDisabled));
}
#[tokio::test]
async fn sync_plugins_from_remote_returns_default_when_feature_disabled() {
let tmp = tempfile::tempdir().unwrap();
write_file(
&tmp.path().join(CONFIG_TOML_FILE),
r#"[features]
plugins = false
"#,
);
let config = load_config(tmp.path(), tmp.path()).await;
let outcome = PluginsManager::new(tmp.path().to_path_buf())
.sync_plugins_from_remote(&config, None)
.await
.unwrap();
assert_eq!(outcome, RemotePluginSyncResult::default());
}
#[tokio::test]
async fn list_marketplaces_includes_curated_repo_marketplace() {
let tmp = tempfile::tempdir().unwrap();
let curated_root = curated_plugins_repo_path(tmp.path());
let plugin_root = curated_root.join("plugins/linear");
write_file(
&tmp.path().join(CONFIG_TOML_FILE),
r#"[features]
plugins = true
"#,
);
fs::create_dir_all(curated_root.join(".agents/plugins")).unwrap();
fs::create_dir_all(plugin_root.join(".codex-plugin")).unwrap();
fs::write(
@@ -1723,11 +1839,8 @@ fn load_plugins_ignores_project_config_files() {
)
.expect("config layer stack should build");
let outcome = PluginsManager::new(codex_home.path().to_path_buf()).plugins_for_layer_stack(
&project_root,
&stack,
false,
);
let outcome =
load_plugins_from_layer_stack(&stack, &PluginStore::new(codex_home.path().to_path_buf()));
assert_eq!(outcome, PluginLoadOutcome::default());
}
codex-rs/core/src/plugins/marketplace.rs
+14 -1
View File
@@ -14,6 +14,7 @@ use std::io;
use std::path::Component;
use std::path::Path;
use std::path::PathBuf;
use tracing::warn;
const MARKETPLACE_RELATIVE_PATH: &str = ".agents/plugins/marketplace.json";
@@ -127,6 +128,9 @@ pub enum MarketplaceError {
marketplace_name: String,
},
#[error("plugins feature is disabled")]
PluginsDisabled,
#[error("{0}")]
InvalidPlugin(String),
}
@@ -238,7 +242,16 @@ fn list_marketplaces_with_home(
let mut marketplaces = Vec::new();
for marketplace_path in discover_marketplace_paths_from_roots(additional_roots, home_dir) {
marketplaces.push(load_marketplace(&marketplace_path)?);
match load_marketplace(&marketplace_path) {
Ok(marketplace) => marketplaces.push(marketplace),
Err(err) => {
warn!(
path = %marketplace_path.display(),
error = %err,
"skipping marketplace that failed to load"
);
}
}
}
Ok(marketplaces)
codex-rs/core/src/plugins/marketplace_tests.rs
+56
View File
@@ -403,6 +403,62 @@ fn list_marketplaces_reads_marketplace_display_name() {
);
}
#[test]
fn list_marketplaces_skips_marketplaces_that_fail_to_load() {
let tmp = tempdir().unwrap();
let valid_repo_root = tmp.path().join("valid-repo");
let invalid_repo_root = tmp.path().join("invalid-repo");
fs::create_dir_all(valid_repo_root.join(".git")).unwrap();
fs::create_dir_all(valid_repo_root.join(".agents/plugins")).unwrap();
fs::create_dir_all(invalid_repo_root.join(".git")).unwrap();
fs::create_dir_all(invalid_repo_root.join(".agents/plugins")).unwrap();
fs::write(
valid_repo_root.join(".agents/plugins/marketplace.json"),
r#"{
"name": "valid-marketplace",
"plugins": [
{
"name": "valid-plugin",
"source": {
"source": "local",
"path": "./plugin"
}
}
]
}"#,
)
.unwrap();
fs::write(
invalid_repo_root.join(".agents/plugins/marketplace.json"),
r#"{
"name": "invalid-marketplace",
"plugins": [
{
"name": "broken-plugin",
"source": {
"source": "local",
"path": "plugin-without-dot-slash"
}
}
]
}"#,
)
.unwrap();
let marketplaces = list_marketplaces_with_home(
&[
AbsolutePathBuf::try_from(valid_repo_root).unwrap(),
AbsolutePathBuf::try_from(invalid_repo_root).unwrap(),
],
None,
)
.unwrap();
assert_eq!(marketplaces.len(), 1);
assert_eq!(marketplaces[0].name, "valid-marketplace");
}
#[test]
fn list_marketplaces_resolves_plugin_interface_paths_to_absolute() {
let tmp = tempdir().unwrap();