Merge pull request #282 from cakephp/issue-281

Only include path + query in redirect targets

Author: markstory

docs/en/migration-from-the-authcomponent.rst

@@ -208,7 +208,7 @@ using the ``queryParam`` option::
 
        // Add the authentication middleware
        $authentication = new AuthenticationMiddleware($this, [
-           'unauthenticatedRedirect' => Router::url('users:login'),
+           'unauthenticatedRedirect' => '/users/login',
            'queryParam' => 'redirect',
        ]);
 
@@ -218,6 +218,30 @@ using the ``queryParam`` option::
        return $middlewareQueue;
    }
 
+Then in your controller's login method you can use the redirect query parameter::
+
+    public function login()
+    {
+        $result = $this->Authentication->getResult();
+
+        // Regardless of POST or GET, redirect if user is logged in
+        if ($result->isValid()) {
+            // Use the redirect parameter if present. Only use the path
+            // and query segments to prevent an open redirect.
+            if ($this->request->getQuery('redirect')) {
+                $parsed = parse_url($this->request->getQuery('redirect'));
+                if ($parsed === false) {
+                    $parsed = ['path' => '/', 'query' => ''];
+                }
+                $parsed += ['path' => '/', 'query' => ''];
+                $redirect = $parsed['path'] . '?' . $parsed['query'];
+            } else {
+                $redirect = ['controller' => 'Pages', 'action' => 'display', 'home'];
+            }
+            return $this->redirect($redirect);
+        }
+    }
+
 Migrating Hashing Upgrade Logic
 ===============================
 
@@ -241,10 +265,11 @@ leveraging the ``AuthenticationService``::
                $this->Users->save($user);
            }
 
-           $redirect = $this->request->getQuery(
-               'redirect',
-               ['controller' => 'Pages', 'action' => 'display', 'home']
-           );
-           return $this->redirect($redirect);
+           // Redirect to a logged in page
+           return $this->redirect([
+               'controller' => 'Pages',
+               'action' => 'display',
+               'home'
+           ]);
        }
    }

src/Middleware/AuthenticationMiddleware.php

@@ -150,7 +150,11 @@ protected function getRedirectUrl($target, ServerRequestInterface $request)
         if (property_exists($uri, 'base')) {
             $uri = $uri->withPath($uri->base . $uri->getPath());
         }
-        $query = urlencode($param) . '=' . urlencode($uri);
+        $redirect = $uri->getPath();
+        if ($uri->getQuery()) {
+            $redirect .= '?' . $uri->getQuery();
+        }
+        $query = urlencode($param) . '=' . urlencode($redirect);
 
         $url = parse_url($target);
         if (isset($url['query']) && strlen($url['query'])) {

tests/TestCase/Middleware/AuthenticationMiddlewareTest.php

@@ -469,7 +469,7 @@ public function testUnauthenticatedRedirectWithQuery()
 
         $response = $middleware($request, $response, $next);
         $this->assertSame(302, $response->getStatusCode());
-        $this->assertSame('/users/login?redirect=http%3A%2F%2Flocalhost%2Ftestpath', $response->getHeaderLine('Location'));
+        $this->assertSame('/users/login?redirect=%2Ftestpath', $response->getHeaderLine('Location'));
         $this->assertSame('', $response->getBody() . '');
     }
 
@@ -498,7 +498,7 @@ public function testUnauthenticatedRedirectWithExistingQuery()
 
         $response = $middleware($request, $response, $next);
         $this->assertSame(302, $response->getStatusCode());
-        $this->assertSame('/users/login?hello=world&redirect=http%3A%2F%2Flocalhost%2Ftestpath', $response->getHeaderLine('Location'));
+        $this->assertSame('/users/login?hello=world&redirect=%2Ftestpath', $response->getHeaderLine('Location'));
         $this->assertSame('', $response->getBody() . '');
     }
 
@@ -528,7 +528,7 @@ public function testUnauthenticatedRedirectWithFragment()
         $response = $middleware($request, $response, $next);
         $this->assertSame(302, $response->getStatusCode());
         $this->assertSame(
-            '/users/login?hello=world&redirect=http%3A%2F%2Flocalhost%2Ftestpath#frag',
+            '/users/login?hello=world&redirect=%2Ftestpath#frag',
             $response->getHeaderLine('Location')
         );
         $this->assertSame('', $response->getBody() . '');
@@ -563,7 +563,36 @@ public function testUnauthenticatedRedirectWithBase()
 
         $response = $middleware($request, $response, $next);
         $this->assertSame(302, $response->getStatusCode());
-        $this->assertSame('/users/login?redirect=http%3A%2F%2Flocalhost%2Fbase%2Ftestpath', $response->getHeaderLine('Location'));
+        $this->assertSame('/users/login?redirect=%2Fbase%2Ftestpath', $response->getHeaderLine('Location'));
+        $this->assertSame('', $response->getBody() . '');
+    }
+
+    /**
+     * test unauthenticated redirects preserving path and query
+     *
+     * @return void
+     */
+    public function testUnauthenticatedRedirectWithQueryStringData()
+    {
+        $request = ServerRequestFactory::fromGlobals(
+            ['REQUEST_URI' => '/testpath', 'QUERY_STRING' => 'a=b&c=d'],
+            [],
+            ['username' => 'mariano', 'password' => 'password']
+        );
+        $response = new Response();
+
+        $middleware = new AuthenticationMiddleware($this->service, [
+            'unauthenticatedRedirect' => '/users/login',
+            'queryParam' => 'redirect',
+        ]);
+
+        $next = function ($request, $response) {
+            throw new UnauthenticatedException();
+        };
+
+        $response = $middleware($request, $response, $next);
+        $this->assertSame(302, $response->getStatusCode());
+        $this->assertSame('/users/login?redirect=%2Ftestpath%3Fa%3Db%26c%3Dd', $response->getHeaderLine('Location'));
         $this->assertSame('', $response->getBody() . '');
     }