diff --git a/electron/bundled-plugin-ids.test.cjs b/electron/bundled-plugin-ids.test.cjs new file mode 100644 index 0000000000..5e4be6b686 --- /dev/null +++ b/electron/bundled-plugin-ids.test.cjs @@ -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//manifest.json + * packages/plugin-dev//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([', ']'), + ); + + // 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 ')}`, + ); +}); diff --git a/electron/electronAPI.d.ts b/electron/electronAPI.d.ts index 4525ee0846..10c9a28932 100644 --- a/electron/electronAPI.d.ts +++ b/electron/electronAPI.d.ts @@ -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; - revokeGrant(pluginId: string, grantToken: string): Promise; -} - export interface ElectronAPI { on( channel: string, diff --git a/electron/plugin-node-executor.test.cjs b/electron/plugin-node-executor.test.cjs index c43242a1eb..0d31ba351b 100644 --- a/electron/plugin-node-executor.test.cjs +++ b/electron/plugin-node-executor.test.cjs @@ -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; + } +}); diff --git a/electron/plugin-node-executor.ts b/electron/plugin-node-executor.ts index 79282330d8..a623b6d98e 100644 --- a/electron/plugin-node-executor.ts +++ b/electron/plugin-node-executor.ts @@ -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?', diff --git a/electron/shared-with-frontend/plugin-node-execution.model.ts b/electron/shared-with-frontend/plugin-node-execution.model.ts new file mode 100644 index 0000000000..c3a9d91558 --- /dev/null +++ b/electron/shared-with-frontend/plugin-node-execution.model.ts @@ -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; + revokeGrant(pluginId: string, grantToken: string): Promise; +} diff --git a/src/app/plugins/plugin-bridge.service.ts b/src/app/plugins/plugin-bridge.service.ts index 2a35ee2cfb..44e4cc4c34 100644 --- a/src/app/plugins/plugin-bridge.service.ts +++ b/src/app/plugins/plugin-bridge.service.ts @@ -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; - revokeGrant(pluginId: string, grantToken: string): Promise; -} +import { PluginNodeExecutionElectronApi } from '../../../electron/shared-with-frontend/plugin-node-execution.model'; type PluginDateFormat = 'short' | 'medium' | 'long' | 'time' | 'datetime'; diff --git a/src/app/plugins/plugin.service.ts b/src/app/plugins/plugin.service.ts index 6ebd642bbf..8791c00c3a 100644 --- a/src/app/plugins/plugin.service.ts +++ b/src/app/plugins/plugin.service.ts @@ -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([ '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);