* docs(plugins): add microsoft 365 calendar provider plan
* docs(plugins): add microsoft 365 calendar provider plan
* docs(ios): plan internal testflight builds
* fix(electron): assert renderer IPC boundary at window creation
Every IPC trust boundary (Jira one-shot capability, plugin node-exec
consent, the window.ea preload bridge) rests on the renderer main world
not having require/ipcRenderer, which is guaranteed solely by
contextIsolation: true + nodeIntegration: false and sub-frames not
getting node integration. If that webPreferences ever silently regressed
(a refactor spreading a shared options object, a bad merge), every gate
would collapse at once while still looking correct in review.
Add web-preferences-guard.ts (assertSecureWebPreferences) and fail closed
before creating a window if the boundary is not intact. It rejects a
non-true contextIsolation, a non-false nodeIntegration, and (fail-closed)
a nodeIntegrationInSubFrames that is not explicitly false; it also
directionally rejects an explicit sandbox: false, nodeIntegrationInWorker:
true, and webviewTag: true (each off by default, so no call site is
forced to set it). Wire it at all three new BrowserWindow sites (main
window, task widget, full-screen blocker); the full-screen blocker
previously relied on Electron defaults, so set its webPreferences
explicitly. A *.test.cjs backs it with behavioral coverage plus a wiring
guard that counts constructor sites vs guard calls per file, so a future
window cannot silently ship without the check.
Closes#9015
* fix(electron): extend webPreferences guard to webSecurity
Follow-up hardening from the multi-agent review of #9018:
- Reject an explicit `webSecurity: false` (directional, like the sandbox
/worker/webviewTag trio). With the app's blanket
Access-Control-Allow-Origin: *, disabling the same-origin policy in a
node-bridged renderer would widen cross-origin reach — and no call site
currently guards against it.
- Broaden the wiring-guard test to also require the assert for
`new BrowserView` / `new WebContentsView`, closing the tripwire's blind
spot for future non-BrowserWindow renderers (none exist today).
- Correct the fail() comment: the `throw` narrows the type regardless of
return-vs-throw; fail() returns an Error only to DRY the message.
230/230 electron tests pass; checkFile + prettier clean.
* docs(plugins): add microsoft 365 calendar provider plan
* fix(jira): gate Electron requests behind one-shot capability
Claim privileged Jira IPC before plugin startup and return responses through invoke instead of a broadcast event. Keep arbitrary HTTP(S) Jira hosts supported while rejecting redirects and bounding request resources.
* fix(jira): enforce Electron request capability
Bind privileged Jira IPC to a main-issued renderer-document token and strip raw Electron events from renderer callbacks. Scope image authentication by origin, base path, and resource type while preserving safe redirects and legacy configurations.
* fix(electron): handle payload-only IPC lifecycle
Clear Jira image authentication before replacement and when a new renderer document claims the capability. Parse before-close IDs from payload-only events so pending sync and finish-day hooks can complete.
* fix(electron): address Jira IPC capability review findings
- electron.effects: read ANY_FILE_DOWNLOADED payload at [0] after the
payload-only IPC refactor (was [1], now undefined -> TypeError on every
download); guard against a malformed payload
- jira-capability: rotate the token on re-register so a renderer reload
that reuses the WebFrameMain object is not permanently locked out of
Jira; invalidates any stale token
- document that the one-shot consumption order, not the bypassable
main-frame check, is the real capability boundary
- jira-electron-bridge: skip the no-op clearImgHeaders IPC round-trip
when image auth was never set up (non-Jira detail-panel open/close)
- jira-api: route a synchronous _toElectronRequestInit throw through
_handleResponse instead of leaking a dangling request-log entry
* test(electron): cover ANY_FILE_DOWNLOADED payload parsing
Extract parseDownloadedFilePayload from ElectronEffects and add a
regression spec pinning the payload-only shape ([file], not [event,
file]) that caused a TypeError on every download. Hardened against
non-array input surfaced by the new test.
* fix(electron): restore Node ambient globals for frontend build
Restore outlook: (reported in #8859) plus the same-class note/task apps
notion:, things:, omnifocus:, bear:, joplin: that the GHSA-hr87 fix also
silently broke. All low-risk registered-app deep-links, same class as the
already-allowed obsidian:/logseq:/zotero:.
planner-task took a non-signal @Input task and exposed per-change-
detection getters (titleHasLinks ran a regex scan, timeEstimate, and
the isDone/isDragReady/isCurrent HostBinding getters) that re-executed
on every CD pass, per row. Convert task to a required signal input,
replace the getters with computed()s, and move the class bindings to
host metadata (mirrors schedule-event). The kb getters in task and
main-header allocated a fresh {} on the no-shortcuts path every CD
pass; return a shared frozen EMPTY_KEYBOARD_CONFIG via a pure
keyboardConfigOrEmpty helper so the value is referentially stable.
Behavior (current-task highlight, drag-ready, done styling, shortcut
hints, sub-task count) is unchanged.
SPAP-28
PATCH /tasks/:id picked allowed keys but never checked value types, so a
wrong-typed value (e.g. tagIds:123, timeEstimate:"abc") was written into
the store and the synced op-log, corrupting state locally and tripping
typia-as-corrupt on other devices when the op replays. Both PATCH and
create now typia.validate the values and return 400 before dispatching.
Also harden the Electron server: reset isListening eagerly and call
closeAllConnections() on stop so a keep-alive socket can't leave the
server stuck and no-op a later re-enable, reject requests with 503 while
disabled, and log an actionable message on EADDRINUSE.
Closes#8732
Refs #7484
* fix(electron): open file:// URLs with special characters (#8695)
Local file:// URLs with non-ASCII names or spaces (e.g. Grüne, "Another
One") failed to open on Windows: both open sinks handed the URL to
shell.openExternal, which percent-encodes the path (ü → %C3%BC, space →
%20) before ShellExecute, so the OS then searched for a literally-named
folder and failed. Pure-ASCII paths worked because there was nothing to
encode.
Route local file: URLs through shell.openPath with a decoded filesystem
path (fileURLToPath) instead. openPath takes a raw path, so Unicode names
and spaces open correctly. This also applies the executable-extension
guard to file: URLs, which previously only ran on the OPEN_PATH sink
(GHSA-hr87-735w-hfq3).
Shared openLocalPath()/isLocalFileUrl() cover both the OPEN_EXTERNAL IPC
handler and the navigation-interception path in main-window.
* test(electron): cover NTLM/exec vectors at the un-pre-gated openPath sink
Follow-up to the #8695 review. Add regression tests locking in that
openLocalPath alone (OPEN_PATH has no scheme pre-gate) blocks the
path-based UNC file: URL (file:////host/share, GHSA-hr87-735w-hfq3) and
an executable hidden behind a percent-encoded dot (evil%2Ebat). Document
that the decode tests run on Linux and therefore don't exercise the
Windows drive-letter/backslash conversion, and clarify why isLocalFileUrl
is intentionally broader than the renderer/scheme-allowlist checks.
* fix(electron): block executable launch via shell.openPath
window.ea.openPath is reachable by any plugin, same-origin iframe plugin, or
renderer XSS, and its guard only blocked UNC/remote paths (NTLM leak,
GHSA-hr87). shell.openPath hands a local file to the OS handler, and on
Windows ShellExecute runs .bat/.cmd/.vbs/.hta/... from a plain file with no
exec bit. Chained with a writable-dir drop (e.g. FILE_SYNC_SAVE into the
local-file sync folder) or a malicious synced FILE attachment clicked by the
user, that is renderer -> native code execution bypassing the nodeExecution
consent gate.
Add hasExecutableFileExtension() (normalizes Windows trailing dots/spaces and
NTFS alternate data streams) and refuse those paths at the openPath sink.
Applied at the sink, not in the shared isPathSafeToOpen, so link/image
rendering (where a remote https://.../x.exe is legitimate) is unaffected.
* fix(electron): close # filename bypass in openPath executable gate
The executable-extension check split every path on `?`/`#` to drop a URL
query/fragment, but `#` is a legal Windows/NTFS filename char (and both `#`
and `?` are legal on POSIX). For a bare filesystem path that split truncated
the real filename: `C:\sync\evil.txt#.bat` was read as `.txt` and allowed,
yet ShellExecute runs it as `.bat` — defeating the gate via the same
drop-a-file-then-openPath vector the fix targets. Gate the query/fragment
split behind an actual `file:` scheme so real filenames keep their extension.
Also add well-known ShellExecute/launcher vectors the curated denylist
missed: settingcontent-ms, appref-ms, library-ms, wsc, chm, hlp, diagcab,
msix/appx family (Windows) and pkg, terminal, fileloc, inetloc (macOS).
Adds regression tests for both.
* fix(electron): require file:// (not bare file:) before query/fragment split
The executable-extension gate stripped a URL query/fragment for any string
starting with `file:`. On POSIX `#`, `?` and `:` are all legal filename
chars, so a file whose name merely starts with the literal `file:` (e.g.
`file:notes.txt#.sh`) was mis-parsed as a URL and its real `.sh`/`.desktop`
extension hidden. Require the `//` of an actual `file://` URL so only genuine
URLs get the query/fragment split; bare paths keep their real extension.
* fix(electron): fail-safe exec confirmation dialog (GHSA-256q)
The EXEC confirmation was the only gate before an arbitrary shell command
runs with the user's privileges, but it did not fail safe:
- defaultId: 2 was out of range for a two-button dialog, leaving the
focused default per-platform-undefined, so an accidental Enter could
execute. Cancel is now defaultId + cancelId (Enter/Escape never runs).
- 'Remember my answer' defaulted to checked, so one careless click could
whitelist a command to the silent allow-list forever. It is now opt-in.
Add electron/ipc-handlers/exec.test.cjs as a regression guard.
* docs(plugins): correct misleading plugin sandboxing claims
Docs claimed JS plugins run in 'isolated VM contexts' and iframes run
'without allow-same-origin' — both are false. JS plugins run in the host
renderer via new Function, and iframes use allow-same-origin (required for
#8467), so both can reach the privileged window.ea bridge. Align the docs
with the code (plugin-iframe.util.ts) and stress the trust model.
* fix(electron): fail closed on corrupt exec allow-list, cover error paths
The EXEC handler is wired to ipcMain.on (fire-and-forget), so a throw on a
corrupt allow-list surfaced as an unhandled promise rejection with no user
feedback. Wrap the handler body in try/catch and route failures through
errorHandlerWithFrontendInform (the same channel exec errors already use),
failing closed so a corrupt store never falls through to executing.
Expand the regression tests: assert the corrupt-config path informs the error
and runs nothing, cover exec-error routing, and guard allow-list append (an
overwrite regression that wipes remembered commands previously passed green).
* docs(plugins): document window.ea.exec shell path in trust model
The trust-model section implied executeNodeScript() was the only process
path; on desktop window.ea.exec also runs arbitrary shell commands via
child_process.exec behind a separate, weaker gate (confirmation dialog +
persistent allow-list, not the nodeExecution consent).
* test(electron): run exec security regression tests in CI
The test:electron runner globs electron/*.test.cjs (non-recursive), so the
guard at electron/ipc-handlers/exec.test.cjs was never executed by CI —
verified: the suite went 160 -> 168 tests once discovered. Move it to
electron/exec.test.cjs (matching every sibling electron test) and point
execModulePath at ipc-handlers/exec.ts.
* refactor(electron): narrow exec allow-list without an unsafe cast
Drop the `as string[]` cast that asserted away the very corruption the
Array.isArray guard is meant to catch; narrow the unknown value honestly so
the guard is a real type-check. Behavior is unchanged (falsy -> empty list,
truthy non-array -> fail closed).
* fix(electron): stop logging exec command content
Per CLAUDE.md rule 9 (log history is exportable, never log user content), a
command can carry a secret in its arguments; log a content-free line instead.
Also fold the duplicated exec-spawn into a single runCommand() helper so the
allow-listed and just-confirmed paths share one audited call site.
* fix(electron): remove exec IPC to close GHSA-256q at the root
window.ea.exec exposed arbitrary shell (child_process.exec) to the whole
renderer: JS plugins (new Function in the host realm), same-origin iframe
plugins (window.parent.ea), and any renderer XSS — bypassing the per-plugin
nodeExecution consent gate entirely. Its only consumer, COMMAND task
attachments, is dormant: no UI creates them (the edit dialog offers only
LINK/IMG/FILE).
Rather than guard a dormant RCE primitive, remove it: delete the EXEC IPC
handler + preload.exec + the ElectronAPI method + the directive's COMMAND
branch. This closes the vector for plugins, iframes, AND host-realm XSS at
once (a bootstrap handoff would only hide it from plugins), and supersedes
the earlier dialog hardening (that primitive no longer exists).
Keep the 'COMMAND' literal in the synced TaskAttachment type (removing a
synced union member breaks typia validation on peers/legacy data); a click
on a legacy COMMAND attachment now shows an informational snack instead of
executing.
* docs(plugins): reflect exec IPC removal in the trust model
window.ea.exec no longer exists (removed to close GHSA-256q); the docs now
state executeNodeScript is the only sanctioned native-code path.
* test(electron): guard exec IPC stays removed (GHSA-256q)
The interim exec security tests were deleted together with the executor, so the
shipped fix had no regression coverage. Add electron/exec.test.cjs (picked up by
the electron/*.test.cjs CI glob) asserting the bare exec primitive stays gone:
no IPC.EXEC event, no exec.ts handler, no preload exec bridge, no ElectronAPI.exec
method, no initExecIpc wiring. Targets only the removed surface, not the
sanctioned PLUGIN_EXEC_NODE_SCRIPT/executeScript nodeExecution path.
* chore(electron): mark ALLOWED_COMMANDS store key as legacy
Its only reader/writer was the deleted exec handler (GHSA-256q). Document that it
is retained purely so older persisted stores keep loading.
* feat(plainspace): add Collaborate action to project context menu
Surface Plainspace sharing from the project context menu (active, non-Inbox,
not-yet-shared projects) so it can be discovered at the moment of collaboration
intent, reusing PlainspaceShareService. A new selectIsProjectSharedOnPlainspace
selector hides the action once a project is shared to avoid provisioning a
duplicate space on a repeat click.
* docs(plainspace): document collaboration in wiki
Add Plainspace to the issue-integration comparison (matrix + per-provider
section) and a how-to for sharing a project via the project menu, including the
network/privacy caveat.
* feat(plainspace): place Collaborate action below the share-list item
Group the two share/export actions and lift Collaborate higher for
discoverability. Gated to active, non-inbox, not-already-shared projects.
* fix(plainspace): smoother first-run connect and value-first dialog
Pre-check connectivity and revalidate a stored token before the space
picker, so a stale/foreign token routes to the connect dialog and an
offline state shows a calm message instead of the raw 'check your token'
picker error. Make the connect dialog value-first: lead with what you
get, drop the 4-step how-to and email hint in favor of one short pointer
(token-creation guidance moves to plainspace.org).
* docs(plans): dedicated from-Super-Productivity flow on plainspace.org
Open plan for a guided token/connect flow on plainspace.org when a user
arrives from SP (Model A manual token, Model B OAuth-style handoff).
* refactor(plainspace): drop redundant connect pre-check (multi-review)
The space picker already detects a stale token and offers a reconnect
(#8616), so the revalidate() pre-check duplicated that path, cost an
extra GET /me, and forced re-auth on a valid token during a transient
server blip. Keep only the one-line offline guard; drop revalidate(),
the discriminated union, and the unnecessary _isOnline() seam (navigator
.onLine is spyable in the runner). Fix stale doc comments and document
the disabled-provider trade-off in the selector.
* feat(plainspace): show brand icon in the connect dialog title
* feat(plainspace): deep-link connect dialog to the from-SP onboarding flow
Point the dialog's 'Open Plainspace' link at the dedicated
/connect/super-productivity entrypoint (which guides token creation)
instead of the bare marketing host, closing the connect-flow funnel leak.
* feat(plainspace): bounce back to the app after connecting (desktop)
Append a validated `?return=superproductivity://plainspace-connect` deep
link to the connect URL, gated on IS_ELECTRON (only desktop registers the
scheme — web/mobile would get dead buttons). Handle that action in the
Electron protocol handler by surfacing the window, so the connect page's
"Open Super Productivity" button re-focuses the app.
Desktop-only; needs an on-device check.
* feat(electron): trigger global shortcuts via superproductivity:// URLs
On Wayland the compositor owns global hotkeys, so Electron's globalShortcut
often does not register. Add three actions — toggle-visibility, new-note and
new-task — to the existing protocol handler so a compositor keybind can call
`xdg-open superproductivity://<action>`. No extra CLI tool or runtime is
needed: xdg-open (Linux), open (macOS) and start (Windows) already ship with
the OS, and the running instance receives the URL via the existing
single-instance / second-instance path.
Extract the show/hide logic into a shared toggleWindowVisibility() used by both
the globalShowHide shortcut and the new protocol action, and add a key-repeat
debounce so one held key press no longer hides then immediately re-shows the
window.
Docs: add a Wayland keybind recipe (Niri/sway/Hyprland) to the keyboard
shortcuts wiki page.
Refs #7114
* fix(electron): correct toggle-visibility focus race and debounce
On Linux/Windows the second-instance handler pre-focused the window before processProtocolUrl ran, so toggle-visibility always read 'visible' and hid the window the user asked to show. Skip that pre-focus for toggle-visibility only (new getProtocolAction helper); every other action keeps the bring-to-front behavior.
Make the key-repeat debounce direction-agnostic with a sliding quiet-gap (1000->750ms): it now guards both show and hide, settles a held key on a single toggle, and fires on the xdg-open path where the old isHidden-only guard was always false.
Stop logging the create-task title and URL path to the exportable log (CLAUDE.md rule 9 / privacy).
Add coverage for the real second-instance path, held-key-from-hidden, gap expiry, the #7282 minimize fallback, the macOS hide path, unfocused-show, and the log redaction; document AppImage/Flatpak/Snap scheme registration. Refs #7114.
* fix(electron): show window on cold-start toggle-visibility launch
Cold start: when superproductivity://toggle-visibility launches the app (it wasn't running), the freshly-shown window was immediately hidden again because the toggle saw it focused. Flag the cold-start URL during the argv scan and SHOW — never toggle — the window once it's ready (via processPendingProtocolUrls), respecting start-minimized-to-tray. The already-running second-instance path keeps real toggle behavior.
Rename the two interactive protocol actions new-task/new-note to add-task/add-note: aligns with the app's Add-Task vocabulary and the globalAddTask/globalAddNote keys, and avoids colliding with the programmatic create-task/<title>. The action names are a frozen public contract once users bind them in compositor configs, so this is the pre-merge moment to settle the naming. Refs #7114.
* feat(plugins): enable OAuth-based issue-provider plugins
Generic, provider-agnostic plugin-framework hooks so an issue-provider plugin
that needs an exact OAuth redirect can work without any built-in code:
- OAuthFlowConfig.redirectUri: a plugin may declare an exact pre-registered
callback; the host uses it for both the authorize request and token exchange.
- The Electron loopback honors a plugin-requested fixed port and rejects with a
clear message when that port is already in use.
- Apply user-supplied clientId/clientSecret/redirectUri overrides onto a
plugin's oauthConfig (bring-your-own OAuth app).
- Fix: merging a partial pluginConfig update no longer drops omitted keys and
deep-merges nested objects (e.g. twoWaySync).
Split out of the Basecamp community-plugin work per PR #8507 feedback; contains
no provider-specific code.
* fix(plugins): address #8546 review
- validate OAuthFlowConfig.redirectUri per platform (loopback / same-origin /
app scheme) and fail fast instead of hanging; restrict desktop loopback to 127.0.0.1
- warn when a client secret is dropped on web/native (bring-your-own credentials)
- validate the IPC loopback port to [1024,65535], register the error handler before
listen(), and close the failed server
- merge pluginConfig via generic recursion instead of a hardcoded twoWaySync case
- nits: named token-store imports; fix stale prepareRedirectUri comment
- tests: redirectUri validation, the web client-secret warning, and generic merge
* fix(plugins): address #8546 round-2 review
- loopback error handler calls cleanupServer() so a post-listen runtime error
doesn't leave the server ref / 5-min timer dangling
- share OAUTH_LOOPBACK_PORT_{MIN,MAX} between the renderer and Electron main so
the bounds never drift; reject out-of-range (incl. 0/80/443) redirect ports early
- drop _getElectronLoopbackPort and parse the already-validated redirectUri once
- document that a bring-your-own clientSecret syncs via pluginConfig (override boundary)
- skip __proto__/constructor/prototype keys in the pluginConfig merge (defense-in-depth)
- test: prototype-pollution guard
* fix(plugins): address #8546 round-3 review
- reject native redirectUri overrides outright (closes CodeQL
js/incomplete-url-scheme-check) via a pure, per-platform validateOAuthRedirectUri
util (electron loopback / native reject / web same-origin)
- gate bring-your-own OAuth credentials to the desktop loopback flow and warn
(instead of silently dropping clientId) when set on web/native
- namespace BYO under pluginConfig.oauthOverrides (was flat keys); document the
convention on OAuthFlowConfig (public plugin API)
- shallow top-level pluginConfig merge: drop the deep recursion + proto guard;
nested objects (e.g. twoWaySync) are replaced wholesale, matching callers
- pin the web redirect to /assets/oauth-callback.html via a shared constant so a
same-origin wrong-path URI fails fast; note the desktop 127.0.0.1-only rule
- companion tests for each
* fix(plugins): strip desktop redirectUri on web/native OAuth flows
A plugin-declared redirectUri is the desktop loopback override; keeping it on
the web/native branches made a web/native-capable plugin throw at connect time
(the loopback URI fails web/native redirectUri validation). Strip it on those
branches so prepareRedirectUri falls through to the platform default. Document
redirectUri as desktop-only in the plugin API and fix a misleading test name.
* refactor(plugins): extract resolveEffectiveOAuthConfig and harden native fallthrough
Move the platform client/secret/redirectUri selection out of the bridge into a
pure, parameterized util so every branch is unit-testable (the IS_* platform
consts are module-level and cannot be mocked in karma). Also strip clientSecret
and redirectUri on the native fall-through — a native platform where the plugin
ships no matching client id — keeping both strictly desktop-only.
Optional hardening on top of the redirectUri fix; safe to drop independently.
* feat(plugins): persist nodeExecution consent per plugin (#8512)
Phase 2 of #8512: remember an uploaded plugin's nodeExecution consent so
it is asked once, not every app session, while keeping the trust decision
local to the device.
- Main-owned, local-only store (electron/plugin-node-consent-store.ts,
wrapping simple-store under key 'pluginNodeExecutionConsent'); never
pfapi-synced, so a grant on one device never auto-grants on another.
- Ask-once is scoped to UPLOADED plugins; built-in plugins (sync-md) keep
the per-session verified prompt unchanged (regression-safe).
- Consent is written only after a native Allow in main; the renderer has a
delete-only clearConsent IPC (fail-safe) and no way to self-grant.
- Cleared on disable / uninstall / re-upload (never in generic teardown),
so revoke = the existing disable toggle and changed code re-consents.
- No code hash: re-ask-on-change is structural (re-upload clears consent);
a renderer hash would be forgeable and security-worthless. version:1 is
the migration anchor if main-owned hashing is ever added.
Tests: electron 154 pass (consent-store + executor ask-once/deny/clear/
built-in-never-persisted); 474 plugin specs pass incl. the sync-exclusion
guard. Docs updated.
* fix(plugins): harden persisted consent against prototype-pollution ids
Multi-review (security) found a CRITICAL in the Phase 2 consent store: the
consents map was a plain object keyed on an attacker-controlled pluginId, so
an uploaded plugin with id 'constructor' / 'toString' / 'valueOf' /
'hasOwnProperty' resolved consents[id] to the inherited Object.prototype
member (a truthy function). The executor's ask-once check treated that as a
prior grant and minted a nodeExecution token with NO consent dialog on a fresh
install — full code execution with zero user approval. ('__proto__' was already
blocked by the id allowlist; these names pass it.) Unit tests missed it because
the executor test stub used a Map, which is immune to the footgun.
Fix:
- Store: null-prototype consents map (Object.create(null)) + own-property
(hasOwnProperty) guarded reads + reject non-object entries. Closes the class
for any prototype-member id, including across a disk round-trip.
- Executor: reject __proto__/prototype/constructor in assertSafePluginId as
defense-in-depth at the boundary.
- Regression tests: consent store returns null for prototype-member ids (fresh,
after a real set, after clear); grant request for these ids is rejected with
no dialog and no mint.
Also from review: ask-once path now re-checks the sender URL after the consent
read (parity with the dialog path); clarified why the consent mutation queue is
not redundant with simple-store's save queue.
Electron suite 156/156 pass.
* refactor(plugins): log consent persist-failure via electron-log
Multi-review follow-ups (non-blocking):
- Route the best-effort consent persist-failure to electron-log/main (the
user-exportable host log) instead of console; console in the executor is
otherwise the sandboxed plugin's own output. Only the validated id is logged.
- Clarify the disable-path comment: clearing revokes the live session grant
always, and the persisted consent only for uploaded plugins (built-ins have
none).
* fix(plugins): clear persisted consent on cache-clear and disclose persistence in dialog
Two gaps found in multi-agent review of the Phase 2 persisted-consent feature:
- clearUploadedPluginsFromMemory() (the 'Clear plugin cache' button) wiped the
plugin code from IndexedDB but left the main-owned persisted nodeExecution
consent behind. A later re-upload of the same id has no existingState, so the
re-upload consent-clear in loadPluginFromZip never fired and the (possibly
different) code was silently granted node execution with no prompt — defeating
the 'replacing code under an id always re-asks' invariant. Now clears consent
for every evicted uploaded id, mirroring removeUploadedPlugin.
- The uploaded-plugin native consent dialog still said access was valid 'for this
app session', but Allow is now persisted across sessions. The prompt now
discloses that the choice is remembered on the device until disable / remove /
re-upload, so the user consents to the actual scope.
Regression tests added on both sides.
* refactor(plugins): key persisted consent on a Map, not a null-prototype object
Multi-review simplification. The consent store keyed an attacker-controlled
pluginId into a plain object, defended against `Object.prototype` member names
(constructor/toString/…) with a null-prototype object + hasOwn guards + a
typeof-object read check. A `Map` makes that safety structural and self-evident —
an unstored key is simply `undefined` — and matches the sibling `grants` Map in
the executor. The on-disk format is unchanged (a plain {version, consents} object);
the Map is serialized via Object.fromEntries (define-semantics, no prototype write)
and rebuilt via Object.entries with the well-formedness guard moved to load time.
Also corrects the stale 'never downgrade-corrupt it' comment with the accurate
downgrade behavior, and adds a round-trip test proving a hand-edited on-disk
__proto__ data key loads inertly without polluting Object.prototype.
* refactor(plugins): funnel disable through PluginService.disablePlugin and de-dup dialog display
Two more multi-review items, now that we own the PR:
- 'Disabling a node plugin revokes its consent' previously lived only in the
plugin-management UI handler, so a future programmatic disable path could unload
the plugin yet leave persisted consent behind — re-enabling would then silently
re-grant node execution. Added PluginService.disablePlugin(setEnabled=false +
unload + clearNodeExecutionConsent) and routed the UI through it, making the
revoke a structural invariant. The consent clear is a safe no-op for non-node
plugins, so the previous requiresNodeExecution gate is dropped.
- The uploaded-plugin name/version were sanitized once for the dialog and again for
persistence (same lengths/fallbacks, duplicated). Extracted sanitizedUploadedDisplay
as the single source of truth so the persisted record always matches what the user
saw in the prompt.
Tests added for the disablePlugin invariant.
* fix(plugins): harden persisted nodeExecution consent (multi-review)
Follow-ups from a multi-agent review of #8600:
- Re-ask structurally on every upload: clear consent unconditionally in
loadPluginFromZip (outside the `existingState` branch) so a same-id
re-upload always re-prompts even if consent was orphaned (crash
mid-uninstall, IndexedDB eviction, external/partial wipe).
- Fail closed on upload: clearNodeExecutionConsent reports a persist failure
via its return value; loadPluginFromZip aborts the upload if the prior
consent could not be revoked, so replacement code can't inherit a stale
grant. Lifecycle edges (disable/uninstall/cache-clear) ignore the result so
a rare disk failure can't abort their bookkeeping.
- Mint the grant before the best-effort consent persist in the executor so a
navigation/destroy during the write drops it via cleanup and a persist
failure can't lose an approved grant.
- Validate the full consent record shape on load so a corrupt {}/array entry
can't read as a grant.
- Log only the validated id + error code on persist failure (no userData path).
- Fix a stale comment (the store keys consent in a Map, not null-proto objects).
Adds regression tests: mint-before-persist ordering, best-effort persist,
malformed-entry rejection, and the consent-clear fail-closed return contract.
* test(plugins): add clearNodeExecutionConsent to PluginBridgeService spy
loadPluginFromZip now clears prior persisted nodeExecution consent
before loading replacement code (#8512 Phase 2). The spy in this spec
lacked the method, so the call threw, was caught, returned false, and
aborted the upload, failing both load-from-zip tests.
* feat(plugins): allow uploaded nodeExecution behind consent gate
Re-open the nodeExecution permission for uploaded/community plugins
(previously built-in only, #8205) behind the existing main-process
consent dialog. Phase 1 of #8512; unblocks the Super Productivity MCP
plugin (discussion #8385).
- Main process sanitizes the attacker-controlled plugin id and the
self-declared name/version before they reach the consent dialog or
the grant map (control/bidi/whitespace rejected, length-capped).
- Bundled vs uploaded is decided by the on-disk manifest, never a
renderer-supplied flag, so uploaded code can't borrow a built-in
plugin's verified name.
- Uploaded-plugin dialog anchors on the validated id, flags the plugin
as unverified third-party with full machine access / no sandbox, and
defaults to Deny.
- Revoke is main-authoritative by (pluginId, webContents) so a re-upload
reusing an id can't inherit a live session grant.
- Consent stays session-scoped: an in-memory, never-synced denied set
prevents re-prompt storms; deny keeps the plugin enabled but fails
node calls closed until re-enable or restart.
Refs #8512#8385
* fix(plugins): reject path-segment ids in nodeExecution consent gate
Multi-agent review of the Phase 1 gate found the uploaded plugin id —
used as a path component in the bundled-manifest existsSync probe — was
not rejecting path separators or dot-segments. Impact was bounded (the
verified-builtin branch re-validates with the strict kebab regex before
any read, so no code exec / file read / dialog spoof), but `..` / `/`
left a filesystem-existence oracle and rendered misleadingly as the
dialog "Plugin ID". assertSafePluginId now rejects `/`, `\`, `.`, `..`.
Also from review: factor the shared Allow/Deny dialog shell, and correct
the denied-cache comment (the existing token short-circuit handles the
multi-call-site case; the cache only makes a denial sticky across a later
non-interactive grant re-entry). Documents the uploaded-id constraints.
Refs #8512
* fix(plugins): harden uploaded nodeExecution consent gate (review follow-ups)
Addresses multi-agent review findings on the uploaded-plugin nodeExecution
consent gate:
- id validation: use an allowlist (/^[A-Za-z0-9][A-Za-z0-9._-]*$/) instead of a
Unicode denylist, closing bidi/zero-width/homoglyph dialog-anchor spoofing the
range list missed (U+061C, U+2060, U+3164, fullwidth chars); strip all Unicode
control+format chars from the self-declared display name/version.
- never upgrade trust: describeVerifiedBuiltInDialog returns null on any imperfect
on-disk verification (id mismatch, missing permission, unreadable manifest) and
the grant handler falls back to the unverified dialog, so a colliding uploaded id
can never borrow a built-in's verified dialog.
- reserve the gitea/linear/trello/azure-devops issue-provider bundled ids (they had
drifted out of BUNDLED_PLUGIN_IDS, leaving an impersonation gap) and guard the
BUNDLED_PLUGIN_PATHS subset-of BUNDLED_PLUGIN_IDS invariant with a node test.
- key the revoke and exec IPC handlers through the same assertSafePluginId as the
grant handler so the "revoke by id on teardown/re-upload" guarantee can't drift.
- clear the session nodeExecution denial when a plugin is uninstalled, so a fresh
re-upload of the same id is prompted again rather than silently failing closed.
- de-duplicate the PluginNodeExecutionElectronApi interface into a single
electron/shared-with-frontend model (was copied byte-identically in two files).
* fix(op-log): lock snapshot save to prevent lost-update window
saveCurrentStateAsSnapshot() read NgRx state then lastSeq without
holding OPERATION_LOG lock. An op appended between the two reads would
get seq <= lastAppliedOpSeq but its effect would be absent from the
snapshot. On next hydration the tail replay would start after that seq,
silently skipping the op forever.
Fix: wrap in lockService.request(LOCK_NAMES.OPERATION_LOG, ...) and
read lastSeq BEFORE state snapshot so the worst interleaving degrades
to harmless re-replay (idempotent) rather than a missed op.
Fixes#8308
* fix(op-log): address review feedback on snapshot lock PR
- Amend JSDoc idempotency claim: syncTimeSpent is additive on re-replay
- Add inline note about compaction's opposite read order and worse failure mode
- Add lock regression tests (#8308): lock acquired, read order, error handling
Co-Authored-By: Claude <noreply@anthropic.com>
* refactor(sync): remove unused error classes, dialog, and constructor-time logging
Phase 1 of #8325: clean up orphaned sync error types and their UI.
Removed error classes that are no longer thrown anywhere:
- NoEtagAPIError, FileExistsAPIError (unused API errors)
- RevMismatchForModelError, SyncInvalidTimeValuesError (superseded by file-based flow)
- RevMapModelMismatchErrorOnDownload/Upload, NoRemoteModelFile, NoRemoteMetaFile
- LockPresentError, LockFromLocalClientPresentError, MetaNotReadyError, InvalidRevMapError
Removed DialogSyncErrorComponent and all references in SyncWrapperService
(_forceDownload, _handleIncoherentTimestampsDialog, _handleIncompleteSyncDialog,
_openSyncErrorDialog, _extractModelIdFromError).
Removed constructor-time logging from JsonParseError, ModelValidationError,
DataValidationFailedError — these errors are logged at the catch site; redundant
construction-time logs risk leaking user data.
Cleaned up dead translation keys (D_INCOMPLETE_SYNC block, DIALOG_RESULT_ERROR,
ERROR_DATA_IS_CURRENTLY_WRITTEN) from en.json and t.const.ts.
Updated file-based-sync-flowchart.md to reflect the removed error types.
---------
Co-authored-by: Claude <noreply@anthropic.com>
Linux StatusNotifierItem hosts redraw the whole tray item on every
Tray.setImage(), so cycling the 16 progress-animation frames made the
icon flicker between a full and a partial ring. Show a single static
running icon on Linux; Windows/macOS keep the animation.
Also re-render the static stopped/running icons (light+dark, @2x) from
vector sources at 24/48px so GNOME no longer upscales a 16px source.
#7097 force-disabled the custom title bar on all GNOME and hid the
toggle, even though only GNOME+Wayland actually fails to render the
Window-Controls-Overlay. This stranded GNOME-X11 users (where the
feature worked) with no way to re-enable it.
- Narrow the kill-switch from IS_GNOME_DESKTOP to a new IS_GNOME_WAYLAND
(XDG_SESSION_TYPE/WAYLAND_DISPLAY, mirroring idle-time-handler). GNOME
on X11 keeps the feature and the settings toggle.
- Unify GNOME detection: preload imports it from common.const instead of
re-implementing, so main and renderer can't drift.
- Align global-theme.service with main-window: force native decorations
on GNOME+Wayland in both, so a persisted/synced `true` no longer lays
the custom header over native controls (doubled header).
- Add unit tests for the detection matrix.
A Windows tray-icon GUID binds to the executable's full path unless the
binary is code-signed with an organization in the signature subject. The
Store (MSIX) and portable/scoop builds run from versioned directories
whose path changes on every update, so the GUID goes stale and
Shell_NotifyIcon(NIM_ADD) fails silently (Electron raises no JS error).
new Tray(image, guid) still returns a live Tray, so the try/catch and
ensureIndicator() truthiness guards never detected the failure — the
window hid to an invisible, unreachable tray.
Use the path-bound GUID only for the stable-path NSIS build; create the
tray without a GUID on Store and portable, so Windows identifies the icon
by window handle and it stays visible across updates. The NSIS path is
untouched, preserving its taskbar-pin persistence.
Refs: https://www.electronjs.org/docs/latest/api/tray (guid)
* fix(keyboard): resolve macOS global shortcut layout mismatch (#8378)
* fix(keyboard-layout): log layout-detection failure and resolve layoutReady with map copy
* fix(keyboard-shortcut): remove debug console logs and add macOS scope comment
* fix(keyboard-shortcut): preserve modifier separator when mapping plus key shortcut
* refactor(keyboard-shortcut): export mapping helpers and avoid as any cast in configuration mapping
* test(keyboard-shortcut): add unit tests for layout shortcut translation logic
* test(keyboard-shortcut): use correct KeyboardConfig type instead of as any in test fixture
* docs(keyboard-shortcut): hoist macOS physical shortcut layout comments to helper JSDoc
* refactor(config): extract global shortcut keys and mapping helpers
* test: add test helper capability for IS_ELECTRON and IS_MAC
* test: add comprehensive integration unit tests for global shortcut effects
* feat(electron): eagerly trigger layout detection on Electron startup for macOS timing fix
* refactor(config): InjectionToken migration, layout detection hardening, and startup optimization
* refactor: address non-blocking suggestions for layout detection and DI tokens
* build(electron): move keyboard-config.model to shared-with-frontend to fix ASAR require
* fix(client-id): increase entropy to 6 chars and fix related test regressions
The GHSA-hr87 scheme allowlist (#8210, v18.10.0) permitted only
http/https/mailto/file, which silently broke custom app deep-links such
as obsidian:// in task-attachment links and markdown notes.
Add low-risk app-launch + standard schemes (obsidian, vscode,
vscode-insiders, zotero, logseq, tel, sms) to the shared allowlist. This
restores the links across all sinks (OPEN_EXTERNAL, OPEN_PATH, the
navigation fallback, and the markdown link renderer) while still blocking
the OS-level handlers (ms-msdt:, search-ms:) the allowlist exists to stop.
If a folder pick (setSyncFolderPath) completes while the very first
getSyncFolderPath disk read is still in flight, the load's resolution
callback would overwrite the freshly-set cache with the now-stale disk
value (null), leaving sync wrongly not-ready until restart. Only seed
the cache from disk when it's still unset; the freshly-picked value wins.
Resolve conflict in electron/local-file-sync.ts by keeping the PR side:
master's assertPathOutside guard was added inside the TO_FILE_URL handler,
which this PR deletes entirely (deletion supersedes the guard). Also drop
master's two new TO_FILE_URL tests (the handler no longer exists).
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.
Phase 1 of the fix for #8228, simplified after KISS review.
The earlier draft (commit 5cb83b41a, since reverted) introduced a
multi-grant capability store with opaque grantIds and a feature
discriminator. Review showed the abstraction was overhead: this app
has exactly one user-pickable filesystem location (the sync folder).
Backgrounds will go through copy-to-cache, not folder grants;
plugins do not get folder grants; backups use a main-derived path.
And the opaque grantId added zero marginal security over a featureless
{relativePath} API since an XSS in the renderer would just replay the
id from memory. The security boundary is "main owns the canonical root
and the resolver logic", not the id.
So: drop the store, keep the resolver. resolveSyncPath takes
(syncFolderPath, relativePath, userDataDir) and returns the vetted
absolute path. Layered defenses:
- syncFolderPath canonicalized every call (folder may have moved
since startup); missing/inaccessible root denies
- canonical root must not be userData or live inside it (a user who
picks userData as their sync folder would otherwise hand the
renderer authority over settings/grants/db)
- relativePath may be '' or '.' for LIST/CHECK ops on the root;
anything else must not be absolute and must not escape
- leaf symlinks refused (v1 policy; O_NOFOLLOW alternative is a
follow-up that needs cross-platform care)
- canonical resolved path (or deepest existing ancestor when the
leaf doesn't exist yet) must still live inside the canonical
root — catches symlinks/junctions in intermediate directories
and case-fold / 8.3-shortname aliases
- ancestor walk fails CLOSED on EACCES; only ENOENT lets us walk
up. A permission-restricted ancestor must not silently allow.
Errors are opaque (PathNotAllowedError, no path in message) and the
stack is stripped — IPC handlers further sanitize before returning to
the renderer.
Phase 2 will wire this into FILE_SYNC_SAVE/LOAD/REMOVE/LIST_FILES and
CHECK_DIR_EXISTS, move syncFolderPath ownership from the renderer's
privateCfg to a main-side simple-store key, and add a re-confirm
migration on first launch.
16 unit tests cover the rejection cases from #8228's test plan plus
the EACCES-fail-closed branches that came out of the KISS review.
Lays the groundwork for issue #8228: today the FILE_SYNC_* IPCs accept
any renderer-supplied absolute path outside userData, which means any
plugin or XSS in the renderer can read/write arbitrary user files
(~/.ssh, /etc/passwd, etc.). The goal is to replace string-path
authorization with capability-based grants: the renderer holds only an
opaque grantId, the main process holds the canonical root, and FS ops
take (grantId, relativePath).
This commit adds the main-side primitives but does not wire them into
any IPC yet:
- grant-store: persisted, main-only, feature-scoped grants backed by
simple-store under a new 'fileGrants' key. Opaque hex ids from
crypto.randomBytes. Roots are canonicalized at create time, and
revalidated at startup so a folder that was moved, deleted, or
swapped for a symlink-elsewhere invalidates the grant cleanly.
- grant-resolver: resolveGrantPath(grantId, relativePath, feature)
returns the absolute path the IPC handler may touch. Enforces the
feature scope match, rejects absolute paths and '..' traversal, and
refuses symlinks. For not-yet-existing leaves it canonicalizes the
deepest existing ancestor so a directory symlink mid-path cannot
sneak a write outside the root. Errors are opaque
(PathNotAllowedError) and never embed the offending path.
Notes on tests: ts-node's transpile-only loader does not downlevel
'for...of map' iteration, so the store uses Array.from(map.entries())
where it iterates the cache. The existing electron *.test.cjs use the
same loader and the same workaround was already implicit in their
patterns.
Phase 2 (switching FILE_SYNC_SAVE/LOAD/REMOVE/LIST_FILES + CHECK_DIR_EXISTS
to grant-based, with re-confirm migration of the existing syncFolderPath)
and Phase 3 (image picker copy-to-cache) follow in separate commits.
will-navigate gated on url.includes('localhost'), so hosts like https://localhost.evil.com (or a 'localhost' substring in the query) were treated as the app's own origin and navigated the main window directly, bypassing the openUrlInBrowser scheme guard. Parse the URL and match the hostname exactly (localhost/127.0.0.1).
No regression for prod: it serves from file:// and routes via hash (withHashLocation), so will-navigate never fires for the app's own origin; only real external links reach this handler.
A sweep for auto-loading subresource sinks fed attacker-controllable
(synced) content found two more no-click NTLM vectors beyond the
markdown/attachment images already gated:
- READ_LOCAL_IMAGE_AS_DATA_URL (electron/local-file-sync.ts): a synced
theme backgroundImage 'file://host/share/x.png' reaches this main-process
handler, where fileURLToPath -> fs.stat/readFile on the resulting UNC path
opens an SMB connection and leaks the NTLM hash — no click, just the theme
being active. This is the most reliable variant (main-process fs, not a
Chromium subresource). Guard with isPathSafeToOpen before any fs access.
- note.imgUrl (note.component): a synced note image URL is bound directly
to <img [src]>/[enlargeImg] and auto-loads on render. Gate at the setter
so only a safe URL reaches the bindings.
Both reuse the existing isPathSafeToOpen helper; local file://, http(s),
data: and blob: are unaffected. New electron test proves stat/readFile are
never reached for UNC / remote file:// input.
(cherry picked from commit 80a3c20b3f5a3f535f5d04d7fd215ba569493ef8)
* fix(plugins): harden node execution grants
* fix(plugins): harden iframe bridge boundaries (#8208)
* fix(plugins): harden iframe bridge boundaries
* fix(plugins): tighten iframe bridge follow-up
* test(plugins): make node-executor electron test hermetic
The new plugin-node-executor test read the real built-in plugin manifest
(src/assets/bundled-plugins/sync-md/manifest.json), which is a build
artifact absent when 'npm run test:electron' runs in CI (before the
frontend/plugin build). Stub the manifest read, scoped to the executor
module via Module._load, so the grant/token/webContents assertions no
longer depend on built plugin assets.
* fix(plugins): allow iframe formatDate & getCurrentLanguage i18n methods
The iframe API allow-list gate added in #8208 only listed a subset of
the i18n methods that master's #8146 exposes to iframe plugins. Without
this, plugin calls to formatDate/getCurrentLanguage (and translate) are
rejected with 'Unknown API method'. Add all three so the merged gate
matches the methods createBoundMethods/createPluginApiScript expose.
* docs(plugins): document node-exec handoff bootstrap-ordering invariant
The one-shot consumePluginNodeExecutionApi() handoff is defended by
construction ordering, not structural isolation: PluginBridgeService must
consume it before any plugin 'new Function' code runs (both share window.ea
in one renderer realm). Document the invariant at the consumption site so a
future lazy-service/early-plugin-load refactor can't silently regress it.
Surfaced by the post-merge security review (latent finding; not currently
exploitable).
* fix(plugins): use static iframe sandbox attribute to avoid NG0910
Binding a security-sensitive iframe attribute (sandbox) via [attr.sandbox]
makes Angular throw RuntimeError NG0910 and tear down the iframe, crashing
the plugin view to the global error screen. This broke the plugin-iframe,
plugin-loading and plugin-lifecycle e2e tests.
Restore the static sandbox attribute (still without allow-same-origin, so
the opaque-origin isolation from #8208 is preserved) and drop the now-unused
iframeSandbox binding/import.
* 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.
* fix(electron): validate URL scheme before shell.openExternal (GHSA-hr87-735w-hfq3)
Task-note Markdown links had their href passed verbatim to shell.openExternal
on click, letting note content (sync, imported backups, issue-provider data)
silently invoke any OS-registered protocol handler — ms-msdt: (Follina),
search-ms:, \\host\share (NTLM hash capture), etc. — with one click and no
prompt. Angular's sanitizer does not help: its SAFE_URL_PATTERN blocks only
javascript:, so these schemes pass.
Add a shared scheme allowlist (http/https/mailto/file) enforced at both layers:
- marked renderer (marked-options-factory): blocked-scheme links render as
inert <span> text instead of an anchor (covers all note render paths + web).
- Electron main process, both shell.openExternal sinks: the OPEN_EXTERNAL IPC
handler (system.ts) and openUrlInBrowser (main-window.ts, the
will-navigate/window-open path used by the fullscreen/archived dialogs).
The allowlist lives in electron/shared-with-frontend so renderer and main agree
on one policy. Blocked links are styled muted/non-interactive with an
explanatory tooltip.
* fix(electron): harden openExternal/openPath against SMB & attribute-injection (GHSA-hr87-735w-hfq3)
Follow-up hardening from multi-agent review of the initial fix:
- file:// SMB bypass: `file:` was allow-listed unconditionally, so
`file://host/share` / `file:////host` reached shell.openExternal — the same
NTLM-leak vector the raw `\\host\share` check blocked. Restrict `file:` to the
canonical local form `file:///<path>`. Gated on the raw string because
Chromium (renderer) and Node (main) parse file: hosts/paths differently.
- IPC.OPEN_PATH was unguarded: a synced FILE-attachment path of `\\host\share`
leaked NTLM via shell.openPath. Reject UNC paths (new isUncPath helper).
- Attribute-injection XSS: the marked link renderer interpolated href/title
raw; marked's angle-bracket link form could inject an event handler that runs
under disableSanitizer. HTML-escape href/title.
- Remove the now-dead/misleading explicit backslash check and redundant
toLowerCase (new URL already throws on raw UNC; protocol is spec-lowercase).
Adds cross-engine-verified tests for file:// SMB forms, isUncPath, and the
href quote-injection case.
* fix(electron): block remote file:// in OPEN_PATH too (GHSA-hr87-735w-hfq3)
Re-review found the OPEN_PATH guard was incomplete: isUncPath only rejects raw
slash-prefixed paths, but shell.openPath('file://host/share') resolves to
\\host\share on Windows — the same NTLM leak. A synced FILE-attachment path is
attacker-controllable.
Centralize the path policy in isPathSafeToOpen() (rejects UNC paths and remote
file:// URLs, allows local paths and file:///) so the two sinks can't drift.
* fix(electron): gate image src against remote file:// / UNC (GHSA-hr87-735w-hfq3)
Markdown image src auto-loads on render (no click), so a remote
file://host/share or UNC src silently triggers an SMB connection and
leaks the user's NTLM hash just by viewing a synced/imported note — the
same vector the link/openExternal gate already blocks, but with no user
interaction. CSP (img-src ... file:) does not block it.
Gate image href through isPathSafeToOpen: remote file:// and UNC srcs
render as inert (HTML-escaped) alt text instead of an <img>; local
file://, http(s), data: and blob: images are unaffected.
Also drop a dead eslint-disable directive on ALLOWED_EXTERNAL_URL_SCHEMES
(the name does not trip naming-convention).
* fix(tasks): gate IMG attachment src against remote file:// / UNC (GHSA-hr87-735w-hfq3)
An IMG-type task attachment renders <img [src]="resolvedOriginalPath">,
which auto-loads on render with no click. A synced/imported remote
file://host or UNC path there silently triggers an SMB fetch and leaks
the user's NTLM hash just by opening the task — the same no-click vector
the markdown-image gate already blocks, but via Angular template binding
(Angular's URL sanitizer does not help; SAFE_URL_PATTERN allows file:).
Drop unsafe srcs via isPathSafeToOpen so they never reach the binding;
local file://, http(s), data: and blob: images are unaffected.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
* feat(electron): add global shortcut for task widget toggle
Adds a configurable global shortcut (`globalToggleTaskWidget`) that
shows/hides the task widget without changing the persisted
enabled/disabled preference. The shortcut is a no-op while the task
widget feature is disabled.
When the user reveals the widget via the shortcut while the main window
is visible, a sticky "user-forced visible" flag keeps it up (like
always-show) instead of letting the next focus/show event immediately
hide it. The flag clears when the user toggles the widget off, opens the
app from the widget, or the feature is disabled.
Rebased onto master to drop the now-superseded tray-indicator refactor;
the only main-window.ts change is the additive user-forced-visible gate.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(electron): handle task widget shortcut races
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(e2e): retry supersync time-estimate dialog until input binds
The fill('10m') could fire its `input` event before the duration input's
value-accessor (an `input` HostListener) was wired, so the typed value
never reached the model and submit() saved an empty time — the panel
stayed "-/-" and toContainText('10m') timed out (scheduled run
27197862391, SuperSync 1/6). Retry the whole open→fill→submit cycle so a
fill that didn't stick is re-applied once the control binds.
* fix(electron): restrict loadBackupData IPC to the backup directory
The BACKUP_LOAD_DATA handler passed the renderer-supplied path straight
to readFileSync with no validation. Because plugin background scripts run
via `new Function` in the renderer, any installed plugin (or an XSS
payload in task content) could read arbitrary files through
window.ea.loadBackupData('/etc/shadow').
Constrain the path to BACKUP_DIR / BACKUP_DIR_WINSTORE via a new
isPathInsideDir guard. It uses path.relative (not a startsWith string
compare) so `..` traversal is collapsed and a name-prefixed sibling
directory (backups vs backups-evil) is not mistaken for a child. The only
legitimate caller already passes paths built from BACKUP_DIR, and
getBackupPath() is display-only, so backup restore is unaffected.
Adds electron/file-path-guard.test.cjs (node --test) covering traversal
escape, prefix-sibling, absolute-outside, and normalize-stays-inside.
Refs GHSA-x937-wf3j-88q3
* docs(electron): note lexical-only containment in file-path-guard
Clarify that isPathInsideDir does string-based containment (no fs.realpath),
so a symlink planted inside the dir is out of scope — it requires local
filesystem write access, outside this guard's renderer-input threat model.
Surfaced during multi-agent review of the GHSA-x937-wf3j-88q3 fix.
* test(electron): load guard via computed path for asar require check
The electron-smoke job runs tools/verify-electron-requires.js over the
packaged app.asar, which flags literal relative require() calls that can't
resolve in the package. The new guard test had require('./file-path-guard.ts'),
but .ts source is excluded from app.asar, so the check failed. Load the module
via a computed path (require(path.resolve(__dirname, ...))) — the same pattern
the other electron *.test.cjs files use; the scanner skips computed requires.
Also reworded the comment, which had reintroduced the literal pattern (the
scanner matches raw text, comments included).
