From 82b26b4ab90caee55043d60117dfc8d52d18c63c Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Sat, 29 Aug 2026 22:33:52 +0200 Subject: [PATCH] fix(security): HTML-encode domain in transactional mails; freeze DomainName on config PUT, validate first-login seed Welcome/farewell mails rendered DomainName unencoded while the full-model config PUT persisted it verbatim, letting a tenant admin ship attacker HTML in a signed product mail to an arbitrary recipient. Encode at Render() and the built-in builders, preserve DomainName from the stored row on PUT for every caller, and only seed a strict DNS host name at first login. Co-Authored-By: Claude Fable 5 --- .../backend/tenant-activation-welcome-mail.md | 7 ++++ docs/log.md | 4 ++ .../AuthFunctionSideEffectTests.cs | 16 ++++++++ .../EmailServiceTests.cs | 12 ++++++ .../EmailTemplateServiceTests.cs | 24 ++++++++++++ .../TenantContactEmailTests.cs | 38 +++++++++++++++++++ .../UpdateTenantConfigurationFunction.cs | 6 +++ .../Functions/Infrastructure/AuthFunction.cs | 9 +++++ .../Helpers/TenantConfigValidation.cs | 34 +++++++++++++++++ .../Services/EmailTemplateService.cs | 10 ++++- .../Services/EmailTemplates.cs | 5 ++- 11 files changed, 161 insertions(+), 4 deletions(-) diff --git a/docs/backend/tenant-activation-welcome-mail.md b/docs/backend/tenant-activation-welcome-mail.md index 1206634a..b511ab86 100644 --- a/docs/backend/tenant-activation-welcome-mail.md +++ b/docs/backend/tenant-activation-welcome-mail.md @@ -107,6 +107,13 @@ properties `Html`, `UpdatedBy`, `UpdatedUtc`), cached 5 minutes, invalidated on * The only placeholder is `{{domainName}}`; an empty domain renders as "your organization", matching the built-ins. Subjects are not editable (pinned constants). +* The domain is HTML-encoded at the point of substitution (`EmailTemplateService.Render` and the + built-in template builders) — the mail goes out under the product's signed sender to a + tenant-chosen recipient, so no writer of `DomainName` may ever inject markup into it. + `DomainName` itself is tenant identity: seeded once at first login from the UPN domain + (`AuthFunction`, only when it passes `TenantConfigValidation.IsValidDomainName` — a strict + dotted host name), preserved from the stored row on the full-model config PUT for every + caller, and "never writable" in the field-patch flow. * Limit 30 000 characters (Table string property cap is 32K UTF-16 chars). * The send path is fail-soft: a storage error falls back to the built-in body. The admin read path is not — a broken store surfaces as an error instead of silently showing the diff --git a/docs/log.md b/docs/log.md index 97191a67..73bdac17 100644 --- a/docs/log.md +++ b/docs/log.md @@ -1,5 +1,9 @@ # Log +## 2026-08-29 (10) + +* **Update**: `backend/tenant-activation-welcome-mail.md` — welcome/farewell mails HTML-encode the domain at substitution; `DomainName` is frozen on the full-model config PUT (was persisted verbatim) and the first-login seed only accepts a strict DNS host name. + ## 2026-08-29 (9) * **Update**: `agent/decision-engine.md`, `agent/overview.md` — `SignalIngress.Post` is worker-thread re-entrancy-safe: signals the worker posts to itself from inside `ApplyStep` (EffectRunner `ClassifierVerdictIssued`) go to a worker-local follow-up queue drained ahead of the bounded channel instead of blocking on it, so a full channel can no longer deadlock the single consumer. diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionSideEffectTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionSideEffectTests.cs index d3dfb172..1ce6005e 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionSideEffectTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionSideEffectTests.cs @@ -123,6 +123,22 @@ public class AuthFunctionSideEffectTests // HandleNewTenantDomainAsync // ------------------------------------------------------------------------- + [Fact] + public async Task HandleNewTenantDomain_WhenUpnDomainIsNotAHostName_DoesNotSeed() + { + var config = DefaultConfig(); + config.DomainName = null!; + config.OnboardedBy = null; + + // The seed is rendered into transactional mails: anything but a strict host name is refused. + await _sut.HandleNewTenantDomainAsync(config, TenantId, "user@contoso.com"); + + Assert.True(string.IsNullOrEmpty(config.DomainName)); + Assert.Null(config.OnboardedBy); + _tenantConfigMock.Verify(x => x.SaveConfigurationAsync(It.IsAny(), It.IsAny(), It.IsAny()), Times.Never); + _telegramMock.Verify(x => x.SendNewTenantSignupAsync(It.IsAny(), It.IsAny()), Times.Never); + } + [Fact] public async Task HandleNewTenantDomain_WhenDomainEmpty_ExtractsAndSaves() { diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/EmailServiceTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/EmailServiceTests.cs index 75fa03b0..6914a8dc 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/EmailServiceTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/EmailServiceTests.cs @@ -247,6 +247,18 @@ public sealed class EmailServiceTests Assert.Contains("linkedin.com", html); } + [Fact] + public void BuiltInTemplates_HtmlEncodeDomain() + { + const string hostile = "x"; + + foreach (var html in new[] { EmailTemplates.GetOffboardingFarewellHtml(hostile), EmailTemplates.GetPreviewApprovedHtml(hostile) }) + { + Assert.DoesNotContain(hostile, html); + Assert.Contains("<a href="https://evil.invalid">x</a>", html); + } + } + [Fact] public void FarewellTemplate_EmptyDomain_FallsBackToGenericLabel() { diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/EmailTemplateServiceTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/EmailTemplateServiceTests.cs index 562aba87..fabdffdb 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/EmailTemplateServiceTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/EmailTemplateServiceTests.cs @@ -29,6 +29,30 @@ public sealed class EmailTemplateServiceTests Assert.DoesNotContain(EmailTemplateService.DomainPlaceholder, html); } + [Theory] + [InlineData("x", "<b>x</b>")] + [InlineData("a\"b&c", "a"b&c")] + public void Render_HtmlEncodesDomain_ForOverrideTemplates(string domain, string expected) + { + var html = EmailTemplateService.Render("

