Merge pull request #796 from cakephp/fix-openredirect-3x

Fix an open redirect weakness in getLoginRedirect()

Author: markstory

composer.json

@@ -61,6 +61,11 @@
         "allow-plugins": {
             "dealerdirect/phpcodesniffer-composer-installer": true
         },
+        "policy": {
+            "advisories": {
+                "ignore": ["PKSA-y2cr-5h3j-g3ys"]
+            }
+        },
         "sort-packages": true
     },
     "scripts": {

src/AuthenticationService.php

@@ -459,15 +459,29 @@ public function getLoginRedirect(ServerRequestInterface $request): ?string
         ) {
             return null;
         }
+        $value = (string)$params[$redirectParam];
 
-        $parsed = parse_url($params[$redirectParam]);
+        // In the `Location` header, Browsers normalize \ to /
+        // (see WHATWG URL Standard).
+        // We do the same to prevent injection via \ sequences.
+        $normalized = str_replace('\\', '/', $value);
+
+        // A leading run of `//` or `\\` are rejected
+        if (str_starts_with($normalized, '//')) {
+            return null;
+        }
+
+        $parsed = parse_url($normalized);
         if ($parsed === false) {
             return null;
         }
         if (!empty($parsed['host']) || !empty($parsed['scheme'])) {
             return null;
         }
         $parsed += ['path' => '/', 'query' => ''];
+        if (str_contains($parsed['path'], '\\')) {
+            return null;
+        }
         if (strlen($parsed['path']) && $parsed['path'][0] !== '/') {
             $parsed['path'] = "/{$parsed['path']}";
         }

tests/TestCase/AuthenticationServiceTest.php

@@ -915,7 +915,7 @@ public function testGetUnauthenticatedRedirectUrlWithBasePath()
         );
     }
 
-    public function testGetLoginRedirect()
+    public function testGetLoginRedirectInvalid()
     {
         $service = new AuthenticationService([
             'unauthenticatedRedirect' => '/users/login',
@@ -956,6 +956,39 @@ public function testGetLoginRedirect()
             '/path/with?query=string',
             $service->getLoginRedirect($request),
         );
+
+        $request = ServerRequestFactory::fromGlobals(
+            ['REQUEST_URI' => '/login'],
+            ['redirect' => '/\\evil.com'],
+        );
+        $this->assertNull(
+            $service->getLoginRedirect($request),
+        );
+
+        $request = ServerRequestFactory::fromGlobals(
+            ['REQUEST_URI' => '/login'],
+            ['redirect' => '\\/\\evil.com'],
+        );
+        $this->assertNull(
+            $service->getLoginRedirect($request),
+        );
+
+        $request = ServerRequestFactory::fromGlobals(
+            ['REQUEST_URI' => '/login'],
+            ['redirect' => '\\evil.com/path'],
+        );
+        $this->assertSame(
+            '/evil.com/path',
+            $service->getLoginRedirect($request),
+        );
+
+        $request = ServerRequestFactory::fromGlobals(
+            ['REQUEST_URI' => '/login'],
+            ['redirect' => '\\\\evil.com/path'],
+        );
+        $this->assertNull(
+            $service->getLoginRedirect($request),
+        );
     }
 
     /**