Song-progress overlay bypasses WatermarkSelector and can hand ffmpeg a nonexistent -i path #653

New Issue

2026-07-26T21:02:21+02:00

timothy commented

2026-07-26 21:02:21 +02:00

Found by independent review of #510 (PR for ffmpeg.watermark-resolution-unified). Pre-existing, not introduced by #510 — but #510's record initially over-claimed that the hazard was fully closed, so this issue exists to make the remaining bypass tracked rather than implied.

The bypass

#510 unified every ChannelWatermark-to-path resolution inside WatermarkSelector.ResolveWatermark, which existence-checks the Resource arm and degrades a missing file to "no watermark". Two handlers construct a WatermarkOptions directly, skipping that resolver entirely:

ErsatzTV.Application/Streaming/Queries/GetPlayoutItemProcessByChannelNumberHandler.cs (~:395-414)
ErsatzTV.Application/Troubleshooting/Commands/PrepareTroubleshootingPlaybackHandler.cs (~:275-294)

Both do the same thing for the song-progress overlay:

var progressWatermark = new ChannelWatermark { /* … */ ImageSource = ChannelWatermarkImageSource.Resource, Image = image };
var progressWatermarkOption = new WatermarkOptions(
    progressWatermark,
    Path.Combine(FileSystemLayout.ResourcesCacheFolder, progressWatermark.Image),  // unchecked
    Option<int>.None);
watermarks.Clear();
watermarks.Add(progressWatermarkOption);

Why it matters

After watermarks.Clear() the list holds exactly one Permanent, non-URL watermark. That is precisely the shape FFmpegLibraryProcessService.CanUseFFmpegNativeWatermark (:88-92) routes to the ffmpeg-native path:

graphicsElementCount == 0 && watermarks.Count == 1
  && watermarks.All(wm => wm.Watermark.Mode is ChannelWatermarkMode.Permanent && !Artwork.IsExternalUrl(wm.ImagePath))

URLs are the only exclusion. So if song_progress_overlay.png / song_progress_overlay_43.png is absent from <config>/cache/resources, ffmpeg receives a nonexistent input as a bare -i argument. Unlike the graphics-engine path — where WatermarkElement.InitializeAsync catches a load failure and disables the element — there is nothing here to degrade to "no overlay".

Likelihood

Low but not zero. ResourceExtractorService extracts both PNGs at startup, but it is a BackgroundService, so a stream starting during first-boot extraction can lose the race. Deleted/corrupted cache dirs are the other route. Notably ResolveWatermark's own Resource arm existence-checks for exactly this reason, so the codebase already treats a missing resource as possible.

Suggested fix

Either:

Route both sites through the shared resolver — the cleanest option, and it makes the "one resolver" property actually true. Needs a seam, since these handlers build a synthetic watermark rather than reading one from the DB.
Or existence-check at both sites and skip the overlay when the resource is missing, mirroring ResolveWatermark's Resource arm.

Option 1 is preferable; option 2 is a two-line hardening if option 1 proves awkward.

While there: WatermarkSelector.cs's // used for song progress overlay comment on the Resource arm is misleading — the real song-progress overlay never passes through it. Nothing in the app writes a Resource watermark to the database, so that arm is reachable only from hand-edited or imported rows.

Done-when

Song-progress overlay resolution existence-checks its resource path (via the shared resolver, or equivalently at both sites)
A missing resource degrades to "no overlay", never a nonexistent ffmpeg -i input
Both call sites covered (streaming and troubleshooting)
Test pins the missing-resource case for at least the streaming path
ffmpeg.watermark-resolution-unified updated to state the property now holds without the carve-out
Misleading // used for song progress overlay comment retargeted
Adversarial review passed

Found by independent review of **#510** (PR for `ffmpeg.watermark-resolution-unified`). Pre-existing, not introduced by #510 — but #510's record initially over-claimed that the hazard was fully closed, so this issue exists to make the remaining bypass tracked rather than implied. ## The bypass #510 unified every `ChannelWatermark`-to-path resolution inside `WatermarkSelector.ResolveWatermark`, which existence-checks the `Resource` arm and degrades a missing file to "no watermark". Two handlers construct a `WatermarkOptions` **directly**, skipping that resolver entirely: - `ErsatzTV.Application/Streaming/Queries/GetPlayoutItemProcessByChannelNumberHandler.cs` (~:395-414) - `ErsatzTV.Application/Troubleshooting/Commands/PrepareTroubleshootingPlaybackHandler.cs` (~:275-294) Both do the same thing for the song-progress overlay: ```csharp var progressWatermark = new ChannelWatermark { /* … */ ImageSource = ChannelWatermarkImageSource.Resource, Image = image }; var progressWatermarkOption = new WatermarkOptions( progressWatermark, Path.Combine(FileSystemLayout.ResourcesCacheFolder, progressWatermark.Image), // unchecked Option<int>.None); watermarks.Clear(); watermarks.Add(progressWatermarkOption); ``` ## Why it matters After `watermarks.Clear()` the list holds exactly one `Permanent`, **non-URL** watermark. That is precisely the shape `FFmpegLibraryProcessService.CanUseFFmpegNativeWatermark` (`:88-92`) routes to the **ffmpeg-native** path: ```csharp graphicsElementCount == 0 && watermarks.Count == 1 && watermarks.All(wm => wm.Watermark.Mode is ChannelWatermarkMode.Permanent && !Artwork.IsExternalUrl(wm.ImagePath)) ``` URLs are the only exclusion. So if `song_progress_overlay.png` / `song_progress_overlay_43.png` is absent from `<config>/cache/resources`, ffmpeg receives a nonexistent input as a bare `-i` argument. Unlike the graphics-engine path — where `WatermarkElement.InitializeAsync` catches a load failure and disables the element — there is nothing here to degrade to "no overlay". ## Likelihood Low but not zero. `ResourceExtractorService` extracts both PNGs at startup, but it is a `BackgroundService`, so a stream starting during first-boot extraction can lose the race. Deleted/corrupted cache dirs are the other route. Notably `ResolveWatermark`'s own `Resource` arm existence-checks for exactly this reason, so the codebase already treats a missing resource as possible. ## Suggested fix Either: 1. **Route both sites through the shared resolver** — the cleanest option, and it makes the "one resolver" property actually true. Needs a seam, since these handlers build a synthetic watermark rather than reading one from the DB. 2. **Or** existence-check at both sites and skip the overlay when the resource is missing, mirroring `ResolveWatermark`'s `Resource` arm. Option 1 is preferable; option 2 is a two-line hardening if option 1 proves awkward. While there: `WatermarkSelector.cs`'s `// used for song progress overlay` comment on the `Resource` arm is misleading — the real song-progress overlay never passes through it. Nothing in the app writes a `Resource` watermark to the database, so that arm is reachable only from hand-edited or imported rows. ## Done-when - [ ] Song-progress overlay resolution existence-checks its resource path (via the shared resolver, or equivalently at both sites) - [ ] A missing resource degrades to "no overlay", never a nonexistent ffmpeg `-i` input - [ ] Both call sites covered (streaming **and** troubleshooting) - [ ] Test pins the missing-resource case for at least the streaming path - [ ] `ffmpeg.watermark-resolution-unified` updated to state the property now holds without the carve-out - [ ] Misleading `// used for song progress overlay` comment retargeted - [ ] Adversarial review passed

timothy added the bug priority: low labels 2026-07-26 21:02:29 +02:00

timothy referenced this issue from a commit

2026-07-26 21:29:21 +02:00

fix(510): address independent review — Resource null guard, honest routing claim

timothy referenced this issue

2026-07-26 21:29:54 +02:00

fix(510): one watermark resolver for all four attachment points #655

timothy referenced this issue

2026-07-26 21:35:02 +02:00

WatermarkSelector: the deco path has a different missing-logo policy than the three precedence levels #510

timothy referenced this issue from a commit

2026-07-26 21:38:12 +02:00