From e78b2dfc5961bd21f8e614d939ac80cc681ee3ef Mon Sep 17 00:00:00 2001 From: Robin Karlsson Date: Thu, 19 Sep 2024 10:21:59 +0300 Subject: [PATCH 1/2] Optimize allowlist handling - Return early if allowlist is disabled. - Handle child path evaluation as strings. --- apps/dashboard/app/models/allowlist_policy.rb | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/apps/dashboard/app/models/allowlist_policy.rb b/apps/dashboard/app/models/allowlist_policy.rb index d8470ef5e0..0cc751d130 100644 --- a/apps/dashboard/app/models/allowlist_policy.rb +++ b/apps/dashboard/app/models/allowlist_policy.rb @@ -17,10 +17,11 @@ def initialize(allowlist) # @raises ArgumentError if any allowlist path or permitted? argument # has the form ~user/some/path where user doesn't exist def permitted?(path) + return true if allowlist.blank? real_path = real_expanded_path(path.to_s) key = path_to_key(real_path) Rails.cache.fetch(key) do - allowlist.blank? || allowlist.any? { |parent| child?(Pathname.new(parent), real_path) } + allowlist.any? { |parent| child?(Pathname.new(parent), real_path) } end end @@ -71,19 +72,19 @@ def real_expanded_path_not_exist(path) Pathname.new(path).expand_path end - # Determine if child is a subpath of parent - # - # If the relative path from the child to the supposed parent includes '..' - # then child is not a subpath of parent + # Determine if child is a subpath of parent. + # This does not access the filesystem, so symlinks should be evaluated before this. # # @param parent [Pathname] # @param child [Pathname] # @return Boolean def child?(parent, child) - !child.expand_path.relative_path_from( - parent.expand_path - ).each_filename.to_a.include?( - '..' - ) + # Expand both paths to evaluate any potential ".." components to be able to compare them as strings. + p = parent.expand_path.to_s + c = child.expand_path.to_s + # Child path shorter than parent => not a child. + return false if c.size < p.size + # Child path is same as parent path, or has additional components after parent path (has "/" after parent path). + c[0...p.size] == p && (c.size == p.size || c[p.size] == "/") end end From 52dc17f29d55e871e1502f0193ef7125a3dcaabc Mon Sep 17 00:00:00 2001 From: Robin Karlsson Date: Fri, 20 Sep 2024 09:04:53 +0300 Subject: [PATCH 2/2] Use start_with? --- apps/dashboard/app/models/allowlist_policy.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/apps/dashboard/app/models/allowlist_policy.rb b/apps/dashboard/app/models/allowlist_policy.rb index 0cc751d130..eb6f668c18 100644 --- a/apps/dashboard/app/models/allowlist_policy.rb +++ b/apps/dashboard/app/models/allowlist_policy.rb @@ -82,9 +82,7 @@ def child?(parent, child) # Expand both paths to evaluate any potential ".." components to be able to compare them as strings. p = parent.expand_path.to_s c = child.expand_path.to_s - # Child path shorter than parent => not a child. - return false if c.size < p.size - # Child path is same as parent path, or has additional components after parent path (has "/" after parent path). - c[0...p.size] == p && (c.size == p.size || c[p.size] == "/") + # Child path if it is same as parent path, or has additional components after "/". + c.start_with?(p) && (c.size == p.size || c[p.size] == "/") end end