Basic CORS support #549
Basic CORS support #549
Conversation
Modified naotifications to allow token-based notifications in addition to user-based notifications. Added an anonymous session scope. Added swagger documentation for notification/stream. Increased tests for a few other cases.
Added support and tests for CORS. The CORS settings are cached so we don't have to parse them every time we need them. Refactored how settings sets a select value so that it more closely matches other settings. Added more tests for settings. Added basic security documentation.
…ep Chrome happy. That doesn't seem to be what the CORS spec says.
|
|
||
| By default, all cross-origin requests that could modify data are refused. | ||
| Different origins may be allowed via the System Configuration. For best | ||
| security is recommended that only a specific list of origins be allowed, and |
| if isOptions: | ||
| raise cherrypy.HTTPError(405) | ||
| return | ||
| cherrypy.response.headers['Access-Control-Allow-Origin'] = origin |
There was a problem hiding this comment.
Is this the right behavior? Does this always just set the Access-Control-Allow-Origin to whoever is making the request? Wouldn't that universally permit CORS? I am certain I'm missing something :)
There was a problem hiding this comment.
Are we just assuming that all browsers always use OPTIONS first to test the CORS settings?
There was a problem hiding this comment.
Any response that is an invalid CORS response will generate a 403. I didn't want to have the code for adding the Access-Control-Allow-Origin on every successful branch, so I set that header early in the function. It should do no harm to do so, as disallowed requests get the 403.
Mozilla's documentation [1] claims that preflight OPTIONS requests are mandatory, but reading the w3 specification [2], I don't actually see that.
Certain requests are always allowed, regardless of origin and other settings: GET, HEAD, and POST (but POST only if Content-Type is one of application/x-www-form-urlencoded, multipart/form-data, text/plain). The requests that are always allowed are termed simple requests. In my reading of the w3 spec, I shouldn't need to add CORS headers to them, but Chrome won't work without them.
Otherwise, we fail if the origin is not allowed and different from the base url of the request. If the request is a preflight request (OPTIONS), we give a 405 -- denying that we can do options. If the request is anything else, we give a 403.
If the request is not a simple request, we check for disallowed methods or headers, and if either of those occur, send a 403.
See tests/cases/routes.py for the grid of verification.
I don't think CORS actually adds much security in an absolute sense, since if the caller spoofs or omits the Origin header, the request won't trigger a CORS error (and, according to the w3 spec, lack of an origin shouldn't trigger an error). However, it does make it so you can't use a reputable browser to make unauthorized requests.
[1] https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS
[2] http://www.w3.org/TR/cors/
There was a problem hiding this comment.
On Wed, Dec 10, 2014 at 8:51 AM, David Manthey notifications@github.com
wrote:
I
don't think CORS actually adds much security in an absolute sense, since
if the caller spoofs or omits the Origin header, the request won't
trigger a CORS error (and, according to the w3 spec, lack of an origin
shouldn't trigger an error). However, it does make it so you can use a
reputable browser to make unauthorized requests.I always thought the same-origin policy
was meant to protect the client from malicious code on the servers it
could otherwise retrieve resources from, and thus that CORS was a way to
relax a policy meant to prevent accidental self-harm.
Is that not right?
There was a problem hiding this comment.
Zach brought up some of the issues with CORS here: #315
Good read if you haven't yet.
There was a problem hiding this comment.
@ronichoudhury correct, this is for relaxation of that policy. I was just making sure the relaxation was in accordance with the specified system settings.
|
LGTM. |
Added support and tests for CORS. The CORS settings are cached so we don't have to parse them every time we need them.
Refactored how settings sets a select value so that it more closely matches other settings. Added more tests for settings.
Added basic security documentation.
We could add additional CORS features, if they are ever needed: origin-specific permissions (currently all origins that are allowed permit the same headers and methods), max-age, expose-headers, and allow-credentials.