Merge remote-tracking branch 'dev/13-12-stable' into 13-12-stable

8 files changed, 97 insertions, 32 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index d7b8dd0cf..543ad70fe 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,11 @@
 # Gitaly changelog
 
+## 13.12.9 (2021-08-03)
+
+### Security (1 change)
+
+- [catfile: Allow parsing of long git commit headers](gitlab-org/security/gitaly@ae22c42551963b5fde79f55fd4f89c136a3d8dbb) ([merge request](gitlab-org/security/gitaly!38))
+
 ## 13.12.8 (2021-07-07)
 
 No changes.
diff --git a/VERSION b/VERSION
index 21edccba2..01452f6e2 100644
--- a/VERSION
+++ b/VERSION
@@ -1 +1 @@
-13.12.8
\ No newline at end of file
+13.12.9
\ No newline at end of file
diff --git a/internal/git/catfile/commit.go b/internal/git/catfile/commit.go
index 5b53e5545..bf4d80389 100644
--- a/internal/git/catfile/commit.go
+++ b/internal/git/catfile/commit.go
@@ -81,14 +81,6 @@ func GetCommitMessage(ctx context.Context, c Batch, repo repository.GitRepo, rev
 	return body, nil
 }
 
-func parseRawCommit(r io.Reader, info *ObjectInfo) (*gitalypb.GitCommit, error) {
-	header, body, err := splitRawCommit(r)
-	if err != nil {
-		return nil, err
-	}
-	return buildCommit(header, body, info)
-}
-
 func splitRawCommit(r io.Reader) ([]byte, []byte, error) {
 	raw, err := ioutil.ReadAll(r)
 	if err != nil {
@@ -106,24 +98,32 @@ func splitRawCommit(r io.Reader) ([]byte, []byte, error) {
 	return header, body, nil
 }
 
-func buildCommit(header, body []byte, info *ObjectInfo) (*gitalypb.GitCommit, error) {
-	commit := &gitalypb.GitCommit{
-		Id:       info.Oid.String(),
-		BodySize: int64(len(body)),
-		Body:     body,
-		Subject:  subjectFromBody(body),
-	}
+func parseRawCommit(r io.Reader, info *ObjectInfo) (*gitalypb.GitCommit, error) {
+	commit := &gitalypb.GitCommit{Id: info.Oid.String()}
 
-	if max := helper.MaxCommitOrTagMessageSize; len(body) > max {
-		commit.Body = commit.Body[:max]
-	}
+	var lastLine bool
+	b := bufio.NewReader(r)
+
+	for !lastLine {
+		line, err := b.ReadString('\n')
+		if err == io.EOF {
+			lastLine = true
+		} else if err != nil {
+			return nil, fmt.Errorf("parse raw commit: header: %w", err)
+		}
 
-	scanner := bufio.NewScanner(bytes.NewReader(header))
-	for scanner.Scan() {
-		line := scanner.Text()
 		if len(line) == 0 || line[0] == ' ' {
 			continue
 		}
+		// A blank line indicates the start of the commit body
+		if line == "\n" {
+			break
+		}
+
+		// There might not be a final line break if there was an EOF
+		if line[len(line)-1] == '\n' {
+			line = line[:len(line)-1]
+		}
 
 		headerSplit := strings.SplitN(line, " ", 2)
 		if len(headerSplit) != 2 {
@@ -143,8 +143,19 @@ func buildCommit(header, body []byte, info *ObjectInfo) (*gitalypb.GitCommit, er
 			commit.TreeId = headerSplit[1]
 		}
 	}
-	if err := scanner.Err(); err != nil {
-		return nil, err
+
+	body, err := ioutil.ReadAll(b)
+	if err != nil {
+		return nil, fmt.Errorf("parse raw commit: body: %w", err)
+	}
+
+	if len(body) > 0 {
+		commit.Subject = subjectFromBody(body)
+		commit.BodySize = int64(len(body))
+		commit.Body = body
+		if max := helper.MaxCommitOrTagMessageSize; len(body) > max {
+			commit.Body = commit.Body[:max]
+		}
 	}
 
 	return commit, nil
diff --git a/internal/git/catfile/commit_test.go b/internal/git/catfile/commit_test.go
index ffb3c5a9e..6c2ebe1c4 100644
--- a/internal/git/catfile/commit_test.go
+++ b/internal/git/catfile/commit_test.go
@@ -93,6 +93,16 @@ func TestParseRawCommit(t *testing.T) {
 				},
 			},
 		},
+		{
+			desc: "huge",
+			in:   append([]byte("author "), bytes.Repeat([]byte("A"), 100000)...),
+			out: &gitalypb.GitCommit{
+				Id: info.Oid.String(),
+				Author: &gitalypb.CommitAuthor{
+					Name: bytes.Repeat([]byte("A"), 100000),
+				},
+			},
+		},
 	}
 
 	for _, tc := range testCases {
diff --git a/internal/git/gittest/command.go b/internal/git/gittest/command.go
index f658b90cd..aea0661bf 100644
--- a/internal/git/gittest/command.go
+++ b/internal/git/gittest/command.go
@@ -39,9 +39,10 @@ func run(t testing.TB, stdin io.Reader, cfg config.Cfg, args ...string) []byte {
 
 	output, err := cmd.Output()
 	if err != nil {
-		stderr := err.(*exec.ExitError).Stderr
 		t.Log(cfg.Git.BinPath, args)
-		t.Logf("%s: %s\n", stderr, output)
+		if ee, ok := err.(*exec.ExitError); ok {
+			t.Logf("%s: %s\n", ee.Stderr, output)
+		}
 		t.Fatal(err)
 	}
 
diff --git a/internal/git/gittest/commit.go b/internal/git/gittest/commit.go
index 8bd960931..da6c18297 100644
--- a/internal/git/gittest/commit.go
+++ b/internal/git/gittest/commit.go
@@ -21,10 +21,11 @@ const (
 )
 
 type writeCommitConfig struct {
-	branch      string
-	parents     []git.ObjectID
-	message     string
-	treeEntries []TreeEntry
+	branch        string
+	parents       []git.ObjectID
+	committerName string
+	message       string
+	treeEntries   []TreeEntry
 }
 
 // WriteCommitOption is an option which can be passed to WriteCommit.
@@ -66,6 +67,13 @@ func WithTreeEntries(entries ...TreeEntry) WriteCommitOption {
 	}
 }
 
+// WithCommitterName is an option for WriteCommit which will set the committer name.
+func WithCommitterName(name string) WriteCommitOption {
+	return func(cfg *writeCommitConfig) {
+		cfg.committerName = name
+	}
+}
+
 // WriteCommit writes a new commit into the target repository.
 func WriteCommit(t testing.TB, cfg config.Cfg, repoPath string, opts ...WriteCommitOption) git.ObjectID {
 	t.Helper()
@@ -97,11 +105,15 @@ func WriteCommit(t testing.TB, cfg config.Cfg, repoPath string, opts ...WriteCom
 		tree = parents[0].String() + "^{tree}"
 	}
 
+	if writeCommitConfig.committerName == "" {
+		writeCommitConfig.committerName = committerName
+	}
+
 	// Use 'commit-tree' instead of 'commit' because we are in a bare
 	// repository. What we do here is the same as "commit -m message
 	// --allow-empty".
 	commitArgs := []string{
-		"-c", fmt.Sprintf("user.name=%s", committerName),
+		"-c", fmt.Sprintf("user.name=%s", writeCommitConfig.committerName),
 		"-c", fmt.Sprintf("user.email=%s", committerEmail),
 		"-C", repoPath,
 		"commit-tree", "-F", "-", tree,
diff --git a/internal/gitaly/service/ref/refs_test.go b/internal/gitaly/service/ref/refs_test.go
index 96db73dd0..ef7f0f5eb 100644
--- a/internal/gitaly/service/ref/refs_test.go
+++ b/internal/gitaly/service/ref/refs_test.go
@@ -785,6 +785,31 @@ func TestSuccessfulFindLocalBranches(t *testing.T) {
 	}
 }
 
+func TestFindLocalBranches_huge_committer(t *testing.T) {
+	cfg, repo, repoPath, client := setupRefService(t)
+
+	gittest.WriteCommit(t, cfg, repoPath,
+		gittest.WithBranch("refs/heads/improve/awesome"),
+		gittest.WithCommitterName(strings.Repeat("A", 100000)),
+	)
+
+	rpcRequest := &gitalypb.FindLocalBranchesRequest{Repository: repo}
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	c, err := client.FindLocalBranches(ctx, rpcRequest)
+	require.NoError(t, err)
+
+	for {
+		_, err := c.Recv()
+		if err == io.EOF {
+			break
+		}
+		require.NoError(t, err)
+	}
+}
+
 func TestFindLocalBranchesPagination(t *testing.T) {
 	_, repo, _, client := setupRefService(t)
 
diff --git a/ruby/proto/gitaly/version.rb b/ruby/proto/gitaly/version.rb
index 2f4dacafb..5bb042a65 100644
--- a/ruby/proto/gitaly/version.rb
+++ b/ruby/proto/gitaly/version.rb
@@ -2,5 +2,5 @@
 # (https://gitlab.com/gitlab-org/release-tools/), and should not be
 # modified.
 module Gitaly
-  VERSION = '13.12.8'
+  VERSION = '13.12.9'
 end