Install GitHub plugins when Git is unavailable - #127
Conversation
📝 WalkthroughWalkthroughPlugin management now supports GitHub archive installation and updates when Git is unavailable. Stored archive metadata determines update eligibility, and the renderer uses the new ChangesGitHub archive plugin flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PluginManager
participant GitHub
participant PluginDirectory
PluginManager->>GitHub: Download repository archive
GitHub-->>PluginManager: Return tarball
PluginManager->>PluginDirectory: Extract plugin and write source metadata
PluginManager->>PluginDirectory: Stage archive and swap plugin directories
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/plugin-manager.ts (1)
255-262: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not report a successful swap as a failed update.
If deletion of
backupDirfails afterstagedDirwas promoted, the outer catch returns failure even though the live plugin was already replaced. Handle backup cleanup separately (log and retry/defer it) so the operation reports its actual outcome.Proposed fix
- fs.rmSync(backupDir, { recursive: true, force: true }); + try { + fs.rmSync(backupDir, { recursive: true, force: true }); + } catch (cleanupError) { + console.warn(`[plugins] Could not remove backup for "${name}"`, cleanupError); + }🤖 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 255 - 262, In the plugin update flow surrounding the stagedDir promotion and backupDir cleanup, prevent errors deleting backupDir from reaching the outer catch after the live plugin has already been replaced. Handle backup cleanup independently by logging the failure and retrying or deferring removal, while preserving failure reporting for errors that occur before or during the swap so the returned success status reflects the actual update outcome.
🤖 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 85-87: Update installGitHubArchive() to stream the GitHub response
directly into a temporary file instead of buffering response.arrayBuffer() in
memory. Enforce a configurable maximum byte limit while streaming, and use an
AbortSignal with a timeout to terminate stalled downloads; preserve the existing
HTTP status validation and archive installation flow.
In `@src/main/plugin-source.ts`:
- Around line 3-10: The parseGitHubRepository function must reject GitHub URLs
containing credentials before accepting the repository path. Validate that
url.username and url.password are both empty, return null otherwise, and add a
regression test covering a credential-bearing URL to ensure it is not persisted
or accepted.
---
Outside diff comments:
In `@src/main/plugin-manager.ts`:
- Around line 255-262: In the plugin update flow surrounding the stagedDir
promotion and backupDir cleanup, prevent errors deleting backupDir from reaching
the outer catch after the live plugin has already been replaced. Handle backup
cleanup independently by logging the failure and retrying or deferring removal,
while preserving failure reporting for errors that occur before or during the
swap so the returned success status reflects the actual update outcome.
🪄 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: 1c0a5540-07dd-4fd3-9842-1f80db008b6f
📒 Files selected for processing (4)
src/main/plugin-manager.tssrc/main/plugin-source.tssrc/renderer/plugin-manager/screen.jstests/plugin-source.test.js
| const response = await fetch(`https://codeload.github.com/${repository.owner}/${repository.repo}/tar.gz/HEAD`); | ||
| if (!response.ok) throw new Error(`GitHub download returned HTTP ${response.status}`); | ||
| fs.writeFileSync(archivePath, Buffer.from(await response.arrayBuffer())); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant plugin-manager implementation around the reported lines,
# plus nearby timeout/limit behavior if present.
if [ -f src/main/plugin-manager.ts ]; then
printf '--- plugin-manager.ts outline ---\n'
ast-grep outline src/main/plugin-manager.ts || true
printf '\n--- lines 60-110 ---\n'
sed -n '60,110p' src/main/plugin-manager.ts | nl -ba -v60
printf '\n--- all fetch/timeout/limit references ---\n'
rg -n "fetch\\(|arrayBuffer|timeout|AbortController|writeFile|Response|Readable|createWriteStream|max|size|limit" src/main/plugin-manager.ts || true
else
printf 'src/main/plugin-manager.ts not found\n'
git ls-files | rg 'plugin-manager\.ts$' || true
fiRepository: got-feedBack/feedBack-desktop
Length of output: 258
🏁 Script executed:
#!/bin/bash
set -euo pipefail
if [ -f src/main/plugin-manager.ts ]; then
echo '--- plugin-manager.ts outline ---'
ast-grep outline src/main/plugin-manager.ts || true
echo
echo '--- lines 60-110 ---'
sed -n '60,110p' src/main/plugin-manager.ts | nl -ba -v60
echo
echo '--- fetch/timeout/limit references ---'
rg -n "fetch\\(|arrayBuffer|timeout|AbortController|writeFile|Response|Readable|createWriteStream|max|size|limit" src/main/plugin-manager.ts || true
else
echo 'src/main/plugin-manager.ts not found'
git ls-files | rg 'plugin-manager\.ts$' || true
fiRepository: got-feedBack/feedBack-desktop
Length of output: 1262
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- lines 78-95 ---'
sed -n '78,95p' src/main/plugin-manager.ts | awk '{print NR+77": "$0}'
echo
echo '--- fetch/timeout/limit references ---'
rg -n "fetch\\(|arrayBuffer|timeout|AbortController|writeFile|Response|Readable|createWriteStream|max|size|limit|archivePath" src/main/plugin-manager.ts || true
echo
echo '--- package runtime hints ---'
grep -n '"type":' package.json 2>/dev/null || true
sed -n '1,120p' src/main/plugin-manager.ts | awk '{print NR": "$0}'Repository: got-feedBack/feedBack-desktop
Length of output: 7702
🌐 Web query:
Node.js global fetch default timeout no abort controller arrayBuffer buffers entire response
💡 Result:
In Node.js, the global fetch API—powered by undici—has a default timeout of 300 seconds [1][2][3]. This applies to both headers and the body, and is not a per-request configurable timeout in the traditional sense, but rather a default configuration within the undici dispatcher [1][2]. There is no global setting to change this default; to enforce a shorter timeout, the standard and recommended approach is to use AbortSignal.timeout passed within the fetch options [4][5][3]. Regarding arrayBuffer, calling response.arrayBuffer does indeed buffer the entire response body into memory [6]. The method is designed to read the response stream to completion and resolve with an ArrayBuffer containing the full data [6]. Because this process consumes the entire body into memory, it is not suitable for large payloads where memory efficiency is a concern; for large data, using the response body as a stream is the recommended approach [7][8].
Citations:
- 1: Default fetch timeout too short nodejs/undici#1373
- 2: How to set a timeout for fetch? vercel/next.js#72443
- 3: https://betterstack.com/community/guides/scaling-nodejs/nodejs-timeouts/
- 4: https://github.com/node-fetch/node-fetch/blob/HEAD/docs/v3-UPGRADE-GUIDE.md
- 5: https://stackoverflow.com/questions/46946380/fetch-api-request-timeout
- 6: https://developer.mozilla.org/en-US/docs/Web/API/Response/arrayBuffer
- 7: https://www.npmjs.com/package/node-fetch?activeTab=readme
- 8: Use case for res.arrayBuffer node-fetch/node-fetch#51
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- updatePlugin installGitHubArchive call sites ---'
sed -n '220,265p' src/main/plugin-manager.ts | awk '{print NR+220": "$0}'
echo
echo '--- installPlugin call site ---'
sed -n '152,200p' src/main/plugin-manager.ts | awk '{print NR+152": "$0}'Repository: got-feedBack/feedBack-desktop
Length of output: 4792
Bound archive downloads.
installGitHubArchive() uses response.arrayBuffer() without any size limit and does not pass an AbortSignal, so stalled GitHub downloads can hang and large archives can block memory before being written to disk. Stream the archive to a temp file with a configurable byte limit and an abort timeout.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 86-86: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(archivePath, Buffer.from(await response.arrayBuffer()))
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 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 85 - 87, Update
installGitHubArchive() to stream the GitHub response directly into a temporary
file instead of buffering response.arrayBuffer() in memory. Enforce a
configurable maximum byte limit while streaming, and use an AbortSignal with a
timeout to terminate stalled downloads; preserve the existing HTTP status
validation and archive installation flow.
| export function parseGitHubRepository(value: string): { owner: string; repo: string } | null { | ||
| try { | ||
| const url = new URL(value); | ||
| const parts = url.pathname.replace(/^\/+|\/+$/g, '').split('/'); | ||
| if (url.protocol !== 'https:' || url.hostname.toLowerCase() !== 'github.com' || parts.length !== 2) return null; | ||
| const owner = parts[0]; | ||
| const repo = parts[1].replace(/\.git$/, ''); | ||
| if (!/^[A-Za-z0-9_.-]+$/.test(owner) || !/^[A-Za-z0-9_.-]+$/.test(repo)) return null; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject URLs containing credentials.
URL accepts https://token@github.com/owner/repo; the archive flow then writes that raw URL to .feedback-plugin-source.json, persisting a token/password in plaintext. Reject non-empty url.username or url.password, and add a regression test.
Proposed fix
- if (url.protocol !== 'https:' || url.hostname.toLowerCase() !== 'github.com' || parts.length !== 2) return null;
+ if (url.protocol !== 'https:' || url.hostname.toLowerCase() !== 'github.com' ||
+ url.username || url.password || parts.length !== 2) return null;📝 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.
| export function parseGitHubRepository(value: string): { owner: string; repo: string } | null { | |
| try { | |
| const url = new URL(value); | |
| const parts = url.pathname.replace(/^\/+|\/+$/g, '').split('/'); | |
| if (url.protocol !== 'https:' || url.hostname.toLowerCase() !== 'github.com' || parts.length !== 2) return null; | |
| const owner = parts[0]; | |
| const repo = parts[1].replace(/\.git$/, ''); | |
| if (!/^[A-Za-z0-9_.-]+$/.test(owner) || !/^[A-Za-z0-9_.-]+$/.test(repo)) return null; | |
| export function parseGitHubRepository(value: string): { owner: string; repo: string } | null { | |
| try { | |
| const url = new URL(value); | |
| const parts = url.pathname.replace(/^\/+|\/+$/g, '').split('/'); | |
| if (url.protocol !== 'https:' || url.hostname.toLowerCase() !== 'github.com' || | |
| url.username || url.password || parts.length !== 2) return null; | |
| const owner = parts[0]; | |
| const repo = parts[1].replace(/\.git$/, ''); | |
| if (!/^[A-Za-z0-9_.-]+$/.test(owner) || !/^[A-Za-z0-9_.-]+$/.test(repo)) return null; |
🤖 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-source.ts` around lines 3 - 10, The parseGitHubRepository
function must reject GitHub URLs containing credentials before accepting the
repository path. Validate that url.username and url.password are both empty,
return null otherwise, and add a regression test covering a credential-bearing
URL to ensure it is not persisted or accepted.
1 participant
Summary
Verification
npm run typechecknode --test tests/plugin-source.test.jsbalki97/feedforge-connectwith the same GitHub codeload/tar path on WindowsFull
npm testreached 100 passing tests; two unrelated environment checks fail because this shallow inspection clone has no installed Electron binary or JUCE submodule.Summary by CodeRabbit
New Features
Bug Fixes