mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-17 16:37:43 +00:00
* 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.
144 lines
6.5 KiB
TypeScript
144 lines
6.5 KiB
TypeScript
import { loadSimpleStoreAll, saveSimpleStore } from './simple-store';
|
|
import { SimpleStoreKey } from './shared-with-frontend/simple-store.const';
|
|
|
|
/**
|
|
* Persisted, per-plugin nodeExecution consent (issue #8512 Phase 2).
|
|
*
|
|
* SECURITY / TRUST MODEL
|
|
* - This store lives in the main-owned `simpleSettings` file under the OS userData
|
|
* dir. It is NEVER part of any pfapi-synced model, so a granted consent on one
|
|
* device does not auto-grant on another (a node call there triggers a fresh native
|
|
* prompt). There is no renderer IPC that can *write* a consent entry — only the
|
|
* native Allow dialog in `plugin-node-executor.ts` calls `setNodeExecutionConsent`.
|
|
* The renderer can only ask to *clear* consent (fail-safe: clearing forces a
|
|
* re-prompt, never an auto-grant).
|
|
* - Consent is keyed on `pluginId` only. The "re-ask when the plugin's code changes"
|
|
* property is achieved structurally, not by a stored code hash: the only legitimate
|
|
* way an uploaded plugin's code changes is a re-upload, and the renderer clears this
|
|
* consent on disable, uninstall, and re-upload. A renderer-computed hash would be a
|
|
* forgeable TOCTOU tripwire with no security value (a granted plugin already has full
|
|
* machine access via `executeScript`), so it is deliberately omitted. The top-level
|
|
* `version` field below is the migration anchor if a main-owned hash is ever added.
|
|
* - `name`/`version` are the self-declared display strings shown at grant time. They
|
|
* are stored for diagnostics/UX only and are NEVER used for authorization — the
|
|
* non-spoofable trust anchor is the `pluginId`.
|
|
*/
|
|
|
|
export const NODE_EXECUTION_CONSENT_STORE_VERSION = 1 as const;
|
|
|
|
export interface PersistedNodeExecutionConsent {
|
|
/** Self-declared display name at grant time (unverified for uploaded plugins). */
|
|
name: string;
|
|
/** Self-declared version at grant time (unverified for uploaded plugins). */
|
|
version: string;
|
|
/** ms epoch when the user allowed it. */
|
|
grantedAt: number;
|
|
}
|
|
|
|
interface NodeExecutionConsentBlob {
|
|
version: number;
|
|
// SECURITY: keyed on an attacker-controlled pluginId. A `Map` (not a plain object) is
|
|
// used so an id that names an `Object.prototype` member (`constructor`, `toString`,
|
|
// `valueOf`, `hasOwnProperty`, …) is just an ordinary key that returns `undefined` when
|
|
// absent — it can never resolve to an inherited function the executor would mistake for
|
|
// a stored grant. Mirrors the sibling `grants` Map in plugin-node-executor.ts.
|
|
consents: Map<string, PersistedNodeExecutionConsent>;
|
|
}
|
|
|
|
const emptyBlob = (): NodeExecutionConsentBlob => ({
|
|
version: NODE_EXECUTION_CONSENT_STORE_VERSION,
|
|
consents: new Map(),
|
|
});
|
|
|
|
const loadBlob = async (): Promise<NodeExecutionConsentBlob> => {
|
|
const all = await loadSimpleStoreAll();
|
|
const raw = all[SimpleStoreKey.PLUGIN_NODE_EXECUTION_CONSENT];
|
|
if (!raw || typeof raw !== 'object') {
|
|
return emptyBlob();
|
|
}
|
|
const blob = raw as { version?: unknown; consents?: unknown };
|
|
// Forward-safe: a future on-disk format we don't understand is ignored (the user is
|
|
// re-prompted) rather than mis-read into a spurious grant.
|
|
if (
|
|
blob.version !== NODE_EXECUTION_CONSENT_STORE_VERSION ||
|
|
!blob.consents ||
|
|
typeof blob.consents !== 'object'
|
|
) {
|
|
return emptyBlob();
|
|
}
|
|
// Build the Map from the persisted plain object. Only a fully well-formed entry counts:
|
|
// mere presence authorizes execution, so a corrupt/tampered value (a primitive, an array,
|
|
// an empty `{}`, or a partial record) is dropped — the user re-prompts — rather than
|
|
// mis-read as a grant. A literal `__proto__`/`constructor` key from a hand-edited file is
|
|
// just an ordinary Map key here, never a prototype write.
|
|
const consents = new Map<string, PersistedNodeExecutionConsent>();
|
|
for (const [pluginId, entry] of Object.entries(
|
|
blob.consents as Record<string, unknown>,
|
|
)) {
|
|
if (entry === null || typeof entry !== 'object' || Array.isArray(entry)) {
|
|
continue;
|
|
}
|
|
const { name, version, grantedAt } = entry as Record<string, unknown>;
|
|
if (
|
|
typeof name === 'string' &&
|
|
typeof version === 'string' &&
|
|
typeof grantedAt === 'number' &&
|
|
Number.isFinite(grantedAt)
|
|
) {
|
|
consents.set(pluginId, { name, version, grantedAt });
|
|
}
|
|
}
|
|
return { version: NODE_EXECUTION_CONSENT_STORE_VERSION, consents };
|
|
};
|
|
|
|
// Serialize read-modify-write mutations so two concurrent grants/clears can't clobber
|
|
// each other (load-load-write-write). This is NOT redundant with simple-store's own save
|
|
// queue: `loadBlob()` happens *outside* `saveSimpleStore`, so without this lock two
|
|
// interleaved mutations could both read the same blob before either writes. Reads
|
|
// (getNodeExecutionConsent) are point-in-time and need no lock.
|
|
let _mutationQueue: Promise<unknown> = Promise.resolve();
|
|
|
|
const mutate = (apply: (blob: NodeExecutionConsentBlob) => boolean): Promise<void> => {
|
|
const run = async (): Promise<void> => {
|
|
const blob = await loadBlob();
|
|
if (apply(blob)) {
|
|
// Serialize the Map to a plain object for JSON persistence. `Object.fromEntries`
|
|
// uses define-semantics, so even a literal `__proto__` key becomes an own data
|
|
// property — it cannot pollute a prototype. Downgrade note: an older client that
|
|
// finds a newer on-disk `version` reads it as empty (loadBlob) and this write then
|
|
// replaces it with a v1 blob, discarding a future client's consents — worst case a
|
|
// re-prompt, never a spurious grant.
|
|
await saveSimpleStore(SimpleStoreKey.PLUGIN_NODE_EXECUTION_CONSENT, {
|
|
version: blob.version,
|
|
consents: Object.fromEntries(blob.consents),
|
|
});
|
|
}
|
|
};
|
|
_mutationQueue = _mutationQueue.then(run, run);
|
|
return _mutationQueue as Promise<void>;
|
|
};
|
|
|
|
export const getNodeExecutionConsent = async (
|
|
pluginId: string,
|
|
): Promise<PersistedNodeExecutionConsent | null> => {
|
|
const blob = await loadBlob();
|
|
return blob.consents.get(pluginId) ?? null;
|
|
};
|
|
|
|
export const setNodeExecutionConsent = async (
|
|
pluginId: string,
|
|
consent: PersistedNodeExecutionConsent,
|
|
): Promise<void> =>
|
|
mutate((blob) => {
|
|
blob.consents.set(pluginId, {
|
|
name: consent.name,
|
|
version: consent.version,
|
|
grantedAt: consent.grantedAt,
|
|
});
|
|
return true;
|
|
});
|
|
|
|
// `Map.delete` returns true only when an entry existed, which is exactly the
|
|
// "write only if changed" signal `mutate` wants — a clear of an absent id is a no-op.
|
|
export const clearNodeExecutionConsent = async (pluginId: string): Promise<void> =>
|
|
mutate((blob) => blob.consents.delete(pluginId));
|