Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed the multi tab issue through unset used token only and set cookie default path / to generate new token while ajax calls #117

Closed
wants to merge 3 commits into from
Closed

Fixed the multi tab issue through unset used token only and set cookie default path / to generate new token while ajax calls #117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2fcbd81
Fixed the multi tab issue through unset used token only and set cooki…
bhavinrshah Jul 9, 2019
29c22fa
Updated test cases to check the unset token in the session.
bhavinrshah Jul 9, 2019
7f379e4
Updated test cases to fixed the failed scenario
bhavinrshah Jul 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion libs/config.sample.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
"jsUrl" => "",
"tokenLength" => 10,
"cookieConfig" => array(
"path" => '',
"path" => '/',
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's expectation here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have set a path here to generate a new token on every ajax request.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean how does '/' helps?

"domain" => '',
"secure" => false,
"expire" => '',
Expand Down
7 changes: 4 additions & 3 deletions libs/csrf/csrfprotector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per logic implemented, token generate based on requests so every request has a unique token and it should not be the same.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

unset($_SESSION[self::$config['CSRFP_TOKEN']][$_key]);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return true;
}
}
return true;
}
}

Expand Down
6 changes: 3 additions & 3 deletions test/csrfprotector_test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,7 +396,7 @@ public function testAuthorisePost_success()
$temp = $_SESSION[csrfprotector::$config['CSRFP_TOKEN']];

csrfprotector::authorizePost(); //will create new session and cookies
$this->assertFalse($temp == $_SESSION[csrfprotector::$config['CSRFP_TOKEN']][0]);
$this->assertTrue(!isset($_SESSION[csrfprotector::$config['CSRFP_TOKEN']][0]));
$this->assertTrue(csrfp_wrapper::checkHeader('Set-Cookie'));
$this->assertTrue(csrfp_wrapper::checkHeader('csrfp_token'));
// $this->assertTrue(csrfp_wrapper::checkHeader($_SESSION[csrfprotector::$config['CSRFP_TOKEN']][0])); // Combine these 3 later
Expand All @@ -406,7 +406,7 @@ public function testAuthorisePost_success()
csrfp_wrapper::changeRequestType('GET');
$_POST[csrfprotector::$config['CSRFP_TOKEN']]
= $_GET[csrfprotector::$config['CSRFP_TOKEN']]
= $_SESSION[csrfprotector::$config['CSRFP_TOKEN']][0];
= $_SESSION[csrfprotector::$config['CSRFP_TOKEN']][1];
$temp = $_SESSION[csrfprotector::$config['CSRFP_TOKEN']];

csrfprotector::authorizePost(); //will create new session and cookies
Expand Down Expand Up @@ -437,7 +437,7 @@ public function testAuthorisePost_success_2()
$temp = $_SESSION[csrfprotector::$config['CSRFP_TOKEN']];

csrfprotector::authorizePost(); //will create new session and cookies
$this->assertFalse($temp == $_SESSION[csrfprotector::$config['CSRFP_TOKEN']][0]);
$this->assertTrue(!isset($_SESSION[csrfprotector::$config['CSRFP_TOKEN']][0]));
$this->assertTrue(csrfp_wrapper::checkHeader('Set-Cookie'));
$this->assertTrue(csrfp_wrapper::checkHeader('csrfp_token'));
// $this->assertTrue(csrfp_wrapper::checkHeader($_SESSION[csrfprotector::$config['CSRFP_TOKEN']][0])); // Combine these 3 later
Expand Down