Merge pull request #276 from basecamp/card-7348960853-room-render-injection

Render link previews only from web URLs

app/views/action_text/attachables/_opengraph_embed.html.erb

@@ -3,9 +3,9 @@
     <div class="og-embed gap">
       <div class="og-embed__content">
         <div class="og-embed__title">
-          <%= link_to truncate(opengraph_embed.filename, length: 280, omission: "…"), opengraph_embed.href, rel: "noreferrer", target: "_blank" %>
+          <%= link_to_if opengraph_embed.href.present?, truncate(opengraph_embed.filename, length: 280, omission: "…"), opengraph_embed.href, rel: "noreferrer", target: "_blank" %>
         </div>
-        <div class="og-embed__description"><%= truncate(opengraph_embed.caption, length: 560, omission: "…").html_safe %></div>
+        <div class="og-embed__description"><%= truncate(opengraph_embed.caption, length: 560, omission: "…") %></div>
       </div>
       <% if opengraph_embed.url %>
         <div class="og-embed__image">

app/views/messages/_message.html.erb

@@ -1,7 +1,7 @@
 <%# Be sure to check/update messages/_template.html.erb when changing this file %>
 
 <%# 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 %>
+<% cache [ message, "presentation-v3" ] do %>
   <%= message_tag message do %>
     <h2 class="message__day-separator"><%= local_datetime_tag message.created_at, style: :date %></h2>
 

lib/rails_ext/actiontext_opengraph_embeds.rb

@@ -16,12 +16,54 @@ class ActionText::Attachment::OpengraphEmbed
     private
       def attributes_from_node(node)
         {
-          href: node["href"],
-          url: node["url"],
+          href: web_url(node["href"]),
+          url: web_url(node["url"]),
           filename: node["filename"],
           description: node["caption"]
         }
       end
+
+      # A link preview points at what we unfurled: an absolute http or https URL
+      # on some other host. Drop anything else a message body asks for, so it
+      # can't aim the preview's link or its image at this Campfire and have every
+      # reader's browser fetch it with their session attached.
+      def web_url(value)
+        return if value.blank?
+
+        parsed = URI.parse(value)
+        value if parsed.is_a?(URI::HTTP) && elsewhere?(parsed.host)
+      rescue URI::InvalidURIError
+        nil
+      end
+
+      # "https:/rooms/1" parses as HTTPS with no host at all, and a browser
+      # resolves both that and our own hostname against the origin Campfire is
+      # served from. A percent-escape hides our hostname from this comparison
+      # while a browser still unescapes it back to us, so an escaped host is out
+      # too, and neither case is anything an unfurl could have produced.
+      def elsewhere?(host)
+        return false unless named_host?(host)
+
+        canonical_host(host) != canonical_host(Current.request_host.to_s)
+      end
+
+      # A preview names a page on the public internet, so its host is a domain
+      # name, written plainly. A bare address is not one, and a browser rewrites
+      # the many spellings of an address ("2130706433", "0x7f.0.0.1") into a
+      # single one before it fetches, which is a race a comparison here loses.
+      def named_host?(host)
+        host.present? && host.exclude?("%") && host.include?(".") && domain_ending?(host.split(".").last)
+      end
+
+      # What keeps a name from reading as an address is its last label, which is
+      # a word: never a number, and never the hexadecimal spelling of one.
+      def domain_ending?(label)
+        label.match?(/[a-z]/i) && !label.match?(/\A0x/i)
+      end
+
+      def canonical_host(host)
+        host.downcase.delete_suffix(".")
+      end
   end
 
   attr_accessor :href, :url, :filename, :description

test/controllers/rooms_controller_test.rb

@@ -20,6 +20,51 @@ class RoomsControllerTest < ActionDispatch::IntegrationTest
     assert response.cookies[:last_room] = users(:david).rooms.last.id
   end
 
+  test "show renders a link preview written by hand without its off-scheme image and link" do
+    room = rooms(:watercooler)
+    post room_messages_url(room, format: :turbo_stream), params: { message: {
+      body: link_preview_body(href: "javascript:alert(1)", url: "data:image/svg+xml;base64,PHN2Zy8+"),
+      client_message_id: "hand-written-preview" } }
+    assert_response :success
+
+    get room_url(room)
+
+    assert_response :success
+    assert_no_match /javascript:alert/, response.body
+    assert_no_match /data:image\/svg/, response.body
+    assert_match "Free cookies", response.body
+  end
+
+  test "show renders a link preview written by hand without its image pointed at this Campfire" do
+    room = rooms(:watercooler)
+    own_url = room_url(room, host: "www.example.com")
+    post room_messages_url(room, format: :turbo_stream), params: { message: {
+      body: link_preview_body(href: own_url, url: own_url),
+      client_message_id: "same-host-preview" } }
+    assert_response :success
+
+    get room_url(room)
+
+    assert_response :success
+    assert_no_match %r{<img src="#{Regexp.escape(own_url)}"}, response.body
+    assert_no_match %r{<a rel="noreferrer" target="_blank" href="#{Regexp.escape(own_url)}"}, response.body
+    assert_match "Free cookies", response.body
+  end
+
+  test "show renders an unfurled link preview" do
+    room = rooms(:watercooler)
+    post room_messages_url(room, format: :turbo_stream), params: { message: {
+      body: link_preview_body(href: "https://example.com/page", url: "https://example.com/image.png"),
+      client_message_id: "unfurled-preview" } }
+    assert_response :success
+
+    get room_url(room)
+
+    assert_response :success
+    assert_match %r{<img src="https://example\.com/image\.png"}, response.body
+    assert_match %r{href="https://example\.com/page"}, response.body
+  end
+
   test "destroy" do
     assert_turbo_stream_broadcasts :rooms, count: 1 do
       assert_difference -> { Room.count }, -1 do
