refactor(downloader): Updated to use node:vm for unsafe scripts #4198
Conversation
|
BG does not run on node:vm. |
There was a problem hiding this comment.
Pull request overview
This PR refactors the downloader plugin to use Node.js's vm module instead of the Function constructor for executing untrusted scripts from YouTube's APIs. The goal is to improve security by sandboxing script execution.
Changes:
- Replaced
Functionconstructor withvm.runInNewContextfor executing YouTube cipher scripts inPlatform.shim.eval - Changed
Platform.shim.evalfrom async to synchronous function - Applied
vm.runInNewContextfor executing interpreter JavaScript from background challenges
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return vm.runInNewContext( | ||
| `(() => { ${code} })()`, | ||
| Object.create(null) as vm.Context, | ||
| ) as Types.EvalResult; |
There was a problem hiding this comment.
The vm.runInNewContext is being called with an empty context (Object.create(null)), but the code being executed references 'exportedVars' which is defined in 'data.output'. Since the context is empty, this will throw a ReferenceError when the code tries to access 'exportedVars.nFunction' or 'exportedVars.sigFunction'. The context should either include the necessary variables from data.output or the implementation needs to be restructured to properly evaluate the code with its dependencies.
| // Maybe there is a better way to do this (e.g. https://github.com/Siubaak/sval ?) | ||
| // eslint-disable-next-line @typescript-eslint/no-implied-eval,@typescript-eslint/no-unsafe-call | ||
| new Function(interpreterJavascript)(); | ||
| vm.runInNewContext(interpreterJavascript, globalThis); |
There was a problem hiding this comment.
Using globalThis as the context for vm.runInNewContext defeats the purpose of sandboxing. This gives the untrusted interpreterJavascript code full access to the Node.js global scope, including process, require, and other sensitive APIs. This is a critical security vulnerability. Instead, use an isolated context object that only includes the specific properties needed (window and document), similar to how Object.create(null) is used elsewhere. Consider using vm.createContext with a restricted sandbox object.
| if (env.n) { | ||
| properties.push(`n: exportedVars.nFunction("${env.n}")`); | ||
| } | ||
|
|
||
| if (env.sig) { | ||
| properties.push(`sig: exportedVars.sigFunction("${env.sig}")`) | ||
| properties.push(`sig: exportedVars.sigFunction("${env.sig}")`); |
There was a problem hiding this comment.
The env.n and env.sig values are directly interpolated into the code string without sanitization or escaping. If these values contain special characters like quotes or backslashes, they could break out of the string literals and inject arbitrary code. Consider using proper escaping or a different approach that doesn't rely on string interpolation to construct the code.
| const ffmpegMutex = new Mutex(); | ||
|
|
||
| Platform.shim.eval = async (data: Types.BuildScriptResult, env: Record<string, Types.VMPrimative>) => { | ||
| Platform.shim.eval = ( |
There was a problem hiding this comment.
The function signature was changed from async to synchronous, removing the async keyword. If the youtubei.js library expects this Platform.shim.eval function to return a Promise (as indicated by the original async signature), this change will break the API contract and cause type errors or runtime issues. Verify whether the library can handle a synchronous return value.
| Platform.shim.eval = ( | |
| Platform.shim.eval = async ( |
2 participants
No description provided.