-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -228,26 +228,18 @@ def test_resp_dont_include_allow_origin(self): | |
self.assertNotIn('Access-Control-Allow-Origin', resp.headers) | ||
self.assertEqual(resp.json, 'squirels') | ||
|
||
def test_resp_allow_origin_wildcard(self): | ||
resp = self.app.options( | ||
'/cors_klass', | ||
status=200, | ||
headers={ | ||
'Origin': 'lolnet.org', | ||
'Access-Control-Request-Method': 'POST'}) | ||
self.assertEqual(resp.headers['Access-Control-Allow-Origin'], '*') | ||
|
||
def test_origin_is_not_wildcard_if_allow_credentials(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand why this test should go away. |
||
resp = self.app.options( | ||
'/cors_klass', | ||
status=200, | ||
headers={ | ||
'Origin': 'lolnet.org', | ||
'Access-Control-Request-Method': 'POST', | ||
'Access-Control-Allow-Credentials': 'true' | ||
}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two ways to support removing this test case:
Make sense? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes! I believe this fix then misses a test that exercise exactly what you described (we should check that the header is returned correctly, but not sent). |
||
self.assertEqual(resp.headers['Access-Control-Allow-Origin'], | ||
'lolnet.org') | ||
self.assertEqual(resp.headers['Access-Control-Allow-Credentials'], | ||
'true') | ||
|
||
def test_responses_include_an_allow_origin_header(self): | ||
resp = self.app.get('/squirel', headers={'Origin': 'notmyidea.org'}) | ||
|
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.