FEATURE: whitelist theme repo mode (experimental)

FEATURE: whitelist theme repo mode (experimental)

In some restricted setups all JS payloads need tight control.

This setting bans admins from making changes to JS on the site and requires all themes be whitelisted to be used.

There are edge cases we still need to work through in this mode hence this is still not supported in production and experimental.

Use an example like this to enable:

DISCOURSE_WHITELISTED_THEME_REPOS="https://repo.com/repo.git,https://repo.com/repo2.git"

By default this feature is not enabled and no changes are made.

One exception is that default theme id was missing a security check this was added for correctness.

diff --git a/app/controllers/admin/themes_controller.rb b/app/controllers/admin/themes_controller.rb
index d51b243..a08bc95 100644
--- a/app/controllers/admin/themes_controller.rb
+++ b/app/controllers/admin/themes_controller.rb
@@ -14,6 +14,9 @@ class Admin::ThemesController < Admin::AdminController
   end
 
   def upload_asset
+
+    ban_in_whitelist_mode!
+
     path = params[:file].path
 
     hijack do
@@ -49,6 +52,9 @@ class Admin::ThemesController < Admin::AdminController
   def import
     @theme = nil
     if params[:theme] && params[:theme].content_type == "application/json"
+
+      ban_in_whitelist_mode!
+
       # .dcstyle.json import. Deprecated, but still available to allow conversion
       json = JSON::parse(params[:theme].read)
       theme = json['theme']
@@ -85,15 +91,21 @@ class Admin::ThemesController < Admin::AdminController
       else
         render json: @theme.errors, status: :unprocessable_entity
       end
-    elsif params[:remote]
+    elsif remote = params[:remote]
+
+      guardian.ensure_allowed_theme_repo_import!(remote.strip)
+
       begin
         branch = params[:branch] ? params[:branch] : nil
-        @theme = RemoteTheme.import_theme(params[:remote], theme_user, private_key: params[:private_key], branch: branch)
+        @theme = RemoteTheme.import_theme(remote, theme_user, private_key: params[:private_key], branch: branch)
         render json: @theme, status: :created
       rescue RemoteTheme::ImportError => e
         render_json_error e.message
       end
     elsif params[:bundle] || (params[:theme] && THEME_CONTENT_TYPES.include?(params[:theme].content_type))
+
+      ban_in_whitelist_mode!
+
       # params[:bundle] used by theme CLI. params[:theme] used by admin UI
       bundle = params[:bundle] || params[:theme]
       theme_id = params[:theme_id]
@@ -139,6 +151,9 @@ class Admin::ThemesController < Admin::AdminController
   end
 
   def create
