Skip to content

Commit 52038f0

Browse files
ADFA-5257: Share one path-containment check instead of two divergent copies (#1736)
* ADFA-5257: Share one path-containment check instead of two divergent copies ZipUtils.unzipFile checked only that a canonical path started with the destination prefix: no lexical rejection of a ".." segment, and nothing to stop an entry writing through a symlink already present at its target. AssetsInstallationHelper.extractZipToDir had the elaborate version -- lexical reject, Path.startsWith, a refusal to follow an existing symlink, and a hand-rolled per-parent cache over toRealPath. Each carried a comment asking whoever fixed one to remember the other. Both now call ContainedPathResolver in common. The file is plain java.io/java.nio with no Android dependency and app already depends on common, so the reason the copies gave for existing was never true in the direction that mattered. The installer's substring reject of ".." goes with it: an archive entry legitimately named notes..txt used to abort an entire asset installation. Only a literal ".." segment can name a parent directory, so the per-segment rule loses nothing. What is deliberately not shared is the policy for an existing symlink at a target whose destination is still inside the base. Unzipping a user's project skips the entry and leaves their own gradlew symlink alone; the installer refuses to write through any symlink. That check stays at each call site, one line, labelled as policy. The resolver carries the ancestor caching the installer did by hand, so a bootstrap archive clustering thousands of entries under a few directories still resolves each ancestor once. Verified: 342 tests pass across both modules, and ZipUtils' symlink test fails against the previous implementation -- this is a stronger guard, not a move. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ADFA-5257: Drop the ancestor cache, and fail properly on an unusable entry name The cached fast path answered a later path under an already-verified directory without looking at it again, so anything that replaced that directory with a symlink in between would be followed. Measuring settled whether the guarantee was affordable: a real 1.8 GB asset installation on device takes 48.0 s with every resolve revalidating, against 51.4 s with the cache and 51.3 s with the hand-rolled cache it replaced. Extraction is I/O and inflate; the check is noise. The cache is gone and the numbers are in the comment. File(destDir, entry.name).toPath() threw InvalidPathException for a name the platform cannot represent -- an unchecked exception escaping unzipFile's declared IOException contract before the resolver ever saw the entry. It now arrives as the IOException the function promises, with a test. PathTraversalTest swallowed every FileSystemException into a skipped test, which could have quietly removed the symlink-escape assertion from CI. It now skips only the known Windows privilege restriction and rethrows anything else, matching ZipUtilsTest. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ADFA-5257: Fail closed when the base directory cannot be resolved Review of #1736 found the containment check could quietly fall back to lexical-only matching -- weaker than the canonical-prefix check it replaced, and silent about it. Two ways in, both fixed by resolving the base per call instead of pinning it in the constructor: - The constructor caught the IOException from toRealPath() and nulled the field, disabling layer 3 for the resolver's whole life. An unresolvable base is now refused outright, with a warning. - Files.exists() is false both for "absent" and for "cannot be determined", so a base under a non-traversable parent read as absent and skipped layer 3. Confirmed-absent is now distinguished by catching NoSuchFileException from toRealPath() itself, which also drops a redundant stat. Pinning the base at construction was stale besides: the asset installer builds its resolver before the directory exists, so layer 3 never ran again even after extraction created the tree. A symlink planted into the base after construction now gets caught. Also in ZipUtils, containment is checked before the existing-symlink policy. In the old order an entry aiming outside the target could hit a symlink first and be skipped as a benign "leave the user's link alone" case, masking the zip-slip rejection; the skip is now logged. Both new tests were confirmed to fail against the unfixed code, for the reasons they are named for. Docs corrected where they overclaimed: the resolver is not yet the only containment check in the tree (ZipRecipeExecutor and PluginLoader remain -- ADFA-5266), it does not memoize, and unzipFile does not extract literally every entry. The deliberate narrowing over the old canonical-prefix check (a/../b.txt now fails) is documented and pinned by a test. * ADFA-5257: Address review: report skips, tolerate dangling links, reject "." Review fixes on the shared containment PR (#1736): - unzipFile now returns an UnzipResult (extracted + skipped) so callers can tell when an entry was left unextracted over an existing symlink. doInstallWrapper verifies the wrapper files actually exist under the project dir instead of trusting a non-empty extraction list. - A dangling symlink inside destDir no longer aborts the archive as an escape: a lexically-contained symlink at the entry's path takes the same skip branch as a live one -- nothing is written at or through it. - Drop the unreachable catch(InvalidPathException): the resolver catches it internally and returns null, so an unusable entry name now surfaces through the one IOException, and the NUL-name test asserts a message substring unique to the branch that fires. - ContainedPathResolver rejects "." and "./" (they normalize to the base itself, which is not a path inside it), and warns instead of silently swallowing an unexpected IOException from ancestor.toRealPath(); a NoSuchFileException there is the dangling-link rejection working and stays quiet. - Reword the ZipUtils ordering comment as a present-tense invariant (the claimed history was false against stage) and the per-call base resolution comments to their true grounds (a caller may construct before the base exists; an existing base can gain a symlink later). - Extract the guarded symlink-creation test helper into SymlinkTestSupport.kt and use it in all three call sites, including the previously unguarded one; add regression tests for the dangling in-base symlink skip and for "." / "./". * ADFA-5257: Reject traversal syntax before the symlink-skip fallback Review of #1736 found a gap between the resolver and unzipFile's symlink-skip fallback: the resolver rejects a ".." segment lexically, but the fallback normalized the entry name before its symlink check, so an entry named a/../link.txt -- with an existing symlink at destDir/link.txt -- was silently skipped as "the user's own link" instead of failing the archive. The narrowing this PR documents ("a ../ entry fails the archive") thus had one path around it whenever a symlink happened to sit at the normalized target. The lexical reject is now extracted from resolve() into ContainedPathResolver.isLexicallyRejected and applied by isContainedSymlink before it looks at the filesystem: an entry that fails on syntax is a bad archive however the disk looks, never fallback material. One shared predicate rather than a duplicate, so the two cannot drift. The new test was confirmed to fail against the unfixed code: the entry was skipped, no IOException. * ADFA-5257: Reject symlink-ancestor escapes in the symlink-skip fallback The fallback stat'ed the entry's normalized path, which follows an ancestor symlink: with dest/a -> /outside and entry "a/link.txt", it stat'ed /outside/link.txt, saw a symlink there, and skipped the entry -- silently tolerating an escaping archive instead of failing it. Now every ancestor between destDir and the candidate must itself be a non-link, so only a symlink whose whole path is real directories inside destDir qualifies for the skip; anything else fails the archive with the containment IOException. The dangling-symlink and existing-symlink skip behaviors are unchanged. Adds a regression test where destDir/a links to an outside directory whose link.txt is itself a symlink; the entry must throw, not skip. * ADFA-5257: Refuse to follow a symlink in the write itself, not just before it The symlink policy is a stat, and the write is a separate open, so a link appearing between them is followed: FileOutputStream resolves links, and Kotlin's File.outputStream() is a thin inline wrapper over it. Both write boundaries now pass LinkOption.NOFOLLOW_LINKS to Files.newOutputStream, which puts O_NOFOLLOW in the open(2) call, so there is no window between deciding and doing. This closes the final component only. A symlink substituted for one of the parent directories is still followed -- by mkdirs() and by the open -- because resolving a path relative to an already-open directory needs openat(2), which java.nio does not expose. Narrowing that further means JNI or a different extraction strategy, so it is recorded in both files rather than implied away. Worth stating the exposure while it is fresh: for the asset installer destDir is app-private storage, which another app cannot write to, so the race needs code execution in this process or root. For project archives extracted into user-visible storage the window is real. ZipUtilsTest covers the enforcement directly -- writeNoFollow is internal for that reason, since the policy check above it means a race is otherwise the only way to reach the open, and a race is not something a test can stage reliably. Without NOFOLLOW_LINKS the same test writes "payload" through the link and fails. 84 common tests and 294 app tests pass. Found in review of PR #1736. * ADFA-5257: Correct a test comment that outlived the guard it describes The comment on the symlinked-grandparent test still explained the depth choice in terms of a toRealPath() check running after createDirectories(). This branch moved containment ahead of every mkdir, so that check is gone and neither depth reaches a mkdir at all. Two levels is still the right shape for the test, for a different reason: "linked/sub/nested.txt" has no ".." and does start with destDir, so it is exactly the case a lexical check alone lets through. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4sTwYg47aK8VB9kRKZicU * ADFA-5257: Surface the resolver tri-state; fail outside-pointing symlinks Re-review follow-ups: - ContainedPathResolver.resolve() returns a sealed Resolution -- Contained / Rejected / Unverifiable -- so "escapes" and "could not be determined" no longer share one null. Both extraction call sites now throw distinct messages: escape, symlink refusal, and cannot-verify (naming the cause). - unzipFile's symlink-skip fallback skips only a pre-existing link that stays inside destDir; a link leading outside -- live, or dangling by its lexical target -- fails the archive again, restoring the old canonicalPath behavior. The KDoc states one policy instead of two. - The installer's "refusing to extract over an existing symlink" branch is reachable again: a symlink at the entry's own target (in-base, dangling, or outside) reports as that refusal, not as zip-slip. - A "." or "./" root directory entry is tolerated as a no-op at both extraction call sites; the resolver itself stays strict. - Layer 2 has one implementation, lexicalResolve() inside the resolver, and the rejected path travels to callers via Rejected.lexicalTarget, leaving only the ancestor/leaf link walk local to ZipUtils. Tests: outside-pointing symlink entries (live and dangling) fail the archive, in-base skips still pass, root entries no-op, all three messages pinned in both callers, and a symlink loop pins Unverifiable deterministically even where the permission-based test is skipped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RKwjPVUcfXJdKP8StU5RDR * ADFA-5257: Brace every when entry the way Spotless formats them The Build Universal APK check failed on spotlessKotlinCheck: the ktlint ruleset Spotless runs braces all entries of a when whose other entries are braced, and separates multi-line entries with a blank line. Apply exactly the formatting its diff demanded to the two resolution whens. No behavior change. * ADFA-5257: Deny absolute paths the root-entry tolerance in namesBase "/" and "\" split into all-empty segments just like "./", so namesBase answered true for them and an absolute directory entry would have been waved through as the archive's root entry (CodeRabbit review). Apply the existing lexical reject first -- it already refuses absolute paths and the empty string -- and pin the boundary with a test. * ADFA-5257: Verify an absent base's nearest existing ancestor resolve() accepted a confirmed-absent base outright, on the reasoning that nothing on disk could be symlinked through. Its *existing* ancestors are on disk, though: with base root/link/missing where root/link points outside root, resolve() returned Contained and a later mkdirs/newOutputStream followed the link, planting the whole "contained" tree outside the base (CodeRabbit, PR #1736). When the base is absent, walk to the nearest existing ancestor of the resolved path (the same NOFOLLOW walk layer 3 already uses). Everything between that ancestor and the target is absent, so the only place a link can hide is the ancestor itself: a symlink there -- a dangling- symlink base included -- is Rejected, and an ancestor that will not toRealPath() is Unverifiable. A plain missing tree beneath real ancestors still resolves to Contained, so first-run installer directories keep working. Regression tests: symlinked ancestor of an absent base, dangling- symlink base, absent base under real ancestors, and a symlink loop above an absent base. The first two fail against the previous code. * ADFA-5257: Record why the absent-base branch is stricter than the other 91b1803 refuses a symlinked nearest-existing-ancestor when the base is absent. The existing-base path answers Contained for the same topology: with base root/link/missing it resolves root/link and the startsWith(realBase) comparison is satisfied, because the link relocates the base and the target together. Measured rather than argued -- creating the base between two otherwise identical calls flips Rejected to Contained. Comment only, no behaviour change. The asymmetry is worth keeping: the branch can only fail closed, and neither production caller reaches it, since ZipUtils.unzipFile and AssetsInstallationHelper.extractZipToDir both create destDir on the line above the resolver construction. The note says what to decide if a caller ever does resolve against a not-yet- created base -- whether an ancestor link is an escape from the base or just where the caller put it. Written down so the next reader does not re-file it as a defect. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4sTwYg47aK8VB9kRKZicU * ADFA-5257: Judge an absent base against its real location, like a created one 91b1803 closed the absent-base hole by rejecting a symlinked nearest-existing-ancestor outright. Review measured the asymmetry that left: with base root/link/missing and root/link -> outside, the absent base answered Rejected while the identical tree after createDirectories(base) answered Contained, because the existing-base path resolves realBase *through* symlinks and compares real paths. A symlink between the base and the filesystem root is where the caller's base lives, not an escape from it, so the absent-base path now answers the same question the same way: walk to the nearest existing ancestor and resolve it with toRealPath(), sharing the existing-base branch's ancestor resolution. The segments below the ancestor are absent, layer-1-vetted plain names, so the base's real location is the ancestor's real path plus those segments and containment holds by construction once the ancestor resolves. Nothing is accepted unvalidated, and fail-closed is kept where there is no real location to judge against: a dangling link (a base that is itself one included) still throws NoSuchFileException from toRealPath() and stays Rejected, any other resolution failure stays Unverifiable (the symlink-loop test is unchanged), and CodeRabbit's original escape never resurfaces because a Contained answer always rests on a resolved real path. Tests: the symlinked-ancestor regression test now pins Contained with the resolved target, a new test pins the measured symmetry itself (same tree, absent then created, identical answers), and the dangling-base test keeps Rejected under the unified rule. * ADFA-5257: Treat undeterminable ancestors as unverifiable The nearest-existing-ancestor walk probed with Files.exists(), which answers false both for "absent" and for "cannot be determined" (an intermediate directory denying execute, say). An entry that merely could not be checked read as absent, the walk carried on to a readable ancestor, and a path nothing had verified -- possibly a symlink under the unreadable directory -- came back Contained. Probe with readAttributes() instead: only a confirmed NoSuchFileException advances the walk; any other IOException is Unverifiable, matching how the base and ancestor resolutions already distinguish absence from failure. Regression test makes an intermediate directory mode 000 and asserts Unverifiable; it skips itself (via a probe-based Assume) where permissions do not bind, e.g. running as root, and was confirmed to fail against the unfixed code under a non-root uid. * ADFA-5257: Guard POSIX permission probe and document blocking I/O Two review findings on the previous commit: - The inaccessible-ancestor test read Files.getPosixFilePermissions() before any assumption, so a filesystem without a PosixFileAttributeView errored the test with UnsupportedOperationException (not an IOException) instead of skipping it. The view is now probed with Files.getFileAttributeView() and assumed non-null before the first permission read; the chmod-000-and-restore behavior is unchanged. - resolve()'s KDoc now states its blocking contract: it performs synchronous filesystem I/O (toRealPath, readAttributes) on every call, so it must not run on the UI thread. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 51a5c2b commit 52038f0

10 files changed

Lines changed: 1406 additions & 70 deletions

File tree

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

Lines changed: 65 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ import androidx.annotation.WorkerThread
66
import com.aayushatharva.brotli4j.Brotli4jLoader
77
import com.itsaky.androidide.app.configuration.IDEBuildConfigProvider
88
import com.itsaky.androidide.resources.R
9+
import com.itsaky.androidide.utils.ContainedPathResolver
10+
import com.itsaky.androidide.utils.ContainedPathResolver.Resolution
911
import com.itsaky.androidide.utils.Environment.DEFAULT_ROOT
1012
import com.itsaky.androidide.utils.useEntriesEach
1113
import kotlinx.coroutines.Dispatchers
@@ -29,7 +31,9 @@ import java.io.FileNotFoundException
2931
import java.io.IOException
3032
import java.io.InputStream
3133
import java.nio.file.Files
34+
import java.nio.file.LinkOption
3235
import java.nio.file.Path
36+
import java.nio.file.StandardOpenOption
3337
import java.util.Locale
3438
import java.util.UUID
3539
import java.util.concurrent.ConcurrentHashMap
@@ -254,62 +258,88 @@ object AssetsInstallationHelper {
254258
destDir: Path,
255259
) = extractZipToDir(Files.newInputStream(srcFile), destDir)
256260

261+
/**
262+
* Containment is [ContainedPathResolver]'s, shared with `ZipUtils.unzipFile`. It does *not*
263+
* memoize: the per-parent cache this loop used to keep was measured against a real 1.8 GB asset
264+
* installation and bought nothing (48.0s without it, 51.4s with), so every path is re-verified
265+
* against the filesystem rather than trusting an ancestor proven earlier.
266+
*
267+
* What stays local is the policy: this refuses to write through *any* existing symlink at an
268+
* entry's target -- in-base, dangling, or pointing outside destDir -- and says so. An installer
269+
* directory reused across runs is the case that matters, and unlike unzipping a user's project
270+
* there is no legitimate reason for a symlink to be there. The three failure messages are kept
271+
* distinct on purpose: an escaping entry is a hostile archive, a symlink at the target is this
272+
* policy, and unverifiable containment is a filesystem problem -- a 1.8 GB install that dies
273+
* 9,000 entries in should name the real cause (ADFA-5257 review).
274+
*/
257275
@WorkerThread
258276
internal fun extractZipToDir(
259277
srcStream: InputStream,
260278
destDir: Path,
261279
) {
262280
Files.createDirectories(destDir)
263-
// Normalize and make destDir absolute for secure path validation
264-
val normalizedDestDir = destDir.toAbsolutePath().normalize()
265-
val realDestDir = normalizedDestDir.toRealPath()
266-
267-
// Zip entries are commonly clustered by directory (e.g. dozens of files
268-
// under the same build-tools/<version>/ prefix); cache the last-verified
269-
// parent so consecutive entries under it skip a redundant toRealPath() call.
270-
// Nothing below can turn an already-verified real directory into a symlink
271-
// mid-run, so caching by lexical parent equality is safe.
272-
var lastVerifiedParent: Path? = null
281+
val contained = ContainedPathResolver(destDir.toFile())
273282

274283
ZipInputStream(srcStream.buffered()).useEntriesEach { zipInput, entry ->
275-
// Validate entry name doesn't contain dangerous patterns
276-
if (entry.name.contains("..") || entry.name.startsWith("/") || entry.name.startsWith("\\")) {
277-
throw IllegalStateException("Zip entry contains dangerous path components: ${entry.name}")
284+
// A "." or "./" root directory entry names destDir itself, which already exists. The
285+
// asset zips are refreshed from an external URL, and archivers that emit such an entry
286+
// exist -- a no-op, not a reason to abort the installation (ADFA-5257 review).
287+
if (entry.isDirectory && ContainedPathResolver.namesBase(entry.name)) {
288+
return@useEntriesEach
278289
}
279290

280-
val destFile = normalizedDestDir.resolve(entry.name).normalize()
291+
val destFile =
292+
when (val resolution = contained.resolve(entry.name)) {
293+
is Resolution.Contained -> {
294+
resolution.file.toPath()
295+
}
281296

282-
// Use Path.startsWith() for proper path validation instead of string comparison
283-
if (!destFile.startsWith(normalizedDestDir)) {
284-
// DO NOT allow extraction to outside of the target dir
285-
throw IllegalStateException("Entry is outside of the target dir: ${entry.name}")
286-
}
297+
is Resolution.Rejected -> {
298+
// A pre-existing symlink at the entry's own target -- dangling, or leading
299+
// outside destDir -- is this caller's refusal policy at work, not a
300+
// zip-slip attempt; report it as such.
301+
val overSymlink = resolution.lexicalTarget?.let { Files.isSymbolicLink(it) } == true
302+
throw IllegalStateException(
303+
if (overSymlink) {
304+
"Refusing to extract over an existing symlink: ${entry.name}"
305+
} else {
306+
"Zip entry escapes the target dir: ${entry.name}"
307+
},
308+
)
309+
}
310+
311+
is Resolution.Unverifiable -> {
312+
throw IllegalStateException(
313+
"Cannot verify that a zip entry stays in the target dir: ${entry.name} (${resolution.cause})",
314+
resolution.cause,
315+
)
316+
}
317+
}
287318

288-
// The checks above are lexical (entry name only) and don't catch a symlink
289-
// already present on disk (e.g. destDir merged/reused across installer
290-
// runs). Reject writing through an existing symlink up front, then
291-
// re-check containment against the real, on-disk path once created.
319+
// Policy, not containment: the resolver allows a symlink whose target is still inside
320+
// destDir, and this caller does not.
292321
if (Files.isSymbolicLink(destFile)) {
293322
throw IllegalStateException("Refusing to extract over an existing symlink: ${entry.name}")
294323
}
295324

296325
if (entry.isDirectory) {
297326
Files.createDirectories(destFile)
298-
if (!destFile.toRealPath().startsWith(realDestDir)) {
299-
throw IllegalStateException("Entry escapes the target dir via symlink: ${entry.name}")
300-
}
301327
} else {
302328
Files.createDirectories(destFile.parent)
303-
if (destFile.parent != lastVerifiedParent) {
304-
if (!destFile.parent.toRealPath().startsWith(realDestDir)) {
305-
throw IllegalStateException("Entry parent escapes the target dir via symlink: ${entry.name}")
329+
// NOFOLLOW_LINKS: the isSymbolicLink check above is a stat, and this is a separate
330+
// open, so a link appearing in between would be followed. O_NOFOLLOW makes the
331+
// refusal part of the open. Parent directories are still followed -- that needs
332+
// openat(2), which java.nio does not expose (ADFA-5257 review).
333+
Files
334+
.newOutputStream(
335+
destFile,
336+
StandardOpenOption.WRITE,
337+
StandardOpenOption.CREATE,
338+
StandardOpenOption.TRUNCATE_EXISTING,
339+
LinkOption.NOFOLLOW_LINKS,
340+
).use { dest ->
341+
zipInput.copyTo(dest)
306342
}
307-
lastVerifiedParent = destFile.parent
308-
}
309-
310-
Files.newOutputStream(destFile).use { dest ->
311-
zipInput.copyTo(dest)
312-
}
313343
}
314344
}
315345
}

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

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -550,10 +550,20 @@ class GradleBuildService :
550550
}
551551
try {
552552
val projectDir = ProjectManagerImpl.getInstance().projectDir
553-
val files = ZipUtils.unzipFile(extracted, projectDir)
554-
if (files.isNotEmpty()) {
553+
val result = ZipUtils.unzipFile(extracted, projectDir)
554+
if (result.skipped.isNotEmpty()) {
555+
log.warn("Gradle wrapper entries not extracted (existing symlinks left alone): {}", result.skipped)
556+
}
557+
558+
// Success means the wrapper is actually usable, not merely that unzipFile returned: an
559+
// entry skipped over a user's own symlink is fine as long as the files it needs exist.
560+
val missing =
561+
listOf("gradlew", "gradle/wrapper/gradle-wrapper.jar", "gradle/wrapper/gradle-wrapper.properties")
562+
.filter { !File(projectDir, it).exists() }
563+
if (missing.isEmpty()) {
555564
return GradleWrapperCheckResult(true)
556565
}
566+
log.error("Gradle wrapper installation is incomplete; missing: {}", missing)
557567
} catch (e: IOException) {
558568
log.error("An error occurred while extracting Gradle wrapper", e)
559569
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,6 @@ public UnzipCallable(File src, File dest) {
3737

3838
@Override
3939
public List<File> call() throws Exception {
40-
return ZipUtils.unzipFile(src, dest);
40+
return ZipUtils.unzipFile(src, dest).getExtracted();
4141
}
4242
}

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

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -183,24 +183,32 @@ class AssetsInstallationHelperTest {
183183
ByteArrayOutputStream().use { baos ->
184184
ZipOutputStream(baos).use { zos ->
185185
// Two levels below the symlink ("linked/sub/nested.txt", no directory
186-
// entries), not one: for a one-level entry ("linked/nested.txt"),
187-
// destFile.parent IS the symlink, so Files.createDirectories() throws
188-
// FileAlreadyExistsException (NOFOLLOW_LINKS rejects the existing
189-
// symlink-to-dir) before the toRealPath() guard below it ever runs. One
190-
// level deeper, createDirectories() silently traverses the symlink to
191-
// create "sub" for real inside outsideDir, and only then does the
192-
// toRealPath() check on destFile.parent fire -- which is what this test
193-
// exercises.
186+
// entries), not one. The depth used to decide which guard caught it, back
187+
// when containment was re-checked with toRealPath() after
188+
// createDirectories() had already run: one level down, createDirectories()
189+
// threw FileAlreadyExistsException on the symlink before that check was
190+
// reached. ADFA-5257 moved containment ahead of every mkdir, so both
191+
// depths are now refused by ContainedPathResolver with nothing created.
192+
// Kept at two levels because that is the case a lexical check alone lets
193+
// through -- "linked/sub/nested.txt" has no ".." and does start with
194+
// destDir, so only resolving "linked" to its real path catches it.
194195
zos.putNextEntry(ZipEntry("linked/sub/nested.txt"))
195196
zos.write(content.toByteArray())
196197
zos.closeEntry()
197198
}
198199
baos.toByteArray()
199200
}
200201

201-
assertThrows(IllegalStateException::class.java) {
202-
AssetsInstallationHelper.extractZipToDir(ByteArrayInputStream(zipBytes), destDir)
203-
}
202+
val thrown =
203+
assertThrows(IllegalStateException::class.java) {
204+
AssetsInstallationHelper.extractZipToDir(ByteArrayInputStream(zipBytes), destDir)
205+
}
206+
// The symlink sits at an *ancestor*, not at the entry's own target: this is an escape,
207+
// and the message must say so -- distinct from the refusal over a symlink at the target.
208+
assertTrue(
209+
"expected an escape message, got: ${thrown.message}",
210+
thrown.message!!.contains("escapes the target dir"),
211+
)
204212
} finally {
205213
outsideDir.deleteRecursivelyWithoutFollowingLinks()
206214
destDir.deleteRecursivelyWithoutFollowingLinks()

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

Lines changed: 139 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -123,18 +123,48 @@ class ExtractZipToDirMergeTest {
123123
}
124124
}
125125

126+
// The lexical guard used to be a bare substring reject, so an entry legitimately named with
127+
// consecutive dots aborted the whole installation. The shared resolver rejects a ".." *segment*
128+
// instead, which lets a name like this through.
129+
@Test
130+
fun `extracts an entry whose name merely contains a double dot`() {
131+
val destDir = Files.createTempDirectory("assets-dots")
132+
try {
133+
AssetsInstallationHelper.extractZipToDir(
134+
zipOf("lib/notes..txt" to "kept", "lib/a..b/c.txt" to "also kept"),
135+
destDir,
136+
)
137+
138+
assertEquals("kept", destDir.resolve("lib/notes..txt").toFile().readText())
139+
assertEquals("also kept", destDir.resolve("lib/a..b/c.txt").toFile().readText())
140+
} finally {
141+
destDir.deleteRecursivelyWithoutFollowingLinks()
142+
}
143+
}
144+
126145
@Test
127146
fun `rejects path traversal`() {
128147
val dest = Files.createTempDirectory("mvn")
129148
try {
130-
assertThrows(IllegalStateException::class.java) {
131-
AssetsInstallationHelper.extractZipToDir(zipOf("../evil.jar" to "x"), dest)
132-
}
149+
val thrown =
150+
assertThrows(IllegalStateException::class.java) {
151+
AssetsInstallationHelper.extractZipToDir(zipOf("../evil.jar" to "x"), dest)
152+
}
153+
// A real escape is reported as one -- distinct from the symlink-refusal and
154+
// cannot-verify messages below.
155+
assertTrue(
156+
"expected an escape message, got: ${thrown.message}",
157+
thrown.message!!.contains("escapes the target dir"),
158+
)
133159
} finally {
134160
dest.deleteRecursivelyWithoutFollowingLinks()
135161
}
136162
}
137163

164+
// The installer's own policy branch: any pre-existing symlink at an entry's target refuses the
165+
// extraction, and says so. A *dangling* link is the case the resolver refuses before the
166+
// explicit isSymbolicLink check is reached, so asserting the message (not just the type) pins
167+
// that it still surfaces as the symlink refusal, not as a zip-slip accusation.
138168
@Test
139169
fun `rejects extraction over an existing symlink`() {
140170
val dest = Files.createTempDirectory("mvn")
@@ -143,15 +173,118 @@ class ExtractZipToDirMergeTest {
143173
val outsideTarget = outside.resolve("payload")
144174
Files.createSymbolicLink(dest.resolve("evil.jar"), outsideTarget)
145175

146-
assertThrows(IllegalStateException::class.java) {
147-
AssetsInstallationHelper.extractZipToDir(zipOf("evil.jar" to "x"), dest)
148-
}
176+
val thrown =
177+
assertThrows(IllegalStateException::class.java) {
178+
AssetsInstallationHelper.extractZipToDir(zipOf("evil.jar" to "x"), dest)
179+
}
180+
assertTrue(
181+
"expected the symlink refusal message, got: ${thrown.message}",
182+
thrown.message!!.contains("Refusing to extract over an existing symlink"),
183+
)
149184
} finally {
150185
dest.deleteRecursivelyWithoutFollowingLinks()
151186
outside.deleteRecursivelyWithoutFollowingLinks()
152187
}
153188
}
154189

190+
// Same refusal for a live link whose target is inside destDir -- the resolver proves it
191+
// contained, and the installer's explicit isSymbolicLink check refuses it anyway.
192+
@Test
193+
fun `rejects extraction over an existing symlink pointing inside destDir`() {
194+
val dest = Files.createTempDirectory("mvn")
195+
try {
196+
Files.write(dest.resolve("real.jar"), "kept".toByteArray())
197+
Files.createSymbolicLink(dest.resolve("evil.jar"), dest.resolve("real.jar"))
198+
199+
val thrown =
200+
assertThrows(IllegalStateException::class.java) {
201+
AssetsInstallationHelper.extractZipToDir(zipOf("evil.jar" to "x"), dest)
202+
}
203+
assertTrue(
204+
"expected the symlink refusal message, got: ${thrown.message}",
205+
thrown.message!!.contains("Refusing to extract over an existing symlink"),
206+
)
207+
assertEquals("kept", String(Files.readAllBytes(dest.resolve("real.jar"))))
208+
} finally {
209+
dest.deleteRecursivelyWithoutFollowingLinks()
210+
}
211+
}
212+
213+
// And for a live link pointing outside destDir -- refused by the resolver's real-path check,
214+
// still reported as the symlink refusal it is, with nothing written through the link.
215+
@Test
216+
fun `rejects extraction over an existing symlink pointing outside destDir`() {
217+
val dest = Files.createTempDirectory("mvn")
218+
val outside = Files.createTempDirectory("outside")
219+
try {
220+
val outsideTarget = outside.resolve("payload")
221+
Files.write(outsideTarget, "original".toByteArray())
222+
Files.createSymbolicLink(dest.resolve("evil.jar"), outsideTarget)
223+
224+
val thrown =
225+
assertThrows(IllegalStateException::class.java) {
226+
AssetsInstallationHelper.extractZipToDir(zipOf("evil.jar" to "x"), dest)
227+
}
228+
assertTrue(
229+
"expected the symlink refusal message, got: ${thrown.message}",
230+
thrown.message!!.contains("Refusing to extract over an existing symlink"),
231+
)
232+
assertEquals("original", String(Files.readAllBytes(outsideTarget)))
233+
} finally {
234+
dest.deleteRecursivelyWithoutFollowingLinks()
235+
outside.deleteRecursivelyWithoutFollowingLinks()
236+
}
237+
}
238+
239+
// The third message: containment that cannot be *verified* (here a symlink loop, ELOOP) is
240+
// neither an escape nor the symlink refusal -- it aborts naming the filesystem cause, so a
241+
// failing install points at the disk, not at the archive.
242+
@Test
243+
fun `reports unverifiable containment distinctly`() {
244+
val dest = Files.createTempDirectory("mvn")
245+
try {
246+
Files.createSymbolicLink(dest.resolve("loop-a"), dest.resolve("loop-b"))
247+
Files.createSymbolicLink(dest.resolve("loop-b"), dest.resolve("loop-a"))
248+
249+
val thrown =
250+
assertThrows(IllegalStateException::class.java) {
251+
AssetsInstallationHelper.extractZipToDir(zipOf("loop-a/file.txt" to "x"), dest)
252+
}
253+
assertTrue(
254+
"expected the cannot-verify message, got: ${thrown.message}",
255+
thrown.message!!.contains("Cannot verify"),
256+
)
257+
} finally {
258+
dest.deleteRecursivelyWithoutFollowingLinks()
259+
}
260+
}
261+
262+
// A "." or "./" root directory entry names destDir itself. Some archivers emit one, and the
263+
// asset zips are refreshed from an external URL -- it must be a no-op, not an aborted install.
264+
@Test
265+
fun `tolerates a root directory entry instead of aborting`() {
266+
val dest = Files.createTempDirectory("mvn")
267+
try {
268+
val zipBytes =
269+
ByteArrayOutputStream().use { baos ->
270+
ZipOutputStream(baos).use { zip ->
271+
zip.putNextEntry(ZipEntry("./"))
272+
zip.closeEntry()
273+
zip.putNextEntry(ZipEntry("com/foo/a.txt"))
274+
zip.write("kept".toByteArray())
275+
zip.closeEntry()
276+
}
277+
baos.toByteArray()
278+
}
279+
280+
AssetsInstallationHelper.extractZipToDir(ByteArrayInputStream(zipBytes), dest)
281+
282+
assertEquals("kept", String(Files.readAllBytes(dest.resolve("com/foo/a.txt"))))
283+
} finally {
284+
dest.deleteRecursivelyWithoutFollowingLinks()
285+
}
286+
}
287+
155288
@Test
156289
fun `rejects extraction into a symlinked parent that escapes destDir`() {
157290
val dest = Files.createTempDirectory("mvn")

0 commit comments

Comments
 (0)