Commit signature parsing must consume all data

Commit with no LF byte at the end of the commit message.
`ReadBytes` returns `io.EOF` and all data it read until
EOF. This data must also be taken into account as part of
commit message.

Fixes: https://gitlab.com/gitlab-org/gitaly/-/issues/2545

6 files changed, 68 insertions, 3 deletions

diff --git a/changelogs/unreleased/ps-2545-commit-signature-parsing.yml b/changelogs/unreleased/ps-2545-commit-signature-parsing.yml
new file mode 100644
index 000000000..9de2c7bf1
--- /dev/null
+++ b/changelogs/unreleased/ps-2545-commit-signature-parsing.yml
@@ -0,0 +1,5 @@
+---
+title: Commit signature parsing must consume all data
+merge_request: 1953
+author:
+type: fixed
diff --git a/internal/service/commit/commit_signatures.go b/internal/service/commit/commit_signatures.go
index 283860322..6b9d3ee4b 100644
--- a/internal/service/commit/commit_signatures.go
+++ b/internal/service/commit/commit_signatures.go
@@ -69,6 +69,7 @@ func extractSignature(reader io.Reader) ([]byte, []byte, error) {
 		line, err := bufferedReader.ReadBytes('\n')
 
 		if err == io.EOF {
+			commitText = append(commitText, line...)
 			break
 		}
 		if err != nil {
diff --git a/internal/service/commit/commit_signatures_test.go b/internal/service/commit/commit_signatures_test.go
index 09493afa1..11ae70b56 100644
--- a/internal/service/commit/commit_signatures_test.go
+++ b/internal/service/commit/commit_signatures_test.go
@@ -1,10 +1,12 @@
 package commit
 
 import (
+	"bytes"
 	"io"
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
@@ -17,9 +19,13 @@ func TestSuccessfulGetCommitSignaturesRequest(t *testing.T) {
 	client, conn := newCommitServiceClient(t, serverSocketPath)
 	defer conn.Close()
 
-	testRepo, _, cleanupFn := testhelper.NewTestRepo(t)
+	testRepo, repoPath, cleanupFn := testhelper.NewTestRepo(t)
 	defer cleanupFn()
 
+	commitData := testhelper.MustReadFile(t, "testdata/dc00eb001f41dfac08192ead79c2377c588b82ee.commit")
+	commit := text.ChompBytes(testhelper.MustRunCommand(t, bytes.NewReader(commitData), "git", "-C", repoPath, "hash-object", "-w", "-t", "commit", "--stdin", "--literally"))
+	require.Equal(t, "dc00eb001f41dfac08192ead79c2377c588b82ee", commit)
+
 	ctx, cancel := testhelper.Context()
 	defer cancel()
 
@@ -31,10 +37,11 @@ func TestSuccessfulGetCommitSignaturesRequest(t *testing.T) {
 			"0000000000000000000000000000000000000000", // does not exist
 			"a17a9f66543673edf0a3d1c6b93bdda3fe600f32", // has signature
 			"8cf8e80a5a0546e391823c250f2b26b9cf15ce88", // has signature and commit message > 4MB
+			"dc00eb001f41dfac08192ead79c2377c588b82ee", // has signature and commit message without newline at the end
 		},
 	}
 
-	expectedSignautes := []*gitalypb.GetCommitSignaturesResponse{
+	expectedSignatures := []*gitalypb.GetCommitSignaturesResponse{
 		{
 			CommitId:   "5937ac0a7beb003549fc5fd26fc247adbce4a52e",
 			Signature:  testhelper.MustReadFile(t, "testdata/commit-5937ac0a7beb003549fc5fd26fc247adbce4a52e-signature"),
@@ -50,6 +57,11 @@ func TestSuccessfulGetCommitSignaturesRequest(t *testing.T) {
 			Signature:  testhelper.MustReadFile(t, "testdata/gitaly-test-commit-8cf8e80a5a0546e391823c250f2b26b9cf15ce88-signature"),
 			SignedText: testhelper.MustReadFile(t, "testdata/gitaly-test-commit-8cf8e80a5a0546e391823c250f2b26b9cf15ce88-signed-text"),
 		},
+		{
+			CommitId:   "dc00eb001f41dfac08192ead79c2377c588b82ee",
+			Signature:  testhelper.MustReadFile(t, "testdata/dc00eb001f41dfac08192ead79c2377c588b82ee-signed-no-newline-signature.txt"),
+			SignedText: testhelper.MustReadFile(t, "testdata/dc00eb001f41dfac08192ead79c2377c588b82ee-signed-no-newline-signed-text.txt"),
+		},
 	}
 
 	c, err := client.GetCommitSignatures(ctx, request)
@@ -57,7 +69,7 @@ func TestSuccessfulGetCommitSignaturesRequest(t *testing.T) {
 
 	fetchedSignatures := readAllSignaturesFromClient(t, c)
 
-	require.Equal(t, expectedSignautes, fetchedSignatures)
+	require.Equal(t, expectedSignatures, fetchedSignatures)
 }
 
 func TestFailedGetCommitSignaturesRequest(t *testing.T) {
diff --git a/internal/service/commit/testdata/dc00eb001f41dfac08192ead79c2377c588b82ee-signed-no-newline-signature.txt b/internal/service/commit/testdata/dc00eb001f41dfac08192ead79c2377c588b82ee-signed-no-newline-signature.txt
new file mode 100644
index 000000000..fe83dc676
--- /dev/null
+++ b/internal/service/commit/testdata/dc00eb001f41dfac08192ead79c2377c588b82ee-signed-no-newline-signature.txt
@@ -0,0 +1,17 @@
+-----BEGIN PGP SIGNATURE-----
+Version: ObjectivePGP
+Comment: https://www.objectivepgp.com
+Charset: UTF-8
+
+wsFcBAABCgAGBQJecon1AAoJEDYMjTn1G2THmSsP/At/jskLdF0i7p0nKf4JLjeeqRJ4k2IUg87U
+ZwV6mbLo5XFm8Sq7CJBAGAhlOZE4BAwKALuawmgs5XMEZwK2z6AIgosGTVpmxDTTI11bXt4XIOdz
+qF7c/gUrJOZzjFXOqDsd5UuPRupwznC5eKlLbfImR+NYxKryo8JGdF5t52ph4kChcQsKlSkXuYNI
++9UgbaMclEjb0OLm+mcP9QxW+Cs9JS2Jb4Jh6XONWW1nDN3ZTDDskguIqqF47UxIgSImrmpMcEj9
+YSNU0oMoHM4+1DoXp1t99EGPoAMvO+a5g8gd1jouCIrI6KOX+GeG/TFFM0mQwg/d/N9LR049m8ed
+vgqg/lMiWUxQGL2IPpYPcgiUEqfn7ete+NMzQV5zstxF/q7Yj2BhM2L7FPHxKaoy/w5Q/DcAO4wN
+5gxVmIvbCDk5JOx8I+boIS8ZxSvIlJ5IWaPrcjg5Mc40it+WHvMqxVnCzH0c6KcXaJ2SibVb59HR
+pdRhEXXw/hRN65l/xwyM8sklQalAGu755gNJZ4k9ApBVUssZyiu+te2+bDirAcmK8/x1jvMQY6bn
+DFxBE7bMHDp24IFPaVID84Ryt3vSSBEkrUGm7OkyDESTpHCr4sfD5o3LCUCIibTqv/CAhe59mhbB
+2AXL7X+EzylKy6C1N5KUUiMTW94AuF6f8FqBoxnf
+=U6zM
+-----END PGP SIGNATURE-----
diff --git a/internal/service/commit/testdata/dc00eb001f41dfac08192ead79c2377c588b82ee-signed-no-newline-signed-text.txt b/internal/service/commit/testdata/dc00eb001f41dfac08192ead79c2377c588b82ee-signed-no-newline-signed-text.txt
new file mode 100644
index 000000000..f8cb5ccab
--- /dev/null
+++ b/internal/service/commit/testdata/dc00eb001f41dfac08192ead79c2377c588b82ee-signed-no-newline-signed-text.txt
@@ -0,0 +1,6 @@
+tree 7dc7f517af9a53aa9817a6b8c33dc94371a7cdad
+parent 6041ff4cb12c282b70be566ff3af88a29b01c65a
+author Bug Fixer <bugfixer@email.com> 1584564725 +0100
+committer Bug Fixer <bugfixer@email.com> 1584564725 +0100
+
+message with no newline byte at the end
\ No newline at end of file
diff --git a/internal/service/commit/testdata/dc00eb001f41dfac08192ead79c2377c588b82ee.commit b/internal/service/commit/testdata/dc00eb001f41dfac08192ead79c2377c588b82ee.commit
new file mode 100644
index 000000000..7ca58d9b9
--- /dev/null
+++ b/internal/service/commit/testdata/dc00eb001f41dfac08192ead79c2377c588b82ee.commit
@@ -0,0 +1,24 @@
+tree 7dc7f517af9a53aa9817a6b8c33dc94371a7cdad
+parent 6041ff4cb12c282b70be566ff3af88a29b01c65a
+author Bug Fixer <bugfixer@email.com> 1584564725 +0100
+committer Bug Fixer <bugfixer@email.com> 1584564725 +0100
+gpgsig -----BEGIN PGP SIGNATURE-----
+ Version: ObjectivePGP
+ Comment: https://www.objectivepgp.com
+ Charset: UTF-8
+ 
+ wsFcBAABCgAGBQJecon1AAoJEDYMjTn1G2THmSsP/At/jskLdF0i7p0nKf4JLjeeqRJ4k2IUg87U
+ ZwV6mbLo5XFm8Sq7CJBAGAhlOZE4BAwKALuawmgs5XMEZwK2z6AIgosGTVpmxDTTI11bXt4XIOdz
+ qF7c/gUrJOZzjFXOqDsd5UuPRupwznC5eKlLbfImR+NYxKryo8JGdF5t52ph4kChcQsKlSkXuYNI
+ +9UgbaMclEjb0OLm+mcP9QxW+Cs9JS2Jb4Jh6XONWW1nDN3ZTDDskguIqqF47UxIgSImrmpMcEj9
+ YSNU0oMoHM4+1DoXp1t99EGPoAMvO+a5g8gd1jouCIrI6KOX+GeG/TFFM0mQwg/d/N9LR049m8ed
+ vgqg/lMiWUxQGL2IPpYPcgiUEqfn7ete+NMzQV5zstxF/q7Yj2BhM2L7FPHxKaoy/w5Q/DcAO4wN
+ 5gxVmIvbCDk5JOx8I+boIS8ZxSvIlJ5IWaPrcjg5Mc40it+WHvMqxVnCzH0c6KcXaJ2SibVb59HR
+ pdRhEXXw/hRN65l/xwyM8sklQalAGu755gNJZ4k9ApBVUssZyiu+te2+bDirAcmK8/x1jvMQY6bn
+ DFxBE7bMHDp24IFPaVID84Ryt3vSSBEkrUGm7OkyDESTpHCr4sfD5o3LCUCIibTqv/CAhe59mhbB
+ 2AXL7X+EzylKy6C1N5KUUiMTW94AuF6f8FqBoxnf
+ =U6zM
+ -----END PGP SIGNATURE-----
+ 
+
+message with no newline byte at the end
\ No newline at end of file