[m4bard]

This bundle collects three commits under one theme: keeping credentials out of logs, and making sure a failure that is swallowed in a catch block leaves a trace somewhere. Two are fixes from earlier work and the third is a small follow-up to the first, one commit per fix so any can be dropped or cherry-picked on its own if you would rather take them separately.

Commits

commit	original PR	issue	effect
6d3ef5c	#987	Fixes #982	redacts the credential from a notification webhook URL before it reaches a log line
18dbbbb	new	Fixes #928	gives each of the 78 silent catch blocks a real log call or a one-line comment saying why not, plus a guard test
dcd9c37	integration		closes two gaps in the webhook-URL sanitizer: legacy discordapp.com, and pushbullet://

6d3ef5c: sanitize webhook URLs before logging them

NotificationService.Webhooks.cs logged outbound webhook URLs through LogRedaction.RedactText(webhookUrl, LogRedaction.GetSensitiveValuesFromEnvironment()) at several log levels, including Information (NotificationService.Webhooks.cs:178, :237 and :422 at a630572e9). That helper only replaces values sourced from the process environment, so a webhook URL typed into Settings went out whole regardless of the level it was logged at. The credential sits in the query string for Pushover and Pushbullet and in a path segment for Telegram, Discord and Slack.

The fix adds LogRedaction.SanitizeWebhookUrl (listenarr.application/Security/Redaction/LogRedaction.cs:133 at the bundle head). It keeps scheme, host and port, drops userinfo and the whole query string, and for the three providers whose credential lives in the path masks just the segment carrying it:

• api.telegram.org/bot<token>/sendMessage becomes api.telegram.org/bot<redacted>/sendMessage
• discord.com/api/webhooks/<id>/<token> becomes discord.com/api/webhooks/<redacted>/<redacted>
• hooks.slack.com/services/<team>/<bot>/<token> becomes hooks.slack.com/services/<redacted>/<redacted>/<redacted>
• every other host keeps its path and loses its query, same as SanitizeUrl

30 {WebhookUrl} templates across the file's log messages now resolve through 25 calls to it (several log lines share one sanitized value computed once per request). It also replaces NotificationDiagnostics.LogFailedResponseAsync's call. The Pushover form body, which carries token= and user= as plain form fields that the existing AggressiveRedact helper could not catch, is dropped from its Information log line instead of redacted; this commit also removes the TryReadContentAsync read of that body, since nothing downstream needs it once it is no longer logged.

Tests, carried over from #987 and still present at the bundle head: LogRedactionTests.SanitizeWebhookUrl_Telegram_MasksTokenSegment_KeepsMethodAndQueryDropped, ..._Discord_MasksIdAndTokenSegments_KeepsApiWebhooksPath, ..._Slack_MasksEverythingAfterServices, ..._Pushover_DropsQueryString_KeepsHostAndPath, plus a control case with no credential-shaped content. SecurityRedactionTests adds integration-level tests (NotificationService_PushoverSuccessLog_DoesNotLeakTokenOrUser, ..._TelegramSuccessLog_DoesNotLeakBotToken, ..._DiscordHttpErrorLog_DoesNotLeakWebhookIdOrToken) that run the real send path with a credential that is deliberately not an environment variable and capture the formatted log line through a mocked ILogger.

NotificationService.Webhooks.cs is also touched by two open PRs: #754, which fixes a double-posting bug through the generic sender fall-through, and #943, which is my own open PR for trigger dispatch. Both still merge cleanly with this bundle as of a630572; happy to rebase whichever lands second.

Full write-up in #987.

18dbbbb: give silent catch blocks a real log call or a stated reason

System.Diagnostics.Debug.WriteLine carries [Conditional("DEBUG")], so in the Release build the Dockerfile and CI publish, the call and its argument compile out entirely. A catch block whose whole body was such a call logged nothing and bound its exception to nothing that survives. This commit gives all 78 such sites at a630572e9 a real log call or a one-line comment stating why not, which is what #928 asks for: it allows a catch to stay silent once the reason is written down, it just has to be written down.

I want to flag something about timing: my earlier comments on #928 said I would wait for a choice between the three treatments before sending a sweep. This commit sends it anyway, ahead of that answer, because I think a concrete diff is easier to react to than an open question, and every per-site choice is a one-line change if a different default is wanted. It is a separate commit from the webhook-URL fix, so it can be dropped on its own, and if you would rather land only the guard test with the existing sites allow-listed and have the sweep follow separately, that is also an option; say which you would prefer.

77 of the 78 sites are the Debug.WriteLine-only shape (measured with the guard test itself: copying BackendArchitectureTests.ProductionCatchBlocks_DoNotRelyOnDebugWriteLineAlone onto a630572e9 unmodified and running it there reports exactly 77 violations, file:line for each). The 78th is QbittorrentConnectionTester.cs, which already called _logger.LogDebug but dropped the exception, so the guard test's Debug.WriteLine-only pattern does not count it; I count it by hand from the fix's own diff.

Each site now gets one of three treatments:

• 7 get LogWarning with the exception, where swallowing changes something an operator could otherwise see: a stale cache entry after a mapping change in RemotePathMappingService (3), a stored trigger list kept in its raw form in ConfigurationService.Helpers (2), a bot identity left unset in DiscordController (1), and a process-execution record that failed to persist in ProcessExecutionStore (1, see below).
• 30 get LogDebug with the exception, where the fallback is the designed path and the detail matters only once someone raises the log level.
• 41 get no log call and a one-line comment stating why. 22 of those are a catch wrapped around a log call itself, where logging again from the handler is not a fix and can throw out of a handler that is not supposed to. Three more are redaction paths (two in LogRedaction.cs, one in NotificationDiagnostics.AggressiveRedact) where the exception message can carry the value being redacted. The rest have no logger in scope: an EF value converter, static parsing helpers, the design-time DbContext factory.

Beyond the catch bodies themselves, the non-logging changes in this commit are:

• Four catch clauses that bound no exception variable now do, so it can be passed to the logger: one in DiscordController, two in ConfigurationService.Helpers, one in StartupDbNormalizer.
• ConfigurationService.NormalizeTriggerList went from a private static method to an instance method so it can reach the service's logger (listenarr.application/Configuration/Core/ConfigurationService.Helpers.cs:25 at the bundle head). I think it is worth flagging that this method already swallows JsonException on the decode path and logs its own warning there (ConfigurationService.Helpers.cs:38), so the caller's own catch (JsonException) and LogWarning around both call sites (ConfigurationService.cs:202) cannot fire for that path. That looked like a pre-existing behavior question rather than a logging one, so I left it as is.
• ProcessExecutionStore's constructor gains an ILogger parameter; it had none before. IProcessExecutionStore is registered at listenarr.infrastructure/DependencyInjection/SystemDiagnostics/SystemDiagnosticRegistrationExtensions.cs:22, but git grep finds nothing that injects or resolves it, so as far as I can tell the new warning there cannot fire today (read). If a constructor change for a class with no current consumer is not wanted, a comment there like the other 41 sites would do just as well; I am glad to switch it if you would rather.

One more left as is: in SabnzbdResponseMapper, the swallowed exception is close to unreachable (listenarr.infrastructure/DownloadClients/Sabnzbd/SabnzbdResponseMapper.cs:227-233). The wrong value comes from the return 0.0 a few lines below when TryParse fails, and no log line in the catch would show that on its own.

None of the new log lines has been observed firing on a running instance. The evidence here is the guard test below plus the full suite.

The guard test

BackendArchitectureTests.ProductionCatchBlocks_DoNotRelyOnDebugWriteLineAlone (tests/Features/Architecture/BackendArchitectureTests.cs:1444) scans listenarr.domain, listenarr.application, listenarr.infrastructure and listenarr.api and fails on any catch whose body, with comments stripped, is only a Debug.WriteLine call, naming each as file:line.

I want to be plain about what this does going forward: once merged, it will fail any future PR, including PRs already open, that adds a new catch block shaped this way. I checked what that means today, across the other open PRs on the repository. 93 of them still merge cleanly into canary on their own (the rest are already stale against canary for unrelated reasons and would need a rebase regardless of this bundle). None of those 93 add a new Debug.WriteLine-only catch (measured: git diff <merge-base> <head> | grep -c '^+.*Debug\.WriteLine' is zero for each). One of the stale ones, #760, does add a new such catch in StartupDbNormalizer.cs; since #760 does not merge into canary as it stands, whoever rebases it would need to pick a different pattern there regardless of this guard test, but I think it is worth flagging in case the shape survives a rebase.

What the test does not catch, which matters for anyone relying on it: a Debug.WriteLine call whose arguments nest parentheses more than one level deep (the pattern only balances one level), Debug.Write or Debug.Print instead of Debug.WriteLine (only that exact method name is matched, and both carry the same [Conditional("DEBUG")]), and a ) inside a string literal passed to the call (the pattern is not string-aware, so it reads that as a closing paren and the match comes apart). It also matches catch-body braces textually rather than parsing C#, so I would expect, though have not tested, that a { or } inside a string literal in a catch body could confuse that half of it too. An empty catch and a catch holding only a comment, which is how this fix marks its 41 deliberate sites, are both intentionally left alone; so is a real logger call that drops the exception, the way QbittorrentConnectionTester.cs did before this fix.

Full write-up in the issue thread on #928, including the four PRs the original branch conflicted with there (#657, #728, #760 and #785). All four currently fail to merge into canary at all on their own, independent of this bundle, so that conflict has not gone away; whoever rebases any of them would need to resolve it first.

