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 c722ea7..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' @@ -10,7 +13,7 @@ jobs: steps: - name: Checkout PR branch - uses: actions/checkout@v5 + uses: actions/checkout@v7 - name: Get PR version id: pr @@ -26,7 +29,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..8dfc5a1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -19,15 +19,18 @@ on: - 'src/PlanViewer.Ssms/**' - 'src/PlanViewer.Ssms.Installer/**' +permissions: + contents: read + jobs: build-and-test: 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/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 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/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 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/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.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/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.App/PlanViewer.App.csproj b/src/PlanViewer.App/PlanViewer.App.csproj index 373c606..a6e4784 100644 --- a/src/PlanViewer.App/PlanViewer.App.csproj +++ b/src/PlanViewer.App/PlanViewer.App.csproj @@ -9,26 +9,22 @@ - + + - - - - - - None - All - + + + - - - + + - + 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/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 @@ - + diff --git a/tests/PlanViewer.Core.Tests/ReproScriptBuilderSafetyTests.cs b/tests/PlanViewer.Core.Tests/ReproScriptBuilderSafetyTests.cs new file mode 100644 index 0000000..68549b5 --- /dev/null +++ b/tests/PlanViewer.Core.Tests/ReproScriptBuilderSafetyTests.cs @@ -0,0 +1,141 @@ +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 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() + { + // 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); + } +}