Merge branch 'strip-invalid-characters-in-signature' into 'master'

Strip invalid characters in signatures on commit

See merge request gitlab-org/gitaly!1709

4 files changed, 129 insertions, 0 deletions

diff --git a/changelogs/unreleased/strip-invalid-characters-in-signature.yml b/changelogs/unreleased/strip-invalid-characters-in-signature.yml
new file mode 100644
index 000000000..71cdcab8e
--- /dev/null
+++ b/changelogs/unreleased/strip-invalid-characters-in-signature.yml
@@ -0,0 +1,5 @@
+---
+title: Strip invalid characters in signatures on commit
+merge_request: 1709
+author:
+type: fixed
diff --git a/internal/service/operations/commit_files_test.go b/internal/service/operations/commit_files_test.go
index 3541a302e..213d9e37a 100644
--- a/internal/service/operations/commit_files_test.go
+++ b/internal/service/operations/commit_files_test.go
@@ -332,6 +332,63 @@ func TestSuccessfulUserCommitFilesRequestStartShaRemoteRepository(t *testing.T)
 	require.Equal(t, newTargetBranchCommit.ParentIds, []string{startCommit.Id})
 }
 
+func TestSuccessfulUserCommitFilesRequestWithSpecialCharactersInSignature(t *testing.T) {
+	server, serverSocketPath := runFullServer(t)
+	defer server.Stop()
+
+	client, conn := operations.NewOperationClient(t, serverSocketPath)
+	defer conn.Close()
+
+	testRepo, _, cleanupFn := testhelper.InitBareRepo(t)
+	defer cleanupFn()
+
+	ctxOuter, cancel := testhelper.Context()
+	defer cancel()
+
+	md := testhelper.GitalyServersMetadata(t, serverSocketPath)
+	targetBranchName := "master"
+
+	testCases := []struct {
+		desc   string
+		user   *gitalypb.User
+		author *gitalypb.CommitAuthor // expected value
+	}{
+		{
+			desc:   "special characters at start and end",
+			user:   &gitalypb.User{Name: []byte(".,:;<>\"'\nJane Doe.,:;<>'\"\n"), Email: []byte(".,:;<>'\"\njanedoe@gitlab.com.,:;<>'\"\n")},
+			author: &gitalypb.CommitAuthor{Name: []byte("Jane Doe"), Email: []byte("janedoe@gitlab.com")},
+		},
+		{
+			desc:   "special characters in the middle",
+			user:   &gitalypb.User{Name: []byte("Ja<ne\n D>oe"), Email: []byte("ja<ne\ndoe>@gitlab.com")},
+			author: &gitalypb.CommitAuthor{Name: []byte("Jane Doe"), Email: []byte("janedoe@gitlab.com")},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			ctx := metadata.NewOutgoingContext(ctxOuter, md)
+			headerRequest := headerRequest(testRepo, tc.user, targetBranchName, commitFilesMessage)
+			setAuthorAndEmail(headerRequest, tc.user.Name, tc.user.Email)
+
+			stream, err := client.UserCommitFiles(ctx)
+			require.NoError(t, err)
+			require.NoError(t, stream.Send(headerRequest))
+
+			_, err = stream.CloseAndRecv()
+			require.NoError(t, err)
+
+			newCommit, err := log.GetCommit(ctxOuter, testRepo, targetBranchName)
+			require.NoError(t, err)
+
+			require.Equal(t, tc.author.Name, newCommit.Author.Name, "author name")
+			require.Equal(t, tc.author.Email, newCommit.Author.Email, "author email")
+			require.Equal(t, tc.author.Name, newCommit.Committer.Name, "committer name")
+			require.Equal(t, tc.author.Email, newCommit.Committer.Email, "committer email")
+		})
+	}
+}
+
 func TestFailedUserCommitFilesRequestDueToHooks(t *testing.T) {
 	server, serverSocketPath := runFullServer(t)
 	defer server.Stop()
@@ -487,6 +544,26 @@ func TestFailedUserCommitFilesRequest(t *testing.T) {
 			desc: "invalid commit ID: \"foobar\"",
 			req:  setStartSha(headerRequest(testRepo, user, branchName, commitFilesMessage), "foobar"),
 		},
+		{
+			desc: "failed to parse signature - Signature cannot have an empty name or email",
+			req:  headerRequest(testRepo, &gitalypb.User{}, branchName, commitFilesMessage),
+		},
+		{
+			desc: "failed to parse signature - Signature cannot have an empty name or email",
+			req:  headerRequest(testRepo, &gitalypb.User{Name: []byte(""), Email: []byte("")}, branchName, commitFilesMessage),
+		},
+		{
+			desc: "failed to parse signature - Signature cannot have an empty name or email",
+			req:  headerRequest(testRepo, &gitalypb.User{Name: []byte(" "), Email: []byte(" ")}, branchName, commitFilesMessage),
+		},
+		{
+			desc: "failed to parse signature - Signature cannot have an empty name or email",
+			req:  headerRequest(testRepo, &gitalypb.User{Name: []byte("Jane Doe"), Email: []byte("")}, branchName, commitFilesMessage),
+		},
+		{
+			desc: "failed to parse signature - Signature cannot have an empty name or email",
+			req:  headerRequest(testRepo, &gitalypb.User{Name: []byte(""), Email: []byte("janedoe@gitlab.com")}, branchName, commitFilesMessage),
+		},
 	}
 
 	for _, tc := range testCases {
diff --git a/ruby/lib/gitlab/git.rb b/ruby/lib/gitlab/git.rb
index 8c041e534..f1ed275f6 100644
--- a/ruby/lib/gitlab/git.rb
+++ b/ruby/lib/gitlab/git.rb
@@ -72,6 +72,12 @@ module Gitlab
       def committer_hash(email:, name:)
         return if email.nil? || name.nil?
 
+        # Git strips newlines and angle brackets silently.
+        # libgit2/Rugged doesn't, but aborts if angle brackets are present.
+        # See upstream issue https://github.com/libgit2/libgit2/issues/5342
+        email = email.delete("\n<>")
+        name = name.delete("\n<>")
+
         {
           email: email,
           name: name,
diff --git a/ruby/spec/lib/gitlab/git_spec.rb b/ruby/spec/lib/gitlab/git_spec.rb
new file mode 100644
index 000000000..2128225a5
--- /dev/null
+++ b/ruby/spec/lib/gitlab/git_spec.rb
@@ -0,0 +1,41 @@
+require 'spec_helper'
+
+describe Gitlab::Git do
+  let(:committer_email) { 'user@example.org' }
+  let(:committer_name) { 'John Doe' }
+
+  describe 'committer_hash' do
+    it "returns a hash containing the given email and name" do
+      committer_hash = described_class.committer_hash(email: committer_email, name: committer_name)
+
+      expect(committer_hash[:email]).to eq(committer_email)
+      expect(committer_hash[:name]).to eq(committer_name)
+      expect(committer_hash[:time]).to be_a(Time)
+    end
+
+    context 'when email is nil' do
+      it "returns nil" do
+        committer_hash = described_class.committer_hash(email: nil, name: committer_name)
+
+        expect(committer_hash).to be_nil
+      end
+    end
+
+    context 'when name is nil' do
+      it "returns nil" do
+        committer_hash = described_class.committer_hash(email: committer_email, name: nil)
+
+        expect(committer_hash).to be_nil
+      end
+    end
+
+    context 'when email or name contains angle brackets or newlines' do
+      it 'strips invalid characters' do
+        committer_hash = described_class.committer_hash(email: "<foo>\n", name: "f<o>\no")
+
+        expect(committer_hash[:email]).to eq('foo')
+        expect(committer_hash[:name]).to eq('foo')
+      end
+    end
+  end
+end