Skip to content

fix(s3): set NextMarker in ListObjects V1 response when truncated#1048

Merged
ferhatelmas merged 3 commits into
supabase:masterfrom
oniani1:fix/s3-listobjects-v1-nextmarker
Apr 27, 2026
Merged

fix(s3): set NextMarker in ListObjects V1 response when truncated#1048
ferhatelmas merged 3 commits into
supabase:masterfrom
oniani1:fix/s3-listobjects-v1-nextmarker

Conversation

@oniani1
Copy link
Copy Markdown
Contributor

@oniani1 oniani1 commented Apr 23, 2026

Closes #1047.

Summary

S3 ListObjects V1 has to return NextMarker alongside IsTruncated so that pagination-aware clients can advance without reverse-engineering the cursor off Contents. S3ProtocolHandler.listObjects never emitted it. The next-page hint was leaking into the Marker field instead, and any client that specifically read NextMarker got undefined.

Sets NextMarker to the V2 NextContinuationToken, which under cursorV1: true is the plain last key (results.nextCursorKey). Only emitted when IsTruncated === true and the token is present, via a conditional spread, so the XML serializer does not render an empty <NextMarker/> element in the non-truncated case.

Marker is intentionally left unchanged. It is still populated with the next-page hint rather than the echoed input Marker per the spec. The AWS SDK paginator prefers NextMarker now that it is available, and changing Marker's value is a backwards-incompatible change for any client that has grown to depend on it. Tracked separately in the issue.

Test plan

Three tests added / adjusted in src/test/s3-protocol.test.ts under ListObjectCommand:

  • sets NextMarker when the response is truncated. Flat listing, confirms NextMarker is defined and usable as next request's Marker.
  • omits NextMarker when the response is not truncated. Confirms the field is absent (not empty string).
  • sets NextMarker when truncated with a delimiter. Delimiter listing, confirms NextMarker carries the cursor and feeding it back returns the remaining common prefixes.

All ListObjectCommand tests pass against a local Docker storage stack; existing ListObjectsV2Command tests are unaffected.

@oniani1 oniani1 requested a review from a team as a code owner April 23, 2026 19:54
@coveralls
Copy link
Copy Markdown

coveralls commented Apr 23, 2026

Coverage Report for CI Build 24995061090

Coverage increased (+0.005%) to 71.588%

Details

  • Coverage increased (+0.005%) from the base build.
  • Patch coverage: 1 of 1 lines across 1 file are fully covered (100%).
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 9767
Covered Lines: 7392
Line Coverage: 75.68%
Relevant Branches: 5466
Covered Branches: 3513
Branch Coverage: 64.27%
Branches in Coverage %: Yes
Coverage Strength: 378.93 hits per line

💛 - Coveralls

Comment thread src/storage/protocols/s3/s3-handler.ts Outdated
Name: v2Result.Name,
Prefix: v2Result.Prefix,
Marker: v2Result.NextContinuationToken,
...(v2Result.IsTruncated && v2Result.NextContinuationToken
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This element is returned only if you have the delimiter request parameter specified

I think we should handle this condition as well and adjust tests accordingly

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in ada90f1. Gated the NextMarker spread on command.Delimiter and flipped the no-delimiter test to assert NextMarker is undefined.

EncodingType: list.responseBody.ListBucketResult.EncodingType,
Name: v2Result.Name,
Prefix: v2Result.Prefix,
Marker: v2Result.NextContinuationToken,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Marker: v2Result.NextContinuationToken,
...(command.Marker !== undefined ? { Marker: command.Marker } : {}),

@supabase/storage I think we should update this separately for compliance. It could be breaking theoretically for some but I guess minimal risk

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Held off on this one in the current push. Details in the top-level reply above: the pre-existing list all keys with pagination test paginates via resp.Marker, and swapping to command.Marker makes that undefined on the first page and breaks external callers doing the same. Happy to fold it in here with a test migration, or split it into a follow-up PR so this can merge cleanly. Let me know which you prefer.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change is good as it is to fix NextMarker.

Let's prepare Marker issue separately to make it obvious so that we can refer people to the exact minimal change if they are affected and mark it breaking.

@oniani1
Copy link
Copy Markdown
Contributor Author

oniani1 commented Apr 24, 2026

Thanks @ferhatelmas! Addressed the NextMarker + delimiter condition in ada90f1. Tightened the condition to require command.Delimiter and flipped the no-delimiter test to assert NextMarker is omitted, matching the spec note that NextMarker is only returned when Delimiter is set.

On the Marker line, good catch on the compliance gap. I held off bundling that one here because of the behavior change you flagged. The pre-existing list all keys with pagination test paginates via resp.Marker, which suggests external callers likely do the same. Swapping to command.Marker makes resp.Marker undefined on the first page and breaks those pagination loops.

Happy to either (a) add it here with a test migration to resp.NextMarker and a note in the PR description, or (b) split it into a follow-up PR so this one can merge cleanly. Which do you prefer?

oniani1 added 3 commits April 27, 2026 14:29
S3 V1 ListObjects is expected to return NextMarker alongside IsTruncated so
pagination-aware clients (like the AWS SDK v3 paginator, which reads
output.NextMarker before falling back to the last Contents key) can continue
iterating. The handler was never populating it, so a client that specifically
keyed off NextMarker got undefined.

Sets NextMarker to the underlying V2 NextContinuationToken (which is the plain
last-key when cursorV1 is set). Only emitted when IsTruncated is true and the
value is present, so the XML serializer does not render an empty <NextMarker/>
element in the non-truncated case.

Marker is intentionally left unchanged here. It is still populated with the
next-page hint rather than the echoed input Marker the spec asks for, but
fixing that is a backwards-incompatible change for clients that coincidentally
work against today's value; tracked separately.
Biome flagged the client.send(new ListObjectsCommand(...)) call in the new
truncation test as a multi-line expression that should collapse onto a single
line. Autoformat applied, no behavior change.
Per the S3 ListObjects V1 spec, NextMarker is only returned when the
Delimiter request parameter is specified. Tighten the condition to
require command.Delimiter in addition to IsTruncated, and flip the
no-delimiter test to assert NextMarker is omitted.
@ferhatelmas ferhatelmas force-pushed the fix/s3-listobjects-v1-nextmarker branch from ada90f1 to 50b8b1c Compare April 27, 2026 12:29
@ferhatelmas ferhatelmas merged commit 7f6ea90 into supabase:master Apr 27, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

S3 ListObjects V1 response never sets NextMarker on truncated results

3 participants