fix: escape interpolated identifiers and literals in constructed SQL (#390)

src/Polecat.Tests/Storage/quoted_identifier_round_trip_tests.cs

@@ -0,0 +1,169 @@
+using JasperFx.Events;
+using Microsoft.Data.SqlClient;
+using Polecat.Linq;
+using Polecat.Tests.Harness;
+using Shouldly;
+
+namespace Polecat.Tests.Storage;
+
+public class QuotedTenantDoc
+{
+    public Guid Id { get; set; }
+    public string Name { get; set; } = string.Empty;
+    public int Rank { get; set; }
+}
+
+public class QuotedNameDoc
+{
+    public Guid Id { get; set; }
+    public string Name { get; set; } = string.Empty;
+}
+
+public record QuotedThingHappened(string What);
+
+/// <summary>
+///     End-to-end coverage for the polecat#390 audit: values that reach a SQL identifier or
+///     string-literal position by way of runtime data or a public-API argument survive a round trip
+///     when they contain the characters that terminate those positions — <c>'</c> for a literal and
+///     <c>]</c> for a bracketed identifier.
+/// </summary>
+/// <remarks>
+///     These are the cases the audit classified as (c) — supplied at runtime or through a public API
+///     rather than fixed at compile time. Categories (a) and (b) are covered by the unit tests in
+///     <see cref="sql_escaping_tests" />.
+/// </remarks>
+[Collection("integration")]
+public class quoted_identifier_round_trip_tests : IntegrationContext
+{
+    // Both terminators, in one value, plus a payload that would be visible if escaping failed open.
+    private const string QuoteTenant = "o'brien'; DROP TABLE pc_events--";
+    private const string BracketTenant = "acme]corp";
+
+    public quoted_identifier_round_trip_tests(DefaultStoreFixture fixture) : base(fixture)
+    {
+    }
+
+    [Fact]
+    public async Task documents_round_trip_for_a_tenant_id_containing_a_quote_or_a_bracket()
+    {
+        await DropTableAsync("quoted_tenant_docs", "pc_doc_quotedtenantdoc");
+        await StoreOptions(opts =>
+        {
+            opts.DatabaseSchemaName = "quoted_tenant_docs";
+            opts.Events.TenancyStyle = TenancyStyle.Conjoined; // document tenancy is store-wide
+        });
+
+        var quoteDoc = new QuotedTenantDoc { Id = Guid.NewGuid(), Name = "Quoted", Rank = 1 };
+        var bracketDoc = new QuotedTenantDoc { Id = Guid.NewGuid(), Name = "Bracketed", Rank = 2 };
+
+        theSession.ForTenant(QuoteTenant).Store(quoteDoc);
+        theSession.ForTenant(BracketTenant).Store(bracketDoc);
+        await theSession.SaveChangesAsync(TestContext.Current.CancellationToken);
+
+        // Load: the tenant id reaches the read filter, which composes it into a string literal.
+        await using var quoteSession = theStore.QuerySession(new SessionOptions { TenantId = QuoteTenant });
+        (await quoteSession.LoadAsync<QuotedTenantDoc>(quoteDoc.Id, TestContext.Current.CancellationToken))
+            .ShouldNotBeNull()
+            .Name.ShouldBe("Quoted");
+
+        await using var bracketSession = theStore.QuerySession(new SessionOptions { TenantId = BracketTenant });
+        (await bracketSession.LoadAsync<QuotedTenantDoc>(bracketDoc.Id, TestContext.Current.CancellationToken))
+            .ShouldNotBeNull()
+            .Name.ShouldBe("Bracketed");
+
+        // And the tenants stay isolated — a broken escape would either error or leak across.
+        (await quoteSession.Query<QuotedTenantDoc>().ToListAsync(TestContext.Current.CancellationToken))
+            .Select(x => x.Name).ShouldBe(["Quoted"]);
+        (await bracketSession.LoadAsync<QuotedTenantDoc>(quoteDoc.Id, TestContext.Current.CancellationToken))
+            .ShouldBeNull();
+    }
+
+    [Fact]
+    public async Task events_round_trip_for_a_tenant_id_containing_a_quote_or_a_bracket()
+    {
+        await StoreOptions(opts =>
+        {
+            opts.DatabaseSchemaName = "quoted_tenant_events";
+            opts.Events.TenancyStyle = TenancyStyle.Conjoined;
+        });
+
+        var quoteStream = Guid.NewGuid();
+        var bracketStream = Guid.NewGuid();
+
+        theSession.ForTenant(QuoteTenant).Events.StartStream(quoteStream, new QuotedThingHappened("quoted"));
+        theSession.ForTenant(BracketTenant).Events.StartStream(bracketStream, new QuotedThingHappened("bracketed"));
+        await theSession.SaveChangesAsync(TestContext.Current.CancellationToken);
+
+        await using var quoteSession = theStore.QuerySession(new SessionOptions { TenantId = QuoteTenant });
+        var quoteEvents = await quoteSession.Events.FetchStreamAsync(
+            quoteStream, token: TestContext.Current.CancellationToken);
+        quoteEvents.Count.ShouldBe(1);
+        quoteEvents[0].Data.ShouldBeOfType<QuotedThingHappened>().What.ShouldBe("quoted");
+
+        await using var bracketSession = theStore.QuerySession(new SessionOptions { TenantId = BracketTenant });
+        var bracketEvents = await bracketSession.Events.FetchStreamAsync(
+            bracketStream, token: TestContext.Current.CancellationToken);
+        bracketEvents.Count.ShouldBe(1);
+        bracketEvents[0].Data.ShouldBeOfType<QuotedThingHappened>().What.ShouldBe("bracketed");
+
+        // The pc_events table is still there — proof the `'; DROP TABLE` payload stayed inert data.
+        (await ScalarAsync("SELECT OBJECT_ID('[quoted_tenant_events].[pc_events]')")).ShouldNotBeNull();
+    }
+
+    [Fact]
+    public async Task an_index_name_containing_a_bracket_or_a_quote_is_created_and_is_idempotent()
+    {
+        // IndexName is a public-API argument that lands in BOTH a string literal (the sys.indexes
+        // existence probe) and a bracketed identifier (CREATE INDEX). Applying the schema twice
+        // proves the two positions agree: if only one were escaped, the probe would never match its
+        // own index and the second apply would fail with "index already exists".
+        const string schema = "quoted_index_names";
+        const string indexName = "idx_qu]oted_o'brien";
+
+        await DropTableAsync(schema, "pc_doc_quotednamedoc");
+
+        // Document tables (and their indexes) are created lazily on first use by DocumentTableEnsurer,
+        // so each pass has to actually write. Two passes: the second re-runs the existence probe
+        // against the index the first one created.
+        for (var pass = 0; pass < 2; pass++)
+        {
+            await StoreOptions(opts =>
+            {
+                opts.DatabaseSchemaName = schema;
+                opts.Schema.For<QuotedNameDoc>().Index(x => x.Name, idx => idx.IndexName = indexName);
+            });
+
+            var doc = new QuotedNameDoc { Id = Guid.NewGuid(), Name = $"pass-{pass}" };
+            theSession.Store(doc);
+            await theSession.SaveChangesAsync(TestContext.Current.CancellationToken);
+
+            (await theSession.LoadAsync<QuotedNameDoc>(doc.Id, TestContext.Current.CancellationToken))
+                .ShouldNotBeNull().Name.ShouldBe($"pass-{pass}");
+        }
+
+        (await ScalarAsync(
+            $"""
+             SELECT COUNT(*) FROM sys.indexes
+             WHERE name = '{indexName.Replace("'", "''")}'
+               AND object_id = OBJECT_ID('[{schema}].[pc_doc_quotednamedoc]')
+             """)).ShouldBe(1);
+    }
+
+    private async Task<object?> ScalarAsync(string sql)
+    {
+        await using var conn = await OpenConnectionAsync();
+        await using var cmd = conn.CreateCommand();
+        cmd.CommandText = sql;
+        var result = await cmd.ExecuteScalarAsync(TestContext.Current.CancellationToken);
+        return result == DBNull.Value ? null : result;
+    }
+
+    private static async Task DropTableAsync(string schema, string table)
+    {
+        await using var conn = new SqlConnection(ConnectionSource.ConnectionString);
+        await conn.OpenAsync(TestContext.Current.CancellationToken);
+        await using var cmd = conn.CreateCommand();
+        cmd.CommandText = $"DROP TABLE IF EXISTS [{schema}].[{table}];";
+        await cmd.ExecuteNonQueryAsync(TestContext.Current.CancellationToken);
+    }
+}

src/Polecat.Tests/Storage/sql_escaping_tests.cs

@@ -0,0 +1,64 @@
+using Polecat.Internal;
+using Shouldly;
+
+namespace Polecat.Tests.Storage;
+
+/// <summary>
+///     Unit coverage for the shared escaping helper introduced by the polecat#390 audit. These are the
+///     two positions T-SQL requires escaping in, and the two mistakes the audit was looking for.
+/// </summary>
+public class sql_escaping_tests
+{
+    [Fact]
+    public void quote_identifier_doubles_an_embedded_closing_bracket()
+    {
+        // Without the doubling the bracket closes early and everything after it is parsed as SQL.
+        SqlEscaping.QuoteIdentifier("plain").ShouldBe("[plain]");
+        SqlEscaping.QuoteIdentifier("we]ird").ShouldBe("[we]]ird]");
+        SqlEscaping.QuoteIdentifier("a]; DROP TABLE x --").ShouldBe("[a]]; DROP TABLE x --]");
+    }
+
+    [Fact]
+    public void quote_identifier_leaves_quotes_and_dots_alone()
+    {
+        // A single quote is not special inside a bracketed identifier, and a dot must NOT be treated
+        // as a separator — [a.b] is one object named "a.b".
+        SqlEscaping.QuoteIdentifier("o'brien").ShouldBe("[o'brien]");
+        SqlEscaping.QuoteIdentifier("a.b").ShouldBe("[a.b]");
+    }
+
+    [Fact]
+    public void qualified_name_escapes_both_halves_independently()
+    {
+        SqlEscaping.QualifiedName("dbo", "pc_events").ShouldBe("[dbo].[pc_events]");
+        SqlEscaping.QualifiedName("sch]ema", "tab]le").ShouldBe("[sch]]ema].[tab]]le]");
+    }
+
+    [Fact]
+    public void literal_doubles_an_embedded_single_quote()
+    {
+        SqlEscaping.Literal("plain").ShouldBe("'plain'");
+        SqlEscaping.Literal("o'brien").ShouldBe("'o''brien'");
+        SqlEscaping.LiteralBody("o'brien").ShouldBe("o''brien");
+    }
+
+    [Fact]
+    public void literal_has_no_already_quoted_shortcut()
+    {
+        // weasel#416's postmortem: an "is this already escaped?" test cannot be made safely from the
+        // shape of untrusted input — a value that happens to start and end with a quote would skip
+        // escaping entirely, which is strictly worse than the missing escape it replaces. Formatting
+        // here is unconditional, so a quote-wrapped input is escaped like any other.
+        SqlEscaping.Literal("'sneaky'").ShouldBe("'''sneaky'''");
+    }
+
+    [Fact]
+    public void a_name_bound_for_both_positions_composes_the_two_escapes()
+    {
+        // The audit's specific trap: the same object name appears bare in `ALTER TABLE [s].[t]` and
+        // as a string in `OBJECT_ID('[s].[t]')`. Those need different escapes, applied in order.
+        var qualified = SqlEscaping.QualifiedName("sch'ema", "tab]le");
+        qualified.ShouldBe("[sch'ema].[tab]]le]");
+        SqlEscaping.Literal(qualified).ShouldBe("'[sch''ema].[tab]]le]'");
+    }
+}

src/Polecat/AdvancedOperations.cs

@@ -379,7 +379,10 @@ SELECT TABLE_NAME FROM INFORMATION_SCHEMA.TABLES
             foreach (var table in tables)
             {
                 await using var deleteCmd = conn.CreateCommand();
-                deleteCmd.CommandText = $"DELETE FROM [{schemaName}].[{table}];";
+                // #390: `table` is read back out of INFORMATION_SCHEMA rather than derived from
+                // configuration, so it is escaped as a bracketed identifier — an embedded `]` in a
+                // table name would otherwise close the bracket early (stored/second-order).
+                deleteCmd.CommandText = $"DELETE FROM {SqlEscaping.QualifiedName(schemaName, table)};";
                 await deleteCmd.ExecuteNonQueryAsync(ct);
             }
 
@@ -443,15 +446,17 @@ SELECT TABLE_NAME FROM INFORMATION_SCHEMA.TABLES
             foreach (var table in tables)
             {
                 await using var dropCmd = conn.CreateCommand();
-                dropCmd.CommandText = $"DROP TABLE IF EXISTS [{schemaName}].[{table}];";
+                // #390: `table` comes back out of INFORMATION_SCHEMA — escape it as an identifier.
+                dropCmd.CommandText = $"DROP TABLE IF EXISTS {SqlEscaping.QualifiedName(schemaName, table)};";
                 await dropCmd.ExecuteNonQueryAsync(ct);
             }
 
             // FlatTableProjection tables may live outside the pc_ prefix; drop them explicitly.
             foreach (var qualified in flatTableNames)
             {
                 await using var dropCmd = conn.CreateCommand();
-                dropCmd.CommandText = $"IF OBJECT_ID('{qualified}', 'U') IS NOT NULL DROP TABLE {qualified};";
+                dropCmd.CommandText =
+                    $"IF OBJECT_ID({SqlEscaping.Literal(qualified)}, 'U') IS NOT NULL DROP TABLE {qualified};";
                 await dropCmd.ExecuteNonQueryAsync(ct);
             }
         }, (connStr, schema, flatTables), token);
