ID3v2: Disallow 4 character TXXX descriptions as ItemKey

CHANGELOG.md

@@ -49,6 +49,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   - Renamed `Popularimeter` -> `PopularimeterFrame`
   - Renamed `SynchronizedText` -> `SynchronizedTextFrame`
 
+### Fixed
+- **ID3v2**: Disallow 4 character TXXX/WXXX frame descriptions from being converted to `ItemKey` ([issue](https://github.com/Serial-ATA/lofty-rs/issues/309)) ([PR](https://github.com/Serial-ATA/lofty-rs/pull/394))
+
 ## [0.19.2] - 2024-04-26
 
 ### Added

lofty/src/id3/v2/frame/conversion.rs

@@ -6,10 +6,9 @@ use crate::id3::v2::tag::{
 };
 use crate::id3::v2::{
 	ExtendedTextFrame, ExtendedUrlFrame, Frame, FrameFlags, FrameId, PopularimeterFrame,
-	UniqueFileIdentifierFrame, UnsynchronizedTextFrame,
+	UniqueFileIdentifierFrame,
 };
 use crate::macros::err;
-use crate::tag::items::UNKNOWN_LANGUAGE;
 use crate::tag::{ItemKey, ItemValue, TagItem, TagType};
 use crate::TextEncoding;
 
@@ -32,73 +31,41 @@ fn frame_from_unknown_item(id: FrameId<'_>, item_value: ItemValue) -> Result<Fra
 impl From<TagItem> for Option<Frame<'static>> {
 	fn from(input: TagItem) -> Self {
 		let value;
-		match input.key().try_into().map(FrameId::into_owned) {
-			Ok(id) => {
-				match (&id, input.item_value) {
-					(FrameId::Valid(ref s), ItemValue::Text(text)) if s == "COMM" => {
-						value = new_comment_frame(text);
-					},
-					(FrameId::Valid(ref s), ItemValue::Text(text)) if s == "USLT" => {
-						value = Frame::UnsynchronizedText(UnsynchronizedTextFrame::new(
-							TextEncoding::UTF8,
-							UNKNOWN_LANGUAGE,
-							EMPTY_CONTENT_DESCRIPTOR,
-							text,
-						));
-					},
-					(FrameId::Valid(ref s), ItemValue::Locator(text) | ItemValue::Text(text))
-						if s == "WXXX" =>
-					{
-						value = Frame::UserUrl(ExtendedUrlFrame::new(
-							TextEncoding::UTF8,
-							EMPTY_CONTENT_DESCRIPTOR,
-							text,
-						));
-					},
-					(FrameId::Valid(ref s), ItemValue::Text(text)) if s == "TXXX" => {
-						value = new_user_text_frame(EMPTY_CONTENT_DESCRIPTOR, text);
-					},
-					(FrameId::Valid(ref s), ItemValue::Binary(text)) if s == "POPM" => {
-						value = Frame::Popularimeter(
-							PopularimeterFrame::parse(&mut &text[..], FrameFlags::default())
-								.ok()?,
-						);
-					},
-					(_, item_value) => value = frame_from_unknown_item(id, item_value).ok()?,
-				};
-			},
-			Err(_) => match input.item_key.map_key(TagType::Id3v2, true) {
-				Some(desc) => match input.item_value {
-					ItemValue::Text(text) => {
-						value = Frame::UserText(ExtendedTextFrame::new(
-							TextEncoding::UTF8,
-							String::from(desc),
-							text,
-						))
-					},
-					ItemValue::Locator(locator) => {
-						value = Frame::UserUrl(ExtendedUrlFrame::new(
-							TextEncoding::UTF8,
-							String::from(desc),
-							locator,
-						))
-					},
-					ItemValue::Binary(_) => return None,
+		if let Ok(id) = input.key().try_into().map(FrameId::into_owned) {
+			return frame_from_unknown_item(id, input.item_value).ok();
+		}
+
+		match input.item_key.map_key(TagType::Id3v2, true) {
+			Some(desc) => match input.item_value {
+				ItemValue::Text(text) => {
+					value = Frame::UserText(ExtendedTextFrame::new(
+						TextEncoding::UTF8,
+						String::from(desc),
+						text,
+					))
 				},
-				None => match (input.item_key, input.item_value) {
-					(ItemKey::MusicBrainzRecordingId, ItemValue::Text(recording_id)) => {
-						if !recording_id.is_ascii() {
-							return None;
-						}
-						let frame = UniqueFileIdentifierFrame::new(
-							MUSICBRAINZ_UFID_OWNER.to_owned(),
-							recording_id.into_bytes(),
-						);
-						value = Frame::UniqueFileIdentifier(frame);
-					},
-					_ => {
+				ItemValue::Locator(locator) => {
+					value = Frame::UserUrl(ExtendedUrlFrame::new(
+						TextEncoding::UTF8,
+						String::from(desc),
+						locator,
+					))
+				},
+				ItemValue::Binary(_) => return None,
+			},
+			None => match (input.item_key, input.item_value) {
+				(ItemKey::MusicBrainzRecordingId, ItemValue::Text(recording_id)) => {
+					if !recording_id.is_ascii() {
 						return None;
-					},
+					}
+					let frame = UniqueFileIdentifierFrame::new(
+						MUSICBRAINZ_UFID_OWNER.to_owned(),
+						recording_id.into_bytes(),
+					);
+					value = Frame::UniqueFileIdentifier(frame);
+				},
+				_ => {
+					return None;
 				},
 			},
 		}

lofty/src/id3/v2/tag.rs

@@ -1081,12 +1081,16 @@ fn handle_tag_split(tag: &mut Tag, frame: &mut Frame<'_>) -> bool {
 			!key_value_pairs.is_empty() // Frame is consumed if we consumed all items
 		},
 
+		// TODO: HACK!! We are specifically disallowing descriptions with a length of 4.
+		//       This is due to use storing 4 character IDs as Frame::Text on tag merge.
+		//       Maybe ItemKey could use a "TXXX:" prefix eventually, so we would store
+		//       "TXXX:MusicBrainz Album Id" instead of "MusicBrainz Album Id".
 		// Store TXXX/WXXX frames by their descriptions, rather than their IDs
 		Frame::UserText(ExtendedTextFrame {
 			ref description,
 			ref content,
 			..
-		}) if !description.is_empty() => {
+		}) if !description.is_empty() && description.len() != 4 => {
 			let item_key = ItemKey::from_key(TagType::Id3v2, description);
 			for c in content.split(V4_MULTI_VALUE_SEPARATOR) {
 				tag.items.push(TagItem::new(
@@ -1101,7 +1105,7 @@ fn handle_tag_split(tag: &mut Tag, frame: &mut Frame<'_>) -> bool {
 			ref description,
 			ref content,
 			..
-		}) if !description.is_empty() => {
+		}) if !description.is_empty() && description.len() != 4 => {
 			let item_key = ItemKey::from_key(TagType::Id3v2, description);
 			for c in content.split(V4_MULTI_VALUE_SEPARATOR) {
 				tag.items.push(TagItem::new(

lofty/src/id3/v2/tag/tests.rs

@@ -108,37 +108,6 @@ fn id3v2_to_tag() {
 	crate::tag::utils::test_utils::verify_tag(&tag, true, true);
 }
 
-#[test]
-fn tag_to_id3v2_popm() {
-	let mut tag = Tag::new(TagType::Id3v2);
-	tag.insert(TagItem::new(
-		ItemKey::Popularimeter,
-		ItemValue::Binary(vec![
-			b'f', b'o', b'o', b'@', b'b', b'a', b'r', b'.', b'c', b'o', b'm', 0, 196, 0, 0, 255,
-			255,
-		]),
-	));
-
-	let expected = PopularimeterFrame::new(String::from("[email protected]"), 196, 65535);
-
-	let converted_tag: Id3v2Tag = tag.into();
-
-	assert_eq!(converted_tag.frames.len(), 1);
-	let actual_frame = converted_tag.frames.first().unwrap();
-
-	assert_eq!(actual_frame.id(), &FrameId::Valid(Cow::Borrowed("POPM")));
-	// Note: as POPM frames are considered equal by email alone, each field must
-	// be separately validated
-	match actual_frame {
-		Frame::Popularimeter(pop) => {
-			assert_eq!(pop.email, expected.email);
-			assert_eq!(pop.rating, expected.rating);
-			assert_eq!(pop.counter, expected.counter);
-		},
-		_ => unreachable!(),
-	}
-}
-
 #[test]
 fn fail_write_bad_frame() {
 	let mut tag = Id3v2Tag::default();
@@ -1322,3 +1291,17 @@ fn preserve_comment_lang_description_on_conversion() {
 		_ => panic!("Expected a CommentFrame"),
 	}
 }
+
+// TODO: Remove this once we have a better solution
+#[test]
+fn hold_back_4_character_txxx_description() {
+	let mut tag = Id3v2Tag::new();
+
+	let _ = tag.insert_user_text(String::from("MODE"), String::from("CBR"));
+
+	let tag: Tag = tag.into();
+	assert_eq!(tag.len(), 0);
+
+	let tag: Id3v2Tag = tag.into();
+	assert_eq!(tag.len(), 1);
+}

lofty/src/tag/item.rs

@@ -1,3 +1,4 @@
+use crate::tag::items::{Lang, UNKNOWN_LANGUAGE};
 use crate::tag::TagType;
 
 use std::borrow::Cow;
@@ -9,7 +10,6 @@ macro_rules! first_key {
 	};
 }
 
-use crate::tag::items::{Lang, UNKNOWN_LANGUAGE};
 pub(crate) use first_key;
 
 // This is used to create the key/ItemKey maps