diff --git a/CHANGELOG.md b/CHANGELOG.md index e6988015..36ecd45a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,11 +8,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -### Added +## [0.8.2] - 2026-03-19 + +### Security -- JFrog Artifactory VCS repository support — explicit FQDN, transparent proxy (`ARTIFACTORY_BASE_URL`), air-gapped mode (`ARTIFACTORY_ONLY=1`), multi-format archive URLs, zip path traversal protection (#354) +- Harden dependency path validation (#364) + +### Added -## [0.8.2] - 2026-03-18 +- JFrog Artifactory VCS repository support (#354) ### Fixed diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index 0ea96d77..42d19826 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -11,6 +11,7 @@ from ..utils.console import _rich_error, _rich_info, _rich_success, _rich_warning from ..utils.diagnostics import DiagnosticCollector from ..utils.github_host import default_host, is_valid_fqdn +from ..utils.path_security import safe_rmtree from ._helpers import ( _create_minimal_apm_yml, _get_default_config, @@ -802,7 +803,10 @@ def _copy_local_package(dep_ref, install_path, project_root): # Ensure parent exists and clean target (always re-copy for local deps) install_path.parent.mkdir(parents=True, exist_ok=True) if install_path.exists(): - shutil.rmtree(install_path) + # install_path is already validated by get_install_path() (Layer 2), + # but use safe_rmtree for defense-in-depth. + apm_modules_dir = install_path.parent.parent # _local/ → apm_modules + safe_rmtree(install_path, apm_modules_dir) shutil.copytree(local, install_path, dirs_exist_ok=False, symlinks=True) return install_path diff --git a/src/apm_cli/commands/prune.py b/src/apm_cli/commands/prune.py index f56544f6..816e45fc 100644 --- a/src/apm_cli/commands/prune.py +++ b/src/apm_cli/commands/prune.py @@ -7,6 +7,7 @@ import click from ..utils.console import _rich_error, _rich_info, _rich_success, _rich_warning +from ..utils.path_security import PathTraversalError, safe_rmtree from ._helpers import _build_expected_install_paths, _scan_installed_packages # APM Dependencies @@ -80,7 +81,7 @@ def prune(ctx, dry_run): path_parts = org_repo_name.split("/") pkg_path = apm_modules_dir.joinpath(*path_parts) try: - shutil.rmtree(pkg_path) + safe_rmtree(pkg_path, apm_modules_dir) _rich_info(f"+ Removed {org_repo_name}") removed_count += 1 pruned_keys.append(org_repo_name) diff --git a/src/apm_cli/commands/uninstall.py b/src/apm_cli/commands/uninstall.py index d321cf64..41b9208f 100644 --- a/src/apm_cli/commands/uninstall.py +++ b/src/apm_cli/commands/uninstall.py @@ -8,6 +8,7 @@ import click from ..utils.console import _rich_error, _rich_info, _rich_success, _rich_warning +from ..utils.path_security import PathTraversalError, safe_rmtree # APM Dependencies try: @@ -210,6 +211,9 @@ def _parse_dependency_entry(dep_entry): try: dep_ref = _parse_dependency_entry(package) package_path = dep_ref.get_install_path(apm_modules_dir) + except (PathTraversalError,) as e: + _rich_error(f"x Refusing to remove {package}: {e}") + continue except (ValueError, TypeError, AttributeError, KeyError): # Fallback for invalid format: use raw path segments package_str = package if isinstance(package, str) else str(package) @@ -221,7 +225,7 @@ def _parse_dependency_entry(dep_entry): if package_path.exists(): try: - shutil.rmtree(package_path) + safe_rmtree(package_path, apm_modules_dir) _rich_info(f"+ Removed {package} from apm_modules/") removed_from_modules += 1 deleted_pkg_paths.append(package_path) @@ -306,7 +310,7 @@ def _find_transitive_orphans(lockfile, removed_urls): if orphan_path.exists(): try: - shutil.rmtree(orphan_path) + safe_rmtree(orphan_path, apm_modules_dir) _rich_info(f"+ Removed transitive dependency {orphan_key} from apm_modules/") removed_from_modules += 1 deleted_orphan_paths.append(orphan_path) diff --git a/src/apm_cli/deps/github_downloader.py b/src/apm_cli/deps/github_downloader.py index b751e872..d9d30ca3 100644 --- a/src/apm_cli/deps/github_downloader.py +++ b/src/apm_cli/deps/github_downloader.py @@ -1566,6 +1566,9 @@ def download_subdirectory_package(self, dep_ref: DependencyReference, target_pat # Check if subdirectory exists source_subdir = temp_clone_path / subdir_path + # Security: ensure subdirectory resolves within the cloned repo + from ..utils.path_security import ensure_path_within + ensure_path_within(source_subdir, temp_clone_path) if not source_subdir.exists(): raise RuntimeError(f"Subdirectory '{subdir_path}' not found in repository") diff --git a/src/apm_cli/models/dependency.py b/src/apm_cli/models/dependency.py index f2e1bb1c..9567a1eb 100644 --- a/src/apm_cli/models/dependency.py +++ b/src/apm_cli/models/dependency.py @@ -7,6 +7,7 @@ from pathlib import Path from typing import Any, Dict, List, Optional +from ..utils.path_security import PathTraversalError, ensure_path_within from ..utils.github_host import ( default_host, is_artifactory_path, @@ -293,12 +294,39 @@ def get_install_path(self, apm_modules_dir: Path) -> Path: Returns: Path: Absolute path to the package installation directory + + Raises: + PathTraversalError: If the computed path escapes apm_modules_dir """ if self.is_local and self.local_path: pkg_dir_name = Path(self.local_path).name - return apm_modules_dir / "_local" / pkg_dir_name + if pkg_dir_name in ('', '.', '..'): + raise PathTraversalError( + f"Invalid local package path '{self.local_path}': " + f"basename must not be empty, '.', or '..'" + ) + result = apm_modules_dir / "_local" / pkg_dir_name + ensure_path_within(result, apm_modules_dir) + return result repo_parts = self.repo_url.split("/") + + # Security: reject traversal in repo_url segments (catches lockfile injection) + if any(seg in ('.', '..') for seg in repo_parts): + raise PathTraversalError( + f"Invalid repo_url '{self.repo_url}': " + f"path segments must not be '.' or '..'" + ) + + # Security: reject traversal in virtual_path (catches lockfile injection) + if self.virtual_path and any( + seg in ('.', '..') for seg in self.virtual_path.replace('\\', '/').split('/') + ): + raise PathTraversalError( + f"Invalid virtual_path '{self.virtual_path}': " + f"path segments must not be '.' or '..'" + ) + result: Path | None = None if self.is_virtual: # Subdirectory packages (like Claude Skills) should use natural path structure @@ -306,30 +334,35 @@ def get_install_path(self, apm_modules_dir: Path) -> Path: # Use repo path + subdirectory path if self.is_azure_devops() and len(repo_parts) >= 3: # ADO: org/project/repo/subdir - return apm_modules_dir / repo_parts[0] / repo_parts[1] / repo_parts[2] / self.virtual_path + result = apm_modules_dir / repo_parts[0] / repo_parts[1] / repo_parts[2] / self.virtual_path elif len(repo_parts) >= 2: # owner/repo/subdir or group/subgroup/repo/subdir - return apm_modules_dir.joinpath(*repo_parts, self.virtual_path) + result = apm_modules_dir.joinpath(*repo_parts, self.virtual_path) else: # Virtual file/collection: use sanitized package name (flattened) package_name = self.get_virtual_package_name() if self.is_azure_devops() and len(repo_parts) >= 3: # ADO: org/project/virtual-pkg-name - return apm_modules_dir / repo_parts[0] / repo_parts[1] / package_name + result = apm_modules_dir / repo_parts[0] / repo_parts[1] / package_name elif len(repo_parts) >= 2: # owner/virtual-pkg-name (use first segment as namespace) - return apm_modules_dir / repo_parts[0] / package_name + result = apm_modules_dir / repo_parts[0] / package_name else: # Regular package: use full repo path if self.is_azure_devops() and len(repo_parts) >= 3: # ADO: org/project/repo - return apm_modules_dir / repo_parts[0] / repo_parts[1] / repo_parts[2] + result = apm_modules_dir / repo_parts[0] / repo_parts[1] / repo_parts[2] elif len(repo_parts) >= 2: # owner/repo or group/subgroup/repo (generic hosts) - return apm_modules_dir.joinpath(*repo_parts) + result = apm_modules_dir.joinpath(*repo_parts) - # Fallback: join all parts - return apm_modules_dir.joinpath(*repo_parts) + if result is None: + # Fallback: join all parts + result = apm_modules_dir.joinpath(*repo_parts) + + # Security: ensure the computed path stays within apm_modules/ + ensure_path_within(result, apm_modules_dir) + return result @staticmethod def _normalize_ssh_protocol_url(url: str) -> str: @@ -428,7 +461,13 @@ def parse_from_dict(cls, entry: dict) -> "DependencyReference": if sub_path is not None: if not isinstance(sub_path, str) or not sub_path.strip(): raise ValueError("'path' field must be a non-empty string") - sub_path = sub_path.strip().strip('/') + # Normalize backslashes to forward slashes for cross-platform safety + sub_path = sub_path.replace('\\', '/').strip().strip('/') + # Security: reject path traversal + if any(seg in ('.', '..') for seg in sub_path.split('/')): + raise PathTraversalError( + f"Invalid path '{sub_path}': path segments must not be '.' or '..'" + ) # Parse the git URL using the standard parser dep = cls.parse(git_url) @@ -633,6 +672,14 @@ def parse(cls, dependency_str: str) -> "DependencyReference": # Extract virtual path (base repo is derived later) virtual_path = '/'.join(path_segments[min_base_segments:]) + + # Security: reject path traversal in virtual path + # Normalize backslashes for cross-platform safety + vp_check = virtual_path.replace('\\', '/') + if any(seg in ('.', '..') for seg in vp_check.split('/')): + raise PathTraversalError( + f"Invalid virtual path '{virtual_path}': path segments must not be '.' or '..'" + ) # Virtual package types (validated later during download): # 1. Collections: /collections/ in path @@ -807,6 +854,10 @@ def parse(cls, dependency_str: str) -> "DependencyReference": # ADO project names may contain spaces allowed_pattern = r'^[a-zA-Z0-9._\- ]+$' if is_ado_host else r'^[a-zA-Z0-9._-]+$' for part in uparts: + if part in ('.', '..'): + raise PathTraversalError( + f"Invalid repository path component: '{part}' is a traversal sequence" + ) if not re.match(allowed_pattern, part.rstrip('.git')): raise ValueError(f"Invalid repository path component: {part}") diff --git a/src/apm_cli/utils/path_security.py b/src/apm_cli/utils/path_security.py new file mode 100644 index 00000000..acbb40ec --- /dev/null +++ b/src/apm_cli/utils/path_security.py @@ -0,0 +1,58 @@ +"""Centralised path-security helpers for APM CLI. + +Every filesystem operation whose target is derived from user-controlled +input (dependency strings, ``virtual_path``, ``apm.yml`` fields) **must** +pass through one of these guards before touching the disk. + +Design +------ +* ``ensure_path_within`` is the single predicate — resolves both paths and + asserts containment via ``Path.is_relative_to``. +* ``safe_rmtree`` wraps ``shutil.rmtree`` with an ``ensure_path_within`` + check so callers get a drop-in replacement. +* ``PathTraversalError`` is a ``ValueError`` subclass for clear error + semantics and easy ``except`` targeting. +""" + +from __future__ import annotations + +import shutil +from pathlib import Path + + +class PathTraversalError(ValueError): + """Raised when a computed path escapes its expected base directory.""" + + +def ensure_path_within(path: Path, base_dir: Path) -> Path: + """Resolve *path* and assert it lives inside *base_dir*. + + Returns the resolved path on success. Raises + :class:`PathTraversalError` if the resolved path escapes *base_dir*. + + This is intentionally strict: symlinks are resolved so that a link + pointing outside the base is caught as well. + """ + resolved = path.resolve() + resolved_base = base_dir.resolve() + try: + if not resolved.is_relative_to(resolved_base): + raise PathTraversalError( + f"Path '{path}' resolves to '{resolved}' which is outside " + f"the allowed base directory '{resolved_base}'" + ) + except (TypeError, ValueError) as exc: + raise PathTraversalError( + f"Cannot verify containment of '{path}' within '{base_dir}': {exc}" + ) from exc + return resolved + + +def safe_rmtree(path: Path, base_dir: Path) -> None: + """Remove *path* only if it resolves within *base_dir*. + + Drop-in replacement for ``shutil.rmtree(path)`` at sites where the + target is derived from user-controlled input. + """ + ensure_path_within(path, base_dir) + shutil.rmtree(path) diff --git a/tests/unit/test_path_security.py b/tests/unit/test_path_security.py new file mode 100644 index 00000000..f93f9503 --- /dev/null +++ b/tests/unit/test_path_security.py @@ -0,0 +1,245 @@ +"""Tests for path security utilities (CVE path traversal fix). + +Covers: +- ensure_path_within() with various traversal payloads +- safe_rmtree() containment enforcement +- Integration with DependencyReference.parse / parse_from_dict / get_install_path +""" + +import shutil +from pathlib import Path + +import pytest + +from apm_cli.utils.path_security import ( + PathTraversalError, + ensure_path_within, + safe_rmtree, +) +from apm_cli.models.dependency import DependencyReference + + +# --------------------------------------------------------------------------- +# ensure_path_within +# --------------------------------------------------------------------------- + + +class TestEnsurePathWithin: + """Unit tests for the ensure_path_within containment check.""" + + def test_path_inside_base_passes(self, tmp_path): + child = tmp_path / "apm_modules" / "owner" / "repo" + child.mkdir(parents=True) + result = ensure_path_within(child, tmp_path / "apm_modules") + assert result == child.resolve() + + def test_dotdot_escape_raises(self, tmp_path): + base = tmp_path / "apm_modules" + base.mkdir() + bad = base / ".." / "etc" + with pytest.raises(PathTraversalError): + ensure_path_within(bad, base) + + def test_deep_dotdot_escape_raises(self, tmp_path): + base = tmp_path / "apm_modules" + base.mkdir() + bad = base / "owner" / "repo" / ".." / ".." / ".." / "secrets" + with pytest.raises(PathTraversalError): + ensure_path_within(bad, base) + + def test_base_itself_passes(self, tmp_path): + base = tmp_path / "apm_modules" + base.mkdir() + result = ensure_path_within(base, base) + assert result == base.resolve() + + def test_absolute_outside_raises(self, tmp_path): + base = tmp_path / "apm_modules" + base.mkdir() + with pytest.raises(PathTraversalError): + ensure_path_within(Path("/tmp/evil"), base) + + def test_symlink_escape_raises(self, tmp_path): + """Symlink inside base pointing outside should be caught.""" + base = tmp_path / "apm_modules" + base.mkdir() + outside = tmp_path / "outside_target" + outside.mkdir() + link = base / "evil_link" + link.symlink_to(outside) + with pytest.raises(PathTraversalError): + ensure_path_within(link, base) + + +# --------------------------------------------------------------------------- +# safe_rmtree +# --------------------------------------------------------------------------- + + +class TestSafeRmtree: + """Unit tests for the safe_rmtree wrapper.""" + + def test_removes_directory_inside_base(self, tmp_path): + base = tmp_path / "apm_modules" + target = base / "owner" / "repo" + target.mkdir(parents=True) + (target / "file.txt").write_text("content") + + safe_rmtree(target, base) + assert not target.exists() + + def test_refuses_to_remove_outside_base(self, tmp_path): + base = tmp_path / "apm_modules" + base.mkdir() + outside = tmp_path / "SAFE_DELETE_ME" + outside.mkdir() + (outside / "proof.txt").write_text("do not delete") + + bad_path = base / ".." / "SAFE_DELETE_ME" + with pytest.raises(PathTraversalError): + safe_rmtree(bad_path, base) + + # Verify the directory was NOT deleted + assert outside.exists() + assert (outside / "proof.txt").read_text() == "do not delete" + + def test_refuses_traversal_in_package_name(self, tmp_path): + """Simulates the MSRC PoC: attacker/repo/../../../SAFE_DELETE_ME.""" + base = tmp_path / "apm_modules" + base.mkdir() + victim = tmp_path / "SAFE_DELETE_ME" + victim.mkdir() + (victim / "proof.txt").write_text("critical data") + + evil_path = base / "attacker" / "repo" / ".." / ".." / ".." / "SAFE_DELETE_ME" + with pytest.raises(PathTraversalError): + safe_rmtree(evil_path, base) + + assert victim.exists() + + +# --------------------------------------------------------------------------- +# DependencyReference parse-time traversal rejection +# --------------------------------------------------------------------------- + + +class TestDependencyParseTraversalRejection: + """Verify that parse() and parse_from_dict() reject traversal sequences.""" + + def test_parse_rejects_dotdot_in_repo(self): + with pytest.raises((ValueError, PathTraversalError)): + DependencyReference.parse("owner/../evil") + + def test_parse_rejects_dotdot_in_virtual_path(self): + with pytest.raises((ValueError, PathTraversalError)): + DependencyReference.parse("owner/repo/../../etc/passwd") + + def test_parse_rejects_single_dot_segment(self): + with pytest.raises((ValueError, PathTraversalError)): + DependencyReference.parse("owner/./repo") + + def test_parse_from_dict_rejects_dotdot_in_path(self): + entry = {"git": "https://github.com/owner/repo", "path": "../../etc"} + with pytest.raises((ValueError, PathTraversalError)): + DependencyReference.parse_from_dict(entry) + + def test_parse_from_dict_rejects_single_dot_in_path(self): + entry = {"git": "https://github.com/owner/repo", "path": "./hidden"} + with pytest.raises((ValueError, PathTraversalError)): + DependencyReference.parse_from_dict(entry) + + def test_parse_from_dict_rejects_nested_dotdot(self): + entry = { + "git": "https://github.com/owner/repo", + "path": "subdir/../../escape", + } + with pytest.raises((ValueError, PathTraversalError)): + DependencyReference.parse_from_dict(entry) + + def test_parse_from_dict_rejects_backslash_traversal(self): + """Windows-style backslash traversal must be caught.""" + entry = {"git": "https://github.com/owner/repo", "path": "..\\..\\etc"} + with pytest.raises((ValueError, PathTraversalError)): + DependencyReference.parse_from_dict(entry) + + def test_parse_from_dict_rejects_mixed_separator_traversal(self): + entry = {"git": "https://github.com/owner/repo", "path": "sub\\..\\..\\esc"} + with pytest.raises((ValueError, PathTraversalError)): + DependencyReference.parse_from_dict(entry) + + def test_parse_from_dict_accepts_valid_subpath(self): + entry = {"git": "https://github.com/owner/repo", "path": "skills/my-skill"} + dep = DependencyReference.parse_from_dict(entry) + assert dep.virtual_path == "skills/my-skill" + assert dep.is_virtual is True + + def test_parse_accepts_normal_virtual_package(self): + dep = DependencyReference.parse("owner/repo/prompts/my-file.prompt.md") + assert dep.is_virtual is True + + +# --------------------------------------------------------------------------- +# DependencyReference.get_install_path containment +# --------------------------------------------------------------------------- + + +class TestGetInstallPathContainment: + """Verify get_install_path() rejects paths that escape apm_modules/.""" + + def test_normal_package_path(self, tmp_path): + base = tmp_path / "apm_modules" + base.mkdir() + dep = DependencyReference.parse("owner/repo") + path = dep.get_install_path(base) + assert path == base / "owner" / "repo" + + def test_traversal_in_virtual_path_raises(self, tmp_path): + """Even if parse-time validation is bypassed, get_install_path catches it.""" + base = tmp_path / "apm_modules" + base.mkdir() + dep = DependencyReference(repo_url="owner/repo") + dep.is_virtual = True + # Need enough ../s to escape: owner/repo/../../../../esc → 2 up from subdir + 2 more + dep.virtual_path = "../../../../etc/passwd" + with pytest.raises(PathTraversalError): + dep.get_install_path(base) + + def test_traversal_in_repo_url_raises(self, tmp_path): + """repo_url with .. should be caught by get_install_path.""" + base = tmp_path / "apm_modules" + base.mkdir() + dep = DependencyReference(repo_url="owner/repo/../../..") + dep.is_virtual = False + with pytest.raises(PathTraversalError): + dep.get_install_path(base) + + def test_ado_normal_path(self, tmp_path): + base = tmp_path / "apm_modules" + base.mkdir() + dep = DependencyReference.parse( + "https://dev.azure.com/myorg/myproject/_git/myrepo" + ) + path = dep.get_install_path(base) + assert "myorg" in str(path) + assert path.resolve().is_relative_to(base.resolve()) + + def test_local_path_dotdot_basename_raises(self, tmp_path): + """Crafted local_path with '..' basename must be caught.""" + base = tmp_path / "apm_modules" + base.mkdir() + dep = DependencyReference(repo_url="unused") + dep.is_local = True + dep.local_path = "/some/path/.." + with pytest.raises(PathTraversalError): + dep.get_install_path(base) + + def test_local_path_dot_basename_raises(self, tmp_path): + """Path('.').name returns '' on some platforms — guard handles it.""" + base = tmp_path / "apm_modules" + base.mkdir() + dep = DependencyReference(repo_url="unused") + dep.is_local = True + dep.local_path = "." + # Path(".").name is "" which is in our reject set + with pytest.raises(PathTraversalError): + dep.get_install_path(base) diff --git a/uv.lock b/uv.lock index a65cb740..8310e7be 100644 --- a/uv.lock +++ b/uv.lock @@ -179,7 +179,7 @@ wheels = [ [[package]] name = "apm-cli" -version = "0.8.1" +version = "0.8.2" source = { editable = "." } dependencies = [ { name = "click" },