.Net: Fix single-quote escaping in OBJECT_ID and dynamic SQL string literals#13900
Conversation
In the SQL Server MEVD provider, table/schema/column names embedded inside SQL string literals (N'...') were not escaping single quotes. This could produce broken SQL when names contain single quotes. Added AppendTableNameInsideLiteral and AppendIdentifierInsideLiteral helpers that escape single quotes by doubling them, and updated the 5 call sites where identifiers appear inside string literals: - OBJECT_ID in CreateTable (ifNotExists check) - OBJECT_ID in full-text index PK lookup - Dynamic SQL table name, column names, and catalog name for full-text indexes Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 92%
✓ Correctness
This PR fixes a correctness bug where SQL identifiers containing single quotes were not properly escaped when embedded inside SQL string literals (N'..'). The fix introduces AppendTableNameInsideLiteral and AppendIdentifierInsideLiteral helper methods that first apply bracket escaping via the existing methods, then double any single quotes in the result. All five call sites that embed identifiers inside string literals (two OBJECT_ID calls, and three places in the dynamic full-text index SQL) are correctly updated. The existing non-literal call sites (e.g., CREATE TABLE, PRIMARY KEY) are correctly left unchanged. The new unit tests thoroughly cover the escaping behavior including edge cases with multiple quotes and brackets. The implementation is sound and the test expectations are verified correct.
✓ Security Reliability
This PR fixes a SQL injection vulnerability where identifiers containing single quotes (e.g., table name "ta'ble") could break out of N'...' string literals in OBJECT_ID() calls and dynamic SQL for full-text index creation. The fix introduces AppendTableNameInsideLiteral and AppendIdentifierInsideLiteral helper methods that double single quotes after bracket-quoting. All call sites where identifiers are embedded inside SQL string literals have been correctly updated, while direct SQL usages remain unchanged. The implementation is sound and well-tested.
✓ Test Coverage
The PR correctly fixes SQL injection/syntax issues where identifiers containing single quotes are used inside SQL string literals (N'..'). Two new helper methods (AppendTableNameInsideLiteral, AppendIdentifierInsideLiteral) are well-tested at the unit level, and a CreateTable integration test covers the OBJECT_ID path with single-quoted names. However, the full-text index code path in CreateTable (lines 127-144 of the source) has 4 call sites changed to use the new InsideLiteral variants, yet no test exercises CreateTable with IsFullTextIndexed properties—neither before nor after this PR. While the building-block unit tests provide good confidence, a CreateTable integration test with full-text properties and single-quoted names would verify the dynamic SQL assembly is correct end-to-end.
✓ Design Approach
The change correctly fixes a real bug: schema/table/column identifiers containing single quotes would break out of SQL string literals in OBJECT_ID(N'...') and SET
@ftSql= N'..' contexts. The fix adds two thin wrapper methods (AppendTableNameInsideLiteral / AppendIdentifierInsideLiteral) that call the existing bracket-quoting helpers and then post-process with Replace("'", "''"). The approach is sound: AppendIdentifier only injects [ and ] characters (escaping ] as ]), so the only single quotes in its output come from the user-supplied identifier text—making the post-processing Replace unambiguous and complete. Tests cover normal identifiers, identifiers with single quotes, and identifiers with brackets, providing good regression coverage. No blocking issues found.
Suggestions
- Consider adding a CreateTable test with IsFullTextIndexed properties and single quotes in schema/table names to exercise all 4 InsideLiteral call sites in the full-text index dynamic SQL path (lines 127-144 of SqlServerCommandBuilder.cs). The current CreateTable_WithSingleQuoteInName test only covers the OBJECT_ID path, and no existing test calls CreateTable with full-text indexed properties at all.
Automated review by roji's agents
There was a problem hiding this comment.
Pull request overview
Fixes SQL Server command generation for cases where bracket-quoted identifiers are embedded inside SQL string literals (e.g., OBJECT_ID(N'...'), dynamic full-text DDL), ensuring single quotes in names are correctly escaped (doubled) to prevent invalid SQL.
Changes:
- Added
AppendTableNameInsideLiteralandAppendIdentifierInsideLiteralhelpers to escape single quotes withinN'...'literals. - Updated
CreateTableSQL generation to use the new helpers at identifier-in-literal call sites (OBJECT_ID + full-text dynamic SQL). - Added unit tests for the new helpers and a command-text regression test for single quotes in schema/table names.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| dotnet/src/VectorData/SqlServer/SqlServerCommandBuilder.cs | Introduces *InsideLiteral helpers and applies them to OBJECT_ID and full-text dynamic SQL generation. |
| dotnet/test/VectorData/SqlServer.ConformanceTests/SqlServerCommandBuilderTests.cs | Adds coverage for quote-escaping helpers and validates CreateTable output when schema/table contain single quotes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
In the SQL Server MEVD provider, table/schema/column names embedded inside SQL string literals (
N'...') were not escaping single quotes. This could produce broken SQL when names contain single quotes.For example, a table named
it'swould generate:which is invalid SQL. The fix produces:
Changes
Added
AppendTableNameInsideLiteralandAppendIdentifierInsideLiteralhelper methods that escape single quotes by doubling them, and updated the 5 call sites where identifiers appear inside string literals:OBJECT_IDinCreateTable(ifNotExists check)OBJECT_IDin full-text index PK lookupTests
Added unit tests for
AppendTableNameInsideLiteral,AppendIdentifierInsideLiteral, and aCreateTable_WithSingleQuoteInNameintegration test. All 283 tests pass (unit + conformance).