Skip to content

Commit 9aec999

Browse files
authored
Merge pull request #799 from cakephp/backport-796
Backport security patches from 3.x and 4.x
2 parents 3f7cb4a + 5bc0e99 commit 9aec999

3 files changed

Lines changed: 54 additions & 2 deletions

File tree

composer.json

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,11 @@
8080
"sort-packages": true,
8181
"allow-plugins": {
8282
"dealerdirect/phpcodesniffer-composer-installer": true
83+
},
84+
"policy": {
85+
"advisories": {
86+
"ignore": ["PKSA-y2cr-5h3j-g3ys"]
87+
}
8388
}
8489
},
8590
"minimum-stability": "dev",

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 (strpos($normalized, '//') === 0) {
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 (strpos($parsed['path'], '\\') !== false) {
437+
return null;
438+
}
425439
if (strlen($parsed['path']) && $parsed['path'][0] !== '/') {
426440
$parsed['path'] = "/{$parsed['path']}";
427441
}

tests/TestCase/AuthenticationServiceTest.php

Lines changed: 34 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,39 @@ 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+
);
891924
}
892925

893926
/**

0 commit comments

Comments
 (0)