From 28e560ebfe49501daa897e1853e10fcb2fe347ec Mon Sep 17 00:00:00 2001 From: jbob0 Date: Wed, 26 Aug 2026 12:11:41 -0400 Subject: [PATCH] Reject PUT changes to ImportList.MinRefreshInterval instead of discarding them MinRefreshInterval is a fixed, per-list-type constant declared on each IImportList implementation (e.g. GoodreadsBookshelf => 12 hours) and is intentionally excluded from persistence (TableMapping ignores it). ImportListFactory.SetProviderCharacteristics unconditionally overwrites it from the provider on every load, so a PUT that changed it was silently accepted (202) with the change discarded - the response goes on to echo the provider's value, not the one that was requested. Add a PutValidator rule that compares the requested value against the provider's authoritative value and returns a clear validation error when they differ, instead of a silent no-op. TimeSpan.Zero (what a non-nullable TimeSpan field deserializes to when omitted from the request body) is always allowed through, since no provider ever uses zero and clients that only send the fields they mean to change shouldn't get a spurious 400. An unknown id is left to the normal Get()-based 404 handling rather than surfacing as a validation error. Fixes #23 --- .../ImportLists/ImportListController.cs | 40 +++++ ...ListControllerMinRefreshIntervalFixture.cs | 150 ++++++++++++++++++ 2 files changed, 190 insertions(+) create mode 100644 src/Chaptarr.Core.Test/Api/ImportListControllerMinRefreshIntervalFixture.cs diff --git a/src/Chaptarr.Api.V1/ImportLists/ImportListController.cs b/src/Chaptarr.Api.V1/ImportLists/ImportListController.cs index 00b09f71..c8973d47 100644 --- a/src/Chaptarr.Api.V1/ImportLists/ImportListController.cs +++ b/src/Chaptarr.Api.V1/ImportLists/ImportListController.cs @@ -1,4 +1,6 @@ +using System; using Chaptarr.Http; +using FluentValidation; using NzbDrone.Core.ImportLists; using NzbDrone.Core.Validation; using NzbDrone.Core.Validation.Paths; @@ -11,11 +13,15 @@ public class ImportListController : ProviderControllerBase r?.Implementation != "HardcoverLibraryImportList" && r?.Implementation != "GoodreadsBookshelf" && @@ -31,6 +37,40 @@ public ImportListController(IImportListFactory importListFactory, SharedValidator.RuleFor(c => c.QualityProfileId).SetValidator(qualityProfileExistsValidator); SharedValidator.RuleFor(c => c.MetadataProfileId).SetValidator(metadataProfileExistsValidator); }); + + // MinRefreshInterval is a fixed, per-list-type constant (see each IImportList implementation) + // and is intentionally excluded from persistence (TableMapping ignores it). Prior to this + // check, a PUT changing it was silently accepted (202) with the change discarded - the + // response goes on to echo the provider's value, not the one that was requested. Reject a + // real change explicitly instead so the client gets a clear error. TimeSpan.Zero (the value + // of an omitted field, since MinRefreshInterval is a non-nullable TimeSpan) is always allowed + // through - no provider uses zero, so this can't mask an actual attempt to zero it out, and it + // keeps clients that only send the fields they mean to change from getting a spurious 400. + PutValidator.RuleFor(c => c.MinRefreshInterval) + .Must((resource, value) => value == default || IsUnchangedOrUnknownList(resource.Id, value)) + .WithMessage(resource => $"minRefreshInterval is fixed by the list type and cannot be changed (current value: {GetProviderMinRefreshInterval(resource.Id)})"); + } + + private bool IsUnchangedOrUnknownList(int id, TimeSpan value) + { + var providerValue = GetProviderMinRefreshInterval(id); + + // An unknown id is left for GetDefinition/_providerFactory.Get to reject with its usual 404 + // rather than surfacing as a validation error here. + return providerValue == null || providerValue == value; + } + + private TimeSpan? GetProviderMinRefreshInterval(int id) + { + var existing = _importListFactory.Find(id); + if (existing == null) + { + return null; + } + + _importListFactory.SetProviderCharacteristics(existing); + + return existing.MinRefreshInterval; } } } diff --git a/src/Chaptarr.Core.Test/Api/ImportListControllerMinRefreshIntervalFixture.cs b/src/Chaptarr.Core.Test/Api/ImportListControllerMinRefreshIntervalFixture.cs new file mode 100644 index 00000000..20d5455d --- /dev/null +++ b/src/Chaptarr.Core.Test/Api/ImportListControllerMinRefreshIntervalFixture.cs @@ -0,0 +1,150 @@ +using System; +using System.Collections.Generic; +using System.Reflection; +using Chaptarr.Api.V1.ImportLists; +using FluentValidation; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; +using NUnit.Framework; +using NzbDrone.Core.ImportLists; +using NzbDrone.Core.Profiles.Metadata; +using NzbDrone.Core.Profiles.Qualities; +using NzbDrone.Core.Validation; + +namespace Chaptarr.Core.Test.Api +{ + [TestFixture] + public class ImportListControllerMinRefreshIntervalFixture + { + private class ThrowingProxy : DispatchProxy where T : class + { + protected override object Invoke(MethodInfo targetMethod, object[] args) + { + throw new NotImplementedException($"Test proxy does not implement {typeof(T).Name}.{targetMethod?.Name}"); + } + } + + private sealed class StubImportListFactory : IImportListFactory + { + public ImportListDefinition Existing { get; set; } + public TimeSpan ProviderMinRefreshInterval { get; set; } = TimeSpan.FromHours(12); + + public List All() => throw new NotImplementedException(); + public List GetAvailableProviders() => throw new NotImplementedException(); + public bool Exists(int id) => throw new NotImplementedException(); + public ImportListDefinition Find(int id) => Existing != null && Existing.Id == id ? Existing : null; + public ImportListDefinition Get(int id) => Existing != null && Existing.Id == id ? Existing : throw new NotImplementedException(); + public IEnumerable Get(IEnumerable ids) => throw new NotImplementedException(); + public ImportListDefinition Create(ImportListDefinition definition) => throw new NotImplementedException(); + public void Update(ImportListDefinition definition) => throw new NotImplementedException(); + public IEnumerable Update(IEnumerable definitions) => throw new NotImplementedException(); + public void Delete(int id) => throw new NotImplementedException(); + public void Delete(IEnumerable ids) => throw new NotImplementedException(); + public IEnumerable GetDefaultDefinitions() => throw new NotImplementedException(); + public IEnumerable GetPresetDefinitions(ImportListDefinition providerDefinition) => throw new NotImplementedException(); + public void SetProviderCharacteristics(ImportListDefinition definition) => definition.MinRefreshInterval = ProviderMinRefreshInterval; + public void SetProviderCharacteristics(IImportList provider, ImportListDefinition definition) => throw new NotImplementedException(); + public IImportList GetInstance(ImportListDefinition definition) => throw new NotImplementedException(); + public FluentValidation.Results.ValidationResult Test(ImportListDefinition definition) => throw new NotImplementedException(); + public object RequestAction(ImportListDefinition definition, string action, IDictionary query) => throw new NotImplementedException(); + public List AllForTag(int tagId) => throw new NotImplementedException(); + public List AutomaticAddEnabled(bool filterBlockedImportLists = true) => throw new NotImplementedException(); + } + + private static ImportListController BuildController(StubImportListFactory factory) + { + var controller = new ImportListController( + factory, + new QualityProfileExistsValidator(DispatchProxy.Create>()), + new MetadataProfileExistsValidator(DispatchProxy.Create>())); + + controller.ControllerContext = new ControllerContext + { + HttpContext = new DefaultHttpContext() + }; + controller.Request.Method = "PUT"; + + return controller; + } + + // ValidateResource is the same protected method OnActionExecuting calls on every real PUT + // (Chaptarr.Http/REST/RestController.cs). Invoking it directly here, with skipSharedValidate:true, + // isolates the PutValidator rule under test from the unrelated SharedValidator rules (name + // uniqueness, config contract, etc.) that would otherwise require a much heavier fixture. + private static void Validate(ImportListController controller, ImportListResource resource) + { + var method = typeof(ImportListController).GetMethod("ValidateResource", BindingFlags.NonPublic | BindingFlags.Instance); + try + { + method.Invoke(controller, new object[] { resource, false, true }); + } + catch (TargetInvocationException ex) when (ex.InnerException != null) + { + throw ex.InnerException; + } + } + + [Test] + public void should_reject_put_that_changes_min_refresh_interval() + { + var factory = new StubImportListFactory + { + Existing = new ImportListDefinition { Id = 7, MinRefreshInterval = TimeSpan.FromHours(12) }, + ProviderMinRefreshInterval = TimeSpan.FromHours(12) + }; + var controller = BuildController(factory); + + var resource = new ImportListResource { Id = 7, MinRefreshInterval = TimeSpan.FromMinutes(5) }; + + var ex = Assert.Throws(() => Validate(controller, resource)); + + Assert.That(ex.Message, Does.Contain("minRefreshInterval is fixed by the list type")); + Assert.That(ex.Message, Does.Contain("12:00:00")); + } + + [Test] + public void should_allow_put_that_leaves_min_refresh_interval_unchanged() + { + var factory = new StubImportListFactory + { + Existing = new ImportListDefinition { Id = 7, MinRefreshInterval = TimeSpan.FromHours(12) }, + ProviderMinRefreshInterval = TimeSpan.FromHours(12) + }; + var controller = BuildController(factory); + + var resource = new ImportListResource { Id = 7, MinRefreshInterval = TimeSpan.FromHours(12) }; + + Assert.DoesNotThrow(() => Validate(controller, resource)); + } + + [Test] + public void should_allow_put_that_omits_min_refresh_interval() + { + // A non-nullable TimeSpan field that's absent from the JSON body deserializes to + // TimeSpan.Zero, indistinguishable from a client explicitly sending "00:00:00". Since no + // provider ever uses zero, this can't mask a real attempt to change the value, and it stops + // clients that only send the fields they mean to change from getting a spurious 400. + var factory = new StubImportListFactory + { + Existing = new ImportListDefinition { Id = 7, MinRefreshInterval = TimeSpan.FromHours(12) }, + ProviderMinRefreshInterval = TimeSpan.FromHours(12) + }; + var controller = BuildController(factory); + + var resource = new ImportListResource { Id = 7, MinRefreshInterval = TimeSpan.Zero }; + + Assert.DoesNotThrow(() => Validate(controller, resource)); + } + + [Test] + public void should_defer_to_the_normal_not_found_handling_for_an_unknown_id() + { + var factory = new StubImportListFactory { Existing = null }; + var controller = BuildController(factory); + + var resource = new ImportListResource { Id = 999, MinRefreshInterval = TimeSpan.FromMinutes(5) }; + + Assert.DoesNotThrow(() => Validate(controller, resource)); + } + } +}