From 1848a63cadea68d447de0cd6ed2e2f6e47acaaf9 Mon Sep 17 00:00:00 2001 From: Eric Franz Date: Thu, 22 Oct 2020 20:24:45 -0400 Subject: [PATCH 1/8] myjobs: properly escape rsync arguments use Open3.capture3 for rsync instead of backticks --- .../app/controllers/templates_controller.rb | 16 +--------------- apps/myjobs/app/models/filesystem.rb | 13 ++++++++++++- apps/myjobs/test/models/filesystem_test.rb | 8 ++++++++ 3 files changed, 21 insertions(+), 16 deletions(-) create mode 100644 apps/myjobs/test/models/filesystem_test.rb diff --git a/apps/myjobs/app/controllers/templates_controller.rb b/apps/myjobs/app/controllers/templates_controller.rb index 0252d38b93..4e084ffc41 100644 --- a/apps/myjobs/app/controllers/templates_controller.rb +++ b/apps/myjobs/app/controllers/templates_controller.rb @@ -24,7 +24,6 @@ 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] @@ -47,7 +46,7 @@ def create if @template.errors.empty? FileUtils.mkdir_p(data_location) - copy_dir(template_location, data_location) + Filesystem.new.copy_dir(template_location, data_location) @template.path = data_location.to_s yaml = { 'name' => @template.name, 'host' => @template.host, 'notes' => @template.notes, 'script' => @template.script } @@ -94,17 +93,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 diff --git a/apps/myjobs/app/models/filesystem.rb b/apps/myjobs/app/models/filesystem.rb index aa80113d67..03f5243af9 100644 --- a/apps/myjobs/app/models/filesystem.rb +++ b/apps/myjobs/app/models/filesystem.rb @@ -70,7 +70,18 @@ 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 Location to a destination path using rsync. + # + # @param [String, Pathname] dest The target location path. + # @return [Pathname] The target location path as Pathname obj + def copy_dir(src, dest) + Open3.capture3({}, 'rsync', "-r --exclude='.svn' --exclude='.git' --exclude='.gitignore' --filter=':- .gitignore", src.to_s + "/", dest.to_s) + # FIXME: error handling + # + Pathname.new(dest) end # FIXME: some duplication here between du command above and this; we probably diff --git a/apps/myjobs/test/models/filesystem_test.rb b/apps/myjobs/test/models/filesystem_test.rb new file mode 100644 index 0000000000..e1c424f9fe --- /dev/null +++ b/apps/myjobs/test/models/filesystem_test.rb @@ -0,0 +1,8 @@ +require 'test_helper' + +class FilesystemTest < ActiveSupport::TestCase + test "copy_dir doesn't allow command injection" do + o,e,s = Filesystem.new.copy_dir("/dev/null';echo INJECTED;'", '/dev/null') + refute_match /INJECTED/, o + end +end From 21778ec6af14bdab02ca8c0eafed96938fb6c2fd Mon Sep 17 00:00:00 2001 From: Eric Franz Date: Thu, 22 Oct 2020 20:29:20 -0400 Subject: [PATCH 2/8] use Open3.capture methods instead of backticks --- apps/dashboard/app/apps/ood_app.rb | 4 ++-- apps/dashboard/app/models/product.rb | 21 ++++++++++++++++----- apps/myjobs/app/models/filesystem.rb | 5 +++-- 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/apps/dashboard/app/apps/ood_app.rb b/apps/dashboard/app/apps/ood_app.rb index b2f8215e08..d9fd1c7503 100644 --- a/apps/dashboard/app/apps/ood_app.rb +++ b/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 diff --git a/apps/dashboard/app/models/product.rb b/apps/dashboard/app/models/product.rb index 7f6b9d023c..1bc1cedfa9 100644 --- a/apps/dashboard/app/models/product.rb +++ b/apps/dashboard/app/models/product.rb @@ -235,13 +235,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 +301,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} diff --git a/apps/myjobs/app/models/filesystem.rb b/apps/myjobs/app/models/filesystem.rb index 03f5243af9..128e1e311d 100644 --- a/apps/myjobs/app/models/filesystem.rb +++ b/apps/myjobs/app/models/filesystem.rb @@ -87,10 +87,11 @@ def copy_dir(src, dest) # 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 From 21086d685feccaeda1ec9fb3dc06a3b75c9e892b Mon Sep 17 00:00:00 2001 From: Eric Franz Date: Thu, 22 Oct 2020 20:30:46 -0400 Subject: [PATCH 3/8] remove broken 'active_users' from develop ui --- apps/dashboard/app/models/product.rb | 6 ------ apps/dashboard/app/views/products/_product.html.erb | 3 --- apps/dashboard/app/views/products/show.html.erb | 6 ------ 3 files changed, 15 deletions(-) diff --git a/apps/dashboard/app/models/product.rb b/apps/dashboard/app/models/product.rb index 1bc1cedfa9..5c55e4dbdc 100644 --- a/apps/dashboard/app/models/product.rb +++ b/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 diff --git a/apps/dashboard/app/views/products/_product.html.erb b/apps/dashboard/app/views/products/_product.html.erb index 5748ff1092..ad1d590ed5 100644 --- a/apps/dashboard/app/views/products/_product.html.erb +++ b/apps/dashboard/app/views/products/_product.html.erb @@ -9,9 +9,6 @@ <% if product.git_repo? %> [<%= product.version %>] <% end %> - <% unless product.active_users.empty? %> - <%= pluralize product.active_users.size, "active user" %> - <% end %>

