fix(security): don't let manage_settings re-enable disabled tools

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,

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": {

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

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