fix: download plugin to temp file first, then atomically replace

.github/workflows/build.yml

@@ -21,8 +21,8 @@ jobs:
           java-version: '8'
           cache: 'maven'
 
-      - name: Build with Maven
-        run: mvn -B -DskipTests package
+      - name: Build and test with Maven
+        run: mvn -B clean verify -Dmaven.antrun.skip=true
 
       - name: Upload artifact
         uses: actions/upload-artifact@v4

src/main/java/common/SafeFileReplace.java

@@ -0,0 +1,83 @@
+package common;
+
+import java.io.IOException;
+import java.nio.channels.FileChannel;
+import java.nio.file.AtomicMoveNotSupportedException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.StandardCopyOption;
+import java.nio.file.StandardOpenOption;
+
+final class SafeFileReplace {
+
+    interface ContentWriter {
+        void write(Path temporaryFile) throws IOException;
+    }
+
+    interface AtomicMover {
+        void move(Path source, Path target) throws IOException;
+    }
+
+    private static final AtomicMover DEFAULT_MOVER = (source, target) -> {
+        try {
+            Files.move(source, target,
+                    StandardCopyOption.REPLACE_EXISTING,
+                    StandardCopyOption.ATOMIC_MOVE);
+        } catch (AtomicMoveNotSupportedException unsupported) {
+            throw new IOException("Atomic replacement is not supported for " + target
+                    + "; the original file was preserved", unsupported);
+        }
+    };
+
+    private SafeFileReplace() {
+    }
+
+    static void replace(Path target, ContentWriter writer) throws IOException {
+        replace(target, writer, DEFAULT_MOVER);
+    }
+
+    static void replace(Path target, ContentWriter writer, AtomicMover mover) throws IOException {
+        if (target == null) {
+            throw new IllegalArgumentException("target");
+        }
+        if (writer == null) {
+            throw new IllegalArgumentException("writer");
+        }
+        if (mover == null) {
+            throw new IllegalArgumentException("mover");
+        }
+
+        Path absoluteTarget = target.toAbsolutePath().normalize();
+        Path parent = absoluteTarget.getParent();
+        if (parent == null) {
+            throw new IOException("Target has no parent directory: " + target);
+        }
+
+        Files.createDirectories(parent);
+        String prefix = absoluteTarget.getFileName().toString() + ".";
+        if (prefix.length() < 3) {
+            prefix = "update.";
+        }
+
+        Path temporaryFile = Files.createTempFile(parent, prefix, ".tmp");
+        boolean replaced = false;
+        try {
+            writer.write(temporaryFile);
+
+            if (!Files.isRegularFile(temporaryFile) || Files.size(temporaryFile) == 0L) {
+                throw new IOException("Downloaded update is empty: " + temporaryFile.getFileName());
+            }
+
+            try (FileChannel channel = FileChannel.open(temporaryFile, StandardOpenOption.WRITE)) {
+                channel.force(true);
+            }
+
+            mover.move(temporaryFile, absoluteTarget);
+            replaced = true;
+        } finally {
+            if (!replaced) {
+                Files.deleteIfExists(temporaryFile);
+            }
+        }
+    }
+}

src/main/java/common/UpdateVias.java

@@ -5,11 +5,17 @@
 import com.fasterxml.jackson.databind.node.ArrayNode;
 
 import java.io.File;
-import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
+import java.io.OutputStream;
 import java.net.URL;
 import java.net.URLConnection;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Enumeration;
+import java.util.jar.JarEntry;
+import java.util.jar.JarFile;
 
 import static common.BuildYml.getDownloadedBuild;
 import static common.BuildYml.updateBuildNumber;
