Commit cda287d2 authored by catch's avatar catch

Issue #2238561 by alexpott, dawehner, andypost, bradjones1, ridhimaabrol24,...

Issue #2238561 by alexpott, dawehner, andypost, bradjones1, ridhimaabrol24, jofitz, eiriksm, Darren Oh, kalyansamanta, znerol, neclimdul, catch, pwolanin: Use the default PHP session ID instead of generating a custom one
parent 7f828c96
......@@ -36,6 +36,22 @@ parameters:
# @default none
# cookie_domain: '.example.com'
#
# Set the session ID string length. The length can be between 22 to 256. The
# PHP recommended value is 48. See
# https://www.php.net/manual/session.security.ini.php for more information.
# This value should be kept in sync with
# \Drupal\Core\Session\SessionConfiguration::__construct()
# @default 48
sid_length: 48
#
# Set the number of bits in encoded session ID character. The possible
# values are '4' (0-9, a-f), '5' (0-9, a-v), and '6' (0-9, a-z, A-Z, "-",
# ","). The PHP recommended value is 6. See
# https://www.php.net/manual/session.security.ini.php for more information.
# This value should be kept in sync with
# \Drupal\Core\Session\SessionConfiguration::__construct()
# @default 6
sid_bits_per_character: 6
twig.config:
# Twig debugging:
#
......
......@@ -9,6 +9,8 @@ parameters:
gc_divisor: 100
gc_maxlifetime: 200000
cookie_lifetime: 2000000
sid_length: 48
sid_bits_per_character: 6
twig.config:
debug: false
auto_reload: null
......@@ -1727,7 +1729,7 @@ services:
- { name: backend_overridable }
tempstore.shared:
class: Drupal\Core\TempStore\SharedTempStoreFactory
arguments: ['@keyvalue.expirable', '@lock', '@request_stack', '%tempstore.expire%']
arguments: ['@keyvalue.expirable', '@lock', '@request_stack', '@current_user', '%tempstore.expire%']
tags:
- { name: backend_overridable }
pager.manager:
......
......@@ -664,8 +664,12 @@ function install_run_task($task, &$install_state) {
// @todo Replace this when we refactor the installer to use a request-
// response workflow.
if ($output instanceof Response) {
if (\Drupal::request()->hasSession()) {
\Drupal::request()->getSession()->save();
}
// Send the response.
$output->send();
$output = NULL;
exit;
}
// The task is complete when we try to access the batch page and receive
// FALSE in return, since this means we are at a URL where we are no
......
......@@ -2,6 +2,7 @@
namespace Drupal\Core\Session;
use Drupal\Component\Utility\Crypt;
use Drupal\Core\Site\Settings;
use Symfony\Component\HttpFoundation\Session\Storage\MetadataBag as SymfonyMetadataBag;
......@@ -48,10 +49,22 @@ public function getCsrfTokenSeed() {
}
}
/**
* {@inheritdoc}
*/
public function stampNew($lifetime = NULL) {
parent::stampNew($lifetime);
// Set the token seed immediately to avoid a race condition between two
// simultaneous requests without a seed.
$this->setCsrfTokenSeed(Crypt::randomBytesBase64());
}
/**
* Clear the CSRF token seed.
*/
public function clearCsrfTokenSeed() {
@trigger_error('Calling ' . __METHOD__ . '() is deprecated in drupal:9.2.0 and will be removed in drupal:10.0.0. Use \Drupal\Core\Session\MetadataBag::stampNew() instead. See https://www.drupal.org/node/3187914', E_USER_DEPRECATED);
unset($this->meta[static::CSRF_TOKEN_SEED]);
}
......
......@@ -22,9 +22,12 @@ class SessionConfiguration implements SessionConfigurationInterface {
*
* @see \Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage::__construct()
* @see http://php.net/manual/session.configuration.php
* @see https://www.php.net/manual/session.security.ini.php
*/
public function __construct($options = []) {
$this->options = $options;
// Provide sensible defaults for sid_length and sid_bits_per_character.
// See core/assets/scaffold/files/default.services.yml for more information.
$this->options = $options + ['sid_length' => 48, 'sid_bits_per_character' => 6];
}
/**
......
......@@ -119,17 +119,6 @@ public function start() {
}
if (empty($result)) {
// Randomly generate a session identifier for this request. This is
// necessary because \Drupal\Core\TempStore\SharedTempStoreFactory::get()
// wants to know the future session ID of a lazily started session in
// advance.
//
// @todo: With current versions of PHP there is little reason to generate
// the session id from within application code. Consider using the
// default php session id instead of generating a custom one:
// https://www.drupal.org/node/2238561
$this->setId(Crypt::randomBytesBase64());
// Initialize the session global and attach the Symfony session bags.
$_SESSION = [];
$this->loadSession();
......@@ -145,6 +134,23 @@ public function start() {
return $result;
}
/**
* {@inheritdoc}
*/
public function getId() {
$id = parent::getId();
if (empty($id)) {
// Legacy code might rely on the existence of a session ID before a real
// session exists. In this case, generate a random session ID to provide
// backwards compatibility.
@trigger_error('Calling ' . __METHOD__ . '() outside of an actual existing session is deprecated in drupal:9.2.0 and will be removed in drupal:10.0.0. This is often used for anonymous users. See https://www.drupal.org/node/3006306', E_USER_DEPRECATED);
$id = Crypt::randomBytesBase64();
$this->setId($id);
}
return $id;
}
/**
* Forcibly start a PHP session.
*
......@@ -210,34 +216,21 @@ public function regenerate($destroy = FALSE, $lifetime = NULL) {
return;
}
// We do not support the optional $destroy and $lifetime parameters as long
// as #2238561 remains open.
if ($destroy || isset($lifetime)) {
throw new \InvalidArgumentException('The optional parameters $destroy and $lifetime of SessionManager::regenerate() are not supported currently');
}
if ($this->isStarted()) {
$old_session_id = $this->getId();
// Save and close the old session. Call the parent method to avoid issue
// with session destruction due to the session being considered obsolete.
parent::save();
// Ensure the session is reloaded correctly.
$this->startedLazy = TRUE;
}
session_id(Crypt::randomBytesBase64());
// We set token seed immediately to avoid race condition between two
// simultaneous requests without a seed.
$this->getMetadataBag()->setCsrfTokenSeed(Crypt::randomBytesBase64());
if (isset($old_session_id)) {
$params = session_get_cookie_params();
$expire = $params['lifetime'] ? REQUEST_TIME + $params['lifetime'] : 0;
setcookie($this->getName(), $this->getId(), $expire, $params['path'], $params['domain'], $params['secure'], $params['httponly']);
$this->migrateStoredSession($old_session_id);
// Drupal will always destroy the existing session when regenerating a
// session. This is inline with the recommendations of @link https://cheatsheetseries.owasp.org/cheatsheets/Session_Management_Cheat_Sheet.html#renew-the-session-id-after-any-privilege-level-change
// OWASP session management cheat sheet. @endlink
$destroy = TRUE;
// Cannot regenerate the session ID for non-active sessions.
if (\PHP_SESSION_ACTIVE !== session_status()) {
// Ensure the metadata bag has been stamped. If the parent::regenerate()
// is called prior to the session being started it will not refresh the
// metadata as expected.
$this->getMetadataBag()->stampNew($lifetime);
return FALSE;
}
$this->startNow();
return parent::regenerate($destroy, $lifetime);
}
/**
......@@ -261,6 +254,12 @@ public function destroy() {
return;
}
// Symfony suggests using Session::invalidate() instead of session_destroy()
// however the former calls session_regenerate_id(TRUE), which while
// destroying the current session creates a new ID; Drupal has historically
// decided to only set sessions when absolutely necessary (e.g., to increase
// anonymous user cache hit rates) and as such we cannot use the Symfony
// convenience method here.
session_destroy();
// Unset the session cookies.
......@@ -332,18 +331,4 @@ protected function getSessionDataMask() {
return array_intersect_key($mask, $_SESSION);
}
/**
* Migrates the current session to a new session id.
*
* @param string $old_session_id
* The old session ID. The new session ID is $this->getId().
*/
protected function migrateStoredSession($old_session_id) {
$fields = ['sid' => Crypt::hashBase64($this->getId())];
$this->connection->update('sessions')
->fields($fields)
->condition('sid', Crypt::hashBase64($old_session_id))
->execute();
}
}
......@@ -2,6 +2,7 @@
namespace Drupal\Core\TempStore;
use Drupal\Component\Utility\Crypt;
use Drupal\Core\DependencyInjection\DependencySerializationTrait;
use Drupal\Core\KeyValueStore\KeyValueStoreExpirableInterface;
use Drupal\Core\Lock\LockBackendInterface;
......@@ -119,17 +120,16 @@ public function get($key) {
* Thrown when a lock for the backend storage could not be acquired.
*/
public function set($key, $value) {
// Ensure that an anonymous user has a session created for them, as
// otherwise subsequent page loads will not be able to retrieve their
// tempstore data.
if ($this->currentUser->isAnonymous()) {
// @todo when https://www.drupal.org/node/2865991 is resolved, use force
// start session API rather than setting an arbitrary value directly.
// Ensure that an anonymous user has a session created for them, as
// otherwise subsequent page loads will not be able to retrieve their
// tempstore data. Note this has to be done before the key is created as
// the owner is used in key creation.
$this->startSession();
$this->requestStack
->getCurrentRequest()
->getSession()
->set('core.tempstore.private', TRUE);
$session = $this->requestStack->getCurrentRequest()->getSession();
if (!$session->has('core.tempstore.private.owner')) {
$session->set('core.tempstore.private.owner', Crypt::randomBytesBase64());
}
}
$key = $this->createkey($key);
......@@ -224,8 +224,10 @@ protected function createkey($key) {
protected function getOwner() {
$owner = $this->currentUser->id();
if ($this->currentUser->isAnonymous()) {
// Check to see if an owner key exists in the session.
$this->startSession();
$owner = $this->requestStack->getCurrentRequest()->getSession()->getId();
$session = $this->requestStack->getCurrentRequest()->getSession();
$owner = $session->get('core.tempstore.private.owner');
}
return $owner;
}
......
......@@ -5,6 +5,7 @@
use Drupal\Core\DependencyInjection\DependencySerializationTrait;
use Drupal\Core\KeyValueStore\KeyValueStoreExpirableInterface;
use Drupal\Core\Lock\LockBackendInterface;
use Drupal\Core\Session\AccountProxyInterface;
use Symfony\Component\HttpFoundation\RequestStack;
/**
......@@ -68,6 +69,13 @@ class SharedTempStore {
*/
protected $owner;
/**
* The current user.
*
* @var \Drupal\Core\Session\AccountProxyInterface
*/
protected $currentUser;
/**
* The time to live for items in seconds.
*
......@@ -90,14 +98,26 @@ class SharedTempStore {
* The owner key to store along with the data (e.g. a user or session ID).
* @param \Symfony\Component\HttpFoundation\RequestStack $request_stack
* The request stack.
* @param \Drupal\Core\Session\AccountProxyInterface $current_user
* The current user.
* @param int $expire
* The time to live for items, in seconds.
*/
public function __construct(KeyValueStoreExpirableInterface $storage, LockBackendInterface $lock_backend, $owner, RequestStack $request_stack, $expire = 604800) {
public function __construct(KeyValueStoreExpirableInterface $storage, LockBackendInterface $lock_backend, $owner, RequestStack $request_stack, $current_user = NULL, $expire = 604800) {
$this->storage = $storage;
$this->lockBackend = $lock_backend;
$this->owner = $owner;
$this->requestStack = $request_stack;
if (!$current_user instanceof AccountProxyInterface) {
@trigger_error('Calling ' . __METHOD__ . '() without the $current_user argument is deprecated in drupal:9.2.0 and will be required in drupal:10.0.0. See https://www.drupal.org/node/3006268', E_USER_DEPRECATED);
if (is_int($current_user)) {
// If the $current_user argument is numeric then this object has been
// instantiated with the old constructor signature.
$expire = $current_user;
}
$current_user = \Drupal::currentUser();
}
$this->currentUser = $current_user;
$this->expire = $expire;
}
......@@ -150,7 +170,9 @@ public function setIfNotExists($key, $value) {
'data' => $value,
'updated' => (int) $this->requestStack->getMasterRequest()->server->get('REQUEST_TIME'),
];
return $this->storage->setWithExpireIfNotExists($key, $value, $this->expire);
$this->ensureAnonymousSession();
$set = $this->storage->setWithExpireIfNotExists($key, $value, $this->expire);
return $set;
}
/**
......@@ -208,6 +230,7 @@ public function set($key, $value) {
'data' => $value,
'updated' => (int) $this->requestStack->getMasterRequest()->server->get('REQUEST_TIME'),
];
$this->ensureAnonymousSession();
$this->storage->setWithExpire($key, $value, $this->expire);
$this->lockBackend->release($key);
}
......@@ -279,4 +302,17 @@ public function deleteIfOwner($key) {
return FALSE;
}
/**
* Stores the owner in the session if the user is anonymous.
*
* This method should be called when a value is set.
*/
protected function ensureAnonymousSession() {
// If this is being run from the CLI then the request will not have a
// session.
if ($this->currentUser->isAnonymous() && $this->requestStack->getCurrentRequest()->hasSession()) {
$this->requestStack->getCurrentRequest()->getSession()->set('core.tempstore.shared.owner', $this->owner);
}
}
}
......@@ -2,8 +2,10 @@
namespace Drupal\Core\TempStore;
use Drupal\Component\Utility\Crypt;
use Drupal\Core\KeyValueStore\KeyValueExpirableFactoryInterface;
use Drupal\Core\Lock\LockBackendInterface;
use Drupal\Core\Session\AccountProxyInterface;
use Symfony\Component\HttpFoundation\RequestStack;
/**
......@@ -32,6 +34,13 @@ class SharedTempStoreFactory {
*/
protected $requestStack;
/**
* The current user.
*
* @var \Drupal\Core\Session\AccountProxyInterface
*/
protected $currentUser;
/**
* The time to live for items in seconds.
*
......@@ -48,13 +57,25 @@ class SharedTempStoreFactory {
* The lock object used for this data.
* @param \Symfony\Component\HttpFoundation\RequestStack $request_stack
* The request stack.
* @param \Drupal\Core\Session\AccountProxyInterface $current_user
* The current user.
* @param int $expire
* The time to live for items, in seconds.
*/
public function __construct(KeyValueExpirableFactoryInterface $storage_factory, LockBackendInterface $lock_backend, RequestStack $request_stack, $expire = 604800) {
public function __construct(KeyValueExpirableFactoryInterface $storage_factory, LockBackendInterface $lock_backend, RequestStack $request_stack, $current_user = NULL, $expire = 604800) {
$this->storageFactory = $storage_factory;
$this->lockBackend = $lock_backend;
$this->requestStack = $request_stack;
if (!$current_user instanceof AccountProxyInterface) {
@trigger_error('Calling ' . __METHOD__ . '() without the $current_user argument is deprecated in drupal:9.2.0 and will be required in drupal:10.0.0. See https://www.drupal.org/node/3006268', E_USER_DEPRECATED);
if (is_int($current_user)) {
// If the $current_user argument is numeric then this object has been
// instantiated with the old constructor signature.
$expire = $current_user;
}
$current_user = \Drupal::currentUser();
}
$this->currentUser = $current_user;
$this->expire = $expire;
}
......@@ -76,12 +97,20 @@ public function get($collection, $owner = NULL) {
// Use the currently authenticated user ID or the active user ID unless
// the owner is overridden.
if (!isset($owner)) {
$owner = \Drupal::currentUser()->id() ?: session_id();
$owner = $this->currentUser->id();
if ($this->currentUser->isAnonymous()) {
$owner = Crypt::randomBytesBase64();
if ($this->requestStack->getCurrentRequest()->hasSession()) {
// Store a random identifier for anonymous users if the session is
// available.
$owner = $this->requestStack->getCurrentRequest()->getSession()->get('core.tempstore.shared.owner', $owner);
}
}
}
// Store the data for this collection in the database.
$storage = $this->storageFactory->get("tempstore.shared.$collection");
return new SharedTempStore($storage, $this->lockBackend, $owner, $this->requestStack, $this->expire);
return new SharedTempStore($storage, $this->lockBackend, $owner, $this->requestStack, $this->currentUser, $this->expire);
}
}
......@@ -1130,6 +1130,7 @@ overrider's
overriders
overridetest
overwritable
owasp
pageable
pagecache
pagetop
......
......@@ -476,6 +476,11 @@ public function testUserDelete() {
$user_storage->resetCache([$account->id()]);
$this->assertNull($user_storage->load($account->id()), 'User is not found in the database.');
// Confirm there's only one session in the database. The user will be logged
// out and their session migrated.
// @see _user_cancel_session_regenerate()
$this->assertSame(1, (int) \Drupal::database()->select('sessions', 's')->countQuery()->execute()->fetchField());
// Confirm that user's content has been deleted.
$node_storage->resetCache([$node->id()]);
$this->assertNull($node_storage->load($node->id()), 'Node of the user has been deleted.');
......
......@@ -79,6 +79,11 @@ public function testUserEdit() {
$this->drupalPostForm("user/" . $user1->id() . "/edit", $edit, 'Save');
$this->assertRaw(t("The changes have been saved."));
// Confirm there's only one session in the database as the existing session
// has been migrated when the password is changed.
// @see \Drupal\user\Entity\User::postSave()
$this->assertSame(1, (int) \Drupal::database()->select('sessions', 's')->countQuery()->execute()->fetchField());
// Make sure the changed timestamp is updated.
$this->assertEqual($user1->getChangedTime(), REQUEST_TIME, 'Changing a user sets "changed" timestamp.');
......
......@@ -3,6 +3,7 @@
namespace Drupal\KernelTests\Core\TempStore;
use Drupal\Core\KeyValueStore\KeyValueExpirableFactory;
use Drupal\Core\Session\AccountProxyInterface;
use Drupal\KernelTests\KernelTestBase;
use Drupal\Core\TempStore\SharedTempStoreFactory;
use Drupal\Core\Lock\DatabaseLockBackend;
......@@ -28,7 +29,14 @@ public function testSharedTempStore() {
// Create a key/value collection.
$database = Database::getConnection();
$factory = new SharedTempStoreFactory(new KeyValueExpirableFactory(\Drupal::getContainer()), new DatabaseLockBackend($database), $this->container->get('request_stack'));
// Mock the current user service so that isAnonymous returns FALSE.
$current_user = $this->prophesize(AccountProxyInterface::class);
$factory = new SharedTempStoreFactory(
new KeyValueExpirableFactory(\Drupal::getContainer()),
new DatabaseLockBackend($database),
$this->container->get('request_stack'),
$current_user->reveal()
);
$collection = $this->randomMachineName();
// Create two mock users.
......
<?php
namespace Drupal\Tests\Core\Session;
use Drupal\Core\Session\MetadataBag;
use Drupal\Core\Site\Settings;
use Drupal\Tests\UnitTestCase;
/**
* @coversDefaultClass \Drupal\Core\Session\MetadataBag
* @group Session
*/
class MetadataBagTest extends UnitTestCase {
/**
* @covers ::stampNew
*/
public function testStampNew() {
$metadata = new MetadataBag(new Settings([]));
$metadata->setCsrfTokenSeed('a_cryptographically_secure_long_random_string_should_used_here');
$metadata->stampNew();
$this->assertNotEquals('a_cryptographically_secure_long_random_string_should_used_here', $metadata->getCsrfTokenSeed());
}
/**
* @covers ::clearCsrfTokenSeed
* @group legacy
*/
public function testDeprecatedClearCsrfTokenSeed() {
$this->expectDeprecation('Calling Drupal\Core\Session\MetadataBag::clearCsrfTokenSeed() is deprecated in drupal:9.2.0 and will be removed in drupal:10.0.0. Use \Drupal\Core\Session\MetadataBag::stampNew() instead. See https://www.drupal.org/node/3187914');
$metadata = new MetadataBag(new Settings([]));
$metadata->clearCsrfTokenSeed();
}
}
......@@ -246,4 +246,33 @@ public function providerTestEnforcedSessionName() {
}, $data);
}
/**
* Tests constructor's default settings.
*
* @covers ::__construct
*
* @dataProvider providerTestConstructorDefaultSettings
*/
public function testConstructorDefaultSettings(array $options, int $expected_sid_length, int $expected_sid_bits_per_character) {
$config = $this->createSessionConfiguration($options);
$options = $config->getOptions(Request::createFromGlobals());
$this->assertSame($expected_sid_length, $options['sid_length']);
$this->assertSame($expected_sid_bits_per_character, $options['sid_bits_per_character']);
}
/**
* Data provider for the constructor test.
*
* @returns array
* Test data
*/
public function providerTestConstructorDefaultSettings() {
return [
[[], 48, 6],
[['sid_length' => 100], 100, 6],
[['sid_bits_per_character' => 5], 48, 5],
[['sid_length' => 100, 'sid_bits_per_character' => 5], 100, 5],
];
}
}
<?php
namespace Drupal\Tests\Core\Session;
use Drupal\Core\Database\Connection;
use Drupal\Core\Session\MetadataBag;
use Drupal\Core\Session\SessionConfigurationInterface;
use Drupal\Core\Session\SessionManager;
use Drupal\Core\Site\Settings;
use Drupal\Tests\UnitTestCase;
use Symfony\Component\HttpFoundation\RequestStack;
use Symfony\Component\HttpFoundation\Session\Storage\Proxy\AbstractProxy;
/**
* @coversDefaultClass \Drupal\Core\Session\SessionManager
* @group Session
*/
class SessionManagerTest extends UnitTestCase {
/**
* @group legacy
*/
public function testGetIdWithoutSession() {
$connection = $this->createMock(Connection::class);
$session_configuration = $this->createMock(SessionConfigurationInterface::class);
$session_manager = new SessionManager(
new RequestStack(),
$connection,
new MetadataBag(new Settings([])),
$session_configuration,
new FakeAbstractProxy()
);
$this->expectDeprecation('Calling Drupal\Core\Session\SessionManager::getId() outside of an actual existing session is deprecated in drupal:9.2.0 and will be removed in drupal:10.0.0. This is often used for anonymous users. See https://www.drupal.org/node/3006306');
$this->assertNotEmpty($session_manager->getId());
}
}
class FakeAbstractProxy extends AbstractProxy {
/**
* Stores the fake session ID.
*
* @var string
*/
protected $id;
public function setId($id) {
$this->id = $id;
}
public function getId() {
return $this->id;
}
}
......@@ -2,13 +2,18 @@
namespace Drupal\Tests\Core\TempStore;
use Drupal\Core\DependencyInjection\ContainerBuilder;
use Drupal\Core\KeyValueStore\KeyValueExpirableFactoryInterface;