Merge branch 'security-207-dont-expand-wiki-paths-13-1' into '13-1-stable'

Don't expand filesystem paths of wiki pages

See merge request gitlab-org/security/gitaly!14

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