From 0bed25816ce832e2dfb26e2b68712fdbf587b981 Mon Sep 17 00:00:00 2001 From: Denis Kirillov Date: Fri, 27 Oct 2023 17:56:51 +0300 Subject: [PATCH] [#224] Add conditional escaping for object name Chi gives inconsistent results in terms of whether the strings returned are URL coded or not See: * https://github.com/go-chi/chi/issues/641 * https://github.com/go-chi/chi/issues/642 Signed-off-by: Denis Kirillov --- CHANGELOG.md | 2 +- api/handler/object_list_test.go | 30 ++++++++++++++++++++++++++++++ api/middleware/reqinfo.go | 13 ++++++++++++- api/router_test.go | 11 ++++++++--- internal/logs/logs.go | 1 + 5 files changed, 52 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 59507d93..96dfa14c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,7 @@ This document outlines major changes between releases. - Replace part on re-upload when use multipart upload (#176) - Fix goroutine leak on put object error (#178) - Fix parsing signed headers in presigned urls (#182) -- Fix url escaping (#188) +- Fix url escaping (#188, #224) - Use correct keys in `list-multipart-uploads` response (#185) - Fix parsing `key-marker` for object list versions (#243) - Fix marshaling errors in `DeleteObjects` method (#222) diff --git a/api/handler/object_list_test.go b/api/handler/object_list_test.go index 5b6b859f..983a281a 100644 --- a/api/handler/object_list_test.go +++ b/api/handler/object_list_test.go @@ -112,6 +112,36 @@ func TestS3CompatibilityBucketListV2BothContinuationTokenStartAfter(t *testing.T require.Equal(t, "quxx", listV2Response2.Contents[1].Key) } +func TestS3BucketListV2EncodingBasic(t *testing.T) { + hc := prepareHandlerContext(t) + + bktName := "bucket-for-listing-v1-encoding" + bktInfo := createTestBucket(hc, bktName) + + objects := []string{"foo+1/bar", "foo/bar/xyzzy", "quux ab/thud", "asdf+b"} + for _, objName := range objects { + createTestObject(hc, bktInfo, objName) + } + + query := make(url.Values) + query.Add("delimiter", "/") + query.Add("encoding-type", "url") + + w, r := prepareTestFullRequest(hc, bktName, "", query, nil) + hc.Handler().ListObjectsV2Handler(w, r) + assertStatus(hc.t, w, http.StatusOK) + listV2Response := &ListObjectsV2Response{} + parseTestResponse(hc.t, w, listV2Response) + + require.Equal(t, "/", listV2Response.Delimiter) + require.Len(t, listV2Response.Contents, 1) + require.Equal(t, "asdf%2Bb", listV2Response.Contents[0].Key) + require.Len(t, listV2Response.CommonPrefixes, 3) + require.Equal(t, "foo%2B1/", listV2Response.CommonPrefixes[0].Prefix) + require.Equal(t, "foo/", listV2Response.CommonPrefixes[1].Prefix) + require.Equal(t, "quux%20ab/", listV2Response.CommonPrefixes[2].Prefix) +} + func TestS3BucketListDelimiterBasic(t *testing.T) { tc := prepareHandlerContext(t) diff --git a/api/middleware/reqinfo.go b/api/middleware/reqinfo.go index 0388dca4..c857732b 100644 --- a/api/middleware/reqinfo.go +++ b/api/middleware/reqinfo.go @@ -241,12 +241,23 @@ func AddObjectName(l *zap.Logger) Func { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() reqInfo := GetReqInfo(ctx) + reqLogger := reqLogOrDefault(ctx, l) rctx := chi.RouteContext(ctx) // trim leading slash (always present) reqInfo.ObjectName = rctx.RoutePath[1:] - reqLogger := reqLogOrDefault(ctx, l) + if r.URL.RawPath != "" { + // we have to do this because of + // https://github.com/go-chi/chi/issues/641 + // https://github.com/go-chi/chi/issues/642 + if obj, err := url.PathUnescape(reqInfo.ObjectName); err != nil { + reqLogger.Warn(logs.FailedToUnescapeObjectName, zap.Error(err)) + } else { + reqInfo.ObjectName = obj + } + } + r = r.WithContext(SetReqLogger(ctx, reqLogger.With(zap.String("object", reqInfo.ObjectName)))) h.ServeHTTP(w, r) diff --git a/api/router_test.go b/api/router_test.go index 153eec51..506a27e0 100644 --- a/api/router_test.go +++ b/api/router_test.go @@ -82,15 +82,20 @@ func TestRouterObjectEscaping(t *testing.T) { objName: "fix/object", }, { - name: "with percentage", - expectedObjName: "fix/object%ac", - objName: "fix/object%ac", + name: "with slash escaped", + expectedObjName: "/foo/bar", + objName: "/foo%2fbar", }, { name: "with percentage escaped", expectedObjName: "fix/object%ac", objName: "fix/object%25ac", }, + { + name: "with awful mint name", + expectedObjName: "äöüex ®©µÄÆÐÕæŒƕƩDž 01000000 0x40 \u0040 amȡȹɆple&0a!-_.*'()&$@=;:+,?<>.pdf", + objName: "%C3%A4%C3%B6%C3%BCex%20%C2%AE%C2%A9%C2%B5%C3%84%C3%86%C3%90%C3%95%C3%A6%C5%92%C6%95%C6%A9%C7%85%2001000000%200x40%20%40%20am%C8%A1%C8%B9%C9%86ple%260a%21-_.%2A%27%28%29%26%24%40%3D%3B%3A%2B%2C%3F%3C%3E.pdf", + }, } { t.Run(tc.name, func(t *testing.T) { target := fmt.Sprintf("/%s/%s", bktName, tc.objName) diff --git a/internal/logs/logs.go b/internal/logs/logs.go index 4ad64aa4..9e844cf2 100644 --- a/internal/logs/logs.go +++ b/internal/logs/logs.go @@ -95,6 +95,7 @@ const ( FailedToPassAuthentication = "failed to pass authentication" // Error in ../../api/middleware/auth.go FailedToResolveCID = "failed to resolve CID" // Debug in ../../api/middleware/metrics.go RequestStart = "request start" // Info in ../../api/middleware/reqinfo.go + FailedToUnescapeObjectName = "failed to unescape object name" // Warn in ../../api/middleware/reqinfo.go CouldNotHandleMessage = "could not handle message" // Error in ../../api/notifications/controller.go CouldNotACKMessage = "could not ACK message" // Error in ../../api/notifications/controller.go CouldntMarshalAnEvent = "couldn't marshal an event" // Error in ../../api/notifications/controller.go