Permalink
Browse files

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."
  • Loading branch information...
1 parent 474dde8 commit fb784af7380a06b6ae078f7af17e61acb0b3a2f6 @chillu chillu committed Jun 20, 2013
@@ -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() {
View
@@ -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 @@ public function checkAccessAction($action) {
// If defined as empty array, deny action
$isAllowed = false;
} elseif($allowedActions === null) {
- // If undefined, allow action
- $isAllowed = true;
+ // If undefined, allow action based on configuration
+ $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;
}
@@ -219,17 +219,21 @@ 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
-accessible through URLs, or any forms.
+on your own `Controller` subclasses if they contain any actions accessible through URLs.
:::php
class MyController extends Controller {
// This action is now denied because no $allowed_actions are specified
public function myaction($request) {}
}
-Please review all rules governing allowed actions in the
-["controller" topic](/topics/controller).
+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).
+
+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`
@@ -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 undefined, all public methods on the specific class are allowed
- (not recommended)
+ * If `$allowed_actions` is an empty array or undefined, only the `index` action is allowed
* 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
View
@@ -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 @@ public function httpSubmission($request) {
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 @@ public function httpSubmission($request) {
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 @@ protected function checkFieldsForAction($fields, $funcName) {
if($field = $this->checkFieldsForAction($field->FieldList(), $funcName)) {
return $field;
}
- } elseif ($field->hasMethod($funcName)) {
+ } elseif ($field->hasMethod($funcName) && $field->checkAccessAction($funcName)) {
return $field;
}
}
View
@@ -1331,6 +1331,12 @@ class UploadField_ItemHandler extends RequestHandler {
'' => 'index',
);
+ private static $allowed_actions = array(
+ 'delete',
+ 'edit',
+ 'EditForm',
+ );
+
/**
* @param UploadFIeld $parent
* @param int $item
@@ -194,6 +194,12 @@ public function getItemEditFormCallback() {
* @subpackage fields-gridfield
*/
class GridFieldDetailForm_ItemRequest extends RequestHandler {
+
+ private static $allowed_actions = array(
+ 'edit',
+ 'view',
+ 'ItemEditForm'
+ );
/**
*
@@ -53,15 +53,31 @@ public function testAllowedActions() {
$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(),
@@ -348,6 +348,13 @@ public function testIsHttps() {
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']; }
@@ -103,10 +103,6 @@ public function testInheritedUrlHandlers() {
}
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 @@ public function testDisallowedExtendedActions() {
$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 @@ public function getViewer($action = null) {
* 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,15 +536,16 @@ public function handleGet($request) {
}
class RequestHandlingTest_ControllerFormWithAllowedActions extends Controller implements TestOnly {
+
+ private static $allowed_actions = array('Form');
public function Form() {
return new RequestHandlingTest_FormWithAllowedActions(
$this,
'Form',
new FieldList(),
new FieldList(
- new FormAction('allowedformaction'),
- new FormAction('disallowedformaction') // disallowed through $allowed_actions in form
+ new FormAction('allowedformaction')
)
);
}
@@ -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 @@ public function handleInPlaceEdit($request) {
* 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 @@ public function customSomething() {
* 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";
+ }
}
@@ -73,6 +73,8 @@ public function php($data) {
class EmailFieldTest_Controller extends Controller implements TestOnly {
+ private static $allowed_actions = array('Form');
+
private static $url_handlers = array(
'$Action//$ID/$OtherID' => "handleAction",
);
View
@@ -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 @@ public function getViewer($action = null) {
}
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 @@ public function doSubmit($data, $form, $request) {
}
class FormTest_ControllerWithStrictPostCheck extends Controller implements TestOnly {
+
+ private static $allowed_actions = array('Form');
+
protected $template = 'BlankPage';
public function Link($action = null) {
@@ -97,6 +97,8 @@ public function testAdd() {
class GridFieldAddExistingAutocompleterTest_Controller extends Controller implements TestOnly {
+ private static $allowed_actions = array('Form');
+
protected $template = 'BlankPage';
public function Form() {
Oops, something went wrong.

0 comments on commit fb784af

Please sign in to comment.