-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Refactor Inquire so it can resolve modules in Browsers with default CPS #1548
base: master
Are you sure you want to change the base?
Refactor Inquire so it can resolve modules in Browsers with default CPS #1548
Conversation
* @memberof util | ||
* @param {string} moduleName Module to require | ||
* @returns {?Object} Required module if available and not empty, otherwise `null` | ||
*/ | ||
function inquire(moduleName) { | ||
try { | ||
var mod = eval("quire".replace(/^/,"re"))(moduleName); // eslint-disable-line no-eval | ||
var mod = require(moduleName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original idea here was to have a way to require
only if require
is present (on node), in turn making it necessary that bundlers do not recognize this, yet still having protobuf.js as a dependency somewhere in a graph doesn't blow up. With this change, the common case seems to become that stuff blows up, unless every dependent goes through the hassle of changing their configs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dcodeIO Yes, as you say, I have kept the original intended behavior by setting the externals
in the package.json, which creates a trade-off between using eval() and having to set configs. If that's not acceptable (I understand - it adds maintenance effort), could we create a distribution of ProtobufJS intended for the browsers that does not use eval at all, called Protobufjs/minimal-browser
? It seems if you are okay with inquire
not working in most browsers, then there's no reason to include it.
This should be easy to configure with gulp: we can replace inquire with a dummy function that always returns null. As I see it, there's no point to calling eval() if it is blocked by the browser, and the message that the browsers print make people think that there is a problem. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dcodeIO Following up on this one more time - I think no config changes are needed by dependent projects. The changes to package.json
in this repo prevent dependents from blowing up.
I'm sorry I did not clearly address your concern in my last comment. If I can demonstrate that no dependent config changes are needed, would you reconsider this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am mostly out of the loop and it's hard for me to see all the implications. Mostly putting a comment here or there for other reviewers :)
Is there anything we can do here to merge this in and cut a release? This is a pretty important change for use of protobufjs in browsers. @tinder-seanlang-brown Did you end up publishing a fork to npm somewhere? |
Is this PR also aimed to fix the codegen problem regarding CSP? #1483 (comment) |
FYI Due to this issue esbuild throws warnings about using Eval, one per instance of protobufjs being imported. |
We worked around this at bundle generation time using a Webpack alias: alias: {
/**
* https://github.com/protobufjs/protobuf.js/issues/997 The original inquire module contains
* usage of eval, which triggers a CSP violation. Currently we always generates static code
* for protos, so there is no need for any reflection, thus we don't need inquire to work.
*/
"@protobufjs/inquire": path.resolve(__dirname, "src", "patches", "inquire.js"),
}, Where our replacement "use strict";
module.exports = inquire;
function inquire() {
return null;
} |
inquire() cannot resolve any modules in browsers that do not allow
unsafe-eval
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 for inquire.
Problem
While investigating #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:
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 affects the entire page and is not recommended for security. Many less experienced web developers using protobufJS see the warning messages in the browser console and think that they must allow unsafe-eval to use protobufjs, for example #593, #1483.
We can show that inquire() does not work with CPS defaults by simulating the browser environment like this:
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
andbuffer
) 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 a website that depends on protobufjs.Implementation