From da1d22a5ffbe5e922ca6aa3086b9667dba8a2ed1 Mon Sep 17 00:00:00 2001 From: "Jeremy D. Miller" Date: Fri, 31 Jul 2026 17:19:30 -0500 Subject: [PATCH 1/5] feat(core): shared identifier checks for the provider migrators (weasel#416) b4ffbe4 hardened PostgresqlMigrator.AssertValidIdentifier and left the other four providers as they were: SqlServerMigrator's body was "// Nothing yet", Oracle and MySql checked length only, Sqlite null/whitespace and length. Bringing all four into line means writing the same loop four times, so it goes here instead. IdentifierValidation.FindProblem takes the name and the characters that are unsafe for that provider specifically, and returns why the name was rejected -- phrased to follow "because" -- or null. The provider keeps what is genuinely its own: the delimiter set, the length limit, and the exception type it has always thrown. Two rules are universal and so are not passed in: * ';' ends the statement and starts another. * '\'' closes a string literal, and object names do reach literals on every provider -- SQL Server's IF OBJECT_ID('...'), Oracle's WHERE table_name = '...' inside the anonymous PL/SQL block it wraps DDL in, SQLite's pragma_table_info('...'). MySQL parameterises its introspection, so there the rule is uniformity rather than a live hole. Whitespace is rejected in full rather than just the literal space character, as on PostgreSQL, so a newline cannot introduce a '--' comment into an unquoted name. PostgresqlMigrator is deliberately not refactored onto this. Its rules are the same minus the single quote; folding it in would be a no-behaviour-change edit to code shipped in 9.23.0, and this change is already touching four providers. Co-Authored-By: Claude Opus 5 --- .../IdentifierValidationTests.cs | 81 ++++++++++++++++++ src/Weasel.Core/IdentifierValidation.cs | 85 +++++++++++++++++++ 2 files changed, 166 insertions(+) create mode 100644 src/Weasel.Core.Tests/IdentifierValidationTests.cs create mode 100644 src/Weasel.Core/IdentifierValidation.cs diff --git a/src/Weasel.Core.Tests/IdentifierValidationTests.cs b/src/Weasel.Core.Tests/IdentifierValidationTests.cs new file mode 100644 index 0000000..bea3c20 --- /dev/null +++ b/src/Weasel.Core.Tests/IdentifierValidationTests.cs @@ -0,0 +1,81 @@ +using Shouldly; +using Xunit; + +namespace Weasel.Core.Tests; + +public class IdentifierValidationTests +{ + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public void null_empty_and_all_whitespace_are_rejected(string? name) + { + IdentifierValidation.FindProblem(name, "\"").ShouldBe("it is null, empty, or entirely whitespace"); + } + + [Theory] + [InlineData("us ers")] + [InlineData("us\ters")] + [InlineData("us\ners")] + [InlineData("us\rers")] + public void all_whitespace_characters_are_rejected_not_just_the_space(string name) + { + IdentifierValidation.FindProblem(name, "\"").ShouldBe("it contains whitespace"); + } + + /// + /// The semicolon and the single quote are unsafe for every provider, so they are checked whatever + /// the caller passes as its own unsafe set: a ';' starts a new statement, and object names reach + /// string literals on all of them via the existence checks and introspection queries. + /// + [Theory] + [InlineData("us;ers", "it contains a semicolon")] + [InlineData("us'ers", "it contains a single quote")] + public void the_universal_characters_are_rejected_without_being_asked_for(string name, string expected) + { + IdentifierValidation.FindProblem(name, "").ShouldBe(expected); + } + + [Theory] + [InlineData("us\"ers", "\"", "it contains a double quote")] + [InlineData("us`ers", "`", "it contains a backtick")] + [InlineData("us]ers", "[]", "it contains a closing square bracket")] + [InlineData("[users]", "[]", "it contains an opening square bracket")] + [InlineData("users\\", "\\", "it contains a backslash")] + [InlineData("us~ers", "~", "it contains the character '~'")] + public void the_provider_specific_characters_are_named_in_the_reason( + string name, + string unsafeCharacters, + string expected + ) + { + IdentifierValidation.FindProblem(name, unsafeCharacters).ShouldBe(expected); + } + + /// + /// A character one provider delimits with is not necessarily unsafe for another -- a backtick is + /// nothing special to PostgreSQL or SQL Server -- so the set has to stay per-provider. + /// + [Theory] + [InlineData("us`ers", "\"")] + [InlineData("us\"ers", "`")] + [InlineData("us]ers", "\"")] + public void characters_outside_the_supplied_set_pass(string name, string unsafeCharacters) + { + IdentifierValidation.FindProblem(name, unsafeCharacters).ShouldBeNull(); + } + + [Theory] + [InlineData("mt_doc_user")] + [InlineData("users$1")] + [InlineData("_leading_underscore")] + [InlineData("MixedCaseName")] + [InlineData("naïve_café")] + [InlineData("table-with-dashes")] + [InlineData("mt_doc_target.p_tenant_one")] + public void ordinary_names_pass(string name) + { + IdentifierValidation.FindProblem(name, "\"[]`").ShouldBeNull(); + } +} diff --git a/src/Weasel.Core/IdentifierValidation.cs b/src/Weasel.Core/IdentifierValidation.cs new file mode 100644 index 0000000..8480063 --- /dev/null +++ b/src/Weasel.Core/IdentifierValidation.cs @@ -0,0 +1,85 @@ +namespace Weasel.Core; + +/// +/// The identifier checks every provider needs, factored out so each provider's +/// only has to supply what is specific to it: the +/// characters its own dialect delimits with, its length limit, and the exception type it has always +/// thrown (weasel#416). +/// +/// +/// +/// is the only identifier check in the stack -- +/// and its provider-specific subclasses do no validation of their own -- +/// and the migration path runs every schema object's name through it +/// (DatabaseBase.ApplyAllConfiguredChangesToDatabaseAsync and +/// DatabaseBase.generateOrUpdateFeature). So it has to reject the characters that let a name +/// escape the statement it is written into. +/// +/// +/// Two of those are the same for everyone. A ; ends the statement and starts another. A +/// ' closes a string literal, and object names do reach string literals on every provider -- +/// the existence checks and introspection queries interpolate them (SQL Server's +/// IF OBJECT_ID('...'), Oracle's WHERE table_name = '...' inside an anonymous PL/SQL +/// block, SQLite's pragma_table_info('...')). Whitespace is rejected in full rather than just +/// the literal space character, so that a newline cannot introduce a -- comment into an +/// unquoted name. +/// +/// +/// The rest is per-provider, because the character that closes an identifier is not: SQL Server +/// delimits with [...] as well as "...", MySQL with backticks, Oracle and SQLite with +/// ". Weasel's quoting helpers do not double an embedded delimiter (SQLite's +/// SchemaUtils.QuoteName does, but only quotes at all for keywords, spaces, dashes and +/// leading digits), so a name carrying one does not stay inside its own quotes. +/// +/// +public static class IdentifierValidation +{ + /// + /// Returns why is unsafe to write into DDL, phrased to follow "because", + /// or null when it passes. Length is deliberately not checked here -- the limit and the + /// exception that reports it differ per provider. + /// + /// The identifier to check. Null, empty and all-whitespace are rejected. + /// + /// The characters that are unsafe for this provider on top of the universal ones: its identifier + /// delimiters, plus anything else that can break out of the context a name lands in -- MySQL's + /// backslash escape, for instance. + /// + public static string? FindProblem(string? name, string unsafeCharacters) + { + if (string.IsNullOrWhiteSpace(name)) + { + return "it is null, empty, or entirely whitespace"; + } + + foreach (var c in name) + { + if (char.IsWhiteSpace(c)) + { + return "it contains whitespace"; + } + + if (c is ';' or '\'' || unsafeCharacters.Contains(c)) + { + return $"it contains {Describe(c)}"; + } + } + + return null; + } + + private static string Describe(char c) + { + return c switch + { + ';' => "a semicolon", + '\'' => "a single quote", + '"' => "a double quote", + '`' => "a backtick", + '[' => "an opening square bracket", + ']' => "a closing square bracket", + '\\' => "a backslash", + _ => $"the character '{c}'" + }; + } +} From 5347fd6ae6efbaaa325fe9d2055911a1d7d78889 Mon Sep 17 00:00:00 2001 From: "Jeremy D. Miller" Date: Fri, 31 Jul 2026 17:19:47 -0500 Subject: [PATCH 2/5] fix(sqlserver): validate identifiers instead of nothing at all (weasel#416) AssertValidIdentifier was an empty method body -- "// Nothing yet" -- and it is the only identifier check in the stack: DbObjectName and SqlServerObjectName do not validate, and DatabaseBase runs every schema object's name through the migrator on the way to DDL. So this provider accepted any name at all. It now rejects, via IdentifierValidation.FindProblem: * ']', which closes a [...] delimited identifier, and '[' with it so that an already-bracketed name is caught rather than bracketed a second time. * '"', which closes a quoted identifier under QUOTED_IDENTIFIER ON. SchemaUtils .QuoteName brackets reserved keywords only and doubles nothing. * '\'', which closes a string literal. Object names reach literals here in the existence checks and introspection -- Table.WriteCreateStatement's IF OBJECT_ID('{Identifier}'), StoredProcedure's sys.objects.name = '{...}', TableColumn's OBJECT_ID('{parent.Identifier}'). * ';', whitespace anywhere, and null/empty. A length limit is new too: sysname is nvarchar(128), so 128 passes and 129 does not. MaxIdentifierLength is settable for parity with PostgresqlMigrator .NameDataLength. InvalidOperationException rather than a new typed exception -- the provider has no existing identifier exception to extend, and adding one across four providers is a wider API change than this needs to be. Behaviour change for downstream consumers: a name with a space in it used to reach DDL and would have worked when bracketed, and no longer does. Nothing in the suites generates such a name. Full Weasel.SqlServer suite green against the azure-sql-edge container (376 passed, 8 pre-existing skips), plus Weasel.EntityFrameworkCore (106 passed, 1 skip) and Weasel.CommandLine (23) as the nearest downstream consumers. Co-Authored-By: Claude Opus 5 --- .../SqlServerMigratorTests.cs | 87 +++++++++++++++++++ src/Weasel.SqlServer/SqlServerMigrator.cs | 33 ++++++- 2 files changed, 119 insertions(+), 1 deletion(-) diff --git a/src/Weasel.SqlServer.Tests/SqlServerMigratorTests.cs b/src/Weasel.SqlServer.Tests/SqlServerMigratorTests.cs index 43d23bb..d710df7 100644 --- a/src/Weasel.SqlServer.Tests/SqlServerMigratorTests.cs +++ b/src/Weasel.SqlServer.Tests/SqlServerMigratorTests.cs @@ -29,6 +29,93 @@ public void create_table_returns_sql_server_table() table.Identifier.ShouldBe(identifier); } + /// + /// weasel#416. AssertValidIdentifier is the only identifier check in the stack, and this provider's + /// was an empty method body until now, so it has to reject the characters that let a name escape the + /// statement it is written into. SQL Server delimits identifiers with [...] as well as + /// "...", so ] matters alongside "; a ' closes a string literal, which is + /// where names land in the existence checks (IF OBJECT_ID('...')); a ; starts a new + /// statement. + /// + [Theory] + [InlineData("users\"", "a trailing double quote")] + [InlineData("\"users", "a leading double quote")] + [InlineData("us\"ers", "an embedded double quote")] + [InlineData("\"users\"", "a fully quote-wrapped name")] + [InlineData("users]", "a bracket that closes a delimited identifier")] + [InlineData("[users]", "a fully bracket-wrapped name")] + [InlineData("us]ers", "an embedded closing bracket")] + [InlineData("users]; drop table users; --", "a bracket-and-semicolon payload")] + [InlineData("users'", "a trailing single quote")] + [InlineData("us'ers", "an embedded single quote")] + [InlineData("users'); drop table users; --", "a quote-and-semicolon payload")] + [InlineData("users;", "a trailing semicolon")] + [InlineData("us;ers", "an embedded semicolon")] + public void assert_identifier_rejects_quote_bracket_and_semicolon(string name, string description) + { + var migrator = new SqlServerMigrator(); + + Should.Throw( + () => migrator.AssertValidIdentifier(name), + $"Expected {description} to be rejected"); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + [InlineData("us ers")] + [InlineData("us\ters")] + [InlineData("us\ners")] + [InlineData("us\rers")] + [InlineData("users\n-- the rest of this statement is now a comment")] + public void assert_identifier_rejects_null_empty_and_whitespace(string? name) + { + var migrator = new SqlServerMigrator(); + + Should.Throw(() => migrator.AssertValidIdentifier(name!)); + } + + [Fact] + public void assert_identifier_rejects_names_past_the_sysname_limit() + { + var migrator = new SqlServerMigrator(); + + Should.NotThrow(() => migrator.AssertValidIdentifier(new string('a', 128))); + Should.Throw(() => migrator.AssertValidIdentifier(new string('a', 129))); + } + + [Fact] + public void invalid_identifier_message_says_which_rule_was_broken() + { + var migrator = new SqlServerMigrator(); + + var ex = Should.Throw(() => migrator.AssertValidIdentifier("us]ers")); + + ex.Message.ShouldContain("us]ers"); + ex.Message.ShouldContain("closing square bracket"); + } + + /// + /// Names Weasel and its consumers actually generate must keep working -- the tightening is aimed at + /// a handful of characters, not at narrowing the identifier grammar. + /// + [Theory] + [InlineData("mt_doc_user")] + [InlineData("mt_stream")] + [InlineData("mt_doc_user_hilo")] + [InlineData("users$1")] + [InlineData("_leading_underscore")] + [InlineData("MixedCaseName")] + [InlineData("naïve_café")] + [InlineData("table-with-dashes")] + [InlineData("#temp_table")] + [InlineData("mt_doc_target.p_tenant_one")] + public void assert_identifier_still_accepts_ordinary_names(string name) + { + Should.NotThrow(() => new SqlServerMigrator().AssertValidIdentifier(name)); + } + [Fact] public async Task can_ensure_database_that_does_not_exist() { diff --git a/src/Weasel.SqlServer/SqlServerMigrator.cs b/src/Weasel.SqlServer/SqlServerMigrator.cs index b6cedae..9299f43 100644 --- a/src/Weasel.SqlServer/SqlServerMigrator.cs +++ b/src/Weasel.SqlServer/SqlServerMigrator.cs @@ -137,9 +137,40 @@ public override string ToExecuteScriptLine(string scriptName) return $":r {scriptName}"; } + /// + /// The characters SQL Server delimits identifiers with. ] is what closes a + /// [...] delimited identifier and " what closes a quoted one (SQL Server accepts both, + /// the latter under QUOTED_IDENTIFIER ON); [ is rejected alongside ] so that an + /// already-bracketed name is caught rather than being bracketed a second time. + /// + private const string UnsafeIdentifierCharacters = "[]\""; + + /// + /// SQL Server's identifier length limit -- sysname is nvarchar(128), so anything + /// longer is rejected by the server itself. + /// + public int MaxIdentifierLength { get; set; } = 128; + + /// + /// Validates a database object name before it is written into DDL. See + /// for why each rule is here; this method had no body at all + /// before weasel#416. + /// + /// The name cannot be safely written into DDL. public override void AssertValidIdentifier(string name) { - // Nothing yet + var problem = IdentifierValidation.FindProblem(name, UnsafeIdentifierCharacters); + if (problem != null) + { + throw new InvalidOperationException( + $"SQL Server identifier '{name}' is not valid because {problem}."); + } + + if (name.Length > MaxIdentifierLength) + { + throw new InvalidOperationException( + $"SQL Server identifiers cannot exceed {MaxIdentifierLength} characters. '{name}' is {name.Length} characters."); + } } private static async Task createSchemas( From 352402c41e5c3274ec11dc5cd627ce1ebbfded2d Mon Sep 17 00:00:00 2001 From: "Jeremy D. Miller" Date: Fri, 31 Jul 2026 17:19:47 -0500 Subject: [PATCH 3/5] fix(oracle): reject quote, semicolon and whitespace in AssertValidIdentifier (weasel#416) The check was length only, and it threw NullReferenceException on a null name rather than saying anything useful. It is the only identifier check in the stack -- DbObjectName and OracleObjectName do not validate -- so '"', '\'' and ';' all went through to DDL. Now rejected, via IdentifierValidation.FindProblem: '"' (closes a quoted identifier; SchemaUtils.QuoteName quotes reserved keywords only and doubles nothing), '\'', ';', whitespace anywhere, and null/empty. The length limit is unchanged at 128, now settable via MaxIdentifierLength. The single quote matters more here than elsewhere: Weasel wraps Oracle DDL in an anonymous PL/SQL block and executes it with EXECUTE IMMEDIATE, so the whole statement -- object name included -- sits inside a string literal, and the existence checks interpolate names into WHERE table_name = '...' besides. InvalidOperationException is kept as the exception type, so existing callers that catch it still do. Full Weasel.Oracle suite green against the oracle-free container (215 passed). Co-Authored-By: Claude Opus 5 --- .../OracleMigratorTests.cs | 82 +++++++++++++++++++ src/Weasel.Oracle/OracleMigrator.cs | 33 +++++++- 2 files changed, 113 insertions(+), 2 deletions(-) diff --git a/src/Weasel.Oracle.Tests/OracleMigratorTests.cs b/src/Weasel.Oracle.Tests/OracleMigratorTests.cs index cc649b0..b1487e2 100644 --- a/src/Weasel.Oracle.Tests/OracleMigratorTests.cs +++ b/src/Weasel.Oracle.Tests/OracleMigratorTests.cs @@ -28,6 +28,88 @@ public void create_table_returns_oracle_table() table.Identifier.ShouldBe(identifier); } + /// + /// weasel#416. AssertValidIdentifier is the only identifier check in the stack, and this provider's + /// checked length only until now, so it has to reject the characters that let a name escape the + /// statement it is written into. A " closes a quoted identifier and a ; starts a new + /// statement. The ' matters especially here: Weasel wraps Oracle DDL in an anonymous PL/SQL + /// block and runs it via EXECUTE IMMEDIATE, so the name is written inside a string literal. + /// + [Theory] + [InlineData("USERS\"", "a trailing double quote")] + [InlineData("\"USERS", "a leading double quote")] + [InlineData("US\"ERS", "an embedded double quote")] + [InlineData("\"USERS\"", "a fully quote-wrapped name")] + [InlineData("USERS\"; DROP TABLE USERS; --", "a quote-and-semicolon payload")] + [InlineData("USERS'", "a trailing single quote")] + [InlineData("US'ERS", "an embedded single quote")] + [InlineData("USERS'; EXECUTE IMMEDIATE 'DROP TABLE USERS", "a literal-breaking payload")] + [InlineData("USERS;", "a trailing semicolon")] + [InlineData("US;ERS", "an embedded semicolon")] + public void assert_identifier_rejects_quote_and_semicolon(string name, string description) + { + var migrator = new OracleMigrator(); + + Should.Throw( + () => migrator.AssertValidIdentifier(name), + $"Expected {description} to be rejected"); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + [InlineData("US ERS")] + [InlineData("US\tERS")] + [InlineData("US\nERS")] + [InlineData("US\rERS")] + [InlineData("USERS\n-- the rest of this statement is now a comment")] + public void assert_identifier_rejects_null_empty_and_whitespace(string? name) + { + var migrator = new OracleMigrator(); + + Should.Throw(() => migrator.AssertValidIdentifier(name!)); + } + + [Fact] + public void assert_identifier_rejects_names_past_the_length_limit() + { + var migrator = new OracleMigrator(); + + Should.NotThrow(() => migrator.AssertValidIdentifier(new string('A', 128))); + Should.Throw(() => migrator.AssertValidIdentifier(new string('A', 129))); + } + + [Fact] + public void invalid_identifier_message_says_which_rule_was_broken() + { + var migrator = new OracleMigrator(); + + var ex = Should.Throw(() => migrator.AssertValidIdentifier("US\"ERS")); + + ex.Message.ShouldContain("US\"ERS"); + ex.Message.ShouldContain("double quote"); + } + + /// + /// Names Weasel and its consumers actually generate must keep working -- the tightening is aimed at + /// a handful of characters, not at narrowing the identifier grammar. + /// + [Theory] + [InlineData("MT_DOC_USER")] + [InlineData("MT_STREAM")] + [InlineData("mt_doc_user_hilo")] + [InlineData("USERS$1")] + [InlineData("_LEADING_UNDERSCORE")] + [InlineData("MixedCaseName")] + [InlineData("naïve_café")] + [InlineData("TABLE-WITH-DASHES")] + [InlineData("MT_DOC_TARGET.P_TENANT_ONE")] + public void assert_identifier_still_accepts_ordinary_names(string name) + { + Should.NotThrow(() => new OracleMigrator().AssertValidIdentifier(name)); + } + [Fact] public async Task ensure_database_is_idempotent() { diff --git a/src/Weasel.Oracle/OracleMigrator.cs b/src/Weasel.Oracle/OracleMigrator.cs index 623f184..e15442d 100644 --- a/src/Weasel.Oracle/OracleMigrator.cs +++ b/src/Weasel.Oracle/OracleMigrator.cs @@ -116,11 +116,40 @@ public override string ToExecuteScriptLine(string scriptName) return $"@{scriptName}"; } + /// + /// The character Oracle delimits identifiers with. A " closes a quoted identifier, and + /// does not double an embedded one. + /// + private const string UnsafeIdentifierCharacters = "\""; + + /// + /// Oracle's identifier length limit (12.2 and later). + /// + public int MaxIdentifierLength { get; set; } = 128; + + /// + /// Validates a database object name before it is written into DDL. See + /// for why each rule is here; before weasel#416 this checked + /// length only, and threw on a null name. + /// + /// + /// The single quote matters more on Oracle than elsewhere: Weasel wraps Oracle DDL in an anonymous + /// PL/SQL block and runs it through EXECUTE IMMEDIATE, so the whole statement -- object name + /// included -- is written inside a string literal. + /// + /// The name cannot be safely written into DDL. public override void AssertValidIdentifier(string name) { - if (name.Length > 128) + var problem = IdentifierValidation.FindProblem(name, UnsafeIdentifierCharacters); + if (problem != null) + { + throw new InvalidOperationException($"Oracle identifier '{name}' is not valid because {problem}."); + } + + if (name.Length > MaxIdentifierLength) { - throw new InvalidOperationException($"Oracle identifiers cannot exceed 128 characters. '{name}' is {name.Length} characters."); + throw new InvalidOperationException( + $"Oracle identifiers cannot exceed {MaxIdentifierLength} characters. '{name}' is {name.Length} characters."); } } From 1ec63fb711cce15ceb4d3d5d4a6c36b0a567f99a Mon Sep 17 00:00:00 2001 From: "Jeremy D. Miller" Date: Fri, 31 Jul 2026 17:20:00 -0500 Subject: [PATCH 4/5] fix(mysql): reject backtick, quote, backslash and semicolon in AssertValidIdentifier (weasel#416) The check was length only, and it threw NullReferenceException on a null name. As on the other providers it is the only identifier check in the stack, so a name carrying a delimiter reached DDL intact. Now rejected, via IdentifierValidation.FindProblem: * '`', the delimiter. SchemaUtils.QuoteName wraps every name in backticks and does not double an embedded one, so a name carrying one does not stay inside its own quotes. * '"', which delimits identifiers under ANSI_QUOTES and string literals otherwise. * '\\', an escape character inside MySQL string literals unless NO_BACKSLASH_ESCAPES is set -- a trailing one swallows the closing quote of the literal a name is written into. * '\'', ';', whitespace anywhere, and null/empty. MySQL is the one provider of the four whose introspection parameterises the table name (Table.FetchExisting binds it), so the single quote here is uniformity with the others rather than a live hole. The 64 character limit is unchanged, now settable via MaxIdentifierLength. ArgumentException is kept as the exception type, so existing callers that catch it still do. Full Weasel.MySql suite green against the mysql:8.0 container (227 passed). Co-Authored-By: Claude Opus 5 --- src/Weasel.MySql.Tests/MySqlMigratorTests.cs | 81 ++++++++++++++++++++ src/Weasel.MySql/MySqlMigrator.cs | 33 +++++++- 2 files changed, 111 insertions(+), 3 deletions(-) diff --git a/src/Weasel.MySql.Tests/MySqlMigratorTests.cs b/src/Weasel.MySql.Tests/MySqlMigratorTests.cs index 0b592c4..5da9245 100644 --- a/src/Weasel.MySql.Tests/MySqlMigratorTests.cs +++ b/src/Weasel.MySql.Tests/MySqlMigratorTests.cs @@ -28,6 +28,87 @@ public void create_table_returns_mysql_table() table.Identifier.ShouldBe(identifier); } + /// + /// weasel#416. AssertValidIdentifier is the only identifier check in the stack, and this provider's + /// checked length only until now, so it has to reject the characters that let a name escape the + /// statement it is written into. MySQL delimits identifiers with backticks -- and with " + /// under ANSI_QUOTES -- a ' closes a string literal, a \ escapes the character after + /// it inside one, and a ; starts a new statement. + /// + [Theory] + [InlineData("users`", "a trailing backtick")] + [InlineData("`users`", "a fully backtick-wrapped name")] + [InlineData("us`ers", "an embedded backtick")] + [InlineData("users`; drop table users; --", "a backtick-and-semicolon payload")] + [InlineData("us\"ers", "an embedded double quote")] + [InlineData("us'ers", "an embedded single quote")] + [InlineData("users\\", "a trailing backslash")] + [InlineData("users;", "a trailing semicolon")] + [InlineData("us;ers", "an embedded semicolon")] + public void assert_identifier_rejects_quote_backtick_backslash_and_semicolon(string name, string description) + { + var migrator = new MySqlMigrator(); + + Should.Throw( + () => migrator.AssertValidIdentifier(name), + $"Expected {description} to be rejected"); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + [InlineData("us ers")] + [InlineData("us\ters")] + [InlineData("us\ners")] + [InlineData("us\rers")] + [InlineData("users\n-- the rest of this statement is now a comment")] + public void assert_identifier_rejects_null_empty_and_whitespace(string? name) + { + var migrator = new MySqlMigrator(); + + Should.Throw(() => migrator.AssertValidIdentifier(name!)); + } + + [Fact] + public void assert_identifier_rejects_names_past_the_length_limit() + { + var migrator = new MySqlMigrator(); + + Should.NotThrow(() => migrator.AssertValidIdentifier(new string('a', 64))); + Should.Throw(() => migrator.AssertValidIdentifier(new string('a', 65))); + } + + [Fact] + public void invalid_identifier_message_says_which_rule_was_broken() + { + var migrator = new MySqlMigrator(); + + var ex = Should.Throw(() => migrator.AssertValidIdentifier("us`ers")); + + ex.Message.ShouldContain("us`ers"); + ex.Message.ShouldContain("backtick"); + } + + /// + /// Names Weasel and its consumers actually generate must keep working -- the tightening is aimed at + /// a handful of characters, not at narrowing the identifier grammar. + /// + [Theory] + [InlineData("mt_doc_user")] + [InlineData("mt_stream")] + [InlineData("mt_doc_user_hilo")] + [InlineData("users$1")] + [InlineData("_leading_underscore")] + [InlineData("MixedCaseName")] + [InlineData("naïve_café")] + [InlineData("table-with-dashes")] + [InlineData("mt_doc_target.p_tenant_one")] + public void assert_identifier_still_accepts_ordinary_names(string name) + { + Should.NotThrow(() => new MySqlMigrator().AssertValidIdentifier(name)); + } + [Fact] public async Task can_ensure_database_that_does_not_exist() { diff --git a/src/Weasel.MySql/MySqlMigrator.cs b/src/Weasel.MySql/MySqlMigrator.cs index 5b07baf..4ebd4fd 100644 --- a/src/Weasel.MySql/MySqlMigrator.cs +++ b/src/Weasel.MySql/MySqlMigrator.cs @@ -107,12 +107,39 @@ public override string ToExecuteScriptLine(string scriptName) return $"source {scriptName}"; } + /// + /// The characters that are unsafe in a MySQL identifier beyond the universal ones: the backtick + /// MySQL delimits identifiers with ( wraps every name in + /// backticks and does not double an embedded one), the double quote that delimits identifiers under + /// ANSI_QUOTES and string literals otherwise, and the backslash, which is an escape character + /// inside MySQL string literals unless NO_BACKSLASH_ESCAPES is set -- a trailing one would + /// otherwise swallow the closing quote of the literal a name is written into. + /// + private const string UnsafeIdentifierCharacters = "`\"\\"; + + /// + /// MySQL's identifier length limit. + /// + public int MaxIdentifierLength { get; set; } = 64; + + /// + /// Validates a database object name before it is written into DDL. See + /// for why each rule is here; before weasel#416 this checked + /// length only, and threw on a null name. + /// + /// The name cannot be safely written into DDL. public override void AssertValidIdentifier(string name) { - // MySQL identifiers can be up to 64 characters - if (name.Length > 64) + var problem = IdentifierValidation.FindProblem(name, UnsafeIdentifierCharacters); + if (problem != null) + { + throw new ArgumentException($"MySQL identifier '{name}' is not valid because {problem}."); + } + + if (name.Length > MaxIdentifierLength) { - throw new ArgumentException($"MySQL identifier '{name}' exceeds the 64 character limit."); + throw new ArgumentException( + $"MySQL identifier '{name}' exceeds the {MaxIdentifierLength} character limit."); } } From 41f0633b7ca9b463d25a1978036689b1726e00e0 Mon Sep 17 00:00:00 2001 From: "Jeremy D. Miller" Date: Fri, 31 Jul 2026 17:20:00 -0500 Subject: [PATCH 5/5] fix(sqlite): reject quote, semicolon and interior whitespace in AssertValidIdentifier (weasel#416) The check covered null/all-whitespace and length, which left '"', '\'', ';' and interior whitespace -- a name could carry a newline and smuggle a '--' comment into the statement it was written into. Now rejected, via IdentifierValidation.FindProblem: '"', '\'', ';', whitespace anywhere, and null/empty. The 255 character limit is unchanged, now settable via MaxIdentifierLength. SQLite's SchemaUtils.QuoteName is the only one of the four that doubles an embedded quote, but it only quotes at all for reserved keywords, spaces, dashes and leading digits -- so an ordinary-looking name carrying a quote is written out raw. The single quote matters because the introspection path interpolates the table name into literals: WHERE name = '...' against sqlite_master, and pragma_table_info('...') in Table.FetchExisting. InvalidOperationException is kept as the exception type; the existing assert_valid_identifier_rejects_empty and _rejects_too_long tests pin it and still pass unchanged. Full Weasel.Sqlite suite green (387 passed, no container needed). Co-Authored-By: Claude Opus 5 --- .../SqliteMigratorTests.cs | 76 +++++++++++++++++++ src/Weasel.Sqlite/SqliteMigrator.cs | 37 +++++++-- 2 files changed, 106 insertions(+), 7 deletions(-) diff --git a/src/Weasel.Sqlite.Tests/SqliteMigratorTests.cs b/src/Weasel.Sqlite.Tests/SqliteMigratorTests.cs index 0ee07f0..57bc387 100644 --- a/src/Weasel.Sqlite.Tests/SqliteMigratorTests.cs +++ b/src/Weasel.Sqlite.Tests/SqliteMigratorTests.cs @@ -105,6 +105,82 @@ public void assert_valid_identifier_rejects_too_long() Should.Throw(() => migrator.AssertValidIdentifier(longName)); } + /// + /// weasel#416. AssertValidIdentifier is the only identifier check in the stack, so it has to reject + /// the characters that let a name escape the statement it is written into. A " closes a + /// quoted identifier -- SchemaUtils.QuoteName doubles an embedded quote but only quotes at all for + /// keywords, spaces, dashes and leading digits, so an ordinary-looking name carrying one is written + /// out raw -- a ' closes the string literal the introspection path interpolates names into + /// (pragma_table_info('...')), and a ; starts a new statement. + /// + [Theory] + [InlineData("users\"", "a trailing double quote")] + [InlineData("\"users", "a leading double quote")] + [InlineData("us\"ers", "an embedded double quote")] + [InlineData("\"users\"", "a fully quote-wrapped name")] + [InlineData("users\"; drop table users; --", "a quote-and-semicolon payload")] + [InlineData("users'", "a trailing single quote")] + [InlineData("us'ers", "an embedded single quote")] + [InlineData("users'; drop table users; --", "a literal-breaking payload")] + [InlineData("users;", "a trailing semicolon")] + [InlineData("us;ers", "an embedded semicolon")] + public void assert_identifier_rejects_quote_and_semicolon(string name, string description) + { + var migrator = new SqliteMigrator(); + + Should.Throw( + () => migrator.AssertValidIdentifier(name), + $"Expected {description} to be rejected"); + } + + /// + /// Only null and all-whitespace names used to be checked, so a name could still carry an interior + /// newline and smuggle a '--' comment into the statement. + /// + [Theory] + [InlineData(null)] + [InlineData("us ers")] + [InlineData("us\ters")] + [InlineData("us\ners")] + [InlineData("us\rers")] + [InlineData("users\n-- the rest of this statement is now a comment")] + public void assert_identifier_rejects_null_and_interior_whitespace(string? name) + { + var migrator = new SqliteMigrator(); + + Should.Throw(() => migrator.AssertValidIdentifier(name!)); + } + + [Fact] + public void invalid_identifier_message_says_which_rule_was_broken() + { + var migrator = new SqliteMigrator(); + + var ex = Should.Throw(() => migrator.AssertValidIdentifier("us\"ers")); + + ex.Message.ShouldContain("us\"ers"); + ex.Message.ShouldContain("double quote"); + } + + /// + /// Names Weasel and its consumers actually generate must keep working -- the tightening is aimed at + /// a handful of characters, not at narrowing the identifier grammar. + /// + [Theory] + [InlineData("mt_doc_user")] + [InlineData("mt_stream")] + [InlineData("mt_doc_user_hilo")] + [InlineData("users$1")] + [InlineData("_leading_underscore")] + [InlineData("MixedCaseName")] + [InlineData("naïve_café")] + [InlineData("table-with-dashes")] + [InlineData("mt_doc_target.p_tenant_one")] + public void assert_identifier_still_accepts_ordinary_names(string name) + { + Should.NotThrow(() => new SqliteMigrator().AssertValidIdentifier(name)); + } + [Fact] public async Task ensure_database_exists_is_noop_for_memory() { diff --git a/src/Weasel.Sqlite/SqliteMigrator.cs b/src/Weasel.Sqlite/SqliteMigrator.cs index a77b3b3..1332245 100644 --- a/src/Weasel.Sqlite/SqliteMigrator.cs +++ b/src/Weasel.Sqlite/SqliteMigrator.cs @@ -84,19 +84,42 @@ public override string ToExecuteScriptLine(string scriptName) return $".read {scriptName}"; } + /// + /// The character SQLite delimits identifiers with. does double + /// an embedded ", but only quotes at all for reserved keywords, spaces, dashes and leading + /// digits -- an ordinary-looking name carrying a quote is written out raw. + /// + private const string UnsafeIdentifierCharacters = "\""; + + /// + /// SQLite itself allows identifiers up to 1073741824 characters, which is not a useful limit; this + /// is the practical one Weasel enforces, in line with the other providers. + /// + public int MaxIdentifierLength { get; set; } = 255; + + /// + /// Validates a database object name before it is written into DDL. See + /// for why each rule is here; before weasel#416 this checked + /// null/whitespace and length only. + /// + /// + /// The single quote matters here because SQLite's introspection path interpolates the table name + /// into string literals -- WHERE name = '...' against sqlite_master and + /// pragma_table_info('...') in Table.FetchExisting. + /// + /// The name cannot be safely written into DDL. public override void AssertValidIdentifier(string name) { - if (string.IsNullOrWhiteSpace(name)) + var problem = IdentifierValidation.FindProblem(name, UnsafeIdentifierCharacters); + if (problem != null) { - throw new InvalidOperationException($"SQLite identifier cannot be empty or whitespace"); + throw new InvalidOperationException($"SQLite identifier '{name}' is not valid because {problem}."); } - // SQLite is quite permissive with identifier names, but we should check for basic issues - // SQLite allows up to 1073741824 characters, but that's impractical - // We'll use a reasonable limit similar to other databases - if (name.Length > 255) + if (name.Length > MaxIdentifierLength) { - throw new InvalidOperationException($"SQLite identifier '{name}' is too long ({name.Length} characters). Maximum recommended length is 255."); + throw new InvalidOperationException( + $"SQLite identifier '{name}' is too long ({name.Length} characters). Maximum recommended length is {MaxIdentifierLength}."); } }