diff options
author | Markus Koller <mkoller@gitlab.com> | 2020-08-07 17:46:35 +0300 |
---|---|---|
committer | Markus Koller <mkoller@gitlab.com> | 2020-08-21 18:03:09 +0300 |
commit | 40bb9a9e9ba9b9b243f63ca80fa8889f4d41b0ce (patch) | |
tree | bf1d5d5c9597c4076f3c1778315493e6b68d858f | |
parent | 257ad7fbd53c6d2b2906f7129326e1302032e43f (diff) |
Don't expand filesystem paths of wiki pages
Paths starting with a `~` tilde character were getting expanded by the
call to `File.expand_path` in `BlobEntry.normalize_dir`. This can cause
an exception when the tilde is followed by an invalid username, which
makes the whole wiki unusable.
This overrides `BlobEntry.normalize_dir` so it doesn't expand tildes
anymore, and in our case we also don't need to handle symlinks or
Windows paths.
-rw-r--r-- | changelogs/unreleased/dont-expand-wiki-paths.yml | 5 | ||||
-rw-r--r-- | internal/service/wiki/find_page_test.go | 71 | ||||
-rw-r--r-- | ruby/lib/gitlab/gollum.rb | 27 | ||||
-rw-r--r-- | ruby/spec/lib/gitlab/gollum_spec.rb | 32 |
4 files changed, 133 insertions, 2 deletions
diff --git a/changelogs/unreleased/dont-expand-wiki-paths.yml b/changelogs/unreleased/dont-expand-wiki-paths.yml new file mode 100644 index 000000000..80855164e --- /dev/null +++ b/changelogs/unreleased/dont-expand-wiki-paths.yml @@ -0,0 +1,5 @@ +--- +title: Don't expand filesystem paths of wiki pages +merge_request: +author: +type: security diff --git a/internal/service/wiki/find_page_test.go b/internal/service/wiki/find_page_test.go index 4d5ab7249..1591ffb53 100644 --- a/internal/service/wiki/find_page_test.go +++ b/internal/service/wiki/find_page_test.go @@ -25,12 +25,19 @@ func TestSuccessfulWikiFindPageRequest(t *testing.T) { page3Name := "Installing/Step 133-c" page4Name := "Encoding is fun" page5Name := "Empty file" + page6Name := "~/Tilde in directory" + page7Name := "~Tilde in filename" + page8Name := "~!/Tilde with invalid user" + page1Commit := createTestWikiPage(t, client, wikiRepo, createWikiPageOpts{title: page1Name}) createTestWikiPage(t, client, wikiRepo, createWikiPageOpts{title: page2Name}) createTestWikiPage(t, client, wikiRepo, createWikiPageOpts{title: page3Name}) createTestWikiPage(t, client, wikiRepo, createWikiPageOpts{title: page4Name, content: []byte("f\xFCr")}) - page5Commit := createTestWikiPage(t, client, wikiRepo, createWikiPageOpts{title: page5Name, forceContentEmpty: true}) - latestCommit := page5Commit + createTestWikiPage(t, client, wikiRepo, createWikiPageOpts{title: page5Name, forceContentEmpty: true}) + createTestWikiPage(t, client, wikiRepo, createWikiPageOpts{title: page6Name}) + createTestWikiPage(t, client, wikiRepo, createWikiPageOpts{title: page7Name}) + page8Commit := createTestWikiPage(t, client, wikiRepo, createWikiPageOpts{title: page8Name}) + latestCommit := page8Commit testCases := []struct { desc string @@ -158,6 +165,66 @@ func TestSuccessfulWikiFindPageRequest(t *testing.T) { }, expectedContent: nil, }, + { + desc: "title for file with tilde in directory", + request: &gitalypb.WikiFindPageRequest{ + Repository: wikiRepo, + Title: []byte(page6Name), + }, + expectedPage: &gitalypb.WikiPage{ + Version: &gitalypb.WikiPageVersion{ + Commit: latestCommit, + Format: "markdown", + }, + Title: []byte("Tilde in directory"), + Format: "markdown", + UrlPath: "~/Tilde-in-directory", + Path: []byte("~/Tilde-in-directory.md"), + Name: []byte("Tilde in directory"), + Historical: false, + }, + expectedContent: mockPageContent, + }, + { + desc: "title for file with tilde in filename", + request: &gitalypb.WikiFindPageRequest{ + Repository: wikiRepo, + Title: []byte(page7Name), + }, + expectedPage: &gitalypb.WikiPage{ + Version: &gitalypb.WikiPageVersion{ + Commit: latestCommit, + Format: "markdown", + }, + Title: []byte(page7Name), + Format: "markdown", + UrlPath: "~Tilde-in-filename", + Path: []byte("~Tilde-in-filename.md"), + Name: []byte(page7Name), + Historical: false, + }, + expectedContent: mockPageContent, + }, + { + desc: "title for file with tilde and invalid user", + request: &gitalypb.WikiFindPageRequest{ + Repository: wikiRepo, + Title: []byte(page8Name), + }, + expectedPage: &gitalypb.WikiPage{ + Version: &gitalypb.WikiPageVersion{ + Commit: latestCommit, + Format: "markdown", + }, + Title: []byte("Tilde with invalid user"), + Format: "markdown", + UrlPath: "~!/Tilde-with-invalid-user", + Path: []byte("~!/Tilde-with-invalid-user.md"), + Name: []byte("Tilde with invalid user"), + Historical: false, + }, + expectedContent: mockPageContent, + }, } for _, testCase := range testCases { diff --git a/ruby/lib/gitlab/gollum.rb b/ruby/lib/gitlab/gollum.rb index 4bf08411f..cd9016dd8 100644 --- a/ruby/lib/gitlab/gollum.rb +++ b/ruby/lib/gitlab/gollum.rb @@ -17,4 +17,31 @@ module Gollum Gitlab::EncodingHelper.encode!(data) end end + + # Override BlobEntry.normalize_dir to remove the call to File.expand_path, + # which also expands `~` and `~username` paths, and can raise exceptions for + # invalid users. + # + # We don't need to worry about symlinks or Windows paths, we only need to + # normalize the slashes in the path, and return an empty string for toplevel + # paths. + class BlobEntry + def self.normalize_dir(dir) + # Return empty string for nil and paths that point to the toplevel + # ('.', '/', '..' etc.) + return '' if !dir || dir =~ %r{\A[\./]*\z} + + # Normalize the path: + # - Add exactly one leading slash + # - Remove trailing slashes + # - Remove repeated slashes + dir.sub(%r{ + \A + /* # leading slashes + (?<path>.*?) # the actual path + /* # trailing slashes + \z + }x, '/\k<path>').gsub(%r{//+}, '/') + end + end end diff --git a/ruby/spec/lib/gitlab/gollum_spec.rb b/ruby/spec/lib/gitlab/gollum_spec.rb new file mode 100644 index 000000000..0ee994c3d --- /dev/null +++ b/ruby/spec/lib/gitlab/gollum_spec.rb @@ -0,0 +1,32 @@ +require 'spec_helper' + +describe Gollum do + describe Gollum::BlobEntry do + describe '.normalize_dir' do + it 'returns the path with normalized slashes' do + expect(described_class.normalize_dir('foo/bar')).to eq('/foo/bar') + expect(described_class.normalize_dir('/foo/bar')).to eq('/foo/bar') + expect(described_class.normalize_dir('/foo/bar/')).to eq('/foo/bar') + expect(described_class.normalize_dir('//foo//bar//')).to eq('/foo/bar') + end + + it 'returns an empty string for toplevel paths' do + expect(described_class.normalize_dir(nil)).to eq('') + expect(described_class.normalize_dir('')).to eq('') + expect(described_class.normalize_dir('.')).to eq('') + expect(described_class.normalize_dir('..')).to eq('') + expect(described_class.normalize_dir('/')).to eq('') + expect(described_class.normalize_dir('//')).to eq('') + expect(described_class.normalize_dir(' ')).to eq('/ ') + expect(described_class.normalize_dir("\t")).to eq("/\t") + end + + it 'does not expand tilde characters' do + expect(described_class.normalize_dir('~/foo')).to eq('/~/foo') + expect(described_class.normalize_dir('~root/foo')).to eq('/~root/foo') + expect(described_class.normalize_dir('~!/foo')).to eq('/~!/foo') + expect(described_class.normalize_dir('foo/~')).to eq('/foo/~') + end + end + end +end |