fix(services): encode version id for cos and tos - #7888
Conversation
Signed-off-by: QuakeWang <wangfuzheng0814@foxmail.com>
dosubot
Bot
added
size:L
|
Can we have a test for versioned object in behavior test? |
Signed-off-by: QuakeWang <wangfuzheng0814@foxmail.com>
@erickguan Thanks for the suggestion. I have updated the behavior tests to cover versioned object operations more explicitly. PTAL 👀 |
|
Alright, I tried S3. S3 seems to respond with proper URI without breaking anything at least in behavior tests. Will have a look. |
| fn restore_encoded_query( | ||
| uri: &mut Uri, | ||
| encoded_query: Option<&str>, | ||
| keep_signed_query: bool, |
There was a problem hiding this comment.
Do we need this? For URI to be correct, I believe we always will need encoded query param.
| .await | ||
| .map_err(|e| new_request_sign_error(e.into()))?; | ||
|
|
||
| Self::restore_encoded_query(&mut parts.uri, encoded_query.as_deref(), true)?; |
There was a problem hiding this comment.
I didn't look into details. Can we build a new parts instead of mutating it?
Signed-off-by: QuakeWang <wangfuzheng0814@foxmail.com>
There was a problem hiding this comment.
I believe the real problem is reqsign didn't keep the raw query as is, we should fix at reqsign side so we don't need to have this query restore logic.
And before we are going too crazy: version id returned by services is always picked and not a randomly generated string. So I'm ok to really fix it but I don't want to merge PR like this one.
Both percent_encode_path and percent_decode_path here are all no-op (at least for 99% cases, I didn't see a real report).
@Xuanwo Thanks for the review. I agree that the root fix should happen in reqsign-core instead of adding service-local query restoration logic in OpenDAL. I've opened an issue to track it: apache/opendal-reqsign#778 I'll pause this PR and will fix the reqsign side firstly, then revisit this change with the workaround removed. |
|
Hi, reqsign has been upgraded, please check this again |
…8073) 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 #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.
3 participants
Which issue does this PR close?
Closes #.
Rationale for this change
COS and TOS used decoded version IDs when building
versionIdquery parameters for read/stat/delete requests. If a version ID contains query-reserved characters like&,=,+, or%, the generated URI can be parsed as different query parameters instead of a single encodedversionId.What changes are included in this PR?
versionIdquery values before appending them to request URIs.Are there any user-facing changes?
Yes. COS and TOS read/stat/delete operations with version IDs containing query-reserved characters now build valid request URIs.
AI Usage Statement
I used OpenAI Codex to implement the test in this change.