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.

[mtangoo]

@edegaudenzi I assume you have resolved the issue and that you no longer have any issue with the implementation.
Feel free to reopen if it is not so.

[edegaudenzi]

@mtangoo thanks for your message. Yes I did solve the problem, I've also created and submitted a pull request; but I do still have issues with the regression introduced between 2.0.49 and 2.0.50, still there.

I will also create a feature request to allow "createAbsoluteUrl()" to be able to Create Absolute Urls, not just the yii2 ones but any absolute url, as it was per 2.0.49 version.

Personally, regression introduced in 2.0.50 meant I needed to patch every project I worked on in the last few years because they suddenly stopped to work, hence time, hence money. Sadly, this also means that soon I'll probably need to look to replace it with another framework.

For those having the same problem described by this issue, I'll submit here a little patch you can apply to keep receiving yii2 updates but having absolute URLs still working.

[edegaudenzi]

Unfortunately this issue is still NOT RESOLVED, despite its 'Closed' status, but I'm writing this for the records and to help someone else in the same situation I was. I'll also update this issue as soon as the yii2 maintainers will do their part to officially fix this.

Here's how to patch your yii2 < 2.0.50 if you were affected by the yii2 >= 2.0.50 regression described in this issue, it's fairly simple. The whole concept is to say yii2 to use your own Url helper instead of its own; your Url helper will extend the core one and override the incriminated method. More details on customizing-helper-classes

1. Create a new class extending from the helpers corresponding base class: yii\helpers\Url
2. Fill the new class with the following code; this is the original and working implementation of Url::isRelative():

<?php
namespace yii\helpers;

/**
 * Url provides a set of static methods for managing URLs.
 *
 * For more details and usage information on Url, see the [guide article on url helpers](guide:helper-url).
 *
 * @author Alexander Makarov <[email protected]>
 * @since 2.0
 */
class Url extends BaseUrl
{
    /**
     * Returns a value indicating whether a URL is relative.
     * A relative URL does not have host info part.
     * @param string $url the URL to be checked
     * @return bool whether the URL is relative
     */
    public static function isRelative($url)
    {
        // return parent::isRelative($url);
        return strncmp($url, '//', 2) && strpos($url, '://') === false;
    }
}

3. Now, you need to tell Yii2 to use this one in place of the core one: in your application's entry script - e.g. common\config\bootstrap.php - add the following line of code after including the yii.php file: Yii::$classMap['yii\helpers\Url'] = '@common/helpers/Url.php';, accordingly to your basic/advanced yii2 project initialisation.
   It should look something like:

<?php
Yii::setAlias('@common', dirname(__DIR__));
Yii::setAlias('@frontend', dirname(dirname(__DIR__)) . '/frontend');
Yii::setAlias('@backend', dirname(dirname(__DIR__)) . '/backend');
Yii::setAlias('@console', dirname(dirname(__DIR__)) . '/console');

//TODO: remove the following line and related extended class as soon as https://github.com/yiisoft/yii2/issues/20199 is resolved
Yii::$classMap['yii\helpers\Url'] = '@common/helpers/Url.php';

job done, your Yii2 apps can build absolute urls again!

Hope to have been helpful to someone else.

Live long and prosper

🖖

[samdark]

@edegaudenzi have you tested on 2.0.51?

[edegaudenzi]

@samdark Yes, I've just run a couple of tries:

• If you are referring to relicate the createAbsoluteUrl() misbehavior described in this thread: yes, unfortunately I can on yii2 version 2.0.51 as well. Anyway, not a surprise because my PR was not merged and Url::isRelative() remained untouched.

• If you are asking if the patch described here also works on yii2 version 2.0.51, then answer is still yes, but only because the patch basically forces the old behavior through Helpers classes customisation.

[rob006]

@samdark Are you sure that UrlManager should be responsible for processing external URLs? Some kind of wrapper around http_build_query() could be useful, but IMO it makes more sense as part of Url helper.

[samdark]

@rob006 the issue is that it was processing it like that before and @edegaudenzi relied on that.

[rob006]

@samdark He used this method in a wrong way, I explained it in #20199 (comment) and #20199 (comment). This issue really feels like https://xkcd.com/1172/

[samdark]

Re-read comments. You're right, @rob006, it was never supposed to accept non-routes.