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

@@ -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

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"

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

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" }

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

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)

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