From 47fced88806935a65fb281951f14bc053b49bd97 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Wed, 23 Aug 2017 11:58:57 +1200 Subject: [PATCH] BUG Capture errors after a reload token redirect to login url Fixes #7289 --- .../Startup/ErrorControlChainMiddleware.php | 14 ++- .../Startup/ParameterConfirmationToken.php | 95 ++++++++++++++++--- .../ParameterConfirmationTokenTest.php | 67 +++++++++++-- .../ParameterConfirmationTokenTest_Token.php | 5 + 4 files changed, 158 insertions(+), 23 deletions(-) diff --git a/src/Core/Startup/ErrorControlChainMiddleware.php b/src/Core/Startup/ErrorControlChainMiddleware.php index a272cc548..6c782e135 100644 --- a/src/Core/Startup/ErrorControlChainMiddleware.php +++ b/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; diff --git a/src/Core/Startup/ParameterConfirmationToken.php b/src/Core/Startup/ParameterConfirmationToken.php index 7a887839c..f13589a42 100644 --- a/src/Core/Startup/ParameterConfirmationToken.php +++ b/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 - $params = array_merge($this->request->getVars(), $this->params()); - $location = Controller::join_links( - $this->currentURL(), - '?'.http_build_query($params) - ); + $location = $this->redirectURL(); $locationJS = Convert::raw2js($location); $locationATT = Convert::raw2att($location); $body = <<reloadRequired()) { + if ($token->reloadRequired() || $token->reloadRequiredIfError()) { $token->suppress(); $target = $token; } diff --git a/tests/php/Core/Startup/ParameterConfirmationTokenTest.php b/tests/php/Core/Startup/ParameterConfirmationTokenTest.php index 153d8459c..ff9cf2ff7 100644 --- a/tests/php/Core/Startup/ParameterConfirmationTokenTest.php +++ b/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 ); - - foreach (array('', '/', 'bar', 'bar/', '/bar', '/bar/') as $url) { - $this->request->setUrl($url); - $expected = rtrim(Controller::join_links(BASE_URL, '/', $url), '/') ?: '/'; - $this->assertEquals($expected, $token->currentURL(), "Invalid redirect for request url $url"); - } + $expected = rtrim(Controller::join_links(BASE_URL, '/', $url), '/') ?: '/'; + $this->assertEquals($expected, $token->currentURL(), "Invalid redirect for request url $url"); } } diff --git a/tests/php/Core/Startup/ParameterConfirmationTokenTest/ParameterConfirmationTokenTest_Token.php b/tests/php/Core/Startup/ParameterConfirmationTokenTest/ParameterConfirmationTokenTest_Token.php index 2f550548e..0ff14947c 100644 --- a/tests/php/Core/Startup/ParameterConfirmationTokenTest/ParameterConfirmationTokenTest_Token.php +++ b/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(); + } }