diff --git a/picard/formats/apev2.py b/picard/formats/apev2.py index ef47556d9..5c8db4c17 100644 --- a/picard/formats/apev2.py +++ b/picard/formats/apev2.py @@ -46,6 +46,32 @@ from picard.util import ( from .mutagenext import tak +INVALID_CHARS = re.compile('[^\x20-\x7e]') +BLACKLISTED_KEYS = ['ID3', 'TAG', 'OggS', 'MP+'] +UNSUPPORTED_TAGS = [ + 'gapless', + 'musicip_fingerprint', + 'podcast', + 'podcasturl', + 'show', + 'showsort', +] + + +def is_valid_key(key): + """ + Return true if a string is a valid APE tag key. + APE tag item keys can have a length of 2 (including) up to 255 (including) + characters in the range from 0x20 (Space) until 0x7E (Tilde). + Not allowed are the following keys: ID3, TAG, OggS and MP+. + + See http://wiki.hydrogenaud.io/index.php?title=APE_key + """ + return (key and 2 <= len(key) <= 255 + and key not in BLACKLISTED_KEYS + and INVALID_CHARS.search(key) is None) + + class APEv2File(File): """Generic APEv2-based file.""" @@ -215,15 +241,8 @@ class APEv2File(File): @classmethod def supports_tag(cls, name): - unsupported_tags = { - 'gapless', - 'musicip_fingerprint', - 'podcast', - 'podcasturl', - 'show', - 'showsort', - } - return bool(name) and name not in unsupported_tags + return (bool(name) and is_valid_key(name) + and name not in UNSUPPORTED_TAGS) class MusepackFile(APEv2File): diff --git a/picard/formats/vorbis.py b/picard/formats/vorbis.py index ca4081000..be37ee9ac 100644 --- a/picard/formats/vorbis.py +++ b/picard/formats/vorbis.py @@ -49,7 +49,7 @@ from picard.util import ( ) -INVALID_CHARS = re.compile('([^\x20-}]|=)') +INVALID_CHARS = re.compile('([^\x20-\x7d]|=)') def sanitize_key(key): @@ -60,6 +60,15 @@ def sanitize_key(key): return INVALID_CHARS.sub('', key) +def is_valid_key(key): + """ + Return true if a string is a valid Vorbis comment key. + Valid characters for Vorbis comment field names are + ASCII 0x20 through 0x7D, 0x3D ('=') excluded. + """ + return INVALID_CHARS.search(key) is None + + class VCommentFile(File): """Generic VComment-based file.""" @@ -296,7 +305,8 @@ class VCommentFile(File): @classmethod def supports_tag(cls, name): unsupported_tags = {} - return bool(name) and name not in unsupported_tags + return (bool(name) and name not in unsupported_tags + and is_valid_key(name)) class FLACFile(VCommentFile): diff --git a/test/formats/common.py b/test/formats/common.py index 1b1fa8ac6..52aa93bf5 100644 --- a/test/formats/common.py +++ b/test/formats/common.py @@ -154,6 +154,7 @@ class CommonTests: self.testfile_path = os.path.join('test', 'data', self.testfile) self.testfile_ext = os.path.splitext(self.testfile)[1] self.filename = self.copy_of_original_testfile() + self.format = ext_to_format(self.testfile_ext[1:]) def copy_of_original_testfile(self): return self.copy_file_tmp(self.testfile_path, self.testfile_ext) @@ -192,7 +193,7 @@ class CommonTests: def setup_tags(self): if self.testfile: - supports_tag = ext_to_format(self.testfile_ext[1:]).supports_tag + supports_tag = self.format.supports_tag self.unsupported_tags = {tag: val for tag, val in self.tags.items() if not supports_tag(tag)} self.remove_tags(self.unsupported_tags.keys()) diff --git a/test/formats/test_apev2.py b/test/formats/test_apev2.py index 42ea5a046..22beb61e8 100644 --- a/test/formats/test_apev2.py +++ b/test/formats/test_apev2.py @@ -1,11 +1,49 @@ +from test.picardtestcase import PicardTestCase + +from picard.formats import apev2 from .common import ( CommonTests, load_metadata, + TAGS, ) from .coverart import CommonCoverArtTests -class MonkeysAudioTest(CommonTests.TagFormatsTestCase): +VALID_KEYS = [ + ' valid Key}', + '{ $ome tag~}', + 'xx', + 'x' * 255, +] + +INVALID_KEYS = [ + 'invalid\x7fkey', + 'invalid\x19key', + '', + 'x', + 'x' * 256, + 'ID3', + 'TAG', + 'OggS', + 'MP+', +] + + +SUPPORTED_TAGS = list(set(TAGS.keys()) - set(apev2.UNSUPPORTED_TAGS)) + + +class CommonApeTests: + + class ApeTestCase(CommonTests.TagFormatsTestCase): + def test_supports_tags(self): + supports_tag = self.format.supports_tag + for key in VALID_KEYS + SUPPORTED_TAGS: + self.assertTrue(supports_tag(key), '%r should be supported' % key) + for key in INVALID_KEYS + apev2.UNSUPPORTED_TAGS: + self.assertFalse(supports_tag(key), '%r should be unsupported' % key) + + +class MonkeysAudioTest(CommonApeTests.ApeTestCase): testfile = 'test.ape' supports_ratings = False expected_info = { @@ -16,7 +54,7 @@ class MonkeysAudioTest(CommonTests.TagFormatsTestCase): } -class WavPackTest(CommonTests.TagFormatsTestCase): +class WavPackTest(CommonApeTests.ApeTestCase): testfile = 'test.wv' supports_ratings = False expected_info = { @@ -26,7 +64,7 @@ class WavPackTest(CommonTests.TagFormatsTestCase): } -class MusepackSV7Test(CommonTests.TagFormatsTestCase): +class MusepackSV7Test(CommonApeTests.ApeTestCase): testfile = 'test-sv7.mpc' supports_ratings = False expected_info = { @@ -36,7 +74,7 @@ class MusepackSV7Test(CommonTests.TagFormatsTestCase): } -class MusepackSV8Test(CommonTests.TagFormatsTestCase): +class MusepackSV8Test(CommonApeTests.ApeTestCase): testfile = 'test-sv8.mpc' supports_ratings = False expected_info = { @@ -46,12 +84,12 @@ class MusepackSV8Test(CommonTests.TagFormatsTestCase): } -class TAKTest(CommonTests.TagFormatsTestCase): +class TAKTest(CommonApeTests.ApeTestCase): testfile = 'test.tak' supports_ratings = False -class OptimFROGLosslessTest(CommonTests.TagFormatsTestCase): +class OptimFROGLosslessTest(CommonApeTests.ApeTestCase): testfile = 'test.ofr' supports_ratings = False expected_info = { @@ -65,7 +103,7 @@ class OptimFROGLosslessTest(CommonTests.TagFormatsTestCase): self.assertEqual(metadata['~format'], 'OptimFROG Lossless Audio') -class OptimFROGDUalStreamTest(CommonTests.TagFormatsTestCase): +class OptimFROGDUalStreamTest(CommonApeTests.ApeTestCase): testfile = 'test.ofs' supports_ratings = False expected_info = { @@ -82,3 +120,11 @@ class OptimFROGDUalStreamTest(CommonTests.TagFormatsTestCase): class ApeCoverArtTest(CommonCoverArtTests.CoverArtTestCase): testfile = 'test.ape' supports_types = False + + +class Apev2UtilTest(PicardTestCase): + def test_is_valid_key(self): + for key in VALID_KEYS: + self.assertTrue(apev2.is_valid_key(key), '%r is valid' % key) + for key in INVALID_KEYS: + self.assertFalse(apev2.is_valid_key(key), '%r is invalid' % key) diff --git a/test/formats/test_vorbis.py b/test/formats/test_vorbis.py index 40e6b3be3..f06003fbe 100644 --- a/test/formats/test_vorbis.py +++ b/test/formats/test_vorbis.py @@ -14,6 +14,7 @@ from .common import ( load_raw, save_and_load_metadata, skipUnlessTestfile, + TAGS, ) from .coverart import ( CommonCoverArtTests, @@ -21,6 +22,19 @@ from .coverart import ( ) +VALID_KEYS = [ + ' valid Key}', + '{ $ome tag}', +] + + +INVALID_KEYS = [ + 'invalid=key', + 'invalid\x19key', + 'invalid~key', +] + + # prevent unittest to run tests in those classes class CommonVorbisTests: @@ -33,6 +47,13 @@ class CommonVorbisTests: log.set_level(old_log_level) self.assertEqual(metadata["~rating"], "THERATING") + def test_supports_tags(self): + supports_tag = self.format.supports_tag + for key in VALID_KEYS + list(TAGS.keys()): + self.assertTrue(supports_tag(key), '%r should be supported' % key) + for key in INVALID_KEYS + ['']: + self.assertFalse(supports_tag(key), '%r should be unsupported' % key) + class FLACTest(CommonVorbisTests.VorbisTestCase): testfile = 'test.flac' @@ -86,6 +107,12 @@ class VorbisUtilTest(PicardTestCase): sanitized = vorbis.sanitize_key(' \x1f=}~') self.assertEqual(sanitized, ' }') + def test_is_valid_key(self): + for key in VALID_KEYS: + self.assertTrue(vorbis.is_valid_key(key), '%r is valid' % key) + for key in INVALID_KEYS: + self.assertFalse(vorbis.is_valid_key(key), '%r is invalid' % key) + class FlacCoverArtTest(CommonCoverArtTests.CoverArtTestCase): testfile = 'test.flac'