StefanandClaude Sonnet 5
f407341f5f
- InvoiceService.ResolveInlineImagesAsync now fetches an invoice's distinct whitelisted images concurrently (SemaphoreSlim-capped at 4) instead of one at a time. It ran inline on every invoice render, including repeated draft-preview renders, so N sequential mfr-portal round-trips could otherwise add N x latency to a user-facing request. - Add fuchs.invoice.inlineimages.fetches (counter, tagged by outcome: success/http_error/bad_content_type/oversized/exception) and fuchs.invoice.inlineimages.resolve.duration (histogram) to FuchsTelemetry, matching the project's observability convention for outbound-HTTP operations. - MigraDocExtensions.HtmlConverter's pre-resolved-image parameter is renamed inlineImages -> resolvedImages with generic docs: the converter fetches nothing and knows nothing about invoices — which images to resolve, and any whitelist/network policy, is entirely the caller's concern (InvoiceService, ADR 0018 today; any future caller tomorrow). - Add tests asserting fetches actually overlap (not just faster) and that the new counter/histogram are recorded. - Update ADR 0018 to describe the concurrent fetch, telemetry, and the generalized converter parameter. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
84 lines
5.8 KiB
Markdown
84 lines
5.8 KiB
Markdown
---
|
||
status: Accepted
|
||
date: 2026-09-10
|
||
applyTo:
|
||
- "Fuchs/code/MigraDocExtensions.cs"
|
||
- "Fuchs/code/FuchsPdf.cs"
|
||
- "Fuchs/Services/InvoiceService.cs"
|
||
- "Fuchs/Services/IInvoiceService.cs"
|
||
- "Fuchs/Services/IPdfService.cs"
|
||
- "Fuchs/Services/FuchsPdfService.cs"
|
||
supersededBy: ""
|
||
---
|
||
|
||
# 0018 — Invoice-text images are fetched at render time from a domain whitelist, not embedded as data URIs
|
||
|
||
## Context
|
||
The invoice line-item description is TinyMCE-authored HTML (`FdsInvoiceData`/`InvoiceBlock.Items`
|
||
item `desc`), and can contain `<img src="https://portal.mobilefieldreport.com/mfr/...">` picture
|
||
links pulled from the mfr ERP — the browser resolves and displays them in the editor. The PDF
|
||
pipeline, however, renders through a hand-rolled HTML→MigraDoc subset converter
|
||
(`MigraDoc.Extensions.Html.HtmlConverter` in `MigraDocExtensions.cs`), which had no `img` handler
|
||
at all: every `<img>`, remote or embedded, was silently dropped, so images the editor showed never
|
||
appeared on the actual invoice.
|
||
|
||
MigraDoc/PdfSharp does not fetch remote URLs itself, and the converter runs synchronously deep
|
||
inside the largely-synchronous `FuchsPdf` static rendering pipeline. Fetching an arbitrary
|
||
user-supplied URL server-side is also an SSRF surface — the invoice text is user/editor-controlled,
|
||
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 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. `<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,
|
||
counted (`FuchsTelemetry.InvoiceInlineImageFetches`, tagged by outcome), and just leave that one
|
||
image out — they never fail the whole render. The distinct URLs fetch concurrently (capped at 4
|
||
in flight via a `SemaphoreSlim`, not one-at-a-time) since this runs inline on every render,
|
||
including repeated draft-preview renders — total resolve time is recorded in
|
||
`FuchsTelemetry.InvoiceInlineImagesResolveDuration`.
|
||
- 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 —
|
||
it does no fetching itself and never sees the network. The converter itself stays generic (its
|
||
constructor parameter is named `resolvedImages`, not invoice-specific) — resolving *which* images
|
||
go in the map, and any network/whitelist policy, is entirely InvoiceService's concern, not the
|
||
converter's. Image sizing mirrors the HTML: the handler reads `width`/`height` (attribute or
|
||
inline `style`, as TinyMCE's resize handles write them) and converts CSS px → cm, capped to
|
||
7×10cm so a malformed/huge size can't blow out the invoice table layout.
|
||
|
||
## Consequences
|
||
- Any future host serving invoice-editor images must be added to the whitelist deliberately (a
|
||
one-line prefix, or a small allowed-prefixes list if a second origin appears) — this is not
|
||
meant to grow into a generic "fetch any URL" facility.
|
||
- `IInvoiceService.GenerateInvoicePdf` → `GenerateInvoicePdfAsync`, and
|
||
`IInvoiceDraftService.RenderPreview` → `RenderPreviewAsync` (same signature change, same reason)
|
||
are breaking signature changes; anything implementing/mocking these interfaces needs updating.
|
||
- Image src values not covered by the whitelist — including a stray non-mfr remote URL, or a raw
|
||
`data:` URI a user might paste in — still render as nothing, matching the pre-existing (silent)
|
||
drop behaviour. If embedded `data:` images need support later, that's a separate, simpler
|
||
decision (no network fetch involved) and can reuse the same `img` handler contract.
|
||
|
||
## Alternatives considered
|
||
- **Fetch inside the HTML converter itself (sync-over-async `HttpClient.GetAsync(...).Result`)** —
|
||
rejected: violates the project's no-sync-over-async rule and keeps SSRF-relevant network code
|
||
inside a generic, reusable HTML converter instead of a single audited call site.
|
||
- **Fully embed images as `data:` base64 URIs at editor save time** — rejected: doubles storage
|
||
size per invoice edit, and the image is already resolved by the mfr portal itself; re-fetching a
|
||
small number of images once at render time is simpler and keeps the source of truth external.
|
||
- **Allow any remote `http(s)://` host** — rejected on SSRF grounds; every invoice image the editor
|
||
can currently produce is from the mfr portal, so a whitelist costs nothing today.
|