Fix ArgumentException when analysis target URI contains characters that are illegal in the file system

.github/workflows/codeql-js-analysis.yml

@@ -12,7 +12,7 @@ on:
 jobs:
   analyze:
     name: Analyze
-    runs-on: ubuntu-20.04
+    runs-on: ubuntu-22.04
     permissions:
       actions: read
       contents: read

.github/workflows/validate.yml

@@ -10,7 +10,7 @@ jobs:
   build-and-test:
     strategy:
       matrix:
-        os: [windows-latest, ubuntu-20.04, macos-13]
+        os: [windows-latest, ubuntu-22.04, macos-13]
     runs-on: ${{matrix.os}}
     steps:
     - name: Check out code

ReleaseHistory.md

@@ -12,6 +12,7 @@
 * BUG: Fix error `ERR997.NoValidAnalysisTargets` when scanning symbolic link files.
 * BUG: Fix `ERR999.UnhandledEngineException: System.IO.FileNotFoundException: Could not find file` when a file name or directory path contains URL-encoded characters.
 * BUG: Fix error `ERR997.NoValidAnalysisTargets` when ambiguous file/directory references are provided to `OrderedFileSpecifier`. Previously, the code required an explicit directory separator to be added to the end of a directory path. Now, the code inspects the file system and assumes that a reference to an existing directory was intended by the user (even without a trailing separator).
+* BUG: Fix `ArgumentException: Illegal characters in path` in `MultithreadedAnalyzeCommandBase`when analysis target URI contains characters that are illegal in the file system.
 * NEW: Allow null archive uri in `MultithreadedZipArchiveArtifactProvider` (which indicates that enumerated artifact paths should not include the base archive).
 * NEW: Update `LogTargetParseError(IAnalysisContext, Region, string, Exception)` to include optional exception argument to denote code location where parse error occurred.
 * NEW: `MultithreadedAnalyzeCommandBase.EnumerateArtifact` now supports scanning into compressed (OPC) files. Initial support file extensions are: `.apk`, `.appx`, `.appxbundle`, `.docx`, `.epub`, `.jar`, `.msix`, `.msixbundle`, `.odp`, `.ods`, `.odt`, `.onepkg`, `.oxps`, `.pkg`, `.pptx`, `.unitypackage`, `.vsdx`, `.xps`, `.xlsx`, `.zip`.

src/Sarif.Driver/Sdk/ExportConfigurationCommandBase.cs

@@ -61,7 +61,7 @@ public override int Run(ExportConfigurationOptions exportOptions)
                     }
                 }
 
