From 76f06f6b1491db6bd09a4017d2c5591431b3b16e Mon Sep 17 00:00:00 2001 From: Natalie Tay Date: Tue, 30 Jul 2024 14:19:08 +0800 Subject: [PATCH] SECURITY: Fixes for stable (#28138) 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 (stable) --------- Co-authored-by: Blake Erickson Co-authored-by: Régis Hanol Co-authored-by: David Taylor --- .../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 272c72da816..6d1816c3e07 100644 --- a/app/assets/javascripts/discourse-markdown-it/src/options.js +++ b/app/assets/javascripts/discourse-markdown-it/src/options.js @@ -63,8 +63,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 694038f1155..a2e76ac3275 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 d00fb9eb055..8494e5e6e1e 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 @@ -116,7 +117,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 8012d4f198d..f68b475b6e6 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -4421,7 +4421,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 6ba00f1b13e..795fa9970f7 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -1960,7 +1960,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" + 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/" 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 + + HTML + + result = preview.to_s + expect(result).to eq("") + end + + it "does not allow text inside svg" do + preview = described_class.new(preview_url) + preview.stubs(:engine_html).returns <<~HTML.strip + Hello world + HTML + + result = preview.to_s + expect(result).to eq("") + end + + it "allows simple svg" do + simple_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 1583ef5fecf..92f67bed731 100644 --- a/spec/lib/pretty_text_spec.rb +++ b/spec/lib/pretty_text_spec.rb @@ -2426,17 +2426,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 - + HTML # we require explicit HTTPS here html = <<~HTML - + HTML cooked = PrettyText.cook(raw).strip