fix(notification-service): resolve NotificationService-005..009 — explicit TLS modes, per-credential token cache, timeout/throttle, address validation, credential redaction

code-reviews/NotificationService/findings.md

@@ -8,7 +8,7 @@
 | Last reviewed | 2026-05-16 |
 | Reviewer | claude-agent |
 | Commit reviewed | `9c60592` |
-| Open findings | 8 |
+| Open findings | 3 |
 
 ## Summary
 
@@ -172,7 +172,7 @@ the resulting client is disposed.
 |--|--|
 | Severity | Medium |
 | Category | Correctness & logic bugs |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.NotificationService/MailKitSmtpClientWrapper.cs:18`, `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:123` |
 
 **Description**
@@ -185,7 +185,16 @@ Pass the `TlsMode` string (or a `TlsMode` enum) through to the wrapper and map e
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-16 (commit pending). Root cause confirmed against source: the
+`bool useTls` parameter cannot represent three states, and the non-StartTLS branch used
+`SecureSocketOptions.Auto`. A new `SmtpTlsMode` enum (`None`/`StartTls`/`Ssl`) and
+`SmtpTlsModeParser` were added; `ISmtpClientWrapper.ConnectAsync` now takes `SmtpTlsMode`
+and `MailKitSmtpClientWrapper` maps it explicitly to `SecureSocketOptions.None`/
+`StartTls`/`SslOnConnect`. `SendAsync`/`DeliverBufferedAsync` validate the configured
+`TlsMode` up front — an unknown value returns a clean `NotificationResult` failure (or
+parks a buffered message) instead of silently negotiating TLS. Regression tests:
+`Send_TlsModeNone_DoesNotNegotiateTls`, `Send_TlsModeSsl_UsesImplicitSsl`,
+`Send_UnknownTlsMode_ReturnsErrorNotSilentFallback`, and the `SmtpTlsModeParserTests` set.
 
 ### NotificationService-006 — OAuth2 token cache is keyed to nothing; wrong token returned when multiple SMTP configs exist
 
@@ -193,7 +202,7 @@ _Unresolved._
 |--|--|
 | Severity | Medium |
 | Category | Concurrency & thread safety |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.NotificationService/OAuth2TokenService.cs:14-15`, `src/ScadaLink.NotificationService/OAuth2TokenService.cs:30-35` |
 
 **Description**
