From 8b0d8ea3767c0d5fccc338fbe6e4da6489d64c9d Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Thu, 23 Apr 2026 16:58:17 -0700 Subject: [PATCH] fix(fs): address symlink/delete/remote safety issues from review (#1012) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Symlink delete/rename now preserve the link entry (new `preserveSymlink` option on resolveAuthorizedPath) instead of operating on the target. - File-explorer delete force-saves dirty editors before trashing so undo restores the user's latest edits, not stale disk content. - Thread `recursive` through preload → SSH provider so remote directory delete works end-to-end. - Remote deletes now confirm (permanent `rm`, no Trash) and the toast reflects that no Trash/Recycle Bin is involved. - Codex managed-home check canonicalizes the storage root before the prefix compare so macOS `/private/var` paths stop being rejected. --- src/main/codex-accounts/service.ts | 32 +++++---- src/main/ipc/filesystem-auth.ts | 29 +++++++- src/main/ipc/filesystem-mutations.test.ts | 27 +++++--- src/main/ipc/filesystem-mutations.ts | 10 ++- src/main/ipc/filesystem.ts | 15 +++- src/main/providers/types.ts | 2 +- src/preload/api-types.d.ts | 6 +- src/preload/index.ts | 7 +- .../right-sidebar/useFileDeletion.ts | 68 +++++++++++++++---- 9 files changed, 151 insertions(+), 45 deletions(-) diff --git a/src/main/codex-accounts/service.ts b/src/main/codex-accounts/service.ts index 72e0ccddc9c..3c67b3e94f7 100644 --- a/src/main/codex-accounts/service.ts +++ b/src/main/codex-accounts/service.ts @@ -321,16 +321,6 @@ export class CodexAccountService { const resolvedCandidate = resolve(candidatePath) const resolvedRoot = resolve(rootPath) - // Why: in dev mode, userData points to orca-dev/ while production uses - // orca/. Accounts created by the packaged app store production paths in - // settings. A quick prefix check before realpathSync avoids noisy errors - // when dev instances encounter production-rooted managed home paths. - if (!resolvedCandidate.startsWith(resolvedRoot + sep)) { - throw new Error( - `Managed Codex home is outside current storage root (expected under ${resolvedRoot}).` - ) - } - if (!existsSync(resolvedCandidate)) { throw new Error('Managed Codex home directory does not exist on disk.') } @@ -340,6 +330,21 @@ export class CodexAccountService { // canonical on-disk target rather than trusting persisted text blindly. const canonicalCandidate = realpathSync(resolvedCandidate) const canonicalRoot = realpathSync(resolvedRoot) + + // Why: the prefix check must compare canonical paths on both sides. On + // macOS, userData sits under /var/folders/... which realpath resolves to + // /private/var/folders/...; comparing a canonical candidate against a + // non-canonical root would spuriously reject every managed home. In dev + // mode (orca-dev/ vs orca/) this check also filters out production-rooted + // paths before downstream sync runs. + if ( + canonicalCandidate !== canonicalRoot && + !canonicalCandidate.startsWith(canonicalRoot + sep) + ) { + throw new Error( + `Managed Codex home is outside current storage root (expected under ${canonicalRoot}).` + ) + } const relativePath = relative(canonicalRoot, canonicalCandidate) const escaped = relativePath === '' || relativePath.startsWith('..') || relativePath.includes(`..${sep}`) @@ -370,8 +375,11 @@ export class CodexAccountService { // just the home/ leaf leaves an empty / directory behind. try { const parentDir = resolve(managedHomePath, '..') - const root = this.getManagedAccountsRoot() - if (parentDir.startsWith(root) && parentDir !== root) { + // Why: managedHomePath is already canonicalized by assertManagedHomePath, + // so the root must be canonicalized too for the prefix check to work on + // macOS where userData resolves through /private/var. + const root = realpathSync(this.getManagedAccountsRoot()) + if (parentDir.startsWith(root + sep) && parentDir !== root) { rmSync(parentDir, { recursive: true, force: true }) } } catch { diff --git a/src/main/ipc/filesystem-auth.ts b/src/main/ipc/filesystem-auth.ts index dea8a5c023b..af9aed31e42 100644 --- a/src/main/ipc/filesystem-auth.ts +++ b/src/main/ipc/filesystem-auth.ts @@ -120,12 +120,39 @@ export function isENOENT(error: unknown): boolean { ) } -export async function resolveAuthorizedPath(targetPath: string, store: Store): Promise { +export type ResolveAuthorizedPathOptions = { + /** + * When true, canonicalize the parent directory but preserve the leaf so + * operations target the symlink itself rather than its destination. Required + * for delete and rename — following the symlink would trash or rename the + * target file (which can live outside allowed roots, or be another tracked + * file a symlink inside the worktree happens to point at). + */ + preserveSymlink?: boolean +} + +export async function resolveAuthorizedPath( + targetPath: string, + store: Store, + options: ResolveAuthorizedPathOptions = {} +): Promise { const resolvedTarget = resolve(targetPath) if (!(await isPathAllowedIncludingRegisteredWorktrees(resolvedTarget, store))) { throw new Error(PATH_ACCESS_DENIED_MESSAGE) } + if (options.preserveSymlink) { + // Canonicalize the parent so symlinks in ancestors cannot redirect us + // outside allowed roots, but keep the final segment untouched so callers + // (delete/rename) act on the link itself. + const realParent = await realpath(dirname(resolvedTarget)) + const candidateTarget = resolve(realParent, basename(resolvedTarget)) + if (!(await isPathAllowedIncludingRegisteredWorktrees(candidateTarget, store))) { + throw new Error(PATH_ACCESS_DENIED_MESSAGE) + } + return candidateTarget + } + try { const realTarget = await realpath(resolvedTarget) if (!(await isPathAllowedIncludingRegisteredWorktrees(realTarget, store))) { diff --git a/src/main/ipc/filesystem-mutations.test.ts b/src/main/ipc/filesystem-mutations.test.ts index 2ff70706bb9..3865fc1ef37 100644 --- a/src/main/ipc/filesystem-mutations.test.ts +++ b/src/main/ipc/filesystem-mutations.test.ts @@ -175,34 +175,39 @@ describe('registerFilesystemMutationHandlers', () => { expect(renameMock).not.toHaveBeenCalled() }) - it('rejects rename when new path escapes allowed roots', async () => { + it('rejects rename when parent directory escapes allowed roots', async () => { + // Why: the parent is still canonicalized (preserveSymlink only preserves + // the leaf). A symlinked ancestor that points outside allowed roots must + // still be rejected so callers cannot redirect rename through it. mockRealpath({ - [path.resolve('/workspace/repo/escape.ts')]: path.resolve('/private/escape.ts') + [path.resolve('/workspace/repo/escape-dir')]: path.resolve('/private/escape-dir') }) await expect( handlers.get('fs:rename')!(null, { oldPath: path.resolve('/workspace/repo/old.ts'), - newPath: path.resolve('/workspace/repo/escape.ts') + newPath: path.resolve('/workspace/repo/escape-dir/new.ts') }) ).rejects.toThrow('Access denied') expect(renameMock).not.toHaveBeenCalled() }) - it('rejects rename when old path escapes allowed roots', async () => { + it('renames a symlink without following its target', async () => { + // Why: rename must operate on the symlink entry, not its target — + // following the link would rename the target file (possibly elsewhere in + // the worktree, or outside allowed roots entirely). Even though the + // symlink points at /private/secret.ts, renaming the link entry inside + // the allowed root is a safe directory-entry mutation. mockRealpath({ [path.resolve('/workspace/repo/symlink.ts')]: path.resolve('/private/secret.ts') }) - await expect( - handlers.get('fs:rename')!(null, { - oldPath: path.resolve('/workspace/repo/symlink.ts'), - newPath: path.resolve('/workspace/repo/new.ts') - }) - ).rejects.toThrow('Access denied') + const oldPath = path.resolve('/workspace/repo/symlink.ts') + const newPath = path.resolve('/workspace/repo/renamed-symlink.ts') + await handlers.get('fs:rename')!(null, { oldPath, newPath }) - expect(renameMock).not.toHaveBeenCalled() + expect(renameMock).toHaveBeenCalledWith(oldPath, newPath) }) // ── Edge cases ───────────────────────────────────────────────── diff --git a/src/main/ipc/filesystem-mutations.ts b/src/main/ipc/filesystem-mutations.ts index 48608908ab3..99bbf12bca2 100644 --- a/src/main/ipc/filesystem-mutations.ts +++ b/src/main/ipc/filesystem-mutations.ts @@ -103,8 +103,14 @@ export function registerFilesystemMutationHandlers(store: Store): void { } return provider.rename(args.oldPath, args.newPath) } - const oldPath = await resolveAuthorizedPath(args.oldPath, store) - const newPath = await resolveAuthorizedPath(args.newPath, store) + // Why: rename() operates on directory entries, not file contents. If + // oldPath is a symlink, we must rename the link itself rather than + // resolving it to its target — following the link would rename the + // target file (potentially elsewhere in the worktree) and leave the + // symlink dangling. newPath must also preserve its leaf so we don't + // accidentally write into a symlinked destination name. + const oldPath = await resolveAuthorizedPath(args.oldPath, store, { preserveSymlink: true }) + const newPath = await resolveAuthorizedPath(args.newPath, store, { preserveSymlink: true }) await assertNotExists(newPath) await rename(oldPath, newPath) } diff --git a/src/main/ipc/filesystem.ts b/src/main/ipc/filesystem.ts index 0c145e66e57..475fcc988b4 100644 --- a/src/main/ipc/filesystem.ts +++ b/src/main/ipc/filesystem.ts @@ -184,15 +184,24 @@ export function registerFilesystemHandlers(store: Store): void { ipcMain.handle( 'fs:deletePath', - async (_event, args: { targetPath: string; connectionId?: string }): Promise => { + async ( + _event, + args: { targetPath: string; connectionId?: string; recursive?: boolean } + ): Promise => { if (args.connectionId) { const provider = getSshFilesystemProvider(args.connectionId) if (!provider) { throw new Error(`No filesystem provider for connection "${args.connectionId}"`) } - return provider.deletePath(args.targetPath) + return provider.deletePath(args.targetPath, args.recursive) } - const targetPath = await resolveAuthorizedPath(args.targetPath, store) + // Why: deleting must operate on the symlink itself, not its target. + // Following the link with realpath() would trash the real file — which + // could be another file inside the worktree, or a path outside all + // allowed roots that we would never be able to delete again. + const targetPath = await resolveAuthorizedPath(args.targetPath, store, { + preserveSymlink: true + }) // Why: once auto-refresh exists, an external delete can race with a // UI-initiated delete. Swallowing ENOENT keeps the action idempotent diff --git a/src/main/providers/types.ts b/src/main/providers/types.ts index a977cd3980d..57618fb1f37 100644 --- a/src/main/providers/types.ts +++ b/src/main/providers/types.ts @@ -92,7 +92,7 @@ export type IFilesystemProvider = { readFile(filePath: string): Promise writeFile(filePath: string, content: string): Promise stat(filePath: string): Promise - deletePath(targetPath: string): Promise + deletePath(targetPath: string, recursive?: boolean): Promise createFile(filePath: string): Promise createDir(dirPath: string): Promise rename(oldPath: string, newPath: string): Promise diff --git a/src/preload/api-types.d.ts b/src/preload/api-types.d.ts index f494c66b17c..52579205c20 100644 --- a/src/preload/api-types.d.ts +++ b/src/preload/api-types.d.ts @@ -524,7 +524,11 @@ export type PreloadApi = { createFile: (args: { filePath: string; connectionId?: string }) => Promise createDir: (args: { dirPath: string; connectionId?: string }) => Promise rename: (args: { oldPath: string; newPath: string; connectionId?: string }) => Promise - deletePath: (args: { targetPath: string; connectionId?: string }) => Promise + deletePath: (args: { + targetPath: string + connectionId?: string + recursive?: boolean + }) => Promise authorizeExternalPath: (args: { targetPath: string }) => Promise stat: (args: { filePath: string diff --git a/src/preload/index.ts b/src/preload/index.ts index ec656813876..52b98c7acca 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -942,8 +942,11 @@ const api = { ipcRenderer.invoke('fs:createDir', args), rename: (args: { oldPath: string; newPath: string; connectionId?: string }): Promise => ipcRenderer.invoke('fs:rename', args), - deletePath: (args: { targetPath: string; connectionId?: string }): Promise => - ipcRenderer.invoke('fs:deletePath', args), + deletePath: (args: { + targetPath: string + connectionId?: string + recursive?: boolean + }): Promise => ipcRenderer.invoke('fs:deletePath', args), authorizeExternalPath: (args: { targetPath: string }): Promise => ipcRenderer.invoke('fs:authorizeExternalPath', args), stat: (args: { diff --git a/src/renderer/src/components/right-sidebar/useFileDeletion.ts b/src/renderer/src/components/right-sidebar/useFileDeletion.ts index 2650a245b25..44996e80790 100644 --- a/src/renderer/src/components/right-sidebar/useFileDeletion.ts +++ b/src/renderer/src/components/right-sidebar/useFileDeletion.ts @@ -6,7 +6,10 @@ import { dirname } from '@/lib/path' import { getConnectionId } from '@/lib/connection-context' import { isPathEqualOrDescendant } from './file-explorer-paths' import type { TreeNode } from './file-explorer-types' -import { requestEditorSaveQuiesce } from '@/components/editor/editor-autosave' +import { + requestEditorFileSave, + requestEditorSaveQuiesce +} from '@/components/editor/editor-autosave' import { commitFileExplorerOp } from './fileExplorerUndoRedo' type UseFileDeletionParams = { @@ -14,6 +17,7 @@ type UseFileDeletionParams = { openFiles: { id: string filePath: string + isDirty?: boolean }[] closeFile: (fileId: string) => void refreshDir: (dirPath: string) => Promise @@ -49,16 +53,39 @@ export function useFileDeletion({ } inFlightRef.current.add(node.path) + const connectionId = getConnectionId(activeWorktreeId ?? null) ?? undefined + const isRemote = connectionId !== undefined + + // Why: remote deletes go through `rm` on the relay — there is no OS-level + // Trash/Recycle Bin, so the operation is permanent. Require an explicit + // confirmation in that case because the UI's usual undo cannot restore + // directories or binary files. + if (isRemote) { + const message = node.isDirectory + ? `Permanently delete '${node.name}' and all its contents? This cannot be undone.` + : `Permanently delete '${node.name}'? This cannot be undone.` + if (!window.confirm(message)) { + inFlightRef.current.delete(node.path) + return + } + } + try { const filesToClose = openFiles.filter((file) => isPathEqualOrDescendant(file.filePath, node.path) ) - // Why: moving a file to Trash/Recycle Bin is another external mutation of - // the file path. Let any in-flight autosave finish first so the delete - // action cannot be undone by a trailing write that recreates the file. + // Why: force-save any dirty buffers before trashing so the undo snapshot + // reads the user's latest edits from disk — not an older version that + // predates debounced autosave or a buffer with autosave disabled. + // Quiesce-only would cancel pending timers and discard those edits. + // If a save fails, surface the error and abort the delete instead of + // silently trashing the stale on-disk content. + const dirtyFiles = filesToClose.filter((file) => file.isDirty) + await Promise.all(dirtyFiles.map((file) => requestEditorFileSave({ fileId: file.id }))) + // After saving, quiesce any remaining scheduled autosaves so trailing + // writes cannot recreate the file after it's been trashed. await Promise.all(filesToClose.map((file) => requestEditorSaveQuiesce({ fileId: file.id }))) - const connectionId = getConnectionId(activeWorktreeId ?? null) ?? undefined const parentDir = dirname(node.path) // Why: read file content before deleting so undo can restore it. // We capture content first but only commit the undo entry after the @@ -76,7 +103,11 @@ export function useFileDeletion({ } } - await window.api.fs.deletePath({ targetPath: node.path, connectionId }) + await window.api.fs.deletePath({ + targetPath: node.path, + connectionId, + recursive: node.isDirectory + }) if (undoContent !== undefined) { commitFileExplorerOp({ @@ -89,7 +120,11 @@ export function useFileDeletion({ await refreshDir(parentDir) }, redo: async () => { - await window.api.fs.deletePath({ targetPath: node.path, connectionId }) + await window.api.fs.deletePath({ + targetPath: node.path, + connectionId, + recursive: node.isDirectory + }) await refreshDir(parentDir) } }) @@ -129,10 +164,18 @@ export function useFileDeletion({ // full-tree reloads (the watcher will also trigger a targeted refresh). await refreshDir(dirname(node.path)) - const destination = isWindows ? 'Recycle Bin' : 'Trash' - toast.success(`'${node.name}' moved to ${destination}`) + // Why: local deletes go to the OS trash and are recoverable; remote + // deletes call `rm` on the relay and are permanent. The toast needs + // to reflect that so users aren't misled into thinking they can + // recover a remote file from a Trash/Recycle Bin that doesn't exist. + if (isRemote) { + toast.success(`'${node.name}' deleted`) + } else { + const destination = isWindows ? 'Recycle Bin' : 'Trash' + toast.success(`'${node.name}' moved to ${destination}`) + } } catch (error) { - const action = isWindows ? 'move to Recycle Bin' : 'move to Trash' + const action = isRemote ? 'delete' : isWindows ? 'move to Recycle Bin' : 'move to Trash' toast.error(error instanceof Error ? error.message : `Failed to ${action} '${node.name}'.`) } finally { inFlightRef.current.delete(node.path) @@ -144,8 +187,9 @@ export function useFileDeletion({ const requestDelete = useCallback( (node: TreeNode) => { setSelectedPath(node.path) - // Why: per product decision, skip the confirmation dialog — trashing is - // reversible (OS-level trash + in-app undo), so the extra prompt is noise. + // Why: local deletes skip confirmation because they're reversible + // (OS-level Trash + in-app undo). Remote deletes are permanent, so + // runDelete prompts for confirmation internally before calling `rm`. void runDelete(node) }, [runDelete, setSelectedPath]