fix(mailing): send real HTML with a text alternative, not bare text - #1380
Open
marcelo-maciel wants to merge 12 commits into
Open
marcelo-maciel wants to merge 12 commits into
marcelo-maciel wants to merge 12 commits into
Conversation
Every provider puts MailRequest.Body in the HTML slot — MailKit's BodyBuilder.HtmlBody, SendGrid's htmlContent — but the password-reset and welcome mails passed plain text. A bare URL inside an HTML part is not auto-linked by most clients, so the reset link arrived as dead text and the user had no way to complete the flow. The welcome mail additionally interpolated the user-supplied first name straight into that HTML. MailRequest gains an optional TextBody carrying the text/plain alternative. SmtpMailService emits both parts as multipart/alternative; SendGridMailService stops passing Body as plainTextContent, which had been shipping raw markup to text-only clients. Identity builds its bodies through EmailBodies, which HTML-encodes every interpolated value, and billing bodies gained their plain twin so no message goes out HTML-only. Verified: build -warnaserror 0/0; unit suites green (Identity 317, Framework 122, Billing 123, and the rest).
The test hosts pull 10.0.8 transitively, which carries HIGH-severity advisories (GHSA-23rf-6693-g89p, GHSA-8q5v-6pqq-x66h, GHSA-cvvh-rhrc-wg4q, GHSA-g8r8-53c2-pm3f) and trips NuGetAudit under TreatWarningsAsErrors, breaking the build of every test project. 10.0.10 is the patched servicing release. Mirrors the existing Microsoft.OpenApi transitive pin.
# Conflicts: # src/Directory.Packages.props
…1333 is open `NU1903` / `GHSA-q939-rpr3-3284` on `SSH.NET` 2025.1.0, pulled transitively by Testcontainers, fails `restore` for the whole solution under `TreatWarningsAsErrors` — on `main` too. It is not introduced here and the fix belongs to fullstackhero#1333, which is still open. Carried byte-identical to fullstackhero#1333's version of the file, comment included, so both stay mergeable in either order and this copy can simply be dropped once fullstackhero#1333 lands.
…advisories `dotnet restore` fails for the whole solution under `TreatWarningsAsErrors`, on `main` and on every open PR alike. Advisory-database drift, not a regression from any change: a commit green on 2026-08-10 is red today with no edits. - `Testcontainers.PostgreSql` / `.Redis` / `.Minio` 4.11.0 -> 4.14.0 (NU1903, GHSA-q939-rpr3-3284). 4.11.0 depends on `SSH.NET` 2025.1.0; 4.14.0 already depends on the patched 2026.0.0, so the advisory clears with no transitive pin to remember to remove later. Same fix as fullstackhero#1369, so the two do not conflict. - `Microsoft.SourceLink.GitHub` 8.0.0 -> 10.0.401 (NU1902, GHSA-23fw-v26w-5fgq). 8.0.0 drags in `Microsoft.Build.Tasks.Git` 8.0.0 and the 8.x line has no patched release, so a transitive pin cannot fix it; the package itself has to move. 10.0.401 depends on `Microsoft.Build.Tasks.Git` 10.0.401, past the patched 10.0.303. Build-time only (`PrivateAssets="all"`), referenced only where `IsPackable == true`, which is the CLI alone - and `src/Tools/**` is excluded from the template, so the scaffold never sees it. Verified: `dotnet restore src/FSH.Starter.slnx` exits 0 with no NU19xx, and `dotnet build src/FSH.Starter.slnx -c Release -warnaserror` reports 0 warnings and 0 errors.
MinIO withdrew `minio/minio` from Docker Hub. Docker Hub's API now answers
`object not found` for the repository, and a pull fails with:
pull access denied for minio/minio, repository does not exist or may
require 'docker login'
That takes down every Testcontainers-backed integration test (the harness boots
a MinIO container per fixture, so all 724 tests in `Integration.Tests` fail at
container start), the Aspire AppHost, and the Docker Compose deployment. The
image is still published at `quay.io/minio/minio`:
- `Integration.Tests` and `Integration.Middleware.Tests` harnesses
- `AppHost.cs`, via Aspire's `WithImageRegistry` / `WithImageTag`
- `deploy/docker/docker-compose.yml` and the image table in its README
The tag is pinned to `RELEASE.2025-09-07T16-13-09Z` rather than `:latest`. quay
has not moved `:latest` since 2025-09-07, so the two resolve to the same digest
today; pinning only removes the surprise of a silent move later, and keeps the
test harness off a floating tag. Whether to track a newer release, or a different
S3-compatible image, is a separate call.
While in the README's image table: `postgres` and `redis` rows had drifted from
what compose actually ships (`postgres:18-alpine`, `valkey/valkey:9.1.0-alpine`).
Verified: `docker pull minio/minio:latest` fails with the error above;
`docker pull quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z` succeeds
(`sha256:14cea493d9a34af32f524e538b8346cf79f3321eff8e708c1e2960462bd8936e`, the
same digest `:latest` resolves to). `dotnet test Integration.Tests -c Release`
passes against the pinned image, and the Aspire manifest renders the container
as `quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z`.
The MinIO carve-out this branch carries only moved `minio/minio`. `minio/mc` is gone from Docker Hub as well (`hub.docker.com/v2/repositories/minio/mc/` answers 404), and it is what `minio-init` runs: without it `dotnet run --project src/Host/FSH.Starter.AppHost` and `docker compose up` both die on the image pull, and the `fsh` bucket is never created, so the first upload fails with NoSuchBucket. Same pinned tag as fullstackhero#1388, which owns the fix, so the copy stays byte-identical to it and can be dropped once that lands.
The pin's own comment says "Testcontainers 4.11.0 and 4.13.0 both depend on 2025.1.0, so bumping Testcontainers does not help", but the branch also bumps Testcontainers to 4.14.0, whose nuspec declares `SSH.NET >= 2026.0.0`. The two statements cannot both be true, and the bump is the one that is: with the pin removed, `dotnet restore src/FSH.Starter.slnx --force` reports zero NU1902/NU1903 and exits 0. It was carrying a transitive pin that no longer pins anything. The MessagePack pin above it stays: that one is still load-bearing (removing it brings GHSA-hv8m-jj95-wg3x straight back, verified in the same probe).
Moving `plainTextContent` from `Body` to `TextBody` made the text part vanish rather than become empty: `MailHelper.CreateSingleEmail` only adds it when the string is non-null and non-empty. Every caller inside this repo was migrated, so the tree is fine — but this is a template, and a consumer who still writes `new MailRequest(to, subject, "Your code is 123456")` silently went from a two-part message to HTML-only, with no compiler error and no warning. `TextBody ?? Body` restores the old behaviour for them and changes nothing for a caller that supplies both. Covered by a test that would have caught the drop.
The `text/plain` regression this PR fixes was gated only on SendGrid, which is the optional provider. SMTP is the default (`UseSendGrid` defaults to false), and its body builder had no test at all, so putting the markup back into both parts there would have kept the suite green. The transport needs a server and is not where the mapping happens, so the MIME build is what these drive: both parts present as `multipart/alternative`, HTML alone staying `text/html` with no invented plain part, and an attachment wrapping the alternative in `multipart/mixed` without dropping the text. `AddAttachmentsAsync` is `internal` with `InternalsVisibleTo` for the test assembly, the same way `Storage` already exposes its internals to the integration suite. Mapping `TextBody` back to `request.Body` turns all three red.
…project The module that produces every templated mail on this branch had no test project at all, so the text/plain twin this PR is about was only gated at the provider end. `Notifications.Tests` closes that: the templates are pure string builders, so the whole thing runs without a DbContext or a host. `BillingEmailBodiesTests` walks all four templates through one theory and asserts the pair is a real pair - an HTML body carrying the subject and markup, and a plain twin that is non-empty, repeats the subject and contains no angle bracket. Plus the two places the copy can drift on its own: a tenant name is escaped in the markup and left literal in the text part (entities in text/plain are read as entities), and the optional due-date line disappears without leaving a blank paragraph behind. `BillingEmailSenderTests` covers the best-effort send: both bodies reach the `MailRequest`, an absent address sends nothing, a throwing provider does not escape the integration-event handler, and the warning it logs identifies the operation without carrying the recipient. `IMailService` is ours, so the doubles are real implementations rather than substitutes, per `.agents/rules/testing.md`. The module exposes its internals to this assembly the same way `Mailing` already does for `Framework.Tests`. Verified: `dotnet test src/Tests/Notifications.Tests` passes 13/13; making `Expired` reuse its HTML as the text body fails the theory arm and the sender's pass-through case. `dotnet build src/FSH.Starter.slnx` exits 0 with the project registered, and Architecture.Tests stays 55/55.
The job enumerates test projects by name rather than running the solution, so adding `Notifications.Tests` to `FSH.Starter.slnx` put it in the build and nowhere else: the previous run on this branch was green without executing a single one of its tests. A gate CI never runs is not a gate. Registering it in the slnx stays - that is what `dotnet test src/FSH.Starter.slnx` and the IDE read - but this list is the wiring that matters on a PR.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Every mail provider in the kit puts
MailRequest.Bodyin the HTML slot: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/htmlpart is not auto-linked by most clients (auto-linking istext/plainbehaviour), 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:
FirstName— user-supplied — straight into markup. A name containing<breaks the message; it is an HTML injection into the rendered mail.Bodyas both parts, so the confirmation and billing templates were shipped as thetext/plainalternative too: a text-only client rendered raw markup.The confirmation mail (
BuildConfirmationEmailHtml) and the billing bodies were already HTML with escaping, so the defect this PR fixes is the inconsistency rather than the templates. Two caveats review was right to add: the billing escaper covers only& < >, andamountText(which embedscurrency) reaches the markup unescaped. Neither is exploitable today —currencyisNotEmpty().Length(3)behindBillingPermissions.Manage, and three characters in element content are not a payload — but "already correct" was too strong. #1385 is what actually fixes both.Solution
MailRequestgains an optionalTextBody(appended last, so existing positional calls keep compiling):SmtpMailServicesets bothHtmlBodyandTextBody→ MailKit emitsmultipart/alternative.SendGridMailServicemapsTextBody→plainTextContentandBody→htmlContent, instead of sending the HTML as both.EmailBodieshelper in Identity (LinkActionHtml,NoticeHtml) renders the action link as a real<a href>and HTML-encodes every interpolated value.text/plaintwin, 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 inTextBody, which is where the existing link-shape test now asserts.Changes to
src/BuildingBlocks(Golden Rule #4, requesting sign-off)Mailing/MailRequest.cs— new optionalTextBodyproperty + XML docs stating thatBodyis HTML.Mailing/Services/SmtpMailService.cs— one line:TextBodyon theBodyBuilder.Mailing/Services/SendGridMailService.cs—plainTextContentnow comes fromTextBody.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,%2Bencoding) now asserts onTextBody.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—TextBodyround-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, zeroNU1903.dotnet build -warnaserror: exit 0.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 theSSH.NETpinThis branch was
CONFLICTING.mainhas since added theSystem.Security.Cryptography.Xml 10.0.10pin that this PR was carrying, which was the only conflict. Resolved by keepingmain'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 onSSH.NET2025.1.0, pulled transitively by Testcontainers. This branch carried an explicit pin for it; review showed the pin was dead once Testcontainers moved to 4.14.0, which declaresSSH.NET >= 2026.0.0, so the pin has been removed and the bump alone clears the advisory (measured:dotnet restore --force, zero NU1902/NU1903, exit 0).Infra carve-outs, corrected after review. Two things in the out-of-topic hunks were wrong and
are fixed on the branch:
minio/minioto quay.io.minio/mcis gone from Docker Hub too(
hub.docker.com/v2/repositories/minio/mc/answers 404) and it is whatminio-initruns, so bothdotnet run --project src/Host/FSH.Starter.AppHostanddocker compose updied on the pull and thefshbucket was never created. Now pinned to the same quay tag #1388 uses.SSH.NETpin is gone: it pinned nothing. Its own comment claimed bumping Testcontainersdoes not help, but 4.14.0 — which this branch also carries — declares
SSH.NET >= 2026.0.0.Measured rather than argued: with the pin removed,
dotnet restore src/FSH.Starter.slnx --forcereports zero NU1902/NU1903 and exits 0. (The MessagePack pin next to it stays; removing that one
does bring its advisory straight back.)
With both applied,
deploy/docker/docker-compose.ymlandsrc/Directory.Packages.propsare nowgenuinely byte-identical to #1388 (
git diff --exit-code, checked today), which the earlier claimwas not.
The gate was on the wrong provider. The
text/plainregression was covered bySendGridMailServiceTests, and SendGrid is the optional provider:UseSendGriddefaults to false,so SMTP is what a default deployment sends with, and its body builder had no test at all. Putting
the markup back into both parts there kept the suite green.
SmtpMailServiceTestsdrives the real MIME build: both parts present asmultipart/alternative,HTML alone staying
text/htmlwith no invented plain part, and an attachment wrapping thealternative in
multipart/mixedwithout dropping the text. MappingTextBodyback torequest.Bodyturns all three red.AddAttachmentsAsyncisinternalwithInternalsVisibleTofor the test assembly, the way
Storagealready exposes its internals to the integration suite;the transport itself needs a server and is not where the mapping happens.
And the module that writes the bodies now has a test project.
Notificationshad none, soeverything it renders was exercised only through the mail services below it. The templates are
pure string builders, so
Notifications.Testsruns without a DbContext or a host.BillingEmailBodiesTestswalks all four templates through one theory and asserts the pair is areal pair: an HTML body carrying the subject and markup, and a plain twin that is non-empty,
repeats the subject and contains no angle bracket. Plus the two places the copy can drift on its
own - a tenant name is escaped in the markup and left literal in the text part (entities in
text/plain are read as entities), and the optional due-date line disappears without leaving a blank
paragraph behind.
BillingEmailSenderTestscovers the best-effort send that every billingintegration-event handler goes through: both bodies reach the
MailRequest, an absent addresssends nothing, a throwing provider does not escape the handler (it runs inside the transaction of
whatever created or renewed the tenant), and the warning it logs identifies the operation without
carrying the recipient.
IMailServiceis ours, so the doubles are real implementations rather than substitutes, per.agents/rules/testing.md. The module exposes its internals to this assembly the same wayMailingalready does forFramework.Tests. Registering the project insrc/FSH.Starter.slnxturned out not to be the wiring. The unit-test job enumerates test projects by name rather than running the solution, so the slnx entry put the new project in the build and nowhere else: the first run on this branch was green without executing one of its tests (run 35359084326 mentionsNotifications.Tests.dllexactly once, from the build step).Notificationsis in that list now. Proven rather than assumed: run 35362147081, Unit Tests job,Passed! - Failed: 0, Passed: 13 ... Notifications.Tests.dll.Verified: 13/13; making
Expiredreuse its HTML as the text body fails the theory arm and thesender's pass-through case.
dotnet build src/FSH.Starter.slnxexits 0 with the projectregistered, and Architecture.Tests stays 55/55.