Skip to content

Commit ebbf7f1

Browse files
derrabusderrabus
committed
Don't produce TypeErrors for non-string CSRF tokens
Signed-off-by: Alexander M. Turek <me@derrabus.de>
1 parent 3d967a0 commit ebbf7f1

File tree

5 files changed

+105
-19
lines changed

5 files changed

+105
-19
lines changed

‎Firewall/LogoutListener.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ public function authenticate(RequestEvent $event)
8787
if (null !== $this->csrfTokenManager) {
8888
$csrfToken = ParameterBagUtils::getRequestParameterValue($request, $this->options['csrf_parameter']);
8989

90-
if (false === $this->csrfTokenManager->isTokenValid(new CsrfToken($this->options['csrf_token_id'], $csrfToken))) {
90+
if (!\is_string($csrfToken) || false === $this->csrfTokenManager->isTokenValid(new CsrfToken($this->options['csrf_token_id'], $csrfToken))) {
9191
throw new LogoutException('Invalid CSRF token.');
9292
}
9393
}

‎Firewall/SimpleFormAuthenticationListener.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ protected function attemptAuthentication(Request $request)
8484
if (null !== $this->csrfTokenManager) {
8585
$csrfToken = ParameterBagUtils::getRequestParameterValue($request, $this->options['csrf_parameter']);
8686

87-
if (false === $this->csrfTokenManager->isTokenValid(new CsrfToken($this->options['csrf_token_id'], $csrfToken))) {
87+
if (!\is_string($csrfToken) || false === $this->csrfTokenManager->isTokenValid(new CsrfToken($this->options['csrf_token_id'], $csrfToken))) {
8888
throw new InvalidCsrfTokenException('Invalid CSRF token.');
8989
}
9090
}

‎Firewall/UsernamePasswordFormAuthenticationListener.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ protected function attemptAuthentication(Request $request)
7272
if (null !== $this->csrfTokenManager) {
7373
$csrfToken = ParameterBagUtils::getRequestParameterValue($request, $this->options['csrf_parameter']);
7474

75-
if (false === $this->csrfTokenManager->isTokenValid(new CsrfToken($this->options['csrf_token_id'], $csrfToken))) {
75+
if (!\is_string($csrfToken) || false === $this->csrfTokenManager->isTokenValid(new CsrfToken($this->options['csrf_token_id'], $csrfToken))) {
7676
throw new InvalidCsrfTokenException('Invalid CSRF token.');
7777
}
7878
}

‎Tests/Firewall/LogoutListenerTest.php‎

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -152,28 +152,42 @@ public function testSuccessHandlerReturnsNonResponse()
152152
$listener($event);
153153
}
154154

155-
public function testCsrfValidationFails()
155+
/**
156+
* @dataProvider provideInvalidCsrfTokens
157+
*/
158+
public function testCsrfValidationFails($invalidToken)
156159
{
157160
$this->expectException(LogoutException::class);
158161
$tokenManager = $this->getTokenManager();
159162

160163
[$listener, , $httpUtils, $options] = $this->getListener(null, $tokenManager);
161164

162165
$request = new Request();
163-
$request->query->set('_csrf_token', 'token');
166+
if (null !== $invalidToken) {
167+
$request->query->set('_csrf_token', $invalidToken);
168+
}
164169

165170
$httpUtils->expects($this->once())
166171
->method('checkRequestPath')
167172
->with($request, $options['logout_path'])
168173
->willReturn(true);
169174

170-
$tokenManager->expects($this->once())
175+
$tokenManager
171176
->method('isTokenValid')
172177
->willReturn(false);
173178

174179
$listener(new RequestEvent($this->createMock(HttpKernelInterface::class), $request, HttpKernelInterface::MASTER_REQUEST));
175180
}
176181

182+
public function provideInvalidCsrfTokens(): array
183+
{
184+
return [
185+
['invalid'],
186+
[['in' => 'valid']],
187+
[null],
188+
];
189+
}
190+
177191
private function getTokenManager()
178192
{
179193
return $this->createMock(CsrfTokenManagerInterface::class);

‎Tests/Firewall/UsernamePasswordFormAuthenticationListenerTest.php‎

Lines changed: 85 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorage;
2525
use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface;
2626
use Symfony\Component\Security\Core\Security;
27+
use Symfony\Component\Security\Csrf\CsrfTokenManagerInterface;
2728
use Symfony\Component\Security\Http\Authentication\AuthenticationFailureHandlerInterface;
2829
use Symfony\Component\Security\Http\Authentication\DefaultAuthenticationFailureHandler;
2930
use Symfony\Component\Security\Http\Authentication\DefaultAuthenticationSuccessHandler;
@@ -37,7 +38,7 @@ class UsernamePasswordFormAuthenticationListenerTest extends TestCase
3738
/**
3839
* @dataProvider getUsernameForLength
3940
*/
40-
public function testHandleWhenUsernameLength($username, $ok)
41+
public function testHandleWhenUsernameLength(string $username, bool $ok)
4142
{
4243
$request = Request::create('/login_check', 'POST', ['_username' => $username]);
4344
$request->setSession($this->createMock(SessionInterface::class));
@@ -84,10 +85,8 @@ public function testHandleWhenUsernameLength($username, $ok)
8485
/**
8586
* @dataProvider postOnlyDataProvider
8687
*/
87-
public function testHandleNonStringUsernameWithArray($postOnly)
88+
public function testHandleNonStringUsernameWithArray(bool $postOnly)
8889
{
89-
$this->expectException(BadRequestHttpException::class);
90-
$this->expectExceptionMessage('The key "_username" must be a string, "array" given.');
9190
$request = Request::create('/login_check', 'POST', ['_username' => []]);
9291
$request->setSession($this->createMock(SessionInterface::class));
9392
$listener = new UsernamePasswordFormAuthenticationListener(
@@ -101,16 +100,18 @@ public function testHandleNonStringUsernameWithArray($postOnly)
101100
['require_previous_session' => false, 'post_only' => $postOnly]
102101
);
103102
$event = new RequestEvent($this->createMock(HttpKernelInterface::class), $request, HttpKernelInterface::MASTER_REQUEST);
103+
104+
$this->expectException(BadRequestHttpException::class);
105+
$this->expectExceptionMessage('The key "_username" must be a string, "array" given.');
106+
104107
$listener($event);
105108
}
106109

107110
/**
108111
* @dataProvider postOnlyDataProvider
109112
*/
110-
public function testHandleNonStringUsernameWithInt($postOnly)
113+
public function testHandleNonStringUsernameWithInt(bool $postOnly)
111114
{
112-
$this->expectException(BadRequestHttpException::class);
113-
$this->expectExceptionMessage('The key "_username" must be a string, "integer" given.');
114115
$request = Request::create('/login_check', 'POST', ['_username' => 42]);
115116
$request->setSession($this->createMock(SessionInterface::class));
116117
$listener = new UsernamePasswordFormAuthenticationListener(
@@ -124,16 +125,18 @@ public function testHandleNonStringUsernameWithInt($postOnly)
124125
['require_previous_session' => false, 'post_only' => $postOnly]
125126
);
126127
$event = new RequestEvent($this->createMock(HttpKernelInterface::class), $request, HttpKernelInterface::MASTER_REQUEST);
128+
129+
$this->expectException(BadRequestHttpException::class);
130+
$this->expectExceptionMessage('The key "_username" must be a string, "integer" given.');
131+
127132
$listener($event);
128133
}
129134

130135
/**
131136
* @dataProvider postOnlyDataProvider
132137
*/
133-
public function testHandleNonStringUsernameWithObject($postOnly)
138+
public function testHandleNonStringUsernameWithObject(bool $postOnly)
134139
{
135-
$this->expectException(BadRequestHttpException::class);
136-
$this->expectExceptionMessage('The key "_username" must be a string, "object" given.');
137140
$request = Request::create('/login_check', 'POST', ['_username' => new \stdClass()]);
138141
$request->setSession($this->createMock(SessionInterface::class));
139142
$listener = new UsernamePasswordFormAuthenticationListener(
@@ -147,13 +150,17 @@ public function testHandleNonStringUsernameWithObject($postOnly)
147150
['require_previous_session' => false, 'post_only' => $postOnly]
148151
);
149152
$event = new RequestEvent($this->createMock(HttpKernelInterface::class), $request, HttpKernelInterface::MASTER_REQUEST);
153+
154+
$this->expectException(BadRequestHttpException::class);
155+
$this->expectExceptionMessage('The key "_username" must be a string, "object" given.');
156+
150157
$listener($event);
151158
}
152159

153160
/**
154161
* @dataProvider postOnlyDataProvider
155162
*/
156-
public function testHandleNonStringUsernameWith__toString($postOnly)
163+
public function testHandleNonStringUsernameWith__toString(bool $postOnly)
157164
{
158165
$usernameClass = $this->createMock(DummyUserClass::class);
159166
$usernameClass
@@ -177,21 +184,86 @@ public function testHandleNonStringUsernameWith__toString($postOnly)
177184
$listener($event);
178185
}
179186

180-
public function postOnlyDataProvider()
187+
/**
188+
* @dataProvider provideInvalidCsrfTokens
189+
*/
190+
public function testInvalidCsrfToken($invalidToken)
191+
{
192+
$formBody = ['_username' => 'fabien', '_password' => 'symfony'];
193+
if (null !== $invalidToken) {
194+
$formBody['_csrf_token'] = $invalidToken;
195+
}
196+
197+
$request = Request::create('/login_check', 'POST', $formBody);
198+
$request->setSession($this->createMock(SessionInterface::class));
199+
200+
$httpUtils = $this->createMock(HttpUtils::class);
201+
$httpUtils
202+
->method('checkRequestPath')
203+
->willReturn(true)
204+
;
205+
$httpUtils
206+
->method('createRedirectResponse')
207+
->willReturn(new RedirectResponse('/hello'))
208+
;
209+
210+
$failureHandler = $this->createMock(AuthenticationFailureHandlerInterface::class);
211+
$failureHandler
212+
->expects($this->once())
213+
->method('onAuthenticationFailure')
214+
->willReturn(new Response())
215+
;
216+
217+
$authenticationManager = $this->createMock(AuthenticationProviderManager::class);
218+
$authenticationManager
219+
->expects($this->never())
220+
->method('authenticate')
221+
;
222+
223+
$csrfTokenManager = $this->createMock(CsrfTokenManagerInterface::class);
224+
$csrfTokenManager->method('isTokenValid')->willReturn(false);
225+
226+
$listener = new UsernamePasswordFormAuthenticationListener(
227+
$this->createMock(TokenStorageInterface::class),
228+
$authenticationManager,
229+
$this->createMock(SessionAuthenticationStrategyInterface::class),
230+
$httpUtils,
231+
'TheProviderKey',
232+
new DefaultAuthenticationSuccessHandler($httpUtils),
233+
$failureHandler,
234+
['require_previous_session' => false],
235+
null,
236+
null,
237+
$csrfTokenManager
238+
);
239+
240+
$listener(new RequestEvent($this->createMock(HttpKernelInterface::class), $request, HttpKernelInterface::MASTER_REQUEST));
241+
}
242+
243+
public function postOnlyDataProvider(): array
181244
{
182245
return [
183246
[true],
184247
[false],
185248
];
186249
}
187250

188-
public function getUsernameForLength()
251+
public function getUsernameForLength(): array
189252
{
190253
return [
191254
[str_repeat('x', Security::MAX_USERNAME_LENGTH + 1), false],
192255
[str_repeat('x', Security::MAX_USERNAME_LENGTH - 1), true],
193256
];
194257
}
258+
259+
public function provideInvalidCsrfTokens(): array
260+
{
261+
return [
262+
['invalid'],
263+
[['in' => 'valid']],
264+
[null],
265+
];
266+
}
195267
}
196268

197269
class DummyUserClass

0 commit comments

Comments
 (0)