Escape user input with Open3 capture (#702)

- use Open3.capture3 for rsync instead of backticks in most cases
- dashboard: remove broken 'active_users' feature from developer dashboard
- myjobs: stick with rsync and Shellwords.escape for rsync command since using filter argument with Open3.capture3 is problematic
- myjobs: fix error reporting on new template page (display all error messages as bullet list at top of form) and fix focus

apps/dashboard/app/apps/ood_app.rb

@@ -213,8 +213,8 @@ def run_setup_production
           #
           # This makes the execution of the setup-production script use the same ruby versions
           # that Passenger uses when launching the app.
-          output = `PATH=#{path.join('bin').to_s}:$PATH bundle exec #{setup} 2>&1`
-          unless $?.success?
+          output, status = Open3.capture2e({'PATH' => path.join('bin').to_s + ':'+ ENV['PATH']}, 'bundle','exec', setup)
+          unless status.success?
             msg = "Per user setup failed for script at #{path}/#{setup} "
             msg += "for user #{Etc.getpwuid.name} with output: #{output}"
             raise SetupScriptFailed, msg

apps/dashboard/app/models/product.rb

@@ -189,12 +189,6 @@ def groups
     permissions(:group)
   end
 
-  def active_users
-    @active_users ||= `ps -o uid= -p $(pgrep -f '^Passenger .*#{Regexp.quote(router.path.to_s)}') 2> /dev/null | sort | uniq`.split.map(&:to_i).map do |id|
-      OodSupport::User.new id
-    end
-  end
-
   def git_repo?
     router.path.join(".git", "HEAD").file?
   end
@@ -235,13 +229,22 @@ def gemfile_specs
     end
 
     def get_git_remote
-      `cd #{router.path} 2> /dev/null && HOME="" git config --get remote.origin.url 2> /dev/null`.strip
+      Dir.chdir(router.path) do
+        o, s = Open3.capture2({'HOME'=>''}, 'git', 'config', '--get', 'remote.origin.url')
+        o.to_s.strip
+      end
     end
 
     def set_git_remote
-      target = router.path
-      Dir.chdir(target) do
-        `HOME="" git config --get remote.origin.url 2>/dev/null && HOME="" git remote set-url origin #{git_remote} 2> /dev/null || HOME="" git remote add origin #{git_remote} 2> /dev/null`
+      Dir.chdir(router.path) do
+        o, s = Open3.capture2({'HOME'=>''}, 'git', 'config', '--get', 'remote.origin.url')
+        if s.success?
+          o, s = Open3.capture2({'HOME'=>''}, 'git', 'remote', 'set-url', 'origin', git_remote)
+
+          unless s.success?
+            o, s = Open3.capture2({'HOME'=>''}, 'git', 'remote', 'add', 'origin', git_remote)
+          end
+        end
       end
     end
 
@@ -292,7 +295,9 @@ def git_describe
     end
 
     def git_status
-      files = `cd #{router.path} 2> /dev/null && HOME="" git status --porcelain 2> /dev/null`.split("\n")
+      files = Dir.chdir(router.path) do
+        `HOME="" git status --porcelain 2> /dev/null`.split("\n")
+      end
       results = {}
       results[:unstaged]  = files.select {|v| /^\s\w .+$/ =~ v}
       results[:staged]    = files.select {|v| /^\w\s .+$/ =~ v}

apps/dashboard/app/views/products/_product.html.erb

@@ -9,9 +9,6 @@
     <% if product.git_repo? %>
       <span class="text-muted">[<%= product.version %>]</span>
     <% end %>
-    <% unless product.active_users.empty? %>
-      <span class="label label-default pull-right"><%= pluralize product.active_users.size, "active user" %></span>
-    <% end %>
     <p>
       <%= manifest_markdown(product.description) %>
     </p>

apps/dashboard/app/views/products/show.html.erb

@@ -43,12 +43,6 @@
       <strong>Name:</strong>
       <%= @product.app.title %>
     </p>
-    <p>
-      <span data-toggle="popover" data-trigger="hover" data-placement="bottom" title="Active Users" data-content="<%= @product.active_users.join("<br />") %>" data-html="true">
-        <strong>Active users:</strong>
-        <%= @product.active_users.size %>
-      </span>
-    </p>
     <p>
       <strong>Type:</strong>
       <%= app_type_title @product.app %>

apps/myjobs/app/controllers/templates_controller.rb

@@ -24,38 +24,47 @@ def new
   # POST /templates
   # POST /templates.json
   def create
-
     @template = template_params[:path].blank? ? Template.default : Template.new(template_params[:path])
-    @template.script = template_params[:script] if template_params[:script].present?
-    @template.name = template_params[:name]
-    @template.host = template_params[:host]
-    @template.notes = template_params[:notes]
+    saved = false
 
-    # TODO this whole create method can be cleaned up
-    template_location = Pathname.new(@template.path)
+    begin
+      @template.script = template_params[:script] if template_params[:script].present?
+      @template.name = template_params[:name]
+      @template.host = template_params[:host]
+      @template.notes = template_params[:notes]
 
-    data_location = OodAppkit.dataroot.join('templates').join(@template.name.parameterize.underscore)
+      # TODO this whole create method can be cleaned up
+      template_location = Pathname.new(@template.path)
 
-    if data_location.exist? then @template.errors.add(:name, "must be unique.") end
-    unless template_location.exist? then @template.errors.add(:path, "does not exist.") end
+      data_location = OodAppkit.dataroot.join('templates').join(@template.name.parameterize.underscore)
 
-    # validate path we are copying from. safe_path is a boolean, error contains the error string if false
-    copy_safe, error = Filesystem.new.validate_path_is_copy_safe(template_location.to_s)
-    @template.errors.add(:path, error) unless copy_safe
+      if data_location.exist? then @template.errors.add(:name, "must be unique.") end
+      unless template_location.exist? then @template.errors.add(:path, "does not exist.") end
 
-    if template_location.file? then @template.errors.add(:path, "must point to a directory.") end
+      # validate path we are copying from. safe_path is a boolean, error contains the error string if false
+      copy_safe, error = Filesystem.new.validate_path_is_copy_safe(template_location.to_s)
+      @template.errors.add(:path, error) unless copy_safe
 
-    if @template.errors.empty?
-      FileUtils.mkdir_p(data_location)
-      copy_dir(template_location, data_location)
-      @template.path = data_location.to_s
+      if template_location.file? then @template.errors.add(:path, "must point to a directory.") end
 
-      yaml = { 'name' => @template.name, 'host' => @template.host, 'notes' => @template.notes, 'script' => @template.script }
-      File.open(data_location.join('manifest.yml'), 'w') do |file|
-        file.write(yaml.to_yaml)
-      end
+      if @template.errors.empty?
+        FileUtils.mkdir_p(data_location)
+        error, status = Filesystem.new.copy_dir(template_location, data_location)
+        unless status.success?
+          @template.errors.add(:path, error)
+        else
+          @template.path = data_location.to_s
+
+          yaml = { 'name' => @template.name, 'host' => @template.host, 'notes' => @template.notes, 'script' => @template.script }
+          File.open(data_location.join('manifest.yml'), 'w') do |file|
+            file.write(yaml.to_yaml)
+          end
 
-      saved = true
+          saved = true
+        end
+      end
+    rescue => e
+      @template.errors.add(:base, e.message)
     end
 
     respond_to do |format|
@@ -94,17 +103,4 @@ def set_template
     def template_params
       params.require(:template).permit(:name, :path, :host, :notes, :script)
     end
-
-  # Copies the data in a Location to a destination path using rsync.
-  #
-  # @param [String, Pathname] dest The target location path.
-  # @return [Location] The target location path wrapped by Location instance.
-  def copy_dir(src, dest)
-    # @path has / auto-dropped, so we add it to make sure we copy everything
-    # in the old directory to the new
-    `rsync -r --exclude='.svn' --exclude='.git' --exclude='.gitignore' --filter=':- .gitignore' '#{src.to_s}/' '#{dest.to_s}'`
-
-    # return target location so we can chain method
-    dest
-  end
 end

apps/myjobs/app/models/filesystem.rb

@@ -70,16 +70,27 @@ def validate_path_is_copy_safe(path)
   end
 
   def du(path, timeout)
-    Open3.capture3 "timeout #{timeout}s du -cbs #{Shellwords.escape(path)}"
+    Open3.capture3("timeout", "#{timeout}s", "du", "-cbs", path)
+  end
+
+  # Copies the data in a source to a destination path using rsync.
+  #
+  # @param [String, Pathname] src The src path.
+  # @param [String, Pathname] dest The target path.
+  # @return array of the [output, status] of the command
+  def copy_dir(src, dest)
+    output = `rsync -r --exclude='.svn' --exclude='.git' --exclude='.gitignore' --filter=':- .gitignore' 2>&1 #{Shellwords.escape(src.to_s)}/ #{Shellwords.escape(dest.to_s)}`
+    [output, $?]
   end
 
   # FIXME: some duplication here between du command above and this; we probably
   # want to use the above
   #
-  # Get the disk usage of a path in bytes, nil if path is invalid
+  # Get the disk usage of a path in bytes, nil if path does not exist
   def path_size (path)
     if Dir.exist? path
-      Integer(`du -s -b #{path}`.split('/')[0])
+      o, e, s = Open3.capture3('du', '-s', '-b', path)
+      o.split('/')[0].to_i
     end
   end
 end

apps/myjobs/app/views/templates/_form.html.erb

@@ -3,12 +3,23 @@
 
     <script type="text/javascript">
       $( document ).ready(function() {
-        $(".has-error input").first().focus();
+          let inputs = $(".has-error input");
+          if(inputs.length > 0){
+            inputs.first().focus();
+          }
+          else{
+            $('#error_explanation').focus()
+          }
       });
     </script>
 
     <div id="error_explanation">
       <h4><%= pluralize(@template.errors.count, "error") %> prohibited this template from being saved:</h4>
+      <ul>
+        <% @template.errors.full_messages.each do |msg| %>
+          <li><%= msg %></li>
+        <% end %>
+      </ul>
     </div>
 
   <% end %>

apps/myjobs/test/models/filesystem_test.rb

@@ -0,0 +1,27 @@
+require 'test_helper'
+
+class FilesystemTest < ActiveSupport::TestCase
+  test "copy_dir copies directories excluding .git .svn and files in gitignore" do
+    Dir.mktmpdir do |dir|
+      tmp = Pathname.new(dir).join('t')
+      tmp2 = Pathname.new(dir).join('t2')
+
+      Filesystem.new.copy_dir Rails.root.join('example_templates', 'default'), tmp
+
+      assert_equal 2, tmp.children.count, "job script and manifest should have been copied"
+
+      tmp.join('.gitignore').write('manifest.yml')
+      tmp.join('.git').mkpath
+      tmp.join('.svn').mkpath
+
+      Filesystem.new.copy_dir tmp, tmp2
+
+      assert_equal 1, tmp2.children.count, "only job template should exist cause the manifest.yml should have been ignored"
+    end
+  end
+
+  test "copy_dir doesn't allow command injection" do
+    o,s = Filesystem.new.copy_dir("/dev/null';echo INJECTED;'", '/dev/null')
+    assert_equal 0, o.split("\n").grep(/^INJECTED$/).count, "expected not to find INJECTED in output: #{o}"
+  end
+end