Merge pull request #17584 from Shadowghost/safeguard-invalid-provider-ids

Safeguard against invalid provider ids
2026-08-22 08:49:07 -04:00
parent 0d4cbb999b 42c70fba63
commit 49112163eb
22 changed files with 760 additions and 85 deletions
@@ -186,6 +186,109 @@ namespace Jellyfin.Model.Tests.Entities
            Assert.Null(nullProvider.ProviderIds);
        }

+        [Theory]
+        [InlineData(nameof(MetadataProvider.Imdb), "tt0113375", true)]
+        [InlineData(nameof(MetadataProvider.Imdb), "nm0000123", true)]
+        [InlineData(nameof(MetadataProvider.Imdb), "0113375", true)]
+        [InlineData(nameof(MetadataProvider.Imdb), "https://www.imdb.com/title/tt0113375", false)]
+        [InlineData(nameof(MetadataProvider.Tmdb), "11", true)]
+        [InlineData(nameof(MetadataProvider.Tmdb), "nm0000123", false)]
+        [InlineData(nameof(MetadataProvider.Tmdb), "0", false)]
+        [InlineData(nameof(MetadataProvider.Tmdb), "-11", false)]
+        [InlineData(nameof(MetadataProvider.TmdbCollection), "nm0000123", false)]
+        [InlineData(nameof(MetadataProvider.AudioDbArtist), "111239", true)]
+        [InlineData(nameof(MetadataProvider.AudioDbArtist), "a3cb23fc-acd3-4ce0-8f36-1e5aa6a18432", false)]
+        [InlineData(nameof(MetadataProvider.MusicBrainzArtist), "a3cb23fc-acd3-4ce0-8f36-1e5aa6a18432", true)]
+        [InlineData(nameof(MetadataProvider.MusicBrainzArtist), "111239", false)]
+        [InlineData(nameof(MetadataProvider.MusicBrainzAlbum), "not-an-mbid", false)]
+        [InlineData(nameof(MetadataProvider.Tvdb), "anything-goes", true)]
+        [InlineData("SomePlugin", "anything-goes", true)]
+        [InlineData(nameof(MetadataProvider.Tmdb), null, false)]
+        [InlineData(null, "11", false)]
+        public void IsValidProviderId_ChecksKnownFormats(string? name, string? value, bool expected)
+        {
+            Assert.Equal(expected, ProviderIdsExtensions.IsValidProviderId(name, value));
+        }
+
+        [Fact]
+        public void TrySetProviderId_ForeignId_False()
+        {
+            var provider = new ProviderIdsExtensionsTestsObject();
+
+            Assert.False(provider.TrySetProviderId(MetadataProvider.Tmdb, "nm0000123"));
+            Assert.Empty(provider.ProviderIds);
+        }
+
+        [Fact]
+        public void TrySetProviderId_ForeignId_KeepsExisting()
+        {
+            var provider = new ProviderIdsExtensionsTestsObject();
+            provider.ProviderIds[MetadataProvider.Tmdb.ToString()] = "11";
+
+            Assert.False(provider.TrySetProviderId(MetadataProvider.Tmdb, "nm0000123"));
+            Assert.Equal("11", provider.GetProviderId(MetadataProvider.Tmdb));
+        }
+
+        [Theory]
+        [InlineData(nameof(MetadataProvider.Imdb), " tt0113375 ")]
+        [InlineData(" Imdb", ExampleImdbId)]
+        public void TrySetProviderId_SurroundingWhitespace_Trimmed(string name, string value)
+        {
+            var provider = new ProviderIdsExtensionsTestsObject();
+
+            Assert.True(provider.TrySetProviderId(name, value));
+            Assert.Equal(ExampleImdbId, provider.GetProviderId(MetadataProvider.Imdb));
+        }
+
+        [Fact]
+        public void SetProviderIds_ReplacesAll()
+        {
+            var provider = new ProviderIdsExtensionsTestsObject();
+            provider.ProviderIds[MetadataProvider.Tvdb.ToString()] = "12345";
+
+            provider.SetProviderIds(new Dictionary<string, string>
+            {
+                [MetadataProvider.Imdb.ToString()] = ExampleImdbId
+            });
+
+            Assert.Equal(ExampleImdbId, provider.GetProviderId(MetadataProvider.Imdb));
+            Assert.False(provider.HasProviderId(MetadataProvider.Tvdb));
+        }
+
+        [Fact]
+        public void SetProviderIds_ForeignId_Dropped()
+        {
+            var provider = new ProviderIdsExtensionsTestsObject();
+
+            provider.SetProviderIds(new Dictionary<string, string>
+            {
+                [MetadataProvider.Tmdb.ToString()] = "nm0000123",
+                [MetadataProvider.Imdb.ToString()] = ExampleImdbId,
+                [MetadataProvider.Tvdb.ToString()] = string.Empty
+            });
+
+            Assert.False(provider.HasProviderId(MetadataProvider.Tmdb));
+            Assert.False(provider.HasProviderId(MetadataProvider.Tvdb));
+            Assert.Equal(ExampleImdbId, provider.GetProviderId(MetadataProvider.Imdb));
+        }
+
+        [Fact]
+        public void SetProviderIds_Null_Clears()
+        {
+            var provider = new ProviderIdsExtensionsTestsObject();
+            provider.ProviderIds[MetadataProvider.Imdb.ToString()] = ExampleImdbId;
+
+            provider.SetProviderIds(null);
+
+            Assert.Empty(provider.ProviderIds);
+        }
+
+        [Fact]
+        public void SetProviderIds_NullInstance_ThrowsArgumentNullException()
+        {
+            Assert.Throws<ArgumentNullException>(() => ProviderIdsExtensions.SetProviderIds(null!, new Dictionary<string, string>()));
+        }
+
        [Fact]
        public void RemoveProviderId_Null_Remove()
        {
@@ -0,0 +1,253 @@
+using System;
+using System.Collections.Generic;
+using System.Net.Http;
+using System.Threading;
+using System.Threading.Tasks;
+using Jellyfin.Data.Enums;
+using MediaBrowser.Controller.Configuration;
+using MediaBrowser.Controller.Entities;
+using MediaBrowser.Controller.Entities.Movies;
+using MediaBrowser.Controller.IO;
+using MediaBrowser.Controller.Library;
+using MediaBrowser.Controller.Persistence;
+using MediaBrowser.Controller.Providers;
+using MediaBrowser.Model.Entities;
+using MediaBrowser.Model.IO;
+using MediaBrowser.Providers.Manager;
+using Microsoft.Extensions.Logging.Abstractions;
+using Moq;
+using Xunit;
+
+namespace Jellyfin.Providers.Tests.Manager
+{
+    public class MetadataServiceRefreshTests
+    {
+        [Theory]
+        // RemoveOldMetadata is only ever set by an explicit user action - a refresh with "replace all
+        // metadata", or Identify. A provider failing must not silently downgrade that to a merge: the
+        // providers that did answer supplied the replacement, and the old values are the wrong match
+        // the user asked to get rid of.
+        [InlineData(false)]
+        [InlineData(true)]
+        public async Task RefreshWithProviders_ReplaceAllMetadata_ErasesOldDataWhenAProviderAnswers(bool allProvidersSucceed)
+        {
+            var item = new Movie
+            {
+                Name = "Test Movie",
+                Overview = "existing overview"
+            };
+
+            // The provider owning the overview fails, so it contributes nothing to the replacement.
+            var failing = new Mock<IRemoteMetadataProvider<Movie, MovieInfo>>(MockBehavior.Loose);
+            failing.Setup(p => p.Name).Returns("Failing");
+            failing.Setup(p => p.GetMetadata(It.IsAny<MovieInfo>(), It.IsAny<CancellationToken>()))
+                .Returns(allProvidersSucceed
+                    ? Task.FromResult(new MetadataResult<Movie> { HasMetadata = true, Item = new Movie() })
+                    : Task.FromException<MetadataResult<Movie>>(new FormatException("bad id")));
+
+            var succeeding = new Mock<IRemoteMetadataProvider<Movie, MovieInfo>>(MockBehavior.Loose);
+            succeeding.Setup(p => p.Name).Returns("Succeeding");
+            succeeding.Setup(p => p.GetMetadata(It.IsAny<MovieInfo>(), It.IsAny<CancellationToken>()))
+                .ReturnsAsync(new MetadataResult<Movie>
+                {
+                    HasMetadata = true,
+                    Item = new Movie { Name = "Test Movie", Tagline = "new tagline" }
+                });
+
+            var service = new TestMetadataService();
+            var result = await service.RefreshWithProvidersInternal(
+                new MetadataResult<Movie> { Item = item },
+                new MovieInfo { Name = item.Name },
+                new MetadataRefreshOptions(Mock.Of<IDirectoryService>())
+                {
+                    MetadataRefreshMode = MetadataRefreshMode.FullRefresh,
+                    ReplaceAllMetadata = true,
+                    RemoveOldMetadata = true
+                },
+                [failing.Object, succeeding.Object]).ConfigureAwait(true);
+
+            Assert.Equal(allProvidersSucceed ? 0 : 1, result.Failures);
+            Assert.Equal("new tagline", item.Tagline);
+            Assert.Null(item.Overview);
+        }
+
+        [Fact]
+        public async Task RefreshWithProviders_ReplaceAllMetadata_KeepsExistingDataWhenEveryRemoteProviderFails()
+        {
+            var item = new Movie
+            {
+                Name = "Test Movie",
+                Overview = "existing overview"
+            };
+
+            // Something has to contribute for the merge to run at all, otherwise the item is never touched
+            // and the case is moot. The local provider is the replacement the remote ones did not deliver.
+            var local = new Mock<ILocalMetadataProvider<Movie>>(MockBehavior.Loose);
+            local.Setup(p => p.Name).Returns("Local");
+            local.Setup(p => p.GetMetadata(It.IsAny<ItemInfo>(), It.IsAny<IDirectoryService>(), It.IsAny<CancellationToken>()))
+                .ReturnsAsync(new MetadataResult<Movie>
+                {
+                    HasMetadata = true,
+                    Item = new Movie { Name = "Test Movie", Tagline = "new tagline" }
+                });
+
+            var remote = new Mock<IRemoteMetadataProvider<Movie, MovieInfo>>(MockBehavior.Loose);
+            remote.Setup(p => p.Name).Returns("Failing");
+            remote.Setup(p => p.GetMetadata(It.IsAny<MovieInfo>(), It.IsAny<CancellationToken>()))
+                .Returns(Task.FromException<MetadataResult<Movie>>(new HttpRequestException("unreachable")));
+
+            var service = new TestMetadataService();
+            var result = await service.RefreshWithProvidersInternal(
+                new MetadataResult<Movie> { Item = item },
+                new MovieInfo { Name = item.Name },
+                new MetadataRefreshOptions(Mock.Of<IDirectoryService>())
+                {
+                    MetadataRefreshMode = MetadataRefreshMode.FullRefresh,
+                    ReplaceAllMetadata = true,
+                    RemoveOldMetadata = true
+                },
+                [local.Object, remote.Object]).ConfigureAwait(true);
+
+            Assert.Equal(1, result.Failures);
+            Assert.Equal("new tagline", item.Tagline);
+
+            // No remote provider answered, so erasing the overview would lose it for good.
+            Assert.Equal("existing overview", item.Overview);
+        }
+
+        [Fact]
+        public async Task RefreshWithProviders_ForeignProviderId_NotStored()
+        {
+            var item = new Movie { Name = "Test Movie" };
+
+            var provider = new Mock<IRemoteMetadataProvider<Movie, MovieInfo>>(MockBehavior.Loose);
+            provider.Setup(p => p.Name).Returns("Provider");
+            provider.Setup(p => p.GetMetadata(It.IsAny<MovieInfo>(), It.IsAny<CancellationToken>()))
+                .ReturnsAsync(() =>
+                {
+                    var found = new Movie { Name = "Test Movie" };
+                    found.ProviderIds[MetadataProvider.Tmdb.ToString()] = "nm0000123";
+                    found.ProviderIds[MetadataProvider.Imdb.ToString()] = "tt0113375";
+                    return new MetadataResult<Movie> { HasMetadata = true, Item = found };
+                });
+
+            var service = new TestMetadataService();
+            await service.RefreshWithProvidersInternal(
+                new MetadataResult<Movie> { Item = item },
+                new MovieInfo { Name = item.Name },
+                new MetadataRefreshOptions(Mock.Of<IDirectoryService>())
+                {
+                    MetadataRefreshMode = MetadataRefreshMode.FullRefresh,
+                    ReplaceAllMetadata = true
+                },
+                [provider.Object]).ConfigureAwait(true);
+
+            Assert.False(item.HasProviderId(MetadataProvider.Tmdb));
+            Assert.Equal("tt0113375", item.GetProviderId(MetadataProvider.Imdb));
+        }
+
+        [Fact]
+        public async Task RefreshWithProviders_ForeignProviderId_ReplacedInLookupInfo()
+        {
+            var item = new Movie { Name = "Test Movie" };
+            var lookupInfo = new MovieInfo { Name = item.Name };
+            lookupInfo.ProviderIds[MetadataProvider.Tmdb.ToString()] = "nm0000123";
+
+            var answering = new Mock<IRemoteMetadataProvider<Movie, MovieInfo>>(MockBehavior.Loose);
+            answering.Setup(p => p.Name).Returns("Answering");
+            answering.Setup(p => p.GetMetadata(It.IsAny<MovieInfo>(), It.IsAny<CancellationToken>()))
+                .ReturnsAsync(() =>
+                {
+                    var found = new Movie { Name = "Test Movie" };
+                    found.ProviderIds[MetadataProvider.Tmdb.ToString()] = "12345";
+                    return new MetadataResult<Movie> { HasMetadata = true, Item = found };
+                });
+
+            string? tmdbIdSeenBySecondProvider = null;
+            var following = new Mock<IRemoteMetadataProvider<Movie, MovieInfo>>(MockBehavior.Loose);
+            following.Setup(p => p.Name).Returns("Following");
+            following.Setup(p => p.GetMetadata(It.IsAny<MovieInfo>(), It.IsAny<CancellationToken>()))
+                .ReturnsAsync((MovieInfo info, CancellationToken _) =>
+                {
+                    tmdbIdSeenBySecondProvider = info.GetProviderId(MetadataProvider.Tmdb);
+                    return new MetadataResult<Movie> { HasMetadata = false };
+                });
+
+            var service = new TestMetadataService();
+            await service.RefreshWithProvidersInternal(
+                new MetadataResult<Movie> { Item = item },
+                lookupInfo,
+                new MetadataRefreshOptions(Mock.Of<IDirectoryService>())
+                {
+                    MetadataRefreshMode = MetadataRefreshMode.FullRefresh,
+                    ReplaceAllMetadata = true
+                },
+                [answering.Object, following.Object]).ConfigureAwait(true);
+
+            // The stored id cannot be a TMDb one, so the provider that still has to run must get the id
+            // that was just found instead of failing on the same bad one.
+            Assert.Equal("12345", tmdbIdSeenBySecondProvider);
+        }
+
+        [Theory]
+        [InlineData(true)]
+        [InlineData(false)]
+        public async Task RefreshWithProviders_ForeignPersonProviderId_NotStored(bool replaceAllMetadata)
+        {
+            var item = new Movie { Name = "Test Movie" };
+            var existing = new MetadataResult<Movie> { Item = item };
+            existing.AddPerson(new PersonInfo { Name = "Some Actor", Type = PersonKind.Actor });
+
+            var provider = new Mock<IRemoteMetadataProvider<Movie, MovieInfo>>(MockBehavior.Loose);
+            provider.Setup(p => p.Name).Returns("Provider");
+            provider.Setup(p => p.GetMetadata(It.IsAny<MovieInfo>(), It.IsAny<CancellationToken>()))
+                .ReturnsAsync(() =>
+                {
+                    var person = new PersonInfo { Name = "Some Actor", Type = PersonKind.Actor };
+                    person.ProviderIds[MetadataProvider.Tmdb.ToString()] = "nm0000123";
+                    person.ProviderIds[MetadataProvider.Imdb.ToString()] = "nm0000123";
+
+                    var found = new MetadataResult<Movie> { HasMetadata = true, Item = new Movie { Name = "Test Movie" } };
+                    found.AddPerson(person);
+                    return found;
+                });
+
+            var service = new TestMetadataService();
+            await service.RefreshWithProvidersInternal(
+                existing,
+                new MovieInfo { Name = item.Name },
+                new MetadataRefreshOptions(Mock.Of<IDirectoryService>())
+                {
+                    MetadataRefreshMode = MetadataRefreshMode.FullRefresh,
+                    ReplaceAllMetadata = replaceAllMetadata
+                },
+                [provider.Object]).ConfigureAwait(true);
+
+            var mergedPerson = Assert.Single(existing.People);
+            Assert.False(mergedPerson.HasProviderId(MetadataProvider.Tmdb));
+            Assert.Equal("nm0000123", mergedPerson.GetProviderId(MetadataProvider.Imdb));
+        }
+
+        private sealed class TestMetadataService : MetadataService<Movie, MovieInfo>
+        {
+            public TestMetadataService()
+                : base(
+                    Mock.Of<IServerConfigurationManager>(),
+                    NullLogger<MetadataService<Movie, MovieInfo>>.Instance,
+                    Mock.Of<IProviderManager>(),
+                    Mock.Of<IFileSystem>(),
+                    Mock.Of<ILibraryManager>(),
+                    Mock.Of<IExternalDataManager>(),
+                    Mock.Of<IItemRepository>())
+            {
+            }
+
+            public Task<RefreshResult> RefreshWithProvidersInternal(
+                MetadataResult<Movie> metadata,
+                MovieInfo id,
+                MetadataRefreshOptions options,
+                ICollection<IMetadataProvider> providers)
+                => RefreshWithProviders(metadata, id, options, providers, ImageProvider, false, CancellationToken.None);
+        }
+    }
+}
@@ -0,0 +1,59 @@
+using MediaBrowser.Controller.Providers;
+using MediaBrowser.Model.Entities;
+using MediaBrowser.Providers.Music;
+using Xunit;
+
+namespace Jellyfin.Providers.Tests.Music;
+
+public static class AlbumInfoExtensionsTests
+{
+    private const string ExampleMbid = "59b5a40b-e2fd-3f18-a218-e8c9aae12ab5";
+    private const string SongMbid = "6c301dbd-6ccb-3403-a6c4-6a22240a0297";
+
+    [Theory]
+    [InlineData(ExampleMbid, ExampleMbid)]
+    // Another provider's id under a MusicBrainz key reads as no id, so the caller searches instead of
+    // handing a value the MusicBrainz client throws on.
+    [InlineData("111239", null)]
+    [InlineData("", null)]
+    public static void GetReleaseId_OnlyReturnsMbids(string id, string? expected)
+    {
+        var info = new AlbumInfo();
+        info.ProviderIds[MetadataProvider.MusicBrainzAlbum.ToString()] = id;
+
+        Assert.Equal(expected, info.GetReleaseId());
+    }
+
+    [Fact]
+    public static void GetReleaseId_ForeignId_FallsBackToSongs()
+    {
+        var song = new SongInfo();
+        song.ProviderIds[MetadataProvider.MusicBrainzAlbum.ToString()] = SongMbid;
+
+        var info = new AlbumInfo { SongInfos = [song] };
+        info.ProviderIds[MetadataProvider.MusicBrainzAlbum.ToString()] = "111239";
+
+        Assert.Equal(SongMbid, info.GetReleaseId());
+    }
+
+    [Fact]
+    public static void GetMusicBrainzArtistId_ForeignId_FallsBackToArtistIds()
+    {
+        var info = new AlbumInfo();
+        info.ProviderIds[MetadataProvider.MusicBrainzAlbumArtist.ToString()] = "111239";
+        info.ArtistProviderIds[MetadataProvider.MusicBrainzArtist.ToString()] = ExampleMbid;
+
+        Assert.Equal(ExampleMbid, info.GetMusicBrainzArtistId());
+    }
+
+    [Theory]
+    [InlineData(ExampleMbid, ExampleMbid)]
+    [InlineData("111239", null)]
+    public static void GetMusicBrainzArtistId_ArtistInfo_OnlyReturnsMbids(string id, string? expected)
+    {
+        var info = new ArtistInfo();
+        info.ProviderIds[MetadataProvider.MusicBrainzArtist.ToString()] = id;
+
+        Assert.Equal(expected, info.GetMusicBrainzArtistId());
+    }
+}
@@ -1,3 +1,5 @@
+using MediaBrowser.Controller.Entities.Movies;
+using MediaBrowser.Model.Entities;
 using MediaBrowser.Providers.Plugins.Tmdb;
 using Xunit;

@@ -34,5 +36,40 @@ namespace Jellyfin.Providers.Tests.Tmdb
        {
            Assert.Equal(expected, TmdbUtils.AdjustImageLanguage(imageLanguage, requestLanguage));
        }
+
+        [Theory]
+        [InlineData("11", true, 11)]
+        // An id another provider filed under the TMDb key must not throw, it is simply not a TMDb id.
+        [InlineData("nm0000123", false, 0)]
+        [InlineData("tt0113375", false, 0)]
+        [InlineData("11.0", false, 0)]
+        [InlineData("-11", false, 0)]
+        [InlineData("0", false, 0)]
+        [InlineData("", false, 0)]
+        [InlineData(null, false, 0)]
+        public static void TryParseTmdbId_OnlyAcceptsTmdbIds(string? value, bool expected, int expectedId)
+        {
+            Assert.Equal(expected, TmdbUtils.TryParseTmdbId(value, out var tmdbId));
+            Assert.Equal(expectedId, tmdbId);
+        }
+
+        [Theory]
+        [InlineData("11", true, 11)]
+        [InlineData("nm0000123", false, 0)]
+        public static void TryGetTmdbId_OnlyAcceptsTmdbIds(string value, bool expected, int expectedId)
+        {
+            var item = new Movie();
+            item.ProviderIds[MetadataProvider.Tmdb.ToString()] = value;
+
+            Assert.Equal(expected, item.TryGetTmdbId(out var tmdbId));
+            Assert.Equal(expectedId, tmdbId);
+        }
+
+        [Fact]
+        public static void TryGetTmdbId_NoId_False()
+        {
+            Assert.False(new Movie().TryGetTmdbId(out var tmdbId));
+            Assert.Equal(0, tmdbId);
+        }
    }
 }