fix(mailing): send real HTML with a text alternative, not bare text

[marcelo-maciel]

Problem

Every mail provider in the kit puts MailRequest.Body in the HTML slot:

// SmtpMailService
var builder = new BodyBuilder { HtmlBody = request.Body };

// SendGridMailService
MailHelper.CreateSingleEmail(from, to, request.Subject, request.Body, request.Body);
//                                                      ^ plainText   ^ html — the same string

But two callers pass plain text into it:

• UserPasswordService.ForgotPasswordAsync — $"Please reset your password using the following link: {resetPasswordUri}"
• UserRegisteredEmailHandler — $"Hi {@event.FirstName}, thanks for registering."

A bare URL inside a text/html part is not auto-linked by most clients (auto-linking is text/plain behaviour), so the password-reset link arrives as dead text and the user cannot complete the flow. I hit this on a real deployment: the reset mail landed with the URL unclickable.

Two more consequences of the same root cause:

• The welcome mail interpolates FirstName — user-supplied — straight into markup. A name containing < breaks the message; it is an HTML injection into the rendered mail.
• SendGrid received Body as both parts, so the confirmation and billing templates were shipped as the text/plain alternative too: a text-only client rendered raw markup.

The confirmation mail (BuildConfirmationEmailHtml) and the billing bodies were already correct HTML with escaping — the defect is the inconsistency, not the templates.

Solution

MailRequest gains an optional TextBody (appended last, so existing positional calls keep compiling):

• SmtpMailService sets both HtmlBody and TextBody → MailKit emits multipart/alternative.
• SendGridMailService maps TextBody → plainTextContent and Body → htmlContent, instead of sending the HTML as both.
• New EmailBodies helper in Identity (LinkActionHtml, NoticeHtml) renders the action link as a real <a href> and HTML-encodes every interpolated value.
• Reset, confirmation, welcome and the four billing mails now all carry a text/plain twin, so nothing goes out HTML-only.

Note for reviewers: inside HTML the query separator is &, so the reset URL in the HTML part reads ...?token=…&email=…&tenant=…. That is correct per the HTML spec — the browser hands & to the server. The verbatim URL lives in TextBody, which is where the existing link-shape test now asserts.

