fix: session isolation for docker_stop and docker_cleanup
- docker_stop now validates container belongs to current session before stopping (prevents one agent from stopping another's work) - docker_cleanup now defaults to session_only=True for safety (agents can still use session_only=False with caution) Addresses audit finding: tools could cause cross-session interference
This commit is contained in:
parent
d1f8779f05
commit
77ce01d313
@ -21,9 +21,9 @@ from typing import Any, Dict, List, Optional
|
|||||||
from fastmcp import Context
|
from fastmcp import Context
|
||||||
from fastmcp.contrib.mcp_mixin import MCPMixin, mcp_tool
|
from fastmcp.contrib.mcp_mixin import MCPMixin, mcp_tool
|
||||||
|
|
||||||
# Port pool configuration
|
# Port pool configuration (32 ports should handle many concurrent sessions)
|
||||||
PORT_POOL_START = 8192
|
PORT_POOL_START = 8192
|
||||||
PORT_POOL_END = 8199
|
PORT_POOL_END = 8223
|
||||||
PORT_LOCK_DIR = Path("/tmp/ghydramcp-ports")
|
PORT_LOCK_DIR = Path("/tmp/ghydramcp-ports")
|
||||||
|
|
||||||
|
|
||||||
@ -204,7 +204,7 @@ class DockerMixin(MCPMixin):
|
|||||||
with the GhydraMCP plugin pre-installed.
|
with the GhydraMCP plugin pre-installed.
|
||||||
|
|
||||||
Supports multi-process environments with:
|
Supports multi-process environments with:
|
||||||
- Dynamic port allocation from a pool (8192-8199)
|
- Dynamic port allocation from a pool (8192-8223)
|
||||||
- Session-scoped container naming with UUIDs
|
- Session-scoped container naming with UUIDs
|
||||||
- Docker label-based tracking for cross-process visibility
|
- Docker label-based tracking for cross-process visibility
|
||||||
- Automatic cleanup of orphaned containers
|
- Automatic cleanup of orphaned containers
|
||||||
@ -520,7 +520,7 @@ class DockerMixin(MCPMixin):
|
|||||||
plugin pre-installed. The binary will be imported and analyzed,
|
plugin pre-installed. The binary will be imported and analyzed,
|
||||||
then the HTTP API will be available.
|
then the HTTP API will be available.
|
||||||
|
|
||||||
Ports are automatically allocated from the pool (8192-8199) to
|
Ports are automatically allocated from the pool (8192-8223) to
|
||||||
prevent conflicts between concurrent sessions. Container names
|
prevent conflicts between concurrent sessions. Container names
|
||||||
are auto-generated with the session ID to ensure uniqueness.
|
are auto-generated with the session ID to ensure uniqueness.
|
||||||
|
|
||||||
@ -544,7 +544,7 @@ class DockerMixin(MCPMixin):
|
|||||||
port = self._port_pool.allocate(self.session_id)
|
port = self._port_pool.allocate(self.session_id)
|
||||||
if port is None:
|
if port is None:
|
||||||
return {
|
return {
|
||||||
"error": "Port pool exhausted (8192-8199). Stop some containers first.",
|
"error": "Port pool exhausted (8192-8223). Stop some containers first.",
|
||||||
"allocated_ports": self._port_pool.get_allocated_ports(),
|
"allocated_ports": self._port_pool.get_allocated_ports(),
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -639,6 +639,10 @@ class DockerMixin(MCPMixin):
|
|||||||
) -> Dict[str, Any]:
|
) -> Dict[str, Any]:
|
||||||
"""Stop a GhydraMCP Docker container.
|
"""Stop a GhydraMCP Docker container.
|
||||||
|
|
||||||
|
For safety, this will only stop containers that belong to the current
|
||||||
|
MCP session. Attempting to stop another session's container will fail
|
||||||
|
with an error explaining whose container it is.
|
||||||
|
|
||||||
Args:
|
Args:
|
||||||
name_or_id: Container name or ID
|
name_or_id: Container name or ID
|
||||||
remove: Also remove the container (default: True)
|
remove: Also remove the container (default: True)
|
||||||
@ -649,18 +653,34 @@ class DockerMixin(MCPMixin):
|
|||||||
if not self._check_docker_available():
|
if not self._check_docker_available():
|
||||||
return {"error": "Docker is not available on this system"}
|
return {"error": "Docker is not available on this system"}
|
||||||
|
|
||||||
# Find the container to get its port for pool release
|
# Get container's session and port labels for validation
|
||||||
container_port = None
|
container_port = None
|
||||||
|
container_session = None
|
||||||
try:
|
try:
|
||||||
inspect_result = self._run_docker_cmd(
|
inspect_result = self._run_docker_cmd(
|
||||||
["inspect", "--format", "{{index .Config.Labels \"" + self.LABEL_PREFIX + ".port\"}}", name_or_id],
|
[
|
||||||
|
"inspect",
|
||||||
|
"--format",
|
||||||
|
"{{index .Config.Labels \"" + self.LABEL_PREFIX + ".port\"}}|{{index .Config.Labels \"" + self.LABEL_PREFIX + ".session\"}}",
|
||||||
|
name_or_id,
|
||||||
|
],
|
||||||
check=False,
|
check=False,
|
||||||
)
|
)
|
||||||
if inspect_result.stdout.strip().isdigit():
|
parts = inspect_result.stdout.strip().split("|")
|
||||||
container_port = int(inspect_result.stdout.strip())
|
if len(parts) >= 2:
|
||||||
|
if parts[0].isdigit():
|
||||||
|
container_port = int(parts[0])
|
||||||
|
container_session = parts[1] if parts[1] else None
|
||||||
except Exception:
|
except Exception:
|
||||||
pass
|
pass
|
||||||
|
|
||||||
|
# Session validation: only allow stopping own containers
|
||||||
|
if container_session and container_session != self.session_id:
|
||||||
|
return {
|
||||||
|
"error": f"Cannot stop container '{name_or_id}' - it belongs to session '{container_session}', not this session '{self.session_id}'.",
|
||||||
|
"hint": "Each MCP session can only stop its own containers for safety.",
|
||||||
|
}
|
||||||
|
|
||||||
try:
|
try:
|
||||||
# Stop the container
|
# Stop the container
|
||||||
self._run_docker_cmd(["stop", name_or_id])
|
self._run_docker_cmd(["stop", name_or_id])
|
||||||
@ -902,7 +922,7 @@ class DockerMixin(MCPMixin):
|
|||||||
3. Optionally waits for the container to become healthy
|
3. Optionally waits for the container to become healthy
|
||||||
4. Returns connection info for the instance
|
4. Returns connection info for the instance
|
||||||
|
|
||||||
Ports are auto-allocated from the pool (8192-8199) to prevent
|
Ports are auto-allocated from the pool (8192-8223) to prevent
|
||||||
conflicts between concurrent sessions.
|
conflicts between concurrent sessions.
|
||||||
|
|
||||||
Args:
|
Args:
|
||||||
@ -1003,7 +1023,7 @@ class DockerMixin(MCPMixin):
|
|||||||
)
|
)
|
||||||
async def docker_cleanup(
|
async def docker_cleanup(
|
||||||
self,
|
self,
|
||||||
session_only: bool = False,
|
session_only: bool = True,
|
||||||
max_age_hours: float = 24.0,
|
max_age_hours: float = 24.0,
|
||||||
dry_run: bool = False,
|
dry_run: bool = False,
|
||||||
ctx: Optional[Context] = None,
|
ctx: Optional[Context] = None,
|
||||||
@ -1013,8 +1033,12 @@ class DockerMixin(MCPMixin):
|
|||||||
This helps recover from crashed processes that left containers or
|
This helps recover from crashed processes that left containers or
|
||||||
port locks behind.
|
port locks behind.
|
||||||
|
|
||||||
|
By default, only cleans containers from the current session to prevent
|
||||||
|
accidentally removing another agent's work. Set session_only=False
|
||||||
|
(with caution) to clean all GhydraMCP containers.
|
||||||
|
|
||||||
Args:
|
Args:
|
||||||
session_only: Only clean up containers from this session
|
session_only: Only clean up containers from this session (default: True for safety)
|
||||||
max_age_hours: Max age for orphaned containers (default: 24 hours)
|
max_age_hours: Max age for orphaned containers (default: 24 hours)
|
||||||
dry_run: If True, only report what would be cleaned up
|
dry_run: If True, only report what would be cleaned up
|
||||||
|
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user