From 4977d26715e23fe81518283caf1587538b1c7c5f Mon Sep 17 00:00:00 2001 From: Jeff Ohrstrom Date: Tue, 3 Oct 2023 17:00:10 -0400 Subject: [PATCH] download hidden paths (#3096) add the ability to download hidden files and folders and add restrictions on downloads too. --- .../app/controllers/files_controller.rb | 6 +- apps/dashboard/app/models/posix_file.rb | 14 +++- .../test/application_system_test_case.rb | 1 + apps/dashboard/test/system/files_test.rb | 80 +++++++++++++++++++ 4 files changed, 94 insertions(+), 7 deletions(-) diff --git a/apps/dashboard/app/controllers/files_controller.rb b/apps/dashboard/app/controllers/files_controller.rb index 4eeb31cf79..f737a4a57b 100644 --- a/apps/dashboard/app/controllers/files_controller.rb +++ b/apps/dashboard/app/controllers/files_controller.rb @@ -54,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) diff --git a/apps/dashboard/app/models/posix_file.rb b/apps/dashboard/app/models/posix_file.rb index fb617becd4..3543f98119 100644 --- a/apps/dashboard/app/models/posix_file.rb +++ b/apps/dashboard/app/models/posix_file.rb @@ -76,14 +76,20 @@ 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) - } + 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 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/system/files_test.rb b/apps/dashboard/test/system/files_test.rb index e439620b15..648b28654e 100644 --- a/apps/dashboard/test/system/files_test.rb +++ b/apps/dashboard/test/system/files_test.rb @@ -335,4 +335,84 @@ class FilesTest < ApplicationSystemTestCase 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