Skip to content

Commit

Permalink
Warn when unsigned package is pushed after a signed one (#6261)
Browse files Browse the repository at this point in the history 
Address #6184
Address #6253
  • Loading branch information
@joelverhagen
joelverhagen committed Aug 3, 2018
1 parent c32974b commit b87e9f3
Show file tree
Hide file tree
Showing 15 changed files with 892 additions and 488 deletions.
46 changes: 34 additions & 12 deletions src/NuGetGallery/Controllers/ApiController.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.Data;
using System.Data.SqlClient;
using System.Globalization;
Expand Down Expand Up @@ -631,6 +632,14 @@ await PackageDeleteService.HardDeletePackagesAsync(
}
}

// Perform all the validations we can before adding the package to the entity context.
var beforeValidationResult = await PackageUploadService.ValidateBeforeGeneratePackageAsync(packageToPush);
var beforeValidationActionResult = GetActionResultOrNull(beforeValidationResult);
if (beforeValidationActionResult != null)
{
return beforeValidationActionResult;
}

var packageStreamMetadata = new PackageStreamMetadata
{
HashAlgorithm = CoreConstants.Sha512HashAlgorithmId,
Expand All @@ -657,21 +666,16 @@ await PackageDeleteService.HardDeletePackagesAsync(
return new HttpStatusCodeWithBodyResult(HttpStatusCode.BadRequest, packagePolicyResult.ErrorMessage);
}

var validationResult = await PackageUploadService.ValidatePackageAsync(
// Perform validations that require the package already being in the entity context.
var afterValidationResult = await PackageUploadService.ValidateAfterGeneratePackageAsync(
package,
packageToPush,
owner,
currentUser);
switch (validationResult.Type)
var afterValidationActionResult = GetActionResultOrNull(afterValidationResult);
if (afterValidationActionResult != null)
{
case PackageValidationResultType.Accepted:
break;
case PackageValidationResultType.Invalid:
case PackageValidationResultType.PackageShouldNotBeSigned:
case PackageValidationResultType.PackageShouldNotBeSignedButCanManageCertificates:
return new HttpStatusCodeWithBodyResult(HttpStatusCode.BadRequest, validationResult.Message);
default:
throw new NotImplementedException($"The package validation result type {validationResult.Type} is not supported.");
return afterValidationActionResult;
}

await AutoCuratePackage.ExecuteAsync(package, packageToPush, commitChanges: false);
Expand Down Expand Up @@ -724,12 +728,15 @@ await AuditingService.SaveAuditRecordAsync(

TelemetryService.TrackPackagePushEvent(package, currentUser, User.Identity);

var warnings = new List<string>();
warnings.AddRange(beforeValidationResult.Warnings);
warnings.AddRange(afterValidationResult.Warnings);
if (package.SemVerLevelKey == SemVerLevelKey.SemVer2)
{
return new HttpStatusCodeWithServerWarningResult(HttpStatusCode.Created, Strings.WarningSemVer2PackagePushed);
warnings.Add(Strings.WarningSemVer2PackagePushed);
}

return new HttpStatusCodeResult(HttpStatusCode.Created);
return new HttpStatusCodeWithServerWarningResult(HttpStatusCode.Created, warnings);
}
}
catch (InvalidPackageException ex)
Expand Down Expand Up @@ -764,6 +771,21 @@ await AuditingService.SaveAuditRecordAsync(
}
}

private static ActionResult GetActionResultOrNull(PackageValidationResult validationResult)
{
switch (validationResult.Type)
{
case PackageValidationResultType.Accepted:
return null;
case PackageValidationResultType.Invalid:
case PackageValidationResultType.PackageShouldNotBeSigned:
case PackageValidationResultType.PackageShouldNotBeSignedButCanManageCertificates:
return new HttpStatusCodeWithBodyResult(HttpStatusCode.BadRequest, validationResult.Message);
default:
throw new NotImplementedException($"The package validation result type {validationResult.Type} is not supported.");
}
}

