Skip to content

Commit

Permalink
Avoid session invalidation when changing session lifetime. (#546)
Browse files Browse the repository at this point in the history
  • Loading branch information
@colinmollenhour
colinmollenhour authored Jun 10, 2022
1 parent 14cf6fa commit 447e27e
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 51 deletions.
92 changes: 48 additions & 44 deletions app/code/core/Mage/Core/Model/Session/Abstract/Varien.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ class Mage_Core_Model_Session_Abstract_Varien extends Varien_Object
const VALIDATOR_HTTP_VIA_KEY = 'http_via';
const VALIDATOR_REMOTE_ADDR_KEY = 'remote_addr';
const VALIDATOR_SESSION_EXPIRE_TIMESTAMP = 'session_expire_timestamp';
const VALIDATOR_SESSION_RENEW_TIMESTAMP = 'session_renew_timestamp';
const VALIDATOR_SESSION_LIFETIME = 'session_lifetime';
const VALIDATOR_PASSWORD_CREATE_TIMESTAMP = 'password_create_timestamp';
const SECURE_COOKIE_CHECK_KEY = '_secure_cookie_check';

Expand Down Expand Up @@ -149,46 +151,46 @@ public function start($sessionName = null)

session_start();

if (Mage::app()->getFrontController()->getRequest()->isSecure() && empty($cookieParams['secure'])) {
// secure cookie check to prevent MITM attack
$secureCookieName = $sessionName . '_cid';
if (isset($_SESSION[self::SECURE_COOKIE_CHECK_KEY])) {
$cookieValue = $cookie->get($secureCookieName);

// Migrate old cookie from 'frontend'
if ( ! $cookieValue
&& $sessionName === \Mage_Core_Controller_Front_Action::SESSION_NAMESPACE
&& $cookie->get('frontend_cid')
&& ! $cookie->get($secureCookieName)
) {
$frontendValue = $cookie->get('frontend_cid');
$_COOKIE[$secureCookieName] = $frontendValue;
$cookie->set($secureCookieName, $frontendValue);
$cookie->delete('frontend_cid');
}
Mage::dispatchEvent('session_before_renew_cookie', ['cookie' => $cookie]);

if (!is_string($cookieValue) || $_SESSION[self::SECURE_COOKIE_CHECK_KEY] !== md5($cookieValue)) {
session_regenerate_id(false);
$sessionHosts = $this->getSessionHosts();
$currentCookieDomain = $cookie->getDomain();
foreach (array_keys($sessionHosts) as $host) {
// Delete cookies with the same name for parent domains
if (strpos($currentCookieDomain, $host) > 0) {
$cookie->delete($this->getSessionName(), null, $host);
}
}
$_SESSION = array();
} else {
/**
* Renew secure cookie expiration time if secure id did not change
*/
$cookie->renew($secureCookieName, null, null, null, true, true);
}
// Secure cookie check to prevent MITM attack
if (Mage::app()->getFrontController()->getRequest()->isSecure() && empty($cookieParams['secure'])) {
$secureCookieName = $this->getSessionName() . '_cid';
$cookieValue = $cookie->get($secureCookieName);

// Migrate old cookie from 'frontend'
if ( ! $cookieValue
&& $sessionName === \Mage_Core_Controller_Front_Action::SESSION_NAMESPACE
&& $cookie->get('frontend_cid')
) {
$cookieValue = $cookie->get('frontend_cid');
$_COOKIE[$secureCookieName] = $cookieValue;
$cookie->set($secureCookieName, $cookieValue);
$cookie->delete('frontend_cid');
}

// Set secure cookie check value in session if not yet set
if (!isset($_SESSION[self::SECURE_COOKIE_CHECK_KEY])) {
$checkId = Mage::helper('core')->getRandomString(16);
$cookie->set($secureCookieName, $checkId, null, null, null, true, true);
$_SESSION[self::SECURE_COOKIE_CHECK_KEY] = md5($checkId);
$cookieValue = Mage::helper('core')->getRandomString(16);
$cookie->set($secureCookieName, $cookieValue, null, null, null, true, true);
$_SESSION[self::SECURE_COOKIE_CHECK_KEY] = md5($cookieValue);
}
// Renew secret check value cookie if it is valid
else if (is_string($cookieValue) && $_SESSION[self::SECURE_COOKIE_CHECK_KEY] === md5($cookieValue)) {
$cookie->renew($secureCookieName, null, null, null, true, true);
}
// Secure cookie check value is invalid, regenerate session
else {
session_regenerate_id(false);
$sessionHosts = $this->getSessionHosts();
$currentCookieDomain = $cookie->getDomain();
foreach (array_keys($sessionHosts) as $host) {
// Delete cookies with the same name for parent domains
if (strpos($currentCookieDomain, $host) > 0) {
$cookie->delete($this->getSessionName(), null, $host);
}
}
$_SESSION = array();
}
}

Expand Down Expand Up @@ -467,7 +469,8 @@ public function validate()

// Refresh expire timestamp
if ($this->useValidateSessionExpire()) {
$_SESSION[self::VALIDATOR_KEY][self::VALIDATOR_SESSION_EXPIRE_TIMESTAMP] = time() + $this->getCookie()->getLifetime();
$_SESSION[self::VALIDATOR_KEY][self::VALIDATOR_SESSION_RENEW_TIMESTAMP] = time();
$_SESSION[self::VALIDATOR_KEY][self::VALIDATOR_SESSION_LIFETIME] = $this->getCookie()->getLifetime();
}
}

Expand Down Expand Up @@ -511,15 +514,18 @@ protected function _validate()
}

if ($this->useValidateSessionExpire()
&& isset($sessionData[self::VALIDATOR_SESSION_EXPIRE_TIMESTAMP])
&& $sessionData[self::VALIDATOR_SESSION_EXPIRE_TIMESTAMP] < time() ) {
&& isset($sessionData[self::VALIDATOR_SESSION_RENEW_TIMESTAMP])
&& isset($sessionData[self::VALIDATOR_SESSION_LIFETIME])
&& ((int)$sessionData[self::VALIDATOR_SESSION_RENEW_TIMESTAMP] + (int)$sessionData[self::VALIDATOR_SESSION_LIFETIME])
< time()
) {
return false;
}
if ($this->useValidateSessionPasswordTimestamp()
&& isset($validatorData[self::VALIDATOR_PASSWORD_CREATE_TIMESTAMP])
&& isset($sessionData[self::VALIDATOR_SESSION_EXPIRE_TIMESTAMP])
&& isset($sessionData[self::VALIDATOR_SESSION_RENEW_TIMESTAMP])
&& $validatorData[self::VALIDATOR_PASSWORD_CREATE_TIMESTAMP]
> $sessionData[self::VALIDATOR_SESSION_EXPIRE_TIMESTAMP] - $this->getCookie()->getLifetime()

This comment has been minimized.

Sign in to view

Copy link
@BrianPittVP

BrianPittVP Aug 23, 2022

Why did you remove the part that subtracts the cookie lifetime from the session expiration? This, combined with an existing Magento timestamp bug, resulted in me not being able to log into a newly created account until 4 hours had passed (that's my UTC offset)

This comment has been minimized.

Sign in to view

Copy link
@kiatng

kiatng Aug 24, 2022

Contributor

@BrianPittVP Can you create an issue so that the community can address it better?

This comment has been minimized.

Sign in to view

Copy link
@BrianPittVP

BrianPittVP Aug 24, 2022

Sure, my apologies, I don't know the protocol :)
> $sessionData[self::VALIDATOR_SESSION_RENEW_TIMESTAMP]
) {
return false;
}
Expand Down Expand Up @@ -557,8 +563,6 @@ public function getValidatorData()
$parts[self::VALIDATOR_HTTP_USER_AGENT_KEY] = (string)$_SERVER['HTTP_USER_AGENT'];
}

$parts[self::VALIDATOR_SESSION_EXPIRE_TIMESTAMP] = time() + $this->getCookie()->getLifetime();

if (isset($this->_data['visitor_data']['customer_id'])) {
$parts[self::VALIDATOR_PASSWORD_CREATE_TIMESTAMP] =
Mage::helper('customer')->getPasswordTimestamp($this->_data['visitor_data']['customer_id']);
Expand Down
8 changes: 1 addition & 7 deletions app/code/core/Mage/Customer/Helper/Data.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -750,13 +750,7 @@ public function getVatValidationUserMessage($customerAddress, $customerGroupAuto
*/
public function getPasswordTimestamp($customerId)
{
/** @var Mage_Customer_Model_Customer $customer */
$customer = Mage::getModel('customer/customer')
->setWebsiteId(Mage::app()->getStore()->getWebsiteId())
->load((int)$customerId);
$passwordCreatedAt = $customer->getPasswordCreatedAt();

return is_null($passwordCreatedAt) ? $customer->getCreatedAtTimestamp() : $passwordCreatedAt;
return Mage::getResourceModel('customer/customer')->getPasswordTimestamp($customerId);
}

/**
Expand Down
19 changes: 19 additions & 0 deletions app/code/core/Mage/Customer/Model/Resource/Customer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -365,4 +365,23 @@ public function changeResetPasswordLinkCustomerId(
}
return $this;
}

/**
* Get password created at timestamp for a customer by id
*
* @param int $customerId
* @return string
*/
public function getPasswordTimestamp($customerId)
{
$field = $this->_getReadAdapter()->getIfNullSql('password_created_at', 'created_at');
$select = $this->_getReadAdapter()->select()
->from($this->getEntityTable(), ['password_created_at' => $field])
->where($this->getEntityIdField() . ' =?', $customerId);
$value = $this->_getReadAdapter()->fetchOne($select);
if ($value && ! is_numeric($value)) { // Convert created_at string to unix timestamp
$value = Varien_Date::toTimestamp($value);
}
return $value;
}
}

0 comments on commit 447e27e

Please sign in to comment.