Issue #2256257 by znerol, cosmicdreams | YesCT: Move token seed in SessionManager in isSessionObsolete() and regenerate() to the MetadataBag.

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:

core/lib/Drupal/Core/Access/CsrfTokenGenerator.php

@@ -9,7 +9,7 @@ namespace Drupal\Core\Access;
 
 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 @@ class CsrfTokenGenerator {
    * @see \Drupal\Core\Session\SessionManager::start()
    */
   public function get($value = '') {
-    if (empty($_SESSION['csrf_token_seed'])) {
+    $seed = $this->sessionMetadata->getCsrfTokenSeed();
-      $_SESSION['csrf_token_seed'] = Crypt::randomBytesBase64();
+    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 @@ class CsrfTokenGenerator {
    *   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);
   }
 
   /**

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 @@ class BatchStorage implements BatchStorageInterface {
   /**
    * {@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();
   }
+
 }

core/lib/Drupal/Core/Session/MetadataBag.php

@@ -15,6 +15,11 @@ use Symfony\Component\HttpFoundation\Session\Storage\MetadataBag as SymfonyMetad
  */
 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 @@ class MetadataBag extends SymfonyMetadataBag {
     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]);
+  }
+
 }

core/lib/Drupal/Core/Session/SessionManager.php

@@ -14,7 +14,6 @@ use Drupal\Core\Session\SessionHandler;
 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 @@ class SessionManager extends NativeSessionStorage implements SessionManagerInter
     }
     session_id(Crypt::randomBytesBase64());
 
-    // @todo The token seed can be moved onto \Drupal\Core\Session\MetadataBag.
+    $this->getMetadataBag()->clearCsrfTokenSeed();
-    //   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']);
-    }
 
     if (isset($old_session_id)) {
       $params = session_get_cookie_params();
@@ -405,18 +398,6 @@ class SessionManager extends NativeSessionStorage implements SessionManagerInter
     // 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();

core/tests/Drupal/Tests/Core/Access/CsrfTokenGeneratorTest.php

@@ -17,6 +17,7 @@ use Symfony\Component\HttpFoundation\Request;
  * 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())
+    $this->sessionMetadata = $this->getMockBuilder('Drupal\Core\Session\MetadataBag')
-      ->method('get')
+      ->disableOriginalConstructor()
-      ->will($this->returnValue($this->key));
+      ->getMock();
 
     $settings = array(
       'hash_salt' => $this->randomMachineName(),
@@ -56,27 +63,71 @@ class CsrfTokenGeneratorTest extends UnitTestCase {
 
     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 @@ class CsrfTokenGeneratorTest extends UnitTestCase {
    * @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->setupDefaultExpectations();
-    $this->generator->get();
 
     // The following check might throw PHP fatals and notices, so we disable
     // error assertions.
@@ -124,12 +175,12 @@ class CsrfTokenGeneratorTest extends UnitTestCase {
    * @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->setupDefaultExpectations();
-    $this->generator->get();
 
     $this->generator->validate($token, $value);
   }
@@ -152,12 +203,13 @@ class CsrfTokenGeneratorTest extends UnitTestCase {
   /**
    * 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();
   }