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.3@115191 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

@@ -245,17 +245,41 @@ class Form extends RequestHandler {
 		if(isset($funcName)) {
 			$this->setButtonClicked($funcName);
 		}
-
+		
-		// First, try a handler method on the controller
+		// 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())
+			);
+		}
+		
+		// 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,18 +4,18 @@
  * Tests for RequestHandler and 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 testMethodCallingOnController() {
 		/* Calling a controller works just like it always has */
 		$response = Director::test("testGoodBase1");
 		$this->assertEquals("This is the controller", $response->getBody());
-
+	
 		/* ID and OtherID are extracted from the URL and passed in $request->params. */
 		$response = Director::test("testGoodBase1/method/1/2");
 		$this->assertEquals("This is a method on the controller: 1, 2", $response->getBody());
-
+	
 		/* In addition, these values are availalbe in $controller->urlParams.  This is mainly for backward compatability. */
 		$response = Director::test("testGoodBase1/legacymethod/3/4");
 		$this->assertEquals("\$this->urlParams can be used, for backward compatibility: 3, 4", $response->getBody());
@@ -25,11 +25,11 @@ class RequestHandlingTest extends SapphireTest {
 		/* The HTTP Request handler can trigger special behaviour for GET and POST. */
 		$response = Director::test("testGoodBase1/TestForm", array("MyField" => 3), null, "POST");
 		$this->assertEquals("Form posted", $response->getBody());
-
+	
 		$response = Director::test("testGoodBase1/TestForm");
 		$this->assertEquals("Get request on form", $response->getBody());
 	}
-
+	
 	function testRequestHandlerChaining() {
 		/* Request handlers can be chained, from Director to Controller to Form to FormField.  Here, we can make a get
 		request on a FormField. */
@@ -47,7 +47,7 @@ class RequestHandlingTest extends SapphireTest {
 		of URL rules written before this new request handler. */
 		$response = Director::test("testBadBase/method/1/2");
 		$this->assertEquals("This is a method on the controller: 1, 2", $response->getBody());
-
+	
 		$response = Director::test("testBadBase/TestForm", array("MyField" => 3), null, "POST");
 		$this->assertEquals("Form posted", $response->getBody());
 		
@@ -77,7 +77,7 @@ class RequestHandlingTest extends SapphireTest {
 		/* $url_handlers can be defined on any class, and */
 		$response = Director::test("testGoodBase1/TestForm/fields/SubclassedField/something");
 		$this->assertEquals("customSomething", $response->getBody());
-
+	
 		/* However, if the subclass' url_handlers don't match, then the parent class' url_handlers will be used */
 		$response = Director::test("testGoodBase1/TestForm/fields/SubclassedField");
 		$this->assertEquals("SubclassedField requested", $response->getBody());
@@ -91,7 +91,7 @@ class RequestHandlingTest extends SapphireTest {
 		/* Actions on an extension are allowed because they specifically provided appropriate allowed_actions items */
 		$response = Director::test("testGoodBase1/otherExtendedMethod");
 		$this->assertEquals("otherExtendedMethod", $response->getBody());
-
+	
 		/* The failoverMethod action wasn't explicitly listed and so isnt' allowed */
 		$response = Director::test("testGoodBase1/failoverMethod");
 		$this->assertEquals(403, $response->getStatusCode());
@@ -99,12 +99,99 @@ class RequestHandlingTest extends SapphireTest {
 		/* However, on RequestHandlingTest_AllowedController it has been explicitly allowed */
 		$response = Director::test("RequestHandlingTest_AllowedController/failoverMethod");
 		$this->assertEquals("failoverMethod", $response->getBody());
-
+	
 		/* The action on the extension has also been explicitly allowed even though it wasn't on the extension */
 		$response = Director::test("RequestHandlingTest_AllowedController/extendedMethod");
 		$this->assertEquals("extendedMethod", $response->getBody());
 		
 	}
+	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);
+		// TODO Disallowed through a different execution path than in newer SilverStripe versions -
+		// Controller->hasAction() doesn't exist in 2.3
+		$this->assertEquals(403, $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());
+	}
+	
 }
 
 /**
@@ -178,6 +265,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
  */
@@ -265,6 +403,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