git: Disallow more whitespace characters in revisionspks-check-objects-exist-improvements

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.

2 files changed, 3 insertions, 10 deletions

diff --git a/internal/git/proto.go b/internal/git/proto.go
index 97dac733b..ed0ac84fd 100644
--- a/internal/git/proto.go
+++ b/internal/git/proto.go
@@ -18,7 +18,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/gitaly/service/commit/check_objects_exist_test.go b/internal/gitaly/service/commit/check_objects_exist_test.go
index 5629e11ba..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",