mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-17 16:37:43 +00:00
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).
This commit is contained in:
parent
762fd3baab
commit
a67323b2b5
11 changed files with 686 additions and 78 deletions
|
|
@ -552,10 +552,24 @@ console.log(data); // '{ count: 42 }'
|
|||
Plugins with `"permissions": ["nodeExecution"]` can run Node.js scripts in the Electron
|
||||
desktop app after the user allows the desktop permission prompt.
|
||||
|
||||
The desktop grant is currently issued only for packaged built-in plugins whose
|
||||
manifest can be verified by the main process. Uploaded plugins that request
|
||||
`nodeExecution` are rejected until uploaded plugin installation is moved to a
|
||||
main-process-owned verification path.
|
||||
Both built-in and uploaded (community) plugins may request `nodeExecution`. The grant is
|
||||
issued by the Electron **main** process after a native consent dialog, and is bound to
|
||||
the plugin id for the current app session (it is never persisted or synced). For uploaded
|
||||
plugins the app cannot verify the manifest, so the dialog flags the plugin as unverified
|
||||
third-party code with full machine access that Super Productivity cannot sandbox, and
|
||||
defaults to **Deny** — only allow plugins whose source you trust. If the user denies,
|
||||
the plugin stays enabled but its node calls fail until it is re-enabled or the app is
|
||||
restarted (consent is re-requested once per session).
|
||||
|
||||
> **Plugin id constraints (for `nodeExecution`):** the consent grant keys on your
|
||||
> manifest `id`, so it must be a single safe token — no whitespace, control/bidi
|
||||
> characters, `:`, path separators (`/`, `\`), and at most 100 characters. Lowercase
|
||||
> kebab-case is recommended; dots and uppercase are accepted.
|
||||
|
||||
> **Security note:** a granted `nodeExecution` plugin can run any program with full
|
||||
> access to your files and system. The file/IPC channel a plugin uses to talk to a
|
||||
> companion process is an open local channel — treat any data it reads as untrusted
|
||||
> input (never `eval`/`require` its contents).
|
||||
|
||||
```javascript
|
||||
const result = await plugin.executeNodeScript({
|
||||
|
|
|
|||
149
electron/bundled-plugin-ids.test.cjs
Normal file
149
electron/bundled-plugin-ids.test.cjs
Normal file
|
|
@ -0,0 +1,149 @@
|
|||
const test = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('node:fs');
|
||||
const path = require('node:path');
|
||||
|
||||
// SECURITY INVARIANT (cross-layer regression guard)
|
||||
// --------------------------------------------------
|
||||
// `src/app/plugins/plugin.service.ts` declares two top-of-file lists:
|
||||
// - BUNDLED_PLUGIN_PATHS: the on-disk asset dirs of the plugins we ship.
|
||||
// - BUNDLED_PLUGIN_IDS: the reserved set of *manifest ids* that an uploaded
|
||||
// plugin is forbidden from claiming.
|
||||
// The renderer rejects an uploaded plugin whose manifest id is in
|
||||
// BUNDLED_PLUGIN_IDS so unverified code cannot impersonate a built-in. With
|
||||
// nodeExecution now openable to uploaded plugins, an unguarded id would also
|
||||
// let an upload borrow a bundled dir's "verified built-in" consent dialog in
|
||||
// the main process. PATHS are keyed by on-disk dir name; IDS by manifest id;
|
||||
// the dir->id mapping is NOT identity (e.g. dir `yesterday-tasks-plugin` has
|
||||
// manifest id `yesterday-tasks`). The two lists already drifted once (gitea /
|
||||
// linear / trello / azure issue-providers were in PATHS but missing from IDS),
|
||||
// which silently opened the impersonation gap for those ids.
|
||||
//
|
||||
// This test enforces PATHS ⊆ IDS: every bundled plugin's real manifest id MUST
|
||||
// be reserved. It deliberately does NOT require equality — IDS may reserve ids
|
||||
// for plugins not currently shipped via PATHS (e.g. `ai-productivity-prompts`),
|
||||
// which only widens the reserved set and is harmless.
|
||||
//
|
||||
// A Karma/browser unit test cannot read the filesystem, and importing
|
||||
// plugin.service.ts would drag in the whole Angular DI graph. So we parse the
|
||||
// file as text here, in the filesystem-capable `node --test` (electron) suite,
|
||||
// and read manifests straight off disk.
|
||||
|
||||
const REPO_ROOT = path.resolve(__dirname, '..');
|
||||
const PLUGIN_SERVICE_PATH = path.join(
|
||||
REPO_ROOT,
|
||||
'src/app/plugins/plugin.service.ts',
|
||||
);
|
||||
const PLUGIN_DEV_DIR = path.join(REPO_ROOT, 'packages/plugin-dev');
|
||||
|
||||
/**
|
||||
* Extract a named array/Set literal's quoted string entries from the source
|
||||
* text. We don't evaluate the file (no Angular import) — we slice the literal
|
||||
* between its opening token and the matching close bracket, then pull every
|
||||
* single/double-quoted string out of that slice. This stays robust to
|
||||
* formatting (line breaks, trailing commas, `as const`) without executing code.
|
||||
*
|
||||
* @param {string} source full plugin.service.ts text
|
||||
* @param {string} declStart the literal's opening, e.g. `BUNDLED_PLUGIN_PATHS = [`
|
||||
* @param {string} closeChar the matching close bracket, `]` or `)`
|
||||
* @returns {string[]} the quoted entries, in source order
|
||||
*/
|
||||
const extractStringLiteralList = (source, declStart, closeChar) => {
|
||||
const startIdx = source.indexOf(declStart);
|
||||
assert.notEqual(
|
||||
startIdx,
|
||||
-1,
|
||||
`Could not find "${declStart}" in plugin.service.ts — the const may have been renamed; update this regression test.`,
|
||||
);
|
||||
const contentStart = startIdx + declStart.length;
|
||||
const closeIdx = source.indexOf(closeChar, contentStart);
|
||||
assert.notEqual(
|
||||
closeIdx,
|
||||
-1,
|
||||
`Could not find closing "${closeChar}" for "${declStart}" in plugin.service.ts.`,
|
||||
);
|
||||
const slice = source.slice(contentStart, closeIdx);
|
||||
const matches = slice.match(/['"]([^'"]+)['"]/g) || [];
|
||||
return matches.map((m) => m.slice(1, -1));
|
||||
};
|
||||
|
||||
/**
|
||||
* Locate a bundled plugin's manifest. Both layouts exist in the repo:
|
||||
* packages/plugin-dev/<dir>/manifest.json
|
||||
* packages/plugin-dev/<dir>/src/manifest.json
|
||||
* Returns the first that exists, or null if neither does.
|
||||
*/
|
||||
const findManifestPath = (dirName) => {
|
||||
const candidates = [
|
||||
path.join(PLUGIN_DEV_DIR, dirName, 'manifest.json'),
|
||||
path.join(PLUGIN_DEV_DIR, dirName, 'src', 'manifest.json'),
|
||||
];
|
||||
return candidates.find((p) => fs.existsSync(p)) || null;
|
||||
};
|
||||
|
||||
test('every BUNDLED_PLUGIN_PATHS plugin has its manifest id reserved in BUNDLED_PLUGIN_IDS', () => {
|
||||
const source = fs.readFileSync(PLUGIN_SERVICE_PATH, 'utf8');
|
||||
|
||||
const bundledPaths = extractStringLiteralList(
|
||||
source,
|
||||
'BUNDLED_PLUGIN_PATHS = [',
|
||||
']',
|
||||
);
|
||||
const bundledIds = new Set(
|
||||
extractStringLiteralList(source, 'BUNDLED_PLUGIN_IDS = new Set<string>([', ']'),
|
||||
);
|
||||
|
||||
// Sanity: if either list parsed empty, the source format changed and the
|
||||
// guard is silently inert — fail loudly rather than pass vacuously.
|
||||
assert.ok(
|
||||
bundledPaths.length > 0,
|
||||
'Parsed zero entries from BUNDLED_PLUGIN_PATHS — the source format likely changed; update this test.',
|
||||
);
|
||||
assert.ok(
|
||||
bundledIds.size > 0,
|
||||
'Parsed zero entries from BUNDLED_PLUGIN_IDS — the source format likely changed; update this test.',
|
||||
);
|
||||
|
||||
const missingIds = [];
|
||||
const missingManifests = [];
|
||||
|
||||
for (const assetPath of bundledPaths) {
|
||||
const dirName = assetPath.split('/').pop();
|
||||
const manifestPath = findManifestPath(dirName);
|
||||
|
||||
if (!manifestPath) {
|
||||
missingManifests.push(dirName);
|
||||
continue;
|
||||
}
|
||||
|
||||
const manifest = JSON.parse(fs.readFileSync(manifestPath, 'utf8'));
|
||||
const id = manifest.id;
|
||||
assert.ok(
|
||||
typeof id === 'string' && id.length > 0,
|
||||
`Manifest for "${dirName}" has no string "id" field (${manifestPath}).`,
|
||||
);
|
||||
|
||||
if (!bundledIds.has(id)) {
|
||||
missingIds.push(`${dirName} -> "${id}"`);
|
||||
}
|
||||
}
|
||||
|
||||
// A missing manifest for a listed dir is itself drift/misconfig.
|
||||
assert.equal(
|
||||
missingManifests.length,
|
||||
0,
|
||||
`No manifest.json (top-level or src/) found under packages/plugin-dev for bundled plugin dir(s): ${missingManifests.join(
|
||||
', ',
|
||||
)}. Each BUNDLED_PLUGIN_PATHS entry must have a manifest so its id can be verified.`,
|
||||
);
|
||||
|
||||
// The core invariant. List ALL offenders, not just the first.
|
||||
assert.equal(
|
||||
missingIds.length,
|
||||
0,
|
||||
`SECURITY: the following bundled plugins' manifest ids are NOT reserved in ` +
|
||||
`BUNDLED_PLUGIN_IDS (src/app/plugins/plugin.service.ts). An uploaded plugin ` +
|
||||
`could claim these ids and impersonate a built-in. Add each missing id to ` +
|
||||
`BUNDLED_PLUGIN_IDS:\n ${missingIds.join('\n ')}`,
|
||||
);
|
||||
});
|
||||
15
electron/electronAPI.d.ts
vendored
15
electron/electronAPI.d.ts
vendored
|
|
@ -10,26 +10,13 @@ import { AppDataCompleteLegacy } from '../src/app/imex/sync/sync.model';
|
|||
import { Task } from '../src/app/features/tasks/task.model';
|
||||
import { LocalBackupMeta } from '../src/app/imex/local-backup/local-backup.model';
|
||||
import { AppDataComplete } from '../src/app/op-log/model/model-config';
|
||||
import {
|
||||
PluginNodeScriptRequest,
|
||||
PluginNodeScriptResult,
|
||||
} from '../packages/plugin-api/src/types';
|
||||
import { PluginNodeExecutionElectronApi } from './shared-with-frontend/plugin-node-execution.model';
|
||||
import {
|
||||
LocalRestApiRequestPayload,
|
||||
LocalRestApiResponsePayload,
|
||||
} from './shared-with-frontend/local-rest-api.model';
|
||||
import { ElectronDistChannel } from './shared-with-frontend/get-dist-channel';
|
||||
|
||||
export interface PluginNodeExecutionElectronApi {
|
||||
requestGrant(pluginId: string): Promise<{ token: string } | null>;
|
||||
executeScript(
|
||||
pluginId: string,
|
||||
grantToken: string,
|
||||
request: PluginNodeScriptRequest,
|
||||
): Promise<PluginNodeScriptResult>;
|
||||
revokeGrant(pluginId: string, grantToken: string): Promise<void>;
|
||||
}
|
||||
|
||||
export interface ElectronAPI {
|
||||
on(
|
||||
channel: string,
|
||||
|
|
|
|||
|
|
@ -221,7 +221,7 @@ test('does not mint a token when the sender navigates while consent is pending',
|
|||
assert.equal(await grantPromise, null);
|
||||
});
|
||||
|
||||
test('revoke requires the issuing webContents and token', async () => {
|
||||
test('revoke is scoped to the issuing webContents', async () => {
|
||||
loadModule();
|
||||
const webContents = new FakeWebContents(6);
|
||||
const otherWebContents = new FakeWebContents(7);
|
||||
|
|
@ -255,3 +255,226 @@ test('revoke requires the issuing webContents and token', async () => {
|
|||
/not authorized/,
|
||||
);
|
||||
});
|
||||
|
||||
test('issues a grant to an uploaded (non-bundled) plugin and labels it unverified', async () => {
|
||||
loadModule();
|
||||
const webContents = new FakeWebContents(10);
|
||||
|
||||
const grant = await callIpc(
|
||||
'PLUGIN_REQUEST_NODE_EXECUTION_GRANT',
|
||||
webContents,
|
||||
'uploaded-node-plugin',
|
||||
{ name: 'Uploaded Node Plugin', version: '1.2.3' },
|
||||
);
|
||||
|
||||
assert.equal(typeof grant.token, 'string');
|
||||
assert.equal(dialogCalls.length, 1);
|
||||
const opts = dialogCalls[0][1];
|
||||
// Dialog anchors on the validated id and flags the self-declared name as unverified,
|
||||
// and defaults to Deny.
|
||||
assert.match(opts.detail, /Plugin ID: uploaded-node-plugin/);
|
||||
assert.match(opts.detail, /self-declared, unverified/);
|
||||
assert.equal(opts.defaultId, 1);
|
||||
});
|
||||
|
||||
test('uploaded plugin executes after grant; a denied request leaves exec unauthorized', async () => {
|
||||
loadModule();
|
||||
const allowWc = new FakeWebContents(11);
|
||||
const grant = await callIpc(
|
||||
'PLUGIN_REQUEST_NODE_EXECUTION_GRANT',
|
||||
allowWc,
|
||||
'uploaded-node-plugin',
|
||||
{ name: 'Uploaded', version: '1.0.0' },
|
||||
);
|
||||
const okResult = await callIpc(
|
||||
'PLUGIN_EXEC_NODE_SCRIPT',
|
||||
allowWc,
|
||||
'uploaded-node-plugin',
|
||||
grant.token,
|
||||
{ script: 'return args[0] + 1;', args: [4] },
|
||||
);
|
||||
assert.equal(okResult.success, true);
|
||||
assert.equal(okResult.result, 5);
|
||||
|
||||
// A denied request mints no token, so exec stays unauthorized.
|
||||
nextDialogResult = { response: 1 };
|
||||
const denyWc = new FakeWebContents(12);
|
||||
const denied = await callIpc(
|
||||
'PLUGIN_REQUEST_NODE_EXECUTION_GRANT',
|
||||
denyWc,
|
||||
'denied-plugin',
|
||||
{ name: 'Denied', version: '1.0.0' },
|
||||
);
|
||||
assert.equal(denied, null);
|
||||
await assert.rejects(
|
||||
() =>
|
||||
callIpc('PLUGIN_EXEC_NODE_SCRIPT', denyWc, 'denied-plugin', 'made-up-token', {
|
||||
script: 'return true;',
|
||||
}),
|
||||
/not authorized/,
|
||||
);
|
||||
});
|
||||
|
||||
test('accepts a non-kebab uploaded id (dots/uppercase) the built-in rule would reject', async () => {
|
||||
loadModule();
|
||||
const webContents = new FakeWebContents(13);
|
||||
|
||||
const grant = await callIpc(
|
||||
'PLUGIN_REQUEST_NODE_EXECUTION_GRANT',
|
||||
webContents,
|
||||
'Community.Plugin-2',
|
||||
{ name: 'Community Plugin', version: '2.0.0' },
|
||||
);
|
||||
|
||||
assert.equal(typeof grant.token, 'string');
|
||||
});
|
||||
|
||||
test('rejects unsafe ids and sanitizes self-declared display strings', async () => {
|
||||
loadModule();
|
||||
const webContents = new FakeWebContents(14);
|
||||
|
||||
// A newline in the id is rejected outright (it is a grant Map key + dialog text).
|
||||
await assert.rejects(
|
||||
() =>
|
||||
callIpc('PLUGIN_REQUEST_NODE_EXECUTION_GRANT', webContents, 'evil\nid', {
|
||||
name: 'x',
|
||||
version: '1',
|
||||
}),
|
||||
/Invalid pluginId/,
|
||||
);
|
||||
|
||||
// Path separators / dot-segments are rejected (the id is used as a path component in
|
||||
// the bundled-manifest existsSync probe).
|
||||
for (const badId of ['../../etc', '..', '.', 'a/b', 'a\\b']) {
|
||||
await assert.rejects(
|
||||
() =>
|
||||
callIpc('PLUGIN_REQUEST_NODE_EXECUTION_GRANT', webContents, badId, {
|
||||
name: 'x',
|
||||
version: '1',
|
||||
}),
|
||||
/Invalid pluginId/,
|
||||
);
|
||||
}
|
||||
|
||||
// A crafted name cannot inject an extra dialog line and is length-capped.
|
||||
const craftedName = `${'A'.repeat(500)}\nVerified by Super Productivity`;
|
||||
const grant = await callIpc(
|
||||
'PLUGIN_REQUEST_NODE_EXECUTION_GRANT',
|
||||
webContents,
|
||||
'crafted-name-plugin',
|
||||
{ name: craftedName, version: '1.0.0' },
|
||||
);
|
||||
assert.equal(typeof grant.token, 'string');
|
||||
const detail = dialogCalls[dialogCalls.length - 1][1].detail;
|
||||
assert.equal(detail.includes('Verified by Super Productivity'), false);
|
||||
assert.ok(detail.includes('…'));
|
||||
});
|
||||
|
||||
test('revoke from the issuing webContents drops the grant even without the token', async () => {
|
||||
loadModule();
|
||||
const webContents = new FakeWebContents(15);
|
||||
const grant = await callIpc(
|
||||
'PLUGIN_REQUEST_NODE_EXECUTION_GRANT',
|
||||
webContents,
|
||||
'uploaded-node-plugin',
|
||||
{ name: 'Uploaded', version: '1.0.0' },
|
||||
);
|
||||
|
||||
// Teardown/re-upload revokes by id without resupplying the token, so a re-uploaded
|
||||
// plugin reusing the id cannot inherit this live grant.
|
||||
await callIpc(
|
||||
'PLUGIN_REVOKE_NODE_EXECUTION_GRANT',
|
||||
webContents,
|
||||
'uploaded-node-plugin',
|
||||
'',
|
||||
);
|
||||
await assert.rejects(
|
||||
() =>
|
||||
callIpc(
|
||||
'PLUGIN_EXEC_NODE_SCRIPT',
|
||||
webContents,
|
||||
'uploaded-node-plugin',
|
||||
grant.token,
|
||||
{
|
||||
script: 'return true;',
|
||||
},
|
||||
),
|
||||
/not authorized/,
|
||||
);
|
||||
});
|
||||
|
||||
test('rejects bidi/zero-width/homoglyph and leading-dot ids the allowlist must exclude', async () => {
|
||||
loadModule();
|
||||
const webContents = new FakeWebContents(16);
|
||||
|
||||
// These are exactly the dialog-anchor spoofing vectors a denylist of explicit Unicode
|
||||
// ranges tends to miss; the allowlist rejects every non-[A-Za-z0-9._-] id by construction.
|
||||
const badIds = [
|
||||
`sync${String.fromCodePoint(0x061c)}md`, // U+061C ARABIC LETTER MARK (bidi)
|
||||
`sync${String.fromCodePoint(0x2060)}md`, // U+2060 WORD JOINER (zero-width)
|
||||
`sync${String.fromCodePoint(0x3164)}md`, // U+3164 HANGUL FILLER (invisible)
|
||||
`${String.fromCodePoint(0xff53)}ync-md`, // U+FF53 fullwidth 's' (homoglyph)
|
||||
'.hidden', // leading dot-segment
|
||||
'-leading-dash',
|
||||
'a b', // whitespace
|
||||
];
|
||||
|
||||
for (const badId of badIds) {
|
||||
await assert.rejects(
|
||||
() =>
|
||||
callIpc('PLUGIN_REQUEST_NODE_EXECUTION_GRANT', webContents, badId, {
|
||||
name: 'x',
|
||||
version: '1',
|
||||
}),
|
||||
/Invalid pluginId/,
|
||||
`expected ${JSON.stringify(badId)} to be rejected`,
|
||||
);
|
||||
}
|
||||
});
|
||||
|
||||
test('strips bidi/zero-width chars from self-declared display strings', async () => {
|
||||
loadModule();
|
||||
const webContents = new FakeWebContents(17);
|
||||
|
||||
const name = `Tru${String.fromCodePoint(0x061c)}sted${String.fromCodePoint(0x2060)} Plugin`;
|
||||
const grant = await callIpc(
|
||||
'PLUGIN_REQUEST_NODE_EXECUTION_GRANT',
|
||||
webContents,
|
||||
'display-sanitize-plugin',
|
||||
{ name, version: `1.0${String.fromCodePoint(0xfeff)}.0` },
|
||||
);
|
||||
|
||||
assert.equal(typeof grant.token, 'string');
|
||||
const detail = dialogCalls[dialogCalls.length - 1][1].detail;
|
||||
// The control/format chars are gone; the visible text survives.
|
||||
assert.equal(detail.includes(String.fromCodePoint(0x061c)), false);
|
||||
assert.equal(detail.includes(String.fromCodePoint(0x2060)), false);
|
||||
assert.equal(detail.includes(String.fromCodePoint(0xfeff)), false);
|
||||
assert.match(detail, /Trusted Plugin/);
|
||||
});
|
||||
|
||||
test('never upgrades trust: an on-disk match that does not cleanly verify uses the unverified dialog', async () => {
|
||||
// Simulate a bundled dir whose manifest exists but is not a grantable nodeExecution
|
||||
// built-in (here: missing the permission). The verified-built-in branch must return
|
||||
// null and fall back to the unverified-uploaded dialog rather than throw or upgrade.
|
||||
const originalPermissions = BUILT_IN_PLUGIN_MANIFEST.permissions;
|
||||
BUILT_IN_PLUGIN_MANIFEST.permissions = [];
|
||||
try {
|
||||
loadModule();
|
||||
const webContents = new FakeWebContents(18);
|
||||
const grant = await callIpc(
|
||||
'PLUGIN_REQUEST_NODE_EXECUTION_GRANT',
|
||||
webContents,
|
||||
BUILT_IN_PLUGIN_MANIFEST.id,
|
||||
{ name: 'Impersonator', version: '9.9.9' },
|
||||
);
|
||||
|
||||
assert.equal(typeof grant.token, 'string');
|
||||
const opts = dialogCalls[dialogCalls.length - 1][1];
|
||||
assert.match(opts.title, /run code on your machine/);
|
||||
assert.match(opts.detail, /self-declared, unverified/);
|
||||
assert.equal(opts.defaultId, 1);
|
||||
} finally {
|
||||
BUILT_IN_PLUGIN_MANIFEST.permissions = originalPermissions;
|
||||
}
|
||||
});
|
||||
|
|
|
|||
|
|
@ -16,11 +16,72 @@ const DEFAULT_TIMEOUT = 30000; // 30 seconds
|
|||
const MAX_TIMEOUT = 300000; // 5 minutes
|
||||
const BUILT_IN_PLUGIN_ID_RE = /^[a-z0-9][a-z0-9-]*$/;
|
||||
|
||||
// An uploaded (community) plugin id is attacker-controlled and used both as a grant Map
|
||||
// key and as the consent dialog's trust anchor ("Plugin ID: ..."), and as a path segment
|
||||
// in getBuiltInManifestPath(). It is NOT held to the strict built-in kebab rule —
|
||||
// community ids may use dots/uppercase, e.g. `super-productivity-mcp` — but it must be a
|
||||
// single safe ASCII token. We use an allowlist rather than a denylist on purpose: the
|
||||
// allowlist rejects control/zero-width/bidi/homoglyph characters that could spoof the
|
||||
// dialog, whitespace that could inject extra dialog lines, the ':' persistence delimiter,
|
||||
// and path separators / leading-dot segments ('.', '..', '/', '\\') — all by construction,
|
||||
// with no Unicode range to keep updated as new code points are assigned.
|
||||
const MAX_UPLOADED_PLUGIN_ID_LENGTH = 100;
|
||||
const SAFE_UPLOADED_PLUGIN_ID_RE = /^[A-Za-z0-9][A-Za-z0-9._-]*$/;
|
||||
// Self-declared name/version are display-only. Strip every Unicode control (Cc) and
|
||||
// format (Cf) character — this covers C0/C1 controls, all zero-width characters, the BOM,
|
||||
// and every bidi control (incl. U+061C ALM, the word-joiner range, and the isolate marks)
|
||||
// without enumerating ranges — then collapse whitespace so a crafted value cannot inject
|
||||
// extra dialog lines. (Global flag is for replace, not test, so no lastIndex statefulness.)
|
||||
const UNSAFE_DISPLAY_CHARS_RE = /[\p{Cc}\p{Cf}]/gu;
|
||||
|
||||
const assertSafePluginId = (pluginId: unknown): string => {
|
||||
if (typeof pluginId !== 'string' || pluginId.length === 0) {
|
||||
throw new Error('Invalid pluginId');
|
||||
}
|
||||
if (pluginId.length > MAX_UPLOADED_PLUGIN_ID_LENGTH) {
|
||||
throw new Error('Invalid pluginId');
|
||||
}
|
||||
// Allowlist match also rejects path separators ('/'/'\\'), leading-dot segments
|
||||
// ('.', '..'), ':', whitespace and all non-ASCII (bidi/zero-width/homoglyph), so the id
|
||||
// can neither escape the bundled-plugins dir in getBuiltInManifestPath() nor spoof the
|
||||
// consent dialog's trust anchor.
|
||||
if (!SAFE_UPLOADED_PLUGIN_ID_RE.test(pluginId)) {
|
||||
throw new Error('Invalid pluginId');
|
||||
}
|
||||
return pluginId;
|
||||
};
|
||||
|
||||
const sanitizeDialogString = (value: unknown, maxLength: number): string => {
|
||||
if (typeof value !== 'string') {
|
||||
return '';
|
||||
}
|
||||
const cleaned = value.replace(UNSAFE_DISPLAY_CHARS_RE, '').replace(/\s+/g, ' ').trim();
|
||||
return cleaned.length > maxLength ? `${cleaned.slice(0, maxLength)}…` : cleaned;
|
||||
};
|
||||
|
||||
// Shared shell for both nodeExecution consent dialogs: a warning with Allow/Deny where
|
||||
// Deny is the default + cancel action, so a reflexive Enter/Escape denies.
|
||||
const NODE_CONSENT_DIALOG_BASE: Pick<
|
||||
Electron.MessageBoxOptions,
|
||||
'type' | 'buttons' | 'defaultId' | 'cancelId'
|
||||
> = {
|
||||
type: 'warning',
|
||||
buttons: ['Allow', 'Deny'],
|
||||
defaultId: 1,
|
||||
cancelId: 1,
|
||||
};
|
||||
|
||||
interface NodeExecutionGrant {
|
||||
token: string;
|
||||
webContentsId: number;
|
||||
}
|
||||
|
||||
/** Self-declared, unverified display metadata supplied by the renderer for uploaded plugins. */
|
||||
interface NodeExecutionGrantDisplayInfo {
|
||||
name?: string;
|
||||
version?: string;
|
||||
}
|
||||
|
||||
interface WebContentsGrantCleanup {
|
||||
webContents: WebContents;
|
||||
cleanup: () => void;
|
||||
|
|
@ -48,42 +109,42 @@ class PluginNodeExecutor {
|
|||
private setupIpcHandler(): void {
|
||||
ipcMain.handle(
|
||||
IPC.PLUGIN_REQUEST_NODE_EXECUTION_GRANT,
|
||||
async (event, pluginId: string) => {
|
||||
async (event, pluginId: string, displayInfo?: NodeExecutionGrantDisplayInfo) => {
|
||||
const window = BrowserWindow.fromWebContents(event.sender);
|
||||
if (!window) {
|
||||
throw new Error('No window found for event sender');
|
||||
}
|
||||
|
||||
// Sanitize first: the id is used as a grant Map key AND shown in the consent
|
||||
// dialog, and for uploaded plugins it is attacker-controlled.
|
||||
const safeId = assertSafePluginId(pluginId);
|
||||
|
||||
const webContentsId = event.sender.id;
|
||||
const existingGrant = this.grants.get(pluginId);
|
||||
const existingGrant = this.grants.get(safeId);
|
||||
if (existingGrant) {
|
||||
if (existingGrant.webContentsId === webContentsId) {
|
||||
return { token: existingGrant.token };
|
||||
}
|
||||
this.grants.delete(pluginId);
|
||||
this.grants.delete(safeId);
|
||||
this.releaseGrantCleanupIfUnused(existingGrant.webContentsId);
|
||||
}
|
||||
|
||||
const manifest = this.getVerifiedBuiltInNodeExecutionManifest(pluginId);
|
||||
// Bundled vs uploaded is decided by the main-owned filesystem, never by a
|
||||
// renderer-supplied flag, and only an id that resolves to a cleanly-verified
|
||||
// on-disk manifest gets the trusted built-in dialog. A partial or colliding match
|
||||
// (id mismatch, missing nodeExecution permission, unreadable manifest) returns
|
||||
// null and falls back to the unverified dialog, so uploaded code can never borrow
|
||||
// a built-in plugin's trusted name even if its id collides with a bundled dir.
|
||||
const dialogOptions =
|
||||
this.describeVerifiedBuiltInDialog(safeId) ??
|
||||
this.describeUnverifiedUploadedDialog(safeId, displayInfo);
|
||||
|
||||
const requestUrl = event.sender.getURL();
|
||||
this.registerGrantCleanup(event.sender);
|
||||
|
||||
let result;
|
||||
let result: Electron.MessageBoxReturnValue;
|
||||
try {
|
||||
result = await dialog.showMessageBox(window, {
|
||||
type: 'warning',
|
||||
buttons: ['Allow', 'Deny'],
|
||||
defaultId: 1,
|
||||
cancelId: 1,
|
||||
title: 'Allow plugin Node.js execution?',
|
||||
message: `Allow "${manifest.name}" to run Node.js scripts?`,
|
||||
detail: [
|
||||
`Plugin ID: ${pluginId}`,
|
||||
`Version: ${manifest.version}`,
|
||||
'',
|
||||
'This permission is valid for the current app session. Node.js execution can access local files and desktop APIs. Only allow plugins you trust.',
|
||||
].join('\n'),
|
||||
});
|
||||
result = await dialog.showMessageBox(window, dialogOptions);
|
||||
} catch (error) {
|
||||
this.releaseGrantCleanupIfUnused(webContentsId);
|
||||
throw error;
|
||||
|
|
@ -94,19 +155,19 @@ class PluginNodeExecutor {
|
|||
!this.grantCleanupByWebContents.has(webContentsId) ||
|
||||
event.sender.getURL() !== requestUrl
|
||||
) {
|
||||
this.grants.delete(pluginId);
|
||||
this.grants.delete(safeId);
|
||||
this.releaseGrantCleanupIfUnused(webContentsId);
|
||||
return null;
|
||||
}
|
||||
|
||||
if (result.response !== 0) {
|
||||
this.grants.delete(pluginId);
|
||||
this.grants.delete(safeId);
|
||||
this.releaseGrantCleanupIfUnused(webContentsId);
|
||||
return null;
|
||||
}
|
||||
|
||||
const token = randomBytes(32).toString('base64url');
|
||||
this.grants.set(pluginId, {
|
||||
this.grants.set(safeId, {
|
||||
token,
|
||||
webContentsId,
|
||||
});
|
||||
|
|
@ -116,10 +177,25 @@ class PluginNodeExecutor {
|
|||
|
||||
ipcMain.handle(
|
||||
IPC.PLUGIN_REVOKE_NODE_EXECUTION_GRANT,
|
||||
(event, pluginId: string, grantToken: string) => {
|
||||
const grant = this.grants.get(pluginId);
|
||||
if (grant?.token === grantToken && grant.webContentsId === event.sender.id) {
|
||||
this.grants.delete(pluginId);
|
||||
// grantToken is accepted for signature compatibility but intentionally not
|
||||
// required: revoking only removes a capability, and the issuing window must be
|
||||
// able to drop its own grant during teardown even if it no longer holds the
|
||||
// token (e.g. on re-upload) — otherwise a re-uploaded plugin reusing the id
|
||||
// could inherit a live session grant. The webContents binding still prevents
|
||||
// another window from revoking this one's grant.
|
||||
(event, pluginId: string, _grantToken?: string) => {
|
||||
// Key the lookup through the same validator the request handler uses, so the
|
||||
// "always revoke by id on teardown/re-upload" guarantee holds even if the id
|
||||
// canonicalisation ever changes (an unsafe id can never hold a grant anyway).
|
||||
let safeId: string;
|
||||
try {
|
||||
safeId = assertSafePluginId(pluginId);
|
||||
} catch {
|
||||
return;
|
||||
}
|
||||
const grant = this.grants.get(safeId);
|
||||
if (grant && grant.webContentsId === event.sender.id) {
|
||||
this.grants.delete(safeId);
|
||||
}
|
||||
},
|
||||
);
|
||||
|
|
@ -137,7 +213,15 @@ class PluginNodeExecutor {
|
|||
throw new Error('No window found for event sender');
|
||||
}
|
||||
|
||||
const grant = this.grants.get(pluginId);
|
||||
// Validate the id the same way the grant handler does so the Map keys match.
|
||||
// An unsafe id can never hold a grant, so treat it as unauthorized.
|
||||
let safeId: string;
|
||||
try {
|
||||
safeId = assertSafePluginId(pluginId);
|
||||
} catch {
|
||||
throw new Error('Plugin is not authorized for nodeExecution');
|
||||
}
|
||||
const grant = this.grants.get(safeId);
|
||||
if (
|
||||
!grant ||
|
||||
grant.token !== grantToken ||
|
||||
|
|
@ -146,7 +230,7 @@ class PluginNodeExecutor {
|
|||
throw new Error('Plugin is not authorized for nodeExecution');
|
||||
}
|
||||
|
||||
return await this.executeScript(pluginId, request);
|
||||
return await this.executeScript(safeId, request);
|
||||
},
|
||||
);
|
||||
}
|
||||
|
|
@ -213,6 +297,64 @@ class PluginNodeExecutor {
|
|||
this.unregisterGrantCleanup(webContentsId);
|
||||
}
|
||||
|
||||
/**
|
||||
* Consent dialog for a verified built-in plugin (name/version read from disk).
|
||||
* Returns null when the id does not resolve to a cleanly-verified built-in
|
||||
* nodeExecution manifest (no on-disk match, id mismatch, missing permission, or
|
||||
* unreadable/invalid manifest), so the caller falls back to the unverified-uploaded
|
||||
* dialog — a partial or colliding match must never *upgrade* trust to the built-in
|
||||
* dialog.
|
||||
*/
|
||||
private describeVerifiedBuiltInDialog(
|
||||
pluginId: string,
|
||||
): Electron.MessageBoxOptions | null {
|
||||
let manifest: PluginManifest;
|
||||
try {
|
||||
manifest = this.getVerifiedBuiltInNodeExecutionManifest(pluginId);
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
return {
|
||||
...NODE_CONSENT_DIALOG_BASE,
|
||||
title: 'Allow plugin Node.js execution?',
|
||||
message: `Allow "${manifest.name}" to run Node.js scripts?`,
|
||||
detail: [
|
||||
`Plugin ID: ${pluginId}`,
|
||||
`Version: ${manifest.version}`,
|
||||
'',
|
||||
'This permission is valid for the current app session. Node.js execution can access local files and desktop APIs. Only allow plugins you trust.',
|
||||
].join('\n'),
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Consent dialog for an uploaded (community) plugin. The app cannot verify an
|
||||
* uploaded plugin's identity, so the dialog anchors on the validated id and marks
|
||||
* the renderer-supplied name/version as self-declared/unverified. Default = Deny.
|
||||
*/
|
||||
private describeUnverifiedUploadedDialog(
|
||||
pluginId: string,
|
||||
displayInfo?: NodeExecutionGrantDisplayInfo,
|
||||
): Electron.MessageBoxOptions {
|
||||
const name = sanitizeDialogString(displayInfo?.name, 80) || '(unnamed)';
|
||||
const version = sanitizeDialogString(displayInfo?.version, 32) || '(unknown)';
|
||||
return {
|
||||
...NODE_CONSENT_DIALOG_BASE,
|
||||
title: 'Allow this plugin to run code on your machine?',
|
||||
message: `Plugin "${pluginId}" wants to run Node.js code`,
|
||||
detail: [
|
||||
`Plugin ID: ${pluginId}`,
|
||||
`Name (self-declared, unverified): ${name}`,
|
||||
`Version (self-declared): ${version}`,
|
||||
'',
|
||||
'This is a third-party plugin. Super Productivity cannot verify its identity and cannot sandbox it.',
|
||||
'If you allow it, the plugin can run any program with full access to your files and system for this app session.',
|
||||
'',
|
||||
'Only allow this if you trust the source of this plugin.',
|
||||
].join('\n'),
|
||||
};
|
||||
}
|
||||
|
||||
private getVerifiedBuiltInNodeExecutionManifest(pluginId: string): PluginManifest {
|
||||
if (typeof pluginId !== 'string' || !pluginId) {
|
||||
throw new Error('Invalid pluginId');
|
||||
|
|
|
|||
|
|
@ -245,8 +245,11 @@ const ea: ElectronAPI = {
|
|||
}
|
||||
pluginNodeExecutionApiConsumed = true;
|
||||
return {
|
||||
requestGrant: (pluginId: string) =>
|
||||
_invoke('PLUGIN_REQUEST_NODE_EXECUTION_GRANT', pluginId) as Promise<{
|
||||
requestGrant: (
|
||||
pluginId: string,
|
||||
displayInfo?: { name?: string; version?: string },
|
||||
) =>
|
||||
_invoke('PLUGIN_REQUEST_NODE_EXECUTION_GRANT', pluginId, displayInfo) as Promise<{
|
||||
token: string;
|
||||
} | null>,
|
||||
executeScript: (
|
||||
|
|
|
|||
24
electron/shared-with-frontend/plugin-node-execution.model.ts
Normal file
24
electron/shared-with-frontend/plugin-node-execution.model.ts
Normal file
|
|
@ -0,0 +1,24 @@
|
|||
import {
|
||||
PluginNodeScriptRequest,
|
||||
PluginNodeScriptResult,
|
||||
} from '../../packages/plugin-api/src/types';
|
||||
|
||||
/**
|
||||
* Shape of the Electron main-process bridge the renderer uses to grant, run, and
|
||||
* revoke Node script execution for a plugin. This is a host-internal IPC contract
|
||||
* (not part of the public plugin API), shared between the renderer
|
||||
* (`plugin-bridge.service.ts`) and the Electron API typing
|
||||
* (`electron/electronAPI.d.ts`) so the two cannot drift.
|
||||
*/
|
||||
export interface PluginNodeExecutionElectronApi {
|
||||
requestGrant(
|
||||
pluginId: string,
|
||||
displayInfo?: { name?: string; version?: string },
|
||||
): Promise<{ token: string } | null>;
|
||||
executeScript(
|
||||
pluginId: string,
|
||||
grantToken: string,
|
||||
request: PluginNodeScriptRequest,
|
||||
): Promise<PluginNodeScriptResult>;
|
||||
revokeGrant(pluginId: string, grantToken: string): Promise<void>;
|
||||
}
|
||||
|
|
@ -117,16 +117,7 @@ import {
|
|||
} from '../features/simple-counter/store/simple-counter.actions';
|
||||
import { getDbDateStr } from '../util/get-db-date-str';
|
||||
import { DataInitService } from '../core/data-init/data-init.service';
|
||||
|
||||
interface PluginNodeExecutionElectronApi {
|
||||
requestGrant(pluginId: string): Promise<{ token: string } | null>;
|
||||
executeScript(
|
||||
pluginId: string,
|
||||
grantToken: string,
|
||||
request: PluginNodeScriptRequest,
|
||||
): Promise<PluginNodeScriptResult>;
|
||||
revokeGrant(pluginId: string, grantToken: string): Promise<void>;
|
||||
}
|
||||
import { PluginNodeExecutionElectronApi } from '../../../electron/shared-with-frontend/plugin-node-execution.model';
|
||||
|
||||
type PluginDateFormat = 'short' | 'medium' | 'long' | 'time' | 'datetime';
|
||||
|
||||
|
|
@ -1716,8 +1707,11 @@ export class PluginBridgeService implements OnDestroy {
|
|||
return this.#nodeExecutionGrantTokens.get(pluginId);
|
||||
}
|
||||
|
||||
async requestNodeExecutionGrant(pluginId: string): Promise<{ token: string } | null> {
|
||||
return (await this.#nodeExecutionApi?.requestGrant(pluginId)) ?? null;
|
||||
async requestNodeExecutionGrant(
|
||||
pluginId: string,
|
||||
displayInfo?: { name?: string; version?: string },
|
||||
): Promise<{ token: string } | null> {
|
||||
return (await this.#nodeExecutionApi?.requestGrant(pluginId, displayInfo)) ?? null;
|
||||
}
|
||||
|
||||
revokeNodeExecutionGrantToken(pluginId: string): string | undefined {
|
||||
|
|
|
|||
|
|
@ -220,7 +220,7 @@ describe('PluginService loadPluginFromZip iframe-only plugins', () => {
|
|||
expect(pluginCache.storePlugin).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('rejects uploaded plugins that declare nodeExecution before storing or loading code', async () => {
|
||||
it('accepts uploaded plugins that declare nodeExecution (gated later by main-process consent)', async () => {
|
||||
const nodeExecutionManifest: PluginManifest = {
|
||||
...iframeManifest,
|
||||
id: 'uploaded-node-plugin',
|
||||
|
|
@ -229,13 +229,17 @@ describe('PluginService loadPluginFromZip iframe-only plugins', () => {
|
|||
};
|
||||
const files: Record<string, string> = {};
|
||||
files['manifest.json'] = JSON.stringify(nodeExecutionManifest);
|
||||
files['plugin.js'] = 'PluginAPI.log.log("should not run")';
|
||||
files['plugin.js'] = 'PluginAPI.log.log("node plugin")';
|
||||
const file = createZipFile(files);
|
||||
|
||||
await expectAsync(service.loadPluginFromZip(file)).toBeRejectedWithError(
|
||||
T.PLUGINS.NODE_EXECUTION_BUILT_IN_ONLY,
|
||||
);
|
||||
expect(pluginCache.storePlugin).not.toHaveBeenCalled();
|
||||
expect(pluginRunner.loadPlugin).not.toHaveBeenCalled();
|
||||
// Resolving (instead of throwing NODE_EXECUTION_BUILT_IN_ONLY) is the behaviour
|
||||
// change: uploaded node plugins are no longer rejected at upload time. The
|
||||
// nodeExecution capability is gated by the main-process consent dialog at grant
|
||||
// time instead.
|
||||
const instance = await service.loadPluginFromZip(file);
|
||||
|
||||
expect(instance).toBeTruthy();
|
||||
expect(instance.manifest.id).toBe('uploaded-node-plugin');
|
||||
expect(pluginCache.storePlugin).toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -255,7 +255,10 @@ describe('PluginService', () => {
|
|||
const result = await service.enableAndActivatePlugin(manifest.id);
|
||||
|
||||
expect(result).toBeNull();
|
||||
expect(pluginBridge.requestNodeExecutionGrant).toHaveBeenCalledOnceWith(manifest.id);
|
||||
expect(pluginBridge.requestNodeExecutionGrant).toHaveBeenCalledOnceWith(manifest.id, {
|
||||
name: manifest.name,
|
||||
version: manifest.version,
|
||||
});
|
||||
expect(pluginMetaPersistenceService.setPluginEnabled).not.toHaveBeenCalled();
|
||||
expect(pluginLoader.loadPluginAssets).not.toHaveBeenCalled();
|
||||
expect(service.getAllPluginStates().get(manifest.id)).toEqual(
|
||||
|
|
@ -266,6 +269,31 @@ describe('PluginService', () => {
|
|||
);
|
||||
});
|
||||
|
||||
it('does not re-prompt for nodeExecution after a denial within the same session', async () => {
|
||||
const runtime = service as unknown as { _isElectronRuntime: () => boolean };
|
||||
spyOn(runtime, '_isElectronRuntime').and.returnValue(true);
|
||||
const manifest: PluginManifest = {
|
||||
...mockManifest,
|
||||
id: 'node-plugin',
|
||||
name: 'Node Plugin',
|
||||
permissions: ['nodeExecution'],
|
||||
};
|
||||
const ensureGrant = (
|
||||
service as unknown as {
|
||||
_ensureNodeExecutionGrant: (m: PluginManifest) => Promise<boolean>;
|
||||
}
|
||||
)._ensureNodeExecutionGrant.bind(service);
|
||||
|
||||
// First interactive attempt prompts and is denied (requestNodeExecutionGrant -> null).
|
||||
await expectAsync(service.checkNodeExecutionPermission(manifest)).toBeResolvedTo(
|
||||
false,
|
||||
);
|
||||
// A later non-interactive grant attempt this session (e.g. startup re-entry via
|
||||
// _fireOnReady) must NOT re-open the native prompt.
|
||||
await expectAsync(ensureGrant(manifest)).toBeResolvedTo(false);
|
||||
expect(pluginBridge.requestNodeExecutionGrant).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('stores main-issued nodeExecution grants for Electron plugins', async () => {
|
||||
pluginBridge.requestNodeExecutionGrant.and.resolveTo({ token: 'token-1' });
|
||||
const runtime = service as unknown as { _isElectronRuntime: () => boolean };
|
||||
|
|
@ -281,7 +309,10 @@ describe('PluginService', () => {
|
|||
true,
|
||||
);
|
||||
|
||||
expect(pluginBridge.requestNodeExecutionGrant).toHaveBeenCalledOnceWith(manifest.id);
|
||||
expect(pluginBridge.requestNodeExecutionGrant).toHaveBeenCalledOnceWith(manifest.id, {
|
||||
name: manifest.name,
|
||||
version: manifest.version,
|
||||
});
|
||||
expect(pluginBridge.setNodeExecutionGrantToken).toHaveBeenCalledOnceWith(
|
||||
manifest.id,
|
||||
'token-1',
|
||||
|
|
|
|||
|
|
@ -62,18 +62,29 @@ const BUNDLED_PLUGIN_PATHS = [
|
|||
'assets/bundled-plugins/doc-mode',
|
||||
] as const;
|
||||
|
||||
// Reserved ids: an uploaded plugin may not reuse a bundled plugin's manifest id (it would
|
||||
// let unverified code impersonate a built-in — and, with nodeExecution now openable to
|
||||
// uploaded plugins, claim a bundled dir's "verified built-in" consent dialog in the main
|
||||
// process, which decides bundled-vs-uploaded by on-disk dir). This set MUST contain the
|
||||
// manifest id of every entry in BUNDLED_PLUGIN_PATHS; the invariant is guarded by
|
||||
// electron/bundled-plugin-ids.test.cjs (a filesystem-reading node test, since a browser
|
||||
// Karma spec cannot read the manifests) so the two lists cannot silently drift again.
|
||||
const BUNDLED_PLUGIN_IDS = new Set<string>([
|
||||
'ai-productivity-prompts',
|
||||
'api-test-plugin',
|
||||
'automations',
|
||||
'azure-devops-issue-provider',
|
||||
'brain-dump',
|
||||
'caldav-calendar-provider',
|
||||
'clickup-issue-provider',
|
||||
'doc-mode',
|
||||
'gitea-issue-provider',
|
||||
'github-issue-provider',
|
||||
'google-calendar-provider',
|
||||
'linear-issue-provider',
|
||||
'procrastination-buster',
|
||||
'sync-md',
|
||||
'trello-issue-provider',
|
||||
'voice-reminder',
|
||||
'yesterday-tasks',
|
||||
]);
|
||||
|
|
@ -109,6 +120,13 @@ export class PluginService implements OnDestroy {
|
|||
private _pluginIcons: Map<string, string> = new Map(); // Store plugin ID -> SVG icon content
|
||||
private _pluginIframeGenerations: Map<string, number> = new Map();
|
||||
private _pluginIconsSignal = signal<Map<string, string>>(new Map());
|
||||
// Plugin ids the user denied nodeExecution for this app session. In-memory only —
|
||||
// never persisted or synced (consent is session-scoped). Makes a denial sticky so a
|
||||
// later non-interactive grant attempt (e.g. startup re-activation via _fireOnReady,
|
||||
// which doesn't pass through checkNodeExecutionPermission) doesn't re-open the native
|
||||
// prompt. Added on deny in _ensureNodeExecutionGrant; cleared only on an explicit
|
||||
// user-initiated enable in checkNodeExecutionPermission (so re-enable always re-asks).
|
||||
private readonly _nodeExecutionDeniedThisSession = new Set<string>();
|
||||
|
||||
// Lazy loading state management
|
||||
private _pluginStates = signal<Map<string, PluginState>>(new Map());
|
||||
|
|
@ -1561,6 +1579,10 @@ export class PluginService implements OnDestroy {
|
|||
this._pluginIcons.delete(pluginId);
|
||||
this._pluginIconsSignal.set(new Map(this._pluginIcons));
|
||||
|
||||
// Drop any session nodeExecution denial so a fresh re-upload of this id is prompted
|
||||
// again rather than silently failing closed against the removed plugin's decision.
|
||||
this._nodeExecutionDeniedThisSession.delete(pluginId);
|
||||
|
||||
// Remove from plugin states
|
||||
this._deletePluginState(pluginId);
|
||||
|
||||
|
|
@ -1796,14 +1818,25 @@ export class PluginService implements OnDestroy {
|
|||
if (this._pluginBridge.hasNodeExecutionGrantToken(manifest.id)) {
|
||||
return true;
|
||||
}
|
||||
// A single enable flow reaches this from several call-sites; once the user has
|
||||
// denied this session, don't re-open the native prompt until they re-enable.
|
||||
if (this._nodeExecutionDeniedThisSession.has(manifest.id)) {
|
||||
return false;
|
||||
}
|
||||
let grant: { token: string } | null;
|
||||
try {
|
||||
grant = await this._pluginBridge.requestNodeExecutionGrant(manifest.id);
|
||||
// name/version are sent for the consent dialog only; main treats them as
|
||||
// self-declared/unverified for uploaded plugins (it never trusts them for auth).
|
||||
grant = await this._pluginBridge.requestNodeExecutionGrant(manifest.id, {
|
||||
name: manifest.name,
|
||||
version: manifest.version,
|
||||
});
|
||||
} catch (error) {
|
||||
PluginLog.err(`Failed to get nodeExecution grant for ${manifest.id}:`, error);
|
||||
return false;
|
||||
}
|
||||
if (!grant) {
|
||||
this._nodeExecutionDeniedThisSession.add(manifest.id);
|
||||
return false;
|
||||
}
|
||||
|
||||
|
|
@ -1813,10 +1846,13 @@ export class PluginService implements OnDestroy {
|
|||
|
||||
private async _revokeNodeExecutionGrant(pluginId: string): Promise<void> {
|
||||
const grantToken = this._pluginBridge.revokeNodeExecutionGrantToken(pluginId);
|
||||
if (!grantToken || !this._isElectronRuntime()) {
|
||||
if (!this._isElectronRuntime()) {
|
||||
return;
|
||||
}
|
||||
await this._pluginBridge.revokeNodeExecutionGrant(pluginId, grantToken);
|
||||
// Always tell main to drop the grant for this id, even if the renderer no longer
|
||||
// holds the token (main revokes by pluginId + webContents), so a re-upload under
|
||||
// the same id can never inherit a live session grant.
|
||||
await this._pluginBridge.revokeNodeExecutionGrant(pluginId, grantToken ?? '');
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -1836,6 +1872,9 @@ export class PluginService implements OnDestroy {
|
|||
return false;
|
||||
}
|
||||
|
||||
// This is the interactive (user-initiated) entry point, so an explicit enable
|
||||
// attempt clears any earlier this-session denial and re-opens the prompt.
|
||||
this._nodeExecutionDeniedThisSession.delete(manifest.id);
|
||||
return this._ensureNodeExecutionGrant(manifest);
|
||||
}
|
||||
|
||||
|
|
@ -1886,6 +1925,9 @@ export class PluginService implements OnDestroy {
|
|||
}
|
||||
|
||||
private _assertUploadedPluginAllowed(manifest: PluginManifest): void {
|
||||
// Uploaded plugins may not reuse a bundled plugin's id (it would let unverified
|
||||
// code impersonate a built-in). nodeExecution is no longer blocked here: uploaded
|
||||
// node plugins are gated by the main-process consent dialog at grant time instead.
|
||||
if (this._isBundledPluginId(manifest.id)) {
|
||||
throw new Error(
|
||||
this._translateService.instant(T.PLUGINS.PLUGIN_ID_RESERVED, {
|
||||
|
|
@ -1893,11 +1935,6 @@ export class PluginService implements OnDestroy {
|
|||
}),
|
||||
);
|
||||
}
|
||||
if (manifest.permissions?.includes('nodeExecution')) {
|
||||
throw new Error(
|
||||
this._translateService.instant(T.PLUGINS.NODE_EXECUTION_BUILT_IN_ONLY),
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue