From 216130b4d77b4873fa674de23a845c8e05d8ebf9 Mon Sep 17 00:00:00 2001 From: Timothy Date: Mon, 13 Jul 2026 00:27:14 +0200 Subject: [PATCH 1/2] =?UTF-8?q?fix(#172):=20API=20hardening=20=E2=80=94=20?= =?UTF-8?q?null-name=20500s,=20duplicate=20template=20items,=20unreachable?= =?UTF-8?q?=20404?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clears the still-live findings from #172 (verified against main; #2/#4/#7 and the auth/search/Trakt tail were already deliberate-documented or fixed since 2026-07-07). - Null/empty Name → 500 (10 create/replace handlers). Block/Template/DecoTemplate/Deco Create+Replace/Update + UpdateFFmpegProfile did `request.Name.Length > 50` on a client-nullable string → unhandled NullReferenceException → HTTP 500 (no global exception filter). Now `string.IsNullOrWhiteSpace(request.Name) || .Length > 50` → 422; also rejects empty/whitespace names, matching the group-create handlers' NotEmpty behavior. CreatePlaylist coalesces null→"" at the DTO so it was an empty-name persist, not a 500; guarded the same way. - ReplaceTemplateItems overlap validation iterated with an `item == otherItem` record value-equality skip, so two exact-duplicate items were value-equal and bypassed the intersection check (both persisted). Now index-based (i != j) so duplicates register as a self-intersection and are rejected 422. - Trimmed the unreachable 404 ProducesResponseType from POST /api/blocks/groups and POST /api/templates/groups (a create has no parent lookup that can 404); v1.json regenerated. - Regression tests: all 10 name-guard paths + the duplicate-items path (19 cases). - Docs: decisions.md entry + api-conventions.md §3b null-safe-validation bullet. fixes #172 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Commands/UpdateFFmpegProfileHandler.cs | 4 +- .../Commands/CreatePlaylistHandler.cs | 2 +- .../Scheduling/Commands/CreateBlockHandler.cs | 2 +- .../Scheduling/Commands/CreateDecoHandler.cs | 2 +- .../Commands/CreateDecoTemplateHandler.cs | 2 +- .../Commands/CreateTemplateHandler.cs | 2 +- .../Commands/ReplaceBlockItemsHandler.cs | 2 +- .../ReplaceDecoTemplateItemsHandler.cs | 2 +- .../Commands/ReplaceTemplateItemsHandler.cs | 14 +- .../Scheduling/Commands/UpdateDecoHandler.cs | 4 +- .../FFmpegProfileHandlerTests.cs | 25 ++ .../Scheduling/Issue172HardeningTests.cs | 316 ++++++++++++++++++ ErsatzTV/Controllers/Api/BlockController.cs | 1 - .../Controllers/Api/TemplateController.cs | 1 - ErsatzTV/wwwroot/openapi/v1.json | 40 --- docs/api-conventions.md | 9 + docs/decisions.md | 34 ++ 17 files changed, 405 insertions(+), 57 deletions(-) create mode 100644 ErsatzTV.Tests/Application/Scheduling/Issue172HardeningTests.cs diff --git a/ErsatzTV.Application/FFmpegProfiles/Commands/UpdateFFmpegProfileHandler.cs b/ErsatzTV.Application/FFmpegProfiles/Commands/UpdateFFmpegProfileHandler.cs index 5fc566c48..c066e4b91 100644 --- a/ErsatzTV.Application/FFmpegProfiles/Commands/UpdateFFmpegProfileHandler.cs +++ b/ErsatzTV.Application/FFmpegProfiles/Commands/UpdateFFmpegProfileHandler.cs @@ -1,4 +1,4 @@ -using ErsatzTV.Core; +using ErsatzTV.Core; using ErsatzTV.Core.Domain; using ErsatzTV.Core.Errors; using ErsatzTV.Core.FFmpeg; @@ -139,7 +139,7 @@ public class UpdateFFmpegProfileHandler(IDbContextFactory dbContextFa TvContext dbContext, UpdateFFmpegProfile updateFFmpegProfile) { - if (updateFFmpegProfile.Name.Length > 50) + if (string.IsNullOrWhiteSpace(updateFFmpegProfile.Name) || updateFFmpegProfile.Name.Length > 50) { return BaseError.New($"FFmpeg profile name \"{updateFFmpegProfile.Name}\" is invalid"); } diff --git a/ErsatzTV.Application/MediaCollections/Commands/CreatePlaylistHandler.cs b/ErsatzTV.Application/MediaCollections/Commands/CreatePlaylistHandler.cs index 3239089df..6ada3527a 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/CreatePlaylistHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/CreatePlaylistHandler.cs @@ -35,7 +35,7 @@ public class CreatePlaylistHandler(IDbContextFactory dbContextFactory TvContext dbContext, CreatePlaylist request) { - if (request.Name.Length > 50) + if (string.IsNullOrWhiteSpace(request.Name) || request.Name.Length > 50) { return BaseError.New($"Playlist name \"{request.Name}\" is invalid"); } diff --git a/ErsatzTV.Application/Scheduling/Commands/CreateBlockHandler.cs b/ErsatzTV.Application/Scheduling/Commands/CreateBlockHandler.cs index 2fa2f6b2f..87b79d865 100644 --- a/ErsatzTV.Application/Scheduling/Commands/CreateBlockHandler.cs +++ b/ErsatzTV.Application/Scheduling/Commands/CreateBlockHandler.cs @@ -53,7 +53,7 @@ public class CreateBlockHandler(IDbContextFactory dbContextFactory) TvContext dbContext, CreateBlock request) { - if (request.Name.Length > 50) + if (string.IsNullOrWhiteSpace(request.Name) || request.Name.Length > 50) { return BaseError.New($"Block name \"{request.Name}\" is invalid"); } diff --git a/ErsatzTV.Application/Scheduling/Commands/CreateDecoHandler.cs b/ErsatzTV.Application/Scheduling/Commands/CreateDecoHandler.cs index 871bac52e..8cc2cbe79 100644 --- a/ErsatzTV.Application/Scheduling/Commands/CreateDecoHandler.cs +++ b/ErsatzTV.Application/Scheduling/Commands/CreateDecoHandler.cs @@ -55,7 +55,7 @@ public class CreateDecoHandler(IDbContextFactory dbContextFactory) TvContext dbContext, CreateDeco request) { - if (request.Name.Length > 50) + if (string.IsNullOrWhiteSpace(request.Name) || request.Name.Length > 50) { return BaseError.New($"Deco name \"{request.Name}\" is invalid"); } diff --git a/ErsatzTV.Application/Scheduling/Commands/CreateDecoTemplateHandler.cs b/ErsatzTV.Application/Scheduling/Commands/CreateDecoTemplateHandler.cs index 226140590..6e3773934 100644 --- a/ErsatzTV.Application/Scheduling/Commands/CreateDecoTemplateHandler.cs +++ b/ErsatzTV.Application/Scheduling/Commands/CreateDecoTemplateHandler.cs @@ -56,7 +56,7 @@ public class CreateDecoTemplateHandler(IDbContextFactory dbContextFac TvContext dbContext, CreateDecoTemplate request) { - if (request.Name.Length > 50) + if (string.IsNullOrWhiteSpace(request.Name) || request.Name.Length > 50) { return BaseError.New($"Deco template name \"{request.Name}\" is invalid"); } diff --git a/ErsatzTV.Application/Scheduling/Commands/CreateTemplateHandler.cs b/ErsatzTV.Application/Scheduling/Commands/CreateTemplateHandler.cs index 6eeabe6a2..3a1439481 100644 --- a/ErsatzTV.Application/Scheduling/Commands/CreateTemplateHandler.cs +++ b/ErsatzTV.Application/Scheduling/Commands/CreateTemplateHandler.cs @@ -52,7 +52,7 @@ public class CreateTemplateHandler(IDbContextFactory dbContextFactory TvContext dbContext, CreateTemplate request) { - if (request.Name.Length > 50) + if (string.IsNullOrWhiteSpace(request.Name) || request.Name.Length > 50) { return BaseError.New($"Template name \"{request.Name}\" is invalid"); } diff --git a/ErsatzTV.Application/Scheduling/Commands/ReplaceBlockItemsHandler.cs b/ErsatzTV.Application/Scheduling/Commands/ReplaceBlockItemsHandler.cs index 1bedfbc3f..3e1c8d8ba 100644 --- a/ErsatzTV.Application/Scheduling/Commands/ReplaceBlockItemsHandler.cs +++ b/ErsatzTV.Application/Scheduling/Commands/ReplaceBlockItemsHandler.cs @@ -202,7 +202,7 @@ public class ReplaceBlockItemsHandler(IDbContextFactory dbContextFact Block block, ReplaceBlockItems request) { - if (request.Name.Length > 50) + if (string.IsNullOrWhiteSpace(request.Name) || request.Name.Length > 50) { return BaseError.New($"Block name \"{request.Name}\" is invalid"); } diff --git a/ErsatzTV.Application/Scheduling/Commands/ReplaceDecoTemplateItemsHandler.cs b/ErsatzTV.Application/Scheduling/Commands/ReplaceDecoTemplateItemsHandler.cs index d2a26586c..470f165eb 100644 --- a/ErsatzTV.Application/Scheduling/Commands/ReplaceDecoTemplateItemsHandler.cs +++ b/ErsatzTV.Application/Scheduling/Commands/ReplaceDecoTemplateItemsHandler.cs @@ -191,7 +191,7 @@ public class ReplaceDecoTemplateItemsHandler( DecoTemplate decoTemplate, ReplaceDecoTemplateItems request) { - if (request.Name.Length > 50) + if (string.IsNullOrWhiteSpace(request.Name) || request.Name.Length > 50) { return BaseError.New($"Deco template name \"{request.Name}\" is invalid"); } diff --git a/ErsatzTV.Application/Scheduling/Commands/ReplaceTemplateItemsHandler.cs b/ErsatzTV.Application/Scheduling/Commands/ReplaceTemplateItemsHandler.cs index 1313f81e7..bdd722980 100644 --- a/ErsatzTV.Application/Scheduling/Commands/ReplaceTemplateItemsHandler.cs +++ b/ErsatzTV.Application/Scheduling/Commands/ReplaceTemplateItemsHandler.cs @@ -120,15 +120,21 @@ public class ReplaceTemplateItemsHandler(IDbContextFactory dbContextF } } - foreach (BlockTemplateItem item in allTemplateItems) + // Compare by index, not value: BlockTemplateItem is a record, so two identical items would be + // value-equal and skipped by an `item == otherItem` guard, letting exact duplicates persist + // unvalidated (issue #172). Index comparison compares every distinct position, so duplicates + // register as a (self-)intersection and are rejected. + for (var i = 0; i < allTemplateItems.Count; i++) { - foreach (BlockTemplateItem otherItem in allTemplateItems) + BlockTemplateItem item = allTemplateItems[i]; + for (var j = 0; j < allTemplateItems.Count; j++) { - if (item == otherItem) + if (i == j) { continue; } + BlockTemplateItem otherItem = allTemplateItems[j]; if (item.StartTime < otherItem.EndTime && otherItem.StartTime < item.EndTime) { return BaseError.New( @@ -154,7 +160,7 @@ public class ReplaceTemplateItemsHandler(IDbContextFactory dbContextF Template template, ReplaceTemplateItems request) { - if (request.Name.Length > 50) + if (string.IsNullOrWhiteSpace(request.Name) || request.Name.Length > 50) { return BaseError.New($"Template name \"{request.Name}\" is invalid"); } diff --git a/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs b/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs index 95fe7a55c..dd88739a3 100644 --- a/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs +++ b/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs @@ -236,7 +236,7 @@ public class UpdateDecoHandler( TvContext dbContext, UpdateDeco request) { - if (request.Name.Length > 50) + if (string.IsNullOrWhiteSpace(request.Name) || request.Name.Length > 50) { return BaseError.New($"Deco name \"{request.Name}\" is invalid"); } @@ -316,7 +316,7 @@ public class UpdateDecoHandler( case CollectionType.Playlist: if (breakContent.PlaylistId is null) { - return BaseError.New("Break content must have valid playlist"); + return BaseError.New("Break content must have valid playlist"); } break; diff --git a/ErsatzTV.Tests/Application/FFmpegProfiles/FFmpegProfileHandlerTests.cs b/ErsatzTV.Tests/Application/FFmpegProfiles/FFmpegProfileHandlerTests.cs index b8ee3b71f..8c61350fc 100644 --- a/ErsatzTV.Tests/Application/FFmpegProfiles/FFmpegProfileHandlerTests.cs +++ b/ErsatzTV.Tests/Application/FFmpegProfiles/FFmpegProfileHandlerTests.cs @@ -80,9 +80,34 @@ public class FFmpegProfileHandlerTests LeftOf(result).ShouldBeOfType(); } + [Test] + public async Task Update_Should_Reject_Null_Name() + { + // Issue #172 FIX A: the handler guarded a client-nullable Name with a bare Name.Length + // check, so a null name threw a NullReferenceException (HTTP 500). The guard now returns a + // validation failure (Left) instead of throwing. + await SeedProfile(1); + await SeedResolution(1); + var handler = new UpdateFFmpegProfileHandler(_db.Factory, _searchTargets); + + // Resolution seeded so the resolution-exists gate passes and the null name is the sole + // failure: the guard must return a Left (validation failure) rather than throwing an NRE. + Either result = + await handler.Handle(MakeUpdate(1) with { Name = null! }, CancellationToken.None); + + LeftOf(result).ShouldBeAssignableTo(); + } + private static BaseError LeftOf(Either either) => either.Match(Left: e => e, Right: _ => throw new AssertionException("Expected a Left result")); + private async Task SeedResolution(int id) + { + await using TvContext context = _db.CreateContext(); + context.Resolutions.Add(new Resolution { Id = id, Name = "1920x1080", Width = 1920, Height = 1080 }); + await context.SaveChangesAsync(); + } + private async Task SeedProfile(int id) { await using TvContext context = _db.CreateContext(); diff --git a/ErsatzTV.Tests/Application/Scheduling/Issue172HardeningTests.cs b/ErsatzTV.Tests/Application/Scheduling/Issue172HardeningTests.cs new file mode 100644 index 000000000..9b551508a --- /dev/null +++ b/ErsatzTV.Tests/Application/Scheduling/Issue172HardeningTests.cs @@ -0,0 +1,316 @@ +using System.Threading.Channels; +using ErsatzTV.Application; +using ErsatzTV.Application.MediaCollections; +using ErsatzTV.Application.Scheduling; +using ErsatzTV.Core; +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Domain.Scheduling; +using ErsatzTV.Infrastructure.Data; +using ErsatzTV.Tests.Support; +using LanguageExt; +using Microsoft.EntityFrameworkCore; +using NSubstitute; +using NUnit.Framework; +using Shouldly; + +namespace ErsatzTV.Tests.Application.Scheduling; + +/// +/// Regression tests for issue #172 (API hardening). +/// FIX A: ten command handlers guarded a client-nullable Name with a bare +/// request.Name.Length > 50, so a null/whitespace name threw a NullReferenceException +/// that surfaced as HTTP 500. The guard is now +/// string.IsNullOrWhiteSpace(request.Name) || request.Name.Length > 50, which returns a +/// validation failure (Left) instead of throwing. +/// FIX B: overlap validation used to skip +/// value-equal items (item == otherItem on a record), which let two exact-duplicate +/// items bypass the intersection check and both persist. Validation now iterates by index, so +/// duplicates register as a self-intersection and are rejected. +/// +[TestFixture] +public class Issue172HardeningTests +{ + private InMemoryTvContext _db = null!; + + [SetUp] + public async Task SetUp() => _db = await InMemoryTvContext.CreateAsync(); + + [TearDown] + public async Task TearDown() => await _db.DisposeAsync(); + + private static bool IsLeft(Either result) => result.Match(Right: _ => false, Left: _ => true); + + // ---- FIX B: duplicate template items must be rejected ------------------------------------- + + [Test] + public async Task ReplaceTemplateItems_Should_Reject_Duplicate_Items() + { + // Block Minutes=30 gives each item a non-zero duration (0:00..0:30) so the two identical + // items overlap. Under the OLD value-equality skip the two records were `==` and skipped, + // letting both persist unvalidated; the index-based check now flags the self-intersection. + await using (TvContext seed = _db.CreateContext()) + { + seed.Blocks.Add( + new Block + { + Id = 10, + BlockGroupId = 1, + Name = "Morning", + Minutes = 30, + StopScheduling = BlockStopScheduling.AfterDurationEnd, + Items = new List() + }); + seed.Templates.Add( + new Template + { + Id = 1, + TemplateGroupId = 1, + Name = "Weekday", + Version = 1, + Items = new List() + }); + await seed.SaveChangesAsync(); + } + + var handler = new ReplaceTemplateItemsHandler(_db.Factory); + + var duplicate = new ReplaceTemplateItem(10, TimeSpan.Zero); + var request = new ReplaceTemplateItems( + 1, + 1, + "Weekday", + new List { duplicate, duplicate }, + ExpectedVersions: default); // None -> force-write path + + Either> result = + await handler.Handle(request, CancellationToken.None); + + IsLeft(result).ShouldBeTrue(); + + // Non-vacuous: nothing was persisted (the old code would have written both items). + await using TvContext ctx = _db.CreateContext(); + (await ctx.TemplateItems.CountAsync(i => i.TemplateId == 1)).ShouldBe(0); + } + + // ---- FIX A: null/whitespace name returns Left instead of throwing ------------------------- + + [TestCase(null)] + [TestCase(" ")] + public async Task CreateBlock_Should_Reject_Blank_Name(string? name) + { + await SeedGroupAsync(g => g.BlockGroups.Add(new BlockGroup { Id = 1, Name = "G", Blocks = new List() })); + + var handler = new CreateBlockHandler(_db.Factory); + Either result = + await handler.Handle(new CreateBlock(1, name!), CancellationToken.None); + + IsLeft(result).ShouldBeTrue(); + } + + [TestCase(null)] + [TestCase(" ")] + public async Task CreateTemplate_Should_Reject_Blank_Name(string? name) + { + await SeedGroupAsync(g => + g.TemplateGroups.Add(new TemplateGroup { Id = 1, Name = "G", Templates = new List