From 38741bfaeae5efde526695906abf15cb1fd8410b Mon Sep 17 00:00:00 2001 From: Mark Kincaid Date: Mon, 10 Nov 2025 16:46:44 -0800 Subject: [PATCH] Addressed Claudebot findings --- test/Seeder.Test/Migration/CsvHandlerTests.cs | 341 ++++++++++++++++++ .../Utils/IdentifierValidatorTests.cs | 307 ++++++++++++++++ test/Seeder.Test/Seeder.Test.csproj | 26 ++ util/Seeder/Migration/CsvHandler.cs | 10 +- .../Migration/Databases/MariaDbImporter.cs | 22 +- .../Migration/Databases/PostgresImporter.cs | 42 ++- .../Migration/Databases/SqlServerExporter.cs | 22 +- .../Migration/Databases/SqlServerImporter.cs | 18 +- .../Migration/Utils/IdentifierValidator.cs | 67 +++- 9 files changed, 813 insertions(+), 42 deletions(-) create mode 100644 test/Seeder.Test/Migration/CsvHandlerTests.cs create mode 100644 test/Seeder.Test/Migration/Utils/IdentifierValidatorTests.cs create mode 100644 test/Seeder.Test/Seeder.Test.csproj diff --git a/test/Seeder.Test/Migration/CsvHandlerTests.cs b/test/Seeder.Test/Migration/CsvHandlerTests.cs new file mode 100644 index 0000000000..b10f50648c --- /dev/null +++ b/test/Seeder.Test/Migration/CsvHandlerTests.cs @@ -0,0 +1,341 @@ +using Bit.Seeder.Migration; +using Bit.Seeder.Migration.Models; +using Microsoft.Extensions.Logging; +using NSubstitute; +using Xunit; + +namespace Bit.Seeder.Test.Migration; + +public class CsvHandlerTests : IDisposable +{ + private readonly ILogger _logger; + private readonly string _testOutputDir; + private readonly CsvSettings _settings; + + public CsvHandlerTests() + { + _logger = Substitute.For>(); + _testOutputDir = Path.Combine(Path.GetTempPath(), $"CsvHandlerTests_{Guid.NewGuid()}"); + Directory.CreateDirectory(_testOutputDir); + + _settings = new CsvSettings + { + OutputDir = _testOutputDir, + Delimiter = ",", + FallbackDelimiter = "|", + IncludeHeaders = true + }; + } + + public void Dispose() + { + if (Directory.Exists(_testOutputDir)) + { + Directory.Delete(_testOutputDir, true); + } + } + + [Fact] + public void ExportTableToCsv_BasicData_CreatesValidCsvFile() + { + // Arrange + var handler = new CsvHandler(_settings, _logger); + var tableName = "Users"; + var columns = new List { "Id", "Name", "Email" }; + var data = new List + { + new object[] { 1, "John Doe", "john@example.com" }, + new object[] { 2, "Jane Smith", "jane@example.com" } + }; + + // Act + var csvPath = handler.ExportTableToCsv(tableName, columns, data); + + // Assert + Assert.True(File.Exists(csvPath)); + var lines = File.ReadAllLines(csvPath); + Assert.Equal(3, lines.Length); // Header + 2 data rows + Assert.Contains("Id", lines[0]); + Assert.Contains("Name", lines[0]); + Assert.Contains("Email", lines[0]); + } + + [Fact] + public void ExportTableToCsv_WithNullValues_HandlesCorrectly() + { + // Arrange + var handler = new CsvHandler(_settings, _logger); + var tableName = "TestTable"; + var columns = new List { "Col1", "Col2", "Col3" }; + var data = new List + { + new object[] { 1, null, "value" }, + new object?[] { 2, "test", null } + }; + + // Act + var csvPath = handler.ExportTableToCsv(tableName, columns, data); + + // Assert + Assert.True(File.Exists(csvPath)); + var content = File.ReadAllText(csvPath); + // Null values should be exported as empty strings + Assert.Contains("\"\"", content); + } + + [Fact] + public void ExportTableToCsv_WithJsonData_HandlesSpecialColumns() + { + // Arrange + var handler = new CsvHandler(_settings, _logger); + var tableName = "Settings"; + var columns = new List { "Id", "Data" }; + var specialColumns = new List { "Data" }; + var data = new List + { + new object[] { 1, "{\"key\":\"value\",\"nested\":{\"prop\":123}}" } + }; + + // Act + var csvPath = handler.ExportTableToCsv(tableName, columns, data, specialColumns); + + // Assert + Assert.True(File.Exists(csvPath)); + var content = File.ReadAllText(csvPath); + // JSON should be preserved + Assert.Contains("key", content); + Assert.Contains("value", content); + } + + [Fact] + public void ExportTableToCsv_WithDateTime_FormatsCorrectly() + { + // Arrange + var handler = new CsvHandler(_settings, _logger); + var tableName = "Events"; + var columns = new List { "Id", "CreatedDate" }; + var testDate = new DateTime(2024, 1, 15, 10, 30, 45, 123); + var data = new List + { + new object[] { 1, testDate } + }; + + // Act + var csvPath = handler.ExportTableToCsv(tableName, columns, data); + + // Assert + Assert.True(File.Exists(csvPath)); + var content = File.ReadAllText(csvPath); + // Should contain formatted date with microseconds + Assert.Contains("2024-01-15", content); + } + + [Fact] + public void ImportCsvToData_BasicCsv_ReadsCorrectly() + { + // Arrange + var handler = new CsvHandler(_settings, _logger); + var csvPath = Path.Combine(_testOutputDir, "test.csv"); + var csvContent = "\"Id\",\"Name\",\"Email\"\n\"1\",\"John\",\"john@test.com\"\n\"2\",\"Jane\",\"jane@test.com\""; + File.WriteAllText(csvPath, csvContent); + + // Act + var (columns, data) = handler.ImportCsvToData(csvPath); + + // Assert + Assert.Equal(3, columns.Count); + Assert.Contains("Id", columns); + Assert.Contains("Name", columns); + Assert.Contains("Email", columns); + Assert.Equal(2, data.Count); + Assert.Equal("1", data[0][0]); + Assert.Equal("John", data[0][1]); + } + + [Fact] + public void ImportCsvToData_NonExistentFile_ThrowsFileNotFoundException() + { + // Arrange + var handler = new CsvHandler(_settings, _logger); + var nonExistentPath = Path.Combine(_testOutputDir, "nonexistent.csv"); + + // Act & Assert + Assert.Throws(() => + handler.ImportCsvToData(nonExistentPath)); + } + + [Fact] + public void ImportCsvToData_WithEmptyValues_ConvertsToDBNull() + { + // Arrange + var handler = new CsvHandler(_settings, _logger); + var csvPath = Path.Combine(_testOutputDir, "test_empty.csv"); + var csvContent = "\"Col1\",\"Col2\",\"Col3\"\n\"1\",\"\",\"value\"\n\"2\",\"test\",\"\""; + File.WriteAllText(csvPath, csvContent); + + // Act + var (columns, data) = handler.ImportCsvToData(csvPath); + + // Assert + Assert.Equal(2, data.Count); + Assert.Equal(DBNull.Value, data[0][1]); // Empty string should become DBNull + } + + [Fact] + public void ImportCsvToData_WithJsonSpecialColumn_RestoresJson() + { + // Arrange + var handler = new CsvHandler(_settings, _logger); + var csvPath = Path.Combine(_testOutputDir, "test_json.csv"); + var jsonData = "{\"key\":\"value\"}"; + var csvContent = $"\"Id\",\"Data\"\n\"1\",\"{jsonData}\""; + File.WriteAllText(csvPath, csvContent); + var specialColumns = new List { "Data" }; + + // Act + var (columns, data) = handler.ImportCsvToData(csvPath, specialColumns); + + // Assert + Assert.Single(data); + var restoredJson = data[0][1].ToString(); + Assert.Contains("key", restoredJson); + Assert.Contains("value", restoredJson); + } + + [Fact] + public void ImportCsvToData_WithMalformedData_LogsWarning() + { + // Arrange + var handler = new CsvHandler(_settings, _logger); + var csvPath = Path.Combine(_testOutputDir, "test_malformed.csv"); + // Create CSV with mismatched column count (intentionally malformed) + var csvContent = "\"Col1\",\"Col2\",\"Col3\"\n\"1\",\"value1\"\n\"2\",\"value2\",\"value3\""; + File.WriteAllText(csvPath, csvContent); + + // Act - this should trigger BadDataFound callback + var exception = Record.Exception(() => handler.ImportCsvToData(csvPath)); + + // Note: CsvHelper may throw or handle this depending on configuration + // The test verifies that BadDataFound handler logs the issue + // We verify the logger was called with Warning level + if (exception == null) + { + _logger.Received().Log( + LogLevel.Warning, + Arg.Any(), + Arg.Is(o => o.ToString()!.Contains("Bad CSV data")), + Arg.Any(), + Arg.Any>()); + } + } + + [Fact] + public void ValidateExport_CorrectRowCount_ReturnsTrue() + { + // Arrange + var handler = new CsvHandler(_settings, _logger); + var csvPath = Path.Combine(_testOutputDir, "validate_test.csv"); + var csvContent = "\"Col1\",\"Col2\"\n\"1\",\"a\"\n\"2\",\"b\"\n\"3\",\"c\""; + File.WriteAllText(csvPath, csvContent); + var originalCount = 3; // 3 data rows + + // Act + var isValid = handler.ValidateExport(originalCount, csvPath); + + // Assert + Assert.True(isValid); + } + + [Fact] + public void ValidateExport_IncorrectRowCount_ReturnsFalse() + { + // Arrange + var handler = new CsvHandler(_settings, _logger); + var csvPath = Path.Combine(_testOutputDir, "validate_fail_test.csv"); + var csvContent = "\"Col1\",\"Col2\"\n\"1\",\"a\"\n\"2\",\"b\""; + File.WriteAllText(csvPath, csvContent); + var originalCount = 5; // Expecting 5 but only have 2 + + // Act + var isValid = handler.ValidateExport(originalCount, csvPath); + + // Assert + Assert.False(isValid); + } + + [Fact] + public void ExportTableToCsv_WithoutHeaders_ExportsDataOnly() + { + // Arrange + var settingsNoHeaders = new CsvSettings + { + OutputDir = _testOutputDir, + Delimiter = ",", + FallbackDelimiter = "|", + IncludeHeaders = false + }; + var handler = new CsvHandler(settingsNoHeaders, _logger); + var tableName = "TestTable"; + var columns = new List { "Col1", "Col2" }; + var data = new List + { + new object[] { 1, "value1" }, + new object[] { 2, "value2" } + }; + + // Act + var csvPath = handler.ExportTableToCsv(tableName, columns, data); + + // Assert + Assert.True(File.Exists(csvPath)); + var lines = File.ReadAllLines(csvPath); + Assert.Equal(2, lines.Length); // Only data rows, no header + } + + [Fact] + public void ImportCsvToData_WithPipeDelimiter_DetectsAndReadsCorrectly() + { + // Arrange + var handler = new CsvHandler(_settings, _logger); + var csvPath = Path.Combine(_testOutputDir, "test_pipe.csv"); + var csvContent = "\"Id\"|\"Name\"|\"Email\"\n\"1\"|\"John\"|\"john@test.com\""; + File.WriteAllText(csvPath, csvContent); + + // Act + var (columns, data) = handler.ImportCsvToData(csvPath); + + // Assert + Assert.Equal(3, columns.Count); + Assert.Single(data); + } + + [Fact] + public void ExportAndImportRoundTrip_PreservesData() + { + // Arrange + var handler = new CsvHandler(_settings, _logger); + var tableName = "RoundTripTest"; + var columns = new List { "Id", "Name", "Value" }; + var originalData = new List + { + new object[] { 1, "Test1", "Value1" }, + new object[] { 2, "Test2", "Value2" }, + new object[] { 3, "Test3", "Value3" } + }; + + // Act - Export then Import + var csvPath = handler.ExportTableToCsv(tableName, columns, originalData); + var (importedColumns, importedData) = handler.ImportCsvToData(csvPath); + + // Assert + Assert.Equal(columns.Count, importedColumns.Count); + Assert.Equal(originalData.Count, importedData.Count); + for (int i = 0; i < originalData.Count; i++) + { + for (int j = 0; j < columns.Count; j++) + { + Assert.Equal(originalData[i][j].ToString(), importedData[i][j].ToString()); + } + } + } +} diff --git a/test/Seeder.Test/Migration/Utils/IdentifierValidatorTests.cs b/test/Seeder.Test/Migration/Utils/IdentifierValidatorTests.cs new file mode 100644 index 0000000000..80a0f10555 --- /dev/null +++ b/test/Seeder.Test/Migration/Utils/IdentifierValidatorTests.cs @@ -0,0 +1,307 @@ +using Bit.Seeder.Migration.Utils; +using Xunit; + +namespace Bit.Seeder.Test.Migration.Utils; + +public class IdentifierValidatorTests +{ + [Fact] + public void IsValid_ValidIdentifier_ReturnsTrue() + { + // Arrange + var validIdentifier = "MyTable123"; + + // Act + var result = IdentifierValidator.IsValid(validIdentifier); + + // Assert + Assert.True(result); + } + + [Fact] + public void IsValid_ValidIdentifierStartingWithUnderscore_ReturnsTrue() + { + // Arrange + var validIdentifier = "_MyTable"; + + // Act + var result = IdentifierValidator.IsValid(validIdentifier); + + // Assert + Assert.True(result); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public void IsValid_NullOrWhiteSpace_ReturnsFalse(string? identifier) + { + // Act + var result = IdentifierValidator.IsValid(identifier); + + // Assert + Assert.False(result); + } + + [Fact] + public void IsValid_TooLongIdentifier_ReturnsFalse() + { + // Arrange - create identifier longer than 128 characters + var tooLongIdentifier = new string('a', 129); + + // Act + var result = IdentifierValidator.IsValid(tooLongIdentifier); + + // Assert + Assert.False(result); + } + + [Theory] + [InlineData("Table-Name")] + [InlineData("Table Name")] + [InlineData("Table.Name")] + [InlineData("Table;Name")] + [InlineData("Table'Name")] + [InlineData("Table\"Name")] + [InlineData("Table*Name")] + [InlineData("Table/Name")] + [InlineData("Table\\Name")] + [InlineData("123Table")] + public void IsValid_InvalidCharactersOrStartingWithNumber_ReturnsFalse(string identifier) + { + // Act + var result = IdentifierValidator.IsValid(identifier); + + // Assert + Assert.False(result); + } + + [Theory] + [InlineData("SELECT")] + [InlineData("DROP")] + [InlineData("DELETE")] + [InlineData("INSERT")] + [InlineData("UPDATE")] + [InlineData("EXEC")] + [InlineData("EXECUTE")] + public void IsValid_SqlReservedKeyword_WithRestrictiveMode_ReturnsFalse(string keyword) + { + // Act + var result = IdentifierValidator.IsValid(keyword, useRestrictiveMode: true); + + // Assert + Assert.False(result); + } + + [Theory] + [InlineData("SELECT")] + [InlineData("DROP")] + [InlineData("DELETE")] + public void IsValid_SqlReservedKeyword_WithoutRestrictiveMode_ReturnsTrue(string keyword) + { + // Act - without restrictive mode, these pass the regex but are still SQL keywords + var result = IdentifierValidator.IsValid(keyword, useRestrictiveMode: false); + + // Assert + Assert.True(result); + } + + [Theory] + [InlineData("_LeadingUnderscore")] + [InlineData("_table")] + public void IsValid_LeadingUnderscore_WithRestrictiveMode_ReturnsFalse(string identifier) + { + // Act + var result = IdentifierValidator.IsValid(identifier, useRestrictiveMode: true); + + // Assert + Assert.False(result); + } + + [Fact] + public void ValidateOrThrow_ValidIdentifier_DoesNotThrow() + { + // Arrange + var validIdentifier = "MyTable"; + + // Act & Assert + var exception = Record.Exception(() => + IdentifierValidator.ValidateOrThrow(validIdentifier, "table name")); + + Assert.Null(exception); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public void ValidateOrThrow_NullOrWhiteSpace_ThrowsArgumentException(string? identifier) + { + // Act & Assert + var exception = Assert.Throws(() => + IdentifierValidator.ValidateOrThrow(identifier, "table name")); + + Assert.Contains("null or empty", exception.Message); + } + + [Fact] + public void ValidateOrThrow_InvalidIdentifier_ThrowsArgumentException() + { + // Arrange + var invalidIdentifier = "Table-Name"; + + // Act & Assert + var exception = Assert.Throws(() => + IdentifierValidator.ValidateOrThrow(invalidIdentifier, "table name")); + + Assert.Contains("Invalid table name", exception.Message); + } + + [Fact] + public void ValidateOrThrow_SqlReservedKeyword_WithRestrictiveMode_ThrowsArgumentException() + { + // Arrange + var keyword = "SELECT"; + + // Act & Assert + var exception = Assert.Throws(() => + IdentifierValidator.ValidateOrThrow(keyword, "table name", useRestrictiveMode: true)); + + Assert.Contains("SQL reserved keyword", exception.Message); + } + + [Fact] + public void ValidateAllOrThrow_AllValidIdentifiers_DoesNotThrow() + { + // Arrange + var identifiers = new[] { "Table1", "Table2", "Column_Name" }; + + // Act & Assert + var exception = Record.Exception(() => + IdentifierValidator.ValidateAllOrThrow(identifiers, "table names")); + + Assert.Null(exception); + } + + [Fact] + public void ValidateAllOrThrow_OneInvalidIdentifier_ThrowsArgumentException() + { + // Arrange + var identifiers = new[] { "Table1", "Invalid-Table", "Table3" }; + + // Act & Assert + var exception = Assert.Throws(() => + IdentifierValidator.ValidateAllOrThrow(identifiers, "table names")); + + Assert.Contains("Invalid table names", exception.Message); + } + + [Fact] + public void FilterValid_MixedValidAndInvalid_ReturnsOnlyValid() + { + // Arrange + var identifiers = new[] + { + "ValidTable1", + "Invalid-Table", + "ValidTable2", + "Another Invalid", + "_ValidUnderscore", + "123Invalid" + }; + + // Act + var validIdentifiers = IdentifierValidator.FilterValid(identifiers); + + // Assert + Assert.Equal(3, validIdentifiers.Count); + Assert.Contains("ValidTable1", validIdentifiers); + Assert.Contains("ValidTable2", validIdentifiers); + Assert.Contains("_ValidUnderscore", validIdentifiers); + } + + [Fact] + public void FilterValid_WithRestrictiveMode_FiltersOutLeadingUnderscores() + { + // Arrange + var identifiers = new[] + { + "ValidTable", + "_UnderscoreTable", + "AnotherValid" + }; + + // Act + var validIdentifiers = IdentifierValidator.FilterValid(identifiers, useRestrictiveMode: true); + + // Assert + Assert.Equal(2, validIdentifiers.Count); + Assert.Contains("ValidTable", validIdentifiers); + Assert.Contains("AnotherValid", validIdentifiers); + Assert.DoesNotContain("_UnderscoreTable", validIdentifiers); + } + + [Fact] + public void FilterValid_WithRestrictiveMode_FiltersOutSqlKeywords() + { + // Arrange + var identifiers = new[] + { + "ValidTable", + "SELECT", + "DROP", + "AnotherValid" + }; + + // Act + var validIdentifiers = IdentifierValidator.FilterValid(identifiers, useRestrictiveMode: true); + + // Assert + Assert.Equal(2, validIdentifiers.Count); + Assert.Contains("ValidTable", validIdentifiers); + Assert.Contains("AnotherValid", validIdentifiers); + Assert.DoesNotContain("SELECT", validIdentifiers); + Assert.DoesNotContain("DROP", validIdentifiers); + } + + [Theory] + [InlineData("'; DROP TABLE users--")] + [InlineData("1' OR '1'='1")] + [InlineData("admin'--")] + [InlineData("1; DELETE FROM users")] + public void IsValid_SqlInjectionAttempts_ReturnsFalse(string injectionAttempt) + { + // Act + var result = IdentifierValidator.IsValid(injectionAttempt); + + // Assert + Assert.False(result); + } + + [Fact] + public void IsValid_MaxLengthIdentifier_ReturnsTrue() + { + // Arrange - create identifier exactly 128 characters long + var maxLengthIdentifier = new string('a', 128); + + // Act + var result = IdentifierValidator.IsValid(maxLengthIdentifier); + + // Assert + Assert.True(result); + } + + [Theory] + [InlineData("select")] + [InlineData("Select")] + [InlineData("SeLeCt")] + public void IsValid_SqlKeywordsCaseInsensitive_WithRestrictiveMode_ReturnsFalse(string keyword) + { + // Act + var result = IdentifierValidator.IsValid(keyword, useRestrictiveMode: true); + + // Assert - should be case-insensitive + Assert.False(result); + } +} diff --git a/test/Seeder.Test/Seeder.Test.csproj b/test/Seeder.Test/Seeder.Test.csproj new file mode 100644 index 0000000000..4411cd71db --- /dev/null +++ b/test/Seeder.Test/Seeder.Test.csproj @@ -0,0 +1,26 @@ + + + false + Bit.Seeder.Test + + + + runtime; build; native; contentfiles; analyzers; buildtransitive + all + + + + + + + all + runtime; build; native; contentfiles; analyzers + + + + + + + + + diff --git a/util/Seeder/Migration/CsvHandler.cs b/util/Seeder/Migration/CsvHandler.cs index b36ee13806..6c44408202 100644 --- a/util/Seeder/Migration/CsvHandler.cs +++ b/util/Seeder/Migration/CsvHandler.cs @@ -105,7 +105,15 @@ public class CsvHandler(CsvSettings settings, ILogger logger) Delimiter = delimiterUsed, HasHeaderRecord = _settings.IncludeHeaders, Encoding = _encoding, - BadDataFound = null, // Ignore bad data + BadDataFound = context => + { + // Log malformed CSV data to prevent silent data corruption + _logger.LogWarning( + "Bad CSV data at row {Row}, field {Field}: {RawRecord}", + context.Context.Parser.Row, + context.Field, + context.RawRecord); + }, TrimOptions = CsvHelper.Configuration.TrimOptions.None // Don't trim anything }; diff --git a/util/Seeder/Migration/Databases/MariaDbImporter.cs b/util/Seeder/Migration/Databases/MariaDbImporter.cs index e5c007e0e7..ceabf9b3b0 100644 --- a/util/Seeder/Migration/Databases/MariaDbImporter.cs +++ b/util/Seeder/Migration/Databases/MariaDbImporter.cs @@ -26,15 +26,21 @@ public class MariaDbImporter(DatabaseConfig config, ILogger log { try { - // Build connection string with redacted password for safe logging - var safeConnectionString = $"Server={_host};Port={_port};Database={_database};" + - $"Uid={_username};Pwd={DbSeederConstants.REDACTED_PASSWORD};" + - $"ConnectionTimeout={DbSeederConstants.DEFAULT_CONNECTION_TIMEOUT};" + - $"CharSet=utf8mb4;AllowLoadLocalInfile=false;MaxPoolSize={DbSeederConstants.DEFAULT_MAX_POOL_SIZE};"; + // SECURITY: Use MySqlConnectionStringBuilder to safely construct connection string + var builder = new MySqlConnectionStringBuilder + { + Server = _host, + Port = (uint)_port, + Database = _database, + UserID = _username, + Password = _password, + ConnectionTimeout = (uint)DbSeederConstants.DEFAULT_CONNECTION_TIMEOUT, + CharacterSet = "utf8mb4", + AllowLoadLocalInfile = false, + MaximumPoolSize = (uint)DbSeederConstants.DEFAULT_MAX_POOL_SIZE + }; - var actualConnectionString = safeConnectionString.Replace(DbSeederConstants.REDACTED_PASSWORD, _password); - - _connection = new MySqlConnection(actualConnectionString); + _connection = new MySqlConnection(builder.ConnectionString); _connection.Open(); _logger.LogInformation("Connected to MariaDB: {Host}:{Port}/{Database}", _host, _port, _database); diff --git a/util/Seeder/Migration/Databases/PostgresImporter.cs b/util/Seeder/Migration/Databases/PostgresImporter.cs index 1bf518a562..94708de1ba 100644 --- a/util/Seeder/Migration/Databases/PostgresImporter.cs +++ b/util/Seeder/Migration/Databases/PostgresImporter.cs @@ -27,13 +27,19 @@ public class PostgresImporter(DatabaseConfig config, ILogger l { try { - var safeConnectionString = $"Host={_host};Port={_port};Database={_database};" + - $"Username={_username};Password={DbSeederConstants.REDACTED_PASSWORD};" + - $"Timeout={DbSeederConstants.DEFAULT_CONNECTION_TIMEOUT};CommandTimeout={DbSeederConstants.DEFAULT_COMMAND_TIMEOUT};"; + // SECURITY: Use NpgsqlConnectionStringBuilder to safely construct connection string + var builder = new NpgsqlConnectionStringBuilder + { + Host = _host, + Port = _port, + Database = _database, + Username = _username, + Password = _password, + Timeout = DbSeederConstants.DEFAULT_CONNECTION_TIMEOUT, + CommandTimeout = DbSeederConstants.DEFAULT_COMMAND_TIMEOUT + }; - var actualConnectionString = safeConnectionString.Replace(DbSeederConstants.REDACTED_PASSWORD, _password); - - _connection = new NpgsqlConnection(actualConnectionString); + _connection = new NpgsqlConnection(builder.ConnectionString); _connection.Open(); _logger.LogInformation("Connected to PostgreSQL: {Host}:{Port}/{Database}", _host, _port, _database); @@ -302,6 +308,7 @@ public class PostgresImporter(DatabaseConfig config, ILogger l var paramNum = idx + 1; var colType = validColumnTypes[idx]; // Cast to appropriate type if needed - PostgreSQL requires explicit casts for text to other types + // SECURITY: Use explicit allowlist to prevent potential SQL injection through compromised schema return colType switch { // UUID types @@ -328,8 +335,27 @@ public class PostgresImporter(DatabaseConfig config, ILogger l // Boolean type "boolean" => $"${paramNum}::boolean", - // Default - no cast needed for text types - _ => $"${paramNum}" + // Text and string types (no cast needed) + "text" => $"${paramNum}", + "character varying" => $"${paramNum}", + "varchar" => $"${paramNum}", + "character" => $"${paramNum}", + "char" => $"${paramNum}", + + // Binary types + "bytea" => $"${paramNum}::bytea", + + // JSON types + "json" => $"${paramNum}::json", + "jsonb" => $"${paramNum}::jsonb", + + // Array types (common cases) + "text[]" => $"${paramNum}::text[]", + "integer[]" => $"${paramNum}::integer[]", + "uuid[]" => $"${paramNum}::uuid[]", + + // Reject unknown types to prevent potential schema-based attacks + _ => throw new InvalidOperationException($"Unsupported PostgreSQL column type '{colType}' for column '{col}'. Type must be explicitly allowed in the PostgresImporter allowlist.") }; }); var insertSql = $"INSERT INTO \"{actualTableName}\" ({string.Join(", ", quotedColumns)}) VALUES ({string.Join(", ", placeholders)})"; diff --git a/util/Seeder/Migration/Databases/SqlServerExporter.cs b/util/Seeder/Migration/Databases/SqlServerExporter.cs index 517b497f8f..666fe00bba 100644 --- a/util/Seeder/Migration/Databases/SqlServerExporter.cs +++ b/util/Seeder/Migration/Databases/SqlServerExporter.cs @@ -26,14 +26,18 @@ public class SqlServerExporter(DatabaseConfig config, ILogger { try { - var safeConnectionString = $"Server={_host},{_port};Database={_database};" + - $"User Id={_username};Password={DbSeederConstants.REDACTED_PASSWORD};" + - $"TrustServerCertificate=True;" + - $"Connection Timeout={DbSeederConstants.DEFAULT_CONNECTION_TIMEOUT};"; + // SECURITY: Use SqlConnectionStringBuilder to safely construct connection string + var builder = new SqlConnectionStringBuilder + { + DataSource = $"{_host},{_port}", + InitialCatalog = _database, + UserID = _username, + Password = _password, + TrustServerCertificate = true, + ConnectTimeout = DbSeederConstants.DEFAULT_CONNECTION_TIMEOUT + }; - var actualConnectionString = safeConnectionString.Replace(DbSeederConstants.REDACTED_PASSWORD, _password); - - _connection = new SqlConnection(actualConnectionString); + _connection = new SqlConnection(builder.ConnectionString); _connection.Open(); _logger.LogInformation("Connected to SQL Server: {Host}/{Database}", _host, _database); @@ -320,12 +324,14 @@ public class SqlServerExporter(DatabaseConfig config, ILogger var whereClause = string.Join(" OR ", textColumns.Select(col => $"[{col}] IS NOT NULL")); + // SECURITY: Use parameterized query for TOP clause to maintain consistent security patterns var query = $@" - SELECT TOP {sampleSize} {columnList} + SELECT TOP (@SampleSize) {columnList} FROM [{tableName}] WHERE {whereClause}"; using var command = new SqlCommand(query, _connection); + command.Parameters.AddWithValue("@SampleSize", sampleSize); using var reader = command.ExecuteReader(); var sampleData = new List(); diff --git a/util/Seeder/Migration/Databases/SqlServerImporter.cs b/util/Seeder/Migration/Databases/SqlServerImporter.cs index 126ecece01..548526f46f 100644 --- a/util/Seeder/Migration/Databases/SqlServerImporter.cs +++ b/util/Seeder/Migration/Databases/SqlServerImporter.cs @@ -28,14 +28,18 @@ public class SqlServerImporter(DatabaseConfig config, ILogger { try { - var safeConnectionString = $"Server={_host},{_port};Database={_database};" + - $"User Id={_username};Password={DbSeederConstants.REDACTED_PASSWORD};" + - $"TrustServerCertificate=True;" + - $"Connection Timeout={DbSeederConstants.DEFAULT_CONNECTION_TIMEOUT};"; + // SECURITY: Use SqlConnectionStringBuilder to safely construct connection string + var builder = new SqlConnectionStringBuilder + { + DataSource = $"{_host},{_port}", + InitialCatalog = _database, + UserID = _username, + Password = _password, + TrustServerCertificate = true, + ConnectTimeout = DbSeederConstants.DEFAULT_CONNECTION_TIMEOUT + }; - var actualConnectionString = safeConnectionString.Replace(DbSeederConstants.REDACTED_PASSWORD, _password); - - _connection = new SqlConnection(actualConnectionString); + _connection = new SqlConnection(builder.ConnectionString); _connection.Open(); _logger.LogInformation("Connected to SQL Server: {Host}/{Database}", _host, _database); diff --git a/util/Seeder/Migration/Utils/IdentifierValidator.cs b/util/Seeder/Migration/Utils/IdentifierValidator.cs index 874abaecb4..2ee326d5cb 100644 --- a/util/Seeder/Migration/Utils/IdentifierValidator.cs +++ b/util/Seeder/Migration/Utils/IdentifierValidator.cs @@ -14,15 +14,34 @@ public static class IdentifierValidator RegexOptions.Compiled ); + // Regex pattern for more restrictive validation (no leading underscores) + private static readonly Regex RestrictiveIdentifierPattern = new Regex( + @"^[a-zA-Z][a-zA-Z0-9_]*$", + RegexOptions.Compiled + ); + // Maximum reasonable length for identifiers (most databases have limits around 128-255) private const int MaxIdentifierLength = 128; + // SQL reserved keywords that should be rejected (common across SQL Server and PostgreSQL) + // SECURITY: Prevent identifiers that match SQL keywords to avoid injection attempts + private static readonly HashSet SqlReservedKeywords = new HashSet(StringComparer.OrdinalIgnoreCase) + { + "SELECT", "INSERT", "UPDATE", "DELETE", "DROP", "CREATE", "ALTER", "EXEC", "EXECUTE", + "TABLE", "DATABASE", "SCHEMA", "INDEX", "VIEW", "PROCEDURE", "FUNCTION", "TRIGGER", + "WHERE", "FROM", "JOIN", "UNION", "ORDER", "GROUP", "HAVING", "INTO", "VALUES", + "SET", "AND", "OR", "NOT", "NULL", "IS", "AS", "ON", "IN", "EXISTS", "BETWEEN", + "LIKE", "CASE", "WHEN", "THEN", "ELSE", "END", "BEGIN", "COMMIT", "ROLLBACK", + "GRANT", "REVOKE", "DECLARE", "CAST", "CONVERT", "TRUNCATE", "MERGE" + }; + /// /// Validates a SQL identifier (table name, column name, schema name). /// /// The identifier to validate + /// If true, disallow leading underscores and SQL reserved keywords /// True if the identifier is valid, false otherwise - public static bool IsValid(string? identifier) + public static bool IsValid(string? identifier, bool useRestrictiveMode = false) { if (string.IsNullOrWhiteSpace(identifier)) return false; @@ -30,7 +49,13 @@ public static class IdentifierValidator if (identifier.Length > MaxIdentifierLength) return false; - return ValidIdentifierPattern.IsMatch(identifier); + // SECURITY: In restrictive mode, reject SQL reserved keywords + if (useRestrictiveMode && SqlReservedKeywords.Contains(identifier)) + return false; + + // Use appropriate pattern based on mode + var pattern = useRestrictiveMode ? RestrictiveIdentifierPattern : ValidIdentifierPattern; + return pattern.IsMatch(identifier); } /// @@ -38,15 +63,35 @@ public static class IdentifierValidator /// /// The identifier to validate /// The type of identifier (e.g., "table name", "column name") + /// If true, disallow leading underscores and SQL reserved keywords /// Thrown when the identifier is invalid - public static void ValidateOrThrow(string? identifier, string identifierType = "identifier") + public static void ValidateOrThrow(string? identifier, string identifierType = "identifier", bool useRestrictiveMode = false) { - if (!IsValid(identifier)) + if (string.IsNullOrWhiteSpace(identifier)) { + throw new ArgumentException( + $"Invalid {identifierType}: identifier is null or empty.", + nameof(identifier) + ); + } + + if (useRestrictiveMode && SqlReservedKeywords.Contains(identifier)) + { + throw new ArgumentException( + $"Invalid {identifierType}: '{identifier}' is a SQL reserved keyword.", + nameof(identifier) + ); + } + + if (!IsValid(identifier, useRestrictiveMode)) + { + var requirements = useRestrictiveMode + ? "must start with a letter (no underscores), contain only letters, numbers, and underscores" + : "must start with a letter or underscore, contain only letters, numbers, and underscores"; + throw new ArgumentException( $"Invalid {identifierType}: '{identifier}'. " + - $"Identifiers must start with a letter or underscore, " + - $"contain only letters, numbers, and underscores, " + + $"Identifiers {requirements}, " + $"and be no longer than {MaxIdentifierLength} characters.", nameof(identifier) ); @@ -58,12 +103,13 @@ public static class IdentifierValidator /// /// The identifiers to validate /// The type of identifiers (e.g., "column names") + /// If true, disallow leading underscores and SQL reserved keywords /// Thrown when any identifier is invalid - public static void ValidateAllOrThrow(IEnumerable identifiers, string identifierType = "identifiers") + public static void ValidateAllOrThrow(IEnumerable identifiers, string identifierType = "identifiers", bool useRestrictiveMode = false) { foreach (var identifier in identifiers) { - ValidateOrThrow(identifier, identifierType); + ValidateOrThrow(identifier, identifierType, useRestrictiveMode); } } @@ -71,9 +117,10 @@ public static class IdentifierValidator /// Filters a list of identifiers to only include valid ones. /// /// The identifiers to filter + /// If true, disallow leading underscores and SQL reserved keywords /// A list of valid identifiers - public static List FilterValid(IEnumerable identifiers) + public static List FilterValid(IEnumerable identifiers, bool useRestrictiveMode = false) { - return identifiers.Where(IsValid).ToList(); + return identifiers.Where(id => IsValid(id, useRestrictiveMode)).ToList(); } }