git: receivepack: Allow commits with invalid timezones to be pushed

In the past, some git clients used to output bad timezones in the author
and committer fields under special circumstances. While the error has
been fixed several years ago already, there are repositories which have
such broken commits in their histories. As git-receive-pack complains
about such objects when running with "receive.fsckObjects=true", which
is our default, users cannot push such repos to GitLab.

Disabling the object checks entirely is a bad idea, especially so as
they also verify that no objects are being pushed that try to exploit
known security vulnerabilities. Instead, this commit selectively
disables the check for bad timezones by setting
"receive.fsck.badTimezone=ignore" and adds a test to verify such commits
get accepted.

3 files changed, 67 insertions, 0 deletions

diff --git a/changelogs/unreleased/pks-receive-pack-fsck-timezone.yml b/changelogs/unreleased/pks-receive-pack-fsck-timezone.yml
new file mode 100644
index 000000000..bb0b4faa3
--- /dev/null
+++ b/changelogs/unreleased/pks-receive-pack-fsck-timezone.yml
@@ -0,0 +1,5 @@
+---
+title: Allow commits with invalid timezones to be pushed
+merge_request: 1947
+author:
+type: fixed
diff --git a/internal/git/receivepack.go b/internal/git/receivepack.go
index 5489ff9a5..7c09d96da 100644
--- a/internal/git/receivepack.go
+++ b/internal/git/receivepack.go
@@ -37,5 +37,13 @@ func ReceivePackConfig() []Option {
 		// Because Git itself will append the pool repository directory, the
 		// command ends with a "#". The end result is that Git runs `/bin/sh -c 'exit 0 # /path/to/pool.git`.
 		ValueFlag{"-c", "core.alternateRefsCommand=exit 0 #"},
+
+		// In the past, there was a bug in git that caused users to
+		// create commits with invalid timezones. As a result, some
+		// histories contain commits that do not match the spec. As we
+		// fsck received packfiles by default, any push containing such
+		// a commit will be rejected. As this is a mostly harmless
+		// issue, we add the following flag to ignore this check.
+		ValueFlag{"-c", "receive.fsck.badTimezone=ignore"},
 	}
 }
diff --git a/internal/service/smarthttp/receive_pack_test.go b/internal/service/smarthttp/receive_pack_test.go
index f1074ef06..9fd168b26 100644
--- a/internal/service/smarthttp/receive_pack_test.go
+++ b/internal/service/smarthttp/receive_pack_test.go
@@ -273,6 +273,60 @@ func TestFailedReceivePackRequestDueToValidationError(t *testing.T) {
 	}
 }
 
+func TestInvalidTimezone(t *testing.T) {
+	_, localRepoPath, localCleanup := testhelper.NewTestRepoWithWorktree(t)
+	defer localCleanup()
+
+	head := text.ChompBytes(testhelper.MustRunCommand(t, nil, "git", "-C", localRepoPath, "rev-parse", "HEAD"))
+	tree := text.ChompBytes(testhelper.MustRunCommand(t, nil, "git", "-C", localRepoPath, "rev-parse", "HEAD^{tree}"))
+
+	buf := new(bytes.Buffer)
+	buf.WriteString("tree " + tree + "\n")
+	buf.WriteString("parent " + head + "\n")
+	buf.WriteString("author Au Thor <author@example.com> 1313584730 +051800\n")
+	buf.WriteString("committer Au Thor <author@example.com> 1313584730 +051800\n")
+	buf.WriteString("\n")
+	buf.WriteString("Commit message\n")
+	commit := text.ChompBytes(testhelper.MustRunCommand(t, buf, "git", "-C", localRepoPath, "hash-object", "-t", "commit", "--stdin", "-w"))
+
+	stdin := strings.NewReader(fmt.Sprintf("^%s\n%s\n", head, commit))
+	pack := testhelper.MustRunCommand(t, stdin, "git", "-C", localRepoPath, "pack-objects", "--stdout", "--revs", "--thin", "--delta-base-offset", "-q")
+
+	pkt := fmt.Sprintf("%s %s refs/heads/master\x00 %s", head, commit, "report-status side-band-64k agent=git/2.12.0")
+	body := &bytes.Buffer{}
+	fmt.Fprintf(body, "%04x%s%s", len(pkt)+4, pkt, pktFlushStr)
+	body.Write(pack)
+
+	_, cleanup := testhelper.CaptureHookEnv(t)
+	defer cleanup()
+
+	server, socket := runSmartHTTPServer(t)
+	defer server.Stop()
+
+	repo, repoPath, cleanup := testhelper.NewTestRepo(t)
+	defer cleanup()
+
+	client, conn := newSmartHTTPClient(t, socket)
+	defer conn.Close()
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	stream, err := client.PostReceivePack(ctx)
+	require.NoError(t, err)
+	firstRequest := &gitalypb.PostReceivePackRequest{
+		Repository:       repo,
+		GlId:             "user-123",
+		GlRepository:     "project-456",
+		GitConfigOptions: []string{"receive.fsckObjects=true"},
+	}
+	response := doPush(t, stream, firstRequest, body)
+
+	expectedResponse := "0030\x01000eunpack ok\n0019ok refs/heads/master\n00000000"
+	require.Equal(t, expectedResponse, string(response), "Expected response to be %q, got %q", expectedResponse, response)
+	testhelper.MustRunCommand(t, nil, "git", "-C", repoPath, "show", commit)
+}
+
 func TestPostReceivePackToHooks(t *testing.T) {
 	secretToken := "secret token"
 	glRepository := "some_repo"