mirror of
https://github.com/openai/codex.git
synced 2026-08-23 13:09:46 +00:00
Restrict plugin migration to home scope (#39663)
## Why Plugin imports persist user-global enabled state. Repository-controlled settings must not be allowed to select executable plugin content for installation. ## What changed - Detect plugin migrations only from home-scoped external agent settings. - Reject plugin imports with a non-empty project `cwd` before loading Codex configuration, while continuing to treat an empty `cwd` as home scope. ## Testing - Cover repository-scoped detection with remote, installed, and project-relative marketplaces. - Verify that forged project-scoped imports fail without writing `config.toml`. GitOrigin-RevId: 53cb46ae049cb49283187312d475d40cf42e35ab
This commit is contained in:
committed by
copyberry
parent
240bbfc14a
commit
97c82c0900
@@ -327,7 +327,9 @@ impl ExternalAgentConfigService {
|
||||
);
|
||||
}
|
||||
|
||||
if self.source.supports_plugin_migration(settings.as_ref()) {
|
||||
// Plugin import persists user-global enabled state, so repository-controlled
|
||||
// settings must never be treated as plugin installation authority.
|
||||
if scope.is_home() && self.source.supports_plugin_migration(settings.as_ref()) {
|
||||
match ConfigBuilder::default()
|
||||
.codex_home(self.codex_home.clone())
|
||||
.fallback_cwd(Some(self.codex_home.clone()))
|
||||
|
||||
@@ -27,16 +27,18 @@ impl ExternalAgentConfigService {
|
||||
&self,
|
||||
cwd: Option<&Path>,
|
||||
) -> io::Result<BTreeMap<String, MarketplaceImportSource>> {
|
||||
let Some(scope) = MigrationScope::from_cwd(cwd)? else {
|
||||
return Ok(BTreeMap::new());
|
||||
};
|
||||
let source_root = scope
|
||||
.repo_root()
|
||||
.unwrap_or(self.external_agent_home.as_path());
|
||||
// Plugin installs are user-global. Only missing or empty `cwd` values
|
||||
// denote home scope; a project scope must not select executable content.
|
||||
if cwd.is_some_and(|cwd| !cwd.as_os_str().is_empty()) {
|
||||
return Err(invalid_data_error(
|
||||
"repository-scoped plugin migration is not allowed".to_string(),
|
||||
));
|
||||
}
|
||||
let scope = MigrationScope::home();
|
||||
let source_settings = self.source_settings(&scope);
|
||||
self.source.marketplace_import_sources(
|
||||
self.external_agent_home.as_path(),
|
||||
source_root,
|
||||
self.external_agent_home.as_path(),
|
||||
&source_settings,
|
||||
)
|
||||
}
|
||||
@@ -86,11 +88,13 @@ impl ExternalAgentConfigService {
|
||||
cwd: Option<&Path>,
|
||||
details: Option<MigrationDetails>,
|
||||
) -> io::Result<PluginImportOutcome> {
|
||||
let cwd = cwd.filter(|cwd| !cwd.as_os_str().is_empty());
|
||||
let Some(MigrationDetails { plugins, .. }) = details else {
|
||||
return Err(invalid_data_error(
|
||||
"plugins migration item is missing details".to_string(),
|
||||
));
|
||||
};
|
||||
let import_sources = self.marketplace_import_sources(cwd)?;
|
||||
let config = ConfigBuilder::default()
|
||||
.codex_home(self.codex_home.clone())
|
||||
.fallback_cwd(Some(
|
||||
@@ -120,7 +124,6 @@ impl ExternalAgentConfigService {
|
||||
.into_iter()
|
||||
.map(|marketplace| (marketplace.name, marketplace.path))
|
||||
.collect::<BTreeMap<_, _>>();
|
||||
let import_sources = self.marketplace_import_sources(cwd)?;
|
||||
for plugin_group in plugins {
|
||||
let marketplace_name = plugin_group.marketplace_name.clone();
|
||||
let plugin_names = plugin_group.plugin_names;
|
||||
|
||||
@@ -394,7 +394,7 @@ async fn detect_home_plugins_uses_local_settings_over_project_settings() {
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn detect_repo_skips_plugins_that_are_already_configured_in_codex() {
|
||||
async fn detect_repo_skips_plugins_from_remote_marketplace() {
|
||||
let root = TempDir::new().expect("create tempdir");
|
||||
let external_agent_home = root.path().join(EXTERNAL_AGENT_DIR);
|
||||
let codex_home = root.path().join(".codex");
|
||||
@@ -435,27 +435,7 @@ enabled = true
|
||||
.await
|
||||
.expect("detect");
|
||||
|
||||
assert_eq!(
|
||||
items,
|
||||
vec![ExternalAgentConfigMigrationItem {
|
||||
item_type: ExternalAgentConfigMigrationItemType::Plugins,
|
||||
description: format!(
|
||||
"Migrate enabled plugins from {}",
|
||||
repo_root
|
||||
.join(EXTERNAL_AGENT_DIR)
|
||||
.join("settings.json")
|
||||
.display()
|
||||
),
|
||||
cwd: Some(repo_root),
|
||||
details: Some(MigrationDetails {
|
||||
plugins: vec![PluginsMigration {
|
||||
marketplace_name: "acme-tools".to_string(),
|
||||
plugin_names: vec!["deployer".to_string()],
|
||||
}],
|
||||
..Default::default()
|
||||
}),
|
||||
}]
|
||||
);
|
||||
assert_eq!(items, Vec::<ExternalAgentConfigMigrationItem>::new());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
@@ -559,7 +539,7 @@ async fn import_plugins_requires_details() {
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn detect_repo_does_not_skip_plugins_only_configured_in_project_codex() {
|
||||
async fn detect_repo_skips_plugins_only_configured_in_project_codex() {
|
||||
let root = TempDir::new().expect("create tempdir");
|
||||
let external_agent_home = root.path().join(EXTERNAL_AGENT_DIR);
|
||||
let codex_home = root.path().join(".codex");
|
||||
@@ -600,27 +580,7 @@ enabled = true
|
||||
.await
|
||||
.expect("detect");
|
||||
|
||||
assert_eq!(
|
||||
items,
|
||||
vec![ExternalAgentConfigMigrationItem {
|
||||
item_type: ExternalAgentConfigMigrationItemType::Plugins,
|
||||
description: format!(
|
||||
"Migrate enabled plugins from {}",
|
||||
repo_root
|
||||
.join(EXTERNAL_AGENT_DIR)
|
||||
.join("settings.json")
|
||||
.display()
|
||||
),
|
||||
cwd: Some(repo_root),
|
||||
details: Some(MigrationDetails {
|
||||
plugins: vec![PluginsMigration {
|
||||
marketplace_name: "acme-tools".to_string(),
|
||||
plugin_names: vec!["formatter".to_string()],
|
||||
}],
|
||||
..Default::default()
|
||||
}),
|
||||
}]
|
||||
);
|
||||
assert_eq!(items, Vec::<ExternalAgentConfigMigrationItem>::new());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
@@ -681,7 +641,7 @@ async fn detect_home_skips_plugins_with_invalid_marketplace_source() {
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn detect_repo_filters_plugins_against_installed_marketplace() {
|
||||
async fn detect_repo_skips_plugins_even_with_installed_marketplace() {
|
||||
let root = TempDir::new().expect("create tempdir");
|
||||
let external_agent_home = root.path().join(EXTERNAL_AGENT_DIR);
|
||||
let codex_home = root.path().join(".codex");
|
||||
@@ -787,25 +747,5 @@ source = "owner/debug-marketplace"
|
||||
.await
|
||||
.expect("detect");
|
||||
|
||||
assert_eq!(
|
||||
items,
|
||||
vec![ExternalAgentConfigMigrationItem {
|
||||
item_type: ExternalAgentConfigMigrationItemType::Plugins,
|
||||
description: format!(
|
||||
"Migrate enabled plugins from {}",
|
||||
repo_root
|
||||
.join(EXTERNAL_AGENT_DIR)
|
||||
.join("settings.json")
|
||||
.display()
|
||||
),
|
||||
cwd: Some(repo_root),
|
||||
details: Some(MigrationDetails {
|
||||
plugins: vec![PluginsMigration {
|
||||
marketplace_name: "debug".to_string(),
|
||||
plugin_names: vec!["available".to_string()],
|
||||
}],
|
||||
..Default::default()
|
||||
}),
|
||||
}]
|
||||
);
|
||||
assert_eq!(items, Vec::<ExternalAgentConfigMigrationItem>::new());
|
||||
}
|
||||
|
||||
@@ -91,7 +91,7 @@ async fn import_plugins_defers_marketplace_source_validation_to_add_marketplace(
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn import_plugins_supports_external_agent_plugin_marketplace_layout() {
|
||||
async fn import_plugins_treats_empty_cwd_as_home_scope() {
|
||||
let (_root, external_agent_home, codex_home) = fixture_paths();
|
||||
let marketplace_root = external_agent_home.join("my-marketplace");
|
||||
let plugin_root = marketplace_root.join("plugins").join("cloudflare");
|
||||
@@ -139,7 +139,7 @@ async fn import_plugins_supports_external_agent_plugin_marketplace_layout() {
|
||||
|
||||
let outcome = service_for_paths(external_agent_home, codex_home.clone())
|
||||
.import_plugins(
|
||||
/*cwd*/ None,
|
||||
/*cwd*/ Some(std::path::Path::new("")),
|
||||
Some(MigrationDetails {
|
||||
plugins: vec![PluginsMigration {
|
||||
marketplace_name: "my-plugins".to_string(),
|
||||
@@ -529,7 +529,7 @@ async fn import_plugins_infers_external_official_marketplace_when_missing_from_s
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn detect_repo_supports_project_relative_external_agent_plugin_marketplace_path() {
|
||||
async fn detect_repo_skips_project_relative_external_agent_plugin_marketplace_path() {
|
||||
let root = TempDir::new().expect("create tempdir");
|
||||
let external_agent_home = root.path().join(EXTERNAL_AGENT_DIR);
|
||||
let codex_home = root.path().join(".codex");
|
||||
@@ -588,31 +588,11 @@ async fn detect_repo_supports_project_relative_external_agent_plugin_marketplace
|
||||
.await
|
||||
.expect("detect");
|
||||
|
||||
assert_eq!(
|
||||
items,
|
||||
vec![ExternalAgentConfigMigrationItem {
|
||||
item_type: ExternalAgentConfigMigrationItemType::Plugins,
|
||||
description: format!(
|
||||
"Migrate enabled plugins from {}",
|
||||
repo_root
|
||||
.join(EXTERNAL_AGENT_DIR)
|
||||
.join("settings.json")
|
||||
.display()
|
||||
),
|
||||
cwd: Some(repo_root),
|
||||
details: Some(MigrationDetails {
|
||||
plugins: vec![PluginsMigration {
|
||||
marketplace_name: "my-plugins".to_string(),
|
||||
plugin_names: vec!["cloudflare".to_string()],
|
||||
}],
|
||||
..Default::default()
|
||||
}),
|
||||
}]
|
||||
);
|
||||
assert_eq!(items, Vec::<ExternalAgentConfigMigrationItem>::new());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn import_plugins_supports_project_relative_external_agent_plugin_marketplace_path() {
|
||||
async fn import_rejects_forged_project_relative_external_agent_plugin_item() {
|
||||
let root = TempDir::new().expect("create tempdir");
|
||||
let external_agent_home = root.path().join(EXTERNAL_AGENT_DIR);
|
||||
let codex_home = root.path().join(".codex");
|
||||
@@ -663,32 +643,66 @@ async fn import_plugins_supports_project_relative_external_agent_plugin_marketpl
|
||||
.expect("write plugin manifest");
|
||||
|
||||
let outcome = service_for_paths(external_agent_home, codex_home.clone())
|
||||
.import_plugins(
|
||||
Some(repo_root.as_path()),
|
||||
Some(MigrationDetails {
|
||||
.import(vec![ExternalAgentConfigMigrationItem {
|
||||
item_type: ExternalAgentConfigMigrationItemType::Plugins,
|
||||
description: String::new(),
|
||||
cwd: Some(repo_root.clone()),
|
||||
details: Some(MigrationDetails {
|
||||
plugins: vec![PluginsMigration {
|
||||
marketplace_name: "my-plugins".to_string(),
|
||||
plugin_names: vec!["cloudflare".to_string()],
|
||||
}],
|
||||
..Default::default()
|
||||
}),
|
||||
)
|
||||
.await
|
||||
.expect("import plugins");
|
||||
}])
|
||||
.await;
|
||||
|
||||
assert_eq!(
|
||||
outcome,
|
||||
PluginImportOutcome {
|
||||
succeeded_marketplaces: vec!["my-plugins".to_string()],
|
||||
succeeded_plugin_ids: vec!["cloudflare@my-plugins".to_string()],
|
||||
failed_marketplaces: Vec::new(),
|
||||
failed_plugin_ids: Vec::new(),
|
||||
raw_errors: Vec::new(),
|
||||
ExternalAgentConfigImportOutcome {
|
||||
pending_plugin_imports: Vec::new(),
|
||||
item_results: vec![ExternalAgentConfigImportItemResult {
|
||||
item_type: ExternalAgentConfigMigrationItemType::Plugins,
|
||||
description: String::new(),
|
||||
cwd: Some(repo_root.clone()),
|
||||
success_count: 0,
|
||||
error_count: 1,
|
||||
successes: Vec::new(),
|
||||
raw_errors: vec![ExternalAgentConfigImportRawError {
|
||||
item_type: ExternalAgentConfigMigrationItemType::Plugins,
|
||||
error_type: None,
|
||||
sub_error_type: None,
|
||||
failure_stage: "plugin_import".to_string(),
|
||||
message: "repository-scoped plugin migration is not allowed".to_string(),
|
||||
cwd: Some(repo_root),
|
||||
source: None,
|
||||
}],
|
||||
}],
|
||||
}
|
||||
);
|
||||
let config = fs::read_to_string(codex_home.join("config.toml")).expect("read config");
|
||||
assert!(config.contains(r#"[plugins."cloudflare@my-plugins"]"#));
|
||||
assert!(config.contains("enabled = true"));
|
||||
assert!(!codex_home.join("config.toml").exists());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn import_plugins_rejects_project_cwd_before_config_loading() {
|
||||
let root = TempDir::new().expect("create tempdir");
|
||||
let external_agent_home = root.path().join(EXTERNAL_AGENT_DIR);
|
||||
let codex_home = root.path().join(".codex");
|
||||
let repo_root = root.path().join("repo");
|
||||
fs::create_dir_all(repo_root.join(".git")).expect("create git dir");
|
||||
fs::create_dir_all(&codex_home).expect("create codex home");
|
||||
fs::write(codex_home.join("config.toml"), "not valid toml").expect("write invalid config");
|
||||
|
||||
let error = service_for_paths(external_agent_home, codex_home)
|
||||
.import_plugins(Some(repo_root.as_path()), Some(github_plugin_details()))
|
||||
.await
|
||||
.expect_err("reject project cwd before config loading");
|
||||
|
||||
assert_eq!(error.kind(), io::ErrorKind::InvalidData);
|
||||
assert_eq!(
|
||||
error.to_string(),
|
||||
"repository-scoped plugin migration is not allowed"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
Reference in New Issue
Block a user