Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions slack-app-manifest.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@
- users:read.email
pkce_enabled: false
settings:
event_subscriptions:
bot_events:

Check warning on line 37 in slack-app-manifest.yaml

View check run for this annotation

@sentry/warden / warden: code-review

Bot-author guard bypassed on persistent `auth_test` failure, causing a reset loop

When `auth_test` fails, `_get_bot_user_id` returns `None` while `_bot_user_id` stays `None` (never cached), so every subsequent event retries `auth_test`. Any `channel_topic` event the bot itself emits via `set_channel_topic` will carry its actual user ID (e.g. `U0000`), which compares `!= None`, so the bot-author guard in `handle_channel_topic_change` fails open — triggering another reset, another Slack event, and so on indefinitely until rate-limited. The dedup cache won't help because each bot-generated topic change arrives with a fresh `event_ts`. Consider returning early (and skipping the reset entirely) when `_get_bot_user_id` returns `None`, or caching a sentinel value on failure to avoid re-invoking `auth_test` on every event.
- message.channels
- message.groups
interactivity:
is_enabled: true
org_deploy_enabled: false
Expand Down
49 changes: 49 additions & 0 deletions src/firetower/slack_app/bolt.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
handle_statuspage_submission,
)
from firetower.slack_app.handlers.subject import handle_subject_command
from firetower.slack_app.handlers.topic_guard import handle_channel_topic_change
from firetower.slack_app.handlers.update_incident import (
handle_update_command,
handle_update_incident_submission,
Expand Down Expand Up @@ -108,6 +109,7 @@
_bolt_app.command("/inc")(handle_command)
_bolt_app.command("/testinc")(handle_command)
_register_views(_bolt_app)
_register_event_handlers(_bolt_app)

Check warning on line 112 in src/firetower/slack_app/bolt.py

View check run for this annotation

@sentry/warden / warden: code-review

[LXY-VDE] Bot-author guard bypassed on persistent `auth_test` failure, causing a reset loop (additional location)

When `auth_test` fails, `_get_bot_user_id` returns `None` while `_bot_user_id` stays `None` (never cached), so every subsequent event retries `auth_test`. Any `channel_topic` event the bot itself emits via `set_channel_topic` will carry its actual user ID (e.g. `U0000`), which compares `!= None`, so the bot-author guard in `handle_channel_topic_change` fails open — triggering another reset, another Slack event, and so on indefinitely until rate-limited. The dedup cache won't help because each bot-generated topic change arrives with a fresh `event_ts`. Consider returning early (and skipping the reset entirely) when `_get_bot_user_id` returns `None`, or caching a sentinel value on failure to avoid re-invoking `auth_test` on every event.
return _bolt_app


Expand Down Expand Up @@ -213,6 +215,33 @@
return decorator


def _with_event_metrics(event_type: str) -> Callable[..., Callable[..., Any]]:
"""Wrap a Bolt event handler to emit submitted/completed/failed metrics.

Uses a ``slack_app.events`` namespace so event volume does not pollute the
``slack_app.views`` dashboards.
"""

def decorator(func: Callable[..., Any]) -> Callable[..., Any]:
@wraps(func)
def wrapper(*args: Any, **kwargs: Any) -> Any:
close_old_connections()
tags = [f"event_type:{event_type}"]
statsd.increment("slack_app.events.submitted", tags=tags)
try:
result = func(*args, **kwargs)
statsd.increment("slack_app.events.completed", tags=tags)
return result
except Exception:
logger.exception("Event handler failed: %s", event_type)
statsd.increment("slack_app.events.failed", tags=tags)
raise

return wrapper

return decorator


def _register_views(app: App) -> None:
"""Register view handlers (modals, etc.) on the Bolt app."""
app.view("backfill_incident_modal")(
Expand Down Expand Up @@ -256,3 +285,23 @@
"affected_region_tags",
):
app.options(action_id)(handle_tag_options)


def _ignore_message_event(ack: Any) -> None:
"""No-op catch-all for message events.

Subscribing to message.channels/message.groups opts the bot into the full
message firehose. Without a catch-all listener Bolt logs an "Unhandled
request" warning for every non-topic message. This listener is registered
after the channel_topic handler, so for topic changes the specific handler
runs first and returns before this one is reached.
"""
ack()


def _register_event_handlers(app: App) -> None:
"""Register Slack event subscriptions on the Bolt app."""
app.event({"type": "message", "subtype": "channel_topic"})(
_with_event_metrics("channel_topic")(handle_channel_topic_change)

Check failure on line 305 in src/firetower/slack_app/bolt.py

View check run for this annotation

@sentry/warden / warden: find-bugs

Bot-author guard fails when `auth_test` errors, enabling an infinite topic-reset loop

When `_get_bot_user_id` returns `None` (transient `auth_test` failure, e.g. before the first successful lookup is cached), the guard `event.get('user') == _get_bot_user_id(client)` evaluates to `'U_BOT' == None` → `False` for bot-authored events, so the guard is bypassed. Firetower's own `_slack_service.set_channel_topic` call then emits a new `channel_topic` event authored by the bot, which also bypasses the guard, and the loop repeats until `auth_test` recovers. A single manual topic edit during an auth outage can trigger a runaway reset/ephemeral loop and Slack rate-limiting.
)
app.event("message")(_ignore_message_event)
114 changes: 114 additions & 0 deletions src/firetower/slack_app/handlers/topic_guard.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
import logging
import threading
from collections import OrderedDict
from typing import Any

from firetower.incidents.hooks import build_channel_topic
from firetower.integrations.services.slack import SlackService, escape_slack_text
from firetower.slack_app.handlers.utils import get_incident_from_channel

logger = logging.getLogger(__name__)

_slack_service = SlackService()

_bot_user_id: str | None = None

# Bounded set of recently handled channel_topic event ids, used to drop Slack
# redeliveries. Socket Mode strips the X-Slack-Retry-Num header (it lives on the
# envelope, not the event payload Bolt forwards), so we dedup on the event's own
# id/timestamp instead. This in-memory cache is per-process, so it only dedups
# correctly while the firetower-slack-app Cloud Run service stays single-instance
# (max_instance_count = 1); scaling it out would split the cache across replicas.
_RECENT_EVENT_CACHE_SIZE = 256
_recent_event_ids: "OrderedDict[str, None]" = OrderedDict()
# Bolt dispatches event listeners on a thread pool, so guard the cache against
# concurrent check-then-set/evict races.
_recent_event_ids_lock = threading.Lock()


def _get_bot_user_id(client: Any) -> str | None:
"""Return the bot's own Slack user id, caching it after the first lookup."""
global _bot_user_id # noqa: PLW0603
if _bot_user_id is None:
try:
_bot_user_id = client.auth_test()["user_id"]
except Exception:
logger.exception("auth_test failed while resolving bot user id")
return None
Comment thread
spalmurray marked this conversation as resolved.
return _bot_user_id


def _event_id(event: dict) -> str | None:
return event.get("event_ts") or event.get("ts")


def _seen_recently(event_id: str) -> bool:
"""Return True if this event id was already handled, recording it otherwise."""
with _recent_event_ids_lock:
if event_id in _recent_event_ids:
return True
_recent_event_ids[event_id] = None
while len(_recent_event_ids) > _RECENT_EVENT_CACHE_SIZE:
_recent_event_ids.popitem(last=False)
return False


def _build_reset_message(attempted: str) -> str:
lines = [
"Channel topics here are managed by Firetower and reflect the incident's "
"current status, so I reset it.",
]
if attempted:
quoted = "\n".join(
f"> {escape_slack_text(line)}" for line in attempted.splitlines()
)
lines.append(f"You tried to set:\n{quoted}")
lines.append(
"To change incident details (which update the topic automatically), use "
"`/ft subject <title>`, `/ft severity <P0-P4>`, or `/ft captain`."
)
return "\n\n".join(lines)


def handle_channel_topic_change(event: dict, client: Any) -> None:
"""Reset manual incident-channel topic edits and nudge the editor.

Slack emits a ``channel_topic`` message event whenever a channel's topic

Check failure on line 76 in src/firetower/slack_app/handlers/topic_guard.py

View check run for this annotation

@sentry/warden / warden: find-bugs

[E7X-VMW] Bot-author guard fails when `auth_test` errors, enabling an infinite topic-reset loop (additional location)

When `_get_bot_user_id` returns `None` (transient `auth_test` failure, e.g. before the first successful lookup is cached), the guard `event.get('user') == _get_bot_user_id(client)` evaluates to `'U_BOT' == None` → `False` for bot-authored events, so the guard is bypassed. Firetower's own `_slack_service.set_channel_topic` call then emits a new `channel_topic` event authored by the bot, which also bypasses the guard, and the loop repeats until `auth_test` recovers. A single manual topic edit during an auth outage can trigger a runaway reset/ephemeral loop and Slack rate-limiting.
changes, including when Firetower sets it. The bot-author guard prevents an
infinite reset loop on our own edits, and the event-id dedup guard prevents a
second reset and ephemeral when Slack redelivers the event.
"""
if event.get("user") == _get_bot_user_id(client):
Comment thread
spalmurray marked this conversation as resolved.
return
Comment thread
spalmurray marked this conversation as resolved.

channel_id = event.get("channel")
if not channel_id:
return

event_id = _event_id(event)
if event_id is not None and _seen_recently(event_id):
logger.info("Skipping duplicate channel_topic event for channel %s", channel_id)
return

incident = get_incident_from_channel(channel_id)
if incident is None:
return

canonical = build_channel_topic(incident)
attempted = event.get("topic", "")

logger.info(
"Resetting manual topic change in incident channel %s (incident %s)",
channel_id,
incident.incident_number,
)
_slack_service.set_channel_topic(channel_id, canonical)

try:
client.chat_postEphemeral(
channel=channel_id,
user=event.get("user"),
text=_build_reset_message(attempted),
Comment thread
spalmurray marked this conversation as resolved.
)
except Exception:
logger.exception("Failed to post topic-reset ephemeral to %s", channel_id)
177 changes: 177 additions & 0 deletions src/firetower/slack_app/tests/test_topic_guard.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,177 @@
from unittest.mock import MagicMock, patch

import pytest

from firetower.slack_app.handlers import topic_guard
from firetower.slack_app.handlers.topic_guard import handle_channel_topic_change

CHANNEL_ID = "C_TEST_CHANNEL"
BOT_USER_ID = "U0000"
CANONICAL = "[P2] <https://ft/INC-1|INC-1 Test>"


@pytest.fixture(autouse=True)
def reset_module_state():
topic_guard._bot_user_id = None
topic_guard._recent_event_ids.clear()
yield
topic_guard._bot_user_id = None
topic_guard._recent_event_ids.clear()


@pytest.fixture
def client():
c = MagicMock()
c.auth_test.return_value = {"user_id": BOT_USER_ID}
return c


_TS_COUNTER = [0]


def _event(user="U_OTHER", topic="Something custom", channel=CHANNEL_ID, ts=None):
if ts is None:
_TS_COUNTER[0] += 1
ts = f"1700000000.{_TS_COUNTER[0]:06d}"
return {
"type": "message",
"subtype": "channel_topic",
"channel": channel,
"user": user,
"topic": topic,
"event_ts": ts,
}


@patch("firetower.slack_app.handlers.topic_guard._slack_service")
@patch("firetower.slack_app.handlers.topic_guard.build_channel_topic")
@patch("firetower.slack_app.handlers.topic_guard.get_incident_from_channel")
def test_manual_change_resets_and_notifies(
mock_get_incident, mock_build_topic, mock_slack_service, client
):
mock_get_incident.return_value = MagicMock()
mock_build_topic.return_value = CANONICAL

handle_channel_topic_change(_event(topic="My custom topic"), client)

mock_slack_service.set_channel_topic.assert_called_once_with(CHANNEL_ID, CANONICAL)
client.chat_postEphemeral.assert_called_once()
kwargs = client.chat_postEphemeral.call_args[1]
assert kwargs["channel"] == CHANNEL_ID
assert kwargs["user"] == "U_OTHER"
assert "My custom topic" in kwargs["text"]


@patch("firetower.slack_app.handlers.topic_guard._slack_service")
@patch("firetower.slack_app.handlers.topic_guard.get_incident_from_channel")
def test_bot_authored_event_is_ignored(mock_get_incident, mock_slack_service, client):
handle_channel_topic_change(_event(user=BOT_USER_ID), client)

mock_get_incident.assert_not_called()
mock_slack_service.set_channel_topic.assert_not_called()
client.chat_postEphemeral.assert_not_called()


@patch("firetower.slack_app.handlers.topic_guard._slack_service")
@patch("firetower.slack_app.handlers.topic_guard.get_incident_from_channel")
def test_non_incident_channel_is_noop(mock_get_incident, mock_slack_service, client):
mock_get_incident.return_value = None

handle_channel_topic_change(_event(), client)

Check failure on line 80 in src/firetower/slack_app/tests/test_topic_guard.py

View check run for this annotation

@sentry/warden / warden: find-bugs

[E7X-VMW] Bot-author guard fails when `auth_test` errors, enabling an infinite topic-reset loop (additional location)

When `_get_bot_user_id` returns `None` (transient `auth_test` failure, e.g. before the first successful lookup is cached), the guard `event.get('user') == _get_bot_user_id(client)` evaluates to `'U_BOT' == None` → `False` for bot-authored events, so the guard is bypassed. Firetower's own `_slack_service.set_channel_topic` call then emits a new `channel_topic` event authored by the bot, which also bypasses the guard, and the loop repeats until `auth_test` recovers. A single manual topic edit during an auth outage can trigger a runaway reset/ephemeral loop and Slack rate-limiting.

mock_slack_service.set_channel_topic.assert_not_called()
client.chat_postEphemeral.assert_not_called()


@patch("firetower.slack_app.handlers.topic_guard._slack_service")
@patch("firetower.slack_app.handlers.topic_guard.get_incident_from_channel")
def test_missing_channel_is_noop(mock_get_incident, mock_slack_service, client):
event = _event()
del event["channel"]

handle_channel_topic_change(event, client)

mock_get_incident.assert_not_called()
mock_slack_service.set_channel_topic.assert_not_called()
client.chat_postEphemeral.assert_not_called()


@patch("firetower.slack_app.handlers.topic_guard._slack_service")
@patch("firetower.slack_app.handlers.topic_guard.build_channel_topic")
@patch("firetower.slack_app.handlers.topic_guard.get_incident_from_channel")
def test_attempted_topic_is_escaped(
mock_get_incident, mock_build_topic, mock_slack_service, client
):
mock_get_incident.return_value = MagicMock()
mock_build_topic.return_value = CANONICAL

handle_channel_topic_change(
_event(topic="<!channel> see <https://evil|here> & <@U999>"), client
)

text = client.chat_postEphemeral.call_args[1]["text"]
assert "<!channel>" not in text
assert "<@U999>" not in text
assert "<https://evil|here>" not in text
assert "&lt;!channel&gt;" in text
assert "&amp;" in text


@patch("firetower.slack_app.handlers.topic_guard._slack_service")
@patch("firetower.slack_app.handlers.topic_guard.build_channel_topic")
@patch("firetower.slack_app.handlers.topic_guard.get_incident_from_channel")
def test_ephemeral_failure_still_resets(
mock_get_incident, mock_build_topic, mock_slack_service, client
):
mock_get_incident.return_value = MagicMock()
mock_build_topic.return_value = CANONICAL
client.chat_postEphemeral.side_effect = Exception("boom")

handle_channel_topic_change(_event(), client)

mock_slack_service.set_channel_topic.assert_called_once_with(CHANNEL_ID, CANONICAL)
client.chat_postEphemeral.assert_called_once()


@patch("firetower.slack_app.handlers.topic_guard._slack_service")
@patch("firetower.slack_app.handlers.topic_guard.build_channel_topic")
@patch("firetower.slack_app.handlers.topic_guard.get_incident_from_channel")
def test_bot_id_cache_is_reused(
mock_get_incident, mock_build_topic, mock_slack_service, client
):
mock_get_incident.return_value = MagicMock()
mock_build_topic.return_value = CANONICAL

handle_channel_topic_change(_event(), client)
handle_channel_topic_change(_event(), client)

assert client.auth_test.call_count == 1


@patch("firetower.slack_app.handlers.topic_guard._slack_service")
@patch("firetower.slack_app.handlers.topic_guard.get_incident_from_channel")
def test_auth_test_failure_does_not_crash(
mock_get_incident, mock_slack_service, client
):
client.auth_test.side_effect = Exception("transient")

handle_channel_topic_change(_event(), client)

mock_get_incident.assert_called_once()


@patch("firetower.slack_app.handlers.topic_guard._slack_service")
@patch("firetower.slack_app.handlers.topic_guard.build_channel_topic")
@patch("firetower.slack_app.handlers.topic_guard.get_incident_from_channel")
def test_duplicate_event_id_is_skipped(
mock_get_incident, mock_build_topic, mock_slack_service, client
):
mock_get_incident.return_value = MagicMock()
mock_build_topic.return_value = CANONICAL
event = _event(ts="1700000000.999999")

handle_channel_topic_change(event, client)
handle_channel_topic_change(event, client)

mock_slack_service.set_channel_topic.assert_called_once()
client.chat_postEphemeral.assert_called_once()
Loading