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

Add Origin to Vary header on credentialed CORS response #1111

Merged
merged 4 commits into from Apr 6, 2021

Conversation

jcwilson
Copy link
Contributor

According to the MDN CORS docs, the Origin item should be added to the Vary header list when
the Access-Control-Allow-Origin is set to an explicit origin value when it could change due to
something like allow_origins being the * wildcard, a multi-item whitelist or allow_origin_regex
being in use.

If the server specifies a single origin (that may dynamically change based on the requesting origin
as part of a white-list) rather than the "*" wildcard, then the server should also include Origin in
the Vary response header — to indicate to clients that server responses will differ based on the
value of the Origin request header.

The existing code fails to update the Vary list when the server is configured to allow all
origins (*) and the request has a Cookie header (ie. credentialed). In that situation, the
Access-Control-Allow-Origin header will be set to the request's Origin value. It should
be noted that the code does currently update the Vary list when the allowed origins are
defined by an explicit whitelist (even if it only has one value and doesn't actually vary) or
a regex pattern.

It appears this may have just been a simple oversight in the original implementation. This updates
the code to add Origin to the Vary header under these circumstances. If it was intentionally
omitted, I'd be delighted to learn why.

Josh Wilson added 2 commits November 26, 2020 02:09
According to the [MDN CORS docs]
(https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS#Access-Control-Allow-Origin), the `Origin`
item should be added to the `Vary` header when the `Access-Control-Allow-Origin` is set to an
explicit origin value, as opposed to the `*` wildcard.

>If the server specifies a single origin (that may dynamically change based on the requesting origin
as part of a white-list) rather than the "*" wildcard, then the server should also include Origin in
the Vary response header — to indicate to clients that server responses will differ based on the
value of the Origin request header.

The existing code fails to update the `Vary` list when the server is configured to allow all
origins (`*`) and the request has a `Cookie` header (ie. credentialed). In that situation, the
`Access-Control-Allow-Origin` header will be set to the request's `Origin` value.

It appears this may have just been a simple oversight in the original implementation. This updates
the code to add `Origin` to the `Vary` header under these circumstancesIf it was intentionally
omitted, I'd be delighted to learn why.
@@ -245,12 +245,30 @@ def homepage(request):
assert response.headers["vary"] == "Origin"


def test_cors_vary_header_is_properly_set():
def test_cors_vary_header_is_properly_set_for_credentialed_request():
Copy link
Member

@euri10 euri10 Dec 16, 2020

Choose a reason for hiding this comment

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

should we add another test_cors_vary_header_is_properly_set_for_not_credentialed_request ? and have the same setup as here without the Cookie sent ?

@jcwilson jcwilson force-pushed the vary-origin-credentialed-cors branch 3 times, most recently from 5df76ea to 1673983 Compare December 17, 2020 23:21
@jcwilson

This comment has been minimized.

@JayH5 JayH5 added the cors Cross-Origin Resource Sharing label Feb 4, 2021
@jcwilson
Copy link
Contributor Author

jcwilson commented Apr 5, 2021

@euri10 This is the other trivial one dealing with CORS improvements.

I think #1113 could use some attention, too. It'd be nice to see all of these CORS updates ship together at some point.

@JayH5
Copy link
Member

JayH5 commented Apr 6, 2021

These PRs all look good, thank you. Let's get them merged. I'll need a bit more time to fully understand #1113, though.

@JayH5 JayH5 merged commit 6022126 into encode:master Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cors Cross-Origin Resource Sharing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants