timothy/ersatztv
1
0
Fork 0
Code Issues 58 Pull Requests 8 Actions Packages Projects Releases 9 Wiki Activity

Harden remote-image fetching in the graphics engine (timeout, size cap, redirects, pooling, caching, SSRF) #511

New Issue
Closed
opened 2026-07-20 22:46:02 +02:00 by timothy · 2 comments
timothy commented 2026-07-20 22:46:02 +02:00
Owner
Copy Link
Copy Source

Split out of #502 (found by that PR's adversarial review). Pre-existing behavior, but #502 routes ordinary channel-logo watermarks onto this path, so it is now on a materially wider surface and deserves its own hardening pass.

Where

ErsatzTV.Infrastructure/Streaming/Graphics/Image/ImageElementBase.cs (~:47-58):

bool isRemoteUri = Uri.TryCreate(image, UriKind.Absolute, out Uri uriResult)
                   && (uriResult.Scheme == Uri.UriSchemeHttp || uriResult.Scheme == Uri.UriSchemeHttps);

if (isRemoteUri)
{
    using var client = new HttpClient();
    await using Stream imageStream = await client.GetStreamAsync(uriResult, cancellationToken);
    _sourceImage = await Image.LoadAsync(imageStream, cancellationToken);
}

Problems

  1. No timeout override — the 100s HttpClient default. A slow-drip or hung host stalls graphics-engine init while ffmpeg waits on the pipe. Cold-start sensitive (cf. #350).
  2. No size cap — no MaxResponseContentBufferSize, no content-length/content-type check before Image.LoadAsync. Decompression-bomb surface.
  3. Redirects auto-followed with no policy — an allowed host can bounce the fetch anywhere, including back inside the network.
  4. new HttpClient() per element, not pooled — the classic socket-exhaustion pattern; should use IHttpClientFactory, as ArtworkController already does for Plex/Jellyfin/Emby artwork.
  5. No caching — the image is re-fetched on every playout item transition, not just once per stream.
  6. SSRF: ChannelValidations.ValidateLogo only checks the scheme is http/https — no host/IP restriction. An admin-set logo URL is a request origin inside the container's network. Admin-only, so low severity, but it is a real capability.

Existing mitigation worth preserving: WatermarkElement.InitializeAsync wraps LoadImage in try/catch and disables the watermark on failure, so a dead URL degrades gracefully rather than killing the stream.

Note (4) and (5) together are the argument for the alternative #502 rejected — fetching once into the image cache via IImageCache.SaveArtworkToCache(Stream, ArtworkKind). If this issue lands that way, it also removes the per-frame-cost asymmetry noted in #502's decision entry.

Done-when

  • Bounded per-request timeout, threaded through to the stream read (not just SendAsync — see the ResponseHeadersRead lesson in docs/decisions.md from #289)
  • Response size cap + content-type check before decode
  • Pooled client via IHttpClientFactory
  • Decision recorded on caching vs. re-fetch-per-item, and on redirect/host policy
  • A dead/slow/oversized URL is covered by tests and still degrades to "watermark disabled", never a killed stream
  • Adversarial review passed

Addressed by PR #518. Five rounds of independent adversarial review; rounds 2–4 returned BLOCKED and found three real decode-bomb bypasses plus one functional regression, round 5 returned MERGEABLE @ 5616fa6d. Full write-up in the PR.

Scope grew beyond the original checklist in one respect worth noting: the byte cap alone does not bound decoding, so the fix also had to budget the decode itself — and, after two failed attempts, to impose that bound on the decoder (DecoderOptions.MaxFrames + post-decode re-verification) rather than on header-reported values, which lie.

Split out of **#502** (found by that PR's adversarial review). Pre-existing behavior, but #502 routes ordinary channel-logo watermarks onto this path, so it is now on a materially wider surface and deserves its own hardening pass. ## Where `ErsatzTV.Infrastructure/Streaming/Graphics/Image/ImageElementBase.cs` (~`:47-58`): ```csharp bool isRemoteUri = Uri.TryCreate(image, UriKind.Absolute, out Uri uriResult) && (uriResult.Scheme == Uri.UriSchemeHttp || uriResult.Scheme == Uri.UriSchemeHttps); if (isRemoteUri) { using var client = new HttpClient(); await using Stream imageStream = await client.GetStreamAsync(uriResult, cancellationToken); _sourceImage = await Image.LoadAsync(imageStream, cancellationToken); } ``` ## Problems 1. **No timeout override** — the 100s `HttpClient` default. A slow-drip or hung host stalls graphics-engine init while ffmpeg waits on the pipe. Cold-start sensitive (cf. #350). 2. **No size cap** — no `MaxResponseContentBufferSize`, no content-length/content-type check before `Image.LoadAsync`. Decompression-bomb surface. 3. **Redirects auto-followed** with no policy — an allowed host can bounce the fetch anywhere, including back inside the network. 4. **`new HttpClient()` per element, not pooled** — the classic socket-exhaustion pattern; should use `IHttpClientFactory`, as `ArtworkController` already does for Plex/Jellyfin/Emby artwork. 5. **No caching** — the image is re-fetched on every playout item transition, not just once per stream. 6. **SSRF**: `ChannelValidations.ValidateLogo` only checks the scheme is http/https — no host/IP restriction. An admin-set logo URL is a request origin inside the container's network. Admin-only, so low severity, but it is a real capability. Existing mitigation worth preserving: `WatermarkElement.InitializeAsync` wraps `LoadImage` in try/catch and disables the watermark on failure, so a dead URL degrades gracefully rather than killing the stream. Note (4) and (5) together are the argument for the alternative #502 rejected — fetching once into the image cache via `IImageCache.SaveArtworkToCache(Stream, ArtworkKind)`. If this issue lands that way, it also removes the per-frame-cost asymmetry noted in #502's decision entry. ## Done-when - [x] Bounded per-request timeout, threaded through to the stream read (not just `SendAsync` — see the `ResponseHeadersRead` lesson in `docs/decisions.md` from #289) - [x] Response size cap + content-type check before decode - [x] Pooled client via `IHttpClientFactory` - [x] Decision recorded on caching vs. re-fetch-per-item, and on redirect/host policy - [x] A dead/slow/oversized URL is covered by tests and still degrades to "watermark disabled", never a killed stream - [x] Adversarial review passed --- **Addressed by PR #518.** Five rounds of independent adversarial review; rounds 2–4 returned BLOCKED and found three real decode-bomb bypasses plus one functional regression, round 5 returned `MERGEABLE @ 5616fa6d`. Full write-up in the PR. Scope grew beyond the original checklist in one respect worth noting: the byte cap alone does **not** bound decoding, so the fix also had to budget the decode itself — and, after two failed attempts, to impose that bound on the **decoder** (`DecoderOptions.MaxFrames` + post-decode re-verification) rather than on header-reported values, which lie.
timothy added the priority: medium label 2026-07-20 23:22:13 +02:00
timothy added the in-progress label 2026-07-20 23:41:52 +02:00
timothy commented 2026-07-20 23:41:55 +02:00
Author
Owner
Copy Link
Copy Source

Claiming this (Claude Code session, 2026-07-20). Selector ranked it top eligible: priority: medium, deps clear, no arc/milestone/review candidates above it.

Plan: read #502's decision entry + the existing IHttpClientFactory usage in ArtworkController first, then decide caching-vs-refetch and redirect/host policy before touching ImageElementBase.

Claiming this (Claude Code session, 2026-07-20). Selector ranked it top eligible: `priority: medium`, deps clear, no arc/milestone/review candidates above it. Plan: read #502's decision entry + the existing `IHttpClientFactory` usage in `ArtworkController` first, then decide caching-vs-refetch and redirect/host policy before touching `ImageElementBase`.
timothy commented 2026-07-21 01:47:55 +02:00
Author
Owner
Copy Link
Copy Source

Closing — merged in PR #518

What was done

ImageElementBase.LoadImage fetched remote images with a throwaway new HttpClient() + GetStreamAsync — unbounded in timeout, size, redirects and pooling, all inside stream startup while ffmpeg waits on the pipe. Replaced with IRemoteImageFetcher / HttpRemoteImageFetcher, modelled on the neighbouring IRemoteStreamProber so it follows an existing convention rather than inventing one.

  • Timeout covers headers and body — linked CTS threaded into GetAsync and every stream read, client Timeout = InfiniteTimeSpan set on the instance (not only in DI, so dropping the registration can't silently widen it to 100s). The #289 ResponseHeadersRead lesson.
  • Wire cap 10 MiB, enforced during the copy; Content-Length is only a cheap early reject since it can be absent or lie.
  • Decode budget width × height × frames ≤ 50 MP, imposed via DecoderOptions.MaxFrames and re-verified against the decoded image.
  • Retention budget frames × scaledWidth × scaledHeight ≤ 200 MP, since every frame is scaled to output resolution and retained.
  • Pooling via IHttpClientFactory; redirects capped at 3 (default 50).

Root cause

Two distinct causes, worth separating:

  1. The original defect was pre-existing convenience code — new HttpClient() inline — that was fine while it only ran for hand-configured image elements. #502 routed ordinary channel-logo watermarks onto the same path, which is what turned a latent weakness into a live one. The lesson is that widening a path's audience is itself a change that deserves a hardening review, even when the path's code doesn't change.

  2. Why it took five review rounds is the more useful root cause: my first three attempts all bounded something other than what actually allocates. A byte cap bounds the wire, not the decode. Independent dimension and frame caps bound neither, because the cost is their product. A product computed from header values bounds nothing at all, because headers lie — ImageSharp's Identify reports 0 frames for an APNG the decoder then expands to 600. A limit the decoder does not enforce is not a limit.

Files changed

  • ErsatzTV.Core/Interfaces/Streaming/IRemoteImageFetcher.cs (new)
  • ErsatzTV.Infrastructure/Streaming/HttpRemoteImageFetcher.cs (new)
  • ErsatzTV.Infrastructure/Streaming/Graphics/Image/ImageElementBase.cs — decode/retention budgets
  • ErsatzTV.Infrastructure/Streaming/Graphics/{GraphicsEngine,Image/ImageElement,Image/WatermarkElement}.cs — threading
  • ErsatzTV/Startup.cs — DI + named client with the redirect cap
  • Tests: HttpRemoteImageFetcherTests, RemoteImageDecodeLimitTests, WatermarkElementRemoteImageTests, HttpTestDoubles (extracted from HttpRemoteStreamProberTests)
  • docs/decisions.md

Deliberately deferred

  • No caching. With pooling, a 10s ceiling and a 10 MiB cap, one small GET per playout-item transition doesn't justify a cache's invalidation policy — and it keeps #502's "external artwork passes through, it is not downloaded into the image cache" convention intact. Revisit only with a measurement showing the re-fetch costs something.
  • SSRF not mitigated. The load-bearing reason is that the primitive is blind: the body is never returned to any caller, only composited into a video frame. Admin-only is the weaker half of the argument. LAN-hosted logos are a legitimate setup here, so a private-IP denylist would break working installs. Revisit if logo URLs ever become settable through a lower-privilege path.
  • Accepted 3× peak (~600 MB vs the nominal 50 MP): detecting "over the limit" requires decoding past it, and tightening the single-frame allowance to budget/3 would reject legitimate 8K stills.
  • Caller cancellation is still swallowed by the elements' pre-existing blanket catch (Exception), so the fetcher's TimeoutException/OperationCanceledException distinction only sharpens the log message today. Pre-existing, out of scope, documented.

Follow-ups created

None. One informational note recorded in the PR: multi-page TIFF safety is inherited from ImageSharp's hard "Images with different sizes are not supported" — worth re-checking on an ImageSharp bump, but there's nothing actionable today.

Docs updated

docs/decisions.md — new entry recording the design, both failed attempts, and the accepted residual risks. Documenting the failures was deliberate: the generalizable lesson is more valuable than the final code, and two earlier versions of that entry claimed bounds the code didn't enforce. No route/endpoint/SPA changes, so no other doc was owed.

## Closing — merged in PR #518 ### What was done `ImageElementBase.LoadImage` fetched remote images with a throwaway `new HttpClient()` + `GetStreamAsync` — unbounded in timeout, size, redirects and pooling, all inside stream startup while ffmpeg waits on the pipe. Replaced with `IRemoteImageFetcher` / `HttpRemoteImageFetcher`, modelled on the neighbouring `IRemoteStreamProber` so it follows an existing convention rather than inventing one. - **Timeout** covers headers *and* body — linked CTS threaded into `GetAsync` and every stream read, client `Timeout = InfiniteTimeSpan` set on the instance (not only in DI, so dropping the registration can't silently widen it to 100s). The #289 `ResponseHeadersRead` lesson. - **Wire cap** 10 MiB, enforced during the copy; `Content-Length` is only a cheap early reject since it can be absent or lie. - **Decode budget** `width × height × frames ≤ 50 MP`, imposed via `DecoderOptions.MaxFrames` and **re-verified against the decoded image**. - **Retention budget** `frames × scaledWidth × scaledHeight ≤ 200 MP`, since every frame is scaled to output resolution and retained. - **Pooling** via `IHttpClientFactory`; **redirects** capped at 3 (default 50). ### Root cause Two distinct causes, worth separating: 1. **The original defect** was pre-existing convenience code — `new HttpClient()` inline — that was fine while it only ran for hand-configured image elements. #502 routed *ordinary channel-logo watermarks* onto the same path, which is what turned a latent weakness into a live one. The lesson is that widening a path's audience is itself a change that deserves a hardening review, even when the path's code doesn't change. 2. **Why it took five review rounds** is the more useful root cause: my first three attempts all bounded something *other than what actually allocates*. A byte cap bounds the wire, not the decode. Independent dimension and frame caps bound neither, because the cost is their product. A product computed from header values bounds nothing at all, because headers lie — ImageSharp's `Identify` reports **0 frames** for an APNG the decoder then expands to 600. **A limit the decoder does not enforce is not a limit.** ### Files changed - `ErsatzTV.Core/Interfaces/Streaming/IRemoteImageFetcher.cs` (new) - `ErsatzTV.Infrastructure/Streaming/HttpRemoteImageFetcher.cs` (new) - `ErsatzTV.Infrastructure/Streaming/Graphics/Image/ImageElementBase.cs` — decode/retention budgets - `ErsatzTV.Infrastructure/Streaming/Graphics/{GraphicsEngine,Image/ImageElement,Image/WatermarkElement}.cs` — threading - `ErsatzTV/Startup.cs` — DI + named client with the redirect cap - Tests: `HttpRemoteImageFetcherTests`, `RemoteImageDecodeLimitTests`, `WatermarkElementRemoteImageTests`, `HttpTestDoubles` (extracted from `HttpRemoteStreamProberTests`) - `docs/decisions.md` ### Deliberately deferred - **No caching.** With pooling, a 10s ceiling and a 10 MiB cap, one small GET per playout-item transition doesn't justify a cache's invalidation policy — and it keeps #502's "external artwork passes through, it is not downloaded into the image cache" convention intact. Revisit only with a measurement showing the re-fetch costs something. - **SSRF not mitigated.** The load-bearing reason is that the primitive is **blind**: the body is never returned to any caller, only composited into a video frame. Admin-only is the weaker half of the argument. LAN-hosted logos are a legitimate setup here, so a private-IP denylist would break working installs. Revisit if logo URLs ever become settable through a lower-privilege path. - **Accepted 3× peak** (~600 MB vs the nominal 50 MP): detecting "over the limit" requires decoding past it, and tightening the single-frame allowance to budget/3 would reject legitimate 8K stills. - **Caller cancellation is still swallowed** by the elements' pre-existing blanket `catch (Exception)`, so the fetcher's `TimeoutException`/`OperationCanceledException` distinction only sharpens the log message today. Pre-existing, out of scope, documented. ### Follow-ups created None. One informational note recorded in the PR: multi-page TIFF safety is inherited from ImageSharp's hard `"Images with different sizes are not supported"` — worth re-checking on an ImageSharp bump, but there's nothing actionable today. ### Docs updated `docs/decisions.md` — new entry recording the design, **both failed attempts**, and the accepted residual risks. Documenting the failures was deliberate: the generalizable lesson is more valuable than the final code, and two earlier versions of that entry claimed bounds the code didn't enforce. No route/endpoint/SPA changes, so no other doc was owed.
timothy removed the in-progress label 2026-07-21 01:48:00 +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 priority: medium
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#511