From b7ff97181dc0a7fa9baa29b8ad46b7c1b29bba4d Mon Sep 17 00:00:00 2001 From: DualFroz Date: Fri, 2 Oct 2026 16:07:01 +0200 Subject: [PATCH] fix: validate and apply the ListBuckets bucket-region query ListBuckets ignored bucket-region: a value that is not a region was accepted, and a region other than the gateway one still listed every bucket. S3 rejects an invalid value with 400 InvalidArgument, "Argument value abc is not a valid AWS Region", and lists only the buckets of the requested region. Check bucket-region against the AWS region code format, always accepting the gateway region whatever its name, and return an empty list for any other region, since every bucket lives in the gateway region. Enable the REST tests that cover both cases. Fixes #1923 Fixes #1930 --- s3api/controllers/bucket-list.go | 17 ++++++++ s3api/controllers/bucket-list_test.go | 57 +++++++++++++++++++++++++++ s3api/utils/utils.go | 9 +++++ s3api/utils/utils_test.go | 25 ++++++++++++ s3err/invalid-argument.go | 8 ++++ tests/integration/ListBuckets.go | 55 ++++++++++++++++++++++++++ tests/integration/group-tests.go | 4 ++ tests/test_rest_list_buckets.sh | 6 --- 8 files changed, 175 insertions(+), 6 deletions(-) diff --git a/s3api/controllers/bucket-list.go b/s3api/controllers/bucket-list.go index c85f77c..93885e0 100644 --- a/s3api/controllers/bucket-list.go +++ b/s3api/controllers/bucket-list.go @@ -17,7 +17,9 @@ package controllers import ( "github.com/gofiber/fiber/v3" "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" ) @@ -25,6 +27,7 @@ func (c S3ApiController) ListBuckets(ctx fiber.Ctx) (*Response, error) { cToken := ctx.Query("continuation-token") prefix := ctx.Query("prefix") maxBucketsStr := ctx.Query("max-buckets") + bucketRegion := ctx.Query("bucket-region") acct := utils.ContextKeyAccount.Get(ctx).(auth.Account) isRoot, _ := utils.ContextKeyIsRoot.Get(ctx).(bool) @@ -55,6 +58,13 @@ func (c S3ApiController) ListBuckets(ctx fiber.Ctx) (*Response, error) { }, err } + if bucketRegion != "" && !utils.IsValidBucketRegion(bucketRegion, region) { + debuglogger.Logf("invalid bucket region: %s", bucketRegion) + return &Response{ + MetaOpts: &MetaOptions{}, + }, s3err.GetInvalidArgBucketRegion(bucketRegion) + } + // IsAdmin is the backends' "return every bucket, unfiltered" flag. res, err := c.be.ListBuckets(ctx.RequestCtx(), s3response.ListBucketsInput{ @@ -68,6 +78,13 @@ func (c S3ApiController) ListBuckets(ctx fiber.Ctx) (*Response, error) { return &Response{}, err } + // every bucket lives in the gateway region, so there is nothing + // to list for any other region + if bucketRegion != "" && bucketRegion != region { + res.Buckets.Bucket = nil + res.ContinuationToken = "" + } + for i := range res.Buckets.Bucket { res.Buckets.Bucket[i].BucketRegion = region } diff --git a/s3api/controllers/bucket-list_test.go b/s3api/controllers/bucket-list_test.go index bfa16f6..bdf9b5b 100644 --- a/s3api/controllers/bucket-list_test.go +++ b/s3api/controllers/bucket-list_test.go @@ -99,6 +99,63 @@ func TestS3ApiController_ListBuckets(t *testing.T) { err: s3err.GetInvalidArgMaxLimiter("max-buckets", "bla"), }, }, + { + name: "invalid bucket region", + input: testInput{ + locals: defaultLocals, + beRes: validRes, + queries: map[string]string{ + "bucket-region": "abc", + }, + }, + output: testOutput{ + response: &Response{ + MetaOpts: &MetaOptions{}, + }, + err: s3err.GetInvalidArgBucketRegion("abc"), + }, + }, + { + name: "bucket region of another region", + input: testInput{ + locals: defaultLocals, + beRes: s3response.ListAllMyBucketsResult{ + Owner: validRes.Owner, + Buckets: validRes.Buckets, + Prefix: "te", + // a continuation token from the backend must not + // leak into the empty listing + ContinuationToken: "test", + }, + queries: map[string]string{ + "bucket-region": "us-west-2", + "prefix": "te", + }, + }, + output: testOutput{ + response: &Response{ + Data: s3response.ListAllMyBucketsResult{ + Owner: validRes.Owner, + Prefix: "te", + }, + }, + }, + }, + { + name: "bucket region of the gateway region", + input: testInput{ + locals: defaultLocals, + beRes: validRes, + queries: map[string]string{ + "bucket-region": "us-east-1", + }, + }, + output: testOutput{ + response: &Response{ + Data: validRes, + }, + }, + }, { name: "backend returns error", input: testInput{ diff --git a/s3api/utils/utils.go b/s3api/utils/utils.go index 16e7bfa..27912bf 100644 --- a/s3api/utils/utils.go +++ b/s3api/utils/utils.go @@ -40,6 +40,8 @@ import ( var ( bucketNameRegexp = regexp.MustCompile(`^[a-z0-9][a-z0-9.-]+[a-z0-9]$`) bucketNameIpRegexp = regexp.MustCompile(`^(?:[0-9]{1,3}\.){3}[0-9]{1,3}$`) + // AWS region codes, such as us-east-1, us-gov-west-1 or eusc-de-east-1 + awsRegionRegexp = regexp.MustCompile(`^[a-z]{2,4}(-[a-z]+)+-[0-9]+$`) ) var strictBucketNameValidation atomic.Bool @@ -1153,6 +1155,13 @@ func ValidateLocationConstraint(constraint *string, region string) error { return nil } +// IsValidBucketRegion checks a ListBuckets bucket-region value. The gateway +// region is valid whatever its name, any other value has to be an AWS region +// code. +func IsValidBucketRegion(bucketRegion, region string) bool { + return bucketRegion == region || awsRegionRegexp.MatchString(bucketRegion) +} + // AwsChunkedEncoding is the coding a client announces when it frames a body in // aws-chunked, as the SDKs do to carry a trailing checksum. const AwsChunkedEncoding = "aws-chunked" diff --git a/s3api/utils/utils_test.go b/s3api/utils/utils_test.go index 217d4bc..9d51f44 100644 --- a/s3api/utils/utils_test.go +++ b/s3api/utils/utils_test.go @@ -1539,6 +1539,31 @@ func TestHasAwsChunkedEncoding(t *testing.T) { } } +func TestIsValidBucketRegion(t *testing.T) { + tests := []struct { + name string + bucketRegion string + region string + want bool + }{ + {"gateway region", "us-east-1", "us-east-1", true}, + {"custom gateway region", "my-region", "my-region", true}, + {"another aws region", "eu-central-1", "us-east-1", true}, + {"gov cloud region", "us-gov-west-1", "us-east-1", true}, + {"iso partition region", "us-isob-east-1", "us-east-1", true}, + {"sovereign cloud region", "eusc-de-east-1", "us-east-1", true}, + {"gibberish", "abc", "us-east-1", false}, + {"missing number", "us-east", "us-east-1", false}, + {"uppercase", "US-EAST-1", "us-east-1", false}, + {"custom region of another gateway", "my-region", "us-east-1", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, IsValidBucketRegion(tt.bucketRegion, tt.region)) + }) + } +} + func TestParseContentEncoding(t *testing.T) { tests := []struct { name string diff --git a/s3err/invalid-argument.go b/s3err/invalid-argument.go index 7bf61cd..e410597 100644 --- a/s3err/invalid-argument.go +++ b/s3err/invalid-argument.go @@ -384,3 +384,11 @@ func GetInvalidArgObjectOwnership(value string) InvalidArgumentError { Description: fmt.Sprintf("Invalid x-amz-object-ownership header: %s", value), } } + +func GetInvalidArgBucketRegion(value string) InvalidArgumentError { + return InvalidArgumentError{ + ArgumentName: "bucket-region", + // no ArgumentValue is returned for this error + Description: fmt.Sprintf("Argument value %s is not a valid AWS Region", value), + } +} diff --git a/tests/integration/ListBuckets.go b/tests/integration/ListBuckets.go index 178fa61..a9a6967 100644 --- a/tests/integration/ListBuckets.go +++ b/tests/integration/ListBuckets.go @@ -17,6 +17,7 @@ package integration import ( "context" "fmt" + "slices" "sort" "github.com/aws/aws-sdk-go-v2/service/s3" @@ -271,6 +272,60 @@ func ListBuckets_invalid_max_buckets(s *S3Conf) error { }) } +func ListBuckets_invalid_bucket_region(s *S3Conf) error { + testName := "ListBuckets_invalid_bucket_region" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := s3client.ListBuckets(ctx, &s3.ListBucketsInput{ + BucketRegion: getPtr("abc"), + }) + cancel() + return checkApiErr(err, s3err.GetInvalidArgBucketRegion("abc")) + }) +} + +func ListBuckets_bucket_region(s *S3Conf) error { + testName := "ListBuckets_bucket_region" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + listBuckets := func(region string) ([]types.Bucket, error) { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + out, err := s3client.ListBuckets(ctx, &s3.ListBucketsInput{ + BucketRegion: ®ion, + }) + cancel() + if err != nil { + return nil, err + } + return out.Buckets, nil + } + + buckets, err := listBuckets(s.awsRegion) + if err != nil { + return err + } + if !slices.ContainsFunc(buckets, func(b types.Bucket) bool { return getString(b.Name) == bucket }) { + return fmt.Errorf("expected bucket %v in the %v listing, instead got %v", + bucket, s.awsRegion, buckets) + } + + // every bucket of the gateway lives in its own region + otherRegion := "us-west-2" + if s.awsRegion == otherRegion { + otherRegion = "us-east-1" + } + buckets, err = listBuckets(otherRegion) + if err != nil { + return err + } + if len(buckets) != 0 { + return fmt.Errorf("expected an empty %v listing, instead got %v", + otherRegion, buckets) + } + + return nil + }) +} + func ListBuckets_truncated(s *S3Conf) error { testName := "ListBuckets_truncated" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index fb99680..47ac55f 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -110,6 +110,8 @@ func TestListBuckets(ts *TestState) { ts.Sync(ListBuckets_as_admin) ts.Sync(ListBuckets_with_prefix) ts.Sync(ListBuckets_invalid_max_buckets) + ts.Sync(ListBuckets_invalid_bucket_region) + ts.Sync(ListBuckets_bucket_region) ts.Sync(ListBuckets_truncated) ts.Sync(ListBuckets_success) ts.Sync(ListBuckets_empty_success) @@ -2985,6 +2987,8 @@ func GetIntTests() IntTests { "ListBuckets_as_admin": ListBuckets_as_admin, "ListBuckets_with_prefix": ListBuckets_with_prefix, "ListBuckets_invalid_max_buckets": ListBuckets_invalid_max_buckets, + "ListBuckets_invalid_bucket_region": ListBuckets_invalid_bucket_region, + "ListBuckets_bucket_region": ListBuckets_bucket_region, "ListBuckets_truncated": ListBuckets_truncated, "ListBuckets_success": ListBuckets_success, "ListBuckets_empty_success": ListBuckets_empty_success, diff --git a/tests/test_rest_list_buckets.sh b/tests/test_rest_list_buckets.sh index 7ca63ad..f925d3d 100644 --- a/tests/test_rest_list_buckets.sh +++ b/tests/test_rest_list_buckets.sh @@ -258,9 +258,6 @@ export RUN_USERS=true # tags: curl,ListBuckets,bucket-region,invalid-query @test "REST - ListBuckets - invalid bucket-region query" { - if [ "$DIRECT" != "true" ]; then - skip "https://github.com/versity/versitygw/issues/1923" - fi local invalid_region="abc" run get_bucket_name "$BUCKET_ONE_NAME" @@ -277,9 +274,6 @@ export RUN_USERS=true # tags: curl,ListBuckets,bucket-region @test "REST - ListBuckets - incorrect bucket region" { - if [ "$DIRECT" != "true" ]; then - skip "https://github.com/versity/versitygw/issues/1930" - fi local test_region="us-east-1" if [ "$AWS_REGION" == "us-east-1" ]; then test_region="us-west-1"