Skip to content

Commit 9bfd724

Browse files
committed
fix: handle isolated-vm compatibility issues and HTTP MCP import bugs
- Make isolated-vm gracefully detect and fallback when unavailable: - Detect Bun runtime (uses JavaScriptCore, not V8) - Detect Node.js 25+ (V8 API changes break isolated-vm) - Use lazy module loading to avoid import failures on incompatible runtimes - Provide helpful error messages for debugging - Fix HTTP MCP server import in tryAutoImportFromClient: - Previously only copied command/args/env, ignoring url and auth - Now properly handles HTTP transport fields (url, auth) - Fix credential storage error handling: - Check if storage succeeded before replacing token with reference - If storage fails, keep token in profile as fallback - Prevents losing tokens when secure storage is unavailable Fixes #9, #10, #11
1 parent a11437b commit 9bfd724

2 files changed

Lines changed: 148 additions & 32 deletions

File tree

‎src/code-mode/sandbox/isolated-vm-sandbox.ts‎

Lines changed: 107 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,46 @@
88
* - Cannot access Node.js APIs by design
99
*
1010
* This is the most secure sandbox option available in Node.js.
11+
*
12+
* Note: isolated-vm requires V8 and won't work on:
13+
* - Bun (uses JavaScriptCore)
14+
* - Node.js 25+ (V8 API changes)
1115
*/
1216

13-
import ivm from 'isolated-vm';
1417
import { logger } from '../../utils/logger.js';
1518

