-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test to make sure comments in argument definitions are still properly stripped? Both (inline /* remove me */) => {}
and
(
// remove this
singleLine
) => {}
test/autoInject.js
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? |
test/autoInject.js
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point @Trott I'll make the changes
test/autoInject.js
Outdated
@@ -224,4 +224,33 @@ describe('autoInject', () => { | |||
done() | |||
}) | |||
}) | |||
|
|||
it('should not be subject to ReDoS', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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