Fix crashes, DoS, and security issues from Fable code review

Parallel sub-agent review surfaced several real bugs; this batch fixes the
high-confidence, self-contained ones (each verified individually; build and
all 77 Core tests green):

Crashes / DoS:
- ShowPlanParser: guard recursion depth in ParseRelOp/ParseStatementAndChildren
  and contain the tree walk, so hostile deeply-nested plan XML yields a
  ParseError instead of an uncatchable StackOverflowException.
- Program.cs: single-instance file forwarding never worked (the mutex was
  disposed as the method returned, so it always launched a duplicate). Detect
  the running instance via the named pipe instead.
- MainWindow: add backoff to the pipe listener's catch so a second instance
  can no longer spin it at 100% CPU.
- QuerySessionControl.Format: add a catch to the Format_Click async void
  handler so a formatter exception no longer crashes the app.

Correctness:
- PlanAnalyzer: the "Cursor Missing LOCAL" rule looked for LOCAL before the
  CURSOR keyword (where it is never legal) and fired on every cursor, including
  ones already declared LOCAL. Match qualifiers between CURSOR and FOR.
- DarkTheme: define the missing ForegroundMutedBrush (#B0B6C0) referenced by
  five call sites, and fix the code-behind fallback (was full foreground).
- HtmlExporter: detect all spill warning types ("Spill to TempDb"/"Spill
  Occurred", not just "* Spill"), and lowercase severity with ToLowerInvariant
  so the CSS classes survive non-invariant locales.

Security:
- PlanShare: generate share IDs with RandomNumberGenerator instead of
  Random.Shared; the 8-char id is the only access control on a shared plan.
- CLI ConnectionHelper: keep Encrypt=Mandatory regardless of --trust-cert;
  --trust-cert previously downgraded the connection to Optional (MITM-able).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: erikdarlingdata

server/PlanShare/Program.cs

@@ -347,8 +347,10 @@ GROUP BY referrer ORDER BY count DESC LIMIT 10
 
 static string GenerateId()
 {
+    // The id is the only access control on a shared plan, so it must be
+    // unpredictable — use a cryptographic RNG, not Random.Shared (xoshiro).
     const string chars = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";
-    return new string(Random.Shared.GetItems<char>(chars.AsSpan(), 8));
+    return new string(RandomNumberGenerator.GetItems<char>(chars.AsSpan(), 8));
 }
 
 static string GenerateDeleteToken()

src/PlanViewer.App/Controls/PlanViewerControl.Rendering.cs

@@ -543,7 +543,7 @@ private IBrush FindBrushResource(string key)
             "BackgroundLightBrush" => new SolidColorBrush(Color.FromRgb(0x23, 0x26, 0x2E)),
             "BorderBrush" => new SolidColorBrush(Color.FromRgb(0x3A, 0x3D, 0x45)),
             "ForegroundBrush" => new SolidColorBrush(Color.FromRgb(0xE4, 0xE6, 0xEB)),
-            "ForegroundMutedBrush" => new SolidColorBrush(Color.FromRgb(0xE4, 0xE6, 0xEB)),
+            "ForegroundMutedBrush" => new SolidColorBrush(Color.FromRgb(0xB0, 0xB6, 0xC0)),
             _ => Brushes.White
         };
     }

src/PlanViewer.App/Controls/QuerySessionControl.Format.cs

@@ -143,6 +143,11 @@ private async void Format_Click(object? sender, RoutedEventArgs e)
             QueryEditor.CaretOffset = Math.Min(caretOffset, QueryEditor.Document.TextLength);
             SetStatus("Formatted");
         }
