mirror of
https://github.com/okieselbach/AutopilotMonitor.git
synced 2026-10-24 04:32:32 +00:00
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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@<b>contoso.com</b>");
|
||||
|
||||
Assert.True(string.IsNullOrEmpty(config.DomainName));
|
||||
Assert.Null(config.OnboardedBy);
|
||||
_tenantConfigMock.Verify(x => x.SaveConfigurationAsync(It.IsAny<TenantConfiguration>(), It.IsAny<string?>(), It.IsAny<string?>()), Times.Never);
|
||||
_telegramMock.Verify(x => x.SendNewTenantSignupAsync(It.IsAny<string>(), It.IsAny<string>()), Times.Never);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task HandleNewTenantDomain_WhenDomainEmpty_ExtractsAndSaves()
|
||||
{
|
||||
|
||||
@@ -247,6 +247,18 @@ public sealed class EmailServiceTests
|
||||
Assert.Contains("linkedin.com", html);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void BuiltInTemplates_HtmlEncodeDomain()
|
||||
{
|
||||
const string hostile = "<a href=\"https://evil.invalid\">x</a>";
|
||||
|
||||
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()
|
||||
{
|
||||
|
||||
@@ -29,6 +29,30 @@ public sealed class EmailTemplateServiceTests
|
||||
Assert.DoesNotContain(EmailTemplateService.DomainPlaceholder, html);
|
||||
}
|
||||
|
||||
[Theory]
|
||||
[InlineData("<b>x</b>", "<b>x</b>")]
|
||||
[InlineData("a\"b&c", "a"b&c")]
|
||||
public void Render_HtmlEncodesDomain_ForOverrideTemplates(string domain, string expected)
|
||||
{
|
||||
var html = EmailTemplateService.Render("<p>{{domainName}}</p>", domain);
|
||||
|
||||
Assert.Equal($"<p>{expected}</p>", 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, "<img src=x onerror=alert(1)>&\"");
|
||||
|
||||
Assert.DoesNotContain("<img", html);
|
||||
Assert.Contains("<img src=x onerror=alert(1)>&"", html);
|
||||
Assert.DoesNotContain("&lt;", html);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Override_WinsAndRendersPlaceholder()
|
||||
{
|
||||
|
||||
@@ -1,11 +1,13 @@
|
||||
using System;
|
||||
using System.Collections.Generic;
|
||||
using System.Linq;
|
||||
using System.Threading;
|
||||
using System.Threading.Tasks;
|
||||
using Azure;
|
||||
using Azure.Data.Tables;
|
||||
using AutopilotMonitor.Functions.DataAccess.TableStorage;
|
||||
using AutopilotMonitor.Functions.Functions.Config;
|
||||
using AutopilotMonitor.Functions.Helpers;
|
||||
using AutopilotMonitor.Functions.Services;
|
||||
using AutopilotMonitor.Shared;
|
||||
using AutopilotMonitor.Shared.DataAccess;
|
||||
@@ -386,4 +388,40 @@ public class TenantContactEmailTests
|
||||
It.IsAny<string>(), It.IsAny<string>(),
|
||||
It.IsAny<IEnumerable<string>>(), It.IsAny<CancellationToken>()), 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("<b>contoso.com</b>")]
|
||||
[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))));
|
||||
}
|
||||
}
|
||||
|
||||
+6
@@ -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).
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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;
|
||||
|
||||
/// <summary>
|
||||
/// True when <paramref name="domainName"/> 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.
|
||||
/// </summary>
|
||||
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;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Validates a candidate configuration against the stored one. Returns a
|
||||
/// user-facing error message (same wording the PUT endpoint always produced),
|
||||
|
||||
@@ -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);
|
||||
|
||||
/// <summary>Substitutes the placeholder; an empty domain becomes "your organization".</summary>
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
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);
|
||||
}
|
||||
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
using System.Net;
|
||||
using AutopilotMonitor.Shared;
|
||||
|
||||
namespace AutopilotMonitor.Functions.Services;
|
||||
@@ -26,7 +27,7 @@ public static class EmailTemplates
|
||||
/// </summary>
|
||||
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 $@"
|
||||
<!DOCTYPE html>
|
||||
@@ -125,7 +126,7 @@ public static class EmailTemplates
|
||||
/// </summary>
|
||||
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 $@"
|
||||
<!DOCTYPE html>
|
||||
|
||||
Reference in New Issue
Block a user