API Enforce $allowed_actions in RequestHandler->checkAccessAction()

See discussion at https://groups.google.com/forum/?fromgroups#!topic/silverstripe-dev/Dodomh9QZjk

Fixes an access issue where all public methods on FormField were allowed,
and not checked for $allowed_actions. Before this patch you could e.g.
call FormField->Value() on the first field by using action_Value.

Removes the following assertion because it only worked due to RequestHandlingTest_AllowedControllerExtension
*not* having $allowed_extensions declared: "Actions on magic methods are only accessible if explicitly allowed on the controller."

admin/code/CMSProfileController.php

@@ -6,6 +6,7 @@ class CMSProfileController extends LeftAndMain {
 	private static $menu_title = 'My Profile';
 
 	private static $required_permission_codes = false;
+
 	private static $tree_class = 'Member';
 
 	public function getResponseNegotiator() {

control/RequestHandler.php

@@ -96,6 +96,16 @@ class RequestHandler extends ViewableData {
 	 * @config
 	 */
 	private static $allowed_actions = null;
+
+	/**
+	 * @config
+	 * @var boolean Enforce presence of $allowed_actions when checking acccess.
+	 * Defaults to TRUE, meaning all URL actions will be denied.
+	 * When set to FALSE, the controller will allow *all* public methods to be called.
+	 * In most cases this isn't desireable, and in fact a security risk, 
+	 * since some helper methods can cause side effects which shouldn't be exposed through URLs.
+	 */
+	private static $require_allowed_actions = true;
 	
 	public function __construct() {
 		$this->brokenOnConstruct = false;
@@ -430,12 +440,12 @@ class RequestHandler extends ViewableData {
 			// If defined as empty array, deny action
 			$isAllowed = false;
 		} elseif($allowedActions === null) {
-			// If undefined, allow action
+			// If undefined, allow action based on configuration
-			$isAllowed = true;
+			$isAllowed = !Config::inst()->get('RequestHandler', 'require_allowed_actions');
 		}
 
 		// If we don't have a match in allowed_actions,
-		// whitelist the 'index' action as well as undefined actions.
+		// whitelist the 'index' action as well as undefined actions based on configuration.
 		if(!$isDefined && ($action == 'index' || empty($action))) {
 			$isAllowed = true;
 		}

docs/en/changelogs/3.1.0.md

@@ -219,8 +219,7 @@ For more information about how to use the config system, see the ["Configuration
 
 In order to make controller access checks more consistent and easier to
 understand, the routing will require definition of `$allowed_actions`
-on your own `Controller` subclasses if they contain any actions
+on your own `Controller` subclasses if they contain any actions accessible through URLs.
-accessible through URLs, or any forms.
 
 	:::php
 	class MyController extends Controller {
@@ -228,8 +227,13 @@ accessible through URLs, or any forms.
 		public function myaction($request) {}
 	}
 
-Please review all rules governing allowed actions in the
+You can overwrite the default behaviour on undefined `$allowed_actions` to allow all actions,
-["controller" topic](/topics/controller).
+by setting the `RequestHandler.require_allowed_actions` config value to `false` (not recommended).
+
+This applies to anything extending `RequestHandler`, so please check your `Form` and `FormField`
+subclasses as well. Keep in mind, action methods as denoted through `FormAction` names should NOT
+be mentioned in `$allowed_actions` to avoid CSRF issues.
+Please review all rules governing allowed actions in the ["controller" topic](/topics/controller).
 
 ### Removed support for "*" rules in `Controller::$allowed_actions`
 

docs/en/topics/controller.md

@@ -87,9 +87,7 @@ way to perform checks against permission codes or custom logic.
 There's a couple of rules guiding these checks:
 
  * Each class is only responsible for access control on the methods it defines
- * If `$allowed_actions` is defined as an empty array, no actions are allowed
+ * If `$allowed_actions` is an empty array or undefined, only the `index` action is allowed
- * If `$allowed_actions` is undefined, all public methods on the specific class are allowed 
-   (not recommended)
  * Access checks on parent classes need to be overwritten via the Config API
  * Only public methods can be made accessible
  * If a method on a parent class is overwritten, access control for it has to be redefined as well
@@ -102,6 +100,8 @@ There's a couple of rules guiding these checks:
  * `$allowed_actions` can be defined on `Extension` classes applying to the controller.
 
 If the permission check fails, SilverStripe will return a "403 Forbidden" HTTP status.
+You can overwrite the default behaviour on undefined `$allowed_actions` to allow all actions,
+by setting the `RequestHandler.require_allowed_actions` config value to `false` (not recommended).
 
 ### Through the action
 
@@ -173,21 +173,6 @@ through `/fastfood/drivethrough/` to use the same order function.
 	        'drivethrough/$Action/$ID/$Name' => 'order'
 	    );
 
-## Access Control
-
-### Through $allowed_actions
-
- * If `$allowed_actions` is undefined, `null` or `array()`, no actions are accessible
- * Each class is only responsible for access control on the methods it defines
- * Access checks on parent classes need to be overwritten via the Config API
- * Only public methods can be made accessible
- * If a method on a parent class is overwritten, access control for it has to be redefined as well
- * An action named "index" is whitelisted by default
- * Methods returning forms also count as actions which need to be defined
- * Form action methods (targets of `FormAction`) should NOT be included in `$allowed_actions`,
-   they're handled separately through the form routing (see the ["forms" topic](/topics/forms))
- * `$allowed_actions` can be defined on `Extension` classes applying to the controller.
-
 ## URL Patterns
 
 The `[api:RequestHandler]` class will parse all rules you specify against the 

forms/Form.php

@@ -149,6 +149,8 @@ class Form extends RequestHandler {
 	 */
 	protected $attributes = array();
 
+	private static $allowed_actions = array('handleField', 'httpSubmission');
+
 	/**
 	 * Create a new form, with the given fields an action buttons.
 	 * 
@@ -298,7 +300,7 @@ class Form extends RequestHandler {
 			Form::set_current_action($funcName);
 			$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,
@@ -360,6 +362,19 @@ class Form extends RequestHandler {
 		return $this->httpError(404);
 	}
 
+	public function checkAccessAction($action) {
+		return (
+			parent::checkAccessAction($action)
+			// Always allow actions which map to buttons. See httpSubmission() for further access checks.
+			|| $this->actions->dataFieldByName('action_' . $action)
+			// Always allow actions on fields
+			|| (
+				$field = $this->checkFieldsForAction($this->Fields(), $action)
+				&& $field->checkAccessAction($action)
+			)
+		);
+	}
+
 	/**
 	 * Returns the appropriate response up the controller chain
 	 * if {@link validate()} fails (which is checked prior to executing any form actions).
@@ -412,7 +427,7 @@ class Form extends RequestHandler {
 				if($field = $this->checkFieldsForAction($field->FieldList(), $funcName)) {
 					return $field;
 				}
-			} elseif ($field->hasMethod($funcName)) {
+			} elseif ($field->hasMethod($funcName) && $field->checkAccessAction($funcName)) {
 				return $field;
 			}
 		}

forms/UploadField.php

@@ -1331,6 +1331,12 @@ class UploadField_ItemHandler extends RequestHandler {
 		'' => 'index',
 	);
 
+	private static $allowed_actions = array(
+		'delete',
+		'edit',
+		'EditForm',
+	);
+
 	/**
 	 * @param UploadFIeld $parent
 	 * @param int $item

forms/gridfield/GridFieldDetailForm.php

@@ -194,6 +194,12 @@ class GridFieldDetailForm implements GridField_URLHandler {
  * @subpackage fields-gridfield
  */
 class GridFieldDetailForm_ItemRequest extends RequestHandler {
+
+	private static $allowed_actions = array(
+		'edit',
+		'view',
+		'ItemEditForm'
+	);
 	
 	/**
 	 *

tests/control/ControllerTest.php

@@ -53,15 +53,31 @@ class ControllerTest extends FunctionalTest {
 
 		$response = $this->get("ControllerTest_UnsecuredController/index");
 		$this->assertEquals(200, $response->getStatusCode(),
-			'Access granted on index action without $allowed_actions on defining controller, ' .
+			'Access denied on index action without $allowed_actions on defining controller, ' .
 			'when called with an action in the URL'
 		);
 
+		Config::inst()->update('RequestHandler', 'require_allowed_actions', false);
+		$response = $this->get("ControllerTest_UnsecuredController/index");
+		$this->assertEquals(200, $response->getStatusCode(),
+			'Access granted on index action without $allowed_actions on defining controller, ' .
+			'when called with an action in the URL, and explicitly allowed through config'
+		);
+		Config::inst()->update('RequestHandler', 'require_allowed_actions', true);
+
+		$response = $this->get("ControllerTest_UnsecuredController/method1");
+		$this->assertEquals(403, $response->getStatusCode(),
+			'Access denied on action without $allowed_actions on defining controller, ' .
+			'when called without an action in the URL'
+		);
+
+		Config::inst()->update('RequestHandler', 'require_allowed_actions', false);
 		$response = $this->get("ControllerTest_UnsecuredController/method1");
 		$this->assertEquals(200, $response->getStatusCode(),
 			'Access granted on action without $allowed_actions on defining controller, ' .
-			'when called without an action in the URL'
+			'when called without an action in the URL, and explicitly allowed through config'
 		);
+		Config::inst()->update('RequestHandler', 'require_allowed_actions', true);
 		
 		$response = $this->get("ControllerTest_AccessBaseController/");
 		$this->assertEquals(200, $response->getStatusCode(),

tests/control/DirectorTest.php

@@ -348,6 +348,13 @@ class DirectorTest extends SapphireTest {
 
 class DirectorTestRequest_Controller extends Controller implements TestOnly {
 
+	private static $allowed_actions = array(
+		'returnGetValue',
+		'returnPostValue',
+		'returnRequestValue',
+		'returnCookieValue',
+	);
+
 	public function returnGetValue($request)		{ return $_GET['somekey']; }
 
 	public function returnPostValue($request)		{ return $_POST['somekey']; }

tests/control/RequestHandlingTest.php

@@ -103,10 +103,6 @@ class RequestHandlingTest extends FunctionalTest {
 	}
 	
 	public function testDisallowedExtendedActions() {
-		/* Actions on magic methods are only accessible if explicitly allowed on the controller. */
-		$response = Director::test("testGoodBase1/extendedMethod");
-		$this->assertEquals(404, $response->getStatusCode());
-		
 		/* Actions on an extension are allowed because they specifically provided appropriate allowed_actions items */
 		$response = Director::test("testGoodBase1/otherExtendedMethod");
 		$this->assertEquals("otherExtendedMethod", $response->getBody());
@@ -119,9 +115,10 @@ class RequestHandlingTest extends FunctionalTest {
 		$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 */
+		/* The action on the extension is allowed when explicitly allowed on extension, 
+			even if its not mentioned in controller */
 		$response = Director::test("RequestHandlingTest_AllowedController/extendedMethod");
-		$this->assertEquals("extendedMethod", $response->getBody());
+		$this->assertEquals(200, $response->getStatusCode());
 		
 		/* This action has been blocked by an argument to a method */
 		$response = Director::test('RequestHandlingTest_AllowedController/blockMethod');
@@ -420,9 +417,13 @@ class RequestHandlingTest_FormActionController extends Controller {
  * Simple extension for the test controller
  */
 class RequestHandlingTest_ControllerExtension extends Extension {
+	
 	public static $called_error = false;
+	
 	public static $called_404_error = false;
 
+	private static $allowed_actions = array('extendedMethod');
+
 	public function extendedMethod() {
 		return "extendedMethod";
 	}
@@ -454,7 +455,6 @@ class RequestHandlingTest_AllowedController extends Controller implements TestOn
 	
 	private static $allowed_actions = array(
 		'failoverMethod', // part of the failover object
-		'extendedMethod', // part of the RequestHandlingTest_ControllerExtension object
 		'blockMethod' => '->provideAccess(false)',
 		'allowMethod' => '->provideAccess',
 	);
@@ -536,6 +536,8 @@ class RequestHandlingTest_Form extends Form {
 }
 
 class RequestHandlingTest_ControllerFormWithAllowedActions extends Controller implements TestOnly {
+
+	private static $allowed_actions = array('Form');
 	
 	public function Form() {
 		return new RequestHandlingTest_FormWithAllowedActions(
@@ -543,8 +545,7 @@ class RequestHandlingTest_ControllerFormWithAllowedActions extends Controller im
 			'Form',
 			new FieldList(),
 			new FieldList(
-				new FormAction('allowedformaction'),
+				new FormAction('allowedformaction')
-				new FormAction('disallowedformaction') // disallowed through $allowed_actions in form
 			)
 		);
 	}
@@ -554,7 +555,6 @@ class RequestHandlingTest_FormWithAllowedActions extends Form {
 
 	private static $allowed_actions = array(
 		'allowedformaction' => 1,
-		'httpSubmission' => 1, // TODO This should be an exception on the parent class
 	);
 	
 	public function allowedformaction() {
@@ -602,6 +602,9 @@ class RequestHandlingTest_FormField extends FormField {
  * Form field for the test
  */
 class RequestHandlingTest_SubclassedFormField extends RequestHandlingTest_FormField {
+
+	private static $allowed_actions = array('customSomething');
+	
 	// We have some url_handlers defined that override RequestHandlingTest_FormField handlers.
 	// We will confirm that the url_handlers inherit.
 	private static $url_handlers = array(
@@ -619,6 +622,8 @@ class RequestHandlingTest_SubclassedFormField extends RequestHandlingTest_FormFi
  * Controller for the test
  */
 class RequestHandlingFieldTest_Controller extends Controller implements TestOnly {
+
+	private static $allowed_actions = array('TestForm');
 	
 	public function TestForm() {
 		return new Form($this, "TestForm", new FieldList(
@@ -641,4 +646,8 @@ class RequestHandlingTest_HandlingField extends FormField {
 	public function actionOnField() {
 		return "Test method on $this->name";
 	}
+
+	public function actionNotAllowedOnField() {
+		return "actionNotAllowedOnField on $this->name";
+	}
 }

tests/forms/EmailFieldTest.php

@@ -73,6 +73,8 @@ class EmailFieldTest_Validator extends Validator {
 
 class EmailFieldTest_Controller extends Controller implements TestOnly {
 
+	private static $allowed_actions = array('Form');
+
 	private static $url_handlers = array(
 		'$Action//$ID/$OtherID' => "handleAction",
 	);

tests/forms/FormTest.php

@@ -458,6 +458,9 @@ class FormTest_Team extends DataObject implements TestOnly {
 }
 
 class FormTest_Controller extends Controller implements TestOnly {
+
+	private static $allowed_actions = array('Form');
+
 	private static $url_handlers = array(
 		'$Action//$ID/$OtherID' => "handleAction",
 	);
@@ -503,6 +506,9 @@ class FormTest_Controller extends Controller implements TestOnly {
 }
 
 class FormTest_ControllerWithSecurityToken extends Controller implements TestOnly {
+	
+	private static $allowed_actions = array('Form');
+
 	private static $url_handlers = array(
 		'$Action//$ID/$OtherID' => "handleAction",
 	);
@@ -537,6 +543,9 @@ class FormTest_ControllerWithSecurityToken extends Controller implements TestOnl
 }
 
 class FormTest_ControllerWithStrictPostCheck extends Controller implements TestOnly {
+
+	private static $allowed_actions = array('Form');
+	
 	protected $template = 'BlankPage';
 	
 	public function Link($action = null) {

tests/forms/gridfield/GridFieldAddExistingAutocompleterTest.php

@@ -97,6 +97,8 @@ class GridFieldAddExistingAutocompleterTest extends FunctionalTest {
 
 class GridFieldAddExistingAutocompleterTest_Controller extends Controller implements TestOnly {
 
+	private static $allowed_actions = array('Form');
+
 	protected $template = 'BlankPage';
 
 	public function Form() {

tests/forms/gridfield/GridFieldDetailFormTest.php

@@ -282,6 +282,7 @@ class GridFieldDetailFormTest_PeopleGroup extends DataObject implements TestOnly
 }
 
 class GridFieldDetailFormTest_Category extends DataObject implements TestOnly {
+	
 	private static $db = array(
 		'Name' => 'Varchar'
 	);
@@ -306,6 +307,9 @@ class GridFieldDetailFormTest_Category extends DataObject implements TestOnly {
 }
 
 class GridFieldDetailFormTest_Controller extends Controller implements TestOnly {
+	
+	private static $allowed_actions = array('Form');
+
 	protected $template = 'BlankPage';
 
 	public function Form() {
@@ -326,6 +330,9 @@ class GridFieldDetailFormTest_Controller extends Controller implements TestOnly
 }
 
 class GridFieldDetailFormTest_GroupController extends Controller implements TestOnly {
+
+	private static $allowed_actions = array('Form');
+
 	protected $template = 'BlankPage';
 
 	public function Form() {
@@ -339,6 +346,9 @@ class GridFieldDetailFormTest_GroupController extends Controller implements Test
 }
 
 class GridFieldDetailFormTest_CategoryController extends Controller implements TestOnly {
+
+	private static $allowed_actions = array('Form');
+	
 	protected $template = 'BlankPage';
 
 	public function Form() {

tests/forms/gridfield/GridField_URLHandlerTest.php

@@ -30,6 +30,9 @@ class GridField_URLHandlerTest extends FunctionalTest {
 }
 
 class GridField_URLHandlerTest_Controller extends Controller implements TestOnly {
+
+	private static $allowed_actions = array('Form');
+
 	public function Link() {
 		return get_class($this) ."/";
 	}
@@ -51,6 +54,9 @@ class GridField_URLHandlerTest_Controller extends Controller implements TestOnly
  * Test URLHandler with a nested request handler
  */
 class GridField_URLHandlerTest_Component extends RequestHandler implements GridField_URLHandler {
+	
+	private static $allowed_actions = array('Form', 'showform', 'testpage', 'handleItem');
+
 	protected $gridField;
 	
 	public function getURLHandlers($gridField) {
@@ -96,8 +102,13 @@ class GridField_URLHandlerTest_Component extends RequestHandler implements GridF
 }
 
 class GridField_URLHandlerTest_Component_ItemRequest extends RequestHandler {
+	
+	private static $allowed_actions = array('Form', 'showform', 'testpage');
+
 	protected $gridField;
+	
 	protected $link;
+	
 	protected $id;
 	
 	public function __construct($gridField, $id, $link) {

tests/security/SecurityTest.php

@@ -439,6 +439,9 @@ class SecurityTest extends FunctionalTest {
 }
 
 class SecurityTest_SecuredController extends Controller implements TestOnly {
+
+	private static $allowed_actions = array('index');
+
 	public function index() {
 		if(!Permission::check('ADMIN')) return Security::permissionFailure($this);