Keep stored secrets when a save carries the redaction sentinel

[m4bard]

Keep stored secrets when a save carries the redaction sentinel

What happens to an operator

Open Settings, change any field, save. The instance API key and the SSL certificate password are both gone, replaced by the literal string REDACTED. The save reports success and the page redraws as normal, so there is nothing to notice at the time.

It sticks. The value is on disk in config.json, so a restart does not help, and a later save from a session that is not affected writes the placeholder again rather than repairing it. From the session that caused it there is no way back at all: RequireApiKeyManagementAccess turns that same caller away from both GET apikey and POST apikey/regenerate, so the key control on the screen comes up empty and a regenerate is refused with a 403 (listenarr.api/Attributes/RequireApiKeyManagementAccessAttribute.cs:49-81). Recovery means reaching the instance some other way, either regenerating from a session that gate does allow or putting the old value back into config.json by hand. Regenerating is the easier of the two but it rotates the key, so anything already configured with the old one has to be updated.

Not every caller hits this. It needs a request that HttpSecurityRequestUtils.ShouldRedactSecretsForCaller answers true for, which is a remote address the helper does not treat as private or loopback, on a request that is not already an authenticated admin or API-key principal (listenarr.api/Security/SecurityRequestHttpContextExtensions.cs:67). The plainest way to reach it is an instance with the login screen off, opened from an address outside the private ranges: a directly exposed instance, or one behind a proxy configured to forward the real client address. With the login screen off, RequireAdminOrApiKeyAttribute lets the request through, and nothing else stands between the browser and the save.

Why it happens

The settings screen fetches the startup config when it mounts and posts the same document back when the operator saves:

• fe/src/views/SettingsView.vue:1380 stores whatever GET /configuration/startupconfig returned.
• fe/src/views/SettingsView.vue:845 spreads that stored document into the outgoing payload and changes only authenticationRequired.
• fe/src/views/SettingsView.vue:850 posts it.

For a caller the redaction gate does not exempt, the document fetched in step one is the redacted one: StartupConfigurationController.cs:75-78 runs it through ApiResponseRedactor.RedactStartupConfig, which puts RedactedValue in ApiKey and SslCertPassword (ApiResponseRedactor.cs:89-103). Nothing on the way back in looks at that. ConfigurationService.SaveStartupConfigAsync passes the payload to startupConfigService.SaveAsync as it stands, and the sentinel is serialised into config.json on top of the real value.

Two things make it harder to notice:

• The POST response is redacted again on the way out (StartupConfigurationController.cs:105-107), so the client is shown REDACTED either way and cannot tell the difference between a successful save and a destroyed key.
• There is a comment above the block in SettingsView.vue:818 reading "If user toggled the authEnabled, attempt to save to startup config", which reads as though the save is conditional. It is not. didEnableAuth and didDisableAuth are computed a few lines down but only gate what happens to the session afterwards, at lines 907 and 918. The save itself runs on every settings save.

I think the settings row has the same shape, though this part is a code reading rather than something I observed. RedactApplicationSettings (ApiResponseRedactor.cs:57-87) covers WebhookUrl, DiscordBotToken, ProwlarrApiKeyEncrypted and each Webhooks[].Url, and SettingsController.cs:61 applies it behind the same gate. Of those, only ProwlarrApiKeyEncrypted is checked on the way back in. AdminUsername and AdminPassword are nulled rather than given the sentinel, and SaveApplicationSettingsAsync already skips provisioning when they are blank, so those two look fine to me.

The fix

You already handle this correctly for the Prowlarr key, in two places in the same file:

• ConfigurationService.cs:158-162, in SaveApplicationSettingsAsync
• ConfigurationService.cs:307-311, in SaveProwlarrImportSettingsAsync

Both compare the incoming value against ApiResponseRedactor.RedactedValue and keep what is stored when it matches. The other secret-bearing fields never got the same check. This PR adds it, in the same shape and with the same comparison, to:

• StartupConfig.ApiKey and StartupConfig.SslCertPassword in SaveStartupConfigAsync. The current config was already being read a few lines lower for the auth-enable backstop, so the read is hoisted and reused rather than repeated.
• WebhookUrl, DiscordBotToken and each Webhooks[].Url in SaveApplicationSettingsAsync, next to the ProwlarrApiKeyEncrypted check that is already there. Webhook URLs are matched back to the stored list by Id.

This stops the value being destroyed. It does not repair a config.json that already holds the sentinel, which still needs a regeneration or a hand edit as above.

There is one deliberate difference from the two checks that already exist. Those treat blank and the sentinel alike and preserve for both. I have matched only the sentinel, because on these paths a blank value currently clears the field, and an operator has to keep being able to remove an API key or empty a webhook URL. Narrowing it to the sentinel is what the bug calls for and leaves every other behaviour where it was. Happy to widen it to match the neighbouring checks if you would rather the file be uniform.

Tests

Five tests in tests/Features/Application/Configuration/Core/ConfigurationServiceTests.cs. I build the payloads by calling the real ApiResponseRedactor rather than writing "REDACTED" into the test, so they exercise the round trip and not my reading of it.

• SaveStartupConfig_RedactedSecrets_KeepStoredValues
• SaveApplicationSettings_RedactedSecrets_KeepStoredValues

and three controls, which matter more than the two above:

• SaveStartupConfig_NewSecrets_ReplaceStoredValues and SaveApplicationSettings_NewSecrets_ReplaceStoredValues, so the check cannot quietly become "never update these fields". A save carrying a genuinely new key or a new certificate password still has to replace what is stored.
• SaveStartupConfig_BlankOrAbsentSecrets_AreWrittenThroughUnchanged, covering the narrowing described above. Blank and null still write through and clear the value.

With the production change stashed and only the tests applied, the suite is 2 failed, 3121 passed, 130 skipped. With the change, 0 failed, 3123 passed, 130 skipped. The three controls pass in both runs, which is the point of them.

dotnet format --verify-no-changes is clean on both files.

Verified on a real install

This was not only measured on a throwaway. An install running this build had its API key destroyed by ordinary use: routine settings saves from a browser on another machine, across two sessions, with nothing unusual being done and nothing to notice at the time.

After deploying the fix to that same install, the same path was run again deliberately:

• The API key was regenerated, so there was a real value in the store rather than the placeholder. This matters: with the placeholder already stored there is nothing for the guard to protect, and an unchanged value afterwards would prove nothing either way.
• The new key was fingerprinted before the test, by SHA-256 rather than by reading it out, and confirmed two ways: a loopback API read and a direct read of config.json hashed to the same value.
• Two settings saves were made from the same remote browser session that had destroyed the key before.
• config.json was then read directly rather than through the API, because a remote GET returns the placeholder whether or not the stored value survived.

The hash after matched the hash before. That is the same value surviving, not merely something that is not the placeholder.

SslCertPassword and the webhook list were confirmed unchanged on the same pass.

One thing worth passing on from doing this. Regenerating from loopback is harder than it looks, because the listener is IPv6-only inside the container and a request from the host to localhost:4545 arrives over the bridge, which the local-address check does not accept. Reaching http://[::1]:4545 from inside the container is what worked.

Every line number above is from a630572e983614a52ea409a23da52a99e3b8b91b.

Disclosure: drafted with Claude Code at my direction; I read the cited code at the stated commit and reviewed this before posting.