Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

API Form::setStrictFormMethodCheck() and strict argument to setFormMe…

…thod()

Thanks to @sminnee for getting this started
  • Loading branch information...
commit 14c59be85e9ec53cb2f0b3e9d0d8ce7569d913c6 1 parent 59be4a3
@chillu chillu authored
View
8 docs/en/topics/forms.md
@@ -339,6 +339,14 @@ or set on a form field instance via anyone of these methods:
SilverStripe tries to protect users against *Cross-Site Request Forgery (CSRF)* by adding a hidden *SecurityID*
parameter to each form. See [secure-development](/topics/security) for details.
+In addition, you should limit forms to the intended HTTP verb (mostly `GET` or `POST`)
+to further reduce attack surface, by using `[api:Form->setStrictFormMethodCheck()]`.
+
+ :::php
+ $myForm->setFormMethod('POST');
+ $myForm->setStrictFormMethodCheck(true);
+ $myForm->setFormMethod('POST', true); // alternative short notation
+
### Remove existing fields
If you want to remove certain fields from your subclass:
View
17 docs/en/topics/security.md
@@ -275,21 +275,14 @@ Some rules of thumb:
## Cross-Site Request Forgery (CSRF)
-SilverStripe has built-in countermeasures against this type of identity theft for all form submissions. A form object
-will automatically contain a *SecurityID* parameter which is generated as a secure hash on the server, connected to the
+SilverStripe has built-in countermeasures against [CSRF](http://shiflett.org/articles/cross-site-request-forgeries) identity theft for all form submissions. A form object
+will automatically contain a `SecurityID` parameter which is generated as a secure hash on the server, connected to the
currently active session of the user. If this form is submitted without this parameter, or if the parameter doesn't
match the hash stored in the users session, the request is discarded.
+You can disable this behaviour through `[api:Form->disableSecurityToken()]`.
-If you know what you're doing, you can disable this behaviour:
-
- :::php
- $myForm->disableSecurityToken();
-
-
-See
-[http://shiflett.org/articles/cross-site-request-forgeries](http://shiflett.org/articles/cross-site-request-forgeries)
-
-
+It is also recommended to limit form submissions to the intended HTTP verb (mostly `GET` or `POST`)
+through `[api:Form->setStrictFormMethodCheck()]`.
## Casting user input
View
50 forms/Form.php
@@ -65,6 +65,11 @@ class Form extends RequestHandler {
protected $validator;
protected $formMethod = "post";
+
+ /**
+ * @var boolean
+ */
+ protected $strictFormMethodCheck = false;
protected static $current_action;
@@ -239,7 +244,22 @@ public function setupFormErrors() {
* if the form is valid.
*/
public function httpSubmission($request) {
- $vars = $request->requestVars();
+ // Strict method check
+ if($this->strictFormMethodCheck) {
+
+ // Throws an error if the method is bad...
+ if($this->formMethod != strtolower($request->httpMethod())) {
+ $response = Controller::curr()->getResponse();
+ $response->addHeader('Allow', $this->formMethod);
+ $this->httpError(405, _t("Form.BAD_METHOD", "This form requires a ".$this->formMethod." submission"));
+ }
+
+ // ...and only uses the vairables corresponding to that method type
+ $vars = $this->formMethod == 'get' ? $request->getVars() : $request->postVars();
+ } else {
+ $vars = $request->requestVars();
+ }
+
if(isset($funcName)) {
Form::set_current_action($funcName);
}
@@ -794,11 +814,37 @@ public function FormMethod() {
* Set the form method: GET, POST, PUT, DELETE.
*
* @param $method string
+ * @param $strict If non-null, pass value to {@link setStrictFormMethodCheck()}.
*/
- public function setFormMethod($method) {
+ public function setFormMethod($method, $strict = null) {
$this->formMethod = strtolower($method);
+ if($strict !== null) $this->setStrictFormMethodCheck($strict);
+ return $this;
+ }
+
+ /**
+ * If set to true, enforce the matching of the form method.
+ *
+ * This will mean two things:
+ * - GET vars will be ignored by a POST form, and vice versa
+ * - A submission where the HTTP method used doesn't match the form will return a 400 error.
+ *
+ * If set to false (the default), then the form method is only used to construct the default
+ * form.
+ *
+ * @param $bool boolean
+ */
+ public function setStrictFormMethodCheck($bool) {
+ $this->strictFormMethodCheck = (bool)$bool;
return $this;
}
+
+ /**
+ * @return boolean
+ */
+ public function getStrictFormMethodCheck() {
+ return $this->strictFormMethodCheck;
+ }
/**
* Return the form's action attribute.
View
77 tests/forms/FormTest.php
@@ -330,6 +330,24 @@ public function testDisableSecurityTokenAcceptsSubmissionWithoutToken() {
);
$this->assertEquals(200, $response->getStatusCode(), 'Submission suceeds with security token');
}
+
+ public function testStrictFormMethodChecking() {
+ $response = $this->get('FormTest_ControllerWithStrictPostCheck');
+ $response = $this->get(
+ 'FormTest_ControllerWithStrictPostCheck/Form/?Email=test@test.com&action_doSubmit=1'
+ );
+ $this->assertEquals(405, $response->getStatusCode(), 'Submission fails with wrong method');
+
+ $response = $this->get('FormTest_ControllerWithStrictPostCheck');
+ $response = $this->post(
+ 'FormTest_ControllerWithStrictPostCheck/Form',
+ array(
+ 'Email' => 'test@test.com',
+ 'action_doSubmit' => 1
+ )
+ );
+ $this->assertEquals(200, $response->getStatusCode(), 'Submission succeeds with correct method');
+ }
public function testEnableSecurityToken() {
SecurityToken::disable();
@@ -468,28 +486,11 @@ public function Form() {
'SomeRequiredField'
)
);
-
- // Disable CSRF protection for easier form submission handling
- $form->disableSecurityToken();
+ $form->disableSecurityToken(); // Disable CSRF protection for easier form submission handling
return $form;
}
- public function FormWithSecurityToken() {
- $form = new Form(
- $this,
- 'FormWithSecurityToken',
- new FieldList(
- new EmailField('Email')
- ),
- new FieldList(
- new FormAction('doSubmit')
- )
- );
-
- return $form;
- }
-
public function doSubmit($data, $form, $request) {
$form->sessionMessage('Test save was successful', 'good');
return $this->redirectBack();
@@ -533,12 +534,40 @@ public function doSubmit($data, $form, $request) {
return $this->redirectBack();
}
- public function getViewer($action = null) {
- return new SSViewer('BlankPage');
- }
}
-Config::inst()->update('Director', 'rules', array(
- 'FormTest_Controller' => 'FormTest_Controller'
-));
+class FormTest_ControllerWithStrictPostCheck extends Controller implements TestOnly {
+ protected $template = 'BlankPage';
+
+ public function Link($action = null) {
+ return Controller::join_links(
+ 'FormTest_ControllerWithStrictPostCheck',
+ $this->request->latestParam('Action'),
+ $this->request->latestParam('ID'),
+ $action
+ );
+ }
+
+ public function Form() {
+ $form = new Form(
+ $this,
+ 'Form',
+ new FieldList(
+ new EmailField('Email')
+ ),
+ new FieldList(
+ new FormAction('doSubmit')
+ )
+ );
+ $form->setFormMethod('POST');
+ $form->setStrictFormMethodCheck(true);
+ $form->disableSecurityToken(); // Disable CSRF protection for easier form submission handling
+ return $form;
+ }
+
+ public function doSubmit($data, $form, $request) {
+ $form->sessionMessage('Test save was successful', 'good');
+ return $this->redirectBack();
+ }
+}
Please sign in to comment.
Something went wrong with that request. Please try again.