refactor: consolidate path traversal validation into shared utility

Replace inline segment-traversal checks in DependencyReference with
calls to a single validate_path_segments() function in path_security.py.
Replaces the inline SSH check from #458 with the shared utility.

The new utility normalises backslashes, rejects '.' and '..' segments,
and optionally rejects empty segments -- applied uniformly across all
parse paths (SSH, HTTPS, shorthand, dict, virtual) and the
defense-in-depth layer in get_install_path().

- Add validate_path_segments() to utils/path_security.py
- Replace all inline checks in models/dependency/reference.py
- Add 28 new tests (18 utility + 10 SSH traversal)
- 3107 tests pass, zero regressions

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: danielmeppiel

CHANGELOG.md

@@ -15,6 +15,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Skills now deploy to all active targets (`.opencode/`, `.cursor/`) instead of only `.github/` (#456)
 - `apm install` no longer rewrites `apm.lock.yaml` when dependencies are unchanged, eliminating `generated_at` churn in version control (#456)
 - `.github/` is no longer auto-created when other target dirs (`.claude/`, `.cursor/`, `.opencode/`) already exist; copilot is only the fallback for greenfield projects (#456)
+- SSH-style Git URLs (`git@host:owner/../evil`) now reject path traversal sequences, closing a bypass of the HTTPS validation added in #437 -- by @thakoreh (#458)
+
+### Changed
+
+- Consolidated path-segment traversal checks in `DependencyReference` into a single `validate_path_segments()` utility in `path_security.py`, eliminating behavioral drift (backslash normalisation now applied uniformly across all parse paths)
 
 ### Added
 

src/apm_cli/models/dependency/reference.py

@@ -15,7 +15,11 @@
     parse_artifactory_path,
     unsupported_host_error,
 )
-from ...utils.path_security import PathTraversalError, ensure_path_within
+from ...utils.path_security import (
+    PathTraversalError,
+    ensure_path_within,
+    validate_path_segments,
+)
 from ..validation import InvalidVirtualPackageExtensionError
 from .types import VirtualPackageType
 
@@ -309,33 +313,23 @@ def get_install_path(self, apm_modules_dir: Path) -> Path:
         """
         if self.is_local and self.local_path:
             pkg_dir_name = Path(self.local_path).name
-            if pkg_dir_name in ("", ".", ".."):
-                raise PathTraversalError(
-                    f"Invalid local package path '{self.local_path}': "
-                    f"basename must not be empty, '.', or '..'"
-                )
+            validate_path_segments(
+                pkg_dir_name,
+                context="local package path",
+                reject_empty=True,
+            )
             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 '..'"
-            )
+        validate_path_segments(self.repo_url, context="repo_url")
 
         # 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 '..'"
-            )
+        if self.virtual_path:
+            validate_path_segments(self.virtual_path, context="virtual_path")
         result: Path | None = None
 
         if self.is_virtual:
@@ -485,10 +479,7 @@ def parse_from_dict(cls, entry: dict) -> "DependencyReference":
             # 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 '..'"
-                )
+            validate_path_segments(sub_path, context="path")
 
         # Parse the git URL using the standard parser
         dep = cls.parse(git_url)
@@ -603,11 +594,7 @@ def _detect_virtual_package(cls, dependency_str: str):
             virtual_path = "/".join(path_segments[min_base_segments:])
 
             # Security: reject path traversal in virtual path
-            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 '..'"
-                )
+            validate_path_segments(virtual_path, context="virtual path")
 
             if "/collections/" in check_str or virtual_path.startswith("collections/"):
                 pass
@@ -656,13 +643,10 @@ def _parse_ssh_url(dependency_str: str):
 
         repo_url = repo_part.strip()
 
-        # Reject path traversal sequences in SSH URLs
-        for segment in repo_url.split('/'):
-            if not segment or segment in ('.', '..'):
-                raise PathTraversalError(
-                    f"Invalid SSH repository path '{repo_url}': "
-                    f"path segments must not be empty or be '.' or '..'"
-                )
+        # Security: reject traversal sequences in SSH repo paths
+        validate_path_segments(
+            repo_url, context="SSH repository path", reject_empty=True
+        )
 
         return host, repo_url, reference, alias
 
@@ -790,11 +774,10 @@ def _parse_standard_url(
             allowed_pattern = (
                 r"^[a-zA-Z0-9._\- ]+$" if is_ado_host else r"^[a-zA-Z0-9._-]+$"
             )
+            validate_path_segments(
+                "/".join(uparts), context="repository path"
+            )
             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}")
 
@@ -840,11 +823,12 @@ def _parse_standard_url(
         allowed_pattern = (
             r"^[a-zA-Z0-9._\- ]+$" if is_ado_host else r"^[a-zA-Z0-9._-]+$"
         )
-        for i, part in enumerate(path_parts):
-            if not part:
-                raise ValueError(
-                    f"Invalid repository format: path component {i+1} cannot be empty"
-                )
+        validate_path_segments(
+            "/".join(path_parts),
+            context="repository URL path",
+            reject_empty=True,
+        )
+        for part in path_parts:
             if not re.match(allowed_pattern, part):
                 raise ValueError(f"Invalid repository path component: {part}")
 
@@ -946,12 +930,9 @@ def parse(cls, dependency_str: str) -> "DependencyReference":
                     f"Invalid Azure DevOps repository format: {repo_url}. Expected 'org/project/repo'"
                 )
             ado_parts = repo_url.split("/")
-            for part in ado_parts:
-                if part in (".", ".."):
-                    raise PathTraversalError(
-                        f"Path traversal segment '{part}' is not allowed in "
-                        f"Azure DevOps repository path: {repo_url}"
-                    )
+            validate_path_segments(
+                repo_url, context="Azure DevOps repository path"
+            )
             ado_organization = ado_parts[0]
             ado_project = ado_parts[1]
             ado_repo = ado_parts[2]
@@ -965,12 +946,8 @@ def parse(cls, dependency_str: str) -> "DependencyReference":
                 raise ValueError(
                     f"Invalid repository format: {repo_url}. Contains invalid characters"
                 )
+            validate_path_segments(repo_url, context="repository path")
             for seg in segments:
-                if seg in (".", ".."):
-                    raise ValueError(
-                        f"Invalid repository format: {repo_url}. "
-                        f"Contains '.' or '..' path segments"
-                    )
                 if any(seg.endswith(ext) for ext in cls.VIRTUAL_FILE_EXTENSIONS):
                     raise ValueError(
                         f"Invalid repository format: '{repo_url}' contains a virtual file extension. "

src/apm_cli/utils/path_security.py

@@ -6,8 +6,11 @@
 
 Design
 ------
-* ``ensure_path_within`` is the single predicate -- resolves both paths and
-  asserts containment via ``Path.is_relative_to``.
+* ``validate_path_segments`` rejects traversal sequences (``.`` / ``..``)
+  at parse time -- before any path is constructed or written.
+* ``ensure_path_within`` is the single predicate for filesystem
+  containment -- resolves both paths and asserts via
+  ``Path.is_relative_to``.
 * ``safe_rmtree`` wraps ``robust_rmtree`` with an ``ensure_path_within``
   check so callers get a drop-in replacement.
 * ``PathTraversalError`` is a ``ValueError`` subclass for clear error
@@ -25,6 +28,45 @@ class PathTraversalError(ValueError):
     """Raised when a computed path escapes its expected base directory."""
 
 
+def validate_path_segments(
+    path_str: str,
+    *,
+    context: str = "path",
+    reject_empty: bool = False,
+) -> None:
+    """Reject path strings containing traversal sequences.
+
+    Normalises backslashes to forward slashes, splits on ``/``, and
+    rejects any segment that is ``.`` or ``..``.  Optionally rejects
+    empty segments (from ``//`` or trailing ``/``).
+
+    Parameters
+    ----------
+    path_str : str
+        Path-like string to validate (repo URL, virtual path, etc.).
+    context : str
+        Human-readable label for error messages.
+    reject_empty : bool
+        If *True*, also reject empty segments.
+
+    Raises
+    ------
+    PathTraversalError
+        If any segment fails validation.
+    """
+    for segment in path_str.replace("\\", "/").split("/"):
+        if segment in (".", ".."):
+            raise PathTraversalError(
+                f"Invalid {context} '{path_str}': "
+                f"segment '{segment}' is a traversal sequence"
+            )
+        if reject_empty and not segment:
+            raise PathTraversalError(
+                f"Invalid {context} '{path_str}': "
+                f"path segments must not be empty"
+            )
+
+
 def ensure_path_within(path: Path, base_dir: Path) -> Path:
     """Resolve *path* and assert it lives inside *base_dir*.
 

tests/unit/test_path_security.py

@@ -15,6 +15,7 @@
     PathTraversalError,
     ensure_path_within,
     safe_rmtree,
+    validate_path_segments,
 )
 from apm_cli.models.dependency import DependencyReference
 
@@ -118,6 +119,84 @@ def test_refuses_traversal_in_package_name(self, tmp_path):
         assert victim.exists()
 
 
+# ---------------------------------------------------------------------------
+# validate_path_segments
+# ---------------------------------------------------------------------------
+
+
+class TestValidatePathSegments:
+    """Unit tests for the validate_path_segments utility."""
+
+    def test_accepts_clean_path(self):
+        validate_path_segments("owner/repo")
+
+    def test_accepts_single_segment(self):
+        validate_path_segments("repo")
+
+    def test_accepts_deep_path(self):
+        validate_path_segments("org/project/repo/sub/dir")
+
+    def test_rejects_dotdot(self):
+        with pytest.raises(PathTraversalError):
+            validate_path_segments("owner/../evil")
+
+    def test_rejects_single_dot(self):
+        with pytest.raises(PathTraversalError):
+            validate_path_segments("owner/./repo")
+
+    def test_rejects_leading_dotdot(self):
+        with pytest.raises(PathTraversalError):
+            validate_path_segments("../escape")
+
+    def test_rejects_nested_dotdot(self):
+        with pytest.raises(PathTraversalError):
+            validate_path_segments("a/b/../../c")
+
+    def test_rejects_backslash_dotdot(self):
+        """Backslashes are normalised to forward slashes before checking."""
+        with pytest.raises(PathTraversalError):
+            validate_path_segments("owner\\..\\evil")
+
+    def test_rejects_mixed_separators(self):
+        with pytest.raises(PathTraversalError):
+            validate_path_segments("sub\\..\\..\\esc")
+
+    def test_empty_segments_allowed_by_default(self):
+        # Double-slash produces empty segments; allowed unless reject_empty
+        validate_path_segments("owner//repo")
+
+    def test_reject_empty_catches_double_slash(self):
+        with pytest.raises(PathTraversalError):
+            validate_path_segments("owner//repo", reject_empty=True)
+
+    def test_reject_empty_catches_trailing_slash(self):
+        with pytest.raises(PathTraversalError):
+            validate_path_segments("owner/repo/", reject_empty=True)
+
+    def test_reject_empty_catches_leading_slash(self):
+        with pytest.raises(PathTraversalError):
+            validate_path_segments("/owner/repo", reject_empty=True)
+
+    def test_reject_empty_passes_clean_path(self):
+        validate_path_segments("owner/repo", reject_empty=True)
+
+    def test_context_appears_in_message(self):
+        with pytest.raises(PathTraversalError, match="repo_url"):
+            validate_path_segments("a/../b", context="repo_url")
+
+    def test_bare_dot_rejected(self):
+        with pytest.raises(PathTraversalError):
+            validate_path_segments(".")
+
+    def test_bare_dotdot_rejected(self):
+        with pytest.raises(PathTraversalError):
+            validate_path_segments("..")
+
+    def test_empty_string_with_reject_empty(self):
+        with pytest.raises(PathTraversalError):
+            validate_path_segments("", reject_empty=True)
+
+
 # ---------------------------------------------------------------------------
 # DependencyReference parse-time traversal rejection
 # ---------------------------------------------------------------------------