Skip to content

Fix(sandbox): repair Worker→Host message routing and add storage backend#1991

Merged
kaizhou-lab merged 2 commits intomainfrom
fix/sandbox-worker-host-bugs
Mar 31, 2026
Merged

Fix(sandbox): repair Worker→Host message routing and add storage backend#1991
kaizhou-lab merged 2 commits intomainfrom
fix/sandbox-worker-host-bugs

Conversation

@TCP404
Copy link
Copy Markdown
Collaborator

@TCP404 TCP404 commented Mar 31, 2026

Summary

Closes #1990

  • Fix SandboxHost.handleMessage() to handle api-call messages from Worker, routing them to a generic apiHandlers map and sending api-response back — previously these were silently dropped, causing aion.storage.* calls to hang forever
  • Fix case 'event' to forward ext:* events to extensionEventBus and ui-message to an onUIMessage callback — previously all extension events were silently discarded
  • Add 30s timeout to callMainThread() in sandboxWorker.ts so Worker→Host calls fail with a clear error instead of hanging indefinitely
  • Add ExtensionStorage: JSON-file-based per-extension KV store backing the aion.storage API (not yet wired — pending ChannelPlugin/Lifecycle migration to SandboxHost)
  • Add TODO markers in ChannelPluginResolver and lifecycle.ts for future migration from main-process eval('require') to Worker Thread sandbox
  • Add extension market research docs (security model, sandbox architecture diagrams, gap analysis)

Test plan

  • bunx tsc --noEmit — zero errors
  • bun run lint — zero errors
  • bun run test — 2226 passed, 0 failed
  • New tests: 23 cases covering SandboxHost message routing (api-call, event, api-response) and ExtensionStorage (get/set/delete, isolation, corruption recovery, createApiHandlers)

