mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-18 21:16:47 +00:00
fix(plugins): close the 4 PARTIAL findings + regressions from the fix-verify pass
A second adversarial pass over the first remediation found four findings only partially closed and five issues the fixes themselves introduced. This closes them: Partial → closed - re-consent gate keyed on `granted.length > 0`, so a plugin first activated with ZERO permissions (granted '[]') was treated as never-consented and a later widening was granted silently. Now discovery marks a never-consented plugin with granted_permissions '' and activate() gates on "ever consented" (any non-empty string, including '[]'). - db:own DoS: block WITH RECURSIVE outright (the one construct that spins the synchronous host unboundedly regardless of the size/row caps, via query OR exec). - dgram: also wrap `new dgram.Socket(...)` (bypassed createSocket) to inject the IP-vetting lookup, and lock createSocket/Socket. - frame self-navigation: documented as a bounded best-effort mitigation (inherent to sandboxed iframes; exposure is the plugin's own routes + already-held context, never the httpOnly cookie). Regressions introduced by the first pass → fixed - proxy: only real redirects (301/302/303/307/308) are gated, to a RELATIVE in-app Location (supports OAuth-callback bounce, blocks open redirect); 300/304 pass through; attachment only on non-redirects. - supervisor: measure RSS via /proc/<pid>/status VmRSS (page-size independent); activation-timeout awaits kill() before disposing the db handle. - manifest HOST_RE: allow single-label hosts (self-hoster sibling services) while keeping wildcards multi-label; mirrored in the SDK + frame CSP filter. Regression tests added (re-consent incl. the '[]' case, WITH RECURSIVE + row cap, single-label host). 236 plugin tests + 24 SDK tests green.
This commit is contained in:
Maurice
@@ -35,10 +35,14 @@ type Inbound =
|
||||
|
||||
export default function PluginFrame({ pluginId, tripId = null, className, title }: PluginFrameProps) {
|
||||
const frameRef = useRef<HTMLIFrameElement | null>(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()
|
||||
|
||||
@@ -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',
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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));
|
||||
}
|
||||
|
||||
|
||||
@@ -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']);
|
||||
|
||||
@@ -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'",
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -72,18 +72,32 @@ export class PluginsProxyController {
|
||||
user?.id,
|
||||
)) as { status?: number; headers?: Record<string, string>; 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' });
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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 */
|
||||
}
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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([
|
||||
|
||||
@@ -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)');
|
||||
|
||||
Reference in New Issue
Block a user