Skip to content
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 post not passing all fields #24

Merged
merged 1 commit into from
Apr 16, 2024
Merged

fix S3 post not passing all fields #24

merged 1 commit into from
Apr 16, 2024

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Apr 11, 2024

We've had an issue since the LocalStack PR: localstack/localstack#10616, raised by @thrau

image (2)

We've added better validation of S3 POST policy conditions, and some SDK set them automatically like in this case.
But when creating and passing the form, we would not forward all the fields created by the pre-signed POST, so the validation would fail.

I've also created an AWS validated test here: localstack/localstack#10641
To validate that LocalStack handles it correctly (we might return some different exceptions in case of invalid credentials, but that's alright for now). Anyway, AWS does not ignore any fields and will fail as well if we would try to do what the sample does at the moment.

I've created the form manually and appended the fields to it:
https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest_API/Using_FormData_Objects

The file field needs to be last, so I've created the form from scratch instead of using the element to fetch it. Otherwise, we would need to create hidden fields in the form for every field returned by the pre-signed.

Also see https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/Package/-aws-sdk-s3-presigned-post/ for the pre-signed POST doc on how you can use it in browser.

I've also sneaked a fix to properly delete the build image when running on macOS.

Also did a quick S3 change in the lambda creating the pre-signed, as we would always call create_bucket to check the bucket exists, but this is only omnipotent in us-east-1 and would raise an exception if done in any other region, so I've did it with head_bucket instead.

@bentsku bentsku added the bug Something isn't working label Apr 11, 2024
@bentsku bentsku requested a review from thrau April 11, 2024 21:22
@bentsku bentsku self-assigned this Apr 11, 2024
Copy link

github-actions bot commented Apr 11, 2024

⚡️ Running CI build with LocalStack ...

@thrau
Copy link
Contributor

thrau commented Apr 12, 2024

thanks so much for jumping on this @bentsku, even though it was clearly a client-side issue here 🦸

i'll let @lakkeger take over the review since he is the owner of the sample :-)

@thrau thrau removed their request for review April 12, 2024 18:28
Copy link
Contributor

@lakkeger lakkeger left a comment

Choose a reason for hiding this comment

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

Sorry for the wait and thanks for addressing this issue.
Great to see the project improving as we exposed it to a larger audience.

@lakkeger lakkeger merged commit 8cb7ca9 into main Apr 16, 2024
3 of 4 checks passed
@alexrashed alexrashed deleted the fix-s3-post branch April 16, 2024 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants