NEW: Forms with invalid/expired SecurityIDs are repopulated (fixes #1891) #2835

Merged
merged 1 commit into from Feb 11, 2014

Conversation

Projects
None yet
4 participants
@kinglozzer
Member

kinglozzer commented Feb 8, 2014

Will need updated translation entities. Session::set("FormInfo.{$this->FormName()}.errors", array()); is necessary due to how Form::setupFormErrors() works (it won’t load data from the session unless that array is at least present).

@simonwelsh

This comment has been minimized.

Show comment Hide comment
@simonwelsh

simonwelsh Feb 8, 2014

Contributor

Please use a different key instead of changing the content of a translated string.

Contributor

simonwelsh commented Feb 8, 2014

Please use a different key instead of changing the content of a translated string.

@prembo

This comment has been minimized.

Show comment Hide comment
@prembo

prembo Feb 8, 2014

Looking forward to this PR coming through as it's a bit of problem on one site at the moment. Large form, end users losing entered data upon submission if the session expires.

prembo commented Feb 8, 2014

Looking forward to this PR coming through as it's a bit of problem on one site at the moment. Large form, end users losing entered data upon submission if the session expires.

@kinglozzer

This comment has been minimized.

Show comment Hide comment
@kinglozzer

kinglozzer Feb 8, 2014

Member

Thanks @simonwelsh, do I need to add the new entity to the lang files in this pull, or is that a transifex job?

Updated the logic slightly to show the old CSRF message if the token isn’t present at all, instead of being invalid/expired.

Member

kinglozzer commented Feb 8, 2014

Thanks @simonwelsh, do I need to add the new entity to the lang files in this pull, or is that a transifex job?

Updated the logic slightly to show the old CSRF message if the token isn’t present at all, instead of being invalid/expired.

@kinglozzer

This comment has been minimized.

Show comment Hide comment
@kinglozzer

kinglozzer Feb 8, 2014

Member

Not sure why/where the Travis build is getting stuck. If I run all the framework tests locally, PHPUnit gets stuck here in SQLQueryTest->testSelectFirst(). I’ve no idea if Travis is getting stuck in the same place.

Member

kinglozzer commented Feb 8, 2014

Not sure why/where the Travis build is getting stuck. If I run all the framework tests locally, PHPUnit gets stuck here in SQLQueryTest->testSelectFirst(). I’ve no idea if Travis is getting stuck in the same place.

@chillu

View changes

forms/Form.php
@@ -277,10 +277,17 @@ public function httpSubmission($request) {
// Protection against CSRF attacks
$token = $this->getSecurityToken();
- if(!$token->checkRequest($request)) {
+ if($token->isEnabled() && empty($vars['SecurityID'])) {

This comment has been minimized.

Show comment Hide comment
@chillu

chillu Feb 9, 2014

Member

Why the if/else construct, as opposed to just if(!$token->checkRequest($request))? If its needed, we should check if the $vars['SecurityID'] key actually exists, right?

@chillu

chillu Feb 9, 2014

Member

Why the if/else construct, as opposed to just if(!$token->checkRequest($request))? If its needed, we should check if the $vars['SecurityID'] key actually exists, right?

@kinglozzer

This comment has been minimized.

Show comment Hide comment
@kinglozzer

kinglozzer Feb 9, 2014

Member

@chillu Updated to remove the ‘elseif’. Travis is still getting stuck but at least it’s not just me now.

Member

kinglozzer commented Feb 9, 2014

@chillu Updated to remove the ‘elseif’. Travis is still getting stuck but at least it’s not just me now.

- "There seems to have been a technical problem. Please click the back button,"
- . " refresh your browser, and try again."));
+ if( ! $token->checkRequest($request)) {
+ if (empty($vars['SecurityID'])) {

This comment has been minimized.

Show comment Hide comment
@chillu

chillu Feb 11, 2014

Member

This will still cause a fatal error if the SecurityID key isn't defined, right?

@chillu

chillu Feb 11, 2014

Member

This will still cause a fatal error if the SecurityID key isn't defined, right?

This comment has been minimized.

Show comment Hide comment
@kinglozzer

kinglozzer Feb 11, 2014

Member

It’ll return a 400 error if the SecurityID key is not present at all, or is an empty string. Thought that would probably be okay to be left in, it should be impossible to submit the form without a key (unless you’ve been tinkering), right?

@kinglozzer

kinglozzer Feb 11, 2014

Member

It’ll return a 400 error if the SecurityID key is not present at all, or is an empty string. Thought that would probably be okay to be left in, it should be impossible to submit the form without a key (unless you’ve been tinkering), right?

chillu added a commit that referenced this pull request Feb 11, 2014

Merge pull request #2835 from kinglozzer/1891-csrf-friendly-error
NEW: Forms with invalid/expired SecurityIDs are repopulated (fixes #1891)

@chillu chillu merged commit 5e38ef9 into silverstripe:3.1 Feb 11, 2014

1 check passed

default Scrutinizer: No new issues — Travis: Passed
Details
@chillu

This comment has been minimized.

Show comment Hide comment
@chillu

chillu Feb 11, 2014

Member

Thanks Loz!

Member

chillu commented Feb 11, 2014

Thanks Loz!

@kinglozzer kinglozzer deleted the kinglozzer:1891-csrf-friendly-error branch Feb 17, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment