Skip to content

Commit 687a0b4

Browse files
committed
fix: guard repo_root.resolve() and .ts/.tsx swap against containment escape in ccarch resolver
- repo_root.resolve() now sits inside the try block so a symlink loop at/above repo_root reports missing instead of raising RuntimeError. - The .ts<->.tsx swap candidate is now routed through the same resolve()+relative_to(repo_resolved) containment check as the primary path (extracted into a shared _contained helper), so a swapped-extension symlink pointing outside the repo can no longer be reported as "moved". Adds regression tests reproducing both findings under tmp_path.
1 parent dcb5a00 commit 687a0b4

2 files changed

Lines changed: 71 additions & 7 deletions

File tree

mcp/ccarch/ccarch/resolver.py

Lines changed: 38 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,22 @@ def _swap_ext(p: str) -> str | None:
4141
return None
4242

4343

44+
def _contained(repo_root: Path, repo_resolved: Path, rel: str) -> Path | None:
45+
# Resolve rel against repo_root and confirm it stays inside
46+
# repo_resolved, returning the resolved Path if so or None otherwise.
47+
# Every candidate path derived from a doc citation — the primary q as
48+
# well as any alternate like a .ts/.tsx extension swap — must go
49+
# through this exact check before being probed with _safe_check.
50+
# Probing an unresolved/uncontained path directly is how a cited path
51+
# can end up resolving outside the repo (constraint B).
52+
try:
53+
candidate = (repo_root / rel).resolve()
54+
candidate.relative_to(repo_resolved)
55+
except (ValueError, OSError, RuntimeError):
56+
return None
57+
return candidate
58+
59+
4460
def resolve(repo_root: Path, cited: str) -> Resolved:
4561
repo_root = Path(repo_root)
4662
raw = (cited or "").strip()
@@ -52,18 +68,26 @@ def resolve(repo_root: Path, cited: str) -> Resolved:
5268

5369
# Never let a cited path escape the repo. A doc citing ../../etc/passwd
5470
# resolves to missing rather than probing outside the tree.
55-
repo_resolved = repo_root.resolve()
5671
try:
57-
target = (repo_root / q).resolve()
58-
target.relative_to(repo_resolved)
59-
except (ValueError, OSError, RuntimeError):
72+
# repo_root.resolve() must stay inside this try too: a circular
73+
# symlink at/above repo_root itself raises the same
74+
# RuntimeError("Symlink loop ...") that .resolve() raises for the
75+
# cited path below, and repo_resolved is still referenced later
76+
# (Python doesn't scope names to blocks), so binding it here is
77+
# both necessary and sufficient.
78+
repo_resolved = repo_root.resolve()
79+
except (OSError, RuntimeError):
6080
# Path.resolve() raises RuntimeError("Symlink loop from ...") for a
6181
# circular symlink instead of OSError/ValueError like every other
6282
# unresolvable-path case. It looks redundant to catch it here, but
63-
# it isn't — leave it, or a symlink loop in a doc-cited path will
83+
# it isn't — leave it, or a symlink loop at/above repo_root will
6484
# crash resolve() instead of reporting "missing".
6585
return Resolved(path=raw, status="missing", kind="dir" if is_dir_cite else "file")
6686

87+
target = _contained(repo_root, repo_resolved, q)
88+
if target is None:
89+
return Resolved(path=raw, status="missing", kind="dir" if is_dir_cite else "file")
90+
6791
# Degenerate citations like "", "///", ".", "./" all normalize to the
6892
# repo root itself once joined and resolved (repo_root / "" is
6993
# repo_root; "." and "./" resolve to the same directory). None of these
@@ -86,7 +110,14 @@ def resolve(repo_root: Path, cited: str) -> Resolved:
86110
return Resolved(path=q, status="ok", kind="dir")
87111

88112
alt = _swap_ext(q)
89-
if alt and _safe_check(repo_root / alt, "file"):
90-
return Resolved(path=alt, status="moved", kind="file")
113+
if alt:
114+
# The swapped-extension candidate must go through the same
115+
# containment check as q above (_contained), not a raw
116+
# repo_root / alt probe — _safe_check only guards OSError during
117+
# the stat, it does not re-verify the result stays inside the
118+
# repo, and is_file() happily follows a symlink to anywhere.
119+
alt_target = _contained(repo_root, repo_resolved, alt)
120+
if alt_target is not None and _safe_check(alt_target, "file"):
121+
return Resolved(path=alt, status="moved", kind="file")
91122

92123
return Resolved(path=q, status="missing", kind="file")

mcp/ccarch/tests/test_resolver.py

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,3 +103,36 @@ def test_permission_denied_dir_does_not_raise(tmp_path):
103103
finally:
104104
# Restore permissions so tmp_path teardown can clean up the dir.
105105
os.chmod(locked, 0o755)
106+
107+
108+
def test_symlink_loop_at_repo_root_does_not_raise(tmp_path):
109+
# Finding 1: repo_resolved = repo_root.resolve() used to sit *before*
110+
# the try: block, so a symlink loop at/above repo_root itself (as
111+
# opposed to inside the repo, which the loop test above already covers)
112+
# raised RuntimeError instead of being caught and reported as missing.
113+
loop_root = tmp_path / "looproot"
114+
loop_root.symlink_to(tmp_path / "looproot2")
115+
(tmp_path / "looproot2").symlink_to(loop_root)
116+
117+
r = resolve(loop_root, "src/foo.ts")
118+
assert r.status == "missing"
119+
120+
121+
def test_swapped_extension_symlink_escaping_repo_is_missing(tmp_path):
122+
# Finding 2: the .ts<->.tsx swap branch probed `repo_root / alt`
123+
# directly with _safe_check, never through .resolve() +
124+
# relative_to(repo_resolved) like the primary `q` path. So an in-repo
125+
# symlink at the swapped-extension path pointing outside the repo was
126+
# reported "moved" with a path string that actually dereferences
127+
# outside the tree, instead of "missing".
128+
outside = tmp_path / "outside"
129+
outside.mkdir()
130+
(outside / "secret.tsx").write_text("TOP SECRET CONTENT OUTSIDE REPO")
131+
132+
repo = tmp_path / "repo"
133+
tool_dir = repo / "packages" / "builtin-tools" / "src" / "tools" / "AgentTool"
134+
tool_dir.mkdir(parents=True)
135+
(tool_dir / "AgentTool.tsx").symlink_to(outside / "secret.tsx")
136+
137+
r = resolve(repo, "packages/builtin-tools/src/tools/AgentTool/AgentTool.ts")
138+
assert r.status == "missing"

0 commit comments

Comments
 (0)