From b004996d515cd7e2e6328c783af9da8a4d026510 Mon Sep 17 00:00:00 2001 From: Denis Kirillov Date: Tue, 13 Jul 2021 17:45:38 +0300 Subject: [PATCH 1/6] [#155] Fix KeyCount Signed-off-by: Denis Kirillov --- api/handler/list.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/handler/list.go b/api/handler/list.go index f8324b26..af179bab 100644 --- a/api/handler/list.go +++ b/api/handler/list.go @@ -241,7 +241,7 @@ func encodeV2(arg *listObjectsArgs, list *layer.ListObjectsInfo) *ListObjectsV2R Name: arg.Bucket, EncodingType: arg.Encode, Prefix: arg.Prefix, - KeyCount: len(list.Objects), + KeyCount: len(list.Objects) + len(list.Prefixes), MaxKeys: arg.MaxKeys, Delimiter: arg.Delimiter, StartAfter: arg.StartAfter, From 5fb4c4fad6d5770f124842d3fcaca8f568f56e9f Mon Sep 17 00:00:00 2001 From: Denis Kirillov Date: Tue, 13 Jul 2021 17:46:32 +0300 Subject: [PATCH 2/6] [#155] Fixed empty delimiter response Signed-off-by: Denis Kirillov --- api/handler/response.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/handler/response.go b/api/handler/response.go index 42f2e556..b57a7a04 100644 --- a/api/handler/response.go +++ b/api/handler/response.go @@ -33,7 +33,7 @@ type ListObjectsV2Response struct { KeyCount int MaxKeys int - Delimiter string + Delimiter string `xml:"Delimiter,omitempty"` // A flag that indicates whether or not ListObjects returned all of the results // that satisfied the search criteria. IsTruncated bool @@ -75,7 +75,7 @@ type ListObjectsResponse struct { NextMarker string `xml:"NextMarker,omitempty"` MaxKeys int - Delimiter string + Delimiter string `xml:"Delimiter,omitempty"` // A flag that indicates whether or not ListObjects returned all of the results // that satisfied the search criteria. IsTruncated bool From 288f6edce867b75cd4524bc68e6e2ed2e2ed5862 Mon Sep 17 00:00:00 2001 From: Denis Kirillov Date: Tue, 13 Jul 2021 17:57:48 +0300 Subject: [PATCH 3/6] [#155] Fixed default maxKey value Signed-off-by: Denis Kirillov --- api/handler/list.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/handler/list.go b/api/handler/list.go index af179bab..bcf0ecde 100644 --- a/api/handler/list.go +++ b/api/handler/list.go @@ -37,7 +37,7 @@ type ListMultipartUploadsResult struct { Xmlns string `xml:"xmlns,attr"` } -var maxObjectList = 10000 // Limit number of objects in a listObjectsResponse/listObjectsVersionsResponse. +const maxObjectList = 1000 // Limit number of objects in a listObjectsResponse/listObjectsVersionsResponse. func (h *handler) registerAndSendError(w http.ResponseWriter, r *http.Request, err error, logText string) { rid := api.GetRequestID(r.Context()) From 9f577563518113aa6201400efe46b1c2d4fe6559 Mon Sep 17 00:00:00 2001 From: Denis Kirillov Date: Tue, 13 Jul 2021 19:58:55 +0300 Subject: [PATCH 4/6] [#155] Added s3 url encoder Signed-off-by: Denis Kirillov --- api/handler/list.go | 14 ++--- api/handler/s3encoder.go | 111 ++++++++++++++++++++++++++++++++++ api/handler/s3encoder_test.go | 53 ++++++++++++++++ 3 files changed, 171 insertions(+), 7 deletions(-) create mode 100644 api/handler/s3encoder.go create mode 100644 api/handler/s3encoder_test.go diff --git a/api/handler/list.go b/api/handler/list.go index bcf0ecde..81b681a0 100644 --- a/api/handler/list.go +++ b/api/handler/list.go @@ -193,14 +193,14 @@ func encodeV1(arg *listObjectsArgs, list *layer.ListObjectsInfo) *ListObjectsRes // fill common prefixes for i := range list.Prefixes { res.CommonPrefixes = append(res.CommonPrefixes, CommonPrefix{ - Prefix: list.Prefixes[i], + Prefix: s3PathEncode(list.Prefixes[i], arg.Encode), }) } // fill contents for _, obj := range list.Objects { res.Contents = append(res.Contents, Object{ - Key: obj.Name, + Key: s3PathEncode(obj.Name, arg.Encode), Size: obj.Size, LastModified: obj.Created.Format(time.RFC3339), @@ -240,11 +240,11 @@ func encodeV2(arg *listObjectsArgs, list *layer.ListObjectsInfo) *ListObjectsV2R res := &ListObjectsV2Response{ Name: arg.Bucket, EncodingType: arg.Encode, - Prefix: arg.Prefix, + Prefix: s3PathEncode(arg.Prefix, arg.Encode), KeyCount: len(list.Objects) + len(list.Prefixes), MaxKeys: arg.MaxKeys, - Delimiter: arg.Delimiter, - StartAfter: arg.StartAfter, + Delimiter: s3PathEncode(arg.Delimiter, arg.Encode), + StartAfter: s3PathEncode(arg.StartAfter, arg.Encode), IsTruncated: list.IsTruncated, @@ -255,14 +255,14 @@ func encodeV2(arg *listObjectsArgs, list *layer.ListObjectsInfo) *ListObjectsV2R // fill common prefixes for i := range list.Prefixes { res.CommonPrefixes = append(res.CommonPrefixes, CommonPrefix{ - Prefix: list.Prefixes[i], + Prefix: s3PathEncode(list.Prefixes[i], arg.Encode), }) } // fill contents for _, obj := range list.Objects { res.Contents = append(res.Contents, Object{ - Key: obj.Name, + Key: s3PathEncode(obj.Name, arg.Encode), Size: obj.Size, LastModified: obj.Created.Format(time.RFC3339), diff --git a/api/handler/s3encoder.go b/api/handler/s3encoder.go new file mode 100644 index 00000000..3e205a81 --- /dev/null +++ b/api/handler/s3encoder.go @@ -0,0 +1,111 @@ +package handler + +import ( + "strings" +) + +type encoding int + +const ( + encodePathSegment encoding = iota + encodeQueryComponent +) + +const ( + urlEncodingType = "url" + upperhex = "0123456789ABCDEF" +) + +func shouldEscape(c byte) bool { + if 'A' <= c && c <= 'Z' || 'a' <= c && c <= 'z' || '0' <= c && c <= '9' { + return false + } + + switch c { + case '-', '_', '.', '/', '*': + return false + } + return true +} + +// s3URLEncode is based on url.QueryEscape() code, +// while considering some S3 exceptions. +func s3URLEncode(s string, mode encoding) string { + spaceCount, hexCount := 0, 0 + for i := 0; i < len(s); i++ { + c := s[i] + if shouldEscape(c) { + if c == ' ' && mode == encodeQueryComponent { + spaceCount++ + } else { + hexCount++ + } + } + } + + if spaceCount == 0 && hexCount == 0 { + return s + } + + var buf [64]byte + var t []byte + + required := len(s) + 2*hexCount + if required <= len(buf) { + t = buf[:required] + } else { + t = make([]byte, required) + } + + if hexCount == 0 { + copy(t, s) + for i := 0; i < len(s); i++ { + if s[i] == ' ' { + t[i] = '+' + } + } + return string(t) + } + + j := 0 + for i := 0; i < len(s); i++ { + switch c := s[i]; { + case c == ' ' && mode == encodeQueryComponent: + t[j] = '+' + j++ + case shouldEscape(c): + t[j] = '%' + t[j+1] = upperhex[c>>4] + t[j+2] = upperhex[c&15] + j += 3 + default: + t[j] = s[i] + j++ + } + } + return string(t) +} + +func s3QueryEncode(name string, encodingType string) (result string) { + if encodingType == "" { + return name + } + encodingType = strings.ToLower(encodingType) + switch encodingType { + case urlEncodingType: + return s3URLEncode(name, encodeQueryComponent) + } + return name +} + +func s3PathEncode(name string, encodingType string) (result string) { + if encodingType == "" { + return name + } + encodingType = strings.ToLower(encodingType) + switch encodingType { + case urlEncodingType: + return s3URLEncode(name, encodePathSegment) + } + return name +} diff --git a/api/handler/s3encoder_test.go b/api/handler/s3encoder_test.go new file mode 100644 index 00000000..da58cd88 --- /dev/null +++ b/api/handler/s3encoder_test.go @@ -0,0 +1,53 @@ +package handler + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestPathEncoder(t *testing.T) { + for _, tc := range []struct { + key string + expected string + }{ + {key: "simple", expected: "simple"}, + {key: "foo/bar", expected: "foo/bar"}, + {key: "foo+1/bar", expected: "foo%2B1/bar"}, + {key: "foo ab/bar", expected: "foo%20ab/bar"}, + {key: "p-%", expected: "p-%25"}, + {key: "p/", expected: "p/"}, + {key: "p/", expected: "p/"}, + {key: "~user", expected: "%7Euser"}, + {key: "*user", expected: "*user"}, + {key: "user+password", expected: "user%2Bpassword"}, + {key: "_user", expected: "_user"}, + {key: "firstname.lastname", expected: "firstname.lastname"}, + } { + actual := s3PathEncode(tc.key, urlEncodingType) + require.Equal(t, tc.expected, actual) + } +} + +func TestQueryEncoder(t *testing.T) { + for _, tc := range []struct { + key string + expected string + }{ + {key: "simple", expected: "simple"}, + {key: "foo/bar", expected: "foo/bar"}, + {key: "foo+1/bar", expected: "foo%2B1/bar"}, + {key: "foo ab/bar", expected: "foo+ab/bar"}, + {key: "p-%", expected: "p-%25"}, + {key: "p/", expected: "p/"}, + {key: "p/", expected: "p/"}, + {key: "~user", expected: "%7Euser"}, + {key: "*user", expected: "*user"}, + {key: "user+password", expected: "user%2Bpassword"}, + {key: "_user", expected: "_user"}, + {key: "firstname.lastname", expected: "firstname.lastname"}, + } { + actual := s3QueryEncode(tc.key, urlEncodingType) + require.Equal(t, tc.expected, actual) + } +} From 1ecf32c3020ca97a0dae8c72442378c4cb34fb5e Mon Sep 17 00:00:00 2001 From: Denis Kirillov Date: Wed, 14 Jul 2021 11:58:28 +0300 Subject: [PATCH 5/6] [#155] Fixed invalid max-keys handling Signed-off-by: Denis Kirillov --- api/handler/list.go | 11 +++++++++-- api/layer/layer.go | 4 ++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/api/handler/list.go b/api/handler/list.go index 81b681a0..42435979 100644 --- a/api/handler/list.go +++ b/api/handler/list.go @@ -2,6 +2,7 @@ package handler import ( "encoding/xml" + "errors" "net/http" "strconv" "time" @@ -119,6 +120,12 @@ func (h *handler) listObjects(w http.ResponseWriter, r *http.Request) (*listObje zap.String("request_id", rid), zap.Error(err)) + var s3Err api.Error + if ok := errors.As(err, &s3Err); ok { + api.WriteErrorResponse(r.Context(), w, s3Err, r.URL) + return nil, nil, err + } + api.WriteErrorResponse(r.Context(), w, api.Error{ Code: api.GetAPIError(api.ErrBadRequest).Code, Description: err.Error(), @@ -287,7 +294,7 @@ func parseListObjectArgs(r *http.Request) (*listObjectsArgs, error) { if r.URL.Query().Get("max-keys") == "" { res.MaxKeys = maxObjectList - } else if res.MaxKeys, err = strconv.Atoi(r.URL.Query().Get("max-keys")); err != nil || res.MaxKeys <= 0 { + } else if res.MaxKeys, err = strconv.Atoi(r.URL.Query().Get("max-keys")); err != nil || res.MaxKeys < 0 { return nil, api.GetAPIError(api.ErrInvalidMaxKeys) } @@ -384,7 +391,7 @@ func parseListObjectVersionsRequest(r *http.Request) (*layer.ListObjectVersionsP if r.URL.Query().Get("max-keys") == "" { res.MaxKeys = maxObjectList - } else if res.MaxKeys, err = strconv.Atoi(r.URL.Query().Get("max-keys")); err != nil || res.MaxKeys <= 0 { + } else if res.MaxKeys, err = strconv.Atoi(r.URL.Query().Get("max-keys")); err != nil || res.MaxKeys < 0 { return nil, api.GetAPIError(api.ErrInvalidMaxKeys) } diff --git a/api/layer/layer.go b/api/layer/layer.go index 328dbea4..45f047e9 100644 --- a/api/layer/layer.go +++ b/api/layer/layer.go @@ -227,6 +227,10 @@ func (n *layer) ListObjects(ctx context.Context, p *ListObjectsParams) (*ListObj uniqNames = make(map[string]bool) ) + if p.MaxKeys == 0 { + return &result, nil + } + if bkt, err = n.GetBucketInfo(ctx, p.Bucket); err != nil { return nil, err } else if ids, err = n.objectSearch(ctx, &findParams{cid: bkt.CID}); err != nil { From f2eeed0b858541af9304c73f5513e6dc4a67f236 Mon Sep 17 00:00:00 2001 From: Denis Kirillov Date: Wed, 14 Jul 2021 19:06:24 +0300 Subject: [PATCH 6/6] [#155] Fix error handling Signed-off-by: Denis Kirillov --- api/errors.go | 2 +- api/handler/list.go | 14 +------------- api/response.go | 2 +- 3 files changed, 3 insertions(+), 15 deletions(-) diff --git a/api/errors.go b/api/errors.go index a619193f..4a7648a1 100644 --- a/api/errors.go +++ b/api/errors.go @@ -1633,7 +1633,7 @@ func GetAPIError(code ErrorCode) Error { // getErrorResponse gets in standard error and resource value and // provides a encodable populated response values. func getAPIErrorResponse(ctx context.Context, err error, resource, requestID, hostID string) ErrorResponse { - code := "BadRequest" + code := "InternalError" desc := err.Error() info := GetReqInfo(ctx) diff --git a/api/handler/list.go b/api/handler/list.go index 42435979..9b68bcd4 100644 --- a/api/handler/list.go +++ b/api/handler/list.go @@ -2,7 +2,6 @@ package handler import ( "encoding/xml" - "errors" "net/http" "strconv" "time" @@ -120,18 +119,7 @@ func (h *handler) listObjects(w http.ResponseWriter, r *http.Request) (*listObje zap.String("request_id", rid), zap.Error(err)) - var s3Err api.Error - if ok := errors.As(err, &s3Err); ok { - api.WriteErrorResponse(r.Context(), w, s3Err, r.URL) - return nil, nil, err - } - - api.WriteErrorResponse(r.Context(), w, api.Error{ - Code: api.GetAPIError(api.ErrBadRequest).Code, - Description: err.Error(), - HTTPStatusCode: http.StatusBadRequest, - }, r.URL) - + api.WriteErrorResponse(r.Context(), w, err, r.URL) return nil, nil, err } diff --git a/api/response.go b/api/response.go index 88b961d6..f739198c 100644 --- a/api/response.go +++ b/api/response.go @@ -119,7 +119,7 @@ var s3ErrorResponseMap = map[string]string{ // WriteErrorResponse writes error headers. func WriteErrorResponse(ctx context.Context, w http.ResponseWriter, err error, reqURL *url.URL) { - code := http.StatusBadRequest + code := http.StatusInternalServerError if e, ok := err.(Error); ok { code = e.HTTPStatusCode