@@ -475,8 +480,11 @@ private static async Task DeleteFlatTablesAsync(SqlConnection conn, string[] qua
         foreach (var qualified in qualifiedNames)
         {
             await using var cmd = conn.CreateCommand();
-            // Guard against the table not existing yet (the projection ensures it lazily).
-            cmd.CommandText = $"IF OBJECT_ID('{qualified}', 'U') IS NOT NULL DELETE FROM {qualified};";
+            // Guard against the table not existing yet (the projection ensures it lazily). #390: the
+            // same name lands in a string-literal position and a bare identifier position, which need
+            // different escaping — only the literal one gets it.
+            cmd.CommandText =
+                $"IF OBJECT_ID({SqlEscaping.Literal(qualified)}, 'U') IS NOT NULL DELETE FROM {qualified};";
             await cmd.ExecuteNonQueryAsync(ct);
         }
     }
@@ -496,7 +504,8 @@ await _resilience.ExecuteAsync(static async (state, ct) =>
             await conn.OpenAsync(ct);
 
             await using var cmd = conn.CreateCommand();
-            cmd.CommandText = $"IF OBJECT_ID('{qualifiedTableName}', 'U') IS NOT NULL DELETE FROM {qualifiedTableName};";
+            cmd.CommandText =
+                $"IF OBJECT_ID({SqlEscaping.Literal(qualifiedTableName)}, 'U') IS NOT NULL DELETE FROM {qualifiedTableName};";
             await cmd.ExecuteNonQueryAsync(ct);
         }, (connStr, tableName), token);
     }
