diff --git a/core/control/RequestHandler.php b/core/control/RequestHandler.php index 535dfe731..c8af381c2 100755 --- a/core/control/RequestHandler.php +++ b/core/control/RequestHandler.php @@ -71,6 +71,12 @@ class RequestHandler extends ViewableData { * 'complexaction' '->canComplexAction' // complexaction can only be accessed if $this->canComplexAction() returns true * ); * + * + * Form getters count as URL actions as well, and should be included in allowed_actions. + * Form actions on the other handed (first argument to {@link FormAction()} shoudl NOT be included, + * these are handled separately through {@link Form->httpSubmission}. You can control access on form actions + * either by conditionally removing {@link FormAction} in the form construction, + * or by defining $allowed_actions in your {@link Form} class. */ static $allowed_actions = null; diff --git a/forms/Form.php b/forms/Form.php index e9d760871..1ed29a32c 100755 --- a/forms/Form.php +++ b/forms/Form.php @@ -264,6 +264,31 @@ class Form extends RequestHandler { $this->setButtonClicked($funcName); } + // Permission checks (first on controller, then falling back to form) + if( + // Ensure that the action is actually a button or method on the form, + // and not just a method on the controller. + $this->controller->hasMethod($funcName) + && !$this->controller->checkAccessAction($funcName) + // If a button exists, allow it on the controller + && !$this->Actions()->fieldByName('action_' . $funcName) + ) { + return $this->httpError( + 403, + sprintf('Action "%s" not allowed on controller (Class: %s)', $funcName, get_class($this->controller)) + ); + } elseif( + $this->hasMethod($funcName) + && !$this->checkAccessAction($funcName) + // No checks for button existence or $allowed_actions is performed - + // all form methods are callable (e.g. the legacy "callfieldmethod()") + ) { + return $this->httpError( + 403, + sprintf('Action "%s" not allowed on form (Name: "%s")', $funcName, $this->Name()) + ); + } + // Validate the form if(!$this->validate()) { if(Director::is_ajax()) { @@ -299,16 +324,15 @@ class Form extends RequestHandler { } } - // First, try a handler method on the controller + // First, try a handler method on the controller (has been checked for allowed_actions above already) if($this->controller->hasMethod($funcName)) { return $this->controller->$funcName($vars, $this, $request); - - // Otherwise, try a handler method on the form object - } else { - if($this->hasMethod($funcName)) { - return $this->$funcName($vars, $this, $request); - } + // Otherwise, try a handler method on the form object. + } elseif($this->hasMethod($funcName)) { + return $this->$funcName($vars, $this, $request); } + + return $this->httpError(404); } /** diff --git a/tests/RequestHandlingTest.php b/tests/RequestHandlingTest.php index 28a4b5300..027d2d8b1 100755 --- a/tests/RequestHandlingTest.php +++ b/tests/RequestHandlingTest.php @@ -4,7 +4,7 @@ * Tests for RequestHandler and SS_HTTPRequest. * We've set up a simple URL handling model based on */ -class RequestHandlingTest extends SapphireTest { +class RequestHandlingTest extends FunctionalTest { static $fixture_file = null; // function testRequestHandlerChainingLatestParams() { @@ -149,6 +149,91 @@ class RequestHandlingTest extends SapphireTest { $this->assertEquals(403, $response->getStatusCode()); } + function testFormActionsCanBypassAllowedActions() { + SecurityToken::enable(); + + $response = $this->get('RequestHandlingTest_FormActionController'); + $this->assertEquals(200, $response->getStatusCode()); + $tokenEls = $this->cssParser()->getBySelector('#Form_Form_SecurityID'); + $securityId = (string)$tokenEls[0]['value']; + + $data = array('action_formaction' => 1); + $response = $this->post('RequestHandlingTest_FormActionController/Form', $data); + $this->assertEquals(400, $response->getStatusCode(), + 'Should fail: Invocation through POST form handler, not contained in $allowed_actions, without CSRF token' + ); + + $data = array('action_disallowedcontrollermethod' => 1, 'SecurityID' => $securityId); + $response = $this->post('RequestHandlingTest_FormActionController/Form', $data); + $this->assertEquals(403, $response->getStatusCode(), + 'Should fail: Invocation through POST form handler, controller action instead of form action, not contained in $allowed_actions, with CSRF token' + ); + + $data = array('action_formaction' => 1, 'SecurityID' => $securityId); + $response = $this->post('RequestHandlingTest_FormActionController/Form', $data); + $this->assertEquals(200, $response->getStatusCode()); + $this->assertEquals('formaction', $response->getBody(), + 'Should pass: Invocation through POST form handler, not contained in $allowed_actions, with CSRF token' + ); + + $data = array('action_controlleraction' => 1, 'SecurityID' => $securityId); + $response = $this->post('RequestHandlingTest_FormActionController/Form', $data); + $this->assertEquals(200, $response->getStatusCode(), + 'Should pass: Invocation through POST form handler, controller action instead of form action, contained in $allowed_actions, with CSRF token' + ); + + $data = array('action_formactionInAllowedActions' => 1); + $response = $this->post('RequestHandlingTest_FormActionController/Form', $data); + $this->assertEquals(400, $response->getStatusCode(), + 'Should fail: Invocation through POST form handler, contained in $allowed_actions, without CSRF token' + ); + + $data = array('action_formactionInAllowedActions' => 1, 'SecurityID' => $securityId); + $response = $this->post('RequestHandlingTest_FormActionController/Form', $data); + $this->assertEquals(200, $response->getStatusCode(), + 'Should pass: Invocation through POST form handler, contained in $allowed_actions, with CSRF token' + ); + + $data = array(); + $response = $this->post('RequestHandlingTest_FormActionController/formaction', $data); + $this->assertEquals(404, $response->getStatusCode(), + 'Should fail: Invocation through POST URL, not contained in $allowed_actions, without CSRF token' + ); + + $data = array(); + $response = $this->post('RequestHandlingTest_FormActionController/formactionInAllowedActions', $data); + $this->assertEquals(200, $response->getStatusCode(), + 'Should pass: Invocation of form action through POST URL, contained in $allowed_actions, without CSRF token' + ); + + $data = array('SecurityID' => $securityId); + $response = $this->post('RequestHandlingTest_FormActionController/formactionInAllowedActions', $data); + $this->assertEquals(200, $response->getStatusCode(), + 'Should pass: Invocation of form action through POST URL, contained in $allowed_actions, with CSRF token' + ); + + $data = array(); // CSRF protection doesnt kick in for direct requests + $response = $this->post('RequestHandlingTest_FormActionController/formactionInAllowedActions', $data); + $this->assertEquals(200, $response->getStatusCode(), + 'Should pass: Invocation of form action through POST URL, contained in $allowed_actions, without CSRF token' + ); + + SecurityToken::disable(); + } + + function testAllowedActionsEnforcedOnForm() { + $data = array('action_allowedformaction' => 1); + $response = $this->post('RequestHandlingTest_ControllerFormWithAllowedActions/Form', $data); + $this->assertEquals(200, $response->getStatusCode()); + $this->assertEquals('allowedformaction', $response->getBody()); + + $data = array('action_disallowedformaction' => 1); + $response = $this->post('RequestHandlingTest_ControllerFormWithAllowedActions/Form', $data); + $this->assertEquals(403, $response->getStatusCode()); + // Note: Looks for a specific 403 thrown by Form->httpSubmission(), not RequestHandler->handleRequest() + $this->assertContains('not allowed on form', $response->getBody()); + } + } /** @@ -235,6 +320,57 @@ class RequestHandlingTest_Controller extends Controller { } +class RequestHandlingTest_FormActionController extends Controller { + + protected $template = 'BlankPage'; + + static $allowed_actions = array( + 'controlleraction', + 'Form', + 'formactionInAllowedActions' + //'formaction', // left out, implicitly allowed through form action + ); + + function Link($action = null) { + return Controller::join_links('RequestHandlingTest_FormActionController', $action); + } + + function controlleraction($request) { + return 'controlleraction'; + } + + function disallowedcontrollermethod() { + return 'disallowedcontrollermethod'; + } + + function Form() { + return new Form( + $this, + "Form", + new FieldSet( + new TextField("MyField") + ), + new FieldSet( + new FormAction("formaction"), + new FormAction('formactionInAllowedActions') + ) + ); + } + + /** + * @param $data + * @param $form Made optional to simulate error behaviour in "live" environment + * (missing arguments don't throw a fatal error there) + */ + function formaction($data, $form = null) { + return 'formaction'; + } + + function formactionInAllowedActions($data, $form = null) { + return 'formactionInAllowedActions'; + } +} + /** * Simple extension for the test controller */ @@ -322,6 +458,37 @@ class RequestHandlingTest_Form extends Form { } } +class RequestHandlingTest_ControllerFormWithAllowedActions extends Controller { + + function Form() { + return new RequestHandlingTest_FormWithAllowedActions( + $this, + 'Form', + new FieldSet(), + new FieldSet( + new FormAction('allowedformaction'), + new FormAction('disallowedformaction') // disallowed through $allowed_actions in form + ) + ); + } +} + +class RequestHandlingTest_FormWithAllowedActions extends Form { + + static $allowed_actions = array( + 'allowedformaction' => 1, + 'httpSubmission' => 1, // TODO This should be an exception on the parent class + ); + + function allowedformaction() { + return 'allowedformaction'; + } + + function disallowedformaction() { + return 'disallowedformaction'; + } +} + /** * Form field for the test