Extend allowed CORS origins to include EDGE_PORT_HTTP endpoint #7554
Conversation
localstack/aws/handlers/cors.py
Outdated
| result = [ | ||
| # allow access from Web app and localhost domains | ||
| "https://app.localstack.cloud", | ||
| "http://app.localstack.cloud", | ||
| "https://localhost", | ||
| "https://localhost.localstack.cloud", | ||
| # for requests from Electron apps, e.g., DynamoDB NoSQL Workbench | ||
| "file://", | ||
| ] | ||
| # Add allowed origins for localhost domains, using different protocol/port combinations. | ||
| # If a different port is configured for EDGE_PORT_HTTP, add it to allowed origins as well | ||
| _ports = set([config.EDGE_PORT] + ([config.EDGE_PORT_HTTP] if config.EDGE_PORT_HTTP else [])) | ||
| for protocol in {"http", "https"}: | ||
| for port in _ports: | ||
| result.append(f"{protocol}://localhost:{port}") | ||
| result.append(f"{protocol}://localhost.localstack.cloud:{port}") |
There was a problem hiding this comment.
one question: how does HOSTNAME_EXTERNAL factor into this? should we consider this variable at all? i can also see we're hardcoding localhost.localstack.cloud instead of using LOCALHOST_HOSTNAME, is that intentional?
/cc @simonrw
There was a problem hiding this comment.
Good point - adding LOCALHOST_HOSTNAME instead of hardcoding the string makes sense! (will update..)
HOSTNAME_EXTERNAL is slightly different and does not directly factor into this - HOSTNAME_EXTERNAL would be the hostname we return in URLs that point back to LocalStack APIs (e.g., SQS queue URLs), whereas the CORS origins are Web applications (accessed via the browser) that are permitted to access our APIs.
There might potentially be a case for either S3 websites or CloudFront distributions where we'd want to make special subdomains able to access our APIs via CORS (to be able to serve HTML/JS pages using endpoints like http://mybucket.s3.localhost.localstack.cloud:4566/mypage.html). /cc @bentsku I'd say we can tackle those in a follow-up, once we have a better understanding of those use cases (may require regex matching on the domains, which could add a bit more complexity). 👍
There was a problem hiding this comment.
Just chiming in here: For S3, this is already implemented by using a "preprocess" handler (which performs S3 specific CORS handling before the actual CORS handler is called). This doesn't really add additional CORS origins, but handles dynamic CORS rules which can be configured on a bucket-level in S3.
However, if we find more use cases where the CORS handling has to be adjusted for a specific service (f.e. cloudfront or API gateway), we might want to implement an explicit API for services such that they can influence the CORS rules and enforcement.
There was a problem hiding this comment.
Coming back to this, sorry for being so late, working with CORS at the moment.
I think I understand better the use case now: we'd want to allow our own hosted S3 website to access other LocalStack API. Configuring S3 CORS rules would not matter, as it is the other way around (allowing some JS code hosted in our bucket to target our LocalStack instance).
Also, just watching the demo app, it doesn't seem to me that we're deploying this bucket as a S3 Website. We're just serving an HTML file, but we're not supporting any S3 website features. An S3 website would be accessible using http://<BUCKET_NAME>.s3-website.localhost.localstack.cloud:<port> only, in the same fashion as AWS.
Coming back to the use case, we could either regex match our known domains, or we could dynamically add origins to our CORS handler.
7f2a3ff to
2184d22
Compare
8a7dcee to
f4203c9
Compare
There was a problem hiding this comment.
LGTM! This solution looks good to me, it's nice that it consolidates the CORS rules again (legacy and new gateway). I think in the long run we might want to iterate over it again, since - as mentioned in a comment above - I think it might become more important that certain services can have an influence on the CORS rules (either to get an exclusion such that they can serve content which accesses localstack, or to influence the CORS rules for their own resources in a dynamic way).
Extend allowed CORS origins to include
EDGE_PORT_HTTPendpoint. This change ensures that thehttp://localhost:4566endpoint is in theALLOWED_CORS_ORIGINSlist (for Community + Pro users).The issue surfaced when deploying our demo sample app Web UI (HTML file) as an S3 website, and testing against LocalStack Pro: In the default Pro setup, we have
EDGE_PORT=443andEDGE_PORT_HTTP=4566, and since we were previously configuring the allowed origins only viaconfig.EDGE_PORT, it was not possible to access the LocalStack APIs from a website running underhttp://localhost:4566(only when using port 443).Touching the CORS setup is always a bit finicky, but this change (1) should have no negative security side effects (we're only allowing the additional localhost port 4566, in addition to localhost port 443), and (2) ensures that we can offer S3 websites (e.g.,
http://localhost:4566/mybucket/index.html) that can connect directly to the LocalStack APIs, working for both Community and Pro.The PR adds a small integration test to cover the logic around
EXTRA_CORS_EXPOSE_HEADERS(existing logic, currently not yet covered), as well as a unit test to cover the new functionality around allowed localhost origins with different ports (incl.EDGE_PORT_HTTP).