test(sms): harden S3 adapter secret-redaction + coverage (review follow-up)

2026-06-19 10:28:58 -04:00
parent a1d484a5ff
commit 4555a3f333
2 changed files with 58 additions and 6 deletions
@@ -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);
@@ -72,7 +72,10 @@ public class SmsNotificationDeliveryAdapterTests

    private SmsNotificationDeliveryAdapter CreateAdapter(HttpMessageHandler handler, SmsOptions? options = null)
    {
-        _httpClientFactory.CreateClient(Arg.Any<string>()).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]