diff --git a/src/TaskAnalyzer.Tests/MultiThreadableTaskAnalyzerTests.cs b/src/TaskAnalyzer.Tests/MultiThreadableTaskAnalyzerTests.cs index cc204fdcddb..4220213708d 100644 --- a/src/TaskAnalyzer.Tests/MultiThreadableTaskAnalyzerTests.cs +++ b/src/TaskAnalyzer.Tests/MultiThreadableTaskAnalyzerTests.cs @@ -1179,6 +1179,223 @@ public override bool Execute() diags.ShouldContain(d => d.Id == DiagnosticIds.FilePathRequiresAbsolute); } + // ═══════════════════════════════════════════════════════════════════════ + // MSBuildTask0003: Path consumers on types that are not obviously file APIs. + // These read the file system through a path string on an unrelated type, so + // they are just as unsafe as File.*/Directory.* under multithreaded execution. + // ═══════════════════════════════════════════════════════════════════════ + + [Fact] + public async Task AssemblyNameGetAssemblyName_WithStringPath_ProducesDiagnostic() + { + var diags = await GetDiagnosticsAsync(""" + using System.Reflection; + public class MyTask : Microsoft.Build.Utilities.Task + { + public string AssemblyPath { get; set; } = ""; + public override bool Execute() + { + var name = AssemblyName.GetAssemblyName(AssemblyPath); + return true; + } + } + """); + + diags.ShouldContain(d => d.Id == DiagnosticIds.FilePathRequiresAbsolute); + } + + [Fact] + public async Task AssemblyNameGetAssemblyName_WithGetAbsolutePath_NoDiagnostic() + { + var diags = await GetDiagnosticsAsync(""" + using System.Reflection; + using Microsoft.Build.Framework; + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } + public string AssemblyPath { get; set; } = ""; + public override bool Execute() + { + var name = AssemblyName.GetAssemblyName(TaskEnvironment.GetAbsolutePath(AssemblyPath)); + return true; + } + } + """); + + diags.ShouldNotContain(d => d.Id == DiagnosticIds.FilePathRequiresAbsolute); + } + + [Fact] + public async Task NewAssemblyName_WithDisplayName_NoDiagnostic() + { + // AssemblyName(string assemblyName) takes a display name, not a path. + var diags = await GetDiagnosticsAsync(""" + using System.Reflection; + public class MyTask : Microsoft.Build.Utilities.Task + { + public override bool Execute() + { + var name = new AssemblyName("MyAssembly, Version=1.0.0.0"); + return true; + } + } + """); + + diags.ShouldNotContain(d => d.Id == DiagnosticIds.FilePathRequiresAbsolute); + } + + [Fact] + public async Task XmlDocumentLoad_WithStringPath_ProducesDiagnostic() + { + var diags = await GetDiagnosticsAsync(""" + using System.Xml; + public class MyTask : Microsoft.Build.Utilities.Task + { + public override bool Execute() + { + var doc = new XmlDocument(); + doc.Load("input.xml"); + doc.Save("output.xml"); + return true; + } + } + """); + + diags.Count(d => d.Id == DiagnosticIds.FilePathRequiresAbsolute).ShouldBe(2); + } + + [Fact] + public async Task XmlDocumentLoad_WithStreamOverload_NoDiagnostic() + { + // Stream overloads carry no path string, so they must not be flagged. + var diags = await GetDiagnosticsAsync(""" + using System.IO; + using System.Xml; + public class MyTask : Microsoft.Build.Utilities.Task + { + public override bool Execute() + { + var doc = new XmlDocument(); + Stream stream = Stream.Null; + doc.Load(stream); + doc.Save(stream); + return true; + } + } + """); + + diags.ShouldNotContain(d => d.Id == DiagnosticIds.FilePathRequiresAbsolute); + } + + [Fact] + public async Task XmlDocumentNonPathMembers_NoDiagnostic() + { + // CreateElement/SelectNodes/LoadXml take names, XPath expressions and XML text — + // none of them are paths, so adding XmlDocument must not flag them. + var diags = await GetDiagnosticsAsync(""" + using System.Xml; + public class MyTask : Microsoft.Build.Utilities.Task + { + public override bool Execute() + { + var doc = new XmlDocument(); + doc.LoadXml(""); + var element = doc.CreateElement("child", "http://example.com/ns"); + doc.DocumentElement!.AppendChild(element); + doc.SelectNodes("//child"); + return true; + } + } + """); + + diags.ShouldNotContain(d => d.Id == DiagnosticIds.FilePathRequiresAbsolute); + } + + [Fact] + public async Task X509CertificateLoaderLoadFromFile_WithStringPath_ProducesDiagnostic() + { + var diags = await GetDiagnosticsAsync(""" + using System.Security.Cryptography.X509Certificates; + public class MyTask : Microsoft.Build.Utilities.Task + { + public string CertificatePath { get; set; } = ""; + public override bool Execute() + { + var certificate = X509CertificateLoader.LoadCertificateFromFile(CertificatePath); + return true; + } + } + """); + + diags.ShouldContain(d => d.Id == DiagnosticIds.FilePathRequiresAbsolute); + } + + [Fact] + public async Task X509CertificateLoaderLoadFromBytes_NoDiagnostic() + { + // The byte[] overload has no path parameter; 'password' must not be treated as one. + var diags = await GetDiagnosticsAsync(""" + using System.Security.Cryptography.X509Certificates; + public class MyTask : Microsoft.Build.Utilities.Task + { + public override bool Execute() + { + var certificate = X509CertificateLoader.LoadPkcs12(new byte[0], "secret"); + return true; + } + } + """); + + diags.ShouldNotContain(d => d.Id == DiagnosticIds.FilePathRequiresAbsolute); + } + + [Fact] + public async Task ZipFileExtensionsExtractToFile_WithStringPath_ProducesDiagnostic() + { + // ZipFileExtensions members are extension methods; the reduced call form must still be seen. + var diags = await GetDiagnosticsAsync(""" + using System.IO.Compression; + using Microsoft.Build.Framework; + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } + public string DestinationPath { get; set; } = ""; + public override bool Execute() + { + using var archive = ZipFile.OpenRead(TaskEnvironment.GetAbsolutePath("archive.zip")); + foreach (var entry in archive.Entries) + { + entry.ExtractToFile(DestinationPath); + } + return true; + } + } + """); + + var pathDiags = diags.Where(d => d.Id == DiagnosticIds.FilePathRequiresAbsolute).ToArray(); + pathDiags.Length.ShouldBe(1); + pathDiags[0].GetMessage().ShouldContain("ExtractToFile"); + } + + [Fact] + public async Task XPathDocumentConstructor_WithStringPath_ProducesDiagnostic() + { + var diags = await GetDiagnosticsAsync(""" + using System.Xml.XPath; + public class MyTask : Microsoft.Build.Utilities.Task + { + public string DocumentPath { get; set; } = ""; + public override bool Execute() + { + var doc = new XPathDocument(DocumentPath); + return true; + } + } + """); + + diags.ShouldContain(d => d.Id == DiagnosticIds.FilePathRequiresAbsolute); + } + // ═══════════════════════════════════════════════════════════════════════ // Iteration 9-13: New APIs and features // ═══════════════════════════════════════════════════════════════════════ diff --git a/src/TaskAnalyzer.Tests/MultiThreadableTaskCodeFixProviderTests.cs b/src/TaskAnalyzer.Tests/MultiThreadableTaskCodeFixProviderTests.cs index ff43df8d145..eb5dd024d3f 100644 --- a/src/TaskAnalyzer.Tests/MultiThreadableTaskCodeFixProviderTests.cs +++ b/src/TaskAnalyzer.Tests/MultiThreadableTaskCodeFixProviderTests.cs @@ -249,4 +249,42 @@ public override bool Execute() Diag(DiagnosticIds.FilePathRequiresAbsolute).WithLocation(0) .WithArguments("new FileInfo(...)", "wrap path argument with TaskEnvironment.GetAbsolutePath()")).RunAsync(); } + + [Fact] + public async Task Fix_ZipFileExtensionsStaticForm_WrapsPathArgumentNotReceiver() + { + // The static form of an extension method puts a non-path argument first. The fix must + // skip it and wrap the string path parameter, otherwise it produces code that does not compile. + await CreateFixTest( + testCode: """ + using System.IO.Compression; + using Microsoft.Build.Framework; + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } + public override bool Execute() + { + ZipArchive archive = null!; + {|#0:ZipFileExtensions.CreateEntryFromFile(archive, "input.txt", "entry.txt")|}; + return true; + } + } + """, + fixedCode: """ + using System.IO.Compression; + using Microsoft.Build.Framework; + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } + public override bool Execute() + { + ZipArchive archive = null!; + ZipFileExtensions.CreateEntryFromFile(archive, TaskEnvironment.GetAbsolutePath("input.txt"), "entry.txt"); + return true; + } + } + """, + Diag(DiagnosticIds.FilePathRequiresAbsolute).WithLocation(0) + .WithArguments("ZipFileExtensions.CreateEntryFromFile(ZipArchive, string, string)", "wrap path argument with TaskEnvironment.GetAbsolutePath()")).RunAsync(); + } } diff --git a/src/TaskAnalyzer.Tests/TestHelpers.cs b/src/TaskAnalyzer.Tests/TestHelpers.cs index 73bbbb2dbf2..21f7f38cbde 100644 --- a/src/TaskAnalyzer.Tests/TestHelpers.cs +++ b/src/TaskAnalyzer.Tests/TestHelpers.cs @@ -285,6 +285,7 @@ private static MetadataReference[] CreateCoreReferences() typeof(System.Xml.XmlReader).Assembly, // System.Xml.ReaderWriter typeof(System.IO.Compression.ZipFile).Assembly, // System.IO.Compression.ZipFile typeof(System.IO.Compression.ZipArchive).Assembly, // System.IO.Compression + typeof(System.Security.Cryptography.X509Certificates.X509Certificate2).Assembly, // System.Security.Cryptography }; var locations = assemblies diff --git a/src/TaskAnalyzer/MultiThreadableTaskCodeFixProvider.cs b/src/TaskAnalyzer/MultiThreadableTaskCodeFixProvider.cs index 240c3b62d82..23fd64ef9cf 100644 --- a/src/TaskAnalyzer/MultiThreadableTaskCodeFixProvider.cs +++ b/src/TaskAnalyzer/MultiThreadableTaskCodeFixProvider.cs @@ -12,6 +12,7 @@ using Microsoft.CodeAnalysis.CSharp; using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.Editing; +using Microsoft.CodeAnalysis.Operations; namespace Microsoft.Build.TaskAuthoring.Analyzer { @@ -38,13 +39,15 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) return; } + var semanticModel = await context.Document.GetSemanticModelAsync(context.CancellationToken).ConfigureAwait(false); + foreach (var diagnostic in context.Diagnostics) { var node = root.FindNode(diagnostic.Location.SourceSpan); if (diagnostic.Id == DiagnosticIds.FilePathRequiresAbsolute) { - RegisterFilePathFix(context, node, diagnostic); + RegisterFilePathFix(context, semanticModel, node, diagnostic); } else if (diagnostic.Id == DiagnosticIds.TaskEnvironmentRequired) { @@ -53,7 +56,7 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) } } - private static void RegisterFilePathFix(CodeFixContext context, SyntaxNode node, Diagnostic diagnostic) + private static void RegisterFilePathFix(CodeFixContext context, SemanticModel? semanticModel, SyntaxNode node, Diagnostic diagnostic) { // Find the invocation or object creation expression var invocation = FindContainingCall(node); @@ -75,17 +78,7 @@ private static void RegisterFilePathFix(CodeFixContext context, SyntaxNode node, return; } - // Find the first argument that is NOT already wrapped with TaskEnvironment.GetAbsolutePath() - ArgumentSyntax? targetArg = null; - foreach (var arg in argumentList.Arguments) - { - if (!IsAlreadyWrapped(arg.Expression)) - { - targetArg = arg; - break; - } - } - + var targetArg = FindPathArgumentToWrap(argumentList, semanticModel, context.CancellationToken); if (targetArg is null) { return; @@ -99,6 +92,41 @@ private static void RegisterFilePathFix(CodeFixContext context, SyntaxNode node, diagnostic); } + /// + /// Finds the first argument that is NOT already wrapped with TaskEnvironment.GetAbsolutePath() + /// and that binds to a string parameter whose name identifies it as a path — the same test the + /// analyzer applies when it reports the diagnostic. Skipping non-path parameters keeps the fix off + /// arguments that cannot be rooted, such as the ZipArchive receiver of the static form of + /// ZipFileExtensions.CreateEntryFromFile(archive, sourceFileName, entryName). + /// + private static ArgumentSyntax? FindPathArgumentToWrap( + ArgumentListSyntax argumentList, SemanticModel? semanticModel, CancellationToken cancellationToken) + { + foreach (var arg in argumentList.Arguments) + { + if (IsAlreadyWrapped(arg.Expression)) + { + continue; + } + + // Without a semantic model the parameter cannot be inspected; fall back to the first + // unwrapped argument, which is the path for the overwhelming majority of path APIs. + if (semanticModel is null) + { + return arg; + } + + if (semanticModel.GetOperation(arg, cancellationToken) is IArgumentOperation { Parameter: { } parameter } && + parameter.Type.SpecialType == SpecialType.System_String && + SharedAnalyzerHelpers.IsPathParameterName(parameter.Name)) + { + return arg; + } + } + + return null; + } + /// /// Checks whether an argument expression is already wrapped in TaskEnvironment.GetAbsolutePath(). /// diff --git a/src/TaskAnalyzer/README.md b/src/TaskAnalyzer/README.md index d4e3cc2f511..e3fca8b23f9 100644 --- a/src/TaskAnalyzer/README.md +++ b/src/TaskAnalyzer/README.md @@ -66,9 +66,22 @@ These APIs access process-global state that varies per task in multithreaded mod File system APIs that accept a path parameter will resolve relative paths against the process working directory — which is shared and unpredictable in multithreaded mode. -**Monitored types:** `File`, `Directory`, `FileInfo`, `DirectoryInfo`, `FileStream`, `StreamReader`, `StreamWriter`, `FileSystemWatcher` +The rule covers the obvious `System.IO` entry points as well as *analyzer-invisible path consumers* — APIs on types that have nothing to do with `System.IO` but still take a path string and hit the file system. -The analyzer inspects parameter names to determine which arguments are paths (e.g., `path`, `fileName`, `sourceFileName`, `destFileName`) and skips non-path string parameters (e.g., `contents`, `searchPattern`). Named arguments are handled correctly. +**Monitored types:** + +| Area | Types | +|---|---| +| Files and directories | `File`, `Directory`, `FileInfo`, `DirectoryInfo`, `FileStream`, `StreamReader`, `StreamWriter`, `FileSystemWatcher`, `BinaryReader`, `BinaryWriter`, `MemoryMappedFile` | +| XML | `XDocument`, `XElement`, `XmlDocument`, `XmlReader`, `XmlWriter`, `XmlTextReader`, `XmlTextWriter`, `XPathDocument`, `XslCompiledTransform`, `XmlSchema` | +| Compression | `ZipFile`, `ZipFileExtensions` | +| Certificates | `X509Certificate`, `X509Certificate2`, `X509Certificate2Collection`, `X509CertificateLoader` | +| Reflection | `AssemblyName` (`GetAssemblyName`), `AssemblyLoadContext` | +| Diagnostics / resources | `FileVersionInfo`, `TextWriterTraceListener`, `ResourceReader`, `ResourceWriter` | + +The analyzer inspects parameter names to determine which arguments are paths (e.g., `path`, `fileName`, `sourceFileName`, `destFileName`) and skips non-path string parameters (e.g., `contents`, `searchPattern`, `password`, `namespaceURI`, `xpath`). Named arguments are handled correctly. + +This parameter-name filter is what makes it safe to monitor whole types that mix path and non-path members: only the string-path overloads are flagged, so `XmlDocument.Load(string)` is reported while `XmlDocument.LoadXml(string)`, `XmlDocument.CreateElement(...)` and the `Stream`/`TextReader` overloads are not. **Recognized safe patterns** (suppress the diagnostic): @@ -334,7 +347,7 @@ The analyzer ships with a code fix provider that offers automatic replacements: | MSBuildTask0007: `new AbsolutePath(Item.GetMetadata("FullPath"))` | → Retype `Item` to ``ITaskItem`` and replace with `Item.Value` | | MSBuildTask0008: relative default `= "obj"` on a path property | → Retype the property (unset default) and move the default into `Execute()` as a guarded, `TaskEnvironment`-rooted assignment | -The MSBuildTask0003 fixer intelligently finds the first **unwrapped** path argument rather than blindly wrapping the first argument — so for `File.Copy(safePath, unsafePath)` it correctly wraps the second argument. +The MSBuildTask0003 fixer finds the first **unwrapped path** argument rather than blindly wrapping the first argument — so for `File.Copy(safePath, unsafePath)` it correctly wraps the second argument. It binds each argument to its parameter, so arguments that cannot be rooted are skipped: for `ZipFileExtensions.CreateEntryFromFile(archive, sourceFileName, entryName)` it wraps `sourceFileName`, not the leading `ZipArchive` or the trailing entry name. The MSBuildTask0006/MSBuildTask0007 fixer is conservative by design: it only offers a fix when every reference to the property — across all partial declarations of the task type, in the current document — can be safely rewritten as part of the same change, so the resulting code keeps compiling after the property type is updated. If the property is referenced from another file (a partial class spread across documents) in a way this single-document fix can't rewrite, no fix is offered. diff --git a/src/TaskAnalyzer/SharedAnalyzerHelpers.cs b/src/TaskAnalyzer/SharedAnalyzerHelpers.cs index 3c8f9643cb1..78dd85d40c6 100644 --- a/src/TaskAnalyzer/SharedAnalyzerHelpers.cs +++ b/src/TaskAnalyzer/SharedAnalyzerHelpers.cs @@ -326,20 +326,24 @@ internal static ImmutableHashSet ResolveFilePathTypes(Compilat "System.IO.BinaryReader", "System.IO.BinaryWriter", - // XML types — only include types where the majority of string - // parameters are file paths. XmlDocument is excluded because methods - // like CreateElement, CreateAttribute etc. take non-path strings. + // XML types. Members that take non-path strings (CreateElement's name/ + // namespaceURI, SelectNodes' xpath, LoadXml's xml, ...) are filtered out by + // IsPathParameterName, and the stream/reader overloads carry no string + // parameter at all, so only the string-path overloads are flagged. "System.Xml.Linq.XDocument", "System.Xml.Linq.XElement", + "System.Xml.XmlDocument", "System.Xml.XmlReader", "System.Xml.XmlWriter", "System.Xml.XmlTextReader", "System.Xml.XmlTextWriter", + "System.Xml.XPath.XPathDocument", "System.Xml.Xsl.XslCompiledTransform", "System.Xml.Schema.XmlSchema", // Compression types that accept file paths "System.IO.Compression.ZipFile", + "System.IO.Compression.ZipFileExtensions", // Memory-mapped files "System.IO.MemoryMappedFiles.MemoryMappedFile", @@ -347,6 +351,8 @@ internal static ImmutableHashSet ResolveFilePathTypes(Compilat // Security / certificates "System.Security.Cryptography.X509Certificates.X509Certificate", "System.Security.Cryptography.X509Certificates.X509Certificate2", + "System.Security.Cryptography.X509Certificates.X509Certificate2Collection", + "System.Security.Cryptography.X509Certificates.X509CertificateLoader", // Diagnostics "System.Diagnostics.FileVersionInfo", @@ -358,6 +364,12 @@ internal static ImmutableHashSet ResolveFilePathTypes(Compilat // Assembly loading (supplements the banned-API list for path-based overloads) "System.Runtime.Loader.AssemblyLoadContext", + + // AssemblyName.GetAssemblyName(assemblyFile) opens the file to read its + // manifest, so a relative path resolves against the shared working directory. + // The AssemblyName(assemblyName) constructor takes a display name, not a path, + // and is filtered out by IsPathParameterName. + "System.Reflection.AssemblyName", }; var builder = ImmutableHashSet.CreateBuilder(SymbolEqualityComparer.Default);