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.
This commit is contained in:
Sam Saffron 2020-06-03 13:19:42 +10:00
parent 062db10c52
commit 57a3d4e0d2
No known key found for this signature in database
GPG Key ID: B9606168D2FFD9F5
8 changed files with 186 additions and 6 deletions

View File

@ -14,6 +14,9 @@ class Admin::ThemesController < Admin::AdminController
end end
def upload_asset def upload_asset
ban_in_whitelist_mode!
path = params[:file].path path = params[:file].path
hijack do hijack do
@ -49,6 +52,9 @@ class Admin::ThemesController < Admin::AdminController
def import def import
@theme = nil @theme = nil
if params[:theme] && params[:theme].content_type == "application/json" if params[:theme] && params[:theme].content_type == "application/json"
ban_in_whitelist_mode!
# .dcstyle.json import. Deprecated, but still available to allow conversion # .dcstyle.json import. Deprecated, but still available to allow conversion
json = JSON::parse(params[:theme].read) json = JSON::parse(params[:theme].read)
theme = json['theme'] theme = json['theme']
@ -85,15 +91,21 @@ class Admin::ThemesController < Admin::AdminController
else else
render json: @theme.errors, status: :unprocessable_entity render json: @theme.errors, status: :unprocessable_entity
end end
elsif params[:remote] elsif remote = params[:remote]
guardian.ensure_allowed_theme_repo_import!(remote.strip)
begin begin
branch = params[:branch] ? params[:branch] : nil 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 render json: @theme, status: :created
rescue RemoteTheme::ImportError => e rescue RemoteTheme::ImportError => e
render_json_error e.message render_json_error e.message
end end
elsif params[:bundle] || (params[:theme] && THEME_CONTENT_TYPES.include?(params[:theme].content_type)) 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 # params[:bundle] used by theme CLI. params[:theme] used by admin UI
bundle = params[:bundle] || params[:theme] bundle = params[:bundle] || params[:theme]
theme_id = params[:theme_id] theme_id = params[:theme_id]
@ -139,6 +151,9 @@ class Admin::ThemesController < Admin::AdminController
end end
def create def create
ban_in_whitelist_mode!
@theme = Theme.new(name: theme_params[:name], @theme = Theme.new(name: theme_params[:name],
user_id: theme_user.id, user_id: theme_user.id,
user_selectable: theme_params[:user_selectable] || false, user_selectable: theme_params[:user_selectable] || false,
@ -282,6 +297,10 @@ class Admin::ThemesController < Admin::AdminController
private private
def ban_in_whitelist_mode!
raise Discourse::InvalidAccess if !GlobalSetting.whitelisted_theme_ids.nil?
end
def add_relative_themes!(kind, ids) def add_relative_themes!(kind, ids)
expected = ids.map(&:to_i) expected = ids.map(&:to_i)
@ -339,6 +358,8 @@ class Admin::ThemesController < Admin::AdminController
def set_fields def set_fields
return unless fields = theme_params[:theme_fields] return unless fields = theme_params[:theme_fields]
ban_in_whitelist_mode!
fields.each do |field| fields.each do |field|
@theme.set_field( @theme.set_field(
target: field[:target], target: field[:target],

View File

@ -414,8 +414,10 @@ class ApplicationController < ActionController::Base
end end
if theme_ids.blank? && SiteSetting.default_theme_id != -1 if theme_ids.blank? && SiteSetting.default_theme_id != -1
if guardian.allow_themes?([SiteSetting.default_theme_id])
theme_ids << SiteSetting.default_theme_id theme_ids << SiteSetting.default_theme_id
end end
end
@theme_ids = request.env[:resolved_theme_ids] = theme_ids @theme_ids = request.env[:resolved_theme_ids] = theme_ids
end end

View File

@ -220,6 +220,23 @@ class GlobalSetting
end end
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) def self.add_default(name, default)
unless self.respond_to? name unless self.respond_to? name
define_singleton_method(name) do define_singleton_method(name) do

View File

@ -285,3 +285,12 @@ compress_anon_cache = false
# This ensures there are no pathological cases where we keep storing data in anonymous cache # 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 # never to use it, set to 1 to store immediately, set to 0 to disable anon cache
anon_cache_store_threshold = 2 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 =

View File

@ -471,9 +471,27 @@ class Guardian
@user.staff? || @user.trust_level >= TrustLevel.levels[:member] @user.staff? || @user.trust_level >= TrustLevel.levels[:member]
end 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) def allow_themes?(theme_ids, include_preview: false)
return true if theme_ids.blank? 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? if include_preview && is_staff? && (theme_ids - Theme.theme_ids).blank?
return true return true
end end

View File

@ -50,3 +50,27 @@ task "themes:install" => :environment do |task, args|
exit 1 exit 1
end end
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

View File

@ -2869,6 +2869,32 @@ describe Guardian do
let!(:theme) { Fabricate(:theme) } let!(:theme) { Fabricate(:theme) }
let!(:theme2) { 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 it "allows staff to use any themes" do
expect(Guardian.new(moderator).allow_themes?([theme.id, theme2.id])).to eq(false) 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) expect(Guardian.new(admin).allow_themes?([theme.id, theme2.id])).to eq(false)

View File

@ -87,6 +87,7 @@ describe Admin::ThemesController do
end end
describe '#import' do describe '#import' do
let(:theme_json_file) do let(:theme_json_file) do
Rack::Test::UploadedFile.new(file_from_fixtures("sam-s-simple-theme.dcstyle.json", "json"), "application/json") Rack::Test::UploadedFile.new(file_from_fixtures("sam-s-simple-theme.dcstyle.json", "json"), "application/json")
end end
@ -99,6 +100,40 @@ describe Admin::ThemesController do
file_from_fixtures("logo.png") file_from_fixtures("logo.png")
end 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 it 'can import a theme from Git' do
RemoteTheme.stubs(:import_theme) RemoteTheme.stubs(:import_theme)
post "/admin/themes/import.json", params: { post "/admin/themes/import.json", params: {
@ -120,7 +155,7 @@ describe Admin::ThemesController do
end end
it 'imports a theme from an archive' do it 'imports a theme from an archive' do
existing_theme = Fabricate(:theme, name: "Header Icons") _existing_theme = Fabricate(:theme, name: "Header Icons")
expect do expect do
post "/admin/themes/import.json", params: { theme: theme_archive } 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 it 'updates an existing theme from an archive by name' do
# Old theme CLI method, remove Jan 2020 # Old theme CLI method, remove Jan 2020
existing_theme = Fabricate(:theme, name: "Header Icons") _existing_theme = Fabricate(:theme, name: "Header Icons")
expect do expect do
post "/admin/themes/import.json", params: { bundle: theme_archive } 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 it 'updates an existing theme from an archive by id' do
# Used by theme CLI # 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") other_existing_theme = Fabricate(:theme, name: "Some other name")
messages = MessageBus.track_publish do messages = MessageBus.track_publish do
@ -273,6 +308,34 @@ describe Admin::ThemesController do
expect(SiteSetting.default_theme_id).to eq(-1) expect(SiteSetting.default_theme_id).to eq(-1)
end 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 it 'updates a theme' do
theme.set_field(target: :common, name: :scss, value: '.body{color: black;}') theme.set_field(target: :common, name: :scss, value: '.body{color: black;}')
theme.save theme.save