ADFA-5005: Fix SDK bootstrap crash extracting android-sdk.zip - #1621
ADFA-5005: Fix SDK bootstrap crash extracting android-sdk.zip#1621davidschachterADFA wants to merge 6 commits into
Conversation
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 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
Warning Review limit reached
Next review available in: 16 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 Walkthrough
WalkthroughZIP extraction now creates each destination file’s parent directory before opening the output stream. A regression test covers nested ZIP entries without explicit directory entries and verifies the extracted contents. ChangesZIP extraction
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt (1)
266-269: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for ZIP files without directory entries.
Create a ZIP containing a nested file entry such as
build-tools/35.0.0/NOTICE.txtwithout directory entries. Verify that extraction creates the parent directories and preserves the file contents inapp/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt.As per coding guidelines, changed non-UI logic must include tests for edge paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt` around lines 266 - 269, Add a regression test in AssetsInstallationHelperTest for extracting a ZIP containing a nested file entry such as build-tools/35.0.0/NOTICE.txt without directory entries; verify extraction creates the required parent directories and preserves the file contents.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt`:
- Line 266: Update the archive extraction flow around destFile to validate the
canonical destination after Files.createDirectories(destFile.parent): resolve
the created parent path and ensure it remains under the intended destination,
reject any symlink components, and reject destFile when it already exists as a
symlink before calling Files.newOutputStream(destFile). Preserve the existing
lexical normalize()/startsWith() checks while preventing symlink traversal
during directory creation and file writing.
---
Nitpick comments:
In `@app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt`:
- Around line 266-269: Add a regression test in AssetsInstallationHelperTest for
extracting a ZIP containing a nested file entry such as
build-tools/35.0.0/NOTICE.txt without directory entries; verify extraction
creates the required parent directories and preserves the file contents.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d2682c36-fc1c-4307-825a-a9068fced8d0
📒 Files selected for processing (1)
app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt
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 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt (1)
10-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse JUnit Jupiter and Truth for the new regression test.
AssetsInstallationHelperTest.ktimports and uses JUnit 4 APIs. Replaceorg.junit.Assert,Before, andTestwith the required JUnit Jupiter/Truth stack, and update@Beforeplus assertion argument order for the affected assertions.Also applies to: 58-59, 77-78
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt` around lines 10 - 14, Update AssetsInstallationHelperTest to use JUnit Jupiter lifecycle and test annotations instead of JUnit 4, and replace org.junit.Assert assertions with Truth assertions. Adjust the affected assertion calls, including those near the referenced assertion ranges, to Truth’s expected argument order while preserving the existing test behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt`:
- Around line 10-14: Update AssetsInstallationHelperTest to use JUnit Jupiter
lifecycle and test annotations instead of JUnit 4, and replace org.junit.Assert
assertions with Truth assertions. Adjust the affected assertion calls, including
those near the referenced assertion ranges, to Truth’s expected argument order
while preserving the existing test behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 202f50d0-575c-4faa-8436-f7b4e39b1b98
📒 Files selected for processing (1)
app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…alls 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 <noreply@anthropic.com>
| } else { | ||
| Files.createDirectories(destFile.parent) | ||
| if (destFile.parent != lastVerifiedParent) { | ||
| if (!destFile.parent.toRealPath().startsWith(realDestDir)) { |
There was a problem hiding this comment.
These toRealPath() escape checks have no test coverage. Every symlink test in ExtractZipToDirMergeTest presents the symlink as a directory entry — zipOf emits an ancestor directory entry, and rejects a bare directory entry… uses linked/ — so they all trip the leaf Files.isSymbolicLink(destFile) throw a few lines up first, and never reach the directory-branch check or this file-branch check. Deleting lines 279-291 would fail no test.
For the archive this PR actually fixes (android-sdk.zip has zero directory entries), this file-branch check is the only symlink defense that runs — and it's the untested one. Please add a test with a file entry under a pre-existing symlinked parent and no directory entry for that parent — e.g. build the zip manually with just linked/nested.txt, and pre-create dest/linked as a symlink escaping dest. As written, zipOf can't produce that shape because it always injects the linked/ directory entry.
| // 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)) { |
There was a problem hiding this comment.
This symlink hardening lands in only 1 of 3 near-duplicate extraction sites. SplitAssetsInstaller.kt (~L170-190) and BundledAssetsInstaller.kt (~L178-198) each have a near-identical inline plugin-zip loop that still does only the lexical !targetPath.startsWith(pluginDirPath) check — no isSymbolicLink / toRealPath guard.
So either the on-disk-symlink threat is real, in which case those two loops are a matching gap and this defense belongs in a shared helper; or it isn't, in which case this is scope creep. Cleanest fix: have those two loops call extractZipToDir() instead of reimplementing it, so all three sites share one hardened path.
| } | ||
|
|
||
| @Test | ||
| fun `extracts multiple sibling files under the same directory`() { |
There was a problem hiding this comment.
Heads up: the zipOf comment above (lines 14-18) is now false. It says extractZipToDir "only Files.createDirectories() on directory entries, not on every file entry's parent" — but this PR's Files.createDirectories(destFile.parent) (in AssetsInstallationHelper.extractZipToDir) makes exactly that call. Please update the comment.
It also matters for coverage: because zipOf still injects a directory entry for every ancestor, this test — and the others built on zipOf — never exercises the no-directory-entry case that is the actual bug. Only AssetsInstallationHelperTest.`…nested entries with no directory entries` does.
|
|
||
| @Test | ||
| fun `rejects extraction over an existing symlink`() { | ||
| val dest = Files.createTempDirectory("mvn") |
There was a problem hiding this comment.
These symlink tests leak temp dirs, and set a data-loss trap for the next person. No test in this file has an @After/finally, so every run leaves the mvn*/outside* temp dirs behind — three of them (here, and the two linked cases below) containing symlinks that point outside the temp dir.
Harmless today only because nothing cleans up. But the sibling file already establishes dest.toFile().deleteRecursively() as the cleanup pattern (AssetsInstallationHelperTest), and Kotlin's File.deleteRecursively() follows symlinks — so copying that pattern here would recurse through dest/evil.jar / dest/linked and delete the target's contents. Please add cleanup that removes the links, not their targets (e.g. an @After that Files.deleteIfExists per created path, or walks with NOFOLLOW_LINKS).
2 participants
Summary
AssetsInstallationHelper.extractZipToDir()only created parent directories for zip entries explicitly flagged as directories, but the bundledandroid-sdk.zip.brhas zero directory entries. The archive's very first entry (build-tools/35.0.0/NOTICE.txt) always failed withNoSuchFileExceptionsinceANDROID_HOMEis wiped before each install.Ticket: ADFA-5005
Test plan
:app:compileV8DebugKotlin— compiles clean:app:assembleV8Debugwith the fix and installed on a physical arm64 deviceandroid-sdk.zipnow extracts without error (previously crashed on entry #0)files/home/android-sdk/build-tools/35.0.0/NOTICE.txtexists at the expected 1,068,025 bytes🤖 Generated with Claude Code