From 1719e9108bc0d589c8a25be0725a84363cf51ced Mon Sep 17 00:00:00 2001 From: Chris Johnston Date: Thu, 25 Aug 2022 17:27:51 -0700 Subject: [PATCH 1/3] Fix #337 (tar): Files added under CurrentDirectory must respect RootPath This changes the behavior of TarArchive when files are added under the CurrentDirectory. When a TarEntry is created from a file which is under the CurrentDirectory, the path is made relative to the CurrentDirectory. This behavior affects how TarArchive removes a prefix from files set by the RootPath directory. The expectation for RootPath is that a matching substring will be removed from the prefix of entries before they are added to the TarArchive. Because the TarEntry for files under CurrentDirectory are modified, RootPath will not work if it is an absolute path under CurrentDirectory. This change fixes this behavior. If RootPath is a directory under CurrentDirectory, it will apply to entres added to TarArchive. An attempt was made to implement this change without additional string manipulation. This could be done with a modified IndexOf, that enables searching for a substring of the pattern within the target substring. This may improve performance. --- src/ICSharpCode.SharpZipLib/Tar/TarArchive.cs | 26 +++- .../Tar/TarTests.cs | 113 ++++++++++++++++++ 2 files changed, 137 insertions(+), 2 deletions(-) diff --git a/src/ICSharpCode.SharpZipLib/Tar/TarArchive.cs b/src/ICSharpCode.SharpZipLib/Tar/TarArchive.cs index 878649017..a20ca2455 100644 --- a/src/ICSharpCode.SharpZipLib/Tar/TarArchive.cs +++ b/src/ICSharpCode.SharpZipLib/Tar/TarArchive.cs @@ -857,12 +857,34 @@ private void WriteEntryCore(TarEntry sourceEntry, bool recurse) if (!String.IsNullOrEmpty(rootPath)) { - if (entry.Name.StartsWith(rootPath, StringComparison.OrdinalIgnoreCase)) + string newRootPath = rootPath; + + // remove CurrentDirectory from RootPath to be consistent with TarEntry behavior + var currentDirectory = Directory.GetCurrentDirectory() + .ToTarArchivePath(); + if (rootPath.StartsWith(currentDirectory, StringComparison.OrdinalIgnoreCase)) { - newName = entry.Name.Substring(rootPath.Length + 1); + if (rootPath.Length == currentDirectory.Length) + { + // if they are the same, rootPath would be empty + // and so the entry name is not modified + newRootPath = string.Empty; + } + else + { + // TarEntry would have removed the current directory from the entry name, and so do the same here + newRootPath = rootPath.Substring(currentDirectory.Length + 1); + } + } + + if (!string.IsNullOrEmpty(newRootPath) && + entry.Name.StartsWith(newRootPath, StringComparison.OrdinalIgnoreCase)) + { + newName = entry.Name.Substring(newRootPath.Length + 1); } } + if (pathPrefix != null) { newName = (newName == null) ? pathPrefix + "/" + entry.Name : pathPrefix + "/" + newName; diff --git a/test/ICSharpCode.SharpZipLib.Tests/Tar/TarTests.cs b/test/ICSharpCode.SharpZipLib.Tests/Tar/TarTests.cs index c6a35ff08..651fcc214 100644 --- a/test/ICSharpCode.SharpZipLib.Tests/Tar/TarTests.cs +++ b/test/ICSharpCode.SharpZipLib.Tests/Tar/TarTests.cs @@ -8,6 +8,7 @@ using System.Threading; using System.Threading.Tasks; using NUnit.Framework.Internal; +using System.Linq; namespace ICSharpCode.SharpZipLib.Tests.Tar { @@ -916,5 +917,117 @@ public void RootPathIsRespected() } } } + + /// + /// This tests the behavior of the property on + /// extracted files. + /// + /// + /// + /// + /// + /// + /// + [Test] + [Category("Tar")] + // {workingDirectory}/temp {workingDirectory}/temp/file.txt -> "file.txt" + [TestCase("temp", true, new string[] { "temp", "file.txt" }, true, true, new string[] { "file.txt" })] + // (unset) {workingDirectory}/temp/file.txt -> "temp/file.txt" + [TestCase("", false, new string[] { "temp", "file.txt" }, true, true, new string[] { "temp", "file.txt" })] + // "temp" {workingDirectory}/temp/file.txt -> "file.txt" + [TestCase("temp", false, new string[] { "temp", "file.txt" }, true, true, new string[] { "file.txt" })] + // nonWorkDir/temp/ nonWorkDir/temp/file.txt -> "file.txt" + [TestCase("temp", true, new string[] { "temp", "file.txt" }, true, false, new string[] { "file.txt" })] + // (unset) /nonWorkDir/temp/file.txt -> "/nonWorkDir/temp/file.txt" + [TestCase("", true, new string[] { "temp", "file.txt" }, true, false, new string[] { "temp", "file.txt" })] + public void TestRootPathBehavior(string rootPath, bool setRootPathPrefix, string[] filePathSegments, bool setFilePathPrefix, bool setCurrentDirectory, string[] expectedFilePath) + { + // when overrideWorkingDirectory is false, assumption is that working directory is that of the assembly + // which is not the same as the temporary directory + + using (var workDir = new TempDir()) + using (var tarFileName = new TempFile()) + using (var extractDirectory = new TempDir()) + { + if (setCurrentDirectory) + { + Environment.CurrentDirectory = workDir; + } + + if (setRootPathPrefix) + { + rootPath = Path.Combine(Environment.CurrentDirectory, rootPath); + } + + string filePath = Path.Combine(filePathSegments); + + if (setFilePathPrefix) + { + filePath = Path.Combine(Environment.CurrentDirectory, filePath); + } + else + { + filePath = Path.Combine(workDir, filePath); + } + + //var expectDir = Path.Combine(workDir.FullName, "temp"); + //Directory.CreateDirectory(expectDir); + //var inputFile = Path.Combine(expectDir, "file.txt"); + Directory.CreateDirectory(Path.GetDirectoryName(filePath)); + File.WriteAllText(filePath, Guid.NewGuid().ToString()); + + // extract files under the given path + using (var tarFile = File.Open(tarFileName.FullName, FileMode.Create)) + { + using (var tarOutputStream = TarArchive.CreateOutputTarArchive(tarFile)) + { + if (rootPath != string.Empty) + { + tarOutputStream.RootPath = rootPath; + } + string assignedRootPath = tarOutputStream.RootPath; + + var entry = TarEntry.CreateEntryFromFile(filePath); + + // TarEntry.Name may be relative or absolute depending on if it + // was created under the CurrentDirectory + if (setCurrentDirectory) + { + Assert.AreEqual("temp/file.txt", entry.Name); + } + else + { + Assert.IsTrue(entry.Name.EndsWith("temp/file.txt")); + } + + tarOutputStream.WriteEntry(entry, true); + + // RootPath property does not change + Assert.AreEqual(assignedRootPath, tarOutputStream.RootPath); + } + } + + using (var file = File.OpenRead(tarFileName.FullName)) + { + using (var archive = TarArchive.CreateInputTarArchive(file, Encoding.UTF8)) + { + archive.ExtractContents(extractDirectory.FullName); + } + } + + // the resulting files must be the same as the expectation dir, should no longer have the "temp" prefix + var expectationDirectory = new DirectoryInfo(extractDirectory.FullName); + var expectedFile = expectationDirectory.GetFiles("", SearchOption.AllDirectories) + .First(); + + var expected = Path.Combine(extractDirectory.FullName, Path.Combine(expectedFilePath)); + + // the archive should contain the entry "testFile.txt", not "temp/testFile.txt", because + // the root dir is configured to "{CurrentDirectory}/tmp/" + // FileAssert.DoesNotExist(Path.Combine(extractDirectory.FullName, "temp", "testFile.txt")); + FileAssert.Exists(expected); // handle directory separators? + FileAssert.AreEqual(filePath, expected); + } + } } } From 063e14bdc8c04aa31b623f87d2bdaa0cf86fe49a Mon Sep 17 00:00:00 2001 From: Chris Johnston Date: Fri, 26 Aug 2022 12:52:15 -0700 Subject: [PATCH 2/3] lint: sort usings --- test/ICSharpCode.SharpZipLib.Tests/Tar/TarTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/ICSharpCode.SharpZipLib.Tests/Tar/TarTests.cs b/test/ICSharpCode.SharpZipLib.Tests/Tar/TarTests.cs index 651fcc214..49a6767b4 100644 --- a/test/ICSharpCode.SharpZipLib.Tests/Tar/TarTests.cs +++ b/test/ICSharpCode.SharpZipLib.Tests/Tar/TarTests.cs @@ -4,11 +4,11 @@ using NUnit.Framework; using System; using System.IO; +using System.Linq; using System.Text; using System.Threading; using System.Threading.Tasks; using NUnit.Framework.Internal; -using System.Linq; namespace ICSharpCode.SharpZipLib.Tests.Tar { From 5577f7447202a5a5be05d25d62a4723a4866fdc1 Mon Sep 17 00:00:00 2001 From: Chris Johnston Date: Fri, 26 Aug 2022 13:09:12 -0700 Subject: [PATCH 3/3] clean up test cases --- .../Tar/TarTests.cs | 74 +++++++++---------- 1 file changed, 35 insertions(+), 39 deletions(-) diff --git a/test/ICSharpCode.SharpZipLib.Tests/Tar/TarTests.cs b/test/ICSharpCode.SharpZipLib.Tests/Tar/TarTests.cs index 49a6767b4..e6236d382 100644 --- a/test/ICSharpCode.SharpZipLib.Tests/Tar/TarTests.cs +++ b/test/ICSharpCode.SharpZipLib.Tests/Tar/TarTests.cs @@ -920,27 +920,37 @@ public void RootPathIsRespected() /// /// This tests the behavior of the property on - /// extracted files. + /// which are created under the . /// - /// - /// - /// - /// - /// - /// + /// + /// The value to set to the property. + /// A value of will keep the property unset. + /// + /// + /// If true, prefixes the with the . + /// + /// + /// If true, changes the to point to the + /// temporary working directory where the test files are created. + /// + /// + /// The list of path segments to join, which represents the expeced path of + /// the file in the archive. + /// [Test] [Category("Tar")] // {workingDirectory}/temp {workingDirectory}/temp/file.txt -> "file.txt" - [TestCase("temp", true, new string[] { "temp", "file.txt" }, true, true, new string[] { "file.txt" })] + [TestCase("temp", true, true, new string[] { "file.txt" })] // (unset) {workingDirectory}/temp/file.txt -> "temp/file.txt" - [TestCase("", false, new string[] { "temp", "file.txt" }, true, true, new string[] { "temp", "file.txt" })] + [TestCase("", false, true, new string[] { "temp", "file.txt" })] // "temp" {workingDirectory}/temp/file.txt -> "file.txt" - [TestCase("temp", false, new string[] { "temp", "file.txt" }, true, true, new string[] { "file.txt" })] + [TestCase("temp", false, true, new string[] { "file.txt" })] // nonWorkDir/temp/ nonWorkDir/temp/file.txt -> "file.txt" - [TestCase("temp", true, new string[] { "temp", "file.txt" }, true, false, new string[] { "file.txt" })] + [TestCase("temp", true, false, new string[] { "file.txt" })] // (unset) /nonWorkDir/temp/file.txt -> "/nonWorkDir/temp/file.txt" - [TestCase("", true, new string[] { "temp", "file.txt" }, true, false, new string[] { "temp", "file.txt" })] - public void TestRootPathBehavior(string rootPath, bool setRootPathPrefix, string[] filePathSegments, bool setFilePathPrefix, bool setCurrentDirectory, string[] expectedFilePath) + // cannot test this case, it relies on the path of the temp dir + // [TestCase("", true, false, new string[] { "temp", "file.txt" })] + public void TestRootPathBehavior(string rootPath, bool setRootPathPrefix, bool setCurrentDirectory, string[] expectedFilePath) { // when overrideWorkingDirectory is false, assumption is that working directory is that of the assembly // which is not the same as the temporary directory @@ -956,27 +966,13 @@ public void TestRootPathBehavior(string rootPath, bool setRootPathPrefix, string if (setRootPathPrefix) { - rootPath = Path.Combine(Environment.CurrentDirectory, rootPath); + rootPath = Path.Combine(workDir, rootPath); } - string filePath = Path.Combine(filePathSegments); + string testFilePath = Path.Combine(workDir, "temp", "file.txt"); + Directory.CreateDirectory(Path.Combine(workDir, "temp")); + File.WriteAllText(testFilePath, Guid.NewGuid().ToString()); - if (setFilePathPrefix) - { - filePath = Path.Combine(Environment.CurrentDirectory, filePath); - } - else - { - filePath = Path.Combine(workDir, filePath); - } - - //var expectDir = Path.Combine(workDir.FullName, "temp"); - //Directory.CreateDirectory(expectDir); - //var inputFile = Path.Combine(expectDir, "file.txt"); - Directory.CreateDirectory(Path.GetDirectoryName(filePath)); - File.WriteAllText(filePath, Guid.NewGuid().ToString()); - - // extract files under the given path using (var tarFile = File.Open(tarFileName.FullName, FileMode.Create)) { using (var tarOutputStream = TarArchive.CreateOutputTarArchive(tarFile)) @@ -985,9 +981,10 @@ public void TestRootPathBehavior(string rootPath, bool setRootPathPrefix, string { tarOutputStream.RootPath = rootPath; } + string assignedRootPath = tarOutputStream.RootPath; - var entry = TarEntry.CreateEntryFromFile(filePath); + var entry = TarEntry.CreateEntryFromFile(testFilePath); // TarEntry.Name may be relative or absolute depending on if it // was created under the CurrentDirectory @@ -1015,18 +1012,17 @@ public void TestRootPathBehavior(string rootPath, bool setRootPathPrefix, string } } - // the resulting files must be the same as the expectation dir, should no longer have the "temp" prefix var expectationDirectory = new DirectoryInfo(extractDirectory.FullName); var expectedFile = expectationDirectory.GetFiles("", SearchOption.AllDirectories) - .First(); + .First(); // should only contain a single file var expected = Path.Combine(extractDirectory.FullName, Path.Combine(expectedFilePath)); - // the archive should contain the entry "testFile.txt", not "temp/testFile.txt", because - // the root dir is configured to "{CurrentDirectory}/tmp/" - // FileAssert.DoesNotExist(Path.Combine(extractDirectory.FullName, "temp", "testFile.txt")); - FileAssert.Exists(expected); // handle directory separators? - FileAssert.AreEqual(filePath, expected); + // the extraced files must contain either "temp/file.txt" or "file.txt" based on test inputs + FileAssert.Exists(expected); + + // contents of the input file must equal the extracted file + FileAssert.AreEqual(testFilePath, expected); } } }