diff --git a/client/src/components/Plugins/PluginFrame.tsx b/client/src/components/Plugins/PluginFrame.tsx index 38cdd957e..ab4ae6814 100644 --- a/client/src/components/Plugins/PluginFrame.tsx +++ b/client/src/components/Plugins/PluginFrame.tsx @@ -35,10 +35,14 @@ type Inbound = export default function PluginFrame({ pluginId, tripId = null, className, title }: PluginFrameProps) { const frameRef = useRef(null) - // A sandboxed frame may navigate ITSELF (connect-src can't stop that). If the - // plugin's document navigates away, its window identity still matches our - // iframe, so we'd keep answering trek:invoke/context to the new (possibly - // attacker) origin. Track loads: after the initial document, refuse the bridge. + // A sandboxed frame may navigate ITSELF (connect-src can't stop that), and its + // window identity keeps matching our iframe afterwards. Track loads and refuse + // the bridge once a second document loads. NOTE: this is best-effort — the load + // event fires at end-of-document, so a navigated attacker doc that posts during + // its own load (or holds it open) can still reach the bridge for one exchange. + // The exposure is bounded (only this plugin's own routes + the trek:context + // ids the plugin already had; never the httpOnly cookie); fully closing it + // would require not running plugin client JS at all. const loadsRef = useRef(0) const { locale } = useTranslation() const navigate = useNavigate() diff --git a/plugin-sdk/src/manifest.ts b/plugin-sdk/src/manifest.ts index dd3819711..1ffa3452d 100644 --- a/plugin-sdk/src/manifest.ts +++ b/plugin-sdk/src/manifest.ts @@ -25,9 +25,10 @@ const ID_RE = /^[a-z][a-z0-9-]{2,39}$/; // install loader, so surface it locally too. const RESERVED_IDS = new Set(['registry', 'install', 'rescan']); const SEMVER_RE = /^\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?$/; -// An outbound host: exact FQDN or `*.`-wildcard, always multi-label. No `*`, no -// `*.`, no whole-TLD `*.com`, no spaces (mirrors the server manifest validator). -const HOST_RE = /^(\*\.)?([a-z0-9-]+\.)+[a-z0-9-]+$/i; +// An outbound host: exact hostname (single-label sibling or dotted FQDN) or a +// `*.`-wildcard with a multi-label suffix. No `*`, no `*.`, no whole-TLD `*.com`, +// no spaces (mirrors the server manifest validator). +const HOST_RE = /^(\*\.[a-z0-9-]+(\.[a-z0-9-]+)+|[a-z0-9-]+(\.[a-z0-9-]+)*)$/i; const TYPES = ['integration', 'page', 'widget']; const KNOWN_PERMISSIONS = [ 'db:own', 'db:read:trips', 'db:read:users', 'ws:broadcast:trip', 'ws:broadcast:user', diff --git a/server/src/nest/plugins/host/plugin-data.service.ts b/server/src/nest/plugins/host/plugin-data.service.ts index 1d96ba392..2b56f74db 100644 --- a/server/src/nest/plugins/host/plugin-data.service.ts +++ b/server/src/nest/plugins/host/plugin-data.service.ts @@ -16,7 +16,12 @@ import { pluginDataDir, pluginDbFile } from '../paths'; */ const MAX_SQL_LENGTH = 100_000; -const FORBIDDEN = /\b(ATTACH|DETACH|VACUUM|PRAGMA)\b/i; +// RECURSIVE is the one construct that generates unbounded rows/CPU independent of +// the (capped) data size — a `WITH RECURSIVE …` can spin the synchronous host +// forever even with an empty database, which neither the size quota nor the +// result-row cap can stop (an aggregate over it never yields a first row). Refuse +// it outright; the row/size caps below bound everything else. +const FORBIDDEN = /\b(ATTACH|DETACH|VACUUM|PRAGMA|RECURSIVE)\b/i; // Per-plugin on-disk quota. better-sqlite3 is synchronous and runs in the HOST // process, so an unbounded plugin DB is both a disk-exhaustion DoS on the shared // trek.db volume and (via a huge scan) an event-loop stall. max_page_count caps diff --git a/server/src/nest/plugins/install/discovery.ts b/server/src/nest/plugins/install/discovery.ts index 662929ab9..4cd05af7e 100644 --- a/server/src/nest/plugins/install/discovery.ts +++ b/server/src/nest/plugins/install/discovery.ts @@ -49,8 +49,10 @@ function upsert(db: BetterSqlite3.Database, m: PluginManifest): void { ).run(m.name, m.description ?? null, m.type, m.icon ?? 'Blocks', m.version, m.apiVersion, m.minTrekVersion ?? null, JSON.stringify(m.permissions), JSON.stringify(m.capabilities), m.id); } else { db.prepare( + // granted_permissions '' (empty, not '[]') marks "never consented" so the + // first activation is distinguishable from a plugin consented to zero perms. `INSERT INTO plugins (id, name, description, type, icon, version, api_version, min_trek_version, permissions, capabilities, granted_permissions, status) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, '[]', 'inactive')`, + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, '', 'inactive')`, ).run(m.id, m.name, m.description ?? null, m.type, m.icon ?? 'Blocks', m.version, m.apiVersion, m.minTrekVersion ?? null, JSON.stringify(m.permissions), JSON.stringify(m.capabilities)); } diff --git a/server/src/nest/plugins/install/manifest.ts b/server/src/nest/plugins/install/manifest.ts index 0def2a633..100e6caeb 100644 --- a/server/src/nest/plugins/install/manifest.ts +++ b/server/src/nest/plugins/install/manifest.ts @@ -51,11 +51,12 @@ export interface PluginManifest { } const ID_RE = /^[a-z][a-z0-9-]{2,39}$/; -// An outbound host: an exact FQDN or a `*.`-prefixed wildcard, always with a real +// An outbound host: an exact hostname (single-label like a `redis` sibling +// service, or a dotted FQDN) OR a `*.`-prefixed wildcard that MUST have a real // multi-label suffix. Rejects `*`, `*.`, whole-TLD `*.com`, schemes, and any // embedded space — all of which would otherwise widen egress or inject a CSP // source token when the host is interpolated into connect-src. -const HOST_RE = /^(\*\.)?([a-z0-9-]+\.)+[a-z0-9-]+$/i; +const HOST_RE = /^(\*\.[a-z0-9-]+(\.[a-z0-9-]+)+|[a-z0-9-]+(\.[a-z0-9-]+)*)$/i; // Static path segments under /api/admin/plugins — a plugin id must never shadow them // (id "registry" would collide with GET registry/:id vs :id/errors routing). const RESERVED_IDS = new Set(['registry', 'install', 'rescan']); diff --git a/server/src/nest/plugins/plugin-frame.controller.ts b/server/src/nest/plugins/plugin-frame.controller.ts index 122cee3c1..c4c9fb323 100644 --- a/server/src/nest/plugins/plugin-frame.controller.ts +++ b/server/src/nest/plugins/plugin-frame.controller.ts @@ -72,7 +72,7 @@ export class PluginFrameController { // (a stray space or `*` would inject an extra CSP source token). const outbound = this.runtime .outboundHostsOf(pluginId) - .filter((h) => /^(\*\.)?[a-z0-9-]+(\.[a-z0-9-]+)+$/i.test(h)); + .filter((h) => /^(\*\.[a-z0-9-]+(\.[a-z0-9-]+)+|[a-z0-9-]+(\.[a-z0-9-]+)*)$/i.test(h)); const connect = ["'self'", ...outbound.map((h) => `https://${h}`)].join(' '); return [ "default-src 'none'", diff --git a/server/src/nest/plugins/plugin-runtime.service.ts b/server/src/nest/plugins/plugin-runtime.service.ts index 6aa010a75..a25239432 100644 --- a/server/src/nest/plugins/plugin-runtime.service.ts +++ b/server/src/nest/plugins/plugin-runtime.service.ts @@ -105,7 +105,11 @@ export class PluginRuntimeService implements OnModuleInit, OnModuleDestroy { const declared = parseArray(row.permissions).filter(isKnownPermission); const granted = parseArray(row.granted_permissions); const newGrants = declared.filter((p) => !granted.includes(p)); - if (granted.length > 0 && newGrants.length > 0 && !consentWiden) { + // "Ever consented" is a non-empty granted_permissions string (even '[]' — the + // consent to zero perms). Only the very first activation (marker '') may grant + // the declared set without an explicit consent; any later widening needs one. + const everConsented = !!row.granted_permissions; + if (everConsented && newGrants.length > 0 && !consentWiden) { const newEgress = newGrants.filter((p) => p.startsWith(HTTP_OUTBOUND)).map((p) => p.slice(HTTP_OUTBOUND.length)).filter(Boolean); const newPermissions = newGrants.filter((p) => !p.startsWith(HTTP_OUTBOUND)); throw new PluginConsentRequired(`plugin ${id} requests new permissions; explicit re-consent is required`, newPermissions, newEgress); diff --git a/server/src/nest/plugins/plugins-proxy.controller.ts b/server/src/nest/plugins/plugins-proxy.controller.ts index 70baddbbb..5276c3655 100644 --- a/server/src/nest/plugins/plugins-proxy.controller.ts +++ b/server/src/nest/plugins/plugins-proxy.controller.ts @@ -72,18 +72,32 @@ export class PluginsProxyController { user?.id, )) as { status?: number; headers?: Record; body?: unknown }; - // A plugin may not choose a 3xx (open redirect via the reflected Location); - // clamp the status to a non-redirect range. const status = reply?.status ?? 200; - res.status(status >= 300 && status < 400 ? 502 : status); - for (const [k, v] of Object.entries(reply?.headers ?? {})) { + const headers = reply?.headers ?? {}; + res.setHeader('X-Content-Type-Options', 'nosniff'); + + // A genuine redirect (301/302/303/307/308) may only target a RELATIVE in-app + // path — never an external URL (open redirect / phishing). This still lets an + // OAuth-callback route bounce the user back into the app. 300/304 aren't + // redirects and fall through to the normal path. + if ([301, 302, 303, 307, 308].includes(status)) { + const loc = Object.entries(headers).find(([k]) => k.toLowerCase() === 'location')?.[1]; + if (typeof loc !== 'string' || !/^\/(?!\/)/.test(loc)) { + res.status(502).json({ error: 'Plugin error', detail: 'unsafe redirect target' }); + return; + } + res.status(status).setHeader('Location', loc); + res.end(); + return; + } + + for (const [k, v] of Object.entries(headers)) { if (SAFE_RESPONSE_HEADERS.has(k.toLowerCase())) res.setHeader(k, v); } - // Neutralize the "render plugin HTML at TREK's origin" sandbox-escape: never - // let the response be treated as a document, and never MIME-sniff it. - res.setHeader('X-Content-Type-Options', 'nosniff'); + // Never let a non-redirect reply render as a document at TREK's origin + // (that would run plugin script at our real origin, outside the sandbox). res.setHeader('Content-Disposition', 'attachment'); - res.send(reply?.body ?? ''); + res.status(status).send(reply?.body ?? ''); } catch (e) { res.status(502).json({ error: 'Plugin error', detail: e instanceof Error ? e.message : 'unknown' }); } diff --git a/server/src/nest/plugins/runtime/plugin-host-entry.ts b/server/src/nest/plugins/runtime/plugin-host-entry.ts index ed285e855..21faf89b6 100644 --- a/server/src/nest/plugins/runtime/plugin-host-entry.ts +++ b/server/src/nest/plugins/runtime/plugin-host-entry.ts @@ -278,14 +278,20 @@ function installEgressGuard(egress: string[]): void { // A dgram HOSTNAME target is resolved via the socket's `lookup` before the packet // is sent; force the IP-vetting guardedLookup so a declared name that resolves to // a private/metadata address is refused (the TCP path's rebind backstop, for UDP). - const dgramApi = dgram as unknown as { createSocket: (...a: unknown[]) => unknown }; + const injectLookup = (arg: unknown): { type?: unknown; lookup: unknown } => + typeof arg === 'string' ? { type: arg, lookup: guardedLookup } : { ...(arg as object), lookup: guardedLookup }; + const dgramApi = dgram as unknown as { createSocket: (...a: unknown[]) => unknown; Socket: new (o?: unknown, cb?: unknown) => unknown }; const realCreateSocket = dgramApi.createSocket; dgramApi.createSocket = function (this: unknown, ...args: unknown[]): unknown { - const first = args[0]; - if (first && typeof first === 'object') (first as { lookup?: unknown }).lookup = guardedLookup; - else if (typeof first === 'string') args[0] = { type: first, lookup: guardedLookup }; + args[0] = injectLookup(args[0]); return realCreateSocket.apply(this, args); }; + // Also cover `new dgram.Socket(...)`, which bypasses createSocket entirely. + const RealDgramSocket = dgramApi.Socket; + const GuardedDgramSocket = function (options?: unknown, cb?: unknown): unknown { + return new RealDgramSocket(injectLookup(options), cb); + } as unknown as new (o?: unknown, cb?: unknown) => unknown; + (GuardedDgramSocket as unknown as { prototype: unknown }).prototype = RealDgramSocket.prototype; // Lock the wrapped choke points so a plugin can't restore the originals. const lock = (obj: object, key: string, value: unknown) => { @@ -294,6 +300,8 @@ function installEgressGuard(egress: string[]): void { lock(proto, 'connect', proto.connect); lock(dgramProto, 'send', dgramProto.send); lock(dgramProto, 'connect', dgramProto.connect); + lock(dgramApi as unknown as object, 'createSocket', dgramApi.createSocket); + lock(dgramApi as unknown as object, 'Socket', GuardedDgramSocket); // Gate the dns resolver family: a forward lookup for an undeclared name is a // DNS-tunnel exfiltration channel even when no socket is ever opened. The name diff --git a/server/src/nest/plugins/supervisor/plugin-supervisor.ts b/server/src/nest/plugins/supervisor/plugin-supervisor.ts index 5fedb58b4..e3c6bfac1 100644 --- a/server/src/nest/plugins/supervisor/plugin-supervisor.ts +++ b/server/src/nest/plugins/supervisor/plugin-supervisor.ts @@ -118,14 +118,16 @@ export class PluginSupervisor { sup.activation = { resolve, reject }; // Deadline: if the plugin never reports 'loaded' (stuck onLoad), reject the // activation and kill the child rather than hanging + leaking a busy core. - sup.activationTimer = setTimeout(() => { + sup.activationTimer = setTimeout(async () => { if (sup.status === 'active') return; this.hooks.onLog?.(sup.id, 'error', 'activation timed out; killing'); this.setStatus(sup, 'error', 'activation timed out'); sup.activation?.reject(new Error('plugin did not finish loading in time')); sup.activation = undefined; this.running.delete(sup.id); - void this.kill(sup); + // await the kill (SIGTERM grace) before closing the plugin db, so a ctx.* + // RPC from the dying child can't hit an already-disposed handle. + await this.kill(sup); sup.rpcHost.dispose(); }, this.tuning.activationTimeoutMs); sup.activationTimer.unref?.(); @@ -382,8 +384,10 @@ export class PluginSupervisor { const pid = sup.child?.pid; if (pid) { try { - const resident = Number(fs.readFileSync(`/proc/${pid}/statm`, 'utf8').split(' ')[1]); - if (Number.isFinite(resident)) return resident * 4096; // pages → bytes + // VmRSS is reported in kB and is page-size-independent (statm pages would + // undercount on 16K/64K-page kernels, loosening the cap). + const m = fs.readFileSync(`/proc/${pid}/status`, 'utf8').match(/^VmRSS:\s+(\d+)\s+kB/m); + if (m) return Number(m[1]) * 1024; } catch { /* not Linux / process gone — fall back */ } diff --git a/server/tests/integration/plugins/plugin-runtime.test.ts b/server/tests/integration/plugins/plugin-runtime.test.ts index aa472fed8..88d3b1d4e 100644 --- a/server/tests/integration/plugins/plugin-runtime.test.ts +++ b/server/tests/integration/plugins/plugin-runtime.test.ts @@ -12,7 +12,7 @@ const { testDb } = vi.hoisted(() => { const Database = require('better-sqlite3'); const db = new Database(':memory:'); db.exec(`CREATE TABLE plugins ( - id TEXT PRIMARY KEY, status TEXT, enabled INTEGER DEFAULT 0, permissions TEXT DEFAULT '[]', granted_permissions TEXT DEFAULT '[]', + id TEXT PRIMARY KEY, status TEXT, enabled INTEGER DEFAULT 0, permissions TEXT DEFAULT '[]', granted_permissions TEXT DEFAULT '', config TEXT DEFAULT '{}', last_error TEXT, updated_at TEXT); CREATE TABLE plugin_error_log (id INTEGER PRIMARY KEY AUTOINCREMENT, plugin_id TEXT, level TEXT, message TEXT, ts TEXT); CREATE TABLE plugin_settings_fields (plugin_id TEXT, field_key TEXT, scope TEXT, secret INTEGER); @@ -109,6 +109,18 @@ describe('PluginRuntimeService (M2 end-to-end)', () => { await expect(runtime.activate('ghost')).rejects.toThrow(/not found/); }); + it('re-consent gate: activating a consented plugin with WIDER permissions is refused without consent', async () => { + // consented to db:own; a version now declaring db:read:users too must not auto-grant + testDb.prepare("INSERT INTO plugins (id, status, permissions, granted_permissions, config) VALUES ('widener','inactive','[\"db:own\",\"db:read:users\"]','[\"db:own\"]','{}')").run(); + await expect(runtime.activate('widener')).rejects.toThrow(/re-consent|new permissions/); + // the key case: a plugin consented to ZERO perms ('[]') is still "consented" — + // a later widening to db:own is blocked, not silently granted. + testDb.prepare("INSERT INTO plugins (id, status, permissions, granted_permissions, config) VALUES ('zeroperm','inactive','[\"db:own\"]','[]','{}')").run(); + await expect(runtime.activate('zeroperm')).rejects.toThrow(/re-consent|new permissions/); + // (a NEVER-consented plugin — granted '' — consenting to its declared set on + // first activate is covered by the 'counter' happy-path test above.) + }); + it('onModuleInit is a no-op when the runtime is disabled', () => { process.env.TREK_PLUGINS_ENABLED = 'false'; expect(() => new PluginRuntimeService().onModuleInit()).not.toThrow(); diff --git a/server/tests/unit/plugins/manifest.test.ts b/server/tests/unit/plugins/manifest.test.ts index aa58e6e38..3003f31cc 100644 --- a/server/tests/unit/plugins/manifest.test.ts +++ b/server/tests/unit/plugins/manifest.test.ts @@ -32,9 +32,10 @@ describe('parseManifest', () => { expect(m.settings[0]).toMatchObject({ key: 'api_key', secret: true, scope: 'instance' }); }); - it('accepts a valid exact + wildcard outbound host', () => { - const m = parseManifest({ ...base, permissions: ['http:outbound:api.x.com', 'http:outbound:*.example.com'], egress: ['api.x.com', '*.example.com'] }); + it('accepts exact, single-label (self-hoster sibling), and wildcard outbound hosts', () => { + const m = parseManifest({ ...base, permissions: ['http:outbound:api.x.com', 'http:outbound:*.example.com', 'http:outbound:redis'], egress: ['api.x.com', '*.example.com', 'redis'] }); expect(m.permissions).toContain('http:outbound:*.example.com'); + expect(m.permissions).toContain('http:outbound:redis'); }); it.each([ diff --git a/server/tests/unit/plugins/plugin-data.test.ts b/server/tests/unit/plugins/plugin-data.test.ts index 21927a2a4..6135b83f4 100644 --- a/server/tests/unit/plugins/plugin-data.test.ts +++ b/server/tests/unit/plugins/plugin-data.test.ts @@ -36,15 +36,29 @@ describe('PluginDataDb', () => { expect(fs.existsSync(path.join(tmp, 'notes', 'plugin.db'))).toBe(true); }); - it('rejects statements that would escape the plugin file, and bad sql', () => { + it('rejects statements that would escape the plugin file, DoS, or exceed limits', () => { const db = new PluginDataDb('guard'); expect(() => db.exec("ATTACH DATABASE 'trek.db' AS core")).toThrow(/not allowed/); expect(() => db.query('PRAGMA table_info(x)')).toThrow(/not allowed/); + // WITH RECURSIVE is the unbounded-CPU vector on the synchronous host — refused + expect(() => db.query('WITH RECURSIVE r(x) AS (SELECT 1 UNION ALL SELECT x+1 FROM r) SELECT x FROM r')).toThrow(/not allowed/); + expect(() => db.exec('WITH RECURSIVE r(x) AS (SELECT 1 UNION ALL SELECT x+1 FROM r) INSERT INTO t SELECT x FROM r')).toThrow(/not allowed/); expect(() => db.exec(123 as unknown as string)).toThrow(/must be a string/); expect(() => db.query('x'.repeat(100_001))).toThrow(/too long/); db.close(); }); + it('row-caps a query so it cannot materialize an unbounded result set', () => { + const db = new PluginDataDb('rowcap'); + db.exec('CREATE TABLE seq (n INTEGER)'); + // Insert a modest table and a self-cross-join that would explode past the cap. + const insert = 'INSERT INTO seq (n) VALUES ' + Array.from({ length: 400 }, (_, i) => `(${i})`).join(','); + db.exec(insert); + // 400 * 400 = 160k rows > MAX_ROWS (100k) → must throw, not return them all + expect(() => db.query('SELECT a.n FROM seq a, seq b')).toThrow(/more than/); + db.close(); + }); + it('removePluginData deletes the whole data dir', () => { const db = new PluginDataDb('temp'); db.migrate('001', 'CREATE TABLE t (id INTEGER)');