Fix/eval safety leaks - #23
Merged
Merged
Conversation
…r leaks
Code-review pass over the js/cjs/mjs eval engine and the catchProcessErrors
safety net turned up several real leaks/gaps, all fixed here:
- runVmEval (js/cjs) and jsk mjs's handler submitted a task and installed
security guards/terminal capture before entering their try block, so a
throw there (e.g. installSecurityGuards failing) skipped cleanup —
jsk mjs specifically could leave process.stdout/stderr permanently
monkeypatched. Moved all of that inside try, with nullable holders so
catch/finally still see whatever got set up before the throw.
- jsk mjs's globalThis[argsKey] bridge could leak the eval scope (including
the live client) if writeFileSync failed between setting it and the
import() that used to be the only place deleting it; now always deleted
in the outer finally.
- jsk cjs's require('child_process') was a direct, unwrapped path to
execSync/execFileSync/spawnSync, bypassing the default-timeout guard
dynamicImport() already gives jsk js/jsk cjs. Added createGuardedRequire
(shares the same Proxy wrap as createDynamicImport) so require gets it
too. jsk mjs's static `import` can't be intercepted the same way without
a process-wide ESM loader hook, so that gap is documented instead.
- catchProcessErrors installed process-wide uncaughtException/
unhandledRejection listeners with no way to remove them, leaking one
Jishaku instance's worth of listeners (and everything they close over)
per construction. Added Jishaku#destroy() to remove them.
jsk mjs's cache-busting import() also permanently grows Node's ESM module
cache by one entry per eval — inherent to needing a fresh module per
invocation with no cache-eviction API, and not fixable without giving up
real top-level import/await support, so it's documented as a known
trade-off rather than "fixed".
- `>=18` to `>=22`
jsk mjs's generated module can reach node:child_process via a real static import, which — unlike dynamicImport (jsk js) or the guarded require (jsk cjs) — goes straight through Node's own loader with no per-call interception point, so it silently bypassed the execSync/execFileSync/ spawnSync default-timeout protection the other two flavors get. Fixed by installChildProcessTimeoutGuard: temporarily patch the real, shared child_process module (via a plain require, not the frozen ESM namespace) right before importing the eval's generated module, then restore it once the import settles. Confirmed experimentally (Node 22 and 24) that a module loaded while the patch is active observes it through default, named, and namespace import forms alike, unlike mutating an already-obtained module object after the fact. The remaining "not fixed" trade-off for jsk mjs is now only the ESM module cache growing by one entry per eval (no Node API to evict a loaded module) — documented in the same doc comment.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Why?
Related issue(s)
closes
Check
pnpm check(lint, format & typecheck)pnpm test(functions test)pnpm build(dist/)