fix: harden security-sensitive execution paths

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();

src/discovery.ts

@@ -66,12 +66,12 @@ export async function discoverClis(...dirs: string[]): Promise<void> {
     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<void> {
  * 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<void> {
+async function loadFromManifest(manifestPath: string, clisDir: string): Promise<boolean> {
   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;
   }
 }
 

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<void>((resolve, reject) => {
+    server.close((err) => (err ? reject(err) : resolve()));
+  })));
+  servers.length = 0;
+});
+
+async function startServer(handler: http.RequestListener): Promise<string> {
+  const server = http.createServer(handler);
+  servers.push(server);
+  await new Promise<void>((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);
+  });
 });

src/download/index.ts

@@ -17,6 +17,7 @@ export interface DownloadOptions {
   headers?: Record<string, string>;
   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;
       }
 

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', () => {

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', () => {

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}". ` +

src/pipeline/steps/fetch.ts

@@ -72,10 +72,11 @@ async function fetchBatchInBrowser(
 ): Promise<unknown[]> {
   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};
 

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');
   });

src/pipeline/template.ts

@@ -2,6 +2,8 @@
  * Pipeline template engine: ${{ ... }} expression rendering.
  */
 
+import vm from 'node:vm';
+
 export interface RenderContext {
   args?: Record<string, unknown>;
   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;