From 40088a2123bc0e4e42dbc46d76b8f7f957a4c804 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 10 Jul 2026 04:36:19 -0400 Subject: [PATCH] =?UTF-8?q?fix(notifications):=20validate=20Twilio=20Accou?= =?UTF-8?q?ntSid=20format=20at=20save=20=E2=80=94=20closes=20the=20un-esca?= =?UTF-8?q?ped=20URI=20interpolation=20door?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj --- .../Notifications/SmsConfiguration.razor | 9 +++ .../ManagementActor.cs | 5 ++ .../Pages/SmsConfigurationPageTests.cs | 18 ++--- .../ManagementActorTests.cs | 77 ++++++++++++++++--- 4 files changed, 90 insertions(+), 19 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Notifications/SmsConfiguration.razor b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Notifications/SmsConfiguration.razor index 89673926..8d50406d 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Notifications/SmsConfiguration.razor +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Notifications/SmsConfiguration.razor @@ -226,6 +226,15 @@ return; } + // The AccountSid is interpolated un-escaped into the Twilio request URI, so reject + // any malformed SID client-side too (the ManagementActor enforces the same guard + // server-side; keep the message identical). + if (!System.Text.RegularExpressions.Regex.IsMatch(_accountSid.Trim(), "^AC[0-9a-fA-F]{32}$")) + { + _formError = "Account SID must match Twilio's format: 'AC' followed by 32 hex characters."; + return; + } + var typedAuthToken = string.IsNullOrWhiteSpace(_authToken) ? null : _authToken.Trim(); try diff --git a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs index e57faab2..63d59395 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs @@ -1897,6 +1897,11 @@ public class ManagementActor : ReceiveActor var configs = await repo.GetAllSmsConfigurationsAsync(); var config = configs.FirstOrDefault(c => c.Id == cmd.SmsConfigId) ?? throw new ManagementCommandException($"SmsConfiguration with ID {cmd.SmsConfigId} not found."); + // The AccountSid is interpolated un-escaped into the Twilio request URI + // (SmsNotificationDeliveryAdapter). Reject any value that is not a well-formed + // Twilio SID at save time, closing the URI-injection door before it is persisted. + if (!System.Text.RegularExpressions.Regex.IsMatch(cmd.AccountSid, "^AC[0-9a-fA-F]{32}$")) + throw new ManagementCommandException("Account SID must match Twilio's format: 'AC' followed by 32 hex characters."); config.AccountSid = cmd.AccountSid; config.FromNumber = cmd.FromNumber; config.MessagingServiceSid = cmd.MessagingServiceSid; diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/SmsConfigurationPageTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/SmsConfigurationPageTests.cs index 8094ed01..d7291dad 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/SmsConfigurationPageTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/SmsConfigurationPageTests.cs @@ -32,7 +32,7 @@ public class SmsConfigurationPageTests : BunitContext private const string SecretToken = "super-secret-auth-token"; private static SmsConfiguration Sample() => - new("ACtest_account_sid", "+15551234567") + new("ACcccccccccccccccccccccccccccccccc", "+15551234567") { Id = 1, MessagingServiceSid = "MGtest_messaging_service", @@ -63,7 +63,7 @@ public class SmsConfigurationPageTests : BunitContext cut.WaitForAssertion(() => { // Config row fields render. - Assert.Contains("ACtest_account_sid", cut.Markup); + Assert.Contains("ACcccccccccccccccccccccccccccccccc", cut.Markup); Assert.Contains("+15551234567", cut.Markup); Assert.Contains("MGtest_messaging_service", cut.Markup); // Auth Token shows a presence indicator only — never the value. @@ -81,7 +81,7 @@ public class SmsConfigurationPageTests : BunitContext WireAuth(); var cut = Render(); - cut.WaitForState(() => cut.Markup.Contains("ACtest_account_sid")); + cut.WaitForState(() => cut.Markup.Contains("ACcccccccccccccccccccccccccccccccc")); cut.FindAll("button").First(b => b.TextContent.Contains("Edit")).Click(); @@ -109,7 +109,7 @@ public class SmsConfigurationPageTests : BunitContext // Re-query between each Change(): two-way binding re-renders the form and // invalidates previously found element references. - cut.FindAll("input[type=text]")[0].Change("ACnew_account"); // Account SID + cut.FindAll("input[type=text]")[0].Change("ACdddddddddddddddddddddddddddddddd"); // Account SID cut.FindAll("input[type=text]")[1].Change("+15559876543"); // From Number cut.FindAll("input[type=password]").First().Change("new-token"); @@ -119,7 +119,7 @@ public class SmsConfigurationPageTests : BunitContext { repo.Received().AddSmsConfigurationAsync( Arg.Is(c => - c.AccountSid == "ACnew_account" && + c.AccountSid == "ACdddddddddddddddddddddddddddddddd" && c.FromNumber == "+15559876543" && c.AuthToken == "new-token")); repo.Received().SaveChangesAsync(); @@ -141,7 +141,7 @@ public class SmsConfigurationPageTests : BunitContext cut.FindAll("button").First(b => b.TextContent.Contains("Add SMS configuration")).Click(); // Account SID + Messaging Service SID, leaving From Number (index 1) blank. - cut.FindAll("input[type=text]")[0].Change("ACmsg_account"); // Account SID + cut.FindAll("input[type=text]")[0].Change("ACeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee"); // Account SID cut.FindAll("input[type=text]")[2].Change("MGmessaging_service"); // Messaging Service SID cut.FindAll("input[type=password]").First().Change("new-token"); @@ -151,7 +151,7 @@ public class SmsConfigurationPageTests : BunitContext { repo.Received().AddSmsConfigurationAsync( Arg.Is(c => - c.AccountSid == "ACmsg_account" && + c.AccountSid == "ACeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee" && c.FromNumber == null && c.MessagingServiceSid == "MGmessaging_service" && c.AuthToken == "new-token")); @@ -168,7 +168,7 @@ public class SmsConfigurationPageTests : BunitContext WireAuth(); var cut = Render(); - cut.WaitForState(() => cut.Markup.Contains("ACtest_account_sid")); + cut.WaitForState(() => cut.Markup.Contains("ACcccccccccccccccccccccccccccccccc")); cut.FindAll("button").First(b => b.TextContent.Contains("Edit")).Click(); // Leave the (blank) Auth Token input untouched, then save. @@ -193,7 +193,7 @@ public class SmsConfigurationPageTests : BunitContext WireAuth(); var cut = Render(); - cut.WaitForState(() => cut.Markup.Contains("ACtest_account_sid")); + cut.WaitForState(() => cut.Markup.Contains("ACcccccccccccccccccccccccccccccccc")); cut.FindAll("button").First(b => b.TextContent.Contains("Edit")).Click(); cut.FindAll("input[type=password]").First().Change("rotated-token"); diff --git a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs index 91b05630..4c6cd189 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs @@ -1658,7 +1658,7 @@ public class ManagementActorTests : TestKit, IDisposable var actor = CreateActor(); var envelope = Envelope( - new UpdateSmsConfigCommand(1, "ACnew", "+15551110000", "MGnew", + new UpdateSmsConfigCommand(1, "ACbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", "+15551110000", "MGnew", "https://new.example.com", "new-secret"), "Administrator"); @@ -1666,7 +1666,7 @@ public class ManagementActorTests : TestKit, IDisposable var response = ExpectMsg(TimeSpan.FromSeconds(5)); Assert.Equal(envelope.CorrelationId, response.CorrelationId); - Assert.Equal("ACnew", existing.AccountSid); + Assert.Equal("ACbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", existing.AccountSid); Assert.Equal("+15551110000", existing.FromNumber); Assert.Equal("MGnew", existing.MessagingServiceSid); Assert.Equal("https://new.example.com", existing.ApiBaseUrl); @@ -1690,7 +1690,7 @@ public class ManagementActorTests : TestKit, IDisposable var actor = CreateActor(); // AuthToken + ApiBaseUrl omitted -> preserve-if-null. var envelope = Envelope( - new UpdateSmsConfigCommand(1, "ACnew", "+15551110000"), + new UpdateSmsConfigCommand(1, "ACbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", "+15551110000"), "Administrator"); actor.Tell(envelope); @@ -1701,7 +1701,7 @@ public class ManagementActorTests : TestKit, IDisposable Assert.Equal("old-secret", existing.AuthToken); Assert.Equal("https://old.example.com", existing.ApiBaseUrl); // Provided fields are still updated. - Assert.Equal("ACnew", existing.AccountSid); + Assert.Equal("ACbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", existing.AccountSid); Assert.Equal("+15551110000", existing.FromNumber); } @@ -1726,7 +1726,7 @@ public class ManagementActorTests : TestKit, IDisposable var actor = CreateActor(); var envelope = Envelope( - new UpdateSmsConfigCommand(1, "ACnew", "+15551110000", AuthToken: "super-secret-token"), + new UpdateSmsConfigCommand(1, "ACbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", "+15551110000", AuthToken: "super-secret-token"), "Administrator"); actor.Tell(envelope); @@ -1774,7 +1774,7 @@ public class ManagementActorTests : TestKit, IDisposable // cannot reach it. var actor = CreateActor(); var envelope = Envelope( - new UpdateSmsConfigCommand(1, "ACnew", "+15551110000"), + new UpdateSmsConfigCommand(1, "ACbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", "+15551110000"), "Viewer"); actor.Tell(envelope); @@ -1791,7 +1791,7 @@ public class ManagementActorTests : TestKit, IDisposable // could rotate a production Twilio Auth Token via the CLI/Management API. var actor = CreateActor(); var envelope = Envelope( - new UpdateSmsConfigCommand(1, "ACnew", "+15551110000", AuthToken: "rotate-me"), + new UpdateSmsConfigCommand(1, "ACbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", "+15551110000", AuthToken: "rotate-me"), "Designer"); actor.Tell(envelope); @@ -1818,7 +1818,7 @@ public class ManagementActorTests : TestKit, IDisposable var actor = CreateActor(); var envelope = Envelope( - new UpdateSmsConfigCommand(1, "ACnew", "+15551110000", AuthToken: " "), + new UpdateSmsConfigCommand(1, "ACbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", "+15551110000", AuthToken: " "), "Administrator"); actor.Tell(envelope); @@ -1828,7 +1828,7 @@ public class ManagementActorTests : TestKit, IDisposable // The blank token was ignored; the stored secret survives. Assert.Equal("old-secret", existing.AuthToken); // Non-secret fields still updated. - Assert.Equal("ACnew", existing.AccountSid); + Assert.Equal("ACbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", existing.AccountSid); } [Fact] @@ -1848,7 +1848,7 @@ public class ManagementActorTests : TestKit, IDisposable var actor = CreateActor(); var envelope = Envelope( - new UpdateSmsConfigCommand(1, "ACnew", FromNumber: null, MessagingServiceSid: "MGonly"), + new UpdateSmsConfigCommand(1, "ACbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", FromNumber: null, MessagingServiceSid: "MGonly"), "Administrator"); actor.Tell(envelope); @@ -1859,6 +1859,63 @@ public class ManagementActorTests : TestKit, IDisposable Assert.Equal("MGonly", existing.MessagingServiceSid); } + [Fact] + public void UpdateSmsConfig_MalformedAccountSid_Rejected() + { + // Task 22: AccountSid is interpolated un-escaped into the Twilio request URI, + // so a malformed SID (e.g. a path-traversal / query-injection payload) must be + // rejected at save time, closing the URI-injection door before it can be persisted. + var notifRepo = Substitute.For(); + var existing = new Commons.Entities.Notifications.SmsConfiguration("ACaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", "+15550000000") + { + Id = 1, + AuthToken = "old-secret", + }; + notifRepo.GetAllSmsConfigurationsAsync(Arg.Any()) + .Returns(new List { existing }); + _services.AddScoped(_ => notifRepo); + + var actor = CreateActor(); + var envelope = Envelope( + new UpdateSmsConfigCommand(1, "../evil?x=", "+15551110000"), + "Administrator"); + + actor.Tell(envelope); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Equal("COMMAND_FAILED", response.ErrorCode); + Assert.Contains("Account SID", response.Error); + // The malformed value must never have been persisted. + Assert.Equal("ACaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", existing.AccountSid); + } + + [Fact] + public void UpdateSmsConfig_ValidSid_Accepted() + { + // Task 22: a well-formed Twilio SID ('AC' + 32 hex chars) passes the guard. + var notifRepo = Substitute.For(); + var existing = new Commons.Entities.Notifications.SmsConfiguration("ACold", "+15550000000") + { + Id = 1, + AuthToken = "old-secret", + }; + notifRepo.GetAllSmsConfigurationsAsync(Arg.Any()) + .Returns(new List { existing }); + _services.AddScoped(_ => notifRepo); + + var validSid = "AC" + new string('a', 32); + var actor = CreateActor(); + var envelope = Envelope( + new UpdateSmsConfigCommand(1, validSid, "+15551110000"), + "Administrator"); + + actor.Tell(envelope); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Equal(envelope.CorrelationId, response.CorrelationId); + Assert.Equal(validSid, existing.AccountSid); + } + [Fact] public void CuratedHandlerFailure_SurfacesTheCuratedMessage() {