Fix for the Gallery unable to find license file if paths with backslashes are used (#6739)

* Fix for the Gallery unable to find license file if paths with backslashes are used.

* Adopted the client way of the file name mangling for license file access. Added logging in case mangling changes the file name.

Author: agr

src/NuGetGallery.Core/Services/CoreLicenseFileService.cs

@@ -72,7 +72,7 @@ public async Task ExtractAndSaveLicenseFileAsync(Package package, Stream package
                     throw new InvalidOperationException("No license file specified in the nuspec");
                 }
 
-                var filename = packageMetadata.LicenseMetadata.License;
+                var filename = FileNameHelper.GetZipEntryPath(packageMetadata.LicenseMetadata.License);
                 var licenseFileEntry = packageArchiveReader.GetEntry(filename); // throws on non-existent file
                 using (var licenseFileStream = licenseFileEntry.Open())
                 {

src/NuGetGallery.Core/Services/FIleNameHelper.cs

@@ -3,6 +3,8 @@
 
 using System;
 using System.Globalization;
+using System.IO;
+using NuGet.Common;
 using NuGet.Services.Entities;
 
 namespace NuGetGallery
@@ -57,5 +59,27 @@ public static string BuildFileName(string id, string version, string pathTemplat
                 version.ToLowerInvariant(),
                 extension);
         }
+
+        /// <summary>
+        /// Enforces the correct file separators for passing paths to work with zip file entries.
+        /// </summary>
+        /// <remarks>
+        /// When client packs the nupkg, it enforces all zip file entries to use forward slashes 
+        /// and relative paths.
+        /// At the same time, paths in nuspec can contain backslashes and start with dot. This
+        /// method fixes the separators so those paths can be used to retrieve files from zip
+        /// archive.
+        /// </remarks>
+        /// <param name="fileName">File name to fix.</param>
+        /// <returns>File path with proper path separators.</returns>
+        public static string GetZipEntryPath(string filePath)
+        {
+            if (filePath == null)
+            {
+                throw new ArgumentNullException(nameof(filePath));
+            }
+
+            return PathUtility.StripLeadingDirectorySeparators(filePath);
+        }
     }
 }

src/NuGetGallery/Services/PackageUploadService.cs

@@ -15,6 +15,7 @@
 using NuGet.Services.Entities;
 using NuGet.Versioning;
 using NuGetGallery.Configuration;
+using NuGetGallery.Diagnostics;
 using NuGetGallery.Helpers;
 using NuGetGallery.Packaging;
 
@@ -58,6 +59,7 @@ public class PackageUploadService : IPackageUploadService
         private readonly ITyposquattingService _typosquattingService;
         private readonly ITelemetryService _telemetryService;
         private readonly ICoreLicenseFileService _coreLicenseFileService;
