From b3d43ad2256d1e604f7f7475c4f8e152895ba6f2 Mon Sep 17 00:00:00 2001 From: botinate <285686135+botinate@users.noreply.github.com> Date: Thu, 11 Jun 2026 22:01:52 +0200 Subject: [PATCH] fix(security): reject brace-style fence metadata; cover the full email set in the friendly toggle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up round 3 on #3681 (thanks @RaresKeY): 1. Brace-style fence metadata no longer executes. The previous narrowing still treated any same-line {/[ after a recognized tag as tool input, so ```bash {title="setup"} ran as a bash call. The fence header is now captured separately and judged by one predicate shared between parse_tool_blocks and strip_tool_blocks (_fenced_tool_call), so the execute and display decisions can't disagree: same-line content only counts as inline args when the tag is NOT a code tag (bash/python never take same-line args — that text is Markdown fence attributes) AND the inline text (plus any continuation lines) parses as standalone JSON. ```bash {title="setup"}, ```python {"title":"example.py"} and ```list_emails {title="x"} all stay visible and inert. 2. The friendly `disable_tool email` toggle covered 3 of the 14 email tools (mcp__email__{list_emails,read_email,send_email}); the other bare aliases this PR routes stayed executable after an operator disabled email. The alias now derives from BUILTIN_EMAIL_TOOLS in BOTH spellings — bare (function-schema hiding, bare-fence dispatch) and mcp__email__* (MCP schema hiding, qualified runtime blocks) — so the toggle and the runtime gate can't drift apart. Tests: brace/bracket metadata regressions for parse and strip symmetry (code tags, invalid-JSON inline on a JSON tool, multi-line inline JSON still parsing), and disable_tool/enable_tool email covering all 14 names in both spellings. Co-Authored-By: Claude Fable 5 --- src/tool_implementations.py | 10 +++++- src/tool_parsing.py | 61 +++++++++++++++++++++++++++----- tests/test_fenced_inline_args.py | 39 ++++++++++++++++++++ tests/test_review_regressions.py | 59 ++++++++++++++++++++++++++++++ 4 files changed, 160 insertions(+), 9 deletions(-) diff --git a/src/tool_implementations.py b/src/tool_implementations.py index 27c05f139..38b98d17e 100644 --- a/src/tool_implementations.py +++ b/src/tool_implementations.py @@ -13,6 +13,7 @@ import re from typing import Any, Dict, List, Optional from src.constants import MAX_READ_CHARS, DEEP_RESEARCH_DIR, VAULT_FILE +from src.tool_security import BUILTIN_EMAIL_TOOLS from src.tool_utils import get_mcp_manager from core.constants import internal_api_base @@ -1097,7 +1098,14 @@ async def do_manage_settings(content: str, owner: Optional[str] = None) -> Dict: "tasks": ["manage_tasks"], "notes": ["manage_notes"], "calendar": ["manage_calendar"], - "email": ["mcp__email__list_emails", "mcp__email__read_email", "mcp__email__send_email"], + # The full built-in email tool set, in BOTH spellings: the + # qualified mcp__email__* names drive MCP schema hiding, the + # bare names drive function-schema hiding, and the runtime + # gate accepts either — deriving from BUILTIN_EMAIL_TOOLS + # keeps the toggle covering every tool the email server + # exposes instead of a hand-picked subset. + "email": sorted(BUILTIN_EMAIL_TOOLS) + + [f"mcp__email__{t}" for t in sorted(BUILTIN_EMAIL_TOOLS)], "research": ["web_search"], # research is a per-request flag, not a tool — closest analog } diff --git a/src/tool_parsing.py b/src/tool_parsing.py index cc1078082..67c949a7e 100644 --- a/src/tool_parsing.py +++ b/src/tool_parsing.py @@ -21,18 +21,61 @@ logger = logging.getLogger(__name__) # Pattern 1: ```bash ... ``` fenced code blocks. The tag may be followed by a # newline (classic form) or by inline JSON args on the same line -# (```list_email_accounts {}). Same-line content is accepted ONLY when it -# starts with { or [ — anything else after the tag is a Markdown info string -# (```python title="example.py"), which must stay display text rather than -# become executable tool input. +# (```list_email_accounts {}). The same-line part is captured separately +# (group 2) and judged by _fenced_tool_call below — the regex alone only +# requires it to start with { or [; anything else after the tag is a Markdown +# info string (```python title="example.py") and the fence never matches. # (?![\w-]) keeps the alternation from prefix-matching longer fence tags: # without it, ```python3 would match as tool "python" with content "3\n..." # and execute as code. _TOOL_BLOCK_RE = re.compile( - r"```(" + "|".join(TOOL_TAGS) + r")(?![\w-])[ \t]*(?=\r?\n|[{\[])\r?\n?([\s\S]*?)```", + r"```(" + "|".join(TOOL_TAGS) + r")(?![\w-])" + r"[ \t]*([{\[][^\n]*?)?[ \t]*(?=\r?\n|```)\r?\n?([\s\S]*?)```", re.IGNORECASE, ) +# Tags whose fenced content is raw code, not JSON args. Same-line text after +# these tags is Markdown fence metadata on a real language (```bash {title= +# "setup"}), never inline tool args — only the classic tag-then-newline form +# executes for them. +_CODE_FENCE_TAGS = frozenset({"bash", "python"}) + + +def _fenced_tool_call(m) -> Optional[tuple]: + """Classify a Pattern-1 fence match: (tag, content) when it is an + executable tool call, None when the fence must stay display text. + + Shared by parse_tool_blocks and strip_tool_blocks so the execute and + display decisions can never disagree: a fence that doesn't execute is + never stripped, and vice versa. + + Same-line text after the tag only counts as inline tool args when the + tag's tool takes JSON args (not a code tag) AND the text is valid + standalone JSON. ```bash {title="setup"} and ```python {"x": 1} are + fence attributes on real languages, and {title="x"} on any tag is + metadata, not arguments — all of those stay visible and inert. + """ + tag = m.group(1).lower() + inline = (m.group(2) or "").strip() + body = (m.group(3) or "").strip() + if not inline: + return tag, body + if tag in _CODE_FENCE_TAGS: + return None + # Inline args may continue onto following lines (a JSON object opened on + # the tag line); the combined text must parse as JSON or nothing runs. + content = f"{inline}\n{body}" if body else inline + try: + json.loads(content) + except (ValueError, TypeError): + return None + return tag, content + + +def _strip_executed_fence(m) -> str: + """re.sub callback: remove only fences that parse as tool calls.""" + return "" if _fenced_tool_call(m) is not None else m.group(0) + # Pattern 2: [TOOL_CALL] ... [/TOOL_CALL] blocks (some models use this format) # Matches: {tool => "shell", args => {--command "ls -la"}} etc. _TOOL_CALL_RE = re.compile( @@ -465,8 +508,10 @@ def parse_tool_blocks(text: str, skip_fenced: bool = False) -> List[ToolBlock]: # Pattern 1: fenced code blocks (skipped when `skip_fenced` — see docstring). if not skip_fenced: for m in _TOOL_BLOCK_RE.finditer(text): - tag = m.group(1).lower() - content = m.group(2).strip() + call = _fenced_tool_call(m) + if call is None: + continue + tag, content = call if not content: continue # If a code block's content is an XML call (some models wrap @@ -536,7 +581,7 @@ def strip_tool_blocks(text: str, skip_fenced: bool = False) -> str: # Normalize DSML first so its markup gets stripped by the # / removers below instead of leaking to the user. text = _normalize_dsml(text) - cleaned = text if skip_fenced else _TOOL_BLOCK_RE.sub('', text) + cleaned = text if skip_fenced else _TOOL_BLOCK_RE.sub(_strip_executed_fence, text) cleaned = _TOOL_CALL_RE.sub('', cleaned) cleaned = _XML_TOOL_CALL_RE.sub('', cleaned) cleaned = _TOOL_CODE_RE.sub('', cleaned) diff --git a/tests/test_fenced_inline_args.py b/tests/test_fenced_inline_args.py index 4604f9a0f..3f0ec7a5f 100644 --- a/tests/test_fenced_inline_args.py +++ b/tests/test_fenced_inline_args.py @@ -79,6 +79,45 @@ def test_inline_json_array_args_still_parse(): ] +def test_brace_metadata_on_bash_is_not_executable(): + # ```bash {title="setup"} is a Markdown fence attribute on a real + # language. Code tags (bash/python) never take same-line args — even a + # brace-shaped info string must stay display text. + blocks = parse_tool_blocks('```bash {title="setup"}\necho hi\n```') + assert blocks == [], blocks + + +def test_valid_json_metadata_on_python_is_not_executable(): + # Same rule when the attribute happens to BE valid JSON: the tag decides. + blocks = parse_tool_blocks('```python {"title": "example.py"}\nprint("hi")\n```') + assert blocks == [], blocks + + +def test_invalid_inline_json_on_email_tool_is_not_executable(): + # JSON-args tools only execute same-line content that parses as JSON — + # {title="x"} is metadata/garbage, not arguments. + blocks = parse_tool_blocks('```list_emails {title="x"}\n```') + assert blocks == [], blocks + + +def test_inline_json_continuing_on_next_lines_still_parses(): + # A JSON object opened on the tag line may close on a later line. + blocks = parse_tool_blocks('```list_emails {"folder": "INBOX",\n"max_results": 5}\n```') + assert [(b.tool_type, b.content) for b in blocks] == [ + ("list_emails", '{"folder": "INBOX",\n"max_results": 5}') + ] + + +def test_brace_metadata_fences_left_intact_in_display(): + # strip must mirror parse for every rejected fence shape. + for text in ( + 'Example:\n```bash {title="setup"}\necho hi\n```', + 'Example:\n```python {"title": "example.py"}\nprint("hi")\n```', + 'Example:\n```list_emails {title="x"}\n```', + ): + assert strip_tool_blocks(text) == text + + def test_inline_args_fence_is_stripped_from_display(): # strip must mirror parse: an executed inline-args fence must not leak # into the displayed text. diff --git a/tests/test_review_regressions.py b/tests/test_review_regressions.py index 35a4fdf2f..165e4dfd7 100644 --- a/tests/test_review_regressions.py +++ b/tests/test_review_regressions.py @@ -688,6 +688,65 @@ async def test_tool_policy_qualified_email_block_covers_bare_alias(monkeypatch): assert result["exit_code"] == 1 +@pytest.mark.asyncio +async def test_disable_tool_email_covers_full_builtin_set(monkeypatch): + """The friendly `disable_tool email` toggle must cover every built-in + email tool, in BOTH spellings — bare names (function-schema hiding, + bare-fence dispatch) and mcp__email__* (MCP schema hiding, runtime + qualified blocks). Hand-picking a subset left tools like delete_email + and download_attachment enabled (PR #3681 review follow-up).""" + # Import first so the module loads against the real core package; only + # the call-time SessionLocal import below sees the stub. + from src.tool_implementations import do_manage_settings + import src.settings as settings_mod + + db_mod = types.ModuleType("core.database") + + class _Db: + def close(self): + pass + + db_mod.SessionLocal = lambda: _Db() + monkeypatch.setitem(sys.modules, "core.database", db_mod) + + store = {} + + def fake_load_settings(): + return dict(store) + + def fake_save_settings(s): + store.clear() + store.update(s) + + monkeypatch.setattr(settings_mod, "load_settings", fake_load_settings) + monkeypatch.setattr(settings_mod, "save_settings", fake_save_settings) + + result = await do_manage_settings( + '{"action": "disable_tool", "tool": "email"}', owner="admin" + ) + + assert result["exit_code"] == 0 + disabled = set(store["disabled_tools"]) + # Spelled out (not imported from BUILTIN_EMAIL_TOOLS) so dropping a name + # from the constant fails here instead of silently shrinking the toggle. + bare_email_tools = ( + "list_email_accounts", "list_emails", "read_email", "search_emails", + "send_email", "reply_to_email", "draft_email", "draft_email_reply", + "ai_draft_email_reply", "archive_email", "delete_email", + "mark_email_read", "bulk_email", "download_attachment", + ) + for tool_name in bare_email_tools: + 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. + result = await do_manage_settings( + '{"action": "enable_tool", "tool": "email"}', owner="admin" + ) + assert result["exit_code"] == 0 + assert store["disabled_tools"] == [] + + def test_public_agent_policy_hides_sensitive_tools(monkeypatch): auth_mod = _install_core_auth_stub(monkeypatch) from src.tool_security import blocked_tools_for_owner