Problem/Motivation

The release of symfony/http-foundation v4.4.42 validates and recreates the session ID when it's not valid.
\Drupal\Core\Session\SessionManager::getId() generates a random session ID if a session is not started yet.
The ID generated through \Drupal\Component\Utility\Crypt::randomBytesBase64() can contain invalid characters such as underscores.
The next time that the session is started, the Symfony component will regenerate the id and the session data will be lost.
Calling \Drupal\Core\Session\SessionManager::getId() is already deprecated as indicated in code and in this change record.

Steps to reproduce

This issue was encountered during tests while using the cas module version 1.7.0 but it can be reproduced by calling \Drupal\Core\Session\SessionManager::getId() before a session exists.

Proposed resolution

None at the moment, I'm creating this just as a reference for users that might run into the same.
It can be fixed by

  1. not calling getId() and using alternatives as explained in change record Drupal uses PHP session ID generation.
  2. locking symfony/http-foundation to 4.4.41, at least until the point above is fixed.

Writing a test is hard as the ID generation is random so it fails only sometimes.

Remaining tasks

To be discussed, since now calling the above method can actually create issues.

User interface changes

None.

API changes

TBD.

Data model changes

None.

CommentFileSizeAuthor
#20 3285696-9.5.x-20.patch3.46 KBalexpott
#20 3285696-9.4.x-20.patch3.46 KBalexpott
#11 3285696-11.patch5.23 KBlongwave
#9 3285696-9-downgrade-http-foundation.patch7.74 KBlongwave

Issue fork drupal-3285696

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

sardara created an issue. See original summary.

sardara’s picture

sardara commented
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 9.5.x-dev » 9.4.x-dev
Priority: Normal » Critical

This could be a critical regression in 9.4.0 which is due out tomorrow.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue tags: +Contrib project blocker

I would imagine this might affect the flag module as well. Lazy sessions and having an ID before has been complex and source of bugs over the years.

sardara opened merge request !2382

sardara’s picture

sardara commented

Pushed a test. Since the session ID generation is not always invalid, the test attempts up to 10 times to fail.

sardara’s picture

sardara commented
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Confirmed locally. With the MR applied and symfony/http-foundation:v4.4.42:

$ ddev exec vendor/bin/phpunit -c core core/modules/system/tests/src/Functional/Session/SessionTest.php --filter testSessionDataLossDueToInvalidId
PHPUnit 8.5.26 #StandWithUkraine

Testing Drupal\Tests\system\Functional\Session\SessionTest
F                                                                   1 / 1 (100%)

Time: 3.64 seconds, Memory: 4.00 MB

There was 1 failure:

1) Drupal\Tests\system\Functional\Session\SessionTest::testSessionDataLossDueToInvalidId
Session content lost at iteration 1.
Failed asserting that null matches expected '-459BAYbiDHFLrYW2LusZPAqlnF7NC6Imx_DivhJs5Q'.

/var/www/html/drupal/vendor/phpunit/phpunit/src/Framework/Constraint/IsEqual.php:100
/var/www/html/drupal/core/modules/system/tests/src/Functional/Session/SessionTest.php:101
/var/www/html/drupal/vendor/phpunit/phpunit/src/Framework/TestResult.php:703

FAILURES!
Tests: 1, Assertions: 6, Failures: 1.

Remaining self deprecation notices (2)

  2x: 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
    2x in SessionTest::testSessionDataLossDueToInvalidId from Drupal\Tests\system\Functional\Session

Failed to execute command vendor/bin/phpunit -c core core/modules/system/tests/src/Functional/Session/SessionTest.php --filter testSessionDataLossDueToInvalidId: exit status 1

This is repeatable and fails somewhere in the first 3 iterations.

Fixed by downgrading:

$ composer require symfony/http-foundation:v4.4.41
...
$ ddev exec vendor/bin/phpunit -c core core/modules/system/tests/src/Functional/Session/SessionTest.php --filter testSessionDataLossDueToInvalidId
PHPUnit 8.5.26 #StandWithUkraine

Testing Drupal\Tests\system\Functional\Session\SessionTest
.                                                                   1 / 1 (100%)

Time: 7.72 seconds, Memory: 4.00 MB

OK (1 test, 24 assertions)

Remaining self deprecation notices (11)

  11x: 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
    11x in SessionTest::testSessionDataLossDueToInvalidId from Drupal\Tests\system\Functional\Session

Failed to execute command vendor/bin/phpunit -c core core/modules/system/tests/src/Functional/Session/SessionTest.php --filter testSessionDataLossDueToInvalidId: exit status 1
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Active » Needs review
StatusFileSize
new 3285696-9-downgrade-http-foundation.patch7.74 KB

