fix(services): percent-encode the pagination marker in list queries - #8073
Merged
Conversation
QueryPairsWriter::push does no escaping of its own and says so:
/// The input key and value must already been percent
/// encoded correctly.
Five list builders hand it a raw marker. The value is whatever the
server returned as the resume point -- an object key for obs, cos and
swift, an opaque continuation token for azblob and azfile -- and for
swift's first page it is the user's own start_after argument, so no
pagination is needed to reach it.
Feeding the exact strings to http::Request::get shows what is sent:
report 2024.csv BUILD ERROR: invalid uri character
a&b=c marker=a plus a stray b=c parameter
note#1.txt marker=note -- the rest is taken as a fragment
AB+cd== marker=AB+cd==, whose + a server decodes as a space
The middle two are the dangerous ones: the marker silently rewinds, the
server resends a page that was already delivered, and since done is
computed from the marker being empty the listing can repeat that page
indefinitely.
The repository already does this correctly in three places, one of them
in the same file as a site being fixed here:
s3/src/core.rs:803 push("marker", &percent_encode_path(marker))
cos/src/core.rs:606 push("key-marker", &percent_encode_path(key_marker))
oss/src/core.rs:478 push("continuation-token", &percent_encode_path(token))
percent_encode_path is the right helper for a marker specifically: it
leaves `/` alone, which an object key needs. That is the difference from
apache#7888, which is encoding cos and tos versionId values in the same family
of defect and introduces a stricter set for them -- a version id is not
a path, a marker is.
No new tests. Each change is one call to a helper the file already
imports, and asserting on it would mean restructuring five URL builders,
which I would rather not fold into a fix. All 39 existing unit tests
across the five crates pass, and none of them assert on a marker URL.
dosubot
Bot
added
size:S
Xuanwo
pushed a commit
that referenced
this pull request
Aug 14, 2026
…re written (#8075) Two independent path defects in this service. 1. The batched lister's startAfter is not encoded. let mut url = format!( "{}/webhdfs/v1/{}?op=LISTSTATUS_BATCH", self.endpoint, percent_encode_path(&p), // the path is encoded ); if !start_after.is_empty() { url += format!("&startAfter={start_after}").as_str(); // this is not } start_after is ctx.token, which lister.rs sets verbatim from the last entry's pathSuffix -- a raw HDFS file name. Measured against http::Request::get: a name with a space is a hard "invalid uri character" build error, one with # truncates the marker so the batch boundary rewinds and a page repeats, and one with & grafts a stray parameter onto the query. This is the same defect just fixed for the marker parameter across obs/swift/cos/azblob/azfile in #8073; it was out of that PR's scope only because it is a format! concatenation rather than a QueryPairsWriter push. startAfter is the one query value in this file carrying data the server chose. user.name is config, at eleven sites, and is a separate question; &{auth} is deliberately a whole query fragment and must stay verbatim. 2. abort_block deletes paths that were never written. write_block creates each block at {atomic_write_dir}{block_id} and complete_block concatenates from the same strings, but abort_block asked for {block_id} alone. So aborting a multi-block write -- Writer::abort, or any mid-write failure -- deleted a path that does not exist and left every uploaded block sitting in atomic_write_dir for ever. It now resolves atomic_write_dir the same way write_block does, which also makes the unsupported case explicit rather than deleting a top-level path named after a UUID. No new tests: both are single expressions inside async methods whose seams are an HTTP round trip, and asserting on either would mean restructuring the URL builder and the writer. The seven existing unit tests pass, fmt and clippy are clean.
This was referenced Aug 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
None — found while reading the listers.
Rationale for this change
QueryPairsWriter::pushdoes no escaping of its own, and says so:Five list builders hand it a raw marker:
obs/src/core.rs:354url.push("marker", next_marker)swift/src/core.rs:217url.push("marker", marker)cos/src/core.rs:413url.push("marker", next_marker)azblob/src/core.rs:804url.push("marker", next_marker)azfile/src/core.rs:436url.push("marker", continuation)The value is whatever the server returned as the resume point — a raw object key for obs, cos and swift, an opaque continuation token for azblob and azfile. For swift's first page it is the user's own
start_afterargument (lister.rs:65-69), so no pagination is needed to reach it at all.Feeding those exact strings to
http::Request::getshows what actually goes on the wire:report 2024.csvBUILD ERROR: invalid uri character— the wholelistfailsa&b=cmarker=aplus a strayb=cparameternote#1.txtmarker=note— everything from#is taken as a fragmentAB+cd==marker=AB+cd==, whose+a server decodes as a spaceThe middle two are the dangerous ones: the marker silently rewinds, the server resends a page that was already delivered, and because
doneis computed from the marker coming back empty, the listing can repeat that page indefinitely rather than erroring.What changes are included in this PR?
One call to
percent_encode_pathat each of the five sites. The repository already does exactly this in three places — one of them in the same file as a site being fixed:percent_encode_pathis the right helper for a marker specifically: it leaves/alone, which an object key needs. That is the difference from #7888, which is fixing the same family of defect for cos/tosversionIdand introduces a stricter set for it — a version id is not a path, a marker is.Tests
No new tests, deliberately. Each change is a single call to a helper the file already imports, and asserting on it would mean restructuring five URL builders — which I would rather not fold into a fix. All 39 existing unit tests across the five crates pass, and none of them assert on a marker URL.
cargo fmt --all -- --checkandcargo clippy --all-targetson each of the five crates are clean with zero warnings.Happy to extract a testable URL builder for any of them if you would like the coverage.
Are there any user-facing changes?
Yes — a truncated listing whose resume marker contains a character that is reserved in a query now resumes at the right place instead of failing to build the request, silently rewinding, or repeating a page. The decoded value the server receives is unchanged for any marker that was already URL-safe.