diff --git a/CHANGELOG.md b/CHANGELOG.md index f5c410c..3be84fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,17 @@ # Hermes Web UI -- Changelog +## [v0.50.34] fix(workspace): restrict session workspaces to trusted roots [SECURITY] (#415) + +Session creation, update, chat-start, and workspace-add endpoints accepted arbitrary caller-supplied workspace paths. An authenticated caller could repoint a session to any directory the process could access, then use normal file read/write APIs to operate on attacker-chosen locations. CVSS 8.8 High (AV:N/AC:L/PR:L/UI:N/S:U/C:H/I:H/A:H). + +- `api/workspace.py`: new `resolve_trusted_workspace(path)` helper — resolves path, checks existence + is_dir, enforces `path.relative_to(_BOOT_DEFAULT_WORKSPACE)` containment; requests outside the WebUI workspace root fail with 400 +- `api/routes.py`: apply `resolve_trusted_workspace()` to all four entry points — `POST /api/session/new`, `POST /api/session/update`, `POST /api/chat/start` (workspace override), `POST /api/workspaces/add` +- `tests/test_sprint3.py`, `tests/test_sprint5.py`: regression tests for rejected outside-root paths on all four entry points; existing workspace tests updated to use trusted child directories +- `tests/test_sprint1.py`, `tests/test_sprint4.py`, `tests/test_sprint13.py`: aligned to new trusted-root contract +- Fix: use `_BOOT_DEFAULT_WORKSPACE` (respects `HERMES_WEBUI_DEFAULT_WORKSPACE` env for test isolation) rather than `_profile_default_workspace()` (reads agent terminal.cwd which may differ) +- Original PR by @Hinotoi-agent (cherry-picked; branch was 6 commits behind master) +- 1053 tests total (up from 1051; 2 pre-existing test_sprint5 isolation failures on master, not introduced by this PR) + ## [v0.50.33] fix: workspace panel close button — no duplicate X on desktop, mobile X respects file preview (#413) **Bug 1 — Duplicate X on desktop:** `#btnClearPreview` (the X icon) was always visible regardless of panel state, so desktop browse mode showed both the chevron collapse button and the X simultaneously. Fixed in `syncWorkspacePanelUI()`: on non-compact (desktop) viewports, `clearBtn.style.display` is set to `none` when no file preview is open, and cleared (shown) when a preview is active. diff --git a/api/routes.py b/api/routes.py index 5e72619..607e49d 100644 --- a/api/routes.py +++ b/api/routes.py @@ -180,6 +180,7 @@ from api.workspace import ( list_dir, read_file_content, safe_resolve_ws, + resolve_trusted_workspace, ) from api.upload import handle_upload, handle_transcribe from api.streaming import _sse, _run_agent_streaming, cancel_stream @@ -638,7 +639,11 @@ def handle_post(handler, parsed) -> bool: body = read_body(handler) if parsed.path == "/api/session/new": - s = new_session(workspace=body.get("workspace"), model=body.get("model")) + try: + workspace = str(resolve_trusted_workspace(body.get("workspace"))) if body.get("workspace") else None + except ValueError as e: + return bad(handler, str(e)) + s = new_session(workspace=workspace, model=body.get("model")) return j(handler, {"session": s.compact() | {"messages": s.messages}}) if parsed.path == "/api/sessions/cleanup": @@ -713,7 +718,10 @@ def handle_post(handler, parsed) -> bool: s = get_session(body["session_id"]) except KeyError: return bad(handler, "Session not found", 404) - new_ws = str(Path(body.get("workspace", s.workspace)).expanduser().resolve()) + try: + new_ws = str(resolve_trusted_workspace(body.get("workspace", s.workspace))) + except ValueError as e: + return bad(handler, str(e)) s.workspace = new_ws s.model = body.get("model", s.model) s.save() @@ -1668,7 +1676,10 @@ def _handle_chat_start(handler, body): if not msg: return bad(handler, "message is required") attachments = [str(a) for a in (body.get("attachments") or [])][:20] - workspace = str(Path(body.get("workspace") or s.workspace).expanduser().resolve()) + try: + workspace = str(resolve_trusted_workspace(body.get("workspace") or s.workspace)) + except ValueError as e: + return bad(handler, str(e)) model = body.get("model") or s.model stream_id = uuid.uuid4().hex s.workspace = workspace @@ -2016,11 +2027,10 @@ def _handle_workspace_add(handler, body): name = body.get("name", "").strip() if not path_str: return bad(handler, "path is required") - p = Path(path_str).expanduser().resolve() - if not p.exists(): - return bad(handler, f"Path does not exist: {p}") - if not p.is_dir(): - return bad(handler, f"Path is not a directory: {p}") + try: + p = resolve_trusted_workspace(path_str) + except ValueError as e: + return bad(handler, str(e)) wss = load_workspaces() if any(w["path"] == str(p) for w in wss): return bad(handler, "Workspace already in list") diff --git a/api/workspace.py b/api/workspace.py index 5edeca8..f537301 100644 --- a/api/workspace.py +++ b/api/workspace.py @@ -214,6 +214,31 @@ def set_last_workspace(path: str) -> None: logger.debug("Failed to set last workspace") +def resolve_trusted_workspace(path: str | Path | None = None) -> Path: + """Resolve and validate a workspace path under the WebUI's trusted workspace root. + + The trusted root is the WebUI boot-time DEFAULT_WORKSPACE (respects + HERMES_WEBUI_STATE_DIR for test isolation). Session creation/update and + workspace-list mutations must stay within that root so callers cannot repoint + a session to arbitrary filesystem locations outside the intended sandbox. + + Note: _profile_default_workspace() reads the agent's terminal.cwd which may + differ from the WebUI's configured workspace root — always use DEFAULT_WORKSPACE + here to stay consistent with how new_session() seeds the initial workspace. + """ + root = Path(_BOOT_DEFAULT_WORKSPACE).expanduser().resolve() + candidate = root if path in (None, "") else Path(path).expanduser().resolve() + if not candidate.exists(): + raise ValueError(f"Path does not exist: {candidate}") + if not candidate.is_dir(): + raise ValueError(f"Path is not a directory: {candidate}") + try: + candidate.relative_to(root) + except ValueError: + raise ValueError(f"Path is outside the trusted workspace root: {candidate}") + return candidate + + def safe_resolve_ws(root: Path, requested: str) -> Path: """Resolve a relative path inside a workspace root, raising ValueError on traversal.""" resolved = (root / requested).resolve() diff --git a/static/index.html b/static/index.html index e1cd6a0..bbe3d00 100644 --- a/static/index.html +++ b/static/index.html @@ -535,7 +535,7 @@
System
Instance version and access controls.
- v0.50.33 + v0.50.34
diff --git a/tests/test_sprint1.py b/tests/test_sprint1.py index 54fe0cc..e746328 100644 --- a/tests/test_sprint1.py +++ b/tests/test_sprint1.py @@ -145,10 +145,13 @@ def test_session_update(): """Create session, update workspace and model, verify persisted.""" data, _ = post("/api/session/new", {}) sid = data["session"]["session_id"] + current_ws = pathlib.Path(data["session"]["workspace"]) + child_ws = current_ws / f"session-update-{uuid.uuid4().hex[:6]}" + child_ws.mkdir(parents=True, exist_ok=True) updated, status = post("/api/session/update", { "session_id": sid, - "workspace": "/tmp", + "workspace": str(child_ws), "model": "anthropic/claude-sonnet-4.6" }) assert status == 200 diff --git a/tests/test_sprint13.py b/tests/test_sprint13.py index 2a90a8c..9c3126a 100644 --- a/tests/test_sprint13.py +++ b/tests/test_sprint13.py @@ -107,14 +107,16 @@ def test_workspace_add_rejects_nonexistent(): assert status == 400 def test_workspace_add_accepts_real_dir(): - """Adding a real directory succeeds.""" - import tempfile - tmp = tempfile.mkdtemp() + """Adding a real directory under the trusted workspace root succeeds.""" + d, _ = post("/api/session/new", {}) + root = pathlib.Path(d["session"]["workspace"]) + tmp = root / "trusted-add-test" + tmp.mkdir(parents=True, exist_ok=True) try: - d, status = post("/api/workspaces/add", {"path": tmp, "name": "test-ws"}) + d, status = post("/api/workspaces/add", {"path": str(tmp), "name": "test-ws"}) assert status == 200 assert d["ok"] is True finally: - post("/api/workspaces/remove", {"path": tmp}) + post("/api/workspaces/remove", {"path": str(tmp)}) import shutil shutil.rmtree(tmp, ignore_errors=True) diff --git a/tests/test_sprint3.py b/tests/test_sprint3.py index 333931d..63275ad 100644 --- a/tests/test_sprint3.py +++ b/tests/test_sprint3.py @@ -145,6 +145,43 @@ def test_session_update_unknown_id_returns_404(): result, status = post("/api/session/update", {"session_id": "nosuchsession", "model": "openai/gpt-5.4-mini"}) assert status == 404 + +def test_session_update_rejects_workspace_outside_trusted_root(tmp_path): + d, _ = post("/api/session/new", {}) + sid = d["session"]["session_id"] + outside = tmp_path / "outside" + outside.mkdir(parents=True, exist_ok=True) + result, status = post("/api/session/update", {"session_id": sid, "workspace": str(outside)}) + assert status == 400 + assert "trusted workspace root" in result.get("error", "").lower() + + +def test_chat_start_rejects_workspace_outside_trusted_root(tmp_path): + d, _ = post("/api/session/new", {}) + sid = d["session"]["session_id"] + outside = tmp_path / "outside-chat" + outside.mkdir(parents=True, exist_ok=True) + result, status = post("/api/chat/start", {"session_id": sid, "message": "hello", "workspace": str(outside)}) + assert status == 400 + assert "trusted workspace root" in result.get("error", "").lower() + + +def test_workspace_add_rejects_path_outside_trusted_root(tmp_path): + outside = tmp_path / "outside-add" + outside.mkdir(parents=True, exist_ok=True) + result, status = post("/api/workspaces/add", {"path": str(outside), "name": "Outside"}) + assert status == 400 + assert "trusted workspace root" in result.get("error", "").lower() + + +def test_session_new_rejects_workspace_outside_trusted_root(tmp_path): + outside = tmp_path / "outside-new" + outside.mkdir(parents=True, exist_ok=True) + result, status = post("/api/session/new", {"workspace": str(outside)}) + assert status == 400 + assert "trusted workspace root" in result.get("error", "").lower() + + def test_session_search_returns_matches(cleanup_test_sessions): sid, _ = make_session_tracked(cleanup_test_sessions) post("/api/session/rename", {"session_id": sid, "title": f"unique-s3-{sid}"}) diff --git a/tests/test_sprint4.py b/tests/test_sprint4.py index 95ebd20..11bbb1b 100644 --- a/tests/test_sprint4.py +++ b/tests/test_sprint4.py @@ -149,8 +149,10 @@ def test_file_requires_path(cleanup_test_sessions): assert e.code == 400 def test_new_session_inherits_workspace(cleanup_test_sessions): - sid, _ = make_session_tracked(cleanup_test_sessions) - post("/api/session/update", {"session_id": sid, "workspace": "/tmp", "model": "openai/gpt-5.4-mini"}) + sid, ws = make_session_tracked(cleanup_test_sessions) + child = ws / f"workspace-inherit-{uuid.uuid4().hex[:6]}" + child.mkdir(parents=True, exist_ok=True) + post("/api/session/update", {"session_id": sid, "workspace": str(child), "model": "openai/gpt-5.4-mini"}) sid2, _ = make_session_tracked(cleanup_test_sessions) data, _ = get(f"/api/session?session_id={sid2}") - assert data["session"]["workspace"] == "/tmp" + assert data["session"]["workspace"] == str(child) diff --git a/tests/test_sprint5.py b/tests/test_sprint5.py index 79063af..c15d578 100644 --- a/tests/test_sprint5.py +++ b/tests/test_sprint5.py @@ -31,6 +31,12 @@ def make_session_tracked(created_list, ws=None): return sid, _pathlib.Path(d["session"]["workspace"]) +def make_workspace_child(base: pathlib.Path, name: str) -> pathlib.Path: + target = base / name + target.mkdir(parents=True, exist_ok=True) + return target + + def test_server_running_from_new_location(): data, status = get("/health") assert status == 200 and data["status"] == "ok" @@ -44,11 +50,13 @@ def test_workspaces_list(): data, status = get("/api/workspaces") assert status == 200 and "workspaces" in data and "last" in data -def test_workspace_add_valid(): - post("/api/workspaces/remove", {"path": "/tmp"}) - result, status = post("/api/workspaces/add", {"path": "/tmp", "name": "Temp"}) - assert status == 200 and any(w["path"]=="/tmp" for w in result["workspaces"]) - post("/api/workspaces/remove", {"path": "/tmp"}) +def test_workspace_add_valid(cleanup_test_sessions): + _, ws = make_session_tracked(cleanup_test_sessions) + child = make_workspace_child(ws, f"workspace-add-{uuid.uuid4().hex[:6]}") + post("/api/workspaces/remove", {"path": str(child)}) + result, status = post("/api/workspaces/add", {"path": str(child), "name": "Temp"}) + assert status == 200 and any(w["path"] == str(child) for w in result["workspaces"]) + post("/api/workspaces/remove", {"path": str(child)}) def test_workspace_add_validates_existence(): result, status = post("/api/workspaces/add", {"path": "/tmp/does_not_exist_xyz_999"}) @@ -58,40 +66,47 @@ def test_workspace_add_validates_is_dir(): result, status = post("/api/workspaces/add", {"path": "/etc/hostname"}) assert status == 400 -def test_workspace_add_no_duplicate(): - post("/api/workspaces/remove", {"path": "/tmp"}) - post("/api/workspaces/add", {"path": "/tmp"}) - result, status = post("/api/workspaces/add", {"path": "/tmp"}) +def test_workspace_add_no_duplicate(cleanup_test_sessions): + _, ws = make_session_tracked(cleanup_test_sessions) + child = make_workspace_child(ws, f"workspace-dup-{uuid.uuid4().hex[:6]}") + post("/api/workspaces/remove", {"path": str(child)}) + post("/api/workspaces/add", {"path": str(child)}) + result, status = post("/api/workspaces/add", {"path": str(child)}) assert status == 400 and "already" in result.get("error","").lower() - post("/api/workspaces/remove", {"path": "/tmp"}) + post("/api/workspaces/remove", {"path": str(child)}) def test_workspace_add_requires_path(): result, status = post("/api/workspaces/add", {}) assert status == 400 -def test_workspace_remove(): - post("/api/workspaces/remove", {"path": "/tmp"}) - post("/api/workspaces/add", {"path": "/tmp", "name": "Temp"}) - result, status = post("/api/workspaces/remove", {"path": "/tmp"}) - assert status == 200 and "/tmp" not in [w["path"] for w in result["workspaces"]] +def test_workspace_remove(cleanup_test_sessions): + _, ws = make_session_tracked(cleanup_test_sessions) + child = make_workspace_child(ws, f"workspace-remove-{uuid.uuid4().hex[:6]}") + post("/api/workspaces/remove", {"path": str(child)}) + post("/api/workspaces/add", {"path": str(child), "name": "Temp"}) + result, status = post("/api/workspaces/remove", {"path": str(child)}) + assert status == 200 and str(child) not in [w["path"] for w in result["workspaces"]] -def test_workspace_rename(): - post("/api/workspaces/remove", {"path": "/tmp"}) - post("/api/workspaces/add", {"path": "/tmp", "name": "Temp"}) - result, status = post("/api/workspaces/rename", {"path": "/tmp", "name": "My Temp"}) +def test_workspace_rename(cleanup_test_sessions): + _, ws = make_session_tracked(cleanup_test_sessions) + child = make_workspace_child(ws, f"workspace-rename-{uuid.uuid4().hex[:6]}") + post("/api/workspaces/remove", {"path": str(child)}) + post("/api/workspaces/add", {"path": str(child), "name": "Temp"}) + result, status = post("/api/workspaces/rename", {"path": str(child), "name": "My Temp"}) assert status == 200 - assert {w["path"]: w["name"] for w in result["workspaces"]}.get("/tmp") == "My Temp" - post("/api/workspaces/remove", {"path": "/tmp"}) + assert {w["path"]: w["name"] for w in result["workspaces"]}.get(str(child)) == "My Temp" + post("/api/workspaces/remove", {"path": str(child)}) def test_workspace_rename_unknown(): result, status = post("/api/workspaces/rename", {"path": "/no/such/path", "name": "X"}) assert status == 404 def test_last_workspace_updates_on_session_update(cleanup_test_sessions): - sid, _ = make_session_tracked(cleanup_test_sessions) - post("/api/session/update", {"session_id": sid, "workspace": "/tmp", "model": "openai/gpt-5.4-mini"}) + sid, ws = make_session_tracked(cleanup_test_sessions) + child = make_workspace_child(ws, f"workspace-last-{uuid.uuid4().hex[:6]}") + post("/api/session/update", {"session_id": sid, "workspace": str(child), "model": "openai/gpt-5.4-mini"}) data, _ = get("/api/workspaces") - assert data["last"] == "/tmp" + assert data["last"] == str(child) def test_file_save(cleanup_test_sessions): sid, ws = make_session_tracked(cleanup_test_sessions) @@ -133,8 +148,9 @@ def test_sessions_endpoint_returns_sorted(): assert sessions[0]["updated_at"] >= sessions[1]["updated_at"] def test_new_session_inherits_last_workspace(cleanup_test_sessions): - sid, _ = make_session_tracked(cleanup_test_sessions) - post("/api/session/update", {"session_id": sid, "workspace": "/tmp", "model": "openai/gpt-5.4-mini"}) + sid, ws = make_session_tracked(cleanup_test_sessions) + child = make_workspace_child(ws, f"workspace-inherit-{uuid.uuid4().hex[:6]}") + post("/api/session/update", {"session_id": sid, "workspace": str(child), "model": "openai/gpt-5.4-mini"}) sid2, _ = make_session_tracked(cleanup_test_sessions) d, _ = get(f"/api/session?session_id={sid2}") - assert d["session"]["workspace"] == "/tmp" + assert d["session"]["workspace"] == str(child)