From ece68176e22d1ac68a3ce8a072a2acbd0fbbb056 Mon Sep 17 00:00:00 2001
From: Xing Xin <xingxin.xx@bytedance.com>
Date: Wed, 21 May 2025 19:06:54 +0800
Subject: [PATCH] operations: Validate object type before updating file

I discovered a critical bug in the UserCommitFiles RPC where updating a
file using a path that points to a directory instead of a regular file
causes repository corruption.

Reproduction steps using the GitLab API:

  $ curl --request PUT --header "PRIVATE-TOKEN: ******" "https://gitlab.com/api/v4/projects/blanet-xx%2Fdemo/repository/files/path%2Fto?branch=test-repo&content=hello&commit_message=demo"

The API returns a 200 OK response:

  {"file_path":"path/to","branch":"test-repo"}

However, cloning the repository afterward results in errors indicating
corruption:

  $ git clone git@gitlab.com:blanet-xx/demo.git
  Cloning into 'demo'...
  remote: error: Object b6fc4c620b67d95f953a5c1c1230aaab5db5a1b0 not a tree
  remote: fatal: bad tree object b6fc4c620b67d95f953a5c1c1230aaab5db5a1b0
  remote: error executing git hook
  remote: aborting due to possible repository corruption on the remote side.
  fatal: early EOF
  error: git upload-pack: git-pack-objects died with error.
  fatal: git upload-pack: aborting due to possible repository corruption on the remote side.
  fatal: fetch-pack: invalid index-pack output

After some digging I find that the PRC lacks necessary validation on
object type before calls `root.Modify`, which blindly overwrites OID
when a matching Path is found.

And the fix is straightforward, let's add an object type validation
before the tree modification.
---
 .../gitaly/service/operations/commit_files.go | 14 ++++++++++--
 .../service/operations/commit_files_test.go   | 22 +++++++++++++++++++
 2 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/internal/gitaly/service/operations/commit_files.go b/internal/gitaly/service/operations/commit_files.go
index 5913288f05..373af1749f 100644
--- a/internal/gitaly/service/operations/commit_files.go
+++ b/internal/gitaly/service/operations/commit_files.go
@@ -256,6 +256,18 @@ func applyAction(
 			return translateError(err, action.Path)
 		}
 	case updateFile:
+		entry, err := root.Get(action.Path)
+		if err != nil {
+			return translateError(err, action.Path)
+		}
+
+		if entry.Type != localrepo.Blob {
+			return indexError{
+				path:      action.Path,
+				errorType: errFileNotFound,
+			}
+		}
+
 		if err := root.Modify(
 			action.Path,
 			func(entry *localrepo.TreeEntry) error {
@@ -415,7 +427,6 @@ func (s *Server) userCommitFilesGit(
 	header *gitalypb.UserCommitFilesRequestHeader,
 	parentCommitOID git.ObjectID,
 	quarantineRepo *localrepo.Repo,
-	repoPath string,
 	actions []commitAction,
 ) (git.ObjectID, error) {
 	committerSignature, err := git.SignatureFromRequest(header)
@@ -693,7 +704,6 @@ func (s *Server) userCommitFiles(
 		header,
 		parentCommitOID,
 		quarantineRepo,
-		repoPath,
 		actions,
 	)
 	if err != nil {
diff --git a/internal/gitaly/service/operations/commit_files_test.go b/internal/gitaly/service/operations/commit_files_test.go
index e4f7dcf568..eb305121db 100644
--- a/internal/gitaly/service/operations/commit_files_test.go
+++ b/internal/gitaly/service/operations/commit_files_test.go
@@ -459,6 +459,28 @@ func testUserCommitFiles(t *testing.T, ctx context.Context) {
 				},
 			},
 		},
+		{
+			desc: "update file fails if the target is a directory",
+			steps: []step{
+				{
+					actions: []*gitalypb.UserCommitFilesRequest{
+						createDirHeaderRequest("directory"),
+					},
+					repoCreated:   true,
+					branchCreated: true,
+					treeEntries: []gittest.TreeEntry{
+						{Mode: DefaultMode, Path: "directory/.gitkeep"},
+					},
+				},
+				{
+					actions: []*gitalypb.UserCommitFilesRequest{
+						updateFileHeaderRequest("directory"),
+						actionContentRequest("content"),
+					},
+					expectedErr: structuredIndexError(t, &indexError{path: "directory", errorType: errFileNotFound}),
+				},
+			},
+		},
 		{
 			desc: "move file with traversal in source",
 			steps: []step{
-- 
GitLab