From d3f22d78e14e778a104db9cacd92a89e693ced0e Mon Sep 17 00:00:00 2001
From: Jeremy Daer <jeremy@37signals.com>
Date: Mon, 10 Aug 2026 15:55:02 -0700
Subject: [PATCH] Harden message content filtering + bump thruster to 0.1.23
 (#237)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* Bump thruster 0.1.15 → 0.1.23

* Scrub disallowed attributes on allowed tags in message rendering

SanitizeTags removes disallowed tags from message presentation, but
attributes on the tags it allows passed through untouched. Extend the
content-filter chain with a SanitizeAttributes filter that runs Rails'
safe-list sanitizer over the remaining markup, stripping event-handler
attributes and unsafe URI schemes as defense-in-depth alongside the
existing Content-Security-Policy.

Running it after SanitizeTags with the same allowed-tags list makes the
sanitizer's tag pass a no-op, preserving SanitizeTags' remove-not-unwrap
semantics while scrubbing attributes. The attribute allowlist is the
standard ActionText set (which keeps attachments intact) plus class,
which presentation styling relies on.
---
 Gemfile.lock                                  |  8 +--
 app/helpers/content_filters.rb                |  2 +-
 .../content_filters/sanitize_attributes.rb    | 47 +++++++++++++
 app/views/messages/_message.html.erb          |  3 +-
 test/helpers/content_filters_test.rb          | 69 +++++++++++++++++++
 test/helpers/messages_helper_test.rb          | 27 ++++++++
 6 files changed, 150 insertions(+), 6 deletions(-)
 create mode 100644 app/helpers/content_filters/sanitize_attributes.rb
 create mode 100644 test/helpers/messages_helper_test.rb

diff --git a/Gemfile.lock b/Gemfile.lock
index 1cd6c74..96a6368 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -375,10 +375,10 @@ GEM
       railties (>= 6.0.0)
     stringio (3.1.8)
     thor (1.4.0)
-    thruster (0.1.15-aarch64-linux)
-    thruster (0.1.15-arm64-darwin)
-    thruster (0.1.15-x86_64-darwin)
-    thruster (0.1.15-x86_64-linux)
+    thruster (0.1.23-aarch64-linux)
+    thruster (0.1.23-arm64-darwin)
+    thruster (0.1.23-x86_64-darwin)
+    thruster (0.1.23-x86_64-linux)
     tilt (2.6.1)
     timeout (0.6.1)
     tsort (0.2.0)
diff --git a/app/helpers/content_filters.rb b/app/helpers/content_filters.rb
index 926a726..7a2737c 100644
--- a/app/helpers/content_filters.rb
+++ b/app/helpers/content_filters.rb
@@ -1,3 +1,3 @@
 module ContentFilters
-  TextMessagePresentationFilters = ActionText::Content::Filters.new(RemoveSoloUnfurledLinkText, StyleUnfurledTwitterAvatars, SanitizeTags)
+  TextMessagePresentationFilters = ActionText::Content::Filters.new(RemoveSoloUnfurledLinkText, StyleUnfurledTwitterAvatars, SanitizeTags, SanitizeAttributes)
 end
diff --git a/app/helpers/content_filters/sanitize_attributes.rb b/app/helpers/content_filters/sanitize_attributes.rb
new file mode 100644
index 0000000..47dedc1
--- /dev/null
+++ b/app/helpers/content_filters/sanitize_attributes.rb
@@ -0,0 +1,47 @@
+class ContentFilters::SanitizeAttributes < ActionText::Content::Filter
+  def applicable?
+    true
+  end
+
+  # Scrub attributes on the tags that SanitizeTags allows, using Rails' safe-list
+  # sanitizer so unsafe URI schemes and event-handler attributes are stripped.
+  # Runs after SanitizeTags, so passing the same allowed tags makes the tag pass
+  # a no-op and only attributes are scrubbed.
+  def apply
+    sanitizer.sanitize fragment.to_html, tags: allowed_tags, attributes: allowed_attributes
+  end
+
+  private
+    # Presentation styling relies on class attributes (e.g. unfurled link embeds),
+    # which the standard ActionText set doesn't include.
+    EXTRA_ALLOWED_ATTRIBUTES = %w[ class ]
+
+    # ActionText::ContentHelper.sanitizer is a single process-wide instance, and
+    # rails-html-sanitizer reuses one mutable permit scrubber that it reconfigures
+    # from the tags/attributes on every #sanitize call. Sharing it here would race
+    # our stricter tag set against ActionText's default rendering on concurrent
+    # requests, so use a dedicated instance of the same sanitizer class to keep the
+    # scrubber isolated.
+    def sanitizer
+      sanitizer_class.new
+    end
+
+    def sanitizer_class
+      ActionText::ContentHelper.sanitizer.class
+    end
+
+    def allowed_tags
+      ContentFilters::SanitizeTags::ALLOWED_TAGS
+    end
+
+    def allowed_attributes
+      standard_allowed_attributes + EXTRA_ALLOWED_ATTRIBUTES
+    end
+
+    # Mirrors ActionText::ContentHelper#sanitizer_allowed_attributes, which isn't
+    # exposed at the module level.
+    def standard_allowed_attributes
+      ActionText::ContentHelper.allowed_attributes ||
+        (sanitizer_class.allowed_attributes + ActionText::Attachment::ATTRIBUTES).to_a
+    end
+end
diff --git a/app/views/messages/_message.html.erb b/app/views/messages/_message.html.erb
index db41dcb..b3b1213 100644
--- a/app/views/messages/_message.html.erb
+++ b/app/views/messages/_message.html.erb
@@ -1,6 +1,7 @@
 <%# Be sure to check/update messages/_template.html.erb when changing this file %>
 
-<% cache message do %>
+<%# Bump this version when the message presentation filters change what they emit. Editing this line changes the template digest, which busts BOTH this fragment cache and the collection cache that keys on this partial's digest (helper Ruby changes alone don't). %>
+<% cache [ message, "presentation-v2" ] do %>
   <%= message_tag message do %>
     <h2 class="message__day-separator"><%= local_datetime_tag message.created_at, style: :date %></h2>
 
diff --git a/test/helpers/content_filters_test.rb b/test/helpers/content_filters_test.rb
index 00b2709..14f48de 100644
--- a/test/helpers/content_filters_test.rb
+++ b/test/helpers/content_filters_test.rb
@@ -61,6 +61,75 @@ class ContentFiltersTest < ActionView::TestCase
     assert_equal "Hello World", filtered.to_html
   end
 
+  test "message with a link using an unsafe URI scheme" do
+    message = Message.create! room: rooms(:pets), body: '<div><a href="javascript:alert(1)">x</a></div>', client_message_id: "0015", creator: users(:jason)
+
+    filtered = ContentFilters::TextMessagePresentationFilters.apply(message.body.body)
+    assert_no_match /javascript:/, filtered.to_html
+    assert_match /<a>x<\/a>/, filtered.to_html
+  end
+
+  test "message with an event handler attribute on an allowed tag" do
+    message = Message.create! room: rooms(:pets), body: '<div><a href="/x" onmouseover="alert(1)">x</a> <span onclick="alert(2)">y</span></div>', client_message_id: "0015", creator: users(:jason)
+
+    filtered = ContentFilters::TextMessagePresentationFilters.apply(message.body.body)
+    assert_no_match /onmouseover/, filtered.to_html
+    assert_no_match /onclick/, filtered.to_html
+    assert_match /<a href="\/x">x<\/a>/, filtered.to_html
+    assert_match /<span>y<\/span>/, filtered.to_html
+  end
+
+  test "message with a data URI link" do
+    message = Message.create! room: rooms(:pets), body: '<div><a href="data:text/html,pwned">x</a></div>', client_message_id: "0015", creator: users(:jason)
+
+    filtered = ContentFilters::TextMessagePresentationFilters.apply(message.body.body)
+    assert_no_match /data:/, filtered.to_html
+    assert_match /<a>x<\/a>/, filtered.to_html
+  end
+
+  test "message with a safe link and formatting is preserved" do
+    body = '<div><a href="https://example.com">example</a> <strong>bold</strong> <code>code</code><ul><li>one</li><li>two</li></ul></div>'
+    message = Message.create! room: rooms(:pets), body: body, client_message_id: "0015", creator: users(:jason)
+
+    filtered = ContentFilters::TextMessagePresentationFilters.apply(message.body.body)
+    assert_match /<a href="https:\/\/example\.com">example<\/a>/, filtered.to_html
+    assert_match /<strong>bold<\/strong>/, filtered.to_html
+    assert_match /<code>code<\/code>/, filtered.to_html
+    assert_match /<ul>\s*<li>one<\/li>\s*<li>two<\/li>\s*<\/ul>/, filtered.to_html
+  end
+
+  test "SanitizeAttributes uses an isolated sanitizer rather than the shared ActionText instance" do
+    filter = ContentFilters::SanitizeAttributes.new(ActionText::Content.new("<div>x</div>"))
+
+    isolated = filter.send(:sanitizer)
+    assert_not_same ActionText::ContentHelper.sanitizer, isolated,
+      "must not reuse the process-wide ActionText sanitizer, whose permit scrubber is mutated per call"
+    assert_not_same isolated, filter.send(:sanitizer),
+      "each call must build a fresh sanitizer so concurrent requests never share scrubber state"
+    assert_kind_of ActionText::ContentHelper.sanitizer.class, isolated
+  end
+
+  test "SanitizeAttributes neutralizes unsafe input and preserves benign content on its isolated sanitizer" do
+    body = <<~HTML.squish
+      <div><a href="javascript:alert(1)" onclick="x()">link</a>
+      <a href="data:text/html,pwned">data</a>
+      <span class="cf-twitter-avatar" onmouseover="y()">avatar</span>
+      <img src="https://evil.example/x.svg"> Hey #{mention_attachment_for(:david)}</div>
+    HTML
+    message = Message.create! room: rooms(:pets), body: body, client_message_id: "0015", creator: users(:jason)
+
+    filtered = ContentFilters::SanitizeAttributes.apply(message.body.body).to_html
+
+    assert_no_match /javascript:/, filtered
+    assert_no_match /data:text\/html/, filtered
+    assert_no_match /onclick/, filtered
+    assert_no_match /onmouseover/, filtered
+    assert_no_match /evil\.example/, filtered
+    assert_match /<span class="cf-twitter-avatar">avatar<\/span>/, filtered
+    assert_match />link</, filtered
+    assert_match /<action-text-attachment sgid="#{users(:david).attachable_sgid}"/, filtered
+  end
+
   test "message with a mention attachment" do
     message = Message.create! room: rooms(:pets), body: "<div>Hey #{mention_attachment_for(:david)}</div>", creator: users(:jason)
 
diff --git a/test/helpers/messages_helper_test.rb b/test/helpers/messages_helper_test.rb
new file mode 100644
index 0000000..d2655a8
--- /dev/null
+++ b/test/helpers/messages_helper_test.rb
@@ -0,0 +1,27 @@
+require "test_helper"
+
+class MessagesHelperTest < ActionView::TestCase
+  test "message_presentation neutralizes unsafe URI schemes in links" do
+    message = Message.create! room: rooms(:pets), body: '<div><a href="javascript:alert(1)">x</a></div>', client_message_id: "0015", creator: users(:jason)
+
+    presentation = view.message_presentation(message)
+    assert_no_match /javascript:/, presentation
+    assert_match /<a>x<\/a>/, presentation
+  end
+
+  test "message_presentation strips event handler attributes from allowed tags" do
+    message = Message.create! room: rooms(:pets), body: '<div><a href="/x" onmouseover="alert(1)">x</a></div>', client_message_id: "0015", creator: users(:jason)
+
+    presentation = view.message_presentation(message)
+    assert_no_match /onmouseover/, presentation
+    assert_match /<a href="\/x">x<\/a>/, presentation
+  end
+
+  test "message_presentation preserves safe links and formatting" do
+    message = Message.create! room: rooms(:pets), body: '<div><a href="https://example.com">example</a> <strong>bold</strong></div>', client_message_id: "0015", creator: users(:jason)
+
+    presentation = view.message_presentation(message)
+    assert_match /<a href="https:\/\/example\.com"[^>]*>example<\/a>/, presentation
+    assert_match /<strong>bold<\/strong>/, presentation
+  end
+end