diff options
| author | Patrick Steinhardt <psteinhardt@gitlab.com> | 2023-06-05 13:07:35 +0300 |
|---|---|---|
| committer | Patrick Steinhardt <psteinhardt@gitlab.com> | 2023-06-05 13:44:27 +0300 |
| commit | 371898d2865d734de2f73d0ad76cacc9c6d420b4 (patch) | |
| tree | e4c69cd6a633fd380f864d2ee0ede50df4a5b2f8 | |
| parent | c1b8d6a09b3b5ed69c1931ee6b95842a5a9dff58 (diff) | |
blob: Convert ListBlobs to use `VerifyRevisions()`
The `ListBlobs()` 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.
| -rw-r--r-- | internal/gitaly/service/blob/blobs.go | 7 | ||||
| -rw-r--r-- | internal/gitaly/service/blob/blobs_test.go | 10 |
2 files changed, 9 insertions, 8 deletions
diff --git a/internal/gitaly/service/blob/blobs.go b/internal/gitaly/service/blob/blobs.go index c477e3ef7..5fb3e5efa 100644 --- a/internal/gitaly/service/blob/blobs.go +++ b/internal/gitaly/service/blob/blobs.go @@ -3,10 +3,9 @@ package blob import ( "context" "errors" - "fmt" "io" - "strings" + "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/git/catfile" "gitlab.com/gitlab-org/gitaly/v16/internal/git/gitpipe" "gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo" @@ -26,8 +25,8 @@ func verifyListBlobsRequest(req *gitalypb.ListBlobsRequest) error { return errors.New("missing revisions") } for _, revision := range req.Revisions { - if strings.HasPrefix(revision, "-") && revision != "--all" && revision != "--not" { - return fmt.Errorf("invalid revision: %q", revision) + if err := git.ValidateRevision([]byte(revision), git.AllowPathScopedRevision(), git.AllowPseudoRevision()); err != nil { + return structerr.NewInvalidArgument("invalid revision: %w", err).WithMetadata("revision", revision) } } return nil diff --git a/internal/gitaly/service/blob/blobs_test.go b/internal/gitaly/service/blob/blobs_test.go index de71c122d..78dcd2bf5 100644 --- a/internal/gitaly/service/blob/blobs_test.go +++ b/internal/gitaly/service/blob/blobs_test.go @@ -10,11 +10,10 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest" "gitlab.com/gitlab-org/gitaly/v16/internal/git/quarantine" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config" + "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" "gitlab.com/gitlab-org/gitaly/v16/streamio" - "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" "google.golang.org/protobuf/proto" ) @@ -64,14 +63,17 @@ func TestListBlobs(t *testing.T) { { desc: "missing revisions", revisions: []string{}, - expectedErr: status.Error(codes.InvalidArgument, "missing revisions"), + expectedErr: structerr.NewInvalidArgument("missing revisions"), }, { desc: "invalid revision", revisions: []string{ "--foobar", }, - expectedErr: status.Error(codes.InvalidArgument, "invalid revision: \"--foobar\""), + expectedErr: testhelper.WithInterceptedMetadata( + structerr.NewInvalidArgument("invalid revision: revision can't start with '-'"), + "revision", "--foobar", + ), }, { desc: "single blob", |