diff --git a/docs/profiles.md b/docs/profiles.md index d80d966..508b259 100644 --- a/docs/profiles.md +++ b/docs/profiles.md @@ -266,6 +266,17 @@ link to any readable file, `.env` included) and the working directory of `terminal` and `code_exec`. Both branches now end in `is_within`; a new call site must go through `resolve_in_areas`, never through its own `Path.resolve()`. +- **`navi-3d` resolves its session paths itself, and now stays inside them (2026-10-12).** + The 3D server runs as a separate process, so `resolve_in_areas` never sees it: whatever + `Settings.resolve_path` (`mcp-servers/navi-3d/app/config.py`) accepted, the tool opened, + created or overwrote. It took `session_id` as an unvalidated path segment — + `"../../../../.."` resolved to `/`, after which every relative path was "inside" — and it + left `..` in a relative path unresolved before the containment check (`is_relative_to` is + lexical, so `..//x.scad` passed and only the OS collapsed it at open time). + `designer_3d` grants `navi-3d` to the agent *and* to its subagents, so this was host-wide + read and write for an ordinary user — `compile_scad` even creates the output's parent + directories. The id is now validated as a single segment and both branches resolve before + checking; any other server that takes a caller-supplied `session_id` must do the same. - **`content_publish` path traversal is closed — keep it closed.** `filename` and `source_filename` are cut to their basename (`content_publish.py:70`, `content_store.py:63`) and checked to exist inside the session directory, so diff --git a/mcp-servers/navi-3d/app/config.py b/mcp-servers/navi-3d/app/config.py index 04f7a89..e818c76 100644 --- a/mcp-servers/navi-3d/app/config.py +++ b/mcp-servers/navi-3d/app/config.py @@ -7,8 +7,14 @@ from __future__ import annotations import os +import re from pathlib import Path +# A session id becomes one path segment under ``session_files/``. Real ids are +# uuid4 strings (``navi/core/session.py``), so this admits every one of them and +# nothing that could walk out of the directory. +_SESSION_ID_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._ -]{0,127}$") + class Settings: """Lightweight env-based settings.""" @@ -27,6 +33,19 @@ return [] return [Path(p.strip()).resolve() for p in raw.split(",") if p.strip()] + def session_dir(self, session_id: str) -> Path: + """The session directory for *session_id*, or ``ValueError``. + + ``session_id`` arrives as a tool argument, so it is caller-controlled + and may not be trusted to be an id: it is joined onto + ``session_files/`` as a path segment, and ``../..`` in it would resolve + to the whole filesystem before any containment check could see it. + """ + sid = session_id.strip() + if ".." in sid or not _SESSION_ID_RE.match(sid): + raise ValueError(f"Invalid session_id: {session_id!r}") + return (self.session_files_dir / sid).resolve() + def resolve_path(self, session_id: str, raw_path: str) -> Path: """Resolve a path that may be relative to the session directory. @@ -37,10 +56,15 @@ Raises ``ValueError`` on anti-escape or disallowed absolute path. """ path = Path(raw_path).expanduser() + session_dir = self.session_dir(session_id) if not path.is_absolute(): - session_dir = (self.session_files_dir / session_id).resolve() - path = session_dir / path + # Resolve before the containment check below. A lexically inside + # path can still point outside — ``..//x.scad`` + # passes ``is_relative_to`` with the ``..`` intact and only the OS + # collapses it at open time — and resolving is also what catches a + # symlink leading out of the session directory. + path = (session_dir / path).resolve() else: path = path.resolve() if self.fs_allowed_paths: @@ -54,7 +78,6 @@ # Anti-escape: the resolved path must still be inside the session dir # or inside an allowed root. - session_dir = (self.session_files_dir / session_id).resolve() if not (path == session_dir or path.is_relative_to(session_dir)): if self.fs_allowed_paths: if not any( diff --git a/tests/unit/mcp/test_navi_3d_paths.py b/tests/unit/mcp/test_navi_3d_paths.py new file mode 100644 index 0000000..3ea4ad8 --- /dev/null +++ b/tests/unit/mcp/test_navi_3d_paths.py @@ -0,0 +1,107 @@ +"""The 3D MCP server must refuse every path outside the caller's session dir. + +``navi-3d`` runs as its own process and resolves paths itself, so the sandbox in +``navi/tools/_internal/areas.py`` never applies to it: whatever ``Settings`` +accepts, the tool opens, creates or overwrites. A regular user reaches it +through the ``designer_3d`` profile, where the server is granted to both the +agent and its subagents — so a ``session_id`` or ``source_path`` that walks out +of ``session_files/`` is host-wide read and write for an ordinary user. + +These tests live in the root suite rather than in ``mcp-servers/navi-3d/tests/`` +because ``pyproject.toml`` collects ``testpaths = ["tests"]`` only. +""" + +import importlib.util +from pathlib import Path + +import pytest + +_SERVER_CONFIG = ( + Path(__file__).resolve().parents[3] + / "mcp-servers" + / "navi-3d" + / "app" + / "config.py" +) + + +@pytest.fixture +def settings(tmp_path, monkeypatch): + """The server's ``Settings``, rooted in a temporary ``session_files``.""" + monkeypatch.setenv("SESSION_FILES_DIR", str(tmp_path / "session_files")) + # The shipped config sets no FS_ALLOWED_PATHS, so the default is what runs. + monkeypatch.delenv("FS_ALLOWED_PATHS", raising=False) + + spec = importlib.util.spec_from_file_location("navi_3d_config_under_test", _SERVER_CONFIG) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + + settings = module.Settings() + settings.sessions = tmp_path / "session_files" + return settings + + +class TestSessionId: + """A session id is one path segment; it may not be a way out.""" + + def test_honest_id_resolves_inside_session_files(self, settings): + assert settings.resolve_path("sess-1", "model.scad") == ( + settings.sessions / "sess-1" / "model.scad" + ) + + def test_relative_traversal_is_refused(self, settings): + with pytest.raises(ValueError): + settings.resolve_path("../..", "navi-1/.env") + + def test_traversal_to_the_filesystem_root_is_refused(self, settings): + # Enough ``..`` reaches ``/``, after which any relative path is inside. + with pytest.raises(ValueError): + settings.resolve_path("../../../../..", "etc/passwd") + + def test_separator_is_refused(self, settings): + with pytest.raises(ValueError): + settings.resolve_path("sess/1", "model.scad") + + def test_empty_id_is_refused(self, settings): + with pytest.raises(ValueError): + settings.resolve_path("", "model.scad") + + +class TestPathResolution: + """Paths resolve inside the session dir and leave it only by absolute path.""" + + def test_relative_path_comes_back_resolved(self, settings): + resolved = settings.resolve_path("sess-1", "model.scad") + + assert ".." not in resolved.parts + assert resolved == (settings.sessions / "sess-1" / "model.scad") + + def test_relative_traversal_is_refused(self, settings): + with pytest.raises(ValueError): + settings.resolve_path("sess-1", "../sess-2/model.scad") + + def test_cross_session_path_is_refused(self, settings): + """``is_relative_to`` is lexical, so the ``..`` must be resolved first.""" + (settings.sessions / "sess-2").mkdir(parents=True) + + with pytest.raises(ValueError): + settings.resolve_path("sess-1", "../sess-2/leaked.scad") + + def test_symlink_out_of_the_session_is_refused(self, settings, tmp_path): + outside = tmp_path / "outside.scad" + outside.write_text("cube([1,1,1]);\n") + session = settings.sessions / "sess-1" + session.mkdir(parents=True) + (session / "link.scad").symlink_to(outside) + + with pytest.raises(ValueError): + settings.resolve_path("sess-1", "link.scad") + + def test_absolute_path_outside_is_refused(self, settings): + with pytest.raises(ValueError): + settings.resolve_path("sess-1", "/etc/passwd") + + def test_absolute_path_inside_the_session_is_allowed(self, settings): + inside = settings.sessions / "sess-1" / "model.stl" + + assert settings.resolve_path("sess-1", str(inside)) == inside