From 0c10622672023b836bec2e544d12644b05aa5ff1 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Fri, 13 Feb 2026 01:47:34 -0500 Subject: [PATCH] Fix double-escaped HTML entities in webhook payloads `Event::Description#to_plain_text` now returns an html-safe string to prevent double-escaping when the ERB template renders the basecamp and campfire payloads. ref: https://3.basecamp.com/2914079/buckets/27/card_tables/cards/9574495379 --- app/models/event/description.rb | 40 +++++++---- test/models/event/description_test.rb | 94 +++++++++++++++++++++++++- test/models/webhook/delivery_test.rb | 96 +++++++++++++++++++++++++++ 3 files changed, 216 insertions(+), 14 deletions(-) diff --git a/app/models/event/description.rb b/app/models/event/description.rb index fd7a66ae1..454e636bb 100644 --- a/app/models/event/description.rb +++ b/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 diff --git a/test/models/event/description_test.rb b/test/models/event/description_test.rb index 9ff271c38..b280ad1d3 100644 --- a/test/models/event/description_test.rb +++ b/test/models/event/description_test.rb @@ -14,10 +14,10 @@ class Event::DescriptionTest < ActiveSupport::TestCase assert_includes description.to_html, "logo" end - test "plain text description is not html safe" do + test "plain text description is html safe" do description = events(:logo_published).description_for(users(:david)) - assert_not_predicate description.to_plain_text, :html_safe? + assert_predicate description.to_plain_text, :html_safe? end test "generates plain text description for card published event" do @@ -45,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 & 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 & Jerry" + assert_not_includes description.to_plain_text, "Tom &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 & 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 & Jerry" + assert_not_includes description.to_plain_text, "Tom &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 & 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 & Jerry" + assert_not_includes description.to_plain_text, "Tom &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 & 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 & Jerry" + assert_not_includes description.to_plain_text, "Tom &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 & 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 & Jerry" + assert_not_includes description.to_plain_text, "Tom &amp; Jerry" + end + test "escapes html in card titles in plain text description" do card = cards(:logo) card.update_column(:title, "") diff --git a/test/models/webhook/delivery_test.rb b/test/models/webhook/delivery_test.rb index d4d95528c..8d40c4417 100644 --- a/test/models/webhook/delivery_test.rb +++ b/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 "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 "Tom & Jerry's <Great> "Adventure"" + ↗︎ + HTML + assert_equal expected, content + end + + test "slack webhook payload html-escapes special characters" do + cards(:logo).update_column(:title, %(Tom & Jerry's "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 "Tom & Jerry's <Great> "Adventure"" + TEXT + assert_equal expected, text + end + + test "campfire webhook payload html-escapes special characters" do + cards(:logo).update_column(:title, %(Tom & Jerry's "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 "Tom & Jerry's <Great> "Adventure"" + ↗︎ + HTML + assert_equal expected, captured_body + end + + test "generic webhook payload json-encodes special characters" do + cards(:logo).update_column(:title, %(Tom & Jerry's "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 "Adventure"), json["eventable"]["title"] + end + test "renders creator name when event creator is not current user" do webhook = Webhook.create!( board: boards(:writebook),