diff --git a/app/models/theme_field.rb b/app/models/theme_field.rb index 1fff6f85f77..5ed57393014 100644 --- a/app/models/theme_field.rb +++ b/app/models/theme_field.rb @@ -204,7 +204,11 @@ class ThemeField < ActiveRecord::Base begin content = File.read(path) - Nokogiri.XML(content) { |config| config.options = Nokogiri::XML::ParseOptions::NOBLANKS } + if content.to_s.bytesize > SvgSprite::MAX_THEME_SPRITE_SIZE + error = "Error with #{self.name}: Icon sprite file is too large" + else + Nokogiri.XML(content) { |config| config.options = Nokogiri::XML::ParseOptions::NOBLANKS } + end rescue => e error = "Error with #{self.name}: #{e.inspect}" end @@ -667,7 +671,7 @@ class ThemeField < ActiveRecord::Base rescue => e Discourse.warn_exception(e, message: "Failed to fetch svg sprite for theme field #{id}") else - if content.length > 4 * 1024**2 + if content.length > SvgSprite::MAX_THEME_SPRITE_SIZE Rails.logger.warn( "can't store theme svg sprite for theme #{theme_id} and upload #{upload_id}, sprite too big", ) diff --git a/lib/svg_sprite.rb b/lib/svg_sprite.rb index 1217bfa26f9..ba1c59b7c63 100644 --- a/lib/svg_sprite.rb +++ b/lib/svg_sprite.rb @@ -244,6 +244,8 @@ module SvgSprite THEME_SPRITE_VAR_NAME = "icons-sprite" + MAX_THEME_SPRITE_SIZE = 512.kilobytes + def self.preload settings_icons group_icons @@ -298,37 +300,45 @@ module SvgSprite def self.theme_svgs(theme_id) if theme_id.present? - theme_ids = Theme.transform_ids(theme_id) + cache + .defer_get_set_bulk( + Theme.transform_ids(theme_id), + lambda { |theme_id| "theme_svg_sprites_#{theme_id}" }, + ) do |theme_ids| + theme_field_uploads = + ThemeField.where( + type_id: ThemeField.types[:theme_upload_var], + name: THEME_SPRITE_VAR_NAME, + theme_id: theme_ids, + ).pluck(:upload_id) - get_set_cache("theme_svg_sprites_#{theme_ids.join(",")}") do - theme_field_uploads = - ThemeField.where( - type_id: ThemeField.types[:theme_upload_var], - name: THEME_SPRITE_VAR_NAME, - theme_id: theme_ids, - ).pluck(:upload_id) + theme_sprites = + ThemeSvgSprite.where(theme_id: theme_ids).pluck(:theme_id, :upload_id, :sprite) + missing_sprites = (theme_field_uploads - theme_sprites.map(&:second)) - theme_sprites = ThemeSvgSprite.where(theme_id: theme_ids).pluck(:upload_id, :sprite) - missing_sprites = (theme_field_uploads - theme_sprites.map(&:first)) - - if missing_sprites.present? - Rails.logger.warn( - "Missing ThemeSvgSprites for theme #{theme_id}, uploads #{missing_sprites.join(", ")}", - ) - end - - theme_sprites.reduce({}) do |symbols, (upload_id, sprite)| - begin - symbols.merge!(symbols_for("theme_#{theme_id}_#{upload_id}.svg", sprite, strict: false)) - rescue => e + if missing_sprites.present? Rails.logger.warn( - "Bad XML in custom sprite in theme with ID=#{theme_id}. Error info: #{e.inspect}", + "Missing ThemeSvgSprites for theme #{theme_id}, uploads #{missing_sprites.join(", ")}", ) end - symbols + theme_sprites + .map do |(theme_id, upload_id, sprite)| + begin + [theme_id, symbols_for("theme_#{theme_id}_#{upload_id}.svg", sprite, strict: false)] + rescue => e + Rails.logger.warn( + "Bad XML in custom sprite in theme with ID=#{theme_id}. Error info: #{e.inspect}", + ) + end + end + .compact + .to_h + .values_at(*theme_ids) end - end + .values + .compact + .reduce({}) { |a, b| a.merge!(b) } else {} end diff --git a/spec/fixtures/images/custom-theme-component-icon-sprite.svg b/spec/fixtures/images/custom-theme-component-icon-sprite.svg new file mode 100644 index 00000000000..901894ececa --- /dev/null +++ b/spec/fixtures/images/custom-theme-component-icon-sprite.svg @@ -0,0 +1,9 @@ + + + + + + + + + diff --git a/spec/lib/svg_sprite/svg_sprite_spec.rb b/spec/lib/svg_sprite/svg_sprite_spec.rb index e928280b7c1..4d7747c8617 100644 --- a/spec/lib/svg_sprite/svg_sprite_spec.rb +++ b/spec/lib/svg_sprite/svg_sprite_spec.rb @@ -275,6 +275,30 @@ RSpec.describe SvgSprite do expect(SvgSprite.bundle(theme.id)).to match(/my-custom-theme-icon/) end + it "includes custom icons in a theme and an attached theme component" do + theme_component = Fabricate(:theme, component: true) + theme.add_relative_theme!(:child, theme_component) + + fname1 = "custom-theme-icon-sprite.svg" + fname2 = "custom-theme-component-icon-sprite.svg" + + [[theme, fname1], [theme_component, fname2]].each do |t, fname| + upload = UploadCreator.new(file_from_fixtures(fname), fname, for_theme: true).create_for(-1) + expect(Upload.exists?(id: upload.id)).to eq(true) + + t.set_field( + target: :common, + name: SvgSprite.theme_sprite_variable_name, + upload_id: upload.id, + type: :theme_upload_var, + ) + t.save! + end + + expect(SvgSprite.bundle(theme.id)).to match(/my-custom-theme-icon/) + expect(SvgSprite.bundle(theme.id)).to match(/my-other-custom-theme-icon/) + end + it "does not fail on bad XML in custom icon sprite" do fname = "bad-xml-icon-sprite.svg" @@ -310,5 +334,39 @@ RSpec.describe SvgSprite do expect(Upload.exists?(id: upload.id)).to eq(true) expect(SvgSprite.bundle(theme.id)).to match(/my-custom-theme-icon/) end + + it "does not include theme icons if custom icon sprite is too large" do + fname = "theme-icon-sprite.svg" + symbols = "" + + 1500.times do |i| + id = "icon-id-#{i}" + path = + "M#{rand(1..100)} 2.18-2.087.277-4.216.42-6.378.42s-4.291-.143-6.378-.42c-1.085-.144-1.872-1.086-1.872-2.18v-4.25m16.5 0a2.18 2.18 0 00.75-1.661V8.706c0-1.081-.768-2.015-1.837-2.175a48.114 48.114 0 00-3.413-.387m4.5 8.006c-.194.165-.42.295-.673.38A23.978 23.978 0 0112 15.75c-2.648 0-5.195-.429-7.577-1.22a2.016 2.016 0 01-.673-.38m0 0A2.18 2.18 0 013 12.489V8.706c0-1.081.768-2.015 1.837-2.175a48.111 48.111 0 013.413-.387m7.5 0V5.25A2.25 2.25 0 0013.5 3h-3a2.25 .008z" + symbols += "\n" + end + + contents = + "#{symbols}" + + child_theme = Fabricate(:theme, component: true) + theme.add_relative_theme!(:child, child_theme) + + upload = + UploadCreator.new(file_from_contents(contents, fname), fname, for_theme: true).create_for( + -1, + ) + + child_theme.set_field( + target: :common, + name: SvgSprite.theme_sprite_variable_name, + upload_id: upload.id, + type: :theme_upload_var, + ) + child_theme.save! + + expect(Upload.exists?(id: upload.id)).to eq(true) + expect(SvgSprite.bundle(theme.id)).not_to match(/customthemeicon/) + end end end diff --git a/spec/models/theme_field_spec.rb b/spec/models/theme_field_spec.rb index 8e2260c903e..550de9a6b84 100644 --- a/spec/models/theme_field_spec.rb +++ b/spec/models/theme_field_spec.rb @@ -666,6 +666,30 @@ HTML FileStore::LocalStore.any_instance.stubs(:path_for).returns(nil) expect(theme_field.validate_svg_sprite_xml).to match("Error with icons-sprite") end + + it "raises an error when sprite is too big" do + fname = "theme-icon-sprite.svg" + symbols = "" + + 1500.times do |i| + id = "icon-id-#{i}" + path = + "M#{rand(1..100)} 2.18-2.087.277-4.216.42-6.378.42s-4.291-.143-6.378-.42c-1.085-.144-1.872-1.086-1.872-2.18v-4.25m16.5 0a2.18 2.18 0 00.75-1.661V8.706c0-1.081-.768-2.015-1.837-2.175a48.114 48.114 0 00-3.413-.387m4.5 8.006c-.194.165-.42.295-.673.38A23.978 23.978 0 0112 15.75c-2.648 0-5.195-.429-7.577-1.22a2.016 2.016 0 01-.673-.38m0 0A2.18 2.18 0 013 12.489V8.706c0-1.081.768-2.015 1.837-2.175a48.111 48.111 0 013.413-.387m7.5 0V5.25A2.25 2.25 0 0013.5 3h-3a2.25 .008z" + symbols += "\n" + end + + contents = + "#{symbols}" + + sprite = + UploadCreator.new(file_from_contents(contents, fname), fname, for_theme: true).create_for( + -1, + ) + + theme_field.update(upload: sprite) + + expect(theme_field.validate_svg_sprite_xml).to match("Error with icons-sprite") + end end describe "local js assets" do diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 7dc882cb25b..eb017855a5e 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -687,6 +687,12 @@ def file_from_fixtures(filename, directory = "images") File.new(tmp_file_path) end +def file_from_contents(contents, filename, directory = "images") + tmp_file_path = File.join(concurrency_safe_tmp_dir, SecureRandom.hex << filename) + File.write(tmp_file_path, contents) + File.new(tmp_file_path) +end + def plugin_from_fixtures(plugin_name) tmp_plugins_dir = File.join(concurrency_safe_tmp_dir, "plugins")