<%= manifest_markdown(product.description) %>

diff --git a/apps/dashboard/app/views/products/show.html.erb b/apps/dashboard/app/views/products/show.html.erb index 3f107de3f9..d6a7276921 100644 --- a/apps/dashboard/app/views/products/show.html.erb +++ b/apps/dashboard/app/views/products/show.html.erb @@ -43,12 +43,6 @@ Name: <%= @product.app.title %>

-

- ") %>" data-html="true"> - Active users: - <%= @product.active_users.size %> - -

Type: <%= app_type_title @product.app %> From 3bc055476bea4e5d62e29df36473405cd4962464 Mon Sep 17 00:00:00 2001 From: Eric Franz Date: Thu, 22 Oct 2020 20:31:24 -0400 Subject: [PATCH 4/8] myjobs: error handling to Filesystem#copy_dir --- apps/myjobs/app/models/filesystem.rb | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/apps/myjobs/app/models/filesystem.rb b/apps/myjobs/app/models/filesystem.rb index 128e1e311d..1e5481a291 100644 --- a/apps/myjobs/app/models/filesystem.rb +++ b/apps/myjobs/app/models/filesystem.rb @@ -70,7 +70,7 @@ def validate_path_is_copy_safe(path) end def du(path, timeout) - Open3.capture3 "timeout", "#{timeout}s", "du", "-cbs", path + Open3.capture3("timeout", "#{timeout}s", "du", "-cbs", path) end # Copies the data in a Location to a destination path using rsync. @@ -78,10 +78,13 @@ def du(path, timeout) # @param [String, Pathname] dest The target location path. # @return [Pathname] The target location path as Pathname obj def copy_dir(src, dest) - Open3.capture3({}, 'rsync', "-r --exclude='.svn' --exclude='.git' --exclude='.gitignore' --filter=':- .gitignore", src.to_s + "/", dest.to_s) - # FIXME: error handling - # - Pathname.new(dest) + o, s = Open3.capture2e({}, 'rsync', "-r --exclude='.svn' --exclude='.git' --exclude='.gitignore' --filter=':- .gitignore", src.to_s + "/", dest.to_s) + + if(s.success?) + Pathname.new(dest) + else + raise "rsync exited with error: #{o}" + end end # FIXME: some duplication here between du command above and this; we probably From 8d018238d699486db25a09b8593dca0be48a9646 Mon Sep 17 00:00:00 2001 From: Eric Franz Date: Thu, 22 Oct 2020 20:31:47 -0400 Subject: [PATCH 5/8] skip broken tests --- apps/myjobs/test/models/configuration_singleton_test.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/apps/myjobs/test/models/configuration_singleton_test.rb b/apps/myjobs/test/models/configuration_singleton_test.rb index e3f12254ba..c1498b67d5 100644 --- a/apps/myjobs/test/models/configuration_singleton_test.rb +++ b/apps/myjobs/test/models/configuration_singleton_test.rb @@ -56,6 +56,10 @@ def config_via_runner(env: 'development', envvars: '') end test "loading custom OSC external config in production env" do + # TypeError: incompatible marshal file format (can't be read) + # format version 4.8 required; 32.32 given + skip + config_root = Rails.root.join('config','examples','osc') config = config_via_runner(env: 'production', envvars: "OOD_APP_CONFIG_ROOT=#{config_root}") @@ -65,6 +69,10 @@ def config_via_runner(env: 'development', envvars: '') end test "loading custom AweSim external config in production env" do + # TypeError: incompatible marshal file format (can't be read) + # format version 4.8 required; 32.32 given + skip + config_root = Rails.root.join('config','examples','awesim') config = config_via_runner(env: 'production', envvars: "OOD_APP_CONFIG_ROOT=#{config_root}") From 35b13e42705f9d72a172d47ad03a3a20112ada86 Mon Sep 17 00:00:00 2001 From: Eric Franz Date: Fri, 23 Oct 2020 11:55:48 -0400 Subject: [PATCH 6/8] myjobs: fix rsync by reverting to backticks Open3.capture methods don't appear to deal well with arguments with spaces so revert to backticks with Shellwords.escape for escaping the values added tests to verify the happy trail as well --- apps/myjobs/app/models/filesystem.rb | 16 ++++++--------- apps/myjobs/test/models/filesystem_test.rb | 23 ++++++++++++++++++++-- 2 files changed, 27 insertions(+), 12 deletions(-) diff --git a/apps/myjobs/app/models/filesystem.rb b/apps/myjobs/app/models/filesystem.rb index 1e5481a291..c610b809fe 100644 --- a/apps/myjobs/app/models/filesystem.rb +++ b/apps/myjobs/app/models/filesystem.rb @@ -73,18 +73,14 @@ def du(path, timeout) Open3.capture3("timeout", "#{timeout}s", "du", "-cbs", path) end - # Copies the data in a Location to a destination path using rsync. + # Copies the data in a source to a destination path using rsync. # - # @param [String, Pathname] dest The target location path. - # @return [Pathname] The target location path as Pathname obj + # @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) - o, s = Open3.capture2e({}, 'rsync', "-r --exclude='.svn' --exclude='.git' --exclude='.gitignore' --filter=':- .gitignore", src.to_s + "/", dest.to_s) - - if(s.success?) - Pathname.new(dest) - else - raise "rsync exited with error: #{o}" - end + 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 diff --git a/apps/myjobs/test/models/filesystem_test.rb b/apps/myjobs/test/models/filesystem_test.rb index e1c424f9fe..d512557d83 100644 --- a/apps/myjobs/test/models/filesystem_test.rb +++ b/apps/myjobs/test/models/filesystem_test.rb @@ -1,8 +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,e,s = Filesystem.new.copy_dir("/dev/null';echo INJECTED;'", '/dev/null') - refute_match /INJECTED/, o + 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 From c633f3dd5826db90212097a73458346b4538dd7c Mon Sep 17 00:00:00 2001 From: Eric Franz Date: Fri, 23 Oct 2020 11:58:15 -0400 Subject: [PATCH 7/8] myjobs: fix error reporting in new template form in controller, move bulk of the create block into begin/rescue and add base error message for any problems check for rsync error and set to path attribute if error message present display all error messages as bullet list at top of form set focus to top of error list of no input valid --- .../app/controllers/templates_controller.rb | 54 +++++++++++-------- .../myjobs/app/views/templates/_form.html.erb | 13 ++++- 2 files changed, 44 insertions(+), 23 deletions(-) diff --git a/apps/myjobs/app/controllers/templates_controller.rb b/apps/myjobs/app/controllers/templates_controller.rb index 4e084ffc41..ab7f66161b 100644 --- a/apps/myjobs/app/controllers/templates_controller.rb +++ b/apps/myjobs/app/controllers/templates_controller.rb @@ -25,36 +25,46 @@ def new # 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) - Filesystem.new.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| diff --git a/apps/myjobs/app/views/templates/_form.html.erb b/apps/myjobs/app/views/templates/_form.html.erb index 8d664eaef4..573a240fc2 100644 --- a/apps/myjobs/app/views/templates/_form.html.erb +++ b/apps/myjobs/app/views/templates/_form.html.erb @@ -3,12 +3,23 @@

