diff --git a/routes/mcp_routes.py b/routes/mcp_routes.py index c09108f..003559a 100644 --- a/routes/mcp_routes.py +++ b/routes/mcp_routes.py @@ -5,6 +5,7 @@ import os import uuid import urllib.parse import html +from pathlib import Path from fastapi import APIRouter, Form, HTTPException, Request from fastapi.responses import RedirectResponse, HTMLResponse import logging @@ -12,6 +13,7 @@ import httpx from core.database import McpServer, SessionLocal from core.middleware import require_admin +from src.constants import DATA_DIR from src.mcp_manager import McpManager logger = logging.getLogger(__name__) @@ -19,6 +21,75 @@ logger = logging.getLogger(__name__) router = APIRouter(prefix="/api/mcp", tags=["mcp"]) +def _mcp_oauth_base_dir() -> Path: + """Directory that may contain OAuth files managed by Odysseus.""" + return (Path(DATA_DIR) / "mcp_oauth").resolve(strict=False) + + +def _resolve_mcp_oauth_path(raw_path, field_name: str) -> str: + """Resolve an MCP OAuth path and keep it under DATA_DIR/mcp_oauth.""" + raw = str(raw_path or "").strip() + if not raw: + return "" + + base = _mcp_oauth_base_dir() + path = Path(os.path.expanduser(raw)) + if not path.is_absolute(): + path = base / path + resolved = path.resolve(strict=False) + + try: + resolved.relative_to(base) + except ValueError as exc: + raise HTTPException( + 400, + f"Invalid OAuth {field_name}: path must stay under {base}", + ) from exc + return str(resolved) + + +def _sanitize_mcp_oauth_config(oauth_cfg): + """Return an OAuth config copy with file paths confined to mcp_oauth.""" + if not oauth_cfg: + return oauth_cfg + if not isinstance(oauth_cfg, dict): + return {} + sanitized = dict(oauth_cfg) + for field_name in ("keys_file", "token_file"): + if sanitized.get(field_name): + sanitized[field_name] = _resolve_mcp_oauth_path( + sanitized[field_name], + field_name, + ) + return sanitized + + +def _mcp_oauth_token_missing(oauth_cfg, *, strict: bool = True) -> bool: + """Check token existence without letting legacy bad paths break listing.""" + if not isinstance(oauth_cfg, dict): + return False + try: + token_file = _resolve_mcp_oauth_path(oauth_cfg.get("token_file", ""), "token_file") + except HTTPException: + if strict: + raise + logger.warning("Ignoring MCP OAuth config with unsafe token_file") + return True + return bool(token_file and not os.path.exists(token_file)) + + +def _apply_mcp_oauth_env(env: dict, oauth_cfg) -> None: + """Pass sanitized Gmail package paths to MCP servers that honor them.""" + if not oauth_cfg or not isinstance(env, dict): + return + keys_file = oauth_cfg.get("keys_file") + token_file = oauth_cfg.get("token_file") + if keys_file: + env["GMAIL_OAUTH_PATH"] = keys_file + if token_file: + env["GMAIL_CREDENTIALS_PATH"] = token_file + + def _load_disabled_map(): """Load per-server disabled tool sets from DB.""" db = SessionLocal() @@ -53,8 +124,7 @@ def setup_mcp_routes(mcp_manager: McpManager): oauth_cfg = json.loads(srv.oauth_config) if srv.oauth_config else None needs_oauth = False if oauth_cfg: - token_file = os.path.expanduser(oauth_cfg.get("token_file", "")) - needs_oauth = token_file and not os.path.exists(token_file) + needs_oauth = _mcp_oauth_token_missing(oauth_cfg, strict=False) disabled_list = json.loads(srv.disabled_tools) if srv.disabled_tools else [] total_tools = status.get("tool_count", 0) result.append({ @@ -111,26 +181,33 @@ def setup_mcp_routes(mcp_manager: McpManager): parsed_env = json.loads(env) if env else {} except json.JSONDecodeError: parsed_env = {} + if not isinstance(parsed_env, dict): + parsed_env = {} # Parse OAuth config parsed_oauth_config = None if oauth_config: try: - parsed_oauth_config = json.loads(oauth_config) + parsed_oauth_config = _sanitize_mcp_oauth_config(json.loads(oauth_config)) except json.JSONDecodeError: pass + _apply_mcp_oauth_env(parsed_env, parsed_oauth_config) # Write OAuth credentials file if provided (for Google MCP servers) logger.info(f"MCP add_server: oauth_file={oauth_file!r}") if oauth_file: try: oauth_data = json.loads(oauth_file) - oauth_dir = os.path.expanduser(oauth_data.get("dir", "")) + oauth_dir = _resolve_mcp_oauth_path(oauth_data.get("dir", ""), "dir") oauth_filename = oauth_data.get("filename", "") client_id = oauth_data.get("client_id", "") client_secret = oauth_data.get("client_secret", "") if oauth_dir and oauth_filename and client_id and client_secret: - os.makedirs(oauth_dir, exist_ok=True) + filepath = _resolve_mcp_oauth_path( + Path(oauth_dir) / str(oauth_filename), + "filename", + ) + os.makedirs(os.path.dirname(filepath), exist_ok=True) creds = { "installed": { "client_id": client_id, @@ -140,7 +217,6 @@ def setup_mcp_routes(mcp_manager: McpManager): "token_uri": "https://accounts.google.com/o/oauth2/token", } } - filepath = os.path.join(oauth_dir, oauth_filename) with open(filepath, "w", encoding="utf-8") as f: json.dump(creds, f, indent=2) logger.info(f"Wrote OAuth credentials to {filepath}") @@ -171,9 +247,7 @@ def setup_mcp_routes(mcp_manager: McpManager): # Check if OAuth token already exists — skip connection attempt if not needs_oauth = False if parsed_oauth_config: - token_file = os.path.expanduser(parsed_oauth_config.get("token_file", "")) - if token_file and not os.path.exists(token_file): - needs_oauth = True + needs_oauth = _mcp_oauth_token_missing(parsed_oauth_config) connected = False if not needs_oauth: @@ -349,8 +423,8 @@ def setup_mcp_routes(mcp_manager: McpManager): if not srv.oauth_config: raise HTTPException(400, "Server has no OAuth config") - oauth_cfg = json.loads(srv.oauth_config) - keys_file = os.path.expanduser(oauth_cfg.get("keys_file", "")) + oauth_cfg = _sanitize_mcp_oauth_config(json.loads(srv.oauth_config)) + keys_file = oauth_cfg.get("keys_file", "") if not keys_file or not os.path.exists(keys_file): raise HTTPException(400, "OAuth keys file not found") @@ -423,9 +497,11 @@ def setup_mcp_routes(mcp_manager: McpManager): if not srv.oauth_config: return HTMLResponse(_oauth_result_page("Error", "No OAuth config."), status_code=400) - oauth_cfg = json.loads(srv.oauth_config) - keys_file = os.path.expanduser(oauth_cfg.get("keys_file", "")) - token_file = os.path.expanduser(oauth_cfg.get("token_file", "")) + oauth_cfg = _sanitize_mcp_oauth_config(json.loads(srv.oauth_config)) + keys_file = oauth_cfg.get("keys_file", "") + token_file = oauth_cfg.get("token_file", "") + if not keys_file or not token_file: + raise HTTPException(400, "OAuth keys/token file not configured") with open(keys_file, encoding="utf-8") as f: keys_data = json.load(f) @@ -488,6 +564,9 @@ def setup_mcp_routes(mcp_manager: McpManager): "Authorized but Connection Failed", f"Tokens saved, but the server failed to connect: {status.get('error', 'unknown error')}. Try reconnecting from Settings.", )) + except HTTPException as e: + logger.warning(f"OAuth callback rejected: {e.detail}") + return HTMLResponse(_oauth_result_page("Error", str(e.detail)), status_code=e.status_code) except Exception as e: logger.exception(f"OAuth callback error: {e}") return HTMLResponse(_oauth_result_page("Error", str(e)), status_code=500) diff --git a/static/js/admin.js b/static/js/admin.js index d69f5e8..2c2ceae 100644 --- a/static/js/admin.js +++ b/static/js/admin.js @@ -1133,11 +1133,11 @@ const _GOOGLE_OAUTH_HELP = `To get Google OAuth credentials: const MCP_PRESETS = [ { name: "Gmail", command: "npx", args: ["-y", "@gongrzhe/server-gmail-autoauth-mcp"], env: { GOOGLE_CLIENT_ID: "", GOOGLE_CLIENT_SECRET: "" }, - oauthFile: { dir: "~/.gmail-mcp", filename: "gcp-oauth.keys.json" }, + oauthFile: { dir: "gmail", filename: "gcp-oauth.keys.json" }, oauth: { provider: "google", - keys_file: "~/.gmail-mcp/gcp-oauth.keys.json", - token_file: "~/.gmail-mcp/credentials.json", + keys_file: "gmail/gcp-oauth.keys.json", + token_file: "gmail/credentials.json", scopes: ["https://www.googleapis.com/auth/gmail.modify", "https://www.googleapis.com/auth/gmail.settings.basic"], }, help: `Setup: diff --git a/tests/test_security_regressions.py b/tests/test_security_regressions.py index 0f3bbe6..8e30986 100644 --- a/tests/test_security_regressions.py +++ b/tests/test_security_regressions.py @@ -14,6 +14,7 @@ These are pure-function tests — no FastAPI app boot, no DB. import sys import types import json +import importlib from pathlib import Path import pytest @@ -938,6 +939,79 @@ def test_mcp_oauth_page_escapes_reflected_values(): assert f"{var} = html.escape({var}" in body, var +def _import_mcp_routes(): + sys.modules.pop("routes.mcp_routes", None) + return importlib.import_module("routes.mcp_routes") + + +def test_mcp_oauth_paths_resolve_under_data_dir(tmp_path, monkeypatch): + mcp_routes = _import_mcp_routes() + monkeypatch.setattr(mcp_routes, "DATA_DIR", str(tmp_path / "data")) + + resolved = Path(mcp_routes._resolve_mcp_oauth_path("gmail/credentials.json", "token_file")) + + base = (tmp_path / "data" / "mcp_oauth").resolve() + assert resolved == base / "gmail" / "credentials.json" + + +@pytest.mark.parametrize("raw_path", [ + "../../etc/passwd", + "/tmp/evil.keys", + "~/.gmail-mcp/credentials.json", +]) +def test_mcp_oauth_paths_reject_escapes(tmp_path, monkeypatch, raw_path): + from fastapi import HTTPException + + mcp_routes = _import_mcp_routes() + monkeypatch.setattr(mcp_routes, "DATA_DIR", str(tmp_path / "data")) + + with pytest.raises(HTTPException) as exc: + mcp_routes._resolve_mcp_oauth_path(raw_path, "token_file") + assert exc.value.status_code == 400 + + +def test_mcp_oauth_filename_join_cannot_escape_base(tmp_path, monkeypatch): + from fastapi import HTTPException + + mcp_routes = _import_mcp_routes() + monkeypatch.setattr(mcp_routes, "DATA_DIR", str(tmp_path / "data")) + + safe_dir = mcp_routes._resolve_mcp_oauth_path("gmail", "dir") + with pytest.raises(HTTPException): + mcp_routes._resolve_mcp_oauth_path(Path(safe_dir) / "../../escape.json", "filename") + + +def test_mcp_oauth_config_sanitizes_paths_and_env(tmp_path, monkeypatch): + mcp_routes = _import_mcp_routes() + monkeypatch.setattr(mcp_routes, "DATA_DIR", str(tmp_path / "data")) + + cfg = mcp_routes._sanitize_mcp_oauth_config({ + "provider": "google", + "keys_file": "gmail/gcp-oauth.keys.json", + "token_file": "gmail/credentials.json", + "scopes": ["https://www.googleapis.com/auth/gmail.modify"], + }) + env = {} + mcp_routes._apply_mcp_oauth_env(env, cfg) + + base = (tmp_path / "data" / "mcp_oauth" / "gmail").resolve() + assert cfg["keys_file"] == str(base / "gcp-oauth.keys.json") + assert cfg["token_file"] == str(base / "credentials.json") + assert env["GMAIL_OAUTH_PATH"] == cfg["keys_file"] + assert env["GMAIL_CREDENTIALS_PATH"] == cfg["token_file"] + + +def test_gmail_mcp_preset_uses_contained_oauth_paths(): + src = Path(__file__).resolve().parents[1] / "static" / "js" / "admin.js" + text = src.read_text() + preset = text.split('{ name: "Gmail"', 1)[1].split('{ name: "Email (IMAP/SMTP)"', 1)[0] + + assert "~/.gmail-mcp" not in preset + assert 'oauthFile: { dir: "gmail"' in preset + assert 'keys_file: "gmail/gcp-oauth.keys.json"' in preset + assert 'token_file: "gmail/credentials.json"' in preset + + # -- export/gallery filename hardening ----------------------------------------