Compatibility with --disallow-code-generation-from-strings

[davidje13]

This package causes applications to fail to start if using NodeJS's --disallow-code-generation-from-strings security option, even if the application is not using a deprecated function, due to the use of dynamically generated code:

nodejs-depd/index.js

Line 425 in 73364d0

var deprecatedfn = new Function('fn', 'log', 'deprecate', 'message', 'site',

This could be fixed in multiple ways:

• Replace dynamic code generation with a non-dynamic version (I'm not actually sure why it generates an argument list which it doesn't use in the body?); or
• Catch the EvalError exception which gets thrown in this environment and fall-back to a simpler alternative; or
• Catch the EvalError and fall-back to a pass-through (just return fn unchanged), since warning about deprecated functions seems more useful at dev-time than in production anyway.

Since this package is being used by express, it seems especially useful to be able to run with additional security options enabled. This is the only change needed to let express run with --disallow-code-generation-from-strings.

[davidje13]

I see there was a similar discussion about this evaled code in #16 where it's explained that the arg0, arg1, ... stuff is to maintain the function's arity.

One option would be to use the (fairly reasonable) assumption that most functions have relatively low arity;

const wrappers = [
  function (fn, log, deprecate, message, site) {
    return function() {
      log.call(deprecate, message, site)
      return fn.apply(this, arguments)
    }
  },
  function (fn, log, deprecate, message, site) {
    return function(arg0) {
      log.call(deprecate, message, site)
      return fn.apply(this, arguments)
    }
  },
  function (fn, log, deprecate, message, site) {
    return function(arg0, arg1) {
      log.call(deprecate, message, site)
      return fn.apply(this, arguments)
    }
  },
  function (fn, log, deprecate, message, site) {
    return function(arg0, arg1, arg2) {
      log.call(deprecate, message, site)
      return fn.apply(this, arguments)
    }
  }
  // (etc. until you think enough parameters are supported)
]
function fallback() {
  throw new Error('unable to wrap function with so many arguments');
}

// later:

var deprecatedfn = (wrappers[fn.length] || fallback)(fn, log, this, message, site)

Which could be combined with the options of using new Function if available (in that case fallback should probably fail non-destructively)

I'm sure there are lots of ways to reduce the duplication; this is just intended as a quick summary of the basic idea.

[davidje13]

Another possibility which may or may not work here;

Object.defineProperty(deprecatedfn, 'length', {value: fn.length})

This successfully changes the .length property of the function, but I don't know if it does it realistically enough for your needs in this project (I'm not sure where the arity of the function matters so I don't know if this applies to it).

[dougwilson]

Hi, I too also would love to see eval gone.the current plan ia here: #33 (comment)

I created this module for Express which supports too far down, Node.js 0.10. The Express 5 won't, so that's why the idea here is to release this change in conjection with Express to change to this new function length method.

It seems you are indeed just seeing this though Express usage, so that should have you covered.

Please let me know if the length method does not work so we can make a different plan. Also it would be a good idea to add that command line to our test suite, and you're welcome to contibute that as well 👍

[davidje13]

I think all 3 current pull requests are keeping support for all versions of NodeJS though (because they all fall-back in one way or another to using evaled code), so I don't think this needs to be a breaking change at all? (all the PRs are passing the checks in the existing CI environments; the latest from me failed one but that appears to be a flake rather than a real issue, and I have no attachment to it in any case; any of the active PRs would fit my needs).

To clarify, the flag I'm talking about here doesn't do any static analysis, it just causes eval or new Function etc. to throw instead of compiling anything if they are actually called. So it's no problem to have eval around as a fall-back path.

Happy to update the test suite to add this flag in one/all of the PRs. I assume it would just be another CI environment with a recent version of Node and the flag turned on.

[davidje13]

@dougwilson I updated the CI scripts to run with and without eval enabled in each environment that supports it (i.e. NodeJS >= 9).

I also had to make one of the tests explicitly ignore this error (it checks behaviour when used with eval, so the test itself causes eval to be called). It uses this.skip() to mark the test as skipped if eval is disabled. I had to make a similar change to the browserify tests, because they have the same problem (the tests themselves need eval).

All of these tests are still run in every environment, because the tests are run with and without eval enabled for each version.

You can see this change in my PR #42 but it can easily be applied to any of the other PRs. It's just a small change to the package.json and CI config; see 51f1ef8 and a3d3099

(not sure why Node 5.12 is consistently failing in Travis but it's not related to the changes here; some problem installing browserify)

[dougwilson]

Hi @davidje13 you are correct in that they support old versions; I did mention that in the comment I linked to. There are also people who do use static analyzers who want it gone too. There is no reason to keep two different implementations and it's a great opportunity to drop old Node.js versions from this module. That is why this module will just bump requirement to Node.js 4+ and just have the simple function length implementation.

[dougwilson]

Hi @davidje13 it looks like we commented at the same time. My comment above was in response to the comment above your most recent comment.

For your most recent comment, I just took a look at the tests, and it doesn't seem right to me. A user (or CI) should just need to call npm test to run everything; I was thinking about having this switch similar to how the --no-deprecation and similar command line Node.js switches are tested in the test suite for the --disallow-code-generation-from-strings

[davidje13]

@dougwilson it should be possible to do that, but when I tried as my first-pass, I hit problems with the code coverage tool failing (it needs eval itself).

However you want to reshuffle the npm commands should be fine, as long as the code coverage doesn't include the eval-less version. So for example there could be explicit test-eval and test-noeval, with test just invoking both but coverage only using test-noeval (and related changes to which commands are called in CI as well).

[davidje13]

I updated it to use cross-env for Windows support (i.e. appveyor). Turns out that package doesn't support Node 0.8.0, but if you're planning to drop support for that anyway, maybe it doesn't matter.

I also reshuffled the commands to give another example of how this could be done. Now npm test will cause the suite to run both with and without eval allowed, which sounds like it's what you're looking for? (it means that the code coverage commands don't run test any more; they run test-eval instead)