ADFA-3695: Implement plugin conflict detection with signature verification and overwrite confirmation#1200
Merged
Daniel-ADFA merged 4 commits intostagefrom Apr 21, 2026
Merged
ADFA-3695: Implement plugin conflict detection with signature verification and overwrite confirmation#1200Daniel-ADFA merged 4 commits intostagefrom
Daniel-ADFA merged 4 commits intostagefrom
Conversation
…ation and overwrite confirmation
Contributor
📝 WalkthroughRelease Notes: Plugin Conflict Detection with Signature VerificationFeatures Added
Technical Changes
Risk Factors & Best Practice Violations
Breaking Changes
Testing Recommendations
WalkthroughAdds an overwrite-confirmation flow for plugin installs: incoming plugin metadata and signatures are read from the file; if an installed plugin with the same ID exists, signatures are compared and either an error is emitted for mismatch or a Replace/Cancel dialog is shown for matching signatures. Changes
Sequence DiagramsequenceDiagram
participant User
participant Activity as PluginManagerActivity
participant ViewModel as PluginManagerViewModel
participant Repo as PluginRepository
participant Manager as PluginManager
User->>Activity: Request install (uri)
Activity->>ViewModel: OnEvent(InstallPlugin(uri))
ViewModel->>Repo: download URI -> tempFile
ViewModel->>Repo: getPluginMetadataFromFile(tempFile)
Repo->>Manager: getPluginMetadataOnly(tempFile)
Manager-->>Repo: PluginMetadata
Repo-->>ViewModel: Result<PluginMetadata>
alt Installed plugin with same ID exists
ViewModel->>Repo: haveMatchingSignatures(tempFile, existingId)
Repo->>Manager: haveMatchingSignatures(...)
Manager-->>Repo: Boolean
Repo-->>ViewModel: Result<Boolean>
alt Signatures mismatch
ViewModel-->>Activity: ShowError(msg_plugin_signature_mismatch)
Activity->>User: Display error
else Signatures match
ViewModel-->>Activity: ShowOverwriteConfirmation(existing, incoming)
Activity->>User: Show Replace/Cancel dialog
User->>Activity: Tap Replace
Activity->>ViewModel: OnEvent(ConfirmOverwrite(uri, deleteAfter))
ViewModel->>Repo: installPluginFromFile(tempFile, checkConflict=false)
Repo->>Manager: installPlugin(...)
Manager-->>Repo: Success
Repo-->>ViewModel: Result<Unit>
ViewModel-->>Activity: ShowSuccess
Activity->>User: Display success
end
else No existing plugin
ViewModel->>Repo: installPluginFromFile(tempFile)
Repo->>Manager: installPlugin(...)
Manager-->>Repo: Success
Repo-->>ViewModel: Result<Unit>
ViewModel-->>Activity: ShowSuccess
Activity->>User: Display success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
app/src/main/java/com/itsaky/androidide/activities/PluginManagerActivity.kt (1)
208-225: Copy action only exposed for parameterized errors — verify this matches intent.The
Copyaction and indefinite duration are only attached wheneffect.formatArgs.isNotEmpty(). Several error strings used in this flow (e.g.msg_plugin_file_not_found,msg_plugin_signature_mismatchdoes take args,msg_plugin_install_faileddoes too) have varying parameter counts, so "has formatArgs" is being used as a proxy for "is a long/detailed error, allow copy and hold open indefinitely." That coupling is implicit and fragile — a future error with no args could still benefit from Copy, and a short arg-based error will now never auto-dismiss. Consider making this an explicit flag onShowError(e.g.isCopyable: BooleanorautoDismiss: Boolean) rather than deriving it from argument presence.🤖 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/activities/PluginManagerActivity.kt` around lines 208 - 225, The current logic in handleUiEffect uses effect.formatArgs.isNotEmpty() to decide both the flashbar duration and whether to show the Copy action, which couples formatting arguments to UI behavior; update the ShowError model (PluginManagerUiEffect.ShowError) to include explicit flags (e.g. isCopyable: Boolean and/or autoDismiss: Boolean), then change handleUiEffect to use those flags instead of effect.formatArgs.isNotEmpty() when configuring builder (duration, positiveActionText, positiveActionTapListener and builder.showOnUiThread), leaving formatArgs only for string formatting.
🤖 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/viewmodels/PluginManagerViewModel.kt`:
- Around line 264-266: The code silently skips conflict/signature checks when
getPluginMetadataFromFile(tempFile).getOrNull() returns null; update
PluginManagerViewModel so that if incoming is null you log a warning (include
tempFile and parsing error if available) and abort with an explicit user-facing
error (or set a UI state error) instead of proceeding to installPluginFromFile;
if you must continue, at minimum record in _uiState why conflict detection was
skipped and ensure existing overwrite/signature guards are not bypassed. Use the
same call sites around incoming/existing to add the logging and error-path
handling so malformed archives cannot circumvent overwrite checks.
- Around line 268-291: The signature check currently treats verification errors
as "match" because haveMatchingSignatures(...).getOrDefault(true) defaults to
true; change this to fail closed by treating any non-success as mismatch (use
getOrDefault(false) or explicitly handle the Result/Optional error case) inside
PluginManagerViewModel where signaturesMatch is computed; ensure tempFile is
still deleted, and replace the branch so that verification failures route to
_uiEffect.trySend(PluginManagerUiEffect.ShowError(...)) (or a distinct "could
not verify signature" error) instead of showing
PluginManagerUiEffect.ShowOverwriteConfirmation for unverified plugins.
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManager.kt`:
- Around line 525-531: The haveMatchingSignatures method currently treats null
signature extraction as a match; change this to fail-closed by treating any null
incomingSig or existingSig as a mismatch (return false or surface an error) so a
missing signature from PluginLoader.getSignatureHash does not allow overwrite;
update the logic in haveMatchingSignatures (and add a clear processLogger/error
path if desired) to return false when either signature is null and only return
contentEquals(...) when both are non-null.
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/loaders/PluginLoader.kt`:
- Around line 111-113: The catch in PluginLoader.loadPluginClasses currently
throws a RuntimeException which makes the declared nullable return type
DexClassLoader? and the null-check in PluginManager.loadPlugin dead code; either
make the return non-nullable and remove the caller's null-check, or preserve the
existing contract by logging the exception and returning null instead of
throwing. Update PluginLoader.loadPluginClasses to catch Exception (e), use the
module logger/processLogger to log the full error (including e and e.message)
with context "Failed to load plugin classes", and then return null so
PluginManager.loadPlugin's if (classLoader == null) branch remains reachable and
correct.
- Around line 251-267: PluginLoader.getSignatureHash currently swallows all
exceptions and returns null, which lets PluginManager.haveMatchingSignatures
treat failures as a match; change getSignatureHash to not fail silently: catch
only expected exceptions if needed, log the exception (use the existing logger)
with context (pluginApk path) and then rethrow or propagate the exception (do
not return null on unexpected errors) so the caller can fail closed; update
caller PluginManager.haveMatchingSignatures to handle the propagated exception
and treat extraction failures as non-matching (or surface the error) instead of
treating null as a match.
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/loaders/PluginManifest.kt`:
- Around line 110-121: The parseFromString/parseFromJar methods
(PluginManifestParser.parseFromString / parseFromJar) currently declare
PluginManifest? but rethrow exceptions, breaking the fallback in
PluginLoader.getPluginMetadata which expects null-on-failure; restore the
original null-on-failure contract by catching Exception inside
parseFromString/parseFromJar, log the error (include exception details) and
return null on parse errors, or alternatively change both method signatures to
non-null and update all callers (notably PluginLoader.getPluginMetadata and any
getPluginMetadataOnly flow that calls PluginManifestParser.parseFromJar) to
handle the thrown exception — pick one consistent approach and apply it across
the referenced symbols.
---
Nitpick comments:
In `@app/src/main/java/com/itsaky/androidide/activities/PluginManagerActivity.kt`:
- Around line 208-225: The current logic in handleUiEffect uses
effect.formatArgs.isNotEmpty() to decide both the flashbar duration and whether
to show the Copy action, which couples formatting arguments to UI behavior;
update the ShowError model (PluginManagerUiEffect.ShowError) to include explicit
flags (e.g. isCopyable: Boolean and/or autoDismiss: Boolean), then change
handleUiEffect to use those flags instead of effect.formatArgs.isNotEmpty() when
configuring builder (duration, positiveActionText, positiveActionTapListener and
builder.showOnUiThread), leaving formatArgs only for string formatting.
🪄 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: 725cbf15-e1f5-4427-91bc-25de5d322909
📒 Files selected for processing (9)
app/src/main/java/com/itsaky/androidide/activities/PluginManagerActivity.ktapp/src/main/java/com/itsaky/androidide/repositories/PluginRepository.ktapp/src/main/java/com/itsaky/androidide/repositories/PluginRepositoryImpl.ktapp/src/main/java/com/itsaky/androidide/ui/models/PluginManagerUiState.ktapp/src/main/java/com/itsaky/androidide/viewmodels/PluginManagerViewModel.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManager.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/loaders/PluginLoader.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/loaders/PluginManifest.ktresources/src/main/res/values/strings.xml
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/viewmodels/PluginManagerViewModel.kt`:
- Line 276: The conflict check currently reads from the cached UI list via
_uiState.value.plugins (val existing = _uiState.value.plugins.find {
it.metadata.id == incoming.id }), which can be stale; replace that lookup with a
direct repository call (e.g. call pluginRepository.getPluginById(incoming.id) or
equivalent) to fetch the canonical on-disk/plugin store entry before deciding
the conflict/signature branch so the overwrite prompt and signature verification
run reliably even while loadPlugins() is still populating the UI state.
- Around line 263-308: The TOCTOU bug is that the verified tempFile is deleted
before the overwrite confirmation, so installPluginFromFile may later install a
new, unverified copy; to fix, persist and install the exact verified file:
change the overwrite flow to pass the verified temp file reference (e.g., its
path or a FileDescriptor) through
PluginManagerUiEffect.ShowOverwriteConfirmation and the ConfirmOverwrite action
instead of the original Uri, keep tempFile alive until the user confirms, and
have the confirm branch call pluginRepository.installPluginFromFile(using that
same tempFile) rather than re-copying the Uri; alternatively, if you prefer the
simpler change, re-run pluginRepository.haveMatchingSignatures(tempFile,
existing.metadata.id) immediately before calling installPluginFromFile in the
confirm branch to ensure the second copy is verified.
🪄 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: 69ef76d1-9aaf-4900-be8e-0b17d6e950d3
📒 Files selected for processing (5)
app/src/main/java/com/itsaky/androidide/viewmodels/PluginManagerViewModel.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManager.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/loaders/PluginLoader.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/loaders/PluginManifest.ktresources/src/main/res/values/strings.xml
✅ Files skipped from review due to trivial changes (2)
- resources/src/main/res/values/strings.xml
- plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/loaders/PluginManifest.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/loaders/PluginLoader.kt
- plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManager.kt
jatezzz
requested changes
Apr 20, 2026
jatezzz
approved these changes
Apr 20, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/core/PluginManager.kt`:
- Around line 553-556: haveMatchingSignatures currently assumes the installed
plugin file is named "$existingPluginId.cgp", which can miss plugins that keep
their original filename; update haveMatchingSignatures to locate the installed
.cgp by scanning pluginsDir for files ending with ".cgp", loading each file's
manifest/signature using PluginLoader (same class used for incomingFile) and
selecting the file whose manifest ID equals existingPluginId, then compare
signature hashes between the incomingFile and that resolved file; ensure you use
PluginLoader(context, file).getSignatureHash() and/or manifest ID access
consistently so existingSig is not null when a matching manifest exists.
🪄 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: a26a6281-7b26-46d1-8a64-af02d3053832
📒 Files selected for processing (7)
app/src/main/java/com/itsaky/androidide/activities/PluginManagerActivity.ktapp/src/main/java/com/itsaky/androidide/repositories/PluginRepositoryImpl.ktapp/src/main/java/com/itsaky/androidide/viewmodels/PluginManagerViewModel.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManager.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/loaders/PluginLoader.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/loaders/PluginManifest.ktresources/src/main/res/values/strings.xml
✅ Files skipped from review due to trivial changes (1)
- plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/loaders/PluginManifest.kt
🚧 Files skipped from review as they are similar to previous changes (4)
- app/src/main/java/com/itsaky/androidide/activities/PluginManagerActivity.kt
- app/src/main/java/com/itsaky/androidide/repositories/PluginRepositoryImpl.kt
- plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/loaders/PluginLoader.kt
- app/src/main/java/com/itsaky/androidide/viewmodels/PluginManagerViewModel.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.
…