From 18bba95530f1c5ffc6503618e0e2c64a88678f65 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 26 Sep 2026 14:48:43 -0700 Subject: [PATCH 1/5] fix(desktop): ask before accessing local files --- apps/desktop/README.md | 4 +- apps/desktop/e2e/local-files.spec.ts | 205 ++++++++++++++++-- apps/desktop/src/main/ipc.test.ts | 65 ------ apps/desktop/src/main/ipc.ts | 47 +++- .../src/main/local-file-permissions.ts | 175 +++++++++++++++ apps/desktop/src/main/local-files.test.ts | 15 +- apps/desktop/src/main/local-files.ts | 83 ++++--- packages/desktop-bridge/src/local-files.ts | 2 +- 8 files changed, 482 insertions(+), 114 deletions(-) create mode 100644 apps/desktop/src/main/local-file-permissions.ts diff --git a/apps/desktop/README.md b/apps/desktop/README.md index 309160f0339..9b3af8618df 100644 --- a/apps/desktop/README.md +++ b/apps/desktop/README.md @@ -188,7 +188,9 @@ Copilot can inspect user-selected local directories through the ordinary VFS too - **Bound to a live Copilot call:** before a native read/search or browser action, Electron asks the authenticated Sim origin for the pending tool-call record. Local requests must exactly match its persisted operation, path, and options; browser actions run with the persisted arguments rather than renderer-supplied ones. Completed, failed, and aborted runs are rejected. - **Abort-aware and bounded:** stop/cancel propagates to active native scans and reads. File size, aggregate grep bytes, line, result, traversal-depth, and scan-count limits remain enforced in Electron, and unsafe regular expressions are rejected before execution. -Raw local file bytes are never exposed through the preload bridge and cannot be staged or uploaded by a model. Bounded text read/search results are returned to the active Copilot request; a user must use the normal attachment UI when they want the file itself to leave the device. +The native `read_local_file` and `import_local_files` tools also accept absolute or `~/` paths. Before accessing a new file or folder, Electron displays a bundled, isolated permission dialog showing the resolved path and the connected server. **Allow for this chat** grants access to that file, or that folder and its contents, for the current desktop session. Closing or declining the dialog returns no contents. Grants are scoped to the server, account generation, chat, and operation; import grants also bind the destination workspace and folder. A read grant never authorizes an import. Grants are not persisted and expire on sign-out, server changes, or app restart. + +Approved native reads can return bounded text, directory listings, images, or PDFs to the chat. Approved imports transfer file bytes to Workspace Files. Electron revalidates the pending call after consent, checks canonical containment and grant identity throughout the operation, and opens files with no-follow and descriptor identity checks. A model or hosted renderer cannot answer the local consent dialog. These permissions govern the native file tools; the separately enabled terminal still runs with the user's OS privileges. ## Auto-update, channels, rollout, rollback diff --git a/apps/desktop/e2e/local-files.spec.ts b/apps/desktop/e2e/local-files.spec.ts index d2645297598..c927a57d866 100644 --- a/apps/desktop/e2e/local-files.spec.ts +++ b/apps/desktop/e2e/local-files.spec.ts @@ -1,4 +1,12 @@ -import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { + mkdirSync, + mkdtempSync, + realpathSync, + renameSync, + rmSync, + symlinkSync, + writeFileSync, +} from 'node:fs' import { createServer, type Server } from 'node:http' import { tmpdir } from 'node:os' import { join } from 'node:path' @@ -8,17 +16,31 @@ import type { DesktopLocalFileRequest, SimDesktopApi } from '@sim/desktop-bridge const DESKTOP_DIR = fileURLToPath(new URL('..', import.meta.url)) -test('native file tools read and import through the installed preload without Sim folder grants', async () => { +test('native file tools require local consent and reuse only the approved chat and path', async () => { const root = mkdtempSync(join(tmpdir(), 'sim-native-files-e2e-')) const source = join(root, 'Reports') + const outside = join(root, 'Reports-other') + mkdirSync(outside) + writeFileSync(join(outside, 'private.txt'), 'outside contents') mkdirSync(join(source, 'empty'), { recursive: true }) writeFileSync(join(source, 'report.txt'), 'native file contents') const png = 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mP8/x8AAusB9Y9Zl1sAAAAASUVORK5CYII=' writeFileSync(join(source, 'image.png'), Buffer.from(png, 'base64')) - let claimed = false - const calls: Record }> = { + const claimed = new Set() + const authorizedCalls = new Set() + let signedIn = true + const calls: Record< + string, + { toolName: string; args: Record; chatId?: string } | undefined + > = { + directory: { toolName: 'read_local_file', args: { path: source } }, text: { toolName: 'read_local_file', args: { path: join(source, 'report.txt') } }, + otherChat: { + toolName: 'read_local_file', + args: { path: join(source, 'report.txt') }, + chatId: 'other-chat', + }, image: { toolName: 'read_local_file', args: { path: join(source, 'image.png') } }, import: { toolName: 'import_local_files', @@ -32,32 +54,49 @@ test('native file tools read and import through the installed preload without Si const path = new URL(request.url ?? '/', 'http://127.0.0.1').pathname if (path === '/api/auth/get-session') { response.writeHead(200, { 'Content-Type': 'application/json' }).end( - JSON.stringify({ - user: { id: 'local-file-user' }, - session: { id: 'local-file-session' }, - }) + JSON.stringify( + signedIn + ? { + user: { id: 'local-file-user' }, + session: { id: 'local-file-session' }, + } + : null + ) ) return } + if (path === '/api/auth/sign-out') { + signedIn = false + response + .writeHead(200, { + 'Content-Type': 'application/json', + 'Set-Cookie': 'better-auth.session_token=; HttpOnly; SameSite=Lax; Path=/; Max-Age=0', + }) + .end('{}') + return + } if (path === '/api/desktop/tool/authorize') { let body = '' for await (const chunk of request) body += chunk.toString() const input = JSON.parse(body) const call = calls[input.toolCallId] - if (!call || (input.claim && claimed)) { + if (!call || (input.claim && claimed.has(input.toolCallId))) { response.writeHead(call ? 409 : 403, { 'Content-Type': 'application/json' }).end('{}') return } - if (input.claim) claimed = true + authorizedCalls.add(input.toolCallId) + if (input.claim) claimed.add(input.toolCallId) response .writeHead(200, { 'Content-Type': 'application/json' }) - .end(JSON.stringify({ ...call, chatId: 'org-chat' })) + .end(JSON.stringify({ ...call, chatId: call.chatId ?? 'org-chat' })) return } response .writeHead(200, { 'Content-Type': 'text/html', - 'Set-Cookie': 'better-auth.session_token=fixture; HttpOnly; SameSite=Lax; Path=/', + ...(signedIn + ? { 'Set-Cookie': 'better-auth.session_token=fixture; HttpOnly; SameSite=Lax; Path=/' } + : {}), }) .end('Local file fixture

Local files

') }) @@ -82,9 +121,46 @@ test('native file tools read and import through the installed preload without Si return api.localFiles(request) }, input) await expect - .poll(async () => (await invoke({ operation: 'read', toolCallId: 'text' })).ok) + .poll(() => + window.evaluate(async () => { + const api = (globalThis as typeof globalThis & { simDesktop: SimDesktopApi }).simDesktop + return (await api.localFilesystem?.({ operation: 'list_mounts' }))?.ok + }) + ) .toBe(true) - expect(await invoke({ operation: 'read', toolCallId: 'text' })).toMatchObject({ + await window.evaluate(async () => { + const api = (globalThis as typeof globalThis & { simDesktop: SimDesktopApi }).simDesktop + await api.settings.setPreference('browserEnabled', false) + await api.settings.setPreference('terminalEnabled', false) + }) + const deniedPrompt = app.waitForEvent('window', { timeout: 10_000 }) + const deniedRead = invoke({ operation: 'read', toolCallId: 'text' }) + const denial = await deniedPrompt + await expect(denial.getByRole('button', { name: "Don't allow", exact: true })).toBeFocused() + await denial.screenshot({ + path: + process.env.DESKTOP_LOCAL_FILES_REPORT_PATH ?? + test.info().outputPath('local-file-consent.png'), + }) + await denial.getByRole('button', { name: "Don't allow", exact: true }).click() + expect(await deniedRead).toMatchObject({ ok: false }) + + const folderPrompt = app.waitForEvent('window') + const folderRead = invoke({ operation: 'read', toolCallId: 'directory' }) + const folderConsent = await folderPrompt + const queuedRead = invoke({ operation: 'read', toolCallId: 'text' }) + expect( + await folderConsent.evaluate(() => typeof (globalThis as { simDesktop?: unknown }).simDesktop) + ).toBe('undefined') + await folderConsent.getByRole('button', { name: 'Allow for this chat', exact: true }).click() + expect(await folderRead).toMatchObject({ ok: true, data: { representation: 'directory' } }) + expect(await queuedRead).toMatchObject({ ok: true, data: { text: 'native file contents' } }) + const canonicalRequest = { + operation: 'read' as const, + toolCallId: 'text', + path: join(outside, 'private.txt'), + } + expect(await invoke(canonicalRequest)).toMatchObject({ ok: true, data: { representation: 'text', text: 'native file contents' }, }) @@ -92,7 +168,74 @@ test('native file tools read and import through the installed preload without Si ok: true, data: { observations: [{ mediaType: 'image/png', data: png }] }, }) - const result = await invoke({ operation: 'manifest', toolCallId: 'import' }) + const runningApp = app + const requestPermission = async (request: DesktopLocalFileRequest) => { + const shown = runningApp.waitForEvent('window', { timeout: 10_000 }) + const result = invoke(request) + void result.catch(() => {}) + const prompt = await shown + await expect(prompt.getByRole('button', { name: "Don't allow", exact: true })).toBeVisible() + return { prompt, result } + } + await test.step('a folder grant does not authorize another chat or a symlink escape', async () => { + const otherChat = await requestPermission({ operation: 'read', toolCallId: 'otherChat' }) + const dismissed = otherChat.prompt.waitForEvent('close') + await otherChat.prompt + .getByRole('button', { name: "Don't allow", exact: true }) + .press('Escape') + .catch(() => {}) + await dismissed + expect(await otherChat.result).toMatchObject({ ok: false }) + symlinkSync(join(outside, 'private.txt'), join(source, 'linked.txt')) + calls.escape = { toolName: 'read_local_file', args: { path: join(source, 'linked.txt') } } + const escapedRead = await requestPermission({ operation: 'read', toolCallId: 'escape' }) + await expect(escapedRead.prompt.getByRole('dialog')).toContainText( + JSON.stringify(realpathSync(join(outside, 'private.txt'))) + ) + await escapedRead.prompt.getByRole('button', { name: "Don't allow", exact: true }).click() + expect(await escapedRead.result).toMatchObject({ ok: false }) + rmSync(join(source, 'linked.txt')) + }) + await test.step('cancelled calls and changed arguments cannot acquire a grant', async () => { + for (const changed of [false, true]) { + calls.stale = { toolName: 'read_local_file', args: { path: join(outside, 'private.txt') } } + const stale = await requestPermission({ operation: 'read', toolCallId: 'stale' }) + if (changed) calls.stale.args.path = join(source, 'report.txt') + else calls.stale = undefined + await stale.prompt.getByRole('button', { name: 'Allow for this chat', exact: true }).click() + expect(await stale.result).toMatchObject({ ok: false }) + } + }) + await test.step('a queued call is revalidated even when its folder is already approved', async () => { + calls.blocker = { toolName: 'read_local_file', args: { path: join(outside, 'private.txt') } } + calls.queued = { toolName: 'read_local_file', args: { path: join(source, 'report.txt') } } + const blocker = await requestPermission({ operation: 'read', toolCallId: 'blocker' }) + const queued = invoke({ operation: 'read', toolCallId: 'queued' }) + void queued.catch(() => {}) + await expect.poll(() => authorizedCalls.has('queued')).toBe(true) + calls.queued = undefined + await blocker.prompt.getByRole('button', { name: "Don't allow", exact: true }).click() + expect(await blocker.result).toMatchObject({ ok: false }) + expect(await queued).toMatchObject({ ok: false }) + }) + await test.step('replacing the proposed folder during consent does not expose its new target', async () => { + const proposed = join(root, 'Proposed') + mkdirSync(proposed) + calls.retargeted = { toolName: 'read_local_file', args: { path: proposed } } + const retargeted = await requestPermission({ operation: 'read', toolCallId: 'retargeted' }) + renameSync(proposed, join(root, 'Original')) + mkdirSync(proposed) + writeFileSync(join(proposed, 'unapproved.txt'), 'replacement folder contents') + await retargeted.prompt + .getByRole('button', { name: 'Allow for this chat', exact: true }) + .click() + expect(await retargeted.result).toMatchObject({ ok: false }) + }) + const importPrompt = app.waitForEvent('window') + const importing = invoke({ operation: 'manifest', toolCallId: 'import' }) + const importConsent = await importPrompt + await importConsent.getByRole('button', { name: 'Allow for this chat', exact: true }).click() + const result = await importing if (!result.ok || result.data.kind !== 'manifest') throw new Error(JSON.stringify(result)) expect(result.data.targetWorkspaceId).toBe('target-workspace') expect(result.data.entries.map((entry) => entry.relativePath)).toEqual([ @@ -136,6 +279,38 @@ test('native file tools read and import through the installed preload without Si ok: false, code: 'ALREADY_STARTED', }) + await test.step('import approval is bound to its destination workspace', async () => { + calls.otherImport = { + toolName: 'import_local_files', + args: { path: source, targetWorkspaceId: 'other-workspace' }, + } + const otherImport = await requestPermission({ + operation: 'manifest', + toolCallId: 'otherImport', + }) + await otherImport.prompt.getByRole('button', { name: "Don't allow", exact: true }).click() + expect(await otherImport.result).toMatchObject({ ok: false }) + }) + + await test.step('sign-out revokes chat grants before the next account session', async () => { + await window.evaluate(async () => { + await fetch('/api/auth/sign-out', { method: 'POST' }) + }) + await expect(window).toHaveURL(`http://127.0.0.1:${address.port}/login`) + signedIn = true + await window.goto(`http://127.0.0.1:${address.port}/`) + await expect + .poll(() => + window.evaluate(async () => { + const api = (globalThis as typeof globalThis & { simDesktop: SimDesktopApi }).simDesktop + return (await api.localFilesystem?.({ operation: 'list_mounts' }))?.ok + }) + ) + .toBe(true) + const revoked = await requestPermission({ operation: 'read', toolCallId: 'text' }) + await revoked.prompt.getByRole('button', { name: "Don't allow", exact: true }).click() + expect(await revoked.result).toMatchObject({ ok: false }) + }) } finally { await app?.close() server?.close() diff --git a/apps/desktop/src/main/ipc.test.ts b/apps/desktop/src/main/ipc.test.ts index c4885d5d70b..cd80c062499 100644 --- a/apps/desktop/src/main/ipc.test.ts +++ b/apps/desktop/src/main/ipc.test.ts @@ -458,71 +458,6 @@ describe('registerIpcHandlers', () => { }) }) - it('reads a native file through canonical IPC arguments without folder grants or user activation', async () => { - const { invoke } = collectHandlers() - const handler = invoke.get('desktop:local-files') - const path = fileURLToPath(import.meta.url) - const fetchAuthorization = vi.fn(async () => - Response.json({ chatId: 'chat-1', toolName: 'read_local_file', args: { path, limit: 64 } }) - ) - const authorizedEvent = { - senderFrame: { url: `${APP}/o/org/home` }, - sender: { session: { fetch: fetchAuthorization } }, - } - const mounts = vi.spyOn(deps.localFilesystem, 'handle') - expect( - await handler?.(authorizedEvent, { - operation: 'read', - toolCallId: 'tool-native', - path: '/not/the/canonical/path', - }) - ).toMatchObject({ - ok: true, - data: { kind: 'read', path, text: readFileSync(path, 'utf8').slice(0, 64) }, - }) - expect(mounts).not.toHaveBeenCalled() - expect(fetchAuthorization).toHaveBeenCalledWith( - `${APP}/api/desktop/tool/authorize`, - expect.objectContaining({ body: JSON.stringify({ toolCallId: 'tool-native' }) }) - ) - expect( - await handler?.(evilEvent, { operation: 'read', toolCallId: 'tool-native' }) - ).toMatchObject({ ok: false }) - }) - - it('claims native imports at IPC before traversal and rejects a replay', async () => { - const { invoke } = collectHandlers() - const handler = invoke.get('desktop:local-files') - const fetchAuthorization = vi - .fn() - .mockResolvedValueOnce( - Response.json({ - chatId: 'chat-1', - toolName: 'import_local_files', - args: { path: fileURLToPath(import.meta.url), targetWorkspaceId: 'workspace' }, - }) - ) - .mockResolvedValueOnce(Response.json({ error: 'already started' }, { status: 409 })) - const event = { - senderFrame: { url: `${APP}/o/org/home` }, - sender: { session: { fetch: fetchAuthorization } }, - } - const request = { operation: 'manifest', toolCallId: 'tool-import' } - expect(await handler?.(event, request)).toMatchObject({ - ok: true, - data: { - kind: 'manifest', - targetWorkspaceId: 'workspace', - entries: [{ relativePath: '', kind: 'file' }], - }, - }) - expect(fetchAuthorization).toHaveBeenCalledWith( - `${APP}/api/desktop/tool/authorize`, - expect.objectContaining({ body: JSON.stringify({ toolCallId: 'tool-import', claim: true }) }) - ) - expect(await handler?.(event, request)).toMatchObject({ ok: false, code: 'ALREADY_STARTED' }) - }) - it('requires server authorization for every privileged filesystem tool request', async () => { const { invoke } = collectHandlers() const handler = invoke.get('desktop:local-filesystem') diff --git a/apps/desktop/src/main/ipc.ts b/apps/desktop/src/main/ipc.ts index 840334ce29c..8b44dcb5ee7 100644 --- a/apps/desktop/src/main/ipc.ts +++ b/apps/desktop/src/main/ipc.ts @@ -1,3 +1,4 @@ +import { isDeepStrictEqual } from 'node:util' import { BROWSER_TOOL_AUTHORIZATION_TIMEOUT_MS, type BrowserPanelAction, @@ -31,6 +32,10 @@ import { isRecordLike, toRecord } from '@sim/utils/object' import { PASTE_LIMITS, utf8ByteLength } from '@sim/utils/paste' import type { BrowserWindow, IpcMainEvent, IpcMainInvokeEvent, WebContents } from 'electron' import { clipboard, ipcMain, shell } from 'electron' +import { + captureAccountDataGeneration, + isAccountDataGenerationCurrent, +} from '@/main/account-data-generation' import { type BrowserToolQueueBoundary, cancelActiveTool, @@ -81,6 +86,7 @@ import { isSafeInternalPath } from '@/main/config' import type { DesktopSettingsService } from '@/main/desktop-settings' import { isDesktopPreferenceKey } from '@/main/desktop-settings' import { hasRecentDeliberateInput, hasRecentDiscreteInput } from '@/main/input-activity' +import { type LocalFileAccess, LocalFilePermissions } from '@/main/local-file-permissions' import { executeLocalFileRequest } from '@/main/local-files' import type { LocalFilesystemService } from '@/main/local-filesystem' import { isAppOrigin, openExternalSafe } from '@/main/navigation' @@ -591,6 +597,7 @@ async function authorizeLocalFilesystemTool( * unvalidated args they must parse themselves. */ export function registerIpcHandlers(deps: IpcDeps): void { + const localFilePermissions = new LocalFilePermissions() const browserScopeBySender = new WeakMap() const terminalScopeBySender = new WeakMap() const browserPendingScopesBySender = new WeakMap>() @@ -722,8 +729,12 @@ export function registerIpcHandlers(deps: IpcDeps): void { requiresAccountData: true, passSender: true, denied: { ok: false, error: 'Local file tools are unavailable from this page.' }, - handler: (_sender, request, authorization) => - executeLocalFileRequest(request, authorization as DesktopToolAuthorization), + handler: (_sender, request, authorization, access) => + executeLocalFileRequest( + request, + authorization as DesktopToolAuthorization, + access as LocalFileAccess + ), }, 'desktop:local-filesystem': { kind: 'invoke', @@ -2073,6 +2084,8 @@ export function registerIpcHandlers(deps: IpcDeps): void { } } if (channel === 'desktop:local-files') { + const generation = captureAccountDataGeneration() + const origin = deps.appOrigin() const request = args[0] if (!isRecordLike(request)) return { ok: false, error: 'Invalid local file request.' } let failureStatus: number | undefined @@ -2096,7 +2109,35 @@ export function registerIpcHandlers(deps: IpcDeps): void { !['read_local_file', 'import_local_files'].includes(authorization.toolName) ) return { ok: false, error: 'This is not an authorized pending local file tool call.' } - handlerArgs = [request, authorization] + if ( + authorization.toolName === 'read_local_file' + ? request.operation !== 'read' + : request.operation !== 'manifest' && request.operation !== 'chunk' + ) + return { ok: false, error: 'The operation does not match the pending tool call.' } + const parent = deps.getWindowForContents(event.sender) + if (!parent) + return { ok: false, error: 'A desktop window is required to approve file access.' } + try { + const access = await localFilePermissions.authorize(authorization, { + parent, + origin, + generation, + isCurrent: () => + isAccountDataGenerationCurrent(generation) && + deps.accountDataAvailable() && + deps.appOrigin() === origin && + isAppOriginSender(event, origin), + revalidate: async () => + isDeepStrictEqual( + authorization, + await fetchDesktopToolAuthorization(event, deps, request.toolCallId) + ), + }) + handlerArgs = [request, authorization, access] + } catch (error) { + return { ok: false, error: getErrorMessage(error) } + } } if (spec.passSender) { handlerArgs = [event.sender, ...handlerArgs] diff --git a/apps/desktop/src/main/local-file-permissions.ts b/apps/desktop/src/main/local-file-permissions.ts new file mode 100644 index 00000000000..0e3b1338ff9 --- /dev/null +++ b/apps/desktop/src/main/local-file-permissions.ts @@ -0,0 +1,175 @@ +import { lstat, realpath, stat } from 'node:fs/promises' +import { homedir } from 'node:os' +import { isAbsolute, join, relative, resolve, sep } from 'node:path' +import { isDesktopScopeId } from '@sim/desktop-bridge' +import type { BrowserWindow } from 'electron' +import { showShellDialog } from '@/main/dialogs' +import type { LocalFileAuthorization } from '@/main/local-files' + +const MAX_GRANTS = 256 +const MAX_PENDING_REQUESTS = 32 + +interface LocalFilePermissionContext { + parent: BrowserWindow + origin: string + generation: number + isCurrent: () => boolean + revalidate: () => Promise +} + +interface LocalFileGrant { + scope: string + path: string + directory: boolean + dev: number + ino: number +} + +export interface LocalFileAccess { + path: string + resolve: (path: string) => Promise +} + +function nativePath(value: unknown): string { + if (typeof value !== 'string' || value.length > 4096 || value.includes('\0')) + throw new Error('A native absolute path or ~/ path is required.') + const path = + value === '~' ? homedir() : value.startsWith('~/') ? join(homedir(), value.slice(2)) : value + if (!isAbsolute(path)) throw new Error('Use an absolute path or ~/ path.') + return resolve(path) +} + +function contains(grant: LocalFileGrant, path: string): boolean { + if (path === grant.path) return true + if (!grant.directory) return false + const rel = relative(grant.path, path) + return !isAbsolute(rel) && rel !== '..' && !rel.startsWith(`..${sep}`) +} + +function assertCurrent(context: LocalFilePermissionContext): void { + if (context.parent.isDestroyed() || !context.isCurrent()) + throw new Error('This local file request expired. Ask again in the current chat.') +} + +/** Chat grants live only in this desktop session and are never writable by the hosted renderer. */ +export class LocalFilePermissions { + private grants: LocalFileGrant[] = [] + private generation = -1 + private queue: Promise = Promise.resolve() + private pending = 0 + + async authorize( + authorization: LocalFileAuthorization & { chatId: string }, + context: LocalFilePermissionContext + ): Promise { + if (this.pending >= MAX_PENDING_REQUESTS) + throw new Error('Too many local file requests are waiting for permission. Try again later.') + const wasQueued = this.pending > 0 + this.pending++ + const pending = this.queue.then(() => this.authorizeNext(authorization, context, wasQueued)) + this.queue = pending.then( + () => undefined, + () => undefined + ) + try { + return await pending + } finally { + this.pending-- + } + } + + private async authorizeNext( + authorization: LocalFileAuthorization & { chatId: string }, + context: LocalFilePermissionContext, + wasQueued: boolean + ): Promise { + assertCurrent(context) + if (this.generation !== context.generation) { + this.grants = [] + this.generation = context.generation + } + const importing = authorization.toolName === 'import_local_files' + if ( + importing && + (!isDesktopScopeId(authorization.args.targetWorkspaceId) || + (authorization.args.folderId !== undefined && + !isDesktopScopeId(authorization.args.folderId))) + ) + throw new Error('A valid destination workspace and folder are required for imports.') + const scope = JSON.stringify([ + context.origin, + authorization.chatId, + authorization.toolName, + ...(importing ? [authorization.args.targetWorkspaceId, authorization.args.folderId] : []), + ]) + const path = await realpath(nativePath(authorization.args.path)) + const info = await stat(path) + if (!info.isFile() && !info.isDirectory()) + throw new Error('The path is not a regular file or directory.') + let grant = this.grants.find((entry) => entry.scope === scope && contains(entry, path)) + if (grant) { + if (wasQueued && !(await context.revalidate())) + throw new Error('This local file tool call is no longer pending or its arguments changed.') + const root = await lstat(grant.path) + if (root.dev !== grant.dev || root.ino !== grant.ino || root.isSymbolicLink()) { + this.grants = this.grants.filter((entry) => entry !== grant) + grant = undefined + } + } + if (!grant) { + if (this.grants.length >= MAX_GRANTS) + throw new Error( + 'Restart Sim to clear this session’s local file permissions before adding more.' + ) + assertCurrent(context) + const kind = info.isDirectory() ? 'folder' : 'file' + const displayedPath = JSON.stringify(path).replace( + /[\u202a-\u202e\u2066-\u2069]/g, + (character) => `\\u${character.charCodeAt(0).toString(16).padStart(4, '0')}` + ) + const result = await showShellDialog(context.parent, { + title: importing ? `Import this ${kind}?` : `Read this ${kind}?`, + message: displayedPath, + detail: [ + importing + ? `Sim will upload this ${kind}${info.isDirectory() ? ' and its contents' : ''} to your workspace on ${context.origin}.` + : `Sim will read this ${kind}${info.isDirectory() ? ' and its contents' : ''} and send the results to ${context.origin} for this chat.`, + ...(importing ? [`Destination workspace: ${authorization.args.targetWorkspaceId}`] : []), + 'This permission applies only to this chat and ends when Sim closes.', + ].join('\n\n'), + buttons: ['Allow for this chat', "Don't allow"], + defaultId: 1, + cancelId: 1, + }) + assertCurrent(context) + if (result.response !== 0) throw new Error('The user did not allow this local file access.') + if (!(await context.revalidate())) + throw new Error('This local file tool call is no longer pending or its arguments changed.') + grant = { scope, path, directory: info.isDirectory(), dev: info.dev, ino: info.ino } + await this.resolve(grant, path, context) + this.grants.push(grant) + } + await this.resolve(grant, path, context) + const approvedGrant = grant + return { path, resolve: (candidate) => this.resolve(approvedGrant, candidate, context) } + } + + private async resolve( + grant: LocalFileGrant, + candidate: string, + context: LocalFilePermissionContext + ): Promise { + assertCurrent(context) + const root = await lstat(grant.path) + if ( + root.dev !== grant.dev || + root.ino !== grant.ino || + (grant.directory ? !root.isDirectory() : !root.isFile()) + ) + throw new Error('The approved file or folder changed. Ask again to request access.') + const path = await realpath(candidate) + if (!contains(grant, path)) throw new Error('This path is outside the approved file or folder.') + assertCurrent(context) + return path + } +} diff --git a/apps/desktop/src/main/local-files.test.ts b/apps/desktop/src/main/local-files.test.ts index 1e01abebfd7..a7dd2c87d07 100644 --- a/apps/desktop/src/main/local-files.test.ts +++ b/apps/desktop/src/main/local-files.test.ts @@ -1,8 +1,19 @@ -import { mkdir, mkdtemp, rm, symlink, truncate, writeFile } from 'node:fs/promises' +import { mkdir, mkdtemp, realpath, rm, symlink, truncate, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, beforeEach, expect, it } from 'vitest' -import { executeLocalFileRequest } from '@/main/local-files' +import { + executeLocalFileRequest as executeApprovedLocalFileRequest, + type LocalFileAuthorization, +} from '@/main/local-files' + +/** Parser and import invariants run with explicit fixture access; consent is covered through Electron. */ +function executeLocalFileRequest(request: unknown, authorization: LocalFileAuthorization) { + return executeApprovedLocalFileRequest(request, authorization, { + path: String(authorization.args.path), + resolve: realpath, + }) +} let root: string beforeEach(async () => { diff --git a/apps/desktop/src/main/local-files.ts b/apps/desktop/src/main/local-files.ts index a4b9cd9a42c..10cfa6674e5 100644 --- a/apps/desktop/src/main/local-files.ts +++ b/apps/desktop/src/main/local-files.ts @@ -1,5 +1,5 @@ -import { open, readdir, realpath, stat } from 'node:fs/promises' -import { homedir } from 'node:os' +import { constants } from 'node:fs' +import { lstat, open, readdir, stat } from 'node:fs/promises' import { basename, isAbsolute, join, relative, resolve, sep } from 'node:path' import type { DesktopLocalFileEntry, @@ -11,6 +11,7 @@ import { MAX_DESKTOP_IMPORT_FILE_BYTES } from '@sim/desktop-bridge' import { getErrorMessage } from '@sim/utils/errors' import { isRecordLike } from '@sim/utils/object' import { PDFDocument } from 'pdf-lib' +import type { LocalFileAccess } from '@/main/local-file-permissions' const CHUNK_BYTES = 8 * 1024 * 1024 const MAX_ENTRIES = 1000 @@ -21,16 +22,6 @@ export interface LocalFileAuthorization { args: Record } -/** Resolve normal native paths; macOS, not Sim folder grants, owns filesystem access. */ -function nativePath(value: unknown): string { - if (typeof value !== 'string' || value.length > 4096 || value.includes('\0')) - throw new Error('A native absolute path or ~/ path is required.') - const path = - value === '~' ? homedir() : value.startsWith('~/') ? join(homedir(), value.slice(2)) : value - if (!isAbsolute(path)) throw new Error('Use an absolute path or ~/ path.') - return resolve(path) -} - function revision(info: Awaited>): string { return `${info.dev}:${info.ino}:${info.size}:${info.mtimeMs}` } @@ -48,7 +39,35 @@ function assertImportPath(root: string, candidate: string): void { throw new Error('The file is outside this import source.') } -async function inspect(path: string, args: Record): Promise { +async function openApprovedFile(path: string, access: LocalFileAccess) { + const canonical = await access.resolve(path) + const file = await open( + canonical, + constants.O_RDONLY | constants.O_NOFOLLOW | constants.O_NONBLOCK + ) + try { + const info = await file.stat() + const verified = await access.resolve(path) + const current = await lstat(verified) + if ( + !info.isFile() || + canonical !== verified || + info.dev !== current.dev || + info.ino !== current.ino + ) + throw new Error('The local file changed while it was being opened. Try again.') + return file + } catch (error) { + await file.close() + throw error + } +} + +async function inspect( + path: string, + args: Record, + access: LocalFileAccess +): Promise { const info = await stat(path) if (info.isDirectory()) { const entries = await readdir(path, { withFileTypes: true }) @@ -73,8 +92,9 @@ async function inspect(path: string, args: Record): Promise): Promise + args: Record, + access: LocalFileAccess ): Promise { if (typeof args.targetWorkspaceId !== 'string') throw new Error('A target workspace is required.') - const root = await realpath(path) + const root = await access.resolve(path) const entries: DesktopLocalFileEntry[] = [] async function walk(current: string, ancestors: ReadonlySet): Promise { if (entries.length >= MAX_ENTRIES) throw new Error( 'The directory exceeds 1,000 entries. Import smaller subdirectories separately.' ) - const canonical = await realpath(current) + const canonical = await access.resolve(current) assertImportPath(root, canonical) const info = await stat(canonical) if (!info.isDirectory() && !info.isFile()) @@ -210,20 +231,27 @@ async function manifest( } } -/** Calls have already been authorized against the pending server record by the IPC boundary. */ +/** Requires both a pending server call and a main-process grant before returning local data. */ export async function executeLocalFileRequest( request: unknown, - authorization: LocalFileAuthorization + authorization: LocalFileAuthorization, + access: LocalFileAccess ): Promise { try { if (!isRecordLike(request)) throw new Error('Invalid local file request.') - const path = nativePath(authorization.args.path) - if (request.operation === 'read' && authorization.toolName === 'read_local_file') - return { ok: true, data: await inspect(path, authorization.args) } + const path = await access.resolve(access.path) + if (request.operation === 'read' && authorization.toolName === 'read_local_file') { + const data = await inspect(path, authorization.args, access) + await access.resolve(path) + return { ok: true, data } + } if (authorization.toolName !== 'import_local_files') throw new Error('The operation does not match the pending tool call.') - if (request.operation === 'manifest') - return { ok: true, data: await manifest(path, authorization.args) } + if (request.operation === 'manifest') { + const data = await manifest(path, authorization.args, access) + await access.resolve(path) + return { ok: true, data } + } if ( request.operation !== 'chunk' || typeof request.relativePath !== 'string' || @@ -232,11 +260,11 @@ export async function executeLocalFileRequest( throw new Error('Invalid file chunk request.') const child = resolve(path, request.relativePath) assertImportPath(path, child) - const root = await realpath(path) - const canonical = await realpath(child) + const root = await access.resolve(path) + const canonical = await access.resolve(child) assertImportPath(root, canonical) const offset = boundedInteger(request.offset, 0, Number.MAX_SAFE_INTEGER) - const file = await open(canonical, 'r') + const file = await openApprovedFile(canonical, access) try { const info = await file.stat() if (!info.isFile() || revision(info) !== request.revision) @@ -248,6 +276,7 @@ export async function executeLocalFileRequest( const { bytesRead } = await file.read(buffer, 0, buffer.length, offset) if (revision(await file.stat()) !== request.revision) throw new Error('The source file changed during import.') + await access.resolve(child) return { ok: true, data: { diff --git a/packages/desktop-bridge/src/local-files.ts b/packages/desktop-bridge/src/local-files.ts index c5820a49721..92ff8916c7a 100644 --- a/packages/desktop-bridge/src/local-files.ts +++ b/packages/desktop-bridge/src/local-files.ts @@ -1,6 +1,6 @@ export const MAX_DESKTOP_IMPORT_FILE_BYTES = 64 * 1024 * 1024 -/** Native desktop file operations use OS permissions and canonical pending chat calls. */ +/** Native desktop file operations require local consent and canonical pending chat calls. */ export type DesktopLocalFileRequest = | { operation: 'read' | 'manifest'; toolCallId: string } | { From 9965e8538b11c55e7b882a2b47b88b4c237077de Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 26 Sep 2026 15:07:25 -0700 Subject: [PATCH 2/5] fix(desktop): remember folder permissions across chats --- apps/desktop/README.md | 6 +- apps/desktop/e2e/local-files.spec.ts | 153 ++++++++++----- apps/desktop/src/main/index.ts | 3 + apps/desktop/src/main/ipc.ts | 6 +- .../src/main/local-file-permissions.ts | 180 +++++++----------- apps/desktop/src/main/local-files.ts | 2 +- .../src/main/local-filesystem-grant-store.ts | 9 + apps/desktop/src/main/local-filesystem.ts | 135 ++++++++++++- apps/desktop/src/main/menu.test.ts | 1 + apps/desktop/src/main/menu.ts | 9 + 10 files changed, 323 insertions(+), 181 deletions(-) diff --git a/apps/desktop/README.md b/apps/desktop/README.md index 9b3af8618df..10a17fad040 100644 --- a/apps/desktop/README.md +++ b/apps/desktop/README.md @@ -182,15 +182,15 @@ Copilot can inspect user-selected local directories through the ordinary VFS too - **Explicit and read-only:** only a user click may open the native folder picker or revoke a grant; model tool calls cannot do either. There are no write/delete/execute/upload operations. - **Remembered securely:** grants are encrypted in Electron's private app data with OS-backed `safeStorage` and restored with the same opaque URI after a normal app restart. (A security-scoped bookmark is stored alongside each grant, but it is a no-op in the current Developer ID build — only the macOS App Sandbox consumes it — and is kept purely for forward-compatibility should a sandboxed/MAS build ever ship.) There is no plaintext fallback: when secure storage is unavailable, the returned mount has `remembered: false` and lasts only for that app session. -- **Revocable:** Desktop settings removes one grant. All grants are removed on explicit sign-out or server-origin change so another Sim account or server cannot inherit them. Normal app quit only releases active OS handles and keeps the encrypted grants. +- **Revocable:** File → Folder Access adds folders and removes individual grants. All grants are removed on explicit sign-out or server-origin change so another Sim account or server cannot inherit them. Normal app quit only releases active OS handles and keeps the encrypted grants. - **Opaque:** the model sees canonical paths such as `user-local/Project--/README.md`, never host paths or internal `localfs://` URIs. Electron resolves every request, checks lexical and realpath containment, and refuses symlink escapes. - **Desktop-only:** the web app advertises `desktopCapabilities.localFilesystem` only when the Electron bridge is present. Mothership adds the `user-local/` prompt surface and per-call client routing only for that capability, including delegated and resumed work. - **Bound to a live Copilot call:** before a native read/search or browser action, Electron asks the authenticated Sim origin for the pending tool-call record. Local requests must exactly match its persisted operation, path, and options; browser actions run with the persisted arguments rather than renderer-supplied ones. Completed, failed, and aborted runs are rejected. - **Abort-aware and bounded:** stop/cancel propagates to active native scans and reads. File size, aggregate grep bytes, line, result, traversal-depth, and scan-count limits remain enforced in Electron, and unsafe regular expressions are rejected before execution. -The native `read_local_file` and `import_local_files` tools also accept absolute or `~/` paths. Before accessing a new file or folder, Electron displays a bundled, isolated permission dialog showing the resolved path and the connected server. **Allow for this chat** grants access to that file, or that folder and its contents, for the current desktop session. Closing or declining the dialog returns no contents. Grants are scoped to the server, account generation, chat, and operation; import grants also bind the destination workspace and folder. A read grant never authorizes an import. Grants are not persisted and expire on sign-out, server changes, or app restart. +The native `read_local_file` and `import_local_files` tools also accept absolute or `~/` paths. They reuse the same remembered folder grants as the VFS tools. For an unapproved path, Electron displays a bundled, isolated dialog showing the canonical folder and connected server. **Allow folder** grants read and import access to that folder and its subfolders across chats and normal app restarts. A file request proposes its containing folder explicitly; no wider folder is approved silently. Closing or declining the dialog returns no contents. Users can add or forget folders through **File → Folder Access**. As with VFS grants, sign-out and server changes clear access, and unavailable secure storage limits persistence to the app session. New consent prompts are serialized, but reads of approved folders proceed independently. -Approved native reads can return bounded text, directory listings, images, or PDFs to the chat. Approved imports transfer file bytes to Workspace Files. Electron revalidates the pending call after consent, checks canonical containment and grant identity throughout the operation, and opens files with no-follow and descriptor identity checks. A model or hosted renderer cannot answer the local consent dialog. These permissions govern the native file tools; the separately enabled terminal still runs with the user's OS privileges. +Approved native reads can return bounded text, directory listings, images, or PDFs to the chat. Approved imports transfer file bytes to Workspace Files. Electron revalidates every pending call before using a grant, including remembered grants, checks canonical containment and grant identity throughout the operation, and opens files with no-follow and descriptor identity checks. A model or hosted renderer cannot answer the local consent dialog. These permissions govern the native file tools; the separately enabled terminal still runs with the user's OS privileges. ## Auto-update, channels, rollout, rollback diff --git a/apps/desktop/e2e/local-files.spec.ts b/apps/desktop/e2e/local-files.spec.ts index c927a57d866..c7c729dc42b 100644 --- a/apps/desktop/e2e/local-files.spec.ts +++ b/apps/desktop/e2e/local-files.spec.ts @@ -16,7 +16,7 @@ import type { DesktopLocalFileRequest, SimDesktopApi } from '@sim/desktop-bridge const DESKTOP_DIR = fileURLToPath(new URL('..', import.meta.url)) -test('native file tools require local consent and reuse only the approved chat and path', async () => { +test('native file tools remember folder consent across chats and restarts until revoked', async () => { const root = mkdtempSync(join(tmpdir(), 'sim-native-files-e2e-')) const source = join(root, 'Reports') const outside = join(root, 'Reports-other') @@ -28,7 +28,7 @@ test('native file tools require local consent and reuse only the approved chat a 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mP8/x8AAusB9Y9Zl1sAAAAASUVORK5CYII=' writeFileSync(join(source, 'image.png'), Buffer.from(png, 'base64')) const claimed = new Set() - const authorizedCalls = new Set() + const expireAfterAuthorization = new Set() let signedIn = true const calls: Record< string, @@ -84,11 +84,11 @@ test('native file tools require local consent and reuse only the approved chat a response.writeHead(call ? 409 : 403, { 'Content-Type': 'application/json' }).end('{}') return } - authorizedCalls.add(input.toolCallId) if (input.claim) claimed.add(input.toolCallId) response .writeHead(200, { 'Content-Type': 'application/json' }) .end(JSON.stringify({ ...call, chatId: call.chatId ?? 'org-chat' })) + if (expireAfterAuthorization.has(input.toolCallId)) calls[input.toolCallId] = undefined return } response @@ -98,21 +98,27 @@ test('native file tools require local consent and reuse only the approved chat a ? { 'Set-Cookie': 'better-auth.session_token=fixture; HttpOnly; SameSite=Lax; Path=/' } : {}), }) - .end('Local file fixture

Local files

') + .end(`Local file fixture

Local files

+ `) }) await new Promise((resolve) => server?.listen(0, '127.0.0.1', resolve)) const address = server.address() if (!address || typeof address === 'string') throw new Error('Missing fixture address') - app = await electron.launch({ - args: ['.'], - cwd: DESKTOP_DIR, - env: { - ...process.env, - SIM_DESKTOP_ORIGIN: `http://127.0.0.1:${address.port}`, - SIM_DESKTOP_USER_DATA: join(root, 'profile'), - }, - }) - const window = await app.firstWindow() + const launch = () => + electron.launch({ + args: ['.', '--use-mock-keychain'], + cwd: DESKTOP_DIR, + env: { + ...process.env, + SIM_DESKTOP_ORIGIN: `http://127.0.0.1:${address.port}`, + SIM_DESKTOP_USER_DATA: join(root, 'profile'), + }, + }) + app = await launch() + let window = await app.firstWindow() await expect(window.getByRole('heading')).toHaveText('Local files') const invoke = (input: DesktopLocalFileRequest) => window.evaluate(async (request) => { @@ -135,6 +141,7 @@ test('native file tools require local consent and reuse only the approved chat a }) const deniedPrompt = app.waitForEvent('window', { timeout: 10_000 }) const deniedRead = invoke({ operation: 'read', toolCallId: 'text' }) + void deniedRead.catch(() => {}) const denial = await deniedPrompt await expect(denial.getByRole('button', { name: "Don't allow", exact: true })).toBeFocused() await denial.screenshot({ @@ -147,12 +154,14 @@ test('native file tools require local consent and reuse only the approved chat a const folderPrompt = app.waitForEvent('window') const folderRead = invoke({ operation: 'read', toolCallId: 'directory' }) + void folderRead.catch(() => {}) const folderConsent = await folderPrompt const queuedRead = invoke({ operation: 'read', toolCallId: 'text' }) + void queuedRead.catch(() => {}) expect( await folderConsent.evaluate(() => typeof (globalThis as { simDesktop?: unknown }).simDesktop) ).toBe('undefined') - await folderConsent.getByRole('button', { name: 'Allow for this chat', exact: true }).click() + await folderConsent.getByRole('button', { name: 'Allow folder', exact: true }).click() expect(await folderRead).toMatchObject({ ok: true, data: { representation: 'directory' } }) expect(await queuedRead).toMatchObject({ ok: true, data: { text: 'native file contents' } }) const canonicalRequest = { @@ -168,29 +177,36 @@ test('native file tools require local consent and reuse only the approved chat a ok: true, data: { observations: [{ mediaType: 'image/png', data: png }] }, }) - const runningApp = app const requestPermission = async (request: DesktopLocalFileRequest) => { - const shown = runningApp.waitForEvent('window', { timeout: 10_000 }) + if (!app) throw new Error('Desktop app is not running') + const shown = app.waitForEvent('window', { timeout: 10_000 }) const result = invoke(request) void result.catch(() => {}) const prompt = await shown await expect(prompt.getByRole('button', { name: "Don't allow", exact: true })).toBeVisible() return { prompt, result } } - await test.step('a folder grant does not authorize another chat or a symlink escape', async () => { - const otherChat = await requestPermission({ operation: 'read', toolCallId: 'otherChat' }) - const dismissed = otherChat.prompt.waitForEvent('close') - await otherChat.prompt - .getByRole('button', { name: "Don't allow", exact: true }) - .press('Escape') - .catch(() => {}) - await dismissed - expect(await otherChat.result).toMatchObject({ ok: false }) + await test.step('a folder grant works in another chat but does not permit symlink escapes', async () => { + expect(await invoke({ operation: 'read', toolCallId: 'otherChat' })).toMatchObject({ + ok: true, + data: { text: 'native file contents' }, + }) + writeFileSync(join(source, 'empty', 'new.txt'), 'new file in a subfolder') + calls.nested = { + toolName: 'read_local_file', + args: { path: join(source, 'empty', 'new.txt') }, + chatId: 'another-chat', + } + expect(await invoke({ operation: 'read', toolCallId: 'nested' })).toMatchObject({ + ok: true, + data: { text: 'new file in a subfolder' }, + }) + rmSync(join(source, 'empty', 'new.txt')) symlinkSync(join(outside, 'private.txt'), join(source, 'linked.txt')) calls.escape = { toolName: 'read_local_file', args: { path: join(source, 'linked.txt') } } const escapedRead = await requestPermission({ operation: 'read', toolCallId: 'escape' }) await expect(escapedRead.prompt.getByRole('dialog')).toContainText( - JSON.stringify(realpathSync(join(outside, 'private.txt'))) + JSON.stringify(realpathSync(outside)) ) await escapedRead.prompt.getByRole('button', { name: "Don't allow", exact: true }).click() expect(await escapedRead.result).toMatchObject({ ok: false }) @@ -202,21 +218,23 @@ test('native file tools require local consent and reuse only the approved chat a const stale = await requestPermission({ operation: 'read', toolCallId: 'stale' }) if (changed) calls.stale.args.path = join(source, 'report.txt') else calls.stale = undefined - await stale.prompt.getByRole('button', { name: 'Allow for this chat', exact: true }).click() + await stale.prompt.getByRole('button', { name: 'Allow folder', exact: true }).click() expect(await stale.result).toMatchObject({ ok: false }) } }) - await test.step('a queued call is revalidated even when its folder is already approved', async () => { + await test.step('an unanswered prompt does not block approved folders', async () => { calls.blocker = { toolName: 'read_local_file', args: { path: join(outside, 'private.txt') } } - calls.queued = { toolName: 'read_local_file', args: { path: join(source, 'report.txt') } } const blocker = await requestPermission({ operation: 'read', toolCallId: 'blocker' }) - const queued = invoke({ operation: 'read', toolCallId: 'queued' }) - void queued.catch(() => {}) - await expect.poll(() => authorizedCalls.has('queued')).toBe(true) - calls.queued = undefined + expect(await invoke({ operation: 'read', toolCallId: 'text' })).toMatchObject({ ok: true }) await blocker.prompt.getByRole('button', { name: "Don't allow", exact: true }).click() expect(await blocker.result).toMatchObject({ ok: false }) - expect(await queued).toMatchObject({ ok: false }) + }) + await test.step('cancelled calls cannot reuse an approved folder', async () => { + calls.expired = { toolName: 'read_local_file', args: { path: join(source, 'report.txt') } } + expireAfterAuthorization.add('expired') + expect(await invoke({ operation: 'read', toolCallId: 'expired' })).toMatchObject({ + ok: false, + }) }) await test.step('replacing the proposed folder during consent does not expose its new target', async () => { const proposed = join(root, 'Proposed') @@ -226,16 +244,10 @@ test('native file tools require local consent and reuse only the approved chat a renameSync(proposed, join(root, 'Original')) mkdirSync(proposed) writeFileSync(join(proposed, 'unapproved.txt'), 'replacement folder contents') - await retargeted.prompt - .getByRole('button', { name: 'Allow for this chat', exact: true }) - .click() + await retargeted.prompt.getByRole('button', { name: 'Allow folder', exact: true }).click() expect(await retargeted.result).toMatchObject({ ok: false }) }) - const importPrompt = app.waitForEvent('window') - const importing = invoke({ operation: 'manifest', toolCallId: 'import' }) - const importConsent = await importPrompt - await importConsent.getByRole('button', { name: 'Allow for this chat', exact: true }).click() - const result = await importing + const result = await invoke({ operation: 'manifest', toolCallId: 'import' }) if (!result.ok || result.data.kind !== 'manifest') throw new Error(JSON.stringify(result)) expect(result.data.targetWorkspaceId).toBe('target-workspace') expect(result.data.entries.map((entry) => entry.relativePath)).toEqual([ @@ -279,22 +291,59 @@ test('native file tools require local consent and reuse only the approved chat a ok: false, code: 'ALREADY_STARTED', }) - await test.step('import approval is bound to its destination workspace', async () => { + await test.step('an approved folder permits imports without repeated destination prompts', async () => { calls.otherImport = { toolName: 'import_local_files', args: { path: source, targetWorkspaceId: 'other-workspace' }, } - const otherImport = await requestPermission({ - operation: 'manifest', - toolCallId: 'otherImport', + expect(await invoke({ operation: 'manifest', toolCallId: 'otherImport' })).toMatchObject({ + ok: true, }) - await otherImport.prompt.getByRole('button', { name: "Don't allow", exact: true }).click() - expect(await otherImport.result).toMatchObject({ ok: false }) + }) + await test.step('folder permissions survive restarting the desktop app', async () => { + await app?.close() + app = await launch() + window = await app.firstWindow() + await expect(window.getByRole('heading')).toHaveText('Local files') + expect(await invoke({ operation: 'read', toolCallId: 'text' })).toMatchObject({ ok: true }) + }) + await test.step('forgetting a folder revokes native reads and survives restart', async () => { + await window.getByRole('button', { name: 'Forget folders', exact: true }).click() + await expect(window.getByRole('button', { name: 'Forgotten', exact: true })).toBeVisible() + await app?.close() + app = await launch() + window = await app.firstWindow() + await expect(window.getByRole('heading')).toHaveText('Local files') + const revoked = await requestPermission({ operation: 'read', toolCallId: 'text' }) + await revoked.prompt.getByRole('button', { name: 'Allow folder', exact: true }).click() + expect(await revoked.result).toMatchObject({ ok: true }) + }) + + await test.step('a remembered grant does not follow a replaced folder after restart', async () => { + await app?.close() + renameSync(source, join(root, 'Original-reports')) + mkdirSync(source) + writeFileSync(join(source, 'report.txt'), 'replacement contents') + app = await launch() + window = await app.firstWindow() + await expect(window.getByRole('heading')).toHaveText('Local files') + const replaced = await requestPermission({ operation: 'read', toolCallId: 'text' }) + await replaced.prompt.getByRole('button', { name: "Don't allow", exact: true }).click() + expect(await replaced.result).toMatchObject({ ok: false }) + rmSync(source, { recursive: true }) + renameSync(join(root, 'Original-reports'), source) + const restored = await requestPermission({ operation: 'read', toolCallId: 'text' }) + await restored.prompt.getByRole('button', { name: 'Allow folder', exact: true }).click() + expect(await restored.result).toMatchObject({ ok: true }) }) - await test.step('sign-out revokes chat grants before the next account session', async () => { - await window.evaluate(async () => { - await fetch('/api/auth/sign-out', { method: 'POST' }) + await test.step('sign-out revokes remembered grants before the next account session', async () => { + await app?.evaluate(({ Menu }) => { + const item = Menu.getApplicationMenu() + ?.items.flatMap((entry) => entry.submenu?.items ?? []) + .find((entry) => entry.label === 'Sign Out') + if (!item) throw new Error('Sign Out menu item missing') + item.click() }) await expect(window).toHaveURL(`http://127.0.0.1:${address.port}/login`) signedIn = true diff --git a/apps/desktop/src/main/index.ts b/apps/desktop/src/main/index.ts index b3a4618d62b..e97f7bb5f92 100644 --- a/apps/desktop/src/main/index.ts +++ b/apps/desktop/src/main/index.ts @@ -849,6 +849,9 @@ function main(): void { allowHttpLocalhost, openSettings, openServerSettings: () => serverWindow.open(), + openFolderAccess: (parent) => { + if (accountDataAvailable()) localFilesystem.showAccessMenu(parent) + }, newWindow: () => void createAndLoadAppWindow(), newChat: () => void openMainWindowAt(newChatRoute(config.get('lastRoute'))), handleFocusedResourceShortcut: (win, shortcut) => diff --git a/apps/desktop/src/main/ipc.ts b/apps/desktop/src/main/ipc.ts index 8b44dcb5ee7..ade293228c6 100644 --- a/apps/desktop/src/main/ipc.ts +++ b/apps/desktop/src/main/ipc.ts @@ -86,9 +86,9 @@ import { isSafeInternalPath } from '@/main/config' import type { DesktopSettingsService } from '@/main/desktop-settings' import { isDesktopPreferenceKey } from '@/main/desktop-settings' import { hasRecentDeliberateInput, hasRecentDiscreteInput } from '@/main/input-activity' -import { type LocalFileAccess, LocalFilePermissions } from '@/main/local-file-permissions' +import { LocalFilePermissions } from '@/main/local-file-permissions' import { executeLocalFileRequest } from '@/main/local-files' -import type { LocalFilesystemService } from '@/main/local-filesystem' +import type { LocalFileAccess, LocalFilesystemService } from '@/main/local-filesystem' import { isAppOrigin, openExternalSafe } from '@/main/navigation' import type { ScopedEventRouter } from '@/main/scoped-event-router' import type { TerminalRegistry } from '@/main/terminal/registry' @@ -597,7 +597,7 @@ async function authorizeLocalFilesystemTool( * unvalidated args they must parse themselves. */ export function registerIpcHandlers(deps: IpcDeps): void { - const localFilePermissions = new LocalFilePermissions() + const localFilePermissions = new LocalFilePermissions(deps.localFilesystem) const browserScopeBySender = new WeakMap() const terminalScopeBySender = new WeakMap() const browserPendingScopesBySender = new WeakMap>() diff --git a/apps/desktop/src/main/local-file-permissions.ts b/apps/desktop/src/main/local-file-permissions.ts index 0e3b1338ff9..1893cc98a11 100644 --- a/apps/desktop/src/main/local-file-permissions.ts +++ b/apps/desktop/src/main/local-file-permissions.ts @@ -1,12 +1,12 @@ import { lstat, realpath, stat } from 'node:fs/promises' import { homedir } from 'node:os' -import { isAbsolute, join, relative, resolve, sep } from 'node:path' +import { dirname, isAbsolute, join, resolve } from 'node:path' import { isDesktopScopeId } from '@sim/desktop-bridge' import type { BrowserWindow } from 'electron' import { showShellDialog } from '@/main/dialogs' import type { LocalFileAuthorization } from '@/main/local-files' +import type { LocalFileAccess, LocalFilesystemService } from '@/main/local-filesystem' -const MAX_GRANTS = 256 const MAX_PENDING_REQUESTS = 32 interface LocalFilePermissionContext { @@ -17,19 +17,6 @@ interface LocalFilePermissionContext { revalidate: () => Promise } -interface LocalFileGrant { - scope: string - path: string - directory: boolean - dev: number - ino: number -} - -export interface LocalFileAccess { - path: string - resolve: (path: string) => Promise -} - function nativePath(value: unknown): string { if (typeof value !== 'string' || value.length > 4096 || value.includes('\0')) throw new Error('A native absolute path or ~/ path is required.') @@ -39,34 +26,44 @@ function nativePath(value: unknown): string { return resolve(path) } -function contains(grant: LocalFileGrant, path: string): boolean { - if (path === grant.path) return true - if (!grant.directory) return false - const rel = relative(grant.path, path) - return !isAbsolute(rel) && rel !== '..' && !rel.startsWith(`..${sep}`) -} - function assertCurrent(context: LocalFilePermissionContext): void { if (context.parent.isDestroyed() || !context.isCurrent()) throw new Error('This local file request expired. Ask again in the current chat.') } -/** Chat grants live only in this desktop session and are never writable by the hosted renderer. */ +async function revalidate(context: LocalFilePermissionContext): Promise { + assertCurrent(context) + if (!(await context.revalidate())) + throw new Error('This local file tool call is no longer pending or its arguments changed.') + assertCurrent(context) +} + +/** Serializes new consent prompts while remembered folder access remains concurrent. */ export class LocalFilePermissions { - private grants: LocalFileGrant[] = [] - private generation = -1 private queue: Promise = Promise.resolve() private pending = 0 + constructor(private readonly filesystem: LocalFilesystemService) {} + async authorize( - authorization: LocalFileAuthorization & { chatId: string }, + authorization: LocalFileAuthorization, context: LocalFilePermissionContext ): Promise { + assertCurrent(context) + if ( + authorization.toolName === 'import_local_files' && + (!isDesktopScopeId(authorization.args.targetWorkspaceId) || + (authorization.args.folderId !== undefined && + !isDesktopScopeId(authorization.args.folderId))) + ) + throw new Error('A valid destination workspace and folder are required for imports.') + const path = await realpath(nativePath(authorization.args.path)) + const existing = await this.filesystem.nativeAccess(path) + if (existing) return this.authorizedAccess(existing, context) if (this.pending >= MAX_PENDING_REQUESTS) throw new Error('Too many local file requests are waiting for permission. Try again later.') - const wasQueued = this.pending > 0 this.pending++ - const pending = this.queue.then(() => this.authorizeNext(authorization, context, wasQueued)) + const pending = this.queue.then(() => this.requestFolder(path, context)) this.queue = pending.then( () => undefined, () => undefined @@ -78,98 +75,53 @@ export class LocalFilePermissions { } } - private async authorizeNext( - authorization: LocalFileAuthorization & { chatId: string }, - context: LocalFilePermissionContext, - wasQueued: boolean + private async requestFolder( + path: string, + context: LocalFilePermissionContext ): Promise { - assertCurrent(context) - if (this.generation !== context.generation) { - this.grants = [] - this.generation = context.generation - } - const importing = authorization.toolName === 'import_local_files' - if ( - importing && - (!isDesktopScopeId(authorization.args.targetWorkspaceId) || - (authorization.args.folderId !== undefined && - !isDesktopScopeId(authorization.args.folderId))) - ) - throw new Error('A valid destination workspace and folder are required for imports.') - const scope = JSON.stringify([ - context.origin, - authorization.chatId, - authorization.toolName, - ...(importing ? [authorization.args.targetWorkspaceId, authorization.args.folderId] : []), - ]) - const path = await realpath(nativePath(authorization.args.path)) + await revalidate(context) + const existing = await this.filesystem.nativeAccess(path) + if (existing) return this.authorizedAccess(existing, context) const info = await stat(path) if (!info.isFile() && !info.isDirectory()) throw new Error('The path is not a regular file or directory.') - let grant = this.grants.find((entry) => entry.scope === scope && contains(entry, path)) - if (grant) { - if (wasQueued && !(await context.revalidate())) - throw new Error('This local file tool call is no longer pending or its arguments changed.') - const root = await lstat(grant.path) - if (root.dev !== grant.dev || root.ino !== grant.ino || root.isSymbolicLink()) { - this.grants = this.grants.filter((entry) => entry !== grant) - grant = undefined - } - } - if (!grant) { - if (this.grants.length >= MAX_GRANTS) - throw new Error( - 'Restart Sim to clear this session’s local file permissions before adding more.' - ) - assertCurrent(context) - const kind = info.isDirectory() ? 'folder' : 'file' - const displayedPath = JSON.stringify(path).replace( - /[\u202a-\u202e\u2066-\u2069]/g, - (character) => `\\u${character.charCodeAt(0).toString(16).padStart(4, '0')}` - ) - const result = await showShellDialog(context.parent, { - title: importing ? `Import this ${kind}?` : `Read this ${kind}?`, - message: displayedPath, - detail: [ - importing - ? `Sim will upload this ${kind}${info.isDirectory() ? ' and its contents' : ''} to your workspace on ${context.origin}.` - : `Sim will read this ${kind}${info.isDirectory() ? ' and its contents' : ''} and send the results to ${context.origin} for this chat.`, - ...(importing ? [`Destination workspace: ${authorization.args.targetWorkspaceId}`] : []), - 'This permission applies only to this chat and ends when Sim closes.', - ].join('\n\n'), - buttons: ['Allow for this chat', "Don't allow"], - defaultId: 1, - cancelId: 1, - }) - assertCurrent(context) - if (result.response !== 0) throw new Error('The user did not allow this local file access.') - if (!(await context.revalidate())) - throw new Error('This local file tool call is no longer pending or its arguments changed.') - grant = { scope, path, directory: info.isDirectory(), dev: info.dev, ino: info.ino } - await this.resolve(grant, path, context) - this.grants.push(grant) - } - await this.resolve(grant, path, context) - const approvedGrant = grant - return { path, resolve: (candidate) => this.resolve(approvedGrant, candidate, context) } + const folder = info.isDirectory() ? path : dirname(path) + const root = await lstat(folder) + if (!root.isDirectory()) throw new Error('The folder is no longer available.') + const displayedPath = JSON.stringify(folder).replace( + /[\u202a-\u202e\u2066-\u2069]/g, + (character) => `\\u${character.charCodeAt(0).toString(16).padStart(4, '0')}` + ) + assertCurrent(context) + const result = await showShellDialog(context.parent, { + title: 'Allow access to this folder?', + message: displayedPath, + detail: `Sim can read files in this folder and its subfolders, use them across chats, and import them into your workspaces on ${context.origin}.\n\nManage or remove access in File → Folder Access.`, + buttons: ['Allow folder', "Don't allow"], + defaultId: 1, + cancelId: 1, + }) + assertCurrent(context) + if (result.response !== 0) throw new Error('The user did not allow this local file access.') + await revalidate(context) + await this.filesystem.grantDirectory({ path: folder }, context.generation, root) + const access = await this.filesystem.nativeAccess(path) + if (!access) throw new Error('The approved folder is no longer available.') + return this.authorizedAccess(access, context) } - private async resolve( - grant: LocalFileGrant, - candidate: string, + private async authorizedAccess( + access: LocalFileAccess, context: LocalFilePermissionContext - ): Promise { - assertCurrent(context) - const root = await lstat(grant.path) - if ( - root.dev !== grant.dev || - root.ino !== grant.ino || - (grant.directory ? !root.isDirectory() : !root.isFile()) - ) - throw new Error('The approved file or folder changed. Ask again to request access.') - const path = await realpath(candidate) - if (!contains(grant, path)) throw new Error('This path is outside the approved file or folder.') - assertCurrent(context) - return path + ): Promise { + await revalidate(context) + const resolveApproved = async (path: string): Promise => { + assertCurrent(context) + const resolved = await access.resolve(path) + assertCurrent(context) + return resolved + } + await resolveApproved(access.path) + return { path: access.path, resolve: resolveApproved } } } diff --git a/apps/desktop/src/main/local-files.ts b/apps/desktop/src/main/local-files.ts index 10cfa6674e5..e76c86448b8 100644 --- a/apps/desktop/src/main/local-files.ts +++ b/apps/desktop/src/main/local-files.ts @@ -11,7 +11,7 @@ import { MAX_DESKTOP_IMPORT_FILE_BYTES } from '@sim/desktop-bridge' import { getErrorMessage } from '@sim/utils/errors' import { isRecordLike } from '@sim/utils/object' import { PDFDocument } from 'pdf-lib' -import type { LocalFileAccess } from '@/main/local-file-permissions' +import type { LocalFileAccess } from '@/main/local-filesystem' const CHUNK_BYTES = 8 * 1024 * 1024 const MAX_ENTRIES = 1000 diff --git a/apps/desktop/src/main/local-filesystem-grant-store.ts b/apps/desktop/src/main/local-filesystem-grant-store.ts index 67216976840..188c2469b60 100644 --- a/apps/desktop/src/main/local-filesystem-grant-store.ts +++ b/apps/desktop/src/main/local-filesystem-grant-store.ts @@ -21,6 +21,8 @@ export interface PersistedLocalFilesystemGrant { id: string name: string rootPath: string + dev?: number + ino?: number bookmark?: string } @@ -55,6 +57,13 @@ function isPersistedGrant(value: unknown): value is PersistedLocalFilesystemGran grant.rootPath.length > 0 && grant.rootPath.length <= MAX_GRANT_PATH_LENGTH && !grant.rootPath.includes('\0') && + ((grant.dev === undefined && grant.ino === undefined) || + (typeof grant.dev === 'number' && + Number.isSafeInteger(grant.dev) && + grant.dev >= 0 && + typeof grant.ino === 'number' && + Number.isSafeInteger(grant.ino) && + grant.ino >= 0)) && (grant.bookmark === undefined || (typeof grant.bookmark === 'string' && grant.bookmark.length > 0 && diff --git a/apps/desktop/src/main/local-filesystem.ts b/apps/desktop/src/main/local-filesystem.ts index 46a6c30b9fb..7f58135dbe6 100644 --- a/apps/desktop/src/main/local-filesystem.ts +++ b/apps/desktop/src/main/local-filesystem.ts @@ -17,10 +17,12 @@ import { MAX_GREP_RESULTS, MAX_READ_LINES, } from '@sim/desktop-bridge/local-filesystem-limits' +import { getErrorMessage } from '@sim/utils/errors' import { generateId } from '@sim/utils/id' import { isRecordLike } from '@sim/utils/object' import { escapeRegExp } from '@sim/utils/string' -import { app, dialog, shell } from 'electron' +import type { BrowserWindow } from 'electron' +import { app, dialog, Menu, shell } from 'electron' import micromatch from 'micromatch' import safeRegex from 'safe-regex2' import { @@ -30,6 +32,7 @@ import { runAccountDataMutation, waitForAccountDataMutations, } from '@/main/account-data-generation' +import { showShellDialog } from '@/main/dialogs' import type { LocalFilesystemGrantStore, PersistedLocalFilesystemGrant, @@ -77,6 +80,8 @@ class LocalFilesystemError extends Error { interface GrantedMount extends LocalFilesystemMount { rootPath: string + dev: number + ino: number bookmark?: string stopAccessing?: () => void } @@ -329,6 +334,11 @@ async function selectDirectoryEntries( return { entries, truncated: seen > entries.length } } +export interface LocalFileAccess { + path: string + resolve: (path: string) => Promise +} + export class LocalFilesystemService { private readonly mounts = new Map() private readonly activeRequests = new Map() @@ -605,7 +615,18 @@ export class LocalFilesystemService { throw new LocalFilesystemError('CANCELLED', 'The folder request expired during sign-out.') } - const selected = typeof selection === 'string' ? { path: selection } : selection + return this.grantDirectory( + typeof selection === 'string' ? { path: selection } : selection, + generation + ) + } + + /** Records a folder selected through trusted desktop UI, with the identity shown at consent. */ + async grantDirectory( + selected: SelectedDirectory, + generation: number, + expected?: { dev: number; ino: number } + ): Promise { const stopAccessing = selected.bookmark ? this.startAccessingBookmark(selected.bookmark) : undefined @@ -628,7 +649,24 @@ export class LocalFilesystemService { throw new LocalFilesystemError('NOT_A_DIRECTORY', 'The selected item is not a directory.') } + if ( + expected && + (rootPath !== selected.path || + rootStat.dev !== expected.dev || + rootStat.ino !== expected.ino) + ) { + throw new LocalFilesystemError( + 'ACCESS_DENIED', + 'The folder changed while awaiting permission. Request access again.' + ) + } const existing = [...this.mounts.values()].find((mount) => mount.rootPath === rootPath) + if (!existing && this.mounts.size >= 256) { + throw new LocalFilesystemError( + 'INVALID_REQUEST', + 'Forget an unused folder in File → Folder Access before adding more.' + ) + } const id = existing?.id ?? generateId() const bookmark = selected.bookmark ?? existing?.bookmark const nextStopAccessing = selected.bookmark @@ -643,6 +681,8 @@ export class LocalFilesystemService { name: basename(rootPath) || 'Local files', uri: localUri(id), rootPath, + dev: rootStat.dev, + ino: rootStat.ino, remembered: existing?.remembered ?? false, ...(bookmark ? { bookmark } : {}), ...(nextStopAccessing ? { stopAccessing: nextStopAccessing } : {}), @@ -667,6 +707,75 @@ export class LocalFilesystemService { } } + /** Resolves native tool paths through the same remembered grants as VFS reads. */ + async nativeAccess(candidate: string): Promise { + const path = await realpath(candidate) + for (const mount of this.mounts.values()) { + if (!isWithinRoot(mount.rootPath, path)) continue + try { + await this.assertMountCurrent(mount) + } catch { + continue + } + const resolveGranted = async (requested: string): Promise => { + await this.assertMountCurrent(mount) + const resolved = await realpath(requested) + if (!isWithinRoot(mount.rootPath, resolved)) { + throw new LocalFilesystemError( + 'ACCESS_DENIED', + 'This path is outside the approved folder.' + ) + } + await this.assertMountCurrent(mount) + return resolved + } + return { path, resolve: resolveGranted } + } + return null + } + + private async assertMountCurrent(mount: GrantedMount): Promise { + const root = await lstat(mount.rootPath) + if ( + this.mounts.get(mount.id) !== mount || + !root.isDirectory() || + root.dev !== mount.dev || + root.ino !== mount.ino + ) { + throw new LocalFilesystemError( + 'ACCESS_DENIED', + 'Folder access was removed or the folder changed. Request access again.' + ) + } + } + + /** Native controls share the same grant store and revocation path as the desktop bridge. */ + showAccessMenu(parent: BrowserWindow): void { + const generation = captureAccountDataGeneration() + const run = (operation: () => Promise) => { + if (!isAccountDataGenerationCurrent(generation) || parent.isDestroyed()) return + void operation().catch((error) => + showShellDialog(parent, { + title: 'Folder access', + message: getErrorMessage(error), + buttons: ['OK'], + }) + ) + } + Menu.buildFromTemplate([ + { label: 'Add Folder…', click: () => run(() => this.mountDirectory()) }, + { type: 'separator' }, + ...[...this.mounts.values()].map((mount) => ({ + label: mount.rootPath, + submenu: [ + { label: 'Show Folder', click: () => run(async () => this.revealMount(mount.uri)) }, + { label: 'Forget Folder', click: () => run(() => this.forgetMount(mount.uri)) }, + ], + })), + ...(this.mounts.size === 0 ? [{ label: 'No folders allowed', enabled: false }] : []), + ]).popup({ window: parent }) + } + private listMounts(): LocalFilesystemData { return { mounts: [...this.mounts.values()].map((mount) => this.publicMount(mount)) } } @@ -685,11 +794,11 @@ export class LocalFilesystemService { const generation = captureAccountDataGeneration() const grants = await this.grantStore.load() if (!isAccountDataGenerationCurrent(generation)) return - let skipped = false + let needsPersist = false for (const grant of grants) { if (!/^[a-zA-Z0-9-]{1,128}$/.test(grant.id) || this.mounts.has(grant.id)) { - skipped = true + needsPersist = true continue } const stopAccessing = grant.bookmark ? this.startAccessingBookmark(grant.bookmark) : undefined @@ -700,27 +809,34 @@ export class LocalFilesystemService { stopAccessing?.() return } - if (!rootStat.isDirectory()) { + if ( + !rootStat.isDirectory() || + rootPath !== grant.rootPath || + (grant.dev !== undefined && (grant.dev !== rootStat.dev || grant.ino !== rootStat.ino)) + ) { stopAccessing?.() - skipped = true + needsPersist = true continue } + if (grant.dev === undefined) needsPersist = true this.mounts.set(grant.id, { id: grant.id, name: basename(rootPath) || grant.name || 'Local files', uri: localUri(grant.id), rootPath, + dev: rootStat.dev, + ino: rootStat.ino, remembered: true, ...(grant.bookmark ? { bookmark: grant.bookmark } : {}), ...(stopAccessing ? { stopAccessing } : {}), }) } catch { stopAccessing?.() - skipped = true + needsPersist = true } } - if (skipped) { + if (needsPersist) { await runAccountDataMutation(generation, () => this.persistMounts()) } } @@ -730,6 +846,8 @@ export class LocalFilesystemService { id: mount.id, name: mount.name, rootPath: mount.rootPath, + dev: mount.dev, + ino: mount.ino, ...(mount.bookmark ? { bookmark: mount.bookmark } : {}), })) } @@ -921,6 +1039,7 @@ export class LocalFilesystemService { private async resolveUri(uri: string): Promise { const { mount, relativePath } = this.parseUri(uri) + await this.assertMountCurrent(mount) const lexicalPath = resolve(mount.rootPath, ...relativePath.split('/').filter(Boolean)) if (!isWithinRoot(mount.rootPath, lexicalPath)) { throw new LocalFilesystemError( diff --git a/apps/desktop/src/main/menu.test.ts b/apps/desktop/src/main/menu.test.ts index 7364b5aaa39..2eb8862dc32 100644 --- a/apps/desktop/src/main/menu.test.ts +++ b/apps/desktop/src/main/menu.test.ts @@ -20,6 +20,7 @@ function makeDeps(origin = 'https://sim.ai'): MenuDeps { allowHttpLocalhost: vi.fn(() => false), openSettings: vi.fn(), openServerSettings: vi.fn(), + openFolderAccess: vi.fn(), newWindow: vi.fn(), newChat: vi.fn(), handleFocusedResourceShortcut: vi.fn(() => false), diff --git a/apps/desktop/src/main/menu.ts b/apps/desktop/src/main/menu.ts index c7d61ec0ba6..2dee9b5c356 100644 --- a/apps/desktop/src/main/menu.ts +++ b/apps/desktop/src/main/menu.ts @@ -19,6 +19,7 @@ export interface MenuDeps { openSettings: () => void /** Opens the native server picker (see main/server-window.ts). */ openServerSettings: () => void + openFolderAccess: (parent: BrowserWindow) => void newWindow: () => void newChat: () => void /** @@ -194,6 +195,14 @@ export function buildMenuTemplate(deps: MenuDeps): MenuItemConstructorOptions[] { label: 'File', submenu: [ + { + label: 'Folder Access…', + click: (_item, focusedWindow) => { + const win = focusedMainOrFallback(focusedWindow) + if (win) deps.openFolderAccess(win) + }, + }, + { type: 'separator' }, { label: 'New Window', accelerator: 'CmdOrCtrl+Shift+N', From b5ae75ae15fcc6c88bd5c2d37f4cb0820ef1a56a Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 26 Sep 2026 15:31:11 -0700 Subject: [PATCH 3/5] fix(desktop): cancel pending file consent and recheck access --- apps/desktop/README.md | 2 +- apps/desktop/e2e/local-files.spec.ts | 92 ++++++++++++++++ apps/desktop/src/main/ipc.ts | 81 +++++++++----- .../src/main/local-file-permissions.ts | 5 +- apps/desktop/src/main/local-files.ts | 101 ++++++++++++++---- .../desktop/src/main/local-filesystem.test.ts | 36 ++++++- apps/desktop/src/main/local-filesystem.ts | 6 ++ .../mothership/tools/client/native-files.ts | 18 +++- packages/desktop-bridge/src/local-files.ts | 2 +- 9 files changed, 284 insertions(+), 59 deletions(-) diff --git a/apps/desktop/README.md b/apps/desktop/README.md index 10a17fad040..6b432de881c 100644 --- a/apps/desktop/README.md +++ b/apps/desktop/README.md @@ -188,7 +188,7 @@ Copilot can inspect user-selected local directories through the ordinary VFS too - **Bound to a live Copilot call:** before a native read/search or browser action, Electron asks the authenticated Sim origin for the pending tool-call record. Local requests must exactly match its persisted operation, path, and options; browser actions run with the persisted arguments rather than renderer-supplied ones. Completed, failed, and aborted runs are rejected. - **Abort-aware and bounded:** stop/cancel propagates to active native scans and reads. File size, aggregate grep bytes, line, result, traversal-depth, and scan-count limits remain enforced in Electron, and unsafe regular expressions are rejected before execution. -The native `read_local_file` and `import_local_files` tools also accept absolute or `~/` paths. They reuse the same remembered folder grants as the VFS tools. For an unapproved path, Electron displays a bundled, isolated dialog showing the canonical folder and connected server. **Allow folder** grants read and import access to that folder and its subfolders across chats and normal app restarts. A file request proposes its containing folder explicitly; no wider folder is approved silently. Closing or declining the dialog returns no contents. Users can add or forget folders through **File → Folder Access**. As with VFS grants, sign-out and server changes clear access, and unavailable secure storage limits persistence to the app session. New consent prompts are serialized, but reads of approved folders proceed independently. +The native `read_local_file` and `import_local_files` tools also accept absolute or `~/` paths. They reuse the same remembered folder grants as the VFS tools. For an unapproved path, Electron displays a bundled, isolated dialog showing the canonical folder and connected server. **Allow folder** grants read and import access to that folder and its subfolders across chats and normal app restarts. A file request proposes its containing folder explicitly; no wider folder is approved silently. Closing or declining the dialog returns no contents. Users can add or forget folders through **File → Folder Access**. As with VFS grants, sign-out and server changes clear access, and unavailable secure storage limits persistence to the app session. New consent prompts are serialized, but reads of approved folders proceed independently. Existing encrypted path-based approvals retain their scope and acquire folder-identity metadata on their first restore. Approved native reads can return bounded text, directory listings, images, or PDFs to the chat. Approved imports transfer file bytes to Workspace Files. Electron revalidates every pending call before using a grant, including remembered grants, checks canonical containment and grant identity throughout the operation, and opens files with no-follow and descriptor identity checks. A model or hosted renderer cannot answer the local consent dialog. These permissions govern the native file tools; the separately enabled terminal still runs with the user's OS privileges. diff --git a/apps/desktop/e2e/local-files.spec.ts b/apps/desktop/e2e/local-files.spec.ts index c7c729dc42b..45e25cbfd19 100644 --- a/apps/desktop/e2e/local-files.spec.ts +++ b/apps/desktop/e2e/local-files.spec.ts @@ -222,6 +222,98 @@ test('native file tools remember folder consent across chats and restarts until expect(await stale.result).toMatchObject({ ok: false }) } }) + await test.step('a symlink replacement cannot redirect an inspected file', async () => { + const file = realpathSync(join(source, 'report.txt')) + const backup = join(source, 'original-report.txt') + const other = join(source, 'other.txt') + writeFileSync(other, 'different file contents') + await app?.evaluate( + (_electron, paths) => { + const fs = process.getBuiltinModule( + 'node:fs/promises' + ) as typeof import('node:fs/promises') + const original = fs.stat + fs.stat = (async (...args: Parameters) => { + const result = await original(...args) + if (args[0] === paths.file) { + fs.stat = original + await fs.rename(paths.file, paths.backup) + await fs.symlink(paths.other, paths.file) + } + return result + }) as typeof fs.stat + }, + { file, backup, other } + ) + try { + expect(await invoke({ operation: 'read', toolCallId: 'text' })).toMatchObject({ ok: false }) + } finally { + rmSync(file) + renameSync(backup, file) + rmSync(other) + } + }) + await test.step('a directory swapped out and back cannot leak outside entries', async () => { + await app?.evaluate( + (_electron, paths) => { + const fs = process.getBuiltinModule( + 'node:fs/promises' + ) as typeof import('node:fs/promises') + const originalOpen = fs.opendir + const originalRead = fs.readdir + const swapped = async (operation: () => Promise): Promise => { + fs.opendir = originalOpen + fs.readdir = originalRead + await fs.rename(paths.source, paths.backup) + await fs.symlink(paths.outside, paths.source) + try { + return await operation() + } finally { + await fs.rm(paths.source) + await fs.rename(paths.backup, paths.source) + } + } + fs.opendir = (path, options) => + path === paths.source + ? swapped(() => originalOpen(path, options)) + : originalOpen(path, options) + fs.readdir = ((...args: Parameters) => + args[0] === paths.source + ? swapped(() => originalRead(...args)) + : originalRead(...args)) as typeof fs.readdir + }, + { source: realpathSync(source), backup: join(root, 'reports-backup'), outside } + ) + expect(await invoke({ operation: 'read', toolCallId: 'directory' })).toMatchObject({ + ok: false, + }) + }) + await test.step('cancelling in the renderer closes consent without remembering access', async () => { + calls.cancelled = { + toolName: 'read_local_file', + args: { path: join(outside, 'private.txt') }, + } + const cancelled = await requestPermission({ operation: 'read', toolCallId: 'cancelled' }) + const closed = cancelled.prompt.waitForEvent('close', { timeout: 5000 }) + await window.evaluate(async () => { + const api = (globalThis as typeof globalThis & { simDesktop: SimDesktopApi }).simDesktop + await api.localFiles?.({ operation: 'cancel', toolCallId: 'cancelled' }) + }) + await closed + expect(await cancelled.result).toMatchObject({ ok: false }) + const again = await requestPermission({ operation: 'read', toolCallId: 'cancelled' }) + await again.prompt.getByRole('button', { name: "Don't allow", exact: true }).click() + expect(await again.result).toMatchObject({ ok: false }) + }) + await test.step('consent escapes direction controls in folder names', async () => { + const folder = join(root, 'Bidi\u061c\u200e\u200f') + mkdirSync(folder) + calls.bidi = { toolName: 'read_local_file', args: { path: folder } } + const bidi = await requestPermission({ operation: 'read', toolCallId: 'bidi' }) + await expect(bidi.prompt.getByRole('dialog')).toContainText('Bidi\\u061c\\u200e\\u200f') + await bidi.prompt.getByRole('button', { name: "Don't allow", exact: true }).click() + expect(await bidi.result).toMatchObject({ ok: false }) + }) await test.step('an unanswered prompt does not block approved folders', async () => { calls.blocker = { toolName: 'read_local_file', args: { path: join(outside, 'private.txt') } } const blocker = await requestPermission({ operation: 'read', toolCallId: 'blocker' }) diff --git a/apps/desktop/src/main/ipc.ts b/apps/desktop/src/main/ipc.ts index ade293228c6..d7a247fa506 100644 --- a/apps/desktop/src/main/ipc.ts +++ b/apps/desktop/src/main/ipc.ts @@ -598,6 +598,8 @@ async function authorizeLocalFilesystemTool( */ export function registerIpcHandlers(deps: IpcDeps): void { const localFilePermissions = new LocalFilePermissions(deps.localFilesystem) + const activeLocalFiles = new Map>() + let activeLocalFileCount = 0 const browserScopeBySender = new WeakMap() const terminalScopeBySender = new WeakMap() const browserPendingScopesBySender = new WeakMap>() @@ -2087,42 +2089,59 @@ export function registerIpcHandlers(deps: IpcDeps): void { const generation = captureAccountDataGeneration() const origin = deps.appOrigin() const request = args[0] - if (!isRecordLike(request)) return { ok: false, error: 'Invalid local file request.' } - let failureStatus: number | undefined - const authorization = await fetchDesktopToolAuthorization( - event, - deps, - request.toolCallId, - request.operation === 'manifest', - (status) => { - failureStatus = status - } - ) - if (failureStatus === 409) + if (!isRecordLike(request) || !isDesktopToolCallId(request.toolCallId)) + return { ok: false, error: 'Invalid local file request.' } + const key = JSON.stringify([event.sender.id, request.toolCallId]) + if (request.operation === 'cancel') { + for (const pending of activeLocalFiles.get(key) ?? []) pending.abort() + return { ok: false, error: 'Local file operation cancelled.' } + } + if (activeLocalFileCount >= 128) return { ok: false, - code: 'ALREADY_STARTED', - error: 'This import is already running or was already started.', + error: 'Too many local file operations are running. Try again later.', } - if ( - !authorization || - !['read_local_file', 'import_local_files'].includes(authorization.toolName) - ) - return { ok: false, error: 'This is not an authorized pending local file tool call.' } - if ( - authorization.toolName === 'read_local_file' - ? request.operation !== 'read' - : request.operation !== 'manifest' && request.operation !== 'chunk' - ) - return { ok: false, error: 'The operation does not match the pending tool call.' } - const parent = deps.getWindowForContents(event.sender) - if (!parent) - return { ok: false, error: 'A desktop window is required to approve file access.' } + const controller = new AbortController() + const controllers = activeLocalFiles.get(key) ?? new Set() + controllers.add(controller) + activeLocalFiles.set(key, controllers) + activeLocalFileCount++ try { + let failureStatus: number | undefined + const authorization = await fetchDesktopToolAuthorization( + event, + deps, + request.toolCallId, + request.operation === 'manifest', + (status) => { + failureStatus = status + } + ) + if (failureStatus === 409) + return { + ok: false, + code: 'ALREADY_STARTED', + error: 'This import is already running or was already started.', + } + if ( + !authorization || + !['read_local_file', 'import_local_files'].includes(authorization.toolName) + ) + return { ok: false, error: 'This is not an authorized pending local file tool call.' } + if ( + authorization.toolName === 'read_local_file' + ? request.operation !== 'read' + : request.operation !== 'manifest' && request.operation !== 'chunk' + ) + return { ok: false, error: 'The operation does not match the pending tool call.' } + const parent = deps.getWindowForContents(event.sender) + if (!parent) + return { ok: false, error: 'A desktop window is required to approve file access.' } const access = await localFilePermissions.authorize(authorization, { parent, origin, generation, + signal: controller.signal, isCurrent: () => isAccountDataGenerationCurrent(generation) && deps.accountDataAvailable() && @@ -2134,9 +2153,13 @@ export function registerIpcHandlers(deps: IpcDeps): void { await fetchDesktopToolAuthorization(event, deps, request.toolCallId) ), }) - handlerArgs = [request, authorization, access] + return await spec.handler(event.sender, request, authorization, access) } catch (error) { return { ok: false, error: getErrorMessage(error) } + } finally { + controllers.delete(controller) + if (controllers.size === 0) activeLocalFiles.delete(key) + activeLocalFileCount-- } } if (spec.passSender) { diff --git a/apps/desktop/src/main/local-file-permissions.ts b/apps/desktop/src/main/local-file-permissions.ts index 1893cc98a11..bfc7133f0f2 100644 --- a/apps/desktop/src/main/local-file-permissions.ts +++ b/apps/desktop/src/main/local-file-permissions.ts @@ -13,6 +13,7 @@ interface LocalFilePermissionContext { parent: BrowserWindow origin: string generation: number + signal: AbortSignal isCurrent: () => boolean revalidate: () => Promise } @@ -27,6 +28,7 @@ function nativePath(value: unknown): string { } function assertCurrent(context: LocalFilePermissionContext): void { + context.signal.throwIfAborted() if (context.parent.isDestroyed() || !context.isCurrent()) throw new Error('This local file request expired. Ask again in the current chat.') } @@ -89,11 +91,12 @@ export class LocalFilePermissions { const root = await lstat(folder) if (!root.isDirectory()) throw new Error('The folder is no longer available.') const displayedPath = JSON.stringify(folder).replace( - /[\u202a-\u202e\u2066-\u2069]/g, + /\p{Bidi_Control}/gu, (character) => `\\u${character.charCodeAt(0).toString(16).padStart(4, '0')}` ) assertCurrent(context) const result = await showShellDialog(context.parent, { + signal: context.signal, title: 'Allow access to this folder?', message: displayedPath, detail: `Sim can read files in this folder and its subfolders, use them across chats, and import them into your workspaces on ${context.origin}.\n\nManage or remove access in File → Folder Access.`, diff --git a/apps/desktop/src/main/local-files.ts b/apps/desktop/src/main/local-files.ts index e76c86448b8..d9179636ed4 100644 --- a/apps/desktop/src/main/local-files.ts +++ b/apps/desktop/src/main/local-files.ts @@ -1,5 +1,5 @@ -import { constants } from 'node:fs' -import { lstat, open, readdir, stat } from 'node:fs/promises' +import { constants, type Dirent } from 'node:fs' +import { lstat, open, opendir, stat } from 'node:fs/promises' import { basename, isAbsolute, join, relative, resolve, sep } from 'node:path' import type { DesktopLocalFileEntry, @@ -10,6 +10,7 @@ import type { import { MAX_DESKTOP_IMPORT_FILE_BYTES } from '@sim/desktop-bridge' import { getErrorMessage } from '@sim/utils/errors' import { isRecordLike } from '@sim/utils/object' +import { compareStrings } from '@sim/utils/string' import { PDFDocument } from 'pdf-lib' import type { LocalFileAccess } from '@/main/local-filesystem' @@ -39,18 +40,23 @@ function assertImportPath(root: string, candidate: string): void { throw new Error('The file is outside this import source.') } -async function openApprovedFile(path: string, access: LocalFileAccess) { +async function openApprovedPath(path: string, access: LocalFileAccess, directory = false) { const canonical = await access.resolve(path) + if (canonical !== path) + throw new Error('The local path changed while it was being opened. Try again.') const file = await open( canonical, - constants.O_RDONLY | constants.O_NOFOLLOW | constants.O_NONBLOCK + constants.O_RDONLY | + constants.O_NOFOLLOW | + constants.O_NONBLOCK | + (directory ? constants.O_DIRECTORY : 0) ) try { const info = await file.stat() const verified = await access.resolve(path) const current = await lstat(verified) if ( - !info.isFile() || + (directory ? !info.isDirectory() : !info.isFile()) || canonical !== verified || info.dev !== current.dev || info.ino !== current.ino @@ -63,6 +69,53 @@ async function openApprovedFile(path: string, access: LocalFileAccess) { } } +async function readApprovedDirectory(path: string, access: LocalFileAccess) { + const handle = await openApprovedPath(path, access, true) + try { + const before = await handle.stat({ bigint: true }) + const verify = async () => { + const canonical = await access.resolve(path) + const current = await lstat(canonical, { bigint: true }) + const after = await handle.stat({ bigint: true }) + if ( + canonical !== path || + !current.isDirectory() || + current.dev !== before.dev || + current.ino !== before.ino || + current.ctimeNs !== before.ctimeNs || + current.mtimeNs !== before.mtimeNs || + after.ctimeNs !== before.ctimeNs || + after.mtimeNs !== before.mtimeNs + ) { + throw new Error('The local directory changed while it was being read. Try again.') + } + } + const directory = await opendir(path) + try { + await verify() + const entries: Dirent[] = [] + let count = 0 + const sort = () => entries.sort((left, right) => compareStrings(left.name, right.name)) + for (let entry = await directory.read(); entry; entry = await directory.read()) { + entries.push(entry) + count++ + if (entries.length === MAX_ENTRIES * 2) { + sort() + entries.length = MAX_ENTRIES + await access.resolve(path) + } + } + await verify() + sort() + return { entries: entries.slice(0, MAX_ENTRIES), truncated: count > MAX_ENTRIES } + } finally { + await directory.close() + } + } finally { + await handle.close() + } +} + async function inspect( path: string, args: Record, @@ -70,29 +123,26 @@ async function inspect( ): Promise { const info = await stat(path) if (info.isDirectory()) { - const entries = await readdir(path, { withFileTypes: true }) + const { entries, truncated } = await readApprovedDirectory(path, access) return { kind: 'read', path, representation: 'directory', - truncated: entries.length > MAX_ENTRIES, - entries: entries - .sort((a, b) => (a.name < b.name ? -1 : a.name > b.name ? 1 : 0)) - .slice(0, MAX_ENTRIES) - .map((entry) => ({ - name: entry.name, - kind: entry.isFile() - ? 'file' - : entry.isDirectory() - ? 'directory' - : entry.isSymbolicLink() - ? 'symlink' - : 'other', - })), + truncated, + entries: entries.map((entry) => ({ + name: entry.name, + kind: entry.isFile() + ? 'file' + : entry.isDirectory() + ? 'directory' + : entry.isSymbolicLink() + ? 'symlink' + : 'other', + })), } } if (!info.isFile()) throw new Error('The path is not a regular file or directory.') - const file = await openApprovedFile(path, access) + const file = await openApprovedPath(path, access) try { const info = await file.stat() const header = Buffer.alloc(16) @@ -218,7 +268,12 @@ async function manifest( }) if (info.isDirectory()) { const next = new Set([...ancestors, canonical]) - for (const name of (await readdir(current)).sort()) await walk(join(current, name), next) + const children = await readApprovedDirectory(canonical, access) + if (children.truncated) + throw new Error( + 'The directory exceeds 1,000 entries. Import smaller subdirectories separately.' + ) + for (const entry of children.entries) await walk(join(current, entry.name), next) } } await walk(path, new Set()) @@ -264,7 +319,7 @@ export async function executeLocalFileRequest( const canonical = await access.resolve(child) assertImportPath(root, canonical) const offset = boundedInteger(request.offset, 0, Number.MAX_SAFE_INTEGER) - const file = await openApprovedFile(canonical, access) + const file = await openApprovedPath(canonical, access) try { const info = await file.stat() if (!info.isFile() || revision(info) !== request.revision) diff --git a/apps/desktop/src/main/local-filesystem.test.ts b/apps/desktop/src/main/local-filesystem.test.ts index 29b01d5e86d..1a6302b7607 100644 --- a/apps/desktop/src/main/local-filesystem.test.ts +++ b/apps/desktop/src/main/local-filesystem.test.ts @@ -3,6 +3,7 @@ import { mkdir, mkdtemp, open, + readFile, realpath, rename, rm, @@ -16,7 +17,12 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' vi.mock('electron', () => import('@/test/electron-mock')) vi.mock('node:fs/promises', async (importOriginal) => { const actual = await importOriginal() - return { ...actual, open: vi.fn(actual.open) } + return { + ...actual, + open: vi.fn(actual.open), + realpath: vi.fn(actual.realpath), + readFile: vi.fn(actual.readFile), + } }) import type { LocalFilesystemMount, LocalFilesystemResponse } from '@sim/desktop-bridge' @@ -337,6 +343,34 @@ describe('LocalFilesystemService', () => { } ) + it.each(['resolution', 'read'] as const)( + 'returns no VFS contents when access is revoked during %s', + async (stage) => { + const granted = await mount(service) + const revoke = () => service.handle({ operation: 'forget_mount', uri: granted.uri }) + if (stage === 'resolution') { + const original = vi.mocked(realpath).getMockImplementation() + if (!original) throw new Error('Missing real filesystem implementation') + vi.mocked(realpath).mockImplementationOnce(async (...args) => { + const result = await original(...args) + await revoke() + return result + }) + } else { + const original = vi.mocked(readFile).getMockImplementation() + if (!original) throw new Error('Missing real filesystem implementation') + vi.mocked(readFile).mockImplementationOnce(async (...args) => { + const result = await original(...args) + await revoke() + return result + }) + } + expect( + await service.handle({ operation: 'read', uri: `${granted.uri}README.md` }) + ).toMatchObject({ ok: false, code: 'ACCESS_DENIED' }) + } + ) + it('rejects lexical traversal before URL normalization can reinterpret it', async () => { const granted = await mount(service) const traversal = await service.handle({ diff --git a/apps/desktop/src/main/local-filesystem.ts b/apps/desktop/src/main/local-filesystem.ts index 7f58135dbe6..5e20d4bf8f7 100644 --- a/apps/desktop/src/main/local-filesystem.ts +++ b/apps/desktop/src/main/local-filesystem.ts @@ -429,8 +429,12 @@ export class LocalFilesystemService { this.activeRequests.set(requestId, controller) } + let accessedMount: GrantedMount | undefined let data: LocalFilesystemData try { + if (['list', 'glob', 'read', 'grep', 'stat'].includes(request.operation)) { + accessedMount = this.parseUri(this.requiredUri(request)).mount + } switch (request.operation) { case 'mount_directory': data = await this.mountDirectory() @@ -475,6 +479,7 @@ export class LocalFilesystemService { 'Local filesystem operation is not supported.' ) } + if (accessedMount) await this.assertMountCurrent(accessedMount) } finally { if (requestId) { this.activeRequests.delete(requestId) @@ -1054,6 +1059,7 @@ export class LocalFilesystemService { 'The requested path is outside the selected folder.' ) } + await this.assertMountCurrent(mount) return { mount, relativePath, lexicalPath, realPath } } diff --git a/apps/sim/lib/mothership/tools/client/native-files.ts b/apps/sim/lib/mothership/tools/client/native-files.ts index 73ed8e8227e..d945492a429 100644 --- a/apps/sim/lib/mothership/tools/client/native-files.ts +++ b/apps/sim/lib/mothership/tools/client/native-files.ts @@ -29,9 +29,21 @@ async function invoke( signal?.throwIfAborted() const bridge = getDesktopBridge() if (!bridge?.localFiles) throw new Error('Update the Sim desktop app to use native file tools.') - const response = await bridge.localFiles(request) - signal?.throwIfAborted() - return response + const onAbort = () => { + void bridge + .localFiles?.({ operation: 'cancel', toolCallId: request.toolCallId }) + .catch((error) => + logger.warn('Could not cancel native file access', { error: getErrorMessage(error) }) + ) + } + signal?.addEventListener('abort', onAbort, { once: true }) + try { + const response = await bridge.localFiles(request) + signal?.throwIfAborted() + return response + } finally { + signal?.removeEventListener('abort', onAbort) + } } interface ImportedFile { diff --git a/packages/desktop-bridge/src/local-files.ts b/packages/desktop-bridge/src/local-files.ts index 92ff8916c7a..3c1da83e265 100644 --- a/packages/desktop-bridge/src/local-files.ts +++ b/packages/desktop-bridge/src/local-files.ts @@ -2,7 +2,7 @@ export const MAX_DESKTOP_IMPORT_FILE_BYTES = 64 * 1024 * 1024 /** Native desktop file operations require local consent and canonical pending chat calls. */ export type DesktopLocalFileRequest = - | { operation: 'read' | 'manifest'; toolCallId: string } + | { operation: 'read' | 'manifest' | 'cancel'; toolCallId: string } | { operation: 'chunk' toolCallId: string From 94a8dad62d9497db8426aae5c49c904059d87356 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 26 Sep 2026 15:51:39 -0700 Subject: [PATCH 4/5] fix(desktop): enumerate approved directory descriptors --- apps/desktop/README.md | 4 +- apps/desktop/e2e/local-files.spec.ts | 123 +++++++++++++---- apps/desktop/native/directory.cc | 154 ++++++++++++++++++++++ apps/desktop/package.json | 4 +- apps/desktop/scripts/build-native.ts | 62 +++++++++ apps/desktop/scripts/build.ts | 52 +------- apps/desktop/src/main/local-files.test.ts | 8 +- apps/desktop/src/main/local-files.ts | 60 ++------- apps/desktop/src/main/native-directory.ts | 25 ++++ 9 files changed, 362 insertions(+), 130 deletions(-) create mode 100644 apps/desktop/native/directory.cc create mode 100644 apps/desktop/scripts/build-native.ts create mode 100644 apps/desktop/src/main/native-directory.ts diff --git a/apps/desktop/README.md b/apps/desktop/README.md index 6b432de881c..287a3ec45ec 100644 --- a/apps/desktop/README.md +++ b/apps/desktop/README.md @@ -38,7 +38,7 @@ src/main/ # main process (bundled to dist/main.cjs) src/preload/ # isolated renderer bridges index.ts # hosted-app contextBridge IPC bridge (dist/preload.cjs) browser/ # minimal agent-browser credential helper (dist/browser-preload.cjs) -native/ # Node-API/AppKit bridge for native macOS Help docs search +native/ # Node-API bridges for directory enumeration and macOS Help docs search static/ # bundled local pages (offline.html, server.html), served over sim-shell: e2e/ # Playwright _electron smoke suite ``` @@ -190,7 +190,7 @@ Copilot can inspect user-selected local directories through the ordinary VFS too The native `read_local_file` and `import_local_files` tools also accept absolute or `~/` paths. They reuse the same remembered folder grants as the VFS tools. For an unapproved path, Electron displays a bundled, isolated dialog showing the canonical folder and connected server. **Allow folder** grants read and import access to that folder and its subfolders across chats and normal app restarts. A file request proposes its containing folder explicitly; no wider folder is approved silently. Closing or declining the dialog returns no contents. Users can add or forget folders through **File → Folder Access**. As with VFS grants, sign-out and server changes clear access, and unavailable secure storage limits persistence to the app session. New consent prompts are serialized, but reads of approved folders proceed independently. Existing encrypted path-based approvals retain their scope and acquire folder-identity metadata on their first restore. -Approved native reads can return bounded text, directory listings, images, or PDFs to the chat. Approved imports transfer file bytes to Workspace Files. Electron revalidates every pending call before using a grant, including remembered grants, checks canonical containment and grant identity throughout the operation, and opens files with no-follow and descriptor identity checks. A model or hosted renderer cannot answer the local consent dialog. These permissions govern the native file tools; the separately enabled terminal still runs with the user's OS privileges. +Approved native reads can return bounded text, directory listings, images, or PDFs to the chat. Approved imports transfer file bytes to Workspace Files. Electron revalidates every pending call before using a grant, including remembered grants, checks canonical containment and grant identity throughout the operation, and opens files with no-follow and descriptor identity checks. Directory enumeration uses `fdopendir` on the verified descriptor, so replacing a parent path cannot redirect the listing. A model or hosted renderer cannot answer the local consent dialog. These permissions govern the native file tools; the separately enabled terminal still runs with the user's OS privileges. ## Auto-update, channels, rollout, rollback diff --git a/apps/desktop/e2e/local-files.spec.ts b/apps/desktop/e2e/local-files.spec.ts index 45e25cbfd19..ae4aa4c1dd2 100644 --- a/apps/desktop/e2e/local-files.spec.ts +++ b/apps/desktop/e2e/local-files.spec.ts @@ -253,40 +253,113 @@ test('native file tools remember folder consent across chats and restarts until rmSync(other) } }) - await test.step('a directory swapped out and back cannot leak outside entries', async () => { + await test.step('swapping an ancestor cannot redirect directory enumeration', async () => { + const parent = join(realpathSync(source), 'parent') + const child = join(parent, 'child') + const backup = join(realpathSync(source), 'parent-backup') + const otherParent = join(outside, 'parent') + mkdirSync(child, { recursive: true }) + mkdirSync(join(otherParent, 'child'), { recursive: true }) + writeFileSync(join(child, 'allowed.txt'), 'allowed') + writeFileSync(join(otherParent, 'child', 'private.txt'), 'outside') + calls.ancestorRace = { toolName: 'read_local_file', args: { path: child } } await app?.evaluate( (_electron, paths) => { const fs = process.getBuiltinModule( 'node:fs/promises' ) as typeof import('node:fs/promises') - const originalOpen = fs.opendir - const originalRead = fs.readdir - const swapped = async (operation: () => Promise): Promise => { - fs.opendir = originalOpen - fs.readdir = originalRead - await fs.rename(paths.source, paths.backup) - await fs.symlink(paths.outside, paths.source) - try { - return await operation() - } finally { - await fs.rm(paths.source) - await fs.rename(paths.backup, paths.source) + const originalStat = fs.lstat + const originalRealpath = fs.realpath + let swapped = false + const restore = async () => { + fs.lstat = originalStat + fs.realpath = originalRealpath + if (swapped) { + await fs.rm(paths.parent) + await fs.rename(paths.backup, paths.parent) + swapped = false } } - fs.opendir = (path, options) => - path === paths.source - ? swapped(() => originalOpen(path, options)) - : originalOpen(path, options) - fs.readdir = ((...args: Parameters) => - args[0] === paths.source - ? swapped(() => originalRead(...args)) - : originalRead(...args)) as typeof fs.readdir + ;( + globalThis as typeof globalThis & { restoreLocalFileRace?: () => Promise } + ).restoreLocalFileRace = restore + fs.lstat = (async (...args: Parameters) => { + const result = await originalStat(...args) + if (args[0] === paths.child) { + fs.lstat = originalStat + await fs.rename(paths.parent, paths.backup) + await fs.symlink(paths.otherParent, paths.parent) + swapped = true + } + return result + }) as typeof fs.lstat + fs.realpath = (async (...args: Parameters) => { + if (swapped && args[0] === paths.child) await restore() + return originalRealpath(...args) + }) as typeof fs.realpath }, - { source: realpathSync(source), backup: join(root, 'reports-backup'), outside } + { parent, child, backup, otherParent } ) - expect(await invoke({ operation: 'read', toolCallId: 'directory' })).toMatchObject({ - ok: false, - }) + try { + expect(await invoke({ operation: 'read', toolCallId: 'ancestorRace' })).toMatchObject({ + ok: true, + data: { entries: [{ name: 'allowed.txt', kind: 'file' }] }, + }) + } finally { + await app?.evaluate(async () => { + const runtime = globalThis as typeof globalThis & { + restoreLocalFileRace?: () => Promise + } + await runtime.restoreLocalFileRace?.() + runtime.restoreLocalFileRace = undefined + }) + rmSync(parent, { recursive: true, force: true }) + rmSync(otherParent, { recursive: true, force: true }) + } + }) + await test.step('cancelling after a file opens prevents its contents from returning', async () => { + await app?.evaluate( + (_electron, path) => { + const fs = process.getBuiltinModule( + 'node:fs/promises' + ) as typeof import('node:fs/promises') + const original = fs.open + fs.open = async (...args: Parameters) => { + const handle = await original(...args) + if (args[0] === path) { + fs.open = original + await new Promise((resolve) => { + ;( + globalThis as typeof globalThis & { releaseLocalFileRead?: () => void } + ).releaseLocalFileRead = resolve + }) + } + return handle + } + }, + realpathSync(join(source, 'report.txt')) + ) + const reading = invoke({ operation: 'read', toolCallId: 'text' }) + void reading.catch(() => {}) + try { + await expect + .poll(() => + app?.evaluate( + () => + typeof (globalThis as typeof globalThis & { releaseLocalFileRead?: () => void }) + .releaseLocalFileRead === 'function' + ) + ) + .toBe(true) + await invoke({ operation: 'cancel', toolCallId: 'text' }) + } finally { + await app?.evaluate(() => { + const runtime = globalThis as typeof globalThis & { releaseLocalFileRead?: () => void } + runtime.releaseLocalFileRead?.() + runtime.releaseLocalFileRead = undefined + }) + } + expect(await reading).toMatchObject({ ok: false }) }) await test.step('cancelling in the renderer closes consent without remembering access', async () => { calls.cancelled = { diff --git a/apps/desktop/native/directory.cc b/apps/desktop/native/directory.cc new file mode 100644 index 00000000000..0e40e20c07e --- /dev/null +++ b/apps/desktop/native/directory.cc @@ -0,0 +1,154 @@ +#include + +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include + +struct Entry { + std::string name; + const char* kind; +}; + +struct DirectoryRead { + napi_async_work work = nullptr; + napi_deferred deferred = nullptr; + int descriptor = -1; + size_t limit = 0; + bool truncated = false; + std::string error; + std::vector entries; +}; + +static const char* EntryKind(DIR* directory, const dirent* entry) { + switch (entry->d_type) { + case DT_REG: return "file"; + case DT_DIR: return "directory"; + case DT_LNK: return "symlink"; + case DT_UNKNOWN: { + struct stat metadata; + if (fstatat(dirfd(directory), entry->d_name, &metadata, AT_SYMLINK_NOFOLLOW) == 0) { + if (S_ISREG(metadata.st_mode)) return "file"; + if (S_ISDIR(metadata.st_mode)) return "directory"; + if (S_ISLNK(metadata.st_mode)) return "symlink"; + } + return "other"; + } + default: return "other"; + } +} + +static void ReadEntries(napi_env, void* data) { + auto* read = static_cast(data); + DIR* directory = fdopendir(read->descriptor); + if (!directory) { + close(read->descriptor); + read->descriptor = -1; + read->error = "Could not enumerate the approved directory."; + return; + } + read->descriptor = -1; + while (true) { + errno = 0; + const dirent* entry = readdir(directory); + if (!entry) { + if (errno != 0) read->error = "Could not finish reading the approved directory."; + break; + } + if (strcmp(entry->d_name, ".") == 0 || strcmp(entry->d_name, "..") == 0) continue; + if (read->entries.size() == read->limit) { + read->truncated = true; + break; + } + read->entries.push_back({entry->d_name, EntryKind(directory, entry)}); + } + closedir(directory); +} + +static napi_value String(napi_env env, const char* value) { + napi_value result; + napi_create_string_utf8(env, value, NAPI_AUTO_LENGTH, &result); + return result; +} + +static void Complete(napi_env env, napi_status status, void* data) { + auto* read = static_cast(data); + if (read->descriptor >= 0) close(read->descriptor); + if (status != napi_ok || !read->error.empty()) { + napi_value error; + napi_create_error(env, nullptr, + String(env, read->error.empty() ? "Directory read cancelled." : read->error.c_str()), + &error); + napi_reject_deferred(env, read->deferred, error); + } else { + napi_value result; + napi_value entries; + napi_value truncated; + napi_create_object(env, &result); + napi_create_array_with_length(env, read->entries.size(), &entries); + for (size_t index = 0; index < read->entries.size(); index++) { + napi_value entry; + napi_create_object(env, &entry); + napi_set_named_property(env, entry, "name", String(env, read->entries[index].name.c_str())); + napi_set_named_property(env, entry, "kind", String(env, read->entries[index].kind)); + napi_set_element(env, entries, index, entry); + } + napi_get_boolean(env, read->truncated, &truncated); + napi_set_named_property(env, result, "entries", entries); + napi_set_named_property(env, result, "truncated", truncated); + napi_resolve_deferred(env, read->deferred, result); + } + napi_delete_async_work(env, read->work); + delete read; +} + +/** The descriptor is duplicated before scheduling; no pathname is reopened by the worker. */ +static napi_value ReadDirectory(napi_env env, napi_callback_info info) { + size_t count = 2; + napi_value arguments[2]; + double descriptor = -1; + double limit = 0; + if (napi_get_cb_info(env, info, &count, arguments, nullptr, nullptr) != napi_ok || count != 2 || + napi_get_value_double(env, arguments[0], &descriptor) != napi_ok || + napi_get_value_double(env, arguments[1], &limit) != napi_ok || + !std::isfinite(descriptor) || descriptor < 0 || descriptor > INT_MAX || + descriptor != std::floor(descriptor) || limit < 1 || limit > 1000 || limit != std::floor(limit)) { + napi_throw_type_error(env, nullptr, "Expected a directory descriptor and an entry limit from 1 to 1000."); + return nullptr; + } + auto* read = new DirectoryRead(); + read->limit = static_cast(limit); + read->descriptor = fcntl(static_cast(descriptor), F_DUPFD_CLOEXEC, 0); + if (read->descriptor < 0) { + delete read; + napi_throw_error(env, nullptr, "Could not retain the approved directory descriptor."); + return nullptr; + } + napi_value promise; + if (napi_create_promise(env, &read->deferred, &promise) != napi_ok || + napi_create_async_work(env, nullptr, String(env, "ReadApprovedDirectory"), ReadEntries, + Complete, read, &read->work) != napi_ok || + napi_queue_async_work(env, read->work) != napi_ok) { + close(read->descriptor); + if (read->work) napi_delete_async_work(env, read->work); + delete read; + napi_throw_error(env, nullptr, "Could not schedule the directory read."); + return nullptr; + } + return promise; +} + +NAPI_MODULE_INIT() { + napi_property_descriptor property = { + "readDirectory", nullptr, ReadDirectory, nullptr, nullptr, nullptr, napi_default, nullptr + }; + napi_define_properties(env, exports, 1, &property); + return exports; +} diff --git a/apps/desktop/package.json b/apps/desktop/package.json index a4484720362..e980fa8202e 100644 --- a/apps/desktop/package.json +++ b/apps/desktop/package.json @@ -25,8 +25,8 @@ "lint:check": "biome check .", "format": "biome format --write .", "format:check": "biome format .", - "test": "vitest run", - "test:watch": "vitest", + "test": "bun run scripts/build-native.ts && vitest run", + "test:watch": "bun run scripts/build-native.ts && vitest", "test:e2e": "playwright test" }, "dependencies": { diff --git a/apps/desktop/scripts/build-native.ts b/apps/desktop/scripts/build-native.ts new file mode 100644 index 00000000000..8980683f31f --- /dev/null +++ b/apps/desktop/scripts/build-native.ts @@ -0,0 +1,62 @@ +import { execFileSync } from 'node:child_process' +import { existsSync, mkdirSync, statSync } from 'node:fs' +import { dirname, join } from 'node:path' +import { createLogger } from '@sim/logger' + +const logger = createLogger('DesktopNativeBuild') +if (process.platform !== 'darwin' && process.platform !== 'linux') { + throw new Error('Native desktop modules require macOS or Linux.') +} +const nodeExecutable = execFileSync('node', ['-p', 'process.execPath'], { encoding: 'utf8' }).trim() +const includeDirectory = join(dirname(nodeExecutable), '..', 'include', 'node') +if (!existsSync(join(includeDirectory, 'node_api.h'))) { + throw new Error(`Could not find Node-API headers in ${includeDirectory}`) +} +const outputDirectory = 'dist/native' +mkdirSync(outputDirectory, { recursive: true }) +const modules = [ + { name: 'directory', source: 'native/directory.cc', appKit: false }, + ...(process.platform === 'darwin' + ? [{ name: 'help-search', source: 'native/help-search.mm', appKit: true }] + : []), +] +for (const module of modules) { + const output = join(outputDirectory, `${module.name}.node`) + if ( + existsSync(output) && + statSync(output).mtimeMs >= + Math.max(statSync(module.source).mtimeMs, statSync(import.meta.filename).mtimeMs) + ) + continue + const macOS = process.platform === 'darwin' + execFileSync( + macOS ? 'xcrun' : 'c++', + [ + ...(macOS ? ['clang++'] : []), + '-std=c++17', + '-DNAPI_VERSION=8', + ...(macOS + ? [ + '-bundle', + '-undefined', + 'dynamic_lookup', + '-mmacosx-version-min=12.0', + '-arch', + 'arm64', + '-arch', + 'x86_64', + ] + : ['-shared', '-fPIC']), + '-I', + includeDirectory, + ...(module.appKit + ? ['-fobjc-arc', '-fblocks', '-framework', 'AppKit', '-framework', 'Foundation'] + : []), + '-o', + output, + module.source, + ], + { stdio: 'inherit' } + ) + logger.info('Compiled native desktop module', { module: module.name }) +} diff --git a/apps/desktop/scripts/build.ts b/apps/desktop/scripts/build.ts index b15dc2fb354..59527b6c616 100644 --- a/apps/desktop/scripts/build.ts +++ b/apps/desktop/scripts/build.ts @@ -1,6 +1,6 @@ import { execFileSync } from 'node:child_process' -import { cpSync, existsSync, mkdirSync, readFileSync, rmSync } from 'node:fs' -import { dirname, join, resolve } from 'node:path' +import { cpSync, readFileSync, rmSync } from 'node:fs' +import { dirname, resolve } from 'node:path' import { type BuildOptions, build } from 'esbuild' import postcss from 'postcss' import loadPostcssConfig from 'postcss-load-config' @@ -34,52 +34,6 @@ rmSync(generatedIcon, { force: true, recursive: true }) cpSync(appIcon, generatedIcon, { recursive: true }) console.log(`• Selecting desktop icon: ${appIcon}`) -function compileNativeHelpSearch(): void { - const outputDirectory = 'dist/native' - rmSync(outputDirectory, { force: true, recursive: true }) - if (process.platform !== 'darwin') return - - const nodeExecutable = execFileSync('node', ['-p', 'process.execPath'], { - encoding: 'utf8', - }).trim() - const nodeIncludeDirectory = join(dirname(nodeExecutable), '..', 'include', 'node') - const nodeApiHeader = join(nodeIncludeDirectory, 'node_api.h') - if (!existsSync(nodeApiHeader)) { - throw new Error(`Could not find Node-API headers at ${nodeApiHeader}`) - } - - mkdirSync(outputDirectory, { recursive: true }) - execFileSync( - 'xcrun', - [ - 'clang++', - '-std=c++17', - '-DNAPI_VERSION=8', - '-fobjc-arc', - '-fblocks', - '-bundle', - '-undefined', - 'dynamic_lookup', - '-mmacosx-version-min=12.0', - '-arch', - 'arm64', - '-arch', - 'x86_64', - '-I', - nodeIncludeDirectory, - '-framework', - 'AppKit', - '-framework', - 'Foundation', - '-o', - join(outputDirectory, 'help-search.node'), - 'native/help-search.mm', - ], - { stdio: 'inherit' } - ) - console.log('• Compiled native macOS documentation Help search') -} - const common = { bundle: true, platform: 'node' as const, @@ -138,7 +92,7 @@ const renderer: BuildOptions = { } async function run(): Promise { - compileNativeHelpSearch() + execFileSync(process.execPath, ['run', 'scripts/build-native.ts'], { stdio: 'inherit' }) if (watch) { const { context } = await import('esbuild') const rendererCtx = await context(renderer) diff --git a/apps/desktop/src/main/local-files.test.ts b/apps/desktop/src/main/local-files.test.ts index a7dd2c87d07..4ef5f6fdb46 100644 --- a/apps/desktop/src/main/local-files.test.ts +++ b/apps/desktop/src/main/local-files.test.ts @@ -1,7 +1,12 @@ import { mkdir, mkdtemp, realpath, rm, symlink, truncate, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' -import { afterEach, beforeEach, expect, it } from 'vitest' +import { fileURLToPath } from 'node:url' +import { app } from 'electron' +import { afterEach, beforeEach, expect, it, vi } from 'vitest' + +vi.mock('electron', () => import('@/test/electron-mock')) + import { executeLocalFileRequest as executeApprovedLocalFileRequest, type LocalFileAuthorization, @@ -17,6 +22,7 @@ function executeLocalFileRequest(request: unknown, authorization: LocalFileAutho let root: string beforeEach(async () => { + vi.mocked(app.getAppPath).mockReturnValue(fileURLToPath(new URL('../..', import.meta.url))) root = await mkdtemp(join(tmpdir(), 'sim-native-files-')) }) afterEach(async () => { diff --git a/apps/desktop/src/main/local-files.ts b/apps/desktop/src/main/local-files.ts index d9179636ed4..4e5ae157fae 100644 --- a/apps/desktop/src/main/local-files.ts +++ b/apps/desktop/src/main/local-files.ts @@ -1,5 +1,5 @@ -import { constants, type Dirent } from 'node:fs' -import { lstat, open, opendir, stat } from 'node:fs/promises' +import { constants } from 'node:fs' +import { lstat, open, stat } from 'node:fs/promises' import { basename, isAbsolute, join, relative, resolve, sep } from 'node:path' import type { DesktopLocalFileEntry, @@ -13,6 +13,7 @@ import { isRecordLike } from '@sim/utils/object' import { compareStrings } from '@sim/utils/string' import { PDFDocument } from 'pdf-lib' import type { LocalFileAccess } from '@/main/local-filesystem' +import { readNativeDirectory } from '@/main/native-directory' const CHUNK_BYTES = 8 * 1024 * 1024 const MAX_ENTRIES = 1000 @@ -72,45 +73,11 @@ async function openApprovedPath(path: string, access: LocalFileAccess, directory async function readApprovedDirectory(path: string, access: LocalFileAccess) { const handle = await openApprovedPath(path, access, true) try { - const before = await handle.stat({ bigint: true }) - const verify = async () => { - const canonical = await access.resolve(path) - const current = await lstat(canonical, { bigint: true }) - const after = await handle.stat({ bigint: true }) - if ( - canonical !== path || - !current.isDirectory() || - current.dev !== before.dev || - current.ino !== before.ino || - current.ctimeNs !== before.ctimeNs || - current.mtimeNs !== before.mtimeNs || - after.ctimeNs !== before.ctimeNs || - after.mtimeNs !== before.mtimeNs - ) { - throw new Error('The local directory changed while it was being read. Try again.') - } - } - const directory = await opendir(path) - try { - await verify() - const entries: Dirent[] = [] - let count = 0 - const sort = () => entries.sort((left, right) => compareStrings(left.name, right.name)) - for (let entry = await directory.read(); entry; entry = await directory.read()) { - entries.push(entry) - count++ - if (entries.length === MAX_ENTRIES * 2) { - sort() - entries.length = MAX_ENTRIES - await access.resolve(path) - } - } - await verify() - sort() - return { entries: entries.slice(0, MAX_ENTRIES), truncated: count > MAX_ENTRIES } - } finally { - await directory.close() - } + const listing = await readNativeDirectory(handle.fd, MAX_ENTRIES) + if ((await access.resolve(path)) !== path) + throw new Error('The local directory changed while it was being read. Try again.') + listing.entries.sort((left, right) => compareStrings(left.name, right.name)) + return listing } finally { await handle.close() } @@ -129,16 +96,7 @@ async function inspect( path, representation: 'directory', truncated, - entries: entries.map((entry) => ({ - name: entry.name, - kind: entry.isFile() - ? 'file' - : entry.isDirectory() - ? 'directory' - : entry.isSymbolicLink() - ? 'symlink' - : 'other', - })), + entries, } } if (!info.isFile()) throw new Error('The path is not a regular file or directory.') diff --git a/apps/desktop/src/main/native-directory.ts b/apps/desktop/src/main/native-directory.ts new file mode 100644 index 00000000000..4babef7bde9 --- /dev/null +++ b/apps/desktop/src/main/native-directory.ts @@ -0,0 +1,25 @@ +import { join } from 'node:path' +import type { DesktopLocalFileRead } from '@sim/desktop-bridge' +import { app } from 'electron' + +interface NativeDirectoryListing { + entries: NonNullable + truncated: boolean +} + +interface NativeDirectoryBridge { + readDirectory: (descriptor: number, limit: number) => Promise +} + +let bridge: NativeDirectoryBridge | undefined + +/** Enumerates the already-validated descriptor without resolving a pathname again. */ +export function readNativeDirectory( + descriptor: number, + limit: number +): Promise { + bridge ??= require( + join(app.getAppPath(), 'dist', 'native', 'directory.node') + ) as NativeDirectoryBridge + return bridge.readDirectory(descriptor, limit) +} From ce6f05bd54e1e0f9dfee5f9248aadf508ac92832 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 26 Sep 2026 15:59:00 -0700 Subject: [PATCH 5/5] fix(desktop): reject replaced directory listings --- apps/desktop/README.md | 2 +- apps/desktop/e2e/local-files.spec.ts | 34 ++++++++++++++++++++++++++++ apps/desktop/src/main/local-files.ts | 5 +++- 3 files changed, 39 insertions(+), 2 deletions(-) diff --git a/apps/desktop/README.md b/apps/desktop/README.md index 287a3ec45ec..5e91e68870d 100644 --- a/apps/desktop/README.md +++ b/apps/desktop/README.md @@ -190,7 +190,7 @@ Copilot can inspect user-selected local directories through the ordinary VFS too The native `read_local_file` and `import_local_files` tools also accept absolute or `~/` paths. They reuse the same remembered folder grants as the VFS tools. For an unapproved path, Electron displays a bundled, isolated dialog showing the canonical folder and connected server. **Allow folder** grants read and import access to that folder and its subfolders across chats and normal app restarts. A file request proposes its containing folder explicitly; no wider folder is approved silently. Closing or declining the dialog returns no contents. Users can add or forget folders through **File → Folder Access**. As with VFS grants, sign-out and server changes clear access, and unavailable secure storage limits persistence to the app session. New consent prompts are serialized, but reads of approved folders proceed independently. Existing encrypted path-based approvals retain their scope and acquire folder-identity metadata on their first restore. -Approved native reads can return bounded text, directory listings, images, or PDFs to the chat. Approved imports transfer file bytes to Workspace Files. Electron revalidates every pending call before using a grant, including remembered grants, checks canonical containment and grant identity throughout the operation, and opens files with no-follow and descriptor identity checks. Directory enumeration uses `fdopendir` on the verified descriptor, so replacing a parent path cannot redirect the listing. A model or hosted renderer cannot answer the local consent dialog. These permissions govern the native file tools; the separately enabled terminal still runs with the user's OS privileges. +Approved native reads can return bounded text, directory listings, images, or PDFs to the chat. Approved imports transfer file bytes to Workspace Files. Electron revalidates every pending call before using a grant, including remembered grants, checks canonical containment and grant identity throughout the operation, and opens files with no-follow and descriptor identity checks. Directory enumeration uses `fdopendir` on the verified descriptor, so replacing a parent path cannot redirect the listing. Listings scan at most 1,001 entries and return up to 1,000 sorted names with an explicit truncation flag; the cap bounds both memory and filesystem work, rather than promising the globally first 1,000 names in an arbitrarily large directory. Imports reject truncated listings. A model or hosted renderer cannot answer the local consent dialog. These permissions govern the native file tools; the separately enabled terminal still runs with the user's OS privileges. ## Auto-update, channels, rollout, rollback diff --git a/apps/desktop/e2e/local-files.spec.ts b/apps/desktop/e2e/local-files.spec.ts index ae4aa4c1dd2..ec33650a0a5 100644 --- a/apps/desktop/e2e/local-files.spec.ts +++ b/apps/desktop/e2e/local-files.spec.ts @@ -317,6 +317,40 @@ test('native file tools remember folder consent across chats and restarts until rmSync(otherParent, { recursive: true, force: true }) } }) + await test.step('a replaced directory cannot return a listing for its old contents', async () => { + const directory = join(realpathSync(source), 'replace-during-read') + const backup = join(realpathSync(source), 'previous-directory') + mkdirSync(directory) + writeFileSync(join(directory, 'old.txt'), 'old contents') + calls.directoryReplaced = { toolName: 'read_local_file', args: { path: directory } } + await app?.evaluate( + (_electron, paths) => { + const fs = process.getBuiltinModule( + 'node:fs/promises' + ) as typeof import('node:fs/promises') + const original = fs.lstat + fs.lstat = (async (...args: Parameters) => { + const result = await original(...args) + if (args[0] === paths.directory) { + fs.lstat = original + await fs.rename(paths.directory, paths.backup) + await fs.mkdir(paths.directory) + await fs.writeFile(`${paths.directory}/new.txt`, 'new contents') + } + return result + }) as typeof fs.lstat + }, + { directory, backup } + ) + try { + expect(await invoke({ operation: 'read', toolCallId: 'directoryReplaced' })).toMatchObject({ + ok: false, + }) + } finally { + rmSync(directory, { recursive: true, force: true }) + rmSync(backup, { recursive: true, force: true }) + } + }) await test.step('cancelling after a file opens prevents its contents from returning', async () => { await app?.evaluate( (_electron, path) => { diff --git a/apps/desktop/src/main/local-files.ts b/apps/desktop/src/main/local-files.ts index 4e5ae157fae..77f1146cfe7 100644 --- a/apps/desktop/src/main/local-files.ts +++ b/apps/desktop/src/main/local-files.ts @@ -74,7 +74,10 @@ async function readApprovedDirectory(path: string, access: LocalFileAccess) { const handle = await openApprovedPath(path, access, true) try { const listing = await readNativeDirectory(handle.fd, MAX_ENTRIES) - if ((await access.resolve(path)) !== path) + const canonical = await access.resolve(path) + const current = await lstat(canonical) + const opened = await handle.stat() + if (canonical !== path || current.dev !== opened.dev || current.ino !== opened.ino) throw new Error('The local directory changed while it was being read. Try again.') listing.entries.sort((left, right) => compareStrings(left.name, right.name)) return listing