Skip to content

fix(extensions): gate aion.storage behind manifest storage permission#1803

Merged
piorpua merged 4 commits intoiOfficeAI:mainfrom
amanharshx:fix/gate-storage-permission-v2
Apr 2, 2026
Merged

fix(extensions): gate aion.storage behind manifest storage permission#1803
piorpua merged 4 commits intoiOfficeAI:mainfrom
amanharshx:fix/gate-storage-permission-v2

Conversation

@amanharshx
Copy link
Copy Markdown
Contributor

@amanharshx amanharshx commented Mar 27, 2026

Closes #1804

Summary

  • Gate aion.storage behind the manifest storage permission so extensions without "storage": true never get the API
  • Add host-side defense-in-depth: SandboxHost.handleMessage rejects storage.* API calls when permission is not granted
  • Use optional chaining (permissions?.storage) to avoid crashing workers for extensions that omit the permissions block entirely

Motivation

The worker comment said "Storage API (if permission granted)", but aion.storage was unconditionally exposed on the proxy and always proxied to the main thread regardless of the manifest declaration. An extension with "storage": false (or no permissions block) could call aion.storage.get/set/delete without restriction.

Files changed

  • src/process/extensions/sandbox/permissions.ts — added hasSandboxStoragePermission and getSandboxPermissionDeniedError helpers
  • src/process/extensions/sandbox/sandboxWorker.ts — conditionally spread storage on the proxy based on permission check
  • src/process/extensions/sandbox/sandbox.ts — reject storage.* API calls in handleMessage when permission is off
  • tests/unit/extensions/sandboxPermission.test.ts — new tests covering denied, granted, and happy-path scenarios

Diff

+78 −16 across 4 files

Testing

  • bunx tsc --noEmit — no type errors
  • bun run lint — clean
  • bun run test -- tests/unit/extensions/ — 9 files, 29 tests pass

Risks / Side effects

  • Extensions that were (incorrectly) relying on aion.storage without declaring "storage": true will now get undefined — this is the intended fix
  • Host-side storage handler does not exist yet (pre-existing gap) — the new api-call case documents this with a comment

@sentry
Copy link
Copy Markdown

sentry bot commented Mar 27, 2026

Codecov Report

❌ Patch coverage is 66.66667% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/process/extensions/sandbox/sandboxWorker.ts 0.00% 4 Missing ⚠️

📢 Thoughts on this report? Let us know!

@amanharshx
Copy link
Copy Markdown
Contributor Author

The 9 uncovered lines are in handleMessage (private method, tested via type-cast access in sandboxPermission.test.ts) and sandboxWorker.ts (Worker Thread entry point — not directly importable by Vitest). The actual gating logic is covered through createSandboxAionProxy and the SandboxHost.handleMessage rejection tests

@piorpua piorpua added the bot:reviewing Review in progress (mutex) label Mar 29, 2026
@piorpua
Copy link
Copy Markdown
Contributor

piorpua commented Mar 29, 2026

CI 检查未通过

以下 job 在本次自动化 review 时未通过,请修复:

Job 结论
Unit Tests (windows-2022) ❌ CANCELLED

本次自动化 review 暂缓,待 CI 全部通过后将重新处理。

@piorpua piorpua added bot:reviewing Review in progress (mutex) and removed bot:reviewing Review in progress (mutex) labels Mar 29, 2026
@piorpua
Copy link
Copy Markdown
Contributor

piorpua commented Mar 29, 2026

CI 检查未通过

以下 job 在本次自动化 review 时未通过,请修复:

Job 结论
Unit Tests (ubuntu-latest) ❌ FAILURE

本次自动化 review 暂缓,待 CI 全部通过后将重新处理。

@piorpua piorpua added bot:ci-waiting CI failed and author notified — snoozed until new commits are pushed bot:reviewing Review in progress (mutex) and removed bot:reviewing Review in progress (mutex) bot:ci-waiting CI failed and author notified — snoozed until new commits are pushed labels Mar 29, 2026
@piorpua piorpua added bot:ci-waiting CI failed and author notified — snoozed until new commits are pushed bot:reviewing Review in progress (mutex) and removed bot:reviewing Review in progress (mutex) bot:ci-waiting CI failed and author notified — snoozed until new commits are pushed labels Mar 31, 2026
@piorpua
Copy link
Copy Markdown
Contributor

piorpua commented Apr 2, 2026

合并冲突(无法自动解决)

本 PR 与目标分支存在冲突,自动 rebase 未能干净解决。请手动 rebase 后重新 push:

git fetch origin
git rebase origin/main
# 解决冲突后
git push --force-with-lease

@piorpua piorpua added bot:needs-human-review Blocking issues — human must intervene and removed bot:reviewing Review in progress (mutex) labels Apr 2, 2026
@amanharshx amanharshx force-pushed the fix/gate-storage-permission-v2 branch from 628f253 to ccd9ff2 Compare April 2, 2026 04:43
@amanharshx
Copy link
Copy Markdown
Contributor Author

@piorpua Thanks for the reminder. I rebased this branch onto main, resolved the sandbox conflict, and force-pushed the updated branch.

@piorpua piorpua added bot:reviewing Review in progress (mutex) and removed bot:needs-human-review Blocking issues — human must intervene labels Apr 2, 2026
@piorpua
Copy link
Copy Markdown
Contributor

