diff --git a/code-reviews/Driver.Galaxy.Browser/findings.md b/code-reviews/Driver.Galaxy.Browser/findings.md new file mode 100644 index 00000000..3b1750da --- /dev/null +++ b/code-reviews/Driver.Galaxy.Browser/findings.md @@ -0,0 +1,162 @@ +# Code Review — Driver.Galaxy.Browser + + + +| Field | Value | +|---|---| +| Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser` | +| Reviewer | Claude Code | +| Review date | 2026-06-19 | +| Commit reviewed | `7286d320` | +| Status | Reviewed | +| Open findings | 1 | + +## Checklist coverage + +A comprehensive review completes every category, recording "No issues found" where +a category produced nothing rather than leaving it blank. + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | Driver.Galaxy.Browser-001 (High): MapSecurityClass codes are shifted — mismatches the runtime SecurityMap | +| 2 | OtOpcUa conventions | No issues found | +| 3 | Concurrency & thread safety | Driver.Galaxy.Browser-002 (Medium): DisposeAsync has a TOCTOU race on _rootGate.Dispose() | +| 4 | Error handling & resilience | No issues found | +| 5 | Security | No issues found | +| 6 | Performance & resource management | No issues found | +| 7 | Design-document adherence | No issues found | +| 8 | Code organization & conventions | Driver.Galaxy.Browser-003 (Low): ResolveApiKey duplicated from GalaxyDriver with no sync mechanism | +| 9 | Testing coverage | Driver.Galaxy.Browser-004 (Low): MapSecurityClass not unit-tested; pure static method with no gateway dependency | +| 10 | Documentation & comments | No issues found | + +## Findings + + + +### Driver.Galaxy.Browser-001 + +| Field | Value | +|---|---| +| Severity | High | +| Category | Correctness & logic bugs | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser/GalaxyBrowseSession.cs:152` | +| Status | Resolved | + +**Description:** `GalaxyBrowseSession.MapSecurityClass` maps Galaxy `security_classification` +integer codes incorrectly. The Browser's switch arms are: + +| Code | Browser output | Runtime `SecurityMap` / `SecurityClassification` enum | +|---|---|---| +| 0 | "FreeAccess" | FreeAccess ✓ | +| 1 | "Operate" | Operate ✓ | +| 2 | "Tune" | **SecuredWrite** ✗ | +| 3 | "Configure" | **VerifiedWrite** ✗ | +| 4 | "ViewOnly" | **Tune** ✗ | +| 5 | "Unknown(5)" | **Configure** ✗ | +| 6 | "Unknown(6)" | **ViewOnly** ✗ | + +Codes 2–6 are all wrong. The AdminUI attribute side-panel labels "SecuredWrite" attributes +as "Tune" and "VerifiedWrite" attributes as "Configure", causing operators to misread +write-protection levels when selecting Galaxy tags. A tag the operator thinks is +"Tune"-restricted is actually read-only (SecuredWrite / VerifiedWrite → ViewOnly from +the OPC UA server's perspective). The correct mapping is already implemented in the +sibling `Driver.Galaxy/Browse/SecurityMap.cs` and matches the `SecurityClassification` +enum's integer assignments exactly. + +**Recommendation:** Fix `MapSecurityClass` to match `SecurityClassification` enum ordinals: +0→FreeAccess, 1→Operate, 2→SecuredWrite, 3→VerifiedWrite, 4→Tune, 5→Configure, +6→ViewOnly; unknown → `"Unknown({raw})"`. Add a unit test asserting each code. + +**Resolution:** Fixed 2026-06-19. Corrected all seven code-to-label arms in +`MapSecurityClass` to match `SecurityClassification` enum ordinals. Regression tests +`MapSecurityClass_maps_all_known_codes` and +`MapSecurityClass_unknown_code_returns_Unknown_label` added. + +--- + +### Driver.Galaxy.Browser-002 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Concurrency & thread safety | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser/GalaxyBrowseSession.cs:167` | +| Status | Resolved | + +**Description:** `DisposeAsync` uses a non-atomic read-then-write pattern on the +`_disposed` volatile field: + +```csharp +if (_disposed) return; +_disposed = true; +_rootGate.Dispose(); +``` + +Two concurrent callers can both observe `_disposed == false`, both set it to `true`, and +both call `_rootGate.Dispose()`. The second call throws `ObjectDisposedException` from +`SemaphoreSlim.Dispose()`, which propagates uncaught because the `try/catch` that +follows only wraps `_client.DisposeAsync()`. The `BrowseSessionReaper` (background +service) and a browser-side "Close" button can race in production. + +**Recommendation:** Wrap `_rootGate.Dispose()` in a `try/catch (ObjectDisposedException)` +mirroring the existing client disposal pattern, or use an atomic int guard via +`Interlocked.Exchange`. + +**Resolution:** Fixed 2026-06-19. Added `try { _rootGate.Dispose(); } catch (ObjectDisposedException) { }` +mirroring the existing pattern for `_client.DisposeAsync()`. Regression test +`DisposeAsync_concurrent_calls_do_not_throw` added. + +--- + +### Driver.Galaxy.Browser-003 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Code organization & conventions | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser/GalaxyDriverBrowser.cs:149` | +| Status | Open | + +**Description:** `GalaxyDriverBrowser.ResolveApiKey` is a verbatim copy of +`GalaxyDriver.ResolveApiKey`. The comment acknowledges this and explains why the Browser +project intentionally does not reference Driver.Galaxy. However, Finding +Driver.Galaxy.Browser-001 (the `MapSecurityClass` drift) demonstrates that duplicated +logic diverges. If a new secret-ref prefix (e.g. `vault:`) is added to the runtime +resolver, the Browser version will silently fall through to the cleartext-literal arm +and emit a spurious warning. + +**Recommendation:** Extract `ResolveApiKey` into the `Driver.Galaxy.Contracts` project, +which both `Driver.Galaxy` and `Driver.Galaxy.Browser` already reference. This is a +one-line addition to Contracts — no migration, no public-contract break. + +**Resolution:** _(deferred — requires a change in the Galaxy.Contracts project, outside +this module's boundary; tracked for a future consolidation pass)_ + +--- + +### Driver.Galaxy.Browser-004 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Testing coverage | +| Location | `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser.Tests/GalaxyBrowseSessionTests.cs` | +| Status | Resolved | + +**Description:** `GalaxyBrowseSession.MapSecurityClass` (all seven codes plus the +`Unknown(N)` fallback) had zero unit-test coverage. The existing test comment correctly +notes that RootAsync/ExpandAsync/AttributesAsync traversal is blocked by the internal +transport seam, but `MapSecurityClass` is a pure static method with no gateway dependency +— it is entirely testable in the unit suite. Finding Driver.Galaxy.Browser-001 (wrong +mapping for codes 2–6) would have been caught immediately had these tests existed. + +**Recommendation:** Add `[Fact]` tests for each of the seven known codes (0–6) and the +unknown-code fallback. + +**Resolution:** Fixed 2026-06-19. Tests `MapSecurityClass_maps_all_known_codes` and +`MapSecurityClass_unknown_code_returns_Unknown_label` added, covering all codes 0–6 +plus an out-of-range value. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser/GalaxyBrowseSession.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser/GalaxyBrowseSession.cs index bdbbeed0..f0f5aa71 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser/GalaxyBrowseSession.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser/GalaxyBrowseSession.cs @@ -146,28 +146,42 @@ internal sealed class GalaxyBrowseSession : IBrowseSession /// /// Maps the Galaxy raw security-classification integer to a display string. - /// Buckets: 0=FreeAccess, 1=Operate, 2=Tune, 3=Configure, 4=ViewOnly; - /// anything else surfaces as Unknown(N). + /// Matches the SecurityClassification enum ordinals and the runtime + /// SecurityMap in Driver.Galaxy: + /// 0=FreeAccess, 1=Operate, 2=SecuredWrite, 3=VerifiedWrite, + /// 4=Tune, 5=Configure, 6=ViewOnly; anything else surfaces as Unknown(N). /// - private static string MapSecurityClass(int raw) => raw switch + // internal for unit-test access (InternalsVisibleTo on the src project). + internal static string MapSecurityClass(int raw) => raw switch { 0 => "FreeAccess", 1 => "Operate", - 2 => "Tune", - 3 => "Configure", - 4 => "ViewOnly", + 2 => "SecuredWrite", + 3 => "VerifiedWrite", + 4 => "Tune", + 5 => "Configure", + 6 => "ViewOnly", _ => $"Unknown({raw})", }; /// /// Idempotently tears down the underlying repository client. Swallows exceptions /// on shutdown — the registry's reaper may be racing a client-initiated close. + /// Both and the client dispose are wrapped + /// in try/catch so a concurrent second dispose call never propagates. /// public async ValueTask DisposeAsync() { if (_disposed) return; _disposed = true; - _rootGate.Dispose(); + try + { + _rootGate.Dispose(); + } + catch (ObjectDisposedException) + { + // Concurrent DisposeAsync caller already disposed the gate — safe to ignore. + } try { await _client.DisposeAsync().ConfigureAwait(false); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser.Tests/GalaxyBrowseSessionTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser.Tests/GalaxyBrowseSessionTests.cs index a2c69819..d2e77df7 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser.Tests/GalaxyBrowseSessionTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser.Tests/GalaxyBrowseSessionTests.cs @@ -112,4 +112,57 @@ public sealed class GalaxyBrowseSessionTests await Should.ThrowAsync( () => session.ExpandAsync("Galaxy.Unknown", TestContext.Current.CancellationToken)); } + + /// + /// Verifies that each of the seven defined Galaxy security_classification codes + /// maps to the correct label string — matching the SecurityClassification + /// enum ordinals and the runtime SecurityMap in Driver.Galaxy. + /// (Regression for Driver.Galaxy.Browser-001: codes 2–6 were all wrong before + /// this fix.) + /// + [Theory] + [InlineData(0, "FreeAccess")] + [InlineData(1, "Operate")] + [InlineData(2, "SecuredWrite")] + [InlineData(3, "VerifiedWrite")] + [InlineData(4, "Tune")] + [InlineData(5, "Configure")] + [InlineData(6, "ViewOnly")] + public void MapSecurityClass_maps_all_known_codes(int code, string expectedLabel) + { + GalaxyBrowseSession.MapSecurityClass(code).ShouldBe(expectedLabel); + } + + /// + /// An unrecognised Galaxy security_classification code must produce an + /// Unknown(N) label — not throw and not silently return a valid class. + /// + [Theory] + [InlineData(7)] + [InlineData(99)] + [InlineData(-1)] + public void MapSecurityClass_unknown_code_returns_Unknown_label(int code) + { + var label = GalaxyBrowseSession.MapSecurityClass(code); + label.ShouldStartWith("Unknown("); + label.ShouldContain(code.ToString()); + } + + /// + /// Two concurrent calls on the + /// same session must not throw. The registry reaper and a browser-side Close + /// can race in production. + /// (Regression for Driver.Galaxy.Browser-002: the second caller could throw + /// from SemaphoreSlim.Dispose().) + /// + [Fact] + public async Task DisposeAsync_concurrent_calls_do_not_throw() + { + var session = new GalaxyBrowseSession(NewClient()); + // Fire two concurrent dispose calls and await both — neither must throw. + await Should.NotThrowAsync(async () => + { + await Task.WhenAll(session.DisposeAsync().AsTask(), session.DisposeAsync().AsTask()); + }); + } }