Removing old CSRF token validation checks.

Removing failing test because the feature moved. Adding tests for expired and wrong keys.
cakephp · Oct 2, 2010 · 7f7c202 · 7f7c202
1 parent ac90916
commit 7f7c202
Show file tree

Hide file tree

Showing 2 changed files with 65 additions and 31 deletions.
diff --git a/cake/libs/controller/components/security.php b/cake/libs/controller/components/security.php
@@ -595,18 +595,9 @@ protected function _validatePost(&$controller) {
 		}
 		$data = $controller->request->data;
 
-		if (!isset($data['_Token']) || !isset($data['_Token']['fields']) || !isset($data['_Token']['key'])) {
+		if (!isset($data['_Token']) || !isset($data['_Token']['fields'])) {
 			return false;
 		}
-		$token = $data['_Token']['key'];
-
-		if ($this->Session->check('_Token')) {
-			$tokenData = $this->Session->read('_Token');
-
-			if ($tokenData['expires'] < time() || $tokenData['key'] !== $token) {
-				return false;
-			}
-		}
 
 		$locked = null;
 		$check = $controller->request->data;
@@ -678,10 +669,8 @@ protected function _generateToken(&$controller) {
 			return false;
 		}
 		$authKey = Security::generateAuthKey();
-		$expires = strtotime('+' . Security::inactiveMins() . ' minutes');
 		$token = array(
 			'key' => $authKey,
-			'expires' => $expires,
 			'allowedControllers' => $this->allowedControllers,
 			'allowedActions' => $this->allowedActions,
 			'disabledFields' => $this->disabledFields,
@@ -694,15 +683,6 @@ protected function _generateToken(&$controller) {
 
 		if ($this->Session->check('_Token')) {
 			$tokenData = $this->Session->read('_Token');
-			$valid = (
-				isset($tokenData['expires']) &&
-				$tokenData['expires'] > time() &&
-				isset($tokenData['key'])
-			);
-
-			if ($valid) {
-				$token['key'] = $tokenData['key'];
-			}
 			if (!empty($tokenData['csrfTokens'])) {
 				$token['csrfTokens'] += $tokenData['csrfTokens'];
 				$token['csrfTokens'] = $this->_expireTokens($token['csrfTokens']);
@@ -723,8 +703,8 @@ protected function _generateToken(&$controller) {
  */
 	protected function _validateCsrf($controller) {
 		$token = $this->Session->read('_Token');
-		$requestToken = $controller->request->data('_Token.nonce');
-		if (isset($token['csrfTokens'][$requestToken])) {
+		$requestToken = $controller->request->data('_Token.key');
+		if (isset($token['csrfTokens'][$requestToken]) && $token['csrfTokens'][$requestToken] >= time()) {
 			$this->Session->delete('_Token.csrfTokens.' . $requestToken);
 			return true;
 		}

diff --git a/cake/tests/cases/libs/controller/components/security.test.php b/cake/tests/cases/libs/controller/components/security.test.php
@@ -606,14 +606,8 @@ function testValidatePostFormHacking() {
 		);
 		$result = $this->Controller->Security->validatePost($this->Controller);
 		$this->assertFalse($result, 'validatePost passed when fields were missing. %s');
-
-		$this->Controller->request->data = array(
-			'Model' => array('username' => 'nate', 'password' => 'foo', 'valid' => '0'),
-			'_Token' => compact('fields')
-		);
-		$result = $this->Controller->Security->validatePost($this->Controller);
-		$this->assertFalse($result, 'validatePost passed when key was missing. %s');
 	}
+
 /**
  * Tests validation of checkbox arrays
  *
@@ -1286,7 +1280,7 @@ function testCsrfNonceConsumption() {
 		$this->Controller->request->params['action'] = 'index';
 		$this->Controller->request->data = array(
 			'_Token' => array(
-				'nonce' => 'nonce1'
+				'key' => 'nonce1'
 			),
 			'Post' => array(
 				'title' => 'Woot'
@@ -1315,4 +1309,64 @@ function testCsrfNonceVacuum() {
 		$tokens = $this->Security->Session->read('_Token.csrfTokens');
 		$this->assertEquals(1, count($tokens), 'Too many tokens left behind');
 	}
+
+/**
+ * test that when the key is missing the request is blackHoled
+ *
+ * @return void
+ */
+	function testCsrfBlackHoleOnKeyMismatch() {
+		$this->Security->validatePost = false;
+		$this->Security->csrfCheck = true;
+		$this->Security->csrfExpires = '+10 minutes';
+
+		$this->Security->Session->write('_Token.csrfTokens', array('nonce1' => strtotime('+10 minutes')));
+
+		$this->Controller->request = $this->getMock('CakeRequest', array('is'));
+		$this->Controller->request->expects($this->once())->method('is')
+			->with('post')
+			->will($this->returnValue(true));
+
+		$this->Controller->request->params['action'] = 'index';
+		$this->Controller->request->data = array(
+			'_Token' => array(
+				'key' => 'not the right value'
+			),
+			'Post' => array(
+				'title' => 'Woot'
+			)
+		);
+		$this->Security->startup($this->Controller);
+		$this->assertTrue($this->Controller->failed, 'fail() was not called.');
+	}
+
+/**
+ * test that when the key is missing the request is blackHoled
+ *
+ * @return void
+ */
+	function testCsrfBlackHoleOnExpiredKey() {
+		$this->Security->validatePost = false;
+		$this->Security->csrfCheck = true;
+		$this->Security->csrfExpires = '+10 minutes';
+
+		$this->Security->Session->write('_Token.csrfTokens', array('nonce1' => strtotime('-5 minutes')));
+
+		$this->Controller->request = $this->getMock('CakeRequest', array('is'));
+		$this->Controller->request->expects($this->once())->method('is')
+			->with('post')
+			->will($this->returnValue(true));
+
+		$this->Controller->request->params['action'] = 'index';
+		$this->Controller->request->data = array(
+			'_Token' => array(
+				'key' => 'nonce1'
+			),
+			'Post' => array(
+				'title' => 'Woot'
+			)
+		);
+		$this->Security->startup($this->Controller);
+		$this->assertTrue($this->Controller->failed, 'fail() was not called.');
+	}
 }