Skip to content

fix: harden dependency path validation#364

Merged
danielmeppiel merged 4 commits intomainfrom
fix/validate-dependency-paths
Mar 18, 2026
Merged

fix: harden dependency path validation#364
danielmeppiel merged 4 commits intomainfrom
fix/validate-dependency-paths

Conversation

@danielmeppiel
Copy link
Collaborator

Summary

Adds defense-in-depth for dependency install path construction and filesystem operations. Ensures computed paths remain within the intended base directory at multiple layers.

Changes

  • New utils/path_security moduleensure_path_within(), safe_rmtree(), and PathTraversalError for centralized path containment checks
  • Parse-time validationDependencyReference.parse() and parse_from_dict() now reject invalid path segments during dependency string parsing
  • get_install_path() containment — validates its return value stays within apm_modules_dir before returning
  • Safe filesystem operationsuninstall, prune, and install commands use safe_rmtree() for user-derived deletion targets
  • Downloader validationgithub_downloader validates subdirectory source paths within the temp clone directory

Testing

  • 21 new unit tests in test_path_security.py covering containment checks, parse rejection, and get_install_path validation
  • All 2036 existing unit tests pass (zero regressions)
  • Integration smoke tests pass

Add defense-in-depth for dependency install path construction and
filesystem operations. Ensures computed paths remain within the
intended base directory at multiple layers.

Changes:
- New utils/path_security module: ensure_path_within(), safe_rmtree(),
  PathTraversalError for centralized path containment checks
- DependencyReference.parse() and parse_from_dict() now reject
  invalid path segments during dependency string parsing
- get_install_path() validates its return value stays within
  apm_modules_dir before returning
- uninstall, prune, and install commands use safe_rmtree() for
  user-derived deletion targets
- github_downloader validates subdirectory source paths within
  the temp clone directory

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings March 18, 2026 21:36
@danielmeppiel danielmeppiel added the bug Something isn't working label Mar 18, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds defense-in-depth protections against path traversal when computing dependency install paths and performing filesystem deletions, introducing centralized containment helpers and expanding validation at parse-time and during download/install/uninstall flows.

Changes:

  • Introduces utils/path_security.py with ensure_path_within() and safe_rmtree() for centralized containment enforcement.
  • Hardens DependencyReference parsing and get_install_path() to reject traversal segments and verify computed install paths stay within apm_modules/.
  • Switches uninstall/prune/install deletion sites to safe_rmtree() and adds downloader validation for subdirectory packages.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/unit/test_path_security.py Adds unit tests covering containment checks, safe deletion, and integration with dependency parsing/install path computation.
src/apm_cli/utils/path_security.py New centralized path containment and safe deletion helpers.
src/apm_cli/models/dependency.py Adds parse-time traversal rejection and containment enforcement in get_install_path().
src/apm_cli/deps/github_downloader.py Validates that requested subdirectories stay within the cloned repo directory.
src/apm_cli/commands/uninstall.py Uses safe_rmtree() when deleting installed dependency paths and handles traversal errors.
src/apm_cli/commands/prune.py Uses safe_rmtree() for pruning installed dependencies.
src/apm_cli/commands/install.py Uses safe_rmtree() for local dependency reinstall cleanup (defense-in-depth).
CHANGELOG.md Adds an Unreleased Security entry describing the hardening work.
Comments suppressed due to low confidence (3)

src/apm_cli/models/dependency.py:289

  • get_install_path() returns early for local dependencies without running the new containment check. Since DependencyReference instances can be constructed from lockfile data (e.g., with is_local=True / local_path=...), a crafted local_path basename like .. could make the computed install path resolve to apm_modules/ (or worse) and then be passed to deletion/copy operations. Compute the local install path into result, validate the basename is non-empty and not ./.., and run ensure_path_within(result, apm_modules_dir) before returning (same as the non-local cases).
            - ADO: apm_modules/org/project/repo/subdir/path/
        
        For local packages:

src/apm_cli/models/dependency.py:432

  • Traversal rejection in parse_from_dict() only checks sub_path.split('/'). On Windows, a user can supply backslash-separated segments (e.g., ..\\..\\etc) which pathlib will treat as separators, bypassing this check and relying solely on later containment enforcement. Normalize sub_path to forward slashes (or split on both '/' and '\\', and also strip both) before validating segments so traversal is rejected consistently cross-platform.

This issue also appears on line 631 of the same file.

            raise ValueError("Object-style dependency must have a 'git' or 'path' field")
        
        git_url = entry['git']
        if not isinstance(git_url, str) or not git_url.strip():
            raise ValueError("'git' field must be a non-empty string")
        

src/apm_cli/models/dependency.py:636

  • Traversal rejection for virtual_path only checks virtual_path.split('/'). On Windows, backslashes are path separators, so a value like prompts\\..\\..\\etc could bypass this parse-time check. Consider normalizing virtual_path to forward slashes (or splitting on both separators) before validating for ./.. segments.
                    any(seg.endswith(ext) for ext in cls.VIRTUAL_FILE_EXTENSIONS)
                    for seg in path_segments
                )
                has_collection = 'collections' in path_segments
                if has_virtual_ext or has_collection:
                    min_base_segments = 2  # Simple repo with virtual path

danielmeppiel and others added 2 commits March 18, 2026 23:08
…alization

- Guard local_path basename in get_install_path(): reject empty, '.',
  '..' basenames and run ensure_path_within() containment check
- Normalize backslashes to forward slashes in parse_from_dict() sub_path
  and parse() virtual_path before traversal segment checks (Windows compat)
- Add tests for backslash traversal and local path edge cases

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds segment-level traversal rejection directly in get_install_path()
for repo_url and virtual_path fields. This catches lockfile injection
and direct-construction cases that bypass parse-time validation.
Defense-in-depth: now validated at construction AND path resolution.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@danielmeppiel danielmeppiel merged commit 5adb278 into main Mar 18, 2026
7 checks passed
@danielmeppiel danielmeppiel deleted the fix/validate-dependency-paths branch March 18, 2026 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants