# 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 = `, and the resulting wide-row query is `ORDER BY 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 `cd4b29a3`'s follow-up commit. **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`, via `TagConfigEditorMap` → `SqlTagConfigEditor` → `SqlAddressPickerBody`) — and **both** carry `@attribute [Authorize(Policy = AdminUiPolicies.ConfigEditor)]`, satisfied by `Administrator` or `Designer` only. `SqlAddressPickerBody` has no other consumer. - There is **no HTTP endpoint** for browse. `IBrowserSessionService` is a scoped service resolved from those pages' Blazor circuits (`EndpointRouteBuilderExtensions` registers it and nothing maps a route), so it inherits the page policy plus the `FallbackPolicy` beneath it. Nothing here is reachable by a lesser role or anonymously. - The gating is anti-drift: `PageAuthorizationGuardTests` pins **both** page types to `AdminUiPolicies.ConfigEditor` by exact policy name, so weakening either attribute fails the build. - `AdminUiPolicies.DriverOperator` gates 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.