FIX: Don't use backticks that take in inputs.

nathanpalmer · Mar 17, 2017 · e7c972a · e7c972a
1 parent b49bf88
commit e7c972a
Show file tree

Hide file tree

Showing 11 changed files with 51 additions and 57 deletions.
diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb
@@ -81,7 +81,8 @@ def create_upload(type, file, url)
       if filename == "image.png" && SiteSetting.convert_pasted_images_to_hq_jpg
         jpeg_path = "#{File.dirname(tempfile.path)}/image.jpg"
         OptimizedImage.ensure_safe_paths!(tempfile.path, jpeg_path)
-        `convert #{tempfile.path} -quality #{SiteSetting.convert_pasted_images_quality} #{jpeg_path}`
+
+        Discourse::Utils.execute_command('convert', tempfile.path, '-quality', SiteSetting.convert_pasted_images_quality.to_s, jpeg_path)
         # only change the format of the image when JPG is at least 5% smaller
         if File.size(jpeg_path) < File.size(tempfile.path) * 0.95
           filename = "image.jpg"

diff --git a/app/models/optimized_image.rb b/app/models/optimized_image.rb
@@ -215,8 +215,7 @@ def self.optimize(operation, from, to, dimensions, opts={})
   end
 
   def self.convert_with(instructions, to)
-    `#{instructions.join(" ")} &> /dev/null`
-    return false if $?.exitstatus != 0
+    return false unless system(instructions.join(" "), '&> /dev/null')
 
     ImageOptim.new.optimize_image!(to)
     true

diff --git a/app/models/upload.rb b/app/models/upload.rb
@@ -253,7 +253,7 @@ def self.get_from_url(url)
   end
 
   def self.fix_image_orientation(path)
-    `convert #{path} -auto-orient #{path}`
+    Discourse::Utils.execute_command('convert', path, '-auto-orient', path)
   end
 
   def self.migrate_to_new_scheme(limit=nil)

diff --git a/lib/backup_restore/backup_restore.rb b/lib/backup_restore/backup_restore.rb
@@ -1,4 +1,3 @@
-require "backup_restore/utils"
 require "backup_restore/backuper"
 require "backup_restore/restorer"
 

diff --git a/lib/backup_restore/backuper.rb b/lib/backup_restore/backuper.rb
@@ -1,8 +1,6 @@
 module BackupRestore
 
   class Backuper
-    include BackupRestore::Utils
-
     attr_reader :success
 
     def initialize(user_id, opts={})
@@ -198,7 +196,7 @@ def pg_dump_command
     def move_dump_backup
       log "Finalizing database dump file: #{@backup_filename}"
 
