BUG Fix handling of empty parameter token

core/startup/ParameterConfirmationToken.php

@@ -11,14 +11,37 @@
  * It will likely be heavily refactored before the release of 3.2
  */
 class ParameterConfirmationToken {
+
+	/**
+	 * The name of the parameter
+	 *
+	 * @var string
+	 */
 	protected $parameterName = null;
+
+	/**
+	 * The parameter given
+	 *
+	 * @var string|null The string value, or null if not provided
+	 */
 	protected $parameter = null;
+
+	/**
+	 * The validated and checked token for this parameter
+	 *
+	 * @var string|null A string value, or null if either not provided or invalid
+	 */
 	protected $token = null;
 
 	protected function pathForToken($token) {
 		return TEMP_FOLDER.'/token_'.preg_replace('/[^a-z0-9]+/', '', $token);
 	}
 
+	/**
+	 * Generate a new random token and store it
+	 *
+	 * @return string Token name
+	 */
 	protected function genToken() {
 		// Generate a new random token (as random as possible)
 		require_once(dirname(dirname(dirname(__FILE__))).'/security/RandomGenerator.php');
@@ -31,7 +54,17 @@ protected function genToken() {
 		return $token;
 	}
 
+	/**
+	 * Validate a token
+	 *
+	 * @param string $token
+	 * @return boolean True if the token is valid
+	 */
 	protected function checkToken($token) {
+		if(!$token) {
+			return false;
+		}
+
 		$file = $this->pathForToken($token);
 		$content = null;
 
@@ -43,26 +76,75 @@ protected function checkToken($token) {
 		return $content == $token;
 	}
 
+	/**
+	 * Create a new ParameterConfirmationToken
+	 *
+	 * @param string $parameterName Name of the querystring parameter to check
+	 */
 	public function __construct($parameterName) {
 		// Store the parameter name
 		$this->parameterName = $parameterName;
+
 		// Store the parameter value
 		$this->parameter = isset($_GET[$parameterName]) ? $_GET[$parameterName] : null;
-		// Store the token
-		$this->token = isset($_GET[$parameterName.'token']) ? $_GET[$parameterName.'token'] : null;
 
-		// If a token was provided, but isn't valid, ignore it
-		if ($this->token && (!$this->checkToken($this->token))) $this->token = null;
+		// If the token provided is valid, mark it as such
+		$token = isset($_GET[$parameterName.'token']) ? $_GET[$parameterName.'token'] : null;
+		if ($this->checkToken($token)) {
+			$this->token = $token;
+		}
 	}
 
+	/**
+	 * Get the name of this token
+	 * 
+	 * @return string
+	 */
+	public function getName() {
+		return $this->parameterName;
+	}
+
+	/**
+	 * Is the parameter requested?
+	 * ?parameter and ?parameter=1 are both considered requested
+	 * 
+	 * @return bool
+	 */
 	public function parameterProvided() {
 		return $this->parameter !== null;
 	}
 
+	/**
+	 * Is the necessary token provided for this parameter?
+	 * A value must be provided for the token
+	 * 
+	 * @return bool
+	 */
 	public function tokenProvided() {
-		return $this->token !== null;
+		return !empty($this->token);
+	}
+
+	/**
+	 * Is this parameter requested without a valid token?
+	 * 
+	 * @return bool True if the parameter is given without a valid token
+	 */
+	public function reloadRequired() {
+		return $this->parameterProvided() && !$this->tokenProvided();
+	}
+
+	/**
+	 * Suppress the current parameter by unsetting it from $_GET
+	 */
+	public function suppress() {
+		unset($_GET[$this->parameterName]);
 	}
 
+	/**
+	 * Determine the querystring parameters to include
+	 *
+	 * @return array List of querystring parameters with name and token parameters
+	 */
 	public function params() {
 		return array(
 			$this->parameterName => $this->parameter,
@@ -107,6 +189,10 @@ protected function currentAbsoluteURL() {
 		return "$proto://" . preg_replace('#/{2,}#', '/', implode('/', $parts));
 	}
 
+	/**
+	 * Forces a reload of the request with the token included
+	 * This method will terminate the script with `die`
+	 */
 	public function reloadWithToken() {
 		$location = $this->currentAbsoluteURL();
 

main.php

@@ -103,12 +103,11 @@
 $token = new ParameterConfirmationToken('flush');
 
 $chain
-	->then(function($chain) use ($token){
+	->then(function($chain) use ($token) {
 		// First, if $_GET['flush'] was set, but no valid token, suppress the flush
-		if (isset($_GET['flush']) && !$token->tokenProvided()) {
-			unset($_GET['flush']);
-		}
-		else {
+		if ($token->reloadRequired()) {
+			$token->suppress();
+		} else {
 			$chain->setSuppression(false);
 		}
 

tests/core/startup/ParameterConfirmationTokenTest.php

@@ -1,11 +1,24 @@
 <?php
 
-class ParameterConfirmationTokenTest_Token extends ParameterConfirmationToken {
+/**
+ * Dummy parameter token
+ */
+class ParameterConfirmationTokenTest_Token extends ParameterConfirmationToken implements TestOnly {
 
 	public function currentAbsoluteURL() {
 		return parent::currentAbsoluteURL();
 	}
+}
+
 
+/**
+ * A token that always validates a given token
+ */
+class ParameterConfirmationTokenTest_ValidToken extends ParameterConfirmationTokenTest_Token {
+
+	protected function checkToken($token) {
+		return true;
+	}
 }
 
 class ParameterConfirmationTokenTest extends SapphireTest {
@@ -18,6 +31,64 @@ private function addPart($answer, $slash, $part) {
 
 		return array($answer, $slash);
 	}
+
+	public function setUp() {
+		parent::setUp();
+		$_GET['parameterconfirmationtokentest_notoken'] = 'value';
+		$_GET['parameterconfirmationtokentest_empty'] = '';
+		$_GET['parameterconfirmationtokentest_withtoken'] = '1';
+		$_GET['parameterconfirmationtokentest_withtokentoken'] = 'dummy';
+		$_GET['parameterconfirmationtokentest_nulltoken'] = '1';
+		$_GET['parameterconfirmationtokentest_nulltokentoken'] = null;
+		$_GET['parameterconfirmationtokentest_emptytoken'] = '1';
+		$_GET['parameterconfirmationtokentest_emptytokentoken'] = '';
+	}
+
+	public function tearDown() {
+		foreach($_GET as $param) {
+			if(stripos($param, 'parameterconfirmationtokentest_') === 0) unset($_GET[$param]);
+		}
+		parent::tearDown();
+	}
+
+	public function testParameterDetectsParameters() {
+		$withoutToken = new ParameterConfirmationTokenTest_Token('parameterconfirmationtokentest_notoken');
+		$emptyParameter = new ParameterConfirmationTokenTest_Token('parameterconfirmationtokentest_empty');
+		$withToken = new ParameterConfirmationTokenTest_ValidToken('parameterconfirmationtokentest_withtoken');
+		$withoutParameter = new ParameterConfirmationTokenTest_Token('parameterconfirmationtokentest_noparam');
+		$nullToken = new ParameterConfirmationTokenTest_Token('parameterconfirmationtokentest_nulltoken');
+		$emptyToken = new ParameterConfirmationTokenTest_Token('parameterconfirmationtokentest_emptytoken');
+
+		// Check parameter
+		$this->assertTrue($withoutToken->parameterProvided());
+		$this->assertTrue($emptyParameter->parameterProvided());  // even if empty, it's still provided
+		$this->assertTrue($withToken->parameterProvided());
+		$this->assertFalse($withoutParameter->parameterProvided());
+		$this->assertTrue($nullToken->parameterProvided());
+		$this->assertTrue($emptyToken->parameterProvided());
+
+		// Check token
+		$this->assertFalse($withoutToken->tokenProvided());
+		$this->assertFalse($emptyParameter->tokenProvided());
+		$this->assertTrue($withToken->tokenProvided()); // Actually forced to true for this test
+		$this->assertFalse($withoutParameter->tokenProvided());
+		$this->assertFalse($nullToken->tokenProvided());
+		$this->assertFalse($emptyToken->tokenProvided());
+
+		// Check if reload is required
+		$this->assertTrue($withoutToken->reloadRequired());
+		$this->assertTrue($emptyParameter->reloadRequired());
+		$this->assertFalse($withToken->reloadRequired());
+		$this->assertFalse($withoutParameter->reloadRequired());
+		$this->assertTrue($nullToken->reloadRequired());
+		$this->assertTrue($emptyToken->reloadRequired());
+
+		// Check suppression
+		$this->assertTrue(isset($_GET['parameterconfirmationtokentest_notoken']));
+		$withoutToken->suppress();
+		$this->assertFalse(isset($_GET['parameterconfirmationtokentest_notoken']));
+	}
+
 
 	/**
 	 * currentAbsoluteURL needs to handle base or url being missing, or any combination of slashes.