I'd like to namespace the shared cookies in a standard way (based on the cookie domain), to prevent getting cookies that can't be deleted on a sub-sub domain.

For example, we have environments like *.example.com, *.a.dev.example.com, *.demo.example.com, *.stage.example.com. Cookies from *.example.com have been encrypted with a different key, so at least they are invalid in these environments, but the subsites can't actually overwrite/delete the cookie from *.example.com, so if you have a cookie from there, you can't login.

I could be wrong on this, here's a stackoverflow q with answers seeming to agree though - http://stackoverflow.com/questions/3923285/how-to-remove-main-domain-coo....

The only solution I could come up with is to namespace the cookies with the cookie domain, e.g. EXAMPLE_COM_CHOCOLATECHIP.

Thoughts?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

glennpratt’s picture

My other thought is to restructure bakery to not care if a cookie from another domain is set, overwrite it if it needs one, ignore it if it doesn't.

In testing though, I can't seem to make that work, the cookie from the parent domain appears to win.

glennpratt’s picture

It seems parent domain cookies override sub domain cookies no matter what I do, so I went with namespace as the original proposal. Basically:

$type . md5($cookie_domain);

Resulting in:

CHOCOLATECHIP10b8124b78109b738f753a7c9962b1ae

D6 patch, D7 to follow.

glennpratt’s picture

Version: 7.x-2.x-dev » 6.x-2.x-dev
FileSize
3.94 KB

Sorry, included another patch. Still rolling the 7.x patch.

glennpratt’s picture

Version: 6.x-2.x-dev » 7.x-2.x-dev
FileSize
4.16 KB

7.x patch.

These patches also have add some DRY-ness to the SSL cookie patch.

glennpratt’s picture

Category: task » bug
Status: Active » Needs review

I'm changing this to a bug, I'm sure it's not major, but it's pretty frustrating for our use case. Basically we can only have one Bakery environment on our domain without it (no demo, no dev-stage, etc).

coltrane’s picture

Category: bug » feature

I understand it's pretty frustrating, but it's a feature request to handle your specific use case. I haven't reviewed the patch yet, will think on this more. I understand you want to support your work flow but I hesitate in all cases of increasing the complexity of the code base.

glennpratt’s picture

I hear you, though I really think the complexity is minimal, considering the SSL patch that is already in. Here is the cookie name function from the patch:

 /**
<?php
/**
 * Generate the name for a bakery cookie, this prevents domain
 * mismatch on subdomains that are not part of a parent domains bakery config.
 *
 * @param string $type
 *   CHOCOLATECHIP or OATMEAL, default CHOCOLATECHIP
 * @return string
 *   The namespaced cookie name for this environment.
 */
function _bakery_cookie_name($type = 'CHOCOLATECHIP') {
  // Use different names for HTTPS and HTTP to prevent a cookie collision.
  if (ini_get('session.cookie_secure')) {
    $type .= 'SSL';
  }
  // Namespace the cookie by the cookie domain.  Use md5 to avoid using any
  // reserved cookie terms (all of which contain non-hex characters):
  //   "expires", "domain", "path", and "secure".
  $name = $type . md5(variable_get('bakery_domain', ''));
  return $name;
}
?>
bcmiller0’s picture

Version: 7.x-2.x-dev » 7.x-2.0-alpha3
FileSize
4.04 KB

re-rolled against alpha3 tag, and also adding some missing cookie_secure's.

drumm’s picture

I like this patch, it will make #1795118: Better support for mixed SSL environments a smaller patch. I haven't had a chance to test it, but I can confirm it applies nicely and looks okay.

glennpratt’s picture

I would love to see this patch go in or get some feedback - it's hard to roll other patches around it.

If the namespace is a show stopper, then maybe we can settle on using a function to name cookies and not smattering code everywhere that does it... that will make the namespace patch much smaller.

coltrane’s picture

Note, for others to help with testing there's a VM with cucumber tests https://github.com/bjeavons/Bakery-Chef

(/me needs to document using it)

coltrane’s picture

BarisW’s picture

Hi coltrane,

thanks for the patches. I've spend days debugging to get Bakery get past Varnish and it seemed that Varnish strips out cookies that don't start with a specific name. See http://help.getpantheon.com/pantheon/topics/_cookie_cookiename_works_on_...

By adding SESS in front of the cookie name (and by making sure the cookiename is lowercase), Varnish lets the cookie pass.

glennpratt’s picture

@BarisW - You probably have something in your VCL that is looking for SESS. Just add something similar to allow CHOCOLATECHIP.

If you need help, post your VCL somewhere and ping me in IRC.

coltrane’s picture

Yes, you should change your Varnish configuration to let the Bakery cookies pass. It would be incorrect to prefix Bakery cookies with "SESS".

BarisW’s picture

Not everyone is able to configure their Varnish configuration. For example; at Pantheon I can submit a request to get it added, but I cannot do it myself.

What if we introduce a variable to do this? Or better, make it pluggable?

By changing this:

<?php
$name = $type . md5(variable_get('bakery_domain', ''));
return 'SESS' . strtolower($type);
?>

Into:

<?php
$name = $type . md5(variable_get('bakery_domain', ''));
return drupal_alter('bakery_cookie_name', $name);
?>

So I can just implement:

<?php
function MYMODULE_bakery_cookie_name(&$name) {
  return 'SESS' . strtolower($name);
}
?>
coltrane’s picture

That would be, ok. In fact this whole hashing addition to the cookie name could be implemented by a custom module for the use-case that Glenn has. Would that be ok with you Glenn?

coltrane’s picture

Version: 7.x-2.0-alpha3 » 7.x-2.x-dev
Assigned: Unassigned » coltrane
Status: Needs review » Needs work

Needs work and reroll because #1915494: Prefix data signature and check prior to unserialize was committed. I'll work on this.

coltrane’s picture

Assigned: coltrane » Unassigned
Status: Needs work » Needs review
FileSize
4.29 KB
4.27 KB

Ah, drupal_alter() requires a full bootstrap of course. That's not a viable option.

Attached patch introduces new variable bakery_cookie_extension to alter the Bakery cookie name per-site. Also includes variable_del()s.

drumm’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, we're using this on {association,portland2013,sydney2013}.drupal.org.

coltrane’s picture

Status: Fixed » Closed (fixed)

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