-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Support for CORS #1237
Support for CORS #1237
Conversation
Hey, thanks! I was about to merge this, but I realized that we should perhaps worry a little bit about security. With "*" CORS enabled, I believe any web site I go to can get access to my music library if Maybe we should just enable it for 127.0.0.1 or something? Or ask the user for allowed origins on the command line? |
In this case enabling CORS does not reduce server security or allow any more access than what was previously available. CORS doesn't change server-side access, it's a check the browser does. When the browser runs Javascript that does an XMLHTTPRequest it will look if that header is returned from the server if the script was served from a different source than the request destination. And if it's not valid, it will stop the request. This is all done from the browser, to protect the end-user from malicious scripts that could impersonate them. To limit access to the library there is another option, the 'host' configuration option that's already in the web plugin. The default setting binds to all interfaces, which already gives access to beets to anyone with network access. Even with CORS limiting the domain, if your firewall and networking allows it, they can connect to port 8337 and have access to the web gui and your library. If you want to limit access to only your localhost, then setting the 'host' option to '127.0.0.1' will limit access to only your local browser. I'm not saying that making CORS a configurable option is a bad idea, but it's probably going to be of limited use. If security is a concern than adding authentication is probably a better option. We have a couple of different scenarios: 1. beetsweb runs on server A, javascript client hosted on server A In this scenario, no CORS is requried. The browser sees that the javascript is hosted on the same server to which the Javascript is doing requests and doesn't validate the Origin header. 2. beetsweb runs on server A, javascript client hosted on known server B For this, CORS is required. The browser sees it's trying to do a request to another server and validates the Origin header. If the header doesn't match, the browser will reject the request. 3. beetsweb runs on server A, javascript client hosted on malicious server C In this case, let's imagine someone is hosting a javascript client player on a server that you do not wish to have access. You've set the Origin header to only allow server B. Someone visits this webpage, the client loads in his browser and tries to connect to your server. The browser checks the Origin header and sees it doesn't match and the browser will deny the request. Unfortunately this scenario is quite easy to circumvent. The person running server C just needs to run a proxy on server C, so that the server connects directly to the beetsweb server. Then, when the browser run the javascript, it connects directly to server C and never looks at the Origin header because the server is the same. You can also circumvent this on the browser, by disabling or bypassing the browser security. For example, here is a Chrome plugin that injects the Allow-Control-Allow-Origin: * into the request header for any page you visit: https://chrome.google.com/webstore/detail/allow-control-allow-origi/nlfbmbojpeacfghkpbjhddihlkkiljbi?hl=en |
Cool; thanks for the background! Perhaps defaulting to the loopback interface only (i.e., 127.0.0.1 instead of 0.0.0.0) is a good idea independently. To clarify, what I'm worried about is your third scenario above. I don't think the proxy attack (i.e., connecting directly from the web server) is as worrisome as abusing CORS, since the beets server can still be protected by a firewall—whereas a (I'm also less worried about users who deliberately circumvent the security policies.) Anyway, I totally agree that CORS is not a replacement for real security. We should add authentication, and then all these problems go away. But at the moment, we don't have any real security 😃, so this kind of leakage is indeed worrisome. I'm going to keep looking into an easy solution—perhaps just a single command line flag will do for now. |
Personally I would prefer the cors setting to be an option in the configuration file, that way I can set it once and forget about it :) Would you mind if I have a look at how to do that? Would be a good exercise for me too. I propose something like this in the config file. Default config values (I could change the default host to 127.0.0.1 while I'm at it). The default would be to not send the CORS header at all:
And if you want to use cors, you specify the domain in the config file:
|
Making the decorator optional based on a configuration setting is a bit tricky because those functions are defined before the BeetsPlugin hook is called. Instead I opted for the following new default configuration. This will only listen on localhost and only allow CORS to scripts also served from localhost.
And if you wish to enable access to everyone you would change it to:
|
Apologies for the long message. I did a few tests and wanted to record the results. Test setup: Host: beets - Server running beets web plugin With the default configuration (config file does not contain any web settings), the following response headers are returned to my browser:
Trying to run the Javascript that is hosted on 192.168.2.80 from my desktop browser, I get the following error using Chrome's developer console:
Which is correct, because the server is only listening on localhost and since my browser is on another PC, it can't connect. I then changed my web plugin config to the following, to allow access to all hosts.
This time the error in the Chrome browser console is:
Which is also expected. I can now connect because beets is listening on all interfaces, but the Cross-Origin header does not match the domain where the Javascript originated so the browser is blocking it. Then I changed my configuration file to the following to specifically allow the web server where the Javascript is hosted:
The headers returned:
Result was that the Javascript worked correctly and could access the server. I did a final test, changing the configuration to allow all origins:
Headers returned:
And the Javascript could still access the beets server. |
Great! Thank you for doing the very thorough testing. You are absolutely right that a config option is better. Not sure what I was thinking suggesting a command-line flag. 😳 I'd love to find a way to do this without using a global variable too. I think the best way to do this would be to refactor the decorator to be WSGI middleware that we apply to the entire application at once, but perhaps there are simpler ways too. Any interest in taking a crack at that, or shall I? |
I'm not too familiar with Flask (I have more experience with Django), but I'm willing to take a stab at it and learn something in the process :) |
Okay, great! Fortunately, you should mostly only need to speak WSGI for this, which is a nice universal skill. 😃 |
Note that flask already has a cors extension: https://pypi.python.org/pypi/Flask-Cors/. It's pretty simple to use. May be worth looking into that instead of writing new code or dealing with raw wsgi. |
Thanks asayler, it does look simple to use. I did manage a proof of concept to add a header, but it still needs to be changed to support various methods and add additional headers. It might be simpler to use the cors extension. @sampsyo , what would be the correct way to package that module with beets? Just for reference, here is a bit of code to add a header to every request:
|
Would I add it to setup.py, like this?
|
Ok, assuming the above post is correct, I've made the changes to use flask-cors. I changed the configuration options a bit, the defaults are now:
It defaults to CORS disabled, which means it will run even if you don't have the extension installed (The extension is only imported if the config option cors is set to yes). I decided to leave the default origin set to "", since by default CORS is disabled now and the most common use case is to leave it set to "". If someone wishes to enable it, he can also change the origin if he wishes. |
Great! This makes it easy. It seems a little silly to depend on a third-party module for something so simple, but this is pretty benign. Two more tiny steps and we're ready to merge:
Thanks for sticking with this! |
# Utilities. | ||
|
||
|
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.
These two whitespace changes are unnecessary.
Yeah, I thought so too at first, but after reading what the module does it's probably not a bad thing. Right now beets web is not using it, but if at a later stage you want to add a POST method, perhaps to update something, then the CORS implementation also needs to support browsers pre-flighting OPTION requests, and this module supports that. For your second point, I don't think leaving it empty makes sense to the CORS module - you should either specify a wildcard or a domain or not send the header. So if I understand you correctly, we can use that fact to disable cors and only have one config option? This would be the new defaults:
Which means CORS is completely disabled and the web plugin can run even without the flask-cors module installed. If it's set to a value, then the module is imported and configured to use the value as the origin. |
Exactly! That looks perfect. And thanks for the clarity on the preflighting handling. Great point. |
Awesome. Thank you for doing the docs too. Merging now. |
Adds support for CORS (Cross Origin Resource Sharing), to allow clients hosted on other web servers to access the API.
Without CORS JavaScript gives the following error:
This pull request is for Issue #1236