Skip to content

Harden the app's local surfaces: pipe, About links, SSMS temp plans, MCP host - #596

Merged
erikdarlingdata merged 2 commits into
devfrom
fix/harden-app-surfaces
Sep 28, 2026
Merged

erikdarlingdata merged 2 commits into
devfrom
fix/harden-app-surfaces

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Sep 28, 2026 •

Copy link
Copy Markdown
Owner

What does this PR do?

Tightens four places where the desktop app deals with other processes or with the shell.

Single-instance pipe (SingleInstance, Program, MainWindow, and the SSMS extension's AppLauncher):

  • The app creates its pipe with PipeOptions.CurrentUserOnly, so only the user who started the app can connect to it.
  • The app's own sender uses the same option. It hands a file only to a pipe that the same user created. On Windows, .NET also compares elevation, so an elevated launch no longer hands its file to a running instance that is not elevated. It opens its own window instead.
  • The SSMS extension runs on .NET Framework, which has no such option on the client, so it checks the pipe's owner itself. It accepts the user and the token's owner, so an elevated SSMS still reaches an app that is not elevated. When the owner is anyone else, it launches the app instead of sending the path.
  • The pipe's two ends are now made in SingleInstance, so a test can check them.

About window: OpenUrl opens only absolute http and https addresses. The update link's address comes from the release server's reply.

Plans sent from SSMS: the extension hands each plan to the app as ssms_plan_*.sqlplan in the temp folder. The app now deletes that file once the plan loads. A file that fails to load stays for the extension's sweep. The tab then works like a pasted plan. It has no file behind it, it is not on the recent list, and it does not come back at the next start. "Save .sqlplan" still saves it.

The app deletes only a file with that name directly in the temp folder, and only on Windows. The extension's sweep of files older than an hour stays as a backstop.

MCP server (McpHostService):

  • The host is now built from WebApplication.CreateEmptyBuilder, with Kestrel and routing added in code. CreateBuilder also read appsettings files from the working directory and the process's environment variables. A Kestrel section in either one added endpoints beside the loopback one.
  • A request from an address that is not loopback gets a 403. This runs before the existing Host and Origin checks.

Which component(s) does this affect?

  • Desktop App (PlanViewer.App)
  • Core Library (PlanViewer.Core)
  • CLI Tool (PlanViewer.Cli)
  • SSMS Extension (PlanViewer.Ssms)
  • Tests
  • Documentation

How was this tested?

  • New tests:
    • McpHostServiceTests: starts the real server on a free loopback port. An MCP client lists the tools and calls list_plans over HTTP. A request with another host name gets a 403. Loopback addresses pass the address check, IPv4-mapped ones included, and other addresses fail it.
    • SingleInstanceTests: a line sent through the pipe arrives. On Windows, the pipe's access list has one entry, for its owner.
    • AboutWindowLinkTests: http and https addresses open. File paths, network paths and other schemes do not.
    • SsmsHandoffTests: a plan from SSMS is deleted once it is read, and it is on neither the recent list nor the restore list. A file with the same name in another folder is kept.
  • Checked by hand:
    • With an environment variable that adds a Kestrel endpoint, the old host opened a second listener and the new host did not.
    • The SSMS extension's owner check, run on .NET Framework, passes for the new pipe and for the pipe an older app creates. The app's new sender also reaches an older app's pipe. So either side can be updated first.
  • The SSMS extension builds with MSBuild, as in the release workflow, with no warnings.
  • Full suite on Windows: 1,016 total, 1,014 passed, 0 failed, 2 skipped. One of the skips is the new test that runs only off Windows. The Release build has 0 warnings.

Checklist

  • I have read the contributing guide
  • My code builds with zero warnings (Release build, --no-incremental)
  • All tests pass (dotnet test)
  • I have not introduced any hardcoded credentials or server names

🤖 Generated with Claude Code

https://claude.ai/code/session_01G625JBNh45iTR1hpT4CxNR

erikdarlingdata and others added 2 commits September 28, 2026 14:56
…MCP host

- Single-instance pipe: the app's server and client use
  PipeOptions.CurrentUserOnly (built in SingleInstance). The SSMS
  extension checks the pipe owner by hand (.NET Framework has no client
  option), accepting the user or token owner SID.
- About window: OpenUrl opens only absolute http/https addresses.
- SSMS handoff: the app deletes ssms_plan_*.sqlplan from the temp folder
  once read, and treats the tab like a pasted plan (no source path, not
  recent, not restored).
- MCP host: CreateEmptyBuilder + UseKestrelCore + AddRoutingCore, so no
  appsettings or environment Kestrel section can add endpoints; requests
  from a non-loopback address get a 403.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G625JBNh45iTR1hpT4CxNR
A file that fails to load, or to delete, is left for the extension's
sweep of files older than an hour.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G625JBNh45iTR1hpT4CxNR
@erikdarlingdata
erikdarlingdata marked this pull request as ready for review September 28, 2026 19:32
@claude

claude Bot commented Sep 28, 2026

Copy link
Copy Markdown

Reviewed the diff. No blocking issues. Two low-severity notes:

  1. IsSsmsHandoffFile (MainWindow.FileOps.cs) deletes any ssms_plan_*.sqlplan sitting directly in %TEMP% once it loads, even if the user put it there themselves. For example, a plan they saved or extracted there and opened by hand. The name, folder and OS checks make this unlikely, but nothing proves the extension wrote the file. If you want it airtight, have the extension pass a marker, such as a dedicated subfolder or a command-line flag, instead of relying on the name pattern.

  2. Elevation behavior change. With CurrentUserOnly on the app's own client, an elevated launch no longer reaches a non-elevated running instance. It opens a second window instead, so single-instance is no longer guaranteed across elevation. The PR description already says this, so I'm only noting it as a deliberate tradeoff.

The rest looks fine: the pipe owner check, the http/https-only OpenUrl, the loopback address check and the empty Kestrel builder. Tests cover each of these. This PR doesn't touch any version files, so the Ssms version-sync rule doesn't apply.

@erikdarlingdata
erikdarlingdata merged commit 027afdc into dev Sep 28, 2026
6 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/harden-app-surfaces branch September 28, 2026 19:38
@erikdarlingdata erikdarlingdata mentioned this pull request Sep 29, 2026
2 of 8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant