Skip to content

Commit

Permalink
code improvements fixing PR comments from @markstory
Browse files Browse the repository at this point in the history 
  * fix sprintf not required and extra exception when unexpected debug token found
  * sort unlocked fields
  * fix docblocks and sprintf usage
  * ensure key 0 exists in array
  * throw exception on _validatePost instead of return false
  * fix no need to restore the debug value
  * test what if the tampered field is mutated into an array, improve expected fields output to display correctly the name of the field
  • Loading branch information
@steinkel
steinkel committed Mar 14, 2016
1 parent 9b8e149 commit 1bf0d82
Show file tree
Hide file tree
Showing 2 changed files with 140 additions and 46 deletions.
148 changes: 108 additions & 40 deletions src/Controller/Component/SecurityComponent.php
View file
Expand Up @@ -39,6 +39,11 @@
class SecurityComponent extends Component
{

/**
* Default message used for exceptions thrown
*/
const DEFAULT_EXCEPTION_MESSAGE = 'The request has been black-holed';

/**
* Default config
*
Expand Down Expand Up @@ -119,11 +124,10 @@ public function startup(Event $event)
}

if (!in_array($this->_action, (array)$this->_config['unlockedActions']) &&
$hasData && $isNotRequestAction
) {
if ($this->_config['validatePost']) {
$hasData &&
$isNotRequestAction &&
$this->_config['validatePost']) {
$this->_validatePost($controller);
}
}
} catch (SecurityException $se) {
$this->blackHole($controller, $se->getType(), $se);
Expand Down Expand Up @@ -203,15 +207,14 @@ public function blackHole(Controller $controller, $error = '', SecurityException
*/
protected function _throwException($exception = null)
{
$defaultMessage = 'The request has been black-holed';
if ($exception !== null) {
if (!Configure::read('debug')) {
if (!Configure::read('debug') && $exception instanceof SecurityException) {
$exception->setReason($exception->getMessage());
$exception->setMessage($defaultMessage);
$exception->setMessage(self::DEFAULT_EXCEPTION_MESSAGE);
}
throw $exception;
}
throw new BadRequestException($defaultMessage);
throw new BadRequestException(self::DEFAULT_EXCEPTION_MESSAGE);
}

/**
Expand Down Expand Up @@ -270,7 +273,7 @@ protected function _authRequired(Controller $controller)

if (in_array($this->request->params['action'], $requireAuth) || $requireAuth == ['*']) {
if (!isset($controller->request->data['_Token'])) {
throw new AuthSecurityException(sprintf('\'%s\' was not found in request data.', '_Token'));
throw new AuthSecurityException('\'_Token\' was not found in request data.');
}

if ($this->session->check('_Token')) {
Expand Down Expand Up @@ -299,7 +302,7 @@ protected function _authRequired(Controller $controller)
);
}
} else {
throw new AuthSecurityException(sprintf('\'%s\' was not found in session.', '_Token'));
throw new AuthSecurityException('\'_Token\' was not found in session.');
}
}
}
Expand All @@ -326,12 +329,12 @@ protected function _validatePost(Controller $controller)
return true;
}

$msg = self::DEFAULT_EXCEPTION_MESSAGE;
if (Configure::read('debug')) {
$msg = $this->_debugPostTokenNotMatching($controller, $hashParts);
throw new SecurityException($msg);
}

return false;
throw new SecurityException($msg);
}

/**
Expand All @@ -358,10 +361,13 @@ protected function _validToken(Controller $controller)
if (Configure::read('debug') && !isset($check['_Token']['debug'])) {
throw new SecurityException(sprintf($message, '_Token.debug'));
}
if (!Configure::read('debug') && isset($check['_Token']['debug'])) {
throw new SecurityException('Unexpected \'_Token.debug\' found in request data');
}

$token = urldecode($check['_Token']['fields']);
if (strpos($token, ':')) {
list($token, $locked) = explode(':', $token, 2);
list($token, ) = explode(':', $token, 2);
}
return $token;
}
Expand All @@ -375,7 +381,8 @@ protected function _validToken(Controller $controller)
protected function _hashParts(Controller $controller)
{
$fieldList = $this->_fieldsList($controller->request->data);
$unlocked = $this->_unlocked($controller->request->data);
$unlocked = $this->_sortedUnlocked($controller->request->data);

return [
$controller->request->here(),
serialize($fieldList),
Expand Down Expand Up @@ -456,20 +463,34 @@ protected function _fieldsList(array $check)
/**
* Get the unlocked string
*
* @param array $check Data array
* @param array $data Data array
* @return string
*/
protected function _unlocked(array $data)
{
return urldecode($data['_Token']['unlocked']);
}

/**
* Get the sorted unlocked string
*
* @param array $data Data array
* @return string
*/
protected function _unlocked(array $check)
protected function _sortedUnlocked($data)
{
return urldecode($check['_Token']['unlocked']);
$unlocked = $this->_unlocked($data);
$unlocked = explode('|', $unlocked);
sort($unlocked, SORT_STRING);
return implode('|', $unlocked);
}

/**
* Create a message for humans to understand why Security token is not matching
*
* @param \Cake\Controller\Controller $controller Instantiating controller
* @param array $hashParts Elements used to generate the Token hash
* @return array Messages to explain why token is not matching
* @return string Message explaining why the tokens are not matching
*/
protected function _debugPostTokenNotMatching(Controller $controller, $hashParts)
{
Expand All @@ -478,11 +499,16 @@ protected function _debugPostTokenNotMatching(Controller $controller, $hashParts
if (!is_array($expectedParts) || count($expectedParts) !== 3) {
return 'Invalid security debug token.';
}
if ($hashParts[0] !== $expectedParts[0]) {
$messages[] = sprintf('URL mismatch in POST data (expected \'%s\' but found \'%s\')', $expectedParts[0], $hashParts[0]);
$expectedUrl = Hash::get($expectedParts, 0);
$url = Hash::get($hashParts, 0);
if ($expectedUrl !== $url) {
$messages[] = sprintf('URL mismatch in POST data (expected \'%s\' but found \'%s\')', $expectedUrl, $url);
}
$expectedFields = Hash::get($expectedParts, 1);
$dataFields = Hash::get($hashParts, 1);
if ($dataFields) {
$dataFields = unserialize($dataFields);
}
$expectedFields = $expectedParts[1];
$dataFields = unserialize($hashParts[1]);
$fieldsMessages = $this->_debugCheckFields(
$dataFields,
$expectedFields,
Expand Down Expand Up @@ -519,24 +545,10 @@ protected function _debugPostTokenNotMatching(Controller $controller, $hashParts
*/
protected function _debugCheckFields($dataFields, $expectedFields = [], $intKeyMessage = '', $stringKeyMessage = '', $missingMessage = '')
{
$messages = [];
foreach ($dataFields as $key => $value) {
if (is_int($key)) {
$foundKey = array_search($value, (array)$expectedFields);
if ($foundKey === false) {
$messages[] = sprintf($intKeyMessage, $value);
} else {
unset($expectedFields[$foundKey]);
}
} elseif (is_string($key)) {
if (isset($expectedFields[$key]) && $value !== $expectedFields[$key]) {
$messages[] = sprintf($stringKeyMessage, $key, $expectedFields[$key], $value);
}
unset($expectedFields[$key]);
}
}
if (count($expectedFields) > 0) {
$messages[] = sprintf($missingMessage, implode(', ', $expectedFields));
$messages = $this->_matchExistingFields($dataFields, $expectedFields, $intKeyMessage, $stringKeyMessage);
$expectedFieldsMessage = $this->_debugExpectedFields($expectedFields, $missingMessage);
if ($expectedFieldsMessage !== null) {
$messages[] = $expectedFieldsMessage;
}
return $messages;
}
Expand Down Expand Up @@ -585,4 +597,60 @@ protected function _callback(Controller $controller, $method, $params = [])
}
return call_user_func_array([&$controller, $method], empty($params) ? null : $params);
}

/**
* Generate array of messages for the existing fields in POST data, matching dataFields in $expectedFields
* will be unset
*
* @param array $dataFields Fields array, containing the POST data fields
* @param array $expectedFields Fields array, containing the expected fields we should have in POST
* @param string $intKeyMessage Message string if unexpected found in data fields indexed by int (not protected)
* @param string $stringKeyMessage Message string if tampered found in data fields indexed by string (protected)
* @return array Error messages
*/
protected function _matchExistingFields($dataFields, &$expectedFields, $intKeyMessage, $stringKeyMessage)
{
$messages = [];
foreach ((array)$dataFields as $key => $value) {
if (is_int($key)) {
$foundKey = array_search($value, (array)$expectedFields);
if ($foundKey === false) {
$messages[] = sprintf($intKeyMessage, $value);
} else {
unset($expectedFields[$foundKey]);
}
} elseif (is_string($key)) {
if (isset($expectedFields[$key]) && $value !== $expectedFields[$key]) {
$messages[] = sprintf($stringKeyMessage, $key, $expectedFields[$key], $value);
}
unset($expectedFields[$key]);
}
}

return $messages;
}

/**
* Generate debug message for the expected fields
*
* @param array $expectedFields Expected fields
* @param string $missingMessage Message template
* @return string Error message about expected fields
*/
protected function _debugExpectedFields($expectedFields = [], $missingMessage = '')
{
if (count($expectedFields) === 0) {
return null;
}

$expectedFieldNames = [];
foreach ((array)$expectedFields as $key => $expectedField) {
if (is_int($key)) {
$expectedFieldNames[] = $expectedField;
} else {
$expectedFieldNames[] = $key;
}
}
return sprintf($missingMessage, implode(', ', $expectedFieldNames));
}
}
38 changes: 32 additions & 6 deletions tests/TestCase/Controller/Component/SecurityComponentTest.php
View file
Expand Up @@ -959,11 +959,8 @@ public function testValidatePostFailNoDebugMode()
],
'_Token' => compact('fields', 'unlocked')
];
$debug = Configure::read('debug');
Configure::write('debug', false);
$result = $this->validatePost();
$this->assertFalse($result);
Configure::write('debug', $debug);
$result = $this->validatePost('SecurityException', 'The request has been black-holed');
}

/**
Expand Down Expand Up @@ -1425,7 +1422,6 @@ public function testBlackholeThrowsException()
public function testBlackholeThrowsBadRequest()
{
$this->Security->config('blackHoleCallback', '');
$debug = Configure::read('debug');
$message = '';

Configure::write('debug', false);
Expand All @@ -1435,7 +1431,6 @@ public function testBlackholeThrowsBadRequest()
$message = $ex->getMessage();
$reason = $ex->getReason();
}
Configure::write('debug', $debug);
$this->assertEquals('The request has been black-holed', $message);
$this->assertEquals('error description', $reason);
}
Expand Down Expand Up @@ -1471,6 +1466,37 @@ public function testValidatePostFailTampering()
$this->assertFalse($result);
}

/**
* Test that validatePost fails with tampered fields and explanation
*
* @return void
* @triggers Controller.startup $this->Controller
*/
public function testValidatePostFailTamperingMutatedIntoArray()
{
$event = new Event('Controller.startup', $this->Controller);
$this->Controller->Security->startup($event);
$unlocked = '';
$fields = ['Model.hidden' => 'value', 'Model.id' => '1'];
$debug = urlencode(json_encode([
'/articles/index',
$fields,
[]
]));
$fields = urlencode(Security::hash(serialize($fields) . $unlocked . Security::salt()));
$fields .= urlencode(':Model.hidden|Model.id');
$this->Controller->request->data = [
'Model' => [
'hidden' => ['some-key' => 'some-value'],
'id' => '1',
],
'_Token' => compact('fields', 'unlocked', 'debug')
];

$result = $this->validatePost('SecurityException', 'Unexpected field \'Model.hidden.some-key\' in POST data, Missing field \'Model.hidden\' in POST data');
$this->assertFalse($result);
}

/**
* Auth required throws exception token not found
*
Expand Down

0 comments on commit 1bf0d82

Please sign in to comment.