Skip to content
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

Merged
merged 7 commits into from Dec 2, 2021
Merged

Fix an inefficient regex in autoInject #1767

merged 7 commits into from Dec 2, 2021

Conversation

meekdenzo
Copy link
Contributor

@meekdenzo meekdenzo commented Nov 5, 2021

@Trott
Copy link
Contributor

@Trott Trott commented Nov 13, 2021

This is open source and no one owes anybody anything, but is there anything that might encourage a review of this PR?

Copy link
Collaborator

@aearly aearly left a 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
) => {}

lib/autoInject.js Outdated Show resolved Hide resolved
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}`
)
});
Copy link
Contributor

@Trott Trott Nov 25, 2021

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:

Suggested change
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();
});
});

Copy link
Contributor Author

@meekdenzo meekdenzo Nov 25, 2021

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!

meekdenzo and others added 2 commits Nov 25, 2021
Co-authored-by: Rich Trott <rtrott@gmail.com>
Co-authored-by: Rich Trott <rtrott@gmail.com>
@meekdenzo
Copy link
Contributor Author

@meekdenzo meekdenzo commented Nov 28, 2021

@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();
});
});

Copy link
Contributor

@Trott Trott Nov 28, 2021

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.

Copy link
Contributor Author

@meekdenzo meekdenzo Nov 28, 2021

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

@@ -224,4 +224,33 @@ describe('autoInject', () => {
done()
})
})

it('should not be subject to ReDoS', () => {
Copy link
Contributor

@Trott Trott Nov 28, 2021

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.)

aearly
aearly approved these changes Dec 2, 2021
@aearly aearly merged commit cdfb491 into caolan:master Dec 2, 2021
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants