diff --git a/README.md b/README.md index 0a5e1e8..b6f68a2 100644 --- a/README.md +++ b/README.md @@ -118,6 +118,8 @@ client.login(process.env.DISCORD_TOKEN) With `catchProcessErrors` (on by default), djsk installs its own `uncaughtException`/`unhandledRejection` listeners for the life of the process, logging instead of crashing. This is process-wide, not scoped to djsk's own commands — set it to `false` if you already install your own top-level handlers (a process manager, crash reporter, ...) and don't want djsk's to shadow them. +If you construct more than one `Jishaku` in the same process (a hot-reload path, a test suite, ...), call `jsk.destroy()` before dropping your reference to an old instance — `process` is a shared `EventEmitter` with no way to tell "this listener's owner was discarded," so without `destroy()` each instance's listeners (and everything they close over) stay registered for the rest of the process's life. + ### Security mode The Discord token is **always** redacted from djsk's own output. Setting `security: true` additionally best-effort redacts, from everything djsk sends, replies with, edits, or logs — including `jsk js`/`jsk cjs`/`jsk mjs` results, `jsk cat` / `jsk curl` output (message and file attachments), and shell output: @@ -216,18 +218,21 @@ The following variables are injected into the evaluation scope of all three: `jsk js` and `jsk cjs` run eval'd code via `vm.Script#runInThisContext()` rather than a plain function, in the *current* realm — Node's ambient globals and live object references (client, message, ...) work exactly as if it were a plain function, but bare `import(...)` doesn't (it needs `--experimental-vm-modules`, which not every djsk consumer's process runs with). Use the injected `dynamicImport(specifier)` instead — it's a normal function defined outside the vm boundary, so it isn't affected by that restriction: `const os = await dynamicImport('node:os')`. `jsk cjs` additionally gets a real `require()`, resolved against `evalModuleDir` (default `process.cwd()`) — so `require('discord.js')`, `require('./some-local-file')`, etc. resolve against *your bot project*, not djsk's own. -**`jsk mjs` is different.** Static `import` syntax can't appear inside a wrapped function body at all (an ECMAScript rule, not a `vm.Script` limitation), so `jsk mjs` instead runs your code as the top level of a real, freshly-loaded ES module — real `import`, real top-level `await`. Two consequences: +**`jsk mjs` is different.** Static `import` syntax can't appear inside a wrapped function body at all (an ECMAScript rule, not a `vm.Script` limitation), so `jsk mjs` instead runs your code as the top level of a real, freshly-loaded ES module — real `import`, real top-level `await`. Consequences: - There's no `return` — a module's top level has no return value. Use `export default ` to produce a result instead (e.g. `export default 1 + 1;`). - It writes a transient `.mjs` file under `/.djsk-tmp` for the duration of the eval (deleted immediately after; a `.gitignore` is dropped in that folder so it never pollutes your repo). This is required for real npm package imports to resolve — dynamically `import()`-ing a `data:` URL works for `node:` builtins but can't resolve real packages (no filesystem location for Node to walk up node_modules from), so a real file is the only way to make `import 'some-package'` actually work. - It does **not** get `evalTimeout`'s synchronous-runaway protection (see below) — a bare `while (true) {}` in `jsk mjs` blocks the whole process with no recovery short of a restart, since that protection is a `vm.Script` feature `jsk mjs` doesn't use. `jsk cancel` still works for an eval stuck *awaiting* something. +- Because Node's ESM loader has no API to evict a module once imported, and `jsk mjs` needs a fresh module per eval (see the temp-file point above), every `jsk mjs` call permanently grows the process's module cache by one entry. This is a slow, inherent memory cost of using the command at all, not just on error — negligible for occasional use, worth knowing about if you script very frequent `jsk mjs` calls in a long-running process. + +A real static `import 'node:child_process'` still gets the same `execSync`/`execFileSync`/`spawnSync` default-timeout protection described below, even though — unlike `dynamicImport`/`require` — it resolves through Node's own loader with no per-call interception point: `jsk mjs` temporarily patches the real, shared `child_process` module itself for the duration of the import, then restores it. **Cancelling a running eval.** All three register themselves in `jsk tasks`, and are cancellable two ways: - `jsk cancel` — stops an eval stuck *awaiting* something (an infinite retry loop with an `await` in it, a Discord call that never resolves, `await new Promise(() => {})`, ...). `signal` is provided so eval'd code can cooperate explicitly too — pass it to anything that accepts an `AbortSignal` (`fetch(url, { signal })`) or poll `signal.aborted` inside a loop. - `evalTimeout` — a hard cap (ms, default `10000`) on any single *synchronous* stretch of a `jsk js`/`jsk cjs` eval, e.g. a bare `while (true) {}`. This case can't be helped by `jsk cancel`: while the eval is stuck in synchronous code, the entire bot process is blocked and can't process *any* Discord events, including a cancel request — so it's enforced automatically instead (via V8's execution watchdog, which can genuinely preempt a tight loop), terminating the eval once it's exceeded. Not available for `jsk mjs` — see above. -Between the two, a `jsk js`/`jsk cjs` eval can (almost) always be recovered from without restarting the bot. `evalTimeout` only preempts synchronous *JS* execution, not time spent parked in a blocking *native* call (`child_process.execSync` on a slow command, say) — for `execSync`/`execFileSync`/`spawnSync` specifically (reached via `dynamicImport('node:child_process')`, since bare `import(...)` isn't available — see above), a call that doesn't set its own `timeout` gets `evalTimeout` as one automatically, since those three already support it natively (killing the child and unblocking the parent). Other blocking natives with no such option (`fs.readFileSync` hung on a slow pipe, a bare `Atomics.wait()`, ...) remain a real, if rarer, gap that still needs a restart. +Between the two, a `jsk js`/`jsk cjs`/`jsk mjs` eval can (almost) always be recovered from without restarting the bot. `evalTimeout` only preempts synchronous *JS* execution, not time spent parked in a blocking *native* call (`child_process.execSync` on a slow command, say) — for `execSync`/`execFileSync`/`spawnSync` specifically, a call that doesn't set its own `timeout` gets `evalTimeout` as one automatically, since those three already support it natively (killing the child and unblocking the parent). This applies to `node:child_process` reached any of the three ways djsk supports: `dynamicImport('node:child_process')` (needed for `jsk js`, since bare `import(...)` isn't available there), `jsk cjs`'s `require('node:child_process')`, and `jsk mjs`'s static `import 'node:child_process'` (see above for how). Other blocking natives with no `timeout` option (`fs.readFileSync` hung on a slow pipe, a bare `Atomics.wait()`, ...) remain a real, if rarer, gap that still needs a restart. > [!Note] > diff --git a/package.json b/package.json index be5f0e8..3bf28dd 100644 --- a/package.json +++ b/package.json @@ -57,7 +57,7 @@ "prepublishOnly": "pnpm build" }, "engines": { - "node": ">=18" + "node": ">=22" }, "publishConfig": { "access": "public", diff --git a/src/commands/cjs.test.ts b/src/commands/cjs.test.ts index 5591d94..533a09f 100644 --- a/src/commands/cjs.test.ts +++ b/src/commands/cjs.test.ts @@ -69,3 +69,65 @@ describe('jsk cjs — require', () => { } }) }) + +describe('jsk cjs — blocking child_process calls via require', () => { + it('kills an execSync call with no explicit timeout after evalTimeout, same as dynamicImport in jsk js', async () => { + const jsk = makeJsk({ evalTimeout: 300 }) + const { ctx, react, send } = makeContext( + // `await Promise.resolve()` closes out the vm.Script's initial synchronous stretch (the + // only part `evalTimeout`'s watchdog actually times) before the blocking execSync call — + // otherwise, since `require` (unlike `dynamicImport`) has no `await` of its own, the + // watchdog would race execSync's own injected timeout and could trip first, which is a + // separate, pre-existing characteristic of any fully-synchronous eval and not what this + // test is about (see js.test.ts's equivalent dynamicImport-based test, which gets the + // same effect for free from `await dynamicImport(...)`). + `const cp = require("node:child_process") + await Promise.resolve() + try { + cp.execSync(${JSON.stringify(process.execPath)} + ' -e "setTimeout(()=>{}, 3000)"') + return 'ran to completion' + } catch (e) { + return e.code + }`, + jsk, + ) + + const start = Date.now() + await cjsCommand.handler(ctx) + const elapsed = Date.now() - start + + // Killed by the injected default timeout (~300ms), not left to run the full 3s — proves + // require('child_process') is guarded the same way dynamicImport('node:child_process') is, + // not just passed through as the raw, unwrapped module. + expect(elapsed).toBeLessThan(2000) + expect(react).toHaveBeenCalledWith('✅') + const [payload] = send.mock.calls[0] as [{ content: string }] + expect(payload.content).toBe('ETIMEDOUT') + }, 10_000) + + it('does not mutate the real child_process module (only the require() result is wrapped)', async () => { + const childProcess = await import('node:child_process') + const originalExecSync = childProcess.execSync + const jsk = makeJsk({ evalTimeout: 5000 }) + const { ctx } = makeContext('return typeof require("node:child_process").execSync', jsk) + + await cjsCommand.handler(ctx) + + expect(childProcess.execSync).toBe(originalExecSync) + }) + + it('preserves require.resolve/cache/main/extensions on the guarded require', async () => { + const { ctx, send } = makeContext( + `return [ + typeof require.resolve, + typeof require.cache, + typeof require.extensions, + ].join(',')`, + ) + + await cjsCommand.handler(ctx) + + const [payload] = send.mock.calls[0] as [{ content: string }] + expect(payload.content).toBe('function,object,object') + }) +}) diff --git a/src/commands/cjs.ts b/src/commands/cjs.ts index 06d8bf7..27757ce 100644 --- a/src/commands/cjs.ts +++ b/src/commands/cjs.ts @@ -1,6 +1,6 @@ import { createRequire } from 'node:module' import path from 'node:path' -import { runVmEval } from './eval-shared' +import { createGuardedRequire, runVmEval } from './eval-shared' import type { Command } from './registry' const cjsCommand: Command = { @@ -12,7 +12,12 @@ const cjsCommand: Command = { // exist. Anchored at `evalModuleDir` (default `process.cwd()`) rather than djsk's own // package directory, so `require(...)` resolves the *host bot's* node_modules and files, // not djsk's. - const require = createRequire(path.join(ctx.jsk.config.evalModuleDir, 'jsk-eval-shim.cjs')) + const realRequire = createRequire(path.join(ctx.jsk.config.evalModuleDir, 'jsk-eval-shim.cjs')) + // Guarded (not the raw `require`) so `require('child_process')` also gets the same + // execSync/execFileSync/spawnSync default-timeout protection `dynamicImport` gives — + // otherwise `require`, being the natural way `jsk cjs` code reaches for child_process, + // would silently bypass it. See `createGuardedRequire`'s doc comment in eval-shared.ts. + const require = createGuardedRequire(realRequire, ctx.jsk.config.evalTimeout) await runVmEval(ctx, ctx.codeblock.content, { require }, 'jsk cjs') }, } diff --git a/src/commands/eval-shared.ts b/src/commands/eval-shared.ts index 93b8eba..a1d9e44 100644 --- a/src/commands/eval-shared.ts +++ b/src/commands/eval-shared.ts @@ -1,3 +1,4 @@ +import { createRequire } from 'node:module' import vm from 'node:vm' import type { Context } from '../context' import { installPrototypeGuards, installRestGuard } from '../prototype-guard' @@ -36,6 +37,29 @@ function withDefaultTimeout(fn: (...args: any[]) => any, timeoutMs: number) { } } +/** + * Proxy-wraps a `node:child_process` module object so its execSync/execFileSync/spawnSync + * default to `evalTimeoutMs` (see {@link withDefaultTimeout}) — the shared interception point + * for both {@link createDynamicImport} and {@link createGuardedRequire}. Not a global + * monkeypatch of the module: mutating the module object reached via a default import (`import + * cp from 'node:child_process'`) does NOT affect what a named/namespace import or a fresh + * `require('node:child_process')` sees, confirmed experimentally — Node hands out the same + * live module object for `require`, but ESM bindings for built-ins aren't reliably live across + * that boundary, so every consumer of this module needs its own wrap at its own entry point + * rather than one shared patch. + */ +function guardChildProcessModule(module: T, evalTimeoutMs: number): T { + return new Proxy(module, { + get(target, prop, receiver) { + const value = Reflect.get(target, prop, receiver) + return typeof prop === 'string' && GUARDED_CHILD_PROCESS_METHODS.has(prop) + ? // biome-ignore lint/suspicious/noExplicitAny: `T` is an opaque module shape here; the actual signature is forwarded verbatim by withDefaultTimeout. + withDefaultTimeout(value as (...args: any[]) => any, evalTimeoutMs) + : value + }, + }) +} + /** * Builds the `dynamicImport` scope entry shared by every eval flavor. * @@ -45,27 +69,89 @@ function withDefaultTimeout(fn: (...args: any[]) => any, timeoutMs: number) { * vm-executed code (in normal module scope), so it can freely `import()` without that * restriction; eval'd code gets the same capability via `dynamicImport(...)`. * - * `node:child_process` specifically comes back Proxy-wrapped so execSync/execFileSync/ - * spawnSync default to `evalTimeoutMs` (see {@link withDefaultTimeout}) — this is the actual - * interception point, not a global monkeypatch of the module: mutating the module object - * reached via a default import (`import cp from 'node:child_process'`) does NOT affect what a - * named or namespace import (what `dynamicImport` returns) sees, confirmed experimentally — - * Node's synthetic ESM bindings for built-ins aren't reliably live across that boundary, so - * patching has to happen right here instead. + * `node:child_process` specifically comes back guarded via {@link guardChildProcessModule} — see + * its doc comment for why this needs its own wrap rather than relying on a shared module patch. */ export function createDynamicImport(evalTimeoutMs: number) { return async (specifier: string) => { const imported = await import(specifier) - if (specifier !== 'node:child_process' && specifier !== 'child_process') return imported - - return new Proxy(imported, { - get(target, prop, receiver) { - const value = Reflect.get(target, prop, receiver) - return typeof prop === 'string' && GUARDED_CHILD_PROCESS_METHODS.has(prop) - ? withDefaultTimeout(value, evalTimeoutMs) - : value - }, - }) + return specifier === 'node:child_process' || specifier === 'child_process' + ? guardChildProcessModule(imported, evalTimeoutMs) + : imported + } +} + +/** + * Wraps a real `require` (from `node:module`'s `createRequire`, used by `jsk cjs`) so + * `require('child_process')`/`require('node:child_process')` also comes back guarded via + * {@link guardChildProcessModule} — without this, `require` is a direct, unwrapped path to + * Node's real child_process module that completely bypasses the timeout `dynamicImport` (see + * above) defaults onto execSync/execFileSync/spawnSync, since `jsk cjs` code has no reason to + * reach for `dynamicImport` when it already has a working `require`. + * + * `Object.assign`s the real `require`'s own properties (`resolve`, `cache`, `main`, + * `extensions`) onto the wrapper, so code that relies on those (`require.resolve(...)`, etc.) + * keeps working exactly as if it had the unwrapped `require`. + */ +export function createGuardedRequire( + realRequire: NodeJS.Require, + evalTimeoutMs: number, +): NodeJS.Require { + const guarded = ((specifier: string) => { + const resolved = realRequire(specifier) + return specifier === 'node:child_process' || specifier === 'child_process' + ? guardChildProcessModule(resolved, evalTimeoutMs) + : resolved + }) as NodeJS.Require + return Object.assign(guarded, realRequire) +} + +// Anchor path is arbitrary — only ever used to `require('node:child_process')`, a builtin that +// resolves without touching the filesystem, so it doesn't matter that this path may not exist. +const moduleRequire = createRequire(process.cwd()) + +/** + * Temporarily patches the real, shared `node:child_process` module's execSync/execFileSync/ + * spawnSync with their `evalTimeoutMs`-guarded versions (see {@link withDefaultTimeout}), + * returning a function that restores the originals. + * + * Exists for `jsk mjs` specifically: `dynamicImport(...)` (used by `jsk js`/`jsk cjs`, see + * {@link createDynamicImport}) and `createGuardedRequire` (used by `jsk cjs`'s `require`) each + * guard only what *they themselves* return, which works because eval'd code has to go through + * one of those two functions to reach `node:child_process` in the first place. `jsk mjs`'s + * generated module can instead reach it via a real static `import 'node:child_process'`, which + * goes straight through Node's own loader with no per-call interception point available — + * patching the shared module directly, before that generated module is ever imported, is the + * only way to still guard it there. + * + * Confirmed experimentally (on both Node 22 and Node 24) that this — unlike mutating an + * *already-obtained* module/namespace object after the fact, which is a different, negative + * case (see {@link guardChildProcessModule}'s doc comment) — IS observed by a + * subsequently-loaded module's `import`, whether default, named, or namespace: Node's synthetic + * ESM bindings for `node:child_process` evidently bind live to the shared CommonJS + * `module.exports` object (what `require('node:child_process')` returns), not a snapshot, as + * long as the patch is in place before that module is first loaded. + * + * Global and single-owner, like the rest of djsk's eval guarding (see + * `captureTerminalOutput`'s doc comment) — two concurrent `jsk mjs` evals installing/restoring + * this at the same time would race and could hand one eval the other's timeout, an accepted + * trade-off for a bot-owner debug tool rather than a general-purpose sandbox. + */ +export function installChildProcessTimeoutGuard(evalTimeoutMs: number): () => void { + // biome-ignore lint/suspicious/noExplicitAny: mutating an opaque, dynamically-`require`d module object. + const cp = moduleRequire('node:child_process') as Record any> + const originals = new Map unknown>() + + for (const method of GUARDED_CHILD_PROCESS_METHODS) { + const original = cp[method] + originals.set(method, original) + cp[method] = withDefaultTimeout(original, evalTimeoutMs) + } + + return () => { + for (const [method, original] of originals) { + cp[method] = original + } } } @@ -392,17 +478,25 @@ export async function runVmEval( // registering the task first guarantees it's visible in `jsk tasks`/cancellable via // `jsk cancel` immediately, without an extra tick's delay whenever security mode is off. const task = jsk.submitTask(taskName, () => controller.abort()) - const { restoreGuards, restoreRestGuard } = await installSecurityGuards(ctx) // Unique per invocation (task.index is a monotonic counter) so concurrent evals can't // clobber each other's stashed arguments on the shared global object. const argsKey = `__djsk_eval_args_${task.index}__` - // In security mode, also scrub what actually reaches the real terminal (not just the copy - // captured for Discord) — otherwise a `console.log(client.token)` still leaks it into the - // bot's own local logs, which may be shipped to a third-party service the operator doesn't - // fully trust. Off by default (matches the general "token redaction is always on, full - // scrubbing is opt-in" convention) so normal debugging output isn't silently altered. - const capture = captureTerminalOutput(jsk.config.security ? (text) => jsk.scrub(text) : null) + // Declared here (rather than as `const` from a destructured `await` above) and populated + // inside the `try` below, so a throw from `installSecurityGuards`/`captureTerminalOutput` + // itself still reaches the `finally` — otherwise such a throw would skip `jsk.removeTask` + // entirely, leaking `task` as a permanent ghost entry in `jsk tasks`. + let restoreGuards: (() => void) | null = null + let restoreRestGuard: (() => void) | null = null + let capture: ReturnType | null = null try { + ;({ restoreGuards, restoreRestGuard } = await installSecurityGuards(ctx)) + // In security mode, also scrub what actually reaches the real terminal (not just the copy + // captured for Discord) — otherwise a `console.log(client.token)` still leaks it into the + // bot's own local logs, which may be shipped to a third-party service the operator doesn't + // fully trust. Off by default (matches the general "token redaction is always on, full + // scrubbing is opt-in" convention) so normal debugging output isn't silently altered. + capture = captureTerminalOutput(jsk.config.security ? (text) => jsk.scrub(text) : null) + // biome-ignore lint/suspicious/noExplicitAny: temporary bridge for vm.runInThisContext, deleted immediately below. ;(globalThis as any)[argsKey] = argValues @@ -431,7 +525,9 @@ export async function runVmEval( await ctx.react('✅') await sendResult(ctx, result, terminalOutput) } catch (error) { - const terminalOutput = capture.restore() + // `capture` can still be `null` here if the throw came from `installSecurityGuards` itself, + // before terminal output capture was even installed. + const terminalOutput = capture?.restore() ?? '' if (error instanceof EvalCancelledError) { // `jsk cancel` already sends its own confirmation — report only whatever terminal @@ -450,7 +546,7 @@ export async function runVmEval( throw error } finally { - capture.restore() + capture?.restore() restoreRestGuard?.() restoreGuards?.() jsk.removeTask(task) diff --git a/src/commands/mjs.test.ts b/src/commands/mjs.test.ts index 24c0d5b..0245674 100644 --- a/src/commands/mjs.test.ts +++ b/src/commands/mjs.test.ts @@ -111,4 +111,49 @@ describe('jsk mjs', () => { const tmpDir = path.join(projectRoot, '.djsk-tmp') expect(readdirSync(tmpDir)).toEqual(['.gitignore']) }) + + describe('blocking child_process calls via a real static import', () => { + it('kills an execSync call with no explicit timeout after evalTimeout', async () => { + const timedJsk = makeJsk({ evalModuleDir: projectRoot, evalTimeout: 300 }) + const { ctx, react, send } = makeContext( + `import cp from "node:child_process"; + let result; + try { + cp.execSync(${JSON.stringify(process.execPath)} + ' -e "setTimeout(()=>{}, 3000)"'); + result = 'ran to completion'; + } catch (e) { + result = e.code; + } + export default result;`, + timedJsk, + ) + + const start = Date.now() + await mjsCommand.handler(ctx) + const elapsed = Date.now() - start + + // Killed by the injected default timeout (~300ms), not left to run the full 3s — proves + // a real static `import cp from "node:child_process"` is guarded too, not just + // dynamicImport (jsk js) / require (jsk cjs). + expect(elapsed).toBeLessThan(2000) + expect(react).toHaveBeenCalledWith('✅') + const [payload] = send.mock.calls[0] as [{ content: string }] + expect(payload.content).toBe('ETIMEDOUT') + }, 10_000) + + it('restores the real, shared child_process module once the eval finishes', async () => { + const childProcess = await import('node:child_process') + const originalExecSync = childProcess.execSync + const { ctx, send } = makeContext( + 'import cp from "node:child_process";\nexport default typeof cp.execSync;', + jsk, + ) + + await mjsCommand.handler(ctx) + + const [payload] = send.mock.calls[0] as [{ content: string }] + expect(payload.content).toBe('function') + expect(childProcess.execSync).toBe(originalExecSync) + }) + }) }) diff --git a/src/commands/mjs.ts b/src/commands/mjs.ts index e574274..ab09271 100644 --- a/src/commands/mjs.ts +++ b/src/commands/mjs.ts @@ -5,6 +5,7 @@ import { buildBaseScope, captureTerminalOutput, EvalCancelledError, + installChildProcessTimeoutGuard, installSecurityGuards, makeGuard, raceAbort, @@ -60,6 +61,22 @@ const mjsCommand: Command = { * of `EvalTimedOutError`'s synchronous-runaway protection here — a bare `while (true) {}` in * `jsk mjs` blocks the whole process with no recovery short of a restart. `jsk cancel` (via * `raceAbort`) still works for an eval stuck *awaiting* something, same as `js`/`cjs`. + * + * A real static `import 'node:child_process'` here resolves through Node's actual module + * loader, bypassing `dynamicImport(...)`/`require(...)`'s own per-call guarding (see + * `createDynamicImport`/`createGuardedRequire` in `eval-shared.ts`) entirely — so this + * additionally installs `installChildProcessTimeoutGuard` around the temp module's import, + * which patches the real, shared `node:child_process` module just for that window instead. + * See that function's doc comment for why this (unlike mutating an already-obtained module + * object) actually reaches a subsequent static import. + * + * Trade-off, not fixed: the cache-busting query string below (needed since Node's ESM loader + * has no API to evict a module once imported) means every `jsk mjs` call permanently grows + * Node's module cache by one entry — a slow memory leak from ordinary (non-error) use of this + * command over a long-running process's lifetime. Not fixable without either giving up real + * top-level `import`/`await` support (this command's entire purpose) or requiring consumers + * run with `--experimental-vm-modules` (see the `data:` URL trade-off above for why a + * `vm.Script`-based approach isn't used instead). */ async handler(ctx) { const code = ctx.codeblock.content @@ -78,34 +95,45 @@ const mjsCommand: Command = { // registering the task first guarantees it's visible in `jsk tasks`/cancellable via // `jsk cancel` immediately, without an extra tick's delay whenever security mode is off. const task = jsk.submitTask('jsk mjs', () => controller.abort()) - const { restoreGuards, restoreRestGuard } = await installSecurityGuards(ctx) // Unique per invocation (task.index is a monotonic counter) so concurrent evals get their // own file and can't clobber each other's stashed arguments on the shared global object. const argsKey = `__djsk_eval_args_${task.index}__` - const capture = captureTerminalOutput(jsk.config.security ? (text) => jsk.scrub(text) : null) - - const dir = ensureTempDir(jsk.config.evalModuleDir) - const file = path.join(dir, `eval-${task.index}.mjs`) + // Declared here and populated inside the `try` below (rather than ahead of it) so a throw + // from installSecurityGuards/captureTerminalOutput/ensureTempDir/writeFileSync still + // reaches the `finally` — otherwise it would skip cleanup entirely: `capture`'s monkeypatch + // of process.stdout/stderr.write would never be undone (permanently, until a restart), + // security-mode guards would stay installed, and `task` would be a permanent ghost entry + // in `jsk tasks`. + let restoreGuards: (() => void) | null = null + let restoreRestGuard: (() => void) | null = null + let restoreChildProcessGuard: (() => void) | null = null + let capture: ReturnType | null = null + let file: string | null = null try { - // biome-ignore lint/suspicious/noExplicitAny: temporary bridge for the generated module, deleted immediately below. + ;({ restoreGuards, restoreRestGuard } = await installSecurityGuards(ctx)) + capture = captureTerminalOutput(jsk.config.security ? (text) => jsk.scrub(text) : null) + + const dir = ensureTempDir(jsk.config.evalModuleDir) + file = path.join(dir, `eval-${task.index}.mjs`) + + // biome-ignore lint/suspicious/noExplicitAny: temporary bridge for the generated module, deleted in the `finally` below. ;(globalThis as any)[argsKey] = scope const preamble = `const { ${Object.keys(scope).join(', ')} } = globalThis[${JSON.stringify(argsKey)}];\n` writeFileSync(file, preamble + code, 'utf-8') - let namespace: Record - try { - // Cache-busting query string so repeated evals of the same task-index-free filename - // (or, after a restart, the same task index again) never serve a stale cached module. - namespace = await raceAbort( - import(`${pathToFileURL(file).href}?t=${Date.now()}`), - controller.signal, - ) - } finally { - // biome-ignore lint/suspicious/noExplicitAny: temporary bridge for the generated module. - delete (globalThis as any)[argsKey] - } + // Installed right before the import that actually loads the generated module — see + // installChildProcessTimeoutGuard's doc comment for why this has to patch the real, + // shared module rather than something scoped to this one eval like dynamicImport/require. + restoreChildProcessGuard = installChildProcessTimeoutGuard(jsk.config.evalTimeout) + + // Cache-busting query string so repeated evals of the same task-index-free filename + // (or, after a restart, the same task index again) never serve a stale cached module. + const namespace: Record = await raceAbort( + import(`${pathToFileURL(file).href}?t=${Date.now()}`), + controller.signal, + ) const terminalOutput = capture.restore() const result = 'default' in namespace ? namespace.default : undefined @@ -115,7 +143,9 @@ const mjsCommand: Command = { await ctx.react('✅') await sendResult(ctx, result, terminalOutput) } catch (error) { - const terminalOutput = capture.restore() + // `capture` can still be `null` here if the throw happened before terminal output + // capture was even installed (e.g. installSecurityGuards or ensureTempDir itself threw). + const terminalOutput = capture?.restore() ?? '' if (error instanceof EvalCancelledError) { // `jsk cancel` already sends its own confirmation — report only whatever terminal @@ -128,14 +158,19 @@ const mjsCommand: Command = { throw error } finally { - capture.restore() + capture?.restore() + restoreChildProcessGuard?.() restoreRestGuard?.() restoreGuards?.() jsk.removeTask(task) - try { - rmSync(file, { force: true }) - } catch { - // Best-effort cleanup; a leftover temp file is harmless (and .gitignore'd). + // biome-ignore lint/suspicious/noExplicitAny: temporary bridge for the generated module. + delete (globalThis as any)[argsKey] + if (file) { + try { + rmSync(file, { force: true }) + } catch { + // Best-effort cleanup; a leftover temp file is harmless (and .gitignore'd). + } } } }, diff --git a/src/jishaku.test.ts b/src/jishaku.test.ts index 6bb0a19..736c999 100644 --- a/src/jishaku.test.ts +++ b/src/jishaku.test.ts @@ -71,48 +71,36 @@ describe('Jishaku — update check on construction', () => { describe('Jishaku — process-wide error safety net', () => { // Jishaku registers real `process.on` listeners for the life of the process; every test here - // captures and removes exactly the ones its own instance added, so no listener leaks into - // later tests (or fires against an unrelated later `uncaughtException`/`unhandledRejection`). - const captureProcessListeners = () => { - const onSpy = vi.spyOn(process, 'on') - return { - onSpy, - cleanup: () => { - for (const [event, listener] of onSpy.mock.calls) { - if (event === 'uncaughtException' || event === 'unhandledRejection') { - process.removeListener(event, listener as (...args: unknown[]) => void) - } - } - onSpy.mockRestore() - }, - } - } + // calls `jsk.destroy()` to remove exactly the ones its own instance added, so no listener + // leaks into later tests (or fires against an unrelated later `uncaughtException`/ + // `unhandledRejection`). it('installs uncaughtException/unhandledRejection listeners by default', () => { - const { onSpy, cleanup } = captureProcessListeners() - - new Jishaku(fakeClient, { consoleLog: false }) + const onSpy = vi.spyOn(process, 'on') + const jsk = new Jishaku(fakeClient, { consoleLog: false }) expect(onSpy).toHaveBeenCalledWith('uncaughtException', expect.any(Function)) expect(onSpy).toHaveBeenCalledWith('unhandledRejection', expect.any(Function)) - cleanup() + + jsk.destroy() + onSpy.mockRestore() }) it('does not install those listeners when catchProcessErrors is false', () => { - const { onSpy, cleanup } = captureProcessListeners() - - new Jishaku(fakeClient, { consoleLog: false, catchProcessErrors: false }) + const onSpy = vi.spyOn(process, 'on') + const jsk = new Jishaku(fakeClient, { consoleLog: false, catchProcessErrors: false }) expect(onSpy).not.toHaveBeenCalledWith('uncaughtException', expect.any(Function)) expect(onSpy).not.toHaveBeenCalledWith('unhandledRejection', expect.any(Function)) - cleanup() + + jsk.destroy() + onSpy.mockRestore() }) it('logs an escaped error instead of letting it propagate', () => { - const { onSpy, cleanup } = captureProcessListeners() + const onSpy = vi.spyOn(process, 'on') const error = vi.spyOn(console, 'error').mockImplementation(() => {}) - - new Jishaku(fakeClient, { consoleLog: true }) + const jsk = new Jishaku(fakeClient, { consoleLog: true }) const handler = onSpy.mock.calls.find((call) => call[0] === 'uncaughtException')?.[1] as ( err: unknown, ) => void @@ -123,15 +111,15 @@ describe('Jishaku — process-wide error safety net', () => { expect.stringContaining('boom'), ) + jsk.destroy() error.mockRestore() - cleanup() + onSpy.mockRestore() }) it('does not log when consoleLog is off', () => { - const { onSpy, cleanup } = captureProcessListeners() + const onSpy = vi.spyOn(process, 'on') const error = vi.spyOn(console, 'error').mockImplementation(() => {}) - - new Jishaku(fakeClient, { consoleLog: false }) + const jsk = new Jishaku(fakeClient, { consoleLog: false }) const handler = onSpy.mock.calls.find((call) => call[0] === 'uncaughtException')?.[1] as ( err: unknown, ) => void @@ -139,7 +127,19 @@ describe('Jishaku — process-wide error safety net', () => { expect(error).not.toHaveBeenCalled() + jsk.destroy() error.mockRestore() - cleanup() + onSpy.mockRestore() + }) + + it('destroy() removes the listeners so they no longer fire, and is safe to call more than once', () => { + const jsk = new Jishaku(fakeClient, { consoleLog: true }) + const before = process.listenerCount('uncaughtException') + + jsk.destroy() + + expect(process.listenerCount('uncaughtException')).toBe(before - 1) + expect(() => jsk.destroy()).not.toThrow() + expect(process.listenerCount('uncaughtException')).toBe(before - 1) }) }) diff --git a/src/jishaku.ts b/src/jishaku.ts index c475118..0c4ea85 100644 --- a/src/jishaku.ts +++ b/src/jishaku.ts @@ -146,6 +146,23 @@ export class Jishaku { } } + /** + * Removes the process-wide `uncaughtException`/`unhandledRejection` listeners installed by + * `catchProcessErrors` (a no-op if it was `false`, or if called more than once). + * + * `process` is a shared, process-wide `EventEmitter` with no concept of "this listener came + * from an instance that's since been discarded" — those two listeners stay registered, and + * `handleProcessError`'s closure keeps this whole instance (client, owners, scrubber, + * taskList) reachable from `process`, for as long as the process runs, unless explicitly + * removed. Call this before dropping the last reference to a `Jishaku` instance you no longer + * need — e.g. a test's `afterEach`, or before constructing a replacement after a hot-reload — + * so listeners (and instances) don't accumulate across the process's lifetime. + */ + destroy(): void { + process.off('uncaughtException', this.handleProcessError) + process.off('unhandledRejection', this.handleProcessError) + } + /** * Redacts secrets from `text` before it is exposed anywhere. *