diff --git a/server/modules/plugins/plugin-registry.service.ts b/server/modules/plugins/plugin-registry.service.ts index f5816516ed..c96d1f09fb 100644 --- a/server/modules/plugins/plugin-registry.service.ts +++ b/server/modules/plugins/plugin-registry.service.ts @@ -97,17 +97,43 @@ export function validateManifest(manifest) { const BUILD_TIMEOUT_MS = 60_000; +/** + * Whether `npm run build` should execute after `npm install` for newly cloned + * plugins. Plugin build scripts run with the host Node process's privileges, + * so they are a remote code execution vector for any party who can supply a + * malicious plugin URL. The default is OFF: only the install/update caller's + * explicit opt-in (via the {@link PluginInstallOptions.allowBuild} flag) will + * permit build scripts to run. + */ +let ALLOW_PLUGIN_BUILD_SCRIPT = false; + +/** Process-wide override used by tests and callers that have vetted the plugin. */ +export function setAllowPluginBuildScript(allowed) { + ALLOW_PLUGIN_BUILD_SCRIPT = allowed === true; +} + /** Run `npm run build` if the plugin's package.json declares a build script. */ -function runBuildIfNeeded(dir, packageJsonPath, onSuccess, onError) { +function runBuildIfNeeded(dir, packageJsonPath, options, onSuccess, onError) { + let pkg; try { - const pkg = JSON.parse(fs.readFileSync(packageJsonPath, 'utf-8')); - if (!pkg.scripts?.build) { - return onSuccess(); - } + pkg = JSON.parse(fs.readFileSync(packageJsonPath, 'utf-8')); } catch { return onSuccess(); // Unreadable package.json — skip build } + if (!pkg.scripts?.build) { + return onSuccess(); + } + + if (!ALLOW_PLUGIN_BUILD_SCRIPT && !options?.allowBuild) { + return onError(new Error( + 'Plugin declares a "build" script but plugin builds are disabled by default. ' + + 'Plugin build scripts run arbitrary code with the server process privileges. ' + + 'To install this plugin, ship a pre-built artifact and remove the build script, ' + + 'or set `allowBuild: true` after manually inspecting the build script.', + )); + } + const buildProcess = spawn('npm', ['run', 'build'], { cwd: dir, stdio: ['ignore', 'pipe', 'pipe'], @@ -249,7 +275,7 @@ export function resolvePluginAssetPath(name, assetPath) { return realResolved; } -export function installPluginFromGit(url) { +export function installPluginFromGit(url, options) { return new Promise((resolve, reject) => { if (typeof url !== 'string' || !url.trim()) { return reject(new Error('Invalid URL: must be a non-empty string')); @@ -257,6 +283,13 @@ export function installPluginFromGit(url) { if (url.startsWith('-')) { return reject(new Error('Invalid URL: must not start with "-"')); } + // Only allow the supported remote transports. The HTTP layer in + // plugins.service.ts already enforces this for incoming requests, but + // validating here too prevents any internal caller (tests, future + // programmatic install paths) from bypassing the scheme check. + if (!url.startsWith('https://') && !url.startsWith('git@')) { + return reject(new Error('Invalid URL: only https:// and git@ remotes are supported')); + } // Extract repo name from URL for directory name const urlClean = url.replace(/\.git$/, '').replace(/\/$/, ''); @@ -350,7 +383,7 @@ export function installPluginFromGit(url) { cleanupTemp(); return reject(new Error(`npm install for ${repoName} failed (exit code ${npmCode})`)); } - runBuildIfNeeded(tempDir, packageJsonPath, () => finalize(manifest), (err) => { cleanupTemp(); reject(err); }); + runBuildIfNeeded(tempDir, packageJsonPath, options, () => finalize(manifest), (err) => { cleanupTemp(); reject(err); }); }); npmProcess.on('error', (err) => { @@ -369,61 +402,145 @@ export function installPluginFromGit(url) { }); } -export function updatePluginFromGit(name) { +export function updatePluginFromGit(name, options) { return new Promise((resolve, reject) => { const pluginDir = getPluginDir(name); if (!pluginDir) { return reject(new Error(`Plugin "${name}" not found`)); } - // Only fast-forward to avoid silent divergence - const gitProcess = spawn('git', ['pull', '--ff-only', '--'], { - cwd: pluginDir, + const pluginsDir = getPluginsDir(); + // Clone the updated tree into a sibling temp directory and only swap it + // into place after every side-effectful step (manifest validation, + // npm install, optional build policy) has succeeded. This means a + // rejected update never mutates the live plugin directory and never + // leaves a running plugin server pointing at a half-updated tree. + const tempDir = fs.mkdtempSync(path.join(pluginsDir, `.tmp-update-${name}-`)); + + const cleanupTemp = () => { + try { fs.rmSync(tempDir, { recursive: true, force: true }); } catch {} + }; + + const finalize = (manifest) => { + // Replace the live directory with the validated temp dir while keeping + // the previous tree in a sibling backup. If the swap fails partway + // through, restore the backup so the operator still has the previous + // plugin on disk; only delete the backup after the swap succeeds. + const backupDir = fs.existsSync(pluginDir) + ? path.join(pluginsDir, `.tmp-previous-${path.basename(pluginDir)}-${process.pid}-${Date.now()}`) + : null; + try { + if (backupDir) { + fs.renameSync(pluginDir, backupDir); + } + fs.renameSync(tempDir, pluginDir); + } catch (err) { + // Roll back: remove the partially-installed temp dir (if the second + // rename succeeded we have nothing to restore) and put the backup + // back in place of the live directory (if one existed). + try { fs.rmSync(tempDir, { recursive: true, force: true }); } catch {} + if (backupDir && fs.existsSync(backupDir) && !fs.existsSync(pluginDir)) { + try { fs.renameSync(backupDir, pluginDir); } catch {} + } + return reject(new Error(`Failed to move updated plugin into place: ${err.message}`)); + } + + if (backupDir) { + try { fs.rmSync(backupDir, { recursive: true, force: true }); } catch {} + } + resolve(manifest); + }; + + // Clone the live plugin's current remote URL into the temp dir. We + // intentionally re-clone (rather than `git pull` against the live + // directory) so a failure or a rejected build leaves the live tree + // untouched. + const configPath = path.join(pluginDir, '.git', 'config'); + let remoteUrl = null; + try { + const gitConfig = fs.readFileSync(configPath, 'utf-8'); + const match = gitConfig.match(/\[remote "origin"\][^[]*url\s*=\s*(.+)/); + if (match) remoteUrl = match[1].trim(); + } catch (err) { + cleanupTemp(); + return reject(new Error(`Failed to read git remote for "${name}": ${err.message}`)); + } + if (!remoteUrl) { + cleanupTemp(); + return reject(new Error(`Plugin "${name}" has no git remote URL`)); + } + // Only allow the supported remote transports. `installPluginFromGit` + // enforces the same scheme; a plugin's stored remote could otherwise + // be `ext::` or `file://`, both of which `git clone` accepts and the + // first of which executes a shell command. + if (!remoteUrl.startsWith('https://') && !remoteUrl.startsWith('git@')) { + cleanupTemp(); + return reject(new Error(`Plugin "${name}" has an unsupported git remote: only https:// and git@ remotes are supported`)); + } + + const cloneProcess = spawn('git', ['clone', '--depth', '1', '--', remoteUrl, tempDir], { stdio: ['ignore', 'pipe', 'pipe'], }); - let stderr = ''; - gitProcess.stderr.on('data', (data) => { stderr += data.toString(); }); + let cloneStderr = ''; + cloneProcess.stderr.on('data', (data) => { cloneStderr += data.toString(); }); - gitProcess.on('close', (code) => { + cloneProcess.on('close', (code) => { if (code !== 0) { - return reject(new Error(`git pull failed (exit code ${code}): ${stderr.trim()}`)); + cleanupTemp(); + return reject(new Error(`git clone failed (exit code ${code}): ${cloneStderr.trim()}`)); + } + + // Validate manifest exists and is well-formed before any further work. + const manifestPath = path.join(tempDir, 'manifest.json'); + if (!fs.existsSync(manifestPath)) { + cleanupTemp(); + return reject(new Error('Cloned repository does not contain a manifest.json')); } - // Re-validate manifest after update - const manifestPath = path.join(pluginDir, 'manifest.json'); let manifest; try { manifest = JSON.parse(fs.readFileSync(manifestPath, 'utf-8')); } catch { + cleanupTemp(); return reject(new Error('manifest.json is not valid JSON after update')); } const validation = validateManifest(manifest); if (!validation.valid) { + cleanupTemp(); return reject(new Error(`Invalid manifest after update: ${validation.error}`)); } - // Re-run npm install if package.json exists - const packageJsonPath = path.join(pluginDir, 'package.json'); + // Re-run npm install if package.json exists. + const packageJsonPath = path.join(tempDir, 'package.json'); if (fs.existsSync(packageJsonPath)) { const npmProcess = spawn('npm', ['install', '--ignore-scripts'], { - cwd: pluginDir, + cwd: tempDir, stdio: ['ignore', 'pipe', 'pipe'], }); + npmProcess.on('close', (npmCode) => { if (npmCode !== 0) { + cleanupTemp(); return reject(new Error(`npm install for ${name} failed (exit code ${npmCode})`)); } - runBuildIfNeeded(pluginDir, packageJsonPath, () => resolve(manifest), (err) => reject(err)); + // Build-policy rejection leaves the live plugin directory untouched + // because we have not swapped `tempDir` into place yet. + runBuildIfNeeded(tempDir, packageJsonPath, options, () => finalize(manifest), (err) => { cleanupTemp(); reject(err); }); + }); + + npmProcess.on('error', (err) => { + cleanupTemp(); + reject(err); }); - npmProcess.on('error', (err) => reject(err)); } else { - resolve(manifest); + finalize(manifest); } }); - gitProcess.on('error', (err) => { + cloneProcess.on('error', (err) => { + cleanupTemp(); reject(new Error(`Failed to spawn git: ${err.message}`)); }); }); diff --git a/server/modules/plugins/plugins.routes.ts b/server/modules/plugins/plugins.routes.ts index 9174f04c10..d01c083fab 100644 --- a/server/modules/plugins/plugins.routes.ts +++ b/server/modules/plugins/plugins.routes.ts @@ -34,8 +34,18 @@ export function createPluginsRouter(service: ReturnType service.setEnabled(routeParameter(req.params.name), req.body?.enabled))); - router.post('/install', respond((req) => service.install(req.body?.url))); - router.post('/:name/update', respond((req) => service.update(routeParameter(req.params.name)))); + router.post('/install', respond((req) => { + const body = (req.body ?? {}) as { url?: unknown; allowBuild?: unknown }; + // Build scripts run arbitrary code with server privileges. Only opt in when the + // operator has manually vetted the plugin's build script. + const allowBuild = body.allowBuild === true; + return service.install(body.url, { allowBuild }); + })); + router.post('/:name/update', respond((req) => { + const body = (req.body ?? {}) as { allowBuild?: unknown }; + const allowBuild = body.allowBuild === true; + return service.update(routeParameter(req.params.name), { allowBuild }); + })); router.all('/:name/rpc/*', async (req, res, next) => { try { const { port, secrets } = await service.prepareRpc(routeParameter(req.params.name)); diff --git a/server/modules/plugins/plugins.service.ts b/server/modules/plugins/plugins.service.ts index debdfd092c..95dfd82dba 100644 --- a/server/modules/plugins/plugins.service.ts +++ b/server/modules/plugins/plugins.service.ts @@ -18,8 +18,8 @@ type PluginDependencies = { resolveAsset(pluginName: string, assetPath: string): string | null; assetIsFile(assetPath: string): boolean; contentType(assetPath: string): string; - install(url: string): Promise; - update(pluginName: string): Promise; + install(url: string, options?: { allowBuild?: boolean }): Promise; + update(pluginName: string, options?: { allowBuild?: boolean }): Promise; uninstall(pluginName: string): Promise; startServer(pluginName: string, pluginDirectory: string, serverConfig: unknown): Promise; stopServer(pluginName: string): Promise; @@ -104,22 +104,38 @@ export function createPluginsService(dependencies: PluginDependencies) { } return { success: true, name: pluginName, enabled }; }, - async install(urlInput: unknown) { + async install(urlInput: unknown, options?: { allowBuild?: boolean }) { const url = typeof urlInput === 'string' ? urlInput.trim() : ''; if (!url || (!url.startsWith('https://') && !url.startsWith('git@'))) { throw new AppError('URL must start with https:// or git@', { code: 'INVALID_PLUGIN_URL', statusCode: 400 }); } - const plugin = normalizePluginManifest(await dependencies.install(url)); + const plugin = normalizePluginManifest(await dependencies.install(url, options)); await startServerIfAvailable(plugin); return { success: true, plugin }; }, - async update(pluginName: string) { + async update(pluginName: string, options?: { allowBuild?: boolean }) { validatePluginName(pluginName); const wasRunning = dependencies.isServerRunning(pluginName); if (wasRunning) await dependencies.stopServer(pluginName); - const plugin = normalizePluginManifest(await dependencies.update(pluginName)); - if (wasRunning) await startServerIfAvailable(plugin); - return { success: true, plugin }; + try { + const plugin = normalizePluginManifest(await dependencies.update(pluginName, options)); + if (wasRunning) await startServerIfAvailable(plugin); + return { success: true, plugin }; + } catch (error) { + // The registry stages updates into a temp directory and only swaps + // them into the live plugin directory once every step (manifest + // validation, npm install, optional build policy) has succeeded. If + // any step rejects, the live directory is untouched, so we can + // safely bring the previously running server back up. + if (wasRunning) { + try { + await startServerIfAvailable(this.getManifest(pluginName)); + } catch (restoreError) { + dependencies.logError(`Failed to restart plugin server for ${pluginName} after a failed update`, restoreError); + } + } + throw error; + } }, async prepareRpc(pluginName: string) { validatePluginName(pluginName); diff --git a/server/modules/plugins/tests/plugin-registry.service.test.ts b/server/modules/plugins/tests/plugin-registry.service.test.ts new file mode 100644 index 0000000000..526c4ceedc --- /dev/null +++ b/server/modules/plugins/tests/plugin-registry.service.test.ts @@ -0,0 +1,30 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; + +import { installPluginFromGit } from '../plugin-registry.service.js'; + +// Smoke tests for the URL pre-checks in the registry. The end-to-end +// swap-failure path is exercised by integration tests elsewhere because +// injecting a controlled failure of `fs.renameSync` requires an fs mock +// framework that this codebase does not currently depend on. + +test('installPluginFromGit rejects file:// URLs', async () => { + await assert.rejects( + async () => installPluginFromGit('file:///tmp/local'), + (error: unknown) => error instanceof Error && /Invalid URL/.test(error.message), + ); +}); + +test('installPluginFromGit rejects URLs that begin with option prefixes', async () => { + await assert.rejects( + async () => installPluginFromGit('--upload-pack=malicious'), + (error: unknown) => error instanceof Error && /Invalid URL/.test(error.message), + ); +}); + +test('installPluginFromGit rejects non-HTTPS, non-SSH URLs', async () => { + await assert.rejects( + async () => installPluginFromGit('http://example.com/repo.git'), + (error: unknown) => error instanceof Error && /Invalid URL/.test(error.message), + ); +}); diff --git a/server/modules/plugins/tests/plugins.service.test.ts b/server/modules/plugins/tests/plugins.service.test.ts index 267ddc1e0d..9c81d0ac16 100644 --- a/server/modules/plugins/tests/plugins.service.test.ts +++ b/server/modules/plugins/tests/plugins.service.test.ts @@ -30,3 +30,28 @@ test('setEnabled persists configuration and starts an enabled plugin server', as await service.setEnabled('demo', true); assert.deepEqual(operations, ['save', 'start']); }); + +test('update() restarts the running plugin server when the update is rejected', async () => { + const operations: string[] = []; + const runningFlags = [true, false]; + let isRunningIndex = 0; + const service = createPluginsService(dependencies({ + scanPlugins: () => [{ name: 'demo', dirName: 'demo', server: { entry: 'server.js' } }], + getPluginDirectory: () => '/plugins/demo', + startServer: async () => { operations.push('start'); return 4000; }, + stopServer: async () => { operations.push('stop'); }, + isServerRunning: () => runningFlags[isRunningIndex++] === true, + update: async () => { + throw new Error('build script rejected'); + }, + })); + + await assert.rejects( + service.update('demo'), + (error: unknown) => error instanceof Error && /build script rejected/.test(error.message), + ); + + // The previously running plugin server must be brought back online after a + // rejected update — leaving it stopped would silently break the plugin. + assert.deepEqual(operations, ['stop', 'start']); +});