Skip to content

Commit db4e6b0

Browse files
author
codejunkie99
committed
fix(schema)!: reject Windows-style path traversal + skills_link paths
Security hardening for manifest path validation. Two P1s flagged post-merge: 1. Windows traversal bypass: the existing guard tokenized only on `/` and treated only `/`-prefixed paths as absolute. Inputs like `..\..\outside`, `\\server\share`, `C:\temp\x`, or `C:foo` slipped through and would let install/remove touch arbitrary filesystem locations when run on Windows. New `_check_path_safe` helper normalizes both separators and rejects every common absolute-path form (POSIX root, Windows root, UNC, drive-letter, drive-relative). 2. skills_link unchecked: `target` and `dst` fields were only validated for presence, then joined with target_root in `_resolve_skills_link`. A manifest with `../../outside` or `/tmp/x` in either field could redirect symlink/rsync into arbitrary paths on any platform. Same `_check_path_safe` helper now applies. 13-case smoke test covers POSIX traversal, Windows traversal, forward-slash and backslash absolute, UNC, drive-absolute, drive-relative, and the lowercase-drive variant. Tests: 40 passed, 1 skipped.
1 parent 7dd35fd commit db4e6b0

2 files changed

Lines changed: 61 additions & 8 deletions

File tree

CHANGELOG.md

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,20 @@ and the project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.
4646
Thanks to @palamp for the report and the proposal that shaped the
4747
larger feature.
4848

49+
### Security
50+
- **Manifest path-safety hardening (`harness_manager/schema.py`).** The
51+
pre-existing path-traversal guard only tokenized on `/` and only
52+
treated `/`-prefixed paths as absolute, so Windows-style inputs
53+
(`..\..\outside`, `\\server\share`, `C:\temp\x`, `C:foo`) bypassed
54+
validation and could let install/remove read or write outside the
55+
adapter/project roots when run on Windows. Also extended the same
56+
validation to `skills_link.target` and `skills_link.dst`, which were
57+
previously only checked for presence — a manifest could otherwise
58+
point the symlink/rsync into arbitrary filesystem locations on any
59+
platform. Both POSIX and Windows separators are now normalized
60+
before traversal detection, and every common absolute-path form
61+
(POSIX root, Windows root, UNC, drive-letter) is rejected.
62+
4963
### Changed
5064
- `install.sh` shrinks from 175 lines of bash case-statements to 35
5165
lines of dispatcher. All install logic moved to `harness_manager/`.

harness_manager/schema.py

Lines changed: 47 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,39 @@ def _check_optional(d: dict, key: str, types: tuple, source: str) -> Any:
5151
return val
5252

5353

54+
def _check_path_safe(p: str, source: str, field: str) -> None:
55+
"""Reject path traversal and absolute paths on POSIX AND Windows.
56+
57+
Manifests are read by an installer that joins these paths with
58+
target_root and writes/symlinks the result. A malicious or buggy
59+
manifest with `../../outside`, `..\\..\\outside`, `/tmp/x`,
60+
`\\\\server\\share`, or `C:\\temp\\x` would otherwise let install
61+
or remove touch arbitrary filesystem locations. The original guard
62+
only tokenized on `/` and only treated `/`-prefixed paths as
63+
absolute, so Windows-style inputs slipped through. Tokenize on
64+
both separators and reject every common absolute-path form.
65+
"""
66+
parts = p.replace("\\", "/").split("/")
67+
if ".." in parts:
68+
raise ManifestError(
69+
source,
70+
f"'{field}': '..' path components not allowed (path traversal)",
71+
)
72+
# POSIX absolute (/foo) or Windows root/UNC (\foo, \\server\share).
73+
if p.startswith("/") or p.startswith("\\"):
74+
raise ManifestError(
75+
source,
76+
f"'{field}': absolute paths not allowed; use repo-relative paths",
77+
)
78+
# Windows drive prefix: C:foo (drive-relative), C:\foo / C:/foo
79+
# (absolute). Catches every X:... form regardless of separator.
80+
if len(p) >= 2 and p[1] == ":" and p[0].isalpha():
81+
raise ManifestError(
82+
source,
83+
f"'{field}': drive-letter paths not allowed; use repo-relative paths",
84+
)
85+
86+
5487
def _validate_files(files: list, source: str) -> None:
5588
if not files:
5689
raise ManifestError(source, "'files' must contain at least one entry")
@@ -62,10 +95,8 @@ def _validate_files(files: list, source: str) -> None:
6295
dst = _require(entry, "dst", (str,), es)
6396
if not src or not dst:
6497
raise ManifestError(es, "'src' and 'dst' must be non-empty strings")
65-
if ".." in src.split("/") or ".." in dst.split("/"):
66-
raise ManifestError(es, "'..' path components not allowed (path traversal)")
67-
if src.startswith("/") or dst.startswith("/"):
68-
raise ManifestError(es, "absolute paths not allowed; use repo-relative paths")
98+
_check_path_safe(src, es, "src")
99+
_check_path_safe(dst, es, "dst")
69100
policy = _check_optional(entry, "merge_policy", (str,), es)
70101
if policy is not None and policy not in VALID_MERGE_POLICIES:
71102
raise ManifestError(
@@ -78,11 +109,19 @@ def _validate_files(files: list, source: str) -> None:
78109
def _validate_skills_link(link: dict, source: str) -> None:
79110
if not isinstance(link, dict):
80111
raise ManifestError(source, "'skills_link' must be an object")
81-
target = _require(link, "target", (str,), f"{source} skills_link")
82-
dst = _require(link, "dst", (str,), f"{source} skills_link")
112+
sl_source = f"{source} skills_link"
113+
target = _require(link, "target", (str,), sl_source)
114+
dst = _require(link, "dst", (str,), sl_source)
83115
if not target or not dst:
84-
raise ManifestError(f"{source} skills_link", "'target' and 'dst' must be non-empty")
85-
fallback = _check_optional(link, "fallback", (str,), f"{source} skills_link")
116+
raise ManifestError(sl_source, "'target' and 'dst' must be non-empty")
117+
# Same path-safety check as file entries: skills_link.target and
118+
# skills_link.dst are joined with target_root in _resolve_skills_link
119+
# without normalization, so a manifest with `..\..\outside` or
120+
# `/tmp/x` here would let install/remove symlink or rsync into
121+
# arbitrary filesystem locations on either POSIX or Windows.
122+
_check_path_safe(target, sl_source, "target")
123+
_check_path_safe(dst, sl_source, "dst")
124+
fallback = _check_optional(link, "fallback", (str,), sl_source)
86125
if fallback is not None and fallback not in VALID_FALLBACKS:
87126
raise ManifestError(
88127
f"{source} skills_link",

0 commit comments

Comments
 (0)