-                string extension = Path.GetExtension(exportOptions.OutputFilePath);
+                string extension = FileSystem.PathGetExtension(exportOptions.OutputFilePath);
 
                 if (extension.Equals(".xml", StringComparison.OrdinalIgnoreCase))
                 {

src/Sarif.Driver/Sdk/ExportRulesMetadataCommandBase.cs

@@ -27,7 +27,7 @@ public override int Run(ExportRulesMetadataOptions exportOptions)
 
                 string format = "";
                 string outputFilePath = exportOptions.OutputFilePath;
-                string extension = Path.GetExtension(outputFilePath);
+                string extension = FileSystem.PathGetExtension(outputFilePath);
 
                 switch (extension)
                 {

src/Sarif.Driver/Sdk/MultithreadedAnalyzeCommandBase.cs

@@ -106,7 +106,7 @@ public virtual int Run(TOptions options, ref TContext globalContext)
                     else
                     {
                         string etlFilePath =
-                        Path.GetExtension(globalContext.EventsFilePath).Equals(".csv", StringComparison.OrdinalIgnoreCase)
+                        FileSystem.PathGetExtension(globalContext.EventsFilePath).Equals(".csv", StringComparison.OrdinalIgnoreCase)
                             ? $"{Path.GetFileNameWithoutExtension(globalContext.EventsFilePath)}.etl"
                             : globalContext.EventsFilePath;
 
@@ -244,7 +244,7 @@ private int Run(TContext globalContext)
             globalContext.Logger.AnalysisStopped(globalContext.RuntimeErrors);
             disposableLogger?.Dispose();
 
-            if (Path.GetExtension(globalContext.EventsFilePath).Equals(".csv", StringComparison.OrdinalIgnoreCase))
+            if (FileSystem.PathGetExtension(globalContext.EventsFilePath).Equals(".csv", StringComparison.OrdinalIgnoreCase))
             {
                 var dumpEventsCommand = new DumpEventsCommand();
 
@@ -679,7 +679,8 @@ private async Task<bool> EnumerateArtifact(IEnumeratedArtifact artifact, TContex
                 return false;
             }
 
-            string extension = Path.GetExtension(filePath);
+            string extension = FileSystem.PathGetExtension(filePath);
+
             if (artifact.Uri.IsAbsoluteUri &&
                 string.IsNullOrEmpty(artifact.Uri.Query) &&
                 globalContext.OpcFileExtensions.Contains(extension))
@@ -940,7 +941,7 @@ protected virtual TContext InitializeConfiguration(string configurationFileName,
 
             if (string.IsNullOrEmpty(configurationFileName)) { return context; }
 
-            string extension = Path.GetExtension(configurationFileName);
+            string extension = FileSystem.PathGetExtension(configurationFileName);
 
             var configuration = new PropertiesDictionary();
             if (extension.Equals(".xml", StringComparison.OrdinalIgnoreCase))

src/Sarif.Multitool.Library/ApplyPolicyCommand.cs

@@ -31,7 +31,7 @@ public int Run(ApplyPolicyOptions options)
 
                 actualLog.ApplyPolicies();
 
-                string fileName = CommandUtilities.GetTransformedOutputFileName(options);
+                string fileName = CommandUtilities.GetTransformedOutputFileName(FileSystem, options);
 
                 WriteSarifFile(_fileSystem, actualLog, fileName, options.Minify);
 

src/Sarif.Multitool.Library/CommandUtilities.cs

@@ -10,7 +10,7 @@ namespace Microsoft.CodeAnalysis.Sarif.Multitool
 {
     internal static class CommandUtilities
     {
-        internal static string GetTransformedOutputFileName(SingleFileOptionsBase options)
+        internal static string GetTransformedOutputFileName(IFileSystem fileSystem, SingleFileOptionsBase options)
         {
             string filePath = Path.GetFullPath(options.InputFilePath);
 
@@ -25,7 +25,7 @@ internal static string GetTransformedOutputFileName(SingleFileOptionsBase option
             }
 
             const string TransformedExtension = ".transformed.sarif";
-            string extension = Path.GetExtension(filePath);
+            string extension = fileSystem.PathGetExtension(filePath);
 
             // For an input file named MyFile.sarif, returns MyFile.transformed.sarif.
             if (extension.Equals(SarifConstants.SarifFileExtension, StringComparison.OrdinalIgnoreCase))

src/Sarif.Multitool.Library/RewriteCommand.cs

@@ -38,7 +38,7 @@ public int Run(RewriteOptions options)
                     return FAILURE;
                 }
 
-                string actualOutputPath = CommandUtilities.GetTransformedOutputFileName(options);
+                string actualOutputPath = CommandUtilities.GetTransformedOutputFileName(FileSystem, options);
 
                 SarifLog actualLog = null;
 

src/Sarif.Multitool.Library/SarifValidationContext.cs

@@ -3,7 +3,6 @@
 
 using System;
 using System.Collections.Generic;
-using System.IO;
 
 using Newtonsoft.Json.Linq;
 
@@ -30,8 +29,8 @@ public override bool IsValidAnalysisTarget
         {
             get
             {
-                return Path.GetExtension(CurrentTarget.Uri.GetFileName()).Equals(SarifConstants.SarifFileExtension, StringComparison.OrdinalIgnoreCase) ||
-                       Path.GetExtension(CurrentTarget.Uri.GetFileName()).Equals(".json", StringComparison.OrdinalIgnoreCase);
+                return FileSystem.PathGetExtension(CurrentTarget.Uri.GetFileName()).Equals(SarifConstants.SarifFileExtension, StringComparison.OrdinalIgnoreCase) ||
+                       FileSystem.PathGetExtension(CurrentTarget.Uri.GetFileName()).Equals(".json", StringComparison.OrdinalIgnoreCase);
             }
             set
             {

src/Sarif.Multitool.Library/SuppressCommand.cs

@@ -41,7 +41,7 @@ public int Run(SuppressOptions options)
                                                               options.ExpiryInDays,
                                                               options.Status).VisitSarifLog(currentSarifLog);
 
-                string actualOutputPath = CommandUtilities.GetTransformedOutputFileName(options);
+                string actualOutputPath = CommandUtilities.GetTransformedOutputFileName(FileSystem, options);
                 if (options.SarifOutputVersion == SarifVersion.OneZeroZero)
                 {
                     var visitor = new SarifCurrentToVersionOneVisitor();

src/Sarif/FileSystem.cs

@@ -488,5 +488,19 @@ public string PathGetFileNameWithoutExtension(string path)
         {
             return Path.GetFileNameWithoutExtension(path);
         }
+
+        /// <summary>
+        /// Returns the extension of the given path. The returned value includes the period (".") character of the extension
+        /// except when you have a terminal period when you get String.Empty, such as ".exe" or ".cpp".
+        ///
+        /// While the latest version of Path.GetExtension will not throw if the path contains any illegal characters, older
+        /// versions of .NET, some of which are still in use, will.  This method acts like the newer version and will not throw.
+        /// </summary>
+        /// <param name="path">The path to extract the extension from</param>
+        /// <returns>The file extension or null if the given path is null or if the given path does not include an extension.</returns>
+        public string PathGetExtension(string path)
+        {
+            return SarifUtilities.PathGetExtension(path);
+        }
     }
 }

src/Sarif/IFileSystem.cs

@@ -385,5 +385,16 @@ public interface IFileSystem
         /// The string returned by <see cref = "Path.GetFileName(string)"/>, minus the last period (.) and all characters following it.
         /// </returns>
         string PathGetFileNameWithoutExtension(string path);
+
+        /// <summary>
+        /// Returns the extension of the given path. The returned value includes the period (".") character of the extension
+        /// except when you have a terminal period when you get String.Empty, such as ".exe" or ".cpp".
+        ///
+        /// While the latest version of Path.GetExtension will not throw if the path contains any illegal characters, older
+        /// versions of .NET, some of which are still in use, will.  This method acts like the newer version and will not throw.
+        /// </summary>
+        /// <param name="path">The path to extract the extension from</param>
+        /// <returns>The file extension or null if the given path is null or if the given path does not include an extension.</returns>
+        string PathGetExtension(string path);
     }
 }

src/Sarif/SarifUtilities.cs

@@ -4,6 +4,7 @@
 using System;
 using System.Collections.Generic;
 using System.Diagnostics;
+using System.IO;
 using System.Resources;
 using System.Text;
 using System.Text.RegularExpressions;
@@ -210,5 +211,51 @@ internal static Encoding GetEncodingFromName(string encodingName)
                 return null;
             }
         }
+
+        /// <summary>
+        /// Returns the extension of the given path. The returned value includes the period (".") character of the extension
+        /// except when you have a terminal period when you get String.Empty, such as ".exe" or ".cpp".
+        ///
+        /// While the latest version of Path.GetExtension will not throw if the path contains any illegal characters, older
+        /// versions of .NET, some of which are still in use, will.  This method acts like the newer version and will not throw.
+        /// </summary>
+        /// <param name="path">The path to extract the extension from</param>
+        /// <returns>The file extension or null if the given path is null or if the given path does not include an extension.</returns>
+        internal static string PathGetExtension(string path)
+        {
+            if (path == null)
+            {
+                return null;
+            }
+
+            // This function was copied from https://referencesource.microsoft.com/#mscorlib/system/io/path.cs,f424e433705aeb09
+            // with one change.  The following line was commented out so that this method will not throw on
+            // illegal characters
+            // CheckInvalidPathChars(path);
+
+            int length = path.Length;
+            for (int i = length; --i >= 0;)
+            {
+                char ch = path[i];
+                if (ch == '.')
+                {
+                    if (i != length - 1)
+                    {
+                        return path.Substring(i, length - i);
+                    }
+                    else
+                    {
+                        return string.Empty;
+                    }
+                }
+
+                if (ch == Path.DirectorySeparatorChar || ch == Path.AltDirectorySeparatorChar || ch == Path.VolumeSeparatorChar)
+                {
+                    break;
+                }
+            }
+
+            return string.Empty;
+        }
     }
 }

src/Test.FunctionalTests.Sarif/Multitool/BaselineOptionTests.cs

@@ -116,7 +116,7 @@ protected override string ConstructTestOutputFromInputResource(string inputResou
                 RuleKindOption = AllRuleKinds// new List<RuleKind>() { RuleKind.Sarif },
             };
 
-            var mockFileSystem = new Mock<IFileSystem>();
+            Mock<IFileSystem> mockFileSystem = MockFactory.MakeMockFileSystem();
 
             mockFileSystem.Setup(x => x.DirectoryExists(inputLogDirectory)).Returns(true);
             mockFileSystem.Setup(x => x.DirectoryGetDirectories(It.IsAny<string>())).Returns(Array.Empty<string>());

src/Test.FunctionalTests.Sarif/Multitool/ValidateCommandTests.cs

@@ -513,7 +513,7 @@ protected override string ConstructTestOutputFromInputResource(string inputResou
                 RuleKindOption = new List<RuleKind>() { RuleKind.Gh, RuleKind.Sarif },
             };
 
-            var mockFileSystem = new Mock<IFileSystem>();
+            Mock<IFileSystem> mockFileSystem = MockFactory.MakeMockFileSystem();
 
             mockFileSystem.Setup(x => x.FileInfoLength(It.IsAny<string>())).Returns(1);
             mockFileSystem.Setup(x => x.DirectoryExists(inputLogDirectory)).Returns(true);

src/Test.UnitTests.Sarif.Driver/Sdk/MultithreadedAnalyzeCommandBaseTests.cs

@@ -873,23 +873,13 @@ public void AnalyzeCommand_EncodedPaths()
             var uri = new Uri(path, UriKind.Absolute);
 
             string content = "foo foo";
-            var mockFileSystem = new Mock<IFileSystem>();
-            mockFileSystem.Setup(x => x.IsSymbolicLink(path)).Returns(false);
-            mockFileSystem.Setup(x => x.FileStreamLength(path)).Returns(content.Length);
-            mockFileSystem.Setup(x => x.FileInfoLength(path)).Returns(content.Length);
-            mockFileSystem.Setup(x => x.FileReadAllText(path)).Returns(content);
-            mockFileSystem.Setup(x => x.FileOpenRead(path)).Returns(new MemoryStream(Encoding.UTF8.GetBytes(content)));
+            IFileSystem mockFileSystem = MockFactory.MakeMockFileSystem(path, content);
 
-            var target = new EnumeratedArtifact(mockFileSystem.Object)
+            var target = new EnumeratedArtifact(mockFileSystem)
             {
                 Uri = uri
             };
 
-            var options = new TestAnalyzeOptions
-            {
-                PluginFilePaths = new[] { typeof(TestRule).Assembly.FullName },
-            };
-
             var properties = new PropertiesDictionary();
             properties.SetProperty(TestRule.Behaviors, TestRuleBehaviors.LogError);
 
@@ -910,6 +900,31 @@ public void AnalyzeCommand_EncodedPaths()
             sarifLog.Runs[0].Results.Count.Should().Be(1);
         }
 
