diff --git a/src/ModelContextProtocol.Core/Client/StdioClientTransport.cs b/src/ModelContextProtocol.Core/Client/StdioClientTransport.cs index 2e44ee34f..6236426ef 100644 --- a/src/ModelContextProtocol.Core/Client/StdioClientTransport.cs +++ b/src/ModelContextProtocol.Core/Client/StdioClientTransport.cs @@ -63,13 +63,24 @@ public async Task ConnectAsync(CancellationToken cancellationToken = string command = _options.Command; IList? arguments = _options.Arguments; + + // 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)) { - // 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 +104,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 +310,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..a3a4791de 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,157 @@ 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 + "\""; + + // 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)[] + { + // 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. + (plainCommand, ["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() {