diff --git a/src/Persistence.Tests/Compression/PaArchivePathTests.cs b/src/Persistence.Tests/Compression/PaArchivePathTests.cs index ec23ab77..638be0cf 100644 --- a/src/Persistence.Tests/Compression/PaArchivePathTests.cs +++ b/src/Persistence.Tests/Compression/PaArchivePathTests.cs @@ -161,12 +161,13 @@ public void ValidDirectoryPathTest(string fullPath, string expectedRelativePathW [DataRow(@"dir1\file3 \", PaArchivePathInvalidReason.SegmentWithLeadingOrTrailingWhitespace)] [DataRow(@"dir1\ file3 ", PaArchivePathInvalidReason.SegmentWithLeadingOrTrailingWhitespace)] // Relative directory segments are illegal (See: Zip Path Traversal vulnerability) - [DataRow(@"parent\..\dir", PaArchivePathInvalidReason.IllegalSegment)] - [DataRow(@"malicious\..\..\..\path", PaArchivePathInvalidReason.IllegalSegment)] - [DataRow("""..\..\..\Windows\System32\drivers\etc\hosts""", PaArchivePathInvalidReason.IllegalSegment)] - [DataRow(@"./current/dir", PaArchivePathInvalidReason.IllegalSegment)] - [DataRow(@"current/./dir", PaArchivePathInvalidReason.IllegalSegment)] - [DataRow(@"illegal/win/filename.", PaArchivePathInvalidReason.IllegalSegment)] // Windows: cannot end with a period + [DataRow(@"parent\..\dir", PaArchivePathInvalidReason.RelativeSegment)] + [DataRow(@"malicious\..\..\..\path", PaArchivePathInvalidReason.RelativeSegment)] + [DataRow("""..\..\..\Windows\System32\drivers\etc\hosts""", PaArchivePathInvalidReason.RelativeSegment)] + [DataRow(@"./current/dir", PaArchivePathInvalidReason.RelativeSegment)] + [DataRow(@"current/./dir", PaArchivePathInvalidReason.RelativeSegment)] + [DataRow(@"illegal/win/filename.", PaArchivePathInvalidReason.SegmentEndsWithDot)] // Windows: cannot end with a period + [DataRow(@"illegal/win/segment./filename", PaArchivePathInvalidReason.SegmentEndsWithDot)] // Windows: cannot end with a period // extended-length paths are not allowed: [DataRow("""\\?\some\long\file\path""", PaArchivePathInvalidReason.InvalidPathChars)] public void InvalidPathTest(string fullName, PaArchivePathInvalidReason expectedReason) @@ -189,6 +190,9 @@ public void GetInvalidPathCharsTest() invalidChars.Should().NotBeEmpty(); invalidChars.Should().Contain(Path.GetInvalidPathChars(), "because all chars returned by Path.GetInvalidPathChars for the current platform should be included"); invalidChars.Should().Contain(Path.GetInvalidFileNameChars().Where(c => !PaArchivePath.GetAllSeparatorChars().Contains(c)), "because all chars returned by Path.GetInvalidFileNameChars (except for separator chars) for the current platform should be included"); + invalidChars.Should().Contain('\b', "BS char should not be allowed"); + invalidChars.Should().Contain('\t', "HT char should not be allowed"); + invalidChars.Should().Contain('\x007F', "DEL char should not be allowed, even though the Path.GetInvalidPathChars doesn't have it"); PaArchivePath.GetInvalidPathChars().Should().NotBeSameAs(invalidChars, "because we should be returning a new array instance each time to prevent callers from modifying the cached array"); } @@ -202,6 +206,137 @@ public void GetAllSeparatorCharsTest() PaArchivePath.GetAllSeparatorChars().Should().NotBeSameAs(allSeparatorChars, "because we should be returning a new array instance each time to prevent callers from modifying the cached array"); } + [TestMethod] + public void GetInvalidSegmentCharsTest() + { + var invalidSegmentChars = PaArchivePath.GetInvalidSegmentChars(); + invalidSegmentChars.Should().NotBeEmpty(); + invalidSegmentChars.Should().Contain(PaArchivePath.GetInvalidPathChars(), "because all chars from GetInvalidPathChars should be included"); + invalidSegmentChars.Should().Contain(PaArchivePath.GetAllSeparatorChars(), "because separator chars delimit segments and are therefore not valid within a segment"); + invalidSegmentChars.Should().Contain(Path.GetInvalidPathChars(), "because all chars returned by Path.GetInvalidPathChars for the current platform should be included"); + invalidSegmentChars.Should().Contain(Path.GetInvalidFileNameChars(), "because all chars returned by Path.GetInvalidFileNameChars for the current platform should be included"); + PaArchivePath.GetInvalidSegmentChars().Should().NotBeSameAs(invalidSegmentChars, "because we should be returning a new array instance each time to prevent callers from modifying the cached array"); + } + + [TestMethod] + // Valid segments + [DataRow("file.txt")] + [DataRow("SomeName")] + [DataRow(".hidden")] + [DataRow("some-dir_with555.common.chars")] + // Whitespace only + [DataRow("", PaArchivePathInvalidReason.WhitespaceOnlySegment)] + [DataRow(" ", PaArchivePathInvalidReason.WhitespaceOnlySegment)] + [DataRow(" ", PaArchivePathInvalidReason.WhitespaceOnlySegment)] + [DataRow("\t", PaArchivePathInvalidReason.WhitespaceOnlySegment)] + // Invalid chars (including separator chars) + [DataRow("foo|", null)] + // Illegal segments + [DataRow(".", null)] + [DataRow("..", null)] + [DataRow("...", null)] + [DataRow(" . ", null)] + [DataRow(" .. ", null)] + [DataRow(" ... ", null)] + [DataRow("ends-with-dot.", "ends-with-dot")] + [DataRow("ends-with-dots...", "ends-with-dots")] + public void TryMakeValidSegmentTest(string segment, string? expectedValidSegment) + { + PaArchivePath.TryMakeValidSegment(segment, out var validSegment) + .Should().Be(expectedValidSegment is not null); + validSegment.Should().Be(expectedValidSegment); + } + + [TestMethod] + // Already valid (returned as-is) + [DataRow("file.txt", "file.txt")] + [DataRow(".hidden", ".hidden")] + [DataRow("foo- -bar.txt", "foo- -bar.txt")] + [DataRow("foo-éñü-bar.txt", "foo-éñü-bar.txt")] + [DataRow("foo-あア-bar.txt", "foo-あア-bar.txt")] + [DataRow("foo-中文-bar.txt", "foo-中文-bar.txt")] + // Invalid chars are replaced + [DataRow("foo|", "___")] + // Illegal segments + [DataRow(".", "_")] + [DataRow("..", "._")] + [DataRow("...", ".._")] + [DataRow(" . ", "_._")] + [DataRow(" .. ", "_.._")] + [DataRow(" ... ", "_..._")] + [DataRow("ends-with-dot.", "ends-with-dot_")] + [DataRow("ends-with-dots...", "ends-with-dots.._")] + public void TryMakeValidSegment_WithReplacementCharTest(string segment, string? expectedValidSegment) + { + PaArchivePath.TryMakeValidSegment(segment, out var validSegment, replacementChar: '_') + .Should().Be(expectedValidSegment is not null); + validSegment.Should().Be(expectedValidSegment); + } [TestMethod] public void GetHashCodeTest() { diff --git a/src/Persistence.Tests/MsApp/MsappArchiveTests.cs b/src/Persistence.Tests/MsApp/MsappArchiveTests.cs index a1d109f9..c9edf9af 100644 --- a/src/Persistence.Tests/MsApp/MsappArchiveTests.cs +++ b/src/Persistence.Tests/MsApp/MsappArchiveTests.cs @@ -133,122 +133,4 @@ private static void SaveNewMinMsappWithHeaderOnly(MemoryStream archiveStream, st var headerFilePath = Path.Combine("_TestData", "headers", headerFileName); writeToArchive.CreateEntryFromFile(headerFilePath, "Header.json"); } - - [TestMethod] - [DataRow(":%/\\?!", false, DisplayName = "Unsafe chars only")] - [DataRow(" :%/\\ ?! ", false, DisplayName = "Unsafe and whitespace chars only")] - [DataRow("", false, DisplayName = "empty string")] - [DataRow(" ", false, DisplayName = "whitespace chars only")] - [DataRow("Foo.Bar", true, "Foo.Bar")] - [DataRow(" Foo Bar ", true, "Foo Bar", DisplayName = "with leading/trailing whitespace")] - [DataRow("Foo:%/\\-?!Bar", true, "Foo-Bar")] - public void TryMakeSafeForEntryPathSegmentWithDefaultReplacementTests(string unsafeName, bool expectedReturn, string? expectedSafeName = null) - { - MsappArchive.TryMakeSafeForEntryPathSegment(unsafeName, out var safeName).Should().Be(expectedReturn); - if (expectedReturn) - { - safeName.ShouldNotBeNull(); - if (expectedSafeName != null) - { - safeName.Should().Be(expectedSafeName); - } - } - else - { - safeName.Should().BeNull(); - } - } - - [TestMethod] - [DataRow(":%/\\?!", true, "______", DisplayName = "Unsafe chars only")] - [DataRow(" :%/\\ ?! ", true, "____ __", DisplayName = "Unsafe and whitespace chars only")] - [DataRow("", false, DisplayName = "empty string")] - [DataRow(" ", false, DisplayName = "whitespace chars only")] - [DataRow("Foo.Bar", true, "Foo.Bar")] - [DataRow(" Foo Bar ", true, "Foo Bar", DisplayName = "with leading/trailing whitespace")] - [DataRow("Foo:%/\\-?!Bar", true, "Foo____-__Bar")] - public void TryMakeSafeForEntryPathSegmentWithUnderscoreReplacementTests(string unsafeName, bool expectedReturn, string? expectedSafeName = null) - { - MsappArchive.TryMakeSafeForEntryPathSegment(unsafeName, out var safeName, unsafeCharReplacementText: "_").Should().Be(expectedReturn); - if (expectedReturn) - { - safeName.ShouldNotBeNull(); - if (expectedSafeName != null) - { - safeName.Should().Be(expectedSafeName); - } - } - else - { - safeName.Should().BeNull(); - } - } - - [TestMethod] - public void TryMakeSafeForEntryPathSegmentWhereInputContainsPathSeparatorCharsTests() - { - MsappArchive.TryMakeSafeForEntryPathSegment("Foo\\Bar.pa.yaml", out var safeName).Should().BeTrue(); - safeName.Should().Be("FooBar.pa.yaml"); - MsappArchive.TryMakeSafeForEntryPathSegment("Foo/Bar.pa.yaml", out safeName).Should().BeTrue(); - safeName.Should().Be("FooBar.pa.yaml"); - - // with replacement - MsappArchive.TryMakeSafeForEntryPathSegment("Foo\\Bar.pa.yaml", out safeName, unsafeCharReplacementText: "_").Should().BeTrue(); - safeName.Should().Be("Foo_Bar.pa.yaml"); - MsappArchive.TryMakeSafeForEntryPathSegment("Foo/Bar.pa.yaml", out safeName, unsafeCharReplacementText: "-").Should().BeTrue(); - safeName.Should().Be("Foo-Bar.pa.yaml"); - } - - [TestMethod] - public void TryMakeSafeForEntryPathSegmentWhereInputContainsInvalidPathCharTests() - { - var invalidChars = Path.GetInvalidPathChars() - .Union(Path.GetInvalidFileNameChars()); - foreach (var c in invalidChars) - { - // Default behavior should remove invalid chars - MsappArchive.TryMakeSafeForEntryPathSegment($"Foo{c}Bar.pa.yaml", out var safeName).Should().BeTrue(); - safeName.Should().Be("FooBar.pa.yaml"); - - // Replacement char should be used for invalid chars - MsappArchive.TryMakeSafeForEntryPathSegment($"Foo{c}Bar.pa.yaml", out safeName, unsafeCharReplacementText: "_").Should().BeTrue(); - safeName.Should().Be("Foo_Bar.pa.yaml"); - - // When input results in only whitespace or empty, return value should be false - MsappArchive.TryMakeSafeForEntryPathSegment($"{c}", out _).Should().BeFalse("because safe segment is empty string"); - MsappArchive.TryMakeSafeForEntryPathSegment($" {c} ", out _).Should().BeFalse("because safe segment is whitespace"); - MsappArchive.TryMakeSafeForEntryPathSegment($"{c} {c}", out _).Should().BeFalse("because safe segment is whitespace"); - } - } - - [TestMethod] - public void IsSafeForEntryPathSegmentTests() - { - MsappArchive.IsSafeForEntryPathSegment("Foo.pa.yaml").Should().BeTrue(); - - // Path separator chars should not be used for path segments - MsappArchive.IsSafeForEntryPathSegment("Foo/Bar.pa.yaml").Should().BeFalse("separator chars should not be used for path segments"); - MsappArchive.IsSafeForEntryPathSegment("/Foo.pa.yaml").Should().BeFalse("separator chars should not be used for path segments"); - MsappArchive.IsSafeForEntryPathSegment("Foo\\Bar.pa.yaml").Should().BeFalse("separator chars should not be used for path segments"); - MsappArchive.IsSafeForEntryPathSegment("\\Foo.pa.yaml").Should().BeFalse("separator chars should not be used for path segments"); - - MsappArchive.IsSafeForEntryPathSegment("Foo/\t.pa.yaml").Should().BeFalse("control chars should not be allowed"); - - // Currently, chars outside of ascii range are not allowed - MsappArchive.IsSafeForEntryPathSegment("Foo/éñü.pa.yaml").Should().BeFalse("latin chars are currently not allowed"); - MsappArchive.IsSafeForEntryPathSegment("Foo/あア.pa.yaml").Should().BeFalse("Japanese chars are currently not allowed"); - MsappArchive.IsSafeForEntryPathSegment("Foo/中文.pa.yaml").Should().BeFalse("CJK chars are currently not allowed"); - } - - [TestMethod] - public void IsSafeForEntryPathSegmentShouldNotAllowInvalidPathCharsTests() - { - var invalidChars = Path.GetInvalidPathChars() - .Union(Path.GetInvalidFileNameChars()); - - foreach (var c in invalidChars) - { - MsappArchive.IsSafeForEntryPathSegment($"Foo{c}Bar.pa.yaml").Should().BeFalse($"Invalid char '{c}' should not be allowed for path segments"); - } - } } diff --git a/src/Persistence/Compression/PaArchivePath.cs b/src/Persistence/Compression/PaArchivePath.cs index aa28c42b..af5dabda 100644 --- a/src/Persistence/Compression/PaArchivePath.cs +++ b/src/Persistence/Compression/PaArchivePath.cs @@ -45,12 +45,13 @@ public partial class PaArchivePath : IEquatable, IEquatable public static char[] GetInvalidPathChars() => [ - // ASCII Control Chars: + // ASCII Control Chars (0x00-0x1F, 0x7F): '\0', // NULL (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, + (char)127, // DEL // non-control chars not allowed in Windows filenames are added here: // We are explicitly only wanting to support valid relative paths, so we don't allow ':' for drive letters etc. @@ -58,10 +59,17 @@ public static char[] GetInvalidPathChars() => [ ':', '*', '?', ]; + /// + /// Characters that are invalid for use within a single path segment (file or directory name) in a . + /// + public static char[] GetInvalidSegmentChars() => [.. GetInvalidPathChars(), .. GetAllSeparatorChars()]; + #if NET8_0_OR_GREATER private static readonly System.Buffers.SearchValues _invalidPathChars = System.Buffers.SearchValues.Create(GetInvalidPathChars()); + private static readonly System.Buffers.SearchValues _invalidSegmentChars = System.Buffers.SearchValues.Create(GetInvalidSegmentChars()); #else private static readonly char[] _invalidPathChars = GetInvalidPathChars(); + private static readonly char[] _invalidSegmentChars = GetInvalidSegmentChars(); #endif /// @@ -228,7 +236,8 @@ public static string GetInvalidReasonExceptionMessage(PaArchivePathInvalidReason PaArchivePathInvalidReason.InvalidPathChars => $"The path contains invalid characters. Example invalid chars are ASCII control characters and {string.Join(string.Empty, GetInvalidPathChars().Where(static c => !Char.IsControl(c)))}.", PaArchivePathInvalidReason.WhitespaceOnlySegment => "The path contains a segment that is whitespace only, which is not allowed.", PaArchivePathInvalidReason.SegmentWithLeadingOrTrailingWhitespace => "The path contains a segment with leading or trailing whitespace, which is not allowed.", - PaArchivePathInvalidReason.IllegalSegment => "The path contains an illegal segment (e.g. '.' or '..'), which is not allowed.", + PaArchivePathInvalidReason.RelativeSegment => "The path contains an illegal relative segment (e.g. '.' or '..'), which is not allowed.", + PaArchivePathInvalidReason.SegmentEndsWithDot => "The path contains a segment that ends with a period ('.'), which is not allowed.", _ => $"The path is invalid. (Reason: {reason})" }; } @@ -339,17 +348,221 @@ private static bool TryValidatePath( { foreach (var segment in segments) { - if (string.IsNullOrWhiteSpace(segment)) - return PaArchivePathInvalidReason.WhitespaceOnlySegment; - if (segment == ".." || segment == "." - // In Windows, a filename cannot end with a period - || segment[^1] == '.') - return PaArchivePathInvalidReason.IllegalSegment; - if (segment.Trim().Length != segment.Length) - return PaArchivePathInvalidReason.SegmentWithLeadingOrTrailingWhitespace; + if (!TryValidateSegment(segment, out var reason2)) + return reason2; } return null; } } + + /// + /// Determines whether a single path segment (file or directory name) is valid for use in a . + /// + /// The path segment to validate. + /// true when the segment is valid; otherwise false. + public static bool IsValidSegment(string segment) + { + return TryValidateSegment(segment, out _); + } + + /// + /// Validates that a single path segment (file or directory name) is valid for use in a . + /// + /// The path segment to validate. + /// When this method returns false, contains the reason why the segment is invalid; otherwise null. + /// true when the segment is valid; otherwise false. + public static bool TryValidateSegment(string segment, [NotNullWhen(false)] out PaArchivePathInvalidReason? reason) + { + ThrowIfNull(segment); + + return TryValidateSegment(segment.AsSpan(), out reason); + } + + /// + /// Validates that a single path segment (file or directory name) is valid for use in a . + /// + /// The path segment to validate. + /// When this method returns false, contains the reason why the segment is invalid; otherwise null. + /// true when the segment is valid; otherwise false. + public static bool TryValidateSegment(ReadOnlySpan segment, [NotNullWhen(false)] out PaArchivePathInvalidReason? reason) + { + if (segment.IsWhiteSpace()) + { + reason = PaArchivePathInvalidReason.WhitespaceOnlySegment; + return false; + } + + if (segment.IndexOfAny(_invalidSegmentChars) >= 0) + { + reason = PaArchivePathInvalidReason.InvalidPathChars; + return false; + } + + if (segment.Trim().Length != segment.Length) + { + reason = PaArchivePathInvalidReason.SegmentWithLeadingOrTrailingWhitespace; + return false; + } + + // Detect relative segments + if (segment.SequenceEqual("..".AsSpan()) || segment.SequenceEqual(".".AsSpan())) + { + reason = PaArchivePathInvalidReason.RelativeSegment; + return false; + } + + // In Windows, a filename cannot end with a period + if (segment[^1] == '.') + { + reason = PaArchivePathInvalidReason.SegmentEndsWithDot; + return false; + } + + // REVIEW: Should we also limit path segments to 255 chars long? + + reason = null; + return true; + } + + /// + /// Attempts to make a path segment valid for use in a by removing invalid chars or problematic segments. + /// + /// The segment to make valid. + /// When this method returns true, contains the valid segment. + /// true when the segment was converted to a valid segment; otherwise false. + public static bool TryMakeValidSegment(string segment, [NotNullWhen(true)] out string? validSegment) + { + ThrowIfNull(segment); + + return TryMakeValidSegmentCore(segment, out validSegment, createProposedSegment: (segmentSpan, buffer) => + { + var writtenLen = 0; + foreach (var c in segmentSpan) + { + if (IsValidSegmentChar(c)) + buffer[writtenLen++] = c; + } + + // Slice to written length, then Trim returns another slice (no allocation) + var proposed = ((ReadOnlySpan)buffer[..writtenLen]) + .TrimStart(); + + // Trip trailing whitespace and '.' + var lenToKeep = proposed.Length; + for (int i = proposed.Length - 1; i >= 0 && (proposed[i] == '.' || char.IsWhiteSpace(proposed[i])); i--) + { + lenToKeep = i; + } + + return proposed[..lenToKeep]; + }); + } + + /// + /// Attempts to make a path segment valid for use in a by replacing invalid characters. + /// + /// The segment to make valid. + /// When this method returns true, contains the valid segment. + /// Invalid characters will be replaced with this character. + /// true when the segment was converted to a valid segment; otherwise false. + public static bool TryMakeValidSegment(string segment, [NotNullWhen(true)] out string? validSegment, char replacementChar) + { + ThrowIfNull(segment); + + if (char.IsWhiteSpace((char)replacementChar)) + throw new ArgumentException("Replacement must not be a whitespace character.", nameof(replacementChar)); + if (IsInvalidSegmentChar(replacementChar)) + throw new ArgumentException("Replacement must be a valid segment character.", nameof(replacementChar)); + if (replacementChar == '.') + throw new ArgumentException("Replacement must not be a period ('.').", nameof(replacementChar)); + + return TryMakeValidSegmentCore(segment, out validSegment, createProposedSegment: (segmentSpan, buffer) => + { + // We know we'll have a string that's the same length as the output + // Set now, so we'll get some runtime validation if indexes get out of range. + var segmentLen = segmentSpan.Length; + buffer = buffer[..segmentLen]; + + // Replace trailing whitespace with replacementChar + var firstTrailingWhitespaceIdx = segmentLen; + for (int i = segmentLen - 1; i >= 0 && char.IsWhiteSpace(segmentSpan[i]); i--) + { + buffer[i] = replacementChar; + firstTrailingWhitespaceIdx = i; + } + + var lastLeadingWhitespaceIdx = -1; + for (int i = 0; i < firstTrailingWhitespaceIdx && char.IsWhiteSpace(segmentSpan[i]); i++) + { + buffer[i] = replacementChar; + lastLeadingWhitespaceIdx = i; + } + + // replace invalid chars not covered by leading/trailing whitespace + for (int i = lastLeadingWhitespaceIdx + 1; i < firstTrailingWhitespaceIdx; i++) + { + var c = segmentSpan[i]; + buffer[i] = IsInvalidSegmentChar(c) ? replacementChar : c; + } + + // Handle segment ending with a '.' + if (segmentSpan[^1] == '.') + buffer[^1] = replacementChar; + + // return the full buffer + return buffer; + }); + } + + private delegate ReadOnlySpan CreateProposedSegment(ReadOnlySpan segmentSpan, Span buffer); + + private static bool TryMakeValidSegmentCore(string segment, [NotNullWhen(true)] out string? validSegment, CreateProposedSegment createProposedSegment) + { + var segmentSpan = segment.AsSpan(); + if (TryValidateSegment(segmentSpan, out _)) + { + validSegment = segment; + return true; + } + + // Trivially handle when input segment is empty, as we can't fix it + if (segmentSpan.Length == 0) + { + validSegment = null; + return false; + } + + // Filter invalid chars into a stack buffer (fall back to pooled array for large inputs) + const int StackThreshold = 256; + char[]? rented = null; + Span buffer = segmentSpan.Length <= StackThreshold + ? stackalloc char[StackThreshold] + : (rented = System.Buffers.ArrayPool.Shared.Rent(segmentSpan.Length)); + + try + { + var proposed = createProposedSegment(segmentSpan, buffer); + + // If we're left with invalid segment still, then don't return anything + if (TryValidateSegment(proposed, out _)) + { + validSegment = proposed.ToString(); + return true; + } + else + { + validSegment = null; + return false; + } + } + finally + { + if (rented is not null) + System.Buffers.ArrayPool.Shared.Return(rented); + } + } + + private static bool IsValidSegmentChar(char c) => !_invalidSegmentChars.Contains(c); + private static bool IsInvalidSegmentChar(char c) => _invalidSegmentChars.Contains(c); } diff --git a/src/Persistence/Compression/PaArchivePathInvalidReason.cs b/src/Persistence/Compression/PaArchivePathInvalidReason.cs index ed03cdd8..c80589ac 100644 --- a/src/Persistence/Compression/PaArchivePathInvalidReason.cs +++ b/src/Persistence/Compression/PaArchivePathInvalidReason.cs @@ -12,5 +12,12 @@ public enum PaArchivePathInvalidReason InvalidPathChars, WhitespaceOnlySegment, SegmentWithLeadingOrTrailingWhitespace, - IllegalSegment, + /// + /// Relative segments like "." or ".." are not allowed, as they can lead to confusion and potential security issues when extracting archives. + /// + RelativeSegment, + /// + /// On Windows, filenames cannot end with ".". So we exclude on all platforms. + /// + SegmentEndsWithDot, } diff --git a/src/Persistence/Microsoft.PowerPlatform.PowerApps.Persistence.csproj b/src/Persistence/Microsoft.PowerPlatform.PowerApps.Persistence.csproj index 7af57930..64af08f0 100644 --- a/src/Persistence/Microsoft.PowerPlatform.PowerApps.Persistence.csproj +++ b/src/Persistence/Microsoft.PowerPlatform.PowerApps.Persistence.csproj @@ -8,6 +8,7 @@ true true + true diff --git a/src/Persistence/MsApp/IMsappArchive.cs b/src/Persistence/MsApp/IMsappArchive.cs deleted file mode 100644 index 4d8bbedf..00000000 --- a/src/Persistence/MsApp/IMsappArchive.cs +++ /dev/null @@ -1,41 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. - -using Microsoft.PowerPlatform.PowerApps.Persistence.Compression; -using System.Diagnostics.CodeAnalysis; -using System.IO.Compression; - -namespace Microsoft.PowerPlatform.PowerApps.Persistence.MsApp; - -/// -/// base interface for MsappArchive -/// -public interface IMsappArchive : IPaArchive, IDisposable -{ - Version MSAppStructureVersion { get; } - - Version DocVersion { get; } - - /// - /// Provides access to the underlying zip archive. - /// Attention: This property might be removed in the future. - /// - [Obsolete("We shouldn't expose the underlying ZipArchive instance, as modifying it will make this instance inconsistent")] - ZipArchive ZipArchive { get; } - - /// - /// Attempts to generate a unique entry path in the specified directory, based on a starter file name and extension. - /// - /// The directory path for the entry; or null. Note: Directory path is expected to already be safe, as it is expected to not contain customer data. - /// The name of the file, without an extension. This file name should already have been made 'safe' by the caller. If needed, use to make it safe. - /// The file extension to add for the file path or null for no extension. Note: This should already contain only safe chars, as it is expected to not contain customer data. - /// The string added just before the unique file number and extension. Default is empty string. - /// The entry path which is unique in the specified . - /// directory is empty or whitespace. - /// A unique filename entry could not be generated. - string GenerateUniqueEntryPath( - string? directory, - string fileNameNoExtension, - string? extension, - string uniqueSuffixSeparator = ""); -} diff --git a/src/Persistence/MsApp/MsappArchive.cs b/src/Persistence/MsApp/MsappArchive.cs index cbb6a0a4..68b95f7a 100644 --- a/src/Persistence/MsApp/MsappArchive.cs +++ b/src/Persistence/MsApp/MsappArchive.cs @@ -19,20 +19,17 @@ namespace Microsoft.PowerPlatform.PowerApps.Persistence.MsApp; /// Represents a .msapp file. /// /// true to leave the stream open after the System.IO.Compression.ZipArchive object is disposed; otherwise, false -public partial class MsappArchive( +public class MsappArchive( Stream stream, ZipArchiveMode mode, bool leaveOpen = false, ILogger? logger = null) - : PaArchive(stream, mode, leaveOpen, entryNameEncoding: null, logger), IMsappArchive + : PaArchive(stream, mode, leaveOpen, entryNameEncoding: null, logger) { private HeaderJson? _header; private bool _packedJsonLoaded; private PackedJson? _packedJson; - /// - public ZipArchive ZipArchive => InnerZipArchive; - internal HeaderJson Header => _header ??= LoadHeader(); /// @@ -55,13 +52,10 @@ public string GenerateUniqueEntryPath( { var directoryPath = PaArchivePath.AsDirectoryOrRoot(directory); ThrowIfNull(fileNameNoExtension); - if (!IsSafeForEntryPathSegment(fileNameNoExtension)) - { - throw new ArgumentException($"The {nameof(fileNameNoExtension)} must be safe for use as an entry path segment. Prevalidate using {nameof(TryMakeSafeForEntryPathSegment)} first.", nameof(fileNameNoExtension)); - } - if (extension != null && !IsSafeForEntryPathSegment(extension)) + + if (!PaArchivePath.IsValidSegment($"{fileNameNoExtension}{extension}")) { - throw new ArgumentException("The extension can be null, but cannot be empty or whitespace only, and must be a valid entry path segment.", nameof(directory)); + throw new ArgumentException($"The {nameof(fileNameNoExtension)} combined with {nameof(extension)} must be safe for use as an entry path segment. Prevalidate using {nameof(PaArchivePath)}.{nameof(PaArchivePath.TryMakeValidSegment)} first.", nameof(fileNameNoExtension)); } var entryPathPrefix = $"{directoryPath.FullName}{fileNameNoExtension}"; @@ -96,57 +90,4 @@ private HeaderJson LoadHeader() _packedJsonLoaded = true; return _packedJson; } - - /// - /// Regular expression that matches any characters that are unsafe for entry filenames.
- /// Note: we don't allow any sort of directory separator chars for filenames to remove cross-platform issues. - /// -#if NET7_0_OR_GREATER - [GeneratedRegex("[^a-zA-Z0-9 ._-]")] - private static partial Regex UnsafeFileNameCharactersRegex(); -#else - private static readonly Regex s_unsafeFileNameCharactersRegex = new("[^a-zA-Z0-9 ._-]", RegexOptions.Compiled); - private static Regex UnsafeFileNameCharactersRegex() => s_unsafeFileNameCharactersRegex; -#endif - - /// - /// Makes a user-provided name safe for use as an entry path segment in the archive. - /// After making the name safe, it will be trimmed and empty strings will result in a false return value.
- /// Note: The set of allowed chars is currently more limited than what is actually allowed in a . - /// - /// An unsafe name which may contain invalid chars for usage in an entry path segment (e.g. directory name or file name). - /// Unsafe characters in the name will be replaced with this string. Default is empty string. - /// true, when was converted to a safe, non-empty string; otherwise, false indicates that input could not be turned into a safe, non-empty string. - public static bool TryMakeSafeForEntryPathSegment( - string unsafeName, - [NotNullWhen(true)] - out string? safeName, - string unsafeCharReplacementText = "") - { - ThrowIfNull(unsafeName); - ThrowIfNull(unsafeCharReplacementText); - - safeName = UnsafeFileNameCharactersRegex() - .Replace(unsafeName, unsafeCharReplacementText) - .Trim() - .EmptyToNull(); - - return safeName != null; - } - - /// - /// Used to verify that a name is safe for use as a single path segment for an entry. - /// Directory separator chars are not allowed in a path segment.
- /// Note: The set of allowed chars is currently more limited than what is actually allowed in a . - /// - /// The proposed path segment name. - /// false when is null, empty, whitespace only, has leading or trailing whitespace, contains path separator chars or contains any other invalid chars. - public static bool IsSafeForEntryPathSegment(string name) - { - ThrowIfNull(name); - - return !string.IsNullOrWhiteSpace(name) - && !UnsafeFileNameCharactersRegex().IsMatch(name) - && name.Trim().Length == name.Length; // No leading or trailing whitespace - } }