Merge pull request #6689 from NuGet/master

FI master to dev

Author: Christy Henriksson

src/NuGetGallery/Controllers/PackagesController.cs

@@ -2328,18 +2328,18 @@ public virtual async Task<JsonResult> PreviewReadMe(ReadMeRequest formData)
         {
             if (formData == null || !_readMeService.HasReadMeSource(formData))
             {
-                return Json(HttpStatusCode.BadRequest, new[] { new JsonValidationMessage(Strings.PreviewReadMe_ReadMeMissing) });
+                return Json(HttpStatusCode.BadRequest, new[] { Strings.PreviewReadMe_ReadMeMissing });
             }
 
             try
             {
                 var readMeHtml = await _readMeService.GetReadMeHtmlAsync(formData, Request.ContentEncoding);
-                return Json(new[] { new JsonValidationMessage(readMeHtml) });
+                return Json(new[] { readMeHtml });
             }
             catch (Exception ex)
             {
                 return Json(HttpStatusCode.BadRequest, new[] {
-                    new JsonValidationMessage(string.Format(CultureInfo.CurrentCulture, Strings.PreviewReadMe_ConversionFailed, ex.Message)) });
+                    string.Format(CultureInfo.CurrentCulture, Strings.PreviewReadMe_ConversionFailed, ex.Message) });
             }
         }
 

src/NuGetGallery/Helpers/LicenseExpressionRedirectUrlHelper.cs

