timothy/ersatztv
1
0
Fork 0
Code Issues 52 Pull Requests 6 Actions 2 Packages Projects Releases 9 Wiki Activity

Add-items handlers validate existence for only 4 of 10 media kinds (collections + playlists) #217

New Issue
Closed
opened 2026-07-10 00:24:58 +02:00 by timothy · 4 comments
timothy commented 2026-07-10 00:24:58 +02:00
Owner
Copy Link
Copy Source

Found by the PR #216 adversarial review (out-of-scope, pre-existing upstream gap).

AddItemsToPlaylistHandler (and the equivalent collection path via AddItemsToCollectionHandler.ValidateMediaItems) existence-check only movie/show/season/episode ids; artist, music-video, song, image, and remote-stream ids in the ten-array add-items request bodies are accepted unvalidated. Bogus ids for the unvalidated kinds either silently no-op or create dangling references instead of returning 422.

Scope: audit both handlers (ErsatzTV.Application/MediaCollections/Commands/AddItemsToPlaylistHandler.cs, AddItemsToCollectionHandler.cs), extend validation to all ten kinds (fix at the handler per docs/decisions.md), add controller/handler tests for a bogus id of each kind.

Same systemic-API-posture family as #215. Surfaced 2026-07-10; affects POST /api/collections/{id}/items and the new POST /api/playlists/{id}/items (PR #216, #208/#209).

Found by the PR #216 adversarial review (out-of-scope, pre-existing upstream gap). `AddItemsToPlaylistHandler` (and the equivalent collection path via `AddItemsToCollectionHandler.ValidateMediaItems`) existence-check only movie/show/season/episode ids; artist, music-video, song, image, and remote-stream ids in the ten-array add-items request bodies are accepted unvalidated. Bogus ids for the unvalidated kinds either silently no-op or create dangling references instead of returning 422. Scope: audit both handlers (`ErsatzTV.Application/MediaCollections/Commands/AddItemsToPlaylistHandler.cs`, `AddItemsToCollectionHandler.cs`), extend validation to all ten kinds (fix at the handler per docs/decisions.md), add controller/handler tests for a bogus id of each kind. Same systemic-API-posture family as #215. Surfaced 2026-07-10; affects `POST /api/collections/{id}/items` and the new `POST /api/playlists/{id}/items` (PR #216, #208/#209).
timothy added the apibugpriority: highreview labels 2026-07-10 08:28:02 +02:00
timothy commented 2026-07-10 08:28:02 +02:00
Author
Owner
Copy Link
Copy Source

