From 6525724b41bf7888c9bf4e3cb4be280204a11ee5 Mon Sep 17 00:00:00 2001 From: Joe Mayo Date: Mon, 24 Jun 2024 17:14:18 -0700 Subject: [PATCH] address feedback --- .../MsApp/MsappArchiveSaveTests.cs | 12 +- .../MsApp/MsappArchiveTests.cs | 191 +++++++++++++++--- .../Persistence.Tests.csproj | 2 +- src/Persistence/MsApp/IMsappArchive.cs | 9 +- src/Persistence/MsApp/MsappArchive.cs | 126 +++++++----- src/Versions.props | 3 + 6 files changed, 258 insertions(+), 85 deletions(-) diff --git a/src/Persistence.Tests/MsApp/MsappArchiveSaveTests.cs b/src/Persistence.Tests/MsApp/MsappArchiveSaveTests.cs index ac884ef7..dc46bdaf 100644 --- a/src/Persistence.Tests/MsApp/MsappArchiveSaveTests.cs +++ b/src/Persistence.Tests/MsApp/MsappArchiveSaveTests.cs @@ -33,8 +33,6 @@ public void Msapp_ShouldSave_Screen(string screenName, string screenEntryName, s msappValidation.App.Should().BeNull(); msappValidation.CanonicalEntries.Count.Should().Be(2); msappValidation.DoesEntryExist(screenEntryName).Should().BeTrue(); - var screenEntry = msappValidation.CanonicalEntries[MsappArchive.NormalizePath(screenEntryName)]; - screenEntry.Should().NotBeNull(); using var streamReader = new StreamReader(msappValidation.GetRequiredEntry(screenEntryName).Open()); var yaml = streamReader.ReadToEnd().NormalizeNewlines(); var expectedYaml = File.ReadAllText(expectedYamlPath).NormalizeNewlines(); @@ -70,17 +68,15 @@ public void Msapp_ShouldSave_Screen_With_Control(string screenName, string contr msappValidation.CanonicalEntries.Count.Should().Be(2); // Validate screen - var screenEntry = msappValidation.CanonicalEntries[MsappArchive.NormalizePath(screenEntryName)]; - screenEntry.Should().NotBeNull(); + msappValidation.DoesEntryExist(screenEntryName).Should().BeTrue(); using var streamReader = new StreamReader(msappValidation.GetRequiredEntry(screenEntryName).Open()); var yaml = streamReader.ReadToEnd().NormalizeNewlines(); var expectedYaml = File.ReadAllText(expectedYamlPath).NormalizeNewlines(); yaml.Should().Be(expectedYaml); // Validate editor state - if (msappValidation.CanonicalEntries.TryGetValue(MsappArchive.NormalizePath(editorStateName), out var editorStateEntry)) + if (msappValidation.DoesEntryExist(editorStateName)) { - editorStateEntry.Should().NotBeNull(); using var editorStateReader = new StreamReader(msappValidation.GetRequiredEntry(editorStateName).Open()); var json = editorStateReader.ReadToEnd().ReplaceLineEndings(); var expectedJson = File.ReadAllText(expectedJsonPath).ReplaceLineEndings().TrimEnd(); @@ -113,7 +109,7 @@ public void Msapp_ShouldSave_App(string screenName) msappValidation.App!.Screens.Count.Should().Be(1); msappValidation.App.Screens.Single().Name.Should().Be(screenName); msappValidation.App.Name.Should().Be(App.ControlName); - msappValidation.CanonicalEntries.Keys.Should().Contain(MsappArchive.NormalizePath(MsappArchive.HeaderFileName)); + msappValidation.DoesEntryExist(MsappArchive.HeaderFileName).Should().BeTrue(); } [TestMethod] @@ -136,6 +132,6 @@ public void Msapp_ShouldSave_WithUniqueName() } msappArchive.CanonicalEntries.Count.Should().Be(sameNames.Length); - msappArchive.CanonicalEntries.Keys.Should().Contain(MsappArchive.NormalizePath(Path.Combine(MsappArchive.Directories.Src, @$"SameName{sameNames.Length + 1}{MsappArchive.YamlPaFileExtension}"))); + msappArchive.DoesEntryExist(Path.Combine(MsappArchive.Directories.Src, @$"SameName{sameNames.Length + 1}.pa.yaml")).Should().BeTrue(); } } diff --git a/src/Persistence.Tests/MsApp/MsappArchiveTests.cs b/src/Persistence.Tests/MsApp/MsappArchiveTests.cs index 7f1d7c35..ee6d47db 100644 --- a/src/Persistence.Tests/MsApp/MsappArchiveTests.cs +++ b/src/Persistence.Tests/MsApp/MsappArchiveTests.cs @@ -80,10 +80,7 @@ public void AddEntryTests(string[] entries, string[] expectedEntries) msappArchive.CanonicalEntries.Count.Should().Be(entries.Length); foreach (var expectedEntry in expectedEntries) { - msappArchive.CanonicalEntries.ContainsKey(expectedEntry) - .Should() - .BeTrue($"Expected entry {expectedEntry} to exist in the archive"); - msappArchive.DoesEntryExist(expectedEntry).Should().BeTrue(); + msappArchive.DoesEntryExist(expectedEntry).Should().BeTrue($"Expected entry {expectedEntry} to exist in the archive"); } // Get the required entry should throw if it doesn't exist @@ -141,6 +138,31 @@ public void Msapp_ShouldHave_Screens(string testDirectory, int allEntriesCount, var screen = msappArchive.App.Screens.Single(c => c.Name == topLevelControlName); } + [TestMethod] + public void ZipArchiveEntryPathTests() + { + using var stream = new MemoryStream(); + using (var zipArchiveWrite = new ZipArchive(stream, ZipArchiveMode.Create, leaveOpen: true)) + { + var entry = zipArchiveWrite.CreateEntry("dir/file1.txt"); + entry.Name.Should().Be("file1.txt"); + + zipArchiveWrite.CreateEntry("/dir/file2.txt"); + zipArchiveWrite.CreateEntry(@"\dir\file3.txt"); + entry = zipArchiveWrite.CreateEntry("dir/"); + entry.Name.Should().Be(""); + } + + stream.Position = 0; + using var zipArchiveRead = new ZipArchive(stream, ZipArchiveMode.Read); + zipArchiveRead.Entries.Select(e => e.FullName).Should().BeEquivalentTo([ + "dir/file1.txt", + "/dir/file2.txt", + @"\dir\file3.txt", + "dir/", + ], "ZipArchive entry paths are not normalized and assumed to be correct for the current OS"); + } + [TestMethod] public void DoesEntryExistTests() { @@ -193,7 +215,7 @@ public void DoesEntryExistWorksWithNewEntriesCreated() } [TestMethod] - public void TryGenerateUniqueEntryPathTests() + public void GenerateUniqueEntryPathTests() { // Setup test archive with a couple entries in it already using var archiveMemStream = new MemoryStream(); @@ -204,33 +226,91 @@ public void TryGenerateUniqueEntryPathTests() archive.CreateEntry("dir1/entryD.pa.yaml"); // - string? actualEntryPath; - archive.TryGenerateUniqueEntryPath(null, "entryA", ".pa.yaml", out actualEntryPath).Should().BeTrue(); - actualEntryPath.Should().Be("entryA1.pa.yaml"); - archive.TryGenerateUniqueEntryPath(null, "entryC", ".pa.yaml", out actualEntryPath).Should().BeTrue(); - actualEntryPath.Should().Be("entryC.pa.yaml"); - archive.TryGenerateUniqueEntryPath("dir1", "entryA", ".pa.yaml", out actualEntryPath).Should().BeTrue(); - actualEntryPath.Should().Be("dir1\\entryA.pa.yaml"); - archive.TryGenerateUniqueEntryPath("dir1", "entryC", ".pa.yaml", out actualEntryPath).Should().BeTrue(); - actualEntryPath.Should().Be("dir1\\entryC1.pa.yaml"); + archive.GenerateUniqueEntryPath(null, "entryA", ".pa.yaml").Should().Be("entryA1.pa.yaml"); + archive.GenerateUniqueEntryPath(null, "entryC", ".pa.yaml").Should().Be("entryC.pa.yaml"); + archive.GenerateUniqueEntryPath("dir1", "entryA", ".pa.yaml").Should().Be(Path.Combine("dir1", "entryA.pa.yaml")); + archive.GenerateUniqueEntryPath("dir1", "entryC", ".pa.yaml").Should().Be(Path.Combine("dir1", "entryC1.pa.yaml")); // Verify repeated calls will keep incrementing the suffix - archive.TryGenerateUniqueEntryPath(null, "entryA", ".pa.yaml", out actualEntryPath).Should().BeTrue(); - actualEntryPath.Should().Be("entryA1.pa.yaml"); + var actualEntryPath = archive.GenerateUniqueEntryPath(null, "entryA", ".pa.yaml").Should().Be("entryA1.pa.yaml").And.Subject; archive.CreateEntry(actualEntryPath!); - archive.TryGenerateUniqueEntryPath(null, "entryA", ".pa.yaml", out actualEntryPath).Should().BeTrue(); - actualEntryPath.Should().Be("entryA2.pa.yaml"); + actualEntryPath = archive.GenerateUniqueEntryPath(null, "entryA", ".pa.yaml").Should().Be("entryA2.pa.yaml").And.Subject; archive.CreateEntry(actualEntryPath!); - archive.TryGenerateUniqueEntryPath(null, "entryA", ".pa.yaml", out actualEntryPath).Should().BeTrue(); - actualEntryPath.Should().Be("entryA3.pa.yaml"); + actualEntryPath = archive.GenerateUniqueEntryPath(null, "entryA", ".pa.yaml").Should().Be("entryA3.pa.yaml").And.Subject; // Verify when using a custom separator - archive.TryGenerateUniqueEntryPath(null, "entryA", ".pa.yaml", out actualEntryPath, uniqueSuffixSeparator: "_").Should().BeTrue(); - actualEntryPath.Should().Be("entryA_1.pa.yaml"); - archive.TryGenerateUniqueEntryPath("dir1", "entryA", ".pa.yaml", out actualEntryPath, uniqueSuffixSeparator: "_").Should().BeTrue(); - actualEntryPath.Should().Be("dir1\\entryA.pa.yaml"); + archive.GenerateUniqueEntryPath(null, "entryA", ".pa.yaml", uniqueSuffixSeparator: "_").Should().Be("entryA_1.pa.yaml"); + archive.GenerateUniqueEntryPath("dir1", "entryA", ".pa.yaml", uniqueSuffixSeparator: "_").Should().Be(Path.Combine("dir1", "entryA.pa.yaml")); + } + + [TestMethod] + public void GenerateUniqueEntryPathReturnsNormalizedPathsTests() + { + // Setup test archive with a couple entries in it already + using var archiveMemStream = new MemoryStream(); + using var archive = new MsappArchive(archiveMemStream, ZipArchiveMode.Create, _mockYamlSerializationFactory.Object); + archive.CreateEntry("entryA.pa.yaml"); + archive.CreateEntry("dir1/entryA.pa.yaml"); + archive.CreateEntry("dir1/dir2/entryA.pa.yaml"); + + // when entry already unique + archive.GenerateUniqueEntryPath(null, "entryC", ".pa.yaml").Should().Be("entryC.pa.yaml"); + archive.GenerateUniqueEntryPath(@"/dir1\", "entryC", ".pa.yaml").Should().Be(Path.Combine("dir1", "entryC.pa.yaml")); + archive.GenerateUniqueEntryPath(@"\dir1/dir2\", "entryC", ".pa.yaml").Should().Be(Path.Combine("dir1", "dir2", "entryC.pa.yaml")); + + // when unique entry generated + archive.GenerateUniqueEntryPath(null, "entryA", ".pa.yaml").Should().Be("entryA1.pa.yaml"); + archive.GenerateUniqueEntryPath("dir1", "entryA", ".pa.yaml").Should().Be(Path.Combine("dir1", "entryA1.pa.yaml")); + archive.GenerateUniqueEntryPath(@"/dir1\", "entryA", ".pa.yaml").Should().Be(Path.Combine("dir1", "entryA1.pa.yaml")); + archive.GenerateUniqueEntryPath(@"\dir1/dir2\", "entryA", ".pa.yaml").Should().Be(Path.Combine("dir1", "dir2", "entryA1.pa.yaml")); + } + + [TestMethod] + public void NormalizeDirectoryEntryPathTests() + { + // Root paths: + MsappArchive.NormalizeDirectoryEntryPath(null).Should().Be(string.Empty, "Normalized directory entry paths are used to compose full paths, and we don't want to return null"); + MsappArchive.NormalizeDirectoryEntryPath(string.Empty).Should().Be(string.Empty, "Empty string should be returned as is"); + MsappArchive.NormalizeDirectoryEntryPath("/").Should().Be(string.Empty, "Root directory should be normalized to empty string"); + MsappArchive.NormalizeDirectoryEntryPath("\\").Should().Be(string.Empty, "Root directory should be normalized to empty string"); + + var expectedDir1 = $"dir1{Path.DirectorySeparatorChar}"; + var expectedDir1Dir2 = $"dir1{Path.DirectorySeparatorChar}dir2{Path.DirectorySeparatorChar}"; + + // Single directory: + MsappArchive.NormalizeDirectoryEntryPath(@"dir1").Should().Be(expectedDir1); + MsappArchive.NormalizeDirectoryEntryPath(@"dir1/").Should().Be(expectedDir1); + MsappArchive.NormalizeDirectoryEntryPath(@"/dir1").Should().Be(expectedDir1); + MsappArchive.NormalizeDirectoryEntryPath(@"/dir1/").Should().Be(expectedDir1); + MsappArchive.NormalizeDirectoryEntryPath(@"dir1\").Should().Be(expectedDir1); + MsappArchive.NormalizeDirectoryEntryPath(@"\dir1").Should().Be(expectedDir1); + MsappArchive.NormalizeDirectoryEntryPath(@"\dir1\").Should().Be(expectedDir1); + + // Multiple directories: + MsappArchive.NormalizeDirectoryEntryPath(@"dir1/dir2").Should().Be(expectedDir1Dir2); + MsappArchive.NormalizeDirectoryEntryPath(@"dir1/dir2/").Should().Be(expectedDir1Dir2); + MsappArchive.NormalizeDirectoryEntryPath(@"/dir1/dir2").Should().Be(expectedDir1Dir2); + MsappArchive.NormalizeDirectoryEntryPath(@"/dir1/dir2/").Should().Be(expectedDir1Dir2); + MsappArchive.NormalizeDirectoryEntryPath(@"dir1\dir2").Should().Be(expectedDir1Dir2); + MsappArchive.NormalizeDirectoryEntryPath(@"dir1\dir2\").Should().Be(expectedDir1Dir2); + MsappArchive.NormalizeDirectoryEntryPath(@"\dir1\dir2").Should().Be(expectedDir1Dir2); + MsappArchive.NormalizeDirectoryEntryPath(@"\dir1\dir2\").Should().Be(expectedDir1Dir2); + + // middle directory separator chars are consolidated to one: + MsappArchive.NormalizeDirectoryEntryPath(@"//dir1//").Should().Be(expectedDir1); + MsappArchive.NormalizeDirectoryEntryPath(@"\\dir1\\").Should().Be(expectedDir1); + MsappArchive.NormalizeDirectoryEntryPath(@"//dir1/dir2//").Should().Be(expectedDir1Dir2); + MsappArchive.NormalizeDirectoryEntryPath(@"\\dir1\dir2\\").Should().Be(expectedDir1Dir2); + MsappArchive.NormalizeDirectoryEntryPath(@"//dir1///dir2//").Should().Be(expectedDir1Dir2); + MsappArchive.NormalizeDirectoryEntryPath(@"\\dir1\\\dir2\\").Should().Be(expectedDir1Dir2); + MsappArchive.NormalizeDirectoryEntryPath(@"\/dir1/\dir2/\").Should().Be(expectedDir1Dir2); + + // When path segment names have leading/trailing whitespace, they are currently preserved: + MsappArchive.NormalizeDirectoryEntryPath(@" \/ dir1 /\ dir2 /\ ") + .Should().Be($" {Path.DirectorySeparatorChar} dir1 {Path.DirectorySeparatorChar} dir2 {Path.DirectorySeparatorChar} {Path.DirectorySeparatorChar}", + "currently, normalization doesn't 'trim' path segment names"); } [TestMethod] @@ -282,4 +362,67 @@ public void TryMakeSafeForEntryPathSegmentWithUnderscoreReplacementTests(string 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/Bar.pa.yaml").Should().BeFalse("separator chars should not be used for path segments"); + } + + [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.Tests/Persistence.Tests.csproj b/src/Persistence.Tests/Persistence.Tests.csproj index 6367c88b..15df682c 100644 --- a/src/Persistence.Tests/Persistence.Tests.csproj +++ b/src/Persistence.Tests/Persistence.Tests.csproj @@ -31,7 +31,7 @@ - + diff --git a/src/Persistence/MsApp/IMsappArchive.cs b/src/Persistence/MsApp/IMsappArchive.cs index d3728f8f..0e05f563 100644 --- a/src/Persistence/MsApp/IMsappArchive.cs +++ b/src/Persistence/MsApp/IMsappArchive.cs @@ -76,14 +76,13 @@ public interface IMsappArchive : IDisposable /// 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 . - /// A value indicating whether a unique entry was able to be created. If true, then will contain the unique path. + /// The entry path which is unique in the specified . /// directory is empty or whitespace. - bool TryGenerateUniqueEntryPath( + /// A unique filename entry could not be generated. + string GenerateUniqueEntryPath( string? directory, string fileNameNoExtension, string? extension, - [NotNullWhen(true)] out string? entryPath, string uniqueSuffixSeparator = ""); /// @@ -113,7 +112,7 @@ bool TryGenerateUniqueEntryPath( /// /// Dictionary of all entries in the archive. - /// The keys are normalized paths for the entry computed using . + /// The keys are normalized paths for the entry computed using . /// IReadOnlyDictionary CanonicalEntries { get; } diff --git a/src/Persistence/MsApp/MsappArchive.cs b/src/Persistence/MsApp/MsappArchive.cs index cdb11339..c8314dbd 100644 --- a/src/Persistence/MsApp/MsappArchive.cs +++ b/src/Persistence/MsApp/MsappArchive.cs @@ -144,7 +144,7 @@ public MsappArchive(Stream stream, ZipArchiveMode mode, bool leaveOpen, Encoding return canonicalEntries; foreach (var entry in ZipArchive.Entries) { - if (!canonicalEntries.TryAdd(NormalizePath(entry.FullName), entry)) + if (!canonicalEntries.TryAdd(CanonicalizePath(entry.FullName), entry)) _logger?.DuplicateEntry(entry.FullName); } return canonicalEntries; @@ -275,7 +275,7 @@ public bool DoesEntryExist(string entryPath) { _ = entryPath ?? throw new ArgumentNullException(nameof(entryPath)); - return CanonicalEntries.ContainsKey(NormalizePath(entryPath)); + return CanonicalEntries.ContainsKey(CanonicalizePath(entryPath)); } /// @@ -361,7 +361,7 @@ public T Deserialize(ZipArchiveEntry archiveEntry) where T : Control /// public IEnumerable GetDirectoryEntries(string directoryName, string? extension = null, bool recursive = true) { - directoryName = NormalizePath(directoryName).TrimEnd('/'); + directoryName = CanonicalizePath(directoryName).TrimEnd('/'); foreach (var entry in CanonicalEntries) { @@ -386,12 +386,7 @@ public IEnumerable GetDirectoryEntries(string directoryName, st /// public ZipArchiveEntry? GetEntry(string entryName) { - if (!TryGetEntry(entryName, out var entry)) - { - return null; - } - - return entry; + return TryGetEntry(entryName, out var entry) ? entry : null; } /// @@ -399,26 +394,21 @@ public bool TryGetEntry(string entryName, [MaybeNullWhen(false)] out ZipArchiveE { _ = entryName ?? throw new ArgumentNullException(nameof(entryName)); - zipArchiveEntry = null; - if (string.IsNullOrWhiteSpace(entryName)) + { + zipArchiveEntry = null; return false; + } - if (CanonicalEntries.TryGetValue(NormalizePath(entryName), out zipArchiveEntry)) - return true; - - return false; + return CanonicalEntries.TryGetValue(CanonicalizePath(entryName), out zipArchiveEntry); } /// public ZipArchiveEntry GetRequiredEntry(string entryName) { - if (!TryGetEntry(entryName, out var entry)) - { - throw new PersistenceLibraryException(PersistenceErrorCode.MsappArchiveError, $"Entry with name '{entryName}' not found in msapp archive."); - } - - return entry; + return TryGetEntry(entryName, out var entry) + ? entry + : throw new PersistenceLibraryException(PersistenceErrorCode.MsappArchiveError, $"Entry with name '{entryName}' not found in msapp archive."); } /// @@ -427,7 +417,7 @@ public ZipArchiveEntry CreateEntry(string entryName) if (string.IsNullOrWhiteSpace(entryName)) throw new ArgumentNullException(nameof(entryName)); - var canonicalEntryName = NormalizePath(entryName); + var canonicalEntryName = CanonicalizePath(entryName); if (_canonicalEntries.Value.ContainsKey(canonicalEntryName)) throw new InvalidOperationException($"Entry {entryName} already exists in the archive."); @@ -458,34 +448,36 @@ private string GetSafeEntryPath(string directory, string name, string extension) if (!TryMakeSafeForEntryPathSegment(name, out var safeName, unsafeCharReplacementText: "")) throw new ArgumentException("Control name is not valid.", nameof(name)); - if (!TryGenerateUniqueEntryPath(directory.WhiteSpaceToNull(), safeName, extension, out var entryPath)) - { - throw new InvalidOperationException("Failed to find a unique name for the control."); - } - - return entryPath; + return GenerateUniqueEntryPath(directory.WhiteSpaceToNull(), safeName, extension); } /// - public bool TryGenerateUniqueEntryPath( + public string GenerateUniqueEntryPath( string? directory, string fileNameNoExtension, string? extension, - [NotNullWhen(true)] out string? entryPath, string uniqueSuffixSeparator = "") { if (directory != null && string.IsNullOrWhiteSpace(directory)) { throw new ArgumentException("The directory can be null, but cannot be empty or whitespace only.", nameof(directory)); } - _ = !string.IsNullOrEmpty(fileNameNoExtension) ? fileNameNoExtension : throw new ArgumentNullException(nameof(fileNameNoExtension)); + _ = fileNameNoExtension ?? throw new ArgumentNullException(nameof(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)) + { + 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)); + } - var entryPathPrefix = directory == null ? fileNameNoExtension : Path.Combine(directory, fileNameNoExtension); + var entryPathPrefix = $"{NormalizeDirectoryEntryPath(directory)}{fileNameNoExtension}"; // First see if we can use the name as is - entryPath = $"{entryPathPrefix}{extension}"; + var entryPath = $"{entryPathPrefix}{extension}"; if (!DoesEntryExist(entryPath)) - return true; + return entryPath; // If file with the same name already exists, add a number to the end of the name entryPathPrefix += uniqueSuffixSeparator; @@ -493,11 +485,10 @@ public bool TryGenerateUniqueEntryPath( { entryPath = $"{entryPathPrefix}{i}{extension}"; if (!DoesEntryExist(entryPath)) - return true; + return entryPath; } - entryPath = null; - return false; + throw new InvalidOperationException("Failed to generate a unique name."); } public void Save() @@ -525,10 +516,10 @@ public void Save() } /// - /// Normalizes an entry path to a value used in the canonical entries dictionary (). + /// Canonicalizes an entry path to a value used in the canonical entries dictionary (). /// It removes leading and trailing slashes, converts backslashes to forward slashes, and makes the path lowercase. /// - public static string NormalizePath(string path) + public static string CanonicalizePath(string path) { if (string.IsNullOrWhiteSpace(path)) return string.Empty; @@ -536,6 +527,30 @@ public static string NormalizePath(string path) return path.Trim().Replace('\\', '/').TrimStart('/').ToLowerInvariant(); } + /// + /// Normalizes a directory path so it can be used as a prefix for entry paths.
+ /// Normalized directory paths are different than file entry paths, in that they end with the platform separator character.
+ /// Directory paths end with '/' unless they are at the root. + /// + /// Empty string for root directory or a path that ends with '/' for a sub-directory. + public static string NormalizeDirectoryEntryPath(string? directoryPath) + { + // we assume that each segment of the path is already valid (i.e. doesn't have leading/trailing whitespace) + // Callers should ensure they enter valid path segments. We can add additional validation if this becomes a problem. + + if (string.IsNullOrEmpty(directoryPath)) + return string.Empty; + + var normalizedPath = EntryPathDirectorySeparatorsRegex() + .Replace(directoryPath, Path.DirectorySeparatorChar.ToString()) + .Trim(Path.DirectorySeparatorChar); // we trim the ends of all '/' chars so that we can be sure to add only a single one at the end + + if (normalizedPath.Length == 0) + return string.Empty; + + return normalizedPath + Path.DirectorySeparatorChar; + } + /// /// 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. @@ -560,6 +575,21 @@ public static bool TryMakeSafeForEntryPathSegment( 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. + /// + /// 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) + { + _ = name ?? throw new ArgumentNullException(nameof(name)); + + return !string.IsNullOrWhiteSpace(name) + && !UnsafeFileNameCharactersRegex().IsMatch(name) + && name.Trim() == name; // No leading or trailing whitespace + } + /// /// 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. @@ -567,6 +597,12 @@ public static bool TryMakeSafeForEntryPathSegment( [GeneratedRegex("[^a-zA-Z0-9 ._-]")] private static partial Regex UnsafeFileNameCharactersRegex(); + /// + /// Matches the directory separators in an entry path. + /// + [GeneratedRegex(@"[/\\]+")] + private static partial Regex EntryPathDirectorySeparatorsRegex(); + #endregion #region Private Methods @@ -574,8 +610,7 @@ public static bool TryMakeSafeForEntryPathSegment( private App? LoadApp() { // For app entry name is always "App.pa.yaml" now - var appEntry = GetEntry(Path.Combine(Directories.Src, AppFileName)); - if (appEntry == null) + if (!TryGetEntry(Path.Combine(Directories.Src, AppFileName), out var appEntry)) return null; var app = Deserialize(appEntry.FullName, ensureRoundTrip: false); @@ -659,8 +694,7 @@ private Header LoadHeader() private AppProperties? LoadProperties() { - var entry = GetEntry(PropertiesFileName); - if (entry == null) + if (!TryGetEntry(PropertiesFileName, out var entry)) return null; var appProperties = DeserializeMsappJsonFile(entry); @@ -669,8 +703,7 @@ private Header LoadHeader() private DataSources? LoadDataSources() { - var entry = GetEntry(DataSourcesFileName); - if (entry == null) + if (!TryGetEntry(DataSourcesFileName, out var entry)) return null; var dataSources = DeserializeMsappJsonFile(entry); @@ -679,8 +712,7 @@ private Header LoadHeader() private Resources? LoadResources() { - var entry = GetEntry(ResourcesFileName); - if (entry == null) + if (!TryGetEntry(ResourcesFileName, out var entry)) return null; var resources = DeserializeMsappJsonFile(entry); diff --git a/src/Versions.props b/src/Versions.props index 8561b614..58267d2c 100644 --- a/src/Versions.props +++ b/src/Versions.props @@ -10,5 +10,8 @@ 6.12.0 4.3.0 7.0.0 + + + 4.16.0