SECURITY: Add limits for themes and theme assets

This commit adds limits to themes and theme components on the:

- file size of about.json and .discourse-compatibility
- file size of theme assets
- number of files in a theme

app/models/remote_theme.rb

@@ -27,6 +27,11 @@ class RemoteTheme < ActiveRecord::Base
   GITHUB_REGEXP = %r{\Ahttps?://github\.com/}
   GITHUB_SSH_REGEXP = %r{\Assh://git@github\.com:}
 
+  MAX_METADATA_FILE_SIZE = Discourse::MAX_METADATA_FILE_SIZE
+  MAX_ASSET_FILE_SIZE = 8.megabytes
+  MAX_THEME_FILE_COUNT = 1024
+  MAX_THEME_SIZE = 256.megabytes
+
   has_one :theme, autosave: false
   scope :joined_remotes,
         -> {
@@ -41,12 +46,24 @@ class RemoteTheme < ActiveRecord::Base
                       allow_nil: true
 
   def self.extract_theme_info(importer)
+    if importer.file_size("about.json") > MAX_METADATA_FILE_SIZE
+      raise ImportError.new I18n.t(
+                              "themes.import_error.about_json_too_big",
+                              limit:
+                                ActiveSupport::NumberHelper.number_to_human_size(
+                                  MAX_METADATA_FILE_SIZE,
+                                ),
+                            )
+    end
+
+    begin
       json = JSON.parse(importer["about.json"])
       json.fetch("name")
       json
     rescue TypeError, JSON::ParserError, KeyError
       raise ImportError.new I18n.t("themes.import_error.about_json")
     end
+  end
 
   def self.update_zipped_theme(
     filename,
@@ -275,8 +292,43 @@ class RemoteTheme < ActiveRecord::Base
             )
     end
 
-    importer.all_files.each do |filename|
+    all_files = importer.all_files
+
+    if all_files.size > MAX_THEME_FILE_COUNT
+      raise ImportError,
+            I18n.t(
+              "themes.import_error.too_many_files",
+              count: all_files.size,
+              limit: MAX_THEME_FILE_COUNT,
+            )
+    end
+
+    theme_size = 0
+
+    all_files.each do |filename|
       next unless opts = ThemeField.opts_from_file_path(filename)
+
+      file_size = importer.file_size(filename)
+
+      if file_size > MAX_ASSET_FILE_SIZE
+        raise ImportError,
+              I18n.t(
+                "themes.import_error.asset_too_big",
+                filename: filename,
+                limit: ActiveSupport::NumberHelper.number_to_human_size(MAX_ASSET_FILE_SIZE),
+              )
+      end
+
+      theme_size += file_size
+
+      if theme_size > MAX_THEME_SIZE
+        raise ImportError,
+              I18n.t(
+                "themes.import_error.theme_too_big",
+                limit: ActiveSupport::NumberHelper.number_to_human_size(MAX_THEME_SIZE),
+              )
+      end
+
       value = importer[filename]
       updated_fields << theme.set_field(**opts.merge(value: value))
     end

config/locales/server.en.yml

@@ -78,8 +78,11 @@ en:
       generic: An error occurred while importing that theme
       upload: "Error creating upload asset: %{name}. %{errors}"
       about_json: "Import Error: about.json does not exist, or is invalid. Are you sure this is a Discourse Theme?"
+      about_json_too_big: "Import Error: about.json is bigger than the %{limit} limit."
       about_json_values: "about.json contains invalid values: %{errors}"
       modifier_values: "about.json modifiers contain invalid values: %{errors}"
+      asset_too_big: "Asset %{filename} is bigger than the %{limit} limit"
+      theme_too_big: "Theme is bigger than the %{limit} limit"
       git: "Error cloning git repository, access is denied or repository is not found"
       git_ref_not_found: "Unable to checkout git reference: %{ref}"
       git_unsupported_scheme: "Unable to clone git repo: scheme unsupported"
@@ -88,6 +91,7 @@ en:
       unknown_file_type: "The file you uploaded does not appear to be a valid Discourse theme."
       not_allowed_theme: "`%{repo}` is not in the list of allowed themes (check `allowed_theme_repos` global setting)."
       ssh_key_gone: "You waited too long to install the theme and SSH key expired. Please try again."
+      too_many_files: "The number of files (%{count}) in the theme has exceeded the maximum allowed number of files (%{limit})"
     errors:
       component_no_user_selectable: "Theme components can't be user-selectable"
       component_no_default: "Theme components can't be default theme"

lib/discourse.rb

@@ -8,6 +8,7 @@ require "version"
 module Discourse
   DB_POST_MIGRATE_PATH ||= "db/post_migrate"
   REQUESTED_HOSTNAME ||= "REQUESTED_HOSTNAME"
+  MAX_METADATA_FILE_SIZE = 64.kilobytes
 
   class Utils
     URI_REGEXP ||= URI.regexp(%w[http https])

lib/theme_store/base_importer.rb

@@ -26,6 +26,12 @@ module ThemeStore
       end
     end
 
+    def file_size(path)
+      fullpath = real_path(path)
+      return -1 unless fullpath
+      File.size(fullpath)
+    end
+
     def all_files
       Dir.glob("**/**", base: temp_folder).reject { |f| File.directory?(File.join(temp_folder, f)) }
     end

lib/theme_store/zip_importer.rb

@@ -6,19 +6,18 @@ class ThemeStore::ZipImporter < ThemeStore::BaseImporter
   attr_reader :url
 
   def initialize(filename, original_filename)
-    @temp_folder = "#{Pathname.new(Dir.tmpdir).realpath}/discourse_theme_#{SecureRandom.hex}"
     @filename = filename
     @original_filename = original_filename
   end
 
   def import!
-    FileUtils.mkdir(@temp_folder)
+    FileUtils.mkdir(temp_folder)
 
     available_size = SiteSetting.decompressed_theme_max_file_size_mb
     Compression::Engine
       .engine_for(@original_filename)
       .tap do |engine|
-        engine.decompress(@temp_folder, @filename, available_size)
+        engine.decompress(temp_folder, @filename, available_size)
         strip_root_directory
       end
   rescue RuntimeError
@@ -32,9 +31,9 @@ class ThemeStore::ZipImporter < ThemeStore::BaseImporter
   end
 
   def strip_root_directory
-    root_files = Dir.glob("#{@temp_folder}/*")
+    root_files = Dir.glob("#{temp_folder}/*")
     if root_files.size == 1 && File.directory?(root_files[0])
-      FileUtils.mv(Dir.glob("#{@temp_folder}/*/*"), @temp_folder)
+      FileUtils.mv(Dir.glob("#{temp_folder}/*/*"), temp_folder)
     end
   end
 end

lib/version.rb

@@ -92,12 +92,37 @@ module Discourse
   # Find a compatible resource from a git repo
   def self.find_compatible_git_resource(path)
     return unless File.directory?("#{path}/.git")
-    compat_resource, std_error, s =
+
-      Open3.capture3(
+    tree_info =
-        "git -C '#{path}' show HEAD@{upstream}:#{Discourse::VERSION_COMPATIBILITY_FILENAME}",
+      Discourse::Utils.execute_command(
+        "git",
+        "-C",
+        path,
+        "ls-tree",
+        "-l",
+        "HEAD",
+        Discourse::VERSION_COMPATIBILITY_FILENAME,
       )
-    Discourse.find_compatible_resource(compat_resource) if s.success?
+    blob_size = tree_info.split[3].to_i
+
+    if blob_size > Discourse::MAX_METADATA_FILE_SIZE
+      $stderr.puts "#{Discourse::VERSION_COMPATIBILITY_FILENAME} file in #{path} too big"
+      return
+    end
+
+    compat_resource =
+      Discourse::Utils.execute_command(
+        "git",
+        "-C",
+        path,
+        "show",
+        "HEAD@{upstream}:#{Discourse::VERSION_COMPATIBILITY_FILENAME}",
+      )
+
+    Discourse.find_compatible_resource(compat_resource)
   rescue InvalidVersionListError => e
     $stderr.puts "Invalid version list in #{path}"
+  rescue Discourse::Utils::CommandError => e
+    nil
   end
 end

spec/lib/version_spec.rb

@@ -177,5 +177,14 @@ RSpec.describe Discourse::VERSION do
 
       expect(output).to include("Invalid version list")
     end
+
+    it "gracefully handles large .discourse-compatibility files" do
+      stub_const(Discourse, "MAX_METADATA_FILE_SIZE", 1) do
+        output =
+          capture_stderr { expect(Discourse.find_compatible_git_resource(git_directory)).to be_nil }
+
+        expect(output).to include(Discourse::VERSION_COMPATIBILITY_FILENAME)
+      end
+    end
   end
 end

spec/models/remote_theme_spec.rb

@@ -195,6 +195,40 @@ RSpec.describe RemoteTheme do
       expect(remote.reload.remote_version).to eq(new_version)
       expect(remote.reload.commits_behind).to eq(-1)
     end
+
+    it "fails if theme has too many files" do
+      stub_const(RemoteTheme, "MAX_THEME_FILE_COUNT", 1) do
+        expect { RemoteTheme.import_theme(initial_repo_url) }.to raise_error(
+          RemoteTheme::ImportError,
+          I18n.t("themes.import_error.too_many_files", count: 14, limit: 1),
+        )
+      end
+    end
+
+    it "fails if files are too large" do
+      stub_const(RemoteTheme, "MAX_ASSET_FILE_SIZE", 1.byte) do
+        expect { RemoteTheme.import_theme(initial_repo_url) }.to raise_error(
+          RemoteTheme::ImportError,
+          I18n.t(
+            "themes.import_error.asset_too_big",
+            filename: "common/color_definitions.scss",
+            limit: ActiveSupport::NumberHelper.number_to_human_size(1),
+          ),
+        )
+      end
+    end
+
+    it "fails if theme is too large" do
+      stub_const(RemoteTheme, "MAX_THEME_SIZE", 1.byte) do
+        expect { RemoteTheme.import_theme(initial_repo_url) }.to raise_error(
+          RemoteTheme::ImportError,
+          I18n.t(
+            "themes.import_error.theme_too_big",
+            limit: ActiveSupport::NumberHelper.number_to_human_size(1),
+          ),
+        )
+      end
+    end
   end
 
   let(:github_repo) do
@@ -232,6 +266,48 @@ RSpec.describe RemoteTheme do
     end
   end
 
+  describe ".extract_theme_info" do
+    let(:importer) { mock }
+
+    let(:theme_info) do
+      {
+        "name" => "My Theme",
+        "about_url" => "https://example.com/about",
+        "license_url" => "https://example.com/license",
+      }
+    end
+
+    it "raises an error if about.json is too big" do
+      importer.stubs(:file_size).with("about.json").returns(100_000_000)
+
+      expect { RemoteTheme.extract_theme_info(importer) }.to raise_error(
+        RemoteTheme::ImportError,
+        I18n.t(
+          "themes.import_error.about_json_too_big",
+          limit:
+            ActiveSupport::NumberHelper.number_to_human_size((RemoteTheme::MAX_METADATA_FILE_SIZE)),
+        ),
+      )
+    end
+
+    it "raises an error if about.json is invalid" do
+      importer.stubs(:file_size).with("about.json").returns(123)
+      importer.stubs(:[]).with("about.json").returns("{")
+
+      expect { RemoteTheme.extract_theme_info(importer) }.to raise_error(
+        RemoteTheme::ImportError,
+        I18n.t("themes.import_error.about_json"),
+      )
+    end
+
+    it "returns extracted theme info" do
+      importer.stubs(:file_size).with("about.json").returns(123)
+      importer.stubs(:[]).with("about.json").returns(theme_info.to_json)
+
+      expect(RemoteTheme.extract_theme_info(importer)).to eq(theme_info)
+    end
+  end
+
   describe ".joined_remotes" do
     it "finds records that are associated with themes" do
       github_repo