dohertj2/ScadaBridge
1
0
Fork 0
Code Issues 1 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(notifications): validate Twilio AccountSid format at save — closes the un-escaped URI interpolation door

Browse Source 
Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj
This commit is contained in:
Joseph Doherty
2026-07-10 04:36:19 -04:00
parent 37cf7c1a44
commit 40088a2123
4 changed files with 90 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Notifications/SmsConfiguration.razor
+9
View File
@@ -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
src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs
+5
View File
@@ -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;
tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/SmsConfigurationPageTests.cs
+9 -9
View File
@@ -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<SmsConfigurationPage>();
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<SmsConfiguration>(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<SmsConfiguration>(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<SmsConfigurationPage>();
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<SmsConfigurationPage>();
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");
tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs
+67 -10
View File
@@ -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<ManagementSuccess>(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<INotificationRepository>();
var existing = new Commons.Entities.Notifications.SmsConfiguration("ACaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", "+15550000000")
{
Id = 1,
AuthToken = "old-secret",
};
notifRepo.GetAllSmsConfigurationsAsync(Arg.Any<CancellationToken>())
.Returns(new List<Commons.Entities.Notifications.SmsConfiguration> { existing });
_services.AddScoped(_ => notifRepo);
var actor = CreateActor();
var envelope = Envelope(
new UpdateSmsConfigCommand(1, "../evil?x=", "+15551110000"),
"Administrator");
actor.Tell(envelope);
var response = ExpectMsg<ManagementError>(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<INotificationRepository>();
var existing = new Commons.Entities.Notifications.SmsConfiguration("ACold", "+15550000000")
{
Id = 1,
AuthToken = "old-secret",
};
notifRepo.GetAllSmsConfigurationsAsync(Arg.Any<CancellationToken>())
.Returns(new List<Commons.Entities.Notifications.SmsConfiguration> { 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<ManagementSuccess>(TimeSpan.FromSeconds(5));
Assert.Equal(envelope.CorrelationId, response.CorrelationId);
Assert.Equal(validSid, existing.AccountSid);
}
[Fact]
public void CuratedHandlerFailure_SurfacesTheCuratedMessage()
{