Never resolve a bare name to a batch file on the overlay PATH [patch]

README.md

@@ -235,7 +235,7 @@ EnvironmentVariables = new Dictionary<string, string?>
 
 Environment variables are the only control surface some tools expose, so this covers behaviour with no command-line equivalent — `GIT_TERMINAL_PROMPT=0` to make an authenticating `git fetch` fail rather than block forever on a prompt no terminal will answer, `GIT_ASKPASS`/`SSH_ASKPASS` to supply credentials without putting them on a command line where any process listing can read them, and `LC_ALL=C` to force stable, machine-parseable output rather than whatever the host locale produces.
 
-When the overlay sets `PATH`, a bare executable name such as `node` or `dotnet` is looked up on that `PATH`, so an overlay can pin which toolchain runs. The first directory holding the name wins. Relative `PATH` entries are skipped. On Windows, a name with no extension is tried with each `PATHEXT` extension. On other platforms the file must have an execute bit. A name with a directory part is a path rather than a name to search for, so it is never looked up. If the overlay `PATH` holds no match, or the overlay leaves `PATH` alone or removes it with `null`, the name is found the usual way, on the calling process's `PATH`.
+When the overlay sets `PATH`, a bare executable name such as `node` or `dotnet` is looked up on that `PATH`, so an overlay can pin which toolchain runs. The first directory holding the name wins. Relative `PATH` entries are skipped. On Windows, a name with no extension is tried with `.exe` only, the one extension `CreateProcess` adds, so a bare name such as `npm` never starts `npm.cmd`. A batch file runs through `cmd.exe`, which re-parses the command line and would run `&`, `|` or `%` in an argument as commands; name the batch file with its extension if you mean to start one, and do not pass it untrusted arguments. On other platforms the file must have an execute bit. A name with a directory part is a path rather than a name to search for, so it is never looked up. If the overlay `PATH` holds no match, or the overlay leaves `PATH` alone or removes it with `null`, the name is found the usual way, on the calling process's `PATH`.
 
 > **_NOTE:_** _`EnvironmentVariables` cannot be combined with `Elevation.Elevated` on Windows. Elevation requires `UseShellExecute`, which offers nowhere to pass an environment, so the call throws `ArgumentException` rather than silently dropping the variables._
 

RunCommand.Test/RunCommandTests.cs

@@ -960,16 +960,30 @@ public void FindOverlayPathShouldMatchTheKeyIgnoringCaseOnlyOnWindows()
 	}
 
 	[TestMethod]
-	public void ResolveOnSearchPathShouldApplyPathExtOnWindows()
+	public void ResolveOnSearchPathShouldAddExeToANameWithNoExtensionOnWindows()
 	{
 		string directory = CreateDirectoryForTest();
-		string name = $"tool-{nameof(ResolveOnSearchPathShouldApplyPathExtOnWindows)}";
-		WriteScript(directory, $"{name}.CMD", "exit 0", executable: true);
-		Dictionary<string, string?> environment = new() { ["PATHEXT"] = ".EXE;.CMD" };
+		string name = $"tool-{nameof(ResolveOnSearchPathShouldAddExeToANameWithNoExtensionOnWindows)}";
+		WriteScript(directory, $"{name}.exe", "exit 0", executable: true);
 
-		string? resolved = RunCommand.ResolveOnSearchPath(name, directory, environment, isWindows: true);
+		string? resolved = RunCommand.ResolveOnSearchPath(name, directory, isWindows: true);
 
-		Assert.AreEqual(Path.Join(directory, $"{name}.CMD"), resolved);
+		Assert.AreEqual(Path.Join(directory, $"{name}.exe"), resolved);
+	}
+
+	[TestMethod]
+	public void ResolveOnSearchPathShouldNeverResolveABareNameToABatchFileOnWindows()
+	{
+		// cmd.exe re-parses a batch file's command line, so a bare name that reached tool.cmd would let & or | in an
+		// argument run a second command (ktsu-dev/RunCommand#138). CreateProcess only ever adds .exe to such a name.
+		string directory = CreateDirectoryForTest();
+		string name = $"tool-{nameof(ResolveOnSearchPathShouldNeverResolveABareNameToABatchFileOnWindows)}";
+		WriteScript(directory, $"{name}.cmd", "exit 0", executable: true);
+		WriteScript(directory, $"{name}.bat", "exit 0", executable: true);
+		WriteScript(directory, $"{name}.com", "exit 0", executable: true);
+
+		Assert.IsNull(RunCommand.ResolveOnSearchPath(name, directory, isWindows: true));
+		Assert.AreEqual(Path.Join(directory, $"{name}.cmd"), RunCommand.ResolveOnSearchPath($"{name}.cmd", directory, isWindows: true), "A batch file named with its extension is the caller's explicit choice.");
 	}
 
 	[TestMethod]
