ADFA-3159 Post-install audit for correctness of binary file unpacking and deploy#1062
Open
hal-eisen-adfa wants to merge 1 commit intostagefrom
Open
ADFA-3159 Post-install audit for correctness of binary file unpacking and deploy#1062hal-eisen-adfa wants to merge 1 commit intostagefrom
hal-eisen-adfa wants to merge 1 commit intostagefrom
Conversation
Collaborator
Author
|
I still need to come back and change how we handle split assets |
Contributor
📝 WalkthroughRelease NotesFeatures:
User-Facing Changes:
Code Quality Notes:
Risks & Best Practices Concerns:
WalkthroughA new post-installation audit system is introduced to validate binary asset integrity and existence. The audit runs after asset installation completes, checking expected entries for proper existence and size before the installation flow proceeds. The InstallationViewModel integrates this audit into its completion flow, handling both success and failure outcomes with appropriate messaging. Changes
Sequence DiagramsequenceDiagram
participant IVM as InstallationViewModel
participant AAA as AssetsInstallationAudit
participant FS as File System
participant DL as Distributions Loader
IVM->>AAA: run(context)
AAA->>FS: Check expected entries<br/>(files, dirs, AARs, etc.)
alt All entries valid
FS-->>AAA: Validation passes
AAA-->>IVM: Result.Success
IVM->>DL: Load distributions
DL-->>IVM: Installation complete
else Entry invalid
FS-->>AAA: Size mismatch or<br/>missing entry
AAA-->>IVM: Result.Failure(entryName, msg)
IVM-->>IVM: Emit error event<br/>Update state
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 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 `@app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationAudit.kt`:
- Around line 104-178: The current audit functions (checkDir, checkFile,
checkBootstrap, checkLlamaAar, checkPluginArtifacts) only validate
existence/size and can be fooled by partial or stale unpacks; replace or augment
size checks with a manifest+hash-based verification: have the installer produce
(or embed) a manifest of expected files with their relative paths and
cryptographic hashes and modify the audit to load that manifest and for each
entry verify presence, correct type, and that the file's hash matches the
manifest (fall back to size-only only if no manifest is available), and for
directories verify all required entries exist (or report missing/extra); update
checkFile to compute/compare a digest instead of relying solely on length,
update checkDir to iterate expected file entries from the manifest rather than
using recursiveSize(), and update
checkBootstrap/checkLlamaAar/checkPluginArtifacts to consult the same manifest
(or per-component manifests) so corrupted or stale same-size files fail the
audit.
In `@app/src/main/java/com/itsaky/androidide/viewmodel/InstallationViewModel.kt`:
- Around line 85-107: The audit is only executed after
AssetsInstallationHelper.install(); ensure AssetsInstallationAudit.run(context)
is invoked before any transition to InstallationComplete (not just
post-install). Find all code paths in InstallationViewModel (and places that
call or rely on checkToolsIsInstalled()) that set _state to InstallationComplete
and replace them with a short audit flow: call
AssetsInstallationAudit.run(context), handle Success by proceeding to load
distributions (IJdkDistributionProvider.getInstance().loadDistributions()) and
update state to InstallationComplete, and handle Failure by logging, emitting
InstallationEvent.ShowError, and setting InstallationError with the composed
error message (same handling as the existing failure branch). Ensure the new
audit run is placed before every assignment to InstallationComplete so
bootstrap/docs/Gradle/plugin assets are always validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 80abbbb1-3bf0-400d-b06d-5888ca7486d6
📒 Files selected for processing (3)
app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationAudit.ktapp/src/main/java/com/itsaky/androidide/viewmodel/InstallationViewModel.ktresources/src/main/res/values/strings.xml
Comment on lines
+104
to
+178
| private fun checkFile(file: File, entryName: String, expectedSize: Long): String? { | ||
| if (!file.exists()) return "File missing: ${file.absolutePath}" | ||
| if (!file.isFile) return "Not a file: ${file.absolutePath}" | ||
| if (expectedSize > 0) { | ||
| val minSize = (expectedSize * SIZE_TOLERANCE).toLong() | ||
| if (file.length() < minSize) { | ||
| return "File too small: ${file.absolutePath} (${file.length()} < $minSize)" | ||
| } | ||
| } else if (file.length() == 0L) { | ||
| return "File empty: ${file.absolutePath}" | ||
| } | ||
| return null | ||
| } | ||
|
|
||
| private fun checkDir(dir: File, entryName: String, expectedSize: Long): String? { | ||
| if (!dir.exists()) return "Directory missing: ${dir.absolutePath}" | ||
| if (!dir.isDirectory) return "Not a directory: ${dir.absolutePath}" | ||
| if (expectedSize > 0) { | ||
| val totalSize = dir.recursiveSize() | ||
| val minSize = (expectedSize * SIZE_TOLERANCE).toLong() | ||
| if (totalSize < minSize) { | ||
| return "Directory too small: ${dir.absolutePath} ($totalSize < $minSize)" | ||
| } | ||
| } else { | ||
| if (dir.recursiveSize() == 0L) return "Directory empty: ${dir.absolutePath}" | ||
| } | ||
| return null | ||
| } | ||
|
|
||
| private fun checkBootstrap(entryName: String, expectedSize: Long): String? { | ||
| val bash = Environment.BASH_SHELL | ||
| val login = Environment.LOGIN_SHELL | ||
| if (!bash.exists() || !bash.isFile || bash.length() == 0L) { | ||
| return "Bootstrap missing or empty: ${bash.absolutePath}" | ||
| } | ||
| if (!login.exists() || !login.isFile || login.length() == 0L) { | ||
| return "Bootstrap missing or empty: ${login.absolutePath}" | ||
| } | ||
| return null | ||
| } | ||
|
|
||
| private fun checkLlamaAar(context: Context, entryName: String, expectedSize: Long): String? { | ||
| val cpuArch = IDEBuildConfigProvider.getInstance().cpuArch | ||
| when (cpuArch) { | ||
| CpuArch.AARCH64, | ||
| CpuArch.ARM, | ||
| -> { | ||
| val destDir = context.getDir("dynamic_libs", Context.MODE_PRIVATE) | ||
| val llamaFile = File(destDir, "llama.aar") | ||
| return checkFile(llamaFile, entryName, expectedSize) | ||
| } | ||
| else -> { | ||
| // Unsupported arch: installer skips; audit passes without file | ||
| return null | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private fun checkPluginArtifacts(entryName: String, expectedSize: Long): String? { | ||
| val pluginDir = Environment.PLUGIN_API_JAR.parentFile ?: return "Plugin dir null" | ||
| if (!pluginDir.exists()) return "Plugin directory missing: ${pluginDir.absolutePath}" | ||
| if (!pluginDir.isDirectory) return "Not a directory: ${pluginDir.absolutePath}" | ||
| if (!Environment.PLUGIN_API_JAR.exists()) { | ||
| return "Plugin API jar missing: ${Environment.PLUGIN_API_JAR.absolutePath}" | ||
| } | ||
| if (expectedSize > 0) { | ||
| val totalSize = pluginDir.recursiveSize() | ||
| val minSize = (expectedSize * SIZE_TOLERANCE).toLong() | ||
| if (totalSize < minSize) { | ||
| return "Plugin dir too small: ${pluginDir.absolutePath} ($totalSize < $minSize)" | ||
| } | ||
| } else if (pluginDir.recursiveSize() == 0L) { | ||
| return "Plugin directory empty: ${pluginDir.absolutePath}" | ||
| } | ||
| return null |
Contributor
There was a problem hiding this comment.
These checks are too weak to prove the unpacked assets are actually correct.
checkDir() compares extracted trees against the input archive sizes returned by BundledAssetsInstaller.expectedSize() / SplitAssetsInstaller.expectedSize(), so a partially unpacked SDK/Gradle/Maven directory can still clear the 95% threshold. checkFile() is also size-only, and checkBootstrap() only requires two non-empty files, so same-size corruption or stale payloads still pass. If this audit is meant to gate setup completion, it needs hashes and/or a manifest of expected extracted outputs instead of archive byte counts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationAudit.kt`
around lines 104 - 178, The current audit functions (checkDir, checkFile,
checkBootstrap, checkLlamaAar, checkPluginArtifacts) only validate
existence/size and can be fooled by partial or stale unpacks; replace or augment
size checks with a manifest+hash-based verification: have the installer produce
(or embed) a manifest of expected files with their relative paths and
cryptographic hashes and modify the audit to load that manifest and for each
entry verify presence, correct type, and that the file's hash matches the
manifest (fall back to size-only only if no manifest is available), and for
directories verify all required entries exist (or report missing/extra); update
checkFile to compute/compare a digest instead of relying solely on length,
update checkDir to iterate expected file entries from the manifest rather than
using recursiveSize(), and update
checkBootstrap/checkLlamaAar/checkPluginArtifacts to consult the same manifest
(or per-component manifests) so corrupted or stale same-size files fail the
audit.
Comment on lines
+85
to
+107
| _installationProgress.value = | ||
| context.getString(R.string.verifying_installation) | ||
| val auditResult = AssetsInstallationAudit.run(context) | ||
| when (auditResult) { | ||
| is AssetsInstallationAudit.Result.Success -> { | ||
| val distributionProvider = IJdkDistributionProvider.getInstance() | ||
| distributionProvider.loadDistributions() | ||
| _state.update { InstallationComplete } | ||
| } | ||
| is AssetsInstallationAudit.Result.Failure -> { | ||
| val errorMsg = | ||
| context.getString( | ||
| R.string.asset_verification_failed, | ||
| auditResult.entryName, | ||
| auditResult.message, | ||
| ) | ||
| log.warn("Post-installation audit failed: {}", errorMsg) | ||
| viewModelScope.launch { | ||
| _events.emit(InstallationEvent.ShowError(errorMsg)) | ||
| } | ||
| _state.update { InstallationError(errorMsg) } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
This only enforces the audit on fresh installs.
The new verification runs only after AssetsInstallationHelper.install() succeeds. Any setup path where checkToolsIsInstalled() is already true still bypasses validation for bootstrap, docs DB, Gradle API jars, plugin artifacts, etc., and can go straight to InstallationComplete. Please run AssetsInstallationAudit before every transition to InstallationComplete, not just after a new install.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/itsaky/androidide/viewmodel/InstallationViewModel.kt`
around lines 85 - 107, The audit is only executed after
AssetsInstallationHelper.install(); ensure AssetsInstallationAudit.run(context)
is invoked before any transition to InstallationComplete (not just
post-install). Find all code paths in InstallationViewModel (and places that
call or rely on checkToolsIsInstalled()) that set _state to InstallationComplete
and replace them with a short audit flow: call
AssetsInstallationAudit.run(context), handle Success by proceeding to load
distributions (IJdkDistributionProvider.getInstance().loadDistributions()) and
update state to InstallationComplete, and handle Failure by logging, emitting
InstallationEvent.ShowError, and setting InstallationError with the composed
error message (same handling as the existing failure branch). Ensure the new
audit run is placed before every assignment to InstallationComplete so
bootstrap/docs/Gradle/plugin assets are always validated.
itsaky-adfa
reviewed
Mar 11, 2026
Comment on lines
+71
to
+74
| if (failure != null) { | ||
| logger.warn("Audit failed for '{}': {}", entryName, failure) | ||
| return Result.Failure(entryName, failure) | ||
| } |
Contributor
There was a problem hiding this comment.
Suggestion: we can accumulate audit results for all entries instead of the fast-fail approach here. So, when debugging, we can know which entries are corrupted/unexpected at once, instead of making the audit fail for each entry sequentially.
Comment on lines
+104
to
+131
| private fun checkFile(file: File, entryName: String, expectedSize: Long): String? { | ||
| if (!file.exists()) return "File missing: ${file.absolutePath}" | ||
| if (!file.isFile) return "Not a file: ${file.absolutePath}" | ||
| if (expectedSize > 0) { | ||
| val minSize = (expectedSize * SIZE_TOLERANCE).toLong() | ||
| if (file.length() < minSize) { | ||
| return "File too small: ${file.absolutePath} (${file.length()} < $minSize)" | ||
| } | ||
| } else if (file.length() == 0L) { | ||
| return "File empty: ${file.absolutePath}" | ||
| } | ||
| return null | ||
| } | ||
|
|
||
| private fun checkDir(dir: File, entryName: String, expectedSize: Long): String? { | ||
| if (!dir.exists()) return "Directory missing: ${dir.absolutePath}" | ||
| if (!dir.isDirectory) return "Not a directory: ${dir.absolutePath}" | ||
| if (expectedSize > 0) { | ||
| val totalSize = dir.recursiveSize() | ||
| val minSize = (expectedSize * SIZE_TOLERANCE).toLong() | ||
| if (totalSize < minSize) { | ||
| return "Directory too small: ${dir.absolutePath} ($totalSize < $minSize)" | ||
| } | ||
| } else { | ||
| if (dir.recursiveSize() == 0L) return "Directory empty: ${dir.absolutePath}" | ||
| } | ||
| return null | ||
| } |
Contributor
There was a problem hiding this comment.
Suggestion: a more robust approach would be to compare SHA-256/512 checksums of the files. The checksums would be computed at compile time (of the IDE), then verify at runtime.
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.
Adds a new AssetsInstallationAudit.kt file and calls it from InstallationViewModel.kt
Enforces audit of file size and SHA256 (where possible) on binary files during onboarding and setup