fix(configuration-database): resolve ConfigurationDatabase-002..007 — remove hardcoded sa creds, fail-fast no-arg DI, encrypt secret columns, resilient audit serialization

code-reviews/ConfigurationDatabase/findings.md

@@ -8,7 +8,7 @@
 | Last reviewed | 2026-05-16 |
 | Reviewer | claude-agent |
 | Commit reviewed | `9c60592` |
-| Open findings | 10 |
+| Open findings | 6 |
 
 ## Summary
 
@@ -113,7 +113,7 @@ template-aggregate contract the callers depend on.
 |--|--|
 | Severity | Medium |
 | Category | Security |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.ConfigurationDatabase/DesignTimeDbContextFactory.cs:21-22` |
 
 **Description**
@@ -136,7 +136,19 @@ and never use `sa`.
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-16 (commit pending). Root cause confirmed against source: the factory
+fell back to a literal `User Id=sa;Password=YourPassword;...` connection string when no
+configured value was found. Removed the hardcoded fallback entirely. The factory now
+resolves the connection string from the Host's appsettings files or, when those are not
+present, from the `SCADALINK_DESIGNTIME_CONNECTIONSTRING` environment variable, and
+throws a clear `InvalidOperationException` (naming both the config key and the env var)
+when neither yields a value. Also hardened `SetBasePath` to be applied only when the
+`ScadaLink.Host` directory exists, so the factory degrades cleanly instead of throwing
+`DirectoryNotFoundException` when run from a context without a sibling Host folder.
+Regression tests added in `DesignTimeDbContextFactoryTests.cs`:
+`CreateDbContext_NoConnectionStringConfigured_ThrowsClearException`,
+`CreateDbContext_ConnectionStringFromEnvironmentVariable_IsUsed`, and
+`DesignTimeDbContextFactory_SourceContainsNoHardcodedSaCredential`.
 
 ### ConfigurationDatabase-003 — No-arg `AddConfigurationDatabase()` silently registers nothing
 
@@ -144,7 +156,7 @@ _Unresolved._
 |--|--|
 | Severity | Medium |
 | Category | Error handling & resilience |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.ConfigurationDatabase/ServiceCollectionExtensions.cs:44-49` |
 
 **Description**
@@ -166,7 +178,21 @@ name (e.g. `AddConfigurationDatabaseNoOp()`), and remove the stale "Phase 0" wor
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-16 (commit pending). Root cause confirmed against source: the
+parameterless `AddConfigurationDatabase()` overload returned `services` unchanged,
+registering no `DbContext`, repositories, `IAuditService`, or `IInstanceLocator`.
+Applied the recommendation's first option: the overload is now marked
+`[Obsolete(..., error: true)]` so any source reference is a compile-time failure, and
+its body throws `InvalidOperationException` with an actionable message as
+defence-in-depth (covering reflection-based invocation or suppressed warnings). The
+stale "Phase 0 stubs / backward compatibility" XML comment was replaced with one
+explaining the obsoletion. The pre-existing
+`ServiceRegistrationTests.AddConfigurationDatabase_NoArgs_DoesNotThrow` test in
+`UnitTest1.cs`, which encoded the old buggy no-op contract, was updated to
+`AddConfigurationDatabase_NoArgs_FailsFast` to assert the corrected behaviour.
+New regression tests added in `ServiceCollectionExtensionsTests.cs`:
+`AddConfigurationDatabase_NoArgOverload_FailsFastWithClearMessage` and
+`AddConfigurationDatabase_NoArgOverload_IsMarkedObsoleteAsError`.
 
 ### ConfigurationDatabase-004 — Secret-bearing columns stored in plaintext with no protection
 
