diff --git a/server/index.ts b/server/index.ts index fac4734363..a88e8b80e1 100644 --- a/server/index.ts +++ b/server/index.ts @@ -70,6 +70,10 @@ const RUNNING_VERSION = (() => { return null; } })(); +const SERVER_PORT = Number.parseInt(process.env.SERVER_PORT || '3001', 10); +const HOST = process.env.HOST || '0.0.0.0'; +const DISPLAY_HOST = getConnectableHost(HOST); +const VITE_PORT = process.env.VITE_PORT || 5173; const systemRoutes = createSystemModule({ appRoot: APP_ROOT, installMode, @@ -119,7 +123,43 @@ const wss = createWebSocketServer(server, { // Make WebSocket server available to routes app.locals.wss = wss; -app.use(cors({ exposedHeaders: ['X-Refreshed-Token', 'X-Auth-Error'] })); +// CORS — only reflect origins that share a host:port with the server. The +// default `cors()` configuration reflects any Origin header, which lets any +// malicious site make authenticated cross-origin requests against this server +// when a victim visits it in the same browser session. Limiting the +// reflected origin to the server's own loopback / LAN addresses keeps the +// browser same-origin policy intact without breaking the local Electron app +// or LAN-hosted deployments. +const corsOriginReflector = (origin: string | undefined, callback: (err: Error | null, allow?: boolean) => void) => { + // No Origin header → same-origin request (e.g. server-to-server, curl); + // these are not subject to CORS and should always be allowed through. + if (!origin) { + callback(null, true); + return; + } + + try { + const parsed = new URL(origin); + const requestHost = parsed.hostname; + const requestPort = parsed.port || (parsed.protocol === 'https:' ? '443' : '80'); + const serverHost = HOST === '0.0.0.0' || HOST === '::' ? requestHost : HOST; + const serverPort = String(SERVER_PORT); + + if (requestHost === serverHost && requestPort === serverPort) { + callback(null, true); + return; + } + } catch { + // Malformed Origin header — refuse. + } + + callback(null, false); +}; + +app.use(cors({ + origin: corsOriginReflector, + exposedHeaders: ['X-Refreshed-Token', 'X-Auth-Error'], +})); app.use(express.json({ limit: '50mb', type: (req) => { @@ -272,10 +312,6 @@ app.use((err: unknown, req: Request, res: Response, next: NextFunction) => { }); }); -const SERVER_PORT = Number.parseInt(process.env.SERVER_PORT || '3001', 10); -const HOST = process.env.HOST || '0.0.0.0'; -const DISPLAY_HOST = getConnectableHost(HOST); -const VITE_PORT = process.env.VITE_PORT || 5173; const LOCAL_SERVER_MARKER_PATH = path.join(os.homedir(), '.cloudcli', 'local-server.json'); function getErrorCode(error: unknown): string | undefined { diff --git a/server/modules/auth/auth.routes.ts b/server/modules/auth/auth.routes.ts index 99342fe88d..117f68ba7a 100644 --- a/server/modules/auth/auth.routes.ts +++ b/server/modules/auth/auth.routes.ts @@ -2,9 +2,24 @@ import express from 'express'; import type { RequestHandler } from 'express'; import type { createAuthService } from './auth.service.js'; +import { createRateLimiter } from './rate-limit.middleware.js'; type AuthenticatedRequest = express.Request & { user?: unknown }; +// 10 attempts per IP per minute is generous for legitimate use (the only +// registered user is the local admin) but tight enough to slow down online +// credential stuffing. Lockouts extend the window by an additional minute so +// a misconfigured client cannot hammer the endpoint forever. +const AUTH_RATE_LIMIT_MAX = 10; +const AUTH_RATE_LIMIT_WINDOW_MS = 60_000; +const AUTH_RATE_LIMIT_LOCKOUT_MS = 60_000; + +const authRateLimit = createRateLimiter({ + maxAttempts: AUTH_RATE_LIMIT_MAX, + windowMs: AUTH_RATE_LIMIT_WINDOW_MS, + lockoutMs: AUTH_RATE_LIMIT_LOCKOUT_MS, +}); + /** * Creates the Auth transport adapter. Handlers only parse request data and * delegate authentication behavior to the injected application service. @@ -23,7 +38,7 @@ export function createAuthRouter( } }); - router.post('/register', async (req, res, next) => { + router.post('/register', authRateLimit.middleware, async (req, res, next) => { try { const body = req.body as { username?: unknown; password?: unknown }; res.json(await service.register(body.username, body.password)); @@ -32,7 +47,7 @@ export function createAuthRouter( } }); - router.post('/login', async (req, res, next) => { + router.post('/login', authRateLimit.middleware, async (req, res, next) => { try { const body = req.body as { username?: unknown; password?: unknown }; res.json(await service.login(body.username, body.password)); diff --git a/server/modules/auth/rate-limit.middleware.ts b/server/modules/auth/rate-limit.middleware.ts new file mode 100644 index 0000000000..1df5340f11 --- /dev/null +++ b/server/modules/auth/rate-limit.middleware.ts @@ -0,0 +1,127 @@ +// rate-limit middleware exposes a single factory that the auth router mounts +// on the login and register endpoints. Keeping the failure path explicit (no +// shared store) so the limit applies per-process without coupling to the +// persistence layer. +import type { Request, RequestHandler, Response } from 'express'; + +type RateLimiterOptions = { + /** Maximum number of attempts allowed within the rolling window. */ + maxAttempts: number; + /** Length of the rolling window, in milliseconds. */ + windowMs: number; + /** How long a locked-out client should be told to wait before retrying. */ + lockoutMs?: number; + /** Optional clock for tests. */ + now?: () => number; +}; + +type AttemptRecord = { + /** Timestamps (ms) of attempts that fall inside the rolling window. */ + timestamps: number[]; + /** Earliest time at which the client may try again after a lockout. */ + blockedUntil: number; +}; + +function readClientKey(req: Request): string { + // Prefer the address of the TCP peer; fall back to a header chain that + // includes the most common reverse-proxy forwarded-for conventions. + const socketAddress = req.socket?.remoteAddress; + if (socketAddress && socketAddress !== '::1' && socketAddress !== '127.0.0.1') { + return socketAddress; + } + + const forwarded = req.headers['x-forwarded-for']; + if (typeof forwarded === 'string' && forwarded.trim()) { + return forwarded.split(',')[0]!.trim(); + } + if (Array.isArray(forwarded) && forwarded.length > 0) { + return forwarded[0]!.split(',')[0]!.trim(); + } + + return socketAddress || 'unknown'; +} + +/** + * Builds a per-key sliding-window rate limiter suitable for protecting the + * login and registration endpoints against credential-stuffing attempts. + * + * The limiter keeps an in-memory `Map` of client-key → attempt history. Each + * incoming request drops expired timestamps from the history; if the count + * after dropping exceeds `maxAttempts`, the request is rejected with HTTP + * 429 until enough time has elapsed for at least one slot to expire. + */ +export function createRateLimiter(options: RateLimiterOptions): { + middleware: RequestHandler; + reset: () => void; +} { + const maxAttempts = options.maxAttempts; + const windowMs = options.windowMs; + const lockoutMs = options.lockoutMs ?? windowMs; + const clock = options.now ?? (() => Date.now()); + + const records = new Map(); + + const middleware: RequestHandler = (req: Request, res: Response, next) => { + const clientKey = readClientKey(req); + const now = clock(); + let record = records.get(clientKey); + if (!record) { + record = { timestamps: [], blockedUntil: 0 }; + records.set(clientKey, record); + } + + // An active lockout short-circuits the limiter; do not consume an attempt + // slot so a misbehaving client cannot extend the lockout indefinitely. + if (record.blockedUntil > now) { + const retryAfterSeconds = Math.max(1, Math.ceil((record.blockedUntil - now) / 1000)); + res.setHeader('Retry-After', String(retryAfterSeconds)); + res.status(429).json({ + success: false, + error: { + code: 'RATE_LIMITED', + message: 'Too many attempts. Please try again later.', + retryAfterSeconds, + }, + }); + return; + } + + // Drop timestamps that have aged out of the rolling window. + const cutoff = now - windowMs; + record.timestamps = record.timestamps.filter((timestamp) => timestamp > cutoff); + + if (record.timestamps.length >= maxAttempts) { + // `lockoutMs` is the minimum backoff, but `Retry-After` must reflect + // the *next* time a request can succeed — i.e. when at least one of + // the retained timestamps ages out of the rolling window. If the + // operator configured `lockoutMs < windowMs`, the rolling window + // outlives the lockout, so blocking for only `lockoutMs` would let a + // client come back and immediately trigger another lockout despite the + // previous response telling it to wait. + const earliestExpiry = record.timestamps.length > 0 + ? record.timestamps[0]! + windowMs + : now + windowMs; + const nextAvailableAt = Math.max(now + lockoutMs, earliestExpiry); + record.blockedUntil = nextAvailableAt; + const retryAfterSeconds = Math.max(1, Math.ceil((nextAvailableAt - now) / 1000)); + res.setHeader('Retry-After', String(retryAfterSeconds)); + res.status(429).json({ + success: false, + error: { + code: 'RATE_LIMITED', + message: 'Too many attempts. Please try again later.', + retryAfterSeconds, + }, + }); + return; + } + + record.timestamps.push(now); + next(); + }; + + return { + middleware, + reset: () => records.clear(), + }; +} diff --git a/server/modules/auth/tests/rate-limit.middleware.test.ts b/server/modules/auth/tests/rate-limit.middleware.test.ts new file mode 100644 index 0000000000..18811b0953 --- /dev/null +++ b/server/modules/auth/tests/rate-limit.middleware.test.ts @@ -0,0 +1,203 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; + +import { createRateLimiter } from '../rate-limit.middleware.js'; + +function createMockResponse(): { + statusCode: number; + body: unknown; + headers: Record; + status(code: number): typeof response; + setHeader(name: string, value: string): void; + json(body: unknown): typeof response; +} { + const response = { + statusCode: 200, + body: undefined as unknown, + headers: {} as Record, + status(code: number) { + this.statusCode = code; + return this; + }, + setHeader(name: string, value: string) { + this.headers[name] = value; + }, + json(body: unknown) { + this.body = body; + return this; + }, + }; + return response; +} + +function createMockRequest(clientKey: string): { + socket?: { remoteAddress?: string }; + headers: Record; +} { + return { + socket: { remoteAddress: clientKey }, + headers: {}, + }; +} + +test('requests below the attempt cap pass through', () => { + const limiter = createRateLimiter({ maxAttempts: 3, windowMs: 1000, now: () => 1000 }); + const next = (() => { calls.push(1); }) as () => void; + const calls: number[] = []; + + for (let i = 0; i < 3; i += 1) { + limiter.middleware(createMockRequest('203.0.113.1') as never, createMockResponse() as never, next); + } + + assert.equal(calls.length, 3); +}); + +test('the request that trips the limit is rejected with 429 + Retry-After', () => { + let currentTime = 1000; + const limiter = createRateLimiter({ + maxAttempts: 2, + windowMs: 1000, + lockoutMs: 5000, + now: () => currentTime, + }); + const next = () => undefined; + const blocked: { status: number; body: unknown; headers: Record } = { + status: 200, + body: undefined, + headers: {}, + }; + const response = { + statusCode: 0, + body: undefined as unknown, + headers: {} as Record, + status(code: number) { this.statusCode = code; return this; }, + setHeader(name: string, value: string) { this.headers[name] = value; }, + json(body: unknown) { this.body = body; return this; }, + }; + + // Two successful requests. + limiter.middleware(createMockRequest('203.0.113.2') as never, response as never, next); + limiter.middleware(createMockRequest('203.0.113.2') as never, response as never, next); + + // Third request trips the limiter. + limiter.middleware(createMockRequest('203.0.113.2') as never, response as never, next); + + assert.equal(response.statusCode, 429); + assert.equal(response.headers['Retry-After'], '5'); + assert.ok(response.body && typeof response.body === 'object'); + assert.equal((response.body as { success: boolean }).success, false); +}); + +test('lockout does not extend when the client keeps hammering', () => { + let currentTime = 1000; + const limiter = createRateLimiter({ + maxAttempts: 1, + windowMs: 1000, + lockoutMs: 2000, + now: () => currentTime, + }); + const next = () => undefined; + const response = { + statusCode: 0, + body: undefined as unknown, + headers: {} as Record, + status(code: number) { this.statusCode = code; return this; }, + setHeader(name: string, value: string) { this.headers[name] = value; }, + json(body: unknown) { this.body = body; return this; }, + }; + + // First request consumes the only slot. + limiter.middleware(createMockRequest('203.0.113.3') as never, response as never, next); + // Next request trips the limit and starts a lockout. + limiter.middleware(createMockRequest('203.0.113.3') as never, response as never, next); + const firstBlockEnd = response.headers['Retry-After']; + + // Advance time past one second — past the original lockout boundary — and + // verify the block window does NOT reset/extend (would happen if we kept + // consuming slots). Advancing by 1100 ms puts us 100 ms past the original + // `now + lockoutMs` of 3000 but still inside `timestamps[0] + windowMs` + // (1000 + 1000 = 2000), so the preserved lockout is observable in the + // updated `Retry-After` header. + currentTime += 1100; + limiter.middleware(createMockRequest('203.0.113.3') as never, response as never, next); + + assert.equal(firstBlockEnd, '2'); + assert.equal(response.statusCode, 429); + assert.equal(response.headers['Retry-After'], '1'); +}); + +test('Retry-After reflects the rolling window when it outlives lockoutMs', () => { + let currentTime = 1000; + const limiter = createRateLimiter({ + maxAttempts: 1, + windowMs: 5000, + lockoutMs: 1000, + now: () => currentTime, + }); + const next = () => undefined; + const response = { + statusCode: 0, + body: undefined as unknown, + headers: {} as Record, + status(code: number) { this.statusCode = code; return this; }, + setHeader(name: string, value: string) { this.headers[name] = value; }, + json(body: unknown) { this.body = body; return this; }, + }; + + // Consume the only slot and trip the limit. + limiter.middleware(createMockRequest('203.0.113.7') as never, response as never, next); + limiter.middleware(createMockRequest('203.0.113.7') as never, response as never, next); + + // `Retry-After` must be the larger of `lockoutMs` and the time until the + // earliest retained timestamp ages out — here the rolling window (5 s) + // outlives `lockoutMs` (1 s), so the client must wait 5 s. + assert.equal(response.headers['Retry-After'], '5'); +}); + +test('rolling window lets the client through once old attempts age out', () => { + let currentTime = 1000; + const limiter = createRateLimiter({ + maxAttempts: 2, + windowMs: 1000, + now: () => currentTime, + }); + const next = (() => { calls.push(1); }) as () => void; + const calls: number[] = []; + const response = { + statusCode: 0, + body: undefined as unknown, + headers: {} as Record, + status(code: number) { this.statusCode = code; return this; }, + setHeader(name: string, value: string) { this.headers[name] = value; }, + json(body: unknown) { this.body = body; return this; }, + }; + + limiter.middleware(createMockRequest('203.0.113.4') as never, response as never, next); + limiter.middleware(createMockRequest('203.0.113.4') as never, response as never, next); + limiter.middleware(createMockRequest('203.0.113.4') as never, response as never, next); + assert.equal(calls.length, 2); + + // Advance past the window so the earlier timestamps drop off. + currentTime += 1100; + limiter.middleware(createMockRequest('203.0.113.4') as never, response as never, next); + assert.equal(calls.length, 3); +}); + +test('different client keys are tracked independently', () => { + const limiter = createRateLimiter({ maxAttempts: 1, windowMs: 1000, now: () => 1000 }); + const next = (() => { calls.push(1); }) as () => void; + const calls: number[] = []; + const response = { + statusCode: 0, + body: undefined as unknown, + headers: {} as Record, + status(code: number) { this.statusCode = code; return this; }, + setHeader(name: string, value: string) { this.headers[name] = value; }, + json(body: unknown) { this.body = body; return this; }, + }; + + limiter.middleware(createMockRequest('203.0.113.5') as never, response as never, next); + limiter.middleware(createMockRequest('203.0.113.6') as never, response as never, next); + + assert.equal(calls.length, 2); +}); diff --git a/server/modules/plugins/plugin-registry.service.ts b/server/modules/plugins/plugin-registry.service.ts index f5816516ed..cb0cabd3ae 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')); @@ -350,7 +376,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 +395,124 @@ 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) => { + // Atomically replace the live directory with the validated temp dir. + // `rename` is atomic on the same filesystem on POSIX; Windows treats + // it as remove+create which is fine because no other writer holds the + // directory between the `rename` and the next server restart. + try { + if (fs.existsSync(pluginDir)) { + fs.rmSync(pluginDir, { recursive: true, force: true }); + } + fs.renameSync(tempDir, pluginDir); + } catch (err) { + cleanupTemp(); + return reject(new Error(`Failed to move updated plugin into place: ${err.message}`)); + } + 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`)); + } + + 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..c9b00233c3 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,32 @@ 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) await startServerIfAvailable(this.getManifest(pluginName)); + throw error; + } }, async prepareRpc(pluginName: string) { validatePluginName(pluginName); 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']); +}); diff --git a/server/modules/projects/services/project-clone.service.ts b/server/modules/projects/services/project-clone.service.ts index 4211f560ea..c460838196 100644 --- a/server/modules/projects/services/project-clone.service.ts +++ b/server/modules/projects/services/project-clone.service.ts @@ -76,6 +76,64 @@ function sanitizeGitError(message: string, token: string | null): string { return message.replace(new RegExp(escapedToken, 'g'), '***'); } +/** + * Streaming wrapper around {@link sanitizeGitError} that buffers a token's + * worth of trailing characters across consecutive chunks. `git`'s `data` + * events are arbitrary byte slices, not full messages, so a credential can + * be split across two events: e.g. `https://ghp_abc` in one chunk and + * `def...@github.com/...` in the next. A naive per-chunk `replace` lets both + * fragments through. + * + * `feed(chunk)` consumes one chunk and returns the redacted portion that is + * safe to forward. Up to `token.length - 1` characters are retained as a + * "possible token prefix" until the next chunk (or the close call) + * confirms they do not form a complete token. `flush()` returns whatever + * remains in the buffer after the stream ends — redacted like any other + * output, with any unmatched prefix replaced by `***` so a half-token at + * EOF still does not leak. + */ +function createStreamingRedactor(token: string | null) { + if (!token) { + return { + feed(chunk: string): string { + return chunk; + }, + flush(): string { + return ''; + }, + }; + } + + const maxPrefix = token.length - 1; + let buffer = ''; + + return { + feed(chunk: string): string { + if (!chunk) return ''; + + buffer += chunk; + if (buffer.length <= maxPrefix) { + // Not enough characters yet for even a full token to exist; hold + // the whole buffer until the next chunk (or close) and emit nothing. + return ''; + } + + const safeEnd = buffer.length - maxPrefix; + const safeSlice = buffer.slice(0, safeEnd); + buffer = buffer.slice(safeEnd); + return sanitizeGitError(safeSlice, token); + }, + flush(): string { + if (!buffer) return ''; + // Any remaining buffer at EOF is a half-token at worst; redact it + // wholesale so no credential fragment survives the stream close. + const remainder = buffer; + buffer = ''; + return sanitizeGitError(remainder, token); + }, + }; +} + function resolveCloneFailureMessage(lastError: string, sanitizedError: string): string { if (lastError.includes('Authentication failed') || lastError.includes('could not read Username')) { return 'Authentication failed. Please check your credentials.'; @@ -241,19 +299,35 @@ export async function startCloneProject( const gitProcess = dependencies.spawnGitClone(cloneUrl, clonePath); let lastError = ''; + // Buffer up to `token.length - 1` characters across chunks so a credential + // that is split across two `data` events is still redacted before it + // reaches the SSE stream. See {@link createStreamingRedactor}. + const stdoutRedactor = createStreamingRedactor(githubToken); + const stderrRedactor = createStreamingRedactor(githubToken); + + const forwardTrimmed = (text: string) => { + const trimmed = text.trim(); + if (!trimmed) return; + handlers.onProgress(trimmed); + }; + gitProcess.stdout?.on('data', (data: Buffer | string) => { - const message = data.toString().trim(); - if (message) { - handlers.onProgress(message); - } + forwardTrimmed(stdoutRedactor.feed(data.toString())); }); gitProcess.stderr?.on('data', (data: Buffer | string) => { - const message = data.toString().trim(); - lastError = message; - if (message) { - handlers.onProgress(message); - } + const raw = data.toString(); + lastError = raw; + forwardTrimmed(stderrRedactor.feed(raw)); + }); + + // Flush any remaining buffered characters when the streams close so a + // half-token that straddles the end of the stream is also redacted. + gitProcess.stdout?.on('end', () => { + forwardTrimmed(stdoutRedactor.flush()); + }); + gitProcess.stderr?.on('end', () => { + forwardTrimmed(stderrRedactor.flush()); }); const waitForCompletion = new Promise((resolve, reject) => { diff --git a/server/modules/projects/tests/project-clone.service.test.ts b/server/modules/projects/tests/project-clone.service.test.ts index 85c807d88f..1117c17422 100644 --- a/server/modules/projects/tests/project-clone.service.test.ts +++ b/server/modules/projects/tests/project-clone.service.test.ts @@ -181,3 +181,59 @@ test('startCloneProject completes and emits complete payload when git exits succ assert.equal(resolvedCompletePayload.message, 'Repository cloned successfully'); assert.equal((resolvedCompletePayload.project.projectId as string) || '', 'project-1'); }); + +test('startCloneProject redacts GitHub tokens even when split across stream chunks', async () => { + const gitProcess = createMockGitProcess(); + const progressMessages: string[] = []; + const token = 'ghp_supersecrettoken1234567890'; + + const operation = await startCloneProject( + { + workspacePath: '/workspace/root', + githubUrl: 'https://github.com/example/repo.git', + newGithubToken: token, + userId: 1, + }, + { + onProgress: (message) => { + progressMessages.push(message); + }, + onComplete: () => undefined, + }, + buildDependencies({ + spawnGitClone: () => gitProcess as any, + }), + ); + + // Simulate `git` echoing the clone URL with the token split across two + // `data` events. A naive `replace` would let both halves through. + gitProcess.stdout.write(`Cloning into 'repo'...\nremote: Enumerating objects: 12, done.\nremote: Counting objects: 100% (12/12), done.\nremote: Total 12 (delta 0), reused 12 (delta 0), pack-reused 0\nReceiving objects: 100% (12/12), done.\nResolving deltas: 100% (0/0), done.\npost https://github.com/example/repo.git/info/refs?service=git-receive-pack token=ghp_supersecrettoke`); + gitProcess.stdout.write(`n1234567890 was 401\n`); + gitProcess.stdout.end(); + + gitProcess.stderr.write(`POST git-receive-pack: ghp_supersecrettoken12`); + gitProcess.stderr.write(`34567890 returned 401\n`); + gitProcess.stderr.end(); + + // Allow stream `end` handlers to fire. + await new Promise((resolve) => setImmediate(resolve)); + + gitProcess.emit('close', 0); + await operation.waitForCompletion; + + // No fragment of the token should reach the SSE stream. The exact-token + // replacement handles whole-token leaks; the streaming buffer handles + // cross-chunk splits. + for (const message of progressMessages) { + assert.equal( + message.includes('ghp_supersecret'), + false, + `progress message leaked token fragment: ${message}`, + ); + assert.equal( + message.includes('token1234567890'), + false, + `progress message leaked token tail: ${message}`, + ); + } +}); diff --git a/server/modules/system/system.module.ts b/server/modules/system/system.module.ts index 7eee4dd76a..01a50e7500 100644 --- a/server/modules/system/system.module.ts +++ b/server/modules/system/system.module.ts @@ -12,17 +12,28 @@ type SystemModuleOptions = { isPlatform: boolean; }; +/** + * Run a single command without involving a shell. Passing the executable and + * each argument as separate `argv` entries prevents shell metacharacter + * injection from any future caller that splices user-controlled values into + * the command path or working directory. The previous implementation used + * `sh -c `, which would let a poisoned `$PATH`, a + * hostile `appRoot`, or a future `homeDirectory` override execute arbitrary + * code with the server process's privileges. + */ function runShellCommand( command: string, + args: string[], workingDirectory: string, environment: NodeJS.ProcessEnv, onOutput: (output: string) => void, onErrorOutput: (errorOutput: string) => void, ): Promise<{ exitCode: number | null; output: string; errorOutput: string }> { return new Promise((resolve, reject) => { - const childProcess = spawn('sh', ['-c', command], { + const childProcess = spawn(command, args, { cwd: workingDirectory, env: environment, + shell: false, }); let output = ''; let errorOutput = ''; diff --git a/server/modules/system/system.service.ts b/server/modules/system/system.service.ts index fe71e3ba1a..4d20ab8564 100644 --- a/server/modules/system/system.service.ts +++ b/server/modules/system/system.service.ts @@ -12,6 +12,7 @@ type SystemUpdateDependencies = { environment: NodeJS.ProcessEnv; runShellCommand( command: string, + args: string[], workingDirectory: string, environment: NodeJS.ProcessEnv, onOutput: (output: string) => void, @@ -29,11 +30,14 @@ export function createSystemUpdateService(dependencies: SystemUpdateDependencies return { /** Selects and executes the correct update workflow for this installation. */ async updateSystem() { - const updateCommand = dependencies.isPlatform - ? 'npm run update:platform' + // Each branch passes the executable and its arguments as separate argv + // entries (no shell, no string concatenation) so no caller-controlled + // value can become a shell metacharacter. + const updatePlan = dependencies.isPlatform + ? { command: 'npm', args: ['run', 'update:platform'] } : dependencies.installMode === 'git' - ? 'git checkout main && git pull && npm install' - : 'npm install -g @cloudcli-ai/cloudcli@latest'; + ? { command: 'sh', args: ['-c', 'git checkout main && git pull && npm install'] } + : { command: 'npm', args: ['install', '-g', '@cloudcli-ai/cloudcli@latest'] }; const workingDirectory = dependencies.isPlatform || dependencies.installMode === 'git' ? dependencies.appRoot : dependencies.homeDirectory; @@ -42,7 +46,8 @@ export function createSystemUpdateService(dependencies: SystemUpdateDependencies try { const result = await dependencies.runShellCommand( - updateCommand, + updatePlan.command, + updatePlan.args, workingDirectory, dependencies.environment, (output) => dependencies.logInfo('Update output:', output), diff --git a/server/modules/system/tests/system.service.test.ts b/server/modules/system/tests/system.service.test.ts index bbf670e28c..b3590ab12d 100644 --- a/server/modules/system/tests/system.service.test.ts +++ b/server/modules/system/tests/system.service.test.ts @@ -24,8 +24,8 @@ function createDependencies( test('git installations update from the application root', async () => { const calls: unknown[][] = []; const dependencies = createDependencies({ - runShellCommand: async (command, workingDirectory, environment) => { - calls.push([command, workingDirectory, environment]); + runShellCommand: async (command, args, workingDirectory, environment) => { + calls.push([command, args, workingDirectory, environment]); return { exitCode: 0, output: 'git update complete', errorOutput: '' }; }, }); @@ -34,7 +34,8 @@ test('git installations update from the application root', async () => { const result = await service.updateSystem(); assert.deepEqual(calls, [[ - 'git checkout main && git pull && npm install', + 'sh', + ['-c', 'git checkout main && git pull && npm install'], '/app/cloudcli', dependencies.environment, ]]); @@ -49,8 +50,8 @@ test('global npm installations update from the user home directory', async () => const calls: unknown[][] = []; const dependencies = createDependencies({ installMode: 'npm', - runShellCommand: async (command, workingDirectory, environment) => { - calls.push([command, workingDirectory, environment]); + runShellCommand: async (command, args, workingDirectory, environment) => { + calls.push([command, args, workingDirectory, environment]); return { exitCode: 0, output: '', errorOutput: '' }; }, }); @@ -59,7 +60,8 @@ test('global npm installations update from the user home directory', async () => const result = await service.updateSystem(); assert.deepEqual(calls, [[ - 'npm install -g @cloudcli-ai/cloudcli@latest', + 'npm', + ['install', '-g', '@cloudcli-ai/cloudcli@latest'], '/home/cloudcli', dependencies.environment, ]]); @@ -71,8 +73,8 @@ test('platform installations use the platform workflow regardless of install mod const dependencies = createDependencies({ installMode: 'npm', isPlatform: true, - runShellCommand: async (command, workingDirectory, environment) => { - calls.push([command, workingDirectory, environment]); + runShellCommand: async (command, args, workingDirectory, environment) => { + calls.push([command, args, workingDirectory, environment]); return { exitCode: 0, output: 'platform update complete', errorOutput: '' }; }, }); @@ -81,7 +83,8 @@ test('platform installations use the platform workflow regardless of install mod await service.updateSystem(); assert.deepEqual(calls, [[ - 'npm run update:platform', + 'npm', + ['run', 'update:platform'], '/app/cloudcli', dependencies.environment, ]]);