Allow the session name suffix to be configurable

add tag.

I am also getting this issue, but with 8.4. I can't test it because my user doesn't have the D7 cookie, making it frustrating as we get reports from other users every day and have to walk them through deleting cookies. Looking into a solution involving deleting all cookies for non-logged-in users on the home page.

I had the same problem when i changed the site from Drupal 7 to Drupal 8. I changed the cookie name from the services.yml.
The file is located in sites/default folder. Look for variable cookie_domain and change it to say '.example.com'. Uncomment and save the file. You might have to try different combination of the domain to make sure you get the one used by Drupal 7. Hope this works for you.

Tagging to get added to the list of possible issues when upgrading from D6 or D7.

Faced with this issue. Spent couple days trying to understand why it was happening and how to fix.

The main reason for it is that Drupal 7 strips www from domain as described in #2522002: Do not strip www. from cookie domain by default because that leaks session cookies to subdomains. While fixing that issue is definitely a must I think we also should made session cookie name generation a bit more ... Not sure about the proper term here, probably "better".

At the moment $session_name is composed of host and base path (core/lib/Drupal/Core/Session/SessionConfiguration.php:96) :

$session_name = $request->getHost() . $request->getBasePath();

I think we should include something more unique to the particular website here, like site UUID or hash salt. So it would be :

$session_name = $request->getHost() . $request->getBasePath() . $settings['hash_salt'];

Haven't found session system in component so setting to base.

This issue will not affect Drupal 6 sites upgrading to Drupal 8 because code to generate cookie name is different between Drupal 6 and Drupal 8.

This is how session cookie name is generated in different Drupal versions:

    'Drupal 6' => md5($session_name),
    'Drupal 7' => substr(hash('sha256', $session_name), 0, 32),
    'Drupal 8' => substr(hash('sha256', $session_name), 0, 32),

Lets try with salt.

We've had this same issue. @RoSk0's solution looks like a reasonable one to me!

@RoSk0 solutions looks reasonable for me too.

I also had this problem on a recent site launch. Applying the patch seems to fix the issue.

Just ran into this when pushing a site live from D7 to D8. Patch fixed this for me.

As this has been reported as fixing the issue for multiple users, and the patch looks sensible, moving to RTBC.

I'd say this is fairly important, as without this, D7 to D8 live site migrations will be broken until cookies expire / are cleared manually.

I don't think we should leak the hash salt in session names.
We should be guarding that as it is critical to site security.

Can we add an optional alternate session suffix instead.

Just spoke to @larowlan on Slack about this, we figured this is good to go RTBC as the hash salt is added to the $session_name string which is eventually used in this:

return substr(hash('sha256', $session_name), 0, 32);

So it's not being spat out directly.

After looking at the hunk with full context, I'm still not convinced we're not leaking the hash salt

else {
      // Otherwise use $base_url as session name, without the protocol
      // to use the same session identifiers across HTTP and HTTPS.
      $session_name = $request->getHost() . $request->getBasePath();
      // Replace "core" out of session_name so core scripts redirect properly,
      // specifically install.php.
      $session_name = preg_replace('#/core$#', '', $session_name);
    }

    return substr(hash('sha256', $session_name), 0, 32);

So in this patch its host + base path + hash salt into sha256 hashing algorithm with enough GPU, someone could reverse engineer that because they would have host, + base path + hash.

So i think we'd need to use the private key too, but I'm still not convinced this is needed, when a settings 'session name suffix' flag with no default would be enough.

I will ask the security team's opinion

Could someone please update the issue summary, explaining what the solution looks like?

After thinking over #19, it seems that any added string would do. There doesn't seem to be a reason to use the most a secret string in the site for this.

FWIW, It looks like \Drupal\Core\Site\Settings::getHashSalt is commonly used instead of the value directly.

The advantage of hash salt over something like a suffix with no default value is that is clearly differentiates any site and therefore avoids the conflicts without a site builder having the think about it.

That seems a definite advantage to me, as it took me quite a while to figure out what was going wrong, especially as in my case none of the dev team experienced the issue as they happened to not have stale cookies. Quite hard to debug for end users.

Are there any other similar suitable identifiers we could use to avoid hash salt?

I can't think of any. What if we could seed a configuration value—a random string—referred to in #19 into settings.php on new installs? Would that help some of these use-cases?