{{domainName}}

", domain); + + Assert.Equal($"

{expected}

", html); + } + + [Theory] + [InlineData(EmailTemplateKind.Welcome)] + [InlineData(EmailTemplateKind.Farewell)] + public async Task BuiltIn_HtmlEncodesDomain_NoDoubleEncoding(EmailTemplateKind kind) + { + var (sut, _) = Build(); + + var html = await sut.GetHtmlAsync(kind, "&\""); + + Assert.DoesNotContain("(), It.IsAny(), It.IsAny>(), It.IsAny()), Times.Never); } + + // ------------------------------------------------------------------------- + // IsValidDomainName — the only shape DomainName may be persisted in + // ------------------------------------------------------------------------- + + [Theory] + [InlineData("contoso.com")] + [InlineData("sub.domain.org")] + [InlineData("x-y.example")] + [InlineData("A1.B2.c3")] + public void IsValidDomainName_AcceptsHostNames(string domain) + => Assert.True(TenantConfigValidation.IsValidDomainName(domain)); + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData("localhost")] + [InlineData(".contoso.com")] + [InlineData("contoso.com.")] + [InlineData("con..toso.com")] + [InlineData("-contoso.com")] + [InlineData("contoso-.com")] + [InlineData("con toso.com")] + [InlineData("contoso.com")] + [InlineData("contoso.com\"onerror=\"x")] + [InlineData("contoso.com/path")] + [InlineData("user@contoso.com")] + public void IsValidDomainName_RejectsNonHostNames(string? domain) + => Assert.False(TenantConfigValidation.IsValidDomainName(domain)); + + [Fact] + public void IsValidDomainName_RejectsOverlongNamesAndLabels() + { + Assert.False(TenantConfigValidation.IsValidDomainName(new string('a', 64) + ".com")); + Assert.False(TenantConfigValidation.IsValidDomainName(string.Join(".", Enumerable.Repeat(new string('a', 60), 5)))); + } } diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Config/UpdateTenantConfigurationFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Config/UpdateTenantConfigurationFunction.cs index 7849f557..f5628119 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Config/UpdateTenantConfigurationFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Config/UpdateTenantConfigurationFunction.cs @@ -110,6 +110,12 @@ namespace AutopilotMonitor.Functions.Functions.Config // admins are served the FULL config so this is normally a no-op for them.) config.RestoreRedactedSecretsFrom(existingConfig); + // DomainName is tenant identity: seeded once from the first login's UPN (AuthFunction) + // and never client-writable — for ANY caller, GA included — mirroring the patch flow + // ("identity — never writable"). Without this preserve the full-model PUT persisted an + // arbitrary string here, which is rendered into the welcome/farewell mails. + config.DomainName = existingConfig.DomainName; + // Shared model validation (rate limits, contact address, webhook/Teams SSRF, custom // headers, notification channels, diagnostics SAS, retention cap) — single source // with the transactional field-patch flow (TenantConfigValidation). diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/AuthFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/AuthFunction.cs index af242cd7..f5241a24 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/AuthFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/AuthFunction.cs @@ -2,6 +2,7 @@ using System.Collections.Generic; using System.Net; using AutopilotMonitor.Functions.Extensions; using AutopilotMonitor.Functions.Functions.Admin; +using AutopilotMonitor.Functions.Helpers; using AutopilotMonitor.Functions.Security; using AutopilotMonitor.Functions.Services; using AutopilotMonitor.Shared; @@ -336,6 +337,14 @@ public class AuthFunction if (string.IsNullOrEmpty(domain)) return; + // The seeded value is rendered into transactional mails — persist only a strict host name, + // regardless of what the identity provider's UPN charset happens to allow. + if (!TenantConfigValidation.IsValidDomainName(domain)) + { + _logger.LogWarning("Refusing to seed DomainName for tenant {TenantId}: UPN domain is not a valid host name", tenantId); + return; + } + _logger.LogInformation("Setting domain name for tenant {TenantId}: {Domain}", tenantId, domain); tenantConfig.DomainName = domain; tenantConfig.UpdatedBy = upn; diff --git a/src/Backend/AutopilotMonitor.Functions/Helpers/TenantConfigValidation.cs b/src/Backend/AutopilotMonitor.Functions/Helpers/TenantConfigValidation.cs index a385dce2..4d101c0b 100644 --- a/src/Backend/AutopilotMonitor.Functions/Helpers/TenantConfigValidation.cs +++ b/src/Backend/AutopilotMonitor.Functions/Helpers/TenantConfigValidation.cs @@ -21,6 +21,40 @@ namespace AutopilotMonitor.Functions.Helpers // RFC 5321 caps a forward path at 254 characters. internal const int MaxContactEmailLength = 254; + // RFC 1035: a host name is at most 253 characters, labels at most 63. + internal const int MaxDomainNameLength = 253; + + /// + /// True when is a strict DNS host name: dotted labels of + /// ASCII letters, digits and inner hyphens only. This is the ONLY shape DomainName may + /// ever be persisted in — it is rendered into transactional mails and admin views. + /// + internal static bool IsValidDomainName(string? domainName) + { + if (string.IsNullOrEmpty(domainName) || domainName.Length > MaxDomainNameLength) + return false; + + var labels = domainName.Split('.'); + if (labels.Length < 2) + return false; + + foreach (var label in labels) + { + if (label.Length == 0 || label.Length > 63) + return false; + if (label[0] == '-' || label[label.Length - 1] == '-') + return false; + foreach (var ch in label) + { + var ok = (ch >= 'a' && ch <= 'z') || (ch >= 'A' && ch <= 'Z') || (ch >= '0' && ch <= '9') || ch == '-'; + if (!ok) + return false; + } + } + + return true; + } + /// /// Validates a candidate configuration against the stored one. Returns a /// user-facing error message (same wording the PUT endpoint always produced), diff --git a/src/Backend/AutopilotMonitor.Functions/Services/EmailTemplateService.cs b/src/Backend/AutopilotMonitor.Functions/Services/EmailTemplateService.cs index 000ba193..fe90c330 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/EmailTemplateService.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/EmailTemplateService.cs @@ -2,6 +2,7 @@ using AutopilotMonitor.Shared.DataAccess; using AutopilotMonitor.Shared.Models; using Microsoft.Extensions.Caching.Memory; using Microsoft.Extensions.Logging; +using System.Net; namespace AutopilotMonitor.Functions.Services; @@ -73,10 +74,15 @@ public class EmailTemplateService : IEmailTemplateProvider ? EmailTemplates.GetPreviewApprovedHtml(DomainPlaceholder) : EmailTemplates.GetOffboardingFarewellHtml(DomainPlaceholder); - /// Substitutes the placeholder; an empty domain becomes "your organization". + /// + /// Substitutes the placeholder; an empty domain becomes "your organization". The value is + /// HTML-encoded at the point of substitution: the mail goes out under the product's signed + /// sender to a tenant-chosen recipient, so no writer of DomainName — current or future — may + /// ever be able to inject markup into it. + /// public static string Render(string rawHtml, string? domainName) { - var display = string.IsNullOrWhiteSpace(domainName) ? EmptyDomainLabel : domainName.Trim(); + var display = string.IsNullOrWhiteSpace(domainName) ? EmptyDomainLabel : WebUtility.HtmlEncode(domainName.Trim()); return rawHtml.Replace(DomainPlaceholder, display, StringComparison.Ordinal); } diff --git a/src/Backend/AutopilotMonitor.Functions/Services/EmailTemplates.cs b/src/Backend/AutopilotMonitor.Functions/Services/EmailTemplates.cs index 3d824f0f..19d82abb 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/EmailTemplates.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/EmailTemplates.cs @@ -1,3 +1,4 @@ +using System.Net; using AutopilotMonitor.Shared; namespace AutopilotMonitor.Functions.Services; @@ -26,7 +27,7 @@ public static class EmailTemplates /// public static string GetOffboardingFarewellHtml(string domainName) { - var displayDomain = string.IsNullOrWhiteSpace(domainName) ? "your organization" : domainName; + var displayDomain = string.IsNullOrWhiteSpace(domainName) ? "your organization" : WebUtility.HtmlEncode(domainName); return $@" @@ -125,7 +126,7 @@ public static class EmailTemplates /// public static string GetPreviewApprovedHtml(string domainName) { - var displayDomain = string.IsNullOrWhiteSpace(domainName) ? "your organization" : domainName; + var displayDomain = string.IsNullOrWhiteSpace(domainName) ? "your organization" : WebUtility.HtmlEncode(domainName); return $@"