2 commits

Author	SHA1	Message	Date
Johannes Millan	7182ff4fba	feat(plugins): persist nodeExecution consent per plugin (#8512) (#8600) ... * 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.	2026-06-29 16:12:00 +02:00
Johannes Millan	a67323b2b5	feat(plugins): allow uploaded nodeExecution behind consent gate (#8576) ... * 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).	2026-06-25 16:45:54 +02:00