git: Disallow more whitespace characters in revisions

Revisions cannot contain any whitespace characters, and this is an
assumption that we make in many parts of our application. Let's make the
enforcement of this stricter by also verifying that there are no tabs or
newlines in revision names.

3 files changed, 18 insertions, 10 deletions

diff --git a/internal/git/revision.go b/internal/git/revision.go
index 1e13a02ad..89d1e05d4 100644
--- a/internal/git/revision.go
+++ b/internal/git/revision.go
@@ -12,7 +12,7 @@ func validateRevision(revision []byte, allowEmpty bool) error {
 	if bytes.HasPrefix(revision, []byte("-")) {
 		return fmt.Errorf("revision can't start with '-'")
 	}
-	if bytes.Contains(revision, []byte(" ")) {
+	if bytes.ContainsAny(revision, " \t\n\r") {
 		return fmt.Errorf("revision can't contain whitespace")
 	}
 	if bytes.Contains(revision, []byte("\x00")) {
diff --git a/internal/git/revision_test.go b/internal/git/revision_test.go
index 0769d8ff9..62751690a 100644
--- a/internal/git/revision_test.go
+++ b/internal/git/revision_test.go
@@ -37,6 +37,21 @@ func TestValidateRevision(t *testing.T) {
 			expectedErr: fmt.Errorf("revision can't contain whitespace"),
 		},
 		{
+			desc:        "newline",
+			revision:    "foo\nbar",
+			expectedErr: fmt.Errorf("revision can't contain whitespace"),
+		},
+		{
+			desc:        "tab",
+			revision:    "foo\tbar",
+			expectedErr: fmt.Errorf("revision can't contain whitespace"),
+		},
+		{
+			desc:        "carriage-return",
+			revision:    "foo\rbar",
+			expectedErr: fmt.Errorf("revision can't contain whitespace"),
+		},
+		{
 			desc:        "NUL-byte",
 			revision:    "foo\x00bar",
 			expectedErr: fmt.Errorf("revision can't contain NUL"),
diff --git a/internal/gitaly/service/commit/check_objects_exist_test.go b/internal/gitaly/service/commit/check_objects_exist_test.go
index efe069bff..f93267bf6 100644
--- a/internal/gitaly/service/commit/check_objects_exist_test.go
+++ b/internal/gitaly/service/commit/check_objects_exist_test.go
@@ -169,15 +169,8 @@ func TestCheckObjectsExist(t *testing.T) {
 					},
 				},
 			},
-			// This is a bug: instead of disallowing revisions with newlines, we just
-			// handle them funnily. Git things we're asking it for two different
-			// revisions, and we expect only one output. So the result is that this
-			// weirdly formatted revision looks like it would exist. Instead, we should
-			// raise an error.
-			expectedErr: nil,
-			expectedResults: map[string]bool{
-				fmt.Sprintf("%s\n%s", commitID1, commitID2): true,
-			},
+			expectedErr:     helper.ErrInvalidArgumentf("invalid revision %q: revision can't contain whitespace", fmt.Sprintf("%s\n%s", commitID1, commitID2)),
+			expectedResults: map[string]bool{},
 		},
 		{
 			desc: "chunked invalid input",