8000 bug #23580 Fix login redirect when referer contains a query string (f… · symfony/symfony@f4172b0 · GitHub
[go: up one dir, main page]
Skip to content

Commit f4172b0

Browse files
fabpotfabpot
committed
bug #23580 Fix login redirect when referer contains a query string (fabpot)
This PR was squashed before being merged into the 2.7 branch (closes #23580). Discussion ---------- Fix login redirect when referer contains a query string | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #19026, #23027, #23061, #23411, #23551 | License | MIT | Doc PR | n/a In 3.3, #19026 was merged to fix a bug that should have been fixed in 2.7. The fix was wrong anyway, so this PR fixes it the proper way. The first two commits refactors test (using mocks for data objects is a bad idea and using too many mocks actually makes tests test nothing). The actual fix is in the third commit. Commits ------- 022ac0b [Security] added more tests 9c7a140 [Security] fixed default target path when referer contains a query string b1f1ae2 [Security] simplified tests 3387612 [Security] refactored tests
2 parents 2040770 + 022ac0b commit f4172b0

File tree

2 files< 8000 !-- --> changed

+86
-147
lines changed

2 files changed

+86
-147
lines changed

‎src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationSuccessHandler.php

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -118,8 +118,14 @@ protected function determineTargetUrl(Request $request)
118118
return $targetUrl;
119119
}
120120

121-
if ($this->options['use_referer'] && ($targetUrl = $request->headers->get('Referer')) && $targetUrl !== $this->httpUtils->generateUri($request, $this->options['login_path'])) {
122-
return $targetUrl;
121+
if ($this->options['use_referer']) {
122+
$targetUrl = $request->headers->get('Referer');
123+
if (false !== $pos = strpos($targetUrl, '?')) {
124+
$targetUrl = substr($targetUrl, 0, $pos);
125+
}
126+
if ($targetUrl !== $this->httpUtils->generateUri($request, $this->options['login_path'])) {
127+
return $targetUrl;
128+
}
123129
}
124130

125131
return $this->options['default_target_path'];

‎src/Symfony/Component/Security/Http/Tests/Authentication/DefaultAuthenticationSuccessHandlerTest.php

Lines changed: 78 additions & 145 deletions
Original file line numberDiff line numberDiff line change
@@ -12,159 +12,92 @@
1212
namespace Symfony\Component\Security\Http\Tests\Authentication;
1313

1414
use PHPUnit\Framework\TestCase;
15-
use Symfony\Component\HttpFoundation\Response;
15+
use Symfony\Component\HttpFoundation\Request;
1616
use Symfony\Component\Security\Http\Authentication\DefaultAuthenticationSuccessHandler;
17+
use Symfony\Component\Security\Http\HttpUtils;
1718

1819
class DefaultAuthenticationSuccessHandlerTest extends TestCase
1920
{
20-
private $httpUtils = null;
21-
22-
private $request = null;
23-
24-
private $token = null;
25-
26-
protected function setUp()
27-
{
28-
$this->httpUtils = $this->getMockBuilder('Symfony\Component\Security\Http\HttpUtils')->getMock();
29-
$this->request = $this->getMockBuilder('Symfony\Component\HttpFoundation\Request')->getMock();
30-
$this->request->headers = $this->getMockBuilder('Symfony\Component\HttpFoundation\HeaderBag')->getMock();
31-
$this->token = $this->getMockBuilder('Symfony\Component\Security\Core\Authentication\Token\TokenInterface')->getMock();
32-
}
33-
34-
public function testRequestIsRedirected()
35-
{
36-
$response = $this->expectRedirectResponse('/');
37-
38-
$handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, array());
39-
$result = $handler->onAuthenticationSuccess($this->request, $this->token);
40-
41-
$this->assertSame($response, $result);
42-
}
43-
44-
public function testDefaultTargetPathCanBeForced()
45-
{
46-
$options = array(
47-
'always_use_default_target_path' => true,
48-
'default_target_path' => '/dashboard',
49-
);
50-
51-
$response = $this->expectRedirectResponse('/dashboard');
52-
53-
$handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, $options);
54-
$result = $handler->onAuthenticationSuccess($this->request, $this->token);
55-
56-
$this->assertSame($response, $result);
57-
}
58-
59-
public function testTargetPathIsPassedWithRequest()
21+
/**
22+
* @dataProvider getRequestRedirections
23+
*/
24+
public function testRequestRedirections(Request $request, $options, $redirectedUrl)
6025
{
61-
$this->request->expects($this->once())
62-
->method('get')->with('_target_path')
63-
->will($this->returnValue('/dashboard'));
64-
65-
$response = $this->expectRedirectResponse('/dashboard');
66-
67-
$handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, array());
68-
$result = $handler->onAuthenticationSuccess($this->request, $this->token);
69-
70-
$this->assertSame($response, $result);
26+
$urlGenerator = $this->getMockBuilder('Symfony\Component\Routing\Generator\UrlGeneratorInterface')->getMock();
27+
$urlGenerator->expects($this->any())->method('generate')->will($this->returnValue('http://localhost/login'));
28+
$httpUtils = new HttpUtils($urlGenerator);
29+
$token = $this->getMockBuilder('Symfony\Component\Security\Core\Authentication\Token\TokenInterface')->getMock();
30+
$handler = new DefaultAuthenticationSuccessHandler($httpUtils, $options);
31+
if ($request->hasSession()) {
32+
$handler->setProviderKey('admin');
33+
}
34+
$this->assertSame('http://localhost'.$redirectedUrl, $handler->onAuthenticationSuccess($request, $token)->getTargetUrl());
7135
}
7236

