Only snapshot valid Git directories

Transaction manager currently includes all requested relative paths
in the snapshot. Typically this would be the relative path of the
target repository. If the client fed it paths such as '@hashed', it
would attempt snapshot the entire storage. If Gitaly controlled the
directory structure, we could easily guard against this by only
allowing leaf/project directories to be snapshotted. The paths are
currently controlled by the client so we can't rely on this without
possibly breaking some paths.

Workaround this by checking whether the snapshotted directory is a
git repository. If it isn't, the snapshotting will error out. This
prevents using any of the higher level directories as the path to
snapshot.

The error equality assertion when beginning a transaction was changed
to a an error.Is assertion. This avoids having to repeat the error
annotations in tests which are not meaningful while still testing that
the expected error is in the chain.

3 files changed, 42 insertions, 1 deletions

diff --git a/internal/gitaly/service/ref/find_refs_by_oid_test.go b/internal/gitaly/service/ref/find_refs_by_oid_test.go
index b7ac3d779..9e821c989 100644
--- a/internal/gitaly/service/ref/find_refs_by_oid_test.go
+++ b/internal/gitaly/service/ref/find_refs_by_oid_test.go
@@ -160,6 +160,15 @@ func TestFindRefsByOID_failure(t *testing.T) {
 						Repository: repo,
 						Oid:        oid.String(),
 					}, func(actual error) {
+						if testhelper.IsWALEnabled() {
+							testhelper.RequireGrpcError(t,
+								status.Error(codes.Internal, "begin transaction: new snapshot: create repository snapshots: validate git directory: invalid git directory"),
+								actual,
+							)
+
+							return
+						}
+
 						testhelper.RequireStatusWithErrorMetadataRegexp(t,
 							structerr.NewFailedPrecondition("%w: %q does not exist", storage.ErrRepositoryNotValid, "objects"),
 							actual,
diff --git a/internal/gitaly/storage/storagemgr/snapshot.go b/internal/gitaly/storage/storagemgr/snapshot.go
index 1b90a978e..aa234c49c 100644
--- a/internal/gitaly/storage/storagemgr/snapshot.go
+++ b/internal/gitaly/storage/storagemgr/snapshot.go
@@ -9,6 +9,7 @@ import (
 	"path/filepath"
 
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/housekeeping"
+	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/helper/perm"
 )
 
@@ -40,6 +41,11 @@ func newSnapshot(ctx context.Context, storagePath, snapshotPath string, relative
 // createRepositorySnapshots creates a snapshot of the partition containing all repositories at the given relative paths
 // and their alternates.
 func createRepositorySnapshots(ctx context.Context, storagePath, snapshotPrefix string, relativePaths []string) error {
+	// Create the root directory always to as the storage would also exist always.
+	if err := os.Mkdir(filepath.Join(storagePath, snapshotPrefix), perm.PrivateDir); err != nil {
+		return fmt.Errorf("mkdir snapshot root: %w", err)
+	}
+
 	snapshottedRepositories := make(map[string]struct{}, len(relativePaths))
 	for _, relativePath := range relativePaths {
 		if _, ok := snapshottedRepositories[relativePath]; ok {
@@ -47,6 +53,20 @@ func createRepositorySnapshots(ctx context.Context, storagePath, snapshotPrefix
 		}
 
 		sourcePath := filepath.Join(storagePath, relativePath)
+		if err := storage.ValidateGitDirectory(sourcePath); err != nil {
+			if errors.Is(err, fs.ErrNotExist) {
+				// It's okay if the repository does not exist. We'll create a snapshot without the directory,
+				// and the RPC handlers can handle the situation as best fit.
+				continue
+			}
+
+			// The transaction logic doesn't require the snapshotted repository to be valid. We want to ensure
+			// we only snapshot a 'leaf'/project directories in the storage. Otherwise relative paths like
+			// `@hashed/xx` could attempt to snapshot an entire subtree. As Gitaly doesn't control the directory
+			// hierarchy yet, we achieve this protection by only snapshotting valid Git directories.
+			return fmt.Errorf("validate git directory: %w", err)
+		}
+
 		targetPath := filepath.Join(storagePath, snapshotPrefix, relativePath)
 		if err := createRepositorySnapshot(ctx, sourcePath, targetPath); err != nil {
 			return fmt.Errorf("create snapshot: %w", err)
diff --git a/internal/gitaly/storage/storagemgr/transaction_manager_test.go b/internal/gitaly/storage/storagemgr/transaction_manager_test.go
index d000c5587..b4188bfe1 100644
--- a/internal/gitaly/storage/storagemgr/transaction_manager_test.go
+++ b/internal/gitaly/storage/storagemgr/transaction_manager_test.go
@@ -6209,6 +6209,18 @@ func TestTransactionManager(t *testing.T) {
 				},
 			},
 		},
+		{
+			desc: "non-git directories are not snapshotted",
+			steps: steps{
+				StartManager{},
+				Begin{
+					TransactionID: 1,
+					// Try to snapshot the parent directory, which is no a valid Git directory.
+					RelativePath:  filepath.Dir(relativePath),
+					ExpectedError: storage.InvalidGitDirectoryError{MissingEntry: "objects"},
+				},
+			},
+		},
 	}
 
 	type invalidReferenceTestCase struct {
@@ -6414,7 +6426,7 @@ func TestTransactionManager(t *testing.T) {
 					}
 
 					transaction, err := transactionManager.Begin(beginCtx, step.RelativePath, step.SnapshottedRelativePaths, step.ReadOnly)
-					require.Equal(t, step.ExpectedError, err)
+					require.ErrorIs(t, err, step.ExpectedError)
 					if err == nil {
 						require.Equal(t, step.ExpectedSnapshotLSN, transaction.SnapshotLSN())
 					}