[security] fix(workspace): restrict session workspaces to trusted roots (#416)

* fix(workspace): restrict session workspaces to trusted roots * fix: use boot-time DEFAULT_WORKSPACE instead of profile default for trusted workspace root _profile_default_workspace() reads the agent's terminal.cwd which may differ from the WebUI's configured workspace root. Use _BOOT_DEFAULT_WORKSPACE (which respects HERMES_WEBUI_DEFAULT_WORKSPACE for test isolation) to stay consistent with how new_session() seeds the initial workspace. * docs: v0.50.34 release — version badge and CHANGELOG --------- Co-authored-by: hinotoi-agent <paperlantern.agent@gmail.com> Co-authored-by: Nathan Esquenazi <nesquena@gmail.com>
2026-04-13 23:44:03 -07:00
parent a5abe51cc5
commit 2a7a5ddfaf
9 changed files with 152 additions and 45 deletions
--- 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.
--- 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")
--- 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()
--- a/static/index.html
+++ b/static/index.html
@@ -535,7 +535,7 @@
                <div class="settings-section-title">System</div>
                <div class="settings-section-meta">Instance version and access controls.</div>
              </div>
-              <span class="settings-version-badge">v0.50.33</span>
+              <span class="settings-version-badge">v0.50.34</span>
            </div>
            <div class="settings-field" style="border-top:1px solid var(--border);padding-top:12px;margin-top:8px">
              <label for="settingsPassword" data-i18n="settings_label_password">Access Password</label>
--- 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
--- 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)
--- 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}"})
--- 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)
--- 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)