+        private readonly IDiagnosticsSource _trace;
 
         public PackageUploadService(
             IPackageService packageService,
@@ -68,7 +70,8 @@ public PackageUploadService(
             IAppConfiguration config,
             ITyposquattingService typosquattingService,
             ITelemetryService telemetryService,
-            ICoreLicenseFileService coreLicenseFileService)
+            ICoreLicenseFileService coreLicenseFileService,
+            IDiagnosticsService diagnosticsService)
         {
             _packageService = packageService ?? throw new ArgumentNullException(nameof(packageService));
             _packageFileService = packageFileService ?? throw new ArgumentNullException(nameof(packageFileService));
@@ -79,6 +82,11 @@ public PackageUploadService(
             _typosquattingService = typosquattingService ?? throw new ArgumentNullException(nameof(typosquattingService));
             _telemetryService = telemetryService ?? throw new ArgumentNullException(nameof(telemetryService));
             _coreLicenseFileService = coreLicenseFileService ?? throw new ArgumentNullException(nameof(coreLicenseFileService));
+            if (diagnosticsService == null)
+            {
+                throw new ArgumentNullException(nameof(diagnosticsService));
+            }
+            _trace = diagnosticsService.GetSource(nameof(PackageUploadService));
         }
 
         public async Task<PackageValidationResult> ValidateBeforeGeneratePackageAsync(PackageArchiveReader nuGetPackage, PackageMetadata packageMetadata)
@@ -234,18 +242,26 @@ private async Task<PackageValidationResult> CheckLicenseMetadataAsync(PackageArc
 
             if (licenseMetadata.Type == LicenseType.File)
             {
+                // fix the path separator. Client enforces forward slashes in all file paths when packing
+                var licenseFilename = FileNameHelper.GetZipEntryPath(licenseMetadata.License);
+                if (licenseFilename != licenseMetadata.License)
+                {
+                    var packageIdentity = nuspecReader.GetIdentity();
+                    _trace.Information($"Transformed license file name from `{licenseMetadata.License}` to `{licenseFilename}` for package {packageIdentity.Id} {packageIdentity.Version}");
+                }
+
                 // check if specified file is present in the package
                 var fileList = new HashSet<string>(nuGetPackage.GetFiles());
-                if (!fileList.Contains(licenseMetadata.License))
+                if (!fileList.Contains(licenseFilename))
                 {
                     return PackageValidationResult.Invalid(
                         string.Format(
                             Strings.UploadPackage_LicenseFileDoesNotExist,
-                            licenseMetadata.License));
+                            licenseFilename));
                 }
 
                 // check if specified file has allowed extension
-                var licenseFileExtension = Path.GetExtension(licenseMetadata.License);
+                var licenseFileExtension = Path.GetExtension(licenseFilename);
                 if (!AllowedLicenseFileExtensions.Contains(licenseFileExtension, StringComparer.OrdinalIgnoreCase))
                 {
                     return PackageValidationResult.Invalid(
@@ -255,7 +271,7 @@ private async Task<PackageValidationResult> CheckLicenseMetadataAsync(PackageArc
                             string.Join(", ", AllowedLicenseFileExtensions.Where(x => x != string.Empty).Select(extension => $"'{extension}'"))));
                 }
 
-                var licenseFileEntry = nuGetPackage.GetEntry(licenseMetadata.License);
+                var licenseFileEntry = nuGetPackage.GetEntry(licenseFilename);
                 if (licenseFileEntry.Length > MaxAllowedLicenseLength)
                 {
                     return PackageValidationResult.Invalid(
@@ -264,7 +280,7 @@ private async Task<PackageValidationResult> CheckLicenseMetadataAsync(PackageArc
                             MaxAllowedLicenseLength.ToUserFriendlyBytesLabel()));
                 }
 
-                using (var licenseFileStream = nuGetPackage.GetStream(licenseMetadata.License))
+                using (var licenseFileStream = nuGetPackage.GetStream(licenseFilename))
                 {
                     if (!await IsStreamLengthMatchesReportedAsync(licenseFileStream, licenseFileEntry.Length))
                     {
@@ -273,7 +289,7 @@ private async Task<PackageValidationResult> CheckLicenseMetadataAsync(PackageArc
                 }
 
                 // zip streams do not support seeking, so we'll have to reopen them
-                using (var licenseFileStream = nuGetPackage.GetStream(licenseMetadata.License))
+                using (var licenseFileStream = nuGetPackage.GetStream(licenseFilename))
                 {
                     // check if specified file is a text file
                     if (!await TextHelper.LooksLikeUtf8TextStreamAsync(licenseFileStream))

tests/NuGetGallery.Core.Facts/Services/CoreLicenseFileServiceFacts.cs

@@ -164,13 +164,15 @@ public async Task ThrowsOnMissingLicenseFile()
                 Assert.Contains(LicenseFileName, ex.Message); // current implementation of the client does not properly set the FileName property
             }
 
-            [Fact]
-            public async Task SavesLicenseFile()
+            [Theory]
+            [InlineData("license.txt")]
+            [InlineData("foo\\bar.txt")]
+            [InlineData("foo/bar.txt")]
+            public async Task SavesLicenseFile(string licenseFilenName)
             {
                 var fileStorageSvc = new Mock<ICoreFileStorageService>();
                 var service = CreateService(fileStorageSvc);
-                const string LicenseFileName = "license.txt";
-                var packageStream = GeneratePackageAsync(LicenseFileName);
+                var packageStream = GeneratePackageAsync(licenseFilenName);
                 var package = PackageServiceUtility.CreateTestPackage();
                 package.EmbeddedLicenseType = EmbeddedLicenseFileType.PlainText;
                 var savedLicenseBytes = new byte[LicenseFileContents.Length];

tests/NuGetGallery.Core.Facts/TestUtils/TestPackage.cs

@@ -7,6 +7,7 @@
 using System.IO.Compression;
 using System.Linq;
 using System.Text;
+using NuGet.Common;
 using NuGet.Packaging;
 using NuGet.Packaging.Core;
 using ClientPackageType = NuGet.Packaging.Core.PackageType;
@@ -212,6 +213,8 @@ public static MemoryStream CreateTestPackageStream(
 
                 if (licenseFileContents != null && licenseFilename != null)
                 {
+                    // enforce directory separators the same way as the client (see PackageArchiveReader.GetStream)
+                    licenseFilename = PathUtility.StripLeadingDirectorySeparators(licenseFilename);
                     var licenseEntry = packageArchive.CreateEntry(licenseFilename, CompressionLevel.Fastest);
                     using (var licenseStream = licenseEntry.Open())
                     {

tests/NuGetGallery.Facts/Services/PackageUploadServiceFacts.cs

@@ -14,6 +14,7 @@
 using NuGet.Packaging.Core;
 using NuGet.Services.Entities;
 using NuGetGallery.Configuration;
+using NuGetGallery.Diagnostics;
 using NuGetGallery.Packaging;
 using NuGetGallery.TestUtils;
 using Xunit;
@@ -63,6 +64,10 @@ private static PackageUploadService CreateService(
 
             validationService = validationService ?? new Mock<IValidationService>();
             config = config ?? new Mock<IAppConfiguration>();
+            var diagnosticsService = new Mock<IDiagnosticsService>();
+            diagnosticsService
+                .Setup(ds => ds.GetSource(It.IsAny<string>()))
+                .Returns(Mock.Of<IDiagnosticsSource>());
 
             var packageUploadService = new Mock<PackageUploadService>(
                 packageService.Object,
@@ -73,7 +78,8 @@ private static PackageUploadService CreateService(
                 config.Object,
                 new Mock<ITyposquattingService>().Object,
                 Mock.Of<ITelemetryService>(),
-                Mock.Of<ICoreLicenseFileService>());
+                Mock.Of<ICoreLicenseFileService>(),
+                diagnosticsService.Object);
 
             return packageUploadService.Object;
         }
@@ -1046,6 +1052,28 @@ public async Task RejectsNupkgsWithUnknownLicenseTypes(string licenseNode)
                 Assert.Empty(result.Warnings);
             }
 
+            [Theory]
+            [InlineData("license/something.txt")]
+            [InlineData("license\\anotherthing.txt")]
+            [InlineData(".\\license\\morething.txt")]
+            [InlineData("./license/lessthing.txt")]
+            public async Task AcceptsLicenseFileInSubdirectories(string licensePath)
+            {
+                _nuGetPackage = GeneratePackageWithLicense(
+                    licenseFilename: licensePath,
+                    licenseUrl: new Uri(LicenseDeprecationUrl),
+                    licenseFileContents: "some license");
+
+                var result = await _target.ValidateBeforeGeneratePackageAsync(
+                    _nuGetPackage.Object,
+                    GetPackageMetadata(_nuGetPackage));
+
+                Assert.Equal(PackageValidationResultType.Accepted, result.Type);
+                Assert.Null(result.Message);
+                Assert.Empty(result.Warnings);
+            }
+
+
             /// <summary>
             /// A (quite ineffective) method to search for a sequence in an array
             /// </summary>
@@ -1777,7 +1805,7 @@ public abstract class FactsBase
             protected readonly Mock<ITyposquattingService> _typosquattingService;
             protected readonly Mock<ITelemetryService> _telemetryService;
             protected readonly Mock<ICoreLicenseFileService> _licenseFileService;
-
+            protected readonly Mock<IDiagnosticsService> _diagnosticsService;
             protected Package _package;
             protected Stream _packageFile;
             protected ArgumentException _unexpectedException;
@@ -1812,6 +1840,11 @@ public FactsBase()
                 _conflictException = new FileAlreadyExistsException("Conflict!");
                 _token = CancellationToken.None;
                 _licenseFileService = new Mock<ICoreLicenseFileService>();
+                _diagnosticsService = new Mock<IDiagnosticsService>();
+
+                _diagnosticsService
+                    .Setup(ds => ds.GetSource(It.IsAny<string>()))
+                    .Returns(Mock.Of<IDiagnosticsSource>());
 
                 _target = new PackageUploadService(
                     _packageService.Object,
@@ -1822,7 +1855,8 @@ public FactsBase()
                     _config.Object,
                     _typosquattingService.Object,
                     _telemetryService.Object,
-                    _licenseFileService.Object);
+                    _licenseFileService.Object,
+                    _diagnosticsService.Object);
             }
 
             protected static Mock<TestPackageReader> GeneratePackage(