diff --git a/docsgpt/api/user/tools/mcp.py b/docsgpt/api/user/tools/mcp.py index c28164f1..5f284c82 100644 --- a/docsgpt/api/user/tools/mcp.py +++ b/docsgpt/api/user/tools/mcp.py @@ -12,7 +12,7 @@ from docsgpt.api.user.team_sharing import visible_with_access from docsgpt.api.user.tools.routes import ( _CREDENTIALS_FOR_NEW_SERVER, _MCP_CREDENTIAL_AUTH_TYPES, - _mcp_host_changed, + _mcp_origin_changed, check_oauth_mcp_owner_only, denied_response, transform_actions, @@ -90,7 +90,8 @@ def _existing_mcp_context(tool_id, user, config): With no ``tool_id`` the caller acts on their own new server. With one, the caller needs ``edit_credentials`` on that tool and everything runs as its owner. Stored secrets are write-only, so an empty secret field reuses - the stored one while the host is unchanged; a new host never inherits them. + the stored one while the origin (scheme, host, port) is unchanged; a new + origin never inherits them. A server that is or would become OAuth is the owner's alone (its tokens are the owner's sign-in). @@ -114,7 +115,7 @@ def _existing_mcp_context(tool_id, user, config): ) existing_config = existing_doc.get("config") or {} check_oauth_mcp_owner_only(ra, existing_config, config) - moved = _mcp_host_changed(config, existing_config) + moved = _mcp_origin_changed(config, existing_config) auth_type = config.get("auth_type", "none") new_secret_keys = set(auth_credentials) - {"api_key_header"} if moved and auth_type in _MCP_CREDENTIAL_AUTH_TYPES and not new_secret_keys: diff --git a/docsgpt/api/user/tools/routes.py b/docsgpt/api/user/tools/routes.py index 8528ba64..25d826e1 100644 --- a/docsgpt/api/user/tools/routes.py +++ b/docsgpt/api/user/tools/routes.py @@ -193,7 +193,7 @@ _META_KEYS = ("name", "displayName", "customName", "description", "actions") class CredentialsRequired(Exception): - """A save moved a tool to a new host without supplying new secrets.""" + """A save moved a tool to a new origin without supplying new secrets.""" def denied_response(err: AccessDenied): @@ -207,12 +207,26 @@ def check_action(ra: ResourceAccess, action: str) -> None: raise AccessDenied(403, _FORBIDDEN_MESSAGE) -def url_host(url: Any) -> str: - """Lower-cased host of ``url`` ('' when it has none).""" +_DEFAULT_PORTS = {"http": 80, "https": 443, "ws": 80, "wss": 443} + + +def url_origin(url: Any) -> str: + """``scheme://host:port`` of ``url``, lower-cased, default port filled in. + + Saved secrets follow the origin, not just the host: the same host over + ``http`` would send them in cleartext, and another port can be another + service. ``''`` when the URL has no host or doesn't parse. + """ try: - return (urlparse(str(url or "").strip()).hostname or "").lower() + parts = urlparse(str(url or "").strip()) + host = (parts.hostname or "").lower() + port = parts.port except ValueError: return "" + if not host: + return "" + scheme = (parts.scheme or "").lower() + return f"{scheme}://{host}:{port or _DEFAULT_PORTS.get(scheme, '')}" def _has_value(value: Any) -> bool: @@ -270,7 +284,7 @@ def _seal_api_tool_secrets(new_config: dict, existing_config: dict, owner_id: st An incoming entry with a value replaces the stored one; an empty value with ``has_value`` keeps it (legacy plaintext values included); anything else - clears it. When an action's URL host changes the stored values are not + clears it. When an action's URL origin changes the stored values are not carried over. Args: @@ -282,7 +296,7 @@ def _seal_api_tool_secrets(new_config: dict, existing_config: dict, owner_id: st The config to persist. Raises: - CredentialsRequired: a host changed, the client asked to keep a value, + CredentialsRequired: an origin changed, the client asked to keep a value, and there is nothing to keep. """ existing_config = existing_config or {} @@ -293,7 +307,7 @@ def _seal_api_tool_secrets(new_config: dict, existing_config: dict, owner_id: st sealed: dict = {} for name, action in (out.get("actions") or {}).items(): old = old_actions.get(name) if isinstance(old_actions, dict) else None - moved = isinstance(old, dict) and url_host(old.get("url")) != url_host( + moved = isinstance(old, dict) and url_origin(old.get("url")) != url_origin( action.get("url") if isinstance(action, dict) else "" ) prior = {} if moved else stored.get(name, {}) @@ -345,9 +359,10 @@ def _api_tool_config_needs_credentials(new_config: dict, existing_config: dict) return False -def _mcp_host_changed(new_config: dict, existing_config: dict) -> bool: +def _mcp_origin_changed(new_config: dict, existing_config: dict) -> bool: + """Whether a save moves an MCP server to another scheme, host or port.""" old_url = (existing_config or {}).get("server_url") - return bool(old_url) and url_host(old_url) != url_host((new_config or {}).get("server_url")) + return bool(old_url) and url_origin(old_url) != url_origin((new_config or {}).get("server_url")) SHARED_OAUTH_OWNER_ONLY = "Only the owner can change or reconnect this server" @@ -386,16 +401,16 @@ def _prepare_tool_config(tool_doc: dict, new_config: dict, config_requirements: Handles the three secret stores: ``config_requirements`` secrets (``encrypted_credentials``), api_tool header/query values, and the MCP - host-change rule (a new server host drops stored credentials). + origin-change rule (a new scheme, host or port drops stored credentials). Raises: - CredentialsRequired: the MCP host changed and no new secret arrived. + CredentialsRequired: the MCP origin changed and no new secret arrived. """ owner_id = tool_doc["user_id"] existing_config = tool_doc.get("config") or {} if tool_doc.get("name") == "api_tool": return _seal_api_tool_secrets(new_config, existing_config, owner_id) - moved = tool_doc.get("name") == "mcp_tool" and _mcp_host_changed(new_config, existing_config) + moved = tool_doc.get("name") == "mcp_tool" and _mcp_origin_changed(new_config, existing_config) if moved: existing_config = {k: v for k, v in existing_config.items() if k != "encrypted_credentials"} final = _merge_secrets_on_update(new_config, existing_config, config_requirements, owner_id) diff --git a/frontend/src/modals/MCPServerModal.test.tsx b/frontend/src/modals/MCPServerModal.test.tsx index b7a1c8c8..54236f20 100644 --- a/frontend/src/modals/MCPServerModal.test.tsx +++ b/frontend/src/modals/MCPServerModal.test.tsx @@ -167,6 +167,21 @@ describe('MCPServerModal', () => { expect(text()).toContain('settings.tools.mcp.errors.apiKeyRequired'); }); + it.each([ + ['a downgrade to http', 'http://mcp.dana-tools.dev/sse'], + ['another port', 'https://mcp.dana-tools.dev:8443/sse'], + ])('clears the saved key for %s', async (_label, url) => { + await render(); + await typeInto(urlInput(), url); + expect(text()).toContain('settings.tools.mcp.serverChangedNotice'); + }); + + it('keeps the saved key when only the default port is spelled out', async () => { + await render(); + await typeInto(urlInput(), 'https://mcp.dana-tools.dev:443/v2/sse'); + expect(text()).not.toContain('settings.tools.mcp.serverChangedNotice'); + }); + it('keeps the saved key for a path-only change on the same host', async () => { await render(); await typeInto(urlInput(), 'https://mcp.dana-tools.dev/v2/sse'); diff --git a/frontend/src/modals/MCPServerModal.tsx b/frontend/src/modals/MCPServerModal.tsx index accc16c7..ba99ddfe 100644 --- a/frontend/src/modals/MCPServerModal.tsx +++ b/frontend/src/modals/MCPServerModal.tsx @@ -30,10 +30,14 @@ interface MCPServerModalProps { onServerSaved: () => void; } -/** The host of a URL, or '' while it doesn't parse. */ -function hostOf(url: string): string { +/** + * The origin (scheme, host, port) of a URL, or '' while it doesn't parse. + * Saved secrets follow the origin, like the server's rule: the same host over + * http would send them in cleartext, and another port can be another service. + */ +function originOf(url: string): string { try { - return new URL(url.trim()).host.toLowerCase(); + return new URL(url.trim()).origin.toLowerCase(); } catch { return ''; } @@ -123,11 +127,11 @@ export default function MCPServerModal({ !!server?.has_encrypted_credentials && !!savedSecret && formData.auth_type === server?.auth_type; - // The server clears the saved secret when the host changes, so the key - // can't be pointed at another server. + // The server clears the saved secret when the origin changes, so the key + // can't be pointed at another server, port or plain http. const serverChanged = hasSavedSecret && - hostOf(formData.server_url) !== hostOf(server?.server_url || ''); + originOf(formData.server_url) !== originOf(server?.server_url || ''); const keepSavedSecret = hasSavedSecret && !serverChanged; const cleanupOAuthListener = useCallback(() => { diff --git a/tests/api/user/test_tools_access.py b/tests/api/user/test_tools_access.py index b404f0e0..27ba7b4d 100644 --- a/tests/api/user/test_tools_access.py +++ b/tests/api/user/test_tools_access.py @@ -262,6 +262,24 @@ class TestApiToolSecrets: assert self._save(app, pg_conn, OWNER, tool["id"], same_host).status_code == 200 assert _runtime_action(pg_conn, tool["id"])["headers"]["properties"]["X-Key"]["value"] == "sk-secret" + @pytest.mark.parametrize("url", [ + "http://api.example.com/users", # https -> http: cleartext + "https://api.example.com:8443/users", # another port, maybe another service + ]) + def test_url_scheme_or_port_change_requires_new_secrets(self, app, pg_conn, url): + tool = _tool(pg_conn, name="api_tool", config={}) + self._save(app, pg_conn, OWNER, tool["id"], _api_config()) + resp = self._save(app, pg_conn, OWNER, tool["id"], self._masked(_api_config(url=url))) + assert resp.status_code == 400 + assert "credentials" in resp.json["message"].lower() + + def test_url_with_explicit_default_port_keeps_secrets(self, app, pg_conn): + tool = _tool(pg_conn, name="api_tool", config={}) + self._save(app, pg_conn, OWNER, tool["id"], _api_config()) + same = self._masked(_api_config(url="https://API.example.com:443/users")) + assert self._save(app, pg_conn, OWNER, tool["id"], same).status_code == 200 + assert _runtime_action(pg_conn, tool["id"])["headers"]["properties"]["X-Key"]["value"] == "sk-secret" + def test_url_host_change_with_new_secrets_succeeds(self, app, pg_conn): tool = _tool(pg_conn, name="api_tool", config={}) self._save(app, pg_conn, OWNER, tool["id"], _api_config()) @@ -326,6 +344,18 @@ class TestToolWrites: stored = _row(pg_conn, tool["id"])["config"] assert stored["server_url"] == "https://mcp.example.com/x" + def test_update_tool_config_scheme_change_clears_stored_secrets(self, app, pg_conn): + from docsgpt.api.user.tools.routes import UpdateToolConfig + + cfg = {"server_url": "https://mcp.example.com/x", "auth_type": "bearer", + "encrypted_credentials": encrypt_credentials({"bearer_token": "tok"}, OWNER)} + tool = _tool(pg_conn, name="mcp_tool", config=cfg) + body = {"id": str(tool["id"]), "config": {"server_url": "http://mcp.example.com/x", + "auth_type": "bearer"}} + resp = _call(app, pg_conn, UpdateToolConfig, OWNER, json=body) + assert resp.status_code == 400 + assert _row(pg_conn, tool["id"])["config"]["server_url"] == "https://mcp.example.com/x" + def test_api_tool_description_edit_needs_only_edit(self, app, pg_conn): from docsgpt.api.user.tools.routes import UpdateTool, _seal_api_tool_secrets @@ -475,6 +505,17 @@ class TestMCPSaveAccess: assert not cls.called assert _row(pg_conn, tool["id"])["config"]["server_url"] == "https://mcp.example.com/mcp" + @pytest.mark.parametrize("url", ["http://mcp.example.com/mcp", "https://mcp.example.com:8443/mcp"]) + def test_scheme_or_port_change_without_credentials_is_rejected(self, app, pg_conn, url): + tool = _mcp_tool(pg_conn, secrets={"bearer_token": "tok"}) + _share(pg_conn, tool["id"], "ed", "editor") + body = {"id": str(tool["id"]), "displayName": "M", + "config": {"server_url": url, "auth_type": "bearer", "transport_type": "http"}} + resp, cls = self._save(app, pg_conn, "ed", body) + assert resp.status_code == 400 + assert not cls.called + assert _row(pg_conn, tool["id"])["config"]["server_url"] == "https://mcp.example.com/mcp" + def test_host_change_with_new_credentials_replaces_them(self, app, pg_conn): tool = _mcp_tool(pg_conn, secrets={"bearer_token": "tok"}) body = {"id": str(tool["id"]), "displayName": "M", @@ -583,6 +624,13 @@ class TestMCPTestEndpointAccess: bearer_token="t2")) assert cls.call_args.kwargs["config"]["auth_credentials"] == {"bearer_token": "t2"} + def test_scheme_downgrade_without_secret_is_400(self, app, pg_conn): + tool = _mcp_tool(pg_conn, secrets={"bearer_token": "tok"}) + _share(pg_conn, tool["id"], "ed", "editor") + resp, cls = self._test(app, pg_conn, "ed", self._body(tool, url="http://mcp.example.com/mcp")) + assert resp.status_code == 400 + assert not cls.called + def test_viewer_and_stranger_denied(self, app, pg_conn): tool = _mcp_tool(pg_conn, secrets={"bearer_token": "tok"}) _share(pg_conn, tool["id"], "vi", "viewer")