byy/qiming

Fork 0

Files

Codex bfd67a0f2c chore: initialize qiming workspace repository

2026-05-29 14:22:48 +08:00

16 KiB

Raw Blame History

沙箱实现代码审查报告

审查日期: 2026-03-22 审查者: Claude Code 版本: 1.0.0

1. 总体评价

评分: 7.5 / 10

优点

类型定义完整，覆盖了沙箱、工作区、权限、执行结果等核心概念
错误处理体系完善，有专门的错误类和错误码
抽象基类设计合理，支持多种沙箱类型（Docker、WSL、Firejail）
权限管理实现了分层策略（自动批准、需要确认、禁止）
事件驱动架构，支持状态监听

不足

部分安全检查存在漏洞
缺少持久化存储实现
代码注释不完整
部分边界情况未处理
测试覆盖未知

2. 各文件详细评价

2.1 `src/shared/types/sandbox.ts` - 类型定义

评分: 8.5/10

优点

类型定义非常完整，覆盖了所有核心概念
JSDoc 注释详细
支持 Harness 架构的 CP 工作流概念（Checkpoint、GateStatus）
事件类型定义完整（SandboxEvents 接口）

问题

严重程度	问题描述	位置
🟡 中	`PermissionLevel` 类型定义了 0-3 级别，但代码中未使用	L34
🟡 中	`WorkflowState.workspaces` 使用 `Record<string, WorkspaceRecord>` 但 `Workspace` 对象更完整	L367
🟢 低	`FileInfo.permissions` 是可选的字符串，缺少类型定义	L213

建议

// 建议: 添加 PermissionLevel 的使用场景说明或移除
// 建议: FileInfo.permissions 使用更结构化的类型
export interface FilePermissions {
  read: boolean;
  write: boolean;
  execute: boolean;
}

2.2 `src/shared/errors/sandbox.ts` - 错误类

评分: 8/10

优点

错误码枚举完整，覆盖所有场景
错误类层次结构清晰（基类 SandboxError + 特化类）
getUserMessage() 提供用户友好的错误信息
isRecoverable() 和 requiresUserIntervention() 有助于错误处理决策

问题

严重程度	问题描述	位置
🔴 高	`toSandboxError` 可能丢失原始错误的堆栈信息	L407-417
🟡 中	`cause` 属性在 `toJSON` 中只序列化 message，可能丢失重要信息	L146
🟡 中	`PermissionError` 硬编码了 `PERMISSION_DENIED`，无法使用其他权限相关错误码	L313-316

建议

// 建议: 保留完整的错误链
if (error instanceof Error) {
  const sandboxError = new SandboxError(error.message || defaultMessage, defaultCode, {
    ...options,
    cause: error,
  });
  // 保留原始堆栈
  sandboxError.stack = error.stack;
  return sandboxError;
}

2.3 `src/main/services/sandbox/SandboxManager.ts` - 基类

评分: 7.5/10

优点

抽象方法定义清晰，子类必须实现
提供了实用的静态工具方法（parseMemoryLimit, formatMemory）
工作区验证逻辑完善
支持事件发射

问题

严重程度	问题描述	位置
🔴 高	`normalizePath` 使用 `toLowerCase()` 可能导致路径比较问题（不区分大小写的文件系统）	L240-241
🟡 中	`destroy()` 方法捕获错误后使用 `console.error` 而非 `log`	L281
🟡 中	`generateWorkspaceId` 仅使用时间戳，高并发下可能冲突	L247-249
🟢 低	`emitEvent` 的泛型参数 `T extends string` 未提供实际类型约束	L266

建议

// 建议: 使用 UUID 或更可靠的 ID 生成方式
protected generateWorkspaceId(sessionId: string): string {
  const { randomUUID } = require('crypto');
  return `workspace-${sessionId}-${randomUUID().slice(0, 8)}`;
}

// 建议: 路径规范化不应使用 toLowerCase
protected normalizePath(path: string): string {
  return path.replace(/\\/g, "/").replace(/\/+/g, "/");
}

2.4 `src/main/services/sandbox/DockerSandbox.ts` - Docker 实现

评分: 7/10

优点

完整的 Docker 生命周期管理
资源限制配置（内存、CPU）
失败时自动清理资源
超时处理完善

问题

严重程度	问题描述	位置
🔴 高	命令注入漏洞: `docker image inspect ${image}` 未对镜像名进行转义	L142
🔴 高	命令注入漏洞: `docker pull ${image}` 同样存在问题	L147
🔴 高	`startContainer` 中 `args.join(" ")` 构建命令字符串，参数未转义	L672
🟡 中	`execAsync` 超时可能导致僵尸进程	多处
🟡 中	`readFile`/`writeFile` 直接操作主机文件系统，未通过容器	L361-408
🟡 中	`getDirectorySize` 递归计算大目录可能很慢	L794-813
🟢 低	`tail -f /dev/null` 作为容器保持运行的方式不够优雅	L667-669

