From 020c8c2fb3804af280049269c3efc6de769f3b41 Mon Sep 17 00:00:00 2001 From: Ramon Smits Date: Fri, 3 Jul 2026 08:36:39 +0200 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20Improve=20SQL=20Server=20connection?= =?UTF-8?q?=20failure=20diagnostics?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the sqlserver command failed to connect, the tool printed the same generic message twice and hid the real SqlException. Now: - Surface SqlException Number, State, Class, Server, and all errors in the failure message - Echo the effective connection identity (Data Source, Initial Catalog, Integrated Security, Windows identity) before probing, never secrets - Unwrap HaltException to the innermost exception when printing and dedupe repeated messages - Always write a redacted diagnostics log file (full exception chain, Password/PWD blanked) and report wrapped errors, including with --unattended - Harden --connectionStringSource parsing: trim whitespace, strip wrapping quotes, and validate each line, reporting the 1-based line number on parse failure --- src/AppCommon/Commands/BaseCommand.cs | 10 +- src/AppCommon/Commands/SqlServerCommand.cs | 65 ++++++++- .../Infra/ConnectionStringSanitizer.cs | 36 +++++ src/AppCommon/Infra/Exceptions.cs | 35 ++++- src/Query/SqlTransport/DatabaseDetails.cs | 26 +++- .../SqlServer/ConnectionStringSourceTests.cs | 131 ++++++++++++++++++ 6 files changed, 293 insertions(+), 10 deletions(-) create mode 100644 src/AppCommon/Infra/ConnectionStringSanitizer.cs create mode 100644 src/Tests/SqlServer/ConnectionStringSourceTests.cs diff --git a/src/AppCommon/Commands/BaseCommand.cs b/src/AppCommon/Commands/BaseCommand.cs index c3929a03..bb80e589 100644 --- a/src/AppCommon/Commands/BaseCommand.cs +++ b/src/AppCommon/Commands/BaseCommand.cs @@ -70,8 +70,14 @@ public async Task Run(CancellationToken cancellationToken = default) Out.WriteError(halt.Message); if (halt.InnerException != null) { - Out.WriteLine(); - Out.WriteLine("Original exception message: " + halt.InnerException.Message); + var original = halt.GetBaseException(); + if (original != halt && !halt.Message.Contains(original.Message)) + { + Out.WriteLine(); + Out.WriteLine("Original exception message: " + original.Message); + } + + Exceptions.ReportError(halt); } Environment.ExitCode = halt.ExitCode; } diff --git a/src/AppCommon/Commands/SqlServerCommand.cs b/src/AppCommon/Commands/SqlServerCommand.cs index e64f3adb..26125265 100644 --- a/src/AppCommon/Commands/SqlServerCommand.cs +++ b/src/AppCommon/Commands/SqlServerCommand.cs @@ -31,11 +31,21 @@ public static Command CreateCommand() command.SetHandler(async context => { var shared = SharedOptions.Parse(context); - var connectionStrings = GetConnectionStrings(context.ParseResult); var cancellationToken = context.GetCancellationToken(); - var runner = new SqlServerCommand(shared, connectionStrings); - await runner.Run(cancellationToken); + try + { + var connectionStrings = GetConnectionStrings(context.ParseResult); + + var runner = new SqlServerCommand(shared, connectionStrings); + await runner.Run(cancellationToken); + } + catch (HaltException halt) + { + Out.WriteLine(); + Out.WriteError(halt.Message); + Environment.ExitCode = halt.ExitCode; + } }); return command; @@ -56,9 +66,7 @@ static string[] GetConnectionStrings(ParseResult parsed) throw new FileNotFoundException($"Could not find file specified by {ConnectionStringSource.Name} parameter", sourcePath); } - return File.ReadAllLines(sourcePath) - .Where(line => !string.IsNullOrWhiteSpace(line)) - .ToArray(); + return ParseConnectionStringSource(File.ReadAllLines(sourcePath), sourcePath); } var single = parsed.GetValueForOption(ConnectionString); @@ -92,6 +100,44 @@ static string[] GetConnectionStrings(ParseResult parsed) return list.ToArray(); } + internal static string[] ParseConnectionStringSource(string[] lines, string sourcePath) + { + var connectionStrings = new List(); + + for (var i = 0; i < lines.Length; i++) + { + var line = lines[i].Trim(); + + if (string.IsNullOrWhiteSpace(line)) + { + continue; + } + + if (line.Length >= 2 && ((line[0] == '"' && line[^1] == '"') || (line[0] == '\'' && line[^1] == '\''))) + { + line = line[1..^1].Trim(); + } + + try + { + _ = new SqlConnectionStringBuilder(line); + } + catch (Exception x) when (x is FormatException or ArgumentException or KeyNotFoundException) + { + throw new HaltException(HaltReason.InvalidConfig, $"ERROR: Line {i + 1} of '{sourcePath}' could not be parsed as a SQL Server connection string: {x.Message}"); + } + + connectionStrings.Add(line); + } + + if (connectionStrings.Count == 0) + { + throw new HaltException(HaltReason.InvalidConfig, $"ERROR: The file '{sourcePath}' does not contain any connection strings."); + } + + return connectionStrings.ToArray(); + } + readonly string[] connectionStrings; DatabaseDetails[] databases; string scopeType; @@ -111,6 +157,13 @@ protected override async Task GetEnvironment(CancellationTok foreach (var db in databases) { + Out.WriteLine($"Testing connection to server '{db.DataSource}', database '{db.DatabaseName ?? "(default)"}', Integrated Security={(db.IntegratedSecurity ? "true" : "false")}..."); + if (db.IntegratedSecurity && OperatingSystem.IsWindows()) + { + using var identity = System.Security.Principal.WindowsIdentity.GetCurrent(); + Out.WriteLine($" - Connecting as Windows identity '{identity.Name}'"); + } + await db.TestConnection(cancellationToken); } diff --git a/src/AppCommon/Infra/ConnectionStringSanitizer.cs b/src/AppCommon/Infra/ConnectionStringSanitizer.cs new file mode 100644 index 00000000..e409f5bd --- /dev/null +++ b/src/AppCommon/Infra/ConnectionStringSanitizer.cs @@ -0,0 +1,36 @@ +using System.Text.RegularExpressions; +using Microsoft.Data.SqlClient; + +static partial class ConnectionStringSanitizer +{ + const string Mask = "*****"; + + [GeneratedRegex("""(?\b(?:password|pwd))\s*=\s*(?:'[^']*'|"[^"]*"|[^;]*)""", RegexOptions.IgnoreCase)] + private static partial Regex SecretRegex(); + + /// + /// Returns the connection string with the Password/PWD value blanked so it can be safely echoed or logged. + /// + public static string Sanitize(string connectionString) + { + try + { + var builder = new SqlConnectionStringBuilder { ConnectionString = connectionString }; + if (!string.IsNullOrEmpty(builder.Password)) + { + builder.Password = Mask; + } + return builder.ToString(); + } + catch (Exception x) when (x is FormatException or ArgumentException) + { + // Not parseable as a connection string, fall back to pattern-based redaction + return RedactText(connectionString); + } + } + + /// + /// Redacts Password/PWD values in free-form text, such as an exception dump that may embed a connection string. + /// + public static string RedactText(string text) => text is null ? null : SecretRegex().Replace(text, $"${{key}}={Mask}"); +} diff --git a/src/AppCommon/Infra/Exceptions.cs b/src/AppCommon/Infra/Exceptions.cs index bffd641c..3c53db85 100644 --- a/src/AppCommon/Infra/Exceptions.cs +++ b/src/AppCommon/Infra/Exceptions.cs @@ -38,6 +38,8 @@ public static void SetupUnhandledExceptionHandling() public static void ReportError(Exception x) { + WriteDiagnosticsLog(x); + var settings = new RaygunSettings() { ApplicationVersion = Versioning.NuGetVersion @@ -45,7 +47,7 @@ public static void ReportError(Exception x) RunInfo.Add("ToolOutput", Out.GetToolOutput()); - if (x is SqlException sqlX) + if (FindInChain(x) is SqlException sqlX) { RunInfo.Add("SqlException.Number", sqlX.Number.ToString()); if (sqlX.Errors is not null) @@ -82,4 +84,35 @@ public static void ReportError(Exception x) } } + + /// + /// Writes the full (redacted) exception chain to a local log file so support can diagnose failures + /// without a live debugger, including when running with --unattended. + /// + static void WriteDiagnosticsLog(Exception x) + { + try + { + var path = Path.Join(Environment.CurrentDirectory, $"throughput-diagnostics-{DateTime.Now:yyyyMMdd-HHmmss}.log"); + File.WriteAllText(path, ConnectionStringSanitizer.RedactText(x.ToString())); + Console.WriteLine($"Diagnostic details written to {path}"); + } + catch (Exception logX) when (logX is IOException or UnauthorizedAccessException) + { + Console.WriteLine($"Unable to write diagnostics log: {logX.Message}"); + } + } + + static T FindInChain(Exception x) where T : Exception + { + while (x is not null) + { + if (x is T match) + { + return match; + } + x = x.InnerException; + } + return null; + } } diff --git a/src/Query/SqlTransport/DatabaseDetails.cs b/src/Query/SqlTransport/DatabaseDetails.cs index 32c2cb69..8c5ba54f 100644 --- a/src/Query/SqlTransport/DatabaseDetails.cs +++ b/src/Query/SqlTransport/DatabaseDetails.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; using System.Linq; + using System.Text; using System.Threading; using System.Threading.Tasks; using Microsoft.Data.SqlClient; @@ -12,6 +13,8 @@ public class DatabaseDetails readonly string connectionString; public string DatabaseName { get; } + public string DataSource { get; } + public bool IntegratedSecurity { get; } public List Tables { get; private set; } public int ErrorCount { get; private set; } @@ -21,6 +24,8 @@ public DatabaseDetails(string connectionString) { var builder = new SqlConnectionStringBuilder { ConnectionString = connectionString, TrustServerCertificate = true }; DatabaseName = builder["Initial Catalog"] as string ?? builder["Database"] as string; + DataSource = builder.DataSource; + IntegratedSecurity = builder.IntegratedSecurity; this.connectionString = builder.ToString(); } catch (Exception x) when (x is FormatException or ArgumentException) @@ -39,10 +44,29 @@ public async Task TestConnection(CancellationToken cancellationToken = default) } catch (SqlException x) when (IsConnectionOrLoginIssue(x)) { - throw new QueryException(QueryFailureReason.Auth, "Could not access SQL database. Is the connection string correct?", x); + throw new QueryException(QueryFailureReason.Auth, BuildConnectionIssueMessage(x), x); } } + string BuildConnectionIssueMessage(SqlException x) + { + var serverName = string.IsNullOrEmpty(x.Server) ? DataSource : x.Server; + + var message = new StringBuilder() + .AppendLine($"SQL error {x.Number} (state {x.State}, class {x.Class}) from {serverName}: {x.Message}"); + + if (x.Errors is not null && x.Errors.Count > 1) + { + for (var i = 0; i < x.Errors.Count; i++) + { + var err = x.Errors[i]; + _ = message.AppendLine($" - SQL error {err.Number} (state {err.State}, class {err.Class}): {err.Message}"); + } + } + + return message.ToString().TrimEnd(); + } + static bool IsConnectionOrLoginIssue(SqlException x) { // Reference is here: https://learn.microsoft.com/en-us/previous-versions/sql/sql-server-2008-r2/cc645603(v=sql.105)?redirectedfrom=MSDN diff --git a/src/Tests/SqlServer/ConnectionStringSourceTests.cs b/src/Tests/SqlServer/ConnectionStringSourceTests.cs new file mode 100644 index 00000000..3900c9dd --- /dev/null +++ b/src/Tests/SqlServer/ConnectionStringSourceTests.cs @@ -0,0 +1,131 @@ +namespace Tests.SqlServer +{ + using NUnit.Framework; + + [TestFixture] + public class ConnectionStringSourceTests + { + const string SourcePath = "connection-strings.txt"; + + [Test] + public void Parses_valid_lines_and_skips_blank_lines() + { + var lines = new[] + { + "", + "Server=srv1;Database=db1", + " ", + "Server=srv2;Database=db2", + "" + }; + + var result = SqlServerCommand.ParseConnectionStringSource(lines, SourcePath); + + Assert.That(result, Is.EqualTo(new[] { "Server=srv1;Database=db1", "Server=srv2;Database=db2" })); + } + + [TestCase("\"Server=srv;Database=db\"")] + [TestCase("'Server=srv;Database=db'")] + [TestCase(" \"Server=srv;Database=db\" ")] + public void Strips_a_single_pair_of_wrapping_quotes(string line) + { + var result = SqlServerCommand.ParseConnectionStringSource(new[] { line }, SourcePath); + + Assert.That(result, Is.EqualTo(new[] { "Server=srv;Database=db" })); + } + + [Test] + public void Reports_one_based_line_number_and_parse_error_for_invalid_line() + { + var lines = new[] + { + "Server=srv1;Database=db1", + "this is not a connection string" + }; + + var halt = Assert.Throws(() => SqlServerCommand.ParseConnectionStringSource(lines, SourcePath)); + + Assert.That(halt.ExitCode, Is.EqualTo((int)HaltReason.InvalidConfig)); + Assert.That(halt.Message, Does.Contain("Line 2")); + Assert.That(halt.Message, Does.Contain(SourcePath)); + } + + [Test] + public void Reports_invalid_keyword_parse_error() + { + var halt = Assert.Throws(() => SqlServerCommand.ParseConnectionStringSource(new[] { "NotAKeyword=value" }, SourcePath)); + + Assert.That(halt.ExitCode, Is.EqualTo((int)HaltReason.InvalidConfig)); + Assert.That(halt.Message, Does.Contain("Line 1")); + } + + [Test] + public void Throws_when_file_contains_no_connection_strings() + { + var halt = Assert.Throws(() => SqlServerCommand.ParseConnectionStringSource(new[] { "", " " }, SourcePath)); + + Assert.That(halt.ExitCode, Is.EqualTo((int)HaltReason.InvalidConfig)); + } + } + + [TestFixture] + public class ConnectionStringSanitizerTests + { + [TestCase("Server=srv;Database=db;User ID=user;Password=s3cret!")] + [TestCase("Server=srv;Database=db;User ID=user;Pwd=s3cret!")] + [TestCase("Server=srv;Database=db;User ID=user;Password='s3cret!;more'")] + public void Sanitize_blanks_the_password(string connectionString) + { + var sanitized = ConnectionStringSanitizer.Sanitize(connectionString); + + Assert.That(sanitized, Does.Not.Contain("s3cret!")); + Assert.That(sanitized, Does.Contain("srv")); + Assert.That(sanitized, Does.Contain("user")); + } + + [Test] + public void Sanitize_keeps_connection_string_without_password_intact() + { + var sanitized = ConnectionStringSanitizer.Sanitize("Server=srv;Database=db;Integrated Security=True"); + + Assert.That(sanitized, Does.Contain("srv")); + Assert.That(sanitized, Does.Not.Contain("*****")); + } + + [Test] + public void Sanitize_falls_back_to_redaction_for_unparseable_input() + { + var sanitized = ConnectionStringSanitizer.Sanitize("some garbage ;; Password=s3cret!;more garbage"); + + Assert.That(sanitized, Does.Not.Contain("s3cret!")); + Assert.That(sanitized, Does.Contain("*****")); + } + + [TestCase("Login failed. Connection: Server=x;Password=abc123;Encrypt=true", "abc123")] + [TestCase("Connection: Server=x;Pwd=abc123;Encrypt=true", "abc123")] + [TestCase("Connection: Server=x;Password='se;cret';Encrypt=true", "se;cret")] + [TestCase("Connection: Server=x;Password=\"se;cret\";Encrypt=true", "se;cret")] + [TestCase("Connection: Server=x; Password = abc123 ;Encrypt=true", "abc123")] + public void RedactText_masks_secrets_in_free_form_text(string text, string secret) + { + var redacted = ConnectionStringSanitizer.RedactText(text); + + Assert.That(redacted, Does.Not.Contain(secret)); + Assert.That(redacted, Does.Contain("*****")); + } + + [Test] + public void RedactText_leaves_text_without_secrets_untouched() + { + const string text = "SQL error 18456 (state 38) from MYSERVER: Login failed for user 'sa'."; + + Assert.That(ConnectionStringSanitizer.RedactText(text), Is.EqualTo(text)); + } + + [Test] + public void RedactText_handles_null() + { + Assert.That(ConnectionStringSanitizer.RedactText(null), Is.Null); + } + } +}