Fixed the multi tab issue through unset used token only and set cookie default path / to generate new token while ajax calls
#117
Conversation
…e default path `/` to generate new token while ajax calls
|
@mebjas : Please review, needs to change the test cases based on changes |
| @@ -302,10 +302,11 @@ private static function isValidToken($token) { | |||
|
|
|||
| // Clear all older tokens assuming they have been consumed | |||
| foreach ($_SESSION[self::$config['CSRFP_TOKEN']] as $_key => $_value) { | |||
| if ($_value == $token) break; | |||
| array_shift($_SESSION[self::$config['CSRFP_TOKEN']]); | |||
| if ($_value == $token) { | |||
There was a problem hiding this comment.
If two requests are sent at same time with same token, one of the request might fail because of this.
In the former logic only, even if two request come with same token they are valid. It's only when a request come with more latest token all older ones get invalidated.
There was a problem hiding this comment.
As per logic implemented, token generate based on requests so every request has a unique token and it should not be the same.
There was a problem hiding this comment.
A token is generated when the former token is consumed. Imagine two tabs load path /a at T0. Both with get token1.
Now if Tab1 sends request and it's verified. A new token will be sent. The cookie would be updated and now both Tab1 & Tab2 has same token again.
However, imagine request originating from both tabs at the same time, with same token: token1 and compare the results with current and proposed logic.
| if ($_value == $token) break; | ||
| array_shift($_SESSION[self::$config['CSRFP_TOKEN']]); | ||
| if ($_value == $token) { | ||
| unset($_SESSION[self::$config['CSRFP_TOKEN']][$_key]); |
There was a problem hiding this comment.
Also, with this if multiple tabs get new tokens and only few of them use the token it may pile up data in session variable with no TTL approach to clean it. Unless ofcourse the whole session is destroyed.
| @@ -18,7 +18,7 @@ | |||
| "jsUrl" => "", | |||
| "tokenLength" => 10, | |||
| "cookieConfig" => array( | |||
| "path" => '', | |||
| "path" => '/', | |||
There was a problem hiding this comment.
We have set a path here to generate a new token on every ajax request.
|
OTOH, I have fixed the build failures. Please merge latest code with your fork to see if there are other failures. |
Fixed the multi tab issue through unset used token only and set cookie default path
/to generate new token while ajax callsReference of issue: #116