-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
S3/ASF parser: fix parsing header list
shape type with multivalue headers
#13022
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
S3 Image Test Results (AMD64 / ARM64) 2 files 2 suites 8m 39s ⏱️ Results for commit ac1d143. |
Test Results - Preflight, Unit22 140 tests 20 405 ✅ 6m 15s ⏱️ Results for commit ac1d143. |
Test Results (amd64) - Acceptance7 tests 5 ✅ 3m 4s ⏱️ Results for commit ac1d143. |
Test Results (amd64) - Integration, Bootstrap 5 files 5 suites 2h 21m 10s ⏱️ Results for commit ac1d143. |
LocalStack Community integration with Pro 2 files 2 suites 1h 43m 3s ⏱️ Results for commit ac1d143. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a nice catch! LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely great catch! I am just wondering about a super-edge case, which really doesn't block a merge here. 💯
# headers may contain a comma separated list of values (e.g., the ObjectAttributes member in | ||
# s3.GetObjectAttributes), so we prepare it here for the handler, which will be `_parse_list`. | ||
# Header lists can contain optional whitespace, so we strip it | ||
# https://www.rfc-editor.org/rfc/rfc9110.html#name-lists-rule-abnf-extension | ||
payload = [value.strip() for value in payload.split(",")] | ||
# It can also directly contain a list of headers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought (non-blocking): Is it also possible that there are multiple headers with the same key, some contain a single value, and some contain multiple values (comma-separated)?
Motivation
We got a report with #12999 that sending a
GetObjectAttributes
request would fail depending on the order of the fields being passed.While debugging and printing out low level HTTP request on the Java side, I realized that Java was sending the request with multiple headers for the same key, like some others SDK (Go in particular)
When receiving headers that way, our web server will return them as a list of values, accessible via
Headers.getlist()
.Because the parser was calling
Headers.get()
, it would only get the first value, and in the user sample above, would not even return the second requested attribute.This PR adapts the parser to use
getlist
in the header shape type islist
, and keep the old automatic splitting on,
that is still required.Changes
headers
override! This was so much easier to write a test for it!!!)requests
concatenates multivalue headers by default buturllib3
does not-Testing
If you remove the parser fix, the tests now fail.
I also validated that the user-provided sample in the linked issue now works 👍