private static ActionResult BadRequestForExceptionMessage(Exception ex)
{
return new HttpStatusCodeWithBodyResult(
Expand Down
111 changes: 62 additions & 49 deletions src/NuGetGallery/Controllers/PackagesController.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,13 +160,20 @@ public virtual async Task<ActionResult> UploadPackage()
{
if (uploadedFile != null)
{

var package = await SafeCreatePackage(currentUser, uploadedFile);
if (package == null)
{
return View(model);
}

var validationResult = await _packageUploadService.ValidateBeforeGeneratePackageAsync(package);
var validationErrorMessage = GetErrorMessageOrNull(validationResult);
if (validationErrorMessage != null)
{
TempData["Message"] = validationErrorMessage;
return View(model);
}

try
{
packageMetadata = PackageMetadata.FromNuspecReader(
Expand All @@ -181,8 +188,6 @@ public virtual async Task<ActionResult> UploadPackage()
return View(model);
}

model.IsUploadInProgress = true;

var existingPackageRegistration = _packageService.FindPackageRegistrationById(packageMetadata.Id);
bool isAllowed;
IEnumerable<User> accountsAllowedOnBehalfOf = Enumerable.Empty<User>();
Expand All @@ -204,6 +209,7 @@ public virtual async Task<ActionResult> UploadPackage()
}

var verifyRequest = new VerifyPackageRequest(packageMetadata, accountsAllowedOnBehalfOf, existingPackageRegistration);
verifyRequest.Warnings.AddRange(validationResult.Warnings);

model.InProgressUpload = verifyRequest;
}
Expand All @@ -230,13 +236,11 @@ public virtual async Task<JsonResult> UploadPackage(HttpPostedFileBase uploadFil

if (uploadFile == null)
{
ModelState.AddModelError(String.Empty, Strings.UploadFileIsRequired);
return Json(HttpStatusCode.BadRequest, new[] { Strings.UploadFileIsRequired });
}

if (!Path.GetExtension(uploadFile.FileName).Equals(CoreConstants.NuGetPackageFileExtension, StringComparison.OrdinalIgnoreCase))
{
ModelState.AddModelError(String.Empty, Strings.UploadFileMustBeNuGetPackage);
return Json(HttpStatusCode.BadRequest, new[] { Strings.UploadFileMustBeNuGetPackage });
}

Expand All @@ -257,11 +261,6 @@ public virtual async Task<JsonResult> UploadPackage(HttpPostedFileBase uploadFil

if (entryInTheFuture != null)
{
ModelState.AddModelError(String.Empty, string.Format(
CultureInfo.CurrentCulture,
Strings.PackageEntryFromTheFuture,
entryInTheFuture.Name));

return Json(HttpStatusCode.BadRequest, new[] {
string.Format(CultureInfo.CurrentCulture, Strings.PackageEntryFromTheFuture, entryInTheFuture.Name) });
}
Expand All @@ -284,8 +283,6 @@ public virtual async Task<JsonResult> UploadPackage(HttpPostedFileBase uploadFil
message = ex.Message;
}

ModelState.AddModelError(String.Empty, message);

return Json(HttpStatusCode.BadRequest, new[] { message });
}
finally
Expand All @@ -301,22 +298,14 @@ public virtual async Task<JsonResult> UploadPackage(HttpPostedFileBase uploadFil
foreach (var error in errors)
{
errorStrings.Add(error.ErrorMessage);
ModelState.AddModelError(String.Empty, error.ErrorMessage);
}

return Json(HttpStatusCode.BadRequest, errorStrings);
return Json(HttpStatusCode.BadRequest, errorStrings.ToArray());
}

// Check min client version
if (nuspec.GetMinClientVersion() > Constants.MaxSupportedMinClientVersion)
{
ModelState.AddModelError(
string.Empty,
string.Format(
CultureInfo.CurrentCulture,
Strings.UploadPackage_MinClientVersionOutOfRange,
nuspec.GetMinClientVersion()));

return Json(HttpStatusCode.BadRequest, new[] {
string.Format(CultureInfo.CurrentCulture, Strings.UploadPackage_MinClientVersionOutOfRange, nuspec.GetMinClientVersion()) });
}
Expand All @@ -328,9 +317,6 @@ public virtual async Task<JsonResult> UploadPackage(HttpPostedFileBase uploadFil
ActionsRequiringPermissions.UploadNewPackageId.CheckPermissionsOnBehalfOfAnyAccount(
currentUser, new ActionOnNewPackageContext(id, _reservedNamespaceService), out accountsAllowedOnBehalfOf) != PermissionsCheckResult.Allowed)
{
ModelState.AddModelError(
string.Empty, string.Format(CultureInfo.CurrentCulture, Strings.UploadPackage_IdNamespaceConflict));

var version = nuspec.GetVersion().ToNormalizedString();
_telemetryService.TrackPackagePushNamespaceConflictEvent(id, version, currentUser, User.Identity);

Expand All @@ -343,9 +329,6 @@ public virtual async Task<JsonResult> UploadPackage(HttpPostedFileBase uploadFil
if (ActionsRequiringPermissions.UploadNewPackageVersion.CheckPermissionsOnBehalfOfAnyAccount(
currentUser, existingPackageRegistration, out accountsAllowedOnBehalfOf) != PermissionsCheckResult.Allowed)
{
ModelState.AddModelError(
string.Empty, string.Format(CultureInfo.CurrentCulture, Strings.PackageIdNotAvailable, existingPackageRegistration.Id));

return Json(HttpStatusCode.Conflict, new[] { string.Format(CultureInfo.CurrentCulture, Strings.PackageIdNotAvailable, existingPackageRegistration.Id) });
}

Expand Down Expand Up @@ -395,10 +378,6 @@ await _packageDeleteService.HardDeletePackagesAsync(
existingPackage.Version);
}

ModelState.AddModelError(
string.Empty,
message);

return Json(HttpStatusCode.Conflict, new[] { message });
}
}
Expand All @@ -407,11 +386,11 @@ await _packageDeleteService.HardDeletePackagesAsync(
}

PackageMetadata packageMetadata;
IReadOnlyList<string> warnings;
using (Stream uploadedFile = await _uploadFileService.GetUploadFileAsync(currentUser.Key))
{
if (uploadedFile == null)
{
ModelState.AddModelError(String.Empty, Strings.UploadFileIsRequired);
return Json(HttpStatusCode.BadRequest, new[] { Strings.UploadFileIsRequired });
}

Expand All @@ -433,9 +412,19 @@ await _packageDeleteService.HardDeletePackagesAsync(

return Json(HttpStatusCode.BadRequest, new[] { ex.GetUserSafeMessage() });
}

var validationResult = await _packageUploadService.ValidateBeforeGeneratePackageAsync(package);
var validationJsonResult = GetJsonResultOrNull(validationResult);
if (validationJsonResult != null)
{
return validationJsonResult;
}

warnings = validationResult.Warnings;
}

var model = new VerifyPackageRequest(packageMetadata, accountsAllowedOnBehalfOf, existingPackageRegistration);
model.Warnings.AddRange(warnings);

return Json(model);
}
Expand Down Expand Up @@ -1602,6 +1591,14 @@ public virtual async Task<JsonResult> VerifyPackage(VerifyPackageRequest formDat
}
}

// Perform all the validations we can before adding the package to the entity context.
var beforeValidationResult = await _packageUploadService.ValidateBeforeGeneratePackageAsync(nugetPackage);
var beforeValidationJsonResult = GetJsonResultOrNull(beforeValidationResult);
if (beforeValidationJsonResult != null)
{
return beforeValidationJsonResult;
}

// update relevant database tables
try
{
Expand All @@ -1621,28 +1618,16 @@ public virtual async Task<JsonResult> VerifyPackage(VerifyPackageRequest formDat
return Json(HttpStatusCode.BadRequest, new[] { ex.Message });
}

var validationResult = await _packageUploadService.ValidatePackageAsync(
// Perform validations that require the package already being in the entity context.
var afterValidationResult = await _packageUploadService.ValidateAfterGeneratePackageAsync(
package,
nugetPackage,
owner,
currentUser);
switch (validationResult.Type)
var afterValidationJsonResult = GetJsonResultOrNull(afterValidationResult);
if (afterValidationJsonResult != null)
{
case PackageValidationResultType.Accepted:
break;
case PackageValidationResultType.Invalid:
case PackageValidationResultType.PackageShouldNotBeSigned:
return Json(HttpStatusCode.BadRequest, new[] { validationResult.Message });
case PackageValidationResultType.PackageShouldNotBeSignedButCanManageCertificates:
return Json(
HttpStatusCode.BadRequest,
new[]
{
validationResult.Message + " " +
Strings.UploadPackage_PackageIsSignedButMissingCertificate_ManageCertificate
});
default:
throw new NotImplementedException($"The package validation result type {validationResult.Type} is not supported.");
return afterValidationJsonResult;
}

if (formData.Edit != null)
Expand Down Expand Up @@ -1753,6 +1738,34 @@ await _auditingService.SaveAuditRecordAsync(
}
}

private JsonResult GetJsonResultOrNull(PackageValidationResult validationResult)
{
var errorMessage = GetErrorMessageOrNull(validationResult);
if (errorMessage == null)
{
return null;
}

return Json(HttpStatusCode.BadRequest, new[] { errorMessage });
}

private static string GetErrorMessageOrNull(PackageValidationResult validationResult)
{
switch (validationResult.Type)
{
case PackageValidationResultType.Accepted:
return null;
case PackageValidationResultType.Invalid:
case PackageValidationResultType.PackageShouldNotBeSigned:
return validationResult.Message;
case PackageValidationResultType.PackageShouldNotBeSignedButCanManageCertificates:
return validationResult.Message + " " +
Strings.UploadPackage_PackageIsSignedButMissingCertificate_ManageCertificate;
default:
throw new NotImplementedException($"The package validation result type {validationResult.Type} is not supported.");
}
}

private async Task<PackageArchiveReader> SafeCreatePackage(User currentUser, Stream uploadFile)
{
Exception caught = null;
Expand Down
19 changes: 13 additions & 6 deletions src/NuGetGallery/Infrastructure/HttpStatusCodeWithServerWarningResult.cs
View file
Original file line number Diff line number Diff line change
@@ -1,29 +1,36 @@
// 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.Collections.Generic;
using System.Linq;
using System.Net;
using System.Web.Mvc;
using NuGet.Protocol;

namespace NuGetGallery
{
public class HttpStatusCodeWithServerWarningResult : HttpStatusCodeResult
{
private readonly string _warningMessage;
public IReadOnlyList<string> Warnings { get; }

public HttpStatusCodeWithServerWarningResult(HttpStatusCode statusCode, string warningMessage)
public HttpStatusCodeWithServerWarningResult(HttpStatusCode statusCode, IReadOnlyList<string> warnings)
: base((int)statusCode)
{
_warningMessage = warningMessage;
Warnings = warnings ?? new string[0];
}

public override void ExecuteResult(ControllerContext context)
{
var response = context.RequestContext.HttpContext.Response;

if (!string.IsNullOrEmpty(_warningMessage) && !response.HeadersWritten)
if (Warnings.Any() && !response.HeadersWritten)
{
response.AppendHeader(ProtocolConstants.ServerWarningHeader, _warningMessage);
foreach (var warning in Warnings)
{
if (!string.IsNullOrWhiteSpace(warning))
{
response.AppendHeader(Constants.WarningHeaderName, warning);
}
}
}

base.ExecuteResult(context);
Expand Down
8 changes: 1 addition & 7 deletions src/NuGetGallery/RequestModels/SubmitPackageRequest.cs
View file
Original file line number Diff line number Diff line change
@@ -1,17 +1,11 @@
// 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.ComponentModel.DataAnnotations;
using System.Web;

namespace NuGetGallery
{
public class SubmitPackageRequest
{
[Required]
[Hint("Your package file will be uploaded and hosted on the gallery server.")]
public HttpPostedFile PackageFile { get; set; }

public bool IsUploadInProgress { get; set; }
public bool IsUploadInProgress => InProgressUpload != null;

public VerifyPackageRequest InProgressUpload { get; set; }
}
Expand Down
Loading

0 comments on commit b87e9f3

Please sign in to comment.