From 250db83b58ffaef947f62d0defc3b192a3c7b599 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 3 Aug 2026 14:24:19 -0700 Subject: [PATCH 1/7] ADFA-5005: Fix SDK bootstrap crash extracting android-sdk.zip The bundled android-sdk.zip.br has no directory entries, so its very first zip entry (build-tools/35.0.0/NOTICE.txt) failed to extract on every fresh install: extractZipToDir only created parent directories for entries explicitly flagged as directories, never for plain file entries, and ANDROID_HOME is wiped before each install. Create the parent directory unconditionally before writing each file entry. Verified end-to-end on-device: OOBE completes and build-tools/35.0.0/NOTICE.txt lands at the expected 1,068,025 bytes. Co-Authored-By: Claude Sonnet 5 --- .../com/itsaky/androidide/assets/AssetsInstallationHelper.kt | 1 + 1 file changed, 1 insertion(+) diff --git a/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt b/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt index 39f668e428..8479519314 100644 --- a/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt +++ b/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt @@ -263,6 +263,7 @@ object AssetsInstallationHelper { if (entry.isDirectory) { Files.createDirectories(destFile) } else { + Files.createDirectories(destFile.parent) Files.newOutputStream(destFile).use { dest -> zipInput.copyTo(dest) } From 60f124ac501a31dafea272e1060f0c41caa44ae2 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 3 Aug 2026 14:34:40 -0700 Subject: [PATCH 2/7] ADFA-5005: Add regression test for extractZipToDir missing parent dirs Builds an in-memory zip with a single nested file entry and no directory entries, matching how android-sdk.zip is packaged, and asserts extraction creates the parent directories and preserves the file content. Confirmed the test fails with NoSuchFileException against the pre-fix code and passes with the fix. Co-Authored-By: Claude Sonnet 5 --- .../assets/AssetsInstallationHelperTest.kt | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt b/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt index 1cd9c8455b..d08b0dce37 100644 --- a/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt +++ b/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt @@ -7,11 +7,17 @@ import io.mockk.every import io.mockk.mockk import io.mockk.mockkObject import kotlinx.coroutines.runBlocking +import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test +import java.io.ByteArrayInputStream +import java.io.ByteArrayOutputStream import java.io.FileNotFoundException +import java.nio.file.Files +import java.util.zip.ZipEntry +import java.util.zip.ZipOutputStream class AssetsInstallationHelperTest { private val ctx: Context = mockk(relaxed = true) @@ -48,4 +54,30 @@ class AssetsInstallationHelperTest { (failure.cause?.cause) is FileNotFoundException, ) } + + @Test + fun `extractZipToDir creates parent directories for nested entries with no directory entries`() { + val destDir = Files.createTempDirectory("extract-zip-to-dir-test") + try { + val content = "test notice content" + val zipBytes = + ByteArrayOutputStream().use { baos -> + ZipOutputStream(baos).use { zos -> + // No directory entries, matching how android-sdk.zip is packaged. + zos.putNextEntry(ZipEntry("build-tools/35.0.0/NOTICE.txt")) + zos.write(content.toByteArray()) + zos.closeEntry() + } + baos.toByteArray() + } + + AssetsInstallationHelper.extractZipToDir(ByteArrayInputStream(zipBytes), destDir) + + val extracted = destDir.resolve("build-tools/35.0.0/NOTICE.txt") + assertTrue("Expected extracted file to exist", Files.exists(extracted)) + assertEquals(content, String(Files.readAllBytes(extracted))) + } finally { + destDir.toFile().deleteRecursively() + } + } } From cf9882837a4b300cda520fe8426c3439a3243920 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 3 Aug 2026 14:40:45 -0700 Subject: [PATCH 3/7] ADFA-5005: Harden extractZipToDir against on-disk symlink escapes The existing normalize()/startsWith() checks validate the zip entry name lexically, but not the actual filesystem state -- a symlink already present under destDir (e.g. from a merged/reused install dir in SplitAssetsInstaller) could redirect Files.createDirectories() or Files.newOutputStream() outside destDir undetected. Resolve destFile's real parent path after creating it and re-check containment against destDir's real path, and refuse to write through a destFile that already exists as a symlink. Added regression tests for both vectors; confirmed they fail without this change. Co-Authored-By: Claude Sonnet 5 --- .../assets/AssetsInstallationHelper.kt | 13 ++++++++++ .../assets/ExtractZipToDirMergeTest.kt | 24 +++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt b/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt index 8479519314..be28f65e83 100644 --- a/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt +++ b/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt @@ -245,6 +245,7 @@ object AssetsInstallationHelper { Files.createDirectories(destDir) // Normalize and make destDir absolute for secure path validation val normalizedDestDir = destDir.toAbsolutePath().normalize() + val realDestDir = normalizedDestDir.toRealPath() ZipInputStream(srcStream.buffered()).useEntriesEach { zipInput, entry -> // Validate entry name doesn't contain dangerous patterns @@ -264,6 +265,18 @@ object AssetsInstallationHelper { Files.createDirectories(destFile) } else { Files.createDirectories(destFile.parent) + + // 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). Resolve the real, on-disk parent path and re-check + // containment, and refuse to write through an existing symlink. + if (!destFile.parent.toRealPath().startsWith(realDestDir)) { + throw IllegalStateException("Entry parent escapes the target dir via symlink: ${entry.name}") + } + if (Files.isSymbolicLink(destFile)) { + throw IllegalStateException("Refusing to extract over an existing symlink: ${entry.name}") + } + Files.newOutputStream(destFile).use { dest -> zipInput.copyTo(dest) } diff --git a/app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt b/app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt index 9f4ab7f879..69d5330b8c 100644 --- a/app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt +++ b/app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt @@ -70,4 +70,28 @@ class ExtractZipToDirMergeTest { AssetsInstallationHelper.extractZipToDir(zipOf("../evil.jar" to "x"), dest) } } + + @Test + fun `rejects extraction over an existing symlink`() { + val dest = Files.createTempDirectory("mvn") + val outsideTarget = Files.createTempDirectory("outside").resolve("payload") + + Files.createSymbolicLink(dest.resolve("evil.jar"), outsideTarget) + + assertThrows(IllegalStateException::class.java) { + AssetsInstallationHelper.extractZipToDir(zipOf("evil.jar" to "x"), dest) + } + } + + @Test + fun `rejects extraction into a symlinked parent that escapes destDir`() { + val dest = Files.createTempDirectory("mvn") + val outside = Files.createTempDirectory("outside") + + Files.createSymbolicLink(dest.resolve("linked"), outside) + + assertThrows(IllegalStateException::class.java) { + AssetsInstallationHelper.extractZipToDir(zipOf("linked/nested.txt" to "x"), dest) + } + } } From c287d5b21e3b47df8b6fdd8d8a148c208ba73d25 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 3 Aug 2026 14:51:40 -0700 Subject: [PATCH 4/7] ADFA-5005: Close symlink-escape gap in extractZipToDir directory entries Review of PR #1621 flagged an asymmetry: the on-disk symlink checks added for file entries didn't cover directory entries, so Files.createDirectories(destFile) would silently no-op through a pre-existing symlink pointing outside destDir (or, if the symlink target didn't exist, create directories at the symlink's target outside destDir). Hoist the existing-symlink check above the isDirectory branch so it applies to both, and add the same real-path containment check after creating a directory entry. Added a regression test confirming a bare directory entry resolving to an escaping symlink is now rejected; confirmed it fails without this change and passes with it. Co-Authored-By: Claude Sonnet 5 --- .../assets/AssetsInstallationHelper.kt | 19 ++++++++++------- .../assets/ExtractZipToDirMergeTest.kt | 21 +++++++++++++++++++ 2 files changed, 32 insertions(+), 8 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt b/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt index be28f65e83..8a13913e00 100644 --- a/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt +++ b/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt @@ -261,21 +261,24 @@ object AssetsInstallationHelper { throw IllegalStateException("Entry is outside of the target dir: ${entry.name}") } + // 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. + 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) - - // 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). Resolve the real, on-disk parent path and re-check - // containment, and refuse to write through an existing symlink. if (!destFile.parent.toRealPath().startsWith(realDestDir)) { throw IllegalStateException("Entry parent escapes the target dir via symlink: ${entry.name}") } - if (Files.isSymbolicLink(destFile)) { - throw IllegalStateException("Refusing to extract over an existing symlink: ${entry.name}") - } Files.newOutputStream(destFile).use { dest -> zipInput.copyTo(dest) diff --git a/app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt b/app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt index 69d5330b8c..f8aaf90ef3 100644 --- a/app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt +++ b/app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt @@ -94,4 +94,25 @@ class ExtractZipToDirMergeTest { AssetsInstallationHelper.extractZipToDir(zipOf("linked/nested.txt" to "x"), dest) } } + + @Test + fun `rejects a bare directory entry that resolves to an existing symlink escaping destDir`() { + val dest = Files.createTempDirectory("mvn") + val outside = Files.createTempDirectory("outside") + + Files.createSymbolicLink(dest.resolve("linked"), outside) + + val zipBytes = + ByteArrayOutputStream().use { baos -> + ZipOutputStream(baos).use { zip -> + zip.putNextEntry(ZipEntry("linked/")) + zip.closeEntry() + } + baos.toByteArray() + } + + assertThrows(IllegalStateException::class.java) { + AssetsInstallationHelper.extractZipToDir(ByteArrayInputStream(zipBytes), dest) + } + } } From ff234ef272eff710a772b19dbbeb9111b6bd5a0d Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 3 Aug 2026 14:55:27 -0700 Subject: [PATCH 5/7] ADFA-5005: Cache last-verified parent to cut redundant toRealPath() calls Review of PR #1621 noted that destFile.parent.toRealPath() runs once per file entry even though zip entries are commonly clustered by directory (e.g. 20 files under build-tools/35.0.0/ alone) -- each consecutive sibling re-walks and re-resolves the same parent path. Cache the last-verified parent and skip the real-path containment check when the current entry's parent is unchanged. Nothing in the loop can turn an already-verified real directory into a symlink mid-run, so caching by lexical parent equality doesn't weaken the check. Added a test exercising multiple sibling files under one directory (the only test that hit the cache-hit branch); full assets test suite still green. Co-Authored-By: Claude Sonnet 5 --- .../assets/AssetsInstallationHelper.kt | 14 ++++++++++++-- .../assets/ExtractZipToDirMergeTest.kt | 16 ++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt b/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt index 8a13913e00..07d7ddd71d 100644 --- a/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt +++ b/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt @@ -247,6 +247,13 @@ object AssetsInstallationHelper { 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// 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 + ZipInputStream(srcStream.buffered()).useEntriesEach { zipInput, entry -> // Validate entry name doesn't contain dangerous patterns if (entry.name.contains("..") || entry.name.startsWith("/") || entry.name.startsWith("\\")) { @@ -276,8 +283,11 @@ object AssetsInstallationHelper { } } else { Files.createDirectories(destFile.parent) - if (!destFile.parent.toRealPath().startsWith(realDestDir)) { - throw IllegalStateException("Entry parent escapes the target dir via symlink: ${entry.name}") + if (destFile.parent != lastVerifiedParent) { + if (!destFile.parent.toRealPath().startsWith(realDestDir)) { + throw IllegalStateException("Entry parent escapes the target dir via symlink: ${entry.name}") + } + lastVerifiedParent = destFile.parent } Files.newOutputStream(destFile).use { dest -> diff --git a/app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt b/app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt index f8aaf90ef3..088221c259 100644 --- a/app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt +++ b/app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt @@ -63,6 +63,22 @@ class ExtractZipToDirMergeTest { ) } + @Test + fun `extracts multiple sibling files under the same directory`() { + val dest = Files.createTempDirectory("mvn") + + AssetsInstallationHelper.extractZipToDir( + zipOf( + "com/foo/1.0/a.txt" to "a", + "com/foo/1.0/b.txt" to "b", + ), + dest, + ) + + assertEquals("a", String(Files.readAllBytes(dest.resolve("com/foo/1.0/a.txt")))) + assertEquals("b", String(Files.readAllBytes(dest.resolve("com/foo/1.0/b.txt")))) + } + @Test fun `rejects path traversal`() { val dest = Files.createTempDirectory("mvn") From ca81ae9becb80a2cde5176741f32d1da5c4bc728 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Tue, 4 Aug 2026 17:41:52 -0700 Subject: [PATCH 6/7] ADFA-5005: Address review feedback on symlink hardening - Route SplitAssetsInstaller/BundledAssetsInstaller's plugin-zip extraction through the hardened extractZipToDir() instead of a lexical-only reimplementation, so all three extraction sites share one symlink-hardened path. - Add a regression test covering a file entry under a pre-existing symlinked parent with no directory entry -- the shape android-sdk.zip actually has, and the only path that previously exercised the toRealPath() escape check. - Fix a stale comment in ExtractZipToDirMergeTest's zipOf helper that no longer matched extractZipToDir's behavior. - Stop leaking temp dirs across the symlink tests, using a walker that won't follow symlinks into deletion. --- .../assets/BundledAssetsInstaller.kt | 23 +-- .../androidide/assets/SplitAssetsInstaller.kt | 23 +-- .../assets/AssetsInstallationHelperTest.kt | 29 +++ .../assets/ExtractZipToDirMergeTest.kt | 165 ++++++++++++------ 4 files changed, 144 insertions(+), 96 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt b/app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt index 9c7b46297a..33fcfc988f 100644 --- a/app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt +++ b/app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt @@ -29,7 +29,6 @@ import java.io.FileNotFoundException import java.io.IOException import java.nio.file.Files import java.nio.file.Path -import java.util.zip.ZipInputStream import kotlin.io.path.ExperimentalPathApi import kotlin.io.path.deleteRecursively @@ -174,27 +173,7 @@ data object BundledAssetsInstaller : BaseAssetsInstaller() { val assetPath = ToolsManager.getCommonAsset("$entryName.br") assets.open(assetPath).use { assetStream -> BrotliInputStream(assetStream).use { brotliStream -> - ZipInputStream(brotliStream).use { pluginZip -> - var pluginEntry = pluginZip.nextEntry - while (pluginEntry != null) { - if (!pluginEntry.isDirectory) { - val targetPath = pluginDirPath.resolve(pluginEntry.name).normalize() - // Security check: prevent path traversal attacks - if (!targetPath.startsWith(pluginDirPath)) { - throw IllegalStateException( - "Zip entry '${pluginEntry.name}' would escape target directory", - ) - } - val targetFile = targetPath.toFile() - targetFile.parentFile?.mkdirs() - logger.debug("Extracting '{}' to {}", pluginEntry.name, targetFile) - targetFile.outputStream().use { output -> - pluginZip.copyTo(output) - } - } - pluginEntry = pluginZip.nextEntry - } - } + AssetsInstallationHelper.extractZipToDir(brotliStream, pluginDirPath) } } logger.debug("Completed extracting plugin artifacts") diff --git a/app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt b/app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt index a7268c2215..ba532d2f8a 100644 --- a/app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt +++ b/app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt @@ -23,7 +23,6 @@ import java.io.FileNotFoundException import java.nio.file.Files import java.nio.file.Path import java.util.zip.ZipFile -import java.util.zip.ZipInputStream import kotlin.io.path.ExperimentalPathApi import kotlin.io.path.deleteRecursively import kotlin.system.measureTimeMillis @@ -167,27 +166,7 @@ data object SplitAssetsInstaller : BaseAssetsInstaller() { } Files.createDirectories(pluginDirPath) - ZipInputStream(zipInput).use { pluginZip -> - var pluginEntry = pluginZip.nextEntry - while (pluginEntry != null) { - if (!pluginEntry.isDirectory) { - val targetPath = pluginDirPath.resolve(pluginEntry.name).normalize() - // Security check: prevent path traversal attacks - if (!targetPath.startsWith(pluginDirPath)) { - throw IllegalStateException( - "Zip entry '${pluginEntry.name}' would escape target directory", - ) - } - val targetFile = targetPath.toFile() - targetFile.parentFile?.mkdirs() - logger.debug("Extracting '{}' to {}", pluginEntry.name, targetFile) - targetFile.outputStream().use { output -> - pluginZip.copyTo(output) - } - } - pluginEntry = pluginZip.nextEntry - } - } + AssetsInstallationHelper.extractZipToDir(zipInput, pluginDirPath) logger.debug("Completed extracting plugin artifacts") } diff --git a/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt b/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt index d08b0dce37..b0f3dfa676 100644 --- a/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt +++ b/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt @@ -9,6 +9,7 @@ import io.mockk.mockkObject import kotlinx.coroutines.runBlocking import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse +import org.junit.Assert.assertThrows import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test @@ -80,4 +81,32 @@ class AssetsInstallationHelperTest { destDir.toFile().deleteRecursively() } } + + @Test + fun `extractZipToDir rejects a file entry whose pre-existing symlinked parent escapes destDir`() { + val destDir = Files.createTempDirectory("extract-zip-to-dir-test") + val outsideDir = Files.createTempDirectory("extract-zip-to-dir-outside") + try { + Files.createSymbolicLink(destDir.resolve("linked"), outsideDir) + + val content = "escaping content" + val zipBytes = + ByteArrayOutputStream().use { baos -> + ZipOutputStream(baos).use { zos -> + // No directory entry for "linked/", matching how android-sdk.zip is packaged. + zos.putNextEntry(ZipEntry("linked/nested.txt")) + zos.write(content.toByteArray()) + zos.closeEntry() + } + baos.toByteArray() + } + + assertThrows(IllegalStateException::class.java) { + AssetsInstallationHelper.extractZipToDir(ByteArrayInputStream(zipBytes), destDir) + } + } finally { + outsideDir.toFile().deleteRecursively() + destDir.toFile().deleteRecursively() + } + } } diff --git a/app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt b/app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt index 088221c259..d5c3177fe9 100644 --- a/app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt +++ b/app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt @@ -6,16 +6,26 @@ import org.junit.Assert.assertTrue import org.junit.Test import java.io.ByteArrayInputStream import java.io.ByteArrayOutputStream +import java.io.IOException +import java.nio.file.FileVisitResult import java.nio.file.Files +import java.nio.file.Path +import java.nio.file.SimpleFileVisitor +import java.nio.file.attribute.BasicFileAttributes import java.util.zip.ZipEntry import java.util.zip.ZipOutputStream class ExtractZipToDirMergeTest { - // Real archives merged in production (e.g. plugin-maven-repo.zip, built by Gradle's - // Zip task -- verified via `unzip -l assets/plugin-maven-repo.zip`) always carry an - // explicit directory entry for every ancestor path. extractZipToDir relies on that - // (it only Files.createDirectories() on directory entries, not on every file - // entry's parent), so mirror that shape here rather than writing bare file entries. + // extractZipToDir now calls Files.createDirectories(destFile.parent) for every file + // entry, not just directory entries, so a bare file entry with no ancestor directory + // entries would extract fine too. zipOf still injects a directory entry for every + // ancestor because that's how real archives merged in production (e.g. + // plugin-maven-repo.zip, built by Gradle's Zip task -- verified via `unzip -l + // assets/plugin-maven-repo.zip`) are actually packaged. The no-directory-entry path + // (e.g. how android-sdk.zip is packaged) is exercised separately by + // AssetsInstallationHelperTest's `extractZipToDir creates parent directories for + // nested entries with no directory entries` and `extractZipToDir rejects a file + // entry whose pre-existing symlinked parent escapes destDir`. private fun zipOf(vararg entries: Pair): ByteArrayInputStream { val bos = ByteArrayOutputStream() ZipOutputStream(bos).use { zip -> @@ -40,6 +50,34 @@ class ExtractZipToDirMergeTest { return ByteArrayInputStream(bos.toByteArray()) } + // Deletes a directory tree without following symlinks it contains, unlike + // File.deleteRecursively(). Files.walkFileTree() doesn't follow symlinks unless + // FileVisitOption.FOLLOW_LINKS is passed (it isn't here), so a symlink is visited + // as a leaf via visitFile() -- deleting it unlinks the link itself, never the + // target it points to. Needed because several tests below symlink out of dest. + private fun Path.deleteRecursivelyWithoutFollowingLinks() { + Files.walkFileTree( + this, + object : SimpleFileVisitor() { + override fun visitFile( + file: Path, + attrs: BasicFileAttributes, + ): FileVisitResult { + Files.delete(file) + return FileVisitResult.CONTINUE + } + + override fun postVisitDirectory( + dir: Path, + exc: IOException?, + ): FileVisitResult { + Files.delete(dir) + return FileVisitResult.CONTINUE + } + }, + ) + } + @Test fun `overlay merges without wiping existing files`() { val dest = @@ -47,55 +85,70 @@ class ExtractZipToDirMergeTest { Files.createDirectories(it.resolve("com/foo/1.0")) Files.write(it.resolve("com/foo/1.0/foo-1.0.jar"), "harvested".toByteArray()) } - - AssetsInstallationHelper.extractZipToDir( - zipOf("com/itsaky/androidide/plugin-api/1.0.0/plugin-api-1.0.0.jar" to "fat"), - dest, - ) - - assertTrue( - "harvested file must survive the merge", - Files.exists(dest.resolve("com/foo/1.0/foo-1.0.jar")), - ) - assertEquals( - "fat", - String(Files.readAllBytes(dest.resolve("com/itsaky/androidide/plugin-api/1.0.0/plugin-api-1.0.0.jar"))), - ) + try { + AssetsInstallationHelper.extractZipToDir( + zipOf("com/itsaky/androidide/plugin-api/1.0.0/plugin-api-1.0.0.jar" to "fat"), + dest, + ) + + assertTrue( + "harvested file must survive the merge", + Files.exists(dest.resolve("com/foo/1.0/foo-1.0.jar")), + ) + assertEquals( + "fat", + String(Files.readAllBytes(dest.resolve("com/itsaky/androidide/plugin-api/1.0.0/plugin-api-1.0.0.jar"))), + ) + } finally { + dest.deleteRecursivelyWithoutFollowingLinks() + } } @Test fun `extracts multiple sibling files under the same directory`() { val dest = Files.createTempDirectory("mvn") - - AssetsInstallationHelper.extractZipToDir( - zipOf( - "com/foo/1.0/a.txt" to "a", - "com/foo/1.0/b.txt" to "b", - ), - dest, - ) - - assertEquals("a", String(Files.readAllBytes(dest.resolve("com/foo/1.0/a.txt")))) - assertEquals("b", String(Files.readAllBytes(dest.resolve("com/foo/1.0/b.txt")))) + try { + AssetsInstallationHelper.extractZipToDir( + zipOf( + "com/foo/1.0/a.txt" to "a", + "com/foo/1.0/b.txt" to "b", + ), + dest, + ) + + assertEquals("a", String(Files.readAllBytes(dest.resolve("com/foo/1.0/a.txt")))) + assertEquals("b", String(Files.readAllBytes(dest.resolve("com/foo/1.0/b.txt")))) + } finally { + dest.deleteRecursivelyWithoutFollowingLinks() + } } @Test fun `rejects path traversal`() { val dest = Files.createTempDirectory("mvn") - assertThrows(IllegalStateException::class.java) { - AssetsInstallationHelper.extractZipToDir(zipOf("../evil.jar" to "x"), dest) + try { + assertThrows(IllegalStateException::class.java) { + AssetsInstallationHelper.extractZipToDir(zipOf("../evil.jar" to "x"), dest) + } + } finally { + dest.deleteRecursivelyWithoutFollowingLinks() } } @Test fun `rejects extraction over an existing symlink`() { val dest = Files.createTempDirectory("mvn") - val outsideTarget = Files.createTempDirectory("outside").resolve("payload") - - Files.createSymbolicLink(dest.resolve("evil.jar"), outsideTarget) + val outside = Files.createTempDirectory("outside") + try { + val outsideTarget = outside.resolve("payload") + Files.createSymbolicLink(dest.resolve("evil.jar"), outsideTarget) - assertThrows(IllegalStateException::class.java) { - AssetsInstallationHelper.extractZipToDir(zipOf("evil.jar" to "x"), dest) + assertThrows(IllegalStateException::class.java) { + AssetsInstallationHelper.extractZipToDir(zipOf("evil.jar" to "x"), dest) + } + } finally { + dest.deleteRecursivelyWithoutFollowingLinks() + outside.deleteRecursivelyWithoutFollowingLinks() } } @@ -103,11 +156,15 @@ class ExtractZipToDirMergeTest { fun `rejects extraction into a symlinked parent that escapes destDir`() { val dest = Files.createTempDirectory("mvn") val outside = Files.createTempDirectory("outside") + try { + Files.createSymbolicLink(dest.resolve("linked"), outside) - Files.createSymbolicLink(dest.resolve("linked"), outside) - - assertThrows(IllegalStateException::class.java) { - AssetsInstallationHelper.extractZipToDir(zipOf("linked/nested.txt" to "x"), dest) + assertThrows(IllegalStateException::class.java) { + AssetsInstallationHelper.extractZipToDir(zipOf("linked/nested.txt" to "x"), dest) + } + } finally { + dest.deleteRecursivelyWithoutFollowingLinks() + outside.deleteRecursivelyWithoutFollowingLinks() } } @@ -115,20 +172,24 @@ class ExtractZipToDirMergeTest { fun `rejects a bare directory entry that resolves to an existing symlink escaping destDir`() { val dest = Files.createTempDirectory("mvn") val outside = Files.createTempDirectory("outside") + try { + Files.createSymbolicLink(dest.resolve("linked"), outside) - Files.createSymbolicLink(dest.resolve("linked"), outside) - - val zipBytes = - ByteArrayOutputStream().use { baos -> - ZipOutputStream(baos).use { zip -> - zip.putNextEntry(ZipEntry("linked/")) - zip.closeEntry() + val zipBytes = + ByteArrayOutputStream().use { baos -> + ZipOutputStream(baos).use { zip -> + zip.putNextEntry(ZipEntry("linked/")) + zip.closeEntry() + } + baos.toByteArray() } - baos.toByteArray() - } - assertThrows(IllegalStateException::class.java) { - AssetsInstallationHelper.extractZipToDir(ByteArrayInputStream(zipBytes), dest) + assertThrows(IllegalStateException::class.java) { + AssetsInstallationHelper.extractZipToDir(ByteArrayInputStream(zipBytes), dest) + } + } finally { + dest.deleteRecursivelyWithoutFollowingLinks() + outside.deleteRecursivelyWithoutFollowingLinks() } } } From 96405649c6832a38b94a1a7c363d42c3d7631e34 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Tue, 4 Aug 2026 18:17:23 -0700 Subject: [PATCH 7/7] ADFA-5005: Fix symlink-escape test to actually reach the toRealPath() guard hal-eisen-adfa found that the previous test (`linked/nested.txt`, one level below the symlink) made destFile.parent the symlink itself, so Files.createDirectories() threw FileAlreadyExistsException under NOFOLLOW_LINKS before the toRealPath() guard ever ran -- the test failed, and the guard stayed uncovered. Use a two-level entry (`linked/sub/nested.txt`) instead, per his repro: createDirectories() silently traverses the symlink to create `sub` for real inside the escape target, and only then does the toRealPath() check on destFile.parent fire and reject it. That's the guard this test is meant to cover. Also switch the test's cleanup to deleteRecursivelyWithoutFollowingLinks() (copied from ExtractZipToDirMergeTest), since the prior deleteRecursively() follows symlinks and would delete the escape target's contents if outsideDir happened to be cleaned up after destDir. Co-Authored-By: Claude Sonnet 5 --- .../assets/AssetsInstallationHelperTest.kt | 51 +++++++++++++++++-- .../assets/ExtractZipToDirMergeTest.kt | 2 +- 2 files changed, 47 insertions(+), 6 deletions(-) diff --git a/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt b/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt index b0f3dfa676..2a4a8e8c28 100644 --- a/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt +++ b/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt @@ -16,7 +16,12 @@ import org.junit.Test import java.io.ByteArrayInputStream import java.io.ByteArrayOutputStream import java.io.FileNotFoundException +import java.io.IOException +import java.nio.file.FileVisitResult import java.nio.file.Files +import java.nio.file.Path +import java.nio.file.SimpleFileVisitor +import java.nio.file.attribute.BasicFileAttributes import java.util.zip.ZipEntry import java.util.zip.ZipOutputStream @@ -83,7 +88,7 @@ class AssetsInstallationHelperTest { } @Test - fun `extractZipToDir rejects a file entry whose pre-existing symlinked parent escapes destDir`() { + fun `extractZipToDir rejects a file entry whose pre-existing symlinked grandparent escapes destDir`() { val destDir = Files.createTempDirectory("extract-zip-to-dir-test") val outsideDir = Files.createTempDirectory("extract-zip-to-dir-outside") try { @@ -93,8 +98,16 @@ class AssetsInstallationHelperTest { val zipBytes = ByteArrayOutputStream().use { baos -> ZipOutputStream(baos).use { zos -> - // No directory entry for "linked/", matching how android-sdk.zip is packaged. - zos.putNextEntry(ZipEntry("linked/nested.txt")) + // 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. + zos.putNextEntry(ZipEntry("linked/sub/nested.txt")) zos.write(content.toByteArray()) zos.closeEntry() } @@ -105,8 +118,36 @@ class AssetsInstallationHelperTest { AssetsInstallationHelper.extractZipToDir(ByteArrayInputStream(zipBytes), destDir) } } finally { - outsideDir.toFile().deleteRecursively() - destDir.toFile().deleteRecursively() + outsideDir.deleteRecursivelyWithoutFollowingLinks() + destDir.deleteRecursivelyWithoutFollowingLinks() } } + + // Deletes a directory tree without following symlinks it contains, unlike + // File.deleteRecursively(). Files.walkFileTree() doesn't follow symlinks unless + // FileVisitOption.FOLLOW_LINKS is passed (it isn't here), so a symlink is visited + // as a leaf via visitFile() -- deleting it unlinks the link itself, never the + // target it points to. Needed because the symlink test above symlinks out of destDir. + private fun Path.deleteRecursivelyWithoutFollowingLinks() { + Files.walkFileTree( + this, + object : SimpleFileVisitor() { + override fun visitFile( + file: Path, + attrs: BasicFileAttributes, + ): FileVisitResult { + Files.delete(file) + return FileVisitResult.CONTINUE + } + + override fun postVisitDirectory( + dir: Path, + exc: IOException?, + ): FileVisitResult { + Files.delete(dir) + return FileVisitResult.CONTINUE + } + }, + ) + } } diff --git a/app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt b/app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt index d5c3177fe9..92868c1daf 100644 --- a/app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt +++ b/app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt @@ -25,7 +25,7 @@ class ExtractZipToDirMergeTest { // (e.g. how android-sdk.zip is packaged) is exercised separately by // AssetsInstallationHelperTest's `extractZipToDir creates parent directories for // nested entries with no directory entries` and `extractZipToDir rejects a file - // entry whose pre-existing symlinked parent escapes destDir`. + // entry whose pre-existing symlinked grandparent escapes destDir`. private fun zipOf(vararg entries: Pair): ByteArrayInputStream { val bos = ByteArrayOutputStream() ZipOutputStream(bos).use { zip ->