mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
fix(channels): suppress mention resolution and escape untrusted fields
User- and model-authored text (task titles, approval headers, command previews, judge output, error fragments, notification bodies) reaches both channel integrations verbatim, and nothing at the channel boundary neutralised it. The Discord client now carries a client-level allowed-mentions-none default, which every message create inherits — plain sends, edits, and embeds — so broadcast and mention syntax in untrusted text cannot resolve, without mutating the text itself. The Slack adapter escapes each untrusted field into mrkdwn entities at its interpolation site — never the assembled message, so deliberately bot-authored markup like the session-opener mention survives. The policy-deny feedback returned to the server stays verbatim; only the rendered notice escapes. Storage and the shared formatter stay channel-neutral and verbatim: projection happens per audience at the render boundary.
This commit is contained in:
Patrick Buckley
@@ -9,11 +9,18 @@ from unittest.mock import AsyncMock, MagicMock, patch
|
||||
import pytest
|
||||
|
||||
# discord.utils.escape_markdown passes 'count' as positional to re.sub,
|
||||
# which is deprecated in Python 3.13+. This is a discord.py bug (fixed
|
||||
# in newer releases); suppress here to keep the test output clean.
|
||||
pytestmark = pytest.mark.filterwarnings(
|
||||
"ignore:.*'count' is passed as positional argument:DeprecationWarning"
|
||||
)
|
||||
# which is deprecated in Python 3.13+, and discord.Client's event
|
||||
# registration calls asyncio.iscoroutinefunction, deprecated in 3.14+.
|
||||
# Both are discord.py bugs (fixed in newer releases); suppress here to
|
||||
# keep the test output clean.
|
||||
pytestmark = [
|
||||
pytest.mark.filterwarnings(
|
||||
"ignore:.*'count' is passed as positional argument:DeprecationWarning"
|
||||
),
|
||||
pytest.mark.filterwarnings(
|
||||
"ignore:.*'asyncio.iscoroutinefunction' is deprecated:DeprecationWarning"
|
||||
),
|
||||
]
|
||||
|
||||
discord = pytest.importorskip("discord")
|
||||
|
||||
@@ -130,6 +137,66 @@ class TestDiscordConfig:
|
||||
assert cfg.auto_approve is True
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Allowed mentions
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def _make_real_bot():
|
||||
"""Construct a TurnstoneBot around a real ``commands.Bot`` (no gateway).
|
||||
|
||||
Storage is mocked and the module's httpx client is patched out; the
|
||||
underlying discord client object is real so tests can assert against
|
||||
the actual connection-state defaults every message create inherits.
|
||||
"""
|
||||
from turnstone.channels.discord.bot import TurnstoneBot
|
||||
from turnstone.channels.discord.config import DiscordConfig
|
||||
|
||||
with patch("turnstone.channels.discord.bot.httpx.AsyncClient", return_value=AsyncMock()):
|
||||
return TurnstoneBot(
|
||||
DiscordConfig(bot_token="token-test"), "http://localhost:8080", MagicMock()
|
||||
)
|
||||
|
||||
|
||||
class TestAllowedMentions:
|
||||
"""Approval headers and previews carry user/model text verbatim, so the
|
||||
client-level allowed_mentions default must suppress every mention
|
||||
resolution (@everyone/@here, users, roles) on the wire."""
|
||||
|
||||
def test_client_default_resolves_to_none(self):
|
||||
bot = _make_real_bot()
|
||||
|
||||
am = bot._bot.allowed_mentions
|
||||
assert am is not None
|
||||
# AllowedMentions has no value equality — compare the wire form.
|
||||
assert am.to_dict() == {"parse": []}
|
||||
|
||||
def test_message_create_inherits_client_default(self):
|
||||
"""A message create with no per-send override carries the default.
|
||||
|
||||
``abc.Messageable.send`` assembles its payload via
|
||||
``handle_message_parameters(previous_allowed_mentions=state.allowed_mentions)``;
|
||||
drive the same assembly with the client's state value and assert
|
||||
the wire payload pins ``allowed_mentions`` to parse-nothing while
|
||||
the hostile text itself stays verbatim.
|
||||
"""
|
||||
from discord.http import handle_message_parameters
|
||||
|
||||
bot = _make_real_bot()
|
||||
state_default = bot._bot._connection.allowed_mentions
|
||||
assert state_default is not None
|
||||
|
||||
hostile = "@everyone @here <@123456789012345678> <@&987654321098765432>"
|
||||
with handle_message_parameters(
|
||||
content=hostile,
|
||||
previous_allowed_mentions=state_default,
|
||||
) as params:
|
||||
payload = params.payload
|
||||
assert payload is not None
|
||||
assert payload["allowed_mentions"] == {"parse": []}
|
||||
assert payload["content"] == hostile
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# StreamingMessage
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@@ -320,6 +320,160 @@ class TestPreviewSanitization:
|
||||
assert out == "short and clean"
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# mrkdwn field escaping
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
class TestMrkdwnFieldEscaping:
|
||||
"""User/model-authored fields must reach the Slack wire entity-escaped
|
||||
while the bot's own mrkdwn framing stays live."""
|
||||
|
||||
def test_approval_card_escapes_hostile_fields(self) -> None:
|
||||
from turnstone.channels.slack.routes import SlackRoute
|
||||
from turnstone.sdk.events import ApproveRequestEvent
|
||||
|
||||
bot, _router, client = _make_bot()
|
||||
|
||||
event = ApproveRequestEvent(
|
||||
ws_id="ws-1",
|
||||
cycle_id="cyc-1",
|
||||
items=[
|
||||
{
|
||||
"call_id": "c-1",
|
||||
"func_name": "tasks",
|
||||
"approval_label": "@everyone <cmd>",
|
||||
"preview": "title=<!channel> ping <@U123> a & b <tag>",
|
||||
"needs_approval": True,
|
||||
}
|
||||
],
|
||||
)
|
||||
route = SlackRoute(channel="C01SAPU5414", user_id="U9", thread_ts="1.2")
|
||||
_run(bot._on_ws_event("ws-1", route, event)) # type: ignore[attr-defined]
|
||||
|
||||
client.chat_postMessage.assert_awaited_once()
|
||||
body = client.chat_postMessage.call_args[1]["blocks"][0]["text"]["text"]
|
||||
# Broadcast keywords, raw mention syntax, and entities arrive escaped.
|
||||
assert "<!channel>" in body
|
||||
assert "<@U123>" in body
|
||||
assert "a & b <tag>" in body
|
||||
assert "<cmd>" in body
|
||||
assert "<!channel>" not in body
|
||||
assert "<@U123>" not in body
|
||||
# A literal @everyone is inert outside angle brackets — kept as-is.
|
||||
assert "@everyone" in body
|
||||
# Bot-authored framing stays live mrkdwn.
|
||||
assert body.startswith("*Tool Approval Required*")
|
||||
assert "```" in body
|
||||
|
||||
def test_policy_deny_notice_escapes_names_but_feedback_stays_verbatim(self) -> None:
|
||||
from turnstone.channels._routing import PolicyVerdict
|
||||
from turnstone.channels.slack.routes import SlackRoute
|
||||
from turnstone.sdk.events import ApproveRequestEvent
|
||||
|
||||
bot, router, client = _make_bot()
|
||||
router.evaluate_tool_policies = AsyncMock(
|
||||
return_value=PolicyVerdict(kind="deny", denied_tools=["evil<!channel>tool"])
|
||||
)
|
||||
|
||||
event = ApproveRequestEvent(
|
||||
ws_id="ws-1",
|
||||
cycle_id="cyc-1",
|
||||
items=[{"call_id": "c-1", "func_name": "evil<!channel>tool", "needs_approval": True}],
|
||||
)
|
||||
route = SlackRoute(channel="C01SAPU5414", user_id="U9", thread_ts="1.2")
|
||||
_run(bot._on_ws_event("ws-1", route, event)) # type: ignore[attr-defined]
|
||||
|
||||
# Slack-rendered notice is escaped...
|
||||
text = client.chat_postMessage.call_args[1]["text"]
|
||||
assert "evil<!channel>tool" in text
|
||||
assert "<!channel>" not in text
|
||||
# ...but the feedback routed back to the server stays verbatim —
|
||||
# projection happens per audience at render, never upstream.
|
||||
router.send_approval.assert_awaited_once_with(
|
||||
"ws-1",
|
||||
"cyc-1",
|
||||
approved=False,
|
||||
feedback="Blocked by tool policy: evil<!channel>tool",
|
||||
)
|
||||
|
||||
def test_intent_verdict_escapes_judge_fields(self) -> None:
|
||||
from turnstone.channels.slack.bot import PendingApproval
|
||||
from turnstone.channels.slack.routes import SlackRoute
|
||||
from turnstone.sdk.events import IntentVerdictEvent
|
||||
|
||||
bot, _router, client = _make_bot()
|
||||
bot._pending_approval[("ws-1", "cyc-1")] = PendingApproval( # type: ignore[attr-defined]
|
||||
channel="C1",
|
||||
message_ts="999.000",
|
||||
owner_user_id="U9",
|
||||
cycle_id="cyc-1",
|
||||
call_ids=frozenset({"c-1"}),
|
||||
)
|
||||
|
||||
event = IntentVerdictEvent(
|
||||
ws_id="ws-1",
|
||||
call_id="c-1",
|
||||
func_name="bash <!everyone>",
|
||||
risk_level="high",
|
||||
confidence=0.9,
|
||||
intent_summary="pings <!channel> & <@U123>",
|
||||
)
|
||||
route = SlackRoute(channel="C1", user_id="U9", thread_ts="1.2")
|
||||
_run(bot._on_ws_event("ws-1", route, event)) # type: ignore[attr-defined]
|
||||
|
||||
client.chat_update.assert_awaited_once()
|
||||
blocks = client.chat_update.call_args[1]["blocks"]
|
||||
verdict_text = blocks[-1]["text"]["text"]
|
||||
assert "bash <!everyone>" in verdict_text
|
||||
assert "pings <!channel> & <@U123>" in verdict_text
|
||||
assert "<!channel>" not in verdict_text
|
||||
assert "<!everyone>" not in verdict_text
|
||||
# Bot framing survives around the escaped fields.
|
||||
assert verdict_text.startswith("*Judge Verdict: ")
|
||||
|
||||
def test_error_event_escapes_hostile_message(self) -> None:
|
||||
from turnstone.channels.slack.routes import SlackRoute
|
||||
from turnstone.sdk.events import ErrorEvent
|
||||
|
||||
bot, _router, client = _make_bot()
|
||||
|
||||
event = ErrorEvent(ws_id="ws-1", message="boom <!channel> & <@U123>")
|
||||
route = SlackRoute(channel="C1", user_id="U9", thread_ts="1.2")
|
||||
_run(bot._on_ws_event("ws-1", route, event)) # type: ignore[attr-defined]
|
||||
|
||||
text = client.chat_postMessage.call_args[1]["text"]
|
||||
assert text.startswith("*Error:* ")
|
||||
assert "boom <!channel> & <@U123>" in text
|
||||
assert "<!channel>" not in text
|
||||
|
||||
def test_send_escapes_notification_content(self) -> None:
|
||||
bot, _router, client = _make_bot()
|
||||
|
||||
_run(
|
||||
bot.send( # type: ignore[attr-defined]
|
||||
"C01SAPU5414:U12345",
|
||||
"Task '<!channel> deploy & retry <now>' is idle",
|
||||
)
|
||||
)
|
||||
|
||||
text = client.chat_postMessage.call_args[1]["text"]
|
||||
assert "<!channel> deploy & retry <now>" in text
|
||||
assert "<!channel>" not in text
|
||||
|
||||
def test_session_opener_mention_survives_unescaped(self) -> None:
|
||||
"""The opener's ``<@user>`` mention is bot-authored mrkdwn at a
|
||||
trusted callsite — per-field escaping must never reach it."""
|
||||
bot, router, client = _make_bot()
|
||||
router.get_or_create_workstream = AsyncMock(return_value=("ws-new", True))
|
||||
|
||||
body = {"channel_id": "C01SAPU5414", "user_id": "U777", "text": ""}
|
||||
_run(bot._on_slash_command(AsyncMock(), body)) # type: ignore[attr-defined]
|
||||
|
||||
texts = [c.kwargs.get("text", "") for c in client.chat_postMessage.call_args_list]
|
||||
assert any("<@U777> started a turnstone session." in t for t in texts)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# StreamingMessage
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@@ -281,6 +281,12 @@ class TurnstoneBot:
|
||||
command_prefix="!ts ",
|
||||
intents=intents,
|
||||
help_command=None,
|
||||
# Client-level default applied to every message create (plain
|
||||
# sends, edits, embeds): approval headers, previews, and
|
||||
# notification bodies carry user/model-authored text verbatim,
|
||||
# so mention resolution (@everyone/@here, users, roles) is
|
||||
# suppressed on the wire rather than by mutating the text.
|
||||
allowed_mentions=discord.AllowedMentions.none(),
|
||||
)
|
||||
|
||||
# Attach ourselves so cogs can access the TurnstoneBot instance.
|
||||
|
||||
@@ -91,6 +91,21 @@ _LINK_RATE_LIMIT: int = 5
|
||||
_LINK_RATE_CAP: int = 2048
|
||||
|
||||
|
||||
def _escape_mrkdwn(text: str) -> str:
|
||||
"""Escape ``&<>`` in user- or model-authored text bound for Slack mrkdwn.
|
||||
|
||||
Slack's contract for interpolated user-generated text: ``&``, ``<``
|
||||
and ``>`` become HTML entities. That one move neutralizes broadcast
|
||||
keywords (``<!channel>``, ``<!everyone>``), raw mention syntax
|
||||
(``<@U\u2026>``) and link markup (``<url|label>``) while a literal
|
||||
``@everyone`` \u2014 which Slack only resolves inside angle brackets \u2014
|
||||
stays plain text. Applied per field, never to an assembled message,
|
||||
so deliberately bot-authored markup (e.g. the session-opener
|
||||
``<@user>`` mention) is preserved.
|
||||
"""
|
||||
return text.replace("&", "&").replace("<", "<").replace(">", ">")
|
||||
|
||||
|
||||
def _sanitize_slack_preview(text: str, max_length: int = 1200) -> str:
|
||||
"""Escape Slack mrkdwn-sensitive content for safe fenced display.
|
||||
|
||||
@@ -102,7 +117,7 @@ def _sanitize_slack_preview(text: str, max_length: int = 1200) -> str:
|
||||
``&<>`` for good measure. Single backticks are kept intact so code
|
||||
snippets in plans / tool previews remain readable.
|
||||
"""
|
||||
text = text.replace("&", "&").replace("<", "<").replace(">", ">")
|
||||
text = _escape_mrkdwn(text)
|
||||
text = text.replace("```", "``\u200b`")
|
||||
if len(text) > max_length:
|
||||
return text[: max_length - 3] + "..."
|
||||
@@ -1103,7 +1118,9 @@ class TurnstoneSlackBot:
|
||||
await self._client.chat_postMessage(
|
||||
channel=slack_channel,
|
||||
thread_ts=thread_ts or None,
|
||||
text=f"_Tool blocked by admin policy: {denied}_",
|
||||
# The feedback sent to the server stays verbatim; only the
|
||||
# Slack-rendered notice escapes the tool names.
|
||||
text=f"_Tool blocked by admin policy: {_escape_mrkdwn(denied)}_",
|
||||
)
|
||||
policy_handled = True
|
||||
elif verdict.kind == "allow":
|
||||
@@ -1150,11 +1167,13 @@ class TurnstoneSlackBot:
|
||||
|
||||
pending_channel = entry.channel
|
||||
pending_ts = entry.message_ts
|
||||
risk = (event.risk_level or "medium").upper()
|
||||
# Judge output is model-authored — escape each field before it is
|
||||
# interpolated into the bot's mrkdwn framing.
|
||||
risk = _escape_mrkdwn((event.risk_level or "medium").upper())
|
||||
verdict_text = (
|
||||
f"*Judge Verdict: {event.func_name or 'tool'}*\n"
|
||||
f"*Judge Verdict: {_escape_mrkdwn(event.func_name or 'tool')}*\n"
|
||||
f"Risk: {risk} | Confidence: {event.confidence or 'N/A'}\n"
|
||||
f"_{event.intent_summary or ''}_"
|
||||
f"_{_escape_mrkdwn(event.intent_summary or '')}_"
|
||||
)
|
||||
# Append the verdict section in-place on the cached blocks so
|
||||
# repeat IntentVerdictEvents stack on the same approval message
|
||||
@@ -1218,7 +1237,9 @@ class TurnstoneSlackBot:
|
||||
self._pop_ws_approvals(ws_id)
|
||||
|
||||
async def _handle_error(self, route: SlackRoute, event: ErrorEvent) -> None:
|
||||
safe_msg = event.message[:500] if event.message else "An error occurred"
|
||||
# Error text can embed user/model-authored fragments (tool output,
|
||||
# provider messages) — escape the field before interpolation.
|
||||
safe_msg = _escape_mrkdwn(event.message[:500]) if event.message else "An error occurred"
|
||||
await self._client.chat_postMessage(
|
||||
channel=route.channel,
|
||||
thread_ts=route.thread_ts or None,
|
||||
@@ -1239,7 +1260,7 @@ class TurnstoneSlackBot:
|
||||
truncated_items = 0
|
||||
for item in items:
|
||||
raw_name = item.get("approval_label") or item.get("func_name") or "tool"
|
||||
name = raw_name.replace("&", "&").replace("<", "<").replace(">", ">")
|
||||
name = _escape_mrkdwn(raw_name)
|
||||
|
||||
raw_preview = item.get("preview", "")
|
||||
preview = (
|
||||
@@ -1348,6 +1369,12 @@ class TurnstoneSlackBot:
|
||||
async def send(self, channel_id: str, content: str) -> str:
|
||||
route = SlackRoute.parse(channel_id)
|
||||
|
||||
# Notification bodies are server/model-authored text (titles, task
|
||||
# names) with no bot-composed mrkdwn — the whole content is one
|
||||
# untrusted field. Escape before chunking so broadcast keywords
|
||||
# and raw mention syntax can't resolve on the wire.
|
||||
content = _escape_mrkdwn(content)
|
||||
|
||||
root_ts = route.thread_ts or ""
|
||||
first_post_ts = ""
|
||||
|
||||
|
||||
Reference in New Issue
Block a user