From 6e9ed9e8a9d4734671756ceeebf2059657ea2ab5 Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Mon, 8 Oct 2018 22:26:36 +0100 Subject: [PATCH 1/5] http: allow local files to access websocket (Fixes #1711) check_origin() still ensures the Origin header is set but now only blocks when missing from the allowed list *if* a network location was extracted from the header. This prevents websocket connections originating from local files (common in Apache Cordova apps such as Mopidy-Mobile) from being blocked; these files don't really have a sensible value for Origin so the client browser sets the header to something like 'file://' or 'null'. Also added some tests for check_origin(). --- docs/changelog.rst | 8 ++++++++ mopidy/http/handlers.py | 11 ++++++++-- tests/http/test_handlers.py | 40 +++++++++++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+), 2 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index aefcd125..3eed6d02 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -5,6 +5,14 @@ Changelog This changelog is used to track all major changes to Mopidy. +v2.2.1 (UNRELEASED) +=================== + +- HTTP: Stop blocking connections where the network location part of the Origin + header is empty, such as websocket connections originating from local files. + (Fixes: :issue:`1711`, PR: :issue:`1712`) + + v2.2.0 (2018-09-30) =================== diff --git a/mopidy/http/handlers.py b/mopidy/http/handlers.py index 1a16d43b..220cba80 100644 --- a/mopidy/http/handlers.py +++ b/mopidy/http/handlers.py @@ -150,11 +150,18 @@ def set_mopidy_headers(request_handler): def check_origin(origin, request_headers, allowed_origins): if origin is None: - logger.debug('Origin was not set') + logger.warn('HTTP request denied for missing Origin header') return False allowed_origins.add(request_headers.get('Host')) parsed_origin = urllib.parse.urlparse(origin).netloc.lower() - return parsed_origin and parsed_origin in allowed_origins + # Some frameworks (e.g. Apache Cordova) use local files. Requests from + # these files don't really have a sensible Origin so the browser sets the + # header to something like 'file://' or 'null'. This results here in an + # empty parsed_origin which we choose to allow. + if parsed_origin and parsed_origin not in allowed_origins: + logger.warn('HTTP request denied for Origin "%s"', origin) + return False + return True class JsonRpcHandler(tornado.web.RequestHandler): diff --git a/tests/http/test_handlers.py b/tests/http/test_handlers.py index 23472499..5941d6b0 100644 --- a/tests/http/test_handlers.py +++ b/tests/http/test_handlers.py @@ -1,6 +1,7 @@ from __future__ import absolute_import, unicode_literals import os +import unittest import mock @@ -86,3 +87,42 @@ class WebSocketHandlerTest(tornado.testing.AsyncHTTPTestCase): for client in handlers.WebSocketHandler.clients: client.ws_connection = None handlers.WebSocketHandler.broadcast('message') + + +class CheckOriginTests(unittest.TestCase): + + def setUp(self): + self.headers = {'Host': 'localhost:6680'} + self.allowed = set() + + def test_missing_origin_blocked(self): + self.assertFalse(handlers.check_origin( + None, self.headers, self.allowed)) + + def test_empty_origin_allowed(self): + self.assertTrue(handlers.check_origin('', self.headers, self.allowed)) + + def test_chrome_file_origin_allowed(self): + self.assertTrue(handlers.check_origin( + 'file://', self.headers, self.allowed)) + + def test_firefox_null_origin_allowed(self): + self.assertTrue(handlers.check_origin( + 'null', self.headers, self.allowed)) + + def test_same_host_origin_allowed(self): + self.assertTrue(handlers.check_origin( + 'http://localhost:6680', self.headers, self.allowed)) + + def test_different_host_origin_blocked(self): + self.assertFalse(handlers.check_origin( + 'http://other:6680', self.headers, self.allowed)) + + def test_different_port_blocked(self): + self.assertFalse(handlers.check_origin( + 'http://localhost:80', self.headers, self.allowed)) + + def test_extra_origin_allowed(self): + self.allowed.add('other:6680') + self.assertTrue(handlers.check_origin( + 'http://other:6680', self.headers, self.allowed)) From 10fafc0228fef99d3d269969643ad771743c8d8d Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Thu, 11 Oct 2018 01:22:40 +0100 Subject: [PATCH 2/5] 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): From cce87edaa09f74f463af5d412942c52497d22e7f Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Wed, 3 Oct 2018 22:38:50 +0200 Subject: [PATCH 3/5] git: Ignore docs/.doctrees/ --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index c4a7825c..19be2ed2 100644 --- a/.gitignore +++ b/.gitignore @@ -15,6 +15,7 @@ cover/ coverage.xml dist/ docs/_build/ +docs/.doctrees/ mopidy.log* nosetests.xml tmp/ From c720d90b53515a99d115b13fc630fac434c28c50 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Mon, 15 Oct 2018 22:08:07 +0200 Subject: [PATCH 4/5] docs: v2.2.1 release notes --- docs/changelog.rst | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 6200a863..b6d7d1b6 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -5,12 +5,14 @@ Changelog This changelog is used to track all major changes to Mopidy. -v2.2.1 (UNRELEASED) +v2.2.1 (2018-10-15) =================== -- HTTP: Stop blocking connections where the network location part of the Origin - header is empty, such as websocket connections originating from local files. - (Fixes: :issue:`1711`, PR: :issue:`1712`) +Bug fix release. + +- HTTP: Stop blocking connections where the network location part of the + ``Origin`` 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 From 7988708cbf071579ba8af2221c0bc05f4a98ef6a Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Mon, 15 Oct 2018 22:09:12 +0200 Subject: [PATCH 5/5] Bump version to 2.2.1 --- mopidy/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mopidy/__init__.py b/mopidy/__init__.py index 391d4e54..67658e48 100644 --- a/mopidy/__init__.py +++ b/mopidy/__init__.py @@ -14,4 +14,4 @@ if not (2, 7) <= sys.version_info < (3,): warnings.filterwarnings('ignore', 'could not open display') -__version__ = '2.2.0' +__version__ = '2.2.1'