NodeVM customRequire does not correctly resolve conditional exports

nick-klaviyo commented 1 year ago

Issue Description

When using the require.resolve option for NodeVM, NodeVM's resolver does not use conditional exports when resolving the module at the returned path. As a result, it is not possible to require ES modules that only expose a CommonJS entrypoint via the require conditional export path.

Example and Repro Steps

Environment Info

Device: Macbook Pro, Apple M1 Max (2021) Node Version: v18.12.0 Working Directory: /nick/projects/axios

package.json

{
  "name": "repro-steps",
  "version": "1.0.0",
  "main": "index.js",
  "license": "MIT",
  "dependencies": {
    "axios": "1.2.6",
    "vm2": "3.9.13"
  }
}

index.js

const { NodeVM } = require("vm2")

const vm = new NodeVM({
  console: "redirect",
  sandbox: {},
  env: {},
  require: {
    external: true,
    resolve: function (moduleName, dirName) {
      return `./node_modules/${moduleName}`
    },

  }
})

const codeStr = "const axios = require('axios');console.info(axios);"
vm.run(codeStr)

Run

node index.js

Output

/me/axios/node_modules/vm2/lib/resolver-compat.js:29
    return require(/* webpackIgnore: true */ moduleName);
        ^

Error [ERR_REQUIRE_ESM]: require() of ES Module /nick/projects/axios/node_modules/axios/index.js from /nick/projects/axios/node_modules/vm2/lib/resolver-compat.js not supported.
Instead change the require of index.js in /nick/projects/axios/node_modules/vm2/lib/resolver-compat.js to a dynamic import() which is available in all CommonJS modules.
    at DefaultResolver.defaultRequire [as hostRequire] (/nick/projects/axios/node_modules/vm2/lib/resolver-compat.js:29:9)
    at DefaultResolver.loadJS (/nick/projects/axios/node_modules/vm2/lib/resolver.js:204:19)
    at loadJS (/nick/projects/axios/node_modules/vm2/lib/nodevm.js:249:46)
    at VM2 Wrapper.apply (/nick/projects/axios/node_modules/vm2/lib/bridge.js:485:11)
    at Module._extensions.<computed> (/nick/projects/axios/node_modules/vm2/lib/setup-node-sandbox.js:144:48)
    at requireImpl (/nick/projects/axios/node_modules/vm2/lib/setup-node-sandbox.js:117:3)
    at require (/nick/projects/axios/node_modules/vm2/lib/setup-node-sandbox.js:165:10)
    at vm.js:1:77
    at VM2 Wrapper.apply (/nick/projects/axios/node_modules/vm2/lib/bridge.js:485:11)
    at NodeVM.run (/nick/projects/axios/node_modules/vm2/lib/nodevm.js:426:23)
    at Object.<anonymous> (/nick/projects/axios/index.js:17:4) {
  code: 'ERR_REQUIRE_ESM'
}

Node.js v18.12.0

Cause

It looks like the custom resolver wrapper will attempt to load the resolved module path as a file or a directory (https://github.com/patriksimek/vm2/blob/master/lib/resolver-compat.js#L326), which will not result in a check for conditional exports.

Proposed Solution

Use resolver.loadNodeModules instead of resolver.loadAsFileOrDirecotry since loadNodeModules will try to first resolve conditional exports, and then try loading as file or directory.

On https://github.com/patriksimek/vm2/blob/master/lib/resolver-compat.js#L326

-                       return resolver.loadAsFileOrDirecotry(resolved, extList);
+                       return resolver.loadNodeModules(x, [resolver.pathBasename(resolver.pathDirname(resolved))], extList);

This change works for me; however, using resolver.pathBasename(resolver.pathDirname(resolved)) seems kind of weird, so not sure if .loadNodeModules is the best method to use, but the main idea would be to use similar logic that of .loadNodeModules.

XmiliaH commented 1 year ago

I would use the following:

--- a/lib/resolver-compat.js
+++ b/lib/resolver-compat.js
@@ -322,8 +322,13 @@ function resolverFromOptions(vm, options, override, compiler) {
                        }
                        const resolved = customResolver(x, path);
                        if (!resolved) return undefined;
-                       if (externals) externals.push(new RegExp('^' + escapeRegExp(resolved)));
-                       return resolver.loadAsFileOrDirecotry(resolved, extList);
+                       if (typeof resolved === 'string') {
+                               if (externals) externals.push(new RegExp('^' + escapeRegExp(resolved)));
+                               return resolver.loadAsFileOrDirecotry(resolved, extList);
+                       }
+                       const {module=x, path: resolvedPath} = resolved;
+                       if (externals) externals.push(new RegExp('^' + escapeRegExp(resolvedPath)));
+                       return resolver.loadNodeModules(module, [resolvedPath], extList);
                };
        }

Then the resolve method could look like:

resolve: function (moduleName, dirName) {
      return {path: `./node_modules/`};
}

This seems better as using resolver.pathBasename(resolver.pathDirname(resolved)) makes assumptions about the result that may not hold.

nick-klaviyo commented 1 year ago

Thanks for the quick response @XmiliaH ! I implemented the change you described in #505 along with a test case. Let me know if there's any other changes I should make.

patriksimek / vm2