From c186a46910431020e8efc425dec2133e7a99fa9a Mon Sep 17 00:00:00 2001
From: Bianca Nenciu <nbianca@users.noreply.github.com>
Date: Wed, 25 Jan 2023 19:17:21 +0200
Subject: [PATCH] SECURITY: Prevent XSS in local oneboxes (#20008)

Co-authored-by: OsamaSayegh <asooomaasoooma90@gmail.com>
---
 ...20_trigger_post_rebake_local_onebox_xss.rb | 22 +++++++
 lib/oneboxer.rb                               | 19 ++++--
 spec/lib/oneboxer_spec.rb                     | 60 +++++++++++++++++++
 3 files changed, 97 insertions(+), 4 deletions(-)
 create mode 100644 db/post_migrate/20230105153520_trigger_post_rebake_local_onebox_xss.rb

diff --git a/db/post_migrate/20230105153520_trigger_post_rebake_local_onebox_xss.rb b/db/post_migrate/20230105153520_trigger_post_rebake_local_onebox_xss.rb
new file mode 100644
index 00000000000..e9981e4d2c0
--- /dev/null
+++ b/db/post_migrate/20230105153520_trigger_post_rebake_local_onebox_xss.rb
@@ -0,0 +1,22 @@
+# frozen_string_literal: true
+
+class TriggerPostRebakeLocalOneboxXss < ActiveRecord::Migration[7.0]
+  def up
+    val =
+      DB.query_single(
+        "SELECT value FROM site_settings WHERE name = 'content_security_policy'",
+      ).first
+
+    return if val == nil || val == "t"
+
+    DB.exec(<<~SQL)
+      UPDATE posts
+      SET baked_version = NULL
+      WHERE cooked LIKE '%<a href=%'
+    SQL
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end
diff --git a/lib/oneboxer.rb b/lib/oneboxer.rb
index 6d6bd34f14f..eb36ebad21e 100644
--- a/lib/oneboxer.rb
+++ b/lib/oneboxer.rb
@@ -343,7 +343,8 @@ module Oneboxer
         end
       end
 
-    html = html.presence || "<a href='#{URI(url).to_s}'>#{URI(url).to_s}</a>"
+    normalized_url = ::Onebox::Helpers.normalize_url_for_output(URI(url).to_s)
+    html = html.presence || "<a href='#{normalized_url}'>#{normalized_url}</a>"
     { onebox: html, preview: html }
   end
 
@@ -355,18 +356,28 @@ module Oneboxer
         ""
       end
 
+    normalized_url = ::Onebox::Helpers.normalize_url_for_output(url)
     case File.extname(URI(url).path || "")
     when VIDEO_REGEX
       <<~HTML
         <div class="onebox video-onebox">
           <video #{additional_controls} width="100%" height="100%" controls="">
-            <source src='#{url}'>
-            <a href='#{url}'>#{url}</a>
+            <source src='#{normalized_url}'>
+            <a href='#{normalized_url}'>
+              #{normalized_url}
+            </a>
           </video>
         </div>
       HTML
     when AUDIO_REGEX
-      "<audio #{additional_controls} controls><source src='#{url}'><a href='#{url}'>#{url}</a></audio>"
+      <<~HTML
+        <audio #{additional_controls} controls>
+          <source src='#{normalized_url}'>
+          <a href='#{normalized_url}'>
+            #{normalized_url}
+          </a>
+        </audio>
+      HTML
     end
   end
 
diff --git a/spec/lib/oneboxer_spec.rb b/spec/lib/oneboxer_spec.rb
index ae969d015ed..7e23517425a 100644
--- a/spec/lib/oneboxer_spec.rb
+++ b/spec/lib/oneboxer_spec.rb
@@ -198,6 +198,66 @@ RSpec.describe Oneboxer do
         "<p><a href=\"#{Discourse.base_url}/new?%27class=black\">http://test.localhost/new?%27class=black</a></p>",
       )
     end
