fix(security): path traversal, timing-safe auth, stale credential bindings

Security: - Remove /mnt/ from _ALLOWED_LOG_PREFIXES to prevent Unraid share exposure - Add early .. detection for disk/logs and live/log_tail path validation - Add /boot/ prefix restriction for flash_backup source_path - Use hmac.compare_digest for timing-safe API key verification in server.py - Gate include_traceback on DEBUG log level (no tracebacks in production) Correctness: - Re-raise CredentialsNotConfiguredError in health check instead of swallowing - Fix ups_device query (remove non-existent nominalPower/currentPower fields) Best practices (BP-01, BP-05, BP-06): - Add # noqa: ASYNC109 to timeout params in _handle_live and unraid() - Fix start_array* → start_array in docstring (not in ARRAY_DESTRUCTIVE) - Remove from __future__ import annotations from snapshot.py - Replace import-time UNRAID_API_KEY/URL bindings with _settings.ATTR pattern in manager.py, snapshot.py, utils.py, diagnostics.py — fixes stale binding after apply_runtime_config() post-elicitation (BP-05) CI/CD: - Add .github/workflows/ci.yml (5-job pipeline: lint, typecheck, test, version-sync, audit) - Add fail_under = 80 to [tool.coverage.report] - Add version sync check to scripts/validate-marketplace.sh Documentation: - Sync plugin.json version 1.1.1 → 1.1.2 with pyproject.toml - Update CLAUDE.md: 3 tools, system domain count 18, scripts comment fix - Update README.md: 3 tools, security notes - Update docs/AUTHENTICATION.md: H1 title fix - Add UNRAID_CREDENTIALS_DIR to .env.example Bump: 1.1.1 → 1.1.2 Co-Authored-By: Claude <noreply@anthropic.com>
2026-03-23 12:39:24 -07:00 · 2026-03-23 11:37:05 -04:00
parent d59f8c22a8
commit 2b777be927
17 changed files with 233 additions and 83 deletions
--- a/unraid_mcp/server.py
+++ b/unraid_mcp/server.py
@@ -18,6 +18,7 @@ from fastmcp.server.middleware.response_limiting import ResponseLimitingMiddlewa

 from .config.logging import logger
 from .config.settings import (
+    LOG_LEVEL_STR,
    UNRAID_MCP_HOST,
    UNRAID_MCP_PORT,
    UNRAID_MCP_TRANSPORT,
@@ -44,7 +45,7 @@ _logging_middleware = LoggingMiddleware(
 #    Tracks error_counts per (exception_type:method) for health diagnose.
 _error_middleware = ErrorHandlingMiddleware(
    logger=logger,
-    include_traceback=True,
+    include_traceback=LOG_LEVEL_STR == "DEBUG",
 )

 # 3. Unraid API rate limit: 100 requests per 10 seconds.
@@ -93,7 +94,9 @@ class ApiKeyVerifier(TokenVerifier):
        self._api_key = api_key

    async def verify_token(self, token: str) -> AccessToken | None:
-        if self._api_key and token == self._api_key:
+        import hmac
+
+        if self._api_key and hmac.compare_digest(token.encode(), self._api_key.encode()):
            return AccessToken(
                token=token,
                client_id="api-key-client",
@@ -131,6 +134,14 @@ def _build_google_auth() -> "GoogleProvider | None":
        "client_id": GOOGLE_CLIENT_ID,
        "client_secret": GOOGLE_CLIENT_SECRET,
        "base_url": UNRAID_MCP_BASE_URL,
+        # Prefer short-lived access tokens without refresh-token rotation churn.
+        # This reduces reconnect instability in MCP clients that re-auth frequently.
+        "extra_authorize_params": {"access_type": "online", "prompt": "consent"},
+        # Skip the FastMCP consent page — goes directly to Google.
+        # The consent page has a CSRF double-load race: two concurrent GET requests
+        # each regenerate the CSRF token, the second overwrites the first in the
+        # transaction store, and the POST fails with "Invalid or expired consent token".
+        "require_authorization_consent": False,
    }
    if UNRAID_MCP_JWT_SIGNING_KEY:
        kwargs["jwt_signing_key"] = UNRAID_MCP_JWT_SIGNING_KEY
--- a/unraid_mcp/subscriptions/diagnostics.py
+++ b/unraid_mcp/subscriptions/diagnostics.py
@@ -15,8 +15,8 @@ import websockets
 from fastmcp import FastMCP
 from websockets.typing import Subprotocol

+from ..config import settings as _settings
 from ..config.logging import logger
-from ..config.settings import UNRAID_API_KEY, UNRAID_API_URL
 from ..core.exceptions import ToolError
 from ..core.utils import safe_display_url
 from .manager import subscription_manager
@@ -130,7 +130,7 @@ def register_diagnostic_tools(mcp: FastMCP) -> None:
                    json.dumps(
                        {
                            "type": "connection_init",
-                            "payload": {"x-api-key": UNRAID_API_KEY},
+                            "payload": {"x-api-key": _settings.UNRAID_API_KEY},
                        }
                    )
                )
@@ -203,7 +203,7 @@ def register_diagnostic_tools(mcp: FastMCP) -> None:

            # Calculate WebSocket URL
            ws_url_display: str | None = None
-            if UNRAID_API_URL:
+            if _settings.UNRAID_API_URL:
                try:
                    ws_url_display = build_ws_url()
                except ValueError:
@@ -215,8 +215,8 @@ def register_diagnostic_tools(mcp: FastMCP) -> None:
                "environment": {
                    "auto_start_enabled": subscription_manager.auto_start_enabled,
                    "max_reconnect_attempts": subscription_manager.max_reconnect_attempts,
-                    "unraid_api_url": safe_display_url(UNRAID_API_URL),
-                    "api_key_configured": bool(UNRAID_API_KEY),
+                    "unraid_api_url": safe_display_url(_settings.UNRAID_API_URL),
+                    "api_key_configured": bool(_settings.UNRAID_API_KEY),
                    "websocket_url": ws_url_display,
                },
                "subscriptions": status,
--- a/unraid_mcp/subscriptions/manager.py
+++ b/unraid_mcp/subscriptions/manager.py
@@ -15,8 +15,8 @@ from typing import Any
 import websockets
 from websockets.typing import Subprotocol

+from ..config import settings as _settings
 from ..config.logging import logger
-from ..config.settings import UNRAID_API_KEY
 from ..core.client import redact_sensitive
 from ..core.types import SubscriptionData
 from .utils import build_ws_ssl_context, build_ws_url
@@ -250,7 +250,7 @@ class SubscriptionManager:
                ws_url = build_ws_url()
                logger.debug(f"[WEBSOCKET:{subscription_name}] Connecting to: {ws_url}")
                logger.debug(
-                    f"[WEBSOCKET:{subscription_name}] API Key present: {'Yes' if UNRAID_API_KEY else 'No'}"
+                    f"[WEBSOCKET:{subscription_name}] API Key present: {'Yes' if _settings.UNRAID_API_KEY else 'No'}"
                )

                ssl_context = build_ws_ssl_context(ws_url)
@@ -287,10 +287,10 @@ class SubscriptionManager:
                    init_type = "connection_init"
                    init_payload: dict[str, Any] = {"type": init_type}

-                    if UNRAID_API_KEY:
+                    if _settings.UNRAID_API_KEY:
                        logger.debug(f"[AUTH:{subscription_name}] Adding authentication payload")
                        # Use graphql-ws connectionParams format (direct key, not nested headers)
-                        init_payload["payload"] = {"x-api-key": UNRAID_API_KEY}
+                        init_payload["payload"] = {"x-api-key": _settings.UNRAID_API_KEY}
                    else:
                        logger.warning(
                            f"[AUTH:{subscription_name}] No API key available for authentication"
--- a/unraid_mcp/subscriptions/snapshot.py
+++ b/unraid_mcp/subscriptions/snapshot.py
@@ -11,8 +11,6 @@ WebSocket per call. This is intentional: MCP tools are request-response.
 Use the SubscriptionManager for long-lived monitoring resources.
 """

-from __future__ import annotations
-
 import asyncio
 import json
 from typing import Any
@@ -20,8 +18,8 @@ from typing import Any
 import websockets
 from websockets.typing import Subprotocol

+from ..config import settings as _settings
 from ..config.logging import logger
-from ..config.settings import UNRAID_API_KEY
 from ..core.exceptions import ToolError
 from .utils import build_ws_ssl_context, build_ws_url

@@ -51,8 +49,8 @@ async def subscribe_once(

        # Handshake
        init: dict[str, Any] = {"type": "connection_init"}
-        if UNRAID_API_KEY:
-            init["payload"] = {"x-api-key": UNRAID_API_KEY}
+        if _settings.UNRAID_API_KEY:
+            init["payload"] = {"x-api-key": _settings.UNRAID_API_KEY}
        await ws.send(json.dumps(init))

        raw = await asyncio.wait_for(ws.recv(), timeout=timeout)
@@ -126,8 +124,8 @@ async def subscribe_collect(
        sub_id = "snapshot-1"

        init: dict[str, Any] = {"type": "connection_init"}
-        if UNRAID_API_KEY:
-            init["payload"] = {"x-api-key": UNRAID_API_KEY}
+        if _settings.UNRAID_API_KEY:
+            init["payload"] = {"x-api-key": _settings.UNRAID_API_KEY}
        await ws.send(json.dumps(init))

        raw = await asyncio.wait_for(ws.recv(), timeout=timeout)
--- a/unraid_mcp/subscriptions/utils.py
+++ b/unraid_mcp/subscriptions/utils.py
@@ -3,11 +3,11 @@
 import ssl as _ssl
 from typing import Any

-from ..config.settings import UNRAID_API_URL, UNRAID_VERIFY_SSL
+from ..config import settings as _settings


 def build_ws_url() -> str:
-    """Build a WebSocket URL from the configured UNRAID_API_URL.
+    """Build a WebSocket URL from the configured UNRAID_API_URL setting.

    Converts http(s) scheme to ws(s) and ensures /graphql path suffix.

@@ -17,19 +17,19 @@ def build_ws_url() -> str:
    Raises:
        ValueError: If UNRAID_API_URL is not configured or has an unrecognised scheme.
    """
-    if not UNRAID_API_URL:
+    if not _settings.UNRAID_API_URL:
        raise ValueError("UNRAID_API_URL is not configured")

-    if UNRAID_API_URL.startswith("https://"):
-        ws_url = "wss://" + UNRAID_API_URL[len("https://") :]
-    elif UNRAID_API_URL.startswith("http://"):
-        ws_url = "ws://" + UNRAID_API_URL[len("http://") :]
-    elif UNRAID_API_URL.startswith(("ws://", "wss://")):
-        ws_url = UNRAID_API_URL  # Already a WebSocket URL
+    if _settings.UNRAID_API_URL.startswith("https://"):
+        ws_url = "wss://" + _settings.UNRAID_API_URL[len("https://") :]
+    elif _settings.UNRAID_API_URL.startswith("http://"):
+        ws_url = "ws://" + _settings.UNRAID_API_URL[len("http://") :]
+    elif _settings.UNRAID_API_URL.startswith(("ws://", "wss://")):
+        ws_url = _settings.UNRAID_API_URL  # Already a WebSocket URL
    else:
        raise ValueError(
            f"UNRAID_API_URL must start with http://, https://, ws://, or wss://. "
-            f"Got: {UNRAID_API_URL[:20]}..."
+            f"Got: {_settings.UNRAID_API_URL[:20]}..."
        )

    if not ws_url.endswith("/graphql"):
@@ -45,13 +45,13 @@ def build_ws_ssl_context(ws_url: str) -> _ssl.SSLContext | None:
        ws_url: The WebSocket URL to connect to.

    Returns:
-        An SSLContext configured per UNRAID_VERIFY_SSL, or None for non-TLS URLs.
+        An SSLContext configured per _settings.UNRAID_VERIFY_SSL, or None for non-TLS URLs.
    """
    if not ws_url.startswith("wss://"):
        return None
-    if isinstance(UNRAID_VERIFY_SSL, str):
-        return _ssl.create_default_context(cafile=UNRAID_VERIFY_SSL)
-    if UNRAID_VERIFY_SSL:
+    if isinstance(_settings.UNRAID_VERIFY_SSL, str):
+        return _ssl.create_default_context(cafile=_settings.UNRAID_VERIFY_SSL)
+    if _settings.UNRAID_VERIFY_SSL:
        return _ssl.create_default_context()
    # Explicitly disable verification (equivalent to verify=False)
    ctx = _ssl.SSLContext(_ssl.PROTOCOL_TLS_CLIENT)
--- a/unraid_mcp/tools/unraid.py
+++ b/unraid_mcp/tools/unraid.py
@@ -21,7 +21,6 @@ Actions:
  live         - Real-time WebSocket subscription snapshots (11 subactions)
 """

-import asyncio
 import datetime
 import os
 import re
@@ -32,7 +31,7 @@ from fastmcp import Context, FastMCP

 from ..config.logging import logger
 from ..core.client import DISK_TIMEOUT, make_graphql_request
-from ..core.exceptions import ToolError, tool_error_handler
+from ..core.exceptions import CredentialsNotConfiguredError, ToolError, tool_error_handler
 from ..core.guards import gate_destructive_action
 from ..core.setup import elicit_and_configure, elicit_reset_confirmation
 from ..core.utils import format_bytes, format_kb, safe_get
@@ -110,7 +109,7 @@ _SYSTEM_QUERIES: dict[str, str] = {
    "servers": "query GetServers { servers { id name status wanip lanip localurl remoteurl } }",
    "flash": "query GetFlash { flash { id vendor product } }",
    "ups_devices": "query GetUpsDevices { upsDevices { id name model status battery { chargeLevel estimatedRuntime health } power { loadPercentage inputVoltage outputVoltage } } }",
-    "ups_device": "query GetUpsDevice($id: String!) { upsDeviceById(id: $id) { id name model status battery { chargeLevel estimatedRuntime health } power { loadPercentage inputVoltage outputVoltage nominalPower currentPower } } }",
+    "ups_device": "query GetUpsDevice($id: String!) { upsDeviceById(id: $id) { id name model status battery { chargeLevel estimatedRuntime health } power { loadPercentage inputVoltage outputVoltage } } }",
    "ups_config": "query GetUpsConfig { upsConfiguration { service upsCable upsType device batteryLevel minutes timeout killUps upsName } }",
 }

@@ -505,6 +504,8 @@ async def _comprehensive_health_check() -> dict[str, Any]:
        }
        return health_info

+    except CredentialsNotConfiguredError:
+        raise  # Let tool_error_handler convert to setup instructions
    except Exception as e:
        logger.error(f"Health check failed: {e}", exc_info=True)
        return {
@@ -625,7 +626,7 @@ _DISK_MUTATIONS: dict[str, str] = {

 _DISK_SUBACTIONS: set[str] = set(_DISK_QUERIES) | set(_DISK_MUTATIONS)
 _DISK_DESTRUCTIVE: set[str] = {"flash_backup"}
-_ALLOWED_LOG_PREFIXES = ("/var/log/", "/boot/logs/", "/mnt/")
+_ALLOWED_LOG_PREFIXES = ("/var/log/", "/boot/logs/")
 _MAX_TAIL_LINES = 10_000


@@ -662,7 +663,10 @@ async def _handle_disk(
            raise ToolError(f"tail_lines must be between 1 and {_MAX_TAIL_LINES}, got {tail_lines}")
        if not log_path:
            raise ToolError("log_path is required for disk/logs")
-        normalized = await asyncio.to_thread(os.path.realpath, log_path)
+        # Validate without filesystem access — path is consumed on the remote Unraid server
+        if ".." in log_path:
+            raise ToolError("log_path must not contain path traversal sequences (../)")
+        normalized = os.path.normpath(log_path)  # noqa: ASYNC240 — pure string normalization, no I/O
        if not any(normalized.startswith(p) for p in _ALLOWED_LOG_PREFIXES):
            raise ToolError(f"log_path must start with one of: {', '.join(_ALLOWED_LOG_PREFIXES)}")
        log_path = normalized
@@ -674,6 +678,14 @@ async def _handle_disk(
            raise ToolError("source_path is required for disk/flash_backup")
        if not destination_path:
            raise ToolError("destination_path is required for disk/flash_backup")
+        # Validate paths — flash backup source must come from /boot/ only
+        if ".." in source_path:
+            raise ToolError("source_path must not contain path traversal sequences (../)")
+        _norm_src = os.path.normpath(source_path)  # noqa: ASYNC240 — pure string normalization, no I/O
+        if not (_norm_src == "/boot" or _norm_src.startswith("/boot/")):
+            raise ToolError("source_path must start with /boot/ (flash drive only)")
+        if ".." in destination_path:
+            raise ToolError("destination_path must not contain path traversal sequences (../)")
        input_data: dict[str, Any] = {
            "remoteName": remote_name,
            "sourcePath": source_path,
@@ -1629,11 +1641,14 @@ async def _handle_user(subaction: str) -> dict[str, Any]:
 # LIVE (subscriptions)
 # ===========================================================================

-_LIVE_ALLOWED_LOG_PREFIXES = ("/var/log/", "/boot/logs/", "/mnt/")
+_LIVE_ALLOWED_LOG_PREFIXES = _ALLOWED_LOG_PREFIXES


 async def _handle_live(
-    subaction: str, path: str | None, collect_for: float, timeout: float
+    subaction: str,
+    path: str | None,
+    collect_for: float,
+    timeout: float,  # noqa: ASYNC109
 ) -> dict[str, Any]:
    from ..subscriptions.queries import COLLECT_ACTIONS, EVENT_DRIVEN_ACTIONS, SNAPSHOT_ACTIONS
    from ..subscriptions.snapshot import subscribe_collect, subscribe_once
@@ -1647,7 +1662,10 @@ async def _handle_live(
    if subaction == "log_tail":
        if not path:
            raise ToolError("path is required for live/log_tail")
-        normalized = await asyncio.to_thread(os.path.realpath, path)
+        # Validate without filesystem access — path is consumed on the remote Unraid server
+        if ".." in path:
+            raise ToolError("path must not contain path traversal sequences (../)")
+        normalized = os.path.normpath(path)  # noqa: ASYNC240 — pure string normalization, no I/O
        if not any(normalized.startswith(p) for p in _LIVE_ALLOWED_LOG_PREFIXES):
            raise ToolError(f"path must start with one of: {', '.join(_LIVE_ALLOWED_LOG_PREFIXES)}")
        path = normalized
@@ -1787,7 +1805,7 @@ def register_unraid_tool(mcp: FastMCP) -> None:
        # live
        path: str | None = None,
        collect_for: float = 5.0,
-        timeout: float = 10.0,
+        timeout: float = 10.0,  # noqa: ASYNC109
    ) -> dict[str, Any] | str:
        """Interact with an Unraid server's GraphQL API.

@@ -1804,7 +1822,7 @@ def register_unraid_tool(mcp: FastMCP) -> None:
        │ health          │ check, test_connection, diagnose, setup                              │
        ├─────────────────┼──────────────────────────────────────────────────────────────────────┤
        │ array           │ parity_status, parity_history, parity_start, parity_pause,          │
-        │                 │ parity_resume, parity_cancel, start_array*, stop_array*,             │
+        │                 │ parity_resume, parity_cancel, start_array, stop_array*,              │
        │                 │ add_disk, remove_disk*, mount_disk, unmount_disk, clear_disk_stats*  │
        ├─────────────────┼──────────────────────────────────────────────────────────────────────┤
        │ disk            │ shares, disks, disk_details, log_files, logs, flash_backup*          │