Skip to content

Commit bb75cce

Browse files
agragr
authored
Targeted block for invalidly URL encoded license URLs for license expressions (#6673)
* Targeted block for invalidly URL encoded license URLs for license expressions. * Updated error message for the malformed URL case. Changed invalid URL from warning to error. Updated tests.
1 parent f612b6c commit bb75cce

9 files changed

Lines changed: 134 additions & 52 deletions

‎src/NuGetGallery/Helpers/LicenseExpressionRedirectUrlHelper.cs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,8 @@ namespace NuGetGallery.Helpers
77
{
88
public static class LicenseExpressionRedirectUrlHelper
99
{
10-
public const string LicenseExpressionDeprecationUrlFormat = "https://licenses.nuget.org/{0}";
10+
public const string LicenseExpressionHostname = "licenses.nuget.org";
11+
public const string LicenseExpressionDeprecationUrlFormat = "https://" + LicenseExpressionHostname + "/{0}";
1112

1213
public static string GetLicenseExpressionRedirectUrl(string licenseExpression)
1314
{

‎src/NuGetGallery/NuGetGallery.csproj‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -427,6 +427,8 @@
427427
<Compile Include="Services\ActionsRequiringPermissions.cs" />
428428
<Compile Include="Services\AsynchronousPackageValidationInitiator.cs" />
429429
<Compile Include="Infrastructure\Mail\BackgroundMarkdownMessageService.cs" />
430+
<Compile Include="Services\InvalidLicenseUrlValidationMessage.cs" />
431+
<Compile Include="Services\InvalidUrlEncodingForLicenseUrlValidationMessage.cs" />
430432
<Compile Include="Services\ISymbolPackageUploadService.cs" />
431433
<Compile Include="Services\ISymbolPackageFileService.cs" />
432434
<Compile Include="Services\ITyposquattingCheckListCacheService.cs" />

‎src/NuGetGallery/Services/InvalidLicenseUrlValidationMessage.cs‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
// Copyright (c) .NET Foundation. All rights reserved.
2+
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
3+
4+
using System;
5+
6+
namespace NuGetGallery
7+
{
8+
/// <summary>
9+
/// The error message to be used when package contains license expression, but
10+
/// its license URL points to a wrong location
11+
/// </summary>
12+
public class InvalidLicenseUrlValidationMessage : IValidationMessage
13+
{
14+
private string DetailsLink => $"<a href=\"https://aka.ms/invalidNuGetLicenseUrl\">{Strings.UploadPackage_LearnMore}</a>.";
15+
private string DetailsPlainText => "https://aka.ms/invalidNuGetLicenseUrl";
16+
17+
private readonly string _baseMessage;
18+
19+
public InvalidLicenseUrlValidationMessage(string baseMessage)
20+
{
21+
_baseMessage = baseMessage ?? throw new ArgumentNullException(nameof(baseMessage));
22+
}
23+
24+
public string PlainTextMessage => _baseMessage + " " + DetailsPlainText;
25+
26+
public bool HasRawHtmlRepresentation => true;
27+
28+
public string RawHtmlMessage => _baseMessage + " " + DetailsLink;
29+
30+
}
31+
}

‎src/NuGetGallery/Services/InvalidUrlEncodingForLicenseUrlValidationMessage.cs‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
// Copyright (c) .NET Foundation. All rights reserved.
2+
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
3+
4+
namespace NuGetGallery
5+
{
6+
/// <summary>
7+
/// The error message to be used when we detect an invalid URL encoding used for a license URL.
8+
/// Specifically, if we see that spaces are encoded as pluses instead of %20
9+
/// </summary>
10+
public class InvalidUrlEncodingForLicenseUrlValidationMessage : IValidationMessage
11+
{
12+
private string DetailsLink => $"<a href=\"https://aka.ms/malformedNuGetLicenseUrl\">{Strings.UploadPackage_LearnMore}</a>.";
13+
private string DetailsPlainText => "https://aka.ms/malformedNuGetLicenseUrl";
14+
15+
private string BaseMessage => Strings.UploadPackage_MalformedLicenseUrl;
16+
17+
public string PlainTextMessage => BaseMessage + " " + DetailsPlainText;
18+
19+
public bool HasRawHtmlRepresentation => true;
20+
21+
public string RawHtmlMessage => BaseMessage + " " + DetailsLink;
22+
}
23+
}

‎src/NuGetGallery/Services/LicenseUrlDeprecationValidationMessage.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ namespace NuGetGallery
1212
/// </summary>
1313
public class LicenseUrlDeprecationValidationMessage : IValidationMessage
1414
{
15-
private string DeprecationLink => $"<a href=\"{GalleryConstants.LicenseDeprecationUrl}\">{Strings.UploadPackage_LearnMore}.</a>";
15+
private string DeprecationLink => $"<a href=\"{GalleryConstants.LicenseDeprecationUrl}\">{Strings.UploadPackage_LearnMore}</a>.";
1616

1717
private readonly string _baseMessage;
1818

‎src/NuGetGallery/Services/PackageUploadService.cs‎

Lines changed: 34 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,6 @@ private async Task<PackageValidationResult> CheckLicenseMetadataAsync(PackageArc
171171
var licenseUrl = nuspecReader.GetLicenseUrl();
172172
var licenseMetadata = nuspecReader.GetLicenseMetadata();
173173
var licenseDeprecationUrl = GetExpectedLicenseUrl(licenseMetadata);
174-
var alternativeDeprecationUrl = GetExpectedAlternativeUrl(licenseMetadata);
175174

176175
if (licenseMetadata == null)
177176
{
@@ -219,19 +218,24 @@ private async Task<PackageValidationResult> CheckLicenseMetadataAsync(PackageArc
219218
string.Join(" ", licenseMetadata.WarningsAndErrors)));
220219
}
221220

222-
if (licenseDeprecationUrl != licenseUrl && (alternativeDeprecationUrl == null || alternativeDeprecationUrl != licenseUrl))
221+
if (licenseDeprecationUrl != licenseUrl)
223222
{
223+
if (IsMalformedDeprecationUrl(licenseUrl))
224+
{
225+
return PackageValidationResult.Invalid(new InvalidUrlEncodingForLicenseUrlValidationMessage());
226+
}
227+
224228
if (licenseMetadata.Type == LicenseType.File)
225229
{
226-
warnings.Add(
227-
new PlainTextOnlyValidationMessage(
228-
string.Format(Strings.UploadPackage_DeprecationUrlSuggestedForLicenseFiles, licenseDeprecationUrl)));
230+
return PackageValidationResult.Invalid(
231+
new InvalidLicenseUrlValidationMessage(
232+
string.Format(Strings.UploadPackage_DeprecationUrlRequiredForLicenseFiles, licenseDeprecationUrl)));
229233
}
230234
else if (licenseMetadata.Type == LicenseType.Expression)
231235
{
232-
warnings.Add(
233-
new PlainTextOnlyValidationMessage(
234-
string.Format(Strings.UploadPackage_DeprecationUrlSuggestedForLicenseExpressions, licenseDeprecationUrl)));
236+
return PackageValidationResult.Invalid(
237+
new InvalidLicenseUrlValidationMessage(
238+
string.Format(Strings.UploadPackage_DeprecationUrlRequiredForLicenseExpressions, licenseDeprecationUrl)));
235239
}
236240
}
237241

@@ -307,6 +311,28 @@ private async Task<PackageValidationResult> CheckLicenseMetadataAsync(PackageArc
307311
return null;
308312
}
309313

314+
private bool IsMalformedDeprecationUrl(string licenseUrl)
315+
{
316+
// nuget.exe 4.9.0 and its dotnet and msbuild counterparts encode spaces as "+"
317+
// when generating legacy license URL, which is bad. We explicitly forbid such
318+
// URLs. On the other hand, if license expression does not contain spaces they
319+
// generate good URLs which we don't want to reject. This method detects the
320+
// case when spaces are in the expression in a meaningful way.
321+
322+
if (Uri.TryCreate(licenseUrl, UriKind.Absolute, out var url))
323+
{
324+
if (url.Host != LicenseExpressionRedirectUrlHelper.LicenseExpressionHostname)
325+
{
326+
return false;
327+
}
328+
329+
var invalidUrlBits = new string[] { "+OR+", "+AND+", "+WITH+" };
330+
return invalidUrlBits.Any(invalidBit => licenseUrl.IndexOf(invalidBit, StringComparison.OrdinalIgnoreCase) >= 0);
331+
}
332+
333+
return false;
334+
}
335+
310336
private List<LicenseData> GetLicenseList(NuGetLicenseExpression licenseExpression)
311337
{
312338
var licenseList = new List<LicenseData>();
@@ -380,25 +406,6 @@ private static string GetExpectedLicenseUrl(LicenseMetadata licenseMetadata)
380406
throw new InvalidOperationException($"Unsupported license metadata type: {licenseMetadata.Type}");
381407
}
382408

383-
/// <summary>
384-
/// 15.9 client does url encoding with <see cref="WebUtility.UrlEncode(string)"/> which
385-
/// replaces spaces with "+". We shouldn't work about it, but we should fix the client,
386-
/// too.
387-
/// </summary>
388-
private static string GetExpectedAlternativeUrl(LicenseMetadata licenseMetadata)
389-
{
390-
if (licenseMetadata != null && licenseMetadata.Type == LicenseType.Expression)
391-
{
392-
return new Uri(
393-
string.Format(
394-
LicenseExpressionRedirectUrlHelper.LicenseExpressionDeprecationUrlFormat,
395-
WebUtility.UrlEncode(licenseMetadata.License)))
396-
.AbsoluteUri;
397-
}
398-
399-
return null;
400-
}
401-
402409
private static bool HasChildElements(XElement xElement)
403410
=> xElement.Elements().Any();
404411

‎src/NuGetGallery/Strings.Designer.cs‎

Lines changed: 16 additions & 7 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/NuGetGallery/Strings.resx‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1056,12 +1056,12 @@ The {1} Team</value>
10561056
<data name="UploadPackage_LicenseFilesAreNotAllowed" xml:space="preserve">
10571057
<value>License files are not yet supported.</value>
10581058
</data>
1059-
<data name="UploadPackage_DeprecationUrlSuggestedForLicenseExpressions" xml:space="preserve">
1060-
<value>To provide better experience for older clients when a license expression is specified, &lt;licenseUrl&gt; should be set to '{0}'.</value>
1059+
<data name="UploadPackage_DeprecationUrlRequiredForLicenseExpressions" xml:space="preserve">
1060+
<value>To provide a better experience for older clients when a license expression is specified, &lt;licenseUrl&gt; must be set to '{0}'.</value>
10611061
<comment>{0} is the licenses.nuget.org link with license expression</comment>
10621062
</data>
1063-
<data name="UploadPackage_DeprecationUrlSuggestedForLicenseFiles" xml:space="preserve">
1064-
<value>To provide better experience for older clients when a license file is packaged, &lt;licenseUrl&gt; should be set to '{0}'.</value>
1063+
<data name="UploadPackage_DeprecationUrlRequiredForLicenseFiles" xml:space="preserve">
1064+
<value>To provide a better experience for older clients when a license file is packaged, &lt;licenseUrl&gt; must be set to '{0}'.</value>
10651065
<comment>{0} is the deprecation page URL</comment>
10661066
</data>
10671067
<data name="UploadPackage_LearnMore" xml:space="preserve">
@@ -1074,4 +1074,7 @@ The {1} Team</value>
10741074
<value>License expression must only contain licenses that are approved by Open Source Initiative or Free Software Foundation. Unsupported licenses: {0}.</value>
10751075
<comment>{0} is list of the package license ids that are neither OSI nor FSF approved</comment>
10761076
</data>
1077+
<data name="UploadPackage_MalformedLicenseUrl" xml:space="preserve">
1078+
<value>The package contains a malformed license URL.</value>
1079+
</data>
10771080
</root>

‎tests/NuGetGallery.Facts/Services/PackageUploadServiceFacts.cs‎

Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -567,28 +567,33 @@ public async Task WarnsWhenInvalidLicenseUrlSpecifiedWithLicenseFile(string lice
567567
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);
568568
}
569569

570-
[Fact]
571-
public async Task AcceptsAlternativeLicenseUrl()
570+
[Theory]
571+
[InlineData("Apache-1.0+ OR MIT", "Apache-1.0%2B+OR+MIT")]
572+
[InlineData("Apache-1.0+ AND MIT", "Apache-1.0%2B+AND+MIT")]
573+
[InlineData("Apache-1.0+ AND MIT WITH Classpath-exception-2.0", "Apache-1.0%2B+AND+MIT+WITH+Classpath-exception-2.0")]
574+
[InlineData("MIT WITH Classpath-exception-2.0", "MIT+WITH+Classpath-exception-2.0")]
575+
[InlineData("MIT", "Apache-1.0%2B+OR+MIT")]
576+
public async Task RejectsAlternativeLicenseUrl(string licenseExpression, string licenseUrlPostfix)
572577
{
573578
_nuGetPackage = GeneratePackageWithLicense(
574-
licenseUrl: new Uri("https://licenses.nuget.org/Apache-1.0%2B+OR+MIT"),
575-
licenseExpression: "Apache-1.0+ OR MIT",
579+
licenseUrl: new Uri($"https://licenses.nuget.org/{licenseUrlPostfix}"),
580+
licenseExpression: licenseExpression,
576581
licenseFilename: null,
577582
licenseFileContents: null);
578583

579584
var result = await _target.ValidateBeforeGeneratePackageAsync(
580585
_nuGetPackage.Object,
581586
GetPackageMetadata(_nuGetPackage));
582587

583-
Assert.Equal(PackageValidationResultType.Accepted, result.Type);
584-
Assert.Null(result.Message);
588+
Assert.Equal(PackageValidationResultType.Invalid, result.Type);
589+
Assert.Contains("malformed license URL", result.Message.PlainTextMessage);
585590
Assert.Empty(result.Warnings);
586591
}
587592

588593
[Theory]
589594
[InlineData(null)]
590595
[InlineData(RegularLicenseUrl)]
591-
public async Task WarnsWhenInvalidLicenseUrlSpecifiedWithLicenseExpression(string licenseUrl)
596+
public async Task ErrorsWhenInvalidLicenseUrlSpecifiedWithLicenseExpression(string licenseUrl)
592597
{
593598
_nuGetPackage = GeneratePackageWithLicense(
594599
licenseUrl: licenseUrl == null ? null : new Uri(licenseUrl),
@@ -599,10 +604,10 @@ public async Task WarnsWhenInvalidLicenseUrlSpecifiedWithLicenseExpression(strin
599604
_nuGetPackage.Object,
600605
GetPackageMetadata(_nuGetPackage));
601606

602-
Assert.Equal(PackageValidationResultType.Accepted, result.Type);
603-
Assert.Null(result.Message);
604-
Assert.Single(result.Warnings);
605-
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);
607+
Assert.Equal(PackageValidationResultType.Invalid, result.Type);
608+
Assert.IsType<InvalidLicenseUrlValidationMessage>(result.Message);
609+
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);
610+
Assert.Empty(result.Warnings);
606611
}
607612

608613
[Fact]
@@ -687,7 +692,8 @@ public async Task RejectsUnknownLicense(string licenseExpression)
687692
[InlineData("Saxpath OR GPL-1.0-only WITH Classpath-exception-2.0", new[] { "Saxpath", "GPL-1.0-only" })]
688693
public async Task RejectsNonOsiFsfLicenses(string licenseExpression, string[] unapprovedLicenses)
689694
{
690-
_nuGetPackage = GeneratePackageWithLicense(licenseUrl: new Uri(LicenseDeprecationUrl), licenseExpression: licenseExpression, licenseFilename: null);
695+
var licenseUri = new Uri($"https://licenses.nuget.org/{licenseExpression}");
696+
_nuGetPackage = GeneratePackageWithLicense(licenseUrl: licenseUri, licenseExpression: licenseExpression, licenseFilename: null);
691697

692698
var result = await _target.ValidateBeforeGeneratePackageAsync(
693699
_nuGetPackage.Object,

0 commit comments

Comments
 (0)