Mcp fix

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:

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)

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');

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(() => {

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")