diff --git a/weed/s3api/filer_multipart_list_encoding_test.go b/weed/s3api/filer_multipart_list_encoding_test.go index 2449c10585..5eb8ed4e82 100644 --- a/weed/s3api/filer_multipart_list_encoding_test.go +++ b/weed/s3api/filer_multipart_list_encoding_test.go @@ -2,11 +2,15 @@ package s3api import ( "encoding/xml" + "net/http" + "net/http/httptest" "net/url" "testing" + "time" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/s3" + "github.com/gorilla/mux" "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -74,3 +78,55 @@ func TestSelectMultipartUploadsKeepsKeysRawWithoutEncodingType(t *testing.T) { } assert.Equal(t, []string{"m\x01p b+.bin", "plain/key.txt"}, got) } + +type listUploadsTestPage struct { + XMLName xml.Name `xml:"ListMultipartUploadsResult"` + KeyMarker string `xml:"KeyMarker"` + Uploads []struct { + Key string `xml:"Key"` + } `xml:"Upload"` +} + +// The request key-marker is a raw key even under encoding-type=url: SDKs +// decode the escaped response fields and send the key itself back, so a +// marker like "a%2Bb" is that literal key, not the escape of "a+b". +func TestListMultipartUploadsUrlKeyMarkerIsRawKey(t *testing.T) { + filer := &fakeBucketDeleteFiler{entriesByDir: map[string][]*filer_pb.Entry{ + "/buckets/test/.uploads": { + uploadEntry("1", "c d", 10), + uploadEntry("2", "a+b", 10), + uploadEntry("3", "a%2Bb", 10), + uploadEntry("4", "z", 10), + uploadEntry("5", "c%20d", 10), + }, + }} + s3a := newFailoverTestServer(t, startFakeFiler(t, filer)) + s3a.option.BucketsPath = "/buckets" + s3a.bucketConfigCache = NewBucketConfigCache(time.Minute) + s3a.bucketConfigCache.Set("test", &BucketConfig{Name: "test"}) + s3a.iam = &IdentityAccessManagement{} + + r := httptest.NewRequest(http.MethodGet, "/test?uploads&"+url.Values{ + "encoding-type": {"url"}, + "key-marker": {"a%2Bb"}, + }.Encode(), nil) + r = mux.SetURLVars(r, map[string]string{"bucket": "test"}) + w := httptest.NewRecorder() + s3a.ListMultipartUploadsHandler(w, r) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + + var page listUploadsTestPage + require.NoError(t, xml.Unmarshal(w.Body.Bytes(), &page), w.Body.String()) + + var got []string + for _, upload := range page.Uploads { + key, err := url.QueryUnescape(upload.Key) + require.NoError(t, err) + got = append(got, key) + } + assert.Equal(t, []string{"a+b", "c d", "c%20d", "z"}, got) + + decoded, err := url.QueryUnescape(page.KeyMarker) + require.NoError(t, err) + assert.Equal(t, "a%2Bb", decoded) +} diff --git a/weed/s3api/filer_multipart_list_test.go b/weed/s3api/filer_multipart_list_test.go index 6e1b5c1dab..9b3f3c3d25 100644 --- a/weed/s3api/filer_multipart_list_test.go +++ b/weed/s3api/filer_multipart_list_test.go @@ -182,8 +182,8 @@ func TestSelectMultipartUploadsUrlEncodesMarkersAndKeys(t *testing.T) { // A raw key that already carries the token prefix must survive an // encoding-type=url round trip: under url mode the wire form is the url escape -// alone, so the request decode only unescapes — running the token decode on -// "base64:..." would misread the raw key as an encoded token. +// alone, so the client unescapes and hands the raw key back — running the +// token decode on "base64:..." would misread the raw key as an encoded token. func TestSelectMultipartUploadsUrlTokenPrefixedKeyRoundTrip(t *testing.T) { entries := []*filer_pb.Entry{ uploadEntry("1", "base64:AQ", 10), @@ -193,7 +193,10 @@ func TestSelectMultipartUploadsUrlTokenPrefixedKeyRoundTrip(t *testing.T) { input.EncodingType = aws.String(s3.EncodingTypeUrl) out := selectMultipartUploads(input, entries) - resume := decodeEscapedEcho(*out.NextKeyMarker, true, urlQueryEscape) + resume, err := url.QueryUnescape(*out.NextKeyMarker) + if err != nil { + t.Fatalf("next key marker %q does not unescape: %v", *out.NextKeyMarker, err) + } if resume != "base64:AQ" { t.Fatalf("next key marker %q resumes at %q, want %q", *out.NextKeyMarker, resume, "base64:AQ") } diff --git a/weed/s3api/s3api_object_handlers_list.go b/weed/s3api/s3api_object_handlers_list.go index 76614932ce..080d7321bd 100644 --- a/weed/s3api/s3api_object_handlers_list.go +++ b/weed/s3api/s3api_object_handlers_list.go @@ -107,10 +107,9 @@ func (s3a *S3ApiServer) ListObjectsV2Handler(w http.ResponseWriter, r *http.Requ return } - startAfterKey := decodeMarkerEcho(startAfter, encodingTypeUrl) marker := decodeListContinuationToken(continuationToken.string) if !continuationToken.set { - marker = startAfterKey + marker = startAfter } // Adjust marker if it ends with delimiter to skip all entries with that prefix @@ -155,7 +154,7 @@ func (s3a *S3ApiServer) ListObjectsV2Handler(w http.ResponseWriter, r *http.Requ } if encodingTypeUrl { responseV2.EncodingType = s3.EncodingTypeUrl - responseV2.StartAfter = urlPathEscape(startAfterKey) + responseV2.StartAfter = urlPathEscape(startAfter) } glog.V(3).Infof("ListObjectsV2Handler response: %+v", responseV2) @@ -197,25 +196,6 @@ func encodeMarkerEcho(marker string, encodingTypeUrl bool) string { return marker } -// decodeMarkerEcho reverses the wire encoding on a marker the client echoes -// back: under encoding-type=url the emitted echo was urlPathEscape'd. Only a -// value that is the canonical escaped form of its unescape could have come -// from that encoder — "a%2541" round-trips to itself, "a%41" does not — so -// anything else stays the raw key the client sent. -func decodeMarkerEcho(marker string, encodingTypeUrl bool) string { - return decodeEscapedEcho(marker, encodingTypeUrl, urlPathEscape) -} - -func decodeEscapedEcho(marker string, encodingTypeUrl bool, escape func(string) string) string { - if !encodingTypeUrl { - return marker - } - if unescaped, err := url.QueryUnescape(marker); err == nil && escape(unescaped) == marker { - return unescaped - } - return marker -} - func decodeListContinuationToken(token string) string { key, ok := strings.CutPrefix(token, continuationTokenEncodingPrefix) if !ok { @@ -297,7 +277,7 @@ func (s3a *S3ApiServer) ListObjectsV1Handler(w http.ResponseWriter, r *http.Requ } // Adjust marker if it ends with delimiter to skip all entries with that prefix - requestMarker := decodeListContinuationToken(decodeMarkerEcho(listMarker, encodingTypeUrl)) + requestMarker := decodeListContinuationToken(listMarker) marker := adjustMarkerForDelimiter(requestMarker, originalPrefix, delimiter) response, err := s3a.listFilerEntries(r.Context(), listObjectsRequest{ @@ -333,26 +313,31 @@ func sanitizeV1MarkerEcho(response *ListBucketResult, marker string, encodingTyp return } - markerCandidates := map[string]struct{}{ + // Content keys travel in the response wire encoding while NextMarker stays + // the raw key, so each compares against the marker in its own form. + rawCandidates := map[string]struct{}{ marker: {}, strings.TrimPrefix(marker, "/"): {}, } + keyCandidates := rawCandidates if encodingTypeUrl { - escapedMarker := urlPathEscape(strings.TrimPrefix(marker, "/")) - markerCandidates[escapedMarker] = struct{}{} + keyCandidates = make(map[string]struct{}, len(rawCandidates)) + for m := range rawCandidates { + keyCandidates[urlPathEscape(m)] = struct{}{} + } } - matchesMarker := func(v string) bool { - if _, ok := markerCandidates[v]; ok { + matchesMarker := func(candidates map[string]struct{}, v string) bool { + if _, ok := candidates[v]; ok { return true } - _, ok := markerCandidates[strings.TrimPrefix(v, "/")] + _, ok := candidates[strings.TrimPrefix(v, "/")] return ok } if len(response.Contents) > 0 { filtered := response.Contents[:0] for _, content := range response.Contents { - if matchesMarker(content.Key) { + if matchesMarker(keyCandidates, content.Key) { continue } filtered = append(filtered, content) @@ -362,7 +347,7 @@ func sanitizeV1MarkerEcho(response *ListBucketResult, marker string, encodingTyp // doListFilerEntries advances nextMarker to the last emitted entry and skips // the marker in exclusive mode. So NextMarker==marker indicates no progress. - if matchesMarker(response.NextMarker) && len(response.Contents) == 0 && len(response.CommonPrefixes) == 0 { + if matchesMarker(rawCandidates, response.NextMarker) && len(response.Contents) == 0 && len(response.CommonPrefixes) == 0 { response.NextMarker = "" response.IsTruncated = false } diff --git a/weed/s3api/s3api_object_handlers_list_test.go b/weed/s3api/s3api_object_handlers_list_test.go index a53e3421a4..8aa6a1d8a3 100644 --- a/weed/s3api/s3api_object_handlers_list_test.go +++ b/weed/s3api/s3api_object_handlers_list_test.go @@ -686,7 +686,7 @@ func TestSanitizeV1MarkerEcho_NoProgressGuard(t *testing.T) { response2 := ListBucketResult{ Marker: "test file.txt", - NextMarker: "test%20file.txt", + NextMarker: "test file.txt", IsTruncated: true, Contents: []ListEntry{ {Key: "test%20file.txt"}, diff --git a/weed/s3api/s3api_object_handlers_list_token_test.go b/weed/s3api/s3api_object_handlers_list_token_test.go index 75ffbdcf9f..f75a1045ab 100644 --- a/weed/s3api/s3api_object_handlers_list_token_test.go +++ b/weed/s3api/s3api_object_handlers_list_token_test.go @@ -108,19 +108,79 @@ func TestListObjectsV2ContinuationTokenWithXMLUnsafeKeys(t *testing.T) { } } -// The StartAfter echo is the wire encoding of the decoded key, so a client -// that feeds the echo back lands on the same resume point instead of -// drifting to the literal escape text. -func TestListObjectsV2StartAfterEchoStable(t *testing.T) { - s3a := newListTokenTestServer(t, []string{"a b", "c"}) - for _, sent := range []string{"a%20b", "a b"} { - result := listV2Page(t, s3a, url.Values{ - "list-type": {"2"}, "encoding-type": {"url"}, "start-after": {sent}, - }) - assert.Equal(t, "a%20b", result.StartAfter, "start-after %q", sent) - require.Len(t, result.Contents, 1) - assert.Equal(t, "c", result.Contents[0].Key) +// A paginating client decodes the url-encoded marker fields and hands the +// raw keys back, so a marker or key that is itself a valid escape must stay +// literal: decoding it again would list from a key the client never named. +func TestListObjectsV1UrlPaginationKeepsPercentEscapesLiteral(t *testing.T) { + keys := []string{"a%2Bb", "a+b", "c d", "c%20d", "z"} + s3a := newListTokenTestServer(t, keys) + + var got []string + marker := "" + for page := 0; ; page++ { + require.Less(t, page, len(keys), "pagination repeated a marker") + query := url.Values{"encoding-type": {"url"}, "max-keys": {"1"}} + if marker != "" { + query.Set("marker", marker) + } + result := listV1Page(t, s3a, query) + for _, content := range result.Contents { + key, err := url.QueryUnescape(content.Key) + require.NoError(t, err) + got = append(got, key) + } + if !result.IsTruncated { + break + } + decoded, err := url.QueryUnescape(result.NextMarker) + require.NoError(t, err) + marker = decoded } + assert.Equal(t, keys, got) +} + +// The StartAfter echo is the wire encoding of the raw request key: a client +// decodes it before sending it back, and a key that is itself a valid escape +// stays literal. +func TestListObjectsV2StartAfterEchoStable(t *testing.T) { + s3a := newListTokenTestServer(t, []string{"a b", "a%20b", "c"}) + for _, tc := range []struct { + sent string + echo string + rest []string + }{ + {"a b", "a%20b", []string{"a%20b", "c"}}, + {"a%20b", "a%2520b", []string{"c"}}, + } { + result := listV2Page(t, s3a, url.Values{ + "list-type": {"2"}, "encoding-type": {"url"}, "start-after": {tc.sent}, + }) + assert.Equal(t, tc.echo, result.StartAfter, "start-after %q", tc.sent) + var got []string + for _, content := range result.Contents { + key, err := url.QueryUnescape(content.Key) + require.NoError(t, err) + got = append(got, key) + } + assert.Equal(t, tc.rest, got, "start-after %q", tc.sent) + } +} + +// start-after names a raw key even when it looks like an escape: listing +// after "a%2Bb" must start after that literal key, not after "a+b". +func TestListObjectsV2UrlStartAfterKeepsPercentEscapesLiteral(t *testing.T) { + s3a := newListTokenTestServer(t, []string{"a%2Bb", "a+b", "c d", "c%20d", "z"}) + + result := listV2Page(t, s3a, url.Values{ + "list-type": {"2"}, "encoding-type": {"url"}, "start-after": {"a%2Bb"}, + }) + var got []string + for _, content := range result.Contents { + key, err := url.QueryUnescape(content.Key) + require.NoError(t, err) + got = append(got, key) + } + assert.Equal(t, []string{"a+b", "c d", "c%20d", "z"}, got) } // A token for a key that XML carries unchanged stays the bare key. @@ -216,24 +276,15 @@ func TestListObjectsV1MarkerWithXMLUnsafeKeys(t *testing.T) { break } require.NotEmpty(t, result.NextMarker) - marker = result.NextMarker + decoded, err := url.QueryUnescape(result.NextMarker) + require.NoError(t, err) + marker = decoded } require.Equal(t, keys, got) } -// A url-encoded marker is only a wire echo when re-escaping its unescaped -// form reproduces it: a real echo decodes, a raw key merely containing '%' -// or a malformed escape passes through untouched. -func TestDecodeMarkerEchoKeepsNonCanonicalRawKey(t *testing.T) { - assert.Equal(t, "a b", decodeMarkerEcho("a%20b", true)) - assert.Equal(t, "a%41", decodeMarkerEcho("a%2541", true)) - assert.Equal(t, "a%41", decodeMarkerEcho("a%41", true)) - assert.Equal(t, "a%2G", decodeMarkerEcho("a%2G", true)) - assert.Equal(t, "a%20b", decodeMarkerEcho("a%20b", false)) -} - // Under encoding-type=url AWS encodes the StartAfter echo the same as the -// listed keys; a client resuming from the emitted form lands on the raw key. +// listed keys; a client decodes the echo and resumes at the raw key. func TestListObjectsV2UrlStartAfterEchoAndResume(t *testing.T) { s3a := newListTokenTestServer(t, []string{"a b", "a b0", "a c", "b"}) @@ -248,8 +299,10 @@ func TestListObjectsV2UrlStartAfterEchoAndResume(t *testing.T) { } assert.Equal(t, []string{"a b0", "a c", "b"}, got) - // Echoing the emitted form back resumes at the same raw key. - result = listV2Page(t, s3a, url.Values{"list-type": {"2"}, "encoding-type": {"url"}, "start-after": {result.StartAfter}}) + // Resuming from the decoded echo lands on the same raw key. + decoded, err := url.QueryUnescape(result.StartAfter) + require.NoError(t, err) + result = listV2Page(t, s3a, url.Values{"list-type": {"2"}, "encoding-type": {"url"}, "start-after": {decoded}}) got = nil for _, content := range result.Contents { key, err := url.QueryUnescape(content.Key) diff --git a/weed/s3api/s3api_object_handlers_multipart.go b/weed/s3api/s3api_object_handlers_multipart.go index 19b802be57..780041f5cf 100644 --- a/weed/s3api/s3api_object_handlers_multipart.go +++ b/weed/s3api/s3api_object_handlers_multipart.go @@ -269,11 +269,12 @@ func (s3a *S3ApiServer) ListMultipartUploadsHandler(w http.ResponseWriter, r *ht s3err.WriteErrorResponse(w, r, s3err.ErrInvalidMaxUploads) return } - // The decode mirrors the response encoding: under encoding-type=url the - // markers go out url-escaped, otherwise they carry the token form. - keyMarker := decodeListContinuationToken(multipartKeyMarker) - if encodingType == s3.EncodingTypeUrl { - keyMarker = decodeEscapedEcho(multipartKeyMarker, true, urlQueryEscape) + // The marker is a raw key: under encoding-type=url clients decode the + // escaped response fields and send the key itself back. The token form + // only appears when the response could not carry the raw key. + keyMarker := multipartKeyMarker + if encodingType != s3.EncodingTypeUrl { + keyMarker = decodeListContinuationToken(multipartKeyMarker) } if keyMarker != "" { // Marker not common with prefix is not implemented.