(fix: plugin-manager): Restrict plugin installation to repos containing plugin.json at root level - #87
Open
jphinspace wants to merge 1 commit into
Open
(fix: plugin-manager): Restrict plugin installation to repos containing plugin.json at root level#87jphinspace wants to merge 1 commit into
jphinspace wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe installPlugin flow in plugin-manager.ts was changed to use a partial, checkout-less git clone followed by a git ls-tree check for plugin.json at the repo root. If missing, the target directory is removed and installation fails; if present, a git checkout retrieves the working tree. ChangesPlugin Install Verification
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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)
src/main/plugin-manager.ts (1)
146-150: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
ls-treename match doesn't confirmplugin.jsonis a file.
git ls-tree --name-only HEAD -- plugin.jsonmatches any tree entry with that name, including a directory. A repo with aplugin.json/folder at root would pass this gate despite having no actual manifest, defeating the intent of this fix. Prefer checking the entry type/mode explicitly.🔍 Proposed fix using full ls-tree output to verify blob type
- const out = await execFileAsync('git', ['ls-tree', '--name-only', 'HEAD', '--', 'plugin.json'], targetDir); - hasManifest = out.trim() === 'plugin.json'; + const out = await execFileAsync('git', ['ls-tree', 'HEAD', '--', 'plugin.json'], targetDir); + // Format: "<mode> blob <sha>\tplugin.json" — require a regular file (100644/100755). + const match = out.trim().match(/^(\d+)\s+blob\s+\S+\s+plugin\.json$/); + hasManifest = !!match && (match[1] === '100644' || match[1] === '100755');🤖 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 `@src/main/plugin-manager.ts` around lines 146 - 150, The manifest check in plugin-manager’s hasManifest logic only matches the name returned by git ls-tree, so it can incorrectly accept a plugin.json directory; update the git lookup in that block to verify the entry is a blob/file type explicitly rather than comparing only the name. Keep the existing try/catch flow, but parse the ls-tree result in a way that distinguishes files from tree entries so hasManifest is true only for an actual plugin.json file.
🤖 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 `@src/main/plugin-manager.ts`:
- Around line 152-155: The missing-plugin.json branch in `plugin-manager.ts` is
letting `fs.rmSync` throw and override the intended install failure message.
Update the `hasManifest` check in the plugin install flow so cleanup of
`targetDir` is best-effort and cannot replace the `"has no plugin.json"` result;
handle any removal error locally and still return the existing `{ success:
false, message: ... }` from the manifest validation path. Use the `hasManifest`
block and the outer install/catch flow in `plugin-manager.ts` to preserve the
original failure reason.
---
Nitpick comments:
In `@src/main/plugin-manager.ts`:
- Around line 146-150: The manifest check in plugin-manager’s hasManifest logic
only matches the name returned by git ls-tree, so it can incorrectly accept a
plugin.json directory; update the git lookup in that block to verify the entry
is a blob/file type explicitly rather than comparing only the name. Keep the
existing try/catch flow, but parse the ls-tree result in a way that
distinguishes files from tree entries so hasManifest is true only for an actual
plugin.json file.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 55e002bb-ec1a-4185-a3e2-0cfc8b5c452c
📒 Files selected for processing (1)
src/main/plugin-manager.ts
Comment on lines
+152
to
155
| if (!hasManifest) { | ||
| fs.rmSync(targetDir, { recursive: true }); | ||
| return { success: false, message: `"${name}" has no plugin.json at its root — not a valid plugin. Install aborted.` }; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unguarded rmSync can mask the real "missing plugin.json" failure.
If this cleanup fs.rmSync throws (e.g. transient lock/permission error), the exception propagates to the outer catch (Line 162-166), and the user sees "Failed to clone: <rmSync error>" instead of the accurate "has no plugin.json" message from Line 154.
🔧 Proposed fix to preserve the correct failure message
if (!hasManifest) {
- fs.rmSync(targetDir, { recursive: true });
+ try { fs.rmSync(targetDir, { recursive: true, force: true }); } catch { /* best-effort cleanup */ }
return { success: false, message: `"${name}" has no plugin.json at its root — not a valid plugin. Install aborted.` };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!hasManifest) { | |
| fs.rmSync(targetDir, { recursive: true }); | |
| return { success: false, message: `"${name}" has no plugin.json at its root — not a valid plugin. Install aborted.` }; | |
| } | |
| if (!hasManifest) { | |
| try { fs.rmSync(targetDir, { recursive: true, force: true }); } catch { /* best-effort cleanup */ } | |
| return { success: false, message: `"${name}" has no plugin.json at its root — not a valid plugin. Install aborted.` }; | |
| } |
🤖 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 `@src/main/plugin-manager.ts` around lines 152 - 155, The missing-plugin.json
branch in `plugin-manager.ts` is letting `fs.rmSync` throw and override the
intended install failure message. Update the `hasManifest` check in the plugin
install flow so cleanup of `targetDir` is best-effort and cannot replace the
`"has no plugin.json"` result; handle any removal error locally and still return
the existing `{ success: false, message: ... }` from the manifest validation
path. Use the `hasManifest` block and the outer install/catch flow in
`plugin-manager.ts` to preserve the original failure reason.
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
1 participant
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.
The existing behavior clones pretty much any repository link passed in, whether or not it has anything to do with feedBack. This change is to add a check that plugin.json exists in the repo before downloading any file contents. It also now aborts the installation if plugin.json is not present (previously it just logged a warning message).
Summary by CodeRabbit