fix: harden dependency path validation#364
Merged
danielmeppiel merged 4 commits intomainfrom Mar 18, 2026
Merged
Conversation
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 <[email protected]>
danielmeppiel
added
the
bug
Contributor
There was a problem hiding this comment.
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.pywithensure_path_within()andsafe_rmtree()for centralized containment enforcement. - Hardens
DependencyReferenceparsing andget_install_path()to reject traversal segments and verify computed install paths stay withinapm_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. SinceDependencyReferenceinstances can be constructed from lockfile data (e.g., withis_local=True/local_path=...), a craftedlocal_pathbasename like..could make the computed install path resolve toapm_modules/(or worse) and then be passed to deletion/copy operations. Compute the local install path intoresult, validate the basename is non-empty and not./.., and runensure_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 checkssub_path.split('/'). On Windows, a user can supply backslash-separated segments (e.g.,..\\..\\etc) whichpathlibwill treat as separators, bypassing this check and relying solely on later containment enforcement. Normalizesub_pathto 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_pathonly checksvirtual_path.split('/'). On Windows, backslashes are path separators, so a value likeprompts\\..\\..\\etccould bypass this parse-time check. Consider normalizingvirtual_pathto 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
…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 <[email protected]>
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 <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
utils/path_securitymodule —ensure_path_within(),safe_rmtree(), andPathTraversalErrorfor centralized path containment checksDependencyReference.parse()andparse_from_dict()now reject invalid path segments during dependency string parsingget_install_path()containment — validates its return value stays withinapm_modules_dirbefore returninguninstall,prune, andinstallcommands usesafe_rmtree()for user-derived deletion targetsgithub_downloadervalidates subdirectory source paths within the temp clone directoryTesting
test_path_security.pycovering containment checks, parse rejection, andget_install_pathvalidation