@@ -42,4 +87,10 @@ class RoomsControllerTest < ActionDispatch::IntegrationTest
       delete room_url(rooms(:designers))
     end
   end
+
+  private
+    def link_preview_body(href:, url:)
+      %(<div><action-text-attachment content-type="application/vnd.actiontext.opengraph-embed" ) +
+        %(href="#{href}" url="#{url}" filename="Free cookies" caption="Cookies here"></action-text-attachment></div>)
+    end
 end

test/lib/rails_ext/actiontext_opengraph_embeds_test.rb

@@ -0,0 +1,102 @@
+require "test_helper"
+
+class ActionText::Attachment::OpengraphEmbedTest < ActiveSupport::TestCase
+  test "keeps absolute http and https links and images" do
+    embed = embed_from href: "http://example.com/page", url: "https://example.com/image.png"
+
+    assert_equal "http://example.com/page", embed.href
+    assert_equal "https://example.com/image.png", embed.url
+  end
+
+  test "drops a link and an image that aren't web URLs" do
+    [ "javascript:alert(1)", "data:text/html,pwned", "vbscript:msgbox(1)", "//example.com/image.png",
+      "/rooms/1", "rooms/1", "", "http://exa mple.com/ ",
+      "https:/rooms/1", "https:rooms/1", "http:/rooms/1", "https://", "http://:80/rooms/1" ].each do |value|
+      embed = embed_from href: value, url: value
+
+      assert_nil embed.href, "expected #{value.inspect} to be dropped as a link"
+      assert_nil embed.url, "expected #{value.inspect} to be dropped as an image"
+    end
+  end
+
+  test "drops a link and an image on this Campfire's own host, however it is spelled" do
+    Current.set request: ActionDispatch::TestRequest.create("HTTP_HOST" => "once.campfire.test") do
+      [ "https://once.campfire.test/rooms/1", "http://once.campfire.test/rooms/1",
+        "https://ONCE.Campfire.Test/rooms/1", "https://once.campfire.test./rooms/1",
+        "https://%6fnce.campfire.test/rooms/1", "https://%77ww.example.com/x.png" ].each do |value|
+        embed = embed_from href: value, url: value
+
+        assert_nil embed.href, "expected #{value.inspect} to be dropped as a link"
+        assert_nil embed.url, "expected #{value.inspect} to be dropped as an image"
+      end
+
+      embed = embed_from href: "https://example.com/page", url: "https://example.com/image.png"
+      assert_equal "https://example.com/page", embed.href
+      assert_equal "https://example.com/image.png", embed.url
+    end
+  end
+
+  test "drops a link and an image on a bare address rather than a domain name" do
+    [ "http://127.0.0.1/rooms/1", "http://2130706433/rooms/1", "http://0177.0.0.1/rooms/1",
+      "http://0x7f.0.0.1/rooms/1", "http://1.2.3.0xff/rooms/1", "http://[::1]/rooms/1",
+      "http://localhost/rooms/1", "https://203.0.113.10/image.png" ].each do |value|
+      embed = embed_from href: value, url: value
+
+      assert_nil embed.href, "expected #{value.inspect} to be dropped as a link"
+      assert_nil embed.url, "expected #{value.inspect} to be dropped as an image"
+    end
+  end
+
+  test "keeps an internationalized domain written in punycode" do
+    embed = embed_from href: "https://xn--80aswg.xn--p1ai/page", url: "https://xn--80aswg.xn--p1ai/image.png"
+
+    assert_equal "https://xn--80aswg.xn--p1ai/page", embed.href
+    assert_equal "https://xn--80aswg.xn--p1ai/image.png", embed.url
+  end
+
+  test "renders the image and the link when both are web URLs" do
+    html = render_embed href: "https://example.com/page", url: "https://example.com/image.png"
+
+    assert_match %r{<a rel="noreferrer" target="_blank" href="https://example\.com/page">Title</a>}, html
+    assert_match %r{<img src="https://example\.com/image\.png"}, html
+  end
+
+  test "renders no image and no link when neither is a web URL" do
+    html = render_embed href: "javascript:alert(1)", url: "data:image/svg+xml;base64,PHN2Zy8+"
+
+    assert_no_match /javascript:/, html
+    assert_no_match /data:/, html
+    assert_no_match /<img/, html
+    assert_no_match /<a /, html
+    assert_match "Title", html
+  end
+
+  test "renders the title and the description as text" do
+    html = render_embed href: "https://example.com/page", url: "https://example.com/image.png",
+      filename: "<b>Title</b>", caption: "<img src=x onerror=alert(1)>"
+
+    assert_no_match /<b>/, html
+    assert_no_match /<img src=x/, html
+    assert_match "&lt;b&gt;Title&lt;/b&gt;", html
+    assert_match "&lt;img src=x onerror=alert(1)&gt;", html
+  end
+
+  private
+    def attachment_for(href:, url:, filename: "Title", caption: "Description")
+      html = %(<action-text-attachment content-type="application/vnd.actiontext.opengraph-embed" ) +
+        %(href="#{href}" url="#{url}" filename="#{filename}" caption="#{caption}"></action-text-attachment>)
+      node = ActionText::Fragment.wrap(html).find_all(ActionText::Attachment.tag_name).first
+
+      ActionText::Attachment.from_node(node)
+    end
+
+    def embed_from(**attributes)
+      attachment_for(**attributes).attachable
+    end
+
+    def render_embed(**attributes)
+      attachment = attachment_for(**attributes)
+
+      ApplicationController.render partial: attachment.to_partial_path, locals: { opengraph_embed: attachment }
+    end
+end