diff --git a/backend/apps/outputs/outputs.py b/backend/apps/outputs/outputs.py index 62dfd17c4..f61da5a1e 100644 --- a/backend/apps/outputs/outputs.py +++ b/backend/apps/outputs/outputs.py @@ -51,6 +51,24 @@ logger = logging.getLogger(__name__) +def safe_workspace_path(folder: str, rel: str) -> Optional[str]: + """Resolve `rel` under workspace `folder`, returning the real path only if it stays inside. + + `os.path.realpath` (not `os.path.normpath`) so a symlink ANYWHERE in the + tree, including an intermediate component of a not-yet-created write target, + can't redirect the read/write/delete outside the workspace (issue #135). + Both sides are realpath'd, so a legit symlink in the base (e.g. macOS + `/var`->`/private/var`) doesn't false-reject. Returns None on escape; the + caller decides whether to 403 or skip. The `+ os.sep` guard also closes the + sibling prefix collision (`abc` vs `abc-evil`). + """ + root = os.path.realpath(folder) + dest = os.path.realpath(os.path.join(folder, rel)) + if dest != root and not dest.startswith(root + os.sep): + return None + return dest + + @asynccontextmanager async def outputs_lifespan(): os.makedirs(DATA_DIR, exist_ok=True) @@ -77,8 +95,8 @@ async def outputs_lifespan(): async def serve_workspace_file(workspace_id: str, filepath: str, p_d: str = ""): """Serve a file from a workspace folder. For index.html, inject OUTPUT data.""" folder = os.path.join(WORKSPACE_DIR, workspace_id) - full_path = os.path.normpath(os.path.join(folder, filepath)) - if not full_path.startswith(os.path.normpath(folder)): + full_path = safe_workspace_path(folder, filepath) + if full_path is None: raise HTTPException(status_code=403, detail="Path traversal not allowed") if not os.path.isfile(full_path): raise HTTPException(status_code=404, detail="File not found") @@ -364,8 +382,8 @@ async def seed_workspace(body: WorkspaceSeedRequest): # Legacy flat path. Seed only fills in MISSING files; it never overwrites what's already on disk. A reopen re-sends the inline output.files snapshot, which lags behind whatever the agent just wrote to the workspace; writing it back reverted every edited file (new files survived, edited ones snapped to the snapshot). Disk wins once an app exists. if body.files: for rel_path, content in body.files.items(): - full_path = os.path.normpath(os.path.join(folder, rel_path)) - if not full_path.startswith(os.path.normpath(folder)): + full_path = safe_workspace_path(folder, rel_path) + if full_path is None: continue if os.path.exists(full_path): continue @@ -525,10 +543,8 @@ async def write_workspace_file(workspace_id: str, filepath: str, body: dict): folder = os.path.join(WORKSPACE_DIR, workspace_id) if not os.path.isdir(folder): raise HTTPException(status_code=404, detail="Workspace not found") - folder_norm = os.path.normpath(folder) - full_path = os.path.normpath(os.path.join(folder, filepath)) - # `startswith(folder_norm + os.sep)` (not just folder_norm) so a workspace `abc-123` can't be tricked into writing into a sibling `abc-1234-evil`, prefix-string collision rather than path-component containment. Today's UUID-format ids make the collision unlikely in practice, but the check is one character and immunizes future id schemes. - if full_path != folder_norm and not full_path.startswith(folder_norm + os.sep): + full_path = safe_workspace_path(folder, filepath) + if full_path is None: raise HTTPException(status_code=403, detail="Path traversal not allowed") content = body.get("content", "") if would_shrink_oversize_file(full_path, content): @@ -545,14 +561,13 @@ async def delete_workspace_file(workspace_id: str, filepath: str): folder = os.path.join(WORKSPACE_DIR, workspace_id) if not os.path.isdir(folder): raise HTTPException(status_code=404, detail="Workspace not found") - folder_norm = os.path.normpath(folder) - full_path = os.path.normpath(os.path.join(folder, filepath)) - if full_path != folder_norm and not full_path.startswith(folder_norm + os.sep): + full_path = safe_workspace_path(folder, filepath) + if full_path is None: raise HTTPException(status_code=403, detail="Path traversal not allowed") if os.path.isfile(full_path): os.remove(full_path) parent = os.path.dirname(full_path) - while parent != os.path.normpath(folder): + while parent != os.path.realpath(folder): if os.path.isdir(parent) and not os.listdir(parent): os.rmdir(parent) parent = os.path.dirname(parent) diff --git a/backend/tests/test_outputs_workspace_path_traversal.py b/backend/tests/test_outputs_workspace_path_traversal.py new file mode 100644 index 000000000..350970be9 --- /dev/null +++ b/backend/tests/test_outputs_workspace_path_traversal.py @@ -0,0 +1,70 @@ +"""Regression tests for workspace path containment (issue #135). + +The Output file endpoints gated writes/reads/deletes with a lexical +`os.path.normpath` prefix check, which does NOT resolve symlinks: a symlink +inside a workspace redirected the operation outside it. `safe_workspace_path` +uses `os.path.realpath` so symlink escapes (and the sibling-prefix collision) +are rejected. + +Run: + cd backend && .venv/bin/python -m pytest tests/test_outputs_workspace_path_traversal.py -v +""" + +from __future__ import annotations + +import os + +import pytest + +from backend.apps.outputs.outputs import safe_workspace_path + + +def test_plain_relative_path_is_allowed(tmp_path): + ws = tmp_path / "ws" + ws.mkdir() + got = safe_workspace_path(str(ws), "sub/file.txt") + assert got == os.path.realpath(str(ws / "sub" / "file.txt")) + + +def test_dotdot_escape_is_rejected(tmp_path): + ws = tmp_path / "ws" + ws.mkdir() + (tmp_path / "secret.txt").write_text("s") + assert safe_workspace_path(str(ws), "../secret.txt") is None + + +def test_sibling_prefix_collision_is_rejected(tmp_path): + (tmp_path / "abc").mkdir() + (tmp_path / "abc-evil").mkdir() + # Lexically `abc/../abc-evil/x` starts with `.../abc`; realpath + os.sep must reject it. + assert safe_workspace_path(str(tmp_path / "abc"), "../abc-evil/x") is None + + +def test_symlink_component_escape_is_rejected(tmp_path): + """A symlink INSIDE the workspace pointing out of it must not be followed — + the core #135 bug. Skips where the OS won't let us create a symlink.""" + ws = tmp_path / "ws" + ws.mkdir() + outside = tmp_path / "outside" + outside.mkdir() + (outside / "secret.txt").write_text("top secret") + try: + os.symlink(str(outside), str(ws / "esc"), target_is_directory=True) + except (OSError, NotImplementedError) as e: + pytest.skip(f"symlink creation unavailable: {e}") + # Lexical normpath would accept `ws/esc/secret.txt`; realpath resolves esc->outside and rejects. + assert safe_workspace_path(str(ws), "esc/secret.txt") is None + + +def test_symlinked_base_is_not_false_rejected(tmp_path): + """A symlink in the BASE path (both sides realpath'd) must still allow + legitimate in-workspace paths, else macOS /var->/private/var breaks.""" + real = tmp_path / "real_ws" + real.mkdir() + link = tmp_path / "linked_ws" + try: + os.symlink(str(real), str(link), target_is_directory=True) + except (OSError, NotImplementedError) as e: + pytest.skip(f"symlink creation unavailable: {e}") + got = safe_workspace_path(str(link), "inner/file.txt") + assert got == os.path.realpath(str(real / "inner" / "file.txt"))