fix!: Correct mishandled escaped path separators

[Trott]

This change fixes the regular expression denial of service
vulnerability.

Refs: https://app.snyk.io/vuln/SNYK-JS-GLOBPARENT-1016905

[Trott]

With this change, at least locally for me:

• All tests continue to pass.
• The following proof-of-concept runs quickly (on the order of milliseconds) whereas with the current regular
  expression, it freezes up for a long time. (I hit control C after maybe five or ten seconds. Not sure how long it
  would run to finish. Don't care, honestly.)

  var globParent = require('./index.js');
  
  globParent('{' + '/'.repeat(5000));

[Trott]

And, because regular expressions can be confusing, here's how the refactor went.

Original regexp: /[\{\[].*[\/]*.*[\}\]]$/

The part that was changed is this: [\/]*.*

So that's basically two things next to each other: [\/]* followed by .*.

That first one is "zero or more forward slashes and/or back slashes". That second one is "zero or more of anything".

So it amounts to "Zero or more forward slashes and/or back slashes, followed by zero or more of anything".

"Zero or more of anything" already includes any possible "zero or more forward slashes and/or back slashes" (which is why the regular expression engine ties itself in knots if there are a ton of slashes in a row) and so I replaced [\/]*.* with just .*. Should do the exact same thing, but with a lot less work on the part of the regular expression engine.

[Trott]

Seems like a test case can be added that will time out on current main but pass with this change. So I've added that.

[phated]

@Trott Awesome! And thank you for the thorough explanation 😄 I believe your solution fixes that, but I think our tests are lacking around this regexp because it is only supposed to match if an enclosure { } or [ ] contains a slash (as mentioned in #32)

I think that needs to be fixed and more tests added to make sure we are correctly handling that case.

[Trott]

@phated Would it be possible to separate the two issues?:

• Fix the ReDoS
• Make the regular expression and tests more robust/correct

If this lands, it addresses the first bullet point and there is a test so that the ReDoS issue doesn't return as y'all work through the second bullet point.

[phated]

No, because that further obscures the meaning of this code branch. As you've likely noticed in the other issue thread, no one knows what is going on.

[Trott]

OK, i've updated this based on the conversation in #32.

New commit message:

fix: eliminate ReDoS

This change fixes the regular expression denial of service
vulnerability.

This also fixes some incorrect tests that concealed a bug.

Fixes: https://github.com/gulpjs/glob-parent/issues/32
Refs: https://app.snyk.io/vuln/SNYK-JS-GLOBPARENT-1016905

[Trott]

So, instead of removing the detection for / in the reg exp, this now changes the "zero or more slashes" to be "one slash surrounded by other stuff".

So, before, the relevant segment was .*[\/]*.*. Now, it's .*\/.*. This has two effects. First, it gets rid of the ReDoS. Second, it fixes this segment of the regular expression so it detects a / rather than matching regardless of whether or not there is a slash.

This made four tests fail, but based on the conversation in the issue, those four tests were wrong. They now instead return the results per expectations. See #32 (comment) and subsequent comments.

[Trott]

I know this is open source and you don't owe it to me or anyone else, but is there anything more I should do to get this reviewed? Based on the conversation in the related issue, I believe this addresses all the outstanding concerns.

[phated]

@Trott Sorry for the delay, I don't mean to make you wait. I've been thinking about your comment about making a breaking release that changes this due to the test suite changing. Even though/if I'm very confident this was a bug with our test suite, it still probably needs to be released in a major.

I'm considering landing your original patch in the current major (since it makes the suite pass) and then doing a major that updates scaffold, fixes this incorrect behavior, and drops older node support.

[andyedwardsibm]

@phated is there any plan to back-port the core fix to previous versions of glob-parent? I couldn't find a definitive statement on whether previous major versions are supported for some time, or whether only the latest major version is supported

[Trott]

I'm considering landing your original patch in the current major (since it makes the suite pass) and then doing a major that updates scaffold, fixes this incorrect behavior, and drops older node support.

@phated I've opened #36 which is the original version of this that fixes the ReDoS but does not change any behavior. If you land that and publish it in a patch release, I'll rebase this against that change and then this can be landed as a breaking change.

[phated]

@Trott I updated the testing patterns with the new scaffold, so there are some conflicts.

[Trott]

@Trott I updated the testing patterns with the new scaffold, so there are some conflicts.

All conflicts resolved.

I guess the commit message should be updated, since this doesn't fix any ReDoS anymore. That happened in #36. This now updates the behavior to be correct, but may also be a breaking change if anyone is relying on the old behavior. I'll try to come up with a reasonable commit message and force push.

[Trott]

I changed the commit message to "fix: fix mishandled escaped path separators" but didn't include any details because it's all in #32 which is referred to in the commit metadata. Hopefully that's good enough, but if not, hey, I'm please edit (you should be able to force push to my branch) or tell me what it should say and I'll edit it.

I also didn't include "breaking change" anywhere in the commit message. Not sure if you plan to treat this as a bug fix or a breaking change. I can add that too, of course, or you can go for it.

[Trott]

I also didn't include "breaking change" anywhere in the commit message. Not sure if you plan to treat this as a bug fix or a breaking change. I can add that too, of course, or you can go for it.

I see in your comment over in #36 that you do intend to release this as a major, so I've changed fix: to fix!: per conventional commits, although you don't use conventional commits on this project. I'm not sure why I feel the need to do this and leave a comment, but here we are. Oh well. Change it any way you want of course. Thanks.

[phated]

Thanks @Trott! I'm trying to finish up #39 which would also be a breaking change so it'll be good to batch them together.

[Trott]

Thanks @Trott! I'm trying to finish up #39 which would also be a breaking change so it'll be good to batch them together.

It looks like #39 may have stalled. If there's anything I can do to nudge this PR forward, let me know!

[phated]

@Trott Thanks for the ping, I closed out the other issue since everyone was talking past each other. I'll merge this now.

[phated]

Published as 6.0.0 - thanks for all your hard work and patience @Trott!!