+        [Fact]
+        public void AnalyzeCommand_IllegalPathCharInURL()
+        {
+            var sarifOutput = new StringBuilder();
+            var command = new TestMultithreadedAnalyzeCommand();
+            using var writer = new StringWriter(sarifOutput);
+            var logger = new SarifLogger(writer,
+                                            run: new Run { Tool = command.Tool },
+                                            levels: BaseLogger.ErrorWarningNote,
+                                            kinds: BaseLogger.Fail);
+
+            var target = new EnumeratedArtifact(FileSystem.Instance) { Uri = new Uri("http://example.com/some<character>test/bad\"characters\"path.txt"), Contents = "fake content" };
+
+            var context = new TestAnalysisContext
+            {
+                TargetsProvider = new ArtifactProvider(new[] { target }),
+                FailureLevels = BaseLogger.ErrorWarningNote,
+                ResultKinds = BaseLogger.Fail,
+                Logger = logger,
+            };
+
+            int result = command.Run(options: null, ref context);
+            result.Should().Be(0);
+        }
+
         [Fact]
         [Trait(TestTraits.WindowsOnly, "true")]
         public void AnalyzeCommand_TracesInMemory()
@@ -1096,7 +1111,7 @@ public void MultithreadedMultithreadedAnalyzeCommandBase_EndToEndMultithreadedAn
             mockStream.Setup(m => m.CanSeek).Returns(true);
             mockStream.Setup(m => m.ReadByte()).Returns('a');
 
