ENHANCEMENT Added SecurityToken to wrap CSRF protection via "SecurityID" request parameter (from r113272)

git-svn-id: svn://svn.silverstripe.com/silverstripe/open/modules/sapphire/trunk@114525 467b73ca-7a2a-4603-9d3b-597d59a354a9

dev/FunctionalTest.php

@@ -70,9 +70,13 @@ class FunctionalTest extends SapphireTest {
         
         // Unprotect the site, tests are running with the assumption it's off. They will enable it on a case-by-case basis.
         BasicAuth::protect_entire_site(false);
+
+		SecurityToken::disable();
 	}
 
 	function tearDown() {
+		SecurityToken::enable();
+		
 		parent::tearDown();
 		unset($this->mainSession);
 	}

forms/Form.php

@@ -119,6 +119,11 @@ class Form extends RequestHandler {
 	
 	protected $security = true;
 	
+	/**
+	 * @var SecurityToken
+	 */
+	protected $securityToken = null;
+	
 	/**
 	 * HACK This is a temporary hack to allow multiple calls to includeJavascriptValidation on
 	 * the validator (if one is present).
@@ -168,10 +173,12 @@ class Form extends RequestHandler {
 		// Check if CSRF protection is enabled, either on the parent controller or from the default setting. Note that
 		// method_exists() is used as some controllers (e.g. GroupTest) do not always extend from Object.
 		if(method_exists($controller, 'securityTokenEnabled') || (method_exists($controller, 'hasMethod') && $controller->hasMethod('securityTokenEnabled'))) {
-			$this->security = $controller->securityTokenEnabled();
+			$securityEnabled = $controller->securityTokenEnabled();
 		} else {
-			$this->security = self::$default_security;
+			$securityEnabled = SecurityToken::is_enabled();
 		}
+		
+		$this->securityToken = ($securityEnabled) ? new SecurityToken() : new NullSecurityToken();
 	}
 	
 	static $url_handlers = array(
@@ -225,12 +232,9 @@ class Form extends RequestHandler {
 		$this->loadDataFrom($vars, true);
 		
 		// Protection against CSRF attacks
-		if($this->securityTokenEnabled()) {
+		$token = $this->getSecurityToken();
-			$securityID = Session::get('SecurityID');
+		if(!$token->checkRequest($request)) {
-
+			$this->httpError(400, "Security token doesn't match, possible CSRF attack.");
-			if(!$securityID || !isset($vars['SecurityID']) || $securityID != $vars['SecurityID']) {
-				$this->httpError(400, "SecurityID doesn't match, possible CSRF attack.");
-			}
 		}
 		
 		// Determine the action button clicked
@@ -430,27 +434,17 @@ class Form extends RequestHandler {
 
 		
 	/**
-	 * Generate extra special fields - namely the SecurityID field
+	 * Generate extra special fields - namely the security token field (if required).
 	 * 
 	 * @return FieldSet
 	 */
 	public function getExtraFields() {
 		$extraFields = new FieldSet();
 		
-		if(!$this->fields->fieldByName('SecurityID') && $this->securityTokenEnabled()) {
+		$token = $this->getSecurityToken();
-			if(Session::get('SecurityID')) {
+		$tokenField = $token->updateFieldSet($this->fields);
-				$securityID = Session::get('SecurityID');
+		if($tokenField) $tokenField->setForm($this);
-			} else {
-				$generator = new RandomGenerator();
-				$securityID = $generator->generateHash('sha1');
-				Session::set('SecurityID', $securityID);
-			}
-			
-			$securityField = new HiddenField('SecurityID', '', $securityID);
-			$securityField->setForm($this);
-			$extraFields->push($securityField);
 		$this->securityTokenAdded = true;
-		}
 		
 		// add the "real" HTTP method if necessary (for PUT, DELETE and HEAD)
 		if($this->FormMethod() != $this->FormHttpMethod()) {
@@ -859,7 +853,11 @@ class Form extends RequestHandler {
 	 * Processing that occurs before a form is executed.
 	 * This includes form validation, if it fails, we redirect back
 	 * to the form with appropriate error messages.
-	 * Triggered through {@link httpSubmission()} which is triggered
+	 * Triggered through {@link httpSubmission()}.
+	 * Note that CSRF protection takes place in {@link httpSubmission()},
+	 * if it fails the form data will never reach this method.
+	 * 
+	 * @return boolean
 	 */
 	 function validate(){
 		if($this->validator){
@@ -1167,32 +1165,49 @@ class Form extends RequestHandler {
 	}
 	
 	/**
-	 * Disable the requirement of a SecurityID in the Form. This security protects
+	 * Disable the requirement of a security token in the Form. This security protects
 	 * against CSRF attacks, but you should disable this if you don't want to tie 
 	 * a form to a session - eg a search form.
 	 */
 	function disableSecurityToken() {
-		$this->security = false;
+		$this->securityToken = new NullSecurityToken();
 	}
 	
-	
-	private static $default_security = true;
-	
 	/**
-	 * Disable security tokens for every form on this site.
+	 * Disable security tokens for every form.
+	 * Note that this doesn't apply to {@link SecurityToken}
+	 * instances outside of the Form class, nor applies
+	 * to existing form instances.
+	 * 
+	 * See {@link enable_all_security_tokens()}.
+	 * 
+	 * @deprecated 2.5 Use SecurityToken::disable()
 	 */
 	static function disable_all_security_tokens() {
-		self::$default_security = false;
+		SecurityToken::disable();
 	}
 	
 	/**
-	 * Returns true if security is enabled - that is if the SecurityID
+	 * Returns true if security is enabled - that is if the security token
 	 * should be included and checked on this form.
 	 * 
+	 * @deprecated 2.5 Use Form->getSecurityToken()->isEnabled()
+	 *
 	 * @return bool
 	 */
 	function securityTokenEnabled() {
-		return $this->security;
+		return $this->securityToken->isEnabled();
+	}
+	
+	/**
+	 * Returns the security token for this form (if any exists).
+	 * Doesn't check for {@link securityTokenEnabled()}.
+	 * Use {@link SecurityToken::inst()} to get a global token.
+	 * 
+	 * @return SecurityToken|null
+	 */
+	function getSecurityToken() {
+		return $this->securityToken;
 	}
 		
 	/**

security/SecurityToken.php

@@ -0,0 +1,273 @@
+<?php
+/**
+ * @package sapphire
+ * @subpackage security
+ */
+
+/**
+ * Cross Site Request Forgery (CSRF) protection for the {@link Form} class and other GET links.
+ * Can be used globally (through {@link SecurityToken::inst()})
+ * or on a form-by-form basis {@link Form->getSecurityToken()}.
+ * 
+ * <b>Usage in forms</b>
+ * 
+ * This protective measure is automatically turned on for all new {@link Form} instances,
+ * and can be globally disabled through {@link disable()}.
+ * 
+ * <b>Usage in custom controller actions</b>
+ * 
+ * <code>
+ * class MyController extends Controller {
+ * 	function mygetaction($request) {
+ * 		if(!SecurityToken::inst()->checkRequest($request)) return $this->httpError(400);
+ * 
+ * 		// valid action logic ...
+ * 	}
+ * }
+ * </code>
+ * 
+ * @todo Make token name form specific for additional forgery protection.
+ */
+class SecurityToken extends Object {
+	
+	/**
+	 * @var String
+	 */
+	protected static $default_name = 'SecurityID';
+	
+	/**
+	 * @var SecurityToken
+	 */
+	protected static $inst = null;
+	
+	/**
+	 * @var boolean
+	 */
+	protected static $enabled = true;
+	
+	/**
+	 * @var String $name
+	 */
+	protected $name = null;
+	
+	/**
+	 * @param $name
+	 */
+	function __construct($name = null) {
+		$this->name = ($name) ? $name : self::get_default_name();
+		// only regenerate if the token isn't already set in the session
+		if(!$this->getValue()) $this->setValue($this->generate());
+		
+		parent::__construct();
+	}
+	
+	/**
+	 * Gets a global token (or creates one if it doesnt exist already).
+	 * 
+	 * @return SecurityToken
+	 */
+	static function inst() {
+		if(!self::$inst) self::$inst = new SecurityToken();
+
+		return self::$inst;
+	}
+	
+	/**
+	 * Globally disable the token (override with {@link NullSecurityToken})
+	 * implementation. Note: Does not apply for 
+	 */
+	static function disable() {
+		self::$enabled = false;
+		self::$inst = new NullSecurityToken();
+	}
+	
+	/**
+	 * Globally enable tokens that have been previously disabled through {@link disable}.
+	 */
+	static function enable() {
+		self::$enabled = true;
+		self::$inst = new SecurityToken();
+	}
+	
+	/**
+	 * @return boolean
+	 */
+	static function is_enabled() {
+		return self::$enabled;
+	}
+	
+	/**
+	 * @return String
+	 */
+	static function get_default_name() {
+		return self::$default_name;
+	}
+	
+	/**
+	 * @return String
+	 */
+	function setName($name) {
+		$val = $this->getValue();
+		$this->name = $name;
+		$this->setValue($val);
+	}
+	
+	/**
+	 * @return String
+	 */
+	function getName() {
+		return $this->name;
+	}
+	
+	/**
+	 * @return String
+	 */
+	function getValue() {
+		return Session::get($this->getName());
+	}
+	
+	/**
+	 * @param String $val
+	 */
+	function setValue($val) {
+		Session::set($this->getName(), $val);
+	}
+	
+	/**
+	 * Checks for an existing CSRF token in the current users session.
+	 * This check is automatically performed in {@link Form->httpSubmission()}
+	 * if a form has security tokens enabled.
+	 * This direct check is mainly used for URL actions on {@link FormField} that are not routed
+	 * through {@link Form->httpSubmission()}.
+	 * 
+	 * Typically you'll want to check {@link Form->securityTokenEnabled()} before calling this method.
+	 * 
+	 * @param String $compare
+	 * @return Boolean
+	 */
+	function check($compare) {
+		return ($compare && $this->getValue() && $compare == $this->getValue());
+	}
+	
+	/**
+	 * See {@link check()}.
+	 * 
+	 * @param SS_HTTPRequest $request
+	 * @return Boolean
+	 */
+	function checkRequest($request) {
+		return $this->check($request->requestVar($this->getName()));
+	}
+	
+	/**
+	 * Note: Doesn't call {@link FormField->setForm()}
+	 * on the returned {@link HiddenField}, you'll need to take
+	 * care of this yourself.
+	 * 
+	 * @param FieldSet $fieldset
+	 * @return HiddenField|false
+	 */
+	function updateFieldSet(&$fieldset) {
+		if(!$fieldset->fieldByName($this->getName())) {
+			$field = new HiddenField($this->getName(), null, $this->getValue());
+			$fieldset->push($field);
+			return $field;
+		} else {
+			return false;
+		}
+	}
+	
+	/**
+	 * @param String $url
+	 * @return String
+	 */
+	function addToUrl($url) {
+		return Controller::join_links($url, sprintf('?%s=%s', $this->getName(), $this->getValue()));
+	}
+	
+	/**
+	 * You can't disable an existing instance, it will need to be overwritten like this:
+	 * <code>
+	 * $old = SecurityToken::inst(); // isEnabled() returns true
+	 * SecurityToken::disable();
+	 * $new = SecurityToken::inst(); // isEnabled() returns false
+	 * </code>
+	 * 
+	 * @return boolean
+	 */
+	function isEnabled() {
+		return !($this instanceof NullSecurityToken);
+	}
+	
+	/**
+	 * @return String
+	 */
+	protected function generate() {
+		return rand();
+	}
+	
+}
+
+/**
+ * Specialized subclass for disabled security tokens - always returns
+ * TRUE for token checks. Use through {@link SecurityToken::disable()}.
+ */
+class NullSecurityToken extends SecurityToken {
+	
+	/**
+	 * @param String
+	 * @return boolean
+	 */
+	function check($compare) {
+		return true;
+	}
+	
+	/**
+	 * @param SS_HTTPRequest $request
+	 * @return Boolean
+	 */
+	function checkRequest($request) {
+		return true;
+	}
+	
+	/**
+	 * @param FieldSet $fieldset
+	 * @return false
+	 */
+	function updateFieldSet(&$fieldset) {
+		// Remove, in case it was added beforehand
+		$fieldset->removeByName($this->getName());
+		
+		return false;
+	}
+	
+	/**
+	 * @param String $url
+	 * @return String
+	 */
+	function addToUrl($url) {
+		return $url;
+	}
+	
+	/**
+	 * @return String
+	 */
+	function getValue() {
+		return null;
+	}
+	
+	/**
+	 * @param String $val
+	 */
+	function setValue($val) {
+		// no-op
+	}
+	
+	/**
+	 * @return String
+	 */
+	function generate() {
+		return null;
+	}
+	
+}

tests/forms/FormTest.php

@@ -239,6 +239,72 @@ class FormTest extends FunctionalTest {
 		);
 	}
 	
+	function testGloballyDisabledSecurityTokenInheritsToNewForm() {
+		SecurityToken::enable();
+		
+		$form1 = $this->getStubForm();
+		$this->assertType('SecurityToken', $form1->getSecurityToken());
+		
+		SecurityToken::disable();
+		
+		$form2 = $this->getStubForm();
+		$this->assertType('NullSecurityToken', $form2->getSecurityToken());
+		
+		SecurityToken::enable();
+	}
+	
+	function testDisableSecurityTokenDoesntAddTokenFormField() {
+		SecurityToken::enable();
+		
+		$formWithToken = $this->getStubForm();
+		$this->assertType(
+			'HiddenField',
+			$formWithToken->Fields()->fieldByName(SecurityToken::get_default_name()),
+			'Token field added by default'
+		);
+		
+		$formWithoutToken = $this->getStubForm();
+		$formWithoutToken->disableSecurityToken();
+		$this->assertNull(
+			$formWithoutToken->Fields()->fieldByName(SecurityToken::get_default_name()),
+			'Token field not added if disableSecurityToken() is set'
+		);
+	}
+	
+	function testDisableSecurityTokenAcceptsSubmissionWithoutToken() {
+		SecurityToken::enable();
+		
+		$response = $this->get('FormTest_ControllerWithSecurityToken');
+		// can't use submitForm() as it'll automatically insert SecurityID into the POST data
+		$response = $this->post(
+			'FormTest_ControllerWithSecurityToken/Form',
+			array(
+				'Email' => 'test@test.com',
+				'action_doSubmit' => 1
+				// leaving out security token
+			)
+		);
+		$this->assertEquals(400, $response->getStatusCode(), 'Submission fails without security token');
+		
+		$response = $this->get('FormTest_ControllerWithSecurityToken');
+		$tokenEls = $this->cssParser()->getBySelector('#Form_Form_SecurityID');
+		$this->assertEquals(
+			1, 
+			count($tokenEls), 
+			'Token form field added for controller without disableSecurityToken()'
+		);
+		$token = (string)$tokenEls[0];
+		$response = $this->submitForm(
+			'Form_Form',
+			null,
+			array(
+				'Email' => 'test@test.com',
+				'SecurityID' => $token
+			)
+		);
+		$this->assertEquals(200, $response->getStatusCode(), 'Submission suceeds with security token');
+	}
+	
 	protected function getStubForm() {
 		return new Form(
 			new Controller(),
@@ -310,6 +376,57 @@ class FormTest_Controller extends Controller {
 				'SomeRequiredField'
 			)
 		);
+
+		// Disable CSRF protection for easier form submission handling
+		$form->disableSecurityToken();
+		
+		return $form;
+	}
+	
+	function FormWithSecurityToken() {
+		$form = new Form(
+			$this,
+			'FormWithSecurityToken',
+			new FieldSet(
+				new EmailField('Email')
+			),
+			new FieldSet(
+				new FormAction('doSubmit')
+			)
+		);
+
+		return $form;
+	}
+	
+	function doSubmit($data, $form, $request) {
+		$form->sessionMessage('Test save was successful', 'good');
+		return $this->redirectBack();
+	}
+}
+
+class FormTest_ControllerWithSecurityToken extends Controller {
+	static $url_handlers = array(
+		'$Action//$ID/$OtherID' => "handleAction",
+	);
+
+	protected $template = 'BlankPage';
+	
+	function Link($action = null) {
+		return Controller::join_links('FormTest_ControllerWithSecurityToken', $this->request->latestParam('Action'), $this->request->latestParam('ID'), $action);
+	}
+	
+	function Form() {
+		$form = new Form(
+			$this,
+			'Form',
+			new FieldSet(
+				new EmailField('Email')
+			),
+			new FieldSet(
+				new FormAction('doSubmit')
+			)
+		);
+
 		return $form;
 	}
 	

tests/security/SecurityTokenTest.php

@@ -0,0 +1,145 @@
+<?
+/**
+ * @package sapphire
+ * @subpackage tests
+ */
+class SecurityTokenTest extends SapphireTest {
+	
+	function testIsEnabled() {
+		$inst1 = SecurityToken::inst();
+		$this->assertTrue($inst1->isEnabled());
+		
+		SecurityToken::disable();
+		$inst2 = SecurityToken::inst();
+		$this->assertFalse($inst2->isEnabled());
+		
+		SecurityToken::enable();
+	}
+
+	function testEnableAndDisable() {
+		$inst = SecurityToken::inst();
+		$this->assertFalse($inst->check('randomvalue'));
+
+		SecurityToken::disable();
+		$inst = SecurityToken::inst();
+		$this->assertTrue($inst->check('randomvalue'));
+		
+		SecurityToken::enable();
+		$inst = SecurityToken::inst();
+		$this->assertFalse($inst->check('randomvalue'));
+	}
+	
+	function testIsEnabledStatic() {
+		$this->assertTrue(SecurityToken::is_enabled());
+
+		SecurityToken::disable();
+		$this->assertFalse(SecurityToken::is_enabled());
+		
+		SecurityToken::enable();
+		$this->assertTrue(SecurityToken::is_enabled());
+	}
+
+	function testInst() {
+		$inst1 = SecurityToken::inst();
+		$this->assertType('SecurityToken', $inst1);
+	}
+	
+	function testInstReturnsSingleton() {
+		$inst1 = SecurityToken::inst();
+		$inst2 = SecurityToken::inst();
+		$this->assertEquals($inst1, $inst2);
+	}
+
+	function testCheck() {
+		$t = new SecurityToken();
+
+		$t->setValue(null);
+		$this->assertFalse($t->check('invalidtoken'), 'Any token is invalid if no token is stored');
+
+		$t->setValue(null);
+		$this->assertFalse($t->check(null), 'NULL token is invalid if no token is stored');
+		
+		$t->setValue('mytoken');
+		$this->assertFalse($t->check('invalidtoken'), 'Invalid token returns false');
+		
+		$t->setValue('mytoken');
+		$this->assertTrue($t->check('mytoken'), 'Valid token returns true');
+	}
+	
+	function testCheckRequest() {
+		$t = new SecurityToken();
+		$n = $t->getName();
+
+		$t->setValue(null);
+		$r = new SS_HTTPRequest('GET', 'dummy', array($n => 'invalidtoken'));
+		$this->assertFalse($t->checkRequest($r), 'Any token is invalid if no token is stored');
+
+		$t->setValue(null);
+		$r = new SS_HTTPRequest('GET', 'dummy', array($n => null));
+		$this->assertFalse($t->checkRequest($r), 'NULL token is invalid if no token is stored');
+		
+		$t->setValue('mytoken');
+		$r = new SS_HTTPRequest('GET', 'dummy', array($n => 'invalidtoken'));
+		$this->assertFalse($t->checkRequest($r), 'Invalid token returns false');
+		
+		$t->setValue('mytoken');
+		$r = new SS_HTTPRequest('GET', 'dummy', array($n => 'mytoken'));
+		$this->assertTrue($t->checkRequest($r), 'Valid token returns true');
+	}
+	
+	function testAddToUrl() {
+		$t = new SecurityToken();
+
+		$url = 'http://absolute.tld/action/';
+		$this->assertEquals(
+			sprintf('%s?%s=%s', $url, $t->getName(), $t->getValue()), 
+			$t->addToUrl($url),
+			'Urls without existing GET parameters'
+		);
+		
+		$url = 'http://absolute.tld/?getparam=1';
+		$this->assertEquals(
+			sprintf('%s&%s=%s', $url, $t->getName(), $t->getValue()), 
+			$t->addToUrl($url),
+			'Urls with existing GET parameters'
+		);
+	}
+	
+	function testUpdateFieldSet() {
+		$fs = new FieldSet();
+		$t = new SecurityToken();		
+		$t->updateFieldSet($fs);
+		$f = $fs->dataFieldByName($t->getName());
+		
+		$this->assertType('HiddenField', $f);
+		$this->assertEquals($f->Name(), $t->getName(), 'Name matches');
+		$this->assertEquals($f->Value(), $t->getValue(), 'Value matches');
+	}
+	
+	function testUpdateFieldSetDoesntAddTwice() {
+		$fs = new FieldSet();
+		$t = new SecurityToken();		
+		$t->updateFieldSet($fs); // first
+		$t->updateFieldSet($fs); // second
+		$f = $fs->dataFieldByName($t->getName());
+		
+		$this->assertType('HiddenField', $f);
+		$this->assertEquals(1, $fs->Count());
+	}
+	
+	function testUnnamedTokensCarrySameValue() {
+		$t1 = new SecurityToken();
+		$t2 = new SecurityToken();
+		
+		$this->assertEquals($t1->getName(), $t2->getName());
+		$this->assertEquals($t1->getValue(), $t2->getValue());
+	}
+	
+	function testNamedTokensCarryDifferentValues() {
+		$t1 = new SecurityToken('one');
+		$t2 = new SecurityToken('two');
+		
+		$this->assertNotEquals($t1->getName(), $t2->getName());
+		$this->assertNotEquals($t1->getValue(), $t2->getValue());
+	}
+}