* [security] fix(workspace): block all symlink escapes from the selected workspace (#3398, @Hinotoi-agent) Previously safe_resolve_ws allowed a symlink placed inside a workspace to resolve to an external host path as long as it wasn't a system dir (/etc, /proc, etc). But the workspace file API is reachable by LLM agent tool calls (read_file_content), so an in-workspace symlink to ~/.ssh, ~/.hermes/auth.json (credentials), etc. was a real read path. Now ANY symlink escape is blocked: safe_resolve_ws resolves and requires the result stay under the workspace root; list_dir hides escaping symlinks (they could never be opened anyway); internal symlinks resolving back under the workspace still work. Updated the upload symlink-target test to accept the new 400 'Path traversal blocked' rejection (was 403) — the invariant (nothing lands outside the workspace) is unchanged. Co-authored-by: Hinotoi-agent <Hinotoi-agent@users.noreply.github.com> * docs(changelog): v0.51.221 release header for #3398 symlink-escape security fix * [security] harden workspace file API against symlink-swap TOCTOU via portable anchored openat-walk (#3398 follow-up) Codex review of #3398 flagged that safe_resolve_ws() validates a path but list_dir/read_file_content/upload/extraction then re-open by pathname, leaving a TOCTOU window: a symlink swapped in AFTER the check could still escape. (This race pre-existed #3398; closing it here so the containment is complete.) A first attempt used /proc/self/fd for the post-open containment check, but that BRICKS workspace browsing on macOS/Windows (no /proc → every read/list rejected). This version is portable: - open_anchored_fd(): opens the (already symlink-resolved) target component-by-component from the workspace root via openat (dir_fd) + O_NOFOLLOW. Every component must be a real non-symlink entry, so a component swapped to a symlink mid-flight is refused. No /proc dependency. Used by read_file_content (read from the fd) and list_dir (enumerate via os.scandir(fd), per-entry fstatat/readlinkat). - open_anchored_create_fd(): same anchored walk for writes, creating missing intermediate dirs with mkdir(dir_fd=) and the leaf with O_CREAT|O_EXCL| O_NOFOLLOW. Used by the workspace upload write AND archive (zip+tar) member writes, anchored against the TRUE workspace root (not the mutable extraction dest_dir, closing Codex's root-swap finding). fd-leak-safe on rejection. - Portability: gated on os.supports_dir_fd; platforms without it (Windows, where symlink creation needs admin) fall back to a plain O_NOFOLLOW open/exclusive create — no new race protection but no regression vs the prior path-based code. Legit in-workspace symlinks still resolve and read/list fine (safe_resolve_ws collapses them to a real in-workspace path, which the anchored walk then opens). Verified: the swap-race leaks external content against the old path-based read and is blocked here; macOS-class symlinked-root workspaces work; no fd leak over 300 rejected creates. Adds TOCTOU + anchored-create regression tests. * [security] close 3 more #3398 TOCTOU gaps from Codex r3: root-swap, pre-create mkdir, Windows list_dir fallback Codex round-3 review found three residual issues in the anchored openat-walk: 1. (CORE) The workspace ROOT itself could be swapped to a symlink after resolve() but before the root os.open() — add _O_NOFOLLOW to the root open in open_anchored_fd() and open_anchored_create_fd() (and make_anchored_dir()), so a raced root symlink is refused. Verified: root-swap race now blocked. 2. (SILENT) Upload/extraction still did pathname Path.mkdir() AFTER the containment check, so a raced symlink component could make the server create dirs outside the workspace before the anchored file create rejected. Removed the redundant member_path.parent.mkdir() calls (open_anchored_create_fd already creates intermediates via anchored mkdirat) and replaced the two base-dir mkdirs (upload target dir + archive extraction root) with a new make_anchored_dir() that walks from the true workspace root via openat+O_NOFOLLOW + mkdir(dir_fd=). 3. (CORE) list_dir() unconditionally used os.scandir(fd)/os.stat(dir_fd=)/ os.readlink(dir_fd=), which would brick workspace browsing on platforms without os.supports_dir_fd (Windows). Split list_dir() into a _DIR_FD_OK anchored branch and a path-based fallback branch (prior behaviour) sharing one _process() entry builder. open_anchored_create_fd()'s Windows fallback now also creates parent dirs. Adds regression tests: no-dir_fd fallback (list+read+create+symlink filtering) and the root-swap race. All prior TOCTOU + anchored-create tests still green. * fix(workspace): portable symlink-loop filtering in list_dir via follow-stat ELOOP CI on Python 3.13 caught test_mutual_symlink_loop_filtered failing: a mutual symlink loop (a->b->a) was NOT filtered from the listing. Root cause: the new readlink-based cycle detection relied on (target_resolved / raw_link).resolve() RAISING on a loop, but Path.resolve() loop handling differs by Python version (3.11 raises RuntimeError, 3.13 can return a path), so the loop slipped through on 3.13. Fix: compute a version-independent 'reachable' flag per symlink via os.stat(..., follow_symlinks=True) — the syscall reliably returns ELOOP for mutual/self loops and ENOENT for broken targets on every platform/version. A symlink whose follow-stat raises can never be opened, so list_dir filters it. Applied in both the dir_fd-anchored branch (fd-relative stat) and the Windows path-based fallback branch. Mutual loop now filtered on all versions. --------- Co-authored-by: nesquena-hermes <[email protected]> Co-authored-by: Hinotoi-agent <Hinotoi-agent@users.noreply.github.com>
201 lines
8.4 KiB
Python
201 lines
8.4 KiB
Python
"""
|
|
Tests for symlink cycle detection in workspace file browser.
|
|
|
|
When a workspace contains symlinks (especially to directories outside the
|
|
workspace root), the directory listing must terminate without infinite
|
|
recursion. Covers:
|
|
|
|
- External symlink dirs (e.g. ln -s /some/path ~/workspace/link)
|
|
- Self-referencing symlink (ln -s . ~/workspace/loop)
|
|
- Ancestor symlink (ln -s .. ~/workspace/up)
|
|
- Internal symlink entries carry correct type / is_dir / target fields
|
|
- External symlink directories are hidden from listings and cannot be traversed
|
|
"""
|
|
import json
|
|
import os
|
|
import pathlib
|
|
import urllib.request
|
|
import urllib.error
|
|
import tempfile
|
|
|
|
from tests._pytest_port import BASE
|
|
|
|
|
|
def get(path):
|
|
url = BASE + path
|
|
with urllib.request.urlopen(url, timeout=10) as r:
|
|
return json.loads(r.read())
|
|
|
|
|
|
def post(path, body=None):
|
|
url = BASE + path
|
|
data = json.dumps(body or {}).encode()
|
|
req = urllib.request.Request(url, data=data,
|
|
headers={"Content-Type": "application/json"})
|
|
try:
|
|
with urllib.request.urlopen(req, timeout=10) as r:
|
|
return json.loads(r.read()), r.status
|
|
except urllib.error.HTTPError as e:
|
|
return json.loads(e.read()), e.code
|
|
|
|
|
|
def make_session(created_list, ws=None):
|
|
body = {}
|
|
if ws:
|
|
# tmp_path_factory creates dirs under /var/folders or /tmp which sit
|
|
# outside the user home tree, so they aren't trusted by default.
|
|
# Register the workspace first via the explicit add API (intent-trusted)
|
|
# before requesting a session against it.
|
|
post("/api/workspaces/add", {"path": str(ws)})
|
|
body["workspace"] = str(ws)
|
|
d, _ = post("/api/session/new", body)
|
|
sid = d["session"]["session_id"]
|
|
created_list.append(sid)
|
|
return sid, pathlib.Path(d["session"]["workspace"])
|
|
|
|
|
|
class TestSymlinkCycleDetection:
|
|
"""Symlink cycle detection in list_dir / safe_resolve_ws."""
|
|
|
|
def test_external_symlink_filtered_from_listing(self, cleanup_test_sessions, tmp_path_factory):
|
|
"""External symlink dirs should be hidden from workspace listings."""
|
|
ws = tmp_path_factory.mktemp("ws")
|
|
target = tmp_path_factory.mktemp("target")
|
|
(target / "file.txt").write_text("hello")
|
|
link = ws / "ext"
|
|
link.symlink_to(target)
|
|
|
|
sid, _ = make_session(cleanup_test_sessions, ws)
|
|
listing = get(f"/api/list?session_id={sid}&path=.")
|
|
names = [e["name"] for e in listing["entries"]]
|
|
assert "ext" not in names
|
|
|
|
def test_internal_symlink_listed_as_symlink(self, cleanup_test_sessions, tmp_path_factory):
|
|
"""Internal symlink dirs should appear with type='symlink', is_dir=True."""
|
|
ws = tmp_path_factory.mktemp("ws")
|
|
target = ws / "target"
|
|
target.mkdir()
|
|
(target / "file.txt").write_text("hello")
|
|
link = ws / "internal"
|
|
link.symlink_to(target)
|
|
|
|
sid, _ = make_session(cleanup_test_sessions, ws)
|
|
listing = get(f"/api/list?session_id={sid}&path=.")
|
|
entries = listing["entries"]
|
|
internal = [e for e in entries if e["name"] == "internal"]
|
|
assert len(internal) == 1
|
|
assert internal[0]["type"] == "symlink"
|
|
assert internal[0]["is_dir"] is True
|
|
assert internal[0]["target"] == str(target)
|
|
|
|
def test_external_symlink_not_browsable(self, cleanup_test_sessions, tmp_path_factory):
|
|
"""Listing inside an external symlink dir is blocked at the workspace boundary."""
|
|
ws = tmp_path_factory.mktemp("ws")
|
|
target = tmp_path_factory.mktemp("target")
|
|
(target / "inner.txt").write_text("data")
|
|
(ws / "ext").symlink_to(target)
|
|
|
|
sid, _ = make_session(cleanup_test_sessions, ws)
|
|
try:
|
|
get(f"/api/list?session_id={sid}&path=ext")
|
|
assert False, "External symlink traversal should be blocked"
|
|
except urllib.error.HTTPError as e:
|
|
assert e.code in (400, 404, 500)
|
|
|
|
def test_self_referencing_symlink_filtered(self, cleanup_test_sessions, tmp_path_factory):
|
|
"""Symlink pointing to the workspace root itself must be filtered out."""
|
|
ws = tmp_path_factory.mktemp("ws")
|
|
(ws / "file.txt").write_text("data")
|
|
(ws / "loop").symlink_to(ws)
|
|
|
|
sid, _ = make_session(cleanup_test_sessions, ws)
|
|
listing = get(f"/api/list?session_id={sid}&path=.")
|
|
names = [e["name"] for e in listing["entries"]]
|
|
assert "loop" not in names, "Self-referencing symlink should be filtered"
|
|
|
|
def test_ancestor_symlink_filtered(self, cleanup_test_sessions, tmp_path_factory):
|
|
"""Symlink pointing to a parent of the workspace must be filtered out."""
|
|
parent = tmp_path_factory.mktemp("parent")
|
|
ws = parent / "workspace"
|
|
ws.mkdir()
|
|
(ws / "file.txt").write_text("data")
|
|
# Symlink pointing to parent dir (ancestor of workspace)
|
|
(ws / "up").symlink_to(parent)
|
|
|
|
sid, _ = make_session(cleanup_test_sessions, ws)
|
|
listing = get(f"/api/list?session_id={sid}&path=.")
|
|
names = [e["name"] for e in listing["entries"]]
|
|
assert "up" not in names, "Ancestor symlink should be filtered"
|
|
|
|
def test_mutual_symlink_loop_filtered(self, cleanup_test_sessions, tmp_path_factory):
|
|
"""Mutually recursive symlinks should be skipped instead of raising RuntimeError."""
|
|
ws = tmp_path_factory.mktemp("ws")
|
|
(ws / "a").symlink_to(ws / "b")
|
|
(ws / "b").symlink_to(ws / "a")
|
|
|
|
sid, _ = make_session(cleanup_test_sessions, ws)
|
|
listing = get(f"/api/list?session_id={sid}&path=.")
|
|
names = [e["name"] for e in listing["entries"]]
|
|
assert "a" not in names
|
|
assert "b" not in names
|
|
|
|
def test_symlink_cycle_in_subdir(self, cleanup_test_sessions, tmp_path_factory):
|
|
"""External symlink subpaths must be blocked instead of traversed."""
|
|
ws = tmp_path_factory.mktemp("ws")
|
|
target = tmp_path_factory.mktemp("target")
|
|
(target / "subdir").mkdir()
|
|
# Create a symlink inside target that points back to workspace
|
|
(target / "subdir" / "back").symlink_to(ws)
|
|
(ws / "ext").symlink_to(target)
|
|
|
|
sid, _ = make_session(cleanup_test_sessions, ws)
|
|
# List root — should hide the external symlink and not recurse.
|
|
listing = get(f"/api/list?session_id={sid}&path=.")
|
|
names = [e["name"] for e in listing["entries"]]
|
|
assert "ext" not in names
|
|
|
|
# Traversing into ext/subdir crosses the workspace boundary and is blocked.
|
|
try:
|
|
get(f"/api/list?session_id={sid}&path=ext/subdir")
|
|
assert False, "External symlink subpath traversal should be blocked"
|
|
except urllib.error.HTTPError as e:
|
|
assert e.code in (400, 404, 500)
|
|
|
|
def test_internal_symlink_file_entry(self, cleanup_test_sessions, tmp_path_factory):
|
|
"""Internal symlink to a file should have is_dir=False and include size."""
|
|
ws = tmp_path_factory.mktemp("ws")
|
|
real = ws / "real"
|
|
real.mkdir()
|
|
(real / "data.txt").write_text("hello world")
|
|
(ws / "link.txt").symlink_to(real / "data.txt")
|
|
|
|
sid, _ = make_session(cleanup_test_sessions, ws)
|
|
listing = get(f"/api/list?session_id={sid}&path=.")
|
|
link = [e for e in listing["entries"] if e["name"] == "link.txt"]
|
|
assert len(link) == 1
|
|
assert link[0]["type"] == "symlink"
|
|
assert link[0]["is_dir"] is False
|
|
assert link[0]["size"] == 11 # len("hello world")
|
|
|
|
def test_external_symlink_file_filtered_from_listing(self, cleanup_test_sessions, tmp_path_factory):
|
|
"""External symlink files should be hidden from workspace listings."""
|
|
ws = tmp_path_factory.mktemp("ws")
|
|
real = tmp_path_factory.mktemp("real")
|
|
(real / "data.txt").write_text("hello world")
|
|
(ws / "link.txt").symlink_to(real / "data.txt")
|
|
|
|
sid, _ = make_session(cleanup_test_sessions, ws)
|
|
listing = get(f"/api/list?session_id={sid}&path=.")
|
|
names = [e["name"] for e in listing["entries"]]
|
|
assert "link.txt" not in names
|
|
|
|
def test_path_traversal_still_blocked(self, cleanup_test_sessions, tmp_path_factory):
|
|
"""Raw .. traversal must still be blocked even with symlink support."""
|
|
ws = tmp_path_factory.mktemp("ws")
|
|
sid, _ = make_session(cleanup_test_sessions, ws)
|
|
try:
|
|
get(f"/api/list?session_id={sid}&path=../../../etc")
|
|
assert False, "Path traversal should be blocked"
|
|
except urllib.error.HTTPError as e:
|
|
assert e.code in (400, 404, 500)
|