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
fix S3 service name parsing consuming the stream #9874
Merged
Merged
Conversation
This file contains 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
bentsku
added
aws:s3
Amazon Simple Storage Service
area: asf
semver: patch
Non-breaking changes which can be included in patch releases
labels
Dec 14, 2023
1 task
bentsku
requested review from
macnev2013,
alexrashed and
thrau
as code owners
December 14, 2023 16:42
thrau
approved these changes
Dec 18, 2023
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.
thanks for fixing this! as discussed offline, i think this solution is the best we can do for now!
maybe two things we can add in the future (as discussed also, not needed for this PR)
- get service indicators not only from headers but also from query args to cover pre-signed URLs
- instead of using
request.values
which triggers form parsing, first userequest.args
and then check if the body can be parsed as form (which would perhaps cover some rare pre-signed URL cases, but we should probably first find specific cases)
just have tiny suggestion for a better logging message
bentsku
force-pushed
the
fix-s3-too-large
branch
from
December 21, 2023 23:18
a9d0985
to
7cf1bd4
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
area: asf
aws:s3
Amazon Simple Storage Service
semver: patch
Non-breaking changes which can be included in patch releases
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.
Motivation
As reported in #9867, we have an issue when using S3 pre-signed URL from a browser encoding the request as
application/x-www-form-urlencoded
and targeting a default LocalStack endpoint and not the S3 specific one. TheServiceNameParser
will try to determine the AWS service, and as S3 is the "fallback" service, it will go through the whole service detection, which includes accessingrequest.values
. This will:utf-8
.Changes
When targeting S3, we have an early handler trying to determine if the request is targeting S3 by checking the bucket name and verifies that it exists. This will allow to actually skip the service name parsing, as we already set the service in the context there.
We also need to have the form parsing around a
try...exception
block in case the bucket does not exist, because the S3 CORS handler will not mark it as an S3 request, and the decoding of the form will fail. We will just swallow the exception and pass so that the service name parser tries the next step. This does not matter that it consumes the stream, because it will lead in an exception anyway.