From dd12220e1e9a5871f6633b3fd38b5fd34aa8eb12 Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Wed, 27 Mar 2019 11:56:09 +0100 Subject: [PATCH 1/2] PICARD-1497: Mark invalid Vorbis keys as unsupported tags --- picard/formats/apev2.py | 14 ++++++++++++++ picard/formats/vorbis.py | 14 ++++++++++++-- test/formats/common.py | 3 ++- test/formats/test_apev2.py | 28 ++++++++++++++++++++++++++++ test/formats/test_vorbis.py | 27 +++++++++++++++++++++++++++ 5 files changed, 83 insertions(+), 3 deletions(-) diff --git a/picard/formats/apev2.py b/picard/formats/apev2.py index ef47556d9..2da37d9b7 100644 --- a/picard/formats/apev2.py +++ b/picard/formats/apev2.py @@ -46,6 +46,20 @@ from picard.util import ( from .mutagenext import tak +INVALID_CHARS = re.compile('[^\x20-\x7e]') + + +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). + + See http://wiki.hydrogenaud.io/index.php?title=APE_key + """ + return key and 2 <= len(key) <= 255 and INVALID_CHARS.search(key) is None + + class APEv2File(File): """Generic APEv2-based file.""" 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..c3e123776 100644 --- a/test/formats/test_apev2.py +++ b/test/formats/test_apev2.py @@ -1,3 +1,6 @@ +from test.picardtestcase import PicardTestCase + +from picard.formats import apev2 from .common import ( CommonTests, load_metadata, @@ -5,6 +8,23 @@ from .common import ( from .coverart import CommonCoverArtTests +VALID_KEYS = [ + ' valid Key}', + '{ $ome tag~}', + 'xx', + 'x' * 255, +] + + +INVALID_KEYS = [ + 'invalid\x7fkey', + 'invalid\x19key', + '', + 'x', + 'x' * 256, +] + + class MonkeysAudioTest(CommonTests.TagFormatsTestCase): testfile = 'test.ape' supports_ratings = False @@ -82,3 +102,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' From ea9ad532b249b8b8cf465f0ef0ebcbf40578f323 Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Thu, 28 Mar 2019 21:41:20 +0100 Subject: [PATCH 2/2] PICARD-1497: Mark invalid Apev2 keys as unsupported tags --- picard/formats/apev2.py | 25 +++++++++++++++---------- test/formats/test_apev2.py | 34 ++++++++++++++++++++++++++-------- 2 files changed, 41 insertions(+), 18 deletions(-) diff --git a/picard/formats/apev2.py b/picard/formats/apev2.py index 2da37d9b7..5c8db4c17 100644 --- a/picard/formats/apev2.py +++ b/picard/formats/apev2.py @@ -47,6 +47,15 @@ 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): @@ -54,10 +63,13 @@ 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 INVALID_CHARS.search(key) is None + return (key and 2 <= len(key) <= 255 + and key not in BLACKLISTED_KEYS + and INVALID_CHARS.search(key) is None) class APEv2File(File): @@ -229,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/test/formats/test_apev2.py b/test/formats/test_apev2.py index c3e123776..22beb61e8 100644 --- a/test/formats/test_apev2.py +++ b/test/formats/test_apev2.py @@ -4,6 +4,7 @@ from picard.formats import apev2 from .common import ( CommonTests, load_metadata, + TAGS, ) from .coverart import CommonCoverArtTests @@ -15,17 +16,34 @@ VALID_KEYS = [ 'x' * 255, ] - INVALID_KEYS = [ 'invalid\x7fkey', 'invalid\x19key', '', 'x', 'x' * 256, + 'ID3', + 'TAG', + 'OggS', + 'MP+', ] -class MonkeysAudioTest(CommonTests.TagFormatsTestCase): +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 = { @@ -36,7 +54,7 @@ class MonkeysAudioTest(CommonTests.TagFormatsTestCase): } -class WavPackTest(CommonTests.TagFormatsTestCase): +class WavPackTest(CommonApeTests.ApeTestCase): testfile = 'test.wv' supports_ratings = False expected_info = { @@ -46,7 +64,7 @@ class WavPackTest(CommonTests.TagFormatsTestCase): } -class MusepackSV7Test(CommonTests.TagFormatsTestCase): +class MusepackSV7Test(CommonApeTests.ApeTestCase): testfile = 'test-sv7.mpc' supports_ratings = False expected_info = { @@ -56,7 +74,7 @@ class MusepackSV7Test(CommonTests.TagFormatsTestCase): } -class MusepackSV8Test(CommonTests.TagFormatsTestCase): +class MusepackSV8Test(CommonApeTests.ApeTestCase): testfile = 'test-sv8.mpc' supports_ratings = False expected_info = { @@ -66,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 = { @@ -85,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 = {