fix: Regular Expression Denial of Service (ReDoS)

[WillianAgostini]

Fixes #6131
An optimization of regex was implemented.

[firattale]

LGTM

[DigitalBrainJS]

Seems this code is still vulnerable

    instance.defaults.baseURL = '///foo' + '/'.repeat(100000) + 'bar/'; 
   // Expected 8803.69999999972 to be less than 20.

Anyway, I believe that replacing /\/+$/ with /\/?\/$/ is enough to solve the issue with ReDos handling only practical cases of user input. The fix now does normalization at more than just the end/beginning and seems overkill as I don't see any cases where we might need to normalize more than two slashes in a valid URL. Moreover, using multiple regular expressions does not benefit performance and code size. Thoughts?

[WillianAgostini]

Seems this code is still vulnerable

    instance.defaults.baseURL = '///foo' + '/'.repeat(100000) + 'bar/'; 
   // Expected 8803.69999999972 to be less than 20.

Anyway, I believe that replacing /\/+$/ with /\/?\/$/ is enough to solve the issue with ReDos handling only practical cases of user input. The fix now does normalization at more than just the end/beginning and seems overkill as I don't see any cases where we might need to normalize more than two slashes in a valid URL. Moreover, using multiple regular expressions does not benefit performance and code size. Thoughts?

Fixes 918286c

[ffk-tecky]

any idea when would this be released?

[DigitalBrainJS]

Is there any reason we can't simplify this solution to /\/?\/$/ or /\/{1,n}$/? The second regexp /^\/+/ is not vulnerable to ReDoS as it has linear complexity, so there is no reason to filter its input.

[WillianAgostini]

Is there any reason we can't simplify this solution to /\/?\/$/ or /\/{1,n}$/? The second regexp /^\/+/ is not vulnerable to ReDoS as it has linear complexity, so there is no reason to filter its input.

However, please note that using /\/?\/$/ or /\/{1,n}$/ does not handle cases where multiple slashes need to be replaced, as demonstrated by the example: '///foo' + '/'.repeat(100000) + 'bar////////'.
In the second case, if you are certain about it, I can remove the filter.

[DigitalBrainJS]

does not handle cases where multiple slashes need to be replaced

I don't see any reason why we need to normalize multiple trailing slashes since it's an invalid url. We just need to join the base url and relative path and there is no point in complicating the code for an impractical case. So for now I'm I tend to the simplest implementation.

In the second case, if you are certain about it, I can remove the filter.

Well, this is quite obvious, since the graph of the regexp has no branches, only a direct loop. This can be confirmed with a test service like https://devina.io/redos-checker or your test case.

[WillianAgostini]

does not handle cases where multiple slashes need to be replaced

I don't see any reason why we need to normalize multiple trailing slashes since it's an invalid url. We just need to join the base url and relative path and there is no point in complicating the code for an impractical case. So for now I'm I tend to the simplest implementation.

In the second case, if you are certain about it, I can remove the filter.

Well, this is quite obvious, since the graph of the regexp has no branches, only a direct loop. This can be confirmed with a test service like https://devina.io/redos-checker or your test case.

I applied the changes ae9f303

[github-actions]

Hi, @WillianAgostini! This PR has been published in v1.6.3 release. Thank you for your contribution ❤️!