Skip to content

Commit

Permalink
Fix unstable session manager
Browse files Browse the repository at this point in the history
  • Loading branch information
elioermini authored and jignesh-baldha committed Aug 14, 2018
1 parent 05f9df7 commit eee587b
Showing 1 changed file with 36 additions and 1 deletion.
37 changes: 36 additions & 1 deletion lib/internal/Magento/Framework/Session/SessionManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -180,10 +180,21 @@ public function start()
// Need to apply the config options so they can be ready by session_start
$this->initIniOptions();
$this->registerSaveHandler();
if (isset($_SESSION['new_session_id'])) {
// Not fully expired yet. Could be lost cookie by unstable network.
session_commit();

This comment has been minimized.

Copy link
@sshymko

sshymko Aug 20, 2019

Function session_commit() is an alias of session_write_close() encapsulated in $this->writeClose() to be used instead of direct PHP functions.

session_id($_SESSION['new_session_id']);

This comment has been minimized.

Copy link
@sshymko

sshymko Aug 20, 2019

Method $this->setSessionId() should be used instead.

The code contradicts the SID resolver logic below.

}
$sid = $this->sidResolver->getSid($this);
// potential custom logic for session id (ex. switching between hosts)
$this->setSessionId($sid);
session_start();
if (isset($_SESSION['destroyed'])) {
if ($_SESSION['destroyed'] < time() - 300) {

This comment has been minimized.

Copy link
@sshymko

sshymko Aug 20, 2019

Hard-coded delay of 5min should be configurable, for example, as constructor argument.

$this->destroy(['clear_storage' => true]);

}
}
$this->validator->validate($this);
$this->renewCookie($sid);

Expand Down Expand Up @@ -498,7 +509,31 @@ public function regenerateId()
return $this;
}

$this->isSessionExists() ? session_regenerate_id(true) : session_start();
if ($this->isSessionExists()) {
//regenerate the session
session_regenerate_id();
$new_session_id = session_id();

This comment has been minimized.

Copy link
@sshymko

sshymko Aug 20, 2019

Method $this->getSessionId() should be used instead.

Code style: variable name should be in $lowerCamelCase. Copy-pasted from the official docs.


$_SESSION['new_session_id'] = $new_session_id;

// Set destroy timestamp
$_SESSION['destroyed'] = time();

// Write and close current session;
session_commit();

This comment has been minimized.

Copy link
@sshymko

sshymko Aug 20, 2019

Method $this->writeClose() should be used instead of direct PHP functions.

$oldSession = $_SESSION; //called after destroy - see destroy!
// Start session with new session ID
session_id($new_session_id);

This comment has been minimized.

Copy link
@sshymko

sshymko Aug 20, 2019

New session ID had already been assigned by session_regenerate_id() above.

ini_set('session.use_strict_mode', 0);
session_start();
ini_set('session.use_strict_mode', 1);

This comment has been minimized.

Copy link
@sshymko

sshymko Aug 20, 2019

Previous value should be restored instead of forcefully turning on the strict mode.

$_SESSION = $oldSession;
// New session does not need them
unset($_SESSION['destroyed']);
unset($_SESSION['new_session_id']);

This comment has been minimized.

Copy link
@sshymko

sshymko Aug 20, 2019

Data manipulations are unnecessary as $oldSession could be saved before modifying $_SESSION.

For example:

$sessionData = $_SESSION;

session_regenerate_id();
$_SESSION['new_session_id'] = $this->getSessionId();
$_SESSION['destroyed'] = time();
$this->writeClose();

session_start();
$_SESSION = $sessionData;
} else {
session_start();
}
$this->storage->init(isset($_SESSION) ? $_SESSION : []);

if ($this->sessionConfig->getUseCookies()) {
Expand Down

0 comments on commit eee587b

Please sign in to comment.