@@ -18,6 +24,10 @@ public class UpdateVias {
     private static String directory;
     private static final ObjectMapper MAPPER = new ObjectMapper();
 
+    interface DownloadAction {
+        void download() throws IOException;
+    }
+
     public static boolean updateVia(String viaName, String dataDirectory, boolean wantSnapshot, boolean isDev, boolean isJava8) throws IOException {
         directory = dataDirectory;
 
@@ -46,21 +56,27 @@ public static boolean updateVia(String viaName, String dataDirectory, boolean wa
 
         String localFileName = buildKey.replace("%20", "-");
 
-        if (getDownloadedBuild(buildKey) == -1) {
-            downloadUpdate(jobPath, latestBuild, localFileName);
-            updateBuildNumber(buildKey, latestBuild);
+        int downloadedBuild = getDownloadedBuild(buildKey);
+        if (downloadedBuild == -1) {
+            downloadAndRecordBuild(buildKey, latestBuild,
+                    () -> downloadUpdate(jobPath, latestBuild, localFileName));
             System.out.println(localFileName + " was downloaded for the first time. " + "Please restart to let the plugin take effect.");
             return true;
 
-        } else if (getDownloadedBuild(buildKey) != latestBuild) {
-            downloadUpdate(jobPath, latestBuild, localFileName);
-            updateBuildNumber(buildKey, latestBuild);
+        } else if (downloadedBuild != latestBuild) {
+            downloadAndRecordBuild(buildKey, latestBuild,
+                    () -> downloadUpdate(jobPath, latestBuild, localFileName));
             return true;
         }
 
         return false;
     }
 
+    static void downloadAndRecordBuild(String buildKey, int build, DownloadAction download) throws IOException {
+        download.download();
+        updateBuildNumber(buildKey, build);
+    }
+
     private static int getLatestBuild(String jobPath, boolean wantSnapshot) throws IOException {
         String listUrl = "https://ci.viaversion.com/" + jobPath + "/api/json?tree=builds[number]";
         ArrayNode builds = (ArrayNode) readJson(listUrl).get("builds");
@@ -112,21 +128,54 @@ private static void downloadUpdate(String jobPath, int build, String localName)
             }
         }
 
-        URLConnection conn = new URL(url).openConnection();
-        conn.setConnectTimeout(10000);
-        conn.setReadTimeout(30000);
-        try (InputStream in = conn.getInputStream();
-             FileOutputStream out = new FileOutputStream(outPath)) {
+        downloadToTarget(new URL(url), Paths.get(outPath));
+        System.out.println("New version of " + localName + " downloaded. Please restart the server.");
+    }
 
-            byte[] buf = new byte[1024];
-            int n;
-            while ((n = in.read(buf)) != -1) {
-                out.write(buf, 0, n);
+    static void downloadToTarget(final URL downloadUrl, Path target) throws IOException {
+        SafeFileReplace.replace(target, temporaryFile -> {
+            URLConnection conn = downloadUrl.openConnection();
+            conn.setConnectTimeout(10000);
+            conn.setReadTimeout(30000);
+
+            try (InputStream in = conn.getInputStream();
+                 OutputStream out = Files.newOutputStream(temporaryFile)) {
+                byte[] buffer = new byte[8192];
+                int read;
+                while ((read = in.read(buffer)) != -1) {
+                    out.write(buffer, 0, read);
+                }
             }
-            System.out.println("New version of " + localName + " downloaded. Please restart the server.");
 
-        } catch (IOException e) {
-            System.out.println("Error downloading new version of " + localName + "\n" + e);
+            validateJar(temporaryFile);
+        });
+    }
+
+    static void validateJar(Path candidate) throws IOException {
+        try (JarFile jar = new JarFile(candidate.toFile())) {
+            Enumeration<JarEntry> entries = jar.entries();
+            byte[] buffer = new byte[8192];
+            boolean containsFile = false;
+
+            while (entries.hasMoreElements()) {
+                JarEntry entry = entries.nextElement();
+                if (entry.isDirectory()) {
+                    continue;
+                }
+
+                containsFile = true;
+                try (InputStream in = jar.getInputStream(entry)) {
+                    while (in.read(buffer) != -1) {
+                        // Read every entry so corrupt compressed data is rejected.
+                    }
+                }
+            }
+
+            if (!containsFile) {
+                throw new IOException("Downloaded update JAR contains no files: " + candidate.getFileName());
+            }
+        } catch (IOException invalidJar) {
+            throw new IOException("Downloaded update is not a valid JAR: " + candidate.getFileName(), invalidJar);
         }
     }
 

src/test/java/common/SafeFileReplaceTest.java

@@ -0,0 +1,125 @@
+package common;
+
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.AtomicMoveNotSupportedException;
+import java.nio.file.DirectoryStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.List;
+
+import static org.junit.Assert.assertArrayEquals;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertThrows;
+
+public class SafeFileReplaceTest {
+
+    @Rule
+    public TemporaryFolder temporaryFolder = new TemporaryFolder();
+
+    @Test
+    public void replacesExistingFileAtomically() throws Exception {
+        Path directory = temporaryFolder.newFolder("success").toPath();
+        Path target = directory.resolve("ViaVersion.jar");
+        Files.write(target, bytes("old"));
+        Path[] temporaryFile = new Path[1];
+
+        SafeFileReplace.replace(target, candidate -> {
+            temporaryFile[0] = candidate;
+            assertEquals(directory, candidate.getParent());
+            Files.write(candidate, bytes("new"));
+        });
+
+        assertArrayEquals(bytes("new"), Files.readAllBytes(target));
+        assertFalse(Files.exists(temporaryFile[0]));
+        assertNoTemporaryFiles(directory, target.getFileName().toString());
+    }
+
+    @Test
+    public void preservesTargetAndCleansUpAfterPartialWriteFailure() throws Exception {
+        Path directory = temporaryFolder.newFolder("partial").toPath();
+        Path target = directory.resolve("ViaVersion.jar");
+        Files.write(target, bytes("original"));
+
+        assertThrows(IOException.class, () -> SafeFileReplace.replace(target, candidate -> {
+            Files.write(candidate, bytes("partial"));
+            throw new IOException("simulated connection failure");
+        }));
+
+        assertArrayEquals(bytes("original"), Files.readAllBytes(target));
+        assertNoTemporaryFiles(directory, target.getFileName().toString());
+    }
+
+    @Test
+    public void rejectsEmptyDownloadWithoutReplacingTarget() throws Exception {
+        Path directory = temporaryFolder.newFolder("empty").toPath();
+        Path target = directory.resolve("ViaVersion.jar");
+        Files.write(target, bytes("original"));
+
+        assertThrows(IOException.class,
+                () -> SafeFileReplace.replace(target, candidate -> Files.write(candidate, new byte[0])));
+
+        assertArrayEquals(bytes("original"), Files.readAllBytes(target));
+        assertNoTemporaryFiles(directory, target.getFileName().toString());
+    }
+
+    @Test
+    public void preservesTargetAndCleansUpAfterAtomicMoveFailure() throws Exception {
+        Path directory = temporaryFolder.newFolder("move-failure").toPath();
+        Path target = directory.resolve("ViaVersion.jar");
+        Files.write(target, bytes("original"));
+
+        assertThrows(IOException.class, () -> SafeFileReplace.replace(
+                target,
+                candidate -> Files.write(candidate, bytes("new")),
+                (source, destination) -> {
+                    throw new AtomicMoveNotSupportedException(
+                            source.toString(), destination.toString(), "simulated unsupported move");
+                }));
+
+        assertArrayEquals(bytes("original"), Files.readAllBytes(target));
+        assertNoTemporaryFiles(directory, target.getFileName().toString());
+    }
+
+    @Test
+    public void createsUniqueTemporaryNames() throws Exception {
+        Path directory = temporaryFolder.newFolder("unique").toPath();
+        Path target = directory.resolve("ViaVersion.jar");
+        List<Path> temporaryFiles = new ArrayList<>();
+
+        SafeFileReplace.replace(target, candidate -> {
+            temporaryFiles.add(candidate);
+            Files.write(candidate, bytes("first"));
+        });
+        SafeFileReplace.replace(target, candidate -> {
+            temporaryFiles.add(candidate);
+            Files.write(candidate, bytes("second"));
+        });
+
+        assertEquals(2, temporaryFiles.size());
+        assertNotEquals(temporaryFiles.get(0), temporaryFiles.get(1));
+        assertArrayEquals(bytes("second"), Files.readAllBytes(target));
+        assertNoTemporaryFiles(directory, target.getFileName().toString());
+    }
+
+    private static byte[] bytes(String value) {
+        return value.getBytes(StandardCharsets.UTF_8);
+    }
+
+    private static void assertNoTemporaryFiles(Path directory, String targetName) throws IOException {
+        int count = 0;
+        try (DirectoryStream<Path> files = Files.newDirectoryStream(directory, targetName + ".*.tmp")) {
+            for (Path ignored : files) {
+                count++;
+            }
+        }
+        assertEquals(0, count);
+    }
+}