Browse Source

Validate POST data whenever request->data is not empty.

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.
Mark Story 10 years ago
parent
commit
0abb7901c4

+ 3 - 3
src/Controller/Component/SecurityComponent.php

@@ -104,7 +104,7 @@ class SecurityComponent extends Component
         $this->_secureRequired($controller);
         $this->_authRequired($controller);
 
-        $isPost = $this->request->is(['post', 'put']);
+        $hasData = !empty($this->request->data);
         $isNotRequestAction = (
             !isset($controller->request->params['requested']) ||
             $controller->request->params['requested'] != 1
@@ -115,7 +115,7 @@ class SecurityComponent extends Component
         }
 
         if (!in_array($this->_action, (array)$this->_config['unlockedActions']) &&
-            $isPost && $isNotRequestAction
+            $hasData && $isNotRequestAction
         ) {
             if ($this->_config['validatePost'] &&
                 $this->_validatePost($controller) === false
@@ -124,7 +124,7 @@ class SecurityComponent extends Component
             }
         }
         $this->generateToken($controller->request);
-        if ($isPost && is_array($controller->request->data)) {
+        if ($hasData && is_array($controller->request->data)) {
             unset($controller->request->data['_Token']);
         }
     }

+ 31 - 2
tests/TestCase/Controller/Component/SecurityComponentTest.php

@@ -350,6 +350,8 @@ class SecurityComponentTest extends TestCase
     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');
@@ -357,13 +359,16 @@ class SecurityComponentTest extends TestCase
         $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');
@@ -393,6 +398,30 @@ class SecurityComponentTest extends TestCase
     }
 
     /**
+     * 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.
      *
      * @return void