edegaudenzi
commented
5 months ago Regression was introduced by commit https://github.com/yiisoft/yii2/commit/b46e2676d347adba7ca43f59008cde9dd17f09f5, claiming to have Improved BaseUrl::isRelative($url) performance.
The Problem
In the commit, regex ~^[[:alpha:]][[:alnum:]+-.]*://|^//~ replaced a couple of strncmp && strpos conditions but they do not implement the same match. In details:
The original version 2.0.49 return strncmp($url, '//', 2) && strpos($url, '://') === false; in English means "Return True if the URL does not start with a double fwdslash AND colon-fwdslash-fwdslash is not found anywhere in the URL"
The solution introduced in v 2.0.50 return preg_match('~^[[:alpha:]][[:alnum:]+-.]*://|^//~', $url) === 0; in English means "Return True if the in URL does not start with an A-z char, followed by an optional A-z0-9 string, preceding a colon-fwdslash-fwdslash OR Return True if the URL does not start with double fwdslash"
As you can see and test, these two solutions outcome will be different, hence the discrepancy between 2.0.49 and 2.0.50.
I've also performed a few speed tests with a 1core 3GB VM on a very old host machine: 9999999 BaseUrl::isRelative() checks on random urls, chosen among 5 different url examples. Every test is run 5 times and averaged. Results in seconds:
strncmp($url, '//', 2) !== 0 && strpos($url, '://') === false
3.2607851028442383
strncmp($url, '//', 2) && strpos($url, '://') === false
3.2602100372314453
/*PHP8 only*/strncmp($url, '//', 2) && !str_contains($url, '://');
3.243748903274536
preg_match('~^[/][^/]((?!://).)*$~', $url)
3.070176839828491
/*PHP8 only*/!str_starts_with($url, '//') && !str_contains($url, '://');
2.0685629844665527The Solution
From the tests performed above, if you were to perform the correct regex match you can see that in 10 millions checks, on a crappy hardware (in fact, not even hardware, a virtualisation of it!) you save ... drum roll... about a fifth of a second if you use PHP7 (officially deprecated). If you use PHP8 it actually takes more time than using native PHP functions. But in exchange, using regexes, we can now have a more vulnerable solution, prone to all sort of regex-driven attacks!
If you ask me, I would rather avoid to use regexes where the input can be potentially user-made like in this case. Also, regexes performances are not a given, they can result in unpredictable performances depending what you feed them; plus, PCRE world is definitely not new to various injections and bo attacks.
My propose is just to revert this line of the commit and bring it back on how it was, that's it. This until even yii2 will bump required php version to 8+, then I would use something similar to the fastest line in the tests above.
For the ones curious, a regex example implementing exactly what the original function does can be found here: https://regex101.com/r/8Fq4T7/4
Hope to have been useful, I'll push a PR with the commit reverted.
Live long and prosper,
\\//_
rob006
mtangoo
samdark
What steps will reproduce the problem?
In Yii 2.0.49 following code generates a correct Authorization URL (in this case for Hubspot OAuth2 authentication process, but it doesn't really matter). On the other hand, in Yii 2.0.50 following code generates a wrong Authorization URL, prepending the host relative path.
It seems to be a regression introduced with 2.0.50. I'll need to investigate deeper but wanted to give an immediate heads up for everyone there that might be affected.
What is the expected result?
What do you get instead?
Additional info