@@ -633,7 +642,8 @@ SELECT TABLE_NAME FROM INFORMATION_SCHEMA.TABLES
                 foreach (var table in nkTables)
                 {
                     await using var deleteCmd = conn.CreateCommand();
-                    deleteCmd.CommandText = $"DELETE FROM [{schemaName}].[{table}];";
+                    // #390: natural-key table names are read out of INFORMATION_SCHEMA — escape.
+                    deleteCmd.CommandText = $"DELETE FROM {SqlEscaping.QualifiedName(schemaName, table)};";
                     await deleteCmd.ExecuteNonQueryAsync(ct);
                 }
             }
@@ -643,19 +653,22 @@ SELECT TABLE_NAME FROM INFORMATION_SCHEMA.TABLES
             // store schema has been created (e.g. during bootstrap of a fresh database).
             await using (var cmd = conn.CreateCommand())
             {
-                cmd.CommandText = $"IF OBJECT_ID('{evtTable}', 'U') IS NOT NULL DELETE FROM {evtTable};";
+                cmd.CommandText =
+                    $"IF OBJECT_ID({SqlEscaping.Literal(evtTable)}, 'U') IS NOT NULL DELETE FROM {evtTable};";
                 await cmd.ExecuteNonQueryAsync(ct);
             }
 
             await using (var cmd = conn.CreateCommand())
             {
-                cmd.CommandText = $"IF OBJECT_ID('{strmTable}', 'U') IS NOT NULL DELETE FROM {strmTable};";
+                cmd.CommandText =
+                    $"IF OBJECT_ID({SqlEscaping.Literal(strmTable)}, 'U') IS NOT NULL DELETE FROM {strmTable};";
                 await cmd.ExecuteNonQueryAsync(ct);
             }
 
             await using (var cmd = conn.CreateCommand())
             {
-                cmd.CommandText = $"IF OBJECT_ID('{progTable}', 'U') IS NOT NULL DELETE FROM {progTable};";
+                cmd.CommandText =
+                    $"IF OBJECT_ID({SqlEscaping.Literal(progTable)}, 'U') IS NOT NULL DELETE FROM {progTable};";
                 await cmd.ExecuteNonQueryAsync(ct);
             }
         }, (connStr, schema, eventsTable, streamsTable, progressionTable, flatTables), token);

