Merge pull request #1161 from phw/PICARD-1497-validate-tag-keys

PICARD-1497: Validate tag keys

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):

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):

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())
 

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)

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'