Fix an inefficient regex in autoInject#1767
Conversation
|
This is open source and no one owes anybody anything, but is there anything that might encourage a review of this PR? |
aearly
left a comment
There was a problem hiding this comment.
Can you add a test to make sure comments in argument definitions are still properly stripped? Both (inline /* remove me */) => {} and
(
// remove this
singleLine
) => {}
| it('should properly strip comments in argument definitions', () => { | ||
| var foo = | ||
| `(inline /* remove me */) => {return a}` + | ||
| `( | ||
| // remove this | ||
| singleline | ||
| ) => {return a}` | ||
| expect (() => async.autoInject({ | ||
| ab (a) { | ||
| a = foo | ||
| return a; | ||
| } | ||
| })).to.deep.eql( | ||
| `(inline ) => {return a}` + | ||
| `( | ||
|
|
||
| singleline | ||
| ) => {return a}` | ||
| ) | ||
| }); |
There was a problem hiding this comment.
Here is a test that will fail if either halves of the regular expression are not working:
| it('should properly strip comments in argument definitions', () => { | |
| var foo = | |
| `(inline /* remove me */) => {return a}` + | |
| `( | |
| // remove this | |
| singleline | |
| ) => {return a}` | |
| expect (() => async.autoInject({ | |
| ab (a) { | |
| a = foo | |
| return a; | |
| } | |
| })).to.deep.eql( | |
| `(inline ) => {return a}` + | |
| `( | |
| singleline | |
| ) => {return a}` | |
| ) | |
| }); | |
| it('should properly strip comments in argument definitions', (done) => { | |
| async.autoInject({ | |
| task1: function(task2, /* ) */ callback) { | |
| callback(null, true); | |
| }, | |
| task2: function task2(task3 // ) | |
| ,callback) { | |
| callback(null, true); | |
| }, | |
| task3: function task3(callback) { | |
| callback(null, true); | |
| } | |
| }, | |
| (err, result) => { | |
| expect(err).to.eql(null); | |
| expect(result).to.deep.eql({task1: true, task2: true, task3: true}); | |
| done(); | |
| }); | |
| }); |
There was a problem hiding this comment.
Thank you for your input @Trott
This looks so much better!
Co-authored-by: Rich Trott <rtrott@gmail.com>
Co-authored-by: Rich Trott <rtrott@gmail.com>
|
@aearly how are we looking here? |
| it('should not be subject to ReDoS', () => { | ||
| // This test will timeout if the bug is present. | ||
| var someComments = 'text/*'.repeat(1000000) | ||
| expect(() => async.autoInject({ | ||
| someComments, | ||
| a () {} | ||
| })).to.throw() | ||
| }); | ||
|
|
||
| it('should properly strip comments in argument definitions', (done) => { | ||
| async.autoInject({ | ||
| task1: function(task2, /* ) */ callback) { | ||
| callback(null, true); | ||
| }, | ||
| task2: function task2(task3 // ) | ||
| ,callback) { | ||
| callback(null, true); | ||
| }, | ||
| task3: function task3(callback) { | ||
| callback(null, true); | ||
| } | ||
| }, | ||
| (err, result) => { | ||
| expect(err).to.eql(null); | ||
| expect(result).to.deep.eql({task1: true, task2: true, task3: true}); | ||
| done(); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
It might be better to move the new tests near the end of the file, or at least somewhere after "basics". I would expect the basic tests to be run before anything checking the efficiency of a regular expression or edge cases involving comments.
There was a problem hiding this comment.
That's a good point @Trott I'll make the changes
| }) | ||
| }) | ||
|
|
||
| it('should not be subject to ReDoS', () => { |
There was a problem hiding this comment.
Is this line indented too far? (I'm surprised if there isn't a lint rule for this.)
This fixes the issues highlighted in https://lgtm.com/projects/g/caolan/async/snapshot/f7b5d714b21bada7bd7a39b75e238bedcd162853/files/lib/autoInject.js?sort=name&dir=ASC&mode=heatmap#L12