diff --git a/plugins/platforms/slack/adapter.py b/plugins/platforms/slack/adapter.py index 7b81f30cae27..24cc0d8cb11f 100644 --- a/plugins/platforms/slack/adapter.py +++ b/plugins/platforms/slack/adapter.py @@ -1241,6 +1241,56 @@ class SlackAdapter(BasePlatformAdapter): ) return SendResult(success=False, error=str(e)) + async def _post_ephemeral_fallback( + self, + chat_id: str, + ctx: Dict[str, Any], + content: str, + ) -> "SendResult": + """Deliver a slash reply via ``chat.postEphemeral``. + + Fallback for when the ``response_url`` POST fails (#19688). + ``chat.postEphemeral`` is an independent Web API path that keeps the + reply private to the invoking user — unlike a public channel post, + which must never happen for a reply the user expects to be ephemeral. + + Unlike response_url, this cannot ``replace_original``, so the + "Running /cmd…" ack stays; the reply arrives as new ephemeral + message(s) below it. Chunked like normal sends; no 5-POST cap + applies to postEphemeral. + """ + user_id = ctx.get("user_id", "") + if not user_id: + return SendResult( + success=False, + error="no user_id in slash context for postEphemeral", + ) + formatted = self.format_message(content) + chunks = self.truncate_message(formatted, self.MAX_MESSAGE_LENGTH) + if not chunks: + chunks = [formatted] + try: + client = self._get_client(chat_id) + for chunk in chunks: + result = await client.chat_postEphemeral( + channel=chat_id, + user=user_id, + text=chunk, + ) + if not (isinstance(result, dict) and result.get("ok")): + err = ( + result.get("error", "unknown_error") + if isinstance(result, dict) + else "unexpected_response" + ) + return SendResult( + success=False, + error=f"chat.postEphemeral failed: {err}", + ) + return SendResult(success=True, message_id=None) + except Exception as e: + return SendResult(success=False, error=str(e)) + def _warn_if_missing_group_dm_scopes(self, auth_response, team_name: str) -> None: """Nudge existing installs to reinstall when group-DM scopes are absent. @@ -1853,15 +1903,35 @@ class SlackAdapter(BasePlatformAdapter): ) if ephemeral_result.success: return ephemeral_result - # Ephemeral delivery failed (#19688): fall through to normal - # channel delivery so the command reply is never silently - # dropped. The stale "Running /cmd…" ack remains, but the - # user still gets the actual answer. + # response_url delivery failed (#19688): fall back to + # chat.postEphemeral — an independent API path that keeps + # the reply private ("Only visible to you"). We do NOT fall + # back to a public channel post: a slash reply the user + # expects to be ephemeral must never surface to the whole + # channel just because a delivery path failed. logger.warning( - "[Slack] Ephemeral slash reply failed (%s); falling back " - "to channel delivery", + "[Slack] response_url slash reply failed (%s); retrying " + "via chat.postEphemeral", ephemeral_result.error, ) + fallback_result = await self._post_ephemeral_fallback( + chat_id, + slash_ctx, + content, + ) + if fallback_result.success: + return fallback_result + # Both ephemeral paths failed — surface the failure instead + # of leaking the reply publicly. The user still has the + # "Running /cmd…" ack; the error is logged and returned so + # the gateway can react (retry surfacing happens upstream). + logger.error( + "[Slack] Ephemeral slash reply failed on both " + "response_url and chat.postEphemeral (%s); dropping " + "rather than posting publicly", + fallback_result.error, + ) + return fallback_result # Convert standard markdown → Slack mrkdwn formatted = self.format_message(content) @@ -5913,6 +5983,9 @@ class SlackAdapter(BasePlatformAdapter): if response_url and user_id and channel_id and text.startswith("/"): self._slash_command_contexts[(channel_id, user_id)] = { "response_url": response_url, + # Kept for the chat.postEphemeral fallback when response_url + # delivery fails — postEphemeral needs an explicit user. + "user_id": user_id, "ts": time.monotonic(), } diff --git a/tests/gateway/test_slack.py b/tests/gateway/test_slack.py index b5da3e8bfc50..f5548b41c93b 100644 --- a/tests/gateway/test_slack.py +++ b/tests/gateway/test_slack.py @@ -5520,12 +5520,14 @@ class TestSlashEphemeralAck: @pytest.mark.asyncio async def test_send_slash_ephemeral_fallback_on_post_failure(self, adapter): - """Failed response_url POST falls back to normal channel delivery (#19688).""" + """Failed response_url POST falls back to chat.postEphemeral — never + a public channel post (#19688).""" import time from plugins.platforms.slack.adapter import _slash_user_id adapter._slash_command_contexts[("C1", "U1")] = { "response_url": "https://hooks.slack.com/commands/bad", + "user_id": "U1", "ts": time.monotonic(), } @@ -5544,6 +5546,9 @@ class TestSlashEphemeralAck: adapter._app.client.chat_postMessage = AsyncMock( return_value={"ts": "1234.5678", "ok": True} ) + adapter._app.client.chat_postEphemeral = AsyncMock( + return_value={"ok": True} + ) token = _slash_user_id.set("U1") try: @@ -5554,18 +5559,69 @@ class TestSlashEphemeralAck: finally: _slash_user_id.reset(token) - # Reply must not be silently dropped: channel fallback delivered it. + # Reply delivered ephemerally via postEphemeral; the public + # chat.postMessage path must NOT be used for a slash reply. assert result.success is True - adapter._app.client.chat_postMessage.assert_awaited_once() + adapter._app.client.chat_postEphemeral.assert_awaited_once() + adapter._app.client.chat_postMessage.assert_not_awaited() + + @pytest.mark.asyncio + async def test_send_slash_ephemeral_both_paths_fail_never_posts_publicly( + self, adapter + ): + """When response_url AND chat.postEphemeral both fail, the reply is + dropped with an error — never leaked to the public channel (#19688).""" + import time + from plugins.platforms.slack.adapter import _slash_user_id + + adapter._slash_command_contexts[("C1", "U1")] = { + "response_url": "https://hooks.slack.com/commands/bad", + "user_id": "U1", + "ts": time.monotonic(), + } + + mock_resp = AsyncMock() + mock_resp.status = 500 + mock_resp.content = None + mock_resp.text = AsyncMock(return_value="Internal Server Error") + mock_resp.__aenter__ = AsyncMock(return_value=mock_resp) + mock_resp.__aexit__ = AsyncMock(return_value=False) + + mock_session = AsyncMock() + mock_session.post = MagicMock(return_value=mock_resp) + mock_session.__aenter__ = AsyncMock(return_value=mock_session) + mock_session.__aexit__ = AsyncMock(return_value=False) + + adapter._app.client.chat_postMessage = AsyncMock( + return_value={"ts": "1234.5678", "ok": True} + ) + adapter._app.client.chat_postEphemeral = AsyncMock( + return_value={"ok": False, "error": "channel_not_found"} + ) + + token = _slash_user_id.set("U1") + try: + with patch( + "plugins.platforms.slack.adapter.aiohttp.ClientSession", return_value=mock_session + ): + result = await adapter.send("C1", "Some response") + finally: + _slash_user_id.reset(token) + + assert result.success is False + assert "postEphemeral" in (result.error or "") + adapter._app.client.chat_postMessage.assert_not_awaited() @pytest.mark.asyncio async def test_send_slash_ephemeral_fallback_on_exception(self, adapter): - """aiohttp exception on response_url falls back to channel delivery (#19688).""" + """aiohttp exception on response_url falls back to chat.postEphemeral, + not to public channel delivery (#19688).""" import time from plugins.platforms.slack.adapter import _slash_user_id adapter._slash_command_contexts[("C1", "U1")] = { "response_url": "https://hooks.slack.com/commands/timeout", + "user_id": "U1", "ts": time.monotonic(), } @@ -5577,6 +5633,9 @@ class TestSlashEphemeralAck: adapter._app.client.chat_postMessage = AsyncMock( return_value={"ts": "1234.5678", "ok": True} ) + adapter._app.client.chat_postEphemeral = AsyncMock( + return_value={"ok": True} + ) token = _slash_user_id.set("U1") try: @@ -5588,7 +5647,8 @@ class TestSlashEphemeralAck: _slash_user_id.reset(token) assert result.success is True - adapter._app.client.chat_postMessage.assert_awaited_once() + adapter._app.client.chat_postEphemeral.assert_awaited_once() + adapter._app.client.chat_postMessage.assert_not_awaited() @pytest.mark.asyncio async def test_send_slash_ephemeral_multichunk_delivers_all_parts(self, adapter): @@ -5703,6 +5763,7 @@ class TestSlashEphemeralAck: adapter._slash_command_contexts[("C1", "U1")] = { "response_url": "https://hooks.slack.com/commands/oversized", + "user_id": "U1", "ts": time.monotonic(), } response = _FakeResponse( @@ -5717,6 +5778,9 @@ class TestSlashEphemeralAck: adapter._app.client.chat_postMessage = AsyncMock( return_value={"ts": "1234.5678", "ok": True} ) + adapter._app.client.chat_postEphemeral = AsyncMock( + return_value={"ok": True} + ) from plugins.platforms.slack.adapter import _slash_user_id