From 2d839e36090b3103fb63ca01d60e4fc4d849a00e Mon Sep 17 00:00:00 2001 From: Sakura Akeno Isayeki Date: Tue, 29 Apr 2025 00:47:19 +0200 Subject: [PATCH] fix(db/postgres): Resolve SQL identifier casing issues Improves SQL query execution by ensuring proper casing for identifiers across various database types, particularly for Postgres. Enhances security and compatibility by using an identifier escaping method, preventing potential errors due to case sensitivity in SQL queries. Addresses potential issues with existing queries for better reliability and consistency. --- TShockAPI/DB/BanManager.cs | 27 +++++++++-------- TShockAPI/DB/CharacterManager.cs | 2 +- TShockAPI/DB/GroupManager.cs | 24 ++++++++++----- TShockAPI/DB/Queries/PostgresQueryCreator.cs | 4 +-- TShockAPI/DB/RegionManager.cs | 17 +++++++---- TShockAPI/DB/SqlTable.cs | 9 ++++-- TShockAPI/DB/UserManager.cs | 12 ++++---- TShockAPI/DB/WarpsManager.cs | 2 +- TShockAPI/Extensions/DbExt.cs | 31 ++++++++++++++------ 9 files changed, 81 insertions(+), 47 deletions(-) diff --git a/TShockAPI/DB/BanManager.cs b/TShockAPI/DB/BanManager.cs index c56e3d0d..2dfb6322 100644 --- a/TShockAPI/DB/BanManager.cs +++ b/TShockAPI/DB/BanManager.cs @@ -78,7 +78,7 @@ namespace TShockAPI.DB } catch (DllNotFoundException) { - System.Console.WriteLine(GetString("Possible problem with your database - is Sqlite3.dll present?")); + Console.WriteLine(GetString("Possible problem with your database - is Sqlite3.dll present?")); throw new Exception(GetString("Could not find a database library (probably Sqlite3.dll)")); } @@ -355,7 +355,9 @@ namespace TShockAPI.DB return Bans[id]; } - using var reader = database.QueryReader("SELECT * FROM PlayerBans WHERE TicketNumber=@0", id); + string query = $"SELECT * FROM PlayerBans WHERE {"TicketNumber".EscapeSqlId(database)}=@0"; + + using var reader = database.QueryReader(query, id); if (reader.Read()) { @@ -380,10 +382,11 @@ namespace TShockAPI.DB /// public IEnumerable RetrieveBansByIdentifier(string identifier, bool currentOnly = true) { - string query = "SELECT * FROM PlayerBans WHERE Identifier=@0"; + string query = $"SELECT * FROM PlayerBans WHERE {"Identifier".EscapeSqlId(database)}=@0"; + if (currentOnly) { - query += $" AND Expiration > {DateTime.UtcNow.Ticks}"; + query += $" AND {"Expiration".EscapeSqlId(database)} > {DateTime.UtcNow.Ticks}"; } using var reader = database.QueryReader(query, identifier); @@ -412,11 +415,11 @@ namespace TShockAPI.DB //Generate a sequence of '@0, @1, @2, ... etc' var parameters = string.Join(", ", Enumerable.Range(0, identifiers.Length).Select(p => $"@{p}")); - string query = $"SELECT * FROM PlayerBans WHERE Identifier IN ({parameters})"; + string query = $"SELECT * FROM PlayerBans WHERE {"Identifier".EscapeSqlId(database)} IN ({parameters})"; if (currentOnly) { - query += $" AND Expiration > {DateTime.UtcNow.Ticks}"; + query += $" AND {"Expiration".EscapeSqlId(database)} > {DateTime.UtcNow.Ticks}"; } using var reader = database.QueryReader(query, identifiers); @@ -449,7 +452,7 @@ namespace TShockAPI.DB List banlist = new List(); try { - using var reader = database.QueryReader($"SELECT * FROM PlayerBans ORDER BY {SortToOrderByMap[sortMethod]}"); + using var reader = database.QueryReader($"SELECT * FROM PlayerBans ORDER BY {SortToOrderByMap(sortMethod)}"); while (reader.Read()) { @@ -490,12 +493,12 @@ namespace TShockAPI.DB return false; } - private readonly Dictionary SortToOrderByMap = new() + private string SortToOrderByMap(BanSortMethod sortMethod) => sortMethod switch { - { BanSortMethod.AddedNewestToOldest, "Date DESC" }, - { BanSortMethod.AddedOldestToNewest, "Date ASC" }, - { BanSortMethod.ExpirationSoonestToLatest, "Expiration ASC" }, - { BanSortMethod.ExpirationLatestToSoonest, "Expiration DESC" } + BanSortMethod.AddedNewestToOldest => $"{"Date".EscapeSqlId(database)} DESC", + BanSortMethod.AddedOldestToNewest => $"{"Date".EscapeSqlId(database)} ASC", + BanSortMethod.ExpirationSoonestToLatest => $"{"Expiration".EscapeSqlId(database)} ASC", + BanSortMethod.ExpirationLatestToSoonest => $"{"Expiration".EscapeSqlId(database)} DESC" }; } diff --git a/TShockAPI/DB/CharacterManager.cs b/TShockAPI/DB/CharacterManager.cs index 3b9890a3..08d6d1ec 100644 --- a/TShockAPI/DB/CharacterManager.cs +++ b/TShockAPI/DB/CharacterManager.cs @@ -82,7 +82,7 @@ namespace TShockAPI.DB try { - using var reader = database.QueryReader("SELECT * FROM tsCharacter WHERE Account=@0", acctid); + using var reader = database.QueryReader($"SELECT * FROM tsCharacter WHERE {"Account".EscapeSqlId(database)}=@0", acctid); if (reader.Read()) { playerData.exists = true; diff --git a/TShockAPI/DB/GroupManager.cs b/TShockAPI/DB/GroupManager.cs index 553a1a33..ba82225a 100644 --- a/TShockAPI/DB/GroupManager.cs +++ b/TShockAPI/DB/GroupManager.cs @@ -314,15 +314,22 @@ namespace TShockAPI.DB group.Parent = parent; } - string query = (TShock.Config.Settings.StorageType.ToLower() == "sqlite") - ? "INSERT OR IGNORE INTO GroupList (GroupName, Parent, Commands, ChatColor) VALUES (@0, @1, @2, @3);" - : "INSERT IGNORE INTO GroupList SET GroupName=@0, Parent=@1, Commands=@2, ChatColor=@3"; - if (database.Query(query, name, parentname, permissions, chatcolor) == 1) + string query = database.GetSqlType() switch + { + SqlType.Sqlite => "INSERT OR IGNORE INTO GroupList (GroupName, Parent, Commands, ChatColor) VALUES (@0, @1, @2, @3);", + SqlType.Mysql => "INSERT IGNORE INTO GroupList SET GroupName=@0, Parent=@1, Commands=@2, ChatColor=@3", + SqlType.Postgres => "INSERT INTO GroupList (\"GroupName\", \"Parent\", \"Commands\", \"ChatColor\") VALUES (@0, @1, @2, @3) ON CONFLICT (\"GroupName\") DO NOTHING", + _ => throw new NotSupportedException(GetString("Unsupported database type.")) + }; + + if (database.Query(query, name, parentname, permissions, chatcolor) is 1) { groups.Add(group); } else + { throw new GroupManagerException(GetString($"Failed to add group {name}.")); + } } /// @@ -362,9 +369,12 @@ namespace TShockAPI.DB } // Ensure any group validation is also persisted to the DB. - var newGroup = new Group(name, parent, chatcolor, permissions); - newGroup.Prefix = prefix; - newGroup.Suffix = suffix; + var newGroup = new Group(name, parent, chatcolor, permissions) + { + Prefix = prefix, + Suffix = suffix + }; + string query = "UPDATE GroupList SET Parent=@0, Commands=@1, ChatColor=@2, Suffix=@3, Prefix=@4 WHERE GroupName=@5"; if (database.Query(query, parentname, newGroup.Permissions, newGroup.ChatColor, suffix, prefix, name) != 1) throw new GroupManagerException(GetString($"Failed to update group \"{name}\".")); diff --git a/TShockAPI/DB/Queries/PostgresQueryCreator.cs b/TShockAPI/DB/Queries/PostgresQueryCreator.cs index 779e38b0..5637d689 100644 --- a/TShockAPI/DB/Queries/PostgresQueryCreator.cs +++ b/TShockAPI/DB/Queries/PostgresQueryCreator.cs @@ -79,9 +79,7 @@ public class PostgresQueryCreator : GenericQueryCreator .Where(c => c.Unique).Select(c => $"\"{c.Name}\"") .ToArray(); // No re-enumeration - return "CREATE TABLE {0} ({1} {2})".SFormat(EscapeTableName(table.Name), - string.Join(", ", columns), - uniques.Any() ? ", UNIQUE({0})".SFormat(string.Join(", ", uniques)) : ""); + return $"CREATE TABLE {EscapeTableName(table.Name)} ({string.Join(", ", columns)} {(uniques.Any() ? ", UNIQUE({0})".SFormat(string.Join(", ", uniques)) : "")})"; } /// diff --git a/TShockAPI/DB/RegionManager.cs b/TShockAPI/DB/RegionManager.cs index d657980a..cc7eacdb 100644 --- a/TShockAPI/DB/RegionManager.cs +++ b/TShockAPI/DB/RegionManager.cs @@ -67,9 +67,9 @@ namespace TShockAPI.DB { try { - using var reader = database.QueryReader("SELECT * FROM Regions WHERE WorldID=@0", Main.worldID.ToString()); - + using var reader = database.QueryReader($"SELECT * FROM Regions WHERE {"WorldID".EscapeSqlId(database)}=@0", Main.worldID.ToString()); Regions.Clear(); + while (reader.Read()) { int id = reader.Get("Id"); @@ -135,10 +135,17 @@ namespace TShockAPI.DB } try { - database.Query( - "INSERT INTO Regions (X1, Y1, width, height, RegionName, WorldID, UserIds, Protected, `Groups`, Owner, Z) VALUES (@0, @1, @2, @3, @4, @5, @6, @7, @8, @9, @10);", - tx, ty, width, height, regionname, worldid, "", 1, "", owner, z); + string query = database.GetSqlType() switch + { + SqlType.Postgres => "INSERT INTO Regions (\"X1\", \"Y1\", \"width\", \"height\", \"RegionName\", \"WorldID\", \"UserIds\", \"Protected\", \"Groups\", \"Owner\", \"Z\") VALUES (@0, @1, @2, @3, @4, @5, @6, @7, @8, @9, @10);", + _ => "INSERT INTO Regions (X1, Y1, width, height, RegionName, WorldID, UserIds, Protected, Groups, Owner, Z) VALUES (@0, @1, @2, @3, @4, @5, @6, @7, @8, @9, @10);", + + }; + + database.Query(query, tx, ty, width, height, regionname, worldid, "", 1, "", owner, z); + int id; + using (QueryResult res = database.QueryReader("SELECT Id FROM Regions WHERE RegionName = @0 AND WorldID = @1", regionname, worldid)) { if (res.Read()) diff --git a/TShockAPI/DB/SqlTable.cs b/TShockAPI/DB/SqlTable.cs index 6bf7b8a8..ce955dbf 100644 --- a/TShockAPI/DB/SqlTable.cs +++ b/TShockAPI/DB/SqlTable.cs @@ -59,7 +59,9 @@ namespace TShockAPI.DB var columns = GetColumns(table); if (columns.Count > 0) { - if (!table.Columns.All(c => columns.Contains(c.Name)) || !columns.All(c => table.Columns.Any(c2 => c2.Name == c))) + // Use OrdinalIgnoreCase to account for pgsql automatically lowering cases. + if (!table.Columns.All(c => columns.Contains(c.Name, StringComparer.OrdinalIgnoreCase)) + || !columns.All(c => table.Columns.Any(c2 => c2.Name.Equals(c, StringComparison.OrdinalIgnoreCase)))) { var from = new SqlTable(table.Name, columns.Select(s => new SqlColumn(s, MySqlDbType.String)).ToList()); database.Query(creator.AlterTable(from, table)); @@ -70,6 +72,7 @@ namespace TShockAPI.DB database.Query(creator.CreateTable(table)); return true; } + return false; } @@ -102,8 +105,8 @@ namespace TShockAPI.DB } case SqlType.Postgres: { - using QueryResult reader = - database.QueryReader("SELECT column_name FROM information_schema.columns WHERE table_name=@0", table.Name); + // HACK: Using "ilike" op to ignore case, due to weird case issues adapting for pgsql + using QueryResult reader = database.QueryReader("SELECT column_name FROM information_schema.columns WHERE table_name ILIKE @0", table.Name); while (reader.Read()) { diff --git a/TShockAPI/DB/UserManager.cs b/TShockAPI/DB/UserManager.cs index e705e3fc..47a1c15f 100644 --- a/TShockAPI/DB/UserManager.cs +++ b/TShockAPI/DB/UserManager.cs @@ -239,7 +239,7 @@ namespace TShockAPI.DB { try { - using var reader = _database.QueryReader("SELECT * FROM Users WHERE Username=@0", username); + using var reader = _database.QueryReader($"SELECT * FROM Users WHERE {"Username".EscapeSqlId(_database)}=@0", username); if (reader.Read()) { return reader.Get("ID"); @@ -293,13 +293,13 @@ namespace TShockAPI.DB object arg; if (account.ID != 0) { - query = "SELECT * FROM Users WHERE ID=@0"; + query = $"SELECT * FROM Users WHERE {"ID".EscapeSqlId(_database)}=@0"; arg = account.ID; type = "id"; } else { - query = "SELECT * FROM Users WHERE Username=@0"; + query = $"SELECT * FROM Users WHERE {"Username".EscapeSqlId(_database)}=@0"; arg = account.Name; type = "name"; } @@ -358,9 +358,9 @@ namespace TShockAPI.DB try { List accounts = new List(); - string search = notAtStart ? string.Format("%{0}%", username) : string.Format("{0}%", username); - using var reader = _database.QueryReader("SELECT * FROM Users WHERE Username LIKE @0", - search); + string search = $"{(notAtStart ? "%" : "")}{username}%"; + using var reader = _database.QueryReader($"SELECT * FROM Users WHERE {"Username".EscapeSqlId(_database)} LIKE @0", search); + while (reader.Read()) { accounts.Add(LoadUserAccountFromResult(new UserAccount(), reader)); diff --git a/TShockAPI/DB/WarpsManager.cs b/TShockAPI/DB/WarpsManager.cs index 06723281..fb28f1a4 100644 --- a/TShockAPI/DB/WarpsManager.cs +++ b/TShockAPI/DB/WarpsManager.cs @@ -86,7 +86,7 @@ namespace TShockAPI.DB { Warps.Clear(); - using var reader = database.QueryReader("SELECT * FROM Warps WHERE WorldID = @0", + using var reader = database.QueryReader($"SELECT * FROM Warps WHERE {"WorldID".EscapeSqlId(database)} = @0", Main.worldID.ToString()); while (reader.Read()) { diff --git a/TShockAPI/Extensions/DbExt.cs b/TShockAPI/Extensions/DbExt.cs index d58a966e..55a681dd 100644 --- a/TShockAPI/Extensions/DbExt.cs +++ b/TShockAPI/Extensions/DbExt.cs @@ -20,6 +20,7 @@ using System; using System.Collections.Generic; using System.Data; using System.Diagnostics.CodeAnalysis; +using System.Diagnostics.Contracts; using Microsoft.Data.Sqlite; using MySql.Data.MySqlClient; using Npgsql; @@ -42,17 +43,17 @@ namespace TShockAPI.DB [SuppressMessage("Microsoft.Security", "CA2100:Review SQL queries for security vulnerabilities")] public static int Query(this IDbConnection olddb, string query, params object[] args) { - using (var db = olddb.CloneEx()) + using IDbConnection db = olddb.CloneEx(); + db.Open(); + using IDbCommand com = db.CreateCommand(); + com.CommandText = query; + + for (int i = 0; i < args.Length; i++) { - db.Open(); - using (var com = db.CreateCommand()) - { - com.CommandText = query; - for (int i = 0; i < args.Length; i++) - com.AddParameter("@" + i, args[i] ?? DBNull.Value); - return com.ExecuteNonQuery(); - } + com.AddParameter("@" + i, args[i] ?? DBNull.Value); } + + return com.ExecuteNonQuery(); } /// @@ -271,6 +272,18 @@ namespace TShockAPI.DB return (T)reader.GetValue(column); } + + /// + /// Escapes an identifier for use in a SQL query. + /// + /// The identifier to escape, typically a table or column name. + /// The escaped identifier. + [Pure] + public static string EscapeSqlId(this string id, IDbConnection db) => db.GetSqlType() switch + { + SqlType.Postgres => $"\"{id}\"", // The main PITA and culprit + _ => id // Default case for agnostic SQL + }; } public enum SqlType