From d91db37b3425745a7564a8b68921237dcf76fd78 Mon Sep 17 00:00:00 2001 From: Bernd Rodler Date: Tue, 4 Aug 2026 08:57:28 +0200 Subject: [PATCH] fix(plugins): scan all bundle scripts, allow audited scanner override MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two problems with the upload scanner, pulling in opposite directions. It only scanned the entrypoint, so a bundle with `eval()` in a second file passed outright — verified against a synthetic bundle whose vendor/openpgp.js tripped three patterns while index.js stayed clean. At the same time, a hard 400 on eval()/new Function()/innerHTML= makes every crypto plugin uninstallable: minified openpgp.js and pkijs legitimately contain all three. That blocks S/MIME and PGP entirely. Scan every .js/.mjs in the bundle and return structured findings ({file, patterns[]}) plus canOverride, so the admin can see exactly what tripped and where. An explicit overrideWarnings=true proceeds and writes a plugin.install.scan_override audit entry recording which patterns in which files were accepted — not merely that an override happened. This route is already admin-authenticated, so the scan is defence in depth against an accidental or compromised upload, not a trust boundary. Treating it as the latter is what made crypto plugins uninstallable. Also log the B-04 and B-01 divergences in vnc/VNC-CHANGES.md. Co-Authored-By: Claude Opus 4.8 --- app/api/admin/plugins/route.ts | 56 +++++++++++++++++++++++++++++----- vnc/VNC-CHANGES.md | 3 ++ 2 files changed, 52 insertions(+), 7 deletions(-) diff --git a/app/api/admin/plugins/route.ts b/app/api/admin/plugins/route.ts index a62c4bf8..b6ad5232 100644 --- a/app/api/admin/plugins/route.ts +++ b/app/api/admin/plugins/route.ts @@ -158,15 +158,47 @@ export async function POST(request: NextRequest) { } const code = await entryFile.async('string'); - // Security: block plugins containing dangerous JS patterns - const warnings: string[] = []; - for (const { pattern, label } of SUSPICIOUS_JS_PATTERNS) { - if (pattern.test(code)) warnings.push(`Contains ${label}`); - pattern.lastIndex = 0; + // Security: scan for dangerous JS patterns across EVERY script in the + // bundle, not just the entrypoint - a second .js file was previously never + // looked at. + // + // The result is a reviewable finding rather than an unconditional reject. + // Minified crypto libraries (openpgp.js, pkijs) legitimately contain these + // patterns, so a hard block makes S/MIME and PGP plugins uninstallable. + // This route is already admin-authenticated, so the scan is defence in + // depth against an accidental or compromised upload, not a trust boundary: + // an admin may proceed with `overrideWarnings`, and the override is + // recorded in the audit log with the exact findings. + const findings: Array<{ file: string; patterns: string[] }> = []; + for (const [filePath, entry] of Object.entries(zip.files)) { + if (entry.dir) continue; + const ext = filePath.slice(filePath.lastIndexOf('.')).toLowerCase(); + if (ext !== '.js' && ext !== '.mjs') continue; + const source = filePath === root + (manifest.entrypoint as string) + ? code + : await entry.async('string'); + const hits: string[] = []; + for (const { pattern, label } of SUSPICIOUS_JS_PATTERNS) { + if (pattern.test(source)) hits.push(label); + pattern.lastIndex = 0; + } + if (hits.length > 0) { + findings.push({ file: filePath.slice(root.length), patterns: hits }); + } } - if (warnings.length > 0) { + + const overrideWarnings = formData.get('overrideWarnings') === 'true'; + if (findings.length > 0 && !overrideWarnings) { + const summary = findings + .map(f => `${f.file}: ${f.patterns.join(', ')}`) + .join('; '); return NextResponse.json( - { error: `Plugin rejected: ${warnings.join(', ')}. These patterns are not allowed for security reasons.` }, + { + error: `Plugin rejected: ${summary}. Review the bundle; if these are expected ` + + `(e.g. a vendored crypto library), re-upload with "overrideWarnings" to proceed.`, + findings, + canOverride: true, + }, { status: 400 }, ); } @@ -212,6 +244,16 @@ export async function POST(request: NextRequest) { await savePlugin(plugin, code); invalidateFrameOriginsCache(); await auditLog('plugin.install', { id: plugin.id, name: plugin.name, version: plugin.version, frameOrigins: declaredFrameOrigins, httpOrigins: declaredHttpOrigins, apiPostPaths: declaredApiPostPaths }, ip); + if (findings.length > 0) { + // Record WHAT was waved through, not merely that an override happened - + // otherwise the audit trail can't answer "which patterns did we accept?". + await auditLog( + 'plugin.install.scan_override', + { id: plugin.id, version: plugin.version, findings }, + ip, + ); + logger.warn('Plugin installed with scanner override', { id: plugin.id, findings }); + } return NextResponse.json({ plugin }); } catch (error) { diff --git a/vnc/VNC-CHANGES.md b/vnc/VNC-CHANGES.md index d0376d5d..15e34ed5 100644 --- a/vnc/VNC-CHANGES.md +++ b/vnc/VNC-CHANGES.md @@ -47,5 +47,8 @@ microfrontends integration was also added and reverted the same day._ | 2026-08-03 | `lib/stalwart/auth-context.ts` | give `jmap_stalwart_ctx` a 6-hour maxAge (was session-cookie → expired on tab close) | session survival across browser restarts | | 2026-08-03 | `lib/builtin-themes.ts` | add `srcSkin` (MD3 component overrides: shape scale, filled buttons, text fields, cards, dialogs, state layers, switches, login card); add @font-face + typography to `builtin-src`; bump to v1.1.0 | SRC theme: keep colors + fonts, apply MD3 design system | +| 2026-08-04 | `lib/plugin-sandbox/loader.ts` | **B-04 security fix** — gate hook registration on granted permissions via new `HOOK_PERMISSIONS` map; refused hooks are skipped, logged and counted | `info.hooks` is self-reported by the sandbox, so an untrusted plugin could claim `onRenderEmailBody` and replace any rendered email body without holding `email:render-takeover`. Consent copy gated what the user was *asked*, not what the host *allowed*. | +| 2026-08-04 | `lib/plugin-sandbox/host-api.ts` | export `hasPermission()` (was module-private) | one source of truth for the permission rule — the loader gate and the RPC gate must not drift apart | +| 2026-08-04 | `app/api/admin/plugins/route.ts` | **B-01** — scan all `.js`/`.mjs` in the bundle (was entrypoint only); return structured `findings` + `canOverride`; allow admin `overrideWarnings=true` with a `plugin.install.scan_override` audit entry | hard-reject on `eval(`/`new Function(`/`innerHTML =` made every crypto plugin uninstallable (minified openpgp.js/pkijs trip it), while only scanning the entrypoint left a trivial bypass. Route is already admin-authenticated, so the scan is defence-in-depth, not a trust boundary. | _(append new rows as you diverge)_