Merge pull request #586 from erikdarlingdata/fix/output-and-cli-hardening

Harden HTML export, repro script header, and CLI encryption

Author: erikdarlingdata

src/PlanViewer.Cli/Commands/CliConnectionResolver.cs

@@ -70,7 +70,10 @@ deep inside MSAL with "0xwindow_handle_required" — a message that tells the us
             DisplayName = server,
             AuthenticationType = authType,
             TrustServerCertificate = trustCert,
-            EncryptMode = trustCert ? "Optional" : "Mandatory"
+            /* Keep encryption mandatory regardless of --trust-cert, as ConnectionHelper does for a
+               direct login. --trust-cert only skips certificate validation (for self-signed certs);
+               it must not also make encryption optional and let queries and results cross in plaintext. */
+            EncryptMode = "Mandatory"
         };
     }
 

src/PlanViewer.Core/Output/HtmlExporter.cs

@@ -509,9 +509,11 @@ private static void WriteWarnings(StringBuilder sb, StatementResult stmt)
 
         foreach (var w in sorted)
         {
-            var sevLower = w.Severity.ToLowerInvariant();
-            sb.AppendLine($"<div class=\"warning-item {sevLower}\">");
-            sb.AppendLine($"<span class=\"sev sev-{sevLower}\">{Encode(w.Severity)}</span>");
+            // A shared plan's analysis is caller-supplied JSON, so Severity can hold anything.
+            // Only a fixed class name goes into the attributes; the text itself is encoded.
+            var sevClass = SeverityClass(w.Severity);
+            sb.AppendLine($"<div class=\"warning-item {sevClass}\">");
+            sb.AppendLine($"<span class=\"sev sev-{sevClass}\">{Encode(w.Severity)}</span>");
             if (w.Operator != null)
                 sb.AppendLine($"<span class=\"warn-op\">{Encode(w.Operator)}</span>");
             sb.AppendLine($"<span class=\"warn-type\">{Encode(w.Type)}</span>");
@@ -619,4 +621,15 @@ private static string FormatKB(long kb)
     }
 
     private static string Encode(string text) => HttpUtility.HtmlEncode(text);
+
+    /// <summary>
+    /// Maps a warning severity to one of the stylesheet's three class names. Any other value,
+    /// null included, gets "info", so severity text never reaches a class attribute.
+    /// </summary>
+    private static string SeverityClass(string? severity) => severity?.ToLowerInvariant() switch
+    {
+        "critical" => "critical",
+        "warning" => "warning",
+        _ => "info"
+    };
 }

src/PlanViewer.Core/Services/ReproScriptBuilder.cs

@@ -111,13 +111,14 @@ rather than leaving an unexplained placeholder. */
             warnings.Add($"Variables in query without values: {string.Join(", ", unresolvedVars)}. These may be local variables — fill in values before executing.");
         }
 
-        /* Header comment */
+        /* Header comment. Every value in it goes through CommentSafe: the database name
+           comes off plan XML, and a crafted one must not be able to end the comment early. */
         sb.AppendLine("/*");
-        sb.AppendLine("Reproduction script generated by SQL Server Performance Monitor");
-        sb.AppendLine($"Source: {source}");
+        sb.AppendLine("Reproduction script generated by Performance Studio");
+        sb.AppendLine($"Source: {CommentSafe(source)}");
         if (!string.IsNullOrEmpty(databaseName))
         {
-            sb.AppendLine($"Database: [{databaseName}]");
+            sb.AppendLine($"Database: [{CommentSafe(databaseName)}]");
         }
         sb.AppendLine($"Generated: {DateTime.Now:yyyy-MM-dd HH:mm:ss}");
 
@@ -127,15 +128,16 @@ rather than leaving an unexplained placeholder. */
             sb.AppendLine("Warnings:");
             foreach (var warning in warnings)
             {
-                sb.AppendLine($" - {warning}");
+                sb.AppendLine($" - {CommentSafe(warning)}");
             }
         }
 
         sb.AppendLine("*/");
         sb.AppendLine();
 
         /* USE database (skip for Azure SQL DB — USE is invalid there).
-           Double any ']' in the identifier so names like 'cool]stuff' still parse. */
+           Double any ']' in the identifier so names like 'cool]stuff' still parse. Line breaks
+           in the name stay: a client that splits batches correctly never splits inside brackets. */
         if (!string.IsNullOrEmpty(databaseName) && !isAzureSqlDb)
         {
             sb.AppendLine($"USE [{databaseName.Replace("]", "]]")}];");
@@ -408,6 +410,21 @@ private static string EscapeSqlString(string value)
         return value.Replace("'", "''");
     }
 
