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
Compatibility with --disallow-code-generation-from-strings #41
Comments
I see there was a similar discussion about this 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 I'm sure there are lots of ways to reduce the duplication; this is just intended as a quick summary of the basic idea. |
Another possibility which may or may not work here; Object.defineProperty(deprecatedfn, 'length', {value: fn.length}) This successfully changes the |
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 👍 |
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 To clarify, the flag I'm talking about here doesn't do any static analysis, it just causes 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. |
@dougwilson I updated the CI scripts to run with and without I also had to make one of the tests explicitly ignore this error (it checks behaviour when used with All of these tests are still run in every environment, because the tests are run with and without 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) |
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. |
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 |
@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 However you want to reshuffle the |
I updated it to use I also reshuffled the commands to give another example of how this could be done. Now |
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
This could be fixed in multiple ways:
EvalError
exception which gets thrown in this environment and fall-back to a simpler alternative; orEvalError
and fall-back to a pass-through (just returnfn
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 letexpress
run with--disallow-code-generation-from-strings
.The text was updated successfully, but these errors were encountered: