From 41ed51b4e3c066de38ef4be3df3fcff0f06783bc Mon Sep 17 00:00:00 2001 From: lohit Date: Tue, 23 Dec 2025 17:31:51 +0530 Subject: [PATCH] fix: handle `additional context root` paths for `node-vm` (#6491) * fix: handle additional context root paths for node vm * fix: handle additional context root paths for node vm * fix: coderabbit review fixes --- .../bruno-js/src/sandbox/node-vm/index.js | 55 ++-- .../src/sandbox/node-vm/index.spec.js | 252 ++++++++++++++++++ 2 files changed, 288 insertions(+), 19 deletions(-) create mode 100644 packages/bruno-js/src/sandbox/node-vm/index.spec.js diff --git a/packages/bruno-js/src/sandbox/node-vm/index.js b/packages/bruno-js/src/sandbox/node-vm/index.js index aecfd9d8b..efb5e60a2 100644 --- a/packages/bruno-js/src/sandbox/node-vm/index.js +++ b/packages/bruno-js/src/sandbox/node-vm/index.js @@ -38,6 +38,15 @@ async function runScriptInNodeVm({ try { const allowScriptFilesystemAccess = get(scriptingConfig, 'filesystemAccess.allow', false); + // Compute additional context roots + const additionalContextRoots = get(scriptingConfig, 'additionalContextRoots', []); + const additionalContextRootsAbsolute = lodash + .chain(additionalContextRoots) + .map((acr) => (path.isAbsolute(acr) ? acr : path.join(collectionPath, acr))) + .map((acr) => path.normalize(acr)) + .value(); + additionalContextRootsAbsolute.push(path.normalize(collectionPath)); + // Create script context with all necessary variables const scriptContext = { // Bruno context @@ -79,7 +88,8 @@ async function runScriptInNodeVm({ scriptContext, currentModuleDir: collectionPath, localModuleCache, - allowScriptFilesystemAccess + allowScriptFilesystemAccess, + additionalContextRootsAbsolute }); // Execute the script in an isolated VM context @@ -107,6 +117,7 @@ async function runScriptInNodeVm({ * @param {string} options.currentModuleDir - Current module directory for relative imports * @param {Map} options.localModuleCache - Cache for loaded local modules * @param {boolean} options.allowScriptFilesystemAccess - Whether to allow fs module access + * @param {Array} options.additionalContextRootsAbsolute - Pre-computed absolute context roots * @returns {Function} Custom require function */ function createCustomRequire({ @@ -115,19 +126,15 @@ function createCustomRequire({ scriptContext, currentModuleDir = collectionPath, localModuleCache = new Map(), - allowScriptFilesystemAccess = false + allowScriptFilesystemAccess = false, + additionalContextRootsAbsolute = [] }) { - const additionalContextRoots = get(scriptingConfig, 'additionalContextRoots', []); - const additionalContextRootsAbsolute = lodash - .chain(additionalContextRoots) - .map((acr) => (acr.startsWith('/') ? acr : path.join(collectionPath, acr))) - .value(); - additionalContextRootsAbsolute.push(collectionPath); - return (moduleName) => { - // Check if it's a local module (starts with ./ or ../) - if (moduleName.startsWith('./') || moduleName.startsWith('../')) { - return loadLocalModule({ moduleName, collectionPath, scriptContext, localModuleCache, currentModuleDir }); + // Check if it's a local module (starts with ./ or ../ or .\ or ..\) + // Normalize backslashes to forward slashes for cross-platform compatibility + const normalizedModuleName = moduleName.replace(/\\/g, '/'); + if (normalizedModuleName.startsWith('./') || normalizedModuleName.startsWith('../')) { + return loadLocalModule({ moduleName: normalizedModuleName, collectionPath, scriptContext, localModuleCache, currentModuleDir, additionalContextRootsAbsolute }); } // Helper function to check if a module is the fs module or a submodule @@ -178,6 +185,7 @@ function createCustomRequire({ * @param {Object} options.scriptContext - Script execution context to inherit * @param {Map} options.localModuleCache - Cache for loaded modules * @param {string} options.currentModuleDir - Directory of the current module for relative resolution + * @param {Array} options.additionalContextRootsAbsolute - Additional allowed context root paths * @returns {*} The exported content of the loaded module * @throws {Error} When module is outside collection path or cannot be loaded */ @@ -186,7 +194,8 @@ function loadLocalModule({ collectionPath, scriptContext, localModuleCache, - currentModuleDir + currentModuleDir, + additionalContextRootsAbsolute = [] }) { // Check if the filename has an extension const hasExtension = path.extname(moduleName) !== ''; @@ -195,12 +204,19 @@ function loadLocalModule({ // Resolve the file path relative to the current module's directory const filePath = path.resolve(currentModuleDir, resolvedFilename); const normalizedFilePath = path.normalize(filePath); - const normalizedCollectionPath = path.normalize(collectionPath); - // Cross-platform security check: ensure the resolved file is within collectionPath - const relativePath = path.relative(normalizedCollectionPath, normalizedFilePath); - if (relativePath.startsWith('..') || path.isAbsolute(relativePath)) { - throw new Error(`Access to files outside of the collectionPath is not allowed: ${moduleName}`); + const isWithinAllowedRoot = additionalContextRootsAbsolute.some((allowedRoot) => { + const normalizedAllowedRoot = path.normalize(allowedRoot); + const relativePath = path.relative(normalizedAllowedRoot, normalizedFilePath); + return !relativePath.startsWith('..') && !path.isAbsolute(relativePath); + }); + + if (!isWithinAllowedRoot) { + const allowedRootsDisplay = additionalContextRootsAbsolute.map((root) => ` - ${root}`).join('\n'); + throw new Error( + `Access to files outside of the allowed context roots is not allowed: ${moduleName}\n\n` + + `Allowed context roots:\n${allowedRootsDisplay}` + ); } // Check cache first (use normalized path as key) @@ -235,7 +251,8 @@ function loadLocalModule({ scriptContext, currentModuleDir: moduleDir, localModuleCache, - allowScriptFilesystemAccess: get(scriptContext.scriptingConfig, 'filesystemAccess.allow', false) + allowScriptFilesystemAccess: get(scriptContext.scriptingConfig, 'filesystemAccess.allow', false), + additionalContextRootsAbsolute }) }; diff --git a/packages/bruno-js/src/sandbox/node-vm/index.spec.js b/packages/bruno-js/src/sandbox/node-vm/index.spec.js new file mode 100644 index 000000000..0b71d41da --- /dev/null +++ b/packages/bruno-js/src/sandbox/node-vm/index.spec.js @@ -0,0 +1,252 @@ +const { describe, it, expect, beforeEach, afterEach } = require('@jest/globals'); +const fs = require('fs'); +const path = require('path'); +const os = require('os'); +const { runScriptInNodeVm } = require('./index'); + +describe('node-vm sandbox', () => { + let testDir; + let collectionPath; + + beforeEach(() => { + // Create a temporary test directory + testDir = fs.mkdtempSync(path.join(os.tmpdir(), 'bruno-test-')); + collectionPath = path.join(testDir, 'collection'); + fs.mkdirSync(collectionPath); + }); + + afterEach(() => { + // Clean up test directory + fs.rmSync(testDir, { recursive: true, force: true }); + }); + + describe('createCustomRequire - local modules', () => { + it('should load local module with ./ path', async () => { + // Create a local module + fs.writeFileSync( + path.join(collectionPath, 'helper.js'), + 'module.exports = { value: 42 };' + ); + + const script = ` + const helper = require('./helper'); + bru.setVar('result', helper.value); + `; + + const context = { + bru: { setVar: jest.fn() }, + console: console + }; + + await runScriptInNodeVm({ script, context, collectionPath, scriptingConfig: {} }); + + expect(context.bru.setVar).toHaveBeenCalledWith('result', 42); + }); + + it('should load local module with ../ path', async () => { + // Create a subdirectory and modules + const subDir = path.join(collectionPath, 'subdir'); + fs.mkdirSync(subDir); + fs.writeFileSync( + path.join(collectionPath, 'parent.js'), + 'module.exports = { name: "parent" };' + ); + fs.writeFileSync( + path.join(subDir, 'child.js'), + 'const parent = require("../parent"); module.exports = parent;' + ); + + const script = ` + const child = require('./subdir/child'); + bru.setVar('result', child.name); + `; + + const context = { + bru: { setVar: jest.fn() }, + console: console + }; + + await runScriptInNodeVm({ script, context, collectionPath, scriptingConfig: {} }); + + expect(context.bru.setVar).toHaveBeenCalledWith('result', 'parent'); + }); + + it('should handle backslashes on Windows', async () => { + const subDir = path.join(collectionPath, 'utils'); + fs.mkdirSync(subDir); + fs.writeFileSync( + path.join(subDir, 'module.js'), + 'module.exports = { platform: "cross-platform" };' + ); + + // Simulate Windows-style path with backslashes + const script = ` + const mod = require('.\\\\utils\\\\module'); + bru.setVar('result', mod.platform); + `; + + const context = { + bru: { setVar: jest.fn() }, + console: console + }; + + await runScriptInNodeVm({ script, context, collectionPath, scriptingConfig: {} }); + + expect(context.bru.setVar).toHaveBeenCalledWith('result', 'cross-platform'); + }); + + it('should block access outside collection path', async () => { + const script = ` + const outside = require('../../outside'); + `; + + const context = { console: console }; + + await expect( + runScriptInNodeVm({ script, context, collectionPath, scriptingConfig: {} }) + ).rejects.toThrow('Access to files outside of the allowed context roots is not allowed'); + }); + }); + + describe('createCustomRequire - additionalContextRoots', () => { + it('should allow module access from additionalContextRoots', async () => { + // Create an additional context root at same level as collection + const additionalRoot = path.join(testDir, 'shared'); + fs.mkdirSync(additionalRoot); + fs.writeFileSync( + path.join(additionalRoot, 'shared.js'), + 'module.exports = { shared: true };' + ); + + // From collection, traverse up to testDir, then into shared directory + const script = ` + const shared = require('../shared/shared'); + bru.setVar('result', shared.shared); + `; + + const context = { + bru: { setVar: jest.fn() }, + console: console + }; + + const scriptingConfig = { + additionalContextRoots: [additionalRoot] + }; + + await runScriptInNodeVm({ script, context, collectionPath, scriptingConfig }); + + expect(context.bru.setVar).toHaveBeenCalledWith('result', true); + }); + + it('should handle relative additionalContextRoots path', async () => { + // Create a sibling directory to collection + const libsDir = path.join(testDir, 'libs'); + fs.mkdirSync(libsDir); + fs.writeFileSync( + path.join(libsDir, 'lib.js'), + 'module.exports = { fromLib: "yes" };' + ); + + const script = ` + const lib = require('../libs/lib'); + bru.setVar('result', lib.fromLib); + `; + + const context = { + bru: { setVar: jest.fn() }, + console: console + }; + + const scriptingConfig = { + additionalContextRoots: ['../libs'] + }; + + await runScriptInNodeVm({ script, context, collectionPath, scriptingConfig }); + + expect(context.bru.setVar).toHaveBeenCalledWith('result', 'yes'); + }); + + it('should handle nested additional context roots modules', async () => { + // Create an additional context root + const additionalRoot = path.join(testDir, 'shared'); + fs.mkdirSync(additionalRoot); + fs.writeFileSync( + path.join(additionalRoot, 'allowed.js'), + 'module.exports = { allowed: true };' + ); + + // Create a nested module that tries to require from additional root + fs.writeFileSync( + path.join(collectionPath, 'parent.js'), + ` + const allowed = require('../shared/allowed'); + module.exports = { nestedAccess: allowed.allowed }; + ` + ); + + const script = ` + const parent = require('./parent'); + bru.setVar('result', parent.nestedAccess); + `; + + const context = { + bru: { setVar: jest.fn() }, + console: console + }; + + const scriptingConfig = { + additionalContextRoots: [additionalRoot] + }; + + await runScriptInNodeVm({ script, context, collectionPath, scriptingConfig }); + + // Nested module should successfully access the additional root + expect(context.bru.setVar).toHaveBeenCalledWith('result', true); + }); + }); + + describe('createCustomRequire - npm modules', () => { + it('should load npm module', async () => { + const script = ` + const lodash = require('lodash'); + bru.setVar('result', typeof lodash.get); + `; + + const context = { + bru: { setVar: jest.fn() }, + console: console + }; + + await runScriptInNodeVm({ script, context, collectionPath, scriptingConfig: {} }); + + expect(context.bru.setVar).toHaveBeenCalledWith('result', 'function'); + }); + }); + + describe('createCustomRequire - module caching', () => { + it('should cache loaded modules', async () => { + let callCount = 0; + fs.writeFileSync( + path.join(collectionPath, 'cached.js'), + ` + module.exports = { count: ${++callCount} }; + ` + ); + + const script = ` + const mod1 = require('./cached'); + const mod2 = require('./cached'); + bru.setVar('same', mod1.count === mod2.count); + `; + + const context = { + bru: { setVar: jest.fn() }, + console: console + }; + + await runScriptInNodeVm({ script, context, collectionPath, scriptingConfig: {} }); + + expect(context.bru.setVar).toHaveBeenCalledWith('same', true); + }); + }); +});