Browse Source

fix(slack): only escape non-unfurls a single time (#49716)

This fixes a bug where issue alert messages are escaped in the message:

<img width="829" alt="Screen Shot 2023-05-24 at 12 42 19 PM"
src="https://github.com/getsentry/sentry/assets/8533851/319eb533-48a2-4b71-9f05-135739994032">

This is because we double escape it. It turns out, we only need to
doubly escape for unfurled messages. Not exactly sure why that is, but
it likely has to do with JSON parsing and the fact we use the `unfurls`
field instead of `attachments`.
Stephen Cefali 1 year ago
parent
commit
dbc6be2df1

+ 5 - 3
src/sentry/integrations/slack/message_builder/base/base.py

@@ -63,6 +63,7 @@ class SlackMessageBuilder(AbstractMessageBuilder, ABC):
         footer: str | None = None,
         color: str | None = None,
         actions: Sequence[MessageAction] | None = None,
+        is_unfurl: bool = False,
         **kwargs: Any,
     ) -> SlackBody:
         """
@@ -93,9 +94,10 @@ class SlackMessageBuilder(AbstractMessageBuilder, ABC):
 
         markdown_in = ["text"]
         if self.escape_text:
-            text = escape_slack_text(
-                escape_slack_text(text)
-            )  # Slack will un-escape so we have to double escape
+            text = escape_slack_text(text)
+            # for unfurling messages we need to double escape the text
+            if is_unfurl:
+                text = escape_slack_text(text)
             markdown_in = []
 
         return {

+ 13 - 1
src/sentry/integrations/slack/message_builder/issues.py

@@ -248,6 +248,7 @@ class SlackIssuesMessageBuilder(SlackMessageBuilder):
         issue_details: bool = False,
         notification: ProjectNotification | None = None,
         recipient: RpcActor | None = None,
+        is_unfurl: bool = False,
     ) -> None:
         super().__init__()
         self.group = group
@@ -260,6 +261,7 @@ class SlackIssuesMessageBuilder(SlackMessageBuilder):
         self.issue_details = issue_details
         self.notification = notification
         self.recipient = recipient
+        self.is_unfurl = is_unfurl
 
     @property
     def escape_text(self) -> bool:
@@ -309,6 +311,7 @@ class SlackIssuesMessageBuilder(SlackMessageBuilder):
                 ExternalProviders.SLACK,
             ),
             ts=get_timestamp(self.group, self.event) if not self.issue_details else None,
+            is_unfurl=self.is_unfurl,
         )
 
 
@@ -321,8 +324,17 @@ def build_group_attachment(
     rules: list[Rule] | None = None,
     link_to_event: bool = False,
     issue_details: bool = False,
+    is_unfurl: bool = False,
 ) -> SlackBody:
     """@deprecated"""
     return SlackIssuesMessageBuilder(
-        group, event, tags, identity, actions, rules, link_to_event, issue_details
+        group,
+        event,
+        tags,
+        identity,
+        actions,
+        rules,
+        link_to_event,
+        issue_details,
+        is_unfurl=is_unfurl,
     ).build()

+ 1 - 1
src/sentry/integrations/slack/unfurl/issues.py

@@ -60,7 +60,7 @@ def unfurl_issues(
                 else None
             )
             out[link.url] = build_group_attachment(
-                group_by_id[issue_id], event=event, link_to_event=True
+                group_by_id[issue_id], event=event, link_to_event=True, is_unfurl=True
             )
     return out
 

+ 2 - 3
tests/sentry/integrations/slack/test_message_builder.py

@@ -259,12 +259,11 @@ class BuildGroupAttachmentTest(TestCase, PerformanceIssueTestCase, OccurrenceTes
     def test_escape_slack_message(self):
         group = self.create_group(
             project=self.project,
-            message="<https://example.com/|*Click Here*>",
             data={"type": "error", "metadata": {"value": "<https://example.com/|*Click Here*>"}},
         )
         assert (
             SlackIssuesMessageBuilder(group, None).build()["text"]
-            == "&amp;lt;https://example.com/|*Click Here*&amp;gt;"
+            == "&lt;https://example.com/|*Click Here*&gt;"
         )
 
 
@@ -554,7 +553,7 @@ class DummySlackNotificationTest(TestCase):
     def test_with_escape(self):
         raw_text = "<https://example.com/|*Click Here*>"
         assert DummySlackNotification(raw_text, True).build() == {
-            "text": "&amp;lt;https://example.com/|*Click Here*&amp;gt;",
+            "text": "&lt;https://example.com/|*Click Here*&gt;",
             "mrkdwn_in": [],
             "color": "#2788CE",
         }

+ 18 - 0
tests/sentry/integrations/slack/test_unfurl.py

@@ -19,6 +19,7 @@ from sentry.snuba.dataset import Dataset
 from sentry.testutils import TestCase
 from sentry.testutils.helpers import install_slack
 from sentry.testutils.helpers.datetime import before_now, iso_format
+from sentry.testutils.helpers.features import with_feature
 
 INTERVAL_COUNT = 300
 INTERVALS_PER_DAY = int(60 * 60 * 24 / INTERVAL_COUNT)
@@ -210,6 +211,23 @@ class UnfurlTest(TestCase):
             == SlackIssuesMessageBuilder(group2, event, link_to_event=True).build()
         )
 
+    @with_feature("organizations:slack-escape-messages")
+    def test_escape_issue(self):
+        group = self.create_group(
+            project=self.project,
+            data={"type": "error", "metadata": {"value": "<https://example.com/|*Click Here*>"}},
+        )
+
+        links = [
+            UnfurlableUrl(
+                url=f"https://sentry.io/organizations/{self.organization.slug}/issues/{group.id}/",
+                args={"issue_id": group.id, "event_id": None},
+            ),
+        ]
+
+        unfurls = link_handlers[LinkType.ISSUES].fn(self.request, self.integration, links)
+        assert unfurls[links[0].url]["text"] == "&amp;lt;https://example.com/|*Click Here*&amp;gt;"
+
     def test_unfurl_metric_alert(self):
         alert_rule = self.create_alert_rule()