+    /// <summary>
+    /// Makes text safe inside the header's block comment. "*/" would close the comment and
+    /// "/*" would open a nested one (T-SQL block comments nest), so both are split with a
+    /// space. Line breaks become spaces too, so each value stays on one line of the header
+    /// and cannot put GO on a line of its own there.
+    /// </summary>
+    private static string CommentSafe(string? text)
+    {
+        /* \p{Cc} covers CR, LF, tab and NEL; U+2028 and U+2029 are the Unicode line and
+           paragraph separators, which some editors also treat as line breaks. */
+        return Regex.Replace(text ?? "", @"[\p{Cc}\u2028\u2029]", " ")
+            .Replace("*/", "* /")
+            .Replace("/*", "/ *");
+    }
+
     /// <summary>
     /// Validates a parameter name from plan XML as a plain @identifier.
     /// Anything else is dropped from the generated script.

tests/PlanViewer.Core.Tests/CliConnectionResolverTests.cs

@@ -1,3 +1,5 @@
+using Microsoft.Data.SqlClient;
+using PlanViewer.Cli;
 using PlanViewer.Cli.Commands;
 using PlanViewer.Core.Interfaces;
 using PlanViewer.Core.Services;
@@ -55,6 +57,29 @@ instead of leaking. */
         }
     }
 
+    /* --trust-cert skips certificate validation and nothing else. It used to make encryption
+       optional as well, unlike the direct-login path in ConnectionHelper, which always kept it
+       mandatory. Both paths must agree. */
+    [Theory]
+    [InlineData("sql", false)]
+    [InlineData("sql", true)]
+    [InlineData("windows", false)]
+    [InlineData("windows", true)]
+    public void BuildServerConnection_KeepsEncryptionMandatory(string auth, bool trustCert)
+    {
+        var store = new InMemoryCredentialService();
+        store.SaveCredential("srv", "user", "pass");
+
+        var connection = CliConnectionResolver.BuildServerConnection("srv", auth, trustCert, store);
+        var resolved = new SqlConnectionStringBuilder(connection.GetConnectionString(store));
+        var direct = new SqlConnectionStringBuilder(
+            ConnectionHelper.BuildConnectionString("srv", "master", "user", "pass", trustCert));
+
+        Assert.Equal(SqlConnectionEncryptOption.Mandatory, resolved.Encrypt);
+        Assert.Equal(trustCert, resolved.TrustServerCertificate);
+        Assert.Equal(direct.Encrypt, resolved.Encrypt);
+    }
+
     /* Minimal stand-in: the resolver only asks whether a credential exists, and the entra refusal must fire
        before credentials ever matter. */
     private sealed class NoCredentials : ICredentialService

tests/PlanViewer.Core.Tests/HtmlExporterTests.cs

@@ -63,4 +63,67 @@ public void Export_EscapesHtmlInQueryText()
         Assert.Contains("<!DOCTYPE html>", html);
         Assert.Contains("</html>", html);
     }
+
+    [Theory]
+    [InlineData("Critical", "critical")]
+    [InlineData("Warning", "warning")]
+    [InlineData("Info", "info")]
+    public void Export_KnownSeverity_KeepsItsClass(string severity, string cssClass)
+    {
+        var html = ExportWithSeverity(severity);
+
+        Assert.Contains($"<div class=\"warning-item {cssClass}\">", html);
+        Assert.Contains($"<span class=\"sev sev-{cssClass}\">{severity}</span>", html);
+    }
+
+    [Fact]
+    public void Export_CraftedSeverity_CannotLeaveTheClassAttribute()
+    {
+        // A shared plan's analysis is caller-supplied JSON, so severity can hold markup.
+        var html = ExportWithSeverity("\"><script>alert(1)</script><div class=\"");
+
+        Assert.DoesNotContain("<script>alert(1)</script>", html);
+        Assert.Contains("<div class=\"warning-item info\">", html);
+        Assert.Contains("&lt;script&gt;alert(1)&lt;/script&gt;", html);
+    }
+
+    [Fact]
+    public void Export_NullSeverity_ExportsAsInfo()
+    {
+        // JSON can send "severity": null, and the export used to throw on it.
+        var html = ExportWithSeverity(null);
+
+        Assert.Contains("<div class=\"warning-item info\">", html);
+    }
+
+    [Fact]
+    public void Export_CraftedSeverityWithoutMarkup_CannotAddAnAttribute()
+    {
+        var html = ExportWithSeverity("x\" onmouseover=\"alert(1)");
+
+        Assert.DoesNotContain("onmouseover=\"alert(1)\"", html);
+        Assert.Contains("<div class=\"warning-item info\">", html);
+    }
+
+    [Fact]
+    public void Export_CraftedSeverityOnAnOperator_IsMappedToo()
+    {
+        // Operator warnings reach the same list through the operator tree.
+        var html = ExportWithSeverity("\"><script>alert(1)</script><div class=\"", onOperator: true);
+
+        Assert.DoesNotContain("<script>alert(1)</script>", html);
+        Assert.Contains("<div class=\"warning-item info\">", html);
+    }
+
+    private static string ExportWithSeverity(string? severity, bool onOperator = false)
+    {
+        var warning = new WarningResult { Severity = severity!, Type = "demo", Message = "demo" };
+        var statement = new StatementResult { StatementText = "SELECT 1" };
+        if (onOperator)
+            statement.OperatorTree = new OperatorResult { Warnings = { warning } };
+        else
+            statement.Warnings.Add(warning);
+
+        return HtmlExporter.Export(new AnalysisResult { Statements = { statement } }, "demo");
+    }
 }

