diff --git a/dev/FunctionalTest.php b/dev/FunctionalTest.php
index 9f62c092e..a27820ef5 100644
--- a/dev/FunctionalTest.php
+++ b/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);
}
diff --git a/forms/Form.php b/forms/Form.php
index 40cd03f9d..a01601705 100755
--- a/forms/Form.php
+++ b/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()) {
- $securityID = Session::get('SecurityID');
-
- if(!$securityID || !isset($vars['SecurityID']) || $securityID != $vars['SecurityID']) {
- $this->httpError(400, "SecurityID doesn't match, possible CSRF attack.");
- }
+ $token = $this->getSecurityToken();
+ if(!$token->checkRequest($request)) {
+ $this->httpError(400, "Security token 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()) {
- if(Session::get('SecurityID')) {
- $securityID = Session::get('SecurityID');
- } 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;
- }
+ $token = $this->getSecurityToken();
+ $tokenField = $token->updateFieldSet($this->fields);
+ if($tokenField) $tokenField->setForm($this);
+ $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,34 +1165,51 @@ 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;
+ }
+
/**
* Returns the name of a field, if that's the only field that the current controller is interested in.
* It checks for a call to the callfieldmethod action.
diff --git a/security/SecurityToken.php b/security/SecurityToken.php
new file mode 100644
index 000000000..96035ab5f
--- /dev/null
+++ b/security/SecurityToken.php
@@ -0,0 +1,273 @@
+getSecurityToken()}.
+ *
+ * Usage in forms
+ *
+ * This protective measure is automatically turned on for all new {@link Form} instances,
+ * and can be globally disabled through {@link disable()}.
+ *
+ * Usage in custom controller actions
+ *
+ *
+ * class MyController extends Controller {
+ * function mygetaction($request) {
+ * if(!SecurityToken::inst()->checkRequest($request)) return $this->httpError(400);
+ *
+ * // valid action logic ...
+ * }
+ * }
+ *
+ *
+ * @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:
+ *
+ * $old = SecurityToken::inst(); // isEnabled() returns true
+ * SecurityToken::disable();
+ * $new = SecurityToken::inst(); // isEnabled() returns false
+ *
+ *
+ * @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;
+ }
+
+}
\ No newline at end of file
diff --git a/tests/forms/FormTest.php b/tests/forms/FormTest.php
index 7e08f9e76..b5e9b30fc 100755
--- a/tests/forms/FormTest.php
+++ b/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;
}
diff --git a/tests/security/SecurityTokenTest.php b/tests/security/SecurityTokenTest.php
new file mode 100644
index 000000000..01e2bd927
--- /dev/null
+++ b/tests/security/SecurityTokenTest.php
@@ -0,0 +1,145 @@
+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());
+ }
+}
\ No newline at end of file