service/smarthttp: Improve validation of input

Gitaly should return the same error for all RPCs where the
Repository input parameter is missing.
The test coverage extended to cover changed code.
The error verification is done not only for the code, but for
the message as well. It gives us confidence that a proper path
is tested.

5 files changed, 110 insertions, 70 deletions

diff --git a/internal/gitaly/service/smarthttp/inforefs.go b/internal/gitaly/service/smarthttp/inforefs.go
index c3d4dfd84..1b54245f3 100644
--- a/internal/gitaly/service/smarthttp/inforefs.go
+++ b/internal/gitaly/service/smarthttp/inforefs.go
@@ -7,6 +7,7 @@ import (
 
 	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
 	log "github.com/sirupsen/logrus"
+	gitalyerrors "gitlab.com/gitlab-org/gitaly/v15/internal/errors"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/pktline"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
@@ -20,6 +21,9 @@ const (
 )
 
 func (s *server) InfoRefsUploadPack(in *gitalypb.InfoRefsRequest, stream gitalypb.SmartHTTPService_InfoRefsUploadPackServer) error {
+	if in.GetRepository() == nil {
+		return helper.ErrInvalidArgument(gitalyerrors.ErrEmptyRepository)
+	}
 	repoPath, err := s.locator.GetRepoPath(in.GetRepository())
 	if err != nil {
 		return err
@@ -35,6 +39,9 @@ func (s *server) InfoRefsUploadPack(in *gitalypb.InfoRefsRequest, stream gitalyp
 }
 
 func (s *server) InfoRefsReceivePack(in *gitalypb.InfoRefsRequest, stream gitalypb.SmartHTTPService_InfoRefsReceivePackServer) error {
+	if in.GetRepository() == nil {
+		return helper.ErrInvalidArgument(gitalyerrors.ErrEmptyRepository)
+	}
 	repoPath, err := s.locator.GetRepoPath(in.GetRepository())
 	if err != nil {
 		return err
diff --git a/internal/gitaly/service/smarthttp/inforefs_test.go b/internal/gitaly/service/smarthttp/inforefs_test.go
index 072156fd1..9574bb65a 100644
--- a/internal/gitaly/service/smarthttp/inforefs_test.go
+++ b/internal/gitaly/service/smarthttp/inforefs_test.go
@@ -24,7 +24,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/stats"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/transaction"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg"
@@ -32,6 +31,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/v15/streamio"
 	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/proto"
 )
 
@@ -130,27 +130,40 @@ func TestInfoRefsUploadPack_internalRefs(t *testing.T) {
 	}
 }
 
-func TestInfoRefsUploadPack_repositoryDoesntExist(t *testing.T) {
+func TestInfoRefsUploadPack_validate(t *testing.T) {
 	t.Parallel()
-
+	ctx := testhelper.Context(t)
 	cfg := testcfg.Build(t)
-
 	serverSocketPath := runSmartHTTPServer(t, cfg)
 
-	rpcRequest := &gitalypb.InfoRefsRequest{Repository: &gitalypb.Repository{
-		StorageName:  cfg.Storages[0].Name,
-		RelativePath: "doesnt/exist",
-	}}
-	ctx := testhelper.Context(t)
-
-	_, err := makeInfoRefsUploadPackRequest(t, ctx, serverSocketPath, cfg.Auth.Token, rpcRequest)
-
-	expectedErr := helper.ErrNotFoundf(`GetRepoPath: not a git repository: "` + cfg.Storages[0].Path + `/doesnt/exist"`)
-	if testhelper.IsPraefectEnabled() {
-		expectedErr = helper.ErrNotFoundf(`accessor call: route repository accessor: consistent storages: repository "default"/"doesnt/exist" not found`)
+	for _, tc := range []struct {
+		desc        string
+		req         *gitalypb.InfoRefsRequest
+		expectedErr error
+	}{
+		{
+			desc: "repository not provided",
+			req:  &gitalypb.InfoRefsRequest{Repository: nil},
+			expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+				"empty Repository",
+				"repo scoped: empty Repository",
+			)),
+		},
+		{
+			desc: "not existing repository",
+			req: &gitalypb.InfoRefsRequest{Repository: &gitalypb.Repository{
+				StorageName:  cfg.Storages[0].Name,
+				RelativePath: "doesnt/exist",
+			}},
+			expectedErr: status.Error(codes.NotFound, testhelper.GitalyOrPraefect(
+				`GetRepoPath: not a git repository: "`+cfg.Storages[0].Path+`/doesnt/exist"`,
+				`accessor call: route repository accessor: consistent storages: repository "default"/"doesnt/exist" not found`,
+			)),
+		},
+	} {
+		_, err := makeInfoRefsUploadPackRequest(t, ctx, serverSocketPath, cfg.Auth.Token, tc.req)
+		testhelper.RequireGrpcError(t, tc.expectedErr, err)
 	}
-
-	testhelper.RequireGrpcError(t, expectedErr, err)
 }
 
 func TestInfoRefsUploadPack_partialClone(t *testing.T) {
@@ -330,37 +343,40 @@ func TestInfoRefsReceivePack_hiddenRefs(t *testing.T) {
 	require.NotContains(t, string(response), commitID+" .have")
 }
 
-func TestInfoRefsReceivePack_repoNotFound(t *testing.T) {
+func TestInfoRefsReceivePack_validate(t *testing.T) {
 	t.Parallel()
-
-	cfg := testcfg.Build(t)
-
-	serverSocketPath := runSmartHTTPServer(t, cfg)
-
-	repo := &gitalypb.Repository{StorageName: cfg.Storages[0].Name, RelativePath: "testdata/scratch/another_repo"}
-	rpcRequest := &gitalypb.InfoRefsRequest{Repository: repo}
 	ctx := testhelper.Context(t)
-	_, err := makeInfoRefsReceivePackRequest(t, ctx, serverSocketPath, cfg.Auth.Token, rpcRequest)
-
-	expectedErr := helper.ErrNotFoundf(`GetRepoPath: not a git repository: "` + cfg.Storages[0].Path + "/" + repo.RelativePath + `"`)
-	if testhelper.IsPraefectEnabled() {
-		expectedErr = helper.ErrNotFoundf(`accessor call: route repository accessor: consistent storages: repository "default"/"testdata/scratch/another_repo" not found`)
-	}
-
-	testhelper.RequireGrpcError(t, expectedErr, err)
-}
-
-func TestInfoRefsReceivePack_repoNotSet(t *testing.T) {
-	t.Parallel()
-
 	cfg := testcfg.Build(t)
-
 	serverSocketPath := runSmartHTTPServer(t, cfg)
 
-	rpcRequest := &gitalypb.InfoRefsRequest{}
-	ctx := testhelper.Context(t)
-	_, err := makeInfoRefsReceivePackRequest(t, ctx, serverSocketPath, cfg.Auth.Token, rpcRequest)
-	testhelper.RequireGrpcCode(t, err, codes.InvalidArgument)
+	for _, tc := range []struct {
+		desc        string
+		req         *gitalypb.InfoRefsRequest
+		expectedErr error
+	}{
+		{
+			desc: "repository not provided",
+			req:  &gitalypb.InfoRefsRequest{Repository: nil},
+			expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+				"empty Repository",
+				"repo scoped: empty Repository",
+			)),
+		},
+		{
+			desc: "not existing repository",
+			req: &gitalypb.InfoRefsRequest{Repository: &gitalypb.Repository{
+				StorageName:  cfg.Storages[0].Name,
+				RelativePath: "testdata/scratch/another_repo",
+			}},
+			expectedErr: status.Error(codes.NotFound, testhelper.GitalyOrPraefect(
+				`GetRepoPath: not a git repository: "`+cfg.Storages[0].Path+`/testdata/scratch/another_repo"`,
+				`accessor call: route repository accessor: consistent storages: repository "default"/"testdata/scratch/another_repo" not found`,
+			)),
+		},
+	} {
+		_, err := makeInfoRefsReceivePackRequest(t, ctx, serverSocketPath, cfg.Auth.Token, tc.req)
+		testhelper.RequireGrpcError(t, tc.expectedErr, err)
+	}
 }
 
 func makeInfoRefsReceivePackRequest(t *testing.T, ctx context.Context, serverSocketPath, token string, rpcRequest *gitalypb.InfoRefsRequest) ([]byte, error) {
diff --git a/internal/gitaly/service/smarthttp/receive_pack_test.go b/internal/gitaly/service/smarthttp/receive_pack_test.go
index c847a01d7..d7b066969 100644
--- a/internal/gitaly/service/smarthttp/receive_pack_test.go
+++ b/internal/gitaly/service/smarthttp/receive_pack_test.go
@@ -32,6 +32,8 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/v15/streamio"
 	"google.golang.org/grpc"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 )
 
 const (
@@ -376,24 +378,18 @@ func TestPostReceivePack_requestValidation(t *testing.T) {
 				},
 				GlId: "user-123",
 			},
-			expectedErr: func() error {
-				if testhelper.IsPraefectEnabled() {
-					return helper.ErrInvalidArgumentf("repo scoped: invalid Repository")
-				}
-
-				return helper.ErrInvalidArgumentf(`GetStorageByName: no such storage: "fake"`)
-			}(),
+			expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+				`GetStorageByName: no such storage: "fake"`,
+				"repo scoped: invalid Repository",
+			)),
 		},
 		{
 			desc:    "Repository is nil",
 			request: &gitalypb.PostReceivePackRequest{Repository: nil, GlId: "user-123"},
-			expectedErr: func() error {
-				if testhelper.IsPraefectEnabled() {
-					return helper.ErrInvalidArgumentf("repo scoped: empty Repository")
-				}
-
-				return helper.ErrInvalidArgumentf("empty Repository")
-			}(),
+			expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+				"empty Repository",
+				"repo scoped: empty Repository",
+			)),
 		},
 		{
 			desc: "Empty GlId",
diff --git a/internal/gitaly/service/smarthttp/upload_pack.go b/internal/gitaly/service/smarthttp/upload_pack.go
index 49b03682d..f65aa9fc9 100644
--- a/internal/gitaly/service/smarthttp/upload_pack.go
+++ b/internal/gitaly/service/smarthttp/upload_pack.go
@@ -8,6 +8,7 @@ import (
 
 	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/command"
+	gitalyerrors "gitlab.com/gitlab-org/gitaly/v15/internal/errors"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/stats"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
@@ -36,7 +37,7 @@ func (s *server) PostUploadPack(stream gitalypb.SmartHTTPService_PostUploadPackS
 
 	repoPath, gitConfig, err := s.validateUploadPackRequest(ctx, req)
 	if err != nil {
-		return err
+		return helper.ErrInvalidArgument(err)
 	}
 
 	stdin := streamio.NewReader(func() ([]byte, error) {
@@ -54,7 +55,7 @@ func (s *server) PostUploadPack(stream gitalypb.SmartHTTPService_PostUploadPackS
 func (s *server) PostUploadPackWithSidechannel(ctx context.Context, req *gitalypb.PostUploadPackWithSidechannelRequest) (*gitalypb.PostUploadPackWithSidechannelResponse, error) {
 	repoPath, gitConfig, err := s.validateUploadPackRequest(ctx, req)
 	if err != nil {
-		return nil, err
+		return nil, helper.ErrInvalidArgument(err)
 	}
 
 	conn, err := sidechannel.OpenSidechannel(ctx)
@@ -108,16 +109,19 @@ func (s *server) runStatsCollector(ctx context.Context, r io.Reader) (io.Reader,
 }
 
 func (s *server) validateUploadPackRequest(ctx context.Context, req basicPostUploadPackRequest) (string, []git.ConfigPair, error) {
+	if req.GetRepository() == nil {
+		return "", nil, gitalyerrors.ErrEmptyRepository
+	}
 	repoPath, err := s.locator.GetRepoPath(req.GetRepository())
 	if err != nil {
-		return "", nil, helper.ErrInvalidArgument(err)
+		return "", nil, err
 	}
 
 	git.WarnIfTooManyBitmaps(ctx, s.locator, req.GetRepository().GetStorageName(), repoPath)
 
 	config, err := git.ConvertConfigOptions(req.GetGitConfigOptions())
 	if err != nil {
-		return "", nil, helper.ErrInvalidArgument(err)
+		return "", nil, err
 	}
 
 	return repoPath, config, nil
diff --git a/internal/gitaly/service/smarthttp/upload_pack_test.go b/internal/gitaly/service/smarthttp/upload_pack_test.go
index 7c8a2a084..398a5df8c 100644
--- a/internal/gitaly/service/smarthttp/upload_pack_test.go
+++ b/internal/gitaly/service/smarthttp/upload_pack_test.go
@@ -30,6 +30,7 @@ import (
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/credentials/insecure"
+	"google.golang.org/grpc/status"
 )
 
 const (
@@ -377,15 +378,31 @@ func testServerPostUploadPackWithSideChannelValidation(t *testing.T, ctx context
 	cfg := testcfg.Build(t, opts...)
 	serverSocketPath := runSmartHTTPServer(t, cfg)
 
-	rpcRequests := []*gitalypb.PostUploadPackRequest{
-		{Repository: &gitalypb.Repository{StorageName: "fake", RelativePath: "path"}}, // Repository doesn't exist
-		{Repository: nil}, // Repository is nil
-	}
-
-	for _, rpcRequest := range rpcRequests {
-		t.Run(fmt.Sprintf("%v", rpcRequest), func(t *testing.T) {
-			_, err := makeRequest(t, ctx, serverSocketPath, cfg.Auth.Token, rpcRequest, bytes.NewBuffer(nil))
-			testhelper.RequireGrpcCode(t, err, codes.InvalidArgument)
+	for _, tc := range []struct {
+		desc        string
+		req         *gitalypb.PostUploadPackRequest
+		expectedErr error
+	}{
+		{
+			desc: "Repository doesn't exist",
+			req:  &gitalypb.PostUploadPackRequest{Repository: &gitalypb.Repository{StorageName: "fake", RelativePath: "path"}},
+			expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+				`GetStorageByName: no such storage: "fake"`,
+				"repo scoped: invalid Repository",
+			)),
+		},
+		{
+			desc: "Repository no provided",
+			req:  &gitalypb.PostUploadPackRequest{Repository: nil},
+			expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+				"empty Repository",
+				"repo scoped: empty Repository",
+			)),
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			_, err := makeRequest(t, ctx, serverSocketPath, cfg.Auth.Token, tc.req, bytes.NewBuffer(nil))
+			testhelper.RequireGrpcError(t, tc.expectedErr, err)
 		})
 	}
 }