feat(293): paginate GET /api/v1/search/all-items to cap DoS exposure #442

Merged

timothy merged 2 commits from feat/293-search-allitems-cap into main 2026-07-18 14:17:21 +02:00

Conversation 1 Commits 2 Files Changed 16

+641 -40

timothy commented 2026-07-18 13:35:27 +02:00

Owner

Copy Link

Copy Source

fixes #293

What

GET /api/v1/search/all-items fired ten index searches with limit: 0 (= every hit), so a broad authenticated query materialized the whole index across ten media kinds into one response — unbounded work per request. This paginates it.

Design: issue option (a) — full pagination (operator-confirmed). Not option (b) (cap + truncation), because "add all to collection/playlist" must stay complete. Recorded in docs/decisions.md 2026-07-18 (#293).

Changes

• Endpoint (additive): SearchAllItems gains optional pageNum (0-based, clamped 0..2_000_000 so pageNum*pageSize can't overflow int→500) and pageSize (default 500, clamped 1..1000, matching the §1 Logs precedent). The response gains an additive per-kind Totals (SearchResultAllItemsTotalsResponseModel) so a client can page to completeness. The ten …Ids buckets are unchanged (frozen-/api/v1 additive-only holds).
• Handler: skip = PageNum*PageSize, limit = PageSize into ISearchIndex.Search (native skip/limit), reads SearchResult.TotalCount per kind. Matches GetLibraryBrowseItemsHandler.
• SPA: getSearchAllItems(query, pageNum, pageSize) gains the params; new getAllSearchItemIds(query) pages until each kind hits its total (empty-page safety break against total drift; falls back to page-until-empty if totals is ever absent). SearchScreen.addAll calls it instead of the single unbounded fetch; the #221 stale-query guard + single add POST are unchanged.
• Deliberate default-behavior change: a caller sending no pageSize now gets one page + Totals, not the entire id set — the security change the issue asks for. Safe: the only in-repo consumer is the SPA (updated here); external callers read Totals and page. Documented.
• Docs: decisions.md + api-conventions.md §5; OpenAPI trio regenerated.

Out of scope

The add POST still accepts the full merged id set in one body — bounding that surface is separate (#308). #293 is the GET.

Verification

• Backend: full ErsatzTV.Tests suite green; new QuerySearchIndexAllItemsHandlerTests (skip/limit threading, per-kind totals) + SearchControllerTests (clamp incl. huge-pageNum, param threading, Totals mapping).
• SPA: typecheck, lint, check:api clean; search.test.ts (paging params, single/multi-page accumulation, drift + missing-totals safety) + existing SearchScreen.test.tsx green (21 tests).
• Format gate (#311) + BOM check clean.
• Cold-context adversarial review: MERGEABLE — verified skip/limit math vs the sibling handler, Lucene Take branch safe at max pageSize 1000, loop termination sound, tests non-vacuous. Its two LOW findings (pageNum overflow, missing-totals under-fetch) are fixed in this diff.

Independent review: read-path GET + SPA paging, no write-path handler change and no DB migration; live-E2E's usual net (lazy-enumeration / reload-through-read-path 500) doesn't apply. The cold adversarial review + full unit coverage stand in for it.

🤖 Generated with Claude Code

fixes #293 ## What `GET /api/v1/search/all-items` fired ten index searches with `limit: 0` (= every hit), so a broad *authenticated* query materialized the whole index across ten media kinds into one response — unbounded work per request. This paginates it. **Design: issue option (a) — full pagination (operator-confirmed).** Not option (b) (cap + truncation), because "add all to collection/playlist" must stay complete. Recorded in `docs/decisions.md` 2026-07-18 (#293). ## Changes - **Endpoint (additive):** `SearchAllItems` gains optional `pageNum` (0-based, clamped `0..2_000_000` so `pageNum*pageSize` can't overflow `int`→500) and `pageSize` (default 500, clamped `1..1000`, matching the §1 Logs precedent). The response gains an **additive** per-kind `Totals` (`SearchResultAllItemsTotalsResponseModel`) so a client can page to completeness. The ten `…Ids` buckets are unchanged (frozen-`/api/v1` additive-only holds). - **Handler:** `skip = PageNum*PageSize`, `limit = PageSize` into `ISearchIndex.Search` (native skip/limit), reads `SearchResult.TotalCount` per kind. Matches `GetLibraryBrowseItemsHandler`. - **SPA:** `getSearchAllItems(query, pageNum, pageSize)` gains the params; new `getAllSearchItemIds(query)` pages until each kind hits its total (empty-page safety break against total drift; falls back to page-until-empty if `totals` is ever absent). `SearchScreen.addAll` calls it instead of the single unbounded fetch; the #221 stale-query guard + single add POST are unchanged. - **Deliberate default-behavior change:** a caller sending no `pageSize` now gets one page + `Totals`, not the entire id set — the security change the issue asks for. Safe: the only in-repo consumer is the SPA (updated here); external callers read `Totals` and page. Documented. - Docs: `decisions.md` + `api-conventions.md` §5; OpenAPI trio regenerated. ## Out of scope The add POST still accepts the full merged id set in one body — bounding *that* surface is separate (#308). #293 is the GET. ## Verification - Backend: full `ErsatzTV.Tests` suite green; new `QuerySearchIndexAllItemsHandlerTests` (skip/limit threading, per-kind totals) + `SearchControllerTests` (clamp incl. huge-pageNum, param threading, Totals mapping). - SPA: typecheck, lint, `check:api` clean; `search.test.ts` (paging params, single/multi-page accumulation, drift + missing-totals safety) + existing `SearchScreen.test.tsx` green (21 tests). - Format gate (#311) + BOM check clean. - Cold-context adversarial review: **MERGEABLE** — verified skip/limit math vs the sibling handler, Lucene `Take` branch safe at max pageSize 1000, loop termination sound, tests non-vacuous. Its two LOW findings (pageNum overflow, missing-totals under-fetch) are fixed in this diff. **Independent review**: read-path GET + SPA paging, no write-path handler change and no DB migration; live-E2E's usual net (lazy-enumeration / reload-through-read-path 500) doesn't apply. The cold adversarial review + full unit coverage stand in for it. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

timothy added 1 commit 2026-07-18 13:35:27 +02:00

feat(293): paginate GET /api/v1/search/all-items to cap DoS exposure ... ac7965dee4

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) <noreply@anthropic.com>

timothy added 1 commit 2026-07-18 13:40:25 +02:00

docs(293): note the pageNum upper clamp (MaxAllItemsPageNum) in the decision + api-conventions ... c57fbf9826

Fix delta re-review flagged the decision entry + §5 note still described the
pre-fix pageNum = Math.Max(0, pageNum); the shipped code clamps the upper bound
too (0..2_000_000) to stop pageNum*pageSize overflowing int to a 500.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

timothy commented 2026-07-18 13:40:54 +02:00

Author

Owner

Copy Link

Copy Source

Independent adversarial review (cold-context, two passes): full-diff pass returned MERGEABLE (verified skip/limit math against the sibling GetLibraryBrowseItemsHandler, the Lucene .Take branch is safe at max pageSize 1000, loop termination sound, tests non-vacuous). Its two LOW findings were fixed in-diff:

• pageNum upper-clamp 0..2_000_000 (was overflow→500) — verified ~147M headroom at the boundary.
• SPA loop now requires totals present before the fast completeness check, else pages until an empty page (no silent truncation).

A focused fix-delta re-review confirmed both fixes + the rebase-conflict resolutions (append-only decisions.md, search.test.ts keeps both main's getSearchFields and the new paging suites) are correct, and flagged one doc drift (the decision still described the pre-fix Math.Max), now corrected in c57fbf98.

Local gates: full ErsatzTV.Tests suite green; SPA typecheck/lint/check:api/vitest (21) green; #311 format gate + BOM clean; OpenAPI regenerated (idempotent).

Review-verdict: MERGEABLE @ c57fbf9826

Independent adversarial review (cold-context, two passes): full-diff pass returned **MERGEABLE** (verified skip/limit math against the sibling `GetLibraryBrowseItemsHandler`, the Lucene `.Take` branch is safe at max `pageSize` 1000, loop termination sound, tests non-vacuous). Its two LOW findings were fixed in-diff: - `pageNum` upper-clamp `0..2_000_000` (was overflow→500) — verified ~147M headroom at the boundary. - SPA loop now requires `totals` present before the fast completeness check, else pages until an empty page (no silent truncation). A focused fix-delta re-review confirmed both fixes + the rebase-conflict resolutions (append-only `decisions.md`, `search.test.ts` keeps both main's `getSearchFields` and the new paging suites) are correct, and flagged one doc drift (the decision still described the pre-fix `Math.Max`), now corrected in `c57fbf98`. Local gates: full `ErsatzTV.Tests` suite green; SPA typecheck/lint/`check:api`/vitest (21) green; #311 format gate + BOM clean; OpenAPI regenerated (idempotent). Review-verdict: MERGEABLE @ c57fbf982645c859ca6300dbd7da2a8dc1f62c1d

timothy referenced this pull request 2026-07-18 14:17:03 +02:00

Page/cap GET /api/search/all-items (DoS hardening — deferred from #285) #293

timothy merged commit 551366aa72 into main 2026-07-18 14:17:21 +02:00

timothy deleted branch feat/293-search-allitems-cap 2026-07-18 14:17:21 +02:00

timothy referenced this issue from a commit 2026-07-18 14:17:21 +02:00

Merge pull request 'feat(293): paginate GET /api/v1/search/all-items to cap DoS exposure' (#442) from feat/293-search-allitems-cap into main

timothy referenced this pull request 2026-07-18 14:18:04 +02:00

Page/cap GET /api/search/all-items (DoS hardening — deferred from #285) #293

timothy referenced this pull request 2026-07-18 14:18:30 +02:00

ChicoryTV rewrite — queue tracker (goal, arc, session protocol) #237

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

ad-hoc

One-off / ad-hoc work not tracked by a dedicated issue

api

REST API / HTTP endpoints

bug

Something isn't working

ci-cd

Build, test, deploy pipeline

content

Channel content / schedules / playlists

dependencies

Dependency updates (Renovate)

enhancement

New feature or improvement

frontend

ChicoryTV React SPA frontend

in-progress

Claimed by an active session — do not pick up

jellyfin

Jellyfin tuner / IPTV integration

parked

Excluded from automatic queue pickup; work only when explicitly selected

priority: high

priority: low

priority: medium

review

Adversarial review finding

security

Security / vulnerability fix

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

renovate timothy (Timothy)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: timothy/ersatztv#442