Skip to content

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Mar 16, 2023

Currently in the process of trying to create some sample concerning CORS configuration of S3, and making a small sample allowing people to create their own CORS rules for their bucket but still allowing the app.localstack.cloud / resource browser to access it, I've encountered an issue with the ListBuckets operation.

S3 is a "self-managed" CORS service, as it can manage the CORS setting of its buckets. However, the ListBuckets operation is not configurable, and should be using the default CORS setting of LocalStack. I've added a check in our general CORS handler to not skip setting CORS headers if the operation is ListBuckets.

While creating the test, I've also found that if you try to access http://localhost:4566, it should return ListBuckets, but our service name parser does not recognise the request as it's not signed.
Funnily, in AWS, if you do the same and try to access s3.amazonaws.com which is the default URL for ListBuckets in us-east-1 without signing the request, you get redirected to https://aws.amazon.com/s3/.

If you use s3.localhost.localstack.cloud without signing the request, you'll get the response for ListBuckets because of the host matching.
Should we match on the path being / as a sign of an S3 request?

@bentsku bentsku requested a review from thrau as a code owner March 16, 2023 20:03
@bentsku bentsku temporarily deployed to localstack-ext-tests March 16, 2023 20:03 — with GitHub Actions Inactive
@coveralls
Copy link

Coverage Status

Coverage: 85.124% (+0.0006%) from 85.123% when pulling 31052e2 on fix-s3-cors into 77aab87 on master.

@github-actions
Copy link

LocalStack integration with Pro

       2 files   -     1         2 suites   - 1   1h 32m 12s ⏱️ - 10m 51s
1 797 tests +    1  1 411 ✔️  -     3  386 💤 +    4  0 ±0 
2 148 runs   - 366  1 582 ✔️  - 198  566 💤  - 168  0 ±0 

Results for commit 31052e2. ± Comparison against base commit 77aab87.

Copy link
Member

@thrau thrau left a comment

Choose a reason for hiding this comment

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

s3 is just one big corner case 😂

i think this solution is great for now! we can think about cleaning this up after we remove the old provider and re-organize the handler chain.

@thrau thrau merged commit f340f2b into master Mar 16, 2023
@thrau thrau deleted the fix-s3-cors branch March 16, 2023 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants