From 188cb58daa833839c54c266ce22db150a3f3a210 Mon Sep 17 00:00:00 2001
From: Natalie Tay <natalie.tay@discourse.org>
Date: Tue, 30 Jul 2024 14:19:01 +0800
Subject: [PATCH] SECURITY: Fixes for main (#28137)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* SECURITY: Update default allowed iframes list

Change the default iframe url list to all include 3 slashes.

* SECURITY: limit group tag's name length

Limit the size of a group tag's name to 100 characters.

Internal ref - t/130059

* SECURITY: Improve sanitization of SVGs in Onebox

---------

Co-authored-by: Blake Erickson <o.blakeerickson@gmail.com>
Co-authored-by: Régis Hanol <regis@hanol.fr>
Co-authored-by: David Taylor <david@taylorhq.com>
---
 .../discourse-markdown-it/src/options.js      |  6 ++--
 .../tests/unit/lib/pretty-text-test.js        | 12 +++++++
 app/models/tag_group.rb                       |  3 +-
 config/locales/client.en.yml                  |  2 +-
 config/site_settings.yml                      |  2 +-
 ...40610150449_limit_tag_group_name_length.rb | 13 ++++++++
 lib/onebox/sanitize_config.rb                 |  7 ++++
 spec/lib/onebox/preview_spec.rb               | 32 +++++++++++++++++++
 spec/lib/pretty_text_spec.rb                  |  6 ++--
 9 files changed, 75 insertions(+), 8 deletions(-)
 create mode 100644 db/migrate/20240610150449_limit_tag_group_name_length.rb

diff --git a/app/assets/javascripts/discourse-markdown-it/src/options.js b/app/assets/javascripts/discourse-markdown-it/src/options.js
index 9ab36231bab..c7899acba20 100644
--- a/app/assets/javascripts/discourse-markdown-it/src/options.js
+++ b/app/assets/javascripts/discourse-markdown-it/src/options.js
@@ -65,8 +65,10 @@ export default function buildOptions(state) {
       ? siteSettings.allowed_href_schemes.split("|")
       : null,
     allowedIframes: siteSettings.allowed_iframes
-      ? siteSettings.allowed_iframes.split("|")
-      : [],
+      ? siteSettings.allowed_iframes
+          .split("|")
+          .filter((str) => (str.match(/\//g) || []).length >= 3)
+      : [], // Only allow urls with at least 3 slashes. Ex: 'https://example.com/'.
     markdownIt: true,
     previewing,
     disableEmojis,
diff --git a/app/assets/javascripts/discourse/tests/unit/lib/pretty-text-test.js b/app/assets/javascripts/discourse/tests/unit/lib/pretty-text-test.js
index 5641ba0c244..3a414444bcf 100644
--- a/app/assets/javascripts/discourse/tests/unit/lib/pretty-text-test.js
+++ b/app/assets/javascripts/discourse/tests/unit/lib/pretty-text-test.js
@@ -70,6 +70,18 @@ module("Unit | Utility | pretty-text", function (hooks) {
         .features.emoji,
       "emoji disabled"
     );
+    assert.deepEqual(
+      build({ siteSettings: { allowed_iframes: "https://example.com/" } })
+        .options.discourse.allowedIframes,
+      ["https://example.com/"],
+      "it doesn't filter out valid urls"
+    );
+    assert.deepEqual(
+      build({ siteSettings: { allowed_iframes: "https://example.com" } })
+        .options.discourse.allowedIframes,
+      [],
+      "it filters out invalid urls. Requires 3 slashes."
+    );
   });
 
   test("basic cooking", function (assert) {
diff --git a/app/models/tag_group.rb b/app/models/tag_group.rb
index d5e7ee7323e..d8eab81be69 100644
--- a/app/models/tag_group.rb
+++ b/app/models/tag_group.rb
@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 class TagGroup < ActiveRecord::Base
+  validates :name, length: { maximum: 100 }
   validates_uniqueness_of :name, case_sensitive: false
 
   has_many :tag_group_memberships, dependent: :destroy
@@ -118,7 +119,7 @@ end
 # Table name: tag_groups
 #
 #  id            :integer          not null, primary key
-#  name          :string           not null
+#  name          :string(100)      not null
 #  created_at    :datetime         not null
 #  updated_at    :datetime         not null
 #  parent_tag_id :integer
diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml
index dd62a9a33dc..b42466d260f 100644
--- a/config/locales/client.en.yml
+++ b/config/locales/client.en.yml
@@ -4610,7 +4610,7 @@ en:
         everyone_can_use: "Tags can be used by everyone"
         usable_only_by_groups: "Tags are visible to everyone, but only the following groups can use them"
         visible_only_to_groups: "Tags are visible only to the following groups"
-        cannot_save: "Cannot save tag group. Make sure that there is at least one tag present, tag group name is not empty, and a group is selected for tags permission."
+        cannot_save: "Cannot save tag group. Make sure that there is at least one tag present, tag group name is not empty and less than 100 characters, and a group is selected for tags permission."
         tags_placeholder: "Search or create tags"
         parent_tag_placeholder: "Optional"
         select_groups_placeholder: "Select groups…"
diff --git a/config/site_settings.yml b/config/site_settings.yml
index 8bf8b6585f1..87f0b132936 100644
--- a/config/site_settings.yml
+++ b/config/site_settings.yml
@@ -2016,7 +2016,7 @@ security:
     allow_any: false
     choices: "['*'] + Onebox::Engine.all_iframe_origins"
   allowed_iframes:
-    default: "https://www.google.com/maps/embed?|https://www.openstreetmap.org/export/embed.html?|https://calendar.google.com/calendar/embed?|https://codepen.io/|https://www.instagram.com|https://open.spotify.com"
+    default: "https://www.google.com/maps/embed?|https://www.openstreetmap.org/export/embed.html?|https://calendar.google.com/calendar/embed?|https://codepen.io/|https://www.instagram.com/|https://open.spotify.com/"
     type: list
     list_type: simple
     client: true
diff --git a/db/migrate/20240610150449_limit_tag_group_name_length.rb b/db/migrate/20240610150449_limit_tag_group_name_length.rb
new file mode 100644
index 00000000000..5f54ec0a636
--- /dev/null
+++ b/db/migrate/20240610150449_limit_tag_group_name_length.rb
@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+
+class LimitTagGroupNameLength < ActiveRecord::Migration[7.0]
+  def change
+    DB.exec <<~SQL
+      UPDATE tag_groups 
+      SET name = LEFT(name, 100)
+      WHERE LENGTH(name) > 100
+    SQL
+
+    change_column :tag_groups, :name, :string, limit: 100
+  end
+end
diff --git a/lib/onebox/sanitize_config.rb b/lib/onebox/sanitize_config.rb
index 77f97ee9257..bec61963db1 100644
--- a/lib/onebox/sanitize_config.rb
+++ b/lib/onebox/sanitize_config.rb
@@ -79,6 +79,13 @@ module Onebox
                     iframe.remove_attribute("src")
                   end
                 end,
+                lambda do |env|
+                  next if env[:node_name] != "svg"
+                  env[:node].traverse do |node|
+                    next if node.element? && %w[svg path use].include?(node.name)
+                    node.remove
+                  end
+                end,
               ],
           protocols: {
             "embed" => {
diff --git a/spec/lib/onebox/preview_spec.rb b/spec/lib/onebox/preview_spec.rb
index f62c28515dd..7123b171e3e 100644
--- a/spec/lib/onebox/preview_spec.rb
+++ b/spec/lib/onebox/preview_spec.rb
@@ -118,4 +118,36 @@ RSpec.describe Onebox::Preview do
       expect(result).to include ' src="https://thirdparty.example.com"'
     end
   end
+
+  describe "svg sanitization" do
+    it "does not allow unexpected elements inside svg" do
+      preview = described_class.new(preview_url)
+      preview.stubs(:engine_html).returns <<~HTML.strip
+        <svg><style>/*Text*/</style></svg>
+      HTML
+
+      result = preview.to_s
+      expect(result).to eq("<svg></svg>")
+    end
+
+    it "does not allow text inside svg" do
+      preview = described_class.new(preview_url)
+      preview.stubs(:engine_html).returns <<~HTML.strip
+        <svg>Hello world</svg>
+      HTML
+
+      result = preview.to_s
+      expect(result).to eq("<svg></svg>")
+    end
+
+    it "allows simple svg" do
+      simple_svg =
+        '<svg height="210" width="400"><path d="M150 5 L75 200 L225 200 Z" style="fill:none;stroke:green;stroke-width:3"></path></svg>'
+      preview = described_class.new(preview_url)
+      preview.stubs(:engine_html).returns simple_svg
+
+      result = preview.to_s
+      expect(result).to eq(simple_svg)
+    end
+  end
 end
diff --git a/spec/lib/pretty_text_spec.rb b/spec/lib/pretty_text_spec.rb
index 2328ae47d4e..123c5c3bfe7 100644
--- a/spec/lib/pretty_text_spec.rb
+++ b/spec/lib/pretty_text_spec.rb
@@ -2529,17 +2529,17 @@ HTML
   end
 
   it "can properly allowlist iframes" do
-    SiteSetting.allowed_iframes = "https://bob.com/a|http://silly.com?EMBED="
+    SiteSetting.allowed_iframes = "https://bob.com/a|http://silly.com/?EMBED="
     raw = <<~HTML
       <iframe src='https://www.google.com/maps/Embed?testing'></iframe>
       <iframe src='https://bob.com/a?testing'></iframe>
-      <iframe src='HTTP://SILLY.COM?EMBED=111'></iframe>
+      <iframe src='HTTP://SILLY.COM/?EMBED=111'></iframe>
     HTML
 
     # we require explicit HTTPS here
     html = <<~HTML
       <iframe src="https://bob.com/a?testing"></iframe>
-      <iframe src="HTTP://SILLY.COM?EMBED=111"></iframe>
+      <iframe src="HTTP://SILLY.COM/?EMBED=111"></iframe>
     HTML
 
     cooked = PrettyText.cook(raw).strip