From 3262f8dc2a7e8fb03ecd345eeca70bdb372389e3 Mon Sep 17 00:00:00 2001
From: Erwin de Haan <1627021+EraYaN@users.noreply.github.com>
Date: Mon, 15 Jul 2024 14:44:22 +0200
Subject: [PATCH] Add check for ProviderIds to prevent '=' from appearing in
 keys, also support '=' in the values. (#12274)

---
 .../Data/SqliteItemRepository.cs              |  5 +-
 .../Entities/ProviderIdsExtensions.cs         | 60 +++++++++++++------
 .../Entities/ProviderIdsExtensionsTests.cs    | 18 ++++--
 3 files changed, 60 insertions(+), 23 deletions(-)

diff --git a/Emby.Server.Implementations/Data/SqliteItemRepository.cs b/Emby.Server.Implementations/Data/SqliteItemRepository.cs
index 81ee55d26c..c2e3312ab2 100644
--- a/Emby.Server.Implementations/Data/SqliteItemRepository.cs
+++ b/Emby.Server.Implementations/Data/SqliteItemRepository.cs
@@ -1046,9 +1046,10 @@ namespace Emby.Server.Implementations.Data
             foreach (var part in value.SpanSplit('|'))
             {
                 var providerDelimiterIndex = part.IndexOf('=');
-                if (providerDelimiterIndex != -1 && providerDelimiterIndex == part.LastIndexOf('='))
+                // Don't let empty values through
+                if (providerDelimiterIndex != -1 && part.Length != providerDelimiterIndex + 1)
                 {
-                    item.SetProviderId(part.Slice(0, providerDelimiterIndex).ToString(), part.Slice(providerDelimiterIndex + 1).ToString());
+                    item.SetProviderId(part[..providerDelimiterIndex].ToString(), part[(providerDelimiterIndex + 1)..].ToString());
                 }
             }
         }
diff --git a/MediaBrowser.Model/Entities/ProviderIdsExtensions.cs b/MediaBrowser.Model/Entities/ProviderIdsExtensions.cs
index cf453d62cb..1c73091f0d 100644
--- a/MediaBrowser.Model/Entities/ProviderIdsExtensions.cs
+++ b/MediaBrowser.Model/Entities/ProviderIdsExtensions.cs
@@ -111,31 +111,32 @@ namespace MediaBrowser.Model.Entities
         /// Sets a provider id.
         /// </summary>
         /// <param name="instance">The instance.</param>
-        /// <param name="name">The name.</param>
+        /// <param name="name">The name, this should not contain a '=' character.</param>
         /// <param name="value">The value.</param>
-        public static void SetProviderId(this IHasProviderIds instance, string name, string? value)
+        /// <remarks>Due to how deserialization from the database works the name can not contain '='.</remarks>
+        public static void SetProviderId(this IHasProviderIds instance, string name, string value)
         {
             ArgumentNullException.ThrowIfNull(instance);
+            ArgumentException.ThrowIfNullOrEmpty(name);
+            ArgumentException.ThrowIfNullOrEmpty(value);
+
+            // When name contains a '=' it can't be deserialized from the database
+            if (name.Contains('=', StringComparison.Ordinal))
+            {
+                throw new ArgumentException("Provider id name cannot contain '='", nameof(name));
+            }
+
+            // Ensure it exists
+            instance.ProviderIds ??= new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
 
-            // If it's null remove the key from the dictionary
-            if (string.IsNullOrEmpty(value))
+            // Match on internal MetadataProvider enum string values before adding arbitrary providers
+            if (_metadataProviderEnumDictionary.TryGetValue(name, out var enumValue))
             {
-                instance.ProviderIds?.Remove(name);
+                instance.ProviderIds[enumValue] = value;
             }
             else
             {
-                // Ensure it exists
-                instance.ProviderIds ??= new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
-
-                // Match on internal MetadataProvider enum string values before adding arbitrary providers
-                if (_metadataProviderEnumDictionary.TryGetValue(name, out var enumValue))
-                {
-                    instance.ProviderIds[enumValue] = value;
-                }
-                else
-                {
-                    instance.ProviderIds[name] = value;
-                }
+                instance.ProviderIds[name] = value;
             }
         }
 
