From cedcdb0057f0f65bba7b3fb019ac40b87e6b4eb5 Mon Sep 17 00:00:00 2001 From: Sam Date: Thu, 7 Apr 2022 07:58:10 +1000 Subject: [PATCH] FEATURE: allow for local theme js assets (#16374) Due to default CSP web workers instantiated from CDN based assets are still treated as "same-origin" meaning that we had no way of safely instansiating a web worker from a theme. This limits the theme system and adds the arbitrary restriction that WASM based components can not be safely used. To resolve this limitation all js assets in about.json are also cached on local domain. { "name": "Header Icons", "assets" : { "worker" : "assets/worker.js" } } This can then be referenced in JS via: settings.theme_uploads_local.worker local_js_assets are unconditionally served from the site directly and bypass the entire CDN, using the pre-existing JavascriptCache Previous to this change this code was completely dormant on sites which used s3 based uploads, this reuses the very well tested and cached asset system on s3 based sites. Note, when creating local_js_assets it is highly recommended to keep the assets lean and keep all the heavy working in CDN based assets. For example wasm files can still live on the CDN but the lean worker that loads it can live on local. This change unlocks wasm in theme components, so wasm is now also allowed in `theme_authorized_extensions` * more usages of upload.content * add a specific test for upload.content * Adjust logic to ensure that after upgrades we still get a cached local js on save --- app/models/javascript_cache.rb | 10 ++- app/models/theme.rb | 9 ++- app/models/theme_field.rb | 8 ++- app/models/upload.rb | 16 +++++ config/site_settings.yml | 2 +- lib/svg_sprite/svg_sprite.rb | 40 ++++++------ lib/theme_store/git_importer.rb | 2 +- lib/theme_store/zip_exporter.rb | 3 +- spec/lib/svg_sprite/svg_sprite_spec.rb | 2 +- spec/models/theme_field_spec.rb | 88 +++++++++++++++++++++++++- spec/models/upload_spec.rb | 7 ++ 11 files changed, 159 insertions(+), 28 deletions(-) diff --git a/app/models/javascript_cache.rb b/app/models/javascript_cache.rb index 20f8b63a846..b92509bb892 100644 --- a/app/models/javascript_cache.rb +++ b/app/models/javascript_cache.rb @@ -8,11 +8,19 @@ class JavascriptCache < ActiveRecord::Base before_save :update_digest def url - "#{GlobalSetting.cdn_url}#{Discourse.base_path}/theme-javascripts/#{digest}.js?__ws=#{Discourse.current_hostname}" + "#{GlobalSetting.cdn_url}#{Discourse.base_path}#{path}" + end + + def local_url + "#{Discourse.base_url}#{path}" end private + def path + "/theme-javascripts/#{digest}.js?__ws=#{Discourse.current_hostname}" + end + def update_digest self.digest = Digest::SHA1.hexdigest(content) if content_changed? end diff --git a/app/models/theme.rb b/app/models/theme.rb index b2d867509c9..990b39c1e85 100644 --- a/app/models/theme.rb +++ b/app/models/theme.rb @@ -539,12 +539,19 @@ class Theme < ActiveRecord::Base end theme_uploads = {} + theme_uploads_local = {} + upload_fields.each do |field| if field.upload&.url theme_uploads[field.name] = Discourse.store.cdn_url(field.upload.url) end + if field.javascript_cache + theme_uploads_local[field.name] = field.javascript_cache.local_url + end end + hash['theme_uploads'] = theme_uploads if theme_uploads.present? + hash['theme_uploads_local'] = theme_uploads_local if theme_uploads_local.present? hash end @@ -652,7 +659,7 @@ class Theme < ActiveRecord::Base end settings_hash&.each do |name, value| - next if name == "theme_uploads" + next if name == "theme_uploads" || name == "theme_uploads_local" contents << to_scss_variable(name, value) end diff --git a/app/models/theme_field.rb b/app/models/theme_field.rb index 22c03f61728..0e3c8663f3a 100644 --- a/app/models/theme_field.rb +++ b/app/models/theme_field.rb @@ -184,7 +184,7 @@ class ThemeField < ActiveRecord::Base begin content = File.read(path) - svg_file = Nokogiri::XML(content) do |config| + Nokogiri::XML(content) do |config| config.options = Nokogiri::XML::ParseOptions::NOBLANKS end rescue => e @@ -568,6 +568,12 @@ class ThemeField < ActiveRecord::Base if (will_save_change_to_value? || will_save_change_to_upload_id?) && !will_save_change_to_value_baked? self.value_baked = nil end + if upload && upload.extension == "js" + if will_save_change_to_upload_id? || !javascript_cache + javascript_cache ||= build_javascript_cache + javascript_cache.content = upload.content + end + end end after_save do diff --git a/app/models/upload.rb b/app/models/upload.rb index 061cc266af3..b24044b7a68 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -171,6 +171,22 @@ class Upload < ActiveRecord::Base end end + def content + original_path = Discourse.store.path_for(self) + external_copy = nil + + if original_path.blank? + external_copy = Discourse.store.download(self) + original_path = external_copy.path + end + + File.read(original_path) + ensure + if external_copy + File.unlink(external_copy.path) + end + end + def fix_image_extension return false if extension == "unknown" diff --git a/config/site_settings.yml b/config/site_settings.yml index 9ebfb5efa25..78ab66041c1 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -1274,7 +1274,7 @@ files: default: 50000 max: 1024000 theme_authorized_extensions: - default: "jpg|jpeg|png|woff|woff2|svg|eot|ttf|otf|gif|webp|js" + default: "wasm|jpg|jpeg|png|woff|woff2|svg|eot|ttf|otf|gif|webp|js" type: list list_type: compact authorized_extensions: diff --git a/lib/svg_sprite/svg_sprite.rb b/lib/svg_sprite/svg_sprite.rb index 634937f0cc1..d7b5ebb0bcd 100644 --- a/lib/svg_sprite/svg_sprite.rb +++ b/lib/svg_sprite/svg_sprite.rb @@ -237,24 +237,7 @@ module SvgSprite custom_sprite_paths = Dir.glob(plugin_paths) - if theme_id.present? - ThemeField.where(type_id: ThemeField.types[:theme_upload_var], name: THEME_SPRITE_VAR_NAME, theme_id: Theme.transform_ids(theme_id)) - .pluck(:upload_id).each do |upload_id| - - upload = Upload.find(upload_id) rescue nil - - if Discourse.store.external? - external_copy = Discourse.store.download(upload) rescue nil - original_path = external_copy.try(:path) - else - original_path = Discourse.store.path_for(upload) - end - - custom_sprite_paths << original_path if original_path.present? - end - end - - custom_sprite_paths.map do |path| + custom_sprites = custom_sprite_paths.map do |path| if File.exist?(path) { filename: "#{File.basename(path, ".svg")}", @@ -262,6 +245,25 @@ module SvgSprite } end end + + if theme_id.present? + ThemeField.where(type_id: ThemeField.types[:theme_upload_var], name: THEME_SPRITE_VAR_NAME, theme_id: Theme.transform_ids(theme_id)) + .pluck(:upload_id, :theme_id).each do |upload_id, child_theme_id| + + begin + upload = Upload.find(upload_id) + custom_sprites << { + filename: "theme_#{theme_id}_#{upload_id}.svg", + sprite: upload.content + } + rescue => e + name = Theme.find(child_theme_id).name rescue nil + Discourse.warn_exception(e, "#{name} theme contains a corrupt svg upload") + end + + end + end + custom_sprites end end @@ -483,7 +485,7 @@ License - https://fontawesome.com/license/free (Icons: CC BY 4.0, Fonts: SIL OFL # Need to load full records for default values Theme.where(id: theme_ids).each do |theme| - settings = theme.cached_settings.each do |key, value| + _settings = theme.cached_settings.each do |key, value| if key.to_s.include?("_icon") && String === value theme_icon_settings |= value.split('|') end diff --git a/lib/theme_store/git_importer.rb b/lib/theme_store/git_importer.rb index 409a39eca49..6ef40d2c16f 100644 --- a/lib/theme_store/git_importer.rb +++ b/lib/theme_store/git_importer.rb @@ -110,7 +110,7 @@ class ThemeStore::GitImporter else Discourse::Utils.execute_command(git_ssh_command, "git", "clone", @url, @temp_folder) end - rescue RuntimeError => err + rescue RuntimeError raise RemoteTheme::ImportError.new(I18n.t("themes.import_error.git")) end ensure diff --git a/lib/theme_store/zip_exporter.rb b/lib/theme_store/zip_exporter.rb index edc8639f8a5..ed1d64c187d 100644 --- a/lib/theme_store/zip_exporter.rb +++ b/lib/theme_store/zip_exporter.rb @@ -53,8 +53,7 @@ class ThemeStore::ZipExporter raise RuntimeError.new("Theme exporter tried to leave directory") unless path.to_s.starts_with?(destination_folder) if ThemeField.types[field.type_id] == :theme_upload_var - filename = Discourse.store.path_for(field.upload) - content = filename ? File.read(filename) : Discourse.store.download(field.upload).read + content = field.upload.content else content = field.value end diff --git a/spec/lib/svg_sprite/svg_sprite_spec.rb b/spec/lib/svg_sprite/svg_sprite_spec.rb index 423c144c7a5..b72829af999 100644 --- a/spec/lib/svg_sprite/svg_sprite_spec.rb +++ b/spec/lib/svg_sprite/svg_sprite_spec.rb @@ -213,7 +213,7 @@ describe SvgSprite do end it "includes Font Awesome icon from groups" do - group = Fabricate(:group, flair_icon: "far-building") + _group = Fabricate(:group, flair_icon: "far-building") expect(SvgSprite.bundle).to match(/far-building/) end diff --git a/spec/models/theme_field_spec.rb b/spec/models/theme_field_spec.rb index 7fc6eacc3ee..ad43c0d1e09 100644 --- a/spec/models/theme_field_spec.rb +++ b/spec/models/theme_field_spec.rb @@ -174,7 +174,7 @@ HTML it "correctly handles extra JS fields" do js_field = theme.set_field(target: :extra_js, name: "discourse/controllers/discovery.js.es6", value: "import 'discourse/lib/ajax'; console.log('hello from .js.es6');") - js_2_field = theme.set_field(target: :extra_js, name: "discourse/controllers/discovery-2.js", value: "import 'discourse/lib/ajax'; console.log('hello from .js');") + _js_2_field = theme.set_field(target: :extra_js, name: "discourse/controllers/discovery-2.js", value: "import 'discourse/lib/ajax'; console.log('hello from .js');") hbs_field = theme.set_field(target: :extra_js, name: "discourse/templates/discovery.hbs", value: "{{hello-world}}") raw_hbs_field = theme.set_field(target: :extra_js, name: "discourse/templates/discovery.hbr", value: "{{hello-world}}") hbr_field = theme.set_field(target: :extra_js, name: "discourse/templates/other_discovery.hbr", value: "{{hello-world}}") @@ -429,4 +429,90 @@ HTML end end + context 'local js assets' do + + let :js_content do + "// not transpiled; console.log('hello world');" + end + + let :upload_file do + tmp = Tempfile.new(["jsfile", ".js"]) + File.write(tmp.path, js_content) + tmp + end + + after do + upload_file.unlink + end + + it "correctly handles local JS asset caching" do + + upload = UploadCreator.new(upload_file, "test.js", for_theme: true) + .create_for(Discourse::SYSTEM_USER_ID) + + js_field = theme.set_field( + target: :common, + type_id: ThemeField.types[:theme_upload_var], + name: 'test_js', + upload_id: upload.id + ) + + common_field = theme.set_field( + target: :common, + name: "head_tag", + value: "", + type: :html + ) + + theme.set_field( + target: :settings, + type: :yaml, + name: "yaml", + value: "hello: world" + ) + + theme.set_field( + target: :extra_js, + name: "discourse/controllers/discovery.js.es6", + value: "import 'discourse/lib/ajax'; console.log('hello from .js.es6');" + ) + + theme.save! + + # a bit fragile, but at least we test it properly + [theme.reload.javascript_cache.content, common_field.reload.javascript_cache.content].each do |js| + js_to_eval = <<~JS + var settings; + var window = {}; + var require = function(name) { + if(name == "discourse/lib/theme-settings-store") { + return({ + registerSettings: function(id, s) { + settings = s; + } + }); + } + } + window.require = require; + #{js} + settings + JS + + ctx = MiniRacer::Context.new + val = ctx.eval(js_to_eval) + ctx.dispose + + expect(val["theme_uploads"]["test_js"]).to eq(js_field.upload.url) + expect(val["theme_uploads_local"]["test_js"]).to eq(js_field.javascript_cache.local_url) + + end + + # this is important, we do not want local_js_urls to leak into scss + expect(theme.scss_variables).to include("$hello: unquote(\"world\");") + expect(theme.scss_variables).to include("$test_js: unquote(\"#{upload.url}\");") + + expect(theme.scss_variables).not_to include("theme_uploads") + end + end + end diff --git a/spec/models/upload_spec.rb b/spec/models/upload_spec.rb index f15dd8c8aeb..69c2e54177f 100644 --- a/spec/models/upload_spec.rb +++ b/spec/models/upload_spec.rb @@ -204,6 +204,13 @@ describe Upload do setup_s3 end + it "can download an s3 upload" do + stub_request(:get, upload.url) + .to_return(status: 200, body: "hello", headers: {}) + + expect(upload.content).to eq("hello") + end + it "should return the right upload when using base url (not CDN) for s3" do upload expect(Upload.get_from_url(upload.url)).to eq(upload)