+
+    ban_in_whitelist_mode!
+
     @theme = Theme.new(name: theme_params[:name],
                        user_id: theme_user.id,
                        user_selectable: theme_params[:user_selectable] || false,
@@ -282,6 +297,10 @@ class Admin::ThemesController < Admin::AdminController
 
   private
 
+  def ban_in_whitelist_mode!
+    raise Discourse::InvalidAccess if !GlobalSetting.whitelisted_theme_ids.nil?
+  end
+
   def add_relative_themes!(kind, ids)
     expected = ids.map(&:to_i)
 
@@ -339,6 +358,8 @@ class Admin::ThemesController < Admin::AdminController
   def set_fields
     return unless fields = theme_params[:theme_fields]
 
+    ban_in_whitelist_mode!
+
     fields.each do |field|
       @theme.set_field(
         target: field[:target],
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index e7f8c97..2ce9f87 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -414,7 +414,9 @@ class ApplicationController < ActionController::Base
     end
 
     if theme_ids.blank? && SiteSetting.default_theme_id != -1
-      theme_ids << SiteSetting.default_theme_id
+      if guardian.allow_themes?([SiteSetting.default_theme_id])
+        theme_ids << SiteSetting.default_theme_id
+      end
     end
 
     @theme_ids = request.env[:resolved_theme_ids] = theme_ids
diff --git a/app/models/global_setting.rb b/app/models/global_setting.rb
index cd80df6..54dc310 100644
--- a/app/models/global_setting.rb
+++ b/app/models/global_setting.rb
@@ -220,6 +220,23 @@ class GlobalSetting
       end
   end
 
+  # test only
+  def self.reset_whitelisted_theme_ids!
+    @whitelisted_theme_ids = nil
+  end
+
+  def self.whitelisted_theme_ids
+    return nil if whitelisted_theme_repos.blank?
+
+    @whitelisted_theme_ids ||= begin
+      urls = whitelisted_theme_repos.split(",").map(&:strip)
+      Theme
+        .joins(:remote_theme)
+        .where('remote_themes.remote_url in (?)', urls)
+        .pluck(:id)
+    end
+  end
+
   def self.add_default(name, default)
     unless self.respond_to? name
       define_singleton_method(name) do
diff --git a/config/discourse_defaults.conf b/config/discourse_defaults.conf
index 39df104..b2b2aff 100644
--- a/config/discourse_defaults.conf
+++ b/config/discourse_defaults.conf
@@ -285,3 +285,12 @@ compress_anon_cache = false
 # This ensures there are no pathological cases where we keep storing data in anonymous cache
 # never to use it, set to 1 to store immediately, set to 0 to disable anon cache
 anon_cache_store_threshold = 2
+
+# EXPERIMENTAL - not yet supported in production
+# by default admins can install and amend any theme
+# you may restrict it so only specific themes are approved
+# in whitelist mode all theme updates must happen via git repos
+# themes missing from the list are automatically disallowed
+# list is a comma seperated list of git repos eg:
+# https://github.com/discourse/discourse-custom-header-links.git,https://github.com/discourse/discourse-simple-theme.git
+whitelisted_theme_repos =
diff --git a/lib/guardian.rb b/lib/guardian.rb
index 456461f..da6d1bf 100644
--- a/lib/guardian.rb
+++ b/lib/guardian.rb
@@ -471,9 +471,27 @@ class Guardian
     @user.staff? || @user.trust_level >= TrustLevel.levels[:member]
   end
 
+  def allowed_theme_repo_import?(repo)
+    return false if !@user.admin?
+
+    whitelisted_repos = GlobalSetting.whitelisted_theme_repos
+    if !whitelisted_repos.blank?
+      urls = whitelisted_repos.split(",").map(&:strip)
+      return urls.include?(repo)
+    end
+
+    true
+  end
+
   def allow_themes?(theme_ids, include_preview: false)
     return true if theme_ids.blank?
 
+    if whitelisted_theme_ids = GlobalSetting.whitelisted_theme_ids
+      if (theme_ids - whitelisted_theme_ids).present?
+        return false
+      end
+    end
+
     if include_preview && is_staff? && (theme_ids - Theme.theme_ids).blank?
       return true
     end
diff --git a/lib/tasks/themes.rake b/lib/tasks/themes.rake
index 8a90196..8a8cde8 100644
--- a/lib/tasks/themes.rake
+++ b/lib/tasks/themes.rake
@@ -50,3 +50,27 @@ task "themes:install" => :environment do |task, args|
     exit 1
   end
 end
+
+desc "List all the installed themes on the site"
+task "themes:audit" => :environment do
+  components = Set.new
+  puts "Selectable themes"
+  puts "-----------------"
+
+  Theme.where("(enabled OR user_selectable) AND NOT component").each do |theme|
+    puts theme.remote_theme&.remote_url || theme.name
+    theme.child_themes.each do |child|
+      if child.enabled
+        repo = child.remote_theme&.remote_url || child.name
+        components << repo
+      end
+    end
+  end
+
+  puts
+  puts "Selectable components"
+  puts "---------------------"
+  components.each do |repo|
+    puts repo
+  end
+end
diff --git a/spec/components/guardian_spec.rb b/spec/components/guardian_spec.rb
index 97a1b0c..53b1b14 100644
--- a/spec/components/guardian_spec.rb
+++ b/spec/components/guardian_spec.rb
@@ -2869,6 +2869,32 @@ describe Guardian do
     let!(:theme) { Fabricate(:theme) }
     let!(:theme2) { Fabricate(:theme) }
 
+    context "whitelist mode" do
+      before do
+        GlobalSetting.reset_whitelisted_theme_ids!
+        global_setting :whitelisted_theme_repos, "  https://magic.com/repo.git, https://x.com/git"
+      end
+
+      after do
+        GlobalSetting.reset_whitelisted_theme_ids!
+      end
+
+      it "should respect theme whitelisting" do
+        r = RemoteTheme.create!(remote_url: "https://magic.com/repo.git")
+        theme.update!(remote_theme_id: r.id)
+
+        guardian = Guardian.new(admin)
+
+        expect(guardian.allow_themes?([theme.id, theme2.id], include_preview: true)).to eq(false)
+
+        expect(guardian.allow_themes?([theme.id], include_preview: true)).to eq(true)
+
+        expect(guardian.allowed_theme_repo_import?('https://x.com/git')).to eq(true)
+        expect(guardian.allowed_theme_repo_import?('https:/evil.com/git')).to eq(false)
+
+      end
+    end
+

[... diff too long, it was truncated ...]

GitHub sha: 57a3d4e0

2 Likes