Files
ersatztv/ErsatzTV.Infrastructure.Tests/Streaming/Graphics/RemoteImageDecodeLimitTests.cs
T
timothy d4e112f1e9
Build ErsatzTV Image / CI image pin matches docker/ci (pull_request) Successful in 15s
Build ErsatzTV Image / Docs update reminder (pull_request) Successful in 13s
Build ErsatzTV Image / decisions.md append-only (pull_request) Successful in 16s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 17s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 20s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 7m34s
Build ErsatzTV Image / Functional E2E (curl contracts) (pull_request) Successful in 14m15s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 18m39s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
fix(511): bound the DECODER, not the header's frame count
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.
2026-07-21 01:04:25 +02:00

261 lines
10 KiB
C#

using System.Buffers.Binary;
using ErsatzTV.Infrastructure.Streaming.Graphics;
using NUnit.Framework;
using Shouldly;
using SixLabors.ImageSharp;
using SixLabors.ImageSharp.Formats.Png;
using SixLabors.ImageSharp.PixelFormats;
using Image = SixLabors.ImageSharp.Image;
namespace ErsatzTV.Infrastructure.Tests.Streaming.Graphics;
/// <summary>
/// The byte cap in <c>HttpRemoteImageFetcher</c> does not bound decoding: a decompression bomb
/// is tiny on the wire and enormous in memory. These pin the header-first check that does.
/// (ersatztv#511)
/// </summary>
[TestFixture]
public class RemoteImageDecodeLimitTests
{
private static readonly Uri ImageUri = new("https://example.com/logo.png");
[Test]
public async Task Should_Decode_A_Normal_Image()
{
await using MemoryStream stream = await RealPng(64, 32);
using Image image = await ImageElementBase.DecodeRemoteImage(stream, ImageUri, CancellationToken.None);
image.Width.ShouldBe(64);
image.Height.ShouldBe(32);
}
// the bomb: a few dozen bytes on the wire, ~3.6 GB if decoded. it sails through the byte cap,
// the content-type check and the Content-Length reject -- only the header dimensions catch it.
[Test]
public async Task Should_Reject_An_Image_Whose_Declared_Dimensions_Are_A_Decompression_Bomb()
{
await using MemoryStream stream = PngHeaderDeclaring(30000, 30000);
stream.Length.ShouldBeLessThan(100, "the point is that this is tiny on the wire");
InvalidOperationException ex = await Should.ThrowAsync<InvalidOperationException>(
() => ImageElementBase.DecodeRemoteImage(stream, ImageUri, CancellationToken.None));
ex.Message.ShouldContain("pixel limit");
}
[Test]
public async Task Should_Accept_Dimensions_Exactly_At_The_Limit()
{
// 10000 x 5000 = 50,000,000 -- exactly the budget, so it must NOT be rejected. the decode
// then fails on the truncated body, which proves the check let it through. NOTE this test
// would also pass with the guard deleted entirely; deletion is covered by the bomb test
// above, and the boundary arithmetic by EnsureDecodeAffordable's own tests.
await using MemoryStream stream = PngHeaderDeclaring(10000, 5000);
Exception ex = await Should.ThrowAsync<Exception>(
() => ImageElementBase.DecodeRemoteImage(stream, ImageUri, CancellationToken.None));
ex.Message.ShouldNotContain("pixel limit");
}
// --- the budget policy itself, tested as arithmetic so no multi-GB image is ever allocated ---
// THE bomb the first fix missed: 2500x2500 x600 frames is ~60 KiB on the wire, passes a
// dimensions-only check (6.25 MP) AND a frames-only check (exactly 600), and costs ~14 GiB to
// decode. Only the PRODUCT catches it. (Found by adversarial re-review; ersatztv#511.)
[Test]
public void Should_Reject_Dimensions_And_Frames_That_Are_Affordable_Alone_But_Not_Together()
{
const int Width = 2500;
const int Height = 2500;
const int Frames = 600;
// each guard, in isolation, says yes
((long)Width * Height).ShouldBeLessThanOrEqualTo(ImageElementBase.MaxRemoteDecodedPixels);
Frames.ShouldBeLessThanOrEqualTo(ImageElementBase.MaxRemoteFrames);
InvalidOperationException ex = Should.Throw<InvalidOperationException>(
() => ImageElementBase.EnsureDecodeAffordable(Width, Height, Frames, ImageUri));
ex.Message.ShouldContain("pixel limit");
}
// M1: the frame guard had no coverage at all in the first fix
[Test]
public void Should_Reject_Too_Many_Frames_Even_When_Each_Is_Tiny()
{
InvalidOperationException ex = Should.Throw<InvalidOperationException>(
() => ImageElementBase.EnsureDecodeAffordable(8, 8, ImageElementBase.MaxRemoteFrames + 1, ImageUri));
ex.Message.ShouldContain("frame limit");
}
[Test]
public void Should_Allow_A_Single_Large_Still_Within_Budget()
{
// 8K is ~33 MP -- must keep working
Should.NotThrow(() => ImageElementBase.EnsureDecodeAffordable(7680, 4320, 1, ImageUri));
}
[Test]
public void Should_Allow_A_Typical_Animated_Logo()
{
Should.NotThrow(() => ImageElementBase.EnsureDecodeAffordable(288, 288, 600, ImageUri));
}
[Test]
public void Should_Afford_Fewer_Frames_As_Frames_Get_Larger()
{
// tiny frames are capped by the frame guard, not the pixel budget
ImageElementBase.AffordableFrames(8, 8).ShouldBe(ImageElementBase.MaxRemoteFrames);
// 1000x1000 -> 50M / 1M = 50 frames
ImageElementBase.AffordableFrames(1000, 1000).ShouldBe(50);
// a frame so large only one fits
ImageElementBase.AffordableFrames(7000, 7000).ShouldBe(1);
}
[Test]
public void Should_Allow_A_Product_Exactly_At_The_Budget()
{
// 10000 x 5000 x 1 == MaxRemoteDecodedPixels exactly
Should.NotThrow(() => ImageElementBase.EnsureDecodeAffordable(10000, 5000, 1, ImageUri));
}
// the retained-frame budget is INDEPENDENT of the decode budget: this source is trivial to
// decode (6 MP total) but retains ~5 GB of SKBitmap once every frame is scaled to 1080p
[Test]
public void Should_Reject_Cheap_Frames_That_Are_Expensive_Once_Scaled()
{
Should.NotThrow(() => ImageElementBase.EnsureDecodeAffordable(100, 100, 600, ImageUri));
InvalidOperationException ex = Should.Throw<InvalidOperationException>(
() => ImageElementBase.EnsureScaledFramesAffordable(600, 1920, 1080, ImageUri));
ex.Message.ShouldContain("pixel limit");
}
[Test]
public void Should_Allow_A_Scaled_Watermark_Sized_Animation()
{
// a 10%-width logo on a 1080p frame, animated
Should.NotThrow(() => ImageElementBase.EnsureScaledFramesAffordable(600, 192, 108, ImageUri));
}
// --- B1 regression: the header's frame count is a LIE for APNG ---
// ImageSharp 3.1.12 reports FrameMetadataCollection.Count == 0 for an APNG while the decoder
// produces every frame. A budget derived from that header count is enforced on a number the
// decoder does not honor — this exact payload shape, at 4000x4000, is ~134 KiB on the wire and
// ~36 GiB decoded. The bound therefore has to be imposed ON THE DECODER (DecoderOptions.
// MaxFrames) and re-verified against the real frame count. (ersatztv#511, second re-review.)
[Test]
public async Task Should_Reject_An_Animation_Whose_Header_Under_Reports_Its_Frames()
{
await using MemoryStream stream = Apng(64, 64, ImageElementBase.MaxRemoteFrames + 100);
// the premise: the header really does under-report, so a header-derived budget waves it through
stream.Position = 0;
ImageInfo info = await Image.IdentifyAsync(stream);
info.FrameMetadataCollection.Count.ShouldBe(0, "the APNG header under-reports; that is the whole point");
Should.NotThrow(() => ImageElementBase.EnsureDecodeAffordable(64, 64, info.FrameMetadataCollection.Count, ImageUri));
stream.Position = 0;
InvalidOperationException ex = await Should.ThrowAsync<InvalidOperationException>(
() => ImageElementBase.DecodeRemoteImage(stream, ImageUri, CancellationToken.None));
ex.Message.ShouldContain("frame limit");
}
// ...and an animation within budget still decodes IN FULL -- the decoder cap must not silently
// truncate legitimate content by a frame
[Test]
public async Task Should_Decode_An_Animation_Within_Budget_Without_Truncating_It()
{
await using MemoryStream stream = Apng(64, 64, 300);
using Image image = await ImageElementBase.DecodeRemoteImage(stream, ImageUri, CancellationToken.None);
image.Frames.Count.ShouldBe(300);
}
/// <summary>A real multi-frame APNG. Small on the wire, many frames — the shape that matters.</summary>
private static MemoryStream Apng(int width, int height, int frames)
{
using var image = new Image<Rgba32>(width, height);
for (var i = 1; i < frames; i++)
{
image.Frames.CreateFrame();
}
var stream = new MemoryStream();
image.Save(stream, new PngEncoder { ColorType = PngColorType.RgbWithAlpha });
stream.Position = 0;
return stream;
}
// PNG chunk CRC-32 (IEEE, reflected). hand-rolled because the repo does not reference
// System.IO.Hashing, and ImageSharp validates the CRC of critical chunks like IHDR.
private static uint Crc32(ReadOnlySpan<byte> data)
{
uint crc = 0xFFFFFFFF;
foreach (byte b in data)
{
crc ^= b;
for (var i = 0; i < 8; i++)
{
crc = (crc & 1) != 0 ? (crc >> 1) ^ 0xEDB88320 : crc >> 1;
}
}
return crc ^ 0xFFFFFFFF;
}
/// <summary>A real, decodable PNG.</summary>
private static async Task<MemoryStream> RealPng(int width, int height)
{
using var image = new Image<Rgba32>(width, height);
var stream = new MemoryStream();
await image.SaveAsync(stream, new PngEncoder());
stream.Position = 0;
return stream;
}
/// <summary>
/// A PNG signature plus a single valid IHDR chunk declaring <paramref name="width" /> x
/// <paramref name="height" /> and nothing else — enough for Identify, far too little to
/// decode. This is what a decompression bomb looks like at the point we have to reject it.
/// </summary>
private static MemoryStream PngHeaderDeclaring(int width, int height)
{
var stream = new MemoryStream();
stream.Write([0x89, (byte)'P', (byte)'N', (byte)'G', 0x0D, 0x0A, 0x1A, 0x0A]);
var ihdr = new byte[17];
"IHDR"u8.CopyTo(ihdr);
BinaryPrimitives.WriteInt32BigEndian(ihdr.AsSpan(4), width);
BinaryPrimitives.WriteInt32BigEndian(ihdr.AsSpan(8), height);
ihdr[12] = 8; // bit depth
ihdr[13] = 6; // color type: truecolor + alpha
ihdr[14] = 0; // compression
ihdr[15] = 0; // filter
ihdr[16] = 0; // interlace
var length = new byte[4];
BinaryPrimitives.WriteInt32BigEndian(length, 13);
stream.Write(length);
stream.Write(ihdr);
var crc = new byte[4];
BinaryPrimitives.WriteUInt32BigEndian(crc, Crc32(ihdr));
stream.Write(crc);
stream.Position = 0;
return stream;
}
}