I am not dead-set against using the salt, however I too don't fully understand the implications of exposing the salt in a hash mixed only with known strings as mentioned above.

It's often, but not always, mixed with the private key and other data in Drupal core (it is a salt, after all...), like here in the CSRF token generator:

Crypt::hmacBase64($value, $seed . $this->privateKey->get() . Settings::getHashSalt());

`_update_manager_unique_identifier()` hashes the hash salt alone but the result is used in a local filesystem context.

Also, it's worth mentioning that in the current patch, the session name will be just half of a sha256 hash. It's not much to go on even if you have every available hash in the sha256 space on a megacomputer.

(edited for clarity)

I agree with @andrewbelcher here, would be nice if the default out of the box setup means D8 sessions don’t conflict with D7 sessions.

We only encountered this around 1 day before going live, and it was only noticed on our live domain. I imagine someone could push a migrated site live without catching this problem on a staging domain.

Even if we added a static string “D8” for example instead of hash salt, there’s still a side affect that any change to the session name will log out all users on existing D8 sites.

Actually in this case I don't care what goes into hash() from a security perspective because this routine returns only half of the hash for the session name. So I think this is perfectly safe. It would be good to get a second opinion but I think I am a thumbs up. We need the patch updated to use Settings::getHashSalt().

php > echo strlen(substr(hash('sha256', 'foobar'), 0, 32));
32
php > echo strlen(hash('sha256', 'foobar'));
64

Doh, good one larowlan - yep you're right @cilefen, sorry for the noise folks

Updated patch to use Settings::getHashSalt() as per #25.

Added Settings initialization to SessionConfigurationTest.

Should be green now.

Nice! I just realized this comment needs updating:

      // Otherwise use $base_url as session name, without the protocol
      // to use the same session identifiers across HTTP and HTTPS.

Updated comment.

Proper patch version. #34 version is missing test fix.

I just experienced this issue after replacing a Drupal 7 site with a Drupal 8 site. I can confirm that the patch in #35 fixes the issue.

The patch as it stands is going to log out everyone after an 8-8 update which doesn't seem acceptable.

Could this use something in $settings instead, defaulting to an empty string?

Also while I don't think using the hash salt here is risky, it's a bit of an odd thing to use since we're not actually using it to salt the hash but just to namespace between major core versions.

With all respect @catch, but why one-time logout event could be a show stopper for this patch?

Frustration faced by that end users of the Drupal-based websites that was upgraded and went live without this patch in my opinion way bigger, like really-really bigger, then the fact they they would need to log in again, which some of them might even not notice.

I'm happy to work more on this patch, but this issue been reported two years ago. This means that most projects upgraded in this period faced with the same issue. Which in turn makes a smooth project switch after upgrade not that smooth and harms to reputation of Drupal and vendor delivering upgrade. I faced with this and I don't want it to happen to other companies/developers and Drupal in general.

With all respect @catch, but why one-time logout event could be a show stopper for this patch?

Because there are other ways to fix it that don't result in that.

Another way to fix it would be a configuration item (without a UI). For new installs default it to '8', but for sites that are upgrading, have an update that sets it to an empty string. Then sites that are migrating from 7-8 get a different cookie name, but existing 8.x sites stay the same.

Question:
Is this patch a one-time fix? Or does this patch need to be reapplied with every core release (until the patch becomes part of core)?

Edit: I tested this out, it is as I expected: You have to patch every time.
... until this becomes part of core, which may never be...

Then sites that are migrating from 7-8 get a different cookie name, but existing 8.x sites stay the same.

@catch, this would trigger a logout when D9 is release though, yeah?

@hoporr, strong recommendation to use cweagans/composer-patches to manage patches like this in composer.

this would trigger a logout when D9 is release though, yeah?

No if we leave the configuration item as is, already-update 8.x sites could just stay the same in to 9.x and onwards.

Patch from #48 is also applicable for 8.9.13 version.
Still testing if this may impact website negatively but I think patch #35 looks safe in the sight of eye.

Re-rolled against 9.2.7

Setting back to the NW as I suspect there are issues with the implementation in ##4 - and the biggest one for me is performance, but also the fact that for now good reason part of the session configuration will live in configuration sub-system rather than suggested settings (settings.php) or, where I think it belongs, as an new option in session.storage.options as the rest of the session configuration.

