From 3cab8af75375876f3501c798002a0b3332fefb89 Mon Sep 17 00:00:00 2001 From: arc53-machine <232052973+arc53-machine@users.noreply.github.com> Date: Tue, 29 Sep 2026 18:12:54 +0100 Subject: [PATCH] Keep the removed-connection note server-owned Only removing a connection notes it on a kept tool. A config save through update_tool or update_tool_config carries the stored note and ignores the client's copy, so it can neither fake nor clear it; creating a tool, an MCP server save and an agent import start without one. The note is now written even for a connection with no catalog key, and a kept tool its owner gave credentials of its own since runs again. --- docsgpt/api/user/agents/portability.py | 4 ++- docsgpt/api/user/tools/mcp.py | 8 ++++-- docsgpt/api/user/tools/routes.py | 8 ++++++ docsgpt/connectors/resolve.py | 35 ++++++++++++++++++++++---- docsgpt/connectors/service.py | 4 +-- tests/api/user/test_tools_access.py | 30 ++++++++++++++++++++++ 6 files changed, 79 insertions(+), 10 deletions(-) diff --git a/docsgpt/api/user/agents/portability.py b/docsgpt/api/user/agents/portability.py index 005b6473..39cd778b 100644 --- a/docsgpt/api/user/agents/portability.py +++ b/docsgpt/api/user/agents/portability.py @@ -38,6 +38,7 @@ from docsgpt.agents.default_tools import ( from docsgpt.api import api from docsgpt.api.pat.rules import allowed_ids from docsgpt.api.user.resource_access import AccessDenied, require +from docsgpt.connectors.resolve import carry_removed_connection from docsgpt.core.model_utils import validate_model_id from docsgpt.core.url_validation import SSRFError, validate_url from docsgpt.security.safe_url import UnsafeUserUrlError, validate_user_base_url @@ -1138,7 +1139,8 @@ def _create_tool_from_spec(conn, user: str, tool: dict, secrets: dict, warnings: warnings.append(f"Tool type '{tool_type}' not available on this instance; skipped") return None config_requirements = inst.get_config_requirements() or {} - config = dict(tool.get("config") or {}) + # An imported tool starts with no note that a connection was removed. + config = carry_removed_connection(dict(tool.get("config") or {}), None) config.update(secrets or {}) if tool_type == "api_tool": label = tool.get("display_name") or tool.get("name") or tool_type diff --git a/docsgpt/api/user/tools/mcp.py b/docsgpt/api/user/tools/mcp.py index f4e2418b..1308d4c3 100644 --- a/docsgpt/api/user/tools/mcp.py +++ b/docsgpt/api/user/tools/mcp.py @@ -19,6 +19,7 @@ from docsgpt.api.user.tools.routes import ( transform_actions, ) from docsgpt.cache import get_redis_instance +from docsgpt.connectors.resolve import REMOVED_CONNECTION_KEY from docsgpt.core.url_validation import SSRFError, validate_url from docsgpt.security.encryption import decrypt_credentials, encrypt_credentials from docsgpt.storage.db.repositories.connector_sessions import ( @@ -37,8 +38,10 @@ def _sanitize_mcp_transport(config): """Normalise and validate the transport_type field. Strips ``command`` / ``args`` keys that are only valid for local STDIO - transports, and ``connection_id``, which only the tool executor sets - (it picks whose MCP tokens the tool uses). Returns the cleaned + transports, ``connection_id``, which only the tool executor sets + (it picks whose MCP tokens the tool uses), and the note that a + connection was removed, which only removing one writes (a save stores + a fresh config, so it drops that note too). Returns the cleaned transport type string. """ transport_type = (config.get("transport_type") or "auto").lower() @@ -47,6 +50,7 @@ def _sanitize_mcp_transport(config): config.pop("command", None) config.pop("args", None) config.pop("connection_id", None) + config.pop(REMOVED_CONNECTION_KEY, None) config["transport_type"] = transport_type return transport_type diff --git a/docsgpt/api/user/tools/routes.py b/docsgpt/api/user/tools/routes.py index ec5bb020..d400496f 100644 --- a/docsgpt/api/user/tools/routes.py +++ b/docsgpt/api/user/tools/routes.py @@ -35,6 +35,7 @@ from docsgpt.api.user.resource_access import ( ) from docsgpt.api.user.team_sharing import visible_with_access from docsgpt.connectors.catalog import base_url, definition_for_tool +from docsgpt.connectors.resolve import carry_removed_connection from docsgpt.connectors.service import account_tool_names from docsgpt.connectors.permissions import owner_credential_writes from docsgpt.core.settings import settings @@ -521,12 +522,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 origin-change rule (a new scheme, host or port drops stored credentials). + A removed connection's note is carried over from the stored config; the + client's copy is ignored. Raises: CredentialsRequired: the MCP origin changed and no new secret arrived. """ owner_id = tool_doc["user_id"] existing_config = tool_doc.get("config") or {} + # The note that its connection was removed is the server's to keep. + new_config = carry_removed_connection(new_config, existing_config) 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_origin_changed(new_config, existing_config) @@ -834,6 +839,9 @@ class CreateTool(Resource): missing_fields = check_required_fields(data, required_fields) if missing_fields: return missing_fields + if isinstance(data.get("config"), dict): + # Only removing a connection notes that it was removed. + data["config"] = carry_removed_connection(data["config"], None) try: if data["name"] == "mcp_tool": server_url = (data.get("config", {}).get("server_url") or "").strip() diff --git a/docsgpt/connectors/resolve.py b/docsgpt/connectors/resolve.py index e8ffdfac..1b3119d0 100644 --- a/docsgpt/connectors/resolve.py +++ b/docsgpt/connectors/resolve.py @@ -28,11 +28,33 @@ CONNECTION_NEEDS_RECONNECT = "connection_needs_reconnect" CONNECTION_REMOVED = "connection_removed" CONNECTOR_DISABLED = "connector_disabled" -# Tool config key ``remove_connection`` sets to the connector key when it -# keeps a connection's tools, so they can say what they lost. +# Tool config key ``remove_connection`` sets when it keeps a connection's +# tools: the connector key, or True when the connection had none, so they can +# say what they lost. Only the server writes it (see +# :func:`carry_removed_connection`). REMOVED_CONNECTION_KEY = "removed_connection" +def carry_removed_connection(new_config: dict, stored_config: Optional[dict]) -> dict: + """``new_config`` with the stored removed-connection note, never a client's. + + A config save must neither fake the note nor clear it; every path that + writes a tool config from a request passes it through here. + + Args: + new_config: The config about to be stored. + stored_config: The tool's stored config (None or ``{}`` when creating it). + + Returns: + dict: A copy of ``new_config`` carrying the stored note, if any. + """ + out = {k: v for k, v in (new_config or {}).items() if k != REMOVED_CONNECTION_KEY} + marker = (stored_config or {}).get(REMOVED_CONNECTION_KEY) + if marker: + out[REMOVED_CONNECTION_KEY] = marker + return out + + @dataclass(frozen=True) class ResolvedConnection: """The connection a call runs with, or why there is none. @@ -175,15 +197,18 @@ def connection_stop_reason(tool: dict, resolved: Optional[ResolvedConnection]) - Returns: :data:`CONNECTION_REMOVED` when its connection was removed and the - tool kept (``remove_connection`` notes it on the tool), or points at + tool kept (``remove_connection`` notes it on the tool) with no + credentials of its own, or points at one it may not use; :data:`CONNECTOR_DISABLED` when an admin turned the service off; :data:`CONNECTION_NEEDS_RECONNECT` when the account must sign in again; else None. """ if resolved is None: # Only a tool that had a connection lost it: a tool that never had - # one (a tokenless ntfy, a legacy tool) runs on its own config. - removed = (tool.get("config") or {}).get(REMOVED_CONNECTION_KEY) + # one (a tokenless ntfy, a legacy tool) runs on its own config, and + # so does a kept one its owner gave credentials of its own since. + config = tool.get("config") or {} + removed = config.get(REMOVED_CONNECTION_KEY) and not config.get("encrypted_credentials") return CONNECTION_REMOVED if removed and not tool.get("connection_id") else None if not resolved.enabled: return CONNECTOR_DISABLED diff --git a/docsgpt/connectors/service.py b/docsgpt/connectors/service.py index 6fa4521b..1b6f9ac4 100644 --- a/docsgpt/connectors/service.py +++ b/docsgpt/connectors/service.py @@ -1460,9 +1460,9 @@ def remove_connection(conn, row: dict, *, sources: str = "keep", tools: str = "d text( "UPDATE user_tools SET status = false, " "config = COALESCE(config, '{}'::jsonb) || jsonb_build_object('removed_connection', " - "CAST(:key AS text)) WHERE connection_id = CAST(:id AS uuid)" + "CAST(:marker AS jsonb)) WHERE connection_id = CAST(:id AS uuid)" ), - {"id": connection_id, "key": catalog.connector_key_for_row(row) or ""}, + {"id": connection_id, "marker": json.dumps(catalog.connector_key_for_row(row) or True)}, ) conn.execute( text( diff --git a/tests/api/user/test_tools_access.py b/tests/api/user/test_tools_access.py index fa1ce17a..1225924f 100644 --- a/tests/api/user/test_tools_access.py +++ b/tests/api/user/test_tools_access.py @@ -408,6 +408,36 @@ class TestToolWrites: # --------------------------------------------------------------------------- # 5. POST /api/update_tool_status # --------------------------------------------------------------------------- +class TestRemovedConnectionNote: + """Only removing a connection notes it on a kept tool; config saves can't fake or clear it.""" + + @pytest.mark.parametrize("route", ["UpdateToolConfig", "UpdateTool"]) + def test_a_config_save_cannot_set_the_note(self, app, pg_conn, route): + from docsgpt.api.user.tools import routes + + tool = _tool(pg_conn, name="brave", config={}) + body = {"id": str(tool["id"]), "config": {"token": "x", "removed_connection": "brave"}} + assert _call(app, pg_conn, getattr(routes, route), OWNER, json=body).status_code == 200 + assert "removed_connection" not in _row(pg_conn, tool["id"])["config"] + + @pytest.mark.parametrize("route", ["UpdateToolConfig", "UpdateTool"]) + def test_a_config_save_keeps_the_stored_note(self, app, pg_conn, route): + from docsgpt.api.user.tools import routes + + tool = _tool(pg_conn, name="brave", config={"removed_connection": "brave"}) + for config in ({"token": "x"}, {"token": "y", "removed_connection": None}): + body = {"id": str(tool["id"]), "config": config} + assert _call(app, pg_conn, getattr(routes, route), OWNER, json=body).status_code == 200 + assert _row(pg_conn, tool["id"])["config"]["removed_connection"] == "brave" + + def test_an_mcp_save_drops_the_note(self): + from docsgpt.api.user.tools.mcp import _sanitize_mcp_transport + + cfg = {"transport_type": "http", "removed_connection": "github"} + _sanitize_mcp_transport(cfg) + assert "removed_connection" not in cfg + + class TestUpdateToolStatusRoles: def test_owner_writes_status(self, app, pg_conn): from docsgpt.api.user.tools.routes import UpdateToolStatus