tests/PlanViewer.Core.Tests/ReproScriptBuilderSafetyTests.cs

@@ -1,3 +1,4 @@
+using Microsoft.SqlServer.TransactSql.ScriptDom;
 using PlanViewer.Core.Services;
 
 namespace PlanViewer.Core.Tests;
@@ -126,6 +127,67 @@ public void BuildReproScript_RealWorldCompiledValues_SurviveTheFilter(
         Assert.DoesNotContain("@p = ?", sql);
     }
 
+    // The header comment shows the plan's database name. A crafted name must stay inside it:
+    // "*/" would close the comment, "/*" would open a nested one that swallows the script,
+    // and a line break could put GO on a line of its own. ScriptDom parses each script, so a
+    // statement the name smuggled out would show up as a PRINT or an extra batch.
+    //
+    // The USE line keeps the name as it is, line breaks included, on purpose. It doubles "]",
+    // and go-sqlcmd and ODBC sqlcmd do not split a batch inside a bracketed name. A client that
+    // splits at every GO line is out of scope: the statement text can hold such a line too.
+    [Theory]
+    [InlineData("master*/\nGO\nPRINT 'INJECTED';\nGO\n/*")]  // its own batch in a GO-aware client
+    [InlineData("master*/ PRINT 'INJECTED'; /*")]            // same batch, no GO needed
+    [InlineData("master\r\nGO\r\nPRINT 'INJECTED';\r\nGO")] // line breaks alone
+    [InlineData("master/*")]                                  // nested comment
+    [InlineData("master/*/")]                                 // delimiters that overlap
+    [InlineData("master*/*")]
+    [InlineData("master\vGO\fPRINT 'INJECTED';\u0085GO\u2028x\u2029y")] // VT, FF, NEL, LS, PS
+    public void BuildReproScript_HostileDatabaseName_StaysInTheHeaderComment(string databaseName)
+    {
+        var sql = ReproScriptBuilder.BuildReproScript("SELECT 1", databaseName, null, null);
+
+        var script = ParseScript(sql);
+        Assert.Single(script.Batches);
+        Assert.DoesNotContain(script.Batches[0].Statements, s => s is PrintStatement);
+
+        var header = HeaderComment(sql);
+        var databaseLine = Assert.Single(header.Split('\n'), line => line.StartsWith("Database: [", StringComparison.Ordinal));
+        Assert.StartsWith("Database: [master", databaseLine);
+        Assert.DoesNotMatch(@"[\p{Cc}\u2028\u2029]", databaseLine.TrimEnd('\r'));
+        Assert.DoesNotContain(header.Split('\n'), line => line.Trim() == "GO");
+    }
+
+    [Fact]
+    public void BuildReproScript_HostileSource_StaysInTheHeaderComment()
+    {
+        var sql = ReproScriptBuilder.BuildReproScript(
+            "SELECT 1", "db", null, null, source: "x*/ PRINT 'INJECTED'; /*");
+
+        var script = ParseScript(sql);
+        Assert.DoesNotContain(script.Batches.SelectMany(b => b.Statements), s => s is PrintStatement);
+        Assert.Contains("Source: x* / PRINT 'INJECTED'; / *", HeaderComment(sql));
+    }
+
+    private static TSqlScript ParseScript(string sql)
+    {
+        var fragment = new TSql160Parser(initialQuotedIdentifiers: true)
+            .Parse(new StringReader(sql), out var errors);
+        Assert.Empty(errors);
+        return (TSqlScript)fragment;
+    }
+
+    // Everything up to the first "*/", which must be the header's own closing line: a value
+    // that ended the comment early would put it somewhere else.
+    private static string HeaderComment(string sql)
+    {
+        Assert.StartsWith("/*", sql);
+        var end = sql.IndexOf("*/", StringComparison.Ordinal);
+        Assert.Equal('\n', sql[end - 1]);
+        Assert.DoesNotContain("/*", sql[2..end]);
+        return sql[..end];
+    }
+
     [Fact]
     public void ExtractParametersFromPlan_StillReturnsRawParameters()
     {