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` 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/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,52 @@ 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)
+        {
+            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/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/Test.FunctionalTests.Sarif/Multitool/BaselineOptionTests.cs

@@ -133,6 +133,7 @@ protected override string ConstructTestOutputFromInputResource(string inputResou
             mockFileSystem.Setup(x => x.FileCreate(outputLogFilePath)).Returns((string path) => File.Create(path));
             mockFileSystem.Setup(x => x.FileCreate(baselineFilePath)).Returns((string path) => File.Create(path));
             mockFileSystem.Setup(x => x.FileInfoLength(It.IsAny<string>())).Returns(100);
+            mockFileSystem.Setup(x => x.PathGetExtension(It.IsAny<string>())).Returns((string path) => Path.GetExtension(path));
 
             var validateCommand = new ValidateCommand();
             var context = new SarifValidationContext { FileSystem = mockFileSystem.Object };

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

@@ -523,6 +523,7 @@ protected override string ConstructTestOutputFromInputResource(string inputResou
             mockFileSystem.Setup(x => x.FileOpenRead(inputLogFilePath)).Returns(new MemoryStream(Encoding.UTF8.GetBytes(v2LogText)));
             mockFileSystem.Setup(x => x.FileReadAllText(It.IsNotIn<string>(inputLogFilePath))).Returns<string>(path => File.ReadAllText(path));
             mockFileSystem.Setup(x => x.FileExists(inputLogFilePath)).Returns(true);
+            mockFileSystem.Setup(x => x.PathGetExtension(It.IsAny<string>())).Returns((string path) => Path.GetExtension(path));
 
             // Some rules are disabled by default, so create a configuration file that explicitly
             // enables the rule under test.

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

@@ -879,6 +879,7 @@ public void AnalyzeCommand_EncodedPaths()
             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)));
+            mockFileSystem.Setup(x => x.PathGetExtension(It.IsAny<string>())).Returns((string path) => Path.GetExtension(path));
 
             var target = new EnumeratedArtifact(mockFileSystem.Object)
             {
@@ -910,6 +911,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()
@@ -1106,6 +1132,7 @@ public void MultithreadedMultithreadedAnalyzeCommandBase_EndToEndMultithreadedAn
                                                                 It.IsAny<SearchOption>())).Returns(files);
             mockFileSystem.Setup(x => x.FileOpenRead(It.IsAny<string>())).Returns(mockStream.Object);
             mockFileSystem.Setup(x => x.FileExists(tempFile.Name)).Returns(true);
+            mockFileSystem.Setup(x => x.PathGetExtension(It.IsAny<string>())).Returns((string path) => Path.GetExtension(path));
 
             Output.WriteLine($"The seed that will be used is: {TestRule.s_seed}");
 
@@ -1307,6 +1334,7 @@ public void MultithreadedMultithreadedAnalyzeCommandBase_TargetFileSizeTestCases
                 mockFileSystem.Setup(x => x.FileExists(tempFile.Name)).Returns(true);
                 mockFileSystem.Setup(x => x.FileInfoLength(It.IsAny<string>())).Returns((long)testCase.fileSizeInBytes);
                 mockFileSystem.Setup(x => x.IsSymbolicLink(It.IsAny<string>())).Returns((bool)testCase.isSymbolicLink);
+                mockFileSystem.Setup(x => x.PathGetExtension(It.IsAny<string>())).Returns((string path) => Path.GetExtension(path));
 
                 bool expectedToBeWithinLimits =
                     testCase.fileSizeInBytes != 0 &&
@@ -1356,6 +1384,7 @@ public void MultithreadedMultithreadedAnalyzeCommandBase_ErrorWhenHashing()
                                                                 It.IsAny<string>(),
                                                                 It.IsAny<SearchOption>())).Returns(files);
             mockFileSystem.Setup(x => x.FileOpenRead(It.IsAny<string>())).Returns(mockStream.Object);
+            mockFileSystem.Setup(x => x.PathGetExtension(It.IsAny<string>())).Returns((string path) => Path.GetExtension(path));
 
             var options = new TestAnalyzeOptions
             {
@@ -2164,6 +2193,7 @@ private static IFileSystem CreateDefaultFileSystemForResultsCaching(IList<string
             mockFileSystem.Setup(x => x.DirectoryEnumerateFiles(It.IsAny<string>())).Returns(new string[0]);
             mockFileSystem.Setup(x => x.DirectoryEnumerateFiles(It.IsAny<string>(), It.IsAny<string>(), SearchOption.TopDirectoryOnly)).Returns(files);
             mockFileSystem.Setup(x => x.DirectoryGetFiles(It.IsAny<string>(), It.IsAny<string>())).Returns(files);
+            mockFileSystem.Setup(x => x.PathGetExtension(It.IsAny<string>())).Returns((string path) => Path.GetExtension(path));
 
             for (int i = 0; i < files.Count; i++)
             {

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

@@ -50,6 +50,7 @@ public void ValidateCommand_AcceptsTargetFileWithSpaceInName()
             mockFileSystem.Setup(x => x.FileOpenRead(logFilePath)).Returns(new MemoryStream(Encoding.UTF8.GetBytes(RewriteCommandTests.MinimalCurrentV2Text)));
             mockFileSystem.Setup(x => x.FileReadAllText(SchemaFilePath)).Returns(SchemaFileContents);
             mockFileSystem.Setup(x => x.FileExists(logFilePath)).Returns(true);
+            mockFileSystem.Setup(x => x.PathGetExtension(It.IsAny<string>())).Returns((string path) => Path.GetExtension(path));
 
             var validateCommand = new ValidateCommand(mockFileSystem.Object);
 

src/Test.UnitTests.Sarif/FileSystemTests.cs

@@ -74,5 +74,19 @@ private void CleanupDirectoryOrFile(string[] paths)
                 }
             }
         }
+
+        [Fact]
+        public void PathGetExtension_ShouldExtractExtension()
+        {
+            FileSystem.Instance.PathGetExtension("test.txt").Should().Be(".txt");
+            FileSystem.Instance.PathGetExtension("C:\\test.abcde").Should().Be(".abcde");
+            FileSystem.Instance.PathGetExtension("C:\\some_dir\\test.yft").Should().Be(".yft");
+            FileSystem.Instance.PathGetExtension("http://example.com/some.file.ext").Should().Be(".ext");
+            FileSystem.Instance.PathGetExtension("some.other.dir/extensionless").Should().Be(string.Empty);
+            FileSystem.Instance.PathGetExtension("yet<another>dir\\with|pipes|file.a").Should().Be(".a");
+            FileSystem.Instance.PathGetExtension("yet<another>dir\\with|pipes|file.a.").Should().Be(string.Empty);
+            FileSystem.Instance.PathGetExtension("yet<another>dir\\with.pipes|file").Should().Be(".pipes|file");
+        }
+
     }
 }

src/Test.UnitTests.Sarif/MockFactory.cs

@@ -2,6 +2,7 @@
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
 using System;
+using System.IO;
 
 using Moq;
 
@@ -16,6 +17,7 @@ public static IFileSystem MakeMockFileSystem(string fileName, string fileText =
             mock.Setup(fs => fs.PathGetFullPath(It.IsAny<string>())).Returns((string path) => path);
             mock.Setup(fs => fs.FileReadAllText(fileName)).Returns(fileText ?? string.Join(Environment.NewLine, fileLines));
             mock.Setup(fs => fs.FileReadAllLines(fileName)).Returns(fileLines ?? fileText.Split(new[] { Environment.NewLine }, StringSplitOptions.RemoveEmptyEntries));
+            mock.Setup(x => x.PathGetExtension(It.IsAny<string>())).Returns((string path) => Path.GetExtension(path));
             return mock.Object;
         }
     }