Attached patch forces a downgrade of symfony/http-foundation until we figure out a better solution.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Maybe the fix should be that SessionManager::getId() repeatedly calls Crypt::randomBytesBase64() until it gets a result that matches the same regex Symfony uses?

Or even, we just replace _ with , as Symfony allows that in /^[a-zA-Z0-9,-]{22,}$/?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
StatusFileSize
new 3285696-11.patch5.23 KB

Implementation of #10 plus fix for the use of deprecated code in the test.

sardara’s picture

sardara commented

I pushed a new test which can fail consistently. I didn't remove the old test yet.
I am not also very happy with the "legacy" word I'm using around, maybe "deprecated" is a better fit.
@longwave thanks for testing locally.
Yes the replace is the fix I was preparing too, I just pushed something similar.

sardara’s picture

sardara commented

I cannot think of a way to write a test that doesn't rely on multiple retries to test that only "correct" ids are generated.
Unfortunately Crypt::randomBytesBase64() cannot be influenced in any way to return a string with an underscore.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

I think the upstream change is wrong and should be reverted. The list of characters they are using is based on file based session handling and the characters that are legal for a cookie are different. See https://github.com/symfony/symfony/pull/46249#issuecomment-1155769309

The last submitted patch, 9: 3285696-9-downgrade-http-foundation.patch, failed testing. View results

quietone’s picture

quietone commented
Issue summary: View changes

Fix link in the IS

sardara’s picture

sardara commented

Given the random id generation will be gone in 10.0.0, I believe the approach of #11 is good.
Maybe tests and comments I wrote could be polished, but the fix by @longwave is correct, underscore is the only invalid character returned by Crypt::randomBytesBase64().

That is if we need to merge something to unblock 9.4.0. I agree with @alexpott for upstream. Along with an arbitrary character list, it looks the wrong place and time to regenerate an ID, as the session data loss proves. Also the public ::setId() method has no validation whatsoever so why ::start() should change ID.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Reviewed & tested by the community

FWIW the session ID will be url encoded automagically by PHP - see documentation for setcookie - https://www.php.net/manual/en/function.setcookie.php

I agree that #11 is a good fix for now to prevent regressions for 9.4.0 but also ensure that if there is a security release for http-foundation it means we can move to it without having to think too much.

The fact that the test added here is not guaranteed to fail without the fix is very very icky. So I think we could consider committing #11 without the tests. OTOH the tests failed all the times on this issue without the fix so maybe leaving them is okay. Will leave up to a release manager to decide - I could go either way.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

I think Symfony might revert the change - see https://github.com/symfony/symfony/pull/46678 - so maybe the best thing to do here will to add a conflict on 4.4.42 and then we can get 4.4.43 when it comes out. The session ID validity check is the only change from 4.4.41 to 4.4.42 - see https://github.com/symfony/http-foundation/compare/v4.4.41...v4.4.42

Going to create a patch based on that because this is certainly the least risky approach.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 3285696-9.4.x-20.patch3.46 KB
new 3285696-9.5.x-20.patch3.46 KB

Here's the patches that declare a conflict. If symfony decides to revert then we should use them instead of #11.

catch’s picture

catch
he/him
Primary language English
commented

Declaring a conflict seems good - given this is time-sensitive we might need to guess what they'll do (i.e. commit the conflict, open a follow-up to track whether they revert or not).

If we go with using commas instead of underscores, given the method is already deprecated and hard to test, and it's a one-line fix that we're very unlikely to ever touch until we remove the whole method, would be fine with committing the fix itself without test coverage.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

The more I think about it the more I think that doing #20 for now is the best option for today. The change in 4.4.42 will impact sites using contrib modules that depend on this. People with existing sessions that have an underscore in them will lose their sessions in the upgrade if we release with 4.4.42 - and that would not normally occur during an upgrade.

The last submitted patch, 20: 3285696-9.4.x-20.patch, failed testing. View results

  • catch committed 462e2fb on 9.5.x 
    Issue #3285696 by sardara, alexpott, longwave: Legacy random session ID...

  • catch committed 0d71861 on 9.4.x 
    Issue #3285696 by sardara, alexpott, longwave: Legacy random session ID...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

OK I've committed the 9.5.x and 9.4.x patches.

The same change is in Symfony 6, but we don't care in Drupal 10 because the deprecated method no-longer exists there.

Opened #3285817: Symfony 4.4.42 breaks legacy random session ID generation, adapt it if there's no rollback to track whether upstream reverts or not and if we still need to implement the fix here or not.

Status: Fixed » Closed (fixed)

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

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Issue tags: -Contrib project blocker +Contributed project blocker

I replaced the used issue tag with the official one. I apologize for bumping this issue.