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
1 change: 0 additions & 1 deletion src/NuGet.Core/NuGet.Protocol/GlobalSuppressions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,6 @@
[assembly: SuppressMessage("Build", "CA1031:Modify 'FireBeforeClose' to catch a more specific allowed exception type, or rethrow the exception.", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Protocol.Plugins.Plugin.FireBeforeClose")]
[assembly: SuppressMessage("Build", "CA1031:Modify 'FireClosed' to catch a more specific allowed exception type, or rethrow the exception.", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Protocol.Plugins.Plugin.FireClosed")]
[assembly: SuppressMessage("Build", "CA2000:Call System.IDisposable.Dispose on object created by 'new MonitorNuGetProcessExitRequestHandler(plugin)' before all references to it are out of scope.", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Protocol.Plugins.PluginFactory.CreateFromCurrentProcessAsync(NuGet.Protocol.Plugins.IRequestHandlers,NuGet.Protocol.Plugins.ConnectionOptions,System.Threading.CancellationToken)~System.Threading.Tasks.Task{NuGet.Protocol.Plugins.IPlugin}")]
[assembly: SuppressMessage("Build", "CA2000:Use recommended dispose pattern to ensure that object created by 'new PluginProcess(startInfo)' is disposed on all paths. If possible, wrap the creation within a 'using' statement or a 'using' declaration. Otherwise, use a try-finally pattern, with a dedicated local variable declared before the try region and an unconditional Dispose invocation on non-null value in the 'finally' region, say 'x?.Dispose()'. If the object is explicitly disposed within the try region or the dispose ownership is transfered to another object or method, assign 'null' to the local variable just after such an operation to prevent double dispose in 'finally'.", Justification = "The responsibility to dispose the object is transferred to another object or wrapper that's created in the method and returned to the caller", Scope = "member", Target = "~M:NuGet.Protocol.Plugins.PluginFactory.CreatePluginAsync(System.String,System.Collections.Generic.IEnumerable{System.String},NuGet.Protocol.Plugins.IRequestHandlers,NuGet.Protocol.Plugins.ConnectionOptions,System.Threading.CancellationToken)~System.Threading.Tasks.Task{NuGet.Protocol.Plugins.IPlugin}")]
[assembly: SuppressMessage("Build", "CA1031:Modify 'SendCloseRequest' to catch a more specific allowed exception type, or rethrow the exception.", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Protocol.Plugins.PluginFactory.SendCloseRequest(NuGet.Protocol.Plugins.IPlugin)")]
[assembly: SuppressMessage("Build", "CA1822:Member GetPluginOperationClaimsAsync does not access instance data and can be marked as static (Shared in VisualBasic)", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Protocol.Plugins.PluginManager.GetPluginOperationClaimsAsync(NuGet.Protocol.Plugins.IPlugin,System.String,Newtonsoft.Json.Linq.JObject,System.Threading.CancellationToken)~System.Threading.Tasks.Task{System.Collections.Generic.IReadOnlyList{NuGet.Protocol.Plugins.OperationClaim}}")]
[assembly: SuppressMessage("Build", "CA1031:Modify 'TryCreatePluginAsync' to catch a more specific allowed exception type, or rethrow the exception.", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Protocol.Plugins.PluginManager.TryCreatePluginAsync(NuGet.Protocol.Plugins.PluginDiscoveryResult,NuGet.Protocol.Plugins.OperationClaim,NuGet.Protocol.Plugins.PluginManager.PluginRequestKey,System.String,Newtonsoft.Json.Linq.JObject,System.Threading.CancellationToken)~System.Threading.Tasks.Task{System.Tuple{System.Boolean,NuGet.Protocol.Plugins.PluginCreationResult}}")]
Expand Down
8 changes: 4 additions & 4 deletions src/NuGet.Core/NuGet.Protocol/Plugins/IPluginFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,20 @@ namespace NuGet.Protocol.Plugins
/// <summary>
/// A plugin factory.
/// </summary>
public interface IPluginFactory : IDisposable
internal interface IPluginFactory : IDisposable
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you explain this change?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We had to update the API for IPluginFactory so that it is able to create plugins for dotnet tools plugins. Since we needed to break the API, I used it as an opportunity to un-expose the interface. The reason for making it internal is that the interface only has one implementation and there is no need for it to be a public API #6113 (comment)

{
/// <summary>
/// Asynchronously gets an existing plugin instance or creates a new instance and connects to it.
/// </summary>
/// <param name="filePath">The file path of the plugin.</param>
/// <param name="pluginFile">A plugin file.</param>
/// <param name="arguments">Command-line arguments to be supplied to the plugin.</param>
/// <param name="requestHandlers">Request handlers.</param>
/// <param name="options">Connection options.</param>
/// <param name="sessionCancellationToken">A cancellation token for the plugin's lifetime.</param>
/// <returns>A task that represents the asynchronous operation.
/// The task result (<see cref="Task{TResult}.Result" />) returns a <see cref="Plugin" />
/// instance.</returns>
/// <exception cref="ArgumentException">Thrown if <paramref name="filePath" />
/// <exception cref="ArgumentException">Thrown if <paramref name="pluginFile.Path" />
/// is either <see langword="null" /> or empty.</exception>
/// <exception cref="ArgumentNullException">Thrown if <paramref name="arguments" />
/// is <see langword="null" />.</exception>
Expand All @@ -37,7 +37,7 @@ public interface IPluginFactory : IDisposable
/// <exception cref="ObjectDisposedException">Thrown if this object is disposed.</exception>
/// <remarks>This is intended to be called by NuGet client tools.</remarks>
Task<IPlugin> GetOrCreateAsync(
string filePath,
PluginFile pluginFile,
IEnumerable<string> arguments,
IRequestHandlers requestHandlers,
ConnectionOptions options,
Expand Down
256 changes: 236 additions & 20 deletions src/NuGet.Core/NuGet.Protocol/Plugins/PluginDiscoverer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Collections.Generic;
using System.IO;
using System.Runtime.InteropServices;
using System.Threading;
using System.Threading.Tasks;
using NuGet.Common;
Expand All @@ -17,17 +18,42 @@ public sealed class PluginDiscoverer : IPluginDiscoverer
{
private bool _isDisposed;
private List<PluginFile> _pluginFiles;
private readonly string _rawPluginPaths;
private readonly string _netCoreOrNetFXPluginPaths;
private readonly string _nuGetPluginPaths;
private IEnumerable<PluginDiscoveryResult> _results;
private readonly SemaphoreSlim _semaphore;
private readonly IEnvironmentVariableReader _environmentVariableReader;
private static bool IsDesktop
{
get
{
#if IS_DESKTOP
return true;
#else
return false;
#endif
}
}

/// <summary>
/// Instantiates a new <see cref="PluginDiscoverer" /> class.
/// </summary>
/// <param name="rawPluginPaths">The raw semicolon-delimited list of supposed plugin file paths.</param>
public PluginDiscoverer(string rawPluginPaths)
public PluginDiscoverer()
: this(EnvironmentVariableWrapper.Instance)
{
}

internal PluginDiscoverer(IEnvironmentVariableReader environmentVariableReader)
{
_rawPluginPaths = rawPluginPaths;
_environmentVariableReader = environmentVariableReader;
#if IS_DESKTOP
_netCoreOrNetFXPluginPaths = environmentVariableReader.GetEnvironmentVariable(EnvironmentVariableConstants.DesktopPluginPaths);
#else
_netCoreOrNetFXPluginPaths = environmentVariableReader.GetEnvironmentVariable(EnvironmentVariableConstants.CorePluginPaths);
#endif

if (string.IsNullOrEmpty(_netCoreOrNetFXPluginPaths))
{
_nuGetPluginPaths = _environmentVariableReader.GetEnvironmentVariable(EnvironmentVariableConstants.PluginPaths);
}

_semaphore = new SemaphoreSlim(initialCount: 1, maxCount: 1);
}

Expand Down Expand Up @@ -75,7 +101,40 @@ public async Task<IEnumerable<PluginDiscoveryResult>> DiscoverAsync(Cancellation
return _results;
}

_pluginFiles = GetPluginFiles(cancellationToken);
if (!string.IsNullOrEmpty(_netCoreOrNetFXPluginPaths))
{
// NUGET_NETFX_PLUGIN_PATHS, NUGET_NETCORE_PLUGIN_PATHS have been set.
var filePaths = _netCoreOrNetFXPluginPaths.Split(new[] { Path.PathSeparator }, StringSplitOptions.RemoveEmptyEntries);
_pluginFiles = GetPluginFiles(filePaths, cancellationToken);
}
else if (!string.IsNullOrEmpty(_nuGetPluginPaths))
{
// NUGET_PLUGIN_PATHS has been set
_pluginFiles = GetPluginsInNuGetPluginPaths();
}
else
{
// restore to default plugins search.
// Search for plugins in %user%/.nuget/plugins
var directories = new List<string> { PluginDiscoveryUtility.GetNuGetHomePluginsPath() };
#if IS_DESKTOP
// Internal plugins are only supported for .NET Framework scenarios, namely msbuild.exe
directories.Add(PluginDiscoveryUtility.GetInternalPlugins());
#endif
var filePaths = PluginDiscoveryUtility.GetConventionBasedPlugins(directories);
_pluginFiles = GetPluginFiles(filePaths, cancellationToken);

// Search for .Net tools plugins in PATH
if (_pluginFiles != null)
{
_pluginFiles.AddRange(GetPluginsInPath());
}
else
{
_pluginFiles = GetPluginsInPath();
}
}

var results = new List<PluginDiscoveryResult>();

for (var i = 0; i < _pluginFiles.Count; ++i)
Expand All @@ -97,14 +156,17 @@ public async Task<IEnumerable<PluginDiscoveryResult>> DiscoverAsync(Cancellation
return _results;
}

private List<PluginFile> GetPluginFiles(CancellationToken cancellationToken)
private static List<PluginFile> GetPluginFiles(IEnumerable<string> filePaths, CancellationToken cancellationToken)
{
cancellationToken.ThrowIfCancellationRequested();

var filePaths = GetPluginFilePaths();

var files = new List<PluginFile>();

if (filePaths == null)
{
return files;
}

foreach (var filePath in filePaths)
{
var pluginFile = new PluginFile(filePath, new Lazy<PluginFileState>(() =>
Expand All @@ -117,26 +179,180 @@ private List<PluginFile> GetPluginFiles(CancellationToken cancellationToken)
{
return PluginFileState.InvalidFilePath;
}
}));
}), requiresDotnetHost: !IsDesktop);
files.Add(pluginFile);
}

return files;
}

private IEnumerable<string> GetPluginFilePaths()
/// <summary>
/// Retrieves authentication plugins by searching through directories and files specified in the `NuGET_PLUGIN_PATHS`
/// environment variable. The method looks for files prefixed with 'nuget-plugin-' and verifies their validity for .net tools plugins.
/// </summary>
/// <returns>A list of valid <see cref="PluginFile"/> objects representing the discovered plugins.</returns>
internal List<PluginFile> GetPluginsInNuGetPluginPaths()
{
if (string.IsNullOrEmpty(_rawPluginPaths))
var pluginFiles = new List<PluginFile>();
string[] paths = _nuGetPluginPaths?.Split(Path.PathSeparator) ?? Array.Empty<string>();

foreach (var path in paths)
{
var directories = new List<string> { PluginDiscoveryUtility.GetNuGetHomePluginsPath() };
#if IS_DESKTOP
// Internal plugins are only supported for .NET Framework scenarios, namely msbuild.exe
directories.Add(PluginDiscoveryUtility.GetInternalPlugins());
if (PathValidator.IsValidLocalPath(path) || PathValidator.IsValidUncPath(path))
{
if (File.Exists(path))
{
FileInfo fileInfo = new FileInfo(path);
if (fileInfo.Name.StartsWith("nuget-plugin-", StringComparison.CurrentCultureIgnoreCase))
{
// A DotNet tool plugin
if (IsValidPluginFile(fileInfo))
{
PluginFile pluginFile = new PluginFile(fileInfo.FullName, new Lazy<PluginFileState>(() => PluginFileState.Valid), requiresDotnetHost: false);
pluginFiles.Add(pluginFile);
}
}
else
{
// A non DotNet tool plugin file
var state = new Lazy<PluginFileState>(() => PluginFileState.Valid);
pluginFiles.Add(new PluginFile(fileInfo.FullName, state, requiresDotnetHost: !IsDesktop));
}
}
else if (Directory.Exists(path))
{
pluginFiles.AddRange(GetNetToolsPluginsInDirectory(path) ?? new List<PluginFile>());
}
}
else
{
pluginFiles.Add(new PluginFile(path, new Lazy<PluginFileState>(() => PluginFileState.InvalidFilePath), requiresDotnetHost: !IsDesktop));
}
}

return pluginFiles;
}

/// <summary>
/// Retrieves .NET tools authentication plugins by searching through directories specified in `PATH`
/// </summary>
/// <returns>A list of valid <see cref="PluginFile"/> objects representing the discovered plugins.</returns>
internal List<PluginFile> GetPluginsInPath()
{
var pluginFiles = new List<PluginFile>();
var nugetPluginPaths = _environmentVariableReader.GetEnvironmentVariable("PATH");
string[] paths = nugetPluginPaths?.Split(Path.PathSeparator) ?? Array.Empty<string>();

foreach (var path in paths)
{
if (PathValidator.IsValidLocalPath(path) || PathValidator.IsValidUncPath(path))
{
pluginFiles.AddRange(GetNetToolsPluginsInDirectory(path) ?? new List<PluginFile>());
}
else
{
pluginFiles.Add(new PluginFile(path, new Lazy<PluginFileState>(() => PluginFileState.InvalidFilePath), requiresDotnetHost: false));
}
}

return pluginFiles;
}

private static List<PluginFile> GetNetToolsPluginsInDirectory(string directoryPath)
{
var pluginFiles = new List<PluginFile>();

if (Directory.Exists(directoryPath))
{
var directoryInfo = new DirectoryInfo(directoryPath);
var files = directoryInfo.GetFiles("nuget-plugin-*");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe the case sensitivity of this match will be platform-dependent, meaning on Windows it will be commonly (not guaranteed) case insensitive, but on Linux it will be case sensitive.

However, the file name comparison here is always case-insensitive.

Can you explain your reasoning?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You are right, that's inconsistent. I will address this in a follow up PR.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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


foreach (var file in files)
{
if (IsValidPluginFile(file))
{
PluginFile pluginFile = new PluginFile(file.FullName, new Lazy<PluginFileState>(() => PluginFileState.Valid), requiresDotnetHost: false);
pluginFiles.Add(pluginFile);
}
}
}

return pluginFiles;
}

/// <summary>
/// Checks whether a file is a valid plugin file for windows/Unix.
/// Windows: It should be either .bat or .exe
/// Unix: It should be executable
/// </summary>
/// <param name="fileInfo"></param>
/// <returns></returns>
internal static bool IsValidPluginFile(FileInfo fileInfo)
{
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
return fileInfo.Extension.Equals(".exe", StringComparison.OrdinalIgnoreCase) ||
fileInfo.Extension.Equals(".bat", StringComparison.OrdinalIgnoreCase);
}
else
{
#if NET8_0_OR_GREATER
var fileMode = File.GetUnixFileMode(fileInfo.FullName);

return fileInfo.Exists &&
((fileMode & UnixFileMode.UserExecute) != 0 ||
(fileMode & UnixFileMode.GroupExecute) != 0 ||
(fileMode & UnixFileMode.OtherExecute) != 0);
#else
return fileInfo.Exists && IsExecutable(fileInfo);
#endif
return PluginDiscoveryUtility.GetConventionBasedPlugins(directories);
}
}

return _rawPluginPaths.Split(new[] { ';' }, StringSplitOptions.RemoveEmptyEntries);
#if !NET8_0_OR_GREATER
/// <summary>
/// Checks whether a file is executable or not in Unix.
/// This is done by running bash code: `if [ -x {fileInfo.FullName} ]; then echo yes; else echo no; fi`
/// </summary>
/// <param name="fileInfo"></param>
/// <returns></returns>
internal static bool IsExecutable(FileInfo fileInfo)
{
#pragma warning disable CA1031 // Do not catch general exception types
try
{
string output;
using (var process = new System.Diagnostics.Process())
{
// Use a shell command to check if the file is executable
process.StartInfo.FileName = "/bin/bash";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This code could run on Alpine Linux, where /bin/bash does not exist by default.

Is there an even more portable way of checking? I don't know, but maybe ls -l <path>?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

On unix systems, FileInfo (and DirectoryInfo) has a UnixFileMode property that is a flags enum with the typical unix properties. You can use HasFlag or your favorite enum comparisons to see if a given file has any executable flags:

$ dotnet fsi

Microsoft (R) F# Interactive version 12.8.102.0 for F# 8.0
Copyright (c) Microsoft Corporation. All Rights Reserved.

For help type #help;;

> open System.IO;;
> let f = new FileInfo("/usr/bin/curl");;
val f: FileInfo = /usr/bin/curl

> f.UnixFileMode;;
val it: UnixFileMode =
  OtherExecute, OtherRead, GroupExecute, GroupRead, UserExecute, UserWrite, UserRead

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This code is in a #if !NET8_0_OR_GREATER code block because the APIs that @baronfel mentioned are not available before .NET 8. When @Nigusu-Allehu created the original PR to add this code to the feature branch (this PR is merging the feature branch into dev) I asked him to use the .NET 8 APIs already, which is being done on line 303.

I also suggested to @Nigusu-Allehu that we could consider skipping this PATH scanning for plugins for .NET Framework on Linux and Mac, since that only means Mono, which we don't officially support, but I don't remember any replies to that comment, and clearly it wasn't actioned either.

But I think it's also ok to keep here, because I really can't imagine anyone using Alpine wanting to install Mono and use NuGet.exe. And even if they do, the whole thing is wrapped in a try-catch block. The plugin won't be discovered, but it won't break restore either (apart from not being able to authenticate if a nuget-plugin-* app is installed)

process.StartInfo.Arguments = $" -c \"if [ -x '{fileInfo.FullName}' ]; then echo yes; else echo no; fi\"";
process.StartInfo.UseShellExecute = false;
process.StartInfo.RedirectStandardOutput = true;

process.Start();
output = process.StandardOutput.ReadToEnd().Trim();

if (!process.HasExited && !process.WaitForExit(1000))
{
process.Kill();
return false;
}
else if (process.ExitCode != 0)
{
return false;
}

// Check if the output is "yes"
return output.Equals("yes", StringComparison.OrdinalIgnoreCase);
}
}
catch
{
return false;
}
#pragma warning restore CA1031 // Do not catch general exception types
}
#endif
}
}
Loading