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

[jeremydmiller]

Closes #390. Companion audit to weasel#416 on the SQL Server side, plus the fixes it turned up and the shared escaping helper the codebase was missing.

The audit

Nothing upstream is a sanitizing boundary, so nothing could be waved through on the grounds that it "came from Weasel". The issue asked specifically about this, and the answers are worse than the Postgres side:

SqlServerMigrator.AssertValidIdentifier	literally // Nothing yet — an empty method body
SchemaUtils.QuoteName	brackets only reserved keywords, escapes nothing, and returns a bare schema.table otherwise
DbObjectName / SqlServerObjectName	no validation at all

Good news first: SQL Server's managed tenant partitioning never puts a tenant id in an identifier position. Weasel.SqlServer.ManagedTenantPartitions allocates a compact integer ordinal and names partition objects from that, binding the tenant id as a parameter everywhere it touches the registry. The stored/second-order shape that bit Marten — a per-tenant sequence name read back out of a registry table and interpolated — structurally does not exist here.

Class (c): runtime data or a public-API argument in an escaping-required position

• AdvancedOperations.CleanAllDocumentsAsync / CleanAllEventDataAsync / CompletelyRemoveAllAsync interpolate table names read back out of INFORMATION_SCHEMA and sys.tables into [{table}]. Second-order by construction: the value is planted in the catalog by one call and fires from an unrelated later one.
• DocumentIndex.IndexName and DocumentForeignKey.ConstraintName are public-API arguments (Index(…, idx => idx.IndexName = …), ForeignKey<T>(…, fk => fk.ConstraintName = …)) that land in a string literal (the sys.indexes / sys.foreign_keys existence probe) and a bracketed identifier (CREATE INDEX / ADD CONSTRAINT) in the same statement pair. Two positions, two different escapes, neither applied — precisely the "easy to get half-right" case the issue calls out.
• DCB tag tables: RegisterTagType<TTag>(string tableSuffix) is a public-API argument that twelve sites composed into [{schema}].[pc_event_tag_{suffix}] by hand.

Class (b), fixed anyway because the divergence is a correctness bug independent of injection

DocumentIndex and DocumentForeignKey built COL_LENGTH('{schema}.{table}', …) unquoted while the ALTER TABLE beside it used [{schema}].[{table}] — the same object, two spellings, agreeing only until quoting is needed. DocumentTableEnsurer nests a qualified name inside a dynamic-SQL string literal. FlatTableProjection's SetValue writes its configured value into a literal unescaped.

Two sites were already correct and are left alone: PolecatDocumentStorage's tenant filter and SubClassPolecatStorage's doc_type filter already double embedded quotes.

The fix

A single Polecat.Internal.SqlEscaping (QuoteIdentifier / QualifiedName / Literal / LiteralBody) that every constructed name and literal routes through — deliberately with no "is this already escaped?" shortcut. weasel#416's postmortem is that such a test cannot be made safely from the shape of untrusted input, and skipping escaping for a quote-wrapped value is strictly worse than the missing escape it replaces.

Beyond escaping, the audit's other lesson was one builder per object name. Twelve independent compositions of the tag table name and five of pc_natural_key_* now go through EventGraph.TagTableName / NaturalKeyTableName, and DocumentMapping, PolecatDocumentSchemaResolver, MasterTableTenancy, HiloSequence (seven copies) and the EventGraph table-name properties all compose through the helper.

Cross-repo finding

SqlServerMigrator.AssertValidIdentifier being an empty method is worth its own Weasel issue — the issue asked whether it rejects ], [, " and ;, and it rejects nothing at all. Polecat does not rely on it either way after this change, but the Marten/Weasel side should know.

Tests

Unit coverage for each escape, including that a name bound for both positions composes the two in order, and that there is no already-quoted shortcut. Round-trip integration coverage per the issue's stated deliverable: tenant ids containing ' (carrying a '; DROP TABLE pc_events-- payload) and ] survive document store/load/LINQ and event append/fetch under conjoined tenancy with tenant isolation intact, and an index name containing both characters is created and its existence probe matches on a second schema apply — which is what proves the literal and identifier positions agree.

Full Polecat.Tests suite locally against dockerized SQL Server 2025, net10.0: 1647 total, 0 failed, 1644 passed, 3 skipped.

🤖 Generated with Claude Code

https://claude.ai/code/session_01G8tN8ApXiKhyVzia4iwmof