From ac7965dee4d9ed9eea0ebddd84c7766743a79589 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 18 Jul 2026 13:04:05 +0200 Subject: [PATCH] feat(293): paginate GET /api/v1/search/all-items to cap DoS exposure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The all-items endpoint fired ten index searches with limit:0 (every hit), so a broad authenticated query materialized the whole index into one response. Add optional pageNum/pageSize (clamped 1..1000; pageNum 0..2_000_000 so skip can't overflow int) and an additive per-kind Totals on the response; the SPA add-all flow now pages to completeness instead of a single unbounded fetch. - SearchController.SearchAllItems: clamp params (Logs §1 precedent), map Totals - QuerySearchIndexAllItemsHandler: skip=pageNum*pageSize, limit=pageSize, read SearchResult.TotalCount per kind - SearchResultAllItemsResponseModel: additive Totals (frozen-v1-safe) - web/src/api/search.ts: getSearchAllItems paging params + getAllSearchItemIds (pages until each kind hits its total; empty-page safety break) - tests: controller clamp/thread/totals, handler skip/limit/totals, SPA paging - docs: decisions.md 2026-07-18 (#293), api-conventions.md §5; regenerated OpenAPI Design: issue option (a) full pagination, operator-confirmed. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Queries/QuerySearchIndexAllItems.cs | 5 +- .../QuerySearchIndexAllItemsHandler.cs | 83 ++++++++++--- .../Search/SearchResultAllItemsViewModel.cs | 17 ++- .../SearchResultAllItemsResponseModel.cs | 3 +- ...SearchResultAllItemsTotalsResponseModel.cs | 17 +++ .../QuerySearchIndexAllItemsHandlerTests.cs | 78 ++++++++++++ .../Controllers/SearchControllerTests.cs | 50 +++++++- ErsatzTV/Controllers/Api/SearchController.cs | 35 +++++- ErsatzTV/wwwroot/openapi/v1.json | 83 ++++++++++++- docs/api-conventions.md | 11 ++ docs/decisions.md | 40 ++++++ web/src/api/generated/v1.d.ts | 13 ++ web/src/api/search.test.ts | 114 +++++++++++++++++- web/src/api/search.ts | 109 ++++++++++++++++- web/src/screens/SearchScreen.test.tsx | 15 ++- web/src/screens/SearchScreen.tsx | 4 +- 16 files changed, 637 insertions(+), 40 deletions(-) create mode 100644 ErsatzTV.Core/Api/Search/SearchResultAllItemsTotalsResponseModel.cs create mode 100644 ErsatzTV.Tests/Application/Search/QuerySearchIndexAllItemsHandlerTests.cs diff --git a/ErsatzTV.Application/Search/Queries/QuerySearchIndexAllItems.cs b/ErsatzTV.Application/Search/Queries/QuerySearchIndexAllItems.cs index 1b53c6a47..21e45d6ef 100644 --- a/ErsatzTV.Application/Search/Queries/QuerySearchIndexAllItems.cs +++ b/ErsatzTV.Application/Search/Queries/QuerySearchIndexAllItems.cs @@ -1,3 +1,4 @@ -namespace ErsatzTV.Application.Search; +namespace ErsatzTV.Application.Search; -public record QuerySearchIndexAllItems(string Query) : IRequest; +public record QuerySearchIndexAllItems(string Query, int PageNum, int PageSize) + : IRequest; diff --git a/ErsatzTV.Application/Search/Queries/QuerySearchIndexAllItemsHandler.cs b/ErsatzTV.Application/Search/Queries/QuerySearchIndexAllItemsHandler.cs index 86a270217..8c84adcf7 100644 --- a/ErsatzTV.Application/Search/Queries/QuerySearchIndexAllItemsHandler.cs +++ b/ErsatzTV.Application/Search/Queries/QuerySearchIndexAllItemsHandler.cs @@ -1,4 +1,5 @@ -using ErsatzTV.Core.Interfaces.Search; +using ErsatzTV.Core.Interfaces.Search; +using ErsatzTV.Core.Search; using ErsatzTV.Infrastructure.Search; namespace ErsatzTV.Application.Search; @@ -8,21 +9,69 @@ public class QuerySearchIndexAllItemsHandler(ISearchIndex searchIndex) { public async Task Handle( QuerySearchIndexAllItems request, - CancellationToken cancellationToken) => - new( - await GetIds(LuceneSearchIndex.MovieType, request.Query, cancellationToken), - await GetIds(LuceneSearchIndex.ShowType, request.Query, cancellationToken), - await GetIds(LuceneSearchIndex.SeasonType, request.Query, cancellationToken), - await GetIds(LuceneSearchIndex.EpisodeType, request.Query, cancellationToken), - await GetIds(LuceneSearchIndex.ArtistType, request.Query, cancellationToken), - await GetIds(LuceneSearchIndex.MusicVideoType, request.Query, cancellationToken), - await GetIds(LuceneSearchIndex.OtherVideoType, request.Query, cancellationToken), - await GetIds(LuceneSearchIndex.SongType, request.Query, cancellationToken), - await GetIds(LuceneSearchIndex.ImageType, request.Query, cancellationToken), - await GetIds(LuceneSearchIndex.RemoteStreamType, request.Query, cancellationToken)); + CancellationToken cancellationToken) + { + int skip = request.PageNum * request.PageSize; + int limit = request.PageSize; - private async Task> GetIds(string type, string query, CancellationToken cancellationToken) => - (await searchIndex.Search($"type:{type} AND ({query})", string.Empty, 0, 0, cancellationToken)).Items - .Map(i => i.Id) - .ToList(); + (List Ids, int Total) movies = + await GetIds(LuceneSearchIndex.MovieType, request.Query, skip, limit, cancellationToken); + (List Ids, int Total) shows = + await GetIds(LuceneSearchIndex.ShowType, request.Query, skip, limit, cancellationToken); + (List Ids, int Total) seasons = + await GetIds(LuceneSearchIndex.SeasonType, request.Query, skip, limit, cancellationToken); + (List Ids, int Total) episodes = + await GetIds(LuceneSearchIndex.EpisodeType, request.Query, skip, limit, cancellationToken); + (List Ids, int Total) artists = + await GetIds(LuceneSearchIndex.ArtistType, request.Query, skip, limit, cancellationToken); + (List Ids, int Total) musicVideos = + await GetIds(LuceneSearchIndex.MusicVideoType, request.Query, skip, limit, cancellationToken); + (List Ids, int Total) otherVideos = + await GetIds(LuceneSearchIndex.OtherVideoType, request.Query, skip, limit, cancellationToken); + (List Ids, int Total) songs = + await GetIds(LuceneSearchIndex.SongType, request.Query, skip, limit, cancellationToken); + (List Ids, int Total) images = + await GetIds(LuceneSearchIndex.ImageType, request.Query, skip, limit, cancellationToken); + (List Ids, int Total) remoteStreams = + await GetIds(LuceneSearchIndex.RemoteStreamType, request.Query, skip, limit, cancellationToken); + + return new SearchResultAllItemsViewModel( + movies.Ids, + shows.Ids, + seasons.Ids, + episodes.Ids, + artists.Ids, + musicVideos.Ids, + otherVideos.Ids, + songs.Ids, + images.Ids, + remoteStreams.Ids, + new SearchResultAllItemsTotals( + movies.Total, + shows.Total, + seasons.Total, + episodes.Total, + artists.Total, + musicVideos.Total, + otherVideos.Total, + songs.Total, + images.Total, + remoteStreams.Total)); + } + + private async Task<(List Ids, int Total)> GetIds( + string type, + string query, + int skip, + int limit, + CancellationToken cancellationToken) + { + SearchResult result = await searchIndex.Search( + $"type:{type} AND ({query})", + string.Empty, + skip, + limit, + cancellationToken); + return (result.Items.Map(i => i.Id).ToList(), result.TotalCount); + } } diff --git a/ErsatzTV.Application/Search/SearchResultAllItemsViewModel.cs b/ErsatzTV.Application/Search/SearchResultAllItemsViewModel.cs index 0b0f69fdd..ed9e4dca8 100644 --- a/ErsatzTV.Application/Search/SearchResultAllItemsViewModel.cs +++ b/ErsatzTV.Application/Search/SearchResultAllItemsViewModel.cs @@ -1,4 +1,4 @@ -namespace ErsatzTV.Application.Search; +namespace ErsatzTV.Application.Search; public record SearchResultAllItemsViewModel( List MovieIds, @@ -10,4 +10,17 @@ public record SearchResultAllItemsViewModel( List OtherVideoIds, List SongIds, List ImageIds, - List RemoteStreamIds); + List RemoteStreamIds, + SearchResultAllItemsTotals Totals); + +public record SearchResultAllItemsTotals( + int MovieCount, + int ShowCount, + int SeasonCount, + int EpisodeCount, + int ArtistCount, + int MusicVideoCount, + int OtherVideoCount, + int SongCount, + int ImageCount, + int RemoteStreamCount); diff --git a/ErsatzTV.Core/Api/Search/SearchResultAllItemsResponseModel.cs b/ErsatzTV.Core/Api/Search/SearchResultAllItemsResponseModel.cs index aa7ca3e91..46a3ad48b 100644 --- a/ErsatzTV.Core/Api/Search/SearchResultAllItemsResponseModel.cs +++ b/ErsatzTV.Core/Api/Search/SearchResultAllItemsResponseModel.cs @@ -11,4 +11,5 @@ public record SearchResultAllItemsResponseModel( List OtherVideoIds, List SongIds, List ImageIds, - List RemoteStreamIds); + List RemoteStreamIds, + SearchResultAllItemsTotalsResponseModel Totals); diff --git a/ErsatzTV.Core/Api/Search/SearchResultAllItemsTotalsResponseModel.cs b/ErsatzTV.Core/Api/Search/SearchResultAllItemsTotalsResponseModel.cs new file mode 100644 index 000000000..ad56d4775 --- /dev/null +++ b/ErsatzTV.Core/Api/Search/SearchResultAllItemsTotalsResponseModel.cs @@ -0,0 +1,17 @@ +#nullable enable +namespace ErsatzTV.Core.Api.Search; + +// Per-kind total hit counts for a search all-items query, independent of the current page. Lets a +// client page GET /api/v1/search/all-items to completeness (the SPA "add all" flow) instead of +// materializing every id in one unbounded response (issue #293). +public record SearchResultAllItemsTotalsResponseModel( + int MovieCount, + int ShowCount, + int SeasonCount, + int EpisodeCount, + int ArtistCount, + int MusicVideoCount, + int OtherVideoCount, + int SongCount, + int ImageCount, + int RemoteStreamCount); diff --git a/ErsatzTV.Tests/Application/Search/QuerySearchIndexAllItemsHandlerTests.cs b/ErsatzTV.Tests/Application/Search/QuerySearchIndexAllItemsHandlerTests.cs new file mode 100644 index 000000000..ee6cf9f78 --- /dev/null +++ b/ErsatzTV.Tests/Application/Search/QuerySearchIndexAllItemsHandlerTests.cs @@ -0,0 +1,78 @@ +using ErsatzTV.Application.Search; +using ErsatzTV.Core.Interfaces.Search; +using ErsatzTV.Core.Search; +using NSubstitute; +using NUnit.Framework; +using Shouldly; + +namespace ErsatzTV.Tests.Application.Search; + +[TestFixture] +public class QuerySearchIndexAllItemsHandlerTests +{ + private ISearchIndex _searchIndex = null!; + private QuerySearchIndexAllItemsHandler _handler = null!; + + [SetUp] + public void SetUp() + { + _searchIndex = Substitute.For(); + + // Default every kind's search to an empty page so unconfigured kinds don't NRE. + _searchIndex.Search( + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Any()) + .Returns(new SearchResult([], 0)); + + _handler = new QuerySearchIndexAllItemsHandler(_searchIndex); + } + + [Test] + public async Task Handle_Should_Translate_Page_To_Skip_And_Limit() + { + await _handler.Handle(new QuerySearchIndexAllItems("star", 2, 50), CancellationToken.None); + + // Every one of the ten media kinds is queried with skip = pageNum * pageSize, limit = pageSize. + await _searchIndex.Received(10).Search( + Arg.Any(), + string.Empty, + 100, + 50, + Arg.Any()); + await _searchIndex.Received(1).Search( + Arg.Is(q => q.Contains("type:movie")), + string.Empty, + 100, + 50, + Arg.Any()); + } + + [Test] + public async Task Handle_Should_Return_Ids_And_Per_Kind_Totals() + { + _searchIndex.Search( + Arg.Is(q => q.Contains("type:movie")), + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Any()) + .Returns(new SearchResult( + [new SearchItem("movie", 1), new SearchItem("movie", 2)], + 7)); + + SearchResultAllItemsViewModel result = + await _handler.Handle(new QuerySearchIndexAllItems("star", 0, 500), CancellationToken.None); + + // The bucket holds this page's ids; the total reflects the full hit count (SearchResult.TotalCount), + // which is what lets a client page to completeness. + result.MovieIds.ShouldBe(new List { 1, 2 }); + result.Totals.MovieCount.ShouldBe(7); + + // A kind with no hits returns an empty bucket and a zero total. + result.ShowIds.ShouldBeEmpty(); + result.Totals.ShowCount.ShouldBe(0); + } +} diff --git a/ErsatzTV.Tests/Controllers/SearchControllerTests.cs b/ErsatzTV.Tests/Controllers/SearchControllerTests.cs index d2b393bf5..5eb4538e0 100644 --- a/ErsatzTV.Tests/Controllers/SearchControllerTests.cs +++ b/ErsatzTV.Tests/Controllers/SearchControllerTests.cs @@ -107,7 +107,7 @@ public class SearchControllerTests [Test] public async Task SearchAllItems_Should_Return_422_For_Empty_Query() { - IActionResult result = await _controller.SearchAllItems(" ", CancellationToken.None); + IActionResult result = await _controller.SearchAllItems(" ", 0, 500, CancellationToken.None); var unprocessable = result.ShouldBeOfType(); unprocessable.StatusCode.ShouldBe(422); @@ -115,7 +115,32 @@ public class SearchControllerTests } [Test] - public async Task SearchAllItems_Should_Map_Id_Lists() + public async Task SearchAllItems_Should_Clamp_Paging_And_Thread_PageNum() + { + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(EmptyAllItems()); + + // A valid pageNum threads through; a pageSize above the max clamps to 1000 (issue #293). + await _controller.SearchAllItems("star", 2, 50_000, CancellationToken.None); + await _mediator.Received(1).Send( + Arg.Is(q => q.Query == "star" && q.PageNum == 2 && q.PageSize == 1000), + Arg.Any()); + + // A negative pageNum clamps to 0; a pageSize below 1 clamps to 1. + await _controller.SearchAllItems("star", -3, 0, CancellationToken.None); + await _mediator.Received(1).Send( + Arg.Is(q => q.Query == "star" && q.PageNum == 0 && q.PageSize == 1), + Arg.Any()); + + // An absurd pageNum clamps to the max so pageNum * pageSize can't overflow int (issue #293 hardening). + await _controller.SearchAllItems("star", 9_999_999, 1000, CancellationToken.None); + await _mediator.Received(1).Send( + Arg.Is(q => q.Query == "star" && q.PageNum == 2_000_000 && q.PageSize == 1000), + Arg.Any()); + } + + [Test] + public async Task SearchAllItems_Should_Map_Id_Lists_And_Totals() { _mediator.Send(Arg.Any(), Arg.Any()) .Returns(new SearchResultAllItemsViewModel( @@ -128,14 +153,17 @@ public class SearchControllerTests [], [], [], - [])); + [], + new SearchResultAllItemsTotals(5, 1, 0, 0, 0, 0, 0, 0, 0, 0))); - IActionResult result = await _controller.SearchAllItems("star", CancellationToken.None); + IActionResult result = await _controller.SearchAllItems("star", 0, 500, CancellationToken.None); var body = result.ShouldBeOfType().Value .ShouldBeOfType(); body.MovieIds.ShouldBe(new List { 1, 2 }); body.ShowIds.ShouldBe(new List { 3 }); + body.Totals.MovieCount.ShouldBe(5); + body.Totals.ShowCount.ShouldBe(1); await _mediator.Received(1).Send( Arg.Is(q => q.Query == "star"), Arg.Any()); @@ -193,4 +221,18 @@ public class SearchControllerTests var empty = new SearchResultGroupResponseModel(0, []); return new SearchResultsResponseModel(empty, empty, empty, empty, empty, empty, empty, empty, empty, empty); } + + private static SearchResultAllItemsViewModel EmptyAllItems() => + new( + [], + [], + [], + [], + [], + [], + [], + [], + [], + [], + new SearchResultAllItemsTotals(0, 0, 0, 0, 0, 0, 0, 0, 0, 0)); } diff --git a/ErsatzTV/Controllers/Api/SearchController.cs b/ErsatzTV/Controllers/Api/SearchController.cs index 6a833bbfc..217613005 100644 --- a/ErsatzTV/Controllers/Api/SearchController.cs +++ b/ErsatzTV/Controllers/Api/SearchController.cs @@ -17,6 +17,16 @@ public class SearchController(IMediator mediator) : ControllerBase { private const int MaxPageSize = 100; + // all-items returns bare ids (cheap), so it tolerates a larger page than the item-returning + // /search endpoint; the clamp still bounds one response to <= 10 kinds * MaxAllItemsPageSize ids + // so a broad query can't materialize the whole index in one request (issue #293). + private const int DefaultAllItemsPageSize = 500; + private const int MaxAllItemsPageSize = 1000; + + // Upper-bound the page number so pageNum * pageSize (the search skip) can't overflow int and 500 — + // no legitimate client pages past this, and it keeps skip + limit inside int range at MaxAllItemsPageSize. + private const int MaxAllItemsPageNum = 2_000_000; + [HttpGet("/api/v1/search", Name = "Search")] [Tags("Search")] [EndpointSummary("Search library items across all media kinds")] @@ -46,13 +56,17 @@ public class SearchController(IMediator mediator) : ControllerBase [Tags("Search")] [EndpointSummary("Search library items across all media kinds and return raw id lists")] [EndpointDescription( - "Returns every matching item's id, grouped by media kind, with no paging. Used by the SPA's " + - "\"add all to collection/playlist\" flow to materialize ids before calling the add endpoints.")] + "Returns matching item ids, grouped by media kind, one clamped page per kind plus per-kind " + + "total counts. Used by the SPA's \"add all to collection/playlist\" flow, which pages to " + + "completeness. Paging bounds a broad query so it can't materialize the whole index in one " + + "request (issue #293).")] [EndpointGroupName("general")] [ProducesResponseType(typeof(SearchResultAllItemsResponseModel), StatusCodes.Status200OK)] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status422UnprocessableEntity)] public async Task SearchAllItems( [FromQuery] string query = "", + [FromQuery] int pageNum = 0, + [FromQuery] int pageSize = DefaultAllItemsPageSize, CancellationToken cancellationToken = default) { if (string.IsNullOrWhiteSpace(query)) @@ -60,8 +74,10 @@ public class SearchController(IMediator mediator) : ControllerBase return BaseError.New("A non-empty query is required").ToErrorResult(); } + int clampedPageNum = Math.Clamp(pageNum, 0, MaxAllItemsPageNum); + int clampedPageSize = Math.Clamp(pageSize, 1, MaxAllItemsPageSize); SearchResultAllItemsViewModel result = await mediator.Send( - new QuerySearchIndexAllItems(query), + new QuerySearchIndexAllItems(query, clampedPageNum, clampedPageSize), cancellationToken); return new OkObjectResult(Project(result)); } @@ -184,5 +200,16 @@ public class SearchController(IMediator mediator) : ControllerBase vm.OtherVideoIds, vm.SongIds, vm.ImageIds, - vm.RemoteStreamIds); + vm.RemoteStreamIds, + new SearchResultAllItemsTotalsResponseModel( + vm.Totals.MovieCount, + vm.Totals.ShowCount, + vm.Totals.SeasonCount, + vm.Totals.EpisodeCount, + vm.Totals.ArtistCount, + vm.Totals.MusicVideoCount, + vm.Totals.OtherVideoCount, + vm.Totals.SongCount, + vm.Totals.ImageCount, + vm.Totals.RemoteStreamCount)); } diff --git a/ErsatzTV/wwwroot/openapi/v1.json b/ErsatzTV/wwwroot/openapi/v1.json index 615fd8691..00df8e51a 100644 --- a/ErsatzTV/wwwroot/openapi/v1.json +++ b/ErsatzTV/wwwroot/openapi/v1.json @@ -17419,7 +17419,7 @@ "Search" ], "summary": "Search library items across all media kinds and return raw id lists", - "description": "Returns every matching item's id, grouped by media kind, with no paging. Used by the SPA's \"add all to collection/playlist\" flow to materialize ids before calling the add endpoints.", + "description": "Returns matching item ids, grouped by media kind, one clamped page per kind plus per-kind total counts. Used by the SPA's \"add all to collection/playlist\" flow, which pages to completeness. Paging bounds a broad query so it can't materialize the whole index in one request (issue #293).", "operationId": "SearchAllItems", "parameters": [ { @@ -17429,6 +17429,24 @@ "type": "string", "default": "" } + }, + { + "name": "pageNum", + "in": "query", + "schema": { + "type": "integer", + "format": "int32", + "default": 0 + } + }, + { + "name": "pageSize", + "in": "query", + "schema": { + "type": "integer", + "format": "int32", + "default": 500 + } } ], "responses": { @@ -30463,7 +30481,8 @@ "otherVideoIds", "songIds", "imageIds", - "remoteStreamIds" + "remoteStreamIds", + "totals" ], "type": "object", "properties": { @@ -30536,6 +30555,66 @@ "type": "integer", "format": "int32" } + }, + "totals": { + "$ref": "#/components/schemas/SearchResultAllItemsTotalsResponseModel" + } + } + }, + "SearchResultAllItemsTotalsResponseModel": { + "required": [ + "movieCount", + "showCount", + "seasonCount", + "episodeCount", + "artistCount", + "musicVideoCount", + "otherVideoCount", + "songCount", + "imageCount", + "remoteStreamCount" + ], + "type": "object", + "properties": { + "movieCount": { + "type": "integer", + "format": "int32" + }, + "showCount": { + "type": "integer", + "format": "int32" + }, + "seasonCount": { + "type": "integer", + "format": "int32" + }, + "episodeCount": { + "type": "integer", + "format": "int32" + }, + "artistCount": { + "type": "integer", + "format": "int32" + }, + "musicVideoCount": { + "type": "integer", + "format": "int32" + }, + "otherVideoCount": { + "type": "integer", + "format": "int32" + }, + "songCount": { + "type": "integer", + "format": "int32" + }, + "imageCount": { + "type": "integer", + "format": "int32" + }, + "remoteStreamCount": { + "type": "integer", + "format": "int32" } } }, diff --git a/docs/api-conventions.md b/docs/api-conventions.md index 864d7e9d7..1635b1c59 100644 --- a/docs/api-conventions.md +++ b/docs/api-conventions.md @@ -366,6 +366,17 @@ Returns the curated `SearchFieldCatalog` (name, friendly label, type, UI group, enum fields) as `List`. Drives the SmartCollection rule builder and is introspectable by MCP; no query parameters. +**Param + DTO expansion (#293, cap `search/all-items`)**: no new endpoint — `GET /api/v1/search/all-items` +gained two **optional** query params (`pageNum` 0-based, `pageSize` default 500, clamped 1–1000 via the §1 +Logs `Math.Clamp` precedent) so a broad query can't materialize the whole index in one response, and one +**additive** response field, `Totals` (`SearchResultAllItemsTotalsResponseModel`, ten per-kind `…Count` +ints), so a client can page to completeness. The clamp is per media kind, so one response is bounded to +≤ 10 × `pageSize` ids. The SPA add-all flow (`getAllSearchItemIds` in `web/src/api/search.ts`) pages until +each kind has collected its `Totals` count. Changing the no-param default from "everything" to one page is an +intentional, security-motivated behavior change (only in-repo consumer is the SPA, updated in the same PR; +external callers read `Totals` and page). Regenerated the OpenAPI trio. See `docs/decisions.md` 2026-07-18 +(#293). + **Resolved wart (#287)**: `DayOfWeek` previously serialized as an integer in the OpenAPI schema while the runtime JSON payload is the enum's **name string** ("Sunday".."Saturday"). It is now added to `Startup.UseStringEnumSchemas`'s hand-list, so the "v1" schema emits it as a **string enum** matching diff --git a/docs/decisions.md b/docs/decisions.md index a029f18ac..368af4dde 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -95,6 +95,7 @@ in-file entries. - [2026-07-18 — Auto-Tune DetailPanel SPA: reusable `SlideOver` + shared advanced-options model; decorative panes dropped to match the backend (#386)](#2026-07-18--auto-tune-detailpanel-spa-reusable-slideover--shared-advanced-options-model-decorative-panes-dropped-to-match-the-backend-386) - [2026-07-18 — SmartCollection rule builder: compile-only closed subset, no stored AST, one-level nesting (#176)](#2026-07-18--smartcollection-rule-builder-compile-only-closed-subset-no-stored-ast-one-level-nesting-176) - [2026-07-18 — Auto-Tune per-source weights ride #70's MultiCollection machinery; created at tune time, not a post-hoc PUT (#425)](#2026-07-18--auto-tune-per-source-weights-ride-70s-multicollection-machinery-created-at-tune-time-not-a-post-hoc-put-425) +- [2026-07-18 — Search all-items is paged to cap DoS exposure; SPA add-all pages to completeness (#293)](#2026-07-18--search-all-items-is-paged-to-cap-dos-exposure-spa-add-all-pages-to-completeness-293) --- @@ -1764,3 +1765,42 @@ isn't known until `CreateChannelFromLineup` runs; ownership is stamped immediate channels set no ownership, so their pre-existing orphan-on-delete behavior is unchanged. The create is non-atomic across the two handlers (mirrors #69) with best-effort rollback of the artifacts on channel-create failure. + +## 2026-07-18 — Search all-items is paged to cap DoS exposure; SPA add-all pages to completeness (#293) + +`GET /api/v1/search/all-items` (`SearchController.SearchAllItems` → `QuerySearchIndexAllItemsHandler`) fired +ten index searches with **`limit: 0`** (= "return every hit", `LuceneSearchIndex` line ~244), so a single +broad query (e.g. one matching the whole library) materialized *every* matching doc across all ten media +kinds into ten `List` buckets and serialized them in one response — unbounded work per request. #285 +closed the original *unauthenticated* exposure (the endpoint is now behind `Api:RequireKeyForReads`, default +true); the residual was DoS-hardening against an **authenticated** caller with a very broad query. Deferred +from #285 because the SPA "add all to collection/playlist" flow materializes the full id set before the add +POST, so a naive hard cap would silently truncate "add all". + +**Decision (issue option (a), operator-confirmed): paginate the endpoint and teach the SPA add-all flow to +page to completeness** — rather than option (b) (a generous cap + truncation signal). Chosen because +"add all" must stay complete for real use, and it matches the sibling `GET /api/v1/search` / +`GET /api/v1/channels/auto-tune/members` (#384) paging convention already in the codebase. + +- **Endpoint (additive).** `SearchAllItems` gains optional `pageNum` (0-based) + `pageSize`, clamped exactly + like the §1 Logs / sibling `Search` precedent: `pageNum = Math.Max(0, pageNum)`, + `pageSize = Math.Clamp(pageSize, 1, MaxAllItemsPageSize)` with `MaxAllItemsPageSize = 1000`, + `DefaultAllItemsPageSize = 500`. The clamp is applied per media kind (a page returns ≤ `pageSize` ids of + *each* of the ten kinds), so one response is bounded to ≤ 10 × `pageSize` ids. `QuerySearchIndexAllItems` + carries `PageNum`/`PageSize`; the handler passes `skip = PageNum × PageSize`, `limit = PageSize` into + `ISearchIndex.Search` (native skip/limit) and reads `SearchResult.TotalCount` (the true total, free) per + kind. +- **Response (additive, frozen-v1-safe).** The ten `…Ids` buckets are unchanged; a new non-null nested + `Totals` (`SearchResultAllItemsTotalsResponseModel`, ten `…Count` ints) is added so a client knows how many + ids exist per kind and can page to completeness. Nothing is removed or retyped (#286 additive-only holds). +- **Deliberate default-behavior change.** A caller that sends no `pageSize` now gets one page (default 500 / + kind) plus `Totals`, not the entire id set. This is the security change the issue asks for; it is safe here + because the only in-repo consumer is the SPA (updated in the same PR) and any external/MCP caller can read + `Totals` and page. Recorded as intentional, not a regression. +- **SPA pages to completeness.** `web/src/api/search.ts` `getSearchAllItems(query, pageNum, pageSize)` gains + the params; a new `getAllSearchItemIds(query)` loops pages (requesting `pageSize = 1000`, the server max), + accumulating every bucket until each kind has collected its `Totals` count (with an empty-page safety break + against total-count drift), and returns the merged `SearchAllItemIds`. `SearchScreen.addAll` calls it + instead of the single-shot fetch; the #221 stale-query guard and the single add POST are unchanged. +- **Out of scope (unchanged):** the add POST itself still accepts the full merged id set in one request body + — bounding *that* surface is a separate concern (see #308 for the add path); #293 is the GET. diff --git a/web/src/api/generated/v1.d.ts b/web/src/api/generated/v1.d.ts index e03297a75..eafeced82 100644 --- a/web/src/api/generated/v1.d.ts +++ b/web/src/api/generated/v1.d.ts @@ -1454,6 +1454,19 @@ export interface components { "songIds": Array; "imageIds": Array; "remoteStreamIds": Array; + "totals": components["schemas"]["SearchResultAllItemsTotalsResponseModel"]; + }; + "SearchResultAllItemsTotalsResponseModel": { + "movieCount": number; + "showCount": number; + "seasonCount": number; + "episodeCount": number; + "artistCount": number; + "musicVideoCount": number; + "otherVideoCount": number; + "songCount": number; + "imageCount": number; + "remoteStreamCount": number; }; "SearchResultGroupResponseModel": { "totalCount": number; diff --git a/web/src/api/search.test.ts b/web/src/api/search.test.ts index 5a1d28c9c..9bf38d5cf 100644 --- a/web/src/api/search.test.ts +++ b/web/src/api/search.test.ts @@ -1,5 +1,41 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; -import { getSearchAllItems, getSearchFields, getSearchResults, toAddItemsRequestFromSearch } from './search'; +import { + getAllSearchItemIds, + getSearchAllItems, + getSearchFields, + getSearchResults, + toAddItemsRequestFromSearch +} from './search'; + +// Builds an all-items page response body. `totals` fills unspecified counts with 0. +function allItemsPage( + buckets: Record, + totals: Record = {} +): Record { + return { + ...buckets, + totals: { + movieCount: 0, + showCount: 0, + seasonCount: 0, + episodeCount: 0, + artistCount: 0, + musicVideoCount: 0, + otherVideoCount: 0, + songCount: 0, + imageCount: 0, + remoteStreamCount: 0, + ...totals + } + }; +} + +function jsonResponse(body: unknown): Response { + return new Response(JSON.stringify(body), { + headers: { 'Content-Type': 'application/json' }, + status: 200 + }); +} const emptyGroup = { totalCount: 0, items: [] }; const sampleResults = { @@ -111,6 +147,82 @@ describe('getSearchFields', () => { }); }); +describe('getSearchAllItems paging params', () => { + beforeEach(() => { + window.localStorage.clear(); + vi.restoreAllMocks(); + }); + + it('includes pageNum and pageSize when provided', async () => { + const fetchSpy = vi.spyOn(window, 'fetch').mockResolvedValue(jsonResponse(allItemsPage({}))); + + await getSearchAllItems('star', 2, 1000); + + const [url] = fetchSpy.mock.calls[0]; + expect(url).toBe('/api/v1/search/all-items?query=star&pageNum=2&pageSize=1000'); + }); +}); + +describe('getAllSearchItemIds', () => { + beforeEach(() => { + window.localStorage.clear(); + vi.restoreAllMocks(); + }); + + it('returns a single page when every bucket is already complete', async () => { + const fetchSpy = vi + .spyOn(window, 'fetch') + .mockResolvedValue(jsonResponse(allItemsPage({ movieIds: [1, 2], showIds: [9] }, { movieCount: 2, showCount: 1 }))); + + const result = await getAllSearchItemIds('star'); + + expect(fetchSpy).toHaveBeenCalledTimes(1); + expect(result.movieIds).toEqual([1, 2]); + expect(result.showIds).toEqual([9]); + expect(result.seasonIds).toEqual([]); + }); + + it('pages to completeness and merges every bucket', async () => { + const fetchSpy = vi + .spyOn(window, 'fetch') + .mockResolvedValueOnce(jsonResponse(allItemsPage({ movieIds: [1, 2] }, { movieCount: 3 }))) + .mockResolvedValueOnce(jsonResponse(allItemsPage({ movieIds: [3] }, { movieCount: 3 }))); + + const result = await getAllSearchItemIds('star'); + + expect(fetchSpy).toHaveBeenCalledTimes(2); + expect(result.movieIds).toEqual([1, 2, 3]); + // The second request advances the page. + expect(fetchSpy.mock.calls[1][0]).toBe('/api/v1/search/all-items?query=star&pageNum=1&pageSize=1000'); + }); + + it('pages until an empty page when the server omits totals', async () => { + // Without `totals` the fast completeness check can't fire, so it must keep paging until empty + // rather than silently truncating to the first page. + const fetchSpy = vi + .spyOn(window, 'fetch') + .mockResolvedValueOnce(jsonResponse({ movieIds: [1, 2] })) + .mockResolvedValueOnce(jsonResponse({ movieIds: [3] })) + .mockResolvedValueOnce(jsonResponse({ movieIds: [] })); + + const result = await getAllSearchItemIds('star'); + + expect(fetchSpy).toHaveBeenCalledTimes(3); + expect(result.movieIds).toEqual([1, 2, 3]); + }); + + it('stops on an empty page even if a total claims more (drift safety)', async () => { + const fetchSpy = vi + .spyOn(window, 'fetch') + .mockResolvedValue(jsonResponse(allItemsPage({}, { movieCount: 5 }))); + + const result = await getAllSearchItemIds('star'); + + expect(fetchSpy).toHaveBeenCalledTimes(1); + expect(result.movieIds).toEqual([]); + }); +}); + describe('toAddItemsRequestFromSearch', () => { it('fills every bucket, defaulting null arrays to []', () => { expect( diff --git a/web/src/api/search.ts b/web/src/api/search.ts index 1c1d2c607..313e46ad9 100644 --- a/web/src/api/search.ts +++ b/web/src/api/search.ts @@ -19,6 +19,37 @@ export function getSearchFields(): Promise { // match AddItemsToCollectionRequest exactly, so a result pipes straight into addItemsToCollection / // addItemsToPlaylist via toAddItemsRequestFromSearch below. export type SearchAllItemIds = components['schemas']['SearchResultAllItemsResponseModel']; +type SearchAllItemTotals = SearchAllItemIds['totals']; + +// The ten id-bucket keys, paired with their per-kind total-count key in `totals`. Used to page the +// all-items endpoint to completeness (issue #293) without hard-coding the loop ten times. +const ALL_ITEM_KINDS = [ + 'movieIds', + 'showIds', + 'seasonIds', + 'episodeIds', + 'artistIds', + 'musicVideoIds', + 'otherVideoIds', + 'songIds', + 'imageIds', + 'remoteStreamIds' +] as const; + +type AllItemKind = (typeof ALL_ITEM_KINDS)[number]; + +const TOTAL_KEY: Record = { + movieIds: 'movieCount', + showIds: 'showCount', + seasonIds: 'seasonCount', + episodeIds: 'episodeCount', + artistIds: 'artistCount', + musicVideoIds: 'musicVideoCount', + otherVideoIds: 'otherVideoCount', + songIds: 'songCount', + imageIds: 'imageCount', + remoteStreamIds: 'remoteStreamCount' +}; export interface GetSearchResultsParams { query: string; @@ -36,17 +67,87 @@ export function getSearchResults(params: GetSearchResultsParams): Promise(`/api/v1/search?${searchParams.toString()}`); } -// Resolves a search query to the full set of matching media-item ids, bucketed by kind. Backs the -// "Add all results" flow so the caller never has to page through every result to add them. -export function getSearchAllItems(query: string): Promise { +// Fetches one clamped page of matching media-item ids, bucketed by kind, plus per-kind totals. The +// endpoint pages so a broad query can't materialize the whole index in one response (issue #293); +// callers that need every id use getAllSearchItemIds below. +export function getSearchAllItems(query: string, pageNum?: number, pageSize?: number): Promise { const searchParams = new URLSearchParams(); searchParams.set('query', query); + if (pageNum != null) { + searchParams.set('pageNum', String(pageNum)); + } + if (pageSize != null) { + searchParams.set('pageSize', String(pageSize)); + } return request(`/api/v1/search/all-items?${searchParams.toString()}`); } +// Resolves a search query to the FULL set of matching media-item ids by paging the all-items endpoint +// to completeness. Backs the "Add all results" flow. Requests the server's max page size and stops once +// every kind has collected its reported total (or a page comes back empty — a safety break against the +// total drifting if the index is written mid-page). +export async function getAllSearchItemIds(query: string): Promise { + const pageSize = 1000; // the server clamps to its own max; ask for the largest page + const merged: Record = { + movieIds: [], + showIds: [], + seasonIds: [], + episodeIds: [], + artistIds: [], + musicVideoIds: [], + otherVideoIds: [], + songIds: [], + imageIds: [], + remoteStreamIds: [] + }; + + let totals: SearchAllItemTotals | undefined; + let pageNum = 0; + + for (;;) { + const page = await getSearchAllItems(query, pageNum, pageSize); + totals = page.totals; + + let pageCount = 0; + for (const kind of ALL_ITEM_KINDS) { + const ids = page[kind] ?? []; + pageCount += ids.length; + merged[kind].push(...ids); + } + pageNum += 1; + + // Only trust the fast completeness check when the server sent totals; otherwise keep paging until an + // empty page so a missing `totals` can't silently truncate to the first page. (Capture in a const so + // the null-narrowing survives into the closure.) + const pageTotals = totals; + const complete = + pageTotals != null && + ALL_ITEM_KINDS.every((kind) => merged[kind].length >= (pageTotals[TOTAL_KEY[kind]] ?? 0)); + if (complete || pageCount === 0) { + break; + } + } + + return { + movieIds: merged.movieIds, + showIds: merged.showIds, + seasonIds: merged.seasonIds, + episodeIds: merged.episodeIds, + artistIds: merged.artistIds, + musicVideoIds: merged.musicVideoIds, + otherVideoIds: merged.otherVideoIds, + songIds: merged.songIds, + imageIds: merged.imageIds, + remoteStreamIds: merged.remoteStreamIds, + totals: totals as SearchAllItemTotals + }; +} + // Normalizes a SearchAllItemIds result (nullable arrays) into a full AddItemsToCollectionRequest // so it can be piped straight into addItemsToCollection / addItemsToPlaylist. -export function toAddItemsRequestFromSearch(result: SearchAllItemIds): AddItemsToCollectionRequest { +export function toAddItemsRequestFromSearch( + result: Omit +): AddItemsToCollectionRequest { return { artistIds: result.artistIds ?? [], episodeIds: result.episodeIds ?? [], diff --git a/web/src/screens/SearchScreen.test.tsx b/web/src/screens/SearchScreen.test.tsx index e5a953db8..d613287a2 100644 --- a/web/src/screens/SearchScreen.test.tsx +++ b/web/src/screens/SearchScreen.test.tsx @@ -37,7 +37,20 @@ const allItems = { otherVideoIds: null, songIds: null, imageIds: null, - remoteStreamIds: null + remoteStreamIds: null, + // Totals equal to this single page's ids, so getAllSearchItemIds treats it as complete after one fetch. + totals: { + movieCount: 2, + showCount: 0, + seasonCount: 0, + episodeCount: 0, + artistCount: 0, + musicVideoCount: 0, + otherVideoCount: 0, + songCount: 0, + imageCount: 0, + remoteStreamCount: 0 + } }; const emptyBuckets = { diff --git a/web/src/screens/SearchScreen.tsx b/web/src/screens/SearchScreen.tsx index 73ceb1329..b74f21a7b 100644 --- a/web/src/screens/SearchScreen.tsx +++ b/web/src/screens/SearchScreen.tsx @@ -2,7 +2,7 @@ import { useCallback, useEffect, useRef, useState } from 'react'; import { CheckSquare, FolderPlus, ListVideo, Save, Search, TriangleAlert, X } from 'lucide-react'; import { Button, Card, Input, Spinner, Toast } from '../components'; import { - getSearchAllItems, + getAllSearchItemIds, getSearchResults, messageFromSearchError, toAddItemsRequestFromSearch, @@ -169,7 +169,7 @@ export function SearchScreen() { } setPendingAll(kind); - getSearchAllItems(trimmed) + getAllSearchItemIds(trimmed) .then((result) => { if (!activeRef.current) { return;