refactor(electron): consistency + UX nits from pre-merge review

Pre-merge review of #8228 flagged four real items in the otherwise
solid security boundary. All non-blocking; rolled into one pass:

1) IMAGE_PICK_AND_IMPORT was the only renderer-callable handler that
   threw raw Errors back across IPC. The FS handlers all funnel
   through createSafeIpcError, which strips e.stack so main-bundle
   paths can't leak via the renderer's error-handling. Switch
   IMAGE_PICK_AND_IMPORT to the same shape: returns Error on
   validation failure, null on user cancel, value on success.
   Renderer adapter uses `instanceof Error` (parity with fileSyncLoad
   et al). New test asserts the returned error has no path content
   and no stack.

2) The image picker button was not disabled while the IPC was in
   flight. A second click would queue a second IPC, opening a second
   dialog after the first resolves; because `replacesId` is computed
   from the form value at click time, the first import's id would
   never be GC'd. Add an `isPickerBusy` signal and bind the button's
   `disabled` to it. New spec asserts the second concurrent click is a
   no-op and the busy flag clears in the `finally`.

3) Stale comment in `local-file.model.ts` pointed at
   `electron/sync-folder-store.ts`, which doesn't exist (the storage
   was inlined into local-file-sync.ts in Phase 2.5). Update to
   describe the actual current shape and clarify that the field is
   read once on upgrade for the migration breadcrumb and never written
   from new code paths.

4) Image input placeholder advertised `file://` URLs as a valid
   input. Phase 4 removed the IPC that resolved them, so the
   placeholder was a footgun. Drop the `file://` half.

68/68 electron security tests; 8/8 formly-image-input karma; lint +
tsc clean.
This commit is contained in:
johannesjo 2026-06-10 00:17:48 +02:00
parent 38d2b16a82
commit 2b628a0a50
8 changed files with 112 additions and 61 deletions

View file

@ -92,7 +92,7 @@ export interface ElectronAPI {
*/
imagePickAndImport(args?: {
replacesId?: string;
}): Promise<{ id: string; mimeType: string } | null>;
}): Promise<{ id: string; mimeType: string } | null | Error>;
/**
* Resolve a cached image id to a `data:` URL the renderer can use as a

View file

@ -279,16 +279,17 @@ test('IMAGE_PICK_AND_IMPORT returns null when the user cancels', async () => {
assert.equal(result, null);
});
test('IMAGE_PICK_AND_IMPORT throws when the picked file fails validation', async () => {
// File inside userData → importImage returns null → handler throws so the
// renderer can show an error snack (vs a silent null for user-cancel).
test('IMAGE_PICK_AND_IMPORT returns a safe Error when validation fails (no path leak)', async () => {
// File inside userData → importImage returns null → handler returns a
// safe Error so the renderer can show a snack (vs a silent null for cancel).
// The error message must not echo back the renderer-picked path.
const planted = path.join(userDataDir, 'planted.png');
fs.writeFileSync(planted, 'fake');
nextDialogResult = { canceled: false, filePaths: [planted] };
await assert.rejects(
() => handlers['IMAGE_PICK_AND_IMPORT']({}, undefined),
/could not be imported/,
);
const result = await handlers['IMAGE_PICK_AND_IMPORT']({}, undefined);
assert.ok(result instanceof Error);
assert.ok(!String(result.message).includes(planted), 'no path in message');
assert.equal(result.stack, undefined, 'stack stripped, no main-bundle paths');
});
test('IMAGE_PICK_AND_IMPORT garbage-collects the prior cached image on replace', async () => {

View file

@ -252,38 +252,48 @@ export const initLocalFileSyncAdapter = (): void => {
async (
_,
args?: { replacesId?: string },
): Promise<{ id: string; mimeType: string } | null> => {
): Promise<{ id: string; mimeType: string } | null | Error> => {
// SECURITY: the dialog + import are atomic and run together in main.
// The renderer never holds the absolute path — it cannot trigger an
// image read without a user clicking through the native picker.
// (The previous shape exposed an `importImage(path)` IPC that the
// renderer could call with any image-extension path it pleased; this
// version closes that gap.)
const { canceled, filePaths } = (await dialog.showOpenDialog(getWin(), {
title: 'Select image',
buttonLabel: 'Select',
properties: ['openFile'],
filters: [{ name: 'Images', extensions: ['png', 'jpg', 'jpeg', 'gif', 'webp'] }],
})) as unknown as { canceled: boolean; filePaths: string[] };
if (canceled || !filePaths[0]) {
// User cancelled — distinct from validation failure so the renderer
// can stay silent instead of showing a "couldn't read image" snack.
return null;
try {
const { canceled, filePaths } = (await dialog.showOpenDialog(getWin(), {
title: 'Select image',
buttonLabel: 'Select',
properties: ['openFile'],
filters: [
{ name: 'Images', extensions: ['png', 'jpg', 'jpeg', 'gif', 'webp'] },
],
})) as unknown as { canceled: boolean; filePaths: string[] };
if (canceled || !filePaths[0]) {
// User cancelled — distinct from validation failure so the renderer
// can stay silent instead of showing a "couldn't read image" snack.
return null;
}
const imported = await importImage(filePaths[0]);
if (!imported) {
// Reject as an error: validation failed (extension, size, etc.) so
// the renderer surfaces the error path, not the cancel path. The
// safe-error wrapper below strips the stack and renames the error
// so it matches the FS handlers' contract — main bundle paths
// (from a raw `e.stack`) are not leaked.
throw new Error('Selected image could not be imported');
}
if (typeof args?.replacesId === 'string' && args.replacesId) {
// GC: drop the file the renderer is about to overwrite in its config.
// Renderer-supplied id is opaque; worst case is removing a cached
// image that wasn't actually orphaned, which the user can recover by
// re-picking.
await removeCachedImage(args.replacesId);
}
return imported;
} catch (e) {
error('Image pick-and-import failed', getSafeErrorMeta(e));
return createSafeIpcError(IPC.IMAGE_PICK_AND_IMPORT, e);
}
const imported = await importImage(filePaths[0]);
if (!imported) {
// Reject as an error: validation failed (extension, size, etc.) so
// the renderer surfaces the error path, not the cancel path.
throw new Error('Selected image could not be imported');
}
if (typeof args?.replacesId === 'string' && args.replacesId) {
// GC: drop the file the renderer is about to overwrite in its config.
// Renderer-supplied id is opaque; worst case is removing a cached
// image that wasn't actually orphaned, which the user can recover by
// re-picking.
await removeCachedImage(args.replacesId);
}
return imported;
},
);

View file

@ -91,10 +91,14 @@ const ea: ElectronAPI = {
}) => _invoke('SHOW_OPEN_DIALOG', options) as Promise<string[] | undefined>,
imagePickAndImport: (args) =>
_invoke('IMAGE_PICK_AND_IMPORT', args) as Promise<{
id: string;
mimeType: string;
} | null>,
_invoke('IMAGE_PICK_AND_IMPORT', args) as Promise<
| {
id: string;
mimeType: string;
}
| null
| Error
>,
imageCacheGetDataUrl: (id: string) =>
_invoke('IMAGE_CACHE_GET_DATA_URL', id) as Promise<string | null>,
// STANDARD