fix(notifications): EWS sender review follow-ups — https guard, transient-path logging, test hardening
This commit is contained in:
@@ -68,6 +68,19 @@ public sealed class EwsSoapMailSender : IEwsMailSender
|
|||||||
{
|
{
|
||||||
ArgumentNullException.ThrowIfNull(request);
|
ArgumentNullException.ThrowIfNull(request);
|
||||||
|
|
||||||
|
// Defense in depth: a Basic header is a replayable cleartext credential, so it never
|
||||||
|
// leaves this process over anything but https — not even if a misconfigured
|
||||||
|
// SmtpConfiguration row supplies an http:// URL. Config defects do not improve on
|
||||||
|
// retry, so this is permanent. Only the scheme and host appear in the message.
|
||||||
|
if (!string.Equals(request.Endpoint.Scheme, Uri.UriSchemeHttps, StringComparison.OrdinalIgnoreCase))
|
||||||
|
{
|
||||||
|
var schemeError =
|
||||||
|
$"EWS endpoint '{request.Endpoint.Scheme}://{request.Endpoint.Host}' is not https; " +
|
||||||
|
"Basic credentials are only ever sent over https.";
|
||||||
|
_logger.LogError("Permanent EWS failure: {Detail}", schemeError);
|
||||||
|
throw new EwsPermanentException(schemeError);
|
||||||
|
}
|
||||||
|
|
||||||
var envelope = EwsSoapEnvelope.BuildCreateItem(
|
var envelope = EwsSoapEnvelope.BuildCreateItem(
|
||||||
request.FromAddress, request.BccRecipients, request.Subject, request.Body);
|
request.FromAddress, request.BccRecipients, request.Subject, request.Body);
|
||||||
|
|
||||||
@@ -114,7 +127,14 @@ public sealed class EwsSoapMailSender : IEwsMailSender
|
|||||||
}
|
}
|
||||||
catch (Exception ex) when (ex is HttpRequestException or OperationCanceledException)
|
catch (Exception ex) when (ex is HttpRequestException or OperationCanceledException)
|
||||||
{
|
{
|
||||||
// Transport failure or our own timeout — availability-shaped, so retry.
|
// Transport failure or our own timeout — availability-shaped, so retry. Logged with
|
||||||
|
// the host and exception type only: the exception message is untrusted text and the
|
||||||
|
// recipient addresses are notification content.
|
||||||
|
_logger.LogWarning(
|
||||||
|
"Transient EWS failure contacting {EwsHost} ({ExceptionType}).",
|
||||||
|
request.Endpoint.Host,
|
||||||
|
ex.GetType().Name);
|
||||||
|
|
||||||
throw new EwsTransientException(
|
throw new EwsTransientException(
|
||||||
Scrub(
|
Scrub(
|
||||||
$"EWS request to {request.Endpoint.Host} failed: {ex.Message}",
|
$"EWS request to {request.Endpoint.Host} failed: {ex.Message}",
|
||||||
|
|||||||
+42
-7
@@ -76,16 +76,34 @@ public class EwsSoapMailSenderTests
|
|||||||
public async Task SendAsync_SuccessResponse_CompletesAndSendsExpectedRequest()
|
public async Task SendAsync_SuccessResponse_CompletesAndSendsExpectedRequest()
|
||||||
{
|
{
|
||||||
var handler = StubHandler.Responding(HttpStatusCode.OK, SuccessBody);
|
var handler = StubHandler.Responding(HttpStatusCode.OK, SuccessBody);
|
||||||
var sender = CreateSender(handler);
|
var factory = new FakeHttpClientFactory(handler);
|
||||||
|
var sender = CreateSender(factory);
|
||||||
|
|
||||||
await sender.SendAsync(CreateRequest());
|
await sender.SendAsync(CreateRequest());
|
||||||
|
|
||||||
|
Assert.Equal(EwsSoapMailSender.HttpClientName, factory.RequestedClientName);
|
||||||
Assert.Equal("Basic", handler.CapturedScheme);
|
Assert.Equal("Basic", handler.CapturedScheme);
|
||||||
Assert.Equal(ExpectedBase64, handler.CapturedParameter);
|
Assert.Equal(ExpectedBase64, handler.CapturedParameter);
|
||||||
Assert.Equal("text/xml", handler.CapturedMediaType);
|
Assert.Equal("text/xml", handler.CapturedMediaType);
|
||||||
Assert.Contains(Recipient, handler.CapturedContent);
|
Assert.Contains(Recipient, handler.CapturedContent);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public async Task SendAsync_NonHttpsEndpoint_ThrowsPermanentWithoutSendingCredentials()
|
||||||
|
{
|
||||||
|
// A Basic header is a replayable cleartext credential: the guard must fire before the
|
||||||
|
// request is ever built, so a misconfigured http:// URL cannot put it on the wire.
|
||||||
|
var handler = StubHandler.Responding(HttpStatusCode.OK, SuccessBody);
|
||||||
|
var sender = CreateSender(handler);
|
||||||
|
var request = CreateRequest() with { Endpoint = new Uri("http://ews.example.test/EWS/Exchange.asmx") };
|
||||||
|
|
||||||
|
var ex = await Assert.ThrowsAsync<EwsPermanentException>(() => sender.SendAsync(request));
|
||||||
|
|
||||||
|
Assert.Equal(0, handler.InvocationCount);
|
||||||
|
Assert.Contains("https", ex.Message, StringComparison.OrdinalIgnoreCase);
|
||||||
|
AssertNoCredentialLeak(ex);
|
||||||
|
}
|
||||||
|
|
||||||
[Fact]
|
[Fact]
|
||||||
public async Task SendAsync_ServerBusyResponseBody_ThrowsTransient()
|
public async Task SendAsync_ServerBusyResponseBody_ThrowsTransient()
|
||||||
{
|
{
|
||||||
@@ -189,13 +207,16 @@ public class EwsSoapMailSenderTests
|
|||||||
}
|
}
|
||||||
|
|
||||||
/// <summary>
|
/// <summary>
|
||||||
/// Neither the password nor the base64 Basic-auth value may reach an exception message —
|
/// Neither the password nor the base64 Basic-auth value may reach an exception — those land
|
||||||
/// those messages land in the operational log and in the notification's stored error.
|
/// in the operational log and in the notification's stored error. The whole
|
||||||
|
/// <see cref="Exception.ToString"/> is scanned, so an inner transport exception carrying the
|
||||||
|
/// credential would fail this too.
|
||||||
/// </summary>
|
/// </summary>
|
||||||
private static void AssertNoCredentialLeak(Exception ex)
|
private static void AssertNoCredentialLeak(Exception ex)
|
||||||
{
|
{
|
||||||
Assert.DoesNotContain(Password, ex.Message, StringComparison.Ordinal);
|
var rendered = ex.ToString();
|
||||||
Assert.DoesNotContain(ExpectedBase64, ex.Message, StringComparison.Ordinal);
|
Assert.DoesNotContain(Password, rendered, StringComparison.Ordinal);
|
||||||
|
Assert.DoesNotContain(ExpectedBase64, rendered, StringComparison.Ordinal);
|
||||||
}
|
}
|
||||||
|
|
||||||
private static EwsSendRequest CreateRequest(int timeoutSeconds = 0)
|
private static EwsSendRequest CreateRequest(int timeoutSeconds = 0)
|
||||||
@@ -210,12 +231,22 @@ public class EwsSoapMailSenderTests
|
|||||||
timeoutSeconds);
|
timeoutSeconds);
|
||||||
|
|
||||||
private static EwsSoapMailSender CreateSender(HttpMessageHandler handler)
|
private static EwsSoapMailSender CreateSender(HttpMessageHandler handler)
|
||||||
=> new(new FakeHttpClientFactory(handler), NullLogger<EwsSoapMailSender>.Instance);
|
=> CreateSender(new FakeHttpClientFactory(handler));
|
||||||
|
|
||||||
|
private static EwsSoapMailSender CreateSender(FakeHttpClientFactory factory)
|
||||||
|
=> new(factory, NullLogger<EwsSoapMailSender>.Instance);
|
||||||
|
|
||||||
/// <summary>Hands every named client the one stub handler under test.</summary>
|
/// <summary>Hands every named client the one stub handler under test.</summary>
|
||||||
private sealed class FakeHttpClientFactory(HttpMessageHandler handler) : IHttpClientFactory
|
private sealed class FakeHttpClientFactory(HttpMessageHandler handler) : IHttpClientFactory
|
||||||
{
|
{
|
||||||
public HttpClient CreateClient(string name) => new(handler, disposeHandler: false);
|
/// <summary>The client name the sender asked for, so tests can pin the named registration.</summary>
|
||||||
|
public string? RequestedClientName { get; private set; }
|
||||||
|
|
||||||
|
public HttpClient CreateClient(string name)
|
||||||
|
{
|
||||||
|
RequestedClientName = name;
|
||||||
|
return new HttpClient(handler, disposeHandler: false);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/// <summary>
|
/// <summary>
|
||||||
@@ -237,6 +268,9 @@ public class EwsSoapMailSenderTests
|
|||||||
|
|
||||||
public string CapturedContent { get; private set; } = string.Empty;
|
public string CapturedContent { get; private set; } = string.Empty;
|
||||||
|
|
||||||
|
/// <summary>How many times the handler was reached — 0 proves a guard fired first.</summary>
|
||||||
|
public int InvocationCount { get; private set; }
|
||||||
|
|
||||||
public static StubHandler Responding(HttpStatusCode status, string body)
|
public static StubHandler Responding(HttpStatusCode status, string body)
|
||||||
=> new(_ => Task.FromResult(new HttpResponseMessage(status)
|
=> new(_ => Task.FromResult(new HttpResponseMessage(status)
|
||||||
{
|
{
|
||||||
@@ -256,6 +290,7 @@ public class EwsSoapMailSenderTests
|
|||||||
protected override async Task<HttpResponseMessage> SendAsync(
|
protected override async Task<HttpResponseMessage> SendAsync(
|
||||||
HttpRequestMessage request, CancellationToken cancellationToken)
|
HttpRequestMessage request, CancellationToken cancellationToken)
|
||||||
{
|
{
|
||||||
|
InvocationCount++;
|
||||||
CapturedScheme = request.Headers.Authorization?.Scheme;
|
CapturedScheme = request.Headers.Authorization?.Scheme;
|
||||||
CapturedParameter = request.Headers.Authorization?.Parameter;
|
CapturedParameter = request.Headers.Authorization?.Parameter;
|
||||||
CapturedMediaType = request.Content?.Headers.ContentType?.MediaType;
|
CapturedMediaType = request.Content?.Headers.ContentType?.MediaType;
|
||||||
|
|||||||
Reference in New Issue
Block a user