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

Use of eval is strongly discouraged #1754

Open
imoye opened this issue Jun 23, 2022 · 16 comments
Open

Use of eval is strongly discouraged #1754

imoye opened this issue Jun 23, 2022 · 16 comments

Comments

@imoye
Copy link

imoye commented Jun 23, 2022

protobuf.js version: 6.10.2

https://rollupjs.org/guide/en/#avoiding-eval
node_modules/@protobufjs/inquire/index.js

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;
@gunta
Copy link

gunta commented Feb 3, 2023

+1

@edevil
Copy link

edevil commented May 12, 2023

eval() is also not available on certain environments such as Cloudflare Workers, which prevents this module, or anything that depends on it, from being used.

@kaiyoma
Copy link

kaiyoma commented Jun 23, 2023

Getting these errors during a Vite build:

00:22:03  common/temp/node_modules/.pnpm/@protobufjs+inquire@1.1.0/node_modules/@protobufjs/inquire/index.js (12:18) Use of eval in "common/temp/node_modules/.pnpm/@protobufjs+inquire@1.1.0/node_modules/@protobufjs/inquire/index.js" is strongly discouraged as it poses security risks and may cause issues with minification.
00:23:24  common/temp/node_modules/.pnpm/@protobufjs+inquire@1.1.0/node_modules/@protobufjs/inquire/index.js (12:18) Use of eval in "common/temp/node_modules/.pnpm/@protobufjs+inquire@1.1.0/node_modules/@protobufjs/inquire/index.js" is strongly discouraged as it poses security risks and may cause issues with minification.
00:24:32  common/temp/node_modules/.pnpm/@protobufjs+inquire@1.1.0/node_modules/@protobufjs/inquire/index.js (12:18) Use of eval in "common/temp/node_modules/.pnpm/@protobufjs+inquire@1.1.0/node_modules/@protobufjs/inquire/index.js" is strongly discouraged as it poses security risks and may cause issues with minification.
00:24:42  common/temp/node_modules/.pnpm/@protobufjs+inquire@1.1.0/node_modules/@protobufjs/inquire/index.js (12:18) Use of eval in "common/temp/node_modules/.pnpm/@protobufjs+inquire@1.1.0/node_modules/@protobufjs/inquire/index.js" is strongly discouraged as it poses security risks and may cause issues with minification.

@oleksandr-dukhovnyy
Copy link

It's not entirely clear why this is needed at all?
Why not var mod = require(moduleName);?

@Assafz1983
Copy link

Hi Protobufjs team!
Are you planning on removing the usage of eval in the near future?

@tsemachh
Copy link

Same goes for Chrome Extensions using Manifest V3 - where it's also forbidden to use eval

@whizzzkid
Copy link

☝🏽 absolutely, had to patch to run on MV3.

@davideberlein
Copy link

This is a huge issue for us as the page we are integrating into forbids eval via CSP

@AntiMoron
Copy link

same here

@AntiMoron
Copy link

#1941

I give it a try, more tests are needed.

@kaiyoma
Copy link

kaiyoma commented Dec 7, 2023

In addition to the Vite errors, this issue causes runtime CSP errors in Firefox:

Content-Security-Policy: The page’s settings blocked the loading of a resource at eval (“script-src”).

Clicking the link in the browser console error takes you here:

image

@ngbrown
Copy link

ngbrown commented Jan 25, 2024

I use this patch-package file:

