Merge branch 'ref-colon-invalid' into 'master'

Colons are not allowed in refs

See merge request gitlab-org/gitaly!747

5 files changed, 58 insertions, 9 deletions

diff --git a/changelogs/unreleased/ref-colon-invalid.yml b/changelogs/unreleased/ref-colon-invalid.yml
new file mode 100644
index 000000000..a01c21137
--- /dev/null
+++ b/changelogs/unreleased/ref-colon-invalid.yml
@@ -0,0 +1,5 @@
+---
+title: Colons are not allowed in refs
+merge_request: 747
+author:
+type: changed
diff --git a/internal/git/helper.go b/internal/git/helper.go
index 5e816584f..64d242c0d 100644
--- a/internal/git/helper.go
+++ b/internal/git/helper.go
@@ -33,6 +33,9 @@ func ValidateRevision(revision []byte) error {
 	if bytes.Contains(revision, []byte("\x00")) {
 		return fmt.Errorf("revision can't contain NUL")
 	}
+	if bytes.Contains(revision, []byte(":")) {
+		return fmt.Errorf("revision can't contain ':'")
+	}
 	return nil
 }
 
diff --git a/internal/git/helper_test.go b/internal/git/helper_test.go
new file mode 100644
index 000000000..e928e69d7
--- /dev/null
+++ b/internal/git/helper_test.go
@@ -0,0 +1,31 @@
+package git
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestValidateRevision(t *testing.T) {
+	testCases := []struct {
+		rev string
+		ok  bool
+	}{
+		{rev: "foo/bar", ok: true},
+		{rev: "-foo/bar", ok: false},
+		{rev: "foo bar", ok: false},
+		{rev: "foo\x00bar", ok: false},
+		{rev: "foo/bar:baz", ok: false},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.rev, func(t *testing.T) {
+			err := ValidateRevision([]byte(tc.rev))
+			if tc.ok {
+				require.NoError(t, err)
+			} else {
+				require.Error(t, err)
+			}
+		})
+	}
+}
diff --git a/internal/service/commit/find_commit.go b/internal/service/commit/find_commit.go
index 6c73687a3..4bd4ba744 100644
--- a/internal/service/commit/find_commit.go
+++ b/internal/service/commit/find_commit.go
@@ -30,12 +30,12 @@ func init() {
 }
 
 func (s *server) FindCommit(ctx context.Context, in *pb.FindCommitRequest) (*pb.FindCommitResponse, error) {
-	if err := git.ValidateRevision(in.GetRevision()); err != nil {
+	revision := in.GetRevision()
+	if err := git.ValidateRevision(revision); err != nil {
 		return nil, status.Errorf(codes.InvalidArgument, "FindCommit: revision: %v", err)
 	}
 
 	repo := in.GetRepository()
-	revision := in.GetRevision()
 
 	if featureflag.IsEnabled(ctx, "gogit-findcommit") {
 		commit, err := gogitFindCommit(repo, revision)
diff --git a/internal/service/commit/find_commit_test.go b/internal/service/commit/find_commit_test.go
index b0874a1df..003f53f5f 100644
--- a/internal/service/commit/find_commit_test.go
+++ b/internal/service/commit/find_commit_test.go
@@ -15,6 +15,7 @@ import (
 	"golang.org/x/net/context"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/metadata"
+	"google.golang.org/grpc/status"
 )
 
 func TestSuccessfulFindCommitRequest(t *testing.T) {
@@ -247,7 +248,7 @@ func TestSuccessfulFindCommitRequest(t *testing.T) {
 		})
 	}
 
-	ctx = metadata.NewOutgoingContext(
+	gogitCtx := metadata.NewOutgoingContext(
 		ctx,
 		metadata.New(map[string]string{featureflag.HeaderKey("gogit-findcommit"): "true"}),
 	)
@@ -258,9 +259,9 @@ func TestSuccessfulFindCommitRequest(t *testing.T) {
 			Revision:   []byte(testCase.revision),
 		}
 
-		response, err := client.FindCommit(ctx, request)
-		require.NoError(t, err)
-		require.Equal(t, allCommits[i], response.Commit)
+		response, err := client.FindCommit(gogitCtx, request)
+		require.NoError(t, err, "request with go-git should succeed")
+		require.Equal(t, allCommits[i], response.Commit, "commit fetched with go-git should match default")
 	}
 }
 
@@ -284,8 +285,16 @@ func TestFailedFindCommitRequest(t *testing.T) {
 		{repo: invalidRepo, revision: []byte("master"), description: "Invalid repo"},
 		{repo: testRepo, revision: []byte(""), description: "Empty revision"},
 		{repo: testRepo, revision: []byte("-master"), description: "Invalid revision"},
+		{repo: testRepo, revision: []byte("mas:ter"), description: "Invalid revision"},
 	}
 
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+	gogitCtx := metadata.NewOutgoingContext(
+		ctx,
+		metadata.New(map[string]string{featureflag.HeaderKey("gogit-findcommit"): "true"}),
+	)
+
 	for _, testCase := range testCases {
 		t.Run(testCase.description, func(t *testing.T) {
 			request := &pb.FindCommitRequest{
@@ -293,10 +302,11 @@ func TestFailedFindCommitRequest(t *testing.T) {
 				Revision:   testCase.revision,
 			}
 
-			ctx, cancel := context.WithCancel(context.Background())
-			defer cancel()
 			_, err := client.FindCommit(ctx, request)
-			testhelper.AssertGrpcError(t, err, codes.InvalidArgument, "")
+			require.Equal(t, codes.InvalidArgument, status.Code(err), "default lookup should fail")
+
+			_, err = client.FindCommit(gogitCtx, request)
+			require.Equal(t, codes.InvalidArgument, status.Code(err), "go-git lookup should fail")
 		})
 	}
 }