From e4ba4b3e5f5691aa7eaef83a8cb9936f32771d9c Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 18 Feb 2015 00:13:24 +0100 Subject: [PATCH 1/3] mpd: Support blacklisting MPD commands in the server. Default blacklist set to listall and listallinfo. This change has been done to avoid clients being able to call "bad" MPD commands which are often misused to try and keep a client db. Note that this change will break some MPD clients, but the blacklist can be controlled via config to allow opting out for now. --- docs/changelog.rst | 5 +++++ docs/ext/mpd.rst | 7 +++++++ mopidy/mpd/__init__.py | 1 + mopidy/mpd/dispatcher.py | 5 +++++ mopidy/mpd/exceptions.py | 9 +++++++++ mopidy/mpd/ext.conf | 1 + mopidy/utils/deprecation.py | 1 + tests/mpd/test_dispatcher.py | 17 +++++++++++------ 8 files changed, 40 insertions(+), 6 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 988a0fcd..f403b269 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -84,6 +84,11 @@ v0.20.0 (UNRELEASED) - Share a single mapping between names and URIs across all MPD sessions. (Fixes: :issue:`934`, PR: :issue:`968`) +- Add support for blacklisting MPD commands. This is used to prevent clients + from using `listall` and `listallinfo` which recursively lookup the entire + "database". If you insist on using a client that needs these commands change + :confval:`mpd/command_blacklist`. + **Audio** - Deprecated :meth:`mopidy.audio.Audio.emit_end_of_stream`. Pass a diff --git a/docs/ext/mpd.rst b/docs/ext/mpd.rst index ecfab949..4f85e88f 100644 --- a/docs/ext/mpd.rst +++ b/docs/ext/mpd.rst @@ -99,3 +99,10 @@ See :ref:`config` for general help on configuring Mopidy. ``$hostname`` and ``$port`` can be used in the name. Set to an empty string to disable Zeroconf for MPD. + +.. confval:: mpd/command_blacklist + + List of MPD commands which are disabled by the server. By default this + setting blacklists `listall` and `listallinfo`. These commands don't fit + well with many of Mopidy's backends and are better left disabled unless + you know what you are doing. diff --git a/mopidy/mpd/__init__.py b/mopidy/mpd/__init__.py index 05c83baa..b2438b07 100644 --- a/mopidy/mpd/__init__.py +++ b/mopidy/mpd/__init__.py @@ -24,6 +24,7 @@ class Extension(ext.Extension): schema['max_connections'] = config.Integer(minimum=1) schema['connection_timeout'] = config.Integer(minimum=1) schema['zeroconf'] = config.String(optional=True) + schema['command_blacklist'] = config.List(optional=True) return schema def validate_environment(self): diff --git a/mopidy/mpd/dispatcher.py b/mopidy/mpd/dispatcher.py index b1b2db77..eece86d9 100644 --- a/mopidy/mpd/dispatcher.py +++ b/mopidy/mpd/dispatcher.py @@ -163,6 +163,11 @@ class MpdDispatcher(object): def _call_handler(self, request): tokens = tokenize.split(request) + # TODO: check that blacklist items are valid commands? + blacklist = self.config['mpd'].get('command_blacklist', []) + if tokens and tokens[0] in blacklist: + logger.warning('Client sent us blacklisted command: %s', tokens[0]) + raise exceptions.MpdDisabled(command=tokens[0]) try: return protocol.commands.call(tokens, context=self.context) except exceptions.MpdAckError as exc: diff --git a/mopidy/mpd/exceptions.py b/mopidy/mpd/exceptions.py index e7ab0068..f62b61da 100644 --- a/mopidy/mpd/exceptions.py +++ b/mopidy/mpd/exceptions.py @@ -87,3 +87,12 @@ class MpdNotImplemented(MpdAckError): def __init__(self, *args, **kwargs): super(MpdNotImplemented, self).__init__(*args, **kwargs) self.message = 'Not implemented' + + +class MpdDisabled(MpdAckError): + error_code = 0 + + def __init__(self, *args, **kwargs): + super(MpdDisabled, self).__init__(*args, **kwargs) + assert self.command is not None, 'command must be given explicitly' + self.message = '"%s" has been disabled in the server' % self.command diff --git a/mopidy/mpd/ext.conf b/mopidy/mpd/ext.conf index c62c37ef..fe9a0494 100644 --- a/mopidy/mpd/ext.conf +++ b/mopidy/mpd/ext.conf @@ -6,3 +6,4 @@ password = max_connections = 20 connection_timeout = 60 zeroconf = Mopidy MPD server on $hostname +command_blacklist = listall,listallinfo diff --git a/mopidy/utils/deprecation.py b/mopidy/utils/deprecation.py index 1b744702..06b0fc7c 100644 --- a/mopidy/utils/deprecation.py +++ b/mopidy/utils/deprecation.py @@ -5,6 +5,7 @@ import warnings def _is_pykka_proxy_creation(): + return False stack = inspect.stack() try: calling_frame = stack[3] diff --git a/tests/mpd/test_dispatcher.py b/tests/mpd/test_dispatcher.py index 63981668..d6b11e43 100644 --- a/tests/mpd/test_dispatcher.py +++ b/tests/mpd/test_dispatcher.py @@ -16,6 +16,7 @@ class MpdDispatcherTest(unittest.TestCase): config = { 'mpd': { 'password': None, + 'command_blacklist': ['disabled'], } } self.backend = dummy_backend.create_proxy() @@ -26,14 +27,18 @@ class MpdDispatcherTest(unittest.TestCase): pykka.ActorRegistry.stop_all() def test_call_handler_for_unknown_command_raises_exception(self): - try: + with self.assertRaises(MpdAckError) as cm: self.dispatcher._call_handler('an_unknown_command with args') - self.fail('Should raise exception') - except MpdAckError as e: - self.assertEqual( - e.get_mpd_ack(), - 'ACK [5@0] {} unknown command "an_unknown_command"') + + self.assertEqual( + cm.exception.get_mpd_ack(), + 'ACK [5@0] {} unknown command "an_unknown_command"') def test_handling_unknown_request_yields_error(self): result = self.dispatcher.handle_request('an unhandled request') self.assertEqual(result[0], 'ACK [5@0] {} unknown command "an"') + + def test_handling_blacklisted_command(self): + result = self.dispatcher.handle_request('disabled') + self.assertEqual(result[0], 'ACK [0@0] {disabled} "disabled" has been ' + 'disabled in the server') From 52814715b453c1845b71bb52ac9bebcf18f2146e Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 18 Feb 2015 20:57:22 +0100 Subject: [PATCH 2/3] mpd: Fix review comments for commands blacklist --- docs/changelog.rst | 2 +- docs/ext/mpd.rst | 4 ++-- mopidy/mpd/exceptions.py | 1 + mopidy/utils/deprecation.py | 1 - 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index f403b269..e1082b09 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -85,7 +85,7 @@ v0.20.0 (UNRELEASED) :issue:`934`, PR: :issue:`968`) - Add support for blacklisting MPD commands. This is used to prevent clients - from using `listall` and `listallinfo` which recursively lookup the entire + from using ``listall`` and ``listallinfo`` which recursively lookup the entire "database". If you insist on using a client that needs these commands change :confval:`mpd/command_blacklist`. diff --git a/docs/ext/mpd.rst b/docs/ext/mpd.rst index 4f85e88f..b02226a2 100644 --- a/docs/ext/mpd.rst +++ b/docs/ext/mpd.rst @@ -103,6 +103,6 @@ See :ref:`config` for general help on configuring Mopidy. .. confval:: mpd/command_blacklist List of MPD commands which are disabled by the server. By default this - setting blacklists `listall` and `listallinfo`. These commands don't fit - well with many of Mopidy's backends and are better left disabled unless + setting blacklists ``listall`` and ``listallinfo``. These commands don't + fit well with many of Mopidy's backends and are better left disabled unless you know what you are doing. diff --git a/mopidy/mpd/exceptions.py b/mopidy/mpd/exceptions.py index f62b61da..62e16ec3 100644 --- a/mopidy/mpd/exceptions.py +++ b/mopidy/mpd/exceptions.py @@ -90,6 +90,7 @@ class MpdNotImplemented(MpdAckError): class MpdDisabled(MpdAckError): + # NOTE: this is a custom error for mopidy that does not exists in MPD. error_code = 0 def __init__(self, *args, **kwargs): diff --git a/mopidy/utils/deprecation.py b/mopidy/utils/deprecation.py index 06b0fc7c..1b744702 100644 --- a/mopidy/utils/deprecation.py +++ b/mopidy/utils/deprecation.py @@ -5,7 +5,6 @@ import warnings def _is_pykka_proxy_creation(): - return False stack = inspect.stack() try: calling_frame = stack[3] From 19b7daed325980cdfbd5e2cbf3a0b4946a69081c Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 18 Feb 2015 21:14:24 +0100 Subject: [PATCH 3/3] mpd: Fix typos in previous commit. --- mopidy/mpd/exceptions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mopidy/mpd/exceptions.py b/mopidy/mpd/exceptions.py index 62e16ec3..6fc925a3 100644 --- a/mopidy/mpd/exceptions.py +++ b/mopidy/mpd/exceptions.py @@ -90,7 +90,7 @@ class MpdNotImplemented(MpdAckError): class MpdDisabled(MpdAckError): - # NOTE: this is a custom error for mopidy that does not exists in MPD. + # NOTE: This is a custom error for Mopidy that does not exist in MPD. error_code = 0 def __init__(self, *args, **kwargs):