patches/protobufjs+7.2.5.patch
diff --git a/node_modules/protobufjs/dist/light/protobuf.js b/node_modules/protobufjs/dist/light/protobuf.js
index 5727c45..3004e3d 100644
--- a/node_modules/protobufjs/dist/light/protobuf.js
+++ b/node_modules/protobufjs/dist/light/protobuf.js
@@ -876,6 +876,10 @@ module.exports = inquire;
  * @returns {?Object} Required module if available and not empty, otherwise `null`
  */
 function inquire(moduleName) {
+    // Don't use eval with CSP in a browser: https://github.com/protobufjs/protobuf.js/pull/1548
+    if (typeof document !== "undefined") {
+        return null;
+    }
     try {
         var mod = eval("quire".replace(/^/,"re"))(moduleName); // eslint-disable-line no-eval
         if (mod && (mod.length || Object.keys(mod).length))
diff --git a/node_modules/protobufjs/dist/minimal/protobuf.js b/node_modules/protobufjs/dist/minimal/protobuf.js
index 87e6f55..d5e2d9e 100644
--- a/node_modules/protobufjs/dist/minimal/protobuf.js
+++ b/node_modules/protobufjs/dist/minimal/protobuf.js
@@ -658,6 +658,10 @@ module.exports = inquire;
  * @returns {?Object} Required module if available and not empty, otherwise `null`
  */
 function inquire(moduleName) {
+    // Don't use eval with CSP in a browser: https://github.com/protobufjs/protobuf.js/pull/1548
+    if (typeof document !== "undefined") {
+        return null;
+    }
     try {
         var mod = eval("quire".replace(/^/,"re"))(moduleName); // eslint-disable-line no-eval
         if (mod && (mod.length || Object.keys(mod).length))
diff --git a/node_modules/protobufjs/dist/protobuf.js b/node_modules/protobufjs/dist/protobuf.js
index cda26c5..012e2f5 100644
--- a/node_modules/protobufjs/dist/protobuf.js
+++ b/node_modules/protobufjs/dist/protobuf.js
@@ -876,6 +876,10 @@ module.exports = inquire;
  * @returns {?Object} Required module if available and not empty, otherwise `null`
  */
 function inquire(moduleName) {
+    // Don't use eval with CSP in a browser: https://github.com/protobufjs/protobuf.js/pull/1548
+    if (typeof document !== "undefined") {
+        return null;
+    }
     try {
         var mod = eval("quire".replace(/^/,"re"))(moduleName); // eslint-disable-line no-eval
         if (mod && (mod.length || Object.keys(mod).length))
diff --git a/node_modules/protobufjs/src/util.js b/node_modules/protobufjs/src/util.js
index 6c50899..bd9a61d 100644
--- a/node_modules/protobufjs/src/util.js
+++ b/node_modules/protobufjs/src/util.js
@@ -199,6 +199,7 @@ util.setProperty = function setProperty(dst, path, value) {
     return setProp(dst, path, value);
 };
 
+if (!util.hasOwnProperty("decorateRoot")){
 /**
  * Decorator root (TypeScript).
  * @name util.decorateRoot
@@ -210,3 +211,4 @@ Object.defineProperty(util, "decorateRoot", {
         return roots["decorated"] || (roots["decorated"] = new (require("./root"))());
     }
 });
+}

The if (!util.hasOwnProperty("decorateRoot")) part is to prevent an error of re-defining the same property when using HMR.

@jakub-g
Copy link

jakub-g commented Feb 14, 2024

To try to move forward with this and close the Content Security Policy threads like #997, could some maintainer explain what problem the usage of eval inside inquire resolves precisely?

❓ Maybe the problem is no longer relevant?

By doing some archeology, it seems that eval was shipped many years ago due to webpack 4 automagically bundling Buffer for the web when it noticed require.

Note that webpack 5 (released Nov 2020) has broke compat on this, and longer automagically ships nodejs polyfills.

So if that's the only reason, I'd suggest to cut a major version of protobufjs (8.0), and remove the usage of eval.

@KAMAELUA
Copy link

Any updates? This completely breaks packaging ESM package because require is not defined and this eval is always running it.

@alaingiller
Copy link

I also got the problem while using @opentelemetry/exporter-trace-otlp-proto so I switch to @opentelemetry/exporter-trace-otlp-http. Less performant, but more secure at least...

@graham-atom
Copy link

I am also seeing this warning when building with vite, any updates?

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

No branches or pull requests