@@ -174,7 +200,7 @@ _Unresolved._
 |--|--|
 | Severity | Medium |
 | Category | Security |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.ConfigurationDatabase/Configurations/NotificationConfiguration.cs:56-57`, `src/ScadaLink.ConfigurationDatabase/Configurations/ExternalSystemConfiguration.cs:25-26,75-77` |
 
 **Description**
@@ -199,7 +225,35 @@ doc to state the chosen at-rest protection.
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-16 (commit pending). Root cause confirmed against source:
+`SmtpConfiguration.Credentials`, `ExternalSystemDefinition.AuthConfiguration`, and
+`DatabaseConnectionDefinition.ConnectionString` were mapped as ordinary `nvarchar(4000)`
+columns and persisted verbatim.
+
+Implemented the recommendation's first option — an in-module EF Core value converter
+backed by ASP.NET Data Protection, which the module already uses
+(`IDataProtectionKeyContext`, `AddDataProtection().PersistKeysToDbContext`). Added
+`EncryptedStringConverter` (purpose-scoped `IDataProtector`; `Protect` on write,
+`Unprotect` on read; null-safe; surfaces a clear message on a `CryptographicException`).
+`ScadaLinkDbContext` gained an `(options, IDataProtectionProvider)` constructor and
+applies the converter to the three secret columns in `OnModelCreating`; the DI
+registration in `ServiceCollectionExtensions` now constructs the context with the
+registered provider. The secret columns were widened to `HasMaxLength(8000)` (EF maps
+this to `nvarchar(max)` on SQL Server) so ciphertext expansion cannot truncate the
+value; migration `20260517010521_EncryptSecretColumns` carries the column-type change.
+Regression tests added in `SecretEncryptionTests.cs` verify the raw column value is
+never the plaintext secret and that EF transparently decrypts on read, for all three
+columns plus a null round-trip.
+
+The encryption scheme itself is fully in-module; the only remaining cross-cutting item
+is a documentation gap — the design doc does not yet state encryption-at-rest for these
+fields. That doc update is outside this module's editable scope (constraint: edit only
+`src/ScadaLink.ConfigurationDatabase`, the tests, and this file) and is surfaced here
+for a follow-up to `docs/requirements/Component-ConfigurationDatabase.md`. The audit
+secret-leak concern is mitigated separately by CD-007's serializer hardening; whether
+callers should additionally redact secret-bearing entities before passing them to
+`IAuditService` is a caller-side concern in other modules and is also surfaced for
+follow-up. The code fix in this module is complete.
 
 ### ConfigurationDatabase-005 — Audit `Id` type disagrees with the design doc
 
@@ -264,7 +318,7 @@ _Unresolved._
 |--|--|
 | Severity | Medium |
 | Category | Error handling & resilience |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.ConfigurationDatabase/Services/AuditService.cs:28-30` |
 
 **Description**
@@ -289,7 +343,23 @@ and document that decision against the design doc's transactional-guarantee sect
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-16 (commit pending). Root cause confirmed against source: `LogAsync`
+called `JsonSerializer.Serialize(afterState)` with default options, so any `afterState`
+graph containing a reference cycle threw `JsonException` — and because the audit entry
+commits in the same transaction as the change it records, that exception rolled back
+the entire business operation.
+
+Fix applied per the recommendation: `AuditService` now serializes via a static
+`JsonSerializerOptions` configured with `ReferenceHandler.IgnoreCycles` and
+`MaxDepth = 32`. The serialization is additionally wrapped in a `SerializeAfterState`
+helper that catches a residual `JsonException`/`NotSupportedException` and substitutes a
+small diagnostic placeholder JSON (`AuditSerializationError` + `StateType`) — an explicit
+decision that an audit-serialization failure must **degrade gracefully** and never roll
+back the audited operation. The audit entry is always recorded; the design doc's
+transactional-guarantee section ("if the change succeeds, the audit entry is always
+recorded") is thereby honoured even for pathological state objects. Regression test
+added in `AuditServiceTests.cs`:
+`LogAsync_AfterStateWithReferenceCycle_DoesNotThrow_AndDoesNotRollBackOperation`.
 
 ### ConfigurationDatabase-008 — `GetApprovedKeysForMethodAsync` CSV parsing silently drops malformed ids