Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize allowlist handling #3804

Merged
merged 2 commits into from
Sep 20, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 11 additions & 10 deletions apps/dashboard/app/models/allowlist_policy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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] == "/")
Copy link
Contributor

@johrstrom johrstrom Sep 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has also bee no my mind lately. Isn't this whole expression c.start_with?(p)? Looks like start_with is a c function so it may be even faster.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is true indeed. Didn't seem to be faster, but it is simpler for sure.

end
end
Loading