diff --git a/CHANGELOG.md b/CHANGELOG.md index ae094ab7e..161287007 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Slash commands installed from APM packages now surface argument hints in Claude Code -- `apm install` automatically maps prompt `input:` to Claude's `arguments:` front-matter, rewrites `${input:name}` references to `$name`, and auto-generates `argument-hint`. Argument names are validated against an allowlist to prevent YAML injection from third-party packages, and the mapping is reported at install time. (#1039) +### Fixed + +- **Transitive `local_path` dependencies now anchor on the declaring `apm.yml`.** Relative paths declared inside a transitive package (e.g. `../sibling`) resolve against that package's directory, not the root consumer, matching what a developer reading the file expects. Hardening: relative `local_path` declarations found inside remotely-fetched packages are now rejected, `_copy_local_package` enforces that the resolved source lies within the project root via `ensure_path_within`, and silent download failures during transitive resolution are now logged for `--verbose` runs. Thanks @JahanzaibTayyab. (#940, closes #857) + ## [0.11.0] - 2026-04-29 ### Added diff --git a/docs/src/content/docs/guides/dependencies.md b/docs/src/content/docs/guides/dependencies.md index 461844bbe..b20c0fef9 100644 --- a/docs/src/content/docs/guides/dependencies.md +++ b/docs/src/content/docs/guides/dependencies.md @@ -96,7 +96,7 @@ dependencies: - gitlab.com/acme/repo/prompts/code-review.prompt.md # Local path (for development / monorepo workflows) - - ./packages/my-shared-skills # relative to project root + - ./packages/my-shared-skills # relative to this apm.yml's directory - /home/user/repos/my-ai-package # absolute path # Object format: git URL + sub-path / ref / alias @@ -288,7 +288,7 @@ Or declare them in `apm.yml`: ```yaml dependencies: apm: - - ./packages/my-shared-skills # relative to project root + - ./packages/my-shared-skills # relative to this apm.yml's directory - /home/user/repos/my-ai-package # absolute path - microsoft/apm-sample-package # remote (can be mixed) ``` @@ -298,6 +298,7 @@ dependencies: - Local packages are validated the same as remote packages (must have `apm.yml` or `SKILL.md`) - `apm compile` works identically regardless of dependency source - Transitive dependencies are resolved recursively (local packages can depend on remote packages) +- **Relative paths anchor on the declaring `apm.yml`** -- a local dep listed inside a nested package (e.g. `../sibling-package` declared in `packages/foo/apm.yml`) resolves against that package's directory, not the root consumer. This matches every other package manager (npm, pip, cargo) and lets sibling packages compose. Use absolute paths if you want anchor-independent behavior. **Re-install behavior:** Local deps are always re-copied on `apm install` since there is no commit SHA to cache against. This ensures you always get the latest local changes. diff --git a/packages/apm-guide/.apm/skills/apm-usage/dependencies.md b/packages/apm-guide/.apm/skills/apm-usage/dependencies.md index 503297591..95c2913d3 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/dependencies.md +++ b/packages/apm-guide/.apm/skills/apm-usage/dependencies.md @@ -35,6 +35,14 @@ dependencies: - ../sibling-repo/my-package ``` +Relative `local_path` entries (`./...`, `../...`) anchor on the directory of +the `apm.yml` that *declares* them, not on the root project. So a transitive +package at `packages/handbook-agents/apm.yml` declaring `../editorial-pipeline` +resolves to `packages/editorial-pipeline/`, matching what a developer reading +the file would expect. Relative `local_path` declarations found inside +remotely-fetched packages are rejected -- only the root project's `apm.yml` +may use them. + ### Custom git ports Non-default git ports are preserved on `https://`, `http://`, and `ssh://` URLs diff --git a/src/apm_cli/deps/apm_resolver.py b/src/apm_cli/deps/apm_resolver.py index 69aa48faa..39be2313e 100644 --- a/src/apm_cli/deps/apm_resolver.py +++ b/src/apm_cli/deps/apm_resolver.py @@ -1,9 +1,13 @@ """APM dependency resolution engine with recursive resolution and conflict detection.""" +import inspect +import logging from pathlib import Path from typing import List, Set, Optional, Protocol, Tuple, runtime_checkable from collections import deque +_logger = logging.getLogger(__name__) + from ..models.apm_package import APMPackage, DependencyReference from .dependency_graph import ( DependencyGraph, DependencyTree, DependencyNode, FlatDependencyMap, @@ -11,10 +15,13 @@ ) # Type alias for the download callback. -# Takes (dep_ref, apm_modules_dir, parent_chain) and returns the install path -# if successful. ``parent_chain`` is a human-readable breadcrumb string like -# "root-pkg > mid-pkg > this-pkg" showing the full dependency path including -# the current node, or just the node's display name for direct (depth-1) deps. +# Takes (dep_ref, apm_modules_dir, parent_chain, parent_pkg) and returns the +# install path if successful. ``parent_chain`` is a human-readable breadcrumb +# string like "root-pkg > mid-pkg > this-pkg" showing the full dependency path +# including the current node, or just the node's display name for direct +# (depth-1) deps. ``parent_pkg`` is the APMPackage that declared this +# dependency (None for direct deps from the root); callers use its +# ``source_path`` to anchor relative ``local_path`` resolution (#857). @runtime_checkable class DownloadCallback(Protocol): def __call__( @@ -22,6 +29,7 @@ def __call__( dep_ref: 'DependencyReference', apm_modules_dir: Path, parent_chain: str = "", + parent_pkg: Optional['APMPackage'] = None, ) -> Optional[Path]: ... @@ -47,7 +55,33 @@ def __init__( self._apm_modules_dir: Optional[Path] = apm_modules_dir self._project_root: Optional[Path] = None self._download_callback = download_callback + # Whether ``download_callback`` accepts ``parent_pkg`` (added in #857). + # Detected once via signature inspection so legacy callbacks that + # predate the field still work without raising a silent TypeError + # that would mask the dependency. + self._callback_accepts_parent_pkg: bool = ( + self._signature_accepts_parent_pkg(download_callback) + if download_callback is not None + else False + ) self._downloaded_packages: Set[str] = set() # Track what we downloaded during this resolution + + @staticmethod + def _signature_accepts_parent_pkg(callback) -> bool: + """Return True if ``callback`` declares a ``parent_pkg`` parameter + (or accepts ``**kwargs``). Falls back to True if the signature can't + be introspected (e.g. C extensions) so the modern path is preferred. + """ + try: + sig = inspect.signature(callback) + except (TypeError, ValueError): + return True + for param in sig.parameters.values(): + if param.kind is inspect.Parameter.VAR_KEYWORD: + return True + if param.name == "parent_pkg": + return True + return False def resolve_dependencies(self, project_root: Path) -> DependencyGraph: """ @@ -79,6 +113,8 @@ def resolve_dependencies(self, project_root: Path) -> DependencyGraph: try: root_package = APMPackage.from_apm_yml(apm_yml_path) + # Anchor for relative local_path dependencies declared at the root. + root_package.source_path = project_root.resolve() except (ValueError, FileNotFoundError) as e: # Create error graph empty_package = APMPackage(name="error", version="0.0.0", package_path=project_root) @@ -213,7 +249,9 @@ def build_dependency_tree(self, root_apm_yml: Path) -> DependencyTree: parent_chain = node.get_ancestor_chain() loaded_package = self._try_load_dependency_package( - dep_ref, parent_chain=parent_chain + dep_ref, + parent_chain=parent_chain, + parent_pkg=parent_node.package if parent_node else None, ) if loaded_package: # Update the node with the actual loaded package @@ -358,56 +396,122 @@ def _validate_dependency_reference(self, dep_ref: DependencyReference) -> bool: return True def _try_load_dependency_package( - self, dep_ref: DependencyReference, parent_chain: str = "" + self, + dep_ref: DependencyReference, + parent_chain: str = "", + parent_pkg: Optional[APMPackage] = None, ) -> Optional[APMPackage]: """ Try to load a dependency package from apm_modules/. - + This method scans apm_modules/ to find installed packages and loads their apm.yml to enable transitive dependency resolution. If a package is not installed and a download_callback is available, it will attempt to fetch the package first. - + Args: dep_ref: Reference to the dependency to load parent_chain: Human-readable breadcrumb of the dependency path that led here (e.g. "root-pkg > mid-pkg"). Forwarded to the download callback for contextual error messages. - + parent_pkg: APMPackage that declared *dep_ref* (None for direct + deps from the root project). Its ``source_path`` is forwarded + to the download callback so relative ``local_path`` deps + resolve against the directory containing the declaring + apm.yml rather than the root consumer (#857). + Returns: APMPackage: Loaded package if found, None otherwise - + Raises: ValueError: If package exists but has invalid format FileNotFoundError: If package cannot be found """ if self._apm_modules_dir is None: return None - + + # Reject relative ``local_path`` deps declared inside a package that + # was itself fetched remotely. The originating apm.yml is not + # user-controlled, so a string like ``../../etc/passwd`` is the most + # plausible path-traversal vector. Direct deps from the root project + # (parent_pkg is None) and absolute paths are unaffected. + if ( + dep_ref.is_local + and dep_ref.local_path + and not Path(dep_ref.local_path).expanduser().is_absolute() + and self._is_remote_parent(parent_pkg) + ): + _logger.warning( + "Rejecting relative local_path %r declared inside remotely-fetched " + "package %r (parent chain: %s); local_path is only allowed in the " + "root project's apm.yml.", + dep_ref.local_path, + getattr(parent_pkg, "name", ""), + parent_chain or "", + ) + return None + # Get the canonical install path for this dependency install_path = dep_ref.get_install_path(self._apm_modules_dir) - + # If package doesn't exist locally, try to download it if not install_path.exists(): if self._download_callback is not None: - unique_key = dep_ref.get_unique_key() + # For local deps, dedupe by the *resolved* on-disk path so that + # the same literal (e.g. ``../common``) declared by two + # different parents does not collapse onto a single graph + # node when each parent's source dir actually points to a + # different sibling (#857). + unique_key = self._download_dedup_key(dep_ref, parent_pkg) # Avoid re-downloading the same package in a single resolution if unique_key not in self._downloaded_packages: try: - downloaded_path = self._download_callback( - dep_ref, self._apm_modules_dir, parent_chain - ) + if self._callback_accepts_parent_pkg: + downloaded_path = self._download_callback( + dep_ref, + self._apm_modules_dir, + parent_chain, + parent_pkg, + ) + else: + # Legacy callback predating #857 -- no parent_pkg. + # Local deps from non-root parents will fall back + # to project_root resolution; this matches behavior + # before the fix, so no surprise regression. + downloaded_path = self._download_callback( + dep_ref, + self._apm_modules_dir, + parent_chain, + ) if downloaded_path and downloaded_path.exists(): self._downloaded_packages.add(unique_key) install_path = downloaded_path - except Exception: - # Download failed - continue without this dependency's sub-deps - pass - + except Exception as exc: + # Download failed - continue without this dependency's + # sub-deps. Surface the underlying error via the stdlib + # logger so ``--verbose`` (which configures handlers) + # makes the silent skip diagnosable; we deliberately + # do not raise so partial resolution still works. + _logger.warning( + "Download of dependency %r (parent chain: %s) failed; " + "skipping its sub-dependencies: %s", + dep_ref.get_display_name(), + parent_chain or "", + exc, + exc_info=True, + ) + # Still doesn't exist after download attempt if not install_path.exists(): return None - + + # Anchor for resolving this package's own relative local_path deps: + # the *original* source dir for local deps (not the apm_modules copy), + # otherwise the install_path itself. + resolved_source_path = self._compute_dep_source_path( + dep_ref, install_path, parent_pkg + ) + # Look for apm.yml in the install path apm_yml_path = install_path / "apm.yml" if not apm_yml_path.exists(): @@ -420,22 +524,119 @@ def _try_load_dependency_package( name=dep_ref.get_display_name(), version="1.0.0", source=dep_ref.repo_url, - package_path=install_path + package_path=install_path, + source_path=resolved_source_path, ) # No manifest found return None - + # Load and return the package try: package = APMPackage.from_apm_yml(apm_yml_path) # Ensure source is set for tracking if not package.source: package.source = dep_ref.repo_url + # Set source_path so transitive resolution of THIS package's + # local-path deps anchors on the right directory. + package.source_path = resolved_source_path return package except (ValueError, FileNotFoundError) as e: # Package has invalid apm.yml - log warning but continue # In production, we might want to surface this to the user return None + + def _effective_base_dir( + self, parent_pkg: Optional[APMPackage] + ) -> Optional[Path]: + """Return the directory used to anchor relative ``local_path`` deps. + + Per #857, a relative ``local_path`` declared inside a transitive + package's apm.yml resolves against *that* package's directory, not + the root consumer. For direct deps from the root project (or for + legacy parents loaded without a ``source_path``), we fall back to + ``self._project_root``. + + Centralizes the parent-vs-root selection used by + ``_download_dedup_key`` and ``_compute_dep_source_path`` so the + anchoring rule cannot drift between the two call sites. + """ + if parent_pkg is not None and parent_pkg.source_path is not None: + return parent_pkg.source_path + return self._project_root + + def _is_remote_parent(self, parent_pkg: Optional[APMPackage]) -> bool: + """Return True iff *parent_pkg* was fetched from a remote source. + + We treat a parent package as "remote" when its ``source_path`` lives + inside ``self._apm_modules_dir`` -- that is the directory the + downloader writes into. Direct deps from the root project + (``parent_pkg is None``) and packages whose ``source_path`` is + outside apm_modules (e.g. user-edited local-path deps) return + False. Used to gate path-traversal-prone behavior in + ``_try_load_dependency_package``. + """ + if parent_pkg is None or parent_pkg.source_path is None: + return False + if self._apm_modules_dir is None: + return False + try: + parent_pkg.source_path.resolve().relative_to( + self._apm_modules_dir.resolve() + ) + except (ValueError, OSError): + return False + return True + + def _download_dedup_key( + self, + dep_ref: DependencyReference, + parent_pkg: Optional[APMPackage], + ) -> str: + """Return a context-aware key for the per-resolution download cache. + + For remote deps this is just ``dep_ref.get_unique_key()``. For local + deps the literal ``local_path`` (e.g. ``../common``) is ambiguous + once #857 anchors it on the declaring package's directory: the same + string can refer to different on-disk packages depending on which + parent declared it. We therefore prefix the key with the resolved + absolute source path so the dedup matches the actual filesystem + identity, not the textual identity. + """ + if dep_ref.is_local and dep_ref.local_path: + local = Path(dep_ref.local_path).expanduser() + if local.is_absolute(): + resolved = local.resolve() + else: + base_dir = self._effective_base_dir(parent_pkg) + resolved = (base_dir / local).resolve() if base_dir else local.resolve() + return f"local::{resolved}" + return dep_ref.get_unique_key() + + def _compute_dep_source_path( + self, + dep_ref: DependencyReference, + install_path: Path, + parent_pkg: Optional[APMPackage], + ) -> Path: + """Return the on-disk anchor for resolving *dep_ref*'s own local deps. + + For local dependencies this is the *original* directory the user + referenced (so relative paths inside the package's apm.yml mean what + a developer reading the file would expect). For remote deps it is + ``install_path`` (the clone location), which is also where the + package's apm.yml lives. + """ + if dep_ref.is_local and dep_ref.local_path: + local = Path(dep_ref.local_path).expanduser() + if local.is_absolute(): + return local.resolve() + base_dir = self._effective_base_dir(parent_pkg) + if base_dir is None: + # Fallback: best-effort relative-to-cwd. Rare; only hit if + # the resolver was driven without a project root. + return local.resolve() + return (base_dir / local).resolve() + return install_path.resolve() def _create_resolution_summary(self, graph: DependencyGraph) -> str: """ diff --git a/src/apm_cli/install/phases/local_content.py b/src/apm_cli/install/phases/local_content.py index 1cf73cdf9..e7805f0d3 100644 --- a/src/apm_cli/install/phases/local_content.py +++ b/src/apm_cli/install/phases/local_content.py @@ -31,7 +31,11 @@ from pathlib import Path from apm_cli.utils.console import _rich_error -from apm_cli.utils.path_security import safe_rmtree +from apm_cli.utils.path_security import ( + PathTraversalError, + ensure_path_within, + safe_rmtree, +) # --------------------------------------------------------------------------- @@ -75,14 +79,28 @@ def _has_local_apm_content(project_root): # --------------------------------------------------------------------------- -def _copy_local_package(dep_ref, install_path, project_root, logger=None): +def _copy_local_package( + dep_ref, install_path, base_dir, logger=None, project_root=None +): """Copy a local package to apm_modules/. Args: dep_ref: DependencyReference with is_local=True install_path: Target path under apm_modules/ - project_root: Project root for resolving relative paths + base_dir: Directory used to resolve a relative ``dep_ref.local_path``. + For direct deps from the root project this is the project root; + for transitive deps it is the source directory of the package + whose apm.yml declared *dep_ref* (#857). logger: Optional CommandLogger for structured output + project_root: Optional path to the root project. When supplied, the + resolved local source is asserted to lie within ``project_root`` + via :func:`ensure_path_within`, so a transitive declaration like + ``../../../etc/passwd`` is rejected even though ``base_dir`` is + now a transitive parent's source directory rather than the root + (#940). Note: we deliberately do *not* call + ``validate_path_segments`` on ``dep_ref.local_path`` -- that + would reject the legitimate one-level-up form ``../sibling`` + which is the very pattern this PR enables. Returns: install_path on success, None on failure @@ -91,10 +109,24 @@ def _copy_local_package(dep_ref, install_path, project_root, logger=None): local = Path(dep_ref.local_path).expanduser() if not local.is_absolute(): - local = (project_root / local).resolve() + local = (base_dir / local).resolve() else: local = local.resolve() + if project_root is not None: + try: + ensure_path_within(local, project_root) + except PathTraversalError as exc: + msg = ( + f"Refusing to copy local package: {dep_ref.local_path!r} " + f"resolves outside the project root ({exc})." + ) + if logger: + logger.error(msg) + else: + _rich_error(msg) + return None + if not local.is_dir(): msg = f"Local package path does not exist: {dep_ref.local_path}" if logger: diff --git a/src/apm_cli/install/phases/resolve.py b/src/apm_cli/install/phases/resolve.py index ff68715ec..5e9ce169e 100644 --- a/src/apm_cli/install/phases/resolve.py +++ b/src/apm_cli/install/phases/resolve.py @@ -121,7 +121,7 @@ def run(ctx: "InstallContext") -> None: logger = ctx.logger verbose = ctx.verbose - def download_callback(dep_ref, modules_dir, parent_chain=""): + def download_callback(dep_ref, modules_dir, parent_chain="", parent_pkg=None): """Download a package during dependency resolution. Args: @@ -129,6 +129,13 @@ def download_callback(dep_ref, modules_dir, parent_chain=""): modules_dir: Target apm_modules directory. parent_chain: Human-readable breadcrumb (e.g. "root > mid") showing which dependency path led to this transitive dep. + parent_pkg: The APMPackage that declared *dep_ref* (None for + direct deps from the root project). For local deps, the + parent's ``source_path`` becomes the base directory for + resolving the relative path -- so a transitive dep like + ``../sibling`` declared inside a package nested several + directories deep resolves against the package's own + location, not the root consumer (#857). """ install_path = dep_ref.get_install_path(modules_dir) if install_path.exists(): @@ -142,8 +149,18 @@ def download_callback(dep_ref, modules_dir, parent_chain=""): # so use .add() rather than dict-style assignment. callback_failures.add(dep_ref.get_unique_key()) return None + # Anchor relative paths on the *declaring* package's source + # directory when available (#857). Falls back to + # ``project_root`` for direct deps and for parents that + # predate the source_path field. + base_dir = ( + parent_pkg.source_path + if parent_pkg is not None and parent_pkg.source_path is not None + else project_root + ) result_path = _copy_local_package( - dep_ref, install_path, project_root, logger=logger + dep_ref, install_path, base_dir, logger=logger, + project_root=project_root, ) if result_path: callback_downloaded[dep_ref.get_unique_key()] = None diff --git a/src/apm_cli/install/sources.py b/src/apm_cli/install/sources.py index 066b4747a..4450a7075 100644 --- a/src/apm_cli/install/sources.py +++ b/src/apm_cli/install/sources.py @@ -154,7 +154,8 @@ def acquire(self) -> Optional[Materialization]: return None result_path = _copy_local_package( - dep_ref, install_path, ctx.project_root, logger=logger + dep_ref, install_path, ctx.project_root, logger=logger, + project_root=ctx.project_root, ) if not result_path: diagnostics.error( diff --git a/src/apm_cli/models/apm_package.py b/src/apm_cli/models/apm_package.py index 34f91b071..c88308dee 100644 --- a/src/apm_cli/models/apm_package.py +++ b/src/apm_cli/models/apm_package.py @@ -74,6 +74,12 @@ class APMPackage: dev_dependencies: Optional[Dict[str, List[Union[DependencyReference, str, dict]]]] = None scripts: Optional[Dict[str, str]] = None package_path: Optional[Path] = None # Local path to package + # Absolute on-disk directory holding this package's apm.yml. Used to + # resolve relative ``local_path`` dependencies declared in this package's + # apm.yml (#857). For local deps this is the *original* source directory, + # not the apm_modules/_local/ copy. For remote deps and the root project + # this matches package_path. + source_path: Optional[Path] = None target: Optional[Union[str, List[str]]] = None # Target agent(s): single string or list (applies to compile and install) type: Optional[PackageContentType] = None # Package content type: instructions, skill, hybrid, or prompts includes: Optional[Union[str, List[str]]] = None # Include-only manifest: 'auto' or list of repo paths diff --git a/tests/test_apm_resolver.py b/tests/test_apm_resolver.py index 214deaee4..7a78a82b0 100644 --- a/tests/test_apm_resolver.py +++ b/tests/test_apm_resolver.py @@ -493,5 +493,403 @@ def test_dependency_graph_error_handling(self): assert not summary["is_valid"] +class TestSourcePathField(unittest.TestCase): + """APMPackage.source_path field exists and is independent of package_path.""" + + def test_source_path_default_is_none(self): + pkg = APMPackage(name="x", version="1.0.0") + assert pkg.source_path is None + + def test_source_path_can_be_set(self): + pkg = APMPackage(name="x", version="1.0.0", source_path=Path("/some/dir")) + assert pkg.source_path == Path("/some/dir") + + +class TestComputeDepSourcePath(unittest.TestCase): + """``_compute_dep_source_path`` anchors local-relative deps correctly (#857).""" + + def setUp(self): + self.resolver = APMDependencyResolver() + self.resolver._project_root = Path("/proj") + + def _local_ref(self, local_path: str): + return DependencyReference.parse(local_path) + + def test_remote_dep_returns_install_path(self): + ref = DependencyReference(repo_url="user/repo", is_local=False) + install = Path("/proj/apm_modules/user/repo") + result = self.resolver._compute_dep_source_path(ref, install, parent_pkg=None) + assert result == install.resolve() + + def test_local_absolute_path_returns_resolved(self): + ref = self._local_ref("/abs/path/pkg") + install = Path("/proj/apm_modules/_local/pkg") + result = self.resolver._compute_dep_source_path(ref, install, parent_pkg=None) + assert result == Path("/abs/path/pkg").resolve() + + def test_local_relative_path_uses_parent_source_path(self): + # The bug: ``../editorial-pipeline`` declared inside a transitive + # package must resolve against that package's directory, not the + # root consumer. + ref = self._local_ref("../editorial-pipeline") + install = Path("/proj/apm_modules/_local/editorial-pipeline") + parent = APMPackage( + name="handbook-agents", + version="1.0.0", + source_path=Path("/proj/packages/handbook-agents"), + ) + result = self.resolver._compute_dep_source_path(ref, install, parent_pkg=parent) + assert result == Path("/proj/packages/editorial-pipeline").resolve() + + def test_local_relative_path_falls_back_to_project_root_when_no_parent(self): + # Direct deps from root: parent_pkg is None, anchor is project_root. + ref = self._local_ref("./packages/foo") + install = Path("/proj/apm_modules/_local/foo") + result = self.resolver._compute_dep_source_path(ref, install, parent_pkg=None) + assert result == Path("/proj/packages/foo").resolve() + + def test_local_relative_path_falls_back_to_project_root_when_parent_has_no_source_path(self): + # Backwards compat: a parent package created before source_path + # was added (still None) shouldn't break resolution -- fall back + # to the project root rather than crashing. + ref = self._local_ref("./packages/foo") + install = Path("/proj/apm_modules/_local/foo") + legacy_parent = APMPackage(name="legacy", version="1.0.0") + result = self.resolver._compute_dep_source_path( + ref, install, parent_pkg=legacy_parent + ) + assert result == Path("/proj/packages/foo").resolve() + + +class TestResolverSetsRootSourcePath(unittest.TestCase): + """``resolve_dependencies`` populates ``source_path`` on the root package.""" + + def test_root_package_has_source_path_after_resolve(self): + with TemporaryDirectory() as temp_dir: + project_root = Path(temp_dir) + (project_root / "apm.yml").write_text( + "name: root-pkg\nversion: 1.0.0\n" + ) + graph = APMDependencyResolver().resolve_dependencies(project_root) + assert graph.root_package.source_path == project_root.resolve() + + +class TestDownloadCallbackReceivesParent(unittest.TestCase): + """The download callback is invoked with ``parent_pkg`` for transitive deps.""" + + def test_callback_called_with_parent_package(self): + with TemporaryDirectory() as temp_dir: + project_root = Path(temp_dir) + apm_modules = project_root / "apm_modules" + apm_modules.mkdir() + (project_root / "apm.yml").write_text( + "name: root-pkg\nversion: 1.0.0\n" + "dependencies:\n apm:\n - user/dep1\n" + ) + + received_calls = [] + + def fake_download(dep_ref, modules_dir, parent_chain="", parent_pkg=None): + received_calls.append((dep_ref.get_unique_key(), parent_pkg)) + return None # Simulate download miss; we only care about args + + resolver = APMDependencyResolver( + apm_modules_dir=apm_modules, + download_callback=fake_download, + ) + resolver.resolve_dependencies(project_root) + + # The first (and only) download attempt is for the direct dep. + # parent_pkg is None for direct deps -- the assert below pins + # the contract that the callback receives the parameter (and + # would receive a real parent for any transitive deps). + assert received_calls, "download_callback was never invoked" + _, parent_pkg = received_calls[0] + assert parent_pkg is None # direct dep from root + + def test_callback_receives_parent_for_transitive_dep(self): + """Transitive deps invoke the callback with the *declaring* package + as ``parent_pkg`` so its ``source_path`` can anchor relative local + paths (#857).""" + with TemporaryDirectory() as temp_dir: + project_root = Path(temp_dir) + apm_modules = project_root / "apm_modules" + apm_modules.mkdir() + + # Root depends on a local sibling that itself declares a + # transitive remote dep. We satisfy the local dep by hand so + # its apm.yml is loaded and its sub-deps get resolved (which + # is when the callback should fire with parent_pkg=mid-pkg). + mid_install = apm_modules / "_local" / "mid" + mid_install.mkdir(parents=True) + (mid_install / "apm.yml").write_text( + "name: mid-pkg\nversion: 1.0.0\n" + "dependencies:\n apm:\n - user/leaf-dep\n" + ) + + mid_src = project_root / "packages" / "mid" + mid_src.mkdir(parents=True) + (mid_src / "apm.yml").write_text( + "name: mid-pkg\nversion: 1.0.0\n" + "dependencies:\n apm:\n - user/leaf-dep\n" + ) + + (project_root / "apm.yml").write_text( + "name: root-pkg\nversion: 1.0.0\n" + "dependencies:\n apm:\n - ./packages/mid\n" + ) + + received_calls = [] + + def fake_download(dep_ref, apm_modules_dir, parent_chain="", parent_pkg=None): + received_calls.append( + (dep_ref.get_unique_key(), parent_pkg, parent_chain) + ) + return None + + resolver = APMDependencyResolver( + apm_modules_dir=apm_modules, + download_callback=fake_download, + ) + resolver.resolve_dependencies(project_root) + + # The transitive ``user/leaf-dep`` invocation must carry the + # mid-pkg APMPackage as parent_pkg. + leaf_calls = [c for c in received_calls if "leaf-dep" in c[0]] + assert leaf_calls, ( + f"expected a callback call for the transitive leaf-dep, " + f"got: {received_calls}" + ) + _, parent_pkg, _ = leaf_calls[0] + assert parent_pkg is not None, "transitive dep should have parent_pkg" + assert parent_pkg.name == "mid-pkg" + # source_path must be set so future relative resolution would work. + assert parent_pkg.source_path is not None + + +class TestLegacyDownloadCallbackCompatibility(unittest.TestCase): + """Callbacks that predate #857 (no ``parent_pkg`` parameter) still work.""" + + def test_legacy_callback_signature_is_called(self): + with TemporaryDirectory() as temp_dir: + project_root = Path(temp_dir) + apm_modules = project_root / "apm_modules" + apm_modules.mkdir() + (project_root / "apm.yml").write_text( + "name: root-pkg\nversion: 1.0.0\n" + "dependencies:\n apm:\n - user/dep1\n" + ) + + invocations = [] + + # Legacy 3-arg callback -- no ``parent_pkg``. Pre-#857 this is + # the only signature that exists; if the resolver tried to call + # with a 4th positional arg it would raise TypeError and the + # download would be silently skipped. + def legacy_download(dep_ref, apm_modules_dir, parent_chain=""): + invocations.append(dep_ref.get_unique_key()) + return None + + resolver = APMDependencyResolver( + apm_modules_dir=apm_modules, + download_callback=legacy_download, # type: ignore[arg-type] + ) + resolver.resolve_dependencies(project_root) + + assert invocations, ( + "legacy callback should still be invoked; resolver must " + "detect missing parent_pkg parameter via signature inspection" + ) + + def test_modern_callback_detected_as_parent_pkg_aware(self): + def modern(dep_ref, apm_modules_dir, parent_chain="", parent_pkg=None): + return None + assert APMDependencyResolver._signature_accepts_parent_pkg(modern) is True + + def test_legacy_callback_detected_as_not_parent_pkg_aware(self): + def legacy(dep_ref, apm_modules_dir, parent_chain=""): + return None + assert APMDependencyResolver._signature_accepts_parent_pkg(legacy) is False + + def test_kwargs_callback_detected_as_parent_pkg_aware(self): + def varargs(dep_ref, apm_modules_dir, parent_chain="", **kwargs): + return None + assert APMDependencyResolver._signature_accepts_parent_pkg(varargs) is True + + +class TestDownloadDedupKey(unittest.TestCase): + """Per-resolution download dedup key disambiguates identical local_path + literals declared by different parents (#857).""" + + def test_remote_dep_uses_get_unique_key(self): + resolver = APMDependencyResolver() + dep = DependencyReference(repo_url="user/repo") + assert resolver._download_dedup_key(dep, parent_pkg=None) == dep.get_unique_key() + + def test_local_dep_keys_include_resolved_path(self): + resolver = APMDependencyResolver() + dep = DependencyReference( + repo_url="../common", is_local=True, local_path="../common" + ) + parent_a = APMPackage( + name="a", version="1.0.0", source="local", + source_path=Path("/proj/packages/team-x/handbook").resolve(), + ) + parent_b = APMPackage( + name="b", version="1.0.0", source="local", + source_path=Path("/proj/packages/team-y/handbook").resolve(), + ) + key_a = resolver._download_dedup_key(dep, parent_pkg=parent_a) + key_b = resolver._download_dedup_key(dep, parent_pkg=parent_b) + assert key_a != key_b, ( + "same local_path literal under different parents must dedup separately" + ) + assert "team-x" in key_a + assert "team-y" in key_b + + +class TestEffectiveBaseDir(unittest.TestCase): + """``_effective_base_dir`` centralizes the parent-or-project-root choice + used by ``_download_dedup_key`` and ``_compute_dep_source_path`` (#940).""" + + def test_uses_parent_source_path_when_set(self): + resolver = APMDependencyResolver() + resolver._project_root = Path("/proj") + parent = APMPackage( + name="p", version="1.0.0", + source_path=Path("/proj/packages/handbook"), + ) + assert resolver._effective_base_dir(parent) == Path("/proj/packages/handbook") + + def test_falls_back_to_project_root_when_parent_is_none(self): + resolver = APMDependencyResolver() + resolver._project_root = Path("/proj") + assert resolver._effective_base_dir(None) == Path("/proj") + + def test_falls_back_to_project_root_when_parent_has_no_source_path(self): + resolver = APMDependencyResolver() + resolver._project_root = Path("/proj") + legacy_parent = APMPackage(name="legacy", version="1.0.0") + assert resolver._effective_base_dir(legacy_parent) == Path("/proj") + + def test_returns_none_when_neither_parent_nor_project_root(self): + resolver = APMDependencyResolver() + # _project_root left as None + assert resolver._effective_base_dir(None) is None + + +class TestRejectsLocalPathInRemoteParent(unittest.TestCase): + """Relative ``local_path`` declared inside a remotely-fetched package is + rejected as a path-traversal vector (#940).""" + + def test_remote_parent_with_relative_local_dep_returns_none(self): + with TemporaryDirectory() as temp_dir: + project_root = Path(temp_dir) + apm_modules = project_root / "apm_modules" + apm_modules.mkdir() + remote_pkg_dir = apm_modules / "evil-pkg" + remote_pkg_dir.mkdir() + + resolver = APMDependencyResolver() + resolver._project_root = project_root + resolver._apm_modules_dir = apm_modules + + remote_parent = APMPackage( + name="evil-pkg", version="1.0.0", + source_path=remote_pkg_dir, + ) + dep = DependencyReference( + repo_url="../../etc/passwd", + is_local=True, + local_path="../../etc/passwd", + ) + result = resolver._try_load_dependency_package( + dep, parent_chain="root > evil-pkg", parent_pkg=remote_parent + ) + assert result is None + + def test_root_project_relative_local_dep_is_allowed(self): + # Sanity check: rejection only fires for *remote* parents. A direct + # local dep declared by the root project (parent_pkg is None) must + # still be processed normally -- here it just won't find anything + # on disk and returns None for that reason, not for rejection. + with TemporaryDirectory() as temp_dir: + project_root = Path(temp_dir) + apm_modules = project_root / "apm_modules" + apm_modules.mkdir() + resolver = APMDependencyResolver() + resolver._project_root = project_root + resolver._apm_modules_dir = apm_modules + dep = DependencyReference( + repo_url="./packages/foo", is_local=True, + local_path="./packages/foo", + ) + # No download_callback set, so it returns None without raising. + result = resolver._try_load_dependency_package( + dep, parent_chain="", parent_pkg=None + ) + assert result is None # not found, but reached the install_path branch + + def test_remote_parent_with_absolute_local_dep_not_rejected_by_this_guard(self): + # The reject guard only targets *relative* local paths; absolute + # paths bypass the relative-anchor ambiguity (the path-traversal + # vector this guard addresses) and are handled by the existing + # install-path lookup (will return None if not on disk). + with TemporaryDirectory() as temp_dir: + project_root = Path(temp_dir) + apm_modules = project_root / "apm_modules" + apm_modules.mkdir() + remote_pkg_dir = apm_modules / "remote-pkg" + remote_pkg_dir.mkdir() + resolver = APMDependencyResolver() + resolver._project_root = project_root + resolver._apm_modules_dir = apm_modules + remote_parent = APMPackage( + name="remote-pkg", version="1.0.0", source_path=remote_pkg_dir, + ) + dep = DependencyReference( + repo_url="/abs/missing", is_local=True, local_path="/abs/missing", + ) + # Should reach the install_path lookup and return None for + # "not found" rather than for rejection. + result = resolver._try_load_dependency_package( + dep, parent_chain="root > remote-pkg", parent_pkg=remote_parent, + ) + assert result is None + + +class TestSilentDownloadFailureLogsWarning(unittest.TestCase): + """Failed transitive downloads no longer fail silently -- the underlying + error is surfaced via the stdlib logger so ``--verbose`` makes the skip + diagnosable (#940).""" + + def test_download_callback_exception_logs_warning(self): + with TemporaryDirectory() as temp_dir: + project_root = Path(temp_dir) + apm_modules = project_root / "apm_modules" + apm_modules.mkdir() + + def boom(*args, **kwargs): + raise RuntimeError("simulated network failure") + + resolver = APMDependencyResolver(download_callback=boom) + resolver._project_root = project_root + resolver._apm_modules_dir = apm_modules + + dep = DependencyReference(repo_url="user/repo") + + from src.apm_cli.deps import apm_resolver as _resolver_mod + with self.assertLogs( + _resolver_mod._logger.name, level="WARNING" + ) as captured: + result = resolver._try_load_dependency_package( + dep, parent_chain="root", parent_pkg=None + ) + assert result is None + joined = "\n".join(captured.output) + assert "simulated network failure" in joined + assert "user/repo" in joined or "user_repo" in joined or "repo" in joined + + if __name__ == '__main__': unittest.main() \ No newline at end of file diff --git a/tests/unit/test_install_command.py b/tests/unit/test_install_command.py index bde802e39..dbd27c94c 100644 --- a/tests/unit/test_install_command.py +++ b/tests/unit/test_install_command.py @@ -454,10 +454,11 @@ def test_download_callback_includes_chain_in_error(self, tmp_path): # Track what the callback receives callback_calls = [] - def tracking_callback(dep_ref, mods_dir, parent_chain=""): + def tracking_callback(dep_ref, mods_dir, parent_chain="", parent_pkg=None): callback_calls.append({ "dep": dep_ref.get_display_name(), "parent_chain": parent_chain, + "parent_pkg": parent_pkg, }) if "leaf-pkg" in dep_ref.get_display_name(): # Simulate what the real callback does: catch internal error, diff --git a/tests/unit/test_local_deps.py b/tests/unit/test_local_deps.py index 323d23009..13e69ffae 100644 --- a/tests/unit/test_local_deps.py +++ b/tests/unit/test_local_deps.py @@ -533,3 +533,53 @@ def test_copy_preserves_symlinks_without_following(self, tmp_path): # The symlink should be preserved as a symlink, NOT followed link = install_path / "escape" assert link.is_symlink(), "Symlink was followed instead of preserved" + + def test_copy_local_package_rejects_target_outside_project_root(self, tmp_path): + """When project_root is supplied, sources resolving outside it are + refused to block path-traversal in transitive ``local_path`` deps (#940).""" + from apm_cli.commands.install import _copy_local_package + + project_root = tmp_path / "project" + project_root.mkdir() + # An attacker-controlled path that resolves outside project_root. + outside_pkg = tmp_path / "outside" / "evil-pkg" + outside_pkg.mkdir(parents=True) + (outside_pkg / "apm.yml").write_text(yaml.dump({ + "name": "evil-pkg", "version": "1.0.0", + })) + + dep_ref = DependencyReference.parse("../outside/evil-pkg") + dep_ref.local_path = "../outside/evil-pkg" + install_path = project_root / "apm_modules" / "_local" / "evil-pkg" + + # base_dir = project_root would resolve to outside/evil-pkg, which + # escapes project_root: must be rejected. + result = _copy_local_package( + dep_ref, install_path, project_root, + project_root=project_root, + ) + assert result is None + assert not install_path.exists(), "no copy should have happened" + + def test_copy_local_package_allows_target_inside_project_root(self, tmp_path): + """Containment guard does not block legitimate within-project deps.""" + from apm_cli.commands.install import _copy_local_package + + project_root = tmp_path / "project" + project_root.mkdir() + sub_pkg = project_root / "packages" / "shared" + sub_pkg.mkdir(parents=True) + (sub_pkg / "apm.yml").write_text(yaml.dump({ + "name": "shared", "version": "1.0.0", + })) + + dep_ref = DependencyReference.parse("./packages/shared") + dep_ref.local_path = "./packages/shared" + install_path = project_root / "apm_modules" / "_local" / "shared" + + result = _copy_local_package( + dep_ref, install_path, project_root, + project_root=project_root, + ) + assert result is not None + assert (result / "apm.yml").exists()