Skip to content

Updating handling of '*' access-control-allow-origin #94

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

Closed
wants to merge 1 commit into from
Closed

Updating handling of '*' access-control-allow-origin #94

wants to merge 1 commit into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Apr 24, 2016

Motivation:
If a CORS enabled server returns the wildcard origin '*' then the
access-control-allow-credentials should not be set. If the server does
provied a actual origin in the access-control-allow-origin header then
access-control-allow-credentials must be true.

Currently there are some inconsitencies in the test in this regard.

Modifications:
Updated the test with checks to see is check for a '*' origin and verify
that access-control-allow-credentials is not set. Also only checking
access-control-allow-credentials when there is an explicit origin.

Result:
All test should still pass.

Motivation:
If a CORS enabled server returns the wildcard origin '*' then the
access-control-allow-credentials should not be set. If the server does
provied a actual origin in the access-control-allow-origin header then
access-control-allow-credentials must be true.

Currently there are some inconsitencies in the test in this regard.

Modifications:
Updated the test with checks to see is check for a '*' origin and verify
that access-control-allow-credentials is not set. Also only checking
access-control-allow-credentials when there is an explicit origin.

Result:
All test should still pass.
@brycekahle
Copy link
Contributor

brycekahle commented Apr 30, 2016

This was recently fixed in sockjs/sockjs-node#177 and the tests actually verify correct behavior

@danbev
Copy link
Contributor Author

danbev commented May 2, 2016

@brycekahle Ah thanks for pointing that out! Will need to fix this on our server side as I got it wrong.

@danbev danbev closed this May 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants