-
Notifications
You must be signed in to change notification settings - Fork 149
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 check for cors allow credentials #320
Conversation
Travis runs of tests generates fails in 2 environments py26 & py34 are unrelated. The py27 has a test fail that I believe is a bad test given it assumes the same thing the original code did i.e. that you can use Access-Control-Allow-Credentials as a request header, and so it necessarily fails my fix. I'm researching this further now. |
apologies for that broken commit message there. What I would add is that when eliminate the incorrect use of 'Access-Control-Allow-Credentials' as a request header in that one test, then it and the previous test can not both pass since they are the same and expect different results. Eliminated the test that is incorrect based on the issue that triggered this fix. |
@@ -96,8 +96,7 @@ def ensure_origin(service, request, response=None): | |||
for o in service.cors_origins_for(method)]): | |||
request.errors.add('header', 'Origin', | |||
'%s not allowed' % origin) | |||
elif request.headers.get( | |||
'Access-Control-Allow-Credentials', False): | |||
elif service.cors_support_credentials_for(method): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is not the correct way of handling the problem: here it prevents the response to contain the correct AC-Allow-Origin response header (as the tests showcase).
This is probably due to the fact this is in an elif where the else tests that the allowed origins contain a wildcard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the test is testing for something that in fact should not be true and that's why I eliminated the test. Let me explain there on your other comment.
Okay, this makes more sense now. Thanks for waiting this long (I was on vacations). I believe we need to add tests still there and then we'll be good. |
Hope you enjoyed your vacation. Added the assertion to the fixed test. Thanks. |
fix check for cors allow credentials
Thanks! Merged. |
As per my submission of #319, I was hitting CORS issue when using credentials. In particular, I get the following error:
What I think should be happening is the server should be setting the 'Access-Control-Allow-Origin' response header to the origin provided in the credentialed request. I've replaced an incorrect check of a request header, Access-Control-Allow-Credentials, which is really a response header with a correct test on the service definition.