- Add 'api-call' case in SandboxHost.handleMessage() so Worker→Host RPC
  (e.g. aion.storage.get/set/delete) is routed to apiHandlers instead of
  being silently dropped (Bug #4)
- Forward 'event' messages to extensionEventBus (ext:* prefix) and
  onUIMessage callback (ui-message), fixing silent discard (Bug #5)
- Add 30s timeout to callMainThread() in sandboxWorker to prevent
  indefinite hangs when Host fails to respond
- Introduce ExtensionStorage: JSON-file-based per-extension KV store
  that backs the aion.storage API (not yet wired — pending ChannelPlugin
  migration to SandboxHost)
- Add TODO markers in ChannelPluginResolver and lifecycle.ts for future
  migration from main-process eval('require') to Worker Thread sandbox
- Add extension market research docs (security model, architecture,
  sandbox architecture diagrams, gap analysis)
@piorpua piorpua added the bot:reviewing Review in progress (mutex) label Mar 31, 2026
@piorpua
Copy link
Copy Markdown
Contributor

piorpua commented Mar 31, 2026

Code Review:Fix(sandbox): repair Worker→Host message routing and add storage backend (#1991)

变更概述

本 PR 修复了 SandboxHost 消息路由中的两个关键 bug:Worker 发出的 api-call 消息被静默丢弃(导致 aion.storage.* 调用永久挂起),以及 event 类型消息未被正确转发到 extensionEventBus 和 UI 回调。同时为 Worker→Host 调用添加了 30s 超时机制,新增了 ExtensionStorage 作为 aion.storage API 的后端存储实现(尚未接入),并添加了扩展市场调研文档。


方案评估

结论:✅ 方案合理

PR 正确地在 handleMessage 中补全了 api-callevent 的路由逻辑,通过 apiHandlers map 和 onUIMessage 回调实现了清晰的关注点分离。ExtensionStorage 采用 JSON 文件 + 内存缓存的方式实现 per-extension KV 存储,对于小型配置数据合理且简单。callMainThread 增加超时避免无限挂起,与 Host 端的 callTimeout 对称。整体方案与项目现有的 Sandbox 架构一致。


问题清单

🟡 MEDIUM — ExtensionStorage 的 set/delete 直接变异缓存对象

文件src/process/extensions/sandbox/ExtensionStorage.ts,第 75-84 行

问题代码

async set(extensionName: string, key: string, value: unknown): Promise<void> {
    const data = this.loadData(extensionName);
    data[key] = value;
    this.saveData(extensionName, data);
}

async delete(extensionName: string, key: string): Promise<void> {
    const data = this.loadData(extensionName);
    delete data[key];
    this.saveData(extensionName, data);
}

问题说明loadData 返回缓存中的引用,setdelete 直接对该引用进行变异(data[key] = value / delete data[key]),违反项目不可变性原则。虽然当前为类内部状态管理,不会泄露到外部,但如果未来扩展(如添加 getAll() 方法),可能导致外部持有的引用被意外修改。

修复建议

async set(extensionName: string, key: string, value: unknown): Promise<void> {
    const data = this.loadData(extensionName);
    const updated = { ...data, [key]: value };
    this.saveData(extensionName, updated);
}

async delete(extensionName: string, key: string): Promise<void> {
    const data = this.loadData(extensionName);
    const { [key]: _, ...rest } = data;
    this.saveData(extensionName, rest);
}

🟡 MEDIUM — ExtensionStorage 文件路径未校验 extensionName

文件src/process/extensions/sandbox/ExtensionStorage.ts,第 102-104 行

问题代码

private getFilePath(extensionName: string): string {
    return path.join(this.storageDir, `${extensionName}.json`);
}

问题说明:如果 extensionName 包含路径遍历字符(如 ../../etc/malicious),生成的文件路径将超出 storageDir 范围。虽然当前 extensionName 由内部系统控制且该类尚未接入,但项目其他位置(lifecycle.tsChannelPluginResolver.ts)均使用了 isPathWithinDirectory 进行路径校验,此处应保持一致的防御性编程。

修复建议

import { isPathWithinDirectory } from './pathSafety';

private getFilePath(extensionName: string): string {
    const filePath = path.join(this.storageDir, `${extensionName}.json`);
    if (!isPathWithinDirectory(filePath, this.storageDir)) {
      throw new Error(`[ExtensionStorage] Invalid extension name: "${extensionName}"`);
    }
    return filePath;
}

🔵 LOW — loadData 异常时静默丢弃错误

文件src/process/extensions/sandbox/ExtensionStorage.ts,第 116 行

问题代码

} catch {
    // File doesn't exist or is corrupted — start fresh
    const empty: Record<string, unknown> = {};
    this.cache.set(extensionName, empty);
    return empty;
}

问题说明:当 JSON 文件损坏时,catch 块静默重置数据而不记录任何日志。文件不存在(ENOENT)不需要日志,但 JSON 解析失败意味着数据丢失,应记录警告以帮助排查问题。

修复建议

} catch (error) {
    // ENOENT is expected for new extensions; log corruption for debugging
    if (error instanceof Error && !error.message.includes('ENOENT')) {
      console.warn(`[ExtensionStorage] Corrupted data for "${extensionName}", starting fresh:`, error.message);
    }
    const empty: Record<string, unknown> = {};
    this.cache.set(extensionName, empty);
    return empty;
}

汇总

# 严重级别 文件 问题
1 🟡 MEDIUM ExtensionStorage.ts:75-84 set/delete 直接变异缓存对象,违反不可变性原则
2 🟡 MEDIUM ExtensionStorage.ts:102-104 getFilePath 未校验 extensionName,存在路径遍历风险
3 🔵 LOW ExtensionStorage.ts:116 JSON 损坏时静默丢弃错误,缺少日志

结论

⚠️ 有条件批准 — 存在两个 MEDIUM 级别问题(不可变性违反和路径校验缺失),均在 ExtensionStorage 中,建议修复后合并。核心 bug 修复(消息路由、超时机制)质量良好,测试覆盖充分(23 个测试用例)。


本报告由本地 pr-review skill 生成,包含完整项目上下文,无截断限制。

CONCLUSION: CONDITIONAL
IS_CRITICAL_PATH: true
PR_NUMBER: 1991

@piorpua piorpua added bot:ready-to-fix CONDITIONAL review done, waiting for bot fix bot:fixing Fix in progress (mutex) and removed bot:reviewing Review in progress (mutex) bot:ready-to-fix CONDITIONAL review done, waiting for bot fix labels Mar 31, 2026
- Use immutable patterns in ExtensionStorage set/delete methods
- Add path traversal validation in getFilePath using isPathWithinDirectory

Review follow-up for #1991
@piorpua
Copy link
Copy Markdown
Contributor

piorpua commented Mar 31, 2026

PR Fix 验证报告

原始 PR: #1991
修复方式: 直接推送到 fix/sandbox-worker-host-bugs

# 严重级别 文件 问题 修复方式 状态
1 🟡 MEDIUM ExtensionStorage.ts:75-84 set/delete 直接变异缓存对象 使用 spread 创建新对象替代直接变异 ✅ 已修复
2 🟡 MEDIUM ExtensionStorage.ts:102-104 getFilePath 未校验 extensionName 添加 isPathWithinDirectory 路径遍历校验 ✅ 已修复

总结: ✅ 已修复 2 个 | ❌ 未能修复 0 个

🔵 LOW 级别问题已跳过(不阻塞合并,修复优先级低)。

@piorpua piorpua added bot:ready-to-merge Bot done, code is clean — human just needs to confirm and merge and removed bot:fixing Fix in progress (mutex) labels Mar 31, 2026
@kaizhou-lab kaizhou-lab merged commit c54d2d0 into main Mar 31, 2026
21 checks passed
@kaizhou-lab kaizhou-lab deleted the fix/sandbox-worker-host-bugs branch March 31, 2026 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:ready-to-merge Bot done, code is clean — human just needs to confirm and merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(sandbox): Worker→Host message routing broken — storage hangs and events silently dropped

3 participants