From 819a69033552df5a0b944e2c57d8758aa7e84f3f Mon Sep 17 00:00:00 2001 From: Ramon Smits Date: Fri, 9 Oct 2026 15:58:43 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Do=20not=20return=20the=20SMTP?= =?UTF-8?q?=20password=20from=20the=20notifications=20API=20(#5980)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GET /api/notifications/email returned the stored SMTP password to every caller that can read the settings. This includes the reader role and, with authentication disabled (the default), anonymous callers. The password is now write-only: - GET never returns it. - A save with an empty password keeps the stored password. ServicePulse sends its form back without a password, because it no longer receives one. - A save without an account removes the stored password. Tests: - NotificationsControllerTests (unit) cover the GET response and how each kind of save changes the stored password. - One acceptance test checks the HTTP response end to end. (cherry picked from commit 5e2364fc54fdeca01a4803da432bfcff25f07b1c) --- ...otification_settings_contain_a_password.cs | 62 ++++++++++ .../NotificationsControllerTests.cs | 107 ++++++++++++++++++ .../Api/NotificationsController.cs | 19 +++- 3 files changed, 187 insertions(+), 1 deletion(-) create mode 100644 src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_email_notification_settings_contain_a_password.cs create mode 100644 src/ServiceControl.UnitTests/Notifications/NotificationsControllerTests.cs diff --git a/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_email_notification_settings_contain_a_password.cs b/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_email_notification_settings_contain_a_password.cs new file mode 100644 index 0000000000..b33a40ed37 --- /dev/null +++ b/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_email_notification_settings_contain_a_password.cs @@ -0,0 +1,62 @@ +namespace ServiceControl.AcceptanceTests.Monitoring.CustomChecks +{ + using System.Threading.Tasks; + using AcceptanceTesting; + using NServiceBus.AcceptanceTesting; + using NUnit.Framework; + using ServiceControl.Notifications; + + /// + /// The SMTP password is write-only: the settings route never returns it. This checks the HTTP + /// response end to end. NotificationsControllerTests covers how a save changes the stored password. + /// + class When_email_notification_settings_contain_a_password : AcceptanceTest + { + [Test] + public async Task Should_not_return_the_password() + { + EmailNotifications returned = null; + + await Define() + .Do("Save settings with a password", async _ => + { + await this.Post("/api/notifications/email", SettingsRequest(Account, Password)); + + return true; + }) + .Do("Read the settings back", async _ => + { + returned = await this.TryGet("/api/notifications/email"); + + return returned != null; + }) + .Done(_ => true) + .Run(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(returned.AuthenticationAccount, Is.EqualTo(Account), "The account is not a secret and stays visible"); + Assert.That(returned.AuthenticationPassword, Is.Null, "The password must never leave the instance through the API"); + } + } + + static object SettingsRequest(string account, string password) => new + { + smtp_server = "localhost", + smtp_port = 25, + authorization_account = account, + authorization_password = password, + enable_tls = false, + from = "servicecontrol@particular.net", + to = "oncall@particular.net" + }; + + const string Account = "svc-notifications"; + const string Password = "p@ssw0rd"; + + public class Context : ScenarioContext, ISequenceContext + { + public int Step { get; set; } + } + } +} diff --git a/src/ServiceControl.UnitTests/Notifications/NotificationsControllerTests.cs b/src/ServiceControl.UnitTests/Notifications/NotificationsControllerTests.cs new file mode 100644 index 0000000000..1f241bcb14 --- /dev/null +++ b/src/ServiceControl.UnitTests/Notifications/NotificationsControllerTests.cs @@ -0,0 +1,107 @@ +#nullable enable +namespace ServiceControl.UnitTests.Notifications; + +using System.Threading; +using System.Threading.Tasks; +using NUnit.Framework; +using ServiceControl.Notifications; +using ServiceControl.Notifications.Api; +using ServiceControl.Persistence; + +/// +/// The SMTP password is write-only: the settings route never returns it, and a save without a +/// password keeps the stored one. ServicePulse reads the settings into its form and sends the form +/// back, so a save without a password must not erase the password that was stored before. +/// +[TestFixture] +class NotificationsControllerTests +{ + [Test] + public async Task Should_not_return_the_password() + { + var store = new InMemoryNotificationsDataStore(Account, Password); + + var returned = await Controller(store).GetEmailNotificationsSettings(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(returned.AuthenticationAccount, Is.EqualTo(Account), "The account is not a secret and stays visible"); + Assert.That(returned.AuthenticationPassword, Is.Null, "The password must never leave the instance through the API"); + Assert.That(store.Stored.AuthenticationPassword, Is.EqualTo(Password), "Reading the settings must not change the stored password"); + } + } + + [Test] + public async Task Should_keep_the_stored_password_when_saved_without_one() + { + var store = new InMemoryNotificationsDataStore(Account, Password); + + await Controller(store).UpdateSettings(Request(Account, password: null)); + + Assert.That(store.Stored.AuthenticationPassword, Is.EqualTo(Password)); + } + + [Test] + public async Task Should_replace_the_stored_password_when_saved_with_a_new_one() + { + var store = new InMemoryNotificationsDataStore(Account, Password); + + await Controller(store).UpdateSettings(Request(Account, NewPassword)); + + Assert.That(store.Stored.AuthenticationPassword, Is.EqualTo(NewPassword)); + } + + [Test] + public async Task Should_remove_the_stored_password_when_saved_without_an_account() + { + var store = new InMemoryNotificationsDataStore(Account, Password); + + await Controller(store).UpdateSettings(Request(account: null, password: null)); + + Assert.That(store.Stored.AuthenticationPassword, Is.Null, "Without an account the password is never used, so it must not stay stored"); + } + + static NotificationsController Controller(INotificationsDataStore store) => new(store, null!, null!); + + static UpdateEmailNotificationsSettingsRequest Request(string? account, string? password) => new() + { + SmtpServer = "localhost", + SmtpPort = 25, + AuthorizationAccount = account, + AuthorizationPassword = password, + EnableTLS = false, + From = "servicecontrol@particular.net", + To = "oncall@particular.net" + }; + + const string Account = "svc-notifications"; + const string Password = "p@ssw0rd"; + const string NewPassword = "n3w-p@ssw0rd"; + + // Loads and saves copies, like the real stores, so the controller cannot change the stored value in place. + class InMemoryNotificationsDataStore(string account, string password) : INotificationsDataStore + { + public EmailNotifications Stored { get; private set; } = new() { AuthenticationAccount = account, AuthenticationPassword = password }; + + public Task LoadSettings(CancellationToken cancellationToken = default) => + Task.FromResult(new NotificationsSettings { Email = Copy(Stored) }); + + public Task SaveSettings(NotificationsSettings settings, CancellationToken cancellationToken = default) + { + Stored = Copy(settings.Email); + return Task.CompletedTask; + } + + static EmailNotifications Copy(EmailNotifications source) => new() + { + Enabled = source.Enabled, + SmtpServer = source.SmtpServer, + SmtpPort = source.SmtpPort, + AuthenticationAccount = source.AuthenticationAccount, + AuthenticationPassword = source.AuthenticationPassword, + EnableTLS = source.EnableTLS, + To = source.To, + From = source.From + }; + } +} diff --git a/src/ServiceControl/Notifications/Api/NotificationsController.cs b/src/ServiceControl/Notifications/Api/NotificationsController.cs index 91ea380b2f..c9ef2655d5 100644 --- a/src/ServiceControl/Notifications/Api/NotificationsController.cs +++ b/src/ServiceControl/Notifications/Api/NotificationsController.cs @@ -22,6 +22,11 @@ public async Task GetEmailNotificationsSettings(Cancellation { var notificationsSettings = await store.LoadSettings(cancellationToken); + // The SMTP password is write-only. It is a credential for the customer's mail system, so it + // never leaves the instance, whatever the caller's permissions. LoadSettings returns a detached + // snapshot, so this does not change the stored password. + notificationsSettings.Email.AuthenticationPassword = null; + return notificationsSettings.Email; } @@ -52,7 +57,19 @@ public async Task UpdateSettings(UpdateEmailNotificationsSettings emailSettings.SmtpPort = request.SmtpPort; emailSettings.AuthenticationAccount = request.AuthorizationAccount; - emailSettings.AuthenticationPassword = request.AuthorizationPassword; + + // The settings route does not return the password, so ServicePulse sends its form back without + // one. An empty password keeps the stored one. Without an account the password is never used, + // so it is removed. + if (string.IsNullOrEmpty(request.AuthorizationAccount)) + { + emailSettings.AuthenticationPassword = null; + } + else if (!string.IsNullOrEmpty(request.AuthorizationPassword)) + { + emailSettings.AuthenticationPassword = request.AuthorizationPassword; + } + emailSettings.EnableTLS = request.EnableTLS; emailSettings.From = request.From;