Skip to content

Keep stored secrets when a save carries the redaction sentinel #994

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
m4bard wants to merge 1 commit into
base: canary
Choose a base branch
from
Open

Keep stored secrets when a save carries the redaction sentinel #994

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 64 additions & 1 deletion listenarr.application/Configuration/Core/ConfigurationService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,41 @@ public async Task SaveApplicationSettingsAsync(ApplicationSettings settings)
settings.EnabledNotificationTriggers = existing.EnabledNotificationTriggers;
if (settings.Webhooks == null)
settings.Webhooks = existing.Webhooks;

// The other fields RedactApplicationSettings covers need the
// same sentinel check as ProwlarrApiKeyEncrypted above. A
// redaction-gated caller is handed RedactedValue for each of
// them by the GET and posts it back verbatim, so without this
// the sentinel overwrites the stored secret. Blank is left
// alone here for the same reason as above: clearing a webhook
// URL or a bot token has to stay possible.
if (string.Equals(settings.WebhookUrl, ApiResponseRedactor.RedactedValue, StringComparison.Ordinal))
{
settings.WebhookUrl = existing.WebhookUrl;
}

if (string.Equals(settings.DiscordBotToken, ApiResponseRedactor.RedactedValue, StringComparison.Ordinal))
{
settings.DiscordBotToken = existing.DiscordBotToken;
}

if (settings.Webhooks != null && existing.Webhooks != null)
{
foreach (var webhook in settings.Webhooks)
{
if (!string.Equals(webhook.Url, ApiResponseRedactor.RedactedValue, StringComparison.Ordinal))
{
continue;
}

var storedWebhook = existing.Webhooks
.FirstOrDefault(candidate => string.Equals(candidate.Id, webhook.Id, StringComparison.Ordinal));
if (storedWebhook != null)
{
webhook.Url = storedWebhook.Url;
}
}
}
}