@@ -7,7 +7,8 @@ namespace NuGetGallery.Helpers
 {
     public static class LicenseExpressionRedirectUrlHelper
     {
-        public const string LicenseExpressionDeprecationUrlFormat = "https://licenses.nuget.org/{0}";
+        public const string LicenseExpressionHostname = "licenses.nuget.org";
+        public const string LicenseExpressionDeprecationUrlFormat = "https://" + LicenseExpressionHostname + "/{0}";
 
         public static string GetLicenseExpressionRedirectUrl(string licenseExpression)
         {

src/NuGetGallery/NuGetGallery.csproj

@@ -429,6 +429,8 @@
     <Compile Include="Services\ActionsRequiringPermissions.cs" />
     <Compile Include="Services\AsynchronousPackageValidationInitiator.cs" />
     <Compile Include="Infrastructure\Mail\BackgroundMarkdownMessageService.cs" />
+    <Compile Include="Services\InvalidLicenseUrlValidationMessage.cs" />
+    <Compile Include="Services\InvalidUrlEncodingForLicenseUrlValidationMessage.cs" />
     <Compile Include="Services\ISymbolPackageUploadService.cs" />
     <Compile Include="Services\ISymbolPackageFileService.cs" />
     <Compile Include="Services\ITyposquattingCheckListCacheService.cs" />

src/NuGetGallery/Services/InvalidLicenseUrlValidationMessage.cs

@@ -0,0 +1,31 @@
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using System;
+
+namespace NuGetGallery
+{
+    /// <summary>
+    /// The error message to be used when package contains license expression, but
+    /// its license URL points to a wrong location
+    /// </summary>
+    public class InvalidLicenseUrlValidationMessage : IValidationMessage
+    {
+        private string DetailsLink => $"<a href=\"https://aka.ms/invalidNuGetLicenseUrl\">{Strings.UploadPackage_LearnMore}</a>.";
+        private string DetailsPlainText => "https://aka.ms/invalidNuGetLicenseUrl";
+
+        private readonly string _baseMessage;
+
+        public InvalidLicenseUrlValidationMessage(string baseMessage)
+        {
+            _baseMessage = baseMessage ?? throw new ArgumentNullException(nameof(baseMessage));
+        }
+
+        public string PlainTextMessage => _baseMessage + " " + DetailsPlainText;
+
+        public bool HasRawHtmlRepresentation => true;
+
+        public string RawHtmlMessage => _baseMessage + " " + DetailsLink;
+
+    }
+}

src/NuGetGallery/Services/InvalidUrlEncodingForLicenseUrlValidationMessage.cs

@@ -0,0 +1,23 @@
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+namespace NuGetGallery
+{
+    /// <summary>
+    /// The error message to be used when we detect an invalid URL encoding used for a license URL.
+    /// Specifically, if we see that spaces are encoded as pluses instead of %20
+    /// </summary>
+    public class InvalidUrlEncodingForLicenseUrlValidationMessage : IValidationMessage
+    {
+        private string DetailsLink => $"<a href=\"https://aka.ms/malformedNuGetLicenseUrl\">{Strings.UploadPackage_LearnMore}</a>.";
+        private string DetailsPlainText => "https://aka.ms/malformedNuGetLicenseUrl";
+
+        private string BaseMessage => Strings.UploadPackage_MalformedLicenseUrl;
+
+        public string PlainTextMessage => BaseMessage + " " + DetailsPlainText;
+
+        public bool HasRawHtmlRepresentation => true;
+
+        public string RawHtmlMessage => BaseMessage + " " + DetailsLink;
+    }
+}

src/NuGetGallery/Services/LicenseUrlDeprecationValidationMessage.cs

@@ -12,7 +12,7 @@ namespace NuGetGallery
     /// </summary>
     public class LicenseUrlDeprecationValidationMessage : IValidationMessage
     {
-        private string DeprecationLink => $"<a href=\"{GalleryConstants.LicenseDeprecationUrl}\">{Strings.UploadPackage_LearnMore}.</a>";
+        private string DeprecationLink => $"<a href=\"{GalleryConstants.LicenseDeprecationUrl}\">{Strings.UploadPackage_LearnMore}</a>.";
 
         private readonly string _baseMessage;
 

src/NuGetGallery/Services/PackageUploadService.cs

@@ -171,7 +171,6 @@ private async Task<PackageValidationResult> CheckLicenseMetadataAsync(PackageArc
             var licenseUrl = nuspecReader.GetLicenseUrl();
             var licenseMetadata = nuspecReader.GetLicenseMetadata();
             var licenseDeprecationUrl = GetExpectedLicenseUrl(licenseMetadata);
-            var alternativeDeprecationUrl = GetExpectedAlternativeUrl(licenseMetadata);
 
             if (licenseMetadata == null)
             {
@@ -219,19 +218,24 @@ private async Task<PackageValidationResult> CheckLicenseMetadataAsync(PackageArc
                         string.Join(" ", licenseMetadata.WarningsAndErrors)));
             }
 
-            if (licenseDeprecationUrl != licenseUrl && (alternativeDeprecationUrl == null || alternativeDeprecationUrl != licenseUrl))
+            if (licenseDeprecationUrl != licenseUrl)
             {
+                if (IsMalformedDeprecationUrl(licenseUrl))
+                {
+                    return PackageValidationResult.Invalid(new InvalidUrlEncodingForLicenseUrlValidationMessage());
+                }
+                
                 if (licenseMetadata.Type == LicenseType.File)
                 {
-                    warnings.Add(
-                        new PlainTextOnlyValidationMessage(
-                            string.Format(Strings.UploadPackage_DeprecationUrlSuggestedForLicenseFiles, licenseDeprecationUrl)));
+                    return PackageValidationResult.Invalid(
+                        new InvalidLicenseUrlValidationMessage(
+                            string.Format(Strings.UploadPackage_DeprecationUrlRequiredForLicenseFiles, licenseDeprecationUrl)));
                 }
                 else if (licenseMetadata.Type == LicenseType.Expression)
                 {
-                    warnings.Add(
-                        new PlainTextOnlyValidationMessage(
-                            string.Format(Strings.UploadPackage_DeprecationUrlSuggestedForLicenseExpressions, licenseDeprecationUrl)));
+                    return PackageValidationResult.Invalid(
+                        new InvalidLicenseUrlValidationMessage(
+                            string.Format(Strings.UploadPackage_DeprecationUrlRequiredForLicenseExpressions, licenseDeprecationUrl)));
                 }
             }
 
@@ -307,6 +311,28 @@ private async Task<PackageValidationResult> CheckLicenseMetadataAsync(PackageArc
             return null;
         }
 
+        private bool IsMalformedDeprecationUrl(string licenseUrl)
+        {
+            // nuget.exe 4.9.0 and its dotnet and msbuild counterparts encode spaces as "+"
+            // when generating legacy license URL, which is bad. We explicitly forbid such
+            // URLs. On the other hand, if license expression does not contain spaces they
+            // generate good URLs which we don't want to reject. This method detects the 
+            // case when spaces are in the expression in a meaningful way.
+
+            if (Uri.TryCreate(licenseUrl, UriKind.Absolute, out var url))
+            {
+                if (url.Host != LicenseExpressionRedirectUrlHelper.LicenseExpressionHostname)
+                {
+                    return false;
+                }
+
+                var invalidUrlBits = new string[] { "+OR+", "+AND+", "+WITH+" };
+                return invalidUrlBits.Any(invalidBit => licenseUrl.IndexOf(invalidBit, StringComparison.OrdinalIgnoreCase) >= 0);
+            }
+
+            return false;
+        }
+
         private List<LicenseData> GetLicenseList(NuGetLicenseExpression licenseExpression)
         {
             var licenseList = new List<LicenseData>();
@@ -380,25 +406,6 @@ private static string GetExpectedLicenseUrl(LicenseMetadata licenseMetadata)
             throw new InvalidOperationException($"Unsupported license metadata type: {licenseMetadata.Type}");
         }
 
-        /// <summary>
-        /// 15.9 client does url encoding with <see cref="WebUtility.UrlEncode(string)"/> which
-        /// replaces spaces with "+". We shouldn't work about it, but we should fix the client,
-        /// too.
-        /// </summary>
-        private static string GetExpectedAlternativeUrl(LicenseMetadata licenseMetadata)
-        {
-            if (licenseMetadata != null && licenseMetadata.Type == LicenseType.Expression)
-            {
-                return new Uri(
-                    string.Format(
-                        LicenseExpressionRedirectUrlHelper.LicenseExpressionDeprecationUrlFormat,
-                        WebUtility.UrlEncode(licenseMetadata.License)))
-                    .AbsoluteUri;
-            }
-
-            return null;
-        }
-
         private static bool HasChildElements(XElement xElement)
             => xElement.Elements().Any();
 

src/NuGetGallery/Strings.Designer.cs

@@ -1056,12 +1056,12 @@ The {1} Team</value>
   <data name="UploadPackage_LicenseFilesAreNotAllowed" xml:space="preserve">
     <value>License files are not yet supported.</value>
   </data>
-  <data name="UploadPackage_DeprecationUrlSuggestedForLicenseExpressions" xml:space="preserve">
-    <value>To provide better experience for older clients when a license expression is specified, &lt;licenseUrl&gt; should be set to '{0}'.</value>
+  <data name="UploadPackage_DeprecationUrlRequiredForLicenseExpressions" xml:space="preserve">
+    <value>To provide a better experience for older clients when a license expression is specified, &lt;licenseUrl&gt; must be set to '{0}'.</value>
     <comment>{0} is the licenses.nuget.org link with license expression</comment>
   </data>
-  <data name="UploadPackage_DeprecationUrlSuggestedForLicenseFiles" xml:space="preserve">
-    <value>To provide better experience for older clients when a license file is packaged, &lt;licenseUrl&gt; should be set to '{0}'.</value>
+  <data name="UploadPackage_DeprecationUrlRequiredForLicenseFiles" xml:space="preserve">
+    <value>To provide a better experience for older clients when a license file is packaged, &lt;licenseUrl&gt; must be set to '{0}'.</value>
     <comment>{0} is the deprecation page URL</comment>
   </data>
   <data name="UploadPackage_LearnMore" xml:space="preserve">
@@ -1074,4 +1074,7 @@ The {1} Team</value>
     <value>License expression must only contain licenses that are approved by Open Source Initiative or Free Software Foundation. Unsupported licenses: {0}.</value>
     <comment>{0} is list of the package license ids that are neither OSI nor FSF approved</comment>
   </data>
+  <data name="UploadPackage_MalformedLicenseUrl" xml:space="preserve">
+    <value>The package contains a malformed license URL.</value>
+  </data>
 </root>

src/NuGetGallery/Strings.resx

@@ -567,28 +567,33 @@ public async Task WarnsWhenInvalidLicenseUrlSpecifiedWithLicenseFile(string lice
                 Assert.Equal("To provide better experience for older clients when a license file is packaged, <licenseUrl> should be set to 'https://aka.ms/deprecateLicenseUrl'.", result.Warnings[0].PlainTextMessage);
             }
 
-            [Fact]
-            public async Task AcceptsAlternativeLicenseUrl()
+            [Theory]
+            [InlineData("Apache-1.0+ OR MIT", "Apache-1.0%2B+OR+MIT")]
+            [InlineData("Apache-1.0+ AND MIT", "Apache-1.0%2B+AND+MIT")]
+            [InlineData("Apache-1.0+ AND MIT WITH Classpath-exception-2.0", "Apache-1.0%2B+AND+MIT+WITH+Classpath-exception-2.0")]
+            [InlineData("MIT WITH Classpath-exception-2.0", "MIT+WITH+Classpath-exception-2.0")]
+            [InlineData("MIT", "Apache-1.0%2B+OR+MIT")]
+            public async Task RejectsAlternativeLicenseUrl(string licenseExpression, string licenseUrlPostfix)
             {
                 _nuGetPackage = GeneratePackageWithLicense(
-                    licenseUrl: new Uri("https://licenses.nuget.org/Apache-1.0%2B+OR+MIT"),
-                    licenseExpression: "Apache-1.0+ OR MIT",
+                    licenseUrl: new Uri($"https://licenses.nuget.org/{licenseUrlPostfix}"),
+                    licenseExpression: licenseExpression,
                     licenseFilename: null,
                     licenseFileContents: null);
 
                 var result = await _target.ValidateBeforeGeneratePackageAsync(
                     _nuGetPackage.Object,
                     GetPackageMetadata(_nuGetPackage));
 
-                Assert.Equal(PackageValidationResultType.Accepted, result.Type);
-                Assert.Null(result.Message);
+                Assert.Equal(PackageValidationResultType.Invalid, result.Type);
+                Assert.Contains("malformed license URL", result.Message.PlainTextMessage);
                 Assert.Empty(result.Warnings);
             }
 
             [Theory]
             [InlineData(null)]
             [InlineData(RegularLicenseUrl)]
-            public async Task WarnsWhenInvalidLicenseUrlSpecifiedWithLicenseExpression(string licenseUrl)
+            public async Task ErrorsWhenInvalidLicenseUrlSpecifiedWithLicenseExpression(string licenseUrl)
             {
                 _nuGetPackage = GeneratePackageWithLicense(
                     licenseUrl: licenseUrl == null ? null : new Uri(licenseUrl),
@@ -599,10 +604,10 @@ public async Task WarnsWhenInvalidLicenseUrlSpecifiedWithLicenseExpression(strin
                     _nuGetPackage.Object,
                     GetPackageMetadata(_nuGetPackage));
 
-                Assert.Equal(PackageValidationResultType.Accepted, result.Type);
-                Assert.Null(result.Message);
-                Assert.Single(result.Warnings);
-                Assert.Equal("To provide better experience for older clients when a license expression is specified, <licenseUrl> should be set to 'https://licenses.nuget.org/MIT'.", result.Warnings[0].PlainTextMessage);
+                Assert.Equal(PackageValidationResultType.Invalid, result.Type);
+                Assert.IsType<InvalidLicenseUrlValidationMessage>(result.Message);
+                Assert.StartsWith("To provide a better experience for older clients when a license expression is specified, <licenseUrl> must be set to 'https://licenses.nuget.org/MIT'.", result.Message.PlainTextMessage);
+                Assert.Empty(result.Warnings);
             }
 
             [Fact]
@@ -687,7 +692,8 @@ public async Task RejectsUnknownLicense(string licenseExpression)
             [InlineData("Saxpath OR GPL-1.0-only WITH Classpath-exception-2.0", new[] { "Saxpath", "GPL-1.0-only" })]
             public async Task RejectsNonOsiFsfLicenses(string licenseExpression, string[] unapprovedLicenses)
             {
-                _nuGetPackage = GeneratePackageWithLicense(licenseUrl: new Uri(LicenseDeprecationUrl), licenseExpression: licenseExpression, licenseFilename: null);
+                var licenseUri = new Uri($"https://licenses.nuget.org/{licenseExpression}");
+                _nuGetPackage = GeneratePackageWithLicense(licenseUrl: licenseUri, licenseExpression: licenseExpression, licenseFilename: null);
 
                 var result = await _target.ValidateBeforeGeneratePackageAsync(
                     _nuGetPackage.Object,