quarantine: Fix calling RPCs with manual object quarantine environments

When calling the `GetObjectDirectorySize()` RPC with a quarantined
repository, then we currently fail with an error because the quarantine
object directory looks like an attempted path traversal to us. This is
because manual object quarantine directories are not located beneath the
repository, but instead in the storage's temporary directory. The main
intent of this is to get cleanup for free via the tempdir package.

Now that the preceding commit has changed the way we create quarantine
directories to contain a repository-specific prefix, we can easily
determine whether a given quarantine directory's path does indeed belong
to a given repository by simply checking whether the expected quarantine
directory prefix matches the actual path we've got.

Implement this logic for `GetObjectDirectoryPath()`, which fixes calls
to `GetObjectDirectorySize()`.

Changelog: fixed

3 files changed, 86 insertions, 9 deletions

diff --git a/internal/gitaly/config/locator.go b/internal/gitaly/config/locator.go
index 5a13ad1e7..27b8329dd 100644
--- a/internal/gitaly/config/locator.go
+++ b/internal/gitaly/config/locator.go
@@ -3,6 +3,7 @@ package config
 import (
 	"os"
 	"path/filepath"
+	"strings"
 
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/repository"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/storage"
@@ -111,8 +112,25 @@ func (l *configLocator) GetObjectDirectoryPath(repo repository.GitRepo) (string,
 		return "", status.Errorf(codes.InvalidArgument, "GetObjectDirectoryPath: empty directory")
 	}
 
-	if _, err = storage.ValidateRelativePath(repoPath, objectDirectoryPath); err != nil {
-		return "", status.Errorf(codes.InvalidArgument, "GetObjectDirectoryPath: %s", err)
+	// We need to check whether the relative object directory as given by the repository is
+	// a valid path. This may either be a path in the Git repository itself, where it may either
+	// point to the main object directory storage or to an object quarantine directory as
+	// created by git-receive-pack(1). Alternatively, if that is not the case, then it may be a
+	// manual object quarantine directory located in the storage's temporary directory. These
+	// have a repository-specific prefix which we must check in order to determine whether the
+	// quarantine directory does in fact belong to the repo at hand.
+	if _, origError := storage.ValidateRelativePath(repoPath, objectDirectoryPath); origError != nil {
+		tempDir, err := l.TempDir(repo.GetStorageName())
+		if err != nil {
+			return "", status.Errorf(codes.InvalidArgument, "GetObjectDirectoryPath: %s", err)
+		}
+
+		expectedQuarantinePrefix := filepath.Join(tempDir, storage.QuarantineDirectoryPrefix(repo))
+		absoluteObjectDirectoryPath := filepath.Join(repoPath, objectDirectoryPath)
+
+		if !strings.HasPrefix(absoluteObjectDirectoryPath, expectedQuarantinePrefix) {
+			return "", status.Errorf(codes.InvalidArgument, "GetObjectDirectoryPath: %s", origError)
+		}
 	}
 
 	fullPath := filepath.Join(repoPath, objectDirectoryPath)
diff --git a/internal/gitaly/config/locator_test.go b/internal/gitaly/config/locator_test.go
index 698b7273b..58198900a 100644
--- a/internal/gitaly/config/locator_test.go
+++ b/internal/gitaly/config/locator_test.go
@@ -5,7 +5,9 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/git/quarantine"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testcfg"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
@@ -17,6 +19,13 @@ func TestConfigLocator_GetObjectDirectoryPath(t *testing.T) {
 	cfg, repo, repoPath := testcfg.BuildWithRepo(t)
 	locator := config.NewLocator(cfg)
 
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	quarantine, err := quarantine.New(ctx, repo, locator)
+	require.NoError(t, err)
+	quarantinedRepo := quarantine.QuarantinedRepo()
+
 	repoWithGitObjDir := func(repo *gitalypb.Repository, dir string) *gitalypb.Repository {
 		repo = proto.Clone(repo).(*gitalypb.Repository)
 		repo.GitObjectDirectory = dir
@@ -69,6 +78,21 @@ func TestConfigLocator_GetObjectDirectoryPath(t *testing.T) {
 			repo: repoWithGitObjDir(repo, "bazqux.git/../.."),
 			err:  codes.InvalidArgument,
 		},
+		{
+			desc: "quarantined repo",
+			repo: quarantinedRepo,
+			path: filepath.Join(repoPath, quarantinedRepo.GetGitObjectDirectory()),
+		},
+		{
+			desc: "quarantined repo with parent directory",
+			repo: repoWithGitObjDir(quarantinedRepo, quarantinedRepo.GetGitObjectDirectory()+"/.."),
+			err:  codes.InvalidArgument,
+		},
+		{
+			desc: "quarantined repo with directory traversal",
+			repo: repoWithGitObjDir(quarantinedRepo, quarantinedRepo.GetGitObjectDirectory()+"/../foobar.git"),
+			err:  codes.InvalidArgument,
+		},
 	}
 
 	for _, tc := range testCases {
diff --git a/internal/gitaly/service/repository/size_test.go b/internal/gitaly/service/repository/size_test.go
index d9a7434c4..75f96f9bb 100644
--- a/internal/gitaly/service/repository/size_test.go
+++ b/internal/gitaly/service/repository/size_test.go
@@ -4,11 +4,13 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/quarantine"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
+	"google.golang.org/protobuf/proto"
 )
 
 // We assume that the combined size of the Git objects in the test
@@ -81,18 +83,51 @@ func TestSuccessfulGetObjectDirectorySizeRequest(t *testing.T) {
 func TestGetObjectDirectorySize_quarantine(t *testing.T) {
 	t.Parallel()
 
-	cfg, repo, _, client := setupRepositoryService(t)
+	cfg, client := setupRepositoryServiceWithoutRepo(t)
 	locator := config.NewLocator(cfg)
 
 	ctx, cancel := testhelper.Context()
 	defer cancel()
 
-	quarantine, err := quarantine.New(ctx, repo, locator)
-	require.NoError(t, err)
+	t.Run("quarantined repo", func(t *testing.T) {
+		repo, _, cleanup := gittest.CloneRepoAtStorage(t, cfg, cfg.Storages[0], t.Name())
+		defer cleanup()
+
+		quarantine, err := quarantine.New(ctx, repo, locator)
+		require.NoError(t, err)
+
+		response, err := client.GetObjectDirectorySize(ctx, &gitalypb.GetObjectDirectorySizeRequest{
+			Repository: quarantine.QuarantinedRepo(),
+		})
+		require.NoError(t, err)
+		require.NotNil(t, response)
 
-	response, err := client.GetObjectDirectorySize(ctx, &gitalypb.GetObjectDirectorySizeRequest{
-		Repository: quarantine.QuarantinedRepo(),
+		// Due to platform incompatibilities we can't assert the exact size of bytes: on
+		// some, the directory entry is counted, on some it's not.
+		require.Less(t, response.Size, int64(10))
+	})
+
+	t.Run("quarantined repo with different relative path", func(t *testing.T) {
+		repo1, _, cleanup := gittest.CloneRepoAtStorage(t, cfg, cfg.Storages[0], "repo-1")
+		defer cleanup()
+		quarantine1, err := quarantine.New(ctx, repo1, locator)
+		require.NoError(t, err)
+
+		repo2, _, cleanup := gittest.CloneRepoAtStorage(t, cfg, cfg.Storages[0], "repo-2")
+		defer cleanup()
+		quarantine2, err := quarantine.New(ctx, repo2, locator)
+		require.NoError(t, err)
+
+		// We swap out the the object directories of both quarantines. So while both are
+		// valid, we still expect that this RPC call fails because we detect that the
+		// swapped-in quarantine directory does not belong to our repository.
+		repo := proto.Clone(quarantine1.QuarantinedRepo()).(*gitalypb.Repository)
+		repo.GitObjectDirectory = quarantine2.QuarantinedRepo().GetGitObjectDirectory()
+
+		response, err := client.GetObjectDirectorySize(ctx, &gitalypb.GetObjectDirectorySizeRequest{
+			Repository: repo,
+		})
+		require.Error(t, err, "rpc error: code = InvalidArgument desc = GetObjectDirectoryPath: relative path escapes root directory")
+		require.Nil(t, response)
 	})
-	require.EqualError(t, err, "rpc error: code = InvalidArgument desc = GetObjectDirectoryPath: relative path escapes root directory")
-	require.Nil(t, response)
 }