From 223aa0ad948103a6ce068bb9946df41599660b1a Mon Sep 17 00:00:00 2001 From: Philipp Zehnder Date: Fri, 5 Apr 2024 17:02:46 +0200 Subject: [PATCH] fix(#2651): incorrect redirect to dashboard instead of app module (#2689) * fix(#2651): Fix app module and deactivate it for default installation * fix(#2651): Refactor and deprecate AssetDashboardResource * fix(#2651): Add sanitize file name to FileManger * fix(#2651): Check for file type in file manager * fix(#2651): Check for file type in AssetDashboardResource * fix(#2651): Fix e2e test for user roles --- .../streampipes/manager/file/FileManager.java | 55 +++++++-- .../manager/file/TestFileManager.java | 112 ++++++++++++++---- .../rest/impl/AssetDashboardResource.java | 57 +++++++-- .../streampipes/sdk/helpers/Filetypes.java | 9 +- .../sdk/helpers/FiletypesTest.java | 37 ++++++ .../testVariousUserRoles.smoke.spec.ts | 2 +- ui/deployment/dev/config.yml | 1 - .../app-asset-monitoring.component.html | 2 +- .../app-asset-monitoring.module.ts | 4 +- .../app/app-overview/app-overview.module.ts | 4 +- 10 files changed, 233 insertions(+), 50 deletions(-) create mode 100644 streampipes-sdk/src/test/java/org/apache/streampipes/sdk/helpers/FiletypesTest.java diff --git a/streampipes-pipeline-management/src/main/java/org/apache/streampipes/manager/file/FileManager.java b/streampipes-pipeline-management/src/main/java/org/apache/streampipes/manager/file/FileManager.java index 192d124bbe..5038ec3af0 100644 --- a/streampipes-pipeline-management/src/main/java/org/apache/streampipes/manager/file/FileManager.java +++ b/streampipes-pipeline-management/src/main/java/org/apache/streampipes/manager/file/FileManager.java @@ -60,22 +60,33 @@ public static FileMetadata storeFile(String user, String filename, InputStream fileInputStream) throws IOException { - String filetype = filename.substring(filename.lastIndexOf(".") + 1); + var filetype = filename.substring(filename.lastIndexOf(".") + 1); - fileInputStream = cleanFile(fileInputStream, filetype); + fileInputStream = validateFileNameAndCleanFile(filename, filetype, fileInputStream); - FileMetadata fileMetadata = makeFileMetadata(user, filename, filetype); - new FileHandler().storeFile(filename, fileInputStream); - storeFileMetadata(fileMetadata); - return fileMetadata; + var sanitizedFilename = sanitizeFilename(filename); + + writeToFile(sanitizedFilename, fileInputStream); + + return makeAndStoreFileMetadata(user, sanitizedFilename, filetype); } + public static void deleteFile(String id) { FileMetadata fileMetadata = getFileMetadataStorage().getMetadataById(id); new FileHandler().deleteFile(fileMetadata.getFilename()); getFileMetadataStorage().deleteFileMetadata(id); } + private static InputStream validateFileNameAndCleanFile(String filename, + String filetype, + InputStream fileInputStream) { + if (!validateFileType(filename)) { + throw new IllegalArgumentException("Filetype for file %s not allowed".formatted(filename)); + } + + return cleanFile(fileInputStream, filetype); + } /** * Remove Byte Order Mark (BOM) from csv files @@ -84,7 +95,7 @@ public static void deleteFile(String id) { * @param filetype file of type * @return input stream without BOM */ - public static InputStream cleanFile(InputStream fileInputStream, String filetype) { + protected static InputStream cleanFile(InputStream fileInputStream, String filetype) { if (Filetypes.CSV.getFileExtensions().contains(filetype.toLowerCase())) { fileInputStream = new BOMInputStream(fileInputStream); } @@ -97,10 +108,21 @@ public static boolean checkFileContentChanged(String filename, String hash) thro return !fileHash.equals(hash); } - private static void storeFileMetadata(FileMetadata fileMetadata) { - getFileMetadataStorage().addFileMetadata(fileMetadata); + public static String sanitizeFilename(String filename) { + return filename.replaceAll("[^a-zA-Z0-9.\\-]", "_"); + } + + public static boolean validateFileType(String filename) { + return Filetypes.getAllFileExtensions() + .stream() + .anyMatch(filename::endsWith); + } + + protected static void writeToFile(String sanitizedFilename, InputStream fileInputStream) throws IOException { + new FileHandler().storeFile(sanitizedFilename, fileInputStream); } + private static IFileMetadataStorage getFileMetadataStorage() { return StorageDispatcher .INSTANCE @@ -108,6 +130,15 @@ private static IFileMetadataStorage getFileMetadataStorage() { .getFileMetadataStorage(); } + protected static FileMetadata makeAndStoreFileMetadata(String user, + String sanitizedFilename, + String filetype) { + var fileMetadata = makeFileMetadata(user, sanitizedFilename, filetype); + storeFileMetadata(fileMetadata); + + return fileMetadata; + } + private static FileMetadata makeFileMetadata(String user, String filename, String filetype) { @@ -121,6 +152,11 @@ private static FileMetadata makeFileMetadata(String user, return fileMetadata; } + private static void storeFileMetadata(FileMetadata fileMetadata) { + getFileMetadataStorage().addFileMetadata(fileMetadata); + } + + private static List filterFiletypes(List allFiles, String filetypes) { return allFiles .stream() @@ -129,4 +165,5 @@ private static List filterFiletypes(List allFiles, S .anyMatch(ft -> ft.equals(fileMetadata.getFiletype()))) .collect(Collectors.toList()); } + } diff --git a/streampipes-pipeline-management/src/test/java/org/apache/streampipes/manager/file/TestFileManager.java b/streampipes-pipeline-management/src/test/java/org/apache/streampipes/manager/file/TestFileManager.java index 58553e72f6..a02a309fa3 100644 --- a/streampipes-pipeline-management/src/test/java/org/apache/streampipes/manager/file/TestFileManager.java +++ b/streampipes-pipeline-management/src/test/java/org/apache/streampipes/manager/file/TestFileManager.java @@ -18,46 +18,116 @@ package org.apache.streampipes.manager.file; import org.apache.commons.io.IOUtils; -import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import java.io.IOException; import java.io.InputStream; import java.nio.charset.StandardCharsets; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; + public class TestFileManager { + @Test + public void storeFile_throwsExceptionForInvalidFileType() { + var filename = "testFile.invalid"; + + assertThrows(IllegalArgumentException.class, () -> + FileManager.storeFile("", filename, mock(InputStream.class))); + } + @Test public void testCleanFileWithoutBom() throws IOException { - String expected = "test"; - InputStream inputStream = IOUtils.toInputStream(expected, StandardCharsets.UTF_8); - InputStream resultStream = FileManager.cleanFile(inputStream, "CSV"); - String resultString = IOUtils.toString(resultStream, StandardCharsets.UTF_8); + var expected = "test"; + var inputStream = IOUtils.toInputStream(expected, StandardCharsets.UTF_8); + var resultStream = FileManager.cleanFile(inputStream, "CSV"); + var resultString = IOUtils.toString(resultStream, StandardCharsets.UTF_8); - Assertions.assertEquals(expected, resultString); + assertEquals(expected, resultString); } @Test public void testCleanFileWithBom() throws IOException { - String expected = "test"; - String utf8Bom = "\uFEFF"; - String inputString = utf8Bom + expected; - InputStream inputStream = IOUtils.toInputStream(inputString, StandardCharsets.UTF_8); - InputStream resultStream = FileManager.cleanFile(inputStream, "CSV"); - String resultString = IOUtils.toString(resultStream, StandardCharsets.UTF_8); + var expected = "test"; + var utf8Bom = "\uFEFF"; + var inputString = utf8Bom + expected; + var inputStream = IOUtils.toInputStream(inputString, StandardCharsets.UTF_8); + var resultStream = FileManager.cleanFile(inputStream, "CSV"); + var resultString = IOUtils.toString(resultStream, StandardCharsets.UTF_8); - Assertions.assertEquals(expected, resultString); + assertEquals(expected, resultString); } @Test public void testCleanFileWithBomAndUmlauts() throws IOException { - String expected = "testäüö"; - String utf8Bom = "\uFEFF"; - String inputString = utf8Bom + expected; - InputStream inputStream = IOUtils.toInputStream(inputString, StandardCharsets.UTF_8); - InputStream resultStream = FileManager.cleanFile(inputStream, "CSV"); - String resultString = IOUtils.toString(resultStream, StandardCharsets.UTF_8); - - Assertions.assertEquals(expected, resultString); + var expected = "testäüö"; + var utf8Bom = "\uFEFF"; + var inputString = utf8Bom + expected; + var inputStream = IOUtils.toInputStream(inputString, StandardCharsets.UTF_8); + var resultStream = FileManager.cleanFile(inputStream, "CSV"); + var resultString = IOUtils.toString(resultStream, StandardCharsets.UTF_8); + + assertEquals(expected, resultString); + } + + @Test + public void sanitizeFilename_replacesNonAlphanumericCharactersWithUnderscore() { + var filename = "file@name#with$special%characters"; + var sanitizedFilename = FileManager.sanitizeFilename(filename); + assertEquals("file_name_with_special_characters", sanitizedFilename); + } + + @Test + public void sanitizeFilename_keepsAlphanumericAndDotAndHyphenCharacters() { + var filename = "file.name-with_alphanumeric123"; + var sanitizedFilename = FileManager.sanitizeFilename(filename); + assertEquals(filename, sanitizedFilename); + } + + @Test + public void sanitizeFilename_returnsUnderscoreForFilenameWithAllSpecialCharacters() { + var filename = "@#$%^&*()"; + var sanitizedFilename = FileManager.sanitizeFilename(filename); + assertEquals("_________", sanitizedFilename); + } + + @Test + public void sanitizeFilename_returnsEmptyStringForEmptyFilename() { + var filename = ""; + var sanitizedFilename = FileManager.sanitizeFilename(filename); + assertEquals("", sanitizedFilename); + } + + @Test + public void sanitizeFilename_removesSingleParentDirectory() { + var filename = "../file.csv"; + var sanitizedFilename = FileManager.sanitizeFilename(filename); + assertEquals(".._file.csv", sanitizedFilename); + } + + @Test + public void sanitizeFilename_removesDoubleParentDirectoryy() { + var filename = "../../file"; + var sanitizedFilename = FileManager.sanitizeFilename(filename); + assertEquals(".._.._file", sanitizedFilename); + } + + @Test + public void validateFileName_returnsTrueForCsv() { + assertTrue(FileManager.validateFileType("file.csv")); + } + + @Test + public void validateFileName_returnsTrueForJson() { + assertTrue(FileManager.validateFileType("file.json")); + } + + @Test + public void validateFileName_returnsFalseForSh() { + assertFalse(FileManager.validateFileType("file.sh")); } } \ No newline at end of file diff --git a/streampipes-rest/src/main/java/org/apache/streampipes/rest/impl/AssetDashboardResource.java b/streampipes-rest/src/main/java/org/apache/streampipes/rest/impl/AssetDashboardResource.java index 919bea60d9..4433d51ae4 100644 --- a/streampipes-rest/src/main/java/org/apache/streampipes/rest/impl/AssetDashboardResource.java +++ b/streampipes-rest/src/main/java/org/apache/streampipes/rest/impl/AssetDashboardResource.java @@ -17,6 +17,7 @@ */ package org.apache.streampipes.rest.impl; +import org.apache.streampipes.manager.file.FileManager; import org.apache.streampipes.model.client.assetdashboard.AssetDashboardConfig; import org.apache.streampipes.rest.core.base.impl.AbstractRestResource; import org.apache.streampipes.rest.shared.exception.SpMessageException; @@ -43,6 +44,7 @@ import java.io.File; import java.io.IOException; +import java.io.InputStream; import java.net.FileNameMap; import java.net.URLConnection; import java.nio.file.Files; @@ -51,6 +53,7 @@ @RestController @RequestMapping("/api/v2/asset-dashboards") +@Deprecated(forRemoval = true, since = "0.95.0") public class AssetDashboardResource extends AbstractRestResource { private static final Logger LOG = LoggerFactory.getLogger(AssetDashboardResource.class); @@ -59,44 +62,52 @@ public class AssetDashboardResource extends AbstractRestResource { @GetMapping(path = "/{dashboardId}", produces = MediaType.APPLICATION_JSON_VALUE) public ResponseEntity getAssetDashboard(@PathVariable("dashboardId") String dashboardId) { - return ok(getNoSqlStorage().getAssetDashboardStorage().getAssetDashboard(dashboardId)); + return ok(getNoSqlStorage().getAssetDashboardStorage() + .getAssetDashboard(dashboardId)); } @PutMapping( path = "/{dashboardId}", produces = MediaType.APPLICATION_JSON_VALUE, consumes = MediaType.APPLICATION_JSON_VALUE) - public ResponseEntity updateAssetDashboard(@PathVariable("dashboardId") String dashboardId, - @RequestBody AssetDashboardConfig dashboardConfig) { + public ResponseEntity updateAssetDashboard( + @PathVariable("dashboardId") String dashboardId, + @RequestBody AssetDashboardConfig dashboardConfig + ) { AssetDashboardConfig dashboard = getAssetDashboardStorage().getAssetDashboard(dashboardId); dashboardConfig.setRev(dashboard.getRev()); - getNoSqlStorage().getAssetDashboardStorage().updateAssetDashboard(dashboardConfig); + getNoSqlStorage().getAssetDashboardStorage() + .updateAssetDashboard(dashboardConfig); return ok(); } @GetMapping(produces = MediaType.APPLICATION_JSON_VALUE) public ResponseEntity> getAllDashboards() { - return ok(getNoSqlStorage().getAssetDashboardStorage().getAllAssetDashboards()); + return ok(getNoSqlStorage().getAssetDashboardStorage() + .getAllAssetDashboards()); } @PostMapping( produces = MediaType.APPLICATION_JSON_VALUE, consumes = MediaType.APPLICATION_JSON_VALUE) public ResponseEntity storeAssetDashboard(@RequestBody AssetDashboardConfig dashboardConfig) { - getNoSqlStorage().getAssetDashboardStorage().storeAssetDashboard(dashboardConfig); + getNoSqlStorage().getAssetDashboardStorage() + .storeAssetDashboard(dashboardConfig); return ok(); } @DeleteMapping(path = "/{dashboardId}") public ResponseEntity deleteAssetDashboard(@PathVariable("dashboardId") String dashboardId) { - getNoSqlStorage().getAssetDashboardStorage().deleteAssetDashboard(dashboardId); + getNoSqlStorage().getAssetDashboardStorage() + .deleteAssetDashboard(dashboardId); return ok(); } @GetMapping(path = "/images/{imageName}") public ResponseEntity getDashboardImage(@PathVariable("imageName") String imageName) { try { - java.nio.file.Path path = Paths.get(getTargetFile(imageName)); + var sanitizedFileName = FileManager.sanitizeFilename(imageName); + java.nio.file.Path path = Paths.get(getTargetFile(sanitizedFileName)); File file = new File(path.toString()); FileNameMap fileNameMap = URLConnection.getFileNameMap(); String mimeType = fileNameMap.getContentTypeFor(file.getName()); @@ -114,16 +125,35 @@ public ResponseEntity getDashboardImage(@PathVariable("imageName") Strin @PostMapping(path = "/images", consumes = MediaType.MULTIPART_FORM_DATA_VALUE) public ResponseEntity storeDashboardImage(@RequestPart("file_upload") MultipartFile fileDetail) { - File targetDirectory = new File(getTargetDirectory()); + try { + var fileName = fileDetail.getName(); + + if (!FileManager.validateFileType(fileName)) { + LOG.error("File type of file " + fileName + " is not supported"); + fail(); + } + + storeFile(fileDetail.getName(), fileDetail.getInputStream()); + return ok(); + } catch (IOException e) { + LOG.error("Could not extract image input stream from request", e); + return fail(); + } + } + + private void storeFile(String fileName, InputStream fileInputStream) { + + var targetDirectory = new File(getTargetDirectory()); if (!targetDirectory.exists()) { targetDirectory.mkdirs(); } - File targetFile = new File(getTargetFile(fileDetail.getName())); + var sanitizedFileName = FileManager.sanitizeFilename(fileName); + + var targetFile = new File(getTargetFile(sanitizedFileName)); try { - FileUtils.copyInputStreamToFile(fileDetail.getInputStream(), targetFile); - return ok(); + FileUtils.copyInputStreamToFile(fileInputStream, targetFile); } catch (IOException e) { LOG.error(e.getMessage(), e); throw new SpMessageException(HttpStatus.INTERNAL_SERVER_ERROR, e); @@ -140,6 +170,7 @@ private String getTargetFile(String filename) { } private IAssetDashboardStorage getAssetDashboardStorage() { - return StorageDispatcher.INSTANCE.getNoSqlStore().getAssetDashboardStorage(); + return StorageDispatcher.INSTANCE.getNoSqlStore() + .getAssetDashboardStorage(); } } diff --git a/streampipes-sdk/src/main/java/org/apache/streampipes/sdk/helpers/Filetypes.java b/streampipes-sdk/src/main/java/org/apache/streampipes/sdk/helpers/Filetypes.java index 2ac00e6c45..681b180983 100644 --- a/streampipes-sdk/src/main/java/org/apache/streampipes/sdk/helpers/Filetypes.java +++ b/streampipes-sdk/src/main/java/org/apache/streampipes/sdk/helpers/Filetypes.java @@ -19,6 +19,7 @@ import java.util.Arrays; import java.util.List; +import java.util.stream.Stream; public enum Filetypes { @@ -30,7 +31,7 @@ public enum Filetypes { XML("xml"), ZIP("zip"); - private List fileExtensions; + private final List fileExtensions; Filetypes(String... fileExtensions) { this.fileExtensions = Arrays.asList(fileExtensions); @@ -39,4 +40,10 @@ public enum Filetypes { public List getFileExtensions() { return fileExtensions; } + + public static List getAllFileExtensions() { + return Stream.of(Filetypes.values()) + .flatMap(filetype -> filetype.getFileExtensions().stream()) + .toList(); + } } diff --git a/streampipes-sdk/src/test/java/org/apache/streampipes/sdk/helpers/FiletypesTest.java b/streampipes-sdk/src/test/java/org/apache/streampipes/sdk/helpers/FiletypesTest.java new file mode 100644 index 0000000000..8c287f2cbc --- /dev/null +++ b/streampipes-sdk/src/test/java/org/apache/streampipes/sdk/helpers/FiletypesTest.java @@ -0,0 +1,37 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package org.apache.streampipes.sdk.helpers; + +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +class FiletypesTest { + + @Test + public void getAllFileExtensions_returnsAllExtensions() { + var actualExtensions = Filetypes.getAllFileExtensions(); + + assertEquals(8, actualExtensions.size()); + assertTrue(actualExtensions.contains("jpg")); + assertTrue(actualExtensions.contains("jpeg")); + assertTrue(actualExtensions.contains("csv")); + } +} \ No newline at end of file diff --git a/ui/cypress/tests/userManagement/testVariousUserRoles.smoke.spec.ts b/ui/cypress/tests/userManagement/testVariousUserRoles.smoke.spec.ts index 1d5195b1da..d64bfef8d7 100644 --- a/ui/cypress/tests/userManagement/testVariousUserRoles.smoke.spec.ts +++ b/ui/cypress/tests/userManagement/testVariousUserRoles.smoke.spec.ts @@ -40,7 +40,7 @@ for (var i = 0; i < testedRoles.length; i++) { UserUtils.goToUserConfiguration(); cy.dataCy('navigation-icon', { timeout: 10000 }).should( 'have.length', - 11, + 10, ); cy.dataCy('user-accounts-table-row', { timeout: 10000 }).should( diff --git a/ui/deployment/dev/config.yml b/ui/deployment/dev/config.yml index d3980f177b..4edd9074ee 100644 --- a/ui/deployment/dev/config.yml +++ b/ui/deployment/dev/config.yml @@ -23,7 +23,6 @@ modules: - spConnect - spDashboard - spDataExplorer - - spAppOverview - spAdd - spAssets - spFiles diff --git a/ui/src/app/app-asset-monitoring/app-asset-monitoring.component.html b/ui/src/app/app-asset-monitoring/app-asset-monitoring.component.html index 4ec241b38a..7fe7439dc4 100644 --- a/ui/src/app/app-asset-monitoring/app-asset-monitoring.component.html +++ b/ui/src/app/app-asset-monitoring/app-asset-monitoring.component.html @@ -16,7 +16,7 @@ ~ --> - + { return { path: app.appLink,