+
+    it "escapes URLs of local audio uploads" do
+      result =
+        described_class.onebox_raw(
+          "#{Discourse.base_url}/uploads/default/original/1X/a1c31803be81b85ecafc4f77b1008eee9b3b82f4.wav#'<>",
+        )
+      expect(result[:onebox]).to eq(<<~HTML)
+        <audio  controls>
+          <source src='http://test.localhost/uploads/default/original/1X/a1c31803be81b85ecafc4f77b1008eee9b3b82f4.wav#&apos;%3C%3E'>
+          <a href='http://test.localhost/uploads/default/original/1X/a1c31803be81b85ecafc4f77b1008eee9b3b82f4.wav#&apos;%3C%3E'>
+            http://test.localhost/uploads/default/original/1X/a1c31803be81b85ecafc4f77b1008eee9b3b82f4.wav#&apos;%3C%3E
+          </a>
+        </audio>
+      HTML
+      expect(result[:preview]).to eq(<<~HTML)
+        <audio  controls>
+          <source src='http://test.localhost/uploads/default/original/1X/a1c31803be81b85ecafc4f77b1008eee9b3b82f4.wav#&apos;%3C%3E'>
+          <a href='http://test.localhost/uploads/default/original/1X/a1c31803be81b85ecafc4f77b1008eee9b3b82f4.wav#&apos;%3C%3E'>
+            http://test.localhost/uploads/default/original/1X/a1c31803be81b85ecafc4f77b1008eee9b3b82f4.wav#&apos;%3C%3E
+          </a>
+        </audio>
+      HTML
+    end
+
+    it "escapes URLs of local video uploads" do
+      result =
+        described_class.onebox_raw(
+          "#{Discourse.base_url}/uploads/default/original/1X/a1c31803be81b85ecafc4f77b1008eee9b3b82f4.mp4#'<>",
+        )
+      expect(result[:onebox]).to eq(<<~HTML)
+        <div class="onebox video-onebox">
+          <video  width="100%" height="100%" controls="">
+            <source src='http://test.localhost/uploads/default/original/1X/a1c31803be81b85ecafc4f77b1008eee9b3b82f4.mp4#&apos;%3C%3E'>
+            <a href='http://test.localhost/uploads/default/original/1X/a1c31803be81b85ecafc4f77b1008eee9b3b82f4.mp4#&apos;%3C%3E'>
+              http://test.localhost/uploads/default/original/1X/a1c31803be81b85ecafc4f77b1008eee9b3b82f4.mp4#&apos;%3C%3E
+            </a>
+          </video>
+        </div>
+      HTML
+      expect(result[:preview]).to eq(<<~HTML)
+        <div class="onebox video-onebox">
+          <video  width="100%" height="100%" controls="">
+            <source src='http://test.localhost/uploads/default/original/1X/a1c31803be81b85ecafc4f77b1008eee9b3b82f4.mp4#&apos;%3C%3E'>
+            <a href='http://test.localhost/uploads/default/original/1X/a1c31803be81b85ecafc4f77b1008eee9b3b82f4.mp4#&apos;%3C%3E'>
+              http://test.localhost/uploads/default/original/1X/a1c31803be81b85ecafc4f77b1008eee9b3b82f4.mp4#&apos;%3C%3E
+            </a>
+          </video>
+        </div>
+      HTML
+    end
+
+    it "escapes URLs of generic local links" do
+      result = described_class.onebox_raw("#{Discourse.base_url}/g/somegroup#'onerror='")
+      expect(result[:onebox]).to eq(
+        "<a href='http://test.localhost/g/somegroup#&apos;onerror=&apos;'>http://test.localhost/g/somegroup#&apos;onerror=&apos;</a>",
+      )
+      expect(result[:preview]).to eq(
+        "<a href='http://test.localhost/g/somegroup#&apos;onerror=&apos;'>http://test.localhost/g/somegroup#&apos;onerror=&apos;</a>",
+      )
+    end
   end
 
   describe ".external_onebox" do