Reject broken ZIPs on API push with better message (#7859)

Address NuGet/Engineering#3046

Author: joelverhagen

src/NuGetGallery/Controllers/ApiController.cs

@@ -549,7 +549,21 @@ private async Task<ActionResult> CreatePackageInternal()
                                 Strings.PackageEntryFromTheFuture,
                                 entryInTheFuture.Name));
                         }
+                    }
+                    catch (Exception ex)
+                    {
+                        // This is not very elegant to catch every Exception type here. However, the types of exceptions
+                        // that could be thrown when reading a garbage ZIP is undocumented. We've seen ArgumentOutOfRangeException
+                        // get thrown from HttpInputStream and InvalidDataException thrown from ZipArchive.
+                        ex.Log();
+
+                        return new HttpStatusCodeWithBodyResult(HttpStatusCode.BadRequest, string.Format(
+                            CultureInfo.CurrentCulture,
+                            Strings.FailedToReadUploadFile));
+                    }
 
+                    try
+                    {
                         using (var packageToPush = new PackageArchiveReader(packageStream, leaveStreamOpen: false))
                         {
                             try

src/NuGetGallery/Controllers/PackagesController.cs

@@ -396,6 +396,9 @@ public virtual async Task<JsonResult> UploadPackage(HttpPostedFileBase uploadFil
                 }
                 catch (Exception ex)
                 {
+                    // This is not very elegant to catch every Exception type here. However, the types of exceptions
+                    // that could be thrown when reading a garbage ZIP is undocumented. We've seen ArgumentOutOfRangeException
+                    // get thrown from HttpInputStream and InvalidDataException thrown from ZipArchive.
                     return FailedToReadFile(ex);
                 }
                 finally

tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs

@@ -696,6 +696,25 @@ public async Task CreatePackageReturns400IfEnsureValidThrowsExceptionMessage(Typ
                 Assert.Equal(expectExceptionMessageInResponse ? EnsureValidExceptionMessage : Strings.FailedToReadUploadFile, (result as HttpStatusCodeWithBodyResult).StatusDescription);
             }
 
+            [Fact]
+            public async Task WillRejectBrokenZipFiles()
+            {
+                // Arrange
+                var package = new MemoryStream(TestDataResourceUtility.GetResourceBytes("Zip64Package.Corrupted.1.0.0.nupkg"));
+
+                var user = new User() { EmailAddress = "[email protected]" };
+                var controller = new TestableApiController(GetConfigurationService());
+                controller.SetCurrentUser(user);
+                controller.SetupPackageFromInputStream(package);
+
+                // Act
+                ActionResult result = await controller.CreatePackagePut();
+
+                // Assert
+                ResultAssert.IsStatusCode(result, HttpStatusCode.BadRequest);
+                Assert.Equal(Strings.FailedToReadUploadFile, (result as HttpStatusCodeWithBodyResult).StatusDescription);
+            }
+
             [Fact]
             public async Task CreatePackageReturns400IfMinClientVersionIsTooHigh()
             {

tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs

@@ -263,7 +263,7 @@ private static PackagesController CreateController(
                     fakeNuGetPackage = TestPackage.CreateTestPackageStream("thePackageId", "1.0.0");
                 }
 
-                controller.Setup(x => x.CreatePackage(It.IsAny<Stream>())).Returns(new PackageArchiveReader(fakeNuGetPackage, true));
+                controller.Setup(x => x.CreatePackage(It.IsAny<Stream>())).Returns(() => new PackageArchiveReader(fakeNuGetPackage, true));
             }
 
             return controller.Object;
@@ -5585,6 +5585,26 @@ public async Task WillShowViewWithErrorsIfEnsureValidThrowsExceptionMessage(Type
                     (result.Data as JsonValidationMessage[])[0].PlainTextMessage);
             }
 
+            [Fact]
+            public async Task WillRejectBrokenZipFiles()
+            {
+                // Arrange
+                var fakeUploadedFile = new Mock<HttpPostedFileBase>();
+                fakeUploadedFile.Setup(x => x.FileName).Returns("file.nupkg");
+                var fakeFileStream = new MemoryStream(TestDataResourceUtility.GetResourceBytes("Zip64Package.Corrupted.1.0.0.nupkg"));
+                fakeUploadedFile.Setup(x => x.InputStream).Returns(fakeFileStream);
+
+                var controller = CreateController(
+                    GetConfigurationService(),
+                    fakeNuGetPackage: fakeFileStream);
+                controller.SetCurrentUser(TestUtility.FakeUser);
+
+                var result = await controller.UploadPackage(fakeUploadedFile.Object) as JsonResult;
+
+                Assert.NotNull(result);
+                Assert.Equal("Central Directory corrupt.", (result.Data as JsonValidationMessage[])[0].PlainTextMessage);
+            }
+
             [Theory]
             [InlineData("ILike*Asterisks")]
             [InlineData("I_.Like.-Separators")]

tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj

@@ -315,6 +315,7 @@
     <EmbeddedResource Include="TestData\certificate.cer" />
     <EmbeddedResource Include="TestData\icon.png" />
     <EmbeddedResource Include="TestData\icon.jpg" />
+    <EmbeddedResource Include="TestData\Zip64Package.Corrupted.1.0.0.nupkg" />
   </ItemGroup>
   <ItemGroup>
     <ProjectReference Include="..\..\src\NuGet.Services.Entities\NuGet.Services.Entities.csproj">