From a4f51bfadd697c179659f7e534e02e24b52026e4 Mon Sep 17 00:00:00 2001 From: yayayouyou Date: Tue, 14 Jul 2026 12:25:52 +0800 Subject: [PATCH 1/2] fix(stdio): handle spaces in Command path on Windows (cmd /d /s /c) On Windows, StdioClientTransport wraps non-cmd.exe commands as `cmd.exe /c ` and adds the arguments through ProcessStartInfo.ArgumentList. When the command is an absolute path containing a space (e.g. under "Program Files") and there is at least one argument, ArgumentList quotes both the path and the argument, so more than two quote characters follow /c. cmd then strips the first and last quote and re-parses the middle, tries to launch "C:\Program", and the server never starts. Build the cmd.exe command line by hand for the wrapping case and assign it to startInfo.Arguments directly, bypassing ArgumentList: - Invoke `cmd.exe /d /s /c ""`. /s makes cmd strip exactly the single outer pair of quotes and take the rest verbatim; /d skips AutoRun. - Quote the command and each argument (Win32 CommandLineToArgvW rules) only when it contains whitespace, a quote, or a cmd metacharacter (& | < > ^ ( )), which also makes those metacharacters literal. A bare command name is left unquoted so cmd still resolves it against the current directory and PATH. Non-Windows and the explicit cmd.exe command path are unchanged. Adds a unit test asserting the exact generated command line and a Windows-only integration test that launches a server from a directory whose path contains a space and round-trips arguments (spaces, & ^ < > |, quotes, trailing backslash). Fixes #1601 Co-Authored-By: Claude Opus 4.8 --- .../Client/StdioClientTransport.cs | 133 +++++++++++++++- .../Transport/StdioClientTransportTests.cs | 147 ++++++++++++++++++ 2 files changed, 276 insertions(+), 4 deletions(-) diff --git a/src/ModelContextProtocol.Core/Client/StdioClientTransport.cs b/src/ModelContextProtocol.Core/Client/StdioClientTransport.cs index 2e44ee34f..8d1af2763 100644 --- a/src/ModelContextProtocol.Core/Client/StdioClientTransport.cs +++ b/src/ModelContextProtocol.Core/Client/StdioClientTransport.cs @@ -63,13 +63,23 @@ public async Task ConnectAsync(CancellationToken cancellationToken = string command = _options.Command; IList? arguments = _options.Arguments; + + // On Windows, for stdio, we need to wrap non-shell commands with cmd.exe /c {command} (usually npx or uvicorn). + // The stdio transport will not work correctly if the command is not run in a shell. + string? cmdArguments = null; if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows) && !string.Equals(Path.GetFileName(command), "cmd.exe", StringComparison.OrdinalIgnoreCase)) { - // On Windows, for stdio, we need to wrap non-shell commands with cmd.exe /c {command} (usually npx or uvicorn). - // The stdio transport will not work correctly if the command is not run in a shell. - arguments = arguments is null or [] ? ["/c", command] : ["/c", command, ..arguments]; + // Build the full "cmd.exe /d /s /c ..." command line by hand rather than relying on + // ProcessStartInfo.ArgumentList. ArgumentList applies the Win32 C-runtime quoting rules to + // each argument independently, which is incompatible with how cmd.exe /c re-parses its command + // line: when more than two quote characters follow /c, cmd strips the first and last quote and + // re-parses the middle, mangling command paths that contain spaces (e.g. under "Program Files"). + // Using /s together with a single outer pair of quotes makes cmd strip exactly that outer pair + // and treat the remainder verbatim, so a space-containing command path launches correctly. + cmdArguments = GetWindowsCmdArguments(command, arguments); command = "cmd.exe"; + arguments = null; } ILogger logger = (ILogger?)_loggerFactory?.CreateLogger() ?? NullLogger.Instance; @@ -93,7 +103,13 @@ public async Task ConnectAsync(CancellationToken cancellationToken = #endif }; - if (arguments is not null) + if (cmdArguments is not null) + { + // The cmd.exe wrapper builds its own already-quoted command line (see GetWindowsCmdArguments); + // assign it directly instead of going through ArgumentList's per-argument quoting. + startInfo.Arguments = cmdArguments; + } + else if (arguments is not null) { #if NET foreach (string arg in arguments) @@ -293,6 +309,115 @@ private static string EscapeArgumentString(string argument) => WindowsCliSpecialArgumentsRegex.Replace(argument, static match => "^" + match.Value) : argument; + /// + /// Builds the raw command line passed to cmd.exe when wrapping a non-cmd.exe command on Windows. + /// + /// + /// Produces /d /s /c "<command> <arg1> <arg2> ..." where: + /// + /// /d skips any AutoRun commands, and /s together with the single outer pair of quotes + /// makes cmd.exe strip exactly that outer pair and treat the remainder verbatim. + /// The command and each argument are wrapped in double quotes when they contain whitespace, a quote, + /// or a cmd.exe metacharacter, using the Win32 CommandLineToArgvW escaping rules, so both + /// cmd.exe and the launched child process parse the tokens back to their original values. Quoting + /// also neutralizes cmd.exe metacharacters (& | < > ^ ( )), which are literal inside + /// double quotes. A bare command name is left unquoted so that cmd.exe still resolves it against the + /// current directory and PATH. + /// + /// + internal static string GetWindowsCmdArguments(string command, IList? arguments) + { + StringBuilder builder = new("/d /s /c \""); + + // The command is quoted only when it needs it (e.g. a path under "Program Files" that contains a + // space): that is what fixes #1601. A bare command name (such as "npx") is deliberately left + // unquoted, because cmd.exe resolves an unquoted bare name against the current directory and PATH + // but does not resolve a quoted one (the outer /s means cmd does not strip the inner quotes). + AppendCmdArgument(builder, command); + + if (arguments is not null) + { + foreach (string argument in arguments) + { + builder.Append(' '); + AppendCmdArgument(builder, argument); + } + } + + builder.Append('"'); + return builder.ToString(); + } + + private static void AppendCmdArgument(StringBuilder builder, string argument) + { + if (argument.Length != 0 && !RequiresCmdQuoting(argument)) + { + builder.Append(argument); + return; + } + + // Quote using the Win32 backslash/quote escaping rules (same rules as PasteArguments), so the child + // process's CommandLineToArgvW recovers the exact original argument. + builder.Append('"'); + int idx = 0; + while (idx < argument.Length) + { + char c = argument[idx++]; + if (c == '\\') + { + int backslashes = 1; + while (idx < argument.Length && argument[idx] == '\\') + { + idx++; + backslashes++; + } + + if (idx == argument.Length) + { + // Backslashes precede the closing quote, so they must be doubled. + builder.Append('\\', backslashes * 2); + } + else if (argument[idx] == '"') + { + // Backslashes precede an embedded quote, so they must be doubled and the quote escaped. + builder.Append('\\', backslashes * 2 + 1); + builder.Append('"'); + idx++; + } + else + { + builder.Append('\\', backslashes); + } + + continue; + } + + if (c == '"') + { + builder.Append('\\'); + builder.Append('"'); + continue; + } + + builder.Append(c); + } + + builder.Append('"'); + } + + private static bool RequiresCmdQuoting(string argument) + { + foreach (char c in argument) + { + if (char.IsWhiteSpace(c) || c is '"' or '&' or '|' or '<' or '>' or '^' or '(' or ')') + { + return true; + } + } + + return false; + } + private const string WindowsCliSpecialArgumentsRegexString = "[&^><|]"; #if NET diff --git a/tests/ModelContextProtocol.Tests/Transport/StdioClientTransportTests.cs b/tests/ModelContextProtocol.Tests/Transport/StdioClientTransportTests.cs index 8b7876271..3211d926e 100644 --- a/tests/ModelContextProtocol.Tests/Transport/StdioClientTransportTests.cs +++ b/tests/ModelContextProtocol.Tests/Transport/StdioClientTransportTests.cs @@ -13,6 +13,8 @@ public class StdioClientTransportTests(ITestOutputHelper testOutputHelper) : Log { public static bool IsStdErrCallbackSupported => !PlatformDetection.IsMonoRuntime; + public static bool IsWindows => RuntimeInformation.IsOSPlatform(OSPlatform.Windows); + [Fact] public async Task ConnectAsync_DoesNotLogEnvironmentVariablesAtTrace() { @@ -187,6 +189,151 @@ public async Task EscapesCliArgumentsCorrectly(string? cliArgumentValue) Assert.Equal(cliArgumentValue ?? "", content.Text); } + // Regression tests for https://github.com/modelcontextprotocol/csharp-sdk/issues/1601: + // on Windows, non-cmd.exe commands are wrapped with cmd.exe. The wrapper must launch the + // command correctly even when the command path contains a space (e.g. under "Program Files"). + + [Fact(Skip = "Windows-only: StdioClientTransport only wraps commands with cmd.exe on Windows.", SkipUnless = nameof(IsWindows))] + public async Task WindowsCmdWrapper_UsesDsCWithSingleOuterQuotePair() + { + // The command and each argument are quoted only when they contain whitespace, a quote, or a cmd.exe + // metacharacter (& | < > ^ ( )); a bare command name is left unquoted so cmd.exe still resolves it. + static string Wrap(string inner) => "/d /s /c \"" + inner + "\""; + static string Quote(string value) => "\"" + value + "\""; + + const string spacedCommand = @"C:\Program Files\My Server\server.exe"; + + var cases = new (string command, string[]? arguments, string expectedArguments)[] + { + // Space in the command path with a trailing argument — the exact shape that failed in #1601. + (spacedCommand, ["--flag"], Wrap(Quote(spacedCommand) + " --flag")), + // Space in the command path with an argument that itself contains spaces. + (spacedCommand, ["arg with spaces"], Wrap(Quote(spacedCommand) + " " + Quote("arg with spaces"))), + // Space in the command path with no arguments. + (spacedCommand, null, Wrap(Quote(spacedCommand))), + // A space-free command path is left unquoted; cmd.exe metacharacters in arguments are quoted so + // cmd treats them literally rather than as operators. + (@"C:\tools\server.exe", ["a&b", "c|d", "e>f", "gf") + " " + Quote("g GetLoggedProcessInvocationAsync(string command, IList? arguments) + { + var provider = new MockLoggerProvider(); + using var loggerFactory = Microsoft.Extensions.Logging.LoggerFactory.Create(builder => + { + builder.AddProvider(provider); + builder.SetMinimumLevel(LogLevel.Trace); + }); + + var transport = new StdioClientTransport(new() { Name = "TestServer", Command = command, Arguments = arguments }, loggerFactory); + try + { + // The command path need not exist: the invocation is logged before the process is started, and + // whether cmd.exe then fails to find the target is irrelevant to the command line we are asserting. + await using var _ = await transport.ConnectAsync(TestContext.Current.CancellationToken); + } + catch + { + // Ignore: we only need the pre-start trace log. + } + + var log = Assert.Single(provider.LogMessages, m => + m.LogLevel == LogLevel.Trace && m.Message.Contains("starting server process", StringComparison.Ordinal)); + return log.Message; + } + + [Theory(Skip = "Windows-only: reproduces #1601 (a space in the command path on Windows).", SkipUnless = nameof(IsWindows))] + [InlineData("simple")] + [InlineData("value with spaces")] + [InlineData("a & b | c")] + [InlineData("&^<>|")] + [InlineData("value with \"quotes\" and spaces")] + [InlineData(@"C:\Program Files\Test App\app.dll")] + [InlineData(@"C:\EndsWithBackslash\")] + [InlineData("http://localhost:1234/callback?foo=1&bar=2")] + public async Task CommandPathWithSpaces_RoundTripsArgumentsThroughCmd(string cliArgumentValue) + { + // Copy the (self-contained) TestServer build output to a directory whose path contains a space, + // then launch it by absolute path. Before the fix this failed to start; the argument echoed back + // confirms both that the process launched and that the argument survived the cmd.exe wrapping. + string serverDirectory = CopyTestServerToDirectoryWithSpaces(); + try + { + var options = new StdioClientTransportOptions + { + Name = "TestServer", + Command = Path.Combine(serverDirectory, "TestServer.exe"), + Arguments = [$"--cli-arg={cliArgumentValue}"], + }; + + var transport = new StdioClientTransport(options, LoggerFactory); + await using var client = await McpClient.CreateAsync(transport, loggerFactory: LoggerFactory, cancellationToken: TestContext.Current.CancellationToken); + + var tools = await client.ListToolsAsync(cancellationToken: TestContext.Current.CancellationToken); + Assert.NotEmpty(tools); + + var result = await client.CallToolAsync("echoCliArg", cancellationToken: TestContext.Current.CancellationToken); + var content = Assert.IsType(Assert.Single(result.Content)); + Assert.Equal(cliArgumentValue, content.Text); + } + finally + { + try + { + Directory.Delete(serverDirectory, recursive: true); + } + catch + { + // Best effort cleanup. + } + } + } + + private static string CopyTestServerToDirectoryWithSpaces() + { + // The TestServer build output is a sibling of the test build output under the artifacts layout, + // differing only by the project-name segment of the path. + string sourceDirectory = AppContext.BaseDirectory.Replace( + $"{Path.DirectorySeparatorChar}ModelContextProtocol.Tests{Path.DirectorySeparatorChar}", + $"{Path.DirectorySeparatorChar}ModelContextProtocol.TestServer{Path.DirectorySeparatorChar}"); + + Assert.True( + File.Exists(Path.Combine(sourceDirectory, "TestServer.exe")), + $"Could not locate the TestServer build output at '{sourceDirectory}'."); + + string destinationDirectory = Path.Combine(Path.GetTempPath(), "mcp sdk 1601 " + Guid.NewGuid().ToString("N")); + CopyDirectory(sourceDirectory, destinationDirectory); + return destinationDirectory; + + static void CopyDirectory(string source, string destination) + { + Directory.CreateDirectory(destination); + foreach (string file in Directory.GetFiles(source)) + { + File.Copy(file, Path.Combine(destination, Path.GetFileName(file)), overwrite: true); + } + + foreach (string directory in Directory.GetDirectories(source)) + { + CopyDirectory(directory, Path.Combine(destination, Path.GetFileName(directory))); + } + } + } + [Fact(Skip = "Platform not supported by this test.", SkipUnless = nameof(IsStdErrCallbackSupported))] public async Task InheritEnvironmentVariables_DefaultTrue_ChildSeesParentEnvVars() { From 042693267dae15c99d7e27c1ee62446a18b977df Mon Sep 17 00:00:00 2001 From: yayayouyou Date: Tue, 14 Jul 2026 12:49:00 +0800 Subject: [PATCH 2/2] Address Copilot review feedback - Update the wrapping comment to reflect the hand-built `cmd.exe /d /s /c` command line instead of the old `cmd.exe /c {command}` description. - Use guaranteed-nonexistent, GUID-based command names in WindowsCmdWrapper_UsesDsCWithSingleOuterQuotePair so the wrapper's cmd.exe never launches a real program if a name like `server.exe` happens to resolve from PATH; the test only inspects the logged command line. Co-Authored-By: Claude Opus 4.8 --- .../Client/StdioClientTransport.cs | 5 +++-- .../Transport/StdioClientTransportTests.cs | 16 +++++++++++----- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/ModelContextProtocol.Core/Client/StdioClientTransport.cs b/src/ModelContextProtocol.Core/Client/StdioClientTransport.cs index 8d1af2763..6236426ef 100644 --- a/src/ModelContextProtocol.Core/Client/StdioClientTransport.cs +++ b/src/ModelContextProtocol.Core/Client/StdioClientTransport.cs @@ -64,8 +64,9 @@ public async Task ConnectAsync(CancellationToken cancellationToken = string command = _options.Command; IList? arguments = _options.Arguments; - // On Windows, for stdio, we need to wrap non-shell commands with cmd.exe /c {command} (usually npx or uvicorn). - // The stdio transport will not work correctly if the command is not run in a shell. + // On Windows, for stdio, we need to wrap non-cmd.exe commands with cmd.exe (usually npx or uvicorn): + // the stdio transport will not work correctly if the command is not run in a shell. The wrapped + // "cmd.exe /d /s /c ..." command line is built by hand (see GetWindowsCmdArguments below). string? cmdArguments = null; if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows) && !string.Equals(Path.GetFileName(command), "cmd.exe", StringComparison.OrdinalIgnoreCase)) diff --git a/tests/ModelContextProtocol.Tests/Transport/StdioClientTransportTests.cs b/tests/ModelContextProtocol.Tests/Transport/StdioClientTransportTests.cs index 3211d926e..a3a4791de 100644 --- a/tests/ModelContextProtocol.Tests/Transport/StdioClientTransportTests.cs +++ b/tests/ModelContextProtocol.Tests/Transport/StdioClientTransportTests.cs @@ -201,7 +201,13 @@ public async Task WindowsCmdWrapper_UsesDsCWithSingleOuterQuotePair() static string Wrap(string inner) => "/d /s /c \"" + inner + "\""; static string Quote(string value) => "\"" + value + "\""; - const string spacedCommand = @"C:\Program Files\My Server\server.exe"; + // Use guaranteed-nonexistent command names (under a unique, nonexistent directory, and a bare name + // that will not resolve from PATH) so the wrapper's cmd.exe never actually launches a real program; + // the test only inspects the logged command line, not the process outcome. + string id = Guid.NewGuid().ToString("N"); + string spacedCommand = $@"C:\{id} dir\My Server\server_{id}.exe"; // contains spaces + string plainCommand = $@"C:\{id}dir\server_{id}.exe"; // no space, has a directory + string bareCommand = $"nonexistent_{id}.exe"; // bare name, not on PATH var cases = new (string command, string[]? arguments, string expectedArguments)[] { @@ -213,12 +219,12 @@ public async Task WindowsCmdWrapper_UsesDsCWithSingleOuterQuotePair() (spacedCommand, null, Wrap(Quote(spacedCommand))), // A space-free command path is left unquoted; cmd.exe metacharacters in arguments are quoted so // cmd treats them literally rather than as operators. - (@"C:\tools\server.exe", ["a&b", "c|d", "e>f", "gf") + " " + Quote("gf", "gf") + " " + Quote("g