Skip to content

refactor(downloader): Updated to use node:vm for unsafe scripts #4198

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
its-iris wants to merge 1 commit into from
Closed

refactor(downloader): Updated to use node:vm for unsafe scripts #4198

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 16 additions & 15 deletions src/plugins/downloader/main/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { existsSync, mkdirSync, writeFileSync } from 'node:fs';
import { join } from 'node:path';
import { randomBytes } from 'node:crypto';
import vm from 'node:vm';

import { app, type BrowserWindow, dialog, ipcMain } from 'electron';
import {
Expand All @@ -9,6 +10,7 @@ import {
Utils,
YTNodes,
Platform,
type Types,
} from '\u0079\u006f\u0075\u0074\u0075\u0062\u0065i.js';
import is from 'electron-is';
import filenamify from 'filenamify';
Expand Down Expand Up @@ -58,21 +60,24 @@ const ffmpeg = lazy(async () =>
);
const ffmpegMutex = new Mutex();

Platform.shim.eval = async (data: Types.BuildScriptResult, env: Record<string, Types.VMPrimative>) => {
Platform.shim.eval = (
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
Platform.shim.eval = (
Platform.shim.eval = async (

Copilot uses AI. Check for mistakes.
data: Types.BuildScriptResult,
env: Record<string, Types.VMPrimative>,
) => {
const properties = [];

if(env.n) {
properties.push(`n: exportedVars.nFunction("${env.n}")`)
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}")`);
Comment on lines +68 to +72
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
}

const code = `${data.output}\nreturn { ${properties.join(', ')} }`;

return new Function(code)();
}
return vm.runInNewContext(
`(() => { ${code} })()`,
Object.create(null) as vm.Context,
) as Types.EvalResult;
Comment on lines +76 to +79
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
};

let yt: Innertube;
let win: BrowserWindow;
Expand Down Expand Up @@ -190,10 +195,7 @@ export const onMainLoad = async ({
.privateDoNotAccessOrElseSafeScriptWrappedValue;

if (interpreterJavascript) {
// This is a workaround to run the interpreterJavascript code
// 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);
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

const poTokenResult = await BG.PoToken.generate({
program: bgChallenge.program,
Expand Down Expand Up @@ -421,8 +423,7 @@ async function downloadSongUnsafe(
let targetFileExtension: string;
if (!presetSetting?.extension) {
targetFileExtension =
VideoFormatList.find((it) => it.itag === format.itag)?.container ??
'mp3';
VideoFormatList.find((it) => it.itag === format.itag)?.container ?? 'mp3';
} else {
targetFileExtension = presetSetting?.extension ?? 'mp3';
}
Expand Down
Loading