Merge branch 'validate_list_last_commits' into 'master'

Validate offset/limit for ListLastCommitsForTree

Closes #2214

See merge request gitlab-org/gitaly!1996

5 files changed, 44 insertions, 12 deletions

diff --git a/changelogs/unreleased/validate_list_last_commits.yml b/changelogs/unreleased/validate_list_last_commits.yml
new file mode 100644
index 000000000..af8682b90
--- /dev/null
+++ b/changelogs/unreleased/validate_list_last_commits.yml
@@ -0,0 +1,5 @@
+---
+title: Validate offset/limit for ListLastCommitsForTree
+merge_request: 1996
+author:
+type: fixed
diff --git a/internal/service/commit/list_last_commits_for_tree.go b/internal/service/commit/list_last_commits_for_tree.go
index 75964b229..05535ca9e 100644
--- a/internal/service/commit/list_last_commits_for_tree.go
+++ b/internal/service/commit/list_last_commits_for_tree.go
@@ -1,6 +1,7 @@
 package commit
 
 import (
+	"fmt"
 	"io"
 	"sort"
 
@@ -146,5 +147,14 @@ func sendCommitsForTree(batch []*gitalypb.ListLastCommitsForTreeResponse_CommitF
 }
 
 func validateListLastCommitsForTreeRequest(in *gitalypb.ListLastCommitsForTreeRequest) error {
-	return git.ValidateRevision([]byte(in.Revision))
+	if err := git.ValidateRevision([]byte(in.Revision)); err != nil {
+		return err
+	}
+	if in.GetOffset() < 0 {
+		return fmt.Errorf("offset negative")
+	}
+	if in.GetLimit() < 0 {
+		return fmt.Errorf("limit negative")
+	}
+	return nil
 }
diff --git a/internal/service/commit/list_last_commits_for_tree_test.go b/internal/service/commit/list_last_commits_for_tree_test.go
index c7b611dba..eecb8a6e5 100644
--- a/internal/service/commit/list_last_commits_for_tree_test.go
+++ b/internal/service/commit/list_last_commits_for_tree_test.go
@@ -288,6 +288,26 @@ func TestFailedListLastCommitsForTreeRequest(t *testing.T) {
 			},
 			code: codes.InvalidArgument,
 		},
+		{
+			desc: "Negative offset",
+			request: &gitalypb.ListLastCommitsForTreeRequest{
+				Repository: testRepo,
+				Revision:   "--output=/meow",
+				Offset:     -1,
+				Limit:      25,
+			},
+			code: codes.InvalidArgument,
+		},
+		{
+			desc: "Negative limit",
+			request: &gitalypb.ListLastCommitsForTreeRequest{
+				Repository: testRepo,
+				Revision:   "--output=/meow",
+				Offset:     0,
+				Limit:      -1,
+			},
+			code: codes.InvalidArgument,
+		},
 	}
 
 	for _, testCase := range testCases {
diff --git a/proto/commit.proto b/proto/commit.proto
index cec978db7..02a05a727 100644
--- a/proto/commit.proto
+++ b/proto/commit.proto
@@ -367,8 +367,6 @@ message ListLastCommitsForTreeRequest {
   Repository repository = 1 [(target_repository)=true];
   string revision = 2;
   bytes path = 3;
-
-  // limit == -1 will get the last commit for all paths
   int32 limit = 4;
   int32 offset = 5;
 }
diff --git a/proto/go/gitalypb/commit.pb.go b/proto/go/gitalypb/commit.pb.go
index 52fd4818e..909df8255 100644
--- a/proto/go/gitalypb/commit.pb.go
+++ b/proto/go/gitalypb/commit.pb.go
@@ -1999,15 +1999,14 @@ func (m *LastCommitForPathResponse) GetCommit() *GitCommit {
 }
 
 type ListLastCommitsForTreeRequest struct {
-	Repository *Repository `protobuf:"bytes,1,opt,name=repository,proto3" json:"repository,omitempty"`
-	Revision   string      `protobuf:"bytes,2,opt,name=revision,proto3" json:"revision,omitempty"`
-	Path       []byte      `protobuf:"bytes,3,opt,name=path,proto3" json:"path,omitempty"`
-	// limit == -1 will get the last commit for all paths
-	Limit                int32    `protobuf:"varint,4,opt,name=limit,proto3" json:"limit,omitempty"`
-	Offset               int32    `protobuf:"varint,5,opt,name=offset,proto3" json:"offset,omitempty"`
-	XXX_NoUnkeyedLiteral struct{} `json:"-"`
-	XXX_unrecognized     []byte   `json:"-"`
-	XXX_sizecache        int32    `json:"-"`
+	Repository           *Repository `protobuf:"bytes,1,opt,name=repository,proto3" json:"repository,omitempty"`
+	Revision             string      `protobuf:"bytes,2,opt,name=revision,proto3" json:"revision,omitempty"`
+	Path                 []byte      `protobuf:"bytes,3,opt,name=path,proto3" json:"path,omitempty"`
+	Limit                int32       `protobuf:"varint,4,opt,name=limit,proto3" json:"limit,omitempty"`
+	Offset               int32       `protobuf:"varint,5,opt,name=offset,proto3" json:"offset,omitempty"`
+	XXX_NoUnkeyedLiteral struct{}    `json:"-"`
+	XXX_unrecognized     []byte      `json:"-"`
+	XXX_sizecache        int32       `json:"-"`
 }
 
 func (m *ListLastCommitsForTreeRequest) Reset()         { *m = ListLastCommitsForTreeRequest{} }