yii\web\UrlManager::createAbsoluteUrl() creates wrong url in version >= 2.0.50

[edegaudenzi]

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.

return Yii::$app->urlManager->createAbsoluteUrl([
        'https://app.hubspot.com/oauth/authorize',
	'client_id'    => $this->client_id,
	'redirect_uri' => $this->redirect_uri,
	'scope'        => $this->scopes,
	'state'        => $this->state,
], 'https');

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?

https://app.hubspot.com/oauth/authorize?client_id=12345&redirect_uri=https%3A%2F%2Fmydomain.com%2Fapp%2Fhubspot%2Fget-access-token&scope=actions&state=M0

What do you get instead?

/app/https://app.hubspot.com/oauth/authorize?client_id=12345&redirect_uri=https%3A%2F%2Fmydomain.com%2Fapp%2Fhubspot%2Fget-access-token&scope=actions&state=M0

Additional info

Q	A
Yii version	2.0.50
PHP version	8.1.2
Operating system	Ubuntu 22.04

[edegaudenzi]

Regression was introduced by commit b46e267, 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.0685629844665527

The 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]

return Yii::$app->urlManager->createAbsoluteUrl([
        'https://app.hubspot.com/oauth/authorize',
	'client_id'    => $this->client_id,
	'redirect_uri' => $this->redirect_uri,
	'scope'        => $this->scopes,
	'state'        => $this->state,
], 'https');

This looks like an invalid call - createAbsoluteUrl() accepts array with route as a first element. If you know URL, and only want to append some parameters, it is a job for http_build_query(). It does not look like a case when you should use URL manager.

I also tested this in Yii 2.0.47 and I get the same results, so I don't think this is regression in Url::isRelative().

[edegaudenzi]

Thank you for your contribution.

The one in the example is actually a valid call, in fact I can re-confirm it is the one that changed the behaviour from 2.0.49 and 2.0.50; also, I've been using it literally for years and it broke all of a sudden in the test environments. Additionally, using regexes to process possible end-user arbitrary strings still remains quite a basic sec issue.

To replicate the misbehaviour in subject, the only thing you might have different is the configuration of the urlManager in your config file. In enterprise-grade environments it is quite common to have baseUrl and scriptUrl assigned with real values. You don't have them by default when you create an empty yii2 project.

I've just created a yii2 project from the scratch to show this to anyone interested in trying this out:

As you may notice, Yii::$app->urlManager->createAbsoluteUrl not only works, but behaves as described between version 2.0.49 and 2.0.50. If you or any reader can confirm this behaviour as well it would be great and really appreciated.

I've spent the last six hours in debugging, writing, testing and profiling; the thing is, I can also go http_build_query() - and I'm already doing for some non yii related code! - but why should I if I decided to use this framework? This bug is holding back for me a number of releases. I'll create a PR for the hotfix, but if not accepted I need to get the yii2 version stuck at 2.0.49 or use my own implementation of UrlManager::createAbsoluteUrl(), carrying the one implemented in version 2.0.49.

[rob006]

https://www.yiiframework.com/doc/api/2.0/yii-web-urlmanager#createAbsoluteUrl()-detail - $params should be either a route or array containing route and params. If it worked before with full URL instead of route, it was probably by pure luck and this was not an intended behavior.

I can also go http_build_query() - and I'm already doing for some non yii related code! - but why should I if I decided to use this framework?

Because you're using a wrong tool - createAbsoluteUrl() is a method for generating absolute URL from route and params. But you don't have a route - you already have an absolute URL.

I've just created a yii2 project from the scratch to show this to anyone interested in trying this out:

Can you share this project?

[edegaudenzi]

Ok, I definitely don't want to step on anyone's toes here. I am just pointing out it might be a documentation miss.

If you read at the createAbsoluteUrl, createUrl implementations you will notice there are if strpos('://')s in places where, by manual, you would only expect relative urls, as if that code was clearly made to also deal with known absolute URLs to be put in the $param[0] element of the array; and it also brilliantly worked as well for years, until version 2.0.49.

In version 2.0.50 the only problem is that the isRelative() regex does not do what the php used to in the previous version 2.0.49. The change claimed to be just a performance improvement, but it also changed the behaviour of the function as abundantly documented here.

Also, I still think validate end-user strings with regex needs to be taken with a grain of salt and avoided.

Sure I can share the project, it's literally a:

composer create-project --prefer-dist yiisoft/yii2-app-basic basic

with this in basic/commands/HelloController.php:

    /**
     * This command echoes what you have entered as the message.
     * @param string $message the message to be echoed.
     * @return int Exit code
     */
    public function actionIndex($message = 'hello world')
    {
        echo Yii::$app->urlManager->createAbsoluteUrl([
	        'https://app.hubspot.com/oauth/authorize',
		'client_id'    => '123456',
		'redirect_uri' => 'localhost:8080',
		'scope'        => 'read write',
		'state'        => 'a;wkmk;lj',
	], 'https');

        return ExitCode::OK;
    }

and this added to basic/config/console.php component section, immediately after 'db' => $db:

        'urlManager' => [
                'class'           => 'yii\web\UrlManager',
                'baseUrl'         => '/app',
                'enablePrettyUrl' => true,
                'hostInfo'        => 'https://localhost',
                'scriptUrl'       => '/index.php',
                'showScriptName'  => false,
        ],

I'm not sure I also need to copy-paste commands to switch between versions but here you go:
composer require -q -W yiisoft/yii2 "2.0.49"
composer require -q -W yiisoft/yii2 "2.0.50"

[rob006]

If you read at the createAbsoluteUrl, createUrl implementations you will notice there are if strpos('://')s in places where, by manual, you would only expect relative urls,

Can you point to specific place? I see strpos('://') check only on URL generated by UrlManager (result of createUrl() - rules can generate absolute URL), never on value that should be a route.

I also tested your example and indeed it worked by accident. If you use createUrl() instead of createAbsoluteUrl() you can see that it generates /app/https://app.hubspot.com/oauth/authorize?client_id=123456&redirect_uri=localhost%3A8080&scope=read+write&state=a%3Bwkmk%3Blj on both 2.0.49 and 2.0.50. But prior to 2.0.50 due to a bug in Url::isRelative() it was treated as absolute URL with /app/https as protocol, which then was replaced by https and as a result you had URL you want.

[edegaudenzi]

Yes, thank you @rob006 , everything's much clearer now. My bad I drove you down this rabbit hole.

All of the issues described in my first comment still persist though and I would add that using regexes to solve performance problems or, even worse, for validating data that may come from end-users, it's just commonly considered a bad idea.

If you suggest a different title for this issue, I'm more than happy to change it for you, but this is pretty much it; yesterday I've created a PR to solve this problem in the interest of the community and mine. In case the PR is rejected, I'll post here a guide on how to solve this issue otherwise, still in a proper yii2 compliant way.

Hope to have been useful, I'll keep you posted.

Live long and prosper,

🖖

[rob006]

All of the issues described in my first comment still persist though and I would add that using regexes to solve performance problems or, even worse, for validating data that may come from end-users, it's just commonly considered a bad idea.

Do you have an example input for which this regex would be really slow? Not all regular expressions are slow and we already use it to parse user input (for example in some validators or for parsing URLs in UrlManager). This solution was also discussed at #20077 - you can find some benchmarks there.