Skip to content

Feature/mcp port validation#453

Open
HannahVernon wants to merge 5 commits intoerikdarlingdata:devfrom
HannahVernon:feature/mcp-port-validation
Open

Feature/mcp port validation#453
HannahVernon wants to merge 5 commits intoerikdarlingdata:devfrom
HannahVernon:feature/mcp-port-validation

Conversation

@HannahVernon
Copy link
Contributor

What does this PR do?

Added validation for the MCP port settings in the Settings window of both Full Dashboard and Lite Dashboard such that the user-defined port must be between 1024 and 65536, and added details to the documentation stating same.

MCP Server Port Validation

Adds TCP port conflict detection and range validation for the MCP server in both Dashboard and Lite editions.

Problem

Previously, the MCP server port setting had no validation beyond a basic numeric check. Users could enter invalid port numbers (e.g., 0), privileged ports (1–1023), or ports already in use by another process — all without any feedback. At startup, the MCP server would silently fail if the port was occupied.

Changes

New: PortUtilityService (Dashboard + Lite)

  • IsTcpPortListeningAsync — checks if a TCP port is already in use (non-invasive, reads OS connection table)
  • CanBindTcpPortAsync — attempts to bind a TcpListener to verify availability
  • GetFreeTcpPortAsync — asks the OS to allocate an available port (via the new "Auto" button in the Settings Window)

Settings Window validation (Dashboard + Lite)

  • Enforces port range 1024–65535; rejects well-known privileged ports (0–1023) with an explanatory message
  • Checks for port conflicts when MCP is enabled and the port has changed
  • Blocks save and keeps the Settings window open on validation failure

Startup port check (Dashboard + Lite)

  • Verifies the configured MCP port is available before starting the server
  • Logs an error and skips MCP startup if the port is occupied (instead of silently failing)

Documentation

  • Added port requirements, conflict detection, and startup validation notes to the MCP Setup section in README

Files changed (7)

File Change
Dashboard/Services/PortUtilityService.cs New — TCP port utility service
Lite/Services/PortUtilityService.cs New — Lite-namespace copy
Dashboard/SettingsWindow.xaml.cs Port range + conflict validation on save
Dashboard/MainWindow.xaml.cs Async port check before MCP startup
Lite/Windows/SettingsWindow.xaml.cs Port range + conflict validation on save
Lite/MainWindow.xaml.cs Async port check before MCP startup
README.md MCP port requirements documentation

Which component(s) does this affect?

  • [✅] Full Dashboard
  • [✅] Lite Dashboard
  • [✅] Documentation

How was this tested?

  • Build: dotnet build PerformanceMonitor.sln — 0 errors
  • Manually tested the settings UI in both apps for invalid port numbers, valid in-use port numbers, and tested the "Auto" button.
  • Tested with claude mcp add --transport http --scope user sql-monitor http://localhost:5151/ using various port numbers, including auto-assigned port numbers, in both Dashboard and Lite.

Checklist

  • [✅] I have read the contributing guide
  • [✅] My code builds with zero warnings (dotnet build -c Debug)
  • [✅] I have tested my changes against at least one SQL Server version
  • [✅] I have not introduced any hardcoded credentials or server names

HannahVernon and others added 5 commits March 6, 2026 10:02
- Add PortUtilityService with IsTcpPortListeningAsync, CanBindTcpPortAsync, and GetFreeTcpPortAsync methods (both Dashboard and Lite)
- Validate port availability in Settings before saving when MCP is enabled and port changed
- Check port availability at startup before starting MCP server; log error and skip if occupied
- Feature parity: both Dashboard and Lite have identical validation behavior

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Show error for invalid MCP port values in both Dashboard and Lite Settings
- Enforce minimum port 1024 to avoid well-known privileged ports (0-1023)
- Use IPEndPoint.MaxPort instead of magic number 65535
- Error message explains privileged port reservation

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add 'Auto' button next to MCP Port in Settings (both Dashboard and Lite)
- Button calls GetFreeTcpPortAsync to find an available port on localhost
- All IsTcpPortListeningAsync calls now explicitly pass IPAddress.Loopback to match MCP server binding

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…-memory objects in MainWindow_Closing(), detection of MCP Server status at Settings_Click() for context sensitivity around MCP status in the Settings window along with a call to RestartMcpServerIfNeeded() after the user makes changes to MCP-related settings. Made similar changes to the Lite app.
Copy link
Owner

@erikdarlingdata erikdarlingdata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review — 3 bugs, 3 minor items

Nice feature — port validation, the Auto button, and the auto-restart on settings change are all welcome improvements. A few issues to address before merging.


Bug 1: Port conflict check skipped when enabling MCP with unchanged port

Files:

  • Dashboard/SettingsWindow.xaml.cs — inside OkButton_Click, the new port validation block
  • Lite/Windows/SettingsWindow.xaml.cs — inside SaveMcpSettings()

