[Bug]: NodeJS vm code + 'eval' needlessly included in web bundle (Causing CSP errors)

[SunsetFi]

Describe the bug

We have an in-house microfrontend framework, and we are in the process of adopting @module-federation/vite to deduplicate our shared libraries and reduce the network overhead and browser memory usage.

However, on our trial projects, we are encountering an odd issue: The process of using @module-federation/vite is causing our browser code to gain an eval() statement.

This is specifically called out by rollup in a warning:

node_modules/@module-federation/sdk/dist/index.cjs.js (670:83): Use of eval in "node_modules/@module-federation/sdk/dist/index.cjs.js" is strongly discouraged as it poses security risks and may cause issues with minification.

Even weirder, on further investigation, this eval() statement seems only relevant for NodeJS, and is never touched or invoked by our web project.

A few things stand out here:

Why is rollup consuming the CJS bundle?

@module-federation/sdk has the proper entries in its package.json to specify ESM builds, which are far more tree-shakable. But, vite seems to be consuming the cjs build instead...

Is it really being included in the bundle?

It's hard to tell with minify on, but yes:
{filename,importModuleDynamically:(_vm_constants_USE_MAIN_CONTEXT_DEFAULT_LOADER=(_vm_constants=vm.constants)==null?void 0:_vm_constants.USE_MAIN_CONTEXT_DEFAULT_LOADER)!=null?_vm_constants_USE_MAIN_CONTEXT_DEFAULT_LOADER:importNodeModule});script.runInThisContext()(scriptContext.exports,scriptContext.module,eval("require"),urlDirname,filename);const exportedInterface=scriptContext.module.exports||scriptContext.exports;if(attrs&&exportedInterface&&attrs.globalName){const o=exportedInterface[attrs.globalName]

With a bit of cross-eyed staring, this code is definitely the createScriptNode function that includes our rogue eval statement, and should in theory be totally unnecessary for a web project.

If we force rollup to consume the mjs bundle, does it tree-shake away the NodeJS code?

...No. We tried this using a resolve alias:

resolve: {
    alias: {
      "@module-federation/sdk": import.meta.resolve("@module-federation/sdk"),
    },
  },

Which properly resolves to the esm build, but we still get the same error, only pointing at the esm this time:

node_modules/@module-federation/sdk/dist/index.esm.mjs (668:83): Use of eval in "node_modules/@module-federation/sdk/dist/index.esm.mjs" is strongly discouraged as it poses security risks and may cause issues with minification

And we can still see it in the bundle:
{filename,importModuleDynamically:(_vm_constants_USE_MAIN_CONTEXT_DEFAULT_LOADER=(_vm_constants=vm.constants)==null?void 0:_vm_constants.USE_MAIN_CONTEXT_DEFAULT_LOADER)!=null?_vm_constants_USE_MAIN_CONTEXT_DEFAULT_LOADER:importNodeModule});script.runInThisContext()(scriptContext.exports,scriptContext.module,eval("require"),urlDirname,filename);const exportedInterface=scriptContext.module.exports||scriptContext.exports;if(attrs&&exportedInterface&&attrs.globalName){const s=exportedInte

Here's that reproduction: https://github.com/SunsetFi/vite-module-federation-createScriptNode/tree/with-esm

Will @module-federation/sdk tree-shake when used directly?

Yep!

https://github.com/SunsetFi/vite-module-federation-createScriptNode/tree/direct-usage-tree-shake

When we disable module federation and try importing createScript directly, createScriptNode is nowhere to be found in the source, and as such the eval function disappears from the bundled code.

Two things of note here though:

• We still get the error about using eval.
• Vite is properly using the ESM version of the sdk, not the cjs one.

Impact

This is affecting our usage in a few ways:

• We are getting 'eval' code into our web builds, flagging warnings for our security scanners.

• This will cause issues for deploying this code to production due to the Content Security Policy we use.

• We are getting larger builds than we would like, as we are carrying around NodeJS specific code we do not need. Given that this is happening in the runtimeInit chunk that generates for every federated library, this is going to be compounded over the many, many, many microfrontends we are planning on using @module-federation/vite with.

Version

6.2.1

Reproduction

https://github.com/SunsetFi/vite-module-federation-createScriptNode

Relevant log output

node_modules/@module-federation/runtime-core/node_modules/@module-federation/sdk/dist/index.cjs.js (670:83): Use of eval in "node_modules/@module-federation/runtime-core/node_modules/@module-federation/sdk/dist/index.cjs.js" is strongly discouraged as it poses security risks and may cause issues with minification.

Validations

• Read the docs.
• Read the common issues list.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• The provided reproduction is a minimal reproducible example of the bug.

[gioboa]

Hi @SunsetFi thanks for the report and for the advanced description. It seems definitely something to investigate but unfortunately in these weeks I don't have possibility to look at it.
Feel free to drop PRs or possible solutions. Thanks

[SunsetFi]

Okay, so there are two issues at play here:

The first is this line:
https://github.com/module-federation/core/blob/1f1a544eff0b046fe700b9371b5f823888006e01/packages/runtime-core/src/utils/load.ts#L253

Vite (rollup) has no idea that isBrowserEnv() will always return true in our env, and therefor is including NodeJS specific code in our builds. If I replace the isBrowserEnv() call with true, the branch is tree-shaken and most of the node code dissapears.

I'm not entirely sure how to approach the 'fix' to this. Since isBrowserEnv is abstracted away in another module, it seems unlikely that rollup will ever be able to unwind the function call and determine that the value is always false. Possibly we can send a request to @module-federation/core to replace this with a raw typeof window !== undefined or something that rollup can more easily understand?

For the time being, we can fix this with the following hack plugin:

    {
      name: 'is-a-browser',
      enforce: 'pre',
      transform(code, id) {
        if (id.includes('@module-federation/runtime')) {
          // If the ESM build is used, it won't be prefixed with sdk.
          // If the CJS build is used, it will be prefixed with sdk.
          const newCode = code.replace(/(sdk\.)?isBrowserEnv\(\)/g, 'true');
          return Promise.resolve(newCode);
        }
        return Promise.resolve();
      },
    },

This replaces the function call with 'true' before letting rollup get ahold of it, which results in the offending code successfully tree-shaken away.

However.

There is another issue that I have been ripping my hair out over.

@module-federation/runtime re-exports a bunch of stuff from @module-federation/runtime-core, which itself is re-exporting a bunch of stuff from @module-federation/sdk, and no matter how hard I try, I cannot get these exports to tree-shake. This means that loadScriptNode gets re-exported and the entire system is unable to cull that last little bit of NodeJS code.

If I comment out the loadScriptNode exports from here https://github.com/module-federation/core/blob/1f1a544eff0b046fe700b9371b5f823888006e01/packages/runtime/src/index.ts#L13, then poof, no more NodeJS code!

But... Why On Earth Is Rollup Not Tree-Shaking loadScriptNode! Absolutely nothing uses it. Shutting off all minification aside from tree-shaking, doing a text search for it in the build output yields zero callers at all!

What's even more baffling is the form of how it appears. Somehow, this code is getting generated in the runtimeInit chunk:

const index_esm = /* @__PURE__ */ Object.freeze(/* @__PURE__ */ Object.defineProperty({
  __proto__: null,
  FederationHost,
  Module,
  getInstance,
  getRemoteEntry,
  getRemoteInfo,
  init,
  loadRemote,
  loadScript,
  loadScriptNode,
  loadShare,
  loadShareSync,
  preloadRemote,
  registerGlobalPlugins,
  registerPlugins,
  registerRemotes
}, Symbol.toStringTag, { value: "Module" }));

It appears to be every single export from @module-federation/runtime

And then, it's shoved into the exports of that chunk:

export {
  index_esm as a,
  example_extension__mf_v__runtimeInit__mf_v__ as e,
  init as i
};

Elsewhere, index_esm/a IS used, but as far as I can tell, the only thing that is ever used is loadShare:

import { a as index_esm, e as example_extension__mf_v__runtimeInit__mf_v__ } from "./example_extension__mf_v__runtimeInit__mf_v__-CR2jWmIN.js";
import { a as getAugmentedNamespace } from "./index-DxQDj2VD.js";
const require$$0 = /* @__PURE__ */ getAugmentedNamespace(index_esm);
const { loadShare } = require$$0;
const { initPromise } = example_extension__mf_v__runtimeInit__mf_v__;
const res = initPromise.then((_) => loadShare("react", {
  customShareInfo: { shareConfig: {
    singleton: false,
    strictVersion: true,
    requiredVersion: "^17.0.2"
  } }
}));

So... why on earth is it bundling up every single export? Why not just export loadShare and let the others tree-shake?

(It's also worth mentioning that I had to go add "sideEffects": false to the package.json of @module-federation/runtime and @module-federation/runtime-core. I'm not too sure if thats the appropriate value, it probably needs to be an array specifically pointing at the index file, given that both packages have other entrypoints that might actually have side-effects.)

[gioboa]

That's an incredible investigation, thanks for doing that. 👏👏
cc @ScriptedAlchemy

[SunsetFi]

Why is the CommonJS build of @module-federation/runtime used?

@module-federation/vite does not properly expose its ESM build

@module-federation/vite is lacking a `module' property in its package.json pointing at its esm build. Because of this, The CommonJS build of @module-federation/vite is always used

As a direct result of this, when it goes to fetch the path to the runtime, require.resolve is used:
implementation: options.implementation || require.resolve('@module-federation/runtime'),
which naturally resolves to the entrypoint of the CommonJS build of @module-federation/runtime

@module-federation/vite ESM build is broken

I tried reconfiguring the package.json to use the included index.esm.js, but this file is malformed; it tries to use 'require' in several places (such as normalizeShareItem), which does not exist in an esm context, resulting in thrown errors while building.

Interestingly, I was forced to add an "exports" section to the package.json to get it to even consider loading the esm file; simply adding a "module" entry pointing at the esm build wasn't enough, and it was still using the CommonJS version.

@module-federation/vite can be coerced into using the ESM runtime through implementation

Despite all this, you can still convince the plugin to use the ESM build of the runtime through the implementation property:

    federation({
      name: "federated_module",
      filename: "remoteEntry.js",
      // This causes us to use the mjs runtime, instead of the cjs runtime.
      // This is because the vite plugin's default uses require.resolve instead of import.meta.resolve
      implementation: import.meta.resolve("@module-federation/runtime"),
      exposes: {
        "./federated-module": "./src/federated-module.tsx",
      },
      shared: {
        react: { strictVersion: true },
        "react/": { strictVersion: true },
        "react-dom": { strictVersion: true },
      },
    }),

While this doesn't solve the issue yet, it seems to be a critical step, as ESM is significantly easier to tree-shake than CommonJS.

[SunsetFi]

I found the last piece of the puzzle. For some unexplained reason, @module-federation/runtime is being interpreted as CommonJS, and is therefor getting wrapped in a CommonJS-to-ESM shim, preventing tree shaking.

Since the module is ESM to begin with, we can tell vite's CommonJS handler to ignore this file:

build: {
    commonjsOptions: {
      ignore(id) {
        if (id.includes("@module-federation/runtime")) {
          return true;
        }
        return false;
      },
    },
}

When combined with my last post's ESM hack (implementation: import.meta.resolve(...)), and with the isBrowserEnv rewrite hack, we now have fully tree-shaken @module-federation/runtime and its dependencies, and no longer have NodeJS code in our build!

Whew...

So, things that need fixing:

• @module-federation/vite needs to export a working ESM build, so that the proper ESM version of the runtime is used by default. This is required for tree-shaking, as CommonJS doesn't do so hot with that.
• Either @module-federation/runtime needs to reimplement its use of isBrowserEnv with something that vite+esbuild will tree-shake natively, or @module-federation/vite should implement the transform in my first post to replace it with true to let the NodeJS code tree-shake out
• We need to figure out why on earth vite is thinking that @module-federation/runtime is CommonJS code. As far as I can tell, 100% of that work is done by @rollup/plugin-commonjs, so that is probably the place to start looking.

Here is my successfully-building example, implementing all hacks to get proper tree-shaking:
https://github.com/SunsetFi/vite-module-federation-createScriptNode/tree/hack-fix

Try playing around with it, searching the dist folder for "eval(". The removal of any 1 of the 3 hacks will either cause eval to show up, or in the case of removing the implementation hack while keeping the no-commonjs hack, result in a nonfunctional build.

I have not yet tested this for actual runtime functionality yet, however. That will have to wait until tomorrow.

[SunsetFi]

Aha! I found the root issue!

So first of all, my hacks above do not result in a functioning build. There is a very good reason rollup is assuming @module-federation/runtime is CommonJS.

That reason is the root of the bug: @module-federation/vite is using require statements, not import statements!

See:

vite/src/virtualModules/virtualRemotes.ts

Line 28 in b425fe5

const {loadRemote} = require("@module-federation/runtime")

vite/src/virtualModules/virtualShared_preBuild.ts

Line 45 in b425fe5

const {loadShare} = require("@module-federation/runtime")

These two lines using require instead of import is what is forcing the runtime to be interpreted as CommonJS and preventing any form of tree-shaking for it. If we stop it doing that with my ignore() hack, the build breaks, because require() is never transformed and doesn't exist on the browser.

Given that vite is ESM centric, and all of its build tools are designed to consume ESM, I believe the fix here is to replace these require() calls with import() calls on all builds of @module-federation/vite, even the CommonJS builds.

I will start preparing some PRs for these.

[gioboa]

I will start preparing some PRs for these.

@SunsetFi You rock 👏 Go for it

[tiagobnobrega]

relates to: #156

[SunsetFi]

So this is going to be a much bigger project than I realized. I believe I might still tackle it but it depends on work priorities.

loadShare modules need to be ESM to tree-shake, but this interferes with CommonJS-to-ESM shimming

Rollup/ESBuild needs to see "@module-federation/runtime" as an ESM import in order to know to tree-shake it. But having any import statement in a js file turns it into an ESM file.

Since it turns the file into an ESM file, this little line becomes a huge deal:

vite/src/virtualModules/virtualShared_preBuild.ts

Line 54 in b425fe5

module.exports = exportModule

We can't "set" the exports of our module if we are ESM; the exports need to be resolvable statically.

I had tried solving this by asking vite to load the original module and to get the exports, and code-generating export const MyExport = exportModule.MyExport. This worked great for ESM targets!

However, CommonJS packages will choke on this. React in particular was Not Happy. Vite will tell us that react:

• has a default export
• exports 'default' and '*'

So following our logic, the best we could really do is
export default exportModule.default

But that causes the build to break! Somehow, @rollup/plugin-commonjs is either bridging the CommonJS-ESM named export gap, or maybe even masking it entirely to ESBuild/Rollup. Either way, converting React's shim into ESM causes ESBuild to start throwing errors like Cannot find export 'jsx' from "...react_loadShare-aaaa.js". It's easy to see why; our ESM module only has a default export. However, that's true without any of our changes too, so @rollup/plugin-commonjs must be doing something special to bridge this gap, and if we are going to turn react's loadShare into an ESM module we will have to do something similar.

What can we do about this?

We kinda have two options here:

Simple

Make a new pre-tree-shaken entrypoint in @module-federation/runtime that is web-only.

This kinda has precedent, in that @module-federation/runtime/embedded seems to be an entrypoint for a dedicated webpack environment. We could make a @module-federation/runtime/web that has only the required bits for us to use, and then we could require() that in our shims.

Complex

Make the shim system always use ESM modules instead of always using CommonJS modules:

1. Use the vite hook functions this.resolve() plus this.load() to retrieve information on each shared module. This gives us that module's exports.

• Its super easy to get the exports from vite: const { exports, hasDefaultExport } = await this.load(this.resolve(source, importer));
• Of note, this means that we will NOT be able to do this in resolve.alias.customResolver, as this.load is async and customResolver does not support promise return values.
• We probably want to use resolveId() for this, since that can return a promise.

2. Generate virtual ES Modules for every shared library to do the equivalent of writeLoadShareModule

• For ESM targets, import them and code-generate exports for all of its exports (plus the default, if present).
• For CommonJS targets, require() them and export their module.exports as the ESM default export

3. Transform the source / input modules to point the imports at our shim module from step 2

• For ESM, this is just replacing the import path
• For CommonJS, we need to change the import { a, b } from "x" to import __thing from "x"; const { a, b } = __thing;. I am pretty sure this is what @rollup/plugin-commonjs does, but I need to go digging to find out. There's probably intricacies around namespace collisions.
• This step should be done in a transform hook, since such rewrites are beyond the scope of require.alias

All things considered, a good approach might be to take resolveId(), make it return a resolution to our shim, and have our shim reference a "myModule?original" ID that we then look for in the load() hook, where we return this.load() on our original module's resolution result. This might allow us to skip step 3 in its entirety. Maybe... Depends on the CommonJS thing.

I'm of two minds about this solution...

Getting the shims to be ESM would be nice as it's what Vite wants to work with, and its possible that ESBuild might be a bit better at optimizing them if that's the entrypoint. The end result of what is described above might even be cleaner, as we will be directly transforming the build process rather than (ab)using the require alias system.

On the other hand, this is a lot of work and major refactor just to tree shake out a few NodeJS specific calls and evil little eval().

Open Questions

What is the VirtualModule and filesystem writing for? Is there a reason we can't create virtual modules through the resolveId() and load() hooks without ever hitting the disk?

[gioboa]

Thanks for your effort @SunsetFi
I would like to have an opinion from you please 🙏
@zhangHongEn @underfin @patak-dev