<%= pluralize(@template.errors.count, "error") %> prohibited this template from being saved:

+
    + <% @template.errors.full_messages.each do |msg| %> +
  • <%= msg %>
    • + <% end %> +
<% end %> From c7182fba9fbc4a8faeaef9d1fbcd6e8a7b127d67 Mon Sep 17 00:00:00 2001 From: Eric Franz Date: Fri, 23 Oct 2020 14:36:00 -0400 Subject: [PATCH 8/8] Revert "skip broken tests" This reverts commit 8d018238d699486db25a09b8593dca0be48a9646. --- apps/myjobs/test/models/configuration_singleton_test.rb | 8 -------- 1 file changed, 8 deletions(-) diff --git a/apps/myjobs/test/models/configuration_singleton_test.rb b/apps/myjobs/test/models/configuration_singleton_test.rb index c1498b67d5..e3f12254ba 100644 --- a/apps/myjobs/test/models/configuration_singleton_test.rb +++ b/apps/myjobs/test/models/configuration_singleton_test.rb @@ -56,10 +56,6 @@ def config_via_runner(env: 'development', envvars: '') end test "loading custom OSC external config in production env" do - # TypeError: incompatible marshal file format (can't be read) - # format version 4.8 required; 32.32 given - skip - config_root = Rails.root.join('config','examples','osc') config = config_via_runner(env: 'production', envvars: "OOD_APP_CONFIG_ROOT=#{config_root}") @@ -69,10 +65,6 @@ def config_via_runner(env: 'development', envvars: '') end test "loading custom AweSim external config in production env" do - # TypeError: incompatible marshal file format (can't be read) - # format version 4.8 required; 32.32 given - skip - config_root = Rails.root.join('config','examples','awesim') config = config_via_runner(env: 'production', envvars: "OOD_APP_CONFIG_ROOT=#{config_root}")