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.
180 lines
7.8 KiB
C#
180 lines
7.8 KiB
C#
using System.Buffers;
|
|
using System.Net;
|
|
using System.Net.Http.Headers;
|
|
using ErsatzTV.Infrastructure.Streaming;
|
|
using Microsoft.Extensions.Logging;
|
|
using Microsoft.IO;
|
|
using NSubstitute;
|
|
using NUnit.Framework;
|
|
using Shouldly;
|
|
|
|
namespace ErsatzTV.Infrastructure.Tests.Streaming;
|
|
|
|
[TestFixture]
|
|
public class HttpRemoteImageFetcherTests
|
|
{
|
|
private static readonly Uri ImageUri = new("https://example.com/logo.png");
|
|
|
|
[Test]
|
|
public async Task Should_Return_The_Buffered_Body_On_Success()
|
|
{
|
|
byte[] payload = [1, 2, 3, 4, 5];
|
|
HttpRemoteImageFetcher fetcher = FetcherReturning(Ok(payload, "image/png"));
|
|
|
|
await using Stream result = await fetcher.Fetch(ImageUri, CancellationToken.None);
|
|
|
|
result.Position.ShouldBe(0);
|
|
var read = new byte[payload.Length];
|
|
await result.ReadExactlyAsync(read);
|
|
read.ShouldBe(payload);
|
|
}
|
|
|
|
[Test]
|
|
public async Task Should_Throw_On_An_Error_Status()
|
|
{
|
|
HttpRemoteImageFetcher fetcher = FetcherReturning(new HttpResponseMessage(HttpStatusCode.NotFound));
|
|
|
|
await Should.ThrowAsync<HttpRequestException>(() => fetcher.Fetch(ImageUri, CancellationToken.None));
|
|
}
|
|
|
|
// an html error page served with a 200 must never reach the decoder
|
|
[Test]
|
|
public async Task Should_Reject_A_Non_Image_Content_Type()
|
|
{
|
|
HttpRemoteImageFetcher fetcher = FetcherReturning(Ok([1, 2, 3], "text/html"));
|
|
|
|
await Should.ThrowAsync<InvalidOperationException>(() => fetcher.Fetch(ImageUri, CancellationToken.None));
|
|
}
|
|
|
|
// hosts that omit the header, and static file servers that default to octet-stream, are common
|
|
// enough that rejecting them would break working logos for no security gain -- ImageSharp
|
|
// decodes by magic bytes, and the wire-size + decode-budget caps are the real protection.
|
|
[TestCase(null)]
|
|
[TestCase("application/octet-stream")]
|
|
public async Task Should_Accept_A_Missing_Or_Generic_Content_Type(string mediaType)
|
|
{
|
|
HttpRemoteImageFetcher fetcher = FetcherReturning(Ok([1, 2, 3], mediaType));
|
|
|
|
await using Stream result = await fetcher.Fetch(ImageUri, CancellationToken.None);
|
|
|
|
result.Length.ShouldBe(3);
|
|
}
|
|
|
|
// the advertised length is the cheap reject: the body must not be pulled at all
|
|
[Test]
|
|
public async Task Should_Reject_An_Oversized_Content_Length_Without_Reading_The_Body()
|
|
{
|
|
var body = new TrackingStream(HttpRemoteImageFetcher.MaxImageBytes + 1);
|
|
var response = new HttpResponseMessage(HttpStatusCode.OK) { Content = new StreamContent(body) };
|
|
response.Content.Headers.ContentType = new MediaTypeHeaderValue("image/png");
|
|
response.Content.Headers.ContentLength = HttpRemoteImageFetcher.MaxImageBytes + 1;
|
|
|
|
HttpRemoteImageFetcher fetcher = FetcherReturning(response);
|
|
|
|
await Should.ThrowAsync<InvalidOperationException>(() => fetcher.Fetch(ImageUri, CancellationToken.None));
|
|
body.BytesRead.ShouldBe(0);
|
|
}
|
|
|
|
// ...and a host that lies about (or omits) Content-Length is still capped, because the copy
|
|
// itself counts bytes. (the DECODE bomb -- small on the wire, huge in memory -- is a different
|
|
// problem, bounded by the decode/retention budgets in ImageElementBase, not by this.)
|
|
[Test]
|
|
public async Task Should_Cap_A_Body_That_Does_Not_Advertise_Its_Length()
|
|
{
|
|
var body = new ChunkedZeroStream(HttpRemoteImageFetcher.MaxImageBytes * 2);
|
|
var response = new HttpResponseMessage(HttpStatusCode.OK) { Content = new StreamContent(body) };
|
|
response.Content.Headers.ContentType = new MediaTypeHeaderValue("image/png");
|
|
response.Content.Headers.ContentLength = null;
|
|
|
|
HttpRemoteImageFetcher fetcher = FetcherReturning(response);
|
|
|
|
await Should.ThrowAsync<InvalidOperationException>(() => fetcher.Fetch(ImageUri, CancellationToken.None));
|
|
|
|
// proves this went through the COPY cap and not the Content-Length early reject: the body
|
|
// really was read, and it stopped at the limit rather than draining all 20 MiB.
|
|
body.Position.ShouldBeGreaterThan(0);
|
|
|
|
// the over-read bound is ONE RENTED BUFFER, and ArrayPool.Rent(81920) actually hands back
|
|
// 131072 -- deriving it keeps this honest if the request size or the pool bucketing changes.
|
|
byte[] rented = ArrayPool<byte>.Shared.Rent(81920);
|
|
ArrayPool<byte>.Shared.Return(rented);
|
|
body.Position.ShouldBeLessThanOrEqualTo(HttpRemoteImageFetcher.MaxImageBytes + rented.Length);
|
|
}
|
|
|
|
// a host that accepts the connection and then hangs must not stall stream startup forever
|
|
[Test]
|
|
public async Task Should_Time_Out_A_Hanging_Host()
|
|
{
|
|
var fetcher = new HttpRemoteImageFetcher(
|
|
new StubHttpClientFactory(new HangingHttpMessageHandler()),
|
|
new RecyclableMemoryStreamManager(),
|
|
Substitute.For<ILogger<HttpRemoteImageFetcher>>(),
|
|
TimeSpan.FromMilliseconds(100));
|
|
|
|
await Should.ThrowAsync<TimeoutException>(() => fetcher.Fetch(ImageUri, CancellationToken.None));
|
|
}
|
|
|
|
|
|
// THE headline claim: under ResponseHeadersRead the body read falls outside HttpClient.Timeout,
|
|
// so a host that returns headers promptly and then drips the body must still hit our deadline.
|
|
// the hanging-host test above only covers the pre-headers case, which a plain HttpClient.Timeout
|
|
// would already bound -- this is the one that pins the actual design.
|
|
[Test]
|
|
public async Task Should_Time_Out_A_Slow_Drip_Body()
|
|
{
|
|
var response = new HttpResponseMessage(HttpStatusCode.OK)
|
|
{
|
|
Content = new StreamContent(new SlowDripStream())
|
|
};
|
|
response.Content.Headers.ContentType = new MediaTypeHeaderValue("image/png");
|
|
|
|
var fetcher = new HttpRemoteImageFetcher(
|
|
new StubHttpClientFactory(new FixedResponseHttpMessageHandler(response)),
|
|
new RecyclableMemoryStreamManager(),
|
|
Substitute.For<ILogger<HttpRemoteImageFetcher>>(),
|
|
TimeSpan.FromMilliseconds(200));
|
|
|
|
await Should.ThrowAsync<TimeoutException>(() => fetcher.Fetch(ImageUri, CancellationToken.None));
|
|
}
|
|
|
|
[Test]
|
|
public async Task Should_Throw_On_Transport_Failure()
|
|
{
|
|
var fetcher = new HttpRemoteImageFetcher(
|
|
new StubHttpClientFactory(new ThrowingHttpMessageHandler(new HttpRequestException("no route to host"))),
|
|
new RecyclableMemoryStreamManager(),
|
|
Substitute.For<ILogger<HttpRemoteImageFetcher>>());
|
|
|
|
await Should.ThrowAsync<HttpRequestException>(() => fetcher.Fetch(ImageUri, CancellationToken.None));
|
|
}
|
|
|
|
// caller cancellation (shutdown / client disconnect) is a genuine signal and must stay an
|
|
// OperationCanceledException rather than being relabelled as our timeout
|
|
[Test]
|
|
public async Task Should_Propagate_Caller_Cancellation()
|
|
{
|
|
var fetcher = new HttpRemoteImageFetcher(
|
|
new StubHttpClientFactory(new HangingHttpMessageHandler()),
|
|
new RecyclableMemoryStreamManager(),
|
|
Substitute.For<ILogger<HttpRemoteImageFetcher>>());
|
|
|
|
using var cts = new CancellationTokenSource();
|
|
await cts.CancelAsync();
|
|
|
|
await Should.ThrowAsync<OperationCanceledException>(() => fetcher.Fetch(ImageUri, cts.Token));
|
|
}
|
|
|
|
private static HttpResponseMessage Ok(byte[] payload, string mediaType)
|
|
{
|
|
var response = new HttpResponseMessage(HttpStatusCode.OK) { Content = new ByteArrayContent(payload) };
|
|
response.Content.Headers.ContentType = mediaType is null ? null : new MediaTypeHeaderValue(mediaType);
|
|
return response;
|
|
}
|
|
|
|
private static HttpRemoteImageFetcher FetcherReturning(HttpResponseMessage response) =>
|
|
new(
|
|
new StubHttpClientFactory(new FixedResponseHttpMessageHandler(response)),
|
|
new RecyclableMemoryStreamManager(),
|
|
Substitute.For<ILogger<HttpRemoteImageFetcher>>());
|
|
}
|