Add option to force CORS headers in responses #562
Conversation
…when services origins is *
cornice/cors.py
Outdated
| if any([o == "*" for o in service.cors_origins_for(method)]): | ||
| response.headers['Access-Control-Allow-Origin'] = '*' |
There was a problem hiding this comment.
I'm considering if this test branch needs to be higher. It seems to me that you shouldn't allow both * and a hostname origin, but if you do, the * should be preferred to the value of the Origin: request.
I'm installing the project to better understand the settings.
There was a problem hiding this comment.
I updated the test locally to check the value of the Access-Control-Allow-Origin, and added a second test that set the request header Origin: notmyidea.org. I then tried some variants for cors_origins on the bacon Service. Here are the values of the Access-Control-Allow-Origin header:
cors_origins |
No Origin |
Origin: notmyidea.org |
|---|---|---|
('*',) |
* |
* |
('*', 'notmyidea.org') |
* |
* |
('notmyidea.org',) |
No header, but response | notmyidea.org |
So, my proposed corner case is not valid, and the docs appear to be correct.
However, it seems always_cors allows the Origin check to be skipped, which seems like it could be a problem for some configurations. Maybe when always_cors is set but * is not in cors_origins, the code should fall back to checking the origin?
There was a problem hiding this comment.
Good catch! I added some tests and changed the shape of the patch, which also makes the code paths clearer IMO :)
Thanks for being so thorough!
There was a problem hiding this comment.
I'm getting the same results as before:
cors_origins |
No Origin |
Origin: notmyidea.org |
|---|---|---|
('*',) |
* |
* |
('*', 'notmyidea.org') |
* |
* |
('notmyidea.org',) |
No header, but response | notmyidea.org |
However, with the newly refactored code, it is clearer that the the case of cors_origins=('notmyidea.org',) and no Origin header is the same in the original code, and this fix is now targeted at the desired case (returning Access-Control-Allow-Origin: * when there is no Origin).
Thanks @leplatrem!
No description provided.