建议

// 建议: 使用 spawn 并正确传递参数，避免命令注入
private async ensureImageExists(): Promise<void> {
  const image = this.dockerConfig.dockerImage;

  // 验证镜像名格式
  if (!/^[a-zA-Z0-9._/:-]+$/.test(image)) {
    throw new SandboxError("无效的镜像名称", SandboxErrorCode.CONFIG_INVALID);
  }

  try {
    await execAsync(`docker image inspect`, [image], { timeout: 10000 });
  } catch {
    await execAsync(`docker pull`, [image], { timeout: 300000 });
  }
}

// 建议: 使用 spawn 替代 exec 构建命令
private async startContainer(...): Promise<string> {
  const args = ["run", "-d", "--name", containerName, ...];
  const proc = spawn("docker", args);
  // ...
}

2.5 `src/main/services/sandbox/PermissionManager.ts` - 权限管理

评分: 7/10

优点

分层权限策略（自动批准、需要确认、禁止）
安全命令白名单
危险命令黑名单检测
权限缓存机制

问题

严重程度	问题描述	位置
🔴 高	`checkDangerousOperation` 使用 `includes` 检测，可被绕过（如 `sudo\n` 或 `sud o`）	L628-638
🔴 高	危险命令检测对大小写敏感，`SUDO` 或 `SuDo` 可绕过	L628-638
🟡 中	`requestPermission` 超时后 pendingRequests 中的请求未清理（虽然 delete 了，但监听器还在）	L305-329
🟡 中	敏感路径检测逻辑错误: `!lowerTarget.startsWith("/etc/")` 永远为 true	L651
🟡 中	缓存键包含完整 target，大路径可能导致内存问题	L606-611
🟢 低	60 秒超时硬编码	L313

建议

// 建议: 增强危险命令检测
private checkDangerousOperation(
  type: PermissionType,
  target: string,
): { isDangerous: boolean; reason?: string } {
  // 规范化: 移除空白字符，转小写
  const normalizedTarget = target.toLowerCase().replace(/\s+/g, ' ').trim();

  for (const dangerous of DANGEROUS_COMMANDS) {
    // 使用更严格的匹配
    const pattern = new RegExp(`\\b${escapeRegex(dangerous.toLowerCase())}\\b`);
    if (pattern.test(normalizedTarget)) {
      return {
        isDangerous: true,
        reason: `检测到危险操作: ${dangerous}`,
      };
    }
  }

  // 修复敏感路径检测
  if (this.isPathPermission(type)) {
    if (normalizedTarget.includes('/.ssh') || normalizedTarget.includes('\\.ssh')) {
      return { isDangerous: true, reason: "禁止访问 SSH 目录" };
    }
    if (normalizedTarget.startsWith('/etc/')) {
      return { isDangerous: true, reason: "禁止访问系统配置目录" };
    }
  }

  return { isDangerous: false };
}

// 建议: 修复超时后的清理
const timeout = setTimeout(() => {
  this.pendingRequests.delete(request.id);
  this.removeAllListeners(`permission:${request.id}`); // 清理监听器
  reject(...);
}, 60000);

2.6 `src/main/services/sandbox/WorkspaceManager.ts` - 工作区管理

评分: 8/10

优点

整合 SandboxManager 和 PermissionManager
统一的工作区生命周期管理
权限检查与文件操作集成
保留策略支持

问题

严重程度	问题描述	位置
🟡 中	`cleanupExpired` 中工作区数量检查逻辑可能导致多次清理同一工作区	L399-410
🟡 中	`validatePathInWorkspace` 重复实现了基类的方法	L536-550
🟡 中	`destroy` 方法的 `force` 参数在错误时只记录日志，不抛出异常，可能隐藏问题	L167-181
🟢 低	`isPathPermission` 重复定义（PermissionManager 中已有）	L529-531

建议

// 建议: 优化工作区数量检查逻辑
if (policy.maxWorkspaces && workspaces.length > policy.maxWorkspaces) {
  const sorted = [...workspaces].sort(
    (a, b) => a.lastAccessedAt.getTime() - b.lastAccessedAt.getTime(),
  );
  const toRemove = sorted.slice(0, workspaces.length - policy.maxWorkspaces);
  const idsToRemove = new Set(toRemove.map(w => w.id));

  if (idsToRemove.has(workspace.id)) {
    shouldCleanup = true;
  }
}

// 建议: 复用基类的路径验证方法
private validatePathInWorkspace(workspace: Workspace, path: string): void {
  // 直接调用 SandboxManager 的方法
  // 或提取为共享工具函数
}

