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 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).
149 lines
6 KiB
JavaScript
149 lines
6 KiB
JavaScript
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 ')}`,
|
|
);
|
|
});
|