diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index f36239269a..930c887276 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -43,6 +43,10 @@ jobs: bundle install gem install rake + - name: Setup Node.js + run: | + npm install -g node-gyp + - name: Setup rclone run: sudo apt-get update && sudo apt-get install rclone diff --git a/apps/dashboard/app/controllers/files_controller.rb b/apps/dashboard/app/controllers/files_controller.rb index 9c6406218e..f737a4a57b 100644 --- a/apps/dashboard/app/controllers/files_controller.rb +++ b/apps/dashboard/app/controllers/files_controller.rb @@ -2,6 +2,8 @@ class FilesController < ApplicationController include ZipTricks::RailsStreaming + before_action :strip_sendfile_headers, only: [:fs] + def fs request.format = 'json' if request.headers['HTTP_ACCEPT'].split(',').include?('application/json') @@ -52,11 +54,11 @@ def fs zip_tricks_stream do |zip| @path.files_to_zip.each do |file| begin - next unless File.readable?(file.path) + next unless File.readable?(file.realpath) - if File.file?(file.path) + if File.file?(file.realpath) zip.write_deflated_file(file.relative_path.to_s) do |zip_file| - IO.copy_stream(file.path, zip_file) + IO.copy_stream(file.realpath, zip_file) end else zip.add_empty_directory(dirname: file.relative_path.to_s) @@ -167,6 +169,13 @@ def edit private + # set these headers to nil so that we (Rails) will read files + # off of disk instead of nginx. + def strip_sendfile_headers + request.headers['HTTP_X_SENDFILE_TYPE'] = nil + request.headers['HTTP_X_ACCEL_MAPPING'] = nil + end + def normalized_path(path = params[:filepath]) Pathname.new("/" + path.to_s.chomp("/").delete_prefix("/")) end diff --git a/apps/dashboard/app/models/active_jobs/jobstatusdata.rb b/apps/dashboard/app/models/active_jobs/jobstatusdata.rb index 6b5c647549..3742a49dd5 100644 --- a/apps/dashboard/app/models/active_jobs/jobstatusdata.rb +++ b/apps/dashboard/app/models/active_jobs/jobstatusdata.rb @@ -118,10 +118,8 @@ def extended_data_slurm(info) attributes.push Attribute.new "Total CPUs", info.native[:cpus] attributes.push Attribute.new "Time Limit", info.native[:time_limit] attributes.push Attribute.new "Time Used", info.native[:time_used] - attributes.push Attribute.new "Start Time", - DateTime.parse(info.native[:start_time]).strftime("%Y-%m-%d %H:%M:%S") unless info.native[:start_time] == "N/A" - attributes.push Attribute.new "End Time", - DateTime.parse(info.native[:end_time]).strftime("%Y-%m-%d %H:%M:%S") unless info.native[:end_time] == "N/A" + attributes.push Attribute.new "Start Time", safe_parse_time(info.native[:start_time]) + attributes.push Attribute.new "End Time", safe_parse_time(info.native[:end_time]) attributes.push Attribute.new "Memory", info.native[:min_memory] attributes.push Attribute.new "GRES", info.native[:gres].gsub(/gres:/, "") unless info.native[:gres] == "N/A" self.native_attribs = attributes @@ -305,6 +303,18 @@ def extended_data_default(info) private + def safe_parse_time(time) + if ['N/A', 'NONE'].include?(time.to_s) + '' + else + begin + DateTime.parse(time.to_s).strftime('%Y-%m-%d %H:%M:%S') + rescue Date::Error + '' + end + end + end + def build_file_explorer_url(path) writable_path = (path.writable? ? path : ENV["HOME"]).to_s diff --git a/apps/dashboard/app/models/batch_connect/session.rb b/apps/dashboard/app/models/batch_connect/session.rb index d62752c32d..5c5d667a86 100644 --- a/apps/dashboard/app/models/batch_connect/session.rb +++ b/apps/dashboard/app/models/batch_connect/session.rb @@ -39,10 +39,6 @@ def get_binding # @return [String] session title attr_accessor :title - # The view used to display the connection information for this session - # @return [String, nil] session view - attr_accessor :view - # Batch connect script type # @return [String] script type attr_accessor :script_type @@ -71,13 +67,13 @@ def render_info_view # Return the Batch Connect app from the session token # @return [BatchConnect::App] def app - BatchConnect::App.from_token(self.token) + @app ||= BatchConnect::App.from_token(self.token) end # Attributes used for serialization # @return [Hash] attributes to be serialized def attributes - %w(id cluster_id job_id created_at token title view script_type cache_completed).map do |attribute| + %w(id cluster_id job_id created_at token title script_type cache_completed).map do |attribute| [ attribute, nil ] end.to_h end @@ -99,6 +95,10 @@ def user_context {} end + def view + app.session_view + end + class << self # The data root directory for this namespace # @param token [#to_s] The data root directory for a given app token @@ -236,7 +236,6 @@ def save(app:, context:, format: nil) self.id = SecureRandom.uuid self.token = app.token self.title = app.title - self.view = app.session_view self.created_at = Time.now.to_i self.cluster_id = context.try(:cluster).to_s @@ -260,7 +259,7 @@ def stage(root, context: nil) staged_root.tap { |p| p.mkpath unless p.exist? } # Sync the template files over - oe, s = Open3.capture2e("rsync", "-a", "#{root}/", "#{staged_root}") + oe, s = Open3.capture2e('rsync', '-av', '--exclude', '*.erb', "#{root}/", staged_root.to_s) raise oe unless s.success? # Output user submitted context attributes for debugging purposes @@ -268,7 +267,8 @@ def stage(root, context: nil) # Render all template files using ERB render_erb_files( - template_files, + template_files(root), + root_dir: root, binding: TemplateBinding.new(self, context).get_binding ) true @@ -465,9 +465,10 @@ def staged_root end # List of template files that need to be rendered + # @param dir [#Pathname] the directory where the templates exist # @return [Array] list of template files - def template_files - Pathname.glob(staged_root.join("**", "*.erb")).select(&:file?) + def template_files(dir) + Pathname.glob(dir.join("**", "*.erb")).select(&:file?) end # Path to script that is sourced before main script is forked @@ -596,14 +597,20 @@ def job_name ].reject(&:blank?).join("/") end - # Render a list of files using ERB - def render_erb_files(files, binding: nil, remove_extension: true) - files.each do |file| - rendered_file = remove_extension ? file.sub_ext("") : file + # Render a list of files using ERB. Note the input .erb files are the system + # installed erb files (/var/www/ood/...). These are rendered and output into + # the staged_root. + def render_erb_files(input_files, binding: nil, remove_extension: true, output_dir: staged_root, root_dir: nil) + input_files.each do |file| template = file.read - rendered = ERB.new(template, nil, "-").result(binding) - file.rename rendered_file # keep same file permissions - rendered_file.write(rendered) + rendered = ERB.new(template, trim_mode: "-").result(binding) + + relative_fname = file.to_s.delete_prefix("#{root_dir}/") + output_file = output_dir.join(relative_fname) + output_file = remove_extension ? output_file.sub_ext('') : output_file + + output_file.write(rendered) + output_file.chmod(file.stat.mode) end end end diff --git a/apps/dashboard/app/models/posix_file.rb b/apps/dashboard/app/models/posix_file.rb index cbf225b05b..3543f98119 100644 --- a/apps/dashboard/app/models/posix_file.rb +++ b/apps/dashboard/app/models/posix_file.rb @@ -1,33 +1,16 @@ # PosixFile is a class representing a file on a local file system. class PosixFile - attr_reader :path + attr_reader :path, :stat - delegate :basename, :descend, :parent, :join, :to_s, :read, :write, :mkdir, to: :path + delegate :basename, :descend, :parent, :join, :to_s, :read, :write, :mkdir, :directory?, :realpath, to: :path + + # include to give us number_to_human_size + include ActionView::Helpers::NumberHelper class << self def stat(path) - path = Pathname.new(path) - - # path.stat will not work for a symlink and will raise an exception - # if the directory or file being pointed at does not exist - begin - s = path.stat - rescue - s = path.lstat - end - - { - id: "dev-#{s.dev}-inode-#{s.ino}", - name: path.basename, - size: s.directory? ? nil : s.size, - human_size: s.directory? ? '-' : ::ApplicationController.helpers.number_to_human_size(s.size, precision: 3), - directory: s.directory?, - date: s.mtime.to_i, - owner: username_from_cache(s.uid), - mode: s.mode, - dev: s.dev - } + PosixFile.new(path).to_h end def num_files(from, names) @@ -59,6 +42,31 @@ def initialize(path) # accepts both String and Pathname # avoids converting to Pathname in every function @path = Pathname.new(path) + begin + @stat = @path.lstat + rescue Errno::ENOENT, Errno::EACCES + @stat = nil + end + end + + def to_h + return { name: basename } if stat.nil? + + { + id: "dev-#{stat.dev}-inode-#{stat.ino}", + name: basename, + size: directory? ? nil : stat.size, + human_size: human_size, + directory: directory?, + date: stat.mtime.to_i, + owner: PosixFile.username_from_cache(stat.uid), + mode: stat.mode, + dev: stat.dev + } + end + + def human_size + directory? ? '-' : number_to_human_size(stat.size, precision: 3) end def raise_if_cant_access_directory_contents @@ -68,28 +76,48 @@ def raise_if_cant_access_directory_contents end #FIXME: a more generic name for this? - FileToZip = Struct.new(:path, :relative_path) + FileToZip = Struct.new(:path, :relative_path, :realpath) + + PATHS_TO_FILTER = ['.', '..'].freeze def files_to_zip expanded = path.expand_path - expanded.glob('**/*').map { |p| - FileToZip.new(p.to_s, p.relative_path_from(expanded).to_s) - } - end - - def directory? - path.stat.directory? + expanded.glob('**/*', File::FNM_DOTMATCH).reject do |p| + PATHS_TO_FILTER.include?(p.basename.to_s) + end.select do |path| + AllowlistPolicy.default.permitted?(path.realpath.to_s) + end.map do |path| + FileToZip.new(path.to_s, path.relative_path_from(expanded).to_s, path.realpath.to_s) + end end def ls path.each_child.map do |child_path| - PosixFile.stat(child_path) - end.select do |stats| - valid_encoding = stats[:name].to_s.valid_encoding? - Rails.logger.warn("Not showing file '#{stats[:name]}' because it is not a UTF-8 filename.") unless valid_encoding - valid_encoding - end.sort_by { |p| p[:directory] ? 0 : 1 } + PosixFile.new(child_path) + end.select(&:valid?) + .map(&:to_h) + .sort_by { |p| p[:directory] ? 0 : 1 } + end + + def valid? + valid_realpath? && valid_encoding? + end + + def valid_realpath? + return false if stat.nil? || !path.exist? + + if stat.symlink? + AllowlistPolicy.default.permitted?(realpath) && AllowlistPolicy.default.permitted?(path) + else + AllowlistPolicy.default.permitted?(path) + end + end + + def valid_encoding? + valid = basename.to_s.valid_encoding? + Rails.logger.warn("Not showing file '#{stats[:name]}' because it is not a UTF-8 filename.") unless valid + valid end def editable? @@ -125,7 +153,7 @@ def can_download_as_zip?(timeout: Configuration.file_download_dir_timeout, downl can_download = false error = nil - if ! (path.directory? && path.readable? && path.executable?) + if ! (directory? && path.readable? && path.executable?) error = I18n.t('dashboard.files_directory_download_unauthorized') else # Determine the size of the directory. diff --git a/apps/dashboard/app/views/layouts/editor.html.erb b/apps/dashboard/app/views/layouts/editor.html.erb index b3ca3a2e63..be980b6335 100644 --- a/apps/dashboard/app/views/layouts/editor.html.erb +++ b/apps/dashboard/app/views/layouts/editor.html.erb @@ -104,6 +104,7 @@ + diff --git a/apps/dashboard/config/initializers/upgrade_to_2.1.rb b/apps/dashboard/config/initializers/upgrade_to_3.0.rb similarity index 82% rename from apps/dashboard/config/initializers/upgrade_to_2.1.rb rename to apps/dashboard/config/initializers/upgrade_to_3.0.rb index 214ab2dd3f..eec51b08e0 100644 --- a/apps/dashboard/config/initializers/upgrade_to_2.1.rb +++ b/apps/dashboard/config/initializers/upgrade_to_3.0.rb @@ -2,11 +2,14 @@ Rails.application.config.after_initialize do # Since https://github.com/OSC/ondemand/pull/1526 all the batch connect cache files - # have moved. So, when folks upgrade to 2.1, let's sync these old files so that + # have moved. So, when folks upgrade to 3.0, let's sync these old files so that # they don't lose their cached choices. old_context_files = "#{Configuration.dataroot}/batch_connect/**/*/context.json" cache_root = BatchConnect::Session.cache_root + # kick out if you've already done this + next if Dir.glob("#{cache_root}/*.json").size.positive? + Dir.glob(old_context_files).map do |old_file| new_filename = old_file.gsub(%r{.*/batch_connect/}, '').gsub('/context.json', '').gsub('/', '_') new_filename = "#{new_filename}.json" diff --git a/apps/dashboard/config/initializers/validate_send_files.rb b/apps/dashboard/config/initializers/validate_send_files.rb new file mode 100644 index 0000000000..131cc705e3 --- /dev/null +++ b/apps/dashboard/config/initializers/validate_send_files.rb @@ -0,0 +1,21 @@ +Rails.application.config.after_initialize do + module ActionDispatch + class Response + class FileBody + + # Stream the file's contents if Rack::Sendfile isn't present, + # which it isn't for FilesController#fs. + def each + File.open(to_path, "rb") do |file| + real_path = File.readlink("/proc/self/fd/#{file.fileno}") + return '' unless AllowlistPolicy.default.permitted?(real_path) + + while chunk = file.read(16_384) + yield chunk + end + end + end + end + end + end +end diff --git a/apps/dashboard/test/application_system_test_case.rb b/apps/dashboard/test/application_system_test_case.rb index 726e0cac50..ea5d26af8e 100644 --- a/apps/dashboard/test/application_system_test_case.rb +++ b/apps/dashboard/test/application_system_test_case.rb @@ -7,6 +7,7 @@ class ApplicationSystemTestCase < ActionDispatch::SystemTestCase options.logging_prefs = { browser: 'ALL' } end + Selenium::WebDriver.logger.level = :debug unless ENV['DEBUG'].nil? Capybara.server = :webrick def find_option_style(ele, opt) diff --git a/apps/dashboard/test/models/batch_connect/session_test.rb b/apps/dashboard/test/models/batch_connect/session_test.rb index 241b079d09..8e0990ef33 100644 --- a/apps/dashboard/test/models/batch_connect/session_test.rb +++ b/apps/dashboard/test/models/batch_connect/session_test.rb @@ -526,7 +526,6 @@ def completed? 'created_at' => now.to_i, 'token' => 'bc_jupyter', 'title' => 'Jupyter Notebook', - 'view' => nil, 'script_type' => 'basic', 'cache_completed' => nil } diff --git a/apps/dashboard/test/models/files_test.rb b/apps/dashboard/test/models/files_test.rb index ff6040028e..d16e672d36 100644 --- a/apps/dashboard/test/models/files_test.rb +++ b/apps/dashboard/test/models/files_test.rb @@ -56,4 +56,65 @@ class FilesTest < ActiveSupport::TestCase test "Ensuring PosixFile.username(uid) returns string" do assert_equal "9999999", PosixFile.username(9999999) end -end \ No newline at end of file + + test 'files not in allowlist are invalid' do + Dir.mktmpdir do |dir| + with_modified_env({ OOD_ALLOWLIST_PATH: "#{dir}/allowed" }) do + `mkdir -p #{dir}/allowed` + `mkdir -p #{dir}/not_allowed` + invalid_file = "#{dir}/not_allowed/actual_file" + `touch #{invalid_file}` + + file = PosixFile.new(invalid_file) + refute(file.valid?) + end + end + end + + test 'symlinks outside allowlist are invalid' do + Dir.mktmpdir do |dir| + with_modified_env({ OOD_ALLOWLIST_PATH: "#{dir}/allowed" }) do + `mkdir -p #{dir}/allowed` + `mkdir -p #{dir}/not_allowed` + real_file = "#{dir}/not_allowed/actual_file" + symlink = "#{dir}/allowed/linked_file" + `touch #{real_file}` + `ln -s #{real_file} #{symlink}` + + real_file = PosixFile.new(real_file) + symlink = PosixFile.new(symlink) + + # both are invalid because they're not in the allowlist + refute(symlink.valid?) + refute(real_file.valid?) + end + end + end + + test 'ls output does not show invalid files' do + Dir.mktmpdir do |dir| + with_modified_env({ OOD_ALLOWLIST_PATH: "#{dir}/allowed" }) do + `mkdir -p #{dir}/allowed` + `mkdir -p #{dir}/not_allowed` + real_file = "#{dir}/not_allowed/actual_file" + symlink = "#{dir}/allowed/linked_file" + `touch #{real_file}` + + # symlink resolves outside of allowlist + `ln -s #{real_file} #{symlink}` + + # this file would be allowed - but is a broken symlink + `touch #{dir}/allowed/tmp` + `ln -s #{dir}/allowed/tmp #{dir}/allowed/broken_symlink` + `rm #{dir}/allowed/tmp` + + # they exist, but cannot be seen + assert_equal(1, PosixFile.new(dir).ls.size) + assert_equal(2, Dir.children(dir).size) + + assert_equal(0, PosixFile.new("#{dir}/allowed").ls.size) + assert_equal(2, Dir.children("#{dir}/allowed").size) + end + end + end +end diff --git a/apps/dashboard/test/system/files_test.rb b/apps/dashboard/test/system/files_test.rb index 9428cac0b7..648b28654e 100644 --- a/apps/dashboard/test/system/files_test.rb +++ b/apps/dashboard/test/system/files_test.rb @@ -290,4 +290,129 @@ class FilesTest < ApplicationSystemTestCase end end end + + test 'symlinked directories' do + Dir.mktmpdir do |dir| + `cd #{dir}; mkdir -p #{dir} real_dir` + `cd #{dir}; touch #{dir} real_dir/real_file` + `cd #{dir}; ln -s real_dir linked_dir` + + visit files_url(dir) + + find('tbody a', exact_text: 'real_dir') + find('tbody a', exact_text: 'linked_dir') + assert_selector('tbody span[title="directory"]', count: 2) + + # click the symlink + find('tbody a', exact_text: 'linked_dir').click + + find('tbody a', exact_text: 'real_file', wait: MAX_WAIT) + assert_selector('tbody span[title="file"]', count: 1) + end + end + + test 'symlinked files outside of allowlist' do + Dir.mktmpdir do |dir| + allowed_dir = "#{dir}/allowed" + with_modified_env({ OOD_ALLOWLIST_PATH: allowed_dir }) do + `mkdir -p #{allowed_dir}` + `mkdir -p #{allowed_dir}/some_dir` + `touch #{allowed_dir}/some_file` + + `mkdir -p #{dir}/not_allowed` + `ln -s #{dir}/not_allowed #{allowed_dir}/symlinked_dir` + + visit files_url(allowed_dir) + + # 3 things are actually in the directory + assert_equal(3, Dir.children(allowed_dir).size) + + # but only 2 things are shown in the UI (symlinked_dir is missing) + find('tbody a', exact_text: 'some_dir') + find('tbody a', exact_text: 'some_file') + assert_selector('tbody span[title="directory"]', count: 1) + assert_selector('tbody span[title="file"]', count: 1) + end + end + end + + test 'can download hidden files and directories' do + zip_file = Rails.root.join('test_dir.zip') + File.delete(zip_file) if File.exist?(zip_file) + + Dir.mktmpdir do |dir| + dir_to_dl = "#{dir}/test_dir" + `mkdir -p #{dir_to_dl}/first_level_dir` + `mkdir #{dir_to_dl}/.first_level_hidden_dir` + `touch #{dir_to_dl}/real_file` + `touch #{dir_to_dl}/first_level_dir/.second_level_hidden_file` + `touch #{dir_to_dl}/first_level_dir/second_level_real_file` + `touch #{dir_to_dl}/.first_level_hidden_dir/.another_second_level_hidden_file` + `touch #{dir_to_dl}/.first_level_hidden_dir/another_second_level_real_file` + + visit files_url(dir) + find('tbody a', exact_text: 'test_dir').ancestor('tr').click + click_on('Download') + + sleep 5 # give it enough time to download + assert(File.exist?(zip_file), "#{zip_file} was never downloaded!") + + # unzip all the files you downloaded into a new tmp directory and iterate + # though them. Verify that the file you downloaded exists in the original tmpdir 'dir'. + Dir.mktmpdir do |unzip_tmp_dir| + `cd #{unzip_tmp_dir}; unzip #{zip_file}` + Dir.glob("#{dir_to_dl}/**/*", File::FNM_DOTMATCH).reject do |file_to_dl| + ['.', '..'].freeze.include?(File.basename(file_to_dl)) + + # get the relative path + end.map do |path_to_dl| + path_to_dl.gsub(dir_to_dl, '').delete_prefix('/') + + # now combine the relative path with the new unzipped directory and verify that + # the file exists in the unzipped directory + end.each do |relative_path_to_dl| + + assert(File.exist?("#{unzip_tmp_dir}/#{relative_path_to_dl}"), "#{relative_path_to_dl} was not downloaded!") + end + end + + File.delete(zip_file) if File.exist?(zip_file) + end + end + + test 'cannot download files outside of allowlist' do + zip_file = Rails.root.join('allowed.zip') + File.delete(zip_file) if File.exist?(zip_file) + + Dir.mktmpdir do |dir| + allowed_dir = "#{dir}/allowed" + with_modified_env({ OOD_ALLOWLIST_PATH: dir }) do + `mkdir -p #{allowed_dir}/real_directory` + `touch #{allowed_dir}/real_file` + `touch #{allowed_dir}/real_directory/other_real_file` + `ln -s #{Rails.root.join('README.md')} #{allowed_dir}/sym_linked_file` + `ln -s #{Rails.root} #{allowed_dir}/sym_linked_directory` + + visit files_url(dir) + find('tbody a', exact_text: 'allowed').ancestor('tr').click + click_on('Download') + + sleep 5 # give it enough time to download + assert(File.exist?(zip_file), "#{zip_file} was never downloaded!") + + Dir.mktmpdir do |unzip_tmp_dir| + `cd #{unzip_tmp_dir}; unzip #{zip_file}` + assert(File.exist?("#{unzip_tmp_dir}/real_directory")) + assert(File.directory?("#{unzip_tmp_dir}/real_directory")) + assert(File.exist?("#{unzip_tmp_dir}/real_directory/other_real_file")) + assert(File.exist?("#{unzip_tmp_dir}/real_file")) + + refute(File.exist?("#{unzip_tmp_dir}/sym_linked_file")) + refute(File.exist?("#{unzip_tmp_dir}/sym_linked_directory")) + end + end + end + + File.delete(zip_file) if File.exist?(zip_file) + end end