OpenId return URL comparison advanced to prevent url encode problem

extensions/authclient/CHANGELOG.md

@@ -4,6 +4,7 @@ Yii Framework 2 authclient extension Change Log
 2.0.0-rc under development
 --------------------------
 
+- Bug #3633: OpenId return URL comparison advanced to prevent url encode problem (klimov-paul)
 - Enh #3416: VKontakte OAuth support added (klimov-paul)
 
 

extensions/authclient/OpenId.php

@@ -809,7 +809,7 @@ class OpenId extends BaseClient implements ClientInterface
             $this->returnUrl .= (strpos($this->returnUrl, '?') ? '&' : '?') . 'openid.claimed_id=' . $claimedId;
         }
 
-        if ($this->data['openid_return_to'] != $this->returnUrl) {
+        if (!$this->compareUrl($this->data['openid_return_to'], $this->returnUrl)) {
             // The return_to url must match the url of current request.
             return false;
         }
@@ -949,4 +949,29 @@ class OpenId extends BaseClient implements ClientInterface
     {
         return array_merge(['id' => $this->getClaimedId()], $this->fetchAttributes());
     }
+
+    /**
+     * Compares 2 URLs taking in account possible GET parameters order miss match and URL encoding inconsistencies.
+     * @param string $expectedUrl expected URL.
+     * @param string $actualUrl actual URL.
+     * @return boolean whether URLs are equal.
+     */
+    protected function compareUrl($expectedUrl, $actualUrl)
+    {
+        $expectedUrlInfo = parse_url($expectedUrl);
+        $actualUrlInfo = parse_url($actualUrl);
+        foreach ($expectedUrlInfo as $name => $expectedValue) {
+            if ($name == 'query') {
+                parse_str($expectedValue, $expectedUrlParams);
+                parse_str($actualUrlInfo[$name], $actualUrlParams);
+                $paramsDiff = array_diff_assoc($expectedUrlParams, $actualUrlParams);
+                if (!empty($paramsDiff)) {
+                    return false;
+                }
+            } elseif ($expectedValue != $actualUrlInfo[$name]) {
+                return false;
+            }
+        }
+        return true;
+    }
 }

tests/unit/extensions/authclient/OpenIdTest.php

@@ -19,6 +19,23 @@ class OpenIdTest extends TestCase
         $this->mockApplication($config, '\yii\web\Application');
     }
 
+    /**
+     * Invokes the object method even if it is protected.
+     * @param object $object object instance
+     * @param string $methodName name of the method to be invoked.
+     * @param array $args method arguments.
+     * @return mixed method invoke result.
+     */
+    protected function invokeMethod($object, $methodName, array $args = [])
+    {
+        $classReflection = new \ReflectionClass(get_class($object));
+        $methodReflection = $classReflection->getMethod($methodName);
+        $methodReflection->setAccessible(true);
+        $result = $methodReflection->invokeArgs($object, $args);
+        $methodReflection->setAccessible(false);
+        return $result;
+    }
+
     // Tests :
 
     public function testSetGet()
@@ -58,4 +75,55 @@ class OpenIdTest extends TestCase
         $this->assertArrayHasKey('ax', $info);
         $this->assertArrayHasKey('sreg', $info);
     }
+
+    /**
+     * Data provider for [[testCompareUrl()]]
+     * @return array test data
+     */
+    public function dataProviderCompareUrl()
+    {
+        return [
+            [
+                'http://domain.com/index.php?r=site%2Fauth&authclient=myclient',
+                'http://domain.com/index.php?r=site%2Fauth&authclient=myclient',
+                true
+            ],
+            [
+                'http://domain.com/index.php?r=site%2Fauth&authclient=myclient',
+                'http://domain.com/index.php?r=site/auth&authclient=myclient',
+                true
+            ],
+            [
+                'http://domain.com/index.php?r=site%2Fauth&authclient=myclient',
+                'http://domain.com/index.php?r=site/auth&authclient=myclient2',
+                false
+            ],
+            [
+                'http://domain.com/index.php?r=site%2Fauth&authclient=myclient&custom=value',
+                'http://domain.com/index.php?r=site%2Fauth&custom=value&authclient=myclient',
+                true
+            ],
+            [
+                'https://domain.com/index.php?r=site%2Fauth&authclient=myclient',
+                'http://domain.com/index.php?r=site%2Fauth&authclient=myclient',
+                false
+            ],
+        ];
+    }
+
+    /**
+     * @see https://github.com/yiisoft/yii2/issues/3633
+     *
+     * @dataProvider dataProviderCompareUrl
+     *
+     * @param string $url1
+     * @param string $url2
+     * @param boolean $expectedResult
+     */
+    public function testCompareUrl($url1, $url2, $expectedResult)
+    {
+        $client = new OpenId();
+        $comparisonResult = $this->invokeMethod($client, 'compareUrl', [$url1, $url2]);
+        $this->assertEquals($expectedResult, $comparisonResult);
+    }
 }