From 739a7d01f74d2d740ecfb42a7d990830376ce113 Mon Sep 17 00:00:00 2001 From: spokenbird Date: Wed, 21 Aug 2024 11:27:40 -0700 Subject: [PATCH 1/7] Add Apache Tika to check file signature in FileValidationService --- build.gradle | 2 ++ .../library/file/FileValidationService.java | 7 +++++-- .../library/file/FileValidationServiceTest.java | 13 ++++++++++++- src/test/resources/I-am-not-a-png.txt.png | 1 + 4 files changed, 20 insertions(+), 3 deletions(-) create mode 100644 src/test/resources/I-am-not-a-png.txt.png diff --git a/build.gradle b/build.gradle index ef86dd5ee..2eed8ffe9 100644 --- a/build.gradle +++ b/build.gradle @@ -85,6 +85,8 @@ dependencies { implementation 'com.mailgun:mailgun-java:1.1.3' implementation 'com.google.crypto.tink:tink:1.14.1' implementation 'org.springframework.session:spring-session-jdbc' + implementation 'org.apache.tika:tika-core:2.9.2' + // For M1 issue? //runtimeOnly 'io.netty:netty-resolver-dns-native-macos:4.1.97.Final:osx-aarch_64' diff --git a/src/main/java/formflow/library/file/FileValidationService.java b/src/main/java/formflow/library/file/FileValidationService.java index 174a79627..8be038ec1 100644 --- a/src/main/java/formflow/library/file/FileValidationService.java +++ b/src/main/java/formflow/library/file/FileValidationService.java @@ -2,9 +2,11 @@ import static java.util.Arrays.stream; +import java.io.IOException; import java.util.List; import java.util.Map; import lombok.extern.slf4j.Slf4j; +import org.apache.tika.Tika; import org.springframework.beans.factory.annotation.Value; import org.springframework.http.MediaType; import org.springframework.stereotype.Service; @@ -99,11 +101,12 @@ public List getAcceptableFileExts() { * @param file the file to check the mime type of * @return Boolean True if the mimetype is one of the ones the system accepts, False otherwise. */ - public Boolean isAcceptedMimeType(MultipartFile file) { + public Boolean isAcceptedMimeType(MultipartFile file) throws IOException { + Tika tikaFileValidator = new Tika(); if (file.getContentType() == null || file.getContentType().isBlank()) { return false; } - return ACCEPTED_MIME_TYPES.contains(MimeType.valueOf(file.getContentType())); + return ACCEPTED_MIME_TYPES.contains(MimeType.valueOf(tikaFileValidator.detect(file.getInputStream()))); } /** diff --git a/src/test/java/formflow/library/file/FileValidationServiceTest.java b/src/test/java/formflow/library/file/FileValidationServiceTest.java index e398ce4e8..aeba0f629 100644 --- a/src/test/java/formflow/library/file/FileValidationServiceTest.java +++ b/src/test/java/formflow/library/file/FileValidationServiceTest.java @@ -2,14 +2,17 @@ import static org.assertj.core.api.Assertions.assertThat; +import java.io.IOException; import java.util.List; import java.util.stream.Stream; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; +import org.springframework.core.io.ClassPathResource; import org.springframework.http.MediaType; import org.springframework.mock.web.MockMultipartFile; +import org.springframework.web.multipart.MultipartFile; class FileValidationServiceTest { @@ -38,7 +41,7 @@ private static Stream provideMultiPartFiles() { @ParameterizedTest @MethodSource("provideMultiPartFiles") - void isAcceptedMimeTypeReturnsTrueIfAccepted(String contentType, boolean assertion) { + void isAcceptedMimeTypeReturnsTrueIfAccepted(String contentType, boolean assertion) throws IOException { FileValidationService fileValidationService = new FileValidationService(".jpeg,.bmp", 1); MockMultipartFile testFile = new MockMultipartFile("test", "test", contentType, new byte[]{}); @@ -46,6 +49,14 @@ void isAcceptedMimeTypeReturnsTrueIfAccepted(String contentType, boolean asserti } + @Test + void detectsFalseMimeTypes() throws IOException { + ClassPathResource resource = new ClassPathResource("I-am-not-a-png.txt.png"); + MultipartFile file = new MockMultipartFile("file", "I-am-not-a-png.txt.png", MediaType.IMAGE_PNG_VALUE, resource.getInputStream()); + FileValidationService fileValidationService = new FileValidationService(".jpeg,.bmp", 1); + assertThat(fileValidationService.isAcceptedMimeType(file)).isFalse(); + } + @Test void isTooLargeReturnsTrueIfSizeExceedsMax() { FileValidationService fileValidationService = new FileValidationService(".jpeg,.bmp", 1); diff --git a/src/test/resources/I-am-not-a-png.txt.png b/src/test/resources/I-am-not-a-png.txt.png new file mode 100644 index 000000000..30d74d258 --- /dev/null +++ b/src/test/resources/I-am-not-a-png.txt.png @@ -0,0 +1 @@ +test \ No newline at end of file From 3d1bb243ac7af91007c21ab15dc54d0f8563d56a Mon Sep 17 00:00:00 2001 From: spokenbird Date: Thu, 29 Aug 2024 10:51:52 -0700 Subject: [PATCH 2/7] Fix failing tests --- .../controllers/FileControllerTest.java | 22 ++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/src/test/java/formflow/library/controllers/FileControllerTest.java b/src/test/java/formflow/library/controllers/FileControllerTest.java index bbac032a3..d6b84ac6e 100644 --- a/src/test/java/formflow/library/controllers/FileControllerTest.java +++ b/src/test/java/formflow/library/controllers/FileControllerTest.java @@ -21,6 +21,7 @@ import formflow.library.file.ClammitVirusScanner; import formflow.library.file.CloudFile; import formflow.library.file.CloudFileRepository; +import formflow.library.file.FileValidationService; import formflow.library.utilities.AbstractMockMvcTest; import formflow.library.utils.UserFileMap; @@ -75,6 +76,8 @@ public class FileControllerTest extends AbstractMockMvcTest { private FileController fileController; @MockBean private ClammitVirusScanner clammitVirusScanner; + @SpyBean + private FileValidationService fileValidationService; @Captor private ArgumentCaptor userFileArgumentCaptor; private final UUID fileId = UUID.randomUUID(); @@ -122,7 +125,8 @@ public void fileUploadEndpointHitsCloudFileRepositoryAndAddsUserFileToSession() // the "name" param has to match what the endpoint expects: "file" MockMultipartFile testImage = new MockMultipartFile("file", "someImage.jpg", MediaType.IMAGE_JPEG_VALUE, "test".getBytes()); - + when(fileValidationService.isAcceptedMimeType(testImage)).thenReturn(true); + mockMvc.perform(MockMvcRequestBuilders.multipart("/file-upload") .file(testImage) .param("flow", "testFlow") @@ -163,6 +167,7 @@ void shouldShowFileContainsVirusErrorIfClammitScanFindsVirus() throws Exception "X5O!P%@AP[4\\PZX54(P^)7CC)7}$EICAR-STANDARD-ANTIVIRUS-TEST-FILE!$H+H*".getBytes()); when(clammitVirusScanner.virusDetected(testVirusFile)).thenReturn(true); when(submissionRepositoryService.findById(any())).thenReturn(Optional.of(submission)); + when(fileValidationService.isAcceptedMimeType(testVirusFile)).thenReturn(true); mockMvc.perform(MockMvcRequestBuilders.multipart("/file-upload") .file(testVirusFile) @@ -186,7 +191,8 @@ void shouldAllowUploadIfBlockIfUnreachableIsSetToFalse() throws Exception { when(clammitVirusScanner.virusDetected(testImage)).thenThrow( new WebClientResponseException(500, "Failed!", null, null, null)); when(submissionRepositoryService.findById(any())).thenReturn(Optional.of(submission)); - + when(fileValidationService.isAcceptedMimeType(testImage)).thenReturn(true); + mockMvc.perform(MockMvcRequestBuilders.multipart("/file-upload") .file(testImage) .param("flow", "testFlow") @@ -208,7 +214,8 @@ void shouldSetFalseIfVirusScannerDidNotRun() throws Exception { when(clammitVirusScanner.virusDetected(testImage)).thenThrow( new WebClientResponseException(500, "Failed!", null, null, null)); doNothing().when(cloudFileRepository).upload(any(), any()); - + when(fileValidationService.isAcceptedMimeType(testImage)).thenReturn(true); + mockMvc.perform(MockMvcRequestBuilders.multipart("/file-upload") .file(testImage) .param("flow", "testFlow") @@ -227,6 +234,7 @@ void shouldSetFalseIfVirusScannerDidNotRun() throws Exception { void shouldReturn413IfUploadedFileViolatesMaxFileSizeConstraint() throws Exception { MockMultipartFile testImage = new MockMultipartFile("file", "testFileSizeImage.jpg", MediaType.IMAGE_JPEG_VALUE, new byte[(int) (FileUtils.ONE_MB + 1)]); + when(fileValidationService.isAcceptedMimeType(testImage)).thenReturn(true); mockMvc.perform(MockMvcRequestBuilders.multipart("/file-upload") .file(testImage) @@ -242,11 +250,15 @@ void shouldReturn413IfUploadedFileViolatesMaxFileSizeConstraint() throws Excepti @Test void shouldReturn4xxIfUploadFileViolatesMaxFilesConstraint() throws Exception { + MockMultipartFile testImage = new MockMultipartFile("file", "testFileSizeImage.jpg", + MediaType.IMAGE_JPEG_VALUE, new byte[10]); + when(userFileRepositoryService.countBySubmission(submission)).thenReturn(10L); when(submissionRepositoryService.findById(any())).thenReturn(Optional.of(submission)); + when(fileValidationService.isAcceptedMimeType(testImage)).thenReturn(true); + mockMvc.perform(MockMvcRequestBuilders.multipart("/file-upload") - .file(new MockMultipartFile("file", "testFileSizeImage.jpg", - MediaType.IMAGE_JPEG_VALUE, new byte[10])) + .file(testImage) .param("flow", "testFlow") .param("inputName", "dropZoneTestInstance") .param("thumbDataURL", "base64string") From 05b98c82fa2d7530e3ee082cff7f6896c9063599 Mon Sep 17 00:00:00 2001 From: spokenbird Date: Thu, 29 Aug 2024 12:28:53 -0700 Subject: [PATCH 3/7] Fix failing tests 2 --- .../controllers/NoOpVirusScannerTest.java | 7 ++++++- .../controllers/ScreenControllerJourneyTest.java | 4 ++++ .../UploadBlockedIfVirusScanUnreachableTest.java | 9 ++++----- .../library/file/FileValidationServiceTest.java | 16 +++++++++------- 4 files changed, 23 insertions(+), 13 deletions(-) diff --git a/src/test/java/formflow/library/controllers/NoOpVirusScannerTest.java b/src/test/java/formflow/library/controllers/NoOpVirusScannerTest.java index 19d67c9b9..33a896ed6 100644 --- a/src/test/java/formflow/library/controllers/NoOpVirusScannerTest.java +++ b/src/test/java/formflow/library/controllers/NoOpVirusScannerTest.java @@ -11,6 +11,7 @@ import formflow.library.data.SubmissionRepositoryService; import formflow.library.data.UserFile; import formflow.library.data.UserFileRepositoryService; +import formflow.library.file.FileValidationService; import formflow.library.utilities.AbstractMockMvcTest; import java.util.HashMap; import java.util.Map; @@ -21,6 +22,7 @@ import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.boot.test.mock.mockito.MockBean; +import org.springframework.boot.test.mock.mockito.SpyBean; import org.springframework.http.HttpStatus; import org.springframework.http.MediaType; import org.springframework.mock.web.MockMultipartFile; @@ -42,7 +44,9 @@ public class NoOpVirusScannerTest extends AbstractMockMvcTest { private UserFileRepositoryService userFileRepositoryService; @Autowired private FileController fileController; - + @SpyBean + FileValidationService fileValidationService; + @Override @BeforeEach public void setUp() throws Exception { @@ -72,6 +76,7 @@ public void setUp() throws Exception { void shouldBypassRealVirusScanningIfDisabled() throws Exception { MockMultipartFile testImage = new MockMultipartFile("file", "someImage.jpg", MediaType.IMAGE_JPEG_VALUE, "test".getBytes()); + when(fileValidationService.isAcceptedMimeType(testImage)).thenReturn(true); mockMvc.perform(MockMvcRequestBuilders.multipart("/file-upload") .file(testImage) .param("flow", "testFlow") diff --git a/src/test/java/formflow/library/controllers/ScreenControllerJourneyTest.java b/src/test/java/formflow/library/controllers/ScreenControllerJourneyTest.java index 499a0b9eb..98a5e1dc2 100644 --- a/src/test/java/formflow/library/controllers/ScreenControllerJourneyTest.java +++ b/src/test/java/formflow/library/controllers/ScreenControllerJourneyTest.java @@ -1,8 +1,11 @@ package formflow.library.controllers; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.when; import static org.springframework.boot.test.context.SpringBootTest.WebEnvironment.RANDOM_PORT; +import formflow.library.file.FileValidationService; import formflow.library.utilities.AbstractBasePageTest; import java.io.IOException; @@ -10,6 +13,7 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.test.mock.mockito.SpyBean; @SpringBootTest(properties = {"form-flow.path=flows-config/test-flow.yaml"}, webEnvironment = RANDOM_PORT) diff --git a/src/test/java/formflow/library/controllers/UploadBlockedIfVirusScanUnreachableTest.java b/src/test/java/formflow/library/controllers/UploadBlockedIfVirusScanUnreachableTest.java index 7934215f4..05b670121 100644 --- a/src/test/java/formflow/library/controllers/UploadBlockedIfVirusScanUnreachableTest.java +++ b/src/test/java/formflow/library/controllers/UploadBlockedIfVirusScanUnreachableTest.java @@ -10,6 +10,7 @@ import formflow.library.data.SubmissionRepositoryService; import formflow.library.data.UserFileRepositoryService; import formflow.library.file.ClammitVirusScanner; +import formflow.library.file.FileValidationService; import formflow.library.utilities.AbstractMockMvcTest; import java.util.Locale; import java.util.UUID; @@ -33,22 +34,19 @@ public class UploadBlockedIfVirusScanUnreachableTest extends AbstractMockMvcTest { private MockMvc mockMvc; - @MockBean - private SubmissionRepositoryService submissionRepositoryService; @Autowired UserFileRepositoryService userFileRepositoryService; @Autowired private FileController fileController; @MockBean private ClammitVirusScanner clammitVirusScanner; + @MockBean + FileValidationService fileValidationService; @Override @BeforeEach public void setUp() throws Exception { - UUID submissionUUID = UUID.randomUUID(); mockMvc = MockMvcBuilders.standaloneSetup(fileController).build(); - Submission submission = Submission.builder().id(submissionUUID).build(); - //when(submissionRepositoryService.findOrCreate(any())).thenReturn(submission); super.setUp(); } @@ -58,6 +56,7 @@ void shouldPreventUploadAndShowAnErrorIfBlockIfUnreachableIsSetToTrue() throws E MediaType.IMAGE_JPEG_VALUE, "test".getBytes()); when(clammitVirusScanner.virusDetected(testImage)).thenThrow( new WebClientResponseException(500, "Failed!", null, null, null)); + when(fileValidationService.isAcceptedMimeType(testImage)).thenReturn(true); mockMvc.perform(MockMvcRequestBuilders.multipart("/file-upload") .file(testImage) .param("flow", "testFlow") diff --git a/src/test/java/formflow/library/file/FileValidationServiceTest.java b/src/test/java/formflow/library/file/FileValidationServiceTest.java index aeba0f629..318ef45d7 100644 --- a/src/test/java/formflow/library/file/FileValidationServiceTest.java +++ b/src/test/java/formflow/library/file/FileValidationServiceTest.java @@ -3,6 +3,8 @@ import static org.assertj.core.api.Assertions.assertThat; import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; import java.util.List; import java.util.stream.Stream; import org.junit.jupiter.api.Test; @@ -32,20 +34,20 @@ void acceptedFileTypesShouldReturnTheIntersectionOfDefaultTypesWithUserProvidedO private static Stream provideMultiPartFiles() { return Stream.of( - Arguments.of("image/jpeg", true), - Arguments.of("fake/nonsense", false), - Arguments.of(null, false), - Arguments.of("", false) + Arguments.of("test.jpeg", true), + Arguments.of("test-archive.zip", false), + Arguments.of("i-am-not-a-png.txt.png", false) ); } @ParameterizedTest @MethodSource("provideMultiPartFiles") - void isAcceptedMimeTypeReturnsTrueIfAccepted(String contentType, boolean assertion) throws IOException { + void isAcceptedMimeTypeReturnsTrueIfAccepted(String contentName, boolean assertion) throws IOException { FileValidationService fileValidationService = new FileValidationService(".jpeg,.bmp", 1); - MockMultipartFile testFile = new MockMultipartFile("test", "test", contentType, new byte[]{}); + ClassPathResource resource = new ClassPathResource(contentName); + MultipartFile file = new MockMultipartFile("file", contentName, Files.probeContentType(Path.of(contentName)), resource.getInputStream()); - assertThat(fileValidationService.isAcceptedMimeType(testFile)).isEqualTo(assertion); + assertThat(fileValidationService.isAcceptedMimeType(file)).isEqualTo(assertion); } From c6493d04ae1ff886f11a17c89265ba2467b0c9e4 Mon Sep 17 00:00:00 2001 From: spokenbird Date: Thu, 29 Aug 2024 14:06:31 -0700 Subject: [PATCH 4/7] Wait for page title in selenium tests --- src/test/java/formflow/library/utilities/Page.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/test/java/formflow/library/utilities/Page.java b/src/test/java/formflow/library/utilities/Page.java index ec58d5b41..503d781ec 100644 --- a/src/test/java/formflow/library/utilities/Page.java +++ b/src/test/java/formflow/library/utilities/Page.java @@ -3,12 +3,14 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.awaitility.Awaitility.await; +import java.time.Duration; import java.util.List; import java.util.stream.Collectors; import org.openqa.selenium.By; import org.openqa.selenium.By.ByTagName; import org.openqa.selenium.WebElement; import org.openqa.selenium.remote.RemoteWebDriver; +import org.openqa.selenium.support.ui.WebDriverWait; public class Page { @@ -20,6 +22,8 @@ public Page(RemoteWebDriver driver) { } public String getTitle() { + WebDriverWait wait = new WebDriverWait(driver, Duration.ofSeconds(10)); + wait.until(webDriver -> !driver.getTitle().isBlank()); return driver.getTitle(); } From cb33703777ecd9c9f12c8c19b37d50231730c862 Mon Sep 17 00:00:00 2001 From: spokenbird Date: Thu, 29 Aug 2024 14:22:10 -0700 Subject: [PATCH 5/7] Fix failing tests 4 --- .../resources/{I-am-not-a-png.txt.png => i-am-not-a-png.txt.png} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename src/test/resources/{I-am-not-a-png.txt.png => i-am-not-a-png.txt.png} (100%) diff --git a/src/test/resources/I-am-not-a-png.txt.png b/src/test/resources/i-am-not-a-png.txt.png similarity index 100% rename from src/test/resources/I-am-not-a-png.txt.png rename to src/test/resources/i-am-not-a-png.txt.png From 21ccf612dd93183ad6d40bfc14e12519c38ce148 Mon Sep 17 00:00:00 2001 From: spokenbird Date: Thu, 29 Aug 2024 14:26:16 -0700 Subject: [PATCH 6/7] Fix failing tests 5 --- .../java/formflow/library/file/FileValidationServiceTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/formflow/library/file/FileValidationServiceTest.java b/src/test/java/formflow/library/file/FileValidationServiceTest.java index 318ef45d7..24d8c8d50 100644 --- a/src/test/java/formflow/library/file/FileValidationServiceTest.java +++ b/src/test/java/formflow/library/file/FileValidationServiceTest.java @@ -53,7 +53,7 @@ void isAcceptedMimeTypeReturnsTrueIfAccepted(String contentName, boolean asserti @Test void detectsFalseMimeTypes() throws IOException { - ClassPathResource resource = new ClassPathResource("I-am-not-a-png.txt.png"); + ClassPathResource resource = new ClassPathResource("i-am-not-a-png.txt.png"); MultipartFile file = new MockMultipartFile("file", "I-am-not-a-png.txt.png", MediaType.IMAGE_PNG_VALUE, resource.getInputStream()); FileValidationService fileValidationService = new FileValidationService(".jpeg,.bmp", 1); assertThat(fileValidationService.isAcceptedMimeType(file)).isFalse(); From 39c7959c035d39ab4131eaf2b6fb95fc949645f5 Mon Sep 17 00:00:00 2001 From: spokenbird Date: Thu, 29 Aug 2024 14:26:28 -0700 Subject: [PATCH 7/7] Fix failing tests 6 --- .../java/formflow/library/file/FileValidationServiceTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/formflow/library/file/FileValidationServiceTest.java b/src/test/java/formflow/library/file/FileValidationServiceTest.java index 24d8c8d50..23df32f38 100644 --- a/src/test/java/formflow/library/file/FileValidationServiceTest.java +++ b/src/test/java/formflow/library/file/FileValidationServiceTest.java @@ -54,7 +54,7 @@ void isAcceptedMimeTypeReturnsTrueIfAccepted(String contentName, boolean asserti @Test void detectsFalseMimeTypes() throws IOException { ClassPathResource resource = new ClassPathResource("i-am-not-a-png.txt.png"); - MultipartFile file = new MockMultipartFile("file", "I-am-not-a-png.txt.png", MediaType.IMAGE_PNG_VALUE, resource.getInputStream()); + MultipartFile file = new MockMultipartFile("file", "i-am-not-a-png.txt.png", MediaType.IMAGE_PNG_VALUE, resource.getInputStream()); FileValidationService fileValidationService = new FileValidationService(".jpeg,.bmp", 1); assertThat(fileValidationService.isAcceptedMimeType(file)).isFalse(); }