From 7fbdf3e129bf8ef726cd3b8def5cf8a39ec04dd5 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Tue, 21 Apr 2026 17:09:21 -0400 Subject: [PATCH 1/6] fix: XSS, DPAPI scope, and rate-limiter key eviction (#249) - PlanShare dashboard: rebuild referrer table with DOM + textContent instead of innerHTML, so legacy rows containing hostile strings can't execute script. - PlanShare /api/event: drop the referrer entirely when Uri.TryCreate rejects it, instead of persisting the raw client-supplied string. - WindowsCredentialService: save credentials with Enterprise scope (current-user, DPAPI-encrypted) instead of LocalMachine (readable by every user on the box). - PlanShare RateLimiter: add Sweep() that prunes and TryRemoves empty keys, invoked from CleanupService's existing hourly tick so the dictionary no longer grows unbounded across unique IPs. Co-authored-by: Claude Opus 4.7 (1M context) --- server/PlanShare/Program.cs | 57 ++++++++++++++++--- server/PlanShare/dashboard.html | 23 ++++++-- .../Services/WindowsCredentialService.cs | 2 +- 3 files changed, 69 insertions(+), 13 deletions(-) diff --git a/server/PlanShare/Program.cs b/server/PlanShare/Program.cs index f04964a..097fea3 100644 --- a/server/PlanShare/Program.cs +++ b/server/PlanShare/Program.cs @@ -44,8 +44,14 @@ created_at TEXT NOT NULL cmd.ExecuteNonQuery(); } +// --- Rate limiters (in-memory) --- +// Created before Build() so they can be DI-registered and swept by CleanupService. +var rateLimiter = new RateLimiter(maxRequests: 10, windowSeconds: 60); +var analyticsRateLimiter = new RateLimiter(maxRequests: 30, windowSeconds: 60); + // Register the cleanup background service builder.Services.AddSingleton(new PlanDbConfig(connectionString)); +builder.Services.AddSingleton(new RateLimiters(rateLimiter, analyticsRateLimiter)); builder.Services.AddHostedService(); // Request size limit (10 MB) @@ -54,10 +60,6 @@ created_at TEXT NOT NULL var app = builder.Build(); app.UseCors(); -// --- Rate limiters (in-memory) --- -var rateLimiter = new RateLimiter(maxRequests: 10, windowSeconds: 60); -var analyticsRateLimiter = new RateLimiter(maxRequests: 30, windowSeconds: 60); - const int MaxTtlDays = 365; // --- Endpoints --- @@ -161,9 +163,15 @@ created_at TEXT NOT NULL return Results.BadRequest("Invalid JSON"); } - // Strip referrer to domain only (no full URLs with query params) - if (!string.IsNullOrEmpty(referrer) && Uri.TryCreate(referrer, UriKind.Absolute, out var refUri)) - referrer = refUri.Host; + // Strip referrer to domain only (no full URLs with query params). + // If it doesn't parse as an absolute URL, drop it — never persist raw + // client-supplied strings, since the dashboard renders referrers in HTML. + if (!string.IsNullOrEmpty(referrer)) + { + referrer = Uri.TryCreate(referrer, UriKind.Absolute, out var refUri) + ? refUri.Host + : null; + } // Visitor hash: SHA256(IP + User-Agent + date) — unique per day, no PII stored var ua = ctx.Request.Headers.UserAgent.FirstOrDefault() ?? ""; @@ -352,14 +360,18 @@ static string GenerateDeleteToken() record PlanDbConfig(string ConnectionString); +record RateLimiters(RateLimiter Share, RateLimiter Analytics); + sealed class CleanupService : BackgroundService { private readonly PlanDbConfig _config; + private readonly RateLimiters _rateLimiters; private readonly ILogger _logger; - public CleanupService(PlanDbConfig config, ILogger logger) + public CleanupService(PlanDbConfig config, RateLimiters rateLimiters, ILogger logger) { _config = config; + _rateLimiters = rateLimiters; _logger = logger; } @@ -401,6 +413,14 @@ private void Cleanup() if (deleted > 0) _logger.LogInformation("Cleaned up {Count} old page views", deleted); } + + // Evict stale rate-limiter keys so the dictionary doesn't grow forever. + var shareEvicted = _rateLimiters.Share.Sweep(); + var analyticsEvicted = _rateLimiters.Analytics.Sweep(); + if (shareEvicted + analyticsEvicted > 0) + _logger.LogInformation( + "Evicted {Share} share + {Analytics} analytics rate-limit keys", + shareEvicted, analyticsEvicted); } catch (Exception ex) { @@ -441,4 +461,25 @@ public bool IsAllowed(string key) return true; } } + + /// + /// Evicts keys whose timestamp lists have gone empty. Call periodically + /// so the dictionary doesn't grow forever across unique IPs. + /// Returns the number of keys evicted. + /// + public int Sweep() + { + var cutoff = DateTime.UtcNow.AddSeconds(-_windowSeconds); + var evicted = 0; + foreach (var kvp in _requests) + { + lock (kvp.Value) + { + kvp.Value.RemoveAll(t => t < cutoff); + if (kvp.Value.Count == 0 && _requests.TryRemove(kvp)) + evicted++; + } + } + return evicted; + } } diff --git a/server/PlanShare/dashboard.html b/server/PlanShare/dashboard.html index b444890..f0a7fd5 100644 --- a/server/PlanShare/dashboard.html +++ b/server/PlanShare/dashboard.html @@ -275,13 +275,28 @@

Top Referrers (30 days)

} function updateReferrers() { var tbody = document.querySelector("#referrersTable tbody"); + tbody.replaceChildren(); if (!planStats || !planStats.traffic.top_referrers.length) { - tbody.innerHTML = "No referrer data yet"; + var tr = document.createElement("tr"); + var td = document.createElement("td"); + td.setAttribute("colspan", "2"); + td.style.color = "#484f58"; + td.textContent = "No referrer data yet"; + tr.appendChild(td); + tbody.appendChild(tr); return; } - tbody.innerHTML = planStats.traffic.top_referrers.map(function(r) { - return "" + r.referrer + "" + fmt(r.count) + ""; - }).join(""); + planStats.traffic.top_referrers.forEach(function(r) { + var tr = document.createElement("tr"); + var tdRef = document.createElement("td"); + tdRef.textContent = r.referrer; + var tdCount = document.createElement("td"); + tdCount.className = "num"; + tdCount.textContent = fmt(r.count); + tr.appendChild(tdRef); + tr.appendChild(tdCount); + tbody.appendChild(tr); + }); } function updateCharts() { updateChart("starsChart", "line", { diff --git a/src/PlanViewer.Core/Services/WindowsCredentialService.cs b/src/PlanViewer.Core/Services/WindowsCredentialService.cs index 2581f99..99c96aa 100644 --- a/src/PlanViewer.Core/Services/WindowsCredentialService.cs +++ b/src/PlanViewer.Core/Services/WindowsCredentialService.cs @@ -16,7 +16,7 @@ public bool SaveCredential(string serverId, string username, string password) userName: username, secret: password, comment: "planview credential", - persistence: CredentialPersistence.LocalMachine); + persistence: CredentialPersistence.Enterprise); return true; } catch { return false; } From 719d861e0bc90b656cf33af43ebc65167ff09e2c Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Tue, 21 Apr 2026 18:07:03 -0400 Subject: [PATCH 2/6] =?UTF-8?q?fix:=20bug-bash=20batch=202=20=E2=80=94=20s?= =?UTF-8?q?ix=20small=20correctness/hygiene=20fixes=20(#250)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ReproScriptBuilder: double ']' in database names before emitting USE, so identifiers like `cool]stuff` produce valid T-SQL instead of unbalanced brackets. - ReproScriptBuilder: whitelist SET TRANSACTION ISOLATION LEVEL against the five valid T-SQL names; unknown/hostile strings drop silently instead of interpolating into the script. - ShowPlanParser.ParseLong: pass NumberStyles.Integer + InvariantCulture to match sibling ParseDouble; makes numeric parsing culture-independent. - KeychainCredentialService: read stdout and stderr concurrently via Task.WhenAll to avoid the classic "sync stderr while stdout is async" deadlock, which could surface on `security dump-keychain` with large keychains. - QueryStoreHistoryWindow: override OnClosed to cancel + dispose the in-flight fetch CancellationTokenSource, so SqlConnection doesn't sit open on the server when the dialog is closed mid-load. - QuerySessionControl: on DetachedFromVisualTree, cancel + dispose _statusClearCts alongside the existing TextMate teardown, so the fire-and-forget status-clear dispatch can't touch a dead control. Co-authored-by: Claude Opus 4.7 (1M context) --- .../Controls/QuerySessionControl.axaml.cs | 6 ++++- .../Dialogs/QueryStoreHistoryWindow.axaml.cs | 10 ++++++++ .../Services/KeychainCredentialService.cs | 9 +++++--- .../Services/ReproScriptBuilder.cs | 23 ++++++++++++++++--- .../Services/ShowPlanParser.cs | 3 ++- 5 files changed, 43 insertions(+), 8 deletions(-) diff --git a/src/PlanViewer.App/Controls/QuerySessionControl.axaml.cs b/src/PlanViewer.App/Controls/QuerySessionControl.axaml.cs index 0c331f0..78ab33a 100644 --- a/src/PlanViewer.App/Controls/QuerySessionControl.axaml.cs +++ b/src/PlanViewer.App/Controls/QuerySessionControl.axaml.cs @@ -78,11 +78,15 @@ public QuerySessionControl(ICredentialService credentialService, ConnectionStore QueryEditor.TextArea.Focus(); }; - // Dispose TextMate when detached (e.g. tab switch) to release renderers/transformers + // Dispose TextMate when detached (e.g. tab switch) to release renderers/transformers. + // Also cancel any in-flight status-clear dispatch so it doesn't fire on a dead control. DetachedFromVisualTree += (_, _) => { _textMateInstallation?.Dispose(); _textMateInstallation = null; + _statusClearCts?.Cancel(); + _statusClearCts?.Dispose(); + _statusClearCts = null; }; // Focus the editor when the Editor tab is selected; toggle plan-dependent buttons diff --git a/src/PlanViewer.App/Dialogs/QueryStoreHistoryWindow.axaml.cs b/src/PlanViewer.App/Dialogs/QueryStoreHistoryWindow.axaml.cs index ca5d9b1..b39825a 100644 --- a/src/PlanViewer.App/Dialogs/QueryStoreHistoryWindow.axaml.cs +++ b/src/PlanViewer.App/Dialogs/QueryStoreHistoryWindow.axaml.cs @@ -169,6 +169,16 @@ public static string MapOrderByToMetricTag(string orderBy) : "AvgCpuMs"; } + protected override void OnClosed(EventArgs e) + { + // Cancel any in-flight history fetch so the SqlConnection doesn't sit open on the + // server after the dialog is dismissed. + _fetchCts?.Cancel(); + _fetchCts?.Dispose(); + _fetchCts = null; + base.OnClosed(e); + } + private async System.Threading.Tasks.Task LoadHistoryAsync() { _fetchCts?.Cancel(); diff --git a/src/PlanViewer.Core/Services/KeychainCredentialService.cs b/src/PlanViewer.Core/Services/KeychainCredentialService.cs index 4e1f9cb..772eceb 100644 --- a/src/PlanViewer.Core/Services/KeychainCredentialService.cs +++ b/src/PlanViewer.Core/Services/KeychainCredentialService.cs @@ -103,11 +103,14 @@ private static (int ExitCode, string Output) RunSecurity(params string[] args) using var process = Process.Start(psi); if (process == null) return (-1, string.Empty); + // Read both streams concurrently. Doing stderr synchronously while stdout is + // async can deadlock if stderr fills its pipe buffer before the process exits + // (reproduces on `security dump-keychain` with large keychains). var stdoutTask = process.StandardOutput.ReadToEndAsync(); - var stderr = process.StandardError.ReadToEnd(); + var stderrTask = process.StandardError.ReadToEndAsync(); process.WaitForExit(); - var stdout = stdoutTask.Result; + Task.WaitAll(stdoutTask, stderrTask); - return (process.ExitCode, stdout + stderr); + return (process.ExitCode, stdoutTask.Result + stderrTask.Result); } } diff --git a/src/PlanViewer.Core/Services/ReproScriptBuilder.cs b/src/PlanViewer.Core/Services/ReproScriptBuilder.cs index aa1136b..8b94f09 100644 --- a/src/PlanViewer.Core/Services/ReproScriptBuilder.cs +++ b/src/PlanViewer.Core/Services/ReproScriptBuilder.cs @@ -20,6 +20,20 @@ namespace PlanViewer.Core.Services; /// public static class ReproScriptBuilder { + /// + /// Valid T-SQL isolation level names (uppercase) that may appear in a SET TRANSACTION + /// ISOLATION LEVEL statement. Used to gate interpolation so arbitrary upstream strings + /// can't land in the generated script. + /// + private static readonly HashSet IsolationLevels = new(StringComparer.Ordinal) + { + "READ UNCOMMITTED", + "READ COMMITTED", + "REPEATABLE READ", + "SNAPSHOT", + "SERIALIZABLE", + }; + /// /// Builds a complete reproduction script from available query data. /// @@ -94,10 +108,11 @@ public static string BuildReproScript( sb.AppendLine("*/"); sb.AppendLine(); - /* USE database (skip for Azure SQL DB — USE is invalid there) */ + /* USE database (skip for Azure SQL DB — USE is invalid there). + Double any ']' in the identifier so names like 'cool]stuff' still parse. */ if (!string.IsNullOrEmpty(databaseName) && !isAzureSqlDb) { - sb.AppendLine($"USE [{databaseName}];"); + sb.AppendLine($"USE [{databaseName.Replace("]", "]]")}];"); sb.AppendLine(); } @@ -116,7 +131,9 @@ public static string BuildReproScript( if (!string.IsNullOrEmpty(isolationLevel)) { - sb.AppendLine($"SET TRANSACTION ISOLATION LEVEL {isolationLevel.ToUpperInvariant()};"); + var upper = isolationLevel.ToUpperInvariant(); + if (IsolationLevels.Contains(upper)) + sb.AppendLine($"SET TRANSACTION ISOLATION LEVEL {upper};"); } sb.AppendLine("SET NOCOUNT ON;"); sb.AppendLine(); diff --git a/src/PlanViewer.Core/Services/ShowPlanParser.cs b/src/PlanViewer.Core/Services/ShowPlanParser.cs index 31c93d6..db1a374 100644 --- a/src/PlanViewer.Core/Services/ShowPlanParser.cs +++ b/src/PlanViewer.Core/Services/ShowPlanParser.cs @@ -1832,6 +1832,7 @@ private static double ParseDouble(string? value) private static long ParseLong(string? value) { if (string.IsNullOrEmpty(value)) return 0; - return long.TryParse(value, out var result) ? result : 0; + return long.TryParse(value, System.Globalization.NumberStyles.Integer, + System.Globalization.CultureInfo.InvariantCulture, out var result) ? result : 0; } } From fabf59df12c4455b4acfc810d0ac5ffdf4b28dbb Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Tue, 21 Apr 2026 18:11:46 -0400 Subject: [PATCH 3/6] fix: SSMS extension writes plan XML to a randomly-named temp file (#251) Swap the predictable `ssms_plan_{yyyyMMdd_HHmmss}.sqlplan` name for a random suffix produced by Path.GetRandomFileName, and open with FileMode.CreateNew so we refuse to overwrite a pre-existing file. Closes the race where another local process could predict the filename and plant a symlink at that path before the extension's File.WriteAllText landed. Co-authored-by: Claude Opus 4.7 (1M context) --- src/PlanViewer.Ssms/AppLauncher.cs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/PlanViewer.Ssms/AppLauncher.cs b/src/PlanViewer.Ssms/AppLauncher.cs index 98baff3..9fe54ba 100644 --- a/src/PlanViewer.Ssms/AppLauncher.cs +++ b/src/PlanViewer.Ssms/AppLauncher.cs @@ -20,12 +20,21 @@ internal static class AppLauncher /// /// Saves plan XML to a temp .sqlplan file and returns the path. + /// Uses a cryptographically-random suffix so the filename can't be predicted + /// or preempted by another local process (e.g. planting a symlink at the + /// expected path before the write lands). FileMode.CreateNew refuses to + /// overwrite a pre-existing file, closing the race further. /// public static string SavePlanToTemp(string planXml) { - var fileName = $"ssms_plan_{DateTime.Now:yyyyMMdd_HHmmss}.sqlplan"; + var suffix = Path.GetFileNameWithoutExtension(Path.GetRandomFileName()); + var fileName = "ssms_plan_" + suffix + ".sqlplan"; var tempPath = Path.Combine(Path.GetTempPath(), fileName); - File.WriteAllText(tempPath, planXml); + using (var fs = new FileStream(tempPath, FileMode.CreateNew, FileAccess.Write, FileShare.None)) + using (var writer = new StreamWriter(fs)) + { + writer.Write(planXml); + } return tempPath; } From ce1fd8a91a3a12526ac2bf4b885335b1c373d2a2 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Tue, 21 Apr 2026 18:40:23 -0400 Subject: [PATCH 4/6] fix: make per-user config writes crash-safe via atomic rename (#252) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Added Services/AtomicFile.WriteAllText which writes to a sibling .tmp and then renames into place. File.Move(src, dst, overwrite: true) maps to MoveFileEx(MOVEFILE_REPLACE_EXISTING) on Windows and rename(2) on Unix — both atomic when source and destination share a filesystem, which is always the case here. Swapped File.WriteAllText for AtomicFile.WriteAllText at every config-save site: - ConnectionStore.Save (saved server list — the worst-case loss) - AppSettingsService.Save (recent/open plans, slicer days-back, etc.) - SqlFormatSettingsService.Save (format options) - AboutWindow.SaveMcpSettings (MCP enable + port) A crash between the .tmp write and the rename leaves the original file intact and a stray .tmp sibling; the next save overwrites .tmp before renaming, so no manual cleanup is needed. Co-authored-by: Claude Opus 4.7 (1M context) --- src/PlanViewer.App/AboutWindow.axaml.cs | 2 +- .../Services/AppSettingsService.cs | 2 +- src/PlanViewer.App/Services/AtomicFile.cs | 27 +++++++++++++++++++ .../Services/ConnectionStore.cs | 2 +- .../Services/SqlFormatSettingsService.cs | 2 +- 5 files changed, 31 insertions(+), 4 deletions(-) create mode 100644 src/PlanViewer.App/Services/AtomicFile.cs diff --git a/src/PlanViewer.App/AboutWindow.axaml.cs b/src/PlanViewer.App/AboutWindow.axaml.cs index d5ce313..89ac6d7 100644 --- a/src/PlanViewer.App/AboutWindow.axaml.cs +++ b/src/PlanViewer.App/AboutWindow.axaml.cs @@ -56,7 +56,7 @@ private void SaveMcpSettings() }, new JsonSerializerOptions { WriteIndented = true }); Directory.CreateDirectory(settingsDir); - File.WriteAllText(settingsFile, json); + Services.AtomicFile.WriteAllText(settingsFile, json); } private void GitHubLink_Click(object? sender, PointerPressedEventArgs e) => OpenUrl(GitHubUrl); diff --git a/src/PlanViewer.App/Services/AppSettingsService.cs b/src/PlanViewer.App/Services/AppSettingsService.cs index 46b374b..afff7fa 100644 --- a/src/PlanViewer.App/Services/AppSettingsService.cs +++ b/src/PlanViewer.App/Services/AppSettingsService.cs @@ -59,7 +59,7 @@ public static void Save(AppSettings settings) { Directory.CreateDirectory(SettingsDir); var json = JsonSerializer.Serialize(settings, JsonOptions); - File.WriteAllText(SettingsPath, json); + AtomicFile.WriteAllText(SettingsPath, json); } catch { diff --git a/src/PlanViewer.App/Services/AtomicFile.cs b/src/PlanViewer.App/Services/AtomicFile.cs new file mode 100644 index 0000000..c2c2447 --- /dev/null +++ b/src/PlanViewer.App/Services/AtomicFile.cs @@ -0,0 +1,27 @@ +using System.IO; + +namespace PlanViewer.App.Services; + +/// +/// Helper for atomic text-file writes: write to a sibling .tmp and rename +/// into place so a crash mid-write can't truncate the target file. Callers +/// are responsible for creating the parent directory first. +/// +internal static class AtomicFile +{ + /// + /// Writes to atomically + /// with respect to process crashes. If the process dies before the rename, + /// keeps its previous contents and a stray + /// .tmp sibling is left behind (cleaned up on the next call). + /// + public static void WriteAllText(string path, string contents) + { + var tmp = path + ".tmp"; + File.WriteAllText(tmp, contents); + // File.Move with overwrite:true maps to MoveFileEx(MOVEFILE_REPLACE_EXISTING) + // on Windows and rename(2) on Unix — both atomic when source and destination + // live on the same filesystem, which is always the case here. + File.Move(tmp, path, overwrite: true); + } +} diff --git a/src/PlanViewer.App/Services/ConnectionStore.cs b/src/PlanViewer.App/Services/ConnectionStore.cs index ae56194..9d0e8b9 100644 --- a/src/PlanViewer.App/Services/ConnectionStore.cs +++ b/src/PlanViewer.App/Services/ConnectionStore.cs @@ -39,7 +39,7 @@ public void Save(List connections) { Directory.CreateDirectory(ConfigDir); var json = JsonSerializer.Serialize(connections, JsonOptions); - File.WriteAllText(ConfigFile, json); + AtomicFile.WriteAllText(ConfigFile, json); } public void AddOrUpdate(ServerConnection connection) diff --git a/src/PlanViewer.App/Services/SqlFormatSettingsService.cs b/src/PlanViewer.App/Services/SqlFormatSettingsService.cs index 23e8cc5..179e908 100644 --- a/src/PlanViewer.App/Services/SqlFormatSettingsService.cs +++ b/src/PlanViewer.App/Services/SqlFormatSettingsService.cs @@ -123,7 +123,7 @@ public static bool Save(SqlFormatSettings settings, out string? error) { Directory.CreateDirectory(SettingsDir); var json = JsonSerializer.Serialize(settings, JsonOptions); - File.WriteAllText(SettingsPath, json); + AtomicFile.WriteAllText(SettingsPath, json); return true; } catch (Exception ex) From b945eba47597f53e801cea4b3e325225071ed127 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Tue, 21 Apr 2026 18:55:24 -0400 Subject: [PATCH 5/6] fix: CLI --password-stdin + honor handler exit codes (#253) Passwords given on the command line are visible in process listings, shell history, and audit logs. Add --password-stdin to analyze and query-store so scripts and humans can feed the password through a pipe instead. When --password is used inline, emit a stderr warning pointing at the safer alternatives (stdin, env var, credential store). Helper `PasswordResolver.TryResolve` centralizes: - Mutual exclusion between --password and --password-stdin. - Conflict detection between --stdin (plan XML) and --password-stdin. - Validation that stdin is actually redirected when --password-stdin is set. - Fallback to PLANVIEW_PASSWORD env var. - The inline warning for --password. CredentialCommand already reads from redirected stdin when --password is omitted; kept that behavior and added the inline warning for symmetry. Also fixed a pre-existing bug where `Environment.ExitCode = 1` set by handlers was clobbered by `return await root.InvokeAsync(args)`. Program.cs now returns `code != 0 ? code : Environment.ExitCode` so scripts can tell a validation failure from a successful run. Co-authored-by: Claude Opus 4.7 (1M context) --- src/PlanViewer.Cli/Commands/AnalyzeCommand.cs | 22 ++++++- .../Commands/CredentialCommand.cs | 3 + .../Commands/QueryStoreCommand.cs | 22 +++++-- src/PlanViewer.Cli/PasswordResolver.cs | 63 +++++++++++++++++++ src/PlanViewer.Cli/Program.cs | 6 +- 5 files changed, 108 insertions(+), 8 deletions(-) create mode 100644 src/PlanViewer.Cli/PasswordResolver.cs diff --git a/src/PlanViewer.Cli/Commands/AnalyzeCommand.cs b/src/PlanViewer.Cli/Commands/AnalyzeCommand.cs index a1f2ae3..39252fb 100644 --- a/src/PlanViewer.Cli/Commands/AnalyzeCommand.cs +++ b/src/PlanViewer.Cli/Commands/AnalyzeCommand.cs @@ -95,7 +95,11 @@ public static Command Create(ICredentialService? credentialService = null) var passwordOption = new Option( "--password", - "SQL Server password (bypasses credential store)"); + "SQL Server password (bypasses credential store). Visible in process listings — prefer --password-stdin."); + + var passwordStdinOption = new Option( + "--password-stdin", + "Read the SQL Server password from stdin (avoids process-listing exposure). Mutually exclusive with --password."); var configOption = new Option( "--config", @@ -118,6 +122,7 @@ public static Command Create(ICredentialService? credentialService = null) timeoutOption, loginOption, passwordOption, + passwordStdinOption, configOption }; @@ -137,7 +142,8 @@ public static Command Create(ICredentialService? credentialService = null) var trustCert = ctx.ParseResult.GetValueForOption(trustCertOption); var timeout = ctx.ParseResult.GetValueForOption(timeoutOption); var login = ctx.ParseResult.GetValueForOption(loginOption); - var password = ctx.ParseResult.GetValueForOption(passwordOption); + var passwordInline = ctx.ParseResult.GetValueForOption(passwordOption); + var passwordStdin = ctx.ParseResult.GetValueForOption(passwordStdinOption); var configPath = ctx.ParseResult.GetValueForOption(configOption); // Load analyzer config @@ -148,10 +154,20 @@ public static Command Create(ICredentialService? credentialService = null) server ??= env.GetValueOrDefault("PLANVIEW_SERVER"); database ??= env.GetValueOrDefault("PLANVIEW_DATABASE"); login ??= env.GetValueOrDefault("PLANVIEW_LOGIN"); - password ??= env.GetValueOrDefault("PLANVIEW_PASSWORD"); if (!trustCert && env.GetValueOrDefault("PLANVIEW_TRUST_CERT")?.Equals("true", StringComparison.OrdinalIgnoreCase) == true) trustCert = true; + // Resolve password from --password-stdin, --password, or PLANVIEW_PASSWORD + // (in that order). --stdin for plan XML conflicts with --password-stdin. + if (!PasswordResolver.TryResolve( + passwordInline, passwordStdin, stdin, + env.GetValueOrDefault("PLANVIEW_PASSWORD"), + out var password)) + { + Environment.ExitCode = 1; + return; + } + if (server != null) { await RunLiveAsync(file, server, database, query, outputDir, estimated, diff --git a/src/PlanViewer.Cli/Commands/CredentialCommand.cs b/src/PlanViewer.Cli/Commands/CredentialCommand.cs index 9503c37..e94e1f1 100644 --- a/src/PlanViewer.Cli/Commands/CredentialCommand.cs +++ b/src/PlanViewer.Cli/Commands/CredentialCommand.cs @@ -36,6 +36,9 @@ private static Command CreateAddCommand(ICredentialService credentialService) string password; if (!string.IsNullOrEmpty(passwordArg)) { + Console.Error.WriteLine( + "Warning: --password is visible in process listings and shell history. " + + "Prefer piping the password into stdin (e.g. `echo hunter2 | planview credential add ...`)."); password = passwordArg; } else if (Console.IsInputRedirected) diff --git a/src/PlanViewer.Cli/Commands/QueryStoreCommand.cs b/src/PlanViewer.Cli/Commands/QueryStoreCommand.cs index d805a05..15baf18 100644 --- a/src/PlanViewer.Cli/Commands/QueryStoreCommand.cs +++ b/src/PlanViewer.Cli/Commands/QueryStoreCommand.cs @@ -72,7 +72,11 @@ public static Command Create(ICredentialService? credentialService = null) "--login", "SQL Server login (bypasses credential store)"); var passwordOption = new Option( - "--password", "SQL Server password"); + "--password", "SQL Server password. Visible in process listings — prefer --password-stdin."); + + var passwordStdinOption = new Option( + "--password-stdin", + "Read the SQL Server password from stdin (avoids process-listing exposure). Mutually exclusive with --password."); var queryIdOption = new Option( "--query-id", "Filter by Query Store query ID"); @@ -93,7 +97,7 @@ public static Command Create(ICredentialService? credentialService = null) { serverOption, databaseOption, topOption, orderByOption, hoursBackOption, outputDirOption, outputOption, compactOption, warningsOnlyOption, configOption, - authOption, trustCertOption, loginOption, passwordOption, + authOption, trustCertOption, loginOption, passwordOption, passwordStdinOption, queryIdOption, planIdOption, queryHashOption, planHashOption, moduleOption }; @@ -112,7 +116,8 @@ public static Command Create(ICredentialService? credentialService = null) var auth = ctx.ParseResult.GetValueForOption(authOption); var trustCert = ctx.ParseResult.GetValueForOption(trustCertOption); var login = ctx.ParseResult.GetValueForOption(loginOption); - var password = ctx.ParseResult.GetValueForOption(passwordOption); + var passwordInline = ctx.ParseResult.GetValueForOption(passwordOption); + var passwordStdin = ctx.ParseResult.GetValueForOption(passwordStdinOption); var filterQueryId = ctx.ParseResult.GetValueForOption(queryIdOption); var filterPlanId = ctx.ParseResult.GetValueForOption(planIdOption); var filterQueryHash = ctx.ParseResult.GetValueForOption(queryHashOption); @@ -122,10 +127,19 @@ public static Command Create(ICredentialService? credentialService = null) // Load .env file if present (CLI args take precedence) var env = ConnectionHelper.LoadEnvFile(); login ??= env.GetValueOrDefault("PLANVIEW_LOGIN"); - password ??= env.GetValueOrDefault("PLANVIEW_PASSWORD"); if (!trustCert && env.GetValueOrDefault("PLANVIEW_TRUST_CERT")?.Equals("true", StringComparison.OrdinalIgnoreCase) == true) trustCert = true; + // Resolve password from --password-stdin, --password, or PLANVIEW_PASSWORD + if (!PasswordResolver.TryResolve( + passwordInline, passwordStdin, stdinAlreadyClaimed: false, + env.GetValueOrDefault("PLANVIEW_PASSWORD"), + out var password)) + { + Environment.ExitCode = 1; + return; + } + if (top < 1) { Console.Error.WriteLine("--top must be >= 1"); diff --git a/src/PlanViewer.Cli/PasswordResolver.cs b/src/PlanViewer.Cli/PasswordResolver.cs new file mode 100644 index 0000000..ed50bb1 --- /dev/null +++ b/src/PlanViewer.Cli/PasswordResolver.cs @@ -0,0 +1,63 @@ +namespace PlanViewer.Cli; + +/// +/// Resolves a SQL Server password from the available CLI inputs: +/// 1. --password-stdin (reads one line from redirected stdin) +/// 2. --password (inline CLI arg; emits a stderr warning because it's +/// visible in process listings, shell history, and audit logs) +/// 3. PLANVIEW_PASSWORD environment variable (from the process environment or +/// a .env file, already looked up by the caller) +/// +internal static class PasswordResolver +{ + /// + /// Returns true with a resolved password (which may be null if no source provided + /// one). Returns false on user error (mutual-exclusion violation or stdin not + /// redirected when --password-stdin was requested). The caller is responsible + /// for setting Environment.ExitCode on failure. + /// + public static bool TryResolve( + string? inlinePassword, + bool passwordFromStdin, + bool stdinAlreadyClaimed, + string? envPassword, + out string? password) + { + password = null; + + if (passwordFromStdin && !string.IsNullOrEmpty(inlinePassword)) + { + Console.Error.WriteLine("--password and --password-stdin are mutually exclusive."); + return false; + } + + if (passwordFromStdin && stdinAlreadyClaimed) + { + Console.Error.WriteLine("--password-stdin can't be combined with --stdin (both read from stdin)."); + return false; + } + + if (passwordFromStdin) + { + if (!Console.IsInputRedirected) + { + Console.Error.WriteLine("--password-stdin requires stdin to be redirected (pipe the password into the command)."); + return false; + } + password = Console.In.ReadLine()?.TrimEnd('\r', '\n') ?? ""; + return true; + } + + if (!string.IsNullOrEmpty(inlinePassword)) + { + Console.Error.WriteLine( + "Warning: --password is visible in process listings and shell history. " + + "Prefer --password-stdin, the PLANVIEW_PASSWORD env var, or the credential store."); + password = inlinePassword; + return true; + } + + password = envPassword; + return true; + } +} diff --git a/src/PlanViewer.Cli/Program.cs b/src/PlanViewer.Cli/Program.cs index c3cbac7..d8b5062 100644 --- a/src/PlanViewer.Cli/Program.cs +++ b/src/PlanViewer.Cli/Program.cs @@ -23,4 +23,8 @@ if (credentialService != null) root.Add(CredentialCommand.Create(credentialService)); -return await root.InvokeAsync(args); +// System.CommandLine's InvokeAsync returns 0 for successful dispatch even when a +// handler set Environment.ExitCode = 1 to signal a validation error. Honor either +// signal so scripts can tell success from failure. +var code = await root.InvokeAsync(args); +return code != 0 ? code : Environment.ExitCode; From 3a40ada881942b264eaec4a238f2bb1076d9f2f9 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Wed, 22 Apr 2026 06:03:58 -0400 Subject: [PATCH 6/6] Fix operator self-time for parents of Compute Scalar (#215 C5) + one decimal on benefit % (#255) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit C5 (operator self-time look-through): GetSerialOwnElapsed subtracted each direct child's ActualElapsedMs, but Compute Scalar operators don't carry runtime stats (ActualElapsedMs = 0). For an operator whose direct children are Compute Scalars — like the node-6 Hash Match in Joe's private plan — that meant "children elapsed = 0" and the parent kept its full elapsed as self-time. Effect: node-6 Hash Spill reported 95% benefit / 8,829ms self-time. With look-through through the Compute Scalars it's 1,609ms self-time / 17.4% — matching Joe's math of 8.829 - 7.121 - 0.099. New GetEffectiveChildElapsedMs helper walks through pass-through operators (Compute Scalar and anything else missing runtime stats) to the first descendant with real stats. Parallelism exchange children keep existing max-child behavior because exchange times are unreliable. Applies to every operator-time-based benefit score (spills, key lookups, bare scans, scan-with-predicate, scan cardinality, eager index spool, etc). C2 (decimal precision on benefit %): Benefit % was rounded to N0 everywhere, so tiny waits showed as "up to 0%". Switched to one decimal except for 100 (shown as "100"), matching the existing pattern in ComparisonFormatter. Applied to web strip, wait-stats card, HTML export (both surfaces), text formatter (both paths), Avalonia Plan Warnings + Node Warnings headers. Version bump 1.7.1 -> 1.7.2. Co-authored-by: Claude Opus 4.7 (1M context) --- .../Controls/PlanViewerControl.axaml.cs | 7 +++- src/PlanViewer.App/PlanViewer.App.csproj | 2 +- src/PlanViewer.Core/Output/HtmlExporter.cs | 4 +- src/PlanViewer.Core/Output/TextFormatter.cs | 4 +- src/PlanViewer.Core/Services/PlanAnalyzer.cs | 41 ++++++++++++++----- src/PlanViewer.Web/Pages/Index.razor | 4 +- 6 files changed, 42 insertions(+), 20 deletions(-) diff --git a/src/PlanViewer.App/Controls/PlanViewerControl.axaml.cs b/src/PlanViewer.App/Controls/PlanViewerControl.axaml.cs index 854b987..bd45cc4 100644 --- a/src/PlanViewer.App/Controls/PlanViewerControl.axaml.cs +++ b/src/PlanViewer.App/Controls/PlanViewerControl.axaml.cs @@ -801,6 +801,9 @@ private static string FormatBytes(double bytes) return $"{bytes / (1024L * 1024 * 1024):N1} GB"; } + private static string FormatBenefitPercent(double pct) => + pct >= 100 ? $"{pct:N0}" : $"{pct:N1}"; + #endregion #region Node Selection & Properties Panel @@ -1737,7 +1740,7 @@ private void ShowPropertiesPanel(PlanNode node) : w.Severity == PlanWarningSeverity.Warning ? "#FFB347" : "#6BB5FF"; var warnPanel = new StackPanel { Margin = new Thickness(10, 2, 10, 2) }; var planWarnHeader = w.MaxBenefitPercent.HasValue - ? $"\u26A0 {w.WarningType} \u2014 up to {w.MaxBenefitPercent:N0}% benefit" + ? $"\u26A0 {w.WarningType} \u2014 up to {FormatBenefitPercent(w.MaxBenefitPercent.Value)}% benefit" : $"\u26A0 {w.WarningType}"; warnPanel.Children.Add(new TextBlock { @@ -1819,7 +1822,7 @@ private void ShowPropertiesPanel(PlanNode node) : w.Severity == PlanWarningSeverity.Warning ? "#FFB347" : "#6BB5FF"; var warnPanel = new StackPanel { Margin = new Thickness(10, 2, 10, 2) }; var nodeWarnHeader = w.MaxBenefitPercent.HasValue - ? $"\u26A0 {w.WarningType} \u2014 up to {w.MaxBenefitPercent:N0}% benefit" + ? $"\u26A0 {w.WarningType} \u2014 up to {FormatBenefitPercent(w.MaxBenefitPercent.Value)}% benefit" : $"\u26A0 {w.WarningType}"; warnPanel.Children.Add(new TextBlock { diff --git a/src/PlanViewer.App/PlanViewer.App.csproj b/src/PlanViewer.App/PlanViewer.App.csproj index c00ff0c..5334a5c 100644 --- a/src/PlanViewer.App/PlanViewer.App.csproj +++ b/src/PlanViewer.App/PlanViewer.App.csproj @@ -6,7 +6,7 @@ app.manifest EDD.ico true - 1.7.1 + 1.7.2 Erik Darling Darling Data LLC Performance Studio diff --git a/src/PlanViewer.Core/Output/HtmlExporter.cs b/src/PlanViewer.Core/Output/HtmlExporter.cs index 897598c..7877137 100644 --- a/src/PlanViewer.Core/Output/HtmlExporter.cs +++ b/src/PlanViewer.Core/Output/HtmlExporter.cs @@ -407,7 +407,7 @@ private static void WriteWaitStatsCard(StringBuilder sb, StatementResult stmt, b { var barPct = maxWait > 0 ? (double)w.WaitTimeMs / maxWait * 100 : 0; var benefitTag = benefitLookup.TryGetValue(w.WaitType, out var pct) - ? $" up to {pct:N0}%" + ? $" up to {(pct >= 100 ? pct.ToString("N0") : pct.ToString("N1"))}%" : ""; sb.AppendLine("
"); sb.AppendLine($"{Encode(w.WaitType)}"); @@ -458,7 +458,7 @@ private static void WriteWarnings(StringBuilder sb, StatementResult stmt) sb.AppendLine($"{Encode(w.Operator)}"); sb.AppendLine($"{Encode(w.Type)}"); if (w.MaxBenefitPercent.HasValue) - sb.AppendLine($"up to {w.MaxBenefitPercent:N0}% benefit"); + sb.AppendLine($"up to {(w.MaxBenefitPercent.Value >= 100 ? w.MaxBenefitPercent.Value.ToString("N0") : w.MaxBenefitPercent.Value.ToString("N1"))}% benefit"); sb.AppendLine($"{Encode(w.Message)}"); if (!string.IsNullOrEmpty(w.ActionableFix)) sb.AppendLine($"{Encode(w.ActionableFix)}"); diff --git a/src/PlanViewer.Core/Output/TextFormatter.cs b/src/PlanViewer.Core/Output/TextFormatter.cs index d5ec7d1..954af41 100644 --- a/src/PlanViewer.Core/Output/TextFormatter.cs +++ b/src/PlanViewer.Core/Output/TextFormatter.cs @@ -139,7 +139,7 @@ public static void WriteText(AnalysisResult result, TextWriter writer) foreach (var w in stmt.WaitStats.OrderByDescending(w => w.WaitTimeMs)) { var benefitTag = benefitLookup.TryGetValue(w.WaitType, out var pct) - ? $" (up to {pct:N0}% benefit)" + ? $" (up to {(pct >= 100 ? pct.ToString("N0") : pct.ToString("N1"))}% benefit)" : ""; writer.WriteLine($" {w.WaitType}: {w.WaitTimeMs:N0}ms{benefitTag}"); } @@ -169,7 +169,7 @@ public static void WriteText(AnalysisResult result, TextWriter writer) foreach (var w in sortedWarnings) { var benefitTag = w.MaxBenefitPercent.HasValue - ? $" (up to {w.MaxBenefitPercent:N0}% benefit)" + ? $" (up to {(w.MaxBenefitPercent.Value >= 100 ? w.MaxBenefitPercent.Value.ToString("N0") : w.MaxBenefitPercent.Value.ToString("N1"))}% benefit)" : ""; writer.WriteLine($" [{w.Severity}] {w.Type}{benefitTag}: {EscapeNewlines(w.Message)}"); if (!string.IsNullOrEmpty(w.ActionableFix)) diff --git a/src/PlanViewer.Core/Services/PlanAnalyzer.cs b/src/PlanViewer.Core/Services/PlanAnalyzer.cs index 01229ea..a66cf67 100644 --- a/src/PlanViewer.Core/Services/PlanAnalyzer.cs +++ b/src/PlanViewer.Core/Services/PlanAnalyzer.cs @@ -1593,26 +1593,45 @@ private static long GetPerThreadOwnElapsed(PlanNode node) } /// - /// Serial row mode self-time: subtract all direct children's elapsed. - /// Exchange children are skipped through to their real child. + /// Serial row mode self-time: subtract all direct children's effective elapsed. + /// Pass-through operators (Compute Scalar, etc.) don't carry runtime stats — + /// look through them to the first descendant that does. Exchange children + /// use max-child elapsed because exchange times are unreliable. /// private static long GetSerialOwnElapsed(PlanNode node) { var totalChildElapsed = 0L; foreach (var child in node.Children) - { - var childElapsed = child.ActualElapsedMs; - - // Exchange operators have unreliable times — skip to their child - if (child.PhysicalOp == "Parallelism" && child.Children.Count > 0) - childElapsed = child.Children.Max(c => c.ActualElapsedMs); - - totalChildElapsed += childElapsed; - } + totalChildElapsed += GetEffectiveChildElapsedMs(child); return Math.Max(0, node.ActualElapsedMs - totalChildElapsed); } + /// + /// Returns the elapsed time a child contributes to its parent's subtree. + /// Looks through pass-through operators (Compute Scalar, Parallelism exchange) + /// that don't carry reliable runtime stats. + /// + private static long GetEffectiveChildElapsedMs(PlanNode child) + { + // Exchange operators: unreliable times, use max child + if (child.PhysicalOp == "Parallelism" && child.Children.Count > 0) + return child.Children.Max(GetEffectiveChildElapsedMs); + + // Child has its own stats: use them + if (child.ActualElapsedMs > 0) + return child.ActualElapsedMs; + + // No stats (Compute Scalar and similar): look through to descendants + if (child.Children.Count == 0) + return 0; + + var sum = 0L; + foreach (var grandchild in child.Children) + sum += GetEffectiveChildElapsedMs(grandchild); + return sum; + } + /// /// Calculates a Parallelism (exchange) operator's own elapsed time. /// Exchange times are unreliable — they accumulate wait time caused by diff --git a/src/PlanViewer.Web/Pages/Index.razor b/src/PlanViewer.Web/Pages/Index.razor index 0fc82d5..3fd618b 100644 --- a/src/PlanViewer.Web/Pages/Index.razor +++ b/src/PlanViewer.Web/Pages/Index.razor @@ -302,7 +302,7 @@ else @w.WaitTimeMs.ToString("N0") ms @if (benefitPct > 0) { - up to @benefitPct.ToString("N0")% + up to @(benefitPct >= 100 ? benefitPct.ToString("N0") : benefitPct.ToString("N1"))% }
@@ -346,7 +346,7 @@ else @w.Type @if (w.MaxBenefitPercent.HasValue) { - up to @w.MaxBenefitPercent.Value.ToString("N0")% benefit + up to @(w.MaxBenefitPercent.Value >= 100 ? w.MaxBenefitPercent.Value.ToString("N0") : w.MaxBenefitPercent.Value.ToString("N1"))% benefit } @w.Message @if (!string.IsNullOrEmpty(w.ActionableFix))