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.
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.
With Phase 3's image cache in place, the picker no longer hands the
renderer an absolute path or produces file:// URLs. The two legacy
IPCs that still accepted renderer-supplied absolute paths are now
dead surface area:
- READ_LOCAL_IMAGE_AS_DATA_URL — read any image-extension file
outside userData and inline it as base64. The renderer could
swap the stored value for any matching path between renders.
- TO_FILE_URL — convert any path outside userData into a file://
URL the renderer could embed. Was the precondition for the
above.
Both are removed entirely (handlers, preload bindings, type
signatures, IPC enum entries). The remaining image flow is:
showOpenDialog (proven user intent)
→ imageCacheImport(path) — main copies into private cache
→ renderer stores `image:<id>`
→ imageCacheGetDataUrl(id) per render
Migration: existing configs that hold `file://` values from older
builds render no background until the user re-picks. The resolver
util explicitly detects the legacy shape and returns null with a
comment pointing to this commit. Same migration model we used for
the sync folder (Phase 2): a one-time re-pick is the cost of
closing the security gap, and the picker UI is already in place.
Also drops the now-stale `IS_ELECTRON` early-return in the resolver
util — `window.ea?.imageCacheGetDataUrl` is undefined on web,
short-circuits naturally, and the implicit check made the spec
unable to exercise the Electron branch.
Tests updated: assertion that the legacy handlers are no longer
registered (a compromised renderer that still tries the IPC must
see no handler), and a positive `image:<id>` round-trip in the
karma util spec. 65 electron security tests + 5 util spec all
green; lint + tsc clean.
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.
Post-review of Phase 2 (#8228). The reviewer flagged one real bug and
several pieces of premature scaffolding.
Real bug — finding #1: the sync form was still writing the sync folder
path back into the renderer credential store. Phase 2 made the IPCs
take a relative path and main own the canonical root, but the Formly
button at sync-form.const.ts:200 had `key: 'syncFolderPath'`, which
caused FormlyBtn.onClick to setValue(picker-return-string) on the
form model. That flowed into privateCfg.syncFolderPath on save. The
"renderer no longer holds the authoritative copy" claim was true in
the IPC contract but untrue at runtime. A compromised renderer could
keep mutating its own privateCfg.syncFolderPath; nothing on master
reads it after Phase 2, but any future code path that does would
re-open the hole. Fix:
- drop `syncFolderPath` from PROVIDER_FIELD_DEFAULTS[LocalFile] so it
is not in the LocalFile credential-store defaults at all
- drop `key: 'syncFolderPath'` from the LocalFile picker button so the
picker's return value is not bound to the form model
KISS simplifications (a/b/d from review):
- delete electron/sync-folder-store.ts and its test; inline the
load/cache/persist helpers at the top of local-file-sync.ts as
~25 LOC. The dedicated module was carrying an init/get/set ceremony
and a Promise singleton for a single string that is read at most a
few times per second in the worst case. simple-store is the durable
layer either way.
- drop the `_resolveOrThrow` typeof precheck; resolveSyncPath already
type-checks `relativePath` and throws the same opaque error
- inline `resolveSyncPath(...).absolutePath` into each handler; the
helper was the third layer doing the same thing
Other findings addressed:
- finding #2 (PICK_DIRECTORY silently undefined on persist failure):
PICK_DIRECTORY now returns a safe Error via createSafeIpcError when
realpath/persist fails, distinct from `undefined` for user-cancel.
New tests cover both branches plus the "cache must not be poisoned
with a non-existent path" invariant.
- finding #4 (set-without-canonicalize): the inlined
setSyncFolderPath now `realpathSync.native`s the picked path before
persisting, so a relative or symlinked picker result is rejected
early and the on-disk form matches what resolveSyncPath compares
against on read.
- finding #5 (fire-and-forget init that was also being awaited per
handler): dropped the fire-and-forget; handlers just `await
getSyncFolderPath()`, which lazy-loads from disk on first call and
caches.
All 52 electron security tests still pass; lint clean; tsc clean.
Phase 2 of #8228. Closes the renderer-supplied-absolute-path hole: the
file-sync IPCs now take a relative path; main resolves it against a
sync folder it owns and never trusts the renderer's copy of.
Main side
- sync-folder-store: persists the path in simple-store (still inside
userData, still renderer-unreachable via assertPathOutside) with an
in-memory cache so per-IPC lookup doesn't stat the file every call.
initSyncFolderStore() is fire-and-forget at adapter init; each handler
awaits it defensively because it's idempotent via _loadOnce.
- FILE_SYNC_SAVE/LOAD/REMOVE: accept { relativePath, ... }, pass through
resolveSyncPath, write/read/delete the vetted absolute path.
- CHECK_DIR_EXISTS and FILE_SYNC_LIST_FILES: accept an optional
relativePath; default to the sync root itself so the existing
"is the sync folder reachable?" check still works.
- PICK_DIRECTORY: now persists the chosen folder via setSyncFolderPath
before returning the display string to the renderer. The renderer
receiving the string is no longer load-bearing — it's display only.
- New GET_SYNC_FOLDER_PATH IPC returns the current value for display
(settings form, "sync folder: …" labels).
- TO_FILE_URL: pulls in the assertPathOutside(userData) backstop that
was missing on master, so a userData path cannot be laundered into a
file:// URL that later flows through READ_LOCAL_IMAGE_AS_DATA_URL or
the navigation guard.
Package (@sp/sync-providers/local-file)
- LocalFileSyncElectron.isReady now asks main (getMainSyncFolderPath
dep) instead of reading the renderer's privateCfg. This drives the
migration UX: an existing install with a legacy privateCfg value but
no main-side path lands on "not configured" and gets re-prompted by
the picker. A one-cycle "please pick again" is the migration cost; we
decided against silently promoting the renderer's persisted value
because it could be tampered with.
- getFilePath returns the *relative* path (no absolute folder prefix).
The leading-slash strip is preserved so existing call sites that
prepend '/' still work.
- pickDirectory no longer writes to renderer privateCfg — main owns
persistence. The dep contract still returns the display string.
- The privateCfg.syncFolderPath field is marked @deprecated; the
field is kept for migration of older configs and may be removed in a
later pass once the migration has rolled out.
Renderer
- ElectronFileAdapter forwards the relative path verbatim to the IPC.
- The renderer's LocalFileSyncElectron deps wire pickDirectory and the
new getMainSyncFolderPath to the bridge.
Tests
- electron/local-file-sync.test.cjs rewritten for the new signatures:
happy path, traversal escapes, absolute-relativePath rejection,
sync-folder-equals-userData rejection (grant-file forgery defense),
PICK persists main-side, GET returns the configured path, TO_FILE_URL
refuses userData.
- electron/sync-folder-store.test.cjs covers persistence,
init-before-get throw, null/empty handling, idempotent set.
- The package's local-file-sync-electron.spec.ts is updated for the new
deps shape and asserts the package no longer writes to the renderer
credential store.
All 56 electron-side tests pass; 374 sync-providers tests pass.
* fix(electron): lock in-window navigation to the app's loaded origin
The previous will-navigate handler accepted any URL whose hostname was
'localhost' or '127.0.0.1', which left the privileged main window
reachable by any local web server (e.g. http://127.0.0.1:1337/) — the
preload bridge (window.ea) would have been exposed to whatever page that
server returned. In production the legitimate origin is file:// only, so
there is no excuse for ever navigating to http://localhost in-window.
Compare against the URL the app actually loaded (captured at loadURL
time) instead of a hostname allowlist. http/https requires exact
host+port match; file:// requires the same html pathname; data:/blob:/
javascript: are rejected outright. The same guard is also applied to
will-redirect so a same-origin start cannot be redirected onto an
attacker page mid-navigation, and a defensive did-create-window handler
destroys any unexpected child window (the deny-all setWindowOpenHandler
should make that path unreachable).
Also tightens TO_FILE_URL with the same userData deny used by the other
file-sync IPCs, so a plugin/XSS cannot launder a userData path into a
file:// URL that later flows through READ_LOCAL_IMAGE_AS_DATA_URL.
* fix(electron): reject UNC/remote-host file:// in navigation guard
`new URL('file://192.168.1.100/Applications/SP.app/Contents/Resources/index.html').pathname`
equals the local app's pathname, so the previous pathname-only check
considered an attacker-controlled UNC host same-origin and let it load
in the privileged main window. The app's loaded file:// URL is always
local (empty host), so require `target.host === ''` explicitly.
* test(electron): add userinfo-@-trick navigation guard regression
`http://localhost:4200@evil.com/` parses with host=evil.com and
username=localhost — a naive substring or startsWith check would be
fooled. The existing host-equality check already rejects it; lock that
in with a test so a future refactor cannot regress.
* fix(security): canonicalize file-path-guard and restrict FILE_SYNC_* to outside userData
file-path-guard now canonicalizes via fs.realpathSync.native (deepest existing
ancestor) before the path.relative check, so symlinks and case-insensitive / 8.3
filesystem aliases can't bypass containment — this also hardens the existing
backup-dir (GHSA-x937) check, which was purely lexical and bypassable on macOS.
Adds assertPathOutside and applies it to every FILE_SYNC_* handler and
READ_LOCAL_IMAGE_AS_DATA_URL in local-file-sync.ts, so the renderer (untrusted
plugin/XSS) can no longer read/write/delete/list/inline files inside the app's
private dir (userData: settings/grants/db) while user-chosen sync folders still
work.
* fix(security): validate clipboard image basePath, fileName and copy source
ipc-handler-wrapper now uses isPathInsideDir containment (not a bare startsWith,
which accepted a sibling like <userData>-evil) and rejects any fileName that is
not a plain image basename or imageId containing separators/.. — closing the
clipboard grant-file forgery (fileName='simpleSettings'). CLIPBOARD_COPY_IMAGE_FILE
now requires an image-extension source. The supported-image-extension allowlist
is consolidated into a single SUPPORTED_IMAGE_EXTENSIONS in mime-type-mapping.
