fix(plugins): disable auto-running npm run build and stage plugin updates (P0) - #1117
fix(plugins): disable auto-running npm run build and stage plugin updates (P0)#1117wjc2821296948 wants to merge 5 commits into
Conversation
`installPluginFromGit` and `updatePluginFromGit` cloned a remote Git repository and ran `npm run build` whenever the package.json declared a build script. Build scripts execute arbitrary code with the server process's privileges, so any party able to supply a plugin URL (e.g. an authenticated user tricked into pasting a malicious URL, or a compromised auth token) gained remote code execution on the CloudCLI host. The build script is now opt-in: the caller must pass `allowBuild: true` to the install/update service after manually inspecting the build command. The HTTP `POST /api/plugins/install` and `POST /api/plugins/<name>/update` endpoints accept an explicit `allowBuild: true` in the JSON body for that purpose. A process-wide escape hatch (`setAllowPluginBuildScript`) is exposed for tests. Co-authored-by: cgsdn <chaogeshuodiannao@users.noreply.github.com>
…ve plugin untouched `updatePluginFromGit` performed `git pull --ff-only` directly against the live plugin directory. After my previous commit made `runBuildIfNeeded` reject updates whose `package.json` declares a build script without `allowBuild: true`, that rejection now happened *after* the pull had already mutated the live directory (and after `npm install --ignore-scripts` had already rewritten `node_modules`). The caller (`plugins.service.ts update()`) had also already stopped the running plugin server before invoking the registry. A rejected update therefore left the operator with both a half-updated plugin directory and a stopped plugin server. Switch the registry to the same staging pattern `installPluginFromGit` already uses: re-clone the plugin's remote URL into a sibling temp directory, validate the manifest, run `npm install`, apply the build policy, and only then atomically rename the temp directory over the live one. A rejection at any step cleans up the temp directory and the live plugin directory is never touched. Update the service to restart the previously running plugin server when the update is rejected — the live directory is unchanged, so a clean restart restores the previous plugin state. Cover the new contract with a service test that verifies a rejected update stops, attempts the update, and then restarts the previously running server. Co-authored-by: cgsdn <chaogeshuodiannao@users.noreply.github.com>
… fails `updatePluginFromGit`'s `finalize` step performed `fs.rmSync(pluginDir)` followed by `fs.renameSync(tempDir, pluginDir)`. If the rename failed (partition full, permissions race, Windows AV scanner holding a file handle, etc.) the temp directory was cleaned up by the catch block but the previous plugin directory was already gone. The previous plugin was lost and `plugins.service.ts update()` could not load the previous manifest during server recovery. Switch to a backup-restore pattern: rename the live directory to a sibling backup, rename the temp directory into place, then delete the backup. If the second rename fails, restore the backup to the live directory and clean up the temp directory. The previous plugin is always recoverable from either the live path or the backup. Add a small smoke test for the registry's URL pre-checks (full swap-failure coverage would require an fs mock framework that this codebase does not currently depend on). Co-authored-by: cgsdn <chaogeshuodiannao@users.noreply.github.com>
`installPluginFromGit` previously validated only that the URL was a non-empty string that did not start with `-`. Any other shape (including `file://` and `http://`) passed through to `git clone`, and the registry's `repoName` regex happened to accept paths like `/tmp/local`. The HTTP route layer already enforces `https://`/`git@` upstream, but the registry should also enforce the scheme so any internal caller (tests, future programmatic install paths) cannot bypass the check. Reject URLs that do not start with `https://` or `git@` with a clear `Invalid URL` error before any disk work. Co-authored-by: cgsdn <chaogeshuodiannao@users.noreply.github.com>
Severity summaryP0 — Plugin install and update execute This PR is split out from the previous umbrella PR (#1106) per @blackmammoth's request that each finding be reviewed in isolation. 🤖 Generated with Claude Code |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughPlugin installation and updates now validate Git remotes, disable build scripts by default, support explicit build authorization, stage updates in temporary directories, and restore previous plugin state after failures. ChangesPlugin safety and lifecycle
Sequence Diagram(s)sequenceDiagram
participant PluginRoutes
participant PluginsService
participant PluginRegistry
participant TemporaryDirectory
participant LivePlugin
PluginRoutes->>PluginsService: pass allowBuild
PluginsService->>PluginRegistry: install or update plugin
PluginRegistry->>TemporaryDirectory: validate, clone, and prepare plugin
PluginRegistry->>LivePlugin: atomically replace plugin
PluginsService->>LivePlugin: restart server after failed update
Possibly related PRs
Poem Poem
🚥 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: 3
🧹 Nitpick comments (3)
server/modules/plugins/tests/plugins.service.test.ts (1)
34-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert that
update()forwards its options.The test verifies the restart order correctly. It calls
service.update('demo')without options, so the newallowBuildplumbing stays uncovered at this layer. Capture the second argument in theupdatestub and assert it equals the options passed in.let receivedOptions: unknown; // ... update: async (_name, options) => { receivedOptions = options; return { name: 'demo', dirName: 'demo' }; }, // ... await service.update('demo', { allowBuild: true }); assert.deepEqual(receivedOptions, { allowBuild: true });🤖 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 `@server/modules/plugins/tests/plugins.service.test.ts` around lines 34 - 57, Update the `update()` restart test to pass `{ allowBuild: true }` to `service.update('demo', ...)`, capture the options argument in the mocked `update` dependency, and assert it matches the provided options while preserving the existing rejection and restart-order assertions.server/modules/plugins/tests/plugin-registry.service.test.ts (1)
6-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the build-policy decision.
The three tests cover URL pre-checks only. The core change of this PR is that a plugin declaring a
buildscript now fails unless the caller opts in. No test asserts that behavior, and no test asserts thatsetAllowPluginBuildScript(true)re-enables it.
runBuildIfNeededis not exported, so a direct unit test needs either an export or a local fixture repository. Do you want me to add a test that exportsrunBuildIfNeededfor testing and covers the three branches: no build script, build script without opt-in, build script withallowBuild: true?🤖 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 `@server/modules/plugins/tests/plugin-registry.service.test.ts` around lines 6 - 30, Add tests covering the build-policy branches in installPluginFromGit or the relevant registry flow: allow plugins without a build script, reject plugins declaring a build script unless opt-in is enabled, and permit the build when setAllowPluginBuildScript(true) or allowBuild: true is applied. Use an existing local fixture repository if possible; otherwise expose runBuildIfNeeded only as needed for focused testing.server/modules/plugins/plugin-registry.service.ts (1)
100-135: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider narrowing the process-wide build override.
ALLOW_PLUGIN_BUILD_SCRIPTis a mutable module global. Once any caller sets it totrue, every later install and update can run build scripts, including requests that did not opt in. The per-requestallowBuildflag then no longer controls the behavior.If the override exists only for tests and vetted callers, gate it on an environment variable read once at startup, or keep the setter but document that it disables the per-request consent model.
🤖 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 `@server/modules/plugins/plugin-registry.service.ts` around lines 100 - 135, Restrict the process-wide override used by setAllowPluginBuildScript so enabling it cannot silently allow builds for all later install and update requests. Prefer gating the override behind an environment variable evaluated once at startup, while preserving options.allowBuild as the normal per-request consent path; alternatively, explicitly document and enforce that the setter intentionally bypasses per-request consent.
🤖 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 `@server/modules/plugins/plugin-registry.service.ts`:
- Around line 458-475: Validate remoteUrl immediately after extracting it in the
re-cloning flow, before invoking spawn for git clone. Apply the same allowlist
used by installPluginFromGit: permit only https:// and git@ remotes, and reject
all other schemes by cleaning up and rejecting with an appropriate error. Keep
the existing missing-remote handling unchanged.
- Around line 424-452: Update the backupDir naming in finalize so the backup
path uses a `.tmp-` prefixed basename, while retaining the plugin identity and
unique process/timestamp suffix. Keep the existing rollback and cleanup behavior
unchanged so the backup remains recoverable but is ignored by scanPlugins().
In `@server/modules/plugins/plugins.service.ts`:
- Around line 124-132: The catch block in the plugin update flow must preserve
the original update error even when recovery fails. Wrap the wasRunning recovery
call to startServerIfAvailable(this.getManifest(pluginName)) in its own guarded
attempt, suppress any recovery error, and rethrow the original caught error
unchanged.
---
Nitpick comments:
In `@server/modules/plugins/plugin-registry.service.ts`:
- Around line 100-135: Restrict the process-wide override used by
setAllowPluginBuildScript so enabling it cannot silently allow builds for all
later install and update requests. Prefer gating the override behind an
environment variable evaluated once at startup, while preserving
options.allowBuild as the normal per-request consent path; alternatively,
explicitly document and enforce that the setter intentionally bypasses
per-request consent.
In `@server/modules/plugins/tests/plugin-registry.service.test.ts`:
- Around line 6-30: Add tests covering the build-policy branches in
installPluginFromGit or the relevant registry flow: allow plugins without a
build script, reject plugins declaring a build script unless opt-in is enabled,
and permit the build when setAllowPluginBuildScript(true) or allowBuild: true is
applied. Use an existing local fixture repository if possible; otherwise expose
runBuildIfNeeded only as needed for focused testing.
In `@server/modules/plugins/tests/plugins.service.test.ts`:
- Around line 34-57: Update the `update()` restart test to pass `{ allowBuild:
true }` to `service.update('demo', ...)`, capture the options argument in the
mocked `update` dependency, and assert it matches the provided options while
preserving the existing rejection and restart-order assertions.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1599a5bd-e106-4f76-bce4-b0694ee962c5
📒 Files selected for processing (5)
server/modules/plugins/plugin-registry.service.tsserver/modules/plugins/plugins.routes.tsserver/modules/plugins/plugins.service.tsserver/modules/plugins/tests/plugin-registry.service.test.tsserver/modules/plugins/tests/plugins.service.test.ts
…lone RCE
Three CodeRabbit follow-ups on the plugin update staging flow:
1. `backupDir` was named `${pluginDir}.previous-<pid>-<timestamp>`,
a sibling inside the plugins directory. `scanPlugins()` skips only
entries whose name starts with `.tmp-`, so the backup would have
been picked up by `scanPlugins()` during the swap window and (if
the restore rename also failed) permanently. Move the backup
under a `.tmp-previous-` prefix so it stays recoverable but
invisible to scanning.
2. `updatePluginFromGit` re-clones the plugin's stored remote URL into
a temp directory. The HTTP route layer enforces `https://`/`git@`,
but the registry re-reads the URL from `.git/config` and passed it
straight to `git clone` with no scheme check. A `.git/config`
containing an `ext::` or `file://` remote (writable by any process
with file-system access to the plugin directory, including the
plugin's own server) would have been honoured by `git clone`,
and `ext::` runs a shell command. Apply the same `https://` /
`git@` allowlist used by `installPluginFromGit`.
3. `plugins.service.ts update()`'s catch block restarted the
previously running server unconditionally and then rethrew. If
`startServerIfAvailable(this.getManifest(pluginName))` itself
threw (e.g. `getManifest` returned `PLUGIN_NOT_FOUND` because
`scanPlugins()` had evicted the entry), the recovery error would
mask the original update error. Isolate the recovery attempt so
the original error always propagates and a recovery failure is
logged but not thrown.
Co-authored-by: cgsdn <chaogeshuodiannao@users.noreply.github.com>
|
1 participant
P0 — Plugin install executes
npm run buildon attacker-controlled repositoriesVulnerability description
installPluginFromGit(url)andupdatePluginFromGit(name)inserver/modules/plugins/plugin-registry.service.tsinvokedrunBuildIfNeeded()immediately afternpm install.npm installresolves transitive dependencies and runs thepreinstall/install/postinstalllifecycle scripts of any package the registry pulls in. The build step then additionally executed whateverbuildscript the repository'spackage.jsondeclared. Because both routes accept an arbitrary URL from the client (POST /api/plugins/install) or pull from a stored remote (POST /api/plugins/:name/update), an attacker who convinced an authenticated operator to install or update a plugin could execute arbitrary Node.js code on the server host under the server's user account. The credentials available to that code (env, plugin directory contents, on-disk auth state) include everything needed to fully compromise the application.Fix
Disable the implicit
npm run buildstep on both install and update. Operators who want to build a plugin now have to opt in explicitly by passingallowBuild: truein the request body after they have manually inspected the repository. The registry surfaces the opt-in through a singlerunBuildIfNeeded(pluginDir, { allowBuild })parameter; routes passallowBuildstraight from the request body. The plugin update path was also rewritten to stage the new tree in a temporary directory (re-clone → validate manifest →npm install→ build policy → atomic rename) so a rejected update never disturbs the live plugin and so apreinstall/install/postinstallfailure leaves the existing plugin running. The plugin update service stops the running plugin server before the swap and restarts it on success and on rejection, so the previous behaviour of leaving the server down after a bad update is no longer reachable.Files
server/modules/plugins/plugin-registry.service.tsserver/modules/plugins/plugins.routes.tsserver/modules/plugins/plugins.service.tsserver/modules/plugins/tests/plugin-registry.service.test.ts(new)server/modules/plugins/tests/plugins.service.test.tsCommits
df75870—fix(plugins): disable auto-running npm run build during plugin install8954fa1—fix(plugins): stage plugin updates so a rejected update leaves the live plugin untouched0c2f5e9—fix(plugins): preserve the live plugin directory when the update swap fails4dd03c2—fix(plugins): require https:// or git@ scheme in installPluginFromGitCode snippet —
runBuildIfNeedednow requires explicit consentCode snippet — registry now refuses non-https / non-git remotes
🤖 Generated with Claude Code
Summary by CodeRabbit
Security & Reliability
Bug Fixes