diff --git a/src/cli.ts b/src/cli.ts index d0eb21ee..07cc0e06 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -408,29 +408,17 @@ export function runCli(BUILTIN_CLIS: string, USER_CLIS: string): void { registerAllCommands(program, siteGroups); // ── Unknown command fallback ────────────────────────────────────────────── - - const DENY_LIST = new Set([ - 'rm', 'sudo', 'dd', 'mkfs', 'fdisk', 'shutdown', 'reboot', - 'kill', 'killall', 'chmod', 'chown', 'passwd', 'su', 'mount', - 'umount', 'format', 'diskutil', - ]); + // Security: do NOT auto-discover and register arbitrary system binaries. + // Only explicitly registered external CLIs (via `opencli register`) are allowed. program.on('command:*', (operands: string[]) => { const binary = operands[0]; - if (DENY_LIST.has(binary)) { - console.error(chalk.red(`Refusing to register system command '${binary}'.`)); - process.exitCode = 1; - return; - } + console.error(chalk.red(`error: unknown command '${binary}'`)); if (isBinaryInstalled(binary)) { - console.log(chalk.cyan(`🔹 Auto-discovered local CLI '${binary}'. Registering...`)); - registerExternalCli(binary); - passthroughExternal(binary); - } else { - console.error(chalk.red(`error: unknown command '${binary}'`)); - program.outputHelp(); - process.exitCode = 1; + console.error(chalk.dim(` Tip: '${binary}' exists on your PATH. Use 'opencli register ${binary}' to add it as an external CLI.`)); } + program.outputHelp(); + process.exitCode = 1; }); program.parse(); diff --git a/src/discovery.ts b/src/discovery.ts index f95d1aef..d6c7ace9 100644 --- a/src/discovery.ts +++ b/src/discovery.ts @@ -66,12 +66,12 @@ export async function discoverClis(...dirs: string[]): Promise { const manifestPath = path.resolve(dir, '..', 'cli-manifest.json'); try { await fs.promises.access(manifestPath); - await loadFromManifest(manifestPath, dir); - continue; // Skip filesystem scan for this directory + const loaded = await loadFromManifest(manifestPath, dir); + if (loaded) continue; // Skip filesystem scan only when manifest is usable } catch { - // Fallback: runtime filesystem scan (development) - await discoverClisFromFs(dir); + // Fall through to filesystem scan } + await discoverClisFromFs(dir); } } @@ -80,7 +80,7 @@ export async function discoverClis(...dirs: string[]): Promise { * YAML pipelines are inlined — zero YAML parsing at runtime. * TS modules are deferred — loaded lazily on first execution. */ -async function loadFromManifest(manifestPath: string, clisDir: string): Promise { +async function loadFromManifest(manifestPath: string, clisDir: string): Promise { try { const raw = await fs.promises.readFile(manifestPath, 'utf-8'); const manifest = JSON.parse(raw) as ManifestEntry[]; @@ -126,8 +126,10 @@ async function loadFromManifest(manifestPath: string, clisDir: string): Promise< registerCommand(cmd); } } + return true; } catch (err) { log.warn(`Failed to load manifest ${manifestPath}: ${getErrorMessage(err)}`); + return false; } } diff --git a/src/download/index.test.ts b/src/download/index.test.ts index 56dc2b9b..6329f173 100644 --- a/src/download/index.test.ts +++ b/src/download/index.test.ts @@ -1,5 +1,29 @@ -import { describe, expect, it } from 'vitest'; -import { formatCookieHeader, resolveRedirectUrl } from './index.js'; +import * as fs from 'node:fs'; +import * as http from 'node:http'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { afterEach, describe, expect, it } from 'vitest'; +import { formatCookieHeader, httpDownload, resolveRedirectUrl } from './index.js'; + +const servers: http.Server[] = []; + +afterEach(async () => { + await Promise.all(servers.map((server) => new Promise((resolve, reject) => { + server.close((err) => (err ? reject(err) : resolve())); + }))); + servers.length = 0; +}); + +async function startServer(handler: http.RequestListener): Promise { + const server = http.createServer(handler); + servers.push(server); + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); + const address = server.address(); + if (!address || typeof address === 'string') { + throw new Error('Failed to start test server'); + } + return `http://127.0.0.1:${address.port}`; +} describe('download helpers', () => { it('resolves relative redirects against the original URL', () => { @@ -13,4 +37,23 @@ describe('download helpers', () => { { name: 'ct0', value: 'def', domain: 'example.com' }, ])).toBe('sid=abc; ct0=def'); }); + + it('fails after exceeding the redirect limit', async () => { + const baseUrl = await startServer((_req, res) => { + res.statusCode = 302; + res.setHeader('Location', '/loop'); + res.end(); + }); + + const tempDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'opencli-download-')); + const destPath = path.join(tempDir, 'file.txt'); + const result = await httpDownload(`${baseUrl}/loop`, destPath, { maxRedirects: 2 }); + + expect(result).toEqual({ + success: false, + size: 0, + error: 'Too many redirects (> 2)', + }); + expect(fs.existsSync(destPath)).toBe(false); + }); }); diff --git a/src/download/index.ts b/src/download/index.ts index 830e20db..21e4d166 100644 --- a/src/download/index.ts +++ b/src/download/index.ts @@ -17,6 +17,7 @@ export interface DownloadOptions { headers?: Record; timeout?: number; onProgress?: (received: number, total: number) => void; + maxRedirects?: number; } export interface YtdlpOptions { @@ -92,8 +93,9 @@ export async function httpDownload( url: string, destPath: string, options: DownloadOptions = {}, + redirectCount = 0, ): Promise<{ success: boolean; size: number; error?: string }> { - const { cookies, headers = {}, timeout = 30000, onProgress } = options; + const { cookies, headers = {}, timeout = 30000, onProgress, maxRedirects = 10 } = options; return new Promise((resolve) => { const parsedUrl = new URL(url); @@ -120,7 +122,16 @@ export async function httpDownload( if (response.statusCode && response.statusCode >= 300 && response.statusCode < 400 && response.headers.location) { file.close(); if (fs.existsSync(tempPath)) fs.unlinkSync(tempPath); - httpDownload(resolveRedirectUrl(url, response.headers.location), destPath, options).then(resolve); + if (redirectCount >= maxRedirects) { + resolve({ success: false, size: 0, error: `Too many redirects (> ${maxRedirects})` }); + return; + } + httpDownload( + resolveRedirectUrl(url, response.headers.location), + destPath, + options, + redirectCount + 1, + ).then(resolve); return; } diff --git a/src/engine.test.ts b/src/engine.test.ts index 8b268171..ff8066c5 100644 --- a/src/engine.test.ts +++ b/src/engine.test.ts @@ -45,6 +45,36 @@ cli({ await fs.promises.rm(tempRoot, { recursive: true, force: true }); } }); + + it('falls back to filesystem discovery when the manifest is invalid', async () => { + const tempBuildRoot = await fs.promises.mkdtemp(path.join('/tmp', 'opencli-manifest-fallback-')); + const distDir = path.join(tempBuildRoot, 'dist'); + const siteDir = path.join(distDir, 'fallback-site'); + const commandPath = path.join(siteDir, 'hello.ts'); + const manifestPath = path.join(tempBuildRoot, 'cli-manifest.json'); + + try { + await fs.promises.mkdir(siteDir, { recursive: true }); + await fs.promises.writeFile(manifestPath, '{ invalid json'); + await fs.promises.writeFile(commandPath, ` +import { cli, Strategy } from '${path.join(process.cwd(), 'src', 'registry.ts')}'; +cli({ + site: 'fallback-site', + name: 'hello', + description: 'hello command', + strategy: Strategy.PUBLIC, + browser: false, + func: async () => [{ ok: true }], +}); +`); + + await discoverClis(distDir); + + expect(getRegistry().get('fallback-site/hello')).toBeDefined(); + } finally { + await fs.promises.rm(tempBuildRoot, { recursive: true, force: true }); + } + }); }); describe('discoverPlugins', () => { diff --git a/src/external.test.ts b/src/external.test.ts index 1818b00e..8645f87f 100644 --- a/src/external.test.ts +++ b/src/external.test.ts @@ -33,6 +33,15 @@ describe('parseCommand', () => { 'Install command contains unsafe shell operators', ); }); + + it('rejects command substitution and multiline input', () => { + expect(() => parseCommand('brew install $(whoami)')).toThrow( + 'Install command contains unsafe shell operators', + ); + expect(() => parseCommand('brew install gh\nrm -rf /')).toThrow( + 'Install command contains unsafe shell operators', + ); + }); }); describe('installExternalCli', () => { diff --git a/src/external.ts b/src/external.ts index e400bf7c..46a22e80 100644 --- a/src/external.ts +++ b/src/external.ts @@ -94,7 +94,7 @@ export function getInstallCmd(installConfig?: ExternalCliInstall): string | null * Object with `binary` and `args` fields, or throws on unsafe input. */ export function parseCommand(cmd: string): { binary: string; args: string[] } { - const shellOperators = /&&|\|\|?|;|[><`]/; + const shellOperators = /&&|\|\|?|;|[><`$#\n\r]|\$\(/; if (shellOperators.test(cmd)) { throw new Error( `Install command contains unsafe shell operators and cannot be executed securely: "${cmd}". ` + diff --git a/src/pipeline/steps/fetch.ts b/src/pipeline/steps/fetch.ts index 67c53ebf..6e0b1923 100644 --- a/src/pipeline/steps/fetch.ts +++ b/src/pipeline/steps/fetch.ts @@ -72,10 +72,11 @@ async function fetchBatchInBrowser( ): Promise { const headersJs = JSON.stringify(headers); const urlsJs = JSON.stringify(urls); + const methodJs = JSON.stringify(method); return (await page.evaluate(` async () => { const urls = ${urlsJs}; - const method = "${method}"; + const method = ${methodJs}; const headers = ${headersJs}; const concurrency = ${concurrency}; diff --git a/src/pipeline/template.test.ts b/src/pipeline/template.test.ts index 266b4e99..5805ef80 100644 --- a/src/pipeline/template.test.ts +++ b/src/pipeline/template.test.ts @@ -66,6 +66,9 @@ describe('evalExpr', () => { it('evaluates method calls on values', () => { expect(evalExpr("args.username.startsWith('@') ? args.username : '@' + args.username", { args: { username: 'alice' } })).toBe('@alice'); }); + it('rejects constructor-based sandbox escapes', () => { + expect(evalExpr("args['cons' + 'tructor']['constructor']('return process')()", { args: {} })).toBeUndefined(); + }); it('applies join filter', () => { expect(evalExpr('item.tags | join(,)', { item: { tags: ['a', 'b', 'c'] } })).toBe('a,b,c'); }); diff --git a/src/pipeline/template.ts b/src/pipeline/template.ts index 786a7554..04e6770a 100644 --- a/src/pipeline/template.ts +++ b/src/pipeline/template.ts @@ -2,6 +2,8 @@ * Pipeline template engine: ${{ ... }} expression rendering. */ +import vm from 'node:vm'; + export interface RenderContext { args?: Record; data?: unknown; @@ -186,58 +188,61 @@ export function resolvePath(pathStr: string, ctx: RenderContext): unknown { /** * Evaluate arbitrary JS expressions as a last-resort fallback. - * - * ⚠️ SECURITY NOTE: Uses `new Function()` to execute the expression. - * This is acceptable here because: - * 1. YAML adapters are authored by trusted repo contributors only. - * 2. The expression runs in the same Node.js process (no sandbox). - * 3. Only a curated set of globals is exposed (no require/import/process/fs). - * If opencli ever loads untrusted third-party adapters, this MUST be replaced - * with a proper sandboxed evaluator. + * Runs inside a `node:vm` sandbox with dynamic code generation disabled. + */ +const FORBIDDEN_EXPR_PATTERNS = /\b(constructor|__proto__|prototype|globalThis|process|require|import|eval)\b/; + +/** + * Deep-copy plain data to sever prototype chains, preventing sandbox escape + * via `args.constructor.constructor('return process')()` etc. */ +function sanitizeContext(obj: unknown): unknown { + if (obj === null || obj === undefined) return obj; + if (typeof obj !== 'object' && typeof obj !== 'function') return obj; + try { + return JSON.parse(JSON.stringify(obj)); + } catch { + return {}; + } +} + function evalJsExpr(expr: string, ctx: RenderContext): unknown { // Guard against absurdly long expressions that could indicate injection. if (expr.length > 2000) return undefined; - const args = ctx.args ?? {}; - const item = ctx.item ?? {}; - const data = ctx.data; + // Block obvious sandbox escape attempts. + if (FORBIDDEN_EXPR_PATTERNS.test(expr)) return undefined; + + const args = sanitizeContext(ctx.args ?? {}); + const item = sanitizeContext(ctx.item ?? {}); + const data = sanitizeContext(ctx.data); const index = ctx.index ?? 0; try { - const fn = new Function( - 'args', - 'item', - 'data', - 'index', - 'encodeURIComponent', - 'decodeURIComponent', - 'JSON', - 'Math', - 'Number', - 'String', - 'Boolean', - 'Array', - 'Object', - 'Date', - `"use strict"; return (${expr});`, - ); - - return fn( - args, - item, - data, - index, - encodeURIComponent, - decodeURIComponent, - JSON, - Math, - Number, - String, - Boolean, - Array, - Object, - Date, + return vm.runInNewContext( + `(${expr})`, + { + args, + item, + data, + index, + encodeURIComponent, + decodeURIComponent, + JSON, + Math, + Number, + String, + Boolean, + Array, + Date, + }, + { + timeout: 50, + contextCodeGeneration: { + strings: false, + wasm: false, + }, + }, ); } catch { return undefined;