@@ -206,7 +215,14 @@ Key the cache by the credential identity (e.g. a dictionary keyed by `tenantId:c
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-16 (commit pending). Root cause confirmed: the singleton held a single
+`_cachedToken`/`_tokenExpiry` pair and `GetTokenAsync` ignored the `credentials` argument
+when validating the cache, so a second SMTP config got the first config's token.
+`OAuth2TokenService` now stores a `ConcurrentDictionary<string, CacheEntry>` keyed by the
+SHA-256 hash of the credential string; each distinct tenant/client/secret gets its own
+cached token, expiry, and per-credential `SemaphoreSlim` (double-checked locking
+preserved). Regression tests: `GetTokenAsync_DifferentCredentials_ReturnPerCredentialTokens`
+and `GetTokenAsync_SameCredentials_CachedPerCredential`.
 
 ### NotificationService-007 — Connection timeout and max-concurrent-connections from the design doc are not implemented
 
@@ -214,7 +230,7 @@ _Unresolved._
 |--|--|
 | Severity | Medium |
 | Category | Design-document adherence |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.NotificationService/NotificationOptions.cs:11-14`, `src/ScadaLink.NotificationService/MailKitSmtpClientWrapper.cs:16-20`, `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:111-140` |
 
 **Description**
@@ -227,7 +243,17 @@ Set `SmtpClient.Timeout` from `ConnectionTimeoutSeconds` in `ConnectAsync` (and/
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-16 (commit pending). Root cause confirmed: `ConnectAsync` never set
+`SmtpClient.Timeout` and no throttle gated `DeliverAsync`. `ISmtpClientWrapper.ConnectAsync`
+now takes a `connectionTimeoutSeconds` argument; `MailKitSmtpClientWrapper` sets
+`SmtpClient.Timeout` from `SmtpConfiguration.ConnectionTimeoutSeconds`. `DeliverAsync`
+acquires a lazily-created `SemaphoreSlim` sized to `SmtpConfiguration.MaxConcurrentConnections`
+(default 5 when non-positive) and releases it in a `finally`, so concurrent SMTP
+deliveries per site are bounded. The timeout is sourced from the deployed
+`SmtpConfiguration` rather than `NotificationOptions`; the `NotificationOptions` fields
+remain as operational fallback defaults. Regression tests:
+`Send_PassesConfiguredConnectionTimeoutToClient` and
+`Send_MaxConcurrentConnections_LimitsConcurrentDeliveries`.
 
 ### NotificationService-008 — Recipient email addresses are not validated before send
 
@@ -235,7 +261,7 @@ _Unresolved._
 |--|--|
 | Severity | Medium |
 | Category | Correctness & logic bugs |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:136-137`, `src/ScadaLink.NotificationService/MailKitSmtpClientWrapper.cs:50-53` |
 
 **Description**
@@ -248,15 +274,24 @@ Validate addresses up front (e.g. `MailboxAddress.TryParse`) and return a `Notif
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-16 (commit pending). Root cause confirmed: `MailboxAddress.Parse` of a
+malformed `FromAddress`/recipient threw `ParseException`, which is unclassified and
+escaped `SendAsync` as an unhandled exception. A new `ValidateAddresses` helper uses
+`MailboxAddress.TryParse` for the sender and every recipient; `SendAsync` now returns a
+clean `NotificationResult(false, ...)` listing the invalid address(es) before any SMTP
+attempt, and `DeliverBufferedAsync` parks a buffered message with a bad address (a fault
+retrying cannot fix). Regression tests:
+`Send_MalformedRecipientAddress_ReturnsCleanError_DoesNotThrow` and
+`Send_MalformedFromAddress_ReturnsCleanError_DoesNotThrow`. Definition-time validation in
+the Central UI is a separate component and out of this module's scope.
 
 ### NotificationService-009 — Credentials handled as plaintext strings; OAuth2 client secret logged risk
 
 | | |
 |--|--|
-| Severity | Medium |
+| Severity | Medium — re-triaged: split into an in-scope log-leak fix (resolved) and a Commons-scoped at-rest-encryption / structured-credential follow-up (NotificationService-013, Deferred). |
 | Category | Security |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:127-134`, `src/ScadaLink.NotificationService/OAuth2TokenService.cs:30-65`, `src/ScadaLink.Commons/Entities/Notifications/SmtpConfiguration.cs:9` |
 
 **Description**
@@ -269,7 +304,55 @@ Store credentials encrypted at rest (DPAPI/Data Protection or a secret store) an
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-16 (commit pending). Root cause re-triaged against source: the finding
+conflates two concerns with different ownership.
+
+1. **Log-leak risk (in scope — fixed).** The original code logged whole exception objects
+   (`_logger.LogWarning(ex, ...)` / `LogError(ex, ...)`); MailKit auth exceptions can echo
+   server responses quoting the supplied credentials. A new internal `CredentialRedactor`
+   masks every colon-delimited credential component out of any text. `SendAsync` and
+   `DeliverBufferedAsync` now log a scrubbed message string (not the raw exception) and the
+   permanent-failure `NotificationResult` is scrubbed before it returns to the caller.
+   `OAuth2TokenService` logs the tenant id only — never the client secret or access token.
+   Regression tests: `CredentialRedactorTests` and
+   `Send_PermanentError_RedactsCredentialFromResultMessage`.
+2. **At-rest encryption + structured-credential modelling (out of scope — Deferred).**
+   Encrypting `SmtpConfiguration.Credentials` at rest and replacing the brittle
+   colon-packed `string` with structured fields requires editing
+   `src/ScadaLink.Commons/Entities/Notifications/SmtpConfiguration.cs` and the
+   ConfigurationDatabase EF layer — both outside this module. Tracked separately as
+   **NotificationService-013** (Deferred) so it is not lost.
+
+### NotificationService-013 — Encrypt SMTP credentials at rest; replace colon-packed string with structured fields
+
+| | |
+|--|--|
+| Severity | Medium |
+| Category | Security |
+| Status | Deferred |
+| Location | `src/ScadaLink.Commons/Entities/Notifications/SmtpConfiguration.cs:9`, ConfigurationDatabase EF mapping |
+
+**Description**
+
+Split out of NotificationService-009. `SmtpConfiguration.Credentials` packs Basic Auth
+`user:pass` and OAuth2 `tenantId:clientId:clientSecret` into a single plaintext
+colon-delimited `string`: (a) there is no encryption at rest in SQLite or the central
+config DB; (b) a password or client secret containing a `:` is split incorrectly by
+`Split(':', 2)` / `Split(':', 3)`, silently corrupting the secret.
+
+**Recommendation**
+
+Model credentials as structured fields (or an encrypted blob) on the Commons entity and
+encrypt at rest via Data Protection / a secret store. The colon-delimited parsing in
+`MailKitSmtpClientWrapper` and `OAuth2TokenService` would then consume the structured
+fields directly.
+
+**Resolution**
+
+Deferred — requires changes to `src/ScadaLink.Commons` and the ConfigurationDatabase
+component, which are outside the NotificationService module. To be addressed in a
+Commons/ConfigurationDatabase-scoped change. The associated log-leak risk is resolved
+under NotificationService-009.
 
 ### NotificationService-010 — `DeliverAsync` does not disconnect the SMTP client on failure