CVE-2023-25172: Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting') (CWE-79)
value: Discourse is an open-source discussion platform. Prior to version 3.0.1 of the `stable` branch and version 3.1.0.beta2 of the `beta` and `tests-passed` branches, a maliciously crafted URL can be included in a user's full name field to to carry out cross-site scripting attacks on sites with a disabled or overly permissive CSP (Content Security Policy). Discourse's default CSP prevents this vulnerability. The vulnerability is patched in version 3.0.1 of the `stable` branch and version 3.1.0.beta2 of the `beta` and `tests-passed` branches. As a workaround, enable and/or restore your site's CSP to the default one provided with Discourse.
CWE description: The product does not neutralize or incorrectly neutralizes user-controllable input before it is placed in output that is used as a web page that is served to other users.
CVSS v3 base score: 5.4
Published: 2023-03-17T17:15Z
Repository: https://github.com/discourse/discourse
Language: Ruby
Files changed: db/post_migrate/20230105153520_trigger_post_rebake_local_onebox_xss.rb, lib/oneboxer.rb, spec/lib/oneboxer_spec.rb
Commit date: 2023-01-25 19:17:21 +0200
Commit message: SECURITY: Prevent XSS in local oneboxes (#20008)

Co-authored-by: OsamaSayegh <asooomaasoooma90@gmail.com>
Diff: 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 000000000000..e9981e4d2c09
--- /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 6d6bd34f14f3..eb36ebad21e8 100644
--- a/lib/oneboxer.rb
+++ b/lib/oneboxer.rb
@@ -341,11 +341,12 @@ module Oneboxer
         if handler = local_handlers[route[:controller]]
           handler.call(url, route)
         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
 
   def self.local_upload_html(url)
     additional_controls =
@@ -353,22 +354,32 @@ module Oneboxer
         "controlslist='nodownload'"
       else
         ""
       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
 
   def self.local_topic(url, route, opts)
     if current_user = User.find_by(id: opts[:user_id])
diff --git a/spec/lib/oneboxer_spec.rb b/spec/lib/oneboxer_spec.rb
index ae969d015ed8..7e23517425ad 100644
--- a/spec/lib/oneboxer_spec.rb
+++ b/spec/lib/oneboxer_spec.rb
@@ -196,10 +196,70 @@ RSpec.describe Oneboxer do
       cpp.post_process_oneboxes
       expect(cpp.html).to eq(
         "<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
     html = <<~HTML
       <html>

Vulnerable code:     html = html.presence || "<a href='#{URI(url).to_s}'>#{URI(url).to_s}</a>"
            <source src='#{url}'>
            <a href='#{url}'>#{url}</a>
      "<audio #{additional_controls} controls><source src='#{url}'><a href='#{url}'>#{url}</a></audio>"
Fixed code: # 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
    normalized_url = ::Onebox::Helpers.normalize_url_for_output(URI(url).to_s)
    html = html.presence || "<a href='#{normalized_url}'>#{normalized_url}</a>"
    normalized_url = ::Onebox::Helpers.normalize_url_for_output(url)
            <source src='#{normalized_url}'>
            <a href='#{normalized_url}'>
              #{normalized_url}
            </a>
      <<~HTML
        <audio #{additional_controls} controls>
          <source src='#{normalized_url}'>
          <a href='#{normalized_url}'>
            #{normalized_url}
          </a>
        </audio>
      HTML

    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