s3: take list markers as raw keys under encoding-type=url (#12275)

* s3: take ListObjects marker/start-after as raw keys

Under encoding-type=url AWS url-encodes the response fields but treats
request markers as raw keys: SDKs decode Marker/NextMarker/StartAfter and
send the decoded key back. Unescaping the marker server-side read a literal
"%XX" key as the key it encodes — StartAfter=a%2Bb resumed after "a+b",
skipping it, and a paginator whose marker decoded to an earlier key got the
same NextMarker twice and failed.

Take the request value as-is; only the server's own base64: continuation
token still decodes. The marker-echo dedup now compares contents in the
response wire encoding and NextMarker in its raw form, since a raw "%XX"
marker equals the escaped emission of a different key.

* s3: take ListMultipartUploads key-marker as a raw key

Same raw-marker rule as ListObjects: under encoding-type=url the emitted
key markers are url-escaped keys, and a client decodes them before sending
the marker back, so the request value is already the raw key. Keeping the
token decode only for the non-url path, where the response could not carry
the raw key and falls back to the base64: token form.

* s3: cover ListMultipartUploads url key-marker at the handler

The selectMultipartUploads tests bypass the request marker decoding, so a
regression that unescapes an encoding-type=url key-marker would pass them.
Drive a listing through ListMultipartUploadsHandler with a literal
"a%2Bb" marker and assert it resumes after that exact key.

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>

---------

Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>

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)
+}

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")
 	}

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
 	}

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"},

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)

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.