src/Polecat/DocumentStore.EventStore.cs

@@ -546,9 +546,12 @@ await Options.ResiliencePipeline.ExecuteAsync(static async (state, ct) =>
                     // rebuild it may not exist yet — guard the tenant-scoped delete accordingly.
                     // #234: single-tenant projection tables have no tenant_id column, so the delete
                     // is unscoped there (only the default tenant's rows exist).
+                    // #390: the same name occupies a string-literal position (OBJECT_ID) and a bare
+                    // identifier position, which take different escaping.
+                    var exists = $"IF OBJECT_ID({SqlEscaping.Literal(tableName)}, 'U') IS NOT NULL";
                     delDocs.CommandText = isConjoined
-                        ? $"IF OBJECT_ID('{tableName}', 'U') IS NOT NULL DELETE FROM {tableName} WHERE tenant_id = @tenant;"
-                        : $"IF OBJECT_ID('{tableName}', 'U') IS NOT NULL DELETE FROM {tableName};";
+                        ? $"{exists} DELETE FROM {tableName} WHERE tenant_id = @tenant;"
+                        : $"{exists} DELETE FROM {tableName};";
                     if (isConjoined) delDocs.Parameters.AddVarChar("@tenant", tenant);
                     await delDocs.ExecuteNonQueryAsync(ct);
                 }