Problem: Both editions only run the port-in-use check when the port value changes:

Dashboard:

if (prefs.McpEnabled && mcpPort != prefs.McpPort)

Lite:

if (newEnabled && newPort != oldPort)

If MCP was previously disabled and the user enables it without changing the default port (5151), newPort == oldPort evaluates to true so the entire port conflict check is skipped. If another process is already listening on 5151, the user gets no warning — MCP silently fails to start at runtime. The startup check logs an error, but there's no user-visible feedback.

Fix: Run the port conflict check whenever MCP will be enabled, not only when the port value changes. For example:

Dashboard — change the condition to:

if (prefs.McpEnabled)

Lite — change the condition to:

if (newEnabled)

The IsTcpPortListeningAsync call is cheap (reads the OS TCP listener table), so running it on every save when MCP is enabled has no noticeable cost.

Note on the range check: The newPort < 1024 || newPort > IPEndPoint.MaxPort range validation inside this block should also run whenever MCP is enabled, not just when the port changes. Move it outside the port-change condition or merge it with the broader if (newEnabled) check.


Bug 2: Dashboard return on MCP validation aborts ALL settings save

File: Dashboard/SettingsWindow.xaml.csOkButton_Click, around the new MCP validation block

Problem: The two new return; statements (one for port-in-use, one for invalid range) exit OkButton_Click entirely. This skips _preferencesService.SavePreferences(prefs), DialogResult = true, and Close(). If a user changes their SMTP server, alert thresholds, AND enters a bad MCP port, all of their other changes are lost.

The existing alert threshold validation (lines ~615-621) follows a different pattern: it collects errors into validationErrors, shows a warning, and continues saving the rest. MCP validation should follow the same pattern.

Fix: Instead of return, add the MCP error to validationErrors (or a separate list) and skip only prefs.McpPort = mcpPort. Let the rest of the method continue so SavePreferences, DialogResult = true, and Close() still execute. Example:

prefs.McpEnabled = McpEnabledCheckBox.IsChecked == true;
if (int.TryParse(McpPortTextBox.Text, out int mcpPort) && mcpPort >= 1024 && mcpPort <= IPEndPoint.MaxPort)
{
    if (prefs.McpEnabled)
    {
        bool inUse = Task.Run(() => PortUtilityService.IsTcpPortListeningAsync(mcpPort, IPAddress.Loopback)).GetAwaiter().GetResult();
        if (inUse)
        {
            validationErrors.Add($"Port {mcpPort} is already in use — MCP port was not changed.");
        }
        else
        {
            prefs.McpPort = mcpPort;
        }
    }
    else
    {
        prefs.McpPort = mcpPort;
    }
}
else if (prefs.McpEnabled)
{
    validationErrors.Add($"MCP port must be between 1024 and {IPEndPoint.MaxPort}.");
}

This keeps the save flowing for all other settings.


Bug 3: Lite shows "Settings saved." when MCP validation fails

File: Lite/Windows/SettingsWindow.xaml.csSaveButton_Click and SaveMcpSettings()

Problem: SaveMcpSettings() returns false both when validation fails (port in use, invalid range) AND when nothing changed. The caller doesn't distinguish these cases — it always proceeds to _saved = true and shows MessageBox.Show("Settings saved.", ...). The user sees a success message even though MCP settings were rejected.

Fix: Either:

  • Return a tri-state or separate bool to distinguish "failed" from "unchanged"
  • Show a combined message like: "Settings saved. MCP port was not changed: {reason}"
  • Or have SaveMcpSettings show its own error and return a saved bool that the caller checks before displaying the generic success message

Minor 1: CanBindTcpPortAsync is unused

Files: Dashboard/Services/PortUtilityService.cs, Lite/Services/PortUtilityService.cs

CanBindTcpPortAsync is defined in both files but never called anywhere. Consider removing it to avoid dead code. If it's intended for future use, that's fine, but it can always be added back when needed.


Minor 2: Dashboard closing handler duplicates StopMcpServerAsync

File: Dashboard/MainWindow.xaml.csMainWindow_Closing

The closing handler now has the same cancel → stop → null → dispose sequence as the new StopMcpServerAsync() method. Consider replacing the inline code with await StopMcpServerAsync() for consistency and to avoid maintaining two copies.


Minor 3: Shutdown timeout inconsistency

Dashboard StopMcpServerAsync uses a 5-second timeout; Lite uses 3 seconds. Not a functional issue, but worth aligning for consistency.


Note: Merge conflict with PR #444

Both this PR and #444 modify the same sections of Dashboard/SettingsWindow.xaml.cs and Lite/Windows/SettingsWindow.xaml.cs. Whichever merges second will need conflict resolution.

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.

2 participants