@@ -978,9 +992,9 @@ public void ResolveOnSearchPathShouldLeaveANameWithADirectoryPartAlone()
 		string directory = CreateDirectoryForTest();
 		WriteScript(directory, "tool", "exit 0", executable: true);
 
-		Assert.IsNull(RunCommand.ResolveOnSearchPath("./tool", directory, new Dictionary<string, string?>(), isWindows: false));
-		Assert.IsNull(RunCommand.ResolveOnSearchPath(@"bin\tool", directory, new Dictionary<string, string?>(), isWindows: true));
-		Assert.IsNull(RunCommand.ResolveOnSearchPath("C:tool", directory, new Dictionary<string, string?>(), isWindows: true));
+		Assert.IsNull(RunCommand.ResolveOnSearchPath("./tool", directory, isWindows: false));
+		Assert.IsNull(RunCommand.ResolveOnSearchPath(@"bin\tool", directory, isWindows: true));
+		Assert.IsNull(RunCommand.ResolveOnSearchPath("C:tool", directory, isWindows: true));
 	}
 
 	[TestMethod]
@@ -991,8 +1005,8 @@ public void ResolveOnSearchPathShouldSkipRelativeAndMissingEntries()
 		WriteScript(directory, name, "exit 0", executable: true);
 		string searchPath = string.Join(Path.PathSeparator, "relative", Path.Join(directory, "missing"), "", directory);
 
-		Assert.AreEqual(Path.Join(directory, name), RunCommand.ResolveOnSearchPath(name, searchPath, new Dictionary<string, string?>(), isWindows: false));
-		Assert.IsNull(RunCommand.ResolveOnSearchPath(name, "relative", new Dictionary<string, string?>(), isWindows: false));
+		Assert.AreEqual(Path.Join(directory, name), RunCommand.ResolveOnSearchPath(name, searchPath, isWindows: false));
+		Assert.IsNull(RunCommand.ResolveOnSearchPath(name, "relative", isWindows: false));
 	}
 
 	private static void SkipOnWindows()

RunCommand/RunCommand.cs

@@ -24,7 +24,7 @@
 	/// </summary>
 	/// <param name="command">The command to execute.</param>
 	/// <returns>The exit code of the executed process.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static int Execute(string command) =>
 		ExecuteAsync(command).Result;
@@ -35,7 +35,7 @@
 	/// <param name="command">The command to execute.</param>
 	/// <param name="outputHandler">The handler for processing command output.</param>
 	/// <returns>The exit code of the executed process.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static int Execute(string command, OutputHandler outputHandler) =>
 		ExecuteAsync(command, outputHandler).Result;
@@ -46,7 +46,7 @@
 	/// <param name="command">The command to execute.</param>
 	/// <param name="elevation">The privilege level under which to run the command.</param>
 	/// <returns>The exit code of the executed process.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static int Execute(string command, Elevation elevation) =>
 		ExecuteAsync(command, elevation).Result;
@@ -62,7 +62,7 @@
 	/// </param>
 	/// <param name="elevation">The privilege level under which to run the command.</param>
 	/// <returns>The exit code of the executed process.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static int Execute(string command, OutputHandler outputHandler, Elevation elevation) =>
 		ExecuteAsync(command, outputHandler, elevation).Result;
@@ -109,7 +109,7 @@
 	/// </summary>
 	/// <param name="command">The command to execute.</param>
 	/// <returns>A task representing the asynchronous operation with the process exit code.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static async Task<int> ExecuteAsync(string command)
 		=> await ExecuteAsync(command, new OutputHandler()).ConfigureAwait(false);
@@ -120,7 +120,7 @@
 	/// <param name="command">The command to execute.</param>
 	/// <param name="outputHandler">The handler for processing command output.</param>
 	/// <returns>A task representing the asynchronous operation with the process exit code.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static async Task<int> ExecuteAsync(string command, OutputHandler outputHandler)
 		=> await ExecuteAsync(command, outputHandler, Elevation.Default).ConfigureAwait(false);
@@ -131,7 +131,7 @@
 	/// <param name="command">The command to execute.</param>
 	/// <param name="elevation">The privilege level under which to run the command.</param>
 	/// <returns>A task representing the asynchronous operation with the process exit code.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static async Task<int> ExecuteAsync(string command, Elevation elevation)
 		=> await ExecuteAsync(command, new(), elevation).ConfigureAwait(false);
@@ -147,7 +147,7 @@
 	/// </param>
 	/// <param name="elevation">The privilege level under which to run the command.</param>
 	/// <returns>A task representing the asynchronous operation with the process exit code.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static async Task<int> ExecuteAsync(string command, OutputHandler outputHandler, Elevation elevation)
 		=> await ExecuteAsync(command, outputHandler, elevation, CancellationToken.None).ConfigureAwait(false);
