mirror of
https://github.com/GNS3/gns3-server.git
synced 2026-10-28 11:12:26 +02:00
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.
This commit is contained in:
parent
629bb9194f
commit
702fc1f6d9
@ -96,7 +96,7 @@ All subsequent tool handler REST API calls use this JWT → zero extra bcrypt
|
|||||||
|
|
||||||
**82 tools** across 12 categories:
|
**82 tools** across 12 categories:
|
||||||
|
|
||||||
### Project (15)
|
### Project (14)
|
||||||
|
|
||||||
| Tool | Description |
|
| 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_readme_update` | Update project README |
|
||||||
| `project_lock` | Lock project (prevent edits) |
|
| `project_lock` | Lock project (prevent edits) |
|
||||||
| `project_unlock` | Unlock project |
|
| `project_unlock` | Unlock project |
|
||||||
| `project_load` | Load project from path |
|
|
||||||
| `project_locked` | Check if project is locked |
|
| `project_locked` | Check if project is locked |
|
||||||
|
|
||||||
### Node (22)
|
### Node (22)
|
||||||
|
|||||||
@ -61,7 +61,7 @@ from .projects import (
|
|||||||
get_project_stats_handler, update_project_handler, duplicate_project_handler,
|
get_project_stats_handler, update_project_handler, duplicate_project_handler,
|
||||||
get_project_readme_handler, update_project_readme_handler,
|
get_project_readme_handler, update_project_readme_handler,
|
||||||
lock_project_handler, unlock_project_handler,
|
lock_project_handler, unlock_project_handler,
|
||||||
load_project_handler, get_locked_project_handler,
|
get_locked_project_handler,
|
||||||
)
|
)
|
||||||
from .server import (
|
from .server import (
|
||||||
get_version_handler, get_statistics_handler,
|
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 ─────────────────────────────────────────────────
|
# ── Server info tools ─────────────────────────────────────────────────
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@ -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}
|
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]:
|
def get_locked_project_handler(params: dict[str, Any], gns3_ctx: dict[str, Any]) -> dict[str, Any]:
|
||||||
project_id = params.get("project_id")
|
project_id = params.get("project_id")
|
||||||
if not project_id:
|
if not project_id:
|
||||||
|
|||||||
@ -745,6 +745,17 @@ class Controller:
|
|||||||
if not os.path.exists(path):
|
if not os.path.exists(path):
|
||||||
raise ControllerError(f"'{path}' does not exist on the controller")
|
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 = load_topology(path)
|
||||||
topo_data.pop("topology")
|
topo_data.pop("topology")
|
||||||
topo_data.pop("version")
|
topo_data.pop("version")
|
||||||
|
|||||||
@ -471,6 +471,18 @@ class Project:
|
|||||||
@path.setter
|
@path.setter
|
||||||
def path(self, path):
|
def path(self, path):
|
||||||
check_path_allowed(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:
|
try:
|
||||||
os.makedirs(path, exist_ok=True)
|
os.makedirs(path, exist_ok=True)
|
||||||
except OSError as e:
|
except OSError as e:
|
||||||
@ -1608,11 +1620,19 @@ class Project:
|
|||||||
await self._cleanup_web_wireshark_container()
|
await self._cleanup_web_wireshark_container()
|
||||||
|
|
||||||
try:
|
try:
|
||||||
project_directory = get_default_project_directory()
|
project_directory = os.path.realpath(get_default_project_directory())
|
||||||
if not os.path.commonprefix([project_directory, self.path]) == project_directory:
|
path = os.path.realpath(self.path)
|
||||||
|
if os.path.commonpath([path, project_directory]) != project_directory:
|
||||||
raise ControllerError(
|
raise ControllerError(
|
||||||
f"Project '{self._name}' cannot be deleted because it is not in the default project directory: '{project_directory}'"
|
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)
|
shutil.rmtree(self.path)
|
||||||
except OSError as e:
|
except OSError as e:
|
||||||
raise ControllerError(f"Cannot delete project directory {self.path}: {str(e)}")
|
raise ControllerError(f"Cannot delete project directory {self.path}: {str(e)}")
|
||||||
|
|||||||
@ -36,7 +36,6 @@ HANDLER_FILES = {
|
|||||||
"lock_project_handler": "projects.py",
|
"lock_project_handler": "projects.py",
|
||||||
"unlock_project_handler": "projects.py",
|
"unlock_project_handler": "projects.py",
|
||||||
"get_locked_project_handler": "projects.py",
|
"get_locked_project_handler": "projects.py",
|
||||||
"load_project_handler": "projects.py",
|
|
||||||
"get_nodes_handler": API_HANDLERS_FILE,
|
"get_nodes_handler": API_HANDLERS_FILE,
|
||||||
"get_node_handler": API_HANDLERS_FILE,
|
"get_node_handler": API_HANDLERS_FILE,
|
||||||
"start_node_handler": API_HANDLERS_FILE,
|
"start_node_handler": API_HANDLERS_FILE,
|
||||||
|
|||||||
@ -55,6 +55,11 @@ class TestControllerProjectRoutes:
|
|||||||
|
|
||||||
params = {"name": "test", "path": str(config.settings.Server.projects_path), "project_id": "00010203-0405-0607-0809-0a0b0c0d0e0f"}
|
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)
|
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.status_code == status.HTTP_201_CREATED
|
||||||
assert response.json()["name"] == "test"
|
assert response.json()["name"] == "test"
|
||||||
assert response.json()["project_id"] == "00010203-0405-0607-0809-0a0b0c0d0e0f"
|
assert response.json()["project_id"] == "00010203-0405-0607-0809-0a0b0c0d0e0f"
|
||||||
|
|||||||
@ -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)
|
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):
|
def test_projects_directory_event_handler_filters_events(controller):
|
||||||
|
|
||||||
controller._notify_projects_directory_event = MagicMock()
|
controller._notify_projects_directory_event = MagicMock()
|
||||||
|
|||||||
@ -87,7 +87,8 @@ async def test_import_project_override(projects_dir, controller):
|
|||||||
override the previous keeping the same project id & location
|
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())
|
project_id = str(uuid.uuid4())
|
||||||
topology = {
|
topology = {
|
||||||
"project_id": project_id,
|
"project_id": project_id,
|
||||||
|
|||||||
@ -741,6 +741,37 @@ async def test_delete(project):
|
|||||||
assert not os.path.exists(project.path)
|
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
|
@pytest.mark.asyncio
|
||||||
async def test_delete_does_not_start_nodes(project):
|
async def test_delete_does_not_start_nodes(project):
|
||||||
"""
|
"""
|
||||||
|
|||||||
@ -192,12 +192,14 @@ async def test_open(controller, projects_dir):
|
|||||||
"version": "2.0.0"
|
"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)
|
json.dump(simple_topology, f)
|
||||||
|
|
||||||
project = Project(name="demo",
|
project = Project(name="demo",
|
||||||
project_id="64ba8408-afbf-4b66-9cdd-1fd854427478",
|
project_id="64ba8408-afbf-4b66-9cdd-1fd854427478",
|
||||||
path=str(projects_dir),
|
path=project_dir,
|
||||||
controller=controller,
|
controller=controller,
|
||||||
filename="demo.gns3",
|
filename="demo.gns3",
|
||||||
status="closed")
|
status="closed")
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user