From 2365f9f1ae848fad9365dbf98425d4bde9e8f57a Mon Sep 17 00:00:00 2001 From: niksis02 Date: Wed, 4 Feb 2026 19:29:56 +0400 Subject: [PATCH] fix: fixes list-limiters parsing and validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #1809 Fixes #1806 Fixes #1804 Fixes #1794 This PR focuses on correcting so-called "list-limiter" parsing and validation. The affected limiters include: `max-keys`, `max-uploads`, `max-parts`, `max-buckets`, `max-uploads` and `part-number-marker`. When a limiter value is outside the integer range, a specific `InvalidArgument` error is now returned. If the value is a valid integer but negative, a different `InvalidArgument` error is produced. `max-buckets` has its own validation rules: completely invalid values and values outside the allowed range (`1 <= input <= 10000`) return distinct errors. For `ListObjectVersions`, negative `max-keys` values follow S3’s special-case behavior and return a different `InvalidArgument` error message. Additionally, `GetObjectAttributes` now follows S3 semantics for `x-amz-max-parts`: S3 ignores invalid values, so the gateway now matches that behavior. --- backend/azure/azure.go | 2 +- backend/posix/posix.go | 2 +- s3api/controllers/base.go | 7 +- s3api/controllers/bucket-get.go | 26 +--- s3api/controllers/bucket-get_test.go | 16 +- s3api/controllers/bucket-list.go | 19 +-- s3api/controllers/bucket-list_test.go | 47 +++++- s3api/controllers/object-get.go | 49 +++--- s3api/controllers/object-get_test.go | 25 +-- s3api/utils/utils.go | 78 ++++++++-- s3api/utils/utils_test.go | 177 ++++++++++++++++++---- s3err/s3err.go | 40 +++-- tests/integration/ListMultipartUploads.go | 2 +- tests/integration/ListObjectVersions.go | 13 ++ tests/integration/ListObjects.go | 2 +- tests/integration/ListParts.go | 43 +++++- tests/integration/group-tests.go | 4 + 17 files changed, 387 insertions(+), 165 deletions(-) diff --git a/backend/azure/azure.go b/backend/azure/azure.go index c89054e2..7846c1f8 100644 --- a/backend/azure/azure.go +++ b/backend/azure/azure.go @@ -1298,7 +1298,7 @@ func (az *Azure) ListParts(ctx context.Context, input *s3.ListPartsInput) (s3res if *input.PartNumberMarker != "" { partNumberMarker, err = strconv.Atoi(*input.PartNumberMarker) if err != nil { - return s3response.ListPartsResult{}, s3err.GetAPIError(s3err.ErrInvalidPartNumberMarker) + return s3response.ListPartsResult{}, s3err.GetInvalidMaxLimiterErr("part-number-marker") } } if input.MaxParts != nil { diff --git a/backend/posix/posix.go b/backend/posix/posix.go index 89112f96..0d980a55 100644 --- a/backend/posix/posix.go +++ b/backend/posix/posix.go @@ -2325,7 +2325,7 @@ func (p *Posix) ListParts(ctx context.Context, input *s3.ListPartsInput) (s3resp var err error partNumberMarker, err = strconv.Atoi(stringMarker) if err != nil { - return lpr, s3err.GetAPIError(s3err.ErrInvalidPartNumberMarker) + return lpr, s3err.GetInvalidMaxLimiterErr("part-number-marker") } } diff --git a/s3api/controllers/base.go b/s3api/controllers/base.go index 01270a9f..62c69110 100644 --- a/s3api/controllers/base.go +++ b/s3api/controllers/base.go @@ -47,10 +47,9 @@ const ( iso8601TimeFormatExtended = "Mon Jan _2 15:04:05 2006" timefmt = "Mon, 02 Jan 2006 15:04:05 GMT" - maxXMLBodyLen = 4 * 1024 * 1024 - minPartNumber = 1 - maxPartNumber = 10000 - defaultMaxBuckets = int32(10000) + maxXMLBodyLen = 4 * 1024 * 1024 + minPartNumber = 1 + maxPartNumber = 10000 defaultRegion = "us-east-1" ) diff --git a/s3api/controllers/bucket-get.go b/s3api/controllers/bucket-get.go index 0cefab6f..aa5194a7 100644 --- a/s3api/controllers/bucket-get.go +++ b/s3api/controllers/bucket-get.go @@ -21,9 +21,7 @@ import ( "github.com/aws/aws-sdk-go-v2/service/s3/types" "github.com/gofiber/fiber/v2" "github.com/versity/versitygw/auth" - "github.com/versity/versitygw/debuglogger" "github.com/versity/versitygw/s3api/utils" - "github.com/versity/versitygw/s3err" "github.com/versity/versitygw/s3response" ) @@ -324,15 +322,13 @@ func (c S3ApiController) ListObjectVersions(ctx *fiber.Ctx) (*Response, error) { }, err } - maxkeys, err := utils.ParseUint(maxkeysStr) + maxkeys, err := utils.ParseMaxLimiter(maxkeysStr, utils.LimiterTypeVersionsMaxKeys) if err != nil { - debuglogger.Logf("error parsing max keys %q: %v", - maxkeysStr, err) return &Response{ MetaOpts: &MetaOptions{ BucketOwner: parsedAcl.Owner, }, - }, s3err.GetAPIError(s3err.ErrInvalidMaxKeys) + }, err } data, err := c.be.ListObjectVersions(ctx.Context(), @@ -474,15 +470,13 @@ func (c S3ApiController) ListMultipartUploads(ctx *fiber.Ctx) (*Response, error) }, }, err } - maxUploads, err := utils.ParseUint(maxUploadsStr) + maxUploads, err := utils.ParseMaxLimiter(maxUploadsStr, utils.LimiterTypeMaxUploads) if err != nil { - debuglogger.Logf("error parsing max uploads %q: %v", - maxUploadsStr, err) return &Response{ MetaOpts: &MetaOptions{ BucketOwner: parsedAcl.Owner, }, - }, s3err.GetAPIError(s3err.ErrInvalidMaxUploads) + }, err } res, err := c.be.ListMultipartUploads(ctx.Context(), &s3.ListMultipartUploadsInput{ @@ -533,15 +527,13 @@ func (c S3ApiController) ListObjectsV2(ctx *fiber.Ctx) (*Response, error) { }, }, err } - maxkeys, err := utils.ParseUint(maxkeysStr) + maxkeys, err := utils.ParseMaxLimiter(maxkeysStr, utils.LimiterTypeMaxKeys) if err != nil { - debuglogger.Logf("error parsing max keys %q: %v", - maxkeysStr, err) return &Response{ MetaOpts: &MetaOptions{ BucketOwner: parsedAcl.Owner, }, - }, s3err.GetAPIError(s3err.ErrInvalidMaxKeys) + }, err } res, err := c.be.ListObjectsV2(ctx.Context(), @@ -593,15 +585,13 @@ func (c S3ApiController) ListObjects(ctx *fiber.Ctx) (*Response, error) { }, err } - maxkeys, err := utils.ParseUint(maxkeysStr) + maxkeys, err := utils.ParseMaxLimiter(maxkeysStr, utils.LimiterTypeMaxKeys) if err != nil { - debuglogger.Logf("error parsing max keys %q: %v", - maxkeysStr, err) return &Response{ MetaOpts: &MetaOptions{ BucketOwner: parsedAcl.Owner, }, - }, s3err.GetAPIError(s3err.ErrInvalidMaxKeys) + }, err } res, err := c.be.ListObjects(ctx.Context(), diff --git a/s3api/controllers/bucket-get_test.go b/s3api/controllers/bucket-get_test.go index 1e675bc5..5ce2722b 100644 --- a/s3api/controllers/bucket-get_test.go +++ b/s3api/controllers/bucket-get_test.go @@ -654,7 +654,7 @@ func TestS3ApiController_ListObjectVersions(t *testing.T) { input: testInput{ locals: defaultLocals, queries: map[string]string{ - "max-keys": "-1", + "max-keys": "invalid", }, }, output: testOutput{ @@ -663,7 +663,7 @@ func TestS3ApiController_ListObjectVersions(t *testing.T) { BucketOwner: "root", }, }, - err: s3err.GetAPIError(s3err.ErrInvalidMaxKeys), + err: s3err.GetInvalidMaxLimiterErr(utils.LimiterTypeMaxKeys), }, }, { @@ -979,7 +979,7 @@ func TestS3ApiController_ListMultipartUploads(t *testing.T) { input: testInput{ locals: defaultLocals, queries: map[string]string{ - "max-uploads": "-1", + "max-uploads": "invalid", }, }, output: testOutput{ @@ -988,7 +988,7 @@ func TestS3ApiController_ListMultipartUploads(t *testing.T) { BucketOwner: "root", }, }, - err: s3err.GetAPIError(s3err.ErrInvalidMaxUploads), + err: s3err.GetInvalidMaxLimiterErr(utils.LimiterTypeMaxUploads), }, }, { @@ -1082,7 +1082,7 @@ func TestS3ApiController_ListObjectsV2(t *testing.T) { }, }, { - name: "invalid max keys", + name: "negative max keys", input: testInput{ locals: defaultLocals, queries: map[string]string{ @@ -1095,7 +1095,7 @@ func TestS3ApiController_ListObjectsV2(t *testing.T) { BucketOwner: "root", }, }, - err: s3err.GetAPIError(s3err.ErrInvalidMaxKeys), + err: s3err.GetNegativeMaxLimiterErr(utils.LimiterTypeMaxKeys), }, }, { @@ -1193,7 +1193,7 @@ func TestS3ApiController_ListObjects(t *testing.T) { input: testInput{ locals: defaultLocals, queries: map[string]string{ - "max-keys": "-1", + "max-keys": "bla", }, }, output: testOutput{ @@ -1202,7 +1202,7 @@ func TestS3ApiController_ListObjects(t *testing.T) { BucketOwner: "root", }, }, - err: s3err.GetAPIError(s3err.ErrInvalidMaxKeys), + err: s3err.GetInvalidMaxLimiterErr(utils.LimiterTypeMaxKeys), }, }, { diff --git a/s3api/controllers/bucket-list.go b/s3api/controllers/bucket-list.go index 07cc9f80..0f7663be 100644 --- a/s3api/controllers/bucket-list.go +++ b/s3api/controllers/bucket-list.go @@ -15,13 +15,9 @@ package controllers import ( - "strconv" - "github.com/gofiber/fiber/v2" "github.com/versity/versitygw/auth" - "github.com/versity/versitygw/debuglogger" "github.com/versity/versitygw/s3api/utils" - "github.com/versity/versitygw/s3err" "github.com/versity/versitygw/s3response" ) @@ -35,16 +31,11 @@ func (c S3ApiController) ListBuckets(ctx *fiber.Ctx) (*Response, error) { region = defaultRegion } - maxBuckets := defaultMaxBuckets - if maxBucketsStr != "" { - maxBucketsParsed, err := strconv.ParseInt(maxBucketsStr, 10, 32) - if err != nil || maxBucketsParsed < 0 || maxBucketsParsed > int64(defaultMaxBuckets) { - debuglogger.Logf("error parsing max-buckets %q: %v", maxBucketsStr, err) - return &Response{ - MetaOpts: &MetaOptions{}, - }, s3err.GetAPIError(s3err.ErrInvalidMaxBuckets) - } - maxBuckets = int32(maxBucketsParsed) + maxBuckets, err := utils.ParseMaxLimiter(maxBucketsStr, utils.LimiterTypeMaxBuckets) + if err != nil { + return &Response{ + MetaOpts: &MetaOptions{}, + }, err } res, err := c.be.ListBuckets(ctx.Context(), diff --git a/s3api/controllers/bucket-list_test.go b/s3api/controllers/bucket-list_test.go index 3406de2d..6de64afd 100644 --- a/s3api/controllers/bucket-list_test.go +++ b/s3api/controllers/bucket-list_test.go @@ -40,7 +40,7 @@ func TestS3ApiController_ListBuckets(t *testing.T) { output testOutput }{ { - name: "invalid max buckets", + name: "negative max buckets", input: testInput{ locals: defaultLocals, queries: map[string]string{ @@ -54,6 +54,51 @@ func TestS3ApiController_ListBuckets(t *testing.T) { err: s3err.GetAPIError(s3err.ErrInvalidMaxBuckets), }, }, + { + name: "max buckets exceeds upper limit", + input: testInput{ + locals: defaultLocals, + queries: map[string]string{ + "max-buckets": "10001", + }, + }, + output: testOutput{ + response: &Response{ + MetaOpts: &MetaOptions{}, + }, + err: s3err.GetAPIError(s3err.ErrInvalidMaxBuckets), + }, + }, + { + name: "max buckets 0", + input: testInput{ + locals: defaultLocals, + queries: map[string]string{ + "max-buckets": "0", + }, + }, + output: testOutput{ + response: &Response{ + MetaOpts: &MetaOptions{}, + }, + err: s3err.GetAPIError(s3err.ErrInvalidMaxBuckets), + }, + }, + { + name: "invalid max buckets", + input: testInput{ + locals: defaultLocals, + queries: map[string]string{ + "max-buckets": "bla", + }, + }, + output: testOutput{ + response: &Response{ + MetaOpts: &MetaOptions{}, + }, + err: s3err.GetInvalidMaxLimiterErr("max-buckets"), + }, + }, { name: "backend returns error", input: testInput{ diff --git a/s3api/controllers/object-get.go b/s3api/controllers/object-get.go index b54a82fd..828f085c 100644 --- a/s3api/controllers/object-get.go +++ b/s3api/controllers/object-get.go @@ -18,7 +18,6 @@ import ( "fmt" "math" "net/http" - "strconv" "strings" "time" @@ -275,31 +274,24 @@ func (c S3ApiController) ListParts(ctx *fiber.Ctx) (*Response, error) { }, err } - // parse the part number marker - if partNumberMarker != "" { - n, err := strconv.Atoi(partNumberMarker) - if err != nil || n < 0 { - debuglogger.Logf("invalid part number marker %q: %v", - partNumberMarker, err) - - return &Response{ - MetaOpts: &MetaOptions{ - BucketOwner: parsedAcl.Owner, - }, - }, s3err.GetAPIError(s3err.ErrInvalidPartNumberMarker) - } - } - - // parse the max parts - maxParts, err := utils.ParseUint(maxPartsStr) + // parse/validate the part number marker + _, err = utils.ParseMaxLimiter(partNumberMarker, utils.LimiterTypePartNumberMarker) if err != nil { - debuglogger.Logf("error parsing max parts %q: %v", - maxPartsStr, err) return &Response{ MetaOpts: &MetaOptions{ BucketOwner: parsedAcl.Owner, }, - }, s3err.GetAPIError(s3err.ErrInvalidMaxParts) + }, err + } + + // parse the max parts + maxParts, err := utils.ParseMaxLimiter(maxPartsStr, utils.LimiterTypeMaxParts) + if err != nil { + return &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: parsedAcl.Owner, + }, + }, err } res, err := c.be.ListParts(ctx.Context(), &s3.ListPartsInput{ @@ -362,16 +354,11 @@ func (c S3ApiController) GetObjectAttributes(ctx *fiber.Ctx) (*Response, error) }, err } + var maxParts *int32 // parse max parts - maxParts, err := utils.ParseUint(maxPartsStr) - if err != nil { - debuglogger.Logf("error parsing max parts %q: %v", - maxPartsStr, err) - return &Response{ - MetaOpts: &MetaOptions{ - BucketOwner: parsedAcl.Owner, - }, - }, s3err.GetAPIError(s3err.ErrInvalidMaxParts) + parsed, err := utils.ParseMaxLimiter(maxPartsStr, utils.LimiterTypeMaxParts) + if err == nil { + maxParts = &parsed } // parse the object attributes @@ -389,7 +376,7 @@ func (c S3ApiController) GetObjectAttributes(ctx *fiber.Ctx) (*Response, error) Bucket: &bucket, Key: &key, PartNumberMarker: &partNumberMarker, - MaxParts: &maxParts, + MaxParts: maxParts, VersionId: &versionId, }) if err != nil { diff --git a/s3api/controllers/object-get_test.go b/s3api/controllers/object-get_test.go index 2c45a03f..f8e89d0c 100644 --- a/s3api/controllers/object-get_test.go +++ b/s3api/controllers/object-get_test.go @@ -497,7 +497,7 @@ func TestS3ApiController_ListParts(t *testing.T) { input: testInput{ locals: defaultLocals, queries: map[string]string{ - "part-number-marker": "-1", + "part-number-marker": "foo", }, }, output: testOutput{ @@ -506,11 +506,11 @@ func TestS3ApiController_ListParts(t *testing.T) { BucketOwner: "root", }, }, - err: s3err.GetAPIError(s3err.ErrInvalidPartNumberMarker), + err: s3err.GetInvalidMaxLimiterErr(utils.LimiterTypePartNumberMarker), }, }, { - name: "invalid max parts", + name: "negative max parts", input: testInput{ locals: defaultLocals, queries: map[string]string{ @@ -523,7 +523,7 @@ func TestS3ApiController_ListParts(t *testing.T) { BucketOwner: "root", }, }, - err: s3err.GetAPIError(s3err.ErrInvalidMaxParts), + err: s3err.GetNegativeMaxLimiterErr(utils.LimiterTypeMaxParts), }, }, { @@ -634,23 +634,6 @@ func TestS3ApiController_GetObjectAttributes(t *testing.T) { err: s3err.GetAPIError(s3err.ErrInvalidVersionId), }, }, - { - name: "invalid max parts", - input: testInput{ - locals: defaultLocals, - headers: map[string]string{ - "X-Amz-Max-Parts": "-1", - }, - }, - output: testOutput{ - response: &Response{ - MetaOpts: &MetaOptions{ - BucketOwner: "root", - }, - }, - err: s3err.GetAPIError(s3err.ErrInvalidMaxParts), - }, - }, { name: "invalid object attributes", input: testInput{ diff --git a/s3api/utils/utils.go b/s3api/utils/utils.go index cbb97196..fdefdaca 100644 --- a/s3api/utils/utils.go +++ b/s3api/utils/utils.go @@ -185,21 +185,79 @@ func SetMetaHeaders(ctx *fiber.Ctx, meta map[string]string) { ctx.Response().Header.EnableNormalizing() } -func ParseUint(str string) (int32, error) { - if str == "" { - return 1000, nil +// LimiterType represents the name of a pagination limiter parameter. +type LimiterType string + +const ( + // Standard S3 limiters + LimiterTypeMaxKeys = "max-keys" // ListObjects, ListObjectVersions + LimiterTypeMaxParts = "max-parts" // ListParts + LimiterTypeMaxUploads = "max-uploads" // ListMultipartUploads + LimiterTypePartNumberMarker = "part-number-marker" // ListParts(partNumberMarker) + LimiterTypeVersionsMaxKeys = "versions_max_keys" // ListObjectVersions (internal alias mapped to max-keys) + LimiterTypeMaxBuckets = "max-buckets" // ListBuckets +) + +const ( + // Default values applied when the limiter is missing. + defaultMaxBuckets = int32(10000) + defaultMaxLimiter = int32(1000) +) + +// ParseMaxLimiter parses and validates the pagination limiter +// - Empty value → return default depending on limiter type. +// - Non-numeric or out-of-range → return the corresponding InvalidArgument error. +// - Negative values → return specific negative-value errors per limiter type. +// - Values above defaultMaxLimiter are clamped. +// - The versions_max_keys limiter is normalized to max-keys for validation. +func ParseMaxLimiter(limiter string, lt LimiterType) (int32, error) { + if limiter == "" { + // Use per-type default when no limiter is provided. + if lt == LimiterTypeMaxBuckets { + return defaultMaxBuckets, nil + } + return defaultMaxLimiter, nil } - num, err := strconv.ParseInt(str, 10, 32) + + num, err := strconv.ParseInt(limiter, 10, 32) if err != nil { - debuglogger.Logf("invalid intager provided: %v\n", err) - return 1000, fmt.Errorf("invalid int: %w", err) + // versions_max_keys follows max-keys error semantics. + if lt == LimiterTypeVersionsMaxKeys { + lt = LimiterTypeMaxKeys + } + debuglogger.Logf("invalid %s provided: %s\n", lt, limiter) + return 0, s3err.GetInvalidMaxLimiterErr(string(lt)) } + + // max-buckets has distinct range rules and errors. + if lt == LimiterTypeMaxBuckets { + if num < 1 || num > int64(defaultMaxBuckets) { + debuglogger.Logf("invalid max-buckets: %v", num) + return 0, s3err.GetAPIError(s3err.ErrInvalidMaxBuckets) + } + return int32(num), nil + } + + // Negative values violate limiter semantics. if num < 0 { - debuglogger.Logf("negative intager provided: %v\n", num) - return 1000, fmt.Errorf("negative uint: %v", num) + logArg := lt + if logArg == LimiterTypeVersionsMaxKeys { + logArg = LimiterTypeMaxKeys + } + + debuglogger.Logf("negative %s provided: %v\n", logArg, num) + + // versions_max_keys uses the MaxKeys negative error. + if lt == LimiterTypeVersionsMaxKeys { + return 0, s3err.GetAPIError(s3err.ErrNegativeMaxKeys) + } + + return 0, s3err.GetNegativeMaxLimiterErr(string(lt)) } - if num > 1000 { - num = 1000 + + // Clamp excessive limiters to defaultMaxLimiter. + if num > int64(defaultMaxLimiter) { + num = int64(defaultMaxLimiter) } return int32(num), nil } diff --git a/s3api/utils/utils_test.go b/s3api/utils/utils_test.go index a3227d44..077dc9dc 100644 --- a/s3api/utils/utils_test.go +++ b/s3api/utils/utils_test.go @@ -253,67 +253,180 @@ func TestSetBucketNameValidationStrict(t *testing.T) { } } -func TestParseUint(t *testing.T) { +func TestParseMaxLimiter(t *testing.T) { type args struct { str string + lt LimiterType + } + type expected struct { + err error + res int32 } tests := []struct { - name string - args args - want int32 - wantErr bool + name string + args args + expected expected }{ { - name: "Parse-uint-empty-string", + name: "empty_string", args: args{ str: "", + lt: LimiterTypeMaxKeys, + }, + expected: expected{ + err: nil, + res: 1000, }, - want: 1000, - wantErr: false, }, { - name: "Parse-uint-invalid-number-string", + name: "empty_max-buckets", + args: args{ + str: "", + lt: LimiterTypeMaxBuckets, + }, + expected: expected{ + err: nil, + res: 10000, + }, + }, + { + name: "invalid_max-parts", args: args{ str: "bla", + lt: LimiterTypeMaxParts, + }, + expected: expected{ + err: s3err.GetInvalidMaxLimiterErr(string(LimiterTypeMaxParts)), + res: 0, }, - want: 1000, - wantErr: true, }, { - name: "Parse-uint-invalid-negative-number", + name: "invalid_max-uploads", + args: args{ + str: "invalid", + lt: LimiterTypeMaxUploads, + }, + expected: expected{ + err: s3err.GetInvalidMaxLimiterErr(string(LimiterTypeMaxUploads)), + res: 0, + }, + }, + { + name: "invalid_max-buckets", + args: args{ + str: "invalid", + lt: LimiterTypeMaxBuckets, + }, + expected: expected{ + err: s3err.GetInvalidMaxLimiterErr(string(LimiterTypeMaxBuckets)), + res: 0, + }, + }, + { + name: "invalid_versions_max-keys", + args: args{ + str: "invalid", + lt: LimiterTypeMaxKeys, + }, + expected: expected{ + err: s3err.GetInvalidMaxLimiterErr(string(LimiterTypeMaxKeys)), + res: 0, + }, + }, + { + name: "negative_max-keys", args: args{ str: "-5", + lt: LimiterTypeMaxKeys, + }, + expected: expected{ + err: s3err.GetNegativeMaxLimiterErr(string(LimiterTypeMaxKeys)), + res: 0, }, - want: 1000, - wantErr: true, }, { - name: "Parse-uint-success", + name: "negative_part-number-marker", + args: args{ + str: "-5", + lt: LimiterTypePartNumberMarker, + }, + expected: expected{ + err: s3err.GetNegativeMaxLimiterErr(string(LimiterTypePartNumberMarker)), + res: 0, + }, + }, + { + name: "negative_max-buckets", + args: args{ + str: "-12", + lt: LimiterTypeMaxBuckets, + }, + expected: expected{ + err: s3err.GetAPIError(s3err.ErrInvalidMaxBuckets), + res: 0, + }, + }, + { + name: "negative_versions_max-keys", + args: args{ + str: "-12", + lt: LimiterTypeVersionsMaxKeys, + }, + expected: expected{ + err: s3err.GetAPIError(s3err.ErrNegativeMaxKeys), + res: 0, + }, + }, + { + name: "greater_than_10000_max-buckets", + args: args{ + str: "25000", + lt: LimiterTypeMaxBuckets, + }, + expected: expected{ + err: s3err.GetAPIError(s3err.ErrInvalidMaxBuckets), + res: 0, + }, + }, + { + name: "greater_than_1000_max-buckets", + args: args{ + str: "1300", + lt: LimiterTypeMaxBuckets, + }, + expected: expected{ + err: nil, + res: 1300, + }, + }, + { + name: "greater_than_1000", + args: args{ + str: "25000", + lt: LimiterTypeMaxParts, + }, + expected: expected{ + err: nil, + res: 1000, + }, + }, + { + name: "success", args: args{ str: "23", + lt: LimiterTypeMaxUploads, }, - want: 23, - wantErr: false, - }, - { - name: "Parse-uint-greater-than-1000", - args: args{ - str: "25000000", + expected: expected{ + err: nil, + res: 23, }, - want: 1000, - wantErr: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := ParseUint(tt.args.str) - if (err != nil) != tt.wantErr { - t.Errorf("ParseMaxKeys() error = %v, wantErr %v", err, tt.wantErr) - return - } - if got != tt.want { - t.Errorf("ParseMaxKeys() = %v, want %v", got, tt.want) - } + got, err := ParseMaxLimiter(tt.args.str, tt.args.lt) + assert.Equal(t, tt.expected.err, err) + assert.Equal(t, tt.expected.res, got) }) } } diff --git a/s3err/s3err.go b/s3err/s3err.go index 11847b00..73ea4ed6 100644 --- a/s3err/s3err.go +++ b/s3err/s3err.go @@ -76,11 +76,8 @@ const ( ErrInvalidBucketName ErrInvalidDigest ErrBadDigest - ErrInvalidMaxKeys ErrInvalidMaxBuckets - ErrInvalidMaxUploads - ErrInvalidMaxParts - ErrInvalidPartNumberMarker + ErrNegativeMaxKeys ErrInvalidObjectAttributes ErrInvalidPart ErrInvalidPartNumber @@ -287,24 +284,9 @@ var errorCodeResponse = map[ErrorCode]APIError{ Description: "Argument max-buckets must be an integer between 1 and 10000.", HTTPStatusCode: http.StatusBadRequest, }, - ErrInvalidMaxUploads: { + ErrNegativeMaxKeys: { Code: "InvalidArgument", - Description: "Argument max-uploads must be an integer between 0 and 2147483647.", - HTTPStatusCode: http.StatusBadRequest, - }, - ErrInvalidMaxKeys: { - Code: "InvalidArgument", - Description: "Argument maxKeys must be an integer between 0 and 2147483647.", - HTTPStatusCode: http.StatusBadRequest, - }, - ErrInvalidMaxParts: { - Code: "InvalidArgument", - Description: "Argument max-parts must be an integer between 0 and 2147483647.", - HTTPStatusCode: http.StatusBadRequest, - }, - ErrInvalidPartNumberMarker: { - Code: "InvalidArgument", - Description: "Argument part-number-marker must be an integer between 0 and 2147483647", + Description: "max-keys cannot be negative", HTTPStatusCode: http.StatusBadRequest, }, ErrInvalidObjectAttributes: { @@ -1063,3 +1045,19 @@ func GetInvalidCORSMethodErr(method string) APIError { HTTPStatusCode: http.StatusBadRequest, } } + +func GetInvalidMaxLimiterErr(limiter string) APIError { + return APIError{ + Code: "InvalidArgument", + Description: fmt.Sprintf("Provided %s not an integer or within integer range", limiter), + HTTPStatusCode: http.StatusBadRequest, + } +} + +func GetNegativeMaxLimiterErr(limiter string) APIError { + return APIError{ + Code: "InvalidArgument", + Description: fmt.Sprintf("Argument %s must be an integer between 0 and 2147483647", limiter), + HTTPStatusCode: http.StatusBadRequest, + } +} diff --git a/tests/integration/ListMultipartUploads.go b/tests/integration/ListMultipartUploads.go index c86ac9a3..f60d946e 100644 --- a/tests/integration/ListMultipartUploads.go +++ b/tests/integration/ListMultipartUploads.go @@ -74,7 +74,7 @@ func ListMultipartUploads_invalid_max_uploads(s *S3Conf) error { MaxUploads: &maxUploads, }) cancel() - if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrInvalidMaxUploads)); err != nil { + if err := checkApiErr(err, s3err.GetNegativeMaxLimiterErr("max-uploads")); err != nil { return err } diff --git a/tests/integration/ListObjectVersions.go b/tests/integration/ListObjectVersions.go index 521390e9..d13a2eba 100644 --- a/tests/integration/ListObjectVersions.go +++ b/tests/integration/ListObjectVersions.go @@ -81,6 +81,19 @@ func ListObjectVersions_non_existing_bucket(s *S3Conf) error { }, withVersioning(types.BucketVersioningStatusEnabled)) } +func ListObjectVersions_negative_max_keys(s *S3Conf) error { + testName := "ListObjectVersions_negative_max_keys" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := s3client.ListObjectVersions(ctx, &s3.ListObjectVersionsInput{ + Bucket: &bucket, + MaxKeys: getPtr(int32(-123)), + }) + cancel() + return checkApiErr(err, s3err.GetAPIError(s3err.ErrNegativeMaxKeys)) + }, withLock()) +} + func ListObjectVersions_list_single_object_versions(s *S3Conf) error { testName := "ListObjectVersions_list_single_object_versions" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { diff --git a/tests/integration/ListObjects.go b/tests/integration/ListObjects.go index 695cafbc..b0789f15 100644 --- a/tests/integration/ListObjects.go +++ b/tests/integration/ListObjects.go @@ -185,7 +185,7 @@ func ListObjects_invalid_max_keys(s *S3Conf) error { MaxKeys: &maxKeys, }) cancel() - if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrInvalidMaxKeys)); err != nil { + if err := checkApiErr(err, s3err.GetNegativeMaxLimiterErr("max-keys")); err != nil { return err } diff --git a/tests/integration/ListParts.go b/tests/integration/ListParts.go index 7b9430b0..8ae454ee 100644 --- a/tests/integration/ListParts.go +++ b/tests/integration/ListParts.go @@ -83,7 +83,48 @@ func ListParts_invalid_max_parts(s *S3Conf) error { MaxParts: &invMaxParts, }) cancel() - if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrInvalidMaxParts)); err != nil { + if err := checkApiErr(err, s3err.GetNegativeMaxLimiterErr("max-parts")); err != nil { + return err + } + + return nil + }) +} + +func ListParts_invalid_part_number_marker(s *S3Conf) error { + testName := "ListParts_invalid_part_number_marker" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + obj := "my-obj" + out, err := createMp(s3client, bucket, obj) + if err != nil { + return err + } + + listparts := func(partNumberMarker string) error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.ListParts(ctx, &s3.ListPartsInput{ + Bucket: &bucket, + Key: &obj, + UploadId: out.UploadId, + PartNumberMarker: &partNumberMarker, + }) + cancel() + return err + } + + // invalid part number marker + err = listparts("invalid") + if err := checkApiErr(err, s3err.GetInvalidMaxLimiterErr("part-number-marker")); err != nil { + return err + } + // out of in range part number marker + err = listparts("2736457823532448723") + if err := checkApiErr(err, s3err.GetInvalidMaxLimiterErr("part-number-marker")); err != nil { + return err + } + // negative part number marker + err = listparts("-14") + if err := checkApiErr(err, s3err.GetNegativeMaxLimiterErr("part-number-marker")); err != nil { return err } diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index 5bffa338..942671c9 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -442,6 +442,7 @@ func TestListParts(ts *TestState) { ts.Run(ListParts_incorrect_uploadId) ts.Run(ListParts_incorrect_object_key) ts.Run(ListParts_invalid_max_parts) + ts.Run(ListParts_invalid_part_number_marker) ts.Run(ListParts_default_max_parts) ts.Run(ListParts_exceeding_max_parts) ts.Run(ListParts_truncated) @@ -1054,6 +1055,7 @@ func TestVersioning(ts *TestState) { ts.Run(Versioning_DeleteObjects_delete_deleteMarkers) // ListObjectVersions ts.Run(ListObjectVersions_non_existing_bucket) + ts.Run(ListObjectVersions_negative_max_keys) ts.Run(ListObjectVersions_list_single_object_versions) ts.Run(ListObjectVersions_list_multiple_object_versions) ts.Run(ListObjectVersions_multiple_object_versions_truncated) @@ -1467,6 +1469,7 @@ func GetIntTests() IntTests { "ListParts_incorrect_uploadId": ListParts_incorrect_uploadId, "ListParts_incorrect_object_key": ListParts_incorrect_object_key, "ListParts_invalid_max_parts": ListParts_invalid_max_parts, + "ListParts_invalid_part_number_marker": ListParts_invalid_part_number_marker, "ListParts_default_max_parts": ListParts_default_max_parts, "ListParts_truncated": ListParts_truncated, "ListParts_with_checksums": ListParts_with_checksums, @@ -1773,6 +1776,7 @@ func GetIntTests() IntTests { "Versioning_DeleteObjects_success": Versioning_DeleteObjects_success, "Versioning_DeleteObjects_delete_deleteMarkers": Versioning_DeleteObjects_delete_deleteMarkers, "ListObjectVersions_non_existing_bucket": ListObjectVersions_non_existing_bucket, + "ListObjectVersions_negative_max_keys": ListObjectVersions_negative_max_keys, "ListObjectVersions_list_single_object_versions": ListObjectVersions_list_single_object_versions, "ListObjectVersions_list_multiple_object_versions": ListObjectVersions_list_multiple_object_versions, "ListObjectVersions_multiple_object_versions_truncated": ListObjectVersions_multiple_object_versions_truncated,