From 4a774bd949143d4dbd43b7f382f9efd220126c68 Mon Sep 17 00:00:00 2001 From: ashvinctrl Date: Sat, 18 Jul 2026 13:42:48 +0530 Subject: [PATCH 1/2] fix(security): don't let manage_settings re-enable disabled tools The manage_settings agent tool exposed an enable_tool action that edited settings.json:disabled_tools directly. disabled_tools is the admin's only global tool-permission boundary, and this handler runs with no admin check while its arguments can be steered by prompt injection (any page or document the agent reads). So a single compromised turn could call enable_tool {"tool":"shell"} and hand itself bash - or clear the whole denylist by re-enabling manage_settings itself - undoing every restriction the admin set in Settings. Make the agent-facing tool-toggle monotonic: disable_tool (tightening) and list_tools stay, but enable_tool no longer mutates the denylist. It reports that re-enabling is an admin action done in Settings -> Agent Tools, which is the require_admin POST /tools route - mirroring how secrets in this same handler are already read-only/panel-only. Re-enabling from the admin UI is unchanged. set/reset/delete already reject the disabled_tools key (not in DEFAULT_SETTINGS), so enable_tool was the only loosening vector. --- src/agent_tools/admin_tools.py | 33 +++++-- src/tool_schemas.py | 2 +- ...st_manage_settings_enable_tool_boundary.py | 90 +++++++++++++++++++ tests/test_review_regressions.py | 10 ++- 4 files changed, 124 insertions(+), 11 deletions(-) create mode 100644 tests/test_manage_settings_enable_tool_boundary.py diff --git a/src/agent_tools/admin_tools.py b/src/agent_tools/admin_tools.py index 2cd6dc1a8f..7b6b13e137 100644 --- a/src/agent_tools/admin_tools.py +++ b/src/agent_tools/admin_tools.py @@ -730,6 +730,27 @@ def _mask(k, v): "exit_code": 0, } + if action == "enable_tool": + # Re-enabling a globally disabled tool loosens the admin's only + # hard permission boundary. This handler runs with no admin + # check and its arguments can be steered by prompt injection + # (any page or document the agent reads), so clearing entries + # from disabled_tools would let a compromised turn hand itself + # bash/web/etc. Enabling is therefore an admin action, done in + # Settings -> Agent Tools (POST /tools, require_admin). Disabling + # (tightening) and listing stay available here, mirroring how + # secrets in this same handler are read-only/panel-only. + current = get_setting("disabled_tools", []) or [] + return { + "response": ( + "Re-enabling a globally disabled tool is an admin action - " + "turn it back on in Settings -> Agent Tools. " + f"Currently disabled: {', '.join(current) if current else '(none)'}." + ), + "disabled": list(current), + "exit_code": 0, + } + tool_name = (args.get("tool") or args.get("name") or "").strip().lower() if not tool_name: return {"error": "tool name required (e.g. 'shell', 'search', 'bash')", "exit_code": 1} @@ -738,21 +759,17 @@ def _mask(k, v): settings = load_settings() current = list(settings.get("disabled_tools") or []) before = set(current) - if action == "disable_tool": - for t in targets: - if t not in current: - current.append(t) - else: # enable_tool - current = [t for t in current if t not in targets] + for t in targets: # disable_tool only; enable_tool handled above + if t not in current: + current.append(t) after = set(current) settings["disabled_tools"] = current save_settings(settings) - verb = "Disabled" if action == "disable_tool" else "Enabled" changed = sorted(after.symmetric_difference(before)) return { "response": ( - f"{verb} {tool_name} ({', '.join(targets)}). " + f"Disabled {tool_name} ({', '.join(targets)}). " f"Now disabled: {', '.join(current) if current else '(none)'}." ), "changed": changed, diff --git a/src/tool_schemas.py b/src/tool_schemas.py index 6adb087c52..8dbf318a73 100644 --- a/src/tool_schemas.py +++ b/src/tool_schemas.py @@ -780,7 +780,7 @@ "type": "function", "function": { "name": "manage_settings", - "description": "Manage user preferences and settings. Use `disable_tool`/`enable_tool`/`list_tools` to turn individual tools on or off globally (e.g. shell, search, browser, documents, memory, skills, images, tasks, notes, calendar, email). Use list/get/set/delete for free-form preferences.", + "description": "Manage user preferences and settings. Use `disable_tool` to turn a tool off globally and `list_tools` to see what is disabled (e.g. shell, search, browser, documents, memory, skills, images, tasks, notes, calendar, email). Re-enabling a globally disabled tool is an admin action done in Settings, not from here — `enable_tool` reports this rather than loosening the denylist. Use list/get/set/delete for free-form preferences.", "parameters": { "type": "object", "properties": { diff --git a/tests/test_manage_settings_enable_tool_boundary.py b/tests/test_manage_settings_enable_tool_boundary.py new file mode 100644 index 0000000000..54de705d99 --- /dev/null +++ b/tests/test_manage_settings_enable_tool_boundary.py @@ -0,0 +1,90 @@ +"""Regression: the manage_settings agent tool must not re-enable disabled tools. + +`disabled_tools` is the admin's only global tool-permission boundary. The +agent-facing manage_settings handler runs with no admin check and its arguments +can be steered by prompt injection, so it must be able to tighten the denylist +(disable_tool) but never loosen it (enable_tool). Re-enabling stays an admin +action in Settings -> Agent Tools (POST /tools, require_admin). +""" +import asyncio +import json + +import src.settings as settings_mod +from src.agent_tools.admin_tools import do_manage_settings + + +def _patch_store(monkeypatch, store): + monkeypatch.setattr(settings_mod, "load_settings", lambda: dict(store)) + monkeypatch.setattr(settings_mod, "save_settings", lambda s: (store.clear(), store.update(s))) + monkeypatch.setattr(settings_mod, "get_setting", lambda k, d=None: store.get(k, d)) + + +def test_enable_tool_does_not_re_enable_disabled_bash(monkeypatch): + # Admin has globally disabled shell/bash. + store = {"disabled_tools": ["bash"]} + _patch_store(monkeypatch, store) + + result = asyncio.run(do_manage_settings(json.dumps({ + "action": "enable_tool", "tool": "shell", + }))) + + # The tool reports success (exit 0) but must NOT clear bash from the denylist. + assert result.get("exit_code") == 0, result + assert store["disabled_tools"] == ["bash"], result + assert "bash" in result.get("disabled", []), result + # It should point the user at the admin path, not silently enable. + assert "admin" in result.get("response", "").lower(), result + + +def test_enable_tool_cannot_undo_manage_settings_self_disable(monkeypatch): + # Self-referential: once manage_settings is disabled, the agent tool cannot + # remove itself from the denylist to regain the ability to loosen it. + store = {"disabled_tools": ["manage_settings"]} + _patch_store(monkeypatch, store) + + result = asyncio.run(do_manage_settings(json.dumps({ + "action": "enable_tool", "tool": "manage_settings", + }))) + + assert result.get("exit_code") == 0, result + assert store["disabled_tools"] == ["manage_settings"], result + + +def test_enable_tool_does_not_re_enable_benign_tool(monkeypatch): + # The block is uniform: disabled_tools encodes a deliberate operator choice + # for EVERY entry, not just dangerous ones. An operator who disabled image + # generation (e.g. to control API cost) keeps that choice until they + # re-enable it themselves in Settings; an injected turn cannot override it. + store = {"disabled_tools": ["generate_image"]} + _patch_store(monkeypatch, store) + + result = asyncio.run(do_manage_settings(json.dumps({ + "action": "enable_tool", "tool": "images", + }))) + + assert result.get("exit_code") == 0, result + assert store["disabled_tools"] == ["generate_image"], result + + +def test_disable_tool_still_works(monkeypatch): + # Tightening the denylist stays available from chat. + store = {"disabled_tools": []} + _patch_store(monkeypatch, store) + + result = asyncio.run(do_manage_settings(json.dumps({ + "action": "disable_tool", "tool": "shell", + }))) + + assert result.get("exit_code") == 0, result + assert "bash" in store["disabled_tools"], result + assert "bash" in result.get("changed", []), result + + +def test_list_tools_still_works(monkeypatch): + store = {"disabled_tools": ["bash", "web_search"]} + _patch_store(monkeypatch, store) + + result = asyncio.run(do_manage_settings(json.dumps({"action": "list_tools"}))) + + assert result.get("exit_code") == 0, result + assert set(result.get("disabled", [])) == {"bash", "web_search"}, result diff --git a/tests/test_review_regressions.py b/tests/test_review_regressions.py index 58c2e8a216..bad75ea057 100644 --- a/tests/test_review_regressions.py +++ b/tests/test_review_regressions.py @@ -766,12 +766,18 @@ def fake_save_settings(s): assert tool_name in disabled, tool_name assert f"mcp__email__{tool_name}" in disabled, tool_name - # enable_tool email must remove the full set again. + # enable_tool must NOT loosen the global denylist from the agent surface: + # re-enabling a globally disabled tool is an admin action done in Settings + # (POST /tools, require_admin), so the full set stays disabled here and the + # tool points the caller at the panel instead. See test_manage_settings_ + # enable_tool_boundary.py for the dedicated regression. + disabled_before = set(store["disabled_tools"]) result = await do_manage_settings( '{"action": "enable_tool", "tool": "email"}', owner="admin" ) assert result["exit_code"] == 0 - assert store["disabled_tools"] == [] + assert set(store["disabled_tools"]) == disabled_before + assert "admin" in result.get("response", "").lower() def _install_admin_auth_stub(monkeypatch): From 8a8444f29d294d7e226b8f77d6d57d2300910159 Mon Sep 17 00:00:00 2001 From: ashvinctrl Date: Mon, 20 Jul 2026 19:06:32 +0530 Subject: [PATCH 2/2] fix(security): close app_api bypass of the tool denylist + align descriptions RaresKeY review on #5594: - P1: the manage_settings guard only covered the named tool. app_api could still POST /api/tools, which rewrites disabled_tools wholesale, and that route's require_admin is satisfied by the loopback identity app_api rides. Block POST /api/tools in the app_api method/path denylist (GET stays, it's a read) so re-enabling remains a human Settings action. Adds a regression that proves the write is rejected before any loopback request. - P3: the served prompt, compact tool index, and the tool param schema still told the agent it could enable tools. Reworded to the one-way toggle (disable/list here; re-enabling is admin-only in Settings) and added a drift regression across all three surfaces. - P3 nit: collapsed the repeated security rationale into one owning comment on the enable_tool handler and trimmed the test comments to their invariant. --- src/agent_loop.py | 4 +- src/agent_tools/admin_tools.py | 15 +++--- src/tool_index.py | 2 +- src/tool_schemas.py | 2 +- src/tools/system.py | 9 ++++ tests/test_app_api_tools_denylist.py | 33 ++++++++++++ ...st_manage_settings_enable_tool_boundary.py | 52 +++++++++++++------ tests/test_review_regressions.py | 7 +-- 8 files changed, 90 insertions(+), 34 deletions(-) create mode 100644 tests/test_app_api_tools_denylist.py diff --git a/src/agent_loop.py b/src/agent_loop.py index 6f7dde6054..bff073c87b 100644 --- a/src/agent_loop.py +++ b/src/agent_loop.py @@ -306,7 +306,7 @@ def _load_mcp_disabled_map() -> Dict[str, set]: - Use `edit_file`/`write_file` for writes; avoid shell redirection/heredocs for editing files.""", "settings": """\ ## Settings/API rules -- Use `manage_settings` for preferences and tool enable/disable. +- Use `manage_settings` for preferences and to turn a tool off / list disabled tools (re-enabling a disabled tool is admin-only, in Settings). - Use named tools over `app_api` when a named wrapper exists. - `app_api` is only for safe UI/API actions without a named tool; do not use it for shell, package installs, engine rebuilds, or sensitive auth/admin paths.""", "contacts": """\ @@ -470,7 +470,7 @@ def _domain_rules_for_tools(tool_names: set) -> list[str]: "manage_tokens": "- ```manage_tokens``` — Generate or revoke API access tokens for external integrations. Args (JSON): {\"action\": \"list|create|delete\", ...}", "manage_documents": "- ```manage_documents``` — List, read/open, delete, or tidy documents in the editor panel. Args (JSON): {\"action\": \"list|read|delete|tidy\", ...}. `list` returns rows like `[Title](#document-) — lang, size, updated 5m ago` sorted MOST-RECENT FIRST; the user clicks the anchor to open. `read` (aliases: view/open/get) takes `document_id` and returns the content. When the user asks \"open/show/read my notes\" or \"what documents do I have\", use this — do NOT shell out, do NOT curl.", "manage_research": "- ```manage_research``` — List, read/open, or delete saved DEEP RESEARCH results from the Library. Args (JSON): {\"action\": \"list|read|delete\", \"id\": \"\", \"search\": \"...\"}. `list` returns rows like `[query](#research-) — N sources` MOST-RECENT FIRST; the user clicks to open. `read` (aliases: open/view/get) takes `id` and returns the report text + sources. Use when the user says \"open/read/find/delete my research\" or \"that report\". This IS how you read a finished report: when the user refers to a just-completed deep-research job (\"check it out\", \"read that report\", \"summarize the research\") WITHOUT giving an id, call `manage_research` with `action:list` to get the most-recent id, then `action:read` with that id, and answer from the returned text. Do NOT `web_fetch`/`app_api` the `/api/research/report/{id}` URL — that endpoint renders HTML for the browser, not clean text — and do NOT start a fresh `web_search`/`trigger_research` just to read an existing report. To START new research, use trigger_research instead.", - "manage_settings": "- ```manage_settings``` — View/change the REAL app settings (same ones the Settings panel writes) AND turn tools on/off. Change a setting: `{\"action\":\"set\",\"key\":\"...\",\"value\":\"...\"}` — keys accept friendly aliases, e.g. voice→tts_voice, \"search engine\"→search_provider, \"default model\"→default_model, \"teacher model\"→teacher_model, \"task/background model\"→task_model, \"image quality\"→image_quality, \"reminder channel\"→reminder_channel (browser|email|ntfy), \"agent timeout\"/\"max tool calls\"/\"token budget\". Read: `{\"action\":\"get\",\"key\":\"...\"}`; see all: `{\"action\":\"list\"}`; reset one: `{\"action\":\"reset\",\"key\":\"...\"}`. Use this when the user asks to change ANY preference instead of making them open Settings. Secrets/API keys are read-only (tell them to set those in the panel). Tool toggles: `{\"action\":\"disable_tool|enable_tool\",\"tool\":\"shell\"}` (aliases: shell/search/browser/documents/memory/skills/images/tasks/notes/calendar/email), list disabled: `{\"action\":\"list_tools\"}`.", + "manage_settings": "- ```manage_settings``` — View/change the REAL app settings (same ones the Settings panel writes) AND turn individual tools off globally. Change a setting: `{\"action\":\"set\",\"key\":\"...\",\"value\":\"...\"}` — keys accept friendly aliases, e.g. voice→tts_voice, \"search engine\"→search_provider, \"default model\"→default_model, \"teacher model\"→teacher_model, \"task/background model\"→task_model, \"image quality\"→image_quality, \"reminder channel\"→reminder_channel (browser|email|ntfy), \"agent timeout\"/\"max tool calls\"/\"token budget\". Read: `{\"action\":\"get\",\"key\":\"...\"}`; see all: `{\"action\":\"list\"}`; reset one: `{\"action\":\"reset\",\"key\":\"...\"}`. Use this when the user asks to change ANY preference instead of making them open Settings. Secrets/API keys are read-only (tell them to set those in the panel). Disable a tool: `{\"action\":\"disable_tool\",\"tool\":\"shell\"}` (aliases: shell/search/browser/documents/memory/skills/images/tasks/notes/calendar/email); list disabled: `{\"action\":\"list_tools\"}`. Re-enabling a globally disabled tool is admin-only — the user does it in Settings -> Agent Tools, not from here.", "manage_notes": """\ ```manage_notes {"action": "add", "title": "", "due_date": ""} diff --git a/src/agent_tools/admin_tools.py b/src/agent_tools/admin_tools.py index 7b6b13e137..076ae366bb 100644 --- a/src/agent_tools/admin_tools.py +++ b/src/agent_tools/admin_tools.py @@ -731,15 +731,12 @@ def _mask(k, v): } if action == "enable_tool": - # Re-enabling a globally disabled tool loosens the admin's only - # hard permission boundary. This handler runs with no admin - # check and its arguments can be steered by prompt injection - # (any page or document the agent reads), so clearing entries - # from disabled_tools would let a compromised turn hand itself - # bash/web/etc. Enabling is therefore an admin action, done in - # Settings -> Agent Tools (POST /tools, require_admin). Disabling - # (tightening) and listing stay available here, mirroring how - # secrets in this same handler are read-only/panel-only. + # One-way boundary: disabled_tools is the operator's global + # tool-permission denylist, and this handler runs with no admin + # check on prompt-injectable args — so it may tighten the list + # (disable_tool) but must never loosen it. Re-enabling is an + # admin action in Settings -> Agent Tools; the generic API route + # is fenced in parallel (app_api blocks POST /api/tools). current = get_setting("disabled_tools", []) or [] return { "response": ( diff --git a/src/tool_index.py b/src/tool_index.py index 2ac95a92a1..96fa34db65 100644 --- a/src/tool_index.py +++ b/src/tool_index.py @@ -98,7 +98,7 @@ "manage_tokens": "API token management: list, create, or delete API access tokens.", "manage_documents": "List, read, delete, or tidy documents in the editor panel. action='list' returns clickable rows (most-recent first) so the user can open any doc by clicking. action='read' (aka view/open/get) with document_id returns the content; supports offset= + limit= to page through large docs (response includes next_offset when more remains, so you can keep calling with offset=next_offset). action='delete' with document_id removes a doc (only way to delete). Use this for ANY 'show/read/list/open my documents/docs/files/notes' request — never shell or curl.", "manage_research": "List, read/open, or delete saved DEEP RESEARCH results from the Library. action='list' returns clickable [query](#research-) rows (most-recent first). action='read' (aka open/view/get) with id returns the report + sources. action='delete' with id removes it. Use this for ANY 'open/read/find/delete my research / that report / the research on X' request. NOTE: this is for EXISTING research; to START new research use trigger_research.", - "manage_settings": "Change ANY real app setting (the ones the Settings panel writes) so the user never has to open it: TTS voice/provider/speed, STT, search engine + result count, default/teacher/task/utility/vision/image/research models, image quality, reminder channel (browser/email/ntfy), agent timeout/tool-call budget, and more. action=set with key (friendly aliases ok: voice, 'search engine', 'default model', 'teacher model', 'image quality', 'reminder channel'...) + value; get/list/reset too. Also toggles tools on/off (disable_tool/enable_tool/list_tools). Secrets/API keys are read-only. Use for any 'change my…/set my…/use X for…/turn on…' preference request.", + "manage_settings": "Change ANY real app setting (the ones the Settings panel writes) so the user never has to open it: TTS voice/provider/speed, STT, search engine + result count, default/teacher/task/utility/vision/image/research models, image quality, reminder channel (browser/email/ntfy), agent timeout/tool-call budget, and more. action=set with key (friendly aliases ok: voice, 'search engine', 'default model', 'teacher model', 'image quality', 'reminder channel'...) + value; get/list/reset too. Also turns individual tools off globally (disable_tool/list_tools); re-enabling a disabled tool is admin-only, done in Settings. Secrets/API keys are read-only. Use for any 'change my…/set my…/use X for…/turn on…' preference request.", "create_session": "Create a new chat with a name and model.", "list_sessions": "List all chats with their metadata (the UI calls these 'chats'). Use for 'list my chats', 'rename all my chats' (list first, then manage_session to rename each).", "send_to_session": "Send a message to another chat. Cross-chat communication.", diff --git a/src/tool_schemas.py b/src/tool_schemas.py index 8dbf318a73..de6c8cb1a7 100644 --- a/src/tool_schemas.py +++ b/src/tool_schemas.py @@ -787,7 +787,7 @@ "action": {"type": "string", "enum": ["list", "get", "set", "delete", "disable_tool", "enable_tool", "list_tools"]}, "key": {"type": "string", "description": "Setting key (for get/set/delete)"}, "value": {"description": "Setting value (for set) — can be string, number, boolean, or object"}, - "tool": {"type": "string", "description": "Tool name to disable/enable (for disable_tool/enable_tool). Accepts aliases: shell, search, browser, documents, memory, skills, images, tasks, notes, calendar, email — or a raw tool name like 'bash' or 'web_search'."} + "tool": {"type": "string", "description": "Tool name to disable (for disable_tool). Accepts aliases: shell, search, browser, documents, memory, skills, images, tasks, notes, calendar, email — or a raw tool name like 'bash' or 'web_search'. (enable_tool does not loosen the denylist — re-enabling is admin-only, in Settings.)"} }, "required": ["action"] } diff --git a/src/tools/system.py b/src/tools/system.py index a901992c26..971694e899 100644 --- a/src/tools/system.py +++ b/src/tools/system.py @@ -557,6 +557,13 @@ async def do_api_call(content: str) -> Dict: ("POST", "/api/calendar/events"), ("PUT", "/api/calendar/events"), ("DELETE", "/api/calendar/events"), + # disabled_tools is the operator's global tool-permission boundary. POST + # /api/tools rewrites it wholesale, and that route's require_admin is + # satisfied by the loopback identity these calls ride — so without this an + # injected turn could re-enable a tool the operator disabled. Loosening the + # denylist stays a human action in Settings -> Agent Tools. (GET /api/tools + # is a read, left callable.) + ("POST", "/api/tools"), ) @@ -648,6 +655,8 @@ async def do_app_api(content: str, owner: Optional[str] = None) -> Dict: if method not in ("GET", "POST", "PUT", "PATCH", "DELETE"): return {"error": f"Unsupported method: {method}", "exit_code": 1} if any(method == m and path.startswith(p) for m, p in _APP_API_BLOCKLIST_METHOD_PATH): + if path.rstrip("/") == "/api/tools": + return {"error": "Don't POST /api/tools via app_api — that rewrites the global tool denylist, which is an admin-only action. Re-enable a disabled tool in Settings -> Agent Tools. From chat you can only tighten the denylist, via manage_settings disable_tool.", "exit_code": 1} if "/api/email/accounts" in path: return {"error": "Don't use /api/email/accounts via app_api — it is owner-filtered in tool context and may return empty. Use the `list_email_accounts` email tool, then pass `account` to list_emails/read_email.", "exit_code": 1} if "/api/cookbook/packages/install" in path: diff --git a/tests/test_app_api_tools_denylist.py b/tests/test_app_api_tools_denylist.py new file mode 100644 index 0000000000..0baca9e5eb --- /dev/null +++ b/tests/test_app_api_tools_denylist.py @@ -0,0 +1,33 @@ +"""Regression: the generic app_api bridge must not rewrite the global tool +denylist. manage_settings already refuses enable_tool, but POST /api/tools +writes the same disabled_tools list and its require_admin is satisfied by the +loopback identity app_api rides, so the bridge has to fence the write too. +""" +import asyncio +import json + +import httpx + +from src.tools.system import do_app_api, _APP_API_BLOCKLIST_METHOD_PATH + + +def test_post_api_tools_is_blocked_before_loopback(monkeypatch): + # Make any outbound loopback fail loudly, so this proves the block returns + # before a request is issued rather than relying on the server to reject it. + def _no_http(*args, **kwargs): + raise AssertionError("app_api reached the network for a blocked write") + + monkeypatch.setattr(httpx, "AsyncClient", _no_http) + + result = asyncio.run(do_app_api(json.dumps({ + "path": "/api/tools", "method": "POST", "body": {"disabled": []}, + }))) + + assert result.get("exit_code") == 1, result + assert "settings" in result.get("error", "").lower(), result + + +def test_block_is_write_only(): + # Only the mutation is fenced; GET /api/tools (a read) stays callable. + assert ("POST", "/api/tools") in _APP_API_BLOCKLIST_METHOD_PATH + assert ("GET", "/api/tools") not in _APP_API_BLOCKLIST_METHOD_PATH diff --git a/tests/test_manage_settings_enable_tool_boundary.py b/tests/test_manage_settings_enable_tool_boundary.py index 54de705d99..2b0b1a2a35 100644 --- a/tests/test_manage_settings_enable_tool_boundary.py +++ b/tests/test_manage_settings_enable_tool_boundary.py @@ -1,10 +1,6 @@ -"""Regression: the manage_settings agent tool must not re-enable disabled tools. - -`disabled_tools` is the admin's only global tool-permission boundary. The -agent-facing manage_settings handler runs with no admin check and its arguments -can be steered by prompt injection, so it must be able to tighten the denylist -(disable_tool) but never loosen it (enable_tool). Re-enabling stays an admin -action in Settings -> Agent Tools (POST /tools, require_admin). +"""Regression: manage_settings can tighten the disabled_tools denylist but +never loosen it. Rationale for the boundary lives on the enable_tool handler +in src/agent_tools/admin_tools.py. """ import asyncio import json @@ -20,7 +16,6 @@ def _patch_store(monkeypatch, store): def test_enable_tool_does_not_re_enable_disabled_bash(monkeypatch): - # Admin has globally disabled shell/bash. store = {"disabled_tools": ["bash"]} _patch_store(monkeypatch, store) @@ -28,17 +23,15 @@ def test_enable_tool_does_not_re_enable_disabled_bash(monkeypatch): "action": "enable_tool", "tool": "shell", }))) - # The tool reports success (exit 0) but must NOT clear bash from the denylist. + # Reports exit 0 but leaves the denylist untouched and points at the admin path. assert result.get("exit_code") == 0, result assert store["disabled_tools"] == ["bash"], result assert "bash" in result.get("disabled", []), result - # It should point the user at the admin path, not silently enable. assert "admin" in result.get("response", "").lower(), result def test_enable_tool_cannot_undo_manage_settings_self_disable(monkeypatch): - # Self-referential: once manage_settings is disabled, the agent tool cannot - # remove itself from the denylist to regain the ability to loosen it. + # Once manage_settings is disabled it can't remove itself from the denylist. store = {"disabled_tools": ["manage_settings"]} _patch_store(monkeypatch, store) @@ -51,10 +44,8 @@ def test_enable_tool_cannot_undo_manage_settings_self_disable(monkeypatch): def test_enable_tool_does_not_re_enable_benign_tool(monkeypatch): - # The block is uniform: disabled_tools encodes a deliberate operator choice - # for EVERY entry, not just dangerous ones. An operator who disabled image - # generation (e.g. to control API cost) keeps that choice until they - # re-enable it themselves in Settings; an injected turn cannot override it. + # The block is uniform: every entry is a deliberate operator choice, not + # just the dangerous ones (here, image generation kept off for cost). store = {"disabled_tools": ["generate_image"]} _patch_store(monkeypatch, store) @@ -88,3 +79,32 @@ def test_list_tools_still_works(monkeypatch): assert result.get("exit_code") == 0, result assert set(result.get("disabled", [])) == {"bash", "web_search"}, result + + +def test_agent_descriptions_do_not_advertise_re_enabling(): + # The served prompt, compact index, and schema describe a one-way toggle: + # they may name enable_tool only to say it won't loosen the denylist, and + # must never tell the agent it can turn a disabled tool back on. Keeps the + # descriptions from drifting back to "toggle on/off" once behavior changed. + import src.agent_tools # import first: avoids a tool_schemas circular import + from src.tool_schemas import FUNCTION_TOOL_SCHEMAS + from src.tool_index import BUILTIN_TOOL_DESCRIPTIONS + from src.agent_loop import TOOL_SECTIONS, _DOMAIN_RULES + + schema_desc = next( + t["function"]["description"] + for t in FUNCTION_TOOL_SCHEMAS + if t["function"]["name"] == "manage_settings" + ) + surfaces = { + "schema": schema_desc, + "index": BUILTIN_TOOL_DESCRIPTIONS["manage_settings"], + "prompt_section": TOOL_SECTIONS["manage_settings"], + "domain_rules": "\n".join(_DOMAIN_RULES.values()), + } + forbidden = ("tools on/off", "tools on or off", "turn tools on", + "enable/disable", "disable/enable", "disable_tool|enable_tool") + for name, text in surfaces.items(): + low = text.lower() + for phrase in forbidden: + assert phrase not in low, f"{name} still advertises enabling: {phrase!r}" diff --git a/tests/test_review_regressions.py b/tests/test_review_regressions.py index bad75ea057..f02e9c14cf 100644 --- a/tests/test_review_regressions.py +++ b/tests/test_review_regressions.py @@ -766,11 +766,8 @@ def fake_save_settings(s): assert tool_name in disabled, tool_name assert f"mcp__email__{tool_name}" in disabled, tool_name - # enable_tool must NOT loosen the global denylist from the agent surface: - # re-enabling a globally disabled tool is an admin action done in Settings - # (POST /tools, require_admin), so the full set stays disabled here and the - # tool points the caller at the panel instead. See test_manage_settings_ - # enable_tool_boundary.py for the dedicated regression. + # enable_tool must not loosen the denylist, even for an admin owner: the + # full email set stays disabled and the tool redirects to Settings. disabled_before = set(store["disabled_tools"]) result = await do_manage_settings( '{"action": "enable_tool", "tool": "email"}', owner="admin"