From 2456058252034ff6a4f1d0b62bc468bdf3652f99 Mon Sep 17 00:00:00 2001 From: Alirie Gray Date: Tue, 4 Aug 2020 22:05:18 -0700 Subject: [PATCH] fix: add quotation marks to bucket name validation --- CHANGELOG.md | 1 + http/bucket_service.go | 5 +- tenant/http_server_bucket.go | 15 ++-- tenant/http_server_bucket_test.go | 2 +- testing/bucket_service.go | 113 ++++++++++++++++++++++++++++-- 5 files changed, 120 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2289fd18331..acb0fd266d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ 1. [19043](https://github.com/influxdata/influxdb/pull/19043): Enforce all influx CLI flag args are valid 1. [19188](https://github.com/influxdata/influxdb/pull/19188): Dashboard cells correctly map results when multiple queries exist 1. [19146](https://github.com/influxdata/influxdb/pull/19146): Dashboard cells and overlay use UTC as query time when toggling to UTC timezone +1. [19222](https://github.com/influxdata/influxdb/pull/19222): Bucket names may not include quotation marks ## v2.0.0-beta.15 [2020-07-23] diff --git a/http/bucket_service.go b/http/bucket_service.go index d0548bb8663..1819a887491 100644 --- a/http/bucket_service.go +++ b/http/bucket_service.go @@ -361,10 +361,7 @@ func (b *postBucketRequest) OK() error { // names starting with an underscore are reserved for system buckets if err := validBucketName(b.toInfluxDB()); err != nil { - return &influxdb.Error{ - Code: influxdb.EUnprocessableEntity, - Msg: err.Error(), - } + return err } return nil diff --git a/tenant/http_server_bucket.go b/tenant/http_server_bucket.go index 5fdb1811d30..a8b6b36bd43 100644 --- a/tenant/http_server_bucket.go +++ b/tenant/http_server_bucket.go @@ -313,10 +313,7 @@ func (b *postBucketRequest) OK() error { // names starting with an underscore are reserved for system buckets if err := validBucketName(b.toInfluxDB()); err != nil { - return &influxdb.Error{ - Code: influxdb.EUnprocessableEntity, - Msg: err.Error(), - } + return err } return nil @@ -493,10 +490,18 @@ func validBucketName(bucket *influxdb.Bucket) error { // names starting with an underscore are reserved for system buckets if strings.HasPrefix(bucket.Name, "_") && bucket.Type != influxdb.BucketTypeSystem { return &influxdb.Error{ - Code: influxdb.EInvalid, + Code: influxdb.EUnprocessableEntity, Op: "http/bucket", Msg: fmt.Sprintf("bucket name %s is invalid. Buckets may not start with underscore", bucket.Name), } } + // quotation marks will cause queries to fail + if strings.Contains(bucket.Name, "\"") { + return &influxdb.Error{ + Code: influxdb.EUnprocessableEntity, + Op: "http/bucket", + Msg: fmt.Sprintf("bucket name %s is invalid. Bucket names may not include quotation marks", bucket.Name), + } + } return nil } diff --git a/tenant/http_server_bucket_test.go b/tenant/http_server_bucket_test.go index a6a7ae5bfd2..3864be450e6 100644 --- a/tenant/http_server_bucket_test.go +++ b/tenant/http_server_bucket_test.go @@ -60,5 +60,5 @@ func initBucketHttpService(f itesting.BucketFields, t *testing.T) (influxdb.Buck } func TestBucketService(t *testing.T) { - itesting.BucketService(initBucketHttpService, t, itesting.WithoutHooks()) + itesting.BucketService(initBucketHttpService, t, itesting.WithoutHooks(), itesting.WithHTTPValidation()) } diff --git a/testing/bucket_service.go b/testing/bucket_service.go index 87a54b7de83..147063c01b0 100644 --- a/testing/bucket_service.go +++ b/testing/bucket_service.go @@ -47,14 +47,24 @@ var bucketCmpOptions = cmp.Options{ } type BucketSvcOpts struct { - NoHooks bool + NoHooks bool + HTTPValidation bool } +type BucketOptsFn func(opts *BucketSvcOpts) + // WithoutHooks allows the test suite to be run without being able to hook into the underlying implementation of theservice // in most cases that is to remove specific id generation controls. -func WithoutHooks() BucketSvcOpts { - return BucketSvcOpts{ - NoHooks: true, +func WithoutHooks() BucketOptsFn { + return func(opts *BucketSvcOpts) { + opts.NoHooks = true + } +} + +// WithHTTPValidation skips tests involving name validation that only occurs at the HTTP layer +func WithHTTPValidation() BucketOptsFn { + return func(opts *BucketSvcOpts) { + opts.HTTPValidation = true } } @@ -76,7 +86,7 @@ type bucketServiceF func( func BucketService( init func(BucketFields, *testing.T) (influxdb.BucketService, string, func()), t *testing.T, - opts ...BucketSvcOpts) { + opts ...BucketOptsFn) { tests := []struct { name string fn bucketServiceF @@ -89,6 +99,10 @@ func BucketService( name: "IDUnique", fn: IDUnique, }, + { + name: "HTTPValidation", + fn: HTTPValidation, + }, { name: "FindBucketByID", fn: FindBucketByID, @@ -110,8 +124,19 @@ func BucketService( fn: DeleteBucket, }, } + opt := BucketSvcOpts{ + NoHooks: false, + HTTPValidation: false, + } + for _, o := range opts { + o(&opt) + } + for _, tt := range tests { - if tt.name == "IDUnique" && len(opts) > 0 && opts[0].NoHooks { + if tt.name == "IDUnique" && opt.NoHooks { + continue + } + if tt.name == "HTTPValidation" && !opt.HTTPValidation { continue } t.Run(tt.name, func(t *testing.T) { @@ -385,6 +410,82 @@ func CreateBucket( } } +func HTTPValidation( + init func(BucketFields, *testing.T) (influxdb.BucketService, string, func()), + t *testing.T, +) { + type args struct { + bucket *influxdb.Bucket + } + type wants struct { + err error + buckets []*influxdb.Bucket + } + tests := []struct { + name string + fields BucketFields + args args + wants wants + }{ + { + name: "create bucket with illegal quotation mark", + fields: BucketFields{ + IDGenerator: mock.NewIDGenerator(bucketOneID, t), + OrgBucketIDs: mock.NewIDGenerator(bucketOneID, t), + TimeGenerator: mock.TimeGenerator{FakeValue: time.Date(2006, 5, 4, 1, 2, 3, 0, time.UTC)}, + Buckets: []*influxdb.Bucket{}, + Organizations: []*influxdb.Organization{}, + }, + args: args{ + bucket: &influxdb.Bucket{ + Name: "namewith\"quote", + OrgID: MustIDBase16(orgOneID), + }, + }, + wants: wants{ + buckets: []*influxdb.Bucket{}, + err: &influxdb.Error{ + Code: influxdb.EUnprocessableEntity, + Msg: "bucket name namewith\"quote is invalid. Bucket names may not include quotation marks", + Op: influxdb.OpCreateBucket, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s, opPrefix, done := init(tt.fields, t) + defer done() + ctx := context.Background() + err := s.CreateBucket(ctx, tt.args.bucket) + diffPlatformErrors(tt.name, err, tt.wants.err, opPrefix, t) + + // Delete only newly created buckets - ie., with a not nil ID + // if tt.args.bucket.ID.Valid() { + defer s.DeleteBucket(ctx, tt.args.bucket.ID) + // } + + buckets, _, err := s.FindBuckets(ctx, influxdb.BucketFilter{}) + if err != nil { + t.Fatalf("failed to retrieve buckets: %v", err) + } + + // remove system buckets + filteredBuckets := []*influxdb.Bucket{} + for _, b := range buckets { + if b.Type != influxdb.BucketTypeSystem { + filteredBuckets = append(filteredBuckets, b) + } + } + + if diff := cmp.Diff(filteredBuckets, tt.wants.buckets, bucketCmpOptions...); diff != "" { + t.Errorf("buckets are different -got/+want\ndiff %s", diff) + } + }) + } +} + // CreateBucket testing func IDUnique( init func(BucketFields, *testing.T) (influxdb.BucketService, string, func()),