From 98e19e803bb4241be6b70e43603c9dc508811a3f Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 5 Dec 2015 12:48:14 +0100 Subject: [PATCH 1/6] mpd: Deleting unkown playlist should return not found --- mopidy/mpd/protocol/stored_playlists.py | 2 ++ tests/mpd/protocol/test_stored_playlists.py | 4 ++++ 2 files changed, 6 insertions(+) diff --git a/mopidy/mpd/protocol/stored_playlists.py b/mopidy/mpd/protocol/stored_playlists.py index 647a1464..1a24608c 100644 --- a/mopidy/mpd/protocol/stored_playlists.py +++ b/mopidy/mpd/protocol/stored_playlists.py @@ -328,6 +328,8 @@ def rm(context, name): Removes the playlist ``NAME.m3u`` from the playlist directory. """ uri = context.lookup_playlist_uri_from_name(name) + if not uri: + raise exceptions.MpdNoExistError('No such playlist') context.core.playlists.delete(uri).get() diff --git a/tests/mpd/protocol/test_stored_playlists.py b/tests/mpd/protocol/test_stored_playlists.py index e212af09..36635505 100644 --- a/tests/mpd/protocol/test_stored_playlists.py +++ b/tests/mpd/protocol/test_stored_playlists.py @@ -330,6 +330,10 @@ class PlaylistsHandlerTest(protocol.BaseTestCase): self.assertInResponse('OK') self.assertIsNone(self.backend.playlists.lookup('dummy:a1').get()) + def test_rm_unknown_playlist_acks(self): + self.send_request('rm "name"') + self.assertInResponse('ACK [50@0] {rm} No such playlist') + def test_save(self): self.send_request('save "name"') From c3393d3d859e0c93fe0c98f2c2f30dccc59f76e5 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 5 Dec 2015 12:49:45 +0100 Subject: [PATCH 2/6] mpd: Refresh mapping when name is not present (Fixes #1348) --- mopidy/mpd/uri_mapper.py | 2 +- tests/mpd/protocol/test_regression.py | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/mopidy/mpd/uri_mapper.py b/mopidy/mpd/uri_mapper.py index 9e7ec2dd..bb627a47 100644 --- a/mopidy/mpd/uri_mapper.py +++ b/mopidy/mpd/uri_mapper.py @@ -71,7 +71,7 @@ class MpdUriMapper(object): """ Helper function to retrieve a playlist URI from its unique MPD name. """ - if not self._uri_from_name: + if name not in self._uri_from_name: self.refresh_playlists_mapping() return self._uri_from_name.get(name) diff --git a/tests/mpd/protocol/test_regression.py b/tests/mpd/protocol/test_regression.py index 1688d064..40a3f103 100644 --- a/tests/mpd/protocol/test_regression.py +++ b/tests/mpd/protocol/test_regression.py @@ -233,3 +233,24 @@ class IssueGH1120RegressionTest(protocol.BaseTestCase): response2 = self.send_request('lsinfo "/"') self.assertEqual(response1, response2) + + +class IssueGH1348RegressionTest(protocol.BaseTestCase): + + """ + The issue: http://github.com/mopidy/mopidy/issues/1348 + """ + + def test(self): + self.backend.library.dummy_library = [Track(uri='dummy:a')] + + # Create a dummy playlist and trigger population of mapping + self.send_request('playlistadd "testing1" "dummy:a"') + self.send_request('listplaylists') + + # Create an other playlist which isn't in the map + self.send_request('playlistadd "testing2" "dummy:a"') + self.assertEqual(['OK'], self.send_request('rm "testing2"')) + + playlists = self.backend.playlists.as_list().get() + self.assertEqual(['testing1'], [ref.name for ref in playlists]) From b21debf6eec60f146c9f6f6c7378a1be5d813528 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 5 Dec 2015 13:51:36 +0100 Subject: [PATCH 3/6] mpd: Sanity check stored playlist names --- mopidy/mpd/exceptions.py | 9 ++++++ mopidy/mpd/protocol/stored_playlists.py | 11 +++++++ tests/mpd/protocol/test_stored_playlists.py | 34 +++++++++++++++++++++ 3 files changed, 54 insertions(+) diff --git a/mopidy/mpd/exceptions.py b/mopidy/mpd/exceptions.py index b64a6cf0..65771f2a 100644 --- a/mopidy/mpd/exceptions.py +++ b/mopidy/mpd/exceptions.py @@ -84,6 +84,15 @@ class MpdSystemError(MpdAckError): error_code = MpdAckError.ACK_ERROR_SYSTEM +class MpdInvalidPlaylistName(MpdAckError): + error_code = MpdAckError.ACK_ERROR_ARG + + def __init__(self, *args, **kwargs): + super(MpdInvalidPlaylistName, self).__init__(*args, **kwargs) + self.message = ('playlist name is invalid: playlist names may not ' + 'contain slashes, newlines or carriage returns') + + class MpdNotImplemented(MpdAckError): error_code = 0 diff --git a/mopidy/mpd/protocol/stored_playlists.py b/mopidy/mpd/protocol/stored_playlists.py index 1a24608c..fd395696 100644 --- a/mopidy/mpd/protocol/stored_playlists.py +++ b/mopidy/mpd/protocol/stored_playlists.py @@ -2,6 +2,7 @@ from __future__ import absolute_import, division, unicode_literals import datetime import logging +import re import warnings from mopidy.compat import urllib @@ -10,6 +11,11 @@ from mopidy.mpd import exceptions, protocol, translator logger = logging.getLogger(__name__) +def _check_playlist_name(name): + if re.search('[/\n\r]', name): + raise exceptions.MpdInvalidPlaylistName() + + @protocol.commands.add('listplaylist') def listplaylist(context, name): """ @@ -149,6 +155,7 @@ def playlistadd(context, name, track_uri): ``NAME.m3u`` will be created if it does not exist. """ + _check_playlist_name(name) uri = context.lookup_playlist_uri_from_name(name) old_playlist = uri is not None and context.core.playlists.lookup(uri).get() if not old_playlist: @@ -219,6 +226,7 @@ def playlistclear(context, name): The playlist will be created if it does not exist. """ + _check_playlist_name(name) uri = context.lookup_playlist_uri_from_name(name) playlist = uri is not None and context.core.playlists.lookup(uri).get() if not playlist: @@ -240,6 +248,7 @@ def playlistdelete(context, name, songpos): Deletes ``SONGPOS`` from the playlist ``NAME.m3u``. """ + _check_playlist_name(name) uri = context.lookup_playlist_uri_from_name(name) playlist = uri is not None and context.core.playlists.lookup(uri).get() if not playlist: @@ -327,6 +336,7 @@ def rm(context, name): Removes the playlist ``NAME.m3u`` from the playlist directory. """ + _check_playlist_name(name) uri = context.lookup_playlist_uri_from_name(name) if not uri: raise exceptions.MpdNoExistError('No such playlist') @@ -343,6 +353,7 @@ def save(context, name): Saves the current playlist to ``NAME.m3u`` in the playlist directory. """ + _check_playlist_name(name) tracks = context.core.tracklist.get_tracks().get() uri = context.lookup_playlist_uri_from_name(name) playlist = uri is not None and context.core.playlists.lookup(uri).get() diff --git a/tests/mpd/protocol/test_stored_playlists.py b/tests/mpd/protocol/test_stored_playlists.py index 36635505..e33b1bc2 100644 --- a/tests/mpd/protocol/test_stored_playlists.py +++ b/tests/mpd/protocol/test_stored_playlists.py @@ -232,6 +232,10 @@ class PlaylistsHandlerTest(protocol.BaseTestCase): self.assertEqual(0, len(self.core.tracklist.tracks.get())) self.assertEqualResponse('ACK [50@0] {load} No such playlist') + # No invalid name check for load. + self.send_request('load "unknown/playlist"') + self.assertEqualResponse('ACK [50@0] {load} No such playlist') + def test_playlistadd(self): tracks = [ Track(uri='dummy:a'), @@ -259,6 +263,12 @@ class PlaylistsHandlerTest(protocol.BaseTestCase): self.assertInResponse('OK') self.assertIsNotNone(self.backend.playlists.lookup('dummy:name').get()) + def test_playlistadd_invalid_name_acks(self): + self.send_request('playlistadd "foo/bar" "dummy:a"') + self.assertInResponse('ACK [2@0] {playlistadd} playlist name is ' + 'invalid: playlist names may not contain ' + 'slashes, newlines or carriage returns') + def test_playlistclear(self): self.backend.playlists.set_dummy_playlists([ Playlist( @@ -276,6 +286,12 @@ class PlaylistsHandlerTest(protocol.BaseTestCase): self.assertInResponse('OK') self.assertIsNotNone(self.backend.playlists.lookup('dummy:name').get()) + def test_playlistclear_invalid_name_acks(self): + self.send_request('playlistclear "foo/bar"') + self.assertInResponse('ACK [2@0] {playlistclear} playlist name is ' + 'invalid: playlist names may not contain ' + 'slashes, newlines or carriage returns') + def test_playlistdelete(self): tracks = [ Track(uri='dummy:a'), @@ -292,6 +308,12 @@ class PlaylistsHandlerTest(protocol.BaseTestCase): self.assertEqual( 2, len(self.backend.playlists.get_items('dummy:a1').get())) + def test_playlistdelete_invalid_name_acks(self): + self.send_request('playlistdelete "foo/bar" "0"') + self.assertInResponse('ACK [2@0] {playlistdelete} playlist name is ' + 'invalid: playlist names may not contain ' + 'slashes, newlines or carriage returns') + def test_playlistmove(self): tracks = [ Track(uri='dummy:a'), @@ -334,8 +356,20 @@ class PlaylistsHandlerTest(protocol.BaseTestCase): self.send_request('rm "name"') self.assertInResponse('ACK [50@0] {rm} No such playlist') + def test_rm_invalid_name_acks(self): + self.send_request('rm "foo/bar"') + self.assertInResponse('ACK [2@0] {rm} playlist name is invalid: ' + 'playlist names may not contain slashes, ' + 'newlines or carriage returns') + def test_save(self): self.send_request('save "name"') self.assertInResponse('OK') self.assertIsNotNone(self.backend.playlists.lookup('dummy:name').get()) + + def test_save_invalid_name_acks(self): + self.send_request('save "foo/bar"') + self.assertInResponse('ACK [2@0] {save} playlist name is invalid: ' + 'playlist names may not contain slashes, ' + 'newlines or carriage returns') From 5de9495eaaa4002b7e62c82431542097f9cc013c Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 5 Dec 2015 14:01:43 +0100 Subject: [PATCH 4/6] mpd: Update playlistdelete to handle unknown names and indexes --- mopidy/mpd/protocol/stored_playlists.py | 9 ++++++--- tests/mpd/protocol/test_stored_playlists.py | 9 +++++++++ 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/mopidy/mpd/protocol/stored_playlists.py b/mopidy/mpd/protocol/stored_playlists.py index fd395696..affd1126 100644 --- a/mopidy/mpd/protocol/stored_playlists.py +++ b/mopidy/mpd/protocol/stored_playlists.py @@ -254,9 +254,12 @@ def playlistdelete(context, name, songpos): if not playlist: raise exceptions.MpdNoExistError('No such playlist') - # Convert tracks to list and remove requested - tracks = list(playlist.tracks) - tracks.pop(songpos) + try: + # Convert tracks to list and remove requested + tracks = list(playlist.tracks) + tracks.pop(songpos) + except IndexError: + raise exceptions.MpdArgError('Bad song index') # Replace tracks and save playlist playlist = playlist.replace(tracks=tracks) diff --git a/tests/mpd/protocol/test_stored_playlists.py b/tests/mpd/protocol/test_stored_playlists.py index e33b1bc2..6b568667 100644 --- a/tests/mpd/protocol/test_stored_playlists.py +++ b/tests/mpd/protocol/test_stored_playlists.py @@ -314,6 +314,15 @@ class PlaylistsHandlerTest(protocol.BaseTestCase): 'invalid: playlist names may not contain ' 'slashes, newlines or carriage returns') + def test_playlistdelete_unknown_playlist_acks(self): + self.send_request('playlistdelete "foobar" "0"') + self.assertInResponse('ACK [50@0] {playlistdelete} No such playlist') + + def test_playlistdelete_unknown_index_acks(self): + self.send_request('save "foobar"') + self.send_request('playlistdelete "foobar" "0"') + self.assertInResponse('ACK [2@0] {playlistdelete} Bad song index') + def test_playlistmove(self): tracks = [ Track(uri='dummy:a'), From 9ac1760dd1867a5f97a2c77dd45b63334bb4b957 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 5 Dec 2015 14:11:27 +0100 Subject: [PATCH 5/6] mpd: Update playlistmove to check names and indexes --- mopidy/mpd/protocol/stored_playlists.py | 15 ++++++--- tests/mpd/protocol/test_stored_playlists.py | 36 +++++++++++++++++++++ 2 files changed, 47 insertions(+), 4 deletions(-) diff --git a/mopidy/mpd/protocol/stored_playlists.py b/mopidy/mpd/protocol/stored_playlists.py index affd1126..2a096c07 100644 --- a/mopidy/mpd/protocol/stored_playlists.py +++ b/mopidy/mpd/protocol/stored_playlists.py @@ -286,6 +286,10 @@ def playlistmove(context, name, from_pos, to_pos): documentation, but just the ``SONGPOS`` to move *from*, i.e. ``playlistmove {NAME} {FROM_SONGPOS} {TO_SONGPOS}``. """ + if from_pos == to_pos: + return + + _check_playlist_name(name) uri = context.lookup_playlist_uri_from_name(name) playlist = uri is not None and context.core.playlists.lookup(uri).get() if not playlist: @@ -293,10 +297,13 @@ def playlistmove(context, name, from_pos, to_pos): if from_pos == to_pos: return # Nothing to do - # Convert tracks to list and perform move - tracks = list(playlist.tracks) - track = tracks.pop(from_pos) - tracks.insert(to_pos, track) + try: + # Convert tracks to list and perform move + tracks = list(playlist.tracks) + track = tracks.pop(from_pos) + tracks.insert(to_pos, track) + except IndexError: + raise exceptions.MpdArgError('Bad song index') # Replace tracks and save playlist playlist = playlist.replace(tracks=tracks) diff --git a/tests/mpd/protocol/test_stored_playlists.py b/tests/mpd/protocol/test_stored_playlists.py index 6b568667..da214486 100644 --- a/tests/mpd/protocol/test_stored_playlists.py +++ b/tests/mpd/protocol/test_stored_playlists.py @@ -340,6 +340,42 @@ class PlaylistsHandlerTest(protocol.BaseTestCase): "dummy:c", self.backend.playlists.get_items('dummy:a1').get()[0].uri) + def test_playlistmove_invalid_name_acks(self): + self.send_request('playlistmove "foo/bar" "0" "1"') + self.assertInResponse('ACK [2@0] {playlistmove} playlist name is ' + 'invalid: playlist names may not contain ' + 'slashes, newlines or carriage returns') + + def test_playlistmove_unknown_playlist_acks(self): + self.send_request('playlistmove "foobar" "0" "1"') + self.assertInResponse('ACK [50@0] {playlistmove} No such playlist') + + def test_playlistmove_unknown_position_acks(self): + self.send_request('save "foobar"') + self.send_request('playlistmove "foobar" "0" "1"') + self.assertInResponse('ACK [2@0] {playlistmove} Bad song index') + + def test_playlistmove_same_index_shortcircuits_everything(self): + # Bad indexes on unknown playlist: + self.send_request('playlistmove "foobar" "0" "0"') + self.assertInResponse('OK') + + self.send_request('playlistmove "foobar" "100000" "100000"') + self.assertInResponse('OK') + + # Bad indexes on known playlist: + self.send_request('save "foobar"') + + self.send_request('playlistmove "foobar" "0" "0"') + self.assertInResponse('OK') + + self.send_request('playlistmove "foobar" "10" "10"') + self.assertInResponse('OK') + + # Invalid playlist name: + self.send_request('playlistmove "foo/bar" "0" "0"') + self.assertInResponse('OK') + def test_rename(self): self.backend.playlists.set_dummy_playlists([ Playlist( From 00b52da6ab360f4a6fc1668aa0575747d6434650 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 5 Dec 2015 14:30:18 +0100 Subject: [PATCH 6/6] mpd: Make sure rename error handling is correct --- mopidy/mpd/exceptions.py | 4 ++++ mopidy/mpd/protocol/stored_playlists.py | 18 ++++++++++++--- tests/mpd/protocol/test_stored_playlists.py | 25 +++++++++++++++++++++ 3 files changed, 44 insertions(+), 3 deletions(-) diff --git a/mopidy/mpd/exceptions.py b/mopidy/mpd/exceptions.py index 65771f2a..05762683 100644 --- a/mopidy/mpd/exceptions.py +++ b/mopidy/mpd/exceptions.py @@ -80,6 +80,10 @@ class MpdNoExistError(MpdAckError): error_code = MpdAckError.ACK_ERROR_NO_EXIST +class MpdExistError(MpdAckError): + error_code = MpdAckError.ACK_ERROR_EXIST + + class MpdSystemError(MpdAckError): error_code = MpdAckError.ACK_ERROR_SYSTEM diff --git a/mopidy/mpd/protocol/stored_playlists.py b/mopidy/mpd/protocol/stored_playlists.py index 2a096c07..68ae1e9e 100644 --- a/mopidy/mpd/protocol/stored_playlists.py +++ b/mopidy/mpd/protocol/stored_playlists.py @@ -322,16 +322,28 @@ def rename(context, old_name, new_name): Renames the playlist ``NAME.m3u`` to ``NEW_NAME.m3u``. """ - uri = context.lookup_playlist_uri_from_name(old_name) - uri_scheme = urllib.parse.urlparse(uri).scheme - old_playlist = uri is not None and context.core.playlists.lookup(uri).get() + _check_playlist_name(old_name) + _check_playlist_name(new_name) + + old_uri = context.lookup_playlist_uri_from_name(old_name) + if not old_uri: + raise exceptions.MpdNoExistError('No such playlist') + + old_playlist = context.core.playlists.lookup(old_uri).get() if not old_playlist: raise exceptions.MpdNoExistError('No such playlist') + new_uri = context.lookup_playlist_uri_from_name(new_name) + if new_uri and context.core.playlists.lookup(new_uri).get(): + raise exceptions.MpdExistError('Playlist already exists') + # TODO: should we purge the mapping in an else? + # Create copy of the playlist and remove original + uri_scheme = urllib.parse.urlparse(old_uri).scheme new_playlist = context.core.playlists.create(new_name, uri_scheme).get() new_playlist = new_playlist.replace(tracks=old_playlist.tracks) saved_playlist = context.core.playlists.save(new_playlist).get() + if saved_playlist is None: raise exceptions.MpdFailedToSavePlaylist(uri_scheme) context.core.playlists.delete(old_playlist.uri).get() diff --git a/tests/mpd/protocol/test_stored_playlists.py b/tests/mpd/protocol/test_stored_playlists.py index da214486..fc3e8214 100644 --- a/tests/mpd/protocol/test_stored_playlists.py +++ b/tests/mpd/protocol/test_stored_playlists.py @@ -387,6 +387,31 @@ class PlaylistsHandlerTest(protocol.BaseTestCase): self.assertIsNotNone( self.backend.playlists.lookup('dummy:new_name').get()) + def test_rename_unknown_playlist_acks(self): + self.send_request('rename "foo" "bar"') + self.assertInResponse('ACK [50@0] {rename} No such playlist') + + def test_rename_to_existing_acks(self): + self.send_request('save "foo"') + self.send_request('save "bar"') + + self.send_request('rename "foo" "bar"') + self.assertInResponse('ACK [56@0] {rename} Playlist already exists') + + def test_rename_invalid_name_acks(self): + expected = ('ACK [2@0] {rename} playlist name is invalid: playlist ' + 'names may not contain slashes, newlines or carriage ' + 'returns') + + self.send_request('rename "foo/bar" "bar"') + self.assertInResponse(expected) + + self.send_request('rename "foo" "foo/bar"') + self.assertInResponse(expected) + + self.send_request('rename "bar/foo" "foo/bar"') + self.assertInResponse(expected) + def test_rm(self): self.backend.playlists.set_dummy_playlists([ Playlist(