From 57a3d4e0d2c87e4552cb7de221de30d2b312c2c7 Mon Sep 17 00:00:00 2001 From: Sam Saffron Date: Wed, 3 Jun 2020 13:19:42 +1000 Subject: [PATCH] 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. --- app/controllers/admin/themes_controller.rb | 25 ++++++- app/controllers/application_controller.rb | 4 +- app/models/global_setting.rb | 17 +++++ config/discourse_defaults.conf | 9 +++ lib/guardian.rb | 18 +++++ lib/tasks/themes.rake | 24 +++++++ spec/components/guardian_spec.rb | 26 +++++++ spec/requests/admin/themes_controller_spec.rb | 69 ++++++++++++++++++- 8 files changed, 186 insertions(+), 6 deletions(-) diff --git a/app/controllers/admin/themes_controller.rb b/app/controllers/admin/themes_controller.rb index d51b2439791..a08bc95495b 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 e7f8c9750b0..2ce9f875ffd 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 cd80df66818..54dc310e618 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 39df104204d..b2b2aff03ed 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 456461fead6..da6d1bf2b79 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 8a90196faf9..8a8cde8f68d 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 97a1b0c2e06..53b1b14c1c3 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 + it "allows staff to use any themes" do expect(Guardian.new(moderator).allow_themes?([theme.id, theme2.id])).to eq(false) expect(Guardian.new(admin).allow_themes?([theme.id, theme2.id])).to eq(false) diff --git a/spec/requests/admin/themes_controller_spec.rb b/spec/requests/admin/themes_controller_spec.rb index 39e24e89401..a7d42ce4f9b 100644 --- a/spec/requests/admin/themes_controller_spec.rb +++ b/spec/requests/admin/themes_controller_spec.rb @@ -87,6 +87,7 @@ describe Admin::ThemesController do end describe '#import' do + let(:theme_json_file) do Rack::Test::UploadedFile.new(file_from_fixtures("sam-s-simple-theme.dcstyle.json", "json"), "application/json") end @@ -99,6 +100,40 @@ describe Admin::ThemesController do file_from_fixtures("logo.png") end + context 'when theme whitelist mode is enabled' do + before do + GlobalSetting.reset_whitelisted_theme_ids! + global_setting :whitelisted_theme_repos, "https://github.com/discourse/discourse-brand-header" + end + + after do + GlobalSetting.reset_whitelisted_theme_ids! + end + + it "allows whitelisted imports" do + RemoteTheme.stubs(:import_theme) + post "/admin/themes/import.json", params: { + remote: ' https://github.com/discourse/discourse-brand-header ' + } + + expect(response.status).to eq(201) + end + + it "bans non whtielisted imports" do + RemoteTheme.stubs(:import_theme) + post "/admin/themes/import.json", params: { + remote: ' https://bad.com/discourse/discourse-brand-header ' + } + + expect(response.status).to eq(403) + end + + it "bans json file import" do + post "/admin/themes/import.json", params: { theme: theme_json_file } + expect(response.status).to eq(403) + end + end + it 'can import a theme from Git' do RemoteTheme.stubs(:import_theme) post "/admin/themes/import.json", params: { @@ -120,7 +155,7 @@ describe Admin::ThemesController do end it 'imports a theme from an archive' do - existing_theme = Fabricate(:theme, name: "Header Icons") + _existing_theme = Fabricate(:theme, name: "Header Icons") expect do post "/admin/themes/import.json", params: { theme: theme_archive } @@ -135,7 +170,7 @@ describe Admin::ThemesController do it 'updates an existing theme from an archive by name' do # Old theme CLI method, remove Jan 2020 - existing_theme = Fabricate(:theme, name: "Header Icons") + _existing_theme = Fabricate(:theme, name: "Header Icons") expect do post "/admin/themes/import.json", params: { bundle: theme_archive } @@ -150,7 +185,7 @@ describe Admin::ThemesController do it 'updates an existing theme from an archive by id' do # Used by theme CLI - existing_theme = Fabricate(:theme, name: "Header Icons") + _existing_theme = Fabricate(:theme, name: "Header Icons") other_existing_theme = Fabricate(:theme, name: "Some other name") messages = MessageBus.track_publish do @@ -273,6 +308,34 @@ describe Admin::ThemesController do expect(SiteSetting.default_theme_id).to eq(-1) end + context 'when theme whitelist mode is enabled' 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 'unconditionally bans theme_fields from updating' do + r = RemoteTheme.create!(remote_url: "https://magic.com/repo.git") + theme.update!(remote_theme_id: r.id) + + put "/admin/themes/#{theme.id}.json", params: { + theme: { + name: 'my test name', + theme_fields: [ + { name: 'scss', target: 'common', value: '' }, + { name: 'scss', target: 'desktop', value: 'body{color: blue;}' }, + ] + } + } + + expect(response.status).to eq(403) + end + end + it 'updates a theme' do theme.set_field(target: :common, name: :scss, value: '.body{color: black;}') theme.save