From 6932a373a34a7150f8514af7edaf9048cc719a7d Mon Sep 17 00:00:00 2001 From: Jarek Radosz Date: Mon, 12 Oct 2020 18:25:06 +0200 Subject: [PATCH] FIX: Handle .discourse-compatibility syntax errors (#10891) Previously, any errors in those files would e.g. blow up the update process in docker_manager. Now it prints out an error and proceeds as if there was no compatibility file. Includes: * DEV: Extract setup_git_repo * DEV: Use `Dir.mktmpdir` * DEV: Default to `main` branch (The latest versions of git already do this, so to avoid problems do this by default) --- lib/version.rb | 14 ++++++++++-- spec/components/version_spec.rb | 38 ++++++++++++++++++++++++++++---- spec/models/remote_theme_spec.rb | 17 -------------- spec/services/themes_spec.rb | 17 -------------- spec/support/helpers.rb | 34 ++++++++++++++++++++++++---- 5 files changed, 76 insertions(+), 44 deletions(-) diff --git a/lib/version.rb b/lib/version.rb index e65fa22422b..cd39dbccea4 100644 --- a/lib/version.rb +++ b/lib/version.rb @@ -16,6 +16,8 @@ module Discourse end end + class InvalidVersionListError < StandardError; end + def self.has_needed_version?(current, needed) Gem::Version.new(current) >= Gem::Version.new(needed) end @@ -29,10 +31,16 @@ module Discourse # 2.4.4.beta6: some-other-branch-ref # 2.4.2.beta1: v1-tag def self.find_compatible_resource(version_list, version = ::Discourse::VERSION::STRING) - return unless version_list - version_list = YAML.load(version_list).sort_by { |v, pin| Gem::Version.new(v) }.reverse + begin + version_list = YAML.safe_load(version_list) + rescue Psych::SyntaxError, Psych::DisallowedClass => e + end + + raise InvalidVersionListError unless version_list.is_a?(Hash) + + version_list = version_list.sort_by { |v, pin| Gem::Version.new(v) }.reverse # If plugin compat version is listed as less than current Discourse version, take the version/hash listed before. checkout_version = nil @@ -54,5 +62,7 @@ module Discourse return unless File.directory?("#{path}/.git") compat_resource, std_error, s = Open3.capture3("git -C '#{path}' show HEAD@{upstream}:#{Discourse::VERSION_COMPATIBILITY_FILENAME}") Discourse.find_compatible_resource(compat_resource) if s.success? + rescue InvalidVersionListError => e + $stderr.puts "Invalid version list in #{path}" end end diff --git a/spec/components/version_spec.rb b/spec/components/version_spec.rb index aba3461f77d..c843a5ce6c5 100644 --- a/spec/components/version_spec.rb +++ b/spec/components/version_spec.rb @@ -4,9 +4,7 @@ require 'rails_helper' require 'version' describe Discourse::VERSION do - context "has_needed_version?" do - it "works for major comparisons" do expect(Discourse.has_needed_version?('1.0.0', '1.0.0')).to eq(true) expect(Discourse.has_needed_version?('2.0.0', '1.0.0')).to eq(true) @@ -44,10 +42,9 @@ describe Discourse::VERSION do expect(Discourse.has_needed_version?('1.3.0.beta4', '1.3.0.beta3')).to eq(true) expect(Discourse.has_needed_version?('1.3.0', '1.3.0.beta3')).to eq(true) end - end - context "compatible_resource" do + context "find_compatible_resource" do shared_examples "test compatible resource" do it "returns nil when the current version is above all pinned versions" do expect(Discourse.find_compatible_resource(version_list, "2.6.0")).to be_nil @@ -70,6 +67,10 @@ describe Discourse::VERSION do expect(Discourse.find_compatible_resource(nil)).to be_nil end + it "raises an error on invalid input" do + expect { Discourse.find_compatible_resource("1.0.0.beta1 12f82d5") }.to raise_error(Discourse::InvalidVersionListError) + end + context "with a regular compatible list" do let(:version_list) { <<~VERSION_LIST 2.5.0.beta6: twofivebetasix @@ -94,4 +95,33 @@ describe Discourse::VERSION do include_examples "test compatible resource" end end + + context "find_compatible_git_resource" do + let!(:git_directory) do + path = nil + + capture_stdout do + # Note the lack of colon between version and hash + path = setup_git_repo(".discourse-compatibility" => "1.0.0.beta1 12f82d5") + + # Simulate a remote upstream + `cd #{path} && git remote add origin #{path}/.git && git fetch -q` + `cd #{path} && git branch -u origin/main` + end + + path + end + + after do + FileUtils.remove_entry(git_directory) + end + + it "gracefully handles invalid input" do + output = capture_stderr do + expect(Discourse.find_compatible_git_resource(git_directory)).to be_nil + end + + expect(output).to include("Invalid version list") + end + end end diff --git a/spec/models/remote_theme_spec.rb b/spec/models/remote_theme_spec.rb index 70ebaaea78f..098fff51a33 100644 --- a/spec/models/remote_theme_spec.rb +++ b/spec/models/remote_theme_spec.rb @@ -4,23 +4,6 @@ require 'rails_helper' describe RemoteTheme do context '#import_remote' do - def setup_git_repo(files) - dir = Dir.tmpdir - repo_dir = "#{dir}/#{SecureRandom.hex}" - `mkdir #{repo_dir}` - `cd #{repo_dir} && git init . ` - `cd #{repo_dir} && git config user.email 'someone@cool.com'` - `cd #{repo_dir} && git config user.name 'The Cool One'` - `cd #{repo_dir} && git config commit.gpgsign 'false'` - files.each do |name, data| - FileUtils.mkdir_p(Pathname.new("#{repo_dir}/#{name}").dirname) - File.write("#{repo_dir}/#{name}", data) - `cd #{repo_dir} && git add #{name}` - end - `cd #{repo_dir} && git commit -am 'first commit'` - repo_dir - end - def about_json(love_color: "FAFAFA", tertiary_low_color: "FFFFFF", color_scheme_name: "Amazing", about_url: "https://www.site.com/about") <<~JSON { diff --git a/spec/services/themes_spec.rb b/spec/services/themes_spec.rb index 5a2f0d7741d..4e5c01eea79 100644 --- a/spec/services/themes_spec.rb +++ b/spec/services/themes_spec.rb @@ -10,23 +10,6 @@ describe ThemesInstallTask do end describe '.new' do - def setup_git_repo(files) - dir = Dir.tmpdir - repo_dir = "#{dir}/#{SecureRandom.hex}" - `mkdir #{repo_dir}` - `cd #{repo_dir} && git init . ` - `cd #{repo_dir} && git config user.email 'someone@cool.com'` - `cd #{repo_dir} && git config user.name 'The Cool One'` - `cd #{repo_dir} && git config commit.gpgsign 'false'` - files.each do |name, data| - FileUtils.mkdir_p(Pathname.new("#{repo_dir}/#{name}").dirname) - File.write("#{repo_dir}/#{name}", data) - `cd #{repo_dir} && git add #{name}` - end - `cd #{repo_dir} && git commit -am 'first commit'` - repo_dir - end - THEME_NAME = "awesome theme" def about_json(love_color: "FAFAFA", tertiary_low_color: "FFFFFF", color_scheme_name: "Amazing", about_url: "https://www.site.com/about", component: false) diff --git a/spec/support/helpers.rb b/spec/support/helpers.rb index ad5fe6873e4..24af7fd1690 100644 --- a/spec/support/helpers.rb +++ b/spec/support/helpers.rb @@ -128,18 +128,29 @@ module Helpers expect(sorted_tag_names(a)).to eq(sorted_tag_names(b)) end - def capture_stdout - old_stdout = $stdout + def capture_output(output_name) if ENV['RAILS_ENABLE_TEST_STDOUT'] yield return end + + previous_output = output_name == :stdout ? $stdout : $stderr + io = StringIO.new - $stdout = io + output_name == :stdout ? $stdout = io : $stderr = io + yield io.string ensure - $stdout = old_stdout + output_name == :stdout ? $stdout = previous_output : $stderr = previous_output + end + + def capture_stdout(&block) + capture_output(:stdout, &block) + end + + def capture_stderr(&block) + capture_output(:stderr, &block) end def set_subfolder(f) @@ -152,6 +163,21 @@ module Helpers end end + def setup_git_repo(files) + repo_dir = Dir.mktmpdir + `cd #{repo_dir} && git init . --initial-branch=main` + `cd #{repo_dir} && git config user.email 'someone@cool.com'` + `cd #{repo_dir} && git config user.name 'The Cool One'` + `cd #{repo_dir} && git config commit.gpgsign 'false'` + files.each do |name, data| + FileUtils.mkdir_p(Pathname.new("#{repo_dir}/#{name}").dirname) + File.write("#{repo_dir}/#{name}", data) + `cd #{repo_dir} && git add #{name}` + end + `cd #{repo_dir} && git commit -am 'first commit'` + repo_dir + end + class StubbedJob def initialize; end def perform(args); end