From 0cf805734b07586b1ca6cad2ac9f2587e16366b5 Mon Sep 17 00:00:00 2001 From: Abbas Jafari Date: Wed, 20 May 2026 13:44:43 +0200 Subject: [PATCH 1/2] fix(plugin): drop destructive rmtree in _map_plugin_artifacts (#1415) The `rmtree(target); copytree(source, target)` cycle wiped the source when a hybrid APM + Claude-plugin manifest pointed paths INTO `.apm/`, causing `apm install` to silently report `(files unchanged)`. The rmtree was redundant -- the downloader already clears install_path before extraction. Replaced with `dirs_exist_ok=True` plus an `_is_same_path` guard for `copy2` self-copies. Tests: 6 regression cases (pre-positioned + mixed-source layouts). --- src/apm_cli/deps/plugin_parser.py | 65 +++++++---- tests/unit/test_plugin_parser.py | 156 +++++++++++++++++++++++++ tests/unit/test_symlink_containment.py | 2 +- 3 files changed, 199 insertions(+), 24 deletions(-) diff --git a/src/apm_cli/deps/plugin_parser.py b/src/apm_cli/deps/plugin_parser.py index afb92220a..4b4012818 100644 --- a/src/apm_cli/deps/plugin_parser.py +++ b/src/apm_cli/deps/plugin_parser.py @@ -445,6 +445,16 @@ def _resolve_sources(component: str, default_dir: str): return [default] return [] + # Helper: True when *src* and *dst* resolve to the same filesystem path + # (e.g. a manifest entry pointing at a file already inside the target). + # Copying onto self raises ``shutil.SameFileError`` and ``shutil.copytree`` + # over identical directories triggers it per-file, so callers must skip. + def _is_same_path(src: Path, dst: Path) -> bool: + try: + return src.resolve() == dst.resolve() + except OSError: + return False + # Map agents/ # Unlike skills (which are named directories containing SKILL.md), agents # are flat files -- each .md is one agent. So we always merge directory @@ -452,25 +462,24 @@ def _resolve_sources(component: str, default_dir: str): agent_sources = _resolve_sources("agents", "agents") if agent_sources: target_agents = apm_dir / "agents" - if target_agents.exists(): - shutil.rmtree(target_agents) agent_dirs = [s for s in agent_sources if s.is_dir()] agent_files = [s for s in agent_sources if s.is_file()] - if agent_dirs: - shutil.copytree(agent_dirs[0], target_agents, ignore=ignore_non_content) - for extra in agent_dirs[1:]: - shutil.copytree(extra, target_agents, dirs_exist_ok=True, ignore=ignore_non_content) + for d in agent_dirs: + if _is_same_path(d, target_agents): + continue + shutil.copytree(d, target_agents, dirs_exist_ok=True, ignore=ignore_non_content) if agent_files: target_agents.mkdir(parents=True, exist_ok=True) for f in agent_files: - shutil.copy2(f, target_agents / f.name) + dst = target_agents / f.name + if _is_same_path(f, dst): + continue + shutil.copy2(f, dst) # Map skills/ skill_sources = _resolve_sources("skills", "skills") if skill_sources: target_skills = apm_dir / "skills" - if target_skills.exists(): - shutil.rmtree(target_skills) skill_dirs = [s for s in skill_sources if s.is_dir()] skill_files = [s for s in skill_sources if s.is_file()] @@ -478,27 +487,32 @@ def _resolve_sources(component: str, default_dir: str): if is_custom_list and skill_dirs: target_skills.mkdir(parents=True, exist_ok=True) for d in skill_dirs: + nested = target_skills / d.name + if _is_same_path(d, nested): + continue shutil.copytree( d, - target_skills / d.name, + nested, ignore=ignore_non_content, dirs_exist_ok=True, ) elif skill_dirs: - shutil.copytree(skill_dirs[0], target_skills, ignore=ignore_non_content) - for extra in skill_dirs[1:]: - shutil.copytree(extra, target_skills, dirs_exist_ok=True, ignore=ignore_non_content) + for d in skill_dirs: + if _is_same_path(d, target_skills): + continue + shutil.copytree(d, target_skills, dirs_exist_ok=True, ignore=ignore_non_content) if skill_files: target_skills.mkdir(parents=True, exist_ok=True) for f in skill_files: - shutil.copy2(f, target_skills / f.name) + dst = target_skills / f.name + if _is_same_path(f, dst): + continue + shutil.copy2(f, dst) # Map commands/ -> .apm/prompts/ (normalize .md -> .prompt.md) command_sources = _resolve_sources("commands", "commands") if command_sources: target_prompts = apm_dir / "prompts" - if target_prompts.exists(): - shutil.rmtree(target_prompts) target_prompts.mkdir(parents=True, exist_ok=True) def _copy_command_file(source_file: Path, dest_dir: Path, rel_to: Path = None): # noqa: RUF013 @@ -511,6 +525,8 @@ def _copy_command_file(source_file: Path, dest_dir: Path, rel_to: Path = None): if not source_file.name.endswith(".prompt.md") and source_file.suffix == ".md": target_path = target_path.with_name(f"{source_file.stem}.prompt.md") target_path.parent.mkdir(parents=True, exist_ok=True) + if _is_same_path(source_file, target_path): + return shutil.copy2(source_file, target_path) for source in command_sources: @@ -538,23 +554,26 @@ def _copy_command_file(source_file: Path, dest_dir: Path, rel_to: Path = None): else: target_hooks = apm_dir / "hooks" target_hooks.mkdir(parents=True, exist_ok=True) - shutil.copy2(src_file, target_hooks / "hooks.json") + dst = target_hooks / "hooks.json" + if not _is_same_path(src_file, dst): + shutil.copy2(src_file, dst) else: # Directory path(s) -- standard flow hook_sources = _resolve_sources("hooks", "hooks") if hook_sources: target_hooks = apm_dir / "hooks" - if target_hooks.exists(): - shutil.rmtree(target_hooks) - shutil.copytree(hook_sources[0], target_hooks, ignore=ignore_non_content) - for extra in hook_sources[1:]: - shutil.copytree(extra, target_hooks, dirs_exist_ok=True, ignore=ignore_non_content) + for d in hook_sources: + if _is_same_path(d, target_hooks): + continue + shutil.copytree(d, target_hooks, dirs_exist_ok=True, ignore=ignore_non_content) # Pass-through files required for MCP/LSP plugins to function for passthrough in (".mcp.json", ".lsp.json", "settings.json"): source_file = plugin_path / passthrough if source_file.exists() and not source_file.is_symlink(): - shutil.copy2(source_file, apm_dir / passthrough) + dst = apm_dir / passthrough + if not _is_same_path(source_file, dst): + shutil.copy2(source_file, dst) def _generate_apm_yml(manifest: dict[str, Any]) -> str: diff --git a/tests/unit/test_plugin_parser.py b/tests/unit/test_plugin_parser.py index 823eb8f18..4fe68d5f7 100644 --- a/tests/unit/test_plugin_parser.py +++ b/tests/unit/test_plugin_parser.py @@ -967,3 +967,159 @@ def test_default_component_dir_as_symlink_rejected(self, tmp_path): assert not agents_dir.exists() or not list(agents_dir.iterdir()), ( "Symlinked default component dir must not be copied" ) + + +class TestMapPluginArtifactsPrePositioned: + """Regression: when plugin.json points to paths already inside .apm/, + _map_plugin_artifacts must NOT destroy the source before copying. + + This reproduces the bug where APM packages with both apm.yml and + .claude-plugin/plugin.json had their .apm/agents/ and .apm/skills/ + directories deleted during validate_apm_package -> normalize_plugin_directory. + """ + + def test_agents_inside_apm_are_preserved(self, tmp_path): + """Manifest agents pointing into .apm/ must not be rmtree'd.""" + plugin_dir = tmp_path / "pkg" + plugin_dir.mkdir() + + # Pre-position agents inside .apm/ (APM package layout) + apm_dir = plugin_dir / ".apm" + agents_dir = apm_dir / "agents" + agents_dir.mkdir(parents=True) + (agents_dir / "my-agent.agent.md").write_text("# Agent") + + # Manifest points into .apm/ + manifest = {"name": "test", "agents": [".apm/agents/my-agent.agent.md"]} + _map_plugin_artifacts(plugin_dir, apm_dir, manifest=manifest) + + assert (agents_dir / "my-agent.agent.md").exists(), ( + ".apm/agents/ content destroyed by _map_plugin_artifacts" + ) + assert (agents_dir / "my-agent.agent.md").read_text() == "# Agent" + + def test_skills_inside_apm_are_preserved(self, tmp_path): + """Manifest skills pointing into .apm/ must not be rmtree'd.""" + plugin_dir = tmp_path / "pkg" + plugin_dir.mkdir() + + apm_dir = plugin_dir / ".apm" + skill_dir = apm_dir / "skills" / "my-skill" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text("# Skill") + + manifest = {"name": "test", "skills": [".apm/skills/my-skill"]} + _map_plugin_artifacts(plugin_dir, apm_dir, manifest=manifest) + + assert (skill_dir / "SKILL.md").exists(), ( + ".apm/skills/ content destroyed by _map_plugin_artifacts" + ) + assert (skill_dir / "SKILL.md").read_text() == "# Skill" + + def test_commands_inside_apm_are_preserved(self, tmp_path): + """Manifest commands pointing into .apm/ must not be rmtree'd.""" + plugin_dir = tmp_path / "pkg" + plugin_dir.mkdir() + + apm_dir = plugin_dir / ".apm" + prompts_dir = apm_dir / "prompts" + prompts_dir.mkdir(parents=True) + (prompts_dir / "run.prompt.md").write_text("# Run") + + manifest = {"name": "test", "commands": [".apm/prompts"]} + _map_plugin_artifacts(plugin_dir, apm_dir, manifest=manifest) + + assert (prompts_dir / "run.prompt.md").exists(), ( + ".apm/prompts/ content destroyed by _map_plugin_artifacts" + ) + + def test_hooks_inside_apm_are_preserved(self, tmp_path): + """Manifest hooks pointing into .apm/ must not be rmtree'd.""" + plugin_dir = tmp_path / "pkg" + plugin_dir.mkdir() + + apm_dir = plugin_dir / ".apm" + hooks_dir = apm_dir / "hooks" + hooks_dir.mkdir(parents=True) + (hooks_dir / "pre-commit.json").write_text("{}") + + manifest = {"name": "test", "hooks": ".apm/hooks"} + _map_plugin_artifacts(plugin_dir, apm_dir, manifest=manifest) + + assert (hooks_dir / "pre-commit.json").exists(), ( + ".apm/hooks/ content destroyed by _map_plugin_artifacts" + ) + + def test_hooks_config_file_inside_apm_is_preserved(self, tmp_path): + """Manifest `hooks: ".apm/hooks/hooks.json"` (config-file form) + must not raise SameFileError when src and dst are the same path.""" + plugin_dir = tmp_path / "pkg" + plugin_dir.mkdir() + + apm_dir = plugin_dir / ".apm" + hooks_dir = apm_dir / "hooks" + hooks_dir.mkdir(parents=True) + (hooks_dir / "hooks.json").write_text('{"on": "pre-commit"}') + + manifest = {"name": "test", "hooks": ".apm/hooks/hooks.json"} + # Must not raise SameFileError + _map_plugin_artifacts(plugin_dir, apm_dir, manifest=manifest) + + assert (hooks_dir / "hooks.json").exists() + assert (hooks_dir / "hooks.json").read_text() == '{"on": "pre-commit"}' + + def test_external_agents_still_copied(self, tmp_path): + """Non-.apm/ agents must still be copied into .apm/ (no regression).""" + plugin_dir = tmp_path / "pkg" + plugin_dir.mkdir() + + # Agents at root level (standard plugin layout) + agents_dir = plugin_dir / "agents" + agents_dir.mkdir() + (agents_dir / "helper.agent.md").write_text("# Helper") + + apm_dir = plugin_dir / ".apm" + apm_dir.mkdir() + _map_plugin_artifacts(plugin_dir, apm_dir) + + assert (apm_dir / "agents" / "helper.agent.md").exists() + assert (apm_dir / "agents" / "helper.agent.md").read_text() == "# Helper" + + def test_mixed_inside_and_external_agents_both_survive(self, tmp_path): + """Hybrid manifest mixing .apm/ paths and root-level paths: + the pre-positioned .apm/ agent must NOT be destroyed, AND the + external root-level agent must still be copied in. + + Regression for the per-source overlap case raised in PR #1416 review. + """ + plugin_dir = tmp_path / "pkg" + plugin_dir.mkdir() + + # Pre-positioned agent inside .apm/ + apm_dir = plugin_dir / ".apm" + apm_agents = apm_dir / "agents" + apm_agents.mkdir(parents=True) + (apm_agents / "pre.agent.md").write_text("# Pre") + + # External agent at root level + root_agents = plugin_dir / "agents" + root_agents.mkdir() + (root_agents / "new.agent.md").write_text("# New") + + manifest = { + "name": "test", + "agents": [".apm/agents/pre.agent.md", "agents/new.agent.md"], + } + _map_plugin_artifacts(plugin_dir, apm_dir, manifest=manifest) + + # Pre-positioned survives + assert (apm_agents / "pre.agent.md").exists(), ( + "pre-positioned .apm/ agent was destroyed in mixed-source case" + ) + assert (apm_agents / "pre.agent.md").read_text() == "# Pre" + + # External got copied in + assert (apm_agents / "new.agent.md").exists(), ( + "external root-level agent was not copied in the mixed-source case" + ) + assert (apm_agents / "new.agent.md").read_text() == "# New" diff --git a/tests/unit/test_symlink_containment.py b/tests/unit/test_symlink_containment.py index 5da56fb9d..f431a7c9a 100644 --- a/tests/unit/test_symlink_containment.py +++ b/tests/unit/test_symlink_containment.py @@ -287,7 +287,7 @@ class TestIgnoreNonContentSourceGuard(unittest.TestCase): # 2 hooks. After PR #1153 follow-up #2, ALL 7 use ignore_non_content; # the import + 7 call sites yields >= 8 references. _MODULES: typing.ClassVar[list[tuple[str, str, int]]] = [ - ("apm_cli.deps", "plugin_parser", 7), + ("apm_cli.deps", "plugin_parser", 5), ("apm_cli.bundle", "packer", 2), ("apm_cli.bundle", "unpacker", 2), ] From f8f7a9db33c733a3d942de4fe22e7826b0cdf866 Mon Sep 17 00:00:00 2001 From: Daniel Meppiel Date: Fri, 22 May 2026 00:16:44 +0200 Subject: [PATCH 2/2] fix(plugin): refuse symlinked dst entries in _map_plugin_artifacts Defense-in-depth follow-up to apm-review-panel finding on #1416: the supply-chain reviewer flagged that removing the unconditional rmtree leaves pre-existing dst symlinks inside target_agents / target_skills / target_prompts / target_hooks unvalidated, so a malicious package shipping .apm// as a symlink to an external path (e.g. /etc, $HOME/.ssh) can redirect shutil.copytree(..., dirs_exist_ok=True) writes outside the plugin root. Add _assert_no_symlink_descendants(target) which walks target with os.walk(followlinks=False) and refuses to copy when target itself or any descendant is a symlink. The new PluginIntegrityError is raised before each copytree / copy2 site that writes into apm_dir (agents, skills, prompts, hooks, .mcp.json / .lsp.json / settings.json). Raising is preferred over silent-skip because the threat model is data-loss-adjacent: a redirected write is more dangerous than a failed install. Regression-trapped by test_dst_symlink_in_target_does_not_redirect_copy which stages target_agents/linked -> external_sentinel/, calls _map_plugin_artifacts on a package that ships agents/linked/evil.md, and asserts PluginIntegrityError fires AND the sentinel is unmodified. Mutation-break gate confirmed: removing the guard at the agents copy site flips the test from PASS to 'DID NOT RAISE'. Co-authored-by: abi-jey <66185192+abi-jey@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/apm_cli/deps/plugin_parser.py | 43 ++++++++++++++++++++++++++ tests/unit/test_plugin_parser.py | 50 +++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+) diff --git a/src/apm_cli/deps/plugin_parser.py b/src/apm_cli/deps/plugin_parser.py index 4b4012818..3d1c78e2e 100644 --- a/src/apm_cli/deps/plugin_parser.py +++ b/src/apm_cli/deps/plugin_parser.py @@ -13,6 +13,7 @@ import json import logging +import os import shutil from pathlib import Path from typing import Any, Dict, List, Optional # noqa: F401, UP035 @@ -25,6 +26,38 @@ _logger = logging.getLogger(__name__) +class PluginIntegrityError(RuntimeError): + """Raised when a plugin destination tree contains a pre-existing symlink. + + Refusing to copy through a symlinked destination is defense-in-depth + for the data-loss-adjacent ``shutil.copytree(..., dirs_exist_ok=True)`` + flow in ``_map_plugin_artifacts``. A malicious package shipping + ``.apm/skills/`` (or any other target_* subtree) as a symlink to + an external path (e.g. ``/etc``, ``$HOME/.ssh``) would otherwise + redirect writes outside the plugin root. + """ + + +def _assert_no_symlink_descendants(target: Path) -> None: + """Refuse to copy when *target* or any of its descendants is a symlink. + + Uses ``lstat``/``os.walk(followlinks=False)`` so the check itself does + not traverse a hostile symlink. No-op when *target* does not exist. + """ + if not target.exists() and not target.is_symlink(): + return + if target.is_symlink(): + raise PluginIntegrityError(f"Refusing to copy into symlinked plugin destination: {target}") + for root, dirs, files in os.walk(target, followlinks=False): + root_path = Path(root) + for name in dirs + files: + entry = root_path / name + if entry.is_symlink(): + raise PluginIntegrityError( + f"Refusing to copy into plugin destination containing symlinked entry: {entry}" + ) + + def _surface_warning(message: str, logger: logging.Logger) -> None: """Emit a warning to both the stdlib logger and the rich console. @@ -462,6 +495,7 @@ def _is_same_path(src: Path, dst: Path) -> bool: agent_sources = _resolve_sources("agents", "agents") if agent_sources: target_agents = apm_dir / "agents" + _assert_no_symlink_descendants(target_agents) agent_dirs = [s for s in agent_sources if s.is_dir()] agent_files = [s for s in agent_sources if s.is_file()] for d in agent_dirs: @@ -480,6 +514,7 @@ def _is_same_path(src: Path, dst: Path) -> bool: skill_sources = _resolve_sources("skills", "skills") if skill_sources: target_skills = apm_dir / "skills" + _assert_no_symlink_descendants(target_skills) skill_dirs = [s for s in skill_sources if s.is_dir()] skill_files = [s for s in skill_sources if s.is_file()] @@ -513,6 +548,7 @@ def _is_same_path(src: Path, dst: Path) -> bool: command_sources = _resolve_sources("commands", "commands") if command_sources: target_prompts = apm_dir / "prompts" + _assert_no_symlink_descendants(target_prompts) target_prompts.mkdir(parents=True, exist_ok=True) def _copy_command_file(source_file: Path, dest_dir: Path, rel_to: Path = None): # noqa: RUF013 @@ -544,6 +580,7 @@ def _copy_command_file(source_file: Path, dest_dir: Path, rel_to: Path = None): if isinstance(hooks_value, dict): # Inline hooks object -> write as .apm/hooks/hooks.json target_hooks = apm_dir / "hooks" + _assert_no_symlink_descendants(target_hooks) target_hooks.mkdir(parents=True, exist_ok=True) (target_hooks / "hooks.json").write_text(json.dumps(hooks_value, indent=2)) elif isinstance(hooks_value, str) and (plugin_path / hooks_value).is_file(): @@ -553,6 +590,7 @@ def _copy_command_file(source_file: Path, dest_dir: Path, rel_to: Path = None): pass else: target_hooks = apm_dir / "hooks" + _assert_no_symlink_descendants(target_hooks) target_hooks.mkdir(parents=True, exist_ok=True) dst = target_hooks / "hooks.json" if not _is_same_path(src_file, dst): @@ -562,6 +600,7 @@ def _copy_command_file(source_file: Path, dest_dir: Path, rel_to: Path = None): hook_sources = _resolve_sources("hooks", "hooks") if hook_sources: target_hooks = apm_dir / "hooks" + _assert_no_symlink_descendants(target_hooks) for d in hook_sources: if _is_same_path(d, target_hooks): continue @@ -572,6 +611,10 @@ def _copy_command_file(source_file: Path, dest_dir: Path, rel_to: Path = None): source_file = plugin_path / passthrough if source_file.exists() and not source_file.is_symlink(): dst = apm_dir / passthrough + if dst.is_symlink(): + raise PluginIntegrityError( + f"Refusing to copy through symlinked plugin pass-through file: {dst}" + ) if not _is_same_path(source_file, dst): shutil.copy2(source_file, dst) diff --git a/tests/unit/test_plugin_parser.py b/tests/unit/test_plugin_parser.py index 4fe68d5f7..97d236894 100644 --- a/tests/unit/test_plugin_parser.py +++ b/tests/unit/test_plugin_parser.py @@ -8,6 +8,7 @@ import yaml from apm_cli.deps.plugin_parser import ( + PluginIntegrityError, _extract_mcp_servers, _generate_apm_yml, _map_plugin_artifacts, @@ -1123,3 +1124,52 @@ def test_mixed_inside_and_external_agents_both_survive(self, tmp_path): "external root-level agent was not copied in the mixed-source case" ) assert (apm_agents / "new.agent.md").read_text() == "# New" + + def test_dst_symlink_in_target_does_not_redirect_copy(self, tmp_path): + """Defense-in-depth: a malicious package shipping a symlinked + destination entry inside .apm/agents/ (or any target_*) must not + let shutil.copytree(..., dirs_exist_ok=True) follow the link and + write through it to an external sentinel path. + + Regression trap for the dst-symlink-write-anywhere follow-up on + PR #1416. The pre-fix code dropped the unconditional rmtree but + left existing dst symlinks unvalidated; copytree(dirs_exist_ok= + True) would happily walk into a symlinked subdirectory. + """ + # Sentinel external directory that MUST stay untouched. + sentinel = tmp_path / "sentinel_external" + sentinel.mkdir() + (sentinel / "MARKER.md").write_text("# untouched-sentinel") + + plugin_dir = tmp_path / "pkg" + plugin_dir.mkdir() + + # Package ships .apm/agents/ as a symlink pointing at the + # sentinel external directory. This is exactly what the panel + # called out: pre-existing dst symlinks left over from package + # extraction. + apm_dir = plugin_dir / ".apm" + target_agents = apm_dir / "agents" + target_agents.mkdir(parents=True) + malicious_link = target_agents / "linked" + try: + malicious_link.symlink_to(sentinel, target_is_directory=True) + except OSError: + pytest.skip("Symlinks not supported on this platform") + + # Source agents at the standard layout, sharing the linked name + # so copytree would naturally descend into the same subdir. + agent_src = plugin_dir / "agents" + nested_src = agent_src / "linked" + nested_src.mkdir(parents=True) + (nested_src / "evil.md").write_text("# evil-payload") + + with pytest.raises(PluginIntegrityError): + _map_plugin_artifacts(plugin_dir, apm_dir) + + # The sentinel must be untouched: no evil.md should have been + # written through the symlink. + assert (sentinel / "MARKER.md").read_text() == "# untouched-sentinel" + assert not (sentinel / "evil.md").exists(), ( + "copytree(dirs_exist_ok=True) followed a dst symlink and wrote outside the plugin root" + )