fix(sql-reaper): validate the GUID suffix and QUOTENAME the drop DDL; short reaper connect timeout; mint names from the prefix constant

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;
 ///         </item>
 ///         <item>
-///             a name whose embedded timestamp does not parse is <b>left alone</b>, because an unparseable
+///             a matching name is dropped only when its <b>whole</b> shape is one this code could have
-///             name is by definition not one this code created;
+///             minted — prefix, a parseable 14-digit UTC timestamp, <c>_</c>, then 32 hex digits. That is
+///             belt <i>and</i> braces against second-order injection: the name is also passed as a
+///             <b>parameter</b> through <c>QUOTENAME</c> rather than interpolated into DDL, but the shape
+///             check independently guarantees a candidate cannot contain <c>]</c> or <c>'</c>;
 ///         </item>
 ///         <item>
 ///             only names <b>strictly older than the cutoff</b> 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;
 
     /// <summary>
     ///     Drops every database matching <paramref name="prefix"/> whose embedded creation timestamp is
@@ -180,7 +183,16 @@ internal static class SqlTestDatabaseReaper
         return reaped;
     }
 
-    /// <summary>Drops the named database, swallowing database errors — reaping is best-effort.</summary>
+    /// <summary>
+    ///     Drops the named database, swallowing every failure — this is called unwrapped from
+    ///     <c>finally</c> blocks, so it must be genuinely best-effort.
+    /// </summary>
+    /// <remarks>
+    ///     The database name is an <b>identifier</b>, which cannot be parameterised directly, so it rides
+    ///     in as a parameter and <c>QUOTENAME</c> 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 <c>sa</c>.
+    /// </remarks>
     /// <param name="masterConnectionString">A connection string pointed at <c>master</c>.</param>
     /// <param name="databaseName">The database to drop.</param>
     /// <returns><see langword="true"/> when the statement ran without error.</returns>
@@ -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(<string>) 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;
+    SET @sql = N'ALTER DATABASE ' + QUOTENAME(@name) + N' SET SINGLE_USER WITH ROLLBACK IMMEDIATE; ' +
-    DROP DATABASE [{databaseName}];
+               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<string>();
 
-        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>_<guid>" — 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],

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;
 ///         </item>
 ///         <item>
-///             a name whose embedded timestamp does not parse is <b>left alone</b>, because an unparseable
+///             a matching name is dropped only when its <b>whole</b> shape is one this code could have
-///             name is by definition not one this code created;
+///             minted — prefix, a parseable 14-digit UTC timestamp, <c>_</c>, then 32 hex digits. That is
+///             belt <i>and</i> braces against second-order injection: the name is also passed as a
+///             <b>parameter</b> through <c>QUOTENAME</c> rather than interpolated into DDL, but the shape
+///             check independently guarantees a candidate cannot contain <c>]</c> or <c>'</c>;
 ///         </item>
 ///         <item>
 ///             only names <b>strictly older than the cutoff</b> 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;
 
     /// <summary>
     ///     Drops every database matching <paramref name="prefix"/> whose embedded creation timestamp is
@@ -281,7 +287,16 @@ internal static class SqlTestDatabaseReaper
         return reaped;
     }
 
-    /// <summary>Drops the named database, swallowing database errors — teardown is best-effort.</summary>
+    /// <summary>
+    ///     Drops the named database, swallowing every failure — this is called unwrapped from
+    ///     <c>Dispose</c> and from test <c>finally</c> blocks, so it must be genuinely best-effort.
+    /// </summary>
+    /// <remarks>
+    ///     The database name is an <b>identifier</b>, which cannot be parameterised directly, so it rides
+    ///     in as a parameter and <c>QUOTENAME</c> 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 <c>sa</c>.
+    /// </remarks>
     /// <param name="masterConnectionString">A connection string pointed at <c>master</c>.</param>
     /// <param name="databaseName">The database to drop.</param>
     /// <returns><see langword="true"/> when the statement ran without error.</returns>
@@ -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(<string>) 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;
+    SET @sql = N'ALTER DATABASE ' + QUOTENAME(@name) + N' SET SINGLE_USER WITH ROLLBACK IMMEDIATE; ' +
-    DROP DATABASE [{databaseName}];
+               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<string>();
 
-        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>_<guid>" — 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],

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 <c>LIKE</c>, or the timestamp parse. It therefore drives
 ///     the internal core with an explicit cutoff rather than waiting 24 hours.
 /// </remarks>
+/// <remarks>
+///     Joined to <see cref="AdminUiSqlServerCollection"/> 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 <see cref="AdminUiSqlServerFixture"/>'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.
+/// </remarks>
+[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
+        // "<prefix><aged timestamp>_" + 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);