From 9fea43e46a7bb9ee1f8a3ce9d22bb41664f0852a Mon Sep 17 00:00:00 2001 From: David Taylor Date: Wed, 13 Nov 2019 09:57:39 +0000 Subject: [PATCH] DEV: Remove use of `cd` in the app (#8337) `FileUtils.cd` and `Dir.chdir` cause the working directory to change for the entire process. We run sidekiq jobs, hijacked requests and deferred jobs in threads, which can make working directory changes have unintended side-effects. - Add a rubocop rule to warn about usage of Dir.chdir and FileUtils.cd - Added rubocop:disable for scripts used outside the app - Refactored code using cd to use alternative methods - Temporarily skipped the rubocop check for lib/backup_restore. This will require more complex refactoring, so I will create a separate PR for review --- .rubocop.yml | 9 ++++++ lib/autospec/simple_runner.rb | 2 +- lib/rubocop/cop/discourse_cops.rb | 34 +++++++++++++++++++++++ lib/theme_store/git_importer.rb | 34 ++++++++++------------- lib/theme_store/zip_exporter.rb | 46 +++++++++++++++---------------- lib/theme_store/zip_importer.rb | 14 ++++------ script/plugin-translations.rb | 2 +- 7 files changed, 86 insertions(+), 55 deletions(-) create mode 100644 lib/rubocop/cop/discourse_cops.rb diff --git a/.rubocop.yml b/.rubocop.yml index 0d66c3bf6c1..0ca7d4314d2 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1,3 +1,6 @@ +require: + - ./lib/rubocop/cop/discourse_cops.rb + AllCops: TargetRubyVersion: 2.4 DisabledByDefault: true @@ -125,3 +128,9 @@ Style/SingleLineMethods: Style/Semicolon: Enabled: true AllowAsExpressionSeparator: true + +DiscourseCops/NoChdir: + Enabled: true + Exclude: + - 'spec/**/*' # Specs are run sequentially, so chdir can be used + - 'lib/backup_restore/**/*' # TODO \ No newline at end of file diff --git a/lib/autospec/simple_runner.rb b/lib/autospec/simple_runner.rb index 816a499b3ef..1f944d906fa 100644 --- a/lib/autospec/simple_runner.rb +++ b/lib/autospec/simple_runner.rb @@ -32,7 +32,7 @@ module Autospec end # launch rspec - Dir.chdir(Rails.root) do + Dir.chdir(Rails.root) do # rubocop:disable DiscourseCops/NoChdir because this is not part of the app env = { "RAILS_ENV" => "test" } if specs.split(' ').any? { |s| s =~ /^(.\/)?plugins/ } env["LOAD_PLUGINS"] = "1" diff --git a/lib/rubocop/cop/discourse_cops.rb b/lib/rubocop/cop/discourse_cops.rb new file mode 100644 index 00000000000..3acc3d71276 --- /dev/null +++ b/lib/rubocop/cop/discourse_cops.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module DiscourseCops + # Avoid using chdir - it is not thread safe. + # + # Instead, you may be able to use: + # Discourse::Utils.execute_command(chdir: 'test') do |runner| + # runner.exec('pwd') + # end + # + # @example + # # bad + # Dir.chdir('test') + class NoChdir < Cop + MSG = 'Chdir is not thread safe.' + + def_node_matcher :using_dir_chdir?, <<-MATCHER + (send (const nil? :Dir) :chdir ...) + MATCHER + + def_node_matcher :using_fileutils_cd?, <<-MATCHER + (send (const nil? :FileUtils) :cd ...) + MATCHER + + def on_send(node) + return if !(using_dir_chdir?(node) || using_fileutils_cd?(node)) + add_offense(node, message: MSG) + end + end + end + end +end diff --git a/lib/theme_store/git_importer.rb b/lib/theme_store/git_importer.rb index ac91cdec13a..2eebf58370e 100644 --- a/lib/theme_store/git_importer.rb +++ b/lib/theme_store/git_importer.rb @@ -33,14 +33,14 @@ class ThemeStore::GitImporter exporter = ThemeStore::ZipExporter.new(theme) local_temp_folder = exporter.export_to_folder - Dir.chdir(@temp_folder) do - Discourse::Utils.execute_command("git", "checkout", local_version) - Discourse::Utils.execute_command("rm -rf ./*/") - Discourse::Utils.execute_command("cp", "-rf", "#{local_temp_folder}/#{exporter.export_name}/.", @temp_folder) - Discourse::Utils.execute_command("git", "checkout", "about.json") + Discourse::Utils.execute_command(chdir: @temp_folder) do |runner| + runner.exec("git", "checkout", local_version) + runner.exec("rm -rf ./*/") + runner.exec("cp", "-rf", "#{local_temp_folder}/#{exporter.export_name}/.", @temp_folder) + runner.exec("git", "checkout", "about.json") # add + diff staged to catch uploads but exclude renamed assets - Discourse::Utils.execute_command("git", "add", "-A") - return Discourse::Utils.execute_command("git", "diff", "--staged", "--diff-filter=r") + runner.exec("git", "add", "-A") + return runner.exec("git", "diff", "--staged", "--diff-filter=r") end ensure FileUtils.rm_rf local_temp_folder if local_temp_folder @@ -49,18 +49,16 @@ class ThemeStore::GitImporter def commits_since(hash) commit_hash, commits_behind = nil - Dir.chdir(@temp_folder) do - commit_hash = Discourse::Utils.execute_command("git", "rev-parse", "HEAD").strip - commits_behind = Discourse::Utils.execute_command("git", "rev-list", "#{hash}..HEAD", "--count").strip + Discourse::Utils.execute_command(chdir: @temp_folder) do |runner| + commit_hash = runner.exec("git", "rev-parse", "HEAD").strip + commits_behind = runner.exec("git", "rev-list", "#{hash}..HEAD", "--count").strip end [commit_hash, commits_behind] end def version - Dir.chdir(@temp_folder) do - Discourse::Utils.execute_command("git", "rev-parse", "HEAD").strip - end + Discourse::Utils.execute_command("git", "rev-parse", "HEAD", chdir: @temp_folder).strip end def cleanup! @@ -82,9 +80,7 @@ class ThemeStore::GitImporter end def all_files - Dir.chdir(@temp_folder) do - Dir.glob("**/*").reject { |f| File.directory?(f) } - end + Dir.glob("**/*", base: @temp_folder).reject { |f| File.directory?(f) } end def [](value) @@ -111,10 +107,8 @@ class ThemeStore::GitImporter ssh_folder = "#{Pathname.new(Dir.tmpdir).realpath}/discourse_theme_ssh_#{SecureRandom.hex}" FileUtils.mkdir_p ssh_folder - Dir.chdir(ssh_folder) do - File.write('id_rsa', @private_key.strip) - FileUtils.chmod(0600, 'id_rsa') - end + File.write("#{ssh_folder}/id_rsa", @private_key.strip) + FileUtils.chmod(0600, "#{ssh_folder}/id_rsa") begin git_ssh_command = { 'GIT_SSH_COMMAND' => "ssh -i #{ssh_folder}/id_rsa -o StrictHostKeyChecking=no" } diff --git a/lib/theme_store/zip_exporter.rb b/lib/theme_store/zip_exporter.rb index 6fd17c635cc..6280601b489 100644 --- a/lib/theme_store/zip_exporter.rb +++ b/lib/theme_store/zip_exporter.rb @@ -26,34 +26,32 @@ class ThemeStore::ZipExporter end def export_to_folder - FileUtils.mkdir(@temp_folder) + destination_folder = File.join(@temp_folder, @export_name) + FileUtils.mkdir_p(destination_folder) - Dir.chdir(@temp_folder) do - FileUtils.mkdir(@export_name) + @theme.theme_fields.each do |field| + next unless path = field.file_path - @theme.theme_fields.each do |field| - next unless path = field.file_path + # Belt and braces approach here. All the user input should already be + # sanitized, but check for attempts to leave the temp directory anyway + pathname = Pathname.new(File.join(destination_folder, path)) + folder_path = pathname.parent.cleanpath + raise RuntimeError.new("Theme exporter tried to leave directory") unless folder_path.to_s.starts_with?(destination_folder) + pathname.parent.mkpath + path = pathname.realdirpath + raise RuntimeError.new("Theme exporter tried to leave directory") unless path.to_s.starts_with?(destination_folder) - # Belt and braces approach here. All the user input should already be - # sanitized, but check for attempts to leave the temp directory anyway - pathname = Pathname.new("#{@export_name}/#{path}") - folder_path = pathname.parent.cleanpath - raise RuntimeError.new("Theme exporter tried to leave directory") unless folder_path.to_s.starts_with?("#{@export_name}") - pathname.parent.mkpath - path = pathname.realdirpath - raise RuntimeError.new("Theme exporter tried to leave directory") unless path.to_s.starts_with?("#{@temp_folder}/#{@export_name}") - - 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 - else - content = field.value - end - File.write(path, content) + 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 + else + content = field.value end - - File.write("#{@export_name}/about.json", JSON.pretty_generate(@theme.generate_metadata_hash)) + File.write(path, content) end + + File.write(File.join(destination_folder, "about.json"), JSON.pretty_generate(@theme.generate_metadata_hash)) + @temp_folder end @@ -62,6 +60,6 @@ class ThemeStore::ZipExporter def export_package export_to_folder - Dir.chdir(@temp_folder) { Compression::Zip.new.compress(@temp_folder, @export_name) } + Compression::Zip.new.compress(@temp_folder, @export_name) end end diff --git a/lib/theme_store/zip_importer.rb b/lib/theme_store/zip_importer.rb index d252037ace6..81ce6222aa0 100644 --- a/lib/theme_store/zip_importer.rb +++ b/lib/theme_store/zip_importer.rb @@ -17,12 +17,10 @@ class ThemeStore::ZipImporter def import! FileUtils.mkdir(@temp_folder) - Dir.chdir(@temp_folder) do - available_size = SiteSetting.decompressed_theme_max_file_size_mb - Compression::Engine.engine_for(@original_filename).tap do |engine| - engine.decompress(@temp_folder, @filename, available_size) - engine.strip_directory(@temp_folder, @temp_folder, relative: true) - end + available_size = SiteSetting.decompressed_theme_max_file_size_mb + Compression::Engine.engine_for(@original_filename).tap do |engine| + engine.decompress(@temp_folder, @filename, available_size) + engine.strip_directory(@temp_folder, @temp_folder, relative: true) end rescue RuntimeError raise RemoteTheme::ImportError, I18n.t("themes.import_error.unpack_failed") @@ -53,9 +51,7 @@ class ThemeStore::ZipImporter end def all_files - Dir.chdir(@temp_folder) do - Dir.glob("**/**").reject { |f| File.directory?(f) } - end + Dir.glob("**/**", base: @temp_folder).reject { |f| File.directory?(f) } end def [](value) diff --git a/script/plugin-translations.rb b/script/plugin-translations.rb index 3bcc5f936b2..78535b4a546 100644 --- a/script/plugin-translations.rb +++ b/script/plugin-translations.rb @@ -42,7 +42,7 @@ class PluginTxUpdater PLUGINS.each do |plugin_name| plugin_dir = File.join(@base_dir, plugin_name) Bundler.with_clean_env do - Dir.chdir(plugin_dir) do + Dir.chdir(plugin_dir) do # rubocop:disable DiscourseCops/NoChdir because this is not part of the app puts '', plugin_dir, '-' * 80, '' begin