dcd9c37: integration fix for two gaps in SanitizeWebhookUrl

The webhook-URL fix above has two gaps, each covered by a new regression test:

• SanitizeWebhookUrl's Discord case matched discord.com but not the legacy discordapp.com domain, which Discord still accepts; a webhook URL saved before the rename to discord.com would still log its id and token intact.
• Pushbullet's legacy pushbullet://TOKEN format puts the access token where a URI's host would be. SanitizeWebhookUrl kept the host as-is for anything that was not Telegram, Discord or Slack, so the token would survive in the authority string it builds for every other provider. This one is defensive rather than a live leak: NotificationService.Webhooks.cs:23 validates every webhook URL through NotificationDiagnostics.TryValidateWebhookTarget, which calls OutboundRequestSecurity.TryValidateExternalHttpUrl (OutboundRequestSecurity.cs:48-53), and that check allows only http and https. A pushbullet:// URL is rejected and logged as a blocked target before any webhook-specific code, including SanitizeWebhookUrl, ever sees it. The test compares case-insensitively because Uri lowercases the host.

Both are fixed in SanitizeWebhookUrl itself: the Discord check now also matches discordapp.com and its subdomains, and a pushbullet scheme now returns pushbullet://<redacted> instead of keeping the host.

I left dispatch routing alone. NotificationService.Webhooks.cs:30 only matches discord.com/api/webhooks when deciding whether to send through the Discord-specific path, so a discordapp.com webhook still falls through to the generic sender rather than the Discord path. That is a separate behavior change from a logging fix, so I am stating it here as a known limitation rather than folding it into this bundle.

New tests in LogRedactionTests: SanitizeWebhookUrl_DiscordappLegacyDomain_MasksIdAndTokenSegments and SanitizeWebhookUrl_PushbulletUriSchemeToken_DoesNotLeakTokenAsHost. Read: both assertions fail against 6d3ef5c, the webhook-URL commit without this fix, since neither gap in SanitizeWebhookUrl is closed there yet; running them did not leave a saved log, so I am grading this read rather than measured.

One wording choice from the 78-site commit worth noting: several of the 22 "catch wrapped around a log call" comments read "the call that failed is the logging call itself," which is accurate but reads a little awkwardly. Rewording all of them (11 files) felt like a disproportionate change for a phrasing preference, so I left those comments as they are; happy to send a small follow-up commit if you would rather have it reworded.

Merge order and dependency

The first two commits apply to canary independently of each other; git merge-tree --merge-base=<parent> upstream/canary <commit> exits 0 for both. The integration-fix commit does not stand alone the same way: it depends on the webhook-URL commit (6d3ef5c), since it adds to a method that commit introduces. It does not depend on the silent-catch-blocks commit; cherry-picking it onto canary plus only the webhook-URL commit also succeeds. So the real ordering constraint is 6d3ef5c before dcd9c37; 18dbbbb can go anywhere relative to both.

Default changes

Neither fix changes a default, and neither makes a setting take effect where it was previously ignored. All three commits only change what gets written to a log line or, in the integration fix, which URLs get redacted the same way; no return value, no config read, and no conditional path outside logging changes. The one user-visible effect worth naming plainly: a discordapp.com webhook URL that previously had its credential logged in full now has it redacted, the same as every other provider already got from #987; dispatch behavior for it is unchanged, so it still falls through to the generic sender rather than the Discord-specific path. The pushbullet://TOKEN half of the integration fix is defensive, not a response to a live leak: that format is already rejected before dispatch today (see above), so no running instance logs that token under either the old or new behavior.

Overlap with other open PRs

I checked every other contributor's open PR that still merges into canary on its own: 93 of them, of which 18 share at least one file with this bundle. All 18 still merge cleanly with this bundle's head, dcd9c3787, which sits on canary a630572e9. This excludes my own open PRs; #943 is one of mine and, besides sharing NotificationService.Webhooks.cs as noted above, also touches DownloadService.cs, the file the next bullet covers.

