Merge pull request #187 from versity/fix/issue-183

Issue 183
versity · Aug 3, 2023 · 13ce76b · 13ce76b
2 parents 3483095 + dde13dd
commit 13ce76b
Show file tree

Hide file tree

Showing 4 changed files with 90 additions and 3 deletions.
diff --git a/integration/tests.go b/integration/tests.go
@@ -339,6 +339,14 @@ func TestPutDirObject(s *S3Conf) {
 		return
 	}
 
+	ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+	_, err = s3client.ListObjectsV2(ctx, &s3.ListObjectsV2Input{Bucket: &bucket, MaxKeys: -4})
+	cancel()
+	if err == nil {
+		failF("%v: expected invalid argument error", testname)
+		return
+	}
+
 	ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
 	out, err := s3client.ListObjectsV2(ctx, &s3.ListObjectsV2Input{Bucket: &bucket})
 	cancel()

diff --git a/s3api/controllers/base.go b/s3api/controllers/base.go
@@ -221,7 +221,7 @@ func (c S3ApiController) ListActions(ctx *fiber.Ctx) error {
 	cToken := ctx.Query("continuation-token")
 	marker := ctx.Query("marker")
 	delimiter := ctx.Query("delimiter")
-	maxkeys := ctx.QueryInt("max-keys")
+	maxkeysStr := ctx.Query("max-keys")
 	access := ctx.Locals("access").(string)
 	isRoot := ctx.Locals("isRoot").(bool)
 	parsedAcl := ctx.Locals("parsedAcl").(auth.ACL)
@@ -252,12 +252,20 @@ func (c S3ApiController) ListActions(ctx *fiber.Ctx) error {
 		if err := auth.VerifyACL(parsedAcl, bucket, access, "READ", isRoot); err != nil {
 			return SendXMLResponse(ctx, nil, err, &MetaOpts{Logger: c.logger, Action: "ListObjectsV2", BucketOwner: parsedAcl.Owner})
 		}
+		maxkeys, err := utils.ParseMaxKeys(maxkeysStr)
+		if err != nil {
+			return SendXMLResponse(ctx, nil, err, &MetaOpts{
+				Logger:      c.logger,
+				Action:      "ListObjectsV2",
+				BucketOwner: parsedAcl.Owner,
+			})
+		}
 		res, err := c.be.ListObjectsV2(ctx.Context(), &s3.ListObjectsV2Input{
 			Bucket:            &bucket,
 			Prefix:            &prefix,
 			ContinuationToken: &cToken,
 			Delimiter:         &delimiter,
-			MaxKeys:           int32(maxkeys),
+			MaxKeys:           maxkeys,
 		})
 		return SendXMLResponse(ctx, res, err, &MetaOpts{Logger: c.logger, Action: "ListObjectsV2", BucketOwner: parsedAcl.Owner})
 	}
@@ -266,12 +274,21 @@ func (c S3ApiController) ListActions(ctx *fiber.Ctx) error {
 		return SendXMLResponse(ctx, nil, err, &MetaOpts{Logger: c.logger, Action: "ListObjects", BucketOwner: parsedAcl.Owner})
 	}
 
+	maxkeys, err := utils.ParseMaxKeys(maxkeysStr)
+	if err != nil {
+		return SendXMLResponse(ctx, nil, err, &MetaOpts{
+			Logger:      c.logger,
+			Action:      "ListObjects",
+			BucketOwner: parsedAcl.Owner,
+		})
+	}
+
 	res, err := c.be.ListObjects(ctx.Context(), &s3.ListObjectsInput{
 		Bucket:    &bucket,
 		Prefix:    &prefix,
 		Marker:    &marker,
 		Delimiter: &delimiter,
-		MaxKeys:   int32(maxkeys),
+		MaxKeys:   maxkeys,
 	})
 	return SendXMLResponse(ctx, res, err, &MetaOpts{Logger: c.logger, Action: "ListObjects", BucketOwner: parsedAcl.Owner})
 }

diff --git a/s3api/utils/utils.go b/s3api/utils/utils.go
@@ -20,10 +20,12 @@ import (
 	"fmt"
 	"net/http"
 	"regexp"
+	"strconv"
 	"strings"
 
 	"github.com/gofiber/fiber/v2"
 	"github.com/valyala/fasthttp"
+	"github.com/versity/versitygw/s3err"
 )
 
 var (
@@ -78,6 +80,17 @@ func SetMetaHeaders(ctx *fiber.Ctx, meta map[string]string) {
 	}
 }
 
+func ParseMaxKeys(str string) (int32, error) {
+	if str == "" {
+		return -1, nil
+	}
+	num, err := strconv.ParseUint(str, 10, 16)
+	if err != nil {
+		return -1, s3err.GetAPIError(s3err.ErrInvalidMaxKeys)
+	}
+	return int32(num), nil
+}
+
 type CustomHeader struct {
 	Key   string
 	Value string

diff --git a/s3api/utils/utils_test.go b/s3api/utils/utils_test.go
@@ -221,3 +221,52 @@ func TestIsValidBucketName(t *testing.T) {
 		})
 	}
 }
+
+func TestParseMaxKeys(t *testing.T) {
+	type args struct {
+		str string
+	}
+	tests := []struct {
+		name    string
+		args    args
+		want    int32
+		wantErr bool
+	}{
+		{
+			name: "Parse-max-keys-empty-string",
+			args: args{
+				str: "",
+			},
+			want:    -1,
+			wantErr: false,
+		},
+		{
+			name: "Parse-max-keys-invalid-number-string",
+			args: args{
+				str: "bla",
+			},
+			want:    -1,
+			wantErr: true,
+		},
+		{
+			name: "Parse-max-keys-invalid-negative-number",
+			args: args{
+				str: "-5",
+			},
+			want:    -1,
+			wantErr: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := ParseMaxKeys(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)
+			}
+		})
+	}
+}