Merge branch 'pks-last-commit-for-path-invalid-absolute-paths' into 'master'

commit: Gracefully handle paths escaping repo in LastCommitForPath

Closes #5575

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6345

Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: James Liu <jliu@gitlab.com>
Reviewed-by: James Liu <jliu@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>

5 files changed, 78 insertions, 25 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,
diff --git a/proto/commit.proto b/proto/commit.proto
index a65523749..919ede288 100644
--- a/proto/commit.proto
+++ b/proto/commit.proto
@@ -115,7 +115,12 @@ service CommitService {
     };
   }
 
-  // This comment is left unintentionally blank.
+  // LastCommitForPath returns the last commit that has changed a given path.
+  //
+  // The following special cases apply and have grown historically:
+  //
+  // - Absolute paths that or relative paths that escape the repository root will cause an error.
+  // - A nonexistent path inside the repostiory leads to a successful but empty response.
   rpc LastCommitForPath(LastCommitForPathRequest) returns (LastCommitForPathResponse) {
     option (op_type) = {
       op: ACCESSOR
@@ -696,25 +701,26 @@ message RawBlameResponse {
   bytes data = 1;
 }
 
-// This comment is left unintentionally blank.
+// LastCommitForPathRequest is a request for the LastCommitForPath RPC.
 message LastCommitForPathRequest {
-  // This comment is left unintentionally blank.
+  // Repository is the repository to run the query in.
   Repository repository = 1 [(target_repository)=true];
-  // This comment is left unintentionally blank.
+  // Revision is the committish that is used as the start commit to perform the search.
   bytes revision = 2;
-  // This comment is left unintentionally blank.
-  // This comment is left unintentionally blank.
-  // This comment is left unintentionally blank.
+  // Path is the path for which the last commit should be searched. This path can either point to a blob or to a
+  // tree. The path must be relative and must not escape the repository root. If the path is empty or "/", then the
+  // repository root will be searched instead.
   bytes path = 3;
-  // This comment is left unintentionally blank.
-  bool literal_pathspec = 4; // Deprecate after Rails stops using this
-  // This comment is left unintentionally blank.
+  // LiteralPathspec will treat the path literally. No globbing or pathspec magic is performed. This option is
+  // deprecated in favor of GlobalOptions.
+  bool literal_pathspec = 4;
+  // GlobalOptions contains the global options used to modify the behaviour of Git.
   GlobalOptions global_options = 5;
 }
 
-// This comment is left unintentionally blank.
+// LastCommitForPathResponse is a response for the LastCommitForPath RPC.
 message LastCommitForPathResponse {
-  // commit is nil when the commit was not found
+  // Commit is the commit that has last modified the given path. Unset in case the path could not be found.
   GitCommit commit = 1;
 }
 
diff --git a/proto/go/gitalypb/commit.pb.go b/proto/go/gitalypb/commit.pb.go
index be6c3a72b..6f6a9b8c1 100644
--- a/proto/go/gitalypb/commit.pb.go
+++ b/proto/go/gitalypb/commit.pb.go
@@ -2845,23 +2845,24 @@ func (x *RawBlameResponse) GetData() []byte {
 	return nil
 }
 
-// This comment is left unintentionally blank.
+// LastCommitForPathRequest is a request for the LastCommitForPath RPC.
 type LastCommitForPathRequest struct {
 	state         protoimpl.MessageState
 	sizeCache     protoimpl.SizeCache
 	unknownFields protoimpl.UnknownFields
 
-	// This comment is left unintentionally blank.
+	// Repository is the repository to run the query in.
 	Repository *Repository `protobuf:"bytes,1,opt,name=repository,proto3" json:"repository,omitempty"`
-	// This comment is left unintentionally blank.
+	// Revision is the committish that is used as the start commit to perform the search.
 	Revision []byte `protobuf:"bytes,2,opt,name=revision,proto3" json:"revision,omitempty"`
-	// This comment is left unintentionally blank.
-	// This comment is left unintentionally blank.
-	// This comment is left unintentionally blank.
+	// Path is the path for which the last commit should be searched. This path can either point to a blob or to a
+	// tree. The path must be relative and must not escape the repository root. If the path is empty or "/", then the
+	// repository root will be searched instead.
 	Path []byte `protobuf:"bytes,3,opt,name=path,proto3" json:"path,omitempty"`
-	// This comment is left unintentionally blank.
-	LiteralPathspec bool `protobuf:"varint,4,opt,name=literal_pathspec,json=literalPathspec,proto3" json:"literal_pathspec,omitempty"` // Deprecate after Rails stops using this
-	// This comment is left unintentionally blank.
+	// LiteralPathspec will treat the path literally. No globbing or pathspec magic is performed. This option is
+	// deprecated in favor of GlobalOptions.
+	LiteralPathspec bool `protobuf:"varint,4,opt,name=literal_pathspec,json=literalPathspec,proto3" json:"literal_pathspec,omitempty"`
+	// GlobalOptions contains the global options used to modify the behaviour of Git.
 	GlobalOptions *GlobalOptions `protobuf:"bytes,5,opt,name=global_options,json=globalOptions,proto3" json:"global_options,omitempty"`
 }
 
@@ -2932,13 +2933,13 @@ func (x *LastCommitForPathRequest) GetGlobalOptions() *GlobalOptions {
 	return nil
 }
 
-// This comment is left unintentionally blank.
+// LastCommitForPathResponse is a response for the LastCommitForPath RPC.
 type LastCommitForPathResponse struct {
 	state         protoimpl.MessageState
 	sizeCache     protoimpl.SizeCache
 	unknownFields protoimpl.UnknownFields
 
-	// commit is nil when the commit was not found
+	// Commit is the commit that has last modified the given path. Unset in case the path could not be found.
 	Commit *GitCommit `protobuf:"bytes,1,opt,name=commit,proto3" json:"commit,omitempty"`
 }
 
diff --git a/proto/go/gitalypb/commit_grpc.pb.go b/proto/go/gitalypb/commit_grpc.pb.go
index 4a9e7a668..e3aeeaa06 100644
--- a/proto/go/gitalypb/commit_grpc.pb.go
+++ b/proto/go/gitalypb/commit_grpc.pb.go
@@ -54,7 +54,12 @@ type CommitServiceClient interface {
 	CommitLanguages(ctx context.Context, in *CommitLanguagesRequest, opts ...grpc.CallOption) (*CommitLanguagesResponse, error)
 	// This comment is left unintentionally blank.
 	RawBlame(ctx context.Context, in *RawBlameRequest, opts ...grpc.CallOption) (CommitService_RawBlameClient, error)
-	// This comment is left unintentionally blank.
+	// LastCommitForPath returns the last commit that has changed a given path.
+	//
+	// The following special cases apply and have grown historically:
+	//
+	// - Absolute paths that or relative paths that escape the repository root will cause an error.
+	// - A nonexistent path inside the repostiory leads to a successful but empty response.
 	LastCommitForPath(ctx context.Context, in *LastCommitForPathRequest, opts ...grpc.CallOption) (*LastCommitForPathResponse, error)
 	// This comment is left unintentionally blank.
 	ListLastCommitsForTree(ctx context.Context, in *ListLastCommitsForTreeRequest, opts ...grpc.CallOption) (CommitService_ListLastCommitsForTreeClient, error)
@@ -694,7 +699,12 @@ type CommitServiceServer interface {
 	CommitLanguages(context.Context, *CommitLanguagesRequest) (*CommitLanguagesResponse, error)
 	// This comment is left unintentionally blank.
 	RawBlame(*RawBlameRequest, CommitService_RawBlameServer) error
-	// This comment is left unintentionally blank.
+	// LastCommitForPath returns the last commit that has changed a given path.
+	//
+	// The following special cases apply and have grown historically:
+	//
+	// - Absolute paths that or relative paths that escape the repository root will cause an error.
+	// - A nonexistent path inside the repostiory leads to a successful but empty response.
 	LastCommitForPath(context.Context, *LastCommitForPathRequest) (*LastCommitForPathResponse, error)
 	// This comment is left unintentionally blank.
 	ListLastCommitsForTree(*ListLastCommitsForTreeRequest, CommitService_ListLastCommitsForTreeServer) error