fix: reject path traversal in SSH URL parsing#458
fix: reject path traversal in SSH URL parsing#458danielmeppiel merged 3 commits intomicrosoft:mainfrom
Conversation
SSH URLs (git@host:owner/repo) bypassed the path validation that HTTPS URLs already apply. Paths like git@host:owner/../etc would parse without error. Apply the same traversal check to _parse_ssh_url: reject '.' and '..' segments, and reject empty segments from double slashes. Fixes microsoft#455
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Pull request overview
Hardens DependencyReference.parse() by applying path traversal rejection to SSH-style Git URLs so SSH and HTTPS parsing paths enforce the same security constraints (fixes #455).
Changes:
- Add traversal/empty-segment validation to
_parse_ssh_url()to reject./..and//-style empty segments. - Add unit tests covering SSH URL traversal rejection and ensuring normal SSH URLs still parse correctly.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/apm_cli/models/dependency/reference.py |
Adds SSH repo path segment validation to block traversal sequences during parse. |
tests/unit/test_path_security.py |
Adds regression tests for SSH traversal payloads and valid SSH URL parsing. |
| if not segment or segment in ('.', '..'): | ||
| raise PathTraversalError( | ||
| f"Invalid SSH repository path '{repo_url}': " | ||
| f"path segments must not be '.' or '..'" |
There was a problem hiding this comment.
The traversal check rejects empty path segments (e.g., double slashes), but the error message only mentions '.' and '..'. Update the message (or the condition) so users get an accurate reason (empty segments vs traversal).
| f"path segments must not be '.' or '..'" | |
| f"path segments must not be empty or be '.' or '..'" |
| # --- SSH URL traversal rejection --- | ||
|
|
||
| def test_ssh_parse_rejects_dotdot_in_repo(self): | ||
| """SSH URLs with '..' traversal in the repo path must be rejected.""" | ||
| with pytest.raises(PathTraversalError): | ||
| DependencyReference.parse("git@github.com:owner/../evil") | ||
|
|
||
| def test_ssh_parse_rejects_nested_dotdot(self): | ||
| with pytest.raises(PathTraversalError): | ||
| DependencyReference.parse("git@github.com:org/../../etc/passwd") | ||
|
|
||
| def test_ssh_parse_rejects_single_dot(self): | ||
| with pytest.raises(PathTraversalError): | ||
| DependencyReference.parse("git@github.com:owner/./repo") | ||
|
|
||
| def test_ssh_parse_accepts_normal_url(self): | ||
| dep = DependencyReference.parse("git@github.com:owner/repo#main") | ||
| assert dep.repo_url == "owner/repo" | ||
| assert dep.reference == "main" | ||
|
|
||
| def test_ssh_parse_accepts_url_with_git_suffix(self): | ||
| dep = DependencyReference.parse("git@gitlab.com:team/project.git#v1.0") | ||
| assert dep.repo_url == "team/project" | ||
| assert dep.reference == "v1.0" | ||
|
|
||
| def test_ssh_parse_rejects_dotdot_with_alias(self): | ||
| with pytest.raises(PathTraversalError): | ||
| DependencyReference.parse("git@github.com:owner/../evil@my-alias") | ||
|
|
||
| def test_ssh_parse_rejects_dotdot_with_reference(self): | ||
| with pytest.raises(PathTraversalError): | ||
| DependencyReference.parse("git@github.com:owner/../../etc#main") | ||
|
|
There was a problem hiding this comment.
The new SSH traversal guard explicitly rejects empty segments (e.g., "git@github.com:owner//repo"), but the added tests only cover '.' and '..'. Add a test that double slashes (or leading/trailing slash) in the SSH repo path raises PathTraversalError so this behavior is locked in.
| # 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 '.' or '..'" | ||
| ) |
There was a problem hiding this comment.
This change tightens accepted SSH repo paths (rejecting '.'/'..' and empty segments). Please update the Starlight docs that describe dependency URL grammar/patterns (e.g., the repository path segment pattern in the dependency guide / manifest schema) to explicitly state that '.' and '..' segments are invalid, so the docs match the parser behavior.
…tests Update the SSH traversal error message to mention empty segments, and add tests for double-slash and trailing-slash cases in SSH URLs.
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>
Fixes #455
SSH URLs skipped the path component validation added for HTTPS URLs
in #437. You could construct
git@host:owner/../etcand it wouldparse successfully.
This applies the same traversal rejection to _parse_ssh_url that
parse_from_dict already performs on the 'path' field. It rejects
'.' and '..' segments and also catches empty segments from double
slashes.
Tests cover the SSH URL traversal cases.