Recent changes to the default settings.php automatically set session.cookie_domain. This patch does two things:

1) do not set session.cookie_domain if HTTP_HOST is an IP address

2) do not include port number when setting this value

Without this patch, valid cookies cannot be set for sites running on an alt port or sites being accessed via IP address (so folks like me cannot log in to their test servers :)

CommentFileSizeAuthor
#8 settings_cookie_domain_0.patch900 bytesJohnAlbin
#6 settings_cookie_domain.patch791 bytesJohnAlbin
cookie_domain_1_0.patch1.31 KBHarry Slaughter
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Harry Slaughter’s picture

Harry Slaughter
Location Omaha NE USA
CreditAttribution: Harry Slaughter commented

We will certainly want to optimize the regexes in this settings.php code.

for example, clouseau suggested using something like
/.\d{1,3}$/
instead of
/\d{1,3}.\d{1,4}.\d{1,3}.\d{1,3}/

It's obviously important that settings.php not contain any unnecessary overhead.

Harry Slaughter’s picture

Harry Slaughter
Location Omaha NE USA
CreditAttribution: Harry Slaughter commented

quick follow up, benchmarked the patch and there's no need to optimize anything. 

Existing method with IP address:

Iterations: 1000000
Total time: 38.44199 seconds
  Avg time: 0.00004 seconds
34.034u 4.460s 0:38.49 100.0%   0+0k 0+0io 0pf+0w

Existing method with IP address & port:

Iterations: 1000000
Total time: 37.85396 seconds
  Avg time: 0.00004 seconds
33.534u 4.340s 0:37.90 99.9%    0+0k 0+0io 0pf+0w


Existing method with domain name:

Iterations: 1000000
Total time: 68.41079 seconds
  Avg time: 0.00007 seconds
55.691u 12.768s 1:08.46 99.9%   0+0k 0+0io 0pf+0w

Existing method with domain name & port:

Iterations: 1000000
Total time: 70.54737 seconds
  Avg time: 0.00007 seconds
58.079u 12.520s 1:10.59 100.0%  0+0k 0+0io 0pf+0w
drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Version: 5.x-dev » 6.x-dev

Looks okay, but why only benchmark the existing function and not the new one?

Dries’s picture

Dries CreditAttribution: Dries commented

What exactly does this patch fix? What's wrong with using an IP address and/or port number?

Steven’s picture

Steven CreditAttribution: Steven commented

You cannot set cookie domains to IPs as per the RFC. Same for port numbers. It must be a domain only.

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
FileSize
settings_cookie_domain.patch791 bytes

I noticed one additional problem with the existing cookie_domain code… it doesn’t strip the text “www.” from the front of the HTTP_HOST, it strips the regular expression “www.” from the front of the HTTP_HOST. (Right now, some guy at http://wwwisforsuckas.example.com is weeping because he can't login to his Drupal.)

The attached patch fixes all the 3 issues while incurring very little performance hit, since it simply expands the regex in the existing preg_replace() call.

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
Title: settings.php > cookie_domain modifications » session cookie not set when using IP address or alt port number

Modifying title to better explain issue.

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
FileSize
settings_cookie_domain_0.patch900 bytes

Re-rolled patch against latest 6.x.

Zen’s picture

Zen CreditAttribution: Zen commented

Isn't this being addressed in http://drupal.org/node/56357 as well?

-K

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented

Yes. The patch in #56357 grew in scope to incapsulate a fix for this issue. But since they are separate issues and neither committed, I’m not comfortable with me setting this issue to duplicate until #56357 is in HEAD.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs review » Closed (duplicate)

Let's move this to: http://drupal.org/node/56357 ...