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

Improve performance #437

Merged
merged 19 commits into from
Aug 1, 2023
Merged
Show file tree
Hide file tree
Changes from 14 commits
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
3 changes: 3 additions & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@ gemspec :name => 'gollum-lib'
gem 'irb'

if RUBY_PLATFORM == 'java' then
gem 'gollum-rjgit_adapter', git: 'https://github.com/dometto/gollum-lib_rjgit_adapter', branch: 'tree_find_blob'
dometto marked this conversation as resolved.
Show resolved Hide resolved
group :development do
gem 'activesupport', '~> 6.0'
end
else
gem 'gollum-rugged_adapter', git: 'https://github.com/gollum/rugged_adapter'
end
55 changes: 33 additions & 22 deletions lib/gollum-lib/file.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,19 @@ class File

class << self

# For use with self.find: returns true if the given query corresponds to the in-repo path of the BlobEntry.
#
# query - The String path to match.
# entry - The BlobEntry to check against.
# global_match - (Not implemented for File, see Page.path_match)
# hyphened_tags - If true, replace spaces in match_path with hyphens.
# case_insensitive - If true, compare query and match_path case-insensitively
def path_match(query, entry, global_match = false, hyphened_tags = false, case_insensitive = false)
path_compare(query, ::File.join('/', entry.path), hyphened_tags, case_insensitive)
end
# Get a canonical path to a file.
# Ensures that the result is always under page_file_dir (prevents path traversal), if set.
dometto marked this conversation as resolved.
Show resolved Hide resolved
# Removes leading slashes.
#
# path - One or more String path elements to join together. `nil` values are ignored.
# page_file_dir - kwarg String, default: nil
def canonical_path(*path, page_file_dir: nil)
dometto marked this conversation as resolved.
Show resolved Hide resolved
prefix = Pathname.new(::File.join('/', page_file_dir.to_s))
dometto marked this conversation as resolved.
Show resolved Hide resolved
rest = Pathname.new(::File.join('/', path.compact)).cleanpath
dometto marked this conversation as resolved.
Show resolved Hide resolved
result = Pathname.new(::File.join(prefix, rest)).cleanpath.to_s[1..-1]
dometto marked this conversation as resolved.
Show resolved Hide resolved
result.sub!(/^\/+/, '') if Gem.win_platform? # On Windows, Pathname#cleanpath will leave double slashes at the start of a path, so replace all (not just the first) leading slashes
result
end

# For use with self.path_match: returns true if 'query' and 'match_path' match, strictly or taking account of the following parameters:
# hyphened_tags - If true, replace spaces in match_path with hyphens.
Expand All @@ -41,24 +44,32 @@ def path_compare(query, match_path, hyphened_tags, case_insensitive)
# version - The String version ID to find.
# try_on_disk - If true, try to return just a reference to a file
# that exists on the disk.
# global_match - If true, find a File matching path's filename, but not it's directory (so anywhere in the repo)
# global_match - If true, find a File matching path's filename, but not its directory (so anywhere in the repo)
#
# Returns a Gollum::File or nil if the file could not be found. Note
# that if you specify try_on_disk=true, you may or may not get a file
# for which on_disk? is actually true.
def self.find(wiki, path, version, try_on_disk = false, global_match = false)
map = wiki.tree_map_for(version.to_s)

