mirror of
https://github.com/pewdiepie-archdaemon/odysseus.git
synced 2026-07-14 12:48:03 +00:00
fix(security): reject brace-style fence metadata; cover the full email set in the friendly toggle
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user