Skip to content

Adding support for partial wildcards for S3 CORS #10365

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.

Sign up for GitHub

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

Jump to bottom
Merged
bentsku merged 6 commits into from
Mar 6, 2024
Merged

Adding support for partial wildcards for S3 CORS #10365

bentsku merged 6 commits into from
Mar 6, 2024

Conversation

@cloutierMat
Copy link
Member

@cloutierMat cloutierMat commented Mar 1, 2024

Motivation

Fixing #10090 . AWS S3 CORS allows for partial wildcard "http://*.example.com". The current changes will allow OPTIONS requests to go through instead of returning a 403 error

Changes

  • Replace the logic of looking for an exact match from Origin header to configured allowed origins by a regex that will match anything in lieu of the *.

Testing

As mentioned on the issues description running the following command targeting an S3 bucket. With CORS configured as follow.

$ aws s3api get-bucket-cors --bucket foo --endpoint-url=http://localhost:4566                                                          
{
    "CORSRules": [
        {
            "AllowedHeaders": [
                "*"
            ],
            "AllowedMethods": [
                "GET",
                "PUT"
            ],
            "AllowedOrigins": [
                "http://*.example.com",
                "https://app.localstack.cloud",
                "http://app.localstack.cloud"
            ]
        }
    ]
}

$ curl -v --request OPTIONS 'http://127.0.0.1:4566/foo/' -H 'Origin: http://subd.example.com' -H 'Access-Control-Request-Method: GET'

@localstack-bot
Copy link
Contributor

localstack-bot commented Mar 1, 2024 •
edited
Loading

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

localstack-bot
localstack-bot reviewed Mar 1, 2024
View reviewed changes
Copy link
Contributor

@localstack-bot localstack-bot left a comment

Choose a reason for hiding this comment

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

Welcome to LocalStack! Thanks for raising your first Pull Request and landing in your contributions. Our team will reach out with any reviews or feedbacks that we have shortly. We recommend joining our Slack Community and share your PR on the #community channel to share your contributions with us. Please make sure you are following our contributing guidelines and our Code of Conduct.

@cloutierMat
Copy link
Member Author

cloutierMat commented Mar 1, 2024

I have read the CLA Document and I hereby sign the CLA

localstack-bot added a commit that referenced this pull request Mar 1, 2024
@localstack-bot
@cloutierMat has signed the CLA from Pull Request #10365
e084ee7
@cloutierMat
Copy link
Member Author

cloutierMat commented Mar 1, 2024

recheck

@alexrashed alexrashed added the semver: patch Non-breaking changes which can be included in patch releases label Mar 1, 2024
@alexrashed alexrashed self-assigned this Mar 1, 2024
@alexrashed alexrashed self-requested a review March 1, 2024 06:59
@alexrashed alexrashed mentioned this pull request Mar 1, 2024
Merged
@cloutierMat
Copy link
Member Author

cloutierMat commented Mar 1, 2024

Should I merge those latest changes coming from #10366 that fix the build to trigger again all of those pipelines?

@bentsku
Copy link
Contributor

bentsku commented Mar 1, 2024

@cloutierMat yes, you can try! Very sorry again that those build was red, it was a bit tricky to detect the fork was coming from an outside repo. You can merge/rebase on master and we will see if it works.

bentsku
bentsku approved these changes Mar 1, 2024
View reviewed changes
Copy link
Contributor

@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

LGTM! Great change, quick and clean fix, and a nice set of tests validating it. You did a great job picking up the tools, thank you again for this fix!

localstack/services/s3/cors.py
if "*" not in rule["AllowedOrigins"] and not any(
# Escapes any characters that needs escaping and replaces * with .+
# Transforms http://*.localhost:1234 to http://.+\\.localhost:1234
re.match(re.escape(allowed_origin).replace("\\*", ".+") + "$", origin)
Copy link
Contributor

@bentsku bentsku Mar 1, 2024

Choose a reason for hiding this comment

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

Nice escaping + adding the end of line terminator to avoid having bad origin like you tested! That's a clean fix 👌

tests/aws/services/s3/test_s3_cors.py
@markers.aws.validated
@markers.snapshot.skip_snapshot_verify(
paths=[
"$..Body.Error.ResourceType",
Copy link
Contributor

@bentsku bentsku Mar 1, 2024

Choose a reason for hiding this comment

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

I can see you've uncovered a parity issue we've missed earlier! I can see the reason why this is wrong, we always set it to OBJECT because we never tested it against the bucket only with no object key in the path. Good catch!
We could tackle this small issue in a follow-up PR one day. Great!

Copy link
Member Author

@cloutierMat cloutierMat Mar 1, 2024

Choose a reason for hiding this comment

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

Oh yes, I should probably have left a TODO comment at this effect.

@bentsku bentsku added the aws:s3 Amazon Simple Storage Service label Mar 1, 2024
@bentsku bentsku linked an issue Mar 1, 2024 that may be closed by this pull request
bug: S3 CORS is not honoring subdomain * syntax #10090
Closed
1 task
@bentsku bentsku merged commit 690fa80 into localstack:master Mar 6, 2024
@thrau
Copy link
Member

thrau commented Mar 6, 2024

welcome to the list of contributors @cloutierMat !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@localstack-bot localstack-bot localstack-bot left review comments

@bentsku bentsku bentsku approved these changes

@macnev2013 macnev2013 Awaiting requested review from macnev2013

@alexrashed alexrashed Awaiting requested review from alexrashed

Assignees

@alexrashed alexrashed

@bentsku bentsku

Labels

aws:s3 Amazon Simple Storage Service semver: patch Non-breaking changes which can be included in patch releases

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

bug: S3 CORS is not honoring subdomain * syntax

5 participants

@cloutierMat @localstack-bot @bentsku @thrau @alexrashed