Problem/Motivation

In my case, it was the following use case that triggered the issue:

I have a process of two forms in a dialog, the first is a customized login form, that is submitted, the user is logged in, the session regenerated, then my ajax callback of the submit button is called, where I immediately build a new form and return that to the user.

That works perfect well, but the second form then can't be submitted, because it fails on form token. On those ajax requests, the user does not have a csrf token in the session, despite getting one generated when the form was built.

The reason is that session regenerating somehow re-initializes $_SESSION and detaches it from the symfony session meta bag, so an updated there isn't reflected in the session.

See

https://gist.github.com/Berdir/9d8739e5962a227dfdae

Proposed resolution

@znerol said in the link above:

Bags are references on $_SESSION['bag_name'], so maybe those references need to be refreshed after the call to session_id. I guess that SessionManager::startNow() is the culprit, because that tries to transfer session data by copying it. Maybe it is enough to call parent::loadSession() after restoring the session data?

Remaining tasks

None.

User interface changes

None.

API changes

None.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug
Issue priority Critical because it prevents data loss of session data set immediately after login
Files: 
CommentFileSizeAuthor
#6 csrf-2437761-6.patch3.62 KBDom.
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,428 pass(es). View
#2 csrf-2437761-2--test-only.patch3.11 KBDom.
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,418 pass(es), 1 fail(s), and 0 exception(s). View
#2 csrf-2437761-2.patch3.55 KBDom.
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,421 pass(es). View

Comments

almaudoh’s picture

Status: Active » Closed (duplicate)
Dom.’s picture

Status: Closed (duplicate) » Needs review
FileSize
3.55 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,421 pass(es). View
3.11 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,418 pass(es), 1 fail(s), and 0 exception(s). View

Re-open to relieve #2228393: Decouple session from cookie based user authentication. Patch there corrects to much issues at once. Let's solve this here.

Status: Needs review » Needs work

The last submitted patch, 2: csrf-2437761-2--test-only.patch, failed testing.

Dom.’s picture

Status: Needs work » Needs review
znerol’s picture

Fantastic work so far!

+++ b/core/modules/system/src/Tests/Session/SessionTest.php
@@ -100,6 +100,10 @@ function testDataPersistence() {
+    // Test property added to session object form hook_user_login().
+    $this->drupalGet('session-test/get-from-session-object');
+    $this->assertText('foobar', 'Session data is saved in Session() object.', 'Session');
+

I think testDataPersistence() is not quite the right place for this assertions. One option is to find a place in testSessionSaveRegenerate(), thought that is already quite convoluted. Probably a cleaner and lower risk option would be to add a specific test method which simply performs a login and then checks whether the value added from within hook_user_login is present.

Perhaps something like this:

/**
 * Tests \Drupal\Core\Session\SessionManager::regenerate().
 */
public function testSessionPersistenceOnLogin() {
  [...]
}
Dom.’s picture

FileSize
3.62 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,428 pass(es). View

Changed as per #5

znerol’s picture

Issue summary: View changes
Priority: Normal » Critical
Status: Needs review » Reviewed & tested by the community

Great. This is RTBC if testbot if testbot permits it.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed abf2467 and pushed to 8.0.x. Thanks!

diff --git a/core/modules/system/tests/modules/session_test/src/Controller/SessionTestController.php b/core/modules/system/tests/modules/session_test/src/Controller/SessionTestController.php
index 2db6f21..4437743 100644
--- a/core/modules/system/tests/modules/session_test/src/Controller/SessionTestController.php
+++ b/core/modules/system/tests/modules/session_test/src/Controller/SessionTestController.php
@@ -39,8 +39,8 @@ public function get() {
   public function getFromSessionObject() {
     $value = \Drupal::request()->getSession()->get("session_test_key");
     return empty($value)
-    ? []
-    : ['#markup' => $this->t('The current value of the stored session variable is: %val', array('%val' => $value))];
+      ? []
+      : ['#markup' => $this->t('The current value of the stored session variable is: %val', array('%val' => $value))];
   }
 
   /**

Fixed indentation on commit.

  • alexpott committed abf2467 on 8.0.x
    Issue #2437761 by Dom., znerol: CSRF token seed and possibly other...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.