query_path = Pathname.new(::File.join(['/', wiki.page_file_dir, path].compact)).cleanpath.to_s
query_path.sub!(/^\/\//, '/') if Gem.win_platform? # On Windows, Pathname#cleanpath will leave double slashes at the start of a path intact, so sub them out.
query_path = self.canonical_path(path, page_file_dir: wiki.page_file_dir)
bartkamphorst marked this conversation as resolved.
Show resolved Hide resolved
path_segments = query_path.split('/')
dometto marked this conversation as resolved.
Show resolved Hide resolved
filename = path_segments.last
dir = path_segments[0..-2].join('/')

begin
entry = map.detect do |entry|
path_match(query_path, entry, global_match, wiki.hyphened_tag_lookup, wiki.case_insensitive_tag_lookup)
if global_match && self.respond_to?(:global_find) # Only implemented for Gollum::Page
return self.global_find(wiki, version, query_path, try_on_disk)
else
begin
dometto marked this conversation as resolved.
Show resolved Hide resolved
root = wiki.commit_for(version.to_s)
dometto marked this conversation as resolved.
Show resolved Hide resolved
return nil unless root
tree = dir.empty? ? root.tree : root.tree / dir
return nil unless tree
dometto marked this conversation as resolved.
Show resolved Hide resolved
entry = tree.find_blob do |blob_name|
path_compare(filename, blob_name, wiki.hyphened_tag_lookup, wiki.case_insensitive_tag_lookup)
end
entry ? self.new(wiki, entry, dir, version, try_on_disk) : nil
rescue Gollum::Git::NoSuchShaFound
nil
end
entry ? self.new(wiki, entry.blob(wiki.repo), entry.dir, version, try_on_disk) : nil
rescue Gollum::Git::NoSuchShaFound
nil
end
end

Expand All @@ -74,7 +85,7 @@ def self.find(wiki, path, version, try_on_disk = false, global_match = false)
def initialize(wiki, blob, path, version, try_on_disk = false)
@wiki = wiki
@blob = blob
@path = "#{path}/#{blob.name}"[1..-1]
@path = self.class.canonical_path(path, blob.name)
@version = version.is_a?(Gollum::Git::Commit) ? version : @wiki.commit_for(version)
get_disk_reference if try_on_disk
end
Expand Down
10 changes: 3 additions & 7 deletions lib/gollum-lib/git_access.rb
Original file line number Diff line number Diff line change
Expand Up @@ -161,15 +161,11 @@ def tree!(sha)
items = []
tree.each do |entry|
if entry[:type] == 'blob'
items << BlobEntry.new(entry[:sha], entry[:path], entry[:size], entry[:mode].to_i(8))
next if @page_file_dir && !entry[:path].start_with?("#{@page_file_dir}/")
dometto marked this conversation as resolved.
Show resolved Hide resolved
items << BlobEntry.new(entry[:sha], entry[:path], entry[:size], entry[:mode])
end
end
if (dir = @page_file_dir)
regex = /^#{dir}\//
items.select { |i| i.path =~ regex }
else
items
end
items
end

# Reads the content from the Git db at the given SHA.
Expand Down
29 changes: 23 additions & 6 deletions lib/gollum-lib/page.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,35 @@ class << self
#
# query - The String path to match.
# entry - The BlobEntry to check against.
# global_match - If true, find a File matching path's filename, but not its directory (so anywhere in the repo)
def path_match(query, entry, global_match = false, hyphened_tags = false, case_insensitive = false)
def global_path_match(query, entry, hyphened_tags = false, case_insensitive = false)
dometto marked this conversation as resolved.
Show resolved Hide resolved
return false if "#{entry.name}".empty?
return false unless valid_extension?(entry.name)
entry_name = valid_extension?(query) ? entry.name : strip_filename(entry.name)
match_path = ::File.join([
'/',
global_match ? nil : entry.dir,
entry_name
'/',
entry.dir,
entry.name
dometto marked this conversation as resolved.
Show resolved Hide resolved
].compact)
path_compare(query, match_path, hyphened_tags, case_insensitive)
end

def global_find(wiki, version, query, try_on_disk)
bartkamphorst marked this conversation as resolved.
Show resolved Hide resolved
map = wiki.tree_map_for(version.to_s)
begin
entry = map.detect do |entry|
global_path_match(query, entry, wiki.hyphened_tag_lookup, wiki.case_insensitive_tag_lookup)
end
entry ? self.new(wiki, entry.blob(wiki.repo), entry.dir, version, try_on_disk) : nil
rescue Gollum::Git::NoSuchShaFound
nil
end
end

def path_compare(query, match_path, hyphened_tags, case_insensitive)
return false unless valid_extension?(match_path)
cmp = valid_extension?(query) ? match_path : strip_filename(match_path)
super(query, cmp, hyphened_tags, case_insensitive)
end

end

# Checks if a filename has a valid, registered extension
Expand Down
11 changes: 11 additions & 0 deletions test/test_file.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,17 @@
@wiki = Gollum::Wiki.new(testpath("examples/lotr.git"))
end

test 'canonical_path method does not allow path traversal exploit' do
page_file_dir = nil
dir = 'pages'
file = '../../'
assert_equal Gollum::File.canonical_path(dir, file, page_file_dir: page_file_dir), ''
page_file_dir = 'pages'
assert_equal Gollum::File.canonical_path(dir, file, page_file_dir: page_file_dir), 'pages'
# also removes leading slashes
assert_equal Gollum::File.canonical_path('/foo/bar', page_file_dir: page_file_dir), 'pages/foo/bar'
end

test "existing file" do
commit = @wiki.repo.commits.first
file = @wiki.file("Mordor/todo.txt")
Expand Down