-      execute_command(
+      Discourse::Utils.execute_command(
         'mv', @dump_filename, File.join(@archive_directory, @backup_filename),
         failure_message: "Failed to move database dump file."
       )
@@ -212,15 +210,15 @@ def create_archive
       tar_filename = "#{@archive_basename}.tar"
 
       log "Making sure archive does not already exist..."
-      execute_command('rm', '-f', tar_filename)
-      execute_command('rm', '-f', "#{tar_filename}.gz")
+      Discourse::Utils.execute_command('rm', '-f', tar_filename)
+      Discourse::Utils.execute_command('rm', '-f', "#{tar_filename}.gz")
 
       log "Creating empty archive..."
-      execute_command('tar', '--create', '--file', tar_filename, '--files-from', '/dev/null')
+      Discourse::Utils.execute_command('tar', '--create', '--file', tar_filename, '--files-from', '/dev/null')
 
       log "Archiving data dump..."
       FileUtils.cd(File.dirname(@dump_filename)) do
-        execute_command(
+        Discourse::Utils.execute_command(
           'tar', '--append', '--dereference', '--file', tar_filename, File.basename(@dump_filename),
           failure_message: "Failed to archive data dump."
         )
@@ -231,7 +229,7 @@ def create_archive
       log "Archiving uploads..."
       FileUtils.cd(File.join(Rails.root, "public")) do
         if File.directory?(upload_directory)
-          execute_command(
+          Discourse::Utils.execute_command(
             'tar', '--append', '--dereference', '--file', tar_filename, upload_directory,
             failure_message: "Failed to archive uploads."
           )
@@ -243,7 +241,7 @@ def create_archive
       remove_tmp_directory
 
       log "Gzipping archive, this may take a while..."
-      execute_command('gzip', '-5', tar_filename, failure_message: "Failed to gzip archive.")
+      Discourse::Utils.execute_command('gzip', '-5', tar_filename, failure_message: "Failed to gzip archive.")
     end
 
     def after_create_hook
@@ -259,11 +257,11 @@ def remove_old
 
     def notify_user
       log "Notifying '#{@user.username}' of the end of the backup..."
-      if @success
-        SystemMessage.create_from_system_user(@user, :backup_succeeded, logs: pretty_logs(@logs))
-      else
-        SystemMessage.create_from_system_user(@user, :backup_failed, logs: pretty_logs(@logs))
-      end
+      status = @success ? :backup_succeeded : :backup_failed
+
+      SystemMessage.create_from_system_user(@user, status,
+        logs: Discouse::Utils.pretty_logs(@logs)
+      )
     end
 
     def clean_up

diff --git a/lib/backup_restore/restorer.rb b/lib/backup_restore/restorer.rb
@@ -6,8 +6,6 @@ class RestoreDisabledError < RuntimeError; end
   class FilenameMissingError < RuntimeError; end
 
   class Restorer
-    include BackupRestore::Utils
-
     attr_reader :success
 
     def initialize(user_id, opts={})
@@ -166,7 +164,7 @@ def sidekiq_has_running_jobs?
 
     def copy_archive_to_tmp_directory
       log "Copying archive to tmp directory..."
-      execute_command('cp', @source_filename, @archive_filename, failure_message: "Failed to copy archive to tmp directory.")
+      Discourse::Utils.execute_command('cp', @source_filename, @archive_filename, failure_message: "Failed to copy archive to tmp directory.")
     end
 
     def unzip_archive
@@ -175,7 +173,7 @@ def unzip_archive
       log "Unzipping archive, this may take a while..."
 
       FileUtils.cd(@tmp_directory) do
-        execute_command('gzip', '--decompress', @archive_filename, failure_message: "Failed to unzip archive.")
+        Discourse::Utils.execute_command('gzip', '--decompress', @archive_filename, failure_message: "Failed to unzip archive.")
       end
     end
 
@@ -185,7 +183,7 @@ def extract_metadata
       @metadata =
         if system('tar', '--list', '--file', @tar_filename, BackupRestore::METADATA_FILE)
           FileUtils.cd(@tmp_directory) do
-            execute_command(
+            Discourse::Utils.execute_command(
               'tar', '--extract', '--file', @tar_filename, BackupRestore::METADATA_FILE,
               failure_message: "Failed to extract metadata file."
             )
@@ -233,7 +231,7 @@ def extract_dump
       log "Extracting dump file..."
 
       FileUtils.cd(@tmp_directory) do
-        execute_command(
+        Discourse::Utils.execute_command(
           'tar', '--extract', '--file', @tar_filename, File.basename(@dump_filename),
           failure_message: "Failed to extract dump file."
         )
@@ -364,7 +362,7 @@ def extract_uploads
         log "Extracting uploads..."
 
         FileUtils.cd(@tmp_directory) do
-          execute_command(
+          Discourse::Utils.execute_command(
             'tar', '--extract', '--keep-newer-files', '--file', @tar_filename, 'uploads/',
             failure_message: "Failed to extract uploads."
           )
@@ -379,7 +377,7 @@ def extract_uploads
           previous_db_name = File.basename(tmp_uploads_path)
           current_db_name = RailsMultisite::ConnectionManagement.current_db
 
-          execute_command(
+          Discourse::Utils.execute_command(
             'rsync', '-avp', '--safe-links', "#{tmp_uploads_path}/", "uploads/#{current_db_name}/",
             failure_message: "Failed to restore uploads."
           )
@@ -404,11 +402,11 @@ def rollback
     def notify_user
       if user = User.find_by(email: @user_info[:email])
         log "Notifying '#{user.username}' of the end of the restore..."
-        if @success
-          SystemMessage.create_from_system_user(user, :restore_succeeded, logs: pretty_logs(@logs))
-        else
-          SystemMessage.create_from_system_user(user, :restore_failed, logs: pretty_logs(@logs))
-        end
+        status = @success ? :restore_succeeded : :restore_failed
+
+        SystemMessage.create_from_system_user(user, status,
+          logs: Discourse::Utils.pretty_logs(@logs)
+        )
       else
         log "Could not send notification to '#{@user_info[:username]}' (#{@user_info[:email]}), because the user does not exists..."
       end

diff --git a/lib/backup_restore/utils.rb b/lib/backup_restore/utils.rb
diff --git a/lib/discourse.rb b/lib/discourse.rb
@@ -1,4 +1,5 @@
 require 'cache'
+require 'open3'
 require_dependency 'plugin/instance'
 require_dependency 'auth/default_current_user_provider'
 require_dependency 'version'
@@ -16,6 +17,23 @@ class SidekiqExceptionHandler
     extend Sidekiq::ExceptionHandler
   end
 
+  class Utils
+    def self.execute_command(*command, failure_message: "")
+      stdout, stderr, status = Open3.capture3(*command)
+
+      if !status.success?
+        failure_message = "#{failure_message}\n" if !failure_message.blank?
+        raise "#{failure_message}#{stderr}"
+      end
+
+      stdout
+    end
+
+    def self.pretty_logs(logs)
+      logs.join("\n".freeze)
+    end
+  end
+
   # Log an exception.
   #
   # If your code is in a scheduled job, it is recommended to use the

diff --git a/lib/file_store/local_store.rb b/lib/file_store/local_store.rb
@@ -59,7 +59,7 @@ def path_for(upload)
     end
 
     def purge_tombstone(grace_period)
-      `find #{tombstone_dir} -mtime +#{grace_period} -type f -delete`
+      Discourse::Utils.execute_command('find', tombstone_dir, '-mtime', "+#{grace_period}", '-type f -delete')
     end
 
     def get_path_for(type, upload_id, sha, extension)

diff --git a/lib/letter_avatar.rb b/lib/letter_avatar.rb
@@ -82,7 +82,7 @@ def generate_fullsize(identity)
         '#{filename}'
       }
 
-      `convert #{instructions.join(" ")}`
+      Discourse::Utils.execute_command('convert', instructions.join(" ".freeze))
 
       ## do not optimize image, it will end up larger than original
       filename

diff --git a/lib/plugin/instance.rb b/lib/plugin/instance.rb
@@ -356,11 +356,12 @@ def activate!
     public_data = File.dirname(path) + "/public"
     if Dir.exists?(public_data)
       target = Rails.root.to_s + "/public/plugins/"
-      `mkdir -p #{target}`
+
+      Discourse::Utils.execute_command('mkdir', '-p', target)
       target << name.gsub(/\s/,"_")
       # TODO a cleaner way of registering and unregistering
-      `rm -f #{target}`
-      `ln -s #{public_data} #{target}`
+      Discourse::Utils.execute_command('rm', '-f', target)
+      Discourse::Utils.execute_command('ln', '-s', public_data, target)
     end
   end