piorpua commented Apr 2, 2026

Code Review:fix(extensions): gate aion.storage behind manifest storage permission (#1803)

变更概述

本 PR 修复了 aion.storage API 无论 manifest 是否声明 storage: true 均无条件暴露给插件的安全漏洞。变更分为两层防御:Worker 端在代理对象构建时按权限条件展开 storage 字段;Host 端在 handleWorkerApiCall 中增加前置权限校验,拒绝未授权的 storage.* 调用。涉及 permissions.tssandbox.tssandboxWorker.ts 及对应测试文件。


方案评估

结论:✅ 方案合理

双层防御(Worker 侧 + Host 侧)设计合理:Worker 侧在代理构建时提前剔除 API,Host 侧作为纵深防御在消息路由前拦截。两层检查均复用同一 hasSandboxStoragePermission 辅助函数,逻辑统一不重复。方案正确解决了 PR 描述的问题,无过度设计,与已有 sandbox 架构完全一致。


问题清单

🟡 MEDIUM — 测试用例重复:sandboxPermission.test.ts 与 sandboxHost.test.ts 中的权限测试完全相同

文件tests/unit/extensions/sandboxPermission.test.ts(全文)和 tests/unit/extensions/sandboxHost.test.ts,第 42–68 行

问题代码(两处相同的 describe 块):

// 出现在 sandboxHost.test.ts(新增)和 sandboxPermission.test.ts(新文件)中
describe('extensions/sandbox permissions', () => {
  it('treats omitted permissions as no storage access', ...)
  it('treats storage permission false as no storage access', ...)
  it('treats storage permission true as storage access enabled', ...)
  it('returns a permission error for storage api calls without storage permission', ...)
  it('does not return a permission error for non-storage api calls without storage permission', ...)
  it('does not return a permission error for storage api calls when storage permission is granted', ...)
});

问题说明:6 个测试用例(describe 名称、it 名称、断言)在两个文件中完全一致,属于复制粘贴遗漏清理。同一测试跑两次既浪费 CI 时间,也会在未来修改时产生混淆(改一处忘改另一处)。

修复建议:删除 sandboxHost.test.ts 中新增的 describe('extensions/sandbox permissions', ...) 块(第 42–68 行),保留 sandboxPermission.test.ts 作为该模块的独立测试文件即可。


🔵 LOW — sandboxWorker.ts 中 config.permissions 类型为 Record<string, unknown>,绕过了 ExtPermissions 的类型约束

文件src/process/extensions/sandbox/sandboxWorker.ts,第 39–44 行

问题代码

const config = workerData as {
  extensionName: string;
  extensionDir: string;
  entryPoint: string;
  permissions: Record<string, unknown>;  // ← 类型过于宽泛
};

问题说明permissions 被断言为 Record<string, unknown> 而非 ExtPermissions,导致将其传入 hasSandboxStoragePermission(config.permissions) 时 TypeScript 无法校验字段类型。运行时行为正确(permissions?.storage === true 语义不变),但若 workerData 中的 permissions 结构将来变化,编译器不会给出警告。

修复建议

import type { ExtPermissions } from './permissions';

const config = workerData as {
  extensionName: string;
  extensionDir: string;
  entryPoint: string;
  permissions: ExtPermissions;
};

汇总

# 严重级别 文件 问题
1 🟡 MEDIUM tests/unit/extensions/sandboxPermission.test.ts 与 sandboxHost.test.ts 中的权限测试块完全重复
2 🔵 LOW src/process/extensions/sandbox/sandboxWorker.ts:43 config.permissions 类型为 Record<string, unknown> 而非 ExtPermissions

结论

⚠️ 有条件批准 — 存在一处测试重复(MEDIUM),清理后可合并


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

@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 Apr 2, 2026
- Remove duplicate describe('extensions/sandbox permissions') block
  from sandboxHost.test.ts (lines 42-68) that was identical to
  sandboxPermission.test.ts, keeping the dedicated file as canonical
- Remove unused imports of hasSandboxStoragePermission and
  getSandboxPermissionDeniedError from sandboxHost.test.ts

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

piorpua commented Apr 2, 2026

PR Fix 验证报告

原始 PR: #1803
修复方式: 直接推送到 fix/gate-storage-permission-v2(fork + maintainer 写入权限)

# 严重级别 文件 问题 修复方式 状态
1 🟡 MEDIUM tests/unit/extensions/sandboxHost.test.ts describe('extensions/sandbox permissions') 块(原第 42-68 行)与 sandboxPermission.test.ts 完全重复 删除 sandboxHost.test.ts 中的重复 describe 块及仅在该块中使用的 2 个 imports ✅ 已修复

总结: ✅ 已修复 1 个 | ⏭️ 跳过 1 个(LOW 级别)

🔵 LOW 级别问题(sandboxWorker.ts:43 类型过宽)已跳过(不阻塞合并,修复优先级低)。

@piorpua piorpua enabled auto-merge (squash) April 2, 2026 13:17
@piorpua piorpua added bot:done Auto-merged by bot and removed bot:fixing Fix in progress (mutex) labels Apr 2, 2026
@piorpua piorpua merged commit e52a5db into iOfficeAI:main Apr 2, 2026
11 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:done Auto-merged by bot

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: aion.storage exposed to extensions regardless of manifest storage permission

2 participants