ADFA-3787: add archive and environment services for plugins#1223
Merged
Conversation
Lets plugins install IDE-managed tooling (NDK, build-tools) without
each bundling compression libraries.
- IdeEnvironmentService exposes ANDROIDIDE_HOME, ANDROID_HOME, NDK,
tmp, and per-plugin data dirs.
- IdeArchiveService extracts XZ/GZIP/TAR/TAR.XZ/TAR.GZ/ZIP with
path-traversal + symlink checks and interruptible progress.
- New IDE_ENVIRONMENT_WRITE permission gates writes into IDE-managed
dirs; filesystem allow-list extended accordingly.
- IdeFileService gains writeBinary, writeStream, delete.
- ResourceManager gains openPluginResource and openPluginAsset for
streaming large bundled payloads.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds IDE environment and archive extraction services, stream-based plugin resource/asset APIs, expanded file I/O (binary/stream/delete), a new Changes
Sequence DiagramsequenceDiagram
participant Plugin
participant PluginManager
participant ArchiveService as IdeArchiveServiceImpl
participant Validator as PathValidator
participant FileSystem as FileSystem
participant Stream as InputStream
Plugin->>PluginManager: request extract(source, format, destination, onProgress)
PluginManager->>ArchiveService: extract(source, format, destination, onProgress)
ArchiveService->>ArchiveService: ensure write permission (FILESYSTEM_WRITE or IDE_ENVIRONMENT_WRITE)
ArchiveService->>Validator: validate destination and entry paths
alt validation or permission failure
ArchiveService-->>Plugin: ExtractResult.Failure(error)
else
ArchiveService->>Stream: read archive / compressed bytes
loop per entry / chunk
ArchiveService->>ArchiveService: check Thread.interrupted()
ArchiveService->>Validator: validateEntryPath(canonicalTarget)
ArchiveService->>FileSystem: create dirs / write file bytes
ArchiveService->>Plugin: onProgress(bytesProcessed, currentEntry)
end
ArchiveService-->>Plugin: ExtractResult.Success(bytesWritten, filesExtracted)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeFileServiceImpl.kt (1)
22-35:⚠️ Potential issue | 🟠 Major
readFilenow requires write permission — reads will be rejected for read-only plugins.
ensureWritePermission()passes only if the plugin hasFILESYSTEM_WRITEorIDE_ENVIRONMENT_WRITE. A plugin that only declaresFILESYSTEM_READwill get aSecurityExceptiononreadFile(...). This looks like a copy/paste carry-over from the generalization of the write gate; reads should be gated by a read permission (or allowed for any filesystem permission).🐛 Proposed fix
override fun readFile(file: File): String? { - ensureWritePermission() + ensureReadPermission() ensurePathAllowed(file)…with a corresponding
ensureReadPermission()that acceptsFILESYSTEM_READ(and, if appropriate, alsoFILESYSTEM_WRITE/IDE_ENVIRONMENT_WRITEsince write implies read).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeFileServiceImpl.kt` around lines 22 - 35, The readFile method incorrectly calls ensureWritePermission(), causing read-only plugins to be blocked; replace that call with a new ensureReadPermission() and implement ensureReadPermission to allow FILESYSTEM_READ and also treat FILESYSTEM_WRITE and IDE_ENVIRONMENT_WRITE as valid (write implies read); keep the existing ensurePathAllowed(file) check and ensure readFile still returns null on exceptions as before.
🧹 Nitpick comments (2)
plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/context/PluginContextImpl.kt (1)
83-98: Narrow exception handling for resource streams.
openPluginResource()doesn't need the try-catch sinceClassLoader.getResourceAsStream()returnsnullfor missing resources instead of throwing.openPluginAsset()should catch onlyIOException(the documented exception fromAssetManager.open()) instead of all exceptions.♻️ Suggested changes
override fun openPluginResource(name: String): InputStream? { - return try { - classLoader.getResourceAsStream(name) - } catch (e: Exception) { - null - } + return classLoader.getResourceAsStream(name) } override fun openPluginAsset(path: String): InputStream? { val assets = assetManager ?: return null return try { assets.open(path) - } catch (e: Exception) { + } catch (e: IOException) { null } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/context/PluginContextImpl.kt` around lines 83 - 98, Remove the broad try-catch blocks: in openPluginResource() remove the try/catch and return classLoader.getResourceAsStream(name) directly since getResourceAsStream returns null rather than throwing; in openPluginAsset() narrow the catch to only java.io.IOException (catch IOException e) when calling assetManager.open(path) instead of catching Exception so other unexpected runtime exceptions propagate.plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeFileServiceImpl.kt (1)
202-217: Allowlist construction is duplicated betweenIdeFileServiceImplandIdeArchiveServiceImpl.
getDefaultAllowedPaths()here anddefaultAllowedPaths()inIdeArchiveServiceImpl(lines 232‑245) are identical, including theIDE_ENVIRONMENT_WRITE-gated additions andcanonicalOrSelffallback. Any future change (e.g., adding a new environment directory, or fixing thestartsWithprefix-match discussed on the archive service) has to be made in two places. Consider extracting to a shared helper (e.g., a top-levelPluginPathPolicyobject or acompanion objectaccessible to both impls) that both services depend on.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeFileServiceImpl.kt` around lines 202 - 217, Extract the duplicated allowlist logic from IdeFileServiceImpl.getDefaultAllowedPaths() and IdeArchiveServiceImpl.defaultAllowedPaths() into a single shared helper (e.g., a top-level object PluginPathPolicy or a companion object) that exposes a function like defaultAllowedPaths(pluginId: String, permissions: Set<PluginPermission>) and rework both classes to call that helper; the helper should build the same base list (Environment.PROJECTS_FOLDER paths, System.getProperty("user.home"), "/tmp/CodeOnTheGoProject") and apply the PluginPermission.IDE_ENVIRONMENT_WRITE gated additions using canonicalOrSelf(Environment.ANDROID_HOME), canonicalOrSelf(Environment.TMP_DIR), and canonicalOrSelf(File(File(Environment.ANDROIDIDE_HOME, PLUGIN_DATA_ROOT), pluginId)) so behavior is unchanged but maintained in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeArchiveServiceImpl.kt`:
- Around line 32-64: The implementation currently wraps the caller-owned source
in BufferedInputStream and then passes it to compressor/archive streams (e.g.,
XZCompressorInputStream, GzipCompressorInputStream, TarArchiveInputStream,
ZipArchiveInputStream) which close the underlying stream, violating the
IdeArchiveService contract; fix by preventing close propagation: create a small
non-closing delegator InputStream (e.g., NonClosingInputStream that overrides
close() to only flush/no-op) and wrap the original source with it before
creating the BufferedInputStream used by extractSingleStream, extractTar, and
extractZip (so ensureWritePermission and ensurePathAllowed stay the same), or
alternatively update the KDoc to state the implementation will close the source
(prefer the non-closing wrapper approach to preserve the existing contract).
- Around line 213-245: The allowlist check in ensurePathAllowed (and similarly
IdeFileServiceImpl.isPathAllowedDefault) uses string prefix matching on
non-canonical entries which allows sibling-directory spoofing; fix by
canonicalizing every allowed-path entry via canonicalOrSelf in
defaultAllowedPaths and then compare using Path.startsWith (java.nio.file.Path)
or by ensuring each allowed string ends with File.separator before startsWith to
avoid prefix collisions; update defaultAllowedPaths to produce canonical paths
for all entries (including the hardcoded storage/sdcard/home/tmp ones) and
change the match logic in ensurePathAllowed to compare canonical Path objects
(or canonical strings with separator) instead of raw string.startsWith.
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeEnvironmentServiceImpl.kt`:
- Around line 19-25: getPluginDataDirectory() currently ignores the boolean
result of dir.mkdirs() and may return a File that doesn't exist; update the
method (referencing getPluginDataDirectory, dir, pluginId, PLUGIN_DATA_ROOT,
Environment.ANDROIDIDE_HOME) to verify that the directory exists after
attempting to create it and if not throw a clear exception (e.g., IOException or
IllegalStateException) that includes the path and reason, or otherwise return
the dir only when dir.exists() && dir.isDirectory(); include the mkdirs() return
value and wrap/propagate any underlying exception or add a helpful error message
so callers won't receive a cryptic FileNotFoundException later.
---
Outside diff comments:
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeFileServiceImpl.kt`:
- Around line 22-35: The readFile method incorrectly calls
ensureWritePermission(), causing read-only plugins to be blocked; replace that
call with a new ensureReadPermission() and implement ensureReadPermission to
allow FILESYSTEM_READ and also treat FILESYSTEM_WRITE and IDE_ENVIRONMENT_WRITE
as valid (write implies read); keep the existing ensurePathAllowed(file) check
and ensure readFile still returns null on exceptions as before.
---
Nitpick comments:
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/context/PluginContextImpl.kt`:
- Around line 83-98: Remove the broad try-catch blocks: in openPluginResource()
remove the try/catch and return classLoader.getResourceAsStream(name) directly
since getResourceAsStream returns null rather than throwing; in
openPluginAsset() narrow the catch to only java.io.IOException (catch
IOException e) when calling assetManager.open(path) instead of catching
Exception so other unexpected runtime exceptions propagate.
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeFileServiceImpl.kt`:
- Around line 202-217: Extract the duplicated allowlist logic from
IdeFileServiceImpl.getDefaultAllowedPaths() and
IdeArchiveServiceImpl.defaultAllowedPaths() into a single shared helper (e.g., a
top-level object PluginPathPolicy or a companion object) that exposes a function
like defaultAllowedPaths(pluginId: String, permissions: Set<PluginPermission>)
and rework both classes to call that helper; the helper should build the same
base list (Environment.PROJECTS_FOLDER paths, System.getProperty("user.home"),
"/tmp/CodeOnTheGoProject") and apply the PluginPermission.IDE_ENVIRONMENT_WRITE
gated additions using canonicalOrSelf(Environment.ANDROID_HOME),
canonicalOrSelf(Environment.TMP_DIR), and
canonicalOrSelf(File(File(Environment.ANDROIDIDE_HOME, PLUGIN_DATA_ROOT),
pluginId)) so behavior is unchanged but maintained in one place.
🪄 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
Run ID: 1a4c12ec-4c5f-4ae1-bec1-f36df2732155
📒 Files selected for processing (12)
gradle/libs.versions.tomlplugin-api/src/main/kotlin/com/itsaky/androidide/plugins/IPlugin.ktplugin-api/src/main/kotlin/com/itsaky/androidide/plugins/PluginContext.ktplugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeArchiveService.ktplugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeEnvironmentService.ktplugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeServices.ktplugin-manager/build.gradle.ktsplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/context/PluginContextImpl.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManager.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeArchiveServiceImpl.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeEnvironmentServiceImpl.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeFileServiceImpl.kt
jomen-adfa
approved these changes
Apr 21, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeFileServiceImpl.kt`:
- Around line 120-128: The writeStream implementation in IdeFileServiceImpl
currently wraps the caller-owned input in BufferedInputStream(...).use which
closes the caller's stream and also catches Exception broadly (swallowing
interrupts). Fix it by not closing the caller-owned input: create a
BufferedInputStream(input) without using .use or closing it, and only
manage/close the FileOutputStream (use FileOutputStream(file).use { output ->
... } around the write). Update exception handling to catch InterruptedException
separately, call Thread.currentThread().interrupt() and return -1, and catch
other IO/Runtime exceptions to return FAILED_WRITE (leave copyInterruptible and
FAILED_WRITE as used).
- Around line 21-23: The readFile function currently calls
ensureWritePermission(), which incorrectly requires write access (and lets
IDE_ENVIRONMENT_WRITE act as read); change it to require read permission by
replacing the ensureWritePermission() call with ensureReadPermission() (or add
ensureReadPermission() before ensurePathAllowed(file)) so readFile enforces
FILESYSTEM_READ instead of write-level permissions while keeping
ensurePathAllowed(file) intact; update any tests or callers that assumed the old
behavior.
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/PluginPathAllowlist.kt`:
- Around line 20-31: Move the project directory entries out of the
always-allowed list inside defaultAllowedPaths and only include them when the
caller has filesystem permissions (PluginPermission.FILESYSTEM_READ or
PluginPermission.FILESYSTEM_WRITE); leave PluginPermission.IDE_ENVIRONMENT_WRITE
to add only environment roots (canonicalOrSelf(Environment.ANDROID_HOME),
canonicalOrSelf(Environment.TMP_DIR)) and the plugin-specific data root
(canonicalOrSelf(File(File(Environment.ANDROIDIDE_HOME, PLUGIN_DATA_ROOT),
pluginId))). Update defaultAllowedPaths to check for
FILESYSTEM_READ/FILESYSTEM_WRITE before adding the three project paths and
ensure the IDE_ENVIRONMENT_WRITE branch only adds the SDK/tmp/plugin data roots.
🪄 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
Run ID: a5d21778-d903-4a9c-96b6-f23fa94911d1
📒 Files selected for processing (5)
plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeEnvironmentService.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeArchiveServiceImpl.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeEnvironmentServiceImpl.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeFileServiceImpl.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/PluginPathAllowlist.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeEnvironmentService.kt
- plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeEnvironmentServiceImpl.kt
- plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeArchiveServiceImpl.kt
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManager.kt (1)
998-1023: Consider extracting shared service registrations to remove the growing duplication betweencreatePluginContextWithResourcesandcreatePluginContext.
createPluginContextWithResources(907-1107) andcreatePluginContext(1109-1280) register nearly the same set of services with identical factories — including the two newenvironmentandarchiveregistrations added here and mirrored at lines 1192-1217. Every such block (and itspathValidatoranonymous adapter) is a copy-paste, which has already caused drift risk (e.g., any future permission tweak must be repeated in two places). Non-blocking, but worth a small helper, e.g.:♻️ Suggested shape
private fun registerCommonPluginServices( registry: ServiceRegistryImpl, pluginId: String, permissions: Set<PluginPermission>, ) { val fileValidatorAdapter = pathValidator?.let { v -> object : IdeFileServiceImpl.PathValidator { override fun isPathAllowed(path: File) = v.isPathAllowed(path) override fun getAllowedPaths() = v.getAllowedPaths() } } // project / UI / build / tooltip / editor_tab / file / environment / archive / // sidebar / theme / feature_flag / template / snippet registrations here, // using fileValidatorAdapter where applicable. }Both factories then become: build the registry, call
registerCommonPluginServices(...), add any context-specific extras (e.g.,IdeCommandServiceis only in the resources variant today), and return thePluginContextImpl.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManager.kt` around lines 998 - 1023, The duplicated service registration between createPluginContextWithResources and createPluginContext (including the registerServiceWithErrorHandling calls for IdeEnvironmentService/IdeArchiveService, the IdeArchiveServiceImpl construction, and the anonymous adapter for pathValidator implementing IdeFileServiceImpl.PathValidator) should be extracted into a single helper (e.g., registerCommonPluginServices) that takes the ServiceRegistryImpl, pluginId, permissions and the nullable pathValidator and performs all shared registerServiceWithErrorHandling calls; create the fileValidatorAdapter inside that helper by adapting the incoming pathValidator to IdeFileServiceImpl.PathValidator, call this helper from both createPluginContextWithResources and createPluginContext, and leave any context-specific registrations (like IdeCommandService) in their original functions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManager.kt`:
- Around line 998-1023: The duplicated service registration between
createPluginContextWithResources and createPluginContext (including the
registerServiceWithErrorHandling calls for
IdeEnvironmentService/IdeArchiveService, the IdeArchiveServiceImpl construction,
and the anonymous adapter for pathValidator implementing
IdeFileServiceImpl.PathValidator) should be extracted into a single helper
(e.g., registerCommonPluginServices) that takes the ServiceRegistryImpl,
pluginId, permissions and the nullable pathValidator and performs all shared
registerServiceWithErrorHandling calls; create the fileValidatorAdapter inside
that helper by adapting the incoming pathValidator to
IdeFileServiceImpl.PathValidator, call this helper from both
createPluginContextWithResources and createPluginContext, and leave any
context-specific registrations (like IdeCommandService) in their original
functions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 228652cb-1b35-4529-8b22-0d7e023744c9
📒 Files selected for processing (3)
plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/IPlugin.ktplugin-manager/build.gradle.ktsplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManager.kt
✅ Files skipped from review due to trivial changes (1)
- plugin-manager/build.gradle.kts
🚧 Files skipped from review as they are similar to previous changes (1)
- plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/IPlugin.kt
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeArchiveServiceImpl.kt`:
- Around line 43-53: The TAR_XZ branch is incorrect: it writes the raw source
(not the buffered stream), never calls ensureDirectory/ensurePathAllowed,
creates a temp file in destination.parentFile (bypassing allowlist) and returns
zeroed ExtractResult.Success; fix by mirroring extractTar/extractZip
behavior—call ensurePathAllowed(destination) and ensureDirectory(destination)
first, avoid creating temp files in destination.parentFile, and instead wrap the
existing buffered InputStream with XZCompressorInputStream then feed that into
TarArchiveInputStream (reusing the same extraction logic that enforces path
traversal, symlink handling, progress and interrupts) so you preserve
bytesWritten/filesExtracted; finally return the actual ExtractResult.Success
populated from the reused tar-extraction code rather than new
ExtractResult.Success(0, 0); remove or deprecate extractTarXzViaTermux usage
accordingly.
- Around line 244-289: extractTarXzViaTermux currently spawns a shell tar
process (vulnerable to path traversal, shell injection, and null PATH) — remove
this method and any unused TERMUX_BIN_PATH/related imports and instead call the
existing in-process extractor (reuse extractTar with an XZ decompressor, the
same protections used by extractZip and the ArchiveFormat.TAR_GZ handling).
Replace all usages of extractTarXzViaTermux with a path that invokes extractTar
(or the ArchiveFormat.TAR_GZ pattern) configured to handle XZ streams so
resolveEntryTarget/symlink checks remain in effect; also delete
kotlin.concurrent.thread and measureTimeMillis imports if they become unused.
🪄 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
Run ID: 935faed3-967e-4cbd-99dd-2582a8fffc7b
📒 Files selected for processing (1)
plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeArchiveServiceImpl.kt
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeArchiveServiceImpl.kt (2)
243-287:⚠️ Potential issue | 🔴 Critical
extractTarXzViaTermuxremains a security and portability liability.Re-reviewing the current form:
- The earlier "bash -c" shell-injection concern no longer applies — arguments are now passed individually via
ProcessBuilder, so metacharacters in paths are safe. Please disregard that specific point from the prior review.- Path traversal is still unprotected.
tar -xJfwill happily materialize../entries and symlink entries pointing outsideoutputDir; none of theresolveEntryTarget/ symlink rejection that guardsextractTar/extractZipapplies here. A hostile archive escapes the allow-listed destination.- Hard dependency on a Termux bin path.
TERMUX_BIN_PATH = "/data/data/com.itsaky.androidide/files/usr/bin"assumes Termux is installed under the IDE's data dir. On devices/builds without it, every TAR_XZ extraction fails.environment()["PATH"] = TERMUX_BIN_PATHfully overrides PATH (nonull:concatenation), so that prior concern is moot.- Line 244's
!archiveFile.exists()is dead defensive code — the temp file was just created on line 43.Removing this method and routing TAR_XZ through
extractTar(XZCompressorInputStream(buffered), …)eliminates the traversal risk, the Termux dependency, the subprocess, and the temp file in one step.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeArchiveServiceImpl.kt` around lines 243 - 287, The extractTarXzViaTermux function is a security and portability liability—remove it and instead handle TAR_XZ archives with the existing Java-based extraction pipeline that already enforces resolveEntryTarget/symlink checks (reuse extractTar or the code path used by extractZip), so TAR_XZ is read via XZCompressorInputStream -> TarArchiveInputStream (or equivalent) and passed into the same safe extraction routine; delete the TERMUX_BIN_PATH subprocess logic and the redundant archiveFile.exists() check, and ensure the code that used extractTarXzViaTermux now calls the TAR_XZ reader + existing safe extractor to preserve path-traversal and symlink protections.
42-52:⚠️ Potential issue | 🟠 MajorTAR_XZ branch bugs still unaddressed.
The four problems previously flagged on this block remain:
- Line 45 reads from raw
source, notbuffered.- No
ensureDirectory(destination)/ the temp path (destination.parentFile) is never run throughensurePathAllowed, so a destination that is itself allow-listed may have a parent the plugin cannot write to (and vice versa).- Returning
ExtractResult.Success(0, 0)silently dropsbytesWritten/filesExtracted— callers tracking progress will see zero for successful TAR_XZ extractions.- This branch bypasses the hardened extractor entirely (see follow-up on
extractTarXzViaTermux).The straightforward fix — reuse the in-process extractor — resolves all four at once:
🛠️ Suggested direction
- ArchiveFormat.TAR_XZ -> { - val tempFile = File.createTempFile("archive", ".tar.xz", destination.parentFile) - try { - tempFile.outputStream().use { source.copyTo(it) } - val ok = extractTarXzViaTermux(tempFile, destination) - if (ok) ExtractResult.Success(0, 0) - else ExtractResult.Failure(Exception("Termux tar extraction failed")) - } finally { - tempFile.delete() - } - } + ArchiveFormat.TAR_XZ -> extractTar( + XZCompressorInputStream(buffered), + destination, + onProgress + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeArchiveServiceImpl.kt` around lines 42 - 52, The TAR_XZ branch in ArchiveFormat.TAR_XZ currently copies from raw source, skips path checks for destination.parentFile, bypasses the hardened extractor, and returns Success(0,0); fix it by reusing the in-process/hardened extractor instead of calling extractTarXzViaTermux: pass a buffered stream (use the existing buffered variable) into the same extraction routine used for other formats so bytesWritten/filesExtracted are produced, ensure you run destination.parentFile through ensurePathAllowed and call ensureDirectory(destination) before writing the temp file, and continue to delete the tempFile in a finally block while returning the ExtractResult returned by the hardened extractor rather than Success(0,0).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeArchiveServiceImpl.kt`:
- Line 72: Replace the interpolated exception string in the error log so the
logger records the stack trace: in IdeArchiveServiceImpl where you currently
call logger.error("error in extract ${e}"), change it to call
logger.error("error in extract", e) (i.e., pass the Throwable e as the second
argument) so SLF4J can log the full exception and stack trace.
- Around line 143-159: extractZip currently lacks the symlink guard present in
extractTar; update the loop inside ZipArchiveInputStream usage in extractZip to
reject Unix symlink entries by checking entry.isUnixSymlink() (or equivalent)
immediately after the unreadable-entry check and before calling
resolveEntryTarget, throwing a SecurityException like "zip entry is a symlink,
refusing: ${entry.name}" to prevent TOCTOU symlink attacks; keep the existing
unresolved-entry and directory/file handling (resolveEntryTarget,
copyInterruptible, etc.) unchanged aside from this early rejection.
---
Duplicate comments:
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeArchiveServiceImpl.kt`:
- Around line 243-287: The extractTarXzViaTermux function is a security and
portability liability—remove it and instead handle TAR_XZ archives with the
existing Java-based extraction pipeline that already enforces
resolveEntryTarget/symlink checks (reuse extractTar or the code path used by
extractZip), so TAR_XZ is read via XZCompressorInputStream ->
TarArchiveInputStream (or equivalent) and passed into the same safe extraction
routine; delete the TERMUX_BIN_PATH subprocess logic and the redundant
archiveFile.exists() check, and ensure the code that used extractTarXzViaTermux
now calls the TAR_XZ reader + existing safe extractor to preserve path-traversal
and symlink protections.
- Around line 42-52: The TAR_XZ branch in ArchiveFormat.TAR_XZ currently copies
from raw source, skips path checks for destination.parentFile, bypasses the
hardened extractor, and returns Success(0,0); fix it by reusing the
in-process/hardened extractor instead of calling extractTarXzViaTermux: pass a
buffered stream (use the existing buffered variable) into the same extraction
routine used for other formats so bytesWritten/filesExtracted are produced,
ensure you run destination.parentFile through ensurePathAllowed and call
ensureDirectory(destination) before writing the temp file, and continue to
delete the tempFile in a finally block while returning the ExtractResult
returned by the hardened extractor rather than Success(0,0).
🪄 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
Run ID: 304360a1-705c-4125-8101-e092ea37665b
📒 Files selected for processing (1)
plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeArchiveServiceImpl.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Lets plugins install IDE-managed tooling (NDK, build-tools) without each bundling compression libraries.