Skip to content

Commit df28ea4

Browse files
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 9a12edc commit df28ea4

2 files changed

Lines changed: 49 additions & 2 deletions

File tree

src/AuthenticationService.php

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -459,15 +459,29 @@ public function getLoginRedirect(ServerRequestInterface $request): ?string
459459
) {
460460
return null;
461461
}
462+
$value = (string)$params[$redirectParam];
462463

463-
$parsed = parse_url($params[$redirectParam]);
464+
// In the `Location` header, Browsers normalize \ to /
465+
// (see WHATWG URL Standard).
466+
// We do the same to prevent injection via \ sequences.
467+
$normalized = str_replace('\\', '/', $value);
468+
469+
// A leading run of `//` or `\\` are rejected
470+
if (str_starts_with($normalized, '//')) {
471+
return null;
472+
}
473+
474+
$parsed = parse_url($normalized);
464475
if ($parsed === false) {
465476
return null;
466477
}
467478
if (!empty($parsed['host']) || !empty($parsed['scheme'])) {
468479
return null;
469480
}
470481
$parsed += ['path' => '/', 'query' => ''];
482+
if (str_contains($parsed['path'], '\\')) {
483+
return null;
484+
}
471485
if (strlen($parsed['path']) && $parsed['path'][0] !== '/') {
472486
$parsed['path'] = "/{$parsed['path']}";
473487
}

tests/TestCase/AuthenticationServiceTest.php

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -915,7 +915,7 @@ public function testGetUnauthenticatedRedirectUrlWithBasePath()
915915
);
916916
}
917917

918-
public function testGetLoginRedirect()
918+
public function testGetLoginRedirectInvalid()
919919
{
920920
$service = new AuthenticationService([
921921
'unauthenticatedRedirect' => '/users/login',
@@ -956,6 +956,39 @@ public function testGetLoginRedirect()
956956
'/path/with?query=string',
957957
$service->getLoginRedirect($request),
958958
);
959+
960+
$request = ServerRequestFactory::fromGlobals(
961+
['REQUEST_URI' => '/login'],
962+
['redirect' => '/\\evil.com'],
963+
);
964+
$this->assertNull(
965+
$service->getLoginRedirect($request),
966+
);
967+
968+
$request = ServerRequestFactory::fromGlobals(
969+
['REQUEST_URI' => '/login'],
970+
['redirect' => '\\/\\evil.com'],
971+
);
972+
$this->assertNull(
973+
$service->getLoginRedirect($request),
974+
);
975+
976+
$request = ServerRequestFactory::fromGlobals(
977+
['REQUEST_URI' => '/login'],
978+
['redirect' => '\\evil.com/path'],
979+
);
980+
$this->assertSame(
981+
'/evil.com/path',
982+
$service->getLoginRedirect($request),
983+
);
984+
985+
$request = ServerRequestFactory::fromGlobals(
986+
['REQUEST_URI' => '/login'],
987+
['redirect' => '\\\\evil.com/path'],
988+
);
989+
$this->assertNull(
990+
$service->getLoginRedirect($request),
991+
);
959992
}
960993

961994
/**

0 commit comments

Comments
 (0)