From 4555a3f333ac9269ea2d5a7c5878c765f3f30fa0 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 19 Jun 2026 10:28:58 -0400 Subject: [PATCH] test(sms): harden S3 adapter secret-redaction + coverage (review follow-up) --- .../SmsNotificationDeliveryAdapter.cs | 16 +++++-- .../SmsNotificationDeliveryAdapterTests.cs | 48 ++++++++++++++++++- 2 files changed, 58 insertions(+), 6 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/Delivery/SmsNotificationDeliveryAdapter.cs b/src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/Delivery/SmsNotificationDeliveryAdapter.cs index 655ba73b..c064a163 100644 --- a/src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/Delivery/SmsNotificationDeliveryAdapter.cs +++ b/src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/Delivery/SmsNotificationDeliveryAdapter.cs @@ -117,6 +117,12 @@ public sealed class SmsNotificationDeliveryAdapter : INotificationDeliveryAdapte } var authToken = smsConfig.AuthToken!; + // Defense-in-depth: build the full Basic-auth credential string so that BOTH + // the bare AuthToken AND the "AccountSid:AuthToken" composite are guaranteed + // scrubbed from any returned / logged error. CredentialRedactor.Scrub redacts + // the whole string it is given AND its last colon-separated component (when + // >= MinSecretLength), so one call with the composite covers both shapes. + var redactSecret = $"{smsConfig.AccountSid}:{authToken}"; var body = ComposeBody(notification.Subject, notification.Body); var baseUrl = string.IsNullOrWhiteSpace(smsConfig.ApiBaseUrl) @@ -147,7 +153,7 @@ public sealed class SmsNotificationDeliveryAdapter : INotificationDeliveryAdapte var attempt = await SendOneAsync( httpClient, requestUri, authHeader, smsConfig, hasMessagingService, - phone, body, timeoutSeconds, authToken, cancellationToken); + phone, body, timeoutSeconds, redactSecret, cancellationToken); switch (attempt.Class) { @@ -180,7 +186,7 @@ public sealed class SmsNotificationDeliveryAdapter : INotificationDeliveryAdapte string toNumber, string body, int timeoutSeconds, - string authToken, + string redactSecret, CancellationToken cancellationToken) { // Per-request timeout layered over the caller's token via a linked CTS, so a @@ -224,7 +230,7 @@ public sealed class SmsNotificationDeliveryAdapter : INotificationDeliveryAdapte var cls = SmsErrorClassifier.Classify(response.StatusCode); var detail = CredentialRedactor.Scrub( $"Twilio returned HTTP {(int)response.StatusCode} ({response.StatusCode}) for {toNumber}", - authToken); + redactSecret); if (cls == SmsErrorClass.Transient) { @@ -250,7 +256,7 @@ public sealed class SmsNotificationDeliveryAdapter : INotificationDeliveryAdapte { var detail = CredentialRedactor.Scrub( $"SMS transport failure for {toNumber} ({ex.GetType().Name}): {ex.Message}", - authToken); + redactSecret); _logger.LogWarning( "Transient SMS failure delivering to {Recipient} ({ExceptionType}): {Detail}", toNumber, ex.GetType().Name, detail); @@ -263,7 +269,7 @@ public sealed class SmsNotificationDeliveryAdapter : INotificationDeliveryAdapte // stance for unclassified failures). var detail = CredentialRedactor.Scrub( $"SMS delivery failed for {toNumber} ({ex.GetType().Name}): {ex.Message}", - authToken); + redactSecret); _logger.LogError( "Unclassified SMS failure delivering to {Recipient} ({ExceptionType}): {Detail}", toNumber, ex.GetType().Name, detail); diff --git a/tests/ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests/Delivery/SmsNotificationDeliveryAdapterTests.cs b/tests/ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests/Delivery/SmsNotificationDeliveryAdapterTests.cs index c185ed9f..52128eb2 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests/Delivery/SmsNotificationDeliveryAdapterTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests/Delivery/SmsNotificationDeliveryAdapterTests.cs @@ -72,7 +72,10 @@ public class SmsNotificationDeliveryAdapterTests private SmsNotificationDeliveryAdapter CreateAdapter(HttpMessageHandler handler, SmsOptions? options = null) { - _httpClientFactory.CreateClient(Arg.Any()).Returns(_ => new HttpClient(handler)); + // Pin to the real named-client name so tests catch accidental renames. + _httpClientFactory + .CreateClient(SmsNotificationDeliveryAdapter.HttpClientName) + .Returns(_ => new HttpClient(handler)); return new SmsNotificationDeliveryAdapter( _repository, _httpClientFactory, @@ -145,6 +148,10 @@ public class SmsNotificationDeliveryAdapterTests Assert.Contains("To=", body); Assert.Contains("From=", body); Assert.Contains("Body=", body); + + // The factory must be asked for the exact named client the adapter is documented + // to use, not an arbitrary string. + _httpClientFactory.Received(1).CreateClient(SmsNotificationDeliveryAdapter.HttpClientName); } [Fact] @@ -360,6 +367,28 @@ public class SmsNotificationDeliveryAdapterTests Assert.Contains("incomplete", outcome.Error); } + [Fact] + public async Task Deliver_IncompleteConfig_NoFromAndNoMessagingServiceSid_ReturnsPermanent() + { + // AccountSid + AuthToken are present but BOTH FromNumber and MessagingServiceSid + // are absent — the adapter cannot build a valid Twilio request. This is a + // configuration error that cannot be resolved by retrying; expect Permanent. + var config = new SmsConfiguration("AC_account_sid", "") // empty FromNumber + { + Id = 1, + AuthToken = AuthToken, + MessagingServiceSid = null, // no fallback either + ApiBaseUrl = "https://fake-twilio.test", + }; + SetupList(config: config); + var adapter = CreateAdapter(ScriptedHttpMessageHandler.ForStatuses()); + + var outcome = await adapter.DeliverAsync(MakeNotification()); + + Assert.Equal(DeliveryResult.PermanentFailure, outcome.Result); + Assert.Contains("incomplete", outcome.Error); + } + [Fact] public async Task Deliver_DefaultsToTwilioBaseUrl_WhenApiBaseUrlAbsent() { @@ -384,13 +413,30 @@ public class SmsNotificationDeliveryAdapterTests { SetupList(); // A permanent failure (400) so the outcome carries an Error string. + // NOTE: the HTTP-error detail string ("Twilio returned HTTP 400…") is built + // without ever embedding the credential, so the primary guarantee here is + // documentation of intent. The CredentialRedactor.Scrub call still runs as + // defense-in-depth in case the detail format ever changes. var handler = ScriptedHttpMessageHandler.ForStatuses(HttpStatusCode.BadRequest); var adapter = CreateAdapter(handler); var outcome = await adapter.DeliverAsync(MakeNotification()); Assert.NotNull(outcome.Error); + + // Bare token must not appear. Assert.DoesNotContain(AuthToken, outcome.Error); + + // The "AccountSid:AuthToken" composite (the Basic-auth credential before + // base64 encoding) must not appear — tests the defense-in-depth Scrub. + const string accountSid = "AC_account_sid"; + var composite = $"{accountSid}:{AuthToken}"; + Assert.DoesNotContain(composite, outcome.Error); + + // The base64-encoded form of the composite must not appear either. + var base64Credential = Convert.ToBase64String( + System.Text.Encoding.ASCII.GetBytes(composite)); + Assert.DoesNotContain(base64Credential, outcome.Error); } [Fact]