Skip to content

Commit 43f7032

Browse files
skofman1skofman1
authored
Validation warnings for repository URLs (#6335)
* works * improve log * PR comments * Fix comments * bug fix * repo url warnings * add UT * align * Update _VerifyMetadata.cshtml Fixed: issue**s**(s) to issue(s) * PR feedback * support explicit 443 port for github * fix build
1 parent d05d2f6 commit 43f7032

23 files changed

Lines changed: 333 additions & 128 deletions

‎src/NuGetGallery.Core/Packaging/ManifestValidator.cs‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,20 +14,24 @@ namespace NuGetGallery.Packaging
1414
{
1515
public class ManifestValidator
1616
{
17-
public static IEnumerable<ValidationResult> Validate(Stream nuspecStream, out NuspecReader nuspecReader)
17+
public static IEnumerable<ValidationResult> Validate(Stream nuspecStream, out NuspecReader nuspecReader, out PackageMetadata packageMetadata)
1818
{
19+
packageMetadata = null;
20+
1921
try
2022
{
2123
nuspecReader = new NuspecReader(nuspecStream);
2224
var rawMetadata = nuspecReader.GetMetadata();
2325
if (rawMetadata != null && rawMetadata.Any())
2426
{
25-
return ValidateCore(PackageMetadata.FromNuspecReader(nuspecReader, strict: true));
27+
packageMetadata = PackageMetadata.FromNuspecReader(nuspecReader, strict: true);
28+
return ValidateCore(packageMetadata);
2629
}
2730
}
2831
catch (Exception ex)
2932
{
3033
nuspecReader = null;
34+
packageMetadata = null;
3135
return new[] { new ValidationResult(ex.Message) };
3236
}
3337

‎src/NuGetGallery/App_Code/ViewHelpers.cshtml‎

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -121,19 +121,6 @@
121121
)
122122
}
123123

124-
@helper AlertPackageVerifyRecommendation()
125-
{
126-
@AlertWarning(
127-
@<text>
128-
Please verify the package metadata below.<br/>
129-
<em>
130-
To make changes, we recommend you cancel the upload, make the edits in the nuspec, and upload a new package.
131-
<a href="https://docs.microsoft.com/en-us/nuget/policies/nuget-faq#managing-packages-on-nugetorg" alt="Read more">Read more</a><br />
132-
</em>
133-
</text>
134-
)
135-
}
136-
137124
@helper ErrorPage(UrlHelper url, System.Web.Mvc.HtmlHelper html, string errorNumber, string errorName, Func<MvcHtmlString, HelperResult> errorTextMain, Func<MvcHtmlString, HelperResult> errorTextSub = null)
138125
{
139126
<section role="main" class="container main-container">

‎src/NuGetGallery/Constants.cs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,8 @@ public static class Constants
9393

9494
public const string DevelopmentEnvironment = "Development";
9595

96+
public const string GitRepository = "git";
97+
9698
public static class ContentNames
9799
{
98100
public static readonly string ReadOnly = "ReadOnly";

‎src/NuGetGallery/Controllers/ApiController.cs‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -591,7 +591,8 @@ private async Task<ActionResult> CreatePackageInternal()
591591
}
592592

593593
NuspecReader nuspec;
594-
var errors = ManifestValidator.Validate(packageToPush.GetNuspec(), out nuspec).ToArray();
594+
PackageMetadata packageMetadata;
595+
var errors = ManifestValidator.Validate(packageToPush.GetNuspec(), out nuspec, out packageMetadata).ToArray();
595596
if (errors.Length > 0)
596597
{
597598
var errorsString = string.Join("', '", errors.Select(error => error.ErrorMessage));
@@ -676,7 +677,7 @@ await PackageDeleteService.HardDeletePackagesAsync(
676677
}
677678

678679
// Perform all the validations we can before adding the package to the entity context.
679-
var beforeValidationResult = await PackageUploadService.ValidateBeforeGeneratePackageAsync(packageToPush);
680+
var beforeValidationResult = await PackageUploadService.ValidateBeforeGeneratePackageAsync(packageToPush, packageMetadata);
680681
var beforeValidationActionResult = GetActionResultOrNull(beforeValidationResult);
681682
if (beforeValidationActionResult != null)
682683
{

‎src/NuGetGallery/Controllers/PackagesController.cs‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -165,15 +165,7 @@ public virtual async Task<ActionResult> UploadPackage()
165165
{
166166
return View(model);
167167
}
168-
169-
var validationResult = await _packageUploadService.ValidateBeforeGeneratePackageAsync(package);
170-
var validationErrorMessage = GetErrorMessageOrNull(validationResult);
171-
if (validationErrorMessage != null)
172-
{
173-
TempData["Message"] = validationErrorMessage;
174-
return View(model);
175-
}
176-
168+
177169
try
178170
{
179171
packageMetadata = PackageMetadata.FromNuspecReader(
@@ -188,6 +180,14 @@ public virtual async Task<ActionResult> UploadPackage()
188180
return View(model);
189181
}
190182

183+
var validationResult = await _packageUploadService.ValidateBeforeGeneratePackageAsync(package, packageMetadata);
184+
var validationErrorMessage = GetErrorMessageOrNull(validationResult);
185+
if (validationErrorMessage != null)
186+
{
187+
TempData["Message"] = validationErrorMessage;
188+
return View(model);
189+
}
190+
191191
var existingPackageRegistration = _packageService.FindPackageRegistrationById(packageMetadata.Id);
192192
bool isAllowed;
193193
IEnumerable<User> accountsAllowedOnBehalfOf = Enumerable.Empty<User>();
@@ -249,6 +249,7 @@ public virtual async Task<JsonResult> UploadPackage(HttpPostedFileBase uploadFil
249249
// If the current user doesn't have the rights to upload the package, the package upload will be rejected by submitting the form.
250250
// Related: https://github.com/NuGet/NuGetGallery/issues/5043
251251
IEnumerable<User> accountsAllowedOnBehalfOf = new[] { currentUser };
252+
PackageMetadata packageMetadata;
252253

253254
using (var uploadStream = uploadFile.InputStream)
254255
{
@@ -291,7 +292,7 @@ public virtual async Task<JsonResult> UploadPackage(HttpPostedFileBase uploadFil
291292
}
292293

293294
NuspecReader nuspec;
294-
var errors = ManifestValidator.Validate(packageArchiveReader.GetNuspec(), out nuspec).ToArray();
295+
var errors = ManifestValidator.Validate(packageArchiveReader.GetNuspec(), out nuspec, out packageMetadata).ToArray();
295296
if (errors.Length > 0)
296297
{
297298
var errorStrings = new List<string>();
@@ -385,7 +386,6 @@ await _packageDeleteService.HardDeletePackagesAsync(
385386
await _uploadFileService.SaveUploadFileAsync(currentUser.Key, uploadStream);
386387
}
387388

388-
PackageMetadata packageMetadata;
389389
IReadOnlyList<string> warnings;
390390
using (Stream uploadedFile = await _uploadFileService.GetUploadFileAsync(currentUser.Key))
391391
{
@@ -413,7 +413,7 @@ await _packageDeleteService.HardDeletePackagesAsync(
413413
return Json(HttpStatusCode.BadRequest, new[] { ex.GetUserSafeMessage() });
414414
}
415415

416-
var validationResult = await _packageUploadService.ValidateBeforeGeneratePackageAsync(package);
416+
var validationResult = await _packageUploadService.ValidateBeforeGeneratePackageAsync(package, packageMetadata);
417417
var validationJsonResult = GetJsonResultOrNull(validationResult);
418418
if (validationJsonResult != null)
419419
{
@@ -1593,7 +1593,7 @@ public virtual async Task<JsonResult> VerifyPackage(VerifyPackageRequest formDat
15931593
}
15941594

15951595
// Perform all the validations we can before adding the package to the entity context.
1596-
var beforeValidationResult = await _packageUploadService.ValidateBeforeGeneratePackageAsync(nugetPackage);
1596+
var beforeValidationResult = await _packageUploadService.ValidateBeforeGeneratePackageAsync(nugetPackage, packageMetadata);
15971597
var beforeValidationJsonResult = GetJsonResultOrNull(beforeValidationResult);
15981598
if (beforeValidationJsonResult != null)
15991599
{

‎src/NuGetGallery/Helpers/PackageHelper.cs‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ public static bool ShouldRenderUrl(string url, bool secureOnly = false)
2424
{
2525
if (secureOnly)
2626
{
27-
return uri.Scheme == Uri.UriSchemeHttps;
27+
return IsHttpsProtocol(uri);
2828
}
2929

3030
return uri.Scheme == Uri.UriSchemeHttps
@@ -34,6 +34,21 @@ public static bool ShouldRenderUrl(string url, bool secureOnly = false)
3434
return false;
3535
}
3636

37+
public static bool IsHttpsProtocol(this Uri uri)
38+
{
39+
return uri.Scheme == Uri.UriSchemeHttps;
40+
}
41+
42+
public static bool IsGitProtocol(this Uri uri)
43+
{
44+
return uri.Scheme == Constants.GitRepository;
45+
}
46+
47+
public static bool IsGitRepositoryType(string repositoryType)
48+
{
49+
return Constants.GitRepository.Equals(repositoryType, StringComparison.OrdinalIgnoreCase);
50+
}
51+
3752
[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Maintainability", "CA1502:AvoidExcessiveComplexity")]
3853
public static void ValidateNuGetPackageMetadata(PackageMetadata packageMetadata)
3954
{
@@ -125,6 +140,11 @@ public static void ValidateNuGetPackageMetadata(PackageMetadata packageMetadata)
125140
{
126141
throw new EntityException(Strings.NuGetPackagePropertyTooLong, "RepositoryType", "100");
127142
}
143+
144+
if (packageMetadata.RepositoryUrl != null && packageMetadata.RepositoryUrl.AbsoluteUri.Length > 4000)
145+
{
146+
throw new EntityException(Strings.NuGetPackagePropertyTooLong, "RepositoryUrl", "4000");
147+
}
128148
}
129149
}
130150
}

‎src/NuGetGallery/Scripts/gallery/async-file-upload.js‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,6 @@
227227
function bindData(model) {
228228
$("#verify-package-block").remove();
229229
$("#submit-block").remove();
230-
$("#verify-warning-container").addClass("hidden");
231230
$("#verify-collapser-container").addClass("hidden");
232231
$("#submit-collapser-container").addClass("hidden");
233232

@@ -270,10 +269,6 @@
270269
$('#icon-preview').attr('src', $('#iconurl-field').val());
271270
});
272271

273-
if ($("#validation-failure-list .alert").length === 0) {
274-
$("#verify-warning-container").removeClass("hidden");
275-
}
276-
277272
$("#verify-collapser-container").removeClass("hidden");
278273
$("#submit-collapser-container").removeClass("hidden");
279274

‎src/NuGetGallery/Services/IPackageUploadService.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ public interface IPackageUploadService
1818
/// </summary>
1919
/// <param name="nuGetPackage">The package archive reader.</param>
2020
/// <returns>The package validation result.</returns>
21-
Task<PackageValidationResult> ValidateBeforeGeneratePackageAsync(PackageArchiveReader nuGetPackage);
21+
Task<PackageValidationResult> ValidateBeforeGeneratePackageAsync(PackageArchiveReader nuGetPackage, PackageMetadata packageMetadata);
2222

2323
Task<Package> GeneratePackageAsync(
2424
string id,

‎src/NuGetGallery/Services/PackageUploadService.cs‎

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,13 +40,21 @@ public PackageUploadService(
4040
_config = config ?? throw new ArgumentNullException(nameof(config));
4141
}
4242

43-
public async Task<PackageValidationResult> ValidateBeforeGeneratePackageAsync(PackageArchiveReader nuGetPackage)
43+
public async Task<PackageValidationResult> ValidateBeforeGeneratePackageAsync(PackageArchiveReader nuGetPackage, PackageMetadata packageMetadata)
4444
{
4545
var warnings = new List<string>();
4646

4747
var result = await CheckForUnsignedPushAfterAuthorSignedAsync(
4848
nuGetPackage,
4949
warnings);
50+
51+
if (result != null)
52+
{
53+
return result;
54+
}
55+
56+
result = CheckRepositoryMetadata(packageMetadata, warnings);
57+
5058
if (result != null)
5159
{
5260
return result;
@@ -55,6 +63,37 @@ public async Task<PackageValidationResult> ValidateBeforeGeneratePackageAsync(Pa
5563
return PackageValidationResult.AcceptedWithWarnings(warnings);
5664
}
5765

66+
/// <summary>
67+
/// Validate repository metadata:
68+
/// 1. If the type is "git" - allow the URL scheme "git://" or "https://". We will translate "git://" to "https://" at display time for known domains.
69+
/// 2. For types other then "git" - URL scheme should be "https://"
70+
/// </summary>
71+
private PackageValidationResult CheckRepositoryMetadata(PackageMetadata packageMetadata, List<string> warnings)
72+
{
73+
if (packageMetadata.RepositoryUrl == null)
74+
{
75+
return null;
76+
}
77+
78+
// git repository type
79+
if (PackageHelper.IsGitRepositoryType(packageMetadata.RepositoryType))
80+
{
81+
if (!packageMetadata.RepositoryUrl.IsGitProtocol() && !packageMetadata.RepositoryUrl.IsHttpsProtocol())
82+
{
83+
warnings.Add(Strings.WarningNotHttpsOrGitRepositoryUrlScheme);
84+
}
85+
}
86+
else
87+
{
88+
if (!packageMetadata.RepositoryUrl.IsHttpsProtocol())
89+
{
90+
warnings.Add(Strings.WarningNotHttpsRepositoryUrlScheme);
91+
}
92+
}
93+
94+
return null;
95+
}
96+
5897
/// <summary>
5998
/// If a package author pushes version X that is author signed then pushes version Y that is unsigned, where Y
6099
/// is immediately after X when the version list is sorted used SemVer 2.0.0 rules, warn the package author.

‎src/NuGetGallery/Services/SymbolPackageService.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ private static void ValidateSymbolPackage(PackageArchiveReader symbolPackage, Pa
122122
PackageHelper.ValidateNuGetPackageMetadata(metadata);
123123

124124
// Validate nuspec manifest.
125-
var errors = ManifestValidator.Validate(symbolPackage.GetNuspec(), out var nuspec).ToArray();
125+
var errors = ManifestValidator.Validate(symbolPackage.GetNuspec(), out var nuspec, out var packageMetadata).ToArray();
126126
if (errors.Length > 0)
127127
{
128128
var errorsString = string.Join("', '", errors.Select(error => error.ErrorMessage));

0 commit comments

Comments
 (0)