From 702fc1f6d9aa4fd19bb8d540ec955d866ff5e885 Mon Sep 17 00:00:00 2001 From: YueGuobin Date: Tue, 25 Aug 2026 13:17:37 +0800 Subject: [PATCH] fix: never allow the projects directory to become a project directory Loading a .gns3 placed directly in the projects root registered the shared projects directory as the project path (load_project derives the path from the file's parent directory). Deleting such an entry ran rmtree on the projects directory itself, wiping every project until a root-owned file stopped it, and left a zombie entry in the controller. Three layers of protection: - Controller.load_project() refuses a .gns3 whose parent directory is the projects directory; the normal subdirectory layout is unaffected - the Project.path setter rejects the projects directory itself and its ancestors, closing the same hole for POST/PUT with an explicit path - Project.delete() uses realpath + commonpath instead of commonprefix: entries whose path is the projects root are refused, and sibling directories sharing a string prefix (/srv/projects-evil vs /srv/projects) are no longer treated as inside the projects dir Also removes the project_load MCP tool: loading by raw server filesystem path is a footgun for automated clients; projects can still be opened by project_id via the remaining tools. --- docs/features/mcp-service.md | 3 +- gns3server/agent/mcp/__init__.py | 12 +---- gns3server/agent/mcp/projects.py | 9 ---- gns3server/controller/__init__.py | 11 +++++ gns3server/controller/project.py | 24 +++++++++- tests/agent/mcp/test_tool_params.py | 1 - tests/api/routes/controller/test_projects.py | 5 ++ tests/controller/test_controller.py | 48 ++++++++++++++++++++ tests/controller/test_import_project.py | 3 +- tests/controller/test_project.py | 31 +++++++++++++ tests/controller/test_project_open.py | 6 ++- 11 files changed, 125 insertions(+), 28 deletions(-) diff --git a/docs/features/mcp-service.md b/docs/features/mcp-service.md index e1f45fc3e..3115f1bab 100644 --- a/docs/features/mcp-service.md +++ b/docs/features/mcp-service.md @@ -96,7 +96,7 @@ All subsequent tool handler REST API calls use this JWT → zero extra bcrypt **82 tools** across 12 categories: -### Project (15) +### Project (14) | Tool | Description | |------|-------------| @@ -113,7 +113,6 @@ All subsequent tool handler REST API calls use this JWT → zero extra bcrypt | `project_readme_update` | Update project README | | `project_lock` | Lock project (prevent edits) | | `project_unlock` | Unlock project | -| `project_load` | Load project from path | | `project_locked` | Check if project is locked | ### Node (22) diff --git a/gns3server/agent/mcp/__init__.py b/gns3server/agent/mcp/__init__.py index 538f0d6e7..608070f0f 100644 --- a/gns3server/agent/mcp/__init__.py +++ b/gns3server/agent/mcp/__init__.py @@ -61,7 +61,7 @@ from .projects import ( get_project_stats_handler, update_project_handler, duplicate_project_handler, get_project_readme_handler, update_project_readme_handler, lock_project_handler, unlock_project_handler, - load_project_handler, get_locked_project_handler, + get_locked_project_handler, ) from .server import ( get_version_handler, get_statistics_handler, @@ -1264,16 +1264,6 @@ async def project_locked( }) -@mcp.tool() -async def project_load( - path: Annotated[str, Field(description="Filesystem path to the .gns3 project file")], -) -> list[dict[str, Any]]: - """Load a project from a file path on the server's filesystem.""" - return await asyncio.to_thread(_run_handler_sync, load_project_handler, { - "path": path, - }) - - # ── Server info tools ───────────────────────────────────────────────── diff --git a/gns3server/agent/mcp/projects.py b/gns3server/agent/mcp/projects.py index 24221db5c..e095354a1 100644 --- a/gns3server/agent/mcp/projects.py +++ b/gns3server/agent/mcp/projects.py @@ -176,15 +176,6 @@ def unlock_project_handler(params: dict[str, Any], gns3_ctx: dict[str, Any]) -> return {"message": f"Project {project_id} unlocked", "project_id": project_id} -def load_project_handler(params: dict[str, Any], gns3_ctx: dict[str, Any]) -> dict[str, Any]: - path = params.get("path") - if not path: - return {"error": "path is required"} - conn = _get_connector(gns3_ctx) - result = conn.http_call("post", f"{conn.base_url}/projects/load", json_data={"path": path}).json() - return {"message": f"Project loaded from {path}", "project": result} - - def get_locked_project_handler(params: dict[str, Any], gns3_ctx: dict[str, Any]) -> dict[str, Any]: project_id = params.get("project_id") if not project_id: diff --git a/gns3server/controller/__init__.py b/gns3server/controller/__init__.py index 9a519a616..9593ee1cc 100644 --- a/gns3server/controller/__init__.py +++ b/gns3server/controller/__init__.py @@ -745,6 +745,17 @@ class Controller: if not os.path.exists(path): raise ControllerError(f"'{path}' does not exist on the controller") + # A .gns3 file must live in its own directory: the project path is + # the file's parent directory. A file placed directly in the + # projects directory would register the shared projects root as the + # project directory, and deleting that project would wipe every + # project on the controller. + projects_path = os.path.realpath(self.projects_directory()) + if os.path.realpath(os.path.dirname(path)) == projects_path: + raise ControllerError( + f"'{path}' cannot be loaded: the .gns3 file must be in its own subdirectory of '{projects_path}'" + ) + topo_data = load_topology(path) topo_data.pop("topology") topo_data.pop("version") diff --git a/gns3server/controller/project.py b/gns3server/controller/project.py index ab39b9496..d6563ead0 100644 --- a/gns3server/controller/project.py +++ b/gns3server/controller/project.py @@ -471,6 +471,18 @@ class Project: @path.setter def path(self, path): check_path_allowed(path) + + # The projects directory itself (or one of its ancestors) must + # never become a project directory: deleting such a "project" + # would wipe every project on the controller. + real_path = os.path.realpath(path) + real_projects_path = os.path.realpath(get_default_project_directory()) + if os.path.commonpath([real_path, real_projects_path]) == real_path: + raise ControllerForbiddenError( + f"The project directory cannot be '{path}': it must be a subdirectory " + f"of '{real_projects_path}', not the projects directory itself or one of its parents" + ) + try: os.makedirs(path, exist_ok=True) except OSError as e: @@ -1608,11 +1620,19 @@ class Project: await self._cleanup_web_wireshark_container() try: - project_directory = get_default_project_directory() - if not os.path.commonprefix([project_directory, self.path]) == project_directory: + project_directory = os.path.realpath(get_default_project_directory()) + path = os.path.realpath(self.path) + if os.path.commonpath([path, project_directory]) != project_directory: raise ControllerError( f"Project '{self._name}' cannot be deleted because it is not in the default project directory: '{project_directory}'" ) + if path == project_directory: + # A poisoned or hand-crafted entry whose path is the + # projects root itself must never be deletable: rmtree + # would wipe every project on the controller. + raise ControllerError( + f"Project '{self._name}' cannot be deleted because its directory is the projects directory itself: '{path}'" + ) shutil.rmtree(self.path) except OSError as e: raise ControllerError(f"Cannot delete project directory {self.path}: {str(e)}") diff --git a/tests/agent/mcp/test_tool_params.py b/tests/agent/mcp/test_tool_params.py index 281293c56..06f4ae173 100644 --- a/tests/agent/mcp/test_tool_params.py +++ b/tests/agent/mcp/test_tool_params.py @@ -36,7 +36,6 @@ HANDLER_FILES = { "lock_project_handler": "projects.py", "unlock_project_handler": "projects.py", "get_locked_project_handler": "projects.py", - "load_project_handler": "projects.py", "get_nodes_handler": API_HANDLERS_FILE, "get_node_handler": API_HANDLERS_FILE, "start_node_handler": API_HANDLERS_FILE, diff --git a/tests/api/routes/controller/test_projects.py b/tests/api/routes/controller/test_projects.py index 3f45d0ba6..aa7ae8d36 100644 --- a/tests/api/routes/controller/test_projects.py +++ b/tests/api/routes/controller/test_projects.py @@ -55,6 +55,11 @@ class TestControllerProjectRoutes: params = {"name": "test", "path": str(config.settings.Server.projects_path), "project_id": "00010203-0405-0607-0809-0a0b0c0d0e0f"} response = await client.post(app.url_path_for("create_project"), json=params) + # The projects directory itself must never become a project directory + assert response.status_code == status.HTTP_403_FORBIDDEN + + params = {"name": "test", "path": os.path.join(str(config.settings.Server.projects_path), "custom"), "project_id": "00010203-0405-0607-0809-0a0b0c0d0e0f"} + response = await client.post(app.url_path_for("create_project"), json=params) assert response.status_code == status.HTTP_201_CREATED assert response.json()["name"] == "test" assert response.json()["project_id"] == "00010203-0405-0607-0809-0a0b0c0d0e0f" diff --git a/tests/controller/test_controller.py b/tests/controller/test_controller.py index 741b2cbce..4f158c9b3 100644 --- a/tests/controller/test_controller.py +++ b/tests/controller/test_controller.py @@ -125,6 +125,54 @@ async def test_load_projects_skip_unexpected_errors(controller, projects_dir): mock_load_project.assert_called_with(os.path.join(projects_dir, "broken_project", "broken.gns3"), load=False) +def _write_topology_file(path, project_id, name): + with open(path, "w+") as f: + json.dump( + { + "name": name, + "project_id": project_id, + "version": __version__, + "revision": 10, + "type": "topology", + "topology": {"computes": [], "drawings": [], "links": [], "nodes": []}, + }, + f, + ) + + +@pytest.mark.asyncio +async def test_load_project_refuses_gns3_in_projects_directory(controller, projects_dir): + """ + A .gns3 placed directly in the projects directory must not be + loadable: its parent directory (the shared projects root) would become + the project directory, and deleting that project would wipe every + project on the controller. + """ + + topology_file = os.path.join(projects_dir, "root-level.gns3") + _write_topology_file(topology_file, str(uuid.uuid4()), "root-level") + + with pytest.raises(ControllerError): + await controller.load_project(topology_file) + assert not controller._projects + + +@pytest.mark.asyncio +async def test_load_project_from_own_subdirectory(controller, projects_dir): + """ + The normal layout — a .gns3 inside its own subdirectory — keeps + loading, with the subdirectory as the project directory. + """ + + project_dir = os.path.join(projects_dir, "sub-project") + os.makedirs(project_dir) + topology_file = os.path.join(project_dir, "sub-project.gns3") + _write_topology_file(topology_file, str(uuid.uuid4()), "sub-project") + + project = await controller.load_project(topology_file, load=False) + assert project.path == project_dir + + def test_projects_directory_event_handler_filters_events(controller): controller._notify_projects_directory_event = MagicMock() diff --git a/tests/controller/test_import_project.py b/tests/controller/test_import_project.py index e0cbfa1b1..0d5cff67f 100644 --- a/tests/controller/test_import_project.py +++ b/tests/controller/test_import_project.py @@ -87,7 +87,8 @@ async def test_import_project_override(projects_dir, controller): override the previous keeping the same project id & location """ - tmpdir = Path(projects_dir) + tmpdir = Path(projects_dir) / "override-location" + tmpdir.mkdir(parents=True, exist_ok=True) project_id = str(uuid.uuid4()) topology = { "project_id": project_id, diff --git a/tests/controller/test_project.py b/tests/controller/test_project.py index 5b5b823e9..31a4a2e7a 100644 --- a/tests/controller/test_project.py +++ b/tests/controller/test_project.py @@ -741,6 +741,37 @@ async def test_delete(project): assert not os.path.exists(project.path) +@pytest.mark.asyncio +async def test_delete_refuses_to_delete_projects_directory(project, projects_dir): + """ + A poisoned entry whose path is the projects directory itself (a .gns3 + loaded directly from the projects root before the guard existed) must + not be deletable: rmtree would wipe every project on the controller. + """ + + other_project = os.path.join(projects_dir, "another-project") + os.makedirs(other_project, exist_ok=True) + + # Simulate the poisoned in-memory state directly: the path setter now + # rejects such an assignment, but a long-running server can still hold + # an entry created before the fix. + project._path = projects_dir + + with pytest.raises(ControllerError): + await project.delete() + assert os.path.exists(other_project) + + +def test_path_setter_rejects_projects_directory(project, projects_dir): + """ + The projects directory itself must never become a project directory. + """ + + with pytest.raises(ControllerForbiddenError): + project.path = projects_dir + assert project.path == os.path.join(projects_dir, project.id) + + @pytest.mark.asyncio async def test_delete_does_not_start_nodes(project): """ diff --git a/tests/controller/test_project_open.py b/tests/controller/test_project_open.py index 40ddd9ca1..073fa3877 100644 --- a/tests/controller/test_project_open.py +++ b/tests/controller/test_project_open.py @@ -192,12 +192,14 @@ async def test_open(controller, projects_dir): "version": "2.0.0" } - with open(os.path.join(projects_dir, "demo.gns3"), "w+") as f: + project_dir = os.path.join(projects_dir, "demo") + os.makedirs(project_dir) + with open(os.path.join(project_dir, "demo.gns3"), "w+") as f: json.dump(simple_topology, f) project = Project(name="demo", project_id="64ba8408-afbf-4b66-9cdd-1fd854427478", - path=str(projects_dir), + path=project_dir, controller=controller, filename="demo.gns3", status="closed")