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:
Johannes Millan 2026-06-25 15:50:41 +02:00
parent a6489e17a8
commit eb80bfc707
7 changed files with 333 additions and 66 deletions

View 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 ')}`,
);
});

View file

@ -10,29 +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,
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>;
}
export interface ElectronAPI {
on(
channel: string,

View file

@ -402,3 +402,79 @@ test('revoke from the issuing webContents drops the grant even without the token
/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;
}
});

View file

@ -16,19 +16,23 @@ const DEFAULT_TIMEOUT = 30000; // 30 seconds
const MAX_TIMEOUT = 300000; // 5 minutes
const BUILT_IN_PLUGIN_ID_RE = /^[a-z0-9][a-z0-9-]*$/;
// Uploaded (community) plugin ids are NOT held to the strict built-in kebab rule —
// they may legitimately use dots/uppercase — but an uploaded id is attacker-controlled
// and used both as a grant Map key and as consent-dialog text, so it must reject
// anything unsafe for those uses (control/zero-width/bidi chars that could spoof the
// dialog, whitespace that could inject extra dialog lines, the ':' persistence
// delimiter) and stay within a sane length.
// 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 UNSAFE_ID_CHARS_RE =
/[\u0000-\u001F\u007F-\u009F\u200B-\u200F\u202A-\u202E\u2066-\u2069:\s]/;
// Self-declared name/version are display-only; strip control/bidi chars and collapse
// whitespace (global flag is for replace, not test, so no lastIndex statefulness).
const UNSAFE_DISPLAY_CHARS_RE =
/[\u0000-\u001F\u007F-\u009F\u200B-\u200F\u202A-\u202E\u2066-\u2069]/g;
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) {
@ -37,18 +41,11 @@ const assertSafePluginId = (pluginId: unknown): string => {
if (pluginId.length > MAX_UPLOADED_PLUGIN_ID_LENGTH) {
throw new Error('Invalid pluginId');
}
if (UNSAFE_ID_CHARS_RE.test(pluginId)) {
throw new Error('Invalid pluginId');
}
// The id is used as a path segment in getBuiltInManifestPath() (existsSync probe),
// so reject path separators and dot-segments — otherwise an id like '..' could probe
// outside the bundled-plugins dir and render misleadingly in the dialog.
if (
pluginId.includes('/') ||
pluginId.includes('\\') ||
pluginId === '.' ||
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;
@ -133,11 +130,14 @@ class PluginNodeExecutor {
}
// Bundled vs uploaded is decided by the main-owned filesystem, never by a
// renderer-supplied flag: a bundled id always uses its verified on-disk
// manifest, so uploaded code can't borrow a built-in plugin's trusted name.
const dialogOptions = this.getBuiltInManifestPath(safeId)
? this.describeVerifiedBuiltInDialog(safeId)
: this.describeUnverifiedUploadedDialog(safeId, displayInfo);
// 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);
@ -184,9 +184,18 @@ class PluginNodeExecutor {
// could inherit a live session grant. The webContents binding still prevents
// another window from revoking this one's grant.
(event, pluginId: string, _grantToken?: string) => {
const grant = this.grants.get(pluginId);
// 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(pluginId);
this.grants.delete(safeId);
}
},
);
@ -204,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 ||
@ -213,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);
},
);
}
@ -280,9 +297,23 @@ class PluginNodeExecutor {
this.unregisterGrantCleanup(webContentsId);
}
/** Consent dialog for a verified built-in plugin (name/version read from disk). */
private describeVerifiedBuiltInDialog(pluginId: string): Electron.MessageBoxOptions {
const manifest = this.getVerifiedBuiltInNodeExecutionManifest(pluginId);
/**
* 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?',

View 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>;
}

View file

@ -117,19 +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,
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>;
}
import { PluginNodeExecutionElectronApi } from '../../../electron/shared-with-frontend/plugin-node-execution.model';
type PluginDateFormat = 'short' | 'medium' | 'long' | 'time' | 'datetime';

View file

@ -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',
]);
@ -1568,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);