Merge pull request #2540 from basecamp/fix-quotes

Fix double-escaped HTML entities in basecamp and campfire webhook payloads

app/models/event/description.rb

@@ -14,7 +14,7 @@ class Event::Description
   end
 
   def to_plain_text
-    to_sentence(creator_name, quoted(card.title))
+    to_sentence(creator_name, quoted(card.title)).html_safe
   end
 
   private
@@ -38,11 +38,11 @@ class Event::Description
     end
 
     def creator_name
-      h(event.creator.name)
+      h event.creator.name
     end
 
     def quoted(text)
-      %("#{h text}")
+      h %("#{text}")
     end
 
     def card
@@ -86,27 +86,43 @@ class Event::Description
       if event.assignees.include?(user)
         "#{creator} will handle #{card_title}"
       else
-        "#{creator} assigned #{h event.assignees.pluck(:name).to_sentence} to #{card_title}"
+        "#{creator} assigned #{assignee_names} to #{card_title}"
       end
     end
 
     def unassigned_sentence(creator, card_title)
-      assignees_text = event.assignees.include?(user) ? "yourself" : event.assignees.pluck(:name).to_sentence
-      "#{creator} unassigned #{h(assignees_text)} from #{card_title}"
+      "#{creator} unassigned #{unassigned_names} from #{card_title}"
     end
 
     def renamed_sentence(creator, card_title)
