Normalize the destination directory when extracting zip files (#1125)

Otherwise, relative installDirectory paths cannot be used because the zip-slip
check fails with "Bad zip entry" exception even the zip is fine.

Closes #1124

Author: mvilliger

CHANGELOG.md

@@ -4,6 +4,10 @@ Last public release: [![Maven Central](https://maven-badges.herokuapp.com/maven-
 
 ## Changelog
 
+### 1.14.3
+
+* Prevent `Bad zip entry` exceptions when installing Node to a relative directory ([#1124](https://github.com/eirslett/frontend-maven-plugin/issues/1124))
+
 ### 1.14.2
 
 * Prevent corrupt downloaded files by waiting for the download to complete before writing the file to disk.

frontend-plugin-core/src/main/java/com/github/eirslett/maven/plugins/frontend/lib/ArchiveExtractor.java

@@ -4,13 +4,17 @@
 import org.apache.commons.compress.archivers.tar.TarArchiveInputStream;
 import org.apache.commons.compress.compressors.gzip.GzipCompressorInputStream;
 
+import java.io.BufferedOutputStream;
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.nio.file.AccessDeniedException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.util.Enumeration;
 import java.util.zip.ZipEntry;
 import java.util.zip.ZipFile;
@@ -73,31 +77,29 @@ public void extract(String archive, String destinationDirectory) throws ArchiveE
                             "Unexpected interruption of while waiting for extraction process", e);
                 }
             } else if ("zip".equals(FileUtils.getExtension(archiveFile.getAbsolutePath()))) {
-                ZipFile zipFile = new ZipFile(archiveFile);
-                try {
+                Path destinationPath = Paths.get(destinationDirectory).normalize();
+                try (ZipFile zipFile = new ZipFile(archiveFile)) {
                     Enumeration<? extends ZipEntry> entries = zipFile.entries();
                     while (entries.hasMoreElements()) {
                         ZipEntry entry = entries.nextElement();
-                        final File destPath = new File(destinationDirectory, entry.getName());
-                        if (!destPath.toPath().normalize().startsWith(destinationDirectory)) {
+                        final Path destPath = destinationPath.resolve(entry.getName()).normalize();
+                        if (!destPath.startsWith(destinationPath)) {
                             throw new RuntimeException("Bad zip entry");
                         }
-                        prepDestination(destPath, entry.isDirectory());
+                        prepDestination(destPath.toFile(), entry.isDirectory());
                         if (!entry.isDirectory()) {
                             InputStream in = null;
                             OutputStream out = null;
                             try {
                                 in = zipFile.getInputStream(entry);
-                                out = new FileOutputStream(destPath);
+                                out = new BufferedOutputStream(Files.newOutputStream(destPath));
                                 IOUtils.copy(in, out);
                             } finally {
                                 IOUtils.closeQuietly(in);
                                 IOUtils.closeQuietly(out);
                             }
                         }
                     }
-                } finally {
-                    zipFile.close();
                 }
             } else {
                 // TarArchiveInputStream can be constructed with a normal FileInputStream if

frontend-plugin-core/src/test/java/com/github/eirslett/maven/plugins/frontend/lib/DefaultArchiveExtractorTest.java

@@ -17,7 +17,9 @@
 public class DefaultArchiveExtractorTest {
 
     private final String BAD_TAR = "src/test/resources/bad.tgz";
+    private final String BAD_ZIP = "src/test/resources/bad.zip"; // sample zip with zip slip vulnerability
     private final String GOOD_TAR = "src/test/resources/good.tgz";
+    private final String GOOD_ZIP = "src/test/resources/good.zip";
 
     @TempDir
     public File temp;
@@ -48,6 +50,26 @@ public void extractBadTarFile() {
                 extractor.extract(BAD_TAR, temp.getPath()));
     }
 
+    @Test
+    public void extractGoodZipFile() throws Exception {
+        assertGoodZipExtractedTo(temp);
+    }
+
+    @Test
+    public void extractGoodZipFileWithRelTarget() throws Exception {
+        assertGoodZipExtractedTo(createRelPath(temp));
+    }
+
+    @Test
+    public void extractBadZipFile() {
+        assertBadZipThrowsException(temp);
+    }
+
+    @Test
+    public void extractBadZipFileWithRelTarget() throws Exception {
+        assertBadZipThrowsException(createRelPath(temp));
+    }
+
     @Test
     public void extractBadTarFileSymlink() {
         File destination = new File(temp + "/destination");
@@ -56,6 +78,26 @@ public void extractBadTarFileSymlink() {
         Assertions.assertThrows(ArchiveExtractionException.class, () -> extractor.extract(BAD_TAR, link.toString()));
     }
 
+    private void assertBadZipThrowsException(File targetDir) {
+        Assertions.assertThrows(RuntimeException.class, () ->
+                extractor.extract(BAD_ZIP, targetDir.getPath()));
+    }
+
+    private void assertGoodZipExtractedTo(File targetDir) throws Exception {
+        extractor.extract(GOOD_ZIP, targetDir.getPath());
+        String nameOfFileInZip = "zip";
+        Assertions.assertTrue(new File(temp, nameOfFileInZip).isFile(), "zip content not found in target directory");
+    }
+
+    private static File createRelPath(File orig) throws IOException {
+        orig = orig.getCanonicalFile();
+        String dirName = orig.getName();
+        File result = new File(orig, "../" + dirName);
+        Assertions.assertNotEquals(orig, result); // ensure result is different from input
+        Assertions.assertEquals(orig, result.getCanonicalFile()); // ensure result points to same dir
+        return result;
+    }
+
     private Path createSymlinkOrSkipTest(Path link, Path target) {
         try {
             return Files.createSymbolicLink(link, target);