From 5088eae51876ebf15427fac0f2817f01eea02590 Mon Sep 17 00:00:00 2001 From: ciregenz Date: Sun, 19 Jul 2026 13:49:02 -0700 Subject: [PATCH] [eric] agents: read-only lockdown was bypassable (disallowed_tools clobbered by reassignment; Write could overwrite existing files) - both sealed --- .../agents/manager/permissions/gate_hooks.py | 17 +++++++++++++++++ backend/apps/agents/manager/run/RunOptions.py | 3 +++ 2 files changed, 20 insertions(+) diff --git a/backend/apps/agents/manager/permissions/gate_hooks.py b/backend/apps/agents/manager/permissions/gate_hooks.py index fbd3bb06..d85b683b 100644 --- a/backend/apps/agents/manager/permissions/gate_hooks.py +++ b/backend/apps/agents/manager/permissions/gate_hooks.py @@ -6,6 +6,7 @@ are the claude_agent_sdk hook protocol (hookSpecificOutput), not internal state. import asyncio import logging +import os import time from typing import Dict, Optional, Union @@ -67,6 +68,22 @@ async def pre_tool_hook(ctx: HookContext, input_data: dict, tool_use_id: Optiona tool_name = input_data.get("tool_name", "") hook_event = input_data.get("hook_event_name", "PreToolUse") + # Read-only sessions (onboarding's unattended audit) block Edit/Bash/NotebookEdit at the tool-list + # level, but Write survives so the audit can drop its ONE report file. Write also CLOBBERS an + # existing path, though, so without this a read-only agent could overwrite ~/Downloads/taxes.pdf. + # Deny Write when its target already exists: new report file yes, destroying an existing file no. + if getattr(ctx.session, "read_only", False) and tool_name == "Write": + p = (input_data.get("tool_input") or {}).get("file_path", "") + if p and os.path.exists(os.path.expanduser(p)): + note_tool_used(ctx.session_id, tool_name, False) + return { + "hookSpecificOutput": { + "hookEventName": hook_event, + "permissionDecision": "deny", + "permissionDecisionReason": "This is a read-only audit: it may create a new report but must never overwrite an existing file. Choose a filename that does not exist yet.", + } + } + # ToolSearch loop-breaker. Gated MCP servers are withheld from the SDK until MCPActivate, so the CLI's native ToolSearch can never find them; small models thrash (empty ToolSearch, retry) for minutes until the user pauses. Let the first couple through, then redirect to the gate. Any non-ToolSearch call is real progress, so the counter resets. Gated-server lookup is deferred behind the threshold so the common (non-looping) path stays free. if tool_name == "ToolSearch": ctx.ts_loop_count += 1 diff --git a/backend/apps/agents/manager/run/RunOptions.py b/backend/apps/agents/manager/run/RunOptions.py index 37b7f3da..bab69e7a 100644 --- a/backend/apps/agents/manager/run/RunOptions.py +++ b/backend/apps/agents/manager/run/RunOptions.py @@ -227,7 +227,10 @@ class RunOptions(AgentManagerProtocol): options_kwargs["extra_args"] = p_ea # The claude_code preset auto-attaches the user's claude.ai- connected partner MCPs (`mcp__claude_ai_*`). Those bypass our MCPActivate gate, don't share OAuth state with the OpenSwarm Gmail/Calendar/Drive connectors the user actually configured here, and confuse the model into picking the partner shim instead of our vetted server. Hard-block them at the SDK layer so the model can't even attempt the call. + # EXTEND, never reassign: a plain assignment here silently discarded every effective_disallowed + # entry (read-only sessions could still run Bash). Same fix as eric/dev ce3e67d6. options_kwargs["disallowed_tools"] = [ + *(options_kwargs.get("disallowed_tools") or []), "mcp__claude_ai_*", ]