From 25456c612c9f9a86cbc97477ba1a225d984ea29d Mon Sep 17 00:00:00 2001 From: Janelle Law Date: Mon, 26 Jul 2021 15:26:06 -0400 Subject: [PATCH] fix(templates): Throw 400 when posting unexpected file upload name (#602) * Throw when posting unexpected file upload * Add itest, fix formatting * Rename itest to match unit test * Remove second unused file upload --- .../web/http/api/v1/TemplatesPostHandler.java | 6 +- .../http/api/v1/TemplatesPostHandlerTest.java | 76 +++++++++-------- src/test/java/itest/TemplatePostIT.java | 84 +++++++++++++++++++ src/test/resources/invalidTemplate.xml | 1 + 4 files changed, 130 insertions(+), 37 deletions(-) create mode 100644 src/test/java/itest/TemplatePostIT.java create mode 100644 src/test/resources/invalidTemplate.xml diff --git a/src/main/java/io/cryostat/net/web/http/api/v1/TemplatesPostHandler.java b/src/main/java/io/cryostat/net/web/http/api/v1/TemplatesPostHandler.java index 91e1291570..90fbb5b630 100644 --- a/src/main/java/io/cryostat/net/web/http/api/v1/TemplatesPostHandler.java +++ b/src/main/java/io/cryostat/net/web/http/api/v1/TemplatesPostHandler.java @@ -110,8 +110,10 @@ public void handleAuthenticated(RoutingContext ctx) throws Exception { Path path = fs.pathOf(u.uploadedFileName()); try (InputStream is = fs.newInputStream(path)) { if (!"template".equals(u.name())) { - logger.info("Received unexpected file upload named {}", u.name()); - continue; + throw new HttpStatusException( + 400, + String.format( + "Received unexpected file upload named {}", u.name())); } notificationFactory .createBuilder() diff --git a/src/test/java/io/cryostat/net/web/http/api/v1/TemplatesPostHandlerTest.java b/src/test/java/io/cryostat/net/web/http/api/v1/TemplatesPostHandlerTest.java index 211f452068..f22ee36cdc 100644 --- a/src/test/java/io/cryostat/net/web/http/api/v1/TemplatesPostHandlerTest.java +++ b/src/test/java/io/cryostat/net/web/http/api/v1/TemplatesPostHandlerTest.java @@ -149,6 +149,29 @@ void shouldThrowIfXmlInvalid() throws Exception { Mockito.verify(fs).deleteIfExists(uploadPath); } + @Test + void shouldThrowIfTemplateUploadNameInvalid() throws Exception { + RoutingContext ctx = Mockito.mock(RoutingContext.class); + + FileUpload upload = Mockito.mock(FileUpload.class); + Mockito.when(upload.name()).thenReturn("invalidUploadName"); + Mockito.when(upload.uploadedFileName()).thenReturn("/file-uploads/abcd-1234"); + + Mockito.when(ctx.fileUploads()).thenReturn(Set.of(upload)); + + Path uploadPath = Mockito.mock(Path.class); + Mockito.when(fs.pathOf("/file-uploads/abcd-1234")).thenReturn(uploadPath); + + InputStream stream = Mockito.mock(InputStream.class); + Mockito.when(fs.newInputStream(Mockito.any())).thenReturn(stream); + + HttpStatusException ex = + Assertions.assertThrows( + HttpStatusException.class, () -> handler.handleAuthenticated(ctx)); + MatcherAssert.assertThat(ex.getStatusCode(), Matchers.equalTo(400)); + Mockito.verify(fs).deleteIfExists(uploadPath); + } + @Test void shouldProcessGoodRequest() throws Exception { RoutingContext ctx = Mockito.mock(RoutingContext.class); @@ -156,32 +179,23 @@ void shouldProcessGoodRequest() throws Exception { HttpServerResponse resp = Mockito.mock(HttpServerResponse.class); Mockito.when(ctx.response()).thenReturn(resp); - FileUpload upload1 = Mockito.mock(FileUpload.class); - Mockito.when(upload1.name()).thenReturn("template"); - Mockito.when(upload1.uploadedFileName()).thenReturn("/file-uploads/abcd-1234"); - - FileUpload upload2 = Mockito.mock(FileUpload.class); - Mockito.when(upload2.name()).thenReturn("unused"); - Mockito.when(upload2.uploadedFileName()).thenReturn("/file-uploads/wxyz-9999"); + FileUpload upload = Mockito.mock(FileUpload.class); + Mockito.when(upload.name()).thenReturn("template"); + Mockito.when(upload.uploadedFileName()).thenReturn("/file-uploads/abcd-1234"); - Mockito.when(ctx.fileUploads()).thenReturn(Set.of(upload1, upload2)); + Mockito.when(ctx.fileUploads()).thenReturn(Set.of(upload)); - Path uploadPath1 = Mockito.mock(Path.class); - Path uploadPath2 = Mockito.mock(Path.class); - Mockito.when(fs.pathOf("/file-uploads/abcd-1234")).thenReturn(uploadPath1); - Mockito.when(fs.pathOf("/file-uploads/wxyz-9999")).thenReturn(uploadPath2); + Path uploadPath = Mockito.mock(Path.class); + Mockito.when(fs.pathOf("/file-uploads/abcd-1234")).thenReturn(uploadPath); - InputStream stream1 = Mockito.mock(InputStream.class); - InputStream stream2 = Mockito.mock(InputStream.class); - Mockito.when(fs.newInputStream(uploadPath1)).thenReturn(stream1); - Mockito.when(fs.newInputStream(uploadPath2)).thenReturn(stream2); + InputStream stream = Mockito.mock(InputStream.class); + Mockito.when(fs.newInputStream(uploadPath)).thenReturn(stream); handler.handleAuthenticated(ctx); - Mockito.verify(templateService).addTemplate(stream1); + Mockito.verify(templateService).addTemplate(stream); Mockito.verifyNoMoreInteractions(templateService); - Mockito.verify(fs).deleteIfExists(uploadPath1); - Mockito.verify(fs).deleteIfExists(uploadPath2); + Mockito.verify(fs).deleteIfExists(uploadPath); Mockito.verify(ctx).response(); Mockito.verify(resp).end(); } @@ -193,25 +207,17 @@ void shouldSendNotifcationOnTemplateDeletion() throws Exception { HttpServerResponse resp = Mockito.mock(HttpServerResponse.class); Mockito.when(ctx.response()).thenReturn(resp); - FileUpload upload1 = Mockito.mock(FileUpload.class); - Mockito.when(upload1.name()).thenReturn("template"); - Mockito.when(upload1.uploadedFileName()).thenReturn("/file-uploads/abcd-1234"); - - FileUpload upload2 = Mockito.mock(FileUpload.class); - Mockito.when(upload2.name()).thenReturn("unused"); - Mockito.when(upload2.uploadedFileName()).thenReturn("/file-uploads/wxyz-9999"); + FileUpload upload = Mockito.mock(FileUpload.class); + Mockito.when(upload.name()).thenReturn("template"); + Mockito.when(upload.uploadedFileName()).thenReturn("/file-uploads/abcd-1234"); - Mockito.when(ctx.fileUploads()).thenReturn(Set.of(upload1, upload2)); + Mockito.when(ctx.fileUploads()).thenReturn(Set.of(upload)); - Path uploadPath1 = Mockito.mock(Path.class); - Path uploadPath2 = Mockito.mock(Path.class); - Mockito.when(fs.pathOf("/file-uploads/abcd-1234")).thenReturn(uploadPath1); - Mockito.when(fs.pathOf("/file-uploads/wxyz-9999")).thenReturn(uploadPath2); + Path uploadPath = Mockito.mock(Path.class); + Mockito.when(fs.pathOf("/file-uploads/abcd-1234")).thenReturn(uploadPath); - InputStream stream1 = Mockito.mock(InputStream.class); - InputStream stream2 = Mockito.mock(InputStream.class); - Mockito.when(fs.newInputStream(uploadPath1)).thenReturn(stream1); - Mockito.when(fs.newInputStream(uploadPath2)).thenReturn(stream2); + InputStream stream = Mockito.mock(InputStream.class); + Mockito.when(fs.newInputStream(uploadPath)).thenReturn(stream); handler.handleAuthenticated(ctx); diff --git a/src/test/java/itest/TemplatePostIT.java b/src/test/java/itest/TemplatePostIT.java new file mode 100644 index 0000000000..5b06db7b3a --- /dev/null +++ b/src/test/java/itest/TemplatePostIT.java @@ -0,0 +1,84 @@ +/* + * Copyright The Cryostat Authors + * + * The Universal Permissive License (UPL), Version 1.0 + * + * Subject to the condition set forth below, permission is hereby granted to any + * person obtaining a copy of this software, associated documentation and/or data + * (collectively the "Software"), free of charge and under any and all copyright + * rights in the Software, and any and all patent rights owned or freely + * licensable by each licensor hereunder covering either (i) the unmodified + * Software as contributed to or provided by such licensor, or (ii) the Larger + * Works (as defined below), to deal in both + * + * (a) the Software, and + * (b) any piece of software and/or hardware listed in the lrgrwrks.txt file if + * one is included with the Software (each a "Larger Work" to which the Software + * is contributed by such licensors), + * + * without restriction, including without limitation the rights to copy, create + * derivative works of, display, perform, and distribute the Software and make, + * use, sell, offer for sale, import, export, have made, and have sold the + * Software and the Larger Work(s), and to sublicense the foregoing rights on + * either these or other terms. + * + * This license is subject to the following condition: + * The above copyright notice and either this complete permission notice or at + * a minimum a reference to the UPL must be included in all copies or + * substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ +package itest; + +import java.io.File; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ExecutionException; + +import io.vertx.ext.web.handler.impl.HttpStatusException; +import io.vertx.ext.web.multipart.MultipartForm; +import itest.bases.StandardSelfTest; +import org.hamcrest.MatcherAssert; +import org.hamcrest.Matchers; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +public class TemplatePostIT extends StandardSelfTest { + static final String FILE_NAME = "invalidTemplate.xml"; + static final String TEMPLATE_NAME = "invalidTemplate"; + static final String MEDIA_TYPE = "application/xml"; + static final String REQ_URL = "/api/v1/templates"; + + @Test + public void shouldThrowIfTemplateUploadNameInvalid() throws Exception { + + CompletableFuture response = new CompletableFuture<>(); + ClassLoader classLoader = getClass().getClassLoader(); + File emptyTemplate = new File(classLoader.getResource(FILE_NAME).getFile()); + String path = emptyTemplate.getAbsolutePath(); + + MultipartForm form = + MultipartForm.create() + .attribute("invalidTemplateAttribute", FILE_NAME) + .binaryFileUpload(TEMPLATE_NAME, FILE_NAME, path, MEDIA_TYPE); + + webClient + .post(REQ_URL) + .sendMultipartForm( + form, + ar -> { + assertRequestStatus(ar, response); + }); + ExecutionException ex = + Assertions.assertThrows(ExecutionException.class, () -> response.get()); + MatcherAssert.assertThat( + ((HttpStatusException) ex.getCause()).getStatusCode(), Matchers.equalTo(400)); + MatcherAssert.assertThat(ex.getCause().getMessage(), Matchers.equalTo("Bad Request")); + } +} diff --git a/src/test/resources/invalidTemplate.xml b/src/test/resources/invalidTemplate.xml new file mode 100644 index 0000000000..2b79f4dc19 --- /dev/null +++ b/src/test/resources/invalidTemplate.xml @@ -0,0 +1 @@ +This is not valid xml.