Changes to src/BuildingBlocks (Golden Rule #4, requesting sign-off)

• Mailing/MailRequest.cs — new optional TextBody property + XML docs stating that Body is HTML.
• Mailing/Services/SmtpMailService.cs — one line: TextBody on the BodyBuilder.
• Mailing/Services/SendGridMailService.cs — plainTextContent now comes from TextBody.

No behaviour change for a caller that does not set TextBody, except on SendGrid, where the text part becomes absent instead of being a copy of the markup.

Tests

• UserPasswordServiceTests — the reset link is a real anchor; & in the HTML part; a text alternative exists, carries the verbatim URL and no markup. The pre-existing link-shape test (single slash, tenant, %2B encoding) now asserts on TextBody.
• UserRegisteredEmailHandlerTests (new) — a first name of <script>alert(1)</script> comes out encoded; the text alternative is present; nothing is sent when the event carries no e-mail.
• SendGridMailServiceTests — the two bodies land in their own MIME parts (text/html / text/plain).
• MailRequestTests — TextBody round-trips and defaults to null.

Verified locally on the pushed tree, with the NuGet audit on rather than disabled:

• dotnet restore src/FSH.Starter.slnx: exit 0, zero NU1903.
• dotnet build -warnaserror: exit 0.
• Full suite: 14 assemblies, 1806 passed / 0 failed / 1 skipped, including Integration at 746 passed / 1 skipped against a real Postgres (Testcontainers).

That closes the gap left in the earlier description, which said the integration suite would not run here and leaned on CI for it. The fault was local and is cleared; the suite ran end to end this time. It remains true that no integration test exercises the mail path itself — the harness does not run enqueued mail jobs — so the mail assertions are the unit tests listed above.

Rebased on main, and the SSH.NET pin

This branch was CONFLICTING. main has since added the System.Security.Cryptography.Xml 10.0.10 pin that this PR was carrying, which was the only conflict. Resolved by keeping main's version, so this PR no longer touches that pin at all — one less shared-config edit to review.

The remaining red was a different advisory: NU1903 / GHSA-q939-rpr3-3284 on SSH.NET 2025.1.0, pulled transitively by Testcontainers, which fails restore for the whole solution under TreatWarningsAsErrors — on main too, re-verified today at 3f2959e6. The fix belongs to #1333, still open. Rather than leave an approved PR red on someone else's advisory, the pin is carried here byte-identical to #1333's version of the file, comment included (same blob), so both stay mergeable in either order and this copy can be dropped once #1333 lands.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[marcelo-maciel]

Pushed bb89a24f: cherry-picked the System.Security.Cryptography.Xml 10.0.10 pin so this branch can actually be evaluated. Without it every backend job dies in restore on NU1903, which is failing main today rather than anything in this diff.

The same pin is in #1323, so whichever lands first leaves the other with a one-hunk conflict in Directory.Packages.props — happy to drop it here the moment #1323 (or your own pin) merges.

Beyond unblocking CI, this matters for review confidence: I could not run the integration suite locally (dotnet test returns instantly with no output in that worktree — an environment fault on my machine), so the CI run is the only place it executes.

[marcelo-maciel]

Docs + changelog for this change (Golden Rule #10): fullstackhero/docs#239 — the Mailing page gets the TextBody signature, a correction to the SendGrid sentence (it no longer receives Body as both parts) and a caution about bare text in Body, plus a changelog entry.

CI here is green now that the NU1903 pin is on the branch: Backend CI, Frontend CI, Unit, Integration (6m26s), Coverage Gate, DbMigrator Smoke, both Scaffold jobs and CodeQL all pass.

[iammukeshm]

Approving. Good catch on a bug that had been sitting in the kit since the mail services were written.

The diagnosis is the valuable part: the defect isn't in either template, it's that MailRequest.Body had no documented contract, so two callers reasonably passed text into what every provider treats as the HTML slot. Adding TextBody and the XML doc stating Body is HTML fixes the instance and the cause. SendGrid receiving the markup as both parts is a real second bug that would never have been noticed without pulling this thread.

The & note in the description is correct and worth keeping in the history — entity-encoding the query separator inside an href is right per spec, and putting the verbatim URL in TextBody (where the link-shape test now asserts) is the correct place for it.

The <script>alert(1)</script> first-name test is the one I'd have asked for if it weren't already there.

One required change before merge — but not the one you flagged

Drop the System.Security.Cryptography.Xml pin from this branch. Your heads-up about main being red on NU1903 is stale — the pin has since landed on main (src/Directory.Packages.props, currently at 10.0.10 with a superset of the advisories you list, five rather than four). Re-adding it here produces a duplicate PackageVersion item on rebase. Rebase onto main and drop the entry; nothing else in this PR depends on it, and it means this branch is not blocked by #1323 after all.

nit (non-blocking, follow-up welcome)

EmailBodies (Identity) and BillingEmailBodies.Wrap (Notifications) are now two independent HTML shells with their own Escape implementations — WebUtility.HtmlEncode in one, a hand-rolled four-Replace chain in the other. They'll drift, and the hand-rolled one is the weaker of the two. Not blocking this PR, but the shell and the encoder belong in BuildingBlocks/Mailing so every module gets the same escaping. Happy to take that as a separate PR if you want it.

BuildingBlocks sign-off (Golden Rule #4)

Granted for the three files listed. They're minimal and additive: one optional property appended last so positional call sites keep compiling, and one line in each provider. The only behaviour change for an unmodified caller is on SendGrid, where the text part becomes absent instead of being a copy of the markup — which is the bug being fixed, and strictly better than shipping raw HTML to a text client.

Noted that the integration suite didn't run locally and that no integration test covers the mail path today (the harness doesn't drain enqueued mail jobs). The unit coverage here is the right level for this change; CI will cover the rest.

[marcelo-maciel]

Unblocked. Pushed bada7cee: main merged in, plus the SSH.NET pin. No behaviour change since your approval.

Your one required change is done. You asked to drop the System.Security.Cryptography.Xml pin and rebase, since it had landed on main with a superset of the advisories and would produce a duplicate PackageVersion. That entry was in fact the only conflict, and it is resolved by keeping main's version — this PR no longer carries that pin. Confirmed there is no duplicate PackageVersion Include in the resolved file. You were right that #1323 was never a blocker here.

One line does come back into that file, for a different advisory. After the merge, restore still failed on NU1903 / GHSA-q939-rpr3-3284 — SSH.NET 2025.1.0, transitive via Testcontainers. That fails main too: dotnet restore src/FSH.Starter.slnx at 3f2959e6 exits 1 with it. The fix belongs to #1333, which is still open, so the pin is carried here byte-identical to that PR's version of the file (same blob), keeping both mergeable in either order. It comes back out once #1333 lands. Say the word if you would rather this PR sit red and wait for #1333 instead.

The description's claim that the integration suite would not run locally is no longer true, and I have replaced it with real numbers. That fault was on my machine and is cleared. Full run on the pushed tree, with the NuGet audit on rather than disabled:

• dotnet restore: exit 0, zero NU1903.
• dotnet build -warnaserror: exit 0.
• 14 assemblies, 1806 passed / 0 failed / 1 skipped, including Integration at 746 passed / 1 skipped against a real Postgres.

Worth flagging given what landed on main in between: the outbox rework touches the same Notifications integration-event handlers this PR edits. The merge was textually clean there, but that is not proof of semantic compatibility, so the integration run above is the evidence rather than the merge itself. Your note still holds that no integration test exercises the mail path itself — the harness does not drain enqueued mail jobs — so the mail assertions remain the unit tests.

On the non-blocking nit: agreed that EmailBodies and BillingEmailBodies.Wrap should not be two shells with two different escapers, and that WebUtility.HtmlEncode is the one to keep. Deliberately not doing it here so this PR stays the diff you approved. I will follow up separately.

Docs companion fullstackhero/docs#239 is MERGEABLE / CLEAN.

[marcelo-maciel]

Reopened as #1380.

This PR was closed automatically on 2026-09-14, when the head fork was deleted.
The branch and the commits are unchanged; the head commit is still bada7ceeea0a11097ab9deab93374e92d5317650.