diff --git a/src/Persistence.Tests/Compression/PaArchiveExtensionsExtractTests.cs b/src/Persistence.Tests/Compression/PaArchiveExtensionsExtractTests.cs new file mode 100644 index 00000000..529b877a --- /dev/null +++ b/src/Persistence.Tests/Compression/PaArchiveExtensionsExtractTests.cs @@ -0,0 +1,75 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.IO.Compression; +using Microsoft.Extensions.Logging; +using Microsoft.PowerPlatform.PowerApps.Persistence.Compression; + +namespace Persistence.Tests.Compression; + +[TestClass] +public class PaArchiveExtensionsExtractTests : TestBase +{ + [TestMethod] + [DataRow("file.txt")] + [DataRow("subdir/file.txt")] + [DataRow("a/b/c/file.txt")] + public void TryComputeAndValidateExtractToPath_WithValidPath_ReturnsTrue(string entryPathStr) + { + var destDir = CreateTestOutputFolder(); + var entryPath = new PaArchivePath(entryPathStr); + + PaArchiveExtensions.TryComputeAndValidateExtractToPathRelativeToDirectory(destDir, entryPath, out var validFullPath) + .Should().BeTrue(); + validFullPath.Should().NotBeNull(); + validFullPath.Should().StartWith(Path.GetFullPath(destDir) + Path.DirectorySeparatorChar); + validFullPath.Should().EndWithEquivalentOf(entryPath.FullName); + } + + [TestMethod] + [DataRow(@"foo\..\..\bar\file.txt")] + public void TryComputeAndValidateExtractToPath_WithPathTraversal_ReturnsFalse(string inputEntryPath) + { + // This simulates an entry path that bypassed PaArchivePath validation (e.g. via a crafted zip), + // resulting in a path that traverses outside the destination directory. +#pragma warning disable CS0618 // TestOnly_CreateInvalidPath is intentionally obsolete for test use only + var maliciousPath = PaArchivePath.TestOnly_CreateInvalidPath(inputEntryPath); +#pragma warning restore CS0618 + + var destDir = CreateTestOutputFolder(); + + PaArchiveExtensions.TryComputeAndValidateExtractToPathRelativeToDirectory(destDir, maliciousPath, out var validFullPath) + .Should().BeFalse(); + validFullPath.Should().BeNull(); + } + + [TestMethod] + [DataRow(@"foo\..\..\bar\file.txt")] + public void ComputeAndValidateExtractToPath_WithPathTraversal_ThrowsAndLogsError(string inputEntryPath) + { + // Arrange: create a PaArchive with a logger and an entry whose NormalizedPath bypasses + // PaArchivePath validation, simulating a crafted zip that slipped through. + using var stream = new MemoryStream(); + var capturingLogger = new CapturingLogger(); + using var paArchive = new PaArchive(stream, ZipArchiveMode.Create, leaveOpen: true, logger: capturingLogger); + +#pragma warning disable CS0618 // TestOnly methods are intentionally obsolete for test use only + var maliciousPath = PaArchivePath.TestOnly_CreateInvalidPath(inputEntryPath); +#pragma warning restore CS0618 + + // Create foux zip entry archive for the bad entry + var zipEntry = paArchive.InnerZipArchive.CreateEntry(inputEntryPath); + var entry = new PaArchiveEntry(paArchive, zipEntry, maliciousPath, skipValidation: true); + + var destDir = CreateTestOutputFolder(); + + // Act & Assert: extracting should throw IOException + FluentActions.Invoking(() => PaArchiveExtensions.ComputeAndValidateExtractToPathRelativeToDirectory(entry, destDir)) + .Should().Throw() + .WithMessage($"Extracting {nameof(PaArchiveEntry)} would have resulted in a file outside the specified destination directory."); + + // Assert: the log entry was recorded with Error level + capturingLogger.Entries.Should().ContainSingle(e => e.Level == LogLevel.Error) + .Which.Message.Should().Match($"Extracting {nameof(PaArchiveEntry)} would have resulted in a file outside the specified destination directory.*'{inputEntryPath}'*"); + } +} diff --git a/src/Persistence.Tests/MsappPacking/MsappPackingServiceTests.cs b/src/Persistence.Tests/MsappPacking/MsappPackingServiceTests.cs index 9f735b16..e90aad50 100644 --- a/src/Persistence.Tests/MsappPacking/MsappPackingServiceTests.cs +++ b/src/Persistence.Tests/MsappPacking/MsappPackingServiceTests.cs @@ -27,7 +27,7 @@ public class MsappPackingServiceTests : TestBase }; [TestMethod] - public void UnpackToDirectoryWithDefaultConfig() + public async Task UnpackToDirectoryWithDefaultConfig() { // Arrange var testDir = CreateTestOutputFolder(ensureEmpty: true); @@ -36,7 +36,7 @@ public void UnpackToDirectoryWithDefaultConfig() var service = new MsappPackingService(MsappArchiveFactory.Default, MsappReferenceArchiveFactory.Default); // Act: unpack with default config (only PaYamlSourceCode is unpacked to disk) - service.UnpackToDirectory(msappPath, unpackedDir); + await service.UnpackToDirectoryAsync(msappPath, unpackedDir); Directory.Exists(unpackedDir).Should().BeTrue("service should have created the output folder if it didn't already exist"); @@ -127,7 +127,7 @@ static MsappArchive OpenMsappWithHeader(string headerFileName) } [TestMethod] - public void PackFromMsappReferenceFile_RoundTrip_ProducesSameEntries() + public async Task PackFromMsappReferenceFile_RoundTrip_ProducesSameEntries() { // Arrange var testDir = CreateTestOutputFolder(ensureEmpty: true); @@ -137,7 +137,7 @@ public void PackFromMsappReferenceFile_RoundTrip_ProducesSameEntries() var service = new MsappPackingService(MsappArchiveFactory.Default, MsappReferenceArchiveFactory.Default); // Act: unpack then pack - service.UnpackToDirectory(msappPath, unpackedDir); + await service.UnpackToDirectoryAsync(msappPath, unpackedDir); var msaprPath = Path.Combine(unpackedDir, AlmTestAppMsaprName); service.PackFromMsappReferenceFile(msaprPath, repackedMsappPath, TestPackingClient); @@ -170,7 +170,7 @@ public void PackFromMsappReferenceFile_RoundTrip_ProducesSameEntries() } [TestMethod] - public void PackFromMsappReferenceFile_ThrowsWhenOutputExists_AndOverwriteIsFalse() + public async Task PackFromMsappReferenceFile_ThrowsWhenOutputExists_AndOverwriteIsFalse() { // Arrange var testDir = CreateTestOutputFolder(ensureEmpty: true); @@ -179,7 +179,7 @@ public void PackFromMsappReferenceFile_ThrowsWhenOutputExists_AndOverwriteIsFals var msappPath = Path.Combine("_TestData", "AlmApps", AlmTestApp_asManyEntitiesAsPossible); var service = new MsappPackingService(MsappArchiveFactory.Default, MsappReferenceArchiveFactory.Default); - service.UnpackToDirectory(msappPath, unpackedDir); + await service.UnpackToDirectoryAsync(msappPath, unpackedDir); var msaprPath = Path.Combine(unpackedDir, AlmTestAppMsaprName); // Create a file at the output path to simulate a conflict @@ -192,7 +192,7 @@ public void PackFromMsappReferenceFile_ThrowsWhenOutputExists_AndOverwriteIsFals } [TestMethod] - public void PackFromMsappReferenceFile_Overwrites_WhenOverwriteIsTrue() + public async Task PackFromMsappReferenceFile_Overwrites_WhenOverwriteIsTrue() { // Arrange var testDir = CreateTestOutputFolder(ensureEmpty: true); @@ -201,7 +201,7 @@ public void PackFromMsappReferenceFile_Overwrites_WhenOverwriteIsTrue() var msappPath = Path.Combine("_TestData", "AlmApps", AlmTestApp_asManyEntitiesAsPossible); var service = new MsappPackingService(MsappArchiveFactory.Default, MsappReferenceArchiveFactory.Default); - service.UnpackToDirectory(msappPath, unpackedDir); + await service.UnpackToDirectoryAsync(msappPath, unpackedDir); var msaprPath = Path.Combine(unpackedDir, AlmTestAppMsaprName); // Create a file at the output path @@ -216,7 +216,7 @@ public void PackFromMsappReferenceFile_Overwrites_WhenOverwriteIsTrue() } [TestMethod] - public void PackFromMsappReferenceFile_PreservesNonAsciiSrcFileNames() + public async Task PackFromMsappReferenceFile_PreservesNonAsciiSrcFileNames() { // Arrange: unpack, then add pa.yaml files with non-ASCII names var testDir = CreateTestOutputFolder(ensureEmpty: true); @@ -225,7 +225,7 @@ public void PackFromMsappReferenceFile_PreservesNonAsciiSrcFileNames() var msappPath = Path.Combine("_TestData", "AlmApps", AlmTestApp_asManyEntitiesAsPossible); var service = new MsappPackingService(MsappArchiveFactory.Default, MsappReferenceArchiveFactory.Default); - service.UnpackToDirectory(msappPath, unpackedDir); + await service.UnpackToDirectoryAsync(msappPath, unpackedDir); var srcDir = Path.Combine(unpackedDir, MsappLayoutConstants.DirectoryNames.Src); var nonAsciiFileNames = new[] @@ -252,7 +252,7 @@ public void PackFromMsappReferenceFile_PreservesNonAsciiSrcFileNames() } [TestMethod] - public void PackFromMsappReferenceFile_IgnoresNonPaYamlFileInSrc() + public async Task PackFromMsappReferenceFile_IgnoresNonPaYamlFileInSrc() { // Arrange var testDir = CreateTestOutputFolder(ensureEmpty: true); @@ -261,7 +261,7 @@ public void PackFromMsappReferenceFile_IgnoresNonPaYamlFileInSrc() var msappPath = Path.Combine("_TestData", "AlmApps", AlmTestApp_asManyEntitiesAsPossible); var service = new MsappPackingService(MsappArchiveFactory.Default, MsappReferenceArchiveFactory.Default); - service.UnpackToDirectory(msappPath, unpackedDir); + await service.UnpackToDirectoryAsync(msappPath, unpackedDir); // Add a non-.pa.yaml file to the Src folder File.WriteAllText(Path.Combine(unpackedDir, "Src", "notes.txt"), "This is not a pa.yaml file"); @@ -277,7 +277,7 @@ public void PackFromMsappReferenceFile_IgnoresNonPaYamlFileInSrc() } [TestMethod] - public void BuildPackInstructions_ProducesCorrectInstructions() + public async Task BuildPackInstructions_ProducesCorrectInstructions() { // Arrange: unpack to a temp dir, then verify BuildPackInstructions output var testDir = CreateTestOutputFolder(ensureEmpty: true); @@ -285,7 +285,7 @@ public void BuildPackInstructions_ProducesCorrectInstructions() var msappPath = Path.Combine("_TestData", "AlmApps", AlmTestApp_asManyEntitiesAsPossible); var service = new MsappPackingService(MsappArchiveFactory.Default, MsappReferenceArchiveFactory.Default); - service.UnpackToDirectory(msappPath, unpackedDir); + await service.UnpackToDirectoryAsync(msappPath, unpackedDir); var msaprPath = Path.Combine(unpackedDir, AlmTestAppMsaprName); using var msaprArchive = MsappReferenceArchiveFactory.Default.Open(msaprPath); diff --git a/src/Persistence.Tests/TestAssemblyInitializer.cs b/src/Persistence.Tests/TestAssemblyInitializer.cs new file mode 100644 index 00000000..ff124108 --- /dev/null +++ b/src/Persistence.Tests/TestAssemblyInitializer.cs @@ -0,0 +1,40 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.Diagnostics; + +namespace Persistence.Tests; + +/// +/// MSTest assembly-level initializer. Runs once before any tests in this assembly. +/// +[TestClass] +public static class TestAssemblyInitializer +{ + [AssemblyInitialize] + public static void AssemblyInitialize(TestContext _) + { + // Replace the default TraceListener (which shows a dialog or aborts) with one that + // throws an exception, so that Debug.Assert failures are surfaced as test failures. + Trace.Listeners.Clear(); + Trace.Listeners.Add(new ThrowingTraceListener()); + } + + /// + /// A that converts failures into + /// s, making them visible as test failures. + /// + private sealed class ThrowingTraceListener : TraceListener + { + public override void Write(string? message) { } + public override void WriteLine(string? message) { } + + public override void Fail(string? message, string? detailMessage) + { + var fullMessage = string.IsNullOrEmpty(detailMessage) + ? $"Debug.Assert failed: {message}" + : $"Debug.Assert failed: {message}{Environment.NewLine}{detailMessage}"; + throw new InvalidOperationException(fullMessage); + } + } +} diff --git a/src/Persistence/Compression/PaArchive.cs b/src/Persistence/Compression/PaArchive.cs index 2c9a11d2..b259ea01 100644 --- a/src/Persistence/Compression/PaArchive.cs +++ b/src/Persistence/Compression/PaArchive.cs @@ -58,6 +58,8 @@ public PaArchive( internal ZipArchive InnerZipArchive => _innerZipArchive; + internal ILogger? InnerLogger => _logger; + public ZipArchiveMode Mode => _innerZipArchive.Mode; protected bool IsDisposed => _isDisposed; diff --git a/src/Persistence/Compression/PaArchiveEntry.cs b/src/Persistence/Compression/PaArchiveEntry.cs index 9439f4d1..fe457eb0 100644 --- a/src/Persistence/Compression/PaArchiveEntry.cs +++ b/src/Persistence/Compression/PaArchiveEntry.cs @@ -9,14 +9,14 @@ namespace Microsoft.PowerPlatform.PowerApps.Persistence.Compression; public class PaArchiveEntry { - internal PaArchiveEntry(PaArchive paArchive, ZipArchiveEntry zipEntry, PaArchivePath normalizedPath) + internal PaArchiveEntry(PaArchive paArchive, ZipArchiveEntry zipEntry, PaArchivePath normalizedPath, bool skipValidation = false) { - Debug.Assert(zipEntry.Archive == paArchive.InnerZipArchive, "The underlying zip archives do not match."); - Debug.Assert( - PaArchivePath.TryParse(zipEntry.FullName, out var reparsedPath, out _) && normalizedPath.Equals(reparsedPath), + Debug.Assert(skipValidation || zipEntry.Archive == paArchive.InnerZipArchive, "The underlying zip archives do not match."); + Debug.Assert(skipValidation || + (PaArchivePath.TryParse(zipEntry.FullName, out var reparsedPath, out _) && normalizedPath.Equals(reparsedPath)), $"normalizedPath '{normalizedPath}' does not match the path parsed from zipEntry.FullName '{zipEntry.FullName}'."); - Debug.Assert(!normalizedPath.IsRoot, "PaArchiveEntry should never be created with a root path."); - Debug.Assert(!normalizedPath.IsDirectory, "PaArchiveEntry should never be created with a directory path."); + Debug.Assert(skipValidation || !normalizedPath.IsRoot, "PaArchiveEntry should never be created with a root path."); + Debug.Assert(skipValidation || !normalizedPath.IsDirectory, "PaArchiveEntry should never be created with a directory path."); PaArchive = paArchive; ZipEntry = zipEntry; diff --git a/src/Persistence/Compression/PaArchiveExtensions.Extract.cs b/src/Persistence/Compression/PaArchiveExtensions.Extract.cs new file mode 100644 index 00000000..390602ef --- /dev/null +++ b/src/Persistence/Compression/PaArchiveExtensions.Extract.cs @@ -0,0 +1,130 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.Diagnostics.CodeAnalysis; +using System.IO.Compression; +using System.Security.Cryptography; +using System.Text.Json; + +namespace Microsoft.PowerPlatform.PowerApps.Persistence.Compression; + +public static partial class PaArchiveExtensions +{ + public static void ExtractToFile(this PaArchiveEntry source, string destinationFileName, bool overwrite = false) + { + source.ZipEntry.ExtractToFile(destinationFileName, overwrite); + } + + /// + /// Extracts the archive to a target directory, preserving relative paths. + /// + /// + /// This method is protected against ZipSlip attacks. + /// + public static void ExtractToDirectory(this PaArchive source, string destinationDirectoryName, bool overwrite = false) + { + ArgumentNullException.ThrowIfNull(source); + + source.Entries.ExtractToDirectory(destinationDirectoryName, overwrite); + } + + /// + /// Extracts the selected archive entries to a target directory, preserving relative paths. + /// + /// + /// This method is protected against ZipSlip attacks. + /// + public static void ExtractToDirectory(this IEnumerable entries, string destinationDirectoryName, bool overwrite = false) + { + ArgumentNullException.ThrowIfNull(entries); + ArgumentNullException.ThrowIfNull(destinationDirectoryName); + + foreach (var entry in entries) + { + entry.ExtractRelativeToDirectory(destinationDirectoryName, overwrite); + } + } + + /// + /// Extracts the current entry to the target using the relative entry path as subdirectory.///
+ /// + /// The target destination directory path. This directory does not need to exist; as it will be created. + /// The effective extracted path occurs outside the directory. + /// + /// This method is protected against ZipSlip attacks. + /// + public static void ExtractRelativeToDirectory(this PaArchiveEntry source, string destinationDirectoryName, bool overwrite = false) + { + ArgumentNullException.ThrowIfNull(source); + ArgumentNullException.ThrowIfNull(destinationDirectoryName); + + var fileDestinationPath = ComputeAndValidateExtractToPathRelativeToDirectory(source, destinationDirectoryName); + + // PaArchiveEntry's should always only ever represent a file + Directory.CreateDirectory(Path.GetDirectoryName(fileDestinationPath)!); + source.ExtractToFile(fileDestinationPath, overwrite: overwrite); + } + + /// + /// Reusable utility for computing a final unzipped path is in the target directory; preventing ZipSlip attacks. + /// + /// The target directory into which the entry should be extracted. This directory does not need to exist, but will at the end of this call. + public static string ComputeAndValidateExtractToPathRelativeToDirectory(PaArchiveEntry source, string destinationDirectoryName) + { + ArgumentNullException.ThrowIfNull(source); + ArgumentNullException.ThrowIfNull(destinationDirectoryName); + + if (!TryComputeAndValidateExtractToPathRelativeToDirectory(destinationDirectoryName, source.NormalizedPath, out var validFullPath)) + { + // Log so we can query and fix PaArchivePath to detect these malicious paths + source.PaArchive.InnerLogger?.LogExtractingResultsInOutside(source.OriginalFullName, source.NormalizedPath); + throw new IOException($"Extracting {nameof(PaArchiveEntry)} would have resulted in a file outside the specified destination directory."); + } + else + { + return validFullPath; + } + } + + /// + /// Reusable utility for computing a final unzipped path is in the target directory; preventing ZipSlip attacks. + /// + /// + /// The target directory into which the entry should be extracted. + /// If a relative path, it will be relative to the current working directory. + /// This directory does not need to exist, but will at the end of this call. + /// + /// + /// When this method returns false, this value is null. + /// When this method returns true, this parameter has the value of the full path to the computed extraction file path. + /// + /// true if the computed path is valid to extract (and is contained in the target directory). otherwise, false, indicating the computed full path is invalid. + public static bool TryComputeAndValidateExtractToPathRelativeToDirectory(string destinationDirectoryName, PaArchivePath entryPath, [NotNullWhen(true)] out string? validFullPath) + { + ArgumentNullException.ThrowIfNull(destinationDirectoryName); + ArgumentNullException.ThrowIfNull(entryPath); + + // Note that this will give us a good DirectoryInfo even if destinationDirectoryName exists: + var di = Directory.CreateDirectory(destinationDirectoryName); + var destinationDirectoryFullPath = di.FullName; + if (!destinationDirectoryFullPath.EndsWith(Path.DirectorySeparatorChar)) + { + destinationDirectoryFullPath = $"{destinationDirectoryFullPath}{Path.DirectorySeparatorChar}"; + } + + // Note: PaArchiveEntry.FullName has already been sanitized of invalid chars (or all known platforms) due to PaArchivePath + validFullPath = Path.GetFullPath(Path.Combine(destinationDirectoryFullPath, entryPath.FullName)); + + // Catch ZipSlip: + // Note: PaArchive instances attempt to detect malicious entry paths via the validation in PaArchivePath. + // But it's possible that certain OS implementations and carefully crafted relative paths may have been missed. + // This check here ensures we catch any missed conditions by adding the recommended check for a ZipSlip condition. + if (!validFullPath.StartsWith(destinationDirectoryFullPath, StringComparison.OrdinalIgnoreCase)) + { + validFullPath = null; + return false; + } + + return true; + } +} diff --git a/src/Persistence/Compression/PaArchiveExtensions.ExtractAsync.cs b/src/Persistence/Compression/PaArchiveExtensions.ExtractAsync.cs new file mode 100644 index 00000000..da3e742b --- /dev/null +++ b/src/Persistence/Compression/PaArchiveExtensions.ExtractAsync.cs @@ -0,0 +1,71 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.IO.Compression; +using System.Security.Cryptography; +using System.Text.Json; + +namespace Microsoft.PowerPlatform.PowerApps.Persistence.Compression; + +public static partial class PaArchiveExtensions +{ + public static ValueTask ExtractToFileAsync(this PaArchiveEntry source, string destinationFileName, bool overwrite = false) + { +#if NET10_0_OR_GREATER + // When we support .net 10, use ExtractToFileAsync +#else + source.ZipEntry.ExtractToFile(destinationFileName, overwrite); + return ValueTask.CompletedTask; +#endif + } + + /// + /// Extracts the archive to a target directory, preserving relative paths. + /// + /// + /// This method is protected against ZipSlip attacks. + /// + public static async Task ExtractToDirectoryAsync(this PaArchive source, string destinationDirectoryName, bool overwrite = false) + { + ArgumentNullException.ThrowIfNull(source); + + await source.Entries.ExtractToDirectoryAsync(destinationDirectoryName, overwrite).ConfigureAwait(false); + } + + /// + /// Extracts the selected archive entries to a target directory, preserving relative paths. + /// + /// + /// This method is protected against ZipSlip attacks. + /// + public static async Task ExtractToDirectoryAsync(this IEnumerable entries, string destinationDirectoryName, bool overwrite = false) + { + ArgumentNullException.ThrowIfNull(entries); + ArgumentNullException.ThrowIfNull(destinationDirectoryName); + + foreach (var entry in entries) + { + await entry.ExtractRelativeToDirectoryAsync(destinationDirectoryName, overwrite).ConfigureAwait(false); + } + } + + /// + /// Extracts the current entry to the target using the relative entry path as subdirectory.///
+ /// + /// The target destination directory path. This directory does not need to exist; as it will be created. + /// The effective extracted path occurs outside the directory. + /// + /// This method is protected against ZipSlip attacks. + /// + public static async Task ExtractRelativeToDirectoryAsync(this PaArchiveEntry source, string destinationDirectoryName, bool overwrite = false) + { + ArgumentNullException.ThrowIfNull(source); + ArgumentNullException.ThrowIfNull(destinationDirectoryName); + + var fileDestinationPath = ComputeAndValidateExtractToPathRelativeToDirectory(source, destinationDirectoryName); + + // PaArchiveEntry's should always only ever represent a file + Directory.CreateDirectory(Path.GetDirectoryName(fileDestinationPath)!); + await source.ExtractToFileAsync(fileDestinationPath, overwrite: overwrite).ConfigureAwait(false); + } +} diff --git a/src/Persistence/Compression/PaArchiveExtensions.cs b/src/Persistence/Compression/PaArchiveExtensions.cs index 8b0247f9..b59403b6 100644 --- a/src/Persistence/Compression/PaArchiveExtensions.cs +++ b/src/Persistence/Compression/PaArchiveExtensions.cs @@ -7,23 +7,8 @@ namespace Microsoft.PowerPlatform.PowerApps.Persistence.Compression; -public static class PaArchiveExtensions +public static partial class PaArchiveExtensions { - public static void ExtractToFile(this PaArchiveEntry source, string destinationFileName, bool overwrite = false) - { - source.ZipEntry.ExtractToFile(destinationFileName, overwrite); - } - - public static ValueTask ExtractToFileAsync(this PaArchiveEntry source, string destinationFileName, bool overwrite = false) - { -#if NET10_0_OR_GREATER - // When we support .net 10, use ExtractToFileAsync -#else - source.ZipEntry.ExtractToFile(destinationFileName, overwrite); - return ValueTask.CompletedTask; -#endif - } - public static T DeserializeAsJson(this PaArchiveEntry entry, JsonSerializerOptions serializerOptions) where T : notnull { diff --git a/src/Persistence/Compression/PaArchivePath.cs b/src/Persistence/Compression/PaArchivePath.cs index 2f53dcf0..5d866e12 100644 --- a/src/Persistence/Compression/PaArchivePath.cs +++ b/src/Persistence/Compression/PaArchivePath.cs @@ -259,6 +259,27 @@ public static PaArchivePath ParseArgument(string? fullName, [CallerArgumentExpre return path; } + /// + /// This method should ONLY be used for unit tests. + /// It allows us to simulate creation of an instance of which has a characteristic which + /// could cause problems. + /// + [Obsolete("Only use within tests for this library")] + internal static PaArchivePath TestOnly_CreateInvalidPath(string invalidFullName) + { + ArgumentException.ThrowIfNullOrWhiteSpace(invalidFullName); + + if (TryValidatePath(invalidFullName, out _, out _, out _)) + throw new ArgumentException($"This method should only be used to create known invalid paths"); + + // At a minimum, normalize path separators to current platform: + invalidFullName = invalidFullName + .Replace(SeparatorCharUnix, Path.DirectorySeparatorChar) + .Replace(SeparatorCharWindows, Path.DirectorySeparatorChar); + + return new PaArchivePath(invalidFullName, Path.GetFileName(invalidFullName)); + } + private static bool TryValidatePath( string? origFullName, [NotNullWhen(true)] out string? validFullName, diff --git a/src/Persistence/MsappPacking/MsappPackingService.cs b/src/Persistence/MsappPacking/MsappPackingService.cs index 1c2415e5..ae611fc5 100644 --- a/src/Persistence/MsappPacking/MsappPackingService.cs +++ b/src/Persistence/MsappPacking/MsappPackingService.cs @@ -34,7 +34,7 @@ public sealed class MsappPackingService( /// public static readonly Version MinSupportedDocVersion = new(1, 348); - public void UnpackToDirectory( + public async Task UnpackToDirectoryAsync( string msappPath, string outputDirectory, bool overwriteOutput = false, @@ -110,21 +110,14 @@ public void UnpackToDirectory( var referenceCount = 0; foreach (var entryInstruction in entryInstructions) { - if (entryInstruction.UnpackToRelativePath is not null) + if (entryInstruction.InstructionType is MsappUnpackInstructionType.UnpackToRelativeDirectory) { - var targetPath = Path.GetFullPath(Path.Combine(outputDirectory, entryInstruction.UnpackToRelativePath)); - // REVIEW: the ZipArchiveEntry.FullName docs example indicates we should ensure the targetPath is actually still under the output path. - // This could be that the relative dest path was maliciously formed. - if (!targetPath.StartsWith(outputDirectoryWithTrailingSlash, StringComparison.Ordinal)) - throw new InvalidOperationException($"Malicious msapp entry path found with FullName '{entryInstruction.MsappEntry.FullName}'."); - - Directory.CreateDirectory(Path.GetDirectoryName(targetPath)!); - entryInstruction.MsappEntry.ExtractToFile(targetPath); - + await entryInstruction.MsappEntry.ExtractRelativeToDirectoryAsync(outputDirectory, overwrite: overwriteOutput).ConfigureAwait(false); extractedCount++; } - else if (entryInstruction.CopyToMsaprEntryPath is not null) + else if (entryInstruction.InstructionType is MsappUnpackInstructionType.CopyToMsapr) { + Debug.Assert(entryInstruction.CopyToMsaprEntryPath is not null); msaprArchive.AddEntryFrom(entryInstruction.CopyToMsaprEntryPath, entryInstruction.MsappEntry); referenceCount++; } @@ -183,15 +176,12 @@ internal static IEnumerable BuildUnpackInstructions var contentType = GetContentType(entry.NormalizedPath); if (contentType is MsappContentType.PaYamlSourceCode && options.EnablesContentType(MsappUnpackableContentType.PaYamlSourceCode)) { - yield return new(entry, contentType) - { - UnpackToRelativePath = entry.FullName, - }; + yield return new(entry, contentType, MsappUnpackInstructionType.UnpackToRelativeDirectory); } else { // Default to copy entry into msapr as is under the 'msapp' folder - yield return new(entry, contentType) + yield return new(entry, contentType, MsappUnpackInstructionType.CopyToMsapr) { CopyToMsaprEntryPath = MsappDirPath.Combine(entry.NormalizedPath), }; @@ -361,9 +351,14 @@ internal static IEnumerable BuildPackInstructions( } } -internal record MsappUnpackEntryInstruction(PaArchiveEntry MsappEntry, MsappContentType ContentType) +internal enum MsappUnpackInstructionType +{ + UnpackToRelativeDirectory, + CopyToMsapr +} + +internal record MsappUnpackEntryInstruction(PaArchiveEntry MsappEntry, MsappContentType ContentType, MsappUnpackInstructionType InstructionType) { - public string? UnpackToRelativePath { get; init; } public PaArchivePath? CopyToMsaprEntryPath { get; init; } } diff --git a/src/Persistence/PAPersistenceLog.cs b/src/Persistence/PAPersistenceLog.cs index c18d9832..4f19d710 100644 --- a/src/Persistence/PAPersistenceLog.cs +++ b/src/Persistence/PAPersistenceLog.cs @@ -28,4 +28,8 @@ internal static partial class PAPersistenceLog [LoggerMessage(EventId = 105, Level = LogLevel.Warning, Message = "An entry found in zip archive has an invalid or malicious path and will be ignored. InvalidReason: '{invalidReason}'; EntryFullName: '{entryFullName}';")] public static partial void LogInvalidPathEntryIgnored(this ILogger logger, string entryFullName, PaArchivePathInvalidReason invalidReason); + + [LoggerMessage(EventId = 106, Level = LogLevel.Error, + Message = $"Extracting {nameof(PaArchiveEntry)} would have resulted in a file outside the specified destination directory. EntryFullName: '{{entryFullName}}'; NormalizedPath: '{{normalizedPath}}';")] + public static partial void LogExtractingResultsInOutside(this ILogger logger, string entryFullName, PaArchivePath normalizedPath); }