mirror of
https://github.com/pewdiepie-archdaemon/odysseus.git
synced 2026-07-15 12:58:04 +00:00
fix(security): gate bare and mcp-qualified email names together; stop executing Markdown info strings
Review follow-up on #3681 (thanks @RaresKeY): 1. P1: execute_tool_block() checked disabled_tools / the turn ToolPolicy only against the incoming block name, then the bare-email branch qualified it to mcp__email__<name> and called the MCP manager. Plan mode and the MCP settings toggle write the QUALIFIED name into the denylist, so a bare fence like ```list_emails``` sailed past a mcp__email__list_emails entry. Both gates now match on both spellings (bare <-> mcp__email__-qualified), in either direction. 2. P2: the relaxed fence regex accepted arbitrary same-line text after a recognized tag, which made ordinary Markdown info strings executable: ```python title="example.py" ran as a python tool call. Same-line content now only counts as tool input when it starts with { or [ (JSON args); anything else leaves the fence as display text, and strip_tool_blocks mirrors that (the fence stays visible). Tests: disabled-tools alias regression (qualified entry blocks bare name and vice versa, never reaching the MCP manager), ToolPolicy alias regression, python/bash title="..." non-execution + display retention, and inline JSON-array args still parsing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
+15
-2
@@ -559,6 +559,19 @@ async def _execute_tool_block_impl(
|
|||||||
tool = block.tool_type
|
tool = block.tool_type
|
||||||
content = block.content
|
content = block.content
|
||||||
|
|
||||||
|
# A bare email tool name is an alias for its MCP-qualified form (the
|
||||||
|
# dispatch below routes it to mcp__email__<name>). Policy sources spell
|
||||||
|
# email tools either way — plan mode and the MCP settings toggle write the
|
||||||
|
# qualified name into the denylist, chat-level toggles the bare one — so
|
||||||
|
# the block/disable gates below must match on BOTH spellings. Gating only
|
||||||
|
# the name the model happened to emit lets a bare fence slip past a
|
||||||
|
# qualified denylist entry (and vice versa).
|
||||||
|
policy_names = {tool}
|
||||||
|
if tool in BUILTIN_EMAIL_TOOLS:
|
||||||
|
policy_names.add(f"mcp__email__{tool}")
|
||||||
|
elif tool.startswith("mcp__email__") and tool[len("mcp__email__"):] in BUILTIN_EMAIL_TOOLS:
|
||||||
|
policy_names.add(tool[len("mcp__email__"):])
|
||||||
|
|
||||||
# Misformatted tool call detection: model put JSON inside ```python``` (or
|
# Misformatted tool call detection: model put JSON inside ```python``` (or
|
||||||
# similar) without naming the tool. Common with MiniMax-style outputs.
|
# similar) without naming the tool. Common with MiniMax-style outputs.
|
||||||
# Return a helpful error so the model retries with the correct format.
|
# Return a helpful error so the model retries with the correct format.
|
||||||
@@ -586,13 +599,13 @@ async def _execute_tool_block_impl(
|
|||||||
pass
|
pass
|
||||||
|
|
||||||
# Reject tools that the user has disabled for this request
|
# Reject tools that the user has disabled for this request
|
||||||
if disabled_tools and tool in disabled_tools:
|
if disabled_tools and not policy_names.isdisjoint(disabled_tools):
|
||||||
desc = f"{tool}: BLOCKED"
|
desc = f"{tool}: BLOCKED"
|
||||||
result = {"error": f"Tool '{tool}' is disabled by user.", "exit_code": 1}
|
result = {"error": f"Tool '{tool}' is disabled by user.", "exit_code": 1}
|
||||||
logger.info(f"Tool blocked by user: {tool}")
|
logger.info(f"Tool blocked by user: {tool}")
|
||||||
return desc, result
|
return desc, result
|
||||||
|
|
||||||
if tool_policy and tool_policy.blocks(tool):
|
if tool_policy and any(tool_policy.blocks(name) for name in policy_names):
|
||||||
desc = f"{tool}: BLOCKED"
|
desc = f"{tool}: BLOCKED"
|
||||||
result = {
|
result = {
|
||||||
"error": f"Execution of tool '{tool}' is forbade by the active guide-only policy.",
|
"error": f"Execution of tool '{tool}' is forbade by the active guide-only policy.",
|
||||||
|
|||||||
+7
-3
@@ -19,13 +19,17 @@ logger = logging.getLogger(__name__)
|
|||||||
# Regex patterns
|
# Regex patterns
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
# Pattern 1: ```bash ... ``` fenced code blocks. The tag may be followed by
|
# Pattern 1: ```bash ... ``` fenced code blocks. The tag may be followed by a
|
||||||
# inline args on the same line (```list_email_accounts {}) or a newline.
|
# 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.
|
||||||
# (?![\w-]) keeps the alternation from prefix-matching longer fence tags:
|
# (?![\w-]) keeps the alternation from prefix-matching longer fence tags:
|
||||||
# without it, ```python3 would match as tool "python" with content "3\n..."
|
# without it, ```python3 would match as tool "python" with content "3\n..."
|
||||||
# and execute as code.
|
# and execute as code.
|
||||||
_TOOL_BLOCK_RE = re.compile(
|
_TOOL_BLOCK_RE = re.compile(
|
||||||
r"```(" + "|".join(TOOL_TAGS) + r")(?![\w-])[ \t]*\n?([\s\S]*?)```",
|
r"```(" + "|".join(TOOL_TAGS) + r")(?![\w-])[ \t]*(?=\r?\n|[{\[])\r?\n?([\s\S]*?)```",
|
||||||
re.IGNORECASE,
|
re.IGNORECASE,
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|||||||
@@ -58,6 +58,27 @@ def test_hyphenated_tag_is_not_a_tool_call():
|
|||||||
assert blocks == [], blocks
|
assert blocks == [], blocks
|
||||||
|
|
||||||
|
|
||||||
|
def test_markdown_info_string_is_not_executable_python():
|
||||||
|
# ```python title="example.py" is Markdown fence metadata, not tool args.
|
||||||
|
# Same-line content other than JSON args ({...}/[...]) must not execute —
|
||||||
|
# otherwise a fence the model meant to display runs as code.
|
||||||
|
blocks = parse_tool_blocks('```python title="example.py"\nprint("hi")\n```')
|
||||||
|
assert blocks == [], blocks
|
||||||
|
|
||||||
|
|
||||||
|
def test_markdown_info_string_is_not_executable_bash():
|
||||||
|
blocks = parse_tool_blocks('```bash title="setup"\necho hi\n```')
|
||||||
|
assert blocks == [], blocks
|
||||||
|
|
||||||
|
|
||||||
|
def test_inline_json_array_args_still_parse():
|
||||||
|
# The narrowed same-line rule must keep accepting JSON args: { or [.
|
||||||
|
blocks = parse_tool_blocks('```bulk_email {"action": "archive", "uids": [1, 2]}\n```')
|
||||||
|
assert [(b.tool_type, b.content) for b in blocks] == [
|
||||||
|
("bulk_email", '{"action": "archive", "uids": [1, 2]}')
|
||||||
|
]
|
||||||
|
|
||||||
|
|
||||||
def test_inline_args_fence_is_stripped_from_display():
|
def test_inline_args_fence_is_stripped_from_display():
|
||||||
# strip must mirror parse: an executed inline-args fence must not leak
|
# strip must mirror parse: an executed inline-args fence must not leak
|
||||||
# into the displayed text.
|
# into the displayed text.
|
||||||
@@ -69,3 +90,10 @@ def test_python3_fence_is_left_intact_in_display():
|
|||||||
# ...and a fence that did NOT parse as a tool call must stay visible.
|
# ...and a fence that did NOT parse as a tool call must stay visible.
|
||||||
text = 'Example:\n```python3\nprint("hi")\n```'
|
text = 'Example:\n```python3\nprint("hi")\n```'
|
||||||
assert strip_tool_blocks(text) == text
|
assert strip_tool_blocks(text) == text
|
||||||
|
|
||||||
|
|
||||||
|
def test_markdown_info_string_fence_is_left_intact_in_display():
|
||||||
|
# strip must mirror parse for info-string fences too: not executed,
|
||||||
|
# so not stripped from the displayed text.
|
||||||
|
text = 'Example:\n```python title="example.py"\nprint("hi")\n```'
|
||||||
|
assert strip_tool_blocks(text) == text
|
||||||
|
|||||||
@@ -635,6 +635,59 @@ async def test_public_agent_policy_blocks_sensitive_tools(monkeypatch):
|
|||||||
assert "restricted to admin users" in result["error"]
|
assert "restricted to admin users" in result["error"]
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_disabled_qualified_email_tool_blocks_bare_alias(monkeypatch):
|
||||||
|
"""A bare email fence is an alias for its mcp__email__ form. Plan mode and
|
||||||
|
the MCP settings toggle write the QUALIFIED name into disabled_tools, so
|
||||||
|
the gate must block the bare spelling too — and never reach the MCP
|
||||||
|
manager (PR #3681 review follow-up)."""
|
||||||
|
import src.tool_execution as tool_execution
|
||||||
|
from src.tool_execution import execute_tool_block
|
||||||
|
|
||||||
|
def fail_get_mcp_manager():
|
||||||
|
raise AssertionError("blocked email tool must not reach the MCP manager")
|
||||||
|
|
||||||
|
monkeypatch.setattr(tool_execution, "get_mcp_manager", fail_get_mcp_manager)
|
||||||
|
|
||||||
|
for bare, disabled in (
|
||||||
|
# qualified denylist entry blocks the bare alias…
|
||||||
|
("list_emails", {"mcp__email__list_emails"}),
|
||||||
|
("download_attachment", {"mcp__email__download_attachment"}),
|
||||||
|
# …and a bare denylist entry blocks the qualified spelling.
|
||||||
|
("mcp__email__delete_email", {"delete_email"}),
|
||||||
|
):
|
||||||
|
desc, result = await execute_tool_block(
|
||||||
|
SimpleNamespace(tool_type=bare, content="{}"),
|
||||||
|
owner="admin-user",
|
||||||
|
disabled_tools=disabled,
|
||||||
|
)
|
||||||
|
assert desc == f"{bare}: BLOCKED"
|
||||||
|
assert result["exit_code"] == 1
|
||||||
|
assert "disabled by user" in result["error"]
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_tool_policy_qualified_email_block_covers_bare_alias(monkeypatch):
|
||||||
|
"""Same aliasing rule for the turn ToolPolicy denylist."""
|
||||||
|
import src.tool_execution as tool_execution
|
||||||
|
from src.tool_execution import execute_tool_block
|
||||||
|
from src.tool_policy import ToolPolicy
|
||||||
|
|
||||||
|
def fail_get_mcp_manager():
|
||||||
|
raise AssertionError("blocked email tool must not reach the MCP manager")
|
||||||
|
|
||||||
|
monkeypatch.setattr(tool_execution, "get_mcp_manager", fail_get_mcp_manager)
|
||||||
|
|
||||||
|
policy = ToolPolicy(disabled_tools=frozenset({"mcp__email__send_email"}))
|
||||||
|
desc, result = await execute_tool_block(
|
||||||
|
SimpleNamespace(tool_type="send_email", content="{}"),
|
||||||
|
owner="admin-user",
|
||||||
|
tool_policy=policy,
|
||||||
|
)
|
||||||
|
assert desc == "send_email: BLOCKED"
|
||||||
|
assert result["exit_code"] == 1
|
||||||
|
|
||||||
|
|
||||||
def test_public_agent_policy_hides_sensitive_tools(monkeypatch):
|
def test_public_agent_policy_hides_sensitive_tools(monkeypatch):
|
||||||
auth_mod = _install_core_auth_stub(monkeypatch)
|
auth_mod = _install_core_auth_stub(monkeypatch)
|
||||||
from src.tool_security import blocked_tools_for_owner
|
from src.tool_security import blocked_tools_for_owner
|
||||||
|
|||||||
Reference in New Issue
Block a user