Fix cookie conflicts on shared domains

+1

#1: allow-configurable-cookie-prefix-1361742-1.patch queued for re-testing.

Updated wording to mention secure cookies, rather than HTTPS.

I refactored the patch so that it's applicable to 8.9 and D9

Fixed fail test cases.

Another use for a custom cookie prefix is to prevent sites that don't have their subdomains set up hierarchically from overwriting each other's cookies. Example:

site.a.example.com and site.b.example.com are different locales for the same site. They use .example.com as their cookie domain.
anothersite.a.example.com and anothersite.b.example.com also share the .example.com cookie domain, and they overwrite the other site's cookies.

I'm glad to see some movement on this issue, but please don't prefix "S" or anything else in a secure context, as it defeats the main reason I and possibly many others need this configurability: the __Secure- and __Host- prefixes which allow a higher level of security in all modern browsers.

To quote the MDN article on the Set-Cookie header:

Note: Some <cookie-name> have a specific semantic:

__Secure- prefix: Cookies with names starting with __Secure- (dash is part of the prefix)
must be set with the secure flag from a secure page (HTTPS).

__Host- prefix: Cookies with names starting with __Host- must be set with the secure flag, must be from a secure page (HTTPS), must not have a domain specified (and therefore, are not sent to subdomains), and the path must be /.

To quote Tough Cookies by Scott Helme:

Another great addition to help create tough cookies is the cookie prefix. This approach was devised to "smuggle cookie state to the server within the confines of the existing Cookie request header syntax". In other words, by implementing a new security feature without requiring changes/patches to servers/applications we are going to see much higher adoption a lot more quickly. There are 2 different prefixes that you can add to your cookie.

Just tested this out and it works great for my use case as described above! My only suggestion is to rephrase the docblock slightly as the wording feels off. Current one:

/**
 * Drupal creates a cookie name consisting of a static prefix and a site-
 * specific name that is automatically created. When hosting multiple sites
 * under a single domain, it may be desirable to add a unique prefix for sites
 * to easily filter these in reverse proxies or utilities.
 */

I suggest this, which adds a first line to match other docblocks and also rewrites the first sentence to flow a bit better and not use "create" twice in a way that felt ambiguous:

/**
 * Session cookie prefix:
 *
 * Drupal creates a session cookie name consisting of a static prefix and a
 * site-specific name. When hosting multiple sites under a single domain, it
 * may be desirable to add a unique prefix for sites to easily filter these in
 * reverse proxies or utilities.
 */

Whoops, did not mean to change these.

Tagging for framework manager reviews for their thoughts.

Either way think this will require a test case for the new feature.

Also MR has failures.

Thanks for working on this @Darren Oh!

I would say we need to split the patch, because it seems to me that it is combining two separate changes:

Adding a cookie prefix:

@@ -66,13 +67,14 @@ public function getOptions(Request $request) {
+    $prefix = Settings::get('session_cookie_prefix', '');
     ...
-    $prefix = $request->isSecure() ? 'SSESS' : 'SESS';
+    $prefix .= $request->isSecure() ? 'SSESS' : 'SESS';

Fixing a problem with overwriting site cookies:

@@ -102,6 +104,10 @@ protected function getUnprefixedName(Request $request) {
+    // Add hash salt to prevent sites on shared domains with separate databases
+    // from overwriting each other's cookies.
+    $session_name .= Settings::get('hash_salt', '');

I think the second change (seems like a bug-fix?) should be in a separate issue. Or the issue title + summary needs to be updated and explained why there are these two changes together. Thanks!

poker10, I have updated the issue description. Both changes are addressing the same bug. The configurable prefix was the original solution, but it requires site owners to take action to prevent the bug. I added the database hash as a more reliable solution that does not require action from site owners. I believe both solutions are required because the same bug affects reverse proxies that filter cookies, and the database hash does not fix that.

This could be tested by leaving the prefix blank, logging into the site, changing the prefix, reloading the site and confirming that the session is no longer logged in, then removing the prefix and confirming the original session is logged in again; might be a little fiddly to do, but it'd be simpler than trying to create concurrent installs of core.

And I suspect the test coverage failures are related to the addition of the hash_salt setting to the session name, maybe this should be optional and it should just use the prefix?

What's the state with this? I see the last activity was 2 years ago. I've got the same issue right now. This change should be in core IMO.

Hi,
I am attaching a proposed patch for this issue, including a unit test to validate the behavior.

The implementation has been tested successfully on a Drupal 11 instance and appears to work correctly.

For maintainability reasons and to ease compatibility with future Drupal core updates, I am proposing a version of the patch that does not rely on the hash salt, along with dedicated test coverage.

I was not able to update the issue repository, which is still based on an older Drupal version (Drupal 10), so I am providing the patch file directly.
Marc