GetItemProvenance should not unescape input (dotnet#1332)

ItemSpec users control unescaping. This resolves dotnet#1106

Also fixes various issues with slash normalization which got exposed by the fix for dotnet#1106:
- slashes are normalized only when the string is a path
- checking whether a string is a valid path uses a union of Windows and !Windows characters, based on: dotnet#781 (comment)
- Re-implement Path.GetFileName so it works with potentially malformed paths.
- Normalize the slashes before normalizing the path. Otherwise .Net's implementation of GetFullPath does not work.

src/Shared/FileUtilities.cs

@@ -55,6 +55,35 @@ internal static void ClearCacheDirectoryPath()
         // TODO: assumption on file system case sensitivity: https://github.com/Microsoft/msbuild/issues/781
         internal static readonly StringComparison PathComparison = StringComparison.OrdinalIgnoreCase;
 
+        /// <summary>
+        /// Copied from https://github.com/dotnet/corefx/blob/056715ff70e14712419d82d51c8c50c54b9ea795/src/Common/src/System/IO/PathInternal.Windows.cs#L61
+        /// MSBuild should support the union of invalid path chars across the supported OSes, so builds can have the same behaviour crossplatform: https://github.com/Microsoft/msbuild/issues/781#issuecomment-243942514
+        /// </summary>
+
+        internal static char[] InvalidPathChars => new char[]
+        {
+            '|', '\0',
+            (char)1, (char)2, (char)3, (char)4, (char)5, (char)6, (char)7, (char)8, (char)9, (char)10,
+            (char)11, (char)12, (char)13, (char)14, (char)15, (char)16, (char)17, (char)18, (char)19, (char)20,
+            (char)21, (char)22, (char)23, (char)24, (char)25, (char)26, (char)27, (char)28, (char)29, (char)30,
+            (char)31
+        };
+
+        /// <summary>
+        /// Copied from https://github.com/dotnet/corefx/blob/387cf98c410bdca8fd195b28cbe53af578698f94/src/System.Runtime.Extensions/src/System/IO/Path.Windows.cs#L18
+        /// MSBuild should support the union of invalid path chars across the supported OSes, so builds can have the same behaviour crossplatform: https://github.com/Microsoft/msbuild/issues/781#issuecomment-243942514
+        /// </summary>
+        internal static char[] InvalidFileNameChars => new char[]
+        {
+            '\"', '<', '>', '|', '\0',
+            (char)1, (char)2, (char)3, (char)4, (char)5, (char)6, (char)7, (char)8, (char)9, (char)10,
+            (char)11, (char)12, (char)13, (char)14, (char)15, (char)16, (char)17, (char)18, (char)19, (char)20,
+            (char)21, (char)22, (char)23, (char)24, (char)25, (char)26, (char)27, (char)28, (char)29, (char)30,
+            (char)31, ':', '*', '?', '\\', '/'
+        };
+
+        private static readonly char[] Slashes = { '/', '\\' };
+
         /// <summary>
         /// Retrieves the MSBuild runtime cache directory
         /// </summary>
@@ -379,7 +408,7 @@ internal static string MaybeAdjustFilePath(string value)
             // have no slashes.
             if (NativeMethodsShared.IsWindows || string.IsNullOrEmpty(value) ||
                 value.StartsWith("$(") || value.StartsWith("@(") || value.StartsWith("\\\\") ||
-                value.IndexOfAny(new[] { '/', '\\' }) == -1)
+                value.IndexOfAny(Slashes) == -1)
             {
                 return value;
             }
