-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(desktop): allow trusted response copy #8587
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,49 @@ | ||||||||
| import { describe, expect, it } from 'bun:test'; | ||||||||
| import { canUseDefaultSessionClipboard } from '../default-session-permissions'; | ||||||||
|
|
||||||||
| const trustedRequest = { | ||||||||
| permission: 'clipboard-sanitized-write', | ||||||||
| isMainFrame: true, | ||||||||
| isWorkspaceWindow: true, | ||||||||
| requestingUrl: 'file:///app/index.html', | ||||||||
| devServerUrl: undefined, | ||||||||
| }; | ||||||||
|
|
||||||||
| describe('default session permissions', () => { | ||||||||
| it('allows clipboard writes from the packaged app renderer', () => { | ||||||||
| expect(canUseDefaultSessionClipboard(trustedRequest)).toBe(true); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('allows clipboard writes from the configured Vite dev origin', () => { | ||||||||
| expect( | ||||||||
| canUseDefaultSessionClipboard({ | ||||||||
| ...trustedRequest, | ||||||||
| requestingUrl: 'http://localhost:5173/chat', | ||||||||
| devServerUrl: 'http://localhost:5173', | ||||||||
| }), | ||||||||
| ).toBe(true); | ||||||||
| }); | ||||||||
|
|
||||||||
| it.each([ | ||||||||
| ['clipboard reads', { permission: 'clipboard-read' }], | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The rejection matrix never pins the distinction between
Suggested change
中文说明拒绝矩阵没有钉住 — qwen3.8-max via Qwen Code /review (v0.21.5) |
||||||||
| ['unrelated permissions', { permission: 'geolocation' }], | ||||||||
| ['subframes', { isMainFrame: false }], | ||||||||
| ['unregistered windows', { isWorkspaceWindow: false }], | ||||||||
| ['external pages', { requestingUrl: 'https://example.com' }], | ||||||||
| [ | ||||||||
| 'a different dev port', | ||||||||
| { | ||||||||
| requestingUrl: 'http://localhost:5174/chat', | ||||||||
| devServerUrl: 'http://localhost:5173', | ||||||||
| }, | ||||||||
| ], | ||||||||
| ['requests without a URL', { requestingUrl: undefined }], | ||||||||
| ])('rejects %s', (_label, overrides) => { | ||||||||
| expect( | ||||||||
| canUseDefaultSessionClipboard({ | ||||||||
| ...trustedRequest, | ||||||||
| ...overrides, | ||||||||
| }), | ||||||||
| ).toBe(false); | ||||||||
| }); | ||||||||
| }); | ||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| import { isTrustedRendererFrameUrl } from './voice/frame-trust'; | ||
|
|
||
| export interface DefaultSessionClipboardRequest { | ||
| permission: string; | ||
| isMainFrame: boolean; | ||
| isWorkspaceWindow: boolean; | ||
| requestingUrl: string | undefined; | ||
| devServerUrl: string | undefined; | ||
| } | ||
|
|
||
| export function canUseDefaultSessionClipboard({ | ||
| permission, | ||
| isMainFrame, | ||
| isWorkspaceWindow, | ||
| requestingUrl, | ||
| devServerUrl, | ||
| }: DefaultSessionClipboardRequest): boolean { | ||
| return ( | ||
| permission === 'clipboard-sanitized-write' && | ||
| isMainFrame && | ||
| isWorkspaceWindow && | ||
| isTrustedRendererFrameUrl(requestingUrl, devServerUrl) | ||
| ); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -116,6 +116,7 @@ import { initNotificationService, initBadgeIcon, initInstanceBadge, updateBadgeC | |
| import { checkForUpdatesOnLaunch, setAutoUpdateEventSink, isUpdating } from './auto-update' | ||
| import type { EventSink } from '@craft-agent/server-core/transport' | ||
| import { validateGitBashPath, checkVCRedistInstalled } from '@craft-agent/server-core/services' | ||
| import { canUseDefaultSessionClipboard } from './default-session-permissions' | ||
|
|
||
| // Initialize electron-log for renderer process support | ||
| log.initialize() | ||
|
|
@@ -457,8 +458,25 @@ app.whenReady().then(async () => { | |
| isAudioOnlyMediaRequest(permission, details) && | ||
| windowManager?.getWorkspaceForWindow(wc.id) != null, | ||
| ) | ||
| const canWriteClipboard = ( | ||
| wc: { id: number } | null | undefined, | ||
| permission: string, | ||
| details: { isMainFrame?: boolean; requestingUrl?: string } | undefined, | ||
| ) => canUseDefaultSessionClipboard({ | ||
| permission, | ||
| isMainFrame: details?.isMainFrame === true, | ||
| isWorkspaceWindow: Boolean( | ||
| wc && windowManager?.getWorkspaceForWindow(wc.id) != null, | ||
| ), | ||
| requestingUrl: details?.requestingUrl, | ||
| devServerUrl: process.env.VITE_DEV_SERVER_URL, | ||
| }) | ||
| session.defaultSession.setPermissionRequestHandler( | ||
| (wc, permission, callback, details) => { | ||
| if (canWriteClipboard(wc, permission, details)) { | ||
| callback(true) | ||
| return | ||
|
Comment on lines
+476
to
+478
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The default-session handler wiring — both Suggested fix: extract the handler decision logic into a testable form (mirroring the 中文说明默认会话处理器的接线部分—— 建议修复:将处理器决策逻辑提取为可测试的形式(参照 — qwen3.8-max via Qwen Code /review (v0.21.5)
Comment on lines
+476
to
+478
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The default-session scoping comment above these handlers ("Scope the grant to mic/media only — do NOT broaden the default session to every permission", lines 417-421) is now stale: this diff adds the Suggested fix: update the comment alongside the grant — e.g. note that sanitized clipboard writes are likewise scoped to the trusted registered main-frame renderer (see 中文说明这两个处理器上方的默认会话授权范围注释("Scope the grant to mic/media only — do NOT broaden the default session to every permission",第 417-421 行)现已过时:本 diff 在其下方新增了 建议修复:在添加授权的同时更新注释——例如说明净化剪贴板写入同样限定于可信的已登记顶层 renderer frame(见 — qwen3.8-max via Qwen Code /review (v0.21.5) |
||
| } | ||
| if (!VOICE_PERMISSIONS.has(permission)) { | ||
| mainLog.debug(`defaultSession: denied non-voice permission '${permission}'`) | ||
| callback(false) | ||
|
|
@@ -472,6 +490,9 @@ app.whenReady().then(async () => { | |
| }, | ||
| ) | ||
| session.defaultSession.setPermissionCheckHandler((wc, permission, _origin, details) => { | ||
| if (canWriteClipboard(wc, permission, details)) { | ||
| return true | ||
| } | ||
| if (!VOICE_PERMISSIONS.has(permission)) { | ||
| mainLog.debug(`defaultSession: denied non-voice permission check '${permission}'`) | ||
| return false | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| import { describe, expect, it, mock } from 'bun:test'; | ||
| import { copyResponseText } from '../copy-response'; | ||
|
Comment on lines
+1
to
+2
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Same unreachability as the permission-policy test: this file sits outside every npm workspace and no CI workflow in this repo runs the desktop bun suite, so the copy-failure status path it covers has no gate in this repository's pipeline (also applies to Suggested fix: confirm the desktop suite runs in the upstream openwork repo's CI, or add a desktop job here. 中文说明与权限策略测试相同的不可达问题:该文件位于所有 npm workspaces 之外,本仓库没有任何 CI 工作流运行 desktop 的 bun 套件,因此它所覆盖的复制失败状态路径在本仓库流水线中没有门禁(同样适用于 建议修复:确认上游 openwork 仓库的 CI 会运行 desktop 套件,或在本仓库添加 desktop 任务。 — qwen3.8-max via Qwen Code /review (v0.21.5) |
||
|
|
||
| describe('copyResponseText', () => { | ||
| it('writes the complete response and reports success', async () => { | ||
| const writeText = mock(async () => {}); | ||
|
|
||
| await expect( | ||
| copyResponseText('complete response', writeText), | ||
| ).resolves.toEqual({ status: 'copied' }); | ||
| expect(writeText).toHaveBeenCalledWith('complete response'); | ||
| }); | ||
|
|
||
| it('reports clipboard failures', async () => { | ||
| const writeText = mock(async () => { | ||
| throw new Error('permission denied'); | ||
| }); | ||
|
|
||
| await expect( | ||
| copyResponseText('response', writeText), | ||
| ).resolves.toMatchObject({ | ||
| status: 'failed', | ||
| error: expect.any(Error), | ||
| }); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| export async function copyResponseText( | ||
| text: string, | ||
| writeText: (value: string) => Promise<void>, | ||
| ): Promise<{ status: 'copied' } | { status: 'failed'; error: unknown }> { | ||
| try { | ||
| await writeText(text); | ||
| return { status: 'copied' }; | ||
| } catch (error) { | ||
| return { status: 'failed', error }; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] This new test file is unreachable from every test command this repository runs:
packages/desktopis excluded from the npm workspaces ("!packages/desktop"in the rootpackage.json), and no CI workflow in this repo executes the desktop bun suite (verified at the base commit). The test passes when run manually, but nothing in this repo gates the change it covers (also applies tocopy-response.test.ts). — Failure scenario: a future commit reverts or breaks the clipboard permission wiring, and response copy silently stops working again — every CI check on this repo stays green because none of them collects this test; it is gated only if the upstream openwork repo's CI happens to run it.Suggested fix: if desktop validation intentionally lives in the upstream openwork repo, confirm its CI runs this suite; otherwise wire a desktop bun-test job into this repo's CI.
中文说明
这个新测试文件对本仓库运行的所有测试命令都不可达:
packages/desktop被排除在 npm workspaces 之外(根package.json中的"!packages/desktop"),且本仓库没有任何 CI 工作流执行 desktop 的 bun 测试套件(已在 base 提交上核实)。手动运行该测试可以通过,但本仓库没有任何门禁覆盖它所保护的改动(同样适用于copy-response.test.ts)。失败场景:未来某个提交还原或破坏了剪贴板权限接线,回复复制再次静默失效——本仓库的所有 CI 检查依然全绿,因为没有任何一个会收集这个测试;只有当上游 openwork 仓库的 CI 恰好运行它时才有门禁。建议修复:如果 desktop 验证有意放在上游 openwork 仓库,请确认其 CI 会运行该套件;否则在本仓库 CI 中接入一个 desktop bun-test 任务。
— qwen3.8-max via Qwen Code /review (v0.21.5)