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

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;
 	

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.
-		// Otherwise, try a handler method on the form object
+		} elseif($this->hasMethod($funcName)) {
-		} else {
+			return $this->$funcName($vars, $this, $request);
-			if($this->hasMethod($funcName)) {
-				return $this->$funcName($vars, $this, $request);
-			}
 		}
+		
+		return $this->httpError(404);
 	}
 	
 	/**

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 @@ 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());
+	}
+	
 }
 
 /**
@@ -230,6 +315,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
  */
@@ -317,6 +453,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