@@ -149,5 +150,30 @@ namespace MediaBrowser.Model.Entities
         {
             instance.SetProviderId(provider.ToString(), value);
         }
+
+        /// <summary>
+        /// Removes a provider id.
+        /// </summary>
+        /// <param name="instance">The instance.</param>
+        /// <param name="name">The name.</param>
+        public static void RemoveProviderId(this IHasProviderIds instance, string name)
+        {
+            ArgumentNullException.ThrowIfNull(instance);
+            ArgumentException.ThrowIfNullOrEmpty(name);
+
+            instance.ProviderIds?.Remove(name);
+        }
+
+        /// <summary>
+        /// Removes a provider id.
+        /// </summary>
+        /// <param name="instance">The instance.</param>
+        /// <param name="provider">The provider.</param>
+        public static void RemoveProviderId(this IHasProviderIds instance, MetadataProvider provider)
+        {
+            ArgumentNullException.ThrowIfNull(instance);
+
+            instance.ProviderIds?.Remove(provider.ToString());
+        }
     }
 }
diff --git a/tests/Jellyfin.Model.Tests/Entities/ProviderIdsExtensionsTests.cs b/tests/Jellyfin.Model.Tests/Entities/ProviderIdsExtensionsTests.cs
index 2a62ab74c3..a6f4164144 100644
--- a/tests/Jellyfin.Model.Tests/Entities/ProviderIdsExtensionsTests.cs
+++ b/tests/Jellyfin.Model.Tests/Entities/ProviderIdsExtensionsTests.cs
@@ -141,7 +141,7 @@ namespace Jellyfin.Model.Tests.Entities
         public void SetProviderId_Null_Remove()
         {
             var provider = new ProviderIdsExtensionsTestsObject();
-            provider.SetProviderId(MetadataProvider.Imdb, null!);
+            Assert.Throws<ArgumentNullException>(() => provider.SetProviderId(MetadataProvider.Imdb, null!));
             Assert.Empty(provider.ProviderIds);
         }
 
@@ -150,8 +150,8 @@ namespace Jellyfin.Model.Tests.Entities
         {
             var provider = new ProviderIdsExtensionsTestsObject();
             provider.ProviderIds[MetadataProvider.Imdb.ToString()] = ExampleImdbId;
-            provider.SetProviderId(MetadataProvider.Imdb, string.Empty);
-            Assert.Empty(provider.ProviderIds);
+            Assert.Throws<ArgumentException>(() => provider.SetProviderId(MetadataProvider.Imdb, string.Empty));
+            Assert.Single(provider.ProviderIds);
         }
 
         [Fact]
@@ -182,10 +182,20 @@ namespace Jellyfin.Model.Tests.Entities
                 ProviderIds = null!
             };
 
-            nullProvider.SetProviderId(MetadataProvider.Imdb, string.Empty);
+            Assert.Throws<ArgumentException>(() => nullProvider.SetProviderId(MetadataProvider.Imdb, string.Empty));
             Assert.Null(nullProvider.ProviderIds);
         }
 
+        [Fact]
+        public void RemoveProviderId_Null_Remove()
+        {
+            var provider = new ProviderIdsExtensionsTestsObject();
+
+            provider.ProviderIds[MetadataProvider.Imdb.ToString()] = ExampleImdbId;
+            provider.RemoveProviderId(MetadataProvider.Imdb);
+            Assert.Empty(provider.ProviderIds);
+        }
+
         private sealed class ProviderIdsExtensionsTestsObject : IHasProviderIds
         {
             public static readonly ProviderIdsExtensionsTestsObject Empty = new ProviderIdsExtensionsTestsObject();