-            var mockFileSystem = new Mock<IFileSystem>();
+            Mock<IFileSystem> mockFileSystem = MockFactory.MakeMockFileSystem();
             mockFileSystem.Setup(x => x.FileInfoLength(It.IsAny<string>())).Returns(2048);
             mockFileSystem.Setup(x => x.DirectoryExists(It.IsAny<string>())).Returns(true);
             mockFileSystem.Setup(x => x.DirectoryGetFiles(It.IsAny<string>(), specifier)).Returns(files);
@@ -1264,7 +1279,7 @@ public void MultithreadedMultithreadedAnalyzeCommandBase_TargetFileSizeTestCases
                 mockStream.Setup(m => m.CanSeek).Returns(true);
                 mockStream.Setup(m => m.ReadByte()).Returns('a');
 
-                var mockFileSystem = new Mock<IFileSystem>();
+                Mock<IFileSystem> mockFileSystem = MockFactory.MakeMockFileSystem();
 
                 if (testCase.actualFileContent != null)
                 {
@@ -1347,7 +1362,7 @@ public void MultithreadedMultithreadedAnalyzeCommandBase_ErrorWhenHashing()
             mockStream.Setup(m => m.ReadByte()).Returns('a');
             mockStream.Setup(m => m.Seek(It.IsAny<long>(), It.IsAny<SeekOrigin>())).Throws(new IOException());
 
-            var mockFileSystem = new Mock<IFileSystem>();
+            Mock<IFileSystem> mockFileSystem = MockFactory.MakeMockFileSystem();
             mockFileSystem.Setup(x => x.FileInfoLength(It.IsAny<string>())).Returns(2048);
             mockFileSystem.Setup(x => x.DirectoryExists(It.IsAny<string>())).Returns(true);
             mockFileSystem.Setup(x => x.DirectoryGetFiles(It.IsAny<string>(), specifier)).Returns(files);
@@ -1491,8 +1506,7 @@ public void MultithreadedAnalyzeCommandBase_LoadConfigurationFile(string configV
                 OutputFilePath = "",
             };
 
-            var mockFileSystem = new Mock<IFileSystem>();
-
+            Mock<IFileSystem> mockFileSystem = MockFactory.MakeMockFileSystem();
             mockFileSystem.Setup(x => x.FileExists(It.IsAny<string>())).Returns(defaultFileExists);
 
             var command = new TestMultithreadedAnalyzeCommand(mockFileSystem.Object);
@@ -2157,8 +2171,7 @@ private static IFileSystem CreateDefaultFileSystemForResultsCaching(IList<string
 
             string logFileContents = Guid.NewGuid().ToString();
 
-            var mockFileSystem = new Mock<IFileSystem>();
-
+            Mock<IFileSystem> mockFileSystem = MockFactory.MakeMockFileSystem();
             mockFileSystem.Setup(x => x.DirectoryExists(It.IsAny<string>())).Returns(true);
             mockFileSystem.Setup(x => x.FileInfoLength(It.IsAny<string>())).Returns(2048);
             mockFileSystem.Setup(x => x.DirectoryEnumerateFiles(It.IsAny<string>())).Returns(new string[0]);

src/Test.UnitTests.Sarif.Multitool.Library/ValidateCommandTests.cs

@@ -42,7 +42,7 @@ public void ValidateCommand_AcceptsTargetFileWithSpaceInName()
 
             string logFilePath = Path.Combine(LogFileDirectoryWithSpace, LogFileName);
 
-            var mockFileSystem = new Mock<IFileSystem>();
+            Mock<IFileSystem> mockFileSystem = MockFactory.MakeMockFileSystem();
             mockFileSystem.Setup(x => x.FileInfoLength(It.IsAny<string>())).Returns(1024);
             mockFileSystem.Setup(x => x.DirectoryExists(LogFileDirectoryWithSpace)).Returns(true);
             mockFileSystem.Setup(x => x.DirectoryEnumerateFiles(LogFileDirectoryWithSpace, It.IsAny<string>(), SearchOption.TopDirectoryOnly)).Returns(new[] { LogFileName });