+        catch (Exception ex)
+        {
+            // async void handler: an unhandled throw here would crash the app.
+            SetStatus($"Format failed: {ex.Message}");
+        }
         finally
         {
             FormatButton.IsEnabled = true;

src/PlanViewer.App/MainWindow.axaml.cs

@@ -175,7 +175,11 @@ await Dispatcher.UIThread.InvokeAsync(() =>
                 }
                 catch
                 {
-                    // Pipe error — restart the listener
+                    // Pipe error (e.g. another instance already holds the single
+                    // server slot). Back off before retrying so we don't spin at
+                    // 100% CPU recreating the listener in a tight loop.
+                    try { await Task.Delay(1000, token); }
+                    catch (OperationCanceledException) { break; }
                 }
             }
         }, token);

src/PlanViewer.App/Program.cs

@@ -2,15 +2,13 @@
 using System;
 using System.IO;
 using System.IO.Pipes;
-using System.Threading;
 using Velopack;
 
 namespace PlanViewer.App;
 
 class Program
 {
     private const string PipeName = "SQLPerformanceStudio_OpenFile";
-    private const string MutexName = "SQLPerformanceStudio_SingleInstance";
 
     [STAThread]
     public static void Main(string[] args)
@@ -33,34 +31,31 @@ public static AppBuilder BuildAvaloniaApp()
             .LogToTrace();
 
     /// <summary>
-    /// Tries to connect to an already-running instance and send the file path.
-    /// Returns true if the message was delivered (caller should exit).
+    /// Tries to hand the file path to an already-running instance over its named pipe.
+    /// A failed/timed-out connect means no instance is listening, so the caller should
+    /// launch normally. Returns true only if the path was actually delivered.
     /// </summary>
+    /// <remarks>
+    /// Detection is via the pipe itself rather than a named mutex: the previous mutex
+    /// was disposed as soon as this method returned, so no instance ever held it and
+    /// the forwarding path was never taken.
+    /// </remarks>
     private static bool TrySendToRunningInstance(string filePath)
     {
-        bool createdNew;
-        using var mutex = new Mutex(true, MutexName, out createdNew);
-
-        if (createdNew)
-        {
-            // We're the first instance — release and let normal startup proceed
-            mutex.ReleaseMutex();
-            return false;
-        }
-
-        // Another instance owns the mutex — try sending the file path
         try
         {
             using var client = new NamedPipeClientStream(".", PipeName, PipeDirection.Out);
-            client.Connect(3000); // 3 second timeout
+            // Short timeout: a running instance's listener is idle and connects
+            // immediately; when none is running this is the only added launch delay.
+            client.Connect(500);
             using var writer = new StreamWriter(client);
             writer.WriteLine(filePath);
             writer.Flush();
             return true;
         }
         catch
         {
-            // Pipe not available — fall through to launch normally
+            // No instance listening (or pipe busy) — fall through to launch normally.
             return false;
         }
     }

src/PlanViewer.App/Themes/DarkTheme.axaml

@@ -5,6 +5,7 @@
     <SolidColorBrush x:Key="BackgroundDarkBrush" Color="#15171C"/>
     <SolidColorBrush x:Key="BackgroundLightBrush" Color="#22252D"/>
     <SolidColorBrush x:Key="ForegroundBrush" Color="#E4E6EB"/>
+    <SolidColorBrush x:Key="ForegroundMutedBrush" Color="#B0B6C0"/>
     <SolidColorBrush x:Key="BorderBrush" Color="#3A3D45"/>
     <SolidColorBrush x:Key="AccentBrush" Color="#2eaef1"/>
 

src/PlanViewer.Cli/ConnectionHelper.cs

@@ -17,7 +17,10 @@ public static string BuildConnectionString(
             ApplicationName = "PlanViewer",
             ConnectTimeout = 15,
             TrustServerCertificate = trustCert,
-            Encrypt = trustCert ? SqlConnectionEncryptOption.Optional : SqlConnectionEncryptOption.Mandatory
+            // Keep encryption mandatory regardless of --trust-cert. --trust-cert only
+            // skips certificate *validation* (for self-signed certs); it must not also
+            // drop the connection to optional/plaintext, which would allow a MITM.
+            Encrypt = SqlConnectionEncryptOption.Mandatory
         };
 
         if (multipleActiveResultSets)

src/PlanViewer.Core/Output/HtmlExporter.cs

@@ -373,7 +373,9 @@ private static bool HasSpillInTree(OperatorResult? node)
         if (node == null) return false;
         foreach (var w in node.Warnings)
         {
-            if (w.Type.EndsWith(" Spill", StringComparison.Ordinal))
+            // Covers "Sort Spill"/"Hash Spill"/"Exchange Spill" as well as the
+            // standalone "Spill to TempDb" and "Spill Occurred" warning types.
+            if (w.Type.Contains("Spill", StringComparison.Ordinal))
                 return true;
         }
         foreach (var child in node.Children)
@@ -507,7 +509,7 @@ private static void WriteWarnings(StringBuilder sb, StatementResult stmt)
 
         foreach (var w in sorted)
         {
-            var sevLower = w.Severity.ToLower();
+            var sevLower = w.Severity.ToLowerInvariant();
             sb.AppendLine($"<div class=\"warning-item {sevLower}\">");
             sb.AppendLine($"<span class=\"sev sev-{sevLower}\">{Encode(w.Severity)}</span>");
             if (w.Operator != null)

src/PlanViewer.Core/Services/PlanAnalyzer.Statement.cs

@@ -290,11 +290,15 @@ private static void AnalyzeStatement(PlanStatement stmt, AnalyzerConfig cfg, Ser
         // https://erikdarling.com/cursor-declarations-that-use-openjson-can-bloat-your-plan-cache/).
         if (!cfg.IsRuleDisabled(37) && !string.IsNullOrEmpty(stmt.StatementText))
         {
-            // DECLARE <name> [qualifier(s)] CURSOR ... FOR
-            // Flags the declaration if LOCAL isn't among the qualifiers before CURSOR.
+            // DECLARE <name> [INSENSITIVE|SCROLL] CURSOR [qualifier(s)] FOR ...
+            // In the T-SQL extended syntax, LOCAL/GLOBAL appear AFTER the CURSOR
+            // keyword (only INSENSITIVE/SCROLL are legal before it), so the LOCAL
+            // qualifier must be looked for between CURSOR and the FOR that introduces
+            // the SELECT. Capturing tokens *before* CURSOR never sees LOCAL and would
+            // fire on every cursor, including ones already declared LOCAL.
             var cursorDeclMatch = Regex.Match(
                 stmt.StatementText,
-                @"\bDECLARE\s+\w+\s+((?:\w+\s+)*)CURSOR\b",
+                @"\bDECLARE\s+\w+\s+(?:INSENSITIVE\s+|SCROLL\s+)*CURSOR\b(.*?)\bFOR\b",
                 RegexOptions.IgnoreCase | RegexOptions.Singleline);
             if (cursorDeclMatch.Success)
             {

src/PlanViewer.Core/Services/ShowPlanParser.RelOp.cs

@@ -9,8 +9,11 @@ namespace PlanViewer.Core.Services;
 
 public static partial class ShowPlanParser
 {
-    private static PlanNode ParseRelOp(XElement relOpEl)
+    private static PlanNode ParseRelOp(XElement relOpEl, int depth = 0)
     {
+        if (depth > MaxParseDepth)
+            throw new InvalidOperationException("Plan operator nesting exceeds the supported depth limit.");
+
         var node = new PlanNode
         {
             NodeId = (int)ParseDouble(relOpEl.Attribute("NodeId")?.Value),
@@ -762,7 +765,7 @@ private static PlanNode ParseRelOp(XElement relOpEl)
         // Recurse into child RelOps
         foreach (var childRelOp in FindChildRelOps(relOpEl))
         {
-            var childNode = ParseRelOp(childRelOp);
+            var childNode = ParseRelOp(childRelOp, depth + 1);
             childNode.Parent = node;
             node.Children.Add(childNode);
         }