add stronger validations on symlinks (OSC#3057)

add stronger validations on symlinks

apps/dashboard/app/models/posix_file.rb

@@ -3,7 +3,7 @@ class PosixFile
 
   attr_reader :path, :stat
 
-  delegate :basename, :descend, :parent, :join, :to_s, :read, :write, :mkdir, :directory?, 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
@@ -95,7 +95,17 @@ def ls
   end
 
   def valid?
-    valid_encoding?
+    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?

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
+
+  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

apps/dashboard/test/system/files_test.rb

@@ -310,4 +310,29 @@ class FilesTest < ApplicationSystemTestCase
       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
 end