2.7 `src/main/ipc/sandboxHandlers.ts` - IPC 通道

评分: 8/10

优点

服务注入模式，解耦良好
统一的错误处理
完整的 JSDoc 注释
IPC 通道命名规范

问题

严重程度	问题描述	位置
🟡 中	服务类型定义使用 `typeof sandboxService`，但初始值为 `null`，类型推断可能不准确	L31-50
🟡 中	`handleError` 返回的 `code` 字段类型不明确（`code?: string`）	L118
🟡 中	缺少 IPC 通道注销函数	-
🟢 低	日志记录了敏感信息（如完整路径）	L264 等

建议

// 建议: 使用接口定义服务类型
interface SandboxService {
  createWorkspace(sessionId: string, options?: CreateWorkspaceOptions): Promise<Workspace>;
  destroyWorkspace(sessionId: string): Promise<void>;
  // ...
}

let sandboxService: SandboxService | null = null;

// 建议: 添加注销函数
export function unregisterSandboxHandlers(): void {
  ipcMain.removeHandler("sandbox:create");
  ipcMain.removeHandler("sandbox:destroy");
  // ...
}

// 建议: 错误码使用枚举类型
function handleError(error: unknown, operation: string): {
  success: false;
  error: string;
  code?: SandboxErrorCode;
} {
  // ...
}

3. 发现的问题汇总

3.1 安全漏洞 (🔴 高优先级)

#	问题	文件	风险
1	Docker 命令注入漏洞	DockerSandbox.ts	攻击者可通过构造镜像名执行任意命令
2	危险命令检测可绕过	PermissionManager.ts	大小写、空白字符可绕过检测
3	敏感路径检测逻辑错误	PermissionManager.ts	`/etc/` 路径检测失效

3.2 功能缺陷 (🟡 中优先级)

#	问题	文件	影响
4	路径规范化使用 toLowerCase	SandboxManager.ts	可能导致路径匹配错误
5	权限请求超时后监听器泄漏	PermissionManager.ts	内存泄漏
6	工作区 ID 生成可能冲突	SandboxManager.ts	高并发下可能产生重复 ID
7	文件操作未通过容器	DockerSandbox.ts	可能绕过沙箱隔离

3.3 代码质量 (🟢 低优先级)

#	问题	文件	影响
8	重复代码（路径验证、isPathPermission）	多文件	维护成本
9	硬编码超时值	多文件	可配置性差
10	缺少持久化存储	全局	应用重启后状态丢失

4. 改进建议

4.1 架构层面

添加持久化层
- 使用 SQLite 存储工作区状态
- 记录权限审批历史
- 支持应用重启后恢复
添加 WSL 和 Firejail 实现
- 当前只有 DockerSandbox 实现
- WSL 实现 Windows 平台支持
- Firejail 实现 Linux 轻量级沙箱
添加指标收集
- 执行时间统计
- 资源使用监控
- 错误率追踪

4.2 安全层面

增强命令验证

// 使用白名单 + 参数解析
const ALLOWED_COMMANDS = new Set(['node', 'npm', 'git', ...]);
const parsed = parseCommand(command);
if (!ALLOWED_COMMANDS.has(parsed.name)) {
  throw new PermissionError(...);
}

路径遍历防护

// 解析并验证真实路径
const realPath = fs.realpathSync(path);
if (!realPath.startsWith(workspaceRoot)) {
  throw new PermissionError(...);
}

资源限制

// 添加磁盘配额检查
async checkDiskQuota(workspace: Workspace): Promise<boolean> {
  const usage = await this.getDiskUsage(workspace.rootPath);
  return usage < parseSize(workspace.sandboxConfig.diskQuota || '10g');
}

4.3 代码层面

统一日志

// 替换所有 console.error 为 log.error

提取共享工具

// 创建 src/shared/utils/pathUtils.ts
export function normalizePath(path: string): string { ... }
export function isPathPermission(type: PermissionType): boolean { ... }

添加配置常量

// src/shared/constants/sandbox.ts
export const PERMISSION_TIMEOUT = 60000;
export const DEFAULT_MEMORY_LIMIT = '2g';
export const WORKSPACE_ID_PREFIX = 'workspace-';

5. 优先修复项

P0 - 立即修复（安全漏洞）

修复 Docker 命令注入
- 文件: DockerSandbox.ts
- 使用 spawn 替代字符串拼接
- 添加镜像名验证
修复危险命令检测绕过
- 文件: PermissionManager.ts
- 规范化命令（去除空白、转小写）
- 使用正则表达式边界匹配
修复敏感路径检测
- 文件: PermissionManager.ts
- 修复 /etc/ 路径检测逻辑

P1 - 尽快修复（功能缺陷）

修复路径规范化
- 文件: SandboxManager.ts
- 移除 toLowerCase() 调用
修复监听器泄漏
- 文件: PermissionManager.ts
- 超时后移除事件监听器
改进工作区 ID 生成
- 文件: SandboxManager.ts
- 使用 UUID 或加密随机数

P2 - 计划修复（代码质量）

提取共享工具函数
统一日志使用
添加配置常量
添加单元测试

6. 与 Harness 架构对齐检查

检查项	状态	说明
CP1 任务确认	✅	有参数验证
CP2 规划分解	⚠️	部分实现，缺少显式的规划阶段
CP3 执行实现	✅	完整实现
CP4 质量门禁	⚠️	类型定义存在，但未实现门禁逻辑
CP5 审查完成	⚠️	有 metrics 类型，但未实现收集

建议: 在 WorkspaceManager 中添加显式的 CP 工作流支持，包括：

门禁检查机制
指标收集
状态持久化

7. 测试建议

7.1 单元测试

// 测试危险命令检测
describe('PermissionManager.checkDangerousOperation', () => {
  it('should detect dangerous commands regardless of case', () => {
    expect(pm.checkDangerousOperation('command:execute', 'SUDO rm -rf /')).toBe(true);
    expect(pm.checkDangerousOperation('command:execute', 'sUdO ls')).toBe(true);
  });

  it('should detect commands with extra whitespace', () => {
    expect(pm.checkDangerousOperation('command:execute', 'sudo  rm -rf /')).toBe(true);
  });
});

// 测试路径验证
describe('SandboxManager.validatePathInWorkspace', () => {
  it('should reject paths outside workspace', () => {
    expect(() => sm.validatePathInWorkspace(workspace, '/etc/passwd')).toThrow();
  });

  it('should handle path traversal attempts', () => {
    expect(() => sm.validatePathInWorkspace(workspace, '../etc/passwd')).toThrow();
  });
});

7.2 集成测试

describe('DockerSandbox Integration', () => {
  it('should create and destroy workspace', async () => {
    const sandbox = new DockerSandbox(config);
    await sandbox.init();
    const workspace = await sandbox.createWorkspace('test-session');
    expect(workspace).toBeDefined();
    await sandbox.destroyWorkspace('test-session');
  });

  it('should execute commands in container', async () => {
    const result = await sandbox.execute('test-session', 'echo', ['hello']);
    expect(result.stdout.trim()).toBe('hello');
  });
});

8. 结论

沙箱实现代码整体架构合理，类型定义完整，错误处理得当。但存在若干安全漏洞需要立即修复，特别是命令注入和危险命令检测绕过问题。建议按优先级修复问题，并补充单元测试和集成测试。

下一步行动:

修复 P0 安全漏洞
添加单元测试
实现 WSL/Firejail 支持
添加持久化存储

16 KiB Raw Blame History Unescape Escape

沙箱实现代码审查报告

1. 总体评价

评分: 7.5 / 10

优点

不足

2. 各文件详细评价

2.1 src/shared/types/sandbox.ts - 类型定义

优点

问题

建议

2.2 src/shared/errors/sandbox.ts - 错误类

优点

问题

建议

2.3 src/main/services/sandbox/SandboxManager.ts - 基类

优点

问题

建议

2.4 src/main/services/sandbox/DockerSandbox.ts - Docker 实现

优点

问题

建议

2.5 src/main/services/sandbox/PermissionManager.ts - 权限管理

优点

问题

建议

2.6 src/main/services/sandbox/WorkspaceManager.ts - 工作区管理

优点

问题

建议

2.7 src/main/ipc/sandboxHandlers.ts - IPC 通道

优点

问题

建议

3. 发现的问题汇总

3.1 安全漏洞 (🔴 高优先级)

3.2 功能缺陷 (🟡 中优先级)

3.3 代码质量 (🟢 低优先级)

4. 改进建议

4.1 架构层面

4.2 安全层面

4.3 代码层面

5. 优先修复项

P0 - 立即修复（安全漏洞）

P1 - 尽快修复（功能缺陷）

P2 - 计划修复（代码质量）

6. 与 Harness 架构对齐检查

7. 测试建议

7.1 单元测试

7.2 集成测试

8. 结论

16 KiB

Raw Blame History

2.1 `src/shared/types/sandbox.ts` - 类型定义

2.2 `src/shared/errors/sandbox.ts` - 错误类

2.3 `src/main/services/sandbox/SandboxManager.ts` - 基类

2.4 `src/main/services/sandbox/DockerSandbox.ts` - Docker 实现

2.5 `src/main/services/sandbox/PermissionManager.ts` - 权限管理

2.6 `src/main/services/sandbox/WorkspaceManager.ts` - 工作区管理

2.7 `src/main/ipc/sandboxHandlers.ts` - IPC 通道