magneto/magento2#12362 Do not remove session instantly #14484
Conversation
Give configurable time delay to capture session id. Remove old sessions only after new session id cookie was received.
There was a problem hiding this comment.
Hi @swnsma,
Thank you for contribution. Currently, I cannot accept your Pull request for merge. First, the codacy build is red see review comments. The second problem is the static test see https://travis-ci.org/magento/magento2/jobs/360827154
Best regards,
Lars
| * | ||
| * @throws \Magento\Framework\Exception\SessionException | ||
| */ | ||
| private function sessionStart() |
There was a problem hiding this comment.
sessionStart accesses the super-global variable $_SESSION.
https://app.codacy.com/app/shyamranpara/magento2/pullRequest?prid=1494337
There was a problem hiding this comment.
@larsroettig, thank you for the review.
The problem has been fixed.
| /** | ||
| * Regenerate session id, with saving relation between two sessions. | ||
| */ | ||
| protected function regenerateSessionId() |
There was a problem hiding this comment.
regenerateSessionId accesses the super-global variable $_SESSION.
Superglobal variables in PHP should not be accessed directly. Instead of writing:
$name = $_POST['foo'];
You should encapsulate the call in an single class. Most PHP frameworks already wrap Superglobals into a request object so you can use that.
https://app.codacy.com/app/shyamranpara/magento2/pullRequest?prid=1494337
There was a problem hiding this comment.
@larsroettig, thank you for the review.
The problem has been fixed.
| @@ -84,6 +84,7 @@ | |||
| <use_http_x_forwarded_for>0</use_http_x_forwarded_for> | |||
| <use_http_user_agent>0</use_http_user_agent> | |||
| <use_frontend_sid>1</use_frontend_sid> | |||
| <old_session_access_delta>300</old_session_access_delta> | |||
There was a problem hiding this comment.
I don't think putting such a sensitive configuration option into Admin UI is a good idea. 300 seconds is a huge delay, I believe we should pick up some reasonable value.
Ideally solution must not involve any timeout at all allowing just to create new session seamlessly.
There was a problem hiding this comment.
@orlangur, thank you for the review.
I agree that such configuration should not be placed in Admin UI.
About timeout, I have added it to the solution as additional security frontier in session recovery.
Old session will be removed as soon as response with new session id will be received.
And this timeout is for that cases when new session was generated but response with new session id will never be received.
Unfortunately, one of PHP session flaw is that you can not be sure that cookie with session id was delivered to browser or no.
About timeout, I agree that 5m is a huge delay. I have decreased the default number to 30s.
Move session calls to separated and shared session storage. Suppress Warnings.
|
Pull request into 2.2-develop has been created: #14487 |
Make compatible wit HHVM.
Give configurable time delay to capture session id.
Remove old sessions only after new session id cookie was received.
Description
Current pull request contains fix for the issue #12362.
Problem related to specific case of session work in PHP.
In case of call function 'session_regenerate_id(true)' there is required to be sure that renewed cookie will be successfully delivered to the browser.
Current PR provide functionality to keep old session till customer will not receive cookie.
Fixed Issues
Manual testing scenarios
Pre-Conditions
Steps to reproduce
Expected result
Actual result
Contribution checklist