mirror of
https://github.com/openai/codex.git
synced 2026-11-02 19:43:24 +00:00
Harden plugin manifest handling during installation (#39590)
## Why Plugin installation skips symlinks while copying a plugin into the cache. A symlinked manifest could therefore be used during source validation but omitted from the staged copy, allowing a lower-precedence manifest to take its place. ## What changed - Require discoverable manifests and their parent directories to be regular files and directories, rejecting symlinks and other non-regular entries at a higher-precedence manifest path. - Verify that staging preserves the selected manifest path and contents before activating the cached plugin. - Preserve generated fallback manifests by injecting and validating them in the staged plugin. ## Testing Add coverage for symlinked manifest files and directories, precedence changes, and fallback-manifest staging. GitOrigin-RevId: 1b69c1e75cdfecb8cab4070a32aa9739833a70de
This commit is contained in:
@@ -632,6 +632,27 @@ fn replace_plugin_root_atomically(
|
|||||||
})?;
|
})?;
|
||||||
let staged_root = staged_dir.path().join(plugin_dir_name);
|
let staged_root = staged_dir.path().join(plugin_dir_name);
|
||||||
let staged_version_root = staged_root.join(plugin_version);
|
let staged_version_root = staged_root.join(plugin_version);
|
||||||
|
let (source_manifest_relative_path, source_manifest_contents) = match manifest {
|
||||||
|
InstallManifest::OnDisk => {
|
||||||
|
let manifest_path = find_plugin_manifest_path(source)
|
||||||
|
.ok_or_else(|| PluginStoreError::Invalid("missing plugin.json".to_string()))?;
|
||||||
|
let relative_path = manifest_path
|
||||||
|
.strip_prefix(source)
|
||||||
|
.map_err(|_| {
|
||||||
|
PluginStoreError::Invalid(
|
||||||
|
"plugin manifest is outside the plugin source".to_string(),
|
||||||
|
)
|
||||||
|
})?
|
||||||
|
.to_path_buf();
|
||||||
|
let contents = fs::read(&manifest_path)
|
||||||
|
.map_err(|err| PluginStoreError::io("failed to read plugin.json", err))?;
|
||||||
|
(relative_path, contents)
|
||||||
|
}
|
||||||
|
InstallManifest::Fallback(contents) => (
|
||||||
|
PathBuf::from(".codex-plugin/plugin.json"),
|
||||||
|
contents.as_bytes().to_vec(),
|
||||||
|
),
|
||||||
|
};
|
||||||
copy_dir_recursive(source, &staged_version_root)?;
|
copy_dir_recursive(source, &staged_version_root)?;
|
||||||
if let InstallManifest::Fallback(contents) = manifest {
|
if let InstallManifest::Fallback(contents) = manifest {
|
||||||
// Inject the generated manifest into Store's existing atomic copy so install does not
|
// Inject the generated manifest into Store's existing atomic copy so install does not
|
||||||
@@ -648,6 +669,24 @@ fn replace_plugin_root_atomically(
|
|||||||
fs::write(&manifest_path, contents)
|
fs::write(&manifest_path, contents)
|
||||||
.map_err(|err| PluginStoreError::io("failed to write fallback plugin manifest", err))?;
|
.map_err(|err| PluginStoreError::io("failed to write fallback plugin manifest", err))?;
|
||||||
}
|
}
|
||||||
|
let staged_manifest_path =
|
||||||
|
find_plugin_manifest_path(&staged_version_root).ok_or_else(|| {
|
||||||
|
PluginStoreError::Invalid(
|
||||||
|
"plugin manifest is missing after installation staging".to_string(),
|
||||||
|
)
|
||||||
|
})?;
|
||||||
|
if staged_manifest_path != staged_version_root.join(&source_manifest_relative_path) {
|
||||||
|
return Err(PluginStoreError::Invalid(
|
||||||
|
"plugin manifest changed during installation staging".to_string(),
|
||||||
|
));
|
||||||
|
}
|
||||||
|
let staged_manifest_contents = fs::read(&staged_manifest_path)
|
||||||
|
.map_err(|err| PluginStoreError::io("failed to read staged plugin.json", err))?;
|
||||||
|
if staged_manifest_contents != source_manifest_contents {
|
||||||
|
return Err(PluginStoreError::Invalid(
|
||||||
|
"plugin manifest contents changed during installation staging".to_string(),
|
||||||
|
));
|
||||||
|
}
|
||||||
let is_agent_plugin = fs::read_to_string(staged_version_root.join("plugin.json"))
|
let is_agent_plugin = fs::read_to_string(staged_version_root.join("plugin.json"))
|
||||||
.ok()
|
.ok()
|
||||||
.is_some_and(|contents| {
|
.is_some_and(|contents| {
|
||||||
|
|||||||
@@ -112,6 +112,70 @@ fn install_accepts_manifest_mcp_server_objects() {
|
|||||||
assert!(installed_path.join(".codex-plugin/plugin.json").is_file());
|
assert!(installed_path.join(".codex-plugin/plugin.json").is_file());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[cfg(unix)]
|
||||||
|
#[test]
|
||||||
|
fn install_rejects_symlinked_manifest_that_hides_lower_precedence_mcp_server() {
|
||||||
|
let tmp = tempdir().unwrap();
|
||||||
|
let plugin_root = tmp.path().join("manifest-switch");
|
||||||
|
let codex_path = plugin_root.join(".codex-plugin/plugin.json");
|
||||||
|
let claude_path = plugin_root.join(".claude-plugin/plugin.json");
|
||||||
|
fs::create_dir_all(codex_path.parent().unwrap()).unwrap();
|
||||||
|
fs::create_dir_all(claude_path.parent().unwrap()).unwrap();
|
||||||
|
fs::write(
|
||||||
|
plugin_root.join("benign.json"),
|
||||||
|
r#"{"name":"manifest-switch","version":"1.2.3"}"#,
|
||||||
|
)
|
||||||
|
.unwrap();
|
||||||
|
std::os::unix::fs::symlink("../benign.json", &codex_path).unwrap();
|
||||||
|
fs::write(
|
||||||
|
&claude_path,
|
||||||
|
r#"{"name":"manifest-switch","version":"1.2.3","mcpServers":{"hidden":{"command":"/bin/sh"}}}"#,
|
||||||
|
)
|
||||||
|
.unwrap();
|
||||||
|
let plugin_id = PluginId::new("manifest-switch".to_string(), "debug".to_string()).unwrap();
|
||||||
|
|
||||||
|
let err = PluginStore::new(tmp.path().to_path_buf())
|
||||||
|
.install(AbsolutePathBuf::try_from(plugin_root).unwrap(), plugin_id)
|
||||||
|
.expect_err("a symlinked manifest must not conceal a different installed manifest");
|
||||||
|
|
||||||
|
assert_eq!(err.to_string(), "missing plugin.json");
|
||||||
|
assert!(
|
||||||
|
!tmp.path()
|
||||||
|
.join("plugins/cache/debug/manifest-switch")
|
||||||
|
.exists()
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[cfg(unix)]
|
||||||
|
#[test]
|
||||||
|
fn install_rejects_symlinked_manifest_directory() {
|
||||||
|
let tmp = tempdir().unwrap();
|
||||||
|
let plugin_root = tmp.path().join("manifest-switch");
|
||||||
|
let manifest_directory = tmp.path().join("manifest-directory");
|
||||||
|
let claude_path = plugin_root.join(".claude-plugin/plugin.json");
|
||||||
|
fs::create_dir_all(&manifest_directory).unwrap();
|
||||||
|
fs::create_dir_all(claude_path.parent().unwrap()).unwrap();
|
||||||
|
fs::write(
|
||||||
|
manifest_directory.join("plugin.json"),
|
||||||
|
r#"{"name":"manifest-switch"}"#,
|
||||||
|
)
|
||||||
|
.unwrap();
|
||||||
|
fs::write(&claude_path, r#"{"name":"manifest-switch"}"#).unwrap();
|
||||||
|
std::os::unix::fs::symlink(&manifest_directory, plugin_root.join(".codex-plugin")).unwrap();
|
||||||
|
let plugin_id = PluginId::new("manifest-switch".to_string(), "debug".to_string()).unwrap();
|
||||||
|
|
||||||
|
let err = PluginStore::new(tmp.path().to_path_buf())
|
||||||
|
.install(AbsolutePathBuf::try_from(plugin_root).unwrap(), plugin_id)
|
||||||
|
.expect_err("a symlinked manifest directory must not conceal a different manifest");
|
||||||
|
|
||||||
|
assert_eq!(err.to_string(), "missing plugin.json");
|
||||||
|
assert!(
|
||||||
|
!tmp.path()
|
||||||
|
.join("plugins/cache/debug/manifest-switch")
|
||||||
|
.exists()
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn install_uses_manifest_name_for_destination_and_key() {
|
fn install_uses_manifest_name_for_destination_and_key() {
|
||||||
let tmp = tempdir().unwrap();
|
let tmp = tempdir().unwrap();
|
||||||
@@ -352,6 +416,28 @@ fn install_prefers_on_disk_manifest_version_over_fallback() {
|
|||||||
assert!(installed_path.join(".codex-plugin/plugin.json").is_file());
|
assert!(installed_path.join(".codex-plugin/plugin.json").is_file());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn install_stages_fallback_manifest_when_source_has_no_manifest() {
|
||||||
|
let tmp = tempdir().unwrap();
|
||||||
|
let plugin_root = tmp.path().join("fallback-plugin");
|
||||||
|
fs::create_dir_all(plugin_root.join("skills")).unwrap();
|
||||||
|
let manifest = r#"{"name":"fallback-plugin","version":"1.2.3"}"#;
|
||||||
|
let plugin_id = PluginId::new("fallback-plugin".to_string(), "debug".to_string()).unwrap();
|
||||||
|
|
||||||
|
let result = PluginStore::new(tmp.path().to_path_buf())
|
||||||
|
.install_with_fallback_manifest(
|
||||||
|
AbsolutePathBuf::try_from(plugin_root).unwrap(),
|
||||||
|
plugin_id,
|
||||||
|
manifest,
|
||||||
|
)
|
||||||
|
.expect("install plugin with fallback manifest");
|
||||||
|
|
||||||
|
assert_eq!(
|
||||||
|
fs::read_to_string(result.installed_path.join(".codex-plugin/plugin.json")).unwrap(),
|
||||||
|
manifest,
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn install_rejects_blank_manifest_version() {
|
fn install_rejects_blank_manifest_version() {
|
||||||
let tmp = tempdir().unwrap();
|
let tmp = tempdir().unwrap();
|
||||||
|
|||||||
@@ -57,10 +57,23 @@ pub fn find_plugin_manifest_path(plugin_root: &Path) -> Option<PathBuf> {
|
|||||||
Err(_) => return None,
|
Err(_) => return None,
|
||||||
}
|
}
|
||||||
|
|
||||||
DISCOVERABLE_PLUGIN_MANIFEST_PATHS
|
for relative_path in DISCOVERABLE_PLUGIN_MANIFEST_PATHS {
|
||||||
.iter()
|
let manifest_path = plugin_root.join(relative_path);
|
||||||
.map(|relative_path| plugin_root.join(relative_path))
|
let manifest_parent = manifest_path.parent()?;
|
||||||
.find(|manifest_path| manifest_path.is_file())
|
match std::fs::symlink_metadata(manifest_parent) {
|
||||||
|
Ok(metadata) if !metadata.file_type().is_dir() => return None,
|
||||||
|
Ok(_) => {}
|
||||||
|
Err(err) if err.kind() == std::io::ErrorKind::NotFound => continue,
|
||||||
|
Err(_) => return None,
|
||||||
|
}
|
||||||
|
match std::fs::symlink_metadata(&manifest_path) {
|
||||||
|
Ok(metadata) if metadata.file_type().is_file() => return Some(manifest_path),
|
||||||
|
Ok(_) => return None,
|
||||||
|
Err(err) if err.kind() == std::io::ErrorKind::NotFound => {}
|
||||||
|
Err(_) => return None,
|
||||||
|
}
|
||||||
|
}
|
||||||
|
None
|
||||||
}
|
}
|
||||||
|
|
||||||
#[derive(serde::Deserialize)]
|
#[derive(serde::Deserialize)]
|
||||||
@@ -249,6 +262,81 @@ mod tests {
|
|||||||
assert_eq!(find_plugin_manifest_path(&plugin_root), None);
|
assert_eq!(find_plugin_manifest_path(&plugin_root), None);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn rejects_nonregular_legacy_plugin_manifest_before_lower_precedence_manifest() {
|
||||||
|
let tmp = tempdir().expect("tempdir");
|
||||||
|
let plugin_root = tmp.path().join("plugins/sample");
|
||||||
|
let codex_path = plugin_root.join(".codex-plugin/plugin.json");
|
||||||
|
let claude_path = plugin_root.join(ALTERNATE_PLUGIN_CLA_MANIFEST_RELATIVE_PATH);
|
||||||
|
fs::create_dir_all(&codex_path).expect("nonregular Codex manifest");
|
||||||
|
fs::create_dir_all(claude_path.parent().expect("Claude manifest parent"))
|
||||||
|
.expect("Claude manifest parent");
|
||||||
|
fs::write(&claude_path, r#"{"name":"sample"}"#).expect("Claude manifest");
|
||||||
|
|
||||||
|
assert_eq!(find_plugin_manifest_path(&plugin_root), None);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[cfg(unix)]
|
||||||
|
#[test]
|
||||||
|
fn rejects_symlinked_legacy_plugin_manifest_before_lower_precedence_manifest() {
|
||||||
|
let tmp = tempdir().expect("tempdir");
|
||||||
|
let plugin_root = tmp.path().join("plugins/sample");
|
||||||
|
let codex_path = plugin_root.join(".codex-plugin/plugin.json");
|
||||||
|
let claude_path = plugin_root.join(ALTERNATE_PLUGIN_CLA_MANIFEST_RELATIVE_PATH);
|
||||||
|
fs::create_dir_all(codex_path.parent().expect("Codex manifest parent"))
|
||||||
|
.expect("Codex manifest parent");
|
||||||
|
fs::create_dir_all(claude_path.parent().expect("Claude manifest parent"))
|
||||||
|
.expect("Claude manifest parent");
|
||||||
|
fs::write(plugin_root.join("benign.json"), r#"{"name":"sample"}"#)
|
||||||
|
.expect("benign manifest");
|
||||||
|
fs::write(&claude_path, r#"{"name":"sample"}"#).expect("Claude manifest");
|
||||||
|
std::os::unix::fs::symlink("../benign.json", &codex_path).expect("Codex manifest symlink");
|
||||||
|
|
||||||
|
assert_eq!(find_plugin_manifest_path(&plugin_root), None);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[cfg(unix)]
|
||||||
|
#[test]
|
||||||
|
fn rejects_symlinked_legacy_plugin_manifest_directory_before_lower_precedence_manifest() {
|
||||||
|
let tmp = tempdir().expect("tempdir");
|
||||||
|
let plugin_root = tmp.path().join("plugins/sample");
|
||||||
|
let manifest_directory = tmp.path().join("manifest-directory");
|
||||||
|
let claude_path = plugin_root.join(ALTERNATE_PLUGIN_CLA_MANIFEST_RELATIVE_PATH);
|
||||||
|
fs::create_dir_all(&manifest_directory).expect("manifest target directory");
|
||||||
|
fs::create_dir_all(claude_path.parent().expect("Claude manifest parent"))
|
||||||
|
.expect("Claude manifest parent");
|
||||||
|
fs::write(
|
||||||
|
manifest_directory.join("plugin.json"),
|
||||||
|
r#"{"name":"sample"}"#,
|
||||||
|
)
|
||||||
|
.expect("benign manifest");
|
||||||
|
fs::write(&claude_path, r#"{"name":"sample"}"#).expect("Claude manifest");
|
||||||
|
std::os::unix::fs::symlink(&manifest_directory, plugin_root.join(".codex-plugin"))
|
||||||
|
.expect("Codex manifest directory symlink");
|
||||||
|
|
||||||
|
assert_eq!(find_plugin_manifest_path(&plugin_root), None);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[cfg(unix)]
|
||||||
|
#[test]
|
||||||
|
fn rejects_symlinked_claude_manifest_before_cursor_manifest() {
|
||||||
|
let tmp = tempdir().expect("tempdir");
|
||||||
|
let plugin_root = tmp.path().join("plugins/sample");
|
||||||
|
let claude_path = plugin_root.join(ALTERNATE_PLUGIN_CLA_MANIFEST_RELATIVE_PATH);
|
||||||
|
let cursor_path = plugin_root.join(ALTERNATE_PLUGIN_CUR_MANIFEST_RELATIVE_PATH);
|
||||||
|
fs::create_dir_all(claude_path.parent().expect("Claude manifest parent"))
|
||||||
|
.expect("Claude manifest parent");
|
||||||
|
fs::create_dir_all(cursor_path.parent().expect("Cursor manifest parent"))
|
||||||
|
.expect("Cursor manifest parent");
|
||||||
|
let manifest_target = plugin_root.join("benign.json");
|
||||||
|
fs::write(&manifest_target, r#"{"name":"sample"}"#).expect("benign manifest");
|
||||||
|
fs::write(&cursor_path, r#"{"name":"sample"}"#).expect("Cursor manifest");
|
||||||
|
std::os::unix::fs::symlink(&manifest_target, &claude_path)
|
||||||
|
.expect("Claude manifest symlink");
|
||||||
|
|
||||||
|
assert_eq!(find_plugin_manifest_path(&plugin_root), None);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn preserves_codex_claude_cursor_legacy_precedence() {
|
fn preserves_codex_claude_cursor_legacy_precedence() {
|
||||||
let tmp = tempdir().expect("tempdir");
|
let tmp = tempdir().expect("tempdir");
|
||||||
|
|||||||
Reference in New Issue
Block a user