End-to-end review of the #8228 sweep flagged one real residual risk
and several smaller items. This commit addresses them in one pass.
The IMAGE_CACHE_IMPORT IPC accepted a renderer-supplied absolute path
with no link to a recent SHOW_OPEN_DIALOG signal. A compromised
renderer could call importImage() with any image-extension path under
5 MB outside userData — same information-disclosure shape the legacy
READ_LOCAL_IMAGE_AS_DATA_URL had, just one-shot instead of repeatable.
Fix: merge the dialog and the import into a single atomic IPC,
IMAGE_PICK_AND_IMPORT. Main opens the picker inline and only calls
importImage() with the file the user actually clicked. The renderer
no longer holds a path at any point in the flow; an attacker who
controls the renderer cannot trigger an image read without a real
user dialog interaction.
Shape change:
- Return null on user-cancel; throw on validation failure. This
preserves the existing UX where cancel is silent and an unreadable
image shows a snack. Renderer-side openFileExplorer is a try/catch.
- Optional { replacesId } arg garbage-collects the previously
cached image when the user picks a new one. Closes the disk-fills
follow-up the review raised (#3) without an extra IPC round-trip.
Smaller items from the review folded in:
- Extract _resolveRelative helper in local-file-sync.ts; the five
FS handlers had the same three-line incantation copy-pasted. The
Phase 2.5 simplification over-corrected by inlining everything.
- Drop the dead __resetSyncFolderCacheForTests export; no caller, and
the tests reset via require.cache surgery anyway.
- image-cache.ts getExt now uses path.extname so Windows backslash
paths are handled correctly (the old slash-only split worked by
accident).
- resolveBgImageToDataUrl: short-circuit on empty id, and log a
console.warn when a legacy file:// is detected (no resolvable IPC
remains; user must re-pick). i18n snack is a documented follow-up.
- Package LocalFileSyncElectron.isReady now logs a critical line via
the injected sync logger when isReady=false but the renderer's
privateCfg still has the legacy syncFolderPath — dev console
surfaces the "needs re-pick" state without dragging i18n into the
security PR.
- Stale comment in electron-file-adapter.ts referenced a deleted file
(sync-folder-store.ts is inlined into local-file-sync.ts now).
Tests:
- electron/local-file-sync.test.cjs gains four IPC-level cases:
legacy IMAGE_CACHE_IMPORT not registered, pick+import round-trip,
cancel returns null, validation failure throws, replacesId GCs the
prior cached file.
- formly-image-input.component.spec rewritten for the new flow:
asserts imagePickAndImport is called with replacesId when one is in
state, snack on rejection but not on null, no setValue on either.
68/68 electron security tests pass; 7/7 formly-image-input karma;
5/5 resolve-bg-image karma; 17/17 sync-providers package
local-file specs. Lint + tsc clean.
What this commit does NOT do (called out so it isn't forgotten):
- CLIPBOARD_COPY_IMAGE_FILE has the same renderer-supplied-path shape
for clipboard images. Parallel issue; out of scope for #8228.
- PICK_DIRECTORY has no defaultPath — could nudge users toward a
dedicated sync subfolder vs picking ~/Documents. Cosmetic.
- Symlink TOCTOU between resolveSyncPath and writeFileSync stands;
O_NOFOLLOW-on-open is the proper fix and needs cross-platform care.
- Migration UX is console-only; a user-visible snack needs i18n
infrastructure not appropriate for the security commit.
The background-image picker previously gave the renderer an absolute
path to an arbitrary image-extension file and stored it as a file://
URL in user config. The render path then asked main to inline that
file:// on every theme apply. Main only refused paths inside userData,
so a compromised renderer could swap the stored value to any png/jpg/
gif/webp/svg path outside userData and re-read it for as long as it
stayed in config.
Switch the picker to copy-to-cache, serve by opaque id:
- electron/image-cache.ts (new): main-owned directory at
`userData/bg-images/<id>.<ext>`. importImage(absolutePath) validates
the source (outside userData, allowed extension, 5 MB cap, non-empty,
is-file), copies it in, and returns a `randomBytes(16)` hex id. The
id is unguessable so a renderer cannot iterate the cache directory
for files it didn't import. getImageDataUrl(id) reads the cached
file and returns a `data:<mime>;base64,…` URL, or null.
- electron/local-file-sync.ts: two new IPCs, IMAGE_CACHE_IMPORT and
IMAGE_CACHE_GET_DATA_URL, registered next to the existing image
handlers.
- electronAPI.d.ts + preload.ts: imageCacheImport / imageCacheGetDataUrl
bindings.
- formly-image-input.component: after showOpenDialog returns a path,
call imageCacheImport(path) and store `image:<id>` in the form
control. Surface the existing "couldn't read image" snack on
rejection. Removes the toFileUrl round-trip and removes the
absolute-path leak into user config.
- resolve-bg-image-to-data-url.util: handle the `image:<id>` prefix by
calling imageCacheGetDataUrl. Legacy `file://` values are still
accepted for back-compat (the existing userData-guarded
READ_LOCAL_IMAGE_AS_DATA_URL handles them); since the picker no
longer produces new file:// values, the long-tail drains as users
re-pick. A future commit can remove the legacy code path.
Tests:
- electron/image-cache.test.cjs: 12 cases — happy path, in-userData
rejection, bad extension, too-large / empty / missing source,
non-string input, unknown / malformed id, cross-restart persistence,
removeCachedImage.
- electron/local-file-sync.test.cjs: 3 new IPC-level cases for
IMAGE_CACHE_IMPORT (round-trip, userData refusal) and
IMAGE_CACHE_GET_DATA_URL (unknown id).
- formly-image-input.component.spec: rewritten to assert
imageCacheImport is called and `image:<id>` is what lands in the
form, plus a failure-path snack assertion.
67 of 67 electron security tests pass; 6 of 6 formly-image-input
karma tests pass; lint and tsc clean.