@@ -551,7 +580,6 @@ internal static string GetFullPathNoThrow(string path)
         /// <returns></returns>
         internal static bool ComparePathsNoThrow(string first, string second, string currentDirectory)
         {
-
             // perf: try comparing the bare strings first
             if (string.Equals(first, second, PathComparison))
             {
@@ -571,46 +599,37 @@ internal static bool ComparePathsNoThrow(string first, string second, string cur
         /// </summary>
         internal static string NormalizePathForComparisonNoThrow(string path, string currentDirectory)
         {
-            return GetFullPathNoThrow(path, currentDirectory).NormalizeForPathComparison();
-        }
-
-        /// <summary>
-        /// A variation of Path.GetFullPath that will return the input value 
-        /// instead of throwing any IO exception.
-        /// Useful to get a better path for an error message, without the risk of throwing
-        /// if the error message was itself caused by the path being invalid!
-        /// </summary>
-        internal static string GetFullPathNoThrow(string fileSpec, string currentDirectory)
-        {
-            var fullPath = fileSpec;
-
             // file is invalid, return early to avoid triggering an exception
-            if (IsInvalidPath(fullPath))
+            if (PathIsInvalid(path))
             {
-                return fullPath;
+                return path;
             }
 
-            try
-            {
-                fullPath = GetFullPath(fileSpec, currentDirectory);
-            }
-            catch (Exception ex) when (ExceptionHandling.IsIoRelatedException(ex))
-            {
-            }
+            var normalizedPath = path.NormalizeForPathComparison();
+            var fullPath = GetFullPathNoThrow(Path.Combine(currentDirectory, normalizedPath));
 
             return fullPath;
         }
 
-        private static bool IsInvalidPath(string path)
+        private static bool PathIsInvalid(string path)
         {
-            if (path.IndexOfAny(Path.GetInvalidPathChars()) >= 0)
+            if (path.IndexOfAny(InvalidPathChars) >= 0)
             {
                 return true;
             }
 
-            var filename = Path.GetFileName(path);
+            var filename = GetFileName(path);
 
-            return filename.IndexOfAny(Path.GetInvalidFileNameChars()) >= 0;
+            return filename.IndexOfAny(InvalidFileNameChars) >= 0;
+        }
+
+        // Path.GetFileName does not react well to malformed filenames.
+        // For example, Path.GetFileName("a/b/foo:bar") returns bar instead of foo:bar
+        // It also throws exceptions on illegal path characters
+        private static string GetFileName(string path)
+        {
+            var lastDirectorySeparator = path.LastIndexOfAny(Slashes);
+            return lastDirectorySeparator >= 0 ? path.Substring(lastDirectorySeparator + 1) : path;
         }
 
         /// <summary>

src/XMakeBuildEngine/Evaluation/ItemSpec.cs

@@ -91,7 +91,7 @@ private IEnumerable<ItemFragment> BuildItemFragments(IElementLocation itemSpecLo
                         var containsRealWildcards = FileMatcher.HasWildcards(splitEscaped);
 
                         // '*' is an illegal character to have in a filename.
-                        // todo file-system assumption on legal path characters: https://github.com/Microsoft/msbuild/issues/781
+                        // todo: file-system assumption on legal path characters: https://github.com/Microsoft/msbuild/issues/781
                         if (containsEscapedWildcards && containsRealWildcards)
                         {
 
@@ -208,7 +208,7 @@ protected ItemFragment(string itemSpecFragment, string projectPath)
             : this(
                 itemSpecFragment,
                 projectPath,
-                new Lazy<Func<string, bool>>(() => EngineFileUtilities.GetMatchTester(itemSpecFragment, projectPath)))
+                new Lazy<Func<string, bool>>(() => EngineFileUtilities.GetFileSpecMatchTester(itemSpecFragment, projectPath)))
         {
         }
 

src/XMakeBuildEngine/Evaluation/LazyItemEvaluator.IncludeOperation.cs

@@ -7,6 +7,7 @@
 using Microsoft.Build.Construction;
 using System.Collections.Immutable;
 using Microsoft.Build.Internal;
+using Microsoft.Build.Shared;
 
 namespace Microsoft.Build.Evaluation
 {
@@ -54,7 +55,7 @@ protected override ICollection<I> SelectItems(ImmutableList<ItemData>.Builder li
 
                     if (excludePatterns.Any())
                     {
-                        excludeTester = new Lazy<Func<string, bool>>(() => EngineFileUtilities.GetMatchTester(excludePatterns, _rootDirectory));
+                        excludeTester = new Lazy<Func<string, bool>>(() => EngineFileUtilities.GetFileSpecMatchTester(excludePatterns, _rootDirectory));
                     }
                 }
 
@@ -84,7 +85,7 @@ protected override ICollection<I> SelectItems(ImmutableList<ItemData>.Builder li
                         string value = ((ValueFragment)fragment).ItemSpecFragment;
 
                         if (excludeTester == null ||
-                            !excludeTester.Value(value))
+                            !excludeTester.Value(EscapingUtilities.UnescapeAll(value)))
                         {
                             var item = _itemFactory.CreateItem(value, value, _itemElement.ContainingProject.FullPath);
                             itemsToAdd.Add(item);

src/XMakeBuildEngine/UnitTestsPublicOM/Definition/ProjectItem_Tests.cs

@@ -479,32 +479,26 @@ public void ExcludeVectorWithIncludeVector()
             "b",
             new string[0],
             new[] { "a" })]
-        //// items as strings: escaped include matches non-escaped exclude
+        // items as strings: escaped include matches non-escaped exclude
         [InlineData(ItemWithIncludeAndExclude,
             "%61",
             "a",
             new string[0],
             new string[0])]
-        //// items as strings: non-escaped include matches escaped exclude
+        // items as strings: non-escaped include matches escaped exclude
         [InlineData(ItemWithIncludeAndExclude,
             "a",
             "%61",
             new string[0],
             new string[0])]
-        // items as strings: include with escaped wildcard and non-escaped wildcard matches exclude with escaped wildcard and non-escaped wildcard. Both are treated as values and not as globs
-        [InlineData(ItemWithIncludeAndExclude,
-            @"**/a%2Axb",
-            @"**/a%2Axb",
-            new string[0],
-            new string[0])]
         // items as files: non-escaped wildcard include matches escaped non-wildcard character
         [InlineData(ItemWithIncludeAndExclude,
             "a?b",
             "a%40b",
             new[] { "acb", "a@b" },
             new[] { "acb" })]
-       // items as files: non-escaped non-wildcard include matches escaped non-wildcard character
-       [InlineData(ItemWithIncludeAndExclude,
+        // items as files: non-escaped non-wildcard include matches escaped non-wildcard character
+        [InlineData(ItemWithIncludeAndExclude,
            "acb;a@b",
            "a%40b",
            new string[0],
@@ -521,16 +515,16 @@ public void ExcludeVectorWithIncludeVector()
             "a%40?b",
             new[] { "a@b", "a@ab", "a@bb" },
             new[] { "a@b" })]
-       // items as files: non-escaped recursive wildcard include matches escaped recursive wildcard exclude
-       [InlineData(ItemWithIncludeAndExclude,
+        // items as files: non-escaped recursive wildcard include matches escaped recursive wildcard exclude
+        [InlineData(ItemWithIncludeAndExclude,
            @"**\a*b",
            @"**\a*%78b",
            new[] { "aab", "aaxb", @"dir\abb", @"dir\abxb" },
            new[] { "aab", @"dir\abb" })]
         // items as files: include with non-escaped glob does not match exclude with escaped wildcard character.
-        // The exclude is treated as a literal and only matches against non-glob include fragments (i.e., against values and item references). %2A is *
+        // The exclude is treated as a literal, not a glob, and therefore should not match the input files
         [InlineData(ItemWithIncludeAndExclude,
-            @"**\a*b;**\a%2Axb",
+            @"**\a*b",
             @"**\a%2Axb",
             new[] { "aab", "aaxb", @"dir\abb", @"dir\abxb" },
             new[] { "aab", "aaxb", @"dir\abb", @"dir\abxb" })]
@@ -539,6 +533,31 @@ public void IncludeExcludeWithEscapedCharacters(string projectContents, string i
             TestIncludeExcludeWithDifferentSlashes(projectContents, includeString, excludeString, inputFiles, expectedInclude);
         }
 
+        [Theory]
+        // items as strings: include with both escaped and unescaped glob should be treated as literal and therefore not match against files as a glob
+        [InlineData(ItemWithIncludeAndExclude,
+            @"**\a%2Axb",
+            @"foo",
+            new[] { "aab", "aaxb", @"dir\abb", @"dir\abxb" },
+            new[] { @"**\a*xb" })]
+        // Include with both escaped and unescaped glob does not match exclude with escaped wildcard character which has a different slash orientation
+        // The presence of the escaped and unescaped glob should make things behave as strings-which-are-not-paths and not as strings-which-are-paths
+        [InlineData(ItemWithIncludeAndExclude,
+            @"**\a%2Axb",
+            @"**/a%2Axb",
+            new string[0],
+            new[] { @"**\a*xb" })]
+        // Slashes are not normalized when contents is not a path
+        [InlineData(ItemWithIncludeAndExclude,
+            @"a/b/foo::||bar;a/b/foo::||bar/;a/b/foo::||bar\;a/b\foo::||bar",
+            @"a/b/foo::||bar",
+            new string[0],
+            new [] { "a/b/foo::||bar/", @"a/b/foo::||bar\", @"a/b\foo::||bar" })]
+        public void IncludeExcludeWithNonPathContents(string projectContents, string includeString, string excludeString, string[] inputFiles, string[] expectedInclude)
+        {
+            TestIncludeExclude(projectContents, inputFiles, expectedInclude, includeString, excludeString, normalizeSlashes: false);
+        }
+
         [Theory]
         [InlineData(ItemWithIncludeAndExclude,
             "a.*",