Skip to content

Commit 206ebb3

Browse files
balex13balex13
committed
MAGETWO-55605: Security Issue with referrer
1 parent 9130f67 commit 206ebb3

File tree

7 files changed

+199
-28
lines changed

7 files changed

+199
-28
lines changed

‎app/code/Magento/Customer/Model/Account/Redirect.php

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
use Magento\Customer\Model\Url as CustomerUrl;
1010
use Magento\Framework\App\RequestInterface;
1111
use Magento\Framework\Controller\ResultFactory;
12+
use Magento\Framework\Url\HostChecker;
1213
use Magento\Framework\UrlInterface;
1314
use Magento\Store\Model\ScopeInterface;
1415
use Magento\Store\Model\StoreManagerInterface;
@@ -53,6 +54,7 @@ class Redirect
5354
protected $customerUrl;
5455

5556
/**
57+
* @deprecated
5658
* @var UrlInterface
5759
*/
5860
protected $url;
@@ -67,6 +69,11 @@ class Redirect
6769
*/
6870
protected $cookieManager;
6971

72+
/**
73+
* @var HostChecker
74+
*/
75+
private $hostChecker;
76+
7077
/**
7178
* @param RequestInterface $request
7279
* @param Session $customerSession
@@ -76,6 +83,7 @@ class Redirect
7683
* @param DecoderInterface $urlDecoder
7784
* @param CustomerUrl $customerUrl
7885
* @param ResultFactory $resultFactory
86+
* @param HostChecker|null $hostChecker
7987
*/
8088
public function __construct(
8189
RequestInterface $request,
@@ -85,7 +93,8 @@ public function __construct(
8593
UrlInterface $url,
8694
DecoderInterface $urlDecoder,
8795
CustomerUrl $customerUrl,
88-
ResultFactory $resultFactory
96+
ResultFactory $resultFactory,
97+
HostChecker $hostChecker = null
8998
) {
9099
$this->request = $request;
91100
$this->session = $customerSession;
@@ -95,6 +104,7 @@ public function __construct(
95104
$this->urlDecoder = $urlDecoder;
96105
$this->customerUrl = $customerUrl;
97106
$this->resultFactory = $resultFactory;
107+
$this->hostChecker = $hostChecker ?: ObjectManager::getInstance()->get(HostChecker::class);
98108
}
99109

100110
/**
@@ -196,7 +206,7 @@ protected function processLoggedCustomer()
196206
$referer = $this->request->getParam(CustomerUrl::REFERER_QUERY_PARAM_NAME);
197207
if ($referer) {
198208
$referer = $this->urlDecoder->decode($referer);
199-
if ($this->url->isOwnOriginUrl()) {
209+
if ($this->hostChecker->isOwnOrigin($referer)) {
200210
$this->applyRedirect($referer);
201211
}
202212
}

‎app/code/Magento/Customer/Model/Url.php

Lines changed: 35 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -56,25 +56,43 @@ class Url
5656
*/
5757
protected $urlEncoder;
5858

59+
/**
60+
* @var \Magento\Framework\Url\DecoderInterface
61+
*/
62+
private $urlDecoder;
63+
64+
/**
65+
* @var \Magento\Framework\Url\HostChecker
66+
*/
67+
private $hostChecker;
68+
5969
/**
6070
* @param Session $customerSession
6171
* @param ScopeConfigInterface $scopeConfig
6272
* @param RequestInterface $request
6373
* @param UrlInterface $urlBuilder
6474
* @param EncoderInterface $urlEncoder
75+
* @param \Magento\Framework\Url\DecoderInterface|null $urlDecoder
76+
* @param \Magento\Framework\Url\HostChecker|null $hostChecker
6577
*/
6678
public function __construct(
6779
Session $customerSession,
6880
ScopeConfigInterface $scopeConfig,
6981
RequestInterface $request,
7082
UrlInterface $urlBuilder,
71-
EncoderInterface $urlEncoder
83+
EncoderInterface $urlEncoder,
84+
\Magento\Framework\Url\DecoderInterface $urlDecoder = null,
85+
\Magento\Framework\Url\HostChecker $hostChecker = null
7286
) {
7387
$this->request = $request;
7488
$this->urlBuilder = $urlBuilder;
7589
$this->scopeConfig = $scopeConfig;
7690
$this->customerSession = $customerSession;
7791
$this->urlEncoder = $urlEncoder;
92+
$this->urlDecoder = $urlDecoder ?: \Magento\Framework\App\ObjectManager::getInstance()
93+
->get(\Magento\Framework\Url\DecoderInterface::class);
94+
$this->hostChecker = $hostChecker ?: \Magento\Framework\App\ObjectManager::getInstance()
95+
->get(\Magento\Framework\Url\HostChecker::class);
7896
}
7997

8098
/**
@@ -95,7 +113,7 @@ public function getLoginUrl()
95113
public function getLoginUrlParams()
96114
{
97115
$params = [];
98-
$referer = $this->request->getParam(self::REFERER_QUERY_PARAM_NAME);
116+
$referer = $this->getRequestReferrer();
99117
if (!$referer
100118
&& !$this->scopeConfig->isSetFlag(
101119
self::XML_PATH_CUSTOMER_STARTUP_REDIRECT_TO_DASHBOARD,
@@ -122,9 +140,10 @@ public function getLoginUrlParams()
122140
public function getLoginPostUrl()
123141
{
124142
$params = [];
125-
if ($this->request->getParam(self::REFERER_QUERY_PARAM_NAME)) {
143+
$referer = $this->getRequestReferrer();
144+
if ($referer) {
126145
$params = [
127-
self::REFERER_QUERY_PARAM_NAME => $this->request->getParam(self::REFERER_QUERY_PARAM_NAME),
146+
self::REFERER_QUERY_PARAM_NAME => $referer,
128147
];
129148
}
130149
return $this->urlBuilder->getUrl('customer/account/loginPost', $params);
@@ -220,4 +239,16 @@ public function getEmailConfirmationUrl($email = null)
220239
{
221240
return $this->urlBuilder->getUrl('customer/account/confirmation', ['email' => $email]);
222241
}
242+
243+
/**
244+
* @return mixed|null
245+
*/
246+
private function getRequestReferrer()
247+
{
248+
$referer = $this->request->getParam(self::REFERER_QUERY_PARAM_NAME);
249+
if ($referer && $this->hostChecker->isOwnOrigin($this->urlDecoder->decode($referer))) {
250+
return $referer;
251+
}
252+
return null;
253+
}
223254
}

‎app/code/Magento/Customer/Test/Unit/Model/Account/RedirectTest.php

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
use Magento\Customer\Model\Account\Redirect;
1414
use Magento\Customer\Model\Url as CustomerUrl;
1515
use Magento\Framework\Controller\ResultFactory;
16+
use Magento\Framework\Url\HostChecker;
1617
use Magento\Store\Model\ScopeInterface;
1718
use Magento\Framework\TestFramework\Unit\Helper\ObjectManager;
1819

@@ -81,6 +82,11 @@ class RedirectTest extends \PHPUnit_Framework_TestCase
8182
*/
8283
protected $resultFactory;
8384

85+
/**
86+
* @var HostChecker | \PHPUnit_Framework_MockObject_MockObject
87+
*/
88+
private $hostChecker;
89+
8490
protected function setUp()
8591
{
8692
$this->request = $this->getMockForAbstractClass(\Magento\Framework\App\RequestInterface::class);
@@ -134,6 +140,10 @@ protected function setUp()
134140
->disableOriginalConstructor()
135141
->getMock();
136142

143+
$this->hostChecker = $this->getMockBuilder(HostChecker::class)
144+
->disableOriginalConstructor()
145+
->getMock();
146+
137147
$objectManager = new ObjectManager($this);
138148
$this->model = $objectManager->getObject(
139149
\Magento\Customer\Model\Account\Redirect::class,
@@ -145,7 +155,8 @@ protected function setUp()
145155
'url' => $this->url,
146156
'urlDecoder' => $this->urlDecoder,
147157
'customerUrl' => $this->customerUrl,
148-
'resultFactory' => $this->resultFactory
158+
'resultFactory' => $this->resultFactory,
159+
'hostChecker' => $this->hostChecker
149160
]
150161
);
151162
}
@@ -254,6 +265,7 @@ public function testGetRedirect(
254265

255266
$this->resultRedirect->expects($this->once())
256267
->method('setUrl')
268+
->with($beforeAuthUrl)
257269
->willReturnSelf();
258270

259271
$this->resultFactory->expects($this->once())
@@ -286,6 +298,7 @@ public function getRedirectDataProvider()
286298
return [
287299
// Loggend In, Redirect by Referer
288300
[1, 2, 'referer', 'base', '', '', 'account', '', '', '', true, false],
301+
[1, 2, 'http://referer.com/', 'http://base.com/', '', '', 'account', '', '', 'dashboard', true, false],
289302
// Loggend In, Redirect by AfterAuthUrl
290303
[1, 2, 'referer', 'base', '', 'defined', 'account', '', '', '', true, true],
291304
// Not logged In, Redirect by LoginUrl

‎lib/internal/Magento/Framework/Test/Unit/UrlTest.php

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
// @codingStandardsIgnoreFile
88

99
namespace Magento\Framework\Test\Unit;
10+
use Magento\Framework\Url\HostChecker;
1011

1112
/**
1213
* Test class for Magento\Framework\Url
@@ -59,6 +60,11 @@ class UrlTest extends \PHPUnit_Framework_TestCase
5960
*/
6061
protected $urlModifier;
6162

63+
/**
64+
* @var HostChecker|\PHPUnit_Framework_MockObject_MockObject
65+
*/
66+
private $hostChecker;
67+
6268
protected function setUp()
6369
{
6470
$this->routeParamsResolverMock = $this->getMock(
@@ -549,18 +555,17 @@ public function testAddSessionParam()
549555

550556
/**
551557
* @param bool $result
552-
* @param string $baseUrl
553558
* @param string $referrer
554559
* @dataProvider isOwnOriginUrlDataProvider
555560
*/
556-
public function testIsOwnOriginUrl($result, $baseUrl, $referrer)
561+
public function testIsOwnOriginUrl($result, $referrer)
557562
{
558563
$requestMock = $this->getRequestMock();
559-
$model = $this->getUrlModel(['scopeResolver' => $this->scopeResolverMock, 'request' => $requestMock]);
564+
$this->hostChecker = $this->getMockBuilder(HostChecker::class)
565+
->disableOriginalConstructor()->getMock();
566+
$this->hostChecker->expects($this->once())->method('isOwnOrigin')->with($referrer)->willReturn($result);
567+
$model = $this->getUrlModel(['hostChecker' => $this->hostChecker, 'request' => $requestMock]);
560568

561-
$this->scopeMock->expects($this->any())->method('getBaseUrl')->will($this->returnValue($baseUrl));
562-
$this->scopeResolverMock->expects($this->any())->method('getScopes')
563-
->will($this->returnValue([$this->scopeMock]));
564569
$requestMock->expects($this->once())->method('getServer')->with('HTTP_REFERER')
565570
->will($this->returnValue($referrer));
566571

@@ -570,8 +575,8 @@ public function testIsOwnOriginUrl($result, $baseUrl, $referrer)
570575
public function isOwnOriginUrlDataProvider()
571576
{
572577
return [
573-
'is origin url' => [true, 'http://localhost/', 'http://localhost/'],
574-
'is not origin url' => [false, 'http://localhost/', 'http://example.com/'],
578+
'is origin url' => [true, 'http://localhost/'],
579+
'is not origin url' => [false, 'http://example.com/'],
575580
];
576581
}
577582

‎lib/internal/Magento/Framework/Url.php

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@
88

99
namespace Magento\Framework;
1010

11+
use Magento\Framework\Url\HostChecker;
12+
1113
/**
1214
* URL
1315
*
@@ -178,6 +180,11 @@ class Url extends \Magento\Framework\DataObject implements \Magento\Framework\Ur
178180
*/
179181
private $escaper;
180182

183+
/**
184+
* @var HostChecker
185+
*/
186+
private $hostChecker;
187+
181188
/**
182189
* @param \Magento\Framework\App\Route\ConfigInterface $routeConfig
183190
* @param \Magento\Framework\App\RequestInterface $request
@@ -191,6 +198,7 @@ class Url extends \Magento\Framework\DataObject implements \Magento\Framework\Ur
191198
* @param \Magento\Framework\Url\RouteParamsPreprocessorInterface $routeParamsPreprocessor
192199
* @param string $scopeType
193200
* @param array $data
201+
* @param HostChecker|null $hostChecker
194202
* @SuppressWarnings(PHPMD.ExcessiveParameterList)
195203
*/
196204
public function __construct(
@@ -205,7 +213,8 @@ public function __construct(
205213
\Magento\Framework\App\Config\ScopeConfigInterface $scopeConfig,
206214
\Magento\Framework\Url\RouteParamsPreprocessorInterface $routeParamsPreprocessor,
207215
$scopeType,
208-
array $data = []
216+
array $data = [],
217+
HostChecker $hostChecker = null
209218
) {
210219
$this->_request = $request;
211220
$this->_routeConfig = $routeConfig;
@@ -218,6 +227,8 @@ public function __construct(
218227
$this->_scopeConfig = $scopeConfig;
219228
$this->routeParamsPreprocessor = $routeParamsPreprocessor;
220229
$this->_scopeType = $scopeType;
230+
$this->hostChecker = $hostChecker ?: \Magento\Framework\App\ObjectManager::getInstance()
231+
->get(HostChecker::class);
221232
parent::__construct($data);
222233
}
223234

@@ -1086,17 +1097,7 @@ public function useSessionIdForUrl($secure = false)
10861097
*/
10871098
public function isOwnOriginUrl()
10881099
{
1089-
$scopeDomains = [];
1090-
$referer = parse_url($this->_request->getServer('HTTP_REFERER'), PHP_URL_HOST);
1091-
foreach ($this->_scopeResolver->getScopes() as $scope) {
1092-
$scopeDomains[] = parse_url($scope->getBaseUrl(), PHP_URL_HOST);
1093-
$scopeDomains[] = parse_url($scope->getBaseUrl(UrlInterface::URL_TYPE_LINK, true), PHP_URL_HOST);
1094-
}
1095-
$scopeDomains = array_unique($scopeDomains);
1096-
if (empty($referer) || in_array($referer, $scopeDomains)) {
1097-
return true;
1098-
}
1099-
return false;
1100+
return $this->hostChecker->isOwnOrigin($this->_request->getServer('HTTP_REFERER'));
11001101
}
11011102

11021103
/**
@@ -1163,7 +1164,7 @@ protected function getRouteParamsResolver()
11631164
private function getUrlModifier()
11641165
{
11651166
if ($this->urlModifier === null) {
1166-
$this->urlModifier = \Magento\Framework\App\ObjectManager::getInstance()->get(
1167+
$this->urlModifier = \Magento\Framework\App\ObjectManager::getInstance()->get(
11671168
\Magento\Framework\Url\ModifierInterface::class
11681169
);
11691170
}

‎lib/internal/Magento/Framework/Url/HostChecker.php

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
<?php
2+
/**
3+
* Copyright © 2016 Magento. All rights reserved.
4+
* See COPYING.txt for license details.
5+
*/
6+
namespace Magento\Framework\Url;
7+
8+
use Magento\Framework\UrlInterface;
9+
10+
/**
11+
* Class provides functionality for checks of a host name
12+
*/
13+
class HostChecker
14+
{
15+
/**
16+
* @var \Magento\Framework\Url\ScopeResolverInterface
17+
*/
18+
private $scopeResolver;
19+
20+
/**
21+
* @param ScopeResolverInterface $scopeResolver
22+
*/
23+
public function __construct(ScopeResolverInterface $scopeResolver)
24+
{
25+
$this->scopeResolver = $scopeResolver;
26+
}
27+
28+
/**
29+
* Check if provided URL is one of the domain URLs assigned to scopes
30+
*
31+
* @param string $url
32+
* @return bool
33+
*/
34+
public function isOwnOrigin($url)
35+
{
36+
$scopeHostNames = [];
37+
$hostName = parse_url($url, PHP_URL_HOST);
38+
if (empty($hostName)) {
39+
return true;
40+
}
41+
foreach ($this->scopeResolver->getScopes() as $scope) {
42+
$scopeHostNames[] = parse_url($scope->getBaseUrl(), PHP_URL_HOST);
43+
$scopeHostNames[] = parse_url($scope->getBaseUrl(UrlInterface::URL_TYPE_LINK, true), PHP_URL_HOST);
44+
}
45+
$scopeHostNames = array_unique($scopeHostNames);
46+
return in_array($hostName, $scopeHostNames);
47+
}
48+
}

0 commit comments

Comments
 (0)