Skip to content

Commit 1c1e29c

Browse files
markstorymarkstory
committed
Fix an open redirect weakness in getLoginRedirect()
Backport of #795 to 3.x Because of how browsers handle the `Location` header, values beginning with `\` can be leveraged to create redirect targets on other domains. Thanks to Volker Dusch and the PHP Ecosystem security team for reporting this.
1 parent 3f7cb4a commit 1c1e29c

2 files changed

Lines changed: 209 additions & 2 deletions

File tree

‎src/AuthenticationService.php‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -413,15 +413,29 @@ public function getLoginRedirect(ServerRequestInterface $request): ?string
413413
) {
414414
return null;
415415
}
416+
$value = (string)$params[$redirectParam];
416417

417-
$parsed = parse_url($params[$redirectParam]);
418+
// In the `Location` header, Browsers normalize \ to /
419+
// (see WHATWG URL Standard).
420+
// We do the same to prevent injection via \ sequences.
421+
$normalized = str_replace('\\', '/', $value);
422+
423+
// A leading run of `//` or `\\` are rejected
424+
if (str_starts_with($normalized, '//')) {
425+
return null;
426+
}
427+
428+
$parsed = parse_url($normalized);
418429
if ($parsed === false) {
419430
return null;
420431
}
421432
if (!empty($parsed['host']) || !empty($parsed['scheme'])) {
422433
return null;
423434
}
424435
$parsed += ['path' => '/', 'query' => ''];
436+
if (str_contains($parsed['path'], '\\')) {
437+
return null;
438+
}
425439
if (strlen($parsed['path']) && $parsed['path'][0] !== '/') {
426440
$parsed['path'] = "/{$parsed['path']}";
427441
}

‎tests/TestCase/AuthenticationServiceTest.php‎

Lines changed: 194 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -847,7 +847,7 @@ public function testGetUnauthenticatedRedirectUrlWithBasePath()
847847
);
848848
}
849849