-      old_title = event.particulars.dig("particulars", "old_title")
-      %(#{creator} renamed #{card_title} (was: "#{h old_title}"))
+      %(#{creator} renamed #{card_title} (was: "#{old_title}"))
     end
 
     def moved_sentence(creator, card_title)
-      new_location = event.particulars.dig("particulars", "new_board") || event.particulars.dig("particulars", "new_collection")
-      %(#{creator} moved #{card_title} to "#{h new_location}")
+      %(#{creator} moved #{card_title} to "#{new_location}")
     end
 
     def triaged_sentence(creator, card_title)
-      column = event.particulars.dig("particulars", "column")
-      %(#{creator} moved #{card_title} to "#{h column}")
+      %(#{creator} moved #{card_title} to "#{column}")
+    end
+
+    def assignee_names
+      h event.assignees.pluck(:name).to_sentence
+    end
+
+    def unassigned_names
+      h(event.assignees.include?(user) ? "yourself" : assignee_names)
+    end
+
+    def old_title
+      h event.particulars.dig("particulars", "old_title")
+    end
+
+    def new_location
+      h(event.particulars.dig("particulars", "new_board") || event.particulars.dig("particulars", "new_collection"))
+    end
+
+    def column
+      h event.particulars.dig("particulars", "column")
     end
 end

test/models/event/description_test.rb

@@ -1,6 +1,12 @@
 require "test_helper"
 
 class Event::DescriptionTest < ActiveSupport::TestCase
+  test "html description is html safe" do
+    description = events(:logo_published).description_for(users(:david))
+
+    assert_predicate description.to_html, :html_safe?
+  end
+
   test "generates html description for card published event" do
     description = events(:logo_published).description_for(users(:david))
 
@@ -8,6 +14,12 @@ class Event::DescriptionTest < ActiveSupport::TestCase
     assert_includes description.to_html, "logo"
   end
 
+  test "plain text description is html safe" do
+    description = events(:logo_published).description_for(users(:david))
+
+    assert_predicate description.to_plain_text, :html_safe?
+  end
+
   test "generates plain text description for card published event" do
     description = events(:logo_published).description_for(users(:david))
 
@@ -33,6 +45,96 @@ class Event::DescriptionTest < ActiveSupport::TestCase
     assert_includes description.to_plain_text, "David added"
   end
 
+  test "to_html escapes assignee names" do
+    users(:jz).update_column(:name, "Tom & Jerry")
+    description = events(:logo_assignment_jz).description_for(users(:david))
+
+    assert_includes description.to_html, "Tom &amp; Jerry"
+    assert_not_includes description.to_html, "Tom & Jerry"
+  end
+
+  test "to_plain_text escapes assignee names" do
+    users(:jz).update_column(:name, "Tom & Jerry")
+    description = events(:logo_assignment_jz).description_for(users(:david))
+
+    assert_includes description.to_plain_text, "Tom &amp; Jerry"
+    assert_not_includes description.to_plain_text, "Tom &amp;amp; Jerry"
+  end
+
+  test "to_html escapes unassigned names" do
+    users(:jz).update_column(:name, "Tom & Jerry")
+    event = events(:logo_assignment_jz)
+    event.update_column(:action, "card_unassigned")
+    description = event.description_for(users(:david))
+
+    assert_includes description.to_html, "Tom &amp; Jerry"
+    assert_not_includes description.to_html, "Tom & Jerry"
+  end
+
+  test "to_plain_text escapes unassigned names" do
+    users(:jz).update_column(:name, "Tom & Jerry")
+    event = events(:logo_assignment_jz)
+    event.update_column(:action, "card_unassigned")
+    description = event.description_for(users(:david))
+
+    assert_includes description.to_plain_text, "Tom &amp; Jerry"
+    assert_not_includes description.to_plain_text, "Tom &amp;amp; Jerry"
+  end
+
+  test "to_html escapes old title in renamed description" do
+    event = events(:logo_published)
+    event.update_columns(action: "card_title_changed", particulars: { "particulars" => { "old_title" => "Tom & Jerry" } })
+    description = event.description_for(users(:david))
+
+    assert_includes description.to_html, "Tom &amp; Jerry"
+    assert_not_includes description.to_html, "Tom & Jerry"
+  end
+
+  test "to_plain_text escapes old title in renamed description" do
+    event = events(:logo_published)
+    event.update_columns(action: "card_title_changed", particulars: { "particulars" => { "old_title" => "Tom & Jerry" } })
+    description = event.description_for(users(:david))
+
+    assert_includes description.to_plain_text, "Tom &amp; Jerry"
+    assert_not_includes description.to_plain_text, "Tom &amp;amp; Jerry"
+  end
+
+  test "to_html escapes board name in moved description" do
+    event = events(:logo_published)
+    event.update_columns(action: "card_board_changed", particulars: { "particulars" => { "new_board" => "Tom & Jerry" } })
+    description = event.description_for(users(:david))
+
+    assert_includes description.to_html, "Tom &amp; Jerry"
+    assert_not_includes description.to_html, "Tom & Jerry"
+  end
+
+  test "to_plain_text escapes board name in moved description" do
+    event = events(:logo_published)
+    event.update_columns(action: "card_board_changed", particulars: { "particulars" => { "new_board" => "Tom & Jerry" } })
+    description = event.description_for(users(:david))
+
+    assert_includes description.to_plain_text, "Tom &amp; Jerry"
+    assert_not_includes description.to_plain_text, "Tom &amp;amp; Jerry"
+  end
+
+  test "to_html escapes column name in triaged description" do
+    event = events(:logo_published)
+    event.update_columns(action: "card_triaged", particulars: { "particulars" => { "column" => "Tom & Jerry" } })
+    description = event.description_for(users(:david))
+
+    assert_includes description.to_html, "Tom &amp; Jerry"
+    assert_not_includes description.to_html, "Tom & Jerry"
+  end
+
+  test "to_plain_text escapes column name in triaged description" do
+    event = events(:logo_published)
+    event.update_columns(action: "card_triaged", particulars: { "particulars" => { "column" => "Tom & Jerry" } })
+    description = event.description_for(users(:david))
+
+    assert_includes description.to_plain_text, "Tom &amp; Jerry"
+    assert_not_includes description.to_plain_text, "Tom &amp;amp; Jerry"
+  end
+
   test "escapes html in card titles in plain text description" do
     card = cards(:logo)
     card.update_column(:title, "<script>alert('xss')</script>")

test/models/webhook/delivery_test.rb

@@ -245,6 +245,102 @@ class Webhook::DeliveryTest < ActiveSupport::TestCase
     assert_requested request_stub
   end
 
+  test "basecamp webhook payload html-escapes special characters" do
+    cards(:logo).update_column(:title, %(Tom & Jerry's <Great> "Adventure"))
+
+    webhook = Webhook.create!(
+      board: boards(:writebook),
+      name: "Basecamp",
+      url: "https://3.basecamp.com/123/integrations/webhook/buckets/456/chats/789/lines"
+    )
+    delivery = Webhook::Delivery.create!(webhook: webhook, event: events(:logo_published))
+
+    captured_body = nil
+    stub_request(:post, webhook.url)
+      .with { |request| captured_body = request.body; true }
+      .to_return(status: 200)
+
+    delivery.deliver
+
+    content = CGI.parse(captured_body)["content"].first
+
+    expected = <<~HTML.strip
+      David added &quot;Tom &amp; Jerry&#39;s &lt;Great&gt; &quot;Adventure&quot;&quot;
+      <a href="http://example.org/897362094/cards/1">↗︎</a>
+    HTML
+    assert_equal expected, content
+  end
+
+  test "slack webhook payload html-escapes special characters" do
+    cards(:logo).update_column(:title, %(Tom & Jerry's <Great> "Adventure"))
+
+    webhook = Webhook.create!(
+      board: boards(:writebook),
+      name: "Slack",
+      url: "https://hooks.slack.com/services/T12345678/B12345678/abcdefghijklmnopqrstuvwx" # gitleaks:allow
+    )
+    delivery = Webhook::Delivery.create!(webhook: webhook, event: events(:logo_published))
+
+    captured_body = nil
+    stub_request(:post, webhook.url)
+      .with { |request| captured_body = request.body; true }
+      .to_return(status: 200)
+
+    delivery.deliver
+
+    text = JSON.parse(captured_body)["text"]
+
+    expected = <<~TEXT.strip
+      David added &quot;Tom &amp; Jerry&#39;s &lt;Great&gt; &quot;Adventure&quot;&quot; <http://example.com/897362094/cards/1|Open in Fizzy>
+    TEXT
+    assert_equal expected, text
+  end
+
+  test "campfire webhook payload html-escapes special characters" do
+    cards(:logo).update_column(:title, %(Tom & Jerry's <Great> "Adventure"))
+
+    webhook = Webhook.create!(
+      board: boards(:writebook),
+      name: "Campfire",
+      url: "https://example.com/rooms/123/456-room-name/messages"
+    )
+    delivery = Webhook::Delivery.create!(webhook: webhook, event: events(:logo_published))
+
+    captured_body = nil
+    stub_request(:post, webhook.url)
+      .with { |request| captured_body = request.body; true }
+      .to_return(status: 200)
+
+    delivery.deliver
+
+    expected = <<~HTML.strip
+      David added &quot;Tom &amp; Jerry&#39;s &lt;Great&gt; &quot;Adventure&quot;&quot;
+      <a href="http://example.org/897362094/cards/1">↗︎</a>
+    HTML
+    assert_equal expected, captured_body
+  end
+
+  test "generic webhook payload json-encodes special characters" do
+    cards(:logo).update_column(:title, %(Tom & Jerry's <Great> "Adventure"))
+
+    webhook = Webhook.create!(
+      board: boards(:writebook),
+      name: "Generic",
+      url: "https://example.com/webhook"
+    )
+    delivery = Webhook::Delivery.create!(webhook: webhook, event: events(:logo_published))
+
+    captured_body = nil
+    stub_request(:post, webhook.url)
+      .with { |request| captured_body = request.body; true }
+      .to_return(status: 200)
+
+    delivery.deliver
+
+    json = JSON.parse(captured_body)
+    assert_equal %(Tom & Jerry's <Great> "Adventure"), json["eventable"]["title"]
+  end
+
   test "renders creator name when event creator is not current user" do
     webhook = Webhook.create!(
       board: boards(:writebook),