diff options
| author | Patrick Steinhardt <psteinhardt@gitlab.com> | 2023-06-05 13:07:35 +0300 |
|---|---|---|
| committer | Patrick Steinhardt <psteinhardt@gitlab.com> | 2023-06-05 14:02:16 +0300 |
| commit | c2a53b00585a5da36b3469d1174aed9ce35856e0 (patch) | |
| tree | 4207d7092cdcf04bebfd1a70a031f0903cec987b | |
| parent | f3d70f4d6c286815b1af11c1cfee86f9dafa0cb0 (diff) | |
commit: Convert `ListCommits()` to use `VerifyRevisions()`
The `ListCommits()` RPC has an open-coded version of revision
validation. This validation is both too lenient and too restrictive as
it does not reject known-bad patterns while also restricting
pseudo-revisions which would be perfectly fine.
Refactor the RPC to use `ValidateRevision()` as our new single source of
truth for revision validation. Note that we do not allow path-scoped
revisions here given that commits cannot ever live at a path anyway.
| -rw-r--r-- | internal/gitaly/service/commit/list_commits.go | 6 | ||||
| -rw-r--r-- | internal/gitaly/service/commit/list_commits_test.go | 12 |
2 files changed, 10 insertions, 8 deletions
diff --git a/internal/gitaly/service/commit/list_commits.go b/internal/gitaly/service/commit/list_commits.go index ea55433c5..00a66a771 100644 --- a/internal/gitaly/service/commit/list_commits.go +++ b/internal/gitaly/service/commit/list_commits.go @@ -2,8 +2,6 @@ package commit import ( "errors" - "fmt" - "strings" "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/git/catfile" @@ -22,8 +20,8 @@ func verifyListCommitsRequest(request *gitalypb.ListCommitsRequest) error { return errors.New("missing revisions") } for _, revision := range request.Revisions { - if strings.HasPrefix(revision, "-") && revision != "--all" && revision != "--not" { - return fmt.Errorf("invalid revision: %q", revision) + if err := git.ValidateRevision([]byte(revision), git.AllowPseudoRevision()); err != nil { + return structerr.NewInvalidArgument("invalid revision: %w", err).WithMetadata("revision", revision) } } return nil diff --git a/internal/gitaly/service/commit/list_commits_test.go b/internal/gitaly/service/commit/list_commits_test.go index 2563f3c71..e06302179 100644 --- a/internal/gitaly/service/commit/list_commits_test.go +++ b/internal/gitaly/service/commit/list_commits_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest" + "gitlab.com/gitlab-org/gitaly/v16/internal/structerr" "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb" "google.golang.org/grpc/codes" @@ -338,12 +339,15 @@ func TestListCommits_verify(t *testing.T) { { desc: "no revisions", req: &gitalypb.ListCommitsRequest{Repository: repo}, - expectedErr: status.Error(codes.InvalidArgument, "missing revisions"), + expectedErr: structerr.NewInvalidArgument("missing revisions"), }, { - desc: "invalid revision", - req: &gitalypb.ListCommitsRequest{Repository: repo, Revisions: []string{"asdf", "-invalid"}}, - expectedErr: status.Error(codes.InvalidArgument, `invalid revision: "-invalid"`), + desc: "invalid revision", + req: &gitalypb.ListCommitsRequest{Repository: repo, Revisions: []string{"asdf", "-invalid"}}, + expectedErr: testhelper.WithInterceptedMetadata( + structerr.NewInvalidArgument("invalid revision: revision can't start with '-'"), + "revision", "-invalid", + ), }, } { t.Run(tc.desc, func(t *testing.T) { |