From 080b75c81ecafed262bb839122b84917e85c3361 Mon Sep 17 00:00:00 2001 From: arc53-machine <232052973+arc53-machine@users.noreply.github.com> Date: Tue, 29 Sep 2026 15:54:30 +0100 Subject: [PATCH] Gate an API tool action only when it sends the owner's saved values Every API tool counted as holding the owner's credentials, so outside callers were refused any write on one even when it sends nothing the owner stored. Now an action counts only when its headers or query parameters carry a saved value (sealed or legacy plaintext) or the tool stores credentials; the allowlist lists just those writes. --- docs/content/Guides/Connectors.mdx | 2 +- docsgpt/agents/tool_executor.py | 10 +++-- docsgpt/connectors/permissions.py | 43 +++++++++++++++----- tests/api/user/test_resource_sponsors.py | 5 ++- tests/connectors/test_public_link_callers.py | 32 +++++++++++++-- tests/connectors/test_tool_routes.py | 7 +++- 6 files changed, 77 insertions(+), 22 deletions(-) diff --git a/docs/content/Guides/Connectors.mdx b/docs/content/Guides/Connectors.mdx index 940d550a..07ca84c5 100644 --- a/docs/content/Guides/Connectors.mdx +++ b/docs/content/Guides/Connectors.mdx @@ -49,7 +49,7 @@ Tools from OAuth MCP servers default to each member's own account. An admin can ### Agents used through an API key -An agent called with its API key (the website widget, the API) or fired by a webhook runs with its owner's connections, and nobody can approve an action there. It can read through those connections, but it can't take write actions on them unless the owner allows each one under **Access details > Actions API, widget and public-link users can take as you**. The same goes for tools that hold the owner's own credentials without a connection: an API tool, or an MCP server the owner signed in to. Only the owner can change that list; team editors can't. The owner previewing their own agent in DocsGPT is not limited. +An agent called with its API key (the website widget, the API) or fired by a webhook runs with its owner's connections, and nobody can approve an action there. It can read through those connections, but it can't take write actions on them unless the owner allows each one under **Access details > Actions API, widget and public-link users can take as you**. The same goes for tools that hold the owner's own credentials without a connection: an API tool action that sends a header or query value the owner saved (a key or token), or an MCP server the owner signed in to. Only the owner can change that list; team editors can't. The owner previewing their own agent in DocsGPT is not limited. The same list covers people who open the agent from its public link without being on a team it's shared with, and anything they schedule from that chat. They can't approve write actions on the owner's accounts or credentials, so those run only when the owner allows them there. On a tool where each member connects their own account, they approve writes on their own account as usual. diff --git a/docsgpt/agents/tool_executor.py b/docsgpt/agents/tool_executor.py index a6f321d7..8644450a 100644 --- a/docsgpt/agents/tool_executor.py +++ b/docsgpt/agents/tool_executor.py @@ -1162,7 +1162,9 @@ class ToolExecutor: # was the owner's choice for themselves, not for anyone with the key. # A public-link user is a stranger to the owner, so their approval # can't stand in for the owner's either. - if (self.external_caller or self.public_link_caller) and self._on_owner_credentials(tool_data, resolved): + if (self.external_caller or self.public_link_caller) and self._on_owner_credentials( + tool_data, resolved, action_name + ): from docsgpt.connectors.permissions import ACCESS_WRITE, action_access if action_access(tool_data.get("name"), action_data) == ACCESS_WRITE: @@ -1242,7 +1244,7 @@ class ToolExecutor: return None - def _on_owner_credentials(self, tool_data: Dict, resolved) -> bool: + def _on_owner_credentials(self, tool_data: Dict, resolved, action_name: Optional[str] = None) -> bool: """Whether a call would act with credentials the caller doesn't hold. The connection's account when there is one, else the tool owner's @@ -1253,6 +1255,8 @@ class ToolExecutor: Args: tool_data: The ``user_tools`` row being called. resolved: The connection ``resolve_connection`` picked, or None. + action_name: The action called; an API tool action carries its + own headers and query values. Returns: True when the call would use someone else's credentials. @@ -1261,7 +1265,7 @@ class ToolExecutor: if resolved is not None: holder = (resolved.row or {}).get("user_id") - elif holds_owner_credentials(tool_data): + elif holds_owner_credentials(tool_data, action_name): holder = tool_data.get("user_id") else: return False diff --git a/docsgpt/connectors/permissions.py b/docsgpt/connectors/permissions.py index aae81bbd..789f3ce0 100644 --- a/docsgpt/connectors/permissions.py +++ b/docsgpt/connectors/permissions.py @@ -115,24 +115,46 @@ def tool_actions(tool: dict) -> list[dict]: return [action for action in tool.get("actions") or [] if isinstance(action, dict)] -def holds_owner_credentials(tool: dict) -> bool: - """Whether ``tool`` acts with credentials stored by its owner. +# Where an API tool keeps the values it sends: header and query values are +# the owner's (sealed per action, flagged ``has_value``; legacy rows hold them +# in plain ``value``). Mirrors ``tool_executor.API_TOOL_SECRET_SECTIONS``. +_API_TOOL_SECRET_SECTIONS = ("headers", "query_params") - A connection's account, an API tool (its headers and query values carry - the owner's keys), a stored secret, or an MCP server the owner signed in - to. Anyone running it acts as the owner there, whoever they are. + +def _api_action_sends_credentials(action: dict) -> bool: + """Whether an API tool action sends a header or query value the owner stored.""" + for section in _API_TOOL_SECRET_SECTIONS: + block = action.get(section) + props = block.get("properties") if isinstance(block, dict) else None + for spec in (props or {}).values(): + if isinstance(spec, dict) and (spec.get("has_value") or spec.get("value") not in (None, "")): + return True + return False + + +def holds_owner_credentials(tool: dict, action_name: Optional[str] = None) -> bool: + """Whether ``tool`` (or its ``action_name``) acts with credentials its owner stored. + + A connection's account, a stored secret, an MCP server the owner signed + in to, or an API tool action that sends a header or query value the + owner saved (a key, a token). Anyone running it acts as the owner there, + whoever they are. An API tool action that sends nothing stored does not. Args: tool: A ``user_tools`` row. + action_name: One action to judge; None judges the tool as a whole. Returns: - True when the tool runs on the owner's credentials. + True when the tool (or that action) runs on the owner's credentials. """ - if tool.get("connection_id") or tool.get("name") == "api_tool": - return True config = tool.get("config") or {} - if config.get("encrypted_credentials"): + if tool.get("connection_id") or config.get("encrypted_credentials"): return True + if tool.get("name") == "api_tool": + actions = config.get("actions") or {} + if action_name is not None: + return _api_action_sends_credentials(actions.get(action_name) or {}) + return any(_api_action_sends_credentials(action or {}) for action in actions.values()) return tool.get("name") == "mcp_tool" and (config.get("auth_type") or "none") != "none" @@ -149,10 +171,9 @@ def owner_credential_writes(tool: dict) -> list[str]: Returns: Action names, empty when the tool holds no owner credentials. """ - if not holds_owner_credentials(tool): - return [] return [ action["name"] for action in tool_actions(tool) if action.get("name") and action.get("active") is not False and action_access(tool.get("name"), action) == ACCESS_WRITE + and holds_owner_credentials(tool, action["name"]) ] diff --git a/tests/api/user/test_resource_sponsors.py b/tests/api/user/test_resource_sponsors.py index 7fd89042..d79cd29d 100644 --- a/tests/api/user/test_resource_sponsors.py +++ b/tests/api/user/test_resource_sponsors.py @@ -491,9 +491,10 @@ class TestToolPrefetch: def test_api_key_callers_prefetch_like_someone_else(self, pg_conn): """A widget or API run carries the owner's id, but the caller is not the owner.""" agent_id, _ = _agent(pg_conn) + key = {"type": "object", "properties": {"X-Key": {"type": "string", "value": "", "has_value": True}}} api = str(UserToolsRepository(pg_conn).create(OWNER, "api_tool", config={"actions": { - "status": {"url": "https://x.test/s", "method": "GET", "active": True}, - "notify": {"url": "https://x.test/n", "method": "POST", "active": True}, + "status": {"url": "https://x.test/s", "method": "GET", "active": True, "headers": key}, + "notify": {"url": "https://x.test/n", "method": "POST", "active": True, "headers": key}, }})["id"]) AgentsRepository(pg_conn).update_by_id(agent_id, {"tools": [api]}) required = {"api_tool": {None}} diff --git a/tests/connectors/test_public_link_callers.py b/tests/connectors/test_public_link_callers.py index 7d6bfc85..25f93b66 100644 --- a/tests/connectors/test_public_link_callers.py +++ b/tests/connectors/test_public_link_callers.py @@ -244,9 +244,18 @@ def _stored_tool(name: str, action: dict, config: dict) -> dict: return {"id": "tool-9", "user_id": "alice", "name": name, "config": config, "actions": [action]} -def _api_tool(method: str) -> dict: - tool = _stored_tool("api_tool", {}, {"actions": {"call": {"url": "https://x.test", "method": method, - "active": True, "require_approval": False}}}) +def _api_tool(method: str, header: dict | None = None, **config) -> dict: + """An API tool with one ``call`` action; ``header`` is its Authorization header spec. + + By default the header holds a sealed secret (``has_value``), the way a + saved key is stored. + """ + action = {"url": "https://x.test", "method": method, "active": True, "require_approval": False} + if header is None: + header = {"type": "string", "value": "", "has_value": True} + if header: + action["headers"] = {"type": "object", "properties": {"Authorization": header}} + tool = _stored_tool("api_tool", {}, {"actions": {"call": action}, **config}) tool["actions"] = [] return tool @@ -266,6 +275,23 @@ class TestOwnerHeldCredentialsWithoutAConnection: assert pause["pause_type"] == "headless_denied" assert "Access details" in pause["deny_reason"] + @pytest.mark.parametrize("header", [ + {"type": "string", "value": "", "has_value": True}, + {"type": "string", "value": "Bearer legacy-plaintext"}, + ]) + def test_api_tool_with_a_stored_header_value_is_gated(self, header): + pause = _pause(self._caller(public_link_caller=True), _api_tool("POST", header), action="call") + assert pause["pause_type"] == "headless_denied" + + def test_api_tool_with_stored_credentials_in_config_is_gated(self): + tool = _api_tool("POST", {}, encrypted_credentials="blob") + assert _pause(self._caller(public_link_caller=True), tool, action="call")["pause_type"] == "headless_denied" + + @pytest.mark.parametrize("header", [{}, {"type": "string", "value": "", "has_value": False}, + {"type": "string", "filled_by_llm": True}]) + def test_api_tool_without_credentials_is_not_gated(self, header): + assert _pause(self._caller(public_link_caller=True), _api_tool("POST", header), action="call") is None + def test_api_tool_read_runs(self): assert _pause(self._caller(public_link_caller=True), _api_tool("GET"), action="call") is None diff --git a/tests/connectors/test_tool_routes.py b/tests/connectors/test_tool_routes.py index 10aee8f4..7535f079 100644 --- a/tests/connectors/test_tool_routes.py +++ b/tests/connectors/test_tool_routes.py @@ -313,9 +313,12 @@ class TestOwnerCredentialWrites: from docsgpt.storage.db.repositories.user_tools import UserToolsRepository repo = UserToolsRepository(pg_conn) + key = {"type": "object", "properties": {"X-Key": {"type": "string", "value": "", "has_value": True}}} repo.create("alice", "api_tool", config={"actions": { - "status": {"url": "https://x.test/s", "method": "GET", "active": True}, - "notify": {"url": "https://x.test/n", "method": "POST", "active": True}, + "status": {"url": "https://x.test/s", "method": "GET", "active": True, "headers": key}, + "notify": {"url": "https://x.test/n", "method": "POST", "active": True, "headers": key}, + # A write that sends nothing of the owner's is not listed. + "ping": {"url": "https://x.test/p", "method": "POST", "active": True}, }}) repo.create("alice", "mcp_tool", config={"server_url": "https://m.test/mcp", "auth_type": "bearer"}, actions=[{"name": "create_issue", "active": True}, {"name": "list_issues", "active": True}])