diff --git a/server/src/nest/plugins/host/rpc-host.ts b/server/src/nest/plugins/host/rpc-host.ts index 4d8b5c33b..df5ce33eb 100644 --- a/server/src/nest/plugins/host/rpc-host.ts +++ b/server/src/nest/plugins/host/rpc-host.ts @@ -82,7 +82,11 @@ export class PluginRpcHost { } async dispatch(req: RpcRequest, actingUserId?: number): Promise { - const raw = (req.params ?? {}) as Record; + // Anything but an object is treated as no params at all. The envelope comes + // off an IPC channel a plugin can write to, and `'_inv' in raw` below throws a + // TypeError on a primitive — outside handle()'s try/catch, so it escapes as a + // rejection rather than an error envelope. + const raw = (typeof req.params === 'object' && req.params !== null ? req.params : {}) as Record; // The supervisor resolves the acting user from `_inv` BEFORE dispatch and passes // the request on untouched, so `_inv` is visible to every handler today and is in // effect a reserved param name. Strip it here: no handler and no audit reads diff --git a/server/src/nest/plugins/install/safe-extract.ts b/server/src/nest/plugins/install/safe-extract.ts index 568f1f79b..dac256435 100644 --- a/server/src/nest/plugins/install/safe-extract.ts +++ b/server/src/nest/plugins/install/safe-extract.ts @@ -83,6 +83,15 @@ function readTarGz(buf: Buffer, lim: Required): Member[] { const prefix = block.subarray(345, 500).toString('utf8').replace(/\0.*$/, ''); if (prefix) name = `${prefix}/${name}`; const size = parseInt(block.subarray(124, 136).toString('utf8').replace(/\0.*$/, '').trim() || '0', 8); + // A negative size walks the cursor backwards: `off += 512` and then + // `off += Math.ceil(size / 512) * 512` cancel out at -512 and go negative + // below it, so the same header is read forever while every pass appends a + // member — a 70-byte archive is enough to hold the main thread and exhaust + // memory. The entry budgets in extractArchive never get a say; they run on + // the list this function has to finish building first. + if (!Number.isSafeInteger(size) || size < 0) { + throw new ExtractError(`invalid entry size in tar header: ${block.subarray(124, 136).toString('utf8').replace(/\0.*$/, '').trim()}`); + } const typeflag = String.fromCharCode(block[156]); off += 512; const data = tar.subarray(off, off + size); diff --git a/server/src/nest/plugins/supervisor/plugin-supervisor.ts b/server/src/nest/plugins/supervisor/plugin-supervisor.ts index 5577b7fb1..3e2e0595e 100644 --- a/server/src/nest/plugins/supervisor/plugin-supervisor.ts +++ b/server/src/nest/plugins/supervisor/plugin-supervisor.ts @@ -474,7 +474,7 @@ export class PluginSupervisor { sup.child = child; sup.lastBeat = Date.now(); - child.on('message', (raw: unknown) => this.onMessage(sup, raw as Envelope)); + child.on('message', (raw: unknown) => this.handleChildMessage(sup, raw)); child.on('exit', (code, signal) => this.onExit(sup, code, signal)); child.on('error', (e) => this.hooks.onLog?.(sup.id, 'error', `child error: ${e.message}`)); child.stdout?.on('data', (b) => this.recordLog(sup, 'info', String(b).trimEnd())); @@ -502,6 +502,19 @@ export class PluginSupervisor { this.hooks.onLog?.(sup.id, level, msg, meta); } + /** + * An EventEmitter listener throws away what its callback returns, and onMessage is + * async — so a rejection in there surfaces as an unhandledRejection, which Node 22 + * turns into process exit. The host installs no net for it (the plugin child does, + * for itself), so one malformed envelope would take down the very supervisor whose + * job is to contain a misbehaving plugin. Log it against the plugin and carry on. + */ + private handleChildMessage(sup: Supervised, raw: unknown): void { + this.onMessage(sup, raw as Envelope).catch((e: unknown) => { + this.hooks.onLog?.(sup.id, 'error', `message handling failed: ${e instanceof Error ? e.message : String(e)}`); + }); + } + private async onMessage(sup: Supervised, msg: Envelope): Promise { if (!msg || typeof msg !== 'object') return; diff --git a/server/tests/unit/plugins/rpc-host-inv-strip.test.ts b/server/tests/unit/plugins/rpc-host-inv-strip.test.ts index 9739dd507..a904b54c2 100644 --- a/server/tests/unit/plugins/rpc-host-inv-strip.test.ts +++ b/server/tests/unit/plugins/rpc-host-inv-strip.test.ts @@ -110,4 +110,29 @@ describe('dispatch strips the supervisor _inv marker', () => { await host.dispatch(req('packing.setBagMembers', { tripId: 1, bagId: 80, userIds: [3], _inv: 'req-7' }), 42); expect(setBagMembers).toHaveBeenCalledWith('1', '80', [3]); }); + + /** + * The `_inv` check runs before handle()'s try/catch, so a params value that is not + * an object used to throw a TypeError out of dispatch instead of coming back as an + * error envelope. The supervisor discards the promise dispatch lives on, which made + * that an unhandledRejection — process exit, from one malformed envelope. + */ + it('RPCINV-007 params that are not an object come back as an envelope, not a rejection', async () => { + const host = new PluginRpcHost('p', new Set(['db:own']), makeDeps(), dbRegistry()); + + for (const params of [5, 'text', true] as unknown as Record[]) { + const res = await host.dispatch({ k: 'req', id: 'x', method: 'db.query', params }); + expect(res).toMatchObject({ k: 'res', id: 'x', ok: false }); + } + }); + + it('RPCINV-008 null and undefined params still behave as an empty object', async () => { + const host = new PluginRpcHost('p', new Set(['db:own']), makeDeps(), dbRegistry()); + + const nulled = await host.dispatch({ k: 'req', id: 'x', method: 'db.query', params: null as unknown as Record }); + // The type says params is required; the IPC channel does not, so a child can + // leave it off entirely. + const missing = await host.dispatch({ k: 'req', id: 'x', method: 'db.query', params: undefined as unknown as Record }); + expect(nulled).toEqual(missing); + }); }); diff --git a/server/tests/unit/plugins/safe-extract.test.ts b/server/tests/unit/plugins/safe-extract.test.ts index 723d305f8..ac12540c9 100644 --- a/server/tests/unit/plugins/safe-extract.test.ts +++ b/server/tests/unit/plugins/safe-extract.test.ts @@ -11,13 +11,15 @@ import zlib from 'node:zlib'; import { safeJoin, extractArchive, ExtractError } from '../../../src/nest/plugins/install/safe-extract'; // ── tiny archive builders ──────────────────────────────────────────────────── -function tarHeader(name: string, size: number, typeflag = '0', linkname = ''): Buffer { +function tarHeader(name: string, size: number, typeflag = '0', linkname = '', rawSize?: string): Buffer { const h = Buffer.alloc(512, 0); h.write(name, 0); h.write('0000644', 100); h.write('0000000', 108); h.write('0000000', 116); - h.write(size.toString(8).padStart(11, '0'), 124); + // rawSize writes the 12-byte field verbatim, for headers a real tar would never + // produce (negative, non-octal) — the reader has to survive those too. + h.write(rawSize !== undefined ? rawSize.padEnd(11, ' ') : size.toString(8).padStart(11, '0'), 124); h.write('00000000000', 136); h.write(' ', 148); // checksum placeholder = spaces h.write(typeflag, 156); @@ -29,12 +31,12 @@ function tarHeader(name: string, size: number, typeflag = '0', linkname = ''): B h.write(sum.toString(8).padStart(6, '0') + '\0 ', 148); return h; } -function makeTarGz(entries: Array<{ name: string; data?: string; type?: string; link?: string }>): Buffer { +function makeTarGz(entries: Array<{ name: string; data?: string; type?: string; link?: string; rawSize?: string }>): Buffer { const parts: Buffer[] = []; for (const e of entries) { const body = Buffer.from(e.data ?? ''); - parts.push(tarHeader(e.name, e.type === '5' || e.type === '2' ? 0 : body.length, e.type ?? '0', e.link ?? '')); - if (e.type !== '5' && e.type !== '2') { + parts.push(tarHeader(e.name, e.type === '5' || e.type === '2' ? 0 : body.length, e.type ?? '0', e.link ?? '', e.rawSize)); + if (e.rawSize === undefined && e.type !== '5' && e.type !== '2') { parts.push(body); const pad = (512 - (body.length % 512)) % 512; if (pad) parts.push(Buffer.alloc(pad, 0)); @@ -130,4 +132,20 @@ describe('extractArchive', () => { it('rejects an unsupported format', () => { expect(() => extractArchive(Buffer.from('not an archive'), dest)).toThrow(/unsupported/); }); + + // A size field is 12 ASCII bytes of octal, and parseInt honours a leading minus. + // The reader advanced by `512 + Math.ceil(size / 512) * 512`, which is zero at + // -512 and negative below it: the cursor never leaves the first header, and every + // pass appends another member until the heap is gone. Synchronous, on the main + // thread, from a 70-byte upload. + it('refuses a negative entry size instead of looping on it', () => { + const gz = makeTarGz([{ name: 'evil.txt', rawSize: '-1000' }]); + expect(() => extractArchive(gz, dest)).toThrow(ExtractError); + expect(() => extractArchive(gz, dest)).toThrow(/invalid entry size/); + }); + + it('refuses a size field that is not a number at all', () => { + const gz = makeTarGz([{ name: 'evil.txt', rawSize: 'not-octal' }]); + expect(() => extractArchive(gz, dest)).toThrow(/invalid entry size/); + }); }); diff --git a/server/tests/unit/plugins/supervisor-lifecycle.test.ts b/server/tests/unit/plugins/supervisor-lifecycle.test.ts index b52fb7a14..7681aa737 100644 --- a/server/tests/unit/plugins/supervisor-lifecycle.test.ts +++ b/server/tests/unit/plugins/supervisor-lifecycle.test.ts @@ -311,3 +311,39 @@ describe('supervisor throttles plugin log/stderr volume (host-thread DoS guard)' expect(sup.droppedLogs).toBe(0); }); }); + +/** + * The 'message' listener is where a rejection would escape: an EventEmitter discards + * what its callback returns, onMessage is async, and the host has no + * unhandledRejection handler — so Node 22 would end the process. A plugin getting to + * kill the supervisor that contains it defeats the point of the supervisor. + */ +describe('supervisor message handling never escapes as a rejection', () => { + it('logs a failed message against the plugin and keeps running', async () => { + const onLog = vi.fn(); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const s = new PluginSupervisor((() => ({ dispose: vi.fn() })) as any, { onLog }, {}); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (s as any).onMessage = () => Promise.reject(new Error('malformed envelope')); + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + expect(() => (s as any).handleChildMessage({ id: 'p' }, { k: 'req', id: 'x' })).not.toThrow(); + + await new Promise((r) => setTimeout(r, 0)); + expect(onLog).toHaveBeenCalledWith('p', 'error', expect.stringContaining('malformed envelope')); + }); + + it('says nothing when the message is handled normally', async () => { + const onLog = vi.fn(); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const s = new PluginSupervisor((() => ({ dispose: vi.fn() })) as any, { onLog }, {}); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (s as any).onMessage = () => Promise.resolve(); + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (s as any).handleChildMessage({ id: 'p' }, { k: 'evt', topic: 'log' }); + + await new Promise((r) => setTimeout(r, 0)); + expect(onLog).not.toHaveBeenCalled(); + }); +});