Merge branch 'fix/encoding-issues-in-wiki-rpcs' into 'master'

Convert inputs to UTF-8 before passing them to Gollum

See merge request gitlab-org/gitaly!575

12 files changed, 82 insertions, 42 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index fe8328883..848a6cabc 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,8 @@
 
 UNRELEASED
 
+- Convert inputs to UTF-8 before passing them to Gollum
+  https://gitlab.com/gitlab-org/gitaly/merge_requests/575
 - Implement OperationService.UserSquash RPC
   https://gitlab.com/gitlab-org/gitaly/merge_requests/548
 - Update recommended and minimum git versions to 2.14.3 and 2.9.0 respectively
diff --git a/internal/service/wiki/delete_page_test.go b/internal/service/wiki/delete_page_test.go
index 4edfa200d..417c4a0e3 100644
--- a/internal/service/wiki/delete_page_test.go
+++ b/internal/service/wiki/delete_page_test.go
@@ -25,7 +25,7 @@ func TestSuccessfulWikiDeletePageRequest(t *testing.T) {
 	client, conn := newWikiClient(t, serverSocketPath)
 	defer conn.Close()
 
-	pageName := "A tale of two wikis"
+	pageName := "A talé of two wikis"
 	authorName := []byte("Ahmad Sherif")
 	authorEmail := []byte("ahmad@gitlab.com")
 	message := []byte("Delete " + pageName)
@@ -35,7 +35,7 @@ func TestSuccessfulWikiDeletePageRequest(t *testing.T) {
 
 	request := &pb.WikiDeletePageRequest{
 		Repository: wikiRepo,
-		PagePath:   []byte("a-tale-of-two-wikis"),
+		PagePath:   []byte("a-talé-of-two-wikis"),
 		CommitDetails: &pb.WikiCommitDetails{
 			Name:    authorName,
 			Email:   authorEmail,
diff --git a/internal/service/wiki/find_file_test.go b/internal/service/wiki/find_file_test.go
index d7d6c4eb7..3ef73d97b 100644
--- a/internal/service/wiki/find_file_test.go
+++ b/internal/service/wiki/find_file_test.go
@@ -42,7 +42,7 @@ func TestSuccessfulWikiFindFileRequest(t *testing.T) {
 	content, err := ioutil.ReadFile("testdata/clouds.png")
 	require.NoError(t, err)
 
-	err = ioutil.WriteFile(path.Join(sandboxWikiPath, "clouds.png"), content, 0777)
+	err = ioutil.WriteFile(path.Join(sandboxWikiPath, "cloúds.png"), content, 0777)
 	require.NoError(t, err)
 
 	// Sandbox wiki is empty, so we create a commit to be used later
@@ -61,9 +61,9 @@ func TestSuccessfulWikiFindFileRequest(t *testing.T) {
 	newHeadID := testhelper.MustRunCommand(t, nil, "git", "-C", sandboxWikiPath, "show", "--format=format:%H", "--no-patch", "HEAD")
 
 	response := &pb.WikiFindFileResponse{
-		Name:     []byte("clouds.png"),
+		Name:     []byte("cloúds.png"),
 		MimeType: "image/png",
-		Path:     []byte("clouds.png"),
+		Path:     []byte("cloúds.png"),
 	}
 
 	testCases := []struct {
@@ -75,7 +75,7 @@ func TestSuccessfulWikiFindFileRequest(t *testing.T) {
 			desc: "name only",
 			request: &pb.WikiFindFileRequest{
 				Repository: sandboxWiki,
-				Name:       []byte("clouds.png"),
+				Name:       []byte("cloúds.png"),
 			},
 			response: response,
 		},
@@ -83,7 +83,7 @@ func TestSuccessfulWikiFindFileRequest(t *testing.T) {
 			desc: "name + revision that includes the file",
 			request: &pb.WikiFindFileRequest{
 				Repository: sandboxWiki,
-				Name:       []byte("clouds.png"),
+				Name:       []byte("cloúds.png"),
 				Revision:   newHeadID,
 			},
 			response: response,
@@ -92,7 +92,7 @@ func TestSuccessfulWikiFindFileRequest(t *testing.T) {
 			desc: "name + revision that does not include the file",
 			request: &pb.WikiFindFileRequest{
 				Repository: sandboxWiki,
-				Name:       []byte("clouds.png"),
+				Name:       []byte("cloúds.png"),
 				Revision:   oldHeadID,
 			},
 			response: &pb.WikiFindFileResponse{},
diff --git a/internal/service/wiki/find_page_test.go b/internal/service/wiki/find_page_test.go
index b8cf9920e..c630085b0 100644
--- a/internal/service/wiki/find_page_test.go
+++ b/internal/service/wiki/find_page_test.go
@@ -22,8 +22,8 @@ func TestSuccessfulWikiFindPageRequest(t *testing.T) {
 	client, conn := newWikiClient(t, serverSocketPath)
 	defer conn.Close()
 
-	page1Name := "Home Page"
-	page2Name := "Installing/Step 133-b"
+	page1Name := "Home Pagé"
+	page2Name := "Instálling/Step 133-b"
 	page3Name := "Installing/Step 133-c"
 	page1Commit := createTestWikiPage(t, client, wikiRepo, createWikiPageOpts{title: page1Name})
 	createTestWikiPage(t, client, wikiRepo, createWikiPageOpts{title: page2Name})
@@ -47,8 +47,8 @@ func TestSuccessfulWikiFindPageRequest(t *testing.T) {
 				},
 				Title:      []byte(page1Name),
 				Format:     "markdown",
-				UrlPath:    "Home-Page",
-				Path:       []byte("Home-Page.md"),
+				UrlPath:    "Home-Pagé",
+				Path:       []byte("Home-Pagé.md"),
 				Name:       []byte(page1Name),
 				Historical: false,
 			},
@@ -67,8 +67,8 @@ func TestSuccessfulWikiFindPageRequest(t *testing.T) {
 				},
 				Title:      []byte(page1Name),
 				Format:     "markdown",
-				UrlPath:    "Home-Page",
-				Path:       []byte("Home-Page.md"),
+				UrlPath:    "Home-Pagé",
+				Path:       []byte("Home-Pagé.md"),
 				Name:       []byte(page1Name),
 				Historical: true,
 			},
@@ -87,7 +87,7 @@ func TestSuccessfulWikiFindPageRequest(t *testing.T) {
 			request: &pb.WikiFindPageRequest{
 				Repository: wikiRepo,
 				Title:      []byte("Step 133-b"),
-				Directory:  []byte("Installing"),
+				Directory:  []byte("Instálling"),
 			},
 			expectedPage: &pb.WikiPage{
 				Version: &pb.WikiPageVersion{
@@ -96,8 +96,8 @@ func TestSuccessfulWikiFindPageRequest(t *testing.T) {
 				},
 				Title:      []byte("Step 133 b"),
 				Format:     "markdown",
-				UrlPath:    "Installing/Step-133-b",
-				Path:       []byte("Installing/Step-133-b.md"),
+				UrlPath:    "Instálling/Step-133-b",
+				Path:       []byte("Instálling/Step-133-b.md"),
 				Name:       []byte("Step 133 b"),
 				Historical: false,
 			},
diff --git a/internal/service/wiki/formatted_data_test.go b/internal/service/wiki/formatted_data_test.go
index 03f60a16e..fecd8caef 100644
--- a/internal/service/wiki/formatted_data_test.go
+++ b/internal/service/wiki/formatted_data_test.go
@@ -26,8 +26,8 @@ func TestSuccessfulWikiGetFormattedDataRequest(t *testing.T) {
 	format := "rdoc"
 	content := bytes.Repeat([]byte("*bold*\n\n"), 10000)
 	expectedContent := bytes.Repeat([]byte("\n<p><strong>bold</strong></p>\n"), 10000)
-	page1Name := "Home Page"
-	page2Name := "Installing/Step 133-b"
+	page1Name := "Home Pagé"
+	page2Name := "Instálling/Step 133-b"
 	page3Name := "Installing/Step 133-c"
 	page1Commit := createTestWikiPage(t, client, wikiRepo, createWikiPageOpts{title: page1Name, format: format, content: content})
 	createTestWikiPage(t, client, wikiRepo, createWikiPageOpts{title: page2Name, format: format, content: content})
@@ -57,7 +57,7 @@ func TestSuccessfulWikiGetFormattedDataRequest(t *testing.T) {
 			request: &pb.WikiGetFormattedDataRequest{
 				Repository: wikiRepo,
 				Title:      []byte("Step 133-b"),
-				Directory:  []byte("Installing"),
+				Directory:  []byte("Instálling"),
 			},
 		},
 	}
diff --git a/internal/service/wiki/get_page_versions_test.go b/internal/service/wiki/get_page_versions_test.go
index abb332741..8bcd3fa8c 100644
--- a/internal/service/wiki/get_page_versions_test.go
+++ b/internal/service/wiki/get_page_versions_test.go
@@ -26,7 +26,7 @@ func TestWikiGetPageVersionsRequest(t *testing.T) {
 
 	client, conn := newWikiClient(t, serverSocketPath)
 	defer conn.Close()
-	pageTitle := "WikiGetPageVersions"
+	pageTitle := "WikiGétPageVersions"
 
 	content := bytes.Repeat([]byte("Mock wiki page content"), 10000)
 	writeWikiPage(t, client, wikiRepo, createWikiPageOpts{title: pageTitle, content: content})
@@ -62,8 +62,8 @@ func TestWikiGetPageVersionsRequest(t *testing.T) {
 				{
 					Commit: &pb.GitCommit{
 						Id:        strings.TrimRight(string(v2cid), "\n"),
-						Body:      []byte("Update WikiGetPageVersions"),
-						Subject:   []byte("Update WikiGetPageVersions"),
+						Body:      []byte("Update WikiGétPageVersions"),
+						Subject:   []byte("Update WikiGétPageVersions"),
 						Author:    gitAuthor,
 						Committer: gitAuthor,
 						ParentIds: []string{strings.TrimRight(string(v1cid), "\n")},
@@ -73,8 +73,8 @@ func TestWikiGetPageVersionsRequest(t *testing.T) {
 				{
 					Commit: &pb.GitCommit{
 						Id:        strings.TrimRight(string(v1cid), "\n"),
-						Body:      []byte("Add WikiGetPageVersions"),
-						Subject:   []byte("Add WikiGetPageVersions"),
+						Body:      []byte("Add WikiGétPageVersions"),
+						Subject:   []byte("Add WikiGétPageVersions"),
 						Author:    gitAuthor,
 						Committer: gitAuthor,
 					},
diff --git a/internal/service/wiki/update_page_test.go b/internal/service/wiki/update_page_test.go
index 884a3450d..302d142da 100644
--- a/internal/service/wiki/update_page_test.go
+++ b/internal/service/wiki/update_page_test.go
@@ -26,7 +26,7 @@ func TestSuccessfulWikiUpdatePageRequest(t *testing.T) {
 	client, conn := newWikiClient(t, serverSocketPath)
 	defer conn.Close()
 
-	writeWikiPage(t, client, wikiRepo, createWikiPageOpts{title: "Installing Gitaly", content: []byte("foobar")})
+	writeWikiPage(t, client, wikiRepo, createWikiPageOpts{title: "Instálling Gitaly", content: []byte("foobar")})
 
 	content := bytes.Repeat([]byte("Mock wiki page content"), 10000)
 
@@ -36,8 +36,8 @@ func TestSuccessfulWikiUpdatePageRequest(t *testing.T) {
 
 	request := &pb.WikiUpdatePageRequest{
 		Repository: wikiRepo,
-		PagePath:   []byte("//Installing Gitaly"),
-		Title:      []byte("Installing Gitaly"),
+		PagePath:   []byte("//Instálling Gitaly"),
+		Title:      []byte("Instálling Gitaly"),
 		Format:     "markdown",
 		CommitDetails: &pb.WikiCommitDetails{
 			Name:    authorName,
@@ -76,7 +76,7 @@ func TestSuccessfulWikiUpdatePageRequest(t *testing.T) {
 	require.Equal(t, authorEmail, commit.Author.Email, "author email mismatched")
 	require.Equal(t, message, commit.Subject, "message mismatched")
 
-	pageContent := testhelper.MustRunCommand(t, nil, "git", "-C", wikiRepoPath, "cat-file", "blob", "HEAD:Installing-Gitaly.md")
+	pageContent := testhelper.MustRunCommand(t, nil, "git", "-C", wikiRepoPath, "cat-file", "blob", "HEAD:Instálling-Gitaly.md")
 	require.Equal(t, content, pageContent, "mismatched content")
 }
 
diff --git a/internal/service/wiki/write_page_test.go b/internal/service/wiki/write_page_test.go
index dacbe82b7..2da1d5fef 100644
--- a/internal/service/wiki/write_page_test.go
+++ b/internal/service/wiki/write_page_test.go
@@ -34,7 +34,7 @@ func TestSuccessfulWikiWritePageRequest(t *testing.T) {
 
 	request := &pb.WikiWritePageRequest{
 		Repository: wikiRepo,
-		Name:       []byte("Installing Gitaly"),
+		Name:       []byte("Instálling Gitaly"),
 		Format:     "markdown",
 		CommitDetails: &pb.WikiCommitDetails{
 			Name:    authorName,
@@ -73,7 +73,7 @@ func TestSuccessfulWikiWritePageRequest(t *testing.T) {
 	require.Equal(t, authorEmail, commit.Author.Email, "author email mismatched")
 	require.Equal(t, message, commit.Subject, "message mismatched")
 
-	pageContent := testhelper.MustRunCommand(t, nil, "git", "-C", wikiRepoPath, "cat-file", "blob", "HEAD:Installing-Gitaly.md")
+	pageContent := testhelper.MustRunCommand(t, nil, "git", "-C", wikiRepoPath, "cat-file", "blob", "HEAD:Instálling-Gitaly.md")
 	require.Equal(t, content, pageContent, "mismatched content")
 }
 
diff --git a/ruby/lib/gitaly_server/utils.rb b/ruby/lib/gitaly_server/utils.rb
index c7b971d69..e316bb1e1 100644
--- a/ruby/lib/gitaly_server/utils.rb
+++ b/ruby/lib/gitaly_server/utils.rb
@@ -28,6 +28,16 @@ module GitalyServer
       )
     end
 
+    def set_utf8!(str)
+      raise ArgumentError unless str.respond_to?(:force_encoding)
+      return str if str.encoding == Encoding::UTF_8 && str.valid_encoding?
+
+      str = str.dup if str.respond_to?(:frozen?) && str.frozen?
+
+      str.force_encoding('UTF-8')
+      str.valid_encoding? ? str : raise(ArgumentError, "string is not valid UTF-8: #{str.inspect}")
+    end
+
     def bridge_exceptions
       yield
     rescue GRPC::BadStatus => e
diff --git a/ruby/lib/gitaly_server/wiki_service.rb b/ruby/lib/gitaly_server/wiki_service.rb
index b2c07fb8f..bba0f812a 100644
--- a/ruby/lib/gitaly_server/wiki_service.rb
+++ b/ruby/lib/gitaly_server/wiki_service.rb
@@ -6,7 +6,7 @@ module GitalyServer
       bridge_exceptions do
         repo = Gitlab::Git::Repository.from_gitaly(request.repository, call)
         wiki = Gitlab::Git::Wiki.new(repo)
-        page_path = request.page_path
+        page_path = set_utf8!(request.page_path)
         commit_details = commit_details_from_gitaly(request.commit_details)
 
         wiki.delete_page(page_path, commit_details)
@@ -24,7 +24,7 @@ module GitalyServer
           call.each_remote_read.with_index do |request, index|
             if index.zero?
               repo = Gitlab::Git::Repository.from_gitaly(request.repository, call)
-              name = request.name
+              name = set_utf8!(request.name)
               format = request.format
               commit_details = request.commit_details
             end
@@ -50,9 +50,9 @@ module GitalyServer
         wiki = Gitlab::Git::Wiki.new(repo)
 
         page = wiki.page(
-          title: request.title,
+          title: set_utf8!(request.title),
           version: request.revision.presence,
-          dir: request.directory.presence
+          dir: set_utf8!(request.directory).presence
         )
 
         unless page
@@ -129,7 +129,7 @@ module GitalyServer
         repo = Gitlab::Git::Repository.from_gitaly(request.repository, call)
         wiki = Gitlab::Git::Wiki.new(repo)
 
-        file = wiki.file(request.name, request.revision.presence)
+        file = wiki.file(set_utf8!(request.name), request.revision.presence)
 
         unless file
           return Enumerator.new do |y|
@@ -140,7 +140,7 @@ module GitalyServer
         response = Gitaly::WikiFindFileResponse.new(
           name: file.name.b,
           mime_type: file.mime_type,
-          path: file.path
+          path: file.path.b
         )
 
         Enumerator.new do |y|
@@ -160,7 +160,7 @@ module GitalyServer
       bridge_exceptions do
         repo = Gitlab::Git::Repository.from_gitaly(request.repository, call)
         wiki = Gollum::Wiki.new(repo.path)
-        path = request.page_path
+        path = set_utf8!(request.page_path)
 
         page = wiki.paged(Gollum::Page.canonicalize_filename(path), File.split(path).first)
 
@@ -198,8 +198,8 @@ module GitalyServer
           if index.zero?
             repo = Gitlab::Git::Repository.from_gitaly(request.repository, call)
             wiki = Gitlab::Git::Wiki.new(repo)
-            title = request.title
-            page_path = request.page_path
+            title = set_utf8!(request.title)
+            page_path = set_utf8!(request.page_path)
             format = request.format
 
             commit_details = commit_details_from_gitaly(request.commit_details)
@@ -220,9 +220,9 @@ module GitalyServer
         wiki = Gitlab::Git::Wiki.new(repo)
 
         page = wiki.page(
-          title: request.title,
+          title: set_utf8!(request.title),
           version: request.revision.presence,
-          dir: request.directory.presence
+          dir: set_utf8!(request.directory).presence
         )
 
         raise GRPC::NotFound unless page
diff --git a/ruby/spec/lib/gitaly_server/utils_spec.rb b/ruby/spec/lib/gitaly_server/utils_spec.rb
new file mode 100644
index 000000000..b3c16610d
--- /dev/null
+++ b/ruby/spec/lib/gitaly_server/utils_spec.rb
@@ -0,0 +1,27 @@
+require 'spec_helper'
+
+describe GitalyServer::Utils do
+  let(:cls) do
+    Class.new do
+      include GitalyServer::Utils
+    end
+  end
+
+  describe '.set_utf8!' do
+    context 'valid encoding' do
+      it 'returns a UTF-8 string' do
+        str = 'écoles'
+
+        expect(cls.new.set_utf8!(str.b)).to eq(str)
+      end
+    end
+
+    context 'invalid encoding' do
+      it 'returns a UTF-8 string' do
+        str = "\xA9coles".b
+
+        expect { cls.new.set_utf8!(str) }.to raise_error(ArgumentError)
+      end
+    end
+  end
+end
diff --git a/ruby/spec/spec_helper.rb b/ruby/spec/spec_helper.rb
index 2480c77c7..4d25e7245 100644
--- a/ruby/spec/spec_helper.rb
+++ b/ruby/spec/spec_helper.rb
@@ -1,2 +1,3 @@
+require_relative '../lib/gitaly_server.rb'
 require_relative '../lib/gitlab/git.rb'
 require 'test_repo_helper'