fix(plugin): scope settings window routes to owning plugin - #2329
Conversation
Backing up a large database froze the whole UI: readFileSync made the main thread wait on disk, stalling every IPC, render and stream flush for seconds. The read now goes through fs.promises, which hands the work to the libuv threadpool and leaves the event loop free. The surrounding steps already used async fs, so this was the last synchronous holdout. Both APIs return a Buffer and Uint8Array(...) copies it either way, so the archive bytes are unchanged. One trade-off: the snapshot is now taken while other tasks can run, so commits landing during the read may be absent from the backup. The TRUNCATE checkpoint just before it keeps the copied file internally consistent; SQLite's own backup API would be the strict fix if that window ever matters.
The async agent.db read yielded the event loop between the draining checkpoint and the file copy, so a later auto or explicit checkpoint could rewrite the main database file mid-copy and corrupt the backup. Reading support files one await at a time could also mix instants across the archive, and copying the whole file into a fresh Uint8Array still stalled the main thread. Collect the backup under a data-layer withBackupReadLock: drain the WAL, then hold a read mark so no checkpoint can move pages into the file being copied while support files are read synchronously and the database is read asynchronously. Fall back to one blocking pass when the WAL cannot drain. Zip entries use zero-copy buffer views and are deflated in 4 MiB slices streamed to disk. Add consistency tests covering concurrent writes, auto checkpoints, encrypted databases, the blocked-checkpoint fallback, and multi-slice round trips.
- Pass the error to output.destroy() on both zip failure paths so a
pending backpressure drain gate is released; previously the producer
coroutine stayed suspended and retained the files record (including
the full agent.db image) for the process lifetime
- Broaden the fallback warning: { acquired: false } now also means a
commit landed during the drain window, not only a blocked checkpoint
- Replace existsSync check-then-act with direct operation plus ENOENT
handling across backup/restore helpers, closing delete-between-check-
and-use races in settings, prompt, temp backup, WAL sidecar, and zip
cleanup paths
Plugin settings windows load plugin-bundled HTML with sandbox disabled, but the preload derived pluginId from location.search, which the page can rewrite via history.replaceState without a reload. Since the enable/disable/invokeAction routes trusted the caller-supplied pluginId, one plugin's settings page could control any other plugin. Deliver pluginId through webPreferences.additionalArguments so the page cannot tamper with it, record a webContentsId -> pluginId mapping when the window is created, and reject enable/disable/invokeAction calls from a plugin settings window that targets a different plugin. Other callers (main window, settings UI, CLI) stay unrestricted.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe change links plugin settings windows to plugin routes, validates renderer ownership for plugin operations, restricts settings-window navigation, moves plugin ID loading to a process argument, and blocks OAuth pop-ups. Tests cover these behaviors. ChangesPlugin settings ownership
OAuth window control
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant SettingsRenderer
participant PluginRoutes
participant PluginSettingsWindow
participant PluginService
SettingsRenderer->>PluginRoutes: Submit plugin operation
PluginRoutes->>PluginSettingsWindow: Resolve caller webContents ownership
PluginSettingsWindow-->>PluginRoutes: Return plugin ID or null
PluginRoutes->>PluginService: Execute operation after ownership validation
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
zerob13
left a comment
zerob13
left a comment
There was a problem hiding this comment.
评审结论：REQUEST CHANGES
修复方向和实现都正确、不过度，但当前不能合——改了实现却漏改了一个旧测试，test:renderer 在本 PR 代码上必挂（本地实测复现），dev 分支上通过。
这个 PR 修了什么
DeepChat 允许插件自带一个「设置页」，用独立小窗口打开。窗口里跑的是插件自己写的网页，而 DeepChat 会给它注入一套 API（启用/禁用插件、执行插件动作）。问题在于：窗口如何知道「我是哪个插件的设置页」？旧实现是把插件 ID 写在窗口的网址参数里（?pluginId=xxx），而网页可以悄悄改写自己的网址。于是一个恶意插件的设置页可以伪造身份，冒充别的插件去启用、禁用或执行任意操作——包括对内置插件。
本 PR 的修法是：插件 ID 改由主进程在创建窗口时通过进程启动参数注入，网页碰不到；同时主进程记录「哪个窗口属于哪个插件」，收到请求时核对身份，对不上就拒绝。修完之后，插件的设置页最多只能操作它自己。安全评估通过：归属校验打在主进程路由层（信任边界正确），注入路径不可伪造，无过度设计。
必须修（P1）
test/renderer/api/preloadBoundaries.test.ts:230-275的插件设置 preload 测试用例还在用旧的「网址传 ID」方式（pushState注入?pluginId=）。preload 改为从process.argv读取后，测试环境 argv 里没有--deepchat-plugin-id=，该用例直接抛 "Plugin settings renderer is missing pluginId"。- 实测：PR head 上跑
pnpm vitest run --config vitest.config.renderer.ts test/renderer/api/preloadBoundaries.test.ts→ 1 failed / 4 passed；同一测试在 dev 分支 → 5/5 通过。 - 主进程侧的
test/main/routes/dispatcher.test.ts已同步更新，说明大概率只跑了test:main漏了 renderer。 - 修法是一行级：import preload 前把
--deepchat-plugin-id=plugin-1push 进process.argv，断言不用动。
- 实测：PR head 上跑
建议（不阻塞）
plugins.get路由（src/main/plugin/routes.ts:95-102）没有做同样的归属校验，和另外三条不一致。当前不可利用（preload 包装层把 pluginId 钉死在 argv 值上），但既然本 PR 的立场是「主进程路由层是信任边界」，保持一致更自洽。- 设置窗口没有拦截
will-navigate（src/main/desktop/pluginSettingsWindow.ts:35）——恶意设置页可以location.href跳到任意远程源，跳走后注入的deepchatPlugin桥还在。修复后爆炸半径已收敛为「只能控制自己的插件」，但加一行限制在file://入口内是顺手的加固。 - 既有
windowsMap 有个与本 PR 无关的重开竞态（src/main/desktop/pluginSettingsWindow.ts:41-44）：close 后立刻重开同 pluginId，旧窗口迟到的closed回调会把新窗口的 Map 项删掉，下次 open 可能开出重复窗口。本 PR 新增的 wcId Map 不受影响。既然动了这块生命周期，可以顺手修。
详细分析/来源
- 身份注入与读取：
src/main/desktop/pluginSettingsWindow.ts:28（additionalArguments 传入）、src/preload/plugin-settings-preload.ts:12-23（argv 读取，失败抛错） - 主进程归属校验：
src/main/plugin/routes.ts:21-31，应用于 enable / disable / invokeAction（routes.ts:104-131） - 信任锚：
src/main/routes/index.ts:231-233（webContentsId 来自event.sender.id，主进程派生，页面不可伪造） - 既有同构模式：
src/main/desktop/routes.ts:287 - 兼容性确认：内置插件设置页只用注入 API 不读网址（
plugins/cua/settings/assets/index.js:47、plugins/feishu/settings/assets/index.js:24）；loadFileURL query 保留，第三方插件页面自读location.search不受影响；主窗口 renderer 走src/renderer/api/PluginClient.ts调这三个路由不受影响（非设置窗口 owner 为 null → 放行） - 验证（PR head 本地实测）：
test/main/plugin/全目录 188/188 通过；dispatcher.test.ts73/73 通过；typecheck:node、oxlint、oxfmt --check全过；preloadBoundaries.test.ts1 失败（见 P1）
zerob13
left a comment
zerob13
left a comment
There was a problem hiding this comment.
评审结论：APPROVE（增量复审）
上一轮 REQUEST CHANGES 的唯一阻塞问题（P1）已在新提交 e53affd 中修复，本地实测通过。上轮的 4 条非阻塞建议未在本 delta 处理，不阻塞合入。
修复验证
- P1 回顾：插件设置页的身份注入从网址参数改为进程启动参数后，
test/renderer/api/preloadBoundaries.test.ts里的旧测试还在用网址方式传插件 ID，导致test:renderer在本 PR 代码上必挂。 - 修复方式与上轮建议完全一致：导入 preload 前把
--deepchat-plugin-id=plugin-1注入process.argv，try/finally 恢复原始 argv，全部断言保持不变。改动最小，没有引入新抽象或多余测试。 - 实测：新 head 上
pnpm vitest run --config vitest.config.renderer.ts test/renderer/api/preloadBoundaries.test.ts→ 5/5 通过（上轮在旧 head 上是 1 failed / 4 passed）；oxfmt --check通过。
遗留建议（不阻塞，维持上轮记录）
plugins.get路由（src/main/plugin/routes.ts:95-102）补上与其余三条一致的归属校验。- 设置窗口拦截
will-navigate，把导航限制在file://入口内（src/main/desktop/pluginSettingsWindow.ts:35）。 - 既有
windowsMap 的重开竞态（close 后立即重开同 pluginId，迟到回调误删新窗口记录）可顺手修复。
详细分析/来源
- 增量范围：
74801f43b..e53affd0e1，单文件test/renderer/api/preloadBoundaries.test.ts（+49/−40，全为该测试用例的 argv 注入与缩进调整）。 - 身份注入链路（未变）：
src/main/desktop/pluginSettingsWindow.ts:28（additionalArguments）→src/preload/plugin-settings-preload.ts:12-23（argv 读取，缺失即抛错）。 - 验证环境：detached worktree @ e53affd，node 22.22.0 / pnpm 10.34.5。
zerob13
left a comment
zerob13
left a comment
There was a problem hiding this comment.
Review: Approve ✅
The threat model in the description is real, and I could not find an exploitable bypass after the fix. All findings are P3 (non-blocking).
What this PR does
Before: the plugin settings preload derived its pluginId from location.search — fully controlled by the plugin-bundled page (rewritable via history.replaceState without a reload). Since plugins.enable / plugins.disable / plugins.invokeAction trusted the caller-supplied id without checking which window sent it, one plugin's settings page could disable, enable, or invoke arbitrary actions (with attacker-controlled payloads) on any other installed plugin.
Fix (two layers):
- Identity is now untamperable: the preload reads
--deepchat-plugin-id=fromprocess.argv, injected at window creation viawebPreferences.additionalArguments— unreachable from page JS undercontextIsolation: true/nodeIntegration: false. No application code trustslocation.searchanymore (the query param is kept only for plugin-side compatibility). - Main process enforces ownership:
assertPluginSettingsCallerOwnsmapswebContents id → pluginId(written before page load; keyed by Electron's monotonic, never-reused webContents id) and rejects mismatches after zod parse and before any service call, on all three routes.
Also checked: navigation/refresh don't change webPreferences (argv and preload unchanged); iframes get no preload bridge (nodeIntegrationInSubFrames off); window.open is rejected by setWindowOpenHandler; the exposed deepchatPlugin API is frozen and takes no pluginId. Unmapped callers (main window UI, CLI, internal callers) still pass — no breaking change. As a side effect, settings.open goes through invokeAction, so a settings page can no longer pop open other plugins' settings windows either.
Verified
- Main plugin + routes suites: 16 files / 366 tests passed (incl. the 6 new ownership-boundary tests). Renderer preload boundary tests: 5 passed. Typecheck, oxfmt, oxlint on touched files: clean.
Findings (P3, non-blocking)
src/main/plugin/routes.ts:22-26— the ownership check fails open for unmapped renderers. Safe today (the binding is written before page load, and the only deletion point isclosed, when the webContents can no longer send), but if a future window type reuses thepluginSettings.mjspreload without registering a binding, it will silently fail open. Suggest a comment inpluginSettingsWindow.tspinning the "binding must be written before page load" invariant.src/main/desktop/pluginSettingsWindow.ts:34,42,53— the binding lifecycle itself (write on open / delete on closed / lookup) has no direct test; the new tests mock the port. A small BrowserWindow-based test followingtest/main/desktop/window.test.tswould lock that invariant.src/main/plugin/routes.ts:48-53—pluginsUninstallUser/pluginsConfigureMcp/pluginsRetryHookalso take apluginId; applying the same assertion there would be consistent defense-in-depth (currently unreachable from settings pages since the preload only exposes 4 methods).- Follow-up, out of scope:
sandbox: falseis kept. With identity now coming from argv,sandbox: truemay be viable — needs a runtime check of sandboxed-preloadprocess.argvsemantics before flipping.
zerob13
left a comment
zerob13
left a comment
There was a problem hiding this comment.
Review: Request changes 🔴 — one-line test fix needed
The production changes in this delta are correct and faithfully implement the previously-agreed hardening (see "What this delta gets right" below). The one blocking problem is that the new test file fails at this head — both tests in pluginSettingsWindow.test.ts error out with TypeError: () => { ... } is not a constructor, so CI will be red. The fix is a one-word-class change in the test's mock.
Blocking
B1 — Arrow-function mock is not constructible (test/main/desktop/pluginSettingsWindow.test.ts:25)
The test replaces the global electron mock's BrowserWindow with mockImplementation(() => {...}). When the code under test runs new BrowserWindow(...) (pluginSettingsWindow.ts:18), V8 tries to construct that arrow function — arrow functions have no [[Construct]], so it throws. Both tests in the file ("keeps the reopened window record…" and "restricts navigation to the file entry") fail identically, meaning the test was never run before pushing.
Fix: use a function expression (or mockImplementationOnce) — the exact pattern already exists in test/main/desktop/window.test.ts:206.
Non-blocking
- The
fix(auth): deny popups in oauth windowcommit is out of this PR's scope — and it hardens a code path nobody can reach. The popup denial is added to the window created byOAuthService.startOAuthFlow(src/main/provider/auth/index.ts:400), which is only reachable throughstartOAuthLogin— a method with zero production callers. The live GitHub Copilot login flow uses the separategithubCopilotOAuth.tsimplementation, which already denies popups (githubCopilotOAuth.ts:63-68). Harmless and security-positive, so fine to keep — just flagging that if the intent was to harden the live OAuth window, that one already had this protection. - The "binding must be written before page load" ordering (
windows.set/pluginIdByWebContentsId.setbeforeloadFile, pluginSettingsWindow.ts:35-36) still lacks the pinning comment suggested earlier. The new lifecycle test partially covers it, so keeping this as a nice-to-have. - Plugin settings pages silently swallow
target=_blanklinks, while the main settings window shells them out to the default browser (window/index.ts:1386-1389). Deny-all is the right call for untrusted plugin content; just an optional UX follow-up.
What this delta gets right (verified)
- will-navigate restriction (pluginSettingsWindow.ts:38-43): denies any navigation whose protocol isn't
file:or whose pathname differs from the entry.will-navigateisn't emitted for the initialloadFileor same-document hash navigations, so the?pluginId=query and#sectionhash routing keep working. Matches the established pattern in the Feishu login window (remote/index.ts:2469-2479). - Reopen race guard (pluginSettingsWindow.ts:51-53): the
closedhandler now only deletes thewindowsrecord if it still points at this window — a staleclosedfrom window A can no longer delete reopened window B's record. plugins.getownership check (routes.ts:96-98): reuses the existingassertPluginSettingsCallerOwnshelper, identical to enable/disable/invokeAction.- Test scope is proportionate (lifecycle/concurrency + security boundary), not excessive — only the mock mechanism is broken.
Verification (detached worktree @ a33e1af)
| Check | Result |
|---|---|
vitest run test/main/desktop/pluginSettingsWindow.test.ts test/main/plugin/pluginRoutes.test.ts |
2 failed / 7 passed (both failures = B1; route tests all green incl. the new ownership test) |
vitest run test/main/plugin test/main/desktop |
2 failed / 331 passed (26 files) — no collateral breakage |
typecheck:node |
Clean |
oxlint / oxfmt --check (changed files) |
Clean |
Fix B1 and this is an approve from me — everything else in the delta lands the previously-discussed suggestions cleanly.
zerob13
left a comment
zerob13
left a comment
There was a problem hiding this comment.
Review: Approve ✅ — B1 fixed, verified locally
This delta (a33e1af → d2cde3e) is exactly the test-only fix requested last round, done in 2 lines and done right. No production code changed.
What changed and why it works
The BrowserWindow mock in pluginSettingsWindow.test.ts:25 now uses a regular function expression with an explicit return win. Arrow functions can't be constructed (new on them throws is not a constructor — the failure that blocked the previous head); a regular function can, and because a constructor that returns an object overrides this, new BrowserWindow(...) in the production code now correctly receives the fake window. The switch to vi.mocked(BrowserWindow) is a small bonus — typed mock access instead of a double cast.
Verification (detached worktree @ d2cde3e)
| Check | Result |
|---|---|
vitest run test/main/desktop/pluginSettingsWindow.test.ts test/main/plugin/pluginRoutes.test.ts |
9 passed / 0 failed — the 2 previously-failing lifecycle tests are green |
vitest run test/main (full suite) |
653 files / 9185 tests passed, no collateral breakage |
typecheck:node |
Clean |
oxfmt --check on the changed file |
Clean |
Status of the previous non-blocking notes
The three non-blocking observations from the last round (out-of-scope fix(auth) commit hardening an unreachable path, missing pinning comment for the set-before-load ordering, target=_blank links silently swallowed in plugin settings pages) are unchanged in this delta. None of them block merge.
Thanks for the quick turnaround — the fix matches the pattern from window.test.ts as suggested. LGTM.
3 participants
Summary
A plugin settings window loads plugin-bundled HTML with
sandbox: false, and thepreload derived its
pluginIdfromlocation.search— state the page fullycontrols and can rewrite via
history.replaceStatewithout a reload. Because theplugins.enable/plugins.disable/plugins.invokeActionroutes trusted thecaller-supplied
pluginIdwithout checking which window sent it, one plugin'ssettings page could disable, enable, or invoke arbitrary actions (with
attacker-controlled payloads) on any other installed plugin. This PR makes the
renderer-side identity untamperable, enforces window ownership in the main
process, and adds a regression test suite (6 tests) covering the boundary.
Changes
Identity delivery
pluginIdis now delivered viawebPreferences.additionalArgumentsand readfrom
process.argvin the preload. The settings page has no API to rewrite it(
nodeIntegration: false), unlikelocation.search, whichhistory.replaceState(null, '', '?pluginId=...')can change silently mid-session.loadFileURL query is kept unchanged, so existing plugin pages that readpluginIdfromlocation.searchthemselves continue to work; it is no longertrusted by any app code.
Ownership enforcement
PluginSettingsWindowrecords awebContentsId -> pluginIdmapping when asettings window is created and cleans it up on
closed; the lookup is exposedthrough
PluginSettingsWindowPort.getPluginIdForWebContents().plugins.enable/plugins.disable/plugins.invokeActionroutes rejectcalls from a plugin settings window whose owning plugin does not match the
target
pluginId. The check only applies once a caller is positivelyidentified as a plugin settings window: the main window, the app's plugin
management UI, CLI, and internal callers stay unrestricted, preserving every
existing legitimate call path.
webContentsId(available directly fromevent.sender.idat dispatch), so a destroyed/recreated webContents can neverinherit a stale identity.
Summary by CodeRabbit
New Features
Bug Fixes
Tests