From 073930e02397d206a1da150aa881e49680325710 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 07:56:58 -0400 Subject: [PATCH 1/5] fix(sql): withhold the JSON parser's message from the probe's red result (Driver.Sql-009) SqlDriverProbe surfaced 'Config JSON is invalid: ' + ex.Message, where SqlDriverBrowser.ParseDocument deliberately withholds the identical message because the malformed text may itself carry a connection string. The two read the same blob shape - the AdminUI Sql picker composes a document with a top-level connectionString literal - so a malformed paste reaches both. The probe now reports the failure class only, matching the browser. --- .../SqlDriverProbe.cs | 16 ++++++++-- .../SqlDriverProbeTests.cs | 32 +++++++++++++++++++ 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriverProbe.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriverProbe.cs index fe28c346..3838db99 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriverProbe.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriverProbe.cs @@ -19,7 +19,10 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Sql; /// timeout, a cancellation — becomes a red , so the AdminUI has nothing to /// catch. /// The resolved connection string never reaches the result message. A red result names the -/// failure class, never the string — the same credential discipline the factory and the driver keep. +/// failure class, never the string — the same credential discipline the factory and the driver keep. +/// That includes the JSON parser: a malformed config is reported as a class, with the parser's own +/// message withheld exactly as SqlDriverBrowser.ParseDocument withholds it, because the malformed +/// text may itself be a pasted connection string. /// public sealed class SqlDriverProbe : IDriverProbe { @@ -80,7 +83,16 @@ public sealed class SqlDriverProbe : IDriverProbe } catch (Exception ex) when (ex is System.Text.Json.JsonException or ArgumentNullException) { - return new DriverProbeResult(false, $"Config JSON is invalid: {ex.Message}", null); + // The parser's message is WITHHELD, matching SqlDriverBrowser.ParseDocument. Nothing has been + // extracted at this point, so there is no known secret to redact the message against — and the + // blob both types read is the one the AdminUI Sql picker composes, which carries a top-level + // `connectionString` literal. A malformed paste is therefore exactly the text that reaches here, + // and System.Text.Json quotes the offending token and states the JSON path and byte position. + return new DriverProbeResult( + false, + "Config JSON is not valid JSON. (The parser's message is withheld because the malformed " + + "text may carry a connection string.)", + null); } if (dto is null) diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlDriverProbeTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlDriverProbeTests.cs index e8eb97a7..54235cee 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlDriverProbeTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlDriverProbeTests.cs @@ -79,6 +79,38 @@ public sealed class SqlDriverProbeTests result.Message.ShouldNotBeNullOrWhiteSpace(); } + /// + /// A malformed config is reported as a failure CLASS, never as the parser's own message — the same + /// discipline SqlDriverBrowser.ParseDocument keeps, and for the same reason: the blob the two + /// read is the one the AdminUI Sql picker composes, which carries a top-level connectionString + /// literal, so a malformed paste is exactly the text that reaches here. + /// 's message quotes the offending token and states the JSON path + /// and the byte position. None of it may leave the probe: at this point nothing has been extracted, + /// so there is no known secret to redact against. + /// + [Fact] + public async Task Probe_malformedConfig_withholdsTheParserMessage() + { + using var fixture = new SqlitePollFixture(); + var probe = SqlDriverProbe.ForTest(fixture.Factory, new SqliteDialect()); + + // A pasted literal whose password carries an unquoted ';' — mis-quoted exactly the way an operator's + // paste is, so the parse fails partway THROUGH the credential. + const string malformed = + """{"provider":"SqlServer","connectionString":Password=Sup3r;SecretTail}"""; + + var result = await probe.ProbeAsync(malformed, TimeSpan.FromSeconds(5), CancellationToken.None); + + result.Ok.ShouldBeFalse(); + result.Message.ShouldNotBeNull(); + result.Message!.ShouldNotContain("LineNumber"); + result.Message.ShouldNotContain("BytePositionInLine"); + result.Message.ShouldNotContain("Sup3r"); + result.Message.ShouldNotContain("SecretTail"); + // Still actionable: it names what is wrong and why the detail is missing. + result.Message.ShouldContain("not valid JSON"); + } + [Fact] public async Task Probe_providerNumberSpelling_parses_soTestConnectMatchesDeploy() { From a8cfc886a3b6187edd1130af8620fb397653c609 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 07:59:35 -0400 Subject: [PATCH 2/5] fix(sql-browser): rank the row-selector candidate so a date-only column is a last resort (Driver.Sql.Browser-001) PreferredTimestampColumn nominated the first column mapping to DriverDataType .DateTime, and that set includes date-only types (SQL Server / PostgreSQL 'date'). A table whose first date/time column is a 'date' committed rowSelector.topByTimestamp = , making the wide-row read ORDER BY DESC LIMIT 1 - which ties for every row written that day, so 'the newest row' became storage order among the ties. Time-bearing candidates now outrank date-only ones; a date-only column is still stated when it is all the table has, so such a table stays bindable. Oracle's DATE carries a time of day and is exempt - pinned by a control dialect that differs from the SQLite one only in the Provider it reports. --- docs/drivers/Sql.md | 15 +++- .../SqlBrowseSession.cs | 64 +++++++++++++-- .../OracleFlavouredSqliteDialect.cs | 64 +++++++++++++++ .../SqlBrowseSessionTests.cs | 77 +++++++++++++++++++ .../SqliteBrowseFixture.cs | 45 +++++++++++ 5 files changed, 254 insertions(+), 11 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests/OracleFlavouredSqliteDialect.cs diff --git a/docs/drivers/Sql.md b/docs/drivers/Sql.md index 38163f50..575a5128 100644 --- a/docs/drivers/Sql.md +++ b/docs/drivers/Sql.md @@ -190,14 +190,23 @@ commit writes as `rowSelector.topByTimestamp` ("read the newest row"). Before th a selector-less blob that `SqlEquipmentTagParser` and the authoring gate both refuse, so no browse-committed Sql tag was ever readable. -Two properties of the choice are deliberate: +Three properties of the choice are deliberate: - **Candidacy is by declared TYPE, never by column name.** The session asks `ISqlDialect.MapColumnType` for `DriverDataType.DateTime` — the same metadata the browse tree already renders per column. A name heuristic (`*_ts`, `*time*`) would happily order a wide-row read by a *string* column called `timestamp`, binding the tag to whichever row sorts last lexically. -- **The tie-break is first in catalog order** (every dialect's column query orders by ordinal - position), so the pick is stable across calls, connections and servers. On a +- **Candidates are RANKED: a time-bearing type beats a date-only one.** `MapColumnType` reports + `DriverDataType.DateTime` for SQL Server's and PostgreSQL's `date` too, and ordering a single-row read + by a date-only column ties for *every row written on the same day* — "the newest row" quietly becomes + storage order among the ties, and the reader cannot warn about it because the server has already + discarded the others. So a `datetime2`/`timestamptz`/… column wins over a `date` one wherever both + exist, regardless of declaration order. A date-only column is still stated when it is **all** the table + has, so such a table stays bindable by browse rather than becoming un-committable. + **Oracle's `DATE` is exempt** — it carries a time-of-day component (which is why `OracleDialect` maps + it faithfully rather than as a widening), so it is not demoted there. +- **The tie-break within a rank is first in catalog order** (every dialect's column query orders by + ordinal position), so the pick is stable across calls, connections and servers. On a `CreatedUtc`/`ModifiedUtc` pair it takes the earlier-declared one. It is a **visible, editable default**: the typed editor shows the selector and the operator can change it — to the other timestamp, or to a `whereColumn`/`whereValue` pair entirely. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/SqlBrowseSession.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/SqlBrowseSession.cs index 5ffb7106..810442bd 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/SqlBrowseSession.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/SqlBrowseSession.cs @@ -63,6 +63,14 @@ internal sealed class SqlBrowseSession : IBrowseSession /// private const string ReadOnlySecurityClass = "ViewOnly"; + /// + /// The one SQL type family that maps to while carrying no + /// time of day — SQL Server's and PostgreSQL's date. Ranked below the time-bearing + /// families by ; see for the Oracle + /// exception. + /// + private const string DateOnlyTypeFamily = "date"; + private readonly DbConnection _connection; private readonly ISqlDialect _dialect; @@ -274,12 +282,23 @@ internal sealed class SqlBrowseSession : IBrowseSession /// row sorts last lexically. So the only input is /// reporting , which is /// the same catalog metadata the browse tree already renders per column, on every dialect. - /// Tie-break: FIRST in catalog order. Every dialect's - /// orders by ordinal position (ORDINAL_POSITION / column_id), so "first" means the - /// earliest-declared date/time column and is stable across calls, connections and servers — an - /// unordered "any candidate" would let two operators browsing the same table commit two differently - /// bound tags. On the usual multi-candidate shape (a CreatedUtc/ModifiedUtc pair) it - /// picks the earlier-declared one, which is a preference and not a fact about the data. + /// Ranked, because "maps to DateTime" includes DATE-ONLY types. + /// reports for SQL + /// Server's and PostgreSQL's date as well — and a wide-row read ordered by a date-only column + /// is ORDER BY <date> DESC limited to one row, which ties for every row written on + /// the same day: "the newest row" silently becomes storage order among the ties, and the reader + /// cannot warn about it because the server has already discarded the others. So a time-bearing + /// candidate always outranks a date-only one, wherever the two are declared. + /// Falling back to a date-only column is deliberate rather than dropping those types from + /// candidacy: a table whose only date/time column is a date stays bindable by browse (with a + /// default the operator can see and change) instead of becoming un-committable. + /// Tie-break within a rank: FIRST in catalog order. Every dialect's + /// orders by ordinal position (ORDINAL_POSITION / + /// column_id), so "first" means the earliest-declared candidate and is stable across calls, + /// connections and servers — an unordered "any candidate" would let two operators browsing the same + /// table commit two differently bound tags. On the usual multi-candidate shape (a + /// CreatedUtc/ModifiedUtc pair) it picks the earlier-declared one, which is a preference + /// and not a fact about the data. /// It is a visible, editable DEFAULT, not a hidden binding. The commit writes it into the /// tag's rowSelector.topByTimestamp, where the typed Sql editor shows it and the operator can /// change it — to the other timestamp, or to a whereColumn/whereValue pair entirely. @@ -289,13 +308,42 @@ internal sealed class SqlBrowseSession : IBrowseSession /// private string? PreferredTimestampColumn(IReadOnlyList<(string Name, string DataType)> columns) { + // One pass, remembering the best date-only candidate seen so far: the first time-bearing column wins + // outright, and the date-only one is only used if the walk ends without finding one. + string? dateOnlyFallback = null; + foreach (var column in columns) { if (string.IsNullOrWhiteSpace(column.Name)) continue; - if (_dialect.MapColumnType(column.DataType) == DriverDataType.DateTime) return column.Name; + if (_dialect.MapColumnType(column.DataType) != DriverDataType.DateTime) continue; + + if (!IsDateOnly(column.DataType)) return column.Name; + dateOnlyFallback ??= column.Name; } - return null; + return dateOnlyFallback; + } + + /// + /// Whether a column's declared type family carries a date but no time of day — the case that makes an + /// ORDER BY … DESC single-row read tie for a whole day's rows. + /// + /// The bare SQL type family name, as the catalog reported it. + /// when the family is date-only on this dialect's backend. + /// + /// Provider-qualified, because the spelling is not the meaning. date is date-only on SQL + /// Server and PostgreSQL, but Oracle's DATE carries a time-of-day component — which is + /// why OracleDialect.MapColumnType calls the faithful + /// mapping there rather than a widening. Demoting it on Oracle would change the pick on the backend + /// where DATE is the ordinary timestamp type, for no gain. + /// The check is a browse-time preference and belongs here rather than on + /// : nothing on the poll path cares, and the type map is deliberately about + /// what a value IS, not about what it is a good idea to sort by. + /// + private bool IsDateOnly(string sqlDataType) + { + if (_dialect.Provider == SqlProvider.Oracle) return false; + return string.Equals(sqlDataType.Trim(), DateOnlyTypeFamily, StringComparison.OrdinalIgnoreCase); } /// diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests/OracleFlavouredSqliteDialect.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests/OracleFlavouredSqliteDialect.cs new file mode 100644 index 00000000..270db34a --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests/OracleFlavouredSqliteDialect.cs @@ -0,0 +1,64 @@ +using System.Data.Common; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.Sql.Contracts; +using ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests; + +/// +/// in every respect except that it reports . +/// It exists for exactly one assertion: Oracle's DATE carries a time-of-day component +/// (OracleDialect.MapColumnType says so in as many words), so it is NOT the date-only type the +/// row-selector ranking demotes on the other dialects, and an Oracle table whose DATE column is +/// declared before its TIMESTAMP must still pick the DATE — the ordinary +/// first-in-catalog-order tie-break. +/// Without this control, implementing the ranking provider-blind ("any type family spelled +/// date is date-only") passes every other test in the suite while silently changing the pick on +/// the one backend where the spelling means something else. +/// +public sealed class OracleFlavouredSqliteDialect : ISqlDialect +{ + private readonly SqliteDialect _inner = new(); + + /// The whole point of the type: an Oracle-shaped provider over the SQLite catalog. + public SqlProvider Provider => SqlProvider.Oracle; + + /// + public DbProviderFactory Factory => _inner.Factory; + + /// + public string LivenessSql => _inner.LivenessSql; + + /// + public string SingleRowLimitPrefix => _inner.SingleRowLimitPrefix; + + /// + public string SingleRowLimitSuffix => _inner.SingleRowLimitSuffix; + + /// + public string ListSchemasSql => _inner.ListSchemasSql; + + /// + public string DefaultSchemaSql => _inner.DefaultSchemaSql; + + /// + public string ListTablesSql => _inner.ListTablesSql; + + /// + public string ListColumnsSql => _inner.ListColumnsSql; + + /// + public string QuoteIdentifier(string ident) => _inner.QuoteIdentifier(ident); + + /// + public string ParameterMarker(string baseName) => _inner.ParameterMarker(baseName); + + /// + public string ParameterBindName(string baseName) => _inner.ParameterBindName(baseName); + + /// + public void ConfigureCommand(DbCommand command) => _inner.ConfigureCommand(command); + + /// + public DriverDataType MapColumnType(string sqlDataType) => _inner.MapColumnType(sqlDataType); +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests/SqlBrowseSessionTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests/SqlBrowseSessionTests.cs index 7288b40d..6b2ca8e7 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests/SqlBrowseSessionTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests/SqlBrowseSessionTests.cs @@ -458,6 +458,83 @@ public sealed class SqlBrowseSessionTests .ShouldBeGreaterThan(0, "the expected pick must NOT also be the alphabetically first one"); } + /// + /// A date-only column is a candidate of last resort, not the first pick. date maps to + /// on every dialect, so a candidacy rule that only asked the + /// mapped type would commit ORDER BY <date> DESC LIMIT 1 — which ties for every row + /// written on the same day, making "the newest row" storage order among the ties. The reader cannot + /// warn about it either: the server has already discarded the other rows. + /// The fixture's table declares the date-only column FIRST, so this fails against the plain + /// first-in-catalog-order rule rather than passing by luck. + /// + [Fact] + public async Task AttributesAsync_prefersATimeBearingColumn_overAnEarlierDateOnlyOne() + { + await using var db = await SqliteBrowseFixture.CreateAsync(); + await using var session = new SqlBrowseSession(db.Connection, new SqliteDialect()); + + var nodeId = SqlBrowseNodeId.ForColumn( + SqliteDialect.MainSchema, + SqliteBrowseFixture.DateOnlyFirstTable, + SqliteBrowseFixture.DateOnlyFirstValueColumn); + + var fields = (await session.AttributesAsync(nodeId, CancellationToken.None)) + .ShouldHaveSingleItem().AddressFields.ShouldNotBeNull(); + + fields[SqlBrowseAddressFields.TopByTimestamp].ShouldBe( + SqliteBrowseFixture.DateOnlyFirstTimeColumn, + "a time-bearing column outranks a date-only one even when the date-only one is declared first"); + } + + /// + /// The fallback half of the ranking: when date-only is ALL the table has, it is still stated. Dropping + /// date-only types from candidacy outright would instead make such a table unbindable by browse, which + /// is a bigger behaviour change than the defect being fixed. + /// + [Fact] + public async Task AttributesAsync_stillStatesADateOnlyColumn_whenTheTableHasNothingBetter() + { + await using var db = await SqliteBrowseFixture.CreateAsync(); + await using var session = new SqlBrowseSession(db.Connection, new SqliteDialect()); + + var nodeId = SqlBrowseNodeId.ForColumn( + SqliteDialect.MainSchema, + SqliteBrowseFixture.DateOnlySoleTable, + SqliteBrowseFixture.DateOnlySoleValueColumn); + + var fields = (await session.AttributesAsync(nodeId, CancellationToken.None)) + .ShouldHaveSingleItem().AddressFields.ShouldNotBeNull(); + + fields[SqlBrowseAddressFields.TopByTimestamp].ShouldBe(SqliteBrowseFixture.DateOnlySoleColumn); + } + + /// + /// The falsification control for the ranking: Oracle's DATE carries a time-of-day + /// component, so it is not date-only there and must not be demoted. Same catalog, same columns, + /// only the dialect's differs — and the pick reverts to the + /// ordinary first-in-catalog-order one. + /// A ranking implemented provider-blind ("any family spelled date") passes every other + /// test in this file and fails only here. + /// + [Fact] + public async Task AttributesAsync_doesNotDemoteOraclesDATE_whichCarriesATimeOfDay() + { + await using var db = await SqliteBrowseFixture.CreateAsync(); + await using var session = new SqlBrowseSession(db.Connection, new OracleFlavouredSqliteDialect()); + + var nodeId = SqlBrowseNodeId.ForColumn( + SqliteDialect.MainSchema, + SqliteBrowseFixture.DateOnlyFirstTable, + SqliteBrowseFixture.DateOnlyFirstValueColumn); + + var fields = (await session.AttributesAsync(nodeId, CancellationToken.None)) + .ShouldHaveSingleItem().AddressFields.ShouldNotBeNull(); + + fields[SqlBrowseAddressFields.TopByTimestamp].ShouldBe( + SqliteBrowseFixture.DateOnlyFirstColumn, + "Oracle's DATE is time-bearing, so the first-in-catalog-order tie-break applies unchanged"); + } + /// /// A table with no date/time column states NO candidate — the key is absent, never blank — so the /// commit can tell "this table cannot be bound by browse" from "the session said nothing". diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests/SqliteBrowseFixture.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests/SqliteBrowseFixture.cs index b77744a1..9a2318cb 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests/SqliteBrowseFixture.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests/SqliteBrowseFixture.cs @@ -96,6 +96,40 @@ public sealed class SqliteBrowseFixture : IAsyncDisposable /// A value column, sitting BETWEEN the two timestamps. public const string MultiTimeValueColumn = "level"; + /// + /// A table whose FIRST date/time-typed column is date-only, with a genuinely time-bearing one + /// declared after it. A date-only column maps to + /// like any other, so a + /// candidacy rule that only asked the mapped type would order the wide-row read by + /// — one row per DAY, ties broken by storage order. + /// + public const string DateOnlyFirstTable = "DailyTotals"; + + /// The date-only column of , declared first. + public const string DateOnlyFirstColumn = "shift_day"; + + /// A value column, sitting between the two candidates. + public const string DateOnlyFirstValueColumn = "units"; + + /// + /// The time-bearing column of , declared LAST — the expected pick, + /// even though the date-only one comes first in catalog order. + /// + public const string DateOnlyFirstTimeColumn = "logged_at"; + + /// + /// A table whose ONLY date/time-typed column is date-only — the fallback control. Dropping date-only + /// types from candidacy outright would make this table unbindable by browse; ranking keeps it bindable + /// while still preferring a better column wherever one exists. + /// + public const string DateOnlySoleTable = "DayBook"; + + /// The sole (date-only) candidate of . + public const string DateOnlySoleColumn = "book_day"; + + /// A value column. + public const string DateOnlySoleValueColumn = "opening_balance"; + private readonly string _databasePath; private SqliteBrowseFixture(string databasePath, string connectionString, SqliteConnection connection) @@ -210,6 +244,17 @@ public sealed class SqliteBrowseFixture : IAsyncDisposable "{MultiTimeValueColumn}" REAL NULL, "{MultiTimeSecondColumn}" TIMESTAMP NULL ); + + CREATE TABLE "{DateOnlyFirstTable}" ( + "{DateOnlyFirstColumn}" DATE NOT NULL, + "{DateOnlyFirstValueColumn}" REAL NULL, + "{DateOnlyFirstTimeColumn}" DATETIME NOT NULL + ); + + CREATE TABLE "{DateOnlySoleTable}" ( + "{DateOnlySoleColumn}" DATE NOT NULL, + "{DateOnlySoleValueColumn}" REAL NULL + ); """).ConfigureAwait(false); // Separate statements: these names must be quoted by the seed itself, so keep them off the From cd4b29a36879110c44311a4fe38540d3fd7aadb6 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 08:01:30 -0400 Subject: [PATCH 3/5] fix(sql-browser): close two races around the registry reaper (Driver.Sql.Browser-002) (1) DisposeAsync disposed _gate unconditionally. SemaphoreSlim.Dispose does not complete pending async waiters, and the in-flight call's Release() then threw ObjectDisposedException from inside the semaphore (swallowed by the finally), so a caller parked in WaitAsync was never released and its call never returned. The gate is now left alive - the pattern SqlPollReader documents - so the waiter is admitted and meets the session's own guarded ThrowIf(_disposed, this). (2) LastUsedUtc advanced only on completion, so a slow catalog call (the AdminUI allows 20 s) was invisible to the TTL reaper for its whole duration and a session at the edge of its TTL could be disposed mid-call. It is now stamped on entry too. --- .../SqlBrowseSession.cs | 33 ++++++--- .../SqlBrowseSessionTests.cs | 69 +++++++++++++++++++ 2 files changed, 91 insertions(+), 11 deletions(-) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/SqlBrowseSession.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/SqlBrowseSession.cs index 810442bd..defef826 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/SqlBrowseSession.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/SqlBrowseSession.cs @@ -205,8 +205,18 @@ internal sealed class SqlBrowseSession : IBrowseSession } /// - /// Idempotently closes the owned connection and the gate. Errors are swallowed: the registry's reaper - /// may be racing a server-side disconnect, and a failed close must never surface in the AdminUI. + /// Idempotently closes the owned connection. Errors are swallowed: the registry's reaper may be racing + /// a server-side disconnect, and a failed close must never surface in the AdminUI. + /// The gate is deliberately NOT disposed — the same call + /// SqlPollReader makes, and for a sharper reason here. This session is disposed by the + /// registry's TTL reaper, on its own thread, while a picker may have a second call parked in + /// . Disposing the semaphore does not complete + /// its pending async waiters, and the in-flight call's Release() would then throw + /// from inside the semaphore — so the parked caller is never + /// released and its call never returns at all. Left undisposed, the waiter is admitted and meets the + /// guarded ObjectDisposedException.ThrowIf(_disposed, this) every other member raises. + /// A that was never handed out as a holds no + /// unmanaged resource, so not disposing it leaks nothing. /// /// A task that represents the asynchronous operation. public async ValueTask DisposeAsync() @@ -216,9 +226,6 @@ internal sealed class SqlBrowseSession : IBrowseSession try { await _connection.DisposeAsync().ConfigureAwait(false); } catch { /* best-effort: the connection may already be broken or closed. */ } - - try { _gate.Dispose(); } - catch { /* best-effort: a concurrent second dispose already tore it down. */ } } private async Task> ExpandSchemaAsync(string schema, CancellationToken ct) @@ -367,8 +374,12 @@ internal sealed class SqlBrowseSession : IBrowseSession /// /// Runs one catalog query under the gate and projects every row. The disposed check happens - /// inside the gate wait so a dispose racing an in-flight call cannot be missed, and - /// only advances on a call that actually completed. + /// inside the gate wait so a dispose racing an in-flight call cannot be missed. + /// is stamped on entry as well as on completion. The AdminUI + /// allows a catalog call 20 s (BrowserSessionService.PerCallTimeout) and a queued one waits + /// behind it; stamping only on completion leaves a long expand invisible to the registry's TTL reaper + /// for its whole duration, so a session that idled to the edge of the TTL and was then given one slow + /// call gets reaped mid-call — tearing the connection down under an open reader. /// private async Task> QueryAsync( string sql, @@ -376,6 +387,7 @@ internal sealed class SqlBrowseSession : IBrowseSession Func project, CancellationToken cancellationToken) { + LastUsedUtc = DateTime.UtcNow; ObjectDisposedException.ThrowIf(_disposed, this); await _gate.WaitAsync(cancellationToken).ConfigureAwait(false); try @@ -400,10 +412,9 @@ internal sealed class SqlBrowseSession : IBrowseSession } finally { - // A dispose that raced this call has already disposed the gate; releasing it then is harmless to - // ignore and must not mask the real result. - try { _gate.Release(); } - catch (ObjectDisposedException) { } + // Unconditional, and it cannot throw: DisposeAsync deliberately leaves the gate alive precisely + // so this release still reaches whoever is parked behind us. + _gate.Release(); } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests/SqlBrowseSessionTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests/SqlBrowseSessionTests.cs index 6b2ca8e7..6be390f9 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests/SqlBrowseSessionTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests/SqlBrowseSessionTests.cs @@ -328,6 +328,75 @@ public sealed class SqlBrowseSessionTests () => session.ExpandAsync(SchemaNodeId, CancellationToken.None)); } + /// + /// A dispose racing an in-flight call must not strand the caller queued behind it. The registry's + /// TTL reaper disposes on its own thread while a picker may have two clicks outstanding, so this is the + /// ordinary shape, not a corner: the second call is parked in SemaphoreSlim.WaitAsync. + /// Disposing the gate does not complete its pending async waiters, and the in-flight call's + /// Release() then throws from inside the semaphore — so + /// the waiter is never released and the call never returns at all. The session must instead let it + /// through to the guarded ObjectDisposedException.ThrowIf(_disposed, this) every other member + /// raises. + /// + [Fact] + public async Task DisposeAsync_doesNotStrandACallerQueuedOnTheGate() + { + await using var db = await SqliteBrowseFixture.CreateAsync(); + var probe = new ConcurrencyProbe { Delay = TimeSpan.FromMilliseconds(400) }; + var probing = new ConcurrencyProbingDbConnection(db.OpenNewConnection(), probe); + var session = new SqlBrowseSession(probing, new SqliteDialect()); + + // First call takes the gate and holds it for the probe's delay; second parks in WaitAsync. + var inFlight = session.RootAsync(CancellationToken.None); + await Task.Delay(100, TestContext.Current.CancellationToken); + var queued = session.ExpandAsync(SchemaNodeId, CancellationToken.None); + + await session.DisposeAsync(); + + // The in-flight call's own outcome is not the assertion — it may fail on the closed connection. + try { await inFlight; } + catch (Exception ex) when (ex is ObjectDisposedException or InvalidOperationException) { } + + // A stranded caller surfaces as TimeoutException here, which reads as "expected + // ObjectDisposedException, got TimeoutException" — the honest description of the defect. + var disposed = await Should.ThrowAsync( + () => queued.WaitAsync(TimeSpan.FromSeconds(5), TestContext.Current.CancellationToken)); + disposed.ObjectName.ShouldBe( + typeof(SqlBrowseSession).FullName, + "the failure must be the session's own guarded throw, not the semaphore's"); + } + + /// + /// must advance when a call starts, not only when it + /// finishes. The AdminUI allows a catalog call 20 s; stamping only on completion leaves a slow expand + /// invisible to the registry's TTL reaper for its whole duration, so a session that idled up to the + /// edge of the TTL and was then given one long expand can be disposed mid-call — tearing the + /// connection down under an open reader. + /// + [Fact] + public async Task LastUsedUtc_advancesWhenACallSTARTS_notOnlyWhenItCompletes() + { + await using var db = await SqliteBrowseFixture.CreateAsync(); + var probe = new ConcurrencyProbe { Delay = TimeSpan.FromSeconds(2) }; + var probing = new ConcurrencyProbingDbConnection(db.OpenNewConnection(), probe); + await using var session = new SqlBrowseSession(probing, new SqliteDialect()); + + var atConstruction = session.LastUsedUtc; + // DateTime.UtcNow ticks at ~15.6 ms on Windows; without this the entry stamp could equal the + // construction stamp and the assertion would be about clock resolution rather than the stamp. + await Task.Delay(50, TestContext.Current.CancellationToken); + + var call = session.RootAsync(CancellationToken.None); + + // Observed WHILE the call is still running: the completion stamp cannot account for it. + await Task.Delay(300, TestContext.Current.CancellationToken); + call.IsCompleted.ShouldBeFalse("the probe's delay must still be holding the call open"); + session.LastUsedUtc.ShouldBeGreaterThan( + atConstruction, "the stamp must advance on entry, not only on completion"); + + await call; + } + /// /// The AdminUI bounds every call with a 20 s linked CTS /// (BrowserSessionService.PerCallTimeout) — the session invents no timeout of its own, it just From 342737083172372dea19ee7235b21c62f1e4357b Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 08:05:12 -0400 Subject: [PATCH 4/5] docs(reviews): close Driver.Sql-009 + Driver.Sql.Browser-001/002/003 -003 is a documented Won't Fix: the mitigating control is the ConfigEditor page policy on the only two surfaces that reach a browse session (/raw and /uns), there is no HTTP endpoint for browse, and PageAuthorizationGuardTests pins both pages to that exact policy. Recorded in docs/security.md under Control-Plane Authorization and pointed at from SqlDriverBrowser's class summary. --- code-reviews/Driver.Sql.Browser/findings.md | 29 +++++++++++++---- code-reviews/Driver.Sql/findings.md | 4 +-- docs/security.md | 32 +++++++++++++++++++ .../SqlDriverBrowser.cs | 7 ++++ 4 files changed, 63 insertions(+), 9 deletions(-) diff --git a/code-reviews/Driver.Sql.Browser/findings.md b/code-reviews/Driver.Sql.Browser/findings.md index 4e22bc63..3f2ecf75 100644 --- a/code-reviews/Driver.Sql.Browser/findings.md +++ b/code-reviews/Driver.Sql.Browser/findings.md @@ -11,7 +11,7 @@ | Review date | 2026-08-10 | | Commit reviewed | `b990adae` | | Status | Reviewed | -| Open findings | 3 | +| Open findings | 0 | ## Checklist coverage @@ -37,13 +37,17 @@ | Severity | Low | | Category | Correctness & logic bugs | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/SqlBrowseSession.cs:290` | -| Status | Open | +| 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:** _(open)_ +**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). --- @@ -54,13 +58,17 @@ | 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 | +| 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:** _(open)_ +**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. --- @@ -71,10 +79,17 @@ | Severity | Low | | Category | Security | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/SqlDriverBrowser.cs:262` | -| Status | Open | +| 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:** _(open)_ +**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. diff --git a/code-reviews/Driver.Sql/findings.md b/code-reviews/Driver.Sql/findings.md index a17eb89a..27dfb7d6 100644 --- a/code-reviews/Driver.Sql/findings.md +++ b/code-reviews/Driver.Sql/findings.md @@ -184,13 +184,13 @@ surface. | Severity | Low | | Category | Security | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriverProbe.cs:83` | -| Status | Open | +| Status | Resolved | **Description:** The probe surfaces the raw parser text on a malformed config ("Config JSON is invalid: " + `ex.Message`) where `SqlDriverBrowser.ParseDocument` deliberately withholds the identical message "because the malformed text may carry a connection string". The two read the same blob shape: the AdminUI Sql picker composes an ad-hoc document carrying a top-level `connectionString` literal, so a malformed paste is exactly the case that reaches both. `System.Text.Json`'s message quotes the offending token and the JSON path rather than the property value, so the current exposure is small — but the rule is stated absolutely elsewhere in this driver ("never `ex.Message` on an operator surface") and the probe is the one place that breaks it. **Recommendation:** Match the browser: report the failure class, not the parser's message. -**Resolution:** _(open)_ +**Resolution (2026-08-10):** Fixed in `073930e0`. The `JsonException`/`ArgumentNullException` catch now returns the browser's own wording — "Config JSON is not valid JSON. (The parser's message is withheld because the malformed text may carry a connection string.)" — and the class summary states the rule beside the existing "never the connection string" paragraph, so the two disciplines read as one. Pinned by `SqlDriverProbeTests.Probe_malformedConfig_withholdsTheParserMessage`, whose malformed input is a `connectionString` literal mis-quoted the way an operator's paste is (`Password=Sup3r;SecretTail`, unquoted `;`) so the parse fails partway *through* the credential. RED before the fix on `"LineNumber"` — the old message was `Config JSON is invalid: 'P' is an invalid start of a value. Path: $.connectionString | LineNumber: 0…`. The test asserts on the parser's structural tokens (`LineNumber`, `BytePositionInLine`) rather than only on the secret, because `System.Text.Json` quotes just the offending token: a leak-only assertion passes vacuously today and would go on passing if the message ever got *more* verbose. --- diff --git a/docs/security.md b/docs/security.md index a2bf5c0d..7942bbbf 100644 --- a/docs/security.md +++ b/docs/security.md @@ -719,6 +719,38 @@ Every routable AdminUI page carries an explicit **named-policy** `@attribute [Au In v2 the authentication + authorization stack is wired centrally by `AddOtOpcUaAuth` (`src/Server/ZB.MOM.WW.OtOpcUa.Security/ServiceCollectionExtensions.cs`), which also installs a `FallbackPolicy` that requires an authenticated user. Razor pages gate inline with the named-policy attribute above, e.g. `@attribute [Authorize(Policy = AdminUiPolicies.ConfigEditor)]`. Nav-menu sections hide via ``. +#### Address-picker browse is an outbound-connect capability (trust boundary) + +Worth stating plainly, because "browse a configured database" reads weaker than what it is: the AdminUI's +live address pickers **dial a target named in the browse form, from the AdminUI host, before anything is +persisted.** The `Sql` picker is the sharpest case — `SqlDriverBrowser.OpenAsync` +(`src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/SqlDriverBrowser.cs`) accepts an **ad-hoc +`connectionString` literal** an operator pastes for a database that has no `connectionStringRef` yet, and +hands it to the ADO.NET provider verbatim. That is an arbitrary host and port plus whatever the provider's +keyword set allows (`Microsoft.Data.SqlClient` alone offers `AttachDbFilename`, `Failover Partner`, +`Enlist`). The OPC UA client and MQTT pickers have the same shape against their own endpoint fields. + +**The control is the page policy, and it is admin-grade.** The only surfaces that reach a browse session +are `/raw` and `/uns`, both `@attribute [Authorize(Policy = AdminUiPolicies.ConfigEditor)]` — satisfied by +`Administrator` or `Designer` only. There is **no HTTP endpoint** for browse: `IBrowserSessionService` is a +scoped service consumed from those pages' Blazor circuits, so it inherits the page's policy and the +`FallbackPolicy` beneath it, and `PageAuthorizationGuardTests` fails the build if either page's attribute +drifts. A `Viewer` cannot open a picker; `AdminUiPolicies.DriverOperator` gates only the *live-ops* actions +inside one (Sparkplug rebirth), never the connect. + +Supporting hygiene, none of which is the control: + +- The pasted literal is **never persisted** — `SqlDriverConfigDto` has no such property, so the driver + factory would drop it; the picker composes a session-only document and the browser keeps no field. +- A literal supplied alongside a `connectionStringRef` **logs a warning** naming the shadowed ref (never + any connection text), so the override is visible rather than silent. +- Every failure on the open path is **redacted** (`SqlDriverBrowser.Sanitize`), and the provider's own + connection-string-parser message is withheld outright because it quotes connection-string text verbatim. +- `NodeEdit.razor` warns against pasting a literal into a *saved* `DriverConfig`, which would persist it. + +If a deployment needs the browse surface narrower than "anyone who may author config", the lever is the +page policy (and the LDAP group→role mapping behind it), not the browser. + ### Role grant source Admin reads `LdapGroupRoleMapping` rows from the Config DB (`src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Entities/LdapGroupRoleMapping.cs`) — the same pattern as the data-plane `NodeAcl` but scoped to Admin roles + (optionally) one cluster for multi-site fleets (a system-wide row, `IsSystemWide = true`, stacks additively with cluster-scoped rows). The `RoleGrants.razor` page lets `Administrator`s edit these mappings without leaving the UI. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/SqlDriverBrowser.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/SqlDriverBrowser.cs index e67d5bd8..349be1ae 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/SqlDriverBrowser.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/SqlDriverBrowser.cs @@ -26,6 +26,13 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser; /// the driver factory would drop it — which makes its presence proof that an operator typed it just now. /// A config carrying both logs a warning (naming the shadowed ref, never any connection text) so /// the override is visible rather than silent. +/// Trust boundary: this is an outbound-connect capability, gated by the AdminUI page policy. +/// Accepting a literal means dialling an arbitrary host and port — plus whatever the provider's keyword +/// set allows — from the AdminUI host, before anything is persisted. The control is that the only +/// surfaces reaching a browse session are /raw and /uns, both +/// [Authorize(Policy = AdminUiPolicies.ConfigEditor)] (Administrator or Designer), with no HTTP +/// endpoint for browse at all; the hygiene below is supporting, not the control. Recorded in +/// docs/security.md §"Address-picker browse is an outbound-connect capability". /// Credential hygiene is the point of this type. A connection string is a secret. It is /// passed to the provider and to nothing else: it never reaches a log line, an exception message, a field, /// or anything persisted. Because ADO.NET providers are free to echo connection-string content in their From cb7096ddff516898509a852aa22a68835fa1cf2d Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 08:05:21 -0400 Subject: [PATCH 5/5] docs(reviews): point the -003 Won't Fix at the commit that carries it --- code-reviews/Driver.Sql.Browser/findings.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/code-reviews/Driver.Sql.Browser/findings.md b/code-reviews/Driver.Sql.Browser/findings.md index 3f2ecf75..c7332b8b 100644 --- a/code-reviews/Driver.Sql.Browser/findings.md +++ b/code-reviews/Driver.Sql.Browser/findings.md @@ -85,7 +85,7 @@ Three tests, of which one was RED. `AttributesAsync_prefersATimeBearingColumn_ov **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: +**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`, 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.