Joseph Doherty
14 KiB
Code Review — Driver.Sql.Browser
| Field | Value |
|---|---|
| Module | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser |
| Reviewer | Claude Code |
| Review date | 2026-08-10 |
| Commit reviewed | b990adae |
| Status | Reviewed |
| Open findings | 0 |
Checklist coverage
| # | Category | Result |
|---|---|---|
| 1 | Correctness & logic bugs | 1 finding (001). The NodeId codec is sound — kind-prefixed arity, escaped separators, a dangling escape rejected rather than truncated — and the ordinal column match on attribute fetch is right for a case-sensitive collation |
| 2 | OtOpcUa conventions | No issues found — DriverType reads DriverTypeNames.Sql, the session is owned by the AdminUI BrowseSessionRegistry unchanged, and the project's no-runtime-driver layering (Gitea #526) holds and is guarded by DriverBrowserLayeringTests |
| 3 | Concurrency & thread safety | 1 finding (002). The single-permit gate correctly serialises catalog calls over one ADO.NET connection, and the disposed check inside the gate wait is the right place for it |
| 4 | Error handling & resilience | No issues found — a missing table yields an empty level rather than an error, a column terminal expand is a no-op, and the open path disposes the connection on every failure while the session owns it on success |
| 5 | Security | 1 finding (003). No name from a NodeId ever reaches a command text (schema/table are bound parameters, QuoteIdentifier is deliberately unused here), and the connection-string redaction on the open path is live-probed against the SqlClient parser's echo behaviour |
| 6 | Performance & resource management | No issues found. AttributesAsync re-reads the table's columns per call, which is one bounded catalog query per side-panel open and is what keeps the timestamp candidate consistent with the rendered tree |
| 7 | Design-document adherence | No issues found — the #552 decision (candidacy by declared TYPE, tie-break first-in-catalog-order, absent key rather than blank) is implemented exactly as docs/drivers/Sql.md and SqlBrowseAddressFields describe it |
| 8 | Code organization & conventions | No issues found |
| 9 | Testing coverage | No issues found — 79 tests including the four topByTimestamp address-field cases (present, chosen-first, absent, and the browsed-column-is-the-timestamp case) and a concurrency-probing connection |
| 10 | Documentation & comments | No issues found |
Findings
Driver.Sql.Browser-001
| Field | Value |
|---|---|
| Severity | Low |
| Category | Correctness & logic bugs |
| Location | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/SqlBrowseSession.cs:290 |
| Status | Resolved |
Description: PreferredTimestampColumn nominates the first column whose dialect-mapped type is DriverDataType.DateTime. That set includes date-only types: SqlServerDialect.MapColumnType maps "date" to DateTime, and so does PostgresDialect for date. A table whose first date/time column is a date therefore commits rowSelector.topByTimestamp = <date column>, and the resulting wide-row query is ORDER BY <date> DESC limited to one row — which ties for every row written on the same day, so "the newest row" becomes storage order among the ties. The reader cannot warn about it either, because the server has already discarded the other rows (see Driver.Sql-008). The choice is a visible, editable default, so the impact is bounded to a tag that silently binds to an arbitrary row of the latest day until someone notices.
Recommendation: Prefer a time-bearing type when the table has one — i.e. rank candidates and fall back to a date-only column only when nothing better exists — or drop date-only types from candidacy entirely, which makes such a table "not bindable by browse" and routes it through the existing DescribeUncommittableLeaf message. Either way state the choice in docs/drivers/Sql.md, which currently describes candidacy as simply "date/time-typed".
Resolution (2026-08-10): Fixed in a8cfc886, taking the ranking option rather than dropping date-only types. PreferredTimestampColumn now returns the first time-bearing candidate outright and remembers the first date-only one as a fallback; the tie-break within a rank is still first-in-catalog-order. Dropping date-only candidacy was rejected because it turns a table whose only date/time column is a date from "bound to an editable, imperfect default" into "un-committable" — a larger behaviour change than the defect.
The date-only test is provider-qualified (IsDateOnly, keyed off ISqlDialect.Provider): date is date-only on SQL Server and PostgreSQL, but Oracle's DATE carries a time-of-day component — which is exactly why OracleDialect.MapColumnType calls DateTime the faithful mapping there — so demoting it on Oracle would change the pick on the one backend where DATE is the ordinary timestamp type. The knowledge lives in the browser rather than on ISqlDialect because it is a browse-time preference (what is good to sort by), not a statement about what a value is, and nothing on the poll path consumes it.
Three tests, of which one was RED. AttributesAsync_prefersATimeBearingColumn_overAnEarlierDateOnlyOne runs against a new fixture table (DailyTotals) whose date-only column is declared first, so it fails against the plain first-in-catalog-order rule rather than passing by luck — RED before the fix (got shift_day, expected logged_at). The other two were green before and after, deliberately: …stillStatesADateOnlyColumn_whenTheTableHasNothingBetter pins the fallback (it goes red if candidacy is dropped instead of ranked), and …doesNotDemoteOraclesDATE_whichCarriesATimeOfDay is the falsification control for the provider branch — it drives the same catalog through OracleFlavouredSqliteDialect, which differs from SqliteDialect only in the Provider it reports, and is the single test a provider-blind implementation would fail. docs/drivers/Sql.md §"The committed row selector" now states the ranking and the Oracle exemption (its "two properties" list became three).
Driver.Sql.Browser-002
| Field | Value |
|---|---|
| Severity | Low |
| Category | Concurrency & thread safety |
| Location | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/SqlBrowseSession.cs:204, src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/SqlBrowseSession.cs:325 |
| Status | Resolved |
Description: Two small races around the registry reaper. (1) DisposeAsync disposes _gate unconditionally; a call already blocked in _gate.WaitAsync then throws ObjectDisposedException from inside the semaphore rather than the guarded ObjectDisposedException.ThrowIf(_disposed, this) the class raises everywhere else — the release side is defended against exactly this (catch (ObjectDisposedException) in the finally) but the wait side is not. (2) LastUsedUtc advances only after a call completes, so a slow catalog query (the AdminUI allows 20 s) is invisible to the TTL reaper for its whole duration; a session that idles up to the edge of the TTL and is then given one long expand can be disposed mid-call, tearing down the connection under an open reader.
Recommendation: Stamp LastUsedUtc on entry as well as on completion, and either leave _gate undisposed (the pattern SqlPollReader uses and documents) or drain it before disposing. Both are one-line changes; neither is observable to the picker today, which is why this is Low.
Resolution (2026-08-10): Both fixed in cd4b29a3, taking the "leave _gate undisposed" option. A SemaphoreSlim never handed out as a WaitHandle holds no unmanaged resource, so not disposing it leaks nothing, and DisposeAsync now says why in as many words.
Race (1) is worse than "the wrong exception type", which the RED run made plain. SemaphoreSlim.Dispose does not complete its pending async waiters, and the in-flight call's Release() in the finally then threw ObjectDisposedException from inside the semaphore — which the catch (ObjectDisposedException) there swallowed. So nothing ever released the parked waiter: the queued call did not fail with the wrong exception, it never returned at all. DisposeAsync_doesNotStrandACallerQueuedOnTheGate reproduces the ordinary reaper shape (one call in flight behind a ConcurrencyProbe delay, a second parked in WaitAsync, then dispose) and asserts through Task.WaitAsync(5 s), so a stranded caller surfaces as "expected ObjectDisposedException, got TimeoutException" — RED before the fix, taking the full 5 s. It also asserts ObjectName == typeof(SqlBrowseSession).FullName, which is what distinguishes the session's own guarded throw from the semaphore's and would go red again if the gate were re-disposed. With the gate left alive the finally release is now unconditional and cannot throw, so the swallowing catch is gone too.
Race (2): LastUsedUtc is stamped at QueryAsync entry as well as on completion, so a queued call refreshes the session too. LastUsedUtc_advancesWhenACallSTARTS_notOnlyWhenItCompletes observes the stamp while the call is still in flight (asserting call.IsCompleted is false at that moment, so the completion stamp cannot account for it) — RED before the fix. It sleeps 50 ms before starting the call so the assertion is about the stamp and not about DateTime.UtcNow's ~15.6 ms resolution on Windows.
Driver.Sql.Browser-003
| Field | Value |
|---|---|
| Severity | Low |
| Category | Security |
| Location | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/SqlDriverBrowser.cs:262 |
| Status | Won't Fix |
Description: OpenAsync accepts an ad-hoc connectionString literal from the browse form and dials it verbatim. That is an unrestricted outbound-connect primitive on the AdminUI host: an arbitrary host and port, plus whatever the provider's keyword set allows (Microsoft.Data.SqlClient alone offers AttachDbFilename, Failover Partner and Enlist). The mitigations in place are real and deliberate — the literal is never persisted (SqlDriverConfigDto has no such property, and the picker composes a session-only document), a paste alongside a connectionStringRef is logged, NodeEdit.razor warns against pasting one into a saved DriverConfig, and every open-path failure is redacted — but the capability itself is bounded only by whichever AdminUI policy guards the browse endpoint.
Recommendation: No code change proposed — the surface is reached only from /raw, which carries [Authorize(Policy = AdminUiPolicies.ConfigEditor)] (Administrator or Designer), so it is already at the right authorization level. Record the trust boundary where a reviewer will see it (docs/security.md, alongside the other AdminUI-authenticated capabilities): a Designer can make the AdminUI host open an arbitrary outbound database connection, which is a stronger capability than "browse a configured database" reads as.
Resolution — Won't Fix (2026-08-10): Documented, not restricted, in 34273708. The mitigating control is the AdminUI page policy, and it is admin-grade. Verified rather than assumed, because the finding's own wording ("bounded only by whichever AdminUI policy guards the browse endpoint") left it open:
- The reachable surfaces are exactly two —
/raw(GlobalRaw.razor) and/uns(GlobalUns.razor, viaTagConfigEditorMap→SqlTagConfigEditor→SqlAddressPickerBody) — and both carry@attribute [Authorize(Policy = AdminUiPolicies.ConfigEditor)], satisfied byAdministratororDesigneronly.SqlAddressPickerBodyhas no other consumer. - There is no HTTP endpoint for browse.
IBrowserSessionServiceis a scoped service resolved from those pages' Blazor circuits (EndpointRouteBuilderExtensionsregisters it and nothing maps a route), so it inherits the page policy plus theFallbackPolicybeneath it. Nothing here is reachable by a lesser role or anonymously. - The gating is anti-drift:
PageAuthorizationGuardTestspins both page types toAdminUiPolicies.ConfigEditorby exact policy name, so weakening either attribute fails the build. AdminUiPolicies.DriverOperatorgates only the live-ops action inside a session (Sparkplug rebirth), never the connect — so the connect is not reachable at a weaker level by that route either.
Adding a restriction (an allow-list of hosts, or refusing the literal outright) was rejected as a net loss: the literal exists so an operator can browse a database that has no connectionStringRef yet, which is the first step of authoring one, and any allow-list would itself be config authored by the same ConfigEditor role — a control that the party it constrains can edit is not a control. The residual capability is stated at both altitudes instead: docs/security.md §"Address-picker browse is an outbound-connect capability (trust boundary)" (under Control-Plane Authorization, naming the policy, the two pages, the guard test, and the fact that the same shape applies to the OPC UA client and MQTT pickers), and a paragraph on SqlDriverBrowser's own class summary pointing at it — so a reader who arrives at the code first does not have to infer the boundary from the hygiene. Existing hygiene is unchanged and explicitly labelled supporting, not the control: the literal is never persisted, a literal alongside a ref logs a warning naming the ref, and every open-path failure is redacted.