@@ -160,7 +160,7 @@
 	/// A token that, when cancelled, terminates the running process and its children.
 	/// </param>
 	/// <returns>A task representing the asynchronous operation with the process exit code.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static async Task<int> ExecuteAsync(string command, CancellationToken cancellationToken)
 		=> await ExecuteAsync(command, new OutputHandler(), cancellationToken).ConfigureAwait(false);
@@ -175,7 +175,7 @@
 	/// A token that, when cancelled, terminates the running process and its children.
 	/// </param>
 	/// <returns>A task representing the asynchronous operation with the process exit code.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static async Task<int> ExecuteAsync(string command, OutputHandler outputHandler, CancellationToken cancellationToken)
 		=> await ExecuteAsync(command, outputHandler, Elevation.Default, cancellationToken).ConfigureAwait(false);
@@ -413,7 +413,7 @@
 				string? overlayPath = FindOverlayPath(options.EnvironmentVariables, isWindows);
 				if (overlayPath is not null)
 				{
-					startInfo.FileName = ResolveOnSearchPath(startInfo.FileName, overlayPath, startInfo.Environment, isWindows) ?? startInfo.FileName;
+					startInfo.FileName = ResolveOnSearchPath(startInfo.FileName, overlayPath, isWindows) ?? startInfo.FileName;
 				}
 			}
 
@@ -456,41 +456,38 @@
 	/// </summary>
 	/// <param name="fileName">The executable as the caller gave it.</param>
 	/// <param name="searchPath">The overlay's <c>PATH</c>.</param>
-	/// <param name="environment">The command's environment, which supplies <c>PATHEXT</c> on Windows.</param>
-	/// <param name="isWindows">Whether to try the <c>PATHEXT</c> extensions and accept a backslash as a separator.</param>
+	/// <param name="isWindows">Whether to add <c>.exe</c> to a name with no extension and accept a backslash as a separator.</param>
 	/// <returns>The full path of the first match, or <see langword="null"/> to keep the usual lookup.</returns>
 	/// <remarks>
 	/// With <see cref="ProcessStartInfo.UseShellExecute"/> off, <c>Process.Start</c> searches the calling process's
 	/// <c>PATH</c>, not the one in <see cref="ProcessStartInfo.Environment"/>. Without this, an overlay that points
 	/// <c>PATH</c> at a pinned toolchain either failed to start a tool only it holds or started a same-named tool from
 	/// the caller's <c>PATH</c>. A name with a directory part is a path rather than a name to search for, so it is
 	/// left alone, and so are relative <c>PATH</c> entries, which would otherwise depend on the caller's directory.
+	/// <para>
+	/// On Windows a name with no extension is tried with <c>.exe</c> only, which is all <c>CreateProcess</c> appends,
+	/// rather than with every <c>PATHEXT</c> extension. A bare name therefore never resolves to a <c>.bat</c> or
+	/// <c>.cmd</c> file: <c>CreateProcess</c> runs those through <c>cmd.exe</c>, which re-parses the command line, so
+	/// <c>&amp;</c>, <c>|</c> or <c>%</c> in an argument would run as commands although the argument vector is escaped
+	/// for <c>CommandLineToArgvW</c>. A caller that wants a batch file names it with its extension.
+	/// </para>
 	/// </remarks>
-	internal static string? ResolveOnSearchPath(string fileName, string searchPath, IDictionary<string, string?> environment, bool isWindows)
+	internal static string? ResolveOnSearchPath(string fileName, string searchPath, bool isWindows)
 	{
 		bool hasDirectoryPart = fileName.Contains('/') || (isWindows && (fileName.Contains('\\') || fileName.Contains(':')));
 		if (fileName.Length == 0 || hasDirectoryPart)
 		{
 			return null;
 		}
 
-		string[] candidates = [fileName];
-		if (isWindows && !Path.HasExtension(fileName))
-		{
-			_ = environment.TryGetValue("PATHEXT", out string? pathExt);
-			string[] extensions = (pathExt ?? ".COM;.EXE;.BAT;.CMD").Split([';'], StringSplitOptions.RemoveEmptyEntries);
-			candidates = Array.ConvertAll(extensions, extension => fileName + extension.Trim());
-		}
+		string candidate = isWindows && !Path.HasExtension(fileName) ? fileName + ".exe" : fileName;
 
 		foreach (string directory in searchPath.Split([Path.PathSeparator], StringSplitOptions.RemoveEmptyEntries))
 		{
-			foreach (string candidate in candidates)
+			string? match = TryExecutable(directory.Trim().Trim('"'), candidate);
+			if (match is not null)
 			{
-				string? match = TryExecutable(directory.Trim().Trim('"'), candidate);
-				if (match is not null)
-				{
-					return match;
-				}
+				return match;
 			}
 		}