Skip to content

Commit

Permalink
Validate POST data whenever request->data is not empty.
Browse files Browse the repository at this point in the history
If the request manages to have data set outside of post/put we should
still validate the request body. This expands SecurityComponent to cover
PATCH and DELETE methods, as well as request methods that should be
safe, but somehow end up not safe.
  • Loading branch information
markstory committed Jan 20, 2016
1 parent 3cf8c6a commit 0abb790
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 5 deletions.
6 changes: 3 additions & 3 deletions src/Controller/Component/SecurityComponent.php
Expand Up @@ -104,7 +104,7 @@ public function startup(Event $event)
$this->_secureRequired($controller);
$this->_authRequired($controller);

$isPost = $this->request->is(['post', 'put']);
$hasData = !empty($this->request->data);

This comment has been minimized.

Copy link
@QuanticPotato

QuanticPotato Feb 19, 2016

Hi,
Souldn't you take the "disableFields" configuration into account here ?

This comment has been minimized.

Copy link
@markstory

markstory Feb 19, 2016

Author Member

Why?

This comment has been minimized.

Copy link
@QuanticPotato

QuanticPotato Feb 20, 2016

I just realized that until 3.2 I was using $request->data before the SecurityComponent startup (ie I added variables).
It didn't throw any security exception, because variables I added were set in the "disabledFields" configuration.
But with this version, it don't check disabledFields, so it calls the blackhole.

Nevermind, it was probably not a good logic. I changed my app so it works with this new stuff ;)

$isNotRequestAction = (
!isset($controller->request->params['requested']) ||
$controller->request->params['requested'] != 1
Expand All @@ -115,7 +115,7 @@ public function startup(Event $event)
}

if (!in_array($this->_action, (array)$this->_config['unlockedActions']) &&
$isPost && $isNotRequestAction
$hasData && $isNotRequestAction
) {
if ($this->_config['validatePost'] &&
$this->_validatePost($controller) === false
Expand All @@ -124,7 +124,7 @@ public function startup(Event $event)
}
}
$this->generateToken($controller->request);
if ($isPost && is_array($controller->request->data)) {
if ($hasData && is_array($controller->request->data)) {
unset($controller->request->data['_Token']);
}
}
Expand Down
33 changes: 31 additions & 2 deletions tests/TestCase/Controller/Component/SecurityComponentTest.php
Expand Up @@ -350,20 +350,25 @@ public function testRequireAuthFail()
public function testRequireAuthSucceed()
{
$_SERVER['REQUEST_METHOD'] = 'AUTH';
$this->Controller->Security->config('validatePost', false);

$event = new Event('Controller.startup', $this->Controller);
$this->Controller->request['action'] = 'posted';
$this->Controller->Security->requireAuth('posted');
$this->Controller->Security->startup($event);
$this->assertFalse($this->Controller->failed);

$this->Controller->Security->session->write('_Token', [
'allowedControllers' => ['SecurityTest'], 'allowedActions' => ['posted']
'allowedControllers' => ['SecurityTest'],
'allowedActions' => ['posted'],
]);
$this->Controller->request['controller'] = 'SecurityTest';
$this->Controller->request['action'] = 'posted';

$this->Controller->request->data = [
'username' => 'willy', 'password' => 'somePass', '_Token' => ''
'username' => 'willy',
'password' => 'somePass',
'_Token' => ''
];
$this->Controller->action = 'posted';
$this->Controller->Security->requireAuth('posted');
Expand Down Expand Up @@ -392,6 +397,30 @@ public function testValidatePost()
$this->assertTrue($this->Controller->Security->validatePost($this->Controller));
}

/**
* Test that validatePost fires on GET with request data.
* This could happen when method overriding is used.
*
* @return void
* @triggers Controller.startup $this->Controller
*/
public function testValidatePostOnGetWithData()
{
$event = new Event('Controller.startup', $this->Controller);
$this->Controller->Security->startup($event);

$fields = '68730b0747d4889ec2766f9117405f9635f5fd5e%3AModel.valid';
$unlocked = '';

$this->Controller->request->env('REQUEST_METHOD', 'GET');
$this->Controller->request->data = [
'Model' => ['username' => 'nate', 'password' => 'foo', 'valid' => '0'],
'_Token' => compact('fields', 'unlocked')
];
$this->Controller->Security->startup($event);
$this->assertFalse($this->Controller->failed);
}

/**
* Test that validatePost fails if you are missing the session information.
*
Expand Down

0 comments on commit 0abb790

Please sign in to comment.