From 406c6f286048c3db8e7ed204d4935c18eaa56e1d Mon Sep 17 00:00:00 2001 From: Stefan Date: Thu, 10 Sep 2026 21:03:09 +0200 Subject: [PATCH] Fix invoice-image code-review findings: aspect ratio, whitelist config, stale refs - MigraDocExtensions: fix a bug where capping an oversized explicit width+height image re-derived the other dimension from the image's native pixel ratio instead of scaling both sides uniformly, silently distorting a deliberately non-native-ratio (e.g. stretched) image. - InvoiceService: derive the mfr-portal image whitelist prefix from the existing Fds:MFR_host config setting (via MFRClientConfig.RestRoot) instead of a second hardcoded literal, so it can't drift from the actual configured host; parse sources with HtmlAgilityPack (already used by the PDF's own HTML converter) instead of a regex, so a decoy attribute like data-src can't be picked up in place of the real src; dedup fetch URLs with a HashSet instead of O(n^2) List scanning; reuse FuchsPdf.MigraDocFilenameFromByteArray instead of re-spelling the "base64:" prefix. - Fix a stale log message and comment left over from the GenerateInvoicePdf -> GenerateInvoicePdfAsync rename, and update INVOICE_LIFECYCLE.md's references to the old sync method name. - Add tests for the config-derived whitelist, a decoy-attribute src, and the 20-image-per-invoice fetch cap boundary. Co-Authored-By: Claude Sonnet 5 --- Fuchs.Tests/InvoiceInlineImageTests.cs | 85 ++++++++++++++++--- ...-invoice-editor-images-inlined-into-pdf.md | 23 +++-- Fuchs/Docs/INVOICE_LIFECYCLE.md | 7 +- Fuchs/Services/InvoiceService.cs | 53 +++++++----- Fuchs/code/FuchsPdf.cs | 3 +- Fuchs/code/MigraDocExtensions.cs | 22 ++++- 6 files changed, 146 insertions(+), 47 deletions(-) diff --git a/Fuchs.Tests/InvoiceInlineImageTests.cs b/Fuchs.Tests/InvoiceInlineImageTests.cs index 4eda1f0..363c84a 100644 --- a/Fuchs.Tests/InvoiceInlineImageTests.cs +++ b/Fuchs.Tests/InvoiceInlineImageTests.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Linq; using System.Net; using System.Net.Http; using System.Threading; @@ -17,8 +18,9 @@ namespace Fuchs.Tests; /// /// Exercises — the whitelist-restricted image /// fetch that lets invoice-editor picture links (mfr portal only) appear in the rendered PDF -/// (ADR 0018). Covers the whitelist boundary, fetch failure modes (each must just drop that one -/// image, never throw), and the per-invoice fetch cap. +/// (ADR 0018). Covers the whitelist boundary (derived from the Fds:MFR_host config entry, +/// not a second hardcoded host), fetch failure modes (each must just drop that one image, never +/// throw), and the per-invoice fetch cap. /// public class InvoiceInlineImageTests { @@ -41,13 +43,20 @@ public class InvoiceInlineImageTests public HttpClient CreateClient(string name) => new(_handler, disposeHandler: false); } - private static InvoiceService MakeService(HttpMessageHandler handler) => new( + private static IConfiguration TestConfig(string mfrHost = "portal.mobilefieldreport.com") => + new ConfigurationBuilder().AddInMemoryCollection(new Dictionary + { + ["Fds:MFR_host"] = mfrHost + }).Build(); + + private static InvoiceService MakeService(HttpMessageHandler handler, string mfrHost = "portal.mobilefieldreport.com") => new( new Fuchs_intranet(new ConfigurationBuilder().Build()), Mock.Of(), Mock.Of(), Mock.Of(), Mock.Of(), new StubFactory(handler), + TestConfig(mfrHost), NullLogger.Instance); private static FdsInvoiceData InvoiceWithDesc(params string[] descs) @@ -63,15 +72,25 @@ public class InvoiceInlineImageTests private static HttpResponseMessage ImageResponse(byte[] bytes) => new(HttpStatusCode.OK) { Content = new ByteArrayContent(bytes) { Headers = { ContentType = new("image/png") } } }; + [Fact] + public void AllowedInlineImagePrefix_DerivedFromConfiguredMfrHost_NotHardcodedSecondTime() + { + var svc = MakeService(new StubHandler(_ => ImageResponse(Array.Empty())), + mfrHost: "other-mfr-host.example.com"); + + Assert.Equal("https://other-mfr-host.example.com/mfr/", svc.AllowedInlineImagePrefix); + } + [Fact] public async Task ResolveInlineImagesAsync_WhitelistedUrl_ReturnsBase64DataForOriginalSrc() { byte[] bytes = { 1, 2, 3, 4 }; var handler = new StubHandler(_ => ImageResponse(bytes)); - string url = InvoiceService.AllowedInlineImagePrefix + "pic1.png"; + var svc = MakeService(handler); + string url = svc.AllowedInlineImagePrefix + "pic1.png"; var invoice = InvoiceWithDesc($"

Before

After

"); - var result = await MakeService(handler).ResolveInlineImagesAsync(invoice); + var result = await svc.ResolveInlineImagesAsync(invoice); Assert.True(result.ContainsKey(url)); Assert.Equal("base64:" + Convert.ToBase64String(bytes), result[url]); @@ -94,6 +113,25 @@ public class InvoiceInlineImageTests Assert.Empty(handler.RequestedUrls); } + [Fact] + public async Task ResolveInlineImagesAsync_DecoyAttributeNextToRealSrc_FetchesOnlyRealSrc() + { + // A description containing something like data-src alongside the real src must never let + // the decoy be picked up in place of (or in addition to) the actual src attribute. + var handler = new StubHandler(_ => ImageResponse(new byte[] { 1 })); + var svc = MakeService(handler); + string real = svc.AllowedInlineImagePrefix + "real.png"; + string decoy = svc.AllowedInlineImagePrefix + "decoy.png"; + var invoice = InvoiceWithDesc($""); + + var result = await svc.ResolveInlineImagesAsync(invoice); + + Assert.Single(result); + Assert.True(result.ContainsKey(real)); + Assert.False(result.ContainsKey(decoy)); + Assert.DoesNotContain(decoy, handler.RequestedUrls); + } + [Fact] public async Task ResolveInlineImagesAsync_NoImages_SkipsHttpEntirely() { @@ -110,10 +148,11 @@ public class InvoiceInlineImageTests public async Task ResolveInlineImagesAsync_HttpErrorStatus_DropsImageWithoutThrowing() { var handler = new StubHandler(_ => new HttpResponseMessage(HttpStatusCode.NotFound)); - string url = InvoiceService.AllowedInlineImagePrefix + "missing.png"; + var svc = MakeService(handler); + string url = svc.AllowedInlineImagePrefix + "missing.png"; var invoice = InvoiceWithDesc($""); - var result = await MakeService(handler).ResolveInlineImagesAsync(invoice); + var result = await svc.ResolveInlineImagesAsync(invoice); Assert.Empty(result); } @@ -125,10 +164,11 @@ public class InvoiceInlineImageTests { Content = new ByteArrayContent(new byte[] { 1, 2 }) { Headers = { ContentType = new("text/html") } } }); - string url = InvoiceService.AllowedInlineImagePrefix + "notreally.png"; + var svc = MakeService(handler); + string url = svc.AllowedInlineImagePrefix + "notreally.png"; var invoice = InvoiceWithDesc($""); - var result = await MakeService(handler).ResolveInlineImagesAsync(invoice); + var result = await svc.ResolveInlineImagesAsync(invoice); Assert.Empty(result); } @@ -137,10 +177,11 @@ public class InvoiceInlineImageTests public async Task ResolveInlineImagesAsync_ThrowingHandler_DropsImageWithoutThrowing() { var handler = new StubHandler(_ => throw new HttpRequestException("network down")); - string url = InvoiceService.AllowedInlineImagePrefix + "boom.png"; + var svc = MakeService(handler); + string url = svc.AllowedInlineImagePrefix + "boom.png"; var invoice = InvoiceWithDesc($""); - var result = await MakeService(handler).ResolveInlineImagesAsync(invoice); + var result = await svc.ResolveInlineImagesAsync(invoice); Assert.Empty(result); } @@ -149,12 +190,30 @@ public class InvoiceInlineImageTests public async Task ResolveInlineImagesAsync_DuplicateSrcAcrossLines_FetchedOnce() { var handler = new StubHandler(_ => ImageResponse(new byte[] { 9 })); - string url = InvoiceService.AllowedInlineImagePrefix + "shared.png"; + var svc = MakeService(handler); + string url = svc.AllowedInlineImagePrefix + "shared.png"; var invoice = InvoiceWithDesc($"", $""); - var result = await MakeService(handler).ResolveInlineImagesAsync(invoice); + var result = await svc.ResolveInlineImagesAsync(invoice); Assert.Single(result); Assert.Single(handler.RequestedUrls); } + + [Fact] + public async Task ResolveInlineImagesAsync_MoreThanCapDistinctImages_StopsAtCap() + { + var handler = new StubHandler(_ => ImageResponse(new byte[] { 1 })); + var svc = MakeService(handler); + // 25 distinct URLs on one invoice; the 20-image-per-invoice cap must stop fetching at 20. + var descs = Enumerable.Range(1, 25) + .Select(i => $"") + .ToArray(); + var invoice = InvoiceWithDesc(descs); + + var result = await svc.ResolveInlineImagesAsync(invoice); + + Assert.Equal(20, result.Count); + Assert.Equal(20, handler.RequestedUrls.Count); + } } diff --git a/Fuchs/Docs/Decisions/0018-invoice-editor-images-inlined-into-pdf.md b/Fuchs/Docs/Decisions/0018-invoice-editor-images-inlined-into-pdf.md index 932b923..52e1591 100644 --- a/Fuchs/Docs/Decisions/0018-invoice-editor-images-inlined-into-pdf.md +++ b/Fuchs/Docs/Decisions/0018-invoice-editor-images-inlined-into-pdf.md @@ -28,17 +28,24 @@ user-supplied URL server-side is also an SSRF surface — the invoice text is us so a naive "download whatever `src` says" would let it reach internal/private network addresses. ## Decision -- Invoice images are inlined into the PDF only when their `src` starts with the literal prefix - `https://portal.mobilefieldreport.com/mfr/` (`InvoiceService.AllowedInlineImagePrefix`) — the - only origin the invoice editor's picture links come from. Anything else (any other host, `http://`, - a different path) is left unrendered, exactly as before this feature — no generic remote fetch. +- Invoice images are inlined into the PDF only when their `src` starts with the mfr REST root + (`InvoiceService.AllowedInlineImagePrefix`, e.g. `https://portal.mobilefieldreport.com/mfr/`) — + the only origin the invoice editor's picture links come from. This is derived from the same + `Fds:MFR_host` configuration entry the MFR REST client itself uses (via + `MFR_RESTClient.MFRClientConfig.RestRoot`), not a second hardcoded literal, so it can't drift if + the configured mfr host ever changes per environment/tenant. Anything else (any other host, + `http://`, a different path) is left unrendered, exactly as before this feature — no generic + remote fetch. - The fetch happens once per invoice render, in `InvoiceService.ResolveInlineImagesAsync` (async, off a named `IHttpClientFactory` client `"InvoiceInlineImages"`, 10s timeout), *before* the synchronous MigraDoc render starts. `InvoiceService.GenerateInvoicePdf` therefore became - `GenerateInvoicePdfAsync` — every caller already ran in an async context. Each fetched image is - capped at 5MB, must report an `image/*` content-type, and the invoice fetches at most 20 distinct - images; failures (unreachable, wrong type, oversized) are logged and just leave that one image out - — they never fail the whole render. + `GenerateInvoicePdfAsync` — every caller already ran in an async context. `` sources are + found by parsing the description HTML with HtmlAgilityPack (`ResolveInlineImagesAsync`) — the + same library the `HtmlConverter` itself parses the identical text with — rather than a regex, so + a decoy attribute next to the real `src` (e.g. `data-src="..."`) can't be picked up in its place. + Each fetched image is capped at 5MB, must report an `image/*` content-type, and the invoice + fetches at most 20 distinct images; failures (unreachable, wrong type, oversized) are logged and + just leave that one image out — they never fail the whole render. - The resolved `{src → "base64:..."}` map flows down as a plain `IReadOnlyDictionary?` parameter through `IPdfService.ApplyInvoice` → `FuchsPdf.ApplyInvoice` → `Cell.AddHtml` into the `HtmlConverter`, whose new `img` handler only emits an image for a `src` present in that map — diff --git a/Fuchs/Docs/INVOICE_LIFECYCLE.md b/Fuchs/Docs/INVOICE_LIFECYCLE.md index 2c907cf..ed64297 100644 --- a/Fuchs/Docs/INVOICE_LIFECYCLE.md +++ b/Fuchs/Docs/INVOICE_LIFECYCLE.md @@ -116,7 +116,7 @@ vat }` plus set header/member tagging) that is posted to the server. | User action | JS entry point | Endpoint | Controller handler | Effect | |---|---|---|---|---| | First save without preview | `$inv.ssave` | `req/save` | `Do_Process_Requests` case `save` | `RegisterInvoiceAsync(change: id present)` — creates or updates the draft row, **no PDF rendered** | -| Preview a **new** invoice | `$inv.sprev(false)` | `req/sprep` | case `sprep` | Registers draft, then `GenerateInvoicePdf` → renders preview images | +| Preview a **new** invoice | `$inv.sprev(false)` | `req/sprep` | case `sprep` | Registers draft, then `GenerateInvoicePdfAsync` → renders preview images | | Preview an **existing** draft | `$inv.sprev(true)` (aka `$inv.sedit`) | `req/sedit` | case `sedit` | Same as `sprep` but requires `id`, updates existing draft | | Cancel out of preview | dialog `cancel` handler | `req/sdel` | case `sdel` | `fds__remInvoice` — deletes the (unfinalised) draft row | @@ -134,7 +134,8 @@ JSON into SQL parameters and calls, in one batch: - **Existing draft**: `fds__setInvoice` (same parameter set, updates in place) → `fds__createInvoice_Details` again. -`GenerateInvoicePdf` (still in `InvoiceService`) builds `FuchsPdf.FdsTextBlocks` +`GenerateInvoicePdfAsync` (still in `InvoiceService`) builds `FuchsPdf.FdsTextBlocks`, +resolves any whitelisted mfr-portal images from the line-item text (ADR 0018), and calls `IPdfService.WriteLetterAsync` + `ApplyInvoice`, which internally uses `InvoiceSetPricing.Build(...)` to turn the posted item contract into ordered, priced/unpriced lines. While the invoice is **not yet final** @@ -160,7 +161,7 @@ sequenceDiagram C->>S: RegisterInvoiceAsync(invoice, change, invId) S->>DB: fds__createInvoice / fds__setInvoice + fds__createInvoice_Details DB-->>S: registered invoice row (Id, ...) - S->>S: GenerateInvoicePdf (draft=true -> watermark, no GiroCode) + S->>S: GenerateInvoicePdfAsync (draft=true -> watermark, no GiroCode) S-->>C: MigraDoc Document C-->>JS: {id, img[] (base64 pages), total} JS->>U: show PDF preview modal (confirm / cancel) diff --git a/Fuchs/Services/InvoiceService.cs b/Fuchs/Services/InvoiceService.cs index b58c18b..ce18178 100644 --- a/Fuchs/Services/InvoiceService.cs +++ b/Fuchs/Services/InvoiceService.cs @@ -1,10 +1,12 @@ using System.Data; using System.Diagnostics; -using System.Text.RegularExpressions; using Fuchs.intranet; using Fuchs.Notifications; using Fuchs.Observability; +using HtmlAgilityPack; +using MFR_RESTClient; using Microsoft.Data.SqlClient; +using Microsoft.Extensions.Configuration; using Microsoft.Extensions.Logging; using MigraDoc.DocumentObjectModel; using Newtonsoft.Json.Linq; @@ -26,19 +28,9 @@ public class InvoiceService : IInvoiceService /// Named client used to fetch invoice-text images. public const string InlineImageHttpClientName = "InvoiceInlineImages"; - /// - /// The only origin invoice-text <img> sources are fetched from for inline PDF rendering - /// (ADR 0018) — the mfr portal picture links the invoice editor already displays. Any other - /// src (including other remote hosts) is left unrendered, exactly as before this feature. - /// - internal const string AllowedInlineImagePrefix = "https://portal.mobilefieldreport.com/mfr/"; - private const int MaxInlineImageBytes = 5 * 1024 * 1024; private const int MaxInlineImagesPerInvoice = 20; - private static readonly Regex ImgSrcRegex = - new(@"]+src\s*=\s*[""']([^""']+)[""']", RegexOptions.IgnoreCase | RegexOptions.Compiled); - private readonly Fuchs_intranet _intranet; private readonly IPdfService _pdf; private readonly IBlobStorageService _blobStorage; @@ -47,9 +39,19 @@ public class InvoiceService : IInvoiceService private readonly IHttpClientFactory _httpClientFactory; private readonly ILogger _logger; + /// + /// The only origin invoice-text <img> sources are fetched from for inline PDF rendering + /// (ADR 0018) — the mfr portal picture links the invoice editor already displays. Any other + /// src (including other remote hosts) is left unrendered, exactly as before this feature. + /// Derived from the same Fds:MFR_host setting the MFR REST client itself uses (via + /// ), not a separately-hardcoded host, so it can't drift + /// from the actual configured mfr portal. + /// + internal readonly string AllowedInlineImagePrefix; + public InvoiceService(Fuchs_intranet intranet, IPdfService pdf, IBlobStorageService blobStorage, IEventService events, IERechnungService erechnung, IHttpClientFactory httpClientFactory, - ILogger logger) + IConfiguration configuration, ILogger logger) { _intranet = intranet; _pdf = pdf; @@ -58,6 +60,11 @@ public class InvoiceService : IInvoiceService _erechnung = erechnung; _httpClientFactory = httpClientFactory; _logger = logger; + + string? mfrHost = configuration["Fds:MFR_host"]; + AllowedInlineImagePrefix = string.IsNullOrWhiteSpace(mfrHost) + ? MFRClientConfig.DefaultHost + "/mfr/" + : new MFRClientConfig(mfrHost).RestRoot; } private string Conn => _intranet.Intranet__SQLConnectionString; @@ -284,7 +291,7 @@ public class InvoiceService : IInvoiceService using var act = FuchsTelemetry.StartActivity("invoice.render"); act?.SetTag("fuchs.invoice.id", invoice.Id); act?.SetTag("fuchs.invoice.draft", draft); - _logger.LogDebug("GenerateInvoicePdf id={Id} draft={Draft}", invoice.Id, draft); + _logger.LogDebug("GenerateInvoicePdfAsync id={Id} draft={Draft}", invoice.Id, draft); var reg = invoice.InvoiceRegistration; var tb = new FuchsPdf.FdsTextBlocks { @@ -297,7 +304,6 @@ public class InvoiceService : IInvoiceService AdminDatumValue = reg?.getString("DateCreated") is { Length: > 0 } dc ? DateTime.Parse(dc) : DateTime.Now }; var inlineImages = await ResolveInlineImagesAsync(invoice); - // WriteLetter is effectively synchronous; no Task.Run thread-hop (safe: no sync context in ASP.NET Core). var doc = await _pdf.WriteLetterAsync(tb, draft); doc.Info.Title = reg?.getString("InvoiceTitle") ?? ""; _pdf.ApplyInvoice(doc, tb, invoice, draft, inlineImages); @@ -315,17 +321,24 @@ public class InvoiceService : IInvoiceService internal async Task> ResolveInlineImagesAsync(FdsInvoiceData invoice) { var result = new Dictionary(); - var urls = new List(); + // Parsed with HtmlAgilityPack (already a project dependency, and what the PDF's own HTML + // converter parses this same text with) rather than a regex, so a decoy attribute like + // data-src="..." next to the real src can never be picked up in its place. + var urls = new HashSet(); foreach (var block in invoice.InvoiceBlocks) foreach (var item in block.Items) { string desc = item.nz("desc", ""); if (desc.Length == 0) continue; - foreach (Match m in ImgSrcRegex.Matches(desc)) + var doc = new HtmlDocument(); + doc.LoadHtml(desc); + var imgNodes = doc.DocumentNode.SelectNodes("//img"); + if (imgNodes == null) continue; + foreach (var node in imgNodes) { - string src = m.Groups[1].Value; - if (src.StartsWith(AllowedInlineImagePrefix, StringComparison.Ordinal) - && !urls.Contains(src) && urls.Count < MaxInlineImagesPerInvoice) + string src = node.GetAttributeValue("src", ""); + if (src.Length > 0 && src.StartsWith(AllowedInlineImagePrefix, StringComparison.Ordinal) + && urls.Count < MaxInlineImagesPerInvoice) urls.Add(src); } } @@ -354,7 +367,7 @@ public class InvoiceService : IInvoiceService _logger.LogWarning("ResolveInlineImagesAsync: {Url} image size {Size} out of bounds", url, bytes.Length); continue; } - result[url] = "base64:" + Convert.ToBase64String(bytes); + result[url] = FuchsPdf.MigraDocFilenameFromByteArray(bytes); } catch (Exception ex) { diff --git a/Fuchs/code/FuchsPdf.cs b/Fuchs/code/FuchsPdf.cs index 977ddf1..2a11c62 100644 --- a/Fuchs/code/FuchsPdf.cs +++ b/Fuchs/code/FuchsPdf.cs @@ -1166,7 +1166,8 @@ public static class FuchsPdf img.Top = ShapePosition.Top; } - private static string MigraDocFilenameFromByteArray(byte[] image) => + /// Formats bytes as a MigraDoc-recognized inline image source ("base64:..."). + internal static string MigraDocFilenameFromByteArray(byte[] image) => "base64:" + Convert.ToBase64String(image); private static byte[] ImageToByteArray(System.Drawing.Image img) diff --git a/Fuchs/code/MigraDocExtensions.cs b/Fuchs/code/MigraDocExtensions.cs index 6afb29a..e038651 100644 --- a/Fuchs/code/MigraDocExtensions.cs +++ b/Fuchs/code/MigraDocExtensions.cs @@ -129,8 +129,26 @@ namespace MigraDoc.Extensions.Html double? wPx = ReadPxDimension(n, "width"); double? hPx = ReadPxDimension(n, "height"); - if (wPx is > 0 && hPx is > 0) { img.LockAspectRatio = false; img.Width = PxToUnit(wPx.Value); img.Height = PxToUnit(hPx.Value); } - else if (wPx is > 0) img.Width = PxToUnit(wPx.Value); + if (wPx is > 0 && hPx is > 0) + { + // Both explicit: honour the HTML's (possibly non-native, e.g. a stretched banner) + // aspect ratio. Enforcing the cap must scale both dimensions by the same factor — + // re-enabling LockAspectRatio here would make MigraDoc recompute the other side from + // the image's *native* pixel ratio instead, silently discarding the explicit value. + img.LockAspectRatio = false; + img.Width = PxToUnit(wPx.Value); + img.Height = PxToUnit(hPx.Value); + double scale = Math.Min(1.0, Math.Min( + MaxInlineImageWidth.Centimeter / img.Width.Centimeter, + MaxInlineImageHeight.Centimeter / img.Height.Centimeter)); + if (scale < 1.0) + { + img.Width = Unit.FromCentimeter(img.Width.Centimeter * scale); + img.Height = Unit.FromCentimeter(img.Height.Centimeter * scale); + } + return para; + } + if (wPx is > 0) img.Width = PxToUnit(wPx.Value); else if (hPx is > 0) img.Height = PxToUnit(hPx.Value); if (img.Width > MaxInlineImageWidth) { img.LockAspectRatio = true; img.Width = MaxInlineImageWidth; }