SECURITY: Fixes for main (#28137)

* 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>

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,

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) {

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

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…"

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

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

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" => {

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

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