73-
public function testTargetPathParameterIsCustomised()
74-
{
75-
$options = array('target_path_parameter' => '_my_target_path');
76-
77-
$this->request->expects($this->once())
78-
->method('get')->with('_my_target_path')
79-
->will($this->returnValue('/dashboard'));
80-
81-
$response = $this->expectRedirectResponse('/dashboard');
82-
83-
$handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, $options);
84-
$result = $handler->onAuthenticationSuccess($this->request, $this->token);
85-
86-
$this->assertSame($response, $result);
87-
}
88-
89-
public function testTargetPathIsTakenFromTheSession()
37+
public function getRequestRedirections()
9038
{
9139
$session = $this->getMockBuilder('Symfony\Component\HttpFoundation\Session\SessionInterface')->getMock();
92-
$session->expects($this->once())
93-
->method('get')->with('_security.admin.target_path')
94-
->will($this->returnValue('/admin/dashboard'));
95-
$session->expects($this->once())
96-
->method('remove')->with('_security.admin.target_path');
97-
98-
$this->request->expects($this->any())
99-
->method('getSession')
100-
->will($this->returnValue($session));
101-
102-
$response = $this->expectRedirectResponse('/admin/dashboard');
103-
104-
$handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, array());
105-
$handler->setProviderKey('admin');
106-
107-
$result = $handler->onAuthenticationSuccess($this->request, $this->token);
108-
109-
$this->assertSame($response, $result);
110-
}
111-
112-
public function testTargetPathIsPassedAsReferer()
113-
{
114-
$options = array('use_referer' => true);
115-
116-
$this->request->headers->expects($this->once())
117-
->method('get')->with('Referer')
118-
->will($this->returnValue('/dashboard'));
119-
120-
$response = $this->expectRedirectResponse('/dashboard');
121-
122-
$handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, $options);
123-
$result = $handler->onAuthenticationSuccess($this->request, $this->token);
124-
125-
$this->assertSame($response, $result);
126-
}
127-
128-
public function testRefererHasToBeDifferentThatLoginUrl()
129-
{
130-
$options = array('use_referer' => true);
131-
132-
$this->request->headers->expects($this->any())
133-
->method('get')->with('Referer')
134-
->will($this->returnValue('/login'));
135-
136-
$this->httpUtils->expects($this->once())
137-
->method('generateUri')->with($this->request, '/login')
138-
->will($this->returnValue('/login'));
139-
140-
$response = $this->expectRedirectResponse('/');
141-
142-
$handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, $options);
143-
$result = $handler->onAuthenticationSuccess($this->request, $this->token);
144-
145-
$this->assertSame($response, $result);
146-
}
147-
148-
public function testRefererTargetPathIsIgnoredByDefault()
149-
{
150-
$this->request->headers->expects($this->never())->method('get');
151-
152-
$response = $this->expectRedirectResponse('/');
153-
154-
$handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, array());
155-
$result = $handler->onAuthenticationSuccess($this->request, $this->token);
156-
157-
$this->assertSame($response, $result);
158-
}
159-
160-
private function expectRedirectResponse($path)
161-
{
162-
$response = new Response();
163-
$this->httpUtils->expects($this->once())
164-
->method('createRedirectResponse')
165-
->with($this->request, $path)
166-
->will($this->returnValue($response));
167-
168-
return $response;
40+
$session->expects($this->once())->method('get')->with('_security.admin.target_path')->will($this->returnValue('/admin/dashboard'));
41+
$session->expects($this->once())->method('remove')->with('_security.admin.target_path');
42+
$requestWithSession = Request::create('/');
43+
$requestWithSession->setSession($session);
44+
45+
return array(
46+
'default' => array(
47+
Request::create('/'),
48+
array(),
49+
'/',
50+
),
51+
'forced target path' => array(
52+
Request::create('/'),
53+
array('always_use_default_target_path' => true, 'default_target_path' => '/dashboard'),
54+
'/dashboard',
55+
),
56+
'target path as query string' => array(
57+
Request::create('/?_target_path=/dashboard'),
58+
array(),
59+
'/dashboard',
60+
),
61+
'target path name as query string is customized' => array(
62+
Request::create('/?_my_target_path=/dashboard'),
63+
array('target_path_parameter' => '_my_target_path'),
64+
'/dashboard',
65+
),
66+
'target path name as query string is customized and nested' => array(
67+
Request::create('/?_target_path[value]=/dashboard'),
68+
array('target_path_parameter' => '_target_path[value]'),
69+
'/dashboard',
70+
),
71+
'target path in session' => array(
72+
$requestWithSession,
73+
array(),
74+
'/admin/dashboard',
75+
),
76+
'target path as referer' => array(
77+
Request::create('/', 'GET', array(), array(), array(), array('HTTP_REFERER' => 'http://localhost/dashboard')),
78+
array('use_referer' => true),
79+
'/dashboard',
80+
),
81+
'target path as referer is ignored if not configured' => array(
82+
Request::create('/', 'GET', array(), array(), array(), array('HTTP_REFERER' => 'http://localhost/dashboard')),
83+
array(),
84+
'/',
85+
),
86+
'target path should be different than login URL' => array(
87+
Request::create('/', 'GET', array(), array(), array(), array('HTTP_REFERER' => 'http://localhost/login')),
88+
array('use_referer' => true, 'login_path' => '/login'),
89+
'/',
90+
),
91+
'target path should be different than login URL (query string does not matter)' => array(
92+
Request::create('/', 'GET', array(), array(), array(), array('HTTP_REFERER' => 'http://localhost/login?t=1&p=2')),
93+
array('use_referer' => true, 'login_path' => '/login'),
94+
'/',
95+
),
96+
'target path should be different than login URL (login_path as a route)' => array(
97+
Request::create('/', 'GET', array(), array(), array(), array('HTTP_REFERER' => 'http://localhost/login?t=1&p=2')),
98+
array('use_referer' => true, 'login_path' => 'login_route'),
99+
'/',
100+
),
101+
);
169102
}
170103
}

0 commit comments

Comments
 (0)
0