From 40801a867a2459d5c5469597dba7224f2da8d507 Mon Sep 17 00:00:00 2001 From: Jeremy Kuhne Date: Mon, 11 Jun 2018 14:46:20 -0700 Subject: [PATCH 1/3] Fix creating subdirectories under directories with trailing separators This broke with #27810. Reworked the logic in an attempt to be clearer and added test cases. This addresses #30283. --- .../src/System/IO/DirectoryInfo.cs | 16 ++++++---- .../tests/DirectoryInfo/CreateSubdirectory.cs | 32 ++++++++++++++++++- 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/src/System.IO.FileSystem/src/System/IO/DirectoryInfo.cs b/src/System.IO.FileSystem/src/System/IO/DirectoryInfo.cs index ecdda000903a..6e8bbe238a10 100644 --- a/src/System.IO.FileSystem/src/System/IO/DirectoryInfo.cs +++ b/src/System.IO.FileSystem/src/System/IO/DirectoryInfo.cs @@ -61,17 +61,21 @@ public DirectoryInfo CreateSubdirectory(string path) if (Path.IsPathRooted(path)) throw new ArgumentException(SR.Arg_Path2IsRooted, nameof(path)); - string fullPath = Path.GetFullPath(Path.Combine(FullPath, path)); + string newPath = Path.GetFullPath(Path.Combine(FullPath, path)); - if (fullPath.Length < FullPath.Length - || (fullPath.Length > FullPath.Length && !PathInternal.IsDirectorySeparator(fullPath[FullPath.Length])) - || string.Compare(FullPath, 0, fullPath, 0, FullPath.Length, PathInternal.StringComparison) != 0) + ReadOnlySpan trimmedNewPath = PathInternal.TrimEndingDirectorySeparator(newPath.AsSpan()); + ReadOnlySpan trimmedCurrentPath = PathInternal.TrimEndingDirectorySeparator(FullPath.AsSpan()); + + // We want to make sure the requested directory is actually under the subdirectory. + if (!trimmedNewPath.StartsWith(trimmedCurrentPath, PathInternal.StringComparison) + // Allow the exact same path, but prevent allowing "..\FooBar" through when the directory is "Foo" + || ((trimmedNewPath.Length > trimmedCurrentPath.Length) && !PathInternal.IsDirectorySeparator(newPath[trimmedCurrentPath.Length]))) { throw new ArgumentException(SR.Format(SR.Argument_InvalidSubPath, path, FullPath), nameof(path)); } - FileSystem.CreateDirectory(fullPath); - return new DirectoryInfo(fullPath); + FileSystem.CreateDirectory(newPath); + return new DirectoryInfo(newPath); } public void Create() => FileSystem.CreateDirectory(FullPath); diff --git a/src/System.IO.FileSystem/tests/DirectoryInfo/CreateSubdirectory.cs b/src/System.IO.FileSystem/tests/DirectoryInfo/CreateSubdirectory.cs index 75af106c41a9..6a6ea3d9789a 100644 --- a/src/System.IO.FileSystem/tests/DirectoryInfo/CreateSubdirectory.cs +++ b/src/System.IO.FileSystem/tests/DirectoryInfo/CreateSubdirectory.cs @@ -89,17 +89,40 @@ public void SubDirectoryIsParentDirectory_ThrowsArgumentException() Assert.Throws(() => new DirectoryInfo(TestDirectory + "/path").CreateSubdirectory("../../path2")); } + [Fact] + public void SubdirectoryOverlappingName_ThrowsArgumentException() + { + // What we're looking for here is trying to create C:\FooBar under C:\Foo by passing "..\FooBar" + DirectoryInfo info = Directory.CreateDirectory(GetTestFilePath()); + + string overlappingName = ".." + Path.DirectorySeparatorChar + info.Name + "overlap"; + + Assert.Throws(() => info.CreateSubdirectory(overlappingName)); + + // Now try with an info with a trailing separator + info = new DirectoryInfo(info.FullName + Path.DirectorySeparatorChar); + Assert.Throws(() => info.CreateSubdirectory(overlappingName)); + } + [Theory, MemberData(nameof(ValidPathComponentNames))] public void ValidPathWithTrailingSlash(string component) { DirectoryInfo testDir = Directory.CreateDirectory(GetTestFilePath()); - string path = IOServices.AddTrailingSlashIfNeeded(component); + string path = component + Path.DirectorySeparatorChar; DirectoryInfo result = new DirectoryInfo(testDir.FullName).CreateSubdirectory(path); Assert.Equal(Path.Combine(testDir.FullName, path), result.FullName); Assert.True(Directory.Exists(result.FullName)); + + // Now try creating subdirectories when the directory info itself has a slash + testDir = Directory.CreateDirectory(GetTestFilePath() + Path.DirectorySeparatorChar); + + result = new DirectoryInfo(testDir.FullName).CreateSubdirectory(path); + + Assert.Equal(Path.Combine(testDir.FullName, path), result.FullName); + Assert.True(Directory.Exists(result.FullName)); } [Theory, @@ -114,6 +137,13 @@ public void ValidPathWithoutTrailingSlash(string component) Assert.Equal(Path.Combine(testDir.FullName, path), result.FullName); Assert.True(Directory.Exists(result.FullName)); + // Now try creating subdirectories when the directory info itself has a slash + testDir = Directory.CreateDirectory(GetTestFilePath() + Path.DirectorySeparatorChar); + + result = new DirectoryInfo(testDir.FullName).CreateSubdirectory(path); + + Assert.Equal(Path.Combine(testDir.FullName, path), result.FullName); + Assert.True(Directory.Exists(result.FullName)); } [Fact] From fa6caa4736dca989d31cc69258cfbc59372ba4ca Mon Sep 17 00:00:00 2001 From: Jeremy Kuhne Date: Mon, 11 Jun 2018 20:56:00 -0700 Subject: [PATCH 2/3] Flip the logic to make it a little easier to follow. (Reduces the number of nots.) --- .../src/System/IO/DirectoryInfo.cs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/System.IO.FileSystem/src/System/IO/DirectoryInfo.cs b/src/System.IO.FileSystem/src/System/IO/DirectoryInfo.cs index 6e8bbe238a10..f07ee9ad87ff 100644 --- a/src/System.IO.FileSystem/src/System/IO/DirectoryInfo.cs +++ b/src/System.IO.FileSystem/src/System/IO/DirectoryInfo.cs @@ -67,15 +67,16 @@ public DirectoryInfo CreateSubdirectory(string path) ReadOnlySpan trimmedCurrentPath = PathInternal.TrimEndingDirectorySeparator(FullPath.AsSpan()); // We want to make sure the requested directory is actually under the subdirectory. - if (!trimmedNewPath.StartsWith(trimmedCurrentPath, PathInternal.StringComparison) + if (trimmedNewPath.StartsWith(trimmedCurrentPath, PathInternal.StringComparison) // Allow the exact same path, but prevent allowing "..\FooBar" through when the directory is "Foo" - || ((trimmedNewPath.Length > trimmedCurrentPath.Length) && !PathInternal.IsDirectorySeparator(newPath[trimmedCurrentPath.Length]))) + && ((trimmedNewPath.Length == trimmedCurrentPath.Length) || PathInternal.IsDirectorySeparator(newPath[trimmedCurrentPath.Length]))) { - throw new ArgumentException(SR.Format(SR.Argument_InvalidSubPath, path, FullPath), nameof(path)); + FileSystem.CreateDirectory(newPath); + return new DirectoryInfo(newPath); } - FileSystem.CreateDirectory(newPath); - return new DirectoryInfo(newPath); + // We weren't nested + throw new ArgumentException(SR.Format(SR.Argument_InvalidSubPath, path, FullPath), nameof(path)); } public void Create() => FileSystem.CreateDirectory(FullPath); From 074f6d3554c34efb6884974b22c2af61e2fd1515 Mon Sep 17 00:00:00 2001 From: Jeremy Kuhne Date: Tue, 12 Jun 2018 11:09:49 -0700 Subject: [PATCH 3/3] Skip new test on desktop --- .../tests/DirectoryInfo/CreateSubdirectory.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/System.IO.FileSystem/tests/DirectoryInfo/CreateSubdirectory.cs b/src/System.IO.FileSystem/tests/DirectoryInfo/CreateSubdirectory.cs index 6a6ea3d9789a..7ed2e27e4a25 100644 --- a/src/System.IO.FileSystem/tests/DirectoryInfo/CreateSubdirectory.cs +++ b/src/System.IO.FileSystem/tests/DirectoryInfo/CreateSubdirectory.cs @@ -90,6 +90,7 @@ public void SubDirectoryIsParentDirectory_ThrowsArgumentException() } [Fact] + [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)] public void SubdirectoryOverlappingName_ThrowsArgumentException() { // What we're looking for here is trying to create C:\FooBar under C:\Foo by passing "..\FooBar"