Warn when deleting symlinks targeting locations outside temp dir (#4306)

Resolves #4303.

Author: marcphilipp

documentation/src/docs/asciidoc/release-notes/release-notes-5.12.0-RC2.adoc

@@ -45,7 +45,9 @@ repository on GitHub.
 [[release-notes-5.12.0-RC2-junit-jupiter-new-features-and-improvements]]
 ==== New Features and Improvements
 
-* ❓
+* The `@TempDir` now warns when deleting symlinks that target locations outside the
+  temporary directory to signal that the target file or directory is _not_ deleted, only
+  the link to it.
 
 
 [[release-notes-5.12.0-RC2-junit-vintage]]

junit-jupiter-api/src/main/java/org/junit/jupiter/api/io/TempDir.java

@@ -84,8 +84,11 @@
  * — when the test method or class has finished execution — JUnit will
  * attempt to clean up the temporary directory by recursively deleting all files
  * and directories in the temporary directory and, finally, the temporary directory
- * itself. In case deletion of a file or directory fails, an {@link IOException}
- * will be thrown that will cause the test or test class to fail.
+ * itself. Symbolic and other types of links, such as junctions on Windows, are
+ * not followed. A warning is logged when deleting a link that targets a
+ * location outside the temporary directory. In case deletion of a file or
+ * directory fails, an {@link IOException} will be thrown that will cause the
+ * test or test class to fail.
  *
  * <p>The {@link #cleanup} attribute allows you to configure the {@link CleanupMode}.
  * If the cleanup mode is set to {@link CleanupMode#NEVER NEVER}, the temporary

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/TempDirectory.java

@@ -404,7 +404,8 @@ private SortedMap<Path, IOException> deleteAllFilesAndDirectories(FileOperations
 				@Override
 				public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) throws IOException {
 					LOGGER.trace(() -> "preVisitDirectory: " + dir);
-					if (isLink(dir)) {
+					if (isLinkWithTargetOutsideTempDir(dir)) {
+						warnAboutLinkWithTargetOutsideTempDir("link", dir);
 						delete(dir);
 						return SKIP_SUBTREE;
 					}
@@ -414,12 +415,6 @@ public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) th
 					return CONTINUE;
 				}
 
-				private boolean isLink(Path dir) throws IOException {
-					// While `Files.walkFileTree` does not follow symbolic links, it may follow other links
-					// such as "junctions" on Windows
-					return !dir.toRealPath().startsWith(rootRealPath);
-				}
-
 				@Override
 				public FileVisitResult visitFileFailed(Path file, IOException exc) {
 					LOGGER.trace(exc, () -> "visitFileFailed: " + file);
@@ -432,8 +427,11 @@ public FileVisitResult visitFileFailed(Path file, IOException exc) {
 				}
 
 				@Override
-				public FileVisitResult visitFile(Path file, BasicFileAttributes attributes) {
+				public FileVisitResult visitFile(Path file, BasicFileAttributes attributes) throws IOException {
 					LOGGER.trace(() -> "visitFile: " + file);
+					if (Files.isSymbolicLink(file) && isLinkWithTargetOutsideTempDir(file)) {
+						warnAboutLinkWithTargetOutsideTempDir("symbolic link", file);
+					}
 					delete(file);
 					return CONTINUE;
 				}
@@ -445,6 +443,27 @@ public FileVisitResult postVisitDirectory(Path dir, IOException exc) {
 					return CONTINUE;
 				}
 
+				private boolean isLinkWithTargetOutsideTempDir(Path path) {
+					// While `Files.walkFileTree` does not follow symbolic links, it may follow other links
+					// such as "junctions" on Windows
+					try {
+						return !path.toRealPath().startsWith(rootRealPath);
+					}
+					catch (IOException e) {
+						LOGGER.trace(e,
+							() -> "Failed to determine real path for " + path + "; assuming it is not a link");
+						return false;
+					}
+				}
+
+				private void warnAboutLinkWithTargetOutsideTempDir(String linkType, Path file) throws IOException {
+					Path realPath = file.toRealPath();
+					LOGGER.warn(() -> String.format(
+						"Deleting %s from location inside of temp dir (%s) "
+								+ "to location outside of temp dir (%s) but not the target file/directory",
+						linkType, file, realPath));
+				}
+
 				private void delete(Path path) {
 					try {
 						fileOperations.delete(path);

jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/CloseablePathTests.java

@@ -43,6 +43,7 @@
 import java.lang.reflect.Method;
 import java.lang.reflect.Parameter;
 import java.nio.file.FileSystem;
+import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.Optional;
 import java.util.logging.Level;
@@ -395,6 +396,65 @@ void onSuccessWithNoException(Class<?> elementType, @TrackLogRecords LogRecordLi
 					.noneMatch(m -> m.startsWith("Skipping cleanup of temp dir"));
 		}
 
+		@DisplayName("deletes symbolic links targeting directory inside temp dir")
+		@ParameterizedTest
+		@ElementTypeSource
+		@DisabledOnOs(WINDOWS)
+		void deletesSymbolicLinksTargetingDirInsideTempDir(Class<?> elementType,
+				@TrackLogRecords LogRecordListener listener) throws IOException {
+
+			reset(factory);
+
+			closeablePath = TempDirectory.createTempDir(factory, ON_SUCCESS, elementType, elementContext,
+				extensionContext);
+			var rootDir = closeablePath.get();
+			assertThat(rootDir).isDirectory();
+
+			var subDir = createDirectory(rootDir.resolve("subDir"));
+			Files.createFile(subDir.resolve("file"));
+			Files.createSymbolicLink(rootDir.resolve("symbolicLink"), subDir);
+
+			closeablePath.close();
+
+			verify(factory).close();
+			assertThat(rootDir).doesNotExist();
+			assertThat(listener.stream(Level.WARNING)).map(LogRecord::getMessage).isEmpty();
+
+		}
+
+		@DisplayName("deletes symbolic links targeting directory outside temp dir")
+		@ParameterizedTest
+		@ElementTypeSource
+		@DisabledOnOs(WINDOWS)
+		void deletesSymbolicLinksTargetingDirOutsideTempDir(Class<?> elementType,
+				@TrackLogRecords LogRecordListener listener) throws IOException {
+
+			reset(factory);
+
+			closeablePath = TempDirectory.createTempDir(factory, ON_SUCCESS, elementType, elementContext,
+				extensionContext);
+			var rootDir = closeablePath.get();
+			assertThat(rootDir).isDirectory();
+
+			var directoryOutsideTempDir = createTempDirectory("junit-");
+			try {
+				var symbolicLink = createSymbolicLink(rootDir.resolve("symbolicLink"), directoryOutsideTempDir);
+
+				closeablePath.close();
+
+				verify(factory).close();
+				assertThat(rootDir).doesNotExist();
+				assertThat(directoryOutsideTempDir).isDirectory();
+				assertThat(listener.stream(Level.WARNING)) //
+						.map(LogRecord::getMessage) //
+						.contains(("Deleting symbolic link from location inside of temp dir (%s) "
+								+ "to location outside of temp dir (%s) but not the target file/directory").formatted(
+									symbolicLink, directoryOutsideTempDir.toRealPath()));
+			}
+			finally {
+				Files.deleteIfExists(directoryOutsideTempDir);
+			}
+		}
 	}
 
 	static class TestCase {

jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/TempDirectoryCleanupTests.java

@@ -24,6 +24,8 @@
 import java.io.IOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.util.logging.Level;
+import java.util.logging.LogRecord;
 
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.MethodOrderer;
@@ -32,9 +34,11 @@
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.TestMethodOrder;
 import org.junit.jupiter.api.condition.EnabledOnOs;
+import org.junit.jupiter.api.fixtures.TrackLogRecords;
 import org.junit.jupiter.api.io.CleanupMode;
 import org.junit.jupiter.api.io.TempDir;
 import org.junit.jupiter.engine.AbstractJupiterTestEngineTests;
+import org.junit.platform.commons.logging.LogRecordListener;
 import org.junit.platform.launcher.LauncherDiscoveryRequest;
 
 /**
@@ -470,7 +474,8 @@ void deletesBrokenJunctions(@TempDir Path dir) throws Exception {
 		}
 
 		@Test
-		void doesNotFollowJunctions(@TempDir Path tempDir) throws IOException {
+		void doesNotFollowJunctions(@TempDir Path tempDir, @TrackLogRecords LogRecordListener listener)
+				throws IOException {
 			var outsideDir = Files.createDirectory(tempDir.resolve("outside"));
 			var testFile = Files.writeString(outsideDir.resolve("test.txt"), "test");
 
@@ -485,6 +490,10 @@ void doesNotFollowJunctions(@TempDir Path tempDir) throws IOException {
 
 			assertThat(outsideDir).exists();
 			assertThat(testFile).exists();
+			assertThat(listener.stream(Level.WARNING)) //
+					.map(LogRecord::getMessage) //
+					.anyMatch(m -> m.matches(
+						"Deleting link from location inside of temp dir \\(.+\\) to location outside of temp dir \\(.+\\) but not the target file/directory"));
 		}
 
 		@SuppressWarnings("JUnitMalformedDeclaration")