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 <img> 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 <noreply@anthropic.com>
2026-09-10 21:03:09 +02:00
parent c182d4fd69
commit 406c6f2860
6 changed files with 146 additions and 47 deletions
@@ -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. `<img>` 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<string,string>?`
  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 —
@@ -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)
@@ -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
    /// <summary>Named <see cref="IHttpClientFactory"/> client used to fetch invoice-text images.</summary>
    public const string InlineImageHttpClientName = "InvoiceInlineImages";

-    /// <summary>
-    /// The only origin invoice-text &lt;img&gt; 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.
-    /// </summary>
-    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(@"<img[^>]+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<InvoiceService> _logger;

+    /// <summary>
+    /// The only origin invoice-text &lt;img&gt; 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 <c>Fds:MFR_host</c> setting the MFR REST client itself uses (via
+    /// <see cref="MFRClientConfig.RestRoot"/>), not a separately-hardcoded host, so it can't drift
+    /// from the actual configured mfr portal.
+    /// </summary>
+    internal readonly string AllowedInlineImagePrefix;
+
    public InvoiceService(Fuchs_intranet intranet, IPdfService pdf, IBlobStorageService blobStorage,
        IEventService events, IERechnungService erechnung, IHttpClientFactory httpClientFactory,
-        ILogger<InvoiceService> logger)
+        IConfiguration configuration, ILogger<InvoiceService> 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<Dictionary<string, string>> ResolveInlineImagesAsync(FdsInvoiceData invoice)
    {
        var result = new Dictionary<string, string>();
-        var urls = new List<string>();
+        // 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<string>();
        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)
            {
@@ -1166,7 +1166,8 @@ public static class FuchsPdf
        img.Top                = ShapePosition.Top;
    }

-    private static string MigraDocFilenameFromByteArray(byte[] image) =>
+    /// <summary>Formats bytes as a MigraDoc-recognized inline image source ("base64:...").</summary>
+    internal static string MigraDocFilenameFromByteArray(byte[] image) =>
        "base64:" + Convert.ToBase64String(image);

    private static byte[] ImageToByteArray(System.Drawing.Image img)
@@ -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; }