ADFA-5257: Share one path-containment check instead of two divergent copies

app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt

@@ -6,6 +6,8 @@ import androidx.annotation.WorkerThread
 import com.aayushatharva.brotli4j.Brotli4jLoader
 import com.itsaky.androidide.app.configuration.IDEBuildConfigProvider
 import com.itsaky.androidide.resources.R
+import com.itsaky.androidide.utils.ContainedPathResolver
+import com.itsaky.androidide.utils.ContainedPathResolver.Resolution
 import com.itsaky.androidide.utils.Environment.DEFAULT_ROOT
 import com.itsaky.androidide.utils.useEntriesEach
 import kotlinx.coroutines.Dispatchers
@@ -29,7 +31,9 @@ import java.io.FileNotFoundException
 import java.io.IOException
 import java.io.InputStream
 import java.nio.file.Files
+import java.nio.file.LinkOption
 import java.nio.file.Path
+import java.nio.file.StandardOpenOption
 import java.util.Locale
 import java.util.UUID
 import java.util.concurrent.ConcurrentHashMap
@@ -254,62 +258,88 @@ object AssetsInstallationHelper {
 		destDir: Path,
 	) = extractZipToDir(Files.newInputStream(srcFile), destDir)
 
+	/**
+	 * Containment is [ContainedPathResolver]'s, shared with `ZipUtils.unzipFile`. It does *not*
+	 * memoize: the per-parent cache this loop used to keep was measured against a real 1.8 GB asset
+	 * installation and bought nothing (48.0s without it, 51.4s with), so every path is re-verified
+	 * against the filesystem rather than trusting an ancestor proven earlier.
+	 *
+	 * What stays local is the policy: this refuses to write through *any* existing symlink at an
+	 * entry's target -- in-base, dangling, or pointing outside destDir -- and says so. An installer
+	 * directory reused across runs is the case that matters, and unlike unzipping a user's project
+	 * there is no legitimate reason for a symlink to be there. The three failure messages are kept
+	 * distinct on purpose: an escaping entry is a hostile archive, a symlink at the target is this
+	 * policy, and unverifiable containment is a filesystem problem -- a 1.8 GB install that dies
+	 * 9,000 entries in should name the real cause (ADFA-5257 review).
+	 */
 	@WorkerThread
 	internal fun extractZipToDir(
 		srcStream: InputStream,
 		destDir: Path,
 	) {
 		Files.createDirectories(destDir)
-		// Normalize and make destDir absolute for secure path validation
-		val normalizedDestDir = destDir.toAbsolutePath().normalize()
-		val realDestDir = normalizedDestDir.toRealPath()
-
-		// Zip entries are commonly clustered by directory (e.g. dozens of files
-		// under the same build-tools/<version>/ prefix); cache the last-verified
-		// parent so consecutive entries under it skip a redundant toRealPath() call.
-		// Nothing below can turn an already-verified real directory into a symlink
-		// mid-run, so caching by lexical parent equality is safe.
-		var lastVerifiedParent: Path? = null
+		val contained = ContainedPathResolver(destDir.toFile())
 
 		ZipInputStream(srcStream.buffered()).useEntriesEach { zipInput, entry ->
-			// Validate entry name doesn't contain dangerous patterns
-			if (entry.name.contains("..") || entry.name.startsWith("/") || entry.name.startsWith("\\")) {
-				throw IllegalStateException("Zip entry contains dangerous path components: ${entry.name}")
+			// A "." or "./" root directory entry names destDir itself, which already exists. The
+			// asset zips are refreshed from an external URL, and archivers that emit such an entry
+			// exist -- a no-op, not a reason to abort the installation (ADFA-5257 review).
+			if (entry.isDirectory && ContainedPathResolver.namesBase(entry.name)) {
+				return@useEntriesEach
 			}
 
-			val destFile = normalizedDestDir.resolve(entry.name).normalize()
+			val destFile =
+				when (val resolution = contained.resolve(entry.name)) {
+					is Resolution.Contained -> {
+						resolution.file.toPath()
+					}
 
-			// Use Path.startsWith() for proper path validation instead of string comparison
-			if (!destFile.startsWith(normalizedDestDir)) {
-				// DO NOT allow extraction to outside of the target dir
-				throw IllegalStateException("Entry is outside of the target dir: ${entry.name}")
-			}
+					is Resolution.Rejected -> {
+						// A pre-existing symlink at the entry's own target -- dangling, or leading
+						// outside destDir -- is this caller's refusal policy at work, not a
+						// zip-slip attempt; report it as such.
+						val overSymlink = resolution.lexicalTarget?.let { Files.isSymbolicLink(it) } == true
+						throw IllegalStateException(
+							if (overSymlink) {
+								"Refusing to extract over an existing symlink: ${entry.name}"
+							} else {
+								"Zip entry escapes the target dir: ${entry.name}"
+							},
+						)
+					}
+
+					is Resolution.Unverifiable -> {
+						throw IllegalStateException(
+							"Cannot verify that a zip entry stays in the target dir: ${entry.name} (${resolution.cause})",
+							resolution.cause,
+						)
+					}
+				}
 
-			// The checks above are lexical (entry name only) and don't catch a symlink
-			// already present on disk (e.g. destDir merged/reused across installer
-			// runs). Reject writing through an existing symlink up front, then
-			// re-check containment against the real, on-disk path once created.
+			// Policy, not containment: the resolver allows a symlink whose target is still inside
+			// destDir, and this caller does not.
 			if (Files.isSymbolicLink(destFile)) {
 				throw IllegalStateException("Refusing to extract over an existing symlink: ${entry.name}")
 			}
 
 			if (entry.isDirectory) {
 				Files.createDirectories(destFile)
-				if (!destFile.toRealPath().startsWith(realDestDir)) {
-					throw IllegalStateException("Entry escapes the target dir via symlink: ${entry.name}")
-				}
 			} else {
 				Files.createDirectories(destFile.parent)
-				if (destFile.parent != lastVerifiedParent) {
-					if (!destFile.parent.toRealPath().startsWith(realDestDir)) {
-						throw IllegalStateException("Entry parent escapes the target dir via symlink: ${entry.name}")
+				// NOFOLLOW_LINKS: the isSymbolicLink check above is a stat, and this is a separate
+				// open, so a link appearing in between would be followed. O_NOFOLLOW makes the
+				// refusal part of the open. Parent directories are still followed -- that needs
+				// openat(2), which java.nio does not expose (ADFA-5257 review).
+				Files
+					.newOutputStream(
+						destFile,
+						StandardOpenOption.WRITE,
+						StandardOpenOption.CREATE,
+						StandardOpenOption.TRUNCATE_EXISTING,
+						LinkOption.NOFOLLOW_LINKS,
+					).use { dest ->
+						zipInput.copyTo(dest)
 					}
-					lastVerifiedParent = destFile.parent
-				}
-
-				Files.newOutputStream(destFile).use { dest ->
-					zipInput.copyTo(dest)
-				}
 			}
 		}
 	}

app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt

@@ -550,10 +550,20 @@ class GradleBuildService :
 		}
 		try {
 			val projectDir = ProjectManagerImpl.getInstance().projectDir
-			val files = ZipUtils.unzipFile(extracted, projectDir)
-			if (files.isNotEmpty()) {
+			val result = ZipUtils.unzipFile(extracted, projectDir)
+			if (result.skipped.isNotEmpty()) {
+				log.warn("Gradle wrapper entries not extracted (existing symlinks left alone): {}", result.skipped)
+			}
+
+			// Success means the wrapper is actually usable, not merely that unzipFile returned: an
+			// entry skipped over a user's own symlink is fine as long as the files it needs exist.
+			val missing =
+				listOf("gradlew", "gradle/wrapper/gradle-wrapper.jar", "gradle/wrapper/gradle-wrapper.properties")
+					.filter { !File(projectDir, it).exists() }
+			if (missing.isEmpty()) {
 				return GradleWrapperCheckResult(true)
 			}
+			log.error("Gradle wrapper installation is incomplete; missing: {}", missing)
 		} catch (e: IOException) {
 			log.error("An error occurred while extracting Gradle wrapper", e)
 		}

app/src/main/java/com/itsaky/androidide/tasks/callables/UnzipCallable.java

@@ -37,6 +37,6 @@ public UnzipCallable(File src, File dest) {
 
 	@Override
 	public List<File> call() throws Exception {
-		return ZipUtils.unzipFile(src, dest);
+		return ZipUtils.unzipFile(src, dest).getExtracted();
 	}
 }

app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt

@@ -183,24 +183,32 @@ class AssetsInstallationHelperTest {
 				ByteArrayOutputStream().use { baos ->
 					ZipOutputStream(baos).use { zos ->
 						// Two levels below the symlink ("linked/sub/nested.txt", no directory
-						// entries), not one: for a one-level entry ("linked/nested.txt"),
-						// destFile.parent IS the symlink, so Files.createDirectories() throws
-						// FileAlreadyExistsException (NOFOLLOW_LINKS rejects the existing
-						// symlink-to-dir) before the toRealPath() guard below it ever runs. One
-						// level deeper, createDirectories() silently traverses the symlink to
-						// create "sub" for real inside outsideDir, and only then does the
-						// toRealPath() check on destFile.parent fire -- which is what this test
-						// exercises.
+						// entries), not one. The depth used to decide which guard caught it, back
+						// when containment was re-checked with toRealPath() after
+						// createDirectories() had already run: one level down, createDirectories()
+						// threw FileAlreadyExistsException on the symlink before that check was
+						// reached. ADFA-5257 moved containment ahead of every mkdir, so both
+						// depths are now refused by ContainedPathResolver with nothing created.
+						// Kept at two levels because that is the case a lexical check alone lets
+						// through -- "linked/sub/nested.txt" has no ".." and does start with
+						// destDir, so only resolving "linked" to its real path catches it.
 						zos.putNextEntry(ZipEntry("linked/sub/nested.txt"))
 						zos.write(content.toByteArray())
 						zos.closeEntry()
 					}
 					baos.toByteArray()
 				}
 
-			assertThrows(IllegalStateException::class.java) {
-				AssetsInstallationHelper.extractZipToDir(ByteArrayInputStream(zipBytes), destDir)
-			}
+			val thrown =
+				assertThrows(IllegalStateException::class.java) {
+					AssetsInstallationHelper.extractZipToDir(ByteArrayInputStream(zipBytes), destDir)
+				}
+			// The symlink sits at an *ancestor*, not at the entry's own target: this is an escape,
+			// and the message must say so -- distinct from the refusal over a symlink at the target.
+			assertTrue(
+				"expected an escape message, got: ${thrown.message}",
+				thrown.message!!.contains("escapes the target dir"),
+			)
 		} finally {
 			outsideDir.deleteRecursivelyWithoutFollowingLinks()
 			destDir.deleteRecursivelyWithoutFollowingLinks()

app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt

@@ -123,18 +123,48 @@ class ExtractZipToDirMergeTest {
 		}
 	}
 
+	// The lexical guard used to be a bare substring reject, so an entry legitimately named with
+	// consecutive dots aborted the whole installation. The shared resolver rejects a ".." *segment*
+	// instead, which lets a name like this through.
+	@Test
+	fun `extracts an entry whose name merely contains a double dot`() {
+		val destDir = Files.createTempDirectory("assets-dots")
+		try {
+			AssetsInstallationHelper.extractZipToDir(
+				zipOf("lib/notes..txt" to "kept", "lib/a..b/c.txt" to "also kept"),
+				destDir,
+			)
+
+			assertEquals("kept", destDir.resolve("lib/notes..txt").toFile().readText())
+			assertEquals("also kept", destDir.resolve("lib/a..b/c.txt").toFile().readText())
+		} finally {
+			destDir.deleteRecursivelyWithoutFollowingLinks()
+		}
+	}
+
 	@Test
 	fun `rejects path traversal`() {
 		val dest = Files.createTempDirectory("mvn")
 		try {
-			assertThrows(IllegalStateException::class.java) {
-				AssetsInstallationHelper.extractZipToDir(zipOf("../evil.jar" to "x"), dest)
-			}
+			val thrown =
+				assertThrows(IllegalStateException::class.java) {
+					AssetsInstallationHelper.extractZipToDir(zipOf("../evil.jar" to "x"), dest)
+				}
+			// A real escape is reported as one -- distinct from the symlink-refusal and
+			// cannot-verify messages below.
+			assertTrue(
+				"expected an escape message, got: ${thrown.message}",
+				thrown.message!!.contains("escapes the target dir"),
+			)
 		} finally {
 			dest.deleteRecursivelyWithoutFollowingLinks()
 		}
 	}
 
+	// The installer's own policy branch: any pre-existing symlink at an entry's target refuses the
+	// extraction, and says so. A *dangling* link is the case the resolver refuses before the
+	// explicit isSymbolicLink check is reached, so asserting the message (not just the type) pins
+	// that it still surfaces as the symlink refusal, not as a zip-slip accusation.
 	@Test
 	fun `rejects extraction over an existing symlink`() {
 		val dest = Files.createTempDirectory("mvn")
@@ -143,15 +173,118 @@ class ExtractZipToDirMergeTest {
 			val outsideTarget = outside.resolve("payload")
 			Files.createSymbolicLink(dest.resolve("evil.jar"), outsideTarget)
 
-			assertThrows(IllegalStateException::class.java) {
-				AssetsInstallationHelper.extractZipToDir(zipOf("evil.jar" to "x"), dest)
-			}
+			val thrown =
+				assertThrows(IllegalStateException::class.java) {
+					AssetsInstallationHelper.extractZipToDir(zipOf("evil.jar" to "x"), dest)
+				}
+			assertTrue(
+				"expected the symlink refusal message, got: ${thrown.message}",
+				thrown.message!!.contains("Refusing to extract over an existing symlink"),
+			)
 		} finally {
 			dest.deleteRecursivelyWithoutFollowingLinks()
 			outside.deleteRecursivelyWithoutFollowingLinks()
 		}
 	}
 
+	// Same refusal for a live link whose target is inside destDir -- the resolver proves it
+	// contained, and the installer's explicit isSymbolicLink check refuses it anyway.
+	@Test
+	fun `rejects extraction over an existing symlink pointing inside destDir`() {
+		val dest = Files.createTempDirectory("mvn")
+		try {
+			Files.write(dest.resolve("real.jar"), "kept".toByteArray())
+			Files.createSymbolicLink(dest.resolve("evil.jar"), dest.resolve("real.jar"))
+
+			val thrown =
+				assertThrows(IllegalStateException::class.java) {
+					AssetsInstallationHelper.extractZipToDir(zipOf("evil.jar" to "x"), dest)
+				}
+			assertTrue(
+				"expected the symlink refusal message, got: ${thrown.message}",
+				thrown.message!!.contains("Refusing to extract over an existing symlink"),
+			)
+			assertEquals("kept", String(Files.readAllBytes(dest.resolve("real.jar"))))
+		} finally {
+			dest.deleteRecursivelyWithoutFollowingLinks()
+		}
+	}
+
+	// And for a live link pointing outside destDir -- refused by the resolver's real-path check,
+	// still reported as the symlink refusal it is, with nothing written through the link.
+	@Test
+	fun `rejects extraction over an existing symlink pointing outside destDir`() {
+		val dest = Files.createTempDirectory("mvn")
+		val outside = Files.createTempDirectory("outside")
+		try {
+			val outsideTarget = outside.resolve("payload")
+			Files.write(outsideTarget, "original".toByteArray())
+			Files.createSymbolicLink(dest.resolve("evil.jar"), outsideTarget)
+
+			val thrown =
+				assertThrows(IllegalStateException::class.java) {
+					AssetsInstallationHelper.extractZipToDir(zipOf("evil.jar" to "x"), dest)
+				}
+			assertTrue(
+				"expected the symlink refusal message, got: ${thrown.message}",
+				thrown.message!!.contains("Refusing to extract over an existing symlink"),
+			)
+			assertEquals("original", String(Files.readAllBytes(outsideTarget)))
+		} finally {
+			dest.deleteRecursivelyWithoutFollowingLinks()
+			outside.deleteRecursivelyWithoutFollowingLinks()
+		}
+	}
+
+	// The third message: containment that cannot be *verified* (here a symlink loop, ELOOP) is
+	// neither an escape nor the symlink refusal -- it aborts naming the filesystem cause, so a
+	// failing install points at the disk, not at the archive.
+	@Test
+	fun `reports unverifiable containment distinctly`() {
+		val dest = Files.createTempDirectory("mvn")
+		try {
+			Files.createSymbolicLink(dest.resolve("loop-a"), dest.resolve("loop-b"))
+			Files.createSymbolicLink(dest.resolve("loop-b"), dest.resolve("loop-a"))
+
+			val thrown =
+				assertThrows(IllegalStateException::class.java) {
+					AssetsInstallationHelper.extractZipToDir(zipOf("loop-a/file.txt" to "x"), dest)
+				}
+			assertTrue(
+				"expected the cannot-verify message, got: ${thrown.message}",
+				thrown.message!!.contains("Cannot verify"),
+			)
+		} finally {
+			dest.deleteRecursivelyWithoutFollowingLinks()
+		}
+	}
+
+	// A "." or "./" root directory entry names destDir itself. Some archivers emit one, and the
+	// asset zips are refreshed from an external URL -- it must be a no-op, not an aborted install.
+	@Test
+	fun `tolerates a root directory entry instead of aborting`() {
+		val dest = Files.createTempDirectory("mvn")
+		try {
+			val zipBytes =
+				ByteArrayOutputStream().use { baos ->
+					ZipOutputStream(baos).use { zip ->
+						zip.putNextEntry(ZipEntry("./"))
+						zip.closeEntry()
+						zip.putNextEntry(ZipEntry("com/foo/a.txt"))
+						zip.write("kept".toByteArray())
+						zip.closeEntry()
+					}
+					baos.toByteArray()
+				}
+
+			AssetsInstallationHelper.extractZipToDir(ByteArrayInputStream(zipBytes), dest)
+
+			assertEquals("kept", String(Files.readAllBytes(dest.resolve("com/foo/a.txt"))))
+		} finally {
+			dest.deleteRecursivelyWithoutFollowingLinks()
+		}
+	}
+
 	@Test
 	fun `rejects extraction into a symlinked parent that escapes destDir`() {
 		val dest = Files.createTempDirectory("mvn")