Drop eval usage about nodejs-depd HOT 21 CLOSED

Hi! Always feel free to make a pull request if you are able to remove the eval yet keep the function arity (I should have tests that validate if you break anything on accident) :)

from nodejs-depd.

As far as I know,it is not possible to preserve arity without eval, which is why I used it. I'm going to close the issue, since there is nothing I can do by myself. If you have an idea of how to approach this please let me know, or better yet, make a proposal via a pull request :)

from nodejs-depd.

If you could tell me what the problem is if we remove eval, perhaps I could help us solve it

from nodejs-depd.

You need to preserve the original function's arity.

from nodejs-depd.

I must be misunderstanding, doesn't this solve the problem? The only side effect we will have is a bit of performance hit, but it's a deprecated function, not recommended anyway so I think it's fair

function proxy() {
  console.log('this thing is deprecated')
  realFunction.apply(this, arguments)
}

from nodejs-depd.

Yes, you are misunderstanding the problem. You need to preserve the original function's arity, which your example does not (it changes the function arity to zero, no matter what the original arity was). This is a feature of this module and will not be removed, because there are APIs that check function length, and if that function is deprecated, it should not have a different length simply because it is deprecated. This would break APIs, which is the entire point of this module: not to break APIs. A very popular module that requires function arity to stay intact is "async", for example.

from nodejs-depd.

I am trying to use the express and send modules in an electron app and hitting this error, thought it could be fixed. I guess not

from nodejs-depd.

There are workarounds like the loophole module, but I thought to fix it the right way. Thank you for your cooperation :)

from nodejs-depd.

Regardless, I don't think I'm asking for you to do much: make a pull request with your proposed change. Do not remove or break any existing tests. Your PR should be accepted.

from nodejs-depd.

I'm not familiar with electron apps. Do those run in Node.js? If not, you should consider building with a tool chain that uses the browser version of this module, which does not use eval.

from nodejs-depd.

Electron is basically Chromium + Node.js so it has the node's native require, therefore instead of browser it uses the node version. It's some security feature of Chromium that it does not allow unsafe eval within apps. I am using the loophole to make this module work now and it's working so far

from nodejs-depd.

I see this loophole uses the vm module. I'll take a look into this yo see if it can be used instead of eval directly in this module.

Since this module has never had dependencies and supports back to Node.js 0.6 not sure how feasible making that change will be, though.

from nodejs-depd.

So @steelbrain I was just looking into loophole and I tried replacing the eval usage in this module with vm.runInThisContext, but that does not work (at least as a straight replacement) due to the error ReferenceError: log is not defined when the returned function is actually ran. It looks like this is because according to the Node.js documentation, the code does not have access to the local scope, only global, so that function has no access to anything in our codebase.

from nodejs-depd.

Thanks for looking into this. My modules weren't using a deprecated function so I never encountered that error, my modules were merely requireing express so it replaced all the evals invoked by top level code to vm.runInThisContext

from nodejs-depd.

Gotcha. I'm still investigating this approach to using vm or something else to replace eval when possible. I'll let you know what I find out :) If you can also help, that would be great to move it along, but otherwise I think this can possibly be solved in some way to remove eval but keep providing preserved arity on wrapped functions.

from nodejs-depd.

@dougwilson Have you considered something like this?

function wrapFunction(fn) {
  var wrapperFunctions = [
    function() {
      return fn.apply(this, arguments);
    },
    function(arg) {
      return fn.apply(this, arguments);
    },
    function(arg, arg) {
      return fn.apply(this, arguments);
    },
    // ... 9-arity or so should be high enough for almost everything.
  ];
  return fn.length < wrapperFunctions.length
    ? wrapperFunctions[fn.length]
    : wrapFunctionUsingEval(fn);
}

from nodejs-depd.

Hi @LukeStebbing there is a PR right now to that effect, but that does not solve the issue at hand here. The appearance of the "eval" function at all is the issue, not if it appears within the used code path. That wouldn't solve the issue is "eval" or "new Function" is still in the source code so electron will not run the code.

from nodejs-depd.

@dougwilson Interesting, I always assumed that CSP used v8::Context::AllowCodeGenerationFromStrings() under the hood, and I think that allows eval as long as it doesn't appear in any function that's actually invoked. I'm not completely sure about that though.

Do you think it'd be workable to support arity up to a fairly high value and then throw an error if attempting to deprecate too-high arity of a function?

from nodejs-depd.

I assume we can deprecate using the API for above some value and run a deprecation cycle within this module (the same users would use this module itself for) to give ample time to migrate to whatever the new API is, provide clear path to migrate and what users need to do instead and then a few months later release a new major version with the new behavior.

from nodejs-depd.

I'm not familiar with how CSP works, but also mot familiar with how electron apps work; just going what people are saying because I haven't been provided any working examples and instructions on how to reproduce whatever the issue is that is trying to be solved here, so I can't experiment with different solutions to thr problem in any way. Are you also having an issue with Electron App or something else?

from nodejs-depd.

In my case, it was happening in a patched version of Node.js that calls v8::Context::AllowCodeGenerationFromStrings(). That code isn't open source, so unfortunately I can't easily provide a repro, but the symptom is that the send module throws EvalError: Code generation from strings disallowed for this context when SendStream.prototype.etag is defined: https://github.com/pillarjs/send/blob/ff0d82ee71ad884966f062f75d5b2b2a520ddd59/index.js#L183

I can simply enable eval in this case, so it's not a blocker for me, but I figured I'd make a drive-by comment just in case it was helpful. :)

from nodejs-depd.