Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,8 @@ 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`.

> **_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._

### Standard Input
Expand Down
137 changes: 137 additions & 0 deletions RunCommand.Test/RunCommandTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
private static readonly string[] ExpectedALast = ["a", "last"];
private static readonly string[] ExpectedALastNext = ["a", "last", "next"];
private static readonly string[] ExpectedDone = ["done"];
private static readonly string[] ExpectedOverlayCopy = ["ran overlay copy"];
private static readonly string[] ExpectedStarted = ["started"];

private static string GetCopyCommand(string source, string destination) =>
RuntimeInformation.IsOSPlatform(OSPlatform.Windows)
Expand Down Expand Up @@ -839,6 +841,141 @@
}
}

[TestMethod]
public async Task ExecuteAsyncShouldStartAToolThatOnlyTheOverlayPathHolds()
{
SkipOnWindows();
string directory = CreateDirectoryForTest();
string name = $"runcommand-{nameof(ExecuteAsyncShouldStartAToolThatOnlyTheOverlayPathHolds)}";
WriteScript(directory, name, "echo ran overlay copy", executable: true);

List<string> output = await RunOnOverlayPathAsync(name, directory).ConfigureAwait(false);

Assert.AreSequenceEqual(ExpectedOverlayCopy, output, "Expected the tool on the overlay PATH to start.");
}

[TestMethod]
public async Task ExecuteAsyncShouldPreferTheOverlayPathOverTheCallersPath()
{
SkipOnWindows();

// echo is on the caller's PATH too, so the old lookup would have started that one and printed the argument.
string directory = CreateDirectoryForTest();
WriteScript(directory, "echo", "printf 'ran overlay copy\\n'", executable: true);

List<string> output = await RunOnOverlayPathAsync("echo", directory, "ran caller copy").ConfigureAwait(false);

Assert.AreSequenceEqual(ExpectedOverlayCopy, output, "Expected the overlay PATH to choose which echo runs.");
}

[TestMethod]
public async Task ExecuteAsyncShouldSkipANonExecutableFileOnTheOverlayPath()
{
SkipOnWindows();
string root = CreateDirectoryForTest();
string notExecutable = Directory.CreateDirectory(Path.Join(root, "a")).FullName;
string executable = Directory.CreateDirectory(Path.Join(root, "b")).FullName;
string name = $"runcommand-{nameof(ExecuteAsyncShouldSkipANonExecutableFileOnTheOverlayPath)}";
WriteScript(notExecutable, name, "echo ran the file that cannot run", executable: false);
WriteScript(executable, name, "echo ran overlay copy", executable: true);

List<string> output = await RunOnOverlayPathAsync(name, $"{notExecutable}{Path.PathSeparator}{executable}").ConfigureAwait(false);

Assert.AreSequenceEqual(ExpectedOverlayCopy, output, "Expected the search to pass over a file without an execute bit, as a shell does.");
}

[TestMethod]
public async Task ExecuteAsyncShouldFindTheToolOnTheCallersPathWhenTheOverlayRemovesPath()
{
SkipOnWindows();
List<string> output = [];

int exitCode = await RunCommand.ExecuteAsync(
"sh",
["-c", "echo started"],
new LineOutputHandler(onStandardOutput: output.Add),
new CommandOptions { EnvironmentVariables = new Dictionary<string, string?> { ["PATH"] = null } }).ConfigureAwait(false);

Check warning on line 897 in RunCommand.Test/RunCommandTests.cs

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Consider using the overload that accepts a CancellationToken and pass 'TestContext.CancellationToken'

See more on https://sonarcloud.io/project/issues?id=ktsu-dev_RunCommand&issues=AaGfj6QuOBUNh9bejKof&open=AaGfj6QuOBUNh9bejKof&pullRequest=137

Assert.AreEqual(0, exitCode);
Assert.AreSequenceEqual(ExpectedStarted, output, "Expected a removed PATH to leave the usual lookup in place.");
}

[TestMethod]
public void FindOverlayPathShouldMatchTheKeyIgnoringCaseOnlyOnWindows()
{
Dictionary<string, string?> overlay = new(StringComparer.Ordinal) { ["Path"] = "/pinned" };

Assert.AreEqual("/pinned", RunCommand.FindOverlayPath(overlay, isWindows: true));
Assert.IsNull(RunCommand.FindOverlayPath(overlay, isWindows: false));
}

[TestMethod]
public void ResolveOnSearchPathShouldApplyPathExtOnWindows()
{
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? resolved = RunCommand.ResolveOnSearchPath(name, directory, environment, isWindows: true);

Assert.AreEqual(Path.Join(directory, $"{name}.CMD"), resolved);
}

[TestMethod]
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));
}

[TestMethod]
public void ResolveOnSearchPathShouldSkipRelativeAndMissingEntries()
{
string directory = CreateDirectoryForTest();
string name = $"tool-{nameof(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));
}

private static void SkipOnWindows()
{
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
Assert.Inconclusive("Uses shell scripts and execute permission bits. The lookup this covers is shared, and the Unix legs cover it.");
}
}

private static void WriteScript(string directory, string name, string body, bool executable)
{
string path = Path.Join(directory, name);
File.WriteAllText(path, $"#!/bin/sh\n{body}\n");
if (!OperatingSystem.IsWindows())
{
UnixFileMode mode = UnixFileMode.UserRead | UnixFileMode.UserWrite;
File.SetUnixFileMode(path, executable ? mode | UnixFileMode.UserExecute : mode);
}
}

private static async Task<List<string>> RunOnOverlayPathAsync(string fileName, string overlayDirectories, params string[] arguments)
{
List<string> output = [];
int exitCode = await RunCommand.ExecuteAsync(
fileName,
arguments,
new LineOutputHandler(onStandardOutput: output.Add),
new CommandOptions { EnvironmentVariables = new Dictionary<string, string?> { ["PATH"] = $"{overlayDirectories}{Path.PathSeparator}/usr/bin{Path.PathSeparator}/bin" } }).ConfigureAwait(false);
Assert.AreEqual(0, exitCode);
return output;
}

[TestMethod]
public async Task ExecuteAsyncShouldRejectEnvironmentVariablesCombinedWithElevation()
{
Expand Down
122 changes: 122 additions & 0 deletions RunCommand/RunCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.Runtime.InteropServices;
using System.Threading;
#if NETSTANDARD2_0
Expand All @@ -23,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 "

Check warning on line 27 in RunCommand/RunCommand.cs

View workflow job for this annotation

GitHub Actions / ci / .NET / Analyze & Release

Do not forget to remove this deprecated code someday.
+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
public static int Execute(string command) =>
ExecuteAsync(command).Result;
Expand All @@ -34,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 "

Check warning on line 38 in RunCommand/RunCommand.cs

View workflow job for this annotation

GitHub Actions / ci / .NET / Analyze & Release

Do not forget to remove this deprecated code someday.
+ "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;
Expand All @@ -45,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 "

Check warning on line 49 in RunCommand/RunCommand.cs

View workflow job for this annotation

GitHub Actions / ci / .NET / Analyze & Release

Do not forget to remove this deprecated code someday.
+ "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;
Expand Down Expand Up @@ -108,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 "

Check warning on line 112 in RunCommand/RunCommand.cs

View workflow job for this annotation

GitHub Actions / ci / .NET / Analyze & Release

Do not forget to remove this deprecated code someday.
+ "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);
Expand All @@ -119,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 "

Check warning on line 123 in RunCommand/RunCommand.cs

View workflow job for this annotation

GitHub Actions / ci / .NET / Analyze & Release

Do not forget to remove this deprecated code someday.
+ "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);
Expand All @@ -130,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 "

Check warning on line 134 in RunCommand/RunCommand.cs

View workflow job for this annotation

GitHub Actions / ci / .NET / Analyze & Release

Do not forget to remove this deprecated code someday.
+ "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);
Expand All @@ -146,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 "

Check warning on line 150 in RunCommand/RunCommand.cs

View workflow job for this annotation

GitHub Actions / ci / .NET / Analyze & Release

Do not forget to remove this deprecated code someday.
+ "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);
Expand All @@ -159,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 "

Check warning on line 163 in RunCommand/RunCommand.cs

View workflow job for this annotation

GitHub Actions / ci / .NET / Analyze & Release

Do not forget to remove this deprecated code someday.
+ "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);
Expand All @@ -174,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 "

Check warning on line 178 in RunCommand/RunCommand.cs

View workflow job for this annotation

GitHub Actions / ci / .NET / Analyze & Release

Do not forget to remove this deprecated code someday.
+ "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);
Expand All @@ -195,7 +196,7 @@
/// </param>
/// <returns>A task representing the asynchronous operation with the process exit code.</returns>
/// <exception cref="OperationCanceledException">The token was cancelled before the process exited.</exception>
[Obsolete("A command string is split on its first space, which cannot handle an executable path "

Check warning on line 199 in RunCommand/RunCommand.cs

View workflow job for this annotation

GitHub Actions / ci / .NET / Analyze & Release

Do not forget to remove this deprecated code someday.
+ "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, CancellationToken cancellationToken)
{
Expand Down Expand Up @@ -381,6 +382,12 @@
startInfo.Environment[variable.Key] = variable.Value;
}
}

string? overlayPath = FindOverlayPath(options.EnvironmentVariables, isWindows);
if (overlayPath is not null)
{
startInfo.FileName = ResolveOnSearchPath(startInfo.FileName, overlayPath, startInfo.Environment, isWindows) ?? startInfo.FileName;
}
}

if (isWindows)
Expand All @@ -392,6 +399,121 @@
return startInfo;
}

/// <summary>
/// Returns the <c>PATH</c> value an environment overlay sets, if it sets one.
/// </summary>
/// <param name="environmentVariables">The overlay the caller passed.</param>
/// <param name="isWindows">Whether variable names compare case-insensitively.</param>
/// <returns>
/// The overlay's <c>PATH</c>, or <see langword="null"/> when the overlay leaves <c>PATH</c> alone or removes it.
/// A removed <c>PATH</c> gives no directories to search, so the executable is found the way it is without an
/// overlay.
/// </returns>
internal static string? FindOverlayPath(IReadOnlyDictionary<string, string?> environmentVariables, bool isWindows)
{
StringComparison comparison = isWindows ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal;
string? path = null;
foreach (KeyValuePair<string, string?> variable in environmentVariables)

Check warning on line 416 in RunCommand/RunCommand.cs

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Loops should be simplified using the "Where" LINQ method

See more on https://sonarcloud.io/project/issues?id=ktsu-dev_RunCommand&issues=AaGfgt5u_1BHaDNpSSji&open=AaGfgt5u_1BHaDNpSSji&pullRequest=137
{
if (string.Equals(variable.Key, "PATH", comparison))
{
path = variable.Value;
}
}
Comment on lines +416 to +422

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This stays as a loop. The suggestion only changes style, and the loop already gives the "last match wins" result the alert describes. That result matters on Windows, where the overlay dictionary can hold both Path and PATH. System.Linq is currently imported only for netstandard2.0, so switching to LINQ would mean adding a using for every target, with no change in behaviour.


Generated by Claude Code


return path;
}

/// <summary>
/// Looks a bare executable name up on the <c>PATH</c> an environment overlay gives the command.
/// </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>
/// <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.
/// </remarks>
internal static string? ResolveOnSearchPath(string fileName, string searchPath, IDictionary<string, string?> environment, 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());
}

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)
{
return match;
}
}
}

return null;
}

/// <summary>
/// Returns the full path of <paramref name="fileName"/> in <paramref name="directory"/> if it is a file the
/// command could start.
/// </summary>
/// <param name="directory">A directory from the search path.</param>
/// <param name="fileName">The file name to look for.</param>
/// <returns>
/// The full path, or <see langword="null"/> if there is no such file. Outside Windows the file also needs an
/// execute permission bit, where the platform exposes one.
/// </returns>
private static string? TryExecutable(string directory, string fileName)
{
try
{
// A rooted name would make Path.Combine drop the directory. The caller only passes bare names, so this
// is a guard rather than a case that arises.
if (!Path.IsPathRooted(directory) || Path.IsPathRooted(fileName))
{
return null;
}

string candidate = Path.Combine(directory, fileName);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I traced this, and it can't happen through the current caller. ResolveOnSearchPath returns early for any name that contains /, or on Windows \ or :, and a PATHEXT entry only appends an extension. So fileName never reaches TryExecutable rooted. The guard costs one condition, so 030eae5 adds it anyway: a rooted name is now skipped instead of being combined.


Generated by Claude Code

if (!File.Exists(candidate))
{
return null;
}

#if NET7_0_OR_GREATER
const UnixFileMode anyExecute = UnixFileMode.UserExecute | UnixFileMode.GroupExecute | UnixFileMode.OtherExecute;
if (!OperatingSystem.IsWindows() && (File.GetUnixFileMode(candidate) & anyExecute) == 0)
{
return null;
}
#endif

return candidate;
}
catch (Exception exception) when (exception is ArgumentException or IOException or UnauthorizedAccessException or NotSupportedException)
{
// An entry that is not a usable path is skipped, the way a shell skips it.
return null;
}
}

#if NETSTANDARD2_0
// ProcessStartInfo.ArgumentList is not part of the netstandard2.0 surface, so build an escaped
// command line by hand on that target only.
Expand Down
Loading