From 10fafc0228fef99d3d269969643ad771743c8d8d Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Thu, 11 Oct 2018 01:22:40 +0100 Subject: [PATCH] http: Add config option to control CSRF protection (Fixes: #1713) Allows users to disable CSRF protection and revert to the HTTP server's previous (less secure) behaviour. Users are advised to leave this config value enabled if possible. However, if disabled this will: * Remove the requirement to set a ``Content-Type: application/json`` header for JSON-RPC POST requests. * Disable all same-origin checks, effectively ignoring the ``allowed_origins`` config since requests from any origin will be allowed. * Suppress all ``Access-Control-Allow-*`` response headers. --- docs/changelog.rst | 7 ++++++ docs/ext/http.rst | 16 +++++++++++++- mopidy/http/__init__.py | 1 + mopidy/http/ext.conf | 1 + mopidy/http/handlers.py | 38 +++++++++++++++++++++----------- tests/http/test_handlers.py | 3 ++- tests/http/test_server.py | 43 +++++++++++++++++++++++++++++++++++++ 7 files changed, 94 insertions(+), 15 deletions(-) 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):