Fix/eval safety leaks

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 <value>` to produce a result instead (e.g. `export default 1 + 1;`).
 - It writes a transient `.mjs` file under `<evalModuleDir>/.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]
 >   

package.json

@@ -57,7 +57,7 @@
     "prepublishOnly": "pnpm build"
   },
   "engines": {
-    "node": ">=18"
+    "node": ">=22"
   },
   "publishConfig": {
     "access": "public",

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')
+  })
+})

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')
   },
 }