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,11 +46,23 @@ class RemoteTheme < ActiveRecord::Base
                       allow_nil: true
 
   def self.extract_theme_info(importer)
-    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")
+    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(
@@ -254,8 +271,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.rb

@@ -0,0 +1,4 @@
+# frozen_string_literal: true
+
+module ThemeStore
+end

lib/theme_store/git_importer.rb

@@ -1,16 +1,14 @@
 # frozen_string_literal: true
 
-module ThemeStore
-end
-
-class ThemeStore::GitImporter
+class ThemeStore::GitImporter < ThemeStore::Importer
   COMMAND_TIMEOUT_SECONDS = 20
 
   attr_reader :url
 
   def initialize(url, private_key: nil, branch: nil)
+    super
+
     @url = GitUrl.normalize(url)
-    @temp_folder = "#{Pathname.new(Dir.tmpdir).realpath}/discourse_theme_#{SecureRandom.hex}"
     @private_key = private_key
     @branch = branch
   end
@@ -60,30 +58,6 @@ class ThemeStore::GitImporter
     FileUtils.rm_rf(@temp_folder)
   end
 
-  def real_path(relative)
-    fullpath = "#{@temp_folder}/#{relative}"
-    return nil unless File.exist?(fullpath)
-
-    # careful to handle symlinks here, don't want to expose random data
-    fullpath = Pathname.new(fullpath).realpath.to_s
-
-    if fullpath && fullpath.start_with?(@temp_folder)
-      fullpath
-    else
-      nil
-    end
-  end
-
-  def all_files
-    Dir.glob("**/*", base: @temp_folder).reject { |f| File.directory?(File.join(@temp_folder, f)) }
-  end
-
-  def [](value)
-    fullpath = real_path(value)
-    return nil unless fullpath
-    File.read(fullpath)
-  end
-
   protected
 
   def redirected_uri

lib/theme_store/importer.rb

@@ -0,0 +1,37 @@
+# frozen_string_literal: true
+
+class ThemeStore::Importer
+  def initialize(filename, original_filename)
+    @temp_folder = "#{Pathname.new(Dir.tmpdir).realpath}/discourse_theme_#{SecureRandom.hex}"
+  end
+
+  def all_files
+    Dir.glob("**/*", base: @temp_folder).reject { |f| File.directory?(File.join(@temp_folder, f)) }
+  end
+
+  def [](value)
+    fullpath = real_path(value)
+    return nil unless fullpath
+    File.read(fullpath)
+  end
+
+  def file_size(path)
+    fullpath = real_path(path)
+    return -1 unless fullpath
+    File.size(fullpath)
+  end
+
+  def real_path(relative)
+    fullpath = "#{@temp_folder}/#{relative}"
+    return nil unless File.exist?(fullpath)
+
+    # careful to handle symlinks here, don't want to expose random data
+    fullpath = Pathname.new(fullpath).realpath.to_s
+
+    if fullpath && fullpath.start_with?(@temp_folder)
+      fullpath
+    else
+      nil
+    end
+  end
+end

lib/theme_store/zip_importer.rb

@@ -2,14 +2,12 @@
 
 require "compression/engine"
 
-module ThemeStore
-end
-
-class ThemeStore::ZipImporter
+class ThemeStore::ZipImporter < ThemeStore::Importer
   attr_reader :url
 
   def initialize(filename, original_filename)
-    @temp_folder = "#{Pathname.new(Dir.tmpdir).realpath}/discourse_theme_#{SecureRandom.hex}"
+    super
+
     @filename = filename
     @original_filename = original_filename
   end
@@ -44,28 +42,4 @@ class ThemeStore::ZipImporter
       FileUtils.mv(Dir.glob("#{@temp_folder}/*/*"), @temp_folder)
     end
   end
-
-  def real_path(relative)
-    fullpath = "#{@temp_folder}/#{relative}"
-    return nil unless File.exist?(fullpath)
-
-    # careful to handle symlinks here, don't want to expose random data
-    fullpath = Pathname.new(fullpath).realpath.to_s
-
-    if fullpath && fullpath.start_with?(@temp_folder)
-      fullpath
-    else
-      nil
-    end
-  end
-
-  def all_files
-    Dir.glob("**/**", base: @temp_folder).reject { |f| File.directory?(File.join(@temp_folder, f)) }
-  end
-
-  def [](value)
-    fullpath = real_path(value)
-    return nil unless fullpath
-    File.read(fullpath)
-  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(
-        "git -C '#{path}' show HEAD@{upstream}:#{Discourse::VERSION_COMPATIBILITY_FILENAME}",
+
+    tree_info =
+      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