Fourth adversarial pass cleared the security design — all three earlier
bypasses are dead, DecoderOptions.MaxFrames is honored by every decoder
that can produce multiple frames (GIF/WebP/TIFF exactly N, APNG N-1),
and it bounds PEAK allocation, not just the final frame count (measured:
65 MiB capped vs 2.41 GiB uncapped on the same 600-frame GIF).
But it caught a functional regression this PR introduced: a *default*
`Image.IdentifyAsync` throws InvalidImageContentException on most APNGs
that `Image.Load` reads back perfectly — including files ImageSharp's
own PngEncoder wrote. Reproduced independently: 13 of 16 shapes throw,
and `MaxFrames = 1` on the Identify fixes all 16 with dimensions intact.
Since #502 routes ordinary channel-logo watermarks through this path, an
admin with an animated PNG logo would have silently lost their watermark
to a log line — a hardening change breaking working content.
The existing tests could not see it: they use 64x64, which happens to be
one of the few shapes a default Identify handles. Now pinned with a
288x288 shape that asserts the default Identify DOES fail and that
DecodeRemoteImage decodes it anyway, in full.
Also, from the same pass:
- document the REAL enforced peak (up to 3x the nominal 50 MP budget,
since detecting "over the limit" means decoding past it) instead of
restating the nominal number. Tightening the single-frame allowance to
budget/3 would reject legitimate 8K stills, so the overshoot is
deliberate; it is ~600 MB against the ~36 GiB it replaces
- correct the MaxFrames off-by-one claim: N-1 is APNG-specific, not
universal, so the stated rationale for +2 was wrong for three of the
four animated formats
Second adversarial re-review defeated the product budget too, and the
mechanism generalizes: the budget was enforced on a number the decoder
does not honor.
Measured on ImageSharp 3.1.12 (reproduced independently before fixing):
600-frame APNG -> Identify: FrameMetadataCollection.Count = 0
Load: Frames.Count = 600
So EnsureDecodeAffordable(w, h, 0) charged Math.Max(0,1) = 1 frame —
the most permissive possible reading. A 4000x4000 x600 APNG is ~134 KiB
on the wire, is charged 16 MP, and decodes to ~36 GiB: 2.5x worse than
the GIF the previous commit exists to stop, at half the wire size. The
retention budget could not backstop it — that runs after LoadAsync, so
the process OOMs first, killing every concurrent stream.
GIF, WebP and TIFF report honestly; PNG/APNG is the sole divergence,
which is the point: you cannot audit every format, so the header cannot
be the source of truth.
DecodeRemoteImage now:
- checks header DIMENSIONS only (trustworthy; a GIF image descriptor
exceeding its logical screen is clamped by the decoder, verified)
- derives how many frames of that size the budget affords
- passes that to DecoderOptions.MaxFrames, which the DECODER enforces
whatever the header claimed. Measured: MaxFrames = N yields N-1
frames, so it asks for affordable + 2 — decoding one more than allowed
is what distinguishes "at the limit" from "over it" without silently
truncating a legitimate animation
- re-verifies the real image.Frames.Count after decoding, disposing and
rejecting if over
Also adds wiring coverage for the retention budget (M4): deleting its
call site now fails a test — negative-controlled, build verified before
trusting the result.
docs/decisions.md records both failed attempts, because the lesson is
the generalizable part: independent caps do not compose into a budget,
and a limit the decoder does not enforce is not a limit.
Adversarial re-review of the first fix defeated its decode guard with a
measured payload: a 2500x2500 x600-frame GIF is ~60 KiB on the wire,
passes the 50 MP dimension check (6.25 MP) AND the 600-frame check
(exactly 600), and costs ~14 GiB to decode — strictly worse than the
30000x30000 PNG the guard was added to stop, at 1/60th the wire size.
Checking dimensions and frames independently never bounded the decode.
- decode budget is now width x height x frames <= 50 MP, as one product;
a zero frame count is charged as one so an unenumerable header cannot
zero it out
- new retention budget: frames x scaledWidth x scaledHeight <= 200 MP.
Independent of the decode budget in both directions — a 100x100 source
is trivial to decode but retains ~5 GB of SKBitmap once every frame is
scaled to 1920x1080, since LoadImage clones and resizes each frame to
output resolution and keeps them
- both budgets are pure functions (EnsureDecodeAffordable,
EnsureScaledFramesAffordable) so the arithmetic is tested at every
boundary without materializing multi-gigabyte images
- the frame guard had NO coverage before; it does now
- fail loudly on a non-seekable fetcher stream instead of letting
Position throw NotSupportedException into the blanket catch
- test the copy over-read against the ACTUAL rented buffer length
(ArrayPool.Rent(81920) returns 131072), not the requested 81920
docs/decisions.md corrected: it claimed the byte cap bounded the
decode-bomb surface and that the header check closed the class. Both
overstated. An append-only file that is confidently wrong is worse than
one with a gap.
`ImageElementBase.LoadImage` fetched http(s) images with a throwaway
`new HttpClient()` + `GetStreamAsync`: no timeout override (the 100s
default), no size cap, unbounded redirects, no pooling — all inside
stream startup, while ffmpeg waits on the pipe. #502 routed ordinary
channel-logo watermarks onto that path, widening a pre-existing weakness.
Introduce `IRemoteImageFetcher` / `HttpRemoteImageFetcher`, modelled on
the neighbouring `IRemoteStreamProber`:
- deadline covers headers AND body (linked CTS + `CancelAfter`, client
`Timeout = InfiniteTimeSpan`) — under `ResponseHeadersRead` the body
read falls outside `HttpClient.Timeout` (the #289 lesson)
- 10 MiB cap enforced during the copy; `Content-Length` is only a cheap
early reject, since it can be absent or a lie
- permissive content-type check (rejects an HTML error page, allows a
missing type and octet-stream)
- pooled via `IHttpClientFactory`; redirects capped at 3, not 50
A byte cap does NOT bound decoding, so `DecodeRemoteImage` additionally
reads declared dimensions + frame count from the header and rejects
before `Image.LoadAsync` allocates (50 MP / 600 frames). A 4 KB PNG
declaring 30000x30000 costs ~3.6 GB to decode and passes every wire-size
check — caught by adversarial review of the first version of this change,
which capped bytes and wrongly claimed that was decode-bomb protection.
Not cached and SSRF not mitigated — both deliberate, with the reasoning
recorded in docs/decisions.md.
fixes#511
Fresh stdio JSON-RPC MCP server wrapping the frozen /api/v1 surface,
superseding the closed read-only PR #76. 26 read tools (six families +
search/all-items & search/artists discovery) and cautious-write CRUD:
collections (incl. idempotent membership adds for #487), smart collections,
schedules, playouts, channels (create/update/delete/reset), and a
Jellyfin-focused media-source sync/scan slice. Writes gated behind
ERSATZTV_ALLOW_WRITES (default false, runtime-enforced).
Security baseline carried forward from PR #76/#289: read-only backstop,
JSON-RPC DoS guards + bounded stdin reader, per-request CTS over
headers+body, response-size cap, arg validation vs InputSchema,
reverse-proxy prefix preservation. Machine-key auth (X-Api-Key,
CSRF-exempt). If-Match/ETag round-trip for the one replace-all PUT that
honors it.
Cold-review fixes folded in:
- HIGH: reject control chars (CR/LF) in the ifMatch value before it reaches
TryAddWithoutValidation — SocketsHttpHandler writes it verbatim, so a
crafted value could smuggle headers onto the X-Api-Key request.
- Cache the empty-args JsonDocument (no per-call pooled-doc leak).
- Accept explicit JSON null for optional fields so a nullable API field
(e.g. dailyRebuildTime) can be cleared as documented.
Deferred (documented): the ~40-field replace-list writes and redesign
workflow tools (#63-#68).
Docs: docs/mcp.md, docs/README.md index, docs/decisions.md.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Re-review of the fix commit flagged two ways the new tests could pass
vacuously in future: the burst assertion was a bare substring (satisfiable
by any input carrying the option) and the still-image test asserted only an
absence. Anchor the first on the input path plus an occurrence count, and
give the second a positive anchor.
`-readrate 1.05` paces input reading at wall clock so a channel behaves like
live TV, but it applies from the very first read. With 4s HLS segments and the
segmenter waiting for the first one, the playlist could not appear sooner than
~4/1.05 = 3.8s, so every tune-in that did not win a work-ahead slot paid a
multi-second wait.
Add `-readrate_initial_burst` (FFmpeg >= 6.1) next to `-readrate` on the normal
playback path, gated on runtime capability detection via the existing
`FFmpegKnownOption`/`HasOption` machinery, whose option list had simply been
empty. Measured on real prod media: time-to-first-playlist 5369/5344ms ->
648/649ms.
Root cause detail: the cold-start bimodality earlier rounds could not explain
was never about the media. `HlsSessionWorker` grants an unthrottled start only
while `_workAheadCount < work_ahead_limit` (prod: 1), so concurrent tune-ins
fall back to the throttled path. Confirmed on prod with three concurrent tunes:
firstGop 866ms for the slot winner vs 3845ms and 6357ms. This also falsifies the
issue's ranked #1 driver — accurate-seek decode-discard measures 30-100ms on
real media, and probe caps 20-50ms; neither can account for seconds.
Still images are excluded: their video input is paced by the realtime filter and
takes no readrate, so a burst would only run a song's separate audio input ahead
of the video. Concat/WrapSegmenter keep the unburst single-arg constructor.
fixes#350
Verified live while promoting v26.11.0. Two corrections:
1. `DeployStack media-servers` targets a DEAD stack. The Komodo stack name
changed to `jazz-media` with the move to jazz; the compose PROJECT is still
`media-servers` (which is what container labels show, so the labels don't
catch this). A `media-servers` stack still exists on bumblebee in state
`unhealthy` — the stopped migration leftovers — so the documented command
silently deploys the wrong, dead thing.
2. There is no Global Auto Update fallback: `jazz-media` has
auto_update=false (poll_for_updates=true only). Promotion is manual, full
stop, and the 'don't cut a tag near the 03:00 run' caveat is obsolete.
The pre-deploy safety chain is intact and jazz-aware (#635) — verified by
reading the deploy's Pre Deploy stage: image-change trigger fired, 286M backup
with integrity_check=ok plus off-box PBS, migration smoke PASS against the
prod-copy, then only ersatztv recreated.
Adversarial review found a documentation defect, not a code one: both
docs/decisions.md and the WatermarkSelector comment asserted the deco path
was unaffected by this change. That is true of the *resolution* half and
false of the *routing* half. SelectWatermarks puts deco-derived options
into the same list the routing guard filters, so a deco watermark whose
resolved path is a URL is rerouted to the graphics engine too — including
the generated-initials localhost URL, which only the deco path still emits
and which plausibly rendered through ffmpeg before.
That reroute is intended (routing by what the path is beats routing by
provenance, which would drift), so the fix is to say so accurately rather
than to narrow the guard. Also records the accepted per-frame cost
asymmetry the entry previously argued on correctness grounds alone.
The guard is extracted as CanUseFFmpegNativeWatermark so it can be tested
directly — review's highest-value gap was that the half of the fix which
decides whether pixels appear had no automated coverage, only the one-off
live E2E. Nine cases pin it, including the localhost-fallback reroute.
Both deferrals now point at real issues instead of an unverifiable
"tracked separately": #510 (deco vs precedence-level missing-logo policy)
and #511 (remote-fetch hardening — timeout, size cap, redirects, pooling,
caching, SSRF).
Also pins scheme-case insensitivity in the selector.
A channel whose logo is an external URL never rendered a watermark, even
with an ImageSource=ChannelLogo watermark attached. WatermarkSelector
resolved the URL correctly and then existence-checked it on the
filesystem — File.Exists("https://…") is always false — so all three
precedence levels (playout item, channel, global) logged "Channel logo
no longer exists" and returned None. The channel editor advertises the
URL as winning over an uploaded logo, which was true for the guide
listing and silently false for the bug.
External artwork passes through rather than being downloaded into the
image cache: that is already the convention everywhere else (M3U, XMLTV,
SPA JSON all emit the raw URL), no fetch->SaveArtworkToCache glue exists,
and the render path does not need it — ImageElementBase.LoadImage already
fetches an http(s) path with HttpClient and decodes it for real pixel
dimensions.
A remote-URL watermark is therefore forced onto the graphics engine
instead of the ffmpeg-native shortcut, which would otherwise hand the URL
to ffprobe and ffmpeg as a bare -i argument, putting an unbounded network
fetch inside stream startup.
The three gated precedence levels now share one ChannelLogoWatermarkOptions
helper — the triplicated block is what let the defect exist three times
over. Scope held narrow: the generated-initials localhost fallback (#1)
stays disabled behind an explicit comment and a scope-guard test, and the
deco path keeps its own long-standing unchecked policy.
Verified by live-E2E against a real channel playout with an external-URL
logo: origin/main renders 0 logo pixels and logs the "no longer exists"
warning verbatim; this branch renders the logo in the expected region.
Whitespace-only reformatting in FFmpegLibraryProcessService.cs is the
fix-as-you-touch format gate on pre-existing violations, plus a BOM strip.
fixes#502
ci(508): move both `docker build` jobs off the `small` runner lane
Fixes#508. `small` is now git-only (ci-image-pin, docs-reminder,
decisions-guard); both heavy `build` jobs (docker-build image push +
ci-image toolchain buildx) move to ubuntu-latest so the lane can widen
to 4 slots across two hosts while committing less RAM than its single
10 GiB slot did. #574's skip-task queueing can't recur (build keeps
needs: [test, migrations]). Runner-side half: server-management#639.
Editing .gitea/workflows/ci-image.yml is itself a trigger path for ci-image.yml, so
the previous commit republished the toolchain image at its own sha. `ci-image-pin`
then correctly failed: the pin still named 07048b8 while
`git log -1 -- docker/ci .gitea/workflows/ci-image.yml` resolved to 4263cf79.
The image content is unchanged — that commit only moved the job's `runs-on:` — but
the pin's contract is "the pin equals the last commit that touched the image
sources", not "the bytes differ", so it has to move. Verified 4263cf7 is actually
published to the registry before pinning it; a pin that doesn't resolve to a real
image would break every `container:` job at once.
This is why the bump lands as a SEPARATE commit: the tag is the short sha of the
pushed branch tip, so a single commit could never contain its own sha. Splitting it
makes the branch self-consistent — the source-touching commit stays the last one to
touch those paths, and this one carries the matching pin.
refs #508
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`runs-on: small` carried two jobs that are not small: docker-build.yml's image
build, and ci-image.yml's toolchain buildx. The second reads as lightweight
because it is "docker-only, no toolchain needed — it *builds* the toolchain",
which is true and yet describes the heaviest job in the lane.
A lane's per-job memory cap is set by its worst member, not its median, so these
two pinned `small` at --memory=10g. On bumblebee's 25 GiB — also the prod media
host — that permits exactly ONE slot, and four jobs shared it. So "widen the
lane" and "keep the heavy jobs" were never simultaneously available.
The symptom that forces the issue is not queue wait. A saturated lane also wedges
DISPATCHED jobs in act's setup phase: >10 min in_progress, no log file written at
all, then failure, before Checkout runs. That is where "decisions.md is a known
flake, just rerun it" came from — the rerun works only because it lands after load
clears, so a capacity problem read as a bug in the guard.
With both builds on ubuntu-latest, `small` is a checkout plus a `git diff` and
server-management#639 caps it at 1 GiB, widening it to 4 slots across two hosts
while committing LESS RAM to CI than the single slot did.
so it cannot be dispatched until the jobs it would queue behind have finished.
refs #508
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The release range grew after the host-correction PR landed: #67 and #498 both
merged into main. #498 in particular flips a default (QsvPreferNativeDecoder is
ON), so it leads the row rather than sitting in a fixes list.
From the Fable whole-branch review:
- M1: guard native VA-API decode with !OperatingSystem.IsWindows() (no vaapi
hwaccel on Windows; QSV caps over-reported there) — DRY'd into a preferNativeDecode local.
- L2: IsIntelVaapiOrQsv also matches decoder mode Vaapi, preserving Intel
audio-dup parity on the (producerless) Nut-output branch.
- L1: replace the vacuous ShouldNotContain(" deinterlace_qsv") with an
occurrence-count assertion that actually catches a second bare occurrence.
- N1/H1: decisions.md — correct the column to nullable-with-default (not NOT NULL),
and record the accepted HDR software-tonemap trade-off + Linux-only guard, with
the tonemap_qsv optimization tracked in #505.
H1 (HDR tonemap reroute) accepted-and-deferred per that decision; #505 filed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Whitespace-only (git diff -w is empty); the #311 format job checks whole
touched files, and these two legacy files carried pre-existing violations
never caught before (no PR had touched them since the gate landed).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Field-reference update skipped: docs/channels.md and docs/domain-model.md
have no per-field FFmpeg-profile catalogue (channels.md's Encoding bullet
is a one-line summary, not a field list; grep for QsvExtraHardwareFrames
or HardwareAcceleration finds no such list in either doc).
- Blocker 1: VA-API decode to SOFTWARE frames (drop -hwaccel_output_format,
new DecoderVaapiToSoftware) so the proven hwupload/vpp_qsv branch bridges to
the QSV encoder — the naive hardware-surface path emits a bare vpp_qsv on
VA-API frames and fails on ~all content.
- Blocker 2: bool? domain property + != false coercion (DeinterlaceVideo
pattern) so create-with-false actually persists false.
- High 3: REST DTOs bool?=null + ?? true for /api/v1 additive-compat.
- Medium 4: correct Task 4 test scaffolding (DefaultHardwareCapabilities).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Mirrors Jellyfin's default-on "prefer native decoder" hybrid via a new
QsvPreferNativeDecoder profile boolean. Reverified against code before
designing; records the rejected decode-family-enum alternative (Option C).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Whole-branch review found the stamping test was structurally vacuous: a bare
foreach over ChannelTemplates.Where(IsSystem) passes with zero iterations, so
the test would have stayed green if template seeding silently bailed out.
Assert the collection is non-empty first. Proven non-vacuous by a negative
control (forcing SeedChannelTemplates to bail makes exactly this test fail).
Also cover the ACTUAL production sequence -- adopt an existing hand-made row,
then delete it -- which the previous no-resurrect test did not exercise (it
covered seed-then-delete). The marker is written on the adopt path too, so
the deleted row must stay deleted.
docs: note that a deleted preset degrades to no default rather than failing,
and that the default applies to newly created channels, not retroactively.
Refs #67
- Add ChannelEditScreen tests exercising the geometry-fetch/preview path (previously
untested because the fixture's blank logo path kept BugPreview from ever rendering):
asserts the fetched watermark geometry lands on the preview image with asymmetric
values, and that an external logo URL suppresses the preview.
- Fix a toggle-off/toggle-on data loss: re-enabling "use logo as on-screen bug" used to
always rebind to the default 'Channel Bug' preset, discarding a channel's own
per-geometry ChannelLogo preset. Remember the last referenced logo-driven watermark id
in a ref and prefer it, falling back to the shared default.
- Strip the geometry cache's `id` key before it reaches BugPreview's props (was leaking
via a spread).
- Drop the "(see #502)" issue reference from the visible help text; keep it in the code
comment.
- Constrain findLogoBugWatermark's imageSource param to the generated
ChannelWatermarkImageSource union instead of `string`, and add unit tests for its
preference/fallback/none-found behavior.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the raw <img> preview in the Watermarks editor's Image row with
the shared BugPreview component, so the on-screen bug's location/size/
margins/opacity render the same way as the other three preview spots.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
WatermarkResponseModel gains ImageSource so a client can identify
logo-driven presets generically instead of matching a user-editable name.
Additive under the frozen-additive /api/v1 contract (#286).
Adding a positional record parameter is source-breaking for existing
constructor call sites, so the two test files that built the DTO
positionally are updated. WatermarkHandlerTests now seeds its two rows with
DIFFERENT image sources so the round-trip assertion proves the field is
actually carried through the mapper rather than matching a constant on both.
Regenerated v1.json, endpoint-index.md and v1.d.ts; check:api clean.
Stripped the inherited UTF-8 BOM from Mapper.cs (#311 fix-as-you-touch).
Refs #67
timothy
