Joseph Doherty
03eadae677
001 the #552 topByTimestamp default admits a date-only column, so the committed wide-row read orders by a column that ties for every row written that day; 002 the session's gate is disposed under a racing call and LastUsedUtc only advances on completion, so a long expand is invisible to the TTL reaper; 003 the ad-hoc pasted connection string is an arbitrary outbound-connect primitive — correctly behind the /raw ConfigEditor policy, but undocumented as such. No code changes: the NodeId codec, the bound-parameter catalog path and the connection-ownership handoff are all sound. 79/79 tests unchanged.
81 lines
7.0 KiB
Markdown
81 lines
7.0 KiB
Markdown
# Code Review — Driver.Sql.Browser
|
|
|
|
<!-- Per-module findings file. See ../../REVIEW-PROCESS.md for the full process.
|
|
The base README.md is generated from these files by regen-readme.py — do not
|
|
edit README.md by hand. -->
|
|
|
|
| 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 | 3 |
|
|
|
|
## 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 | Open |
|
|
|
|
**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:** _(open)_
|
|
|
|
---
|
|
|
|
### 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 | Open |
|
|
|
|
**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:** _(open)_
|
|
|
|
---
|
|
|
|
### Driver.Sql.Browser-003
|
|
|
|
| Field | Value |
|
|
|---|---|
|
|
| Severity | Low |
|
|
| Category | Security |
|
|
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/SqlDriverBrowser.cs:262` |
|
|
| Status | Open |
|
|
|
|
**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:** _(open)_
|