19+
// Lazy-loaded isolated-vm module (may fail on Bun or incompatible runtimes)
20+
let ivm: typeof import('isolated-vm') | null = null;
21+
let ivmLoadAttempted = false;
22+
let ivmLoadError: string | null = null;
23+
24+
/**
25+
* Lazily load isolated-vm module
26+
* Returns null if loading fails (e.g., on Bun or incompatible Node.js versions)
27+
*/
28+
async function getIVM(): Promise<typeof import('isolated-vm') | null> {
29+
if (ivmLoadAttempted) {
30+
return ivm;
31+
}
32+
ivmLoadAttempted = true;
33+
34+
try {
35+
ivm = await import('isolated-vm');
36+
return ivm;
37+
} catch (e) {
38+
ivmLoadError = (e as Error)?.message ?? String(e);
39+
logger.debug(`Failed to load isolated-vm: ${ivmLoadError}`);
40+
return null;
41+
}
42+
}
43+
44+
/**
45+
* Synchronously check if isolated-vm was loaded (for isAvailable check)
46+
*/
47+
function getIVMSync(): typeof import('isolated-vm') | null {
48+
return ivm;
49+
}
50+
1651
/**
1752
* Tool definition for the sandbox
1853
*/
@@ -65,7 +100,7 @@ const DEFAULT_CONFIG: IsolatedVMSandboxConfig = {
65100
*/
66101
export class IsolatedVMSandbox {
67102
private config: IsolatedVMSandboxConfig;
68-
private isolate: ivm.Isolate | null = null;
103+
private isolate: any = null; // ivm.Isolate (lazy loaded)
69104

70105
constructor(config?: Partial<IsolatedVMSandboxConfig>) {
71106
this.config = { ...DEFAULT_CONFIG, ...config };
@@ -87,9 +122,15 @@ export class IsolatedVMSandbox {
87122
const startTime = Date.now();
88123
const logs: string[] = [];
89124

125+
// Load isolated-vm module
126+
const ivmModule = await getIVM();
127+
if (!ivmModule) {
128+
throw new Error(`isolated-vm not available: ${ivmLoadError || 'module failed to load'}`);
129+
}
130+
90131
try {
91132
// Create isolated V8 isolate with memory limit
92-
this.isolate = new ivm.Isolate({
133+
this.isolate = new ivmModule.Isolate({
93134
memoryLimit: this.config.memoryLimit,
94135
inspector: this.config.inspector,
95136
});
@@ -104,10 +145,10 @@ export class IsolatedVMSandbox {
104145
await jail.set('global', jail.derefInto());
105146

106147
// Create log capture
107-
await this.setupLogging(context, jail, logs);
148+
await this.setupLogging(ivmModule, context, jail, logs);
108149

109150
// Create tool callback system
110-
await this.setupToolCallbacks(context, jail, tools, toolExecutor);
151+
await this.setupToolCallbacks(ivmModule, context, jail, tools, toolExecutor);
111152

112153
// Set up basic utilities (JSON is built into V8)
113154
await this.setupUtilities(jail);
@@ -127,7 +168,7 @@ export class IsolatedVMSandbox {
127168

128169
// Copy result out of isolate
129170
const result = typeof resultRef === 'object' && resultRef !== null
130-
? await (resultRef as ivm.Reference<unknown>).copy()
171+
? await resultRef.copy()
131172
: resultRef;
132173

133174
const duration = Date.now() - startTime;
@@ -147,6 +188,9 @@ export class IsolatedVMSandbox {
147188
errorMessage = 'Execution terminated (memory limit exceeded)';
148189
} else if (errorMessage.includes('Script execution timed out')) {
149190
errorMessage = `Execution timeout after ${this.config.timeout}ms`;
191+
} else if (errorMessage.includes('undefined') && errorMessage.includes('length')) {
192+
// Node.js 25+ V8 API incompatibility
193+
errorMessage = 'V8 callback API incompatibility - please use Node.js 22-24 or set NCP_DISABLE_ISOLATED_VM=true';
150194
}
151195

152196
return {
@@ -165,26 +209,27 @@ export class IsolatedVMSandbox {
165209
* Set up logging in the isolated context
166210
*/
167211
private async setupLogging(
168-
context: ivm.Context,
169-
jail: ivm.Reference<Record<string, unknown>>,
212+
ivmModule: typeof import('isolated-vm'),
213+
context: any, // ivm.Context
214+
jail: any, // ivm.Reference<Record<string, unknown>>
170215
logs: string[]
171216
): Promise<void> {
172217
// Create log callback that runs in main thread
173-
const logCallback = new ivm.Callback((...args: unknown[]) => {
218+
const logCallback = new ivmModule.Callback((...args: unknown[]) => {
174219
const message = args
175220
.map((arg) => (typeof arg === 'object' ? JSON.stringify(arg) : String(arg)))
176221
.join(' ');
177222
logs.push(message);
178223
});
179224

180-
const errorCallback = new ivm.Callback((...args: unknown[]) => {
225+
const errorCallback = new ivmModule.Callback((...args: unknown[]) => {
181226
const message =
182227
'[ERROR] ' +
183228
args.map((arg) => (typeof arg === 'object' ? JSON.stringify(arg) : String(arg))).join(' ');
184229
logs.push(message);
185230
});
186231

187-
const warnCallback = new ivm.Callback((...args: unknown[]) => {
232+
const warnCallback = new ivmModule.Callback((...args: unknown[]) => {
188233
const message =
189234
'[WARN] ' +
190235
args.map((arg) => (typeof arg === 'object' ? JSON.stringify(arg) : String(arg))).join(' ');
@@ -210,13 +255,14 @@ export class IsolatedVMSandbox {
210255
* Set up tool callbacks in the isolated context
211256
*/
212257
private async setupToolCallbacks(
213-
context: ivm.Context,
214-
jail: ivm.Reference<Record<string, unknown>>,
258+
ivmModule: typeof import('isolated-vm'),
259+
context: any, // ivm.Context
260+
jail: any, // ivm.Reference<Record<string, unknown>>
215261
tools: IsolatedVMTool[],
216262
toolExecutor: (toolName: string, params: unknown) => Promise<unknown>
217263
): Promise<void> {
218264
// Create a callback that executes tools in the main thread
219-
const executeToolCallback = new ivm.Callback(
265+
const executeToolCallback = new ivmModule.Callback(
220266
async (toolName: string, paramsJson: string): Promise<string> => {
221267
try {
222268
const params = paramsJson ? JSON.parse(paramsJson) : {};
@@ -279,7 +325,7 @@ export class IsolatedVMSandbox {
279325
/**
280326
* Set up basic utilities in the isolated context
281327
*/
282-
private async setupUtilities(jail: ivm.Reference<Record<string, unknown>>): Promise<void> {
328+
private async setupUtilities(jail: any): Promise<void> { // ivm.Reference<Record<string, unknown>>
283329
// JSON, Promise, Array, Object, etc. are already available in V8
284330
// We just need to ensure they're accessible
285331

@@ -323,29 +369,70 @@ export class IsolatedVMSandbox {
323369
}
324370

325371
/**
326-
* Check if the sandbox is available (isolated-vm is installed)
372+
* Check if the sandbox is available (isolated-vm is installed and working)
373+
*
374+
* Returns false in cases where isolated-vm won't work:
375+
* - Node.js 25+ (V8 API changes cause "Cannot read properties of undefined (reading 'length')")
376+
* - Bun runtime (uses JavaScriptCore, not V8)
377+
* - Environment variable NCP_DISABLE_ISOLATED_VM=true
378+
* - Module load failures
327379
*/
328380
static isAvailable(): boolean {
329381
if (process.env.NCP_DISABLE_ISOLATED_VM === 'true') {
330382
logger.warn('isolated-vm sandbox disabled via NCP_DISABLE_ISOLATED_VM');
331383
return false;
332384
}
333385

386+
// Check for Bun runtime (uses JavaScriptCore, not V8)
387+
if (typeof (process.versions as any).bun !== 'undefined') {
388+
logger.warn('isolated-vm unavailable: Bun runtime uses JavaScriptCore, not V8');
389+
return false;
390+
}
391+
392+
// Check Node.js version - isolated-vm has issues with Node.js 25+
393+
const nodeVersion = process.versions.node;
394+
const majorVersion = parseInt(nodeVersion.split('.')[0], 10);
395+
if (majorVersion >= 25) {
396+
logger.warn(`isolated-vm unavailable: Node.js ${nodeVersion} has V8 API changes that break isolated-vm`);
397+
return false;
398+
}
399+
400+
// If module already failed to load, return false
401+
if (ivmLoadAttempted && !ivm) {
402+
logger.warn(`isolated-vm unavailable: ${ivmLoadError || 'module failed to load'}`);
403+
return false;
404+
}
405+
406+
// If module not yet loaded, try synchronous check
407+
// This will be attempted again in execute() with async loading
408+
if (!ivmLoadAttempted) {
409+
// Return true optimistically - the async load in execute() will handle failures
410+
return true;
411+
}
412+
334413
try {
335-
// Try to create a minimal isolate to verify the module works
336-
const testIsolate = new ivm.Isolate({ memoryLimit: 8 });
414+
// Module is loaded, try to create a minimal isolate to verify it works
415+
const testIsolate = new ivm!.Isolate({ memoryLimit: 8 });
337416
testIsolate.dispose();
338417
return true;
339418
} catch (e) {
340-
logger.warn(`isolated-vm unavailable: ${(e as Error)?.message ?? e}`);
419+
const errorMessage = (e as Error)?.message ?? String(e);
420+
// Detect common failure patterns
421+
if (errorMessage.includes('undefined') && errorMessage.includes('length')) {
422+
logger.warn('isolated-vm unavailable: V8 callback API incompatibility detected');
423+
} else if (errorMessage.includes('ERR_DLOPEN_FAILED') || errorMessage.includes('undefined symbol')) {
424+
logger.warn('isolated-vm unavailable: Native module incompatibility (possibly Bun or unsupported runtime)');
425+
} else {
426+
logger.warn(`isolated-vm unavailable: ${errorMessage}`);
427+
}
341428
return false;
342429
}
343430
}
344431

345432
/**
346433
* Get memory usage of the current isolate
347434
*/
348-
getHeapStatistics(): ivm.HeapStatistics | null {
435+
getHeapStatistics(): any | null { // ivm.HeapStatistics
349436
if (!this.isolate) return null;
350437

351438
try {

‎src/profiles/profile-manager.ts‎

Lines changed: 41 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -61,27 +61,42 @@ export class ProfileManager {
6161
// Store token/API key
6262
if (config.auth.token) {
6363
const type: CredentialType = config.auth.type === 'bearer' ? 'bearer_token' : 'api_key';
64-
await this.credentialStore.setCredential(
64+
const success = await this.credentialStore.setCredential(
6565
mcpName,
6666
type,
6767
config.auth.token,
6868
`${config.auth.type} credential for ${mcpName}`
6969
);
70-
// Replace with secure reference
71-
securConfig.auth.token = '_USE_SECURE_STORAGE_';
70+
71+
if (success) {
72+
// Only replace with secure reference if storage succeeded
73+
securConfig.auth.token = '_USE_SECURE_STORAGE_';
74+
logger.debug(`Stored ${type} for ${mcpName} in secure storage`);
75+
} else {
76+
// Storage failed - keep the token in the config as fallback
77+
// This is less secure but ensures the MCP can still work
78+
logger.warn(`Failed to store ${type} for ${mcpName} in secure storage - keeping in profile`);
79+
}
7280
}
7381

7482
// Store basic auth credentials
7583
if (config.auth.username && config.auth.password) {
76-
await this.credentialStore.setCredential(
84+
const success = await this.credentialStore.setCredential(
7785
mcpName,
7886
'basic_auth',
7987
{ username: config.auth.username, password: config.auth.password },
8088
`Basic auth credentials for ${mcpName}`
8189
);
82-
// Replace with secure references
83-
securConfig.auth.username = '_USE_SECURE_STORAGE_';
84-
securConfig.auth.password = '_USE_SECURE_STORAGE_';
90+
91+
if (success) {
92+
// Only replace with secure references if storage succeeded
93+
securConfig.auth.username = '_USE_SECURE_STORAGE_';
94+
securConfig.auth.password = '_USE_SECURE_STORAGE_';
95+
logger.debug(`Stored basic auth for ${mcpName} in secure storage`);
96+
} else {
97+
// Storage failed - keep credentials in the config as fallback
98+
logger.warn(`Failed to store basic auth for ${mcpName} in secure storage - keeping in profile`);
99+
}
85100
}
86101

87102
return securConfig;
@@ -262,11 +277,25 @@ export class ProfileManager {
262277

263278
try {
264279
// Remove metadata fields before adding (internal use only)
265-
const cleanConfig = {
266-
command: config.command,
267-
args: config.args || [],
268-
env: config.env || {}
269-
};
280+
// Support both stdio (command) and HTTP (url) transport types
281+
const cleanConfig: MCPConfig = {};
282+
283+
// Stdio transport fields
284+
if (config.command) {
285+
cleanConfig.command = config.command;
286+
cleanConfig.args = config.args || [];
287+
cleanConfig.env = config.env || {};
288+
}
289+
290+
// HTTP/SSE transport fields
291+
if (config.url) {
292+
cleanConfig.url = config.url;
293+
}
294+
295+
// Auth configuration (for HTTP MCPs)
296+
if (config.auth) {
297+
cleanConfig.auth = config.auth;
298+
}
270299

271300
// Use addMCPToProfile to ensure cache updates happen
272301
await this.addMCPToProfile('all', name, cleanConfig);

0 commit comments

Comments
 (0)