diff --git a/CLAUDE.md b/CLAUDE.md index 6c96cedc..f3a2eae3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -267,6 +267,16 @@ ANTHROPIC_API_KEY=sk-ant-... # Required for Anthropic provider (default E2B_API_KEY=e2b_... # Required for --engine cloud DATABASE_PATH=./codeframe.db # Optional +# Server deployment + workspace allowlist (#655) +CODEFRAME_DEPLOYMENT_MODE=self_hosted # self_hosted (default) or hosted (multi-tenant) +WORKSPACE_ROOT=/srv/workspaces # os.pathsep-separated allowlist of permitted + # workspace roots. Any client-supplied workspace + # path (REST, workspace init, session create) must + # resolve inside a root, else 403. Unset = no + # allowlist (single-operator self-hosted default). + # In hosted mode it is MANDATORY (fail closed) and + # each user is confined to /. + # LLM Provider selection (multi-provider support) # Priority: CLI flag > env var > .codeframe/config.yaml > default (anthropic) CODEFRAME_LLM_PROVIDER=anthropic # Provider: anthropic (default), openai, ollama, vllm, compatible diff --git a/codeframe/platform_store/repositories/interactive_sessions.py b/codeframe/platform_store/repositories/interactive_sessions.py index 2f80b38f..5ebb4c92 100644 --- a/codeframe/platform_store/repositories/interactive_sessions.py +++ b/codeframe/platform_store/repositories/interactive_sessions.py @@ -23,6 +23,7 @@ def create( task_id: Optional[str] = None, agent_type: str = "claude", model: Optional[str] = None, + user_id: Optional[int] = None, ) -> dict: now = datetime.now(UTC).isoformat() session_id = str(uuid.uuid4()) @@ -30,10 +31,11 @@ def create( """ INSERT INTO interactive_sessions (id, workspace_path, task_id, state, agent_type, model, - cost_usd, input_tokens, output_tokens, created_at, updated_at, ended_at) - VALUES (?, ?, ?, 'active', ?, ?, 0.0, 0, 0, ?, ?, NULL) + cost_usd, input_tokens, output_tokens, created_at, updated_at, + ended_at, user_id) + VALUES (?, ?, ?, 'active', ?, ?, 0.0, 0, 0, ?, ?, NULL, ?) """, - (session_id, workspace_path, task_id, agent_type, model, now, now), + (session_id, workspace_path, task_id, agent_type, model, now, now, user_id), ) self._commit() return self.get(session_id) diff --git a/codeframe/platform_store/schema_manager.py b/codeframe/platform_store/schema_manager.py index 10aa8d45..18eef2ac 100644 --- a/codeframe/platform_store/schema_manager.py +++ b/codeframe/platform_store/schema_manager.py @@ -179,10 +179,23 @@ def _create_interactive_session_tables(self, cursor: sqlite3.Cursor) -> None: output_tokens INTEGER DEFAULT 0, created_at TEXT NOT NULL, updated_at TEXT NOT NULL, - ended_at TEXT + ended_at TEXT, + -- Owning user; the terminal/chat WebSockets reject a session + -- whose owner != the authenticated user (issue #655). NULL in + -- no-auth mode, where ownership is intentionally not enforced. + user_id INTEGER REFERENCES users(id) ON DELETE SET NULL ) """ ) + # Migrate pre-#655 databases that created the table without user_id. + existing_cols = { + row[1] for row in cursor.execute("PRAGMA table_info(interactive_sessions)") + } + if "user_id" not in existing_cols: + cursor.execute( + "ALTER TABLE interactive_sessions ADD COLUMN user_id INTEGER " + "REFERENCES users(id) ON DELETE SET NULL" + ) cursor.execute( """ diff --git a/codeframe/ui/dependencies.py b/codeframe/ui/dependencies.py index 8d69bc26..c002293a 100644 --- a/codeframe/ui/dependencies.py +++ b/codeframe/ui/dependencies.py @@ -6,17 +6,79 @@ v2-only: All dependencies use codeframe.core modules. """ +import os from pathlib import Path -from typing import Optional +from typing import Any, Dict, Optional -from fastapi import HTTPException, Query, Request +from fastapi import Depends, HTTPException, Query, Request +from codeframe.auth.dependencies import require_auth from codeframe.workspace import WorkspaceManager # v2 imports from codeframe.core.workspace import Workspace, get_workspace, workspace_exists +def _allowed_workspace_roots() -> list[Path]: + """Permitted workspace roots from ``WORKSPACE_ROOT`` (os.pathsep-separated). + + Empty when unset — meaning "no allowlist" (single-operator self-hosted, the + default). Each root is resolved so containment checks defeat ``..`` escapes. + """ + raw = os.getenv("WORKSPACE_ROOT", "").strip() + if not raw: + return [] + return [ + Path(p).expanduser().resolve() + for p in raw.split(os.pathsep) + if p.strip() + ] + + +def _within_any_root(path: Path, roots: list[Path]) -> bool: + return any(path == r or path.is_relative_to(r) for r in roots) + + +def enforce_workspace_allowlist(path: Path, user_id: Optional[int]) -> Path: + """Validate a resolved workspace path against the allowlist (issue #655). + + Shared by every entry point that resolves a client-supplied workspace path: + ``get_v2_workspace`` (REST) and interactive session creation (whose stored + path later becomes a terminal shell's ``cwd``). Without it, an authenticated + user can point operations at any host directory — authenticated + cross-tenant RCE once the server serves >1 user. + + Returns the (resolved) path on success; raises ``HTTPException`` otherwise. + """ + # Local import avoids a circular import (server -> routers -> dependencies). + from codeframe.ui.server import is_hosted_mode + + path = path.resolve() + roots = _allowed_workspace_roots() + if is_hosted_mode(): + # Hosted/multi-tenant: the allowlist is mandatory (fail closed) and each + # user is confined to / so one tenant can't reach + # another's subtree. + # ponytail: path-namespace binding, not a DB ownership table. Upgrade to + # registry-backed owner_user_id checks if workspaces ever live outside a + # per-user root. + if not roots: + raise HTTPException( + status_code=500, + detail="Server misconfigured: WORKSPACE_ROOT must be set in hosted mode.", + ) + if user_id is None: + raise HTTPException(status_code=403, detail="Authenticated user required.") + roots = [r / str(user_id) for r in roots] + + if roots and not _within_any_root(path, roots): + raise HTTPException( + status_code=403, + detail="Workspace path is outside the permitted workspace roots.", + ) + return path + + def get_workspace_manager(request: Request) -> WorkspaceManager: """Get workspace manager from application state. @@ -41,6 +103,7 @@ def get_v2_workspace( description="Path to workspace directory (defaults to server's working directory)", ), request: Request = None, + auth: Dict[str, Any] = Depends(require_auth), ) -> Workspace: """Get v2 Workspace from path or server default. @@ -76,6 +139,9 @@ async def endpoint(workspace: Workspace = Depends(get_v2_workspace)): # Fall back to current working directory path = Path.cwd() + # Enforce the workspace allowlist (issue #655). + path = enforce_workspace_allowlist(path, auth.get("user_id")) + # Validate workspace exists # Note: Avoid exposing full filesystem paths in error messages for hosted deployments if not workspace_exists(path): @@ -100,4 +166,5 @@ async def endpoint(workspace: Workspace = Depends(get_v2_workspace)): __all__ = [ "get_workspace_manager", "get_v2_workspace", + "enforce_workspace_allowlist", ] diff --git a/codeframe/ui/routers/interactive_sessions_v2.py b/codeframe/ui/routers/interactive_sessions_v2.py index 4e5fec8b..c11a2f4f 100644 --- a/codeframe/ui/routers/interactive_sessions_v2.py +++ b/codeframe/ui/routers/interactive_sessions_v2.py @@ -10,17 +10,21 @@ POST /api/v2/sessions/{id}/messages - Add message to session GET /api/v2/sessions/{id}/messages - Get message history (?limit=&offset=) -Auth note: auth is a project-wide gap tracked in #336. All other v2 routers also -lack auth — adding it only here would be inconsistent with the rest of the API. +Auth: enforced project-wide via router-level ``require_auth`` (#336). Session +creation additionally validates ``workspace_path`` against the workspace +allowlist (#655) since the stored path becomes a terminal shell ``cwd``. """ import logging -from typing import Optional +from pathlib import Path +from typing import Any, Dict, Optional -from fastapi import APIRouter, HTTPException, Query, Request +from fastapi import APIRouter, Depends, HTTPException, Query, Request from pydantic import BaseModel, field_validator +from codeframe.auth.dependencies import require_auth from codeframe.lib.rate_limiter import rate_limit_standard +from codeframe.ui.dependencies import enforce_workspace_allowlist logger = logging.getLogger(__name__) @@ -136,14 +140,27 @@ def _session_to_response(row: dict) -> SessionResponse: @router.post("", status_code=201, response_model=SessionResponse) @rate_limit_standard() -def create_session(body: SessionCreate, request: Request): - """Create a new interactive agent session.""" +def create_session( + body: SessionCreate, + request: Request, + auth: Dict[str, Any] = Depends(require_auth), +): + """Create a new interactive agent session. + + The stored ``workspace_path`` later becomes a terminal shell's ``cwd`` + (terminal_ws / session_chat_ws), so it must clear the same allowlist as + REST workspace access (issue #655) — validated and resolved here. + """ repo = _get_repo(request) + workspace_path = enforce_workspace_allowlist( + Path(body.workspace_path), auth.get("user_id") + ) session = repo.create( - workspace_path=body.workspace_path, + workspace_path=str(workspace_path), task_id=body.task_id, agent_type=body.agent_type, model=body.model, + user_id=auth.get("user_id"), ) return _session_to_response(session) diff --git a/codeframe/ui/routers/session_chat_ws.py b/codeframe/ui/routers/session_chat_ws.py index 5a4aa758..6c0473c2 100644 --- a/codeframe/ui/routers/session_chat_ws.py +++ b/codeframe/ui/routers/session_chat_ws.py @@ -108,7 +108,7 @@ async def _run_streaming_adapter( async def session_chat_ws(session_id: str, websocket: WebSocket) -> None: """Bidirectional WebSocket for streaming agent chat on an interactive session.""" # --- Auth --- - authenticated, _user_id = await _authenticate_websocket(websocket) + authenticated, user_id = await _authenticate_websocket(websocket) if not authenticated: return @@ -123,6 +123,19 @@ async def session_chat_ws(session_id: str, websocket: WebSocket) -> None: await websocket.close(code=4008, reason="Session not found or ended") return + # --- Ownership check (issue #655) --- + # The chat agent runs file-system tools scoped to the session's workspace, so + # one authenticated user must not drive another user's session. Skipped in + # no-auth mode (user_id is None), matching terminal_ws / REST behavior. + # Fail closed: when auth is enabled (user_id is not None), an ownerless + # (NULL, e.g. pre-#655 migrated) or mismatched session is rejected. + session_user_id = session.get("user_id") + if user_id is not None and ( + session_user_id is None or int(session_user_id) != user_id + ): + await websocket.close(code=1008, reason="Forbidden: session belongs to another user") + return + # --- Accept connection; everything after this point must run inside the # try/finally so unregister() and close() always execute even if # update_state, register, or get_token_queue raises. --- diff --git a/codeframe/ui/routers/terminal_ws.py b/codeframe/ui/routers/terminal_ws.py index d59eab1c..8b40f30e 100644 --- a/codeframe/ui/routers/terminal_ws.py +++ b/codeframe/ui/routers/terminal_ws.py @@ -76,9 +76,13 @@ async def session_terminal_ws(session_id: str, websocket: WebSocket) -> None: # --- Ownership check --- # Skipped in no-auth mode (user_id is None): there is no identity to enforce, - # matching REST behavior under CODEFRAME_AUTH_REQUIRED=false. + # matching REST behavior under CODEFRAME_AUTH_REQUIRED=false. When auth IS + # enabled, fail closed — an ownerless (NULL, e.g. pre-#655 migrated) or + # mismatched session is rejected. session_user_id = session.get("user_id") - if user_id is not None and session_user_id is not None and int(session_user_id) != user_id: + if user_id is not None and ( + session_user_id is None or int(session_user_id) != user_id + ): await websocket.close(code=4003, reason="Forbidden: session belongs to another user") return diff --git a/codeframe/ui/routers/workspace_v2.py b/codeframe/ui/routers/workspace_v2.py index 535db3c5..986cdc41 100644 --- a/codeframe/ui/routers/workspace_v2.py +++ b/codeframe/ui/routers/workspace_v2.py @@ -19,7 +19,8 @@ from codeframe.core import workspace as ws from codeframe.lib.rate_limiter import rate_limit_standard -from codeframe.ui.dependencies import get_v2_workspace +from codeframe.auth.dependencies import require_auth +from codeframe.ui.dependencies import enforce_workspace_allowlist, get_v2_workspace from codeframe.core.workspace import WORKSPACE_CONFIG_FILENAME, Workspace from codeframe.ui.response_models import api_error, ErrorCodes from codeframe.ui.routers._helpers import atomic_write_json @@ -225,6 +226,7 @@ async def list_workspaces(request: Request) -> WorkspaceListResponse: async def init_workspace( request: Request, body: InitWorkspaceRequest, + auth: dict = Depends(require_auth), ) -> WorkspaceResponse: """Initialize a new workspace for a repository. @@ -244,7 +246,13 @@ async def init_workspace( - 404: Repository path not found """ try: - repo_path = Path(body.repo_path).resolve() + # Enforce the workspace allowlist before touching disk (issue #655): + # initializing a workspace writes a .codeframe/ marker, so an arbitrary + # repo_path would let an authenticated user seed workspaces anywhere on + # the host. + repo_path = enforce_workspace_allowlist( + Path(body.repo_path), auth.get("user_id") + ) # Validate path exists if not repo_path.exists(): diff --git a/tests/api/test_session_chat_ws.py b/tests/api/test_session_chat_ws.py index f1260da8..97aff545 100644 --- a/tests/api/test_session_chat_ws.py +++ b/tests/api/test_session_chat_ws.py @@ -9,6 +9,8 @@ and explicit teardown within tests where needed. """ +import os + import pytest from unittest.mock import patch from fastapi.testclient import TestClient @@ -25,8 +27,14 @@ # --------------------------------------------------------------------------- -def _create_session(client: TestClient, workspace_path: str = "/tmp/ws-test") -> str: - """Create an interactive session and return its ID.""" +def _create_session(client: TestClient, workspace_path: str | None = None) -> str: + """Create an interactive session and return its ID. + + Defaults to a path under the test ``WORKSPACE_ROOT`` (set by the api + conftest) so it clears the workspace allowlist (#655). + """ + if workspace_path is None: + workspace_path = os.path.join(os.environ.get("WORKSPACE_ROOT", "/tmp"), "ws-test") resp = client.post( "/api/v2/sessions", json={"workspace_path": workspace_path, "agent_type": "claude"}, @@ -39,6 +47,36 @@ def _ws_url(session_id: str, token: str) -> str: return f"/ws/sessions/{session_id}/chat?token={token}" +def test_chat_ws_ownership_mismatch_closes(): + """A session owned by another user is refused (issue #655).""" + from unittest.mock import AsyncMock, MagicMock, patch + + from fastapi import FastAPI + from fastapi.testclient import TestClient + + from codeframe.ui.routers.session_chat_ws import router + + app = FastAPI() + app.include_router(router) + fake_db = MagicMock() + fake_db.interactive_sessions.get.return_value = { + "state": "active", + "workspace_path": "/tmp", + "user_id": 999, + } + app.state.db = fake_db + client = TestClient(app) + + with patch( + "codeframe.ui.routers.session_chat_ws._authenticate_websocket", + new=AsyncMock(return_value=(True, 1)), + ): + with pytest.raises(WebSocketDisconnect) as exc: + with client.websocket_connect("/ws/sessions/s1/chat?token=x"): + pass + assert exc.value.code == 1008 + + # --------------------------------------------------------------------------- # Auth tests # --------------------------------------------------------------------------- diff --git a/tests/ui/test_workspace_allowlist.py b/tests/ui/test_workspace_allowlist.py new file mode 100644 index 00000000..c252c873 --- /dev/null +++ b/tests/ui/test_workspace_allowlist.py @@ -0,0 +1,196 @@ +"""Tests for workspace path allowlist (issue #655). + +`get_v2_workspace` previously resolved an arbitrary client-supplied +`workspace_path` and only checked the `.codeframe` marker — authenticated +cross-tenant RCE the moment the server is exposed to >1 user. These tests +pin the allowlist + HOSTED-mode per-user binding. +""" + +import shutil +from types import SimpleNamespace + +import pytest +from fastapi import HTTPException + +from codeframe.core.workspace import create_or_load_workspace +from codeframe.ui.dependencies import get_v2_workspace + +pytestmark = pytest.mark.v2 + + +@pytest.fixture +def root(tmp_path): + """A permitted root dir containing one initialized workspace at /proj.""" + proj = tmp_path / "proj" + proj.mkdir(parents=True, exist_ok=True) + ws = create_or_load_workspace(proj) + yield tmp_path, ws + shutil.rmtree(tmp_path, ignore_errors=True) + + +def _request(): + """Minimal Request stand-in: only app.state is read.""" + return SimpleNamespace(app=SimpleNamespace(state=SimpleNamespace())) + + +def _call(path, *, auth=None): + return get_v2_workspace( + workspace_path=str(path), + request=_request(), + auth=auth if auth is not None else {"user_id": None}, + ) + + +def test_no_root_set_allows_any_path(root, monkeypatch): + """Self-hosted default (no WORKSPACE_ROOT): behavior unchanged.""" + monkeypatch.delenv("WORKSPACE_ROOT", raising=False) + monkeypatch.setenv("CODEFRAME_DEPLOYMENT_MODE", "self_hosted") + _, ws = root + assert _call(ws.repo_path).repo_path == ws.repo_path + + +def test_path_inside_root_allowed(root, monkeypatch): + base, ws = root + monkeypatch.setenv("WORKSPACE_ROOT", str(base)) + monkeypatch.setenv("CODEFRAME_DEPLOYMENT_MODE", "self_hosted") + assert _call(ws.repo_path).repo_path == ws.repo_path + + +def test_path_outside_root_rejected(root, monkeypatch): + base, ws = root + # Allow a sibling dir, not the one the workspace lives in. + monkeypatch.setenv("WORKSPACE_ROOT", str(base / "other")) + monkeypatch.setenv("CODEFRAME_DEPLOYMENT_MODE", "self_hosted") + with pytest.raises(HTTPException) as exc: + _call(ws.repo_path) + assert exc.value.status_code == 403 + + +def test_traversal_escape_rejected(root, monkeypatch): + base, ws = root + monkeypatch.setenv("WORKSPACE_ROOT", str(base)) + monkeypatch.setenv("CODEFRAME_DEPLOYMENT_MODE", "self_hosted") + # ../ escapes the root even though the literal string starts under it. + with pytest.raises(HTTPException) as exc: + _call(f"{base}/../{ws.repo_path.name}") + assert exc.value.status_code == 403 + + +def test_hosted_requires_root(root, monkeypatch): + _, ws = root + monkeypatch.delenv("WORKSPACE_ROOT", raising=False) + monkeypatch.setenv("CODEFRAME_DEPLOYMENT_MODE", "hosted") + with pytest.raises(HTTPException) as exc: + _call(ws.repo_path, auth={"user_id": 1}) + assert exc.value.status_code == 500 + + +def test_hosted_confines_to_user_subdir(monkeypatch, tmp_path): + monkeypatch.setenv("WORKSPACE_ROOT", str(tmp_path)) + monkeypatch.setenv("CODEFRAME_DEPLOYMENT_MODE", "hosted") + # User 1 owns /1/proj; user 2 must not reach it. + proj = tmp_path / "1" / "proj" + proj.mkdir(parents=True, exist_ok=True) + ws1 = create_or_load_workspace(proj) + assert _call(ws1.repo_path, auth={"user_id": 1}).repo_path == ws1.repo_path + with pytest.raises(HTTPException) as exc: + _call(ws1.repo_path, auth={"user_id": 2}) + assert exc.value.status_code == 403 + shutil.rmtree(tmp_path, ignore_errors=True) + + +def test_hosted_requires_authenticated_user(root, monkeypatch): + base, ws = root + monkeypatch.setenv("WORKSPACE_ROOT", str(base)) + monkeypatch.setenv("CODEFRAME_DEPLOYMENT_MODE", "hosted") + with pytest.raises(HTTPException) as exc: + _call(ws.repo_path, auth={"user_id": None}) + assert exc.value.status_code == 403 + + +# --- Session creation must clear the same allowlist (codex P1 / #655) --------- +# create_session stores workspace_path, which terminal_ws later uses as a shell +# cwd — a bypass of get_v2_workspace's check unless validated here too. + + +@pytest.fixture +def sessions_client(): + from fastapi import FastAPI + from fastapi.testclient import TestClient + + from codeframe.platform_store.database import Database + from codeframe.ui.routers.interactive_sessions_v2 import router + + app = FastAPI() + app.include_router(router) + db = Database(":memory:") + db.initialize() + app.state.db = db + with TestClient(app, raise_server_exceptions=True) as c: + yield c + + +def test_session_create_rejects_path_outside_root(sessions_client, tmp_path, monkeypatch): + monkeypatch.setenv("WORKSPACE_ROOT", str(tmp_path / "allowed")) + monkeypatch.setenv("CODEFRAME_DEPLOYMENT_MODE", "self_hosted") + resp = sessions_client.post( + "/api/v2/sessions", json={"workspace_path": str(tmp_path / "elsewhere")} + ) + assert resp.status_code == 403 + + +def test_session_create_stores_resolved_path_inside_root(sessions_client, tmp_path, monkeypatch): + base = tmp_path / "allowed" + base.mkdir() + monkeypatch.setenv("WORKSPACE_ROOT", str(base)) + monkeypatch.setenv("CODEFRAME_DEPLOYMENT_MODE", "self_hosted") + resp = sessions_client.post( + "/api/v2/sessions", json={"workspace_path": f"{base}/../allowed/proj"} + ) + assert resp.status_code == 201 + # Stored path is resolved (traversal collapsed), so the terminal cwd is exactly + # the validated path. + assert resp.json()["workspace_path"] == str(base / "proj") + + +def test_session_create_persists_owner_user_id(sessions_client, tmp_path, monkeypatch): + # Auth is off in tests → require_auth yields user_id=None; assert the column + # is written (None here) so the terminal/chat ownership check has data to act + # on once auth is enabled. + monkeypatch.delenv("WORKSPACE_ROOT", raising=False) + monkeypatch.setenv("CODEFRAME_DEPLOYMENT_MODE", "self_hosted") + resp = sessions_client.post( + "/api/v2/sessions", json={"workspace_path": str(tmp_path)} + ) + assert resp.status_code == 201 + row = sessions_client.app.state.db.interactive_sessions.get(resp.json()["id"]) + assert "user_id" in row + + +# --- Workspace initialization must clear the allowlist too (codex P1 / #655) -- + + +@pytest.fixture +def workspace_init_client(): + from fastapi import FastAPI + from fastapi.testclient import TestClient + + from codeframe.ui.routers.workspace_v2 import router + + app = FastAPI() + app.include_router(router) + with TestClient(app, raise_server_exceptions=True) as c: + yield c + + +def test_workspace_init_rejects_path_outside_root(workspace_init_client, tmp_path, monkeypatch): + base = tmp_path / "allowed" + base.mkdir() + outside = tmp_path / "elsewhere" + outside.mkdir() + monkeypatch.setenv("WORKSPACE_ROOT", str(base)) + monkeypatch.setenv("CODEFRAME_DEPLOYMENT_MODE", "self_hosted") + resp = workspace_init_client.post( + "/api/v2/workspaces", json={"repo_path": str(outside)} + ) + assert resp.status_code == 403