Off course this requires profiling, but common sense suggests that making @config.factory dependency of the @session_configuration would not improve system performance in any way.

For now, until I found time to implement suggested above approach, attaching patch from #35 re-rolled on 9.5.x (applies to 9.2.x no problem).

Well, documentation will do with help of course, but this is what was proposed in #54:

• is configurable, allows to customise generated name when and where needed - those of us who deal with upgrade from Drupal 7 to 9
• and doesn't change current behaviour - generated cookie name, to satisfy #39

Whoops, missed one TODO...

Uploading #56 with only the core/* changes for composer

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Re-roll Patch #56 against Drupal 10.1.x

Fixed failed commands on #61
Attached patch against Drupal 10.1.x
Attached interdiff

Unfortunately, I have patched the core files (#56 for 9.5) but the reset password process lead to an access denied page. Removing the D7 cookies solve this issue.

@DuneBL Did you configure a session suffix after installing the patch?

No, I didn't... and I have no clue on how to do it... many thanks for pointing me to this... If you can provide me some explanation it would be great

Copy default.services.yml to services.yml, if you don't have one. Edit, or add a session_suffix key after the sid_bits_per_character key. Put a short string of characters in it. That's how this patch works, but it lacks documentation at this time.

In my opinion, for clarity the configuration key and its reference in code should be session_name_suffix rather than session_suffix.

In fact name_suffix would be short and accurate, because the context is clearly about sessions.

@cilefen many thanks it is working. I confirm patch #56 is working on 9.5

Corrected re-roll errors in #61 that fed into #62.

(patch written for 10.1.x, but also applies to 11.x)

Would be good to see the issue summary updated to the standard template,

Previously tagged for a change record.

Recommend using MRs

Re-roll for D10.2
edit: sorry, empty file

I prefer the name I suggested in comment #69.

Thanks all for working on this! I have converted the latest patch to the MR. Also updated the name as per #69.

Draft CR is here: https://www.drupal.org/node/3446100

I think this issue is pretty important to fix, as with the D7 EOL approaching, this has potential to cause a lot of troubles to sites still going to migrate. We were also caught by this on a few sites and no user with a D7 cookie was able to login until the cookie was deleted.

Pipeline seems to be green, moving to Needs review.

Slight title update as assuming this is an issue no matter the landing version.

Reviewing the issue summary solution and seems straight forward about adding the suffix, also reviewed the CR and reads fine to me.

Reviewing the code changes and appears to be doing as advertised, test coverage update seems to be there too.

Believe this is probably good.

I reviewed the comments and I think they need improvement. Also, the title is the statement of the problem not what is being changed. The change record title is more what I am thinking. Although the change record does not need Drupal in the title because it is a change record for Drupal :-).

The change record title could be 'The session_name suffix can be configured.' and the issue title "Allow the session_name suffix to be configurable'

Finally, should it be 'session name' or 'session_name' in the documentation. I'd also like to know why if it a session name suffix why not name the new property 'session_name_suffix'?

It should be name_suffix. The entire context is the session.

The full name is session.storage.options.name_suffix, which to me makes more sense than session.storage.options.session_name_suffix.

I have updated the comments in the MR (still open to further suggestions), issue title and the CR.

I have not used session_name (with underscore) in those sentences, but instead used session name, because from what I have seen, we use this phrase without underscore on different places in code already (phrase session_name is only used as a $session_name variable). So I think it would be better without the underscore.

Also agree with @cilefen, that the name_suffix is probably sufficient, as it is under the session.storage.options.

Thanks!

Actually after the suggested title change, it may look like this is no longer a "Bug report", but I suggest this is still treated as a bug, because it is still a broken functionality related to D7 -> D10 upgrade (and pretty major).

There is an unrelated MR pipeline failure. It seem to be related to this: #3445211: Composer tests fail because of invalid Drupal version

Therefore keeping this as Needs Review.

From what I can tell the feedback has been addressed. The added comment definitely clearly states what this is for and does.

There are failed tests - can the MR be updated for the latest changes and have a green test run.

Rebased and the tests are now green, so setting the status to RTBC.

This looks good to me now. Committed/pushed to 11.x and cherry-picked to 11.0.x, 10.4.x, 10.3.x, thanks!