From 580b77cecf578e0afa45e6457eebeea07bcb2965 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 17 Aug 2026 05:52:45 -0400 Subject: [PATCH] fix(sql-reaper): validate the GUID suffix and QUOTENAME the drop DDL; short reaper connect timeout; mint names from the prefix constant --- .../SchemaComplianceFixture.cs | 55 ++++++++++++++---- .../AdminUiSqlServerFixture.cs | 57 +++++++++++++++---- .../SqlTestDatabaseReaperTests.cs | 39 +++++++++++++ 3 files changed, 128 insertions(+), 23 deletions(-) diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/SchemaComplianceFixture.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/SchemaComplianceFixture.cs index 91bf2c5d..5e4c02b7 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/SchemaComplianceFixture.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/SchemaComplianceFixture.cs @@ -1,4 +1,3 @@ -using System.Data.Common; using System.Globalization; using Microsoft.Data.SqlClient; using Microsoft.EntityFrameworkCore; @@ -114,8 +113,11 @@ END"; /// single-character wildcard in T-SQL) — never widen it; /// /// -/// a name whose embedded timestamp does not parse is left alone, because an unparseable -/// name is by definition not one this code created; +/// a matching name is dropped only when its whole shape is one this code could have +/// minted — prefix, a parseable 14-digit UTC timestamp, _, then 32 hex digits. That is +/// belt and braces against second-order injection: the name is also passed as a +/// parameter through QUOTENAME rather than interpolated into DDL, but the shape +/// check independently guarantees a candidate cannot contain ] or '; /// /// /// only names strictly older than the cutoff are dropped. At 24 hours that is far beyond @@ -135,6 +137,7 @@ END"; internal static class SqlTestDatabaseReaper { private const string TimestampFormat = "yyyyMMddHHmmss"; + private const int ConnectTimeoutSeconds = 3; /// /// Drops every database matching whose embedded creation timestamp is @@ -180,7 +183,16 @@ internal static class SqlTestDatabaseReaper return reaped; } - /// Drops the named database, swallowing database errors — reaping is best-effort. + /// + /// Drops the named database, swallowing every failure — this is called unwrapped from + /// finally blocks, so it must be genuinely best-effort. + /// + /// + /// The database name is an identifier, which cannot be parameterised directly, so it rides + /// in as a parameter and QUOTENAME escapes it server-side. Interpolating it into the DDL + /// instead would make any attacker-chosen database name on this shared server a second-order + /// injection executed as sa. + /// /// A connection string pointed at master. /// The database to drop. /// when the statement ran without error. @@ -192,17 +204,27 @@ internal static class SqlTestDatabaseReaper conn.Open(); using var cmd = conn.CreateCommand(); - cmd.CommandText = $@" -IF DB_ID(N'{databaseName}') IS NOT NULL + + // The QUOTENAME result must be assembled into a variable first: EXEC() accepts only a + // concatenation of literals and variables, so the tempting one-liner + // EXEC('ALTER DATABASE ' + QUOTENAME(@name) + …) is a syntax error, not a subtle bug. + cmd.CommandText = @" +DECLARE @sql nvarchar(max); +IF DB_ID(@name) IS NOT NULL BEGIN - ALTER DATABASE [{databaseName}] SET SINGLE_USER WITH ROLLBACK IMMEDIATE; - DROP DATABASE [{databaseName}]; + SET @sql = N'ALTER DATABASE ' + QUOTENAME(@name) + N' SET SINGLE_USER WITH ROLLBACK IMMEDIATE; ' + + N'DROP DATABASE ' + QUOTENAME(@name) + N';'; + EXEC sp_executesql @sql; END"; + cmd.Parameters.Add(new SqlParameter("@name", databaseName)); + cmd.ExecuteNonQuery(); return true; } - catch (DbException) + catch (Exception) { + // Teardown is best-effort: a dropped connection here must not fail an otherwise-green run. + // The database name carries a timestamp, so a leaked one is reaped by the next run instead. return false; } } @@ -211,7 +233,15 @@ END"; { var names = new List(); - using var conn = new SqlConnection(masterConnectionString); + // A short connect timeout, because this runs BEFORE whatever the fixture does next: on an + // unreachable server the default ~15 s would simply be added to the latency the caller already pays + // — here, on top of the connect timeout Migrate() is about to spend proving the same thing. + var probe = new SqlConnectionStringBuilder(masterConnectionString) + { + ConnectTimeout = ConnectTimeoutSeconds, + }.ConnectionString; + + using var conn = new SqlConnection(probe); conn.Open(); using var cmd = conn.CreateCommand(); @@ -232,8 +262,11 @@ END"; var rest = databaseName.AsSpan(prefix.Length); - // Expect exactly "<14-digit timestamp>_" — anything else is not a name this code minted. + // Expect exactly "<14-digit timestamp>_<32 hex digits>" — anything else is not a name this code + // minted. Validating the GUID tail is what bounds the character set of every candidate name: a + // "N"-format GUID is hex only, so no candidate can carry a ']' or a quote into the drop statement. if (rest.Length < TimestampFormat.Length + 1 || rest[TimestampFormat.Length] != '_') return false; + if (!Guid.TryParseExact(rest[(TimestampFormat.Length + 1)..], "N", out _)) return false; return DateTime.TryParseExact( rest[..TimestampFormat.Length], diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AdminUiSqlServerFixture.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AdminUiSqlServerFixture.cs index 55bcea41..02eee6ac 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AdminUiSqlServerFixture.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AdminUiSqlServerFixture.cs @@ -1,4 +1,3 @@ -using System.Data.Common; using System.Globalization; using Microsoft.Data.SqlClient; using Microsoft.EntityFrameworkCore; @@ -79,7 +78,10 @@ public sealed class AdminUiSqlServerFixture : IDisposable // old enough to be certainly nobody's (once per process, and only after the probe answered). AdminUiSqlServer.ReapOrphanedTestDatabasesOnce(); - DatabaseName = $"OtOpcUaAdminUi_Test_{DateTime.UtcNow:yyyyMMddHHmmss}_{Guid.NewGuid():N}"; + // Mint from the same constant the reaper's LIKE anchors on — a rename that touched only one of the + // two would leave every future database unreapable, with nothing to signal it. + DatabaseName = + $"{AdminUiSqlServer.TestDatabasePrefix}{DateTime.UtcNow:yyyyMMddHHmmss}_{Guid.NewGuid():N}"; ConnectionString = AdminUiSqlServer.ConnectionStringFor(DatabaseName); try @@ -220,8 +222,11 @@ internal static class AdminUiSqlServer /// single-character wildcard in T-SQL) — never widen it; /// /// -/// a name whose embedded timestamp does not parse is left alone, because an unparseable -/// name is by definition not one this code created; +/// a matching name is dropped only when its whole shape is one this code could have +/// minted — prefix, a parseable 14-digit UTC timestamp, _, then 32 hex digits. That is +/// belt and braces against second-order injection: the name is also passed as a +/// parameter through QUOTENAME rather than interpolated into DDL, but the shape +/// check independently guarantees a candidate cannot contain ] or '; /// /// /// only names strictly older than the cutoff are dropped. At 24 hours that is far beyond @@ -236,6 +241,7 @@ internal static class AdminUiSqlServer internal static class SqlTestDatabaseReaper { private const string TimestampFormat = "yyyyMMddHHmmss"; + private const int ConnectTimeoutSeconds = 3; /// /// Drops every database matching whose embedded creation timestamp is @@ -281,7 +287,16 @@ internal static class SqlTestDatabaseReaper return reaped; } - /// Drops the named database, swallowing database errors — teardown is best-effort. + /// + /// Drops the named database, swallowing every failure — this is called unwrapped from + /// Dispose and from test finally blocks, so it must be genuinely best-effort. + /// + /// + /// The database name is an identifier, which cannot be parameterised directly, so it rides + /// in as a parameter and QUOTENAME escapes it server-side. Interpolating it into the DDL + /// instead would make any attacker-chosen database name on this shared server a second-order + /// injection executed as sa. + /// /// A connection string pointed at master. /// The database to drop. /// when the statement ran without error. @@ -293,16 +308,24 @@ internal static class SqlTestDatabaseReaper conn.Open(); using var cmd = conn.CreateCommand(); - cmd.CommandText = $@" -IF DB_ID(N'{databaseName}') IS NOT NULL + + // The QUOTENAME result must be assembled into a variable first: EXEC() accepts only a + // concatenation of literals and variables, so the tempting one-liner + // EXEC('ALTER DATABASE ' + QUOTENAME(@name) + …) is a syntax error, not a subtle bug. + cmd.CommandText = @" +DECLARE @sql nvarchar(max); +IF DB_ID(@name) IS NOT NULL BEGIN - ALTER DATABASE [{databaseName}] SET SINGLE_USER WITH ROLLBACK IMMEDIATE; - DROP DATABASE [{databaseName}]; + SET @sql = N'ALTER DATABASE ' + QUOTENAME(@name) + N' SET SINGLE_USER WITH ROLLBACK IMMEDIATE; ' + + N'DROP DATABASE ' + QUOTENAME(@name) + N';'; + EXEC sp_executesql @sql; END"; + cmd.Parameters.Add(new SqlParameter("@name", databaseName)); + cmd.ExecuteNonQuery(); return true; } - catch (DbException) + catch (Exception) { // Teardown is best-effort: a dropped connection here must not fail an otherwise-green run. // The database name carries a timestamp, so a leaked one is reaped by the next run instead. @@ -314,7 +337,14 @@ END"; { var names = new List(); - using var conn = new SqlConnection(masterConnectionString); + // A short connect timeout, because this runs BEFORE whatever the fixture does next: on an + // unreachable server the default ~15 s would simply be added to the latency the caller already pays. + var probe = new SqlConnectionStringBuilder(masterConnectionString) + { + ConnectTimeout = ConnectTimeoutSeconds, + }.ConnectionString; + + using var conn = new SqlConnection(probe); conn.Open(); using var cmd = conn.CreateCommand(); @@ -335,8 +365,11 @@ END"; var rest = databaseName.AsSpan(prefix.Length); - // Expect exactly "<14-digit timestamp>_" — anything else is not a name this code minted. + // Expect exactly "<14-digit timestamp>_<32 hex digits>" — anything else is not a name this code + // minted. Validating the GUID tail is what bounds the character set of every candidate name: a + // "N"-format GUID is hex only, so no candidate can carry a ']' or a quote into the drop statement. if (rest.Length < TimestampFormat.Length + 1 || rest[TimestampFormat.Length] != '_') return false; + if (!Guid.TryParseExact(rest[(TimestampFormat.Length + 1)..], "N", out _)) return false; return DateTime.TryParseExact( rest[..TimestampFormat.Length], diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/SqlTestDatabaseReaperTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/SqlTestDatabaseReaperTests.cs index 1523cef8..92d67675 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/SqlTestDatabaseReaperTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/SqlTestDatabaseReaperTests.cs @@ -1,5 +1,6 @@ using Microsoft.Data.SqlClient; using Shouldly; +using Xunit; namespace ZB.MOM.WW.OtOpcUa.AdminUI.Tests; @@ -14,6 +15,14 @@ namespace ZB.MOM.WW.OtOpcUa.AdminUI.Tests; /// someone later widening the age window, the LIKE, or the timestamp parse. It therefore drives /// the internal core with an explicit cutoff rather than waiting 24 hours. /// +/// +/// Joined to deliberately, though it never touches the +/// fixture: xunit builds a collection fixture before running the collection's tests and serialises them +/// within it, which is what keeps 's own startup reaper from +/// racing this one. Left in its own (parallel) collection, that reaper can drop the aged database this +/// test plants, between the plant and the assertion — observed as an intermittent failure. +/// +[Collection(nameof(AdminUiSqlServerCollection))] public sealed class SqlTestDatabaseReaperTests { [SqlServerFact] @@ -85,6 +94,36 @@ public sealed class SqlTestDatabaseReaperTests } } + [SqlServerFact] + public void Reap_leaves_an_aged_name_whose_suffix_is_not_a_guid_alone() + { + var master = AdminUiSqlServer.ConnectionStringFor("master"); + + // Aged enough to be reaped on the timestamp alone — only the suffix check saves it. That check is + // what bounds the character set of every candidate: without it a database named + // "_" + arbitrary text would be fed to the drop statement, and before the + // statement was parameterised that was a second-order injection running as sa. + var notAGuid = $"{AdminUiSqlServer.TestDatabasePrefix}20250101000000_notaguid"; + + CreateDatabase(master, notAGuid); + + try + { + var reaped = SqlTestDatabaseReaper.ReapOrphans( + master, + AdminUiSqlServer.TestDatabasePrefix, + DateTime.UtcNow - TimeSpan.FromHours(24)); + + reaped.ShouldNotContain(notAGuid); + DatabaseExists(master, notAGuid) + .ShouldBeTrue("only names shaped exactly as this code mints them are ever dropped"); + } + finally + { + SqlTestDatabaseReaper.TryDropDatabase(master, notAGuid); + } + } + private static void CreateDatabase(string masterConnectionString, string databaseName) { using var conn = new SqlConnection(masterConnectionString);