BUG Capture errors after a reload token redirect to login url

Fixes #7289

src/Core/Startup/ErrorControlChainMiddleware.php

@@ -49,7 +49,7 @@ class ErrorControlChainMiddleware implements HTTPMiddleware
 
                 try {
                     // Check if a token is requesting a redirect
-                    if ($reloadToken) {
+                    if ($reloadToken && $reloadToken->reloadRequired()) {
                         $result = $this->safeReloadWithToken($request, $reloadToken);
                     } else {
                         // If no reload necessary, process application
@@ -61,7 +61,7 @@ class ErrorControlChainMiddleware implements HTTPMiddleware
             })
             // Finally if a token was requested but there was an error while figuring out if it's allowed, do it anyway
             ->thenIfErrored(function () use ($reloadToken) {
-                if ($reloadToken) {
+                if ($reloadToken && $reloadToken->reloadRequiredIfError()) {
                     $result = $reloadToken->reloadWithToken();
                     $result->output();
                 }
@@ -87,14 +87,20 @@ class ErrorControlChainMiddleware implements HTTPMiddleware
         $request->getSession()->init($request);
 
         // Request with ErrorDirector
-        $result = ErrorDirector::singleton()->handleRequestWithToken($request, $reloadToken, $this->getApplication()->getKernel());
+        $result = ErrorDirector::singleton()->handleRequestWithToken(
+            $request,
+            $reloadToken,
+            $this->getApplication()->getKernel()
+        );
         if ($result) {
             return $result;
         }
 
         // Fail and redirect the user to the login page
+        $params = array_merge($request->getVars(), $reloadToken->params(false));
+        $backURL = $request->getURL(). '?' . http_build_query($params);
         $loginPage = Director::absoluteURL(Security::config()->get('login_url'));
-        $loginPage .= "?BackURL=" . urlencode($request->getURL());
+        $loginPage .= "?BackURL=" . urlencode($backURL);
         $result = new HTTPResponse();
         $result->redirect($loginPage);
         return $result;

src/Core/Startup/ParameterConfirmationToken.php

@@ -2,6 +2,7 @@
 
 namespace SilverStripe\Core\Startup;
 
+use function GuzzleHttp\Psr7\parse_query;
 use SilverStripe\Control\Controller;
 use SilverStripe\Control\HTTPRequest;
 use SilverStripe\Control\HTTPResponse;
@@ -34,12 +35,19 @@ class ParameterConfirmationToken
     protected $request = null;
 
     /**
-     * The parameter given
+     * The parameter given in the main request
      *
      * @var string|null The string value, or null if not provided
      */
     protected $parameter = null;
 
+    /**
+     * The parameter given in the backURL
+     *
+     * @var string|null
+     */
+    protected $parameterBackURL = null;
+
     /**
      * The validated and checked token for this parameter
      *
@@ -106,6 +114,7 @@ class ParameterConfirmationToken
 
         // Store the parameter value
         $this->parameter = $request->getVar($parameterName);
+        $this->parameterBackURL = $this->backURLToken($request);
 
         // If the token provided is valid, mark it as such
         $token = $request->getVar($parameterName.'token');
@@ -114,6 +123,29 @@ class ParameterConfirmationToken
         }
     }
 
+    /**
+     * Check if this token exists in the BackURL
+     *
+     * @param HTTPRequest $request
+     * @return string Value of token in backurl, or null if not in backurl
+     */
+    protected function backURLToken(HTTPRequest $request)
+    {
+        $backURL = $request->getVar('BackURL');
+        if (!strstr($backURL, '?')) {
+            return null;
+        }
+
+        // Filter backURL if it contains the given request parameter
+        list(,$query) = explode('?', $backURL);
+        $queryArgs = parse_query($query);
+        $name = $this->getName();
+        if (isset($queryArgs[$name])) {
+            return $queryArgs[$name];
+        }
+        return null;
+    }
+
     /**
      * Get the name of this token
      *
@@ -135,6 +167,16 @@ class ParameterConfirmationToken
         return $this->parameter !== null;
     }
 
+    /**
+     * Is the parmeter requested in a BackURL param?
+     *
+     * @return bool
+     */
+    public function existsInReferer()
+    {
+        return $this->parameterBackURL !== null;
+    }
+
     /**
      * Is the necessary token provided for this parameter?
      * A value must be provided for the token
@@ -156,6 +198,18 @@ class ParameterConfirmationToken
         return $this->parameterProvided() && !$this->tokenProvided();
     }
 
+    /**
+     * Check if this token is provided either in the backurl, or directly,
+     * but without a token
+     *
+     * @return bool
+     */
+    public function reloadRequiredIfError()
+    {
+        // Don't reload if token exists
+        return $this->reloadRequired() || $this->existsInReferer();
+    }
+
     /**
      * Suppress the current parameter by unsetting it from $_GET
      */
@@ -167,14 +221,18 @@ class ParameterConfirmationToken
     /**
      * Determine the querystring parameters to include
      *
+     * @param bool $includeToken Include the token value as well?
      * @return array List of querystring parameters with name and token parameters
      */
-    public function params()
+    public function params($includeToken = true)
     {
-        return array(
+        $params = array(
             $this->parameterName => $this->parameter,
-            $this->parameterName.'token' => $this->genToken()
         );
+        if ($includeToken) {
+            $params[$this->parameterName . 'token'] = $this->genToken();
+        }
+        return $params;
     }
 
     /**
@@ -190,6 +248,26 @@ class ParameterConfirmationToken
         );
     }
 
+    /**
+     * Get redirection URL
+     *
+     * @return string
+     */
+    protected function redirectURL()
+    {
+        // If url is encoded via BackURL, defer to home page (prevent redirect to form action)
+        if ($this->existsInReferer() && !$this->parameterProvided()) {
+            $url = BASE_URL ?: '/';
+            $params = $this->params();
+        } else {
+            $url = $this->currentURL();
+            $params = array_merge($this->request->getVars(), $this->params());
+        }
+
+        // Merge get params with current url
+        return Controller::join_links($url, '?' . http_build_query($params));
+    }
+
     /**
      * Forces a reload of the request with the token included
      *
@@ -197,12 +275,7 @@ class ParameterConfirmationToken
      */
     public function reloadWithToken()
     {
-        // Merge get params with current url
+        $location = $this->redirectURL();
-        $params = array_merge($this->request->getVars(), $this->params());
-        $location = Controller::join_links(
-            $this->currentURL(),
-            '?'.http_build_query($params)
-        );
         $locationJS = Convert::raw2js($location);
         $locationATT = Convert::raw2att($location);
         $body = <<<HTML
@@ -231,7 +304,7 @@ HTML;
         foreach ($keys as $key) {
             $token = new ParameterConfirmationToken($key, $request);
             // Validate this token
-            if ($token->reloadRequired()) {
+            if ($token->reloadRequired() || $token->reloadRequiredIfError()) {
                 $token->suppress();
                 $target = $token;
             }

tests/php/Core/Startup/ParameterConfirmationTokenTest.php

@@ -29,7 +29,8 @@ class ParameterConfirmationTokenTest extends SapphireTest
         $get['parameterconfirmationtokentest_nulltokentoken'] = null;
         $get['parameterconfirmationtokentest_emptytoken'] = '1';
         $get['parameterconfirmationtokentest_emptytokentoken'] = '';
-        $this->request = new HTTPRequest('GET', '/', $get);
+        $get['BackURL'] = 'page?parameterconfirmationtokentest_backtoken=1';
+        $this->request = new HTTPRequest('GET', 'anotherpage', $get);
         $this->request->setSession(new Session([]));
     }
 
@@ -41,6 +42,7 @@ class ParameterConfirmationTokenTest extends SapphireTest
         $withoutParameter = new ParameterConfirmationTokenTest_Token('parameterconfirmationtokentest_noparam', $this->request);
         $nullToken = new ParameterConfirmationTokenTest_Token('parameterconfirmationtokentest_nulltoken', $this->request);
         $emptyToken = new ParameterConfirmationTokenTest_Token('parameterconfirmationtokentest_emptytoken', $this->request);
+        $backToken = new ParameterConfirmationTokenTest_Token('parameterconfirmationtokentest_backtoken', $this->request);
 
         // Check parameter
         $this->assertTrue($withoutToken->parameterProvided());
@@ -49,6 +51,16 @@ class ParameterConfirmationTokenTest extends SapphireTest
         $this->assertFalse($withoutParameter->parameterProvided());
         $this->assertTrue($nullToken->parameterProvided());
         $this->assertTrue($emptyToken->parameterProvided());
+        $this->assertFalse($backToken->parameterProvided());
+
+        // Check backurl
+        $this->assertFalse($withoutToken->existsInReferer());
+        $this->assertFalse($emptyParameter->existsInReferer());  // even if empty, it's still provided
+        $this->assertFalse($withToken->existsInReferer());
+        $this->assertFalse($withoutParameter->existsInReferer());
+        $this->assertFalse($nullToken->existsInReferer());
+        $this->assertFalse($emptyToken->existsInReferer());
+        $this->assertTrue($backToken->existsInReferer());
 
         // Check token
         $this->assertFalse($withoutToken->tokenProvided());
@@ -57,6 +69,7 @@ class ParameterConfirmationTokenTest extends SapphireTest
         $this->assertFalse($withoutParameter->tokenProvided());
         $this->assertFalse($nullToken->tokenProvided());
         $this->assertFalse($emptyToken->tokenProvided());
+        $this->assertFalse($backToken->tokenProvided());
 
         // Check if reload is required
         $this->assertTrue($withoutToken->reloadRequired());
@@ -65,6 +78,25 @@ class ParameterConfirmationTokenTest extends SapphireTest
         $this->assertFalse($withoutParameter->reloadRequired());
         $this->assertTrue($nullToken->reloadRequired());
         $this->assertTrue($emptyToken->reloadRequired());
+        $this->assertFalse($backToken->reloadRequired());
+
+        // Check if a reload is required in case of error
+        $this->assertTrue($withoutToken->reloadRequiredIfError());
+        $this->assertTrue($emptyParameter->reloadRequiredIfError());
+        $this->assertFalse($withToken->reloadRequiredIfError());
+        $this->assertFalse($withoutParameter->reloadRequiredIfError());
+        $this->assertTrue($nullToken->reloadRequiredIfError());
+        $this->assertTrue($emptyToken->reloadRequiredIfError());
+        $this->assertTrue($backToken->reloadRequiredIfError());
+
+        // Check redirect url
+        $home = (BASE_URL ?: '/') . '?';
+        $current = Controller::join_links(BASE_URL, '/', 'anotherpage'). '?';
+        $this->assertStringStartsWith($current, $withoutToken->redirectURL());
+        $this->assertStringStartsWith($current, $emptyParameter->redirectURL());
+        $this->assertStringStartsWith($current, $nullToken->redirectURL());
+        $this->assertStringStartsWith($current, $emptyToken->redirectURL());
+        $this->assertStringStartsWith($home, $backToken->redirectURL());
 
         // Check suppression
         $this->assertEquals('value', $this->request->getVar('parameterconfirmationtokentest_notoken'));
@@ -90,6 +122,25 @@ class ParameterConfirmationTokenTest extends SapphireTest
             $this->request
         );
         $this->assertEmpty($token);
+
+        // Test backurl token
+        $token = ParameterConfirmationToken::prepare_tokens(
+            [ 'parameterconfirmationtokentest_backtoken' ],
+            $this->request
+        );
+        $this->assertEquals('parameterconfirmationtokentest_backtoken', $token->getName());
+    }
+
+    public function dataProviderURLs()
+    {
+        return [
+            [''],
+            ['/'],
+            ['bar'],
+            ['bar/'],
+            ['/bar'],
+            ['/bar/'],
+        ];
     }
 
     /**
@@ -97,18 +148,18 @@ class ParameterConfirmationTokenTest extends SapphireTest
      *
      * There should always be exactly one slash between each part in the result, and any trailing slash
      * should be preserved.
+     *
+     * @dataProvider dataProviderURLs
      */
-    public function testCurrentAbsoluteURLHandlesSlashes()
+    public function testCurrentAbsoluteURLHandlesSlashes($url)
     {
+        $this->request->setUrl($url);
+
         $token = new ParameterConfirmationTokenTest_Token(
             'parameterconfirmationtokentest_parameter',
             $this->request
         );
-
+        $expected = rtrim(Controller::join_links(BASE_URL, '/', $url), '/') ?: '/';
-        foreach (array('', '/', 'bar', 'bar/', '/bar', '/bar/') as $url) {
+        $this->assertEquals($expected, $token->currentURL(), "Invalid redirect for request url $url");
-            $this->request->setUrl($url);
-            $expected = rtrim(Controller::join_links(BASE_URL, '/', $url), '/') ?: '/';
-            $this->assertEquals($expected, $token->currentURL(), "Invalid redirect for request url $url");
-        }
     }
 }

tests/php/Core/Startup/ParameterConfirmationTokenTest/ParameterConfirmationTokenTest_Token.php

@@ -15,4 +15,9 @@ class ParameterConfirmationTokenTest_Token extends ParameterConfirmationToken im
     {
         return parent::currentURL();
     }
+
+    public function redirectURL()
+    {
+        return parent::redirectURL();
+    }
 }