diff --git a/persistence-service-v1/persistence-service-external-api-impl-v1/src/main/java/com/iqser/red/persistence/service/v1/external/api/impl/controller/UploadController.java b/persistence-service-v1/persistence-service-external-api-impl-v1/src/main/java/com/iqser/red/persistence/service/v1/external/api/impl/controller/UploadController.java index 57c5a5387..026e41a59 100644 --- a/persistence-service-v1/persistence-service-external-api-impl-v1/src/main/java/com/iqser/red/persistence/service/v1/external/api/impl/controller/UploadController.java +++ b/persistence-service-v1/persistence-service-external-api-impl-v1/src/main/java/com/iqser/red/persistence/service/v1/external/api/impl/controller/UploadController.java @@ -23,12 +23,11 @@ import org.springframework.web.bind.annotation.RequestPart; import org.springframework.web.bind.annotation.RestController; import org.springframework.web.multipart.MultipartFile; -import com.iqser.red.service.persistence.management.v1.processor.service.FileFormatValidationService; -import com.iqser.red.service.persistence.management.v1.processor.exception.NotAllowedException; -import com.knecon.fforesight.keycloakcommons.security.KeycloakSecurity; import com.iqser.red.service.pdftron.redaction.v1.api.model.ByteContentDocument; import com.iqser.red.service.persistence.management.v1.processor.exception.BadRequestException; +import com.iqser.red.service.persistence.management.v1.processor.exception.NotAllowedException; import com.iqser.red.service.persistence.management.v1.processor.service.AccessControlService; +import com.iqser.red.service.persistence.management.v1.processor.service.FileFormatValidationService; import com.iqser.red.service.persistence.management.v1.processor.service.ReanalysisService; import com.iqser.red.service.persistence.management.v1.processor.service.UploadService; import com.iqser.red.service.persistence.management.v1.processor.service.persistence.AuditPersistenceService; @@ -37,6 +36,7 @@ import com.iqser.red.service.persistence.service.v1.api.external.resource.Upload import com.iqser.red.service.persistence.service.v1.api.shared.model.AuditCategory; import com.iqser.red.service.persistence.service.v1.api.shared.model.FileUploadResult; import com.iqser.red.service.persistence.service.v1.api.shared.model.audit.AuditRequest; +import com.knecon.fforesight.keycloakcommons.security.KeycloakSecurity; import com.knecon.fforesight.tenantcommons.TenantContext; import feign.FeignException; @@ -53,9 +53,9 @@ import lombok.extern.slf4j.Slf4j; @SuppressWarnings("PMD") public class UploadController implements UploadResource { - private static final int THRESHOLD_ENTRIES = 10000; - private static final int THRESHOLD_SIZE = 1000000000; // 1 GB - private static final double THRESHOLD_RATIO = 10; + private static final int THRESHOLD_ENTRIES = 10000; // Maximum number of files allowed + private static final int THRESHOLD_SIZE = 1000000000; // 1 GB total unzipped data + private static final double THRESHOLD_RATIO = 10; // Max allowed compression ratio private final UploadService uploadService; private final ReanalysisService reanalysisService; @@ -72,31 +72,25 @@ public class UploadController implements UploadResource { @Parameter(name = DISABLE_AUTOMATIC_ANALYSIS_PARAM, description = "Disables automatic redaction for the uploaded file, imports only imported redactions") @RequestParam(value = DISABLE_AUTOMATIC_ANALYSIS_PARAM, required = false, defaultValue = "false") boolean disableAutomaticAnalysis) { accessControlService.checkAccessPermissionsToDossier(dossierId); - if (file.getOriginalFilename() == null) { + + String originalFilename = file.getOriginalFilename(); + if (originalFilename == null) { throw new BadRequestException("Could not upload file, no filename provided."); } - var extension = getExtension(file.getOriginalFilename()); - + String extension = getExtension(originalFilename); try { - switch (extension) { - case "zip": - return handleZip(dossierId, file.getBytes(), keepManualRedactions, disableAutomaticAnalysis); - case "csv": - return uploadService.importCsv(dossierId, file.getBytes()); - default: - if (!fileFormatValidationService.getAllFileFormats().contains(extension)) { - throw new BadRequestException("Invalid file uploaded"); - } - if (!fileFormatValidationService.getValidFileFormatsForTenant(TenantContext.getTenantId()).contains(extension)) { - throw new NotAllowedException("Insufficient permissions"); - } - return uploadService.processSingleFile(dossierId, file.getOriginalFilename(), file.getBytes(), keepManualRedactions, disableAutomaticAnalysis); - } + return switch (extension) { + case "zip" -> handleZip(dossierId, file.getBytes(), keepManualRedactions, disableAutomaticAnalysis); + case "csv" -> uploadService.importCsv(dossierId, file.getBytes()); + default -> { + validateExtensionOrThrow(extension); + yield uploadService.processSingleFile(dossierId, originalFilename, file.getBytes(), keepManualRedactions, disableAutomaticAnalysis); + } + }; } catch (IOException e) { - throw new BadRequestException(e.getMessage(), e); + throw new BadRequestException("Failed to process file: " + e.getMessage(), e); } - } @@ -111,7 +105,6 @@ public class UploadController implements UploadResource { accessControlService.verifyUserIsReviewerOrApprover(dossierId, fileId); try { - reanalysisService.importRedactions(ByteContentDocument.builder().dossierId(dossierId).fileId(fileId).document(file.getBytes()).pages(pageInclusionRequest).build()); auditPersistenceService.audit(AuditRequest.builder() @@ -122,84 +115,116 @@ public class UploadController implements UploadResource { .details(Map.of("dossierId", dossierId)) .build()); } catch (IOException e) { - throw new BadRequestException(e.getMessage(), e); + throw new BadRequestException("Failed to import redactions: " + e.getMessage(), e); } catch (FeignException e) { throw processFeignException(e); } } - private String getExtension(String fileName) { + private void validateExtensionOrThrow(String extension) { - return fileName.substring(fileName.lastIndexOf(".") + 1).toLowerCase(Locale.ROOT); + if (!fileFormatValidationService.getAllFileFormats().contains(extension)) { + throw new BadRequestException("Invalid file uploaded (unrecognized extension)."); + } + if (!fileFormatValidationService.getValidFileFormatsForTenant(TenantContext.getTenantId()).contains(extension)) { + throw new NotAllowedException("Insufficient permissions for this file type."); + } } + /** + * 1. Write the uploaded content to a temp ZIP file + * 2. Check the number of entries and reject if too big or if symlinks found + * 3. Unzip and process each file, while checking size and ratio. + */ private FileUploadResult handleZip(String dossierId, byte[] fileContent, boolean keepManualRedactions, boolean disableAutomaticAnalysis) throws IOException { - File tempFile = FileUtils.createTempFile(UUID.randomUUID().toString(), ".zip"); - try (var fileOutputStream = new FileOutputStream(tempFile)) { - IOUtils.write(fileContent, fileOutputStream); + File tempZip = FileUtils.createTempFile(UUID.randomUUID().toString(), ".zip"); + try (FileOutputStream fos = new FileOutputStream(tempZip)) { + IOUtils.write(fileContent, fos); } - try { - checkForSymlinks(tempFile); + validateZipEntries(tempZip); - var zipData = unzip(tempFile, dossierId, keepManualRedactions, disableAutomaticAnalysis); + try { + ZipData zipData = processZipContents(tempZip, dossierId, keepManualRedactions, disableAutomaticAnalysis); if (zipData.csvBytes != null) { try { - var importResult = uploadService.importCsv(dossierId, zipData.csvBytes); - zipData.fileUploadResult.getProcessedAttributes().addAll(importResult.getProcessedAttributes()); - zipData.fileUploadResult.getProcessedFileIds().addAll(importResult.getProcessedFileIds()); + FileUploadResult csvResult = uploadService.importCsv(dossierId, zipData.csvBytes); + zipData.fileUploadResult.getProcessedAttributes().addAll(csvResult.getProcessedAttributes()); + zipData.fileUploadResult.getProcessedFileIds().addAll(csvResult.getProcessedFileIds()); } catch (Exception e) { - log.debug("CSV file inside ZIP failed", e); - // TODO return un-processed files to client + log.debug("CSV file inside ZIP failed to import", e); } } else if (zipData.fileUploadResult.getFileIds().isEmpty()) { if (zipData.containedUnpermittedFiles) { - throw new NotAllowedException("Zip file contains unpermitted files"); + throw new NotAllowedException("Zip file contains unpermitted files."); } else { - throw new BadRequestException("Only unsupported files in zip file"); + throw new BadRequestException("Only unsupported files in the ZIP."); } } return zipData.fileUploadResult; - } finally { - boolean isDeleted = tempFile.delete(); - if (!isDeleted) { - log.warn("tempFile could not be deleted"); + + if (!tempZip.delete()) { + log.warn("Could not delete temporary ZIP file: {}", tempZip); } } } - private void checkForSymlinks(File tempFile) throws IOException { + private void validateZipEntries(File tempZip) throws IOException { + + try (FileInputStream fis = new FileInputStream(tempZip); ZipFile zipFile = new ZipFile(fis.getChannel())) { + + int count = 0; + var entries = zipFile.getEntries(); + while (entries.hasMoreElements()) { + ZipArchiveEntry ze = entries.nextElement(); - try (var fis = new FileInputStream(tempFile); var zipFile = new ZipFile(fis.getChannel())) { - for (var entryEnum = zipFile.getEntries(); entryEnum.hasMoreElements(); ) { - var ze = entryEnum.nextElement(); if (ze.isUnixSymlink()) { - throw new BadRequestException("ZIP-files with symlinks are not allowed"); + throw new BadRequestException("ZIP-files with symlinks are not allowed."); + } + + if (!ze.isDirectory() && !ze.getName().startsWith(".")) { + count++; + if (count > THRESHOLD_ENTRIES) { + throw new BadRequestException("ZIP-Bomb detected: too many entries."); + } } } } } - private ZipData unzip(File tempFile, String dossierId, boolean keepManualRedactions, boolean disableAutomaticAnalysis) throws IOException { + private ZipData processZipContents(File tempZip, String dossierId, boolean keepManualRedactions, boolean disableAutomaticAnalysis) throws IOException { - var zipData = new ZipData(); + ZipData zipData = new ZipData(); - try (var fis = new FileInputStream(tempFile); var zipFile = new ZipFile(fis.getChannel())) { + try (FileInputStream fis = new FileInputStream(tempZip); ZipFile zipFile = new ZipFile(fis.getChannel())) { - for (var entryEnum = zipFile.getEntries(); entryEnum.hasMoreElements(); ) { - var ze = entryEnum.nextElement(); - zipData.totalEntryArchive++; + var entries = zipFile.getEntries(); + while (entries.hasMoreElements()) { + ZipArchiveEntry entry = entries.nextElement(); - if (!ze.isDirectory()) { - processFileZipEntry(ze, zipFile, dossierId, keepManualRedactions, zipData, disableAutomaticAnalysis); + if (entry.isDirectory() || entry.getName().startsWith(".")) { + continue; + } + + byte[] entryBytes = readEntryWithRatioCheck(entry, zipFile); + zipData.totalSizeArchive += entryBytes.length; + if (zipData.totalSizeArchive > THRESHOLD_SIZE) { + throw new BadRequestException("ZIP-Bomb detected (exceeds total size limit)."); + } + + String extension = getExtension(entry.getName()); + if ("csv".equalsIgnoreCase(extension)) { + zipData.csvBytes = entryBytes; + } else { + handleRegularFile(dossierId, entryBytes, extension, extractFileName(entry.getName()), zipData, keepManualRedactions, disableAutomaticAnalysis); } } } @@ -207,73 +232,70 @@ public class UploadController implements UploadResource { } - private void processFileZipEntry(ZipArchiveEntry ze, ZipFile zipFile, String dossierId, boolean keepManualRedactions, ZipData zipData, boolean disableAutomaticAnalysis) throws IOException { + private byte[] readEntryWithRatioCheck(ZipArchiveEntry entry, ZipFile zipFile) throws IOException { - var extension = getExtension(ze.getName()); + long compressedSize = entry.getCompressedSize() > 0 ? entry.getCompressedSize() : 1; + try (var is = zipFile.getInputStream(entry); var bos = new ByteArrayOutputStream()) { - final String fileName; - if (ze.getName().lastIndexOf("/") >= 0) { - fileName = ze.getName().substring(ze.getName().lastIndexOf("/") + 1); - } else { - fileName = ze.getName(); - } + byte[] buffer = new byte[4096]; + int bytesRead; + int totalUncompressed = 0; - if (fileName.startsWith(".")) { - return; - } + while ((bytesRead = is.read(buffer)) != -1) { + bos.write(buffer, 0, bytesRead); + totalUncompressed += bytesRead; - var entryAsBytes = readCurrentZipEntry(ze, zipFile); - zipData.totalSizeArchive += entryAsBytes.length; - - // 1. the uncompressed data size is too much for the application resource capacity - // 2. too many entries in the archive can lead to inode exhaustion of the file-system - if (zipData.totalSizeArchive > THRESHOLD_SIZE || zipData.totalEntryArchive > THRESHOLD_ENTRIES) { - throw new BadRequestException("ZIP-Bomb detected."); - } - - if ("csv".equals(extension)) { - zipData.csvBytes = entryAsBytes; - } else if (fileFormatValidationService.getAllFileFormats().contains(extension)) { - - if (!fileFormatValidationService.getValidFileFormatsForTenant(TenantContext.getTenantId()).contains(extension)) { - zipData.containedUnpermittedFiles = true; - return; - } - zipData.containedUnpermittedFiles = false; - - try { - var result = uploadService.processSingleFile(dossierId, fileName, entryAsBytes, keepManualRedactions, disableAutomaticAnalysis); - zipData.fileUploadResult.getFileIds().addAll(result.getFileIds()); - } catch (Exception e) { - log.debug("PDF File inside ZIP failed", e); - // TODO return un-processed files to client + double ratio = (double) totalUncompressed / compressedSize; + if (ratio > THRESHOLD_RATIO) { + throw new BadRequestException("ZIP-Bomb detected (compression ratio too high)."); + } } + return bos.toByteArray(); } } - private byte[] readCurrentZipEntry(ZipArchiveEntry ze, ZipFile zipFile) throws IOException { + private void handleRegularFile(String dossierId, + byte[] fileBytes, + String extension, + String fileName, + ZipData zipData, + boolean keepManualRedactions, + boolean disableAutomaticAnalysis) { - var bos = new ByteArrayOutputStream(); - - try (var entryStream = zipFile.getInputStream(ze)) { - var buffer = new byte[2048]; - var nBytes = 0; - int totalSizeEntry = 0; - - while ((nBytes = entryStream.read(buffer)) > 0) { - bos.write(buffer, 0, nBytes); - totalSizeEntry += nBytes; - - double compressionRatio = (float) totalSizeEntry / ze.getCompressedSize(); - if (compressionRatio > THRESHOLD_RATIO) { - // ratio between compressed and uncompressed data is highly suspicious, looks like a Zip Bomb Attack - throw new BadRequestException("ZIP-Bomb detected."); - } - } + if (!fileFormatValidationService.getAllFileFormats().contains(extension)) { + zipData.containedUnpermittedFiles = false; + return; } - return bos.toByteArray(); + if (!fileFormatValidationService.getValidFileFormatsForTenant(TenantContext.getTenantId()).contains(extension)) { + zipData.containedUnpermittedFiles = true; + return; + } + + try { + FileUploadResult result = uploadService.processSingleFile(dossierId, fileName, fileBytes, keepManualRedactions, disableAutomaticAnalysis); + zipData.fileUploadResult.getFileIds().addAll(result.getFileIds()); + } catch (Exception e) { + log.debug("Failed to process file '{}' in ZIP: {}", fileName, e.getMessage(), e); + } + } + + + private String extractFileName(String path) { + + int idx = path.lastIndexOf('/'); + return (idx >= 0) ? path.substring(idx + 1) : path; + } + + + private String getExtension(String fileName) { + + int idx = fileName.lastIndexOf('.'); + if (idx < 0) { + return ""; + } + return fileName.substring(idx + 1).toLowerCase(Locale.ROOT); } @@ -282,7 +304,6 @@ public class UploadController implements UploadResource { byte[] csvBytes; int totalSizeArchive; - int totalEntryArchive; FileUploadResult fileUploadResult = new FileUploadResult(); boolean containedUnpermittedFiles; diff --git a/persistence-service-v1/persistence-service-server-v1/src/test/java/com/iqser/red/service/peristence/v1/server/integration/tests/ZipFileUploadTest.java b/persistence-service-v1/persistence-service-server-v1/src/test/java/com/iqser/red/service/peristence/v1/server/integration/tests/ZipFileUploadTest.java new file mode 100644 index 000000000..385267112 --- /dev/null +++ b/persistence-service-v1/persistence-service-server-v1/src/test/java/com/iqser/red/service/peristence/v1/server/integration/tests/ZipFileUploadTest.java @@ -0,0 +1,79 @@ +package com.iqser.red.service.peristence.v1.server.integration.tests; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.apache.commons.io.IOUtils; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.core.io.ClassPathResource; +import org.springframework.mock.web.MockMultipartFile; +import org.springframework.test.context.TestPropertySource; + +import com.iqser.red.service.peristence.v1.server.integration.client.FileClient; +import com.iqser.red.service.peristence.v1.server.integration.client.UploadClient; +import com.iqser.red.service.peristence.v1.server.integration.service.DossierTesterAndProvider; +import com.iqser.red.service.peristence.v1.server.integration.utils.AbstractPersistenceServerServiceTest; + +import feign.FeignException; +import lombok.SneakyThrows; + +@TestPropertySource(properties = { + "spring.servlet.multipart.max-file-size=50MB", + "spring.servlet.multipart.max-request-size=50MB" +}) +public class ZipFileUploadTest extends AbstractPersistenceServerServiceTest { + + @Autowired + private DossierTesterAndProvider dossierTesterAndProvider; + + @Autowired + private UploadClient uploadClient; + + @Autowired + private FileClient fileClient; + + @SneakyThrows + @Test + void testZipUploadWithEntryCountCheck() { + + var dossier = dossierTesterAndProvider.provideTestDossier(); + + var smallZipResource = new ClassPathResource("files/zip/ArchiveWithManyFiles.zip"); + var smallZip = new MockMultipartFile( + "ArchiveWithManyFiles.zip", + "ArchiveWithManyFiles.zip", + "application/zip", + IOUtils.toByteArray(smallZipResource.getInputStream()) + ); + + var uploadResult = uploadClient.upload(smallZip, dossier.getId(), false, false); + + assertNotNull(uploadResult, "Upload result for small zip should not be null."); + assertEquals(9993, uploadResult.getFileIds().size()); + + var largeZipResource = new ClassPathResource("files/zip/ArchiveWithTooManyFiles.zip"); + var largeZip = new MockMultipartFile( + "ArchiveWithTooManyFiles.zip", + "ArchiveWithTooManyFiles.zip", + "application/zip", + IOUtils.toByteArray(largeZipResource.getInputStream()) + ); + + FeignException ex = assertThrows( + FeignException.class, + () -> uploadClient.upload(largeZip, dossier.getId(), false, false), + "Uploading a zip with more than 10000 entries should throw a FeignException." + ); + + assertEquals(400, ex.status(), "Expected HTTP 400 (Bad Request) for a ZIP bomb scenario"); + assertTrue(ex.getMessage().contains("ZIP-Bomb detected"), + "Exception message should contain 'ZIP-Bomb detected' or similar."); + + var filesInDossier = fileClient.getDossierStatus(dossier.getId()); + assertEquals(9993, filesInDossier.size()); + } +} + diff --git a/persistence-service-v1/persistence-service-server-v1/src/test/resources/files/zip/ArchiveWithManyFiles.zip b/persistence-service-v1/persistence-service-server-v1/src/test/resources/files/zip/ArchiveWithManyFiles.zip new file mode 100644 index 000000000..0925d7a88 Binary files /dev/null and b/persistence-service-v1/persistence-service-server-v1/src/test/resources/files/zip/ArchiveWithManyFiles.zip differ diff --git a/persistence-service-v1/persistence-service-server-v1/src/test/resources/files/zip/ArchiveWithTooManyFiles.zip b/persistence-service-v1/persistence-service-server-v1/src/test/resources/files/zip/ArchiveWithTooManyFiles.zip new file mode 100644 index 000000000..45b520e21 Binary files /dev/null and b/persistence-service-v1/persistence-service-server-v1/src/test/resources/files/zip/ArchiveWithTooManyFiles.zip differ