Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

BUGFIX Checking for existence of FormAction in Form->httpSubmission()…

… to avoid bypassing $allowed_actions definitions in controllers containing this form

BUGFIX Checking for $allowed_actions in Form class, through Form->httpSubmission() (from r115182)

git-svn-id: svn://svn.silverstripe.com/silverstripe/open/modules/sapphire/branches/2.4@115188 467b73ca-7a2a-4603-9d3b-597d59a354a9
  • Loading branch information...
commit 2962fb8d1399036ace7f58da6d00936c417bfb04 1 parent 521a76b
@chillu chillu authored sminnee committed
View
6 core/control/RequestHandler.php
@@ -66,6 +66,12 @@ class RequestHandler extends ViewableData {
* 'complexaction' '->canComplexAction' // complexaction can only be accessed if $this->canComplexAction() returns true
* );
* </code>
+ *
+ * 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;
View
38 forms/Form.php
@@ -264,6 +264,31 @@ function httpSubmission($request) {
$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 @@ function httpSubmission($request) {
}
}
- // 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);
}
/**
View
169 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() {
@@ -144,6 +144,91 @@ function testMethodsOnParentClassesOfRequestHandlerDeclined() {
$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());
+ }
+
}
/**
@@ -230,6 +315,57 @@ public function throwhttperror() {
}
+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
*/
@@ -317,6 +453,37 @@ function handleGet($request) {
}
}
+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
Please sign in to comment.
Something went wrong with that request. Please try again.