diff --git a/docs/changelog.rst b/docs/changelog.rst index 3eed6d02..6200a863 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -12,6 +12,13 @@ v2.2.1 (UNRELEASED) header is empty, such as websocket connections originating from local files. (Fixes: :issue:`1711`, PR: :issue:`1712`) +- HTTP: Add new config value :confval:`http/csrf_protection` which enables all + CSRF protections introduced in Mopidy 2.2.0. It is enabled by default and + should only be disabled by those users who are unable to set a + ``Content-Type: application/json`` request header or cannot utilise the + :confval:`http/allowed_origins` config value. (Fixes: :issue:`1713`, PR: + :issue:`1714`) + v2.2.0 (2018-09-30) =================== diff --git a/docs/ext/http.rst b/docs/ext/http.rst index 4b521b97..cea7a252 100644 --- a/docs/ext/http.rst +++ b/docs/ext/http.rst @@ -102,7 +102,7 @@ See :ref:`config` for general help on configuring Mopidy. .. confval:: http/allowed_origins A list of domains allowed to perform Cross-Origin Resource Sharing (CORS) - requests. This applies to both JSON-RPC and Websocket requests. Values + requests. This applies to both JSON-RPC and WebSocket requests. Values should be in the format ``hostname:port`` and separated by either a comma or newline. @@ -110,3 +110,17 @@ See :ref:`config` for general help on configuring Mopidy. allowed and so you don't need an entry for those. However, if your requests originate from a different web server, you will need to add an entry for that server in this list. + +.. confval:: http/csrf_protection + + Enable the HTTP server's protection against Cross-Site Request Forgery + (CSRF) from both JSON-RPC and WebSocket requests. + + Disabling this will remove the requirement to set a ``Content-Type: application/json`` + header for JSON-RPC POST requests. It will also disable all same-origin + checks, effectively ignoring the :confval:`http/allowed_origins` config + since requests from any origin will be allowed. Lastly, all + ``Access-Control-Allow-*`` response headers will be suppressed. + + This config should only be disabled if you understand the security implications + and require the HTTP server's old behaviour. diff --git a/mopidy/http/__init__.py b/mopidy/http/__init__.py index 4f6239b9..3d39f200 100644 --- a/mopidy/http/__init__.py +++ b/mopidy/http/__init__.py @@ -26,6 +26,7 @@ class Extension(ext.Extension): schema['static_dir'] = config_lib.Path(optional=True) schema['zeroconf'] = config_lib.String(optional=True) schema['allowed_origins'] = config_lib.List(optional=True) + schema['csrf_protection'] = config_lib.Boolean(optional=True) return schema def validate_environment(self): diff --git a/mopidy/http/ext.conf b/mopidy/http/ext.conf index 5750e855..dbbcc4fd 100644 --- a/mopidy/http/ext.conf +++ b/mopidy/http/ext.conf @@ -5,3 +5,4 @@ port = 6680 static_dir = zeroconf = Mopidy HTTP server on $hostname allowed_origins = +csrf_protection = true diff --git a/mopidy/http/handlers.py b/mopidy/http/handlers.py index 220cba80..654fad05 100644 --- a/mopidy/http/handlers.py +++ b/mopidy/http/handlers.py @@ -20,6 +20,9 @@ logger = logging.getLogger(__name__) def make_mopidy_app_factory(apps, statics): def mopidy_app_factory(config, core): + if not config['http']['csrf_protection']: + logger.warn( + 'HTTP Cross-Site Request Forgery protection is disabled') allowed_origins = { x.lower() for x in config['http']['allowed_origins'] if x } @@ -27,10 +30,12 @@ def make_mopidy_app_factory(apps, statics): (r'/ws/?', WebSocketHandler, { 'core': core, 'allowed_origins': allowed_origins, + 'csrf_protection': config['http']['csrf_protection'], }), (r'/rpc', JsonRpcHandler, { 'core': core, 'allowed_origins': allowed_origins, + 'csrf_protection': config['http']['csrf_protection'], }), (r'/(.+)', StaticFileHandler, { 'path': os.path.join(os.path.dirname(__file__), 'data'), @@ -102,9 +107,10 @@ class WebSocketHandler(tornado.websocket.WebSocketHandler): # One callback per client to keep time we hold up the loop short loop.add_callback(functools.partial(_send_broadcast, client, msg)) - def initialize(self, core, allowed_origins): + def initialize(self, core, allowed_origins, csrf_protection): self.jsonrpc = make_jsonrpc_wrapper(core) self.allowed_origins = allowed_origins + self.csrf_protection = csrf_protection def open(self): self.set_nodelay(True) @@ -139,6 +145,8 @@ class WebSocketHandler(tornado.websocket.WebSocketHandler): self.close() def check_origin(self, origin): + if not self.csrf_protection: + return True return check_origin(origin, self.request.headers, self.allowed_origins) @@ -166,19 +174,21 @@ def check_origin(origin, request_headers, allowed_origins): class JsonRpcHandler(tornado.web.RequestHandler): - def initialize(self, core, allowed_origins): + def initialize(self, core, allowed_origins, csrf_protection): self.jsonrpc = make_jsonrpc_wrapper(core) self.allowed_origins = allowed_origins + self.csrf_protection = csrf_protection def head(self): self.set_extra_headers() self.finish() def post(self): - content_type = self.request.headers.get('Content-Type', '') - if content_type != 'application/json': - self.set_status(415, 'Content-Type must be application/json') - return + if self.csrf_protection: + content_type = self.request.headers.get('Content-Type', '') + if content_type != 'application/json': + self.set_status(415, 'Content-Type must be application/json') + return data = self.request.body if not data: @@ -205,14 +215,16 @@ class JsonRpcHandler(tornado.web.RequestHandler): self.set_header('Content-Type', 'application/json; utf-8') def options(self): - origin = self.request.headers.get('Origin') - if not check_origin( - origin, self.request.headers, self.allowed_origins): - self.set_status(403, 'Access denied for origin %s' % origin) - return + if self.csrf_protection: + origin = self.request.headers.get('Origin') + if not check_origin(origin, self.request.headers, + self.allowed_origins): + self.set_status(403, 'Access denied for origin %s' % origin) + return + + self.set_header('Access-Control-Allow-Origin', '%s' % origin) + self.set_header('Access-Control-Allow-Headers', 'Content-Type') - self.set_header('Access-Control-Allow-Origin', '%s' % origin) - self.set_header('Access-Control-Allow-Headers', 'Content-Type') self.set_status(204) self.finish() diff --git a/tests/http/test_handlers.py b/tests/http/test_handlers.py index 5941d6b0..9445e3bd 100644 --- a/tests/http/test_handlers.py +++ b/tests/http/test_handlers.py @@ -48,7 +48,8 @@ class WebSocketHandlerTest(tornado.testing.AsyncHTTPTestCase): self.core = mock.Mock() return tornado.web.Application([ (r'/ws/?', handlers.WebSocketHandler, { - 'core': self.core, 'allowed_origins': [] + 'core': self.core, 'allowed_origins': [], + 'csrf_protection': True }) ]) diff --git a/tests/http/test_server.py b/tests/http/test_server.py index aa721f29..7b2a8595 100644 --- a/tests/http/test_server.py +++ b/tests/http/test_server.py @@ -21,6 +21,7 @@ class HttpServerTest(tornado.testing.AsyncHTTPTestCase): 'static_dir': None, 'zeroconf': '', 'allowed_origins': [], + 'csrf_protection': True, } } @@ -205,6 +206,48 @@ class MopidyRPCHandlerTest(HttpServerTest): response.headers['Access-Control-Allow-Headers'], 'Content-Type') +class MopidyRPCHandlerNoCSRFProtectionTest(HttpServerTest): + + def get_config(self): + config = super(MopidyRPCHandlerNoCSRFProtectionTest, self).get_config() + config['http']['csrf_protection'] = False + return config + + def get_cmd(self): + return tornado.escape.json_encode({ + 'method': 'core.get_version', + 'params': [], + 'jsonrpc': '2.0', + 'id': 1, + }) + + def test_should_ignore_incorrect_content_type(self): + response = self.fetch( + '/mopidy/rpc', method='POST', body=self.get_cmd(), + headers={'Content-Type': 'text/plain'}) + + self.assertEqual(response.code, 200) + + def test_should_ignore_missing_content_type(self): + response = self.fetch( + '/mopidy/rpc', method='POST', body=self.get_cmd(), headers={}) + + self.assertEqual(response.code, 200) + + def test_different_origin_returns_allowed(self): + response = self.fetch('/mopidy/rpc', method='OPTIONS', headers={ + 'Host': 'me:6680', 'Origin': 'http://evil:666'}) + + self.assertEqual(response.code, 204) + + def test_should_not_return_cors_headers(self): + response = self.fetch('/mopidy/rpc', method='OPTIONS', headers={ + 'Host': 'me:6680', 'Origin': 'http://me:6680'}) + + self.assertNotIn('Access-Control-Allow-Origin', response.headers) + self.assertNotIn('Access-Control-Allow-Headers', response.headers) + + class HttpServerWithStaticFilesTest(tornado.testing.AsyncHTTPTestCase): def get_app(self):