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

fix(cors) set missing vary=origin #3765

Merged
merged 4 commits into from Oct 4, 2018

Conversation

Projects
None yet
3 participants
@marckhouzam
Copy link
Contributor

marckhouzam commented Sep 12, 2018

When Access-Control-Allow-Origin is not *, the cors plugins normally
sets Vary=Origin. In the case where the plugin sets
Access-Control-Allow-Credentials the Vary=Origin is missing.

When that happens, the browser is told to uses its cache even when the
origin is different and then CORS fails by rejecting the non-matching
origin.

fix(cors) set missing vary=origin
When Access-Control-Allow-Origin is not *, the cors plugins normally
sets Vary=Origin.  In the case where the plugin sets
Access-Control-Allow-Credentials the Vary=Origin is missing.

When that happens, the browser is told to uses its cache even when the
origin is different and then CORS fails by rejecting the non-matching
origin.
@thibaultcha

This comment has been minimized.

Copy link
Member

thibaultcha commented Sep 12, 2018

@marckhouzam Thank you for the patch! Would you mind adding a test for this fix as well? We would need it in order for us to merge your PR. Thank you :)

@marckhouzam

This comment has been minimized.

Copy link
Contributor Author

marckhouzam commented Sep 13, 2018

I'll give it a try.

@thibaultcha thibaultcha added the pr/wip label Sep 13, 2018

@nijikokun

This comment has been minimized.

Copy link
Member

nijikokun commented Sep 20, 2018

We should review this further before allowing it to go through as there are implications when it isn't the origin. This blog post describes it:

https://www.fastly.com/blog/caching-cors

In fact vary could be it's own plugin or within some "browser cache" plugin.

@marckhouzam

This comment has been minimized.

Copy link
Contributor Author

marckhouzam commented Sep 22, 2018

@nijikokun I read through the documentations you posted and it does look like the cache handling through the use of Vary could be improved. However, I am thinking that currently the CORS plugin has a bug in its handling of the cache when credentials=true and this PR has value in fixing that. What do you think?

With that in mind, I modified the CORS tests in a second commit. After looking at the existing tests for the cors plugin, I thought that adding a check in the different relevant tests for header Vary=Origin would cover the most ground. Since there was already a test where credentials=true, then checking for Vary=Origin in there shows that the test fails with the current master branch while it passes once my PR is applied.

I could add an is_nil() check for header Vary in all the other CORS tests if you would like.

Thanks

@marckhouzam

This comment has been minimized.

Copy link
Contributor Author

marckhouzam commented Oct 1, 2018

Any comments on this updated PR?
Thanks!

@thibaultcha

This comment has been minimized.

Copy link
Member

thibaultcha commented Oct 1, 2018

Thanks @marckhouzam, we'll have a look at it soon!

@thibaultcha
Copy link
Member

thibaultcha left a comment

LGTM. I agree with your statement @marckhouzam:

it does look like the cache handling through the use of Vary could be improved.

And furthermore:

However, I am thinking that currently the CORS plugin has a bug in its handling of the cache when credentials=true and this PR has value in fixing that.

Yes, this is entirely in the scope of this PR, and the point you previously raised can be addressed in a subsequent improvement (for which we would welcome another PR, of course!).

I could add an is_nil() check for header Vary in all the other CORS tests if you would like.

If you have time for it, that would be great! Approving it, but waiting on it before merging.

Thanks again!

@marckhouzam

This comment has been minimized.

Copy link
Contributor Author

marckhouzam commented Oct 4, 2018

Thanks for approving!

Here are the extra changes to the tests. There was a couple Vary=Origin checks missing, and then I added the is_nil() for header Vary.

Thanks!

@thibaultcha thibaultcha merged commit acdd89d into Kong:master Oct 4, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@thibaultcha

This comment has been minimized.

Copy link
Member

thibaultcha commented Oct 4, 2018

@marckhouzam Thank you!

Btw, feel free to claim your contributor T-shirt now that your first PR has been merged :)

@marckhouzam

This comment has been minimized.

Copy link
Contributor Author

marckhouzam commented Oct 4, 2018

Thanks for the quick turnaround!
It makes it a great experience to contribute to your project.

@marckhouzam marckhouzam deleted the VilledeMontreal:fix/cors-vary-origin branch Oct 4, 2018

xiejiangzhi added a commit to thecarevoice/kong that referenced this pull request Oct 17, 2018

fix(cors) set 'Vary: Origin' when ACAC is enabled
When Access-Control-Allow-Origin is not *, the cors plugin
normally sets `Vary: Origin`. In the case when the plugin sets
Access-Control-Allow-Credentials, `Vary: Origin` is missing.

When that happens, the browser is told to uses its cache even
when the origin is different and then CORS fails by rejecting the
non-matching origin.

From Kong#3765
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.