Skip to content
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.

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

Remove eval from inquire #1

Closed
wants to merge 1 commit into from

Conversation

seanlangbrown
Copy link
Owner

@seanlangbrown seanlangbrown commented Feb 5, 2021

Summary

inquire() will always return null in browsers with default CPS settings, even if the requested module is present. This PR fixes that issue while maintaining all desired bundling behavior or inquire.

Problem

While investigating protobufjs#1483 I found that browsers are blocking the execution of eval() in inquire(). This repl reproduces and investigates the issue.

The purpose of inquire is to require a module if it is already available in the environment, but "hide" the module from bundlers so that it is not included as a dependency. To do this, inquire js used eval() and regex as a workaround so that bundlers do not notice the require call during static code analysis:

function inquire(moduleName) {
    try {
        var mod = eval("quire".replace(/^/,"re"))(moduleName); // eslint-disable-line no-eval
        if (mod && (mod.length || Object.keys(mod).length))
            return mod;
    } catch (e) {} // eslint-disable-line no-empty
    return null;
}

Unfortunately, today's browsers have CPS settings that block eval() by default, so inquire() cannot require any modules at all (including modules that are available in the environment). Changing the defaults so that 'unsafe-eval' is allowed, which affects the entire page and is not recommended for security. In addition to the functional issues, many less experienced web developers using protobufJS see warning messages in the browser console and think that they must allow unsafe-eval to use protobufjs, for example protobufjs#593, protobufjs#1483.

We can show that inquire() does not work with CPS defaults by simulating the browser environment like this:

function runInquireInBrowser(moduleName) {
  var blockedEval = function(){throw Error("Unsafe-Eval is blocked")};
  var browserCSPDefault = { eval: blockedEval, "Function": blockedEval };
  
  with (browserCSPDefault) {
      try {
          var mod = eval("quire".replace(/^/,"re"))(moduleName); // EXCEPTION: "Unsafe-Eval is blocked"
          if (mod && (mod.length || Object.keys(mod).length))
              return mod;
      } catch (e) {} // EXCEPTION is caught
        return null; // always returns null
  }
}
const result = runInquireInBrowser('fs');

We can reason that result will always be null.

Solution

Using eval() for this made sense 10+ years ago. There is now an accepted "standard" way of configuring "externals" so dynamic requires can be made while excluding those modules from the bundles. It is implemented by all major bundlers including webpack, browserify (gulp), rollup, and possibly others. By using this standard we can exclude inquired modules (long and buffer) from protobufjs distributions.

There is also now a way to instruct bundlers not to include dependencies of protobufJS distributions: https://github.com/defunctzombie/package-browser-field-spec#ignore-a-module. This can be used to prevent webpack from bundling long with my website that uses protobufjs.

Implementation

  • New test cases for inquire() in a simulated browser environment with eval blocked
  • Removed eval() from inquire
  • Added all modules loaded with inquire() to the broswerify config "externals" to prevent protobufJS from including inquired modules in it's bundle.
  • Added all modules loaded with inquire() to the protobufJS package.json "browser" field so that any project that uses protobufJS and a bundler will not add inquired modules to it's bundle.

@seanlangbrown seanlangbrown force-pushed the seanLB.removeEvalFromInquire branch 2 times, most recently from 309a2eb to 758d37e Compare February 5, 2021 22:03
@seanlangbrown
Copy link
Owner Author

Closing this draft to open a PR against the public repo: protobufjs#1548

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants