From 246e90a87a5a4ab47c6ab7c8ee0d2e0b0811925b Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sat, 25 Jul 2026 10:37:13 -0400 Subject: [PATCH 1/7] Quarterly maintenance: dependency bumps, CVE fix, zero-warning build Dependencies: - PlanShare: Microsoft.Data.Sqlite 10.0.9 -> 10.0.10, add direct SQLitePCLRaw.bundle_e_sqlite3 2.1.12 to clear CVE-2025-6965 (GHSA-2m69-gcr7-jv3q, High) - Microsoft.Data.Sqlite still floors the transitive at the vulnerable 2.1.11 - Avalonia 11.3.17 -> 11.3.18 (in-line; ScottPlot.Avalonia stays 5.1.58, the last Avalonia 11 build - 5.1.59+ requires Avalonia 12) - Microsoft.Data.SqlClient 7.0.1 -> 7.0.2, Extensions.Azure 1.0.0 -> 7.0.2 (version-scheme alignment with SqlClient) - ScriptDom 180.37.3 -> 180.59.2, ModelContextProtocol 1.4.0 -> 1.4.1, System.CommandLine 2.0.9 -> 2.0.10, AspNetCore WASM 10.0.9 -> 10.0.10, Test.Sdk 18.6.0 -> 18.8.1 - Remove redundant Microsoft.Win32.Registry (in-box on net10.0, NU1510) Warnings (restore zero-warning build): - Migrate drag/drop to the DataTransfer API (DragEventArgs.Data and DataFormats.Files are obsolete in Avalonia 11.3.18, CS0618 x6) CI: - actions/checkout v5 -> v7, setup-dotnet v5 -> v6, upload-artifact v6 -> v7, upload-pages-artifact v3 -> v5, deploy-pages v4 -> v5 Validated: clean Debug build 0 warnings, 182/182 tests pass, linux-x64 + osx-arm64 publish smoke OK (SkiaSharp natives present). Co-Authored-By: Claude Fable 5 --- .github/workflows/check-version-bump.yml | 4 ++-- .github/workflows/ci.yml | 4 ++-- .github/workflows/deploy-web.yml | 8 ++++---- .github/workflows/nightly.yml | 6 +++--- .github/workflows/release.yml | 6 +++--- server/PlanShare/PlanShare.csproj | 6 +++++- src/PlanViewer.App/MainWindow.FileOps.cs | 8 ++++---- src/PlanViewer.App/PlanViewer.App.csproj | 19 ++++++++++--------- src/PlanViewer.Cli/PlanViewer.Cli.csproj | 2 +- src/PlanViewer.Core/PlanViewer.Core.csproj | 6 +++--- src/PlanViewer.Web/PlanViewer.Web.csproj | 4 ++-- .../PlanViewer.Core.Tests.csproj | 2 +- 12 files changed, 40 insertions(+), 35 deletions(-) diff --git a/.github/workflows/check-version-bump.yml b/.github/workflows/check-version-bump.yml index c722ea7..f0d91d4 100644 --- a/.github/workflows/check-version-bump.yml +++ b/.github/workflows/check-version-bump.yml @@ -10,7 +10,7 @@ jobs: steps: - name: Checkout PR branch - uses: actions/checkout@v5 + uses: actions/checkout@v7 - name: Get PR version id: pr @@ -26,7 +26,7 @@ jobs: Write-Host "PR version: $version (from $path)" - name: Checkout main - uses: actions/checkout@v5 + uses: actions/checkout@v7 with: ref: main path: main-branch diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b356ba7..67c53c2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -24,10 +24,10 @@ jobs: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@v7 - name: Setup .NET 10.0 - uses: actions/setup-dotnet@v5 + uses: actions/setup-dotnet@v6 with: dotnet-version: 10.0.x cache: true diff --git a/.github/workflows/deploy-web.yml b/.github/workflows/deploy-web.yml index 2c3ce82..9e0f879 100644 --- a/.github/workflows/deploy-web.yml +++ b/.github/workflows/deploy-web.yml @@ -27,10 +27,10 @@ jobs: url: ${{ steps.deployment.outputs.page_url }} steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@v7 - name: Setup .NET 10.0 - uses: actions/setup-dotnet@v5 + uses: actions/setup-dotnet@v6 with: dotnet-version: 10.0.x @@ -50,10 +50,10 @@ jobs: run: cp publish/wwwroot/index.html publish/wwwroot/404.html - name: Upload Pages artifact - uses: actions/upload-pages-artifact@v3 + uses: actions/upload-pages-artifact@v5 with: path: publish/wwwroot - name: Deploy to GitHub Pages id: deployment - uses: actions/deploy-pages@v4 + uses: actions/deploy-pages@v5 diff --git a/.github/workflows/nightly.yml b/.github/workflows/nightly.yml index 3fc4fa5..3581793 100644 --- a/.github/workflows/nightly.yml +++ b/.github/workflows/nightly.yml @@ -15,7 +15,7 @@ jobs: outputs: has_changes: ${{ steps.check.outputs.has_changes }} steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@v7 with: ref: dev fetch-depth: 0 @@ -38,12 +38,12 @@ jobs: runs-on: windows-latest steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@v7 with: ref: dev - name: Setup .NET 10.0 - uses: actions/setup-dotnet@v5 + uses: actions/setup-dotnet@v6 with: dotnet-version: 10.0.x diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 608c77d..250d5aa 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -16,7 +16,7 @@ jobs: runs-on: windows-latest steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@v7 - name: Get version id: version @@ -46,7 +46,7 @@ jobs: gh release create "v${{ steps.version.outputs.VERSION }}" --title "v${{ steps.version.outputs.VERSION }}" --generate-notes --target main - name: Setup .NET 10.0 - uses: actions/setup-dotnet@v5 + uses: actions/setup-dotnet@v6 with: dotnet-version: 10.0.x @@ -143,7 +143,7 @@ jobs: - name: Upload Windows build for signing id: upload-unsigned - uses: actions/upload-artifact@v6 + uses: actions/upload-artifact@v7 with: name: App-unsigned path: publish/win-x64/ diff --git a/server/PlanShare/PlanShare.csproj b/server/PlanShare/PlanShare.csproj index 598b444..67530a6 100644 --- a/server/PlanShare/PlanShare.csproj +++ b/server/PlanShare/PlanShare.csproj @@ -7,7 +7,11 @@ - + + + diff --git a/src/PlanViewer.App/MainWindow.FileOps.cs b/src/PlanViewer.App/MainWindow.FileOps.cs index 2579ea2..2abefe3 100644 --- a/src/PlanViewer.App/MainWindow.FileOps.cs +++ b/src/PlanViewer.App/MainWindow.FileOps.cs @@ -88,9 +88,9 @@ private void OnDragOver(object? sender, DragEventArgs e) { e.DragEffects = DragDropEffects.None; - if (e.Data.Contains(DataFormats.Files)) + if (e.DataTransfer.Contains(DataFormat.File)) { - var files = e.Data.GetFiles(); + var files = e.DataTransfer.TryGetFiles(); if (files != null && files.Any(f => IsSupportedFile(f.TryGetLocalPath()))) e.DragEffects = DragDropEffects.Copy; } @@ -98,9 +98,9 @@ private void OnDragOver(object? sender, DragEventArgs e) private void OnDrop(object? sender, DragEventArgs e) { - if (!e.Data.Contains(DataFormats.Files)) return; + if (!e.DataTransfer.Contains(DataFormat.File)) return; - var files = e.Data.GetFiles(); + var files = e.DataTransfer.TryGetFiles(); if (files == null) return; foreach (var file in files) diff --git a/src/PlanViewer.App/PlanViewer.App.csproj b/src/PlanViewer.App/PlanViewer.App.csproj index 373c606..7d33949 100644 --- a/src/PlanViewer.App/PlanViewer.App.csproj +++ b/src/PlanViewer.App/PlanViewer.App.csproj @@ -9,26 +9,27 @@ - + + - - - + + + - + None All - - - + + - + diff --git a/tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj b/tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj index 80553b3..0dd84ee 100644 --- a/tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj +++ b/tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj @@ -11,7 +11,7 @@ - + From 04ae735300c0529cb96b5a12953ff59cbc066b3b Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sat, 25 Jul 2026 10:47:51 -0400 Subject: [PATCH 2/7] Security: block MCP DNS rebinding, harden generated T-SQL From the maintenance pass security review of the diff since the last pass (f57fad8). Findings triaged against the single-user-laptop calibration: fixes the one remotely-reachable vector and the two generated-SQL escaping gaps. HIGH - MCP DNS rebinding (McpHostService.cs): Kestrel listens on loopback, but nothing validated Host/Origin, so a malicious page could rebind a DNS name it controls to 127.0.0.1 and reach the server same-origin (CORS never applies), then call get_connections / get_query_store_top / get_plan_xml to exfiltrate saved server names, query text, and plan XML. Added middleware ahead of MapMcp that 403s any non-loopback Host, and any present Origin that is not a loopback origin. Verified at runtime: rebound Host -> 403, hostile Origin -> 403, legitimate loopback initialize -> 200 with full server handshake. MEDIUM - plan-derived parameters spliced into an executed batch (ReproScriptBuilder.cs): parameter names, types, and compiled values come straight off untrusted plan XML and landed raw in the generated EXECUTE sys.sp_executesql, which the app runs and the get_repro_script MCP tool serves. A crafted .sqlplan with ParameterCompiledValue="1; --" escaped the parameterization. Now names/types must match a plain identifier/type pattern or the parameter is dropped, and values must be a single self-contained literal (NULL, number, 0x binary, or a fully-quoted string) or they fall through to the existing ? placeholder. MEDIUM - identifier escaping in scripted DDL (DdlScripter.cs, SchemaQueryService.cs): BracketName did not double embedded ']' and passed through anything already starting with '[', and index key/ include column lists were never bracketed at all. An object named 'x] ; --' produced a script that ran attacker SQL when copied and executed - which is that output's entire purpose. BracketName now doubles ']' (mirroring ReproScriptBuilder), and the FOR XML PATH column lists use QUOTENAME. LOW (mechanical): permissions: {} on the pull_request_target workflow and contents: read on ci/check-version-bump; Uri.EscapeDataString on share id and delete token in the web client. Tests: 190 pass (8 new covering both injection paths). The FormatIndexes_AlreadyBracketedName test pinned the passthrough that was the bypass, so it now asserts correct escaping instead. Co-Authored-By: Claude Fable 5 --- .github/workflows/check-pr-branch.yml | 4 + .github/workflows/check-version-bump.yml | 3 + .github/workflows/ci.yml | 3 + src/PlanViewer.App/Mcp/McpHostService.cs | 41 ++++++++ src/PlanViewer.Core/Services/DdlScripter.cs | 8 +- .../Services/ReproScriptBuilder.cs | 71 ++++++++++++-- .../Services/SchemaQueryService.cs | 4 +- .../Services/PlanShareService.cs | 4 +- .../PlanViewer.Core.Tests/DdlScripterTests.cs | 19 +++- .../ReproScriptBuilderSafetyTests.cs | 98 +++++++++++++++++++ 10 files changed, 236 insertions(+), 19 deletions(-) create mode 100644 tests/PlanViewer.Core.Tests/ReproScriptBuilderSafetyTests.cs diff --git a/.github/workflows/check-pr-branch.yml b/.github/workflows/check-pr-branch.yml index 1c14086..f160a3f 100644 --- a/.github/workflows/check-pr-branch.yml +++ b/.github/workflows/check-pr-branch.yml @@ -6,6 +6,10 @@ on: - reopened - synchronize - edited + +# pull_request_target runs with the base repo's token — this job needs none of it. +permissions: {} + jobs: check-branches: runs-on: ubuntu-latest diff --git a/.github/workflows/check-version-bump.yml b/.github/workflows/check-version-bump.yml index f0d91d4..6401013 100644 --- a/.github/workflows/check-version-bump.yml +++ b/.github/workflows/check-version-bump.yml @@ -3,6 +3,9 @@ on: pull_request: branches: [main] +permissions: + contents: read + jobs: check-version: if: github.head_ref == 'dev' diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 67c53c2..8dfc5a1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -19,6 +19,9 @@ on: - 'src/PlanViewer.Ssms/**' - 'src/PlanViewer.Ssms.Installer/**' +permissions: + contents: read + jobs: build-and-test: runs-on: ubuntu-latest diff --git a/src/PlanViewer.App/Mcp/McpHostService.cs b/src/PlanViewer.App/Mcp/McpHostService.cs index 779445b..41c9dcf 100644 --- a/src/PlanViewer.App/Mcp/McpHostService.cs +++ b/src/PlanViewer.App/Mcp/McpHostService.cs @@ -3,6 +3,7 @@ using System.Threading.Tasks; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.Http; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Logging; @@ -74,6 +75,30 @@ so it never drifts from the actual release. */ .WithTools(); _app = builder.Build(); + + /* DNS-rebinding guard. Kestrel only listens on loopback, but a malicious + web page can point a DNS name it controls at 127.0.0.1 and reach this + server same-origin (CORS never applies). Reject any request whose Host + isn't a loopback name, and any browser request whose Origin isn't a + loopback origin, before it can touch an MCP endpoint. */ + _app.Use(async (context, next) => + { + if (!IsLoopbackHost(context.Request.Host.Host)) + { + context.Response.StatusCode = StatusCodes.Status403Forbidden; + return; + } + + var origin = context.Request.Headers.Origin; + if (origin.Count > 0 && !IsLoopbackOrigin(origin[0])) + { + context.Response.StatusCode = StatusCodes.Status403Forbidden; + return; + } + + await next(); + }); + _app.MapMcp(); await _app.RunAsync(stoppingToken); @@ -88,6 +113,22 @@ so it never drifts from the actual release. */ } } + private static bool IsLoopbackHost(string host) + { + /* HostString.Host keeps IPv6 brackets ([::1]); Uri.Host strips them. */ + return host.Equals("localhost", StringComparison.OrdinalIgnoreCase) + || host == "127.0.0.1" + || host == "[::1]" + || host == "::1"; + } + + private static bool IsLoopbackOrigin(string? origin) + { + return origin != null + && Uri.TryCreate(origin, UriKind.Absolute, out var uri) + && IsLoopbackHost(uri.Host); + } + public override async Task StopAsync(CancellationToken cancellationToken) { if (_app != null) diff --git a/src/PlanViewer.Core/Services/DdlScripter.cs b/src/PlanViewer.Core/Services/DdlScripter.cs index 3405606..e8c9c9a 100644 --- a/src/PlanViewer.Core/Services/DdlScripter.cs +++ b/src/PlanViewer.Core/Services/DdlScripter.cs @@ -237,9 +237,9 @@ public static string FormatColumns(string objectName, IReadOnlyList private static string BracketName(string name) { - // Already bracketed - if (name.StartsWith('[')) - return name; - return $"[{name}]"; + // Double embedded ']' so a name can't terminate the identifier early — + // this output exists to be copy/pasted and executed. Callers always pass + // raw catalog names, never pre-bracketed ones. + return "[" + name.Replace("]", "]]") + "]"; } } diff --git a/src/PlanViewer.Core/Services/ReproScriptBuilder.cs b/src/PlanViewer.Core/Services/ReproScriptBuilder.cs index d70e1f7..0cf87e1 100644 --- a/src/PlanViewer.Core/Services/ReproScriptBuilder.cs +++ b/src/PlanViewer.Core/Services/ReproScriptBuilder.cs @@ -59,12 +59,20 @@ public static string BuildReproScript( warnings.Add("Plan XML not available — parameters could not be extracted"); } + /* Plan XML is untrusted input: names, types, and compiled values come straight + off ParameterList attributes. Drop parameters whose name or type isn't a + plausible T-SQL token before anything is interpolated — the name lands in + the warning comment and the sp_executesql assignment list. */ + var safeParameters = parameters + .Where(p => IsValidParameterName(p.Name) && IsValidDataType(p.DataType)) + .ToList(); + /* Check for temp tables and table variables in query text */ var tempTableWarnings = DetectTempTablesAndTableVariables(queryText); warnings.AddRange(tempTableWarnings); /* Check for parameters with missing compiled values */ - var missingValueParams = parameters.Where(p => string.IsNullOrEmpty(p.CompiledValue)).ToList(); + var missingValueParams = safeParameters.Where(p => string.IsNullOrEmpty(p.CompiledValue)).ToList(); if (missingValueParams.Count > 0) { warnings.Add($"Parameters with missing values (set to ?): {string.Join(", ", missingValueParams.Select(p => p.Name))}. Fill in values before executing."); @@ -138,15 +146,20 @@ Double any ']' in the identifier so names like 'cool]stuff' still parse. */ sb.AppendLine("SET NOCOUNT ON;"); sb.AppendLine(); - /* Query body — wrap in sp_executesql if parameters found */ - if (parameters.Count > 0) + /* Query body — wrap in sp_executesql if parameters found. Values that + aren't a single self-contained literal are emitted as ? so a crafted + plan can't splice statements into the generated batch. */ + if (safeParameters.Count > 0) { /* Build parameter declaration and value assignment */ - var paramDecl = string.Join(", ", parameters.Select(p => $"{p.Name} {p.DataType}")); - var paramValues = parameters.Select(p => + var paramDecl = string.Join(", ", safeParameters.Select(p => $"{p.Name} {p.DataType}")); + var paramValues = safeParameters.Select(p => { - /* Use ? for missing values so query can't accidentally run with wrong data */ - var value = string.IsNullOrEmpty(p.CompiledValue) ? "?" : p.CompiledValue; + /* Use ? for missing or unsafe values so query can't accidentally run + with wrong data */ + var value = string.IsNullOrEmpty(p.CompiledValue) || !IsSafeLiteral(p.CompiledValue) + ? "?" + : p.CompiledValue; return $" {p.Name} = {value}"; }); @@ -403,6 +416,50 @@ private static string EscapeSqlString(string value) return value.Replace("'", "''"); } + /// + /// Validates a parameter name from plan XML as a plain @identifier. + /// Anything else is dropped from the generated script. + /// + private static bool IsValidParameterName(string name) + { + return Regex.IsMatch(name, @"^@[\p{L}_@#$][\p{L}\p{Nd}_@#$]*$"); + } + + /// + /// Validates a parameter data type from plan XML: type name with optional + /// schema prefix, brackets, and (size/precision) suffix. No quotes or comment + /// characters, so it can't disturb the sp_executesql declaration list. + /// + private static bool IsValidDataType(string dataType) + { + return Regex.IsMatch(dataType, @"^[\p{L}\p{Nd}_\[\]., ()]+$"); + } + + /// + /// Accepts a compiled value only when it's a single self-contained literal: + /// NULL, a number, a 0x binary value, or one complete N'...' string with every + /// embedded quote doubled. Such values can't escape the @param = value slot. + /// + private static bool IsSafeLiteral(string value) + { + if (value.Equals("NULL", StringComparison.OrdinalIgnoreCase)) + return true; + + /* Integer/decimal/float/money forms: -12, 3.14, 1.5E+3, $9.99 */ + if (Regex.IsMatch(value, @"^-?\$?\d+(\.\d+)?([eE][+-]?\d+)?$")) + return true; + + /* Binary literal */ + if (Regex.IsMatch(value, @"^0x[0-9A-Fa-f]*$")) + return true; + + /* One complete string literal — every embedded quote must be doubled */ + if (Regex.IsMatch(value, @"^N?'([^']|'')*'$")) + return true; + + return false; + } + /// /// Finds @variables in query text that aren't in the extracted parameter list. /// These are likely local variables without sniffed values. diff --git a/src/PlanViewer.Core/Services/SchemaQueryService.cs b/src/PlanViewer.Core/Services/SchemaQueryService.cs index 2105654..aba076b 100644 --- a/src/PlanViewer.Core/Services/SchemaQueryService.cs +++ b/src/PlanViewer.Core/Services/SchemaQueryService.cs @@ -57,7 +57,7 @@ public static class SchemaQueryService i.is_unique, i.is_primary_key, STUFF(( - SELECT ', ' + c.name + CASE WHEN ic2.is_descending_key = 1 THEN ' DESC' ELSE '' END + SELECT ', ' + QUOTENAME(c.name) + CASE WHEN ic2.is_descending_key = 1 THEN ' DESC' ELSE '' END FROM sys.index_columns AS ic2 JOIN sys.columns AS c ON c.object_id = ic2.object_id AND c.column_id = ic2.column_id WHERE ic2.object_id = i.object_id AND ic2.index_id = i.index_id AND ic2.is_included_column = 0 @@ -65,7 +65,7 @@ ORDER BY ic2.key_ordinal FOR XML PATH('') ), 1, 2, '') AS key_columns, ISNULL(STUFF(( - SELECT ', ' + c.name + SELECT ', ' + QUOTENAME(c.name) FROM sys.index_columns AS ic2 JOIN sys.columns AS c ON c.object_id = ic2.object_id AND c.column_id = ic2.column_id WHERE ic2.object_id = i.object_id AND ic2.index_id = i.index_id AND ic2.is_included_column = 1 diff --git a/src/PlanViewer.Web/Services/PlanShareService.cs b/src/PlanViewer.Web/Services/PlanShareService.cs index c9c02ee..3368dfb 100644 --- a/src/PlanViewer.Web/Services/PlanShareService.cs +++ b/src/PlanViewer.Web/Services/PlanShareService.cs @@ -67,14 +67,14 @@ public async Task ShareAsync(AnalysisResult result, string text public async Task DeleteAsync(string shareId, string deleteToken) { - var response = await _http.DeleteAsync($"{ApiBase}/api/plans/{shareId}?token={deleteToken}"); + var response = await _http.DeleteAsync($"{ApiBase}/api/plans/{Uri.EscapeDataString(shareId)}?token={Uri.EscapeDataString(deleteToken)}"); if (!response.IsSuccessStatusCode) throw new PlanShareException("Failed to delete shared plan."); } public async Task LoadAsync(string id) { - var response = await _http.GetAsync($"{ApiBase}/api/plans/{id}"); + var response = await _http.GetAsync($"{ApiBase}/api/plans/{Uri.EscapeDataString(id)}"); if (!response.IsSuccessStatusCode) { throw new PlanShareException(response.StatusCode == HttpStatusCode.NotFound diff --git a/tests/PlanViewer.Core.Tests/DdlScripterTests.cs b/tests/PlanViewer.Core.Tests/DdlScripterTests.cs index 5ab7531..3874118 100644 --- a/tests/PlanViewer.Core.Tests/DdlScripterTests.cs +++ b/tests/PlanViewer.Core.Tests/DdlScripterTests.cs @@ -91,13 +91,24 @@ public void FormatIndexes_Columnstore_EmitsColumnstoreCreate() } [Fact] - public void FormatIndexes_AlreadyBracketedName_IsNotDoubleBracketed() + public void FormatIndexes_NameContainingBracket_IsEscapedNotPassedThrough() { - // Canonical behavior: BracketName leaves an already-bracketed identifier alone. + // These scripts exist to be copy/pasted and executed, so a ']' in a catalog + // name must not be able to close the identifier early and start new T-SQL. + var sql = DdlScripter.FormatIndexes("dbo.T", new[] { Index("IX] ; DROP TABLE dbo.T --", "NONCLUSTERED", "A") }); + + Assert.Contains("[IX]] ; DROP TABLE dbo.T --]", sql); + Assert.DoesNotContain("[IX] ; DROP TABLE dbo.T --]", sql); + } + + [Fact] + public void FormatIndexes_NameLookingBracketed_IsStillEscaped() + { + // Catalog names are always raw, so a leading '[' is part of the name, not + // pre-quoting — escaping it is what keeps a crafted name inert. var sql = DdlScripter.FormatIndexes("dbo.T", new[] { Index("[IX_T]", "NONCLUSTERED", "A") }); - Assert.Contains("[IX_T]", sql); - Assert.DoesNotContain("[[IX_T]]", sql); + Assert.Contains("[[IX_T]]]", sql); } [Fact] diff --git a/tests/PlanViewer.Core.Tests/ReproScriptBuilderSafetyTests.cs b/tests/PlanViewer.Core.Tests/ReproScriptBuilderSafetyTests.cs new file mode 100644 index 0000000..eea76cb --- /dev/null +++ b/tests/PlanViewer.Core.Tests/ReproScriptBuilderSafetyTests.cs @@ -0,0 +1,98 @@ +using PlanViewer.Core.Services; + +namespace PlanViewer.Core.Tests; + +/// +/// Plan XML is untrusted input — a .sqlplan can be crafted or emailed. These cover the +/// generated script staying inert: the repro script is executed by ActualPlanExecutor +/// and handed to users to run, so a hostile ParameterList must not reach it intact. +/// +public class ReproScriptBuilderSafetyTests +{ + private static string PlanWithParameter(string column, string dataType, string compiledValue) => + $""" + + + + + + + + + + + + """; + + [Fact] + public void BuildReproScript_LegitimateParameter_IsEmittedVerbatim() + { + var plan = PlanWithParameter("@id", "int", "(42)"); + var sql = ReproScriptBuilder.BuildReproScript("SELECT 1", "db", plan, null); + + Assert.Contains("EXECUTE sys.sp_executesql", sql); + Assert.Contains("@id int", sql); + Assert.Contains("@id = 42", sql); + } + + [Fact] + public void BuildReproScript_StringParameter_KeepsQuotedLiteral() + { + var plan = PlanWithParameter("@name", "nvarchar(50)", "N'Erik'"); + var sql = ReproScriptBuilder.BuildReproScript("SELECT 1", "db", plan, null); + + Assert.Contains("@name = N'Erik'", sql); + } + + [Fact] + public void BuildReproScript_CompiledValueBreakingOutOfLiteral_BecomesPlaceholder() + { + // The injection this guards: a value that closes its own literal and appends T-SQL. + var plan = PlanWithParameter("@id", "int", "1; DROP TABLE dbo.Orders --"); + var sql = ReproScriptBuilder.BuildReproScript("SELECT 1", "db", plan, null); + + Assert.DoesNotContain("DROP TABLE", sql); + Assert.Contains("@id = ?", sql); + } + + [Fact] + public void BuildReproScript_CompiledValueWithUnbalancedQuote_BecomesPlaceholder() + { + var plan = PlanWithParameter("@name", "nvarchar(50)", "N'x'; EXEC sp_who --'"); + var sql = ReproScriptBuilder.BuildReproScript("SELECT 1", "db", plan, null); + + Assert.DoesNotContain("sp_who", sql); + Assert.Contains("@name = ?", sql); + } + + [Fact] + public void BuildReproScript_HostileParameterName_IsDroppedEntirely() + { + var plan = PlanWithParameter("@id = 1, @x int = 1; SHUTDOWN --", "int", "(1)"); + var sql = ReproScriptBuilder.BuildReproScript("SELECT 1", "db", plan, null); + + Assert.DoesNotContain("SHUTDOWN", sql); + } + + [Fact] + public void BuildReproScript_HostileDataType_IsDroppedEntirely() + { + var plan = PlanWithParameter("@id", "int; DROP TABLE dbo.Orders --", "(1)"); + var sql = ReproScriptBuilder.BuildReproScript("SELECT 1", "db", plan, null); + + Assert.DoesNotContain("DROP TABLE", sql); + } + + [Fact] + public void ExtractParametersFromPlan_StillReturnsRawParameters() + { + // The filter lives in script generation, not extraction — callers that only + // display parameters should still see what the plan actually contained. + var plan = PlanWithParameter("@id", "int", "(42)"); + var parameters = ReproScriptBuilder.ExtractParametersFromPlan(plan); + + Assert.Single(parameters); + Assert.Equal("@id", parameters[0].Name); + Assert.Equal("42", parameters[0].CompiledValue); + } +} From 26833b2ea15178470ff9b9946b66966d8909d9a3 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sat, 25 Jul 2026 10:51:09 -0400 Subject: [PATCH 3/7] Remove unused Avalonia.Diagnostics package reference It was the only deprecated package in the solution (NuGet "Legacy", superseded by AvaloniaUI.DiagnosticsSupport), and nothing referenced it: there is no AttachDevTools() call anywhere in the tree, so the DevTools inspector it provides was never reachable. Upstream says the old package "can be safely removed, as it is not used by new Developer Tools." The parked Avalonia 12 branch already dropped it for the same reason. Verified: Debug and Release both build clean (0 warnings), 190 tests pass, app launches and paints its window. dotnet list package --deprecated is now empty across every project. Kept the AVLN3001 suppression in Directory.Build.props - confirmed it still fires on 4 controls when cleared, so its documented reason is still accurate. Co-Authored-By: Claude Fable 5 --- src/PlanViewer.App/PlanViewer.App.csproj | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/PlanViewer.App/PlanViewer.App.csproj b/src/PlanViewer.App/PlanViewer.App.csproj index 7d33949..a6e4784 100644 --- a/src/PlanViewer.App/PlanViewer.App.csproj +++ b/src/PlanViewer.App/PlanViewer.App.csproj @@ -18,11 +18,6 @@ - - - None - All - From 32be789da8ce21cd200dc470b6003266459f72a6 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sat, 25 Jul 2026 10:53:32 -0400 Subject: [PATCH 4/7] Explain repro-script parameters the safety filter rejects Self-review of the previous commit: a value rejected as unsafe, or a parameter dropped for a bad name/type, silently became ? (or vanished) with no explanation. The existing warning only covered values that were missing from the plan, so the output read as a tool bug rather than a deliberate refusal. Both cases now produce a warning naming the affected parameters. Also added 10 real-world compiled-value cases as a Theory so the filter cannot quietly get strict enough to degrade ordinary repro scripts: parenthesized and negative integers, bit, decimal, scientific-notation float, money, binary, quoted date literals, doubled quotes inside a string, and NULL. Tests: 202 pass (12 new). Co-Authored-By: Claude Fable 5 --- .../Services/ReproScriptBuilder.cs | 18 ++++++++ .../ReproScriptBuilderSafetyTests.cs | 43 +++++++++++++++++++ 2 files changed, 61 insertions(+) diff --git a/src/PlanViewer.Core/Services/ReproScriptBuilder.cs b/src/PlanViewer.Core/Services/ReproScriptBuilder.cs index 0cf87e1..c19569c 100644 --- a/src/PlanViewer.Core/Services/ReproScriptBuilder.cs +++ b/src/PlanViewer.Core/Services/ReproScriptBuilder.cs @@ -78,6 +78,24 @@ the warning comment and the sp_executesql assignment list. */ warnings.Add($"Parameters with missing values (set to ?): {string.Join(", ", missingValueParams.Select(p => p.Name))}. Fill in values before executing."); } + /* Values that aren't a single self-contained literal also become ?, so say why + rather than leaving an unexplained placeholder. */ + var unsafeValueParams = safeParameters + .Where(p => !string.IsNullOrEmpty(p.CompiledValue) && !IsSafeLiteral(p.CompiledValue)) + .ToList(); + if (unsafeValueParams.Count > 0) + { + warnings.Add($"Parameters whose compiled value was not a simple literal (set to ?): {string.Join(", ", unsafeValueParams.Select(p => p.Name))}. Fill in values before executing."); + } + + /* Parameters dropped entirely because the plan's name or data type wasn't a + plain T-SQL token — the script would be incomplete, so don't stay silent. */ + var droppedCount = parameters.Count - safeParameters.Count; + if (droppedCount > 0) + { + warnings.Add($"{droppedCount} parameter(s) omitted — the plan's parameter name or data type was not a valid T-SQL identifier. Declare them manually before executing."); + } + /* Check for local variables: query has parameter prefix but plan has no/few parameters */ var trimmedQuery = queryText.Trim(); var cleanedQuery = StripParameterPrefix(trimmedQuery); diff --git a/tests/PlanViewer.Core.Tests/ReproScriptBuilderSafetyTests.cs b/tests/PlanViewer.Core.Tests/ReproScriptBuilderSafetyTests.cs index eea76cb..68549b5 100644 --- a/tests/PlanViewer.Core.Tests/ReproScriptBuilderSafetyTests.cs +++ b/tests/PlanViewer.Core.Tests/ReproScriptBuilderSafetyTests.cs @@ -83,6 +83,49 @@ public void BuildReproScript_HostileDataType_IsDroppedEntirely() Assert.DoesNotContain("DROP TABLE", sql); } + [Fact] + public void BuildReproScript_UnsafeValue_ExplainsThePlaceholder() + { + // A bare ? with no explanation would read as a tool bug rather than a + // deliberate refusal to emit the plan's value. + var plan = PlanWithParameter("@id", "int", "1; DROP TABLE dbo.Orders --"); + var sql = ReproScriptBuilder.BuildReproScript("SELECT 1", "db", plan, null); + + Assert.Contains("not a simple literal", sql); + Assert.Contains("@id", sql); + } + + [Fact] + public void BuildReproScript_DroppedParameter_IsReportedInWarnings() + { + var plan = PlanWithParameter("@id", "int; DROP TABLE dbo.Orders --", "(1)"); + var sql = ReproScriptBuilder.BuildReproScript("SELECT 1", "db", plan, null); + + Assert.Contains("1 parameter(s) omitted", sql); + } + + [Theory] + [InlineData("int", "(42)", "42")] // parenthesized integer + [InlineData("int", "(-7)", "-7")] // negative + [InlineData("bit", "(0)", "0")] // bit + [InlineData("decimal(18,2)", "(1.50)", "1.50")] // decimal + [InlineData("float", "(1.0000000000000000e+000)", "1.0000000000000000e+000")] // scientific + [InlineData("money", "($10.50)", "$10.50")] // money + [InlineData("varbinary(8)", "(0x1234ABCD)", "0x1234ABCD")] // binary + [InlineData("datetime", "('2024-01-01 00:00:00.000')", "'2024-01-01 00:00:00.000'")] // date literal + [InlineData("nvarchar(50)", "N'O''Brien'", "N'O''Brien'")] // doubled quote inside + [InlineData("int", "NULL", "NULL")] // null + public void BuildReproScript_RealWorldCompiledValues_SurviveTheFilter( + string dataType, string compiledValue, string expected) + { + // Guards against the filter being so strict it degrades ordinary repro scripts. + var plan = PlanWithParameter("@p", dataType, compiledValue.Replace("\"", """)); + var sql = ReproScriptBuilder.BuildReproScript("SELECT 1", "db", plan, null); + + Assert.Contains($"@p = {expected}", sql); + Assert.DoesNotContain("@p = ?", sql); + } + [Fact] public void ExtractParametersFromPlan_StillReturnsRawParameters() { From 177295183beef41948787d05cbf72981bae594b3 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sat, 25 Jul 2026 11:04:38 -0400 Subject: [PATCH 5/7] Add Claude Code PR review workflows claude-code-review.yml runs automatically on every non-draft PR (opened/synchronize/ready_for_review) and posts a top-level comment plus inline comments. The prompt encodes the repo conventions CI cannot check: zero-warning standard, the hand-maintained PlanViewer.Ssms version pair, the linked-source requirement in PlanViewer.Web.csproj, TRY_CAST over TRY_CONVERT, and the security severity calibration for a single-user local install so it does not file loopback issues as High. claude.yml is the on-demand @claude mention workflow. Fork PRs are deliberately excluded from the automatic review: GitHub withholds repository secrets from pull_request runs on forks and issues a read-only token, so the job would fail rather than review. The job is skipped instead of failing a contributor's PR with a red X. The fix is NOT pull_request_target, which would expose a writable token and this repo's secrets to an agent running untrusted fork code. claude.yml covers fork PRs instead - comment events run in the base-repo context, and the action checks the commenter has write access first. Cost/noise controls: concurrency cancels an in-flight review when a second push lands, docs-only changes are filtered out via paths-ignore mirroring ci.yml, drafts are skipped, and --max-turns is capped at 20. Co-Authored-By: Claude Fable 5 --- .github/workflows/claude-code-review.yml | 103 +++++++++++++++++++++++ .github/workflows/claude.yml | 43 ++++++++++ 2 files changed, 146 insertions(+) create mode 100644 .github/workflows/claude-code-review.yml create mode 100644 .github/workflows/claude.yml diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml new file mode 100644 index 0000000..d73366c --- /dev/null +++ b/.github/workflows/claude-code-review.yml @@ -0,0 +1,103 @@ +name: Claude Code Review + +# Automatic review on every PR. Fork PRs are skipped on purpose - see the note +# at the bottom of this file. +on: + pull_request: + types: [opened, synchronize, ready_for_review] + paths-ignore: + - '**.md' + - 'LICENSE' + - '.gitattributes' + - '.gitignore' + - 'CITATION.cff' + - 'llms.txt' + - '.github/ISSUE_TEMPLATE/**' + - 'docs/**' + - 'screenshots/**' + +permissions: + contents: read + pull-requests: write + id-token: write + +# One review at a time per PR; a rapid second push cancels the in-flight run +# instead of paying for two reviews of the same branch. +concurrency: + group: claude-review-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + review: + # Drafts aren't ready for review, and a fork PR gets no secrets (so + # CLAUDE_CODE_OAUTH_TOKEN would be empty) and a read-only token it could + # not post with. Skip rather than fail a contributor's PR with a red X. + if: | + github.event.pull_request.draft == false && + github.event.pull_request.head.repo.full_name == github.repository + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v7 + with: + fetch-depth: 1 + + - uses: anthropics/claude-code-action@v1 + with: + claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} + prompt: | + REPO: ${{ github.repository }} + PR NUMBER: ${{ github.event.pull_request.number }} + + Review this pull request. The PR branch is already checked out in + the working directory. + + Performance Studio is a cross-platform Avalonia desktop app (plus a + CLI, a Blazor WASM viewer, and an SSMS extension) that analyzes SQL + Server execution plans. Prioritize, in this order: + + 1. Correctness and crashes - null/empty plan XML, malformed + showplan attributes, off-by-one in operator tree walking. + 2. Untrusted input. Execution plan XML is untrusted (users open + .sqlplan files they were emailed). Any generated T-SQL must stay + inert: identifiers bracket-escaped with ']' doubled, values + emitted only as self-contained literals. Flag string + concatenation into SQL that isn't a SqlParameter. + 3. Security, calibrated to the deployment. This runs on a + single-user personal machine and its MCP tools are read-only, so + loopback-bound, opt-in, or local-IPC issues are Low here. Reserve + High for remotely reachable vectors (missing Host/Origin checks, + DNS rebinding) or credential disclosure. + 4. Repo conventions that CI does not catch: + - The build standard is zero warnings. Flag new warnings and any + new NoWarn that lacks a comment explaining it. + - src/Directory.Build.props is the single source of + truth, but PlanViewer.Ssms is a legacy non-SDK project: its + version must be bumped by hand in + source.extension.vsixmanifest AND Properties/AssemblyInfo.cs. + Flag a version bump that updates one and not the others. + - A PlanViewer.Core file the Blazor app needs must also be added + as a linked in PlanViewer.Web.csproj - a + ProjectReference is deliberately not used. + - T-SQL uses TRY_CAST, never TRY_CONVERT. + 5. Test coverage for the behavior actually changed. + + Be specific and concrete. Skip praise, style nits already handled by + the compiler, and restating what the diff does. If the PR looks good, + say so briefly rather than inventing findings. + + Use `gh pr comment` for top-level feedback. + Use `mcp__github_inline_comment__create_inline_comment` (with + `confirmed: true`) to flag specific lines. + Only post GitHub comments - do not return review text as a message. + + claude_args: | + --max-turns 20 + --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*)" + +# Fork PRs: GitHub withholds repository secrets from `pull_request` runs on +# forks and issues a read-only GITHUB_TOKEN, so this workflow cannot review +# them. The fix is NOT `pull_request_target` - that would hand a writable token +# and this repo's secrets to an agent running untrusted fork code. To review a +# fork PR, comment `@claude review this` on it (claude.yml), which runs in the +# base-repo context and checks the commenter's write permission first. diff --git a/.github/workflows/claude.yml b/.github/workflows/claude.yml new file mode 100644 index 0000000..60c2d22 --- /dev/null +++ b/.github/workflows/claude.yml @@ -0,0 +1,43 @@ +name: Claude + +# On-demand: mention @claude in an issue, a PR comment, or a review comment. +# +# Unlike claude-code-review.yml this DOES work on fork PRs: issue_comment and +# review-comment events run in the base-repo context, so secrets are available +# and the token can post. The action verifies the commenter has write access +# before doing anything, so an outside user cannot trigger it. +on: + issue_comment: + types: [created] + pull_request_review_comment: + types: [created] + pull_request_review: + types: [submitted] + issues: + types: [opened, assigned] + +permissions: + contents: write + pull-requests: write + issues: write + id-token: write + +jobs: + claude: + if: | + (github.event_name == 'issue_comment' && contains(github.event.comment.body, '@claude')) || + (github.event_name == 'pull_request_review_comment' && contains(github.event.comment.body, '@claude')) || + (github.event_name == 'pull_request_review' && contains(github.event.review.body, '@claude')) || + (github.event_name == 'issues' && contains(github.event.issue.body, '@claude')) + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v7 + with: + fetch-depth: 1 + + - uses: anthropics/claude-code-action@v1 + with: + claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} + claude_args: | + --max-turns 20 From 457a158c1fe962d5a371bb8bd61f11d6171428b7 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sat, 25 Jul 2026 11:11:56 -0400 Subject: [PATCH 6/7] Bump version to 1.17.0 All three version sources, kept in sync: - src/Directory.Build.props (single source for SDK-style projects) - src/PlanViewer.Ssms/source.extension.vsixmanifest - src/PlanViewer.Ssms/Properties/AssemblyInfo.cs The two SSMS files are not covered by Directory.Build.props because PlanViewer.Ssms is a legacy non-SDK project; they have drifted on past releases, so they are bumped by hand here. Verified: clean Release build 0 warnings, 202/202 tests pass. Co-Authored-By: Claude Fable 5 --- src/Directory.Build.props | 2 +- src/PlanViewer.Ssms/Properties/AssemblyInfo.cs | 4 ++-- src/PlanViewer.Ssms/source.extension.vsixmanifest | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Directory.Build.props b/src/Directory.Build.props index eac080f..4bd8a71 100644 --- a/src/Directory.Build.props +++ b/src/Directory.Build.props @@ -15,7 +15,7 @@ Tests and server/ projects are outside src/ and are unaffected. --> - 1.16.0 + 1.17.0 Erik Darling Darling Data LLC Performance Studio diff --git a/src/PlanViewer.Ssms/Properties/AssemblyInfo.cs b/src/PlanViewer.Ssms/Properties/AssemblyInfo.cs index 2dfd404..f972400 100644 --- a/src/PlanViewer.Ssms/Properties/AssemblyInfo.cs +++ b/src/PlanViewer.Ssms/Properties/AssemblyInfo.cs @@ -7,5 +7,5 @@ [assembly: AssemblyProduct("Performance Studio for SSMS")] [assembly: AssemblyCopyright("Copyright Darling Data 2026")] [assembly: ComVisible(false)] -[assembly: AssemblyVersion("1.16.0.0")] -[assembly: AssemblyFileVersion("1.16.0.0")] +[assembly: AssemblyVersion("1.17.0.0")] +[assembly: AssemblyFileVersion("1.17.0.0")] diff --git a/src/PlanViewer.Ssms/source.extension.vsixmanifest b/src/PlanViewer.Ssms/source.extension.vsixmanifest index 604e4c5..ee1fb42 100644 --- a/src/PlanViewer.Ssms/source.extension.vsixmanifest +++ b/src/PlanViewer.Ssms/source.extension.vsixmanifest @@ -3,7 +3,7 @@ xmlns:d="http://schemas.microsoft.com/developer/vsx-schema-design/2011"> Performance Studio for SSMS From 42f7b5e6ff56fa900de3d426abab958b611ca588 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sat, 25 Jul 2026 11:19:32 -0400 Subject: [PATCH 7/7] Update CITATION.cff to the current release Its software `version` had been left at 1.2.0 since the file was added in #111 (2026-03-18), 15 minor versions behind. It likely went unnoticed because it happened to match the `cff-version: 1.2.0` schema version on line 1, which is a different field and correctly stays at 1.2.0. Co-Authored-By: Claude Fable 5 --- CITATION.cff | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CITATION.cff b/CITATION.cff index 7d14671..28f3d33 100644 --- a/CITATION.cff +++ b/CITATION.cff @@ -9,8 +9,8 @@ authors: website: "https://erikdarling.com" repository-code: "https://github.com/erikdarlingdata/PerformanceStudio" license: MIT -version: "1.2.0" -date-released: "2026-03-18" +version: "1.17.0" +date-released: "2026-07-25" keywords: - sql-server - execution-plan