fix: reliable deletion of node_modules (#12822)

Do not set permissions nor follow symlinks when deleting node_modules. Fixes #12810
vaadin · Jan 25, 2022 · eab25b3 · eab25b3
1 parent 6c2a432
commit eab25b3
Show file tree

Hide file tree

Showing 4 changed files with 98 additions and 10 deletions.
diff --git a/...ugins/flow-maven-plugin/src/main/java/com/vaadin/flow/plugin/maven/CleanFrontendMojo.java b/...ugins/flow-maven-plugin/src/main/java/com/vaadin/flow/plugin/maven/CleanFrontendMojo.java
@@ -26,6 +26,8 @@
 import org.apache.maven.plugins.annotations.LifecyclePhase;
 import org.apache.maven.plugins.annotations.Mojo;
 
+import com.vaadin.flow.server.frontend.FrontendUtils;
+
 import elemental.json.Json;
 import elemental.json.JsonObject;
 import elemental.json.impl.JsonUtil;
@@ -103,15 +105,12 @@ public void execute() throws MojoFailureException {
     private void removeNodeModules() {
         // Remove node_modules folder
         File nodeModules = new File(npmFolder(), "node_modules");
-        if (nodeModules.exists()) {
-            try {
-                FileUtils.deleteDirectory(nodeModules);
-            } catch (IOException exception) {
-                getLog().debug("Exception removing node_modules", exception);
-                getLog().error(
-                        "Failed to remove '" + nodeModules.getAbsolutePath()
-                                + "'. Please remove it manually.");
-            }
+        try {
+            FrontendUtils.deleteNodeModules(nodeModules);
+        } catch (IOException exception) {
+            getLog().debug("Exception removing node_modules", exception);
+            getLog().error("Failed to remove '" + nodeModules.getAbsolutePath()
+                    + "'. Please remove it manually.");
         }
     }
 

diff --git a/flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java b/flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java
@@ -25,16 +25,19 @@
 import java.net.HttpURLConnection;
 import java.net.URL;
 import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
 import java.nio.file.Path;
 import java.time.LocalDateTime;
 import java.time.ZonedDateTime;
 import java.time.format.DateTimeFormatter;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Comparator;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.Scanner;
+import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
 import org.apache.commons.io.FileUtils;
@@ -1215,4 +1218,39 @@ public static void console(String format, Object message) {
         System.out.print(format(format, message));
     }
 
+    /**
+     * Try to remove the {@code node_modules} directory, if it exists inside the
+     * given base directory. Note that pnpm uses symlinks internally, so delete
+     * utilities that follow symlinks when deleting and/or modifying permissions
+     * may not work as intended.
+     *
+     * @param nodeModules
+     *            the {@code node_modules} directory
+     * @throws IOException
+     *             on failure to delete any one file, or if the directory name
+     *             is not {@code node_modules}
+     */
+    public static void deleteNodeModules(File nodeModules) throws IOException {
+        if (!nodeModules.exists()) {
+            return;
+        }
+
+        if (!nodeModules.isDirectory()
+                || !nodeModules.getName().equals("node_modules")) {
+            throw new IOException(nodeModules.getAbsolutePath()
+                    + " does not look like a node_modules directory");
+        }
+
+        Path nodeModulesPath = nodeModules.toPath();
+        try (Stream<Path> walk = Files.walk(nodeModulesPath)) {
+            String undeletable = walk.sorted(Comparator.reverseOrder())
+                    .map(Path::toFile).filter(file -> !file.delete())
+                    .map(File::getAbsolutePath)
+                    .collect(Collectors.joining(", "));
+
+            if (!undeletable.isEmpty()) {
+                throw new IOException("Unable to delete files: " + undeletable);
+            }
+        }
+    }
 }
diff --git a/flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskRunNpmInstall.java b/flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskRunNpmInstall.java
@@ -531,7 +531,7 @@ private void cleanUp() throws ExecutionFailedException {
     private void deleteNodeModules(File nodeModulesFolder)
             throws ExecutionFailedException {
         try {
-            FileUtils.forceDelete(nodeModulesFolder);
+            FrontendUtils.deleteNodeModules(nodeModulesFolder);
         } catch (IOException exception) {
             Logger log = packageUpdater.log();
             log.debug("Exception removing node_modules", exception);

diff --git a/flow-server/src/test/java/com/vaadin/flow/server/frontend/FrontendUtilsTest.java b/flow-server/src/test/java/com/vaadin/flow/server/frontend/FrontendUtilsTest.java
@@ -20,6 +20,7 @@
 import java.io.IOException;
 import java.io.UnsupportedEncodingException;
 import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.Arrays;
 import java.util.List;
@@ -316,6 +317,56 @@ public void clearCachedStatsContent_clearsCache()
                 service.getContext().getAttribute(CACHE_KEY));
     }
 
+    @Test
+    public void deleteNodeModules_nopIfNotExists() throws IOException {
+        File nodeModules = new File(tmpDir.getRoot(), "node_modules");
+        FrontendUtils.deleteNodeModules(nodeModules);
+    }
+
+    @Test(expected = IOException.class)
+    public void deleteNodeModules_throwsIfNotNamedNodeModules()
+            throws IOException {
+        File myModules = new File(tmpDir.getRoot(), "my_modules");
+        myModules.mkdirs();
+        FrontendUtils.deleteNodeModules(myModules);
+    }
+
+    @Test
+    public void deleteNodeModules_canDeleteSymlinksAndNotFollowThem()
+            throws IOException {
+        File externalDir = new File(tmpDir.getRoot(), "external");
+        File externalLicense = new File(externalDir, "LICENSE");
+
+        externalLicense.getParentFile().mkdirs();
+        externalLicense.createNewFile();
+
+        File nodeModules = new File(tmpDir.getRoot(), "node_modules");
+        File containing = new File(nodeModules, ".pnpm/a/node_modules/dep");
+        containing.mkdirs();
+        File license = new File(containing, "LICENSE");
+        license.createNewFile();
+
+        File linking = new File(nodeModules, ".pnpm/b/node_modules/dep");
+        linking.getParentFile().mkdirs();
+        Files.createSymbolicLink(linking.toPath(),
+                new File("../../a/node_modules/dep").toPath());
+
+        File linkingExternal = new File(nodeModules,
+                ".pnpm/b/node_modules/external");
+        Files.createSymbolicLink(linkingExternal.toPath(),
+                new File("../../../../external").toPath());
+
+        Assert.assertTrue(nodeModules.exists());
+        Assert.assertTrue(linking.exists());
+        Assert.assertTrue(new File(linking, "LICENSE").exists());
+        Assert.assertTrue(new File(linkingExternal, "LICENSE").exists());
+
+        FrontendUtils.deleteNodeModules(nodeModules);
+
+        Assert.assertFalse(nodeModules.exists());
+        Assert.assertTrue(externalLicense.exists());
+    }
+
     private ResourceProvider mockResourceProvider(VaadinService service) {
         DeploymentConfiguration config = Mockito
                 .mock(DeploymentConfiguration.class);