diff --git a/core/core.services.yml b/core/core.services.yml index e5d19b1ae49bfe8e0a3a02c8a1c06cd50b5c74f0..f8c428f2d5cfe504fa233eed667b603e3ea4f48e 100644 --- a/core/core.services.yml +++ b/core/core.services.yml @@ -659,7 +659,7 @@ services: arguments: ['@state'] csrf_token: class: Drupal\Core\Access\CsrfTokenGenerator - arguments: ['@private_key'] + arguments: ['@private_key', '@session_manager.metadata_bag'] access_arguments_resolver: class: Drupal\Core\Access\AccessArgumentsResolver access_manager: @@ -823,7 +823,7 @@ services: arguments: ['@module_handler', '@cache.discovery', '@language_manager'] batch.storage: class: Drupal\Core\Batch\BatchStorage - arguments: ['@database'] + arguments: ['@database', '@session_manager', '@csrf_token'] tags: - { name: backend_overridable } replica_database_ignore__subscriber: diff --git a/core/lib/Drupal/Core/Access/CsrfTokenGenerator.php b/core/lib/Drupal/Core/Access/CsrfTokenGenerator.php index 7d38ba05869d3b4130f2b62d8b94465470bfeb65..083dd6cff4fc8e536f7a3ae999c23f6465f2b6f6 100644 --- a/core/lib/Drupal/Core/Access/CsrfTokenGenerator.php +++ b/core/lib/Drupal/Core/Access/CsrfTokenGenerator.php @@ -9,7 +9,7 @@ use Drupal\Component\Utility\Crypt; use Drupal\Core\PrivateKey; -use Drupal\Core\Session\AccountInterface; +use Drupal\Core\Session\MetadataBag; use Drupal\Core\Site\Settings; /** @@ -26,14 +26,24 @@ class CsrfTokenGenerator { */ protected $privateKey; + /** + * The session metadata bag. + * + * @var \Drupal\Core\Session\MetadataBag + */ + protected $sessionMetadata; + /** * Constructs the token generator. * * @param \Drupal\Core\PrivateKey $private_key * The private key service. + * @param \Drupal\Core\Session\MetadataBag $session_metadata + * The session metadata bag. */ - public function __construct(PrivateKey $private_key) { + public function __construct(PrivateKey $private_key, MetadataBag $session_metadata) { $this->privateKey = $private_key; + $this->sessionMetadata = $session_metadata; } /** @@ -56,11 +66,13 @@ public function __construct(PrivateKey $private_key) { * @see \Drupal\Core\Session\SessionManager::start() */ public function get($value = '') { - if (empty($_SESSION['csrf_token_seed'])) { - $_SESSION['csrf_token_seed'] = Crypt::randomBytesBase64(); + $seed = $this->sessionMetadata->getCsrfTokenSeed(); + if (empty($seed)) { + $seed = Crypt::randomBytesBase64(); + $this->sessionMetadata->setCsrfTokenSeed($seed); } - return $this->computeToken($_SESSION['csrf_token_seed'], $value); + return $this->computeToken($seed, $value); } /** @@ -75,11 +87,12 @@ public function get($value = '') { * TRUE for a valid token, FALSE for an invalid token. */ public function validate($token, $value = '') { - if (empty($_SESSION['csrf_token_seed'])) { + $seed = $this->sessionMetadata->getCsrfTokenSeed(); + if (empty($seed)) { return FALSE; } - return $token === $this->computeToken($_SESSION['csrf_token_seed'], $value); + return $token === $this->computeToken($seed, $value); } /** diff --git a/core/lib/Drupal/Core/Batch/BatchStorage.php b/core/lib/Drupal/Core/Batch/BatchStorage.php index 851fe00a1a19224e7795df31bec4f2009791cb68..da0fe94764d0f5356233a10ab758b571770ac486 100644 --- a/core/lib/Drupal/Core/Batch/BatchStorage.php +++ b/core/lib/Drupal/Core/Batch/BatchStorage.php @@ -8,25 +8,57 @@ namespace Drupal\Core\Batch; use Drupal\Core\Database\Connection; +use Drupal\Core\Session\SessionManager; +use Drupal\Core\Access\CsrfTokenGenerator; class BatchStorage implements BatchStorageInterface { /** + * The database connection. + * * @var \Drupal\Core\Database\Connection */ protected $connection; - public function __construct(Connection $connection) { + /** + * The session manager. + * + * @var \Drupal\Core\Session\SessionManager + */ + protected $sessionManager; + + /** + * The CSRF token generator. + * + * @var \Drupal\Core\Access\CsrfTokenGenerator + */ + protected $csrfToken; + + /** + * Constructs the database batch storage service. + * + * @param \Drupal\Core\Database\Connection $connection + * The database connection. + * @param \Drupal\Core\Session\SessionManager $session_manager + * The session manager. + * @param \Drupal\Core\Access\CsrfTokenGenerator $csrf_token + * The CSRF token generator. + */ + public function __construct(Connection $connection, SessionManager $session_manager, CsrfTokenGenerator $csrf_token) { $this->connection = $connection; + $this->sessionManager = $session_manager; + $this->csrfToken = $csrf_token; } /** * {@inheritdoc} */ public function load($id) { + // Ensure that a session is started before using the CSRF token generator. + $this->sessionManager->start(); $batch = $this->connection->query("SELECT batch FROM {batch} WHERE bid = :bid AND token = :token", array( ':bid' => $id, - ':token' => \Drupal::csrfToken()->get($id), + ':token' => $this->csrfToken->get($id), ))->fetchField(); if ($batch) { return unserialize($batch); @@ -66,14 +98,17 @@ public function cleanup() { /** * {@inheritdoc} */ - function create(array $batch) { + public function create(array $batch) { + // Ensure that a session is started before using the CSRF token generator. + $this->sessionManager->start(); $this->connection->insert('batch') ->fields(array( 'bid' => $batch['id'], 'timestamp' => REQUEST_TIME, - 'token' => \Drupal::csrfToken()->get($batch['id']), + 'token' => $this->csrfToken->get($batch['id']), 'batch' => serialize($batch), )) ->execute(); } + } diff --git a/core/lib/Drupal/Core/Session/MetadataBag.php b/core/lib/Drupal/Core/Session/MetadataBag.php index 164062b1f68349ce88707c606f9ef505b0f6b52f..4b44739204e8883b6903267ca44d2064bc309a15 100644 --- a/core/lib/Drupal/Core/Session/MetadataBag.php +++ b/core/lib/Drupal/Core/Session/MetadataBag.php @@ -15,6 +15,11 @@ */ class MetadataBag extends SymfonyMetadataBag { + /** + * The key used to store the CSRF token seed in the session. + */ + const CSRF_TOKEN_SEED = 's'; + /** * Constructs a new metadata bag instance. * @@ -26,4 +31,33 @@ public function __construct(Settings $settings) { parent::__construct('_sf2_meta', $update_threshold); } + /** + * Set the CSRF token seed. + * + * @param string $csrf_token_seed + * The per-session CSRF token seed. + */ + public function setCsrfTokenSeed($csrf_token_seed) { + $this->meta[static::CSRF_TOKEN_SEED] = $csrf_token_seed; + } + + /** + * Get the CSRF token seed. + * + * @return string|null + * The per-session CSRF token seed or null when no value is set. + */ + public function getCsrfTokenSeed() { + if (isset($this->meta[static::CSRF_TOKEN_SEED])) { + return $this->meta[static::CSRF_TOKEN_SEED]; + } + } + + /** + * Clear the CSRF token seed. + */ + public function clearCsrfTokenSeed() { + unset($this->meta[static::CSRF_TOKEN_SEED]); + } + } diff --git a/core/lib/Drupal/Core/Session/SessionManager.php b/core/lib/Drupal/Core/Session/SessionManager.php index df6cdee90bd8bf050b8b49f7ad743daedcfcacad..31989e69bd0d957883c216be289fa12f91e856e7 100644 --- a/core/lib/Drupal/Core/Session/SessionManager.php +++ b/core/lib/Drupal/Core/Session/SessionManager.php @@ -14,7 +14,6 @@ use Drupal\Core\Site\Settings; use Symfony\Component\HttpFoundation\RequestStack; use Symfony\Component\HttpFoundation\Session\Storage\Handler\WriteCheckSessionHandler; -use Symfony\Component\HttpFoundation\Session\Storage\MetadataBag as SymfonyMetadataBag; use Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage; /** @@ -83,14 +82,13 @@ class SessionManager extends NativeSessionStorage implements SessionManagerInter * The request stack. * @param \Drupal\Core\Database\Connection $connection * The database connection. - * @param \Symfony\Component\HttpFoundation\Session\Storage\MetadataBag $metadata_bag + * @param \Drupal\Core\Session\MetadataBag $metadata_bag * The session metadata bag. * @param \Drupal\Core\Site\Settings $settings * The settings instance. */ - public function __construct(RequestStack $request_stack, Connection $connection, SymfonyMetadataBag $metadata_bag, Settings $settings) { + public function __construct(RequestStack $request_stack, Connection $connection, MetadataBag $metadata_bag, Settings $settings) { $options = array(); - $this->requestStack = $request_stack; $this->connection = $connection; @@ -261,12 +259,7 @@ public function regenerate($destroy = FALSE, $lifetime = NULL) { } session_id(Crypt::randomBytesBase64()); - // @todo The token seed can be moved onto \Drupal\Core\Session\MetadataBag. - // The session manager then needs to notify the metadata bag when the - // token should be regenerated. https://drupal.org/node/2256257 - if (!empty($_SESSION)) { - unset($_SESSION['csrf_token_seed']); - } + $this->getMetadataBag()->clearCsrfTokenSeed(); if (isset($old_session_id)) { $params = session_get_cookie_params(); @@ -405,18 +398,6 @@ protected function getSessionDataMask() { // Ignore the metadata bag, it does not contain any user data. $mask[$this->metadataBag->getStorageKey()] = FALSE; - // Ignore the CSRF token seed. - // - // @todo Anonymous users should not get a CSRF token at any time, or if they - // do, then the originating code is responsible for cleaning up the - // session once obsolete. Since that is not guaranteed to be the case, - // this check force-ignores the CSRF token, so as to avoid performance - // regressions. - // The token seed can be moved onto \Drupal\Core\Session\MetadataBag. This - // will result in the CSRF token being ignored automatically. - // https://drupal.org/node/2256257 - $mask['csrf_token_seed'] = FALSE; - // Ignore attribute bags when they do not contain any data. foreach ($this->bags as $bag) { $key = $bag->getStorageKey(); diff --git a/core/tests/Drupal/Tests/Core/Access/CsrfTokenGeneratorTest.php b/core/tests/Drupal/Tests/Core/Access/CsrfTokenGeneratorTest.php index 1f3a42e03614c84bb9341d825bfb24d48f501604..ec2abec46604c02e3e8fd70b47c8b8172429321a 100644 --- a/core/tests/Drupal/Tests/Core/Access/CsrfTokenGeneratorTest.php +++ b/core/tests/Drupal/Tests/Core/Access/CsrfTokenGeneratorTest.php @@ -17,6 +17,7 @@ * Tests the CsrfTokenGenerator class. * * @group Access + * @coversDefaultClass \Drupal\Core\Access\CsrfTokenGenerator */ class CsrfTokenGeneratorTest extends UnitTestCase { @@ -34,21 +35,27 @@ class CsrfTokenGeneratorTest extends UnitTestCase { */ protected $privateKey; + /** + * The mock session metadata bag. + * + * @var \Drupal\Core\Session\MetadataBag|\PHPUnit_Framework_MockObject_MockObject + */ + protected $sessionMetadata; + /** * {@inheritdoc} */ protected function setUp() { parent::setUp(); - $this->key = Crypt::randomBytesBase64(55); $this->privateKey = $this->getMockBuilder('Drupal\Core\PrivateKey') ->disableOriginalConstructor() ->setMethods(array('get')) ->getMock(); - $this->privateKey->expects($this->any()) - ->method('get') - ->will($this->returnValue($this->key)); + $this->sessionMetadata = $this->getMockBuilder('Drupal\Core\Session\MetadataBag') + ->disableOriginalConstructor() + ->getMock(); $settings = array( 'hash_salt' => $this->randomMachineName(), @@ -56,27 +63,71 @@ protected function setUp() { new Settings($settings); - $this->generator = new CsrfTokenGenerator($this->privateKey); + $this->generator = new CsrfTokenGenerator($this->privateKey, $this->sessionMetadata); + } + + /** + * Set up default expectations on the mocks. + */ + protected function setupDefaultExpectations() { + $key = Crypt::randomBytesBase64(); + $this->privateKey->expects($this->any()) + ->method('get') + ->will($this->returnValue($key)); + + $seed = Crypt::randomBytesBase64(); + $this->sessionMetadata->expects($this->any()) + ->method('getCsrfTokenSeed') + ->will($this->returnValue($seed)); } /** * Tests CsrfTokenGenerator::get(). + * + * @covers ::get */ public function testGet() { + $this->setupDefaultExpectations(); + $this->assertInternalType('string', $this->generator->get()); $this->assertNotSame($this->generator->get(), $this->generator->get($this->randomMachineName())); $this->assertNotSame($this->generator->get($this->randomMachineName()), $this->generator->get($this->randomMachineName())); } + /** + * Tests that a new token seed is generated upon first use. + * + * @covers ::get + */ + public function testGenerateSeedOnGet() { + $key = Crypt::randomBytesBase64(); + $this->privateKey->expects($this->any()) + ->method('get') + ->will($this->returnValue($key)); + + $this->sessionMetadata->expects($this->once()) + ->method('getCsrfTokenSeed') + ->will($this->returnValue(NULL)); + + $this->sessionMetadata->expects($this->once()) + ->method('setCsrfTokenSeed') + ->with($this->isType('string')); + + $this->assertInternalType('string', $this->generator->get()); + } + /** * Tests CsrfTokenGenerator::validate(). + * + * @covers ::validate */ public function testValidate() { + $this->setupDefaultExpectations(); + $token = $this->generator->get(); $this->assertTrue($this->generator->validate($token)); $this->assertFalse($this->generator->validate($token, 'foo')); - $token = $this->generator->get('bar'); $this->assertTrue($this->generator->validate($token, 'bar')); } @@ -89,11 +140,11 @@ public function testValidate() { * @param mixed $value * (optional) An additional value to base the token on. * + * @covers ::validate * @dataProvider providerTestValidateParameterTypes */ public function testValidateParameterTypes($token, $value) { - // Ensure that there is a valid token seed on the session. - $this->generator->get(); + $this->setupDefaultExpectations(); // The following check might throw PHP fatals and notices, so we disable // error assertions. @@ -124,12 +175,12 @@ public function providerTestValidateParameterTypes() { * @param mixed $value * (optional) An additional value to base the token on. * + * @covers ::validate * @dataProvider providerTestInvalidParameterTypes * @expectedException InvalidArgumentException */ public function testInvalidParameterTypes($token, $value = '') { - // Ensure that there is a valid token seed on the session. - $this->generator->get(); + $this->setupDefaultExpectations(); $this->generator->validate($token, $value); } @@ -152,12 +203,13 @@ public function providerTestInvalidParameterTypes() { /** * Tests the exception thrown when no 'hash_salt' is provided in settings. * + * @covers ::get * @expectedException \RuntimeException */ public function testGetWithNoHashSalt() { // Update settings with no hash salt. new Settings(array()); - $generator = new CsrfTokenGenerator($this->privateKey); + $generator = new CsrfTokenGenerator($this->privateKey, $this->sessionMetadata); $generator->get(); }