From 0d19ccdb8abcddb293df44e426384beef3f2e345 Mon Sep 17 00:00:00 2001 From: John Niang Date: Mon, 24 Jul 2023 17:22:15 +0800 Subject: [PATCH] Delete file already wrote partially into attachment folder when content is terminated with an error (#4286) #### What type of PR is this? /kind bug /area core /milestone 2.8.x #### What this PR does / why we need it: If content is terminated with an error, the file already wrote partially into attachment folder won't be cleaned. Imagine a scenario where we check that the content size is not larger than 2MB when we write content to the attachments folder. Once the limit is reached, files that have been partially written should be cleaned instead of being kept. #### Does this PR introduce a user-facing change? ```release-note None ``` --- .../LocalAttachmentUploadHandler.java | 4 ++ .../run/halo/app/infra/utils/FileUtils.java | 18 +++++++++ .../halo/app/infra/utils/FileUtilsTest.java | 39 ++++++++++++------- 3 files changed, 46 insertions(+), 15 deletions(-) diff --git a/application/src/main/java/run/halo/app/core/extension/attachment/endpoint/LocalAttachmentUploadHandler.java b/application/src/main/java/run/halo/app/core/extension/attachment/endpoint/LocalAttachmentUploadHandler.java index 283ea4432..fc97c217f 100644 --- a/application/src/main/java/run/halo/app/core/extension/attachment/endpoint/LocalAttachmentUploadHandler.java +++ b/application/src/main/java/run/halo/app/core/extension/attachment/endpoint/LocalAttachmentUploadHandler.java @@ -3,6 +3,7 @@ package run.halo.app.core.extension.attachment.endpoint; import static java.nio.file.StandardOpenOption.CREATE_NEW; import static run.halo.app.infra.utils.FileNameUtils.randomFileName; import static run.halo.app.infra.utils.FileUtils.checkDirectoryTraversal; +import static run.halo.app.infra.utils.FileUtils.deleteFileSilently; import java.io.IOException; import java.net.URI; @@ -236,6 +237,9 @@ class LocalAttachmentUploadHandler implements AttachmentHandler { var fileName = randomFileName(oldPath.toString(), 4); pathRef.set(oldPath.resolveSibling(fileName)); })) + // Delete file already wrote partially into attachment folder + // in case of content is terminated with an error + .onErrorResume(t -> deleteFileSilently(pathRef.get()).then(Mono.error(t))) .then(Mono.fromSupplier(pathRef::get)); }); } diff --git a/application/src/main/java/run/halo/app/infra/utils/FileUtils.java b/application/src/main/java/run/halo/app/infra/utils/FileUtils.java index 2926fa2e6..44bedaf08 100644 --- a/application/src/main/java/run/halo/app/infra/utils/FileUtils.java +++ b/application/src/main/java/run/halo/app/infra/utils/FileUtils.java @@ -26,6 +26,8 @@ import lombok.extern.slf4j.Slf4j; import org.springframework.lang.NonNull; import org.springframework.util.AntPathMatcher; import org.springframework.util.Assert; +import reactor.core.publisher.Mono; +import reactor.core.scheduler.Schedulers; import run.halo.app.infra.exception.AccessDeniedException; /** @@ -243,6 +245,21 @@ public abstract class FileUtils { } } + public static Mono deleteFileSilently(Path file) { + return Mono.fromSupplier( + () -> { + if (file == null || !Files.isRegularFile(file)) { + return false; + } + try { + return Files.deleteIfExists(file); + } catch (IOException ignored) { + return false; + } + }) + .subscribeOn(Schedulers.boundedElastic()); + } + public static void copy(Path source, Path dest, CopyOption... options) { try { Files.copy(source, dest, options); @@ -277,4 +294,5 @@ public abstract class FileUtils { } }); } + } diff --git a/application/src/test/java/run/halo/app/infra/utils/FileUtilsTest.java b/application/src/test/java/run/halo/app/infra/utils/FileUtilsTest.java index 95c972d50..e4ba7195f 100644 --- a/application/src/test/java/run/halo/app/infra/utils/FileUtilsTest.java +++ b/application/src/test/java/run/halo/app/infra/utils/FileUtilsTest.java @@ -4,7 +4,7 @@ import static java.util.Objects.requireNonNull; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import static run.halo.app.infra.utils.FileUtils.checkDirectoryTraversal; -import static run.halo.app.infra.utils.FileUtils.deleteRecursivelyAndSilently; +import static run.halo.app.infra.utils.FileUtils.deleteFileSilently; import static run.halo.app.infra.utils.FileUtils.jar; import static run.halo.app.infra.utils.FileUtils.unzip; import static run.halo.app.infra.utils.FileUtils.zip; @@ -16,14 +16,17 @@ import java.nio.file.NoSuchFileException; import java.nio.file.Path; import java.nio.file.Paths; import java.util.zip.ZipInputStream; -import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import reactor.test.StepVerifier; import run.halo.app.infra.exception.AccessDeniedException; class FileUtilsTest { + @TempDir + Path tempDirectory; + @Nested class DirectoryTraversalTest { @@ -49,18 +52,6 @@ class FileUtilsTest { @Nested class ZipTest { - Path tempDirectory; - - @BeforeEach - void setUp() throws IOException { - tempDirectory = Files.createTempDirectory("halo-test-fileutils-zip-"); - } - - @AfterEach - void cleanUp() { - deleteRecursivelyAndSilently(tempDirectory); - } - @Test void zipFolderAndUnzip() throws IOException, URISyntaxException { var uri = requireNonNull(getClass().getClassLoader().getResource("folder-to-zip")) @@ -104,5 +95,23 @@ class FileUtilsTest { assertThrows(NoSuchFileException.class, () -> jar(Paths.get("no-such-folder"), tempDirectory.resolve("example.zip"))); } + } + + @Test + void deleteFileSilentlyTest() throws IOException { + StepVerifier.create(deleteFileSilently(null)) + .expectNext(false) + .verifyComplete(); + + StepVerifier.create(deleteFileSilently(tempDirectory)) + .expectNext(false) + .verifyComplete(); + + StepVerifier.create( + deleteFileSilently(Files.createFile(tempDirectory.resolve("for-deleting")))) + .expectNext(true) + .verifyComplete(); + } + } \ No newline at end of file