@@ -611,8 +614,7 @@ private async Task TeardownProjectionStateAsync(IEventDatabase database, string
             // same as for the doc tables, so it rides the same loop below.
             if (source is JasperFx.Events.Aggregation.IAggregateProjection { NaturalKeyDefinition: not null } natural)
             {
-                var aggregateName = natural.NaturalKeyDefinition.AggregateType.Name.ToLowerInvariant();
-                tables.Add($"[{Events.DatabaseSchemaName}].[pc_natural_key_{aggregateName}]");
+                tables.Add(Events.NaturalKeyTableName(natural.NaturalKeyDefinition.AggregateType));
             }
 
             publishedTableNames = tables.ToArray();

src/Polecat/DocumentStore.EventStoreExplorer.cs

@@ -210,7 +210,6 @@ async IAsyncEnumerable<EventRecord> IEventStore.QueryByTagsAsync(
         ArgumentNullException.ThrowIfNull(tags);
         if (tags.Count == 0) yield break;
 
-        var schema = Events.DatabaseSchemaName;
         var options = Events.EventOptions;
         var registered = Events.TagTypes;
 
@@ -243,7 +242,7 @@ async IAsyncEnumerable<EventRecord> IEventStore.QueryByTagsAsync(
             // value arrives as a string from the explorer, so it's compared case-insensitively via nvarchar —
             // matching whatever native type (uniqueidentifier, etc.) the tag's value column stores.
             sb.Append(
-                $"e.seq_id IN (SELECT seq_id FROM [{schema}].[pc_event_tag_{registration.TableSuffix}] WHERE LOWER(CONVERT(nvarchar(4000), value)) = LOWER(@tag_value_{idx}))");
+                $"e.seq_id IN (SELECT seq_id FROM {Events.TagTableName(registration)} WHERE LOWER(CONVERT(nvarchar(4000), value)) = LOWER(@tag_value_{idx}))");
             cmd.Parameters.AddWithValue($"@tag_value_{idx}", tagValue);
             idx++;
         }