improve CORS handler: no CORS headers if no Origin #9344
Conversation
bentsku
added
the
semver: minor
LocalStack Community integration with Pro 2 files ±0 2 suites ±0 1h 14m 47s ⏱️ + 3m 12s Results for commit 85c3295. ± Comparison against base commit 6665dde. This pull request removes 8 and adds 8 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
| @@ -258,3 +266,6 @@ def add_cors_headers(request_headers: Headers, response_headers: Headers): | |||
| and ACL_ALLOW_PRIVATE_NETWORK not in response_headers | |||
| ): | |||
| response_headers[ACL_ALLOW_PRIVATE_NETWORK] = "true" | |||
|
|
|||
| # we conditionally apply CORS headers depending on the Origin, so add it to `Vary` | |||
| response_headers["Vary"] = "Origin" | |||
| else "*" | ||
| ) | ||
| if "*" not in response_headers.get(ACL_ORIGIN, ""): | ||
| response_headers[ACL_CREDENTIALS] = "true" |
There was a problem hiding this comment.
Did we have any problems with that or is this just for completeness?
There was a problem hiding this comment.
I think I've seen the issue pops sometimes in Chrome or Safari where it would say that the request was not allowed because of this header missing. I believe it can be good to add it, as the browser could block some requests in the frontend if we don't include it.
When a request's credentials mode (Request.credentials) is include, browsers will only expose the response to the frontend JavaScript code if the Access-Control-Allow-Credentials value is true.
If the CORS request includes credentials, the response must include the Access-Control-Allow-Credentials: true header, and the value of Access-Control-Allow-Origin must reflect the request's Origin header (* isn't an acceptable value if the request has credentials).
As we control the list of allowed domains, I believe this can be useful, maybe for cognito login page for example?
We also could add a CORS_ALLOW_CREDENTIALS feature flag in the same way as flask_cors, to add this header only if it's set?
Flask-CORS docstring for it:
"""
:param supports_credentials:
Allows users to make authenticated requests. If true, injects the
`Access-Control-Allow-Credentials` header in responses. This allows
cookies and credentials to be submitted across domains.
:note: This option cannot be used in conjunction with a '*' origin
Default : False
:type supports_credentials: bool
"""There was a problem hiding this comment.
@dfangl do you think we should do something like above, or just merge this PR?
There was a problem hiding this comment.
I would just merge it, the impact is probably minimal anyway.
Motivation
By default, we always add and send the CORS headers to LocalStack responses.
However, those headers should be sent only when the request has an
Originheader set, indicating that this is a CORS request.We can read more about this here:
https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS#the_http_request_headers
https://jakearchibald.com/2021/cors/
When debugging and checking logs, we always have the CORS headers being sent, and they pollute the logs quite a lot for no benefits.
Changes
Added a check for the presence of the
Originheader, and add the headers only if it's there.Added the
Varyheader as we conditionally apply CORS headers depending on it.Added the credentials header.