• fix(search): reject automatic-search results whose title is irrelevant to the audiobook #591, fix(wanted): recover books with no quality profile — picker, explicit bulk-assign, failures named #605 and fix(search): result filters never ran on auto-pick paths; add title-relevance gate #744 (kevinheneveld), Add Deluge download client support #740 (blackax1), Retry automatic search with title only when title and author returns nothing #825 (matheusfillipe), feat(notifications): fire lifecycle triggers across the book-acquisition pipeline #989 (dny238) and fix(wanted): scope search all to filtered audiobooks and add confirmation modal #1009 (noy-solvin) all touch DownloadService.cs; this bundle only changes a catch block there.
• fix(search): fall back to keyword search when author doesn't resolve to an Audible author page #750 (kevinheneveld) touches AudibleAuthorSearchWorkflow.cs.
• Fix Discord/NTFY notifications double-posting via generic sender fall-through #754 (dny238) touches NotificationService.Webhooks.cs, covered above.
• fix(downloads): SABnzbd category filter hid tracked jobs reassigned to Default, stranding them unimported #759, Fix SABnzbd downloads completing with no path set (race between active queue and history) #840 (t3chnaztea, lisim) touch SabnzbdResponseMapper.cs.
• fix(qbittorrent): add URL Base support for reverse-proxied instances #772, fix(qbittorrent): report IP bans and stop retrying rejected logins #1007 (schmitzkr, rghvgrv) touch QbittorrentConnectionTester.cs.
• Add freeleech wedge support for MyAnonamouse #803 (sloan134) touches MyAnonamouseDownloadUrlBuilder.cs.
• Fix optional token elision in naming patterns #850 (krejko) touches FileNamingService.cs.
• Make the OpenLibrary fallback reachable #861 (krejko) touches SearchService.Intelligent.cs.
• feat(search): search by narrator #886 (krejko) touches StructuredSearchWorkflow.cs and AudibleService.cs.
• Fix weak-storage imports and recovery #993 (therobbiedavis) shares only BackendArchitectureTests.cs.

Your own #579, #611 and #841 do not merge into a630572 themselves as of this writing, so they are not part of this comparison; #993 is the one of yours that still does.

Tests

Backend, measured: Passed 3129, Failed 0, Skipped 130, Total 3259. Canary baseline: Passed 3118, Failed 0, Skipped 130, Total 3248. The 11 extra passing tests are the ones this bundle adds (5 SanitizeWebhookUrl tests from #987, 3 integration redaction tests, 1 architecture guard test, 2 more SanitizeWebhookUrl tests from the integration fix). The run was on a commit whose tree is identical to this head's (git rev-parse of both trees gives the same hash), so it applies here unchanged.

Frontend, measured: vue-tsc clean, vitest 89 files / 607 tests passed, same as the canary baseline; this bundle does not touch the frontend.

If it would be easier to review these as separate PRs instead, I am glad to split them back out.

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

[therobbiedavis]

@m4bard Reviewing this one for the next release. I'll keep you updated of any findings. Thanks!

[therobbiedavis]

@m4bard Replacing RedactText drops its existing masking of known secrets from the environment.

For a generic URL like https://example.com/hooks/<token>, where the token is also stored in TOKEN, canary masks it but this PR writes it into the Information-level log. I verified that against the rebased head.

Could we retain that existing masking alongside the new sanitizer and add a regression test for this case? The generic path can still be preserved while masking any known secret it contains.

[m4bard]

I think both asks are covered now. The branch head is 22b9b7d, which is 254e2e3 (the masking and the regression tests) plus one test-only commit.

Masking: SanitizeWebhookUrl still builds the structural sanitized URL, and it now also masks the known environment secrets (the same set RedactText uses, case-insensitive) as <redacted>. I did not call RedactText from inside it, because as far as I can tell it appends a trailing <redacted> whenever any secret is set but none matched, and that would corrupt the sanitized URL. The generic path is kept and only the secret segment is masked. The change is inside SanitizeWebhookUrl, so I think it also covers the other call sites that moved from RedactText to the sanitizer.

Regression test: SecurityRedactionTests.NotificationService_GenericWebhookLog_MasksPathTokenThatIsAlsoAnEnvironmentSecret sets TOKEN to the path token of a generic https://example.com/hooks/<token> URL and checks the "Sending Generic POST" log line. There is a helper-level twin, LogRedactionTests.SanitizeWebhookUrl_GenericUrl_MasksPathTokenThatIsAlsoAnEnvironmentSecret. With LogRedaction.cs taken from 27feab3, both fail; at head both pass. The full suite at head is 3706 passed, 0 failed, 150 skipped.

One extra: those two tests set TOKEN in the process environment and run together without it one of them failed on every run (10 of 10), so 22b9b7d puts both redaction test classes in a non-parallel xunit collection. With it, the pair passed 20 of 20 runs.

Gap: the masking runs after Uri canonicalization. I measured that an environment secret containing a backslash, %7E, a dot segment or a ?, and also stored in a webhook path, is not fully masked at head, and neither is a secret with a space that appears as %20 in the URL. As far as I can tell from reading canary's RedactText, it replaces on the raw string, so it would mask the first four shapes. I did not run canary on them. All of these need an unusual secret, which is why I think the gap is narrow, but I may be wrong about that. Does this match what you would expect? If you want it closed, I can do that in a follow-up PR.

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

[therobbiedavis]

Requesting changes for the existing inline finding: please retain the masking of known environment secrets alongside the new webhook URL sanitizer and add a regression test for a generic URL containing one of those secrets.