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, as it poses security risks #44

Closed
benbucksch opened this issue Jul 15, 2021 · 9 comments
Closed

Comments

@benbucksch
Copy link

benbucksch commented Jul 15, 2021

Environment

  • Sapper

Reproduction

  • Add express to your project, directly or indirectly, which adds depd to your project
  • yarn run dev
  • Change any source code file.

Actual result

On every code file change, the compiler spits out the following warning on the console:

Use of eval is strongly discouraged, as it poses security risks and may cause issues with minification
408: 
409:    // eslint-disable-next-line no-eval
410:   var deprecatedfn = eval('(function (' + args + ') {\n' +
                          ^
411:     '"use strict"\n' +
412:     'log.call(deprecate, message, site)\n' +

This is due to depd.

Expected result

eval() is not used at all. The error message is correct.

@benbucksch benbucksch changed the title Use of eval is strongly discouraged, as it poses security risks Use of eval() is strongly discouraged, as it poses security risks Jul 15, 2021
@dougwilson
Copy link
Owner

The usage of eval has already been removed from this module, so there wouldn't be any further changes to remove it as it no longer exists.

@dougwilson
Copy link
Owner

@benbucksch
Copy link
Author

benbucksch commented Jul 15, 2021

The official doc for eval() has a big fat yellow warning at the top, saying:

Warning: Executing JavaScript from a string is an enormous security risk. It is far too easy for a bad actor to run arbitrary code when you use eval(). See Never use eval()!, below.

image

There's a certain irony in the fact that the very module which warns other developers about deprecated code uses deprecated code itself. Not only that, but the most well-known deprecated function and kind-of the mother of all deprecated functions in JavaScript.

@dougwilson
Copy link
Owner

Hi @benbucksch you can view the current code for this module right here on GitHub... There is no eval() usage. It was removed 3 years ago from this module.

@benbucksch
Copy link
Author

benbucksch commented Jul 15, 2021

Oh, right...

So, why does express depending explicitly on an outdated depd version, even in the latest version of express?

https://github.com/expressjs/express/blob/28db2c2c5cf992c897d1fbbc6b119ee02fe32ab1/package.json#L39

    "depd": "~1.1.2",

The other deps are also "~" instead of "^". Are they just being silly?

@dougwilson
Copy link
Owner

Hi @benbucksch there are hundreds of modules that depend on this module. The downstream modules (like this one) do not have control over how others decide to use it. Of course, the removal of eval was a major version change in this module, so perhaps there is some kind of incompatibility with it or something, I'm not sure off-hand. Even if it was ^1.1.2 it would still not pick up the 2.0.0 release in that version range.

@benbucksch
Copy link
Author

benbucksch commented Jul 15, 2021

@dougwilson : Yes, I understand that, but express is a rather popular module. I now tried to file a bug against express, but for some reason, I cannot ("You can't perform that action at this time."). I see that you made the most recent commits in express, so it's not a third party module from your perspective, but you're active in express as well. Could you see to it that this is fixed in express, please?

@dougwilson
Copy link
Owner

Hi @benbucksch sure, I will take a look in to it when I get some time. In the future, please try to keep issues to the respective issue tracker they belong in; perhaps GitHub is having an issue at the moment or something. I would move the issue, but issues cannot be moved across organizations.

This issue is closed, but mainly because there is no issue in this module as the usage of eval has already been removed and published to npm as an update for dependent modules to upgrade to as they can.

@benbucksch
Copy link
Author

benbucksch commented Jul 15, 2021

Yup, sorry about that. I didn't realize that I was using an outdated dependency.

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

2 participants