850-
public function testGetLoginRedirect()
850+
public function testGetLoginRedirectInvalid()
851851
{
852852
$service = new AuthenticationService([
853853
'unauthenticatedRedirect' => '/users/login',
@@ -888,6 +888,199 @@ public function testGetLoginRedirect()
888888
'/path/with?query=string',
889889
$service->getLoginRedirect($request)
890890
);
891+
892+
$request = ServerRequestFactory::fromGlobals(
893+
['REQUEST_URI' => '/login'],
894+
['redirect' => '/\\evil.com']
895+
);
896+
$this->assertNull(
897+
$service->getLoginRedirect($request)
898+
);
899+
900+
$request = ServerRequestFactory::fromGlobals(
901+
['REQUEST_URI' => '/login'],
902+
['redirect' => '\\/\\evil.com']
903+
);
904+
$this->assertNull(
905+
$service->getLoginRedirect($request)
906+
);
907+
908+
$request = ServerRequestFactory::fromGlobals(
909+
['REQUEST_URI' => '/login'],
910+
['redirect' => '\\evil.com/path']
911+
);
912+
$this->assertSame(
913+
'/evil.com/path',
914+
$service->getLoginRedirect($request)
915+
);
916+
917+
$request = ServerRequestFactory::fromGlobals(
918+
['REQUEST_URI' => '/login'],
919+
['redirect' => '\\\\evil.com/path']
920+
);
921+
$this->assertNull(
922+
$service->getLoginRedirect($request)
923+
);
924+
}
925+
926+
/**
927+
* testGetLoginRedirectValidationDisabled
928+
*
929+
* @return void
930+
*/
931+
public function testGetLoginRedirectValidationDisabled()
932+
{
933+
$service = new AuthenticationService([
934+
'unauthenticatedRedirect' => '/users/login',
935+
'queryParam' => 'redirect',
936+
'redirectValidation' => [
937+
'enabled' => false,
938+
]
939+
]);
940+
941+
// With validation disabled, even nested redirects should pass
942+
$request = ServerRequestFactory::fromGlobals(
943+
['REQUEST_URI' => '/secrets'],
944+
['redirect' => '/login?redirect=/secrets']
945+
);
946+
$this->assertSame(
947+
'/login?redirect=/secrets',
948+
$service->getLoginRedirect($request)
949+
);
950+
}
951+
952+
/**
953+
* testGetLoginRedirectValidationNestedRedirects
954+
*
955+
* @return void
956+
*/
957+
public function testGetLoginRedirectValidationNestedRedirects()
958+
{
959+
$service = new AuthenticationService([
960+
'unauthenticatedRedirect' => '/users/login',
961+
'queryParam' => 'redirect',
962+
'redirectValidation' => [
963+
'enabled' => true,
964+
]
965+
]);
966+
967+
// Valid single-level redirect
968+
$request = ServerRequestFactory::fromGlobals(
969+
['REQUEST_URI' => '/secrets'],
970+
['redirect' => '/articles/view/1']
971+
);
972+
$this->assertSame(
973+
'/articles/view/1',
974+
$service->getLoginRedirect($request)
975+
);
976+
977+
// Nested redirect (should be blocked)
978+
$request = ServerRequestFactory::fromGlobals(
979+
['REQUEST_URI' => '/secrets'],
980+
['redirect' => '/login?redirect=/articles/view/1']
981+
);
982+
$this->assertNull($service->getLoginRedirect($request));
983+
984+
// Deeply nested redirect (should be blocked)
985+
$request = ServerRequestFactory::fromGlobals(
986+
['REQUEST_URI' => '/secrets'],
987+
['redirect' => '/login?redirect=%2Flogin%3Fredirect%3D%252Farticles']
988+
);
989+
$this->assertNull($service->getLoginRedirect($request));
990+
}
991+
992+
/**
993+
* testGetLoginRedirectValidationEncodingLevels
994+
*
995+
* @return void
996+
*/
997+
public function testGetLoginRedirectValidationEncodingLevels()
998+
{
999+
$service = new AuthenticationService([
1000+
'unauthenticatedRedirect' => '/users/login',
1001+
'queryParam' => 'redirect',
1002+
'redirectValidation' => [
1003+
'enabled' => true,
1004+
]
1005+
]);
1006+
1007+
// Normal single-encoded URL (should pass)
1008+
$request = ServerRequestFactory::fromGlobals(
1009+
['REQUEST_URI' => '/secrets'],
1010+
['redirect' => '/articles%2Fview%2F1'],
1011+
);
1012+
$this->assertSame(
1013+
'/articles%2Fview%2F1',
1014+
$service->getLoginRedirect($request)
1015+
);
1016+
1017+
// Double-encoded URL (should be blocked)
1018+
$request = ServerRequestFactory::fromGlobals(
1019+
['REQUEST_URI' => '/secrets'],
1020+
['redirect' => '/articles%252Fview%252F1']
1021+
);
1022+
$this->assertNull($service->getLoginRedirect($request));
1023+
}
1024+
1025+
/**
1026+
* testGetLoginRedirectValidationMaxLength
1027+
*
1028+
* @return void
1029+
*/
1030+
public function testGetLoginRedirectValidationMaxLength()
1031+
{
1032+
$service = new AuthenticationService([
1033+
'unauthenticatedRedirect' => '/users/login',
1034+
'queryParam' => 'redirect',
1035+
'redirectValidation' => [
1036+
'enabled' => true,
1037+
'maxLength' => 100,
1038+
]
1039+
]);
1040+
1041+
// Short URL (should pass)
1042+
$request = ServerRequestFactory::fromGlobals(
1043+
['REQUEST_URI' => '/secrets'],
1044+
['redirect' => '/articles/view/1']
1045+
);
1046+
$this->assertSame(
1047+
'/articles/view/1',
1048+
$service->getLoginRedirect($request)
1049+
);
1050+
1051+
// Excessively long URL (should be blocked)
1052+
$longUrl = '/articles/' . str_repeat('a', 150);
1053+
$request = ServerRequestFactory::fromGlobals(
1054+
['REQUEST_URI' => '/secrets'],
1055+
['redirect' => $longUrl]
1056+
);
1057+
$this->assertNull($service->getLoginRedirect($request));
1058+
}
1059+
1060+
/**
1061+
* testGetLoginRedirectValidationWithQueryParameters
1062+
*
1063+
* @return void
1064+
*/
1065+
public function testGetLoginRedirectValidationWithQueryParameters()
1066+
{
1067+
$service = new AuthenticationService([
1068+
'unauthenticatedRedirect' => '/users/login',
1069+
'queryParam' => 'redirect',
1070+
'redirectValidation' => [
1071+
'enabled' => true,
1072+
]
1073+
]);
1074+
1075+
// Valid redirect with query parameters
1076+
$request = ServerRequestFactory::fromGlobals(
1077+
['REQUEST_URI' => '/secrets'],
1078+
['redirect' => '/articles/index?sort=created&direction=desc']
1079+
);
1080+
$this->assertSame(
1081+
'/articles/index?sort=created&direction=desc',
1082+
$service->getLoginRedirect($request)
1083+
);
8911084
}
8921085

8931086
/**

0 commit comments

Comments
 (0)