From 87820c4772acabeb641a48c3aae7e24dccf7c7c7 Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Sun, 30 Aug 2026 07:26:56 +0000 Subject: [PATCH 1/2] desktop: right-click file operations in both panes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refresh, New folder, New file, Rename and Delete, on a context menu, in the local pane and the remote one alike — the FileZilla operations the panes looked like they already had. F2 renames and Del deletes, because a file manager that answers only the mouse is half a file manager. The remote half already had mkdir/rename/delete over SFTP and nothing called them; the local half had no mutations at all. Both sides now go through one bridge where the connection id is what selects local or remote, so a caller cannot act on the wrong pane by picking the wrong method name. Every mutation takes a directory and a bare entry name and joins them in the main process. EntryNameSchema rejects a separator, `..`, a NUL and surrounding space, so "New folder" in a listing cannot write outside the folder being listed — on the remote side, anywhere the SSH user can reach. Delete re-stats its target and refuses when what is on disk disagrees with what the renderer claimed, so a mislabelled request cannot turn one unlink into a recursive delete; a symlink is unlinked, never followed. Two things SFTP does not give you: creating a file (open 'wx', which fails rather than truncating an existing one) and removing a populated directory (rmdir only unlinks an empty one, so removeRecursive walks it depth first). Verified in a headless render of the real export under the app's real CSP, both themes: the menu opens on a row with all five items live, the row it opened on becomes the selection, and the New folder dialog focuses its field with its footer inside the panel. That render caught the first draft's bug — the container's handler ran after the row's as the event bubbled and cleared the target, leaving Rename and Delete greyed out on every row. 20 new schema tests cover the traversal cases. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GTQ3RzTAey9nT6r1kbGBCd --- apps/desktop/electron/main/ipc.ts | 138 ++++++++++---- apps/desktop/electron/preload/index.ts | 19 +- apps/desktop/electron/shared/contract.test.ts | 56 ++++++ apps/desktop/electron/shared/contract.ts | 51 +++++ apps/desktop/src/components/entry-dialogs.tsx | 141 ++++++++++++++ apps/desktop/src/components/pane.tsx | 175 +++++++++++++++++- .../src/components/ui/context-menu.tsx | 90 +++++++++ apps/desktop/src/lib/api.ts | 12 +- packages/ssh-core/src/browser.ts | 34 ++++ 9 files changed, 666 insertions(+), 50 deletions(-) create mode 100644 apps/desktop/src/components/entry-dialogs.tsx create mode 100644 apps/desktop/src/components/ui/context-menu.tsx diff --git a/apps/desktop/electron/main/ipc.ts b/apps/desktop/electron/main/ipc.ts index b3cc42a..1265160 100644 --- a/apps/desktop/electron/main/ipc.ts +++ b/apps/desktop/electron/main/ipc.ts @@ -1,18 +1,19 @@ -import { readFile } from 'node:fs/promises' -import { readdir, stat } from 'node:fs/promises' +import { lstat, mkdir, open, readdir, readFile, rename, rm, stat, unlink } from 'node:fs/promises' import { homedir } from 'node:os' -import { isAbsolute, join, resolve } from 'node:path' +import { isAbsolute, join, posix, resolve } from 'node:path' import { ipcMain, shell, type IpcMainInvokeEvent } from 'electron' -import { probeConnection, parseSshConfig, sshConfigConnections } from '@diskpush/ssh-core' +import { probeConnection, parseSshConfig, sshConfigConnections, type SftpBrowser } from '@diskpush/ssh-core' import { z } from 'zod' import { ConnectionInputSchema, + CreateEntryRequestSchema, + DeleteEntryRequestSchema, ExternalUrlSchema, IPC, JobIdSchema, PathSchema, RemotePathRequestSchema, - RenameRequestSchema, + RenameEntryRequestSchema, TransferRequestSchema, type IpcResult, } from '../shared/contract.js' @@ -51,6 +52,32 @@ function resolveLocalPath(input: string): string { return isAbsolute(expanded) ? expanded : resolve(expanded) } +/** Whether a path exists, without making the caller catch ENOENT. */ +async function exists(path: string): Promise { + try { + await lstat(path) + return true + } catch { + return false + } +} + +/** + * Runs `fn` against an SFTP browser for `connectionId` and always closes it. + * + * Every remote mutation had its own copy of this, and a `finally` that is + * written six times is a `finally` that eventually is not. + */ +async function withBrowser(connectionId: string | undefined, fn: (browser: SftpBrowser) => Promise): Promise { + if (!connectionId) throw new Error('That operation needs a server.') + const browser = await browserFor(await requireConnection(connectionId)) + try { + return await fn(browser) + } finally { + browser.close() + } +} + export function registerIpc(): void { // --- connections --------------------------------------------------------- @@ -164,42 +191,87 @@ export function registerIpc(): void { } }) - handle(IPC.fsMkdirRemote, RemotePathRequestSchema, async ({ connectionId, path }) => { - const connection = await requireConnection(connectionId) - const browser = await browserFor(connection) - try { - await browser.mkdir(path) - return true - } finally { - browser.close() - } + /** + * The mutating operations, local and remote. + * + * Each takes a directory and a bare entry name and joins them here, so the + * renderer names a thing inside the folder it is showing rather than handing + * the main process a path to act on. `resolveLocalPath` still expands `~`, + * but it is applied to the directory only. + */ + handle(IPC.fsMkdirLocal, CreateEntryRequestSchema, async ({ directory, name }) => { + await mkdir(join(resolveLocalPath(directory), name)) + return true }) - handle(IPC.fsRenameRemote, RenameRequestSchema, async ({ connectionId, from, to }) => { - const connection = await requireConnection(connectionId) - const browser = await browserFor(connection) - try { - await browser.rename(from, to) + handle(IPC.fsCreateFileLocal, CreateEntryRequestSchema, async ({ directory, name }) => { + // `wx` fails when the file exists rather than truncating it. + const handle = await open(join(resolveLocalPath(directory), name), 'wx') + await handle.close() + return true + }) + + handle(IPC.fsRenameLocal, RenameEntryRequestSchema, async ({ directory, from, to }) => { + const root = resolveLocalPath(directory) + const target = join(root, to) + // Renaming onto an existing name silently destroys it, so refuse. There is + // an unavoidable race here; it narrows a footgun rather than closing it. + if (await exists(target)) throw new Error(`“${to}” already exists here.`) + await rename(join(root, from), target) + return true + }) + + handle(IPC.fsDeleteLocal, DeleteEntryRequestSchema, async ({ directory, name, isDirectory }) => { + const target = join(resolveLocalPath(directory), name) + const stats = await lstat(target) + // A symlink to a directory reports as a directory to the caller; deleting + // it must still unlink the link rather than recurse into what it points at. + if (stats.isSymbolicLink()) { + await unlink(target) return true - } finally { - browser.close() } + if (stats.isDirectory() !== isDirectory) throw new Error('That item changed on disk; refresh and try again.') + if (isDirectory) await rm(target, { recursive: true }) + else await unlink(target) + return true }) - handle( - IPC.fsDeleteRemote, - z.object({ connectionId: z.string().min(1), path: PathSchema, isDirectory: z.boolean() }), - async ({ connectionId, path, isDirectory }) => { - const connection = await requireConnection(connectionId) - const browser = await browserFor(connection) - try { - if (isDirectory) await browser.rmdir(path) - else await browser.unlink(path) + handle(IPC.fsMkdirRemote, CreateEntryRequestSchema, async ({ connectionId, directory, name }) => + withBrowser(connectionId, async (browser) => { + await browser.mkdir(posix.join(directory, name)) + return true + }), + ) + + handle(IPC.fsCreateFileRemote, CreateEntryRequestSchema, async ({ connectionId, directory, name }) => + withBrowser(connectionId, async (browser) => { + await browser.createFile(posix.join(directory, name)) + return true + }), + ) + + handle(IPC.fsRenameRemote, RenameEntryRequestSchema, async ({ connectionId, directory, from, to }) => + withBrowser(connectionId, async (browser) => { + await browser.rename(posix.join(directory, from), posix.join(directory, to)) + return true + }), + ) + + handle(IPC.fsDeleteRemote, DeleteEntryRequestSchema, async ({ connectionId, directory, name, isDirectory }) => + withBrowser(connectionId, async (browser) => { + const target = posix.join(directory, name) + const stats = await browser.stat(target) + if (stats.type === 'symlink') { + await browser.unlink(target) return true - } finally { - browser.close() } - }, + if ((stats.type === 'directory') !== isDirectory) { + throw new Error('That item changed on the server; refresh and try again.') + } + if (isDirectory) await browser.removeRecursive(target) + else await browser.unlink(target) + return true + }), ) // --- transfers ----------------------------------------------------------- diff --git a/apps/desktop/electron/preload/index.ts b/apps/desktop/electron/preload/index.ts index cee35bb..d6ac72c 100644 --- a/apps/desktop/electron/preload/index.ts +++ b/apps/desktop/electron/preload/index.ts @@ -24,11 +24,20 @@ const api = { homeLocal: () => call(IPC.fsHomeLocal), listLocal: (path: string) => call(IPC.fsListLocal, { path }), listRemote: (connectionId: string, path: string) => call(IPC.fsListRemote, { connectionId, path }), - mkdirRemote: (connectionId: string, path: string) => call(IPC.fsMkdirRemote, { connectionId, path }), - renameRemote: (connectionId: string, from: string, to: string) => - call(IPC.fsRenameRemote, { connectionId, from, to }), - deleteRemote: (connectionId: string, path: string, isDirectory: boolean) => - call(IPC.fsDeleteRemote, { connectionId, path, isDirectory }), + // Mutations name an entry inside a directory; the main process joins them. + mkdir: (directory: string, name: string, connectionId?: string) => + call(connectionId ? IPC.fsMkdirRemote : IPC.fsMkdirLocal, { connectionId, directory, name }), + createFile: (directory: string, name: string, connectionId?: string) => + call(connectionId ? IPC.fsCreateFileRemote : IPC.fsCreateFileLocal, { connectionId, directory, name }), + rename: (directory: string, from: string, to: string, connectionId?: string) => + call(connectionId ? IPC.fsRenameRemote : IPC.fsRenameLocal, { connectionId, directory, from, to }), + remove: (directory: string, name: string, isDirectory: boolean, connectionId?: string) => + call(connectionId ? IPC.fsDeleteRemote : IPC.fsDeleteLocal, { + connectionId, + directory, + name, + isDirectory, + }), }, transfers: { preview: (request: unknown) => call(IPC.transfersPreview, request), diff --git a/apps/desktop/electron/shared/contract.test.ts b/apps/desktop/electron/shared/contract.test.ts index 7e7728e..b6ae6c9 100644 --- a/apps/desktop/electron/shared/contract.test.ts +++ b/apps/desktop/electron/shared/contract.test.ts @@ -1,9 +1,13 @@ import { describe, expect, it } from 'vitest' import { ConnectionInputSchema, + CreateEntryRequestSchema, + DeleteEntryRequestSchema, EndpointRefSchema, + EntryNameSchema, ExternalUrlSchema, PathSchema, + RenameEntryRequestSchema, TransferOptionsSchema, TransferRequestSchema, } from './contract.js' @@ -141,3 +145,55 @@ describe('ConnectionInputSchema', () => { expect(ConnectionInputSchema.parse({ name: 'x', host: 'h', username: 'u' }).forwardAgent).toBe(false) }) }) + +describe('EntryNameSchema', () => { + /** + * Every mutating file operation joins a directory with one of these in the + * main process. If a name can carry a separator or a `..`, then "new folder" + * in a listing of /home/you is a way to write anywhere on the disk — and on + * the remote side, anywhere the SSH user can reach. + */ + it.each(['../etc', 'a/b', 'a\\b', '..', '.', '', 'x\0y', ' leading', 'trailing '])( + 'rejects %j', + (name) => { + expect(EntryNameSchema.safeParse(name).success).toBe(false) + }, + ) + + it.each(['notes.md', '.zshrc', 'a b c', 'München', 'file.tar.gz', '-rf'])('accepts %j', (name) => { + expect(EntryNameSchema.safeParse(name).success).toBe(true) + }) + + it('caps a name at 255 bytes, the limit every filesystem here shares', () => { + expect(EntryNameSchema.safeParse('a'.repeat(255)).success).toBe(true) + expect(EntryNameSchema.safeParse('a'.repeat(256)).success).toBe(false) + }) +}) + +describe('the mutating request schemas', () => { + it('takes a directory and a name, never a path to act on', () => { + const parsed = CreateEntryRequestSchema.parse({ directory: '/home/you', name: 'reports' }) + expect(parsed).toEqual({ directory: '/home/you', name: 'reports' }) + }) + + it('refuses a traversal in any name field', () => { + expect(CreateEntryRequestSchema.safeParse({ directory: '/home/you', name: '../x' }).success).toBe(false) + expect( + RenameEntryRequestSchema.safeParse({ directory: '/home/you', from: 'a', to: '../b' }).success, + ).toBe(false) + expect( + DeleteEntryRequestSchema.safeParse({ directory: '/home/you', name: '../b', isDirectory: false }).success, + ).toBe(false) + }) + + it('makes connectionId optional, because omitting it is what selects local', () => { + expect(CreateEntryRequestSchema.parse({ directory: '/tmp', name: 'x' }).connectionId).toBeUndefined() + expect( + CreateEntryRequestSchema.parse({ connectionId: 'ssh-config:dev', directory: '/tmp', name: 'x' }).connectionId, + ).toBe('ssh-config:dev') + }) + + it('requires isDirectory on a delete, so the caller cannot leave it to chance', () => { + expect(DeleteEntryRequestSchema.safeParse({ directory: '/tmp', name: 'x' }).success).toBe(false) + }) +}) diff --git a/apps/desktop/electron/shared/contract.ts b/apps/desktop/electron/shared/contract.ts index c3c2287..ee1546a 100644 --- a/apps/desktop/electron/shared/contract.ts +++ b/apps/desktop/electron/shared/contract.ts @@ -23,6 +23,11 @@ export const IPC = { fsMkdirRemote: 'fs:mkdir-remote', fsRenameRemote: 'fs:rename-remote', fsDeleteRemote: 'fs:delete-remote', + fsCreateFileRemote: 'fs:create-file-remote', + fsMkdirLocal: 'fs:mkdir-local', + fsRenameLocal: 'fs:rename-local', + fsDeleteLocal: 'fs:delete-local', + fsCreateFileLocal: 'fs:create-file-local', transfersPreview: 'transfers:preview', transfersStart: 'transfers:start', @@ -119,6 +124,52 @@ export const RenameRequestSchema = z.object({ to: PathSchema, }) +/** + * A single entry name inside a directory — never a path. + * + * Every mutating operation takes a directory plus one of these and joins them + * in the main process, so the renderer cannot walk out of the folder it is + * showing. `..`, a separator or a NUL would each be a way to do exactly that. + */ +export const EntryNameSchema = z + .string() + .min(1) + .max(255) + .refine((name) => !name.includes('/') && !name.includes('\\') && !name.includes('\0'), { + message: 'A name cannot contain a path separator.', + }) + .refine((name) => name !== '.' && name !== '..', { message: 'That name is reserved.' }) + .refine((name) => name.trim() === name, { message: 'A name cannot begin or end with a space.' }) + +/** Create a directory or an empty file: `name` inside `directory`. */ +export const CreateEntryRequestSchema = z.object({ + connectionId: ConnectionIdSchema.optional(), + directory: PathSchema, + name: EntryNameSchema, +}) + +/** Rename `from` to `to`, both inside `directory`. */ +export const RenameEntryRequestSchema = z.object({ + connectionId: ConnectionIdSchema.optional(), + directory: PathSchema, + from: EntryNameSchema, + to: EntryNameSchema, +}) + +/** + * Delete `name` from `directory`. + * + * `isDirectory` is not a hint — the main process refuses when it disagrees + * with what is actually on disk, so a mislabelled request cannot turn a + * single unlink into a recursive delete. + */ +export const DeleteEntryRequestSchema = z.object({ + connectionId: ConnectionIdSchema.optional(), + directory: PathSchema, + name: EntryNameSchema, + isDirectory: z.boolean(), +}) + /** Only http(s) may be handed to the system browser. */ export const ExternalUrlSchema = z.string().url().refine((value) => /^https?:\/\//i.test(value), { message: 'Only http and https URLs can be opened externally.', diff --git a/apps/desktop/src/components/entry-dialogs.tsx b/apps/desktop/src/components/entry-dialogs.tsx new file mode 100644 index 0000000..0c2e0b2 --- /dev/null +++ b/apps/desktop/src/components/entry-dialogs.tsx @@ -0,0 +1,141 @@ +'use client' + +import { useEffect, useState } from 'react' +import { TriangleAlert } from 'lucide-react' +import { Button } from '@/components/ui/button' +import { Dialog, DialogContent, DialogDescription, DialogFooter, DialogHeader, DialogTitle } from '@/components/ui/dialog' +import { Input } from '@/components/ui/input' +import { Label } from '@/components/ui/label' + +/** + * Asks for one name. + * + * Used by New folder, New file and Rename. They differ only in their words and + * their starting value, and three near-identical dialogs would have drifted. + */ +export function NameDialog({ + open, + title, + action, + initialValue = '', + busy, + error, + onSubmit, + onClose, +}: { + open: boolean + title: string + action: string + initialValue?: string + busy: boolean + error: string | null + onSubmit: (name: string) => void + onClose: () => void +}) { + const [value, setValue] = useState(initialValue) + + // Reopening for a different entry must not show the previous entry's name. + useEffect(() => { + if (open) setValue(initialValue) + }, [open, initialValue]) + + const trimmed = value.trim() + const invalid = + trimmed === '' || trimmed === '.' || trimmed === '..' || trimmed.includes('/') || trimmed.includes('\\') + + return ( + !next && onClose()}> + + + {title} + +
{ + event.preventDefault() + if (!invalid && !busy) onSubmit(trimmed) + }} + > +
+ + setValue(event.target.value)} + /> + {/* Said before submitting rather than after failing, because the + main process rejects these names and a round trip to learn so + reads as a bug. */} + {trimmed.includes('/') || trimmed.includes('\\') ? ( +

A name cannot contain a path separator.

+ ) : null} +
+ {error ?

{error}

: null} + + + + +
+ + + ) +} + +/** + * Confirms a delete. + * + * Names the thing and says plainly that a folder takes its contents with it — + * there is no trash on the far side of an SFTP connection. + */ +export function DeleteDialog({ + open, + name, + isDirectory, + where, + busy, + error, + onConfirm, + onClose, +}: { + open: boolean + name: string + isDirectory: boolean + where: string + busy: boolean + error: string | null + onConfirm: () => void + onClose: () => void +}) { + return ( + !next && onClose()}> + + + + + Delete {isDirectory ? 'folder' : 'file'}? + + + {name} on {where} + {isDirectory ? ' and everything inside it' : ''} will be deleted. This cannot be undone. + + + {error ?

{error}

: null} + + + + + + + ) +} diff --git a/apps/desktop/src/components/pane.tsx b/apps/desktop/src/components/pane.tsx index d1ccc6d..0c84b62 100644 --- a/apps/desktop/src/components/pane.tsx +++ b/apps/desktop/src/components/pane.tsx @@ -4,19 +4,32 @@ import { useCallback, useEffect, useMemo, useRef, useState } from 'react' import { ChevronRight, CornerLeftUp, + FilePlus2, FileText, Folder, FolderOpen, + FolderPlus, Link2, + PenLine, RefreshCw, Search, ServerCrash, SearchX, + Trash2, } from 'lucide-react' import { api, unwrap, type Connection, type FileEntry } from '@/lib/api' import { formatBytes, formatDate, formatMode, joinPath, parentPath } from '@/lib/format' import { EndpointSelect, type PaneEndpoint } from '@/components/endpoint-select' +import { DeleteDialog, NameDialog } from '@/components/entry-dialogs' import { Checkbox } from '@/components/ui/checkbox' +import { + ContextMenu, + ContextMenuContent, + ContextMenuItem, + ContextMenuSeparator, + ContextMenuShortcut, + ContextMenuTrigger, +} from '@/components/ui/context-menu' import { Input } from '@/components/ui/input' import { ScrollArea } from '@/components/ui/scroll-area' import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip' @@ -214,6 +227,12 @@ export function Pane({ const [cursor, setCursor] = useState(0) const listRef = useRef(null) const anchor = useRef(null) + // The row the context menu was opened on. Null when it was opened on empty + // space, which is what distinguishes "new file here" from "rename this". + const [target, setTarget] = useState(null) + const [dialog, setDialog] = useState<'mkdir' | 'create-file' | 'rename' | 'delete' | null>(null) + const [busy, setBusy] = useState(false) + const [opError, setOpError] = useState(null) useEffect(() => { setFilter('') @@ -273,6 +292,49 @@ export function Pane({ [onNavigate, state.path], ) + const connectionId = state.endpoint.kind === 'local' ? undefined : state.endpoint.connectionId + + const closeDialog = useCallback(() => { + setDialog(null) + setOpError(null) + }, []) + + /** + * Runs one file operation and reloads the directory. + * + * The listing is re-read from the endpoint rather than patched locally: the + * server is the only thing that knows whether the operation really happened, + * and a pane that shows an optimistic folder which does not exist is worse + * than one that takes a moment. + */ + const run = useCallback( + async (operation: () => Promise) => { + setBusy(true) + setOpError(null) + try { + await operation() + closeDialog() + onNavigate(state.path) + } catch (error) { + setOpError(error instanceof Error ? error.message : String(error)) + } finally { + setBusy(false) + } + }, + [closeDialog, onNavigate, state.path], + ) + + /** Opens the menu against `entry`, selecting it the way a right-click should. */ + const aimAt = useCallback( + (entry: FileEntry | null, index: number) => { + setTarget(entry) + if (!entry) return + setCursor(index) + if (!state.selected.has(entry.name)) onChange({ selected: new Set([entry.name]) }) + }, + [onChange, state.selected], + ) + /** * Arrow keys walk the list, Enter opens, Backspace goes up. * @@ -307,6 +369,14 @@ export function Pane({ case 'Backspace': event.preventDefault() return onNavigate(parentPath(state.path)) + case 'F2': + event.preventDefault() + aimAt(visible[cursor] ?? null, cursor) + return setDialog('rename') + case 'Delete': + event.preventDefault() + aimAt(visible[cursor] ?? null, cursor) + return setDialog('delete') case 'a': if (event.ctrlKey || event.metaKey) { event.preventDefault() @@ -402,14 +472,27 @@ export function Pane({ -
+ + { + if (!(event.target as HTMLElement).closest('[data-row]')) setTarget(null) + }} + className="focus-ring h-full outline-none" + /> + } + > {state.error ? ( open(entry)} + onContextMenu={() => aimAt(entry, index)} className={cn( 'grid h-[var(--row)] cursor-default items-center gap-3 border-l-2 px-3 text-[12.5px] transition-colors', COLUMNS, @@ -487,9 +571,82 @@ export function Pane({ ) }) )} -
+ + + + onNavigate(state.path)}> + + Refresh + + + setDialog('mkdir')}> + + New folder + + setDialog('create-file')}> + + New file + + + setDialog('rename')}> + + Rename + F2 + + setDialog('delete')}> + + Delete + Del + + + + + run(() => + unwrap( + dialog === 'mkdir' + ? api()?.fs.mkdir(state.path, name, connectionId) + : api()?.fs.createFile(state.path, name, connectionId), + ), + ) + } + /> + + + target && run(() => unwrap(api()?.fs.rename(state.path, target.name, name, connectionId))) + } + /> + + + target && + run(() => unwrap(api()?.fs.remove(state.path, target.name, target.type === 'directory', connectionId))) + } + /> +