diff --git a/README.md b/README.md index 89e2123d4..51487fadc 100644 --- a/README.md +++ b/README.md @@ -488,7 +488,7 @@ DeerFlow supports configurable MCP servers and skills to extend its capabilities For HTTP/SSE MCP servers, OAuth token flows are supported (`client_credentials`, `refresh_token`). For stdio MCP servers, per-tool call timeouts can be configured with `tool_call_timeout`; durable background-task calls honor the same setting for HTTP/SSE servers as well. MCP tool names are prefixed with `_` by default to prevent collisions across servers. If a server already namespaces its own tools, set `tool_name_prefix: false` on that server in `extensions_config.json` to keep the original names. Disable the prefix only when the resulting names remain unique across all enabled servers. -Settings > Tools updates one MCP server at a time: an invalid stdio command on one server no longer blocks toggling another, while enabling that invalid server remains protected by the command allowlist and surfaces the backend validation message in the UI. +Settings > Tools adds, replaces, and deletes one MCP server at a time through targeted mutations that preserve concurrent sibling changes; deletes use a bodyless URL-addressed request. An invalid stdio command on one server no longer blocks toggling another, while enabling that invalid server remains protected by the command allowlist and surfaces the backend validation message in the UI. Targeted updates accept both DeerFlow's `type` field and the MCP-spec `transport` field for SSE/HTTP servers. Runtime MCP and skill updates replace `extensions_config.json` atomically, so an interrupted write cannot leave the shared configuration truncated or partially written. MCP routing hints can also prefer a specific MCP tool for matching requests without forbidding other tools. When `tool_search` defers MCP schemas, matching routing metadata can auto-promote up to `tool_search.auto_promote_top_k` deferred schemas before the model call. diff --git a/backend/app/gateway/AGENTS.md b/backend/app/gateway/AGENTS.md index d9accda26..9955d9a8b 100644 --- a/backend/app/gateway/AGENTS.md +++ b/backend/app/gateway/AGENTS.md @@ -46,7 +46,7 @@ owner-scoped assistant version selection remains enabled. | **Models** (`/api/models`) | `GET /` - list models; `GET /{name}` - model details | | **Features** (`/api/features`) | `GET /` - UI capabilities: hot-reloaded agents, guarded browser, startup MCP tasks, and separate batch repository/worker states so history stays readable without a worker | | **Console** (`/api/console`) | Read-only cross-thread observability for the current user (the data layer for an operations dashboard or external monitoring): `GET /stats` - headline counters (runs/threads/agents/tokens/cost); `GET /runs` - paginated run history joined with thread titles (per-run cost); `GET /usage` - zero-filled daily token series + per-model breakdown with spend. Queries `runs`/`threads_meta` directly as a reporting layer (no new `RunStore` methods); requires a SQL database backend — returns 503 on `database.backend: memory`. Real-cost estimation reads optional `models[*].pricing` (`currency`, `input_per_million`, `output_per_million`, `input_cache_hit_per_million`; `ModelConfig` is `extra="allow"`, so no schema change) and prices each run from its `token_usage_by_model` input/output split. Pricing is **cache-aware**: `RunJournal` accumulates prompt-cache hits from `usage_metadata.input_token_details.cache_read` into a sparse `cache_read_tokens` bucket key (also threaded through `SubagentTokenCollector` → `record_external_llm_usage_records`), and cache-hit input tokens are billed at `input_cache_hit_per_million` (omitted → billed at the miss price, a conservative upper bound). All priced models must use one currency; mixed currencies disable cost reporting and leave cost/currency fields null instead of producing invalid aggregates. Legacy rows fall back to run-level totals at `model_name`; unpriced models yield `cost: null` and cost fields are null when no pricing is configured | -| **MCP** (`/api/mcp`) | `GET /config` - get config; `PUT /config` - replace the full config with whole-payload stdio validation; `PATCH /config` - toggle one server while preserving the raw extensions config and validating only an enabled target; both writes reload config and reset the process-local MCP cache | +| **MCP** (`/api/mcp`) | GET /config - raw/masked; PUT /config - bulk; PATCH /config - toggle; POST /config/servers - add; PUT /config/server - replace; DELETE /config/servers/{server_name:path} - bodyless. Validate expanded, save raw; reload/reset; invalid -> 400. | | **MCP Tasks** (`/api/threads/{id}/mcp-tasks`) | `GET /` - current user's durable tasks for one owned thread; `GET /{task_id}` - bounded result/input/status-error/cancellation-error detail, including cancellation attempt count, without remote task IDs or driver configuration | | **Skills** (`/api/skills`) | `GET /` - list; `GET /{name}` - inspect; `PUT /{name}` - toggle; `POST /install` - install a thread-local .skill archive; `POST /install/upload` - admin-only multipart, authorized before parsing and capped at a 100 MiB file plus 1 MiB framing; `POST /reload` - invalidate process-local cache after trusted filesystem changes | | **Subagents** (`/api/subagents`) | Admin managed-worker CRUD and listing. | diff --git a/backend/app/gateway/routers/mcp.py b/backend/app/gateway/routers/mcp.py index 8d87934d0..577a0312b 100644 --- a/backend/app/gateway/routers/mcp.py +++ b/backend/app/gateway/routers/mcp.py @@ -4,10 +4,10 @@ import logging import os import re from pathlib import Path -from typing import Any, Literal, NamedTuple +from typing import Any, Literal, NamedTuple, NoReturn from fastapi import APIRouter, HTTPException, Request, status -from pydantic import BaseModel, ConfigDict, Field, field_validator, model_validator +from pydantic import BaseModel, ConfigDict, Field, ValidationError, field_validator, model_validator from app.gateway.deps import require_admin_user from deerflow.config.extensions_config import ( @@ -22,6 +22,7 @@ from deerflow.config.extensions_config import ( normalize_mcp_transport_alias, reload_extensions_config, ) +from deerflow.config.runtime_paths import project_root from deerflow.constants import DEFAULT_MCP_SESSION_INIT_TIMEOUT from deerflow.mcp.cache import reset_mcp_tools_cache @@ -424,6 +425,9 @@ class McpOAuthConfigResponse(BaseModel): default_token_type: str = Field(default="Bearer", description="Default token type when response omits token_type") refresh_skew_seconds: int = Field(default=60, description="Refresh this many seconds before expiry") extra_token_params: dict[str, str] = Field(default_factory=dict, description="Additional form params sent to token endpoint") + # Mirror the harness-side McpOAuthConfig (extra="allow"): provider-specific + # OAuth fields must survive the Gateway's GET -> edit -> PUT round-trip. + model_config = ConfigDict(extra="allow") class McpServerConfigResponse(BaseModel): @@ -506,12 +510,24 @@ class McpServerStateUpdateRequest(BaseModel): server_name: str = Field( ..., - min_length=1, description="Name of the MCP server to update", ) enabled: bool = Field(..., description="Whether the MCP server is enabled") +class McpServerConfigUpdateRequest(BaseModel): + """Request model for replacing one MCP server configuration.""" + + server_name: str = Field( + ..., + description="Name of the existing MCP server to update", + ) + server: McpServerConfigResponse = Field( + ..., + description="Complete replacement configuration for the selected MCP server", + ) + + class McpCacheResetResponse(BaseModel): """Response model for resetting the MCP tools cache.""" @@ -544,6 +560,66 @@ def _mask_sensitive_extra_value(value: Any) -> Any: return value +def _contains_masked_sensitive_extra_value(key: str, value: Any) -> bool: + if value == _MASKED_VALUE and _is_sensitive_extra_key(key): + return True + if isinstance(value, dict): + return any(_contains_masked_sensitive_extra_value(str(nested_key), nested_value) for nested_key, nested_value in value.items()) + if isinstance(value, list): + return any(_contains_masked_sensitive_extra_value(key, item) for item in value) + return False + + +def _ensure_no_masked_secrets(server: McpServerConfigResponse) -> None: + """Reject request-only masked placeholders before config persistence.""" + + def reject(location: str) -> None: + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=f"Cannot persist masked secret placeholder for {location}; provide a real value.", + ) + + for key, value in server.env.items(): + if value == _MASKED_VALUE: + reject(f"env key '{key}'") + for key, value in server.headers.items(): + if value == _MASKED_VALUE: + reject(f"header '{key}'") + for key, value in (server.model_extra or {}).items(): + if _contains_masked_sensitive_extra_value(str(key), value): + reject(f"extra config key '{key}'") + + if server.oauth is not None: + if server.oauth.client_secret == _MASKED_VALUE: + reject("oauth client_secret") + if server.oauth.refresh_token == _MASKED_VALUE: + reject("oauth refresh_token") + for key, value in server.oauth.extra_token_params.items(): + if value == _MASKED_VALUE: + reject(f"oauth extra_token_params key '{key}'") + for key, value in (server.oauth.model_extra or {}).items(): + if _contains_masked_sensitive_extra_value(str(key), value): + reject(f"oauth extra config key '{key}'") + + if server.user_auth is not None: + for key, value in server.user_auth.users.items(): + if value == _MASKED_VALUE: + reject(f"user_auth credential '{key}'") + for key, value in (server.user_auth.model_extra or {}).items(): + if _contains_masked_sensitive_extra_value(str(key), value): + reject(f"user_auth extra config key '{key}'") + + if server.headers_from_context is not None: + for key, value in (server.headers_from_context.model_extra or {}).items(): + if _contains_masked_sensitive_extra_value(str(key), value): + reject(f"headers_from_context extra config key '{key}'") + + for tool_name, tool_override in server.tools.items(): + for key, value in (tool_override.model_extra or {}).items(): + if _contains_masked_sensitive_extra_value(str(key), value): + reject(f"tools override '{tool_name}' extra config key '{key}'") + + def _merge_extra_value_preserving_masked(key: str, incoming_value: Any, existing_value: Any, *, existing_present: bool) -> Any: if incoming_value == _MASKED_VALUE and _is_sensitive_extra_key(key): if existing_present: @@ -565,8 +641,15 @@ def _merge_extra_value_preserving_masked(key: str, incoming_value: Any, existing ) return merged - if isinstance(incoming_value, list) and isinstance(existing_value, list) and len(incoming_value) == len(existing_value): - return [_merge_extra_value_preserving_masked(key, nested_value, existing_value[index], existing_present=True) for index, nested_value in enumerate(incoming_value)] + if isinstance(incoming_value, list) and isinstance(existing_value, list): + if _contains_masked_sensitive_extra_value(key, incoming_value): + if incoming_value != _mask_sensitive_extra_value(existing_value): + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=f"Cannot edit extra config array '{key}' while masked secrets remain; provide real values for every masked secret.", + ) + return existing_value + return incoming_value return incoming_value @@ -675,37 +758,43 @@ def _arbitrary_exec_arg(args: list[str], *, command: str) -> str | None: return None -def _validate_mcp_update_request(request: McpConfigUpdateRequest) -> None: +def _validate_mcp_update_request( + request: McpConfigUpdateRequest, + *, + enforce_execution_policy: bool = True, +) -> None: """Validate API-submitted MCP config before it is persisted. Local config files can still express arbitrary advanced setups, but the HTTP API is an untrusted boundary. Restricting stdio commands here reduces the blast radius of a compromised authenticated browser session. - The command name alone is not a meaningful restriction, so the launcher's - ``args`` and ``env`` are screened for the flags and variables that turn an - allowlisted binary into an arbitrary code evaluator. + Command shape and code-injecting environment variables are invalid at the + API boundary even while a server remains disabled. The allowlist and its + companion argument screen are execution policy, so targeted offline edits + may defer only those checks until the server is enabled. """ - allowed_commands = _allowed_stdio_commands() + allowed_commands = _allowed_stdio_commands() if enforce_execution_policy else set() for name, server in request.mcp_servers.items(): transport_type = (server.type or "stdio").lower() if transport_type != "stdio": continue command_name = _stdio_command_name(server.command, server_name=name) - if command_name not in allowed_commands: - allowed = ", ".join(sorted(allowed_commands)) or "" - raise HTTPException( - status_code=status.HTTP_400_BAD_REQUEST, - detail=(f"MCP server '{name}' uses disallowed stdio command '{command_name}'. Allowed commands: {allowed}. Configure {_MCP_STDIO_COMMAND_ALLOWLIST_ENV} to extend this list."), - ) + if enforce_execution_policy: + if command_name not in allowed_commands: + allowed = ", ".join(sorted(allowed_commands)) or "" + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=(f"MCP server '{name}' uses disallowed stdio command '{command_name}'. Allowed commands: {allowed}. Configure {_MCP_STDIO_COMMAND_ALLOWLIST_ENV} to extend this list."), + ) - exec_flag = _arbitrary_exec_arg(server.args, command=command_name) - if exec_flag is not None: - raise HTTPException( - status_code=status.HTTP_400_BAD_REQUEST, - detail=(f"MCP server '{name}' passes '{exec_flag}' to '{command_name}', which would run arbitrary code. Point the server at a package or module instead."), - ) + exec_flag = _arbitrary_exec_arg(server.args, command=command_name) + if exec_flag is not None: + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=(f"MCP server '{name}' passes '{exec_flag}' to '{command_name}', which would run arbitrary code. Point the server at a package or module instead."), + ) for env_name in server.env: if env_name.strip().upper() in _CODE_INJECTING_ENV_VARS: @@ -725,10 +814,17 @@ def _mask_server_config(server: McpServerConfigResponse) -> McpServerConfigRespo masked_headers = {k: _MASKED_VALUE for k in server.headers} masked_oauth = None if server.oauth is not None: + # These values are arbitrary form fields sent directly to the token + # endpoint. Treat the whole map as credential-bearing instead of + # trying to recognize secrets from an open-ended key vocabulary. + masked_extra_token_params = {key: _MASKED_VALUE for key in server.oauth.extra_token_params} + masked_oauth_extra = {key: _MASKED_VALUE if _is_sensitive_extra_key(key) else _mask_sensitive_extra_value(value) for key, value in (server.oauth.model_extra or {}).items()} masked_oauth = server.oauth.model_copy( update={ "client_secret": None, "refresh_token": None, + "extra_token_params": masked_extra_token_params, + **masked_oauth_extra, } ) masked_user_auth = None @@ -747,6 +843,10 @@ def _mask_server_config(server: McpServerConfigResponse) -> McpServerConfigRespo # an operator store a secret-bearing key that round-trips through PUT. masked_ch_extra = {key: _MASKED_VALUE if _is_sensitive_extra_key(key) else _mask_sensitive_extra_value(value) for key, value in (server.headers_from_context.model_extra or {}).items()} masked_headers_from_context = server.headers_from_context.model_copy(update=masked_ch_extra) + masked_tools = {} + for tool_name, tool_override in server.tools.items(): + masked_tool_extra = {key: _MASKED_VALUE if _is_sensitive_extra_key(key) else _mask_sensitive_extra_value(value) for key, value in (tool_override.model_extra or {}).items()} + masked_tools[tool_name] = tool_override.model_copy(update=masked_tool_extra) masked_extra = {key: _MASKED_VALUE if _is_sensitive_extra_key(key) else _mask_sensitive_extra_value(value) for key, value in (server.model_extra or {}).items()} return server.model_copy( update={ @@ -755,6 +855,7 @@ def _mask_server_config(server: McpServerConfigResponse) -> McpServerConfigRespo "oauth": masked_oauth, "user_auth": masked_user_auth, "headers_from_context": masked_headers_from_context, + "tools": masked_tools, **masked_extra, } ) @@ -763,6 +864,8 @@ def _mask_server_config(server: McpServerConfigResponse) -> McpServerConfigRespo def _merge_preserving_secrets( incoming: McpServerConfigResponse, existing: McpServerConfigResponse, + *, + preserve_omitted_fields: bool = True, ) -> McpServerConfigResponse: """Merge incoming config with existing, preserving secrets masked by GET. @@ -778,6 +881,11 @@ def _merge_preserving_secrets( so masked GET responses can be safely round-tripped. To explicitly clear a stored secret, clients may send an empty string, which is converted to ``None`` before persisting. + + ``preserve_omitted_fields`` keeps the legacy bulk PUT's partial-update + behavior. Targeted PUT disables it because that endpoint is a complete + replacement: omissions must delete/reset ordinary fields, while explicit + masked placeholders still restore only their matching stored secrets. """ merged_env = {} for k, v in incoming.env.items(): @@ -806,93 +914,163 @@ def _merge_preserving_secrets( merged_headers[k] = v merged_oauth = incoming.oauth - if incoming.oauth is not None and existing.oauth is not None: - # None = preserve (masked round-trip), "" = explicitly clear, else = new value - merged_client_secret = existing.oauth.client_secret if incoming.oauth.client_secret is None else (None if incoming.oauth.client_secret == "" else incoming.oauth.client_secret) - merged_refresh_token = existing.oauth.refresh_token if incoming.oauth.refresh_token is None else (None if incoming.oauth.refresh_token == "" else incoming.oauth.refresh_token) - merged_oauth = incoming.oauth.model_copy( + if incoming.oauth is not None: + incoming_oauth = incoming.oauth + base_oauth = existing.oauth + base_extra_token_params = base_oauth.extra_token_params if base_oauth is not None else {} + merged_extra_token_params: dict[str, str] = {} + for key, value in incoming_oauth.extra_token_params.items(): + if value == _MASKED_VALUE: + if key not in base_extra_token_params: + raise HTTPException( + status_code=400, + detail=f"Cannot set oauth extra_token_params key '{key}' to masked value '***'; provide a real value.", + ) + merged_extra_token_params[key] = base_extra_token_params[key] + else: + merged_extra_token_params[key] = value + if preserve_omitted_fields and "extra_token_params" not in incoming_oauth.model_fields_set: + merged_extra_token_params = dict(base_extra_token_params) + + base_oauth_extra = (base_oauth.model_extra or {}) if base_oauth is not None else {} + merged_oauth_extra: dict[str, Any] = {} + for key, value in (incoming_oauth.model_extra or {}).items(): + merged_oauth_extra[key] = _merge_extra_value_preserving_masked( + key, + value, + base_oauth_extra.get(key), + existing_present=key in base_oauth_extra, + ) + if preserve_omitted_fields: + for key, value in base_oauth_extra.items(): + if key not in (incoming_oauth.model_extra or {}): + merged_oauth_extra[key] = value + + if base_oauth is not None: + # None = preserve (masked round-trip), "" = explicitly clear, + # else = new value. + merged_client_secret = base_oauth.client_secret if incoming_oauth.client_secret is None else (None if incoming_oauth.client_secret == "" else incoming_oauth.client_secret) + merged_refresh_token = base_oauth.refresh_token if incoming_oauth.refresh_token is None else (None if incoming_oauth.refresh_token == "" else incoming_oauth.refresh_token) + else: + merged_client_secret = incoming_oauth.client_secret + merged_refresh_token = incoming_oauth.refresh_token + merged_oauth = incoming_oauth.model_copy( update={ "client_secret": merged_client_secret, "refresh_token": merged_refresh_token, + "extra_token_params": merged_extra_token_params, + **merged_oauth_extra, } ) merged_user_auth = incoming.user_auth if incoming.user_auth is not None: - # Sub-field-aware merge: a partial user_auth payload (e.g. just - # {"enabled": false}) must not wipe the stored credential map or reset - # other stored sub-fields. Only sub-fields the request explicitly set - # replace stored values; the rest carry over — the same contract the - # block-level `model_fields_set` check below applies one level up. incoming_ua = incoming.user_auth base = existing.user_auth set_fields = incoming_ua.model_fields_set - effective: dict[str, Any] = {} - if base is not None: - effective.update({name: getattr(base, name) for name in ("enabled", "header", "users", "on_missing")}) - effective.update(base.model_extra or {}) - for name in ("enabled", "header", "on_missing"): - if name in set_fields: - effective[name] = getattr(incoming_ua, name) - # Extras are masked by GET (see _mask_server_config), so a round-trip - # PUT must swap masked sentinel values back for the stored ones — - # the same contract server-level extras get below. base_extra = (base.model_extra or {}) if base is not None else {} + merged_extra: dict[str, Any] = {} for key, value in (incoming_ua.model_extra or {}).items(): - effective[key] = _merge_extra_value_preserving_masked( + merged_extra[key] = _merge_extra_value_preserving_masked( key, value, base_extra.get(key), existing_present=key in base_extra, ) - if "users" in set_fields: - # An explicitly sent map replaces the stored one (so a full - # round-trip can remove a user), with masked values swapped back - # for the stored credentials. - existing_users = base.users if base is not None else {} - merged_users = {} - for k, v in incoming_ua.users.items(): - if v == _MASKED_VALUE: - if k in existing_users: - merged_users[k] = existing_users[k] - else: - raise HTTPException( - status_code=400, - detail=f"Cannot set user_auth credential for '{k}' to masked value '***'; provide a real value.", - ) + + existing_users = base.users if base is not None else {} + merged_users = {} + for k, v in incoming_ua.users.items(): + if v == _MASKED_VALUE: + if k in existing_users: + merged_users[k] = existing_users[k] else: - merged_users[k] = v - effective["users"] = merged_users - merged_user_auth = McpUserScopedAuthConfigResponse(**effective) + raise HTTPException( + status_code=400, + detail=f"Cannot set user_auth credential for '{k}' to masked value '***'; provide a real value.", + ) + else: + merged_users[k] = v + + if preserve_omitted_fields: + # A partial user_auth payload (for example only enabled=false) + # inherits omitted sub-fields under the legacy bulk PUT contract. + effective: dict[str, Any] = {} + if base is not None: + effective.update({name: getattr(base, name) for name in ("enabled", "header", "users", "on_missing")}) + effective.update(base_extra) + for name in ("enabled", "header", "on_missing"): + if name in set_fields: + effective[name] = getattr(incoming_ua, name) + effective.update(merged_extra) + if "users" in set_fields: + effective["users"] = merged_users + merged_user_auth = McpUserScopedAuthConfigResponse(**effective) + else: + # Targeted PUT is a complete replacement. Start from the incoming + # block so omitted ordinary sub-fields reset and omitted extras or + # users disappear; only explicit masked values above are restored. + merged_user_auth = incoming_ua.model_copy( + update={"users": merged_users, **merged_extra}, + ) merged_context_headers = incoming.headers_from_context if incoming.headers_from_context is not None: - # Sub-field-aware merge, mirroring user_auth above: an explicit partial - # block (e.g. {"enabled": false}) must not wipe the stored mapping or - # reset on_missing back to its default. Only fields the request set are - # replaced, so an explicitly supplied ``headers`` (even {}) replaces the - # map while omitted fields carry over from the stored block. incoming_ch = incoming.headers_from_context base_ch = existing.headers_from_context set_fields = incoming_ch.model_fields_set - effective: dict[str, Any] = {} - if base_ch is not None: - effective.update({name: getattr(base_ch, name) for name in ("enabled", "headers", "on_missing")}) - effective.update(base_ch.model_extra or {}) - for name in ("enabled", "headers", "on_missing"): - if name in set_fields: - effective[name] = getattr(incoming_ch, name) # Extras are masked by GET (see _mask_server_config), so a round-trip # PUT must swap masked sentinel values back for the stored ones — the # same contract user_auth extras and server-level extras get. base_ch_extra = (base_ch.model_extra or {}) if base_ch is not None else {} + merged_ch_extra: dict[str, Any] = {} for key, value in (incoming_ch.model_extra or {}).items(): - effective[key] = _merge_extra_value_preserving_masked( + merged_ch_extra[key] = _merge_extra_value_preserving_masked( key, value, base_ch_extra.get(key), existing_present=key in base_ch_extra, ) - merged_context_headers = McpContextHeadersConfigResponse(**effective) + + if preserve_omitted_fields: + # The legacy bulk PUT accepts partial nested blocks. Only fields + # the request set are replaced, while omitted fields carry over. + effective: dict[str, Any] = {} + if base_ch is not None: + effective.update({name: getattr(base_ch, name) for name in ("enabled", "headers", "on_missing")}) + effective.update(base_ch_extra) + for name in ("enabled", "headers", "on_missing"): + if name in set_fields: + effective[name] = getattr(incoming_ch, name) + effective.update(merged_ch_extra) + merged_context_headers = McpContextHeadersConfigResponse(**effective) + else: + # The targeted PUT is a complete replacement. Omitted ordinary + # fields and extras reset/disappear; explicit masked extras alone + # are restored from the stored block. + merged_context_headers = incoming_ch.model_copy(update=merged_ch_extra) + + merged_tools = {} + for tool_name, incoming_tool in incoming.tools.items(): + base_tool = existing.tools.get(tool_name) + base_tool_extra = (base_tool.model_extra or {}) if base_tool is not None else {} + merged_tool_extra: dict[str, Any] = {} + for key, value in (incoming_tool.model_extra or {}).items(): + merged_tool_extra[key] = _merge_extra_value_preserving_masked( + key, + value, + base_tool_extra.get(key), + existing_present=key in base_tool_extra, + ) + if preserve_omitted_fields: + for key, value in base_tool_extra.items(): + if key not in (incoming_tool.model_extra or {}): + merged_tool_extra[key] = value + merged_routing = incoming_tool.routing + if preserve_omitted_fields and base_tool is not None and "routing" not in incoming_tool.model_fields_set: + merged_routing = base_tool.routing + merged_tools[tool_name] = incoming_tool.model_copy( + update={"routing": merged_routing, **merged_tool_extra}, + ) update = { "env": merged_env, @@ -900,14 +1078,15 @@ def _merge_preserving_secrets( "oauth": merged_oauth, "user_auth": merged_user_auth, "headers_from_context": merged_context_headers, + "tools": merged_tools, } - if "user_auth" not in incoming.model_fields_set: + if preserve_omitted_fields and "user_auth" not in incoming.model_fields_set: update["user_auth"] = existing.user_auth - if "headers_from_context" not in incoming.model_fields_set: + if preserve_omitted_fields and "headers_from_context" not in incoming.model_fields_set: update["headers_from_context"] = existing.headers_from_context - if "routing" not in incoming.model_fields_set: + if preserve_omitted_fields and "routing" not in incoming.model_fields_set: update["routing"] = existing.routing - if "tools" not in incoming.model_fields_set: + if preserve_omitted_fields and "tools" not in incoming.model_fields_set: update["tools"] = existing.tools incoming_extra = incoming.model_extra or {} existing_extra = existing.model_extra or {} @@ -918,10 +1097,13 @@ def _merge_preserving_secrets( existing_extra.get(key), existing_present=key in existing_extra, ) - for key, value in (existing.model_extra or {}).items(): - if key not in (incoming.model_extra or {}): - update[key] = value - return incoming.model_copy(update=update) + if preserve_omitted_fields: + for key, value in (existing.model_extra or {}).items(): + if key not in (incoming.model_extra or {}): + update[key] = value + merged = incoming.model_copy(update=update) + _ensure_no_masked_secrets(merged) + return merged @router.get( @@ -953,12 +1135,42 @@ async def get_mcp_configuration(request: Request) -> McpConfigResponse: """ await require_admin_user(request, detail=_ADMIN_REQUIRED_DETAIL) - config = get_extensions_config() - - servers = {name: _mask_server_config(McpServerConfigResponse(**server.model_dump())) for name, server in config.mcp_servers.items()} + raw_servers = await asyncio.to_thread(_load_raw_mcp_server_responses) + servers = {name: _mask_server_config(server) for name, server in raw_servers.items()} return McpConfigResponse(mcp_servers=servers) +def _raise_invalid_mcp_configuration(detail: str, *, cause: Exception | None = None) -> NoReturn: + error = HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=f"Invalid MCP configuration: {detail}", + ) + if cause is not None: + raise error from cause + raise error + + +def _validation_error_summary(exc: ValidationError) -> str: + errors = exc.errors(include_url=False, include_input=False) + return "; ".join(f"{'.'.join(str(part) for part in error['loc']) or 'config'}: {error['msg']}" for error in errors) + + +def _mcp_server_response_from_raw(server_name: str, raw_server: Any) -> McpServerConfigResponse: + try: + return McpServerConfigResponse.model_validate(raw_server) + except ValidationError as exc: + _raise_invalid_mcp_configuration(f"mcpServers.{server_name}: {_validation_error_summary(exc)}", cause=exc) + + +def _validate_extensions_config_candidate(raw_data: dict) -> None: + """Reject a runtime-invalid candidate without changing its placeholders.""" + try: + resolved_data = ExtensionsConfig.resolve_env_variables(raw_data) + ExtensionsConfig.model_validate(resolved_data) + except ValidationError as exc: + _raise_invalid_mcp_configuration(_validation_error_summary(exc), cause=exc) + + def _apply_mcp_config_update(body: McpConfigUpdateRequest) -> dict: """Worker-thread body for :func:`update_mcp_configuration`. @@ -972,38 +1184,37 @@ def _apply_mcp_config_update(body: McpConfigUpdateRequest) -> dict: # same sidecar path for the complete read-modify-write cycle. config_path = ExtensionsConfig.resolve_config_path() if config_path is None: - config_path = Path.cwd().parent / "extensions_config.json" + config_path = project_root() / "extensions_config.json" logger.info(f"No existing extensions config found. Creating new config at: {config_path}") with extensions_config_write_lock, extensions_config_file_lock(config_path): # Load raw (un-resolved) JSON from disk to use as the merge source. # This preserves $VAR placeholders in env values and top-level keys # like mcpInterceptors that would otherwise be lost. - raw_servers: dict[str, dict] = {} + raw_data = _load_raw_extensions_config(config_path, create=True) + raw_servers = _raw_mcp_servers(raw_data) raw_other_keys: dict = {} raw_skills: dict[str, dict] | None = None - if config_path is not None and config_path.exists(): - with open(config_path, encoding="utf-8") as f: - raw_data = json.load(f) - raw_servers = raw_data.get("mcpServers", {}) - if isinstance(raw_data.get("skills"), dict): - raw_skills = raw_data["skills"] - # Preserve any top-level keys beyond mcpServers/skills - for key, value in raw_data.items(): - if key not in ("mcpServers", "skills"): - raw_other_keys[key] = value + if isinstance(raw_data.get("skills"), dict): + raw_skills = raw_data["skills"] + # Preserve any top-level keys beyond mcpServers/skills + for key, value in raw_data.items(): + if key not in ("mcpServers", "skills"): + raw_other_keys[key] = value # Merge incoming server configs with raw on-disk secrets merged_servers: dict[str, McpServerConfigResponse] = {} for name, incoming in body.mcp_servers.items(): raw_server = raw_servers.get(name) if raw_server is not None: - merged_servers[name] = _merge_preserving_secrets( + merged = _merge_preserving_secrets( incoming, - McpServerConfigResponse(**raw_server), + _mcp_server_response_from_raw(name, raw_server), ) else: - merged_servers[name] = incoming + merged = incoming + _ensure_no_masked_secrets(merged) + merged_servers[name] = merged # Build config data preserving all top-level keys from the original file config_data = dict(raw_other_keys) @@ -1013,6 +1224,7 @@ def _apply_mcp_config_update(body: McpConfigUpdateRequest) -> dict: raw_skills = {name: {"enabled": skill.enabled} for name, skill in current_config.skills.items()} config_data["skills"] = raw_skills + _validate_extensions_config_candidate(config_data) atomic_write_extensions_config(config_path, config_data) logger.info(f"MCP configuration updated and saved to: {config_path}") @@ -1020,8 +1232,8 @@ def _apply_mcp_config_update(body: McpConfigUpdateRequest) -> dict: # Reload the Gateway configuration and update the global cache. The # agent runtime lives in Gateway, so this keeps API reads and tool # execution aligned after extensions_config.json changes. - reloaded_config = reload_extensions_config() - return reloaded_config.mcp_servers + reload_extensions_config() + return _mcp_server_responses_from_raw(config_data) def _apply_mcp_server_state_update(body: McpServerStateUpdateRequest) -> dict: @@ -1040,19 +1252,17 @@ def _apply_mcp_server_state_update(body: McpServerStateUpdateRequest) -> dict: detail=f"MCP server '{body.server_name}' not found", ) - with open(config_path, encoding="utf-8") as f: - raw_data = json.load(f) - - raw_servers = raw_data.get("mcpServers", {}) - raw_server = raw_servers.get(body.server_name) if isinstance(raw_servers, dict) else None - if not isinstance(raw_server, dict): + raw_data = _load_raw_extensions_config(config_path, create=False) + raw_servers = _raw_mcp_servers(raw_data) + if body.server_name not in raw_servers: raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, detail=f"MCP server '{body.server_name}' not found", ) + raw_server = raw_servers[body.server_name] + target_server = _mcp_server_response_from_raw(body.server_name, raw_server) if body.enabled: - target_server = McpServerConfigResponse(**raw_server) _validate_mcp_update_request( McpConfigUpdateRequest( mcp_servers={body.server_name: target_server}, @@ -1060,11 +1270,155 @@ def _apply_mcp_server_state_update(body: McpServerStateUpdateRequest) -> dict: ) raw_server["enabled"] = body.enabled + _validate_extensions_config_candidate(raw_data) atomic_write_extensions_config(config_path, raw_data) logger.info("MCP server %s enabled state updated to %s", body.server_name, body.enabled) - reloaded_config = reload_extensions_config() - return reloaded_config.mcp_servers + reload_extensions_config() + return _mcp_server_responses_from_raw(raw_data) + + +def _mcp_config_path(*, create: bool) -> Path: + """Resolve the shared extensions config path for a targeted mutation.""" + config_path = ExtensionsConfig.resolve_config_path() + if config_path is None: + if create: + config_path = project_root() / "extensions_config.json" + logger.info("No existing extensions config found. Creating new config at: %s", config_path) + else: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail="MCP configuration not found", + ) + return config_path + + +def _load_raw_extensions_config(config_path: Path, *, create: bool) -> dict: + if config_path.exists(): + try: + with open(config_path, encoding="utf-8") as f: + raw_data = json.load(f) + except json.JSONDecodeError as exc: + _raise_invalid_mcp_configuration( + f"Extensions configuration is not valid JSON: {exc.msg} at line {exc.lineno} column {exc.colno}", + cause=exc, + ) + if not isinstance(raw_data, dict): + _raise_invalid_mcp_configuration("Extensions configuration must be a JSON object") + return raw_data + if not create: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail="MCP configuration not found", + ) + return {} + + +def _raw_mcp_servers(raw_data: dict) -> dict[str, dict]: + raw_servers = raw_data.get("mcpServers", {}) + if not isinstance(raw_servers, dict): + _raise_invalid_mcp_configuration("`mcpServers` must be a JSON object") + return raw_servers + + +def _mcp_server_responses_from_raw(raw_data: dict) -> dict[str, McpServerConfigResponse]: + """Build editable API models without expanding environment placeholders.""" + return {name: _mcp_server_response_from_raw(name, server) for name, server in _raw_mcp_servers(raw_data).items()} + + +def _load_raw_mcp_server_responses() -> dict[str, McpServerConfigResponse]: + """Read editable MCP server definitions under the shared config lock.""" + config_path = ExtensionsConfig.resolve_config_path() + if config_path is None: + return {} + + with extensions_config_write_lock, extensions_config_file_lock(config_path): + raw_data = _load_raw_extensions_config(config_path, create=False) + return _mcp_server_responses_from_raw(raw_data) + + +def _ensure_skills_key(raw_data: dict) -> None: + if isinstance(raw_data.get("skills"), dict): + return + current_config = get_extensions_config() + raw_data["skills"] = {name: {"enabled": skill.enabled} for name, skill in current_config.skills.items()} + + +def _apply_mcp_servers_create(body: McpConfigUpdateRequest) -> dict: + """Atomically add servers without replacing entries already on disk.""" + config_path = _mcp_config_path(create=True) + with extensions_config_write_lock, extensions_config_file_lock(config_path): + raw_data = _load_raw_extensions_config(config_path, create=True) + raw_servers = _raw_mcp_servers(raw_data) + duplicate = next((name for name in body.mcp_servers if name in raw_servers), None) + if duplicate is not None: + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail=f"MCP server '{duplicate}' already exists", + ) + + for name, incoming in body.mcp_servers.items(): + _ensure_no_masked_secrets(incoming) + raw_servers[name] = incoming.model_dump() + raw_data["mcpServers"] = raw_servers + _ensure_skills_key(raw_data) + _validate_extensions_config_candidate(raw_data) + atomic_write_extensions_config(config_path, raw_data) + + logger.info("Added MCP servers: %s", ", ".join(body.mcp_servers)) + reload_extensions_config() + return _mcp_server_responses_from_raw(raw_data) + + +def _apply_mcp_server_config_update(body: McpServerConfigUpdateRequest) -> dict: + """Atomically replace one server while preserving concurrent sibling edits.""" + config_path = _mcp_config_path(create=False) + with extensions_config_write_lock, extensions_config_file_lock(config_path): + raw_data = _load_raw_extensions_config(config_path, create=False) + raw_servers = _raw_mcp_servers(raw_data) + if body.server_name not in raw_servers: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=f"MCP server '{body.server_name}' not found", + ) + existing_server = _mcp_server_response_from_raw(body.server_name, raw_servers[body.server_name]) + + merged = _merge_preserving_secrets( + body.server, + existing_server, + preserve_omitted_fields=False, + ) + _ensure_no_masked_secrets(merged) + raw_servers[body.server_name] = merged.model_dump() + raw_data["mcpServers"] = raw_servers + _validate_extensions_config_candidate(raw_data) + atomic_write_extensions_config(config_path, raw_data) + + logger.info("Updated MCP server: %s", body.server_name) + reload_extensions_config() + return _mcp_server_responses_from_raw(raw_data) + + +def _apply_mcp_server_delete(server_name: str) -> dict: + """Atomically remove one server while preserving every sibling entry.""" + config_path = _mcp_config_path(create=False) + with extensions_config_write_lock, extensions_config_file_lock(config_path): + raw_data = _load_raw_extensions_config(config_path, create=False) + raw_servers = _raw_mcp_servers(raw_data) + if server_name not in raw_servers: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=f"MCP server '{server_name}' not found", + ) + + del raw_servers[server_name] + raw_data["mcpServers"] = raw_servers + _validate_extensions_config_candidate(raw_data) + atomic_write_extensions_config(config_path, raw_data) + + logger.info("Deleted MCP server: %s", server_name) + reload_extensions_config() + return _mcp_server_responses_from_raw(raw_data) @router.post( @@ -1148,6 +1502,77 @@ async def update_mcp_configuration(request: Request, body: McpConfigUpdateReques raise HTTPException(status_code=500, detail=f"Failed to update MCP configuration: {str(e)}") +@router.post( + "/mcp/config/servers", + response_model=McpConfigResponse, + summary="Add MCP Servers", + description="Add one or more MCP servers without replacing existing configurations.", +) +async def create_mcp_servers(request: Request, body: McpConfigUpdateRequest) -> McpConfigResponse: + """Add servers atomically and reject names that already exist.""" + try: + await require_admin_user(request, detail=_ADMIN_REQUIRED_DETAIL) + _validate_mcp_update_request(body) + reloaded_servers = await asyncio.to_thread(_apply_mcp_servers_create, body) + + servers = {name: _mask_server_config(McpServerConfigResponse(**server.model_dump())) for name, server in reloaded_servers.items()} + reset_mcp_tools_cache() + return McpConfigResponse(mcp_servers=servers) + except HTTPException: + raise + except Exception as e: + logger.error("Failed to add MCP servers: %s", e, exc_info=True) + raise HTTPException(status_code=500, detail=f"Failed to add MCP servers: {str(e)}") + + +@router.put( + "/mcp/config/server", + response_model=McpConfigResponse, + summary="Update MCP Server", + description="Replace one MCP server without replacing sibling configurations.", +) +async def update_mcp_server(request: Request, body: McpServerConfigUpdateRequest) -> McpConfigResponse: + """Update one existing server and reload the MCP tool cache.""" + try: + await require_admin_user(request, detail=_ADMIN_REQUIRED_DETAIL) + _validate_mcp_update_request( + McpConfigUpdateRequest(mcp_servers={body.server_name: body.server}), + enforce_execution_policy=body.server.enabled, + ) + reloaded_servers = await asyncio.to_thread(_apply_mcp_server_config_update, body) + + servers = {name: _mask_server_config(McpServerConfigResponse(**server.model_dump())) for name, server in reloaded_servers.items()} + reset_mcp_tools_cache() + return McpConfigResponse(mcp_servers=servers) + except HTTPException: + raise + except Exception as e: + logger.error("Failed to update MCP server %s: %s", body.server_name, e, exc_info=True) + raise HTTPException(status_code=500, detail=f"Failed to update MCP server: {str(e)}") + + +@router.delete( + "/mcp/config/servers/{server_name:path}", + response_model=McpConfigResponse, + summary="Delete MCP Server", + description="Delete one MCP server without replacing sibling configurations.", +) +async def delete_mcp_server(request: Request, server_name: str) -> McpConfigResponse: + """Delete one existing server and reload the MCP tool cache.""" + try: + await require_admin_user(request, detail=_ADMIN_REQUIRED_DETAIL) + reloaded_servers = await asyncio.to_thread(_apply_mcp_server_delete, server_name) + + servers = {name: _mask_server_config(McpServerConfigResponse(**server.model_dump())) for name, server in reloaded_servers.items()} + reset_mcp_tools_cache() + return McpConfigResponse(mcp_servers=servers) + except HTTPException: + raise + except Exception as e: + logger.error("Failed to delete MCP server %s: %s", server_name, e, exc_info=True) + raise HTTPException(status_code=500, detail=f"Failed to delete MCP server: {str(e)}") + + @router.patch( "/mcp/config", response_model=McpConfigResponse, diff --git a/backend/docs/API.md b/backend/docs/API.md index c70470af7..7651dc6c7 100644 --- a/backend/docs/API.md +++ b/backend/docs/API.md @@ -437,7 +437,10 @@ GET /api/mcp/config ``` Requires an authenticated admin session. Sensitive env/header/OAuth secret -values are masked in the response. +values are masked in the response. Environment placeholders outside secret +containers are returned in their raw form so editing cannot expose or persist +their expanded values. Invalid operator-authored JSON/config shapes return +`400` instead of being reported as a Gateway fault. **Response:** ```json @@ -537,6 +540,63 @@ The response is the full masked MCP configuration, matching `GET` and `PUT`. An unknown `server_name` returns `404`; attempting to enable a server with a disallowed `stdio` command returns `400`. +#### Add MCP Servers + +Add one or more servers without replacing existing entries. The Gateway +re-reads the file under the shared configuration lock, so concurrent sibling +changes are preserved. Existing names return `409`. + +```http +POST /api/mcp/config/servers +Content-Type: application/json +``` + +The request body uses the same `mcp_servers` map as the full `PUT` endpoint. + +#### Replace One MCP Server + +Completely replace one existing server while preserving sibling entries. +Omitted ordinary fields are deleted or reset; explicit `***` placeholders +restore the corresponding stored secret. + +A disabled `stdio` replacement may keep a syntactically valid command outside +the allowlist for offline editing. Command-shape and code-injecting environment +variable checks still run when saving; the allowlist and executable-argument +policy run when the server is enabled. + +```http +PUT /api/mcp/config/server +Content-Type: application/json +``` + +```json +{ + "server_name": "github", + "server": { + "enabled": true, + "command": "npx", + "args": ["-y", "@modelcontextprotocol/server-github"], + "env": {"GITHUB_TOKEN": "***"} + } +} +``` + +#### Delete One MCP Server + +Delete one server without replacing sibling entries. The server name is a +path parameter and the DELETE request has no body. Percent-encode names before +placing them in the URL; the path converter also keeps legacy empty and +slash-containing names addressable. + +```http +DELETE /api/mcp/config/servers/{server_name} +``` + +All targeted mutations return the full masked MCP configuration. Before any +write, the Gateway resolves environment variables in a copy and validates the +same expanded document the runtime will load while persisting the original raw +placeholders. + #### Reset MCP Tools Cache Clear cached MCP tools and persistent MCP sessions process-wide. This affects diff --git a/backend/tests/blocking_io/test_mcp_router.py b/backend/tests/blocking_io/test_mcp_router.py index 5ef82f52e..a4ade213f 100644 --- a/backend/tests/blocking_io/test_mcp_router.py +++ b/backend/tests/blocking_io/test_mcp_router.py @@ -1,9 +1,9 @@ -"""Regression anchor: updating MCP config must not block the event loop. +"""Regression anchor: reading or updating MCP config must not block the event loop. -The PUT and PATCH handlers resolve the extensions config path, probe its -existence, read raw JSON, atomically write it, and reload it — all blocking -filesystem IO. They offload the whole read-modify-write via -``asyncio.to_thread``; if either regresses back onto the event loop, the strict +The GET handler resolves the extensions config path and reads raw JSON. PUT +and PATCH also atomically write and reload it. All of that is blocking +filesystem IO, so the handlers offload the read or whole read-modify-write via +``asyncio.to_thread``. If one regresses back onto the event loop, the strict Blockbuster gate raises ``BlockingError`` and this test fails. The admin check is patched to a no-op so the anchor exercises the handler's own @@ -26,6 +26,7 @@ from app.gateway.routers.mcp import ( McpConfigUpdateRequest, McpServerConfigResponse, McpServerStateUpdateRequest, + get_mcp_configuration, update_mcp_configuration, update_mcp_server_state, ) @@ -33,6 +34,27 @@ from app.gateway.routers.mcp import ( pytestmark = pytest.mark.asyncio +async def test_get_mcp_configuration_does_not_block_or_expand_placeholders(tmp_path: Path, monkeypatch) -> None: + config_path = tmp_path / "extensions_config.json" + placeholder = "$CODEX_PR_5022_BLOCKING_TOKEN" + await asyncio.to_thread( + config_path.write_text, + '{"mcpServers":{"stdio":{"type":"stdio","command":"npx","args":["--token","' + placeholder + '"]}},"skills":{}}', + encoding="utf-8", + ) + monkeypatch.setenv("DEER_FLOW_EXTENSIONS_CONFIG_PATH", str(config_path)) + monkeypatch.setenv("CODEX_PR_5022_BLOCKING_TOKEN", "must-not-reach-the-editor") + + async def _noop_admin(_request, **_kwargs) -> None: + return None + + monkeypatch.setattr(mcp_router, "require_admin_user", _noop_admin) + + response = await get_mcp_configuration(request=None) + + assert response.mcp_servers["stdio"].args == ["--token", placeholder] + + async def test_update_mcp_configuration_does_not_block_event_loop(tmp_path: Path, monkeypatch) -> None: config_path = tmp_path / "extensions_config.json" # resolve_config_path() requires the env-pointed file to exist; seed a minimal one. diff --git a/backend/tests/test_mcp_config_secrets.py b/backend/tests/test_mcp_config_secrets.py index 3816224e4..d2c1daacf 100644 --- a/backend/tests/test_mcp_config_secrets.py +++ b/backend/tests/test_mcp_config_secrets.py @@ -3,7 +3,7 @@ Verifies that GET /api/mcp/config masks sensitive fields (env values, header values, OAuth secrets) and that PUT /api/mcp/config correctly preserves existing secrets when the frontend round-trips masked values. -PATCH /api/mcp/config coverage pins targeted state changes, raw-config +Targeted CRUD and PATCH /api/mcp/config coverage pin concurrent-sibling preservation, transport aliases, authorization, and command validation. """ @@ -13,7 +13,8 @@ import json from types import SimpleNamespace import pytest -from fastapi import HTTPException +from fastapi import FastAPI, HTTPException +from fastapi.testclient import TestClient from app.gateway.deps import require_admin_user from app.gateway.routers import mcp as mcp_router @@ -23,12 +24,18 @@ from app.gateway.routers.mcp import ( McpConfigUpdateRequest, McpOAuthConfigResponse, McpServerConfigResponse, + McpServerConfigUpdateRequest, McpServerStateUpdateRequest, _mask_server_config, _merge_preserving_secrets, + _validate_extensions_config_candidate, _validate_mcp_update_request, + create_mcp_servers, + delete_mcp_server, + get_mcp_configuration, reset_mcp_tools_cache_endpoint, update_mcp_configuration, + update_mcp_server, update_mcp_server_state, ) from deerflow.config.extensions_config import ExtensionsConfig, McpServerConfig @@ -75,6 +82,48 @@ def test_mask_removes_oauth_secrets(): assert masked.oauth.token_url == "https://auth.example.com/token" +def test_mask_scrubs_sensitive_oauth_extras_but_preserves_safe_extras(): + server = McpServerConfigResponse( + oauth=McpOAuthConfigResponse( + token_url="https://auth.example.com/token", + vendor_endpoint="https://vendor.example.com/oauth", + vendor_api_key="vendor-secret", + nested={"refreshToken": "refresh-secret", "safe": "visible"}, + ), + ) + + masked = _mask_server_config(server) + + assert masked.oauth is not None + assert masked.oauth.model_extra == { + "vendor_endpoint": "https://vendor.example.com/oauth", + "vendor_api_key": "***", + "nested": {"refreshToken": "***", "safe": "visible"}, + } + + +def test_mask_scrubs_all_oauth_extra_token_params(): + server = McpServerConfigResponse( + oauth=McpOAuthConfigResponse( + token_url="https://auth.example.com/token", + extra_token_params={ + "api_key": "vendor-secret", + "client_assertion": "signed-assertion", + "resource": "https://resource.example.com", + }, + ), + ) + + masked = _mask_server_config(server) + + assert masked.oauth is not None + assert masked.oauth.extra_token_params == { + "api_key": "***", + "client_assertion": "***", + "resource": "***", + } + + def test_mask_preserves_non_secret_fields(): """Non-sensitive fields should pass through unchanged.""" server = McpServerConfigResponse( @@ -136,6 +185,27 @@ def test_mask_scrubs_sensitive_extra_fields_but_preserves_safe_extra_fields(): assert server.model_extra["api_key"] == "real-extra-secret" +def test_mask_scrubs_sensitive_per_tool_override_extras(): + server = McpServerConfigResponse( + tools={ + "search": { + "routing": {"mode": "prefer", "priority": 70}, + "api_key": "tool-secret", + "nested": {"refreshToken": "refresh-secret", "safe": "visible"}, + } + } + ) + + masked = _mask_server_config(server) + + assert masked.tools["search"].routing.priority == 70 + assert masked.tools["search"].model_extra == { + "api_key": "***", + "nested": {"refreshToken": "***", "safe": "visible"}, + } + assert server.tools["search"].model_extra["api_key"] == "tool-secret" + + # --------------------------------------------------------------------------- # _merge_preserving_secrets # --------------------------------------------------------------------------- @@ -179,6 +249,160 @@ def test_merge_preserves_oauth_secrets_when_none(): assert merged.oauth.refresh_token == "existing-refresh" +def test_merge_round_trip_preserves_masked_oauth_extras(): + existing = McpServerConfigResponse( + oauth=McpOAuthConfigResponse( + token_url="https://auth.example.com/token", + vendor_endpoint="https://vendor.example.com/oauth", + vendor_api_key="vendor-secret", + nested={"refreshToken": "refresh-secret", "safe": "visible"}, + ), + ) + + merged = _merge_preserving_secrets( + _mask_server_config(existing), + existing, + preserve_omitted_fields=False, + ) + + assert merged.oauth is not None + assert merged.oauth.model_extra == { + "vendor_endpoint": "https://vendor.example.com/oauth", + "vendor_api_key": "vendor-secret", + "nested": {"refreshToken": "refresh-secret", "safe": "visible"}, + } + + +def test_merge_round_trip_preserves_masked_oauth_extra_token_params(): + existing = McpServerConfigResponse( + oauth=McpOAuthConfigResponse( + token_url="https://auth.example.com/token", + extra_token_params={ + "api_key": "vendor-secret", + "client_assertion": "signed-assertion", + "resource": "https://resource.example.com", + }, + ), + ) + incoming = McpServerConfigResponse( + oauth=McpOAuthConfigResponse( + token_url="https://auth.example.com/token", + extra_token_params={ + "api_key": "***", + "client_assertion": "***", + "resource": "***", + }, + ), + ) + + merged = _merge_preserving_secrets( + incoming, + existing, + preserve_omitted_fields=False, + ) + + assert merged.oauth is not None + assert merged.oauth.extra_token_params == existing.oauth.extra_token_params + + +def test_merge_targeted_oauth_replacement_removes_omitted_extras(): + existing = McpServerConfigResponse( + oauth=McpOAuthConfigResponse( + token_url="https://auth.example.com/token", + extra_token_params={"client_assertion": "remove-me"}, + vendor_api_key="vendor-secret", + vendor_note="remove-me", + ), + ) + incoming = McpServerConfigResponse( + oauth=McpOAuthConfigResponse( + token_url="https://auth.example.com/token", + vendor_api_key="***", + ), + ) + + merged = _merge_preserving_secrets( + incoming, + existing, + preserve_omitted_fields=False, + ) + + assert merged.oauth is not None + assert merged.oauth.extra_token_params == {} + assert merged.oauth.model_extra == {"vendor_api_key": "vendor-secret"} + + +def test_merge_bulk_oauth_update_preserves_omitted_extras(): + existing = McpServerConfigResponse( + oauth=McpOAuthConfigResponse( + token_url="https://auth.example.com/token", + extra_token_params={"client_assertion": "keep-me"}, + vendor_api_key="vendor-secret", + vendor_note="keep-me", + ), + ) + incoming = McpServerConfigResponse( + oauth=McpOAuthConfigResponse( + enabled=False, + token_url="https://auth.example.com/token", + ), + ) + + merged = _merge_preserving_secrets(incoming, existing) + + assert merged.oauth is not None + assert merged.oauth.enabled is False + assert merged.oauth.extra_token_params == {"client_assertion": "keep-me"} + assert merged.oauth.model_extra == { + "vendor_api_key": "vendor-secret", + "vendor_note": "keep-me", + } + + +def test_merge_rejects_masked_new_oauth_extra_token_param(): + existing = McpServerConfigResponse( + oauth=McpOAuthConfigResponse( + token_url="https://auth.example.com/token", + ), + ) + incoming = McpServerConfigResponse( + oauth=McpOAuthConfigResponse( + token_url="https://auth.example.com/token", + extra_token_params={"client_assertion": "***"}, + ), + ) + + with pytest.raises(HTTPException) as exc_info: + _merge_preserving_secrets( + incoming, + existing, + preserve_omitted_fields=False, + ) + + assert exc_info.value.status_code == 400 + + +def test_merge_rejects_structural_edits_to_masked_oauth_extra_arrays(): + existing = McpServerConfigResponse( + oauth=McpOAuthConfigResponse( + token_url="https://auth.example.com/token", + providers=[{"name": "alpha", "apiKey": "secret-alpha"}], + ), + ) + incoming = McpServerConfigResponse( + oauth=McpOAuthConfigResponse( + token_url="https://auth.example.com/token", + providers=[ + {"name": "alpha", "apiKey": "***"}, + {"name": "beta", "apiKey": "secret-beta"}, + ], + ), + ) + + with pytest.raises(HTTPException): + _merge_preserving_secrets(incoming, existing) + + def test_merge_accepts_new_secret_values(): """Incoming real secret values should replace existing ones.""" incoming = McpServerConfigResponse( @@ -250,6 +474,60 @@ def test_merge_preserves_masked_sensitive_extra_values(): assert merged.model_extra["endpoints"] == [{"access_key": "real-access", "name": "prod"}] +@pytest.mark.parametrize("operation", ["add", "remove", "reorder"]) +def test_merge_rejects_structural_edits_to_masked_sensitive_extra_arrays(operation): + """Array entries cannot be identified safely while nested secrets are masked.""" + existing = McpServerConfigResponse( + providers=[ + {"name": "alpha", "apiKey": "secret-alpha"}, + {"name": "beta", "apiKey": "secret-beta"}, + ] + ) + masked = _mask_server_config(existing) + providers = masked.model_extra["providers"] + if operation == "add": + providers = [*providers, {"name": "gamma", "apiKey": "secret-gamma"}] + elif operation == "remove": + providers = providers[:1] + else: + providers = list(reversed(providers)) + + with pytest.raises(HTTPException) as exc_info: + _merge_preserving_secrets(McpServerConfigResponse(providers=providers), existing) + + assert exc_info.value.status_code == 400 + assert "providers" in exc_info.value.detail + assert "real values" in exc_info.value.detail + + +def test_merge_allows_structural_extra_array_edits_with_real_replacement_secrets(): + """Supplying every replacement secret makes an array edit unambiguous.""" + existing = McpServerConfigResponse( + providers=[ + {"name": "alpha", "apiKey": "secret-alpha"}, + {"name": "beta", "apiKey": "secret-beta"}, + ] + ) + incoming_providers = [ + {"name": "beta", "apiKey": "replacement-beta"}, + {"name": "gamma", "apiKey": "secret-gamma"}, + ] + + merged = _merge_preserving_secrets(McpServerConfigResponse(providers=incoming_providers), existing) + + assert merged.model_extra["providers"] == incoming_providers + + +def test_merge_allows_structural_edits_to_non_sensitive_extra_arrays(): + """Secret-free advanced arrays remain fully editable.""" + existing = McpServerConfigResponse(routes=[{"name": "alpha"}]) + incoming_routes = [{"name": "beta"}, {"name": "gamma"}] + + merged = _merge_preserving_secrets(McpServerConfigResponse(routes=incoming_routes), existing) + + assert merged.model_extra["routes"] == incoming_routes + + def test_merge_rejects_masked_sensitive_extra_value_for_new_key(): """A new unknown secret field must provide a real value, not a mask.""" incoming = McpServerConfigResponse(api_key="***") @@ -262,6 +540,51 @@ def test_merge_rejects_masked_sensitive_extra_value_for_new_key(): assert "api_key" in exc_info.value.detail +def test_merge_round_trip_preserves_masked_per_tool_override_extras(): + existing = McpServerConfigResponse( + tools={ + "search": { + "routing": {"mode": "prefer", "priority": 40}, + "api_key": "tool-secret", + "nested": {"refreshToken": "refresh-secret", "safe": "visible"}, + } + } + ) + incoming = _mask_server_config(existing) + incoming.tools["search"].routing.priority = 80 + + merged = _merge_preserving_secrets( + incoming, + existing, + preserve_omitted_fields=False, + ) + + assert merged.tools["search"].routing.priority == 80 + assert merged.tools["search"].model_extra == { + "api_key": "tool-secret", + "nested": {"refreshToken": "refresh-secret", "safe": "visible"}, + } + + +def test_merge_rejects_masked_per_tool_override_secret_for_new_key(): + incoming = McpServerConfigResponse( + tools={"search": {"api_key": "***"}}, + ) + existing = McpServerConfigResponse( + tools={"search": {"routing": {"priority": 20}}}, + ) + + with pytest.raises(HTTPException) as exc_info: + _merge_preserving_secrets( + incoming, + existing, + preserve_omitted_fields=False, + ) + + assert exc_info.value.status_code == 400 + assert "api_key" in exc_info.value.detail + + # --------------------------------------------------------------------------- # Comment 2 fix: masked value for new key is rejected # --------------------------------------------------------------------------- @@ -593,6 +916,1200 @@ async def test_update_mcp_configuration_preserves_server_extra_fields(monkeypatc assert response.mcp_servers["playwright"].model_extra["api_key"] == "***" +@pytest.mark.asyncio +async def test_create_mcp_servers_preserves_concurrent_siblings_and_rejects_duplicates(monkeypatch, tmp_path): + config_path = tmp_path / "extensions_config.json" + original = { + "mcpServers": { + "sibling": { + "enabled": True, + "type": "http", + "url": "https://changed-in-another-tab.example/mcp", + } + }, + "skills": {"research": {"enabled": False}}, + "customTopLevel": {"preserve": True}, + } + config_path.write_text(json.dumps(original), encoding="utf-8") + reset_calls = 0 + + def fake_reload_extensions_config(): + return ExtensionsConfig.model_validate(json.loads(config_path.read_text(encoding="utf-8"))) + + def fake_reset_mcp_tools_cache(): + nonlocal reset_calls + reset_calls += 1 + + monkeypatch.setattr(mcp_router.ExtensionsConfig, "resolve_config_path", lambda: config_path) + monkeypatch.setattr(mcp_router, "reload_extensions_config", fake_reload_extensions_config) + monkeypatch.setattr(mcp_router, "reset_mcp_tools_cache", fake_reset_mcp_tools_cache) + + response = await create_mcp_servers( + _request_with_role("admin"), + McpConfigUpdateRequest( + mcp_servers={ + "added": McpServerConfigResponse( + command="npx", + args=["-y", "@example/mcp"], + ) + } + ), + ) + + persisted = json.loads(config_path.read_text(encoding="utf-8")) + assert persisted["mcpServers"]["sibling"] == original["mcpServers"]["sibling"] + assert persisted["customTopLevel"] == original["customTopLevel"] + assert response.mcp_servers["added"].command == "npx" + assert reset_calls == 1 + + before_duplicate = config_path.read_text(encoding="utf-8") + with pytest.raises(HTTPException) as exc_info: + await create_mcp_servers( + _request_with_role("admin"), + McpConfigUpdateRequest( + mcp_servers={ + "added": McpServerConfigResponse(command="npx"), + "never-written": McpServerConfigResponse(command="uvx"), + } + ), + ) + + assert exc_info.value.status_code == 409 + assert config_path.read_text(encoding="utf-8") == before_duplicate + assert reset_calls == 1 + + +@pytest.mark.asyncio +async def test_update_mcp_server_preserves_latest_sibling_and_masked_secret(monkeypatch, tmp_path): + config_path = tmp_path / "extensions_config.json" + original = { + "mcpServers": { + "target": { + "enabled": True, + "type": "http", + "url": "https://old.example/mcp", + "headers": {"Authorization": "Bearer real-secret"}, + }, + "sibling": { + "enabled": False, + "command": "uvx", + "args": ["changed-by-another-tab"], + }, + }, + "skills": {}, + } + config_path.write_text(json.dumps(original), encoding="utf-8") + + def fake_reload_extensions_config(): + return ExtensionsConfig.model_validate(json.loads(config_path.read_text(encoding="utf-8"))) + + monkeypatch.setattr(mcp_router.ExtensionsConfig, "resolve_config_path", lambda: config_path) + monkeypatch.setattr(mcp_router, "reload_extensions_config", fake_reload_extensions_config) + monkeypatch.setattr(mcp_router, "reset_mcp_tools_cache", lambda: None) + + response = await update_mcp_server( + _request_with_role("admin"), + McpServerConfigUpdateRequest( + server_name="target", + server=McpServerConfigResponse( + enabled=False, + type="http", + url="https://new.example/mcp", + headers={"Authorization": "***"}, + ), + ), + ) + + persisted = json.loads(config_path.read_text(encoding="utf-8")) + assert persisted["mcpServers"]["sibling"] == original["mcpServers"]["sibling"] + assert persisted["mcpServers"]["target"]["url"] == "https://new.example/mcp" + assert persisted["mcpServers"]["target"]["headers"]["Authorization"] == "Bearer real-secret" + assert response.mcp_servers["target"].headers["Authorization"] == "***" + + +@pytest.mark.asyncio +async def test_update_mcp_server_masks_and_restores_per_tool_override_secrets(monkeypatch, tmp_path): + config_path = tmp_path / "extensions_config.json" + original = { + "mcpServers": { + "target": { + "enabled": True, + "type": "http", + "url": "https://example.com/mcp", + "tools": { + "search": { + "routing": {"mode": "prefer", "priority": 40}, + "api_key": "tool-secret", + "nested": {"refreshToken": "refresh-secret", "safe": "visible"}, + } + }, + } + }, + "skills": {}, + } + config_path.write_text(json.dumps(original), encoding="utf-8") + + def fake_reload_extensions_config(): + return ExtensionsConfig.model_validate(json.loads(config_path.read_text(encoding="utf-8"))) + + monkeypatch.setattr(mcp_router.ExtensionsConfig, "resolve_config_path", lambda: config_path) + monkeypatch.setattr(mcp_router, "reload_extensions_config", fake_reload_extensions_config) + monkeypatch.setattr(mcp_router, "reset_mcp_tools_cache", lambda: None) + + response = await get_mcp_configuration(_request_with_role("admin")) + masked_server = response.mcp_servers["target"] + assert masked_server.tools["search"].model_extra == { + "api_key": "***", + "nested": {"refreshToken": "***", "safe": "visible"}, + } + masked_server.tools["search"].routing.priority = 80 + + updated = await update_mcp_server( + _request_with_role("admin"), + McpServerConfigUpdateRequest(server_name="target", server=masked_server), + ) + + persisted = json.loads(config_path.read_text(encoding="utf-8")) + assert persisted["mcpServers"]["target"]["tools"]["search"] == { + "routing": {"mode": "prefer", "priority": 80, "keywords": []}, + "api_key": "tool-secret", + "nested": {"refreshToken": "refresh-secret", "safe": "visible"}, + } + assert updated.mcp_servers["target"].tools["search"].model_extra["api_key"] == "***" + + +@pytest.mark.asyncio +async def test_update_mcp_server_rejects_new_masked_per_tool_override_secret_without_writing(monkeypatch, tmp_path): + config_path = tmp_path / "extensions_config.json" + original = { + "mcpServers": { + "target": { + "enabled": True, + "type": "http", + "url": "https://example.com/mcp", + "tools": {"search": {"routing": {"priority": 20}}}, + } + }, + "skills": {}, + } + config_path.write_text(json.dumps(original), encoding="utf-8") + + monkeypatch.setattr(mcp_router.ExtensionsConfig, "resolve_config_path", lambda: config_path) + monkeypatch.setattr(mcp_router, "reload_extensions_config", lambda: None) + monkeypatch.setattr(mcp_router, "reset_mcp_tools_cache", lambda: None) + + with pytest.raises(HTTPException) as exc_info: + await update_mcp_server( + _request_with_role("admin"), + McpServerConfigUpdateRequest( + server_name="target", + server=McpServerConfigResponse( + type="http", + url="https://example.com/mcp", + tools={"search": {"api_key": "***"}}, + ), + ), + ) + + assert exc_info.value.status_code == 400 + assert "api_key" in exc_info.value.detail + assert json.loads(config_path.read_text(encoding="utf-8")) == original + + +@pytest.mark.asyncio +async def test_update_mcp_server_honors_deletions_in_complete_replacement(monkeypatch, tmp_path): + config_path = tmp_path / "extensions_config.json" + original_sibling = { + "enabled": False, + "command": "uvx", + "args": ["changed-by-another-tab"], + } + config_path.write_text( + json.dumps( + { + "mcpServers": { + "target": { + "enabled": True, + "type": "http", + "url": "https://old.example/mcp", + "headers": {"Authorization": "Bearer real-secret"}, + "api_key": "real-extra-secret", + "custom_note": "remove-me", + "routing": { + "mode": "prefer", + "priority": 80, + "keywords": ["legacy"], + }, + "tools": { + "search": { + "routing": {"priority": 90}, + } + }, + "user_auth": { + "users": {"u1": "Bearer user-secret"}, + }, + "headers_from_context": { + "enabled": True, + "headers": {"X-Tenant": "tenant_id"}, + "on_missing": "passthrough", + }, + }, + "sibling": original_sibling, + }, + "skills": {}, + } + ), + encoding="utf-8", + ) + + def fake_reload_extensions_config(): + return ExtensionsConfig.model_validate(json.loads(config_path.read_text(encoding="utf-8"))) + + monkeypatch.setattr(mcp_router.ExtensionsConfig, "resolve_config_path", lambda: config_path) + monkeypatch.setattr(mcp_router, "reload_extensions_config", fake_reload_extensions_config) + monkeypatch.setattr(mcp_router, "reset_mcp_tools_cache", lambda: None) + + await update_mcp_server( + _request_with_role("admin"), + McpServerConfigUpdateRequest( + server_name="target", + server=McpServerConfigResponse( + enabled=True, + type="http", + url="https://new.example/mcp", + headers={"Authorization": "***"}, + api_key="***", + ), + ), + ) + + persisted = json.loads(config_path.read_text(encoding="utf-8")) + target = persisted["mcpServers"]["target"] + assert persisted["mcpServers"]["sibling"] == original_sibling + assert target["headers"]["Authorization"] == "Bearer real-secret" + assert target["api_key"] == "real-extra-secret" + assert "custom_note" not in target + assert target["user_auth"] is None + assert target["headers_from_context"] is None + assert target["routing"] == McpServerConfigResponse().routing.model_dump() + assert target["tools"] == {} + + +@pytest.mark.asyncio +async def test_update_mcp_server_rejects_masked_array_structural_edit_without_writing(monkeypatch, tmp_path): + config_path = tmp_path / "extensions_config.json" + original = { + "mcpServers": { + "target": { + "enabled": True, + "type": "http", + "url": "https://old.example/mcp", + "providers": [ + {"name": "alpha", "apiKey": "secret-alpha"}, + {"name": "beta", "apiKey": "secret-beta"}, + ], + } + }, + "skills": {}, + } + config_path.write_text(json.dumps(original), encoding="utf-8") + + def fake_reload_extensions_config(): + return ExtensionsConfig.model_validate(json.loads(config_path.read_text(encoding="utf-8"))) + + monkeypatch.setattr(mcp_router.ExtensionsConfig, "resolve_config_path", lambda: config_path) + monkeypatch.setattr(mcp_router, "reload_extensions_config", fake_reload_extensions_config) + monkeypatch.setattr(mcp_router, "reset_mcp_tools_cache", lambda: None) + + with pytest.raises(HTTPException) as exc_info: + await update_mcp_server( + _request_with_role("admin"), + McpServerConfigUpdateRequest( + server_name="target", + server=McpServerConfigResponse( + enabled=True, + type="http", + url="https://new.example/mcp", + providers=[ + {"name": "alpha", "apiKey": "***"}, + {"name": "beta", "apiKey": "***"}, + {"name": "gamma", "apiKey": "secret-gamma"}, + ], + ), + ), + ) + + assert exc_info.value.status_code == 400 + assert json.loads(config_path.read_text(encoding="utf-8")) == original + + +@pytest.mark.asyncio +async def test_create_mcp_servers_rejects_masked_secret_sentinel_without_writing(monkeypatch, tmp_path): + config_path = tmp_path / "extensions_config.json" + original = {"mcpServers": {}, "skills": {}} + config_path.write_text(json.dumps(original), encoding="utf-8") + + def fake_reload_extensions_config(): + return ExtensionsConfig.model_validate(json.loads(config_path.read_text(encoding="utf-8"))) + + monkeypatch.setattr(mcp_router.ExtensionsConfig, "resolve_config_path", lambda: config_path) + monkeypatch.setattr(mcp_router, "reload_extensions_config", fake_reload_extensions_config) + monkeypatch.setattr(mcp_router, "reset_mcp_tools_cache", lambda: None) + + with pytest.raises(HTTPException) as exc_info: + await create_mcp_servers( + _request_with_role("admin"), + McpConfigUpdateRequest( + mcp_servers={ + "added": McpServerConfigResponse( + type="http", + url="https://new.example/mcp", + providers=[{"name": "alpha", "apiKey": "***"}], + ) + } + ), + ) + + assert exc_info.value.status_code == 400 + assert json.loads(config_path.read_text(encoding="utf-8")) == original + + +@pytest.mark.asyncio +async def test_bulk_update_rejects_masked_secret_for_new_server_without_writing(monkeypatch, tmp_path): + config_path = tmp_path / "extensions_config.json" + original = {"mcpServers": {}, "skills": {}} + config_path.write_text(json.dumps(original), encoding="utf-8") + + def fake_reload_extensions_config(): + return ExtensionsConfig.model_validate(json.loads(config_path.read_text(encoding="utf-8"))) + + monkeypatch.setattr(mcp_router.ExtensionsConfig, "resolve_config_path", lambda: config_path) + monkeypatch.setattr(mcp_router, "reload_extensions_config", fake_reload_extensions_config) + monkeypatch.setattr(mcp_router, "reset_mcp_tools_cache", lambda: None) + + with pytest.raises(HTTPException) as exc_info: + await update_mcp_configuration( + _request_with_role("admin"), + McpConfigUpdateRequest( + mcp_servers={ + "added": McpServerConfigResponse( + type="http", + url="https://new.example/mcp", + providers=[{"name": "alpha", "apiKey": "***"}], + ) + } + ), + ) + + assert exc_info.value.status_code == 400 + assert json.loads(config_path.read_text(encoding="utf-8")) == original + + +@pytest.mark.asyncio +@pytest.mark.parametrize("handler", [create_mcp_servers, update_mcp_configuration]) +async def test_new_server_writes_reject_masked_headers_from_context_extra_without_writing(handler, monkeypatch, tmp_path): + config_path = tmp_path / "extensions_config.json" + original = {"mcpServers": {}, "skills": {}} + config_path.write_text(json.dumps(original), encoding="utf-8") + + def fake_reload_extensions_config(): + return ExtensionsConfig.model_validate(json.loads(config_path.read_text(encoding="utf-8"))) + + monkeypatch.setattr(mcp_router.ExtensionsConfig, "resolve_config_path", lambda: config_path) + monkeypatch.setattr(mcp_router, "reload_extensions_config", fake_reload_extensions_config) + monkeypatch.setattr(mcp_router, "reset_mcp_tools_cache", lambda: None) + + with pytest.raises(HTTPException) as exc_info: + await handler( + _request_with_role("admin"), + McpConfigUpdateRequest( + mcp_servers={ + "added": McpServerConfigResponse( + type="http", + url="https://new.example/mcp", + headers_from_context={"api_key": "***"}, + ) + } + ), + ) + + assert exc_info.value.status_code == 400 + assert json.loads(config_path.read_text(encoding="utf-8")) == original + + +@pytest.mark.asyncio +@pytest.mark.parametrize("handler", [create_mcp_servers, update_mcp_configuration]) +async def test_new_server_writes_reject_masked_per_tool_override_secret_without_writing(handler, monkeypatch, tmp_path): + config_path = tmp_path / "extensions_config.json" + original = {"mcpServers": {}, "skills": {}} + config_path.write_text(json.dumps(original), encoding="utf-8") + + monkeypatch.setattr(mcp_router.ExtensionsConfig, "resolve_config_path", lambda: config_path) + monkeypatch.setattr(mcp_router, "reload_extensions_config", lambda: None) + monkeypatch.setattr(mcp_router, "reset_mcp_tools_cache", lambda: None) + + with pytest.raises(HTTPException) as exc_info: + await handler( + _request_with_role("admin"), + McpConfigUpdateRequest( + mcp_servers={ + "added": McpServerConfigResponse( + type="http", + url="https://new.example/mcp", + tools={"search": {"api_key": "***"}}, + ) + } + ), + ) + + assert exc_info.value.status_code == 400 + assert "api_key" in exc_info.value.detail + assert json.loads(config_path.read_text(encoding="utf-8")) == original + + +@pytest.mark.asyncio +@pytest.mark.parametrize("handler", [create_mcp_servers, update_mcp_configuration]) +@pytest.mark.parametrize( + "oauth_fields", + [ + {"client_secret": "***"}, + {"refresh_token": "***"}, + {"vendor_api_key": "***"}, + {"extra_token_params": {"api_key": "***"}}, + {"extra_token_params": {"client_assertion": "***"}}, + ], +) +async def test_new_server_writes_reject_masked_oauth_secret_without_writing(handler, oauth_fields, monkeypatch, tmp_path): + config_path = tmp_path / "extensions_config.json" + original = {"mcpServers": {}, "skills": {}} + config_path.write_text(json.dumps(original), encoding="utf-8") + + def fake_reload_extensions_config(): + return ExtensionsConfig.model_validate(json.loads(config_path.read_text(encoding="utf-8"))) + + monkeypatch.setattr(mcp_router.ExtensionsConfig, "resolve_config_path", lambda: config_path) + monkeypatch.setattr(mcp_router, "reload_extensions_config", fake_reload_extensions_config) + monkeypatch.setattr(mcp_router, "reset_mcp_tools_cache", lambda: None) + + with pytest.raises(HTTPException) as exc_info: + await handler( + _request_with_role("admin"), + McpConfigUpdateRequest( + mcp_servers={ + "added": McpServerConfigResponse( + type="http", + url="https://new.example/mcp", + oauth=McpOAuthConfigResponse( + token_url="https://auth.example.com/token", + **oauth_fields, + ), + ) + } + ), + ) + + assert exc_info.value.status_code == 400 + assert json.loads(config_path.read_text(encoding="utf-8")) == original + + +@pytest.mark.asyncio +@pytest.mark.parametrize("operation", ["create", "bulk", "targeted"]) +async def test_mcp_writes_validate_runtime_server_constraints_before_writing(operation, monkeypatch, tmp_path): + config_path = tmp_path / "extensions_config.json" + original = { + "mcpServers": { + "target": { + "type": "http", + "url": "https://old.example/mcp", + "task_toolsets": [ + { + "name": "existing", + "submit_tool": "submit_existing", + "status_tool": "status_existing", + "cancel_tool": "cancel_existing", + } + ], + } + }, + "skills": {"research": {"enabled": False}}, + } + config_path.write_text(json.dumps(original), encoding="utf-8") + + def load_config(): + return ExtensionsConfig.model_validate(json.loads(config_path.read_text(encoding="utf-8"))) + + monkeypatch.setattr(mcp_router.ExtensionsConfig, "resolve_config_path", lambda: config_path) + monkeypatch.setattr(mcp_router, "get_extensions_config", load_config) + monkeypatch.setattr(mcp_router, "reload_extensions_config", load_config) + monkeypatch.setattr(mcp_router, "reset_mcp_tools_cache", lambda: None) + + invalid_server = McpServerConfigResponse( + type="http", + url="https://new.example/mcp", + task_toolsets=[ + { + "name": "first", + "submit_tool": "submit", + "status_tool": "status_first", + "cancel_tool": "cancel_first", + }, + { + "name": "second", + "submit_tool": "submit", + "status_tool": "status_second", + "cancel_tool": "cancel_second", + }, + ], + ) + + with pytest.raises(HTTPException) as exc_info: + if operation == "create": + await create_mcp_servers( + _request_with_role("admin"), + McpConfigUpdateRequest(mcp_servers={"added": invalid_server}), + ) + elif operation == "bulk": + await update_mcp_configuration( + _request_with_role("admin"), + McpConfigUpdateRequest(mcp_servers={"target": invalid_server}), + ) + else: + await update_mcp_server( + _request_with_role("admin"), + McpServerConfigUpdateRequest(server_name="target", server=invalid_server), + ) + + assert exc_info.value.status_code == 400 + assert "must be unique" in exc_info.value.detail + assert json.loads(config_path.read_text(encoding="utf-8")) == original + + +@pytest.mark.asyncio +@pytest.mark.parametrize("handler", [create_mcp_servers, update_mcp_configuration]) +async def test_new_server_writes_validate_extensions_constraints_before_writing(handler, monkeypatch, tmp_path): + config_path = tmp_path / "extensions_config.json" + original = {"mcpServers": {}, "skills": {}} + config_path.write_text(json.dumps(original), encoding="utf-8") + + def load_config(): + return ExtensionsConfig.model_validate(json.loads(config_path.read_text(encoding="utf-8"))) + + monkeypatch.setattr(mcp_router.ExtensionsConfig, "resolve_config_path", lambda: config_path) + monkeypatch.setattr(mcp_router, "get_extensions_config", load_config) + monkeypatch.setattr(mcp_router, "reload_extensions_config", load_config) + monkeypatch.setattr(mcp_router, "reset_mcp_tools_cache", lambda: None) + + with pytest.raises(HTTPException) as exc_info: + await handler( + _request_with_role("admin"), + McpConfigUpdateRequest( + mcp_servers={ + "": McpServerConfigResponse( + type="http", + url="https://new.example/mcp", + task_toolsets=[ + { + "name": "task", + "submit_tool": "submit", + "status_tool": "status", + "cancel_tool": "cancel", + } + ], + ) + } + ), + ) + + assert exc_info.value.status_code == 400 + assert "server name" in exc_info.value.detail + assert json.loads(config_path.read_text(encoding="utf-8")) == original + + +def test_candidate_validation_uses_environment_expanded_values(monkeypatch): + monkeypatch.setenv("CODEX_PR_5022_ON_MISSING", "passthrough") + raw_data = { + "mcpServers": { + "target": { + "type": "http", + "url": "https://example.invalid/mcp", + "user_auth": { + "on_missing": "$CODEX_PR_5022_ON_MISSING", + }, + } + }, + "skills": {}, + } + + _validate_extensions_config_candidate(raw_data) + + assert raw_data["mcpServers"]["target"]["user_auth"]["on_missing"] == "$CODEX_PR_5022_ON_MISSING" + + +@pytest.mark.asyncio +@pytest.mark.parametrize("operation", ["create", "bulk", "targeted"]) +async def test_mcp_writes_validate_environment_expanded_candidate_before_writing(operation, monkeypatch, tmp_path): + config_path = tmp_path / "extensions_config.json" + original = { + "mcpServers": { + "target": { + "type": "http", + "url": "https://old.example/mcp", + } + }, + "skills": {"research": {"enabled": False}}, + } + config_path.write_text(json.dumps(original), encoding="utf-8") + monkeypatch.delenv("CODEX_PR_5022_UNSET_TOOLSET", raising=False) + + def load_config_like_production(): + return ExtensionsConfig.from_file(str(config_path)) + + monkeypatch.setattr(mcp_router.ExtensionsConfig, "resolve_config_path", lambda _config_path=None: config_path) + monkeypatch.setattr(mcp_router, "get_extensions_config", load_config_like_production) + monkeypatch.setattr(mcp_router, "reload_extensions_config", load_config_like_production) + monkeypatch.setattr(mcp_router, "reset_mcp_tools_cache", lambda: None) + + invalid_after_expansion = McpServerConfigResponse( + type="http", + url="https://new.example/mcp", + task_toolsets=[ + { + "name": "$CODEX_PR_5022_UNSET_TOOLSET", + "submit_tool": "submit", + "status_tool": "status", + "cancel_tool": "cancel", + } + ], + ) + + with pytest.raises(HTTPException) as exc_info: + if operation == "create": + await create_mcp_servers( + _request_with_role("admin"), + McpConfigUpdateRequest(mcp_servers={"added": invalid_after_expansion}), + ) + elif operation == "bulk": + await update_mcp_configuration( + _request_with_role("admin"), + McpConfigUpdateRequest(mcp_servers={"target": invalid_after_expansion}), + ) + else: + await update_mcp_server( + _request_with_role("admin"), + McpServerConfigUpdateRequest(server_name="target", server=invalid_after_expansion), + ) + + assert exc_info.value.status_code == 400 + assert "at least 1 character" in exc_info.value.detail + assert json.loads(config_path.read_text(encoding="utf-8")) == original + + +@pytest.mark.asyncio +@pytest.mark.parametrize("operation", ["state", "delete"]) +async def test_state_and_delete_validate_expanded_document_before_writing(operation, monkeypatch, tmp_path): + config_path = tmp_path / "extensions_config.json" + original = { + "mcpServers": { + "target": { + "enabled": True, + "type": "http", + "url": "https://target.example/mcp", + }, + "invalid-sibling": { + "type": "http", + "url": "https://sibling.example/mcp", + "task_toolsets": [ + { + "name": "$CODEX_PR_5022_UNSET_SIBLING_TOOLSET", + "submit_tool": "submit", + "status_tool": "status", + "cancel_tool": "cancel", + } + ], + }, + }, + "skills": {}, + } + config_path.write_text(json.dumps(original), encoding="utf-8") + monkeypatch.delenv("CODEX_PR_5022_UNSET_SIBLING_TOOLSET", raising=False) + monkeypatch.setattr(mcp_router.ExtensionsConfig, "resolve_config_path", lambda _config_path=None: config_path) + monkeypatch.setattr(mcp_router, "reload_extensions_config", lambda: ExtensionsConfig.from_file(str(config_path))) + monkeypatch.setattr(mcp_router, "reset_mcp_tools_cache", lambda: None) + + with pytest.raises(HTTPException) as exc_info: + if operation == "state": + await update_mcp_server_state( + _request_with_role("admin"), + McpServerStateUpdateRequest(server_name="target", enabled=False), + ) + else: + await delete_mcp_server( + _request_with_role("admin"), + "target", + ) + + assert exc_info.value.status_code == 400 + assert "at least 1 character" in exc_info.value.detail + assert json.loads(config_path.read_text(encoding="utf-8")) == original + + +@pytest.mark.asyncio +async def test_get_and_targeted_put_preserve_environment_placeholders_outside_secret_fields(monkeypatch, tmp_path): + config_path = tmp_path / "extensions_config.json" + placeholder = "$CODEX_PR_5022_EDITOR_TOKEN" + original = { + "mcpServers": { + "target": { + "type": "stdio", + "command": "npx", + "args": ["--api-key", placeholder], + "env": {"MCP_TOKEN": placeholder}, + "provider_note": placeholder, + } + }, + "skills": {}, + } + config_path.write_text(json.dumps(original), encoding="utf-8") + monkeypatch.setenv("CODEX_PR_5022_EDITOR_TOKEN", "resolved-editor-secret") + + def load_config_like_production(): + return ExtensionsConfig.from_file(str(config_path)) + + monkeypatch.setattr(mcp_router.ExtensionsConfig, "resolve_config_path", lambda _config_path=None: config_path) + monkeypatch.setattr(mcp_router, "get_extensions_config", load_config_like_production) + monkeypatch.setattr(mcp_router, "reload_extensions_config", load_config_like_production) + monkeypatch.setattr(mcp_router, "reset_mcp_tools_cache", lambda: None) + + get_response = await get_mcp_configuration(_request_with_role("admin")) + editable = get_response.mcp_servers["target"] + assert editable.args == ["--api-key", placeholder] + assert editable.env == {"MCP_TOKEN": "***"} + assert editable.model_extra == {"provider_note": placeholder} + + update_response = await update_mcp_server( + _request_with_role("admin"), + McpServerConfigUpdateRequest(server_name="target", server=editable), + ) + + returned = update_response.mcp_servers["target"] + assert returned.args == ["--api-key", placeholder] + assert returned.env == {"MCP_TOKEN": "***"} + assert returned.model_extra == {"provider_note": placeholder} + persisted = json.loads(config_path.read_text(encoding="utf-8"))["mcpServers"]["target"] + assert persisted["args"] == ["--api-key", placeholder] + assert persisted["env"] == {"MCP_TOKEN": placeholder} + assert persisted["provider_note"] == placeholder + + +@pytest.mark.asyncio +async def test_targeted_oauth_extra_round_trip_preserves_extensions_and_secrets(monkeypatch, tmp_path): + config_path = tmp_path / "extensions_config.json" + original = { + "mcpServers": { + "target": { + "enabled": True, + "type": "http", + "url": "https://old.example/mcp", + "oauth": { + "token_url": "https://auth.example.com/token", + "extra_token_params": { + "client_assertion": "signed-assertion", + "resource": "https://resource.example.com", + }, + "vendor_endpoint": "https://vendor.example.com/oauth", + "vendor_api_key": "vendor-secret", + "nested": {"refreshToken": "refresh-secret", "safe": "visible"}, + }, + } + }, + "skills": {}, + } + config_path.write_text(json.dumps(original), encoding="utf-8") + + def load_config(): + return ExtensionsConfig.model_validate(json.loads(config_path.read_text(encoding="utf-8"))) + + monkeypatch.setattr(mcp_router.ExtensionsConfig, "resolve_config_path", lambda: config_path) + monkeypatch.setattr(mcp_router, "get_extensions_config", load_config) + monkeypatch.setattr(mcp_router, "reload_extensions_config", load_config) + monkeypatch.setattr(mcp_router, "reset_mcp_tools_cache", lambda: None) + + get_response = await get_mcp_configuration(_request_with_role("admin")) + masked = get_response.mcp_servers["target"] + assert masked.oauth is not None + assert masked.oauth.extra_token_params == { + "client_assertion": "***", + "resource": "***", + } + assert masked.oauth.model_extra == { + "vendor_endpoint": "https://vendor.example.com/oauth", + "vendor_api_key": "***", + "nested": {"refreshToken": "***", "safe": "visible"}, + } + + await update_mcp_server( + _request_with_role("admin"), + McpServerConfigUpdateRequest( + server_name="target", + server=masked.model_copy(update={"url": "https://new.example/mcp"}), + ), + ) + + persisted = json.loads(config_path.read_text(encoding="utf-8")) + target = persisted["mcpServers"]["target"] + assert target["url"] == "https://new.example/mcp" + assert target["oauth"]["vendor_endpoint"] == "https://vendor.example.com/oauth" + assert target["oauth"]["vendor_api_key"] == "vendor-secret" + assert target["oauth"]["extra_token_params"] == { + "client_assertion": "signed-assertion", + "resource": "https://resource.example.com", + } + assert target["oauth"]["nested"] == { + "refreshToken": "refresh-secret", + "safe": "visible", + } + + +@pytest.mark.asyncio +async def test_delete_mcp_server_accepts_empty_name_and_preserves_siblings(monkeypatch, tmp_path): + config_path = tmp_path / "extensions_config.json" + original_sibling = { + "enabled": True, + "type": "http", + "url": "https://changed-in-another-tab.example/mcp", + } + config_path.write_text( + json.dumps( + { + "mcpServers": { + "": {"enabled": False, "command": "npx"}, + "sibling": original_sibling, + }, + "skills": {}, + } + ), + encoding="utf-8", + ) + + def fake_reload_extensions_config(): + return ExtensionsConfig.model_validate(json.loads(config_path.read_text(encoding="utf-8"))) + + monkeypatch.setattr(mcp_router.ExtensionsConfig, "resolve_config_path", lambda: config_path) + monkeypatch.setattr(mcp_router, "reload_extensions_config", fake_reload_extensions_config) + monkeypatch.setattr(mcp_router, "reset_mcp_tools_cache", lambda: None) + + response = await delete_mcp_server( + _request_with_role("admin"), + "", + ) + + persisted = json.loads(config_path.read_text(encoding="utf-8")) + assert persisted["mcpServers"] == {"sibling": original_sibling} + assert set(response.mcp_servers) == {"sibling"} + + +@pytest.mark.asyncio +@pytest.mark.parametrize("operation", ["create", "bulk"]) +async def test_mcp_create_without_existing_config_uses_resolvable_project_root(operation, monkeypatch, tmp_path): + project_dir = tmp_path / "project" + working_dir = tmp_path / "runtime" / "nested" + project_dir.mkdir() + working_dir.mkdir(parents=True) + expected_path = project_dir / "extensions_config.json" + + def resolve_created_config(_config_path=None): + return expected_path if expected_path.exists() else None + + monkeypatch.chdir(working_dir) + monkeypatch.setattr(mcp_router, "project_root", lambda: project_dir, raising=False) + monkeypatch.setattr(mcp_router.ExtensionsConfig, "resolve_config_path", resolve_created_config) + monkeypatch.setattr(mcp_router, "get_extensions_config", lambda: SimpleNamespace(skills={})) + monkeypatch.setattr(mcp_router, "reload_extensions_config", lambda: ExtensionsConfig.from_file(str(expected_path))) + monkeypatch.setattr(mcp_router, "reset_mcp_tools_cache", lambda: None) + + body = McpConfigUpdateRequest(mcp_servers={"added": McpServerConfigResponse(command="npx")}) + if operation == "create": + response = await create_mcp_servers(_request_with_role("admin"), body) + else: + response = await update_mcp_configuration(_request_with_role("admin"), body) + + assert expected_path.is_file() + assert resolve_created_config() == expected_path + assert set(response.mcp_servers) == {"added"} + assert not (working_dir.parent / "extensions_config.json").exists() + + +@pytest.mark.asyncio +@pytest.mark.parametrize("endpoint", ["get", "bulk", "create", "update", "delete", "state"]) +@pytest.mark.parametrize( + ("raw_config", "expected_detail"), + [ + ("{not-json", "not valid JSON"), + ("[]", "Extensions configuration must be a JSON object"), + ('{"mcpServers": [], "skills": {}}', "`mcpServers` must be a JSON object"), + ], +) +async def test_mcp_config_endpoints_map_invalid_operator_document_to_400(endpoint, raw_config, expected_detail, monkeypatch, tmp_path): + config_path = tmp_path / "extensions_config.json" + config_path.write_text(raw_config, encoding="utf-8") + monkeypatch.setattr(mcp_router.ExtensionsConfig, "resolve_config_path", lambda _config_path=None: config_path) + monkeypatch.setattr(mcp_router, "reload_extensions_config", lambda: None) + monkeypatch.setattr(mcp_router, "reset_mcp_tools_cache", lambda: None) + + request = _request_with_role("admin") + with pytest.raises(HTTPException) as exc_info: + if endpoint == "get": + await get_mcp_configuration(request) + elif endpoint == "bulk": + await update_mcp_configuration( + request, + McpConfigUpdateRequest(mcp_servers={"target": McpServerConfigResponse(command="npx")}), + ) + elif endpoint == "create": + await create_mcp_servers( + request, + McpConfigUpdateRequest(mcp_servers={"added": McpServerConfigResponse(command="npx")}), + ) + elif endpoint == "update": + await update_mcp_server( + request, + McpServerConfigUpdateRequest( + server_name="target", + server=McpServerConfigResponse(command="npx"), + ), + ) + elif endpoint == "delete": + await delete_mcp_server(request, "target") + else: + await update_mcp_server_state( + request, + McpServerStateUpdateRequest(server_name="target", enabled=False), + ) + + assert exc_info.value.status_code == 400 + assert exc_info.value.detail.startswith("Invalid MCP configuration: ") + assert expected_detail in exc_info.value.detail + assert config_path.read_text(encoding="utf-8") == raw_config + + +@pytest.mark.asyncio +@pytest.mark.parametrize("endpoint", ["get", "update", "state"]) +async def test_mcp_config_endpoints_map_invalid_stored_server_to_400(endpoint, monkeypatch, tmp_path): + config_path = tmp_path / "extensions_config.json" + raw_config = '{"mcpServers": {"target": "not-an-object"}, "skills": {}}' + config_path.write_text(raw_config, encoding="utf-8") + monkeypatch.setattr(mcp_router.ExtensionsConfig, "resolve_config_path", lambda _config_path=None: config_path) + monkeypatch.setattr(mcp_router, "reload_extensions_config", lambda: None) + monkeypatch.setattr(mcp_router, "reset_mcp_tools_cache", lambda: None) + + request = _request_with_role("admin") + with pytest.raises(HTTPException) as exc_info: + if endpoint == "get": + await get_mcp_configuration(request) + elif endpoint == "update": + await update_mcp_server( + request, + McpServerConfigUpdateRequest( + server_name="target", + server=McpServerConfigResponse(command="npx"), + ), + ) + else: + await update_mcp_server_state( + request, + McpServerStateUpdateRequest(server_name="target", enabled=False), + ) + + assert exc_info.value.status_code == 400 + assert exc_info.value.detail.startswith("Invalid MCP configuration: mcpServers.target: ") + assert config_path.read_text(encoding="utf-8") == raw_config + + +@pytest.mark.parametrize( + ("server_name", "request_path"), + [ + ("", "/api/mcp/config/servers/"), + ("team/tools", "/api/mcp/config/servers/team%2Ftools"), + ], +) +def test_delete_mcp_server_route_uses_bodyless_path_parameter(server_name, request_path, monkeypatch): + deleted_names: list[str] = [] + + async def allow_admin(_request, *, detail): + assert detail == _ADMIN_REQUIRED_DETAIL + + def fake_delete(name: str): + deleted_names.append(name) + return {} + + monkeypatch.setattr(mcp_router, "require_admin_user", allow_admin) + monkeypatch.setattr(mcp_router, "_apply_mcp_server_delete", fake_delete) + monkeypatch.setattr(mcp_router, "reset_mcp_tools_cache", lambda: None) + + app = FastAPI() + app.router.redirect_slashes = False + app.include_router(mcp_router.router) + delete_route = next(route for route in app.routes if getattr(route, "path", None) == "/api/mcp/config/servers/{server_name:path}" and "DELETE" in route.methods) + assert delete_route.body_field is None + + with TestClient(app) as client: + response = client.request("DELETE", request_path) + + assert response.status_code == 200 + assert response.json() == {"mcp_servers": {}} + assert deleted_names == [server_name] + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + ("handler", "body"), + [ + ( + create_mcp_servers, + McpConfigUpdateRequest(mcp_servers={"added": McpServerConfigResponse(command="npx")}), + ), + ( + update_mcp_server, + McpServerConfigUpdateRequest( + server_name="existing", + server=McpServerConfigResponse(command="npx"), + ), + ), + ], +) +async def test_targeted_mcp_server_crud_requires_admin(handler, body): + with pytest.raises(HTTPException) as exc_info: + await handler(_request_with_role("user"), body) + + assert exc_info.value.status_code == 403 + + +@pytest.mark.asyncio +async def test_delete_mcp_server_requires_admin(): + with pytest.raises(HTTPException) as exc_info: + await delete_mcp_server(_request_with_role("user"), "existing") + + assert exc_info.value.status_code == 403 + + +@pytest.mark.asyncio +async def test_update_mcp_server_allows_editing_disabled_disallowed_stdio_server(monkeypatch, tmp_path): + config_path = tmp_path / "extensions_config.json" + original = { + "mcpServers": { + "semantic-scholar": { + "enabled": False, + "type": "stdio", + "command": "s2-mcp-server", + "args": ["--old"], + "description": "Old description", + } + }, + "skills": {}, + } + config_path.write_text(json.dumps(original), encoding="utf-8") + + def fake_reload_extensions_config(): + return ExtensionsConfig.model_validate(json.loads(config_path.read_text(encoding="utf-8"))) + + monkeypatch.setattr(mcp_router.ExtensionsConfig, "resolve_config_path", lambda: config_path) + monkeypatch.setattr(mcp_router, "reload_extensions_config", fake_reload_extensions_config) + monkeypatch.setattr(mcp_router, "reset_mcp_tools_cache", lambda: None) + monkeypatch.delenv(_MCP_STDIO_COMMAND_ALLOWLIST_ENV, raising=False) + + response = await update_mcp_server( + _request_with_role("admin"), + McpServerConfigUpdateRequest( + server_name="semantic-scholar", + server=McpServerConfigResponse( + enabled=False, + type="stdio", + command="s2-mcp-server", + args=["--new"], + description="Updated while offline", + ), + ), + ) + + persisted = json.loads(config_path.read_text(encoding="utf-8")) + assert persisted["mcpServers"]["semantic-scholar"]["args"] == ["--new"] + assert persisted["mcpServers"]["semantic-scholar"]["description"] == "Updated while offline" + assert response.mcp_servers["semantic-scholar"].enabled is False + + before_enable = config_path.read_text(encoding="utf-8") + with pytest.raises(HTTPException) as exc_info: + await update_mcp_server( + _request_with_role("admin"), + McpServerConfigUpdateRequest( + server_name="semantic-scholar", + server=McpServerConfigResponse( + enabled=True, + type="stdio", + command="s2-mcp-server", + ), + ), + ) + + assert exc_info.value.status_code == 400 + assert "s2-mcp-server" in exc_info.value.detail + assert config_path.read_text(encoding="utf-8") == before_enable + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + ("server_fields", "expected_detail"), + [ + ({"command": " "}, "requires a command"), + ({"command": "/usr/local/bin/npx"}, "single executable name"), + ({"command": "npx --yes"}, "single executable name"), + ({"command": "npx;echo"}, "single executable name"), + ({"command": "npx", "env": {"BASH_ENV": "/tmp/payload.sh"}}, "environment variable"), + ], + ids=["blank-command", "command-path", "command-whitespace", "command-metachar", "injecting-env"], +) +async def test_update_mcp_server_rejects_invalid_disabled_stdio_without_writing( + monkeypatch, + tmp_path, + server_fields, + expected_detail, +): + config_path = tmp_path / "extensions_config.json" + original = { + "mcpServers": { + "target": { + "enabled": False, + "type": "stdio", + "command": "npx", + "args": ["mcp-server-fetch"], + } + }, + "skills": {}, + } + config_path.write_text(json.dumps(original), encoding="utf-8") + + monkeypatch.setattr(mcp_router.ExtensionsConfig, "resolve_config_path", lambda: config_path) + monkeypatch.setattr(mcp_router, "reload_extensions_config", lambda: None) + monkeypatch.setattr(mcp_router, "reset_mcp_tools_cache", lambda: None) + monkeypatch.delenv(_MCP_STDIO_COMMAND_ALLOWLIST_ENV, raising=False) + + with pytest.raises(HTTPException) as exc_info: + await update_mcp_server( + _request_with_role("admin"), + McpServerConfigUpdateRequest( + server_name="target", + server=McpServerConfigResponse( + enabled=False, + type="stdio", + **server_fields, + ), + ), + ) + + assert exc_info.value.status_code == 400 + assert expected_detail in exc_info.value.detail + assert json.loads(config_path.read_text(encoding="utf-8")) == original + + @pytest.mark.asyncio @pytest.mark.parametrize("enabled", [False, True]) async def test_update_mcp_server_state_updates_valid_target_despite_unrelated_disallowed_command( diff --git a/backend/tests/test_mcp_context_headers.py b/backend/tests/test_mcp_context_headers.py index 04a4e6fb1..cad590ab8 100644 --- a/backend/tests/test_mcp_context_headers.py +++ b/backend/tests/test_mcp_context_headers.py @@ -955,6 +955,46 @@ def test_gateway_keeps_block_extras_a_put_does_not_mention(): assert merged.headers_from_context.model_extra["vendor_note"] == "keep-me" +def test_gateway_complete_replacement_resets_omitted_block_fields_and_extras(): + """Targeted PUT keeps only explicitly masked secrets from the stored block.""" + from app.gateway.routers.mcp import ( + McpContextHeadersConfigResponse, + McpServerConfigResponse, + _merge_preserving_secrets, + ) + + existing = McpServerConfigResponse( + type="http", + url="https://mcp.example.com/mcp", + headers_from_context=McpContextHeadersConfigResponse( + headers={"X-Tenant-Token": "tenant_token"}, + on_missing="passthrough", + api_key="real-secret", + vendor_note="remove-me", + ), + ) + incoming = McpServerConfigResponse( + type="http", + url="https://mcp.example.com/mcp", + headers_from_context=McpContextHeadersConfigResponse( + enabled=False, + api_key="***", + ), + ) + + merged = _merge_preserving_secrets( + incoming, + existing, + preserve_omitted_fields=False, + ) + + assert merged.headers_from_context is not None + assert merged.headers_from_context.enabled is False + assert merged.headers_from_context.headers == {} + assert merged.headers_from_context.on_missing == "deny" + assert merged.headers_from_context.model_extra == {"api_key": "real-secret"} + + def test_gateway_rejects_a_masked_value_for_an_unknown_block_extra(): """A sentinel with nothing stored behind it must not be written to disk.""" from fastapi import HTTPException diff --git a/backend/tests/test_mcp_user_scoped_auth.py b/backend/tests/test_mcp_user_scoped_auth.py index a0a95ebb8..53e3787a2 100644 --- a/backend/tests/test_mcp_user_scoped_auth.py +++ b/backend/tests/test_mcp_user_scoped_auth.py @@ -312,6 +312,46 @@ def test_explicit_users_map_still_replaces_and_can_remove(): assert merged.user_auth.users == {"u1": "Bearer s1"} # u2 removed, u1 preserved through mask +def test_complete_replacement_user_auth_honors_omitted_subfields(): + from app.gateway.routers.mcp import ( + McpServerConfigResponse, + McpUserScopedAuthConfigResponse, + _merge_preserving_secrets, + ) + + existing = McpServerConfigResponse( + type="http", + url="https://x", + user_auth=McpUserScopedAuthConfigResponse( + header="X-Api-Key", + users={"u1": "Bearer s1", "u2": "Bearer s2"}, + on_missing="passthrough", + custom_note="remove-me", + ), + ) + incoming = McpServerConfigResponse( + type="http", + url="https://x", + user_auth=McpUserScopedAuthConfigResponse( + enabled=False, + users={"u1": "***"}, + ), + ) + + merged = _merge_preserving_secrets( + incoming, + existing, + preserve_omitted_fields=False, + ) + + assert merged.user_auth is not None + assert merged.user_auth.enabled is False + assert merged.user_auth.header == "Authorization" + assert merged.user_auth.users == {"u1": "Bearer s1"} + assert merged.user_auth.on_missing == "deny" + assert "custom_note" not in (merged.user_auth.model_extra or {}) + + def test_user_auth_extra_keys_survive_parse_mask_and_merge(): from app.gateway.routers.mcp import ( McpServerConfigResponse, @@ -332,6 +372,42 @@ def test_user_auth_extra_keys_survive_parse_mask_and_merge(): assert (merged.user_auth.model_extra or {}).get("custom_note") == "keep-me" +def test_user_auth_extra_array_rejects_structural_edit_while_secrets_are_masked(): + from fastapi import HTTPException + + from app.gateway.routers.mcp import ( + McpServerConfigResponse, + McpUserScopedAuthConfigResponse, + _mask_server_config, + _merge_preserving_secrets, + ) + + existing = McpServerConfigResponse( + type="http", + url="https://x", + user_auth=McpUserScopedAuthConfigResponse( + providers=[ + {"name": "alpha", "apiKey": "secret-alpha"}, + {"name": "beta", "apiKey": "secret-beta"}, + ] + ), + ) + masked = _mask_server_config(existing) + incoming = McpServerConfigResponse( + type="http", + url="https://x", + user_auth=McpUserScopedAuthConfigResponse( + providers=list(reversed(masked.user_auth.model_extra["providers"])), + ), + ) + + with pytest.raises(HTTPException) as exc_info: + _merge_preserving_secrets(incoming, existing) + + assert exc_info.value.status_code == 400 + assert "providers" in exc_info.value.detail + + def test_stdio_server_user_auth_is_skipped_with_warning(caplog): import logging diff --git a/frontend/src/AGENTS.md b/frontend/src/AGENTS.md index fa599310d..0734a0a25 100644 --- a/frontend/src/AGENTS.md +++ b/frontend/src/AGENTS.md @@ -18,6 +18,11 @@ mutation, disables switches until that mutation's success refetch completes, displays the backend error `detail` through a toast, and invalidates `["mcpConfig"]` only after success. + Server management uses targeted `POST /api/mcp/config/servers`, + `PUT /api/mcp/config/server`, and bodyless + `DELETE /api/mcp/config/servers/{server_name}` mutations. Delete names are + percent-encoded, including legacy empty and slash-containing names; every + successful mutation invalidates `["mcpConfig"]` only after the response. Current-chat MCP background tasks use `core/background-tasks`: the header trigger is hidden for new/mock/static-demo threads and unless `/api/features` reports the startup-scoped `mcp_tasks` capability; the list query is disabled diff --git a/frontend/src/components/workspace/settings/tool-settings-page.tsx b/frontend/src/components/workspace/settings/tool-settings-page.tsx index 7572dfe30..7fd5da326 100644 --- a/frontend/src/components/workspace/settings/tool-settings-page.tsx +++ b/frontend/src/components/workspace/settings/tool-settings-page.tsx @@ -1,5 +1,17 @@ "use client"; +import { PencilIcon, Trash2 } from "lucide-react"; +import { useState } from "react"; + +import { Button } from "@/components/ui/button"; +import { + Dialog, + DialogContent, + DialogDescription, + DialogFooter, + DialogHeader, + DialogTitle, +} from "@/components/ui/dialog"; import { Item, ItemActions, @@ -8,9 +20,19 @@ import { ItemTitle, } from "@/components/ui/item"; import { Switch } from "@/components/ui/switch"; +import { Textarea } from "@/components/ui/textarea"; import { useI18n } from "@/core/i18n/hooks"; import { MCPConfigRequestError } from "@/core/mcp/api"; -import { useMCPConfig, useEnableMCPServer } from "@/core/mcp/hooks"; +import { + useEnableMCPServer, + useMCPConfig, + useMCPServerMutation, +} from "@/core/mcp/hooks"; +import { + formatMCPServerDefinition, + MCPServerDefinitionError, + parseMCPServerDefinition, +} from "@/core/mcp/parse"; import type { MCPServerConfig } from "@/core/mcp/types"; import { env } from "@/env"; @@ -48,41 +70,267 @@ function MCPServerList({ }) { const { t } = useI18n(); const { isPending, mutate: enableMCPServer } = useEnableMCPServer(); - const entries = Object.entries(servers ?? {}); - if (entries.length === 0) { - return ( -
- {t.settings.tools.empty} -
+ const { isPending: isWriting, mutate: mutateServer } = useMCPServerMutation(); + const [editor, setEditor] = useState< + { mode: "add" } | { mode: "edit"; name: string } | null + >(null); + const [definition, setDefinition] = useState(""); + const [definitionError, setDefinitionError] = useState(null); + const [pendingRemoval, setPendingRemoval] = useState(null); + + const readOnly = env.NEXT_PUBLIC_STATIC_WEBSITE_ONLY === "true"; + const current = servers ?? {}; + const entries = Object.entries(current); + const isMutating = isPending || isWriting; + + function displayServerName(name: string | null) { + return name === null || name.length === 0 + ? t.settings.tools.unnamedServer + : name; + } + + function closeEditor() { + setEditor(null); + setDefinition(""); + setDefinitionError(null); + } + + function openAddEditor() { + setDefinition(""); + setDefinitionError(null); + setEditor({ mode: "add" }); + } + + function openEditEditor(name: string, config: MCPServerConfig) { + setDefinition(formatMCPServerDefinition(name, config)); + setDefinitionError(null); + setEditor({ mode: "edit", name }); + } + + function handleSaveDefinition() { + if (editor === null) { + return; + } + + let parsed: Record; + try { + parsed = parseMCPServerDefinition(definition); + } catch (parseError) { + if (parseError instanceof MCPServerDefinitionError) { + const messages = { + emptyDefinition: t.settings.tools.definitionEmpty, + invalidJson: t.settings.tools.definitionInvalidJson, + rootNotObject: t.settings.tools.definitionRootNotObject, + emptyServerMap: t.settings.tools.definitionNoServers, + serverConfigNotObject: + t.settings.tools.definitionServerNotObject.replace( + "{name}", + parseError.serverName ?? "", + ), + }; + setDefinitionError(messages[parseError.code]); + } else { + setDefinitionError(t.settings.tools.definitionInvalidJson); + } + return; + } + + if (editor.mode === "add") { + const duplicate = Object.keys(parsed).find((name) => + Object.hasOwn(current, name), + ); + if (duplicate !== undefined) { + setDefinitionError( + t.settings.tools.serverAlreadyExists.replace("{name}", duplicate), + ); + return; + } + setDefinitionError(null); + mutateServer( + { operation: "create", servers: parsed }, + { onSuccess: closeEditor }, + ); + } else { + const editedEntries = Object.entries(parsed); + if (editedEntries.length !== 1) { + setDefinitionError(t.settings.tools.editSingleServer); + return; + } + const [editedName, editedConfig] = editedEntries[0]!; + if (editedName !== editor.name) { + setDefinitionError( + t.settings.tools.editServerNameMismatch.replace( + "{name}", + editor.name, + ), + ); + return; + } + setDefinitionError(null); + mutateServer( + { + operation: "update", + serverName: editor.name, + server: editedConfig, + }, + { onSuccess: closeEditor }, + ); + } + } + + function handleRemove(name: string) { + mutateServer( + { operation: "delete", serverName: name }, + { onSuccess: () => setPendingRemoval(null) }, ); } + return (
- {entries.map(([name, config]) => ( - - - -
-
{name}
-
- - - {config.description} - - - - + +
+ + {entries.length === 0 ? ( +
+ {t.settings.tools.empty} +
+ ) : ( + entries.map(([name, config]) => { + const displayName = displayServerName(name); + return ( + + + +
+
{displayName}
+
+ + + {config.description} + + + + + enableMCPServer({ serverName: name, enabled: checked }) + } + /> + + + + + ); + }) + )} + + !open && !isWriting && closeEditor()} + > + + + + {editor?.mode === "edit" + ? t.settings.tools.editServer + : t.settings.tools.addServer} + + + {editor?.mode === "edit" + ? t.settings.tools.editServerDescription.replace( + "{name}", + editor.name, + ) + : t.settings.tools.addServerDescription} + + +