diff --git a/code-reviews/Configuration/findings.md b/code-reviews/Configuration/findings.md index 9159ebb..b62a1bb 100644 --- a/code-reviews/Configuration/findings.md +++ b/code-reviews/Configuration/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-22 | | Commit reviewed | `76d35d1` | | Status | Reviewed | -| Open findings | 5 | +| Open findings | 0 | ## Checklist coverage @@ -78,13 +78,13 @@ | Severity | Low | | Category | OtOpcUa conventions | | Location | `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Enums/NodePermissions.cs:8`, `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/OtOpcUaConfigDbContext.cs:417` | -| Status | Open | +| Status | Resolved | **Description:** `NodePermissions` is declared `[Flags] enum ... : uint`, while its XML doc and `NodeAcl.PermissionFlags`' doc both say "stored as int", and `ConfigureNodeAcl` uses `HasConversion()` — a `uint`→`int` conversion. Only bits 0–11 are used today, but the underlying-type/storage-type mismatch is a latent trap: a future bit-31 flag yields a `uint` value that overflows `int` and the conversion round-trip would corrupt it. **Recommendation:** Change the enum underlying type to `int` (consistent with the docs and the conversion). No high bit is in use, so this is the smaller change. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — changed `NodePermissions` underlying type from `uint` to `int` so it matches the documented "stored as int" semantics and the `HasConversion()` mapping in `OtOpcUaConfigDbContext.ConfigureNodeAcl`. Added regression test `NodePermissionsTests` pinning the underlying type and round-trip safety through `int` storage. ### Configuration-005 @@ -93,13 +93,13 @@ | Severity | Low | | Category | Concurrency & thread safety | | Location | `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/LocalCache/LiteDbConfigCache.cs:50` | -| Status | Open | +| Status | Resolved | **Description:** `PutAsync` performs a non-atomic find-then-insert/update. Two concurrent `PutAsync` calls for the same `(ClusterId, GenerationId)` can both observe `existing is null` and both `Insert`, producing two rows for one generation. The constructor's `EnsureIndex` calls are non-unique, so the storage layer does not prevent the duplicate, and `PruneOldGenerationsAsync`'s `keepLatest` accounting is then off. **Recommendation:** Declare a unique index on `(ClusterId, GenerationId)` and treat the duplicate-key exception as an idempotent no-op, or guard `PutAsync` with an instance `SemaphoreSlim`/lock. Document the concurrency contract on `ILocalConfigCache`. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — guarded `PutAsync` with an instance-level `SemaphoreSlim` so the find-then-insert/update window runs atomically for a given cache instance; documented the concurrency contract on `ILocalConfigCache`. Added regression test `PutAsync_concurrent_for_same_cluster_and_generation_does_not_duplicate` that runs 64 concurrent puts and inspects the LiteDB file directly to confirm exactly one row per `(ClusterId, GenerationId)` survives. ### Configuration-006 @@ -123,13 +123,13 @@ | Severity | Low | | Category | Error handling & resilience | | Location | `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Apply/GenerationApplier.cs:44` | -| Status | Open | +| Status | Resolved | **Description:** `ApplyPass` wraps each callback in `catch (Exception ex)`. This swallows `OperationCanceledException` — a cancellation during a callback is recorded as just another entity error string and the applier keeps walking the remaining passes instead of stopping. It also masks fatal exceptions. The applier continues applying Added/Modified passes even after a Removed callback failed, leaving a partially-applied runtime state. **Recommendation:** Rethrow `OperationCanceledException` rather than recording it as an entity error; call `ct.ThrowIfCancellationRequested()` between passes. Document or enforce whether a failed Removed pass should abort before the Added/Modified passes run. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — added `catch (OperationCanceledException) when (ct.IsCancellationRequested) { throw; }` ahead of the generic catch in `ApplyPass` so genuine caller cancellation propagates rather than being recorded as an entity error, and added a `ct.ThrowIfCancellationRequested()` at the top of each Added/Modified pass iteration. The "failed Removed pass keeps walking Added/Modified" behaviour was confirmed as the intended contract (cascades must settle) and pinned by `Apply_continues_to_Added_pass_when_a_Removed_callback_throws`. New regression tests: `Apply_propagates_OperationCanceledException_from_callback_when_token_cancelled`, `Apply_stops_between_passes_when_cancellation_requested`. ### Configuration-008 @@ -168,13 +168,13 @@ | Severity | Low | | Category | Security | | Location | `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/LocalCache/ResilientConfigReader.cs:81` | -| Status | Open | +| Status | Resolved | **Description:** On central-DB read failure the warning log records the full exception object. Callers pass arbitrary `centralFetch` delegates; if any delegate closes over a connection string, an exception thrown from it (or a `SqlException` carrying server/credential context) is logged verbatim. There is no scrubbing of connection-string fragments before logging, against the project's no-secret-logging rule. **Recommendation:** Log `ex.GetType().Name` and `ex.Message` for SQL failures rather than the full exception, or run exception messages through a connection-string scrubber before they reach the log sink. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — stopped passing the raw exception object to `LogWarning`; the fallback log now records only `ex.GetType().Name` and a `ScrubSecrets`-redacted `ex.Message` so connection-string fragments (Password, User Id, Pwd, Uid, AccessToken, Authorization, ApiKey/Api-Key) are stripped before reaching any sink. Added regression test `FallbackWarning_does_not_log_full_exception_object_or_password_fragment` that captures emitted log records and asserts no raw exception attached and no credential keyword present in the rendered message. ### Configuration-011 @@ -183,10 +183,10 @@ | Severity | Low | | Category | Testing coverage | | Location | `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Apply/GenerationApplier.cs:7`, `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/DraftValidator.cs:60` | -| Status | Open | +| Status | Resolved | **Description:** The companion test project covers the cache, schema compliance, stored procedures, and `DraftValidator` well, but two flagged behaviours are not pinned: (a) `GenerationApplier` ordering/cancellation when a Removed callback fails — no test asserts the Added/Modified passes still run or that cancellation aborts; (b) `ValidatePathLength`'s constant 32+32 approximation — no test exercises a long Enterprise/Site. The publish-bypasses-validation bug (Configuration-001) is also untested against the live SQL fixture. **Recommendation:** Add `GenerationApplierTests` cases for a throwing callback (assert error recorded, assert cancellation propagates) and a `DraftValidatorTests` path-length boundary case. Add a `StoredProceduresTests` case that publishes an invalid draft and asserts it stays `Draft`. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — all three gaps now covered. (a) `GenerationApplierTests.Apply_continues_to_Added_pass_when_a_Removed_callback_throws` pins ordering; `Apply_propagates_OperationCanceledException_from_callback_when_token_cancelled` and `Apply_stops_between_passes_when_cancellation_requested` (added under Configuration-007) pin cancellation. (b) `DraftValidatorTests.PathLength_uses_actual_Enterprise_Site_when_provided` and `PathLength_conservative_fallback_when_Enterprise_Site_absent` (added under Configuration-003) pin the path-length boundary. (c) `StoredProceduresTests.Publish_aborts_when_ValidateDraft_rejects_the_draft` (added under Configuration-001) pins the publish-bypasses-validation regression against the live SQL fixture. diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Apply/GenerationApplier.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Apply/GenerationApplier.cs index 8e0e000..982b026 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Apply/GenerationApplier.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Apply/GenerationApplier.cs @@ -19,6 +19,10 @@ public sealed class GenerationApplier(ApplyCallbacks callbacks) : IGenerationApp foreach (var kind in new[] { ChangeKind.Added, ChangeKind.Modified }) { + // Honour cancellation between passes — a caller can abort the apply between Removed + // and Added phases even if individual callbacks don't observe the token themselves + // (Configuration-007). + ct.ThrowIfCancellationRequested(); await ApplyPass(diff.Namespaces, kind, callbacks.OnNamespace, errors, ct); await ApplyPass(diff.Drivers, kind, callbacks.OnDriver, errors, ct); await ApplyPass(diff.Devices, kind, callbacks.OnDevice, errors, ct); @@ -42,6 +46,12 @@ public sealed class GenerationApplier(ApplyCallbacks callbacks) : IGenerationApp foreach (var change in changes.Where(c => c.Kind == kind)) { try { await callback(change, ct); } + // Configuration-007: cancellation must propagate, not be silently recorded as an + // entity error. Distinguish caller cancellation (token signalled) from any + // OperationCanceledException raised independently of the caller's token, which we + // still want to surface as an entity error so a single misbehaving callback does + // not crash the entire apply. + catch (OperationCanceledException) when (ct.IsCancellationRequested) { throw; } catch (Exception ex) { errors.Add($"{typeof(T).Name} {change.Kind} '{change.LogicalId}': {ex.Message}"); } } } diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Enums/NodePermissions.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Enums/NodePermissions.cs index aee9777..44a3062 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Enums/NodePermissions.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Enums/NodePermissions.cs @@ -5,7 +5,7 @@ namespace ZB.MOM.WW.OtOpcUa.Configuration.Enums; /// Stored as int bitmask in . /// [Flags] -public enum NodePermissions : uint +public enum NodePermissions : int { None = 0, diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/LocalCache/ILocalConfigCache.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/LocalCache/ILocalConfigCache.cs index 6c44f60..a4cca53 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/LocalCache/ILocalConfigCache.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/LocalCache/ILocalConfigCache.cs @@ -4,6 +4,13 @@ namespace ZB.MOM.WW.OtOpcUa.Configuration.LocalCache; /// Per-node local cache of the most-recently-applied generation(s). Used to bootstrap the /// address space when the central DB is unreachable (decision #79 — degraded-but-running). /// +/// +/// Concurrency contract: implementations must serialize writes — specifically, +/// for the same (ClusterId, GenerationId) from concurrent +/// callers must not produce duplicate rows. Reads may run concurrently with reads and writes. +/// The implementation enforces this via an instance-level +/// around the find-then-insert/update window. +/// public interface ILocalConfigCache { Task GetMostRecentAsync(string clusterId, CancellationToken ct = default); diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/LocalCache/LiteDbConfigCache.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/LocalCache/LiteDbConfigCache.cs index eca7a73..8c04b2d 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/LocalCache/LiteDbConfigCache.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/LocalCache/LiteDbConfigCache.cs @@ -13,6 +13,12 @@ public sealed class LiteDbConfigCache : ILocalConfigCache, IDisposable private const string CollectionName = "generations"; private readonly LiteDatabase _db; private readonly ILiteCollection _col; + // PutAsync is a find-then-insert/update; without serialization, two concurrent puts for the + // same (ClusterId, GenerationId) can both observe `existing is null` and both Insert, + // producing duplicate rows (Configuration-005). Serialize writes through this semaphore so + // the read-modify-write block is atomic for a given instance. LiteDB itself only locks the + // page-level write, not the find-then-insert window. + private readonly SemaphoreSlim _writeGate = new(initialCount: 1, maxCount: 1); public LiteDbConfigCache(string dbPath) { @@ -47,23 +53,32 @@ public sealed class LiteDbConfigCache : ILocalConfigCache, IDisposable return Task.FromResult(snapshot); } - public Task PutAsync(GenerationSnapshot snapshot, CancellationToken ct = default) + public async Task PutAsync(GenerationSnapshot snapshot, CancellationToken ct = default) { ct.ThrowIfCancellationRequested(); - // upsert by (ClusterId, GenerationId) — replace in place if already cached - var existing = _col - .Find(s => s.ClusterId == snapshot.ClusterId && s.GenerationId == snapshot.GenerationId) - .FirstOrDefault(); - - if (existing is null) - _col.Insert(snapshot); - else + // Serialize the find-then-insert/update so concurrent callers do not observe a stale + // `existing is null` and both Insert (Configuration-005). LiteDB's per-call lock is + // not enough — the read and the write are independent calls. + await _writeGate.WaitAsync(ct).ConfigureAwait(false); + try { - snapshot.Id = existing.Id; - _col.Update(snapshot); - } + // upsert by (ClusterId, GenerationId) — replace in place if already cached + var existing = _col + .Find(s => s.ClusterId == snapshot.ClusterId && s.GenerationId == snapshot.GenerationId) + .FirstOrDefault(); - return Task.CompletedTask; + if (existing is null) + _col.Insert(snapshot); + else + { + snapshot.Id = existing.Id; + _col.Update(snapshot); + } + } + finally + { + _writeGate.Release(); + } } public Task PruneOldGenerationsAsync(string clusterId, int keepLatest = 10, CancellationToken ct = default) @@ -82,7 +97,11 @@ public sealed class LiteDbConfigCache : ILocalConfigCache, IDisposable return Task.CompletedTask; } - public void Dispose() => _db.Dispose(); + public void Dispose() + { + _writeGate.Dispose(); + _db.Dispose(); + } } public sealed class LocalConfigCacheCorruptException(string message, Exception inner) diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/LocalCache/ResilientConfigReader.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/LocalCache/ResilientConfigReader.cs index f8e8cc0..d628565 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/LocalCache/ResilientConfigReader.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/LocalCache/ResilientConfigReader.cs @@ -1,3 +1,4 @@ +using System.Text.RegularExpressions; using Microsoft.Extensions.Logging; using Polly; using Polly.Retry; @@ -61,6 +62,23 @@ public sealed class ResilientConfigReader _pipeline = builder.Build(); } + /// + /// Configuration-010: redact connection-string fragments (Password, User Id, Pwd, etc.) + /// that a caller's exception message could carry. Conservative regex pass — anything + /// matching Key=Value with a known credential key gets its value replaced. + /// + private static readonly Regex SecretsRegex = new( + @"(?ix)\b(Password|Pwd|User\s*Id|Uid|AccessToken|Authorization|Api[-_]?Key)\s*=\s*[^;,)\s]*", + RegexOptions.Compiled); + + internal static string ScrubSecrets(string? message) + { + if (string.IsNullOrEmpty(message)) return message ?? string.Empty; + // Replace the entire matched fragment (key + value) with a redaction marker so the + // key name itself doesn't leak — log scrapers grep for "Password=" too. + return SecretsRegex.Replace(message, "[redacted credential]"); + } + /// /// Execute through the resilience pipeline. On full failure /// (post-retry), reads the sealed cache for and passes the @@ -88,7 +106,15 @@ public sealed class ResilientConfigReader // that case, not propagate. Only rethrow if the caller actually requested cancellation. catch (Exception ex) when (ex is not OperationCanceledException || !cancellationToken.IsCancellationRequested) { - _logger.LogWarning(ex, "Central-DB read failed after retries; falling back to sealed cache for cluster {ClusterId}", clusterId); + // Configuration-010: do NOT pass the raw exception object — it carries the stack + // and inner-exception chain, and SqlException/wrapping delegates can surface + // connection-string fragments (Password=…, User Id=…) embedded in messages. + // Log only the exception type and a scrubbed message so secrets stay out of logs. + _logger.LogWarning( + "Central-DB read failed after retries ({ExceptionType}: {SanitizedMessage}); falling back to sealed cache for cluster {ClusterId}", + ex.GetType().Name, + ScrubSecrets(ex.Message), + clusterId); // GenerationCacheUnavailableException surfaces intentionally — fails the caller's // operation. StaleConfigFlag stays unchanged; the flag only flips when we actually // served a cache snapshot. diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/GenerationApplierTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/GenerationApplierTests.cs index b5f00a9..7219f05 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/GenerationApplierTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/GenerationApplierTests.cs @@ -128,4 +128,94 @@ public sealed class GenerationApplierTests result.Succeeded.ShouldBeFalse(); result.Errors.ShouldContain(e => e.Contains("tag-bad") && e.Contains("simulated")); } + + // ------------------------------------------------------------------------------------ + // Configuration-011 — pin the documented ordering behaviour: a thrown Removed callback + // records an entity error but the applier still runs the Added/Modified passes (the + // current contract — see GenerationApplier comment about cascades settling). + // ------------------------------------------------------------------------------------ + + [Fact] + public async Task Apply_continues_to_Added_pass_when_a_Removed_callback_throws() + { + var callLog = new List(); + var applier = new GenerationApplier(new ApplyCallbacks + { + OnTag = (c, _) => + { + callLog.Add($"tag:{c.Kind}:{c.LogicalId}"); + if (c.Kind == ChangeKind.Removed) + throw new InvalidOperationException("removed-failed"); + return Task.CompletedTask; + }, + }); + + var from = SnapshotWith(tags: [Tag("tag-old", "X")]); + var to = SnapshotWith(tags: [Tag("tag-new", "Y")]); + + var result = await applier.ApplyAsync(from, to, CancellationToken.None); + + result.Succeeded.ShouldBeFalse(); + result.Errors.ShouldContain(e => e.Contains("tag-old") && e.Contains("removed-failed")); + // The Added pass still runs even though Removed failed. + callLog.ShouldContain("tag:Removed:tag-old"); + callLog.ShouldContain("tag:Added:tag-new"); + } + + // ------------------------------------------------------------------------------------ + // Configuration-007 — ApplyPass must propagate OperationCanceledException rather than + // recording it as an entity error. Cancellation between passes must also halt the apply. + // ------------------------------------------------------------------------------------ + + [Fact] + public async Task Apply_propagates_OperationCanceledException_from_callback_when_token_cancelled() + { + // A callback that observes a cancelled token and throws OperationCanceledException + // must abort the entire apply, not be silently swallowed and recorded as an error. + using var cts = new CancellationTokenSource(); + var applier = new GenerationApplier(new ApplyCallbacks + { + OnTag = (c, ct) => + { + cts.Cancel(); + ct.ThrowIfCancellationRequested(); + return Task.CompletedTask; + }, + }); + + var to = SnapshotWith(tags: [Tag("tag-1", "A")]); + + await Should.ThrowAsync(async () => + await applier.ApplyAsync(from: null, to, cts.Token)); + } + + [Fact] + public async Task Apply_stops_between_passes_when_cancellation_requested() + { + // After a Removed pass completes, the applier should observe cancellation before + // running the Added/Modified passes — not silently keep walking. + var callLog = new List(); + using var cts = new CancellationTokenSource(); + var applier = new GenerationApplier(new ApplyCallbacks + { + OnTag = (c, _) => + { + callLog.Add($"tag:{c.Kind}:{c.LogicalId}"); + // Cancel after the Removed pass finishes — before the Added pass runs. + if (c.Kind == ChangeKind.Removed) cts.Cancel(); + return Task.CompletedTask; + }, + }); + + // `from` has tag-1, `to` has tag-2 — produces one Removed + one Added. + var from = SnapshotWith(tags: [Tag("tag-1", "A")]); + var to = SnapshotWith(tags: [Tag("tag-2", "B")]); + + await Should.ThrowAsync(async () => + await applier.ApplyAsync(from, to, cts.Token)); + + callLog.ShouldContain("tag:Removed:tag-1"); + callLog.ShouldNotContain("tag:Added:tag-2", + "Added pass must not run after cancellation observed between passes"); + } } diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/LiteDbConfigCacheTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/LiteDbConfigCacheTests.cs index 3316656..d1b7161 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/LiteDbConfigCacheTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/LiteDbConfigCacheTests.cs @@ -90,6 +90,38 @@ public sealed class LiteDbConfigCacheTests : IDisposable (await cache.GetMostRecentAsync("c-1"))!.PayloadJson.ShouldBe("{\"v\":2}"); } + // ------------------------------------------------------------------------------------ + // Configuration-005 — concurrent PutAsync for the same (ClusterId, GenerationId) must + // not produce duplicate rows. The original find-then-insert was non-atomic so two racing + // callers could both observe `existing is null` and both Insert. + // ------------------------------------------------------------------------------------ + [Fact] + public async Task PutAsync_concurrent_for_same_cluster_and_generation_does_not_duplicate() + { + using var cache = new LiteDbConfigCache(_dbPath); + // Pre-seed gen=99 so prune keepLatest:1 has a sentinel that survives independent of + // any potential duplicate (gen=42) row count. + await cache.PutAsync(Snapshot("c-1", 99)); + + // Many parallel writes for the same key. Without serialization, racing find-then-insert + // would Insert multiple rows for the same (ClusterId, GenerationId=42). + var tasks = Enumerable.Range(0, 64).Select(_ => Task.Run(async () => + { + var s = Snapshot("c-1", 42); + await cache.PutAsync(s); + })).ToArray(); + + await Task.WhenAll(tasks); + + // Count rows for gen=42 directly by inspecting the LiteDB file via a fresh handle. + cache.Dispose(); + using var verify = new LiteDB.LiteDatabase(_dbPath); + var col = verify.GetCollection("generations"); + var gen42Count = col.Find(s => s.ClusterId == "c-1" && s.GenerationId == 42).Count(); + gen42Count.ShouldBe(1, + $"PutAsync must upsert atomically — found {gen42Count} rows for (c-1, gen=42) after 64 concurrent puts"); + } + [Fact] public void Corrupt_file_surfaces_as_LocalConfigCacheCorruptException() { diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/NodePermissionsTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/NodePermissionsTests.cs new file mode 100644 index 0000000..b6235d4 --- /dev/null +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/NodePermissionsTests.cs @@ -0,0 +1,49 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Configuration.Enums; + +namespace ZB.MOM.WW.OtOpcUa.Configuration.Tests; + +/// +/// Pins the underlying type of to int so the SQL +/// storage type (HasConversion<int>() in OtOpcUaConfigDbContext) and the +/// XML doc ("Stored as int") cannot drift back into the latent uint→int overflow +/// trap caught by Configuration-004. +/// +[Trait("Category", "Unit")] +public sealed class NodePermissionsTests +{ + [Fact] + public void Underlying_type_is_int_so_it_matches_HasConversion_in_DbContext() + { + // Configuration-004: NodePermissions was declared : uint while NodeAcl.PermissionFlags + // is persisted via HasConversion(). A bit-31 grant would overflow int and corrupt + // the round-trip. Lock the underlying type to int. + typeof(NodePermissions).GetEnumUnderlyingType().ShouldBe(typeof(int)); + } + + [Fact] + public void All_defined_bits_round_trip_through_int_cast_without_loss() + { + // Belt-and-braces: every declared bit must survive a (int) round-trip — fails today + // if anyone re-introduces a bit-31 flag while the underlying type is uint. + foreach (NodePermissions value in Enum.GetValues()) + { + var asInt = (int)value; + var roundTripped = (NodePermissions)asInt; + roundTripped.ShouldBe(value); + } + } + + [Fact] + public void Bitwise_combinations_round_trip_through_int_storage() + { + // The PermissionFlags column stores int; combinations of bits must survive the conversion. + var combo = NodePermissions.Engineer | NodePermissions.MethodCall; + var stored = (int)combo; + var rebuilt = (NodePermissions)stored; + rebuilt.ShouldBe(combo); + rebuilt.HasFlag(NodePermissions.WriteTune).ShouldBeTrue(); + rebuilt.HasFlag(NodePermissions.MethodCall).ShouldBeTrue(); + } +} diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/ResilientConfigReaderTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/ResilientConfigReaderTests.cs index 056c734..2a72f98 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/ResilientConfigReaderTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/ResilientConfigReaderTests.cs @@ -1,3 +1,4 @@ +using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using Polly.Timeout; using Shouldly; @@ -186,6 +187,52 @@ public sealed class ResilientConfigReaderTests : IDisposable flag.IsStale.ShouldBeTrue("cache fallback marks the stale flag"); } + // ------------------------------------------------------------------------------------ + // Configuration-010 — fallback warning log must scrub connection-string fragments and + // must not include the full exception object (which carries the stack and any inner- + // exception chain). Project rule: no credential or connection-string fragment in logs. + // ------------------------------------------------------------------------------------ + + [Fact] + public async Task FallbackWarning_does_not_log_full_exception_object_or_password_fragment() + { + var cache = new GenerationSealedCache(_root); + await cache.SealAsync(new GenerationSnapshot + { + ClusterId = "cluster-e", GenerationId = 1, CachedAt = DateTime.UtcNow, + PayloadJson = "{\"ok\":true}", + }); + var flag = new StaleConfigFlag(); + var capturing = new CapturingLogger(); + var reader = new ResilientConfigReader(cache, flag, capturing, + timeout: TimeSpan.FromSeconds(10), retryCount: 0); + + // Simulated SqlException-style message carrying a connection-string fragment, the + // kind of thing a poorly-wrapped delegate could surface. + const string secretBearingMessage = + "Login failed for user 'sa'. (Server=sql.example.com,1433;User Id=sa;Password=SuperSecret123!)"; + + await reader.ReadAsync( + "cluster-e", + _ => throw new InvalidOperationException(secretBearingMessage), + snap => snap.PayloadJson, + CancellationToken.None); + + var warning = capturing.Records.ShouldHaveSingleItem(); + warning.LogLevel.ShouldBe(LogLevel.Warning); + + // The exception object passed as the first arg to LogWarning(ex, ...) drives the + // formatter's stack-trace dump; capturing it lets us assert the scrubbing surface. + warning.Exception.ShouldBeNull( + "the warning must not attach the raw exception — it can carry connection-string fragments"); + + // The rendered message must not echo password / user-id strings even if the caller + // embedded them in the exception message. + warning.RenderedMessage.ShouldNotContain("Password=", Case.Insensitive); + warning.RenderedMessage.ShouldNotContain("SuperSecret123!"); + warning.RenderedMessage.ShouldNotContain("User Id=", Case.Insensitive); + } + [Fact] public async Task CallerCancellation_Propagates_NotFallback() { @@ -220,6 +267,26 @@ public sealed class ResilientConfigReaderTests : IDisposable } } +internal sealed record LogRecord(LogLevel LogLevel, string RenderedMessage, Exception? Exception); + +internal sealed class CapturingLogger : ILogger +{ + public List Records { get; } = new(); + + public IDisposable BeginScope(TState state) where TState : notnull => NullScope.Instance; + public bool IsEnabled(LogLevel logLevel) => true; + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, Func formatter) + { + Records.Add(new LogRecord(logLevel, formatter(state, exception), exception)); + } + + private sealed class NullScope : IDisposable + { + public static readonly NullScope Instance = new(); + public void Dispose() { } + } +} + [Trait("Category", "Unit")] public sealed class StaleConfigFlagTests {