add restrictions to downloads too.

OSC · Oct 3, 2023 · f394d95 · f394d95
1 parent 5cb090c
commit f394d95
Show file tree

Hide file tree

Showing 3 changed files with 46 additions and 6 deletions.
diff --git a/apps/dashboard/app/controllers/files_controller.rb b/apps/dashboard/app/controllers/files_controller.rb
@@ -47,16 +47,18 @@ def fs
             response.sending_file = true
             response.cache_control[:public] ||= false
 
+            # raise StandardError, @path.files_to_zip
+
             # FIXME: strategy 1: is below, use zip_tricks
             # strategy 2: use actual zip command (likely much faster) and ActionController::Live
             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
@@ -76,7 +76,7 @@ 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
 
@@ -85,8 +85,10 @@ def files_to_zip
 
     expanded.glob('**/*', File::FNM_DOTMATCH).reject do |p|
       PATHS_TO_FILTER.include?(p.basename.to_s)
-    end.map do |p|
-      FileToZip.new(p.to_s, p.relative_path_from(expanded).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
 

diff --git a/apps/dashboard/test/system/files_test.rb b/apps/dashboard/test/system/files_test.rb
@@ -379,4 +379,40 @@ class FilesTest < ApplicationSystemTestCase
       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