Retro review (adversarial-reviewer#18): this issue needs a scope correction and should gate the #208/#209 parity claim.

  • Collections are not missing existence validation for the six remaining kinds. AddItemsToCollectionHandler.ValidateMediaItems aggregates all ten arrays and verifies every id exists in dbContext.MediaItems. The four type-specific checks are redundant/stronger checks, but a bogus artist/song/image/etc. id already returns an error.
  • Playlists do have the systemic validation gap, and there is a worse direct bug: AddItemsToPlaylistHandler.ApplyAddItemsRequest builds its type dictionary for only nine kinds and omits CollectionType.RemoteStream. A valid remoteStreamIds request returns success while adding nothing.
  • PR #216 exposes per-card/bulk playlist adds for remote streams through the shared AddToMenu, so this is now a user-visible false-success path, not only malformed-client hardening.

Re-scope the fix around the playlist handler: include RemoteStream, validate existence and bucket type for all ten arrays before creating typed PlaylistItem rows, and test one valid + bogus + wrong-kind id per bucket. Keep the collection aggregate existence test, adding type validation only if the API contract actually requires bucket/type rejection there. Added review/high-priority labels.

Retro review (adversarial-reviewer#18): this issue needs a scope correction and should gate the #208/#209 parity claim. - **Collections are not missing existence validation for the six remaining kinds.** `AddItemsToCollectionHandler.ValidateMediaItems` aggregates all ten arrays and verifies every id exists in `dbContext.MediaItems`. The four type-specific checks are redundant/stronger checks, but a bogus artist/song/image/etc. id already returns an error. - **Playlists do have the systemic validation gap**, and there is a worse direct bug: `AddItemsToPlaylistHandler.ApplyAddItemsRequest` builds its type dictionary for only nine kinds and omits `CollectionType.RemoteStream`. A valid `remoteStreamIds` request returns success while adding nothing. - PR #216 exposes per-card/bulk playlist adds for remote streams through the shared `AddToMenu`, so this is now a user-visible false-success path, not only malformed-client hardening. Re-scope the fix around the playlist handler: include RemoteStream, validate existence **and bucket type** for all ten arrays before creating typed `PlaylistItem` rows, and test one valid + bogus + wrong-kind id per bucket. Keep the collection aggregate existence test, adding type validation only if the API contract actually requires bucket/type rejection there. Added `review`/high-priority labels.
timothy commented 2026-07-10 23:12:24 +02:00
Author
Owner
Copy Link
Copy Source

Scope correction after code verification (adversarial-reviewer#18 response, 2026-07-10) — the original description overstates the collection gap and understates the playlist gap:

  • Collections are already fully validated. AddItemsToCollectionHandler.ValidateMediaItems (lines 146-158) aggregate-counts all requested ids (all ten kinds, via GetRequestedMediaItemIds lines 160-170, including RemoteStreamIds) against MediaItems and rejects on any missing id. The four typed validators are redundant extra checks. No collection-side fix is needed.
  • Playlists have two defects, not one:
    1. AddItemsToPlaylistHandler.Validate (lines 76-85) checks only Movie/Show/Season/Episode — Artist, MusicVideo, OtherVideo, Song, Image, RemoteStream are unvalidated (OtherVideo was missing from the original list too).
    2. ApplyAddItemsRequest (lines 39-50) builds its kind dictionary from only nine kinds — RemoteStreamIds is absent, so valid RemoteStream ids are silently dropped, never added to the playlist. This is a functional bug independent of validation.

Revised scope: playlist handler only — add RemoteStream to the apply dictionary, add aggregate existence validation matching the collection handler's pattern, tests for a bogus id of each kind + a valid RemoteStream add. POST /api/collections/{id}/items drops out of scope.

**Scope correction after code verification (adversarial-reviewer#18 response, 2026-07-10)** — the original description overstates the collection gap and understates the playlist gap: - **Collections are already fully validated.** `AddItemsToCollectionHandler.ValidateMediaItems` (lines 146-158) aggregate-counts *all* requested ids (all ten kinds, via `GetRequestedMediaItemIds` lines 160-170, including `RemoteStreamIds`) against `MediaItems` and rejects on any missing id. The four typed validators are redundant extra checks. **No collection-side fix is needed.** - **Playlists have two defects, not one:** 1. `AddItemsToPlaylistHandler.Validate` (lines 76-85) checks only Movie/Show/Season/Episode — Artist, MusicVideo, **OtherVideo**, Song, Image, RemoteStream are unvalidated (OtherVideo was missing from the original list too). 2. `ApplyAddItemsRequest` (lines 39-50) builds its kind dictionary from only **nine** kinds — `RemoteStreamIds` is absent, so valid RemoteStream ids are **silently dropped**, never added to the playlist. This is a functional bug independent of validation. Revised scope: playlist handler only — add RemoteStream to the apply dictionary, add aggregate existence validation matching the collection handler's pattern, tests for a bogus id of each kind + a valid RemoteStream add. `POST /api/collections/{id}/items` drops out of scope.
timothy added the in-progress label 2026-07-11 01:41:21 +02:00
timothy commented 2026-07-11 01:41:21 +02:00
Author
Owner
Copy Link
Copy Source

claiming — session started 2026-07-10 ~23:00 (retroactive label per the #237 protocol adopted mid-session). Fix is in flight: PRs #222–#228 cover #221/#220/#217+#219/#215/#218/#213; awaiting CI green → merge → close.

claiming — session started 2026-07-10 ~23:00 (retroactive label per the #237 protocol adopted mid-session). Fix is in flight: PRs #222–#228 cover #221/#220/#217+#219/#215/#218/#213; awaiting CI green → merge → close.
timothy commented 2026-07-11 11:36:04 +02:00
Author
Owner
Copy Link
Copy Source

Closed via PR #224 (integration PR #239)

Root cause: AddItemsToPlaylistHandler was written against only the 4 original media kinds — its Validate never grew the aggregate existence check the collection handler has, and ApplyAddItemsRequest's kind dictionary was never extended when RemoteStream arrived, so valid RemoteStream ids returned success while silently adding nothing.

What was done (per the verified re-scope — collections needed NO changes, they already aggregate-validate all ten kinds): RemoteStream added to the playlist apply dictionary; aggregate existence validation added mirroring AddItemsToCollectionHandler.ValidateMediaItems (distinct ids counted against MediaItems, all ten kinds). Tests: bogus id per kind → 422; valid RemoteStream actually persisted (fails on revert of the apply fix). Adversarial review confirmed exact semantic parity with the collection handler (no drift, ordering preserved).

Deferred — explicitly: bucket-type validation (rejecting e.g. a movie id passed in songIds) from the reviewer's comment is NOT implemented — the fix deliberately matches the collection handler's existence-only posture for consistency. Wrong-kind ids remain accepted in both handlers. Deferring the strictness decision to #197 (API contract/security review), where it should be decided for collections + playlists together.

Docs: none needed (no contract shape change; OpenAPI unchanged).

## Closed via PR #224 (integration PR #239) **Root cause**: `AddItemsToPlaylistHandler` was written against only the 4 original media kinds — its `Validate` never grew the aggregate existence check the collection handler has, and `ApplyAddItemsRequest`'s kind dictionary was never extended when RemoteStream arrived, so valid RemoteStream ids returned success while silently adding nothing. **What was done** (per the verified re-scope — collections needed NO changes, they already aggregate-validate all ten kinds): RemoteStream added to the playlist apply dictionary; aggregate existence validation added mirroring `AddItemsToCollectionHandler.ValidateMediaItems` (distinct ids counted against `MediaItems`, all ten kinds). Tests: bogus id per kind → 422; valid RemoteStream actually persisted (fails on revert of the apply fix). Adversarial review confirmed exact semantic parity with the collection handler (no drift, ordering preserved). **Deferred — explicitly**: bucket-type validation (rejecting e.g. a movie id passed in `songIds`) from the reviewer's comment is NOT implemented — the fix deliberately matches the collection handler's existence-only posture for consistency. Wrong-kind ids remain accepted in both handlers. Deferring the strictness decision to **#197** (API contract/security review), where it should be decided for collections + playlists together. **Docs**: none needed (no contract shape change; OpenAPI unchanged).
timothy removed the in-progress label 2026-07-11 11:37:04 +02:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
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 api bug priority: high review
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
renovate timothy (Timothy)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: timothy/ersatztv#217