Merge pull request #1851 from chillu/pulls/form-strict-method-check

Form strict method check

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:

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
 

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 @@ class Form extends RequestHandler {
 	 * 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 @@ class Form extends RequestHandler {
 	 * 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.

security/MemberLoginForm.php

@@ -89,6 +89,9 @@ class MemberLoginForm extends LoginForm {
 			$fields->push(new HiddenField('BackURL', 'BackURL', $backURL));
 		}
 
+		// Reduce attack surface by enforcing POST requests
+		$this->setFormMethod('POST', true);
+
 		parent::__construct($controller, $name, $fields, $actions);
 
 		// Focus on the email input when the page is loaded

tests/forms/FormTest.php

@@ -330,6 +330,24 @@ class FormTest extends FunctionalTest {
 		);
 		$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 @@ class FormTest_Controller extends Controller implements TestOnly {
 				'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 @@ class FormTest_ControllerWithSecurityToken extends Controller implements TestOnl
 		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();
+	}
+}