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);