if (!string.IsNullOrWhiteSpace(settings.OutputPath)
Expand Down Expand Up @@ -357,6 +392,35 @@ public async Task SaveStartupConfigAsync(StartupConfig config)
{
try
{
var currentConfig = startupConfigService.GetConfig();

// The GET that populates the settings screen replaces each
// secret with ApiResponseRedactor.RedactedValue for callers the
// redaction gate does not exempt, and the screen posts that same
// document straight back when the operator saves. Treat the
// sentinel as "unchanged" and keep what is already stored, the
// way SaveApplicationSettingsAsync and
// SaveProwlarrImportSettingsAsync already do for the Prowlarr
// key. Without this the literal sentinel lands in config.json
// and the real value is gone.
//
// Only the sentinel is special-cased. A blank or absent field
// still means what it has always meant here, which is clear the
// value, so an operator who empties the API key box can still
// do so.
if (config != null && currentConfig != null)
{
if (string.Equals(config.ApiKey, ApiResponseRedactor.RedactedValue, StringComparison.Ordinal))
{
config.ApiKey = currentConfig.ApiKey;
}

if (string.Equals(config.SslCertPassword, ApiResponseRedactor.RedactedValue, StringComparison.Ordinal))
{
config.SslCertPassword = currentConfig.SslCertPassword;
}
}

// Defense-in-depth backstop against the auth-enable lockout.
// SaveApplicationSettingsAsync's throw-on-failure (above) only
// covers the case where admin credentials were *supplied* but
Expand All @@ -378,7 +442,6 @@ public async Task SaveStartupConfigAsync(StartupConfig config)
// management path, not here.
if (config != null && config.IsAuthenticationEnabled())
{
var currentConfig = startupConfigService.GetConfig();
var wasAuthEnabled = currentConfig?.IsAuthenticationEnabled() == true;
if (!wasAuthEnabled)
{
Expand Down
189 changes: 189 additions & 0 deletions tests/Features/Application/Configuration/Core/ConfigurationServiceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,195 @@ await svc.SaveStartupConfigAsync(new StartupConfig
currentConfigEnabled.Verify(s => s.SaveAsync(It.IsAny<StartupConfig>()), Times.Once);
}

[Fact]
public async Task SaveStartupConfig_RedactedSecrets_KeepStoredValues()
{
// GET /configuration/startupconfig hands a redaction-gated caller
// ApiResponseRedactor.RedactedValue in place of the API key and the
// SSL certificate password, and the settings screen posts that same
// document back on save. The sentinel must be read as "unchanged",
// not written to config.json on top of the real secret.
var stored = new StartupConfig
{
AuthenticationRequired = "false",
ApiKey = "stored-api-key",
SslCertPassword = "stored-cert-password",
Port = 5000,
};

var startupConfigService = new Mock<IStartupConfigService>();
startupConfigService.Setup(s => s.GetConfig()).Returns(stored);
StartupConfig? saved = null;
startupConfigService.Setup(s => s.SaveAsync(It.IsAny<StartupConfig>()))
.Callback<StartupConfig>(c => saved = c)
.Returns(Task.CompletedTask);

Init(b => b.WithSingleton(startupConfigService.Object));
var svc = _provider.GetRequiredService<IConfigurationService>();

// Build the incoming payload the way the API actually produces it,
// so the test exercises the real redaction sentinel rather than a
// hand-written copy of it.
var incoming = ApiResponseRedactor.RedactStartupConfig(stored);
Assert.Equal(ApiResponseRedactor.RedactedValue, incoming.ApiKey);
Assert.Equal(ApiResponseRedactor.RedactedValue, incoming.SslCertPassword);

await svc.SaveStartupConfigAsync(incoming);

Assert.NotNull(saved);
Assert.Equal("stored-api-key", saved!.ApiKey);
Assert.Equal("stored-cert-password", saved.SslCertPassword);
}

[Fact]
public async Task SaveStartupConfig_NewSecrets_ReplaceStoredValues()
{
// Control for the test above. The sentinel check must not degrade
// into "never update these fields": an operator who types a new API
// key or a new certificate password still has to be able to save it.
var stored = new StartupConfig
{
AuthenticationRequired = "false",
ApiKey = "stored-api-key",
SslCertPassword = "stored-cert-password",
};

var startupConfigService = new Mock<IStartupConfigService>();
startupConfigService.Setup(s => s.GetConfig()).Returns(stored);
StartupConfig? saved = null;
startupConfigService.Setup(s => s.SaveAsync(It.IsAny<StartupConfig>()))
.Callback<StartupConfig>(c => saved = c)
.Returns(Task.CompletedTask);

Init(b => b.WithSingleton(startupConfigService.Object));
var svc = _provider.GetRequiredService<IConfigurationService>();

await svc.SaveStartupConfigAsync(new StartupConfig
{
AuthenticationRequired = "false",
ApiKey = "rotated-api-key",
SslCertPassword = "rotated-cert-password",
});

Assert.NotNull(saved);
Assert.Equal("rotated-api-key", saved!.ApiKey);
Assert.Equal("rotated-cert-password", saved.SslCertPassword);
}

[Fact]
public async Task SaveStartupConfig_BlankOrAbsentSecrets_AreWrittenThroughUnchanged()
{
// Second control. Only the sentinel is special-cased; blank keeps
// the meaning it has always had on this path, which is clear the
// value. Preserving on blank as well would take away the operator's
// way of removing an API key or a certificate password.
var stored = new StartupConfig
{
AuthenticationRequired = "false",
ApiKey = "stored-api-key",
SslCertPassword = "stored-cert-password",
};

var startupConfigService = new Mock<IStartupConfigService>();
startupConfigService.Setup(s => s.GetConfig()).Returns(stored);
var savedConfigs = new List<StartupConfig>();
startupConfigService.Setup(s => s.SaveAsync(It.IsAny<StartupConfig>()))
.Callback<StartupConfig>(savedConfigs.Add)
.Returns(Task.CompletedTask);

Init(b => b.WithSingleton(startupConfigService.Object));
var svc = _provider.GetRequiredService<IConfigurationService>();

await svc.SaveStartupConfigAsync(new StartupConfig
{
AuthenticationRequired = "false",
ApiKey = string.Empty,
SslCertPassword = string.Empty,
});
await svc.SaveStartupConfigAsync(new StartupConfig
{
AuthenticationRequired = "false",
ApiKey = null,
SslCertPassword = null,
});

Assert.Equal(2, savedConfigs.Count);
Assert.Equal(string.Empty, savedConfigs[0].ApiKey);
Assert.Equal(string.Empty, savedConfigs[0].SslCertPassword);
Assert.Null(savedConfigs[1].ApiKey);
Assert.Null(savedConfigs[1].SslCertPassword);
}

[Fact]
public async Task SaveApplicationSettings_RedactedSecrets_KeepStoredValues()
{
// Same round trip on the settings row. ProwlarrApiKeyEncrypted was
// already guarded; the rest of what RedactApplicationSettings covers
// was not, so the sentinel reached the database.
var svc = _provider.GetRequiredService<IConfigurationService>();

var seed = await svc.GetApplicationSettingsAsync();
seed.WebhookUrl = "https://example.test/global-hook";
seed.DiscordBotToken = "stored-discord-token";
seed.Webhooks =
[
new() { Id = "webhook-1", Name = "First", Url = "https://example.test/one", Type = "Zapier" },
new() { Id = "webhook-2", Name = "Second", Url = "https://example.test/two", Type = "Slack" },
];
await svc.SaveApplicationSettingsAsync(seed);

var current = await svc.GetApplicationSettingsAsync();
var incoming = ApiResponseRedactor.RedactApplicationSettings(current);
Assert.Equal(ApiResponseRedactor.RedactedValue, incoming.WebhookUrl);
Assert.Equal(ApiResponseRedactor.RedactedValue, incoming.DiscordBotToken);
Assert.All(incoming.Webhooks!, w => Assert.Equal(ApiResponseRedactor.RedactedValue, w.Url));

await svc.SaveApplicationSettingsAsync(incoming);

var stored = await _applicationSettingsRepository.GetAsync();
Assert.NotNull(stored);
Assert.Equal("https://example.test/global-hook", stored!.WebhookUrl);
Assert.Equal("stored-discord-token", stored.DiscordBotToken);
Assert.NotNull(stored.Webhooks);
Assert.Equal(
"https://example.test/one",
stored.Webhooks!.Single(w => w.Id == "webhook-1").Url);
Assert.Equal(
"https://example.test/two",
stored.Webhooks!.Single(w => w.Id == "webhook-2").Url);
}

[Fact]
public async Task SaveApplicationSettings_NewSecrets_ReplaceStoredValues()
{
// Control for the test above, on the settings row this time.
var svc = _provider.GetRequiredService<IConfigurationService>();

var seed = await svc.GetApplicationSettingsAsync();
seed.WebhookUrl = "https://example.test/global-hook";
seed.DiscordBotToken = "stored-discord-token";
seed.Webhooks =
[
new() { Id = "webhook-1", Name = "First", Url = "https://example.test/one", Type = "Zapier" },
];
await svc.SaveApplicationSettingsAsync(seed);

var incoming = await svc.GetApplicationSettingsAsync();
incoming.WebhookUrl = "https://example.test/changed-hook";
incoming.DiscordBotToken = "rotated-discord-token";
incoming.Webhooks!.Single(w => w.Id == "webhook-1").Url = "https://example.test/changed-one";

await svc.SaveApplicationSettingsAsync(incoming);

var stored = await _applicationSettingsRepository.GetAsync();
Assert.NotNull(stored);
Assert.Equal("https://example.test/changed-hook", stored!.WebhookUrl);
Assert.Equal("rotated-discord-token", stored.DiscordBotToken);
Assert.Equal(
"https://example.test/changed-one",
stored.Webhooks!.Single(w => w.Id == "webhook-1").Url);
}

[Fact]
public async Task SaveStartupConfig_SkipsAdminCheck_WhenAuthDisabled()
{
Expand Down