commit: Gracefully handle paths escaping repo in LastCommitForPath

In 91e842a31 (git/log: Read last commit ID into buffer, 2023-08-28), we
refactored LastCommitForPath to perform better error checking. Part of
this change was to also verify that the git-log(1) command doesn't
return an error where we previously just returned successfully in case
its stdout was empty.

This change had the unexpected side effect that we now indeed detect
errors in the way the RPC is getting called. Previously, when called
with a path that escapes outside the repository root, we would have
silently returned a successful response. But in general, it is quite
likely that the actual intent was to treat the path as relative to the
repostiory's root directory.

So even though we now start to fail in new ways, we can consider this to
be a good thing because it causes us to detect misuses of our API that
never worked. The way we treat this error isn't great though, as all we
return to the caller is the information that the git-log(1) command has
exited with an error.

Handle these erorrs more gracefully by detecting whether the provided
paths would escape the repository root. If so, we now return an error
with `codes.InvalidArgument` and a proper error message.

Changelog: fixed

2 files changed, 36 insertions, 0 deletions

diff --git a/internal/gitaly/service/commit/last_commit_for_path.go b/internal/gitaly/service/commit/last_commit_for_path.go
index 4bd3a1b2b..e0f69a97c 100644
--- a/internal/gitaly/service/commit/last_commit_for_path.go
+++ b/internal/gitaly/service/commit/last_commit_for_path.go
@@ -3,6 +3,7 @@ package commit
 import (
 	"context"
 	"errors"
+	"path/filepath"
 
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/catfile"
@@ -66,5 +67,18 @@ func validateLastCommitForPathRequest(locator storage.Locator, in *gitalypb.Last
 	if err := git.ValidateRevision(in.Revision); err != nil {
 		return err
 	}
+
+	path := string(in.GetPath())
+	switch {
+	case path == "", path == "/":
+		// We map both the empty path and "/" to instead refer to the root directory, so these are fine.
+	case filepath.IsAbs(path):
+		// Strictly speaking this is already handled by `filepath.IsLocal()`, but handling this case explicitly
+		// allows us to generate a better error message.
+		return structerr.NewInvalidArgument("path is an absolute path").WithMetadata("path", path)
+	case !filepath.IsLocal(path):
+		return structerr.NewInvalidArgument("path escapes repository").WithMetadata("path", path)
+	}
+
 	return nil
 }
diff --git a/internal/gitaly/service/commit/last_commit_for_path_test.go b/internal/gitaly/service/commit/last_commit_for_path_test.go
index f4a2362a3..dd6e7d9af 100644
--- a/internal/gitaly/service/commit/last_commit_for_path_test.go
+++ b/internal/gitaly/service/commit/last_commit_for_path_test.go
@@ -127,6 +127,28 @@ func TestLastCommitForPath(t *testing.T) {
 			},
 		},
 		{
+			desc: "absolute path escaping",
+			request: &gitalypb.LastCommitForPathRequest{
+				Repository: repoProto,
+				Revision:   []byte(latestCommitID),
+				Path:       []byte(repoPath),
+			},
+			expectedErr: testhelper.ToInterceptedMetadata(
+				structerr.NewInvalidArgument("path is an absolute path").WithMetadata("path", repoPath),
+			),
+		},
+		{
+			desc: "relative path escaping root directory",
+			request: &gitalypb.LastCommitForPathRequest{
+				Repository: repoProto,
+				Revision:   []byte(latestCommitID),
+				Path:       []byte("foo/bar/../../../baz"),
+			},
+			expectedErr: testhelper.ToInterceptedMetadata(
+				structerr.NewInvalidArgument("path escapes repository").WithMetadata("path", "foo/bar/../../../baz"),
+			),
+		},
+		{
 			desc: "deleted file",
 			request: &gitalypb.LastCommitForPathRequest{
 				Repository: repoProto,